Skip to content

Fix: use the node: protocol for the bare built-in imports outside core - #550

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/node-protocol-import-prefix-stragglers
Open

Fix: use the node: protocol for the bare built-in imports outside core#550
AmaadMartin wants to merge 2 commits into
mainfrom
fix/node-protocol-import-prefix-stragglers

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):
    N/A — no public issue; this is a repository-convention cleanup.

Read this first — this PR is very likely redundant. Every one of the four
changed lines is already changed by an open sibling PR on this fork, and the
durable fix (a lint rule) is proposed by three others. See
Duplication below before reviewing further. It is scoped to the four files
outside the sibling-owned trees, so it is the narrowest member of the set, but
a maintainer should merge one of these and close the rest rather than
stacking duplicates.

  1. Or, if no issue exists, describe the change:
    Problem: adk-js prefers Node built-ins to be imported via the node: scheme so that an npm package published under the same bare name cannot shadow the built-in at resolution time. Four call sites outside core/src, core/test and tests/e2e had not been converted:
# Site Before After
1 vitest.config.ts:7 import path from 'path'; import path from 'node:path';
2 dev/src/cli/cli.ts:18 import * as path from 'path'; import * as path from 'node:path';
3 integrations/build.js:70 banner emits from 'module' banner emits from 'node:module'
4 core/build.js:76 banner emits from 'module' banner emits from 'node:module'

Sites 3 and 4 are not ordinary imports — they are emitted text. esbuild prepends the banner verbatim to every file of the esm and web targets, so the bare specifier shipped inside the published artifacts (402 emitted files). Despite how this is sometimes described, the banner is attached under if (format === 'esm'), not to the CJS build; the CJS output never contained it and does not change. The format === 'esm' condition itself and core/build.js's alias map are deliberately untouched.

Scope — this does NOT make the repo free of bare built-in specifiers. An earlier draft of this PR claimed to fix "the last" of them; that was wrong and has been corrected. 54 bare specifiers remain across 25 files, all inside the three trees owned by separate changes: core/src (3), core/test (8), tests/e2e (43). The most significant remainder is shipped library source — core/src/artifacts/file_artifact_service.ts:8-10 still imports fs/promises, path and url bare, which is the same shadowing exposure this PR cites as motivation, in a more important file than the four fixed here. Deliberately not expanded: touching those trees would collide head-on with the PRs listed below.

Solution: Rename the specifier at all four sites. node:path and path resolve to the same built-in, and createRequire from node:module is the same function as from module, so there is no behavioural change. The in-repo precedent is dev/src/utils/agent_loader.ts:14, which already does import {createRequire} from 'node:module';.

Duplication (please read before reviewing). A pre-implementation collision check (gh pr list --repo AmaadMartin/adk-js --state open --limit 1000, 448 open PRs) plus a follow-up audit found that all 4/4 changed lines are already covered by open PRs:

this diff already changed by
vitest.config.ts:7 #547, #422
dev/src/cli/cli.ts:18 #548, #485, #289
core/build.js:76 #430, #547
integrations/build.js:70 #430, #547

#547 makes byte-identical edits to all four files and additionally sweeps core/src, core/test and tests/e2e. #430 covers exactly the two build banners and also ships a banner regression test. Nothing in this branch is unique. Furthermore, the durable fix is a lint rule — eslint.config.js has no n/prefer-node-protocol or equivalent no-restricted-imports guard, so stragglers must be found by hand and nothing prevents the next one landing; #346, #422 and #548 each add exactly that rule and sweep the violations in one pass. Adding the rule here is out of scope for this branch (it cannot pass until the sibling trees are converted), which is precisely why landing a rule PR and closing the per-call-site PRs, including this one, is the better outcome.

Blast radius, stated plainly rather than claimed to be zero: the node: scheme requires Node >= 14.18 / >= 16. This floor is pre-existing for core and dev, whose src trees already contain 15 and 31 node: imports respectively. integrations/src has none, but its ESM artifact already carried a bare 'module' import, which is equally unresolvable in a browser — so this changes the scheme of an already-Node-only import, not the set of environments the artifact works in. A browser-targeted bundler old enough to lack node: support (webpack 4, browserify) cannot resolve node:module, but it could not resolve bare 'module' either, so no working configuration regresses; webpack 5, Vite and Rollup with node-resolve handle node: natively. esbuild's target: ['node10.4'] is unaffected because banner text is prepended verbatim and never parsed or down-levelled. Separately, the ESM banner is also prepended to the web/browser target, where neither module nor node:module resolves — pre-existing and out of scope 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.

One test added: tests/integration/build_setup/build_banner_test.ts (+50). The banner is generated text, so nothing in the source tree stops a future edit to either build script from putting the bare 'module' specifier straight back into published artifacts. The test asserts the emitted banner on the built output — core/dist/esm/index.js, core/dist/web/index_web.js and the two integrations equivalents must import node:module — and asserts the two cjs entry points still carry no banner at all, which pins the "CJS is unaffected" claim as a test rather than a promise. It runs in the existing integration project, which CI executes via npm run test:coverage after npm run build.

No other test was added, and none was modified, skipped, weakened or deleted. The other two sites need no new test: a bad specifier in vitest.config.ts aborts every vitest run at config load, and dev/src/cli/cli.ts is a module-level import already loaded by unit:dev. A test asserting that a source file contains the literal string 'node:path' would test source text rather than behaviour.

Commands run from the repo root on the pushed tree:

npm install
npm run build --workspace core
npm run ts:check
npm run lint                                          # clean
npm run format:check                                  # "All matched files use Prettier code style!"
npx vitest run --project unit:core --project unit:dev
npx vitest run --project integration tests/integration/build_setup/build_banner_test.ts
npm run build --workspace integrations
  • npm run lint and npm run format:check pass clean, including the new test file. No import reordering occurred, so prettier-plugin-organize-imports keeps both changed files in sorted order as-is.
  • New test: 6 passed (4 bannered artifacts + 2 unbannered CJS entry points).
  • npm run ts:check reports 281 errors, all pre-existing. Verified by stashing the new test file and re-running: the error list is byte-identical with and without it, and none are in any file this PR touches. (These are the core/test import-convention errors that Fix: import ADK symbols from source in core/test and pin the convention (281 -> 139 type errors) #514 addresses. The count was 285 before dev was built locally; building dev/dist resolves 4 module-resolution errors and is unrelated to this diff.)
  • npx vitest run --project unit:core --project unit:dev: 2570 passed, 4 failed. The 4 failures are pre-existing and unrelated — 3 in core/test/code_executors/unsafe_local_code_executor_test.ts and 1 in dev/test/cli/cli_create_test.ts. Verified by checking out the unmodified base for the changed files and re-running those two files: the same 4 fail identically.

Prove-it-can-fail. Four mutations, each confirmed to break a real gate; all reverted afterwards.

  1. The new test, against the unfixed code. Reverted both build-script banners to bare 'module', rebuilt, re-ran build_banner_test.ts:
    × imports node:module in core/dist/esm/index.js               → expected 'module' to be 'node:module'
    × imports node:module in core/dist/web/index_web.js           → expected 'module' to be 'node:module'
    × imports node:module in integrations/dist/esm/index.js       → expected 'module' to be 'node:module'
    × imports node:module in integrations/dist/web/index_web.js   → expected 'module' to be 'node:module'
    ✓ emits no banner in core/dist/cjs/index.js
    ✓ emits no banner in integrations/dist/cjs/index.js
    Tests  4 failed | 2 passed (6)
    
    The two CJS assertions correctly keep passing, which is the point — they pin a property the fix must not change.
  2. vitest.config.ts -> from 'node:path_MUTANT'. Every vitest run dies before a single test executes:
    failed to load config from .../vitest.config.ts
    Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:path_MUTANT
    
  3. dev/src/cli/cli.ts -> from 'node:path_MUTANT'. unit:dev fails (Test Files 1 failed (1)):
    Error: Cannot find package 'node:path_MUTANT' imported from .../dev/src/cli/cli.ts
    
  4. core/build.js banner -> from 'node:module_MUTANT', rebuild, then load the ESM entry point:
    ERR_UNKNOWN_BUILTIN_MODULE No such built-in module: node:module_MUTANT
    

Manual End-to-End (E2E) Tests:

Before/after build-output evidence (core and integrations built from the clean base, then rebuilt after the edits):

# BEFORE
$ head -3 integrations/dist/esm/index.js
import {createRequire as topLevelCreateRequire} from 'module';
$ head -3 core/dist/esm/index.js
import {createRequire as topLevelCreateRequire} from 'module';
# 402 emitted files contained bare `from 'module'`; 0 contained `node:module`.

# AFTER
$ head -3 integrations/dist/esm/index.js
import {createRequire as topLevelCreateRequire} from 'node:module';
$ head -3 core/dist/esm/index.js
import {createRequire as topLevelCreateRequire} from 'node:module';
# 402 emitted files now contain `node:module`; 0 contain bare `from 'module'`.

Postconditions, asserted after the rebuild:

# No unprefixed built-in imports remain OUTSIDE core/src, core/test, tests/e2e:
grep -rn -E "from '(module|path|fs|fs/promises|os|url|http|net|crypto|child_process)'" \
  --include='*.ts' --include='*.js' --exclude-dir=node_modules --exclude-dir=dist . \
  | grep -v -E '^\./(core/(src|test)|tests/e2e)/'          # -> no matches
# (54 hits remain INSIDE those three trees; see Scope above.)

grep -rn "from 'node:module'" core/dist/esm core/dist/web \
                              integrations/dist/esm integrations/dist/web   # -> matches (402 files)
grep -rn "from 'module'"      core/dist/esm core/dist/web \
                              integrations/dist/esm integrations/dist/web   # -> no matches
grep -rn "createRequire"      core/dist/cjs integrations/dist/cjs           # -> no matches

The "CJS is unchanged" claim is verified rather than asserted: I checksummed all 203 files under core/dist/cjs + integrations/dist/cjs, rebuilt from the unmodified build scripts, checksummed again, and diffed — byte-identical.

Smoke-loads of the rebuilt artifacts:

node -e "import('./integrations/dist/esm/index.js').then(...)"   # integrations ESM loads OK
node -e "import('./core/dist/esm/index.js').then(...)"           # core ESM loads OK
node -e "require('./integrations/dist/cjs/index.js')"            # integrations CJS loads OK

CLI end-to-end, exercising the consumer of the renamed path import (getAbsolutePath at dev/src/cli/cli.ts:54):

npm run build --workspace dev        # succeeded; fetched adk-web v1.0.0 assets
node dev/dist/esm/cli_entrypoint.js --help          # prints the full command list
node dev/dist/esm/cli_entrypoint.js run ./no_such_agent.ts
# -> AgentFileLoadingError: Agent file /<repo-root>/no_such_agent.ts does not exists

That last line is the point: the relative path was joined onto process.cwd() by path.join resolved through node:path, so the renamed import is exercised in the built binary, not just type-checked.

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. — the only comment is the JSDoc on the new test's banner regex, explaining why the scheme matters.
[x] I have added tests that prove my fix is effective or that my feature works. — build_banner_test.ts, shown failing against the unfixed build scripts above.
[x] New and existing unit tests pass locally with my changes. — 2570 pass plus the 6 new; the 4 failures and 281 ts:check errors are pre-existing and unchanged by this diff.

CI

All test jobs green on 0728884c:

run-tests                    pass   1m33s
run-tests (ubuntu-latest)    pass   5m37s
run-tests (windows-latest)   pass   9m30s
run-tests (macos-latest)     pass   7m11s
check-license                pass   5s

Two flaky first attempts, both in files this diff does not touch, both passing on
re-run with no code change:

Amaad Martin added 2 commits August 2, 2026 17:14
Four call sites outside core/src, core/test and tests/e2e imported a Node
built-in by its bare name, so an npm package published under the same name
could shadow the built-in at resolution time:

  - vitest.config.ts           import path from 'path'
  - dev/src/cli/cli.ts         import * as path from 'path'
  - core/build.js              esbuild ESM banner emitting from 'module'
  - integrations/build.js      esbuild ESM banner emitting from 'module'

The last two are emitted text: the banner is prepended verbatim to every
file of the esm and web targets, so the bare specifier shipped inside the
published artifacts. The cjs target never receives the banner and is
unaffected.

This does not make the repo free of bare built-in specifiers. 54 remain
across 25 files, all inside core/src (3), core/test (8) and tests/e2e (43),
which are owned by separate changes. Notably core/src/artifacts/
file_artifact_service.ts still imports fs/promises, path and url bare.

No behavioural change: node:path and path resolve to the same built-in, and
createRequire from node:module is the same function as from module.
The banner is generated text, so nothing in the source tree stops a future
edit to core/build.js or integrations/build.js from reintroducing the bare
'module' specifier into published artifacts. Assert the emitted banner
directly on the built output, and assert that the cjs target still carries
no banner at all.

Verified to fail against the unfixed build scripts: reverting both banners
to 'module' and rebuilding fails all four bannered assertions with
"expected 'module' to be 'node:module'", while the two cjs assertions
correctly keep passing.
@AmaadMartin
AmaadMartin force-pushed the fix/node-protocol-import-prefix-stragglers branch from 39407c3 to 0728884 Compare August 3, 2026 00:17
@AmaadMartin AmaadMartin changed the title Fix: prefix the last bare Node built-in imports with the node: protocol Fix: use the node: protocol for the bare built-in imports outside core Aug 3, 2026
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