Code Review Rules

---
name: code-review-rules
description: Review standards for code-reviewer agent. Auto-loaded via agent frontmatter when code-reviewer runs (Phase 4). Covers: severity classification (BLOCKER/MAJOR/MINOR/NIT), decision matrix (APPROVED/APPROVED_WITH_COMMENTS/CHANGES_REQUESTED), auto-escalation rules, grep search patterns for automated checks.
disable-model-invocation: true
---

# 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). This preserves the S/M happy-path performance — first-pass coverage is high-trust.
**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](examples.md):
- 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](security-checklist.md))
- 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](examples.md).

## 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](troubleshooting.md).

## 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](examples.md) — **Read when:** running grep search patterns during REVIEW step 3. Contains bad/good code patterns and automated grep commands.
- [Security Checklist](security-checklist.md) — **Read when:** complexity M+ AND reviewing security area. SKIP entirely for S complexity.
- [Checklist](checklist.md) — **Read when:** self-verifying before outputting verdict (optional, only if uncertain about coverage).
- [Troubleshooting](troubleshooting.md) — **Read when:** encountering an unexpected issue during review (e.g., git fails, unclear verdict). Do NOT read preemptively.