# Review Cycle Principles Tool-agnostic principles for responding to code review findings on a PR. Adapters (Greptile, CodeRabbit, Codacy, host babysit loops, …) implement these with tool-specific mechanics. This file is the single source of truth for the universal process so consumers without a given adapter skill still get the review discipline (#1471 / #212). Legend (from RFC2119): !=MUST, ~=SHOULD, ≉=SHOULD NOT, ⊗=MUST NOT, ?=MAY. **See also:** [coding.md](coding.md) (quality chain) · [testing.md](testing.md) · [skills/deft-directive-review-cycle/SKILL.md](../skills/deft-directive-review-cycle/SKILL.md) (Greptile + GitHub adapter) ## Universal Requirements - ! ALL review findings MUST be read before any fixes begin - ! Findings MUST be classified by severity: **P0** (critical/blocking), **P1** (real defect), **P2** (style / non-blocking). P0 and P1 are merge-blocking; P2 is not - ! Findings MUST be fixed in a single batch commit — never incrementally per finding - ! Changed values, terms, or fields MUST be grepped across all PR files for cross-file consistency in the same batch - ~ Structured data files (JSON / YAML / TOML) SHOULD be validated locally before commit (e.g. `python3 -m json.tool`, YAML lint) — do not rely on the reviewer alone to catch syntax errors - ! Do not push additional commits while a review is in progress on the current head - ! Exit condition: no P0 or P1 remaining = ready to merge; P2 does not block merge - ! Post-merge: verify that closing keywords (`Closes #N`, `Fixes #N`) actually closed the referenced issues (squash-merge pitfall; #167) ## Severity and merge gate | Severity | Meaning | Blocks merge? | | --- | --- | --- | | P0 | Critical / correctness / security / data-loss | Yes | | P1 | Real defect or incomplete acceptance | Yes | | P2 | Style, nits, non-blocking suggestion | No | - ! Agents MUST NOT claim merge-ready while any P0 or P1 from the current review remains open - ⊗ Elevate P2-only findings into a merge block without operator agreement ## Policy-anchored classification (#3452) - ! Invariant-shaped findings (concurrency, error handling, containment/security) MUST NOT be classified out-of-model until a written policy (assumptions / guarantees / non-goals) exists on the **current HEAD** of the file under review. Absent -> write the anchor first. Anchor-wrong -> revise the anchor, then classify - ! Classify then act: in-model -> patch; out-of-model -> accepted-risk reply citing the HEAD anchor. Deterministic arity/wiring claims MUST check the head blob before confirmation - ! One consolidated push per review round; local review pass before push; never push per finding. Riders allowed on mechanical rebases - ! More than 3 review rounds on the same file: escalate to a design pass, not round K+1 and not parking. Compose with the adapter same-fingerprint stop; do not invent a second detector ## Anti-Patterns - ⊗ Classify invariant-shaped findings out-of-model with no HEAD policy (#3452) - ⊗ Start fixing individual findings as you encounter them — read and plan the full batch first - ⊗ Push one commit per finding - ⊗ Push while a bot or human review of the current head is still in flight - ⊗ Treat P2-only findings as merge-blocking by default - ⊗ Assume squash merge auto-closed referenced issues — always verify issue state after merge (#167) - ⊗ Skip cross-file grep when a fix renames or retargets a shared term/value/field