Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Builtin module metadata src/jsc/modules/NodeModuleModule.cpp |
The builtin module names include node:test. A comment documents that node:test and node:sqlite are listed with the node: prefix. |
Builtin module validation test/internal/source-lints/builtin-module-tables.test.ts, test/js/node/module/node-module-module.test.js, test/aot/runtime-use-of-built-in-code.test.ts, test/js/bun/module-graph/module-graph-isolation.test.ts |
Source checks validate builtin-table contents and consistency. Runtime tests check listing, recognition, and prefixed resolution, replace a fixed array-length assertion, and classify node:test as host-owned and as a TODO case in the AOT test. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
45763
-
No actionable merge-blocking risk is established; the change is mergeable.
Pre-merge checks: 
-
Check name Status Explanation Title check The title "node:module: list node:test in builtinModules" names the main change: adding "node:test" to the builtinModuleNames array so that module.builtinModules matches isBuiltin() and Node. The chan… Description check The description follows the repository template. It has a Problem section, a Fix section that describes the verification steps, and a Background section, so it covers the template's intent of explaini… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "node:module: list node:test in builtinModules" names the main change: adding "node:test" to the builtinModuleNames array so that module.builtinModules matches isBuiltin() and Node. The changeset edits NodeModuleModule.cpp to add the entry, and the other changes (lint, tests, allowlist) support that fix. The title is specific and short, and it does not use vague terms.
Full details: Description check
-
Explanation
The description follows the repository template. It has a Problem section, a Fix section that describes the verification steps, and a Background section, so it covers the template's intent of explaining the change and how it was verified. The 'How did you verify your code works?' content is detailed: it names the lint and test files, the repro command, and the CI results for the AOT tests. The template headings 'What does this PR do?' and 'How did you verify your code works?' are not used verbatim; the Problem/Fix sections cover the first heading's content. One item is noted as deferred to CI for platform-specific tests, but the description still provides complete reasoning and verification details.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
StatusReproduced with a canary build ( With this branch the debug build prints The branch is rebased on CIThe diff is green. Build 134278 on
Everything else that gates a merge passed:
The 70 other red jobs are The branch has one Fix: this PR (#39700). #33430 was an earlier attempt with a single table. It is closed. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one string literal added to a static table, following the existing node:sqlite pattern.
What was reviewed:
- Counted the table: 77 entries after the addition, matching the updated
toHaveLength(77). isBuiltinModule.cppalready listsnode:test, so the newisBuiltin/process.getBuiltinModuleassertions hold.- Only other consumer of
node:-prefixed entries is the REPL'sgetBuiltinLibs(), which already filters them out (src/js/internal/repl/node-shims.js:184). - New tests assert the exact prefixed set and that bare
"test"stays out; they fail on main per the PR notes.
Extended reasoning...
Overview
Adds "node:test"_s to the static builtinModuleNames array in src/jsc/modules/NodeModuleModule.cpp, alongside the existing "node:sqlite"_s entry, plus a three-line comment explaining the prefix-only convention. Bumps the length assertion in node-module-module.test.js from 76 to 77 and adds two focused tests: one enumerating the node:-prefixed subset and asserting each is prefix-only and requireable, and one in node-test.test.ts asserting node:test is a builtin.
Security risks
None. The change is a compile-time string literal in a constexpr array; no user input, no allocation, no new code paths, no exception scopes touched.
Level of scrutiny
Low. This is a one-line data change to a lookup table that already contains an entry of the exact same shape (node:sqlite, added in #32498). The array is consumed only by getBuiltinModulesObject via countof(), so no hardcoded size to update elsewhere. I verified the entry count (77) by hand, confirmed isBuiltinModule.cpp:48 already recognizes node:test (so the new test's isBuiltin and process.getBuiltinModule assertions are consistent), and confirmed the REPL consumer at src/js/internal/repl/node-shims.js:184 filters node:-prefixed names so completion is unaffected.
Other factors
The PR description is unusually thorough — it enumerates the set difference against Node 26.3.0, justifies leaving out node:quic (not in stock Node builds), bun:wrap/bun:main (deliberately removed in #31831), and defers freezing the array as an independent change. Tests are placed in the correct existing files, use exact-value assertions (toEqual(["node:sqlite", "node:test"]) rather than toContain), and cover the negative contract (bare "test" absent, isBuiltin("test") false). No outstanding reviewer comments; no prior reviews from me.
|
Thanks, the review asks for no changes. @coderabbitai review (The first attempt hit the review rate limit before it started.) |
|
✅ Action performedReview finished.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Update after a self-review of the first version. The head is now f06a92c.
The PR description describes the current state. |
|
@coderabbitai review The three commits after 8baf99b (the source lint, the test rework and the comment change) have not been reviewed yet. The earlier attempts hit the review rate limit. |
|
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
LGTM — one-line data addition to a static table, matching Node's behavior for prefix-only builtins.
What was reviewed:
- Verified
node:testis already inisBuiltinModule.cppandnode_entry_only_prefix!(), so this only closes the gap inbuiltinModules. - Checked the new source-lint's
notListedentries (bun:main,bun:wrap,node:quic) against both tables — all consistent; the regex parsers match the actual table declarations. - Confirmed the runtime test's new loop over
node:-prefixed entries only hitsnode:sqliteandnode:test, both requireable in-process; the removed.toHaveLength(76)is subsumed by the cross-table lint.
Extended reasoning...
Overview
The PR adds "node:test"_s to the builtinModuleNames[] table in src/jsc/modules/NodeModuleModule.cpp so that require("node:module").builtinModules includes it, matching Node 26. It also corrects the first sentence of the pre-existing comment above that table, adds a source-lint test that cross-checks the three hand-written builtin-name tables (builtinModuleNames, builtinModuleNamesSortedLength, node_entry_only_prefix!), wires the new file into the source-lints workflow path filter, and replaces the fragile .toHaveLength(76) runtime assertion with invariant-based checks.
Security risks
None. This is a metadata list of module names; adding an entry does not change what require() resolves — node:test was already resolvable and already accepted by isBuiltin(). The source-lint test only reads source files with regex.
Level of scrutiny
Low. The production change is a single string literal appended to a static constexpr ASCIILiteral[], following the exact precedent set by node:sqlite immediately above it. No control flow, no allocation, no exception paths are touched. The test and workflow changes are defensive tooling.
Other factors
- I verified the source-lint test's assumptions hold:
isBuiltinModule.cppcontainsbun:main,bun:wrap,node:quic,node:test,node:sqlite;HardcodedModule.rshasnode_entry_only_prefix!fornode:sqlite,node:test,node:quic. ThenotListedmap exactly covers the intended isBuiltin-only names, and the reverse check ensures the map cannot go stale. - The runtime test dropped the hardcoded length in favor of stronger invariants (every listed name passes
isBuiltin, prefix-only entries are not resolvable bare,process.getBuiltinModulematchesrequire). The lint test enforces completeness in the other direction, so no coverage is lost. - The comment-cop bot flagged the table comment three times; the author iterated down to a minimal one-sentence correction of the pre-existing comment, and all threads are resolved.
- The PR description references assertions in
node-test.test.tsthat are not in the final diff — presumably reverted in a later commit — but this does not affect the change's correctness or test coverage. - The PR author enumerated downstream consumers of
builtinModules(REPL,util.inspect,test-require-builtins,test-process-get-builtin) and explained why each is unaffected or improved.
|
Thanks. On the one observation: the PR description was rewritten for the current diff before this review ran and no longer mentions |
|
On the "wrong build configuration" note in the merge risk summary: the new test is a source lint. It reads three source files and never runs the bun binary, so the source-lints workflow runs it with a released bun on purpose. That is the documented setup for every test in |
Node lists the modules that only resolve with the node: prefix under
that prefixed name in module.builtinModules. Bun already does this for
node:sqlite. node:test was missing from the list although
isBuiltin("node:test") is true and require("node:test") works.
module.builtinModules and module.isBuiltin() read two hand-written tables, and the module loader has a third marker for the builtins that resolve with the node: prefix only. A new module tends to reach the last two and not the first. The new source lint requires every name isBuiltin() accepts to be listed in builtinModules, with an allowlist that names the reasons for the deliberate exceptions. The runtime test now checks the array's properties instead of its length, and the copy of those assertions in node-test.test.ts is gone.
Three tests that main gained since this branch started read the list: - node-module-module.test.js: the Object.entries(Module) test pinned the length of the copied list at 76. It now checks that the copy holds "fs". The first test of the file and the source lint check what the list holds. - module-graph-isolation.test.ts: every listed builtin has to say what a graph's dispose() does with what the module opens. node:test is "host": a test it registers goes to the process's test runner, like Bun.jest. - aot/runtime-use-of-built-in-code.test.ts: the trim report of a program that requires node:test lists globalThis as escaped, so node:test joins the modules of which that is a known gap. The source-lints workflow triggers on src/**/*.cpp since #39833, so the extra path for NodeModuleModule.cpp is gone.
f06a92c to
ddf03aa
Compare
The lint counted a name in a commented-out line as listed, and it folded each table into a set, so a name that was there twice passed. It now drops comment lines, reports a line that is not one name, and fails when a table holds a name twice. The runtime test also fails on a repeated name. The Object.entries test compares the length of the copied list with the length of the list of the test process again, in place of a check for one name.
Problem
require("node:module").builtinModuleslacks"node:test", althoughisBuiltin("node:test")is true and the module loads. Node lists it. A tool that takes itsexternallist from the array missesnode:test.isBuiltin()read separate hand-written tables (src/jsc/modules/NodeModuleModule.cpp:55,src/jsc/bindings/isBuiltinModule.cpp:3). Nothing compared them. require.resolve withpathsoption #18851 dropped the name from the first.Fix
"node:test"tobuiltinModuleNames. As in Node, a builtin that resolves only asnode:xis listed with the prefix, likenode:sqlite(node:sqlite: implement the module and pass the Node v26.3.0 test suite #32498).test/internal/source-lints/builtin-module-tables.test.ts: every nameisBuiltin()accepts is inbuiltinModuleNames, or an allowlist says why not.test/js/node/module/node-module-module.test.jsfail without the entry and pass here. Other suites: Notes.Background
module.builtinModulesis the public array of builtin names. Bun's array also holds Bun's own modules and replaced packages such asws.module.isBuiltin(name)also accepts internal names such asbun:wrap, whichBun.pluginmust not override. A source lint reads the source tree and needs no build.bun:wrapandbun:mainagain, which process: port Node.js v26.3.0 process compatibility tests and fix the gaps they surface (env exotic-object/TZ semantics, warnings pipeline + CLI flags, uncaught origin/exit codes, execve throw, threadCpuUsage/finalization/loadEnvFile, native-module identity; +26 tests) #31831 removed. The lint fails when the two tables drift.Downsides
builtinModuleshas 77 names, was 76. Code that pins the length or a snapshot of the list breaks.builtinModules.node:sea,node:test/reportersandnode:ffi, which Bun does not have.Notes
bun -e 'const m=require("node:module"); console.log(m.isBuiltin("node:test"), m.builtinModules.includes("node:test"), typeof require("node:test"))'printstrue false functionon a build without the entry (canary367d939d9). The table on main11c367163ahas no"node:test"entry. Node 26.3.0 and 26.12.1 printtrue true function, and so does a debug build of this branch.node:test#18140 (v1.2.6) listednode:test. require.resolve withpathsoption #18851 (v1.2.9) dropped it again and changed the length assertion innode-module-module.test.jsfrom 77 to 76. That is why no test pins the length now.test/js/node/module/node-module-module.test.js, "Object.entries(Module) passes exception check validation" (Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021). It expectedbuiltinModules: 76for the copied list. It now expects the length of the test process's own list. The first test of the file and the lint check what the list holds.test/js/bun/module-graph/module-graph-isolation.test.ts, "every node: builtin module is classified" (Bun.ModuleGraph: a context per graph for timers and I/O, per-graph CommonJS #42590). Every listed name needs an entry that says whatgraph.dispose()does with what the module opens. Without one the test fails with["node:test"]. The new entry is"node:test": "host", the classBun.jesthas in the same file:node:testregisters its tests throughBun.jest(src/js/node/test.ts:9).test/aot/runtime-use-of-built-in-code.test.ts, "only a call opens the report: requiring %s leaves it closed" (Compile TypeScript to machine code ahead of time:bun build --compile --aot#45631). It has a row for every listed name. Fornode:testthe report ofbun check --aot --explain=trimhas the lineuses globalThis (escapes, so any member may be used), so the row fails.node:testis now inletTheGlobalObjectEscape, the modules whose rows aretest.todofor that reason.node:testin it, on a debug build of this branch:test/js/bun/test/parallel/test-require-builtins.ts(it requires all 77 names, each in a subprocess),test/js/node/test/parallel/test-process-get-builtin.mjs,test-require-resolve.js,test-module-builtin.js,test-repl-options.js,test/bundler/bun-build-compile.test.ts("every internal module's functions have names"),test/internal/lazy-function-table.test.ts,test/js/web/workers/worker.test.ts("terminate() while importing every builtin module"),test/cli/lint/js-plugins.test.ts("a config file and a rule see Node.js's list of builtin modules"),test/js/bun/resolve/builtin-esm-lazy-exports.test.ts, the three builtin tests oftest/js/bun/repl/repl.test.ts,test/js/node/sqlite/node-sqlite.test.ts("node:sqlite is a built-in module"), and all oftest/internal/source-lints/.bun check --aotof onerequire("node:test")took 263 s. Their results are from CI onddf03aabc3, on the release builds for Debian 13 x64 and aarch64. The head has the samesrc/and differs from it in two test files.test/aot/load/every-runtime-module-compiles.test.ts: 3 pass.test/aot/runtime-use-of-built-in-code.test.ts: 85 pass, 53 todo.test/aot/generated-library-modules-that-use-keys-of-one-another.test.ts: 28 of 28 (x64) and 34 of 34 (aarch64) of the drawn programs pass. Its "beside every module" programs now importnode:test.node-module-module.test.jstakes 23 to 35 s. Subprocess tests that this change does not touch hit the 5 s default timeout (7 in one run). With--timeout 30000all 80 tests pass.ASCIILiteralholds onestd::span). The string with its terminator is 10 bytes.src/**/*.cppsince Update the jsc class skill for BUN_SUBSPACE_SLOTS and lint IsoSubspace construction #39833, which coversNodeModuleModule.cpp.--testCLI mode, node:test/reporters, standalone execution, and reporter-output parity (+18 tests, test_runner 32%→55%) #34515 (addnode:test/reporterstoisBuiltinModule.cppandHardcodedModule.rsonly): it fails with["node:test/reporters"]. Node listsnode:test/reporters, so that is the wanted outcome. The lint also fails when an allowlist entry goes stale. Its allowlist:bun:mainandbun:wrapare internal (process: port Node.js v26.3.0 process compatibility tests and fix the gaps they surface (env exotic-object/TZ semantics, warnings pipeline + CLI flags, uncaught origin/exit codes, execve throw, threadCpuUsage/finalization/loadEnvFile, native-module identity; +26 tests) #31831), and stock Node 26 builds do not listnode:quic."ws"commented out inbuiltinModuleNamesgives["ws"], and a second"fs"givestwice: ["fs"].builtinModuleNamesinsyncBuiltinESMExports(). It looks each name up in the module registry and skips a module that is not loaded. With this change it also looks upnode:test.node:sea,node:testandnode:test/reporters, Bun lists none of them. Node 26.12.1 addsnode:ffi. After this change the only entries Bun lacks are modules Bun does not have:isBuiltin()is false for each.node:quic: Bun resolves it without a flag and prints an ExperimentalWarning. The stock node 26.3.0 and 26.12.1 binaries do not list it, also not with--experimental-quic. To list it is a separate decision. The allowlist entry records the current one.stream/iterand third-party aliases), the separate table behindprocess.binding("natives")insrc/jsc/bindings/ProcessBindingNatives.cpp, and the REPL's list of prefix-only names (getSchemeOnlyModuleNames()insrc/js/internal/repl/node-shims.ts, which hastestand notsqlite). A table derived from the resolver'sAliasentries would remove the copies. That is a larger change than this fix.node:testentry by merging the two tables into one. That would listbun:wrapandbun:mainagain, and the branch conflicted with main, so I closed it. The freeze ofbuiltinModulesthat it also did is independent of this bug and is not part of this change.getBuiltinLibs()already filters outnode:names (src/js/internal/repl/node-shims.ts:183).util.inspectonly looks up bare names taken from stack frames.bun linthands configs and plugins the array without Bun's own names (src/lint/driver/evaluate-start.js:13,src/lint/js_plugin/worker/main.js:45). They now seenode:test, as under Node.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/module/node-module-module.test.js