fix: improve ax error handling around missing commands or unknown flags#8116
fix: improve ax error handling around missing commands or unknown flags#8116
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds centralized handling for Commander option-related errors and makes command-name optional. BaseCommand.createCommand now accepts an optional name and wraps registered actions to set Commander’s exitOverride at action-registration time. An exitOverride handler checks for option errors via new utilities ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
📊 Benchmark resultsComparing with 3cefa0e
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/base-command.ts (1)
68-69: Extract the Commander option-error policy into one shared helper.The handled code list and the
log()/outputHelp()/log()branch now live in both this file andsrc/commands/main.ts. Centralizing that logic will keep root and subcommand behavior aligned and lets the flow stay self-explanatory without the new block comments.As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Never write comments on what the code does; make the code clean and self-explanatory instead.Also applies to: 283-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/base-command.ts` around lines 68 - 69, Extract the Commander option-error handling into a shared helper instead of duplicating OPTION_ERROR_CODES and the log()/outputHelp()/log() branch; create a single exported function (e.g., handleCommanderOptionError or isCommanderOptionErrorPolicy) that encapsulates the OPTION_ERROR_CODES array and the logic that logs the error, calls outputHelp(), and logs again, then replace the duplicated code in both base-command.ts (where OPTION_ERROR_CODES is declared) and main.ts with calls to this new helper so both root and subcommand flows use the same implementation.
🤖 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/commands/main.ts`:
- Around line 188-193: The help output in non-interactive error branches is
being written to stdout; change the calls to outputHelp() to route to stderr by
calling outputHelp({ error: true }) in the non-interactive/error paths —
specifically replace command.outputHelp() in the non-interactive branch (where
isInteractive() is false) and the this.outputHelp() calls referenced in the
other error branches (methods containing this.outputHelp()) so they pass the {
error: true } option to ensure help text goes to stderr.
In `@tests/integration/commands/error-handling/error-handling.test.ts`:
- Around line 7-19: The test currently calls
callCli(['status','--invalid-option']) twice and only inspects the stderr from
the second run; change the test (and the similar cases in lines 22-48) to call
callCli once and capture the thrown ExecaError (e.g., assign the rejected value
to a variable via await expect(...).rejects.toThrow() replacement pattern or
try/catch) and then assert both the brief error banner and that the same error
object's stderr contains the full USAGE/OPTIONS help block (use normalize to
compare), referencing the existing callCli and normalize helpers and the
'unknown option shows error message' test so all assertions are made against the
single captured error instance.
---
Nitpick comments:
In `@src/commands/base-command.ts`:
- Around line 68-69: Extract the Commander option-error handling into a shared
helper instead of duplicating OPTION_ERROR_CODES and the
log()/outputHelp()/log() branch; create a single exported function (e.g.,
handleCommanderOptionError or isCommanderOptionErrorPolicy) that encapsulates
the OPTION_ERROR_CODES array and the logic that logs the error, calls
outputHelp(), and logs again, then replace the duplicated code in both
base-command.ts (where OPTION_ERROR_CODES is declared) and main.ts with calls to
this new helper so both root and subcommand flows use the same implementation.
🪄 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: e86b4a59-ef43-43d6-8cab-53e52a70169b
📒 Files selected for processing (4)
src/commands/base-command.tssrc/commands/main.tssrc/utils/run-program.tstests/integration/commands/error-handling/error-handling.test.ts
| test('unknown option shows error message', async (t) => { | ||
| await expect(callCli(['status', '--invalid-option'])).rejects.toThrow() | ||
|
|
||
| try { | ||
| await callCli(['status', '--invalid-option']) | ||
| } catch (error) { | ||
| const stderr = (error as { stderr: string }).stderr | ||
| const normalized = normalize(stderr) | ||
|
|
||
| // In interactive mode (test environment), shows brief error | ||
| t.expect(normalized).toContain('Error: unknown option') | ||
| t.expect(normalized).toContain('See more help with --help') | ||
| } |
There was a problem hiding this comment.
Assert the new help-on-error contract from the same failed run.
These cases only check the generic error banner and then rerun the CLI, so a regression where the extra help text disappears or lands on the wrong stream would still pass. Capture the rejected ExecaError once and assert the USAGE/OPTIONS block from that same failure.
Also applies to: 22-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/commands/error-handling/error-handling.test.ts` around
lines 7 - 19, The test currently calls callCli(['status','--invalid-option'])
twice and only inspects the stderr from the second run; change the test (and the
similar cases in lines 22-48) to call callCli once and capture the thrown
ExecaError (e.g., assign the rejected value to a variable via await
expect(...).rejects.toThrow() replacement pattern or try/catch) and then assert
both the brief error banner and that the same error object's stderr contains the
full USAGE/OPTIONS help block (use normalize to compare), referencing the
existing callCli and normalize helpers and the 'unknown option shows error
message' test so all assertions are made against the single captured error
instance.
ndhoule
left a comment
ndhoule
left a comment
There was a problem hiding this comment.
I agree with both of the Coderabbit comments, but otherwise looks good.
src/commands/base-command.ts
Outdated
| const HELP_SEPARATOR_WIDTH = 5 | ||
|
|
||
| /** Commander error codes for option-related errors */ | ||
| const OPTION_ERROR_CODES = ['commander.unknownOption', 'commander.missingArgument', 'commander.excessArguments'] |
There was a problem hiding this comment.
nit: Making this a Set would be neat.
3 participants
🎉 Thanks for submitting a pull request! 🎉
Summary
Today:
With this change:
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)