Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesThe update flow identifies catalog-only packages, filters catalog edits to requested names, preserves Catalog-aware update flow
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:38 PM PT - Aug 12th, 2026
❌ @autofix-ci[bot], your commit 81092a9 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32810That installs a local version of the PR into your bun-32810 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Is there no existing catalog map? I feel like there is?
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/install/PackageManager/PackageManagerEnqueue.rs`:
- Around line 2020-2022: The re-resolution predicate in PackageManagerEnqueue is
too broad because it only checks `updating_packages` by package name, so catalog
dependencies with the same name can be re-resolved across unrelated catalog
groups. Update the logic around `is_named_catalog_update` and the `is_root_dep`
bypass to also compare `dependency.version.catalog()` against the
`UpdateRequest.catalog_name` for the matching request, so only the intended
catalog group is treated as eligible for re-resolution.
In `@test/cli/install/bun-update.test.ts`:
- Around line 449-468: The subprocess test is reading piped output sequentially
instead of draining it concurrently, which can block the child process when
stderr/stdout fills up. Update the relevant spawn-based checks in
bun-update.test.ts to consume stdout, stderr, and exited together using
Promise.all, including the initial install step and the update step. Use the
existing spawn calls and the variables returned from bunExe(), package_dir, and
env to locate the affected test blocks and keep pipe draining concurrent as
required by the test guidelines.
🪄 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: 32b5da52-aca4-4363-afa5-d5ce1cb7980f
📒 Files selected for processing (4)
src/install/PackageManager/PackageJSONEditor.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/UpdateRequest.rstest/cli/install/bun-update.test.ts
|
Yeah, there are two existing pieces and I went through both:
Separately, heads up that #32064 overlaps this area (it covers the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/PackageManager/PackageJSONEditor.rs:900-920— When a catalog-only package is updated with an explicit version (bun update baz@^1.0.0) or with--latest, the request is markedis_catalogand skipped by both the append-new-dep block and thee_stringrewrite loop — sorequest.versionandDo::UPDATE_TO_LATESTare never applied. The catalog literal stays at its old constraint during install, resolution happens within the original range, andcommit_catalog_updatewrites back with the original prefix, silently dropping the user's explicit constraint. Consider either rewriting the catalog literal to the requested version /latestin thebefore_installpass (so resolution honors it), or at minimum emitting a warning that@<version>/--latestis not yet supported for catalog entries.Extended reasoning...
What the bug is
The new catalog handling in
edit()correctly interceptsbun update <name>for catalog-only packages and re-resolves them within the existing constraint. But it does so unconditionally — it never inspectsrequest.versionorDo::UPDATE_TO_LATEST. So when the user supplies an explicit constraint (bun update baz@^1.0.0) or passes--latest, that information is silently discarded: the package is re-resolved only within the catalog's existing semver range, and the result is written back with the original prefix.Code path
In
UpdateRequest::parse_with_error, inputbaz@^1.0.0hits the@-split branch:alias = Some("baz"),value = "^1.0.0", sorequest.is_aliased = true,request.name = "baz", andrequest.versioncarries^1.0.0with tagNpm.get_name()therefore returns"baz".In
edit()(before_install pass), the dependency-group scan finds nothing (baz isn't in any root dep group), sorequest.e_stringstaysNone. The new catalog block then runs:if let Some(catalog_name) = find_package_catalog(current_package_json, name) { if options.before_install { manager.updating_packages.get_or_put(name)?; } request.is_catalog = true; request.catalog_name = catalog_name; } if request.is_catalog { remaining -= 1; }
request.version(^1.0.0) is never read. Withis_catalog = trueande_string = None, the request is excluded from theremaining != 0append block (if request.e_string.is_some() || request.is_catalog { continue; }) and from the post-loopfor request in updates.iter_mut() { if let Some(e_string) = request.e_string { ... } }rewrite — which is the only placeDo::UPDATE_TO_LATEST(→b"latest") andrequest.version.literalare propagated for non-catalog deps.During install,
PackageManagerEnqueue.rsresolves the workspace'scatalog:aidependency vialockfile.catalogs.get(...), which returns the catalog's existing~0.0.3constraint parsed from the unmodified package.json.is_named_catalog_updateforces re-resolution, butfind_best_versionis called with~0.0.3, not the user's^1.0.0orlatest.After install,
commit_catalog_updatereads the original literal (~0.0.3), callswhich_version_is_pinnedon it (→Minor), and writes back~<resolved>— preserving the original~and discarding the user's^.Why nothing prevents it
For non-catalog deps, an explicit
@<version>works because the request either matches an existing dep-group entry (capturing itse_string) or falls into the append block (allocating a freshe_string); either way the final loop rewrites that slot.--latestworks the same way via the'uninitializedbranch. Catalog requests deliberately bypass both paths to avoid synthesizing a root dep, but no equivalent rewrite of the catalog literal was added.Step-by-step example
Setup: root
package.jsonhascatalogs: { ai: { baz: "~0.0.3" } };packages/serverdepends onbaz: "catalog:ai". Registry has0.0.3,0.0.5,1.0.0withlatest = 1.0.0.- User runs
bun update baz --latest(orbun update baz@^1.0.0). parse_with_error→name="baz",is_aliased=true; for--latest,Do::UPDATE_TO_LATESTis set on options.edit()before_install: dep-group scan misses;find_package_catalogmatches →is_catalog=true,remaining -= 1. Catalog literal in the AST is not touched.- Install:
should_updateis true viais_named_catalog_update; resolver picks the best match for~0.0.3→0.0.5. edit()after_install:commit_catalog_updatereads original~0.0.3, formats resolved0.0.5, appliesPinnedVersion::Minor→ writes~0.0.5.- Result:
catalogs.ai.baz = "~0.0.5". Expected:^1.0.0(or1.0.0). No warning, no error.
Contrast with a non-catalog
dependencies: { baz: "~0.0.3" }:bun update baz --latestrewrites the slot to"latest"before install, resolves to1.0.0, and writes back^1.0.0.Impact
Users who try to bump a catalog dependency past its current major/minor with
--latestor@<ver>will see the command succeed but the catalog stay pinned within the old range. This is inconsistent with howbun update <name>@<ver>/--latestbehaves for ordinary dependencies, and there's no feedback that the explicit constraint was ignored. It's strictly an improvement over the pre-PR behavior (which wrongly added a root dep), but it leaves a user-visible gap.Suggested fix
In the
before_installcatalog branch, whenrequest.version.tagisNpm/DistTagorDo::UPDATE_TO_LATESTis set, rewrite the catalog object's literal in place (torequest.version.literalorb"latest") before install — mirroring what thee_stringpath does for root deps. Thencommit_catalog_updatecan usewhich_version_is_pinnedon the new literal (or just respectexact_versions/ the user's prefix). If that's out of scope for this PR, a TODO plus a warning when an explicit version/--latestis supplied for a catalog package would at least avoid the silent surprise. - User runs
|
Good catch on the Added two tests: The multi-catalog-group note is answered in the inline thread (kept to first-match, scoped re-resolution, |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/install/PackageManager/PackageJSONEditor.rs:904-924— When a package exists only in a catalog and the user runsbun update <name>@<version>, the new catalog block matches it (by name only), setsis_catalog=true, and decrementsremaining— butrequest.versionis never consulted, so the explicit@<version>is silently dropped and the command behaves identically tobun update <name>. Pre-PR the explicit version at least landed (in the wrong place — a spurious root dep); now it's silently ignored. Consider guarding the catalog branch withrequest.version.tag == dependency::Tag::Uninitialized, and either writingrequest.version.literalinto the catalog constraint before install or printing a note that explicit versions on catalog packages aren't yet supported (the broader #32064 may already cover this).Extended reasoning...
What the gap is. This PR teaches
bun update <name>to recognise catalog-only packages and update the catalog entry in place. But the new catalog block atPackageJSONEditor.rs:904-924matches the request purely by package name viafind_package_catalogand never inspectsrequest.version. So when the user supplies an explicit version spec —bun update baz@2.0.0— that spec is silently discarded: the request is markedis_catalog, the append-new-dependency block skips it (request.e_string.is_some() || request.is_catalogat line 1002), thee_stringrewrite loop never runs (e_stringisNone), andcommit_catalog_updateonly reads the lockfile resolution that was constrained by the unchanged catalog literal. The net effect is thatbun update baz@2.0.0on a catalog-only package behaves identically tobun update baz.The specific code path. For input
baz@2.0.0,UpdateRequest::parseproducesis_aliased=true,name=b"baz",version.tag=Npmwithversion.literal="2.0.0".get_name()returnsb"baz"(becauseis_aliased). The dependency-group scan at the top ofedit()findsbazin none of the four root groups (it's catalog-only), sorequest.e_stringstaysNoneand the innerrequest.version.tag == Npmrewrite arm — which would have written2.0.0into a root-dep slot — never fires. Then the newSubcommand::Updateblock runs:e_string.is_some()is false,is_catalogis false,find_package_catalog(pkg_json, b"baz")matches the catalog entry, sois_catalog=true,catalog_nameis recorded, andremaining -= 1.request.versionis never read.Why nothing downstream catches it. Install proceeds with the catalog literal still at its old value (e.g.
~0.0.3);is_named_catalog_updateinPackageManagerEnqueue.rsallows re-resolution, but only within~0.0.3, not to2.0.0. After install,commit_catalog_updatecallsresolved_catalog_version(reads the lockfile resolution) andwhich_version_is_pinned(original_literal)(reads the old catalog literal) —request.versionis not a parameter. So the user's2.0.0never reaches any sink. Compare with a root-dep package, where the samebun update foo@2.0.0does honour the explicit version via the'uninitializedarm of thee_stringrewrite loop.Step-by-step proof. Take the PR's own test fixture (
catalogs: { ai: { baz: "~0.0.3" } }, registry has 0.0.3 and 0.0.5) and runbun update baz@0.1.0instead ofbun update baz:UpdateRequest::parse("baz@0.1.0")→is_aliased=true,name="baz",version.tag=Npm,version.literal="0.1.0".- Dep-group scan:
baznot independencies/devDependencies/optionalDependencies/peerDependencies→e_string=None. - Catalog block:
find_package_catalog(pkg_json, "baz")→Some("ai").is_catalog=true,remaining=0.request.versionuntouched. - Append block:
is_catalog→ skip.e_stringrewrite loop:e_string=None→ skip. - Install resolves
catalog:ai(=~0.0.3) →baz@0.0.5. commit_catalog_update(name="baz", catalog_name="ai")reads lockfile →0.0.5, reads old literal~0.0.3→ pinnedMinor→ writes~0.0.5.- Result:
catalogs.ai.baz = "~0.0.5". The user's0.1.0appears nowhere; no warning is printed.
Impact. Silently dropping explicit user input is surprising and diverges from how
bun update <name>@<ver>behaves for root deps. That said, this is adjacent to the PR's stated scope (#32808 is about barebun update <name>), pre-PR behaviour was also wrong (it appended a spurious rootdependencies.baz="2.0.0"), and the author has already noted that #32064 covers more catalog-update cases. So this is a nit / follow-up, not a blocker.How to fix. Guard the catalog block so it only claims requests with no explicit spec — e.g. add
&& request.version.tag == dependency::Tag::Uninitializedto thefind_package_catalogcondition. Then either (a) when an explicit version is given, writerequest.version.literalinto the catalog literal before install so resolution honours it (mirroring what the interactive updater'supdate_named_catalog/update_default_catalogdo), or (b) at minimum print anote:that explicit versions on catalog packages aren't yet supported, so the drop isn't silent. Falling through to the old append-root behaviour would just reintroduce #32808 for this input, so that's not the right mitigation.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/install/PackageManager/PackageJSONEditor.rs:936-940— Follow-up:bun update <name>on a catalog-only package now correctly rewrites the catalog entry but prints no per-package summary line — theget_or_put(name)?here inserts a defaultPackageUpdateInfo(emptyoriginal_version_literal,original_version: None), and neitherrecord_updating_package_versionsnorprint_installed_workspace_sectionreaches a catalog dep that isn't a root dependency. So the user sees only the generic "N packages installed" line, whereas the equivalent root-dep update printsinstalled baz@0.0.5. Not blocking (strictly better than the pre-PR spurious line, and the first new test only assertsnot.toContain("installed baz@")), but worth populatingoriginal_version_literalfrom the catalog literal so the summary pipeline can report the bump.Extended reasoning...
What the gap is
For a catalog-only package,
edit()at line 938 callsmanager.updating_packages.get_or_put(name)?and discards the entry handle, so the insertedPackageUpdateInfostays at its default:original_version_literalis empty,is_aliasisfalse, andoriginal_versionisNone. Compare the root-dep scan just above (the'add_packages_to_updateblock), which writes*entry.value_ptr = PackageUpdateInfo { original_version_literal: version_literal_owned, ... }with the dependency's existing literal. This default-valued entry then never gets enriched by the post-install summary pipeline, so no per-package output is produced.Why the summary pipeline can't recover it
There are two paths that would normally emit a per-package line for a named
bun update <pkg>, and a catalog-only dep falls through both:-
record_updating_package_versions(install_with_manager.rs:1449-1454) iterates onlyworkspace_deps— the install workspace's root package's direct dependencies — and skips any whoseversion.tag != Npm && != DistTag. A catalog package consumed only by member workspaces is not a root dep at all (that is the whole reasonis_named_catalog_updatehad to be added in this PR), and where it does appear its tag isCatalog. Both filters exclude it, soentry.original_versionstaysNoneandtree_printer.rs:221'sif let Some(original_version)falls through — no^ baz X -> Yline. -
print_installed_workspace_section(tree_printer.rs:422-443, non-verbose path) is called once forworkspace_package_id = 0(root) withSome(&mut id_map). It iteratesresolutions_list[0], which for the test fixture contains only the workspace memberserver, notbaz.should_print_package_installchecks each root dep againstupdate.matches(...);server's name-hash ≠ hash(baz), so nothing matches andid_map[0]staysINVALID_PACKAGE_ID. The later loop attree_printer.rs:495-498then hitsif dependency_id == INVALID_PACKAGE_ID { continue; }— noinstalled baz@0.0.5line.
Net: the catalog entry is correctly rewritten from
~0.0.3to~0.0.5, but stdout shows only the genericN packages installed [Xms]summary, with nothing namingbazor its version.Step-by-step proof
Using the first new test fixture:
// root: { workspaces: ["packages/*"], catalogs: { ai: { baz: "~0.0.3" } } } // packages/server: { dependencies: { baz: "catalog:ai" } }
Run
bun update bazfrom the root:edit(before_install=true): root has nodependencies/devDependencies/etc., so the dep-group scan finds nothing andrequest.e_stringstaysNone. The new catalog block callsfind_package_catalog→Some(b"ai"), setsrequest.is_catalog = true, and callsmanager.updating_packages.get_or_put(b"baz")?. The entry is inserted withPackageUpdateInfo::default()—original_version_literal = b"",original_version = None.- Install runs.
record_updating_package_versionsiterates root's deps (just theserverworkspace dep, tagWorkspace) → filtered at the!= Npm && != DistTagcheck.updating_packages["baz"].original_versionremainsNone. - Tree printer (non-verbose):
print_installed_workspace_section(0, Some(&mut id_map))iterates root's resolutions → onlyserver, which doesn't match thebazrequest →id_map[0] = INVALID_PACKAGE_ID. - The
installed <name>@<ver>loop skips index 0 becauseid_map[0] == INVALID_PACKAGE_ID. The^ X -> Yupgrade line is gated onSome(original_version), which isNone. - Result: no line mentioning
bazis printed. The catalog inpackage.jsonis correctly bumped to~0.0.5.
By contrast, the existing test in this same file (
should update to latest version of dependency) shows that a root-depbun update bazprintsinstalled baz@0.0.5 with binaries:— so the catalog path is the only named-update flow with no per-package feedback.Test evidence
The first new test captures stdout but only asserts the negative —
expect(out).not.toContain("installed baz@")— confirming the old spurious line is gone without asserting any replacement. The other four catalog tests usestdout: "ignore"entirely. So the silent-success behavior is exercised but not pinned down either way.Impact and severity
This is a UX/output gap, not a correctness bug — the catalog is updated correctly, the lockfile is re-resolved, and the workspace references stay intact. It is also strictly better than pre-PR, which printed a misleading
installed baz@<latest>for a spurious root dependency it had just synthesized. So: nit, fine as a follow-up.Suggested fix
When the catalog branch inserts into
updating_packages, populate the entry from the catalog literal so the existing summary machinery has something to work with, mirroring the root-dep path:if let Some(catalog_name) = find_package_catalog(current_package_json, name) { if options.before_install { let entry = manager.updating_packages.get_or_put(name)?; if !entry.found_existing { if let Some(obj) = catalog_object_with_key(current_package_json, &catalog_name, name) { if let Some(s) = obj.get(name).and_then(|e| e.data.e_string()) { *entry.value_ptr = PackageUpdateInfo { original_version_literal: Box::from(s.data.slice()), ..Default::default() }; } } } } ... }
That alone won't make the line print (the tree printer still only walks root deps), but it brings the catalog path to parity with the root-dep path's bookkeeping and is the prerequisite for whichever summary fix lands — likely as part of the planned helper-hoisting that consolidates this with the interactive updater.
-
|
Re the two latest notes:
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/PackageManager/PackageJSONEditor.rs:945-962— When a package appears in both a root dependency group and acatalog/catalogsmap, this block hijacks the request on the secondedit()pass and stops the root dep from being rewritten. The dep-group scan takes thereplacing += 1branch (which deliberately leavese_stringunset andremainingundecremented, relying on theif remaining != 0block below to do the in-place rewrite), but the catalog block then seese_string.is_none()and!is_catalog, claims the request, and decrementsremainingto 0 — sodependencies.bazis left at its old literal whilecatalogs.*.bazis bumped (a regression vs. pre-PR, which rewrote the root dep). Also skip this block whenrequest.package_id != INVALID_PACKAGE_ID(the same discriminant thereplacingbranch keys on).Extended reasoning...
What the bug is
edit()is called twice with the sameupdatesslice — once withbefore_install=true(updatePackageJSONAndInstall.rs:310) and once withbefore_install=false(:569). Between the two passes,clean_with_loggerresetsupdate.e_string = None(lockfile.rs:918) and, because the package is a root workspace dependency, setsupdate.package_idto the resolved id (lockfile.rs:1254-1264). The new catalog block guards only onrequest.e_string.is_some(), which is insufficient: on the second pass, the dep-group scan above takes thereplacing += 1branch (becausepackage_id != INVALID && list == dependency_list), and that branch intentionally leavese_stringunset andremainingundecremented — it defers the actual rewrite to theif remaining != 0block. The catalog block then claims the request, setsis_catalog = true, and decrementsremainingto 0, starving the in-place-replacement block.Step-by-step proof
Given:
// root package.json { "workspaces": ["packages/*"], "dependencies": { "baz": "^1.0.0" }, // root uses baz directly "catalogs": { "ai": { "baz": "^1.0.0" } } // workspaces use it via catalog:ai }
Run
bun update bazafterbaz@1.5.0is published.Pass 1 (
before_install=true,package_id == INVALID): the dep-group scan findsbazindependenciesand takes the else branch → setse_string,remaining -= 1. The catalog block'se_string.is_some()guard skips it, sois_catalogstaysfalse.Between passes: lockfile.rs:918 sets
e_string = None; lockfile.rs:1264 setspackage_idto the resolved id (baz matches a workspace-root dep).is_catalogis untouched (stillfalse).Pass 2 (
before_install=false): the dep-group scan findsbazindependencies;package_id != INVALID && list == dependency_list→replacing += 1, does not sete_string, does not decrementremaining. Now the catalog block:e_string.is_some()→ false,!is_catalog→ true,find_package_catalog(..., b"baz")finds it incatalogs.ai→is_catalog = true,catalog_name = b"ai",remaining -= 1→remaining == 0. Theif remaining != 0block — which pre-PR re-found the entry innew_dependencies, sete_string, and let the final write-back loop write the resolved version — is skipped. The finalfor request ... if let Some(e_string)loop does nothing for this request.commit_catalog_updatethen rewrites onlycatalogs.ai.baz.Why the existing guards don't catch it
The catalog block uses
e_string.is_some()as its "already handled by the dep-group scan" signal. That works for the else branch of the dep-group scan (which setse_string), but not for thereplacingbranch, which by design leavese_string = Noneuntil theif remaining != 0block runs. The!request.is_catalogguard doesn't help either:is_catalogwas never set in pass 1 (because pass 1 sete_stringand skipped the catalog block), and nothing between the passes sets it.Impact
Pre-PR,
bun update bazon this configuration rewrotedependencies.bazto the resolved version (and didn't touch the catalog). Post-PR,dependencies.bazis silently left at its old literal whilecatalogs.ai.bazis bumped and the lockfile/node_modules are re-resolved (since baz is a root dep,should_updatewas true). The rootpackage.jsonliteral now diverges from both the lockfile and the catalog entry the same command just updated. This is an edge-case configuration (same package in both a root dep group and a catalog) but a legitimate one, and it's a behavioral regression introduced by this PR.Suggested fix
Also skip catalog detection when
request.package_id != INVALID_PACKAGE_ID— that's exactly the discriminant thereplacingbranch keys on, so it cleanly identifies "the dep-group scan already matched this request":for request in updates.iter_mut() { if request.e_string.is_some() || request.package_id != INVALID_PACKAGE_ID { continue; } ... }
Alternatively, gate the whole
find_package_catalogcall onoptions.before_installand rely on the persistedis_catalogflag in pass 2 (the block already doesif !request.is_catalog { find... }, so detection only ever needs to happen once).
|
Both addressed:
|
|
CI status: the diff is green on the relevant lanes. The one hard failure is unrelated to this change:
|
…36304) Fixes #23739 Fixes #21852 ## Problem In a workspace using a catalog, non-interactive `bun update` never touched the `catalog`/`catalogs` entries in the root package.json: ```jsonc // root package.json { "workspaces": { "packages": ["packages/*"], "catalog": { "lodash": "4.17.0" } } } // packages/app/package.json { "dependencies": { "lodash": "catalog:" } } ``` ```console $ bun update -r --latest # "no changes" — catalog.lodash still 4.17.0 ``` Worse, running `bun update --latest` from inside a workspace package rewrote the `catalog:` reference to `^<version>`, silently detaching it from the catalog. Interactive mode (`bun update -r -i`) already handled both correctly. **Root cause:** `edit_update_no_args` only iterates the four dependency groups of the current package.json; it never looks at `catalog`/`catalogs`. When it does encounter a `catalog:` value (running inside a workspace), it treats it like an npm dependency, registers it in `updating_packages`, and writes the resolved range back over the `catalog:` literal. Nothing ever edits the root catalog objects. ## Fix In `PackageJSONEditor.rs`: * `catalog:`-tagged dependencies are excluded from the value-rewrite paths (`edit_update_no_args` before/after install, and the `bun update <pkg>` path in `edit`). * New `edit_catalogs_before_update` / `edit_catalogs_after_update` walk the root `catalog`/`catalogs` objects (under `workspaces` or at the top level, matching `CatalogMap::parse_append`). With `--latest` the entries are set to a temporary `latest` in the cached root package.json so the resolver fetches the newest version through the existing `catalog:` resolution path; after install the resolved version is written back preserving the `^`/`~`/exact pin. Without `--latest`, ranges move within range and exact pins stay, matching direct-dependency behavior. `updatePackageJSONAndInstall.rs` calls these around the install and handles both run-from-root and run-from-inside-a-workspace (the root package.json is a different file in the second case and is written separately), honoring `--dry-run`/`--no-save`. Identity is `(catalog name, dependency name)`, so the same package in multiple catalogs updates independently; entries not referenced by any workspace stay untouched. `bun add <pkg>` still replaces a `catalog:` reference (adding is an explicit request to pin a version in that package). `bun update <pkg>` on a `catalog:` reference keeps the reference and re-resolves within the catalog range; bumping the catalog entry from a named update is #32808 / #32810 territory. This PR adopts #32064 (thank you @CarlosZiegler) on current main with a small test addition for the `-r` flag from #23739. Also includes a harness fix: bind the verdaccio test registry to `127.0.0.1` explicitly. A bare port makes verdaccio listen on whatever `localhost` resolves to (`::1` on hosts that list it first) while the install client connects to `127.0.0.1`, refusing every request. ## Tests `test/cli/install/catalogs.test.ts` (new `describe("update")`, verdaccio registry): - `--latest` updates default + named catalogs, top-level and `workspaces.*` locations, with and without `-r` - run from inside a workspace package updates the root catalog and keeps `catalog:` refs - same package in default + named catalogs updates independently; unreferenced entries unchanged - plain `bun update` stays in range, does not move exact pins, keeps refs - `bun update <pkg> --latest` keeps the catalog reference - `--dry-run` writes nothing Fail-before with src/ reverted: 7/8 new tests fail (catalog unchanged, `catalog:` overwritten with `^2.0.0`). With the fix: 14/14 pass (including the four pre-existing catalog tests). `bun-update.test.ts` 6/6, `bun-add.test.ts` 54/54, `bun-install-registry.test.ts -t update` 16 pass + 1 todo. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 2 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/install/catalogs.test.ts bun test v1.4.0 (75bcb99) test/cli/install/catalogs.test.ts: (pass) basic > both catalog and catalogs in top-level [587.35ms] (pass) basic > both catalog and catalogs in workspaces [457.07ms] (pass) basic > detect changes (bun.lockb) [803.98ms] (pass) basic > detect changes (bun.lock) [881.35ms] 241 | expect(err).not.toContain("error:"); 242 | 243 | // catalog entries are updated, preserving the pinning style 244 | const root = await file(join(packageDir, "package.json")).json(); 245 | const { catalog, catalogs } = isTopLevel ? root : root.workspaces; 246 | expect(catalog).toEqual({ "no-deps": "^2.0.0" }); ^ error: expect(received).toEqual(expected) { - "no-deps": "^2.0.0", + "no-deps": "^1.0.0", } - Expected - 1 + Received + 1 at <anonymous> (/workspace/bun/test/cli/install/catalogs.test.ts:246:23) (fail) update > --latest updates catalog versions in top-level [584.67ms] 241 | expect(err).not.toContain("error:"); 24 ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (3bfb037) test/cli/install/catalogs.test.ts: (pass) basic > both catalog and catalogs in top-level [70.22ms] (pass) basic > both catalog and catalogs in workspaces [27.96ms] (pass) basic > detect changes (bun.lockb) [45.77ms] (pass) basic > detect changes (bun.lock) [43.93ms] (pass) update > --latest updates catalog versions in top-level [50.49ms] (pass) update > --latest updates catalog versions in workspaces [41.35ms] (pass) update > --latest updates catalog versions with -r [43.70ms] (pass) update > --frozen-lockfile passes after --latest updates catalogs [44.07ms] (pass) update > --latest run from inside a workspace package updates the root catalog [38.69ms] (pass) update > --latest updates the same package independently per catalog [34.68ms] (pass) update > --latest --dry-run does not modify any package.json (from root) [34.78ms] (pass) update > --latest --dry-run does not modify any package.json (from workspace) [58.05ms] (pass) update > update without --latest stays in range and keeps catalog references [39.07ms] (pass) update > update <pkg> --latest keeps the catalog reference [56.67ms] (pass) errors > fails gracefully when no cat ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/install/catalogs.test.ts bun test v1.4.0 (75bcb99) test/cli/install/catalogs.test.ts: (pass) basic > both catalog and catalogs in top-level [433.37ms] (pass) basic > both catalog and catalogs in workspaces [331.82ms] (pass) basic > detect changes (bun.lockb) [537.51ms] (pass) basic > detect changes (bun.lock) [552.37ms] (pass) update > --latest updates catalog versions in top-level [445.45ms] (pass) update > --latest updates catalog versions in workspaces [399.59ms] (pass) update > --latest updates catalog versions with -r [404.73ms] (pass) update > --frozen-lockfile passes after --latest updates catalogs [558.91ms] (pass) update > --latest run from inside a workspace package updates the root catalog [386.68ms] (pass) update > --latest updates the same package independently per catalog [411.74ms] (pass) update > --latest --dry-run does not modify any package.json (from root) [379.27ms] (pass) update > --latest --dry-run does not modify any package.json (from workspace) [396.85ms] (pass) update > update without --latest stay ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 695ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/7] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [2/7] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited [2/7] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/install/PackageManager.rs | 13 + src/install/PackageManager/PackageJSONEditor.rs | 336 ++++++++++++++++++++- .../PackageManager/updatePackageJSONAndInstall.rs | 128 ++++++++ test/cli/install/catalogs.test.ts | 278 +++++++++++++++++ test/harness.ts | 6 +- 5 files changed, 756 insertions(+), 5 deletions(-) ``` </details> **gate history** · 6 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/install/PackageManager.rs 2 1 0 src/install/PackageManager/PackageJSONEditor.rs 9 8 0 …c/install/PackageManager/updatePackageJSONAndInstall.rs 8 7 0 test/cli/install/catalogs.test.ts 3 5 0 test/harness.ts 2 1 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Carlos Ricardo Ziegler <38855507+CarlosZiegler@users.noreply.github.com>
|
@robobun rebase m. Update. Make it good |
|
On it. Main has since landed the non-interactive catalog-update machinery (#36304, #36379, #36360), so I'm rebasing onto that: the named catalog-only path will reuse |
9cdaa09 to
b45c0b7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
b45c0b7 to
46222cf
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/install/PackageManager/PackageJSONEditor.rs`:
- Around line 1110-1122: Update the Update handling around the mutable requests
loop so matching direct `catalog:` references are classified as catalog targets
even when request.e_string is already set. Preserve precedence for direct
non-catalog dependencies, and apply the same classification to root and
workspace packages using the existing catalog lookup symbols. Add regression
coverage for both package scopes verifying bun update changes the catalog
version.
🪄 Autofix
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: 7ddbdd7a-8d4c-4664-9a67-7e51746aef58
📒 Files selected for processing (4)
src/install/PackageManager/PackageJSONEditor.rssrc/install/PackageManager/UpdateRequest.rssrc/install/PackageManager/updatePackageJSONAndInstall.rstest/cli/install/catalogs.test.ts
…dding a root dependency `bun update <name>` for a package defined in a `catalog`/`catalogs` map did not update the catalog definition. A catalog-only target (consumed by workspaces via `catalog:`) got a brand-new root `dependencies.<name>` appended at the latest version, bypassing the catalog and its constraint; a target referenced as `catalog:` in a dependency group kept the reference but the definition never moved. `PackageJSONEditor::edit` only scanned the four root dependency groups, so a catalog-only package matched none of them and the append-new-dependency tail synthesized a root entry that resolved to `latest`. The non-interactive catalog machinery only ran for bare `bun update`. Route named catalog targets through that machinery instead: `edit` classifies both catalog-only targets (no root dependency appended) and kept `catalog:` references (`UpdateRequest.is_catalog`), and `edit_catalogs_before_update` gains a name filter so only the targeted entries are recorded (and, with `--latest`, temporarily rewritten for resolution). The post-install `edit_catalogs_after_update` call now also runs on the named path, writing resolved versions back into the catalog definition with the original pin style preserved and `npm:` aliases kept. `preprocess_update_requests` no longer rewrites a `catalog:` dependency's lockfile literal to a resolved range, which both corrupted the lockfile reference and hid the dependency from the catalog write-back. A name defined in several catalog groups updates every group, matching the interactive updater; a name also present as a plain root dependency keeps its root-dependency behavior and the catalog entry is left alone. Fixes #32808
46222cf to
2cc7499
Compare
|
CI note: the last four builds (93540, 93541, 93548, 93549) each failed only in build lanes downloading vendored dependency tarballs from github.com (mimalloc, cares, lolhtml, WebKit; HTTP 503 / fetch failed after 5 retries), across FreeBSD, linux-musl, linux-android, and darwin agents. Nothing in the diff is implicated; no test lane has failed. Locally green on the rebased commit: catalogs.test.ts 26/26 (including the 9 named-update catalog tests), bun-update.test.ts 38/38, bun-add.test.ts 54/54. A rebuild once GitHub recovers should go green. |
|
Build 93549 finished: 174 passed, 3 failed, none related to this diff. The darwin aarch64 build lane hit the same GitHub tarball outage (mimalloc download). The two red test lanes are grpc-js/test-tonic.test.ts (debian x64) and install/migration/complex-workspace.test.ts (debian x64-asan); both are untouched by this change (third-party grpc; lockfile migration, which never runs the update path), appear in the flaky annotation on other lanes, and have been reported to main-break triage. catalogs.test.ts and bun-update.test.ts passed on every lane that ran them. |
|
Closing: #38333 has landed on main since this was opened and removes the bug this PR targeted (#32808). Verified with the issue's repro on 7d276b9: Main also pins a different behavior from this PR for named updates of catalog packages: only a bare |
2 participants
Fixes #32808
Problem
In a workspace monorepo, a package defined only in a
catalog/catalogsmap (and consumed by workspaces viacatalog:<name>) could not be updated by name.bun update <name>ignored the catalog and appended a brand-new rootdependencies.<name>at the latest version, bypassing both the catalog definition and its semver constraint.bun update bazleftcatalogs.ai.bazuntouched and added a spurious"dependencies": { "baz": "^<latest>" }to the root, so the workspacecatalog:aireference diverged from the new root dependency.Cause
PackageJSONEditor::edit(the named-update path) only scans the four root dependency groups. A catalog-only package matches none of them, so the append-new-dependency tail synthesizes a root entry that resolves tolatest. The non-interactive catalog machinery (#36304, #36379, #36360) only runs for barebun update(update_requests.is_empty()).Fix
Route named catalog-only targets through that existing machinery instead of synthesizing a root dependency:
editclassifies a named target found in no root dependency group but defined in acatalog/catalogsmap (UpdateRequest.is_catalog), so the append block leaves it alone. Classification happens only on the before-install pass; a name matched in a root dependency group keeps its root-dependency behavior and the catalog entry is left alone.edit_catalogs_before_updategains anames_filter: barebun updatestill records every entry (None); a named update records only the targeted entries, and with--latesttemporarily rewrites just those for resolution. A name defined in several catalog groups updates every group, matchingbun update -i.edit_catalogs_after_updatecall now runs on the named path too (hoisted out of the no-args branch), writing resolved versions back into the catalog definition with the original pin style preserved,npm:aliases kept, and unresolved (unconsumed) entries restored.Resolution-side re-targeting needs no changes: #36360's
UpdateRequest::contains_namealready re-resolves every named-update target, includingcatalog:dependencies.After the fix,
bun update bazupdatescatalogs.ai.bazin place (in range without--latest, past it with--latest), adds no root dependency, and leavescatalog:aireferences intact.Background
A catalog is a version map in the workspace root package.json (
catalogfor the default group,catalogs.<group>for named ones, optionally nested underworkspaces). Member workspaces reference entries with thecatalog:/catalog:<group>protocol so one constraint is shared across the monorepo. The catalog definition is the single source of truth, which is why an update must edit it in place rather than materialize a root dependency.Verification
Six tests added to the
updatesection oftest/cli/install/catalogs.test.ts, alongside the existing non-interactive catalog-update coverage:--latestpast the range, pin style preserved (^1.0.0to^2.0.0)--latestwith the name in several groups: every consumed group updates per its own pin style; an unconsumed entry is restorednpm:aliased catalog entry: alias preserved (npm:no-deps@^1.0.0tonpm:no-deps@^1.1.0)All fail on main (the spurious root dependency appears) and pass with this change. Full
catalogs.test.ts(24),bun-update.test.ts(38), andbun-add.test.ts(54) pass locally.