Skip to content

install: fail --frozen-lockfile on manifest drift and fix the spurious lockfile re-saves behind it - #33632

Open
robobun wants to merge 5 commits into
mainfrom
farm/8cbd0739/frozen-lockfile-new-direct-dep
Open

install: fail --frozen-lockfile on manifest drift and fix the spurious lockfile re-saves behind it#33632
robobun wants to merge 5 commits into
mainfrom
farm/8cbd0739/frozen-lockfile-new-direct-dep

Conversation

@robobun

@robobun robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Fixes #22689
Fixes #24223
Fixes #13823

Problem

  • bun install --frozen-lockfile (and bun ci, --production) exits 0 on a stale lockfile whenever the manifest change resolves to packages the lockfile already has: a dependency added to one workspace that another workspace (or a transitive) already pulls in, a removed direct dependency that stays in the tree transitively, a range change the locked version still satisfies. npm/pnpm/yarn all refuse these.
  • Cause: the frozen check in install_with_manager.rs compares the resolved package set (Lockfile::eql for bun.lock, the meta hash for bun.lockb) and, since install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333, the overrides and catalog flags of the differ; the differ's dependency comparison itself (Diff::generate, root and workspace members) is not consulted.
  • Consulting it is only possible once it is quiet on stable repos. On main it reports a diff on every install for a number of shapes (since install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 the byte-identical re-save is suppressed, so this is invisible, but every one of them would become a permanent frozen failure): a workspace member with a lifecycle script, trustedDependencies declared in a member, a sibling workspace listed in two dependency sections, a catalog: entry or an override pointing at a workspace, * on a versionless workspace, a root entry for a member that the parser does not link ({a: "^9.0.0"} plus an override), every bun.lockb monorepo whose members depend on each other, stale trustedDependencies / patchedDependencies entries, a trustedDependencies entry for a package installed under an npm: alias (which bun.lock also never records, so installing from the lockfile relied on that perpetual diff to trust it), and monorepos migrated from pnpm-lock.yaml whose members depend on each other by version range.

Fix

