| name | code-review |
| description | Structured review of recent code changes across functionality, UX/UI, design, code quality, security, testing — produces a prioritized findings list. Activates when: the user asks to review implementation, review changes, review code, audit code, check for improvements, or when user mentions: review, audit, improvements, code quality, code review. |
| argument-hint | ["file path","feature name","or description of changes"] |
Code Review
A structured review of recent code changes, evaluating them across multiple quality dimensions and producing a prioritized list of actionable findings. The governing principle is proportionality: match every finding's weight to its real impact, and review for code health over time — net improvement, not perfection.
When to Use This Skill
Use this skill when:
- Reviewing a recently implemented feature or change
- Auditing code before committing or creating a PR
- The user asks "can you review this" or "are there improvements"
- Checking implementation quality after a subagent completes work
Do NOT use for:
- Reviewing code you haven't read yet (read first, then review)
- General codebase audits with no specific scope
- Reviewing third-party packages
Workflow
Phase 1: Identify the Scope
Determine what to review:
- If given specific files: read those files
- If given a feature description: find all related files (src/, tests/)
- If no scope specified: use
git diff or git diff --cached to find recently changed files
Read ALL files in scope before starting the review. Do not review code you haven't read.
Phase 2: Review Each Dimension
Evaluate the changes against each category below. Only report findings where there is a clear, concrete improvement — do not nitpick or pad the review.
Security
Look for:
- Input validation: Unvalidated user input reaching critical paths
- Type safety: Loose comparisons where strict is needed, missing type declarations
- Information disclosure: Sensitive data in error messages or logs
Functionality
Look for:
- Conformance & scope: does the change deliver what was actually asked? Fetch the real requirement — the spec's stated goals (
## Overview), its normative technical/design sections (e.g. ## Data Model, ## API Design, ## Proposed Changes), and every ## Edge Cases row; any linked issue acceptance criteria the spec hasn't superseded (where a spec and issue differ, the spec is the source of truth); and, when there's neither spec nor issue, the caller's own task description / explicit request — never the PR's paraphrase (a verdict against a paraphrase inherits its errors; only when there is genuinely no requirement source → note conformance wasn't reviewable rather than inventing one). Work requirement-down, not diff-up — the diff shows what was built, never what was forgotten: verdict each criterion Met (file:line, plus the test if one proves it) / Partial (the concrete gap) / Unmet / Untestable from the diff (say what would prove it). Then scope-check — implied requirements (an "edit X" ticket implies the edit persists and re-renders), unrequested extras (scope creep: flag as a decision, not a free bonus), silent interpretation of an ambiguous requirement (name the readings, flag a decision), and side-effect behaviour changes the ticket doesn't cover. A documented scope cut ("phase 2 covers X") is out-of-scope-noted, not Unmet. The Met/Partial/Unmet pass is how you review, not what you print — report only the actionable gaps (Partial/Unmet criteria and scope decisions to confirm), not a full conformance matrix, unless the caller asks for one.
- Logic errors: Conditions that don't match intent, off-by-one errors, wrong operator
- Missing edge cases: Null handling, empty collections, zero values, boundary conditions
- Stale data: Values computed once but displayed alongside live-updating data
- Duplicate work: Same query or computation running multiple times unnecessarily
- Broken flows: Actions that silently fail, missing error handling for expected failures
- Race conditions: Concurrent requests causing data corruption (missing locks, non-atomic operations)
- Cross-version compatibility: Does the code work across every runtime and framework version the project supports?
Code Quality & Maintenance
Look for:
- Project convention violations: Does the code follow established patterns? Check sibling files.
- Over-engineering: Unrequested abstractions, speculative generality, premature flexibility, or hand-rolled code a stdlib/native/framework feature or installed dependency replaces — could it be simpler or deleted without losing required behavior? Never cut validation, error handling, security, or accessibility to shrink code.
- DRY violations: Duplicated logic that should be extracted
- Dead code: Unused variables, unreachable branches, commented-out code
- Type safety: Missing return types, loose comparisons where strict is needed
- Naming: Do names clearly communicate intent?
- Separation of concerns: Business logic mixed with validation logic, etc.
UX, UI & Design
Only when the change touches a user-facing interface. Look for:
- Loading states: Do async actions show feedback while processing?
- Error states: What does the user see when something fails?
- Empty states: What shows when there's no data?
- Consistency: Do new elements match existing patterns (colors, spacing, button styles, icon sets)?
- Accessibility: Missing labels, no keyboard support, insufficient contrast
- Mobile/touch: Hover-dependent interactions that break on touch devices
- Visual glitches: Mismatched icon sizes, layout shifts on state change, inconsistent spacing
Testing
Look for:
- Missing happy path tests: Core functionality not tested
- Missing failure path tests: Error cases, invalid input, unauthorized access
- Missing edge case tests: Boundary values, empty data, null values
- Fragile assertions: Tests that pass for the wrong reason (e.g., an absence assertion matching unrelated text)
- Missing security tests: No tests verifying auth/authorization on actions
- Test isolation: Tests that depend on each other or on specific state
Phase 3: Compile Findings
Present findings in a structured format:
- Group by category (Security, Functionality, etc.)
- Number each finding for easy reference
- Include file + line number for each finding
- Explain the issue concisely — what's wrong and why it matters
- Skip categories with no findings — don't add filler
Phase 4: Prioritize
End with a summary table ranking findings by severity:
| Severity | Meaning |
|---|
| High | Security vulnerabilities, data corruption risks, broken functionality |
| Medium | Stale data, missing tests for important paths, UX issues that confuse users |
| Low | Minor inconsistencies, polish items, nice-to-have improvements |
Output Format
### Security
**1. [Short title]**
`file/path.php:42` — Description of the issue and why it matters.
### Functionality
**2. [Short title]**
`file/path.php:18` — Description...
### Testing
**3. [Short title]**
`tests/FooTest.php` — Description...
---
| # | Finding | Severity |
|---|---------|----------|
| 1 | Short title | High |
| 2 | Short title | Medium |
| 3 | Short title | Low |
Guidelines
- Be concrete: Every finding must point to a specific line or pattern in the code. No vague suggestions.
- Be actionable: Each finding should make clear what needs to change.
- Proportionality: Don't flag style preferences as security issues. Match severity to actual impact, and review for code health over time — approve net improvements rather than blocking on perfection.
- Respect existing conventions: If the codebase uses a pattern consistently, don't flag it as an issue even if you'd do it differently.
- Every finding earns its line: before writing a finding, ask whether it names a concrete, proportional improvement. If not, cut it. If a category has no findings, skip it entirely — a short review with real findings beats a long one with filler.
- Read before reviewing: Never review code you haven't read in this session. If you need more context, read the relevant files first.
