plugin: run onResolve/onLoad for imports whose query string contains a dot by robobun · Pull Request #37702 · oven-sh/bun

robobun · 2026-08-12T01:22:47Z

Repro

// main.ts
Bun.plugin({
  name: "repro",
  setup(build) {
    build.onLoad({ filter: /plain\.ts/ }, () => ({ contents: "export const source = 'plugin'", loader: "ts" }));
  },
});
const mod = await import(process.argv[2]);
console.log("source =", mod.source);

$ bun main.ts "$PWD/plain.ts?v=123"
source = plugin
$ bun main.ts "$PWD/plain.ts?v=123.456"
source = disk

With ?v=123.456 neither onResolve nor onLoad fires; the file silently loads through the native loader. Real-world trigger: OpenCode cache-busts plugin imports with ?mtime=${stat.mtimeMs}, and fractional mtimes disabled the plugin that supplies their JSX transform.

Cause

PluginRunner::could_be_plugin (src/bundler/transpiler.rs) is the cheap pre-filter that decides whether plugin hooks run at all. It looked at the byte after the last . of the full specifier, query string included. For /app/plain.ts?v=123.456 the last dot is inside the query and the next byte is a digit, so the pre-filter returned false and run_on_load_plugins / the resolve hook were skipped entirely. This matches the reporter's matrix exactly: hooks are skipped iff the text after the last . of the whole specifier starts with a non-letter.

Fix

Slice the specifier at the first ? and run the existing extension check on the path portion only. This mirrors normalizeSpecifier in the loader path, which unconditionally splits the query off at the first ? before picking a loader, so the pre-filter now agrees with what the loader will actually do. No extra cases: the function is the same single check as before, applied to the pre-query slice.

Related but distinct from #36592, which accepts digit-leading extensions (no query involved); that change alone would still miss ?v=123. and ?v=1.., where the pseudo-extension is empty.

Verification

New test in test/js/bun/plugin/plugins.test.ts covers ?v=123 (control), ?v=123.456, ?v=.456, ?v=123., ?v=1.2.3, and ?mtime=1786494961337.0317, asserting both onResolve and onLoad fire. It fails on main and passes with this change. Full plugins.test.ts (39 tests) and bundler_plugin.test.ts (53 tests) pass.

Earlier revision

The first version kept a second extension check on the full specifier as a fallback for paths containing a literal ?. Dropped after review: the loader's normalizeSpecifier already treats everything after the first ? as a query regardless, so the fallback defended a case the loader does not support anyway.

Dropping the fallback intentionally narrows one accidental case: an extensionless specifier whose query contains a dot followed by a letter (e.g. ./LICENSE?v=1.a) previously slipped through the pre-filter because the query was misread as an extension. It now behaves like its no-query form, which was always skipped.

…gin pre-filter PluginRunner::could_be_plugin judged the extension from the last '.' of the full specifier, query string included. For an import like "/app/plain.ts?v=123.456" the last dot is inside the query, the byte after it is a digit, and the pre-filter returned false, so runtime onResolve/onLoad hooks were silently skipped and the file loaded through the native loader. Split the specifier at the first '?' (matching normalizeSpecifier in the loader path) before checking the extension, keeping the full-specifier check as a fallback for paths that contain a literal '?'. Fixes #37699

robobun · 2026-08-12T01:22:57Z

^{Updated 8:29 PM PT - Aug 12th, 2026}

❌ @robobun, your commit e54b85c has 2 failures in Build #93986 (All Failures):

step failed outside runner - exit 1 on 🍎 aarch64 - build-bun
step failed outside runner - exit 1 on 🪟 x64 - build-bun
step failed outside runner - exit 1 on 🍎 x64 - build-bun
scripts/build/ci.ts - build failed on 🍎 aarch64 - build-cpp
scripts/build/ci.ts - build failed on 🍎 x64 - build-cpp
scripts/build/ci.ts - build failed on 🪟 x64 - build-cpp

