install: move existing dependency when bun add is given an explicit group flag

[robobun]

Problem

• bun add -d <pkg> (also --optional / --peer, and bun install -d <pkg>) leaves a package that is already listed in another dependency group where it is, so the only way to move a package between groups is bun remove followed by bun add.
• PackageJSONEditor::edit scans the four groups for an existing entry and, when it finds one in a group other than the target, updates that entry's value in place instead of moving it.

Fixes #5714.
Fixes #4852.

Fix

• When a group flag was given (dependency_list is not dependencies) and the package is found in another group, remove that entry and let the normal add path write it into the target group. The scan visits every group in that mode so a stale copy in a later group is cleaned up too, and a removal marks the file as changed.
• Removal is an ordered remove of the entry (and of the group itself once it is empty), so the remaining entries and the other root keys of package.json keep their order.
• Exceptions, matching npm (@npmcli/arborist add-rm-pkg-deps.js) and pnpm:
  • a peerDependencies entry is left alone when adding to another group;
  • when adding to peerDependencies, an existing devDependencies entry is kept and rewritten to the new peer range.
• A bare bun add <pkg> keeps the existing placement, and --only-missing still leaves existing entries untouched. bun update and bun link never set a group flag, so they are unaffected. --filter and --catalog adds go through the same editor and pick the new behavior up.
• Out of scope: non-aliased URL/path positionals (git, tarball, folder, link) are matched by value, not by name, and keep their current in-place behavior.
• Verified:
  • test/cli/install/bun-add.test.ts: 14 cases compare the printed package.json text and cover every group pair, removal from the middle of a group, unranked root keys around an emptied group, several packages in one command, both peer/dev cases with a range that changes, and the no-flag / --only-missing opt-outs; the 12 that exercise the move fail on the released bun. "should let you add the same package twice" asserted the old behavior and now expects the move.
  • test/cli/install/bun-add-filter.test.ts and bun-add-catalog.test.ts: the three cases that pinned update-in-place now expect the move; the filtered --peer one keeps its checks that bun.lock matches the edited file, --frozen-lockfile passes and a second install saves nothing.
  • bun-add, bun-add-filter and bun-add-catalog pass in full on the rebased branch; swapping the ordered removes back to swap_remove + re-sort fails the two ordering tests.

Background

• edit() runs twice per bun add: once before the install with the literal the user typed, and once after the install on a re-parsed package.json with the resolved range. The removal happens on the first pass, so the install already sees the moved entry and bun.lock is built from it; the second pass only rewrites version literals, which package_json_write_back::sync_lockfile copies into the lockfile.
• Each UpdateRequest carries an e_string pointer to the package.json value that receives the final version string. For the peer/dev case the existing dev value is recorded during the scan and copied from the peer value after the version strings have been written.

---

no test proof · iteration 10 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-add-filter.test.ts test/cli/install/bun-add.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 35d88b4c-bab2-44cb-b599-543c9e0fad61

📥 Commits

Reviewing files that changed from the base of the PR and between 3f8b7e1 and 47fcb62.

📒 Files selected for processing (4)

• src/install/PackageManager/PackageJSONEditor.rs
• test/cli/install/bun-add-catalog.test.ts
• test/cli/install/bun-add-filter.test.ts
• test/cli/install/bun-add.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 11:05 PM PT - Aug 14th, 2026}

❌ @robobun, your commit 47fcb62 has some failures in Build #97280 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 36284

That installs a local version of the PR into your bun-36284 executable, so you can run:

bun-36284 --bun

[github-actions]

Found 1 issue this PR may fix:

1. Bun install --dev doesn't change dependency type of preexisting dependency #4852 - Reports the same bug: bun install --dev doesn't change the dependency type of a preexisting dependency, which this PR's PackageJSONEditor changes directly fix.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #4852

🤖 Generated with Claude Code

[claude]

Beyond the inline nit, I also checked: (1) query.i stays valid across the nested list_obj.properties.swap_remove since root properties aren't touched until after, and each 'dependency_group iteration re-fetches query fresh so the root swap_remove doesn't corrupt later lookups; (2) the empty-group removal (swap_remove + package_json_sort, else alphabetize_properties) mirrors the existing bun remove path in updatePackageJSONAndInstall.rs:288-296; (3) the pre-existing "should add dependency alongside peerDependencies" test is unaffected because move_to_target is false with no group flag.

Extended reasoning...

This PR changes user-facing bun add --dev|--optional|--peer semantics (moving an existing entry between dependency groups, with peer-coexistence rules) and updates an existing test's assertion to match the new behavior. The implementation looks correct and well-tested, and the empty-group cleanup follows the established bun remove pattern. The one nit found (order-sensitivity when the package already sits in both the target group and a later-scanned group) is a narrow non-regression. Leaving final sign-off on the design rules to a maintainer.

[robobun]

Final CI state for the rebased head (47fcb62, build 97280): 177 of 179 Buildkite jobs passed and none failed. The other two, both darwin 14 aarch64 test lanes, expired without ever being picked up by an agent, so they did not run; the same lanes were stuck on the previous builds of this PR as well. The six annotated tests are tagged flaky and each passed alone or on retry (none under test/cli/install). GitHub checks (mordant, clippy, Format, source lints) are green. Nothing further planned from my side; whether to merge over the two expired macOS lanes or re-run them is your call.

[robobun]

Rebased onto main (d2bc8ad) to resolve merge conflict with the keep_catalog_reference change in PackageJSONEditor::edit. Squashed to a single commit.

Verified locally:

• bun bd test test/cli/install/bun-add.test.ts: 65 pass, 0 fail
• bun bd test test/cli/install/bun-update.test.ts: 6 pass, 0 fail (catalog path intact)
• USE_SYSTEM_BUN=1 bun test bun-add.test.ts -t 'should move existing dependency': 9 fail (expected, confirms fail-before)
• End-to-end repro from Bun install --dev doesn't change dependency type of preexisting dependency #4852 now produces devDependencies after bun install <pkg> --dev.

[claude]

No issues found after the rebase; my earlier two inline comments were addressed. Deferring to a human because this is a user-facing behavior change to bun add with a few policy choices (peer↔dev/optional coexistence rules, URL/path positionals left out of scope) and it flips an existing test's expected output — worth a maintainer sign-off on the semantics.

What was reviewed:

• Traced move_to_target through both edit() calls (before/after install, fresh re-parse + e_string reset in lockfile.rs) — remaining/replacing bookkeeping stays balanced on the after-install pass.
• Checked the swap_remove + package_json_sort/alphabetize_properties path against the bun remove pattern; empty-group removal and sibling-entry preservation are covered by tests.
• Confirmed Subcommand::Update and the keep_catalog_reference path from the recent merge are unaffected (move_to_target is Add-only).

Extended reasoning...

Overview

Changes PackageJSONEditor::edit so that bun add --dev|--optional|--peer <pkg> moves an existing entry from another dependency group into the target group instead of updating it in place. Adds a move_to_target flag gated on Subcommand::Add + an explicit group flag + not --only-missing. When the entry is found in a non-target group, it's swap_removed (with the group itself removed if emptied, matching bun remove), and the loop continues so the normal add path writes it into the target group. peerDependencies is never auto-removed; dev/optional are kept when the target is peer. 11 new tests in bun-add.test.ts plus one existing test ("should let you add the same package twice") updated to assert the new behavior.

Security risks

None. This is package.json AST editing over an already-parsed object; no new untrusted-input parsing, no filesystem/network surface.

Level of scrutiny

Medium-high. edit() is core package.json mutation logic called on both the before-install and after-install passes with subtle remaining/replacing/e_string bookkeeping. I traced the after-install pass (fresh re-parse via new_package_json_source, e_string reset at lockfile.rs:828) with move_to_target active: the target-group hit now takes replacing += 1 then continue instead of break, but remaining - replacing still nets to 0 extra slots and the while k loop overwrites the existing key correctly. The peer-coexistence branch (remove = false) also behaves on both passes.

Other factors

• Behavior/API change: this changes what bun add -d <existing> does and modifies an existing test that asserted the old behavior. The peer-coexistence rules and the URL/path-positional exclusion (noted in the PR description after my earlier comment) are policy choices a maintainer should confirm.
• Prior review: my two earlier inline comments (order-sensitivity on FOUR when already in target group; URL/path fallback not covered) were addressed — the first with a fix + test, the second by scoping it out in the description.
• Tests: 11 new subprocess tests cover the group-move matrix, peer coexistence, sibling-entry preservation, and the no-flag / --only-missing opt-outs. Author verified fail-before with USE_SYSTEM_BUN=1 and that bun-update.test.ts (catalog path) still passes after the rebase.
• CI: remaining failures on the last build are unrelated infra/flakes per the author's note; bun-add.test.ts passed on every lane that ran.

[alii]

Behavior is right, but this needs a rebase and a few fixes before it can land.

• Does not compile on current main: options.update.{development,optional,peer} were removed in #36764. Derive the predicate from dependency_list instead.
• --peer target keeps optionalDependencies and leaves the dev range stale; npm and pnpm both remove optional and both update dev, so the parity claim in the body is wrong for that row.
• swap_remove + package_json_sort re-sorts unrelated root keys of package.json on bun add -d; use an ordered remove.
• The new tests compare parsed JSON so none of the ordering or index logic is covered.

[robobun]

Thanks, all four addressed in b7bb4e8 (rebased onto main):

• the predicate is derived from dependency_list; the removed options.update fields and the Subcommand::Add check are gone
• --peer now removes optionalDependencies and rewrites an existing devDependencies entry to the new peer range, as npm and pnpm do; only a peerDependencies entry is still left alone when adding elsewhere
• ordered removes, no re-sorting of the group or of the root keys
• tests compare the printed package.json text and cover the middle-of-group, unranked-root-key, multi-package and peer/dev range cases

PR description updated to match. bun-add.test.ts passes in full locally (68 tests); the 12 move cases fail on the released build.

[alii]

Checked the new head against the four points from last time: the predicate now comes from dependency_list, --peer removes optional and syncs the dev range after the versions are written (matches arborist add-rm-pkg-deps.js), both removals are ordered Vec::remove with no re-sort, and the tests compare printed text with fixtures that distinguish the ordered group remove and the root re-sort. Neither touched file has moved on main since the merge base. Looks ready to merge from this side; one test fixture nit below, not blocking. Test run not repeated here, so pass status is as reported.

[robobun]

Thanks. Fixture nit taken in ae82c07 (dependencies moved ahead of the unranked keys in the initial object; expected output unchanged). No source changes since b7bb4e8.

[alii]

@robobun this conflicts with main now, please rebase and get a fresh CI run so it can be merged.

[robobun]

Rebased onto main as 47fcb62 (single commit, GitHub reports it mergeable). Two things came up in the rebase:

• edit() now returns a changed flag on main, so a removal (and a dev entry actually rewritten by --peer) sets it.
• main added tests that pinned the update-in-place behavior for the shared --filter and --catalog paths: -F alias with --dev and --exact, a name declared in two groups stays in sync with bun.lock after a filtered --peer add (its bun.lock / --frozen-lockfile / second-install checks are unchanged and pass with the move) and name only in peerDependencies, adding with --dev, matches plain add. They now expect the move; no other expectations changed.

bun-add, bun-add-filter and bun-add-catalog pass in full locally on the rebased build. PR description updated.