fix: actionable error messages + fire cli.link event on --template#62
Conversation
Network failures from fetch and spawned npx previously surfaced as
"fetch failed" or "Failed to install agent skills" with no root cause.
Added formatFetchError (unwraps undici cause codes: ENOTFOUND, ECONNREFUSED,
ETIMEDOUT, CERT_HAS_EXPIRED, etc.) and describeExecError (classifies
timeout, missing npx, DNS, TLS, 404, EACCES, ENOSPC, npm auth) so users
see what to fix instead of being told to retry.
Also fixes reportCliUsage("cli.link") being silently skipped when
--template is passed: config isn't written to cwd until after the
template subdirectory is created, so the event returned early on a
null config. reportCliUsage now accepts an explicit config, wired
through from both link branches.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughUpdated network fetch error handling with a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ting, gate publish on tests Covers formatFetchError (all cause codes + fallbacks), describeExecError (timeout/ENOENT/stderr classifiers and priority), and reportCliUsage with an explicit config (the path that fixes the --template regression). prepublishOnly now runs test:unit so a broken build can't be published. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
`npm run lint` now runs vitest first, then eslint — so a broken test fails the same command as a style regression. CI and the publish workflow call `npm run lint` before build, so tests gate both paths without an extra step. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/skills.ts (1)
74-83: Consider extracting install commands into constants to prevent drift.The executed
npxcommand and the “run this manually” hint are currently maintained separately; centralizing command strings will keep them in sync.♻️ Suggested refactor
+const INSTALL_AGENT_SKILLS_CMD = + 'npx skills add insforge/agent-skills -y -a antigravity -a augment -a claude-code -a cline -a codex -a cursor -a gemini-cli -a github-copilot -a kilo -a qoder -a qwen-code -a roo -a trae -a windsurf'; +const INSTALL_FIND_SKILLS_CMD = + 'npx skills add https://github.com/vercel-labs/skills --skill find-skills -y'; - await execAsync('npx skills add insforge/agent-skills -y -a antigravity -a augment -a claude-code -a cline -a codex -a cursor -a gemini-cli -a github-copilot -a kilo -a qoder -a qwen-code -a roo -a trae -a windsurf', { + await execAsync(INSTALL_AGENT_SKILLS_CMD, { cwd: process.cwd(), timeout: SKILL_INSTALL_TIMEOUT_MS, }); ... - clack.log.info('Run `npx skills add insforge/agent-skills` once resolved to see the full output.'); + clack.log.info(`Run \`${INSTALL_AGENT_SKILLS_CMD}\` once resolved to see the full output.`); ... - await execAsync('npx skills add https://github.com/vercel-labs/skills --skill find-skills -y', { + await execAsync(INSTALL_FIND_SKILLS_CMD, { cwd: process.cwd(), timeout: SKILL_INSTALL_TIMEOUT_MS, }); ... - clack.log.info('Run `npx skills add https://github.com/vercel-labs/skills --skill find-skills` once resolved.'); + clack.log.info(`Run \`${INSTALL_FIND_SKILLS_CMD}\` once resolved.`);Also applies to: 89-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skills.ts` around lines 74 - 83, Extract the long npx install command into a named constant (e.g., AGENT_SKILLS_INSTALL_CMD) and replace the inline string in the execAsync call inside install logic (the call using execAsync and SKILL_INSTALL_TIMEOUT_MS) with that constant, then reuse the same constant in the user-facing hint passed to clack.log.info (and similarly refactor the second occurrence around lines 89-98) so the executed command and the "run this manually" message always stay in sync; keep describeExecError(err), clack.log.warn and the timeout usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/skills.ts`:
- Around line 74-83: Extract the long npx install command into a named constant
(e.g., AGENT_SKILLS_INSTALL_CMD) and replace the inline string in the execAsync
call inside install logic (the call using execAsync and
SKILL_INSTALL_TIMEOUT_MS) with that constant, then reuse the same constant in
the user-facing hint passed to clack.log.info (and similarly refactor the second
occurrence around lines 89-98) so the executed command and the "run this
manually" message always stay in sync; keep describeExecError(err),
clack.log.warn and the timeout usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2314482-010a-403c-a457-e58cb3b21fd9
📒 Files selected for processing (5)
src/commands/projects/link.tssrc/lib/api/platform.tssrc/lib/auth.tssrc/lib/errors.tssrc/lib/skills.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
14-14: Remove--passWithNoTeststo fail hard when test discovery breaks.The
--passWithNoTestsflag causes Vitest to exit with code 0 when no tests are discovered, allowing the lint command to pass even if test discovery fails. This masks issues in CI/prepublish pipelines. Remove the flag to fail when no tests are found.♻️ Proposed change
- "lint": "vitest run --passWithNoTests && eslint src/", + "lint": "vitest run && eslint src/",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 14, The "lint" npm script in package.json currently includes the Vitest flag --passWithNoTests which masks test discovery failures; remove that flag from the "lint" script (the "lint" script value referencing vitest run --passWithNoTests && eslint src/) so vitest fails hard when no tests are found, leaving the rest of the script unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/skills.ts`:
- Around line 24-31: The ENOENT check in the npx error handling doesn't catch
shell-reported "npx not found" messages; update the logic around the existing
e.code === 'ENOENT' branch and the computed stderr (the stderr variable) to also
test stderr for patterns like /npx: not found/i and /'npx' is not recognized/i
(and similar OS variants), and return the same '`npx` is not on your PATH —
install Node.js 18+ and reopen your shell' message when either e.code ===
'ENOENT' OR stderr matches those patterns so missing-npx cases surfaced by
child_process.exec() are handled.
---
Nitpick comments:
In `@package.json`:
- Line 14: The "lint" npm script in package.json currently includes the Vitest
flag --passWithNoTests which masks test discovery failures; remove that flag
from the "lint" script (the "lint" script value referencing vitest run
--passWithNoTests && eslint src/) so vitest fails hard when no tests are found,
leaving the rest of the script unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e01b2428-0e4c-4ff6-a65a-0e0d3b3be2dc
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/publish.ymlpackage.jsonsrc/lib/errors.test.tssrc/lib/skills.test.tssrc/lib/skills.ts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/publish.yml
| if (e.code === 'ENOENT') { | ||
| return '`npx` is not on your PATH — install Node.js 18+ and reopen your shell'; | ||
| } | ||
|
|
||
| const stderr = (typeof e.stderr === 'string' ? e.stderr : e.stderr?.toString()) ?? ''; | ||
| if (/ENOTFOUND|EAI_AGAIN|getaddrinfo/i.test(stderr)) return 'cannot reach the npm registry (DNS lookup failed) — check your internet connection'; | ||
| if (/ECONNREFUSED/i.test(stderr)) return 'connection to the npm registry was refused — a proxy or firewall is likely blocking it'; | ||
| if (/ETIMEDOUT|ESOCKETTIMEDOUT|network timeout/i.test(stderr)) return 'the npm registry timed out — check your VPN, proxy, or corporate network'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.exec, when a command is missing (e.g., npx not installed), what values are typically returned on the error object (code, signal, stderr) on Linux/macOS and Windows?
💡 Result:
In Node.js child_process.exec, when a command like npx is missing (not found in PATH), the shell (/bin/sh on Linux/macOS, cmd.exe on Windows) fails to execute it and exits with a non-zero exit code. The error object in the callback receives this shell exit code in error.code, null signal (no signal termination), and the "command not found" message in stderr. Linux/macOS (using /bin/sh): - error.code: 127 (standard for command not found in many shells like bash/dash) - error.signal: null - error.stderr: "/bin/sh: 1: npx: not found" Windows (using cmd.exe): - error.code: 1 (cmd.exe standard non-zero for command errors) - error.signal: null - error.stderr: "'npx' is not recognized as an internal or external command, operable program or batch file." This behavior is consistent across Node.js versions per official docs: error.code is the shell's exit code for exec (non-shell spawn uses ENOENT). Examples from StackOverflow confirm code=1 for Windows command failures and code=127 implied for Unix "not found" cases like arp/node/mongodump.
Citations:
- 1: https://stackoverflow.com/questions/58177312/nodejs-child-process-exec-cant-find-command
- 2: https://stackoverflow.com/questions/72064623/nodejs-exec-from-child-process-returns-bin-sh-1-command-not-found-but-works-w
- 3: https://stackoverflow.com/questions/34622560/node-child-process-exec-command-failed-with-error-code-1
- 4: https://nodejs.org/api/child_process.html
- 5: https://nodejs.org/api/child%5Fprocess.html
- 6: https://stackoverflow.com/questions/37732331/execute-bash-command-in-node-js-and-get-exit-code
Add stderr pattern checks to catch real "npx not found" shell errors.
On Line 24–26, the condition e.code === 'ENOENT' alone will not catch missing npx. When using child_process.exec(), a missing command surfaces as a shell exit code (typically 127 on Linux/macOS, 1 on Windows) with stderr like "npx: not found" or "'npx' is not recognized". Without matching these stderr patterns, the specific "install Node.js" message is lost and a generic error displays instead.
Suggested fix
- if (e.code === 'ENOENT') {
- return '`npx` is not on your PATH — install Node.js 18+ and reopen your shell';
- }
-
const stderr = (typeof e.stderr === 'string' ? e.stderr : e.stderr?.toString()) ?? '';
+ if (
+ e.code === 'ENOENT' ||
+ /\bnpx\b.*(not found|not recognized)/i.test(stderr) ||
+ /command not found/i.test(stderr)
+ ) {
+ return '`npx` is not on your PATH — install Node.js 18+ and reopen your shell';
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (e.code === 'ENOENT') { | |
| return '`npx` is not on your PATH — install Node.js 18+ and reopen your shell'; | |
| } | |
| const stderr = (typeof e.stderr === 'string' ? e.stderr : e.stderr?.toString()) ?? ''; | |
| if (/ENOTFOUND|EAI_AGAIN|getaddrinfo/i.test(stderr)) return 'cannot reach the npm registry (DNS lookup failed) — check your internet connection'; | |
| if (/ECONNREFUSED/i.test(stderr)) return 'connection to the npm registry was refused — a proxy or firewall is likely blocking it'; | |
| if (/ETIMEDOUT|ESOCKETTIMEDOUT|network timeout/i.test(stderr)) return 'the npm registry timed out — check your VPN, proxy, or corporate network'; | |
| const stderr = (typeof e.stderr === 'string' ? e.stderr : e.stderr?.toString()) ?? ''; | |
| if ( | |
| e.code === 'ENOENT' || | |
| /\bnpx\b.*(not found|not recognized)/i.test(stderr) || | |
| /command not found/i.test(stderr) | |
| ) { | |
| return '`npx` is not on your PATH — install Node.js 18+ and reopen your shell'; | |
| } | |
| if (/ENOTFOUND|EAI_AGAIN|getaddrinfo/i.test(stderr)) return 'cannot reach the npm registry (DNS lookup failed) — check your internet connection'; | |
| if (/ECONNREFUSED/i.test(stderr)) return 'connection to the npm registry was refused — a proxy or firewall is likely blocking it'; | |
| if (/ETIMEDOUT|ESOCKETTIMEDOUT|network timeout/i.test(stderr)) return 'the npm registry timed out — check your VPN, proxy, or corporate network'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/skills.ts` around lines 24 - 31, The ENOENT check in the npx error
handling doesn't catch shell-reported "npx not found" messages; update the logic
around the existing e.code === 'ENOENT' branch and the computed stderr (the
stderr variable) to also test stderr for patterns like /npx: not found/i and
/'npx' is not recognized/i (and similar OS variants), and return the same '`npx`
is not on your PATH — install Node.js 18+ and reopen your shell' message when
either e.code === 'ENOENT' OR stderr matches those patterns so missing-npx cases
surfaced by child_process.exec() are handled.
2 participants
Summary
fetch failed/ generic catch-alls now get unwrapped.formatFetchErrorreads the undicicause.code(ENOTFOUND, ECONNREFUSED, ETIMEDOUT, CERT_HAS_EXPIRED, etc.) and returns a sentence the user can act on. Wired intoplatformFetchand the OAuth token endpoints inauth.ts.describeExecErrorinskills.tsclassifiesexecAsyncfailures (timeout, missingnpx, DNS, TLS, 404, EACCES, ENOSPC, npm auth). Warning now explains the real reason and tells the user to run the manual command once resolved — not a useless "try again".cli.linkon--template. Regression: when--templateis passed,saveProjectConfigis deferred until the template subdirectory is created, soreportCliUsage('cli.link')ran before config existed in cwd and returned early with no event.reportCliUsagenow accepts an explicit config; both link branches pass it through.Test plan
insforge link --template reactfrom an empty dir — confirm the link event reaches the project (previously missing).networksetup -setairportpower en0 offor turn off Wi-Fi) and runinsforge link— should see a DNS-failure message, not "fetch failed".insforge createorlink --template— "Could not install agent skills: cannot reach the npm registry …" instead of generic warn.insforge linkon a known-good project (no template) — unchanged behavior,cli.linkstill fires.🤖 Generated with Claude Code
Note
Add actionable network error messages and fire
cli.linkevent on--templateformatFetchErrorinsrc/lib/errors.tsto map common network/TLS error codes to user-facing messages; used inplatformFetchand the OAuth token exchange/refresh flows.describeExecErrorinsrc/lib/skills.tsto classify npx/npm exec failures (timeout, DNS, TLS, 404, auth, disk full, etc.) into actionable log messages during skill installation.reportCliUsageto accept an optional explicit config, allowingcli.linkto be fired from the--templatepath without reading project config from disk.lintscript sonpm run lintexecutes Vitest before ESLint.Macroscope summarized 48a70dd.
Summary by CodeRabbit