install: re-resolve catalog references on plain bun update from the workspace root - #36379
Conversation
… workspace root `bun update` (no --latest) gated re-resolution on `is_root_dependency`, which only covers direct dependencies of the package in cwd. A `catalog:` reference is declared in a workspace package, so running from the root left it deduped onto the locked version even when a newer in-range version was published; only `--latest` (which rewrites the catalog text and trips `catalogs_changed`) re-resolved it. Catalog definitions live in the root package.json, so treat a catalog reference as eligible for re-resolution regardless of which workspace the update runs in.
WalkthroughChangesCatalog dependency updates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the fix is a narrow, well-tested widening of should_update for catalog: refs. Deferring to a human because this touches package-manager resolution semantics (catalog refs now bypass the cwd-scoped is_root_dependency gate on every bun update, not just from the root).
What was reviewed:
- Confirmed
dependency.version.tagis stillCatalogat this callsite (the resolved catalog version is passed separately asversion), so the new predicate identifies the right deps. - Checked the change is gated by
to_updateand the existingupdate_requests/updating_packagesfilter, sobun installand targetedbun update <pkg>behavior are unchanged. - Test uses the Verdaccio harness, synthesizes a lockfile pinned to 1.0.0 under
^1.0.0, and asserts the result stays in-range at 1.1.0 (not 2.0.0) — fail-before is documented.
Extended reasoning...
Overview
The PR adds one disjunct to the should_update computation in get_or_put_resolved_package_with_find_result (PackageManagerEnqueue.rs): a dependency whose version.tag == Catalog is now eligible for re-resolution during bun update regardless of whether is_root_dependency holds. It adds two parametrized tests in catalogs.test.ts that pin no-deps@1.0.0 in a synthetic lockfile under a ^1.0.0 catalog and assert bun update / bun update -r from the workspace root move it to 1.1.0.
Security risks
None. This is version-resolution bookkeeping inside the package manager; no new inputs are parsed and no trust boundaries change.
Level of scrutiny
Medium-high. The diff is tiny (3 effective lines of logic) and clearly a follow-up to #36304, but it lives in the resolution path that decides which package version gets installed. The predicate is checked before the Catalog tag is replaced with the looked-up version (verified against the 'version block around PackageManagerEnqueue.rs:728), so the tag test is sound. It short-circuits before the is_root_dependency unsafe split, which is fine. It remains gated by this.to_update and by the update_requests.is_empty() || updating_packages.contains(name) filter, so bun install and bun update <specific> are unaffected.
Other factors
The one semantic question worth a human eye: the new disjunct makes every catalog reference re-resolve on bun update, not just those in the cwd package. That's the intent (catalogs are root-scoped), and it mirrors how edit_catalogs_after_update already writes back to the root package.json regardless of cwd — but it is a slight broadening beyond the stated bug (root cwd only). The PR reports the surrounding suites (bun-update, bun-workspaces, bun-add, bun-lock, bun-install-registry -t update) all pass, and the new tests follow harness conventions (VerdaccioRegistry, runUpdate helper, exitCode asserted last, no network). Given the resolution-path stakes I'm deferring rather than approving.
|
CI: |
|
Updated 6:41 PM PT - Jul 29th, 2026
❌ @robobun, your commit 02dc295 has 1 failures in
The baseline build crashed or failed tests under 🧪 To try this PR locally: bunx bun-pr 36379That installs a local version of the PR into your bun-36379 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/cli/install/catalogs.test.ts`:
- Around line 431-448: In the runUpdate assertion block, add the exitCode
success check immediately after verifying stderr does not contain an error and
before reading package.json or bun.lock. Keep the existing artifact assertions
unchanged, ensuring command failures are reported before any generated-file
reads occur.
🪄 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: 6551b0bb-c91d-4475-80b0-9a67d36c4499
📒 Files selected for processing (2)
src/install/PackageManager/PackageManagerEnqueue.rstest/cli/install/catalogs.test.ts
There was a problem hiding this comment.
I didn't find bugs, but this changes the install resolver's should_update gate — worth a human confirming the semantic widening is what's wanted.
What was reviewed:
- Traced
dependency.version.tagat both call sites ofget_or_put_resolved_package_with_find_result— it retainsCatalogafter the catalog→npm substitution (the resolved version is the separateversionparam), so the new predicate fires where intended. - Checked the
update_requestsnon-empty branch: catalog refs now pass the cwd gate there too, still bounded byupdating_packages.contains(name). - The predicate makes catalog refs re-resolve from any cwd, not only root — the PR says this is intentional and matches #36304's
edit_catalogs_after_update; noting it since the tests only exercise the from-root direction.
Extended reasoning...
Overview
One-condition change to the should_update boolean in get_or_put_resolved_package_with_find_result (src/install/PackageManager/PackageManagerEnqueue.rs): a dependency whose original tag is Catalog now bypasses the is_root_dependency(dependency_id) cwd check during bun update. Two new tests in test/cli/install/catalogs.test.ts seed a lockfile pinned below the catalog range's max and assert bun update / bun update -r from the workspace root move the catalog, node_modules, and lockfile forward.
Security risks
None. No untrusted input parsing, no allocation, no unsafe changes — the added clause reads an enum tag on an already-borrowed &Dependency. The existing unsafe raw-pointer split is unchanged.
Level of scrutiny
Medium-high. The diff is tiny and mechanically obvious, but it sits in the package manager's re-resolution gate, which every bun update hits. I verified at both call sites (lines ~1082 and ~2510) that dependency is the original lockfile Dependency (tag still Catalog) while the resolved npm version is passed separately as version, so the new check reads the right field. The gate is still ANDed with this.to_update, so bun install is unaffected.
Other factors
- The predicate is symmetric: it also causes catalog refs in other workspace packages to re-resolve when you run
bun updatefrom inside one workspace. The PR description argues this is correct (catalogs are root-scoped, andedit_catalogs_after_updatefrom #36304 already rewrites them regardless of cwd), but the new tests only cover from-root; a human should confirm that widening is intended. - The comment-cop bot flagged the added 2-line comment; I read it as a false positive (it states an invariant, not a workaround justification), but that's the author's/maintainer's call.
- Fail-before/pass-after evidence is solid, and the adjacent update/workspace/add/lock suites were run green.
There was a problem hiding this comment.
LGTM — targeted follow-up to #36304 that widens the should_update gate to include catalog: references, with fail-before/pass-after tests.
What was reviewed:
- The
should_updatepredicate change: catalog check short-circuits beforeis_root_dependency, usesdependency.version.tag(the originalcatalog:tag, not the resolvedversionparam) consistent with the same check at line 728. - Confirmed the third clause (
update_requests.is_empty() || updating_packages.contains(name)) still gates targeted updates, sobun update <pkg>from root won't re-resolve unrelated catalog entries. - New test writes a pre-pinned lockfile and asserts catalog rewrite, installed version, workspace
catalog:reference preserved, and lockfile contents — matches the file's existing patterns; both bot threads (comment-cop, coderabbit exit-code ordering) are resolved.
Extended reasoning...
Overview
Two files: a 4-line predicate change in src/install/PackageManager/PackageManagerEnqueue.rs (get_or_put_resolved_package_with_find_result) and two new parametrized tests in test/cli/install/catalogs.test.ts. The predicate previously required is_root_dependency(dependency_id) — true only for direct deps of the package in cwd — so a catalog: reference declared in a workspace package failed the gate when bun update was run from the workspace root. The fix ORs in dependency.version.tag == Tag::Catalog, treating catalog references as always eligible (catalogs are defined at the root and shared across workspaces).
Security risks
None. No untrusted-input parsing, no new I/O paths, no allocation or FFI changes. The change only widens a boolean predicate governing whether get_package_id receives None (re-resolve) vs Some(version) (dedupe onto locked) during bun update.
Level of scrutiny
Moderate — this is core package-manager resolution, but the change is narrow: it only takes effect when this.to_update is already true (i.e., during bun update), and only for dependencies whose original tag is Catalog. I checked that dependency.version.tag is the correct field (vs the version parameter, which is the post-catalog-resolution npm range) by comparing against the identical check at PackageManagerEnqueue.rs:728. The catalog check is placed before the unsafe is_root_dependency call, which short-circuits cleanly.
Other factors
- Fail-before is demonstrated on release canary in the PR body (the two new tests fail with
expected ^1.1.0, received ^1.0.0). The ASAN-without-fix build failure in the evidence block is a build-infra artifact, not a test result. - The PR body reports the wider install/update suite passing: catalogs.test.ts 18/18, bun-update 6/6, bun-workspaces 63/63, bun-add 54/54, bun-lock 13/13, and CI is green on all lanes for catalogs.test.ts per the robobun status.
- Both inline review threads are resolved: the comment-cop paragraph-comment flag was addressed by condensing to a single line in 02dc295; coderabbit's exit-code-ordering suggestion was withdrawn after robobun pointed out the setup-written files can't ENOENT and the file's convention (and test/CLAUDE.md) puts
exitCodelast. - The one behavioral edge I considered — running
bun updatefrom insidepkg1now also marks catalog deps declared only inpkg2asshould_update— is consistent with catalogs being root-scoped (the catalog entry itself lives in the root package.json and is shared), and the 63-test workspace suite passes.
…esolver Address issues found in review of the dependency-resolution rewrite: - Guard against version-conflict dependency cycles (a@1 depending on a@2 depending on a@1) by refusing to re-place a package that already sits in the ancestor chain, matching npm. Previously such cycles nested nodes without bound. - Treat catalog references as update targets so a bare `bun update` moves them too, restoring the rule from #36379 that the rewrite dropped. - Chain a checkout for every dependency waiting on a git clone, each from its own recorded commit, instead of only the clone task's originating dependency. Fixes isolated-linker installs with several git deps on one repository, and drops the now-unused originating-dependency field. - Prefetch a node's manifests when its parent finishes deciding rather than when the cursor reaches the node: the whole frontier fetches in parallel while the parent's slots (including any alias) already exist, so a name a sibling slot satisfies is still never requested. - Skip the resolution passes entirely on an install whose manifest diff produced no work, so an in-sync install pays no resolution cost. - Give peers one conflict policy in every resolution path: a peer whose parent scope holds a different same-kind package binds to it with a warning instead of installing a second copy. This also stops dist-tag peers from warning when the scope already holds the tagged version. - On a failed tarball download in the callback-free task loop, reset the package back to Extract so a resolver still waiting on it re-checks, hits the recorded failure, and fails fast instead of polling an extraction that will never land. Adds a resolution-order test: a fake registry that parks each manifest response and releases them in a controlled order (forward, reversed, and seeded shuffles), asserting the resolved lockfile is byte-identical across every order.
2 participants
Follow-up to #36304.
Problem
bun update/bun update -r(no--latest) is a no-op for catalog entries when run from the workspace root: a catalog entry^1.0.0locked to 1.0.0 stays on 1.0.0 after 1.5.0 is published, while a direct dependency with the identical range moves to 1.5.0. Only--latestbumps catalogs.The only existing non-
--latesttest ran update from inside the workspace package, where this already worked.Cause
get_or_put_resolved_package_with_find_resultgates re-resolution onis_root_dependency(dependency_id), i.e. "is this a direct dependency of the package in cwd". Acatalog:reference lives in a workspace package, so when running from the root it fails that check,should_updateis false, andget_package_iddedupes onto the locked version instead of taking the manifest's best match. With--latestthe catalog text is rewritten tolatestbefore install, which tripscatalogs_changedand clears the resolution, bypassing the dedupe.(Separately,
-ris not read anywhere in the non-interactive resolve path, so it is equivalent to no flag here; that's #36360 / #33182.)Fix
In
should_update, treat acatalog:reference (dependency.version.tag == Catalog) as eligible for re-resolution regardless of cwd. Catalog definitions are a root-level concept shared across workspaces; re-resolving them matches what #36304 already does for the package.json rewrite inedit_catalogs_after_update.Tests
New in
test/cli/install/catalogs.test.ts: writes a lockfile pinningno-deps@1.0.0with a catalog range^1.0.0(the registry has 1.1.0), runsbun update/bun update -rfrom the root, asserts the catalog is rewritten to^1.1.0,node_modules/no-depsis 1.1.0, and the lockfile no longer containsno-deps@1.0.0.Fail-before (release canary):
With fix:
catalogs.test.ts18/18,bun-update.test.ts6/6,bun-install-registry.test.ts -t update16 pass + 1 todo,bun-workspaces.test.ts63/63,bun-add.test.ts54/54,bun-lock.test.ts13/13.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file