install: fix the package id boundary in Lockfile::eql

[robobun]

Problem

• bun install --frozen-lockfile fails with error: lockfile had changes, but lockfile is frozen on a bun.lock whose tree has not changed, as soon as the file contains an entry that nothing depends on and that entry is not the last one in the file. Field case: activepieces/activepieces at HEAD, whose committed (1.3-written) lockfile 1.3.14 accepts; with install: keep optional-peer-held packages when the lockfile is frozen #38853 applied, main still rejects it.
• Lockfile::eql (src/install/lockfile.rs) compares the cleaned lockfile's tree with the tree the loaded lockfile was hoisted into at load time. It took a cut_off_pkg_id and left out every placement bound to a package id at or past it, on both sides. The callers pass the cleaned lockfile's package count. On the cleaned side that is a no-op. On the loaded side it does two things: it catches edges that resolving rebound to packages appended after load (those ids are past the loaded count; the loaded tree still places them, so leaving them out makes the counts differ; bun update --frozen-lockfile moving a transitive dependency relies on this, bun-update-transitive.test.ts), and, as soon as the clean dropped an entry, it also leaves out the loaded side's own last packages, whose ids are past the cleaned count but below the loaded one. That second effect is the bug: a real placement disappears from the comparison and the counts differ.
• In activepieces the dropped entry is react-dom@18.3.1: the file nests it under react-json-view for a peer range (^15 || ^16 || ^17) nothing in the file satisfies, and since install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182 the loader binds that edge by version, falling back to the first candidate (react-dom@19.2.5 at the root), so the nested copy is referenced by nothing and the clean drops it (4069 -> 4068 packages). The file's last entry, mdast-util-find-and-replace@1.1.1 (id 4068), is placed twice; both placements were left out on the loaded side. The two hoists otherwise agree placement for placement (trace in the details below).

Fix

• The boundary on the loaded side is now the loaded lockfile's own package count at load time, which mark_loaded_packages already records for the resolver (loaded_package_count); the two callers in install_with_manager.rs pass lockfile_before_clean.loaded_package_count. A placement bound past it was rebound to a package appended by this install, so eql now returns false on it directly instead of leaving it out and relying on the counts. The cleaned side is compared whole. The debug-build determinism check in save_lockfile compares a lockfile with itself and keeps passing its own count.
• Correct because the two things the old boundary mixed up are now separate: a rebound edge is a changed resolution and fails, and a package the clean dropped only matters if it was placed in the loaded tree, in which case its placements are now counted on the loaded side and missing on the cleaned side, which still fails. An entry that was in no tree no longer affects the result.
• Visible consequence outside the frozen check: the same comparison decides whether a non-frozen install re-saves, so a no-op bun install on a lockfile whose only difference is an entry outside the tree no longer rewrites the file just to drop it; it is dropped the next time anything else causes a save. Installs with a package.json diff still save regardless (had_any_diffs).
• Test: test/cli/install/bun-lock.test.ts, "--frozen-lockfile accepts a bun.lock with an entry nothing depends on, wherever it is listed". A hand-written lockfile with no-deps (depended on) and a-dep (not depended on) in both orders; on main the order with a-dep first fails with the error above, with this change both orders install no-deps only and leave the file untouched. The rebound case stays covered by the existing bun update --frozen-lockfile test in bun-update-transitive.test.ts, which failed on the first version of this PR (it dropped the loaded-side check entirely) and passes now.
• With this change plus install: keep optional-peer-held packages when the lockfile is frozen #38853, the committed lockfiles of activepieces, opencode, eliza and supermemory pass --frozen-lockfile with the debug build (activepieces still logs 4069 -> 4068 and passes); hono and remotion still pass, and a further 72 small repos with a committed bun.lock behave identically on 1.3.14, main and the two changes combined (65 pass everywhere, 7 are already out of date everywhere). Suites run with the debug build: bun-lock, bun-update-transitive, bun-update, bun-add, bun-remove, migration/migrate, frozen-lockfile-pruned, frozen-lockfile-missing-workspace, lockfile-only, bun-lockb, bun-workspaces, isolated-install, bun-dedupe, bun-prune, hoist, catalogs, overrides, nested-overrides, lockfile-version-2, bun-install-registry.
• Not changed here: the loader binding an out-of-range peer to a different version than the entry the file nests next to the dependent (the producer of the unreferenced entry above). That rewrites such 1.3 lockfiles on the next non-frozen install and changes which copy the dependent gets; it is the same family as install: bind peer edges in the hoister, the way loading bun.lock binds them #38767 / install: rebind ranged peers whose target the saved tree drops #38768 / install: keep bundled peer edges on their own bun.lock entry when loading #38837 and is tracked separately.

Background

• bun.lock stores the hoisted tree as keys: a/b means b is installed in a's node_modules. Loading the file creates one package per distinct entry (ids in file order), binds every dependency edge, and hoists the result into an in-memory tree. mark_loaded_packages then records the package count; anything resolving appends afterwards (a newer version bun update picked, a package added to package.json) gets a higher id, and edges may be rebound to those packages in place, while the tree built at load time is not rebuilt.
• clean_with_logger builds a fresh lockfile by walking from the root (ids in walk order), so packages no edge reaches are dropped, and hoists it again. --frozen-lockfile compares that tree with the load-time tree using Lockfile::eql: placements are listed as (path, package), sorted, and compared pairwise on name, resolution, bins and scripts. The same comparison decides after a normal install whether the lockfile is saved again. bun.lockb uses a hash over the package list instead (packages_len_before_install still feeds that).

Hoist trace on activepieces (temporary instrumentation, not part of the change)

Every decision the hoister made for mdast-util-find-and-replace, first while loading the file (ids 2628/4068), then during the clean (ids 1344/2474). The two runs are identical; only the comparison differed.

hoist dep_id=10546 pkg_id=2628 parent_pkg_id=2640 tree_node=858  -> Placement(ancestor)
hoist dep_id=10499 pkg_id=2628 parent_pkg_id=2631 tree_node=1120 -> Hoisted(dedupe)
hoist dep_id=16465 pkg_id=4068 parent_pkg_id=4043 tree_node=1222 -> Placement(own node)
hoist dep_id=16465 pkg_id=4068 parent_pkg_id=4043 tree_node=1227 -> Placement(own node)
hoist dep_id=3661  pkg_id=1344 parent_pkg_id=1347 tree_node=858  -> Placement(ancestor)
hoist dep_id=3651  pkg_id=1344 parent_pkg_id=1343 tree_node=1120 -> Hoisted(dedupe)
hoist dep_id=7043  pkg_id=2474 parent_pkg_id=2473 tree_node=1222 -> Placement(own node)
hoist dep_id=7043  pkg_id=2474 parent_pkg_id=2473 tree_node=1227 -> Placement(own node)

Placement diff reported by an instrumented eql (raw hoisted_dependencies lengths were equal; only the filtered lists differed, by exactly the two placements of loaded id 4068):

only in cleaned: @tryfabric/martian/remark-gfm/mdast-util-gfm/mdast-util-gfm-autolink-literal :: mdast-util-find-and-replace@1.1.1
only in cleaned: slackify-markdown/remark-gfm/mdast-util-gfm/mdast-util-gfm-autolink-literal  :: mdast-util-find-and-replace@1.1.1

First version of this PR

The first push removed the boundary altogether, on the reasoning that nothing appends packages during the install step any more. That is true of the step after the clean, but resolving before the clean still appends and rebinds, and the loaded tree is not rebuilt, so bun update --frozen-lockfile on a stale lockfile was accepted (bun-update-transitive.test.ts caught it in CI). The current version keeps that check and only moves the boundary to the loaded side's own count.

---

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: 11 minutes

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: 0d8db83e-b0d3-467f-9144-599c8630b7b8

📥 Commits

Reviewing files that changed from the base of the PR and between 87b26b5 and c7e8840.

📒 Files selected for processing (3)

• src/install/PackageManager/install_with_manager.rs
• src/install/lockfile.rs
• test/cli/install/bun-lock.test.ts

---

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

[robobun]

Reproduced on activepieces/activepieces at HEAD: its committed bun.lock passes bun install --frozen-lockfile on the 1.3.14 release binary and fails on main even with #38853 applied. Instrumenting the hoister showed the loaded and cleaned trees agree placement for placement; the difference came from Lockfile::eql leaving the loaded side's last package (placed twice) out of the comparison once the clean had dropped one unreferenced entry. The reduced form is the new test in test/cli/install/bun-lock.test.ts: a two-entry lockfile fails on main when the unused entry is listed first and passes when it is listed last; with this change both orders pass.

Current state: the first version of the change dropped the loaded-side boundary entirely and was caught by bun-update-transitive.test.ts in CI; the current version keys the boundary to the loaded lockfile's own package count instead (see the description). Together with #38853 this makes all four of the committed lockfiles I found regressing against 1.3.14 (opencode, eliza, supermemory, activepieces) pass --frozen-lockfile with the debug build; 72 further repos behave identically before and after. The two changes are independent; activepieces needs both.

[robobun]

^{Updated 11:05 PM PT - Aug 14th, 2026}

❌ @robobun, your commit c7e8840 has some failures in Build #97375 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38870

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

bun-38870 --bun

[claude]

I reviewed this PR and didn't find any bugs. Because it changes the semantics of Lockfile::eql — the comparison behind --frozen-lockfile and the "should bun.lock be re-saved" decision on every install — a human look would still be worthwhile.

The three eql call sites are the only ones in the tree and all are updated; no stray cut_off_pkg_id references remain. The removed filter only ever applied to the loaded side (on the cleaned side no package id can reach the cleaned count), so dropping it cannot mask a real difference — it can only stop hiding placements that were being wrongly skipped. The debug-build save_lockfile call compares the lockfile with itself via ParentRef, so the parameter was already a no-op there. The new test exercises both entry orderings, asserts the install succeeds, checks the on-disk tree, and verifies bun.lock is left byte-identical under --frozen-lockfile.

Extended reasoning...

Overview

Removes the cut_off_pkg_id parameter from Lockfile::eql (src/install/lockfile.rs) and updates its three call sites: the frozen-lockfile check and the re-save check in install_with_manager.rs, and the debug-only determinism assertion in save_lockfile (PackageManagerDirectories.rs). Adds a regression test in test/cli/install/bun-lock.test.ts with a hand-written two-entry lockfile where one entry is unreferenced, run in both file orders.

Security risks

None. No parsing of untrusted input is touched; the change only removes an id-range filter from an in-memory tree comparison. No new I/O, no path handling, no allocation-size arithmetic.

Level of scrutiny

High. --frozen-lockfile is the CI reproducibility guarantee, and eql also gates whether bun.lock is rewritten on a plain install. A false positive here spuriously fails CI (the reported bug); a false negative would silently accept a drifted lockfile or skip a needed re-save. The change is small and the mechanism is well-argued — the parameter's own doc comment already said it "should be removed when we stop appending packages to lockfile during install step", and save_lockfile's parameter comment says the same — but the blast radius is every install.

Other factors

• The removed condition could only affect the loaded side of the comparison: cut_off_pkg_id is the cleaned lockfile's package count, so no cleaned-side package id can reach it. The filter was therefore asymmetric and semantically meaningless once the two sides stopped sharing an id space.
• Verified all three callers are updated and no other reference to cut_off remains under src/install/.
• packages_len_before_install is retained for the bun.lockb meta-hash path, which is unchanged.
• The test follows harness conventions (await using on the spawned process, drains stdout/stderr/exited concurrently, asserts a combined {order, err, exitCode} object so the failing order is visible, uses the Verdaccio registry via registry.createTestDir()). It also asserts the negative contract (a-dep not installed) and that bun.lock is untouched.
• The PR description reports validation against nine real-world lockfiles and twelve install test files; that plus the depth of the root-cause trace gives high confidence, but a maintainer familiar with the history of git-dep appending during install (the original reason for the cut-off) is best placed to confirm nothing else still relies on it.

[robobun]

Revised after the first CI run. The first push removed the boundary from eql entirely; bun-update-transitive.test.ts ("bun update --frozen-lockfile refuses to move the transitive dependency") failed on every lane, which shows the loaded-side check is still needed: resolving appends the moved version to the loaded lockfile and rebinds the edge in place while the load-time tree still places it, and the boundary was what made that show up as a difference. (The review above reasoning that the boundary could not mask anything missed the same thing I did.)

The current version keeps that check and moves the boundary to the loaded lockfile's own count (loaded_package_count, recorded by mark_loaded_packages), failing the comparison directly on a rebound placement. That separates the two cases cleanly: a rebound edge still fails, an entry the clean dropped only matters if it was actually placed in the loaded tree. Description rewritten for the current shape; the old version is in a details block at the bottom.

