[robobun]

Problem

• test/cli/install/bun-add-catalog.test.ts takes 14s on debian 13 x64-asan (build 104578): half is the verdaccio start as the first registry user in its shard, half is 272 bun children run 5 at a time (the ASAN --max-concurrency default, src/options_types/context.rs:506). It is already describe.concurrent throughout.
• Most of the 149 cases checked stderr with not.toContain("error:") and one or two fields of package.json and bun.lock. A new note, a stale row or a moved resolution passed.

Fix

• spawnBun strips the two progress lines. Success cases then compare stderr exactly: Saved lockfile, nothing, or the note lines before it. Error cases compare the whole message.
• expectFiles (89 sites) compares every package.json and derives the bun.lock workspaces rows and catalog objects from them. Each case adds the resolution of every dependency, so a nested second version (pkg2/no-deps) is visible.
• installSavesNothing also pins the (no changes) summary. Tarball cases use installKeepsLockfile: every install re-extracts a tarball dependency (bun install always reinstalls vendored packages #8260, fix in install: skip reinstalling file:/URL tarball deps on second install #35576). The 9 --frozen-lockfile runs right after such an install are gone: it already showed bun.lock is in step. 272 spawns become 263.
• Verified: bun bd test test/cli/install/bun-add-catalog.test.ts, 149 pass in 5 runs. Wall time is unchanged within noise (Notes): no speedup is claimed.

Background

• bun.lock repeats each member's dependency rows under workspaces, the root's catalog objects, and one packages row per resolved package, keyed by its hoisted path (pkg2/no-deps for a second version nested under a workspace).
• bun install prints Saved lockfile only when the bytes of bun.lock changed.

Notes

CI timeline (build 104578, shard 01a03209-fdf7-4f04-a162-576b9c284b16, position 7/289). Header at +0.07s, first dots at +7.12s, summary at +13.95s. The coordinator: minio/mysql_tls/postgres_tls/mysql_plain ready lines land inside the first window. Across the 20 x64-asan shards the first verdaccio file has 5.2s to 10.5s before its first dot, later ones 2.4s to 3.0s. Spawn arithmetic: 272 children / 5 in flight = 54 rounds, 6.8s / 54 = 125ms per child, which is about what a release ASAN bun add costs.

Local measurements (debug ASAN, 16 cores, verdaccio under the release bun). Old and new alternated on the same machine: 14.39s/15.12s against 14.33s/14.99s. registry.start() 1.1s. A single bun add in isolation is 190ms to 210ms cold, 150ms with a warm cache, bun --version 105ms, so process start is most of a spawn. --max-concurrency=200 finishes in 9.5s but times out most cases at 5s: the work is CPU bound. Sum of per-test durations 53.9s with 6 children in flight at most. Each spawn removed saves about 25ms of CI wall time, so the 9 removed frozen runs are about 0.2s. Removing the pre-installs or the post-install checks would cut more, but each of them is a scenario (an existing lockfile, or a lockfile a later install accepts), so none was removed.

What the stricter assertions surfaced. (1) The note: catalog entry ... changed line, printed whenever an explicit version replaces an entry; with (also used by <member>) when other members reference it. (2) A plain bun install after any tarball --catalog add prints 1 package installed every time, in a plain project too (probed with a non-workspace package.json): verify_package_json_name_and_version compares the installed version with the formatted resolution, which is the URL (src/install/PackageInstaller.rs, resolution.fmt). That is #8260; #35576 fixes it, after which the 11 installKeepsLockfile sites can become installSavesNothing. (3) bun update --latest on a catalogs.default entry prints only 1 package installed; #38763 adds the update row and will update that line. (4) Hoisting after an add: the added tarball or range is hoisted and the version another member already had moves under that member (pkg2/no-deps), also for --only-missing.

Assertion changes. expectOk (not.toContain("error:"), not.toContain("panic:")) is replaced by expectSaved (108 sites) and expectUnchanged (8 sites: re-runs and adds that find the entry and reference already in place). Notes are pinned with their order: pkg1 before pkg2 in the two-note case. The --dry-run listing, the --lockfile-only summary (Saved bun.lock (3 packages)), --no-save (installed no-deps@2.0.0 / 2 packages installed), bun remove (done while pkg2 still uses it, 1 package removed after), bun update --latest and --silent (empty stdout and stderr) pin stdout. The frozen failure pins the error and both notes. The both-catalogs error pins the code frame shape and the at .../package.json:line:col line. The expect() count drops from 1856 to 1274 because one toStrictEqual on the lockfile view replaces several per-field checks and the harness runBunInstall (6 expects per call) is no longer used.

Left minimal on purpose. "alias@npm:pkg is written verbatim, like plain add" (#39774 rewrites its expectations) and "name only in peerDependencies, adding with --dev, matches plain add" (#36284 does the same) keep their original shape; only expectOk became expectSaved.

Also run. bunx prettier --check clean. tsc -p test/tsconfig.json reports nothing for this file. Windows x64 (Server 2019, debug build of this branch): 149 pass in 9.3s. The only platform difference found was the --dry-run listing, which prints the workspace path with a backslash; the test compares it with forward slashes.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)

• test/cli/install/bun-add-catalog.test.ts

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 72ed5dc1-3e88-4c94-bce6-5fcb0395ba32

📥 Commits

Reviewing files that changed from the base of the PR and between 4d35aad and 9510c29.

📒 Files selected for processing (1)

• test/cli/install/bun-add-catalog.test.ts

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

[robobun]

^{Updated 4:28 AM PT - Aug 24th, 2026}

✅ @robobun, your commit 9510c292bb9b9e2e609ba898e1c674c9264a7293 passed in Build #104829! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 40327

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

bun-40327 --bun

[robobun]

Status: the rewrite passes locally, bun bd test test/cli/install/bun-add-catalog.test.ts (debug ASAN): 149 pass in 5 runs. On Windows x64 (Server 2019, debug build of this branch): 149 pass in 9.3s. The first Windows run found one platform difference, the --dry-run listing prints the workspace path with a backslash; 9510c29 compares it with forward slashes.

How the time was measured: the x64-asan log of build 104578 (shard position 7/289) shows the bun test header at +0.07s, the first test result at +7.12s and the summary at +13.95s. The first window is the verdaccio start while the job still boots its service containers. The second window is 272 child processes run 5 at a time. An instrumented copy of the file counted the spawns (272 before, 263 after) and the peak number of children in flight (5).

[claude]

I reviewed this PR and didn't find any bugs. Because it's a large test refactor (~800 lines across 149 cases) that both strengthens assertions and removes 9 --frozen-lockfile spawns as redundant, and the Windows CI result mentioned in the description is still pending, a human look would still be worthwhile.

What was reviewed:

• expectFiles/lockView/catalogsOf/lockRow derive the expected bun.lock shape from the package.json inputs; checked that the workspace-vs-top-level catalog precedence and empty-object filtering match how the tests use them.
• The removed frozen() calls each follow an installSavesNothing/installKeepsLockfile that already proves bun.lock is accepted unchanged; frozenOk() is kept where no such install precedes it.
• PROGRESS_LINES stripping and summary() header/timing regexes — anchored per-line so partial matches can't swallow real output; --dry-run stdout normalizes backslashes for Windows.

Extended reasoning...

Overview

This PR rewrites the assertion helpers in test/cli/install/bun-add-catalog.test.ts (test-only, no runtime code). It replaces expectOk (loose not.toContain("error:")) with expectSaved/expectUnchanged (exact stderr match after stripping the two progress lines), and replaces per-field lockfile checks with expectFiles, which compares every package.json in full and derives the expected workspaces/catalog/catalogs/packages sections of bun.lock from those files plus a caller-supplied resolution map. It also pins several stdout summaries (dry-run, lockfile-only, no-save, remove, update --latest) and exact error messages. Nine --frozen-lockfile spawns that immediately followed a no-op install are removed as redundant; the remaining ones are wrapped in frozenOk().

Security risks

None. Test-only change to a CLI install test file; no auth, crypto, or untrusted-input parsing is touched.

Level of scrutiny

Medium. It's test-only, so it can't break users, and the direction is toward stricter assertions (exact stderr, full lockfile structure) — which is exactly what REVIEW.md asks for. But the diff is large (~800 lines across 149 cases), it deliberately removes some existing checks with a stated justification, and it introduces non-trivial helper logic (catalogsOf picks between workspaces.catalog and top-level catalog; lockRow projects the six keys bun.lock keeps per workspace). REVIEW.md explicitly flags "never silently weaken, skip, or delete an existing test or safety net" — the deletions here are stated and justified, but a maintainer should confirm they agree the installSavesNothing check genuinely subsumes the removed --frozen-lockfile runs.

Other factors

• The author's own status comment says a Windows x64 run is still in progress. Several new exact-string assertions (the dry-run listing, the frozen-lockfile error notes, the code-frame regex) are the kind that can differ by platform; the second commit already handles the dry-run path separator, but confirming the Windows shard is green before merge seems prudent.
• The removed runBunInstall harness helper (6 built-in expects per call) is replaced by direct spawnBun + explicit checks. That's fine — the new checks are stricter — but it's a pattern change worth a maintainer's nod.
• No prior human or bot review comments on this PR beyond CI status. The bug-hunting system found nothing.

[robobun]

The Windows x64 run is in now: 149 pass in 9.3s on a debug build of this branch (Server 2019). The one platform difference it found, the backslash in the --dry-run listing, is what 9510c29 normalizes. The PR body and the status comment above carry the result.

[robobun]

Closing after a self-review of the premise. #39549 did the same work on this file six days ago, measured no gain in CI, and closed with the conclusion that the assertion rewrite alone is not worth a diff of this size without a maintainer asking for it. Nothing has changed since, so the same conclusion applies here.

The numbers:

• The file's 14s on debian 13 x64-asan (build 104578) is 7.1s of verdaccio start, as the first registry user in its shard while the job still boots its service containers, plus 272 child processes run 5 at a time (the ASAN --max-concurrency default). This PR removes 9 of the 272, about 0.2s. Alternating local runs: 14.39s and 15.12s before, 14.33s and 14.99s after.
• In that build the shard that runs this file finished 163s before the slowest shard of the lane (310s against 471s). A faster version of this file does not change the lane's wall time.
• The levers that would change it live outside this file: the verdaccio start (test: replace verdaccio and the mock npm servers with an in-process spec-compliant registry #33115, test(harness): let verdaccio pick its own port and fail start() when it exits early #38891) and the ASAN concurrency cap (src/options_types/context.rs:506).

What is left is the assertion rewrite. The reusable part of it, stripping the two progress lines and comparing stderr exactly, belongs in runBunInstall in test/harness.ts, where 25 files would pick it up at once. #39746 and #40095 each carry their own copy of that normalizer too. That is a separate change.

For the record, three things the exact assertions showed:

• bun add --catalog prints note: catalog entry <name> changed from "<old>" to "<new>" (also used by <member>) when an explicit version replaces an entry. No test asserts that line today.
• A plain bun install after any tarball --catalog add re-extracts the tarball every time. This is bun install always reinstalls vendored packages #8260, not a catalog issue, and install: skip reinstalling file:/URL tarball deps on second install #35576 fixes it.
• bun update --latest prints no update row for a catalog entry. install: print the update row for catalog entries moved by bun update #38763 adds it.

The branch stays available for reference.