Code Review

<essential_principles>

Core Philosophy

Code review should be: 1. Convention-aware - Follows project-specific rules from CLAUDE.md 2. Actionable - Every comment includes a specific fix 3. Prioritized - Critical issues first, style last 4. Contextual - Understands the change's purpose

Review Categories (Priority Order)

1. 🔴 Critical - Security vulnerabilities, data loss risks, crashes 2. 🟠 Bugs - Logic errors, edge cases, race conditions 3. 🟡 Performance - N+1 queries, memory leaks, inefficient algorithms 4. 🔵 Maintainability - Code clarity, naming, documentation 5. ⚪ Style - Formatting, conventions, nitpicks

Key Rules

• NEVER approve code you haven't read
• ALWAYS read CLAUDE.md first if it exists
• Spawn fresh context for large reviews to avoid bias
• Be specific - line numbers, code snippets, fixes
• Stay focused - review what changed, not the whole file

</essential_principles>

<intake>

Code Review

What would you like to review?

1. Current diff - Review uncommitted changes (git diff) 2. Staged changes - Review staged changes (git diff --staged) 3. PR by number - Review a specific pull request 4. Specific file - Review a single file in depth 5. Branch comparison - Compare two branches

Enter your choice or describe what you want reviewed:

</intake>

<routing>

</routing>

<reference_index>

References

• references/review-checklist.md - Comprehensive checklist by category
• references/common-issues.md - Frequently found problems and fixes
• references/security-patterns.md - Security vulnerabilities to watch for

</reference_index>

<workflows_index>

Available Workflows

1. review-diff.md - Review current uncommitted changes 2. review-staged.md - Review staged changes before commit 3. review-pr.md - Review a pull request by number 4. review-file.md - Deep review of a specific file 5. review-branch.md - Compare branches

</workflows_index>

<scripts_index>

Automation Scripts

Optional helper scripts for code review tasks:

• scripts/get-diff.sh - Get formatted git diff (unstaged/staged/branch)
• scripts/check-tests.sh - Verify test coverage for changed files
• scripts/format-report.sh - Generate review report template

Usage:

# Get diff
./scripts/get-diff.sh [unstaged|staged|branch] [branch-name]

# Check test coverage
./scripts/check-tests.sh [unstaged|staged]

# Generate report template
./scripts/format-report.sh "Feature Name"

</scripts_index>

Common Code Review Issues

Security Issues

Command Injection

Bad:

// Shell interpolation allows injection
import { execSync } from 'child_process'
const result = execSync(`ls ${userInput}`) // DANGEROUS

Good:

// execFile with array args prevents injection
import { execFile } from 'child_process'
execFile('ls', [userInput], (err, stdout) => { /* ... */ })

---

SQL Injection

Bad:

const query = `SELECT * FROM users WHERE id = ${userId}`

Good:

const query = 'SELECT * FROM users WHERE id = $1'
await db.query(query, [userId])

---

Path Traversal

Bad:

const filePath = path.join(baseDir, userInput)
await fs.readFile(filePath)

Good:

const filePath = path.join(baseDir, userInput)
const resolved = path.resolve(filePath)
if (!resolved.startsWith(path.resolve(baseDir))) {
  throw new Error('Path traversal detected')
}
await fs.readFile(resolved)

---

Logic Issues

Floating Promises

Bad:

async function process() {
  doAsyncThing() // Promise not awaited!
  return result
}

Good:

async function process() {
  await doAsyncThing()
  return result
}

---

Missing Null Checks

Bad:

const name = user.profile.name // Crashes if profile is null

Good:

const name = user?.profile?.name ?? 'Unknown'

---

Race Conditions

Bad:

if (!cache.has(key)) {
  const value = await fetchValue(key)
  cache.set(key, value) // Another request might have set it already
}

Good:

const existing = cache.get(key)
if (existing) return existing

const value = await fetchValue(key)
cache.set(key, value) // Or use a proper mutex/semaphore
return value

---

Performance Issues

N+1 Queries

Bad:

const users = await db.query('SELECT * FROM users')
for (const user of users) {
  const posts = await db.query('SELECT * FROM posts WHERE user_id = $1', [user.id])
}

Good:

const users = await db.query('SELECT * FROM users')
const userIds = users.map(u => u.id)
const posts = await db.query('SELECT * FROM posts WHERE user_id = ANY($1)', [userIds])

