Repository navigation
fix(cli): report remaining command argument errors without stack traces - #495
KrasimirKralev wants to merge 2 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b098488399
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| if (parsed.dryRun && !parsed.from) { | ||
| throw new Error('--dry-run currently requires --from <artifact>.'); | ||
| throw new CliUsageError('--dry-run currently requires --from <artifact>.'); |
There was a problem hiding this comment.
Classify all generate-cli usage failures
When generate-cli is invoked with no target, with --from plus --name/--command, or with both tool-selection flags, this handler still reaches the plain Error branches at lines 17–18 and 59–62 or the plain error in validateToolSelection, so the top-level handler still prints the duplicate message and stack this patch is meant to remove. This CliUsageError therefore fixes only the --dry-run branch for this command; convert the other user-input validation branches and cover them in the integration table as well.
Useful? React with 👍 / 👎.
|
Codex review: needs maintainer review before merge. What this changesClassifies remaining CLI argument-validation failures as usage errors so they print once without internal stack traces. Example: Run
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open for merge: it fixes argument errors still present on main, and the final diff has no actionable findings. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherCommand parsers validate CLI arguments and pass failures to the existing top-level handler, which renders CliUsageError messages without stacks. flowchart LR
A[CLI arguments] --> B[Command parsers]
B --> C[CliUsageError]
C --> D[Top level handler]
D --> E[One diagnostic and exit 1]
B --> F[Valid command execution]
Technical reviewBest possible solution: Use the existing CliUsageError rendering path for these argument failures, with the focused CLI regression coverage supplied here. Do we have a high-confidence way to reproduce the issue? Pinned main source shows plain validation errors reaching the stack-printing handler, corroborated by contributor before/after CLI transcripts; this read-only review ran no target code. Is this the best way to solve the issue? The patch fixes classification at the existing validation sites and reuses the shared renderer without adding a competing error-handling path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against ce3e6d058a0f. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Preflight sourceSource unknown. Source comparison at the observed PR snapshot only. Completion is self-reported; body, policy, tests, execution, and report bytes are not verified. Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed November 5, 2026, 4:00 AM ET / 09:00 UTC. |
ea68568 to
da9fed9
Compare
Related: #482, #484
What Problem This Solves
Fixes: a mistyped argument to
resource,inspect-cli,auth,list,serve,record/replayorgenerate-cli(or a bare global--config) prints the message twice, followed by an internal stack trace.emit-ts,daemonand--timeoutparsers toCliUsageError, which the top-level handler prints once without a stack. The other command parsers still threw plainError.Failed to authorize,nothing to serve, daemon fast-path routing, persistence-file and artifact-content errors) stay plainErrorwith their stack.callargument parsing, last touched in fix(cli): preserve arguments and safe generated command behavior #484, is not changed here.CliUsageErrorextendsError, so existing callers that catch these errors behave the same.User Impact
User impact: a typo on these commands now prints one
[mcporter] …line saying what to fix, the same asemit-ts,daemon,configandvaultalready do, instead of a duplicate message and a stack trace pointing intodist/.Why This Change Was Made
The argument-parser
throw new Error(...)sites in 18src/clifiles now throwCliUsageError(src/cli/errors.ts), the typesrc/cli.tsalready prints without a stack. The existingoutput-format,record/replayandflag-utilsvalidation tables now also assertCliUsageError, and a new built-CLI table (tests/cli-usage-errors.integration.test.ts) runsnode dist/cli.jsfor one bad argument per touched command and asserts exit 1 and a single[mcporter] <message>line on stderr. Against the unchangedsrc/those three files fail 25 of 38 cases (every new assertion); with the change they pass, and a follow-up commit adds three moregenerate-clirows (no target,--fromwith--name, both tool-selection flags) for branches the first pass missed.pnpm checkpasses. In the fullpnpm test, the only failures are 8 Bun native-build cases that stop atBun 1.4.4 or newer is required for native builds; found 1.4.0(this machine's Bun); that version check is not touched here. CHANGELOG line added under[0.16.2] - Unreleased.Evidence
pnpm build, isolatedHOME, empty working directory.node dist/cli.js <args>for each command below, onmain(ce3e6d0) and on this branch.node dist/cli.jswith each bad argument prints the same message once, with no stack trace, and still exits 1.mainstack traces are shortened to….