Fix: declare zod, @google-cloud/vertexai and openapi-types at the workspace root - #601
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: declare zod, @google-cloud/vertexai and openapi-types at the workspace root#601AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
…he workspace root Files under tests/** are owned by the workspace-root package, so the root package.json is the nearest manifest on their Node resolution path. Three packages they import are declared in no manifest on that path and resolve only because npm workspaces hoists a sibling's dependency into the repo-root node_modules. Declare all three in the root devDependencies at the exact range the owning workspace already uses: zod ^4.2.1 (core dependencies) @google-cloud/vertexai ^1.12.0 (core dependencies) openapi-types ^12.1.3 (core devDependencies) openapi-types is the sharpest case: a root e2e test imports something core itself treats as a dev-only dependency. package-lock.json is regenerated by a plain `npm install`; the change is confined to the root workspace node. The installed tree is unchanged -- every range added is already satisfied by the copy npm hoists today, verified by an identical pre/post signature over node_modules (1086 packages, sha256 4be4f0928146bc39d9659ea8665b13774f525014b06b3b2fe1fa8b2a17eb9044). Follows the @google/genai hoist in google#564.
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 (adk, the manifest atpackage.json) — they are not insidecore/,dev/orintegrations/, so the rootpackage.jsonis the nearest manifest on their Node resolution path. Three third-party packages they import are declared in no manifest on that path:tests/**resolution path?zod^4.2.1core/package.jsondependencies@google-cloud/vertexai^1.12.0core/package.jsondependenciesopenapi-types^12.1.3core/package.jsondevDependenciesThey resolve today only because npm workspaces hoists
core's dependencies into the repo-rootnode_modules/. That coupling is invisible and unenforced — a phantom dependency.openapi-typesis the sharpest case: it is only a devDependency ofcore, i.e. somethingcoreitself treats as disposable, yet a root e2e test imports it.13 import sites are affected:
zod(8):tests/e2e/routing/planning_mode_test.ts,tests/e2e/streaming/streaming_e2e_test.ts,tests/integration/streaming_sse/sse_agent_test.ts,tests/integration/a2a/basic/remote_a2a/weather_time_agent.ts,tests/integration/adk_web/agent/agent.ts,tests/integration/agents/simple_agent_with_tools/agent.ts,tests/integration/skills/inline/agent.ts,tests/integration/streaming/agent.ts@google-cloud/vertexai(4):tests/integration/agents/agent_with_sandbox_executor_test.ts,tests/integration/memory/vertex_ai_memory_bank_service_test.ts,tests/integration/sessions/vertex_ai_session_service_test.ts,tests/integration/tools/agent_tool_vertexai_test.tsopenapi-types(1):tests/e2e/tools/rest_api_tool_auth_e2e_test.tsSeverity is workspace hygiene, not a consumer-facing break: the root package is the workspace host and is never published, so no installer of
@google/adkor@google/adk-devtoolsis affected.Solution: Declare all three in the root
devDependenciesat the exact range the owning workspace already declares (copied verbatim, not re-derived from the lockfile), and let a plainnpm installregeneratepackage-lock.json. This is the direct sibling of the@google/genaihoist that landed as google#564, and reproduces its shape: added lines indevDependencies, plus the mirrored hunk in the lock's root workspace node. Nothing else changes — no source file is touched.Mirroring the owner's range (rather than pinning, widening, or upgrading) keeps the two manifests textually consistent, so the root range cannot silently drift from its owner.
Collision check against open PRs on this fork
Run before implementing, per contribution workflow:
499 open PRs; one overlaps and the reviewer should know about it. PR #360 ("Fix: declare the root workspace's test dependencies in package.json", branch
fix/root-phantom-test-dependencies) is a strict superset of this PR: it adds the same three declarations at the same three ranges, plus@mikro-orm/core(^6.6.10) anddotenv(^17.2.3). The two PRs edit the samedevDependenciesblock in the same two files, so whichever lands second will conflict textually and should be rebased, not merged blind.They are not equivalent in scope. This PR deliberately excludes the two extra packages (see the exclusion list below); #360 includes them. Landing this one and rebasing #360 leaves only the
@mikro-orm/core/dotenvquestion to decide; landing #360 makes this PR redundant and it should be closed. Either order is fine — please just don't merge both without a rebase.Adjacent PRs reviewed and ruled out as non-collisions:
scripts/check_phantom_deps.mjs, a CI gate that detects phantom dependencies. Complementary: it would flag exactly what this PR fixes, and does not declare anything itself.openapi-typesbetween sections withincore/package.json; does not touch the root manifest.devworkspace manifest, not the workspace-root host manifest.Explicitly out of scope (and why)
dotenv— a real phantom dependency (20 files undertests/**, declared only indev/package.jsondependenciesat^17.2.3), being handled separately.@mikro-orm/core— imported only bytests/integration/build_setup/{ts_esm,js_esm}/db_init_check.*. Thosebuild_setupfixtures each ship their ownpackage.jsonand install their own dependencies at test time, so they are not root-owned.onnxruntime-node— the two*_native_addonfixtures declare it themselves asfile:./fake-onnxruntime-nodeagainst a checked-in stub and ship an ambientonnxruntime-node.d.ts. It does not appear inpackage-lock.jsonat all; declaring it at the root would pull a real native addon nothing resolves to.@google-cloud/vertexai/build/src/genai/*.jsimports at 3 of the 4 vertexai sites violate the "import from the package root" guidance. Pre-existing, and fixing it is a source change that would break the manifest-only constraint of this PR. Noted as a known follow-up; several open PRs (Fix: import Client from the @google-cloud/vertexai package root in VertexAiSessionService #474, Chore(lint): forbid deep imports into the @google-cloud/vertexai build output #475, Fix: contain @google-cloud/vertexai build-output deep imports behind one adapter module #303, Fix: drop @google-cloud/vertexai deep build-output imports from the Agent Engine deploy CLI #279) already target it.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. — N/A, deliberately. This change adds no executable code.
vitest.config.tsscopes coverage tocore/src,dev/src,integrations/src, none of which is touched, so the configured thresholds are unmoved. A test asserting the contents ofpackage.jsonwould be test theatre; the verification burden is discharged by the falsifiability and tree-invariance evidence below instead.[x] All unit tests pass locally.
Falsifiability — proving the change is load-bearing
1. Resolution-path check (all 13 sites). For each import site, walk the Node resolution path upwards and report the first manifest declaring the bare specifier. Run against
HEAD(before) and the working tree (after):2.
TS2307experiment (scratch copy: drop the three fromcore/package.json,rm -rf node_modules package-lock.json && npm install, thentsc --noEmit). Filtered totests/**, without a root declaration:Honest caveat on
zod: it does not reproduce under experiment 2, and I am not going to claim it does. Removingzodfromcoreanddevstill leaveszod@4.4.3installed and hoisted, because it is a realdependenciesentry of@modelcontextprotocol/sdk(^3.25 || ^4.0) and a peer ofzod-to-json-schema, both of whichcoredepends on:That makes the
zodexposure worse, not milder, and it is why experiment 1 is the primary evidence:tests/**currently satisfies itszodimport from a transitive dependency of a third-party package, entirely outside first-party control. The range that governs it today is the MCP SDK's^3.25 || ^4.0, which permits zod 3 — a different major with an incompatible API — whilecoreis written against^4.2.1. Declaringzod: ^4.2.1at the root makes the root a direct declarer, so npm must satisfy^4.2.1at the top level regardless of what the SDK resolves to.Tree-invariance proof
The installed dependency tree must not change — every range added is already satisfied by the copy npm hoists today. Signature over
node_modules(count + sha256 of every sortedpath name@version), captured before and afternpm install:Lock entries confirm no resolution or
devflag moved:Regression checks (before → after, same machine, identical
node_modules)npm run buildnpm run lintnpx prettier --check package.json package-lock.jsonnpm run ts:check--project integration(5 files)--project e2e(3 files)Two pre-existing conditions worth flagging, both unchanged by this PR and neither introduced by it:
npm run ts:checkdoes not exit 0 onmain. It reports 281 errors, almost all from test files importing@google/adk(which resolves tocore/disttypes) and comparing them againstcore/srctypes. I verified the error set is byte-identical before and after this change (diffof the error lines is empty), so this PR adds zero new type errors. CI does not runts:check; PR Fix: make npm run ts:check a deterministic, build-independent check (tranche 1/5) #568 targets making it deterministic.API key must be provided via constructor or GOOGLE_GENAI_API_KEY or GEMINI_API_KEY environment variable.Identical counts before and after.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Note on the lockfile: it was regenerated with
npm install --registry=https://registry.npmjs.orgso that every newresolvedURL points at the public registry. All 1167resolvedentries remain onregistry.npmjs.org, unchanged frommain.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. — N/A; the diff is six manifest lines with no code.
[x] I have added tests that prove my fix is effective or that my feature works. — see the falsifiability evidence above; no test file added, deliberately, for the reason stated.
[x] New and existing unit tests pass locally with my changes.