Skip to content

test(install): run security-scanner matrix concurrently - #36215

Closed
robobun wants to merge 5 commits into
mainfrom
claude/farm/26427b38/security-scanner-matrix-concurrent
Closed

robobun wants to merge 5 commits into
mainfrom
claude/farm/26427b38/security-scanner-matrix-concurrent

Conversation

@robobun

@robobun robobun commented Jul 28, 2026 •

Copy link
Copy Markdown
Collaborator

What

Run the security-scanner matrix tests concurrently instead of serially. Applies to both bun-security-scanner-matrix-with-node-modules.test.ts and bun-security-scanner-matrix-without-node-modules.test.ts via the shared runner.

Why it was slow

The runner generates 720 cases per file and ran them one at a time because:

  1. SimpleRegistry held the request log and test-security-scanner tarball variant as process-global mutable state, so two cases could not share the server.
  2. The registry-request assertion used toMatchSnapshot, which bun:test rejects inside concurrent tests.
  3. The CI-sampling skips used plain test.skip, and a non-concurrent skip between two test.concurrent calls ends the concurrent batch, serializing the rest of the file.

Fix

  • SimpleRegistry.newSession(behavior) now hands out a per-test session with its own URL prefix (/_s/<id>/…), request log, and scanner-tarball variant. One Bun.serve({ port: 0 }) still backs the whole file; sessions just namespace state. The legacy global path stays intact for bun-security-scanner-workspaces.test.ts.
  • The two .snap files (5500+ lines each, 1440 entries, 14 unique values between them) are folded into bun-security-scanner-matrix-expected.json, keyed by the zero-padded test id, and asserted with toEqual. Regenerate with SCANNER_TEST_UPDATE_EXPECTED=1 bun bd test <file>; the fixture preserves whatever a partial run did not touch.
  • test → test.concurrent, test.skip → test.concurrent.skip.
  • Per-test timeout of 30s: the npm-scanner + remove/uninstall cases spawn two bun install-class subprocesses and were already close to the 5s default in isolation, so they need headroom under load.
  • cache.disable = true → cache = false in the per-case bunfig. The former leaves the manifest cache on; the .npm cache files are written by a fire-and-forget thread-pool task that sometimes lands before process exit and sometimes does not, so the second command's set of registry requests depended on whether that race was won. The old snapshots were generated sequentially where the save almost always won, which is why they held up on main but not under concurrent load (showed up on Windows CI first). With the manifest cache actually off the fixture captures the deterministic request set.

Assertion improvements

  • The npm.bunfigonly branch used to assert toContain(""), which is vacuous. It now asserts the real message: Security scanner 'test-security-scanner' is configured in bunfig.toml but is not installed.
  • expect(exitCode).toBe(...) now runs after the stdout/stderr checks so a wrong exit code surfaces the command output first.
  • Registry requests are asserted as a single { packages, tarballs } object with toEqual instead of two separate snapshot hits.

Timing (local debug+ASAN, CI=1, 3 runs each)

file before after
with-node-modules 97s / 131s / 138s 28s / 36s / 37s
without-node-modules 123s / 131s 37s / 41s / 57s

Full (unsampled) runs: with-node-modules 720/720 pass twice in ~294s each, without-node-modules 688/688 pass (32 skipped) in ~331s. Windows release build: both files 0 fail across 3 full runs each (4-5s per run). bun-security-scanner-workspaces.test.ts still passes unchanged.


no test proof · iteration 2 · docs-only change; test-proof not applicable

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The security scanner matrix now runs concurrent install cases through isolated registry sessions, records session-specific requests, validates deterministic expected results, and supports regenerating the expected request fixture. Scanner-specific bunfig assertions and configuration handling were updated.

Security scanner matrix

Layer / File(s) Summary
Isolated registry sessions
test/cli/install/simple-dummy-registry.ts
Adds per-session request logging and scanner behavior, session-prefixed registry routing, and session-aware metadata and tarball URLs.
Concurrent matrix execution and validation
test/cli/install/bun-security-scanner-matrix-runner.ts
Runs matrix cases concurrently, configures session-specific registries, validates requests by test ID and install state, and optionally rewrites expected data.
Deterministic expected request matrix
test/cli/install/bun-security-scanner-matrix-expected.json
Adds expected package and tarball request combinations for cases with and without existing node_modules.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed Concise and specific, it accurately summarizes the main change to run the security-scanner matrix concurrently.
Description check ✅ Passed It covers the change, motivation, fixes, and verification evidence, though it does not use the repository's exact template headings.

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

