[robobun]

Problem

• A required peer whose range no version in bun.lock satisfies does not stay on the version the file records. activepieces/activepieces at HEAD has react-json-view (peers react / react-dom at ^17.0.0 || ^16.3.0 || ^15.5.4) with react-json-view/react-dom = 18.3.1 and react-json-view/react = 18.3.1 printed next to it, and flux/react = 18.3.1 likewise, while the root holds 19.2.5. On an untouched checkout the 1.4 canary's bun install --lockfile-only binds those edges to 19.2.5 and removes flux/react, react-json-view/react, react-json-view/react-dom and react-json-view/react-dom/scheduler from the file; 1.3.14 leaves them alone.
• The installed tree changes the same way when the file is not rewritten: with --frozen-lockfile the hoisted linker no longer installs the nested copy (the dependent resolves the root copy at runtime) and the isolated linker links the dependent against the other version, re-keying its store entry. The drift also runs the other way: when the peer was deduped onto the copy hoisted above it and a higher out-of-range version is nested elsewhere, a no-op re-save adds a <dependent>/<peer> entry for that higher version (second test shape below).
• Cause: resolve_peer_dep_version_based (src/install/lockfile/bun.lock.rs), which since install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182 binds required ranged peers on load and since install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 in the pnpm migration, falls back to the highest version present when nothing satisfies the range (candidates.first(), mirroring the resolver's "incorrect peer dependency" branch). The resolver makes that pick once, against the versions present at that moment, and later installs keep the binding (Package::clone), so the version the edge is actually on is recorded only by the tree; re-picking the highest version of the final set moves the edge whenever another out-of-range version exists. The recorded target then has no edge left, the clean drops it, and the next save prints a different tree than the one loaded. The pnpm migration loses pnpm's recorded resolution the same way.

Fix

• The helper returns None when nothing satisfies the range. Every caller already has the record for that case and already uses it for the other edges the helper declines (optional, *, overridden): the three loader loops fall back to the printed tree (the root entry, <workspace>/<name> then the root, and the path walk, whose first probe is <dependent>/<name>), and the pnpm migration falls back to the version pnpm recorded in the snapshot.
• Why this is right: the satisfying scan has a criterion (highest version matching the declared range) that can be recomputed from any package set, which is what makes the binding reproducible. An unsatisfied edge has none; the resolver's pick is arbitrary and recorded only in the tree, so the tree is the thing to read. Every edge this changes ends up bound exactly where 1.3's loader bound it; edges with a satisfying candidate are not touched, so install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182's store-key stability is unchanged (its isolated-install.test.ts tests pass; none of them has an unsatisfied range), and a satisfying version entering the file later still rebinds the edge (pinned by the "rebinds" test). The resolver's own highest-version pick for a fresh resolve is intentionally kept; only the re-derivation on load goes. Since install: stop looping on a peer dependency no published version satisfies #38851 an edge the fallback cannot bind stays unbound rather than failing the load.
• Shape that binds differently from a fresh resolve: the root package.json itself pins an out-of-range version and a higher out-of-range version is nested elsewhere. Nothing is printed for the edge (the hoister dedupes a peer onto a root dependency regardless of range), the fresh resolve binds the higher version, this PR binds the root's copy. Both linkers install the root's copy for either binding (the hoister by that same rule, the isolated linker because its ancestor walk takes a parent's dependency of that name regardless of range), so the file, the hoisted layout and the store key (+7347ae2d86f1441a for the test shape, checked with both builds) are identical either way; the binding shows in pm why and in trees built without the root copy (--production when the pin is a devDependency, --filter). Pinned by the pm why test.
• Shape this improves without fully fixing: a dependent printed at two or more paths. The loader binds a package's edges once per printed path and the last path wins, so the recorded copy is read back when it sits under the last-sorted path, while a walk from another path can reach a different copy (the root's) and the next save then rewrites the entry to that copy, once. Main drops such a record in every order; when nothing is recorded (the root-pin shape above with two paths) main is exact and this PR can rewrite once depending on the order. The per-path overwrite is pre-existing loader behavior for every edge the walk binds (optional and * peers included) and is tracked separately; the order that works is pinned by the "two paths" shape.
• pnpm migration: for a peer pnpm recorded a resolution for, the snapshot suffix now wins (first pnpm test). For a peer pnpm left unmet (no suffix), main bound the highest version present, nesting a copy under the dependent that pnpm's tree did not have whenever that version was not the root's; it is now left unbound, like unmet * peers already are, and the first load of the migrated file binds it to the root copy, which is what the package sees in pnpm's layout too (second pnpm test). The isolated linker keys the migration's own install without that peer and re-keys the entry on the next install; unmet * peers already do that on main, tracked separately.
• Interaction with the open peer work: install: bind peer edges in the hoister, the way loading bun.lock binds them #38767 rebinds every peer edge through this same helper inside the hoister, on every tree build including the one at the end of a load; with the fallback in place it would move these edges at save time too, with this change it leaves them where the loader put them (verified by applying install: bind peer edges in the hoister, the way loading bun.lock binds them #38767's src/ on top of this branch: the tests here, install: bind peer edges in the hoister, the way loading bun.lock binds them #38767's own tests and the activepieces re-save all agree with this branch alone). install: rebind ranged peers whose target the saved tree drops #38768's resolve_peer_dep_version_based_among re-implements the fallback for edges whose target was dropped from the tree and should drop it for the same reason. install: re-resolve a peer row instead of binding it to a stale bun.lock leftover #38901's existing_peer_target describes the loader as binding to the highest candidate and should say the tree is read instead once this lands. install: keep bundled peer edges on their own bun.lock entry when loading #38837 (bundled peers) is independent.
• install: bind an unsatisfiable peer to the copy bun.lock nests under its dependent #38992 fixed the same activepieces case in the same function and is consolidated into this PR. It kept the fallback and looked up only the <dependent>/<peer> entry before it, which covers the first test shape and none of the others: built with its src/, the "hoisted above" shape gains strict-peer-dep/no-deps = 1.1.0, the root's-own-peer shape rebinds to 1.0.1 and drops no-deps@1.0.0, and the pnpm migration still binds 1.0.1 (its root loop and the migration passed nothing to look up). Its own test passes on this branch; the one assertion it had that the tests here lacked, --frozen-lockfile accepting the re-saved file, is now the last step of every re-save test.
• Verified:
  • test/cli/install/bun-lock.test.ts, describe("loading bun.lock keeps a peer nothing in the file satisfies where the file records it"), nine tests on hand-written lockfiles: five re-save shapes (the copy printed next to a package; deduped onto the copy hoisted above it with a higher version nested elsewhere; printed next to a workspace; the root's own peer; a package printed at two paths), each of which must print the expected entries, be a fixed point and pass --frozen-lockfile; the rebind-once-something-satisfies boundary; the root-pin shape via pm why; and --frozen-lockfile placement of the first shape under the hoisted and the isolated linker. Built with main's helper (732491c) eight of the nine fail (the boundary test passes on both by design): entries removed or rewritten to the higher version, the second shape gains strict-peer-dep/no-deps = 1.1.0, the two-paths record is dropped, pm why lists the peer under 1.0.1, the hoisted copy is missing, the isolated entry links 1.0.1.
  • test/cli/install/migration/pnpm-lock-v9.test.ts, two tests under "peer dependencies": the recorded suffix is kept (strict-peer-dep/no-deps = 1.0.0, installed there by --frozen-lockfile; main binds 1.0.1 and drops it) and an unmet peer gets no nested copy (main adds strict-peer-dep/no-deps = 1.1.0).
  • activepieces (workspace manifests regenerated from the lockfile): with this build bun install --lockfile-only keeps the four entries above; the only remaining diff is the dockerode cluster, held solely by an optional peer of sandbox-agent, which is a separate case.
  • Suites run with this build: bun-lock (49), pnpm-lock-v9 (83), and with the same src/ change before the test additions: isolated-install (66), bun-install-registry (243), the pnpm and npm migration suites (all snapshots unchanged), hoist, bun-workspaces, catalogs, overrides, nested-overrides, frozen-lockfile-*, lockfile-only, lockfile-version-2, bun-lockb, migrate-bun-lockb-v2, isolated-relink, config-version, test-dev-peer-dependency-priority, yarn-lock-migration, bun-add, bun-remove, bun-update, bun-update-transitive, bun-prune, bun-pm-why, bun-dedupe. The only failures were 5-second per-test timeouts under the debug build when many files ran at once (pnpm-migration-complete, yarn-cli-repo, a few nested-overrides and bun-prune cases); each passes run on its own with unchanged snapshots.
  • cargo clippy on the crate passes; the first revision's for scan tripped manual_find, which is why the scan is written as find.

Background

• bun.lock stores packages keyed by their path in a hoisted node_modules layout ("a", "a/b") and has no field for what an edge resolved to. On load a regular edge is bound by probing <dependent path>/<name> and then each enclosing path up to the root.
• A peer edge is satisfied by whatever is installed next to or above the dependent. When the version hoisted above satisfies the range, Tree::hoist_dependency dedupes the edge onto it and prints nothing for the edge, which is why install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182 binds such edges by version on load (the path walk would rebind them to the hoisted copy while the writer had bound the highest satisfying version, re-keying isolated store entries, which are named name@version+<hash of peer bindings>). When the version above is out of range and not a root dependency, the hoister places the edge's own target in the dependent's node instead, which is what produces a <dependent>/<peer> entry.
• When no version in the tree satisfies a peer, the resolver binds the edge to the highest version present at that moment and warns incorrect peer dependency. A required peer edge keeps its target alive through the clean, so a version nothing else depends on can stay in the file next to its dependent for as long as the edge points at it; the shapes in the tests are what such a file looks like after the dependency that originally brought that version in has left the project.
• Two rules the trade-off bullets lean on: Tree::hoist_dependency dedupes a peer onto a copy held as a direct dependency of the root even when it is out of range, and the isolated linker resolves a peer by walking the dependent's ancestors for a regular dependency of that name, taking the first one regardless of range; the edge's own binding only matters to either linker when no such copy is above the dependent. A package that is nested (because the root holds another version of it) under two parents is printed once per parent, and the loader binds its edges once per printed row.

Earlier revision of this PR

The first revision kept the fallback and instead made the three loader loops bind any peer edge to the entry printed in its dependent's own node before running the scan, which also covered entries holding an in-range version lower than one elsewhere in the file. Review pointed out that #38767 rebinds every edge through the helper inside the hoister, so a loader-only rule is overridden during the same load, and that the field case (and every shape with a printed record) goes through the helper's nothing-satisfies fallback, which all callers share. This revision fixes that fallback instead; the in-range case is left to the hoister-side binding #38767 is establishing.

---

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-lock.test.ts

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 22 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: 5e81b08b-5eeb-4b32-952f-d5ea11e6d472

📥 Commits

Reviewing files that changed from the base of the PR and between 88a6398 and e2d1e20.

📒 Files selected for processing (3)

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

---

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

[robobun]

^{Updated 11:58 AM PT - Aug 15th, 2026}

@robobun, your commit 2ff1795 is building: #98557

[robobun]

Status: fixed, awaiting review.

Reproduced on main two ways: the new tests (test/cli/install/bun-lock.test.ts, test/cli/install/migration/pnpm-lock-v9.test.ts) fail on a build of main without the src/ change and pass with it, and activepieces/activepieces at HEAD, where bun install --lockfile-only on the 1.4 canary removes flux/react, react-json-view/react, react-json-view/react-dom and react-json-view/react-dom/scheduler; with this branch the re-save keeps all four. Fix: #38892 (the helper no longer falls back to the highest version when nothing satisfies the range; see the PR body).

Update: #38992, which fixed the same case by binding the <dependent>/<peer> entry ahead of the fallback, is closed in favor of this PR; its test passes here and its frozen-install assertion is folded into the re-save tests. After a self-review of the consolidated change, e2d1e20 adds tests pinning the boundaries described in the body (a satisfying version arriving later rebinds, a package printed at two paths, the root-pin shape via pm why, and the pnpm migration's unmet-peer arm), and the body's trade-off bullets were rewritten to match what both builds actually do in those shapes. src/ is unchanged apart from 2ff1795 writing the scan as find for the clippy job.

[claude]

I reviewed this PR and didn't find any bugs. Because it reorders peer-edge binding in the lockfile loader and the description flags a coordination point with #38767's save-side scan, a human look would still be worthwhile.

Checked that the reorder is behavior-preserving for edges the version scan already skipped (optional peers, * peers, non-peers) — each still reaches the same first probe. Verified get_in_own_node's length-overflow None falls through to the same handling as before. Confirmed the test fixtures (strict-peer-dep, no-deps@1.0.0/1.0.1, one-dep, peer-deps-fixed, one-fixed-dep) exist in the verdaccio registry.

Extended reasoning...

Overview

This PR changes parse_into_binary_lockfile in src/install/lockfile/bun.lock.rs so that when binding a peer dependency edge, the loader first checks for an entry in the dependent's own node (<pkg_path>/<name>), then runs the version scan, then walks the printed tree. Previously the version scan ran first, which could bind the edge to a different package than the one the lockfile explicitly recorded, causing that entry to be dropped on the next save. The change is applied uniformly across the three binding loops (root, workspace, package) and adds a small PkgMap::get_in_own_node helper for the package loop. Six new tests in bun-lock.test.ts cover the four dependent shapes plus both linker modes.

Security risks

None. This is a change to the order in which existing lockfile entries are consulted when reconstructing dependency-edge bindings. No new inputs are trusted, no new syscalls or allocations are introduced, and the bounds check in get_in_own_node mirrors the one already present in find_resolution_impl.

Level of scrutiny

High. The lockfile loader determines what gets installed on disk, and peer-dependency binding is one of the subtler parts of it — the PR description itself traces how #32182, #38767, #38837 and #38853 all touch adjacent semantics. I traced each of the three loops before and after to confirm that non-peer, optional-peer, *-peer and overridden-peer edges resolve identically (each already reached the own-node probe first via the walk or via deferred_peer_range returning None), and that the only edges whose binding changes are required non-* peers that have an entry in their dependent's node — exactly the class the PR targets.

Other factors

The tests are well-constructed: hand-written lockfiles for each of the three loop shapes plus both branches of the version scan (nothing-satisfies fallback and higher-in-range-elsewhere), with a fixed-point re-save assertion and installed-tree checks for both linkers. The description is unusually thorough about mechanism and about which existing suites were re-run. What tips this to defer rather than approve is the author's own note that #38767's save-side rescan would fight this loader on the same shape and "whichever lands second" needs adjustment — that is a coordination decision for a maintainer, not something automated review should sign off on alone.

[robobun]

(Superseded after the rework: the interaction with #38767 described in the first revision no longer exists; see #38767 (comment) and the "Interaction with the open save-side work" bullet in the PR body.)

[github-actions]

This PR may be a duplicate of:

1. install: keep bundled peer edges on their own bun.lock entry when loading #38837 - Fixes the same load-time defect in parse_into_binary_lockfile, where the resolve_peer_dep_version_based scan rebinds a peer edge off the <dependent>/<peer> entry printed for it; install: keep bundled peer edges on their own bun.lock entry when loading #38837 handles the bundled-peer subcase that this PR subsumes.

🤖 Generated with Claude Code

[robobun]

Not a duplicate of #38837; the two are independent.

(Edited after this PR was reworked. The original version of this comment said the branch bound every edge with a <dependent>/<peer> entry before the version scan and therefore also passed #38837's test; that was true of the first revision only.)

The current revision removes the helper's fallback for peers whose range nothing in the file satisfies. #38837 exempts bundled peer edges from the scan, and its test is an in-range shape (a bundled no-deps@1.0.0 entry with a root no-deps@1.1.0 that satisfies ^1.0.0), which the scan here still rebinds, so #38837 is still needed for that case and this PR does not cover it. Conversely #38837 does nothing for the unbundled entries this PR is about (activepieces' react-json-view/react-dom and friends carry no bundled marker). The diffs do not overlap textually.

[robobun]

Reworked after self-review; the PR body now describes the current shape. Summary of what changed and why:

• The first revision kept resolve_peer_dep_version_based's nothing-satisfies fallback and made the three loader loops prefer a <dependent>/<peer> entry before calling it. Two problems with that: install: bind peer edges in the hoister, the way loading bun.lock binds them #38767 (in its current form) re-runs the helper for every peer edge inside the hoister, including the lockfile.resolve() at the end of a load, so a loader-only rule is overwritten during the same load and all of the tests here would fail once it lands; and the field case, along with every other shape that has a printed record, goes through the helper's fallback, which the loader loops, the pnpm migration and install: bind peer edges in the hoister, the way loading bun.lock binds them #38767 all share.
• This revision removes that fallback instead (net -12 lines in src/): when nothing satisfies the range the helper returns None, and each caller keeps the record it already falls back to for optional / * / overridden peers (the printed tree on load, pnpm's snapshot suffix in the migration). The in-range case the first revision also covered (an entry holding a lower satisfying version than one elsewhere in the file) is deliberately left to the hoister-side binding install: bind peer edges in the hoister, the way loading bun.lock binds them #38767 is establishing, since that is a question of which binding is wanted rather than of reading the file.
• Checked the interaction directly by applying install: bind peer edges in the hoister, the way loading bun.lock binds them #38767's src/ on top of this branch: the seven tests here pass, install: bind peer edges in the hoister, the way loading bun.lock binds them #38767's own tests pass, and the activepieces re-save is identical to this branch alone (the four react entries stay). The only textual overlap between the two is one doc paragraph.
• Tests now cover the two loader outcomes for such an edge (recorded next to the dependent, or deduped onto the copy above, where the old fallback added a new nested entry on a no-op re-save), the workspace and root loops, both linkers under --frozen-lockfile, and the pnpm migration path, which is the other caller whose behavior changes. All seven fail on main built without the src/ change.
• The remaining comment-cop flags are on the function's three-line return contract, which replaced a seven-line description of the fallback; the rationale lives in the PR body and commit message rather than in the source.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/install/lockfile/bun.lock.rs:3426-3429 — The PR title/description describe a broader fix than the code implements: they reference a new PkgMap::get_in_own_node that reorders the three binding loops to probe the dependent's own node first, claim to fix the in-range case (higher satisfying version elsewhere rebinds the edge), list a test for that shape, and claim #38837's test passes on this branch — but get_in_own_node does not exist, the loops at :3091/:3173/:3244 are unchanged, and the only code change is dropping the candidates.first() fallback so resolve_peer_dep_version_based returns None when nothing satisfies. Either the own-node-first commit was dropped from the branch, or the description is stale from an earlier iteration; if the latter, please narrow the title/description and drop the #38837-subsumption claim (and the follow-up comment restating it) so a reviewer doesn't close that PR or believe the in-range case is covered.

  Extended reasoning...

  What the description says vs. what the diff does

  The Fix section of the PR body states:

  Each binding loop now looks the edge up in the dependent's own node first (the root's bare <name> entry, <workspace>/<name>, and <pkg path>/<name> via the new PkgMap::get_in_own_node), then runs the version scan, then walks the printed tree.

  and

  Bundled peer edges always have their own entry, so this also covers the shape #38837 fixes … its test passes on this branch

  and lists among the verified test shapes:

  a package whose entry holds a lower in-range version than one elsewhere in the file

  None of that is in the diff:

  • grep get_in_own_node src/ returns nothing — the method the description names does not exist.
  • The three binding loops in parse_into_binary_lockfile at bun.lock.rs:3091, :3173, :3244 are byte-identical to main: each still calls resolve_peer_dep_version_based first, then falls through to pkg_map.get / find_resolution. No own-node probe precedes the scan.
  • git diff HEAD~1 --stat on bun.lock.rs shows +16/-19 — that is the doc-comment rewrite plus removing the candidates.first() fallback block. There is no loop reordering and no new PkgMap method.
  • All four new test shapes in bun-lock.test.ts use peerDependencies: { "no-deps": "^2.0.0" } with only no-deps@1.0.0/1.0.1/1.1.0 present — every one exercises the nothing-satisfies case. The described in-range test (entry holds a lower satisfying version while a higher satisfying version exists elsewhere) is absent.

  What the code actually fixes

  The narrower change — deleting the candidates.first() fallback so resolve_peer_dep_version_based returns None when no candidate satisfies the range — fixes exactly the nothing-satisfies case: the caller now falls through to the path walk, whose first probe is the <dependent>/<name> entry, so the edge stays on the version the file records. The commit title ("keep a peer nothing in bun.lock satisfies where the file records it"), the new doc-comment paragraph, the describe title, and all six new tests match this narrower scope precisely, so the code and tests are internally consistent.

  What is not fixed but the description says is

  The in-range case. When the <dependent>/<peer> entry holds a version that satisfies the range but a higher satisfying version exists elsewhere in the file, the scan loop at :3420-3426 still returns Some(higher) before any own-node lookup runs, so the edge is still rebound off its entry. The description's Problem section explicitly names this case ("in the in-range case whenever a higher satisfying version exists elsewhere in the file") and the Fix section claims to address it via own-node-first ordering — but that ordering is not in the code.

  #38837 subsumption. #38837 exempts bundled peers from the scan. This branch does not: for a bundled peer whose range something in the file does satisfy, the scan still returns that version and the own-node entry is never consulted. So #38837's test would only pass here if its bundled peer's range is unsatisfied by everything in the file — the general bundled-peer case is not covered. The description (and the author's follow-up reply to the duplicate-PR bot) both claim "#38837's test passes here as well" and "this branch binds every edge with an own-node entry to it before the scan", which is not what the code does.

  Step-by-step: why this matters

  1. Bot comments "This PR may be a duplicate of #38837"; author replies that this branch subsumes #38837's load-side fix.
  2. A reviewer reads the description + reply, sees "own-node-first" and "#38837's test passes here", and marks #38837 as superseded on the load side.
  3. This PR merges with only the nothing-satisfies fix.
  4. A bundled peer whose range is satisfied by something in the file, or any peer with an own-node entry holding a lower in-range version, is still rebound on load — but the merged PR's description says otherwise, so the next report of that shape gets triaged as "already fixed in #38892".

  Which is it — dropped code or stale description?

  The evidence points to a stale description: the commit title, doc-comment, describe title, and every test all consistently scope to nothing-satisfies-only. If code had been dropped you'd expect the in-range test the description lists to be present (and failing). But only the author can say for sure.

  Suggested action

  Either:

  • (a) push the missing PkgMap::get_in_own_node + loop-reordering commit and the in-range test shape the description lists, or
  • (b) narrow the PR title to match the commit title, cut the own-node-first paragraph, the in-range test-shape claim, and the #38837-subsumption claim from the description, and edit the reply to the duplicate-PR bot so nobody closes #38837 on the strength of it.

  CLAUDE.md rule 11: "NEVER overstate what you got done or what actually works in commits, PRs or in messages to the user."

[robobun]

The review above compared the reworked code against the description of the first revision; it is right that the two did not match at that moment. Current state: the title and body were rewritten for the reworked code (the body's "Earlier revision" section records what was dropped and why), and the earlier reply to the duplicate-PR bot has been edited: this PR does not cover #38837's case (its test is an in-range shape, which the scan here still rebinds), so #38837 stays independent and should not be closed on the strength of this one. The in-range own-entry case is intentionally out of scope here, as the body now says. The code is as the review describes: the only src/ change is that resolve_peer_dep_version_based returns None when nothing satisfies the range.

[claude]

I reviewed this PR and found no bugs. The source change is small and well-reasoned (removing the nothing-satisfies fallback so callers read the printed tree instead), but it changes lockfile-load semantics for unsatisfied peers, carries an acknowledged fresh-install-vs-reload trade-off, and coordinates with three other open PRs (#38767, #38768, #38837) — a human should confirm the design direction.

What was reviewed:

• Traced all four callers of resolve_peer_dep_version_based (three loader loops, pnpm migration) — each already falls back to the recorded tree/snapshot on None, so no caller is left unhandled.
• Checked that the removed block was the only consumer of candidates.first(); the satisfying-scan loop above it is unchanged, so in-range edges keep their #32182 binding.
• New tests cover the four loader shapes, both linkers under --frozen-lockfile, re-save fixed-point, and the pnpm migration path; assertions are specific (entry substrings, installed versions on disk) and use the local Verdaccio registry.

Extended reasoning...

Overview

The PR removes 12 lines from resolve_peer_dep_version_based in src/install/lockfile/bun.lock.rs: the fallback that bound an unsatisfied required peer to the highest-ordered candidate of the same tag. With it gone the helper returns None when no candidate satisfies the range, and every caller falls through to its existing path — the three loader loops walk the printed tree (<dependent>/<name> first), and the pnpm migration reads the snapshot suffix. The doc comment is shortened to match. Seven tests are added across bun-lock.test.ts (six) and pnpm-lock-v9.test.ts (one).

Security risks

None. This is package-manager lockfile-loading logic operating on already-parsed structures; no untrusted input parsing, auth, crypto, or filesystem-path handling is touched.

Level of scrutiny

High. bun.lock loading determines what bun install places on disk for every user; a wrong binding here silently installs a different version than the lockfile records. The change also intentionally accepts a divergence between a fresh resolve and a subsequent reload in one shape (root pins an out-of-range version while a higher out-of-range version exists elsewhere), trading it against the previous behavior where every shape with a printed record drifted. That is a defensible trade but a design call, not a mechanical fix.

Other factors

• The PR is tightly coupled to three open PRs: #38767 re-runs this helper inside the hoister on every tree build, #38768 re-implements the fallback for a subset and (per the body) should drop it too, and #38837 is a sibling covering bundled peers. The author verified composition with #38767 by applying its src/ on top, but the sequencing of these merges is a maintainer decision.
• The comment-cop bot flagged the function's doc comment four times; the author responded that the current three-line return contract replaced a seven-line fallback description and the rationale lives in the PR body. That is a judgment call for a human.
• Test coverage is thorough: the four it.each shapes exercise the root loop, workspace loop, and package loop; the two linker tests verify the on-disk tree matches the recorded binding under --frozen-lockfile; the re-save fixed-point check catches drift in either direction; the pnpm test covers the fourth caller. All are hermetic (Verdaccio, hand-written lockfiles) and assert specific entry strings and installed package.json versions.
• The extensive suite runs listed in the PR body (bun-lock, isolated-install, bun-install-registry, migration suites, etc.) give reasonable confidence no existing behavior regressed, but the acknowledged trade-off case is not covered by an automated test.

[robobun]

On the one open point, the trade-off shape not having a test: the shape only differs in what the isolated linker links the dependent against on the first reload after a fresh resolve (the hoisted layout is identical either way, since the root's pin wins the slot regardless of the binding), and which of the two bindings is preferable there is the design question flagged for maintainers in the body. It is documented rather than pinned on purpose; if the direction is confirmed I am happy to add an isolated-linker test that asserts the reload binds to the root's pin and that later reloads are a fixed point.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes lockfile-load peer-binding semantics, carries a documented trade-off the author has flagged for maintainer input, and interacts with three other open PRs (#38767, #38768, #38837) whose overall direction should be confirmed together, a human look would still be worthwhile.

What was reviewed:

• Traced all four callers of resolve_peer_dep_version_based (three loader loops in bun.lock.rs, three sites via resolve_peer_like_bun_lock in pnpm.rs) — each already falls back to the printed tree / pnpm snapshot on None, and may_stay_unresolved covers peers so an unbound edge does not fail the load.
• Checked that the removed fallback's tag-match guard (Npm/Git/Github) has no remaining consumer; the iterator rewrite of the satisfying scan is behavior-preserving.
• New tests cover all three loader loops, both linkers under --frozen-lockfile, the fixed-point property, and the pnpm migration path; the strict-peer-dep fixture exists in the local registry.

Extended reasoning...

Overview

The src/ change is small: resolve_peer_dep_version_based in src/install/lockfile/bun.lock.rs drops its "nothing satisfies the range" fallback (which returned candidates.first() when its resolution tag matched the dependency's version tag) and now returns None in that case, so callers fall back to the tree recorded in the lockfile / pnpm snapshot. The explicit loop over candidates is also rewritten as .iter().copied().find(..) with .get().is_some_and(..), which is behavior-identical to the old bounds-checked loop. The doc comment is trimmed to describe the new contract. Seven new tests cover four lockfile shapes (nested next to a package, deduped onto the hoisted copy, next to a workspace, root's own peer), both linkers, and the pnpm-v9 migration path.

Security risks

None identified. The change only affects which already-recorded lockfile entry a peer edge is bound to on load; it does not touch integrity verification, registry auth, tarball fetching, or any untrusted-input parsing surface. Returning None instead of a package id makes the helper strictly more conservative.

Level of scrutiny

High. This is core bun install lockfile-load semantics: the binding chosen here decides what the hoisted linker installs under a dependent and how the isolated linker keys its store entries. A wrong binding silently changes the runtime dependency graph. The PR body itself documents one shape (root pins an out-of-range version while a higher out-of-range version exists elsewhere) where a fresh resolve and a reload now disagree on the first cycle, and the author has explicitly left whether that trade is acceptable — and whether to pin it with a test — as a maintainer call.

Other factors

• The author verified the tests fail on main without the src/ change and pass with it, and validated against activepieces/activepieces.
• All four callers were checked: on None they fall through to the printed-tree lookup (root entry / <workspace>/<name> then root / path walk starting at <dependent>/<name>) or pnpm's importer_versions, and every path is guarded by may_stay_unresolved (peers qualify) or an explicit is_peer() continue, so an unbound edge never fails the load.
• The change is entangled with #38767 (which the body says re-runs this helper in the hoister), #38768 (which the body says re-implements the removed fallback and should drop it too), and #38837 (bundled peers, independent). The author cross-tested with #38767's src/ applied on top, but confirming the direction across all four is a maintainer decision.
• The comment-cop inline comments are all resolved; the remaining doc comment on the function is a three-line return contract, not a workaround justification.

[claude]

I reviewed this PR and didn't find any bugs. The src/ change is small and the callers' None fallbacks check out, but because it changes how unsatisfied peer edges bind on load (with an acknowledged trade-off shape flagged for a maintainer call) and needs to be sequenced against #38767/#38768/#38837, a human look is still worthwhile.

What was reviewed:

• Confirmed all four callers of resolve_peer_dep_version_based (three loader loops + resolve_peer_like_bun_lock in pnpm.rs) already handle None via the tree/snapshot fallback the PR body describes.
• Checked the rewritten find scan is behavior-identical to the old satisfying-candidate loop; only the post-loop candidates.first() fallback is dropped.
• Tests cover the caller matrix (root/workspace/package loops, hoisted+isolated linkers, pnpm migration) and pin the fixed-point + --frozen-lockfile acceptance.

Extended reasoning...

Overview

The PR removes the "nothing satisfies" fallback from resolve_peer_dep_version_based in src/install/lockfile/bun.lock.rs (net -12 lines). When no version in the lockfile satisfies a required peer's range, the helper now returns None instead of picking candidates.first(), letting each caller fall back to the record it already uses for optional/*/overridden peers: the printed tree in the three loader loops, and pnpm's snapshot suffix in the migration. Nine new tests across bun-lock.test.ts and pnpm-lock-v9.test.ts cover the affected shapes, both linkers, and fixed-point re-saves.

Security risks

None. This is peer-dependency edge binding during lockfile load; no untrusted parsing, no network, no filesystem writes beyond what bun install already does. Tests use the local Verdaccio registry and hand-written lockfiles.

Level of scrutiny

High. This is core bun install resolution logic that decides which package version a peer edge binds to, which directly affects what lands in node_modules and what --frozen-lockfile accepts. The line-count change is small, but the semantics are subtle: the PR body itself documents a trade-off shape (root pins an out-of-range version with a higher out-of-range version nested elsewhere) where the new behavior differs from a fresh resolve on the first reload, and the author explicitly left that direction open for maintainer confirmation rather than pinning it with a test.

Other factors

• The PR was already reworked once after self-review; the current shape is the second design.
• It interacts with three other open PRs (#38767 rebinds through this helper in the hoister, #38768 reimplements the same fallback, #38837 handles bundled peers) and consolidates a fourth (#38992). A maintainer should confirm the sequencing and that #38768 will drop its copy of the fallback as the body suggests.
• Test coverage is thorough and follows the file's conventions (makeInstallRunner, --lockfile-only fixed-point checks, both linkers). The strict-peer-dep fixture and integrity constants added to the pnpm test look consistent with neighbors.
• CI build #98557 is referenced but its result isn't in the timeline; the author reports the install suites pass locally with debug-build timeout noise only.

[Jarred-Sumner]

This is Claude (an AI assistant) writing on Jarred's behalf.

Included in #44968.