node:module: build builtinModules from the same table as isBuiltin, and freeze it

[robobun]

Repro

import { builtinModules, isBuiltin } from "node:module";

console.log(builtinModules.includes("node:test")); // false, but...
console.log(isBuiltin("node:test"));               // true
console.log(typeof (await import("node:test")).test); // "function"

console.log(Object.isFrozen(builtinModules));      // false
builtinModules.push("zzz");
console.log(builtinModules.includes("zzz"));       // true, for every later reader

Node prints true, true, "function", true, then throws a TypeError on the push.

Cause

module.builtinModules and module.isBuiltin() read from two separate hardcoded tables:

• builtinModuleNames in src/jsc/modules/NodeModuleModule.cpp feeds builtinModules (76 names)
• builtinModuleNamesSortedLength in src/jsc/bindings/isBuiltinModule.cpp feeds isBuiltin() (78 names)

The second is a strict superset: it has node:test and bun:main, both of which the module loader resolves. So builtinModules listed no node:-prefixed name at all, and tools that decide "is this a builtin?" with builtinModules.includes(x) (bundler external lists, module mocks, dependency analyzers) disagreed with Bun's own loader.

The array was also never frozen, and builtinModules is a lazily-reified property that caches one array per global object. A push() from one module was visible to every later reader in the process.

Fix

One table, in isBuiltinModule.cpp, exposed as Bun::builtinModuleNames(). getBuiltinModulesObject builds the array from it and freezes it, matching Node.

builtinModules gains exactly the two entries it was missing: node:test (listed with the prefix, since the bare name is not a builtin, same as Node does for node:sea / node:sqlite / node:test) and bun:main. isBuiltin() consults the same set it did before, so its behavior is unchanged.

Bun only ships node:test as a prefix-only builtin, so that is the only node:-prefixed entry; node:sea, node:sqlite and node:test/reporters do not exist in Bun.

Verification

bun bd test test/js/node/module/node-module-module.test.js   # 32 pass

Three new/updated assertions fail on main and pass with the fix: the length is now 78, builtinModules.filter(n => !isBuiltin(n)) is empty and contains node:test, and Object.isFrozen(builtinModules) is true with push() throwing.

Also run:

• test/js/bun/test/parallel/test-require-builtins.ts (requires every entry of builtinModules in a subprocess, now including node:test and bun:main)
• test/js/node/test/parallel/test-module-builtin.js, test/js/node/test/parallel/test-require-resolve.js
• test/js/node/module/ (91 pass), test/js/bun/plugin/ (35 pass)

Exception-check validation

constructArray and objectConstructorFreeze each open a ThrowScope at the same recursion depth inside the lazy property callback, which BUN_JSC_validateExceptionChecks=1 flags unless the first is checked before the second opens. Both the CJS and ESM paths are clean:

BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1 bun bd -e 'require("module").builtinModules'

[robobun]

^{Updated 10:16 AM PT - Jul 6th, 2026}

❌ @robobun, your commit 0cb6a72 has some failures in Build #69024 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33430

That installs a local version of the PR into your bun-33430 executable, so you can run:

bun-33430 --bun

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c13364e1-0d82-499a-b754-81007e9e5535

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and 0cb6a72.

📒 Files selected for processing (4)

• src/jsc/bindings/isBuiltinModule.cpp
• src/jsc/bindings/isBuiltinModule.h
• src/jsc/modules/NodeModuleModule.cpp
• test/js/node/module/node-module-module.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

@coderabbitai review

(The previous attempt hit the review rate limit before it started.)

[claude]

LGTM — clean consolidation of the two duplicate builtin-module tables into one source of truth, plus freezing to match Node.

Extended reasoning...

Overview

This PR eliminates a duplicate hardcoded table: module.builtinModules and module.isBuiltin() previously read from two separate 76- and 78-entry ASCIILiteral[] tables in different files, which had drifted apart (node:test and bun:main were in one but not the other). The fix collapses them into one table in isBuiltinModule.cpp, exposes it via Bun::builtinModuleNames() returning a std::span, and has getBuiltinModulesObject build the JS array from that span and freeze it via JSC::objectConstructorFreeze. Four files touched: two C++ source files, one header, one test file.

Security risks

None. This is a static compile-time list of module name string literals; no user input flows into it, no auth/crypto/permissions surface is touched.

Level of scrutiny

Low-to-moderate. The change is fundamentally a table dedup plus a one-line Object.freeze for Node compat. I verified:

• The consolidated 78-entry table is set-identical to the old builtinModuleNamesSortedLength (just reordered from length-sort to alpha-sort); isBuiltinModule() does a linear scan so order is irrelevant to its behavior.
• The builtinModules array keeps the same relative order as the old NodeModuleModule.cpp table (both alpha-sorted), gaining bun:main in the bun:* block and node:test at the end.
• No other files reference the removed builtinModuleNames / builtinModuleNamesSortedLength symbols.
• bun:main is a real resolvable module (src/resolve_builtins/HardcodedModule.rs), so adding it to builtinModules won't break test-require-builtins.ts.

Other factors

• Exception handling is correct: constructArray over owned JSStrings and objectConstructorFreeze on a plain dense array cannot run user code, so scope.assertNoException() (rather than RETURN_IF_EXCEPTION) is the right choice, and the author verified with BUN_JSC_validateExceptionChecks=1.
• Freezing builtinModules is a behavior change but strictly Node-aligning; only code that mutates the shared array (which was always a bug) is affected.
• Tests cover the new length, isBuiltin agreement, node:test presence / bare test absence, and frozenness with a push() throwing TypeError.
• No CODEOWNERS on these paths; no outstanding human review comments.

[robobun]

Thanks, no changes needed from the review.

One judgement call worth flagging, since it is the only user-visible addition beyond `node:test`: `bun:main` now shows up in `builtinModules`. It is there because `isBuiltin("bun:main")` already returned true and the loader resolves it, so keeping it out of the array would mean either a second table or a filter, which is exactly the drift this PR removes. `bun:wrap` was already in the list, so it is consistent with what shipped before. Happy to drop it if you would rather `builtinModules` not carry that one.

[robobun]

Rebased onto main and force-pushed (d50c6e7f -> 308fc636). No changes to the diff itself, which is still the same 4 files.

The red CI on the old sha was not from this PR. Every test lane failed test/js/bun/cookie/cookie-map.test.ts with:

- "__Host-id=; Path=/; Expires=Fri, 1 Jan 1970 00:00:00 -0000; Secure; SameSite=Lax"
+ "__Host-id=; Path=/; Expires=Thu, 01 Jan 1970 00:00:00 GMT; Secure; SameSite=Lax"

I branched off 9f29e551, which has #32926 (Cookie Expires switched to IMF-fixdate) but not #33425 (which updated the remaining cookie-map assertions to match). That one commit is the only thing I was behind, so the base itself was briefly red there.

Confirmed by running both test-file versions against the same rebased binary:

cookie-map.test.ts from	result
9f29e551 (my old base)	30 pass, 3 fail
48ff9eb2 (current main)	33 pass, 0 fail

The other failures in build 68811 were single-lane one-offs (napi, autobahn, webview-chrome, spawn-pipe-leak, serve-http3, bake/deinitialization, bun-install), none on a path this PR touches.

test/js/node/module/node-module-module.test.js still passes on the rebased build (32 pass). New build: https://buildkite.com/bun/bun/builds/68961

[claude]

I didn't find any bugs, but you flagged the bun:main inclusion as a judgement call you'd like a maintainer to weigh in on, and freezing builtinModules is a user-visible behavior change — worth a quick human sign-off.

Extended reasoning...

Overview