Re-run with the debug build: bun-update-transitive, bun-lock (including the new test), bun-update, bun-add, bun-remove, migration/migrate, the frozen-lockfile suites, lockfile-only, bun-lockb, bun-workspaces, isolated-install, bun-dedupe, bun-prune, hoist, catalogs, overrides, nested-overrides, lockfile-version-2 and bun-install-registry all pass. activepieces, opencode, eliza and supermemory still pass --frozen-lockfile with this plus #38853, and 72 further repos with a committed bun.lock behave identically on 1.3.14, main and the two changes combined.

[claude]

I reviewed this PR and the automated bug hunt found no issues. The fix is well-reasoned and thoroughly tested against real-world lockfiles, but it changes the semantics of Lockfile::eql in a way that also affects when a non-frozen install re-saves (an unreferenced entry outside the tree is no longer dropped on a no-op install), and the first iteration already broke bun-update-transitive.test.ts. Given the subtlety of the boundary invariant and the visible behavior change, a human look would be worthwhile.

Checked: the two install_with_manager.rs callers now key the boundary to lockfile_before_clean.loaded_package_count; the third caller in save_lockfile's debug determinism check still passes packages_len_before_install as documented. Verified the early return Ok(false) on a rebound placement replaces the old skip-and-rely-on-count path. Verified mark_loaded_packages runs unconditionally after load so loaded_package_count is set on lockfile_before_clean. The new test covers both entry orders and asserts the lockfile is left byte-identical.

Extended reasoning...

Overview

The PR fixes Lockfile::eql in src/install/lockfile.rs and updates its two callers in src/install/PackageManager/install_with_manager.rs, plus adds a regression test in test/cli/install/bun-lock.test.ts. The core change: the package-id boundary that filters placements out of the comparison now applies only to the loaded (r) side and is keyed to r's own loaded_package_count (recorded by mark_loaded_packages right after load) rather than the cleaned lockfile's package count. A placement bound past that count is a resolution rebound during this install and now returns false directly instead of being skipped on both sides.

Security risks

None. This is pure comparison logic over in-memory lockfile state; no parsing of untrusted input, no I/O paths, no auth/crypto.

Level of scrutiny

High. Lockfile::eql gates two user-visible behaviors: whether --frozen-lockfile accepts a lockfile, and whether a non-frozen install re-saves bun.lock. A wrong true silently accepts a stale lockfile in CI; a wrong false breaks reproducible installs (the reported regression). The first version of this PR already got it wrong (dropped the boundary entirely, caught by bun-update-transitive.test.ts), which underlines how subtle the invariant is.

Other factors

• Intentional behavior change: the PR description calls out that a no-op bun install on a lockfile whose only difference is an unreferenced entry no longer rewrites the file to drop it. That is a product decision (arguably correct — the tree is unchanged) that a maintainer should acknowledge.
• Third caller: the debug-build determinism check in save_lockfile (PackageManagerDirectories.rs:1170) still passes packages_len_before_install. Since it compares the lockfile with itself post-install, the new early-return-on-rebound semantics would trip the debug panic if the install step ever appends and rebinds a placed dependency. The author ran ~20 install suites in debug without hitting it, and the old doc comment ("should be removed when we stop appending packages during install step") suggests that no longer happens — but this is worth a maintainer's confirmation.
• Test coverage: the new test exercises both orderings and asserts exit code, stdout, node_modules layout, and that the lockfile is untouched. The rebound case is covered by the existing bun-update-transitive test that caught v1.
• Field validation: verified against activepieces, opencode, eliza, supermemory plus 72 other repos.
• The comment-cop bot's feedback on comment length was addressed in follow-up commits (now a single-line doc comment).

[robobun]

On the save_lockfile debug check: with the new parameter meaning, passing packages_len_before_install there says "any placement bound to a package appended during the install step is a change", which is what that check was written to detect in the first place (the git dependency appends the NOTE above it refers to), so it is left as it was rather than made vacuous. It runs on every lockfile save in debug builds, and every suite listed in the description was run with the debug build (bun bd), so the roughly 1,500 installs in bun-install-registry, bun-add, bun-update, bun-remove, bun-workspaces, isolated-install, bun-install-git-deps, the migration suite and the rest all went through it without tripping it. If something did start appending and placing packages after the clean again, a debug-build panic pointing at it is the behaviour we would want.

The re-save change (an entry outside the tree no longer forces a rewrite on a no-op install) is called out in the description for the same reason: it falls out of comparing what is actually installed, and it is the part a maintainer may want to weigh in on.