Skip to content

Run runtime plugin onResolve for bare and relative specifiers - #40398

Open
robobun wants to merge 5 commits into
mainfrom
farm/6c969155/plugin-onresolve-bare-specifier
Open

robobun wants to merge 5 commits into
mainfrom
farm/6c969155/plugin-onresolve-bare-specifier

Conversation

@robobun

@robobun robobun commented Aug 25, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • resolve_maybe_needs_trailing_slash also runs the hook for a bare or relative specifier that user code imports. Four cases keep the old pre-filter: no referrer, a builtin name, inside an onResolve callback, inside require.resolve(id, { paths }).
  • The linker hook and the onLoad pre-filter are unchanged. A static import reaches the hook through the loader.
  • A path equal to the specifier claims nothing for such a specifier, and goes through the resolver from the importer for any other. A relative or bare path for such a specifier does the same. The resolver runs without hooks and without auto-install. If nothing is found, the string stays the module key.
  • Verified: test/js/bun/plugin/plugins.test.ts (stock bun fails 6 tests), on Linux and Windows. Also the plugin, resolve, mock-module, isolation and hot suites.

Background

  • Bun.plugin() registers a runtime plugin. onResolve maps a specifier to a path. onLoad supplies the source for a path.
  • The hook has two call sites: the linker (at transpile time) and resolve_maybe_needs_trailing_slash (every resolve at run time).
  • A module key is the string that the loader fetches. The loader resolves the key of an import() once more, with no referrer.
Notes

Why each rule exists. Each one comes from a case that works on stock bun and failed with a wider pre-filter. I found them with probes of real plugin shapes against a stock release build.

Rule What broke without it
onLoad pre-filter unchanged Under bun test, Bun__runVirtualModule runs before the builtin lookup. require("ws") reached a catch-all onLoad({ filter: /.*/ }) with path: "ws": ENOENT: no such file or directory, open 'ws'.
Linker unchanged The linker calls the hook when a file is transpiled. For a literal require("optional-dep") inside try/catch, a hook that throws for a missing name made the whole module fail to load. The hook result also went into the on-disk transpiler cache, so a later run with no plugin loaded the redirected module. A hook that loads a module at that time panics (import_record_index < import_records.len()). Stock has all three for a specifier with an extension.
No referrer The loader resolves the key of an import() once more. A hook my-pkg to my-pkg/dist ran again on its own result: Cannot find module 'my-pkg/dist/dist'.
Builtin names A static import of fs never reaches the hook, because the transpiler resolves it. require("fs") did reach it, so a hook could claim a builtin for one import kind only.
Inside a callback const x = require("helper-pkg") inside a catch-all callback called the hook again: RangeError: Maximum call stack size exceeded.
require.resolve(id, { paths }) paths is resolver state (custom_dir_paths). A nested resolve inside the callback used and cleared it. Debug builds stop at debug_assert!(custom_dir_paths.is_none()) (BunObject.rs:1246).
Unchanged path claims nothing onResolve({ filter: /.*/ }, args => ({ path: args.path })) broke import pkg from "dep-pkg" (ENOENT reading "dep-pkg") and require("..") ("path" is invalid in onResolve plugin). For a specifier the hook already saw (dotted.pkg, ./config.local/index) stock bun has the same ENOENT. Such an unchanged result now goes through the resolver from the importer, and a miss keeps the verbatim key so a file-namespace onLoad keyed on it still serves it.
Absolute path with an extension is never newly hooked require(path.join(__dirname, "lib/foo")) under the no-op hook: ENOENT reading "/abs/lib/foo". An absolute result ends the resolve, so the resolver never added .js.
Relative or bare result goes through the resolver The raw string was the module key. import() found it from cwd through the second resolve. A static import and require() failed: ENOENT reading "./services/__mocks__/api".
Fallback to the module key, no auto-install A result can name a virtual module (build.module("my-shim"), mock.module) or a path that only a file-namespace onLoad serves. Without the fallback these stopped working. With auto-install, the resolver asked the registry for my-shim.
No directory re-read for a hook result _resolve busts the directory cache and retries after a miss. For a result that names a virtual module, every require() re-read the directory: 4 s for 2000 calls in a 300-file directory (stock: 3 ms), and one cache slot lost for each call.
Length guard The resolver aborts on a path that does not fit its path buffer (#42806 fixes that, stock bun aborts on require.resolve("/" + "a".repeat(4092)) with no plugin). A result goes to the resolver only when importer, result and an extension fit in MAX_PATH_BYTES. A longer result stays the module key.

Absolute result with no extension. For every specifier, an absolute path whose file name has no extension (/src/store) now goes through the resolver too, with the same fallback. The loader already does this for import() through its second resolve, and the linker path does it for a static import. require() failed with ENOENT reading "/src/store". Without this, a hook that maps @/store to path.join(root, "src/store") works for import() only.

What a plugin author can see

  • A hook whose filter matches a bare or relative specifier now runs for it. On stock bun such a hook was dead code for these specifiers. This is the purpose of the change, and it is the one behavior change that cannot be avoided.
  • A catch-all hook that resolves everything itself (Bun.resolveSync(args.path, ...)) now also decides export conditions for bare packages. The callback args carry no kind (namespace property is undefined in OnLoadArgs of Bun plugin #3894), so such a hook cannot pick require conditions. That was already true for every specifier with a dot.
  • Results are not offered to the hooks again, so two bare aliases do not chain.
  • A path equal to the specifier ends the hook chain, as every result does. Later hooks do not run for that import, and then normal resolution continues.
  • When a result names both a virtual module and a package that exists on disk, the package on disk wins for a bare or relative specifier. For a specifier with an extension and a result that differs from it, the virtual module wins, as before. An absolute extension-less result was completed from disk before too (stock: disk file wins, or Cannot find module without one).
  • require.resolve(id, { paths }) and builtin names do not reach the hook.

Cost. The new check runs only when a plugin is registered. Measured on the stock release build (1.4.3-canary, c6b7fcb, Linux x64, best of 7): 5000 modules, 10,000 static imports, one registered filter that never matches. With .js in each import (the hook runs today, twice for each import): 263 ms. With no extension (the hook is skipped today): 236 ms. With no plugin: 124 ms and 135 ms. So one hook call costs about 2 microseconds. After this PR an extension-less import calls the hook once, not twice, because the linker does not call it.

Relation to #42939. #42939 made the same pre-filter change as the first version of this PR. Its second fix (a file that onResolve creates is not found: Cannot find module '<path>' from '') is a separate resolver bug and is not part of this PR. It relates to #40585, #40587 and #40279. The extensionlessPackage test row comes from #42939, with a co-author credit on the commit.

Earlier work. c230fe1 (farm/b3b5d814/plugin-onresolve-bare-specifiers, offered in this thread) removes the pre-filter at both sites and resolves a relative result from the importer. This PR takes the second idea, with the fallback and without auto-install, and keeps the linker as it is for the reasons in the table.

Known and not changed here

Tests (all fixtures use --no-install, so a bare name that no plugin claims does not reach the npm registry when a test fails)

  • Fail on stock bun: "onResolve runs for a bare specifier without an extension", "onResolve can redirect a specifier to a real file in the file namespace" (rows extensionlessPackage, noExtensionResultRequire), "onResolve sees scoped, bare and extension-less relative specifiers", "a relative or bare onResolve result for a bare specifier resolves from the importer", "an onResolve callback can require and resolve modules itself".
  • Also fails on stock bun: "a catch-all onResolve that returns args.path unchanged is transparent" (rows requireDottedBare, requireDottedRelativeDirectory).
  • Pass on stock bun and guard the rules: "a file-namespace onLoad does not run for a bare builtin under bun test", "an unchanged onResolve result stays the module key when nothing is on disk".
  • Suites run on the debug build: test/js/bun/plugin/, test/js/bun/resolve/ (resolve, resolve-error, resolve-bad-parent, import-meta-resolve, import-meta, require, resolve-ts, import-query, build-error), test/js/bun/test/mock/mock-module.test.ts, test/cli/test/isolation.test.ts, test/cli/run/preload-test.test.js, test/js/node/module/node-module-module.test.js, test/js/node/v8/capture-stack-trace.test.js, test/bundler/bundler_plugin.test.ts, test/cli/hot/hot.test.ts, test/regression/issue/22199.test.ts, test/regression/issue/12548.test.ts.

[human-review] gate passed · iteration 1 · 3 files touched

fails on main (without fix)
ASAN without fix: 6 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/plugin/plugins.test.ts
bun test v1.4.3 (c6b7fcb5b)

test/js/bun/plugin/plugins.test.ts:
If bundling, conditions should include development or production. If not bundling, conditions or NODE_ENV should include development or production. See https://www.npmjs.com/package/esm-env for tips on setting conditions in popular bundlers and runtimes.
(pass) require > SSRs `<h1>Hello world!</h1>` with Svelte [1615.96ms]
(pass) require > beep:boop returns 42 [17.03ms]
(pass) require > object module works [11.11ms]
(pass) module > throws with require() [10.47ms]
(pass) module > async module works with async import [28.94ms]
(pass) module > sync module module works with require() [6.57ms]
(pass) module > sync module module works with require.resolve() [3.90ms]
(pass) module > sync module module works with import [107.30ms]
(pass) module > modules are overridable [31.21ms]
(pass) dynamic import > SSRs `<h1>Hello world!</h1>` with Svelte [11.51ms]
(pass) dynamic import > beep:boop returns 42 [9.22ms]
(pass) dynamic import > async:onLoad ret
... (truncated)

release without fix: 6 FAILED
bun test v1.4.3-canary.1 (c6b7fcb5b)

test/js/bun/plugin/plugins.test.ts:
If bundling, conditions should include development or production. If not bundling, conditions or NODE_ENV should include development or production. See https://www.npmjs.com/package/esm-env for tips on setting conditions in popular bundlers and runtimes.
(pass) require > SSRs `<h1>Hello world!</h1>` with Svelte [24.82ms]
(pass) require > beep:boop returns 42 [0.23ms]
(pass) require > object module works [0.13ms]
(pass) module > throws with require() [0.23ms]
(pass) module > async module works with async import [1.38ms]
(pass) module > sync module module works with require() [0.08ms]
(pass) module > sync module module works with require.resolve() [0.08ms]
(pass) module > sync module module works with import [0.10ms]
(pass) module > modules are overridable [0.25ms]
(pass) dynamic import > SSRs `<h1>Hello world!</h1>` with Svelte [0.15ms]
(pass) dynamic import > beep:boop returns 42 [0.07ms]
(pass) dynamic import > async:onLoad returns 42 [1.45ms]
(pass) dynamic import > async object loader returns 42 [1.26ms]
(pass) import statement > SSRs `<h1>Hello world!</h1>` with Svelte [2.42ms]
(pass) erro
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/plugin/plugins.test.ts
bun test v1.4.3 (c6b7fcb5b)

test/js/bun/plugin/plugins.test.ts:
If bundling, conditions should include development or production. If not bundling, conditions or NODE_ENV should include development or production. See https://www.npmjs.com/package/esm-env for tips on setting conditions in popular bundlers and runtimes.
(pass) require > SSRs `<h1>Hello world!</h1>` with Svelte [1714.45ms]
(pass) require > beep:boop returns 42 [16.65ms]
(pass) require > object module works [13.22ms]
(pass) module > throws with require() [13.68ms]
(pass) module > async module works with async import [37.18ms]
(pass) module > sync module module works with require() [9.44ms]
(pass) module > sync module module works with require.resolve() [115.31ms]
(pass) module > sync module module works with import [10.13ms]
(pass) module > modules are overridable [27.35ms]
(pass) dynamic import > SSRs `<h1>Hello world!</h1>` with Svelte [18.07ms]
(pass) dynamic import > beep:boop returns 42 [7.64ms]
(pass) dynamic import > async:onLoad re
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 844ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/7] gen generated_host_exports.rs
generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 242 extern-C blocks audited
[1/7] cargo bun_runtime → libbun_runtime.a
�[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m
   �[1m�[94m|�[0m
�[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl
   �[1m�[94m|�[0m                                              �[1m�[33m^^^^^^^^^^^^^�[0m
   �[1m�[94m|�[0m
   �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default
�[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name`
  �[1m�[94m--> �[0msrc/install/windows-shim/Cargo.toml:41:8
   �[1m�[94m|�[0m
�[1m�[94m41�[0m �[91m- �[0mname = �[91m"bun_shim_impl"�[0m
�[1m�[94m41�[0m �[92m+ �[0mname = �[92m"bun-shim-impl"�[0m
   �[1m�[94m|�[0m
�[1m�[33mwarning�[0m: `bun_shim_impl` (manifest) generated 1 warning
�[1m�[33mwarning�[0m�[1m: `feature(generic_const_exprs)` is not supported with
... (truncated)
diff hotspot
src/jsc/JSGlobalObject.rs          |   5 +-
 src/jsc/VirtualMachine.rs          | 144 +++++++++---
 test/js/bun/plugin/plugins.test.ts | 448 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 568 insertions(+), 29 deletions(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file
file                                reads  edits  tests
src/jsc/JSGlobalObject.rs               0      0     17
src/jsc/VirtualMachine.rs               7      9     17
test/js/bun/plugin/plugins.test.ts      3      4     17

could_be_plugin skipped any specifier with no file extension and no
namespace colon, so a bare specifier like host-package/subpath never
reached a runtime plugin's onResolve hook and resolution failed with
"Cannot find module". Let bare specifiers through the pre-filter.
Extension-less relative and absolute paths stay excluded.

Fixes #40397
@coderabbitai

coderabbitai Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 78b40551-f60e-48cd-ba7d-d1c8f32da82c

📥 Commits

Reviewing files that changed from the base of the PR and between b64b630 and 1b43422.

📒 Files selected for processing (3)
  • src/jsc/JSGlobalObject.rs
  • src/jsc/VirtualMachine.rs
  • test/js/bun/plugin/plugins.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.


Walkthrough

The resolver now tracks nested onResolve callbacks, supports skipped bare and relative specifiers, processes resolver-backed plugin paths, adjusts plugin-originated caching and retries, and preserves original specifiers when redirected resolution fails. Tests cover these cases across import and resolver APIs.

Changes

Plugin resolution

Layer / File(s) Summary
Track nested onResolve execution
src/jsc/VirtualMachine.rs, src/jsc/JSGlobalObject.rs
The VM tracks active onResolve callbacks. The depth is restored when host callbacks return errors.
Resolve plugin-originated paths
src/jsc/VirtualMachine.rs
Plugin results can resolve skipped bare or relative specifiers and extensionless absolute paths. Plugin-originated resolution changes cache and retry behavior, ignores no-op file claims, and falls back to the original specifier when redirected resolution fails.
Validate plugin resolution cases
test/js/bun/plugin/plugins.test.ts
Tests cover extensionless paths, dotted directories, bare and scoped packages, builtins, long paths, resolver APIs, importer-relative resolution, and nested callbacks.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 1b434

The resolver changes and regression coverage present no concrete merge-blocking risk.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary change: runtime plugin onResolve now handles bare and relative specifiers.
Description check ✅ Passed The description explains the problem, implementation, compatibility rules, testing scope, and verification results. It does not use the exact template headings, but it provides the required informatio…

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

Comment thread src/bundler/transpiler.rs Outdated
@robobun

robobun commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

I worked on #40397 in parallel and stopped when I found this PR. My branch is farm/b3b5d814/plugin-onresolve-bare-specifiers (commit c230fe1). It takes the same root cause further, in case you want to fold any of it in:

  • It removes the could_be_plugin gate at the two onResolve sites (VirtualMachine.rs and linker.rs) instead of widening it. The C++ hook already runs the registered filters, so the filter decides. With the widened gate, an extension-less relative import (import "./rel") still never reaches an onResolve filter.
  • A file-namespace result that is a relative path or a bare specifier ({ path: "./real.js" }, { path: "real-pkg" }) now goes through the resolver from the importer. Today that string becomes the module key as-is, so it only loads when cwd is the importer's directory. Repro: lib/entry.js registers onResolve({ filter: /^alias$/ }, () => ({ path: "./real.js" })) and imports alias. Run bun lib/entry.js from the parent directory. Result: Cannot find module './real.js' from ''. This is the docs' own onResolve example (images/ to ./public/images/), and aliasing a bare package to a local file hits it as soon as bare specifiers reach the plugin.

Two tests in test/js/bun/plugin/plugins.test.ts cover both: "onResolve is consulted for bare package specifiers" and "a relative or bare path returned in the file namespace is resolved from the importer". Both fail on stock bun and pass with the branch. The existing test/js/bun/plugin/ suite passes.

robobun added a commit that referenced this pull request Aug 25, 2026
…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.
@robobun

robobun commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up from #40465, which changes the same area. Two interactions:

  1. Under bun test, Bun::runVirtualModule runs before the builtin lookup (src/jsc/bindings/ModuleLoader.cpp, the isBunTest blocks). With this PR's tail (!specifier.is_empty() && specifier[0] != b'.'), could_be_plugin("ws") becomes true, so Bun__runVirtualModule routes the bare builtin keys without a colon (ws, bun, undici, node-fetch, abort-controller, ...) to the file namespace onLoad group. A catch-all onLoad({ filter: /.*/ }) in a test preload would then be called with path: "ws". Today these keys never reach the file group. Worth a test here if this lands first.

  2. Run runtime plugin onLoad for a resolved file without an extension #40465 removes the could_be_plugin and extract_namespace calls from Bun__runVirtualModule (it routes by registered namespace, then absolute path). After it lands, this PR's change no longer affects that site, and the Background bullet about the virtual-module fallback is stale. No textual conflict: the two PRs touch different files.

@robobun

robobun commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

This also fixes the bare-specifier half of #40579. Verified locally together with #36402: both registration shapes in that issue fire.

robobun and others added 2 commits September 17, 2026 04:40
The `could_be_plugin` pre-filter let a specifier reach the runtime
onResolve hook only with a `.ext` or a `namespace:` prefix. A bare name
(`pkg`, `pkg/sub`, `@scope/pkg`) or an extension-less relative path
(`./other`) never reached a filter that matches it.

`resolve_maybe_needs_trailing_slash` now also runs the hook for such a
specifier when user code imports it. Four cases keep the old
pre-filter, because each one broke code that works today:
- no referrer: the loader resolves each module key once more
- a builtin name (`fs`, `ws`), which a static import never shows to
  the hook
- inside an onResolve callback, where `require("pkg")` would call the
  hook again without end
- inside `require.resolve(id, { paths })`, whose paths are resolver
  state

The linker and the onLoad pre-filter are unchanged. A hook call at
link time runs outside the caller's try/catch, and its result goes
into the transpiler cache.

Results: for a newly hooked specifier, a `path` equal to the specifier
claims nothing, so a no-op hook stays transparent. A relative or bare
`path` for such a specifier, and an absolute `path` whose file name
has no extension for any specifier, now go through the resolver from
the same importer, without hooks, without auto-install and without a
directory cache bust. When the resolver finds nothing, or the path
cannot fit a path buffer, the string is the module key, as before.

This replaces the first version of this PR, which widened
`could_be_plugin` for all three call sites. The
`extensionlessPackage` test row comes from #42939.

Co-authored-by: Peter Steinberger <steipete@gmail.com>
@robobun

robobun commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

I reworked this PR. The first version widened could_be_plugin, and #42939 (opened 2026-09-16) makes the same change. A review of both, with probes of real plugin shapes against a stock release build, found that a wider pre-filter breaks code that works today:

  • Under bun test, require("ws") reaches a catch-all onLoad({ filter: /.*/ }) with path: "ws" (ENOENT ... open 'ws').
  • A no-op hook args => ({ path: args.path }) breaks import pkg from "dep-pkg" (ENOENT reading "dep-pkg") and require("..").
  • The linker calls the hook when a file is transpiled. try { require("optional-dep") } catch {} makes the module fail to load when a catch-all hook throws for a missing name, and the hook result goes into the on-disk transpiler cache.
  • require("helper-pkg") inside a catch-all callback calls the hook again without end.

The new version does not touch could_be_plugin, the linker, or the onLoad site. It runs the hook for bare and relative specifiers in resolve_maybe_needs_trailing_slash only, and it defines what a relative, bare or extension-less result means. The PR body has one table row for each rule, with the case that needs it. The earlier comment about c230fe1 is partly adopted: a relative or bare result resolves from the importer, with a fallback to the old behavior. The pre-filter is not removed at the linker, for the reasons in the table.

How to reproduce the original problem: bun --no-install entry.ts with the fixture of "onResolve runs for a bare specifier without an extension" fails on stock bun with Cannot find module 'host-package/subpath'.

Checked: bun bd test test/js/bun/plugin/plugins.test.ts passes (53 tests) on Linux and Windows, and stock bun fails 5 of them. The list of other suites is in the PR body.

The second fix in #42939 (a file that onResolve creates is not found) is a separate resolver bug and stays out of this PR. See my comment there.

Comment thread src/jsc/VirtualMachine.rs Outdated
Comment thread src/jsc/VirtualMachine.rs Outdated
Comment thread src/jsc/VirtualMachine.rs Outdated
Comment thread src/jsc/VirtualMachine.rs Outdated
Comment thread src/jsc/VirtualMachine.rs Outdated
Comment thread src/jsc/VirtualMachine.rs Outdated
Comment thread src/jsc/VirtualMachine.rs Outdated
@robobun robobun changed the title Run runtime plugin onResolve for bare specifiers without an extension Run runtime plugin onResolve for bare and relative specifiers Sep 17, 2026
@robobun

robobun commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:15 AM PT - Sep 17th, 2026

❌ @robobun, your commit debb291 has 1 failures in Build #116933 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 40398

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

bun-40398 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked two spots in this push and they hold up: the on_resolve_depth bracket in src/jsc/JSGlobalObject.rs decrements before the ?, so a throwing onResolve callback does not leave the recursion guard stuck; and the redirected / specifier_utf8 reassignment in resolve_maybe_needs_trailing_slash keeps the borrowed UTF-8 view inside the lifetime of redirected, which is declared in the enclosing scope.

Extended reasoning...

The bug hunt on this push hit its finding cap, so approval is not on the table and the inline comments already signal that a human should look. This note only records the two additional concerns I traced from the diff and ruled out — the re-entrancy counter's error-path balance in src/jsc/JSGlobalObject.rs and the bun_core::String::to_utf8() borrow lifetime across the plugin-redirect swap in src/jsc/VirtualMachine.rs — so the author does not have to re-derive them. It is informational and not a correctness guarantee for the rest of the change.

Comment thread src/jsc/VirtualMachine.rs
Comment thread src/jsc/VirtualMachine.rs Outdated
Comment thread src/jsc/VirtualMachine.rs
…cifier

A no-op hook (args => ({ path: args.path })) returned the specifier
verbatim as the module key for a specifier that passed the pre-filter,
so require("dotted.pkg") and require("./config.local/index") failed
with ENOENT. Such a result now goes through the resolver from the
importer. A miss keeps the verbatim key, so a file-namespace onLoad
keyed on the unchanged specifier still serves it.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Ready for review. All review threads are resolved. On the latest CI run (debb291) the only test that is red on every retry is test/js/bun/spawn/spawn.test.ts on x64-asan, which fails the same way on other open PRs and is reported as a break on main. The other failures passed on retry. The plugin and resolve suites pass locally on the debug build.

steipete added a commit to steipete/bun that referenced this pull request Oct 5, 2026
Admit bare aliases at runtime using the guards from oven-sh#44593 and
@robobun's oven-sh#40398. Keep onLoad filtering and nested resolution policy.
Use the resolver's existing cache-miss retry for files created by hooks.

Preserve the original extensionless and created-target regressions.
steipete added a commit to steipete/bun that referenced this pull request Oct 5, 2026
Carry caller intent across the Rust/C++ plugin boundary and consume the
synchronous dynamic-import marker before callbacks can reenter resolution.
Retain static, require, require.resolve and runtime resolver distinctions.

Use oven-sh#44593's bare-alias guards adapted from @robobun's oven-sh#40398. Preserve
original assertions and add static-import and nested-resolution coverage.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant