install: update bun.lock workspace versions when only the version changes

[robobun]

Fixes #18906

Problem

• Bump a workspace's "version" in its package.json and run bun install or bun install --lockfile-only: bun.lock keeps the old version. With the hoisted linker every later install also reports 1 package installed for the bumped workspace, because the installer checks it against the stale version map.
• install_with_manager parses the root package.json into a fresh lockfile and calls Diff::generate against the loaded one. DiffSummary::has_diffs() (src/install/lockfile/Package.rs) only covers dependency add/remove/update, overrides, catalogs, trusted and patched dependencies. Workspace versions are never compared, so a version-only change yields no diff, the workspace_versions copy in install_with_manager.rs is skipped, and the save gate stays closed.

Fix

• Diff::generate compares workspace_versions between the loaded and fresh lockfiles on the root pass (count, then per entry major/minor/patch plus the pre and build identifier bytes read from each lockfile's own string buffer, then the reverse key walk), mirroring the patched_dependencies_changed block above it. The result is a new DiffSummary::workspace_versions_changed included in has_diffs(), which opens the existing copy-and-save path. It is deliberately not part of changes_resolutions(): a version bump does not change what any dependency resolves to, so it should not affect the optional-peer and direct-dependency snapshots that method gates.
• Bytes rather than tag hashes: bun.lock omits empty identifiers, so "1.0.0-" is written back as "1.0.0"; comparing the bytes makes such a project settle after one write instead of depending on how empty identifiers hash.
• Removing a version (or making it unparseable) is caught by the count check; the reverse walk is kept for symmetry with the neighbouring block.
• Test: test/regression/issue/18906.test.ts (8 cases). Each case that changes a version checks the next install prints Saved lockfile, inline-snapshots the whole bun.lock, and then checks one more install prints no changes without saving. Cases: bump under --linker=isolated and --linker=hoisted, bump with --lockfile-only, build metadata only, prerelease (1.0.0 to 1.0.0-beta.1+build.7 to 1.0.0-beta.2+build.7), adding a version, removing a version, and the 1.0.0- settle guard.
• Verified on this branch (rebased onto 39fb3c1): with main's src/install/lockfile/Package.rs checked out and rebuilt, bun bd test test/regression/issue/18906.test.ts gives 7 fail / 1 pass (the 1.0.0- guard passes either way; it guards the comparison, not the bug); with the change restored it gives 8 pass on repeated runs. The file sets a 30s default timeout because each case runs three or four installs concurrently with its siblings, which tipped over the 5s default once on a cold debug build.
• Also ran test/cli/install/lockfile-only.test.ts (pass). --frozen-lockfile after a bump behaves as on main (no error, file untouched).

Background

• bun.lock has a workspaces section listing each workspace's name, version and dependencies. In memory that version lives in Lockfile.workspace_versions, a map from package name hash to Semver::Version.
• On bun install, bun loads the existing lockfile, parses the current package.json files into a second, throwaway Lockfile, and runs Diff::generate between them. DiffSummary is the result; has_diffs() decides whether the loaded lockfile gets the fresh data copied in and is rewritten, or is reused as is.
• Semver::Version stores the numeric parts inline and the prerelease/build identifiers as offsets into the owning lockfile's string buffer, so comparing two versions from different lockfiles needs both buffers. Version::eql is SemVer precedence equality (ignores build metadata), which is why it is not used here.

Earlier revision

The first revision compared with Version::eql plus tag.build.hash, had its tests in bun-workspaces.test.ts, only ran under the isolated linker and never reinstalled after a bump. Reworked after review: byte comparison, reverse key walk, standalone test file covering both linkers, settle-after-change, prerelease, add and remove.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 23 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: 7facab73-1bd5-4b94-b527-ce9a5c9981da

📥 Commits

Reviewing files that changed from the base of the PR and between 39fb3c1 and c9bdca1.

📒 Files selected for processing (2)

• src/install/lockfile/Package.rs
• test/regression/issue/18906.test.ts

---

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

[robobun]

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

❌ @robobun, your commit c9bdca1 has 1 failures in Build #97270 (All Failures):

• test/js/bun/shell/shell-pipe-read-fault.test.ts - code 1 on 🐧 3.23 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 36285

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

bun-36285 --bun

[github-actions]

Found 2 issues this PR may fix:

1. bun pm version does not update bun.lock in workspaces #28935 - bun pm version does not update bun.lock in workspaces; same root cause where lockfile is not rewritten after a workspace version bump
2. bun pm pack replaces workspace packages with bun.lock version, not their package.json one #20477 - bun pm pack uses stale workspace version from bun.lock instead of current package.json version; downstream consequence of the lockfile not being updated

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

Fixes #28935
Fixes #20477

🤖 Generated with Claude Code

[robobun]

Neither of those is covered by this change:

• bun pm version does not update bun.lock in workspaces #28935 is bun pm version, which writes package.json directly and never goes through Diff::generate. That one is being handled in pm version: update bun.lock for workspace packages #28936.
• bun pm pack replaces workspace packages with bun.lock version, not their package.json one #20477 is about bun pm pack/publish reading the workspace version from the lockfile rather than package.json, which is a separate question addressed by fix: publish bug with bun.lock #26797.

This PR only changes bun install / bun install --lockfile-only, so leaving the body at "Fixes #18906".

[claude]

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

• 🟡 test/cli/install/bun-workspaces.test.ts:2155-2157 — Nit: setup() acquires a semaphore slot via await setupTest() and then runs expect(...) assertions before returning ctx — if any of those throw, the caller's using ctx = await setup() never binds and the slot is never released. Wrap the post-setupTest() body in a try/catch that calls ctx[Symbol.dispose]() before rethrowing (mirroring how setupTest() itself releases on error). Impact today is nil (3 serial tests, last in the file, 12-slot pool), but it violates the file's own convention and REVIEW.md's "release every resource … BEFORE the assertions" rule.

  Extended reasoning...

  What the bug is

  The new setup() helper in the "bun.lock workspace version tracks package.json version" describe block does:

  async function setup() {
    const ctx = await setupTest();          // <-- acquires a semaphore slot, returns disposable
    const { packageDir, packageJson, env } = ctx;
    await Promise.all([ write(...), ... ]);
    let { stderr, exited } = spawn({ cmd: [bunExe(), "install"], ... });
    const err = await stderr.text();
    expect(err).not.toContain("error:");   // <-- can throw
    expect(await exited).toBe(0);           // <-- can throw
    const lockfile = await file(...).text();
    expect(workspaceEntry(lockfile, "packages/pkg-a")).toBe(...); // <-- can throw
    return ctx;
  }

  Callers use using ctx = await setup();. The using disposal only arms after the awaited expression resolves — if setup() rejects, ctx is never bound and ctx[Symbol.dispose]() (which calls releaseSlot()) never runs. The slot acquired by setupTest() is leaked for the rest of the file.

  Why existing code doesn't prevent it

  setupTest() itself is careful: after await acquireSlot() it wraps the remainder in try { ... } catch (e) { releaseSlot(); throw e; } so a failure inside setupTest() never leaks. But once it returns, that guard is gone — the responsibility passes to whoever holds ctx. Every other test in this file arms using ctx = await setupTest(); before running any assertions, so disposal is always registered when the first expect runs. setup() breaks that contract by inserting assertions between acquire and return.

  This is exactly the pattern REVIEW.md's test rules call out:

  Release every resource via using/await using or try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners)

  Step-by-step proof

  1. Test "bun install --lockfile-only" after bumping... starts, calls await setup().
  2. setup() calls await setupTest() → acquireSlot() increments activeSlots from 0 → 1 and returns a ctx whose [Symbol.dispose] calls releaseSlot().
  3. Suppose the initial bun install on this CI runner exits nonzero (disk full, ASAN issue, etc.). expect(await exited).toBe(0) throws.
  4. setup() has no try/catch around this — the exception propagates and the returned promise rejects.
  5. In the test body, using ctx = await setup() never completes the binding, so no dispose is registered. Test fails.
  6. activeSlots stays at 1. releaseSlot() is never called for this test.

  Repeat for the other two setup() callers → up to 3 leaked slots.

  Impact

  Effectively none today, which is why this is a nit:

  • Only 3 tests call setup(), all plain test (serial), so at most 3 of MAX_CONCURRENT = 12 slots can leak — not enough to starve anything.
  • The describe block is the last thing in the file; nothing runs after it.
  • activeSlots is a module-local let, not persisted across processes, so "poisons later tests on persistent CI runners" doesn't apply here.

  The only concern is fragility: if someone later adds test.concurrent cases after this block, or reorders, a flaky initial install would silently reduce available concurrency for the rest of the file.

  Fix

  Mirror setupTest()'s own pattern:

  async function setup() {
    const ctx = await setupTest();
    try {
      const { packageDir, packageJson, env } = ctx;
      // ... writes, spawn, expects ...
      return ctx;
    } catch (e) {
      ctx[Symbol.dispose]();
      throw e;
    }
  }

  (Alternatively, move the initial-install assertions into each test body after the using binding, but the try/catch is the smaller diff.)

[claude]

LGTM — the earlier build-metadata gap is fixed and covered by a test; the remaining describe.concurrent note is just a CI-speed nit.

Also checked this pass: DiffSummary derives Default so the new flag stays false on non-root/per-member diffs (Package.rs:1483 unaffected); the had_any_diffs branch in install_with_manager.rs:412-421 already re-copies workspace_versions, so flipping has_diffs() is sufficient; ExternalString.hash is content-derived, so comparing pre/build hashes across the two lockfiles' independent string buffers is valid.

Extended reasoning...

Overview

Adds a workspace_versions_changed flag to DiffSummary in src/install/lockfile/Package.rs, computed on the root diff pass by comparing from_lockfile.workspace_versions against to_lockfile.workspace_versions (count, key presence, and per-entry Version::eql + tag.build.hash). The flag feeds into has_diffs(), which gates both the workspace_versions re-copy in install_with_manager.rs:412-421 and the lockfile save at line 850. New regression test file with 5 cases: bump via --lockfile-only, bump via plain install, build-metadata-only bump, adding a version where none existed, and a no-op guard.

Security risks

None. This is a lockfile-freshness comparison over already-parsed semver data; no new parsing of untrusted input, no filesystem writes beyond the existing lockfile save path.

Level of scrutiny

Medium. The change touches bun install's diff decision, but it is purely additive — a new boolean OR'd into has_diffs() — and mirrors the patched_dependencies_changed block immediately above it line-for-line. Failure modes are bounded: a false positive would rewrite an unchanged lockfile (guarded against by the no-op test), and a false negative is the pre-existing bug. I confirmed the second has_diffs() call site at Package.rs:1483 (per-workspace-member diffs) is unaffected because the flag is only set under if is_root and DiffSummary derives Default.

