install: link wildcard deps to prerelease workspace members in any position - #37264
install: link wildcard deps to prerelease workspace members in any position#37264robobun wants to merge 5 commits into
Conversation
…sition A '*' range on a name that is also a workspace member resolved differently depending on who declared it: declared by another member it linked the workspace, declared by the root it went to the registry, because Package::parse_dependency decided link-vs-override with Group::satisfies alone while get_or_put_resolved_package also accepts is_star for members. Prerelease member versions fail satisfies under npm semver rules, so the two predicates disagreed exactly there. Mirror the resolver's rule in the parse predicate. This also stops the per-install lockfile re-save for member-declared wildcards on prerelease members: parse now produces the same workspace-tagged dependency the bun.lock loader produces, so the loaded and fresh roots compare equal.
|
Warning Review limit reached
Next review available in: 13 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)
WalkthroughThe installer now links wildcard dependencies to prerelease workspace packages. Shared matching logic is used during package resolution and lockfile validation. Tests cover root and member dependencies, repeated installs, frozen lockfiles, and incompatible non-wildcard ranges. ChangesWorkspace prerelease matching
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. The one-line predicate change correctly mirrors get_or_put_resolved_package and the tests are thorough, but since this shifts install resolution behavior for prerelease workspace members and needs to be sequenced with #37248/#37249 on the same arm, a human look would still be worthwhile.
Checked: the is_star() disjunct matches PackageManagerEnqueue.rs:2298 exactly; link_workspace_packages = false still takes the override branch (condition short-circuits before satisfies); workspace_path.unwrap() remains sound since this arm is inside if let Some(workspace_version). Tests cover both declaration positions, lockfile round-trip stability, --frozen-lockfile, and the non-wildcard boundary.
Extended reasoning...
Overview
One-line logic change in src/install/lockfile/Package.rs (parse_dependency, Npm arm): the satisfies check gains || npm_version.is_star(), so a * range accepts a workspace member with a prerelease version at parse time. This aligns the parse-time link-vs-override decision with the resolve-time decision in get_or_put_resolved_package (PackageManagerEnqueue.rs:2292-2298), which already had workspace_path.is_some() && npm_group.is_star(). Three new tests in bun-workspaces.test.ts exercise root-declared and member-declared wildcards on a 3.0.0-beta.1 member shadowing a registry package, plus a boundary guard for ^1.0.0.
Security risks
None. No untrusted input parsing, no filesystem/network changes; this only adjusts which of two existing branches (workspace-link vs registry-override) an npm-tagged dependency takes when the member version is a prerelease and the range is *.
Level of scrutiny
Medium-high. The diff is tiny and mechanically correct — I verified the mirrored predicate at PackageManagerEnqueue.rs:2298 and confirmed the else (override) branch is unchanged for link_workspace_packages = false and for non-wildcard ranges. But this is package-manager resolution: it changes which package gets installed for a real user shape (root "pkg": "*" on a prerelease member previously hit the registry, now links the workspace). That is the correct npm-matching behavior and fixes both the 404 and the perpetual-resave loop, but it is a user-visible behavioral shift.
Other factors
- The PR description explicitly flags coordination with open PRs #37248 (restructures this same arm) and #37249 (gates the loader rewrite on this predicate). Whichever lands second needs the shared hunk reconciled — a human should confirm the intended landing order.
- An automated
comment-copflag on the new 4-line comment is unaddressed. The comment is arguably fine (it names the mirrored function, which is the non-obvious part), but it's an outstanding bot review. - Tests are strong: they use the Verdaccio harness, assert lockfile contents and byte-stability across repeat installs, exercise
--frozen-lockfile, and include a negative boundary guard. The PR description reports the full install test matrix green locally.
… resolve Extract npm_range_accepts_workspace_member so Package::parse_dependency and get_or_put_resolved_package call one predicate instead of keeping two hand-copied expressions in sync; the drift between them was this bug.
0632b98 to
df33bcb
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes package-manager resolution behavior (wildcard-vs-registry for prerelease workspace members) and the description states it must land after #37248, a human look would still be worthwhile to confirm the landing order and the behavior change.
What was reviewed:
get_or_put_resolved_packagecall site — the extracted predicate is byte-equivalent to the old inline expression, so resolver behavior is unchanged.Package::parse_dependencyNpm arm — passinghas_workspace_path: trueis sound because the enclosingif let Some(workspace_version)block already unwrapsworkspace_pathunconditionally.- New tests follow harness conventions (per-test
setupTest, Verdaccio, drain-then-exit assertions) and include a boundary guard for the non-wildcard override path.
Extended reasoning...
Overview
The PR extracts a shared predicate dependency::npm_range_accepts_workspace_member (satisfies || (has_path && is_star())) and calls it from both get_or_put_resolved_package in PackageManagerEnqueue.rs and the Npm arm of Package::parse_dependency in lockfile/Package.rs. The resolver call site is a pure refactor; the parse site gains the wildcard disjunct, so a root-declared * on a prerelease-versioned workspace member now links the workspace instead of falling through to the registry-override branch. Three tests are added to bun-workspaces.test.ts.
Security risks
None identified. The change is a narrow adjustment to which of two existing branches (link-workspace vs override-with-registry) an npm-tagged dependency takes when its name matches a workspace member. No untrusted input handling, path resolution, or network code changes.
Level of scrutiny
High. bun install resolution semantics are user-visible and load-bearing; a subtle change here can silently alter what gets installed in real monorepos. The change itself is small and well-argued, but it is a deliberate behavior change (previously, a root * dep on a prerelease member fetched from the registry; now it links the workspace), not a mechanical fix. The PR also carries an explicit landing-order constraint ("must land after #37248") because mixed root/sibling range declarations can otherwise produce a lockfile re-save loop — that coordination is a maintainer decision.
Other factors
The extracted predicate is verified equivalent to the resolver's prior inline expression (workspace_version.is_some_and(|v| range.satisfies(v, ..)) || (has_workspace_path && range.is_star())). At the parse site, has_workspace_path is hard-coded true, which is safe because the surrounding block already unwrap()s workspace_path on the accept branch. The versionless-member case is untouched (parse's Npm arm still requires Some(workspace_version)). Tests cover both root- and member-declared shapes plus a negative guard, use the Verdaccio harness and test.concurrent, and assert lockfile stability across repeat installs and --frozen-lockfile. The comment-cop bot flags on the doc comment have been resolved by the author. Given the behavior change and the cross-PR landing dependency, deferring to a human reviewer.
|
CI status: the install suites and all test lanes for this diff are green on build 90974 (194/196 jobs). The two red jobs are unrelated: the darwin 26 aarch64 lane failed during runner provisioning (SSH to the tart VM rejected before any test ran, marked pre-existing), and the debian x64-asan lane has a Blob/Store leak in test/js/web/workers/worker-terminate-funnels.test.ts, reported separately as a main break. No install test failed on any lane (bun-install-registry flaked once on windows aarch64 and passed on retry). |
|
One more call site for |
1 participant
Repro
Same member, same range, opposite outcomes depending on which package declares the dependency. No registry involvement needed;
pkg-xis not a registry package.Shape A, the root declares it:
Shape B, another member declares it:
npm installs both shapes by linking the workspace member.
Cause
Two hand-copied versions of the same rule ("does this npm range accept this workspace member?") drifted:
Package::parse_dependency(Npm arm) decided link-vs-override withGroup::satisfies(range, member_version)alone.*does not satisfy1.0.0-beta.1under npm semver rules (a prerelease only satisfies a range that names a prerelease of the same version), so a root-declared wildcard took the override branch: the member's workspace dependency is displaced and the name resolves from the registry.get_or_put_resolved_packageadditionally acceptsnpm_group.is_star()for any present member (the Fix workspace packages not being found when they are moved #10899 rule), so a member-declared wildcard resolves to the workspace.The disagreement also produced shape B's permanent re-save: resolution links the member and the bun.lock loader rewrites the loaded dependency to workspace-tagged (
map_dep_to_pkg), but a fresh parse keeps it npm-tagged, so the loaded and re-parsed roots never compare equal and every install re-saves identical bytes (#37249 fixed the versionless flavor of this loop and listed this prerelease flavor as out of scope).Fix
Extract the rule into one shared predicate,
dependency::npm_range_accepts_workspace_member(satisfies, or wildcard on any present member), and call it from both sites. Resolution behavior is unchanged (the helper is the resolver's exact expression); parse gains the wildcard disjunct, which is the fix. The versionless case is unchanged: the Npm arm still requires a member version, and wildcard-on-versionless stays with the resolver per #10899.A wildcard therefore links the member wherever it is declared, matching npm's resolution for this shape, and the only registry fallback left for a member name is a non-wildcard range the member's version does not satisfy, which is bun's existing, deliberate divergence from npm and is untouched here (npm links the member for any range; bun's override rule lets e.g.
^2.0.0fetch a registry copy of a name that is also a member, and several existing tests depend on that).No loader change is needed: parse now produces the same workspace-tagged dependency
map_dep_to_pkgproduces on load, so lockfiles round-trip byte-stable and shape B's re-save loop stops. Lockfiles written by current bun for shape B load without a diff.Landing order: after #37248
Review of the first revision probed mixed declarations, where one package declares a range the member fails and another declares
*, and found this PR must land after #37248 (same-name collision fixes). Details, all verified against a local registry:^2.0.0+ sibling member*+ member1.0.0-beta.1: with this PR alone, install 1 resolves per-declarer (root registry, sibling workspace), and that split is a state today's bun.lock writer emits but its parser rejects, so install 2 hitsDuplicate package path, ignores the lockfile, and re-saves forever, with--frozen-lockfilefailing. This is not a new failure mode: the identical loop exists on an unmodified build whenever the sibling's range satisfies (root^2.0.0+ sibling^1.0.0+ member1.0.0), because satisfying ranges have always linked per-declarer. The representation bug is exactly what install: fix npm dependencies that share a workspace member's name #37248 fixes.--frozen-lockfilepasses, and the combined test suites pass (78 tests inbun-workspaces.test.ts, including install: fix npm dependencies that share a workspace member's name #37248's collision matrix). The merge reconciliation is mechanical because of the shared predicate: install: fix npm dependencies that share a workspace member's name #37248's restructured Npm arm (its versionlessis_starmatch is the helper's semantics) and its reload-mirror scan inbun.lock.rsboth become calls tonpm_range_accepts_workspace_member.dependenciesanddevDependenciesof one package.json (bun already warns "duplicate dependency") goes from a hardDuplicate package pathloop on an unmodified build to a quiet re-save under install: fix npm dependencies that share a workspace member's name #37248, identically for satisfying and wildcard ranges; and a package.jsonoverridesentry for a member's name already fails every install with a hardDependencyLooperror on an unmodified build when the range satisfies, which neither this PR nor install: fix npm dependencies that share a workspace member's name #37248 addresses (tracked separately; after this PR the wildcard flavor joins that family instead of escaping to the registry).#37249's loader gate mirrors the parse rule for versioned members and gains the same wildcard disjunct when both land; with the shared predicate that is a call-site change. The dist-tag flavor is #35468.
Verification
Three tests in
test/cli/install/bun-workspaces.test.ts(memberno-deps@3.0.0-beta.1shadowing registryno-deps, latest 2.0.0):*: resolves tono-deps@workspace:packages/no-deps, not registry 2.0.0; repeat install does not re-save;--frozen-lockfilepasses*: same resolution, and the second install no longer prints "Saved lockfile" (lockfile byte-identical)^1.0.0against the prerelease member still installs registry 1.1.0 (passes on an unmodified build by design)The first two fail on an unmodified build (registry 2.0.0 wins; "Saved lockfile" on the second install).
Locally green:
bun-workspaces(66),bun-install-registry(229 + 5 todo),isolated-install(62),bun-lock(17),bun-lockb(6),lockfile-only,lockfile-version-2(10),migration/migrate(21),migrate-bun-lockb-v2(2),bun-add(54). Checked by hand: both shapes install offline with no registry contact, and a package-lock.json written by npm 11 migrates to the workspace link and converges with a stable lockfile in both shapes.