Skip to content

Fix: restore 'adk help' and 'adk help <command>' at the CLI root - #816

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-root-help-command
Open

Fix: restore 'adk help' and 'adk help <command>' at the CLI root#816
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-root-help-command

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

No existing issue.

  1. Or, if no issue exists, describe the change:

Problem: adk help and adk help web fail with error: too many arguments. The root program attaches an action handler so that a bare adk -v prints the version. Commander only registers its implicit help [command] subcommand when a command has subcommands and no action handler (commander/lib/command.js:463-468), so the root lost help while the nested deploy and integration groups kept it. The root usage block does not list help either, so the gap is also undiscoverable.

Solution: createProgram() now calls program.helpCommand(true). That sets the flag _getHelpCommand() short-circuits on (commander/lib/command.js:409-417), so the implicit help command is registered despite the action handler. The change is additive: adk --help, adk web --help and adk -v are unchanged, and the root usage block gains one help [command] line. A comment on the new statement records why it is needed, because commander adds help by default everywhere else in this file.

Collision check: I listed the open pull requests on the fork and read the diffs of the four that touch dev/src/cli/cli.ts. None of them calls helpCommand. #597 and #706 rewrite the root .action() body; this statement sits outside that body, so it does not conflict with either.

Before, on main:

$ node dev/dist/esm/cli_entrypoint.js help
error: too many arguments. Expected 0 arguments but got 1.
$ node dev/dist/esm/cli_entrypoint.js help web
error: too many arguments. Expected 0 arguments but got 2.

After:

$ node dev/dist/esm/cli_entrypoint.js help
Usage: ADK CLI [options] [command]

Options:
  -v, --version                      Get ADK CLI version
  -h, --help                         display help for command

Commands:
  web [options] [agents_dir]         Start ADK web server
  api_server [options] [agents_dir]  Start ADK API server
  create [options] [agent]           Creates a new agent
  run [options] <agent>              Runs agent
  deploy                             Deploy agent
  integration                        Run ADK integration and conformance tests
  help [command]                     display help for command

$ node dev/dist/esm/cli_entrypoint.js help web
Usage: ADK CLI web [options] [agents_dir]

Start ADK web server

Arguments:
  agents_dir                       Agent file or directory of agents to serve.
                                   ...
Options:
  -h, --host <string>              Optional. The binding host of the server
  ...
  --session_service_uri <string>   Optional. The URI of the session service.
  ...

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:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

I added one describe('command: help') block with four tests to dev/test/cli/cli_test.ts. No existing test was changed.

npx vitest run --project unit:dev dev/test/cli/cli_test.ts
Test Files  1 passed (1)
     Tests  30 passed (30)

Two hazards the new block handles. Commander copies the exit callback into a subcommand when the subcommand is created, so the shared exitOverride() in beforeEach does not reach the subcommands, and adk help web would call the real process.exit and kill the vitest worker. The new exitOverrideDeep helper walks the command tree. The shared parse helper also only swallows commander.exit, so the block uses its own runner that returns the CommanderError code and asserts on it.

Mutation test 1 (proves the three help tests fail without the fix). I commented out program.helpCommand(true) and re-ran the block:

× command: help > should print the root usage block for 'adk help'
  → expected 'commander.excessArguments' to be 'commander.help'
× command: help > should print the web usage block for 'adk help web'
  → expected 'commander.excessArguments' to be 'commander.help'
× command: help > should print the deploy group usage block for 'adk help deploy'
  → expected 'commander.excessArguments' to be 'commander.help'

Mutation test 2 (proves the fourth test, the <command> --help regression guard, can fail). That test pins behaviour the fix must not change, so the first mutation leaves it green by design. I changed the web description to 'MUTATED description' instead:

× command: help > should keep '<command> --help' working
× command: help > should print the web usage block for 'adk help web'
Tests  2 failed | 28 passed (30)

Coverage. The production change is one statement with no branches. A v8 run scoped to dev/src/cli/cli.ts reports 30 hits on that line, so new line and branch coverage is 100%. The file total stays at its pre-existing 95.30% lines / 73.17% branches; the uncovered lines are other command handlers this change does not touch.

Type check. npm run ts:check reports 287 errors on main and 287 errors with this branch applied, none of them in dev/src/cli/cli.ts or dev/test/cli/cli_test.ts. The failures are pre-existing and unrelated. npx eslint and npx prettier --check are clean on both changed files.

Manual End-to-End (E2E) Tests:

npm install && npm run build
node dev/dist/esm/cli_entrypoint.js help          # root usage, lists `help [command]`, exit 0
node dev/dist/esm/cli_entrypoint.js help web      # web usage block, exit 0
node dev/dist/esm/cli_entrypoint.js help deploy   # deploy group usage, exit 0
node dev/dist/esm/cli_entrypoint.js web --help    # unchanged, exit 0
node dev/dist/esm/cli_entrypoint.js --help        # unchanged, exit 0
node dev/dist/esm/cli_entrypoint.js -v            # prints the version only, exit 0

All six exit 0. The first three fail on main.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

The root program sets an action handler so that a bare `adk -v` prints the
version. Commander only registers its implicit `help [command]` subcommand
when a command has subcommands AND no action handler, so the root lost
`help` while the nested `deploy` and `integration` groups kept it.

Call `program.helpCommand(true)` to re-enable it, and pin the behaviour with
four new tests in `dev/test/cli/cli_test.ts`.
The structural guard accepted any object carrying a string `code`, so a
stray Node error such as `ENOENT` would have passed it and the test would
have asserted on the wrong error. `commander` exports `CommanderError`, so
use it directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant