install: let the hoister bind optional peers when loading bun.lock - #37925
install: let the hoister bind optional peers when loading bun.lock#37925robobun wants to merge 1 commit into
Conversation
The bun.lock parser bound optional peer edges by walking the printed package paths, while a fresh install and Lockfile::clean_with_logger (Package::clone resets these edges) both leave them for Tree::process_subtree to bind. An edge bound up front hoists differently: it escapes the hoist root of a bundled dependency's subtree, and it places its target's subtree earlier in the breadth-first fill. The tree built at load time then differs from the tree the clean rebuilds, and Lockfile::eql reports a lockfile bun itself just wrote as changed, so --frozen-lockfile fails. #37350 bounded the walk at entries marked "bundled", which covers a bundled package's own optional peers but not the optional peers of its dependencies (hoisted to the same bundle root, but printed without a marker). Skip optional peers in all three binding loops instead and let the resolve() call at the end of parsing bind them exactly like the other two paths do; the bounded walk is no longer needed. The next-pages lockfile dumps change in one field: the root jiti slot is now owned by its prod dependency edge instead of eslint's optional peer edge, as in a fresh install. Same package id.
|
Warning Review limit reached
Next review available in: 8 minutes 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 ignored due to path filters (3)
📒 Files selected for processing (6)
Comment |
|
Updated 6:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 078a59d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37925That installs a local version of the PR into your bun-37925 --bun |
|
Status: reproduced, fixed, ready for review. CI is green everywhere it ran; the red jobs are infrastructure (details below). Reproduced with the new registry fixtures in CI (build 93533, finished): 170 jobs passed, including the install suites and the new test on every lane that produced a binary. The three jobs that did not pass are all infrastructure from the run: the windows aarch64 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how the bun.lock parser binds optional peer edges — package-manager core semantics that interact with the hoister and with the related open work in #35571 and #37426 — a human look would still be worthwhile.
What was reviewed:
- The three binding loops in
parse_into_binary_lockfilenow skip optional peers beforeis_deferred_peer/find_resolution, andlockfile.resolve()at the end still runs the hoister that binds them — same code path asclean_with_logger. find_resolution_bounded_at_bundleand itsbundled_pkgsargument are fully dead after the skip;bundled_pkgsitself remains used by theCHECK_FOR_BUNDLEDbehavior tagging at ~L2520/L3352.- The next-pages snapshot change (jiti slot owner 434→937, same
package_id316) matches the description: the optional-peer edge is no longer the one that claims the root slot during parse. - New test mirrors the existing #37346 test one level deeper and asserts warm + cold
--frozen-lockfileplus byte-identical re-save.
Extended reasoning...
Overview
The PR changes src/install/lockfile/bun.lock.rs so that the three dependency-binding loops in parse_into_binary_lockfile (root, workspaces, packages) skip optional peer edges entirely, leaving them for the trailing lockfile.resolve() call — the same hoister that clean_with_logger uses after Package::clone resets those edges. The find_resolution_bounded_at_bundle helper introduced in #37350 (a partial fix bounded at "bundled": true entries only) is removed and find_resolution is restored to its pre-#37350 form. A new test in bun-lock.test.ts reproduces the transitive-bundled case with two new Verdaccio fixtures, and three next-pages lockfile-dump snapshots are updated for the one field whose owning dep_id shifts.
Security risks
None. This is purely lockfile-parsing/hoist-tree semantics; no untrusted-input parsing surface, network, filesystem write, or auth path is touched.
Level of scrutiny
High. parse_into_binary_lockfile is the load path for every bun install that has a bun.lock, and its output feeds both --frozen-lockfile's Lockfile::eql comparison and the did_meta_hash_change re-save trigger. The change is small in surface (three continues + a helper removal) but shifts which code binds optional peers at load time, and the PR description explicitly notes overlap with two other open PRs (#35571 carries the same three-loop skip; #37426 changes the clean side). A maintainer who owns the hoister invariants and is tracking those threads should confirm the sequencing and that the snapshot change (root jiti slot owned by dep 937 instead of eslint's optional peer 434, same package_id) is the expected shape.
Other factors
The correctness argument is well-stated and I could follow it against the code: Package::clone at Package.rs:610 resets optional peers, enqueue_dependency_with_main_and_success_fn returns before resolving them, so the parser was the one path that bound them from printed paths — and a path-walk binding places the target earlier and ignores a bundle-subtree hoist root that only the hoister knows about. Deferring to lockfile.resolve() makes the parsed tree and the cleaned tree come from identical inputs and identical code. bundled_pkgs is still live for Behavior::BUNDLED tagging, so removing the bounded-walk helper leaves nothing dead. The new test is placed alongside the existing #37346 test, uses local registry fixtures, asserts both warm and cold --frozen-lockfile, and checks that the lockfile is byte-identical after. The PR reports the full install-related suite passing on a debug build. No bugs found by the multi-agent hunt or by my read.
|
On the sequencing question for whoever picks this up:
|
2 participants
Problem
bun installwritesbun.lock, thenbun install --frozen-lockfileon the unchanged project fails witherror: lockfile had changes, but lockfile is frozen(warm or afterrm -rf node_modules), when a dependency bundles a package whose own dependency has an optional peer that the root satisfies. Plainbun installreports no changes and re-saves the file byte-identically.Lockfile::eqlbetween the lockfile as parsed and the oneclean_with_loggerrebuilds from it (install_with_manager.rs:710).eqlcompares the hoisted trees.Package::cloneresets every optional peer edge and letsTree::process_subtreebind it during the clean (lockfile/Package.rs:610); a fresh install does the same, sinceenqueue_dependency_with_main_and_success_fnreturns before resolving optional peers (PackageManagerEnqueue.rs:635). Thebun.lockparser was the one path that bound these edges itself, by walking the printed package paths (bun.lock.rs, the three binding loops inparse_into_binary_lockfile).bop/mid(a dependency of the bundledbop/opd, hoisted to the bundle root, printed without a"bundled"marker) has an optional peer ontdep; the walk finds the roottdepand binds it, and the hoister then placestdepunderbopbecausemidinherits the bundle's hoist root (Tree.rs:943). The clean leaves the edge unbound and places nothing, so the trees differ by one placement."bundled": true, which covers the bundled package's own optional peers but not those of its dependencies (its docstring noted this gap). The same mismatch also shows up without bundles: a bound edge places its target's subtree earlier in the breadth-first fill, so a version conflict under the target can hoist differently (this is the shape described in install: keep optional peer bindings when cleaning the lockfile #37426;test/bun.lockin this repo fails--frozen-lockfileon main because of it).Fix
lockfile.resolve()call at the end of parsing, so the tree built at load time comes from the same inputs and the same code as the tree the clean builds. The bounded walk from install: don't resolve a bundled dependency's optional peer past the bundle hoist root when loading bun.lock #37350 is removed, since nothing uses it anymore;find_resolutionis back to its previous form.eqlstops reporting a difference (--frozen-lockfile, and thedid_meta_hash_changere-save trigger on the same comparison).test/cli/install/bun-lock.test.ts, new test "dependency of a bundled dependency has an optional peer satisfiable from the root" with two new registry fixtures (bundled-transitive-optional-peerbundlesdepends-on-optional-peer-deps, which depends onoptional-peer-deps, which has an optional peer onno-deps). Fails on main (including install: don't resolve a bundled dependency's optional peer past the bundle hoist root when loading bun.lock #37350) with the error above, passes with the fix. The --frozen-lockfile rejects bun's own lockfile output (1.4.0-canary) #37346 test still passes.cd test && bun install --frozen-lockfilepasses with the fix (fails on main) and--lockfile-onlyre-savestest/bun.lockbyte-identically.bun-lock(19),isolated-install(62, includes the install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182 optional peer tests),bun-install-registry(229),bun-lockb,hoist,migrate-bun-lockb-v2,bun-workspaces,lockfile-version-2,catalogs,bun-update,bun-pm-why,migration/pnpm-*all pass.next-pagessnapshots are lockfile dumps taken through the parser. Dumping the committedtest/integration/next-pages/bun.lockbefore and after differs in exactly one field, updated here: the rootjitislot is owned by dependency 937 (jiti@^1.21.6, prod) instead of 434 (eslint's optional peerjiti@*),package_id316 either way, and dependency 434 still resolves to 316.Background
bun.lockstores the hoisted tree as keys:a/bmeansbis installed ina'snode_modules. On load, a regular edge of packagea/bis bound by looking upa/b/<name>, thena/<name>, then<name>, mirroring node resolution.Tree::process_subtree/hoist_dependency) places each package as high in the tree as it can, walking up from the dependent'snode_modules. It runs breadth-first; a placed package's dependencies are queued and processed later, so the point at which a package gets placed decides how later conflicts under it are laid out.node_modulesand makes that folder a hoist root: nothing in the bundle's subtree (the bundled package or its dependencies) is hoisted or resolved above it. Only the directly bundled package is printed with"bundled": true.Resolve), or later when such a package gets placed next to the unbound edge (ResolveReplace); otherwise the edge stays unbound and is dropped from the tree.--frozen-lockfiledoes not compare files: it parses the lockfile, runs the same clean a normal install runs, and compares the two in-memory trees withLockfile::eql.Lockfile written by the fixture (same before and after; only loading it changes)
On main, loading this binds
optional-peer-deps' peer to the rootno-depsand the load-time tree gains abundled-transitive-optional-peer/no-depsplacement that the clean never produces: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