bundler: keep the symlink spelling of entry-point output paths - #35660
bundler: keep the symlink spelling of entry-point output paths#35660robobun wants to merge 2 commits into
Conversation
When an entry point is reached through a symlink (e.g. a workspace package under node_modules), the resolver swaps in the real path and the linker's per-chunk open+get_fd_path canonicalization strips the symlink again, so the output lands at dist/_.._/packages/mypkg/index.js instead of dist/node_modules/mypkg/index.js. Record the pre-symlink absolute path in entry_point_original_names (the same map plugin-resolved virtual entries use), and have computeChunks skip the get_fd_path round-trip when a plain relative() already sits under root_dir. The canonicalization fallback is kept for short-name/symlinked-cwd spellings that need it to find a common prefix. Fixes #8467 Fixes #13365
|
Warning Review limit reached
Next review available in: 5 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 (3)
Comment |
|
Updated 9:35 AM PT - Jul 25th, 2026
⏳ @robobun, your commit 81b9ef9 is still building in
|
|
Reproduced with a symlinked workspace package as entry point: 81b9ef9 addresses self-review: the onResolve NoMatch fallback was overwriting the absolute pre-symlink path with the relative specifier, which bypassed the fix for any build with an
CI: the new test passed on every lane that ran it (darwin x64/aarch64, debian x64, debian x64-asan, windows 2019 x64). The red on #80940 is unrelated to this diff:
Ready for a maintainer. |
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-visible bundler output-path derivation (skipping the per-chunk get_fd_path canonicalization whenever the dir already relativizes under root_dir) and the description notes it will conflict with two open PRs touching adjacent naming code — worth a human look.
What was reviewed:
entry_point_original_names.putcopies into aBox<[u8]>, so storing the borrowedpath.prettyis safe; the map's existing consumer inLinkerGraph::loadalready routes it intooutput_path.- The new
relative_platformfast-path writes into a thread-local buffer that is only inspected for the..prefix and not held across the laterrelative_alloccall — no aliasing. - The
b"."/ non-absolutedir_pathcase is gated out byis_absolute, so it still falls through to the existing open+get_fd_path branch.
Extended reasoning...
Overview
Two source hunks and a test. bundle_v2.rs:enqueue_entry_item records path.pretty (the pre-set_realpath absolute path) into graph.entry_point_original_names when the resolver followed a symlink for a file-namespace entry point; that map already feeds LinkerGraph::load's output_path column, so this reuses an existing mechanism rather than adding new plumbing. computeChunks.rs hoists root_dir a few lines earlier and adds a guard: if dir_path is absolute and relative(root_dir, dir_path) doesn't start with .., use dir_path directly instead of openat + get_fd_path. The new test in bun-build-api.test.ts builds three entries (two behind a symlinked/junctioned node_modules dir, one local) in a subprocess and asserts the exact sorted output-path list.
Security risks
None. The change reads a path the user already provided as an entry point and uses it for output naming; it doesn't introduce any new file opens, and in the common case removes one. No untrusted-input parsing was added.
Level of scrutiny
Medium-high. The diff is small and mechanically clear, but it alters output-path derivation for every build where entry-point dirs sit under root_dir — previously those were always canonicalized via get_fd_path, now they aren't. The PR verified the naming/loader/splitting/plugin suites, which is reassuring, but this is the kind of user-visible behavior change (and the noted overlap with #31958 / #34558) that benefits from a maintainer glance.
Other factors
I checked that IndexStringMap::put boxes the value (so storing path.pretty before path_with_pretty_initialized overwrites it is fine), that path.is_file() is a namespace check (not a stat), and that the relative_platform result in the guard is consumed immediately and not aliased into the later relative_alloc. The test follows repo conventions (tempDir, bunEnv, subprocess, drains all pipes concurrently, exit-code asserted last, issue URL comment) and covers both Windows junctions and POSIX dir symlinks.
The onResolve NoMatch fallback was overwriting the absolute pre-symlink path that enqueue_entry_item recorded with the relative specifier, so any onResolve plugin whose filter matched the entry and returned undefined still produced _.._/ paths. That put is now redundant (enqueue_entry_item records the absolute pre-symlink path itself) and dropped. Test extended to cover both the no-plugin and no-op-onResolve paths plus a file-level symlink with a differing basename, which pins [name] as well as [dir]. Comment blocks collapsed to one line each.
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-visible bundler output-path semantics (when get_fd_path canonicalization is skipped, and what entry_point_original_names stores for the plugin-NoMatch fallback), so it's worth a human look — the description also flags two open PRs that will conflict here.
What was reviewed:
path.prettyat the stash point is the pre-symlink absolute path (viaset_realpath), andIndexStringMap::putclones — no lifetime issue.- Removed unconditional specifier store in the plugin-NoMatch arm: for non-symlinked entries the fallback to
source.path.textplus the newcompute_chunksfast path yields the same[dir]; for symlinked entries the new stash covers it. relative_platformresult is only read for.starts_with(b"..")before the thread-local buffer is reused byrelative_alloc.- The test's
symlinkSync(..., "file")on Windows may need dev-mode; worth confirming CI is configured for it.
Extended reasoning...
Overview
The PR fixes #8467/#13365: entry points reached through a symlink (typical monorepo node_modules/mypkg -> ../../packages/mypkg) were being emitted at dist/_.._/packages/... because compute_chunks canonicalized the entry directory via get_fd_path before relativizing against root_dir. Two changes: (1) enqueue_entry_item now stashes the pre-symlink absolute path (held in path.pretty after set_realpath) into entry_point_original_names for symlinked file entries, replacing the plugin-NoMatch arm's unconditional store of the raw specifier; (2) compute_chunks short-circuits the open + get_fd_path step when dir_path is already absolute and lexically under root_dir, falling through to canonicalization only when the plain relative walks above root (short-name / symlinked-cwd cases).
Security risks
None identified. This is output-path derivation for build artifacts; input is the user's own entry-point spellings and the resolver's realpath output. No untrusted-boundary parsing is added.
Level of scrutiny
Moderate-to-high. The mechanism is sound and matches esbuild, but it is a user-visible behavior change to where files land on disk, and it removes a previously-unconditional canonicalization. I traced the plugin-NoMatch semantic change: non-symlinked entries now fall back to source.path.text (absolute) instead of the stored relative specifier, which combined with the new is_absolute && !starts_with("..") fast path produces the same [dir] result — but a maintainer who owns this area should confirm nothing else depends on the old canonicalize-everything behavior (e.g. de-duplicating differently-spelled paths).
Other factors
Test coverage is good (dir junction, file symlink with a different basename, plugin/no-plugin matrix, asserts against both _.._ and the real target basename). Two things a human should sanity-check: the Windows symlinkSync(..., "file") call assumes dev-mode on the CI runner (the neighboring symlink test is .skipIf(isWindows)), and the description explicitly notes merge-conflict coordination with #31958 and #34558.
Fixes #8467
Fixes #13365
Repro
Cause
resolve_entry_pointrunsfinalize_result, which callsPath::set_realpathsosource.path.textbecomes the canonical location.LinkerGraph::loadcopies that intoentry_points.output_path, andcompute_chunksadditionally opens the directory and reads itsget_fd_pathbefore relativizing againstroot_dir. The real path sits aboveroot_dir(which is derived from the user-spelled entry-point prefix), so the result is../packages/mypkgand the[dir]sanitizer rewrites that to_.._/packages/mypkg.Fix
enqueue_entry_item: when the resolver followed a symlink for a file entry, record the pre-symlink absolute path (still inpath.prettyat that point) ingraph.entry_point_original_names[source_index]. That map already feedsentry_points.output_pathfor plugin-resolved virtual entries.compute_chunks: before opening the directory, try a plainrelative(root_dir, dir). If it does not walk aboveroot_dir, usediras-is; only fall back toopen + get_fd_pathotherwise. This preserves the entry-point spelling, skips a per-chunk syscall in the common case, and keeps the canonicalization path for Windows short-name / symlinked-cwd spellings that need it to find a common prefix.esbuild derives entry output paths from the input path the user wrote, not the canonical path; this brings Bun in line.
Test
test/bundler/bun-build-api.test.tsadds a three-entry build (two behind a junction/symlink intonode_modules, one local) and asserts the output paths aredist/node_modules/mypkg/*anddist/src/local.jswith no_.._. Fails on main withdist/_.._/packages/mypkg/*, passes with this change.Verified:
bundler_naming,bundler_loader,esbuild/loader,bundler_splitting,bundler_plugin, and the fullbun-build-apisuite. The CLI (bun build) path goes through the sameenqueue_entry_item, confirmed manually.Related: #31958 adds
preserveSymlinksfor resolution but leaves output naming on the real path; #34558 touches the adjacent asset-naming canonicalization. Both are open and will have a small, aligned merge conflict with thecompute_chunkshunk here.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts