Fix: declare @google/genai in the build_setup integration fixture manifests - #329
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: declare @google/genai in the build_setup integration fixture manifests#329AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
…ifests
All six tests/integration/build_setup fixtures import @google/genai in their
agent entry point, but none declared it. Each fixture is a self-contained npm
project that build_setup_test.ts installs and builds for real, so resolution
succeeded only as a side effect of npm hoisting a transitive copy.
That copy is contested: core declares ^2.9.0 while @google-cloud/vertexai@1.12.0
declares ^1.45.0, and nothing pins which one lands at the fixture node_modules
root -- the exact slot the fixture's own import resolves from. Declaring the
dependency makes npm place a 2.x there deterministically, and makes the fixtures
survive core dropping or bumping the dependency.
The range mirrors core/package.json ("@google/genai": "^2.9.0").
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
No existing issue.
Problem: All six
tests/integration/build_setupfixtures import@google/genaiin their agent entry point, but none of them declare it in their ownpackage.json:ts_esm/agent.ts:15import {createModelContent, GenerateContentResponse} from '@google/genai';ts_esm_native_addon/agent.ts:14import {...} from '@google/genai';ts_commonjs_native_addon/agent.ts:14import {...} from '@google/genai';ts_commonjs/agent.ts:17require('@google/genai')js_esm/agent.js:13import {...} from '@google/genai';js_commonjs/agent.js:13require('@google/genai')Each fixture is a self-contained npm project that
build_setup_test.tsinstalls (:29) and builds (:34) for real — it is a miniature user project. Resolution of@google/genaicurrently succeeds only as a side effect of npm hoisting a transitive copy, or of the fixture directory happening to live under a repo root that has its ownnode_modules. Neither is something a real user copyingts_esmwould get.Two concrete consequences:
core/package.json:48declares"@google/genai": "^2.9.0", butcorealso declares"@google-cloud/vertexai": "^1.12.0", and@google-cloud/vertexai@1.12.0declares"@google/genai": "^1.45.0". Two incompatible ranges compete for the single hoisted slot at the fixture'snode_modulesroot — the exact slot the fixture's own import resolves from. The committed root lockfile shows that collision resolving againstcore:package-lock.json:1608puts1.52.0at the top level, whilecore's2.9.0is pushed down tocore/node_modules/@google/genai(package-lock.json:91). Verified on disk after a root install: root copy1.52.0,core/node_modulescopy2.9.0.coredrops@google/genaior bumps its major, the fixtures break withTS2307/ERR_MODULE_NOT_FOUNDeven though nothing in the fixtures changed — and, worse, they can silently keep compiling against a copy resolved from outside the fixture (demonstrated below).Solution: Declare
"@google/genai": "^2.9.0"in each of the six fixture manifests. That is the whole change — six one-line additions todependencies, 10 insertions total, no executable code touched.Why this solution:
node_modules, so the declaration deterministically pins which copy the fixture's import resolves from, instead of leaving it to a hoist race.core/package.json:48("@google/genai": "^2.9.0") — not invented, not pinned exactly, not*. It was^2.9.0at implementation time, so there is no discrepancy to report."@google/adk-devtools"(and before"onnxruntime-node"where present), preserving the alphabetical ordering already used in all six files.Deliberately not done, to keep the diff minimal:
agent.ts/agent.js— the imports are correct as written.core/package.json,dev/package.json, or the rootpackage.json; the root-manifest anddevgaps are separately tracked.tests/**/package-lock.jsoncommitted —.gitignore:8ignores it by design, and each fixture install is torn down inafterAll(build_setup_test.ts:111-122).overridesblock (separately tracked; there are currently none anywhere in the repo).build_setup_test.ts(see the note on the pre-existing hook timeout below).package-lock.json. Running the rootnpm installlocally produced one incidental hunk (a"dev": trueflag flip onadm-zip); it was reverted, since this change adds no workspace dependency.Scope note (6 fixtures, not 4). The originating task claimed the two
js_*fixtures do not import@google/genaiand need no change. That is false against this checkout —js_commonjs/agent.js:13andjs_esm/agent.js:13both import it (see the table above). Both have the identical defect and the identical one-line fix, so they are included here rather than left as a knowingly-open defect requiring a second review cycle for two lines.Collision check. Before implementing, all 232 open PRs on the fork were scanned for file overlap (
gh pr listplus a GraphQL query over every open PR's file list). No open PR touches anytests/integration/build_setup/*/package.json. Six PRs touchbuild_setup_test.ts(#75, #106, #117, #129, #218, #257) — this PR does not touch that file, so there is no overlap and no need to stack. The nearby@google/genaidedupe / manifest PRs (#226, #228, #244, #274) and the fixture-install PRs (#276, #299) were diffed individually and are all confined to the root/workspace manifests or to other fixture trees; none of them declares@google/genaiin abuild_setupfixture. This PR therefore branches frommain.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.
[x] All unit tests pass locally.
No unit test was added, deliberately. This change adds zero lines of executable code — it is six one-line additions to JSON manifests. There is no statement, branch, or function for a unit test to cover, and the coverage config (
vitest.config.ts,coverage.include) only instrumentscore/src/**,dev/src/**,integrations/src/**, so coverage is unaffected in either direction. A test that read these JSON files back and asserted the literal string just written would be a tautology against the diff: it would pass regardless of whether dependency resolution is actually correct. The meaningful verification is the existing integration suite plus the falsification experiment below.Existing integration suite — targeted run, all six fixtures, no changes to the test file:
Result:
Test Files 1 passed (1)/Tests 20 passed | 4 skipped (24). The 4 skips are the suite's ownit.skipIf, which excludes the two*_native_addonfixtures from thetest:db/test:devtoolscases. This exercises, per fixture: a realnpm install,tscfor the fourts_*,npm run startagainst the mock LLM,npm run test:db/npm run test:devtools, andnpx @google/adk-devtools --version.One caveat on how that run was invoked, stated plainly. With the committed config the suite fails before reaching any assertion:
beforeAll(:28) runs a realnpm installand passes no explicit timeout, so it inherits vitest's 10s default. This is pre-existing and unrelated to this change — verified by reverting all six manifests tomainand re-running the identical command, which fails identically (Hook timed out in 10000ms,24 skipped). It is already tracked by open PRs #117 and #257. Rather than modifybuild_setup_test.ts(out of scope, and it would collide with those PRs), the run above was performed through a local, uncommitted vitest config identical to theintegrationproject but withhookTimeout: 900_000. Nothing in the repo was changed to obtain the green run.Repo-wide checks on the exact pushed commit:
npm run build→ exit 0;npm run lint(eslint "**/*.ts") → exit 0;npm run format:check→All matched files use Prettier code style!.Manual End-to-End (E2E) Tests:
Two experiments prove the change is load-bearing rather than cosmetic. Both were run on
ts_esm; reproduce from the repo root afternpm install && npm run build.Proof 1 — the declaration takes effect.
Before (fixture undeclared) — the fixture is not a dependent; the copy at its
node_modulesroot is whatever won the hoist:After — the fixture itself is a direct dependent, and the root copy is pinned to a
2.x:Proof 2 — mutation / falsification: the fixture survives
coredropping the dependency. Mutation applied: temporarily delete both@google/genaiand@google-cloud/vertexaifromcore/package.jsondependencies(these are the only two providers of@google/genaiin the fixture tree), then reinstall the fixture.core/package.jsonwas restored afterwards and is not part of this diff.Sub-case A — with the repo-root
node_modulesstill present. Both variants build, but they resolve to different files.tsc --traceResolutionin the unfixed fixture:It escaped the fixture entirely and compiled against 1.52.0 — a major
coredoes not use — from a directory that exists only because the fixture happens to sit inside this repo. The fixed fixture, same conditions:Fixture-local, correct major. This ancestor-
node_modulesfallback is the finding that most justifies the change, and it also means the plain "does it build" check is not sufficient to detect the defect — hence sub-case B.Sub-case B — with the repo-root copy also moved aside, i.e. modelling a genuine user project. Unfixed fixture:
npm run buildexit code 1. Fixed fixture, identical conditions:tscreports no errors,Build complete, exit code 0, fixture-local@google/genaiat2.15.0.So the change flips a hard
TS2307build failure into a pass, and in the softer case flips a silent wrong-major resolution into a correct fixture-local one. All fixturenode_modules,dist, andpackage-lock.jsonwere removed afterwards;git statusis clean apart from the six intended manifests.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[ ] 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.
Notes on the two unticked boxes:
"@google/genai": "^2.9.0"lines in JSON manifests, which admits no comments and needs none; the rationale is in this description instead.TS2307and the fixed one passing.No type-checker or linter suppressions were added by this change (
@ts-expect-error,@ts-ignore,eslint-disable,as any,as never, coverage-ignore): the diff contains no code. Verified with a grep overgit diff main.CI
All checks pass on this PR:
run-testsonubuntu-latest,macos-latestandwindows-latest, pluscheck-licenseandauto-assign. Overall run conclusionsuccess.Disclosure — the Windows leg was flaky and needed re-runs (2 fail, then pass), and that is worth explaining rather than hiding. Both failures were the same pre-existing 10s
beforeAlltimeout described above (build_setup_test.ts:28,Hook timed out in 10000ms), affecting 1 fixture on the first run and 3 on the second. Evidence that this is a pre-existing flake and not a regression introduced here:windows-latestwith the byte-identical error at the same line, onts_esm_native_addon. Sampling recent Windows legs (Fix: require TEST_API_SERVER_PORT in the A2A multi-hop test agent #314, Feat: Port the typed-errors module (errors/) from adk-python #317, Feat: Port the environment abstraction (BaseEnvironment, ExecutionResult, LocalEnvironment) from adk-python #319, Feat: Workflow node model — BaseNode, FunctionNode, JoinNode, NodeContext (Part 1/3) #320, Feat: fail the build on phantom dependencies in the published src trees (import/no-extraneous-dependencies) #323, Fix: raise vitest coverage thresholds to the measured worst-leg floor #324, Fix: point ts:check at a program that type-checks sources, not build output #326) shows the suite passing but taking 124–166s for six fixtures — the per-fixturebeforeAllsits close to its 10s budget on Windows, so it tips over at random.ts_esmwith and without the declaration produces an identical tree:added 606 packagesboth times, 593 installed package directories, 423M on disk, and exactly 2 copies of@google/genai(2.15.0at the fixture root,1.52.0nested under@google-cloud/vertexai) in both cases. The copy npm previously hoisted by accident is the same copy it now places deliberately, so nothing extra is fetched or written. Wall-clock was 88.4s undeclared vs 71.4s declared — i.e. dominated by network noise, and if anything faster with the fix.The proper fix is an explicit timeout on that hook, which is already the subject of open PRs #117 and #257. Duplicating it here would create a competing implementation, so
build_setup_test.tsis deliberately left untouched.