Other factors

My previous review flagged that Version::eql ignores build metadata; the author added the tag.build.hash comparison and a dedicated 1.0.0+build.1 → 1.0.0+build.2 test in 1abc9e3. The comment-cop feedback was addressed in 46339b0. The version comparison uses content hashes (ExternalString.hash), not buffer offsets, so comparing across two lockfiles with separate string buffers is sound. One expected consequence not explicitly tested: --frozen-lockfile will now (correctly) fail when a workspace's package.json version disagrees with bun.lock, since the copy path now runs and Lockfile::eql sees the difference — that's the semantically correct behavior for a frozen check. The only new finding this run is the describe.concurrent nit, which is a test-speed suggestion, not a correctness issue.

[claude]

LGTM — small additive change following the existing patched_dependencies_changed pattern, with all prior review feedback addressed.

What was reviewed:

• Verified the version comparison is hash-based (major/minor/patch ints + pre.hash + build.hash), so it's safe across two lockfiles with different string buffers.
• Confirmed the is_root gate keeps the check out of the recursive per-workspace Diff::generate call (which passes id_mapping = None), so update_mapping at Package.rs:1483 is unaffected.
• Traced has_diffs() to the had_any_diffs else-branch in install_with_manager.rs — that path already runs for every other diff type, and the no-op test guards against false positives.

Extended reasoning...

Overview

This PR fixes #18906: bumping a workspace package's "version" in its package.json and running bun install (or --lockfile-only) left the workspace's version in bun.lock stale. The fix adds a workspace_versions_changed: bool to DiffSummary, computes it in Diff::generate on the root pass by comparing the workspace_versions maps between the loaded and fresh lockfiles, and includes it in has_diffs(). Two files touched: 25 lines in src/install/lockfile/Package.rs and a new 157-line regression test.

Security risks

None. No untrusted input handling, no auth/crypto/permission surface. The change reads two in-memory hash maps and compares u64 hashes and integer version components.

Level of scrutiny

