Skip to content

Fix: validate --log_level against a closed vocabulary and stop discarding LogLevel.DEBUG - #592

Open
AmaadMartin wants to merge 3 commits into
fix/cli-file-type-choices-validationfrom
fix/cli-log-level-choices-validation
Open

Fix: validate --log_level against a closed vocabulary and stop discarding LogLevel.DEBUG#592
AmaadMartin wants to merge 3 commits into
fix/cli-file-type-choices-validationfrom
fix/cli-log-level-choices-validation

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 3, 2026

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):
    N/A — no public issue is tracking this.
  2. Or, if no issue exists, describe the change:

Problem: Two distinct defects in the same four lines of dev/src/cli/cli.ts.

Defect 1 — --log_level has no validator. LOG_LEVEL_OPTION was declared with no
choices() and no argParser(), and getLogLevelFromOptions resolved the value with
LOG_LEVEL_MAP[value] || LogLevel.INFO. Any string outside debug|info|warn|error
(debbug, DEBUG2, trace, "") silently fell through to INFO with no warning and
exit code 0. Every command carrying the shared option was affected: web, api_server,
run, deploy cloud_run, deploy agent_engine, deploy reasoning_engine, and
integration conformance.

The deploy paths were the worst case, because the bad value is persisted:
deploy_utils.ts appends --log_level=${options.logLevel} verbatim into the entrypoint
baked into the generated Dockerfile, and cli_deploy_cloud_run.ts lowercases the same
value into the gcloud run deploy --verbosity argv. Verified against the pre-fix build —
adk deploy cloud_run --log_level DEBUG2 produced:

CMD npx adk api_server /app/agents/... --port=8000 --host=0.0.0.0 --log_level=DEBUG2

so a typo degrades logging for the lifetime of the deployment.

Defect 2 — --log_level debug never enabled debug logging. LogLevel is a numeric
enum with DEBUG = 0, which is falsy, so LOG_LEVEL_MAP['debug'] || LogLevel.INFO
evaluated to 0 || 1 === 1. adk web --log_level debug ran at INFO. Only -v/--verbose
reached DEBUG, because it takes an early return and never touches the map. Confirmed
end-to-end against the pre-fix build (full table below). No existing test covered this —
the only LogLevel.DEBUG assertion in cli_test.ts goes through --verbose.

Solution: Attach the vocabulary to the single shared LOG_LEVEL_OPTION instance so
all seven commands validate at parse time, and switch the fallback from || to ??.

  • LOG_LEVEL_CHOICES = [...LOG_LEVEL_MAP.keys()] — the level table stays the single source
    of truth, so the advertised choices cannot drift from the levels we can resolve. No second
    literal ['debug','info','warn','error'] is written anywhere.
  • .choices(LOG_LEVEL_CHOICES).argParser(parseLogLevel).choices() populates
    argChoices, which is what --help and commander's Allowed choices are ... message
    render; .argParser() then replaces commander's case-sensitive validator with a
    case-insensitive one that normalizes to the canonical lower-case name. Order matters and
    is called out in a comment. Errors go through commander's own channel
    (InvalidArgumentErrorCommand.error()); no bespoke error type, no hand-rolled
    console.error + process.exit.
  • ?? instead of || in getLogLevelFromOptions, so LogLevel.DEBUG === 0 survives.
  • LOG_LEVEL_MAP is a Map<string, LogLevel> rather than an object literal. A Map has no
    prototype chain to walk, so constructor and __proto__ are ordinary misses for
    has/get — no guard and no explanation needed. See the review-round notes below.

Deliberate decision: --log_level DEBUG keeps working. A bare .choices() is
case-sensitive and would reject upper-case values that work today — a second, unrelated
breaking change smuggled into a bug fix. The Python SDK validates the same flag with
click.Choice(..., case_sensitive=False), and the CLI surface is observable across the
language boundary, so parity wins here over a 3-line implementation. Normalizing in the
parser also means the value baked into a generated Dockerfile is always canonical, so the
containerised adk api_server — which after this change validates its own --log_level
can never reject an image the CLI just produced. Alternative considered and rejected:
plain .choices(LOG_LEVEL_CHOICES) with no custom parser.

Deliberately out of scope (independent of this fix, queued separately): --log_level warn produces the invalid gcloud verbosity warn (gcloud wants warning); --verbose is
silently ignored on the deploy commands and beats an explicit --log_level elsewhere,
contrary to adk-python; and the JS/Python vocabularies differ (warn vs
WARNING/CRITICAL). Nothing from the approved spec was dropped.

