Fix: describe what --file_type selects in the CLI help text - #870
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: describe what --file_type selects in the CLI help text#870AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 9, 2026 08:18
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.
The description claimed the auto-detected format comes from "the agent file extension and the nearest package.json type field", which reads as if both inputs always apply. getFileModuleType() is extension-first: it returns cjs for .cjs/.cts and esm for .mjs/.mts without opening any package.json, and only .js and .ts walk up to the nearest package.json. State that order instead, and pin it with a second parameterised help assertion over the same six command surfaces.
This was referenced Aug 11, 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
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem:
--file_typeshipped the placeholder description'Optional. ', soadk web --helpprinted onlyOptional. (choices: "cjs", "esm"). The commanderhelp string is the only documentation for this flag, so a reader cannot tell what
cjschanges, whatesmchanges, or what happens when the flag is omitted. Thesame empty row appears on all six command surfaces that share the option:
web,api_server,run,deploy cloud_run,deploy agent_engineanddeploy reasoning_engine.Solution: Fill in the description from the behaviour in
dev/src/utils/agent_loader.ts. The value becomes esbuild's outputformatandpicks the emitted extension through
FILE_MODULE_TYPE_EXTENSION_MAP(cjs->.cjs,esm->.mjs). When the flag is unset,getFileModuleType()reads theagent file extension first, and only
.jsand.tsfall back to the"type"field of the nearest
package.json. This is a help-string change only:argChoices, the flag spelling, the absent default and the fiveaddOptioncall sites are untouched, so parsing and the values handed to the loader are
identical.
Collision check: I listed the 300 open PRs on this fork and diffed every
adjacent CLI one (#803, #816, #700, #695, #693, #685, #706, #705, #709, #592,
#587). None touches
--file_type. Two things do overlap and are disclosed here:fix/cli-bundle-file-type-help-textalready carried this fix on thefork, with no PR of its own. This PR is that commit, rebased onto current
main, plus one accuracy commit.Fix: reject an invalid --file_type up front with commander's choices validator) is stacked on that branch, so its branch history also contains thehelp-text commit. Fix: reject an invalid --file_type up front with commander's choices validator #451 should be rebased onto this branch before it is ported,
or the two will carry the same hunk.
Deviation from the plan: the plan proposed wording that says the format "is
detected from the agent file extension and the nearest package.json type field".
That reads as if both inputs always apply.
getFileModuleType()returnscjsfor
.cjs/.ctsandesmfor.mjs/.mtswithout opening anypackage.json.The shipped wording states that order instead.
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.
Two parameterised cases over the six command surfaces in
dev/test/cli/cli_test.ts(12 cases). Both read the real--file_typehelp rowthrough
Command.helpInformation(), whitespace-normalized so the assertions donot depend on commander's column wrapping. Existing cases are unchanged.
Mutation proof 1 — restore the placeholder
new Option('--file_type <string>', 'Optional. '). All 12 cases fail:Mutation proof 2 — restore only the over-claiming sentence ("detected from
the agent file extension and the nearest package.json type field"). The 6 new
cases fail and the 6 older ones still pass, so the second case pins the accuracy
fix on its own:
Coverage: the change is one string literal on a module-initialisation line that
every existing case in this file already executes, so line and branch coverage
of the change is 100% by construction. The tests above exist to pin the string's
content, which coverage cannot do.
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Each surface prints the description and still ends with
(choices: "cjs", "esm"). I ran all six. To confirm parsing did not change,node dev/dist/esm/cli_entrypoint.js run missing_agent.ts --file_type esmstill failsin
AgentFile.loadwithAgent file .../missing_agent.ts does not exists, notwith an option error, so the flag is still accepted and threaded to the loader.
Gates run on the pushed commit:
npx eslint dev/src/cli/cli.ts dev/test/cli/cli_test.ts(clean) andnpx prettier ... --check(clean).npm run ts:checkreports 309 errors on this branch and the same 309 onmain;none of them is in a file this PR touches.
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 note
The first Windows run failed in
tests/integration/app_loader/app_loader_test.tswith
Test timed out in 40000ms. That test compiles fixture agents with esbuildand does not import anything this PR touches. A re-run passed, and PR #858
already targets that timeout. All three platforms are green.