code-reviewer

# Code Reviewer ## Overview This skill teaches a reviewer to evaluate implementation changes for: - Adherence to the approved plan/task (did we build what we said?) - Correctness (does it work, including edge cases?) - Simplicity (YAGNI, dead code, over-abstraction) - Risk (security, performance, maintainability) **Core principle:** The best change is the smallest correct change that satisfies the plan. ## Iron Laws - Review against the task/plan first. Code quality comes second. - Bias toward deletion and simplification. Every extra line is a liability. - Prefer changes that leverage existing patterns and dependencies. - Be specific: cite file paths and (when available) line numbers. - Do not invent requirements. If the plan/task is ambiguous, mark it and request clarification. ## What Inputs You Need Minimum: - The task intent (1-3 sentences) - The plan/task requirements (or a link/path to plan section) - The code changes (diff or list of changed files) If available (recommended): - Acceptance criteria / verification steps from the plan - Test output or proof the change was verified - Any relevant context files (design decisions, constraints) ## Review Process (In Order) ### 1) Identify Scope 1. List all files changed. 2. For each file, state why it changed (what requirement it serves). 3. Flag any changes that do not map to the task/plan. **Rule:** If you cannot map a change to a requirement, treat it as suspicious until justified. ### 2) Plan/Task Adherence (Non-Negotiable) Create a simple checklist: - What the task says must happen - Evidence in code/tests that it happens Flag as issues: - Missing requirements (implemented behavior does not match intent) - Partial implementation with no follow-up task (TODO-driven shipping) - Behavior changes that are not in the plan/task ### 3) Correctness Layer Review for: - Edge cases and error paths - Incorrect assumptions about inputs/types - Inconsistent behavior across platforms/environments - Broken invariants (e.g., state can become invalid) Prefer "fail fast, fail loud": invalid states should become clear errors, not silent fallbacks. ### 4) Simplicity / YAGNI Layer Be ruthless and concrete: - Remove dead branches, unused flags/options, unreachable code - Remove speculative TODOs and "reserved for future" scaffolding - Remove comments that restate the code or narrate obvious steps - Inline one-off abstractions (helpers/classes/interfaces used once) - Replace cleverness with obvious code - Reduce nesting with guard clauses / early returns Prefer clarity over brevity: - Avoid nested ternary operators; use `if/else` or `switch` when branches matter - Avoid dense one-liners that hide intent or make debugging harder ### 4b) De-Slop Pass (AI Artifacts / Style Drift) Scan the diff (not just the final code) for AI-generated slop introduced in this branch: - Extra comments that a human would not add, or that do not match the file's tone - Defensive checks or try/catch blocks that are abnormal for that area of the codebase - Especially swallowed errors ("ignore and continue") and silent fallbacks - Especially redundant validation in trusted internal codepaths - TypeScript escape hatches used to dodge type errors (`as any`, `as unknown as X`) without necessity - Style drift: naming, error handling patterns, logging style, and structure inconsistent with nearby code Default stance: - Prefer deletion over justification. - If validation is needed, do it at boundaries; keep internals trusting parsed inputs. - If a cast is truly unavoidable, localize it and keep the justification to a single short note. When recommending simplifications, do not accidentally change behavior. If the current behavior is unclear, request clarification or ask for a test that pins it down. **Default stance:** Do not add extensibility points without an explicit current requirement. ### 5) Risk Layer (Security / Performance / Maintainability) Only report what you are confident about. Security checks (examples): - No secrets in code/logs - No injection vectors (shell/SQL/HTML) introduced - Authz/authn checks preserved - Sensitive data not leaked Performance checks (examples): - Avoid unnecessary repeated work (N+1 queries, repeated parsing, repeated filesystem hits) - Avoid obvious hot-path allocations or large sync operations Maintainability checks: - Clear naming and intent - Consistent error handling - API boundaries not blurred - Consistent with local file patterns (imports, export style, function style) ### 6) Make One Primary Recommendation Provide one clear path to reach approval. Mention alternatives only when they have materially different trade-offs. ### 7) Signal the Investment Tag the required follow-up effort using: - Quick (<1h) - Short (1-4h) - Medium (1-2d) - Large (3d+) ## Confidence Filter Only report findings you believe are >=80% likely to be correct. If you are unsure, explicitly label it as "Uncertain" and explain what evidence would confirm it. ## Output Format (Use This Exactly) --- **Files Reviewed:** [list] **Plan/Task Reference:** [task name + link/path to plan section if known] **Overall Assessment:** [APPROVE | REQUEST_CHANGES | NEEDS_DISCUSSION] **Bottom Line:** 2-3 sentences describing whether it matches the task/plan and what must change. ### Critical Issues - None | [file:line] - [issue] (why it blocks approval) + (recommended fix) ### Major Issues - None | [file:line] - [issue] + (recommended fix) ### Minor Issues - None | [file:line] - [issue] + (suggested fix) ### YAGNI / Dead Code - None | [file:line] - [what to remove/simplify] + (why it is unnecessary) ### Positive Observations - [at least one concrete good thing] ### Action Plan 1. [highest priority change] 2. [next] 3. [next] ### Effort Estimate [Quick | Short | Medium | Large] --- ## Common Review Smells (Fast Scan) Task/plan adherence: - Adds features not mentioned in the plan/task - Leaves TODOs as the mechanism for correctness - Introduces new configuration modes/flags "for future" YAGNI / dead code: - Options/config that are parsed but not used - Branches that do the same thing on both sides - Comments like "reserved for future" or "we might need this" AI slop / inconsistency: - Commentary that restates code, narrates obvious steps, or adds process noise - try/catch that swallows errors or returns defaults without a requirement - `as any` used to silence type errors instead of fixing types - New helpers/abstractions with a single call site Correctness: - Silent fallbacks to defaults on error when the task expects a hard failure - Unhandled error paths, missing cleanup, missing returns Maintainability: - Abstractions used once - Unclear naming, "utility" grab-bags ## When to Escalate Use NEEDS_DISCUSSION (instead of REQUEST_CHANGES) when: - The plan/task is ambiguous and multiple implementations could be correct - The change implies a product/architecture decision not documented - Fixing issues requires changing scope, dependencies, or public API