[IAmJSD]

Why

On main's CI run 40153816430 (after #1400 put each package's integration tests on one shared Postgres server), the Integration Tests job took 531s:

• setup: about 45s (service container, setup-go, golang-migrate, migrations)
• compiling every package with -race and coverage: about 73s before the first package finished
• test step: 480s of wall time, holding 856s of package time on one 4-vCPU runner (CPU-bound)

Of the test time, 536s was tests in //go:build integration files. 271s was untagged unit tests, which the Unit Tests job already runs with -race -short. Only two test files check testing.Short(), so almost every unit test ran twice.

Slowest packages: migrations 190s, api 130s, auth 122s, server 92s, purchase 76s, config 70s, testhelpers 25s, secrets 21s (no integration tests at all), database 18s, scheduler 15s, cleanup-lambda 14s, analytics 12s.

What changed

1. The job runs only integration tests. scripts/integration-test-selection.sh prints <package> <tab> ^(TestA|TestB|...)$ <tab> <count> for each package with tests the Unit Tests job doesn't run:

• Top-level Test* functions in *_test.go files that compile only with -tags integration. The script doesn't parse build constraints itself. It compares the test files from go list -tags integration with those from untagged go list, so &&, ||, !, parentheses and legacy // +build lines are evaluated exactly as go test evaluates them. An integration && foo file is excluded because CI never sets foo. A !integration file is excluded because CI never compiles it.
• Untagged tests that reach testing.Short(), either directly, through a package-local helper (followed to a fixed point), or through testutil.SkipIfShort. I grepped for them: internal/server/integration_test.go is tagged anyway; internal/testutil.SkipIfShort has no callers. The only untagged one is internal/accounts TestDiscoverOrgAccounts_CancelledContext, which -short skips in the Unit Tests job, so before this PR only this job ran it. It is selected.
• Packages with neither are not passed to go test, so they aren't compiled either. internal/secrets, for example, drops out entirely.
• The count column is the number of top-level tests the regex runs. It can exceed the number of names: internal/analytics defines TestPostgresAnalyticsStore_Close in both the in-package untagged file and the external-package tagged file. -run matches by name, so that package's unit copy still runs here, one test of de-duplication we can't get without renaming a test.

2. The job is sharded into six legs (strategy.matrix, fail-fast: false). Each leg has its own Postgres service container, go install of migrate, the migrations step and the same env vars, -race, -tags=integration, -timeout=20m and -coverprofile. The grouping comes from this PR's first CI run (three legs, run for 89de873):

Leg (first run)	Job	Test step	First package done	Package test times
migrations	178s	116s	~1 min into the step	migrations 64s
api, purchase, config	308s	245s	~4 min into the step	api 87s, config 85s, purchase 53s
auth, server, rest	365s	294s		auth 62s, server 55s, testhelpers 32s, cleanup-lambda 15s, database 15s, scheduler 12s, analytics 12s, commitmentopts 9s

Each leg spent most of its time compiling (point 3), and the package times put api, config, auth, server and purchase each near the old migrations leg on its own. The new legs:

Leg (job name)	Packages	Tests	Package time on the first run
Integration Tests (api)	internal/api	97	87s
Integration Tests (config)	internal/config	217	85s
Integration Tests (purchase)	internal/purchase	63	53s
Integration Tests (auth)	internal/auth	23	62s
Integration Tests (server)	internal/server	16	55s
Integration Tests (migrations, rest)	internal/database/postgres/migrations + catch-all: cmd/cleanup-lambda, internal/{accounts, analytics, commitmentopts, database, database/postgres/testhelpers, scheduler, testutil}	117	64s + 95s, side by side

