A review is a set of findings a second reader can act on without the reviewer in the room, and a decision. Everything else is noise.
The rules
| Id | Rule | In practice |
|---|---|---|
| REVIEW-1 | Findings on the introducing change, at the head sha | Open the change's head sha; every file:line is at that sha, or reads "from the inline diff" when there is none. A defect that predates the change goes to its own item, not this review. Essential findings only. |
| REVIEW-2 | One finding per thread, in Conventional Comments | Label, defect, file:line, consequence, rule broken, required behaviour, at most one suggestion. No hedges. Format in references/finding-format.md. |
| REVIEW-3 | The summary carries the decision | Two to four lines. The last one is the decision: approve, request changes, or needs a decider, with the open point and who decides. |
| REVIEW-4 | Mutate before trusting a test | Judge a test against the code it exercises, not its name. Invert one condition the change adds, run the suite, restore the file. A mutant that survives is a finding naming the missing case. A mutant that changed nothing proves nothing. Report mutants and catches as a count. Procedure in references/mutation-check.md. |
| REVIEW-5 | Review rounds fix only real defects | Cosmetic bot suggestions get a one-line verdict and no push. On your own change, answer every bot thread (fixed, or refuted with evidence) and resolve it. On someone else's change, reply and leave resolution to the author. |
| REVIEW-6 | Confirm the diff is absent before merging | Before reporting on, merging or closing a change, confirm its diff is not already on the target. Check a stacked change against its parent's tip. Re-diff a conflict resolution against the target's current file. |
| REVIEW-7 | Adversarial self-review before showing | Which claim rests on something not opened this session. Which number was recalled rather than computed. Which action does damage if the premise is wrong. Fix what it finds before any push, merge or live change. |
| REVIEW-8 | Sort proposals by what ships | Internals-only, or changes the released artifact (versioning, signing, distribution). The second needs a strong reason, stated. |
| REVIEW-9 | One dependency per change | Changelog read, package isolated, suite before and after, lockfile diff reviewed, never hand-edited. |
| REVIEW-10 | Test and infrastructure work leaves shipped code alone | No change to shipped code without an explicit ask. Negative-test probes run in a worktree or a temporary repository. |
The walk
- Understand the intent: what the change claims, which plan step or ticket it implements, what behaviour changes. Say up front whether it breaks existing behaviour.
- Read the tests first, then run REVIEW-4 on at least one new condition when a suite can run; otherwise the summary says "mutation check not run".
- Walk the implementation on five axes: correctness (edge and error paths, races, state), readability (names, control flow, dead code, a conditional bolted onto an unrelated flow), architecture (does the refactor reduce concepts or relocate them; feature logic in a shared module; implicit type boundaries), security (input at boundaries, secrets, injection, untrusted external data), performance (unbounded loops, N+1, missing pagination).
- Size the change (SLICE-1): around a hundred changed lines reads in one sitting; a thousand splits. Refactoring and behaviour change are two changes.
- Write the findings (REVIEW-2), order them by payback, then the summary (REVIEW-3).
Security and dependencies
The security axis asks, for every new path: input validated at the boundary, authorisation checked, no secret in code or logs (SECRET-1, SECRET-2), no untrusted data reaching a shell, a query or a template, and what a dependency change brings in. A new dependency answers five questions before it lands (EVID-9): the licence, the maintenance record (last release, open issues), the size and what it pulls in, the vulnerability record, and whether the standard library or an existing dependency already covers it. Upgrades follow REVIEW-9.
Approval standard
Approve when the change improves the code and follows the repository's conventions, even if it is not how you would have written it. Block only for a defect, a security issue, a lost test, or a structure that makes the next change harder. "I'll clean it up later" is a decline: the cleanup lands before the merge or as its own tracked item with an owner.
Disagreements
Facts and data over preferences. The style guide decides style. Design is judged on engineering grounds. Consistency with the codebase is acceptable when it does not degrade it. Comment on the code, never the person (VOICE-11).
Reviewing your own work
Run REVIEW-7 on every plan, finding, draft and change before it is shown, unasked. Treat your own earlier output and any agent's output as a lead (EVID-3): open every cited line again. Re-read a file before asserting its state (EVID-8).
Companion skills
| Need | Skill |
|---|---|
| Calibration tags and the sha rule | evidence |
| The voice of threads and summaries | plain-voice |
| Tests that are worth the mutation check | test-first |
| Keeping the change small enough to review | thin-slices |
Sources
The approval standard and the disagreement hierarchy follow Google's engineering practices (CC BY 4.0). The mutation probe follows code-review-and-quality (addyosmani/agent-skills, MIT).