install: keep the workspace: spelling given to bun add <member>@workspace:<range> - #38866
install: keep the workspace: spelling given to bun add <member>@workspace:<range>#38866robobun wants to merge 6 commits into
Conversation
…pace:<range> bun add pkg1@workspace:^ (or ~, or a version) wrote "workspace:*" into package.json and bun.lock. The write-back that runs after the install replaces the entry's value before deciding what to save, and the request itself is bound to the root's implicit row for the member, so the typed spelling was gone by the time the workspace arm ran. Carry the declared literal into the rebuilt entry and keep it when it uses the workspace: protocol; anything else that linked a member still saves workspace:*.
|
Warning Review limit reached
Next review available in: 25 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 (2)
Comment |
|
Status: ready for review. The change is complete as of 4f1e346 (1c30726 is an empty re-run commit); no further pushes are planned. CI on that head (build 97849) has every lane that ran passing; the two macOS 14 aarch64 test lanes have been sitting unscheduled on every build today, not just this one. Reproduced on a debug build of main (39fb3c1): root Shape note: a version of the fix that reads the bound lockfile row's literal (on top of #38847's binding fix) was tried and fails the two "replaces an existing entry" tests, because a respelled entry that resolves to the same member is no diff to the install and the old row is kept; hence the fix reads the entry's own text. Details in the PR body. Related: #38847 covers the |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of either, though both are neighbors in the same function:
The nine spelling tests added here fail on main and would fail the same way with either of those PRs applied. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow, well-reasoned fix with thorough test coverage.
Checked that seeding declared from the existing property value only reaches the resolution::Tag::Workspace arm: the before-install pass never has replacing entries (package_id == INVALID_PACKAGE_ID), Subcommand::Update never counts entries as replacing, and every other match arm (Npm, catch-all) plus the uninitialized branch overwrite the value without reading it. Also confirmed edit()'s changed return is discarded by add_catalog::edit_target, so the case where the workspace literal is kept in place (comparison at the end sees equal slices) doesn't skip a needed write. dependency::Tag::infer returns Tag::Workspace for any workspace: prefix (dependency.rs:1044), and the negative-case tests pin 1.0.0/^1.0.0 still saving workspace:*.
Extended reasoning...
Overview
Two changes in PackageJSONEditor::edit (src/install/PackageManager/PackageJSONEditor.rs): (1) when rebuilding a replaced dependency entry, seed the new value node with the entry's declared literal (arena-duped) instead of b""; (2) the resolution::Tag::Workspace write-back arm now keeps the entry text when dependency::Tag::infer classifies it as Workspace, otherwise falls back to workspace:*. Eleven new tests in test/cli/install/bun-update-lockfile-sync.test.ts cover workspace:^/~/1.0.0/^1.0.0 from the root, from a member, with --filter, with -d and no lockfile, replacing an existing entry in the target group, editing in place in devDependencies, plus the negative case that a plain semver range still saves workspace:*.
Security risks
None. This is package.json string formatting for workspace references; no untrusted input parsing, no filesystem/network changes beyond what bun add already does.
Level of scrutiny
Medium. edit() is on the bun add/update/link write-back path and mistakes surface directly in user package.json files. I traced whether change (1) could leak into any other arm: it cannot, because replacing is only incremented when package_id != INVALID_PACKAGE_ID && subcommand != Update, i.e., only in the after-install pass for add/link, where the match then runs and every non-Workspace arm computes new_literal without reading e_string.data. The before-install uninitialized branch reads e_string only under Subcommand::Update, which never rebuilds. I also checked that the changed bool going false when the literal is kept in place is harmless — the sole caller add_catalog::edit_target discards it and returns Result<(), _>.
Other factors
The PR description explains why request.version.literal cannot be used directly (bind_update_requests rebinds it to the member's path on the root) and why the entry text is the right source. The arena_dup on declared matches the file's convention for every other E::EString::init site. Tests assert both package.json and bun.lock and each finishes with a --frozen-lockfile reinstall, so a saved literal that the next install would reject would fail the test. The description reports the nine spelling tests fail on the unfixed build and that six related suites pass with the debug build. This composes cleanly with the open #38847 (the bun update side) since both branch on distinct predicates inside the same arm.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that preserves the user's workspace: spelling in the after-install write-back, with thorough test coverage.
What was reviewed:
- Traced the
declaredseeding through every write-back arm — confirmed it's only non-empty on the after-install "replacing" path, and every arm other thanWorkspaceoverwrites (or byte-equals) it, so no side effects on npm/git/tarball entries. - Checked
dependency::Tag::infer— anyworkspace:prefix classifies asWorkspace; ranges, paths, and empty strings hit theworkspace:*fallback, preserving prior behavior (covered by the two negative tests). - Verified the
changedflag is still set viaremaining != 0even when the self-compare skips the write;arena_dupkeeps the captured literal on the process-lifetime arena. - Tests cover the variant matrix: root/member/
--filter,-d, replacing an existing entry, devDependencies edit-in-place, plus--frozen-lockfileround-trip.
Extended reasoning...
Overview
Two-file change: src/install/PackageManager/PackageJSONEditor.rs (~15 lines in edit()) and eleven new tests in test/cli/install/bun-update-lockfile-sync.test.ts. The fix makes bun add <member>@workspace:<range> write the range as typed (workspace:^, workspace:~, workspace:1.0.0) instead of unconditionally saving workspace:*. It does this by (a) seeding the rebuilt property with the declared literal instead of "" so the value survives to the write-back, and (b) branching the resolution::Tag::Workspace arm on dependency::Tag::infer(e_string.data.slice()) — keep if already workspace:-prefixed, else workspace:*.
Security risks
None. This is the package.json write-back for bun add; the input is a CLI positional the user typed, and the install has already resolved it. No parsing of untrusted network data, no path handling, no auth.
Level of scrutiny
Medium — package manager write-back touches user files, but the change is small and mechanically verifiable. I traced the declared capture through all four write-back arms (unresolved/before-install, Npm, Workspace, catch-all) and confirmed the PR's claim that every arm except the new Workspace-keep case overwrites it: the before-install branch always reaches the if e_string.data.slice() != version_literal write (and declared is only non-empty in the after-install "replacing" path anyway, since replacing requires package_id != INVALID); the Npm and catch-all arms compute a fresh literal and byte-compare. The changed return value is already forced true by remaining != 0 before the rebuild loop, so the skipped self-compare in the keep case doesn't lose the write signal. arena_dup copies the Store-backed literal into the process-lifetime arena, so no lifetime issue.
Other factors
Test coverage is strong: nine spelling tests across root/member/--filter/-d/devDependencies/replacing, each asserting package.json + bun.lock + --frozen-lockfile reinstall, plus two negative tests (pkg1@1.0.0 and pkg1@^1.0.0 still save workspace:*). The author confirmed the nine tests fail on main and ran the adjacent suites (bun-add, bun-add-filter, bun-add-catalog, bun-workspaces, bun-update, catalogs). The comment-cop bot flagged earlier long comments; the author shortened them to one-liners in the final two commits. No CODEOWNERS coverage on src/install/. Composes with the open #38847 (bun update side of the same arm) — that PR guards on Subcommand::Update and would match before this arm.
There was a problem hiding this comment.
LGTM — targeted fix that preserves the user's workspace: spelling in bun add, falling back to the old workspace:* for non-workspace literals.
What was reviewed:
- Traced the
declaredinitialization: only the after-installreplacingrebuild path sees a non-empty value (before-install haspackage_id == INVALIDsoreplacing = 0); every other write-back arm overwrites it, so it's inert outside the workspace arm. - Verified
dependency::Tag::inferreturnsWorkspacefor anyworkspace:prefix andNpm/Folderfor the fallback cases the tests pin. - Checked the
changedreturn value is discarded at every call site (add_catalog::edit_target), so the no-op comparison when the literal is kept doesn't skip a write. - Tests cover the variant matrix (four spellings, root/member/
--filter, fresh/existing entry, same/other group, no-lockfile) and each reinstalls with--frozen-lockfile.
Extended reasoning...
Overview
Two hunks in src/install/PackageManager/PackageJSONEditor.rs::edit: (1) the rebuilt property slot for a replacing entry now starts out holding the entry's previous literal (arena-duped) instead of "", and (2) the resolution::Tag::Workspace write-back arm keeps e_string.data.slice() when dependency::Tag::infer classifies it as Workspace, otherwise falls back to the old b"workspace:*". Twelve new test cases in test/cli/install/bun-update-lockfile-sync.test.ts under the existing bun add describe block.
Security risks
None. This is package.json write-back formatting; no untrusted input parsing, no filesystem traversal, no network. The literal being kept is what the user typed on the CLI and what the install just successfully resolved.
Level of scrutiny
Medium — bun install write-back touches users' package.json, so a wrong value would be user-visible and annoying to undo. But the change is strictly narrowing: the new arm either keeps exactly what the before-install pass wrote (which the install just parsed and accepted) or produces the identical old output. I traced that (a) before-install never hits the replacing rebuild (package_id is still INVALID, so fresh slots stay "" and existing entries are edited in place), (b) the Subcommand::Update catalog check and !e_string.is_blank() guard in the unresolved branch are both gated on Update, which never reaches the non-empty declared path, and (c) edit()'s bool return is discarded at every caller so the equal-slice comparison is harmless. dependency::Tag::infer on workspace: at src/install/dependency.rs:1044 confirms the classification.
Other factors
Test coverage is thorough per REVIEW.md's variant-matrix rule: four spellings × root, -d without a lockfile, from another member, replacing an existing workspace:* and 1.0.0 entry (the case that ruled out reading the bound row), an entry in devDependencies (edit-in-place path), --filter targeting two workspaces, and two negative tests pinning the workspace:* fallback for plain ranges. Each asserts both package.json and bun.lock and follows with --frozen-lockfile. The PR body documents why the alternative (reading request.version.literal) fails those two tests. All three comment-cop threads are resolved (comments cut to one line). No CODEOWNERS coverage on these files. Composes with the adjacent open PR #38847 (bun update side) in either order.
1 participant
Problem
bun add pkg1@workspace:^(pkg1 being a member) installs fine ("installed pkg1@workspace:packages/pkg1") but writes"pkg1": "workspace:*"into package.json, and from there into bun.lock. Same forworkspace:~,workspace:1.0.0,workspace:^1.0.0; from the root, from another member, and with--filter. The spelling matters:bun pm pack/bun publishturnworkspace:^into^1.0.0,workspace:~into~1.0.0andworkspace:*into1.0.0(docs/pm/workspaces.mdx), so the published range silently changes. pnpm and yarn keep the spelling as given. Found while checking pnpm parity; no issue is filed for it.PackageJSONEditor::editruns after the install (src/install/PackageManager/PackageJSONEditor.rs, thematchon the resolved package's resolution tag) has an unconditionalresolution::Tag::Workspace => b"workspace:*"arm. It dates from fix(install): workspace version added to package.json #11241, which replaced the member's path that the generic arm used to write; at that point the arm had nothing better to write, because neither place that once held the typed spelling still does:request.version.literalis the literal of the lockfile row the request was bound to, not the request. Between the two passesLockfile::bind_update_requestsbinds the request to the first matching row of the workspace being edited. On the root that is today the implicit row created for each member of theworkspaceslist (literalpackages/pkg1). And when the entry already existed, it is the row of the lockfile the install started with:dependency::Version::eqlcompares two workspace rows by the path they resolve to, so changingworkspace:*(or a plain1.0.0that linked the member) toworkspace:~is no diff, the old rows are kept, and the bound literal is the previous spelling.sync_lockfilecorrects the rows from package.json only after this write-back has run.workspace:^there), but the after-install pass counts a bound request whose name already sits in the target group asreplacingand rebuilds that property with an empty value (the// we set it laterslot) before thematchruns.Fix
"". Every arm of the write-back overwrites it except the new case below, so on its own this changes nothing; it only makes the declared text reachable where the decision is made. An entry that lives in another dependency group is edited in place and already held it.workspace:protocol (dependency::Tag::infer(...) == Workspace) and writesworkspace:*otherwise.--filter). It is also safe to keep as is: the root's package.json containing exactly this literal is what the install just parsed and resolved. An arm readingrequest.version.literalinstead was tried (with the binding fix from install: keep workspace and other non-registry entries as written on bun update <name> #38847 applied) and fails the two existing-entry tests below, writing the previous spelling back.workspace:*stays the fallback:bun add pkg1@1.0.0,bun add pkg1@^1.0.0, a folder path, or a bare name the registry cannot satisfy all link the member without saying so; their entry text is a range or a path, whichinferdoes not classify asWorkspace, so they are saved exactly as before.bun add pkg1@workspace:*still producesworkspace:*.bun update <member>side of the same arm (skips the write-back underSubcommand::Update, and stops binding to the root's implicit rows); this PR is thebun addside. The two touch adjacent lines of thematchand compose in either order; this change does not depend on the binding fix. Until install: keep workspace and other non-registry entries as written on bun update <name> #38847 lands,bun update <member>on aworkspace:entry incidentally keeps it too, which is the behavior install: keep workspace and other non-registry entries as written on bun update <name> #38847 tests.bun addblock of test/cli/install/bun-update-lockfile-sync.test.ts:workspace:^/~/1.0.0/^1.0.0from the root,-dwithout a lockfile, from another member, replacing an existingworkspace:*entry and an existing1.0.0entry (both keep bun.lock's old row through the install), an entry listed in devDependencies (the edit-in-place path),--filtertargeting a member and the root, pluspkg1@1.0.0andpkg1@^1.0.0pinningworkspace:*. The ten spelling tests fail on the unfixed build (package.json hasworkspace:*) and pass with the fix; each asserts package.json and bun.lock and reinstalls with--frozen-lockfile.cargo clippy -p bun_installis clean.Background
bun addedits package.json in two passes around the install:edit()runs before it to put the requested literal in place (so the install resolves it) and again afterwards to replace that literal with what should be saved (^1.2.3for a dist-tag, the literal itself for git/folder/tarball,workspace:*for a workspace member). The second pass is where this change lives.UpdateRequestis one CLI positional. After resolution,bind_update_requestsattaches it to a dependency row of the workspace being edited, settingpackage_id(what it resolved to) andversion(that row's literal and tag).e_stringis the request's pointer at the string node of its package.json entry. In the after-install pass an entry already in the target group is rebuilt (re-keyed with the resolved name) ande_stringpoints at the rebuilt node; an entry found in another group is pointed at directly.dependency::Tag::inferclassifies a version literal by its text; anyworkspace:prefix isTag::Workspace, a range like1.0.0isNpm,./xisFolder, an empty string isDistTag.