Conversation
…le._extensions Node.js routes every load handled by the CJS loader through Module._extensions[ext]: the entrypoint, nested require(), and a CJS module reached from an ESM import (via the ESM loader's commonjs translator). Bun only routed nested require(), so a -r/--require preload hook (babel-register, pirates, nyc) left the entry file untransformed and was a no-op for any CJS reached from an ESM entry. Both bypass cases funneled through fetchESMSourceCode, which called Bun__transpileFile with isCommonJSRequire=false and had no handler for the CommonJSCustomExtension result tag. This computes the file's module_type before the require.extensions check and gates on (is_commonjs_require || module_type != Esm) so the entrypoint and ESM-imports-CJS paths consult the override map too, while .mjs/.mts and .js under "type": "module" remain on the ESM path. fetchESMSourceCode now handles CommonJSCustomExtension by creating a JSCommonJSModule with the handler stashed on m_pendingCustomExtension; the deferred synthetic-module generator invokes it in place of evaluateCommonJSModuleOnce. Also adds Node's .js fallback to find_longest_registered_extension so .cjs files use an overridden .js handler (Node's _extensions has no .cjs key and falls back). The fallback is restricted to .cjs and extensions Bun has no native loader for, so hooking .js does not hijack .jsx/.tsx.
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Updated 11:05 PM PT - Jul 25th, 2026
❌ @robobun, your commit 0c9da8d has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35774That installs a local version of the PR into your bun-35774 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…et-or-create
When an ESM entry statically imports two CJS siblings and the first one's
body require()s the second, overridableRequire finds the second already in
requireMap (put there at fetch time with null sourceCode and the extension
handler stashed on m_pendingCustomExtension). load() bailed on
sourceCode.isNull() without checking the pending handler, so require()
returned the empty {} exports object. load() now runs the pending handler
first.
Also extracts the common requireMap get-or-create block shared by the
Ref<SourceProvider> createCommonJSModule overload and the new custom
extension path into getOrCreateCommonJSModule.
…file The != Esm gate also caught ModuleType::Unknown (a .js/.ts with no nearest package.json type), which regressed Bun's syntax-driven ESM auto-detection: a passthrough .js override made import of an ESM-syntax .js in that state re-enter the ESM loader on its own in-flight key and fail. Gating on == Cjs leaves Unknown to the parser as before. Moved the new tests to require-extensions-entrypoint-esm.test.ts and reverted require-extensions.test.ts to main; that file's in-process 'custom require extension 1' is flaky in release builds depending on the on-disk runtime-transpiler-cache state (reported separately), and touching it pulls that flake into the gate. Added a regression test for the no-package-json auto-detect case.
…itional .js fallback, drop unused filename parameter Pass lr.path.text (query-stripped) instead of the raw specifier to find_longest_registered_extension at the first call site so a ?query suffix cannot defeat the native-loader guard and hijack e.g. .jsx?v=1. Drop the !last_ext.is_empty() guard so extensionless basenames and pure dotfiles fall back to .js like Node. Remove the unused String filename parameter from evaluateCommonJSCustomExtension and the toWTFString + exception check in JSCommonJSModule::load() that only existed to fill it.
getOrCreateCommonJSModule can return an already-evaluated module (require() then import() of the same key). Setting the one-shot field on it would leave it stashed forever and, if user code later flipped module.loaded = false, cause load() to re-run the handler.
There was a problem hiding this comment.
All prior findings are addressed and this pass surfaced nothing new, but this is a ~350-line change to the core module-loader path (fetchESMSourceCode, JSCommonJSModule::load, transpile_file's require.extensions gate) with an explicitly acknowledged Node divergence for ModuleType::Unknown, so a human should sign off.
What was reviewed:
- New
m_pendingCustomExtensionWriteBarrier: visited invisitChildrenImpl, cleared before invoking the handler in bothload()and the synthetic-source lambda, and only set when!hasEvaluated. getOrCreateCommonJSModulerefactor: behavior-preserving vs. the pre-PRRef<SourceProvider>overload; the un-refactoredResolvedSource&overload's isolation-cache work is intentionally left inline.find_longest_registered_extension.js fallback: theEXTENSIONS_DEFAULT_KEYSearly-return keeps.ts/.mjs/etc. from falling through, and theDEFAULT_LOADERSguard keeps.jsx/.tsxfrom being hijacked; both call sites now pass the query-strippedlr.path.text.- New
fetchESMSourceCodeCommonJSCustomExtensionbranch mirrors the adjacentisCommonJSModuleblock's exception/promise handling.
Extended reasoning...
Overview
Routes CJS entrypoints and CJS-reached-from-ESM through Module._extensions, matching Node's behavior for tools like babel-register/pirates/nyc. Touches transpile_file in jsc_hooks.rs (moves the module_type sniff before the require.extensions gate and widens the gate to is_commonjs_require || module_type == Cjs), adds a CommonJSCustomExtension handler to fetchESMSourceCode, adds a GC-visited m_pendingCustomExtension slot on JSCommonJSModule consumed by both load() and the synthetic-source generator, extracts a shared getOrCreateCommonJSModule helper, and adds Node's .js fallback to find_longest_registered_extension (restricted so it doesn't hijack Bun-native loaders). Seven new subprocess tests cover entrypoint, .cjs fallback, ESM-import-of-CJS, two negative cases, sibling cross-require, and an end-to-end source-transforming hook.
Security risks
None identified. No untrusted input parsing, no path resolution changes, no new syscall surface. The .js fallback is guarded by DEFAULT_LOADERS/EXTENSIONS_DEFAULT_KEYS so a .js override cannot hijack Bun-native extensions.
Level of scrutiny
High. This is the module loader — every import/require in every Bun program flows through the touched code. The change reorders transpile_file's gate, adds a deferred-evaluation slot with GC and exception-path implications, and refactors a createCommonJSModule overload. The four prior review rounds each surfaced a real correctness issue (sibling cross-require returning {}; ModuleType::Unknown re-entering the loader on an in-flight key; query-suffixed specifiers defeating the fallback guard; a stale one-shot slot). All were fixed, but the density of subtle interactions here is exactly the kind of thing a maintainer with module-loader context should look at.
Other factors
- The author explicitly documents a Node divergence: a CJS-syntax
.jswith no package.jsontypereached from ESM import no longer hits the hook (gating on== Cjsinstead of!= Esmto preserve Bun's syntax auto-detection). That's a design trade-off a maintainer should ratify. - The
getOrCreateCommonJSModulerefactor changes one behavior of theRef<SourceProvider>overload: when the requireMap already has a matching entry, the passedsourceProvideris now dropped instead of being assigned to the module'ssourceCode— actually, on re-read, the pre-PR code also only usedsourceProviderin the!moduleObjectbranch, so this is behavior-preserving. TheResolvedSource&overload was intentionally left un-refactored per prior discussion. - Test coverage is good (7 concurrent subprocess tests, all hermetic, exact-value assertions, both positive and negative cases) and the mechgate evidence shows fail-without-fix / pass-with-fix on ASAN debug.
|
CI on build 81609 (finished, 193/196 jobs passed): The only hard failure is the The three |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, 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. |
2 participants
What
Module._extensions['.js']/require.extensionsoverrides now fire for the CommonJS entrypoint and for CJS modules reached via ESMimport/import(), matching Node.js. Previously only nestedrequire()hit the hook, so a-rpreload hook (babel-register, pirates, nyc) left the entry file untransformed and was a complete no-op for any CJS reached from an ESM entry.Repro
Cause
Both bypass cases funneled through
fetchESMSourceCode, which calledBun__transpileFilewithisCommonJSRequire=false(so therequire.extensionscheck intranspile_filewas skipped) and had no handler for theCommonJSCustomExtensionresult tag. The result went straight tocreateCommonJSModule/evaluateCommonJSModuleOncewith no_extensionsconsultation.Separately,
find_longest_registered_extensionhad no.jsfallback, so.cjsfiles never matched an overridden.jshandler (Node's_extensionshas no.cjskey and falls back to.js).Fix
src/runtime/jsc_hooks.rs: computemodule_typebefore therequire.extensionscheck and gate on(is_commonjs_require || module_type == Cjs)so the entrypoint and ESM-imports-CJS paths consult the override map too, while.mjs/.mts,.jsunder"type": "module", and.js/.tswith no package.jsontype(where Bun auto-detects from syntax) stay on the existing path.src/jsc/bindings/ModuleLoader.cpp: handleCommonJSCustomExtensioninfetchESMSourceCodeby creating aJSCommonJSModulewith the handler stashed on a newm_pendingCustomExtensionfield. The deferred synthetic-module generator andJSCommonJSModule::load()invoke it instead ofevaluateCommonJSModuleOnce, so a siblingrequire()of a module pre-fetched by the ESM loader also runs the handler.src/jsc/NodeModuleModule.rs: add Node's.jsfallback tofind_longest_registered_extension, restricted to.cjsand extensions Bun has no native loader for so hooking.jsdoes not hijack.jsx/.tsx/.toml.src/jsc/bindings/JSCommonJSModule.cpp: extract the requireMap get-or-create block shared by twocreateCommonJSModulepaths intogetOrCreateCommonJSModule.Verification
New tests in
test/js/node/module/require-extensions-entrypoint-esm.test.tscover: CJS entrypoint,.cjsfallback, ESM-import-of-CJS, two negative cases (ESM.jsunder"type": "module"and auto-detected ESM.jswith no package.json type), a sibling-cross-require case, and an end-to-end source-transforming preload hook. Existingrequire-extensions/require-extensions-override/22929-module-extensions-asisuites pass unchanged.[review] gate passed · iteration 1 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file