@robobun

robobun commented Jul 28, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:01 PM PT - Jul 28th, 2026

❌ @robobun, your commit 11cb410 has 1 failures in Build #84398 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36215

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

bun-36215 --bun

@robobun

robobun commented Jul 28, 2026 •

Copy link
Copy Markdown
Collaborator Author

Local debug+ASAN timing (3 CI-sampled runs each):

  • with-node-modules: 97s/131s/138s before, 28s/36s/37s after
  • without-node-modules: 123s/131s before, 37s/41s/57s after

Full unsampled runs green on Linux (720/720 and 688/688+32 skip, two runs each) and on a Windows release build (both files, 3 runs each, ~5s per run).

The second commit swaps cache.disable = true for cache = false so the manifest cache is actually off; the old setting left a fire-and-forget .npm cache write racing process exit, which made the registry-request fixture nondeterministic under concurrent load. The fixture was regenerated against the deterministic behavior.

CI: the two matrix files this PR touches are green across all lanes on builds #84318 and #84398. Remaining red on #84398 is unrelated to this diff: test/js/bun/http/proxy-stress-protocol.test.ts on debian 13 x64-asan (reported for main-break triage) plus parallel-batch timeouts that passed alone; #84318 failed on a darwin-aarch64 build-step timeout. Retrigger already spent. Ready for a maintainer to merge.

@claude claude Bot left a comment

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.

No bugs found. This is a substantive test-infrastructure refactor (session-multiplexed registry helper, custom JSON fixture replacing two 5k+ line .snap files, test.concurrent migration, tightened assertions), so worth a human look at the design choices even though it's test-only.

What was reviewed:

  • SimpleRegistry session mechanism — URL-prefix stripping regex, tarball URL rewrite, and per-session request log all route correctly; the legacy global path used by bun-security-scanner-workspaces.test.ts still works.
  • Spot-checked 7 entries in the new -expected.json against the deleted .snap files — values match; entry counts (720 with / 688 without) line up with the 32 TESTS_TO_SKIP entries.
  • UPDATE_EXPECTED regeneration merges into the imported fixture and re-emits both sections, so running one file preserves the other's entries.
  • newSession's counter increment and the expectedRequests mutation under test.concurrent are single-event-loop safe.
Extended reasoning...

Overview

Test-only change touching test/cli/install/: the shared matrix runner, the SimpleRegistry helper, a new 1414-line JSON fixture, and deletion of two large .snap files. No production code (src/) is touched. The runner now uses test.concurrent with per-session registry isolation (/_s/<id>/... URL prefix) so 720 cases can share one Bun.serve. Snapshot assertions are replaced with toEqual against a keyed JSON fixture (regenerable via SCANNER_TEST_UPDATE_EXPECTED=1). One vacuous toContain("") is tightened to the real error string, and the exit-code assertion is moved after output checks.

Security risks

None. Test infrastructure only; the in-process registry is port: 0 on localhost and only serves fixture tarballs.

Level of scrutiny

