Fix: describe what --file_type actually selects in the CLI help - #358
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: describe what --file_type actually selects in the CLI help#358AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
The --file_type option shipped the placeholder description 'Optional. ', so `adk web --help` (and five other command surfaces that share the option) rendered only "Optional. (choices: \"cjs\", \"esm\")". Nothing told the reader what picking cjs over esm actually changes. Replace it with the behaviour read off AgentFile.load(): the value becomes esbuild's output `format` and selects the emitted extension via FILE_MODULE_TYPE_EXTENSION_MAP (cjs -> .cjs, esm -> .mjs), and when the flag is omitted getFileModuleType() infers it from the agent file extension and the nearest package.json "type" field. Pure help-text change: the flag spelling, the absence of a default, and argChoices are all untouched, so parsing and the values handed to agent_loader are identical. Commander still appends the choices list itself, so it is not duplicated in the description. Add a regression pin over all six command surfaces asserting the entry is no longer the bare placeholder, that it names both emitted extensions, and that commander still renders the choices list.
This was referenced Jul 31, 2026
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 existing issue.
Problem:
AGENT_FILE_MODULE_TYPEindev/src/cli/cli.tsshipped the placeholder description'Optional. ', so--file_typerendered with no information at all:The option is a shared
Optioninstance registered on six command surfaces (adk web,adk api_server,adk run,adk deploy cloud_run,adk deploy agent_engine,adk deploy reasoning_engine), so the empty description appears six times. There is no other user-facing documentation for this flag in the repo — the help string is the documentation — so nothing tells a reader what pickingcjsoveresmactually changes.Solution: Replace the placeholder with the behaviour read off the implementation, not invented:
format(dev/src/utils/agent_loader.ts,AgentFile.load()), and selects the emitted file's extension throughFILE_MODULE_TYPE_EXTENSION_MAP—cjs→.cjs,esm→.mjs.getFileModuleType()infers the format from the agent file extension (.cjs/.cts→ cjs,.mjs/.mts→ esm) and otherwise from the nearestpackage.json"type"field viagetTypeFromPackageJson().This is a pure help-text change with no behavioural change. The flag spelling
--file_type <string>is unchanged, it still has no default, andargChoicesis untouched — so parsing and the values handed toagent_loaderare byte-identical. The description deliberately does not repeat thecjs/esmlist, because commander (v14, perdev/package.json) appends(choices: "cjs", "esm")itself fromargChoices; that auto-append was verified against the rendered help rather than assumed, and is pinned by a test assertion.Before (
adk web --help):After (
adk web --help):Scope note —
--bundleis deliberately NOT touched here. The original task also covered--bundle, whose description is a verbatim copy of--compile's. A collision check found that is already fixed by a live PR, and that a second live PR is actively changing what the flag means:gh pr list --repo AmaadMartin/adk-js --state open --limit 100→ thengh api .../pulls/<n>/filesacross every open PR to find the ones touchingdev/src/cli/cli.ts/dev/test/cli/cli_test.ts(Fix: scrub ambient DATABASE_URL across the whole dev CLI unit suite #348, Chore: enforce the node: protocol for Node built-in imports (ESLint) #346, Fix: Enable no-floating-promises and await five floating promises (Part 2/2) #333, Fix: decouple minification from --bundle in the dev agent loader #328, Fix: import node:path in the dev CLI (last bare Node built-in specifier in dev/src) #289, Perf: externalize third-party packages when compiling agent files (~54x faster AgentFile.load) #285)."Optional. Whether to bundle the agent's own imported modules into a single compiled file before execution". Writing a competing description on the same line would be a guaranteed merge conflict and two rival wordings for one flag.--bundle") splits minification out into a new--minifyflag, which would invalidate any description of--bundlethat mentions minifying.--file_type, and neither PR's hunk overlaps theAGENT_FILE_MODULE_TYPElines (they sit atcli.ts:137-147; this change is atcli.ts:150). So this PR branches frommainrather than stacking — the--file_typefix is genuinely independent, and stacking it behind an unrelated in-flight PR would strand it if that PR does not land.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.
New
describe('option help text')block indev/test/cli/cli_test.ts. No existing test was edited, reordered, skipped or deleted — the block is purely additive and sits afterdescribe('command: version'). It drivesit.eachover all six command surfaces, readscmd.helpInformation(), and asserts the--file_typeentry:/^--file_type <string> Optional\. \(choices/);.cjs,.mjs);(choices: "cjs", "esm"), pinning thatargChoicessurvives and is not duplicated in the description.Assertions run against a whitespace-normalized slice of the rendered help so they do not break when commander re-wraps columns, and the slice is scoped to the
--file_typeentry so a match cannot be satisfied by some other option's text. The help is deliberately not snapshotted — a full snapshot would turn every unrelated option addition into a failure.Proof the tests can fail (mutation testing). Each new test was run against mutated source and confirmed to FAIL:
new Option('--file_type <string>', 'Optional. ')): all 6 new tests fail.'Optional. The module format.'), to prove the content assertions are not dead weight: all 6 fail.AGENT_FILE_MODULE_TYPE.argChoices = [...], to prove the choices assertion is live: all 6 fail.Coverage: the change is two string literals inside
createProgram(), which the existingbeforeEachalready executes, so line/branch coverage of the changed code is 100% by construction. No suppressions of any kind were added (@ts-expect-error,@ts-ignore,eslint-disable,any,as never, coverage-ignore): the pre-PR grep overgit diff main -U0returns zero hits.npm run ts:checkreports 313 errors in 53 files both with and without this diff — an identical, pre-existing repo-wide condition in the test tree (already being addressed by separate PRs); this change introduces none of them and none are in the two touched files.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Confirm on each surface that the
--file_typeline reads as in the "After" block above, that it still shows(choices: "cjs", "esm")and still shows no(default: ...), and that no other option line changed. Ran locally on the pushed commit; all six render correctly.Other local validation on the pushed commit:
npm run build(clean),npm run lint(clean, exit 0),npx prettier --checkon both touched files (clean).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.
CI
All checks green:
run-testson ubuntu-latest, macos-latest and windows-latest all pass.The first windows-latest run failed on two tests in
tests/integration/tools/run_skill_script_tool_test.ts("successfully executes a real Python skill script" and "successfully executes a real PowerShell skill script"), bothError: Test timed out in 5000ms.while spawning a real Python/PowerShell subprocess. That is unrelated to this diff, which touches onlydev/src/cli/cli.tsanddev/test/cli/cli_test.tsand cannot affect subprocess spawn latency; the same job passes on sibling PRs, and it passed on re-run with no code change (8m50s vs the 7m41s failing run — a slow-runner flake, not a real failure). Queued separately as a timeout-hardening follow-up rather than papered over here.