install: collapse a workspace's same-name dependency slots into one entry so --frozen-lockfile is stable

[robobun]

Fixes #19088

Problem

• bun install followed by bun install --frozen-lockfile fails with error: lockfile had changes, but lockfile is frozen when a workspace package lists the same dependency name in two dependency groups (issue thread: vitest in dependencies and devDependencies; opencode v0.9.9: @hey-api/openapi-ts).
• A workspace package.json is parsed with Features::WORKSPACE, which has check_for_duplicate_dependencies: false (src/install_types/resolver_hooks.rs:1299), so the package ends up with two dependency slots for one name. The root package is parsed with the check on, but that only collapses an optionalDependencies duplicate; a devDependencies duplicate is kept as a second slot with a warning and a peerDependencies one is not checked at all. The root was still unaffected because its slots collide at its own level, where the old dev check (or, for peers, the root-dependency peer rule below it) already merged them; a workspace's slots collide in a parent tree instead.
• Tree::hoist_dependency (src/install/lockfile/Tree.rs) only merged the two slots when one was dev and the other was not, and only when they collided at the package's own level (AS_DEFINED). In a workspace the first slot is usually hoisted into the root tree before the second is processed, so the merge never fired and both slots were written to bun.lock (baz hoisted plus pkg-a/baz nested).
• On reload the text lockfile parser resolves both slots by tree path, so both point at the nested entry and the hoisted one is orphaned; clean_with_logger then builds a different tree than the one loaded and the frozen check rejects it.
• Same mechanism for dependencies + peerDependencies (when the peer resolves to a different package, for example because a sibling workspace pins it) and for dependencies + optionalDependencies. When the root additionally pins a third version of the name, the optional shape did not get as far as the frozen check: the two slots collided inside the workspace's own folder and install failed outright with Package "baz@0.0.5" has a dependency loop.

Fix

• Thread the package's own dependency range (resolution_list, already in hand in process_subtree) into hoist_dependency, and merge whenever the colliding entry is inside that range. Two slots of one package can only ever share one node_modules/<name> folder, whatever their groups, so the range check is the whole condition; registry packages never get here because Package::from_npm already drops such duplicates.
• Everything placed at a package's own level comes from its own range, so the AS_DEFINED "dependency loop" error after the peer checks can no longer be reached. Deleted it together with its plumbing, none of which had another constructor: SubtreeError::DependencyLoop, Error::DependencyLoop, MigratePnpmLockfileError::DependencyLoop, and the ParseError::InvalidPackagesObject mapping for it in bun.lock.rs. hoist_dependency now returns HoistDependencyResult directly, which also removes the unreachable_unchecked on the recursive call. That error was also the only &mut use inside the lookup loop, so the raw-pointer detachment of the tree's dependency list is gone too (the loop iterates the slice); the refuse_declared_positionals doc comment in add_catalog.rs, which named the old error as the fallback, now describes the collapse that would actually happen without the guard; and the two not.toContain("dependency loop") assertions in bun-add-filter.test.ts are removed since nothing emits that message any more.
• Which slot wins: the one the hoister processes first, i.e. the DepSorter order dev, optional, prod, peer. This is what the root package has always done (it pins the devDependencies entry, with a "Duplicate dependency" warning, or the optionalDependencies entry; a workspace prints nothing either way). It is a change for regenerated workspace lockfiles: on main such a workspace eventually settled on the dependencies entry once bun install had been run twice; after this change a regenerated lockfile pins the devDependencies / optionalDependencies entry instead, including under --production. Lockfiles that are already stable are left alone: loading one produced by main and running either bun install or bun install --frozen-lockfile with this build keeps it byte-identical (checked by hand on the dev shape).
• Verified with the table test added to test/cli/install/bun-install.test.ts (--frozen-lockfile passes after a workspace lists a name in ...): four shapes (dev; optional; optional while the root pins a third version; peer while a sibling pins the peer's version). Each asserts the packages section of the generated lockfile and that both a frozen and a plain reinstall leave the file byte-identical. With src/ at main, all four fail (the dev, optional and peer rows with an extra pkg-a/baz entry, the root-pin row with the dependency loop error); with this change all four pass.
• Also ran bun-install.test.ts (only the pre-existing network-dependent git tests fail in this container) and bun-add.test.ts; cargo clippy -p bun_install is clean.

Background

• Dependency slot: each entry of a package.json dependency group becomes one Dependency in lockfile.buffers.dependencies; a package owns a contiguous range of them (resolution_list), and buffers.resolutions maps each slot to a resolved package id. Two slots with the same name are therefore possible even though only one folder can be installed for that name.
• Hoisting: for each package, process_subtree creates a tree node for the package's folder and, for every slot, walks up towards the root looking for a folder that already holds that name. Same package id means reuse (Hoisted); a different package id normally means the slot is placed in the current folder instead (the DependencyLoop result, which only names the outcome of the walk, is unrelated to the deleted error). The new check sits in that walk: a same-name entry that came from the same package's range is a second slot for a folder that already exists, so it is reused rather than placed.
• --frozen-lockfile does not diff files. It loads bun.lock, re-resolves and re-hoists into a fresh lockfile, and compares the two trees (Lockfile::eql), so any shape the hoister cannot reproduce from its own output trips it even when bun.lock itself is unchanged.

Repro

mkdir -p repro/packages/a && cd repro
echo '{"name":"root","private":true,"workspaces":["packages/*"]}' > package.json
echo '{"name":"pkg-a","version":"1.0.0","devDependencies":{"is-number":"6.0.0"},"dependencies":{"is-number":"7.0.0"}}' > packages/a/package.json
bun install
bun install --frozen-lockfile
# error: lockfile had changes, but lockfile is frozen

Swap devDependencies for optionalDependencies, or for peerDependencies plus a second workspace that depends on the peer's version, for the other two shapes. Add "dependencies":{"is-number":"5.0.0"} to the root with the optional shape for the dependency loop error.

---

no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts

[coderabbitai]

Walkthrough

Changes

Dependency hoisting

Layer / File(s)	Summary
Propagate dependency range context src/install/lockfile/Tree.rs	process_subtree and recursive hoisting calls pass the original dependency-id range into hoist_dependency.
Handle same-source dependency groups src/install/lockfile/Tree.rs, test/cli/install/bun-install.test.ts	Hoisting recognizes same-source dependencies in different dev or optional groups, with tests covering frozen-lockfile stability and selected package resolution.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR addresses #19088 by stabilizing frozen installs for workspace lockfiles and includes the relevant duplicate-dependency fix.
Out of Scope Changes check	✅ Passed	The code changes and new test stay focused on the lockfile hoisting regression and related verification.
Title check	✅ Passed	The title clearly summarizes the main change: merging same-name workspace dependency slots to stabilize frozen lockfiles.
Description check	✅ Passed	The description explains the problem, fix, scope, implementation details, and verification results, despite not using the template headings exactly.

---

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

[robobun]

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

❌ @robobun, your commit 6c251e2 has 1 failures in Build #97327 (All Failures):

• test/bake/deinitialization.test.ts - pid 5476 segmentation fault at address 0x0 on 🪟 2019 x64

---

🧪   To try this PR locally:

bunx bun-pr 36303

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

bun-36303 --bun

[github-actions]

Found 1 issue this PR may fix:

1. --frozen-lockfile is saying changes to lockfile when bun install says there is not #20913 - Same root cause: --frozen-lockfile falsely reports lockfile changes in a monorepo immediately after bun install, due to non-idempotent tree building when dev/prod duplicate dependencies are hoisted

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #20913

🤖 Generated with Claude Code

[claude]

No bugs found. The fix is small and the mechanism reads correctly — input_dep_range.contains(dep_id) can only match a sibling from the same source package's own dependency slice, so the widened merge is scoped exactly to the dev/prod-duplicate case. Deferring to a human because this changes hoisting output (one of the two versions is now dropped from the tree/lockfile for affected workspaces), and which version wins is worth a maintainer's eye.

What was reviewed

• Traced resolution_list → input_dep_range and confirmed ExternalSlice::contains is the [off, off+len) index check, so the new branch cannot fire on a dep from a different package.
• Checked that the recursive hoist_dependency::<false, _> call threads the same range, so the merge fires whichever ancestor the first sibling landed in.
• Test exercises fresh install → --frozen-lockfile → reinstall and asserts byte-stable lockfile plus a single hoisted entry.

Extended reasoning...

Overview

The PR fixes #19088: a workspace package that lists the same dependency name in both dependencies and devDependencies at different versions produces an unstable lockfile — --frozen-lockfile fails immediately after a fresh install, and a second install rewrites bun.lock.

The change threads the source package's dependency-ID range (resolution_list, already computed in process_subtree) into hoist_dependency as input_dep_range, and extends the existing dev-vs-prod merge from if AS_DEFINED to if AS_DEFINED || input_dep_range.contains(dep_id). This makes the workspace case collapse to one tree entry the same way the root-package case already does. A regression test in bun-install.test.ts covers install → frozen → reinstall and pins the surviving version.

Security risks

None. This is internal hoisting logic operating on already-parsed lockfile dependency IDs; no new untrusted input, path handling, or network surface.

Level of scrutiny

Medium-high. Tree::hoist_dependency is the core of the hoisted-linker layout algorithm — it decides what lands in node_modules and what the lockfile records. The diff itself is tight (~15 net lines), the added guard is provably scoped (ExternalSlice::contains is an off <= id < off+len check against the source package's own slice, so it cannot match another package's dep), and it mirrors behavior that already exists for the root package via AS_DEFINED. I did not find a way for the new branch to fire outside the intended dev/prod-duplicate-from-same-package case.

Why defer rather than approve

This is a behavior change to install output, not just a stability fix: previously both versions were written to the tree (one hoisted, one nested under the workspace); now only the first-sorted sibling survives and the other is dropped entirely. The PR notes users with existing affected lockfiles will need a regenerating bun install. The test asserts the dev entry (0.0.3) wins over the prod entry (0.0.5), which follows from DepSorter's behavior ordering — a maintainer should confirm that's the intended winner (it matches the existing root-package path, but it does mean the dependencies specifier is silently ignored). Given hoisting changes have historically had long tails of edge cases, I'd rather a human sign off on the semantics.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/cli/install/bun-install.test.ts`:
- Around line 6627-6637: Update each install subprocess block around spawn to
await stderr.text() and exited concurrently with Promise.all, applying the same
change to all three referenced cases. Preserve the existing stderr assertion and
exit-status assertion order after both promises resolve.
- Around line 6650-6652: Update the frozen-install test to compare bun.lock with
its original bytes immediately after the frozen run completes, before starting
the subsequent normal install. Preserve the existing byte-for-byte assertion and
ensure the normal-install assertions remain separate.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b5e0d53d-db6c-4611-b18f-7ffe0f98d7c0

📥 Commits

Reviewing files that changed from the base of the PR and between 0316b6f and 736b306.

📒 Files selected for processing (2)

• src/install/lockfile/Tree.rs
• test/cli/install/bun-install.test.ts

[claude]

Thanks for addressing the optional/prod sibling case — the extended different_group check and the parameterized test look right. I'm deferring to a maintainer rather than auto-approving because this changes hoisting/lockfile-shape semantics (workspaces with a same-name dep in two groups now get one tree entry instead of two), and someone familiar with the hoisting design should confirm that's the intended resolution.

What was reviewed:

• input_dep_range.contains(dep_id) is a plain [off, off+len) range check on ExternalSlice<u32>; dep IDs are globally unique indices, so this cannot false-positive on an unrelated package's entry.
• is_optional() is OPTIONAL && !PEER, so the new arm doesn't intersect the existing optional-peer handling paths.
• At AS_DEFINED=true the new-tree only contains siblings from the same resolution_list (subtrees are queued, not processed inline), so AS_DEFINED || contains(...) preserves the original gate exactly.
• Test drains stderr/exit concurrently, asserts lockfile byte-identity after both the frozen and the second plain install, and pins the installed version.

Extended reasoning...

Overview

The PR threads the source package's dependency-ID range (resolution_list) into Tree::hoist_dependency and extends the existing "same name in two dependency groups → merge" rule so it also fires when the colliding entry sits in a parent tree but provably came from the same source package. Previously the merge was gated on AS_DEFINED (i.e., only at the package's own tree level), which missed the workspace case where the first duplicate hoists to root before the second is processed. The condition was also widened from dev-only to dev-or-optional per my earlier review comment.

Two files touched: ~15 lines of logic in src/install/lockfile/Tree.rs and one new it.each test in test/cli/install/bun-install.test.ts.

Security risks

None. No untrusted input parsing, no path handling, no allocation changes. ExternalSlice::contains is an integer range check on internally-produced dependency indices.

Level of scrutiny

Medium-high. Package manager hoisting is a critical, subtle code path where small changes cascade into lockfile shape and on-disk layout. The change is small and the mechanism is well-explained in the PR description, but it is a semantic change: workspaces that hit this pattern go from two installed copies (hoisted + nested) to one, and the winning version is whichever DepSorter places first (dev/optional, per the test's 0.0.3 assertion). That matches existing root-package behavior, so it is consistency rather than a new design — but a maintainer who owns the hoisting design should confirm that resolution is intended, especially since users with existing lockfiles containing the spurious nested entry will need one regeneration pass.

Other factors

• All prior review feedback (mine on the optional sibling, CodeRabbit's on concurrent pipe draining and the post-frozen lockfile assertion, comment-cop's on the long comment) is addressed and the threads are resolved.
• The bug-hunting system found nothing this run.
• The PR body notes the test was deferred to CI rather than run locally; CI build #84782 is in progress.

[robobun]

Superseded: see the build 93904 summary below (branch was rebased onto main since this build).

[alii]

Right place for the fix, but the condition is narrower than it needs to be and the same failure is still there for dependencies + peerDependencies.

• Tree.rs:1037: comparing group flags misses prod+peer, a workspace in that shape still writes two entries and --frozen-lockfile still fails. The range check on its own is the whole condition.
• Tree.rs:1038: the optionalDependencies half is a separate fix (a hard "dependency loop" error on main in one shape) that the title, body and test do not cover.
• The body should say that a regenerated workspace lockfile now pins the devDependencies/optionalDependencies entry; on main it settled on the dependencies entry.

[robobun]

Thanks, all three taken in bcc8902 (rebased onto main):

• The condition is now just input_dep_range.contains(dep_id); reproduced the prod+peer shape on main and it is a row in the test.
• The root-pins-a-third-version optional shape (dependency loop on main) is a row too. Since the range check means nothing at the package's own level can reach that error any more, the error and the DependencyLoop variants behind it are deleted rather than left unreachable.
• Body rewritten: says which entry wins (dev/optional, as root already does, now also for regenerated workspace lockfiles and under --production), that already-stable lockfiles load unchanged, and names the deletions.

The test now asserts the full packages section of the generated lockfile per shape, then that a frozen and a plain reinstall leave it byte-identical.

[alii]

Checked the new head: the condition is the bare range check, all four shapes in the test fail on main the way the body says (three with the extra pkg-a/baz entry, the root-pin one with the dependency loop error) and the deleted error is unreachable now, since a package's own folder only holds entries from its own range while its slots are being placed. Looks ready to merge from this side; one non-blocking note on the body below. Not re-run here: the four rows passing on this branch and the byte-identical reload of a main-produced lockfile.

[robobun]

Thanks. Body corrected per the note on Tree.rs (root parse only collapses the optionalDependencies duplicate; the failing rows on main are dev, optional and peer with the extra entry, root-pin with the dependency loop).

CI for d1de2c9 (build 93904, finished): 176/181 jobs pass and bun-install.test.ts is green on every lane. The three red lanes are unrelated to install and reported separately: test/bake/deinitialization.test.ts segfaults during dev server teardown (Windows 2019 x64), test-cluster-shared-leak.js times out (Windows 11 aarch64), and grpc-js test-tonic gets a CANCELLED status in its flow control test (macOS 14 aarch64).

[robobun]

Data point from the 1.4 install fuzzer (ledger entry 14824): the dependencies/devDependencies + peerDependencies shape reproduces with two registry packages and one workspace, on 1.3.14 and current main, hoisted and isolated:

b@1.0.0, b@2.0.0, d@1.0.0 (dependencies: b@1.0.0)
packages/ws-1: devDependencies { b: ^2.0.0, d: 1.0.0 }, peerDependencies { b: ^1.0.0 }

bun install                    # bun.lock gets "b" = 2.0.0, "d/b" = 1.0.0 and "ws-1/b" = 1.0.0
bun install --frozen-lockfile  # error: lockfile had changes, but lockfile is frozen
bun install                    # rewrites bun.lock, "b" is now 1.0.0 only

The transitive d -> b@1.0.0 plays the role of the sibling workspace pin in this PR's fourth table row: the workspace's peer slot binds to it (fresh resolve in the deferred peer phase, reload via resolve_peer_dep_version_based), the hoister places it as ws-1/b, and on reload the dev slot is bound to that placement too, so 2.0.0 is dropped.

I merged this branch onto current main (2c2ef7cbff) locally to check it against that case. Two small conflicts: src/install/error.rs (main has since removed the variants next to DependencyLoop, so the resolution is just deleting DependencyLoop) and the two return Ok(dedupe()) sites in Tree::hoist_dependency, which become return dedupe(). With that, the four table tests here pass, and so does a test for the shape above (both linkers): test/cli/install/bun-workspaces.test.ts on main...farm/e9faed7a/peer-follows-own-dependency.

That branch also carries an alternative fix limited to the peer shape (the peer slot is bound to the package's own non-peer slot on the same name, after resolve, in the bun.lock loader and in Package::clone). It is not needed once this lands; the only user-visible difference is that --production / --omit=dev would then install the same version of the name that the full install uses, instead of the peer slot's own pick. Leaving it as a pointer in case that direction is preferred; this PR covers the dev and optional shapes as well, which that branch does not.

[alii]

@robobun this conflicts with main now, please rebase and get a fresh CI run so it can be merged.

[robobun]

Rebased onto main as a single commit (09dc937); GitHub reports it mergeable again and a fresh CI run is going.

The diff is unchanged apart from layering over #37426: the two peer-path returns are now return dedupe(); instead of return Ok(dedupe());, and main had since removed the variants that used to sit next to Error::DependencyLoop, so that hunk shrank. The invariant the deleted error relied on still holds with the new folder-cycle check and Rebind arm: both only ever place or swap in the slot currently being processed, so a package's own folder still holds nothing but its own range while its slots are placed.

Re-ran locally on the rebased build: the four table rows, plus bun-lock (30, includes the optional peer binding tests), bun-workspaces (73), bun-dedupe (76), frozen-lockfile-pruned (101), bun-prune (108), bun-add (70) and bun-install (only the usual network-dependent git/URL tests fail in this container), all green.

[robobun]

One small follow-up on top of the rebase, 6c251e2, prompted by a bot note: with the dependency loop error gone, the lookup loop in hoist_dependency no longer takes any &mut borrow, so its raw-pointer detachment is replaced by iterating the slice (borrowck accepts it, no behaviour change); the refuse_declared_positionals doc comment that described the old error as its fallback is reworded; and two not.toContain("dependency loop") assertions that could no longer fail are dropped. Table rows plus bun-add-filter, bun-add-catalog, bun-lock, bun-workspaces and bun-dedupe are green locally on it; CI is running.