2.7 KiB
Adversarial review procedure (#151 — structuralized)
Binding for every P0/P1 change and every bug-fix discharge. The doctrine and
evidence live on the issue (mutation-testing formalization, Google's reviewer
checklist, Hypothesis property tests); this document is the operating
procedure. Enforcement: dispatch templates (docs/reviewer-dispatch-template.md)
carry the mandatory steps; the bug-fix contract (docs/bugfix-contract.md)
carries the discharge-side half.
The problem this kills
LLM-authored changes share wrong assumptions between implementation, tests, and docs. An implementer that writes its own tests tests its own interpretation. A reviewer that reads the implementation first anchors on it. Cosmic Ray's formulation: if a test suite passes on mutated code, you have a mismatch between your tests and your functionality.
The procedure (in order, no steps skipped)
- Freeze intent. Issue acceptance criteria + synthesis kill test, written down before any test exists.
- Black-box test author (fresh-context agent, P0/P1 required): writes behavioral tests from the issue + synthesis + plan ONLY — never sees the implementation. These tests are the spec.
- Enumerate plausible incorrect implementations. Before inspecting any implementation-specific test, list the wrong implementations the frozen tests would NOT catch. This list goes in the plan.
- Only then inspect implementation-specific tests; reconcile.
- Run both suites. Black-box + implementer suites must both pass.
- Mandatory reviewer question (spec gate, verbatim): "What incorrect implementation could still pass these tests?" A review that cannot answer is incomplete and bounces.
- Mutation spot-check (proportionate): for pure logic changed by P0/P1 PRs, one manual mutant per load-bearing branch (flip a comparison, swap an operand) — if the frozen tests still pass, the tests are vacuous and the finding is BLOCKER-grade. (Full mutation tooling lands with #130's calibration harness; spot-checks until then.)
Role separation (hard rules)
- The implementer never writes the spec tests for P0/P1 changes — the black-box author does.
- The spec gate never sees the implementation before answering the mandatory question.
- The black-box author's tests are frozen — implementer or reviewer may extend, never weaken (bug-fix contract §4).
Proportionality
- P0/P1 features and all BLOCKER/MAJOR discharges: full procedure.
- MINORs and mechanical fixes: steps 3+6 (enumerate + mandatory question), spot-checks at reviewer discretion.
- Workflow/config changes: the kill test is the protected CI run itself, stated explicitly in the synthesis (contract §kill-test).