Skip to content

Chore: fail lint on deep @google/adk* subpath imports (stacked on #380) - #435

Open
AmaadMartin wants to merge 4 commits into
fix/vitest-alias-exact-matchfrom
fix/remove-deep-package-subpath-imports
Open

Chore: fail lint on deep @google/adk* subpath imports (stacked on #380)#435
AmaadMartin wants to merge 4 commits into
fix/vitest-alias-exact-matchfrom
fix/remove-deep-package-subpath-imports

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Related: Fix: anchor the vitest workspace aliases so deep @google/adk/* specifiers fail at test time #380

  2. Or, if no issue exists, describe the change:

Collision check (done before writing any code). The task assigned to this
branch was "remove the four unresolvable deep-subpath @google/adk* imports".
gh pr list --repo AmaadMartin/adk-js --state open --limit 100 plus
gh pr diff --name-only over every plausibly adjacent PR found that #380
(fix/vitest-alias-exact-match) already rewrites all four of those imports
,
in the same two files, to the same targets. Rather than ship a competing
implementation, this PR is stacked on #380 and contributes only the part
#380 does not have: the regression guard. No open PR adds a
no-restricted-imports rule (checked #426, #422, #346, #332, #333 — the other
PRs that touch eslint.config.js; they add the node: protocol rule and
type-aware linting). Stacking is also load-bearing rather than merely polite:
on main the four deep imports still exist, so this rule would fail
npm run lint if it landed there on its own.

Problem: core/package.json, dev/package.json and
integrations/package.json each declare an exports map with only a "."
entry, so @google/adk/<anything> is not a resolvable specifier for a
consumer, for tsc under moduleResolution: nodenext, or for node. Four
call sites imported one anyway. Nothing in the repo failed, because every gate
looks away:

  • vitest.config.ts aliased '@google/adk' as a string find, and Vite
    treats a string alias as matching the specifier or anything starting with
    find + '/'
    — so @google/adk/sessions/session.js was silently rewritten to
    core/src/sessions/session.js and the exports map was never consulted.
    (Fix: anchor the vitest workspace aliases so deep @google/adk/* specifiers fail at test time #380 anchors those aliases.)
  • .github/workflows/validation.yaml does not run npm run ts:check; the only
    compilation in CI is per-package tsc --emitDeclarationOnly over src/**,
    which never sees core/test/** or tests/**.
  • npm run docs:check (TypeDoc, entryPoints: ["./core/src/index.ts"],
    exclude: ["**/*_test.ts", "**/test"]) and npm run lint (non-type-checked
    typescript-eslint recommended) do not resolve module specifiers at all.

Removing the four call sites without a guard leaves nothing to stop the fifth.

Solution: Direction (a) — deep subpath imports into @google/adk* are
not public API, and lint now says so.
One no-restricted-imports group,
'@google/adk*/**', rejects any subpath of @google/adk,
@google/adk-devtools, @google/adk-integrations and any future
@google/adk-* workspace package, with a message that names the cause and both
supported alternatives. Package roots stay allowed.

Why (a) rather than adding a "./*" export condition:

  1. Nothing in the repo, the docs or the README depends on deep imports outside
    those four lines, and they have never worked outside Vitest.
  2. A "./*" condition would make every file path under dist/ part of the
    packages' semver surface — the whole internal module tree, for three
    packages, effectively forever — to serve four test-only imports.
  3. None of the four symbols is public API: quoteFilterLiteral is an AIP-160
    literal-escaping helper, isVertexAiConnectionString is a URI predicate used
    only by core/src/sessions/registry.ts, responseProcessor mirrors a
    module-private singleton in google/adk-python
    (src/google/adk/flows/llm_flows/_code_execution.py), and logger already
    has public accessors (getLogger, setLogger, setLogLevel, LogLevel,
    Logger) exported from core/src/common.ts.

The public API surface is byte-identical. Nothing was added to
core/src/index.ts or core/src/common.ts; npm run docs:check passes
unchanged. No package manifest, no dist/ layout, no runtime behaviour is
touched by this PR — the entire diff is eslint.config.js plus one test.

On the plan's glob claim — corrected by measurement, not assumed. The brief
asserted that ESLint's patterns.group matching "does not cross / with a
single *", making a trailing ** mandatory. ESLint 9 builds an ignore()
matcher, i.e. gitignore semantics: a * does not itself cross /, but a match
on a parent path also covers everything below it, so @google/adk/* blocks
@google/adk/utils/logger.js all the same. Verified by mutating the config and
re-running the suite. The trailing ** is kept because it states the intent
unambiguously and because a/** does not match bare a — that is what keeps
the package roots allowed.

Review round 1 (complexity). Two findings, both applied: the three
enumerated package globs collapsed into one '@google/adk*/**' (−4 lines, and
a future @google/adk-<new> package is now covered instead of silently
escaping), and the new suite moved to tests/integration/repo_config/, where
repo-tooling tests belong. The reviewer also asked for
tests/integration/vitest_alias_test.ts to move alongside it; that file belongs
to #380, this PR's base, so moving it here would put a rename of another PR's
file into this diff and conflict with any revision of it. It should move when
#380 lands or in its own change.