• api (87s) is the floor: it's one package, and packages aren't split by test. Every other leg is at or under it: migrations (64s) runs in parallel with the rest (95s spread over the other three cores, the longest 32s). Fewer legs might not cost wall time: server and purchase (55s + 53s) in one leg run side by side and could still finish under api. But those times were measured while sharing 4 cores, and the -race tests are CPU-bound, so a merged leg risks becoming the long pole. A separate leg costs runner time, not wall time, so I kept them apart. If runner minutes matter more, server + purchase is the first merge to try.
• The shard list lives in one job-level env var, INTEGRATION_SHARDS. The * entry is the catch-all, computed at runtime as every package with integration tests minus the named ones, so a new package lands in "migrations, rest" automatically.
• Guard step ("Check every package with integration tests is in exactly one shard") fails if a package lands in no shard or in two, if a named package no longer has integration tests, if a shard ends up empty, if a shard name has characters other than letters, digits, ., _ and - (names go into cache keys and artifact names), or if the matrix legs and the INTEGRATION_SHARDS lines disagree. It checks strategy.job-index and strategy.job-total, so every spec shard has exactly one leg.
• The test step fails if fewer top-level tests reported a result than were selected. Before, it only failed when zero ran. If more ran (a selected name also names a unit test in another package of the same shard), it emits a warning.
• The tests/e2e go vet -tags=e2e type-check runs on one leg (migrations, rest), and it still runs if that leg's tests fail.

3. A warm Go build cache per leg. setup-go's cache: true is one cache entry shared by every job, keyed on go.sum and saved by whichever job finishes first. It never holds this job's -race -tags=integration coverage build, so every leg compiled cold. On main the x64 entry (setup-go-Linux-x64-ubuntu24-go-1.26.10-9e4b…) is 7.6 KB, so in practice it holds nothing at all.

• setup-go's cache is now off for this job. In its place:
  • Build cache, one per leg: path go env GOCACHE, key integration-gobuild-<os>-<arch>-<go version>-shard-<shard>-src-<hash>. The hash covers go.work, go.work.sum, every go.mod/go.sum, every *.go file, and the whole cmd/, internal/ and iac/ trees, which holds the go:embed inputs (openapi.yaml, IaC templates). restore-keys fall back to this shard's latest entry (the PR's own, else main's), then to any shard's. The build cache is content-addressed, so a stale entry is safe: it only recompiles what changed.
  • Module cache, shared by the legs: path go env GOMODCACHE, key integration-gomod-<os>-<arch>-<hash of go.work/go.work.sum/go.mod/go.sum>, falling back to the latest. On a miss, leg 0 runs go list -deps -test -tags=integration ./... so its saved set covers every leg, then saves it. No two caches restore the same path.