Moderate. It is test-only and self-contained, but it is not a mechanical change: it introduces a novel-for-this-suite pattern (custom JSON fixture with a hand-rolled line emitter instead of toMatchSnapshot), a session-multiplexing scheme in the registry helper, and a 30s per-test timeout. These are all reasonable — I checked each for correctness — but they are design decisions a maintainer may want to weigh in on (e.g., whether the JSON-fixture approach is preferred over sharding/other alternatives, and whether 30s per test is acceptable given the repo's guidance on timeouts).

Other factors

I verified backward compat with bun-security-scanner-workspaces.test.ts (still uses startRegistry() return + setScannerBehavior; the non-session code path is untouched). Spot-checked that recorded tarball paths after prefix-stripping (/left-pad-1.3.0.tgz etc.) match the old snapshot values, and that the handleMetadata tarball URL now carries the session prefix so downloads land in the right session log. The fixture entry counts (720/688) match 720 total minus 32 TESTS_TO_SKIP (without-modules only). Concurrent-safety concerns (session counter, fixture mutation in update mode) are non-issues under a single JS event loop. PR description reports full unsampled runs pass on both files plus the workspaces test.

@claude claude Bot left a comment

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.

I didn't find any bugs, but this is a fairly large test-infrastructure refactor that regenerates the expected-requests fixture from observed behavior after switching cache.disable = true → cache = false, so the new fixture values differ from the old snapshots (e.g. 0006 (with modules) now expects packages: ["left-pad"] where the old snapshot expected []). Worth a human spot-check that those new expectations are the intended behavior rather than a nondeterminism artifact being certified.

What was reviewed:

  • RegistrySession routing in simple-dummy-registry.ts — session prefix is stripped before metadata/tarball dispatch and tarball URLs carry the prefix back, so per-test logs stay isolated; the legacy global path used by bun-security-scanner-workspaces.test.ts is untouched.
  • Runner: test.concurrent/test.concurrent.skip swap, testId keying, exitCode assertion moved after output checks, vacuous toContain("") replaced with the real message.
  • writeExpected() / SCANNER_TEST_UPDATE_EXPECTED regeneration path — mutates the imported JSON in-place and writes sorted keys in afterAll, preserving untouched entries.
Extended reasoning...

Overview

Test-only change spanning test/cli/install/bun-security-scanner-matrix-runner.ts, test/cli/install/simple-dummy-registry.ts, a new 1400-entry bun-security-scanner-matrix-expected.json, and deletion of two ~5500/~7400-line .snap files. The runner now uses test.concurrent, the dummy registry hands out per-test sessions namespaced by a /_s/<id>/ URL prefix, and the two snapshot files are replaced by a JSON fixture asserted with toEqual. A follow-up commit switches the bunfig from cache.disable = true to cache = false to make the manifest-request set deterministic under concurrent load, and regenerates the fixture accordingly.

Security risks

None. Test infrastructure only; no production code touched. The registry is a local Bun.serve({ port: 0 }) fixture.

Level of scrutiny

Medium. It's test-only, but it's not mechanical: a new session-routing layer, a bespoke snapshot-replacement mechanism with its own regeneration env var, a bunfig cache-setting change that alters observed registry traffic, and a fully regenerated fixture whose values differ from the old snapshots. The important explicit assertions (scanner-ran markers, exit codes, tarball-only-on-cancel, node_modules presence) are preserved, and the npm.bunfigonly branch's vacuous toContain("") is now a real assertion. But the regenerated fixture is the piece I can't fully verify from the diff alone — someone familiar with the security-scanner install flow should eyeball a few of the changed entries.

Other factors

  • The workspaces test's use of SimpleRegistry (setScannerBehavior, no sessions) still routes through the untouched legacy path.
  • newSession never removes entries from the sessions Map — fine for a per-file 720-entry lifetime.
  • CI (#84063) was still building at review time, so cross-platform results aren't confirmed yet.
  • 30s per-test timeout is justified in a comment; concurrent batching bounds the wall-clock.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/cli/install/bun-security-scanner-matrix-expected.json`:
- Around line 1-3: Replace the positional numeric keys in the expected fixture
with stable semantic keys derived from each matrix case’s parameters, reusing
the naming logic from getTestName. Update the fixture generation and lookup
paths consistently so adding, removing, or reordering matrix dimensions does not
shift existing entries.
- Around line 1205-1220: Update the test expectations around the
security-scanner matrix to assert that the `without` collection contains every
non-skipped test ID, using `TESTS_TO_SKIP` to exclude the 32 skipped IDs.
Preserve the existing 720 total versus 688 non-skipped result validation and
make the assertion explicit rather than relying only on the expected JSON
entries.

In `@test/cli/install/bun-security-scanner-matrix-runner.ts`:
- Around line 515-534: The writeExpected function rewrites both expected-data
sections, allowing concurrent matrix tests to overwrite each other’s updates.
Change it to update only the section owned by hasExistingNodeModules, or
document the required one-file-at-a-time regeneration beside UPDATE_EXPECTED;
preserve the other section’s existing contents.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c25f5d08-ff01-4b69-a1ef-6ea6155c8989

📥 Commits

Reviewing files that changed from the base of the PR and between 9b678b4 and 37a4632.

⛔ Files ignored due to path filters (2)
  • test/cli/install/__snapshots__/bun-security-scanner-matrix-with-node-modules.test.ts.snap is excluded by !**/*.snap
  • test/cli/install/__snapshots__/bun-security-scanner-matrix-without-node-modules.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (3)
  • test/cli/install/bun-security-scanner-matrix-expected.json
  • test/cli/install/bun-security-scanner-matrix-runner.ts
  • test/cli/install/simple-dummy-registry.ts

Comment thread test/cli/install/bun-security-scanner-matrix-expected.json
Comment thread test/cli/install/bun-security-scanner-matrix-expected.json
Comment thread test/cli/install/bun-security-scanner-matrix-runner.ts Outdated

@claude claude Bot left a comment

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.

LGTM — test-only speedup that converts the 720-case matrix to test.concurrent with per-session registry isolation.

What was reviewed:

  • SimpleRegistry.newSession URL-prefix routing keeps the legacy global path intact; verified bun-security-scanner-workspaces.test.ts still uses it via setScannerBehavior/registryUrl with prefix="".
  • Snapshot → JSON fixture migration: assertions are equal-or-stronger (toEqual on {packages, tarballs}, missing-entry guard with regeneration hint); the vacuous toContain("") is now a real message check.
  • cache = false swap and fixture regeneration are explained as removing a manifest-cache write race, not changing the property under test.
  • writeExpected re-reads from disk and only rewrites the owned section (d797563), addressing the sibling-clobber concern.
Extended reasoning...

Overview

Test-infrastructure refactor across five files under test/cli/install/: the shared matrix runner switches from serial test + toMatchSnapshot to test.concurrent + a keyed JSON fixture; simple-dummy-registry.ts gains a RegistrySession abstraction that namespaces request logs and scanner-tarball variants behind a /_s/<id>/ URL prefix so one Bun.serve backs all concurrent cases; the two 5.5k/7.4k-line .snap files are replaced by bun-security-scanner-matrix-expected.json. No src/ changes.

Security risks

None. The change is confined to test harness code and a local in-process dummy registry. No auth, crypto, or user-facing surface is touched.

Level of scrutiny

Medium — it is test-only, but it rewrites how 1,440 assertions are checked and regenerates the expected-request data under different cache semantics (cache = false instead of cache.disable = true). I checked that:

  • the legacy non-session path in SimpleRegistry is byte-compatible for the one remaining consumer (bun-security-scanner-workspaces.test.ts): non-session requests still log to self.requestedUrls, use self.scannerBehavior, and get prefix="" in tarball URLs;
  • session-prefixed tarball paths are stripped back to /<name>-<ver>.tgz before logging, so getRequestedTarballs() output matches the fixture format;
  • test.concurrent.skip is an established pattern in 8 other test files;
  • concurrent mutation of expectedRequests[key][testId] during UPDATE_EXPECTED is safe because each test writes a unique key and JS is single-threaded.

Other factors

The PR description carries concrete evidence: 3× CI-sampled runs on debug+ASAN Linux showing ~3-4× speedup, full 720/720 and 688/688 unsampled runs green twice each, and 3× Windows release runs. The cache = false rationale (fire-and-forget .npm manifest-cache write racing process exit → nondeterministic second-command request set) is a de-flake, not a weakening — the test's purpose is scanner behavior, not manifest caching. Assertions were strictly strengthened: the vacuous toContain("") now checks the actual not-installed message, and expect(exitCode) moved after output checks per repo convention. The 30s per-test timeout is headroom for two-subprocess cases under concurrent debug+ASAN load; file-level wall-clock dropped, so it is not masking a slow test. All three CodeRabbit findings are resolved (one addressed in d797563, two withdrawn as out-of-scope with author agreement).

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
test/cli/install/bun-security-scanner-matrix-runner.ts (1)

649-651: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Remove the local per-test timeout.

Keep timeout enforcement centralized in the Bun test runner; a 30-second override makes hung matrix cases consume substantially longer before CI can stop them. Based on learnings, this repository’s test files should not set per-test timeouts.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/cli/install/bun-security-scanner-matrix-runner.ts` around lines 649 -
651, Remove the local 30-second timeout argument from the matrix test case near
the subprocess setup, leaving timeout enforcement to the centralized Bun test
runner. Ensure the test file does not configure a per-test timeout while
preserving the existing test logic and subprocess behavior.

Source: Learnings

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/cli/install/bun-security-scanner-matrix-runner.ts`:
- Around line 519-524: The writeExpected function still permits concurrent
fixture regenerations to overwrite each other’s matrix sections. Serialize
update-mode writes across both matrix files using an interprocess lock or
central collector, and ensure the lock covers the read-modify-write operation so
each regenerated section is preserved.

---

Outside diff comments:
In `@test/cli/install/bun-security-scanner-matrix-runner.ts`:
- Around line 649-651: Remove the local 30-second timeout argument from the
matrix test case near the subprocess setup, leaving timeout enforcement to the
centralized Bun test runner. Ensure the test file does not configure a per-test
timeout while preserving the existing test logic and subprocess behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2c9b6838-4839-4686-a657-124a90091163

📥 Commits

Reviewing files that changed from the base of the PR and between 37a4632 and 3ec9884.

📒 Files selected for processing (1)
  • test/cli/install/bun-security-scanner-matrix-runner.ts

Comment thread test/cli/install/bun-security-scanner-matrix-runner.ts

@claude claude Bot left a comment

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.

No bugs found. This is a well-reasoned test-infrastructure refactor, but it's ~200 lines of non-mechanical change (per-session registry routing, snapshot→JSON-fixture migration, cache = false swap that changes the asserted registry-request set) and CI on 3ec9884 hasn't finished yet, so it's worth a human look before merge.

What was reviewed:

  • Session prefix routing in simple-dummy-registry.ts — legacy non-session path preserved for bun-security-scanner-workspaces.test.ts (only uses setScannerBehavior/getUrl).
  • test.concurrent/test.concurrent.skip conversion and 30s timeout — matches CLAUDE.md guidance for independent subprocess suites.
  • npm.bunfigonly assertion: previously vacuous toContain(""), now checks the real message on the non-TTY path only (PTY race documented).
  • writeExpected re-reads from disk and rewrites only the owned section (addresses the concurrent-regen concern).
Extended reasoning...

Overview

Test-only change touching test/cli/install/bun-security-scanner-matrix-runner.ts (~130 lines changed), test/cli/install/simple-dummy-registry.ts (~60 lines added), a new bun-security-scanner-matrix-expected.json fixture, and deletion of two ~5500/~7400-line .snap files. The runner now creates a per-test RegistrySession with its own URL prefix, request log, and scanner-tarball variant so 720 cases can run under test.concurrent against one Bun.serve. Snapshot assertions become toEqual against the JSON fixture (regenerable via SCANNER_TEST_UPDATE_EXPECTED=1). No src/ code touched.

Security risks

None. All changes are in test harness code. The dummy registry only serves local .tgz fixtures on port: 0; the session prefix is a test-internal routing key.

Level of scrutiny

Moderate. It is test-only and the bug hunter found nothing, but it is not a mechanical change: it restructures how the matrix isolates state, replaces the assertion mechanism, and regenerates the expected request sets under a different cache setting (cache = false vs cache.disable = true). The PR argues the old snapshots encoded a manifest-cache write race that usually won sequentially but not concurrently — plausible, but that reasoning and the resulting fixture values are worth a maintainer glance rather than auto-approval.

Other factors

  • All three CodeRabbit findings are resolved/withdrawn; d797563 addressed the fixture-clobber concern by re-reading from disk and writing only the owned section.
  • The vacuous toContain("") on the npm.bunfigonly branch is replaced with a real message check, gated to !hasTTY because the PTY data callback can lose the last lines to proc.exited — a net tightening, though it means the TTY variant of that branch still has no error-text assertion (only the exit-code check).
  • bun-security-scanner-workspaces.test.ts is the only other consumer of SimpleRegistry; it uses only the legacy global-state API, which is unchanged.
  • The 30s per-test timeout is justified (two bun install-class spawns under concurrent debug+ASAN load) and does not mask a hang — the workload itself did not grow.
  • CI build #84090 for the head commit was still in progress at review time, so passing CI has not been confirmed.

robobun added 4 commits July 28, 2026 21:00
Make bun-security-scanner-matrix-{with,without}-node-modules.test.ts run
concurrently instead of serially. The shared SimpleRegistry now issues a
per-test session (isolated URL prefix, request log, and scanner-tarball
variant) so cases no longer clobber each other's state.

Snapshot matchers are not supported in concurrent tests, so the two
5500-line .snap files (1440 entries, 14 unique values) are folded into a
single JSON fixture keyed by test id and asserted with toEqual. The
fixture is regenerated with SCANNER_TEST_UPDATE_EXPECTED=1.

Use test.concurrent.skip for the CI-sampled/Windows/known-bad cases so
the skipped entries stay inside the concurrent batch; plain test.skip
between concurrent tests serializes the run.

Also replaces a no-op toContain("") in the npm.bunfigonly path with the
real error string and moves the exit-code assertion after the
stdout/stderr checks.
cache.disable = true leaves the manifest cache on, and the .npm cache
files are written by a fire-and-forget thread-pool task that may or may
not land before process exit. Under concurrent load the second command
sometimes found a cached manifest and sometimes did not, which made the
set of registry requests nondeterministic and showed up on Windows CI.

Switch to cache = false (which also clears the manifest cache) and
regenerate the expected-request fixture against the now-deterministic
behavior.
…atrix fixture

Re-reads the JSON from disk in afterAll so regenerating one file with
SCANNER_TEST_UPDATE_EXPECTED=1 cannot clobber the sibling section if
the other file was regenerated in between.
The npm.bunfigonly case exits before the interactive prompt, so the
Bun.Terminal data callback has no synchronization point and the final
lines can race proc.exited. Keep the strengthened assertion for the
non-TTY path where stdout/stderr are read to EOF.
@robobun
robobun force-pushed the claude/farm/26427b38/security-scanner-matrix-concurrent branch from 3ec9884 to 3e4570c Compare July 28, 2026 21:00

@claude claude Bot left a comment

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.

LGTM — test-harness-only speedup with tightened assertions.

What was reviewed:

  • SimpleRegistry session isolation: per-test /_s/<id>/ prefix routes request-log and scanner-tarball variant correctly; legacy global path still intact for bun-security-scanner-workspaces.test.ts (startRegistry return, setScannerBehavior, non-prefixed URLs all preserved).
  • Snapshot → JSON fixture swap: toEqual on {packages, tarballs} keyed by the pre-existing 4-digit test id; writeExpected re-reads from disk and updates only its owned section.
  • cache = false fix removes the manifest-cache save race that made request sets nondeterministic; vacuous toContain("") replaced with the real error message (non-TTY only, with a comment explaining the PTY drain race).
Extended reasoning...

Overview

This PR converts the two 720-case security-scanner matrix test files from serial test() to test.concurrent(). To make that safe it (a) adds RegistrySession to simple-dummy-registry.ts so each concurrent case gets its own URL-prefixed request log and scanner-tarball behavior on a shared Bun.serve, (b) replaces two ~5.5k-line .snap files with a single bun-security-scanner-matrix-expected.json fixture asserted via toEqual (snapshots are disallowed in concurrent tests), (c) switches skips to test.concurrent.skip so the CI sampler doesn't serialize the batch, and (d) swaps cache.disable = true for cache = false in the per-case bunfig to actually disable the manifest cache and make the observed request set deterministic. No files under src/ are touched.

Security risks

None. Everything is confined to test/cli/install/; the registry is a local Bun.serve({port:0}) and the "security scanner" here is a test fixture, not the production scanner code.

Level of scrutiny

Moderate for a test refactor: it changes how ~1400 assertions are checked and restructures shared test state for concurrency. I traced that the non-session code path (used by bun-security-scanner-workspaces.test.ts) is preserved — startRegistry still returns the URL, the global requestedUrls/scannerBehavior remain, and handleMetadata/handleTarball receive empty prefix / self.scannerBehavior when no /_s/ prefix is present. Session logs receive the stripped pathname so getRequestedPackages()'s slice(1) still yields bare package names, matching the fixture.

Other factors

The PR fixes two pre-existing test-quality issues per REVIEW.md: a vacuous expect(errAndOut).toContain("") is now a real assertion (guarded to non-TTY where stdout drains to EOF — the last commit's rationale), and the exit-code assertion was moved after the output assertions. The writeExpected clobber concern CodeRabbit raised was addressed in d797563 (re-read + write only the owned with/without section); the follow-up interprocess-lock request was withdrawn as the update-mode workflow is sequential-only. The 30s per-test timeout is justified with a comment (two bun install spawns under debug+ASAN load). Author reports full-matrix green on Linux ×2 and Windows release ×3, plus the untouched workspaces suite passing.

@claude claude Bot left a comment

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.

No issues found. This is test-only, but it's a substantial refactor — a new per-session isolation layer in SimpleRegistry, a fixture-format migration replacing ~13k lines of .snap with a regenerated JSON, a switch from cache.disable = true to cache = false that intentionally shifts what the fixture records, and a 5s→30s per-test timeout — so it's worth a human sanity-check on the regenerated fixture values and the concurrency isolation before landing.

What was reviewed:

  • Session routing in simple-dummy-registry.ts: /_s/<id>/… prefix is stripped before dispatch, tarball URLs in metadata carry the prefix back, and lockfiles generated by the pre-install use session.url, so all second-command requests land in the right session log.
  • Legacy global-state path (setScannerBehavior, requestedUrls) is kept intact; bun-security-scanner-workspaces.test.ts still uses it and is unaffected.
  • writeExpected re-reads from disk and rewrites only the owned with/without section (addresses the sequential-regeneration concern).
  • The previously vacuous toContain("") is now a real message assertion, gated to !hasTTY with a stated reason for the PTY exclusion.
Extended reasoning...

Overview

Test-infrastructure change to run the 720-case × 2-file security-scanner matrix concurrently. Touches test/cli/install/bun-security-scanner-matrix-runner.ts (session-scoped registry, test.concurrent/test.concurrent.skip, 30s per-test timeout, exit-code assertion moved after output checks, JSON-fixture assertion replacing toMatchSnapshot, SCANNER_TEST_UPDATE_EXPECTED regeneration path) and test/cli/install/simple-dummy-registry.ts (new RegistrySession and newSession(), URL-prefix routing in the fetch handler, session-aware tarball URLs and scanner-behavior selection). Two large .snap files are deleted and replaced by bun-security-scanner-matrix-expected.json. No src/ changes.

Security risks

None. Test-helper HTTP server on port: 0 localhost; no auth, crypto, or user-facing surface.

Level of scrutiny

Medium. It is test-only, but not mechanical: it introduces a new isolation abstraction, migrates the assertion format for 1400+ cases, and changes a bunfig setting (cache = false) that alters observed registry traffic and therefore the fixture contents. REVIEW.md flags snapshot/fixture regeneration and per-test timeout increases as things reviewers explicitly look at, so a human should confirm the regenerated fixture is sane and that the 30s per-test bound is acceptable under CI's concurrent load.

Other factors

The bug hunter found nothing. All four CodeRabbit findings are resolved or withdrawn with sound reasoning (positional keys are pre-existing; per-case toBeDefined covers completeness; writeExpected re-reads and writes only its owned section; interprocess locking is out of scope for the sequential local regeneration workflow). I traced the session-prefix routing end-to-end (metadata → tarball URL → lockfile → second-command fetch → session log) and confirmed the workspaces test's legacy path is untouched. The npm.bunfigonly assertion now checks the real error text but only for !hasTTY; the comment explains the PTY output race that motivates the exclusion, which is reasonable but leaves the TTY variant of that branch without a message assertion. CI for the latest commit was still building at review time, so there is no green signal yet — one more reason to leave the final call to a human.

@robobun

robobun commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #39956. This branch conflicts with main in the runner and both snapshot files, and its request fixture predates the bun update change in #38333. #39956 makes the cases concurrent the same way (per case registry state, test.concurrent.skip) and replaces the snapshots with per case expectations derived from the options instead of a fixture file. Closing this one.

@robobun robobun closed this Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants