Fix: require a value on the ADK CLI options that always take one - #803
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: require a value on the ADK CLI options that always take one#803AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 8, 2026 04:34
Commander treats `[value]` as an optional argument. When a value-taking flag is the last token, or the next token starts with `-`, commander stores the boolean `true` instead of a string. `adk deploy cloud_run --project` then deploys with the project name `true` and writes `ENV GOOGLE_CLOUD_PROJECT=true` into the generated Dockerfile. Declare the twelve options that always need a value with `<value>` so commander rejects the command. The boolean flags are out of scope.
…ptions Cover the rejection of each valueless flag, the positional agent directory surviving a flag with a value, the empty `--project=` form, and the boolean `--with_ui` staying valueless. The harness calls `exitOverride()` after `createProgram()` has built the subcommands, so it never reaches them. A parse error on `deploy cloud_run` would call `process.exit` and kill the vitest worker. Apply the override recursively in a nested `beforeEach`.
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
No issue. This came out of a CLI audit.
Problem: Commander treats
[value]as an optional argument, soadk deploy cloud_run --projectstores the booleantrueinstead of a string. The action handler types its options asRecord<string, string>, so TypeScript does not catch the boolean.deployToCloudRunthen skips the gcloud-config fallback and writesENV GOOGLE_CLOUD_PROJECT=trueinto the generated Dockerfile. Twelve options have this shape, and none of them has a meaningful "present but valueless" state.Solution: I declared those twelve options with
<value>, so commander rejects the command before the action handler runs. The long names, placeholder words, help text and defaults are unchanged, and every form that supplies a value behaves exactly as before, including--project=. The boolean flags keep[boolean]; #693 handles those, and a new test pins--with_uiso this change cannot leak into them. Commander owns the error, so there is no validation branch and noargParser.Two limits stay as they are.
--project ./agentsstill swallows the directory, and--project --region us-west1now yieldsproject === '--region'instead ofproject === true. Commander consumes the next token either way.Collision check: I listed the 300 open PRs on the fork and read the diffs of the nine that touch
dev/src/cli/cli.ts(#585, #587, #592, #693, #694, #695, #700, #705, #706). None changes a[string],[id]or[dir]option. #693 is the closest neighbour and is disjoint: it converts the[boolean]flags. I branched frommainbecause no open PR is a prerequisite.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-> 44 passed (25 existing, 19 new). No existing test was changed or removed.The existing harness calls
program.exitOverride()aftercreateProgram()has built the subcommands. Commander copies the exit callback at.command()time, so the override never reachesdeploy cloud_run, and a missing-argument error would callprocess.exitand kill the vitest worker. A newapplyExitOverrideRecursivelyhelper, applied in a nestedbeforeEach, fixes that without touching the existing setup.Proof each new test can fail. I mutated
dev/src/cli/cli.tsand reran the file:'--project <string>'->'--project [string]'promise resolved "undefined" instead of rejecting[...]'--with_ui [boolean]'->'<boolean>'(over-application)should still accept --with_ui as the last token:error: option '--with_ui <boolean>' argument missing'--project <string>'->'<string...>'expected { …(16) } to match object { project: 'p', …(1) }'--agent_engine_id <id>'->'<id...>''--tests_dir <dir>'->'--tests_dir'error: too many arguments for 'conformance'Coverage: the change adds no line and no branch to
dev/src/cli.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Run
npm run build, then:node dev/dist/esm/cli_entrypoint.js deploy cloud_run --projectprintserror: option '--project <string>' argument missingand exits 1. I confirmed no temp directory and no Dockerfile were created.node dev/dist/esm/cli_entrypoint.js integration conformance --agents_dirprintserror: option '--agents_dir <dir>' argument missingand exits 1.node dev/dist/esm/cli_entrypoint.js deploy cloud_run --helplists every option with unchanged help text. The twelve now show<...>;--with_ui,--verbose,--compile,--bundleand--a2astill show[boolean].CI:
run-testspasses on ubuntu, macOS and windows. The first windows run failed on two timeouts intests/integration/app_loader/app_loader_test.ts, a file this PR does not touch and which does not use the CLI parser. The job passed on re-run with no code change, so the timeout is a flake; #545 targets it.I also validated locally on the pushed commit:
npx vitest run --project unit:dev dev/test/cli/cli_test.ts-> 44 passed.npm run build-> succeeds.npx eslint dev/src/cli/cli.ts dev/test/cli/cli_test.ts-> clean.npx prettier --check dev/src/cli/cli.ts dev/test/cli/cli_test.ts-> clean.npm run ts:checkreports the same 42 pre-existing failing files before and after this change. None of them is indev/src/cliordev/test/cli.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.