code-review-rules

Code Review Rules

Severity Classification

• BLOCKER: Architecture/security violation — blocks approval
• MAJOR: Error handling, logging, significant gaps — blocks approval
• MINOR: Code style, naming, documentation — does not block
• NIT: Stylistic preference — does not block

Decision Matrix

• APPROVED: 0 BLOCKER, 0 MAJOR (clean merge)
• APPROVED_WITH_COMMENTS: 0 BLOCKER, 0 MAJOR, has MINOR/NIT (merge with notes)
• CHANGES_REQUESTED: 1+ BLOCKER or 1+ MAJOR or 5+ MINOR same file (return to coder)
• Note: the per-file 5+ MINOR same file threshold mirrors the Auto-Escalation rule below — both files use the file as the natural unit for code review, giving a single decision point.

Auto-Escalation

• 5+ MINOR in same file → escalate to MAJOR (files are the natural unit for code review)
• Security issue (any severity) → always BLOCKER
• Layer-dependency violation (when {LAYER_RULE} SET AND {ARCHITECTURE_STYLE} == "layered") → always BLOCKER. SKIP entries (slot unset/non-layered) → consolidated NIT, NOT BLOCKER.

Spec Check Trust

Iteration 1: If coder handoff includes spec_check with status=PASS → trust spec compliance, skip plan compliance re-check during REVIEW. Focus REVIEW entirely on code quality (architecture, error handling, security, test coverage). Iteration 2+ (CHANGES_REQUESTED loop): Even when spec_check.status=PASS, perform a Parts-coverage spot-check by running git diff --name-only $BASE...HEAD and verifying each plan Part has at least one associated changed file. The verification is mechanical: read .claude/prompts/{feature}.md Parts list, map each Part name to one or more files mentioned in the Part body, then assert the changed-files set covers each Part. If any Part is found in parts_implemented but has zero matching changed files, raise a MINOR with category=completeness and a stable problem string "Iter ≥2 spot-check: Part \"{Part name}\" claimed implemented but no matching changed files in this iteration's diff. Possible silent regression.". The spot-check is targeted (not a full re-run) and adds 1-3 extra Bash invocations only on iter ≥ 2. spec_check.status=PARTIAL: note documented gaps as MINOR during REVIEW (unchanged). spec_check missing: backward compat — check plan coverage during REVIEW (unchanged).

Instructions

Step 1: Quick Check — lint + test (blocking)

If coder handoff includes verify_status with lint=PASS and test=PASS → trust coder verification, skip re-run. Otherwise: run {LINT_CMD} and {TEST_CMD} (resolved from PROJECT-KNOWLEDGE.md; CLAUDE.md fallback; kit-default Go: make lint, make test). If EITHER fails → STOP, return to coder. If both slots unset AND no CLAUDE.md fallback → SKIP QUICK CHECK, emit consolidated NIT. Do NOT proceed to review if Quick Check fails (whether trusted or re-run). Also check spec_check from coder handoff. If status=PASS → note compliance trusted. If PARTIAL → note gaps. If missing → plan to check coverage during REVIEW (backward compat).

Step 2: Get changes and assess scope

Run git diff $BASE...HEAD (detect base branch first — see code-reviewer.md process). Assess: files changed, lines changed, layers affected. If >100 lines or >5 files or 3+ layers → use Sequential Thinking.

Step 3: Review all concern areas

Check each area using grep search patterns from Examples:

• Architecture: layer-dependency compliance per {LAYER_RULE} slot (SKIP if unset or non-layered)
• Error handling: no log+return, proper wrapping
• Security: no hardcoded secrets, no token leaks (see Security Checklist)
• Test coverage: new code has tests

Step 4: Apply Decision Matrix and form verdict

Count issues by severity. Apply Decision Matrix and Auto-Escalation rules above. CRITICAL: NEVER approve with BLOCKER issues. Form handoff for completion.

Example

Log AND return — most common blocker

Principle: Choose one — return with wrap (domain/service) or log (handler), never both.

Reference shapes (syntax-correct examples per language):

• See ../planner-rules/code-shapes/<LANGUAGE>.md for language-correct error wrapping per {ERROR_WRAP} slot
• Kit Go example: return fmt.Errorf("context: %w", err)
• SKIP if {ERROR_WRAP} slot unset

Why: [BLOCKER] Log AND return creates duplicate logs in error chain — language-agnostic anti-pattern.

For more examples (incl. grep search patterns), see Examples.

Common Issues

Approved with blocker issues

Cause: Rushed review, missed severity classification. Fix: NEVER approve with blockers — RULE is absolute. Re-check all findings against Severity Classification above before verdict.

Log AND return pattern not caught

Cause: Trusted visual review instead of grep. Fix: ALWAYS run Grep 'log\.(Error|Warn|Info).*\n.*return' on changed files. Automated checks catch what eyes miss.

Sequential Thinking skipped on large diff

Cause: Changes seemed straightforward at first glance. Fix: ALWAYS use Sequential Thinking for 100+ lines, 5+ files, or 3+ layers. No exceptions.

For all troubleshooting cases, see Troubleshooting.

References (ON-DEMAND — do NOT read eagerly)

Do NOT read supporting files upfront. This SKILL.md contains all essential rules inline. Load files only when the specific trigger condition is met:

• Examples — Read when: running grep search patterns during REVIEW step 3. Contains bad/good code patterns and automated grep commands.
• Security Checklist — Read when: complexity M+ AND reviewing security area. SKIP entirely for S complexity.
• Checklist — Read when: self-verifying before outputting verdict (optional, only if uncertain about coverage).
• Troubleshooting — Read when: encountering an unexpected issue during review (e.g., git fails, unclear verdict). Do NOT read preemptively.