Skip to content

Fix: prefix the remaining bare Node built-in imports with the node: protocol - #547

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/node-protocol-builtin-imports
Open

Fix: prefix the remaining bare Node built-in imports with the node: protocol#547
AmaadMartin wants to merge 2 commits into
mainfrom
fix/node-protocol-builtin-imports

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 2, 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):
    No existing issue — this is a repository-wide hygiene sweep, described below.
  2. Or, if no issue exists, describe the change:
    Problem: A bare import specifier for a Node built-in (import * as fs from 'fs') resolves to the built-in only because no package of that name happens to be installed. fs, path, os, url, http, net, fs/promises and module are all real, publishable npm package names, so a package anywhere in the resolution path can shadow the built-in. This is a dependency-confusion concern, not a style preference — it is filed under the repository's security guidelines ("Prefix Node Built-Ins with node: … so a same-named npm package cannot shadow them"), and the convention is currently held only by review discipline, which has drifted.

Running Node's own module.builtinModules over the tree found 64 bare built-in references. 58 of them are ours and are fixed here; the other 6 are deliberately left alone (see below).

Two of the offenders are shipped source, not tests:

  • core/src/artifacts/file_artifact_service.tsfs/promises, path, url
  • dev/src/cli/cli.tspath

And two live in published build output. core/build.js:76 and integrations/build.js:70 prepend a createRequire banner to every emitted ESM file:

`import {createRequire as topLevelCreateRequire} from 'module';\nconst require = topLevelCreateRequire(import.meta.url);`;

That bare 'module' ships to every downstream consumer of @google/adk and @google/adk-integrations, so this is the one place where the hazard lands in users' dependency graphs rather than ours. dev/build.js is already fully node:-prefixed and was the in-repo reference for what the other two should look like.

Solution: Prefix all 58 with node:, in two atomic commits:

  1. 56 import specifiers across 27 .ts files — 2 shipped-source files, vitest.config.ts, 4 core/test files, 20 tests/e2e files. Every edit changes only the text inside the quotes; the import type modifiers in core/test/a2a/agent_to_a2a_body_parsing_test.ts and the import * as / named-import forms are all preserved verbatim.
  2. Both esbuild ESM banners'module''node:module' in core/build.js and integrations/build.js. Only the specifier inside the template literal changed.

Diffstat is 29 files, 58 insertions, 58 deletions — no files added, deleted or renamed, and package.json / package-lock.json are untouched (no dependency change).

Deliberately out of scope:

  • The 6 remaining sweep hits are not touched. They are require('fs') inside string literals — fixture payloads representing user-authored code that a code executor or skill runner writes to disk and runs (core/test/code_executors/unsafe_local_code_executor_test.ts:271,301,328, tests/integration/tools/run_skill_inline_script_tool_test.ts:151,229, tests/integration/tools/run_skill_script_tool_test.ts:67). They are not this repository's imports and no AST-based lint rule would flag them. After this change the sweep reports exactly these 6 and nothing else.
  • No ESLint rule is added here. Enforcing this with n/prefer-node-protocol requires a new devDependency and a large package-lock.json delta, so it is left to a follow-up that can land on top of an already-clean tree rather than turning main red on arrival.

One expected formatting side effect: prettier-plugin-organize-imports re-sorts the import block of tests/e2e/tools/rest_api_tool_auth_e2e_test.ts, moving import {OpenAPIV3} from 'openapi-types'; below import * as path from 'node:path';. That is the only line movement in the whole change; the other 26 .ts files keep their existing order because the node: prefix preserves the sort. npm run format:check passes.

One externally visible difference, stated honestly: the emitted banner is applied whenever format === 'esm', which includes the dist/web browser target. For Node consumers 'module''node:module' is a strict no-op. For someone running the web build through a browser bundler, an unresolved-builtin error may now surface where a same-named npm shim was previously being silently substituted — that is the defect the guideline exists to prevent, surfaced rather than hidden, but it is a visible change rather than a pure no-op. core/src/index_web.ts only re-exports ./common.js, so file_artifact_service.ts is not reachable from the browser entrypoint and its rename has no bundler-visible effect.

Duplicate-work check (run before writing any code, against this fork): open PRs #422 (feat/enforce-node-protocol-lint-rule) and #346 (feat/lint-enforce-node-protocol-imports) each contain this same 56-specifier sweep bundled together with the eslint-plugin-n rule and its lockfile churn; open PR #430 (fix/node-protocol-esm-build-banner) contains the two banner edits plus a build-output regression test; #315, #368, #420 and #289 are single-slice subsets. This PR is the consolidated sweep-only change with no dependency bump, so the lint rule can be reviewed on its own merits afterwards. Whichever of these lands first, the others should be closed rather than merged on top — they are the same edits.

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 test file is added, and that is deliberate: every edit is a module-specifier string, so there is no new executable line for a unit test to assert against — a new test could only re-assert that Node resolves node:path. The existing suites already exercise every renamed import at load time, and I verified that they fail when a specifier is wrong (mutation results below). The two regression guards this change actually wants are a lint rule and a build-output assertion, both of which need a new devDependency / a new file and are being handled separately.

[x] All unit tests pass locally.

Targeted runs (node -v v22.22.2), all against the exact commits pushed here:

