Repository navigation
Conversation
dir_info_uncached reads the tsconfig override when parent.is_none(), i.e. while scanning the filesystem root. It passed that directory fd as dirname_fd to parse_tsconfig, so read_file_with_allocator did openat(root_fd, basename(override_path)) which fails with ENOENT and prints the "Internal error: directory mismatch" fallback warning on every run. The absolute-path retry succeeds, so the override still applies; only the warning is spurious. Pass FD::ZERO for the override path so the absolute open is taken directly, matching the extends-chain call site. Drop the test workaround that tolerated the warning so the existing six cases assert an empty stderr.
|
Warning Review limit reached
Next review available in: 21 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 (2)
Comment |
|
Updated 11:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 17674b2 has some failures in 🧪 To try this PR locally: bunx bun-pr 34980That installs a local version of the PR into your bun-34980 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the dirname_fd passed to parse_tsconfig now correctly falls back to FD::ZERO when --tsconfig-override is set, so read_file_with_allocator opens by absolute path instead of attempting openat(root_fd, basename).
What was reviewed
- Traced
tsconfig_pathassignment: override branch is only entered whenparent.is_none()andtsconfig_override.is_some(), so gating ontsconfig_override.is_none()exactly discriminates the two sources oftsconfig_path. - Confirmed
FD::ZEROaliasesFd::INVALID(resolver.rs:252), sodirname_fd.is_valid()is false andread_file_with_allocator(lib.rs:2512) takes the absolute-open path — matching theextends-chain call below. - Test change only removes the pre-existing
if (!stderr.includes("directory mismatch"))guard; the underlyingexpect(stderr).toBe("")assertions were already there. ASAN + release evidence in the PR shows 6 fail before / 6 pass after.
Extended reasoning...
Overview
One-line semantic change in src/resolver/resolver.rs (dir_info_uncached): when computing the dirname_fd argument to parse_tsconfig, add && self.opts.tsconfig_override.is_none() to the existing STORE_FILE_DESCRIPTORS gate so the override path is opened absolutely rather than via openat(fd, basename). The rest of the resolver diff is reindentation from hoisting dirname_fd into a let — the error-handling body is byte-identical. The test diff removes six if (!stderr.includes("Internal error: directory mismatch")) guards that had been tolerating the bug.
Correctness
I traced the three ways tsconfig_path can be populated at this site (resolver.rs:6372–6417): (a) tsconfig_override.is_none() → tsconfig.json/jsconfig.json found in the directory being scanned, where fd is that directory's fd, so openat(fd, basename) is correct; (b) tsconfig_override.is_some() && parent.is_none() → the override path, where fd is the filesystem root's fd, so openat would miss; (c) tsconfig_override.is_some() && parent.is_some() → tsconfig_path stays None and the block isn't entered. Gating on tsconfig_override.is_none() therefore passes fd exactly in case (a) and FD::ZERO exactly in case (b). FD::ZERO is defined as Fd::INVALID at resolver.rs:252, so read_file_with_allocator at lib.rs:2512 sees !dirname_fd.is_valid() and opens by absolute path directly — no fallback, no warning. This mirrors how the extends-chain call a few lines below already passes an invalid fd for absolute-path opens.
Security risks
None. This changes which fd is used to open a config file the user explicitly named on the CLI; no new inputs are trusted and no validation is weakened.
Level of scrutiny
Low-to-moderate. The resolver is hot-path code, but this change only affects the --tsconfig-override branch (a once-per-process config load, gated on parent.is_none()). The behavioral outcome was already correct — the absolute-path fallback in read_file_with_allocator was succeeding — this just stops the spurious diagnostic by taking the right path first. No allocation, ownership, or error-handling semantics change.
Other factors
The PR includes fails-before/passes-after evidence on both ASAN-debug and release builds across all six existing --tsconfig-override tests. The test tightening is a strict strengthening of pre-existing assertions (the expect(stderr).toBe("") calls were already there, just conditionally skipped), not new bare-stderr assertions. The 3-line comment added at the change site is within repo guidelines and explains the non-obvious fd-vs-override-parent mismatch.
|
CI status for 17674b2 (build 93992): every lane that ran passed (177/177 jobs), including |
FD::ZERO was a resolver-local alias for Fd::INVALID and this was its last use, so spell it directly (as the extends-chain call below already does) and drop the alias. Adds a bun build --tsconfig-override case whose stderr must be empty, alongside the existing bun run cases.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that passes FD::INVALID as dirname_fd when --tsconfig-override is set, so read_file_with_allocator opens the override by absolute path instead of openat(root_fd, basename).
What was reviewed:
- Traced
tsconfig_pathassignment: whentsconfig_override.is_some()it's only set in theparent.is_none()branch, so the non-override case (wherefdis correct) is unaffected. - Confirmed
read_file_with_allocator(src/resolver/lib.rs:2339) takes the direct absolute-path open whendirname_fd.is_valid()is false — no fallback warning. - Verified the removed
FdZerotrait had no other users;FD::ZEROwas just an alias forFD::INVALID. - The comment-cop flag on line 6536 looks like a false positive — the 3-line comment explains a non-local invariant (why
fdis the wrong directory here), not a workaround. Trimming it to one line would be fine but isn't blocking.
Extended reasoning...
Overview
Two files: src/resolver/resolver.rs (one-condition change in dir_info_uncached's tsconfig loading, plus removal of the now-unused FdZero trait alias) and test/cli/run/tsconfig-override.test.ts (removed six if (!stderr.includes("Internal error: directory mismatch")) guards so all cases now assert stderr === "", and added a bun build --tsconfig-override test).
The mechanism is sound: tsconfig_path is set to the override only when parent.is_none() (scanning the filesystem root), so the local fd is the root directory's fd, not the override file's parent. read_file_with_allocator at lib.rs:2339-2361 gates the openat(dirname_fd, basename(path)) fast path on dirname_fd.is_valid(); passing FD::INVALID sends it straight to the absolute open_file(path) at line 2360, which is what the ENOENT fallback was reaching anyway — just without the warning. The non-override branch (tsconfig_override.is_none()) still passes fd, which is correct there since tsconfig_path is <dir>/tsconfig.json and fd is <dir>'s fd.
Security risks
None. This changes which fd is passed to an internal file-open helper for a CLI-provided config path; no new inputs, no privilege changes, no path validation weakened.
Level of scrutiny
Low-to-medium. The functional change is a single boolean condition on which fd to pass; the rest is reindentation of the existing error-handling match and dead-code removal (FdZero had one caller, replaced inline with FD::INVALID). The error branches (ENOENT → "Cannot find tsconfig", other → "Cannot read file") are byte-identical to before. This matches the extends-chain call further down which already passes FD::INVALID for the same reason.
Other factors
- Tests: six existing tests were tightened from tolerating the warning to asserting clean stderr — a strict strengthening. A new
bun buildtest covers the bundler entry point per the sibling-entry-point rule. PR evidence shows the tightened tests fail on the unfixed debug build and pass with the fix. - CI: the diff's own test file passed on every lane; the one CI failure (
bun-server.test.tswebsocket-idle on darwin aarch64) is unrelated and reproduces on main per the robobun follow-up. - The comment-cop bot flagged the new 3-line comment as "paragraph-long workaround justification". I read it as a legitimate why-comment for a non-local invariant (you'd otherwise have to trace
fd's provenance up throughdir_info_uncachedto see it's the root fd here), not a workaround — the code is the fix. Could be one line, but that's cosmetic.
There was a problem hiding this comment.
LGTM — targeted one-condition fix with the tolerance guards removed from the existing tests so they actually assert it.
Reviewed: the dir_info_uncached branch structure — tsconfig_path only comes from the override when tsconfig_override.is_some(), so gating fd on .is_none() is exact. Confirmed read_file_with_allocator falls through to the absolute-path open when dirname_fd.is_valid() is false (lib.rs:2339). Verified FdZero/FD::ZERO have no remaining users repo-wide. The comment-cop note was addressed in 17674b2 (now a single line) and the thread is resolved.
Extended reasoning...
Overview
Two files touched. In src/resolver/resolver.rs, the parse_tsconfig call inside dir_info_uncached now passes FD::INVALID as dirname_fd when --tsconfig-override is set, instead of the fd of the directory currently being scanned (which is / when the override is loaded, since that happens at parent.is_none()). The dead FdZero trait alias for Fd::INVALID is removed. In test/cli/run/tsconfig-override.test.ts, six existing tests drop the if (!stderr.includes("Internal error: directory mismatch")) guard so they assert stderr === "" unconditionally, and a seventh bun build --tsconfig-override case is added with the override file in a subdirectory.
Correctness
I traced the enclosing if/else if at resolver.rs:6487–6531. The tsconfig.json/jsconfig.json lookup branch is entered only when tsconfig_override.is_none(), and there fd is genuinely the containing directory. The else if parent.is_none() branch is the only place tsconfig_path becomes the override path, and that requires tsconfig_override.is_some(). So the new condition && self.opts.tsconfig_override.is_none() cleanly separates the two: the non-override path keeps the openat fast path, the override path falls through to open_file(path, ...) in read_file_with_allocator (lib.rs:2359) via the dirname_fd.is_valid() check at lib.rs:2339 — the same route the extends-chain resolution already uses. Grep confirms FD::ZERO/FdZero have no other references, so the trait removal is safe.
Security risks
None. This changes which fd is passed to an internal file-open helper for a CLI-provided config path; no new inputs, no privilege boundaries crossed.
Level of scrutiny
Low-to-moderate. The functional change is a single boolean guard on an argument that already had a working absolute-path fallback (the override was applying correctly before; only the warning was spurious). The test changes strictly tighten assertions rather than loosening them, which is the right direction. CI (robobun) reports the touched test file green on every lane; the lone darwin-aarch64 failure in bun-server.test.ts is unrelated and reproduces on main.
Other factors
The comment-cop bot flagged a long comment on an earlier revision; commit 17674b2 shortened it to one line and the thread is marked resolved. PR evidence shows the tightened tests fail on a debug build without the fix and pass with it, satisfying the USE_SYSTEM_BUN=1 gate. No prior reviews from me on this PR.
|
Closing in favor of #38606, which removes the same Verified against a debug build of #38606: the line is gone on both the |
* fix(resolver): avoid unrelated fd for tsconfig overrides Read explicit tsconfig overrides by their absolute path instead of borrowing the directory descriptor from the resolver walk. Keep ordinary config discovery and override precedence unchanged. Adapt the narrow FD repair from oven-sh#34980 and require clean stderr for CLI run/build override resolution, including unused malformed disk configs. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com> * docs(changelog): place override fd fix beside descriptor entry --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
Problem
bun --tsconfig-override <path> ...(andbun build --tsconfig-override) prints on every run:Internal error: directory mismatch for directory "/path/to/tsconfig.custom.json", fd 4. You don't need to do anything, but this indicates a bug.dir_info_uncached(src/resolver/resolver.rs, tsconfig block around line 6533) loads the override while scanning the filesystem root and passed that root directory's fd asdirname_fdtoparse_tsconfig.read_file_with_allocator(src/resolver/lib.rs) then triesopenat(root_fd, basename(override)), gets ENOENT, prints the warning, and falls back to an absolute open that succeeds.--tsconfig-overrideshow internal error #22023. Also removes the warning mentioned in passing inBun.build()tsconfigoption doesn't resolve path aliases (CLI works) #26793 and jsxImportSource from nested tsconfig ignored when running from workspace root #28605 (those issues track other bugs).Fix
FD::INVALIDasdirname_fdwhenevertsconfig_overrideis set, so the absolute open is taken directly. The non-override branch still passesfd, where it is the directory that actually containstsconfig.json.extendschain a few lines below, which already passesFD::INVALID.FD::ZEROwas a resolver-local alias forFd::INVALIDwith this one remaining use, so the alias is removed.bun runcases had anif (!stderr.includes("Internal error: directory mismatch"))guard tolerating the warning; that guard is removed so they assertstderr === "", and abun build --tsconfig-overridecase is added.USE_SYSTEM_BUN=1 bun test test/cli/run/tsconfig-override.test.ts: 7 fail (warning on stderr)bun bd test test/cli/run/tsconfig-override.test.ts: 7 passBackground
DirInfoper directory;dir_info_uncachedfills it in, and when it is created for a directory that has atsconfig.jsonit parses that file. With--tsconfig-overridethe override is parsed once instead, attached to the rootDirInfo(theparent.is_none()case), so every directory inherits it.STORE_FILE_DESCRIPTORSis on, the resolver keeps directories open and reads files withopenat(dir_fd, basename)to save path lookups.read_file_with_allocatortakes an optionaldirname_fdfor this; an invalid fd means "open by absolute path".[review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 4
evidence per changed file