🧪 To try this PR locally:

bunx bun-pr 37702

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

bun-37702 --bun

coderabbitai · 2026-08-12T01:24:58Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1cf08e5e-5f46-4fa7-a273-244951384f36

📥 Commits

Reviewing files that changed from the base of the PR and between 6d3b79c and ad57bb4.

📒 Files selected for processing (2)

src/bundler/transpiler.rs
test/js/bun/plugin/plugins.test.ts

Walkthrough

The change updates plugin detection to evaluate file extensions before query strings. It extracts the extension check into a helper and adds a concurrent regression test for .ts imports with dotted query values.

Changes

Plugin query handling

Layer / File(s)	Summary
Extension detection and regression coverage `src/bundler/transpiler.rs`, `test/js/bun/plugin/plugins.test.ts`	`PluginRunner::could_be_plugin` checks the specifier before its query component and reuses the extracted extension helper. The regression test verifies plugin resolution and loading for multiple `.ts` imports with dotted query strings.

Possibly related issues

bun test --isolate drops the plugin onResolve namespace for query-suffixed imports #33904 — Addresses related plugin handling for query-suffixed imports and extension or namespace resolution.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The implementation and tests address issue `#37699` by preserving plugin processing for dotted query-string variants.
Out of Scope Changes check	✅ Passed	The changes are limited to the plugin pre-filter fix and its targeted regression test.
Title check	✅ Passed	The title clearly identifies the plugin hook behavior fixed for imports with dotted query strings.
Description check	✅ Passed	The description explains the problem, root cause, fix, regression coverage, and verification results, despite using different section headings than the template.

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

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/plugin/plugins.test.ts`:
- Line 832: Update the command arguments in the affected test to pass the
fixture’s absolute entry-point path instead of the relative "entry.ts" string,
while preserving the existing bunExe() invocation and test behavior.
- Around line 811-847: Expand the plugin test around the existing query loop to
register an onResolve callback and verify it runs for every query variant while
preserving the plugin-provided module results. Add a compiled-executable
execution path for the same entry point and assert all query variants still
resolve to the plugin contents, covering both onResolve and compiled execution.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 91674896-a9d4-41c1-ae0d-6b8bbcd7d638

📥 Commits

Reviewing files that changed from the base of the PR and between e7abdf7 and 6d3b79c.

📒 Files selected for processing (2)

src/bundler/transpiler.rs
test/js/bun/plugin/plugins.test.ts

claude

LGTM — the pre-filter is now strictly more permissive, so nothing that passed before can start being skipped.

What was reviewed:

Confirmed the fallback keeps could_be_plugin monotonic: every specifier that returned true before still does (the full-specifier check is preserved), so this cannot regress existing plugin routing.
Traced all call sites (jsc_hooks.rs, linker.rs, ModuleLoader.rs, VirtualMachine.rs) — they all funnel through this one helper via re-exports, so the fix is at the right layer.
Checked slice bounds in starts_with_letter_after_last_dot: last_dot + 1 is at most len, and the empty-ext case is guarded.
Test follows the neighboring subprocess pattern exactly, drains pipes concurrently, asserts stdout before exitCode, and covers onResolve + onLoad across six query variants including the control.

Extended reasoning...

Overview

Two files changed. src/bundler/transpiler.rs refactors PluginRunner::could_be_plugin to split the specifier at the first ? before running the last-dot extension heuristic, extracting the heuristic itself into a private starts_with_letter_after_last_dot helper. The full-specifier check is retained as a fallback so the pre-filter only becomes more permissive. test/js/bun/plugin/plugins.test.ts adds a subprocess-based regression test covering six query-string variants for issue #37699.

Security risks

None. This is a byte-level heuristic on a module specifier that only decides whether to run plugin hooks (which the caller already registered). No untrusted input reaches allocation sizes, syscalls, or path resolution here; the actual query-stripping in the loader path (normalizeSpecifier) is unchanged.

Level of scrutiny

Low-to-medium. The diff is ~20 net lines of pure byte-slice logic with no allocation, no unsafe, no FFI, and no state. The key correctness property — that the change is strictly more permissive — is easy to verify by inspection: the original starts_with_letter_after_last_dot(full_specifier) check remains as the second branch, so any input that previously returned true still does. The only new behavior is returning true for additional inputs where the pre-? slice has a letter-leading extension, which is exactly the intended fix. All call sites route through this single function via re-exports.

Other factors

The test is well-constructed per REVIEW.md: it uses tempDir/bunEnv/bunExe, it.concurrent, drains all three subprocess pipes concurrently, asserts the JSON payload before exitCode, and includes a control case (?v=123) alongside the failing variants so it can't vacuously pass. All prior review threads (CodeRabbit's onResolve coverage request, comment-cop's verbose-comment flags, my own as usize nit) are resolved. The remaining identity cast matches the pre-existing pattern at line 73 and unnecessary_cast is workspace-allowed, so it's not a blocker.

claude

LGTM — the pre-filter is now strictly more permissive (the old full-specifier check is retained as a fallback), so nothing that passed before can regress, and the new test covers both onResolve and onLoad across the dotted-query variants. The one inline note is pre-existing and not a blocker.

Extended reasoning...

Overview

This PR fixes #37699: PluginRunner::could_be_plugin in src/bundler/transpiler.rs was judging the file extension by the last . in the entire specifier, so a query string like ?v=123.456 produced a pseudo-extension of 456 (digit-leading) and the plugin pre-filter returned false, silently bypassing onResolve/onLoad. The fix splits at the first ? and applies the extension heuristic to the path portion, keeping the original full-specifier check as a fallback. The extension check itself is extracted unchanged into starts_with_letter_after_last_dot. A new subprocess test in test/js/bun/plugin/plugins.test.ts covers six query variants and asserts both onResolve and onLoad fire.

Security risks

None. This is a byte-level heuristic on an import specifier that only decides whether to consult registered plugin hooks; the actual filter regex still gates whether a hook runs. The change is strictly widening (the fallback path is byte-identical to the old code), so no previously-rejected specifier is now rejected, and no new user input reaches a sink.

Level of scrutiny

Low-to-medium. The function is a cheap pre-filter with three call sites (jsc_hooks.rs, ModuleLoader.rs, VirtualMachine.rs) that all fall through to the normal loader when it returns false. Because the new code only adds a return true path ahead of the unchanged old logic, the worst possible regression is that plugin hooks are consulted for a specifier they weren't before — and if no filter matches, behavior is identical to today. The refactor into a helper is mechanical; last_index_of_char and slice indexing are unchanged from the pre-PR body.

Other factors

The PR includes evidence the test fails on main (both ASAN debug and release) and passes with the fix.
All prior review threads (coderabbit onResolve coverage, comment-cop, my earlier as usize nit) are resolved; the author added onResolve assertions in 53a8c62 and trimmed comments in bf66a4e.
The one inline finding this run — the A/Z off-by-one in extract_namespace's Windows drive-letter guard — is pre-existing (aef109a), untouched by this PR, and flagged only as a well-placed drive-by opportunity.
Test follows harness conventions: it.concurrent, tempDir, bunEnv, concurrent pipe drain, stdout asserted before exitCode.

alii · 2026-08-13T02:06:38Z

Seems oddly specific - is there special casing we could actually just remove here instead and not have to wire up extra handling?

github-actions · 2026-08-13T03:12:20Z

+        // The loader ignores everything from the first '?' (`normalizeSpecifier`),
+        // so judge "/a/b.ts?v=1.2" by ".ts", not ".2".


If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

claude · 2026-08-13T03:26:28Z

+        // The loader ignores everything from the first '?' (`normalizeSpecifier`),
+        // so judge "/a/b.ts?v=1.2" by ".ts", not ".2".
+        let path = match bun_core::strings::index_of_char_usize(specifier, b'?') {
+            Some(query_start) => &specifier[..query_start as usize],
+            None => specifier,
+        };
+        if let Some(last_dot) = bun_core::strings::last_index_of_char(path, b'.') {
+            let ext = &path[last_dot + 1..];
+            // A letter or non-ascii byte; rules out "../", "..", and "./".


🟡 The PR description (both the top "Fix" bullet and the collapsed original description, plus the "root cause" footer) still says "The old whole-specifier check remains as a fallback, so the filter only gets more permissive: whatever reached plugins before still does" — but e54b85c dropped that fallback in response to @alii's simplification request, so the pre-filter now judges only the pre-? slice. That means e.g. /app/README?tag=v1.beta and ./LICENSE?v=1.a flip from true on main to false here. The narrowing looks intentional and just makes the query case match the no-query case, so this is only a description edit: drop the stale "fallback / only more permissive" sentences and note the narrowing so a future bisect doesn't misread it.

Extended reasoning...

What's stale

Commit e54b85c ("plugin: drop the full-specifier fallback, judge only the pre-query slice") removed the whole-specifier extension fallback that earlier iterations of this PR added. The current could_be_plugin (src/bundler/transpiler.rs:88-106) computes path as everything before the first ? and runs the last-dot check only on that slice; there is no second pass over the full specifier.

The PR description was not updated after that commit and still claims monotonicity in three places:

Top Fix bullet: "The old whole-specifier check remains as a fallback, so the filter only gets more permissive: whatever reached plugins before still does."

The collapsed Original description → Fix section: "The full-specifier check stays as a fallback so the pre-filter only gets more permissive: paths containing a literal ? that passed before still pass."

The root cause footer written by the author bot: "The original full-specifier check is retained as a fallback, making the pre-filter strictly mor…"

All three are now false.

Step-by-step proof of narrowing vs. main

Take specifier = b"/app/README?tag=v1.beta".

On main (old code, single pass over the full specifier):

last_index_of_char(specifier, b'.') → the dot before beta.

ext = b"beta"; ext[0] = b'b' → is_ascii_lowercase() → return true.

On this PR after e54b85c:

index_of_char_usize(specifier, b'?') → Some(11); path = b"/app/README".

last_index_of_char(path, b'.') → None (no dot in the pre-query slice).

Fall through to !is_absolute(specifier) && contains ':'. is_absolute(b"/app/README?tag=v1.beta") → true → return false.

Second example, specifier = b"./LICENSE?v=1.a":

On main: last dot is before a; ext = b"a", letter → true.

On this PR: path = b"./LICENSE"; last dot is at index 0; ext = b"/LICENSE"; ext[0] = b'/' → not a letter, not >127 → fall through. is_absolute → false; index_of_char_usize(specifier, b':') → None → false.

So the description's claim "whatever reached plugins before still does" is provably false for the class {extensionless-or-dot-relative pre-? slice} × {query containing .<letter>}.

Why this is a nit, not a code defect

The narrowing is intentional. The commit title literally states it, and it was made in direct response to a maintainer (@alii) asking on 2026-08-13 whether the extra handling could be removed rather than wired up. The change is recorded in git history — it's only the PR body that lags.

The narrowing makes behavior more consistent, not less. On main, /app/README (no query) already returns false and ./LICENSE (no query) already returns false. The old code only accidentally returned true when a query string happened to contain .<letter> — it was misreading a query fragment as a file extension, which is exactly the bug class this PR exists to fix. After e54b85c the query-carrying case matches the no-query case, which is the principled behavior since normalizeSpecifier strips the query before loader selection anyway.

The affected input class is vanishingly narrow (a plugin intercepting an extensionless import that carries a query containing a dot-then-letter), and any such plugin was already broken on main for the corresponding no-query import.

Impact

None on runtime correctness for the fix's target (the #37699 repro and all six test variants pass). The risk is purely archaeological: someone bisecting a future "my plugin stopped firing for ./LICENSE?tag=v1.beta" report will read this PR body, see "only gets more permissive", and rule it out incorrectly.

Fix

Edit the PR description only — no code change. Delete the three stale "fallback / only gets more permissive" sentences and, if desired, add a one-liner in the Fix section noting that the pre-filter now judges only the pre-? slice, so extensionless paths whose query happened to contain .<letter> no longer accidentally reach plugins (aligning them with the no-query case).

robobun · 2026-08-25T18:05:34Z

Heads-up from #40465. It routes every resolved absolute path to the file namespace onLoad filters in Bun__runVirtualModule, with no extension check. On that branch import("/abs/plain.ts?v=123.456") with onLoad({ filter: /plain\.ts/ }) already returns the plugin source (the query string stays in the module key, and the key is absolute). Once it lands, the onLoad half of the test in this PR passes on main. The onResolve half still needs the could_be_plugin change here, so the test keeps a fail-before only through its onResolve assertions.

robobun · 2026-09-13T08:03:47Z

A cross-reference: #35601 makes import(fileUrl + "?v=1.5") keep its query in the module key, the same as import("./x.ts?v=1.5") on main. So the file:// spelling then reaches this bug too. On main it ran the plugin hooks, because the query was dropped. This PR should land with #35601 or before it. This branch conflicts with main at the moment.

One more case with the same cause, seen with the repro above on main: a : in the query (./plain.ts?t=12:30) also loads from disk. PluginRunner::extract_namespace reads everything before the first : as the namespace, path and query included.

github-actions Bot added the claude label Aug 12, 2026

robobun mentioned this pull request Aug 12, 2026

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

Closed

coderabbitai Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread test/js/bun/plugin/plugins.test.ts

Comment thread test/js/bun/plugin/plugins.test.ts

test: also assert onResolve fires for every query variant

53a8c62

github-actions Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread src/bundler/transpiler.rs Outdated

github-actions Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread src/bundler/transpiler.rs Outdated

chore: trim comments in could_be_plugin

bf66a4e

github-actions Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread src/bundler/transpiler.rs Outdated

[autofix.ci] apply automated fixes

ad57bb4

claude Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread src/bundler/transpiler.rs Outdated

claude Bot reviewed Aug 12, 2026

View reviewed changes

ci: retrigger

87522f3

claude Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread src/bundler/transpiler.rs

plugin: drop the full-specifier fallback, judge only the pre-query slice

e54b85c

github-actions Bot reviewed Aug 13, 2026

View reviewed changes

claude Bot reviewed Aug 13, 2026

View reviewed changes

This was referenced Aug 17, 2026

fix(loader): keep a file:// URL's query in module keys #39426

Closed

resolver: keep ?query and #fragment on file:// URL import specifiers #35601

Open

robobun mentioned this pull request Aug 25, 2026

Run runtime plugin onLoad for a resolved file without an extension #40465

Open

robobun mentioned this pull request Sep 17, 2026

Run runtime plugin onResolve for bare and relative specifiers #40398

Open

		// The loader ignores everything from the first '?' (`normalizeSpecifier`),
		// so judge "/a/b.ts?v=1.2" by ".ts", not ".2".

Conversation

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repro

Cause

Fix

Verification

Uh oh!

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

alii commented Aug 13, 2026

Uh oh!

github-actions Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 13, 2026

Choose a reason for hiding this comment

What's stale

Step-by-step proof of narrowing vs. main

Why this is a nit, not a code defect

Impact

Fix

Uh oh!

robobun commented Aug 25, 2026

Uh oh!

robobun commented Sep 13, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Aug 12, 2026 •

edited

Loading

robobun commented Aug 12, 2026 •

edited

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading