references/smells.md
# Code Smell Baseline
A fixed set of Fowler code smells (_Refactoring_, ch. 3) that applies even when a repo documents nothing. Two rules bind it:
- **The repo overrides.** A documented repo standard always wins; where it endorses something the baseline would flag, suppress the smell.
- **Always a judgement call.** Each smell is a labelled heuristic ("possible Feature Envy"), never a hard violation. Skip anything tooling already enforces.
Each smell reads _what it is_ → _how to fix_; match it against the diff:
- **Mysterious Name**: a function, variable, or type whose name doesn't reveal what it does or holds. → rename it; if no honest name comes, the design's murky.
- **Duplicated Code**: the same logic shape appears in more than one hunk or file in the change. → extract the shared shape, call it from both.
- **Feature Envy**: a method that reaches into another object's data more than its own. → move the method onto the data it envies.
- **Data Clumps**: the same few fields or params keep travelling together (a type wanting to be born). → bundle them into one type, pass that.
- **Primitive Obsession**: a primitive or string standing in for a domain concept that deserves its own type. → give the concept its own small type.
- **Repeated Switches**: the same `switch`/`if`-cascade on the same type recurs across the change. → replace with polymorphism, or one map both sites share.
- **Shotgun Surgery**: one logical change forces scattered edits across many files in the diff. → gather what changes together into one module.
- **Divergent Change**: one file or module is edited for several unrelated reasons. → split so each module changes for one reason.
- **Speculative Generality**: abstraction, parameters, or hooks added for needs the spec doesn't have. → delete it; inline back until a real need shows.
- **Message Chains**: long `a.b().c().d()` navigation the caller shouldn't depend on. → hide the walk behind one method on the first object.
- **Middle Man**: a class or function that mostly just delegates onward. → cut it, call the real target direct.
- **Refused Bequest**: a subclass or implementer that ignores or overrides most of what it inherits. → drop the inheritance, use composition.
SKILL.md
---
name: code-review
effort: high
description: Review a GitHub Pull Request for bugs, security, performance, and code quality. Use when user asks to review a PR or wants pull request feedback. Don't use for reviewing local uncommitted changes, creating new PRs, or merging branches.
---
# Review Pull Request
Mode: $ARGUMENTS
If mode is one of the following, adjust the review:
- BUGS: Focus only on logical or other bugs
- SECURITY: Focus only on security issues
- PERFORMANCE: Focus only on performance issues
## Approval standard
Approve when the change definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — don't block a change because it isn't how you would have written it. If it improves the codebase and follows its conventions, approve.
If the change is too large to review well (~1000+ lines), asking the author to split it is a valid review outcome. Suggest a strategy: stack (small change, next one based on it), by file group, horizontal (shared code first, then consumers), or vertical (one end-to-end slice per PR).
## Workflow
1. Analyze the diff and pre-loaded PR context
2. Read changed files to understand full context
3. Review based on mode (or all categories if no mode set)
4. Provide structured feedback
## Review criteria
Apply all axes (or narrow to the mode above):
- **Correctness**: Logic bugs, off-by-ones, race conditions, unhandled states, missing error paths
- **Readability**: Functions <50 lines, nesting <3 levels, no dead code/unused imports
- **Security**: No exposed secrets, no `any`, no unvalidated external data
- **Immutability**: No push/pop/splice/direct mutation
- **Patterns**: Consistent with codebase conventions, no reinvented wheels
- **Performance**: Unnecessary re-renders, O(n²) where O(n) works, missing memoization
- **Code smells**: match the diff against the baseline in [smells.md](references/smells.md) — always judgement calls, and the repo's documented style overrides the baseline
## Output format
Group by severity:
- **Critical** - must fix before merge (bugs, security vulnerabilities)
- **Suggestions** - improvements worth considering
- **Nit** - minor and optional; the author may ignore (formatting, naming taste)
- **FYI** - informational only, no action needed
- **Positives** - good patterns to call out
If you have one structural problem and ten nits, the structural problem _is_ the review — lead with it.
Use `file:line` references for all findings. Include suggested fix for each critical issue.
## Rules
- Review ALL changed files, not just the latest commit
- Be specific — label true nitpicks as **Nit** rather than dropping or inflating them
- **Dependency upgrades** (when `package.json`/lockfile is in the diff): one dependency per change — a bulk bump that breaks hides which package did it; verify against the changelog, not the version number; review the lockfile diff (one direct bump pulls dozens of transitive changes); flag hand-edited lockfiles
## Common Rationalizations
| Excuse | Rebuttal |
| ------------------------- | -------------------------------------------------------------- |
| "Too small to review" | Small changes cause big bugs — review everything |
| "It's just a refactor" | Refactors break behavior silently — verify contracts preserved |
| "Tests pass so it's fine" | Tests don't catch readability, security, or design issues |
| "I'll clean it up later" | Later never comes — fix now or it ships as-is |
## Verification
- [ ] Every changed file reviewed (not just the diff summary)
- [ ] No critical issue left without a suggested fix
- [ ] Security concerns flagged with specific fix
- [ ] Feedback grouped by severity, not file order
- [ ] Verdict stated against the approval standard — improves code health, or blocked with a reason
## Error Handling
- If `gh pr view` fails → run `gh auth status` to verify authentication; ask user for PR number if not on a PR branch
- If a changed file is deleted in the PR → skip reading it; note it was removed
- If diff is too large → prioritize changed files with highest risk (auth, payments, data mutation)