git:20260601.e77f1d7 to git:20260624.c286c01

---

name: golang-test-review

description: Use when reviewing Go test code (new or changed *_test.go files, test harnesses, fixtures, or test tooling) for a Go-test-expert pass — behavior-over-surface, determinism, parallel-safety, build-tag correctness, and harness reuse. Complements the general code-reviewer with test-specific rigor.

---

A focused Go-test-engineering review. Run it over the changed `*_test.go` files

(and any test harness / fixtures / test tooling) **after** writing tests and

before merge. It does not replace `code-reviewer` — it adds the test-specific

lens that general review misses.

## How to run

1. Identify the changed test surface: `git diff --stat <base>...HEAD -- '*_test.go' 'go/test/**' 'go/cmd/testlatency/**'`.

2. Read each changed test file and score it against the checklist below.

3. Report findings by severity (CRITICAL / HIGH / MEDIUM / LOW), each with

`file:line`, the rule violated, and a concrete fix. End with a verdict:

APPROVE (no CRITICAL/HIGH) / CHANGES-REQUESTED.

## Checklist

### 1. Behavior over surface (the highest-signal rule)

- Does the test pin the **invariant under change**, or just re-walk lines for

coverage? A test that would still pass if the behavior broke is a no-op —

flag it CRITICAL. (AGENTS.md Rule 9.)

- Are assertions specific (exact value / error) rather than "not nil" / "len > 0"

where an exact check is possible?

- Negative + edge cases present, not only the happy path?

### 2. Determinism (non-negotiable)

- No dependence on ambient state: live repo, `$HOME`, real clock, network,

`.evolve/runs/`. Tests build isolated state (`t.TempDir()` + `git init`, or the

shared `fixtures.NewWorkspace`).

- No real `time.Sleep` for synchronization where an injected clock / channel /

poll-with-deadline would be deterministic. Real sleeps belong only in

build-tagged integration/e2e tiers.

- No ordering dependence between tests; no shared mutable package globals.

### 3. Parallel-safety

- `t.Parallel()` is present where safe — BUT never combined with `t.Setenv`

(panics) or shared-mutable-state tests.

- Subtests that capture a loop variable are Go-1.22-safe or copy the variable.

- `-race` clean (the suite runs with `-race`).

### 4. Build-tag / cost-axis correctness (this repo's two-axis model)

- Slow tests (real subprocess: git/gh/tmux/bash; real timing) carry

`//go:build integration` or `//go:build e2e` and are NOT in the fast default.

- **Self-containment:** an untagged (fast) test must not reference a symbol

defined only in a tagged file — the default build would break. Shared helpers

the fast suite needs live in an untagged file.

- Tag sits at the top, followed by a blank line, before `package`.

### 5. Harness reuse (no re-duplication)

- Uses `go/test/fixtures` (Workspace builder, Fake{Storage,Ledger,Runner,Bridge},

FixedClock, assertion facade) instead of a new local temp-dir builder, fake,

clock, or `must*` helper. Reintroducing a local clone of a harness primitive

is a HIGH finding.

- `fixtures.FilePresent` (pure bool) for skip preconditions — NOT

`acsassert.FileExists` (which logs an `Errorf` and shouldn't gate a skip).

### 6. Naming & structure

- Behavior-named (`TestShipGate_BlocksWhenRedCountNonZero`), never cycle-pegged

(`TestC102_*`).

- AAA (Arrange / Act / Assert) visually distinct; table-driven where it reduces

repetition; `t.Run` subtests named for the case.

- `t.Helper()` on every helper so failures point at the caller.

### 7. Debuggability of failures

- Failure messages state got-vs-want and the value under test

(`got %q, want %q`), not bare `t.Fatal("failed")`.

- Skips explain why and what's missing (`t.Skip("git not on PATH")`).

### 8. Hygiene

- `t.Cleanup` / `defer` restore any mutated state; no leaked goroutines,

temp files (prefer `t.TempDir`), or open handles.

- No `time.Now()`/`rand` without a seam where the result is asserted.

## Severity guide

- CRITICAL: tautological/no-op test, non-deterministic test, `-race` violation,

build-tag self-containment break.

- HIGH: missing negative/edge coverage on changed behavior, re-duplicated

harness primitive, `t.Setenv`+`t.Parallel` combo, real sleep in fast tier.

- MEDIUM: weak assertions, missing `t.Helper()`, poor failure messages.

- LOW: naming, AAA structure, comment nits.