| name | code-review |
| description | Authoritative code review rubric for changed files. Covers 4 review layers, severity, confidence, merge recommendation, and concrete file:line findings. |
Code Review Skill
TL;DR
Systematic code review across 4 layers with severity classification. Scope restricted to changed files only. Report findings with ≥80% confidence, file:line references, and actionable suggestions. End with a merge recommendation.
Scope Rules
- Review only files changed in the diff, branch, or PR.
- Do not comment on untouched files unless required for direct impact analysis.
- No speculative issues — every finding must map to concrete code in changed files.
- Prefer correctness and architectural issues over style nits.
- Provide an actionable fix suggestion for every finding.
The 4 Review Layers
Apply in priority order:
Layer 1: Correctness & Safety
- Logic errors, off-by-one, incorrect conditionals
- Contract-driven edge cases: missing required inputs, explicitly optional or undefined values, error conditions, race conditions
- Error handling completeness — silent failure paths, swallowed catches, or fallback paths that hide failures or change behavior
- Incorrect error semantics (auth vs config vs runtime errors)
- Type safety and contract-accurate optionality
- Do not ask for optional chaining, null handling, fallback branches, or empty-input handling unless the type, schema, prompt, or existing behavior makes absence possible
- Behavioral changes — flag if a change alters existing behavior, intentionally or not
Layer 2: Security
- No hardcoded secrets or API keys
- Input validation and sanitization
- Injection vulnerability prevention (SQL, XSS, command)
- Authentication and authorization checks
- Sensitive data not logged
- OWASP Top 10 awareness
Layer 3: Performance
- N+1 query patterns, O(n²) on unbounded data
- Blocking I/O on hot paths
- Unnecessary re-renders (React/frontend)
- Memory leak prevention, missing cleanup
- Only flag if obviously problematic — don't invent hypotheticals
Layer 4: Style & Maintainability
- Adherence to project conventions (check AGENTS.md, .editorconfig)
- Code duplication (DRY violations)
- Excessive nesting (>3 levels) — suggest early returns or extraction
- Test coverage for changed behavior and key unhappy paths
- Assertions validate intent, not implementation accidents
Severity Classification
| Severity | Icon | Criteria | Blocks Merge? |
|---|
| Critical | 🔴 | Security vulnerabilities, crashes, data loss, corruption, production outage risk | Yes |
| Major | 🟠 | Bugs, reliability risk, missing error handling, significant architectural violation | Yes |
| Minor | 🟡 | Code smells, maintainability issues, moderate improvements, test gaps | No |
| Nit | 🟢 | Style preferences, naming suggestions, readability consistency | No |
Blocking vs Non-Blocking
- Blocking = Critical + Major → triggers "Needs changes" recommendation
- Non-blocking = Minor + Nit → can merge, fix later or optionally in this PR
Confidence Threshold
Only report findings with ≥80% confidence.
If uncertain about an issue:
- State the uncertainty: "Potential issue (70% confidence): ..."
- Suggest investigation rather than assert a problem
- Prefer false negatives over false positives (reduce noise)
- If you can't verify, say "I'm not sure about X" rather than flag it
Review Process
- Identify Scope — List all changed files from the diff
- Read Full Files — Diffs alone aren't enough. Read complete files to understand control flow, surrounding patterns, and existing error handling
- Check Conventions — Look for AGENTS.md, CONVENTIONS.md, .editorconfig for project-specific rules
- Deep Analysis — Apply all 4 layers systematically to each changed file
- Detect Behavioral Changes — Any change to observable behavior should be explicitly noted
- Philosophy Check — Verify against code-philosophy (5 Laws) if applicable
- Synthesize Findings — Group by severity, deduplicate, count blocking issues
- Merge Recommendation — Ready if 0 blocking; Needs changes if ≥1 blocking
Output Format
Per-Finding Format
#### [SEVERITY: critical|major|minor|nit] File: <path> Line: <line-or-range>
**Issue:** <clear problem statement>
**Suggestion:** <specific fix or approach>
Full Review Structure
**Files Reviewed:** [list all files]
**Overall Assessment:** APPROVE | REQUEST_CHANGES | NEEDS_DISCUSSION
**Summary:** [2-3 sentences — what the change does, overall quality]
### Findings
(findings listed with severity, each with file:line, issue, suggestion)
### 🟢 Positive Observations
[What's done well — always include at least one]
### Philosophy Compliance
- Early Exit: [PASS|FAIL|N/A]
- Parse Don't Validate: [PASS|FAIL|N/A]
- Atomic Predictability: [PASS|FAIL|N/A]
- Fail Fast: [PASS|FAIL|N/A]
- Intentional Naming: [PASS|FAIL|N/A]
### Review Summary
- Blocking: <n> (critical + major)
- Non-blocking: <n> (minor + nit)
- Recommendation: <Ready to merge | Needs changes>
Exclusions
- Do NOT propose broad refactors outside the changed scope
- Do NOT request cosmetic rewrites unless they hide real risk
- Do NOT ask for fallback paths in new required flows unless compatibility or an explicit contract requires them
- Do NOT ask for empty-input handling unless empty input is part of the stated, typed, or existing contract
- Do NOT request docs or README updates unless the user asked for docs or the change alters a documented public contract
- Do NOT include praise/fluff — keep comments direct and useful
- Do NOT be a zealot about style — some "violations" are acceptable when they're the simplest option
- Do NOT flag something as a bug if you're unsure — investigate first
Adherence Checklist
Before completing a review, verify:
