Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 17 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 (5)
Comment |
|
Updated 3:46 AM PT - Aug 1st, 2026
❌ @robobun, your commit 47bfecc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35592That installs a local version of the PR into your bun-35592 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found after the 03a7b89/5429c720 follow-ups, but this changes user-facing Bun.plugin semantics (onLoad/onResolve no longer fire on later preload files or their imports), so worth a maintainer confirming that's the intended contract — particularly that dropping onResolve for cross-preload imports (while keeping build.module()) is acceptable.
What was reviewed:
- Confirmed
run_on_resolve_pluginsis the sole path toBun__runOnResolvePlugins(VirtualMachine.rs:6572 and PluginRunner.rs:71 both route through it), so the single gate covers module-loader, resolve_hook, and linker onResolve. - Confirmed
run_on_load_pluginshas one caller (Bun__runVirtualModule), and C++runVirtualModuleonly handlesvirtualModulesbefore falling back — filter-based onLoad always reaches the gated Rust path. - Checked
is_in_preloadset/clear inload_preloadsuses a scopeguard (jsc_hooks.rs:678/685); no stale per-site gates remain in VirtualMachine.rs/jsc_hooks.rs after the move to the shared helper.
Extended reasoning...
Overview
Two-line runtime change plus ~120 lines of tests. run_on_resolve_plugins (JSGlobalObject.rs) now early-returns Ok(None) when vm.is_in_preload, and Bun__runVirtualModule (ModuleLoader.rs) adds || vm.is_in_preload to its existing plugin_runner.is_none() bail-out. Net effect: while load_preloads is running, registered onLoad/onResolve filter callbacks are suppressed; build.module() virtual modules (handled in C++ before the gated Rust fallback) still work. Three subprocess tests cover bunfig preload, --preload flags, and the virtual-module carve-out. My earlier round flagged a missing fourth onResolve site (the linker) and a vacuous test — both addressed by moving the gate into the shared FFI wrapper and adding a static ./helper.ts import plus a dummy build.onLoad to force plugin_runner = Some.
Security risks
None. No parsing of untrusted input, no new FFI surface, no allocation changes. The gate reads a same-thread bool that's already scopeguard-managed around the preload loop.
Level of scrutiny
Medium. Mechanically the change is trivial and correct — I traced every caller of both plugin-dispatch helpers and confirmed the shared gate covers them, and confirmed C++ runVirtualModule only short-circuits for build.module() entries before hitting the gated Rust fallback. What warrants a human is the semantic decision: this silently changes when plugin hooks fire for anyone with multiple preloads, and the trade-off (fixing #9373's surprise interception vs. breaking a hypothetical setup where preload-2 relies on preload-1's onResolve for a custom namespace) is a product call. The build.module() carve-out suggests the author considered cross-preload dependencies, but a maintainer should sign off on where the line is drawn.
Other factors
- PR description is stale — it still names
VirtualMachine.rsandjsc_hooks.rsas changed, but the final diff moved the gate to the shared helper so only JSGlobalObject.rs and ModuleLoader.rs are touched. Not a correctness issue, just worth updating. - comment-cop feedback was addressed in 5429c72 (comments are single-line now).
- Minor asymmetry: the onResolve gate lives in the shared helper but the onLoad gate lives in the sole caller. Fine today since
run_on_load_pluginshas exactly one caller; noting it only for future readers. - Tests follow harness conventions (
tempDir,bunEnv, concurrent subprocess drain, exit-code asserted last) and the summarize helper asserts the negative contract (hooksOnPreloadFiles: []) rather than just positive output.
66f6db1 to
015e443
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/internal/inspector/cdp.ts:1-11— This PR adds two brand-new files that are entirely unrelated to the preload-plugin fix:src/js/internal/inspector/cdp.ts(702-line CDP↔JSC inspector protocol adapter) andsrc/jsc/bindings/BunDebugger.h(declares fourjsFunction_*NodeInspectorhost functions). Grep confirms neither is referenced anywhere else in the tree — no consumer, no#include, noJSC_DEFINE_HOST_FUNCTION— so both are dead code as landed, and comment-cop has 17+ unresolved findings on cdp.ts. Drop these two files from this PR and land them with the node:inspector work they belong to.Extended reasoning...
What the issue is
This PR is titled "Bun.plugin: don't run onLoad/onResolve hooks while loading preload files" and its description covers only the preload/plugin gating fix (issue #9373). The diff for that fix is small and focused:
VirtualMachine.rs,ModuleLoader.rs,JSGlobalObject.rs,jsc_hooks.rs, andplugins.test.ts. But the PR also adds two entirely new, unrelated files:src/js/internal/inspector/cdp.ts— a 702-line Chrome DevTools Protocol ↔ JSC inspector protocol adapter, self-described in its header comment as serving "clients of node:inspector (Chrome DevTools, vscode-js-debug, vitest --inspect, ...)".src/jsc/bindings/BunDebugger.h— declaresjsFunction_openNodeInspector,jsFunction_waitForNodeInspectorConnection,jsFunction_postNodeInspectorControl, andjsFunction_closeNodeInspector, commented as "node:inspector's inspector.open() / close() / waitForDebugger()".
Neither has any connection — code or conceptual — to
Bun.plugin,onLoad/onResolve, or preload gating.Why they're dead code as landed
Grep across the tree confirms:
InspectorCDPAdapterandinternal/inspector/cdpappear only insrc/js/internal/inspector/cdp.tsitself. There is norequire("internal/inspector/cdp")or import ofInspectorCDPAdapteranywhere.BunDebugger.hand each of the fourjsFunction_*NodeInspectorsymbols appear only in the new header itself. There is no#include "BunDebugger.h"in any.cppfile, and noJSC_DEFINE_HOST_FUNCTIONfor any of the four declared functions.
So both files compile/bundle into the artifact but nothing calls into them. This is exactly the pattern REVIEW.md's "Delete dead code in the same PR that makes it dead" rule targets — except here the code is born dead in a PR that never made it live.
Why existing process didn't catch it
The PR title/description makes no mention of node:inspector, CDP, or debugger work — so a reviewer scanning the description against the diff would have no reason to expect these files, and CodeRabbit was rate-limited on this PR. The comment-cop bot did flag it: there are 17+ unresolved
comment-cop:src/js/internal/inspector/cdp.ts:*and onecomment-cop:src/jsc/bindings/BunDebugger.h:*inline comment on the PR, none of which the author has addressed — a strong signal these files were not intended to be reviewed here.Impact
Merging as-is lands 717 lines of unreviewed, untested, unreferenced code on a focused bugfix. Concretely:
- Review/bisect pollution:
git blame/git bisecton the future node:inspector feature will point at a preload-plugin PR whose description explains nothing about CDP. - REVIEW.md scoping: "don't ride file-wide standardization on a focused bugfix" and "Every behavioral change ships an automated test in the same PR" — this ships a 702-line protocol adapter with zero tests.
- CI: the 17+ unresolved comment-cop findings on
cdp.tsblock merge anyway.
Step-by-step proof
git diff --name-only main...HEADlists 7 files; two aresrc/js/internal/inspector/cdp.ts(new, +702) andsrc/jsc/bindings/BunDebugger.h(new, +15).rg -l 'InspectorCDPAdapter|internal/inspector/cdp'→ onlysrc/js/internal/inspector/cdp.ts.rg -l 'BunDebugger\.h|jsFunction_(open|close|waitFor|post)NodeInspector'→ onlysrc/jsc/bindings/BunDebugger.h.- Therefore no
.cppincludes the header (the fourJSC_DECLARE_HOST_FUNCTIONs have no matchingJSC_DEFINE_HOST_FUNCTION), and no built-in modulerequires the CDP adapter. - The PR description's "Cause" and "Fix" sections reference only
load_preloads,vm.currently_loading_preload,Bun__runVirtualModule, andrun_on_resolve_plugins— none of which touch inspector code.
How to fix
git rm src/js/internal/inspector/cdp.ts src/jsc/bindings/BunDebugger.hand force-push. These files belong on the separate node:inspector branch they came from, where they'll land alongside the.cppthat defines the host functions, therequire("internal/inspector/cdp")consumer, and the tests that exercise the adapter.
|
The two stray inspector files ( |
There was a problem hiding this comment.
All three prior review rounds are addressed (linker onResolve path now covered via the shared run_on_resolve_plugins gate; currently_loading_preload is dropped in VirtualMachine::destroy(); the build.module test forces plugin_runner = Some). I didn't find anything further, but this changes runtime plugin-dispatch semantics and adds a heap-owning VM field, and the approach pivoted mid-review from a blanket is_in_preload gate to a per-entry-path gate — worth a maintainer confirming that semantic choice.
What was reviewed:
- Traced both
run_on_resolve_pluginscallers (module-loader resolve at VirtualMachine.rs:6583 and linker via PluginRunner.rs:71) against the newnamespace=="" && path == currently_loading_preloadcheck — the entry file is skipped, its imports are not, matching the stated intent. currently_loading_preloadlifecycle: init viaptr::write, per-iterationclear()+extend_from_slice, scopeguard clear on allload_preloadsexits, explicitdrop(mem::take)indestroy().- The four tests' assertions (
hooksOnPreloadEntries: [], cross-preload virt: import, build.module with a live plugin_runner) map onto the gated sites.
Extended reasoning...
Overview
Fixes #9373: when multiple preload files each register Bun.plugin hooks, the first preload's onLoad/onResolve was intercepting subsequent preload entry files. The fix tracks the resolved path of the preload entry currently being imported on vm.currently_loading_preload and short-circuits run_on_resolve_plugins (the shared FFI wrapper both the module-loader resolve path and the linker route through) and Bun__runVirtualModule when the specifier equals that path. Imports of a preload file still flow through already-registered hooks — covered by two negative tests (virt: namespace and build.module).
Five files: a new Vec<u8> field on VirtualMachine (declared, ptr::write-initialised, dropped in destroy()), a small is_loading_preload_entry helper, the two gate sites, the load_preloads write + scopeguard clear, and four subprocess tests in plugins.test.ts.
Security risks
None identified. The change narrows when user plugin callbacks run (skips them for preload entry files), so the attack-surface delta is a reduction. No new untrusted input parsing; the only new comparison is a byte-slice equality against a path Bun itself resolved.
Level of scrutiny
Medium-high. The mechanism is small, but it lives in the module-loader / plugin-dispatch path and adds state to VirtualMachine with the raw-dealloc lifecycle constraint (which the earlier revision missed and has now fixed). More importantly, the approach changed mid-review: the first revision gated on is_in_preload (all hooks off during preload), then pivoted in fe9a933 to a per-entry-path gate so a later preload can still consume an earlier preload's loader. That's a reasonable and now well-tested design, but it's a user-facing semantic choice about how preloaded plugins compose that a maintainer should sign off on rather than a mechanical bug fix.
Other factors
Three prior review rounds each surfaced a real issue (missing linker-path coverage, a test that didn't reach the gate, and the destroy() leak) — all addressed. The current bug-hunt found nothing. Tests follow harness conventions (tempDir, bunEnv, Promise.all drain, exitCode last, it.concurrent) and assert both the fix (no hooks on plugin-N.ts) and the preserved behavior (cross-preload virt:/build.module still work). Given the iteration history and the semantic decision embedded in the fix, deferring rather than approving.
There was a problem hiding this comment.
My earlier findings are all addressed (linker onResolve path now covered via the shared run_on_resolve_plugins gate, the virtual-module test now flips plugin_runner to Some, and currently_loading_preload is dropped in VirtualMachine::destroy()). No further issues found, but this changes preload/plugin-dispatch semantics in the module loader and encodes a design choice — skip only the preload entry file itself, let its imports flow through already-registered hooks — that's worth a human sign-off.
What was reviewed:
- The
currently_loading_preloadlifecycle: written per-iteration inload_preloads, cleared by scopeguard, and reclaimed indestroy()alongsidepreload. - Both gate sites compare against the resolved path that
load_preloadspasses toJSModuleLoader::import_ptr, so the specifier seen byBun__runVirtualModuleand the resolve hook is the same string that was stored. - The
namespace_.length() == 0guard means namespaced specifiers (e.g.virt:...) are never suppressed — matches the third test's contract.
Extended reasoning...
Overview
Adds a currently_loading_preload: Vec<u8> field to VirtualMachine holding the resolved path of the preload entry load_preloads is currently importing. Bun__runVirtualModule (onLoad) and JSGlobalObject::run_on_resolve_plugins (onResolve, shared by the module-loader resolve hook, resolve_hook, and the linker's PluginResolver::on_resolve) short-circuit when the specifier equals that path. load_preloads sets the field before each import_ptr call and a scopeguard clears it on exit; VirtualMachine::destroy() reclaims it. Four subprocess tests cover bunfig preload, --preload flags, cross-preload virtual namespaces, and build.module().
Security risks
None. This narrows when user plugin callbacks run; no new inputs are parsed and no trust boundary moves.
Level of scrutiny
Moderate-to-high. Module loading and plugin dispatch are on the critical path for every bun run, and the change is a user-visible semantic shift. The PR also went through three revisions in response to review — each fixing a real defect (missed linker dispatch site, non-load-bearing test, worker-VM leak) — which is a signal the area is subtle.
Other factors
The design pivoted mid-PR from "suppress hooks for everything under is_in_preload" to "suppress only the exact entry file". That's the right call for the virt: cross-preload use case (covered by a test), but it means a preload file's own static imports still route through earlier preloads' hooks. The PR description states this is intentional; a maintainer should confirm that's the desired contract before merging. All three of my prior inline findings are resolved in the current diff, and the bug-hunting pass found nothing new.
|
CI on d020b7a (build 80974): the six failing tests are all tagged flaky by the annotator and none are in the plugin/preload area this PR touches ( |
When multiple preload files each register runtime plugin hooks, the first
preload's onLoad/onResolve would intercept the loading of subsequent
preload files. With bunfig preload = [a.ts, b.ts, c.ts] and a.ts
registering onLoad({filter: /\.ts$/}), b.ts and c.ts would be loaded
through a.ts's onLoad callback instead of the normal loader.
The module loader's plugin path checks only plugin_runner.is_some() and
not is_in_preload, so once the first preload flips plugin_runner to Some,
every subsequent preload file routes through it. Gate Bun__runVirtualModule
and the two runtime onResolve call sites on !is_in_preload so preload
files (and their imports) load with the default loader; once preload
finishes, all registered hooks apply to the entrypoint as before.
build.module() virtual modules remain available across preloads since
those are resolved on the C++ side before the gated Rust fallback runs.
Fixes #9373
The module-loader resolve call sites are not the only path to the runtime onResolve hooks: the transpiler's linker walks a preload file's static import records and dispatches PluginResolver::on_resolve for each, reaching run_on_resolve_plugins without going through either previously gated call site. Gate in the shared FFI wrapper instead so every onResolve caller (module loader and linker) is covered. The bunfig-preload test now gives plugin-2 a relative ./helper.ts import to exercise the linker path, and the virtual-module test registers a no-op onLoad so vm.plugin_runner is actually populated while the gate is evaluated.
The blanket is_in_preload gate regressed setups where an earlier preload supplies a loader that a later preload's imports need (e.g. preload-1 registers onLoad for the virt: namespace and preload-2 imports virt:foo). Only the preload entry file itself should bypass runtime plugin hooks; its own imports should still be served by whatever plugins are already registered. Track the resolved path of the preload entry being imported on the VM and compare the specifier against that in Bun__runVirtualModule and run_on_resolve_plugins. A new test has preload-2 importing through preload-1's onLoad to guard against the regression.
The VM box is raw-dealloc'd on worker teardown so field Drops never run; the Vec buffer retained after load_preloads' clear() would leak per terminated Worker with preloads.
d020b7a to
47bfecc
Compare
There was a problem hiding this comment.
All three of my earlier concerns (the ungated linker onResolve site, the vacuous build.module test, and the missing currently_loading_preload drop in VirtualMachine::destroy) are addressed, and I didn't find anything new this pass. Deferring to a human because the final design — skip hooks only for the preload entry file itself while letting its imports go through already-registered hooks — is a user-facing semantic choice for Bun.plugin that's worth a maintainer sign-off.
What was reviewed:
run_on_resolve_plugins/Bun__runVirtualModulegates match on the resolved entry path; namespaced specifiers correctly bypass the gate.- New
Vec<u8>field isptr::write-initialized, cleared by scopeguard on every exit, andmem::take-dropped indestroy()alongsidepreload. - Four subprocess tests cover bunfig +
--preload, cross-preloadvirt:imports, andbuild.modulewithplugin_runnerpopulated.
Extended reasoning...
Overview
Adds currently_loading_preload: Vec<u8> to VirtualMachine, set per-iteration in load_preloads to the resolved path of the preload entry being imported and cleared by scopeguard on exit. Bun__runVirtualModule (onLoad) and run_on_resolve_plugins (onResolve, the shared FFI wrapper reached by module-loader resolve, resolve_hook, and the linker) return early when the specifier equals that path in the default namespace, so a preload entry file loads with the default loader while anything it imports still routes through whatever hooks are already registered. The field is initialized via ptr::write in init and reclaimed via mem::take in destroy() next to the sibling preload Vec.
Security risks
None identified. The gate only narrows when user plugin callbacks run; it never widens access. Path comparison is exact byte equality against a value the runtime itself resolved — no user-controlled parsing.
Level of scrutiny
High. This is core module-loader dispatch on a hot path, and it changes observable Bun.plugin behavior. The PR went through three revisions in response to my inline feedback (fourth onResolve site in the linker, a test that never reached the new gate, and a per-Worker leak of the new Vec), and the approach was narrowed mid-PR from "suppress all hooks while is_in_preload" to "suppress hooks only for the entry file's own path." That narrowing is what makes the two negative-space tests (virt:shared-thing and build.module across preloads) meaningful, but it's also a design call — a maintainer should confirm the intended contract is "preload entry files are exempt, their imports are not."
Other factors
Tests are subprocess-based, hermetic, use tempDir/bunEnv, drain both pipes concurrently, and assert exact structured output rather than substrings. The mechgate evidence in the description shows the new tests fail on main and pass on the PR under ASAN. CI on the head commit is green aside from unrelated known-flaky tests. All prior inline threads on this PR are resolved.
This branch has not been deployed
1 participant
When multiple preload files each register runtime plugin hooks, the first preload's onLoad/onResolve intercepts the loading of subsequent preload files themselves.
Repro
(plugin-2.ts and plugin-3.ts are identical with the name changed.)
Cause
load_preloadsimports each preload entry sequentially through the normal module loader and waits on it before moving to the next. Once plugin-1's setup registers an onLoad (flippingvm.plugin_runnertoSome), the nextJSModuleLoader::importfor plugin-2.ts reachesBun__runVirtualModuleandrun_on_resolve_plugins, which match plugin-2.ts against plugin-1's filter and hand it to plugin-1's callback.Fix
Track the resolved path of the preload entry currently being imported on
vm.currently_loading_preload, set for each iteration of the preload loop and cleared on exit.Bun__runVirtualModule(onLoad) andrun_on_resolve_plugins(onResolve) bail out when the specifier equals that path, so the preload entry file itself loads with the default loader while anything it imports continues to use whatever hooks are already registered.That second property matters: a setup where preload-1 provides a
virt:loader and preload-2 doesimport v from "virt:shared-thing"keeps working, andbuild.module()virtual modules remain available across preloads. Both are covered by tests alongside the interception fix.After:
Fixes #9373
[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file