A Re-Review's Base Is the Last Fully Covered Pass
Problem¶
SPEC-216 lets a review pass after the first select its reviewers from the diff since a base, the head of an earlier pass, instead of from the whole branch. The roster gets smaller and cheaper. The risk is the reverse of what it looks like: if the base moves past code that a reviewer was owed and never saw, a later pass selects that reviewer from a diff that no longer contains the code. That reviewer then never sees the code at all, and every gate passes.
Review passes 3 to 7 each found one new High of this kind, and each was a different way to move the base too early:
| Pass | How the base moved too early |
|---|---|
| 5–6 | A pass counted once the complete event existed, although one dispatched reviewer had not returned (P6-SEC-1) |
| 6 | A narrowing complete for an older pass was written after a later pass had already used it as its base (P6-SEC-6, P7-SEC-4) |
| 7 | A reviewer was selected but omitted from the dispatch, and the base only checked the dispatched set (P7-DIG-1) |
| 7 | check-gate tied reviewers_returned in one direction only, so a gate could claim reviewers the ledger never completed (P7-SEC-3) |
Solution¶
The base is the head of the last pass whose complete names the whole selection and the whole
dispatch. Nothing less counts:
if sel is not None and disp and set(sel['selected']) | set(disp[0]['dispatched']) <= returned[p]:
base = sel['head']
The rules around that test:
- A partial return does not move the base. The pass stays usable as a review, but the next pass selects from the old base.
- An omission does not move the base either. A selected reviewer the pass did not dispatch is owed the same code as one that did not return.
- History is append-only in time. A
completefor pass N is refused once pass N+1 is dispatched, including a narrowing one, because pass N+1 may already have used pass N as its base. - The gate and the ledger agree in both directions.
reviewersequals the dispatch event's list exactly, andreviewers_returnedequals what thecompletenames. A pass without acompletecounts as nothing returned, not as everything.
Prevention¶
- When a step narrows who looks at what, state the invariant as coverage: every reviewer who is owed a range of code sees it in some pass. Then test each way that range can be lost (a missing return, an omission, a late correction, a gate over-claim) as its own mutant. One fixture per rule was not enough here: each fix closed one route and the next pass found another.
- Fall back to the broad selection whenever no pass qualifies. A wider roster costs money. A narrower one can hide a defect permanently.
Related: docs/solutions/patterns/verdict-needs-a-denominator.md, and the SPEC-216 decisions
(specs/216-scoped-reviewer-execution-and-re-review-selection/decisions.md, Decisions 18 and 20).