Repository navigation
Conversation
require.resolve('x', { paths: [nonString] }) and the equivalent
Module._resolveFilename call were coercing each paths entry via
toWTFString, so objects became '[object Foo]' and reached the resolver
as a non-absolute directory, tripping the dir_info_cached assertion.
Relative string paths hit the same assertion.
Both callers now throw ERR_INVALID_ARG_TYPE for non-string entries to
match Node.js, and the resolver absolutizes custom search directories
against cwd before calling check_package_path so relative string paths
behave like Node instead of panicking.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 2:52 PM PT - Jul 22nd, 2026
@autofix-ci[bot], your commit 80a887d is building: |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #31566, which already covers this crash more thoroughly: it absolutizes with native-platform semantics (so |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| err = undefined; | ||
| try { | ||
| require.resolve("nonexistent", { paths: ["/tmp", {}] }); | ||
| } catch (e) { | ||
| err = e; | ||
| } | ||
| expect(err.code).toBe("ERR_INVALID_ARG_TYPE"); |
There was a problem hiding this comment.
🟡 The new tests only assert err.code, but both C++ sites go to the trouble of building makeString("paths["_s, i, "]"_s) so the error names the offending index — and the ["/tmp", {}] case is clearly designed to exercise index 1. Consider also asserting expect(err.message).toContain("paths[1]") here (and "paths[0]" for the DataView / _resolveFilename cases) so that an off-by-one in pathIndex++ or the loop counter would be caught.
Extended reasoning...
What the gap is
Both changed C++ sites deliberately construct an indexed argument name for the error:
ImportMetaObject.cpp:Bun::ERR::INVALID_ARG_TYPE(scope, globalObject, makeString("paths["_s, i, "]"_s), "string"_s, path)inside thefor (size_t i = 0; ...)loop.NodeModuleModule.cpp: a manually-maintainedsize_t pathIndex = 0;incremented inside theforEachInIterablelambda, withmakeString("paths["_s, pathIndex, "]"_s).
The new tests, however, only assert err.code === "ERR_INVALID_ARG_TYPE". They never look at err.message, so the index that the C++ code computes is untested.
Why it matters
The ["/tmp", {}] case at lines ~212-218 exists specifically to put the bad entry at a non-zero index. But if the NodeModuleModule.cpp implementation had pathIndex++ placed before the isString() check instead of after — a very easy mistake with a captured counter inside a lambda — the error would say paths[1] for the first element and paths[2] for the second, and every test in this file would still pass. Similarly, if the ImportMetaObject.cpp loop reported the wrong i, or if either site dropped the index entirely and just said "paths", nothing would fail.
REVIEW.md's test guidance is explicit: "assert the strongest invariant… specific error class/code/message (never bare toThrow())" and "Error messages are reviewed word-for-word as code". The tests already assert the specific error code (which is good — better than bare toThrow()), but since this PR's diff went out of its way to add index formatting, the tests should lock that in.
Step-by-step proof
- In
NodeModuleModule.cpp, the lambda body is:if (!item.isString()) { …INVALID_ARG_TYPE(…, makeString("paths["_s, pathIndex, "]"_s), …); return; } pathIndex++;
- Suppose a future refactor moves
pathIndex++;above theisString()check (or switches to a post-increment inside themakeStringcall and gets it wrong). - For
{ paths: ["/tmp", {}] }: iteration 0 sees"/tmp"(string) →pathIndexbecomes 1; iteration 1 sees{}→ error reportspaths[1]. Correct today. But with the increment moved before the check: iteration 0 →pathIndexbecomes 1, string, continue; iteration 1 →pathIndexbecomes 2, not string → error reportspaths[2]. - The current test only checks
err.code === "ERR_INVALID_ARG_TYPE", which is unchanged → test still passes despite the wrong index in the user-visible message.
Suggested fix
Add one assertion per case:
// DataView case
expect(err.code).toBe("ERR_INVALID_ARG_TYPE");
expect(err.message).toContain("paths[0]");
// ["/tmp", {}] case
expect(err.code).toBe("ERR_INVALID_ARG_TYPE");
expect(err.message).toContain("paths[1]");
// Module._resolveFilename [{}] case
expect(err.code).toBe("ERR_INVALID_ARG_TYPE");
expect(err.message).toContain("paths[0]");This is a test-strengthening suggestion, not a production defect — the runtime behavior is correct as written.
| test("require.resolve with a relative directory in options.paths does not crash", () => { | ||
| let err; | ||
| try { | ||
| require.resolve("nonexistent", { paths: ["not/an/absolute/path"] }); | ||
| } catch (e) { | ||
| err = e; | ||
| } | ||
| expect(err.code).toBe("MODULE_NOT_FOUND"); | ||
| }); |
There was a problem hiding this comment.
🟡 This test only proves the panic is gone — a resolver that skipped every non-absolute entry (replacing the abs_buf_checked call with a bare continue) would also pass it. The PR claims relative paths entries resolve against cwd like Node, but the load-bearing Some(p) => p arm has no coverage. Consider adding a positive case that creates a package under a cwd-relative directory and asserts it actually resolves.
Extended reasoning...
What's missing
The resolver.rs change introduces two distinct behaviors for non-absolute custom_dir_paths entries:
let abs_custom = if bun_paths::is_absolute(custom_slice) {
custom_slice
} else {
match self.fs_ref().abs_buf_checked(&[custom_slice], &mut abs_buf) {
Some(p) => p, // (a) absolutize against top_level_dir and search there
None => continue, // (b) skip on failure
}
};The new test at line 231 only asserts require.resolve('nonexistent', { paths: ['not/an/absolute/path'] }) throws MODULE_NOT_FOUND. That covers (a) doesn't panic, but it does not prove (a) actually searches the absolutized path.
Step-by-step: the test can't distinguish the fix from a weaker one
- Suppose the else-branch were replaced with just
continue;— i.e., silently skip every non-absolute entry without absolutizing. require.resolve('nonexistent', { paths: ['not/an/absolute/path'] })runs; the resolver sees one non-absolute entry, hitscontinue, exhaustscustom_paths, and returnsMODULE_NOT_FOUND.- The test asserts
err.code === 'MODULE_NOT_FOUND'→ passes. - Now with the real fix:
abs_buf_checkedreturnsSome(<cwd>/not/an/absolute/path),check_package_pathlooks there, finds nothing (the directory doesn't exist), returnsMODULE_NOT_FOUND. - The test asserts
err.code === 'MODULE_NOT_FOUND'→ also passes.
Both implementations produce identical observable behavior for this input. Per REVIEW.md: "confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test." The Some(p) => p arm is load-bearing for the PR description's claim ("This matches Node, which resolves relative paths entries against cwd"), and deleting it breaks no test.
Why existing coverage doesn't help
The other tests in this file (Module._resolveFilename respects options.paths for package resolution, require.resolve respects options.paths, etc.) all pass absolute temp-dir paths, so they take the is_absolute fast path and never exercise abs_buf_checked at all.
Suggested addition
Something along these lines (mirroring the existing createTempDir pattern in this file, plus a process.chdir so the relative path is deterministic):
test("require.resolve resolves a relative options.paths entry against cwd", () => {
const { path: dir, cleanup } = createTempDir("require-resolve-relative-paths", {
"sub/node_modules/rel-pkg/package.json": JSON.stringify({ name: "rel-pkg", main: "index.js" }),
"sub/node_modules/rel-pkg/index.js": "module.exports = 1;",
});
const prev = process.cwd();
try {
process.chdir(dir);
const resolved = require.resolve("rel-pkg", { paths: ["./sub"] });
expect(resolved).toBe(resolve(dir, "sub/node_modules/rel-pkg/index.js"));
} finally {
process.chdir(prev);
cleanup();
}
});This would fail (MODULE_NOT_FOUND) if the resolver merely skipped relative entries, and pass only when abs_buf_checked actually joins against cwd — so it pins the Node-compat behavior the PR description claims.
Severity
nit — the fuzzer-reported panic (the primary bug) is properly regression-tested: a panic would not yield err.code === 'MODULE_NOT_FOUND'. This is a coverage gap on the secondary Node-parity claim, worth adding but not blocking.
Fuzzer found a panic:
cannot resolve DirInfo for non-absolute path: [object DataView].What
require.resolve('x', { paths: [...] })andModule._resolveFilename(..., { paths: [...] })were coercing everypathsentry to a string viatoWTFString, so an object became"[object Foo]". That string then flowed into the resolver'scheck_package_pathas a search directory and hit thedir_info_cachedassertion that the input is absolute. A plain relative string like"rel/dir"hit the same assertion.Fix
ImportMetaObject.cpp/NodeModuleModule.cpp: throwERR_INVALID_ARG_TYPEwithpaths[i]when an entry is not a string, matching Node.js.resolver.rs: when iteratingcustom_dir_pathsfor package resolution, absolutize non-absolute entries againsttop_level_dirbefore callingcheck_package_path. This matches Node, which resolves relativepathsentries against cwd.Repro