This is a breaking CLI change. Behaviour changes worth calling out to users:

  • --log_level <anything outside debug|info|warn|error> now exits non-zero. Previously
    --log_level trace (or any typo) fell back to INFO silently. Failing loudly is the point
    of this PR, but a script that passed a bad level and relied on it being ignored will now
    break — with a message naming the fix.
  • --log_level debug now actually produces DEBUG. That is the advertised behaviour finally
    taking effect, but it is a real change in log volume for anyone already passing it.
  • Beyond the CLI surface: deployToCloudRun / deployToAgentEngine still take an
    unvalidated logLevel: string (deliberately — the spec keeps the programmatic API open).
    If a programmatic caller passes something like 'warning', deploy_utils.ts bakes
    --log_level=warning into the entrypoint and the containerised adk api_server will now
    reject it at boot rather than degrading to INFO. Garbage-in, and arguably the better
    failure mode, but it is a behaviour change outside the CLI and worth stating.

Review round 1 — prototype-chain bypass. The first revision validated with
if (!(normalized in LOG_LEVEL_MAP)). in walks the prototype chain, so
adk web --log_level constructor exited 0, started the server, and stored the Object
constructor as the log level (making every later this.logLevel > level guard a NaN
comparison), while deploy cloud_run --log_level __proto__ reached deployToCloudRun and
baked --log_level=__proto__ into the generated entrypoint — reproducing both of the
defects this PR exists to eliminate. constructor and __proto__ are the only
Object.prototype keys that survive .toLowerCase(), so they were the whole exposure.

Review round 2 — the level table is now a Map. Round 1 fixed the bypass with an
Object.hasOwn-based resolveLogLevel helper; that was a hand-rolled own-key lookup plus a
comment explaining the hazard. A Map has no prototype chain at all, so has/get are the
guard and both the helper and its rationale comment are gone — the bug class is removed
structurally rather than guarded against (net −15 lines in cli.ts). This also makes spec
invariant 2 true for the first time: LOG_LEVEL_MAP['constructor'] ?? LogLevel.INFO
returned Object on main, so "returns LogLevel.INFO for an unrecognised string" was
never actually the case. Map preserves insertion order, so LOG_LEVEL_CHOICES and the
help text are byte-identical and no test changed. The ?? stays — it is the Defect 2 fix
and is still required by get's LogLevel | undefined return.

While there, the .toLowerCase() in getLogLevelFromOptions is dropped as dead:
LOG_LEVEL_OPTION is the only writer of options.log_level at every addOption site,
parseLogLevel already returns the canonical lower-case name, and commander's 'info'
default bypasses parseArg already lower-cased. Keeping it implied unvalidated input could
reach that line. Removing it also strengthened a mutation: parseLogLevel returning value
instead of normalized now fails 3 tests rather than 1.

Collision check (required before implementation). gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 returned 490 open PRs; none implements
--log_level validation. The nearest neighbours were reviewed and are unrelated: #431
(getLogLevel() accessor on the core logger), #557 / #349 (pinning the log level inside
vitest workers), #432 (winston logger dedup) — all in core/, none touching
dev/src/cli/cli.ts.

This PR is stacked on #451 (fix/cli-file-type-choices-validation, itself stacked on
#358), which touches the same two files for the sibling --file_type flag. It is not a
competing implementation — different option, different code region — but it lands the
applyExitOverride / argvFor / findCommand / expectNoActionRan test helpers this
change needs, so stacking reuses them instead of adding a second copy that would conflict
on merge. Base is fix/cli-file-type-choices-validation; please merge #358 and #451 first.

Files changed: dev/src/cli/cli.ts only, on the production side. deploy_utils.ts,
cli_deploy_cloud_run.ts, cli_deploy_agent_engine.ts, core/src/utils/logger.ts and
AGENT_FILE_MODULE_TYPE are untouched. In particular the now-redundant .toLowerCase() in
cli_deploy_cloud_run.ts stays: deployToCloudRun is also callable programmatically,
bypassing commander.

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.

29 new cases in dev/test/cli/cli_test.ts, in a new describe('option: --log_level validation') block. No existing test was modified, weakened, skipped, or deleted; the
shared beforeEach is untouched. The one edit to shared test scaffolding is additive:
expectNoActionRan() now also asserts runIntegrationTests was not called, so the
integration conformance row of the rejection table is not vacuous.

npx vitest run --project unit:dev dev/test/cli/cli_test.ts
  ✓ dev/test/cli/cli_test.ts (67 tests)   Tests  67 passed (67)

Coverage:

  • Every line and branch this PR adds to dev/src/cli/cli.ts is covered, except one:
    the LogLevel.INFO right-hand arm of ?? LogLevel.INFO (line 70, col 46-65) reports 0
    hits. That is unreachable by design: commander now guarantees the CLI only ever hands
    getLogLevelFromOptions a validated, normalized level. The fallback is deliberately kept
    so the function stays total for programmatic callers (deployToCloudRun /
    deployToAgentEngine still accept an unvalidated logLevel: string and bypass
    commander). Per the repo guideline, the safety net is kept and the measured shortfall
    reported rather than deleting a guard to move a number. Measured with
    --coverage.include='dev/src/cli/cli.ts' and read out of coverage-final.json;
    parseLogLevel is hit 22 times with both arms exercised.
  • The other 0-hit branch in that function (the implicit else of typeof options.log_level === 'string') is pre-existing code, unreachable because LOG_LEVEL_OPTION has
    .default('info'). Not introduced or changed here.

Proof the tests can fail. Each mutation below was applied to dev/src/cli/cli.ts and
the targeted suite re-run:

Mutation Result
?? reverted to || 2 failedapplies --log_level debug and accepts --log_level DEBUG regardless of letter case: AssertionError: expected "spy" to be called with arguments: [ +0 ] (i.e. LogLevel.DEBUG; INFO was passed)
.argParser(parseLogLevel) removed 3 failedaccepts --log_level DEBUG…, accepts --log_level Warn…, normalizes the value handed to the Cloud Run deploy path
.choices(LOG_LEVEL_CHOICES) removed 7 failed — every lists the accepted levels in \` help` case
both .choices() and .argParser() removed (pre-fix wiring) 25 failed — all 7 rejection cases, all 4 inherited-key cases, both deploy-rejection cases, the invalid-argument-shape case, the empty-string case, all 7 help cases, both case-insensitivity cases, and the deploy normalization case
parseLogLevel returns value instead of normalized 3 failednormalizes the value handed to the Cloud Run deploy path (expected { …(15) } to match object { logLevel: 'debug' }, actual "logLevel": "DEBUG") plus both case-insensitivity cases
LOG_LEVEL_MAP reverted to a plain object literal consulted with in 5 failedrejects the inherited key --log_level constructor / __proto__ / CONSTRUCTOR / __PROTO__ and keeps an inherited key out of the Cloud Run deploy path: AssertionError: promise resolved "undefined" instead of rejecting

The inherited-key list is exactly the discriminating set: constructor and __proto__ are
the only Object.prototype keys that survive .toLowerCase(), so cases like toString
were dropped from the table rather than shipped as tests that pass either way.

Manual End-to-End (E2E) Tests:
Please describe the tests that you ran to verify your changes.

All of the following were run against the real built CLI (npm run build, then
node dev/dist/esm/cli_entrypoint.js …) with nothing mocked.

  1. Rejection is total, on every option holder. Each exits 1, prints commander's
    standard message, and runs no action:

    $ node dev/dist/esm/cli_entrypoint.js web --log_level debbug
    error: option '--log_level <string>' argument 'debbug' is invalid. Allowed choices are debug, info, warn, error.
    exit=1
    

    Same for run agent.ts --log_level trace, api_server --log_level verbose,
    integration conformance --log_level '', and deploy cloud_run --log_level DEBUG2
    (which sets allowUnknownOption() — confirmed it does not bypass the validator).
    Also re-verified for the prototype-chain keys after the review fix —
    web --log_level constructor, __proto__, CONSTRUCTOR, __PROTO__ and
    deploy cloud_run --log_level __proto__ all exit 1 with the standard message.

  2. Nothing is written on the deploy path when the level is invalid.
    deploy cloud_run --log_level DEBUG2 --temp_folder <dir> exits 1 and <dir> is never
    created. Same for --log_level __proto__.

  3. Level resolution, real CLI, before vs after. Driven through createProgram() from
    dev/dist/ with a recording logger installed via the public setLogger() extension
    point:

    argv pre-fix post-fix
    adk web INFO (1) INFO (1)
    adk web --log_level debug INFO (1) DEBUG (0)
    adk web --log_level DEBUG INFO (1) DEBUG (0)
    adk web --log_level Warn WARN (2) WARN (2)
    adk web --log_level error ERROR (3) ERROR (3)
    adk web --log_level debbug INFO (1), silent rejected, exit 1
    adk web --log_level constructor Object, server started rejected, exit 1
    adk web --log_level __proto__ {}, server started rejected, exit 1
    adk web --verbose DEBUG (0) DEBUG (0)
  4. Generated Dockerfile is normalized. deploy cloud_run --log_level DEBUG …, with the
    temp folder captured before cleanup:

    CMD npx adk api_server /app/agents/... --port=8000 --host=0.0.0.0 --log_level=debug
    

    (pre-fix, the same run with DEBUG2 baked in --log_level=DEBUG2).

  5. Help advertises the choices. node dev/dist/esm/cli_entrypoint.js web --help:

    --log_level <string>   Optional. The log level of the server
                           (choices: "debug", "info", "warn", "error",
                           default: "info")
    

CI note — no test job will run on this PR. .github/workflows/validation.yaml is
gated on pull_request: branches: [main], and this PR targets
fix/cli-file-type-choices-validation (see the stacking note above), so the workflow never
triggers. Validated locally instead, on the exact pushed commit:

npm run build            -> OK
npx vitest run --project unit:dev dev/test/cli/cli_test.ts -> 67 passed (67)
npx vitest run --project unit:dev dev/test/cli               -> 111 passed, 1 failed
npm run lint             -> clean
npm run format:check     -> All matched files use Prettier code style!
npx secretlint "dev/**/*.ts" -> exit 0
npm run docs:check       -> exit 0

The one failure across the whole dev/test/cli directory is
cli_create_test.ts > should handle Vertex AI selection with gcloud defaults, which is
pre-existing and environment-dependent — it reads the developer's ambient gcloud project
instead of the mocked one. Confirmed it fails identically with this branch's changes
stashed, and it is unrelated to --log_level (see the open hermeticity fixes #569 / #576 /
#589).

(npm run ts:check is not a CI gate and reports ~308 pre-existing errors in
core/test/** and tests/integration/** on this base and ~331 on main; none are in the
files touched here, and the count is unchanged by this PR.)

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.

Amaad Martin added 3 commits August 3, 2026 13:56
`--log_level` had no validator, so a typo such as `debbug` silently
resolved to INFO on every command that carries the option, including the
deploy commands, which bake the raw string into the generated Dockerfile
entrypoint and the gcloud `--verbosity` argv. The bad value then degrades
logging for the lifetime of the deployment with no signal to the user.

`--log_level debug` was also broken independently: `LogLevel.DEBUG` is 0,
so the `||` fallback in `getLogLevelFromOptions` discarded it and the CLI
ran at INFO. Only `-v/--verbose` reached DEBUG.

Attach the level vocabulary to the shared `LOG_LEVEL_OPTION` instance so
all seven commands validate at parse time, and switch the fallback to
`??`. The choices are derived from `LOG_LEVEL_MAP`, which stays the single
source of truth. A custom `argParser` keeps commander's help and error
text while accepting any letter case and normalizing to the canonical
lower-case name, matching the Python SDK's
`click.Choice(..., case_sensitive=False)` and keeping today's
`--log_level DEBUG` invocations working.
…evels

`parseLogLevel` tested membership with `name in LOG_LEVEL_MAP`. `in` walks
the prototype chain, so `constructor` and `__proto__` — the two
`Object.prototype` keys that survive lower-casing — passed the validator.
`adk web --log_level constructor` exited 0 and started the server with the
`Object` constructor stored as the log level, which makes every subsequent
`logLevel > level` guard a NaN comparison; and
`deploy cloud_run --log_level __proto__` reached `deployToCloudRun`, baking
`--log_level=__proto__` into the generated container entrypoint.

Route both the validator and `getLogLevelFromOptions` through one
prototype-safe `resolveLogLevel` helper built on `Object.hasOwn`. Sharing
the lookup also closes the same hole in the defensive fallback, so that
fallback is finally total for programmatic callers as intended, and adds no
separately-tested code path.

The rejection suite now covers `constructor`, `__proto__` and their
upper-case forms, and asserts an inherited key cannot reach the Cloud Run
deploy path. `expectNoActionRan` also asserts `runIntegrationTests` was not
called, so the `integration conformance` row of the rejection table is no
longer vacuous.
A Map has no prototype chain to walk, so `constructor` and `__proto__` are
ordinary misses. That removes the need for the hand-rolled
`Object.hasOwn`-based lookup and the comment explaining why the prototype
chain was a hazard: `has` and `get` are the guard.

`Map` preserves insertion order, so `LOG_LEVEL_CHOICES` and the help text
are unchanged.

Also drop the `.toLowerCase()` in `getLogLevelFromOptions`. It is dead now
that `LOG_LEVEL_OPTION` is the only writer of `options.log_level` and
`parseLogLevel` returns the canonical lower-case name (commander's `'info'`
default bypasses `parseArg` already lower-cased), and keeping it implied
unvalidated input could reach that line.
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