Code review methodology

Code Review Methodology

Domain skill for structured, multi-faceted code review.

Iron Law

FIRST VERIFY IT WORKS, THEN VERIFY IT'S GOOD. Never review code quality on code that doesn't function correctly.

Spec compliance is Stage 1. Code quality is Stage 2. Reviewing style on broken logic is wasted effort.

Two-Stage Review

Stage 1 — Spec Compliance: Does the code do what the issue/acceptance criteria require? Map each criterion to implementation evidence. If Stage 1 fails, stop — no point reviewing quality on code that doesn't meet requirements.

Stage 2 — Code Quality (in priority order):

1. Security — vulnerabilities, auth bypass, injection, secrets
2. Correctness — logic errors, race conditions, edge cases
3. Performance — O(n^2) in hot paths, unnecessary allocations, N+1 queries
4. Maintainability — readability, naming, structure (lowest priority)

Do NOT flag maintainability issues if security or correctness issues exist. Fix the important things first.

6-Facet Review

Every review evaluates these facets (parallelizable):

Facet	Focus	Agent / Skill
Security	OWASP top 10, secrets, auth/authz, input validation	security-reviewer
Quality	Logic correctness, edge cases	code-reviewer
Conventions	Commit format, branch naming, PR structure, patterns	convention-checker
Tests	Coverage, quality commands pass, test quality	test-runner
Error handling	Unhandled errors, silent failures, missing edge cases in error paths	error-handler-inspector
Claim verification	Self-review claims cross-referenced against actual file state	holdout-validation (skill)

Requirements compliance is Stage 1 (Spec Compliance) of the Two-Stage Review section above, not a parallel facet — it runs first on the main thread before the 6 facets fan out.

For the Tests facet specifically, see test-review-checklist.md for a runnable checklist of coverage, quality, and integration-test signals reviewers can flag with citations.

Finding Synthesis

After all facets complete, synthesize:

1. Deduplicate by file:line — same location = same finding, keep highest priority
2. Prioritize P1 → P2 → P3
3. Group by file for readability
4. Count findings per priority level

Requirements Compliance

Map each acceptance criterion to evidence:

Status	Meaning
Met	Directly implemented and testable
Interpreted	Criterion was ambiguous, implementation reflects interpretation
Partially Met	Some aspects done, others pending
Not Addressed	Not implemented in this change

Finding Format

### P1 - Critical
| Finding | Suggested Fix |
|---------|---------------|
| **1 · security · `auth.rb:42`**<br>SQL injection via string interpolation. | Use parameterized query. |

### P2 - Should Fix
| Finding | Suggested Fix |
|---------|---------------|

### P3 - Consider
| Finding | Suggested Fix |
|---------|---------------|

Confidence Assessment

For each finding, assess confidence:

• High: Verified by reading code + running test
• Medium: Verified by reading code
• Low: Pattern match only — needs investigation

Only P1 findings with High confidence should block merge.

Signal Quality Rules

Signal Type	Confidence	Include In Review?
Verified by running code/test	High	Always
LSP diagnostic (error/warning from language server)	High	Always — language server has full project context
LSP find-references (verified all callers handled)	High	Always for P1/P2 — semantic, not text-based
Verified by reading code path	Medium	Always for P1/P2
Pattern-match only (looks like a bug)	Low	Only if P1, flag as "needs investigation"
Style preference	N/A	Only as P3, never blocks merge

Noise filter: If a finding cannot be explained with a file:line citation and a concrete scenario where it causes harm, it is noise. Drop it.

Boy Scout Recognition

When reviewing, recognize improve: commits as legitimate Boy Scout cleanup:

• APPROVE improve: commits that pass the proximity test (file already modified, self-evidently correct, <10 lines, no API change, no explanation needed)
• Flag as P2 "scope creep" only if the cleanup fails the proximity test (untouched files, architecture changes, new tests required, subjective style)

Review Cycle Awareness

Check review history to understand cycle count:

• Count CHANGES_REQUESTED reviews to determine cycle number
• Focus on delta since last review — findings on unchanged code from prior cycles are noise
• On 3rd+ cycle: only flag NEW P1 findings, note persistent P2s, suggest synchronous discussion for unresolved items
• Note convergence signal if findings are shrinking each cycle — this is healthy progress

Structured Cycle Parsing

Parse FLOW_REVIEW_CYCLE and FLOW_RESOLUTION_CYCLE markers from prior PR comments to build cycle context:

REPO=$(gh repo view --json nameWithOwner --jq '.nameWithOwner')

# Parse prior review findings (from review bodies)
gh api repos/$REPO/pulls/$PR_NUM/reviews --jq '
  [.[] | select(.body | test("FLOW_REVIEW_CYCLE")) | {
    cycle: (.body | capture("FLOW_REVIEW_CYCLE:(?<n>[0-9]+)") | .n),
    findings: (.body | capture("FINDINGS:\\[(?<f>[^\\]]+)\\]") | .f)
  }]'

# Parse prior resolution outcomes (from issue comments posted via gh pr comment)
gh api repos/$REPO/issues/$PR_NUM/comments --jq '
  [.[] | select(.body | test("FLOW_RESOLUTION_CYCLE")) | {
    cycle: (.body | capture("FLOW_RESOLUTION_CYCLE:(?<n>[0-9]+)") | .n),
    resolved: (.body | capture("RESOLVED:\\[(?<r>[^\\]]*?)\\]") | .r),
    escalated: (.body | capture("ESCALATED:\\[(?<e>[^\\]]*?)\\]") | .e)
  }]'

Cross-reference for each prior finding:

1. Was it marked as resolved in a resolution comment?
2. Has the code at that location changed in git diff?
3. Build a Previous Feedback Status table:

### Previous Feedback Status
| Cycle | Finding | Priority | Claimed Status | Verified |
|-------|---------|----------|----------------|----------|

If a finding was claimed resolved but the code at that location is unchanged, flag it as "Not verified — code unchanged".

Review Stop Conditions

• Stage 1 finds >3 unmet acceptance criteria — REQUEST_CHANGES immediately, skip Stage 2
• PR modifies files unrelated to the issue — flag as out-of-context, ask for split (but improve: commits in already-modified files are NOT out-of-context)
• Diff is >500 lines with no test changes — flag as P1 "untested large change"

Review Decision

Findings	Decision
P1 findings (any)	REQUEST_CHANGES
P2 findings (any)	REQUEST_CHANGES
P3 findings only	COMMENT (fix-expected — author must fix in-PR; P3 is not a free pass)
No findings	APPROVE

Note: P3 → COMMENT is NOT "approve with nits." The PR author is expected to fix every P3 in-PR. Finding triage is NEVER a valid escalation trigger (see skills/llm-operator-principles/SKILL.md and references/escalation-format.md) — reviewers should not approve PRs with unaddressed P3s, and authors should not file escalations to ask whether to fix them.

Adversarial Protocol (Agent Teams)

When agent teams are enabled, use adversarial synthesis from team-coordination skill (skills/team-coordination/SKILL.md). Reviewers work independently, share findings, challenge each other, and disputed findings escalate to human.

Rationalization Prevention

Excuse	Response
"It looks correct to me"	Looking is not verifying. Trace the data flow.
"This is just a style issue"	Then it's P3 at most. Don't flag it as P2.
"I don't have time for all 6 facets"	Then prioritize: Security > Correctness > the rest. Never skip security.
"The tests pass so the logic is fine"	Tests prove what's tested. Review proves what's not.
"This is too small to review thoroughly"	Small changes, same process. Small bugs cause big outages.