Skip to content

Fix: keep Node built-ins out of the @google/adk browser entry graph - #671

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/browser-entry-node-builtin-leak
Open

Fix: keep Node built-ins out of the @google/adk browser entry graph#671
AmaadMartin wants to merge 4 commits into
mainfrom
fix/browser-entry-node-builtin-leak

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 5, 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):

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

Problem: core/package.json#browser names ./dist/web/index_web.js, but the browser entry graph reaches node:dns/promises, node:net, node:fs/promises and node:path, so a bundler cannot resolve it. core/src/common.ts is the export surface shared by the Node entry and the browser entry, and the filesystem-backed skills toolset, the skills loader and loadWebPage had leaked into it. The bundled build also wrote dist/web/index.js, a name the browser field never points at. esbuild rejects alias without bundle, so the node:async_hooks shim never reached the non-bundled build that prepublishOnly runs.

Solution: I moved those exports from core/src/common.ts to core/src/index.ts, so the Node surface is unchanged and the browser graph loses the built-ins. core/build.js now derives the bundled output name from the entry, and the web target always bundles so the shim alias reaches the published artifact. The esm and cjs outputs are unchanged in both build modes.

Consequences worth knowing:

  • core/dist/web/ goes from 198 emitted .js files to index_web.js plus its sourcemap. core/package.json#browser is the only reference to core/dist/web in the repo.
  • minify and sourcemap derive from bundle, so the published web artifact is now minified with a sourcemap. That was already the shape of the build:bundle web output.
  • Browser consumers lose the skills symbols and loadWebPage. Neither could ever run in a browser, and the browser entry does not load at all today.
  • integrations/build.js:60 has the byte-identical output-name bug, and integrations/package.json:21 names ./dist/web/index_web.js too, so that browser field is dead the same way. I left it for a follow-up: integrations/src/index_web.ts exports only version, so it has no built-in leak and no alias table, and fixing it here would widen a leak-focused PR.

Two deviations from the task spec, both because the tree moved under it:

  • The spec expects node:crypto in the browser graph and a crypto_shim.ts alias. Neither exists now: core/src/utils/env_aware_utils.ts already uses globalThis.crypto, and core/build.js aliases only node:async_hooks. The test pins the set the alias table actually declares.
  • The spec asks me to correct a stale comment in env_aware_utils.ts. That comment no longer says the stale thing, so I left the file alone.

Collision check

I checked the 569 open PRs on the fork. Nothing lands this change. The adjacent ones are complementary and I did not stack on any of them:

#555 also adds tests/integration/build_output/web_output_test.ts, which asserts the emitted dist/web contents. I did not add a second build-output test at that path; the build script edits are verified by the manual steps below.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

New file core/test/index_web_test.ts (4 cases). The first bundles core/src/index_web.ts with esbuild and no alias, reads the external imports out of the metafile, and asserts the ones isBuiltin accepts equal the set core/build.js aliases. The metafile is what makes the guard complete: an unprefixed specifier such as the import * as fs from 'fs/promises' in core/src/artifacts/file_artifact_service.ts:8 is a real built-in that a node: text scan cannot see. The rest pin the symbol lists on both entry points.

npx vitest run --project unit:core core/test/index_web_test.ts
  Test Files  1 passed (1)       Tests  4 passed (4)

npx vitest run --project unit:core core/test/tools/skill core/test/skills core/test/tools/load_web_page_test.ts
  Test Files  11 passed (11)     Tests  191 passed (191)

npx vitest run --project integration tests/integration/tools/run_skill_script_tool_test.ts tests/integration/tools/run_skill_inline_script_tool_test.ts
  Test Files  2 passed (2)       Tests  18 passed | 4 skipped (22)

Proof the tests can fail. Each mutation below was applied to the fixed tree, run, and reverted.

  1. Re-add export {LOAD_WEB_PAGE, loadWebPage} from './tools/load_web_page.js'; to core/src/common.ts:
AssertionError: expected Set{ 'node:async_hooks', …(2) } to deeply equal Set{ 'node:async_hooks' }
  Set {
    "node:async_hooks",
+   "node:dns/promises",
+   "node:net",
  }
  1. Re-add export {SkillToolset} from './tools/skill/skill_toolset.js'; to core/src/common.ts:
  Set {
    "node:async_hooks",
+   "node:fs/promises",
+   "node:path",
  }
  1. Add export {FileArtifactService} from './artifacts/file_artifact_service.js'; to core/src/common.ts, the unprefixed case:
  Set {
+   "fs/promises",
    "node:async_hooks",
+   "path",
+   "url",
  }
  1. Drop loadSkillFromZipBuffer from the ./skills/loader.js block in core/src/index.ts:
AssertionError: loadSkillFromZipBuffer disappeared from the Node entry point:
expected { …(205), …(1) } to have property "loadSkillFromZipBuffer"
  1. Revert bundle: true to bundle on the web target in core/build.js: npm run build emits 198 per-file modules into core/dist/web/, and core/dist/web/utils/client_labels.js keeps its raw node:async_hooks import.

  2. Revert the outfile derivation to ./dist/${targetDir}/index.js: npm run build emits core/dist/web/index.js, which core/package.json#browser does not name.

Coverage: this change adds no executable source line. core/src/common.ts and core/src/index.ts are re-export only, and core/build.js is outside the coverage include globs in vitest.config.ts.

Manual End-to-End (E2E) Tests:

$ rm -rf core/dist && npm run build --workspace core
$ ls core/dist/web/
index_web.js
index_web.js.map
$ grep -c 'node:' core/dist/web/index_web.js
0

$ npm run build:bundle --workspace core
$ ls core/dist/web/
index_web.js
index_web.js.map
$ grep -c 'node:' core/dist/web/index_web.js
0
$ ls core/dist/web/index.js
ls: cannot access 'core/dist/web/index.js': No such file or directory

core/dist/esm/index.js, core/dist/cjs/index.js and core/dist/types/index.d.ts exist in both modes, and the .d.ts still declares all 14 moved symbols.

Repo gates, run from the root on the pushed commit: npm run build, npm run lint, npm run format:check and npm run docs:check all exit 0. npm run ts:check reports the same 286 errors in 46 files before and after this change — the two error sets are byte-identical, so the failure is pre-existing and unrelated.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Amaad Martin added 4 commits August 5, 2026 01:59
core/src/common.ts is the export surface shared by the Node entry and the
browser entry, but the skills toolset, the skills loader and loadWebPage
reached node:fs/promises, node:path, node:dns/promises and node:net through
it. Re-export them from core/src/index.ts so the Node surface is unchanged.
The bundled build hardcoded dist/<target>/index.js, so it wrote
dist/web/index.js while core/package.json#browser names
dist/web/index_web.js. esbuild also rejects alias without bundle, so the
browser shims never reached the non-bundled build that prepublishOnly runs.
…egex

The regex could not see an unprefixed specifier, and core/src/artifacts/
file_artifact_service.ts imports 'fs/promises' that way, so an unprefixed
leak into the common.ts graph would have passed. The metafile lists every
external import structurally and node:module's isBuiltin accepts both
spellings.
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