Fix: validate the agent name at the start of adk create - #598
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: validate the agent name at the start of adk create#598AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 3, 2026 16:47
createAgent() passed the raw commander argument straight to path.join(process.cwd(), name) and generateAgentFolder(), so a malformed or reserved name was only discovered after a directory had been created (or, on confirmation, deleted and recreated). 'adk create ../../foo' scaffolded out of tree, 'adk create user' produced an app whose name is reserved for end-user input, and 'adk create "my agent"' wrote an invalid package.json name. Call the existing validateAppName() from @google/adk as the first statement of createAgent so the error is raised before any I/O, matching the contract adk-python's run_cmd already provides. Unlike adk-python, which validates os.path.basename(os.path.normpath(name)) and therefore accepts path-shaped arguments, this validates the argument as given: the adk-js CLI documents it as a name, and the value is also interpolated into the generated package.json "name" field where a path separator is invalid.
Add an 'Agent Name Validation' describe block to cli_create_test that pins the three rejected shapes (space, reserved 'user', path traversal) and asserts none of the file_utils mocks were called, plus a valid name that still reaches createFolder. No existing test is modified.
AmaadMartin
pushed a commit
that referenced
this pull request
Aug 4, 2026
#599) * fix(core): fall back to node:crypto so randomUUID cannot throw on Node randomUUID() consults only globalThis.crypto. That global was added in Node v17.4.0 and stayed behind --experimental-global-webcrypto until v19.0.0, so on a default Node 18 or earlier neither branch matches and the function throws, where it previously returned a weak UUID. That surfaces as a hard failure in createSession, AuthHandler.generateAuthUri and every A2A message id. node:crypto's randomUUID has existed since v14.17.0 and does not depend on the global, so using it as the last resort makes the throw unreachable on any Node this package could plausibly target. The two globalThis.crypto branches keep precedence, so browsers and Node 19+ are unaffected. randomUUID reaches the web bundle via index_web.ts -> common.ts -> events/event.js, so the node:crypto import is aliased to a browser shim, following the existing node:async_hooks precedent in build.js. The shim throws the message the function used to throw: it is reached only once both globalThis.crypto branches have been ruled out, which in a browser means the Web Crypto API is genuinely absent. Fixes #598. * docs(core): scope the alias note to the bundled web build The comment said the import is aliased in "the web build", but the alias in build.js applies only when platform is browser and bundle is set. The output package.json#browser points at, dist/web/index_web.js, comes from the non-bundle path and keeps the import verbatim, as it already does for node:async_hooks and node:path.
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 public issue; described below.
Problem:
adk createperforms no validation of the agent name before it starts writing to the filesystem.createAgent()(dev/src/cli/cli_create.ts) took the raw commander argument and immediately computedpath.join(process.cwd(), options.agentName)and calledgenerateAgentFolder(), which creates — or, after a confirmation prompt,removeFolder()s and recreates — that directory. The same raw string is also interpolated into the generatedpackage.json"name"field.adk create ../../foogenerateAgentFoldermayremoveFolder()an out-of-tree directory.adk create useruser, which is reserved for end-user input and breaks session/branch semantics at runtime.adk create "my agent"package.json"name".adk-python already blocks these:
run_cmdinsrc/google/adk/cli/cli_create.pycallsvalidate_app_name(...)as its first statement, before any folder is touched.Solution: call the validator adk-js already has, as the first statement of
createAgent, so the error is raised before any I/O.validateAppNameis already public API of@google/adk(core/src/apps/app.ts→core/src/common.ts:328→core/src/index.ts:40), and is already used by theAppconstructor. No new export, no new helper module, no new type, no refactor — the source diff is 3 added lines (1 import + 1 call + 1 blank).validateAppNamestays the single source of truth: its regex, reserved-name list, and message strings are not duplicated or reworded in thedevpackage.Why this solution over the alternatives:
devwould fork the source of truth and drift fromApp's constructor and from adk-python.generateAgentFolder) would not meet the requirement, which is specifically that nothing is written to disk for a bad name.process.exit(1)to thecreateaction's catch block is deliberately not done here — the non-zero-exit gap is a separate concern and is already addressed by a separate change.Deliberate divergence from adk-python (cross-language parity — which side wins):
adk-python validates
os.path.basename(os.path.normpath(agent_name)), so it accepts path-shaped arguments such as../../foo(basenamefoo) and scaffolds out of tree. adk-js validatesoptions.agentNameas given, with nopath.basename/path.normalizepreprocessing, because:.argument('[agent]', 'Name to give the new agent', 'adk_agent')(dev/src/cli/cli.ts:307);package.json"name", where a path separator is invalid; andLocal convention wins here because the divergence is stricter: the resulting rule can never accept a name adk-python would reject. The error messages are kept verbatim identical to adk-python's, since those are the observable, user-facing surface. A dedicated test pins this divergence, so a future
path.basenamenormalisation cannot land silently.Breaking-change note: this tightens accepted input for
adk create. Path-shaped names,user, and names with spaces or leading digits/hyphens are now rejected. All of those previously produced a scaffold that was broken at runtime or in npm, so the rejection is the fix rather than a regression.grep -rn "adk create"finds onlyREADME.md:50, which documents no path form. No API signature, export, or type changes; no effect on any other command.Collision check (required before implementing):
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000→ 496 open PRs, none of which adds name validation toadk create(verified by grepping every plausibly adjacent title forcreate/agent name/validate/app name, thengh pr diff --name-onlyon the 13 candidates and grepping the diffs of the three that touchdev/src/cli/cli_create.tsforvalidateAppName). Three open PRs touch the same file but different, non-overlapping regions — #456 (dotenv secret warning), #364 (.gitignoregeneration), #286 (GOOGLE_GENAI_USE_ENTERPRISE) — all editing the template/output blocks, notcreateAgent's prologue. Since they are disjoint hunks in the same file rather than the same change, this branches frommainrather than stacking; pulling their diffs in would violate diff hygiene.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.
Added a new
describe('Agent Name Validation', ...)block to the existingdev/test/cli/cli_create_test.ts. No existing test was modified, weakened, skipped, or deleted. Four cases:'my agent'with/Invalid app name 'my agent'/and assertsisFolderExists,createFolder,removeFolder,saveToFilewere none of them called (the direct analogue of adk-python'stest_run_cmd_invalid_app_name);'user'with/reserved for end-user input/, same no-I/O assertion;'../../foo'with/Invalid app name '\.\.\/\.\.\/foo'/, same no-I/O assertion — this pins the deliberate divergence above;'valid_agent-1'and assertscreateFolderwas still called with a path containing it.The
validateAppNamerule set itself is already unit-tested incore/test/apps/app_test.ts; those cases are deliberately not duplicated here. These tests exist to prove the CLI calls the validator before touching the filesystem.Coverage. The change adds exactly one executable statement in
src. Per-line coverage ofdev/src/cli/cli_create.tsconfirms it is hit 15 times, with both outcomes exercised (3 tests take the throw path, 12 the pass-through path):i.e. 100% line and branch coverage of the new code. The file-level number is 91.86% lines / 82.81% branches, but every uncovered line listed is pre-existing and untouched by this change — none is within 39 lines of the added statement.
Proof the tests can fail (mutation testing). Two mutations were run against the new tests:
Mutation 1 — delete the
validateAppName(options.agentName);line (i.e. the unfixed code). Cases 1–3 fail, case 4 correctly still passes:Mutation 2 — adopt adk-python's basename behaviour,
validateAppName(path.basename(path.normalize(options.agentName))). Only the traversal case fails, confirming case 3 uniquely pins the documented divergence and is not merely redundant with case 1:The source was restored to the committed state and all 15 tests re-run green afterwards.
Note on a pre-existing, unrelated failure. On a machine with ambient
GOOGLE_CLOUD_PROJECT/GOOGLE_CLOUD_LOCATIONexported, the pre-existing testInteractive Mode > should handle Vertex AI selection with gcloud defaultsfails. It fails identically on the unmodified branch (1 failed | 10 passed), so it is not caused by this change; it is the non-hermeticity already tracked by open PR #569. It is out of scope here and was not touched. With those variables unset the whole file is green.No integration test was added:
adk createis not covered bytests/integration/**today, and the failure path is fully observable at the unit level. Adding one would mean running a realnpm install.Manual End-to-End (E2E) Tests:
Run against the real built CLI with no mocks — real
commander, real filesystem:Observed:
ls -Ain the scratch directory was empty after all four, and nofoodirectory appeared in any ancestor — confirming nothing was written for a rejected name. The happy path is unchanged:create my_agent -yproduced.env,agent.ts,package.json("name": "my_agent"),tsconfig.json,package-lock.jsonandnode_modules, and printed the same summary as before.Other checks run locally on the pushed commit:
npm run ts:checkreports 281 errors on this tree, but the count is identical with and without this change (281vs281) and none is indev/. They come fromcore/dist/typesco-existing withcore/srcafter a build, are pre-existing, andts:checkis not part of the validation workflow.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.
No
any,@ts-expect-error,@ts-ignore,eslint-disable, or coverage suppression is added anywhere in this diff (source or tests) — verified by grep overgit diff.