Implement module.syncBuiltinESMExports()

[robobun]

module.syncBuiltinESMExports() was a stub that returned undefined. A patch applied to a builtin's CommonJS exports object (the graceful-fs / APM instrument-and-patch pattern) never reached the ESM namespace bindings that were snapshotted from it at import time. Patch libraries saw a successful call and kept running, while every import ... from 'node:fs' consumer in the same process held the unpatched binding.

Repro

import { createRequire, syncBuiltinESMExports } from "node:module";
import * as fsns from "node:fs";
const cjs = createRequire(import.meta.url)("fs");
const orig = fsns.readFileSync;
cjs.readFileSync = function patched() { return "PATCHED"; };
syncBuiltinESMExports();
console.log(JSON.stringify({
  cjsPatched: String(cjs.readFileSync()),
  esmAfterSync: fsns.readFileSync === orig ? "orig" : String(fsns.readFileSync()),
}));

node v26.3.0: {"cjsPatched":"PATCHED","esmAfterSync":"PATCHED"}
bun 1.4.0:    {"cjsPatched":"PATCHED","esmAfterSync":"orig"}

Cause

jsFunctionSyncBuiltinESMExports in src/jsc/modules/NodeModuleModule.cpp was return jsUndefined();. Importing a builtin creates a SyntheticModuleRecord whose export values are copied out of the builtin's CommonJS exports object once, at instantiation. Nothing re-read them afterwards.

Fix

Walk the ESM registry for builtin module keys, and for each one fetch the builtin's CommonJS exports object (the same lookup process.getBuiltinModule() uses) and re-bind every named export through JSModuleNamespaceObject::overrideExportValue, which writes the module environment slot and touches its watchpoint set. That is the same path mock.module() and spyOn() already use, so both named imports and the namespace object observe the new value.

Semantics follow node's syncBuiltinESMExports in lib/internal/modules/esm/utils.js:

• exports deleted from the CommonJS object become undefined (the export itself stays)
• properties added to the CommonJS object afterwards do not become new exports
• default is left pointing at the exports object
• builtins that were never imported as ESM are untouched

Registry keys are snapshotted before any JS runs, because reading an export can invoke a getter that loads another module. Records that have not linked yet are skipped, matching the other registry walkers.

Prerequisite: one exports object per native builtin

This relies on an invariant node guarantees and bun was violating for nine builtins. node:buffer, node:constants, node:string_decoder, node:util/types, bun:jsc, bun:test, bun:app, utf-8-validate and abort-controller each have a CommonJS entry in InternalModuleRegistry, but their ESM namespace was built by running the native generator a second time. INIT_NATIVE_MODULE's putNativeFn mints a fresh JSFunction per run, so require() and import ended up with distinct-but-equivalent twins. Syncing would then swap every ESM binding for its twin even when nothing had been patched, breaking ===, WeakMap keys, and spies held against stored references.

They all already carry InternalModuleRegistryFlag, so they now fall through to generateInternalModuleSourceCode (which resolves via requireId) instead of being intercepted by the ESM switch. node:module, node:process and bun keep the native generator: their CommonJS value is a canonical singleton rather than a registry entry, and their generators already read off that same singleton.

This also closes a latent divergence:

	before	after	node
(await import('node:buffer')).default === require('node:buffer')	false	true	true

Verification

bun bd test test/js/node/module/node-module-module.test.js — 36 pass. The six new cases cover the repro, named-import bindings, delete/add semantics, a native builtin, identity stability of unpatched exports, and the untouched-until-called control. Three of the six fail on USE_SYSTEM_BUN=1.

The repro now prints node's output byte for byte. Also checked, not in the test file: a throwing getter propagates out of syncBuiltinESMExports() exactly as in node, the function works inside workers, and 200 syncs across 10 imported builtins under Bun.gc(true) plus a getter that loads a module mid-iteration run clean under BUN_JSC_validateExceptionChecks=1.

bun:test is one of the rerouted modules and is imported by every test file, so the blast radius got the most attention. Verified all nine now agree with require() on export names, export values and default, and ran test/js/bun/test/mock/ + mock-fn (95 pass), test/js/node/module/ (95 pass), buffer.test.js (543 pass), string_decoder/ (86 pass).

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 36 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: fec67084-4523-4bd8-a938-f98e2ded6701

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and 5cf9502.

📒 Files selected for processing (4)

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

---

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

[robobun]

^{Updated 3:20 PM PT - Jul 6th, 2026}

❌ @robobun, your commit 5cf9502 has some failures in Build #69132 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33434

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

bun-33434 --bun

[robobun]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

Found 3 issues this PR may fix:

1. Shimmer patching breaks on bundled bun code #13165 - Shimmer patching (used by Sentry, OpenTelemetry) fails with TypeError: Cannot replace module namespace object's binding because CJS patches to builtins don't propagate to ESM namespace bindings — exactly what syncBuiltinESMExports() fixes
2. OpenTelemetry doesn't seem to work on Bun 0.7.0 #3775 - OpenTelemetry instrumentation silently fails because it relies on require-in-the-middle which calls syncBuiltinESMExports() after monkey-patching builtins, but the stub returned undefined
3. Add an integration test that uses graceful-fs #7022 - graceful-fs monkey-patches node:fs via CJS and depends on syncBuiltinESMExports() to push patches to ESM consumers — the canonical use case for this API

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #13165
Fixes #3775
Fixes #7022

🤖 Generated with Claude Code

[robobun]

I checked each of these against the actual diff and I don't think any of the three should be auto-closed by this PR, so I'm leaving the Fixes lines out. Happy to be corrected.

#13165 (shimmer / Sentry): the failure there is Object.defineProperty() against a module namespace object, which JSModuleNamespaceObject::defineOwnProperty rejects. This PR doesn't touch that path, and node rejects it too:

bun (this branch): Cannot replace module namespace object's binding with configurable attribute
node v26.3.0:      Cannot redefine property: readFileSync

#7022 (graceful-fs): that issue asks for an integration test, and graceful-fs never calls syncBuiltinESMExports(). Its CJS patch failing to reach a hoisted import * as fs behaves the same under node, so there is nothing here to fix:

import * as fsns from "node:fs";          // hoisted, namespace snapshotted here
import { createRequire } from "node:module";
createRequire(import.meta.url)("fs").readFileSync = () => "patched";
fsns.readFileSync();                      // still the original, in bun AND node

#3775 (OpenTelemetry): that one is about --experimental-loader / --require hooks.

For what it's worth, I grepped the installed sources of @opentelemetry/instrumentation, require-in-the-middle, import-in-the-middle, shimmer, global-agent, nock and graceful-fs: none of them call syncBuiltinESMExports(). So this PR isn't the missing piece in those three reports.

What it does do is make the documented API behave like node's for code that does call it, instead of silently returning undefined.

[robobun]

Both findings were right. Fixed in 6bd6ff3.

The identity swap (blocker)

Reproduced exactly as described, and the diagnosis was correct down to the mechanism: the ESM path ran generateNativeModule_<Name> a second time, and putNativeFn mints a fresh JSFunction per run, so ESM and CommonJS held distinct-but-equivalent twins.

import { isUtf8 } from "node:buffer";
import { syncBuiltinESMExports } from "node:module";
const before = isUtf8;
syncBuiltinESMExports();   // nothing patched
console.log(isUtf8 === before);

before: bun false  /  node true
after:  bun true   /  node true

The note that my node:buffer test masked it by calling require() first was also correct. That test now additionally asserts bufferNamespace.default === buffer, and there is a new leaves unpatched exports identity-stable case covering a native builtin and a JS builtin together.

I went with the second suggestion (share the cache) rather than skipping natives, because skipping would have made require('node:buffer').Buffer = X; syncBuiltinESMExports() silently not propagate, which is the same class of half-applied instrumentation this PR set out to remove. The 9 modules in BUN_FOREACH_ESM_AND_CJS_NATIVE_MODULE all already carry InternalModuleRegistryFlag, so they just stop being intercepted by the ESM switch and fall through to generateInternalModuleSourceCode, which goes via requireId. node:module / node:process / bun keep the native generator, since their CommonJS value is a canonical singleton rather than a registry entry (m_nodeModuleConstructor, processObject(), bunObject()) and their generators already read off that same singleton.

This also fixes a latent divergence for free:

	before	after	node
(await import('node:buffer')).default === require('node:buffer')	false	true	true

Verified across all nine rerouted modules that ESM and CommonJS now agree on the export names, the export values, and default. bun:test is in that set and is imported by every test file, so the blast radius got the most attention: test/js/bun/test/mock/ + mock-fn (95 pass), test/js/node/module/ (95 pass), buffer.test.js (543 pass), string_decoder/ (86 pass).

The unlinked-record guard (nit)

Added if (!record->moduleEnvironmentMayBeNull()) continue; plus a null check on the namespace. You're right that it was the only getModuleNamespace() caller omitting both, and skipping an unlinked builtin is the correct semantics regardless of reachability.

[robobun]

Status: ready for review, CI red only on a starved darwin lane

Latest build (#69132, sha 5cf9502e): 280 jobs passed, 1 failed.

The single failure is :darwin: 26 aarch64 - test-bun, and it never ran a test:

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).

Why this is infrastructure and not the diff:

• That lane runs as two shards. On this build one shard passed and the other failed on the download, same commit, same artifact.
• On the previous build the upstream darwin-aarch64-build-bun step expired twice before passing, so the test job raced it.
• Of the 30 most recent non-main builds, zero darwin 26 aarch64 - test-bun jobs have executed at all (22 waiting, 26 scheduled, 10 cancelled). The darwin agent pool is saturated, which is what produces both the expired build steps and the 120s artifact-download timeouts.
• The binary-size annotation reports +0.0 KB on all 15 platforms.

The earlier red x64-baseline lanes were a transient 404 fetching bun-tracestrings@github:oven-sh/bun.report#912ca63 during bun install (that commit exists and its tarball serves 200; the dependency is pinned identically on main). They cleared on re-run.

I have used my one re-run and will not push again, since only a push re-triggers CI and more empty commits are noise.

What changed

Two commits:

1. Implement module.syncBuiltinESMExports(), which was a return jsUndefined() stub, so CommonJS patches to builtins never reached ESM consumers.
2. Make require() and import share one exports object for the nine native builtins that had two. This was a prerequisite: without it, syncing swapped every ESM binding for an identity-different twin even when nothing had been patched. Caught in review, fixed at the root rather than by excluding those modules.

Verification

• test/js/node/module/node-module-module.test.js: 36 pass. Six new cases cover the repro, named-import bindings, delete/add semantics, a native builtin, identity stability of unpatched exports, and an untouched-until-called control. Three of the six fail under USE_SYSTEM_BUN=1.
• The issue's repro now prints node's output byte for byte.
• bun:test is one of the rerouted modules and every test file imports it, so that got the most attention: all nine now agree with require() on export names, export values and default. test/js/bun/test/mock/ + mock-fn (95 pass), test/js/node/module/ (95 pass), buffer.test.js (543 pass), string_decoder/ (86 pass).
• A throwing getter propagates exactly as in node, the function works inside workers, and 200 syncs across 10 imported builtins under Bun.gc(true) plus a getter that loads a module mid-iteration run clean under BUN_JSC_validateExceptionChecks=1.

[robobun]

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.