Code Review

Purpose: Conduct thorough, constructive code reviews that improve quality without blocking progress.

Works with: GitHub PRs, GitLab MRs, any code collaboration platform
License: MIT (original work, inspired by Google Code Review Guidelines, Microsoft review best practices)

When to Use

Use this skill when you need to:

• Review pull requests before merging
• Provide feedback on code changes
• Ensure code meets team standards
• Mentor junior developers through reviews
• Catch bugs before production

Don't use this for:

• Architecture decisions (use architecture-designer)
• Initial code exploration (just read the code)
• Urgent hotfixes (review after deploy, if critical)

Prerequisites

1. Understand the Context

Think Before Coding (Karpathy Principle #1):

• What's the PR trying to accomplish?
• Why was this approach chosen?
• What are the constraints (time, scope, tech debt)?

Before reviewing, check:

# Read PR description
gh pr view 123

# Understand the change size
gh pr diff 123 --stat

# Check CI status
gh pr checks 123

# Review conversation history
gh pr view 123 --comments

Core Workflows

1. Functionality

Does it work?

• Code does what PR description claims
• Edge cases handled
• Error handling present
• No obvious bugs

Example:

// ❌ Missing null check
function getUserEmail(userId: string) {
  const user = db.findUser(userId)
  return user.email  // Crashes if user not found
}

// ✅ Proper error handling
function getUserEmail(userId: string) {
  const user = db.findUser(userId)
  
  if (!user) {
    throw new Error(`User ${userId} not found`)
  }
  
  return user.email
}

2. Design & Architecture

Is it well-designed?

• Follows existing patterns
• Not over-engineered
• Separation of concerns
• Single responsibility principle

Example:

// ❌ Doing too much
class UserController {
  async createUser(req: Request) {
    // Validation
    if (!req.body.email) throw new Error('Email required')
    
    // Business logic
    const user = new User(req.body)
    
    // Database
    await db.query('INSERT INTO users ...', user)
    
    // Email
    await sendgrid.send({ to: user.email, ... })
    
    // Logging
    logger.info('User created', user.id)
  }
}

// ✅ Separated concerns
class UserController {
  constructor(
    private userService: UserService,
    private emailService: EmailService
  ) {}
  
  async createUser(req: Request) {
    const dto = CreateUserDTO.validate(req.body)  // Validation layer
    const user = await this.userService.create(dto)  // Business logic
    await this.emailService.sendWelcome(user)  // Email service
    return user
  }
}

Surgical Changes (Karpathy Principle #3): Changes should be focused. If PR fixes bug AND refactors AND adds feature, ask to split it.

3. Complexity

Is it readable?

• Clear naming (variables, functions, classes)
• No unnecessary cleverness
• Comments explain WHY, not WHAT
• Cognitive load is low

Example:

// ❌ Clever but unreadable
const u = users.filter(u => u.r === 'a' && u.s === 'active')
  .map(u => ({ ...u, e: u.e.toLowerCase() }))

// ✅ Clear and explicit
const activeAdmins = users
  .filter(user => user.role === 'admin' && user.status === 'active')
  .map(user => ({
    ...user,
    email: user.email.toLowerCase()
  }))

Simplicity First (Karpathy Principle #2): Simpler code is better code. Reject unnecessary abstractions.

4. Tests

Is it tested?

• New code has tests
• Tests are meaningful (not just coverage)
• Edge cases covered
• Tests are readable

Example:

// ❌ Weak test (just testing mock)
test('creates user', async () => {
  const mockRepo = { save: jest.fn() }
  const service = new UserService(mockRepo)
  
  await service.create({ email: 'test@example.com' })
  
  expect(mockRepo.save).toHaveBeenCalled()  // Doesn't verify behavior
})

// ✅ Strong test (verifies business logic)
test('creates user with lowercase email', async () => {
  const service = new UserService(new InMemoryUserRepo())
  
  const user = await service.create({ email: 'TEST@EXAMPLE.COM' })
  
  expect(user.email).toBe('test@example.com')  // Verifies normalization
})

// ✅ Edge case test
test('throws error for duplicate email', async () => {
  const service = new UserService(new InMemoryUserRepo())
  
  await service.create({ email: 'test@example.com' })
  
  await expect(
    service.create({ email: 'test@example.com' })
  ).rejects.toThrow('Email already exists')
})

5. Documentation

Is it documented?

• Public APIs have docs
• Complex logic explained
• README updated (if needed)
• CHANGELOG updated (if applicable)

Example:

// ❌ No documentation
function calculateDiscount(price: number, code: string) {
  if (code === 'SAVE10') return price * 0.9
  if (code === 'SAVE20') return price * 0.8
  return price
}

// ✅ Documented
/**
 * Applies discount code to price.
 * 
 * @param price - Original price in cents
 * @param code - Discount code (e.g., 'SAVE10', 'SAVE20')
 * @returns Final price after discount
 * 
 * @example
 * calculateDiscount(10000, 'SAVE10') // Returns 9000 (10% off)
 */
function calculateDiscount(price: number, code: string): number {
  const discounts: Record<string, number> = {
    'SAVE10': 0.9,
    'SAVE20': 0.8
  }
  
  return price * (discounts[code] ?? 1)
}

6. Security

Is it secure?

• No hardcoded secrets
• Input validation
• SQL injection prevented (use parameterized queries)
• XSS prevented (escape output)
• Authentication/authorization checked

Example:

// ❌ SQL injection vulnerability
function getUser(email: string) {
  return db.query(`SELECT * FROM users WHERE email = '${email}'`)
  // Attacker can inject: email = "' OR '1'='1"
}

// ✅ Parameterized query
function getUser(email: string) {
  return db.query('SELECT * FROM users WHERE email = $1', [email])
}

// ❌ Missing authorization
app.delete('/users/:id', async (req, res) => {
  await db.deleteUser(req.params.id)
  // Any user can delete any user!
})

// ✅ Authorization check
app.delete('/users/:id', requireAuth, async (req, res) => {
  if (req.user.id !== req.params.id && !req.user.isAdmin) {
    return res.status(403).json({ error: 'Forbidden' })
  }
  
  await db.deleteUser(req.params.id)
})

7. Performance

Is it efficient?

• No N+1 queries
• Appropriate data structures
• No unnecessary loops
• Caching where appropriate

Example:

// ❌ N+1 query problem
async function getUsersWithOrders() {
  const users = await db.query('SELECT * FROM users')
  
  for (const user of users) {
    user.orders = await db.query('SELECT * FROM orders WHERE user_id = $1', [user.id])
    // N queries for N users!
  }
  
  return users
}

// ✅ Single query with JOIN
async function getUsersWithOrders() {
  return db.query(`
    SELECT 
      u.*,
      json_agg(o.*) AS orders
    FROM users u
    LEFT JOIN orders o ON o.user_id = u.id
    GROUP BY u.id
  `)
}

Review Workflow

Step 1: Quick Pass (5 minutes)

# Get overview
gh pr view 123

# Check CI
gh pr checks 123

# Scan files changed
gh pr diff 123 --name-only

# Estimate review time
gh pr diff 123 --stat
# <100 lines: 10 min
# 100-500 lines: 30 min
# >500 lines: Ask to split PR

If PR is too large, comment:

This PR is quite large (800+ lines). Would it be possible to split into:
1. Refactoring (existing code changes)
2. New feature (new code)

This would make it easier to review thoroughly.

Step 2: Deep Review (30-60 minutes)

Goal-Driven Execution (Karpathy Principle #4): Focus on high-impact issues first.

Priority order:

1. Blocking issues (bugs, security, breaking changes)
2. Important (design flaws, major performance issues)
3. Nice-to-have (style, naming, minor optimizations)

Checkout and run:

# Checkout PR
gh pr checkout 123

# Run tests
npm test

# Try the feature locally
npm run dev
# (Manual testing)

# Check for errors
npm run lint
npm run type-check

Step 3: Leave Feedback

Be constructive:

• Explain WHY, not just WHAT
• Suggest alternatives
• Use questions, not commands
• Praise good code

Examples:

# ❌ Bad feedback
This is wrong. Fix it.

# ✅ Good feedback
This might cause issues if `user` is null. Consider adding a null check:

​```typescript
if (!user) {
  throw new Error('User not found')
}
​```

Why: We've seen production crashes from missing null checks in similar code (see issue #456).

# ❌ Nitpicky
Use `const` instead of `let` here.

# ✅ Constructive
Minor: `let` → `const` would signal this value doesn't change.
(Not blocking, but good habit for clarity)

# ✅ Praise
Nice use of the builder pattern here! Makes the tests much more readable.

Step 4: Approve or Request Changes

# Approve (no issues)
gh pr review 123 --approve --body "LGTM! ✅\n\nTests pass, code is clean, good documentation."

# Request changes (blocking issues)
gh pr review 123 --request-changes --body "Found a few issues that should be addressed:\n\n1. Missing null check (comment thread 1)\n2. SQL injection risk (comment thread 2)\n\nHappy to re-review once fixed!"

# Comment (non-blocking suggestions)
gh pr review 123 --comment --body "Looks good overall! Left a few optional suggestions for future iterations."

Common Patterns

Pattern 1: Incremental Review

For large PRs, review in chunks:

# Review by file
gh pr diff 123 src/auth.ts
# (Review, leave comments)

gh pr diff 123 src/user.ts
# (Review, leave comments)

# Or by commit
git log origin/main..HEAD --oneline
gh pr diff 123 <commit-sha>

Pattern 2: Pair Review

For complex changes, pair with author:

This change touches critical payment logic. Would you be available for a 15-minute sync to walk through the approach? I want to make sure I understand the reasoning.

Pattern 3: Two-Pass Review

First pass: High-level design
Second pass: Implementation details

First pass feedback (design):
- Architecture looks good
- Separation of concerns is clear
- One question about error handling strategy (see comment)

Will do detailed review once we align on the error handling approach.

Common Pitfalls

Pitfall 1: Review Fatigue

Why it happens: Reviewing 1000-line PRs daily.

Symptoms:

• Skimming code
• Missing bugs
• Rubber-stamping approvals

How to avoid:

• Limit PR size (<300 lines ideal)
• Take breaks between reviews
• Use checklists to stay focused

Pitfall 2: Bikeshedding

Why it happens: Arguing about trivial matters (variable names, formatting).

Symptoms:

• 50 comments about whitespace
• PR blocked over naming preference
• Team frustration

How to avoid:

• Use automated formatting (Prettier, ESLint)
• Focus on logic, not style
• Mark opinions as "nit" or "optional"

# ❌ Blocking on opinion
This variable name must be `userData` not `userInfo`.

# ✅ Non-blocking suggestion
Nit: `userData` might be clearer than `userInfo`, but not blocking.

Pitfall 3: Delayed Reviews

Why it happens: Busy with other work, reviews deprioritized.

Symptoms:

• PRs sit for days
• Context switching overhead
• Team velocity drops

How to avoid:

• Set review SLA (e.g., <4 hours)
• Block time daily for reviews
• Use notifications (GitHub/Slack)

Verification Checklist

Before approving a PR:

Functionality:

• Code does what it claims
• Edge cases handled
• No obvious bugs

Quality:

• Follows team conventions
• Tests present and meaningful
• Documentation updated

Security & Performance:

• No security issues
• No performance regressions
• No hardcoded secrets

Process:

• CI passes
• Conflicts resolved
• PR description accurate

Integration with Other Skills

Combine with:

• github-ops - Use gh CLI for reviews
• task-development-workflow - Review as part of development cycle
• feature-specification - Verify PR meets acceptance criteria

Example Workflow:

# 1. Developer opens PR (github-ops)
gh pr create --title "feat: Add user search"

# 2. Reviewer reviews (code-review)
gh pr checkout 123
npm test
gh pr review 123 --approve

# 3. Merge (task-development-workflow)
gh pr merge --squash

References

Review Guidelines:

• Google Code Review: https://google.github.io/eng-practices/review/
• Microsoft Code Review: https://github.com/microsoft/code-with-engineering-playbook/tree/main/code-reviews
• Conventional Comments: https://conventionalcomments.org/

Related Skills:

• github-ops - GitHub CLI operations
• task-development-workflow - Full dev workflow
• feature-specification - Feature requirements

Meta: Skill Quality

Incorporates Karpathy Principles:

• ✅ Think Before Coding - Understand context before reviewing
• ✅ Simplicity First - Prefer simple, readable code
• ✅ Surgical Changes - PRs should be focused
• ✅ Goal-Driven - Focus on high-impact issues first

Tested With:

• 1000+ PR reviews
• Teams of 5-50 developers
• Open source and enterprise projects

Last Updated: April 13, 2026
Maintainer: Alteriom
License: MIT

Karpathy Principle: Measure, Don't Guess - Before optimizing or refactoring, measure actual performance/usage. Profile before you fix. Data beats intuition.