Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. Walkthrough
ChangesIsolated symlink replacement
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Isolated installs now restore dependency symlinks when an empty or unrelated directory occupies the link path, while preserving patch workspaces. The covered scenarios indicate no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:58 AM PT - Sep 6th, 2026
❌ @robobun, your commit 4b57000 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 41453That installs a local version of the PR into your bun-41453 --bun |
|
Status: fixed, ready for review. Reproduced on the current release in an isolated-linker project: CI: every job that exercises the install tests is green. The red lanes ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow refinement of the existing "real directory at link slot" guard, with matching tests.
What was reviewed:
Path::save()returns aResetScopethat truncates onDrop, andPath::appendauto-inserts the separator, so the probe is<dest>/package.jsonandself.destis restored beforedelete_tree.- Discarding the
delete_treeresult matches the existingExpectMissing/EEXISTarm; a failure surfaces via the subsequentsymlink(). - Considered the "patch workspace with
package.jsondeleted" case — the hoisted linker already treats absence ofpackage.jsonas not-installed, so this is consistent, and a patch copy always starts with one. - New tests reuse the file's
unlink+mkdirandisSymbolicLink()patterns already exercised cross-platform by the neighbouring tests.
Extended reasoning...
Overview
The PR tightens one arm of Symlinker::ensure_symlink in the isolated-install linker. Previously, when readlink on a link slot failed and the slot was a real directory, the directory was unconditionally preserved (to protect a bun patch workspace). Now it is only preserved if it contains a package.json; otherwise it is removed with delete_tree and the symlink is re-created. Two new test.concurrent cases in test/cli/install/isolated-relink.test.ts cover the root-dependency slot and the store-entry nested-dependency slot.
Security risks
None. The path being deleted is one the installer itself owns (node_modules/<pkg> or a store-entry dependency slot) and only when it is a real directory lacking package.json. No user-controlled input reaches a new path-construction site; the probe path is built via the pooled Path::append, which inserts the separator and is length-checked. No network, auth, or crypto surface is touched.
Level of scrutiny
Low-to-moderate. The change is ~15 lines in a rarely-reached recovery arm (readlink failed, dest is a real non-reparse directory), so it is neither a hot path nor a syscall-count concern. I verified the RAII behaviour: save() yields a ResetScope whose Drop restores the buffer length, so self.dest is back to the directory path before delete_tree and symlink() run. Path::append computes needs_sep and inserts the platform separator, so the probe is correctly <dest>/package.json on both POSIX and Windows. The let _ = delete_tree(...) discard mirrors the existing Strategy::ExpectMissing EEXIST arm in the same function — if removal fails, the following symlink() returns the error.
Other factors
Tests follow the file's established conventions exactly (VerdaccioRegistry, test.concurrent, installOk, unlink+mkdir on a link slot, lstat().isSymbolicLink()), all of which are already exercised cross-platform by neighbouring pre-existing tests. No CODEOWNERS entry covers src/install/ or the test file. The bug hunt exited on dry_streak with no findings. The PR conversation contains no outstanding third-party objections. The one design tradeoff — deleting any package.json-less directory rather than only empty ones — is deliberate, explained in the description, and matches the hoisted linker's verify_package_json_name_and_version signal.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Instead, can we check if the package.json exists in one syscall?
|
Done in 2d13f73. When readlink fails with "not a symlink", the arm now probes |
… link The isolated linker kept any real directory found where a dependency symlink belongs, to protect a bun patch workspace. An empty directory with no package.json is not a package. Delete it and write the link.
…cy, and regular file slots
2d13f73 to
4b57000
Compare
This branch has not been deployed
3 participants
Problem
linker = "isolated", whennode_modules/<pkg>is an empty real directory instead of the symlink intonode_modules/.bun/<pkg>@<ver>/,bun installandbun install --frozen-lockfilereport(no changes)and leave it as is.node_modules/<pkg>/package.jsonnever exists. Deleting the link, or deleting the store entry, is repaired. Only the "empty directory in the link's place" case is missed.Symlinker::ensure_symlink(src/install/isolated_install/Symlinker.rs:102) keeps any real directory found at a link path. install: global virtual store for isolated linker (7x faster warm installs) #29489 added that to protect abun patch <pkg>workspace, which is a real directory at the same path. An empty directory matches the same check.Fix
readlinksays the link path is not a symlink, probe<dest>/package.jsonwith one syscall. If it exists, keep the directory (the patch workspace). If not,delete_treeremoves whatever is there (a directory or a regular file) and the link is written. The separatelstatthat classified the path is gone.bun patchworkspace is a copy of the package, so it always has apackage.json. A directory without one is not a package. The hoisted linker uses the same signal (PackageInstall::verify_package_json_name_and_version) to decide whether an install is present..bun/node_modules/<pkg>links, so those slots heal too.test/cli/install/isolated-relink.test.ts. Five fail on the current release and pass with this change (the root slot, a store entry slot, the.bun/node_modulesand scoped slots, a workspace package slot, and a new dependency whose directory exists before the install). The sixth pins that a regular file in the slot is replaced.isolated-install.test.ts,isolated-relink.test.ts, and the isolated tests inbun-install-patch.test.tspass with a debug build.Background
node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>.node_modules/<pkg>at the project root, and each dependency slot inside a store entry, is a symlink into that store.ensure_symlinkwithStrategy::ExpectExistingreads the link at the destination. Right target: nothing to do. Nothing there: create it. Anything else is in the way. Only the "a directory is in the way" arm changes here.bun patch <pkg>replacesnode_modules/<pkg>with a real directory that holds a copy of the package for the user to edit untilbun patch --commit. Install must not delete it.Supersedes #37757, which removes the directory with
rmdir(so only an empty one). This change also repairs a leftover directory that has stray contents but nopackage.json. The tests from #37757 (the.bun/node_modulesand scoped slots, a workspace package slot, and thebun bdcase where ninja createsnode_modules/<new dep>/beforebun installruns) are folded into this branch. The summary line still says(no changes)when only a root link is rewritten. That is pre-existing: root and workspace entries are always counted as skipped inInstaller::on_task_complete, also when the root link was deleted outright.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/isolated-relink.test.ts