Conversation
… concurrently setup() installs every distinct single-package.json project once, from verdaccio, and hands each test a copy with bun.lock re-pointed at the test's registry; setupVulnerableADep becomes one two-step template. A new test checks that such a copy is identical to installing through the test's registry. doAuditTest registers concurrent tests against a per-case fixture registry that records the bulk request bodies, and every case asserts the full stdout, stderr and request bodies instead of substrings and file snapshots. bun processes spawned by the file: 512 -> 411 (bun install 256 -> 155).
|
Warning Review limit reached
Next review available in: 41 seconds 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 ignored due to path filters (1)
📒 Files selected for processing (1)
Comment |
|
Status: test-only change, ready for review (5 commits; the last four are review follow-ups: warm cache for the manifest-404 tests, retry when the cache write loses the race with exit, templates dir created before verdaccio starts, dispose-before-assert in that retry).
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a substantial test-infrastructure refactor (new memoized template-install layer shared across ~150 concurrent tests, assertion semantics changed for ~20 cases, snapshot file removed), so a human look would still be worthwhile.
What was reviewed:
installedProjectmemoization: the get→size-read→set path has noawait, so concurrent callers with the same or different keys can't race on the map or collide on template subdirectory indices.copyProject/setup()isolation: template's bunfig and cache are removed before copy, bun.lock URLs are rewritten to the per-test registry, and each copy gets its ownBUN_INSTALL_CACHE_DIR— copies share no writable state; the new equivalence test pins this.- Assertion changes: file snapshots replaced with inline snapshots /
toEqual(resolveBulkAdvisoryFixture(...)); every case now also asserts stderr and the exact bulk request bodies — checked that no assertion was silently weakened.
Extended reasoning...
Overview
Test-only change to test/cli/install/bun-audit.test.ts (+250/−100) plus deletion of the 436-line .snap file. Introduces installedProject() (memoized per-package.json-sequence installs into a shared templates tempDir, installed once from verdaccio) and copyProject() (per-test copy with bun.lock URL rewrite + fresh bunfig). setup() now routes single-package.json projects through the copy path and only workspace/scoped projects through a real per-test install. doAuditTest becomes test.concurrent with a per-test fixture registry that records request bodies; three neighboring tests are also made concurrent. File snapshots are replaced with inline snapshots and MS_REPORT, and every case now asserts stderr and the exact bulk request payload. setupVulnerableADep becomes a two-step template + copy. A new test proves a copied project is byte-identical to a fresh install through the same proxy registry.
Security risks
None. Test-only; no runtime, auth, or network-facing code touched. The registries are local Bun.serve({ port: 0 }) proxies in front of a local verdaccio.
Level of scrutiny
Medium. Nothing here ships to users, but the change rewires how ~150 tests obtain their fixtures and moves ~25 tests from serial to concurrent execution against a shared verdaccio and a shared installedProjects map. I traced the memoization for concurrency safety (no await between get, installedProjects.size read, and set, so it's atomic within the JS event loop) and the copy path for isolation (bunfig/cache stripped from templates, bun.lock rewritten, fresh bunfig + BUN_INSTALL_CACHE_DIR per copy). Both look correct. The assertion rewrites are strictly stronger (full stdout/stderr + request bodies vs. the old toContain/substring checks), and I didn't find any case where a load-bearing property of the old assertion was dropped.
Other factors
The PR description is thorough and the author added a self-test for the copy invariant, which is good. That said, this is a moderately large refactor with a new caching pattern and mixed wall-time numbers by the author's own measurement (some paired runs got slower under load). The design choice of copy-and-rewrite-lockfile vs. per-test install, the switch to test.concurrent for these cases, and whether the --json assertions now proving "output equals fixture response" instead of specific severity/count checks are the right shape are all judgment calls a maintainer should sign off on rather than an automated approval.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate flag: #33704 is a 53-file concurrency sweep whose change to this file is limited to |
|
Updated 2:48 PM PT - Aug 15th, 2026
✅ @robobun, your commit d4fcd069c987d621bbd2096cf9d40549682a88de passed in 🧪 To try this PR locally: bunx bun-pr 39034That installs a local version of the PR into your bun-39034 --bun |
audit fix turns the manifest cache off before it looks packages up, and the three tests that make the registry stop serving a manifest only prove that while the cache holds one; they now install through their own registry and check the cache before breaking it, instead of starting from a copied project whose cache is empty.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a substantial test-infrastructure refactor (memoized install templates with bun.lock rewriting, cold-cache copies replacing warm-cache installs, snapshot → inline assertion migration), a human look would still be worthwhile.
What was reviewed:
installedProjectmemoization is race-free (synchronous check-then-set before any await;installedProjects.sizeread and map insert happen in the same tick).- The three
denyManifeststests that relied on a warm manifest cache were correctly migrated tosetupWithCachedManifests; thedenyTarballstest is unaffected since the denied tarball (1.0.4) was never cached before either. - Assertions were strengthened, not weakened — the deleted
.snapcontent is fully covered by the new inline snapshots andMS_REPORT, plus new exact request-body checks.
Extended reasoning...
Overview
This PR refactors test/cli/install/bun-audit.test.ts (~400 lines changed) and deletes its 436-line .snap file. It introduces installedProject() to memoize installs of distinct single-package.json projects (installed once from verdaccio, then copied per-test with bun.lock URLs rewritten to the test's registry), converts doAuditTest and three neighbouring tests to test.concurrent with a per-test fixture registry that records request bodies, replaces all toMatchSnapshot calls with inline snapshots or exact toBe/toEqual, and adds setupWithCachedManifests for the three tests that need a warm manifest cache. No production code is touched.
Security risks
None. Test-only; no auth, crypto, or user-facing surface. All servers are local Bun.serve({ port: 0 }) proxies over the file's own verdaccio instance.
Level of scrutiny
Moderate. While test-only, the change alters what setup() produces: previously every test's directory had a warm .bun-cache from a real install; now most tests get a copied directory with no cache. This is a semantic difference that could silently weaken tests whose point was "audit fix ignores the cache" — the author identified the three denyManifests tests and moved them to setupWithCachedManifests (which asserts the manifest count in .bun-cache), and I checked that the remaining denyTarballs case is unaffected. The copy-equals-install assumption is pinned by a new test that diffs every file of a copy against a fresh install.
Other factors
- The
installedProjectmap usesinstalledProjects.sizeas the directory index and sets the map entry synchronously before awaiting, so concurrent callers with different keys cannot collide on the same index and same-key callers share one promise. join(templates, ...)wheretemplatesis aDisposableString(boxedString) works in Bun'spath.join— verified, and the pattern is already used throughout the file withtempDirresults.- Assertion coverage increased: every
doAuditTestcase now checks the exact bulk request body (including[]for the no-contact cases) and full stderr; the deleted snapshot content is byte-for-byte reproduced in the express@3 inline snapshot andMS_REPORT. The--jsoncases now assert equality against the fixture response object rather than a stored snapshot. - The local timing numbers in the description are mixed (some pairs slower), which the author attributes to host load; the spawn-count reduction (512→411, installs 256→155) is the concrete metric. A maintainer should confirm the CI ASAN lane actually improves.
- There's a flagged overlap with PR #33704; the author has stated a rebase plan if that lands first.
|
On the open question about the ASAN lane: measured from the job log timestamps around the file (header line to The second commit is a coverage fix that came out of review: the three |
…ace with exit bun install saves manifests from its thread pool without waiting for them before exiting, so asserting that the cache is warm right after an install flaked on Windows; install into a fresh directory again instead until the files are there.
|
One more commit (43470e2): in build #98329 the new warm-cache check failed once on Windows 2019 x64 and passed on retry. The cause is in |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a ~560-line refactor of the test setup infrastructure (memoized template installs + copy-and-rewrite in place of per-test installs, plus a retry loop for the manifest-cache write race), a human look would still be worthwhile to confirm the copy-based projects are equivalent to fresh installs for every way audit fix reads project state.
Checked: the removed module-level fixture server has no remaining references (each concurrent test now starts its own); installedProjects memoization is race-free (get/set is synchronous before any await); tempDir(prefix, path) supports the directory-copy overload used by copyProject; the bun.lock URL rewrite in copyProject matches the existing pattern in startRegistry's manifest proxy; the three denyManifests tests were correctly moved to setupWithCachedManifests so they still exercise the manifest-cache bypass.
Extended reasoning...
Overview
Test-only refactor of test/cli/install/bun-audit.test.ts (~560 lines changed) plus deletion of its 436-line .snap file. The change memoizes single-package.json project installs into a shared templates directory (installed once from verdaccio, then copied per test with bun.lock URLs rewritten to the test's registry), converts doAuditTest and three neighboring tests to test.concurrent with a per-test fixture registry, replaces five toMatchSnapshot assertions with inline snapshots / exact toBe/toEqual comparisons, and adds request-body assertions to every doAuditTest case. Three denyManifests tests move to a new setupWithCachedManifests helper that installs through the test's own registry and retries until the async manifest-cache write lands, restoring the warm-cache precondition that copied projects lack. A new self-test diffs a copied project against a fresh install byte-for-byte.
Security risks
None. Test-only; no production code, no network egress (verdaccio + local Bun.serve proxies), no new dependencies.
Level of scrutiny
Moderate-to-high. It touches no shipped code, but it rewires how ~150 existing tests obtain their fixture project. The repo's review rules are explicit that weakening or subtly changing what an existing test asserts is a merge blocker, and the first revision of this PR did exactly that for the three manifest-404 tests (fixed in the second commit after review). The author's verification is unusually thorough — spawn counts, CI wall-clock, mutation testing against audit_fix.rs, and a byte-level equivalence test — so I have reasonable confidence, but the copy-vs-install equivalence is the kind of assumption a maintainer who owns bun audit should sign off on.
Other factors
- The
installedProjectsmap is module-global mutable state shared across concurrent tests; it's safe because the get→set happens synchronously before the firstawait, so concurrent callers with the same key share one promise. setupWithCachedManifestshas a bounded retry (expect(attempt).toBeLessThan(5)) around a documented race insrc/install/npm.rs(save_async); this is a poll-for-condition, not a sleep, and disposes the failed dir between attempts.- Assertions are strictly strengthened everywhere I checked (substring → exact stdout/stderr, added
requestsbody checks, addedstderr= "" checks). The one place coverage could have regressed (warm cache) was caught and fixed. - No prior claude[bot] review on this PR; CI for the latest commit is still building (#98510).
- ~560 lines of non-mechanical test-infrastructure change is beyond what I'd approve without a human look.
|
Build #98510 (the current revision, with the retry) has |
…daccio If verdaccio fails to start, afterAll otherwise adds a TypeError about the unset templates handle on top of the real failure.
|
Measurements are now in the description in a form that separates what this PR changes from what it does not. Each lane's block for this file starts 3rd to 9th in its shard, while the runner is still starting its service containers, so 4 to 10s of every block passes before the first test finishes, and that window varies more between builds than the tests themselves take on the fast lanes (it is also why this PR's builds, which run modified files first, looked slower on a couple of lanes when comparing whole blocks). Splitting at the first progress dot: the tests-running part on x64-asan goes from 15.3s / 14.7s (unmodified builds) to 11.8s / 11.1s here, windows 11 aarch64 from about 10s to 8.7s, and the ubuntu and windows 2019 lanes, where the tests take 2s and 4.5s, stay within their noise. A release build of this branch runs the whole file 3.5s -> 3.3s on Linux and 3.0s -> 2.7s on Windows Server 2019 in interleaved rounds, so there is no platform where the copies cost more than the installs they replace (a copy is about 0.7ms in a release build). d4fcd06 takes the dispose-before-assert suggestion from the line comment. |
There was a problem hiding this comment.
Thanks for addressing the dispose-before-assert nit in setupWithCachedManifests (d4fcd06). I re-reviewed and found no further issues. Given the scope — new template/copy install memoization shared across ~150 setup calls, the concurrent conversion of 25 tests, and the wholesale snapshot→exact-assertion rewrite — a human pass on the infrastructure design would still be worthwhile.
What was reviewed:
installedProject/copyProjectmemoization: template dirs live under onetempDirdisposed inafterAll; the map key is the full step list; bun.lock rewrite covers the only registry reference and the new self-test pins that.- Concurrency safety: each
doAuditTestnow spawns its own fixture registry and temp dir, so nothing writable is shared; verdaccio and the templates are read-only after install. setupWithCachedManifests: the threedenyManifeststests keep their warm-cache precondition; retry loop now disposes before the bound check.- Snapshot deletions: each removed
toMatchSnapshotis replaced by an equal-or-stronger exact assertion (full stdout/stderr + request bodies), not weakened.
Extended reasoning...
Overview
Test-only refactor of test/cli/install/bun-audit.test.ts (~420 additions, ~580 deletions including the deleted .snap file). Three orthogonal changes: (1) setup() now memoizes single-package.json installs into a shared templates directory and hands each test a copy with bun.lock re-pointed at its own registry, cutting bun install spawns from 256 to ~160; (2) the 22 doAuditTest cases and 3 neighbors move from test() to test.concurrent, each with its own fixture registry; (3) five toMatchSnapshot calls become inline snapshots or exact toBe/toEqual assertions on full stdout/stderr and recorded request bodies, deleting the 436-line snapshot file. A new setupWithCachedManifests helper preserves the warm-cache precondition for the three denyManifests tests, with a bounded retry for the save_async race the author observed on Windows.
Security risks
None. Test code only; no production paths, no auth/crypto/permissions touched. The registries are local Bun.serve({port: 0}) proxies in front of the file-shared verdaccio.
Level of scrutiny
Medium-high for a test change. It is test-only, so correctness risk to shipped behavior is nil, but the refactor introduces non-trivial shared infrastructure (a module-level Map of install promises, a shared templates temp dir, a copy-and-rewrite step that must be equivalent to a fresh install for every downstream assertion to remain meaningful). The author added a dedicated self-test proving copy ≡ install byte-for-byte and verified the denyManifests coverage by locally reverting audit_fix.rs's cache bypass — both are the right kind of evidence. Still, the assertion rewrite touches ~20 test bodies and the concurrency conversion changes execution ordering, which is the sort of thing a maintainer familiar with this file's flake history should eyeball.
Other factors
My prior inline nit (dispose the cold retry dir before the bound assertion) was addressed in d4fcd06. The bug-hunting system found nothing this run. The PR description is unusually thorough on methodology (spawn counts via preload, CI timings from job logs, the mutation test on audit_fix.rs). The one design choice I'd want a maintainer to confirm is that memoizing installs by JSON.stringify(steps) and copying via tempDir(prefix, dir) is the pattern they want for this file going forward — it is sound, but it is new infrastructure that future test authors will build on.
|
For whoever picks this up, the one design question left open by the reviews is whether the install templates ( |
|
doesn't move numbers enough |
2 participants
Problem
test/cli/install/bun-audit.test.tsis slow on the ASAN lane. Measured from the job log timestamps, the file's block took 23.2s, 21.8s and 20.4s on debian 13 x64-asan in three builds of unmodified code (#97275, #98555, #98543; the handoff that prompted this quoted 28s); the file sits 3rd to 9th in its shard on every lane, so 4 to 10s of each block is spent before the first test finishes, while the runner is still bringing up its service containers. The other lanes spend 2 to 10s actually running its tests (ubuntu x64 about 2s, windows 2019 x64 about 4.5s, windows 11 aarch64 about 10s). Under ASAN every spawnedbunis the expensive unit.bun install: 147setup()calls installed only 63 distinct projects, andsetupVulnerableADepinstalled the same two package.jsons for each of its 28 callers.doAuditTestcases and the 3 tests next to them were plaintest(), so they ran one at a time, whilebun testruns the file's other tests 5 wide (the ASAN default for--max-concurrency,src/options_types/context.rs:506; 20 elsewhere). A serial test costs as much wall time as 5 concurrent ones.not.toContain("error"),toContain("vulnerabilities")), and 5 relied on file snapshots.Fix
setup()installs each distinct single-package.json project once (installedProject, memoized by package.json text, installed straight from verdaccio) and gives every test its own copy throughtempDir(prefix, dir), the same copy the static fixtures use. The copy gets its bun.lock tarball URLs re-pointed at the test's registry and a fresh bunfig;setupVulnerableADepis one two-step template. Workspace and scoped-registry projects (21 calls, 14 distinct shapes) still install per test.setup() > a copied project is what installing through the test's registry producesreads every file of a copy and of a fresh install through the same proxy registry and requires the two sets to be equal. Installs never hit the bulk endpoint, sobulkHitsandbulkBodiesassertions mean what they did before.denyManifeststests only prove thataudit fixfetches manifests afresh (it turns the manifest cache off,src/install/audit_fix.rs) while the cache holds a manifest that would have answered. Those three now go throughsetupWithCachedManifests, which installs through the test's own registry and hands the project over only once.bun-cacheholds one.npmmanifest per dependency.bun installwrites those files from its thread pool without waiting for them at exit (save_asyncinsrc/install/npm.rs), so an install occasionally ends without them (asserting on them flaked once on Windows in build #98329); a project that came out cold is thrown away and installed again, at most a few times. Checked by deleting the twoenable.set(..., false)lines in audit_fix.rs: main's versions of the three tests fail, the copy-based versions from the first revision of this PR pass (the coverage gap review caught), and the current versions fail again, withaudit fixupgrading a-dep from the stale cached manifest. ThedenyTarballstest is unaffected: the tarball it denies was never in the cache.doAuditTestregisterstest.concurrentand starts a fixture registry per case that records the request bodies; the two scoped-registry tests and the double-quote test are concurrent too. The tests share nothing writable: verdaccio is read only, and each test has its own directory, cache and registry.doAuditTestcase now asserts the full normalized stdout and stderr (inline snapshots for the express@3 report, the--audit-level criticalreport and the two error cases;MS_REPORTfor the four cases that print the ms report) and the exact bulk request bodies:[]where the registry must not be contacted (no package.json, no lockfile, invalid--audit-level), the exact name/version map otherwise (the mix fixture proves is-number is sent and not printed, the workspace case proves the workspace package is not sent, the scoped-registry cases prove the default registry is asked about nothing). The--jsoncases compare the parsed document against the fixture response (MS_ADVISORIES) with onetoEqual.__snapshots__/bun-audit.test.ts.snapis deleted; nothing usestoMatchSnapshotany more (it is also rejected inside concurrent tests).Bun.spawn: 512 -> 416 bun processes,bun install256 -> 160 (one more per cold-cache retry, which is rare). What is left: ~50 template installs, 21 workspace/scoped setups, 28 second-stepreinstall()s (almost all distinct), the 6 warm-cache installs above, ~40--frozen-lockfileinstalls that check the lockfileaudit fixwrote (kept on purpose), and the few tests that runbun installthemselves to pass linker flags.bun audit(96) andbun audit fix(160) spawns are unchanged: every repeated invocation checks a different flag, mode or cwd, so none was dropped. Theexpect()count goes 2993 -> 2360 becauserunBunInstallmakes about six calls per install.bun bd test --timeout=90000 test/cli/install/bun-audit.test.ts(the flag mirrors the runner's ASAN per-test timeout): 183 pass, 182 before plus the new one.Local before/after runs
Release build of this branch, whole file, interleaved rounds (
bun testruns it 20 wide there): Linux, 12 cores, load average about 38: 2.91 / 3.69 / 4.03 / 3.45s -> 2.86 / 3.05 / 3.65 / 3.53s. Windows Server 2019 x64, 16 logical CPUs, with the canary on PATH: 3.00 / 2.97 / 3.00s -> 2.65 / 2.67 / 2.66s (182 -> 183 tests passing in every run). Copying a template costs 0.7ms per project in a release build.Debug + ASAN build (
bun bd, 5 wide like the CI lane, but the copies cost about 30ms each there and the box's load average was 60 to 250, so wall time mostly measures the host): CPU of the whole process tree over six interleaved pairs 155 / 172 / 175 / 145 / 156 / 159s -> 146 / 139 / 146 / 144 / 127 / 138s; with--max-concurrency=20the two valid pairs were 28.6s -> 23.1s and 30.3s -> 30.7s wall, 126 / 138s -> 105 / 113s CPU.Background
bun auditposts{ name: [installed versions] }to the registry's bulk advisory endpoint and prints what comes back;bun audit fixalso fetches manifests, rewrites bun.lock (and pins in package.json) and runs an install. Each test in this file therefore needs an installed project plus a registry, which is aBun.serveproxy in front of one verdaccio shared by the whole file; the proxy answers the bulk endpoint itself.copyProjectdoes and what the new test pins down.<name hash>-<registry URL hash>.npmfiles (src/install/npm.rs,manifest_file_name). The registry hash is why copying a template's cache could not have stood in for a warm cache either: a copy uses a different registry URL, so its lookups would never hit the template's files.audit fixdisables this cache so that a fix is planned from what the registry says now.audit-fixtures.jsonholds the real npm responses for the four checked-in projects underregistry/fixtures/audit, keyed by request body;startFixtureRegistryserves those, which is why the--jsoncases can assert against the fixture response directly.[stamp-90s] gate passed · iteration 2 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file