Whole-file Prettier reformat, isolated in its own commit. lint-staged
runs prettier --write over **/*.{js,ts}, but npm run format only targets
**/*.ts, so eslint.config.js had never been formatted and any commit
touching it drags a 17-line quote-style reformat along. Commit
Chore: format eslint.config.js ... is that reformat with no behaviour change;
the rule itself is a clean +16 on top of it.

Out of scope, deliberately (stated rather than silently dropped):

  • LlmAgent defaults responseProcessors to [], which is why the sandbox
    integration test wires responseProcessor by hand. That gap is tracked
    separately and is not addressed here.
  • npm run ts:check reports 277 pre-existing errors on this branch, none of
    them TS2307. One is a direct consequence of Fix: anchor the vitest workspace aliases so deep @google/adk/* specifiers fail at test time #380's rewrite and is worth
    flagging: at tests/integration/agents/agent_with_sandbox_executor_test.ts:95
    the previously-unresolvable import used to yield TS2307, and now yields
    TS2322LlmAgent comes from @google/adk (which tsc resolves to
    core/dist/types/**) while responseProcessor now comes from
    core/src/**, and BaseSessionService has a private member, so the two
    declarations are nominally distinct. The root cause is the root type check
    resolving workspace packages to dist rather than to source; that is exactly
    what Fix: gate CI on a root type check that resolves workspace packages to source #414 changes, so it is not duplicated here.

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.

tests/integration/repo_config/eslint_restricted_imports_test.ts (new) lints an in-memory
import statement with the repo's own config and asserts the package roots are
accepted while a subpath at every depth of all three packages is rejected.

Targeted runs (no whole-repo suite):

npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts
#   Test Files  1 passed (1)      Tests  54 passed (54)
npx vitest run --project integration \
  tests/integration/agents/agent_with_sandbox_executor_test.ts \
  tests/integration/vitest_alias_test.ts \
  tests/integration/repo_config/eslint_restricted_imports_test.ts
#   Test Files  3 passed (3)      Tests  5 passed (5)

Whole-repo gates (these are what prove the rule does not false-positive on the
126 legitimate @google/adk root imports in core/test alone):

npm run lint          # exit 0
npm run format:check  # All matched files use Prettier code style!
npm run docs:check    # exit 0 (no public API/TypeDoc surface moved)

Proof the new test can fail (mutations applied to eslint.config.js, each
reverted afterwards):

Mutation Result
Delete the no-restricted-imports rule × rejects a subpath at any depth of any published packageexpected [] to have a length of 1 but got +0
Widen the group to '@google/adk*' (dropping /**, so it also covers the roots) × allows the package root specifiersexpected [ Array(1) ] to deeply equal []
Drop the package-name wildcard, '@google/adk/**' (so the other two packages escape) × rejects a subpath at any depth of any published packageexpected [] to have a length of 1 but got +0
Narrow a trailing '/**' to '/*' stayed green — the measurement behind the glob-semantics correction above

ts:check before/after. With the packages built, npm run ts:check was run
against the pre-fix versions of the two test files and against this branch. The
four TS2307s are gone; the remaining errors are pre-existing and untouched
(the error-count delta also includes line-number shifts, because #380's base
predates the ttl/expireTime tests on main):

# before (pre-fix test files):
core/test/sessions/vertex_ai_session_service_test.ts:9:23  - error TS2307: Cannot find module '@google/adk/sessions/session.js' ...
core/test/sessions/vertex_ai_session_service_test.ts:27:8  - error TS2307: Cannot find module '@google/adk/sessions/vertex_ai_session_service.js' ...
core/test/sessions/vertex_ai_session_service_test.ts:28:22 - error TS2307: Cannot find module '@google/adk/utils/logger.js' ...
tests/integration/agents/agent_with_sandbox_executor_test.ts:9:33 - error TS2307: Cannot find module '@google/adk/agents/processors/code_execution_request_processor.js' ...

# after: 0 TS2307 across the whole repo (277 unrelated pre-existing errors remain)

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Vitest cannot prove anything here, because its alias was the bug. The check
below uses node against a genuinely installed package — the ts_esm
build_setup fixture depends on "@google/adk": "file:../../../../core":

npm install && npm run build
cd tests/integration/build_setup/ts_esm && npm install

node -e "import('@google/adk/utils/logger.js').then(()=>console.log('RESOLVED')).catch(e=>console.log('FAILED', e.code))"
# FAILED ERR_PACKAGE_PATH_NOT_EXPORTED

node -e "import('@google/adk/agents/processors/code_execution_request_processor.js').then(()=>console.log('RESOLVED')).catch(e=>console.log('FAILED', e.code))"
# FAILED ERR_PACKAGE_PATH_NOT_EXPORTED

node -e "import('@google/adk').then(m=>console.log('ROOT_OK', typeof m.getLogger)).catch(e=>console.log('ROOT_FAILED', e.code))"
# ROOT_OK function

rm -rf node_modules package-lock.json   # leave the tree pristine

That is the behaviour the lint rule now encodes: the package root resolves, any
subpath does not. No permanent build_setup fixture assertion was added — it
would buy a seventh npm install cycle in CI to restate what the exports map
already enforces.

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 status: absent (validated locally)

.github/workflows/validation.yaml triggers on pull_request: branches: [main]
only, so a stacked PR based on fix/vitest-alias-exact-match never starts the
test workflow — gh pr checks shows just auto-assign. Reporting that as
"green" would be false. Everything below was therefore run locally against the
exact pushed commit a7a92ea7:

Command Result
npm run build exit 0
npm run lint exit 0
npm run format:check exit 0 — all matched files use Prettier code style
npm run docs:check exit 0
npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts 1 file, 54 tests passed
npx vitest run --project integration tests/integration/{agents/agent_with_sandbox_executor,vitest_alias,repo_config/eslint_restricted_imports}_test.ts 3 files, 5 tests passed

Once #380 merges, retargeting this PR at main will let the full workflow run.

Amaad Martin added 4 commits July 31, 2026 20:53
The repo's lint-staged hook runs `prettier --write` over `**/*.{js,ts}`, but
`npm run format` only targets `**/*.ts`, so this file had never been
formatted and any commit touching it drags a whole-file reformat along.
Doing it on its own keeps the next commit reviewable. No behavior change.
The three published packages export only "." from their exports maps, so
`@google/adk/<anything>` is unresolvable for a consumer, for tsc under
nodenext, and for node. The four call sites that used one are removed on the
base branch; this rule stops them coming back. Package-root specifiers stay
allowed; every subpath under the three packages is an error.
Lints an in-memory import statement with the repo's own config, so deleting
the rule or widening its group to cover the package roots fails the suite
instead of quietly leaving the guard inert.
One glob replaces the three enumerated package names and picks up any future
@google/adk-* workspace package instead of letting it escape the rule. The
test moves next to the other repo-tooling suites.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant