install: fix npm dependencies that share a workspace member's name - #37248
install: fix npm dependencies that share a workspace member's name#37248robobun wants to merge 12 commits into
Conversation
- parse: a versionless member can only be linked by a wildcard range (mirrors resolution); any other range replaces the member's workspace dependency instead of leaving both in the root list and failing with a "dependency loop" internal error - bun.lock: only pre-claim a member's name as a root packages key when the lockfile assigns that key to the member itself, and skip injecting a member's workspace dependency when a root npm dependency replaced it, so a loaded lockfile matches a fresh parse and --frozen-lockfile passes - isolated linker: stop counting workspace packages reached through ordinary workspace: edges as installed on every run
WalkthroughChangesWorkspace and npm dependency resolution now handles versionless workspace members, package-name collisions, displaced workspace paths, duplicate workspace names, and ChangesWorkspace installation behavior
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}
|
Updated 8:00 AM PT - Aug 9th, 2026
❌ @robobun, your commit a2a3a2f has 4 failures in
🧪 To try this PR locally: bunx bun-pr 37248That installs a local version of the PR into your bun-37248 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches three interlocking pieces of workspace resolution — the parse-time link/override decision, lockfile reload's key pre-claiming and root-dependency injection, and isolated-linker install accounting — a human familiar with src/install/ should confirm the approach.
Checked member_owns_packages_key slice indexing — first[name.len()] and rest[b"workspace:".len()..] are guarded by the preceding .get() and short-circuited has_prefix.
Checked that the Installer.rs reclassification only widens the existing Skipped path (workspace-resolved packages reached via non-workspace edges) and does not affect the failure path.
Checked that the duplicated Npm-arm decision in parse_append_dependencies keys on npm().name (real name) so npm: aliases match, mirroring parse_dependency.
Extended reasoning...
Overview
The PR fixes bun install when a root npm dependency shares a name with a workspace member, across three coupled sites: Package::parse_dependency's Npm arm now runs for versionless members (using is_star() as the satisfaction rule); bun.lock.rs conditionally pre-claims a member's root packages key via a new member_owns_packages_key helper and skips injecting a workspace dependency the fresh parse would have overridden; and the isolated linker's on_task_complete reclassifies workspace-resolution completions as skipped so re-run symlink tasks don't inflate the install count. Six new tests cover both repro shapes, the isolated linker, npm: aliasing, linkWorkspacePackages=false, and a wildcard guard.
Security risks
None identified. Inputs are the user's own package.json and bun.lock; no network, path, or privilege boundaries are crossed. The new member_owns_packages_key string parsing is bounds-safe (.get() and short-circuited prefix checks guard every direct index).
Level of scrutiny
High. This is core package-manager logic where a subtle mismatch between the parse-time decision in Package.rs and its replicated form in bun.lock.rs would break lockfile round-tripping or --frozen-lockfile for workspace users. The author explicitly notes the two sites must stay in sync, and the Installer.rs change broadens which completions count as skipped (any workspace-resolved package via a non-workspace edge, not only displaced members). A maintainer who owns src/install/ should confirm the layering — particularly whether replicating the Npm-arm rule inside parse_append_dependencies is the right home versus deriving it from a single source, and whether the broadened Skipped classification affects first-install summary counts in shapes beyond the tested one.
Other factors
Test coverage is thorough (five of six new tests fail on an unmodified build per the description; the sixth is a stated behavior guard). The comment-cop bot flagged several comments and the author trimmed them in 5cbe12c with all threads resolved. The PR description references sibling PRs #37245 (file:) and #35468 (dist-tag) sharing the member_owns_packages_key hunk, so a maintainer coordinating those is another reason for a human look. CI build #90875 was still building at last timeline update.
|
While fixing the versionless-member re-save loop in #37249, a self-review turned up the pre-release variant of your Shape 1, reproducible on main: a member with a pre-release version (e.g. |
…and stale state
- decide member workspace-dependency injection from the lockfile's own
shape (packages-key ownership) before consulting the current
linkWorkspacePackages, so flipping the config re-resolves through the
normal changes diff instead of failing every install with an internal
DependencyLoop error
- resolve a displaced member's own dependencies under its recorded
packages key ("beta/member/dep"), not its name, so versions it pins
survive a cold install from the lockfile
- detect duplicate workspace names independently of the packages-key
claim and size the workspace package-id range by appended packages,
so a hand-edited lockfile with duplicate workspace keys errors
instead of indexing out of bounds
- replace an existing destination and retry when linking a workspace on
POSIX, matching the Windows branch: a member's internal node_modules
survives a root node_modules wipe
![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-workspaces.test.ts`:
- Around line 693-702: Update the subprocess handling in the install test to
read proc.stdout.text() concurrently with proc.stderr.text() and proc.exited via
Promise.all. Capture stdout alongside stderr and exitCode, then assert the
combined output while preserving the existing exit-code and duplicate-workspace
expectations.
🪄 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: 0a3ad19c-82a8-434b-8cf6-d7cf292d457b
📒 Files selected for processing (5)
src/install/PackageInstall.rssrc/install/isolated_install/Installer.rssrc/install/lockfile/Package.rssrc/install/lockfile/bun.lock.rstest/cli/install/bun-workspaces.test.ts
…from the re-save quirk
…ot its name The "<name>/<dep>" probe belongs to whichever package owns the root <name> entry; for a displaced member that is the npm package that replaced it, so a member dependency hoisted to the root bound to the npm package's nested copy instead. Walk the member's recorded placement key upward like hoisting does and only probe by name when the member owns its root name entry.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/lockfile/bun.lock.rs (1)
2136-2179: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject duplicate workspace names during the initial
workspacesscan.NameHashMap::insertoverwrites entries with the samename_hash, so duplicate names at different paths leave only one path in the snapshot and bypassseen_workspace_names. The current test passes only because distinct names create repeated entries for one path.🤖 Prompt for 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. In `@src/install/lockfile/bun.lock.rs` around lines 2136 - 2179, Update the initial workspaces scan to detect duplicate names before constructing the workspace-path snapshot, rather than relying on the deduplicating NameHashMap::insert result. Ensure every workspace entry is checked against seen_workspace_names, including duplicate names at different paths, and return InvalidWorkspaceObject with the existing duplicate-name error.
🤖 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/lockfile/bun.lock.rs`:
- Around line 2901-2922: Update the nested resolution logic around
member_tree_keys to use the same scope-aware boundary handling as
find_resolution, rather than trimming prefix at every raw slash. Ensure scoped
package paths such as `@scope/pkg/member` do not probe `@scope/dep` as a placement
node, while preserving valid package-boundary resolution.
In `@test/cli/install/bun-workspaces.test.ts`:
- Around line 740-742: Update the reinstall step in the test around
runBunInstall to capture its result, then assert that no lockfile-save event or
message occurred, matching the negative assertion used by the sibling test near
line 693. Keep the existing byte-for-byte lockfile comparison to verify content
stability, but do not rely on savesLockfile: false because it only disables the
harness assertion.
- Around line 688-696: Strengthen the assertions in the workspace install test
by reading the root-level no-deps manifest after installation and asserting its
version is 2.0.0. Keep the existing lockfile and absence-of-nested-copy checks,
but add the manifest-version assertion so the test verifies the member resolves
to the intended root version.
---
Outside diff comments:
In `@src/install/lockfile/bun.lock.rs`:
- Around line 2136-2179: Update the initial workspaces scan to detect duplicate
names before constructing the workspace-path snapshot, rather than relying on
the deduplicating NameHashMap::insert result. Ensure every workspace entry is
checked against seen_workspace_names, including duplicate names at different
paths, and return InvalidWorkspaceObject with the existing duplicate-name error.
🪄 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: 16dfd0c4-32ca-488d-98d1-20761e32724f
📒 Files selected for processing (2)
src/install/lockfile/bun.lock.rstest/cli/install/bun-workspaces.test.ts
- reject duplicate workspace names in the initial workspaces scan; NameHashMap::insert silently kept only the last path - treat a scoped package name as a single node when walking a member's placement, so "@scope/pkg/member" never probes "@scope/<dep>" as a placement level - strengthen reload tests: assert the resolved root version, and that reinstalls do not rewrite an identical lockfile
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/lockfile/Package.rs (1)
1754-1764: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle wildcard ranges consistently for versioned prerelease workspace members.
"*"links a prerelease workspace member during resolution, but both parse predicates reject it through normal semver satisfaction. This causes lockfile churn and can break frozen reinstalls.
src/install/lockfile/Package.rs#L1754-L1764: acceptis_star()beforesatisfies(...).src/install/lockfile/bun.lock.rs#L3509-L3514: use the same wildcard-aware predicate for root workspace injection.test/cli/install/bun-workspaces.test.ts#L758-L797: add a1.0.0-beta.1plus"*"regression that checks stable reinstall and frozen install.🤖 Prompt for 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. In `@src/install/lockfile/Package.rs` around lines 1754 - 1764, Update the workspace dependency predicate in src/install/lockfile/Package.rs lines 1754-1764 to accept is_star() before satisfies(...) for versioned members, while preserving the existing versionless behavior. Apply the same wildcard-aware predicate to root workspace injection in src/install/lockfile/bun.lock.rs lines 3509-3514. Add a regression in test/cli/install/bun-workspaces.test.ts lines 758-797 covering a 1.0.0-beta.1 workspace member with "*" and verifying stable reinstall plus frozen install.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/install/lockfile/Package.rs`:
- Around line 1754-1764: Update the workspace dependency predicate in
src/install/lockfile/Package.rs lines 1754-1764 to accept is_star() before
satisfies(...) for versioned members, while preserving the existing versionless
behavior. Apply the same wildcard-aware predicate to root workspace injection in
src/install/lockfile/bun.lock.rs lines 3509-3514. Add a regression in
test/cli/install/bun-workspaces.test.ts lines 758-797 covering a 1.0.0-beta.1
workspace member with "*" and verifying stable reinstall plus frozen install.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e74cc000-03b9-491b-866f-ab0388770d71
📒 Files selected for processing (3)
src/install/lockfile/Package.rssrc/install/lockfile/bun.lock.rstest/cli/install/bun-workspaces.test.ts
|
On the review note about wildcard ranges and prerelease member versions: that asymmetry is pre-existing and unchanged by this PR. Verified on an unmodified build: a root |
|
Status: the branch is complete and verified. The twelve tests in the new describe block pass with the fix and ten fail without it (the wildcard and hoisted-placement guards pass by design on an unmodified build); bun-workspaces (74), isolated-install (62), the lockfile suites (37), migration/migrate (21), bun-add (54), and bun-install-registry (234) pass locally, and clippy is clean. The robobun/evidence check is currently failing for environment reasons in its sandbox: its last two runs hit toolchain download timeouts (static.rust-lang.org), a failed vendored-dependency fetch (libwebp), posix_spawn EAGAIN storms, and cargo aborting with no diagnostic, on the legs that run without this diff applied as well as the ones with it. The check's one-line summary does not reflect the change. Buildkite CI on the current head is running normally. |
|
Heads up: #37264 changes the link-or-override predicate in the Npm arm of |
|
CI update for the current head (a2a3a2f, Buildkite build 90940): the failures are unrelated to this diff. One red is a 30s timeout in test/cli/run/require-cache.test.ts on darwin x64 (module loading, reported for triage separately); the rest are annotated flaky (passed on retry or alone) or pre-existing. No install-related lane failed. The diff stands as verified in the description. |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no new issues; the three findings from earlier rounds (workspace_node_modules false-hit, intermediate-node hoist, scoped-name walk boundary) are all addressed with regression tests. Because this reworks lockfile-reload resolution semantics and the workspace/npm override predicate — correctness-critical install paths with several interacting edge cases — a human look is still warranted before merge.
What was reviewed
- The scope-aware ancestor walk in
nested_res_id— verified@scope/nameis treated as a single node and a leading scope stops the walk. member_owns_packages_keyand the conditionalpkg_mapclaim — checked that duplicate detection moved to a dedicated seen-set so displaced members don't bypass it.- The
parse_append_dependenciesoverride mirror — confirmed it honors the recorded lockfile shape before falling back to the config-driven predicate, so alinkWorkspacePackagesflip re-resolves rather than bricks.
Extended reasoning...
Overview
The PR fixes bun install failures when a root npm dependency shares a name with a workspace member. It touches four Rust files in src/install/: the Npm arm of Package::parse_dependency now handles versionless members; bun.lock.rs gains member_owns_packages_key, a member_tree_keys list, a scope-aware ancestor walk for resolving a displaced member's dependencies on reload, dedicated duplicate-workspace-name detection, and two new parameters on parse_append_dependencies; the isolated linker skips workspace-resolution completions from the install count; and install_from_link retries once on EEXIST on POSIX. Twelve tests are added under a new describe block in bun-workspaces.test.ts.
Security risks
None identified. The change is confined to lockfile parsing and dependency-graph resolution over data Bun itself wrote (or the user hand-edited). The new duplicate-name guard tightens validation of hand-edited lockfiles rather than relaxing anything.
Level of scrutiny
High. This is core package-manager logic: the lockfile reload path decides which package version every dependency binds to, and the changes here introduce a new resolution walk that duplicates (with a stated reason) part of find_resolution's traversal. Earlier review rounds on this PR found three real bugs in successive revisions of that walk, each requiring a follow-up commit and regression test — that history alone argues for a maintainer sign-off on the final shape. The parse_append_dependencies signature change and the linkWorkspacePackages config-flip semantics are behavior decisions a human owner should confirm.
Other factors
All prior review threads (mine, CodeRabbit's, comment-cop's) are resolved. Test coverage is thorough — twelve targeted tests, ten of which fail on an unmodified build per the description, plus the existing suites reported passing locally. There is a noted overlap with open PR #37264 on the same Package::parse_dependency hunk and with #37245 on member_owns_packages_key, which a human should be aware of when sequencing merges. The change is well-motivated and carefully done, but it is not a mechanical fix.
|
Follow-up to my earlier note, with the interaction verified on a local merge of this branch with #37264 (which now extracts the acceptance rule into a shared
#37264's body now states it lands after this PR. |
|
A third shape of this, in case it is cheap to cover here: a dist-tag instead of a range. With the test registry (no-deps latest = 2.0.0): Same on current main (a5c86ae); |
1 participant
Repro
Shape 1, versionless member:
Shape 2, versioned member the range does not satisfy, kept alive by another member:
Every install after the first ignores the lockfile and re-resolves, and frozen installs (CI) are permanently broken. npm installs both shapes.
Cause
Three gaps around one state: a root npm dependency replacing a member's implicit workspace dependency (the existing "Override the workspace with the other dependency" rule in
Package::parse_dependency).DependencyLoopinternal error. Resolution already has a rule for this member (get_or_put_resolved_package, Fix workspace packages not being found when they are moved #10899: a versionless member is linkable only by a wildcard range); parsing never mirrored it.packageskey. When the override gave that key to the npm package and the member only appears nested ("beta/alpha"), parsing failed withDuplicate package pathon a key that appears once.parse_append_dependenciesunconditionally injected every member's workspace dependency into the loaded root dependency list, so it never matched a fresh package.json parse and the install re-resolved and re-saved forever.One follow-on in the isolated linker: workspace store tasks re-run on every install by design (no on-disk freshness check) and are excluded from the install summary only when the dependency edge carries workspace behavior. A displaced member is reachable only through ordinary
workspace:edges, so every install of an already-converged tree reported+ alpha@workspace:packages/alphaand a nonzero install count, never "no changes".Review of the first revision found three more problems in the newly loadable state, all reproduced before fixing:
linkWorkspacePackagesafter the lockfile was written brickedbun installpermanently: the injection decision read the current config while thepackages-key ownership embodied the old one, so the injected workspace dependency bound to the npm package and every install (plain and frozen) died with theDependencyLoopinternal error untilbun.lockwas deleted."beta/alpha/dep"by the writer, but the reload resolver only tried"alpha/dep"and"dep", so a cold install silently bound the member's dependency to the root version. Review of that fix found the complementary hole: the"alpha/dep"probe belongs to the npm package that owns the rootalphakey, so a member dependency that dedup-hoisted to the root bound to the npm package's nested copy of the same name.packages-key claim, so a hand-edited lockfile with duplicate workspace keys could over-count the workspace package range and fail with an index-out-of-bounds crash instead of a parse error. Review also pointed out that duplicate names at different paths silently kept only the last path (the name map overwrites on insert); the workspaces scan now rejects those too.And one pre-existing gap this state exposes: linking a workspace symlink on POSIX failed with
EEXIST: failed linking dependency/workspace to node_moduleswhen the destination survived inside a member's own node_modules (a rootrm -rf node_modulesdoes not resetpackages/*/node_modules, andskip_deleteassumes a fresh tree). The Windows branch already replaces and retries.Fix
Package.rs: the Npm arm runs whenever a member shares the dependency's real name. Satisfaction for a versionless member isis_star(), matching resolution. Wildcard plus versionless keeps today's behavior (the dependency stays as-is and resolution links the member); any other unsatisfied case takes the existing override branch, the same as a versioned mismatch.bun.lock.rs:packageskey when the lockfile assigns that key<name>@workspace:<path>(member_owns_packages_key);--frozen-lockfilewith the normal "lockfile had changes" error, and re-resolves on a plain install;packageskey (displaced or aliased) resolve their own dependencies by walking that placement upward the way hoisting does ("beta/alpha/dep", then"beta/dep", then the root), treating a scoped package name as a single node, and the name-based"alpha/dep"probe only runs when the member owns its root name entry.isolated_install/Installer.rs:on_task_completealso skips completions whose package resolution is a workspace, so the re-run symlink task stops being counted as an install.PackageInstall.rs: on POSIX,install_from_linkreplaces an existing destination and retries once onEEXIST, matching the Windows branch.The
file:cousin of this collision is #37245 (open);member_owns_packages_keyhere is the same code, so whichever PR lands second rebases that hunk away. The dist-tag flavor is #35468.Verification
Eleven tests in
test/cli/install/bun-workspaces.test.tsunderworkspace and npm dependency sharing a name:"no-deps"key, correct registry/nested placement, stable reinstall,--frozen-lockfilepassesnpm:dependency colliding with a member's namelinkWorkspacePackages = falseround-tripping a satisfied rangelinkWorkspacePackagesin both directions: loud frozen failure, plain install re-resolves and convergesa-dep@1.0.2, member keepsa-dep@1.0.1)two-range-deps/no-deps)Duplicate workspace name, no crashTen of eleven fail on an unmodified build with the errors quoted above (only the wildcard guard passes there by design).
bun-workspaces(72),isolated-install(62),bun-lock/lockfile-only/lockfile-version-2/bun-lockb/migrate-bun-lockb-v2/migration/migrate/bun-add(112), andbun-install-registry(234) pass locally;migration/complex-workspacefails only on gitlab/bitbucket network access andbun-link's "link dependency without crashing" times out locally, both identically without this diff.