Skip to main content

Code Review Charter

One migration of one demo tenant took ~8 rounds of adversarial review over several days. A forensic post-mortem of the 28 findings (rounds 5–8) found that 82% were hardening a general, reusable engine against inputs the actual one-shot task provably cannot contain — and 100% of the last two rounds were general-engine hardening with zero defects in the delivered behavior. The reviewer verified the real migration correct at round 8 and still returned ten "blockers," every one about a hypothetical input.

Nobody was doing a bad job. The author kept building general machinery; an adversary correctly finds infinite holes in general machinery. The loop had no bottom because two things were missing: a written definition of done, and a way to say "real defect, but out of this change's scope."

This charter supplies both. It is the single contract the author (Claude) and the adversarial reviewer (Codex, or a human, or the repo's reviewer agents) both work from.

The rule in one line: the author declares scope and invariants before code; the reviewer gates severity on them; merge is blocked only by an in-scope invariant violation. Everything else is a follow-up.

Why the loop didn't converge​

Two structural causes, ranked by leverage:

  1. Scope-shape mismatch (root). A one-shot, fully-enumerable job (a fixed 218-entity graph, run once, offline, by an operator) was built as an unbounded, adversary-resistant general engine. A general engine's safety contract must hold against every hostile input, and you can always construct one more. That surface never bottoms out — it generated ~82% of the findings.
  2. "Done" was never written down. With no invariant list, completeness was uncheckable, so the reviewer discovered a new dimension each round (atomicity, then crash-recovery, then binding, then type-safety) — one per round-trip.

A secondary, mechanical accelerant: the reviewer was instructed to "prefer one strong finding over several weak ones" with a memoryless stop-gate — drip-feed by construction — and had no severity category between "blocker" and "clean," so every defensible robustness gap collapsed to no-go.

The tell that a review is in this failure mode: the count of new blockers rises or holds steady across rounds. A converging review's blocker count falls. Recurring finding classes across rounds (the same shape fixed one site at a time) is the same signal.

The four-way severity taxonomy (shared vocabulary)​

Both sides use these, and only the first gates merge.

SeverityDefinitionGates merge?
BLOCKING-IN-SCOPEViolates an invariant on inputs the change declares it handles.Yes
SHOULD-FIXReal in-scope defect, but degraded-not-broken (observability gap, recoverable partial failure).No — author acknowledges
HARDENING-OPTIONALReal robustness gain on a reusable surface, with no in-scope trigger.No — backlog
OUT-OF-SCOPE-HYPOTHETICALOnly manifests on inputs the declared scope excludes.No — recorded for the record

A verdict of no-go requires at least one BLOCKING-IN-SCOPE finding, named, with its violated invariant and the in-scope input that triggers it. If the reviewer cannot name an invariant and an in-scope input, the finding does not block.

Done is now decidable: the invariant ledger is complete and there are zero open BLOCKING-IN-SCOPE findings.

Author obligations (before requesting review)​

For any change with safety / atomicity / concurrency / authorization dimensions, the PR body must carry two sections. (Trivial fixes are exempt.)

1. Scope contract​

State what the change guarantees for the actual task, and what general properties are explicitly deferred — each deferred row with a tracking issue and a fallback.

## Scope contract

**Actual task (what must be correct):** One-shot migration of tenant `contoso` across a
ServiceNow PDI rotation. Input is a fully-enumerated, frozen 218-entity graph, run once,
by an operator, offline, with no concurrent writers.

**Guaranteed for the actual task:**
- Every one of the 218 entities is remapped or explicitly reported unmapped (no silent drops).
- The migration is authorized before any write.
- On any failure the tenant is left in its pre-migration state (verified: re-run == no-op).

**EXPLICITLY OUT OF SCOPE (deferred — do NOT raise as blockers):**
| Deferred property | Why it can't occur here | Fallback if it ever does |
|---|---|---|
| Concurrent migrations of same tenant | Single operator, run once, offline | Tenant advisory lock — #NNNN |
| Hostile/untrusted `connectorId` in payload | Payload is author-generated from the frozen graph, not network input | Validate against the enumerated set — #NNNN |
| Crash-recovery / resumable partial runs | Atomic-or-abort on one box; on crash, re-run from clean backup | Idempotency keys + journal — #NNNN |

**If the actual-task assumptions change** (this becomes a reusable engine, or input becomes
untrusted / concurrent / streaming), this contract is void and the deferred rows become
in-scope — re-open review.

The load-bearing rule: descope in the code OR defend it in review — not both. If the code contains a concurrency guard, concurrency is in scope and fully reviewable. Building general machinery is what silently pulls every hypothetical into scope. If a property is genuinely out of scope, the machinery for it should not exist.

2. Invariant ledger​

Write the complete list of invariants up front — the properties the change must uphold — each marked in-scope or deferred, each mapped to a proof. This is what makes completeness answerable instead of discovered one-per-round.

## Invariants (this change must uphold ALL in-scope rows; each maps to a proof)

| #  | Dimension              | Invariant                                                        | In scope? | Proof |
|----|------------------------|------------------------------------------------------------------|-----------|-------|
| I1 | Authorization          | Verifies caller authority BEFORE the first write                 | YES       | test: rejects-unauthorized |
| I2 | Atomicity              | All-or-nothing: partial failure leaves tenant unchanged          | YES       | test: mid-write abort → no-op |
| I3 | Ordering               | Every guard runs BEFORE the destructive write it guards          | YES       | test + grep: no write precedes its check |
| I4 | Idempotency under retry| Re-running a completed/partial run is a no-op                     | YES       | test: double-run == single |
| I5 | Binding                | tenant/connectorId from a trusted source, never echoed from payload | YES    | grep: no payload field reaches a write key |
| I6 | Truthful reporting     | "success" reported only after the write durably commits          | YES       | test: partial delete never reports success |
| I7 | Type-safety            | Remap covers every node/edge type in the enumerated graph        | YES       | test spans all present types |
| I8 | Crash-recovery         | Resumable after process kill                                     | NO (#NNNN)| — clean-backup re-run |
| I9 | Concurrency            | Safe under simultaneous runs                                     | NO (#NNNN)| — single-writer assumption |

The in-scope rows of the ADR-033 saga (I1/I2/I3/I5/I6) map almost 1:1 onto the findings that were "discovered" serially across rounds — because they were never listed. Written up front, they are built and tested together, and the reviewer checks the table, not their imagination.

3. Fix the class, not the finding​

When a finding lands, before resubmitting:

  1. Name its class (e.g. "guard after destructive write" → ordering; "connectorId from payload" → binding).
  2. Grep the whole diff for every sibling of that shape.
  3. Map it to an invariant. If it maps to none, the ledger was incomplete — add the row, then sweep again.
  4. Fix the whole class in one commit and note the sweep: "Class: ordering — swept 4 sites, fixed 3 (file:line ×3), 1 already correct."

This converts "N rounds for N siblings" into one round for the class. Recurring classes across rounds are the signature this step was skipped.

4. Self-adversarial pass​

Do not self-review with the mental model that wrote the code. Before external review, run the repo's independent reviewer agents against the diff — code-correctness-reviewer and security-auditor — plus, for engine-shaped changes, one prompt framed exactly like the external adversary: "Assume this is a general reusable engine. Find every input that breaks it." Route what it raises through the scope contract: an input that only breaks a deferred row is either evidence the code is too general (delete the generality) or a legitimately deferred row (already documented). This is how you see the round-8 findings at round 0.

Reviewer obligations​

  1. Read the scope contract and invariant ledger first. If a change with safety/atomicity/concurrency/authorization dimensions has neither, that absence is the top finding: return needs-scope and ask for a declaration. Do not assume the change is fully general — that assumption is what turns a one-shot into an unbounded audit.
  2. One exhaustive pass, not drip-feed. In round 1, sweep every invariant class breadth-first and report a verdict for each — including "checked, clean." The deliverable is the complete finding set plus a coverage ledger, not the single strongest doubt.
  3. Gate severity on scope. A defect reachable only on excluded inputs is OUT-OF-SCOPE-HYPOTHETICAL — recorded, never blocking, even if the general contract is genuinely broken. Promote it to BLOCKING-IN-SCOPE only by citing an input in the declared actual task that reaches it.
  4. Honor the convergence contract. After round 1, a new blocker is admissible only if it is (a) a regression introduced by the fix under review, or (b) in a class the prior pass explicitly marked not-reviewable-from-context that new context now covers. A hostile input that was findable in round 1 is a round-1 miss — report it as SHOULD-FIX or HARDENING-OPTIONAL; it does not re-block an otherwise-green, in-scope-correct change. Every post-round-1 finding carries a one-line why_not_findable_last_round.

Verdict format​

Every round returns: (1) an invariant ledger — each class × CLEAN / FINDING / N-A / NOT-REVIEWABLE; (2) the complete finding list, each tagged with severity, invariant_violated, in_declared_scope, triggering_input, file:line, recommendation, and (rounds ≥2) why_not_findable_last_round; (3) a ship decision — no-go iff ≥1 BLOCKING-IN-SCOPE, else go-with-followups.

Round circuit-breaker​

After 2 review rounds that produce new BLOCKERs, stop resubmitting and escalate a scope decision to the human. Do not open round 3 on autopilot. Reach for this early whenever the reviewer has verified the actual task correct and every remaining finding concerns inputs the actual task never contains — that is a scope problem, not a code problem.

## Scope-sanity escalation — <PR> after round <N>

- Rounds so far: <N>. New BLOCKERs per round: <r1>, <r2>, … (trend: rising / falling)
- Reviewer has VERIFIED correct: <the actual task, e.g. "the 218-entity migration runs correctly and reversibly">
- Class of remaining open findings: <e.g. "all concern hostile / concurrent / streaming inputs">
- Do those inputs occur in the actual task? <yes/no + why>
- **Decision requested:**
  (a) DESCOPE — delete the general machinery, ship the one-shot, defer the rest to #NNNN; or
  (b) CONTINUE — we genuinely need the general engine now; accept M more rounds; or
  (c) ABANDON — the effort isn't worth the remaining cost.
- Author recommendation: <one line>

This is the check that would have ended the ADR-033 review at round 2 or 3.

Worked example: how this collapses ADR-033​

  • Scope contract declares: one-shot, frozen 218-entity graph, single operator, offline. Concurrency, crash-recovery, untrusted-payload, and streaming are deferred rows with tracking issues.
  • Descope-or-defend forces the choice: because those are deferred, the concurrency/CAS/retry/fencing machinery should not exist — the executor becomes a minimal apply-only tool pinned to the verified manifest. ~23 of 28 findings become inexpressible: there is no input surface to attack.
  • Reviewer gates severity on the contract: the ten round-8 "blockers" are all OUT-OF-SCOPE-HYPOTHETICAL (the reviewer had already verified the real migration correct). Verdict: go-with-followups.
  • Invariant ledger makes done decidable at round 1: I1/I2/I3/I5/I6 in-scope with proofs; I8/I9 deferred and visible.

Eight rounds → one exhaustive pass plus, at most, one class-sweep.

Relationship to the tooling​

This charter is the source of truth. The following are downstream reinforcements, applied separately:

  • sv0-skills/review-pr/SKILL.md — rewritten from a routing checklist into a scope-gated flow that reads the contract first, severity-gates, and runs the circuit-breaker. (pending)
  • sv0-platform/.claude/agents/{code-correctness-reviewer,security-auditor}.md — a scope-gating paragraph so the self-adversarial panel honors declared scope; a class-first step. (pending)
  • Codex reviewer prompts — the same scope-binding block, convergence contract, and four-way taxonomy. These live in a plugin cache and are overwritten on update, so the durable form is this charter plus a per-PR scope block fed to the reviewer. (pending)

TL;DR checklist​

Author, before every review request:

  • Scope contract present; every deferred row has a tracking issue + fallback.
  • Invariant ledger present; each in-scope row maps to a proof.
  • No code exists for a property marked deferred (descope in code OR defend in review — not both).
  • Ran the self-adversarial panel; cleared its BLOCKER/MAJOR.
  • For each prior-round finding: named its class, swept the diff, fixed the class in one commit, noted the sweep.
  • Round 3+ with only deferred/hypothetical findings left → escalated a scope decision instead of resubmitting.

Reviewer, every round:

  • Read the scope contract + ledger first; if absent on a safety-relevant change, return needs-scope.
  • One exhaustive pass; return a coverage ledger, not the single strongest doubt.
  • Every finding severity-tagged; only BLOCKING-IN-SCOPE gates merge.
  • Post round 1, each new blocker carries why_not_findable_last_round; findable-last-round → not a blocker.