[dborup]

Relates to #445

Summary

The cmd/server Go tests (race and coverage) run in three parallel jobs instead of one serial step. Ingestor coverage and its ForceReconnect race check, the small Go libraries/CLI/anomaly checks, the JavaScript units/lint/XSS checks, and the shell/container/proto checks now run as independent jobs. The existing E2E shards, fixtures and ports are unchanged. No benchmark, active fuzzing, short mode, retry, new skip or test-workload reduction is introduced.

Draft only, auto-merge off. No ready marking, merge, issue closure, settings changes, upstream writes or staging/production contact.

How sharding works (manifest v2, catch-all)

• scripts/go-server-shards.json pins only the slowest entries: shard 1 and shard 2 run their pins with go test -run '^(…)$'.
• Shard 3 is the catch-all. It runs every other Test, Fuzz seed target and Example with go test -skip '^(all pinned names)$'.
• A new, renamed or moved func Test… needs no manifest edit. It runs in shard 3. The five ops: bundled Mosquitto opt-in, normalised DISABLE_MOSQUITTO, pprof loopback default (#441) #444 tests (compose_optin_441_test.go) are not in the manifest and ran there on CI.
• Current pins: 2 on shard 1, 9 on shard 2. The remaining 2,483 entries run in the catch-all.

Exactly-once guard

• The 📋 Verify Go Test Assignments job checks that every active entry from the standard-library AST inventory is selected by exactly one shard. It also checks that the inventory equals the race-enabled go test -list set.
• Each shard verifies that its real go test -json output contains exactly its expected set, each entry once.
• The aggregator re-checks that the receipts together ran every active entry once, and that each shard used the right flag (-run or -skip).
• A pinned entry whose Test was removed selects nothing. It is reported, not fatal, and the next rebalance drops it.
• A shard with no active entries, a duplicate pin, or a pin to the catch-all fails, and the error names the fix command.

Rebalancing: when and how (also in AGENTS.md, "Go server test shards")

The ✅ Go Build & Test summary prints the test seconds per shard. Rebalance when one of these holds:

• the slowest shard stays above 1.25× the fastest on two green runs;
• a Test that takes more than ~10 s on CI was added or removed;
• --validate reports stale pins.

gh run download <run-id> -p 'go-server-*' -D /tmp/go-run-<run-id>     # artifacts are kept for one day
node scripts/rebalance-go-server-shards.js /tmp/go-run-<run-id> [/tmp/go-run-<other-id> ...]

The script averages each Test's top-level go test -json Elapsed on the GitHub runners. It fills the catch-all with the cheapest entries up to 1/3 of the total and pins the rest longest-first to the lightest pinned shard. The output is deterministic, with one pin per line, and records the SHA-256 of every source file.

Weights come from GitHub runners

The checked-in weights come from run 38529224242 attempts 1 and 2 on the GitHub runners. Attempt 1's JSON was taken from the job logs and attempt 2's from the go-server-* artifacts. The local arm64 measurements are no longer used. The projection is 319 / 325 / 299 test seconds.

Argument length (MAX_ARG_STRLEN)

go test passes the pattern to the test binary as one argv string (-test.run=… / -test.skip=…), and Linux limits one argv string to 128 KiB. Only pinned names go into a pattern, so the patterns are now 92 / 354 / 443 bytes, compared with about 34 KB each before. The guard refuses any pattern over 96 KiB.

Fail-closed gates (unchanged by this round)

• go-test requires a literal success from all eight Go/split jobs, including the guard.
• The stable required check 🏗️ Build & Publish Docker Image requires those eight jobs, the Go aggregate and the five E2E/report/image prerequisites: fourteen in total.
• The real Bash loops are executed by the JavaScript and Go guard tests (57 stable-gate and 33 Go-gate cases).
• Exactly 9 fork-guard literals remain in deploy.yml and 1 in release-fast-path.yml. No squad-*.yml, other workflow or repository setting changes.
• Coverage: the per-shard atomic profiles must have identical blocks, and their counters are summed (a union, not an average). The badge and its thresholds are unchanged.
• Only js-test keeps fetch-depth: 0, for the XSS --diff step. The split jobs use the default shallow checkout.

Measurements (GitHub Actions)

"Go span" runs from the first Go job start to go-test completion. "Runner minutes" is the sum of start-to-completion times of the non-skipped jobs, not billed minutes.

Run	Layout	Server shard test steps	Go span	Whole pipeline	Runner minutes
Before: 38520501264 / 38520401105 / 38479778504	serial	1151 / 632 / 1152 s (one step)	1578 / 960 / 1556 s	27:51 / 16:15 / 26:16	55.63 / 44.87 / 60.88 (mean 53.79)
38529224242 attempt 1 (f030d440)	v1, local weights	514 / 388 / 260 s	567 s	12:25	61.58
38529224242 attempt 2 (f030d440)	v1, local weights	451 / 294 / 412 s	517 s	11:32	63.25
38548653340 (d5f14aa0)	v1, local weights	321 / 387 / 421 s	480 s	11:23	61.23
38551184270 (4312030d)	v2 catch-all, GitHub weights	555 / 453 / 415 s	614 s	11:31	69.22

• Cost: sharding raises runner time by about +14 to +29%, a mean of 63.82 min over the four sharded runs against 53.79 serial, i.e. about +19%. The added time comes from three setups and three race compiles plus the guard job. Wall time for the Go path drops by about 60%, and E2E (about 11 min) remains the critical path.
• Balance limit: one test, TestHotStartup_ConcurrentQueryDuringBackgroundLoad, took 320 / 281 / 194 / 454 s in these four runs. It sits on shard 1 with a single ~20 s companion, so shard 1 cannot finish faster than that test. A rebalance over all four runs keeps the same structure (checked, not committed). The remaining spread is that test's runner variance, not the assignment.
• Run 38551184270: 2,494 unique entries, each run exactly once (2,485 PASS and 9 existing skips). Coverage is 25,976/28,554 = 90.97%, with the 91.0% badge unchanged.

Local verification (tree 2c21da81)

• All three real shards under -race, plus aggregation: 2,494 unique entries, 2,485 PASS and 9 SKIP, 91.0%. --runtime-list passes.
• cmd/ingestor suite: PASS. go vet on cmd/server: clean.
• sh test-all.sh: 248 passed, 0 failed. test-ci-go-shards.js: 26/26, including a real go test exact-once run of the catch-all on a fixture module.
• node test-frontend-helpers.js: 713 passed, 0 failed.

🤖 Generated with Claude Code

[dborup-agent]

Review — CS-pve-agent3 PR#446 — head f030d44

Dom: REQUEST CHANGES

Evidence tags: [T] I ran it myself (local scratch archives of head / merged tree / base), [A] GitHub Actions API or downloaded CI artifacts, [K] code reading.

The design is solid. The fail-closed gates, exact-once execution, receipts and coverage merge all hold up, and the speed-up is real. Two things block a merge: the manifest is already stale against current master, and there is no documented or tooled way to keep it current. The second is what caused the first.

Findings

#	Severity	Finding	Evidence
1	Blocker	The manifest is stale against current master, so merging now turns master red. #444 (merged after this PR's CI started) added 5 top-level tests in cmd/server/compose_optin_441_test.go: TestComposeFiles_BundledBrokerIsOptIn, TestStagingComposes_PprofPublishDefaultsToLoopback, TestStagingComposes_InContainerPprofListenerStaysReachable, TestNonPublishingComposes_LeavePprofOnLoopback and TestDeploymentDocs_CarryThe441UpgradeNote. On the merged tree (git merge-tree of 89adc5be + head = tree 98bc4da3), node scripts/run-go-server-shard.js --validate exits 1 with unassigned Go test: TestComposeFiles_BundledBrokerIsOptIn. GO_SERVER_SHARD=1 … --list fails the same way. All 3 shard jobs call readManifest too, so the guard, all three shards, go-test and build-and-publish would all fail. The green CI ran on merge commit c2883a44 (parents bcf263e1 + head). Attempt 2 reused that SHA, so CI never saw the new tests. The guard is working as designed, but the branch needs a rebase, 5 new entries and a fresh CI run.	[T] [A]
2	Major (maintainability)	No documented or tooled path for adding a test. Every new func Test… in cmd/server needs a hand-written entry in a 2,558-line JSON. validateManifest requires exactly three samplesSeconds, a matching meanSeconds, an exact file and a shard. A contributor has to invent [0,0,0] samples and pick a shard by hand. Under AGENTS.md ("every bug fix gets a regression test") that will happen in most PRs. Finding 1 shows it already happens within hours. Gaps: (a) the script that produced the manifest is not checked in (LPT assignment, 5 ms floor, sample ingestion), so rebalancing cannot be reproduced; (b) nothing in AGENTS.md, a script header or the error message says how to fix unassigned Go test: X; (c) sh test-all.sh and go test ./... both pass on the stale merged tree, so contributors only find out in CI; (d) moving a test to another file also fails (test source mismatch, mutant M6), which is fail-closed but adds friction. Requested: a small helper (e.g. --assign-new) that adds unassigned entries to the lightest shard, with zero samples or an explicit "unmeasured" marker, and removes stale ones. Also check in the regenerate/rebalance script, add a short doc paragraph and make the guard's error message name the fix command.	[T] [K]
3	Minor (test gap)	Mutant M4 survives. Changing coverageStats from block.count > 0n to >= 0n (every block counts as covered) passes all 18 cases in test-ci-go-shards.js, because every fixture block is covered after the merge. The impact is limited: the badge uses go tool cover -func on the merged profile. The report JSON and step-summary percentage would still be wrong. Add a case where one block stays at count 0 in every shard.	[T]
4	Minor	Shard balance on GitHub runners is uneven. The "Run assigned Go server tests" step took 514 / 388 / 260 s in attempt 1 and 451 / 294 / 412 s in attempt 2, so the slowest shard is up to 2× the fastest. The weights come from local arm64 / UID 0 runs (as the PR says). This is not on the critical path today, because E2E now finishes last. A later rebalance could use the CI test.jsonl artifacts.	[A]
5	Nit	Each shard's -run argument is about 34 KB (34,061 / 33,768 / 33,806 bytes). Linux limits a single argv string to 128 KiB (MAX_ARG_STRLEN), which leaves about 3.8× headroom. Around 3,300 tests per shard it would fail with E2BIG. Not urgent, but worth a comment or a guard-time length assertion.	[T] [K]
6	Nit	Runner minutes rise by about 15%: before 44.87 / 55.63 / 60.88 (mean 53.8), after 61.58 / 63.25. That cost is expected (3× setup and race compile, plus the guard job) and should be stated in the PR body.	[A]
7	Nit	The PR body still says "PR CI measurements are pending". Two green full executions now exist (run 38529224242, attempts 1 and 2). The issue's acceptance item "before/after for at least two runs" belongs in the PR body; numbers below.	[A]
8	Nit	The new shard, guard, ingestor and library jobs copy fetch-depth: 0 from the old job. Only js-test needs history (the XSS --diff step).	[K]

Answers to the focus points

Fail-closed gate. The gate is fail-closed. build-and-publish needs: all 14 prerequisites (guard, 3 shards, ingestor, libraries, JS, scripts, go-test, 3 E2E shards, e2e-report, image-check), keeps if: ${{ !cancelled() }}, and its loop requires the literal success. go-test has its own loop over the 8 split jobs with the same semantics. aggregateShards checks the guard, ingestor and each shard result again. The PR's tests execute the real Bash loops: 57 stable-gate cases and 33 Go-gate cases, in both JS and the Go TestForkGuardLeavesTestJobsRunning. Mutants M2 and M3 are red. [T] [K]

Coverage of tests. Each func Test… lands in exactly one shard. The AST inventory and the race-enabled go test -list set are equal on head (--runtime-list: 2,489 entries, 57 s). In the CI test.jsonl artifacts from all three shards, 2,489 distinct top-level entries ran, each exactly once (2,480 pass + 9 skip). Every manifest entry appears, and 971 subtest runs are included. The -run pattern is anchored ^(…)$ (M8 red without the anchors). Names are restricted to [A-Za-z0-9_], so the escaping is a belt-and-braces measure. Names contain no /, so -run never filters subtests, and the fuzz seed#N subtests run. A new unassigned test fails the guard (M1). [T] [A] [K]

Coverage merge. The merge is correct, and the percentage is unchanged. I re-merged the three downloaded shard profiles independently in Python: 14,229 identical blocks, all statement counts equal, 0 counter mismatches against the uploaded server-coverage.out. The result is 25,977 / 28,554 = 90.975%. Shards alone cover 79.15 / 81.39 / 82.95%, so it really is a union. The three "before" go-badges artifacts and the "after" badge all read 91.0%. The PR's local unsharded baselines (90.968–90.978%) agree. Mutants M7 (max instead of sum) and the PR's own "overwrite" mutant are red. [T] [A]

Fork guards. 9 in deploy.yml and 1 in release-fast-path.yml, on base, head and merged tree. All new jobs have contents: read and no job-level if, and none are repository-guarded. No publish or deploy job changed. release-fast-path.yml and squad-*.yml are untouched; deploy.yml is the only workflow file in the diff. Every relocated step is unchanged (compared after a YAML parse). The only intended differences are the replaced server step and overwrite: true on the badge upload. Apart from go-test, no existing job changed except build-and-publish (needs/env). [T] [K]

Measured times. The "before" numbers are accurate. I re-derived them via gh api. The server step took 1151 / 632 / 1152 s and the full Go job 1578 / 960 / 1556 s for 38520501264 / 38520401105 / 38479778504, with runner minutes 55.63 / 44.87 / 60.88, all exactly as stated. After (run 38529224242):

	Before (3 runs)	After attempt 1	After attempt 2
Go span (first Go job start → go-test done)	1578 / 960 / 1556 s	567 s	517 s
Whole pipeline (run start → last job)	27:51 / 16:15 / 26:16	12:25	11:32
Runner minutes (non-skipped job durations)	55.63 / 44.87 / 60.88	61.58	63.25

The Go critical path drops well below the ~19-minute server step. E2E is now the critical path. [A]

Complexity and maintenance. Yes, the manifest has to be regenerated or extended by hand, and none of that is documented (Findings 1 and 2). It is otherwise fairly heavy: about 600 lines of runner, report and inventory code with receipts, SHA bindings and attempt logic. That is defensible for fail-closed CI, but only if the day-to-day "I added a test" path is a one-liner.

Always-checks. There is no production code change: no public/ files, and in cmd/server/ only fork_guard_workflow_test.go, a test file. So cmd/server stays read-only, and there are no new map[string]interface{} or hex colours (#… hits in the diff are issue numbers in step names). scripts/check-xss-sinks.sh --diff origin/master on the merged tree: "no public/**/*.{js,html} changes to scan", exit 0. There are no closing keywords in the commit or PR body (closingIssuesReferences is empty). The commit author and committer match the required dborup identity. One commit. [T] [A]

Tests run (merged tree 98bc4da3 = 89adc5be + head, unless noted)

• cd cmd/server && go build . && go vet ./... && go test -count=1 ./...: vet clean, ok (42 s). [T]
• cd cmd/ingestor && go build . && go test -count=1 ./...: ok (223 s). [T]
• sh test-all.sh: 248 passed, 0 failed (including test-ci-go-shards.js 18/18 and test-ci-parallel-gates.js). Note that this passes even though the manifest is stale (Finding 2c). [T]
• node test-frontend-helpers.js: 713 passed, 0 failed. [T]
• node scripts/run-go-server-shard.js --validate on head: OK (2,489 entries). On the merged tree: exit 1, unassigned TestComposeFiles_BundledBrokerIsOptIn. --runtime-list on head: OK. [T]
• E2E: the PR touches no frontend or server code, so there are no PR-specific or affected E2E suites. As a smoke test I ran the core test-e2e-playwright.js against a locally built Go server from the merged tree, with e2e-fixture.db prepared as in CI (freshen, Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL taken from the workflow, corescope-migrate, seeds 2073, 199 and 245). Result: 152/155 passed, 3 existing skips, exit 0. I stopped the server by port lookup, and the port was free afterwards. [T]
• CI on head: run 38529224242 is green on both attempts. Attempt 2 is a full rerun with all 18 jobs at run_attempt 2. Neither attempt hit the known flakies flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256 or flaky: TestResolvedPathBackfill_WriteHoldUnderBudget_188 — wall-clock write-hold budget fails under CI load #267. [A]
• Red before: the PR's test-ci-parallel-gates.js and test-ci-go-shards.js both fail on the base tree, and the PR's TestForkGuardLeavesTestJobsRunning fails on base. All are green on head. [T]

Mutants (reviewer-authored, scratch only)

ID	Mutation	Check	Result
M1	New TestReviewerMutantUnassigned in cmd/server, not in manifest	--validate	red: unassigned Go test: TestReviewerMutantUnassigned
M2	go-test gate also accepts skipped	test-ci-parallel-gates.js + Go fork-guard test	red (both; Go reports 8 gate failures)
M3	Drop go-server-2 from build-and-publish.needs	JS + Go guard tests	red (both)
M4	coverageStats: count > 0n → >= 0n	test-ci-go-shards.js	survives (Finding 3)
M5	Drop the "every selected test completed" loop in verifyTestJSON	test-ci-go-shards.js	red
M6	Rename advert_intervals_test.go (test moves file)	--validate	red: test source mismatch
M7	Coverage merge uses max instead of sum	test-ci-go-shards.js	red
M8	Drop ^…$ anchors from testPattern	test-ci-go-shards.js	red
(natural)	Current master + head	--validate	red (Finding 1)

Green controls on unmodified head: 18/18, 57 + 33 gate cases, and the Go fork-guard test all pass.

Not verified

• I did not run the shards under -race locally. Execution evidence comes from the CI artifacts (receipts, test.jsonl, profiles) of attempt 2, which I re-verified offline.
• I did not run the full sharded E2E suite (3 shards). I ran only the core Playwright smoke, since the PR has no frontend or server changes.
• I did not exercise the cancellation and rerun-failed-jobs paths (earlier-attempt receipts) on GitHub. I read them in code and their unit tests only.
• I did not check branch-protection settings (whether "require up-to-date branch" would have caught Finding 1).
• I did not reproduce the PR's local arm64 timing samples.

Head was f030d440 at both the start and the end of this review (git ls-remote).

[dborup-agent]

Rapport — CS-pve-agent1 PR#446 runde-2 — head 4312030

Review feedback addressed (commit 4312030d)

Evidence tags: [T] I ran it myself, [A] GitHub Actions API or downloaded CI artifacts, [K] code reading.

Commits, in order, all ordinary with no rebase, amend or force-push:

• 71eae69b: merge of origin/master 89adc5be.
• 3e614f6c: Finding 3.
• d42501b0: Finding 8.
• 4d9acd51: Findings 1, 2, 4 and 5.
• 4312030d: merge of the concurrent branch head d5f14aa0; see "Concurrent push" below.

1. Blocker, stale manifest: fixed. I merged origin/master 89adc5be with a merge commit. On the merge, before the fix, --validate exits 1 with unassigned Go test: TestComposeFiles_BundledBrokerIsOptIn (red, [T]). After the fix, all five ops: bundled Mosquitto opt-in, normalised DISABLE_MOSQUITTO, pprof loopback default (#441) #444 tests run in the catch-all shard without any manifest entry, and they passed on CI (shard 3, -skip) [A].
2. Maintainability: fixed with a catch-all shard, not with --assign-new.
   • Manifest v2 pins only the slowest entries: 2 on shard 1 and 9 on shard 2 (-run '^(…)$'). Shard 3 runs everything else with -skip '^(all pinned names)$'. A new, renamed or moved Test needs no manifest edit, so reviewer mutant M1 is now green by design and M6 (moving a file) no longer fails [T].
   • The guard now proves exactly-once instead: every active AST entry is selected by exactly one shard (assertPartition), and the AST inventory still has to equal go test -race -list.
   • The aggregator re-checks that the receipts together ran every active entry once, and that each shard used -run or -skip as expected.
   • Stale pins select nothing. They are reported, not fatal, and the next rebalance drops them.
   • scripts/rebalance-go-server-shards.js regenerates the manifest from CI go test -json output. AGENTS.md ("Go server test shards") documents when and how to run it, the guard errors name the command, and the go-test step summary prints the test seconds per shard.
   • I disagree with one detail of the request: an --assign-new helper is unnecessary once unpinned tests run automatically, so I did not add one.
3. M4: fixed. The fixtures now contain a block that stays at count 0 in every shard, so the merge must report 5/8 statements. On the old fixture, M4 (>= 0n) passed 18/18 [T]. On the new fixture, M4 is red [T].
4. Balance: weights now come from GitHub runners.
   • The weights come from run 38529224242 attempt 1 (JSON extracted from the job logs) and attempt 2 (the go-server-* artifacts), with source SHA-256s recorded in the manifest. The local arm64 data is gone.
   • Shard test steps after the fix (run 38551184270): 555 / 453 / 415 s, a max/min ratio of 1.34 [A]. Before: 514/388/260 s (1.98) and 451/294/412 s (1.53).
   • The floor is a single test. TestHotStartup_ConcurrentQueryDuringBackgroundLoad took 320 / 281 / 194 / 454 s across the four GitHub runs [A], and it sits on shard 1 with one ~20 s companion.
   • A rebalance over all four runs produces the same structure (projection 335 / 335 / 324). I checked it but did not commit it, because it would only churn the manifest [T].
5. Nit MAX_ARG_STRLEN: fixed by using the shorter form. Only pinned names go into a pattern, so the patterns are 92 / 354 / 443 bytes instead of about 34 KB each. testPattern refuses anything over 96 KiB and explains the Linux 128 KiB per-argv limit, and AGENTS.md documents it [T] [K].
6. Nit, runner-minute cost: in the PR body. Before, the mean was 53.79 min. After, the four sharded runs took 61.58 / 63.25 / 61.23 / 69.22 min, a mean of 63.82, which is +14 to +29% (mean +19%) [A]. The +29% run is the one where HotStartup took 454 s.
7. Nit, stale PR body: fixed. "Measurements pending" is gone, and the body now has before/after for four sharded runs.
8. Nit, fetch-depth: 0: fixed. Only js-test keeps it. The guard, the three shards, ingestor, libraries, script and go-test jobs use the default checkout. No Go test or relocated script needs history (resolveCommit uses only rev-parse HEAD) [K]. test-ci-parallel-gates.js now pins this, red before and green after [T].

Unchanged: 9 fork guards in deploy.yml and 1 in release-fast-path.yml, the fail-closed 14-prerequisite gate, no squad-*.yml changes, no stg or prod access [T] [K].

Concurrent push

While I was working, the remote branch moved from f030d440 to d5f14aa0 ("Merge origin/master into Go CI sharding and refresh measured assignments", a re-measured v1 manifest). My push was rejected as a non-fast-forward. That commit has the same parents (f030d440 + 89adc5be), and its tree differs from my merge 71eae69b only in scripts/go-server-shards.json. I merged it with an ordinary merge commit (4312030d) and kept the v2 manifest; no force-push. The trees of 4312030d and the tested 4d9acd51 are identical (2c21da81) [T]. Whoever is still writing to this branch should stop, or coordinate before pushing again.

Tests (tree 2c21da81 = head)

• Red before the fix [T]:
  • --validate on the merge: unassigned Go test: TestComposeFiles_BundledBrokerIsOptIn.
  • New test-ci-go-shards.js: Cannot find module './scripts/rebalance-go-server-shards'.
  • test-ci-parallel-gates.js: go-shards-check must use the default shallow checkout.
• test-ci-go-shards.js: 26/26 [T]. New cases cover the catch-all with an unpinned Test, the partition, pin validation, stale pins, -run/-skip selection, the pattern limit, the uncovered block, per-shard seconds, the rebalance (CI JSON, averaging, determinism, a test running twice), and a real go test -race exact-once run of all three shards plus aggregation on the fixture module, including subtests and fuzz seeds in the catch-all.
• test-ci-parallel-gates.js: 57 + 33 gate cases pass. The Go TestForkGuardLeavesTestJobsRunning passes [T].
• sh test-all.sh: 248 passed, 0 failed [T]. node test-frontend-helpers.js: 713 passed, 0 failed [T].
• All three real shards locally (-race, coverage) plus the real aggregator [T]:
  • 2,494 unique entries, 2,485 PASS + 9 SKIP;
  • 25,972/28,554 statements, 91.0%;
  • --runtime-list OK;
  • steps 227 / 265 / 236 s.
• cmd/ingestor go test -count=1 ./...: ok (203 s) [T]. cmd/server go vet ./...: clean [T].
• E2E: the PR touches no frontend or server code, so no suite is affected. As a smoke test I ran the core test-e2e-playwright.js against a local Go server with e2e-fixture.db prepared as in CI: freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL extracted from the workflow, corescope-migrate, and seeds 2073 / 199 / 245.
  • It fails fast at Version info lives on Perf dashboard, not in navbar (#navStats wait timeout). It fails identically on origin/master 89adc5be (control run, same seed), so it is environmental or pre-existing and not caused by this PR [T].
  • A standalone browser probe shows #navStats filled correctly, with no console errors [T].
  • On CI, all three E2E shards passed [A].
  • Ports 13591 and 13592 were free afterwards.

Mutants (one per finding, scratch copy of head, never committed) [T]

Finding	Mutation	Check	Result
1	The catch-all selects only entries explicitly pinned to it (old semantics)	test-ci-go-shards.js, --validate	red: empty active Go shard 3: regenerate with scripts/rebalance-go-server-shards.js …
2	The catch-all -skip omits shard 2's pins (a test would run twice)	test-ci-go-shards.js	red
2	Rebalance puts the next entry in the heaviest shard	test-ci-go-shards.js	red
3	coverageStats > 0n → >= 0n (M4)	test-ci-go-shards.js	red (it survived 18/18 on the old fixture)
4	Rebalance keeps only the first run's sample	test-ci-go-shards.js	red
5	The pattern-length assertion is disabled	test-ci-go-shards.js	red
8	fetch-depth: 0 reintroduced in the shard steps	test-ci-parallel-gates.js	red: go-server-1 must use the default shallow checkout

Green control on unmodified head: 26/26, 57 + 33, and --validate OK (2,494 entries, 2 / 9 / 2,483).

CI per job: run 38551184270, attempt 1, head 4312030d [A]

Conclusion: success. 15 jobs succeeded and 3 were skipped as expected (Release Artifacts, Publish Badges & Summary, Deploy Staging). No reruns. The known flakies #271 and #301 did not occur.

Job	Result	Duration
📋 Verify Go Test Assignments	success	66 s
✅ Go Server Tests (1/3)	success	577 s (test step 555 s)
✅ Go Server Tests (2/3)	success	479 s (test step 453 s)
✅ Go Server Tests (3/3), catch-all	success	436 s (test step 415 s)
✅ Go Ingestor Tests	success	225 s
✅ Go Libraries & CLI	success	84 s
✅ JavaScript Unit Tests & Lint	success	122 s
✅ Script & Container Unit Checks	success	69 s
✅ Go Build & Test (aggregate)	success	35 s
🧱 Validate Docker Image	success	81 s
🎭 Playwright E2E Tests (1/3)	success	640 s
🎭 Playwright E2E Tests (2/3)	success	654 s
🎭 Playwright E2E Tests (3/3)	success	654 s
📊 Verify E2E Shards & Coverage	success	18 s
🏗️ Build & Publish Docker Image	success	13 s
📦 Release Artifacts / 📝 Publish Badges & Summary / 🚀 Deploy Staging	skipped	—

From the go-coverage report: 2,494 unique entries, each run once (2,485 PASS + 9 skip), with selections -run:2 / -run:9 / -skip:2483. Test seconds are 476.6 / 372.7 / 333.0. Coverage is 25,976/28,554 = 90.97%, with the 91.0% badge unchanged. The Go span is 614 s and the whole pipeline 11:31. Runner time was 69.22 min.

Not verified

• I did not exercise the rerun-failed-jobs path for the new selectionFlag receipt field on GitHub. Earlier-attempt receipts from before this change do not carry it, but they could only exist for an older head, which the run/head check already rejects [K].
• I did not compare against branch protection settings.

[adminopenclaw8-sketch]

Review — CS-Macmini PR#446 — head 4312030

Dom: APPROVE med nits

Evidence tags: [T] I ran it myself (scratch archives of head / merged tree / base, local servers on ports 13801–13802), [A] Actions API or downloaded CI artifacts, [K] code reading.

Round 2 closes all six points of the previous review. The catch-all design removes the whole class of problem behind blocker 1 rather than patching the symptom, and I verified that twice over: master moved under me during this review (94dedd78 → 288c5b7e, 6 more cmd/server test files) and the guard stayed green on both merges with no manifest edit. Coverage is now provably identical to an unsharded run on the same tree, not just equal after rounding. What is left is small: two unasserted properties and a documented rebalance trigger that currently fires when rebalancing cannot help.

Findings

#	Severity	Finding	Evidence
1	Nit (test gap)	The new jobs' permissions: contents: read is not asserted anywhere. The PR adds least-privilege permissions to all eight new jobs plus go-test (go-test previously inherited the workflow default), which is a real improvement — but only image-check has a permissions assertion (test-ci-parallel-gates.js:129, fork_guard_workflow_test.go:175-180). My mutant MM7 adds packages: write to go-server-1 and everything stays green: test-ci-go-shards.js 26/26, test-ci-parallel-gates.js 57+33, TestForkGuardLeavesTestJobsRunning ok. These jobs run PR-authored code and upload artifacts, so the property is worth pinning the way image-check already does.	[T] [K]
2	Nit (test gap / doc rot)	The manifest metadata block is purely decorative and unvalidated. validateManifest checks version, package, shards and every pinned entry, but nothing in metadata. Mutant MM8 sets projectedTestSeconds: [1,1,99999], unmeasuredEntries: -7 and sources: [], and --validate, test-ci-go-shards.js, test-ci-parallel-gates.js and the Go workflow test all stay green. The field is already stale: the committed value is 5, while the merged tree against current master has 22 unmeasured active entries (my own run of scripts/rebalance-go-server-shards.js reports it). Since the PR body and AGENTS.md lean on this block as the provenance record (source run SHA-256s, "weights come from GitHub runners"), either assert its consistency with pinned or label it explicitly as informational-at-generation-time.	[T] [K]
3	Nit (documented rule misfires)	The 1.25× rebalance trigger is already met by the very run that validates this PR, and a rebalance does not fix it. AGENTS.md and the go-test step summary say to rebalance when the slowest shard stays above 1.25× the fastest. Run 38551184270 reports test seconds 476.58 / 372.65 / 332.96 → 1.43×. I downloaded that run's own go-server-* artifacts and ran the committed rebalance script on them: the new projection is 453.77 / 358.47 / 369.95 → 1.27×, still over the threshold, because the floor is a single test (TestHotStartup_ConcurrentQueryDuringBackgroundLoad alone is 453.77 s there). So a maintainer following the documented rule will regenerate the manifest, see the ratio stay above 1.25, and have nothing left to do. The PR body explains the floor correctly; the rule itself should carve it out (e.g. compare against the floor, or state the threshold only applies once no single Test exceeds total/shards).	[T] [A]
4	Nit	sh test-all.sh now needs the Go toolchain with cgo, which it did not before. test-ci-go-shards.js runs the real runShard, which always passes -race. On base, no test-all.sh entry invokes the Go toolchain at all. With go off PATH the suite fails with Go inventory failed: spawnSync go ENOENT rather than skipping. The cost is negligible (5 s measured) and CI's js-test sets Go up, so this is only about the local loop AGENTS.md describes as the JS unit suite — a one-line note or a skip-with-reason would do.	[T] [K]
5	Nit	Stale pins are reported only on --validate stdout in the guard job. The rebalance guidance lives in the go-test step summary, which does not mention them, so a pin that silently selects nothing is visible only to someone reading the guard log. Consider echoing the stale-pin note into GITHUB_STEP_SUMMARY next to the shard seconds.	[K]

No blocking findings. Nothing above needs to hold up the merge.

Answers to the points

Blocker 1 — manifest current against master, all tests covered. Closed, and demonstrated twice during this review. master was 94dedd78 when I started: merged tree (git merge-tree) b795df83, --validate exit 0, 2 505 runnable entries as 2 / 9 / 2 494. master then advanced to 288c5b7e mid-review (adding scope_approval_test.go, scope_approval_auth_test.go, scope_approval_review_test.go, unknown_scopes_test.go, openapi_sri_test.go, vendor_cache_headers_test.go): merged tree 851a5d71, --validate exit 0, 2 514 entries as 2 / 9 / 2 503. Both merges are clean and neither needed a manifest edit. I confirmed the 11 scope_audit_test.go entries from the first merge land in shard 3's selection and are absent from its -skip pattern. On the newest merged tree test-ci-go-shards.js 26/26, test-ci-parallel-gates.js 57+33, go vet clean and the workflow Go tests pass. [T] [K]

Maintainability — catch-all, exactly-once guard, rebalance script. All three hold up under my own mutants.

• New test needs no manifest edit (MM1): I added a brand-new func TestMacminiCatchAllAdoptsNewTests with a subtest to cmd/server. --validate stays green (2 506 entries, 2 / 9 / 2 495), the test appears in shard 3's selection and not in its -skip pattern. No manifest change. [T]
• Nothing runs twice (MM2): making the catch-all stop excluding pins fails --validate with Go test selected by two shards: TestChannelsSQL_PlanWithStats and fails test-ci-go-shards.js. [T]
• Nothing falls out (MM3): dropping one unpinned entry from the catch-all fails with Go test selected by no shard: TestEstimateAdvertInterval_RegularSeries, and the unit suite with Go test selected by no shard: TestGamma. [T]
• A real double execution the static partition cannot see (MM4): I built a fixture module (cmd/server with a pinned parent, an unpinned parent, a prefix-sharing sibling and a Fuzz target) and mutated shardSelection so the catch-all's -skip omits shard 1's pins. --validate still passes — and the real go test -race catch-all run then fails with unassigned Go test executed: TestPinned, with status: "failure" recorded in the receipt. The receipt layer, not just the static check, is what catches this. [T]
• Go -run/-skip semantics the design rests on: verified directly against the real toolchain. Anchored -run '^(TestPinned)$' runs the parent and all its subtests; anchored -skip '^(TestPinned)$' skips the parent and its whole subtree while the prefix-sharing TestPinnedSuffix still runs, and fuzz seeds (FuzzThing/seed#0) run in the catch-all. [T]
• Rebalance script: it exists, is documented in AGENTS.md ("Go server test shards") and in its own header, and the guard errors name it. I ran the documented workflow end to end on run 38551184270's downloaded go-server-* artifacts: it succeeds, is byte-identical across two runs, and correctly treats the new master tests as unmeasured and leaves them in the catch-all. Its per-input ran twice in one CI run assertion is also an independent exactly-once cross-check and passed on the real artifacts. [T] [A]

M4 killed. coverageStats count > 0n → >= 0n now fails test-ci-go-shards.js with actual: 8, expected: 5 on the new fixture (MM5). [T]

Balance from runner times; MAX_ARG_STRLEN and runner minutes documented. The manifest's samplesSeconds match the GitHub-runner figures and the local arm64 data is gone. Patterns are now 92 / 354 / 443 bytes on the merged tree, against the previous ~34 KB. MAX_PATTERN_BYTES is 98 304 with the Linux 128 KiB per-argv limit explained in a comment and in AGENTS.md; I exercised the guard with 3 000 synthetic names and it refuses at 112 893 bytes with a message naming the rebalance command, and it still refuses an empty pattern. Runner minutes are in the PR body and I re-derived every figure from the jobs API: 69.22 (this head), 63.25 / 61.23 / 61.58 (earlier sharded), 55.63 / 44.87 / 60.88 (serial, mean 53.79) — all exactly as stated. Go span 614 s confirmed (first Go job start 10:40:16Z → last Go job end 10:50:30Z). For reference, today's serial master run is 58.70 runner-min at 27:40 wall, against 69.22 at 11:34 here. Finding 3 is the only balance remark. [T] [A]

Gate fail-closed, fork guards, squad-*.yml. The gate is fail-closed in both directions and the real Bash loops are executed, not just inspected. MM6a (drop go-shards-check from build-and-publish.needs) is red in both test-ci-parallel-gates.js and TestForkGuardLeavesTestJobsRunning; MM6b (accept skipped too) is red in both, the Go test naming all eight affected keys. build-and-publish needs all 14 prerequisites and keeps if: ${{ !cancelled() }}. A YAML parse of head against base shows 8 added jobs, 0 removed, the three shard jobs' step lists byte-identical to each other (the anchor resolves correctly), fetch-depth: 0 only on js-test and the pre-existing E2E jobs, and the only base go-test step that disappears is the replaced server step; every relocated step's run/uses/with is unchanged except overwrite: true on the badge upload. The upstream fork-guard literal appears 9 times in deploy.yml (8 if: guards plus the header comment) and 1 time in release-fast-path.yml, identical to base. deploy.yml is the only workflow file in the diff; no squad-*.yml. The removed server-test.log was referenced nowhere else. [T] [K]

Merged coverage equals master. Strongest check I ran. On one identical merged tree I ran both a single serial go test -race -coverprofile (498.9 s) and the three real shards plus the aggregator, and compared block by block:

	covered / total	percent
serial unsharded	26 208 / 28 820	90.9368 %
3-shard union	26 208 / 28 820	90.9368 %

Exactly equal, with only two single-statement blocks differing in direction (one each way, netting zero) — ordinary coverage nondeterminism, not a merge artifact. Independently of that I re-merged the three downloaded CI shard profiles in Python: identical block sets, 14 229 blocks, all statement counts equal, 0 counter mismatches against the uploaded merged profile, 25 976 / 28 554 = 90.971 %. It is a true union (shard 3 alone 90.93 %, shards 1 and 2 alone 4.37 % and 6.57 %). The go-badges artifacts read 91.0 % / 81.9 % on this head and on both master runs from today, and the serial master job log reports 91.0 %. [T] [A]

Always-checks. No production change: public/ untouched, and in cmd/ only cmd/server/fork_guard_workflow_test.go, a test file — so cmd/server stays read-only. No new map[string]interface{} anywhere in the diff, and no hex colours (the #… hits are issue numbers in step names). scripts/check-xss-sinks.sh --diff origin/master on the merged tree: "no public/**/*.{js,html} changes to scan", exit 0. No closing keywords in any commit message and closingIssuesReferences is empty. All seven commits have author and committer dborup <kontakt@meshview.dk>. The PR is still a draft with auto-merge off. [T] [A] [K]

Issue acceptance. The timing data is in the PR as the manifest's samplesSeconds/meanSeconds, the manifest is checked in, before/after wall time and runner minutes are given for four sharded runs against three serial ones, the unassigned-test guard and the fail-closed gate both have tests with mutants shown red, and coverage is unchanged. Item 1's "look for outliers that can be fixed outright" is documented rather than acted on — the single 453 s test now sets the Go path's floor — which is Finding 3's underlying cause and worth a follow-up rather than a change here.

Tests run

On the merged tree against master 94dedd78 (tree b795df83) unless noted:

• cmd/server: go build ., go vet ./... clean, go test -count=1 ./... ok (45.4 s). Separately go test -race -coverprofile -count=1 ./... ok (498.9 s). [T]
• cmd/ingestor: go build ., go test -count=1 ./... ok (108.3 s). [T]
• sh test-all.sh: 249 passed, 0 failed (249 files), including test-ci-go-shards.js 26/26 and test-ci-parallel-gates.js 57+33. [T]
• node test-frontend-helpers.js: 713 passed, 0 failed. [T]
• node scripts/run-go-server-shard.js --validate on both merged trees: exit 0 (2 505 and 2 514 entries). --list for all three shards inspected. node scripts/run-e2e-shard.js --validate: 159 suites, exit 0. [T]
• All three real shards plus the real aggregator on the merged tree under -race with coverage: all exit 0, block sets identical, union as tabled above. [T]
• E2E: the PR touches no frontend or server code, so no suite is affected and the PR adds none. As a smoke test I ran the core test-e2e-playwright.js against a local Go server built from the merged tree with e2e-fixture.db prepared exactly as in CI — freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL extracted programmatically from the workflow, corescope-migrate, then seeds 2073, 199 and 245: 154/157 passed, 3 skipped, exit 0 on port 13801. Server stopped by port lookup; 13801 and 13802 free afterwards. [T]
  • A note that corrects my predecessor's and the author's reading: the Version info lives on Perf dashboard failure both of them hit locally is not environmental. It needs a .git-commit file in the server's working directory — without it /api/health reports commit: "unknown", the Perf dashboard renders no Version card, and the test's second waitForFunction times out. I reproduced the failure identically on plain origin/master (control, port 13802), then added .git-commit and the full suite passed. Worth adding to the local E2E recipe. [T]
  • Out of scope but verified while chasing it: in test-e2e-playwright.js (a master file this PR does not touch) the { timeout: … } objects passed to page.waitForFunction land in Playwright's arg position, not options, so those waits silently use setDefaultTimeout. Changing the inline value has no effect; changing setDefaultTimeout(10000) does. Not filed.
• Red before, green after: with head's test-ci-parallel-gates.js and fork_guard_workflow_test.go dropped onto the base tree, the JS suite fails with missing go-shards-check job and the Go test with job "go-server-1" not found; both are green on head. [T]
• CI per job, run 38551184270, attempt 1, head 4312030d — I queried the jobs API myself: 15 success, 3 expected skips (Release Artifacts, Publish Badges & Summary, Deploy Staging), guard 66 s, shards 577 / 479 / 436 s, ingestor 225 s, libraries 84 s, JS 122 s, scripts 69 s, aggregate 35 s, image 81 s, E2E 640 / 654 / 654 s, E2E report 18 s, publish 13 s. No reruns; it is the only run on this head. The shard report records 2 494 unique entries each run once (2 485 pass + 9 skip), selections -run:2 / -run:9 / -skip:2 483, and its manifestSha256 matches the committed manifest byte for byte. Neither flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256 nor flaky: TestResolvedPathBackfill_WriteHoldUnderBudget_188 — wall-clock write-hold budget fails under CI load #267 appeared. [A]

Mutants (reviewer-authored, scratch only)

ID	Mutation	Check	Result
MM1	Brand-new unpinned func Test… with a subtest added to cmd/server	--validate, shard-3 selection	green by design: no manifest edit, lands in the catch-all
MM2	Catch-all stops excluding pins (every pin would run twice)	--validate + test-ci-go-shards.js	red (both)
MM3	Catch-all drops one unpinned entry (a test falls out)	--validate + test-ci-go-shards.js	red (selected by no shard)
MM4	Catch-all -skip omits shard 1's pins; static partition unaffected	real go test -race catch-all run on a fixture module	red: unassigned Go test executed: TestPinned
MM5	coverageStats > 0n → >= 0n (the old M4)	test-ci-go-shards.js	red
MM6a	Drop go-shards-check from build-and-publish.needs	JS gate suite + Go workflow test	red (both)
MM6b	Both gate loops accept skipped as well	JS gate suite + Go workflow test	red (both)
MM7	Add packages: write to go-server-1	all four suites	survives (Finding 1)
MM8	Garbage manifest metadata (projections, unmeasured count, empty sources)	all four suites	survives (Finding 2)
—	Oversize -run pattern: 3 000 synthetic names	testPattern	refused at 112 893 bytes with the fix command

Green controls on the unmutated merged tree: 26/26, 57+33, --validate 2 505 entries, Go workflow tests ok.

Not verified

• I did not run the three sharded E2E jobs, only the core Playwright suite, since the PR has no frontend or server change and no E2E of its own.
• I did not exercise the rerun-failed-jobs path on GitHub (earlier-attempt receipts and the new selectionFlag field). Code and unit tests only.
• I did not exercise the cancellation path (spawnLogged signal forwarding) against a real CI cancellation; its unit test covers the mechanism.
• I did not check branch-protection settings, so I cannot say whether "require branches to be up to date" would have caught the original blocker 1 independently.
• My serial-versus-sharded coverage comparison was run on macOS/arm64, not on the Linux runners. The AST inventory's active flag is evaluated for the local GOOS, so a future GOOS-constrained test file could make --validate platform-dependent; today there are 0 inactive entries, so this is latent only.
• I did not re-run the four earlier sharded CI runs; their figures come from the Actions API.

Head was 4312030d at both the start and the end of this review (git ls-remote). master advanced from 94dedd78 to 288c5b7e during it, and I re-validated against both.