install: keep a package below an ancestor whose version of its peer is out of range - #36300
install: keep a package below an ancestor whose version of its peer is out of range#36300robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 38 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. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 2:04 PM PT - Aug 16th, 2026
✅ @robobun, your commit b790347cf124d8b5b24b0334d07c58fc8d5bfb7a passed in 🧪 To try this PR locally: bunx bun-pr 36300That installs a local version of the PR into your bun-36300 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes the hoisting algorithm in a way that alters node_modules layout for every install where a peer version differs across the tree — worth a human eye from someone who owns the install code.
What was reviewed:
- The new peer-conflict check runs only on ancestors (
!AS_DEFINED), which are fully populated by the BFS queue before being visited, so the check isn't order-sensitive. - The
unsafeaccesses reuse the same bounds invariants as the existing name-match loop just below. - Both updated assertions in
bun-install-registry.test.tsnow match their test titles / yarn semantics; the yarn peer-deps test's "5 packages installed" line was checked and still holds.
Extended reasoning...
Overview
This PR adds a guard to Tree::hoist_dependency in src/install/lockfile/Tree.rs: while walking up ancestor trees during hoisting, if an ancestor already holds a different resolution for one of the package's peer dependencies, hoisting stops there (DependencyLoop) so the package stays nested next to the peer version it actually resolved against. Two existing tests in bun-install-registry.test.ts had their assertions updated to reflect the new (correct) layout, and a new regression test was added in hoist.test.ts using pre-existing hoisting-peer-check-* registry fixtures.
Security risks
None. This is install-layout logic operating on already-resolved lockfile data; no new untrusted-input parsing, no new syscalls, no path construction from user data.
Level of scrutiny
High. The hoisted linker's tree builder decides the on-disk node_modules shape for every bun install. The change is small and well-targeted, but it is an algorithm change with user-visible effects: two existing tests changed output (the yarn "different peer dependencies are different instances" test flipped from true/true/false to false/true/true, and the workspace peer test now installs 3 packages instead of 2 with an extra nested copy). Both changes look like corrections rather than regressions — the yarn test now actually matches its own title — but a maintainer who knows this code should confirm that returning DependencyLoop here (rather than, say, a targeted Placement) composes correctly with the rest of the hoist state machine, and that the extra nesting in the workspace case is the intended tradeoff.
Other factors
The mechanism is sound: the check is gated on !AS_DEFINED, so it only fires on ancestor trees, and because subtrees are processed BFS via TreeFiller, ancestors are fully populated before any descendant tries to hoist through them — the check sees the complete ancestor dep list, not a partial one. The unsafe { deps.get_unchecked(...) } / raw-pointer reads reuse exactly the same invariants documented for the name-match loop immediately below. The added nested loop is O(peers × ancestor-deps) per ancestor; peers are typically 0–3, so perf impact should be negligible, but I did not benchmark. The new test covers both fresh install and reinstall-from-lockfile. Given the algorithm-level nature and the two existing-test behavior changes, deferring to a human.
There was a problem hiding this comment.
I didn't find any bugs, but this changes the core hoisting algorithm and flips assertions in two pre-existing tests, so it's worth a human look from someone who owns src/install/.
What was reviewed:
- Traced the new
!AS_DEFINEDpeer check through the recursion —DependencyLooppropagates to theAS_DEFINED=trueframe and falls through toPlacement, consistent with how the existing name-collision path already behaves. - Confirmed the
hoisting-peer-check-parent/-childfixtures exist intest/cli/install/registry/packages/(they aren't shown in the diff). - Checked the yarn-compat test: the new
false\ntrue\ntrueoutput matches what the test name says should happen (two peer-deps instances are no longer===), and the5 packages installedcount is unchanged because bothpeer-depscopies were already counted.
Extended reasoning...
Overview
Adds ~30 lines to Tree::hoist_dependency in src/install/lockfile/Tree.rs: at each ancestor visited during hoisting (!AS_DEFINED), if that ancestor already holds a different resolution for any of the hoistee's peer dependencies, hoisting stops there (DependencyLoop). Adds a new regression test in hoist.test.ts (with new registry fixtures) and updates two existing assertions in bun-install-registry.test.ts whose behavior changed.
Security risks
None. No untrusted-input parsing was added; the new loop reads existing lockfile buffers via the same get_unchecked invariants as the surrounding code, gated on package_id != invalid_package_id so resolution_lists[package_id] is in bounds.
Level of scrutiny
High. The hoisted linker's tree builder runs on every install and the placement it chooses is user-observable at runtime via require() resolution. Two pre-existing tests now assert different output — the yarn-compat one now matches its own name, and the workspace one now nests peer-deps-fixed (with a nested no-deps@1.0.0) instead of leaving it at root next to no-deps@2.0.0. Both look more correct than before, but a maintainer should confirm those layout changes are intended and match npm.
Other factors
The check only fires when the ancestor has already had the conflicting peer placed in it, so correctness depends on DepSorter ordering / queue processing having placed the peer first. In the shipped test cases that holds (root pins no-deps before the parent's subtree is processed; no-deps sorts before peer-deps-fixed), but I'd want a human to confirm there isn't an ordering where the peer arrives after the dependent and the check is skipped. The added inner loop is O(peers × ancestor-deps) per ancestor per package — likely fine, but worth a sanity check on a large tree.
There was a problem hiding this comment.
I didn't find any bugs, but this changes the core hoisting algorithm in a way that alters on-disk node_modules layouts (two existing tests had their expected output flipped), so it's worth a human look from someone who owns the install/lockfile code.
What was reviewed:
- Traced the
!AS_DEFINEDgate andDependencyLooppropagation through thehoist_dependencyrecursion — the new check only runs on ancestors and falls through toPlacementat the originating subtree, consistent with the existing name-conflict path. - Checked the
get_uncheckedbounds — same invariants as the pre-existing name-match loop below it. - Confirmed the flipped yarn-compat assertion (
false\ntrue\ntrue) now matches the test's stated intent, and thehoisting-peer-check-*verdaccio fixtures already exist on main.
Extended reasoning...
Overview
This PR adds a peer-dependency-aware guard to Tree::hoist_dependency in src/install/lockfile/Tree.rs. When walking up the ancestor chain (AS_DEFINED = false), it now checks whether the current ancestor tree already holds a different resolution for any of the hoisting package's peer dependencies; if so it returns DependencyLoop, which keeps the package nested next to the peer version it actually resolved against. A new test in hoist.test.ts covers the fresh-install and install-from-lockfile paths, and two existing assertions in bun-install-registry.test.ts were updated to reflect the new (correct) layout.
Security risks
None. This is pure in-process resolution logic over lockfile buffers; no untrusted input parsing, no filesystem path construction, no network. The new get_unchecked calls use the same bounds invariants as the adjacent pre-existing loop.
Level of scrutiny
High. hoist_dependency runs for every dependency of every package during bun install with the hoisted linker, and its output determines the physical node_modules layout that ships to users. The change is small, but it's a semantic change to a load-bearing algorithm — two existing tests had their expected behavior changed (the yarn-compat "different peer deps → different instances" test flipped from true/true/false to false/true/true, and the workspace-peer test now installs 3 packages instead of 2 with peer-deps-fixed nested). Both changes look like genuine fixes rather than regressions, but someone who owns this code should sign off on the layout change and any disk-usage implications (packages that previously deduped to root will now sometimes stay nested).
Other factors
- The new check is order-dependent: it only fires if the conflicting peer has already been placed in the ancestor tree by the time this package is processed.
DepSorterhandles the common case, but I can't rule out orderings where the peer lands after the dependent and the guard is bypassed — worth a domain expert's eye. - The
hoisting-peer-check-parent/-childverdaccio fixtures referenced by the new test already exist on main (added in an earlier PR), so the test is self-contained. - CI on the completed lanes is green for both touched test files per the author's report; the red is unrelated build-job timeouts.
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
The ajv shape does need to nest, but not like this. The comparison has to be by range, not package id, and it must not read optional or * peer bindings; as written it duplicates packages in any lockfile that has two in-range versions of a peer, and the yarn test flip is that duplication, not npm's layout.
- id comparison against the lockfile-wide binding, and it runs before the same-package dedupe (Tree.rs:1013)
- optional and * peer bindings are outputs of hoisting and of the bun.lock path walk, so bun.lock and node_modules can disagree (Tree.rs:998)
- only the direct-dependent shape is fixed; one level up the peer still resolves to the root copy (Tree.rs:1016)
- workspace packages get inert nested symlinks and new bun.lock keys (Tree.rs:992)
#37426 landed on main and changes this function; the hunk still applies cleanly but the optional peer interaction above gets worse with it, so rebase before the next push.
robobun
changed the title
|
Reworked in 26a9be6 and merged up to current main (includes #37426); PR body rewritten for the new shape.
Checked with a script that resolves every required peer from the dependent's directory and compares it with what the dependent's parent resolves: smallest shape goes from 2 out-of-range / 2 split instances to 0 / 0, the orval repro from 5 split instances (lockfile install) to 0 on both install paths, matching npm; its |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
One thing left, same root cause as before. The five items from the last round are addressed at this head: the ancestor entry is compared by range with no root override, optional peers are skipped and * never reaches the id fallback, workspace and link: packages and peer edges shadowed by a regular dependency are skipped, the yarn expectation is back to main's, and the one-level-up shape is written down. What remains is that the check still nests when nesting cannot change what the package resolves, and in a common shape (root pins the peer outside the range and a second version of that name exists anywhere in the lockfile) that now installs one copy of the peer-dependent package per consumer where main installs one; the id fallback is also unstable between the first and second install for ranges the range comparison cannot evaluate. Both inline; layouts there are traced from the source, not run. The branch is merged up past the main I have locally, so nothing to say about conflicts.
|
a8e0acf addresses both. The check is now two steps: the required npm-range peers of the package are collected when its walk starts, each flagged with whether the tree it is leaving will hold an in-range version; each ancestor then either marks a peer as provided (in-range entry) or, on an out-of-range entry, stops the hoist if the peer is provided and drops the peer if not. The peer binding is not read anywhere. Net effect on the shapes you traced: root-pinned-out-of-range with consumers that do not provide the peer hoists one copy as on main (tested), the chain where the provider is above the direct dependent still nests (tested), and |
There was a problem hiding this comment.
Both nits from my previous pass are addressed in 2c48523: the probe is now dropped when the owner has a resolved non-peer edge on the peer name that fails the range (Some(false) => continue 'peers, covered by the new "hoists when its own dependent installs an out-of-range copy of the peer" test), and the describe block is .concurrent. No new findings on re-review. Leaving this for a human sign-off since the a8e0acf redesign of the probe hasn't been re-reviewed by a maintainer yet.
Checked on this pass: begin_peer_probes is only reached from the AS_DEFINED recursion site where next_id's owner is a resolved package (the Folder/bundled/unresolved cases short-circuit before it), so the owner_dep_id → resolutions[...] index is always valid. probe_peers reads tree.dependencies via the per-entry DependencyIDList, which is populated for all ancestors by the time the walk reaches them (BFS), and swap_remove on an unprovided out-of-range hit keeps the loop index correct.
Extended reasoning...
Overview
The PR adds a peer-aware guard to the hoisted linker's Tree::hoist_dependency walk. Two new methods on Builder (begin_peer_probes, probe_peers) collect a package's required npm-range peers when it starts walking up from its declaring tree, track whether any tree along the walk holds an in-range version, and stop the hoist (DependencyLoop) at the first ancestor holding an out-of-range version — but only if an in-range version is reachable from where the package started. A single Vec<PeerProbe> scratch buffer is added to Builder and cleared per package. Seven new tests in hoist.test.ts cover the fix, its inverse cases, and the exclusions (workspace/symlink packages, peers shadowed by a regular dep, owner installing an out-of-range copy). No existing test expectations change.
Changes since my last review
Commit 2c48523 addressed both prior nits: (1) when the owner has a resolved non-peer edge on the peer name whose resolution fails the child's range, the probe is now skipped entirely (Some(false) => continue 'peers) rather than pushed with provided = false, preventing an ancestor's shadowed in-range copy from spuriously flipping provided; a dedicated test ("hoists when its own dependent installs an out-of-range copy of the peer") covers this. (2) The test block is now describe.concurrent. The bug-hunting pass on this revision found nothing.
Security risks
None. This is deterministic tree-layout logic over already-resolved lockfile data; no untrusted input parsing, no I/O, no allocation driven by external sizes.
Level of scrutiny
High. This is the core hoisting decision for every dependency in every hoisted install, and subtle mistakes here silently change node_modules layouts across the ecosystem. The maintainer review history on this PR bears that out: three rounds of substantive correctness feedback (id-vs-range comparison, optional-peer instability, dist-tag/non-npm binding instability, workspace symlinks, redundant nesting when nothing is gained), each of which reshaped the approach. The current begin_peer_probes/probe_peers design was introduced in a8e0acf specifically to address the last round; the maintainer has not yet reviewed that revision.
Other factors
- The PR body is thorough and explicitly documents the one shape not fixed (peer-set placement when the provider itself was hoisted away).
bun-install-registry.test.tsis back to its main-branch expectations, so the change is now purely additive on the test side.- All seven new tests install twice (fresh + from
bun.lock) and assert exact directory contents and versions. - I verified the new code paths for index safety:
begin_peer_probesis only called from theAS_DEFINEDbranch after the bundled/folder/unresolved early-outs, soowner_dep_id != ROOT_DEP_ID(the root tree has no parent to walk to) andresolutions[owner_dep_id]is a valid package;probe_peersreads ancestor dependency lists that BFS ordering guarantees are already populated.
Given the maintainer's active engagement and the non-trivial redesign since their last look, deferring to a human sign-off rather than auto-approving.
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Looks ready to merge from this side. Both remaining items are addressed at this head: the probe now only nests when a copy the package would still see from its declared position is in range and the ancestor's is not, only npm ranges are collected and no peer binding is read, and the owner's own out-of-range copy skips the probe. Read the new Tree.rs code and traced the seven layouts from it; also ran the new hoist.test.ts against a main build, where the two fixing tests fail and the five unchanged-layout tests pass, so the as-before claims hold. bun-install-registry.test.ts is back to main's version and none of the touched files moved on main since the merge-base. One optional test nit inline.
|
Thanks. The optional nit is in as ca40287 (second install asserts no "Saved lockfile"; passes for all seven shapes). The only other change since your look is d62d889, test-only: the concurrent tests now each get their own BUN_INSTALL_CACHE_DIR, because the runner sets one per file and the Windows lanes failed with ENOTEMPTY when several of them extracted no-deps@2.0.0 into it at once (the Windows move-into-cache path in extract_tarball.rs evicts a concurrent winner on retry; filed separately, not touched here). Nothing under src/ changed since a8e0acf apart from the owner-copy skip in 2c48523 you already read. |
ca40287 to
725bf3f
Compare
|
Rebased onto current main (725bf3f, squashed) to clear the conflicts from #36303 / #38333 rewriting |
…s out of range With the hoisted linker a package was hoisted past an ancestor holding a version of one of its peer dependencies that the peer range rejects, even when the package that pulled it in had an in-range version nested next to it, so at runtime it resolved the wrong version (ajv-errors next to the root's ajv@6 while spectral-core used its own ajv@8). When a package starts hoisting out of the directory it was declared in, its required npm-range peers are collected. A peer starts out provided if the owner of that directory installs an in-range copy, and is not considered at all if the owner installs an out-of-range one, since that copy is what the package would find either way. Walking up, an ancestor holding an in-range version marks the peer provided; one holding an out-of-range version stops the hoist if the peer is provided and drops the peer otherwise, because a nested copy would resolve the same version the hoisted one does. Only ranges and the resolutions of entries already in the tree are compared; peer bindings are never read. Optional peers, peers shadowed by a regular dependency of the same name, and workspace or link: packages (installed as symlinks) are left alone. Fixes #20376
73f11e3 to
b790347
Compare
3 participants
Problem
node_moduleseven when that ancestor holds a version of one of its peer dependencies that the peer range rejects, while the package that pulled it in has the right version nested next to itself. From the hoisted positionrequire(peer)finds the wrong version, and when the peer is shared state between the two (ajv/ajv-errors) they end up with different module instances. Reported as Bug(works in npm): Using Orval to generate API client fails after installing deps usingbun install#20376:ajv@6and@stoplight/spectral-core; spectral-core depends onajv@^8andajv-errors, andajv-errorshaspeerDependencies: { ajv: "^8" }.ajv@8nests under spectral-core, butajv-errorsis hoisted to the root, next toajv@6.Tree::hoist_dependency(src/install/lockfile/Tree.rs) only looks at whether an ancestor already holds an entry with the same name as the package being hoisted. The package's own peer edges are not consulted.Fix
When
hoist_dependencystarts walking up from the directory a package was declared in,Builder::begin_peer_probescollects the package's required peers with an npm range (optional peers,catalog:/dist-tag/git peers, and peers the package also lists as a regular dependency are skipped; nothing is collected for workspace orlink:packages, which install as symlinks). If the package that owns that directory has a regular dependency on the peer, that copy is the one the package finds from there, so the probe starts outprovidedwhen it is in range and is not created at all when it is not. Otherwise the probe starts out not provided.At every ancestor,
Builder::probe_peerslooks at the entry with the peer's name, if any. In range: the probe becomesprovided(the package would still see that copy if it stayed put). Out of range: if the probe isprovided, the hoist stops and the package is placed where it was declared (DependencyLoop, the existing result for a conflict); otherwise the probe is dropped, because a nested copy would resolve the same out-of-range version the hoisted copy does, so the layout stays what it is on main (one shared copy).Only version ranges and the resolutions of entries already placed in the tree are compared (
Resolution::satisfies_dependency_version, the comparison the resolver binds peers with). The peer edge's own binding is never read: for required peers it is the highest in-range version anywhere in the lockfile, so comparing ids nested a copy whenever two in-range versions existed, and for non-npm ranges it is re-derived from the printed tree on reload, so a decision based on it would not survive a second install.The probe runs before the same-name match on purpose: when the root holds the package next to an out-of-range peer and a consumer provides an in-range one, deduplicating onto the root copy is the bug.
Not covered: a peer-dependent package whose dependent was itself hoisted away from the provider (root
no-deps@1,outer -> { mid, no-deps@2 },mid -> childwith a peer onno-deps@2).midhoists to the root, so nothingchildcould stay below holdsno-deps@2, and it is hoisted to the root and resolvesno-deps@1, as on main. Placing it correctly means placing the peer next to it (peer-set placement), which this change does not attempt.Also known: the owner-copy seed in
begin_peer_probesreads the owner's full edge list, so in the install-time tree an edge omitted by--production/--omitcan seed a probe. Either direction only adds or skips a nested copy; the package never resolves a different version than it does on main, and the pass that writesbun.lockplaces every edge, so it is exact there. Left for a follow-up.No existing test expectation changes. New tests in
test/cli/install/hoist.test.ts(run concurrently), each installing from scratch and again from the writtenbun.lock:stays next to the peer version it was resolved against: the shape above on registry fixtures. Fails on main.stays below a peer version provided further up than its own dependent:mismatched-peer-deps-lvl0 -> lvl1 -> lvl2, all with peers onno-deps, under a workspace that provides an in-rangeno-depswhile the root pins an out-of-range one. Fails on main (all three are hoisted to the root).hoists past an out-of-range version when nothing below would satisfy the peer either: root pins the peer out of range, a 1.x exists elsewhere in the lockfile, two workspaces depend on the package without providing the peer. One copy at the root; this is the shape the previous revision duplicated.hoists when its own dependent installs an out-of-range copy of the peer: the dependent (a tarball kept under a workspace by a name clash at the root) nests an out-of-rangeno-depsunder itself while the workspace above holds an in-range one; staying put would find the dependent's copy, so the package hoists as on main.hoists when the version at the root satisfies the peer range: two in-range versions, the lower one at the root; the dependent stays shared. This is the shape an id comparison duplicated.ignores a peer that the package also lists as a regular dependency(tarball, sincePackage::from_npmalready drops that peer edge for registry packages) andnever nests a workspace package.Verified on the reporter's repro and on the smallest shape with a script that resolves every required peer from the dependent's directory the way node does, and also checks that a package and the package depending on it resolve the peer to the same directory:
bun.lock)bun.lockbun.lock)On this branch the orval repro's
bun.lockis unchanged by a second install andorvalruns to completion. Also run with the debug build:bun-install-registry.test.ts(all),bun-lock.test.ts,bun-workspaces.test.ts, and the peer tests inisolated-install.test.ts.Rebased (squashed to one commit) after install: collapse a workspace's same-name dependency slots into one entry so --frozen-lockfile is stable #36303 and install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 rewrote
hoist_dependency. Two mechanical adjustments: it no longer returns aResult, and since it now receives the declaring package's dependency range (input_dep_range),begin_peer_probestakes that directly instead of looking the owner up through the tree'sdependency_id. The probe logic itself is unchanged from the reviewed revision; on the rebased main the two fixing tests still fail with this PR'ssrc/changes reverted and all eight pass with them, andbun-install-registry.test.ts,bun-lock.test.tsandbun-workspaces.test.tspass.Background
node_moduleslayout from the resolved lockfile inTree.rs. Each tree is onenode_modulesdirectory, built breadth first, so every ancestor is complete by the time a directory's own entries are placed.hoist_dependencywalks from the directory a dependency was declared in (AS_DEFINED) up through its ancestors and either dedupes onto an entry with the same name, keeps walking, or, on a conflict, places the package back in the directory it was declared in.resolve_peer_dep_version_basedreproduces this on load), and optional,*and non-npm peers are bound to whatever ends up next to them. This change does not read those bindings.hoist_dependencyhas a root-dependency override: a peer edge that reaches a version pinned by the rootpackage.jsondedupes onto it even when the range rejects it. This PR does not change that; it is why nesting a package whose peer is provided nowhere below the root would change nothing, and why such conflicts are dropped.Fixes #20376
[review] gate passed · iteration 11 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 2 rejected · iteration 11
evidence per changed file