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

[amir-deris]

Summary

• Race Detection in go-test.yml ran all ~420 packages in a single uci-default job (~18 min baseline), while the Makefile's test-group-N/NUM_SPLIT sharding sat unused by CI.
• Wired the race job to a dynamically-generated 3-way matrix (same index-generation pattern as eth_blocktests.yml), with an aggregate test-check job that keeps the "Race Detection" status-check name so branch protection doesn't need to change.
• Packages are split round-robin (package i → shard i % NUM_SPLIT) via a small awk one-liner in the Makefile's split-test-packages target — not a contiguous chunk split. This matters: an earlier count-based split -d -n l/N attempt dumped a whole cluster of alphabetically-adjacent (and often runtime-correlated, e.g. a module's many keeper packages) packages into one shard, leaving it at ~17 min while another finished in ~5 min. Round-robin interleaving spreads that correlation out.
• make test-group-N reproduces a given CI shard exactly, closing a pre-existing local/CI drift (sei-db/state_db/... is now excluded from the Makefile's package list too, since it's owned by sei-db-tests.yml).
• Preserves -race, -tags=ledger,test_ledger_mock, and the occ_tests -parallel=1 special case per shard.

Also explored and reverted: duration-aware bin-packing

A follow-up iteration tried balancing shards by actual historical per-package test duration instead of round-robin — a small dependency-free Go tool (plan/record/merge) that queried the GitHub API at run time for the current branch's (falling back to main's) last successful run's timing artifact and bin-packed accordingly, with round-robin as the final fallback. It worked mechanically (correctly found and used real timing data), but the one real-data run it produced came in slower (24m40s/16m16s/16m27s) than the plain round-robin baseline (12m17s max), most likely due to uci-default runner contention rather than a packing flaw — but that's exactly the problem: the added complexity (a Go tool, GITHUB_TOKEN + actions: read permissions, a record-timings job, cross-run artifacts) wasn't earning its keep against noisy CI infra we can't yet cleanly separate from real signal. Dropped it in favor of the much simpler round-robin split for now; revisit if/when there's a cleaner way to validate duration-based balancing (e.g. a quieter runner pool, or averaging over many runs).

Context

Linear: PLT-959

uci-default is an org-wide autoscaling ARC pool capped at maxRunners: 30 (not dedicated to sei-chain), so shard count is intentionally conservative to start — going straight to 6-8 shards risked runner queuing that could offset the wall-clock win. Starting at NUM_SPLIT=3 and watching actual runner behavior in practice before scaling further.

Test plan

• Confirm this PR 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
• Compare total wall-clock for Race Detection against the ~18 min baseline over a few more runs (single-run numbers so far have been noisy — 17 min imbalanced-chunk, 12.3 min round-robin, 24.7 min contended bin-pack)
• Spot-check 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 7, 2026, 4:48 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 (f0ace34) to head (b88efd4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3874      +/-   ##
==========================================
- Coverage   61.62%   60.85%   -0.77%     
==========================================
  Files        2348     2287      -61     
  Lines      200842   191099    -9743     
==========================================
- Hits       123760   116298    -7462     
+ Misses      66028    64545    -1483     
+ Partials    11054    10256     -798     

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

Flags with carried forward coverage won't be shown. Click here to find out more.
see 175 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
Changes are limited to CI orchestration and test sharding; runtime behavior and test flags are preserved, with only wall-clock and branch-protection aggregation semantics affected.

Overview
Race Detection in go-test.yml no longer runs the full package tree in one job. It fans out into three parallel matrix shards (NUM_SPLIT: 3), each running make split-test-packages and testing only build/packages.txt.<shard>. A test-check aggregate job keeps the branch-protection name "Race Detection" unchanged.

The Makefile package list now excludes sei-db/state_db (aligned with sei-db-tests.yml) and lists all packages (not only those with *_test.go) so shards still act as -race compile checks. Splitting switched from contiguous split -d -n l/N chunks to round-robin (package i → shard i % NUM_SPLIT) via awk, with empty shard files pre-created so test-group-% stays valid.

AGENTS.md documents that CI uses three shards and that NUM_SPLIT=3 make test-group-N reproduces a shard locally. occ_tests still forces -parallel=1 per shard when that package appears in the shard file.

^{Reviewed by Cursor Bugbot for commit b88efd4. 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.