Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/install/PackageManager/PackageJSONEditor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1336,9 +1336,9 @@
// derived from a `StoreRef` to the same `E::EString` is live inside this loop body,
// so this is the sole mutable borrow.
let e_string = unsafe { &mut *e_string };
// `bun update <pkg>` keeps a `catalog:` reference; `bun add` still replaces it.
// `bun update <pkg>` only moves registry entries, like `edit_update_entries`; `bun add` still replaces any entry.
if manager.subcommand == Subcommand::Update
&& dependency::Tag::infer(e_string.data.slice()) == dependency::Tag::Catalog
&& !dependency::Tag::infer(e_string.data.slice()).is_npm()
{
continue;
}
Expand Down Expand Up @@ -1451,6 +1451,8 @@
arena_dup(arena, installed)
}

// 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,

Check warning on line 1455 in src/install/PackageManager/PackageJSONEditor.rs

View check run for this annotation

Claude / Claude Code Review

Plain-range workspace link with @<spec> drops the declared operator

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 cha
Comment on lines +1454 to +1455

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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, so is_npm() is true and the !is_npm() skip does not fire. In the unresolved branch, request.version.tag == Npm (the positional pkg1@1.0.0 parses as an npm range), so the Some(existing) if request.version.tag != Npm && !explicit_dist_tag guard is bypassed and version_literal = requested = "1.0.0". with_alias_of("^1.0.0", "1.0.0") returns "1.0.0" (split_npm_alias("^1.0.0") is None). e_string.data becomes "1.0.0". Meanwhile the 'add_packages_to_update block does capture updating_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 new is_workspace() skip) binds the request to the root's dependencies row, whose resolved package's resolution.tag == Workspace.
  • After-install: the scan re-binds e_string to 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 == Workspace and subcommand == Update → the new arm fires and continues, 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 --latest and @latest variants 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

  1. Root package.json: {"name": "root", "workspaces": ["packages/*"], "dependencies": {"pkg1": "^1.0.0"}}; packages/pkg1/package.json: {"name": "pkg1", "version": "1.0.0"}. bun install.
  2. bun update pkg1@1.0.0.
  3. Before-install edit(): scan sets request.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 → guard tag != Npm && !explicit_dist_tag is falseversion_literal = "1.0.0". with_alias_of("^1.0.0", "1.0.0") = "1.0.0". e_string.data = "1.0.0".
  4. Install: root's "pkg1": "1.0.0" matches member pkg1@1.0.0 → linked as workspace. bind_update_requests: implicit WORKSPACE row skipped by the PR's new dep.behavior.is_workspace() → continue; the dependencies row matches → request.package_id = member; resolutions[member].tag == Workspace.
  5. After-install edit(): e_string"1.0.0". Tag::infer("1.0.0").is_npm() = true → skip does not fire. Match on resolutions[package_id].tag: Workspace and subcommand == Updatecontinue. e_string.data stays "1.0.0".
  6. flush sees "1.0.0" != "^1.0.0" on disk → writes "pkg1": "1.0.0" to package.json and bun.lock.

