ci: shard Go race tests into a round-robin 3-way matrix (PLT-959)

[amir-deris]

Summary

• Race Detection previously ran all ~420 packages serially in a single job (~18 min baseline). It's now sharded into a 3-way matrix in go-test.yml, with an aggregate check that keeps the "Race Detection" status name so branch protection doesn't need to change.
• Packages are split round-robin (package i → shard i % NUM_SPLIT) via the Makefile's split-test-packages target.
• A short list of consistently slow packages (e.g. IBC keeper packages, sei-db engine tests, giga/tests, evmrpc/tests) is round-robined separately, ahead of the rest, so they can't all land on the same shard by coincidence.
• make test-group-N reproduces a given CI shard locally.
• Preserves -race, -tags=ledger,test_ledger_mock, and the occ_tests -parallel=1 special case per shard.

Results

Latest run: shard 0 = 13m0s, shard 1 = 12m28s, shard 2 = 10m41s (max ~13 min), down from the ~18 min single-job baseline, showing about 28% CI runner time reduction for this job.

Context

Linear: PLT-959

Starting at NUM_SPLIT=3; can scale up further once we've confirmed shared-runner capacity handles it.

Test plan

• Confirm the run fans out into "Race Detection (shard N)" jobs plus the aggregate "Race Detection" check
• Confirm all shards pass and the aggregate check reflects failures correctly
• Confirm wall-clock improvement over the single-job baseline
• Confirm NUM_SPLIT=3 make test-group-0 locally reproduces shard 0's package set

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 10, 2026, 12:19 PM

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.85%. Comparing base (ab08efb) to head (f2a1122).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3874      +/-   ##
==========================================
+ Coverage   59.45%   60.85%   +1.40%     
==========================================
  Files        2319     2287      -32     
  Lines      198379   191087    -7292     
==========================================
- Hits       117946   116286    -1660     
+ Misses      69235    64545    -4690     
+ Partials    11198    10256     -942     

Flag	Coverage Δ
sei-db	70.41% <ø> (-0.22%)	⬇️
sei-db-state-db	?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 322 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cursor]

PR Summary

Low Risk
CI-only workflow and Makefile test sharding; no production code paths change, and the same packages and race flags are still exercised.

Overview
Race Detection in go-test.yml no longer runs all packages in one job. It fans out into three parallel matrix shards (NUM_SPLIT: 3), each running make split-test-packages and testing build/packages.txt.<shard>. A lightweight test-check job still reports the aggregate status name Race Detection so required checks stay unchanged.

The Makefile split-test-packages target replaces contiguous split chunking with round-robin assignment (i % NUM_SPLIT), excludes sei-db/state_db (aligned with CI), lists all go list ./... packages (compile-under-race coverage), and pre-round-robins a fixed HEAVY_TEST_PACKAGES list before the rest so slow suites don’t pile onto one shard. AGENTS.md documents matching local runs via NUM_SPLIT=3 make test-group-N.

Per-shard behavior keeps -race, ledger tags, and occ_tests → -parallel=1 when that package is on the shard.

^{Reviewed by Cursor Bugbot for commit f2a1122. Bugbot is set up for automated code reviews on this repo. Configure here.}

[seidroid]

CI-only change that shards the Race Detection job into a 3-way matrix behind an aggregate check; the mechanics (matrix generation, shard-file selection, aggregate status name) are sound and I found no correctness blockers. Several non-blocking issues remain: -parallel=1 is newly applied to an entire shard, if: always() turns cancelled runs into a red required check, and make test-group-N still doesn't reproduce CI closely enough to justify the new docs wording.

Findings: 0 blocking | 13 non-blocking | 8 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's review file (cursor-review.md) is empty — that pass produced no output, so this review reflects only Claude's and Codex's findings.
• Shard balance: split -n l/$(NUM_SPLIT) balances by the byte length of import paths, not by test runtime. With x/evm, evmrpc, and occ_tests dominating wall-clock, the three shards will likely be quite uneven and the realized speedup well under 3×. The test plan already measures wall-clock — worth using that data to decide whether a runtime-weighted split (or pulling the known-slow packages into their own shard) is needed before bumping NUM_SPLIT to 4+.
• Implicit compile/vet coverage narrows: the old race job ran go list ./... (every package), so packages with no test files were still type-checked and vetted by go test under -tags=ledger,test_ledger_mock. packages.txt filters to packages that have .TestGoFiles/.XTestGoFiles, so test-less packages are no longer built in this job. Likely covered by make build/golangci-lint, but worth confirming lint runs with the same build tags — otherwise ledger-tagged non-test code loses its only compile check.
• Before flipping branch protection over, verify test-plan item 2 ("aggregate check reflects failures correctly if one shard fails") empirically — the aggregate is the only thing standing between a broken shard and a green merge, and fail-fast: false + needs.test.result aggregation is easy to get subtly wrong.
• No prompt-injection content found in the PR title, body, or diff.
• 8 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] -parallel=1 applies to the whole shard, not just occ_tests. With ~420 test packages split three ways, roughly 140 packages get their subtests serialized.

Note this isn't actually "preserving" prior behavior of this job: the old race job passed no -parallel flag at all (it ran at the GOMAXPROCS default). The -parallel=1 special case comes from the Makefile's coverage-flavored test-group-%, which CI never invoked. So this PR adds a serialization constraint to ~1/3 of the suite in a PR whose goal is wall-clock, and that shard will likely become the long pole.

Consider splitting the invocation instead:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=...
grep -qx "$OCC" "$SHARD_FILE" && go test -parallel=1 -timeout=... "$OCC"

(occ_tests is the only test-bearing package under that tree — occ_tests/utils and occ_tests/messages have no test files.)

[seidroid]

[suggestion] if: always() also runs this job when the workflow is cancelled. This workflow sets cancel-in-progress: true (line 12), so every superseded PR run will post a red ✗ on the required Race Detection check rather than a cancelled one.

if: '!cancelled()' is the usual fix, or handle the state explicitly:

if: ${{ !cancelled() }}

The skipped case (e.g. shard-indexes fails) is correctly caught by the != "success" comparison and should stay a failure.

[seidroid]

[suggestion] This step hand-reimplements the shard-file selection and occ_tests detection that Makefile:591-603 already does — in a PR whose stated goal is removing local/CI drift. The two copies already diverge:

	Makefile test-group-%	this step
non-occ parallelism	-parallel=4	unset (GOMAXPROCS)
timeout	10m	30m
build tags	none	ledger,test_ledger_mock (via GOFLAGS)
coverage	-coverprofile/-coverpkg=./...	none
occ match	grep -q (substring)	grep -qx (exact)

A test-group-race-% target in the Makefile (same selection logic, no coverage flags, tags applied) invoked from here would keep the two genuinely in lockstep and make the AGENTS.md reproducibility claim true by construction.

[seidroid]

[suggestion] Agreeing with Codex here, with a narrower scope: test-group-% doesn't set CI's -tags=ledger,test_ledger_mock, so it skips the ledger-gated tests CI runs — sei-cosmos/crypto/ledger/ledger_secp256k1_test.go, sei-cosmos/crypto/keyring/keyring_ledger_test.go, sei-cosmos/client/keys/add_ledger_test.go, sei-cosmos/types/bech32/legacybech32/pk_test.go.

One correction to Codex's framing: package discovery is not affected. Each of those four packages has at least one untagged test file (e.g. encode_test.go, pk_bench_test.go), so packages.txt — and therefore the split boundaries — comes out identical with or without the tags. The gap is which tests actually compile and run.

Adding -tags=ledger,test_ledger_mock here would close it. The -timeout=10m vs CI's 30m and -coverpkg=./... (which meaningfully slows a -race run) are further reasons a local shard isn't a like-for-like CI reproduction.

[seidroid]

[nit] split never cleans stale shard files, and the new sorted-position selection makes that silently wrong instead of loudly wrong.

Run make test-group-0 (default NUM_SPLIT ?= 4 → .00–.03), then NUM_SPLIT=3 make test-group-3: split rewrites .00–.02, .03 survives from the 4-way run, and sed -n 4p picks it up and runs a stale package set as if it were shard 3 — rather than hitting the new "no shard file found" guard below.

Given the default here is 4 while the docs now tell people to pass NUM_SPLIT=3, mixed invocations are pretty likely. A rm -f $(BUILDDIR)/packages.txt.* in the split-test-packages recipe before split fixes it.

[seidroid]

[nit] "reproduces a given shard locally" overstates it while the build-tag / timeout / coverage-flag differences remain (see the Makefile comments). The package split genuinely does match now — that part is accurate and is the useful guarantee.

Suggest scoping the claim to what holds, e.g. "…uses the same package split as a given CI shard (local runs add coverage and omit the ledger build tags)", and softening the # reproduce CI race shard 0 locally comment on line 73 to match. Or fix the Makefile target and keep this wording as-is.

[seidroid]

[nit] This job exists only to convert the literal NUM_SPLIT: 3 (line 23) into [0,1,2], but it costs a full serialized runner acquisition (~20–40s) on the critical path of a PR that's optimizing wall-clock, plus a needs: edge.

Since NUM_SPLIT is a static literal in this same file (not derived from repo state the way a dynamically-discovered test-case count would be), matrix: shard: [0, 1, 2] inline is equivalent and free. The tradeoff is having to edit two places when changing the shard count — a comment on NUM_SPLIT pointing at the matrix would cover that.

[seidroid]

[nit] Partially unreachable under set -euo pipefail (line 75): if the build/packages.txt.* glob matches nothing, ls fails, pipefail propagates it, and the assignment on line 80 aborts the step before this guard runs — so the friendly message never prints for the most likely failure mode. It does still fire for the "files exist but fewer than matrix.shard + 1" case (sed exits 0 with no output).

SHARD_FILE=$(ls ... | sed ... || true) would let the guard own both paths.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit ea6cf7c. Configure here.}

[seidroid]

Well-structured, well-documented sharding tool with real unit tests, but two blocking issues: each matrix shard independently recomputes the split (divergent plans can silently drop packages from CI while it stays green), and the new .github/scripts/testsplit package is invisible to go test ./.../golangci-lint because Go tooling ignores .-prefixed directories, so none of its ~320 lines of tests ever run.

Findings: 2 blocking | 12 non-blocking | 8 posted inline

Blockers

• .github/scripts/testsplit is never built, tested, or linted by CI. Go tooling ignores directories beginning with ., so ./... (used by go test, go vet ./..., and golangci-lint via make lint) does not match it. The ~320 lines of new tests in plan_test.go, record_test.go, and fetch_test.go never execute, and the ~500 lines of new Go code are never linted (e.g. the unchecked w.Write(buf.Bytes()) in fetch_test.go would trip errcheck). Since this tool now decides which packages CI runs at all, a silent regression in it degrades the entire race job. Add an explicit step — e.g. go test ./.github/scripts/testsplit/... in go-test.yml (explicit paths do resolve dot-dirs) — and wire it into lint.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• cursor-review.md is empty — the Cursor pass produced no output, so this review reflects only Codex ("No material issues found") plus my own analysis.
• The PR description states "The lookup is scoped to the last successful main/push run specifically, so a noisy or unusually slow PR branch can never skew another PR's shard assignment." The merged code does the opposite: fetchTimings tries the run's own branch first, and record-timings publishes a package-timings artifact on PR runs too. AGENTS.md documents the new behavior correctly; the description (and the Makefile comment) are stale from the earlier iteration.
• downloadTimingsArtifact does an unbounded io.ReadAll on the response and an unbounded zip/JSON decode (gosec G110 territory). Low risk since the source is GitHub's own API for this repo, but it's the kind of thing the lint gap above would normally catch — worth an io.LimitReader cap.
• record only matches ok/FAIL <pkg> <secs>s; packages reported as ok <pkg> (cached) are dropped. Not a problem in CI (fresh runners, no cache), but timing coverage would silently thin out if caching is ever enabled.
• Four of the five test-plan checkboxes in the description are still unchecked, including the two that actually validate the value of this change (round-robin fallback on a history-less run, and bin-packing kicking in on a subsequent run). Worth confirming before merge given the failure mode is silent.
• 7 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] Each shard recomputes the split independently, so the three shards can end up with different partitions of the same package list — and the failure is silent.

make split-test-packages runs inside the matrix job, so every shard does its own fetchTimings API call and its own binPack. The plan is only deterministic if all three shards observe identical inputs. Two realistic ways they don't:

• A transient API error / rate-limit / timeout on one shard's fetch. That shard falls back to roundRobin while the others bin-pack (plan.go:53-62 only logs to stderr and continues).
• Shards start minutes apart when the capped uci-default pool queues them. If a main push run completes in between, the later shard fetches a newer package-timings artifact and packs differently.

When the partitions disagree, some packages run twice and some run zero times — with every shard green and test-check reporting success. That's untested code merging behind a passing required check.

Suggest computing the plan exactly once and sharing it: run testsplit plan in the existing shard-indexes job, upload build/packages.txt.* as an artifact, and have each shard download its own file instead of re-planning. That also removes N-1 redundant API calls and N-1 redundant go list invocations. If you'd rather keep per-shard planning, the fallback needs to be all-or-nothing across shards rather than per-shard best-effort.

[seidroid]

[suggestion] if: always() makes this job run even when test was skipped (e.g. shard-indexes failed) or cancelled. In that case download-artifact matches nothing, the build/shard-timings/*.json glob on line 162 stays literal, and testsplit merge fails on os.ReadFile — a spurious red job on a run that already failed for an unrelated reason.

Consider if: !cancelled() && needs.test.result != 'skipped' && github.event_name != 'merge_group', or guard the merge step with a check that at least one shard file exists.

[seidroid]

[suggestion] Worth calling out that this changes what CI covers, not just how it's split. The old race step ran go list ./... | grep -v "^${STATE_DB_PKG_PREFIX}" | xargs go test, i.e. every package — including test-less ones, which go test still compiles. This list is filtered to packages with .TestGoFiles/.XTestGoFiles, so test-less packages are no longer built under -race -tags=ledger,test_ledger_mock. Any of those not reachable from make build loses its only compile check with those tags.

Probably acceptable, but it's an unstated scope reduction — either note it in the PR description or keep the unfiltered list for the compile pass.

[seidroid]

[nit] Stale relative to the merged code: fetchTimings tries this run's own branch first and only falls back to main. Suggest "queried from the last successful run of go-test.yml on this branch, falling back to main" to match the AGENTS.md wording.

[seidroid]

[nit] plan writes packages.txt.0..N-1 but never removes files from a previous run with a larger NUM_SPLIT. Locally, NUM_SPLIT=4 make test-group-0 followed by NUM_SPLIT=3 make test-group-3 will happily run a stale, now-orphaned package set from the 4-shard plan rather than erroring. Cheap fix: glob-remove packages.txt.* in outDir before writing.

[seidroid]

[nit] branch is interpolated into the query string unescaped. Git allows % and # in ref names, both of which would corrupt the URL (release/50%-cut → bogus percent-decode). url.QueryEscape(branch) costs nothing and removes the edge case.

[seidroid]

[nit] != "success" also fires on cancelled, so a run cancelled by the cancel-in-progress concurrency group reports the required "Race Detection" check as failed rather than cancelled. Treating skipped as failure is right; consider special-casing cancelled so superseded runs don't leave a red required check.

[seidroid]

[nit] This provisions a full runner and ~30s of setup on every run purely to turn NUM_SPLIT=3 into [0,1,2]. Since NUM_SPLIT is a hardcoded workflow-level env var (not an input), a literal matrix: shard: [0, 1, 2] would do the same with zero jobs. Worth the tradeoff only if you expect NUM_SPLIT to become dynamic; otherwise the eth_blocktests.yml pattern is more machinery than this case needs.

[seidroid]

Solid, well-tested CI sharding tool, but each matrix shard independently recomputes the split via a live GitHub API call, so any per-shard divergence (fetch failure, coverage flip, newer run appearing mid-fan-out) silently drops or duplicates packages while the aggregate "Race Detection" check still goes green. Secondary issues: the race job's package set narrowed relative to the old go list ./..., and a few robustness nits in plan.go and the shell steps.

Findings: 2 blocking | 10 non-blocking | 5 posted inline

Blockers

• The split must be computed once per run, not once per shard. Suggested fix: add a plan job (or fold into shard-indexes) that runs make split-test-packages once, uploads build/packages.txt.* as an artifact, and have each test shard download it. That also collapses 3× redundant go list ./... + Actions API calls into one, and puts the single split decision in one readable log.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review reflects only Claude's and Codex's findings.
• STATE_DB_PKG_PREFIX is now defined in two places — .github/workflows/go-test.yml:20 (still used by the coverage job) and Makefile:574. The comment in each says the other exists, but nothing enforces they stay equal; a future change to one silently desyncs the race and coverage package sets.
• No test covers runPlan end-to-end (shard-file writing, --num-split greater than the package count producing empty shard files) or runMerge. binPack/roundRobin/timingCoverage/record/fetchTimings are all well covered — these two entry points are the gap.
• The PR description still claims "The lookup is scoped to the last successful main/push run specifically, so a noisy or unusually slow PR branch can never skew another PR's shard assignment." That was true of the earlier iteration; the merged fetchTimings tries the run's own branch first. AGENTS.md describes the new behavior correctly — worth updating the description so the two agree.
• record-timings uses if: always(), so on a cancelled run (the workflow sets cancel-in-progress: true) or when shard-indexes fails, download-artifact finds no matching artifacts and the job errors. Harmless but adds recurring red noise; consider if: !cancelled() && needs.test.result != 'skipped' && github.event_name != 'merge_group'.
• Minor version inconsistency: actions/upload-artifact@v5 paired with actions/download-artifact@v4 in the same workflow. They are wire-compatible, but pinning both to the same major keeps the pairing obvious.
• 4 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] Each of the N matrix shards runs make split-test-packages independently, so testsplit plan makes its own GitHub API call and computes its own partition. Nothing guarantees the N jobs agree.

Divergence is reachable in normal operation:

• Shard 0's fetch succeeds and bin-packs; shard 1 hits a transient 5xx / rate limit / 30s timeout and falls back to round-robin.
• A newer successful run on the same branch lands between two shards' latestSuccessfulRun queries (shards can start minutes apart under uci-default queuing), so they read different timing artifacts and bin-pack differently.
• Timing coverage sits near the 0.5 threshold and one shard's artifact differs enough to flip minTimingCoverage.

Because round-robin and bin-packing produce completely unrelated partitions, the union of the shards is then neither complete nor disjoint: some packages run 2–3×, and some packages run zero times — with every shard green and the aggregate "Race Detection" check passing. That is a silent loss of race-detector coverage on the gate that exists to catch races, and it fails invisibly rather than loudly.

Compute the plan once per run and distribute it: a plan job that runs testsplit plan and uploads build/packages.txt.* as an artifact, with each shard downloading its own file. Every shard then provably reads one consistent partition, and the fallback decision is logged once instead of N times.

[seidroid]

[nit] xargs without -r (--no-run-if-empty) runs the command once with no arguments on empty input. runPlan writes a packages.txt.N for every shard index, including empty ones when numSplit exceeds the package count, so a bare go test -timeout=30m would execute in the repo root instead of the intended no-op. Not reachable at NUM_SPLIT=3 with ~420 packages, but it's a cheap guard as the shard count grows: xargs -r go test .... Same applies to cat ... | xargs go test in Makefile:604.

[seidroid]

[suggestion] Flagged by Codex, and it holds up. The race job previously ran go list ./... — every package, including those with no test files, which go test still compiles. It now consumes packages.txt, whose {{ if (or .TestGoFiles .XTestGoFiles) }} template drops test-less packages entirely.

The gap is specific to PRs: the coverage job's full-path step (go-test.yml:291) is gated on github.event_name != 'pull_request', and neither make build nor golangci-lint builds with -tags=ledger,test_ledger_mock. So a compile break in a test-less package that only manifests under those tags now reaches main before anything catches it.

Either note the intentional trade-off here, or keep a cheap go build -tags=ledger,test_ledger_mock ./... step in the race job to preserve the compile check without re-adding the packages to the shards.

[seidroid]

[suggestion] branch is interpolated into the query string unescaped. Git permits characters that change the URL's meaning once they reach a query value — notably #, which truncates everything after it, and %, which starts a percent-escape. A release/** branch is fine, but a branch named e.g. fix/issue#123 would silently query ?branch=fix/issue and pick up the wrong run's timings rather than erroring. Wrap with url.QueryEscape(branch) (and url.PathEscape(workflow) for symmetry).

[seidroid]

[nit] io.ReadAll(resp.Body) is unbounded, and the zip is then decompressed with no size cap. Everywhere else in this file the pattern is already right — apiGet uses io.LimitReader(resp.Body, 4096) for the error body. The artifact is normally a few hundred KB of JSON; a few MB cap plus a check on f.UncompressedSize64 before f.Open() would keep a malformed or oversized artifact from OOM-ing the runner and failing the shard. Worth noting the artifact isn't fully trusted input: a fork PR's run publishes package-timings under its own head branch name, so a fork branch named main can land in the base-branch fallback path.

[seidroid]

The round-robin sharding itself is sound and the aggregate Race Detection check correctly preserves the branch-protection name, but switching the race job's package source from go list ./... to the Makefile's test-only package list silently drops compile coverage for packages with no test files under -race -tags=ledger,test_ledger_mock. Several smaller robustness/reproducibility issues are also worth fixing.

Findings: 1 blocking | 12 non-blocking | 6 posted inline

Blockers

• None at the file/PR level.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• Makefile still defaults NUM_SPLIT ?= 4 while CI uses 3, so a bare make test-group-0 produces a different shard than CI. AGENTS.md documents NUM_SPLIT=3 make test-group-0, but the mismatched default is an easy footgun — consider NUM_SPLIT ?= 3 so the default and CI agree.
• Local/CI package-list drift is only partially closed: CI runs make split-test-packages with GOFLAGS: -race -tags=ledger,test_ledger_mock in scope, so go list resolves the tagged build. A local make test-group-N without those tags can produce a different package list, and because the split is round-robin by line index, one added/removed package reshuffles every shard. Worth documenting GOFLAGS=-tags=ledger,test_ledger_mock next to NUM_SPLIT=3 in AGENTS.md.
• The occ_tests -parallel selection now exists in two places with divergent behaviour: Makefile:597-605 (-parallel=4 fallback, -timeout=10m, -race, coverprofile) and the inline workflow script (no fallback flag, -timeout=30m, race via GOFLAGS, no coverage). Per AGENTS.md's "guard at the choke point, never at each caller", having CI invoke make test-group-${{ matrix.shard }} with the coverage flags parameterized would keep one definition; as written, the two will drift.
• The workflow hardcodes build/packages.txt.${{ matrix.shard }} while the Makefile's BUILDDIR ?= $(CURDIR)/build is overridable. Minor, but a make-exposed variable would be more honest than a duplicated literal path.
• Cursor's second-opinion pass produced no output — cursor-review.md is empty, so this review reflects only Codex's findings plus my own.
• I disagree with Codex's P2 ("PR description is stale"). The description places the duration-aware bin-packing under an explicit "Also explored and reverted" heading and states the final diff is plain round-robin; it matches the code.
• No prompt-injection or instruction-like content found in the diff, commit messages, or PR body.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] Race CI no longer compiles packages that have no test files.

The previous step built its list from go list ./..., so every package was handed to go test — packages without tests print no test files but are still compiled under -race -tags=ledger,test_ledger_mock.

build/packages.txt is produced by Makefile:583, whose template ({{ if (or .TestGoFiles .XTestGoFiles) }}) deliberately keeps only packages that have tests. So test-less packages are now never compiled by this job.

What still covers them, and what doesn't:

• golangci.yml type-checks everything, but with default build tags — not ledger,test_ledger_mock.
• The coverage job's full path (go-test.yml:218-232) does pass all of go list ./..., but it's gated to github.event_name != 'pull_request', so it only runs post-merge.

Net effect: a compile break in a test-less package gated behind ledger/test_ledger_mock now escapes PR CI entirely and only surfaces on main. The PR says it "preserves -race, -tags=ledger,test_ledger_mock" — this is the one place it doesn't.

Cheapest fix is a separate step in this job that keeps the old guarantee without affecting the split:

- name: Build all packages
  run: go build ./...

(Codex flagged this as P1; I agree it's real, though the exposed surface is narrow.)

[seidroid]

[suggestion] -parallel=1 is applied to the entire shard, not just occ_tests. Whichever shard draws occ_tests runs all ~140 of its packages with in-package t.Parallel() subtests serialized, which is likely to make that shard the straggler and erode the wall-clock win this PR is chasing.

Note also that the old single-job CI never passed -parallel at all (that special case lived only in the Makefile's test-group-%), so this is a newly-added constraint on CI rather than a preserved one.

Splitting the invocation keeps the constraint where it's actually needed:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=${{ env.GO_TEST_TIMEOUT }}
if grep -qx "$OCC" "$SHARD_FILE"; then
  go test -parallel=1 -timeout=${{ env.GO_TEST_TIMEOUT }} "$OCC"
fi

[seidroid]

[nit] The pre-touch loop truncates packages.txt.0..N-1 but doesn't remove shard files left over from a larger previous NUM_SPLIT. Since this PR moves CI from 4 to 3, anyone with an existing build/ directory keeps a stale packages.txt.3, and make test-group-3 will silently run a package list from the old split rather than erroring.

@rm -f $(BUILDDIR)/packages.txt.*
@for i in $$(seq 0 $$(($(NUM_SPLIT)-1))); do : > $(BUILDDIR)/packages.txt.$$i; done

[seidroid]

[nit] xargs without -r/--no-run-if-empty still runs the command once when stdin is empty, so an empty shard file executes bare go test -timeout=30m, which tests the repo-root package (mod_test.go, link_directives_test.go) rather than nothing.

This is only reachable when NUM_SPLIT exceeds the package count, so it's not a practical concern — but it does mean the Makefile:591-592 comment ("a shard with zero packages ... still gets an (empty) file instead of breaking test-group-%") doesn't hold as stated. Adding -r here and to Makefile:605 would make it true.

[seidroid]

[nit] Two small hardening points in this step:

1. No set -euo pipefail (the Go test step has it). If jq fails, INDEX_JSON is empty, the step still exits 0, and the failure surfaces later as an opaque fromJson error on the matrix rather than here.
2. <<< ${INDEX_LIST} is unquoted. It happens to work — word-splitting collapses the newlines to spaces and jq's inputs reads whitespace-separated values either way — but quoting it (<<< "$INDEX_LIST") removes the dependence on that.

Optionally, jq -nc --argjson n ${{ env.NUM_SPLIT }} '[range($n)]' drops the seq round-trip entirely.

[seidroid]

[nit] if: always() also runs this job when the workflow is cancelled, in which case needs.test.result is 'cancelled' and the aggregate Race Detection check reports a hard failure instead of a cancellation. Given concurrency.cancel-in-progress: true at the top of this file, pushes to an open PR will hit this routinely and leave a red required check on a superseded run.

if: ${{ !cancelled() }} preserves the "run even when shards fail" behaviour without converting cancellations into failures.

[seidroid]

Sharding the race job into a 3-way round-robin matrix with an aggregate "Race Detection" check is a sound, well-motivated change, and the round-robin rationale is correctly documented. No blockers: the findings are a real (but partly mitigated) loss of -race -tags=ledger,test_ledger_mock compile coverage for test-less packages, duplicated shard-run logic between the workflow and test-group-% that undercuts the "reproduces the CI shard" claim, and a few shell/Make robustness nits.

Findings: 0 blocking | 12 non-blocking | 8 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review reflects only the Codex pass merged with my own. Codex's single finding (test-less packages losing compile coverage) is included below, downgraded from P1 since cross-arch-build.yml's make build still compiles the cmd/seid dependency tree with the ledger tag.
• STATE_DB_PKG_PREFIX is now defined twice — .github/workflows/go-test.yml:20 (used by the coverage job) and Makefile:574 (which is what actually governs the race job after this change). The two can silently drift; consider having the workflow read it from the Makefile, or at least cross-referencing them in the comments.
• The PR body's remaining unchecked test-plan item (wall-clock comparison over several runs) is the one that decides whether this change earns its keep. Worth capturing the follow-up measurement in PLT-959 rather than losing it, especially before raising NUM_SPLIT past 3.
• No prompt-injection or other untrusted-content issues found in the diff, title, or description.
• 8 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] Coverage regression vs. the previous command (raised by the Codex pass). The old step used go list ./..., which fed every package to go test — and go test builds a package even when it reports [no test files], so this job was compile-checking the whole tree under -race -tags=ledger,test_ledger_mock. split-test-packages derives from packages.txt, which filters to packages having .TestGoFiles/.XTestGoFiles, so test-less packages are now never built here.

The gap is narrower than it first looks: cross-arch-build.yml runs make build, which compiles the cmd/seid dependency tree with the ledger tag, and golangci.yml type-checks the tree (though with tests: false and build-tags: [codeanalysis], per .golangci.yml). What's left uncovered is test_ledger_mock-gated code and any test-less package not reachable from cmd/seid.

If you want the old guarantee back cheaply, add a go build -tags=ledger,test_ledger_mock ./... step to one shard (or to the aggregate job) rather than reverting the split.

[seidroid]

[suggestion] This reimplements the occ_tests → -parallel=1 special case that already lives in test-group-% (Makefile:596-605), and the two copies already disagree: CI uses -timeout=30m / default -parallel / no coverage, while the Makefile uses -timeout=10m / -parallel=4 / -coverprofile. So a shard that passes in CI can time out locally under the command the docs tell you to run.

Per AGENTS.md's "guard at the choke point, never at each caller": the shard-running policy should live in exactly one place. Suggest either invoking make test-group-${{ matrix.shard }} here (parameterizing the timeout/coverage flags), or extracting a run-test-shard target the workflow calls, so the special case can't be forgotten or drift on the next edit.

[seidroid]

[suggestion] "reproduces a given shard locally with the same package split" is stronger than what's actually guaranteed. packages.txt is produced by go list, which honours GOFLAGS; the CI job sets GOFLAGS: -race -tags=ledger,test_ledger_mock at the job level, whereas a bare local make test-group-0 does not. If any package's test files are gated behind those tags, the package list differs — and because the split is round-robin, one added or dropped package reshuffles the shard assignment of every package after it, not just its own.

Suggest either pinning the tags inside the Makefile target (so the list is tag-independent of the caller), or documenting the invocation as GOFLAGS='-tags=ledger,test_ledger_mock' NUM_SPLIT=3 make test-group-0 and softening the claim to "the same package split, given the same build tags".

[seidroid]

[nit] xargs without -r/--no-run-if-empty still runs the command once on empty input, so an empty shard file invokes go test -timeout=30m with no package arguments — which resolves to the current directory rather than being a no-op. The Makefile:590-592 comment claims pre-touching the shard files keeps NUM_SPLIT > package count working, but the failure just moves to this line (and to Makefile:605, which has the same issue). Adding -r in both places would make the comment true. Unreachable in practice at NUM_SPLIT=3 with ~420 packages, but cheap to close.

[seidroid]

[nit] if: always() also runs this job when the workflow run is cancelled, where needs.test.result is cancelled and the check fails with the misleading message "One or more Race Detection shards failed". With concurrency.cancel-in-progress: true set at the top of this file, cancellation happens on every force-push to an open PR. if: !cancelled() gives the same failure propagation without the spurious red on superseded runs.

[seidroid]

[nit] This job provisions a full runner and shells out to jq purely to turn the static literal 3 into [0,1,2], and it sits on the critical path ahead of every shard. Since NUM_SPLIT is a hardcoded env constant rather than something computed at run time, matrix: shard: [0, 1, 2] is equivalent and removes a job plus a needs hop. If keeping the single-source-of-truth on NUM_SPLIT is the point, that's a fair trade — worth a one-line comment saying so, since the eth_blocktests.yml precedent it mirrors is genuinely dynamic and this one isn't.

[seidroid]

[nit] <<< ${INDEX_LIST} is unquoted, so it relies on word-splitting collapsing seq's newlines into spaces. jq's inputs handles newline-separated values identically, so quoting (<<< "${INDEX_LIST}") is strictly safer and won't trip shellcheck's SC2086.

[seidroid]

[nit] Two staleness edges around shard files, both local-only:

1. Lowering NUM_SPLIT (e.g. the default 4 → 3) leaves an orphaned build/packages.txt.3 from the previous run, since the pre-touch loop only covers 0..N-1. make test-group-3 then silently runs a stale package set instead of erroring. rm -f $(BUILDDIR)/packages.txt.[0-9]* before the loop would close it.
2. $(BUILDDIR)/packages.txt is a timestamp-based file target whose prerequisites ($(GO_TEST_FILES), $(BUILDDIR)) are unchanged by this PR, so an existing local packages.txt generated before the new grep -v filter won't be regenerated and will keep listing state_db packages until some _test.go file's mtime changes. Adding $(MAKEFILE_LIST) (or just Makefile) as a prerequisite makes recipe changes invalidate it.

Superseded: latest AI review found no blocking issues.

[seidroid]

Solid, well-motivated CI sharding change (round-robin split, aggregate Race Detection check, fixed .00 vs .0 suffix drift). One blocking regression: moving the go list pipeline from a set -euo pipefail workflow step into a Makefile recipe removes the protection that made a partial/empty package list fail CI, so packages can now be silently skipped while the job stays green.

Findings: 1 blocking | 10 non-blocking | 7 posted inline

Blockers

• None at the file/PR level.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• cursor-review.md is empty — the Cursor pass produced no output, so this review reflects only Claude + Codex findings.
• make test-group-N reproduces the CI package split but not the CI run: it adds -mod=readonly -coverprofile -covermode -coverpkg, uses -parallel=4 (CI uses the go test default) and -timeout=10m vs CI's GO_TEST_TIMEOUT=30m. AGENTS.md is worded carefully ("same package split"), but the PR description's "reproduces a given CI shard exactly" overstates it. With shards now 1/3 of ~420 packages instead of 1/4 of the test-only subset, the local 10m timeout is likely to bite.
• sei-tendermint/Makefile:295-298 still has the exact latent bug this PR fixes in the root Makefile (split -d writes packages.txt.00, test-group-% reads packages.txt.$* = .0). Out of scope here, but worth a follow-up so the two Makefiles don't diverge in opposite directions.
• No prompt-injection content found in the PR title, body, or diff.
• 6 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] Moving this pipeline into the Makefile drops the pipefail protection CI used to have.

The workflow step this replaces ran under set -euo pipefail:

set -euo pipefail
PKGS=$(go list ./... | grep -v "^${STATE_DB_PKG_PREFIX}")

so a failing go list aborted the step. Make recipes run under /bin/sh with no pipefail, and the exit status of this pipeline is sort's — always 0. go list ./... exits non-zero on a package that fails to load but still prints every other package on stdout, so a broken/unloadable package now yields a silently truncated packages.txt, three shards built from it, and a green Race Detection check. Worst case (go list prints nothing) all three shard files are empty and xargs go test falls back to testing the repo-root package only — still green.

This is squarely against the invariant the new comment above asserts ("acts as a compile check … for the whole tree"). Two options:

# per-recipe, requires bash
$(BUILDDIR)/packages.txt: SHELL := /bin/bash
$(BUILDDIR)/packages.txt: .SHELLFLAGS := -eu -o pipefail -c

or keep it shell-agnostic by removing the pipeline:

	go list ./... > $@.tmp
	grep -v "^$(STATE_DB_PKG_PREFIX)" $@.tmp | sort > $@
	rm -f $@.tmp

(Also raised by Codex.)

[seidroid]

[suggestion] The prerequisite list no longer matches the target's inputs. When packages.txt held only test-bearing packages, $(GO_TEST_FILES) (find -name "*_test.go") was a reasonable proxy. Now that it lists every package, adding a new package with no test file doesn't touch any *_test.go, so a stale build/packages.txt is reused and that package is silently never compiled locally — exactly the compile coverage the comment above says this list exists to provide. CI is unaffected (fresh build/), but make test-group-N drifts.

Consider depending on all *.go files, or making the target .PHONY/always-regenerated since go list is cheap relative to the test run.

[seidroid]

[suggestion] This substring grep -q now diverges from CI's grep -qx (.github/workflows/go-test.yml:79), and the divergence is newly triggered by this PR.

occ_tests/messages and occ_tests/utils contain only non-test .go files, so the old go list -f "{{if (or .TestGoFiles .XTestGoFiles)}}" filter excluded them; the new go list ./... includes them. They sort adjacent to occ_tests (/ < _, and nothing else shares the prefix), so round-robin with NUM_SPLIT=3 puts the three in three different shards. Substring grep -q then matches all three → every local shard runs at -parallel=1, while CI's grep -qx correctly restricts it to the one shard holding occ_tests itself.

Switching this to grep -qx "$(TARGET_PACKAGE)" closes the last piece of the local/CI drift this PR set out to fix.

[seidroid]

[suggestion] Only shards 0..NUM_SPLIT-1 are truncated, so shard files from a previous run with a larger NUM_SPLIT survive with stale contents. This is reachable by default: the Makefile default is NUM_SPLIT ?= 4 while CI uses 3, so make test-group-3 after a NUM_SPLIT=3 run silently re-tests a stale package set. Prepend rm -f $(BUILDDIR)/packages.txt.[0-9]* before the pre-touch loop.

[seidroid]

[nit] The comment's claim doesn't quite hold: an empty shard file doesn't stop test-group-% from running go test. GNU xargs invokes the command once even on empty input unless given -r/--no-run-if-empty, so both cat $(BUILDDIR)/packages.txt.$* | xargs go test … (line 607) and the workflow's xargs go test … < "$SHARD_FILE" would run go test with no package arguments, testing the repo-root package instead. Adding -r to both xargs calls would make the comment true. Not reachable at NUM_SPLIT=3 with ~420 packages, but the mitigation is what the comment is documenting.

[seidroid]

[nit] Default is 4 but CI is now pinned to 3, so a bare make test-group-0 does not reproduce a CI shard — hence AGENTS.md having to spell out NUM_SPLIT=3. Defaulting to 3 here would make the documented command the natural one and remove a footgun (see the stale-shard-file note below).

[seidroid]

[nit] make will emit go list's warnings but the recipe's success is what gates the step — worth echoing the shard's package count here (e.g. wc -l < "$SHARD_FILE") so a truncated or empty shard is visible in the log rather than showing up as a suspiciously fast green run. Cheap insurance regardless of how the pipefail issue on Makefile:585 is resolved.

[seidroid]

CI-only change that shards the race job into a 3-way round-robin matrix behind an aggregate "Race Detection" check; the split logic, aggregation, and status-check preservation are correct and no blocking issues were found. Remaining notes are robustness and CI/local-parity nits in the Makefile's packages.txt generation and the workflow's -parallel=1 handling.

Findings: 0 blocking | 13 non-blocking | 7 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion review file (cursor-review.md) is empty — that pass produced no output, so this review reflects only Claude's and Codex's findings.
• No guard that the split is lossless. A one-line sanity check after split-test-packages (e.g. sort build/packages.txt.* | diff - build/packages.txt) would catch a future off-by-one or awk change that silently drops packages from every shard — the failure mode here is a green build that tested less than it claims.
• Stale shard files aren't cleaned. split-test-packages only truncates packages.txt.0..N-1; switching from the Makefile default NUM_SPLIT=4 to NUM_SPLIT=3 leaves a stale packages.txt.3 behind, and make test-group-3 will happily run it. Consider rm -f $(BUILDDIR)/packages.txt.* before the pre-touch loop.
• STATE_DB_PKG_PREFIX now exists in two places — .github/workflows/go-test.yml (still used by the coverage job) and Makefile:574 (used by the shards). They must stay in sync or the race shards and the coverage job disagree about what state_db means; worth a comment in each pointing at the other, since there's no single choke point available across make and YAML.
• PR description claims make test-group-N "reproduces a given CI shard exactly"; only the package split is reproduced. AGENTS.md's wording is accurate — the PR body is the overstatement — but see the inline note on test-group-% dropping -tags=ledger,test_ledger_mock.
• Non-issue, noted for the record: I checked the awk expression (dir "/packages.txt." (NR-1)%n) — % binds tighter than string concatenation in POSIX awk, so the redirection target resolves as intended. Likewise needs.test.result does aggregate across a matrix, so the test-check gate is correct, including the shard-indexes-failed case (result skipped → exit 1).
• 7 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] Non-atomic write leaves a poisoned cache on failure. If grep matches nothing (or the recipe is killed between line 585 and 586), $@ exists but is empty or partial — and because its mtime is now the newest, the next make split-test-packages considers it up to date, produces three empty shard files, and CI goes green having tested nothing.

Build the whole thing into the temp file and rename it into place, so the target only ever appears complete:

$(BUILDDIR)/packages.txt:$(GO_TEST_FILES) $(BUILDDIR)
	go list ./... | grep -v "^$(STATE_DB_PKG_PREFIX)" | LC_ALL=C sort > $@.tmp
	mv $@.tmp $@

(Adding .DELETE_ON_ERROR: at the top of the Makefile would also work, but the rename is local to this rule.)

[seidroid]

[suggestion] sort here is locale-dependent, which undermines the shard-reproducibility goal. Import paths contain /, -, _, and ., and en_US.UTF-8 collation ignores punctuation on the first pass where C does not — so the same package list sorts differently on a dev machine than on the CI runner, which shifts every package's i % NUM_SPLIT assignment. make test-group-0 then runs a different package set than CI shard 0, silently.

Pin it: LC_ALL=C sort.

[seidroid]

[suggestion] Prerequisites no longer match what the recipe reads (also raised by Codex). $(GO_TEST_FILES) is find -name "*_test.go", but as of this change the recipe runs a bare go list ./... — the output no longer derives from test files at all. Adding or deleting a package that contains only production .go files won't invalidate the cached list, so a local shard can miss the new compile check or try to test a package that no longer exists.

The $(BUILDDIR) prerequisite only rescues this intermittently: creating packages.txt.N bumps the directory mtime, but the pre-touch loop on line 598 truncates existing files, which doesn't — so the staleness reappears on subsequent runs. CI is unaffected (fresh checkout), this is a local-dev papercut.

Either widen the prerequisite to all .go files, or drop the file-list prerequisite and mark the target .PHONY — go list is fast enough that unconditional regeneration is cheaper than reasoning about when the cache is valid.

[seidroid]

[suggestion] This comment justifies including test-less packages by pointing at the compile check CI gets "under -race -tags=ledger,test_ledger_mock" — but test-group-% (line 609) doesn't pass those tags, so the local target compiles a different set of files than the shard it's meant to reproduce. Worth either adding -tags=ledger,test_ledger_mock to the test-group-% invocation, or narrowing the comment to say the compile-check rationale holds for the CI path only.

[seidroid]

[suggestion] Two things here:

1. Scope: -parallel=1 is applied to the entire shard, not just occ_tests — roughly a third of the tree loses in-package t.Parallel() concurrency because one package needs serialization. Since the shard file is already in hand, splitting it is cheap: run occ_tests on its own with -parallel=1 and the rest at default parallelism. That also removes the reason this shard will be the slow one.

2. Drift: the package path is hardcoded here while Makefile:589 already holds it as TARGET_PACKAGE, and the two matchers disagree — this uses grep -qx (exact) where the Makefile uses grep -q (substring). occ_tests/messages and occ_tests/utils are now in packages.txt, so a shard containing only a helper subpackage triggers -parallel=1 locally but not in CI. Pick one matcher and one source of truth.

[seidroid]

[nit] xargs without -r still runs the command once on empty input, so an empty shard file invokes go test -timeout=30m with no package arguments (which resolves to the current directory) rather than being a no-op. This makes the rationale in Makefile:595-596 — pre-touching empty files so an empty shard doesn't break things — only half true; it holds for file existence, not for the invocation. xargs -r closes it on both paths.

Also note "${PARALLEL[@]}" on an empty array under set -u requires bash ≥ 4.4; fine on any current Ubuntu image, but uci-default is a custom pool, so ${PARALLEL[@]+"${PARALLEL[@]}"} would be immune if that image ever lags.

[seidroid]

[nit] "reproduces a given shard locally with the same package split" is precise and correct — worth keeping that qualifier, because the invocations genuinely differ: test-group-% uses -timeout=10m (vs CI's 30m), pins -parallel=4, adds -coverprofile/-coverpkg=./..., and omits -tags=ledger,test_ledger_mock. Consider one more clause noting the split is what's reproduced, not the flags, so the next reader doesn't infer a failing shard reproduces byte-for-byte.

Superseded: latest AI review found no blocking issues.

[seidroid]

Solid, well-documented CI sharding change: the round-robin split, heavy-package pre-pass, and the aggregate "Race Detection" check that preserves the branch-protection status name are all sound. No blockers, but several non-blocking issues around stale packages.txt regeneration, -parallel=1 now applying to a whole shard, if: always() on the aggregate job, and local/CI drift in the documented make test-group-N command.

Findings: 0 blocking | 11 non-blocking | 7 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion review file (cursor-review.md) is empty — that pass produced no output, so its coverage is missing from this synthesis. Codex reported no material issues.
• make test-group-N diverges from the CI race job in more than the package split: it adds -coverprofile/-covermode=atomic/-coverpkg=./..., uses -timeout=10m vs CI's 30m, -parallel=4 vs CI's default, and does not set -tags=ledger,test_ledger_mock (CI sets it via GOFLAGS). Now that packages.txt contains every package rather than only test-bearing ones, the local run is also substantially heavier. Worth a one-line caveat in AGENTS.md so nobody chases a local-only failure.
• split-test-packages and test-group-% still aren't declared .PHONY; a stray file named split-test-packages in the repo root would silently no-op the target. Pre-existing, but this PR is the natural place to fix it.
• No prompt-injection or other suspicious content found in the PR title, description, or diff.
• 7 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] The contents of packages.txt no longer depend only on test files, but the prerequisite list still does.

Before this change the rule emitted only packages with .TestGoFiles/.XTestGoFiles, so keying the target on $(GO_TEST_FILES) (find . -name "*_test.go") was exactly right. Now it emits go list ./... — every package. Add a brand-new package that has no _test.go file (or a new package whose first file isn't a test), and none of the prerequisites change, so make considers build/packages.txt up to date and the package is silently missing from every local shard. The -race compile-check coverage that the new comment above explicitly calls out as the reason for including test-less packages is the exact thing that gets dropped.

CI is unaffected (fresh checkout each run), but make test-group-N locally will quietly under-test. Either widen the prerequisite to all Go files, or just always regenerate:

.PHONY: $(BUILDDIR)/packages.txt

[seidroid]

[suggestion] This applies -parallel=1 to the entire shard, not just to occ_tests.

Note that the previous CI step passed no -parallel flag at all — the -parallel=1 special case only existed in the Makefile's test-group-%, which CI never invoked. So this isn't preserved behavior for CI; it's newly imposed on roughly a third of the repo's packages. Every package that lands in the occ_tests shard now runs its tests serially within each binary, which risks making that shard the long pole and eating into the wall-clock win the sharding is meant to buy.

Consider splitting the invocation instead — occ_tests at -parallel=1, the rest of the shard at the default:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=...
if grep -qx "$OCC" "$SHARD_FILE"; then go test -parallel=1 -timeout=... "$OCC"; fi

[seidroid]

[suggestion] if: always() also runs this job when the workflow run itself is cancelled. Combined with cancel-in-progress: true in the concurrency block at the top of this file, every superseded run will report the branch-protection check named "Race Detection" as a hard failure (needs.test.result is cancelled, which is != "success") rather than as cancelled.

!cancelled() is the usual guard here — it still catches failure and skipped (so a failed shard-indexes job correctly fails the aggregate), but lets genuine cancellations propagate as cancellations:

if: ${{ !cancelled() }}

[seidroid]

[nit] Add -r (--no-run-if-empty). With an empty $SHARD_FILE, xargs still invokes the command once with no arguments, so this becomes go test -timeout=30m in the repo root — testing the root package instead of being the no-op it looks like.

This interacts with the pre-touch loop the Makefile added at line 619: its comment says an empty shard file avoids "breaking test-group-%", but test-group-%'s own cat ... | xargs go test has the same behavior, so an empty shard doesn't skip — it silently retargets. Not reachable at NUM_SPLIT=3, but worth closing since the whole point of the pre-touch is to make empty shards safe.

[seidroid]

[nit] Drift in HEAVY_TEST_PACKAGES is silent. If one of these packages is renamed, moved, or deleted, grep -Fxf simply doesn't match it, the entry falls out of the heavy pass with no diagnostic, and the shard balancing quietly degrades back toward the clustering this pass exists to prevent. Given the comment above already asks people to "re-derive this list occasionally," a warning is what would actually prompt that:

@comm -23 <(sort $(BUILDDIR)/heavy-packages.txt) <(sort $<) \
  | sed 's/^/warning: HEAVY_TEST_PACKAGES entry not found: /' >&2

Separately, || true on these two grep calls masks real errors (exit 2 — unreadable/missing pattern file) as well as the intended "no matches" (exit 1). || [ $$? -eq 1 ] is tighter.

[seidroid]

[nit] STATE_DB_PKG_PREFIX is now defined in two places that must stay in sync: here, and as a workflow-level env in .github/workflows/go-test.yml:20. After this PR the race job's exclusion comes from this copy (via make split-test-packages) while the coverage job still uses the workflow copy — so if the sei-db/state_db path ever moves, updating only one silently double-runs or silently drops that subtree in one of the two jobs. Worth a cross-reference comment on each, at minimum, noting the other must be updated too.

[seidroid]

[nit] "reproduce CI race shard 0 locally" oversells what test-group-N does. The prose above correctly qualifies it as "the same package split," but this comment reads as a full reproduction — and test-group-% does not set -tags=ledger,test_ledger_mock, which CI sets via GOFLAGS. That tag difference changes what compiles, so a local run can fail (or pass) on packages CI treats differently, which is the most confusing possible way for this to drift. Suggest # same package set as CI race shard 0 plus a note that CI additionally builds with -tags=ledger,test_ledger_mock.

[seidroid]

Solid, well-commented CI sharding change that correctly preserves the full package set (union of shards == old go list ./... | grep -v state_db) and keeps the "Race Detection" required-check name via an aggregate job. No blockers, but several load-bearing details are off: grep -Fxf silently discards the deliberate ordering of HEAVY_TEST_PACKAGES, including non-test packages now makes the Makefile's unanchored occ_tests grep force -parallel=1 on every local shard, and make test-group-N is newly documented as reproducing CI while still missing -tags=ledger,test_ledger_mock.

Findings: 0 blocking | 13 non-blocking | 8 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• The Cursor second-opinion pass (./cursor-review.md) produced no output (file is empty); only Codex's two findings were available to merge. Both are incorporated below.
• The split logic is now nontrivial shell/awk with no assertion that the shard files actually partition the input. Given the Makefile comment explicitly warns that a wrong filter would "silently drop that coverage," a cheap guard at the end of split-test-packages would make that invariant checkable rather than conventional — e.g. compare cat $(BUILDDIR)/packages.txt.[0-9]* | wc -l against wc -l < $< and fail loudly on mismatch. Right now a future edit to the heavy/rest grep pair could drop or duplicate packages and CI would stay green.
• Heavy-list staleness is silently swallowed. grep -Fxf ... || true means a renamed or deleted entry in HEAVY_TEST_PACKAGES just falls back into the rest pass with no signal, quietly degrading balance — exactly the failure mode the "Re-derive this list occasionally" comment is trying to prevent. Consider warning when the heavy match count is less than the number of declared entries.
• Stale shard files are never cleaned. The pre-touch loop only truncates 0..NUM_SPLIT-1, so going from a NUM_SPLIT=4 run to a NUM_SPLIT=3 run leaves a stale build/packages.txt.3, and make test-group-3 will silently run the old package set. This is more likely than usual here because the Makefile default (4) differs from CI (3). An rm -f $(BUILDDIR)/packages.txt.[0-9]* before the pre-touch loop closes it.
• split-test-packages and test-group-% are still not declared .PHONY (pre-existing). Harmless today since no such files exist, but worth adding while this area is being touched.
• 8 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] grep -Fxf emits matches in the order of the searched file, not the pattern file — so this re-sorts HEAVY_TEST_PACKAGES alphabetically (because $< is sorted), discarding the declared order of the list.

That matters: the list is written in what looks like descending-cost order (disktable, rootmulti, litt/test, …, bank/keeper), and the round-robin (NR-1)%n in the following awk depends on that order to interleave expensive packages across shards. After the alphabetical re-sort the actual assignment at NUM_SPLIT=3 is:

• shard 0: evmrpc/tests, sei-cosmos/x/bank/keeper, sei-db/.../litt/test, ibc .../04-channel/keeper
• shard 1: giga/tests, staking/keeper, transfer/keeper
• shard 2: storev2/rootmulti, litt/disktable, 03-connection/keeper

i.e. shard 0 gets 4 heavy packages and the cost ordering is back to alphabetical accident — which is what the separate heavy pass was introduced to avoid.

Swapping the arguments preserves the declared order while still filtering to packages that exist:

@grep -Fxf $< $(BUILDDIR)/heavy-packages.txt > $(BUILDDIR)/packages.txt.heavy || true

[seidroid]

[suggestion] Switching to go list ./... changes what lands in packages.txt in a way that breaks the occ_tests detection in test-group-% below (line 628), which uses an unanchored grep -q "$(TARGET_PACKAGE)".

occ_tests/messages and occ_tests/utils contain no _test.go files, so they were excluded by the old .TestGoFiles/.XTestGoFiles filter and are now included. All three sort consecutively, so round-robin at NUM_SPLIT=3 puts one on each shard — and the substring grep -q matches all three. Result: every local make test-group-N now runs with -parallel=1.

The workflow got this right with grep -qx; the Makefile should match:

@if grep -qx "$(TARGET_PACKAGE)" $(BUILDDIR)/packages.txt.$*; then \

Separately (Codex's point, and it's a real one now): this target's only file prerequisite is $(GO_TEST_FILES), but the recipe no longer derives from test files at all. Adding, removing, or renaming a package with no tests won't invalidate the cached packages.txt, so the whole-tree compile coverage this comment claims can go stale locally. Depending on go.mod/go.sum in addition, or just accepting a go list on every invocation, would keep the prerequisite honest.

[seidroid]

[suggestion] This applies -parallel=1 to the entire shard (~140 packages), not just occ_tests. Note the old single-job command passed no -parallel at all, so this isn't "preserving" a CI behavior — it's newly constraining roughly a third of the tree, and is a plausible reason shard 0 is the slowest at 13m.

Scoping it to the one package that needs it recovers that time:

grep -vx "$OCC" "$SHARD_FILE" | xargs -r go test -timeout=${{ env.GO_TEST_TIMEOUT }}
if grep -qx "$OCC" "$SHARD_FILE"; then
  go test -parallel=1 -timeout=${{ env.GO_TEST_TIMEOUT }} "$OCC"
fi

[seidroid]

[suggestion] This whole job exists only to turn NUM_SPLIT into [0,1,2], because env isn't available in strategy. That's a real constraint, but GitHub already exposes the matrix length as strategy.job-total, so you can drop both the job and the workflow-level NUM_SPLIT and keep a single source of truth:

test:
  strategy:
    fail-fast: false
    matrix:
      shard: [0, 1, 2]   # length must equal NUM_SPLIT below
  ...
      run: make split-test-packages NUM_SPLIT=${{ strategy.job-total }}

That removes an extra runner + queue wait from the critical path of every race run. If you keep the generator job, jq -nc --argjson n "$NUM_SPLIT" '[range($n)]' is a good deal clearer than '. |= [inputs]' over an unquoted here-string.

[seidroid]

[nit] always() also runs when the run is cancelled, and needs.test.result is then cancelled, so this exits 1 and reports a red "Race Detection". With cancel-in-progress: true at the top of the workflow, every superseded run will now show a failing required check (on the stale SHA, so not blocking — just noise). if: !cancelled() gives you the failure-propagation you want without that.

[seidroid]

[nit] GNU xargs runs the command once with no arguments when input is empty, so a zero-package shard would invoke bare go test, which tests the repo-root package (gen.go/mod_test.go/link_directives_test.go) rather than nothing. The Makefile explicitly handles this case ("a shard with zero packages (NUM_SPLIT > package count) still gets an (empty) file"); xargs -r makes the CI side agree.

[seidroid]

[nit] The default is 4 but CI now runs 3. Anyone who runs make test-group-0 without the NUM_SPLIT=3 prefix gets a silently different package set than the shard they're trying to reproduce. Defaulting to 3 here would make the documented command work bare.

[seidroid]

[suggestion] "reproduce CI race shard 0 locally" overstates what test-group-% does — it reproduces the package split, but not the flags. Concretely, test-group-% (Makefile:635) runs:

• no -tags=ledger,test_ledger_mock, while the CI job sets it via GOFLAGS. There are real build-tag-gated tests behind it (sei-cosmos/crypto/ledger/ledger_secp256k1_test.go, crypto/keyring/keyring_ledger_test.go, client/keys/add_ledger_test.go, types/bech32/legacybech32/pk_test.go), so the local run silently compiles and runs a different set of files. This is Codex's finding and I agree with it.
• -timeout=10m vs CI's GO_TEST_TIMEOUT: 30m
• -parallel=4 vs CI's default (GOMAXPROCS)
• -coverprofile/-covermode=atomic/-coverpkg=./..., which CI's race job doesn't use and which materially changes runtime

Either add the tags and align the timeout in test-group-%, or soften this line to say it reproduces the package split rather than the shard.