Fix: declare @types/js-yaml in dev/package.json - #567
Open
AmaadMartin wants to merge 2 commits into
Open
Conversation
added 2 commits
August 2, 2026 23:06
@google/adk-devtools imports js-yaml (dev/src/conformance/yaml_agent_loader.ts, dev/src/conformance/yaml_test_loader.ts) and adm-zip (dev/build.js), but neither package ships bundled type declarations and dev/package.json declared neither @types/* package. They resolved only because npm workspace hoisting places them in the repository-root node_modules -- @types/js-yaml from the root manifest and @types/adm-zip from core's. The manifest was therefore untruthful about the workspace's build requirements: a standalone `npm install` + `tsc` inside dev/ failed with TS7016 for 'js-yaml'. Ranges are copied verbatim from the existing declarations (^4.0.9 from the root manifest, ^0.5.8 from core) so no second copy of either type package enters the tree. Both are devDependencies because neither type leaks into the published dist/types declarations.
Review feedback: @types/adm-zip has no reader in the dev workspace, so declaring it asserts a typecheck relationship that does not exist. dev/tsconfig.json has "include": ["src/**/*"] with no allowJs/checkJs, so tsc never reads dev/build.js -- the only adm-zip importer in the workspace. Verified with `tsc -p dev/tsconfig.json --listFilesOnly`, which does not list dev/build.js at all. The declaration would land as an unused devDependency that pins a version with no signal behind it; it belongs with a change that actually makes build.js typechecked. @types/js-yaml is unaffected and remains the whole fix: dev/src/conformance imports js-yaml from inside src/**/*, so that one is load-bearing.
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
N/A — no existing issue.
Problem:
@google/adk-devtools(dev/) consumes a type-only package that its manifest does not declare. It resolves today only because npm workspace hoisting puts it in the repository-rootnode_modules, so the defect is invisible from inside the monorepo and only appears when the workspace is installed on its own.Import sites:
dev/src/conformance/yaml_agent_loader.ts:9—import yaml from 'js-yaml';dev/src/conformance/yaml_test_loader.ts:10—import yaml from 'js-yaml';The value package is declared (
dev/package.jsondependencies,"js-yaml": "^4.1.1"), butjs-yamlships no bundled declarations — verified against the version the lockfile actually resolves:The declarations come from
@types/js-yaml, which is declared only in the repository-rootpackage.json:45. The repo'stsconfig.jsonextendsgts/tsconfig-google.json, which sets"strict": true(hencenoImplicitAny), so a missing declaration is a hardTS7016, not a silentany.dev's own build script is"build": "tsc --emitDeclarationOnly && node ./build.js", so a standalonenpm install+npm run buildinsidedev/is exactly the path that breaks.That the types come from the hoisted root rather than from
devis directly observable:Solution: declare
@types/js-yamlindev/package.jsondevDependencies, keeping the block alphabetically sorted.package-lock.jsonis regenerated bynpm installand mirrors the same range underpackages["dev"].devDependencies. The whole PR is 2 added lines across 2 files, 0 deletions.Deliberate details:
^4.0.9from rootpackage.json:45. Not bumped, not pinned, not widened; divergent ranges across workspaces are how a second copy of a type package ends up in the tree.@types/js-yaml@4.0.9was already resolved (root-hoisted) in the lockfile, so no new package enters the dependency tree — only the range declaration.devDependenciesand notdependencies. A sibling change moves@types/expressin the opposite direction (out ofdev'sdevDependenciesand intodependencies), so the apparent inconsistency is worth pre-answering. The test is whether the type leaks into the publisheddist/typesdeclarations, which a consumer must resolve at install time:@types/expressdoes leak —dev's emitted.d.tsfiles reference express types. That makes it a runtime dependency.js-yamldoes not leak.yaml.load()returnsunknownin@types/js-yamlv4 and both call sites narrow immediately (yaml_agent_loader.ts:31,yaml_test_loader.ts:47,58,72). The only exported symbols from those modules arebatchLoadYamlAgentConfigandbatchLoadYamlTestDefs, typed with local domain types (YamlAgentConfig,TestSpec), and neither module is re-exported fromdev/src/index.ts. Nojs-yamltype reaches the public surface.@types/adm-zipwas considered and deliberately excluded.dev/build.js:14importsadm-zipanddevdoes not declare its types (onlycoredoes), which looks like the same defect. It is not, because that declaration would have no reader:dev/tsconfig.jsonhas"include": ["src/**/*"]with noallowJs/checkJs, sotscnever readsdev/build.js. Confirmed directly —npx tsc -p dev/tsconfig.json --listFilesOnly | grep -c dev/build.jsreturns0. Declaring@types/adm-zipindevwould therefore assert a typecheck relationship that does not exist, and would land as an unused devDependency thatdepcheck/knipflag and that churns on version bumps with no signal behind it. It belongs with a change that actually makesbuild.jstypechecked, not here.dev/build.jsis untouched by this PR, including its pre-existing@ts-ignore.knip,depcheck,import/no-extraneous-dependencies, a new CI step) is introduced. Out of scope here.Collision check (open PRs on this fork). Enumerated all 465 open PRs and resolved the changed-file list for every one; 21 touch
dev/package.json(#133, #148, #158, #169, #228, #244, #249, #250, #277, #290, #323, #382, #387, #444, #445, #484, #485, #508, #541, #544, #562). Diffed every one of them plus the adjacent lockfile/build.jsPRs (#183, #345, #482, #507) for^\+.*"@types/(js-yaml|adm-zip)": zero hits. No open PR lands this change, so this is not a duplicate. Textual overlap only: #562 deletes@types/expressfrom this exact block (two lines above the insertion point) and #541/#323 add entries todev'sdependenciesblock. Neither is a functional prerequisite and this change stands alone, so it is branched frommainrather than stacked; if #562 lands first, the resolution is to keep its deletion and place@types/js-yamlbetween@types/corsand@types/node.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:
[ ] I have added or updated unit tests for my change. — intentionally none; reasoning immediately below.
[x] All existing unit tests for the touched workspace pass locally (one pre-existing, environment-dependent failure documented below).
No test file is added, deliberately. This change adds zero executable lines — one JSON key naming a
.d.ts-only package that is erased at compile time.vitest.config.tsscopes coverage tocore/src/**/*.ts,dev/src/**/*.tsandintegrations/src/**/*.ts, so both the numerator and the denominator are unchanged and the configured thresholds are unaffected. The only test one could write is an assertion about the contents ofdev/package.jsonfrom a_test.tsfile — that is the dependency-checking harness this change explicitly excludes, it would be a tautology against the file the same PR edits, and no such manifest test exists onmainto extend. The regression signal for this class of bug belongs in a repo-wide manifest check, which is separately scoped.The "prove it can fail" obligation is therefore discharged at the manifest level, with a real isolated install rather than a percentage.
Before/after repro (the primary evidence). The defect is invisible in the monorepo, so the repro isolates the
devworkspace and installs only whatdev/package.jsondeclares:BEFORE (
main) — 734 packages installed,node_modules/@types/contains nojs-yaml:AFTER (this branch) — 735 packages, exactly one added,
@types/js-yamlnow present:Targeted mutation — is the added line load-bearing? Coverage-style "it ran" is not proof, so the line was removed from the fixed tree and the check re-run: reinstalling without
"@types/js-yaml": "^4.0.9"and re-running the sametscinvocation restores bothTS7016errors verbatim andexit=2. The single added line is the whole fix, and nothing else in the diff is doing work.Repo-wide checks (run on the pushed commit, in the monorepo):
npm installpackages["dev"].devDependenciesand nothing elsenpm run builddev'stsc --emitDeclarationOnly && node ./build.js)npm run lintnpm run format:checknpx secretlinton both changed filesnpx vitest run --project unit:dev dev/test/conformance/yaml_agent_loader_test.ts dev/test/conformance/yaml_test_loader_test.tsnpx vitest run --project unit:dev(whole touched workspace)Two honest caveats rather than a clean-sweep claim:
dev/test/cli/cli_create_test.ts > "should handle Vertex AI selection with gcloud defaults"fails identically with this change stashed, i.e. on the unmodified base. It is environment-dependent: the test expects a mocked project id but the workstation's realgclouddefault project leaks through. Unrelated to this PR and not something it should paper over.npm run ts:checkreports 281 pre-existingerror TSdiagnostics (278 incore/test, 3 intests/integration). Captured with and without this change: the two outputs are byte-identical, which is the expected result — in the monorepo@types/js-yamlis already root-hoisted, so declaring it indevcannot change root type resolution. Notets:checkis not one of the steps.github/workflows/validation.yamlruns.Manual End-to-End (E2E) Tests:
npm install && npm run build— thedevworkspace builds.git diff main --stat— exactlydev/package.jsonandpackage-lock.json, 2 insertions, 0 deletions.git diff main -- package-lock.json— the change is confined topackages["dev"].devDependencies; noresolvedURLs, integrity hashes or unrelated regions move.@types/js-yamlsits between@types/expressand@types/node.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.
[ ] I have added tests that prove my fix is effective or that my feature works. — no test file; effectiveness is proven by the isolated before/after repro and the mutation above.
[x] New and existing unit tests pass locally with my changes.