resolution::Tag::Workspace => b"workspace:*",
_ => arena_dup(arena, request.version.literal.slice(request.version_buf())),
};
Expand Down
4 changes: 4 additions & 0 deletions src/install/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,10 @@ impl Lockfile {
let resolved_ids: &[PackageID] = res_list.get(self.buffers.resolutions.as_slice());
debug_assert_eq!(resolved_ids.len(), workspace_deps.len());
for (&package_id, dep) in resolved_ids.iter().zip(workspace_deps.iter()) {
// The root's implicit `workspaces` rows are not package.json entries; bind to the entry naming the package.
if dep.behavior.is_workspace() {
continue;
}
if update.matches(dep, string_buf) {
if package_id as usize > self.packages.len() {
continue;
Expand Down
69 changes: 69 additions & 0 deletions test/cli/install/bun-update-lockfile-sync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,75 @@ describe.concurrent("bun update rewrites bun.lock together with package.json", (
await expectInSync(dir);
});

// Naming a workspace member used to rewrite the entry linking it to `workspace:*` (or, with --latest / an explicit
// range, to send the name to the registry), and to add an entry to a root that did not declare the member.
test.each([
["workspace:^", ["pkg1"]],
["workspace:~", ["pkg1"]],
["workspace:1.0.0", ["pkg1"]],
["^1.0.0", ["pkg1"]],
["workspace:^", ["pkg1", "--latest"]],
["workspace:^", ["pkg1@^1.0.0"]],
["workspace:^", ["pkg1@latest"]],
])("a %s entry linking a workspace member is kept as written by bun update %j", async (literal, args) => {
const dir = await setup(MONOREPO({}, { dependencies: { pkg1: literal } }));
const [pkgBefore, lockBefore] = await Promise.all([pkgText(dir), lockText(dir)]);
await run(dir, "update", ...args);
expect(await pkgText(dir)).toBe(pkgBefore);
expect(await lockText(dir)).toBe(lockBefore);
await expectInSync(dir, ["", PKG1]);
});

// Same rule for the other non-registry kinds: the registry has a no-deps, so naming this entry with --latest or an
// explicit spec used to replace the folder with the registry package (exit 0).
test.each([[["no-deps"]], [["no-deps", "--latest"]], [["no-deps@^1.0.0"]], [["no-deps@latest"]]])(
"a file: entry is kept as written by bun update %j",
async args => {
const dir = await setup({
"package.json": root({ dependencies: { "no-deps": "file:./local-no-deps" } }),
"local-no-deps/package.json": { name: "no-deps", version: "1.0.0" },
});
const [pkgBefore, lockBefore] = await Promise.all([pkgText(dir), lockText(dir)]);
await run(dir, "update", ...args);
expect(await pkgText(dir)).toBe(pkgBefore);
expect(await lockText(dir)).toBe(lockBefore);
expect(await installed(dir, "no-deps")).toMatchObject({ version: "1.0.0" });
await expectInSync(dir);
},
);

test("bun update <workspace member> from a member declaring it keeps the entry as written", async () => {
const dir = await setup(WORKSPACES({}, { pkg1: {}, pkg2: { dependencies: { pkg1: "workspace:~" } } }));
const [pkgBefore, lockBefore] = await Promise.all([pkgText(dir, PKG2), lockText(dir)]);
await runIn(dir, PKG2, "update", "pkg1");
expect(await pkgText(dir, PKG2)).toBe(pkgBefore);
expect(await lockText(dir)).toBe(lockBefore);
await expectInSync(dir, ["", PKG1, PKG2]);
});

test("bun update <workspace member> -r keeps every workspace's entry as written", async () => {
const dir = await setup(
WORKSPACES(
{ dependencies: { pkg1: "workspace:^" } },
{ pkg1: {}, pkg2: { dependencies: { pkg1: "workspace:1.0.0" } } },
),
);
const [rootBefore, pkg2Before, lockBefore] = await Promise.all([pkgText(dir), pkgText(dir, PKG2), lockText(dir)]);
await run(dir, "update", "pkg1", "-r");
expect(await pkgText(dir)).toBe(rootBefore);
expect(await pkgText(dir, PKG2)).toBe(pkg2Before);
expect(await lockText(dir)).toBe(lockBefore);
await expectInSync(dir, ["", PKG1, PKG2]);
});

test("bun update <workspace member> does not add it to a root that does not declare it", async () => {
const dir = await setup(MONOREPO());
const [pkgBefore, lockBefore] = await Promise.all([pkgText(dir), lockText(dir)]);
await run(dir, "update", "pkg1");
expect(await pkgText(dir)).toBe(pkgBefore);
expect(await lockText(dir)).toBe(lockBefore);
});

test.each([[[]], [["--latest"]]])("bun update %j leaves folder, tarball and workspace literals alone", async args => {
const dependencies = {
"no-deps": "^1.0.0",
Expand Down
Loading