Uh oh!
There was an error while loading. Please reload this page.
fix(cli): report built-in command errors instead of crashing - #363
Merged
Conversation
`runCli` was `createProgram(...).parse()` with no error handling, so anything a built-in command threw escaped: sync throws printed a raw Node stack trace, and async ones surfaced as unhandled rejections. Either way the `exitCode` the error carried was discarded. Five commands hit this in practice: $ webcmd adapter path hackernews/top file:///…/dist/src/cli.js:1635 throw new ArgumentError(`Adapter source is unavailable …`); ^ ArgumentError: Adapter source is unavailable for hackernews/top. at localAdapterPath (file:///…/dist/src/cli.js:1635:19) … Adapter commands never had this problem — execution.ts wraps them and renders the shared error envelope. Use the same envelope here, so built-ins and adapters report failures identically: $ webcmd adapter path hackernews/top ok: false error: code: ARGUMENT message: Adapter source is unavailable for hackernews/top. exitCode: 2 Exit codes now come from the error rather than being lost, which also settles the taxonomy in errors.ts for these paths — `site fixture get` on a missing fixture exits 66 (EMPTY_RESULT) instead of 1, and `session close <bad-id>` exits 2 (USAGE_ERROR). Stacks stay off unless WEBCMD_DEBUG is set. `parse()` -> `parseAsync()` is what lets async rejections reach the handler, and main.ts now awaits runCli. That also keeps the daemon-run signal cancellation installed for the real duration of a run: `parse()` returned as soon as it kicked off an async action, so main.ts's `finally` uninstalled the SIGINT handler while the run was still in flight, and Ctrl-C never cancelled it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
🟠 Maintainer review suggested — low confidenceThe automated review could not reach a fully supported conclusion. Limitations
This review is advisory and does not block merging. |
Doctor rendering was reading ~/.webcmd aliases. Windows git fixtures were hitting the 5s default timeout.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 19, 2026
ankitranjan7 added a commit
that referenced
this pull request
Aug 19, 2026
This was referenced Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finding #1 from the CLI audit. Independent of #361 — branched from
main, no overlapping hunks.The bug
runCliwascreateProgram(...).parse()with no error handling. Anything a built-in command threw escaped: sync throws printed a raw Node stack trace, async ones surfaced as unhandled rejections, and either way theexitCodethe error carried was thrown away.Adapter commands never had this problem —
execution.tswraps them and renders the shared error envelope. Only built-ins were unprotected.Five commands this fixes
adapter path <site>/<cmd>adapter source get <site>/<cmd>adapter source put <a>/<b> <path>site fixture get <site>/<cmd>session close <bad-id>Same envelope adapters already emit, on stderr, so an agent parses built-in and adapter failures the same way. Exit codes now come from the error instead of being lost, which finally applies the taxonomy already declared in
errors.ts—site fixture geton a missing fixture exits66(EMPTY_RESULT), not1.Stacks stay off unless
WEBCMD_DEBUGis set.Reuse, not new machinery
toEnvelope()(errors.ts) andformatErrorEnvelope()(output.ts) already existed and are whatcommanderAdapter.tsuses for adapter errors.reportCliErrorjust wires them to the built-in path; it takes an injectable stream so it is directly testable.Two consequences worth reviewing
parse()→parseAsync(), and main.ts awaits. This is what lets an async rejection reach the handler at all.Signal cancellation now covers the actual run.
parse()returned as soon as it kicked off an async action, so main.ts'sfinally { uninstallSignalCancellation() }tore down the SIGINT handler while the run was still in flight — Ctrl-C during an adapter run never cancelled the daemon run. Awaiting keeps it installed for the real duration. This is a behaviour fix that falls out of the same change; calling it out because it is not obvious from the diff.Verification
npm run typecheck— cleannpm test— 5687 passed, 1 failed:src/doctor.test.ts:218(profile alias rendering), pre-existing onmain, unrelated$?hackernews top,list,--help) — still exit 0src/cli-error-report.test.tscovers the envelope, the hint, theUNKNOWNfallback, and theWEBCMD_DEBUGstack toggleTests went in a new file rather than
cli.test.tsso this branch and #361 stay conflict-free.Not in this PR
adapter pathstill fails for every command that is not already a local override (finding #2) — that is a separate bug inresolveAdapterSourcePath. This PR makes it report the failure properly instead of crashing; it does not make the command work.🤖 Generated with Claude Code