Fix: declare the root workspace's test dependencies in package.json - #360
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: declare the root workspace's test dependencies in package.json#360AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
…json Files under tests/** are owned by the workspace-root package, but the third-party packages they import are declared in no manifest on their resolution path. They resolve today only because npm hoists core/'s and dev/'s dependencies into the repo-root node_modules/, so the tests/ suite is silently coupled to manifests that never mention it -- openapi-types is merely a devDependency of core, yet a root e2e test imports it. Declare them in the root devDependencies at the exact range the owning workspace already declares, so the coupling is recorded where it lives: @google-cloud/vertexai ^1.12.0 (core dependencies) @mikro-orm/core ^6.6.10 (core dependencies) dotenv ^17.2.3 (dev dependencies) openapi-types ^12.1.3 (core devDependencies) zod ^4.2.1 (core dependencies) One further package, @google/genai, meets the same criterion but is deliberately left out: upstream already hoisted it to the root in google#564, which this branch's base predates. Re-adding it here would duplicate a change that is already merged upstream. onnxruntime-node is not in scope either: the build_setup native-addon fixtures declare it themselves as "file:./fake-onnxruntime-node" against a checked-in stub and ship an ambient .d.ts for it, and it appears nowhere in package-lock.json. Declaring it at the root would pull a real native addon that nothing resolves to. Every range is already satisfied by the copy npm hoists today, so the installed tree is unchanged: a name@version signature over the whole node_modules/ tree is identical before and after. package-lock.json is regenerated by npm install, never hand-edited.
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
Closes: #issue_number
Related: #issue_number
Problem: Files under
tests/**are owned by the workspace-root package — they are not insidecore/,dev/orintegrations/— so the rootpackage.jsonis the nearest manifest on their resolution path. Five third-party packages they import are declared in no manifest on that path:@google-cloud/vertexaitests/integration/core/package.jsondependencies@mikro-orm/coretests/integration/build_setup/*/db_init_check.*fixturescore/package.jsondependenciesdotenvtests/e2e/dev/package.jsondependenciesopenapi-typestests/e2e/tools/rest_api_tool_auth_e2e_test.ts:11core/package.jsondevDependencieszodtests/e2e/andtests/integration/core/package.jsondependenciesnpm hoists each workspace's dependencies into the repo-root
node_modules/, so Node,tsc, ESLint and vitest all resolve these specifiers by walking up fromtests/**and finding a copy installed on behalf of a different workspace. Nothing in the root manifest records that relationship, which makes the coupling silent and non-local: the moment a workspace drops or renames one of these, the wholetests/suite breaks with no signal thattests/was the consumer.openapi-typesis the sharpest case — it is only a devDependency ofcore, i.e. somethingcoreitself considers disposable, yet a root e2e test imports it. Thebuild_setupfixtures are the second sharpest: they are self-contained npm projects that get a realnpm install, declare only@google/adkand@google/adk-devtools, and still resolve@mikro-orm/coreby escaping to the repo root.Severity is workspace hygiene, not a consumer-facing break: the root package (
name: "adk") is never published, so no installer of@google/adkor@google/adk-devtoolsis affected.Solution: Declare all five in the root
devDependencies, each at the exact range the owning workspace already declares (not a range re-derived from the lockfile), and letnpm installregeneratepackage-lock.json. Mirroring the owner's range keeps the two manifests textually consistent and means the range can never drift from its owner for free:Every range is already satisfied by the copy npm hoists today, so this documents the dependency graph without altering it. This is deliberately the smallest possible shape for the fix: no new script, no CI step, no source or test file touched.
Two packages deliberately left out
@google/genaimeets the same criterion (50 references fromtests/**) but is out of scope: upstreammainalready declares it at the workspace root (package.json:42,"@google/genai": "^2.9.0"), added by commit00f37755— Fix: hoist @google/genai 2.x to the workspace root to end the version duality google/adk-js#564, "fix(deps): hoist @google/genai 2.x to the workspace root". This branch is based on1210acc7, which predates that commit (git merge-base --is-ancestor 00f37755 HEAD→ false), so the package looks phantom only because of where this branch sits, not because the project has an open gap. Re-adding it would duplicate an already-merged upstream change and conflict on the next rebase forward.onnxruntime-nodeis not a phantom. Both native-addon fixtures declare it themselves as"onnxruntime-node": "file:./fake-onnxruntime-node"against a checked-in stub ({"name":"onnxruntime-node","version":"1.0.0","main":"index.cjs"}) and each ships an ambientonnxruntime-node.d.tsfortsc. Consistent with that, it appears nowhere inpackage-lock.json(grep -c onnxruntime-node package-lock.json→0). Declaring it at the root would download a real multi-hundred-MB native addon that nothing resolves to. Verified after this change that nonode_modules/onnxruntime-nodeappears at the repo root.vi.mock('@modelcontextprotocol/sdk/client/index.js', …)andvi.mock('google-auth-library', …)intests/integration/agent_registry/agent_registry_test.tsare not treated as root dependencies. Those specifiers name modules that@google/adkimports, intercepted in the dependency's own graph; the root workspace never loads them, and declaring them at the root would protect nothing (ifcoredropped either,coreitself would be broken first). The rule applied throughout is: staticimport/export … from, dynamicimport(), andrequire()— i.e. modules the root-owned code actually loads.Collision check (performed before any code was written)
gh pr list --repo AmaadMartin/adk-js --state open --limit 300→ 262 open PRs; the changed-file list of every one was fetched and 21 touch the rootpackage.json. Their root-manifest patches were read individually. No open PR declares any of these five packages at the root, so there is no competing implementation. Adjacent-but-not-colliding work, recorded for the reviewer:deps:check/ knip hygiene gate) and Feat: fail the build on phantom dependencies in the published src trees (import/no-extraneous-dependencies) #323 (import/no-extraneous-dependenciesfor the publishedsrctrees) are the guard for this class of bug. Both were kept separate on purpose — folding a CI gate into a 5-line manifest fix would make it unreviewable, and Feat: fail the build on phantom dependencies in the published src trees (import/no-extraneous-dependencies) #323 scopes tocore/src/dev/src, nottests/**."@google/genai": "^2.9.0"to the root manifest, a second reason not to duplicate that line here.Disclosure: one incidental line in the lockfile
The
package-lock.jsondiff is 6 added lines (thepackages[""].devDependenciesmirror) plus one deletion:"dev": trueis dropped from theadm-zipentry. That is pre-existing drift onmain, not a product of this change — running a barenpm installon an otherwise unmodified checkout ofmainproduces exactly that line and nothing else. It is correct:adm-zipis a productiondependencyofcore, so thedevflag was stale, and hand-reverting it would leave the committed lockfile no longer a fixed point ofnpm install. Hand-editing the lockfile was avoided entirely. #345 fixes this same line directly; if it merges first, this hunk simply disappears on rebase.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.
No test was added, and that is deliberate. This change adds zero executable lines — it is a documentation change expressed in JSON — so there is no new line or branch to cover.
vitest.config.ts:109-113scopes coverage tocore/src/**,dev/src/**andintegrations/src/**, none of which is touched, so the configured thresholds (statements 86 / branches 87 / functions 88 / lines 86) are unaffected by construction. A test that walked the filesystem asserting manifest completeness would be the separately-tracked guard task (see #250 / #323), not this one.Falsifiability check — the direct analogue of "prove the test can fail".
npm explain <pkg>reports the requiring parents of an installed package, and it is the check that fails without this edit and passes only with it. Captured on the same checkout by stashing and restoring the change:Proof the installed tree is unchanged. A
sha256overname@versionfor everypackage.jsoninnode_modules/is byte-identical before and after:53f757341af6d16ce256e7b5e0febe9b6a4eeb06f0cab23c22ac11a5bad54fd2.npm installreported no package added, removed, or moved, andnpm lsresolves the same versions as onmain—@google-cloud/vertexai@1.12.0,@mikro-orm/core@6.6.14,dotenv@17.4.2,openapi-types@12.1.3,zod@4.4.3.npm installis idempotent on the committed lockfile (re-running it produces a byte-identical file), and noERESOLVEor peer conflict occurred.Local gate, run on the exact pushed commit:
npm installnpm run buildnpm run lintnpm run format:checknpx prettier --check package.json package-lock.jsonlint-stagedhook will not reformat the manifests)npm run ts:checkmain— see belownpx vitest run --project integration tests/integration/agent_registry/… tests/integration/memory/vertex_ai_memory_bank_service_test.tsnpx vitest run --project integration tests/integration/skillsskills/script_js)npm run ts:checkexits 2 on this base before and after this change. Its output was captured in both states and diffed: byte-identical. The errors are the known pre-existing test-tree type errors that #178 / #207 address; this change neither adds nor fixes any of them.Two suites could not be validated in this sandbox, in the same way on
mainand on this branch:tests/integration/build_setup/build_setup_test.ts(the@mikro-orm/coreandonnxruntime-node-stub consumer) runs a realnpm installper fixture against a 10sbeforeAllhook, which this environment cannot meet, andnpx @google/adk-devtools --versionfails here with "could not determine executable to run". Run from a clean fixture state on both sides, it fails with the same two failure modes either way; the pass/fail split varies run to run purely by how far each run gets before the timeout. Given thenode_modulestree is provably identical, this change cannot affect it.e2eproject needs live credentials;tests/e2e/tools/rest_api_tool_auth_e2e_test.ts(theopenapi-types+dotenvconsumer) fails withAPI key must be provided … GOOGLE_GENAI_API_KEY. Note this failure occurs inside the test body, which confirms the module graph — includingopenapi-typesanddotenv— resolved and loaded successfully.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
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 three
run-testslegs pass on the pushed commit:ubuntu-latest,macos-latest,windows-latest.The first
windows-latestattempt failed and was re-run on the identical commit, where it passed. Both failures were in files this PR does not touch and are known Windows flakes, not regressions:tests/integration/tools/run_skill_script_tool_test.ts:190— "successfully executes a real PowerShell skill script",Test timed out in 5000ms. Upstream addressed exactly this test in Fix: add explicit per-test timeout to real-PowerShell skill-script integration test google/adk-js#550, "add explicit per-test timeout to real-PowerShell skill-script integration test", a commit this branch's base predates.tests/integration/a2a/stream/stream_test.ts—CLI exited prematurely with code 1, a child-process startup race.Neither can be caused by this change: it adds only
devDependenciesdeclarations at ranges already satisfied by the installed copies, and the resultingnode_modules/tree is provably byte-identical (samesha256name@version signature before and after).