install: keep workspace and other non-registry entries as written on bun update <name> - #38847
install: keep workspace and other non-registry entries as written on bun update <name>#38847robobun wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesWorkspace dependency update preservation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced on a debug build of main with a root declaring |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not linking #32808 from this PR. Its scenario ( |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Neither is a duplicate:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reworked in 8b08ff8 (comments shortened in 89d1bf3) after the review finding that the pass before the install still rewrote a Two other changes in the same push: |
There was a problem hiding this comment.
LGTM — the concern from my earlier review is addressed: both editor passes now skip workspace: literals via the same Tag::infer check that already guards catalog:, so --latest / @<spec> no longer rewrite the entry before the after-install continue fires.
Checked that bun add <member>@workspace:* still reaches the b"workspace:*" arm (subcommand guard), and that skipping behavior.is_workspace() rows in bind_update_requests still binds bun add to the newly-created dependencies row after the before-install reparse. The skip mirrors tree_printer.rs:214 and dedupe.rs:589 for the same implicit rows.
Extended reasoning...
Overview
Three small changes to the named-update path of the package manager: (1) PackageJSONEditor::edit extends the existing catalog: literal skip to also match workspace: via matches!(Tag::infer(...), Catalog | Workspace), so neither the before-install nor after-install pass rewrites a workspace: entry under bun update <name>; (2) a new resolution::Tag::Workspace if subcommand == Update => continue arm before the unconditional b"workspace:*" write-back, which keeps a plain range that linked a member (e.g. ^1.0.0) as written while leaving bun add on the existing arm; (3) Lockfile::bind_update_requests skips rows with behavior.is_workspace() — the root's implicit workspaces-glob rows — so a request binds to the actual package.json entry (or stays unbound if the root does not declare the member, which the existing never-add check then honors). Ten test cases cover workspace:^/~/1.0.0/plain range, --latest, @<range>, @<tag>, from a member, -r, and the undeclared root.
Security risks
None. This is package.json/bun.lock editing logic for bun update <name>; no untrusted input parsing, network, or filesystem-path handling changes.
Level of scrutiny
Moderate — bun install/update is user-facing and the edit() two-pass state machine is intricate, but each change here is a narrow continue that follows the exact shape already used for catalog: in the same function, and the bind_update_requests skip mirrors tree_printer.rs:214 for the same implicit rows. The subcommand guards keep bun add on its existing paths.
Other factors
My earlier review on the first revision flagged that the after-install continue alone would leave a --latest/@<spec>-rewritten literal in place; 8b08ff8 addressed that by moving workspace: into the pre-existing literal check that runs before the unresolved branch, and added the three explicit-spec cases to the test matrix. I traced bun add pkg1@workspace:* through the new code: after the before-install pass creates the dependencies row and reparses, bind_update_requests matches that row (its behavior is not WORKSPACE), resolution.tag == Workspace with subcommand == Add falls through to b"workspace:*" — the existing test for that command (bun add <workspace>@workspace:*) is in the same file. The comment-cop feedback on comment length was resolved in 89d1bf3. The PR description reports the wider install/add/update/workspace suites passing on the debug build.
|
Updated 10:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 2776a09 has some failures in 🧪 To try this PR locally: bunx bun-pr 38847That installs a local version of the PR into your bun-38847 --bun |
|
Heads-up on an overlap: #38827 changes the same per-entry check in |
The named update path wrote "workspace:*" over any entry that resolved to
a workspace member ("workspace:^", "workspace:~", "workspace:1.2.3", a
plain range linked to the member), and added such an entry to a root that
did not declare the member, because the root's dependency rows include one
row per workspace member and the never-add check was keyed on whether the
request had been bound to a row. Under update, skip the write-back for
workspace resolutions and never create a slot; bun add still writes
"workspace:*".
…d requests to the declared row The pass that runs before the install rewrote a workspace: entry to "latest" (--latest) or to an explicit <name>@<range>, so skipping it only after the install was not enough. Extend the literal check that already keeps catalog: references to workspace: ranges, which covers both passes. bind_update_requests now skips the root's implicit workspaces rows, so a request is bound to the package.json entry that names it: an undeclared member stays unbound and is not added, and the write-back is keyed on the entry's own resolution rather than the member's. The earlier remaining rewrite is no longer needed and is reverted.
Use the predicate the bare update already applies (Npm or DistTag literals move, nothing else) instead of listing catalog: and workspace:, so a folder, tarball or git entry named on the command line is no longer replaced by the registry package under --latest or an explicit spec.
89d1bf3 to
c826a98
Compare
robobun
changed the title
|
Pushed c826a98 (rebased on current main). Following up on the self-review: the literal check that exempted |
Both files are asserted identical to what bun install just wrote, so the reinstall cannot add anything; it only adds a process per case.
| // A range that linked a workspace member has nothing to move to; `workspace:*` is what `bun add` writes. | ||
| resolution::Tag::Workspace if manager.subcommand == Subcommand::Update => continue, |
There was a problem hiding this comment.
🟡 The Workspace if Update => continue arm relies on the before-install pass leaving the entry byte-identical, but for a plain range like "pkg1": "^1.0.0" + bun update pkg1@1.0.0 the !is_npm() skip does not fire and the before-install pass rewrites to "1.0.0", so the ^ is dropped on disk — the registry path restores it via updating_packages[name].original_version_literal, this arm does not. On main the same input wrote workspace:* (also not kept-as-written), so this is a lateral change on a narrow input rather than a regression; the test.each covers ["^1.0.0", ["pkg1"]] and ["workspace:^", ["pkg1@^1.0.0"]] but not the cross-product ["^1.0.0", ["pkg1@1.0.0"]].
Extended reasoning...
What the bug is
The new after-install arm
resolution::Tag::Workspace if manager.subcommand == Subcommand::Update => continue,assumes the before-install pass left the entry untouched. That now holds for workspace: / file: / catalog: literals via the widened !dependency::Tag::infer(e_string.data.slice()).is_npm() skip, but not for a plain npm range that links a member. With root "pkg1": "^1.0.0" (member pkg1 at 1.0.0) and bun update pkg1@1.0.0:
- Before-install:
Tag::infer("^1.0.0") == Npm, sois_npm()is true and the!is_npm()skip does not fire. In the unresolved branch,request.version.tag == Npm(the positionalpkg1@1.0.0parses as an npm range), so theSome(existing) if request.version.tag != Npm && !explicit_dist_tagguard is bypassed andversion_literal = requested = "1.0.0".with_alias_of("^1.0.0", "1.0.0")returns"1.0.0"(split_npm_alias("^1.0.0")isNone).e_string.databecomes"1.0.0". Meanwhile the'add_packages_to_updateblock does captureupdating_packages["pkg1"].original_version_literal = "^1.0.0"(the tag check there is== Npm, which passes). - Install:
"1.0.0"is satisfied by the member's version, so the row resolves to the workspace member.bind_update_requests(with the PR's newis_workspace()skip) binds the request to the root'sdependenciesrow, whose resolved package'sresolution.tag == Workspace. - After-install: the scan re-binds
e_stringto the"1.0.0"node.Tag::infer("1.0.0").is_npm()is true, so the skip again does not fire.resolutions[package_id].tag == Workspaceandsubcommand == Update→ the new arm fires andcontinues, leaving"1.0.0"in place.
Result: "pkg1": "^1.0.0" → "pkg1": "1.0.0" in package.json and bun.lock. The declared ^ operator is dropped.
Why the existing code does not prevent it
The !is_npm() skip is the fix for the previous review round's finding (the workspace:^ + @<spec> case), and it works for every literal whose Tag::infer is not Npm / DistTag — but a plain range is exactly Npm, so it falls through. The Some(existing) if tag != Npm && !explicit_dist_tag guard keeps the existing literal only for a bare bun update pkg1 (which parses as an empty DistTag), which is why the tested ["^1.0.0", ["pkg1"]] case passes. An explicit @<npm-range> sets request.version.tag == Npm and takes the requested literal.
Inconsistency with the registry path
For the equivalent registry input — "no-deps": "^1.0.0" + bun update no-deps@1.0.1 — the resolution::Tag::Npm arm consults manager.updating_packages.get(request.name) and calls updated_version_literal(&entry.original_version_literal, resolved, …), producing "^1.0.1" (the existing test "bun update <name>@<version> -r keeps each workspace's operator" asserts exactly this). The Workspace arm never reads updating_packages, so original_version_literal = "^1.0.0" — which is populated — is ignored.
Impact
- On main the same input wrote
"workspace:*"— also not kept as written, but at least always linking the member. With this PR the entry becomes an exact/narrower npm range, which stops matching the member the next time its version is bumped past it. So on this specific narrow input the outcome is arguably worse than main's, but it is not a regression of previously-correct behavior — main was differently broken here. - The scenario is narrow: it requires (a) a plain npm range declaring a workspace member (uncommon vs.
workspace:), (b) an explicit@<npm-range>on that member's name, and (c) the member's version satisfying that range. The--latestand@latestvariants of the plain-range case rewrite to"latest"before install, which then fails (registry 404 or DependencyLoop per install: fix npm dependencies that share a workspace member's name #37248) and never reaches disk, so only@<npm-range>with a satisfied range is observable. - The user did explicitly type
@1.0.0, so writing"1.0.0"is defensible from their perspective — it is just inconsistent with how the same command treats registry packages.
Test gap
The PR's test.each covers ["^1.0.0", ["pkg1"]] (bare) and ["workspace:^", ["pkg1@^1.0.0"]] / ["workspace:^", ["pkg1@latest"]] (explicit spec on a workspace: literal), but not the cross-product ["^1.0.0", ["pkg1@1.0.0"]] — the one variant where the plain-range literal meets an explicit @<spec>. Per REVIEW.md's "Cover the variant matrix, not just the repro", that row belongs in the table.
How to fix
Have the Workspace if Update arm restore updating_packages[name].original_version_literal when present instead of a bare continue, mirroring how the Npm arm uses that map:
resolution::Tag::Workspace if manager.subcommand == Subcommand::Update => {
if let Some(entry) = manager.updating_packages.get(request.name) {
arena_dup(arena, &entry.original_version_literal)
} else {
continue;
}
}Alternatively, also skip the before-install rewrite when the entry's existing lockfile row resolves to a workspace — but that requires the lockfile in the before-install pass, which currently only sees resolutions == &[].
Step-by-step proof
- Root
package.json:{"name": "root", "workspaces": ["packages/*"], "dependencies": {"pkg1": "^1.0.0"}};packages/pkg1/package.json:{"name": "pkg1", "version": "1.0.0"}.bun install. bun update pkg1@1.0.0.- Before-install
edit(): scan setsrequest.e_string→ the"^1.0.0"node.Tag::infer("^1.0.0")=Npm(discriminant < 3),is_npm()= true → skip does not fire. Unresolved branch:existing = Some("^1.0.0"),requested = "1.0.0",request.version.tag == Npm,explicit_dist_tag = false→ guardtag != Npm && !explicit_dist_tagis false →version_literal = "1.0.0".with_alias_of("^1.0.0", "1.0.0")="1.0.0".e_string.data = "1.0.0". - Install: root's
"pkg1": "1.0.0"matches memberpkg1@1.0.0→ linked as workspace.bind_update_requests: implicitWORKSPACErow skipped by the PR's newdep.behavior.is_workspace() → continue; thedependenciesrow matches →request.package_id= member;resolutions[member].tag == Workspace. - After-install
edit():e_string→"1.0.0".Tag::infer("1.0.0").is_npm()= true → skip does not fire. Match onresolutions[package_id].tag:Workspaceandsubcommand == Update→continue.e_string.datastays"1.0.0". flushsees"1.0.0" != "^1.0.0"on disk → writes"pkg1": "1.0.0"to package.json and bun.lock.
|
Since 2776a09 this PR carries the same Coverage that #38827 had and this PR does not yet, in case you want to fold it in (all of it is in c2f2216, same test file, and fails with the editor from main):
|
1 participant
Problem
"pkg1": "workspace:^"for a member,bun update pkg1reports(no changes)but rewrites the entry to"pkg1": "workspace:*"in package.json and bun.lock. Same forworkspace:~,workspace:1.2.3and a plain range such as^1.0.0that links the member; same from a member that declares it, and in every workspace with-r/--filter.--latestor an explicit spec (bun update pkg1 --latest,pkg1@^1.0.0,pkg1@latest) the entry is first replaced bylatest/ the spec, so the install asks the registry for the member's name. The same happens to every other non-registry entry: with"no-deps": "file:./local-no-deps"and a registry that hasno-deps,bun update no-deps --latestexits 0 and leaves"no-deps": "^2.0.0"(the registry package) in package.json and bun.lock;no-deps@^1.0.0leaves^1.1.0,no-deps@latestleaveslatest.bun update pkg1adds"pkg1": "workspace:*"to the root'sdependencies; a named update is documented never to add.PackageJSONEditor::edit, which runs once before and once after the install):catalog:entries. Every other kind went on to the rewrite before the install (latest/ the explicit spec) and, after it, to the unconditionalresolution::Tag::Workspace => b"workspace:*"arm, which is the valuebun add <member>@workspace:*needs for the entry it just created.Lockfile::bind_update_requestsattached a request to the first row of the workspace with that name. The root's rows begin with one implicit row per workspace member (Package.rs parses theworkspaceslist as a dependency group ahead ofdependencies), so on the root a member's name was always bound, whether or not the root declared it: that is what let the undeclared name past the editor's never-add check (package_id == INVALID_PACKAGE_ID), and for a declared entry it keyed the write-back on the member row instead of the entry's own resolution.Fix
edit_update_entries: onlyNpmandDistTagliterals move): underbun update, an entry whose literal is not a registry range or tag is skipped in both passes. That coverscatalog:as before plusworkspace:,file:folders and tarballs, git andlink:entries;bun addis unchanged. This is the rulebun updatealready follows for patterns (update_scope.rsskips rows resolving to a workspace) and that bun-add-catalog.test.ts pins forcatalog:entries with an explicit spec; pnpm and yarn leave such entries alone on update as well. A named update still re-resolves the entry (a git entry follows its branch); only package.json is left as written.bun update(continue), which is what keeps a plain range or dist-tag that happened to link a member ("pkg1": "^1.0.0") as written;bun addstill takes theworkspace:*arm.bind_update_requestsskips rows with theWORKSPACEbehavior, so a request is bound to the package.json entry that names it, as the tree printer already does when it decides what to print (should_print_package_install). An undeclared member stays unbound and the existing never-add check holds. The other case where the member row and the entry resolve differently (a dist-tag the registry satisfies on a member's name) does not install on main at all (DependencyLoop, noted on install: fix npm dependencies that share a workspace member's name #37248), so today this hunk is only observable as the spurious add.--latestrewrite in this same function withis_npm(). With this change that line is no longer reached for a non-registry entry on the named path, and its tests pass either way; a note is left there.workspace:^/workspace:~/workspace:1.0.0/^1.0.0withbun update pkg1;workspace:^withpkg1 --latest,pkg1@^1.0.0,pkg1@latest; afile:entry whose name the registry has, withno-deps,no-deps --latest,no-deps@^1.0.0,no-deps@latest; from a member that declares it;-r; and the undeclared root. Each asserts package.json and bun.lock byte-identical to whatbun installwrote (so a frozen reinstall would add nothing and is not run). With the two source files at their main versions, 13 of the 14 fail (rewrite, registry 404,^2.0.0/^1.1.0/latestwritten, or the added entry); the plainfile:case already passed and guards the new route. All pass with the fix.updateblock of bun-install-registry, all passing;cargo clippy -p bun_installis clean.Background
bun add/bun update <name>edit package.json in two passes around the install:edit()runs before it to put the literal to resolve into the entry (for update: the existing one,latestunder--latest, or the spec given on the command line), and again after it to replace that with the resolved version in the declared style.dependency::Tag::inferclassifies a version literal by its text:^1.0.0andlatestareNpm/DistTag(is_npm()), whileworkspace:^,catalog:,file:..., a git URL orlink:each get their own tag. It is what the barebun updateuses to decide which entries it may rewrite.UpdateRequestis one command-line positional. After resolution,Lockfile::bind_update_requestsattaches it to a dependency row of the current workspace by name, which setspackage_id(the package that row resolved to); the second pass switches on that package's resolution.e_stringpoints at the package.json value the first pass found for the name; a request without one is a name the file does not declare.WORKSPACE, literal = the member's path) ahead of the root's own entries. They are what links members into node_modules; they do not correspond to anything in the root's package.json.