Command Result
npx vitest run --project unit:core core/test/artifacts/file_artifact_service_test.ts core/test/a2a/agent_to_a2a_body_parsing_test.ts core/test/agents/llm_agent_auth_integration_test.ts core/test/tools/openapi_tool/openapi_toolset_integration_test.ts 4 files, 49 passed
npx vitest run --project unit:dev dev/test/cli/cli_test.ts 24 passed (covers the renamed dev/src/cli/cli.ts)
npm run build succeeds for all three workspaces
npm run lint clean
npm run format:check "All matched files use Prettier code style!"
npx tsc --noEmit byte-identical output before and after the change (diff of the two runs is empty; 747 pre-existing errors in 270 files on main, unchanged — this change neither adds nor fixes any)
npx vitest run --project e2e 16 failed / 6 passed / 4 skipped files, 22 failed / 11 passed / 12 skipped tests — identical set to the pre-change baseline captured by stashing the diff and rebuilding. The failures are all API key must be provided…, i.e. credential-gated. The value of this run is that all 26 files still collect, which proves the 43 renamed e2e specifiers resolve at import time.
npx vitest run --project unit:integrations 1 pre-existing failure, identical before and after: integrations/test/version_test.ts asserts '1.3.0' while the workspace is at 1.5.0. Unrelated to this change and untouched by it.

Any vitest invocation at all also proves the vitest.config.ts rename, since a broken config fails before a single test is collected.

Proof the tests can fail (mutation testing). Each mutation was applied on its own, run, then reverted, and the source diffed against a pristine copy:

  1. core/src/artifacts/file_artifact_service.ts: 'node:fs/promises''node:fs/promisez'core/test/artifacts/file_artifact_service_test.ts fails: Error: Cannot find package 'node:fs/promisez' imported from …/file_artifact_service.ts (Test Files 1 failed).
  2. dev/src/cli/cli.ts: 'node:path''node:pathz'dev/test/cli/cli_test.ts fails: Error: Cannot find package 'node:pathz' imported from …/cli.ts (Test Files 1 failed).
  3. core/build.js banner reverted to 'module'npm run build --workspace coregrep -rl "from 'module'" core/dist lists files again (core/dist/esm/skills/gcp_skill_registry.js, …). With the fix in place the same grep over core/dist and integrations/dist returns nothing.

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

From a clean checkout, npm ci, then:

  1. The sweep is clean. Run Node's own built-in list over the tree:

    node -e '
    const {execSync} = require("node:child_process");
    const builtins = new Set(require("node:module").builtinModules);
    const fs = require("node:fs");
    const files = execSync(`git ls-files "*.ts" "*.js" "*.mjs" "*.cjs"`, {encoding:"utf8"}).trim().split("\n");
    const re = /(?:from|import|require\()\s*[\x27"]([^\x27"]+)[\x27"]/g;
    let n = 0;
    for (const f of files) {
      if (f.includes("node_modules") || f.includes("/dist/")) continue;
      fs.readFileSync(f, "utf8").split("\n").forEach((line, i) => {
        let m; re.lastIndex = 0;
        while ((m = re.exec(line))) {
          if (builtins.has(m[1]) && !m[1].startsWith("node:")) { console.log(`${f}:${i+1}: ${line.trim()}`); n++; }
        }
      });
    }
    console.log("TOTAL:", n);'

    Before: TOTAL: 64. After: TOTAL: 6, and all 6 are the fixture payloads listed above.

  2. The shipped banner is fixed.

    npm run build
    head -2 core/dist/esm/index.js                            # from 'node:module'
    grep -rl "from 'module'" core/dist integrations/dist      # prints nothing
  3. The built ESM output still loads.

    node -e "import('./core/dist/esm/index.js').then(m => console.log(typeof m.FileArtifactService))"   # function
  4. The diff is the right shape. git diff --stat → 29 files, 58 insertions, 58 deletions; no package.json, no package-lock.json, no new files.

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 note. The first run-tests (windows-latest) attempt failed in tests/integration/a2a/basic/a2a_agent_test.ts with Error: CLI exited prematurely with code 1 (from tests/integration/test_case_utils.ts:341), while ubuntu-latest and macos-latest passed and 2679 tests passed on Windows itself. That is the known Windows harness flake: BaseTestServer picks its port as 40000 + Math.floor(Math.random() * 10000) (tests/integration/test_case_utils.ts:276) and hands the guess to the spawned CLI without checking it is bindable; a failed bind makes the child process.exit(1). Neither that harness nor the failing test file is in this diff — a2a_agent_test.ts already imported node:path before this change — and a module-specifier rename cannot be platform-conditional. Re-running the job passed, so all three OS legs of run-tests are green on this commit. Open PR #546 is the actual fix for that harness.

Amaad Martin added 2 commits August 2, 2026 15:25
A bare specifier like 'fs' or 'path' only resolves to the Node built-in
because no package of that name happens to be installed; all of them are
real, publishable npm package names, so a package in a consumer's
dependency tree can shadow the built-in. Prefix the 56 remaining bare
built-in specifiers across core/src, dev/src, core/test, tests/e2e and
vitest.config.ts with node:.

Specifier-only edits; no behaviour, type or API surface changes.
prettier-plugin-organize-imports re-sorts the import block of
tests/e2e/tools/rest_api_tool_auth_e2e_test.ts as a result, which is the
only line movement in the change.
core/build.js and integrations/build.js prepend a createRequire banner to
every emitted ESM file. It imported the built-in with the bare specifier
'module', so the shadowing hazard shipped inside published bundles and
landed in consumers' dependency graphs rather than ours.
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