install: prune stale workspace node_modules in hoisted installs - #29794
install: prune stale workspace node_modules in hoisted installs#29794robobun wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a pre-install prune step for hoisted installs that, using the unfiltered lockfile tree captured before filtering, computes expected direct dependency folder names per workspace and removes stale top-level and Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fixes in 6e28d84 look correct — keying expected_by_ws_path by the workspace filesystem path and building it from original_trees resolves both issues I flagged, and the new "preserves non-hoistable" test is good coverage. Given this adds unconditional deleteTree pruning to every hoisted workspace install, I'd still like a human to sign off.
Extended reasoning...
Overview
This PR adds a pre-install pruning pass to installHoistedPackages (src/install/hoisted_install.zig) that walks each workspace's node_modules/ and recursively deletes any top-level entry (and one level into @scope/) that the unfiltered hoisted tree does not place there. It fixes #29793, where stale workspace-local packages shadow hoisted copies during module resolution. ~225 lines of new Zig plus two regression tests in test/cli/install/bun-update.test.ts.
Prior review and fixes
My first-pass review found two data-loss bugs in the original revision: (1) the expected-set map was keyed by tree-iterator paths (node_modules/<pkg-name>/node_modules) while lookups used filesystem paths (packages/<dir>/node_modules), so lookups always missed and every workspace's node_modules was wiped; and (2) the expected set was built from the post-filter() trees, so --filter and the security-scanner pre-install would nuke excluded workspaces. Commit 6e28d84 fixes both by keying the map via workspace_fs_path_by_pkg_id (resolved from pkg_resolutions[pkg_id].tag == .workspace) and by passing original_trees/original_tree_dep_ids (captured before filter(), populated by cleanWithLogger → resolve() → hoist(.resolvable)). A second test now asserts that a legitimately non-hoistable workspace-local package survives a re-install. I verified workspace_paths are stored with forward slashes across all writers (migration.zig:403 asserts no backslashes), so the Windows-only posix normalization on the map key being absent on the lookup key is harmless in practice.
Security risks
None in the conventional sense — no auth, network input, or untrusted data parsing. The risk profile here is destructive filesystem operations: deleteTree runs against user project directories on every hoisted install with workspaces, and the first revision would have silently destroyed legitimate nested packages. The corrected logic looks sound, but errors in the expected-set computation translate directly into deleted user files with no recovery path (catch {} swallows everything).
Level of scrutiny
High. hoisted_install.zig is the core package install path for the default linker; this code runs on every bun install/bun update in monorepos. The change is non-trivial new logic (not a mechanical fix), the initial revision contained two distinct data-loss bugs, and edge cases like nested workspaces, Windows path handling, trailing-slash normalization asymmetry between map-key and lookup-key, and the expected = null fallthrough semantics all merit a maintainer's eye.
Other factors
The two new tests cover the headline bug and the over-prune regression, but there is no test for --filter interaction, scoped-package pruning, or the security-scanner packages_to_install path. CI build #48382 is still in progress. Given the blast radius and that I already caught two destructive bugs here, I'm deferring rather than approving.
511d1ad to
2e1008f
Compare
There was a problem hiding this comment.
All prior feedback is addressed and I didn't find new issues, but this adds unconditional deleteTree logic to every hoisted workspace install — given the earlier revisions had data-loss edge cases and the failure mode is silent deletion, it's worth a human pass over the final expected == null semantics and the security-scanner / packages_to_install entry point.
Extended reasoning...
Overview
Adds a pre-install prune step to src/install/hoisted_install.zig (~230 new lines: pruneStaleWorkspaceNodeModules, workspaceNodeModulesKey, pruneNodeModulesAt, pruneScopedNodeModules) plus four regression tests in test/cli/install/bun-update.test.ts. The prune walks every workspace's node_modules/ and @scope/* and deleteTrees any entry not present in the unfiltered lockfile tree for that workspace. Runs unconditionally whenever workspace_paths.count() > 0, i.e. on every hoisted install in a monorepo.
Security risks
No injection/auth/crypto surface. The risk profile here is availability/data-loss: a logic error in key construction or tree-to-workspace mapping silently deletes legitimate package directories. Two such bugs already existed in earlier revisions of this PR (tree-iterator-path vs filesystem-path key mismatch; filtered-tree vs unfiltered-workspace-list mismatch wiping excluded workspaces and the security-scanner pre-install nuking everything). Both are fixed and now covered by tests, but the expected == null → delete every non-dotfile entry fallthrough remains the load-bearing behavior for the original bug, so any future divergence in workspaceNodeModulesKey or workspace_fs_path_by_pkg_id routes straight to mass deletion.
Level of scrutiny
High. This is production-critical install-path code that performs irreversible filesystem deletions on user projects, and it executes on every bun install/bun update in a hoisted workspace setup — not an opt-in flag. The logic depends on subtle lockfile invariants (tree dependency_id → resolution → .workspace tag → workspace_paths name-hash lookup → filesystem path) that aren't obvious from local reading. The PR history demonstrates the subtlety: the first two revisions would have caused data loss in common scenarios (--filter, security-scanner pre-install).
Other factors
- All three of my prior inline comments are resolved; the author factored out
workspaceNodeModulesKeyso map insert and lookup are structurally identical, switched tooriginal_treesfor filter-safety, and moved tobun.FD.deleteTree. - Test coverage is good: stale removal, non-hoistable preservation,
--filterpreservation, scoped-package pruning. - No CODEOWNERS for
src/install/. - Remaining things a human reviewer may want to sanity-check: behavior when
getWorkspacePackageIDreturns 0 for a workspace that does exist (it skips the map insert but the workspace is still walked withexpected = null); whether the security-scanner pre-install'soriginal_treesis always the full unfiltered set at that call site; and whether deleting entries while iterating the directory is safe across all platforms'DirIteratorimplementations.
There was a problem hiding this comment.
All my earlier feedback has been addressed and the bug hunter found nothing new, but this adds ~250 lines of recursive-delete logic to the hot path of every hoisted workspace install — given the blast radius (and the asan unreachable hit in bun-install.test.ts on the latest CI run), I'd like a human to sign off on the approach before it lands.
Extended reasoning...
Overview
The PR adds a pre-install prune step to src/install/hoisted_install.zig (~250 new lines across four functions: pruneStaleWorkspaceNodeModules, workspaceNodeModulesKey, pruneNodeModulesAt, pruneScopedNodeModules) plus four regression tests in test/cli/install/bun-update.test.ts. On every hoisted install that has workspaces, it walks each workspace's node_modules/ (and one level into @scope/) and deleteTrees any entry the unfiltered lockfile tree does not place there.
Security risks
No injection/auth/crypto surface. The risk profile here is data loss: recursive deletion of directories under user-controlled workspace paths, driven by lockfile state. The first two revisions of this PR had exactly that failure mode (key-namespace mismatch and --filter interaction both caused legitimate packages to be wiped); both were fixed after review and now have dedicated tests. Paths are derived from lockfile.workspace_paths (already posix-normalized at every write site) and opened relative to bun.FD.cwd(), so there's no obvious traversal vector beyond what the lockfile already controls.
Level of scrutiny
High. This runs unconditionally on every bun install/bun update in a workspace monorepo with the hoisted linker, and its failure mode is silent recursive deletion. The semantic choice — "delete anything in a workspace's node_modules that the current lockfile doesn't place there" — also removes packages a user manually dropped in, or artifacts left by another package manager. That may well be the right call (it matches the issue's intent), but it's a behavioral policy decision a maintainer should ratify.
Other factors
- Review history: three rounds of my own inline feedback (two 🔴 correctness bugs, one 🟡 key-normalization nit, one 🟡
.stdDir()nit) have all been addressed and resolved; subsequent commits (c98d98e,2e1008f,0b4b117) further harden againstgetWorkspacePackageIDfailure and mid-iteration deletes. - CI: the most recent robobun status (commit
c98d98e) reports musl build failures andpid 4162 reached unreachable codeintest/cli/install/bun-install.test.tson x64-asan — worth confirming whether that's related to this change before merging. - Callers:
installHoistedPackagesis invoked from bothinstall_with_manager.zigandsecurity_scanner.zig; the original--filter/packages_to_installover-prune was specifically dangerous for the security-scanner path, and while it's now driven offoriginal_trees, a maintainer eye on that interaction would be valuable. - Test coverage: good — the four new tests cover the happy path, non-hoistable preservation,
--filterexclusion, and scoped-package pruning.
There was a problem hiding this comment.
Earlier feedback is all addressed and I didn't find new issues, but this adds ~270 lines of destructive deleteTree logic to the hot bun install path for every monorepo — given the first revision had two bugs that wiped legitimate workspace node_modules, the design choice to prune even --filter-excluded workspaces, and the asan/musl CI failures on c98d98e, this one really wants human eyes before merge.
Extended reasoning...
Overview
This PR adds a pre-install prune step to src/install/hoisted_install.zig (~270 new lines) that walks every workspace's node_modules/ (and one level into @scope/) and recursively deletes any top-level entry the unfiltered lockfile tree doesn't place there. It also adds four regression tests in test/cli/install/bun-update.test.ts covering the original shadowing bug, preservation of legitimately non-hoistable nested packages, --filter interaction, and scoped-package pruning, plus a setDefaultTimeout bump.
Security risks
No auth/crypto/network surface. The risk is purely destructive filesystem behavior: deleteTree runs against paths derived from lockfile.workspace_paths relative to cwd. Paths come from the lockfile (not arbitrary user input mid-request), dotfile entries are preserved, and the walk is bounded to <ws>/node_modules top-level + @scope/*. I don't see an injection or traversal vector, but a logic error here translates directly into silently deleting user files — which is exactly what the first revision of this PR did under --filter and the security-scanner pre-install before it was fixed.
Level of scrutiny
High. This runs unconditionally on every hoisted bun install/bun update in any repo with workspaces, and the failure mode is silent data loss (errors are swallowed with catch {}). The initial revision shipped two separate bugs that each would have wiped entire workspace node_modules directories; both were caught in review and fixed, but that history argues for a careful human pass on the final logic rather than a bot sign-off.
Other factors
- All three of my prior inline comments (two 🔴 destructive-prune bugs, one 🟡
bun.FD.deleteTreeconvention nit) have been addressed and resolved in subsequent commits. - There's a non-trivial design decision baked in: the prune walks the unfiltered tree, so a
bun install --filter Xwill still delete stale entries in workspaces the user explicitly excluded. The test asserts this is intentional, but it's the kind of UX/semantics call a maintainer should ratify. - The robobun CI comment shows musl build failures and an x64-asan "reached unreachable code" in
bun-install.test.tsfor commit c98d98e; several hardening commits landed after, but I can't confirm CI is green on the current head. - Test coverage for the new behavior is solid (four targeted cases including the over-prune guards), which raises confidence but doesn't substitute for a human review of code that does recursive deletes in the install hot path.
There was a problem hiding this comment.
All my earlier feedback is addressed and I didn't find anything new in this revision, but since this adds an unconditional deleteTree sweep over every workspace's node_modules on every hoisted install, it's worth a human sign-off on the pruning semantics — specifically that wiping anything in <ws>/node_modules not placed by the current lockfile (e.g. npm linked packages, manually-dropped folders) is the intended product behavior.
Extended reasoning...
Overview
Adds ~270 lines to src/install/hoisted_install.zig introducing pruneStaleWorkspaceNodeModules and helpers, called unconditionally from installHoistedPackages whenever workspace_paths.count() > 0. For each workspace it computes the set of folder names the unfiltered lockfile tree places directly in <ws>/node_modules, then walks the on-disk directory (one level, plus one level into @scope/) and deleteTrees every non-dotfile entry not in that set. Four new regression tests in bun-update.test.ts cover the original shadowing bug, preservation of legitimately non-hoistable nested packages, --filter safety, and scoped-package pruning.
Security risks
No injection/auth/data-exposure surface. The risk profile here is data loss: this is destructive filesystem code (deleteTree) running on every hoisted workspace install, driven by lockfile-derived state. The first revision of this PR contained two separate data-loss bugs (path-key namespace mismatch causing every workspace node_modules to be wiped; --filter wiping excluded workspaces) that I flagged and the author fixed. The current revision looks correct — keys are derived through a single workspaceNodeModulesKey helper, the expected set is built from pre-filter original_trees, and workspaces whose getWorkspacePackageID lookup fails are skipped rather than walked with an empty expected set.
Level of scrutiny
High. installHoistedPackages is the hot path for every bun install/bun update in a hoisted-linker monorepo, and the new code unconditionally deletes directories based on a computed expected set. Beyond correctness, there is an embedded product decision: anything a user has placed in packages/<ws>/node_modules that the lockfile doesn't account for (an npm link/bun linked package, a manually-vendored folder, output from another tool) will now be silently removed on the next install. That may well be the right call, but it's a behavior change a human owner of bun install should ratify rather than a bot.
Other factors
- All three of my prior review rounds are resolved; the author responded to each with targeted fixes (key unification,
original_trees, sharedworkspaceNodeModulesKey,bun.FD.deleteTree). - Test coverage is good for the cases that broke earlier revisions, though there's no test asserting that an
npm link-style symlink in a workspacenode_modulesis (or isn't) preserved — which is the main product-behavior question. - Errors are swallowed (
catch {}) by design so prune failures never block installs; that's reasonable but means edge-case misbehavior would be silent. src/install/is core package-manager code; CODEOWNERS-level scrutiny is appropriate.
f2159db to
5f91995
Compare
There was a problem hiding this comment.
All earlier feedback is addressed and carried into the Rust port, and I didn't find new issues — but this adds ~250 lines that recursively delete directories inside every workspace's node_modules on every hoisted install, reimplemented from scratch after the Zig→Rust rebase, so it's worth a human pass over the deletion logic (especially the expected == None → wipe-everything path and the cwd-relative open on Windows).
Extended reasoning...
Overview
The PR adds a pre-install sweep to src/install/hoisted_install.rs that, for each workspace, opens <workspace>/node_modules, computes the set of folder names the unfiltered lockfile tree places there, and delete_trees anything else (including one level into @scope/ dirs). Four regression tests in bun-update.test.ts cover the original shadowing bug, preservation of legitimately non-hoistable nested packages, --filter safety, and scoped-package pruning. Since my last review the entire implementation moved from hoisted_install.zig to hoisted_install.rs following the package-manager port on main; the logic was rewritten rather than mechanically translated.
Security risks
No injection/auth/crypto surface. The risk profile is data loss: this code runs unconditionally on every hoisted install in a workspace repo and recursively deletes directories under user-controlled paths. The first two iterations of this PR had exactly that failure mode (wiping legitimate nested packages, wiping --filter-excluded workspaces). The current revision fixes those — workspace_node_modules_key() is a single shared helper, the expected set is built from original_trees captured before filter(), and unresolvable workspaces are skipped — but the expected == None branch still means "delete every non-dotfile entry," so any future divergence between the map key and lookup key (or a workspace whose tree node is absent for an unanticipated reason) degrades to a full wipe.
Level of scrutiny
High. This is core bun install behavior touching the filesystem destructively, with errors deliberately swallowed (best-effort), so a wrong deletion produces no diagnostic. The Rust reimplementation is new code that no human has reviewed yet — only the Zig version went through review cycles. Specific spots worth a human eye: the cfg!(windows) separator normalization being applied to a path that's then passed to open_dir_for_iteration(cwd, …); the linear fs_path_for scan inside the tree loop (fine for typical workspace counts but O(workspaces × trees)); and Dir::from_fd ownership vs. the borrowed parent_dir: Fd passed into prune_scoped_node_modules.
Other factors
Test coverage is solid and directly exercises the previously-broken cases. All four of my earlier inline comments are resolved and the fixes survived the port. No outstanding reviewer comments. Given the destructive nature, the fresh-rewrite-after-rebase, and this PR's own history of data-loss bugs in earlier revisions, deferring to a human is the safer call.
|
CI is green on everything my diff touches. On build #59756 all 224 non-darwin This needs a maintainer to re-run the expired darwin lanes / merge — I don't want to spam more |
|
Status at head f693a65 (build #97058): the only CI red is Everything this diff touches is green: All automated review findings are addressed (declared-deps-only pruning, filtered/scanner gates, symlinked |
7cef654 to
3a9c810
Compare
f155403 to
4d9d54b
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/install/hoisted_install.rs:885-893— nit:sys::rmdirat(parent_dir, "@scope")runs whilescope_dir(the owningDirhandle for that same directory) is still live — it doesn't drop until the closing brace two lines later.Dir::delete_treedocuments the opposite ordering as a Windows requirement (src/sys/dir.rs:188-196: "On Windows we must close before removing") and explicitly closes the fd beforeunlinkat_a(..., AT_REMOVEDIR).FILE_SHARE_DELETEmakes it work today (Windows CI passed, and the scoped-prune test would catch a hard failure), so this is a convention match rather than a demonstrated bug: addingdrop(scope_dir);before theif !has_remainingblock mirrors dir.rs:196.Extended reasoning...
What
prune_scoped_node_modulesopensscope_dir(an owningDirfor the@scopedirectory) at the top of the function, then — insideif !has_remaining { ... }— callssys::rmdirat(parent_dir, z)to remove that same directory whilescope_diris still live. TheDirhandle (and theitercreated fromscope_dir.fd()) don't drop until the function's closing brace, one line after thermdirat.The codebase's own recursive-delete,
Dir::delete_tree, follows the opposite ordering and documents why. At src/sys/dir.rs:188-196:// Reached the end of the directory entries — exhausted; remove the // directory itself. On Windows we must close before removing. let dir_fd = top.iter.dir(); ... stack.pop(); let _ = close(dir_fd); ... unlinkat_a(parent_dir, &name, AT_REMOVEDIR) ...
Per REVIEW.md: "If neighboring code does something differently than you're about to, find out why." This is the same operation ("remove a directory I've been iterating") with an explicitly commented ordering requirement in the in-tree helper.
Step-by-step
Take the scoped-prune test's setup:
packages/backend/node_modules/@stale/pkg/exists, nothing else under@stale/.sys::open_dir_for_iteration(parent_dir, b"@stale")→scope_dir: Dirowns an open handle to@stale.- Iteration finds
pkg; it's not inexpected, soscope_dir.delete_tree(b"pkg")removes it.has_remainingstaysfalse. if !has_remaining→sys::rmdirat(parent_dir, "@stale\0")runs.scope_diris still open on@stale.- On POSIX this is fine — you can
rmdira directory with an open fd on it. On Windows, whether this succeeds depends on the sharing mode and the delete semantics in effect. }—scope_dirdrops, closing the handle.
Why this works today (and is therefore a nit)
bun_sysopens directory handles withFILE_SHARE_DELETE(part of theFILE_SHAREconstant at src/sys/lib.rs:6432-6434), and modern Windows honors POSIX-delete semantics forFILE_DISPOSITION_INFORMATION_EX, sormdiraton a directory with an open share-delete handle succeeds. The scoped-prune test assertsexpect(exists("@stale")).toBe(false), which would fail on Windows if thermdiratwere rejected — and per the PR timeline Windows CI passed. So there's no demonstrated failure here; this is a divergence from an explicitly documented in-tree convention for the identical operation.Impact if it ever did fail
Purely cosmetic.
let _ = sys::rmdirat(...)swallows the error,scope_dirdrops on the very next line, and the now-empty@stale/directory is left behind — the exact outcome the block exists to prevent, but not a correctness issue (module resolution walks past empty scope dirs).Fix
One line, mirroring dir.rs:196 exactly:
} drop(scope_dir); // Remove the scope directory if it ended up empty. if !has_remaining {
(
iterwas created fromscope_dir.fd()and is also still live at thermdirat; droppingscope_dircloses the underlying fd, but for symmetry with the snapshot-then-delete pattern inprune_node_modules_atyou coulddrop(iter)first as well.) -
🟡
src/install/hoisted_install.rs:118-134— nit: this 17-line block restates the doc-comment onprune_stale_workspace_node_modules(lines 641–658) almost verbatim — both explain the shadowing problem, whyoriginal_treesis used, whypackages_to_installis gated, and both cite #29793. Per REVIEW.md ("Only comment what the code cannot say. One line. … Prefer links to GitHub issues"), the call site can collapse to something like// Sweep stale workspace node_modules before the install loop (#29793).and let the function's doc-comment carry the detail. The comment-cop bot has flagged 10 similar multi-paragraph blocks in this diff (lines 134, 659, 679, 704, 717, 771, 782, 795, 799, 809, 828); most narrate what the code already says and can be trimmed the same way.Extended reasoning...
What
The github-actions comment-cop check posted 10 unresolved inline comments on
src/install/hoisted_install.rs(lines 134, 659, 679, 704, 717, 771, 782, 795, 799, 809, 828), each quoting CLAUDE.md rule #13: "If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code." This corresponds to REVIEW.md's explicit rule under Code style & idioms reviewers enforce:Only comment what the code cannot say. One line. Never restate what the code does. Never narrate the change. Prefer links to GitHub issues.
None of the flagged blocks are
SAFETYcomments (which are required and exempt) — they're explanatory prose.The clearest instance: call-site vs. doc-comment duplication
The 17-line block at lines 118–134 preceding the
prune_stale_workspace_node_modules(...)call and the 18-line///doc-comment on the function itself (lines 641–658) say the same thing twice:Fact Call-site comment (118–134) Doc-comment (641–658) Stale workspace-local copies shadow hoisted ones "those directories would shadow the hoisted copies during module resolution" "the leftover workspace-local copy shadows the hoisted one during module resolution" Tree iterator never visits empty workspace node_modules"The installer only visits trees that still have dependencies, so … stale entries are never seen" "The tree iterator only visits node_modulesdirectories that still contain entries, so those stale folders are otherwise never seen"original_treesis unfiltered so--filterdoesn't wipe excluded workspaces"We build the expected set from the unfiltered original_treeson purpose: with--filter…"" trees/tree_dep_idsare the unfiltered buffers captured beforeLockfile::filter()runs, so entries a--filter-excluded workspace legitimately owns stay put"Issue reference "(Issue #29793.)" "issue #29793" The only content unique to the call site is the
packages_to_install.is_none()gate rationale — and that's already self-evident from the condition plus a one-line note.Step-by-step: how each flagged block violates the rule
- Lines 118–134 (call site): duplicates the function doc-comment; narrates the change ("Remove stale packages … before the install loop runs"). → Collapse to one line + issue link; the function name and doc already say all of it.
- Lines 641–658 (function doc): 4 paragraphs. Trim to what the signature can't express — the why (shadowing) and the contract (
treesmust be unfiltered). Two–three lines suffice. - Lines ~674–679 ("Resolve every workspace to (package id, filesystem-relative path). Only workspaces that resolve cleanly are walked …"): restates what the loop body does. The
if pkg_id == 0 { continue; }line is self-explanatory. - Lines ~700–704 ("Map of workspace filesystem-relative
node_modulespath …"): restates the type declaration and its use. - Lines ~715–717 ("Only trees whose
dependency_idresolves to a workspace package …"): restates theTag::Workspaceguard four lines below. - Lines ~767–771 (
workspace_node_modules_keydoc, 4 lines): the second half narrates a hypothetical failure mode; "single source of truth for the map key" is enough. - Lines ~780–782 / 795–800 / 807–809 / 825–828 in
prune_node_modules_at/prune_scoped_node_modules: each is a multi-line paragraph narrating what the next few lines do ("Snapshot the directory listing before deleting anything …", "Scoped package directory. Recurse one level …"). Per REVIEW.md these should be one line or nothing — the readdir-snapshot rationale is the only one that carries non-obvious information ("batch refill after a delete can re-surface entries"), and even that fits on one line.
Why existing code doesn't prevent it
The comment-cop check is an automated GitHub Action that flags multi-paragraph comment blocks in the diff; it doesn't auto-fix. The 10 unresolved bot comments are still open on the PR (posted 2026-08-13T21:59Z), so this is currently a live check the author needs to address.
Impact
Style/maintainability only — no runtime effect. The code works correctly; the concern is (a) the same explanation living in two places will drift, (b) narration-heavy comments bury the one non-obvious fact per block, and (c) it violates a repo-enforced convention that comment-cop is actively flagging. This is a nit, not a correctness defect.
Fix
Keep one canonical explanation on the function doc-comment (trimmed), and reduce the call site to:
// Sweep stale workspace node_modules before installing (#29793); skipped on the // scanner's narrowed pre-install pass — the full install that follows does it. if packages_to_install.is_none() && this.lockfile.workspace_paths.count() > 0 { prune_stale_workspace_node_modules(&this.lockfile, &original_trees, &original_tree_dep_ids); }
For the other flagged blocks: delete the ones that restate the code (679, 704, 717, 809, 828), and cut the rest to a single line stating only the non-obvious invariant (e.g. line 795 →
// Snapshot before deleting: batch readdir refill after unlink can re-surface entries.).
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Because the change adds a destructive pre-install pass that recursively deletes entries from every workspace's node_modules/ based on a lockfile-derived expected set, a human look would still be worthwhile — the failure mode of a wrong expected set is silent data loss.
Checked: the map key and lookup key now go through the same workspace_node_modules_key helper; the prune uses the unfiltered original_trees so --filter doesn't wipe excluded workspaces (covered by test); packages_to_install.is_none() gates out the security-scanner pre-install; dotfiles (.bin, .cache) are skipped; pkg_id == 0 workspaces are excluded so a stale-lockfile miss can't wipe a directory with an empty expected set.
Confirmed Lockfile::filter() only rewrites buffers.trees/hoisted_dependencies — the dependencies/string_bytes/packages buffers the prune indexes via original_trees are unchanged, so the pre-filter indices stay valid.
Extended reasoning...
Overview
Adds ~230 lines to src/install/hoisted_install.rs implementing prune_stale_workspace_node_modules and helpers, invoked once before the hoisted install loop. For each workspace it opens <ws>/node_modules/, builds an expected-folder-name set from the unfiltered lockfile tree, and delete_trees any top-level entry (plus one level into @scope/) not in that set. Four tests are added to test/cli/install/bun-update.test.ts covering the repro, a non-hoistable-preservation guard, --filter interaction, and scoped-package cleanup.
Security risks
The new code recursively deletes directories under user-controlled workspace paths. Paths come from lockfile.workspace_paths (already posix-normalized at every write site) and are opened relative to Fd::cwd(); no user-supplied string reaches delete_tree beyond directory-entry names read from the fd. The main risk is over-deletion if the expected-set construction misses a legitimately-placed entry (e.g. an edge in tree/resolution tag handling), which would silently remove packages the lockfile actually expects. No injection/auth surface.
Level of scrutiny
High. This is a behavioral change to bun install/bun update on every hoisted monorepo, and the operation is irreversible rm -rf on user directories. Correctness hinges on lockfile-tree invariants (a workspace with anything placed in its node_modules always has a matching Tree node whose dependency_id resolves to a Tag::Workspace package present in workspace_paths), on filter() not perturbing the buffers the pre-filter tree indices point into, and on dependency name matching the on-disk folder name for aliased deps. These held up under my reading, but they're the kind of invariants a package-manager maintainer should sign off on.
Other factors
The PR has been through 17 iterations; my four prior nits (key-normalization asymmetry, stdDir().deleteTree, security-scanner double-prune, unread stdout pipes) were all addressed and are resolved. The comment-cop bot's flags were addressed in 5fb1bca. Tests use the local dummy registry, drain stderr, and 3 of 4 fail on base per the gate evidence. The setDefaultTimeout(5min) addition is broad but matches other install test files. No outstanding human review comments.
Closes #29793. A previous package-local install (or a manual edit) can leave `packages/<workspace>/node_modules/<pkg>` behind. When the current hoisted install hoists `<pkg>` to the root, the leftover workspace-local copy shadows the hoisted one during module resolution, so `bun update --latest <pkg> --linker hoisted` reports success while the workspace keeps resolving the stale version. The hoisted installer drives off the lockfile tree, which only lists `node_modules` directories that still hold dependencies. When a workspace's deps all hoist to the root, its own tree is dropped and the installer never visits `packages/<workspace>/node_modules`, so the stale entry is never removed (isolated installs rebuild each workspace's `node_modules` from scratch and sidestep this). Before the install loop runs, walk each workspace's `node_modules/` (plus one level into `@scope/`) and delete any entry the tree layout doesn't place there. The expected set is built from the unfiltered tree buffers captured before `Lockfile::filter()`, so `--filter`-excluded workspaces keep the packages they legitimately own: only genuinely stale entries (not placed anywhere by the lockfile) are removed. The prune is skipped on the security scanner's narrowed pre-install pass (`packages_to_install`); the full install that follows performs it. Tests: stale removal + hoist, non-hoistable workspace-local copy preserved across re-install, `--filter` preservation of excluded workspaces, and scoped-package (`@scope/pkg`) pruning with empty-scope cleanup.
--filter installs leave unselected workspaces' node_modules alone (bun prune handles those), matching the semantics the pnpm-parity change established. Adapt the collapsed-row test: an unfiltered install now prunes the stale nested copy immediately, so re-plant it before exercising the filtered update's removal.
5fe9ad9 to
735d5c5
Compare
|
Rebased onto current main (735d5c5). This one was more than a textual conflict: #38333 (pnpm parity) landed tests that codify filtered-install semantics, so the prune is now gated to unfiltered installs (--filter installs leave unselected workspaces' node_modules alone; bun prune covers those). #38333 also fixed the update path for the basic shadowing case, so the install-path tests here are what still fail on base (3 of them). Its collapsed-row test was adapted: an unfiltered install now prunes the stale nested copy immediately, so the test re-plants it before exercising the filtered update's removal. Full file passes 160/160 locally; details in the PR description's rebase note. |
A workspace entry is stale only when its name is a dependency the workspace declares and the tree resolves elsewhere; that is the shadowing case from #29793. Entries the lockfile never placed (a manual bun link, a hand-dropped folder) survive installs, matching the dedupe and add --filter tests that codify that policy.
Reuse prune.rs's open_real_subdir (lstat + O_NOFOLLOW) so the scoped descent cannot delete through a symlink outside the workspace's node_modules, matching the guard bun prune uses for the same walk.
A previously nested dep with bins leaves links in the workspace's node_modules/.bin when the prune removes it; reuse prune_bins (now pub(crate)) the way remove_collapsed_copies' housekeeping does.
There was a problem hiding this comment.
I reviewed the latest revision (f693a65) and found no new issues — the prune_bins sweep addresses my last note, and the earlier open_real_subdir switch covers the symlinked-@scope descent. Because this adds a recursive-delete pass to every unfiltered hoisted install and rewrites a #38333 test assertion, a human sign-off on the prune policy (declared-deps-only, pre-filter tree, --filter/scanner gates) would still be worthwhile.
What was reviewed:
- Prunable-set construction — only names the workspace declares, minus names the tree places locally; undeclared entries and dotfiles untouched.
- Symlink handling — scoped descent now refuses symlinked
@scope; unscopeddelete_treeacts on the entry name so a symlink is unlinked, not followed. .binsweep gated ondeleted_any; both#[cfg]branches ofprune_binsmadepub(crate).- The Resolvable-vs-Filter tree divergence under
--production/--omitis over-conservative only (never over-deletes), per the earlier thread.
Extended reasoning...
Overview
Adds a pre-install prune pass to install_hoisted_packages (~260 new lines in src/install/hoisted_install.rs) that walks each workspace's node_modules/ and @scope/*, deleting entries whose name the workspace declares as a dependency but the hoisted tree places elsewhere. Makes open_real_subdir and prune_bins pub(crate) in src/install/prune.rs for reuse. Adds four tests and adapts one #38333 test in bun-update.test.ts.
Security risks
The operation is a recursive delete inside user-controlled node_modules trees. The scoped-descent symlink-escape I flagged earlier is now guarded by open_real_subdir (lstat + O_NOFOLLOW). The unscoped path calls delete_tree on the entry name relative to the real node_modules fd, which unlinks a symlink rather than following it. Paths derive from the lockfile's workspace_paths (repo-local package.json data), not from network input. No auth/crypto surface.
Level of scrutiny
High. This is destructive filesystem behavior wired into every unfiltered hoisted bun install/update in a workspace repo — a hot path in core package-manager infrastructure. The PR has been through 18 iterations with several substantive corrections (symlink escape, .bin dangling links, filtered-install semantics, a disputed test rewrite). The design choices — prune only declared names (leaving bun link/hand-dropped folders to bun prune), use the pre-filter() Resolvable tree (accepting a false-negative under --production/--omit), and skip filtered/scanner-narrowed installs — are reasonable but are policy calls a maintainer should ratify.
Other factors
All prior automated findings are addressed as of f693a65; the bug hunter found nothing new this run. One open item is a comment-cop bot flag on the two-line comment above the new prune_bins call, consistent with its earlier flags the author already answered. The nestedBazRepo test rewrite at line ~571 was disputed in an earlier round; the author's empirical rebuttal (lockfile dump shows only baz@0.0.5 after install #2) is plausible and the gate evidence shows the file passing on HEAD, but the assertion change to a pre-existing #38333 test is worth a human glance.
1 participant
Closes #29793
Repro
Before:
is-number@7.0.0installs at the root, but the pre-existingpackages/backend/node_modules/is-number@0.0.0-stalesurvives and shadows it during module resolution from the workspace. After: the stale workspace-local copy is removed.Cause
The hoisted installer drives off
Lockfile's tree, which only yieldsnode_modulesdirectories that still contain dependencies. When every dep in a workspace hoists to the root, that workspace's own tree node is dropped, so the installer never visitspackages/<workspace>/node_modulesand nothing removes stale entries there. (isolated_installrebuilds each workspace'snode_modulesfrom scratch, so it doesn't have this bug; the reporter confirmed a fresh isolated install doesn't reproduce.)Fix
Before the install loop runs, walk each workspace's
node_modules/(plus one level into@scope/) and delete any entry whose name is a dependency the workspace declares but the tree resolves elsewhere, which is exactly the shadowing case: the stale copy wins module resolution over the hoisted one. Entries the lockfile never placed (a manualbun link, a hand-dropped folder) survive installs, and dotfiles (.bin,.cache) are untouched; empty@scope/dirs are cleaned up. The prune only runs on unfiltered installs:--filterinstalls leave unselected workspaces'node_modulesalone, and the security scanner's narrowed pre-install pass (packages_to_install) is skipped because the full install that follows performs the prune. Both policies match the semantics #38333's dedupe and--filtertests codify, withbun pruneas the tool for extraneous entries.Tests (
test/cli/install/bun-update.test.ts)--filterpreserves the excluded workspace's non-hoistable copy (and still prunes genuinely stale entries)@scope/pkg) pruning with empty-scope cleanup3 of the 4 fail on base (the non-hoistable preservation test is a no-regression guard), all pass with the fix.
Rebase note
mainported the package manager from Zig to Rust while this PR was open, so the fix lives insrc/install/hoisted_install.rs(the.zigfile is no longer compiled). Rebased onto current main and squashed to a single commit. Rebased again after #36360 landed: merged its new imports (pack,basename) with this PR's (exists,setDefaultTimeout) and kept both its appended named-update tests and this PR's four tests; no logic changes on either side. Rebased a third time after #38333 (pnpm parity) landed: its tests establish that filtered installs leave unselected workspaces alone, so the prune is now gated to unfiltered installs, and its collapsed-row test was adapted (an unfiltered install now prunes the stale nested copy immediately, so the test re-plants it before exercising the filtered update's removal).[review] gate passed · iteration 18 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 18
evidence per changed file
root cause · written by the author bot
The root cause was that the hoisted installer only wrote to node_modules directories that the hoisted dependency tree placed packages into, so when a dependency previously installed inside a workspace's local node_modules was later hoisted to the root, the stale workspace-local copy was never visited or removed and continued to shadow the updated package during module resolution. The fix adds a prune step before hoisted installs that uses the unfiltered lockfile tree to compute which dependency folder names each workspace's node_modules should contain, then deletes workspace-local entries (…