---
name: Code Review
description: Use this skill when reviewing code for quality, security, bugs, or best practice violations. Invoke after implementing features or before creating PRs.
version: 1.0.0
---

# Code Review Skill

> Based on Claude Code's official code-review plugin

---

## When To Use This Skill

Use this skill when:
- You've written code and need quality review
- You want to check for bugs, security issues, or best practice violations
- Before creating a PR or merging code
- After completing a feature implementation

---

## Review Focus Areas

### 1. Code Quality
- **Readability**: Is the code easy to understand?
- **Maintainability**: Will this be easy to modify later?
- **Complexity**: Can this be simplified?
- **DRY**: Is there code duplication?

### 2. Security (OWASP Top 10)
- **Injection**: SQL, XSS, command injection
- **Authentication**: Proper session handling, password storage
- **Authorization**: Role-based access control
- **Input Validation**: All user input validated

### 3. Correctness
- **Logic Errors**: Edge cases, off-by-one errors
- **Error Handling**: Try/catch, graceful failures
- **Null/Undefined**: Proper null checks
- **Type Safety**: TypeScript strict compliance

### 4. Project Conventions
- **CLAUDE.md Compliance**: Following project standards
- **Naming Conventions**: Consistent naming
- **File Organization**: Correct file placement
- **Import Structure**: Organized imports

---

## Review Output Format

```markdown
## Code Review Results

### 🔴 Critical Issues (must fix before merge)
1. **[File:Line]** Issue description
   - Why it's critical
   - Suggested fix

### 🟡 Major Issues (should fix)
1. **[File:Line]** Issue description
   - Impact
   - Suggested fix

### 🟢 Minor Issues (nice to fix)
1. **[File:Line]** Issue description
   - Suggested improvement

### ✅ Positive Observations
- What was done well
- Good patterns followed

### 📊 Overall Assessment
[Summary of code quality]
```

---

## Confidence-Based Filtering

Only report issues with high confidence (≥80%):
- **75-100**: Critical issues (definitely report)
- **50-74**: Important issues (report with caveat)
- **Below 50**: Skip (too uncertain)

---

## Review Checklist

### Before Review
- [ ] Identify all files changed
- [ ] Read project's CLAUDE.md
- [ ] Understand the feature being implemented

### During Review
- [ ] Check for security vulnerabilities
- [ ] Look for logic errors
- [ ] Verify error handling
- [ ] Check code style consistency
- [ ] Ensure tests exist for new code

### After Review
- [ ] Summarize findings by severity
- [ ] Provide specific file:line references
- [ ] Suggest concrete fixes
- [ ] Note what was done well

---

## Example Review Session

**Trigger:**
```
Review my recent changes for the authentication feature
```

**Expected Output:**
```markdown
## Code Review: Authentication Feature

### 🔴 Critical Issues

1. **src/auth/login.ts:45** - SQL Injection vulnerability
   - User input directly concatenated in query
   - Fix: Use parameterized queries
   
2. **src/auth/session.ts:23** - Session token stored in localStorage
   - XSS vulnerable
   - Fix: Use httpOnly cookies

### 🟡 Major Issues

1. **src/auth/login.ts:67** - Missing rate limiting
   - Brute force attacks possible
   - Fix: Add rate limiter middleware

### 🟢 Minor Issues

1. **src/auth/utils.ts:12** - Could extract validation to helper
   - Duplicated across 3 files

### ✅ Positive Observations
- Good use of TypeScript strict mode
- Proper password hashing with bcrypt
- Clean separation of concerns

### 📊 Overall Assessment
Authentication flow is well-structured. Fix critical security issues before deployment.
```

---

## Integration with Workflow

This skill is typically invoked:
1. After implementing a feature (Phase 6 in feature-dev workflow)
2. Before creating a PR
3. On-demand for code quality checks

---

## Security Review Checklist

Before approving any code review:
- [ ] Input validation: All user inputs sanitized and validated
- [ ] Authentication: Auth checks on all protected routes
- [ ] Authorization: RLS/RBAC properly configured
- [ ] SQL injection: Parameterized queries used (no string concatenation)
- [ ] XSS: Output encoding applied
- [ ] CSRF: Tokens on state-changing requests
- [ ] Secrets: No hardcoded keys, tokens, or passwords
- [ ] Dependencies: No known vulnerable packages
- [ ] Error handling: No sensitive data in error messages
- [ ] Logging: Sensitive data excluded from logs

---

## Rationalizations to Reject

| Rationalization | Why It's Wrong | Required Action |
|----------------|---------------|----------------|
| "It's just a small change" | Small changes cause big bugs | Review with same rigor as large changes |
| "Tests pass so it's fine" | Tests don't cover all edge cases | Check logic independently of test results |
| "We'll fix it later" | Technical debt compounds | Fix now or create a tracked issue |
| "It works on my machine" | Environment differences cause prod issues | Test in CI/staging environment |
| "The framework handles that" | Frameworks have defaults that may not be secure | Verify framework security configuration |

---

## Two-Stage Review Protocol

> Adapted from obra/superpowers subagent-driven review discipline.

For non-trivial changes, split the review into two independent stages:

### Stage 1: Spec Compliance Review

**Question: Does this code do what it's supposed to do?**

```
CHECKLIST:
- [ ] All requirements from the spec/ticket are addressed
- [ ] Edge cases from the spec are handled
- [ ] No requirements are missing or partially implemented
- [ ] Error states match spec expectations
- [ ] The feature works as the USER expects (not just as the developer intended)
```

### Stage 2: Code Quality Review

**Question: Is this code well-written and maintainable?**

```
CHECKLIST:
- [ ] Code is readable without comments explaining "what" (comments explain "why")
- [ ] No unnecessary complexity
- [ ] Proper error handling (not just happy path)
- [ ] No security vulnerabilities
- [ ] Performance is acceptable
- [ ] Tests exist and are meaningful
- [ ] No dead code or debugging artifacts
```

**Why two stages?** Combining spec review and code quality review in one pass causes reviewers to either:
- Focus on code quality and miss missing requirements, OR
- Focus on requirements and miss code quality issues

Separate passes catch more issues.

---

## Anti-Sycophancy Protocol

> Adapted from obra/superpowers anti-sycophancy discipline.

When reviewing code or receiving feedback:

### Banned Phrases in Code Review

| Banned | Replace With |
|--------|-------------|
| "Great point!" | State the fix directly |
| "That's a good idea" | "Changing X to Y because [reason]" |
| "Thanks for catching that" | "Fixed: [description of fix]" |
| "You're right" | "Confirmed: [fact]. Updating [code]." |
| "I appreciate the feedback" | [Just fix it and move on] |

### Review Response Protocol

When you receive code review feedback:

```
1. READ the feedback
2. UNDERSTAND what needs to change
3. MAKE the change
4. STATE what you changed and why
5. DO NOT add social pleasantries
```

**Example:**

```
BAD:  "Great catch! You're absolutely right that this could cause a race 
       condition. I appreciate you pointing that out! I've updated the code 
       to use a mutex."

GOOD: "Fixed: Added mutex to prevent race condition in handlePayment(). 
       The concurrent access was possible when two checkout requests hit 
       the same cart simultaneously."
```

### Why This Matters

Sycophantic responses:
- Waste tokens and reading time
- Signal social compliance, not technical competence
- Can mask disagreement (saying "great point" when you think the feedback is wrong)
- Erode trust in the reviewer-developer relationship

**If you disagree with feedback, SAY SO with technical reasoning.** A respectful disagreement is more valuable than a sycophantic agreement.

---

*Based on Claude Code's official code-review plugin*