Fix: emit the node: protocol in the ESM build banner and format-check .js/.mjs (stacked on #426) - #552
Open
AmaadMartin wants to merge 7 commits into
Conversation
added 7 commits
July 31, 2026 19:14
…banner core/build.js and integrations/build.js inject a createRequire shim into every ESM and web artifact via esbuild's banner option, importing Node's module builtin with a bare specifier. A bare specifier can be shadowed by a userland npm package literally named "module", in which case topLevelCreateRequire is not the builtin and the emitted shim throws at module-evaluation time, before any ADK code runs. The node: prefix is unshadowable by construction and matches the form already used everywhere else in the tree, including dev/src/utils/agent_loader.ts. The banner is attached under `if (format === 'esm')`, so this affects dist/esm and dist/web only; dist/cjs and dist/types are bit-identical.
The banner specifier lives inside a template literal in core/build.js and integrations/build.js, so it is plain text to ESLint rather than an ImportDeclaration; no lint rule can reach it, and the repo's lint entry point (eslint "**/*.ts") does not cover build.js at all. This test is the guard static analysis structurally cannot be. Placed under tests/integration/build_setup/ because the root test and test:coverage scripts run the integration project but not unit:integrations, and it sits next to the only other test about build output. It is pure file I/O, so it adds negligible time to that project.
… fix/node-protocol-build-banner-and-js-lint-scope Incorporates the banner specifier fix verbatim from its own open PR rather than re-implementing it, so this branch carries the full task while #430 keeps sole authorship of the core/build.js and integrations/build.js hunks.
The root `format` and `format:check` scripts still globbed `**/*.ts`, so the
`.js` and `.mjs` files that are now linted were never format-checked. Widen
both to `**/*.{ts,js,mjs}` and commit the resulting reformat of the two files
that had drifted (`.prettierrc.js`, `scripts/auto-assignment.mjs`); both
changes are cosmetic.
…ript
The existing build_banner_test.ts asserts on the text of core/build.js and
integrations/build.js, so it cannot tell whether the banner is actually
emitted: with the banner block left intact but its guard disabled, that test
still passes 2/2 while dist/esm and dist/web ship no banner at all.
Assert on dist/{esm,web}/index.js instead, and assert the banner stays out of
the cjs target.
…akes dead Widening lint scope to .js turned every @typescript-eslint/no-require-imports suppression in a JavaScript file from an inert comment into a load-bearing one. The override that exempts CommonJS sources matched '**/*.cjs' only, so the fixtures that are CommonJS by their package.json "type" rather than by extension were left suppressing the rule at each call site. Match those two fixture trees in the override and delete the suppressions: three inline in js_commonjs, two more in its helpers, one in devtools_check, and a file-scope disable in app_js/app.js that was hiding five require() sites on its own. Narrowing the override back to '**/*.cjs' fails lint with 11 errors, so the config change is what makes the deletions safe.
Two gaps between the two halves of the widened quality scope:
- eslint.config.js ignores 'api-reference/**' (typedoc output, not gitignored,
contains typedoc's own bundled minified assets/main.js) but .prettierignore
did not, so after npm run docs:generate a local format:check scanned that
bundle and format rewrote it. CI is unaffected -- docs:check runs
typedoc --emit none, after format:check -- so this was local-only.
- .cjs is linted, and has a dedicated override, but the format globs stopped at
{ts,js,mjs}, leaving the three tracked .cjs files linted and never
format-checked. Widening reformats one file, .github/scripts/csat.cjs.
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A — no public issue tracks this.
Problem: Two related defects in the repo's own tooling.
built-in import.
core/build.jsandintegrations/build.jsinjectimport {createRequire as topLevelCreateRequire} from 'module';as anesbuild
banneron everyformat: 'esm'target — that isdist/esmanddist/webfor both@google/adkand@google/adk-integrations, 198 filesper target. The repo's convention everywhere else is
node:-prefixed(
core/build.jsitself writesimport {writeFile} from 'node:fs/promises';seven lines up), so a userland package named
modulecannot shadow thebuilt-in. Not a crash — Node resolves both spellings to the same built-in —
but bundlers and non-Node resolvers cannot identify the bare form as a
built-in.
core/build.jshad never been linted: the rootlint/formatscriptsglobbed
**/*.tsonly, andeslint.config.jsgatedjs/recommendedand allglobals behind
files: ["**/*.ts"], so a.jsfile resolved to 23 rules,zero globals, and no
no-undef.Solution: This PR is stacked, and most of it is not newly authored here.
See the collision note below — the honest summary is that two open PRs already
covered a half each, so this branch incorporates them rather than
re-implementing them, and adds the two things neither one does.
Incorporated by merge, not re-typed — the banner specifier fix
(
'module'→'node:module'in both build scripts) is#430's two commits, merged
in verbatim so that PR keeps sole authorship of those hunks.
Base of the stack — the ESLint scope widening
(
files: ['**/*.{js,cjs,mjs,ts}'],lint/lint:fix→eslint ., anddeletion of the dead duplicate
generateContentAsyncin the twojs_*fixtures) is #426, which
this PR is based on.
Authored here (the delta reviewers should read):
package.json—formatandformat:checkwere still**/*.ts, so the.js/.mjsfiles Chore: lint JavaScript files, not just TypeScript #426 brought under ESLint were still neverformat-checked. Widened both to
**/*.{ts,js,cjs,mjs}, matching the lintscope exactly, and committed the resulting reformat of the three files that
had drifted:
.prettierrc.js(double → single quotes),scripts/auto-assignment.mjs({ github, context }→{github, context},one 82-column
ifwrapped) and.github/scripts/csat.cjs. All cosmetic;eslint.config.jsneeded nothing because Chore: lint JavaScript files, not just TypeScript #426 already formatted it..prettierignore—+api-reference/.eslint.config.jsignoresapi-reference/**(typedoc output; not gitignored, and it containstypedoc's own bundled minified
assets/main.js) but.prettierignoredidnot, so after
npm run docs:generatea localformat:checkscanned thatbundle and
formatrewrote it. CI was unaffected —docs:checkistypedoc --emit noneand runs afterformat:check— so this was alocal-dev-only trap. Verified by planting a file at
api-reference/typescript/assets/main.js: before the fix Prettier listedit as different while ESLint correctly ignored it.
eslint.config.js+ 5 fixtures — removed 7eslint-disable @typescript-eslint/no-require-importssuppressions that the scopewidening had just turned from inert comments into load-bearing ones. Chore: lint JavaScript files, not just TypeScript #426's
override exempts CommonJS sources but matched
'**/*.cjs'only, so thefixtures that are CommonJS by their package.json
"type"rather than byextension were left papering over the rule at each call site. Fixed at the
root — the override now also matches
tests/integration/build_setup/js_commonjs/**/*.jsandtests/integration/app_loader/app_js/**/*.js(verified accurate:js_commonjs/package.jsondeclares"type": "commonjs",app_js/package.jsondeclares no
"type", sosourceType: 'commonjs'is correct for both) —and all 7 suppressions are deleted, including a file-scope
/* eslint-disable */inapp_js/app.jsthat was hiding fiverequire()sites by itself.
tests/integration/build_setup/esm_build_banner_test.ts— a regression testthat asserts on the shipped artifacts instead of on the build script's
source text. See "why a second banner test" below; this is not redundant
with Fix: use the node: protocol for the module builtin in the generated ESM build banner #430's test, and the mutation results prove it.
Net suppression count for this branch: +0 added, −7 removed.
Collision check (performed before writing any code).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000returned 451open PRs; filtering titles/branches for
node:,banner,lint,format,prettier,eslint,build.js,mjs,modulesurfaced two direct hits, bothOPEN and MERGEABLE:
fix/node-protocol-esm-build-bannerfix/lint-scope-javascript-files.cjsblock)Rather than open a third competing implementation, this PR stacks on #426 and
merges #430. Merge order matters for whoever lands these: if #430 merges
first the merge commit here becomes empty; if this PR merges first, #430 becomes
empty. Either is fine, but they should not be reviewed as independent
implementations of the same hunk.
Why a second banner test rather than reusing #430's. #430 adds
build_banner_test.ts, which readscore/build.js/integrations/build.jsandasserts the source text contains the
node:modulestring. That pins theimplementation, not the behaviour: it cannot observe whether the banner is
actually emitted. Demonstrated by mutation 2 below — with the banner block left
completely intact but its guard disabled, #430's test stays green at 2/2 while
dist/esmanddist/webship no banner at all. The new test readsdist/{esm,web}/index.jsand fails. #430's test is kept, not replaced; the twoare complementary and cost 8 ms together.
Deliberate scope decisions (stated explicitly rather than left as silent
omissions):
.cjsdeviates from the approved spec, deliberately. The spec scoped.cjsout of the widened globs because bringing it under ESLint would need aseparate
files: ['**/*.cjs']block withsourceType: 'commonjs'— but Chore: lint JavaScript files, not just TypeScript #426,the base of this stack, already added exactly that block, so the spec's stated
reason for the exclusion no longer exists here. Leaving it out would ship
files that are linted but never format-checked. The globs therefore include
cjs; measured cost is one extra reformatted file,.github/scripts/csat.cjs.lint-stagedstill globs**/\*.{js,ts}, so the pre-commit hook does notformat
.cjs/.mjseven though CI now checks them. One-line fix, outside thespec's enumerated file list; queued as a follow-up (
raw-7402ee44).ts_commonjsfixtures keep theirno-require-importssuppressions.Those 5 live in
.tsfiles, which were already linted before this change, sothey are pre-existing and not made load-bearing by it. Removing them is a
different judgement call (a
.tsfile usingrequire()is not CommonJS byconstruction the way a
.cjsfile is) and would be unrelated churn here..tssource file is modified, matching the spec — the remaining barebuilt-in imports in
core/src/artifacts/file_artifact_service.tsanddev/src/cli/cli.tsbelong to other tasks and are untouched here.Not a breaking change. No public API, export, type, or
exportsfieldchanges.
dist/cjsis unaffected (theformat === 'esm'guard is untouched).The widened globs are developer-facing only and touch no published artifact.
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.
New:
tests/integration/build_setup/esm_build_banner_test.ts, 6 cases —coreandintegrations× (dist/esmfirst line,dist/webfirst line,dist/cjscontains notopLevelCreateRequire). It asserts on build output, soit inherits the
integrationproject's existing precondition thatnpm run buildhas run; CI runsnpm run buildbefore the test step.Existing fixtures affected by #426's dead-method deletion, re-run in full:
Proof the new test can fail. Two mutations, both run against the built tree.
Mutation 1 — revert the fix (
sed 's/node:module/module/'on both buildscripts, rebuild). The four positive assertions fail, the two
cjsnegativeassertions correctly stay green:
Mutation 2 — banner still in the source but never emitted
(
if (format === 'esm')→if (false && format === 'esm')incore/build.jsonly, rebuild core). This is the case that justifies adding the test at all:
| Test | Result under mutation 2 |
| ---------------------------------------------- | ----------------------------------------- | --------------------------------------------------------------------------- |
|
build_banner_test.ts(#430, source-text) |Tests 2 passed (2)— false negative ||
esm_build_banner_test.ts(this PR, artifact) |Tests 2 failed | 4 passed (6)— the twocorecases fail,integrationscorrectly stays green |Both mutations were reverted and the tree rebuilt before committing; the
git diffforcore/build.js/integrations/build.jsagainst the merged #430commits is empty.
Proof the suppression removal is safe because of the config change, not in
spite of it. Same discipline applied to the ESLint config edit: narrow the
override back to
files: ['**/*.cjs']with the 7 suppressions already deleted,and lint fails —
11, not 7, because the single file-scope
/* eslint-disable */inapp_js/app.jswas covering fiverequire()sites on its own. With thewidened override restored,
npm run lintexits 0.The fixtures whose suppressions were deleted are executable, so they were run,
not just linted:
app_loader_test.tsis intermittently failing on this machine, before andafter this change — measured 3/6 runs failing with the
app_jsfixturereverted to its unmodified state, and a comparable rate with it modified. The
edit to that fixture is the deletion of one comment line, which is inert at
runtime. This is the known pre-existing app_loader flake already covered by
#235, #247, #256, #260, #499, #506, #521 and #545; it is not re-queued here.
Repo-wide gates on the exact pushed commit:
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
All verified locally on Node v22.22.2 / npm 9.2.0 (the
enginesand CI matrixare unchanged by this PR).
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.