Repository navigation
Conversation
Node resolves each options.paths entry with path.resolve() before it
searches node_modules under it. Bun passed the raw entry to the
resolver, which has no DirInfo for a non-absolute directory, so
require.resolve("pkg", { paths: ["./lib"] }) threw MODULE_NOT_FOUND
while Node found <cwd>/lib/node_modules/pkg.
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: reproduced against node v26.3.0 with CI: the diff is green on every lane. The only red test on the second run is |
|
Updated 2:24 AM PT - Sep 6th, 2026
❌ @robobun, your commit 403d5a9 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41491That installs a local version of the PR into your bun-41491 --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.
LGTM — the follow-up commits addressed the earlier note by switching to the shared fs_ref().abs_buf_checked() helper, so both custom_dir_paths loops now normalize relative entries with the same platform::Loose semantics.
What was reviewed: the cwd-join in resolver.rs:1970-1993 — pooled buffer returns on Drop, None overflow skips the entry rather than panicking, absolute entries pass through unchanged; confirmed abs_buf_checked anchors on top_level_dir (updated by process.chdir); the new test restores process.cwd() in finally, asserts the exact MODULE_NOT_FOUND code, and stays serial (mutates cwd).
Extended reasoning...
Overview
The PR fixes require.resolve(id, { paths: [...] }) and Module._resolveFilename when a paths entry is relative. In src/resolver/resolver.rs, the custom_dir_paths package loop now checks bun_paths::is_absolute() on each entry and, for relative ones, joins against the cwd via self.fs_ref().abs_buf_checked() into a pooled bun_paths::path_buffer_pool buffer before calling check_package_path. Absolute entries are unchanged. A new test in test/js/node/module/module-resolve-filename-paths.test.js covers ./lib, lib, a fake parent module in an unrelated directory, and the MODULE_NOT_FOUND case for a non-existent relative entry.
Since the prior review, commits 9c59071 and 8012e2f replaced the hand-rolled join_abs_string_buf_checked::<platform::Auto> with the existing FileSystem::abs_buf_checked helper (platform::Loose, src/resolver/lib.rs:359), matching the sibling relative-specifier loop, and shortened the comment to a one-line why-note.
Security risks
None identified. This is a module-resolution path change scoped to the case where options.paths is explicitly provided and an entry is relative. Previously such entries were passed as-is to dir_info_cached, which returned nothing, so resolution always failed — this change moves from "never works" to "cwd-anchored like Node". No new filesystem writes, no untrusted-length arithmetic; overflow from abs_buf_checked yields None → continue, not a panic.
Level of scrutiny
Moderate. The resolver is hot-path code, but the change is gated behind custom_dir_paths.is_some() (already marked cold()) and only alters behavior for non-absolute entries that previously failed unconditionally. The implementation reuses the exact helper the neighboring relative-path loop already uses, so cross-platform separator/drive handling is consistent. The pooled buffer is scoped to one loop iteration and released via Drop. The PR gate evidence shows the new test fails on main and passes on the branch under both ASAN-debug and release.
Other factors
No CODEOWNERS entry covers src/resolver/ or the test file. The test follows the file's local createTempDir convention, restores process.cwd() in finally, asserts the specific error .code, and does not use test.concurrent (correct — it mutates process-global cwd). The earlier inline thread's concern is fully addressed by the current diff.
This branch has not been deployed
2 participants
Problem
require.resolve("pkg", { paths: ["./lib"] })throwsMODULE_NOT_FOUNDin Bun. Node resolves it to<cwd>/lib/node_modules/pkg/index.js. The same applies toModule._resolveFilename(..., { paths })andcreateRequire(...).resolve.pathsentry as-is tocheck_package_path(src/resolver/resolver.rs:1974).dir_info_cached(resolver.rs:4192) returns noDirInfofor a non-absolute path, so the entry is never searched. Node runspath.resolve(from)on each entry inModule._nodeModulePathsfirst.Fix
custom_dir_pathspackage loop, a non-absolute entry is joined against the cwd (top_level_dir) with host-native path semantics. The joined path lives in a pooled path buffer for the duration of that onecheck_package_pathcall../xwithpaths) already joins against the cwd viaabs_buf_checked, so it needs no change.process.chdir()updatestop_level_dir, so the anchor follows the current cwd like Node'sprocess.cwd().test/js/node/module/module-resolve-filename-paths.test.js(new test, passes undernode --testv26.3.0, fails on bun 1.4.3). Alsotest/js/node/module/node-module-module.test.jsandtest/js/bun/resolve/.Background
custom_dir_pathsis a per-call slot on the resolver.Bun__resolveSyncWithPaths(src/runtime/api/BunObject.rs) sets it fromoptions.pathsand clears it when the resolve returns. The resolver then searches only those directories, not the importer's ancestors.top_level_diron the resolver'sFileSystemis the process cwd.process.chdirrewrites it.Notes
Matrix checked against node v26.3.0 from a directory with
lib/node_modules/foo/index.jsandlib/bar.js. Entries./lib,lib,../<dir>/lib, absolute,.. Specifiersfoo,./bar,./bar.js,foo/index.js. Before the fix the six bare-name cells with a relative entry differed. After the fix all 20 cells match.When the cwd has no
package.jsonand thepathsentry does not exist, Bun falls through to auto-install from the global cache where Node throwsMODULE_NOT_FOUND. That is the existing auto-install extension and is unchanged here.test/js/bun/resolve/load-same-js-file-a-lot.test.ts("load the same empty JS file 2000 times") times out at 5 s in this debug container with and without this diff.[human-review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file
root cause · written by the author bot
When
require.resolve(id, { paths: [...] })orModule._resolveFilenamereceived a relativepathsentry, the resolver passed it straight todir_info_cached, which cannot locate a non-absolute directory, so every lookup through that entry failed withMODULE_NOT_FOUNDeven when the package existed. The fix makes thecustom_dir_pathsloop inresolver.rscheckbun_paths::is_absolute()and, for relative entries, join them against the current working directory via the existingFileSystem::abs_buf_checkedhelper into a pooled buffer before callingcheck_package_path, leaving abso…