Repository navigation
Conversation
Bun__runVirtualModule skipped the onLoad filters whenever the resolved module key failed the could_be_plugin pre-filter, which accepts only a specifier with a .ext or a namespace colon. An absolute path such as /dir/LICENSE never reached the file-namespace filters and was parsed as JavaScript instead. Every absolute path is a real file, so run the filters for it. Keep the pre-filter for the other key forms so that bare builtin names stay out of the file namespace under bun test, where plugins run before the builtin lookup.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe runtime now detects registered ChangesRuntime onLoad routing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the problem, implementation, scope, edge cases, and verification results. It does not use the exact template headings, but it contains the required information and is substantially complete. Comment |
|
Status: reproduced with Tests: the five new cases in CI: the new tests pass on every lane. The one red test, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. The one-line gate change is narrowly scoped (only extension-less absolute paths newly reach run_on_load_plugins; every other specifier shape is unchanged), and the two subprocess tests cover the fix and the bun test builtin-screening concern. The inline note about extract_namespace's A:/Z: off-by-one is pre-existing and optional to fold in.
Extended reasoning...
Overview
This PR changes one guard in Bun__runVirtualModule (src/jsc/ModuleLoader.rs): the early-return that used to key solely on PluginRunner::could_be_plugin(specifier) now also lets any absolute path through. The rest is a 5-line comment and two new subprocess tests in test/js/bun/plugin/plugins.test.ts.
Behavioral delta
I traced the four specifier shapes through the old and new guard: absolute-with-extension (already proceeded via could_be_plugin's dot check → unchanged), absolute-without-extension (previously returned early, now proceeds → the fix), bare builtin like ws (not absolute, no dot/colon → still returns early), namespaced like node:fs (not absolute, has colon → still proceeds). Only the intended case changes. The function is already gated on plugin_runner.is_some(), so there is no cost when no plugin is registered, and files with extensions already ran the regex filters — so no new hot-path work for the common case. Confirmed bun_paths::is_absolute is the cfg-dispatched native check at src/paths/lib.rs:200.
Security risks
None. This routes more resolved-on-disk file paths to user-registered onLoad regex filters, which is the documented contract; it does not expose new data or bypass any check.
Level of scrutiny
Medium — module loading is load-bearing, but the diff is a single boolean disjunct with a clear truth-table, the PR description enumerates the other could_be_plugin call sites and leaves them alone, and both tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain, assert output before exit code). The second test explicitly guards the bun test builtin-lookup ordering concern the comment raises.
Other factors
The one inline finding is a pre-existing off-by-one in extract_namespace (strict >/< excludes drives A/Z/a/z on Windows). It is not introduced by this PR — extension-bearing paths on those drives already hit it, and extension-less ones simply move from one no-op path to another — so it is a fold-in suggestion rather than a blocker.
An absolute path is a file on disk, so it no longer goes through extract_namespace. That function read the first colon in a POSIX path such as /dir/a:b/x.js, or an A: or Z: drive letter on Windows, as a plugin namespace, and the file namespace filters never saw the path. Non-absolute keys keep the could_be_plugin pre-filter. Add a test for a directory name that contains a colon.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
A module key such as /virtual:foo.txt is both a namespaced virtual module and, by shape, an absolute path. Check the registered onLoad namespaces first, with the same rule moduleLoaderResolve uses (a single letter before the colon is a Windows drive). Only then treat an absolute path as a file. Bun__onLoadPluginsHasNamespace exposes the namespace lookup to Rust.
There was a problem hiding this comment.
I reviewed this again after 4c5a79a/457090b and found no further issues — both of my earlier concerns (the A:/Z: drive-letter gap and the is_absolute false positives on /ns:path and @:/path keys) are resolved by the registered-namespace-first rule, and I confirmed it mirrors moduleLoaderResolve at ZigGlobalObject.cpp:3411-3422 (same colon == 1 && isASCIIAlpha drive skip, same onLoadPlugins.namespaces lookup). Given this reworks the routing in Bun__runVirtualModule — every resolved module key passes through here when a plugin is registered — a maintainer sign-off on the disambiguation rule would still be worthwhile.
What was reviewed:
registered_on_load_namespacebounds:&specifier[namespace.len()+1..]cannot over-slice (colon index < len);colon==0and single-letter alpha prefixes short-circuit before the FFI call.Bun__onLoadPluginsHasNamespaceFFI:&BunString↔const BunString*,boolreturn;group("")returns&fileNamespacebut the Rust side never passes empty.- Fallback path preserved for non-absolute keys (
node:fs,ws,myns:foo) —could_be_plugin/extract_namespacestill gate builtins underbun test; test 4 pins that. - Single-letter alpha namespaces (
a:foo,a:/foo) traced on both platforms — the only behavior delta isa:/,z:/on Windows now treated as drives (matchingmoduleLoaderResolve) instead of reaching namespacea/zvia the pre-existing off-by-one.
Extended reasoning...
Overview
Fixes runtime Bun.plugin onLoad never firing for resolved files without an extension (and for POSIX paths containing a colon). The change reworks namespace routing in Bun__runVirtualModule (src/jsc/ModuleLoader.rs): first check whether the prefix before : is a registered onLoad namespace (new Bun__onLoadPluginsHasNamespace FFI in BunPlugin.cpp + has_on_load_namespace wrapper in JSGlobalObject.rs), then treat any absolute path as the file namespace, else fall through to the existing could_be_plugin/extract_namespace pre-filter. Four new subprocess tests in plugins.test.ts.
Security risks
None identified. The change only widens which resolved module keys reach user-registered onLoad filters; those filters are user code the user already opted into. No untrusted-input parsing beyond finding a : and checking is_ascii_alphabetic on one byte. The new FFI is a read-only predicate (group() != nullptr) with no allocation on the C++ side beyond toWTFString(ZeroCopy).
Level of scrutiny
High — Bun__runVirtualModule is on the module-load path for every import/require when any runtime plugin is registered, and the disambiguation between ns:path virtual keys and colon-containing filesystem paths is inherently heuristic. That said, the chosen rule is not new: it duplicates the logic already in moduleLoaderResolve (ZigGlobalObject.cpp:3411-3422), which I verified line-for-line (same colon == 1 && isASCIIAlpha(key[0]) drive-letter skip, same linear scan of onLoadPlugins.namespaces). The PR went through three revisions in response to my earlier reviews; the final shape addresses both the A:/Z: drive gap and the /ns:path / @:/path false-positive shapes I flagged.
Other factors
- I traced slice bounds in
registered_on_load_namespace:colonis a valid index intokey, so&key[..colon]and&specifier[namespace.len()+1..]are always in-bounds;colon==0is rejected so the namespace passed togroup()is never empty (which would otherwise hit&fileNamespaceand always return true). - The fallback branch is byte-identical to the pre-PR code, so
ws,node:fs, and unregisteredns:pathkeys behave exactly as before — test 4 pins thewscase underbun testwhere plugins run before builtins. - The FFI signature matches (
&JSGlobalObject/&BunString↔ non-null pointers,boolreturn);String::from_bytesborrows the slice for the call duration only. - The one behavior delta I could find vs. pre-PR: on Windows, a single-letter alphabetic namespace whose path starts with a separator (e.g.
a:/foowith namespacearegistered) previously reached theagroup via the strict-range off-by-one inextract_namespace, and now goes to the file namespace. This aligns withmoduleLoaderResolve's rule and the PR description calls it out; it is obscure enough not to block. - Tests follow harness conventions (
tempDir,bunEnv, concurrent subprocess drain, exit-code asserted last,skipIf(isWindows)for the colon-in-path case). robobun reports the four new tests pass on all lanes and three fail on stock bun.
Deferring rather than approving because this is a routing change in the module loader itself, and the registered-namespace-wins rule — while consistent with moduleLoaderResolve — is a design call a maintainer should ratify.
…ries A resolved key that has neither a registered namespace nor an absolute path is a builtin name or a data: key. No file exists for it, so the file namespace has nothing to load and the old extension heuristic is not needed. This also keeps bare builtin names out of the file namespace under bun test once could_be_plugin accepts them (#40398). The <cwd>/[eval] and <cwd>/[stdin] keys of bun -e and bun - are absolute by shape but have no file either. Stock bun ran the file namespace filters for them only when the cwd contained a dot.
…separator The single-letter check ran on every platform and without the separator requirement, so a key such as a:boop from a registered namespace "a" never reached its onLoad group once the extract_namespace fallback was gone. Mirror the guard extract_namespace uses: a drive path is a letter, a colon and a separator, on Windows only.
There was a problem hiding this comment.
I re-reviewed at b49d9bd and didn't find any new bugs. The three earlier findings (the A/Z drive-letter dependency, the is_absolute false-positive on /ns:.../@:/... keys, and the un-gated single-letter drive check) are each addressed — traced registered_on_load_namespace against the full case matrix (a:boop on both platforms, C:\\..., /virtual:..., /dir/a:b/..., node:fs, @:/id) and it routes correctly. Because this rewires the module-loader onLoad gate (and intentionally changes behavior for [eval]/[stdin] and non-absolute onResolve results), a human sign-off would still be worthwhile.
Extended reasoning...
Overview
Rewrites how Bun__runVirtualModule decides whether a resolved module key reaches runtime onLoad plugins. Drops the could_be_plugin + extract_namespace pre-filter (designed for raw specifiers) in favor of: (1) if the prefix before the first colon is a registered onLoad namespace, route there; (2) else if the key is an absolute path (and not the [eval]/[stdin] entry), route to the file namespace; (3) else skip. Adds Bun__onLoadPluginsHasNamespace (C++, a group() lookup — no JS entry, no throw), a Rust wrapper, and extracts is_eval_or_stdin_entry into a shared helper on VirtualMachine. Five subprocess tests cover extension-less files, colon-in-path, path-like namespaces, eval/stdin, and bun test builtin routing.
Security risks
None identified. No untrusted input reaches new allocation or side effects; the new FFI is a read-only vector scan.
Level of scrutiny
High. This is the module-loader hook that runs for every import when a runtime plugin is registered. The PR went through three review rounds here, each surfacing a real routing regression that was then fixed; the current logic is sound against every case I traced, but the space of resolved-key shapes is subtle (Windows drives, colon-in-dirname, namespace names starting with / or a single letter, eval/stdin sentinels).
Other factors
- All three of my prior inline findings are resolved:
is_windows_driveis nowcfg!(windows)-gated and requires a trailing separator, soa:boopreaches namespaceaon every platform whileC:\\fileon Windows still goes to the file namespace and a registeredCnamespace cannot capture real drive paths. group()returns&fileNamespacefor an empty string, butcolon == 0returnsNonefirst, so":foo"cannot match the file namespace via the registered-namespace path.is_drive_letteruses inclusive bounds (src/paths/resolve_path.rs:800), so the pre-existing A/Z off-by-one inextract_namespaceno longer matters here.- Acknowledged behavior changes beyond the headline fix:
[eval]/[stdin]never reach file-namespaceonLoad(previously did when cwd contained a dot), and a relative-path onResolve result without a namespace no longer reachesonLoad(already broken on main per the description). Both are defensible, but a maintainer should confirm intent. - CI's one failure (
test-http-should-accept-custom-certs-when-provided.ts, expired fixture cert) is unrelated.
This branch has not been deployed
1 participant
Fixes the
onLoadhalf of #4609 and #37699.Problem
Bun.pluginonLoadcallback never runs for a resolved file without an extension.await import("./LICENSE")with a matchingonLoad({ filter: /LICENSE$/ })fails witherror: Expected ";" but found "License". Bun parses the file as JavaScript. The same happens forrequire()and for a static import.Bun__runVirtualModule(src/jsc/ModuleLoader.rs:245). It gates the resolved module key withcould_be_plugin(src/bundler/transpiler.rs:88), a pre-filter written for raw import specifiers. It accepts only a.extor anamespace:colon, so/dir/LICENSEnever reaches the filters. An extension that starts with a digit (Bun plugin can't load files with number in the file extension #4609) or a query string with a dot (Runtime Bun.plugin onResolve/onLoad silently bypassed when import query string contains a dot followed by a non-letter (e.g. ?mtime=123.456) #37699) fails the same gate.extract_namespaceon the key. For a POSIX path with a colon in a directory name (/dir/a:b/note.txt) it returns/dir/aas the namespace, so thefilenamespace filters never see that file either.Fix
onLoadcallbacks, the key is a virtual module in that namespace. Otherwise, if the key is an absolute path, it is a file and goes to thefilenamespace filters as is. Any other key (node:fs,ws,data:) has no file to load and gets no callback./(/virtual:foo.txt), which only the registered set can tell apart from a file, and the in-memory<cwd>/[eval]and<cwd>/[stdin]entries ofbun -eandbun -, which are skipped.moduleLoaderResolve(src/jsc/bindings/ZigGlobalObject.cpp:3411) already applies the registered namespace rule to resolved keys.bun testthe hook runs before the builtin lookup (somock.module("fs", ...)can replace a builtin). A bare builtin name such aswshas no registered namespace and is not absolute, so it gets no callback. This no longer depends oncould_be_plugin, which Run runtime plugin onResolve for bare and relative specifiers #40398 widens to accept bare names.test/js/bun/plugin/plugins.test.ts(four fail on stock bun, one guards the registered namespace rule). Also all oftest/js/bun/plugin/,test/js/bun/test/mock/,test/bundler/bundler_plugin.test.ts, andtest/cli/run/run-eval.test.ts.Background
onLoad({ filter, namespace })callbacks.BunPlugin::OnLoad::run(src/jsc/bindings/BunPlugin.cpp) picks the group for the namespace and calls the first callback whose RegExp matches the path. An empty namespace meansfile. The newBun__onLoadPluginsHasNamespaceasks whether a group exists for a namespace.Bun__runVirtualModuleis the Rust entry the module loader calls for every resolved module key before it reads the file from disk. The key is an absolute path, anamespace:pathfromonResolve, or a builtin name.could_be_pluginandextract_namespacestill gate the twoonResolvesites (src/jsc/VirtualMachine.rs:4594,src/bundler/linker.rs:442), which see raw import specifiers. This PR does not change them. plugin: match onLoad filters for extensions starting with a digit #36592, plugin: run onResolve/onLoad for imports whose query string contains a dot #37702 and Run runtime plugin onResolve for bare and relative specifiers #40398 do.Notes
printf 'MIT License text' > LICENSE, then a script that registersonLoad({ filter: /LICENSE$/ })and runsawait import("./LICENSE"). Before:onLoadnever logs and the import rejects withExpected ";" but found "License". After:onLoadreceives the absolute path and the import evaluates to the file text.onLoad({ filter: /\.txt$/ })registered,import("./with:colon/note.txt")loads through the built-intextloader and the callback never runs. The first colon in the path was read as a namespace.onLoad({ filter: /\.1$/ })forok.yaml.1) returns the plugin value, stock returnsundefined. The Runtime Bun.plugin onResolve/onLoad silently bypassed when import query string contains a dot followed by a non-letter (e.g. ?mtime=123.456) #37699onLoadcase (import("/abs/plain.ts?v=123.456")withonLoad({ filter: /plain\.ts/ })) returns the plugin source, stock returns the disk source. TheironResolvehalves still need the resolve-side changes in plugin: match onLoad filters for extensions starting with a digit #36592 and plugin: run onResolve/onLoad for imports whose query string contains a dot #37702. After this lands, theonLoadtests in those two PRs pass on main, so they need a rescope toonResolve.[eval]and[stdin]: stock bun ran thefilenamespace filters for<cwd>/[eval]only when the cwd contained a dot (the text after the last dot looked like an extension). A plugin cannot decline a matchedonLoad, and there is no file at that path. This PR never runs them for these entries. Test 4 pins that, in a directory whose name contains a dot.Bun.buildalready runsonLoadfor the LICENSE and colon files. The runtime was the outlier.import(),require(), and a static import in a later-loaded module, and asserts thatargs.pathis the absolute path. The callback is synchronous becauserequire()rejects a promise result (existing, tested behavior).onLoadin the namespaces/virtualanda. The keys/virtual:foo.txtanda:boopread as an absolute path and as a drive path. The registered namespace wins. This passes on stock bun too and guards the rule.bun test --preloadwith the same plugin and a secondonLoad({ filter: /^ws$/ })in thefilenamespace. It asserts that the extension-less file goes throughonLoadand thatwsis still the builtin.extract_namespaceuses, but with every letter accepted.Z:\dir\fileis an absolute path and reaches thefilenamespace. Before,extract_namespacereturnedZfor it (a pre-existing strict range check at src/bundler/transpiler.rs:78). The guard is Windows only and needs the separator, so a single letter namespace (a:boop) keeps working throughrequire()on every platform.@:/id.txtwith a registered@namespace also reaches its group.onResolveresult without a namespace ({ path: "real.js" }) no longer reachesonLoad. That flow is already broken on main: the dynamic import yields an empty namespace, and a debug build of main asserts ingetModuleNamespaceon a laterrequire()of the same key, before and after this change.is_absolute || could_be_plugin) and still calledextract_namespace. The second sent every absolute path to thefilenamespace, which would have broken a registered namespace that starts with/. Review pointed at both, hence the registered namespace rule.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/plugin/plugins.test.ts