node:module: check exceptions from the parent lookup in the native require resolver

[robobun]

Problem

• Any program that overrides require("module")._resolveFilename and then calls require() aborts under JSC's exception-check validator:

  BUN_JSC_validateExceptionChecks=1 bun-debug test/js/node/module/resolveFilenameOverwrite.cjs
  ERROR: Unchecked JS exception:
      This scope can throw a JS exception: getImpl @ JavaScriptCore/JSOrderedHashTable.h:180
      But the exception was unchecked as of this scope: executeCallImpl @ JavaScriptCore/interpreter/Interpreter.cpp:1294
  vendor/WebKit/Source/JavaScriptCore/runtime/VM.cpp(1590) : void JSC::VM::verifyExceptionCheckNeedIsSatisfied(...)
  

• Cause: functionImportMeta__resolveSyncPrivate (src/jsc/bindings/ImportMetaObject.cpp) is the native resolver behind require() and require.resolve(). In its _resolveFilename override branch it calls globalObject->requireMap()->get(globalObject, from) (line 232) and parentID.toWTFString(globalObject) (line 244) with no RETURN_IF_EXCEPTION after either. Every other requireMap()->get() call site in the tree checks (JSCommonJSModule.cpp:1511, :1582, :1632, BunPlugin.cpp:689).
• from is this.filename of whatever require was invoked on, so the toWTFString genuinely throws when the parent's filename is an object with a throwing toString (for example Module.prototype.require.call(fakeModule, id)). A plain debug build then calls the override with that exception already pending and asserts:

  ASSERTION FAILED: Unexpected exception observed on thread ...
  !exception()
  JavaScriptCore/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()
  

  In release builds the error happens to propagate only because JSC's own executeCallImpl bails out on the pending exception before entering the override.
• The virtual-module branch a few lines above (line 217-218) coerces the same from (and the specifier) with toWTFString and is unchecked in the same way; with a virtual module registered (Bun.plugin build.module() or mock.module()) the same throwing parent trips the validator there instead (toWTFStringSlowCase unchecked as of the next scope in Bun__resolveSync).
• CI does not catch any of this because test/js/node/module/node-module-module.test.js, which spawns the _resolveFilename fixture, is listed in test/no-validate-exceptions.txt.

Fix

• Add RETURN_IF_EXCEPTION(scope, {}) after the four exception-capable calls in functionImportMeta__resolveSyncPrivate: the specifier and parent coercions in the virtual-module branch, and requireMap()->get() and the parent-id coercion in the override branch. scope is the function's DECLARE_THROW_SCOPE; the rest of the function (profiledCall, result.toString, the paths loop) already follows this pattern.
• Why this is correct: each of these calls can leave an exception pending (JSMap::get hashes the key under a throw scope; toWTFString on a non-string runs user toString), and the only sensible outcome is to propagate that exception to the require() caller. The checks do exactly that and change nothing on the non-throwing path; the eager coercion order is kept as it was.
• functionImportMeta__resolveSync (the public import.meta.resolveSync) has a similar-looking virtual-module branch but is not touched: it only ever passes a string or undefined as from and checks moduleName.isString() first, so neither coercion there can run user code.
• Verification (test/js/node/module/node-module-module.test.js, new test _resolveFilename override and virtual modules propagate parent lookup exceptions): it spawns a fixture under BUN_JSC_validateExceptionChecks=1 that exercises the override with a cached parent, require.resolve(..., { paths }) through the override, a throwing parent against the override (asserting the override is never called), and the same parent against the virtual-module branch for both a matching and a non-matching specifier, and asserts the transcript plus the validator's report lines plus exit code in one toEqual.
  • Unfixed debug build: fails with exit 134 and uncheckedScopes naming getImpl @ JSOrderedHashTable.h:180.
  • Fixed debug build: passes. resolveFilenameOverwrite.cjs and the brief's one-liner repro also run clean under the validator; the throwing-parent repro no longer asserts.
  • test/js/node/module/module-resolve-filename-paths.test.js and test/js/bun/plugin/plugins.test.ts (virtual modules via require()) pass on the fixed build.
  • On release builds the env var is a no-op and the test passes before and after; the debug and ASAN lanes are the ones that exercise it.
• node-module-module.test.js stays in test/no-validate-exceptions.txt for now: with this change applied, the file's remaining validator failures are the ones owned by node:module: check exceptions from jsString/jsSubstring in Module.wrap and new Module() #34745 (Module.wrap) and the Module.runMain override path (reported separately), so the entry can be dropped once those land too.
• Overlaps textually with node:module: throw instead of crashing when _resolveFilename is set to a non-callable #38089 (non-callable _resolveFilename), which re-indents this block without adding these checks, and with node:module: pass Node-compatible arguments to an overridden Module._resolveFilename #34102, which replaces the parent lookup in the override branch with arguments passed from JS (the virtual-module hunk is unaffected by it); whichever lands second needs a trivial rebase.

Background

• Throw scope / RETURN_IF_EXCEPTION: JSC host code declares a ThrowScope and must check for a pending exception after every call that may throw (RETURN_IF_EXCEPTION), instead of continuing to run with the exception set. Calling back into JS with an exception pending is a debug assertion (assertNoException), and in release it makes later throws silently replace or reorder the original error.
• BUN_JSC_validateExceptionChecks=1: debug-only JSC mode in which every scope that could have thrown records that the caller owes a check; creating the next scope (or destroying the current one) without that check aborts the process and prints the two locations shown above. Bun's ASAN CI lanes run tests with it enabled except for files listed in test/no-validate-exceptions.txt.
• functionImportMeta__resolveSyncPrivate: the $resolveSync builtin used by the CommonJS require/require.resolve implementation in src/js/builtins/CommonJS.ts. It receives the specifier and the parent's filename, first consults runtime virtual modules (Bun.plugin build.module() / mock.module()), then an overridden Module._resolveFilename if one was installed, then the native resolver.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 1 minute

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: 800e009f-b8db-431b-a011-12aba612a34c

📥 Commits

Reviewing files that changed from the base of the PR and between b7a0431 and 016b8e5.

📒 Files selected for processing (2)

• src/jsc/bindings/ImportMetaObject.cpp
• test/js/node/module/node-module-module.test.js

---

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

[robobun]

^{Updated 6:31 AM PT - Aug 13th, 2026}

✅ @robobun, your commit 016b8e5f190a62b69bac1864df53c18f7b72f7c6 passed in Build #94358! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38099

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

bun-38099 --bun

[robobun]

Status: fix pushed, waiting on CI and review.

Reproduced on a debug build of main with the existing fixture:

BUN_JSC_validateExceptionChecks=1 bun-debug test/js/node/module/resolveFilenameOverwrite.cjs
  This scope can throw a JS exception: getImpl @ JavaScriptCore/JSOrderedHashTable.h:180
  But the exception was unchecked as of this scope: executeCallImpl @ Interpreter.cpp:1294

and, without the validator, a parent whose filename has a throwing toString hits ExceptionScope::assertNoException() at the override call. The new test in test/js/node/module/node-module-module.test.js fails on an unfixed debug build (exit 134, validator naming getImpl @ JSOrderedHashTable.h:180) and passes with this change.

[claude]

LGTM — four mechanical RETURN_IF_EXCEPTION insertions after calls that can throw, matching the pattern already used elsewhere in the same function.

What was reviewed:

• The four new checks each follow a call that can leave an exception pending (toWTFString on user-controlled values, JSMap::get); scope is the function's existing DECLARE_THROW_SCOPE.
• Extracting from.toWTFString() to a local preserves the original evaluation order (moduleString first, then fromString) — no happy-path behavior change.
• The sibling functionImportMeta__resolveSync was intentionally left alone; the PR description's reasoning (its from is always string-or-undefined and moduleName.isString() is checked first) matches the code.
• Test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, single toEqual on transcript+validator lines+exit code) and covers both the override and virtual-module branches, including the negative assertion that the override is never entered when the parent coercion throws.

Extended reasoning...

Overview

This PR adds four RETURN_IF_EXCEPTION(scope, {}) checks in functionImportMeta__resolveSyncPrivate (src/jsc/bindings/ImportMetaObject.cpp): after the specifier and parent toWTFString coercions in the virtual-module branch, after requireMap()->get(), and after the parent-id toWTFString in the _resolveFilename override branch. It also adds a subprocess test in test/js/node/module/node-module-module.test.js that runs under BUN_JSC_validateExceptionChecks=1 and exercises all four sites.

Security risks

None. This adds exception propagation guards; it does not change validation, permissions, or any security-sensitive path. The only behavioral change is that a throwing toString on a fake parent's filename now propagates cleanly instead of entering the override with an exception already pending.

Level of scrutiny

Low-to-medium. The C++ change is a textbook JSC exception-check fix — the exact pattern the repo's review guidance mandates ("Exception checks after every call that can enter JS"), and the same pattern already used a few lines below for profiledCall, result.toString, and the paths loop. Each inserted check is placed immediately after the throwing call and before its result is used or the next scope is created. The virtual-module branch refactor (pulling from.toWTFString() out of the argument position) preserves evaluation order.

Other factors

• The PR description enumerates the other requireMap()->get() call sites in the tree that already check, so this brings the outlier in line.
• The similar-looking branch in functionImportMeta__resolveSync is deliberately not touched, and the stated reason (its from is only ever a string or jsUndefined(), and moduleName.isString() gates the coercion) checks out against the preloaded source.
• The new test uses tempDir, spreads bunEnv, drains stdout/stderr/exited concurrently, asserts a combined object, and surfaces the validator's "This scope can throw" / "But the exception was unchecked" lines so a regression points at the offending scope rather than just a truncated transcript. It also asserts overrideCalls: 0 to prove the override is not entered when the parent coercion throws, and re-verifies the happy path (virtual module still loads = true) after the throwing cases.
• No prior reviews or outstanding comments on the PR.