| name | code-reviewer |
| description | Use when reviewing implementation changes against an approved plan or task (especially before merging or between Hive tasks) to catch missing requirements, YAGNI, dead code, and risky patterns |
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
- List all files changed.
- For each file, state why it changed (what requirement it serves).
- 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
- [highest priority change]
- [next]
- [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
