Repository navigation
Conversation
When a package is listed in peerDependencies and a lockfile already has an older version that still satisfies the range, `bun update` (and `bun update <pkg>`) would reuse that stale entry instead of picking the newest matching version. Peer dependencies are resolved in a second pass (install_peer=true). The peer-reuse path in get_or_put_resolved_package scans package_index for any existing entry whose resolution satisfies the range and short circuits to it, which is correct for transitive peers but skipped the should_update check that direct dependencies already get via get_or_put_resolved_package_with_find_result. Extract should_update_dependency() and use it to bypass the reuse path for direct peer dependencies that are update targets. Fixes #13509
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 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 (1)
Walkthrough
ChangesPeer dependency update resolution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: closed by the stale sweep, not merged. The branch is out of date and conflicts with main.
Original repro, against the base of this branch: bun bd test test/cli/install/bun-install-registry.test.ts -t 'peer dependency with stale lockfile entry' |
|
Updated 4:05 AM PT - Aug 1st, 2026
❌ @autofix-ci[bot], your commit e3a16b1 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36688That installs a local version of the PR into your bun-36688 --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/bun-install-registry.test.ts`:
- Around line 4970-5011: Extend the existing stale-lockfile test matrix around
the peer dependency update case to cover catalog-backed peerDependencies, while
preserving the current no-argument, named-package, and --latest variants.
Configure the test package and catalog so a-dep resolves through a catalog
entry, then verify bun update refreshes the stale lockfile target and updates
the resulting manifest and installed version consistently with the existing
cases.
🪄 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: d1655cc9-e115-474f-85e9-5b28c939c8e2
📒 Files selected for processing (2)
src/install/PackageManager/PackageManagerEnqueue.rstest/cli/install/bun-install-registry.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found. The extraction of should_update_dependency() is behavior-preserving and the new gate is scoped to bun update only (to_update early-returns false otherwise), but this touches peer resolution ordering in the package manager — worth a human look to confirm skipping the peer-reuse short-circuit for root peers has no unintended effect on multi-version peer scenarios.
Checked: the extracted function is semantically identical to the removed inline block (early-return on !to_update ≡ the old && prefix); the updated SAFETY comment now correctly matches what is_root_dependency actually reads (root_package_id/workspace_name_hash); transitive peers are unaffected since is_root_dependency is false for them; bun install is unaffected via the to_update guard. The comment-cop warnings are firing on a moved pre-existing SAFETY comment, not new justification.
Extended reasoning...
Overview
Two files: src/install/PackageManager/PackageManagerEnqueue.rs extracts the existing inline should_update computation from get_or_put_resolved_package_with_find_result into a standalone should_update_dependency() function, then calls it at one new site — the top of get_or_put_resolved_package — to skip the peer-reuse short-circuit when the peer is a direct bun update target. test/cli/install/bun-install-registry.test.ts adds a 3-variant test loop (no args, named, --latest) that seeds a stale lockfile entry and asserts it re-resolves.
Security risks
None. No untrusted input parsing, no new unsafe — the raw-pointer split for the is_root_dependency borrowck workaround is moved verbatim from the original site, and the SAFETY invariant (disjoint fields from lockfile) still holds per lockfile.rs:859-863.
Level of scrutiny
Medium. The diff itself is mechanically simple (extract + one guard), and the guard is dead code outside bun update because of the if !this.to_update { return false } early exit. But it changes which branch peer dependencies take during resolution, and the package manager's resolution ordering is subtle enough (see the long determinism comment immediately below the extracted call site) that a maintainer who knows the peer-hoisting invariants should confirm that bypassing the package_index reuse for root peers can't produce a different result than the phase-2 descending scan would for a root peer that also has transitive providers.
Other factors
- Tests follow existing patterns in the
updatedescribe block (runBunInstall/runBunUpdate, exact version + package.json assertions), and robobun confirmed fail-before withUSE_SYSTEM_BUN=1. - The two comment-cop bot warnings target the doc comment and the moved borrowck/SAFETY comment — both pre-existing content, not new workaround justification. Not a blocker.
- CodeRabbit's suggestion to add catalog-backed-peer coverage is a reasonable nit but not a correctness concern; the
Tag::Catalogarm was already in the original predicate.
There was a problem hiding this comment.
I didn't find any issues — the extraction of should_update_dependency is byte-identical to the previous inline predicate, and the new gate on the peer-reuse block is narrowly scoped (fires only when to_update and the dep is a root/catalog update target, so bun install and transitive peers are untouched). Deferring to a human because this sits in the package manager's resolution path.
What was reviewed:
- Confirmed the extracted predicate matches the original inline logic exactly, including the
unsafealiasing pattern and short-circuit ordering. - Traced the fallthrough: skipping the peer-reuse block lands in the same npm-manifest →
_with_find_resultpath that non-peer root deps already use underbun update, so no new resolution codepath is introduced. - Checked
is_root_dependencyonly touchesroot_package_id/workspace_name_hash(disjoint fromlockfile), matching the SAFETY comment. - Tests cover all three update forms (no args, named,
--latest), assert bothnode_modulesstate andpackage.jsonrewrite, and use the hermetic registry harness.
Extended reasoning...
Overview
Two files changed. In PackageManagerEnqueue.rs, the pre-existing inline should_update predicate inside get_or_put_resolved_package_with_find_result is extracted into should_update_dependency() and additionally used to gate the peer-reuse short-circuit at the top of get_or_put_resolved_package. In bun-install-registry.test.ts, three parametrized tests are added inside the existing describe("update") block covering bun update, bun update a-dep, and bun update --latest against a lockfile seeded with a stale peer resolution.
Security risks
None. This is dependency-resolution control flow inside bun update; no parsing of untrusted input, no auth/crypto, no filesystem/network surface changes.
Level of scrutiny
Medium-high. The diff itself is small and mechanical — a pure extraction plus one added && !should_update_dependency(...) conjunct — but it lives in the package manager's resolution path, where a subtle regression could affect every bun update. That said, blast radius is tightly bounded: the new guard early-returns false unless this.to_update, so bun install is entirely unaffected, and it further requires the dependency to be root-level (or catalog) and match the update request set, so transitive peer reuse is unchanged. When the guard fires, control falls through to the same manifest lookup → _with_find_result path that non-peer root dependencies already take under bun update, so no new resolution logic is introduced — peers just stop short-circuiting past it.
Other factors
The extraction is verifiably behavior-preserving: same to_update gate, same Catalog-or-root check, same update_requests.is_empty() || updating_packages.contains(name) clause, same unsafe split-borrow pattern. The updated SAFETY comment (root_package_id/workspace_name_hash) matches what is_root_dependency actually reads at src/install/lockfile.rs:854. All prior bot feedback (comment-cop verbosity, CodeRabbit's catalog-peer coverage suggestion) has been addressed and resolved. The PR description states existing update/peer groups plus bun-lock.test.ts -t peer and catalogs.test.ts -t update continue to pass, and fail-before was verified with USE_SYSTEM_BUN=1. Given the location in resolution logic, a maintainer glance is still warranted.
|
Closing this PR because it has been inactive for more than 90 days. |
Fixes #13509.
Repro
bun outdatedsees the newer version butbun updatenever moves it.Cause
Peer dependencies are resolved in a deferred second pass with
install_peer = true.get_or_put_resolved_packagehas a peer-reuse path that scanspackage_indexfor any existing entry whose resolution satisfies the requested range and short-circuits to it. That is correct for transitive peers (they piggyback on whatever sibling already provides the package) but the stale lockfile entry fortypescript@5.5.2is still in the index, satisfies^5.5.2, and wins.The
should_updategate that forces a fresh resolve for update targets already existed inget_or_put_resolved_package_with_find_result, but that runs after the peer-reuse short-circuit, so it was never consulted for peers.Fix
Extract the existing check into
should_update_dependency()and use it to skip the peer-reuse path when the dependency is a direct update target (root-level, and eitherbun updatewas run without args or the name matches an explicit request). Transitive peers are unaffected becauseis_root_dependencyis false for them.Verification
New tests in the
updatedescribe block ofbun-install-registry.test.tsseed the lockfile witha-dep@1.0.1, widen to^1.0.1, then run each ofbun update,bun update a-dep, andbun update --latest. All three now land on1.0.10and rewritepeerDependenciesinpackage.json. The first two fail on main (stay at1.0.1).Existing
updateandpeertest groups inbun-install-registry.test.ts, plusbun-lock.test.ts -t peerandcatalogs.test.ts -t update, continue to pass.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-install-registry.test.ts