Fix: name the unknown subcommand instead of reporting too many arguments (stacked on #597) - #706
Open
AmaadMartin wants to merge 3 commits into
Open
Conversation
added 3 commits
August 6, 2026 01:15
The root command carries an action handler, so commander never reaches its unknownCommand() branch and answers a typo with an argument count instead. Guard the root action on its leftover operands and raise commander's own unknown-command error. The one pre-existing assertion that pinned commander.excessArguments for `adk bogus` encoded the behaviour this change fixes, so it now pins commander.unknownCommand.
… guard Cover both branches of the new root guard, and pin that allowExcessArguments(true) stays after the subcommand registrations so the subcommands keep rejecting their own excess arguments.
An unchecked cast reported any unrelated throw as a commander error with an undefined code, which hid the real stack. Unexpected errors now rethrow.
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 free
to 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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem:
adk bogusanswers witherror: too many arguments. Expected 0 arguments but got 1., which never names the word the user mistyped. The root command carries an action handler, so commander skips itsunknownCommand()branch and falls through to the operand count check.adk deploy bogusalready printserror: unknown command 'bogus'because thedeploygroup has no action handler.Solution: The root action now reads its leftover operands and raises commander's own unknown-command error for the first one.
program.allowExcessArguments(true)lets the operand reach the action; it is set after every.command()call becauseCommand.command()copies that setting into new subcommands. A variadic root argument would also work, but it adds[command...]to the usage line and anArguments:section toadk --help.Behaviour before and after:
adk boguserror: too many arguments. Expected 0 arguments but got 1.error: unknown command 'bogus'adk bogus extraerror: too many arguments. Expected 0 arguments but got 2.error: unknown command 'bogus'adk deploy boguserror: unknown command 'bogus'adk web a b cerror: too many arguments for 'web'.adk --nopeerror: unknown option '--nope'Exit code stays 1 on every error path.
Four notes for the reviewer:
Command.prototype.unknownCommand()emits this exact string, but it is absent fromcommander/typings/index.d.tsandsuggestSimilaris not exported, so reaching either needs a cast. One consequence: the root has no suggestion hint.adk depoyprintserror: unknown command 'depoy'whileadk deploy cloud_rustill adds(Did you mean cloud_run?). Re-implementing the suggestion is out of scope.commander.excessArgumentsforadk bogus— the behaviour this change fixes. I checked all 597 open PRs on the fork; none implements this fix, so I stacked rather than branching frommain. This PR targetsfix/adk-js-bare-invocation-usage.err.codein'should exit non-zero for an unknown subcommand'now readscommander.unknownCommand. That assertion came in with Fix: print usage for a bare adk invocation instead of only the version #597 and encoded the old message, so it could not survive this fix. Its other assertions (exitCode === 1, empty stdout) are untouched, and the flip is isolated in commit 1.console.log(version)is pre-existing. I did not add it and did not refactor it.adk helpandadk help webdo not work today: commander suppresses its implicithelpsubcommand when the root has an action handler. They now reporterror: unknown command 'help'instead of an argument count. Restoring a workinghelpcommand is separate work and is not in this PR.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
npx vitest run --project unit:dev dev/test/cli/cli_test.ts— 31 passed.A new
describe('unknown subcommand')block adds four cases. It appliesexitOverride()andconfigureOutput()recursively overprogram.commands, because the file's outerbeforeEachsets them on the root only and a subcommand error would otherwise call the realprocess.exit(1).adk bogusnames the command, throwscommander.unknownCommandwithexitCode1, and does not print the version.adk bogus extrareportsbogusand notextra.adkstays on the non-error path.adk create x ystill throwscommander.excessArguments— the regression test for whereallowExcessArguments(true)sits.Each test was run against mutated source and observed to fail:
expected commander to reject an unknown subcommandprogram.allowExcessArguments(true)expected 'error: too many arguments. Expected 0…' to contain 'error: unknown command \'bogus\''program.allowExcessArguments(true)aboveprogram.command('web')expected commander to reject excess subcommand arguments{code: 'commander.unknownCommand'}expected 'commander.error' to be 'commander.unknownCommand'expected CommanderError: error: unknown command 'u… to be undefinedCoverage of the added lines is 100% line and 100% branch, measured with
--coverage.include='dev/src/cli/cli.ts'. The file total is 95.42% line / 75.55% branch; every uncovered range is a pre-existingcatchblock in the deploy and integration actions.npm run ts:checkreports 281 errors on the base commit and 281 with this change, none in the two files touched here.npx eslintandnpx prettier --checkare clean on both files.CI does not run on this PR.
.github/workflows/validation.yamltriggers onpull_request: branches: [main], and this PR targetsfix/adk-js-bare-invocation-usage. Onlyauto-assignran. I ran the workflow's own steps locally on the pushed commit857457d9395be8eef8f94b3419aede6f48c83413:npm run buildnpx vitest run --project unit:dev dev/test/cli/cli_test.tsnpm run lintnpm run format:checknpm run docs:checknpx secretlint "dev/**/*.ts"Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Transcript:
adk web a b cis the leak check for theallowExcessArguments(true)placement.adk --helpshows noArguments:section and an unchanged usage line.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.