review-loop · diff
git:20260605.d3c4f1b to git:20260730.e837d97
155 added, 9 removed. Audit A to A.
---
description: 'Multi-round adversarial code review loop — four specialized agents (correctness,
adversarial, API design, test rigor) run in parallel against a PR, repeated until
all four return `CLEAN — ship it` or a budgeted number of rounds (default 5) is
exhausted. Use when the user wants a thorough review of a substantive PR — new interfaces,
contract changes, load-bearing refactors — and has signalled they want both correctness
AND polish. Heavy: up to 5 rounds × 4 agents = 20 sub-invocations, so not for tiny
bug fixes, WIP sketches, or doc-only PRs. Invoke explicitly; do not auto-trigger
from a generic "review this" request unless the user names the high-bar mandate.'
disable-model-invocation: true
name: review-loop
---
# Review loop
A four-lens adversarial review pattern, iterated until the team converges or a
budgeted number of rounds elapses. Use it when the user wants the bar set
high on both **correctness** and **polish** — substantive PRs where a single
review pass would miss the second-order issues four lenses catch
independently.
Loop logic:
```
for round in 1..N: # N defaults to 5
- launch 4 agents IN PARALLEL: correctness, adversarial, api-design, test-rigor
- wait for all 4 to report
+ launch 4 READ-ONLY agents IN PARALLEL: correctness, adversarial, api-design, test-rigor
+ wait for all 4 to report # they report; they never edit
synthesize findings (convergent vs single-reviewer; rank by severity)
if every reviewer says "CLEAN — ship it":
stop # convergence reached
- fix every actionable finding # disputed findings → user mandate decides
+ audit each finding yourself # reviewers are wrong sometimes
+ fix every accepted finding # MAIN SESSION ONLY — see below
commit + push
end
report final status
```
+ ## The single-writer rule — non-negotiable
+
+ **The four reviewers are READ-ONLY. The main session is the only process that
+ edits the worktree.** Reviewers produce reports; the main session audits them,
+ decides what to accept, and makes every change itself.
+
+ This is not a style preference. Four agents editing one worktree in parallel
+ corrupts it, and the failure modes are quiet:
+
+ - A reviewer leaves a probe file behind (`zz_probe_test.go`), and the package
+ stops compiling for everyone — including the next round's reviewers, who then
+ report a "build failure" that is really litter.
+ - A reviewer break-verifies by mutating a source file and doesn't restore it. The
+ mutation is now indistinguishable from the main session's in-flight work. In one
+ real run a reviewer's stream-swap sat in `root.go` long enough that it was nearly
+ committed; it was caught only because a test written minutes later happened to
+ fail on it.
+ - A reviewer notices *another* reviewer's litter and helpfully runs
+ `git checkout -- <file>` to clean up. That silently discarded the main session's
+ uncommitted fix to the same file, which surfaced later as an unexplained build
+ break.
+
+ Note the shape of the last two: the tree was wrong, and nothing said so. You find
+ out from a build failure, a mystery test result, or not at all.
+
+ So:
+
+ - **Every brief must end with an explicit read-only instruction** (see Step 1).
+ - **Break-verification is the MAIN SESSION's job**, not a reviewer's. It mutates
+ source, so only the single writer may do it. A reviewer that wants a mutation
+ measured says so in its report; you run it.
+ - **A reviewer that genuinely must measure a mutation** does it in a throwaway
+ clone outside the worktree (`git clone . /tmp/…`), never in place — and says
+ which in its report.
+ - **Before every commit, confirm the tree holds only your own changes.**
+ `git status --porcelain` plus a skim of `git diff` beats trusting four agents to
+ have cleaned up. If something appeared that you did not write, find out what it
+ is before committing — do not `git checkout` it reflexively either, since it may
+ be your own uncommitted work.
+
+ Reviewers are still free to *run* things: builds, tests, `go vet`, `git log`,
+ `grep`. Read-only means no writes to tracked files and no new files in the
+ worktree.
+
## When to use
Use it for:
- New interfaces / public API surface.
- Non-trivial refactors that touch contracts (renames, lifecycle
changes, cross-package plumbing).
- Anything load-bearing that the user has signalled they want
"right", not just "working".
Don't use it for:
- Tiny bug fixes, doc-only PRs, or work-in-progress sketches — the
overhead is wasted.
- PRs where the user wants a quick sanity check, not a thorough
review — use a single review agent instead.
If the user's request is ambiguous ("review this PR" with no
mandate signal), ASK before kicking off the loop. The cost is real.
## Step 0: framing
Before launching agents, you have to know what the PR is. Collect:
- **PR URL or branch name** the agents will read from.
- **What changed in this round** — a concise summary of the diff
against the prior reviewed commit, OR against main on the first
round.
- **What previous rounds found and fixed** — carried forward into
each round's brief. Fresh subagents don't have the conversation
history; they need that context to avoid re-flagging closed items.
- **The user's mandate**, in their own words: "ship it ASAP",
"make it bulletproof", "fix everything", etc. This decides what
to do with single-reviewer LOWs and reviewer disagreements.
## Step 1: launch the four agents in parallel
The agents run as separate subagents, in parallel, **in the same
message** (parallel tool calls). Each gets a self-contained brief
with:
1. The PR identifier + commit hash + branch (so they can read the
actual files; do NOT paste the diff into the prompt — it'll
exceed budget).
2. Their lens (which one of the four).
3. A short summary of what's in the PR and what changed in this
round.
4. A short summary of what previous rounds found and fixed.
5. A specific list of angles to consider (the lens-specific
section below).
6. The expected output shape: ranked punch-list, file:line refs,
severity levels (`BLOCKER` / `ISSUE` / `NIT` / `CLEAN`), word
cap (~300–450 words).
7. **The self-termination signal**: if they have nothing real,
they must say `CLEAN — ship it` so convergence is
programmatically detectable.
+ 8. **The read-only instruction** (see below). Every brief, every
+ round — the lens templates that follow omit it only because it
+ is identical for all four.
Use the **general-purpose** subagent type. Code review needs
reading whole files and reasoning across them; `Explore`-style
agents read excerpts and miss content past their read window.
+ ### The read-only clause — append to every brief
+
+ Paste this verbatim at the end of each of the four briefs:
+
+ > **CRITICAL — you are READ-ONLY. Change nothing in the worktree.**
+ > Do not edit, create, or delete any file, and do not run
+ > `git checkout` / `git restore` / `git stash` — the main session
+ > has uncommitted work in this tree, and reverting a file you did
+ > not write destroys it.
+ >
+ > You may freely READ and RUN: `git log`/`show`/`diff`, `grep`,
+ > builds, tests, `go vet`. If a finding needs a mutation measured
+ > (delete this line, invert that check — does any test fail?),
+ > either describe the mutation precisely in your report so the main
+ > session can run it, or measure it in a throwaway clone outside
+ > this worktree (`git clone . /tmp/probe-$$`) and say that you did.
+ >
+ > Before finishing, run `git status --porcelain` and confirm it is
+ > empty. State that in your report. If it is NOT empty, do not
+ > "fix" it — say exactly what you saw; it is probably the main
+ > session's in-flight work.
+
+ Two reasons this is a paste-in clause rather than a summary:
+ `general-purpose` agents have write tools and will use them to be
+ helpful, and a reviewer that reports "tree clean" gives the main
+ session a cheap cross-check that costs one line.
+
### Lens 1 — Correctness
> You are doing a CORRECTNESS code review of <PR>. The repo is at
> <path> on branch <branch>; commits <commit-list>.
>
> **What changed in the round you're reviewing:** <summary>
>
> **What previous rounds covered:** <prior-rounds-summary>
>
> **Your job, three layers:**
>
> 1. **Validate the round-N fixes are correct.** Each fix was
> break-verified at commit time, but I want a second pair of
> eyes. For each non-trivial fix, walk through what happens:
> - on the happy path,
> - under each error / panic / Goexit path,
> - under concurrent or unusual inputs the author may not have
> considered.
>
> 2. **Regression-scan the fixes for new bugs.** Anything the
> round-N fixes broke or made subtler? Anything the rename /
> refactor / move shifted in a way that's now wrong?
>
> 3. **What did previous rounds miss?** Fresh eyes. Read the
> actual files; don't trust prior summaries:
> - <file 1>
> - <file 2>
> - <file 3>
>
> Report as a punch-list with file:line refs and severity
> (BLOCKER / ISSUE / NIT / CLEAN). If no real findings, say
> `CLEAN — ship it`. Don't manufacture findings. Under 350 words.
### Lens 2 — Adversarial
> You are doing an ADVERSARIAL code review of <PR>. The repo is at
> <path> on branch <branch>; commits <commit-list>.
>
> **What changed in this round:** <summary>
>
> **What previous adversarial rounds flagged + what was fixed:**
> <prior-rounds-with-disposition>
>
> **Your job — three layers, adversarial.**
>
> 1. **Did this round close the holes the previous adversarial
> pass flagged?** For each prior finding: read the fix, attack
> it. The fix that closes a hole on the happy path often leaves
> the error / Goexit / panic / typed-nil / concurrent paths
> still open.
>
> 2. **Did this round introduce new attack surfaces?** New code,
> new invariants, new contracts. What hostile inputs /
> lifecycles / orderings / concurrency could break it?
>
> 3. **What did the previous adversarial passes still miss?**
> Fresh eyes. Specifically consider:
> - Typed-nil pointers, untyped nils, nilable kinds beyond
> pointers (map/chan/func/slice/interface).
> - Defer ordering (LIFO), panic during deferred-arg
> evaluation, Goexit unwinding through pending I/O.
> - Goroutine lifecycle: leaks, blocked sends/receives on
> buffered vs unbuffered channels, cleanup ordering.
> - Process-global state (os.Stderr swaps, log defaults, env
> vars) and cross-test pollution.
> - Hostile-input injection: ANSI / control bytes / embedded
> newlines in attacker-influenced strings flowing through
> styling or logging code.
> - Doc-vs-code gaps: anywhere the doc promises behaviour the
> code does not enforce.
>
> Report as a ranked punch-list (most-concerning first) with
> file:line refs and severity. If no real findings, say
> `CLEAN — ship it`. Don't manufacture findings. Under 450 words.
### Lens 3 — API design
> You are doing an API DESIGN review of <PR>. The repo is at <path>
> on branch <branch>; commits <commit-list>.
>
> **What changed in this round:** <summary>
>
> **What previous API-design rounds asked + how this round
> answered:** <prior-rounds-with-disposition>
>
> **Your job:**
>
> 1. **Validate the round-N design decisions.** Each one was a
> trade-off. Walk each one against alternatives. Examples to
> consider when relevant:
> - Interface naming: does it describe what the IMPLEMENTOR
> does (PluginIngester precedent) or what the parameter is
> named?
> - Closure / restore-handle vs symmetric setup/teardown pair
> (signal.Notify/Stop, log.SetOutput-via-defer,
> context.WithCancel).
> - `any` vs nominal interface parameter — type safety vs
> cycle-breaking.
> - Structural vs nominal interface duplication; when is the
> compile-pin "good enough" and when does a neutral shared
> package pay off?
> - Shared test helpers vs per-package copies (httptest
> precedent).
> - Doc-only contracts vs runtime-enforced contracts.
>
> 2. **Critique the new code.** Names, shapes, doc quality
> (compared against established precedents in this codebase).
>
> 3. **Ship readiness.** Anything still blocking? Anything to
> defer to a follow-up? Anything you've over-engineered across
> the rounds?
>
> If shipping: say `CLEAN — ship it`. Otherwise, name what one
> more round must address. Under 400 words.
### Lens 4 — Test rigor
> You are doing a TEST RIGOR review of <PR>. The repo is at <path>
> on branch <branch>; commits <commit-list>.
>
> **What changed in this round:** <summary>
>
> **What previous test-rigor rounds asked + how this round
> answered:** <prior-rounds-with-disposition>
>
> **Your job:**
>
> 1. **Validate the new tests.** Read them yourself:
> - Do the assertions actually prove what they claim, or could
> they pass for the wrong reason (vacuous green)?
> - For each test pinning a documented contract, walk the
> "would this fail if the contract were silently violated?"
> question.
> - Are positive and negative assertions both load-bearing,
> or is one decorative?
> - For each ANSI / regex / byte-sequence pin: is it too
> tight (brittle to correct refactors) or too loose (matches
> unrelated bytes)?
>
> 2. **What's still missing?**
> - Coverage gaps per-package vs interface-level.
> - Race detection — anything that needs `-race`?
> - Lifecycle / cleanup / Goexit / panic paths.
> - Doc claims with no enforcing test.
>
> 3. **Test maintenance.** Duplication, fragility, naming. Is the
> suite a sustainable contract anchor or a maintenance trap?
>
> Read the files. If sufficient: say `CLEAN — ship it`. Otherwise
> name what's missing. Under 350 words.
## Step 2: synthesize
When all four reports are in, build the punch-list:
1. **Convergent findings** — flagged by ≥ 2 reviewers — go to the
top. Two independent reviewers naming the same hole is much
stronger signal than one.
2. **Single-reviewer findings** are ranked by severity (the
reviewer's own rating, sanity-checked). A single MEDIUM is
worth doing; a single LOW is judgement.
3. **Reviewer disagreements** are normal — one says "add this
test", another says "YAGNI". Decide by the user's mandate:
- "Ship it ASAP" → defer disputed items.
- "Fix everything / make it bulletproof" → take the more
defensive side, even if a minority of one.
- Unclear mandate → tell the user and ask.
4. **`CLEAN — ship it`** from a reviewer counts as zero findings
from that lens. Programmatically check for the substring to
detect convergence.
- ## Step 3: fix
+ 5. **Audit each finding before accepting it.** Reviewers are
+ confidently wrong often enough that this is a real step, not a
+ formality. Verify the claim against the code yourself — a
+ `grep`, a `go list -deps`, a two-line probe. In practice this
+ catches two kinds of error: a claim that is simply false (a
+ reviewer flagged `%s` interpolations as unsanitized when the
+ values were a self-sanitizing type), and two reviewers reaching
+ opposite conclusions where one is *factually* right rather than
+ merely more cautious (one said a deleted test was subsumed by
+ its replacement; the other showed it was not, by recolouring the
+ thing and watching the suite stay green). Verified facts beat
+ reviewer confidence, and beat vote counts.
- Apply every finding in the synthesized punch-list. For each fix:
+ Say so when you overrule a reviewer, and — if the loop
+ continues — tell that lens next round that you did, so it can
+ push back if you were the one who got it wrong.
+ ## Step 3: fix — the main session, and only the main session
+
+ Apply every accepted finding yourself. Reviewers do not edit (see
+ the single-writer rule); this step is where all changes happen.
+
+ For each fix:
+
- Make it the smallest correct change. Don't snowball.
- **Break-verify** non-trivial fixes: temporarily induce the bug
(comment out the fix, sed-corrupt the contract), confirm the
relevant test fails CLEANLY (the failure message points at the
right thing), then restore. This catches "test passes for the
wrong reason" before the next review round does.
+
+ Because break-verification mutates source, it belongs to the
+ single writer. Back the file up first (`cp f /tmp/f.bak`),
+ restore from that backup rather than from git — `git checkout`
+ would also wipe your other uncommitted edits to that file — and
+ confirm `git diff` is back to your intended change before moving
+ on. A mutation left in place is the single worst thing you can do
+ to this loop: the next round's reviewers will report it as a
+ finding, and you may commit it.
+
+ Two failure modes worth naming, both observed:
+ *the mutation didn't apply* (a `sed`/`replace` that silently
+ matched nothing, so "no test failed" meant nothing was tested),
+ and *the test under test was a copy* (a helper duplicating the
+ production function's body, so mutating production changed
+ nothing). Assert the mutation landed, and make sure the test
+ calls the real thing.
- Update docs (CHANGELOG, architecture, prompts) in the SAME
commit. The reviewers WILL flag drift.
- Commit with a clear message naming which round's findings the
commit closes.
## Step 4: repeat (until convergence or budget)
- Push. Launch the next round's four agents. Each one's brief
- carries forward the running summary of what previous rounds found
- and what this round's commit just fixed — fresh agents need that
- context to avoid re-flagging closed items.
+ Confirm the tree is clean (`git status --porcelain` empty), then
+ push. Launch the next round's four agents. Each one's brief carries
+ forward the running summary of what previous rounds found and what
+ this round's commit just fixed — fresh agents need that context to
+ avoid re-flagging closed items.
+ Pin the round's commit SHA in every brief and tell them to review
+ **that** commit. Reviewers start at different times and a long
+ round can overlap your next edits; naming the SHA means all four
+ judge the same tree, and a report referencing a line you have since
+ moved is immediately recognizable as stale rather than confusing.
+
## Step 5: terminate
Stop when EITHER:
- **All four reviewers in the most recent round emit
`CLEAN — ship it`**, OR
- **The configured round budget is exhausted** (default: 5).
If the budget is exhausted with unresolved findings, report the
residual to the user with a clear ship-it / one-more-round / defer
recommendation. Don't silently merge with open issues.
## Lessons from the field
Hard-won observations from the loop that this skill captures:
+ - **Reviewers report; one process writes.** The most expensive
+ problem in a real run was not a missed defect — it was four
+ agents editing one worktree. Litter left behind, mutations not
+ restored, and one reviewer "helpfully" `git checkout`-ing a file
+ the main session had uncommitted work in. Every symptom was
+ quiet: a build break with no obvious cause, or a mutation that
+ almost got committed. Isolated worktrees per reviewer would also
+ solve it, but read-only reviewers are simpler and lose nothing —
+ a review's output is a report, not a diff.
+ - **The recurring defect has one shape: behavior whose deletion
+ breaks no test.** Across five rounds of one PR, every round's
+ real finding was that — the installation of a handler, a
+ per-stream decision, a mutex, a colour. Each was found by luck,
+ by a reviewer happening to try that mutation. If a loop keeps
+ surfacing this, stop reading and start *sweeping*: mechanically
+ mutate every behavior the change introduces and record
+ CAUGHT/UNPINNED per item. One sweep found 13 gaps that four
+ rounds of careful reading had missed. Consider spending a lens on
+ it from round 1.
+ - **The second recurring defect is stale prose.** Comments and docs
+ describing code the same commit moved: a comment citing a symbol
+ that never shipped, a doc asserting the mechanism a fix had just
+ replaced, sample output the code cannot produce. Nine instances
+ in one PR. After any rename or deletion, grep for the old name
+ *and* for claims about the old behavior — including in files you
+ did not touch this round. Fixing the website and leaving the
+ CHANGELOG is the characteristic version of this.
- **Four lenses, not three.** Correctness alone misses the design
smell, adversarial alone misses the design rationale, API
design alone misses the test coverage gap, test rigor alone
misses the attack surface. The combinatorial coverage is the
point.
- **Parallel, not sequential.** Running them serially lets one
reviewer's output bias the next. Parallel keeps the lenses
independent.
- **General-purpose agents, not Explore.** Code review needs
reading whole files and reasoning across them; Explore reads
excerpts and will miss content past its read window.
- **Word caps matter.** Without a cap, reviewers manufacture
findings to fill space. With a cap (~300–450 words), they
prioritise.
- **Severity labels (`BLOCKER`/`ISSUE`/`NIT`/`CLEAN`) prevent
drift.** Reviewers without a vocabulary will rate everything
equally important.
- **Convergence trumps unanimity.** Three SHIP-IT + one LOW from
a minority of one is convergence. Four SHIP-IT is unanimity.
Either terminates the loop; the difference is whether you act
on the dissent before merging.
- **The user's mandate is load-bearing.** "Fix everything" and
"ship it" point at opposite responses to disputed findings.
Get it before the first round; carry it through every
synthesis.
- **Reviewers reverse themselves.** It's normal for round-N's
API design reviewer to say "keep per-package" and round-N+1's
to say "consolidate". They're reading a different tree; the
rationale often changes as the surface stabilises.
- **Break-verify every non-trivial fix.** The number-one source
of "test passes for the wrong reason" is a fix that doesn't
actually exercise the thing it claims to. Sed-corrupt, run the
test, watch it fail with a clear message, then restore.