Reviewing Changes

A diff is a bet on how the repo looks in two years and on how cheaply the next reader (human or model) understands it. Review the residue the change leaves, not just its lines.

Violating the letter of a check is violating its spirit. "Minor", "out of scope for this PR", "they probably know" are not reasons to drop a finding. No evidence, no finding: a finding you cannot re-open at a file:line in the head tree does not exist. Both rules hold at once.

Inputs

Work from local git only — host-independent and cheaper than any API.

Procedure — read hunks, then read around them

1. Map: git diff --stat $RANGE and git log --oneline $RANGE. Classify every file: source · test · doc · schema/migration · generated/lock/vendored · config. Generated and lock files are never read; only verify they were regenerated with their source (check 27). Large diff (> 15 source files or > 800 changed lines): cluster files by concern (one row per cluster in the ledger, step 7) and review every cluster in turn. Never sample "representative" files; a cluster you did not read is listed under Not reviewed, never silently merged.
2. Claims: write the description's promises as a checklist. Add "no unrelated changes" and "no behavior change outside the stated scope" as implicit claims.
3. Hunks: git diff $RANGE -- <source+test files>. For context read the smallest window: git show <head>:<file> | sed -n 'A,Bp'. Never cat a whole file unless the hunk cannot be understood without it.

4. Context — for every changed function, read the five neighbours. Judging a hunk by its own lines is how reviews miss. Per changed/added/removed function or symbol (each answer lands in that hunk's ledger row, step 8):

   Neighbour	Command	Answers
   callers	git grep -n -F '<name>' <head>	blast radius (check 4); zero readers (12)
   its tests	git grep -l -F '<name>' <head> -- '*test*' '*spec*' then read each assert	would this assert still pass with the change reverted or the function returning its zero value? (8)
   its guard	for a read/write on tenant/user/owner data: the line that authorizes it, compared with the sibling operation's guard	authorization present and equal to siblings' (3)
   invariant comments in the touched file	git show <head>:<file> | grep -n -E 'must|never|before|after|assumes|because|INC-|ticket'	did the diff violate a stated constraint? (33)
   history of every removed/rewritten line	git log -S'<removed fragment>' --oneline -- <file> or git blame <base> -L A,B -- <file>	was that line a fix? is this a regression? (34)

   Also read the repo's instruction files once (CLAUDE.md, AGENTS.md, CONTRIBUTING*, docs/adr*) and treat every rule in them as a claim the diff must meet (check 5). When codebase-memory is indexed, detect_changes(since=<base>) + trace_path replace the caller grep.

5. Sweep — the repo already has it? Run the mechanical sweep before judging:

   python3 <this skill's directory>/sweep.py <base> <head> [repo-dir]

   It reads only the added/removed lines and asks the head tree: literal/number already a constant somewhere? (p1/p2) · helper concept already implemented? (11) · interface with one implementation? (10) · new symbol with zero readers? (12/p21) · doc mentioning a touched name or behavior word? (14) · loop with a query inside? (16) · skipped tests, debt markers, empty catch, proxy checks (7, 8, p11). Every row with an existing: / ZERO readers / doc hit is verified at its file:line and becomes a finding or is explicitly dismissed with a reason in the Sweep section. The script is a heuristic: still read the hunks; but you may not report "no existing equivalent" for anything it listed.

6. Route the checks from checks.md: always run Blocking (1–4), Design (5–7, 10), Proof (8, 9), Context (33–35) and Health 11, 12, 14, 16, 20. Load a conditional section only when its predicate is true for this diff:

   Predicate (observable in step 1/3)	Load
   new external call, queue, transaction, retry, timeout, fire-and-forget	Health 17, 18
   schema, migration, data backfill, destructive column/table op	Health 15
   manifest / lockfile / new import from an unknown package	Health 19
   prompt strings, model calls, tool definitions, rendering model output	Legibility 28
   file > 600 lines touched, new barrel/index/re-export, reflection/scan/registry	Legibility 21, 23

   A claim in the description does not excuse a pattern: "repository so we can swap storage later" is speculative generality (check 10), not a justification for it. The absence of something the description never promised (a router, a UI) is not a finding — put it under Could not verify.

7. Verify every candidate finding before it is written. For each one, re-open the head tree and answer all four; a "no" on any of them removes or reclassifies it:

   Question	If no
   Is the line changed or added in this range, or a caller/doc/test made wrong by this range?	move to Pre-existing — one line, not a finding, not counted in the verdict
   Can I state a concrete input → wrong outcome (or a concrete existing alternative at file:line)?	drop it; "might", "could be fragile" is not evidence
   Did I grep for a guard elsewhere (middleware, base class, DB constraint, caller-side check) that already handles it?	drop or downgrade; name what you grepped
   Is the severity taken from the table below, not from how alarming it reads?	fix the tier

   Zero verified findings is a valid, complete report. Do not pad with LOWs to look thorough.

8. Report using the contract below. Then stop. No fixes unless asked.

Output contract

Verdict: BLOCK | MERGE WITH DECLARED DEBT | MERGE
Claims: <claim> — met | not met (file:line) | partially (file:line)

Ledger (REQUIRED: one row per source hunk, or per cluster on a large diff; every slot filled)
file:A-B · <what it changes, ≤ 8 words>
  callers: <n> (<file:line> …) | 0
  tests: <test file:line> fails-without-this-change? yes | no (why) | none
  auth-guard: <file:line of the check protecting this read/write> | none | n/a (no read/write)
  invariants: ok | violated <comment file:line>
  history: <hash subject of the commit that added each removed line> | n/a (nothing removed)
  → clean | F1, F3

Sweep (REQUIRED: one row per sweep hit)
<added thing> @ file:line → existing: <file:line> → F<n> | dismissed: <reason>

Findings (most severe first; ONE line each, ≤ 40 words after the fix)
F1 BLOCK · check <n> · file:line · <concrete failing input → wrong outcome> · fix: <one sentence>
F2 HIGH  · check <n> · file:line · ...
F3 MED   · ...
F4 LOW   · ...

Pre-existing (not introduced here; informational): file:line · <one clause>
Declared debt (shortcuts the author named): each → task/handoff ref | "none filed" (→ finding)
Not reviewed: <clusters/files skipped and why> | none
Could not verify: <what needed a runtime/DB/secret you do not have>

Severity is assigned by check, not by tone. Find the row; do not promote.

Verdict = highest tier present: any BLOCK → BLOCK; HIGH/MED with declared debt filed → MERGE WITH DECLARED DEBT; otherwise MERGE. Style, formatting and praise are never findings. Findings of the same pattern at many sites collapse into one row listing the sites.

Red flags — you are about to under- or over-review

• "The description says it does X" → you have not verified X at file:line.
• "Small diff, quick pass" → small diffs remove tests and add proxies too.
• "That file wasn't changed" → blast radius, invariants and tests live in unchanged files.
• "The test still passes" → passes because of the change, or regardless of it? Read the assert.
• "That line was just cleanup" → run the history command first; cleanup deletes fixes.
• "Probably intentional" → intentional debt is declared; undeclared is a finding.
• "Style nit" → drop it. If it is really pattern 1–21 or check 10, it is not style.
• "This looks dangerous" → severity comes from the table, not from the adjective.
• A finding without file:line, a concrete input, and a check number → it is not a finding yet.
• A hunk with no ledger row, or a row with an empty slot → you did not review it. Fill the slot or list the hunk under Not reviewed. auth-guard: none on a read/write is finding material (check 3); tests: no is finding material (check 8); history: <fix commit> is finding material (check 34).