Skip to content

Chore(lint): forbid deep imports into the @google-cloud/vertexai build output - #475

Open
AmaadMartin wants to merge 2 commits into
mainfrom
feat/lint-restrict-vertexai-deep-imports
Open

Chore(lint): forbid deep imports into the @google-cloud/vertexai build output#475
AmaadMartin wants to merge 2 commits into
mainfrom
feat/lint-restrict-vertexai-deep-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):
    N/A — no existing issue.
  2. Or, if no issue exists, describe the change:
    Problem: 13 import statements across 8 files reach into the compiled output of a third-party dependency, at @google-cloud/vertexai/build/src/genai/*.js. @google-cloud/vertexai@1.12.0 declares "main": "build/src/index.js" and no exports map (verified: node -e "console.log(require('@google-cloud/vertexai/package.json').exports)"undefined), which is the only reason those subpaths resolve at all. They are the dependency's private layout and break whenever it reorganises its output. The repository's TypeScript guidance already states the rule ("Import main classes directly from the root package … instead of deep paths"), but it is only prose: nothing stops a contributor adding another one, and nothing points them at the public entry point when one exists.

Solution: turn the prose into an enforced lint rule, and clean up the imports that violate it.

  1. Add @typescript-eslint/no-restricted-imports to eslint.config.js restricting the @google-cloud/vertexai/build/** subtree, with two escape valves:

    • allowTypeImports: true — most of the Agent Engine genai types have no root-level re-export, and a type-only import is erased at compile time so it can never break at runtime;
    • allowImportNames: ["Sessions", "Language"] — the only two runtime symbols the repo needs that the package root does not export.

    The allow-list is deliberate: it is a single reviewable ledger of "symbols we can only reach through the dependency's private build output", and it replaces what would otherwise be scattered eslint-disable comments (an automatic reject under this repo's guidelines). No suppression of any kind is added by this PR — no eslint-disable, no @ts-expect-error, no as any, no files-scoped override.

    The usual companion "no-restricted-imports": "off" line is deliberately not included: unlike no-unused-vars (which js/recommended does enable, which is why the existing off beside it is load-bearing), the base no-restricted-imports is not enabled by @eslint/js's recommended nor by tseslint.configs.recommended, so disabling it would be dead config. Verified two ways — 'no-restricted-imports' in require('@eslint/js').configs.recommended.rules is false, and the 15 errors below are all reported under @typescript-eslint/no-restricted-imports with zero base-rule duplicates.

  2. Convert every remaining offending import: Client moves to the package root (@google-cloud/vertexai re-exports it — verified by enumerating the root module's exports at runtime), and the type-only deep imports become import type.

  3. Retarget the one vi.mock() specifier coupled to a deep path being moved.

What the package root actually exports (enumerated at runtime from the installed @google-cloud/vertexai@1.12.0): BlockedReason, ChatSession, ChatSessionPreview, Client, ClientError, FinishReason, FunctionCallingMode, FunctionDeclarationSchemaType, GenerateContentResponseHandler, GenerativeModel, GenerativeModelPreview, GoogleApiError, GoogleAuthError, GoogleGenerativeAIError, HarmBlockThreshold, HarmCategory, HarmProbability, HarmSeverity, IllegalArgumentError, Mode, SchemaType, VertexAI. So Client is reachable from the root; Sessions, Memories, Language, ReasoningEngine, SessionEvent, Session and the rest of the Agent Engine genai surface are not.

Deliberately left on deep specifiers (they are the allow-listed value imports; converting them breaks the build):

  • core/src/code_executors/agent_engine_sandbox_code_executor.ts:8Language is export declare enum Language and is dereferenced as a runtime value (Language.LANGUAGE_PYTHON) at lines 79–81.
  • tests/integration/sessions/vertex_ai_session_service_test.ts:7Sessions is constructed (new Sessions(apiClient)) at lines 59 and 124.

No runtime behaviour change. The root entry point does require('./genai/client'), so Client imported from the root is the same class object from the same module instance as Client imported from build/src/genai/client.js — no dual-identity hazard. import type is erased at compile time. No test assertion needed editing, no test was skipped, weakened or deleted, and no production line was added or removed (coverage thresholds are unaffected).

Commit prefix: chore(lint): rather than feat:/fix:, because release-please (release-type: node) would otherwise emit a user-facing changelog entry and a version bump for a change with no user-visible effect.

eslint.config.js is not reformatted. The file predates the repo's prettier config and npm run format:check (which globs **/*.ts only) never sees it, so main has carried it unformatted all along. The lint-staged hook covers **/*.{js,ts} and would reflow all 39 lines on any commit that touches it; the rule commit therefore uses --no-verify and the new block is written in the file's existing double-quote style. The diff on this file is a pure 16-line insertion with zero deletions.

Known limitation (stated, not worked around). The rule catches named, default, namespace (import * as) and re-export (export … from, export * from) forms — all verified below. It does not flag a bare side-effect import (import '@google-cloud/vertexai/build/…';), because allowImportNames only inspects named bindings and a side-effect import has none. That form imports nothing and does not appear in the repository, so it is not worth a second pattern entry.

Collision check against open fork PRs (required)

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 (376 open PRs), filtered for titles matching vertexai|vertex|eslint|lint|import, plus gh pr diff <n> --name-only on every plausibly adjacent one. Four overlap; none lands this change, because none adds any lint enforcement for @google-cloud/vertexai:

PR Relationship Handling
#303 fix/vertexai-deep-import-adapter Alternative design for the same problem. Adds core/src/utils/vertex_ai_internal.ts (35 lines) + a test (27 lines) that centralises the deep specifiers behind an adapter and renames the re-exported types (VertexAiSession, VertexAiEventMetadata, VertexAiLanguage). Touches 6 of my 8 files. Not stacked on — mutually exclusive, only one should land. Stacking was considered and rejected because the two designs contradict: #303's premise is that deep specifiers live in exactly one module, while this PR's rule permits type-only deep imports anywhere. Stacked, the adapter would exist with nothing forcing its use. This PR is the smaller of the two (0 new files, 0 new symbols, 0 renames) and is the only one of the pair that prevents the next violation. If #303 is preferred, note its adapter passes this rule unchanged (export type {…}allowTypeImports; export {Language as …} → allow-listed — both verified below), so the rule can be rebased on top of it.
#279 fix/vertexai-root-import-deploy-cli Partial duplicate: same two files (dev/src/cli/deploy/cli_deploy_agent_engine.ts, dev/test/cli/cli_deploy_agent_engine_test.ts), same root-import fix, same vi.mock retarget. It additionally deletes the ReasoningEngine type import and replaces the typed cast with apiResponse.response!. Overlapping hunk kept, because it is a precondition for npm run lint to pass here; this PR keeps ReasoningEngine as import type rather than dropping the type. Whichever lands first, the other is a two-line conflict.
#474 fix/vertexai-session-service-root-client-import Subset: exactly one line of this diff — core/src/sessions/vertex_ai_session_service.ts:7, the same Client → package-root move. Opened while this branch was being built, so it post-dates the initial scan; caught on the re-check before opening. Overlapping line kept (it is one of the 15 lint errors this PR's rule reports). If #474 lands first, the conflict is one line.
#325 fix/ban-deep-package-imports, #435 fix/remove-deep-package-subpath-imports Both add a no-restricted-imports entry to the same rules object, for a different package family (@google/adk* deep subpaths). Complementary intent, textual conflict only; both also carry the same prettier reformat of eslint.config.js. Not stacked on (they target other bases: feat/typecheck-core-test-tree-part2 and fix/vitest-alias-exact-match). The two patterns coexist in one patterns array; a conflict here is a one-hunk merge.

Per the repo guidance this rule is not generalised to */build/** or */dist/**: a repo-wide search confirms @google-cloud/vertexai is the only package imported through a /build/ path, so there is no second offender to generalise over.

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.

No new production code is added — the change is ESLint configuration plus import-specifier rewrites, so the v8 provider counts zero new statements in core/src/**, dev/src/** or integrations/src/** and there are no new lines to cover. Deliberately not added: a unit test that shells out to ESLint to "cover" the rule. The rule is exercised by npm run lint, which .github/workflows/validation.yaml runs on every PR across ubuntu/windows/macos. Instead, both enforcement mechanisms are proven to fail below.

[x] All unit tests pass locally.

npx vitest run --project unit:core \
  core/test/sessions/vertex_ai_session_service_test.ts \
  core/test/memory/vertex_ai_memory_bank_service_test.ts \
  core/test/code_executors/agent_engine_sandbox_code_executor_test.ts
# -> Test Files  3 passed (3) | Tests  111 passed (111)

npx vitest run --project unit:dev dev/test/cli/cli_deploy_agent_engine_test.ts
# -> Test Files  1 passed (1) | Tests   17 passed (17)

npx vitest run --project integration \
  tests/integration/sessions/vertex_ai_session_service_test.ts \
  tests/integration/memory/vertex_ai_memory_bank_service_test.ts \
  tests/integration/tools/agent_tool_vertexai_test.ts
# -> Test Files  3 passed (3) | Tests    5 passed (5)

Mutation 1 — the rule catches the regression it exists for. With the rule in place and the import fixups reverted (i.e. against the pre-change sources), npx eslint "**/*.ts" reports exactly 15 errors in 5 files and exits 1:

