install: print the update row for catalog entries moved by bun update - #38763
install: print the update row for catalog entries moved by bun update#38763robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes 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 (6)
Comment |
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 870f7ab has some failures in 🧪 To try this PR locally: bunx bun-pr 38763That installs a local version of the PR into your bun-38763 --bun |
|
Status: reproduced on a debug build of main at e7460e3 with a local registry serving zed@1.0.0 / 1.4.0 / 3.0.0 and a root catalog The row assertions added to CI for the current revision 870f7ab (build 97000) has finished: 177 of 179 jobs passed (every Linux platform including ASAN, Windows x64 and arm64; the few yellow entries passed on retry) and no test failed anywhere. The remaining two jobs are the macOS arm64 test shards, which expired four times in a row without ever being assigned an agent: the test-darwin queue has been around 300 jobs deep all night and PR builds run at a lower priority than main and merge-queue builds there, so that lane is currently starving every PR. Nothing in this diff is platform-specific, and the merge queue runs those shards at a higher priority, so I am leaving the build as is rather than re-pushing into the same queue; the two jobs can also simply be retried once the backlog clears. Ready for review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
A bare `bun update` (with or without --latest) rewrites the root's catalog entries and bun.lock, but the install summary never showed a `^ name from -> to` row for them: the row printer reads `updating_packages`, which only the cwd's own dependency lists (and the transitive plan) fill in, and `record_updating_package_versions` skips `catalog:` rows. Depending on who consumed the entry the output was nothing, a plain `+ name@new` install row, or `(no changes)` when the new version was already in the isolated store. Before the differ runs, register every `catalog:` row of a recorded catalog entry under its dependency name with the version it was locked to, marked as a catalog entry and already written back (the entry itself is written by edit_catalogs_after_update, so the cwd's dependency lists must not pick the registration up). The summary then prints the walked importers' catalog rows as update rows, and additionally walks the `catalog:` rows of names registered this way so an entry consumed only by a workspace outside the walked set is reported too, whether or not the new version had to be installed. The version copy shared by the three registration sites moves into PackageUpdateInfo::set_original_version.
The verbose summary prints a section per importer, and each section already prints the ^ rows of its own catalog: dependencies, so walking the catalog rows from the ungrouped section as well printed a moved entry twice. Only the non-verbose summary, which has a single section, walks them.
035a078 to
870f7ab
Compare
There was a problem hiding this comment.
LGTM. My earlier --verbose double-print note doesn't apply to this revision — print_catalog_entry_updates is gated on sole_section, which is false on both verbose-path calls, and the new --latest --verbose test pins exactly one row under pkg1:.
What was reviewed:
record_catalog_originalsregistration:written_back: true+catalog_entry: truekeepsedit_update_entriesfrom rewriting a same-named non-catalog dep (traced the root"latest"case); direct-dep registrations win viafound_existing.set_original_versionrefactor matches the priorregister_movedshape (unconditionalclone_into); the oldinstall_with_managervariant only differed by skipping a zero-length tag buffer.sole_sectionthreading: verbose passesfalse(root and per-workspace), non-verbose passestrueon the single call — catalog sweep runs exactly where the sharedupdate_dedupeholds.
Extended reasoning...
Overview
Cosmetic fix to bun update's install summary: catalog entries that move now print a ^ name old -> new row like direct dependencies do, instead of nothing (from root), a + install row (from a member), or (no changes) (isolated rerun). Touches five Rust files in src/install/ — a new record_catalog_originals that registers catalog rows in updating_packages alongside direct deps, a catalog_entry flag on PackageUpdateInfo, a print_catalog_entry_updates sweep in the tree printer gated by a new sole_section parameter, and a shared set_original_version helper replacing three open-coded copies. Tests add row assertions to every existing update case and three new tests (root-consumer, --verbose, isolated-store rerun).
Prior review
My earlier inline comment claimed a --verbose double-print. That was a misread: print_catalog_entry_updates only runs when sole_section is true, and both verbose-path calls pass false. In verbose mode each workspace's own update_owners walk prints its catalog: rows (root has none, so only pkg1: prints). The new --latest --verbose test asserts exactly one row under pkg1:, directly covering that scenario. The concern is moot for this revision.
Security risks
None. This changes only what the install summary prints; lockfile writing, package.json write-back, and installation are unaffected. The one write-back interaction — written_back: true on catalog-registered entries — was traced: edit_update_entries (after-install) short-circuits on written_back, so a same-named non-catalog dependency of the cwd is not rewritten from the catalog entry, and a name the cwd's dependency lists registered first skips catalog registration entirely (found_existing).
Level of scrutiny
Medium. The package manager is critical, but the surface here is the summary printer plus a bookkeeping map (updating_packages) whose only other consumer is the post-install package.json write-back, guarded by written_back. The set_original_version consolidation is a straight refactor of two existing shapes into the register_moved one (unconditional clone_into into an owned tag buffer; the old install_with_manager variant only skipped a zero-length allocation).
Other factors
Tests are thorough — 8 row assertions across the existing --latest/plain/-r/from-member cases plus dedicated root-consumer, verbose, and isolated-rerun tests, and a negative assertion that the unmoved a-dep entry produces no row. The PR description confirms all fail on the unfixed build. CI is green on the install suites; the two red lanes are unrelated pre-existing failures per the robobun status.
2 participants
Problem
bun update/bun update --latestthat moves a root catalog entry rewrites package.json and bun.lock but the summary never prints the^ name old -> newrow for it. Catalogzed: ^1.0.0consumed by a workspace member, locked at 1.4.0, latest 3.0.0:bun update --latestfrom the root prints only1 package installed; with-rthe same.zed: "catalog:", it prints+ zed@3.0.0(a plain install row).node_modules/.bun, a second run printsDone! Checked 3 packages (no changes)while a semver-major bump was written.zed: ^1.0.0prints^ zed 1.4.0 -> 3.0.0in every one of those cases, and--dry-run/--lockfile-onlyalready print the row for the catalog case too (update_transitive::print_plandiffs the rows directly), so only the install summary disagreed.manager.updating_packages(tree_printer.rs,should_print_package_install), keyed by dependency name with the version the name was locked to.edit_catalogs_before_updaterecords catalog entries only inupdating_catalogs, andrecord_updating_package_versions(install_with_manager.rs) skips every row whose version is notNpm/DistTag, so acatalog:row never has an original to compare against. The printer also only walks the rows of the workspaces whose package.json it edited, so an entry consumed elsewhere had no row to print from even if it had been registered.Fix
package_json_editor::record_catalog_originals(called at the end ofrecord_updating_package_versions, i.e. on the loaded lockfile before the differ re-enqueues anything): for everycatalog:row whose entryedit_catalogs_before_updaterecorded, register the dependency name inupdating_packageswith the row's locked version as the original, the same shape the cwd's own dependency lists produce. The entry is markedcatalog_entryandwritten_back, so the post-installedit_update_entriespass does not rewrite a same-named non-catalog dependency of the cwd from it (verified: a rootzed: "latest"next to a catalogzedentry stays"latest"on a plain update). A name the dependency lists already registered keeps their original, so the existing+/^output for direct dependencies is unchanged.tree_printer.rs: rows of the walked workspaces now print^rows for theircatalog:dependencies for free (and stop printing+for them). In the normal single-section summary,print_catalog_entry_updatesadditionally walks thecatalog:rows of names registered this way, sharing the section's per-name dedupe, so an entry consumed only outside the walked set is reported too; the--verbosesummary prints one section per workspace, each of which already reports its owncatalog:rows, so it skips the walk (sole_section) and a moved entry shows up once, under the workspace that depends on it. Both go throughshould_print_package_install, which forbun updatereports a moved row whether or not its new version had to be installed, so the isolated-store rerun now prints the row as well (the count line stays(no changes), as it does for a direct dependency in the same situation).--filter <member>(catalogs untouched, no row).record_updating_package_versionsandupdate_transitive::register_movedeach open-coded becomesPackageUpdateInfo::set_original_version, used by all three registration sites. (semver: record cloned prerelease/build tag offsets against the whole destination buffer #38687 changes theclone_intosignature those copies call; whichever lands second adjusts this one helper.)test/cli/install/catalogs.test.ts(updateblock): the--latestvariants (catalog inworkspaces, top-levelcatalog,-r), the plain update within range (with and without-r), the run from inside a member, plus three new cases: the root itself consuming the entry (one row, no+row),--verbose(exactly one row, underpkg1:) and the isolated-store rerun. All 9 row assertions fail on the unfixed build (nullor+ no-deps@2.0.0instead of the row) and pass with the fix; the unmoveda-depentry is asserted to produce no row. Also ranbun-update.test.ts(156),bun-update-transitive,bun-update-lockfile-sync,bun-add-catalog,lockfile-only(413) and theupdatesubset ofbun-install-registry.test.ts: all green.Background
catalog/catalogsmaps hold version ranges; a workspace declares"zed": "catalog:"(orcatalog:<group>) and resolves through the map. A barebun updatetreats the entries like the root's own dependencies:edit_catalogs_before_updatepoints them atlatest(with--latest) in memory and remembers each entry inupdating_catalogs; after resolving,edit_catalogs_after_updatewrites the resolved range back into the root package.json.updating_packages: the map a bare update keeps per dependency name, holding the literal being updated and (record_updating_package_versions) the version bun.lock had for it. The install summary walks the rows of the updated workspaces and prints^ name old -> newfor every row whose name is in the map and whose resolution now differs from that recorded version; that check runs before the "was it installed" check, which is why direct dependencies print even when nothing new was linked.update_transitive::register_moveduses the same map, with an empty literal, for packages moved by the transitive half of the update;print_transitive_updatesprints those from the set of packages installed this run, which is why this change does not reuse that path for catalogs.[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file