install: bind unbound required peers while hoisting, like optional peers

[robobun]

Problem

• Under the isolated linker, the bun install that migrates a pnpm-lock.yaml in which pnpm left a package's peer unmet (no peer suffix on its snapshot key, e.g. autoInstallPeers: false) is not a fixed point. With one-dep (depends on no-deps) and peer-deps-too (peer no-deps: *), the migrating install creates node_modules/.bun/peer-deps-too@1.0.0; the next bun install, with nothing changed, prints + peer-deps-too@1.0.0, creates peer-deps-too@1.0.0+f8a822eca018d0a1 next to it and relinks to it. bun.lock is byte-identical between the two runs, so there is nothing to diff.
• Same thing after a yarn.lock migration (yarn.lock has no entry for the no-deps@* spec), and for the root's or a workspace member's own unmet peers the migrating install leaves the peer out of that importer's node_modules and the next install, while reporting (no changes), links it.
• Cause: the migrations leave these edges unbound (src/install/pnpm.rs, the continue arms for a peer with nothing recorded; src/install/yarn.rs binds a peer only when an entry with that exact spec exists), and Tree::process_subtree (src/install/lockfile/Tree.rs) walked the tree for an unresolved edge only when it was an optional peer; an unresolved required peer was skipped. So the edge stayed unbound through the migrating install, and the isolated linker drops an unbound required peer (is_filtered_dependency_or_workspace) and keys the entry without it. Loading the saved bun.lock binds the same edge by the path walk (PkgMap::find_resolution; * peers and everything else resolve_peer_dep_version_based declines take that walk), so every later install keys the entry with the peer.

Fix

• process_subtree walks for any unbound peer, not only an optional one. Required peers now go through the existing Resolve / ResolveLater / ResolveReplace results, so the asserts in hoist_dependency and the names pending_optional_peers / late_bound_optional_peer (now pending_peers / late_bound_peer) follow; nothing else changes. Every lockfile producer hoists after building its packages (pnpm.rs, yarn.rs, migration.rs for package-lock.json, and the bun.lock loader itself), which is why the fix is here rather than in each migration.
• Why this binding is the right one: it is the binding the next load produces. The hoister's walk and the loader's path walk look at the same places (the dependent's own node, then each enclosing node, then the root). A package's non-peer edges are placed before its peers (Behavior::cmp), an enclosing node is complete before a package inside it is processed, and a copy that reaches the root later binds through ResolveReplace, after which Lockfile::resolve repeats the pass with the binding known up front, exactly as a reload runs (the loop install: keep optional peer bindings when cleaning the lockfile #37426 added for optional peers; it still ends because every repeated pass fills a slot and none empties one). A fresh resolve also binds a * peer to a copy already in the lockfile, which is why the tests can compare the migrated store against a fresh install's.
• What it does not change: peers the loader binds by version never reach this branch from the pnpm migration, which binds them by version before hoisting. install: bind peer edges in the hoister, the way loading bun.lock binds them #38767 applies that rule inside the hoister, before the unresolved check, so it composes with this (it binds what it can, this walks for what it declines), and once install: keep a peer nothing in bun.lock satisfies where the file records it #38892 stops binding peers nothing satisfies, those take this walk, which is what its loader fallback does too. bun.lock loads are unaffected: anything this walk can find, the loader already bound. Fresh resolves bind every peer that has a candidate, so they are unaffected as well. Printing does not read peer bindings, so bun.lock output is unchanged (all 102 migration snapshots pass unmodified); the binding shows in the isolated store key, the importer's node_modules links, pm why and the yarn printer.
• Verified:
  • test/cli/install/migration/pnpm-lock-v9.test.ts, describe("a peer pnpm left unmet is bound by the install that migrates"): peer-deps-too in dependencies (the copy is above it when its peer is walked) and in devDependencies (it hoists first, so the peer is bound by ResolveReplace when no-deps reaches the root and the pass repeats); each requires the migrating install to create a +<hash> entry, the reinstall to print (no changes) with the same store and the same bun.lock, and a fresh install of the same package.json to produce the same store. A third test does the same for the root's and a workspace member's own unmet peers, checking the no-deps links in both node_modules. With main's src/ all three fail at the first store / link assertion (bare peer-deps-too@1.0.0; no-deps missing from both directories).
  • test/cli/install/migration/yarn-lock-migration.test.ts, describe("installing from a migrated yarn.lock"): the same shape from a yarn.lock against the test registry; with main's src/ the store has the bare entry.
  • Run with this build: every file in test/cli/install/migration/, and bun-lock, isolated-install, hoist, bun-lockb, migrate-bun-lockb-v2, lockfile-only, lockfile-version-2, isolated-relink, config-version, bun-pm-why, frozen-lockfile-pruned, frozen-lockfile-missing-workspace, public-hoist-pattern, test-dev-peer-dependency-priority, bun-install-registry, bun-workspaces, catalogs, overrides, nested-overrides, bun-add, bun-add-catalog, bun-add-filter, bun-remove, bun-update, bun-update-transitive, bun-update-lockfile-sync, bun-prune, bun-dedupe, bun-patch, bun-audit. complex-workspace fails the same way without this change (gitlab/bitbucket are unreachable here); the only other failures were 5 second per-test timeouts in bun-add-catalog and pnpm-migration-complete while the host was overloaded; the same files time out more with main's src/ under the same load, and the timed-out tests pass when rerun on their own. cargo clippy -p bun_install is clean.

Background

• A peer edge is satisfied by whatever copy of the package is installed next to or above its dependent, so the hoister dedupes a peer onto that copy instead of placing anything, and bun.lock, which keys packages by their hoisted path and records no target for edges, binds a peer on load by probing <dependent>/<name>, each enclosing path, then the root. Required peers with a version range are the exception since install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182: they are bound to the highest version in the file that satisfies the range, because the path walk would move them.
• The isolated linker names a store entry name@version, or name@version+<hash> where the hash covers the packages the entry's peers are bound to, and links the bound peers into the entry (and, for the root and workspaces, into their own node_modules). A dependent whose peer binding differs between two installs therefore gets two entries and is relinked; an unbound required peer contributes nothing.
• The hoister builds the tree breadth-first. For an unresolved optional peer it already walked up from the dependent: Resolve when a copy is there, ResolveLater when another unresolved peer of the name got there first, otherwise an empty slot at the root that a later copy of the name fills through ResolveReplace, which marks the pass as having bound something late so Lockfile::resolve repeats it. This change sends required peers down the same path.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 5 minutes

Limit details: You’ve used all 5 included reviews currently available under your plan.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f4ac9841-57f6-468f-bfd7-d314ba237c6c

📥 Commits

Reviewing files that changed from the base of the PR and between 669bd82 and 9605fb6.

📒 Files selected for processing (4)

• src/install/lockfile.rs
• src/install/lockfile/Tree.rs
• test/cli/install/migration/pnpm-lock-v9.test.ts
• test/cli/install/migration/yarn-lock-migration.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 2:44 AM PT - Aug 16th, 2026}

✅ @robobun, your commit 9605fb676d23dd149554ae7761bf7d13e3cac5f8 passed in Build #99387! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 39343

That installs a local version of the PR into your bun-39343 executable, so you can run:

bun-39343 --bun

[robobun]

Status: reproduced and fixed; waiting on CI and review.

Reproduced with a debug build of main (aec33f5) using the registry fixtures: package.json with one-dep@1.0.0 and peer-deps-too@1.0.0, a v9 pnpm-lock.yaml with autoInstallPeers: false and no peer suffix on peer-deps-too@1.0.0, linker = "isolated". First bun install creates node_modules/.bun/peer-deps-too@1.0.0; the second prints + peer-deps-too@1.0.0 and adds peer-deps-too@1.0.0+f8a822eca018d0a1 with bun.lock unchanged. The same shape from a yarn.lock, and the root's / a workspace's own unmet peers (missing node_modules/no-deps link after the first install), behave the same way. With this branch the first install produces the +f8a822eca018d0a1 entry and the links, and the second install reports (no changes).

The new tests in test/cli/install/migration/pnpm-lock-v9.test.ts and test/cli/install/migration/yarn-lock-migration.test.ts fail with main's src/ and pass with this branch.

[claude]

I reviewed this PR and didn't find any bugs. The one-line guard widening in the hoister is well-argued and thoroughly tested, but it changes peer-binding semantics in Tree::process_subtree — the path every lockfile producer runs — so a human look at the resolve-loop and version-based-binding interactions is still worthwhile.

Checked the widened is_peer() guard against the Resolve/ResolveLater/ResolveReplace arms and the relaxed debug_assert!s in hoist_dependency — all reachable states still hold under is_peer().
Checked Lockfile::resolve termination: late_bound_peer is set only in ResolveReplace, which fills a previously-invalid slot, so the loop still converges.
Checked the METHOD == Filter path: the early pkg_id == invalid_package_id → continue there fires before this branch, so the Filter tree is unchanged.
Checked the pending_optional_peers → pending_peers rename — no other references in the tree.

Extended reasoning...

Overview

The PR changes one guard in src/install/lockfile/Tree.rs (process_subtree): the tree walk for an unresolved dependency now fires on is_peer() instead of is_optional_peer(), so an unbound required peer takes the same Resolve/ResolveLater/ResolveReplace path optional peers already took. Everything else in Tree.rs and lockfile.rs is mechanical: relaxing four debug_assert!(…is_optional_peer()) to is_peer(), renaming pending_optional_peers/late_bound_optional_peer to pending_peers/late_bound_peer, and updating comments. Two test files add fixed-point tests for pnpm-lock and yarn.lock migrations under the isolated linker (migrate → store keyed with +<hash>, reinstall is no-op with identical store, fresh install produces the same store), plus a workspace/root-peer link test.

Security risks

None. This is dependency-tree hoisting logic; no untrusted-input parsing, path handling, network, or auth surface is touched. The tests use the local Verdaccio registry.

Level of scrutiny

High. The code diff is small but the hoister runs for every bun install and every lockfile producer (pnpm/yarn/package-lock migrations, bun.lock load, fresh resolve). The PR description is unusually thorough — it explains why the binding matches the loader's path walk, why Lockfile::resolve still terminates, and how it composes with #32182/#37426/#38767/#38892 — but that same breadth of interaction is exactly why a maintainer who owns this subsystem should confirm the argument. In particular: (a) that required peers reaching ResolveReplace and triggering a re-hoist can never oscillate, and (b) that no fresh-resolve or bun.lock-load caller relied on unresolved required peers not being placed.

Other factors

• The METHOD == Filter branch skips invalid_package_id before this guard is reached, so the install-time filtered tree is unchanged; only the Resolvable (save-to-disk) tree sees new behavior. That matches the claim that bun.lock output is byte-identical (102 snapshots pass).
• Tests are strong: they cover both the Resolve path (peer-deps-too in dependencies, no-deps already at root) and the ResolveReplace path (peer-deps-too in devDependencies, hoists first), assert store contents exactly, and cross-check against a fresh install of the same manifest.
• No CODEOWNERS entry covers src/install/.
• No prior human reviews or outstanding comments; CI was still building at review time.

[robobun]

On the two points above, for whoever picks this up:

• Termination of Lockfile::resolve: late_bound_peer is set in exactly one place, the ResolveReplace arm (Tree.rs:848), and hoist_dependency returns ResolveReplace only for a slot whose edge is still invalid_package_id; the arm then writes a valid id into that edge. The five writes to resolutions in Tree.rs (826, 841, 849, 862, 886) all store a valid id and nothing in a pass stores invalid_package_id, and buffers.resolutions is carried from one pass to the next, so every pass that asks for another one leaves strictly fewer unbound peer edges than it started with. That bound is the number of peer edges, the same argument install: keep optional peer bindings when cleaning the lockfile #37426 made when it added the loop; this change only lets required peers count towards it.

• Placement: the cleaned tree never contains an unresolved edge either before or after this change. An unbound peer now ends up either bound without placing anything (Resolve), bound with the root slot handed to the real edge that arrived (ResolveReplace, so the tree holds the regular edge exactly where it would have gone with no slot there), or still unbound and dropped by Builder::clean. So buffers.trees / hoisted_dependencies, and with them bun.lock and the hoisted layout, come out the same as before; what changes is buffers.resolutions for edges that previously stayed unbound. For a bun.lock load those edges are the ones the loader's own walk already failed to bind (same places searched), and for a fresh resolve a required peer stays unbound only when the registry could not supply the package at all, in which case there is no copy for this walk to find either. The migrations are the producers where the walk actually binds something, which is what the tests exercise.