Code Review Methodology

Contract

Iron law: verify it works before verifying it is good; never review quality on code that misses the acceptance criteria. Required by /flow:review, /flow:pr, code-reviewer, security-reviewer. Returns a P1/P2/P3 finding set (references/finding-schema.md), a requirements map, and APPROVE, COMMENT or REQUEST_CHANGES. Skips: Stage 2 when Stage 1 finds >3 unmet criteria; on the 3rd+ cycle, only new P1s.

Two-stage review

Stage 1, spec compliance. Give each acceptance criterion one status: Met (implemented and testable), Interpreted (ambiguous; one reading implemented), Partially Met, Not Addressed. If Stage 1 fails, stop.

Stage 2, code quality, in priority order: security, correctness, performance, maintainability. Never flag maintainability while security or correctness findings stand.

6-facet review

Stage 1 on the main thread; facets fan out in parallel:

Tests facet rule: derive the expected behavior from the spec BEFORE reading the tests, then check each expected value and input against it; an expectation copied from the implementation confirms nothing (references/test-review-checklist.md).

Synthesis

1. Deduplicate by file:line, keeping the highest priority

• Optionally, a System One provider is asked whether two remaining findings in one file, whose reviewer lists differ, describe the same defect (review.dedup, off by default; references/system-one.md). In on mode a confident yes merges them under the finding with the highest priority, listing every location and reviewer. A security finding, a LOW finding paired with a HIGH or MEDIUM one, and two findings with the same reviewer list are never merged.

1. Order P1, P2, P3 by file
2. Count per priority; counts must match the Finding | Suggested Fix rows (references/finding-schema.md)

Confidence and signal

HIGH (ran code, a test or LSP) and MEDIUM (read the code path) decide at their priority. LOW (pattern match, or a P1/P2 finding a confident System One answer says its cited code does not show: review.confidence, off by default, references/system-one.md; on someone else's pull request such a P1 stays counted, with the answer as a note), any priority, goes to Needs investigation, outside the decision and the FLOW_REVIEW_CYCLE marker; own-PR handling: commands/review.md Phase 4 step 5. Absent or invalid confidence is MEDIUM. bin/flow-finding-route.sh applies it. Style is P3 at most; a finding with no file:line and no harm scenario is noise.

Boy Scout recognition

APPROVE improve: commits passing the proximity test (already-modified file, self-evidently correct, <10 lines, no API change); else P2 "scope creep".

Review cycle awareness

The cycle number counts prior FLOW_REVIEW_CYCLE markers. Review the delta only, verify each claimed resolution against git diff, and on the 3rd+ cycle raise only new P1s (references/review-cycle-parsing.md).

Stop conditions

• >3 unmet criteria in Stage 1: REQUEST_CHANGES, skip Stage 2
• Files unrelated to the issue: out-of-context, ask for a split (improve: commits in modified files stay in context)
• Diff >500 lines, no test changes: P1 "untested large change"

Review decision

Finding triage never triggers escalation (skills/llm-operator-principles/SKILL.md).

Grounding pass (off by default)

review.groundingCritic: on inserts one step into a Path B fan-out, between synthesis and display: Agent(finding-critic) answers each P1/P2 finding in a three-verdict grammar, and on a disagreement the reviewer that raised it cites code or drops it. Path A is unchanged and keeps its own challenge round; the grammar and both placements are in references/paired-review-protocol.md. Default off until references/review-precision-eval.md.

Adversarial protocol

With agent teams enabled, apply skills/team-coordination/SKILL.md: independent reviewers, mutual challenge, disputed findings escalate.