install: write a migrated lockfile and its package.json edits only when the command saves a lockfile - #38804
install: write a migrated lockfile and its package.json edits only when the command saves a lockfile#38804robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
Comment |
|
Status: reproduced on main ( Fix and tests are in this PR ( CI for 9b53a1c (build 97150): 177 of 179 jobs passed with no test failures. The remaining two are the darwin 14 aarch64 test lanes, which expired without ever running because the |
…en the install saves a lockfile Loading a package-lock.json, yarn.lock, or pnpm-lock.yaml when there is no bun.lock used to write files as a side effect of the load: bun.lock was saved even under --frozen-lockfile, --dry-run, and --no-save, and the pnpm migration wrote the root package.json (workspaces, overrides, patchedDependencies moved in from pnpm-workspace.yaml and the pnpm key) from inside the migrator, so read-only commands such as bun outdated and bun pm why rewrote package.json too. The pnpm migration now only edits the cached package.json entry and records what it moved on the PackageManager. install_with_manager, bun pm migrate, and bun pm trust queue that entry for package_json_write_back at the point where they save the lockfile, so both files are written together or not at all. The unconditional save exception is narrowed to the documented bun.lockb -> bun.lock conversion; a migrated lockfile saves through the normal FORCE_SAVE_LOCKFILE path, which --frozen-lockfile, --dry-run, and --no-save disable. --frozen-lockfile prints a note saying the migration was not written. --lockfile-only honors the same flag. bun remove writes the cache entry instead of the source it printed before the install, so it no longer drops the migration's edits. Messages about the loaded lockfile name the migrated file instead of bun.lock.
…migrate and bun pm trust Both commands save the lockfile without looking at --dry-run / --no-save, so routing the package.json half through flush (which those flags disable) left bun.lock migrated and package.json not. Write the entry directly instead.
5ef3df5 to
9b53a1c
Compare
There was a problem hiding this comment.
Thanks for addressing the earlier note — write_migrated_root now writes package.json directly (bypassing flush's WRITE_PACKAGE_JSON gate) and the pm migrate --dry-run test confirms both files land together. I re-reviewed and found no further issues.
Given this reverses the #38333 decision on whether --frozen-lockfile writes bun.lock during migration (flagged as a judgment call in the description) and reshapes the write-path across install/add/remove/pm migrate/pm trust, a maintainer should confirm the behavior change before merge.
What was reviewed:
converts_binary_lockfile_to_textcorrectly narrows the frozen-write exception to bun.lockb→bun.lock only; migrated foreign lockfiles now go throughFORCE_SAVE_LOCKFILEand are gated byDo::SAVE_LOCKFILE.- The
bun remove/patch --commitwrite-back now reads the cache entry (which the migration edited) instead of the stale pre-install print — verified this is the same path for both branches. write_migrated_rootvsrecord_migrated_rootsplit keeps bun.lock and package.json paired at all four save sites; thepm migrate --dry-runandpm trustcases I raised earlier are covered.
Extended reasoning...
Overview
This PR changes when the package manager writes a migrated lockfile and its accompanying package.json edits to disk. Previously, migrating from pnpm-lock.yaml/package-lock.json/yarn.lock wrote bun.lock (and, for pnpm, rewrote package.json) even under --frozen-lockfile, --dry-run, --no-save, and read-only commands like bun outdated. Now the pnpm migration edits only the in-memory cache entry, and the file writes are deferred to the same points that save bun.lock — so both files are written together or not at all. Touches install_with_manager.rs, package_json_write_back.rs, updatePackageJSONAndInstall.rs, pnpm.rs, migration.rs, lockfile.rs, update_scope.rs, package_manager_command.rs, pm_trusted_command.rs, plus docs and three test files.
Prior feedback addressed
My earlier inline comment noted that bun pm migrate --dry-run / bun pm trust --dry-run would write bun.lock unconditionally while the WRITE_PACKAGE_JSON gate on record_migrated_root skipped package.json, breaking the pair. Commit 9b53a1c split the function: record_migrated_root (install path, flushed later) and write_migrated_root (pm migrate/trust, calls write_target directly). The doc comment on write_migrated_root explains the reasoning, and the new ["pm migrate --dry-run", ...] test case in pnpm-lock-v9.test.ts asserts both files are written together. This resolves the concern.
Security risks
None identified. No untrusted input parsing changes; the pnpm YAML and package.json parsing paths are unchanged. The change is about when already-computed edits hit disk, not what is computed.
Level of scrutiny
High — this is core package-manager write-path logic with user-visible behavioral changes. The PR explicitly reverses two tests from #38333 and changes what --frozen-lockfile does during migration; the author correctly flags this as a judgment call. There are also three overlapping open PRs (#38754/#38775/#38780) touching update_package_json_after_migration. A maintainer should confirm the --frozen-lockfile semantics change and the interaction with those PRs.
Other factors
Test coverage is thorough: the new package.json edits block in pnpm-lock-v9.test.ts covers frozen/ci/production/dry-run/no-save/outdated/pm why/pm untrusted (untouched), install/lockfile-only/pm migrate/pm migrate --dry-run/add/remove/pm trust/silent (both written), and the out-of-sync frozen error. lockfile-only.test.ts covers all three foreign formats × frozen/dry-run/no-save. The one CI failure (test-http-chunk-problem.js) is unrelated to this change.
Problem
bun install --frozen-lockfile/bun cion a repo that has apnpm-lock.yaml(orpackage-lock.json/yarn.lock) but nobun.lockinstalls correctly, then leaves the checkout dirty:bun.lockis created and, for pnpm,package.jsonis rewritten (workspaces,overrides,patchedDependenciesmoved in,pnpmkey removed).git diff --exit-codestyle CI gates and turbo hashing trip on it (pnpm parity ledger bun install prints npmrc registry authentication credentials when npmrc options cant be parsed #14444).saves_migrated_lockfile(src/install/PackageManager/install_with_manager.rs, added in install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333) saved any migrated lockfile regardless ofDo::SAVE_LOCKFILE. That flag is what--frozen-lockfile,--dry-run, and--no-saveclear, so on current mainbun install --dry-runandbun install --no-savealso writebun.lockfor all three foreign formats (1.4.0-canary before install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 did not).package.jsonitself, from inside the lockfile load (update_package_json_after_migrationinsrc/install/pnpm.rs, theFile::write_fileat the end). Every command that loads the lockfile runs the migration, sobun outdated,bun pm why,bun pm untrusted,bun audit,bun install --dry-run, etc. all rewrotepackage.jsonon a pnpm repo.bun removethen overwrote the file again with the source it had printed before the install, leavingbun.lockwith workspaces thatpackage.jsondoes not declare (the next install fails withWorkspace dependency "a" not found).Fix
update_package_json_after_migrationnow only edits the cached rootpackage.jsonentry (print_package_json_into_cache_entry+reparse_root, the same pathbun adduses) and appends what it moved to a newPackageManager.migrated_package_json_moveslist. The frozen check and the differ already read the cache entry, so frozen installs behave exactly as before; nothing touches disk during the load.package_json_write_backtakes that list over exactly where the lockfile is saved, printing themoved ... in package.jsonline once. The two save sites ininstall_with_manager(the normal path and--lockfile-only, which now also flushes) queue the entry for the existingflush;bun pm migrateandbun pm trust(write_migrated_root) write the entry directly, because they save the lockfile without consulting--dry-run/--no-saveandflushhonors those flags (pm trustalso re-readspackage.jsonfrom disk before editing it, so the migration's edits have to be on disk first).bun.lockandpackage.jsonare therefore written together or not at all, for every command.saves_migrated_lockfilebecomesconverts_binary_lockfile_to_text: the only save that ignoresDo::SAVE_LOCKFILEis the documentedbun.lockbtobun.lockrecipe (--save-text-lockfile --frozen-lockfile --lockfile-only, still covered bybun-lock.test.tsandfrozen-lockfile-pruned.test.ts). A migrated lockfile is saved throughFORCE_SAVE_LOCKFILElike any other change, so--frozen-lockfile,--dry-run, and--no-savekeep it in memory.--lockfile-onlychecks the same flag instead of onlyfrozen_lockfile(), so--lockfile-only --dry-run/--no-savestop writing too.--frozen-lockfilethe install prints one line, e.g.note: the lockfile is frozen, so the migration from pnpm-lock.yaml was not written to bun.lock and package.json; run 'bun install' and commit the result.--dry-run/--no-saveprint nothing extra.bun removewrites the cache entry for the file it edited instead of the stale pre-install print, which is what thepatch --commitbranch next to it already did; the two branches are now one.loaded_lockfile_namenames the migrated file, so the frozen failure readsoverrides in package.json changed since pnpm-lock.yaml was savedinstead of claiming abun.lockthat does not exist (update_scopenow shares the helper). Small and on the same path; easy to drop if unwanted.lockfile-only.test.tscases from install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 that pinned "--frozen-lockfilewritesbun.lockwhen migrating". The bun.lockb recipe test infrozen-lockfile-pruned.test.tscalls that recipe "the one sanctioned frozen write", the flag's docs say it disallows lockfile changes, and pnpm's--frozen-lockfilenever writes, so the foreign-lockfile write looked like the outlier. Keeping it would be a one-predicate change on top of the rest of this PR.test/cli/install/lockfile-only.test.ts:--frozen-lockfile,--dry-run,--no-save(with and without--lockfile-only) against all three foreign formats write nothing; plain install still writes. The migration cases and the--dry-run/--no-save--lockfile-onlycases fail on main (bun.lockexists / gets rewritten).test/cli/install/migration/pnpm-lock-v9.test.ts, newpackage.json editsblock: frozen /ci/--productioninstall without writing either file;--dry-run,--no-save,outdated,pm why,pm untrustedleave both files alone;install,install --lockfile-only,pm migrate,pm migrate --dry-run,add,remove,pm trust,install --silentwrite both and the result passes--frozen-lockfile; the out-of-sync frozen error still fires. The untouched /remove/ frozen-note cases fail on main.migrate.test.tsB7 used--frozen-lockfile --lockfile-onlyas a way to writebun.lock; it now uses--lockfile-only(the frozen round trip is asserted separately in the same test).migrate.test.ts,nested-overrides.test.ts,pnpm-*.test.ts,yarn-lock-migration.test.ts,bun-lock.test.ts,frozen-lockfile-pruned.test.ts,bun-dedupe.test.ts,bun-audit.test.ts -t fix,bun-update*.test.ts,bun-add*.test.ts,bun-remove.test.ts,bun-patch.test.ts,bun-install-patch.test.ts, thepm trusttests inbun-install-lifecycle-scripts.test.ts;cargo clippy -p bun_install -p bun_runtimeis clean.update_package_json_after_migration: install: import pnpm-workspace.yaml even without a migratable pnpm-lock.yaml #38754, install: keep the pnpm block in package.json when migrating from pnpm #38775, install: migrate pnpm onlyBuiltDependencies and report blocked scripts under the isolated linker #38780. They change what the function edits; this one changes when the file is written. Whichever lands second needs a small rebase: push the extra moves intomigrated_package_json_moves, drop thedirargument, and (for install: import pnpm-workspace.yaml even without a migratable pnpm-lock.yaml #38754's new call site) rely on the save-time write instead of writing in place.Background
bun.lock/bun.lockbis missing,Lockfile::load_from_dirfalls through tomigration::detect_and_load_other_lockfile, which builds the in-memory lockfile frompackage-lock.json,yarn.lock, orpnpm-lock.yaml. Every command that loads the lockfile with "attempt other lockfiles" set (install,add,remove,update,outdated,pm why/ls/trust/untrusted/migrate,audit, ...) goes through it, so anything the migrator does to disk happens for all of them.pnpm-workspace.yamland thepnpmkey ofpackage.json; bun reads them from root-levelpackage.jsonfields. The pnpm migration therefore has to editpackage.json, and the rest of the install has to see those edits, or the frozen check reports that overrides / workspaces changed.WorkspacePackageJSONCacheis the per-process cache of parsedpackage.jsonfiles keyed by absolute path. The install reads the root manifest from it, which is why editing the cache entry is enough for the load;MapEntry::reparse_rootre-derives the cached AST from the printed text so later readers ofentry.root(bun update -r,audit fix, the frozen error message) see the edits as well.package_json_write_backis the existing deferred writer: commandsrecordthe cache entries they edited, andflush(run afterbun.lockis saved, gated onDo::WRITE_PACKAGE_JSON, which--dry-run/--no-saveclear) writes the ones whose bytes differ from disk. The migration's edit is now one more recorded entry.Do::SAVE_LOCKFILEis cleared by--frozen-lockfile,--dry-run,--no-save, andBUN_CONFIG_SKIP_SAVE_LOCKFILEinPackageManagerOptions::load;Enable::FORCE_SAVE_LOCKFILEis set for migrated loads ininstall_with_managerand only consulted whenSAVE_LOCKFILEis on.Before / after on a pnpm repo (root package.json with a pnpm.overrides block, pnpm-workspace.yaml, one workspace member, no registry needed)
main (
e7460e3c7):this branch:
no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/migration/migrate.test.ts