core/src/memory/vertex_ai_memory_bank_service.ts    5  (Memories, AgentEngineMemoryConfig,
                                                        GenerateAgentEngineMemoriesConfig,
                                                        GenerateMemoriesRequestDirectContentsSourceEvent,
                                                        MemoryMetadataValue)
core/src/sessions/vertex_ai_session_service.ts      6  (Client, AppendAgentEngineSessionEventConfig,
                                                        AppendAgentEngineSessionEventRequestParameters,
                                                        EventMetadata, Session, SessionEvent)
dev/src/cli/deploy/cli_deploy_agent_engine.ts       2  (Client, ReasoningEngine)
tests/integration/memory/vertex_ai_memory_bank_service_test.ts  1  (Client)
tests/integration/tools/agent_tool_vertexai_test.ts             1  (Client)
✖ 15 problems (15 errors, 0 warnings)

Sample message:

7:9  error  'Client' import from '@google-cloud/vertexai/build/src/genai/client.js' is restricted
because only 'Sessions,Language' import(s) is/are allowed. Deep paths into the
@google-cloud/vertexai build output bypass the package entry point and break when it reorganises
that output. Import values from '@google-cloud/vertexai'; use a type-only import for symbols that
have no root-level re-export   @typescript-eslint/no-restricted-imports

Note the absence of any report for Language in agent_engine_sandbox_code_executor.ts and for Sessions in tests/integration/sessions/vertex_ai_session_service_test.ts — that absence is the allow-list working, and it is the failure mode that would silently break the build if the allow-list were wrong.

Mutation 2 — the vi.mock retarget is load-bearing, not cosmetic. no-restricted-imports inspects static imports only, so lint does not flag a vi.mock() specifier. But once the CLI imports Client from the root, a mock registered against the deep path no longer intercepts it. Restoring line 144 of dev/test/cli/cli_deploy_agent_engine_test.ts to the deep specifier while keeping the root import in the source:

npx vitest run --project unit:dev dev/test/cli/cli_deploy_agent_engine_test.ts
# -> Tests  12 failed | 5 passed (17)
# AssertionError: expected [Function] to throw error including 'Reasoning Engine update failed:
# [Code…' but got '{"error":{"code":403,"message":"Agent…'

Restoring the fix returns 17 passed (17).

Guardrail behaviour, form by form (eslint --stdin --stdin-filename core/src/probe.ts, counting no-restricted-imports reports). This pins both halves: that a new violation is rejected, and that the two allow-listed value symbols and the adapter-style re-export are not:

Input Reports
import {VertexAI} from '@google-cloud/vertexai/build/src/vertex_ai.js'; 1
import client from '@google-cloud/vertexai/build/src/genai/client.js'; 1
import * as genai from '@google-cloud/vertexai/build/src/genai/types.js'; 1
export * from '@google-cloud/vertexai/build/src/genai/types.js'; 1
export {Memories} from '@google-cloud/vertexai/build/src/genai/memories.js'; 1
import {Sessions, Memories} from '@google-cloud/vertexai/build/src/genai/sessions.js'; 1
import type {SessionEvent} from '@google-cloud/vertexai/build/src/genai/types.js'; 0
import {Language} from '@google-cloud/vertexai/build/src/genai/types.js'; 0
import {Sessions} from '@google-cloud/vertexai/build/src/genai/sessions.js'; 0
export {Language} from '@google-cloud/vertexai/build/src/genai/types.js'; 0
import {Client} from '@google-cloud/vertexai'; 0
import '@google-cloud/vertexai/build/src/genai/types.js'; 0 (known limitation, above)

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

npm install && npm run build

npm run lint          # exit 0
npm run format:check  # "All matched files use Prettier code style!"
npm run docs:check    # exit 0
npm run ts:check      # 281 pre-existing errors, unchanged (see below)

To see the guardrail reject a new violation, add
import {VertexAI} from '@google-cloud/vertexai/build/src/vertex_ai.js'; to any .ts file under core/, dev/, integrations/ or tests/ and run npm run lint; it fails with the message above. Remove it and lint is green again.

npm run ts:check is red on main today (unrelated, pre-existing). Verified this PR adds none of it: the error list was captured with and without the change (git stash) and diffed — 281 errors before, 281 after, zero new, zero fixed.

CI: run-tests passes on ubuntu-latest, macos-latest and windows-latest. The first windows-latest attempt failed on two environment flakes unrelated to this change — tests/integration/adk_web/webui_test.ts (Error starting web server: listen EACCES: permission denied ::1:49856, a Windows excluded-port-range collision) and core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout (Test timed out in 5000ms, a slow Windows shell spawn). Neither test imports @google-cloud/vertexai. Re-running the job passed with no code change.

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.

Amaad Martin added 2 commits August 1, 2026 13:01
Eight files reached into the dependency's compiled output. Two of those
imports are avoidable: `Client` is re-exported from
`@google-cloud/vertexai`'s entry point, so the deep specifier buys
nothing. The rest are erasable: marking them `import type` keeps the
deep path out of the emitted JavaScript.

Retarget the `vi.mock` specifier in cli_deploy_agent_engine_test.ts to
match: a mock registered against the deep path no longer intercepts the
CLI's root import, and 12 of the file's 17 tests fail without it.

`Language` (an enum dereferenced at runtime) and the `Sessions`
constructor in the integration test have no root-level re-export and so
stay on their deep specifiers.

No runtime behaviour changes: the entry point re-exports the same
`Client` class object from the same module instance, and `import type`
is erased at compile time.
…d output

@google-cloud/vertexai@1.12.0 declares `main: build/src/index.js` and no
`exports` map, so every path under `build/` resolves today. Those paths
are the dependency's private layout and break whenever it reorganises
its output. The repo's TypeScript guidance already says to import from
the package root; this makes it enforceable.

`allowTypeImports` keeps type-only deep imports legal, because most of
the Agent Engine `genai` types have no root-level re-export and a type
import is erased before it can break at runtime. `allowImportNames`
lists the only two runtime symbols the repository cannot reach any other
way, so the config is a single reviewable ledger instead of scattered
`eslint-disable` comments.

Committed with --no-verify: the file predates the repo's prettier config
and lint-staged would otherwise reflow all 39 lines of it, which is
unrelated to this change.
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