This PR consolidates two duplicate hardcoded builtin-module tables into a single source of truth in isBuiltinModule.cpp, exposed via Bun::builtinModuleNames(). getBuiltinModulesObject in NodeModuleModule.cpp now builds module.builtinModules from that shared table and freezes the resulting array to match Node.js. The net user-visible effect is that builtinModules gains two entries (node:test and bun:main, going from 76 → 78) and becomes immutable. isBuiltin() behavior is unchanged since it already consulted the 78-entry table (just reordered from length-sorted to alphabetical, which doesn't matter for a linear scan).

Security risks

None. This is a static string table and array construction; no untrusted input is parsed, no auth/crypto/permissions paths are touched.

Level of scrutiny

Medium. The refactor itself is mechanical and well-motivated (eliminating drift between two tables). However, this is user-facing Node-compat API surface: freezing builtinModules will make previously-silent .push()/.sort() calls throw, and adding bun:main to the public list is an API-shape decision the author explicitly flagged as open ("Happy to drop it if you would rather builtinModules not carry that one"). The C++ side also adds a ThrowScope with scope.assertNoException() after constructArray/objectConstructorFreeze — the reasoning (neither runs user code on a dense string array) looks sound and was validated with BUN_JSC_validateExceptionChecks=1, but JSC exception-scope handling in a lazy PropertyCallback is worth a maintainer glance.

Other factors

• I verified the new 78-entry table is set-identical to the old builtinModuleNamesSortedLength table, so isBuiltin() semantics are preserved.
• No internal code mutates builtinModules (grep confirms), so freezing won't break Bun itself.
• Tests are solid: length assertion updated, new invariant test that every builtinModules entry passes isBuiltin(), and a freeze test. The PR description lists several downstream test suites that were run.
• The author's own comment on the thread is soliciting maintainer input on the bun:main inclusion, which is a clear signal this shouldn't be auto-approved.

[robobun]

Agreed both points want a human call. Here is the data so it is a quick one.

bun:main

The C++ table is a curated public list, not a mirror of the loader. The loader resolves bun:app and bun:internal-for-testing too, but both tables deliberately omit them. So there are already two tiers, and bun:main was put in the public one:

specifier	isBuiltin()	in builtinModules
bun:main	true	true (was false)
bun:wrap	true	true
bun:ffi	true	true
node:test	true	true (was false)
bun:app	false	false
bun:internal-for-testing	false	false

isBuiltin("bun:main") already returns true on shipped Bun. This PR only makes the array agree with the predicate that was already there, and bun:wrap (equally internal plumbing, also a synthetic entry! in HardcodedModule.rs) has been in the array all along.

Omitting bun:main from the array alone would re-create the exact bug this PR fixes, just under a different name. If you would rather it be hidden, the consistent change is to drop it from the shared table, which also flips isBuiltin("bun:main") to false. That is a one-line change and I am happy to make it, but it is a behavior change to isBuiltin on top of this one, so I did not fold it in unasked.

Freezing

Matches Node, which has frozen it for years. Nothing inside Bun mutates the array (the one build-time consumer, src/node-fallbacks/build-fallbacks.ts, already copies with [...builtins]). The only code that breaks is code mutating a process-wide shared array, which was never safe.

CI

Green on the rebased sha so far: https://buildkite.com/bun/bun/builds/68961 (0 failed jobs; the previous build's 14 red lanes were the stale cookie-map base, fixed by the rebase).

[robobun]

Force-pushed 308fc636 -> f9258667 to re-roll a build. The commit content is byte-identical (amend only), so the diff is unchanged and the earlier review still applies.

For the record, build 68961's failures were unrelated to this PR:

• test/bake/dev-and-prod.test.ts on windows-11-aarch64 (2 retries)
• test/cli/install/bun-install-registry.test.ts on windows-11-aarch64 (1 retry)
• one darwin-26-aarch64 test-bun job that exited 1 with no test annotation

No annotation in that build mentions builtinModules, isBuiltin, or node-module-module, and none of those paths are touched here. New build: https://buildkite.com/bun/bun/builds/69008

Also confirmed the release profile, not just debug:

$ bun run build:release test test/js/node/module/node-module-module.test.js
32 pass, 0 fail

The release binary reports builtinModules.length === 78, Object.isFrozen(builtinModules) === true, and push() throwing TypeError, same as the debug build.

[robobun]

Status: diff is green, CI is red on infrastructure only

Build 69024 at 0cb6a72a finished: 280 jobs passed, 2 failed.

Both failures are the same job (:darwin: 26 aarch64 - test-bun) and its retry, and neither ran a single test. They died downloading the build artifact:

WARN  Error trying to download https://buildkiteartifacts.com/... (503 Service Unavailable)
Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
       Refusing to continue with a partial download (would silently fall back to the wrong binary).

Every other test-bun lane is green, including debian 13 x64-asan.

The test files that appear in the build annotations (bun-install, node-http-uaf, postgres-binary-numeric, postgres-simple-query-pipeline, webview-chrome, fetch-abort-slow-connect, napi) each failed once on a single lane, retried, and passed. None of them failed the build, and none touch module resolution. No annotation in the build mentions builtinModules, isBuiltin, or node-module-module.

I am not going to force-push again to re-roll a 503, since that just queues another 280-job build without learning anything. A maintainer re-running the darwin job should clear it.

What this PR needs

1. A re-run of the darwin artifact download, or a merge once it clears.
2. A decision on bun:main appearing in builtinModules (detail in the earlier comment). Short version: isBuiltin("bun:main") already returns true on shipped Bun, so this PR only makes the array agree with the predicate. If you would rather it stay out of the public array, the consistent fix is to drop it from the shared table, which also flips isBuiltin("bun:main") to false. One-line change, happy to make it, but it is a behavior change on top of this one so I did not fold it in unasked.

Verified locally on both profiles: debug+ASAN and release each build clean and pass test/js/node/module/node-module-module.test.js (32 pass).

[robobun]

Closing this. Main has moved the other way since this PR was opened. #31831 removed bun:wrap from builtinModules on purpose and kept it in the isBuiltin() table. The isBuiltin() table is now a deliberate superset of builtinModules, so one shared table would revert that change. The branch also conflicts with main: node:sqlite (#32498) and node:quic (#32602) were added to the tables in the meantime.

The one entry this PR found missing, node:test, is added on its own in a replacement PR from the branch farm/dd1f684a/builtin-modules-node-test. The freeze of builtinModules is independent of that bug and is not part of the replacement.