feat(review-loop): loop review→fix→re-review until N consecutive rounds come back empty

[jtomaszewski]

The problem

"Review it until nothing's left" is a loop we already run — but the loop as it exists converges on the wrong thing.

deliver's Phase 7 and the equivalent in other collections re-review after fixing. The re-review is performed by the context that wrote the fix, holding the reason the fix is right. That context checks the change against its own intent instead of against the code: reliably good at catching self-contradiction, structurally blind to whatever it never thought of.

Field data from one run (a 2,400-line spec series, five prior human review rounds, each of which found something new): four self-review passes produced 3 → 3 → 1 → 0 findings. Round 1's best find was a real design hole — but it was a self-catch, an earlier fix claiming one mode inherited safety from another while the doc's own table already said it didn't. That's the shape of what a self-review finds. What it establishes is "no internal inconsistency survives four passes", not "the sixth reviewer finds nothing" — and the run's report had to say so.

Two smaller failures on the same run, both structural:

• A fixed iteration count stops at the wrong place. --max-iterations 3 would have ended that run before the dry round — i.e. before there was any evidence of convergence at all.
• A single empty round is one sample from a non-deterministic reviewer, not a result.

The fix

A new skill, review-loop. It keeps the loop and replaces the reviewer.

Each round fans out fresh read-only subagents across five lenses — correctness & data, security & tenancy, tests (would this fail against the pre-change code?), repo conventions, claims-vs-code — and none of them ever sees the fix rationale, the round number, or the ledger. The prompt-contamination list is explicit and includes the non-obvious one: telling a reviewer it's round 5 and that rounds 3–4 were empty is an instruction to find nothing.

Three mechanics keep that terminating and cheap, each written down with the failure it prevents:

• Dedupe against every finding ever raised, rejected ones included. Dedupe against only the fixed list and a rejected finding returns every round from a reviewer that never heard the argument against it — the loop then runs to the cap every time. The ledger is where a rejection is kept.
• Refute each fresh finding before fixing it. One skeptic subagent per finding, defaulting to refuted when it can't decide. An unverified finding costs a code change, and a code change satisfying a finding that was never real is how a hardening loop makes a change worse.
• Exit on N consecutive empty rounds (--dry-rounds, default 2), capped by --max-rounds (default 6). Real curves decay; the exit condition should watch the decay, not a counter.

The report is the findings-per-round curve plus a claim scoped to exactly what it proves — N consecutive independent passes under these lenses found nothing, naming the lenses and what they structurally can't see (no browser, no runtime, no production data). Never "this change is clean".

Judgement calls — design disagreements, scope questions, trade-offs the repo hasn't decided — get parked as yours rather than fixed or silently dropped. A PR target seeds the ledger with the review comments already on it as inherited findings, so the loop doesn't ignore the review that prompted it.

Local only: posts nothing, pushes nothing, merges nothing, moves no tracker task. It leaves a branch and a report, and hands off to deliver. No confidentiality gate, because it publishes nothing — the gate applies at the deliver step that follows.

Prior art in this repo — and a question

#30 ("bound the review loop, and defer non-blocking findings") attacked the same area from the opposite end: it wanted deliver's loop bounded, because each non-empty external review guarantees another 25-minute poll. It was closed with no comment and no review, and its content never landed — deliver still polls ten minutes and has no Phase 7c.

If that close meant "no review-loop machinery in this collection", this PR is in the same bucket and should be closed too. If it meant "not bolted onto deliver", this is the separate skill.

I took #30's one genuinely load-bearing rule: a round that changed a security boundary (auth, credentials, a trust or permission decision, what a sandbox allows, what leaves the machine) earns one more review even at the cap. Every other class of miss is correctable by a follow-up commit; that one is exposed the moment it merges.

Why standalone, not deliver --until-dry

deliver's loop waits on an external bot, is priced in 25-minute polls, and ends in a merge. This loop is local, subagent-driven, priced in tokens, and ends in a report. Same words, different economics and different terminal state — folding them together would mean one skill with two incompatible stop conditions.

Name pairs with review-queue: that one is many PRs, one pass each; this one is one change, many passes.

Verification

• Version bumped 1.2.2 → 1.3.0 (new skill, per AGENTS.md), README skill table row added, and review-loop added to the repo-agnostic list.
• The version-check workflow's condition checked by hand against this diff: touches skills/**, head version strictly above base by sort -V.
• Every runtime knob it reads already exists in the ## Skill profile (check commands, review landmines, specs location, scratch dir) — no new profile knobs, so nothing for a consuming repo to add before this works.
• Not yet exercised end-to-end as a skill; the mechanics are transcribed from the run described above rather than from a review-loop invocation. Worth knowing before it's trusted unattended.

🤖 Generated with Claude Code

[jtomaszewski]

Reworked the core after feedback: the rounds now run the review the change is actually headed for, instead of five lenses invented in this skill.

• Repo has a review engine installed (om-code-review under .agents/skills/ or .claude/skills/) → each subagent runs that skill verbatim on the diff and reports in its severity scale, fanned out across its checklist sections. Its validation gate is skipped inside the round — a full gate per lens per round is what would make the loop too expensive to run; phase 5's targeted checks and CI still cover it.
• No engine (the usual client repo) → the built-in lenses, plus CODE_REVIEW.md / BACKWARD_COMPATIBILITY.md / agent instructions. This is the review deliver currently discovers one 25-minute Copilot round at a time.

That makes the exit condition mean something: the next real review finds nothing, rather than my own checklist went quiet. New hard rule 3 says so, and the report names which standard ran.