Make --preserve-symlinks and --preserve-symlinks-main work in the runtime and bundler - #36757
Make --preserve-symlinks and --preserve-symlinks-main work in the runtime and bundler#36757robobun wants to merge 11 commits into
Conversation
--preserve-symlinks was parsed into resolver options but the per-file symlink resolution in Resolver::finalize_result was unconditional, so every resolved module was canonicalized to its real path and the flag had no effect on relative specifiers, bare specifiers, import.meta.url, or __filename. Gate that block on the option, matching Node: with the flag, the link path stays the module's identity so specifiers inside a symlinked module resolve from the link's directory. bun build hardcoded preserve_symlinks: false in BundleOptions::from_api, so the CLI flag never reached the bundler's resolver. Read it from the transform options. --preserve-symlinks-main had two gaps: the fast entry-point path in run_command derived the script path via get_fd_path (a realpath), and the synthetic main module resolve in the VM applied the general flag rather than the main flag. Node applies only --preserve-symlinks-main / NODE_PRESERVE_SYMLINKS_MAIN to the entry point (workers included), so the VM main resolve now overrides the resolver option with the main flag for that one resolve. Fixes #36755
Walkthrough
ChangesSymlink Preservation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:49 AM PT - Aug 2nd, 2026
❌ @robobun, your commit 88fed92 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36757That installs a local version of the PR into your bun-36757 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Not adding those to the PR description: #13365 and #8467 are about the Bun.build JS API, which this PR does not touch (no preserveSymlinks option is added, so the behavior cannot be enabled from Bun.build). #31957 is partially covered here (the bun build CLI flag now works, including for individual file symlinks), but #31958 is already open and scoped to it, including the Bun.build API option, so I'll leave closing that one to it. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Overlapping area but not the same fix, as noted at the end of the PR description: #31958 keeps realpathing individual file symlinks (only composed directory realpaths are skipped), so the repro in #36755 still fails with it, and it is scoped to the Bun.build API for #31957. #35782 addresses __filename spelling by swapping to the pretty path at the VM layer for a different issue and does not touch the resolver, so relative and bare specifiers inside symlinked modules still resolve from the target directory in the bundler. This PR gates the canonicalization in the resolver itself, which is what #36755 needs. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Flipping resolver.opts.preserve_symlinks around the entry-point resolve could seed the path-keyed DirInfo cache with entries computed under the wrong mode (abs_real_path left empty), so later resolves under a symlinked directory kept their link path even with the flag off. Instead, resolve with the process-wide mode and adjust only the returned entry path: recover the link spelling from the pretty path when the main flag is on, or canonicalize via open + get_fd_path when only the general flag is on (workers included).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/cli/run_command.rs (1)
2863-2901: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert exact paths in the symlink test. Compare
entryLineandfooLinewith the expected absolute paths derived fromdir, after normalizing separators.toEndWith()would not detect a duplicated working-directory prefix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/run_command.rs` around lines 2863 - 2901, Update the symlink test assertions in test/js/bun/resolve/resolve.test.ts (lines 1209-1230) to compare entryLine and fooLine against the expected absolute paths derived from dir, normalizing path separators first; replace suffix-only toEndWith checks with exact equality. The runtime path handling in src/runtime/cli/run_command.rs (lines 2863-2901) requires no direct change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/VirtualMachine.rs`:
- Around line 4106-4119: The temporary main-preserve handling in the resolver
path must not mutate shared resolver options or allow path-only DirInfo cache
entries to be reused across preserve_symlinks modes. Update the main-module
resolution flow around main_preserve to apply symlink preservation after
resolution via the resolved path/realpath handling, while leaving
opts.preserve_symlinks unchanged for the cached directory lookup.
In `@test/js/bun/resolve/resolve.test.ts`:
- Around line 1196-1207: Strengthen the negative case in the
`--preserve-symlinks` invocation by asserting that `withoutMain.stderr` contains
the expected module-not-found failure from resolving the relative import through
`shared/`, while retaining the nonzero exit-code assertion.
---
Outside diff comments:
In `@src/runtime/cli/run_command.rs`:
- Around line 2863-2901: Update the symlink test assertions in
test/js/bun/resolve/resolve.test.ts (lines 1209-1230) to compare entryLine and
fooLine against the expected absolute paths derived from dir, normalizing path
separators first; replace suffix-only toEndWith checks with exact equality. The
runtime path handling in src/runtime/cli/run_command.rs (lines 2863-2901)
requires no direct change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f452e9d7-ed0a-41d7-abdf-d544db5f41bf
📒 Files selected for processing (5)
src/bundler/options.rssrc/jsc/VirtualMachine.rssrc/resolver/resolver.rssrc/runtime/cli/run_command.rstest/js/bun/resolve/resolve.test.ts
…utating resolver options Same dir-cache hazard as the VM main resolve: the DirInfo cache is a process-lifetime singleton keyed by path only, so flipping opts.preserve_symlinks for the bun run fallback resolve could cache entries computed under the wrong mode. Resolve with the process-wide mode and adjust only the entry spelling afterwards (pretty path when the main flag is on, open + get_fd_path when only the general flag is on). Also strengthen the tests: exact path assertions in the symlinked-directory case, a stderr assertion on the negative case, and coverage for the extensionless bun run fallback path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/resolve/resolve.test.ts`:
- Around line 1205-1210: Extend the extensionless fallback test around the
existing `fallback` invocation to also run `bun run --preserve-symlinks
./mainsym` without `--preserve-symlinks-main`. Assert that this invocation emits
the expected missing-module diagnostic, produces empty stdout, and exits with a
nonzero status, while preserving the existing positive-case assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c69b59c-93f3-451c-b62b-c134e987406f
📒 Files selected for processing (2)
src/runtime/cli/run_command.rstest/js/bun/resolve/resolve.test.ts
…r --preserve-symlinks)
|
CI status at 361e8b5: every failure is unrelated to this diff. The new preserve-symlinks tests pass on all lanes; the red entries are a pre-existing bun-upgrade failure on Windows 11 aarch64 (canary artifact missing for that platform, also red on main) and batch-flaky tests that passed when retried or run alone (napi GC condition, request-clone leak thresholds, tty/shell regression batch flakes). The diff itself is green and ready for review. |
…rve_symlinks_main_enabled Collapses the duplicated open + get_fd_path canonicalization in VirtualMachine and run_command into one bun_sys helper (using the path buffer pool in both places), and folds the repeated flag-or-env-var expression into a RuntimeOptions method.
The worker entry funnel (resolve_entry_point_specifier) returned the resolved realpath unconditionally, discarding the link spelling before the VM's main resolve could recover it, so a symlinked worker entry diverged from Node under --preserve-symlinks-main. Recover the pretty spelling there, mirroring the two run_command entry paths. Verified against Node: worker entry keeps the link path with -main, is realpathed without it (test-require-symlink still passes).
…oads resolve_entry_point_specifier also serves the preload_modules loop in create(); preloads are not the main module, so they follow the general --preserve-symlinks flag. Thread is_main_entry through and gate the pretty recovery on it.
There was a problem hiding this comment.
All four prior findings (dir-cache option flip, weak negative assertion, duplicated realpath helper, worker entry / preload scoping) have been addressed and no new issues surfaced this pass, but module resolution identity is a critical path and the -main/-general interaction across four entry funnels (fast path, resolve fallback, VM synthetic main, worker) plus the shared dir-cache is subtle enough that a maintainer should sign off.
What was reviewed:
finalize_resultgate onopts.preserve_symlinksand its effect on the shared dir cache — the post-resolve adjustment (rather than flipping the option) keeps DirInfo entries consistent.realpath_for_main's&'statictransmute — the box is pushed intoresolved_path_dupsbefore the slice escapes, matchingdupe_resolved_path.is_main_entrythreading inresolve_entry_point_specifier— both callers pass the correct value; the preload loop stays on the general flag.- New
bun_sys::realpath_by_open— fd is closed on both the ok and err arms ofget_fd_path.
Extended reasoning...
Overview
The PR wires --preserve-symlinks and --preserve-symlinks-main through to actual behavior across the runtime and bundler. Eight files change: the resolver's finalize_result gains a gate on opts.preserve_symlinks; BundleOptions::from_api reads transform.preserve_symlinks instead of hardcoding false; the VM's _resolve gains a main_preserve post-resolve adjustment for the synthetic main import; run_command.rs gains the same adjustment at both the resolve-fallback path and the direct-file fast path; web_worker.rs threads is_main_entry so a worker's entry (but not its preloads) honors -main; bun_sys gains a shared realpath_by_open helper; and RuntimeOptions gains preserve_symlinks_main_enabled() to fold the flag+env check. Eight new concurrent tests in resolve.test.ts cover the ESM/CJS/bare/import.meta.url/entry/worker/build matrix with both positive and negative (specific stderr) assertions.
Security risks
None identified. This is a resolution-semantics change; no new untrusted-input parsing, no auth/crypto, no privilege boundaries. realpath_by_open opens read-only and bounds-checks the input path length before copying into the pooled buffer.
Level of scrutiny
High. Module resolution identity is on the hot path of every import in every project, and symlink handling is exactly where package-manager layouts (pnpm, workspaces) diverge. The fix deliberately avoids flipping resolver.opts.preserve_symlinks for a single call because that option shapes the process-lifetime DirInfo cache (keyed by path only) — an earlier revision did flip it and was corrected during review. That kind of non-local invariant, plus the four separate entry funnels that must agree in both the Some(true) (recover .pretty) and Some(false) (realpath after the fact) directions, is the sort of thing a maintainer familiar with the resolver's cache model should confirm.
Other factors
The PR went through four rounds of substantive review feedback (all from the automated bug hunter, all now resolved): the dir-cache option flip, a weak negative test assertion, near-identical realpath helpers, the worker-entry Some(true) gap, and the preload-vs-entry scoping. Each was fixed in a follow-up commit and the fixes look correct on re-inspection. CI is reported green for the diff (unrelated flakes only). The unsafe transmute in realpath_for_main follows the existing dupe_resolved_path pattern in the same file (box pushed to resolved_path_dups, drained in destroy()). The test suite is thorough and uses test.concurrent per file conventions. Given the breadth (resolver + bundler + four entry paths + workers) and the criticality of module identity, this exceeds the bar for auto-approval even with clean CI and no open findings.
1 participant
Fixes #36755
--preserve-symlinkswas accepted but had no effect in the runtime: module identity was always the real path, so every specifier inside a symlinked module (relative and bare alike) resolved from the symlink target's directory instead of the link's. Reproduces on Linux as well as Windows.bun build --preserve-symlinksand--preserve-symlinks-mainwere also no-ops.Repro
Cause
The flag was parsed and stored on
resolver.opts.preserve_symlinks, but its only consumer was the directory realpath skip indir_info. The per-file symlink resolution inResolver::finalize_resultwas unconditional: it always calledpath.set_realpath(symlink), which swapspath.text(the module's identity) to the target. Three more gaps on top of that:BundleOptions::from_apihardcodedpreserve_symlinks: false, sobun build --preserve-symlinksnever reached the bundler's resolver.maybe_open_with_bun_js) derived the script path viaget_fd_path(fd), a realpath, ignoring--preserve-symlinks-main.--preserve-symlinks-main/NODE_PRESERVE_SYMLINKS_MAINthere (workers included).Fix
finalize_resulton!opts.preserve_symlinks, so the link path stays the module's identity and specifiers inside it resolve from the link's directory. This fixes the runtime andbun buildin one place.BundleOptions::from_apireadstransform.preserve_symlinks.--preserve-symlinks-main/NODE_PRESERVE_SYMLINKS_MAINis set.NODE_PRESERVE_SYMLINKS=1alone no longer preserves a worker's entry path, matching Node).Verification
Full matrix (relative/bare/ESM/CJS/
import.meta.url/symlinked entry, with each flag combination) now matchesnodeexactly; 7 new tests intest/js/bun/resolve/resolve.test.tsfail on the released bun and pass with this change.test/js/node/test/parallel/test-require-symlink.js,test-module-main-preserve-symlinks-fail.js, andtest-module-symlinked-peer-modules.jspass.Related open PRs touch neighboring ground with different scope: #31958 threads the flag into
Bun.buildand handles symlinked directories (but keeps realpathing individual file symlinks, so the repro above still fails), and #35782 addresses__filenamespelling at the VM layer for a different issue. This PR fixes the resolver itself, which is what #36755's repro needs.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/resolve/resolve.test.ts