Fix: forward every unrecognised gcloud flag from adk deploy cloud_run - #805
Open
AmaadMartin wants to merge 4 commits into
Open
Fix: forward every unrecognised gcloud flag from adk deploy cloud_run#805AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 4 commits
August 8, 2026 05:00
The parse() helper set process.argv without the [node, script] prefix that it passed to parseAsync. The two-slot skew cancels the two real tokens, so the harness cannot reproduce the argv offsets the CLI sees in production. It also leaked a mutated global process.argv into later tests. All 26 existing tests still pass unchanged.
The cloud_run action built the pass-through set by slicing raw process.argv at a hardcoded index. That index only lines up when the user supplies the optional [agents_dir], so `adk deploy cloud_run --no-allow-unauthenticated --min-instances=2` dropped the first flag and deployed a public service. Commander also bound [agents_dir] to that same first flag, because it concatenates unrecognised options with the operands before it assigns the positionals. The action now splits Command.args at the first option-shaped token: the prefix is the operands, the suffix is what gcloud gets. [agents_dir] falls back to its documented default unless a real operand precedes the flags. --help now documents the ordering and the `--` escape hatch, as adk-python does.
Four cases pin the contract: an unknown flag in the first slot, unknown flags after an agent directory, a space-separated unknown flag and its value, and the -- separator. Cases 1, 3 and 4 fail against the old process.argv.slice(5) scan.
Review feedback. The helper had one caller and its operands field was read only for emptiness, so the index it already computes says the same thing in three lines. The test harness also no longer sets process.argv: nothing under test reads it now, and leaving the assignment kept main's unrestored clobber of the global.
7 tasks
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 deploy cloud_runbuilt its gcloud pass-through set by slicingprocess.argvat a fixed index. That index is only correct when the user passes the optional[agents_dir], so the documented "run it from inside the agent directory" form dropped the first flag.adk deploy cloud_run --no-allow-unauthenticated --min-instances=2therefore deployed a publicly invocable service, the opposite of what the user asked for. Commander compounded it: it concatenates unrecognised options with the operands before it binds the positionals, so[agents_dir]resolved to<cwd>/--no-allow-unauthenticated.Solution: The action now reads commander's
Command.argsinstead ofprocess.argv. Commander has already removed the options the command declared, and it stops collecting operands at the first unrecognised option, so the split point is the first option-shaped token.[agents_dir]takes a value only from a real operand and otherwise keeps itsprocess.cwd()default.--helpnow documents the ordering and the--separator, which is the contract adk-python already prints for this command.Notes:
dev/src/cli/cli.tsno longer readsprocess.argv. The only remaining read indev/srciscli_entrypoint.ts:10.dev/src/cli/deploy/cli_deploy_cloud_run.tschanges.validateGcloudExtraArgsstill rejects a forwarded flag that ADK manages.--.gh pr list --repo AmaadMartin/adk-js --state open --limit 200returned no PR that fixes this. Fix: expose --otel_to_cloud on adk deploy cloud_run #694 (--otel_to_cloud) and Test: stop swallowing the parse error in the cloud_run pass-through CLI test #709 (test cleanup) touch the same two files in different hunks. I branched frommainrather than stacking, because neither is a dependency of this fix. Test: stop swallowing the parse error in the cloud_run pass-through CLI test #709 merges into this branch cleanly; Fix: expose --otel_to_cloud on adk deploy cloud_run #694 conflicts only where both append a test to the end of the samedescribeblock.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— 30 passed.npx vitest run --project unit:dev dev/test/cli/cli_deploy_cloud_run_test.ts— 27 passed.Four new cases in
describe('command: deploy cloud_run'). No existing test was edited, weakened, or deleted.The harness change deletes one line:
parse()assigned the globalprocess.argvand never restored it. No code under test reads argv any more, so the assignment is dead. It also made the existing "should pass args to deployToCloudRun including unknowns" test stronger, because that test can now only pass throughcommand.args.New code coverage: 100% of lines and branches. I read
coverage-final.json; no uncovered statement or branch falls incli.ts:426-448.Proof the tests can fail. Mutation:
git checkout main -- dev/src/cli/cli.ts, which restores theprocess.argv.slice(5)scan and leaves the tests in place. All four new cases fail, and so does one existing case:Manual End-to-End (E2E) Tests:
A deploy needs a Cloud project, so I exercised the argv plumbing against the real built CLI instead.
validateGcloudExtraArgsrejects a forwarded--source, which makes the pass-through set observable without contacting gcloud.With this PR, the first flag reaches the validator:
With the same command on the unfixed build, both defects show at once — the flag never reaches the validator, and it became the agent directory:
node <repo>/dev/dist/esm/cli_entrypoint.js deploy cloud_run --helpprints the new pass-through guidance.Other local checks on the pushed commit:
npm run lintclean,npm run format:checkclean.npm run ts:checkreports 287 errors in 42 files on this branch and 287 onmain; none are indev/src/cliordev/test/cli. That backlog is pre-existing and is being cleared by other PRs.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.