bundler: compute [dir] for entries whose directory does not exist on disk without re-joining the cwd - #38679
bundler: compute [dir] for entries whose directory does not exist on disk without re-joining the cwd#38679robobun wants to merge 1 commit into
Conversation
…n disk When the entry's directory cannot be opened (an in-memory files: entry or a plugin-resolved path), compute_chunks normalized it with normalize_buf before handing it to relative_alloc. normalize_buf drops the leading separator of an absolute path (and the leading .. of a relative one), so relative_alloc took the result as cwd-relative and joined it onto the cwd: /tmp/app/missing/a.js built from /tmp/app came out as tmp/app/missing/a.js. relative_alloc normalizes both of its arguments itself, so pass the directory through unchanged.
|
Warning Review limit reached
Next review available in: 35 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 |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the removed normalize_buf call was redundant with relative_alloc's own normalization and was dropping the leading / (or ..), which is exactly what caused the doubled-cwd output paths.
What was reviewed:
- Confirmed
relative_alloc→relative_platform_bufnormalizes both args itself and re-prepends the separator for absolute inputs (resolve_path.rs:661-680), so passingdir_pathunchanged is correct; the on-diskopenat+get_pathbranch is untouched. - Lifetime of
dir_pathin the fallback: it borrowsoutput_paths[...]which outlives the block (and borrowck would reject otherwise). - Tests pair an on-disk dir with a missing one for each case and assert both, use
tempDir/bunEnv/bunExe, drain subprocess pipes concurrently, and assert stderr/output before exit code. - Windows path-separator concern in the new test assertions was examined and ruled out.
Extended reasoning...
Overview
One-line source change in src/bundler/linker_context/computeChunks.rs: when openat on an entry's directory fails (in-memory files: entry or plugin-resolved virtual path), the fallback now passes the raw dir_path to relative_alloc instead of first running it through resolve_path::normalize_buf. Four new tests in test/bundler/bundler_files.test.ts cover absolute in-root, absolute out-of-root, plugin-resolved absolute, and plugin-resolved relative-with-.. entries, each paired with an on-disk sibling directory to assert the two produce the same [dir].
Correctness
I read relative_platform_buf (src/paths/resolve_path.rs:651-691) and confirmed the PR description's claim: it branches on is_absolute, normalizes into an internal buffer, and for POSIX absolute inputs writes the separator back at index 0 before slicing — so the leading / is preserved. For relative inputs it normalizes (keeping ..) and joins against FileSystem::top_level_dir, which is the same cwd the failed openat(Fd::cwd(), ...) was relative to. The old normalize_buf call therefore added nothing and only stripped the root/.., causing relative_platform_buf to then re-join the cwd and produce tmp/app/missing instead of missing. The success path (directory exists on disk → get_path) is unchanged, so symlink resolution and the outside-root _.._ rewriting behave exactly as before.
The returned dir_path slice borrows pathname.dir → output_paths[entry_point_id], which is a graph column that lives for the whole function; the borrow checker guarantees this, and relative_alloc copies into a fresh Box<[u8]> before the loop iteration ends.
Security risks
None. This is output-path naming for bundle artifacts, not a filesystem write path; the .. → _.._ rewrite that keeps outputs inside outdir happens later in PathTemplate::print and is untouched. The change strictly makes the fallback match the on-disk branch's result.
Level of scrutiny
Low-to-medium. A single deleted call in a well-understood fallback branch, with a precise mechanistic explanation traced to specific line numbers in resolve_path.rs, verified against the unfixed binary (USE_SYSTEM_BUN=1 fails, bun bd passes), and adjacent bundler test suites re-run. The PR description also flags three overlapping open PRs that rewrite this block for other reasons and notes the tests remain valid regardless — that's a coordination note for maintainers, not a correctness concern for this change.
Other factors
Tests follow repo conventions cleanly: tempDir with using, describe.concurrent for independent cases, subprocess pipes drained via Promise.all, stderr and parsed stdout asserted before exitCode, and each test asserts the on-disk control alongside the in-memory case so a regression in either direction fails. The one candidate finding (Windows backslash separators in the new assertions) was examined by the verifier agents and ruled out. No prior review comments to address.
1 participant
Problem
Bun.build()gives an entry point whose directory does not exist on disk an output path that repeats the cwd's own components. With cwd/tmp/app, afiles:entry/tmp/app/missing/a.jsis emitted astmp/app/missing/a.js; the same entry in a directory that exists (/tmp/app/src/a.js) is emitted assrc/a.js.onResolveplugin resolves to a virtual namespace: its output path is computed from the name passed inentrypoints, and that directory usually does not exist either. A relative name that leaves the cwd (../lib/a.js) loses its..and is emitted aslib/a.jsinstead of_.._/lib/a.js.src/bundler/linker_context/computeChunks.rs:654. Whenopenatof the entry's directory fails, the[dir]code falls back toresolve_path::normalize_bufbefore callingrelative_alloc(root_dir, dir).normalize_buf(normalize_string_generic_tz,src/paths/resolve_path.rs:907) does not write the leading separator of a POSIX absolute path, and drops the leading..of a relative one, so/tmp/app/missingbecomestmp/app/missing.relative_platform_buf(resolve_path.rs:695) then sees a relative path and joins it onto the cwd, giving/tmp/app/tmp/app/missing, and the output path is computed from that./src/entry.jsfrom thefiles:map is emitted assrc/entry.json a machine without a/srcdirectory and as a path outside root on a machine that has one.Fix
relative_allocunchanged instead of pre-normalizing it.relative_allocnormalizes both arguments itself (relative_platform_buf): an absolute path is normalized and keeps its root, a relative one is normalized with..kept and then resolved against the cwd, which is the same directory theopenatthat just failed was relative to. The pre-normalization added nothing and only lost the root. A directory that does exist still goes throughopenat+get_pathas before, so resolved paths, symlink handling and the outside-root_.._rewriting are unchanged; an in-memory directory now produces the same[dir]as an on-disk one at the same path.test/bundler/bundler_files.test.ts, describe[dir] of an entry whose directory does not exist on disk. Each test pairs a directory that exists with one that does not and checks both output paths: absolutefiles:entries inside root (including a..segment in the key, showing normalization still happens), absolutefiles:entries outside root, plugin-resolved absolute entries, and plugin-resolved relative entries that leave the cwd (a subprocess, since relative names resolve against the cwd).bun bd test test/bundler/bundler_files.test.ts: 27 pass.USE_SYSTEM_BUN=1 bun test test/bundler/bundler_files.test.ts -t "does not exist on disk"(1.4.0): the 4 new tests fail with the paths described above.bun bd test test/bundler/bundler_naming.test.ts test/bundler/bundler_plugin_chain.test.ts test/bundler/bundler_plugin.test.ts test/bundler/bun-build-api.test.ts test/bundler/bundler_edgecase.test.ts: all pass.files:keys are not used in the tests because a relative key still tripsPath::assert_file_path_is_absoluteon debug builds; bundler: resolve relative Bun.build files keys against the cwd #38650 covers that separately. bundler: canonicalize the asset source directory for [dir] only when string relativization falls outside root #34558, bundler: keep the symlink spelling of entry-point output paths #35660 and Remove get_fd_path: derive paths from cwd and what was opened, not from fds #38365 rewrite this block for other reasons (asset[dir]canonicalization, symlink spelling, removingget_fd_path) and would make the source change here moot if they land first; the tests still apply to them.Background
[dir]is the placeholder in thenamingtemplates (default entry template[dir]/[name].[ext]) that stands for the entry's directory relative to the build root. For a directory outside the root the relative path starts with.., whichPathTemplate::printrewrites to_.._so the output cannot escape the outdir.root_dir) is therootoption, or for a build whose entry points all come fromfiles:the cwd;JSBundlercanonicalizes it withget_fd_path.files:is theBun.buildoption that supplies in-memory sources. Its keys are used verbatim as source paths, so a key's directory need not exist on disk. A plugin-resolved entry point keeps the name given inentrypointsas its output path (entry_point_original_names), which reaches the same code.resolve_path::normalize_bufcollapses./../duplicate separators in place. On POSIX the underlying normalizer emits only the path components, never the leading/; callers that need an absolute result add it back themselves (relative_platform_bufdoes exactly this withbuf[1..]).relative_allocispath.relative()over two paths that it first normalizes and makes absolute againstFileSystem::top_level_dir(the cwd).