Five commits:

  1. install: stop reporting manifest diffs for repos whose lockfile is already up to date (Package.rs, lockfile.rs, bun.lock.rs, bun.lockb.rs, a hook in install_with_manager.rs)
    • lifecycle scripts are only compared when the loaded package recorded them (scripts.filled, i.e. bun.lockb); bun.lock stores none and reads them from package.json at install time.
    • overrides, catalogs, trustedDependencies and patchedDependencies are compared once, at the root, and the last two only after the members have been parsed (the lockfile holds the union over all workspaces; before, a list declared in a member was reported removed on every install). The caller already applies changes to these sections across every package's edges.
    • workspace members: Diff::generate used to compare a member only by recursing through the root's workspace-behavior edge onto it. diff_workspace_members now compares every workspace package in the lockfile with its package.json regardless of how the root reaches it (its workspaces entry, an override on its own entry, only through a sibling), records the changed ones in changed_workspaces (which changes_resolutions / changes_dependencies / has_diffs include, so the existing consumers keep their meaning), and install_with_manager re-resolves one dependency onto each changed member (Lockfile::dependency_to_reresolve_workspace, preferring a workspace: edge declared by the root or a member), which re-reads it into the same package id through the folder resolver, exactly what re-resolving the synthetic edge used to do. Root edges onto members always stay mapped. The pass also feeds install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333's survivors / removed_names bookkeeping, so pruned checkouts behave as before.
    • the bun.lock loader tagged every root/workspace edge that resolved to a workspace package as a workspace dependency, except the second entry of a duplicated name, and always synthesized the root's edge to each member. The parser tags only workspace: specifiers and npm ranges satisfied by the member's version, tags every entry, and replaces the synthetic edge with the root's own entry when that entry is an npm range it does not link; map_manifest_dep_to_pkg and the synthesis loop now follow those rules (using the versions recorded in the lockfile, so a member that moved out of range still produces a diff). This is the bun-workspaces.test.ts.snap change: loaded */dist-tag edges onto a versionless member keep their own tag.
    • bun.lockb stores an edge as tag plus literal, so a member's workspace:* edge reloaded holding * while a fresh parse holds the member's path, which is what Version::eql compares. restore_workspace_dependency_paths points the root's and members' workspace edges back at the path of the package they resolved to after loading, as the bun.lock loader already did (the migrate-bun-lockb-v2 snapshot shows the path now; the bytes written are unchanged).
    • bun.lock only writes the trustedDependencies names and patchedDependencies keys that match something in the tree, while the differ compared the full package.json sets. Entries that are only in package.json now count as additions when the lockfile has something for them to apply to (stores_trusted_dependency, stores_any_patched_dependency); what the loaded lockfile holds is compared as before. has_trusted_dependency checks an npm package under the package's own name, so {"x": "npm:real@1"} is trusted by listing real; the writer only looked at edge names and never stored such an entry, and stores_trusted_dependency has to agree with has_trusted_dependency, otherwise the entry is filtered out of the diff and an install from the lockfile stops trusting it. Both now also match the name of the npm package an edge resolved to (one extra insert in the writer's collection loop; for a non-aliased edge it re-inserts the key just inserted, so install: keep trustedDependencies and patchedDependencies order in bun.lock #38736's byte order is unaffected). And since the differ now leaves some package.json entries out on purpose, install_with_manager copies trustedDependencies and patchedDependencies from the fresh parse whether or not it reported a diff, as it already did for the root scripts (the patched block is the else branch's existing code moved into copy_patched_dependencies, called from both branches). Otherwise a run without a manifest diff kept the loaded subsets, which shows on the one such run that still changes resolutions: bun audit fix moving a package to the version a patch is written for installed it unpatched. Declaring trustedDependencies at all is what turns the default trusted list off, so the writer now emits "trustedDependencies": [] when the field is declared but none of its names are in the tree (bun.lockb already has a tag for this), and a package.json that declares the field while the lockfile does not record it is a diff (trusted_dependencies_declared). On main a bun.lock written for "trustedDependencies": [] drops the field and the default list is back in effect from the second install on, bun ci included (verified with the electron fixture); such lockfiles now get re-saved once.
  2. install: record workspace versions when migrating pnpm-lock.yaml (pnpm.rs): the migration read the version off the pnpm importer entry, which never has one, so migrated lockfiles listed members without versions and the loader rule above could not tell a linked range apart; it now reads the member's package.json, like the package-lock.json migration. Two migration snapshots gain the "version" lines bun's own writer produces.
  3. install: fail --frozen-lockfile when a manifest diverges from bun.lock, and say which one (install_with_manager.rs): the check becomes summary.changes_dependencies() || <eql / meta hash>. changes_dependencies is install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333's notion of "the dependency declarations changed" (root and members, overrides, catalog; not lifecycle scripts), so trustedDependencies and patchedDependencies stay out of the comparison, as the pruned-checkout tests require (turbo prune strips them; they are applied from package.json either way). The notes reuse install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333's wording:
    error: lockfile had changes, but lockfile is frozen
    note: dependencies in packages/pkg2/package.json changed since bun.lock was saved (1 added, 0 removed, 0 updated)
    note: try re-running without --frozen-lockfile and commit the updated lockfile
    
    (dependencies in package.json ... for the root; overrides / resolutions / the catalog in package.json changed since ... exactly as before, so install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333's assertions still hold; the packages resolved from the manifests differ from bun.lock when only the resolution comparison fires.) frozen_changed_section is folded into this.
  4. tests.
  5. install: debug lockfile JSON wrote the package name as a dist-tag dependency's tag: the bun:internal-for-testing dump wrote info.name for a dist-tag's tag; it surfaced because a loaded dist-tag edge now stays a dist-tag.

Dropped while rebasing onto current main: the bun update lockfile-specifier commit and the $ref override storage fix, both of which #38333 implemented independently (package_json_write_back::sync_lockfile, bun-update-lockfile-sync.test.ts), together with their tests.

Verification

test/cli/install/bun-install-registry.test.ts (the file fails on main, passes here):

  • failing direction: direct dep added that is already transitive (asserts the note and that bun.lock is left untouched), direct dep removed but still transitive, range literal changed (bun install --frozen-lockfile does not fail when lockfile is out of sync #24223), dep added to a second workspace (bun install --frozen-lockfile allows bun.lock changes in workspaces #22689, asserts the member note and no root note), a bun.lockb variant (bun install --frozen-lockfile does not exit with error when it should #13823); and --frozen-lockfile fails when <member> gains a dependency: a member kept only by an override on the root's entry (bun.lock) and a member only a sibling depends on while the root depends on its published version (bun.lockb; bun.lock cannot load that shape today, install: fix npm dependencies that share a workspace member's name #37248). The latter two add a dependency the root already has, so only the member comparison can notice; they check no diff on the second install, the frozen failure naming the member, the member actually being re-read by a plain install (lockfile dump), and frozen passing afterwards. All fail on main.
  • passing direction, --frozen-lockfile passes and nothing is re-saved (<format>): <shape>, 16 shapes under both formats (member depending on a sibling, root depending on members, override-kept unlinked entry, lifecycle script, member trustedDependencies, sibling in dev+peer, sibling linked in two groups, * on a versionless sibling, npm: alias of a sibling, catalog: -> workspace, override -> workspace, $ref override, stale / all-stale / empty trustedDependencies, stale patchedDependencies key). With the gate in place these rows are what proves the differ is quiet; on main's byte-compare save they pass trivially, so they act as guards there.
  • trustedDependencies [] / ["not-a-dependency"] still disables the default trusted list when installing from bun.lock (electron stays blocked on the first install, a plain install from the lockfile and a frozen one; bun.lock contains the empty field), and <change> passes --frozen-lockfile and is stored by the next install for an installed name, [], an all-stale list and a patch (stores ["no-deps"], [], [], the patch entry; frozen passes before and after). The electron tests and the []-storing rows fail on main.
  • trustedDependencies naming the package behind an npm: alias is stored and stays trusted when installing from bun.lock: lifecycle-postinstall installed as postinstall-alias and trusted under its real name; the postinstall runs on the first install, bun.lock records the name, and it runs again on a plain and on a frozen install from the lockfile. Fails on main on the bun.lock content (the name is not stored); failed on the previous revision of this PR at the second install (entry filtered out, bun.lock held [], script blocked).
  • the existing production = true test's inline snapshot gains the new note.

bun-audit.test.ts, applies the patchedDependencies entry written for the version it installs: no-deps@1.0.0 locked as a transitive, a patchedDependencies entry for 1.0.1 (so bun.lock stores nothing for it), and bun audit fix moving it to 1.0.1 has to install it patched and store the entry; frozen passes afterwards. Passes on main, fails on this branch with the unconditional copy removed (checked by building without it), passes here.

migration/migrate.test.ts: the arborist fixture edit-package-json--changed (package.json says abbrev: ^1.1.0, its lockfile was written from ^1.1.1) now records a frozen exit code of 1; #38853 had just recorded 0 for it and deferred this to this PR. It is the range-literal shape from #24223.

bun-install.test.ts: two existing bun.lockb workspace tests now assert the second install neither re-saves nor creates a .cache entry (they re-resolve on every install on main). pnpm-lock-v9.test.ts "link: version with a semver specifier" is the test that exposed commit 2.

Also run locally with the debug build: all of test/cli/install/ (the only failures are the tests that need network access or git clones), which includes #38333's frozen-lockfile-pruned, frozen-lockfile-missing-workspace, nested-overrides, bun-update-lockfile-sync, bun-lock (incl. #38736's byte-order test), bun-workspaces, the migration suites, isolated-install; source lints; cargo clippy -p bun_install. Plus registry-free ad-hoc shapes under both formats: a member reached through a catalog entry and then edited, two members edited at once (one removing a dependency) producing one note each and no root note, and linkWorkspacePackages flipped either way after the lockfile exists.

CI on this head (13f2595): every GitHub check passes (clippy, mordant, miri, format, source lints) and Buildkite build 97980 has all 177 Linux and Windows jobs green, with the only test failures being retried flakes in files this PR does not touch; its two darwin jobs expired because no darwin agent has been connected today, which affects every build, so the build shows as failed until the fleet is back.

Background

  • Diff::generate (Package.rs) compares the root package as loaded from the lockfile against a fresh parse of package.json; its DiffSummary drives the rebuild of the root's edges, should_save_lockfile, and (since install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333) changes_resolutions / changes_dependencies consumers such as dedupe, prune and the transitive update planner. had_any_diffs true on an unchanged repo means a full re-resolve every install; before install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 it also meant a visible re-save, which is how these shapes were found.
  • bun.lock stores each workspace's dependency sections as literal specifiers plus a resolved packages map; the loader re-parses the literals, synthesizes the root's edge to each member, and fixes up edges that resolved to workspace packages. bun.lockb stores the parsed edges as tag plus literal. Both loaders have to land on exactly what the parser produces for the differ to stay quiet.
  • When the install re-resolves a workspace: dependency declared by the root or a member, the folder resolver re-parses that member's package.json and replaces its package in place (same id), so every other edge onto it sees the new dependency list; the re-read hook relies on that.
  • bun.lock writes trustedDependencies / patchedDependencies filtered to what is in the tree, bun.lockb writes the full sets; a stable repo therefore reloads a subset of what package.json declares under bun.lock, which is why the differ asks "would a save store this" for new entries instead of comparing the sets.
Earlier versions

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

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Tightens --frozen-lockfile handling so early lockfile-equality exits only happen when no diffs were detected, and expands install tests to cover direct/transitive dependency, workspace, spec-literal, and bun.lockb regression cases.

Changes

Frozen Lockfile Diff Detection

Layer / File(s) Summary
Gate frozen-lockfile early exit on detected diffs
src/install/PackageManager/install_with_manager.rs
The --frozen-lockfile branch now only breaks on lockfile equality/meta-hash checks when had_any_diffs is false; otherwise it follows the frozen-lockfile error path.
Regression tests for lockfile staleness
test/cli/install/bun-install-registry.test.ts
Adds frozen-lockfile coverage for direct dependencies already present transitively, removal of a still-transitive direct dependency, dependency spec literal changes, a workspace transitive-resolution case, and a bun.lockb transitive-to-direct case.

Sequence Diagram(s)

sequenceDiagram
  participant InstallWithManager
  participant ResolveSession
  participant Lockfile

  InstallWithManager->>ResolveSession: compute had_any_diffs
  alt had_any_diffs is false
    InstallWithManager->>Lockfile: compare eql/meta-hash
    alt lockfile matches
      InstallWithManager-->>InstallWithManager: break frozen_lockfile
    else lockfile differs
      InstallWithManager-->>InstallWithManager: frozen-lockfile error
    end
  else had_any_diffs is true
    InstallWithManager-->>InstallWithManager: frozen-lockfile error
  end
Loading

Compact metadata: Base commit 2bffc927, head commit d89649a7, 2 files changed (+298/-18 lines).

Related issues: None referenced in the provided summary.

Related PRs: None referenced in the provided summary.

Suggested labels: bug, install, tests

Suggested reviewers: None determinable from provided summary.

Poem
Frozen installs checked the trail,
diffs now block the “all is well” sail,
transitive roots and workspaces too,
now fail when stale, as they should do.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly describes the main changes: detecting manifest drift during frozen installs and preventing unnecessary lockfile saves.
Description check ✅ Passed The description explains the problem, implementation, affected scenarios, and extensive verification, although it does not use the exact template headings.

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

@github-actions github-actions Bot added the claude label Jul 7, 2026
@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:38 AM PT - Aug 15th, 2026

@robobun, your commit 13f2595 is building: #97980

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. bun install --frozen-lockfile allows bun.lock changes in workspaces #22689 - --frozen-lockfile allows bun.lock changes when a dependency already resolved in one workspace is added to another workspace — exact scenario this PR fixes
  2. bun install --frozen-lockfile does not fail when lockfile is out of sync #24223 - --frozen-lockfile does not fail when a version specifier in package.json changes but the resolved package is already in the lockfile
  3. bun install --frozen-lockfile does not exit with error when it should #13823 - --frozen-lockfile does not exit with error when package.json has changes not reflected in bun.lock

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

Fixes #22689
Fixes #24223
Fixes #13823

🤖 Generated with Claude Code

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Verified all three and added Fixes #... to the description. Pushed two more test cases covering #24223 (version literal changed, locked resolution still satisfies) and #22689 (dep added to a second workspace) in 7de479b.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-registry.test.ts`:
- Around line 859-863: The test blocks in bun-install-registry.test.ts are
asserting the process exit code before finishing stderr and bun.lock
validations, which breaks the file’s convention. Reorder the assertions in each
affected test so the stderr/file checks run first and the exit-code check with
exited is the final assertion, keeping the more informative failure output. Use
the existing assertions around stderr.text(), Bun.file(join(packageDir,
"bun.lock")).text(), and exited to locate and reorder them consistently.
- Around line 744-973: The new frozen-lockfile tests only cover the text
lockfile path via --save-text-lockfile and never exercise the binary lockfile
branch guarded by has_meta_hash_changed. Add a similar install test in
bun-install-registry.test.ts that uses the default binary bun.lock format (no
text-lockfile flag) and reproduces one of the same drift cases, so the
frozen-lockfile check in install_with_manager.rs is verified for both
Lockfile::eql and the binary metadata-hash path.
🪄 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: 4a08c63e-523a-4baf-ae2c-be5a3439897d

📥 Commits

Reviewing files that changed from the base of the PR and between 2bffc92 and 7de479b.

⛔ Files ignored due to path filters (1)
  • test/cli/install/__snapshots__/bun-install-registry.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (2)
  • src/install/PackageManager/install_with_manager.rs
  • test/cli/install/bun-install-registry.test.ts

Comment thread test/cli/install/bun-install-registry.test.ts
Comment thread test/cli/install/bun-install-registry.test.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
test/cli/install/bun-install-registry.test.ts (1)

1113-1117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Move exited assertion after the filesystem checks.

expect(await exited).toBe(0) (Line 1115) is asserted before the exists() checks on Lines 1116-1117. Per this file's established convention, the exit-code assertion should be the last assertion in the block, after stderr and filesystem/side-effect validations, to keep the most informative diff on failure.

💚 Proposed reorder
   let err = await stderr.text();
   expect(err).not.toContain("error:");
-  expect(await exited).toBe(0);
   expect(await exists(join(packageDir, "bun.lockb"))).toBe(true);
   expect(await exists(join(packageDir, "bun.lock"))).toBe(false);
+  expect(await exited).toBe(0);

Based on learnings, "keep the process exit-code assertion as the LAST assertion in the test. Place it after any stderr checks and any filesystem/side-effect validations."

🤖 Prompt for 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.

In `@test/cli/install/bun-install-registry.test.ts` around lines 1113 - 1117, In
bun-install-registry.test.ts, reorder the assertions in the affected test so the
exited check is last: keep the stderr expectation first, then the filesystem
exists() validations, and move expect(await exited).toBe(0) to the end of the
block. Use the existing exited, stderr.text(), and exists(join(packageDir, ...))
assertions in that test as the anchor points.

Source: Learnings

🤖 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.

Duplicate comments:
In `@test/cli/install/bun-install-registry.test.ts`:
- Around line 1113-1117: In bun-install-registry.test.ts, reorder the assertions
in the affected test so the exited check is last: keep the stderr expectation
first, then the filesystem exists() validations, and move expect(await
exited).toBe(0) to the end of the block. Use the existing exited, stderr.text(),
and exists(join(packageDir, ...)) assertions in that test as the anchor points.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 02849827-e5b7-425a-a823-9abcc5520f48

📥 Commits

Reviewing files that changed from the base of the PR and between 7de479b and 6a7e0c2.

📒 Files selected for processing (1)
  • test/cli/install/bun-install-registry.test.ts

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Applied the same exit-code-last reorder to the bun.lockb test in d89649a.

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the only hard failure on both build 69786 and the re-run build 69850 is the darwin-26-aarch64-test-bun lane dying with

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'

before any tests run. The new tests in bun-install-registry.test.ts passed on every lane that ran them; the other entries in the flaky annotation (hot.test.ts ENOENT on Windows, test-http-client-keep-alive-hint.js ECONNREFUSED, bun-install.test.ts lifecycle-script EBADF on Windows, complex-workspace.test.ts sharp install on macOS ASAN) all passed on retry and don't touch the frozen-lockfile path this PR changes.

The diff itself is green; this needs a maintainer to re-run or override the darwin-26 artifact-download lane.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Triage note: #13823 was closed as fixed on main, but that verification only covered adding a dependency that is absent from the lockfile, which already failed correctly before this PR. The case this PR fixes (the added, removed or re-specified dependency is already resolved elsewhere in bun.lock) still reproduces on main.

Checked by applying only this PR's test changes onto main at 165dc9f and running bun-install-registry.test.ts -t "frozen-lockfile fails when": all 5 new tests fail on main, --frozen-lockfile exits 0 with empty stderr in each. #22689 and #24223 are still open. This PR is still needed.

@alii alii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as-is. had_any_diffs is the "re-save the lockfile" signal, not a "lockfile is stale" signal: on main it is already true on every install for several stable monorepo shapes, so gating frozen on it makes --frozen-lockfile, bun ci and --production fail on every run for those repos with nothing to commit.

  • workspace member with any lifecycle script; bun.lock stores no scripts, so every install counts an update (verified on main, lock byte-identical)
  • workspace-level trustedDependencies, a sibling workspace listed in two dependency sections, catalog:/overrides pointing at workspace:*, and $ref overrides, all verified the same way
  • bun update --latest leaves latest literals in bun.lock, so the frozen test added to catalogs.test.ts in #36304 fails once this is rebased
    Fix those diffs first (or gate on the per-workspace add/remove/update edges only) and add the install-then-frozen-passes tests for these shapes.

Comment thread src/install/PackageManager/install_with_manager.rs Outdated
Comment thread src/install/PackageManager/install_with_manager.rs Outdated
Comment thread src/install/PackageManager/install_with_manager.rs Outdated
Comment thread src/install/PackageManager/install_with_manager.rs Outdated
Comment thread test/cli/install/bun-install-registry.test.ts
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Agreed, gating on had_any_diffs as-is is wrong. Reworking this on top of current main: a narrow per-workspace dependency-edge drift flag in Diff::generate (propagated through the workspace recursion so the scripts/trusted/overrides comparisons can't trip it), fixing the loader/update-path cases that still produce spurious edge diffs on a stable repo, a more specific error naming the workspace that drifted, and install-then-frozen-passes round trips for each of the shapes above. Will push when it is green locally.

@robobun
robobun force-pushed the farm/8cbd0739/frozen-lockfile-new-direct-dep branch from 231fa6a to 8f005e3 Compare August 13, 2026 06:15
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/PackageJSONEditor.rs Outdated
Comment thread src/install/PackageManager/install_with_manager.rs Outdated
Comment thread src/install/lockfile/Package.rs
Comment thread src/install/lockfile/Package.rs
Comment thread src/install/lockfile/Package.rs
Comment thread src/install/pnpm.rs
@robobun
robobun force-pushed the farm/8cbd0739/frozen-lockfile-new-direct-dep branch from a95b43d to 3852ec0 Compare August 15, 2026 06:04
Comment thread src/install/lockfile.rs Outdated
Comment thread src/install/lockfile.rs
Comment thread src/install/lockfile/bun.lock.rs
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (head 3852ec0, on top of #38736); the round-3 changes are included. Since #38333 landed in between, this was more of a re-port than a rebase, so a summary of what changed relative to the version you last looked at:

  • Dropped: the bun update lockfile-specifier commit and the $ref override storage fix. install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 implemented both (package_json_write_back::sync_lockfile, bun-update-lockfile-sync.test.ts, overrides stored under the key), and with those tests applied on main only the new semantics differed (a named update now also moves aliases of that package), so the commit and its tests are gone. Down to five commits.
  • Differ: re-applied onto the new generate_inner. The member pass now also feeds the pruned-checkout bookkeeping (survivors, removed_names), changed_workspaces is folded into changes_resolutions / changes_dependencies / has_diffs so dedupe, prune, the transitive planner and clean keep seeing what they saw before, and the root remove count keeps install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333's combined meaning (root_remove carries the root's own count for the note).
  • Gate: main now has its own narrow check (overrides / catalog). Generalized it to changes_dependencies() rather than has_diffs(): frozen-lockfile-pruned.test.ts documents that trustedDependencies / patchedDependencies are deliberately not part of the frozen comparison (turbo prune strips them), and 16 of its tests failed with the round-3 gate. So those two sections no longer fail frozen; the round-3 work on them remains as differ accuracy plus the [] storage fix, and the tests for that direction now assert "passes frozen, stored by the next plain install". Notes use install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333's wording (dependencies in packages/x/package.json changed since bun.lock was saved (...)), so its existing assertions hold unchanged; one pre-existing inline snapshot gains the new note.
  • Writer: after install: keep trustedDependencies and patchedDependencies order in bun.lock #38736 the collection loop and its order are left exactly as on main; the only writer change left is emitting "trustedDependencies": [] when the field is declared but empty after filtering. bun-lock.test.ts including the new byte-order test passes.
  • New small commit: the pnpm migration never recorded member versions (it read them off the importer entry), which the loader rule needs to tell linked ranges apart, so bun pm migrate followed by --frozen-lockfile failed on pnpm-lock-v9.test.ts's link shape. It now reads the member's package.json like the npm migration does; two migration snapshots gain the "version" lines.
  • Note for the round-trip table: main now skips byte-identical re-saves, so "no Saved lockfile" no longer shows differ noise; the frozen step of each row is what does now.

Locally: all of test/cli/install/ passes apart from the network/git-clone tests (that includes frozen-lockfile-pruned, nested-overrides, bun-update-lockfile-sync, bun-lock, the migration suites), plus clippy and the source lints. Description rewritten for this state.

Comment thread src/install/lockfile.rs
@robobun
robobun force-pushed the farm/8cbd0739/frozen-lockfile-new-direct-dep branch from 3852ec0 to c6ce217 Compare August 15, 2026 08:06
Comment thread src/install/PackageManager/install_with_manager.rs
Comment thread src/install/PackageManager/install_with_manager.rs
Comment thread src/install/PackageManager/install_with_manager.rs
Comment thread src/install/lockfile.rs
Comment thread src/install/lockfile/bun.lock.rs
Comment thread src/install/lockfile/bun.lock.rs
Jarred-Sumner pushed a commit that referenced this pull request Aug 15, 2026
…#38853)

### Problem
- `bun install --frozen-lockfile` (and `bun ci`, `--production`) on main
fails with `error: lockfile had changes, but lockfile is frozen` on
committed lockfiles that 1.3.14 accepts, with no change to the project.
Reproduced on sst/opencode, elizaOS/eliza, supermemoryai/supermemory and
activepieces/activepieces at HEAD (4 of the 19 repos I checked whose
lockfile passes on 1.3.14; the other 15 pass on both).
- Since #35681, `clean_with_logger` drops packages that only an optional
peer slot still reaches. 1.3 never dropped them, so lockfiles it wrote
still list them: `bun remove encoding` on 1.3.14 leaves `encoding` in
`bun.lock` because `node-fetch` has an optional peer on it; opencode's
file carries `encoding`, `@vitest/coverage-v8` and two nested `effect`
copies this way.
- #38333 limited the drop to installs whose package.json diff is not
empty (`keep_optional_peer_targets = !summary.changes_resolutions()`,
`src/install/lockfile.rs`). That diff is not empty on many untouched
projects: a workspace with a lifecycle script (`bun.lock` does not
record workspace scripts, so it diffs as updated on every install;
supermemory) or a workspace dependency listed in two sections (opencode,
eliza, activepieces). 1.3.14 reports the same diffs (`--verbose` prints
`Workspace package "..." has ... updated 1 dependencies` on both) and
they are harmless for the frozen check by themselves, but on main they
switch the drop on: `Clean lockfile: 2711 packages -> 2697 packages` on
opencode where 1.3.14 prints `2711 -> 2711`, and `Lockfile::eql` then
rejects the file.

### Fix
- Keep the optional-peer-held targets whenever the lockfile is frozen
(`manager.options.enable.frozen_lockfile()`), in addition to the
existing in-sync case. A frozen install never saves the lockfile
(`PackageManagerOptions` clears `SAVE_LOCKFILE`), so the drop has no
output to affect there; all it could do is make the comparison reject
files an older version wrote. The frozen install then builds the tree
the file describes and installs exactly what 1.3 installed from it.
- Non-frozen installs are unchanged: with a diff they re-save anyway
(`had_any_diffs` in `install_with_manager.rs`), and the drop goes out
with that save, so the `bun remove` / edited package.json cases from
#35681 still clean up (their tests still pass).
- Test: `test/cli/install/bun-lock.test.ts`, "--frozen-lockfile keeps a
package that an older lockfile lists only as an optional peer". It
installs `optional-peer-deps` + `no-deps` in a project with a workspace
that has a `postinstall` script, then edits `bun.lock` into the shape
1.3 left behind (root edge to `no-deps` removed, entry kept) and removes
the dependency from package.json. Without the fix the frozen install
exits 1 with the error above; with it the install succeeds, leaves
`bun.lock` byte-identical and installs `no-deps`.
- One snapshot changes: `migrate.test.ts` > arborist fixtures >
`edit-package-json--changed` now records a frozen exit code of 0. The
fixture's only diff is a root range edit (`abbrev` `^1.1.0` vs `^1.1.1`)
that the locked version satisfies, which the frozen check accepts
everywhere else; it exited 1 only because that diff switched on the drop
of `semver`, which npm had installed as an optional peer. #38333
recorded the code when it added the sweep; manifest-drift strictness is
#33632's topic.
- With the debug build, the committed lockfiles of opencode, eliza and
supermemory pass `--frozen-lockfile` (`Clean lockfile: N -> N`); hono
and remotion still pass. `bun-lock.test.ts` (31),
`frozen-lockfile-pruned`, `frozen-lockfile-missing-workspace`,
`bun-dedupe`, `bun-prune`, `lockfile-only`, `bun-lockb`, `hoist`,
`nested-overrides`, `lockfile-version-2`, `catalogs`,
`isolated-install`, `bun-update`, `bun-remove` pass.
- activepieces still fails after this change for a second, independent
reason: its 1.3 lockfile nests `react-dom@18.3.1` under
`react-json-view`, whose peer range (`^15 || ^16 || ^17`) no satisfying
version exists for, and the loader's version-based peer binding
(`resolve_peer_dep_version_based`, `bun.lock.rs`) binds that edge to the
first candidate (`react-dom@19.2.5` at the root) instead of the entry
printed next to the dependent, which orphans 18.3.1. That is a loader
change and will be a separate PR.
- The spurious "updated" diff for a workspace dependency listed in two
sections is a separate pre-existing bug (it also makes `bun dedupe`
refuse a freshly installed project) and is tracked separately; #33632
lists more shapes that keep this diff non-empty on untouched projects,
which is why the frozen path should not depend on it.

### Background
- Optional peers: a package can declare a peer dependency as optional
(`peerDependenciesMeta`). Bun never installs one on its own; if a
package of that name is in the tree for another reason, the hoister
binds the edge to it. Such an edge is recorded in the lockfile as a
resolution slot like any other.
- `clean_with_logger` rebuilds the lockfile after resolution by walking
from the root and cloning every reachable package (`Package::clone` /
`Cloner`). Since #35681 it leaves optional peer slots out of that walk,
so a package reachable only through them is dropped;
`keep_optional_peer_targets` is the switch that puts them back into the
walk.
- `--frozen-lockfile` does not compare files. It loads the lockfile,
runs the same clean a normal install runs, and compares the two
in-memory trees with `Lockfile::eql` (the meta hash for `bun.lockb`);
any difference is reported as "lockfile had changes". It also disables
saving, so a frozen install can never write the result of the clean.
- `DiffSummary` is the comparison of package.json (root and workspaces)
against the root entries in the lockfile. `changes_resolutions()` is
true for any added, removed or updated row; a workspace whose lifecycle
scripts differ from the (never recorded) lockfile copy counts as
updated, as does a workspace whose rows do not round-trip exactly.

<details>
<summary>Field reproduction with public packages (1.3.14 binary vs
canary a5c86ae)</summary>

```sh
# package.json: workspaces ["packages/*"], dependencies node-fetch@2.7.0 + encoding@0.1.13
# packages/a/package.json: { "scripts": { "postinstall": "echo a" } }
bun-1.3.14 install --ignore-scripts
bun-1.3.14 remove encoding            # bun.lock keeps encoding, iconv-lite, safer-buffer (node-fetch's optional peer holds them)
bun-1.3.14 install --frozen-lockfile  # exit 0
bun-canary install --frozen-lockfile  # error: lockfile had changes, but lockfile is frozen
# drop the postinstall script from packages/a and canary passes too: the diff is what turns the drop on
```

Frozen dry-run on the committed lockfiles (exit codes 1.3.14 / main):
opencode 0/1, activepieces 0/1, eliza 0/1, supermemory 0/1; hono,
elysia, remotion, claude-code-action, create-better-t-stack,
react-starter-kit, sst (lockb), openauth (lockb), humanlayer, tscircuit,
tscircuit/cli, claude-code-base-action, setup-bun, bun.report, bunup
0/0; coolify, onlook, midday, eden fail on both (already out of date,
not regressions).
</details>

<!-- robobun:evidence:begin -->

---

**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

<!-- robobun:evidence:end -->

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed after the latest push and didn't find any new bugs. The aliased-npm trustedDependencies regression I flagged earlier is addressed on all three fronts (stores_trusted_dependency now matches the resolved npm package name, the writer stores it, and trusted_dependencies is copied unconditionally before the !had_any_diffs branch), with a dedicated test. Given the scope — the differ restructure, both loaders' workspace-edge tagging, and the frozen gate — a human pass is still worthwhile.

Also checked this run:

  • copy_patched_dependencies in the !had_any_diffs branch: the builder counts the paths before allocate(), so the append is safe.
  • diff_workspace_members iterating from_lockfile.packages while generate_inner sorts from_lockfile.overrides/catalogs — no reallocation of packages/buffers, so the raw views stay valid.
  • The unconditional trusted_dependencies copy runs before builder.allocate() and doesn't touch the builder, so no ordering hazard.
Extended reasoning...

Overview

This is a follow-up review after the author addressed my prior 🔴 finding about stores_trusted_dependency and aliased npm packages. The push applies all three suggested fixes: stores_trusted_dependency (lockfile.rs) now also matches when a resolved npm package has the given name; the bun.lock writer (bun.lock.rs) inserts pkg_name into found_trusted_dependencies for npm resolutions so the entry is persisted; and install_with_manager.rs now copies lockfile.trusted_dependencies into manager.lockfile unconditionally before the !had_any_diffs split, so the runtime trust check always sees the package.json list regardless of whether the differ counted a change. A new test (trustedDependencies naming the package behind an npm: alias is stored and stays trusted when installing from bun.lock) covers install 1, plain re-install, and --frozen-lockfile from the lockfile. copy_patched_dependencies was also extracted and now runs in both branches, with the builder counting the paths first.

The overall PR remains a substantial change: it restructures Diff::generate to stop recursing through root edges and instead compare every workspace package via diff_workspace_members; adds a re-resolve hook for changed members via dependency_to_reresolve_workspace; changes the bun.lock loader's tagging of workspace-resolved edges (map_manifest_dep_to_pkg) and its synthetic-root-edge synthesis; adds restore_workspace_dependency_paths to the bun.lockb loader; changes the bun.lock writer to emit "trustedDependencies": [] when declared-but-empty and to include aliased npm package names; moves trusted/patched comparison to a post-member-parse pass; generalizes the frozen gate to changes_dependencies(); and reads workspace member versions from package.json in the pnpm migration.

Security risks

None identified. The change is confined to lockfile diffing and the install flow's decision of when to re-resolve/re-save/refuse. The one script-execution-adjacent piece (trustedDependencies) now unconditionally sources from package.json, which is strictly no-worse-than-main and closes the hole where the loaded lockfile's filtered list could diverge from the manifest.

Level of scrutiny

High. This is core package-manager logic that every bun install / bun ci exercises, with a long tail of monorepo shapes; the PR itself has been through four review rounds with three previous bugs found and fixed (spurious frozen failures on stale trusted/patched entries, members reached only via override going unchecked, all-stale trustedDependencies re-enabling the default list, and the aliased-npm regression). The differ restructure changes what had_any_diffs / changes_dependencies mean for downstream consumers (dedupe, prune, transitive planner), and the loader tagging changes affect how loaded edges compare against fresh parses under both formats. This is not a mechanical change I can approve without a human look, especially with a human reviewer already engaged.

Other factors

  • Extensive test coverage: 16 round-trip shapes × 2 formats, 5 failing-direction tests, the aliased-trusted test, the electron default-list tests, and updated bun-install.test.ts assertions that the second install no longer re-resolves.
  • The remaining unresolved comments are comment-cop bot noise on doc comments that the author has justified on prior rounds; not blocking.
  • CI on the pre-fix head passed except two darwin lanes that expired for lack of an agent; no CI result yet on the current head.

…ready up to date

A plain `bun install` re-saved a byte-identical lockfile on every run for
several stable repo shapes because Diff::generate compared state the lockfile
never stores or that the loaders reconstructed differently from a fresh
package.json parse:

- lifecycle scripts: bun.lock stores none, so every workspace member with a
  script counted as updated. Only compare them when the loaded package
  recorded scripts (bun.lockb), which is also the only case where a change
  has to refresh anything.
- trustedDependencies: the lockfile stores the union across workspaces but
  the comparison ran before the workspace package.jsons were parsed, so a
  list declared in a member was reported removed every time (and never
  reported added). Compare once, at the root, after the members have been
  parsed. overrides and patchedDependencies are likewise compared once at
  the root instead of once per workspace; the caller already applies
  changes to those sections across every package's dependencies.
- bun.lock only stores the trustedDependencies names and patchedDependencies
  keys that match something in the tree, while the differ compared the full
  package.json sets, so a stale entry (a trusted name that is not installed,
  a patch for a version that is not) was reported as added on every install.
  Entries that are only in package.json now count as additions when the
  lockfile has something for them to apply to (Lockfile::stores_trusted_dependency,
  stores_any_patched_dependency); what the lockfile already holds is compared
  as before. A trusted name applies to an npm package under the package's
  own name (has_trusted_dependency), which the writer missed when the
  package is installed under an npm: alias; it now stores that name too,
  and stores_trusted_dependency follows the same rule. Declaring the field
  at all is what turns the default trusted list off, so bun.lock now writes
  "trustedDependencies": [] when none of the names are stored (bun.lockb
  already had a tag for this), and a package.json that declares the field
  while the lockfile lacks it counts as a diff. Before, a bun.lock written
  for `"trustedDependencies": []` dropped the field, and from the second
  install on the default list was back in effect.
  Since the differ now leaves some package.json entries out on purpose,
  install_with_manager copies both sections from package.json whether or
  not it reported a diff (as it already did for the root scripts); before,
  a run without a diff kept the loaded subsets, so an entry it had left out
  did not apply to what that run installed (`bun audit fix` moving a
  package to the version a patch is written for, for instance).
- workspace members were compared against their package.json by recursing
  through the root's edges that carry the workspace behavior, which the
  loader guaranteed to exist by synthesizing one per member (next bullet).
  Once the loader follows the parser, a member the root reaches some other
  way (its own entry for the member plus an override, or only through
  another member) has no such edge, so every workspace package in the
  lockfile is compared instead, and the install re-resolves one dependency
  on each changed member, which re-reads it into the same package id.
- the bun.lock loader re-tagged every root/workspace edge that resolved to a
  workspace package as a workspace dependency, but skipped the second entry
  when a name appears in two dependency sections. The parser keeps catalog:,
  dist-tag, peer and override-forced edges on their own tag, tags every
  entry, and replaces the root's synthetic edge for a member with the root's
  own entry when that entry is an npm range it does not link. The loader now
  follows those rules for the edges the differ looks at.
- bun.lockb stores an edge as tag plus literal, so a member's workspace edge
  reloaded holding the literal's value (`*`) while a fresh parse holds the
  member's path, which is what Version::eql compares; every monorepo on
  bun.lockb whose members depend on each other re-saved each run. The edges
  of the root and workspace packages are pointed back at the member's path
  after loading, as the bun.lock loader does.
Diff::generate also records which workspaces changed so callers can say so.
The pnpm migration looked for the version on the pnpm-lock.yaml importer
entry, which never has one, so migrated bun.lock files listed every member
without its version. bun's own writer records it, and the bun.lock loader
uses it to tell a range that links a sibling (`"common": "^1.0.0"`) apart
from one that does not; without it every install after `bun pm migrate`
saw that member's dependency as changed, which --frozen-lockfile now
reports. Take the version from the member's package.json, which the
migration already reads for the name, the way the package-lock.json
migration does.
…k, and say which one

The frozen check compared the resolved package set (Lockfile::eql for
bun.lock, the meta hash for bun.lockb) plus, since the pnpm parity work,
the overrides and catalog sections. A package.json change that resolves to
packages already in the lockfile, such as a dependency added to one
workspace that another already pulls in, or a range change the locked
version still satisfies, passed even though the lockfile no longer matches
the manifests. The dependency comparison Diff::generate already performs
(root and workspace members) is now part of the check as well, through
DiffSummary::changes_dependencies, and the error names each manifest and
section that diverged.

trustedDependencies and patchedDependencies stay out of the comparison:
they are applied from package.json either way and tools like `turbo prune`
strip them from bun.lock, which the pruned-checkout tests rely on. Lifecycle
script changes are left out for the same reason.
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up on overlap: #38869 is the bun.lock loader piece of this PR's first commit on its own (dropping seen_deps, and a map_manifest_dep_to_pkg that links an entry only when Package::parse_dependency would, with the same linking-off rule this PR arrives at: ranges keep loading as linked, peers and overridden entries do not), plus two things that came out of its review: the range's name is compared against the bound workspace's name rather than trusting the hash-keyed lookups, and catalog:/dist-tag entries are left as parsed with linking off too. It also carries the same six bun-workspaces snapshot hunks. If #38869 lands first, this PR's loader hunk and those snapshot hunks rebase away and the synthetic root edge skip, the frozen-lockfile gate and the rest stay as they are.

Failing direction for the gate (root and workspace manifests, both lockfile
formats, members the root reaches through an override or only through a
sibling), a table of stable repo shapes that must install twice without a
diff and pass --frozen-lockfile under both formats, the trustedDependencies
field surviving a bun.lock round trip (default list stays off, `electron`
fixture, a name listed for a package installed under an npm: alias), a
patchedDependencies entry for the version `bun audit fix` moves to, and the
two existing bun.lockb workspace tests that stopped re-resolving on every
install.

The arborist fixture edit-package-json--changed now records a frozen exit
code of 1: its package.json was edited after its lockfile was written
(abbrev ^1.1.0 against the migrated ^1.1.1), which is the manifest drift
the gate is for.
…endency's tag

The bun-workspaces snapshots recorded "tag": "no-deps" for latest and
unknown-tag dependencies; they now show the tag.
@robobun
robobun force-pushed the farm/8cbd0739/frozen-lockfile-new-direct-dep branch from c6ce217 to 13f2595 Compare August 15, 2026 11:38
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Since the rebase comment (head is now 13f2595, still five commits):

Description updated for this state. Locally the lockfile, patch, frozen, audit, registry, workspaces and migration suites pass; the remaining install suites only had the usual network tests and load-induced timeouts failing, all of which pass when re-run alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants