Conversation
… at its hoisted path With the isolated linker a package has a folder at its hoisted path only when the root, or a workspace that the root links, depends on it. `bun patch <name>` still created the editable copy at the hoisted path. No install removed that directory. It shadowed the package for every importer above it, and a directory at `node_modules/<workspace>` kept a later install from linking the workspace there. When nothing exists at the hoisted path, `bun patch <name>` and `bun patch --commit <name>` now use the store folder of the package, `node_modules/.bun/<entry>/node_modules/<name>`. An entry that is a link into the global store is not used, because an install puts that link back over a detached copy. `bun patch --commit` also follows a symlink argument to the folder it points to, because `git diff --no-index` reads a symlink operand as a file.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 14 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
StatusReproduced on a release build of main (1.4.3-canary.1+b52d51348) with bun install
bun patch a # prints node_modules/a
echo "// patched" >> node_modules/a/index.js
bun patch --commit a
Test: PR: #43383 |
A package with peer dependencies has one store entry for each peer resolution. `bun patch <name>` took the first entry in store order. In a workspace that links another entry, the edits were not behind its `node_modules/<name>`, and `bun patch --commit node_modules/<name>` from that workspace found no changes. The search now goes breadth first from the entry of the root or of the workspace of the current directory, and falls back to store order.
|
Updated 5:00 PM PT - Sep 18th, 2026
✅ @robobun, your commit ed14bb8c2b092f0409ed1a5df43d3b82b2403cc7 passed in 🧪 To try this PR locally: bunx bun-pr 43383That installs a local version of the PR into your bun-43383 --bun |
|
Follow-up to the review, in 53f2b4e and c491b6f:
Tests: |
… the other entries when --commit finds no changes A workspace that another workspace depends on has a second store entry with no dependencies, so the search for the entry that the current workspace loads stopped there. Every entry of a workspace is now visited. `bun patch --commit <name>` names the entry that the current directory loads. When `bun patch` ran in another directory, that entry has no changes. The output now lists the other store folders of the package with the command that commits them.
|
Follow-up to the second review round, in ed14bb8:
Tests: |
There was a problem hiding this comment.
I re-reviewed the latest push (ed14bb8) and found no new bugs; given that bun patch now rebuilds the isolated store and walks its entry graph, a human look at the entry-selection logic is still worthwhile.
What was reviewed in the new commit:
- The
same_packageexpansion inenqueue(patchPackage.rs:1140-1155): aworkspace:edge now reaches the entry that carries the workspace's dependencies, so the BFS crosses workspace-to-workspace links as the newconsumertest expects. - The
other_store_foldershint (patchPackage.rs:551-563) prints root-relative paths; from a workspace cwd the process has already chdir'd to the root (PackageManager.rs:1845) and the Path guard at patchPackage.rs:131 keeps the argument, so the printed command works from either directory. build_store(isolated_install.rs:216) is a pure function of the lockfile, so calling it frombun patch/--commithas no filesystem side effects.
Extended reasoning...
Overview
The PR changes src/install/PackageManager/patchPackage.rs (about 200 lines) and adds a test block to test/cli/install/bun-patch.test.ts. Since my last review, commit ed14bb8 added the workspace-aware expansion in the store-entry walk (treating Root/Workspace resolutions as sharing all entries of the same package) and the "has more than one folder" hint printed when --commit <name> finds no changes. Both are responses to threads I opened at 22:55; the peer-variant cwd mismatch is now mitigated by the hint rather than by making --commit <name> locate the prepared folder, which is a design choice a maintainer should weigh.
Security risks
None specific to this change. The new paths are built from lockfile-derived store entry names (fmt_store_path) and package names that the lockfile already validated; resolve_symlinked_folder only follows a symlink that already sits inside the project's node_modules and falls back to the original path on any failure. build_store does not write to disk. The only new subprocess input is changes_dir, which was already passed to git diff --no-index on the base branch.
Level of scrutiny
Moderate-to-high. This is a package-manager code path with several new invariants: the BFS from the current workspace entry must agree with what the installer linked, the global-store exclusion (RealDir check on node_modules/.bun/<entry>) must not select a link into the shared cache, and the symlink resolution must happen before git diff sees the operand. The #[cfg(windows)] branch of folder_kind and the Windows get_fd_path result are not type-checked or exercised in this Linux run; the author reports a Windows debug build passing the new block. I did not build or run the tests in this run.
Other factors
The new tests cover the transitive, workspace-only, nested-under-dependent, peer-variant (from root, from each workspace, through another workspace), cross-directory commit, and global-store shapes, and the three existing tests that asserted the old hoisted copy were updated with stated reasons. Two of my earlier inline threads remain open (the cwd-dependent entry choice, now mitigated by a hint, and the --production peer-hash naming note); I did not restate them here. No third-party CHANGES_REQUESTED review is visible in the timeline, but the change is too large and layered for me to approve without a human maintainer's look.
Problem
linker = "isolated",bun patch <name>puts the copy at the hoisted path of the package. The isolated layout can have nothing there. The root depends onb,bdepends ona:bun patch acreates a realnode_modules/athat stays after--commitand after every install.node_modules/<workspace>/node_modules/<name>later stops the install from linking that workspace (error: Cannot find package 'w'). A hoisted path through a link puts the copy inside another store entry.prepare_patchanddo_patch_commit(src/install/PackageManager/patchPackage.rs) take the folder from the hoisted tree.Fix
node_modules/.bun/<entry>/node_modules/<name>.build_storenames the entry, and the entry that the current workspace links wins. The install after--commitrebuilds it in place, and dependents load the edits before the commit.bun patch --commitfollows a symlink argument.git diff --no-indexreads a symlink as a file and wrotenew file mode 120000.test/cli/install/bun-patch.test.ts(9 new tests, 3 updated, 11 fail without the fix). Alsobun-install-patch.test.ts,isolated-install.test.ts.Background
bun patch <pkg>replaces the package folder with a copy that shares no files with the cache.--commitdiffs the copy against the cache and installs again.Notes
Repro output (loopback registry, root depends on
b,bdepends ona,linker = "isolated").Before (1.4.3-canary.1+b52d51348):
After:
The three shapes that the new tests cover:
node_modules/<name>.node_modules/<workspace>/node_modules/<name>. After the root adds"w": "workspace:*",node_modules/wmust be a link.Symlinker::ensure_symlinkkeeps a real directory it finds there, so the leftover brokerequire("w").node_modules/<dependent>/node_modules/<name>.node_modules/<dependent>is a link, so the copy landed inside the store entry of the dependent and shadowed the real package for it.Why the store folder:
bresolvesathrough its own link.bun patch node_modules/.bun/a@1.0.0/node_modules/aand--commitwith the same path behave correctly on the release build: the install finds no.bun-tag-<hash>of the new patch and rebuilds the folder.Peer variants: a package with peer dependencies has one store entry for each peer resolution (
peer-deps@1.0.0+<hash>).build_storeis the function thatbun installandbun pruneuse to name the entries. The search goes breadth first from the entry of the root, or of the workspace of the current directory, sobun patchin a workspace prepares the entry that this workspace loads. Itsnode_modules/<name>link then leads to the edits, for its own code and for--commit node_modules/<name>. The walk visits every entry of a workspace package, because the entry that aworkspace:range links has no dependencies of its own. An entry that the walk does not reach is found in store order. The install after--commitpatches every variant.--commit <name>uses the same rule, so it names the prepared folder when both commands run in the same directory. Whenbun patchran in another directory, the named entry has no changes. The output then lists the other store folders of the package with the command that commits each one. The command thatbun patchprints works from every directory. I did not make--commit <name>search for the entry that differs from the cache: nothing marks a folder as prepared, an entry that a lifecycle script changed also differs, and for an already patched package every entry differs from the unpatched cache.Global store (
install.globalStore = true, off by default):node_modules/.bun/<entry>is a link into<cache>/links.bun patchon a store folder replaces that link with a real directory. The next install sees a real directory where the link belongs and replaces it (link_project_to_global_store), so edits made before--commitare lost. This happens on the release build with an explicit store path. To not make it reachable by name, the store folder is used only when the entry is a real directory. With the global store the copy is still made at the hoisted path. One test pins that the edits survive an install there.Symlink argument: on the release build,
bun patch --commit node_modules/bfrompackages/w, where that path is the link to the store folder, writesand the install fails with
failed to parse patchfile: bad_file_mode. The existing test "relative node_modules/" (#12200, #12882) passed only because the copy was at the rootnode_modules/is-odd. That path no longer exists, so the command resolves the link.Existing tests that changed: "inside workspace with hoisting > @types/ws@8.5.4" expected
node_modules/@types/ws, a folder that onlybun patchcreated, and read the leftover copy after--commit. The two "committing with the path bun suggested" tests wrote to a fixed root path. They now write to the path thatbun patchprints, and assert that the root folder does not exist.Related work:
do_patch_committhat refuses a folder that is a link. After this PR a link can lead to the folder thatbun patchprepared, so if both land the guard must check the resolved folder. The resolved folder never reachesgit diffas a link, which is what the guard protects.node_modules/w, but not the copy.Not in this PR:
bun prunereports it..bun-tag-<hash>file gets into the new patch. The release build does the same with the hoisted linker (bun patch --commitcausing patch header to grow on each run #19327).Review follow-up (two rounds of automated review, every thread has an answer):
bun patchin a workspace could prepare the peer variant of another workspace (first entry in store order). 53f2b4e fixes it with the breadth first search above, and two tests cover it. ed14bb8 makes the walk continue through a workspace that another workspace depends on, and adds the list of other folders to theNo changes detectedoutput. Two more tests cover these.bun patch <name>still writes throughnode_modules/<dependent>into<cache>/linkswhen the hoisted tree nests the package under a dependent. Main does the same (checked withis-odd@3.0.1andis-number@7.0.0: a second project that shares the cache loads the edited copy). The fix needs the installer to keep a detachednode_modules/.bun/<entry>, so it is a separate change.Tests run:
bun-patch.test.ts46 pass on Linux (debug, ASAN), and the new block plus "workspace interactions" pass on a Windows debug build.bun-install-patch.test.ts31 pass,isolated-install.test.ts85 pass.