Skip to content

fix(cli): report remaining command argument errors without stack traces - #495

Open
KrasimirKralev wants to merge 2 commits into
openclaw:mainfrom
KrasimirKralev:fix/remaining-usage-errors
Open

KrasimirKralev wants to merge 2 commits into
openclaw:mainfrom
KrasimirKralev:fix/remaining-usage-errors

Conversation

@KrasimirKralev

@KrasimirKralev KrasimirKralev commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Related: #482, #484

What Problem This Solves

Fixes: a mistyped argument to resource, inspect-cli, auth, list, serve, record/replay or generate-cli (or a bare global --config) prints the message twice, followed by an internal stack trace.

User Impact

User impact: a typo on these commands now prints one [mcporter] … line saying what to fix, the same as emit-ts, daemon, config and vault already do, instead of a duplicate message and a stack trace pointing into dist/.

Why This Change Was Made

The argument-parser throw new Error(...) sites in 18 src/cli files now throw CliUsageError (src/cli/errors.ts), the type src/cli.ts already prints without a stack. The existing output-format, record/replay and flag-utils validation tables now also assert CliUsageError, and a new built-CLI table (tests/cli-usage-errors.integration.test.ts) runs node dist/cli.js for one bad argument per touched command and asserts exit 1 and a single [mcporter] <message> line on stderr. Against the unchanged src/ those three files fail 25 of 38 cases (every new assertion); with the change they pass, and a follow-up commit adds three more generate-cli rows (no target, --from with --name, both tool-selection flags) for branches the first pass missed. pnpm check passes. In the full pnpm test, the only failures are 8 Bun native-build cases that stop at Bun 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

  • Behavior or issue addressed: command-line usage errors print once without a stack trace.
  • Real environment tested: Linux (Ubuntu 26.04), Node 24, mcporter built from source with pnpm build, isolated HOME, empty working directory.
  • Exact steps or command run after this patch: node dist/cli.js <args> for each command below, on main (ce3e6d0) and on this branch.
  • Evidence after fix:
# main (ce3e6d0)
$ node dist/cli.js resource
[mcporter] Missing server name. Usage: mcporter resource <server> [uri]
Error: Missing server name. Usage: mcporter resource <server> [uri]
    at handleResource (file:///…/dist/cli/resource-command.js:16:15)
    at runCli (file:///…/dist/cli.js:306:19)
    at async main (file:///…/dist/cli.js:379:5)
exit=1
$ node dist/cli.js serve --http
[mcporter] Flag '--http' requires a port.
Error: Flag '--http' requires a port.
    at parseServeArgs (file:///…/dist/cli/serve-command.js:113:23)
    at handleServeCli (file:///…/dist/cli/serve-command.js:8:20)
    at runCli (file:///…/dist/cli.js:165:15)
    at async main (file:///…/dist/cli.js:379:5)
exit=1
$ node dist/cli.js list --output bogus
[mcporter] --output format must be one of: auto, text, markdown, json, raw.
Error: --output format must be one of: auto, text, markdown, json, raw.
    at consumeOutputFormat (file:///…/dist/cli/output-format.js:19:23)
    at extractListFlags (file:///…/dist/cli/list-flags.js:16:20)
    at handleList (file:///…/dist/cli/list-command.js:18:19)
    at runCli (file:///…/dist/cli.js:262:19)
    at async main (file:///…/dist/cli.js:379:5)
exit=1
(inspect-cli and auth --browser x printed the same duplicate + stack shape)

# this branch
$ node dist/cli.js resource
[mcporter] Missing server name. Usage: mcporter resource <server> [uri]
exit=1
$ node dist/cli.js inspect-cli
[mcporter] Usage: mcporter inspect-cli <artifact> [--json]
exit=1
$ node dist/cli.js auth --browser x foo
[mcporter] --browser must be 'none' when provided to mcporter auth.
exit=1
$ node dist/cli.js list --output bogus
[mcporter] --output format must be one of: auto, text, markdown, json, raw.
exit=1
$ node dist/cli.js list --brief
[mcporter] --brief requires a server target.
exit=1
$ node dist/cli.js list --stdio
[mcporter] Flag '--stdio' requires a value.
exit=1
$ node dist/cli.js list --http-url ftp://example.com
[mcporter] Unsupported protocol 'ftp:' for --http-url.
exit=1
$ node dist/cli.js serve --http
[mcporter] Flag '--http' requires a port.
exit=1
$ node dist/cli.js record
[mcporter] Usage: mcporter record <session-name> [--server <name>] [-- <command-to-run>]
exit=1
$ node dist/cli.js generate-cli --bundler x
[mcporter] --bundler must be 'rolldown' or 'bun'.
exit=1
$ node dist/cli.js generate-cli --runtime x
[mcporter] --runtime must be 'node' or 'bun'.
exit=1
$ node dist/cli.js generate-cli
[mcporter] Provide --server with a definition or a command we can infer a name from (use --name to override).
exit=1
$ node dist/cli.js generate-cli --from x.js --name y
[mcporter] --from cannot be combined with --command/--description/--name.
exit=1
$ node dist/cli.js generate-cli --include-tools a --exclude-tools b
[mcporter] --include-tools and --exclude-tools cannot be used together.
exit=1
$ node dist/cli.js --config
[mcporter] Flag '--config' requires a value.
exit=1
  • Observed result after fix: running node dist/cli.js with each bad argument prints the same message once, with no stack trace, and still exits 1.
  • What was not tested: Windows and macOS; the Bun native-build paths (local Bun is older than the repo's minimum).
  • Proof limitations or environment constraints: absolute paths in the main stack traces are shortened to ….

@clawsweeper

clawsweeper Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-05T08:41:06.681539Z b098488 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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>.');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. labels Nov 5, 2026
@clawsweeper

clawsweeper Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

Classifies remaining CLI argument-validation failures as usage errors so they print once without internal stack traces.

Example: Run mcporter serve --http without a port.

  • Before: Prints the missing-port message twice and an internal stack trace, then exits 1.
  • After: Prints [mcporter] Flag '--http' requires a port. once, then exits 1.

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) Focused repair with shipped-CLI proof, meaningful regression coverage, passing current-head checks, and no actionable findings.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): Contributor terminal output exercises node dist/cli.js on Ubuntu 26.04 with Node 24 and an isolated home across the affected parser families, including final-head generate-cli cases, showing one diagnostic without a stack and exit 1.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Mistyped command arguments produce duplicate diagnostics and internal stack traces.
Reason: Restores the existing usage-error convention across additional parsers and fits VISION.md guidance for clear, small CLI UX fixes.

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
Reviewed head: ea68568c4cac7b97e221afcfa0f922c93fa0931b

Before merge

None.

Findings

None.

Agent review details

How this fits together

Command 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]
Loading

Technical review

Best 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

  • Shared error rendering and flag/output validation keeps the original intent (fada75e: Introduced CliUsageError for expected user-input failures; later command extraction and output-format additions retained ordinary Error at these sites.)
  • Ad-hoc server and auth argument validation keeps the original intent (63f8dd6: Ad-hoc server support established transport and command validation; later auth and inline-flag changes preserved those checks.)
  • Generator validation and artifact output paths keeps the original intent (refactor: dedupe and modularize runtime, OAuth, and CLI internals #249: The refactor centralized generator helpers without changing public behavior; the later bundle-path repair in fix(cli): preserve arguments and safe generated command behavior #484 established explicit-path validation preserved here.)
  • Serve argument validation keeps the original intent (feat: expose daemon-managed MCP servers via serve #172: The serve bridge introduced port, transport, host, and server-selection checks; this PR retains their conditions and messages.)
  • Record/replay argument validation keeps the original intent (fix(replay): rewrite response ids during replay #192: Record/replay repair preserved child-command flags and validated wrapper arguments; this PR changes only the validation error class.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

What I checked:

  • Current main still contains the defect: The resource parser throws plain Error for missing arguments; the top-level handler includes the error object for ordinary errors but prints only the message for CliUsageError. (src/cli/resource-command.ts:18, ce3e6d058a0f)
  • Existing rendering owner: The unchanged top-level CliUsageError branch logs the message once and exits 1; the patch reuses that owner. (src/cli.ts:444, ce3e6d058a0f)
  • Exact introduced change: Production hunks replace Error constructors with CliUsageError and add imports; validation conditions, diagnostic text, and successful parse results are preserved. (ea68568c4cac)
  • Shipped CLI proof: The full PR body supplies before/after terminal output from Ubuntu 26.04 and Node 24 using node dist/cli.js with an isolated home, including the final generate-cli cases; after-fix commands print one diagnostic and exit 1. Captured body source identity: 7c6c521148b924a4a1e28bf4d4b75fa7afb4630842a8973ab7533c70b728ffe3. (ea68568c4cac)
  • Earlier review addressed: The earlier generate-cli finding is addressed by the second commit and three additional integration rows covering no target, incompatible --from options, and mutually exclusive tool selections. (tests/cli-usage-errors.integration.test.ts:37, ea68568c4cac)
  • Current-head validation: Linux Node 24/26, Windows, macOS, CodeQL, and security checks succeed on the reviewed head; the cancelled dispatch is an automation job, not a failing build or test. (.github/workflows/ci.yml, ea68568c4cac)

Likely related people:

  • Peter Steinberger: Raw commit fada75e adds src/cli/errors.ts:1 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: fada75e6cfaf; files: src/cli/errors.ts)

Preflight source

Source 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

Metric Value Why it matters
Change balance Production +87/-68; tests +73; changelog +2 Production changes consist of error-type substitutions and imports; test growth covers the user-visible diagnostic contract.
CLI regression scenarios 15 The integration table checks exit 1 and a single diagnostic through the built CLI across the affected command families.

Labels

Label changes:

  • add P3: Improves diagnostics for invalid CLI input while preserving validation and exit status.
  • add proof: sufficient: Contributor real behavior proof is sufficient.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: Improves diagnostics for invalid CLI input while preserving validation and exit status.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • proof: sufficient: Contributor real behavior proof is sufficient.

Rating scale

6/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.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed November 5, 2026, 4:00 AM ET / 09:00 UTC.

@KrasimirKralev
KrasimirKralev force-pushed the fix/remaining-usage-errors branch from ea68568 to da9fed9 Compare November 6, 2026 11:38

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant