Fix: anchor the vitest workspace aliases so deep @google/adk/* specifiers fail at test time - #380
Open
AmaadMartin wants to merge 3 commits into
Open
Fix: anchor the vitest workspace aliases so deep @google/adk/* specifiers fail at test time#380AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
July 31, 2026 03:26
A string alias in Vite/Rollup matches by prefix, so '@google/adk' also rewrote '@google/adk/sessions/session.js' to core/src/sessions/session.js. core/package.json and integrations/package.json each export only ".", so those deep specifiers resolve for nobody outside the test runner: tsc reports TS2307 and a published-package consumer gets a resolution error, while vitest quietly passed. Switch the six project blocks to one shared array of anchored RegExp aliases so the resolver is no more permissive than the exports map, and migrate the four in-tree deep imports that relied on the old behaviour. Session is public, so it comes from '@google/adk'; logger, isVertexAiConnectionString, quoteFilterLiteral and responseProcessor are not exported, so they move to relative paths into the source tree. Both forms land on the same absolute file, so vi.spyOn(logger, ...) keeps observing the same module instance as the code under test.
Asserts the bare specifiers match exactly one entry each and that the deep forms, plus @google/adk-devtools, match none. Matching goes through `find instanceof RegExp`, so reverting to the prefix-matching string form matches nothing and fails the suite.
@rollup/plugin-alias matches a string `find` as `importee === pattern || importee.startsWith(pattern + '/')`, not as a raw prefix, so the old comment claimed a broader bug than existed: `@google/adk-devtools` was never captured by the `'@google/adk'` key. State the actual mechanism, name the tsc setting that rejects the subpath, and drop the sentence restating the preceding one.
This was referenced Aug 1, 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
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem:
vitest.config.tsdeclared, identically in all six test projects, a string-keyed alias map:A string
findin@rollup/plugin-aliasmatches the package root and every subpath under it — the matcher isimportee === pattern || importee.startsWith(pattern + '/')(vite's bundled copy,node_modules/vite/dist/node/chunks/config.js:8109). So@google/adkwas rewritten tocore/src(intended), but@google/adk/sessions/session.jswas also rewritten tocore/src/sessions/session.js(not intended). Note this is subpath matching, not raw prefix matching: the sibling package@google/adk-devtoolswas never hijacked, since it does not start with@google/adk/.core/package.jsonandintegrations/package.jsoneach declare exactly one export —"."— with no./*subpath. Those deep specifiers therefore resolve for nobody outside the test runner:tscreportsTS2307and a consumer of the published package gets a Node exports-map error. The harness was more permissive than the package manifest, so an unresolvable import could land onmainwith a green CI. Four such imports were already in the tree.Solution: replace the six duplicated object literals with one shared, exported array of anchored
RegExpalias entries. Rollup matches aRegExpfindwithfind.test(importee), so^…$gives exact matching and the resolver becomes no more permissive than the publishedexportsmap. The four in-tree deep imports are migrated in the same commit somainstays green — there is no flag day.@google/adkcore/srccore/src(unchanged)@google/adk-integrationsintegrations/srcintegrations/src(unchanged)@google/adk/<subpath>core/src/<subpath>@google/adk-integrations/<subpath>integrations/src/<subpath>@google/adk-devtoolsWhy the array form and not a stricter string key:
@rollup/plugin-aliashas no "exact" mode for string keys, and both patterns are anchored with no capture groups, so they cannot overlap and ordering is irrelevant.path.resolve(__dirname, …)is kept verbatim sowindows-latestin the CI matrix behaves as it does today.Migrated imports. Public symbol → package name; non-exported symbol → relative path into the source tree, which is the existing
core/testconvention (../../src/utils/logger.jsis already used by 11 other files there).core/test/sessions/vertex_ai_session_service_test.tsSessionis public (core/src/common.ts:226, re-exported bycore/src/index.ts) and used only in type position, so it becomesimport type {Session} from '@google/adk';.isVertexAiConnectionString/quoteFilterLiteral/loggerare not in theexportssurface (core/src/common.ts:283-284exports onlyLogLevel,getLogger,setLogLevel,setLoggerand theLoggertype), so they move to../../src/....tests/integration/agents/agent_with_sandbox_executor_test.tsresponseProcessor(core/src/agents/processors/code_execution_request_processor.ts:165) is internal, so it moves to../../../core/src/.... It was deliberately not added tocore/src/index.ts/core/src/common.ts: widening the public API to make a test import prettier would be an API change smuggled into a test-config fix. The relative path is the intended outcome — it makes the test's reach into package internals visible.Module identity is preserved.
vi.spyOn(logger, 'error')(lines 380/474/505/590) still observes the same object the code under test uses:core/test/sessions/../../src/utils/logger.jsandcore/src/sessions/../utils/logger.jsare the same absolute file, and the alias collapses@google/adkontocore/srcas well. No assertion in either migrated file was touched — only specifier strings.Collision check (
gh pr list --repo AmaadMartin/adk-js --state open --limit 100, thengh pr diffon every plausibly adjacent PR):fix/ban-deep-package-importsoverlaps: it adds an ESLintno-restricted-importsban on@google/adk/*and makes the identical one-line rewrite totests/integration/agents/agent_with_sandbox_executor_test.ts. It does not touchvitest.config.tsor the other three deep imports, so it does not land this change. The two are complementary layers — a lint guard vs a resolver guard — and neither is a prerequisite for the other. I did not stack on it: its branch is behindmainand conflicts with currentmainin unrelated files, stacking would suppress CI (the workflow triggers on basemain), and a test merge confirms the one genuinely shared hunk auto-merges cleanly because both sides write the same line.vitest.config.ts, but each edits a different key (unstubEnvs, coverage thresholds,globalSetup, coverageinclude); none changes thealiasform.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.
New file
tests/integration/vitest_alias_test.ts(2 tests) asserts onworkspaceAliasesdirectly: each bare specifier matches exactly one entry;@google/adk/sessions/session.js,@google/adk/agents/processors/code_execution_request_processor.jsand@google/adk-integrations/foo.jsmatch none; and each entry resolves to the right source root (derived fromimport.meta.url, not hardcoded). Matching goes throughentry.find instanceof RegExp && entry.find.test(specifier), mirroring@rollup/plugin-alias, so a silent revert to the string form matches nothing and fails.The test also asserts
@google/adk-devtoolsmatches no entry. That is not evidence of the original bug — the old string alias never captured it — but a guard against the obvious wrong fix: an unanchored/^@google\/adk/would swallow both sibling packages.Commands run on the pushed commit:
npx vitest run --project unit:core --project unit:devnpx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.tsnpx vitest run --project integration tests/integration/vitest_alias_test.ts tests/integration/agents/agent_with_sandbox_executor_test.tsnpx vitest run --project unit:integrationsnpm run buildnpm run lintnpm run format:checkProof the new tests can fail. Three separate mutations of
workspaceAliases, each reverted afterwards:TypeError: workspaceAliases.filter is not a function— 2 failed.finda string (i.e. restore prefix matching) →AssertionError: expected [] to have a length of 1 but got +0andAssertionError: expected [] to deeply equal [ { find: Any<RegExp>, …(1) } ]— 2 failed.$from the@google/adkpattern →AssertionError: expected [ …(2) ] to have a length of 1 but got 2— 2 failed (the unanchored pattern swallows@google/adk-integrationsand the deep forms).ts:checkdelta (reported as a delta only —npm run ts:checkis not a CI gate and its overall baseline is a separate task). The fourTS2307errors are gone:280total errors onmain→277on this branch.Disclosed honestly: removing the
TS2307atagent_with_sandbox_executor_test.ts:9surfaces one previously-masked error at line 95,TS2322: Type 'CodeExecutionResponseProcessor' is not assignable to type 'BaseLlmResponseProcessor'. The unresolvable import used to typeresponseProcessorasany. The real cause is thattscresolves@google/adkthrough theexportsmap tocore/dist/typeswhile the relative import points atcore/src, so it sees two declarations ofInvocationContext(Types have separate declarations of a private property 'updateSessionState'). That split predates this change and applies to any test file mixing the two, and it does not affect what runs: under vitest the alias collapses both ontocore/src, which is why the test passes. Net for the file is unchanged (1 error before, 1 after) and net for the repo is −3. I did not "fix" it by importingLlmAgentfrom a relative path too — that would trade atsc-only artifact for a real regression in import hygiene.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
From the repo root, after
npm install && npm run build:npm run test:unit— passes (covers the rewrittenvertex_ai_session_service_test.ts).npm run test:integration— passes (covers the rewrittenagent_with_sandbox_executor_test.tsand the newvitest_alias_test.ts).npx vitest run --project unit:integrations— the only project that exercises the@google/adk-integrationsalias (integrations/test/version_test.tsimports the bare specifier). The module resolves; see the pre-existing failure note below.core/test/and rununit:core. It now fails to resolve, with Vite falling through to Node's exports-map check:Reverting only
vitest.config.tsto the string alias form makes that exact file pass (Test Files 1 passed) — that asymmetry is the bug this PR removes. The temporary file was deleted; it is not part of the diff.Note the import must be a value import: an
import typespecifier is erased before resolution and never reaches the resolver, so it neither fails today nor failed before.Two pre-existing failures found while testing, both reproduced on unmodified
mainand both unrelated to this change:integrations/test/version_test.tsassertsexpect(version).toBe('1.3.0')whileintegrations/src/version.tsis'1.5.0'.unit:integrationsis not part ofnpm run test:coverage, which is why CI does not see it. Queued as separate work rather than fixed here.dev/test/cli/cli_create_test.ts:214reads an ambient gcloud project from the developer machine (initialValue: 'gcloud-project'vs'global').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 checks green on
9ac5a3f8:run-testsonubuntu-latest,macos-latestandwindows-latest, pluscheck-licenseandauto-assign.The first
macos-latestattempt failed ontests/integration/app_loader/app_loader_test.tswithError: Test timed out in 40000msat thenpm installthe fixture shells out to (TEST_EXECUTION_TIMEOUT = 40000);windows-latestwas then cancelled byfail-fast. That file is not in this diff and imports nothing this PR touches,ubuntu-latestpassed the same commit, and re-running the two legs unchanged turned both green — a slow-runner flake, already the subject of #276 (drop thenpm installfrom that fixture).