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; 2 remain after this review. WalkthroughThe patch flow now rejects workspace, ChangesFolder target validation
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to Some folder-target patch attempts may receive guidance for the wrong parent package or suggest direct editing when the target is registry-shipped. The command still refuses the unsupported patch safely, so this is bounded but should be corrected. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The automated review is paused by a plan limit, so there are no review findings to address yet. CI is running on 459c3ad. The diff is ready for a maintainer review. |
|
On the merge risk note about the note text: the refusal itself does not depend on which package declares the folder. The note names the declaring package only when the path is not user authored (Lockfile::is_trusted_folder_dependency is false for the row), which is the case where the folder lives inside that package. A root override with the same name makes the path project relative, so 'edit directly' is right there. When two registry packages share one folder row, the note names the first of them. Both ship the folder, so the remedy holds. |
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
|
Both automated reviews report nothing further on cb0ba74 and every review thread is resolved. The one red CI lane is test/bake/deinitialization.test.ts on alpine aarch64, which also fails on main and is unrelated to bun patch. The rest of the build is green so far. |
|
Updated 9:23 PM PT - Sep 17th, 2026
✅ @robobun, your commit d6155a5543a6fd9c4d56301a3ecfee4b3919921f passed in 🧪 To try this PR locally: bunx bun-pr 43223That installs a local version of the PR into your bun-43223 --bun |
|
One overlap with #43199, for whichever PR merges second. Both PRs add a guard at the same line of the path arm in For the bundled With |
Problem
bun patch <name>on afile:dependency that a registry package ships reads the folder from the project root. With thebundled-file@2.0.0fixture,bun patch bundled-file-depexits 1 witherror: error overwriting folder in node_modules: ENOENTafter it deletes the installed folder. If the project has an unrelated folder at that path, it copies that folder over the installed package and exits 0. Fixesbun patchreads thefile:dependency of a registry package from the project root #43200.compute_cache_dir_and_subpath(src/install/PackageManager/PackageManagerDirectories.rs,Folderarm) joins every folder path onto the cwd. A registry manifest's path is stored as written (Package::from_npm), relative to the package that declares it.bun patch --commitfails for everyfile:directory and workspace member withfailed to read from cache (readlink), andbun installnever applies a patch to one (patched_package_missing_from_cachefinds the folder itself).bun patch <workspace member>also replaces thenode_modulessymlink with a copy.Fix
crash_if_folder_target(src/install/PackageManager/patchPackage.rs) runs at the three entry points (prepare_patchname and path form,do_patch_commit) before anything innode_modulesis touched. It exits 1 for aResolution::FolderorResolution::Workspacetarget:error: cannot patch <name>: it is a file:<folder> dependency, and bun install never applies a patch to a file: folder.run bun patch <dependent> and edit <folder> inside that package. For a folder in the project (root, workspace, localfile:package, root override) and for a workspace member:edit <folder> directly. The wording usesLockfile::is_trusted_folder_dependency, the same row predicate the resolver and installer use. The refusal itself does not depend on it.node_modulesintact. The remedy works end to end: a patch of the package that ships the folder applies to the folder, and the nested install links the patched copy.test/cli/install/bun-patch.test.ts, block "a folder dependency as the target" (8 tests, 7 fail on main). Alsobun-patch.test.ts(45 pass),bun-install-patch.test.ts(31 pass),cargo clippy -p bun_install,cargo check -p bun_install --target x86_64-pc-windows-msvc.Background
bun patch <pkg>copies the package's cache entry overnode_modules/<pkg>so the user can edit it.bun patch --commitdiffs the edited copy against the cache entry, andbun installapplies that diff to a cache copy before it links the package.file:directory or workspace member has no cache entry. The installer links the folder in place (a symlink or per-file symlinks), so the lockfile resolution is the folder path itself, with one of two bases: the project root for paths the project writes, the declaring package for paths a registry manifest writes.file:dependency under that package's ownnode_modules, never hoisted.Notes
Found while working on a bundled
file:dependency (#42884). Present on main and on 1.4.3-canary.1 (b52d513).Manual checks with the debug build, both linkers left at the hoisted default:
"dep": "file:./dep"and aworkspace:*member:bun patch depandbun patch wsexit 0 on main and print the edit folder.bun patch --commit node_modules/depthen fails withENOENT: No such file or directory: failed to read from cache (readlink).node_modules/wsis a real directory afterwards, no longer the workspace symlink. Both are refused now, with theedit <folder> directlynote.bun patch bundled-file, editnode_modules/bundled-file/vendor/bundled-file-dep/index.js,bun patch --commit node_modules/bundled-file,rm -rf node_modules,bun install.node_modules/bundled-file/node_modules/bundled-file-dep/index.jshas the edit. The last test in the block pins that round trip.Shape: a first version refused only the folder of a registry package, with a new lockfile predicate layered on the base directory rule that #38856, #38994 and #38816 change. A self-review found that the reason for the refusal (a patch of a folder never applies) holds for every folder target, so the change now refuses every
FolderandWorkspaceresolution and uses no new lockfile predicate. The declaring package only picks the note text. Self-reviewed: 5 concerns raised, 5 addressed.Related: #43160 stops the delete on the ENOENT path for other errors. #43199 refuses a bundled dependency at the same three call sites. #43178 keeps nested dependencies when the patched package itself is prepared. This change is independent of them in code and can merge in any order.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-patch.test.ts