module loader: don't abort on .node specifiers with a query string

[robobun]

Repro

With any file named addon.node next to it:

import("./addon.node?v=1");

panic: internal error: entered unreachable code: napi modules go through provideFetch()

The process SIGABRTs. The same specifier without the query string gets the intended error instead (TypeError: To load Node-API modules, use require() or process.dlopen instead of import.). A static import "./addon.node?v=1" and require("./addon.node?v=1") abort the same way. So does import("./thing.xyz") under --loader=.xyz:napi, with no query string involved at all.

Cause

The loader classifier strips the ?query from the module key before mapping the extension, so /x/addon.node?v=1 classifies as Loader::Napi. But the checks that divert NAPI modules away from the transpiler (moduleKey->endsWith(".node") in moduleLoaderFetch, id.endsWith(".node") in overridableRequire) run on the raw key, which ends with ?v=1. The specifier slips past them into transpile_source_code_inner, whose NAPI arm was unreachable!(). The --loader <ext>:napi case reaches the same arm because the extension is not .node to begin with.

Fix

• src/runtime/jsc_hooks.rs: the Loader::Napi arm of the transpiler now throws the same TypeError the .node guard produces, instead of unreachable!(). Every spelling the string checks miss lands here, so no specifier or loader configuration can abort the process through this arm.
• src/js/builtins/CommonJS.ts: overridableRequire matches .node against the path portion of the resolved id (the id keys the module cache and keeps its ?query), and internalRequire passes the query-stripped path to process.dlopen. require("./addon.node?v=1") now loads the addon exactly like require("./addon.node").

The two endsWith(".node") checks in ZigGlobalObject.cpp are intentionally unchanged: they are still correct fast paths for the plain spelling, and the cases they miss now reach the new Loader::Napi arm and get the identical error.

Verification

Four tests added to test/js/bun/resolve/import-query.test.ts: dynamic import, static import, require, and --loader=.xyz:napi. All four abort the child process on the unfixed build and pass with the fix. The 11 existing tests in the file still pass, as do test/js/node/module/node-module-module.test.js, require-extensions.test.ts, and module-resolve-filename-paths.test.js.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 5 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: e306b7d0-aa0c-4996-9006-a3636ae71344

📥 Commits

Reviewing files that changed from the base of the PR and between 3e08719 and b9554ea.

📒 Files selected for processing (3)

• src/js/builtins/CommonJS.ts
• src/runtime/jsc_hooks.rs
• test/js/bun/resolve/import-query.test.ts

---

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

[robobun]

^{Updated 2:14 PM PT - Jun 30th, 2026}

❌ @robobun, your commit b9554ea has 3 failures in Build #67299 (All Failures):

• test/integration/next-pages/test/dev-server-ssr-100.test.ts - code 1 on 🍎 26 aarch64
• test/js/bun/http/bun-serve-file.test.ts - timeout on 🍎 26 aarch64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 33148

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

bun-33148 --bun

[claude]

I didn't find any issues — the fix is small, well-tested, and follows the existing null-check/throw_type_error pattern in adjacent loader arms — but since it touches the module loader and changes require() semantics for .node files (each distinct ?query now dlopens the same addon under a separate cache key), it's worth a quick human look.

Extended reasoning...

Overview

This PR fixes a process abort (unreachable!() panic) when a .node specifier carries a ?query suffix or when a custom extension is mapped to the napi loader. Three files change: src/runtime/jsc_hooks.rs replaces the unreachable!() in the Loader::Napi transpiler arm with the same TypeError the C++ fast-path produces; src/js/builtins/CommonJS.ts makes overridableRequire match .node against the path portion (before ?) and has internalRequire strip the query before calling process.dlopen; and four regression tests are added to test/js/bun/resolve/import-query.test.ts.

Security risks

None apparent. The Rust change introduces an unsafe { &*global_object } deref, but it is guarded by an explicit is_null() check and is byte-for-byte identical to the pattern used in the adjacent L::Wasm and file-loader arms in the same function. No new attack surface — the change converts an abort into a catchable TypeError.

Level of scrutiny

Moderate-to-high. The diff is small (~20 production lines) and mechanically follows established patterns, but it sits in the module loader and the CommonJS require() builtin — critical, widely-exercised paths. The CommonJS change also introduces a behavioral choice: require("./addon.node?v=1") and require("./addon.node?v=2") will now each dlopen the same on-disk file under distinct cache keys (mirroring how query strings work for JS modules). That is strictly better than the previous SIGABRT, but whether re-dlopening a native addon per query variant is the desired semantics is a design call worth a maintainer's confirmation.

Other factors

No CODEOWNERS cover these paths. The bug-hunting system found nothing. Test coverage is thorough (dynamic import, static import, require, and --loader=.xyz:napi), and the PR description verifies adjacent test suites still pass. The endsWith(".node", queryIndex) usage is correct (second arg is endPosition). I'm deferring only because of the criticality of the code path and the small embedded design decision, not because of any identified defect.

[robobun]

On the design point raised in the review, the per-?query cache key for require("./addon.node?v=1"):

That is Bun's existing semantics for a ?query on a resolved specifier. The query is part of the module cache key for every loader, which is the behavior the rest of import-query.test.ts exists to test; making .node the only loader that silently drops the query from the key would be the inconsistency. And before this PR the query spelling neither aliased to the plain one nor loaded separately: it aborted the process, so nothing depends on either outcome.

Two cache keys also do not mean two copies of the library. dlopen / LoadLibrary return the already-loaded handle for a repeated path, and Process::dlopen in BunProcess.cpp already has explicit repeat-load handling: the DLHandleMap replay for addons that self-register from a static constructor, and the napi_register_module_v1 fall-through for module-ABI addons. Both populate a fresh exports object from the already-loaded handle, which is what calling process.dlopen twice on the same path does today, so this PR does not introduce a new loading state for addons.

[robobun]

CI status: the only failure in build 67299 is test/js/node/test/parallel/test-net-connect-memleak.js on the two linux-x64-musl lanes (alpine 3.23 x64 and x64-baseline), on both attempts. That is the known musl flake tracked in #33044 (quarantine PR open at #33045): 11 of the 29 most recent Buildkite builds across unrelated branches hit the same failure, and nothing in this PR touches net, GC, or finalization.

The four tests added here (test/js/bun/resolve/import-query.test.ts) pass on every lane that has finished. Not retriggering for the musl lanes since they will stay red until #33044 is resolved; the diff is ready for review.