Moderate. This is bun install core diff logic, but the change is purely additive: a new bool field (defaults to false via #[derive(Default)]), only set in the root-pass if is_root block. It follows the exact same shape as patched_dependencies_changed and catalogs_changed directly above it. I checked:

• DiffSummary is only ever built via ::default(), so no explicit-field constructors break.
• The recursive Diff::generate at Package.rs:1457 passes id_mapping = None, so is_root = false there and the new check doesn't run — the !diff.has_diffs() at line 1483 that gates update_mapping is unaffected.
• Version::eql compares major/minor/patch as integers and tag via pre.hash; the added tag.build.hash comparison closes the build-metadata gap I flagged earlier. All hash/int-based, so comparing versions from two lockfiles with different string buffers is safe (VersionHashMap is keyed by PackageNameHash, also buffer-independent).
• The count check + per-key-in-to loop is sufficient: if counts match and every to key exists in from with an equal value, the maps are equal; a rename (remove A, add B, same count) hits the None arm.
• The had_any_diffs else-branch in install_with_manager.rs:278 that this now triggers for version-only bumps is the standard "something changed, re-copy state" path already exercised by every other diff type. Worst-case false positive is an unnecessary lockfile rewrite; the no-op test guards this.

Other factors

Two prior rounds of my review feedback are resolved: the tag.build.hash comparison was added with a dedicated test, and the test file was switched to describe.concurrent. The comment-cop flag was addressed. Tests cover --lockfile-only, plain install, build-metadata-only bumps, adding a version where none existed, and the no-change no-op guard. The PR body includes evidence that tests fail on both ASAN-debug and release without the fix and pass with it. Tests use tempDir/bunEnv/bunExe, drain pipes concurrently, and assert stderr before exit code per harness conventions.

[claude]

LGTM — the earlier feedback (build-metadata comparison, describe.concurrent) is addressed and the no-op guard test covers the false-positive case.

What was reviewed:

• Diff::generate addition mirrors the existing patched_dependencies_changed pattern; is_root gate matches the catalogs_changed block and the recursive workspace call passes id_mapping: None, so it stays untouched on non-root passes.
• Version::eql + tag.build.hash are buffer-independent (integer/hash comparisons), so comparing versions across the two lockfiles' string buffers is sound.
• Confirmed had_any_diffs gates the existing workspace_versions copy in install_with_manager.rs:411-421, so the new flag reaches the save path with no other wiring needed.

Extended reasoning...

Overview

The PR adds a workspace_versions_changed flag to DiffSummary and computes it in Diff::generate on the root pass by comparing from_lockfile.workspace_versions against to_lockfile.workspace_versions (count, key set, and per-entry Version::eql + tag.build.hash). The flag is OR'd into has_diffs(), which install_with_manager already uses to decide whether to copy the fresh lockfile's workspace_versions into the loaded lockfile and mark it for saving. Five regression tests in test/regression/issue/18906.test.ts cover plain bun install, --lockfile-only, build-metadata-only bumps, adding a version where none existed, and the no-change no-op guard.

Security risks

None. This is a lockfile-freshness check on locally-parsed package.json versions; no untrusted input, no allocation driven by external data, no auth/crypto.

Level of scrutiny

Moderate — bun install is production-critical, but the change is small (~25 lines), purely additive (only widens the set of cases where has_diffs() is true), and follows the exact shape of the neighboring patched_dependencies_changed and catalogs_changed blocks. The recursive Diff::generate call for workspace packages passes id_mapping: None, so is_root is false there and the new block is skipped — the inner diff.has_diffs() at Package.rs:1483 is unaffected. Version::eql and the .hash fields are integer comparisons, so the cross-lockfile-buffer comparison is sound without slicing into either buffer.

Other factors

Two prior rounds of my feedback are resolved: (1) the Version::eql precedence-vs-identity gap for +build metadata is closed by the added tag.build.hash comparison plus a dedicated test, and (2) the test file now uses describe.concurrent. The PR description includes fails-on-main / passes-on-PR evidence for both debug-ASAN and release. The no-op test ("with no version change is still a no-op") guards against the new flag causing spurious lockfile rewrites. The comment-cop note about a long explanatory comment was also addressed (comment removed). No outstanding reviewer comments remain.

[robobun]

CI is blocked on build infrastructure, not on this diff.

build 84639: 34 jobs passed, then :linux: aarch64 - build-bun timed out after 1h; canceled by retrigger. Every test annotation was [flaky] (passed alone / on retry), none in install code.

build 84714 (retrigger): the Rust build step finished successfully on every lane (Finished release profile in 1m36s on linux-x64, for example), but six build-bun jobs then failed waiting on their sibling build-cpp step with "Sibling step ...-build-cpp errored, nothing to link". Every build-cpp job is now in state passed, so this was a pipeline race between the original errored build-cpp attempts and their retries. Five more build jobs (darwin-aarch64, windows-aarch64, freebsd-x64, linux-aarch64-android, linux-x64-android) hit the 1h timeout. The only non-flaky test failure is test/cli/run/no-orphans.test.ts on darwin 14 x64 (a perl daemon reap timeout), which does not touch install/lockfile code.

test/regression/issue/18906.test.ts has not failed on any lane. The source change is 25 lines in src/install/lockfile/Package.rs; this PR touches no C++. Ready for a maintainer to re-run or merge.

[alii]

The fix is the right shape but the comparison needs to be on tag contents, and the tests need a second install after a bump to show the flag settles.

• hash compare flaps forever on versions with an empty identifier (1.0.0-, 1.0.0+), see inline
• no test reinstalls after a bump, so nothing pins that the rewrite happens once; removal direction and hoisted linker are also untested
• the Verification section in the body still describes the earlier revision (bun-workspaces.test.ts, 4 tests). The "release without fix" block was built from 079df30, which is this branch's own commit, so it is not evidence of anything; the ASAN leg is the one that shows the tests fail without the fix.
• needs a rebase, DiffSummary fields are pub(crate) on main now.

[robobun]

Reworked in d050ae7, rebased onto main (pub(crate) fields):

• Comparison now uses the pre/build bytes from each lockfile's string buffer, like the patched_dependencies block, plus the reverse key walk.
• Tests rewritten: every version change is followed by a second install that must print "no changes", the bump runs under both linkers (the hoisted leg is the one that printed "1 package installed" forever on main), and there are prerelease, add, remove and a 1.0.0- settle case. Whole-file inline snapshots, helper removed.
• PR body rewritten for this revision. Fail-before numbers there come from stashing src/ on this branch and rebuilding: 7 fail / 1 pass without, 8 pass with.

[alii]

Looks ready to merge from this side. Traced the byte comparison for 1.0.0-, 1.0.0+, 1.0.0rc.1 and a normal prerelease with identifiers longer than the inline limit: both maps slice their own string buffer, empty identifiers come out as the zeroed String on both sides, and everything the writer emits reparses to the same pre and build bytes, so a second install produces no diff. The removal direction, the from-side walk, the hoisted leg (main prints "1 package installed" on every install after the bump, reproduced) and the whole-file snapshots all check out against main's writer output; the pass/fail counts in the body were not rerun here.

[robobun]

Final CI state for the rebased c9bdca1 (build 97270, now finished): 176 of 179 jobs passed, including every ASAN lane; test/regression/issue/18906.test.ts has no failures on any lane.

• The one red job is alpine 3.23 aarch64 - test-bun, failing on test/js/bun/shell/shell-pipe-read-fault.test.ts, a Bun.$ pipe-reader test that is also flaking on main's own latest build (97390); reported separately.
• The other two jobs are darwin 14 aarch64 - test-bun, which never got an agent and expired after about five hours, so that lane did not run on this build at all. A retry from the Buildkite side would re-run just those two if you want them before merging.
• Everything else annotated is yellow (passed on retry): two verdaccio startup timeouts, a fresh-project first install in bun-patch on Windows aarch64 that never reaches the differ, child_process IPC, inspect-error-leak, nodemailer SMTP auth, cluster-shared-leak.

Nothing in the build is attributable to this change; ready to merge from my side.

[bondz]

Please address #28935 in pr #28936 too since this is related.

[alii]

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

[robobun]

Rebased onto 39fb3c1 and force-pushed as c9bdca1; fresh CI is running.

Two things changed in the rebase, both noted in the body:

• main split DiffSummary into changes_resolutions() / has_diffs() / changes_dependencies(). The new flag is only in has_diffs() (next to trusted and patched dependencies), since a version bump does not change any resolution and should not disturb the snapshots changes_resolutions() gates.
• the test file now sets a 30s default timeout: each case runs three or four installs concurrently with its siblings and one cold debug run tipped over the 5s default.

Re-verified after the rebase with main's Package.rs swapped in: 7 fail / 1 pass without, 8 pass with. The diff itself is otherwise identical to what you reviewed.

[github-actions]

Superseded by #41302.

[robobun]

Confirmed on main at f942cb0. The 8 cases in this branch's test/regression/issue/18906.test.ts all pass there: both linkers, --lockfile-only, build metadata, prerelease, the 1.0.0- settle case, and adding and removing a version. Nothing is left for this PR. The test file stays on the branch if you want those extra cases on main.