---

Memory Leaks - Event Listeners

Bad:

useEffect(() => {
  window.addEventListener('resize', handler)
  // Missing cleanup!
}, [])

Good:

useEffect(() => {
  window.addEventListener('resize', handler)
  return () => window.removeEventListener('resize', handler)
}, [])

---

Unnecessary Re-renders

Bad:

function Component() {
  const handler = () => {} // New function every render
  return <Button onClick={handler} />
}

Good:

function Component() {
  const handler = useCallback(() => {}, [])
  return <Button onClick={handler} />
}

---

TypeScript Issues

Using any

Bad:

function process(data: any) {
  return data.value // No type safety
}

Good:

interface Data {
  value: string
}
function process(data: Data) {
  return data.value // Type checked
}

---

Missing Return Types

Bad:

function calculate(x: number, y: number) {
  return x + y // Return type inferred but not explicit
}

Good:

function calculate(x: number, y: number): number {
  return x + y
}

Code Review Checklist

🔴 Critical (Must Fix)

Security

• [ ] No hardcoded secrets, API keys, or passwords
• [ ] Input validation on all user-provided data
• [ ] SQL queries use parameterized statements
• [ ] No command injection vulnerabilities (use execFile, not shell strings)
• [ ] Authentication/authorization checks in place
• [ ] Sensitive data not logged or exposed in errors
• [ ] HTTPS enforced for external requests

Data Integrity

• [ ] Database transactions for multi-step operations
• [ ] Proper error handling that doesn't corrupt state
• [ ] Backup/rollback strategy for destructive operations
• [ ] No race conditions in concurrent code

---

🟠 Bugs (Should Fix)

Logic

• [ ] Edge cases handled (empty arrays, null values, zero)
• [ ] Off-by-one errors checked
• [ ] Proper boolean logic (De Morgan's laws)
• [ ] Correct comparison operators (== vs ===)
• [ ] Async/await properly used (no floating promises)

Error Handling

• [ ] Errors caught and handled appropriately
• [ ] Error messages are helpful for debugging
• [ ] Cleanup happens even on error (finally blocks)
• [ ] No swallowed exceptions

---

🟡 Performance (Consider Fixing)

Database

• [ ] No N+1 query patterns
• [ ] Proper indexes exist for query patterns
• [ ] Pagination for large result sets
• [ ] Connection pooling configured

Memory

• [ ] No memory leaks (event listeners cleaned up)
• [ ] Large objects garbage collected when done
• [ ] Streams used for large file operations
• [ ] Caching strategy appropriate

Algorithms

• [ ] Appropriate data structures chosen
• [ ] No unnecessary iterations
• [ ] Early returns to avoid work
• [ ] Expensive operations cached or memoized

---

🔵 Maintainability (Nice to Fix)

Clarity

• [ ] Functions do one thing
• [ ] Variable names describe content
• [ ] Function names describe behavior
• [ ] Complex logic has explanatory comments
• [ ] No magic numbers (use named constants)

Structure

• [ ] DRY - no significant code duplication
• [ ] Single Responsibility Principle followed
• [ ] Dependencies injected, not hardcoded
• [ ] Testable code structure

Documentation

• [ ] Public APIs documented
• [ ] Complex algorithms explained
• [ ] README updated if behavior changed
• [ ] Breaking changes documented

---

⚪ Style (Optional)

Formatting

• [ ] Consistent indentation
• [ ] Line length reasonable
• [ ] Import organization consistent
• [ ] No trailing whitespace

Conventions

• [ ] Naming follows project conventions
• [ ] File organization matches patterns
• [ ] TypeScript types explicit where needed
• [ ] ESLint/Prettier rules followed

Workflow: Review Branch Comparison

Compare two branches to review all changes.

<required_reading>

Read these files NOW:

1. CLAUDE.md (if exists) - Project conventions 2. references/review-checklist.md - Review categories

</required_reading>

<process>

<step name="get-branches">

Step 1: Identify Branches

# Current branch
git branch --show-current

# Compare branches (default: main)
git log --oneline main..<BRANCH>

# Full diff
git diff main..<BRANCH> --stat

If no branches specified:

• Source: current branch
• Target: main (or master)

</step>

<step name="review-commits">

Step 2: Review Commit History

# Show commits in branch
git log main..<BRANCH> --oneline

# Show detailed commits
git log main..<BRANCH> --pretty=format:"%h %s" --no-merges

Check:

• [ ] Commits are logical units
• [ ] Commit messages are clear
• [ ] No "WIP" or "fix" only commits
• [ ] Squash candidates identified

</step>

<step name="review-diff">

Step 3: Review Full Diff

git diff main..<BRANCH>

Apply standard review process: 1. Critical issues first 2. Bugs second 3. Performance third 4. Style last

</step>

<step name="generate-report">

Step 4: Generate Report

# Branch Review: `<BRANCH>` → `main`

## Summary
- **Commits:** <N>
- **Files Changed:** <N>
- **Additions:** +<N>
- **Deletions:** -<N>

## Commit Quality
- [ ] Commits are atomic
- [ ] Messages are descriptive
- [ ] History is clean

### Squash Recommendations
[If any commits should be combined]

## Code Review

### 🔴 Critical Issues
[Must fix before merge]

### 🟠 Bugs
[Should fix]

### 🟡 Suggestions
[Nice to have]

## Files Changed
| File | +/- | Notes |
|------|-----|-------|
| path/file.ts | +50/-10 | [brief note] |

## Ready to Merge?
- [ ] ✅ **Yes** - Approve
- [ ] 🔄 **Almost** - Minor fixes
- [ ] ❌ **No** - Significant issues

</step>

</process>

<success_criteria>

• [ ] All commits reviewed
• [ ] Full diff reviewed
• [ ] Issues prioritized
• [ ] Merge readiness assessed

</success_criteria>

Workflow: Review Current Diff

<required_reading>

Read these files NOW before proceeding:

1. CLAUDE.md (if exists in project root) - Project-specific conventions 2. references/review-checklist.md - Review categories and priorities 3. references/common-issues.md - Patterns to watch for

</required_reading>

<process>

<step name="gather-context">

Step 1: Gather Context

1. Check for project conventions:

   cat CLAUDE.md 2>/dev/null || echo "No CLAUDE.md found"

2. Get the current diff:

   git diff --stat
   git diff

3. Understand what changed:

• Which files were modified?
• What's the apparent purpose of these changes?
• Are tests included?

</step>

<step name="categorize-changes">

Step 2: Categorize Changes

Group the changes by type:

• New code - Additions that need full review
• Modifications - Changes to existing code (check for regressions)
• Deletions - Removed code (check nothing depends on it)
• Refactoring - Behavior should be unchanged
• Config/Dependencies - Check for security implications

</step>

<step name="review-critical">

Step 3: Review Critical Issues First

Check for 🔴 Critical issues:

1. Security vulnerabilities:

• Hardcoded secrets?
• Command/SQL injection?
• Path traversal?
• Missing auth checks?

2. Data integrity risks:

• Missing transactions?
• Race conditions?
• Improper error handling?

Stop and report critical issues immediately before continuing.

</step>

<step name="review-bugs">

Step 4: Review for Bugs

Check for 🟠 Bug issues:

1. Logic errors:

• Edge cases (null, empty, zero)?
• Off-by-one errors?
• Incorrect comparisons?

2. Async issues:

• Floating promises?
• Missing await?
• Proper error propagation?

3. Error handling:

• Errors caught appropriately?
• Cleanup in finally blocks?

</step>

<step name="review-performance">

Step 5: Review Performance

Check for 🟡 Performance issues:

1. Database:

• N+1 queries?
• Missing indexes?
• Unbounded queries?

2. Memory:

• Event listener cleanup?
• Large object lifecycle?

3. Algorithms:

• Appropriate data structures?
• Unnecessary work?

</step>

<step name="review-maintainability">

Step 6: Review Maintainability

Check for 🔵 Maintainability issues:

1. Code clarity:

• Descriptive names?
• Single responsibility?
• No magic numbers?

2. Project conventions:

• Follows CLAUDE.md rules?
• Consistent with codebase patterns?

</step>

<step name="generate-report">

Step 7: Generate Report

Format your review as:

# Code Review: [Brief description of changes]

## Summary
[1-2 sentences about what the change does]

## 🔴 Critical Issues
[List critical issues with line numbers and fixes, or "None found"]

## 🟠 Bugs
[List potential bugs with line numbers and fixes, or "None found"]

## 🟡 Performance
[List performance concerns, or "None found"]

## 🔵 Maintainability
[List suggestions, or "Looks good"]

## ⚪ Style Notes
[Minor style suggestions, or skip this section]

## Verdict
- [ ] ✅ **Approve** - Good to merge
- [ ] 🔄 **Request Changes** - Needs fixes before merge
- [ ] ❓ **Questions** - Need clarification on intent

</step>

</process>

<anti_patterns>

Avoid These Mistakes

• ❌ Reviewing without reading CLAUDE.md first
• ❌ Focusing on style before critical issues
• ❌ Being vague ("this could be better")
• ❌ Not providing specific fixes
• ❌ Reviewing unchanged code
• ❌ Bikeshedding on minor issues

</anti_patterns>

<success_criteria>

Review is Complete When

• [ ] CLAUDE.md conventions checked (if exists)
• [ ] All changed files reviewed
• [ ] Critical/security issues addressed first
• [ ] Each issue has specific file:line reference
• [ ] Each issue has suggested fix
• [ ] Clear verdict provided

</success_criteria>

Workflow: Review Specific File

Deep review of a single file.

<required_reading>

Read these files NOW:

1. CLAUDE.md (if exists) - Project conventions 2. references/review-checklist.md - Full checklist 3. references/common-issues.md - Patterns to watch

</required_reading>

<process>

<step name="read-file">

Step 1: Read the File

# Read the specified file
cat <FILE_PATH>

# Check git history for context
git log --oneline -10 <FILE_PATH>

</step>

<step name="understand-purpose">

Step 2: Understand Purpose

• What does this file do?
• What is its role in the system?
• What are its dependencies?
• Who are its dependents?

</step>

<step name="deep-review">

Step 3: Deep Review

Structure

• [ ] Single responsibility
• [ ] Logical organization
• [ ] Appropriate file size
• [ ] Clear exports

Code Quality

• [ ] Functions are focused
• [ ] Names are descriptive
• [ ] Comments explain why
• [ ] No magic numbers
• [ ] DRY principle followed

Security

• [ ] Input validation
• [ ] Safe data handling
• [ ] No hardcoded secrets
• [ ] Proper error messages

Performance

• [ ] No obvious inefficiencies
• [ ] Appropriate data structures
• [ ] Resource cleanup

Testing

• [ ] Test file exists
• [ ] Edge cases covered
• [ ] Mocks appropriate

</step>

<step name="generate-report">

Step 4: Generate Report

# File Review: `<FILE_PATH>`

## Purpose
[What this file does]

## Overall Health: [🟢 Good / 🟡 Needs Work / 🔴 Problematic]

## Issues Found

### 🔴 Critical
[Security/data issues]

### 🟠 Bugs
[Logic issues]

### 🟡 Improvements
[Performance/maintainability]

## Specific Recommendations

### Line <N>

// problematic code

**Issue:** [description]
**Fix:** [suggestion]

## Test Coverage
[Assessment of test file if exists]

## Refactoring Suggestions
[Larger structural improvements if needed]

</step>

</process>

<success_criteria>

• [ ] File purpose understood
• [ ] All code sections reviewed
• [ ] Issues prioritized
• [ ] Specific line references
• [ ] Actionable suggestions

</success_criteria>

Workflow: Review Pull Request

<required_reading>

Read these files NOW before proceeding:

1. CLAUDE.md (if exists) - Project-specific conventions 2. references/review-checklist.md - Review categories 3. references/common-issues.md - Patterns to watch for

</required_reading>

<process>

<step name="fetch-pr">

Step 1: Fetch PR Details

Use the GitHub Issues MCP (if available) or gh CLI:

# Get PR details
gh pr view <PR_NUMBER> --json title,body,files,additions,deletions,baseRefName,headRefName

# Get the diff
gh pr diff <PR_NUMBER>

Note:

• PR number from user input
• If no number provided, check for current branch PR

</step>

<step name="understand-context">

Step 2: Understand Context

1. Read PR description - What is this PR trying to accomplish? 2. Check linked issues - Is there context in related issues? 3. Review file list - Which areas of codebase are affected? 4. Check PR size - Large PRs may need to be broken down

Size guidance:

• Small (<100 lines): Quick review
• Medium (100-500 lines): Standard review
• Large (>500 lines): Consider suggesting split

</step>

<step name="check-conventions">

Step 3: Check Project Conventions

If CLAUDE.md exists: 1. Read project conventions 2. Note specific rules for this file type 3. Check for required patterns

Common checks:

• [ ] Follows naming conventions
• [ ] Imports organized correctly
• [ ] Tests included (if required)
• [ ] Documentation updated (if required)

</step>

<step name="review-changes">

Step 4: Review Changes by Priority

🔴 Critical (Security/Data)

• Authentication/authorization
• Input validation
• Data handling
• Error exposure

🟠 Bugs (Logic)

• Edge cases
• Error handling
• Async patterns
• State management

🟡 Performance

• Database queries
• Memory management
• Algorithm efficiency

🔵 Maintainability

• Code clarity
• DRY violations
• Test coverage

</step>

<step name="check-tests">

Step 5: Review Tests

• [ ] Tests exist for new functionality
• [ ] Tests cover edge cases
• [ ] Tests are meaningful (not just coverage)
• [ ] Existing tests still pass

If tests missing:

**Missing Tests:**
- [ ] Test for [specific scenario]
- [ ] Edge case: [empty input / null / etc]

</step>

<step name="generate-review">

Step 6: Generate PR Review

Format output as:

# PR Review: #<NUMBER> - <TITLE>

## Summary
[Brief description of what this PR does]

## Overall Assessment
- [ ] ✅ **Approve** - Ready to merge
- [ ] 🔄 **Request Changes** - Issues need addressing
- [ ] 💬 **Comment** - Questions/suggestions only

## 🔴 Critical Issues
[Must fix before merge]

## 🟠 Bugs/Concerns
[Should fix]

## 🟡 Suggestions
[Nice to have]

## 📝 Inline Comments

### `path/to/file.ts:42`

// Problematic code

**Issue:** [Description]
**Suggestion:** [Fix]

---

## Tests
- [ ] Adequate coverage
- [ ] Edge cases covered
- [ ] [Specific missing test]

## Documentation
- [ ] README updated (if needed)
- [ ] API docs updated (if needed)
- [ ] Comments adequate

</step>

</process>

<anti_patterns>

Avoid These Mistakes

• ❌ Reviewing without understanding PR purpose
• ❌ Nitpicking style over substance
• ❌ Not checking for missing tests
• ❌ Approving without reading all files
• ❌ Focusing on unchanged code
• ❌ Vague feedback ("this could be better")

</anti_patterns>

<success_criteria>

Review is Complete When

• [ ] PR description understood
• [ ] All changed files reviewed
• [ ] Critical issues identified first
• [ ] Each comment has specific file:line
• [ ] Each issue has suggested fix
• [ ] Test coverage assessed
• [ ] Clear approve/request changes verdict

</success_criteria>

Workflow: Review Staged Changes

Review changes that are staged for commit (git add).

<required_reading>

Read these files NOW:

1. CLAUDE.md (if exists) - Project conventions 2. references/review-checklist.md - Review categories

</required_reading>

<process>

<step name="get-staged">

Step 1: Get Staged Changes

# Show staged files
git diff --staged --stat

# Show staged diff
git diff --staged

</step>

<step name="quick-review">

Step 2: Quick Pre-Commit Review

Focus on commit-blocking issues:

Must Check

• [ ] No secrets/credentials staged
• [ ] No debug code (console.log, debugger)
• [ ] No commented-out code blocks
• [ ] No TODO without issue reference
• [ ] File names follow conventions

Should Check

• [ ] Imports cleaned up
• [ ] No unused variables
• [ ] Error handling present
• [ ] Types explicit (TypeScript)

</step>

<step name="generate-feedback">

Step 3: Generate Feedback

# Staged Changes Review

## Ready to Commit?
- [ ] ✅ **Yes** - Looks good
- [ ] ⚠️ **Almost** - Minor fixes needed
- [ ] ❌ **No** - Issues found

## Issues Found
[List any blocking issues]

## Suggestions
[Optional improvements]

## Recommended Commit Message

<type>(<scope>): <description>

[body if needed]

</step>

</process>

<success_criteria>

• [ ] No secrets staged
• [ ] No debug code
• [ ] Changes are intentional
• [ ] Commit message suggested

</success_criteria>