• Restore and save are separate steps (actions/cache/restore / actions/cache/save, the same 0057852… v4.3.0 pin the security-scan job uses). The build cache is saved whenever the test step ran, pass or fail, but not on cancel and not on an exact hit.
• Size control. Restored entries are backdated two hours (Go's own trim only removes entries more than 5 days old). Go moves the mtime of any entry over an hour old to now when it uses it. Before saving, entries still older than a marker taken right after restore are deleted. So a saved cache holds exactly what that leg used, and a fallback restore (an older key, another shard) doesn't keep growing.
• go test -count=1. -run, -v, -timeout and -coverprofile are all cacheable test flags, and go test can't see that the tests depend on the database. Without -count=1, a restored cache would replay earlier results instead of running the tests.

4. Coverage. I grepped all workflows: nothing in CI downloads integration-coverage. It's a 30-day artifact for people. Codecov and the 80% coverage warning (a ::warning::, not a gate) both read the unit job's coverage.out, which this PR doesn't change. No threshold or gate changes. Each leg uploads integration-coverage-<shard> (1-day retention). A new Merge integration coverage job (needs: [unit-tests, integration-tests]) merges them and uploads the same integration-coverage / coverage-integration.out as before. The old job's profile also covered the unit tests it re-ran, so the merge adds the unit job's profile back in. That keeps the artifact's meaning: 81.6% total, the same as main's artifact (see Verification). Without the unit profile it would be about 38%.

5. CI Success now needs integration-tests (the matrix), integration-coverage and integration-test-selection. Its check is an allowlist on result == "success", and a matrix job's needs result is failure if any leg failed and cancelled if any was cancelled. That's the same mechanism terraform-validate and docker-build already rely on. The required checks on main (rulesets API) are only CI Success and Run pre-commit hooks, so the renamed per-shard job names don't affect branch protection.

6. Self-test: scripts/test-integration-test-selection.sh (23 cases) runs as its own Integration test selection self-test job, next to the other script self-tests. It needs setup-go (cache off; the fixtures are stdlib-only). The fixture packages in a temp dir cover: tagged files (in-package and external), an untagged file, !integration, integration && foo, integration || foo, (integration && !foo) || ignore, legacy // +build, Short-gated tests (direct, through a two-level helper, through testutil.SkipIfShort, inside a subtest closure, and a comment/string mention that must not count), the same test name in both test packages in both tag orders, a package with no integration tests, a package with no test files, and regex anchoring and escaping. The escaping is checked against Go's regexp, and the regexes against go test -list. It also covers every shard-spec failure, shard names unfit for a cache key, and the one-package / catch-all-that-names-a-package layout ci.yml uses. I mutation-checked it: breaking the Short filter, the tag diff, the anchoring or the duplicate-package check each makes 1-5 cases fail.

Verification

All of this ran locally on this branch's head (1bcae5b6):

• actionlint (pinned rhysd/actionlint:1.7.12 image): clean.
• pre-commit run --files .github/workflows/ci.yml scripts/integration-test-selection.sh scripts/test-integration-test-selection.sh (trivy skipped there, run below): all hooks pass, including actionlint, zizmor and git-secrets. trivy: docker run aquasec/trivy:0.69.3 config --severity HIGH,CRITICAL --exit-code 1 --skip-dirs terraform/environments/aws --skip-dirs "**/.terraform" --skip-dirs .claude ., exit 0.
• shellcheck -x on both scripts: clean.
• bash scripts/test-integration-test-selection.sh: 23 passed, 0 failed. Mutation check: dropping the new shard-name check fails exactly the new case (22 passed, 1 failed).
• bash ~/…/secrets-scan.sh printed clean; planted key flagged. bash scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• No Go code changed, so golangci-lint has nothing new to check.

Guard on the real repo with the new spec. I extracted the guard step's run: block, INTEGRATION_SHARDS and the matrix from ci.yml and ran the block once per leg with SHARD_INDEX/SHARD_TOTAL substituted. All six legs passed:

Leg	Packages	Tests
api	1	97
config	1	217
purchase	1	63
auth	1	23
server	1	16
migrations-rest	9	117
total	14	533, the same as before

Run as leg 0 with shard config, the block fails with matrix leg 1 of 6 is shard 'config', but INTEGRATION_SHARDS line 1 of 6 is 'api'.

Cache effect, simulated locally. I built a shard's test binaries exactly as the test step does (go test -race -tags=integration -count=1 -coverprofile=… -run '^$' <packages>: full compile, coverage instrumentation and link, no tests run), pinned to 4 cores (taskset -c 0-3) with go1.26.10 and a warm module cache. Then I copied the GOCACHE dir to simulate a restore, ran the workflow's own "Mark restored build cache entries unused" and "Drop build cache entries this run did not use" run: blocks around the build, and built again:

Case	Build time
api shard, cold GOCACHE	351s
api shard, its cache restored (exact hit)	10s (15s on a rerun), 0 compile and 0 cover actions in go test -x, 1 link
api shard after a comment edit to internal/config and internal/api (a partial hit through restore-keys)	43s
api shard, the restored cache after the prune step	0 compile actions; the prune dropped 67 of 8754 entries
config shard, cold	119s
config shard with the api shard's cache restored (the any-shard fallback)	23s; the prune then dropped 2669 entries, leaving 451 MB, the same size as config's own cold cache
go install of golang-migrate, cold / warm	20s / 0.2s

• Coverage builds are cached. The warm api run had no compile or cover invocations under -x; only the test binary was linked.

• Cache sizes after one shard's build (a fresh GOCACHE per shard, so it holds only that shard's build; on CI each leg's default GOCACHE starts empty because setup-go's cache is off):

  Shard	On disk	zstd (as actions/cache stores it)
  api	1.7 GB	213 MB
  migrations-rest	1.6 GB	205 MB
  server	1.6 GB	~200 MB (estimated from size on disk)
  purchase	1.4 GB	~180 MB (estimated)
  config	451 MB	84 MB
  auth	454 MB	~85 MB (estimated)
  module cache (all shards' modules)	976 MB	255 MB

  That's about 1.0 GB per full set of six build caches, saved whenever Go inputs change, plus 255 MB when a go.mod/go.sum changes.

• Cold builds ran three at a time on a 16-vCPU box (load from the other two), so they were slower than one alone: api 539s in parallel against 351s alone. All the cold/warm comparisons in the table ran alone.

Selection against the real repo (bash scripts/integration-test-selection.sh, about 7s; regexes elided, count column shown):

cmd/cleanup-lambda                          3
internal/accounts                           1   ^(TestDiscoverOrgAccounts_CancelledContext)$
internal/analytics                         20
internal/api                               97
internal/auth                              23
internal/commitmentopts                     5
internal/config                           217
internal/database                           4
internal/database/postgres/migrations      69
internal/database/postgres/testhelpers      8
internal/purchase                          63
internal/scheduler                          6
internal/server                            16
internal/testutil                           1
total                                     533

Before/after test sets. I listed every test with go test -list '.*' ./... with and without -tags integration, then listed each shard with its exact -run regex and packages under -tags integration:

	(package, test) pairs
Unit Tests job (untagged, unchanged)	4592
Integration Tests job before (tagged, everything)	5123
Integration Tests job after (all shards together)	533
Union before (unit ∪ integration)	5122 unique
Union after (unit ∪ shards)	5122 unique

• diff of the two unions: empty.
• The 4590 pairs dropped from the integration job are all in the Unit Tests job's list (0 not covered).
• 0 tests were added that the integration job didn't run before.
• The one duplicated pair is the internal/analytics name above, which is in both test packages (5123 lines, 5122 unique).
• Selected names compared against the tagged-only go test -list difference: identical except the intended Short-gated internal/accounts test.

Earlier local run of the test steps, on the three-leg layout before this rebalance (the packages and each package's go test flags are unchanged apart from -count=1). I ran them locally against testcontainers, one leg after another. I extracted the "Select integration tests", guard, "Run integration tests" and e2e steps' run: blocks from ci.yml and ran them with bash -eo pipefail, the job and step env, and strategy.job-index/job-total substituted. Each leg was pinned to 4 cores (taskset -c 0-3) with a fresh GOCACHE holding only pre-built third-party deps (repo packages compiled cold), GOFLAGS=-count=1, CI=true. The Postgres containers weren't pinned. All passed, each reporting ran N of N selected:

Leg	Wall	Select + guard	Compile (approx.)	Slowest package	Tests ran
migrations	291s	6s	~14s	migrations 271s	69 of 69
api, purchase, config	201s	8s	~77s	api 116s (config 105s, purchase 61s)	377 of 377
auth, server, rest	214s	6s	~90s	server 83s (auth 64s)	87 of 87, plus e2e vet 34s

This machine is shared with other agents' test runs (load average 5 to 45 while measuring), so these local times run slower than CI's: migrations took 271s here and 190s on CI. With a fully cold GOCACHE (third-party deps compiled too) the legs took 351s, 609s and 732s. Those runs were compile-bound and happened under the heaviest load, so they're an upper bound only.

Coverage merge, simulated by running the merge job's run: block on the local shard profiles plus the unit job's coverage.out from run 40153816430: 4 profiles, 31438 blocks, 81.6% total. Main's integration-coverage artifact from the same run: 81.6%. Per function, the only lower one is newMigratorWithRecovery, one block (migrate.go:132, logging when m.Close() errors after an auto-heal error). It's hit only by the tagged autoheal/idempotency tests, which ran in both, so this is a timing-dependent error path, not a selection gap. The merge step reads every integration-coverage-* artifact whatever the shard names are. With six legs it merges seven profiles.

Only CI exercises these (not verifiable locally):

• the six-leg strategy.matrix expansion and job names, strategy.job-index/job-total, and the e2e-typecheck include key
• the Postgres service container and migrate step per leg
• actions/cache/restore/save: real hit rates, restore-key fallback across PR and main scopes, upload/download times, and that the backdate/prune steps behave the same on the runner's filesystem
• all timings. The run for this push is cold: no integration-gobuild-* or integration-gomod-* entry exists yet, so each leg compiles from scratch and then saves. A second push shows the warm numbers. After merge, main's entries become the fallback for every PR.
• per-shard upload-artifact, download-artifact with pattern/merge-multiple, the merge job, and the CI Success aggregator's view of the matrix

Expected effect

Before: main 531s (one job). This PR's first run (three legs, 89de873): 365s wall (legs 178s / 308s / 365s), compile-bound.

After, warm (a later push to this PR, or any PR once main has the entries). Per leg: setup ~45s, which shrinks by roughly 20s because golang-migrate's compile is now cached; ~10-20s to restore ~85-213 MB of build cache plus 255 MB of modules; a few seconds of link; then the slowest package; then ~10-20s to prune and save when the Go inputs changed:

Leg	Estimate
api	~30 + ~15 + ~10 + 87 + ~15 ≈ ~2.5-3 min (the long pole)
config	≈ 2.5-3 min (85s of tests)
migrations, rest	≈ 2.5 min (64s alongside 95s over three cores)
auth / server / purchase	≈ 2-2.5 min

• Job wall ≈ 2.5-3 min, down from 365s on the first run and 531s on main. These are estimates from the first run's package times. Run on its own, api may beat its 87s, which it shared 4 cores with config and purchase to get. A partial hit (most PR pushes) rebuilds only the changed packages and their dependents: +~30s in the local simulation for a small edit.
• Cold runs (this push, a Go version bump, or a full eviction) stay compile-bound, at about the first run's 6 min plus the save: each leg compiles less than before, but the api, purchase, server and migrations-rest legs each still build most of the cloud SDKs.
• Runner time goes up: six setups. Roughly 6 × 2.5 min ≈ 15 runner-minutes warm, against 9 on main and 14 on the first sharded run.

Note for cristim

• Cache budget. The repo already holds 11.3 GB of active caches (gh api …/actions/cache/usage), above the 10 GB default. Most of it is 9.6 GB of buildkit-blob entries. GitHub evicts least-recently-used entries past the limit. This PR adds about 1.0 GB per push that changes Go inputs (six build caches), plus 255 MB per go.mod/go.sum change. Main's entries stay recent because every PR restores from them; old PR entries and buildkit blobs are what get evicted first. If that churn is too much, add github.event_name == 'push' to the two save steps' if:. PRs would then only restore main's entries (a partial hit, ~43s in the simulation instead of 351s cold) and save nothing.
• Unrelated finding: main's shared setup-go x64 cache entry is 7.6 KB. Whichever job saves it first apparently has nothing in GOMODCACHE/GOCACHE yet, so lint and unit tests download modules and compile cold on every run as well. I didn't touch it here.
• If a cache misbehaves, removing the restore steps (or deleting the integration-gobuild-* entries) falls back to a cold compile; correctness doesn't depend on the cache, and -count=1 keeps every test running.
• The shard balance is in one place: INTEGRATION_SHARDS in ci.yml, kept in the same order as the matrix.include entries; the guard fails if they drift. Each leg's log prints per-package ok ... <time> lines for rebalancing.
• integration-coverage now includes the unit profile on purpose, so it matches what it measured before (unit + integration, 81.6%). If you'd rather it mean integration-only, drop the "Download unit coverage" step.
• The merge job needs unit-tests as well. As before, the artifact exists only on green runs, but now a red Unit Tests job also prevents it.
• The selector's Short detection assumes gofmt layout and treats calls named SkipIfShort as Short-gated. Over-selection fails loudly, because a selected name that doesn't run makes ran < selected fail the leg. A missed test would not fail anything at runtime: the guard against that is the self-test (now its own CI job) and the before/after go test -list diff above, which is worth re-running if someone adds a new way to skip under -short.
• The merge job and self-test job use permissions: contents: read, like the other jobs. Downloading artifacts from the same run doesn't need actions: read, but the PR's CI run is the first real test of that.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 5 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 8793aa3
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a01cf197-4de8-4300-840c-259971ce02f1

Commits

Reviewing files that changed from the base of the PR and between 89de873 and d822d4d.

Files selected for processing: 3

• .github/workflows/ci.yml
• scripts/integration-test-selection.sh
• scripts/test-integration-test-selection.sh

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 8793aa3
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 47275a8a-b5a4-4f60-bf9d-269a5eb1b991

Commits

Reviewing files that changed from the base of the PR and between 8793aa3 and 89de873.

Files selected for processing: 4

• .github/workflows/README.md
• .github/workflows/ci.yml
• scripts/integration-test-selection.sh
• scripts/test-integration-test-selection.sh

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The CI workflow now selects integration-tagged tests and short-mode tests, assigns packages to four shards, and runs each shard with race detection and coverage. It validates shard assignments and test counts, merges shard coverage with unit coverage, and runs selector self-tests as a required CI job.

Changes: Integration test CI

Layer / File(s)	Summary
Test selection and shard assignment scripts/integration-test-selection.sh, scripts/test-integration-test-selection.sh, .github/workflows/ci.yml	The selector finds qualifying test functions, creates anchored test-name patterns, and assigns selected packages to shards. Its self-tests cover selection rules, regex matching, and shard validation. CI runs the selector self-tests.
Sharded CI test execution .github/workflows/ci.yml, .github/workflows/README.md	The workflow runs selected packages across four shards. Each shard validates test results and produces a coverage profile. The e2e type-check runs on one shard. The README describes the selected test groups and sharding script.
Coverage merge and required CI jobs .github/workflows/ci.yml	A merge job combines shard profiles with unit coverage and checks profile availability and mode. The merge and selector self-test jobs are required by ci-success.

Priority: Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant CI as GitHub Actions CI
  participant Selector as integration-test-selection.sh
  participant Shard as Integration shard
  participant Merge as Coverage merge job
  CI->>Selector: Generate test selection and assign packages
  Selector-->>CI: Return package patterns and test counts
  CI->>Shard: Run selected tests with race detection and coverage
  Shard-->>CI: Upload shard coverage profile
  CI->>Merge: Provide shard and unit coverage profiles
  Merge-->>CI: Return merged coverage or validation failure

Loading

Suggested reviewers: cristim

Merge Risk: Minimal · up to 89de8

No merge-blocking issue is established. The CI run still needs to confirm the matrix and artifact behavior.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly identifies the main change: selective integration testing with sharding. The stated six-runner count does not match the objective's three matrix legs, but the title remains related a…

Full details: Title check

Explanation

The title clearly identifies the main change: selective integration testing with sharding. The stated six-runner count does not match the objective's three matrix legs, but the title remains related and specific.

Full details: Docstring Coverage

Explanation

Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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