Repository navigation
Conversation
WalkthroughChangesThe bundler plugin now validates resolver results through a shared async path and supports Namespace-prefixed resolution
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description fully explains the problem, cause, fix, compatibility behavior, and verification results. It uses different headings from the template but includes the required change summary and testing information. 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new issues found — the six earlier findings (result-namespace default, BunString leak on the empty-list early return, drive-letter guard, file: prefix double-dispatch, describe.concurrent, and the toBe ordering assertions) are all addressed in c395e86 and c6f6a25. Deferring because this changes user-facing plugin dispatch semantics: each context still prefers a different primary form (runtime→{namespace:"virt"} with stripped path, bundler→{filter:/^virt:/} with full specifier), and the test now locks that asymmetry in — worth a maintainer confirming that's the intended contract vs. unifying on one.
What was reviewed
BunPlugin::OnResolve::runfallback: checked both Rust callers normalize"file"→empty before the call, so!nsString.isEmpty()cannot re-runfileNamespacefor file-namespace imports.anyMatchesCrossThread:transferToWTFString()now precedes the empty-list return; the#if OS(WINDOWS)drive-letter skip requires a following separator, matchingextract_namespace.runOnResolvePlugins:userNamespacedefaults toinputNamespace;prefix !== "file"prevents re-running the default group forfile:specifiers.
Extended reasoning...
Overview
The PR makes runtime Bun.plugin and Bun.build each fall back to the other onResolve registration form when their primary lookup misses, so a plugin registered as either {filter: /^virt:/} or {filter: /.*/, namespace: "virt"} handles import "virt:x" in both contexts. It touches three dispatch sites — BunPlugin::OnResolve::run (runtime C++), BundlerPlugin::anyMatchesCrossThread (bundler C++ pre-filter), and runOnResolvePlugins (bundler builtin JS) — plus a new 4-case subprocess test file.
Security risks
None identified. The change is scoped to plugin callback dispatch; no new untrusted input parsing beyond splitting a specifier at :. The drive-letter guard is Windows-gated and requires a following separator, matching the existing extract_namespace. No path escaping, credential, or privilege surface is touched.
Level of scrutiny
High. This is a user-facing API-semantics change to the plugin system across both the runtime module loader and the bundler, touching C++ JSC bindings (exception scopes, BunString ownership) and a hot-path builtin. Two prior review rounds surfaced six issues (a result-namespace default divergence, a WTFStringImpl leak from a reordered early return, an over-broad drive-letter skip, a file:-prefix double-dispatch in the JS Map, serial subprocess tests, and vacuous toContain assertions); all are fixed in c395e86/c6f6a255 and I re-verified each against the current diff. I also traced both Rust callers (PluginRunner::on_resolve, plugin_runner_on_resolve_jsc) to confirm they normalize "file"→empty before Bun__runOnResolvePlugins, so the new C++ fallback's !nsString.isEmpty() gate cannot re-run fileNamespace for file-namespace imports.
Other factors
The remaining reason to defer is design, not correctness: the fix preserves each context's existing primary lookup (runtime prefers the namespace-keyed group with a stripped path; bundler prefers the file-namespace group with the full specifier) and adds the other as a fallback. That means a plugin registering both forms still sees different args.path shapes in each context, and the "both forms" test now pins runtime === VIA-B / bundled === VIA-A. That's a defensible backward-compat choice, but it's a contract a maintainer should explicitly sign off on rather than have it locked in by a bot approval.
|
CI status: the diff itself is green. The new Rebased onto main at 9ecb464. One trivial conflict in 51e3a31: Latest build 106428 has two non-flaky failures, both also present in 106412, both pre-existing on main and unrelated to this change:
Everything else in 106428 is marked flaky. 106412 also had Ready for review. |
There was a problem hiding this comment.
All six issues from the earlier review rounds are addressed in c395e86 and c6f6a25 — I have nothing further to flag. Deferring to a human because this is a user-facing plugin-dispatch semantics change coordinated across three separate codepaths (runtime C++, bundler C++ pre-filter, bundler JS builtins), and the deliberate choice to keep different primary orderings per context (runtime prefers the namespace group, Bun.build prefers the file group) is worth a maintainer sign-off.
What was reviewed: the tryNamespace refactor preserves the original loop's result handling and userNamespace default; transferToWTFString() now runs before every early return in anyMatchesCrossThread; the drive-letter skip is Windows-gated and requires a following separator in both the JS and C++ fallbacks; the prefix !== "file" guard prevents re-running the default group on file: specifiers; the both-forms test now asserts exact primary-wins ordering.
Extended reasoning...
Overview
The PR makes onResolve dispatch consistent between the runtime module loader and Bun.build for ns:path-style specifiers by having each side fall back to the other registration form when its primary lookup produces no match. It touches BunPlugin::OnResolve::run (runtime C++, refactored into a shared runOnResolveGroup helper plus a file-namespace fallback), BundlerPlugin::anyMatchesCrossThread (bundler C++ pre-filter, refactored to hoist transferToWTFString and add a prefix-namespace fallback), and runOnResolvePlugins in BundlerPlugin.ts (bundler JS dispatch, refactored into a tryNamespace closure plus a prefix-namespace fallback). A new 4-case subprocess test covers form-A-only, form-B-only, both-forms, and { path }-without-namespace.
Security risks
None identified. The change only affects which registered plugin callbacks are consulted for a given specifier; no untrusted-input parsing, no filesystem/network effects, no auth or crypto surface.
Level of scrutiny
Moderate-to-high. This is not a mechanical fix — it changes plugin API semantics in a way that affects every ns:-style import handled by a plugin, and it does so across three independently-maintained implementations that must agree. The refactor in BunPlugin.cpp also changes how the path argument is materialized (jsString(vm, pathString) instead of Bun::toJS(globalObject, *path)), which I traced as behavior-preserving. The intentional asymmetry (runtime primary = namespace group, bundler primary = file group) is sound for backward-compat but is a design call a maintainer should confirm.
Other factors
Two prior review rounds surfaced a result-namespace default regression, a WTFStringImpl leak on the empty-list early return, an over-broad drive-letter skip, a file:-prefix double-dispatch, a serial-test nit, and a weak both-forms assertion — all fixed and verified in the current diff. The bug-hunting pass on the latest revision found nothing new. Tests are hermetic (tempDir + Bun.spawn), concurrent, and assert stderr/exit code plus exact per-form outputs. Given the scope and the API-surface nature of the change, I'm deferring rather than approving.
|
This also fixes the reported repro in #40579 (a full-specifier filter like /^virtual:thing$/ at runtime). Verified locally together with #40398. Note: the branch now conflicts with main in src/jsc/bindings/BunPlugin.cpp because #40404 made the BunString parameters const. The resolution is mechanical: add const to the new runOnResolveGroup and OnResolve::run signatures. |
…ifiers
For an import like "virt:x", Bun.plugin at runtime would only consult
onResolve callbacks registered with { namespace: "virt" } (passing the
stripped path "x"), while Bun.build would only consult callbacks
registered with { filter: /^virt:/ } in the default namespace (passing
the full "virt:x"). A plugin written one way worked in one context and
failed in the other.
The runtime now falls back to the default-namespace group with the full
specifier when the prefix-namespace group has no match. Bun.build's
onResolve pre-filter and dispatch now also consult the prefix namespace
with the stripped path when the importer-namespace group has no match.
In each context the previous primary lookup runs first, so existing
plugins (including those that register both forms as a workaround) keep
their current behavior.
…trings before early return, Windows-only drive-letter skip, concurrent tests
…orms test to assert primary ordering
0c25495 to
9ecb464
Compare
|
Updated 4:22 PM PT - Aug 26th, 2026
❌ @robobun, your commit 51e3a31 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36402That installs a local version of the PR into your bun-36402 --bun |
…synchronous fast path
There was a problem hiding this comment.
Re-reviewed after the latest push — all seven earlier inline points are now addressed in the code, and this pass found no further bugs. Since this changes user-facing plugin dispatch semantics (which onResolve registration form fires for ns:... specifiers, and in what order), a maintainer sign-off on the behavior itself is still worthwhile.
What was reviewed:
tryNamespacenow peeks the returned promise before awaiting, so the synchronousonResolveAsyncfast path is preserved end-to-end (traced through lines 487-518).transferToWTFString()is hoisted above the empty-list early return inanyMatchesCrossThread, so the caller'sBunStringallocations are consumed on every path.userNamespacedefaults toinputNamespace(notmatchNamespace), theprefix !== "file"guard prevents double-dispatch onfile:specifiers, and the drive-letter skip is now Windows-gated with a following-separator check in both the JS and C++ sides.- Tests use
describe.concurrent, assert exactVIA-A/VIA-Bper context, and cover the{ path }-without-namespace → file-on-disk case in both runtime andBun.build.
Extended reasoning...
Overview
This PR makes onResolve plugin dispatch symmetric between the runtime module loader (Bun.plugin) and the bundler (Bun.build) for specifiers of the form "ns:path". It touches three native/builtin dispatch sites — src/js/builtins/BundlerPlugin.ts (runOnResolvePlugins), src/jsc/bindings/JSBundlerPlugin.cpp (anyMatchesCrossThread), and src/jsc/bindings/BunPlugin.cpp (OnResolve::run) — each adding a fallback that consults the "other" registration form when the primary lookup produces no match. A new test file exercises form A only, form B only, both forms together, and the { path }-without-namespace case, in both runtime and Bun.build contexts.
Since my earlier inline reviews, commits have landed addressing every point: the result-namespace default is back to inputNamespace (with a dedicated test proving runtime/bundler agree when the callback omits namespace); the BunString::transferToWTFString() calls are hoisted above the empty-list early return so the leak on the "no plugins of this hook type" path is gone; the drive-letter guard is now process.platform === "win32" / #if OS(WINDOWS) and additionally requires a following / or \, matching the runtime's extract_namespace; the prefix !== "file" guard prevents re-running the default-namespace group on file: URLs; the tests use describe.concurrent and assert exact toBe("VIA-B") / toBe("VIA-A") per context; and the most recent commit restores the synchronous fast path by peeking $peekPromiseStatus/$peekPromiseSettledValue on the tryNamespace result before falling back to await.
Security risks
None identified. The change is dispatch routing inside plugin resolution — no new untrusted-input parsing beyond String.indexOf(":") / slice, no allocation sized from external data, and the C++ additions maintain DECLARE_THROW_SCOPE / RETURN_IF_EXCEPTION / RELEASE_AND_RETURN discipline around the extracted runOnResolveGroup helper. The jsString(vm, pathString) replacing Bun::toJS(globalObject, *path) is a like-for-like substitution now that pathString is a WTF::String local (and jsString doesn't throw for a valid WTF::String, so the removed RETURN_IF_EXCEPTION there is correct).
Level of scrutiny
Medium-high. The implementation is small and every earlier concern I raised has a corresponding code change plus test coverage, so I'm satisfied on correctness. What keeps this from a straight approve is that it's a user-facing API semantics change: after this PR, an onResolve({ namespace: "virt" }) callback that previously never fired in Bun.build will now fire (with a stripped path), and an onResolve({ filter: /^virt:/ }) callback that never fired at runtime will now fire (with the full specifier). The primary-then-fallback ordering preserves existing behavior for plugins that register both forms, but the design choice — whether to add this fallback at all, and whether the runtime→file-namespace fallback in BunPlugin.cpp should reconstruct "ns:path" vs. something else — is the kind of thing a maintainer should explicitly ack per the API-design section of .claude/docs/landing-prs.md.
Other factors
The PR's evidence block shows the new tests fail on main (both debug+ASAN and release) and pass with the fix, and the PR description lists the existing plugin suites as passing unchanged. The synchronous-throw path (tryNamespace rejects with status 2) now takes one await before reaching the outer .then(..., e => addError) handler — but pre-PR that error also went through the .then rejection handler (the outer peek only unwraps status 1), so the observable error reporting is unchanged; only the happy path's zero-await property mattered, and that's preserved. No CODEOWNERS-gated paths are apparent for these files, and there are no outstanding third-party CHANGES_REQUESTED reviews in the timeline.
This branch has not been deployed
2 participants
What
For an import like
import "virt:x", the runtime module loader andBun.buildconsulted disjointonResolveregistrations:import "virt:x"Bun.buildonResolve({ filter: /^virt:/ })args.path = "virt:x"onResolve({ filter: /.*/, namespace: "virt" })args.path = "x"So a plugin written for esbuild (
filter: /^virt:/) bundled fine and failed at runtime withCannot find package 'virt:x'; one written against the runtime's namespace parsing ran fine and failedBun.buildwithCould not resolve: "virt:x". The only workaround was to register both forms.Repro:
Cause
BunPlugin::OnResolve::runlooks up the group for the parsed prefix namespace and runs only that group. If nothing is registered under that namespace it returns early; the default-namespace (file) group is never consulted forns:...specifiers.anyMatchesCrossThread/runOnResolvePluginskey the lookup on the importer's namespace (alwaysfilefor a first-level import) and test filters against the full specifier. A callback registered undernamespace: "virt"is never reached.Fix
Make each side fall back to the other form when its primary lookup produces no match:
BunPlugin::OnResolve::run(runtime): after trying the prefix-namespace group, also try the default-namespace group with the reconstructed full specifierns:path.BundlerPlugin::anyMatchesCrossThread(bundler pre-filter, onResolve only): when the importer namespace isfile/empty and the specifier has ans:prefix, also test thensgroup against the stripped path.runOnResolvePlugins(bundler JS dispatch): after the importer-namespace pass, try the prefix-namespace group with the stripped path.The previous primary lookup runs first in each context, so plugins that register both forms (the current workaround) keep their existing behavior. A single-letter
X:prefix is treated as a drive letter and skipped, consistent with the existing checks.Testing
test/js/bun/plugin/plugin-onresolve-namespace-prefix.test.tsspawns a subprocess per case (form A only / form B only / both) and asserts that both the runtime import andBun.buildresolve through the plugin, with the expectedargs.pathin each form.Existing plugin suites (
test/js/bun/plugin/plugins.test.ts,test/bundler/bundler_plugin.test.ts,test/bundler/bundler_plugin_chain.test.ts,test/bundler/bun-build-api.test.ts) pass unchanged.Fixes #9863
[review] gate passed · iteration 6 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 0 rejected · iteration 6
evidence per changed file