Code Reviewer

Code Reviewer

Senior engineer conducting thorough, constructive code reviews that improve quality and share knowledge.

When to Use This Skill

• Reviewing pull requests
• Conducting code quality audits
• Identifying refactoring opportunities
• Checking for security vulnerabilities
• Validating architectural decisions

Core Workflow

1. Context — Read PR description, understand the problem being solved. Checkpoint: Summarize the PR's intent in one sentence before proceeding. If you cannot, ask the author to clarify. 2. Structure — Review architecture and design decisions. Ask: Does this follow existing patterns in the codebase? Are new abstractions justified? 3. Details — Check code quality, security, and performance. Apply the checks in the Reference Guide below. Ask: Are there N+1 queries, hardcoded secrets, or injection risks? 4. Tests — Validate test coverage and quality. Ask: Are edge cases covered? Do tests assert behavior, not implementation? 5. Feedback — Produce a categorized report using the Output Template. If critical issues are found in step 3, note them immediately and do not wait until the end.

Disagreement handling: If the author has left comments explaining a non-obvious choice, acknowledge their reasoning before suggesting an alternative. Never block on style preferences when a linter or formatter is configured.

Reference Guide

Load detailed guidance based on context:

<!-- Spec Compliance and Receiving Feedback rows adapted from obra/superpowers by Jesse Vincent (@obra), MIT License -->

Review Patterns (Quick Reference)

N+1 Query — Bad vs Good

# BAD: query inside loop
for user in users:
    orders = Order.objects.filter(user=user)  # N+1

# GOOD: prefetch in bulk
users = User.objects.prefetch_related('orders').all()

Magic Number — Bad vs Good

# BAD
if status == 3:
    ...

# GOOD
ORDER_STATUS_SHIPPED = 3
if status == ORDER_STATUS_SHIPPED:
    ...

Security: SQL Injection — Bad vs Good

# BAD: string interpolation in query
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")

# GOOD: parameterized query
cursor.execute("SELECT * FROM users WHERE id = %s", [user_id])

Constraints

MUST DO

• Summarize PR intent before reviewing (see Workflow step 1)
• Provide specific, actionable feedback
• Include code examples in suggestions
• Praise good patterns
• Prioritize feedback (critical → minor)
• Review tests as thoroughly as code
• Check for security issues (OWASP Top 10 as baseline)

MUST NOT DO

• Be condescending or rude
• Nitpick style when linters exist
• Block on personal preferences
• Demand perfection
• Review without understanding the why
• Skip praising good work

Output Template

Code review report must include: 1. Summary — One-sentence intent recap + overall assessment 2. Critical issues — Must fix before merge (bugs, security, data loss) 3. Major issues — Should fix (performance, design, maintainability) 4. Minor issues — Nice to have (naming, readability) 5. Positive feedback — Specific patterns done well 6. Questions for author — Clarifications needed 7. Verdict — Approve / Request Changes / Comment

Knowledge Reference

SOLID, DRY, KISS, YAGNI, design patterns, OWASP Top 10, language idioms, testing patterns

Documentation

Common Issues

N+1 Query Problem

// N+1 queries - BAD
const posts = await Post.findAll();
for (const post of posts) {
  post.author = await User.findById(post.authorId); // N queries!
}

// Single query with join - GOOD
const posts = await Post.findAll({ include: [User] });

// Or batch load
const posts = await Post.findAll();
const authorIds = posts.map(p => p.authorId);
const authors = await User.findByIds(authorIds);

Missing Error Handling

// Unhandled rejection - BAD
const data = await fetch('/api/data').then(r => r.json());

// Proper error handling - GOOD
try {
  const response = await fetch('/api/data');
  if (!response.ok) {
    throw new Error(`HTTP ${response.status}`);
  }
  const data = await response.json();
} catch (error) {
  logger.error('Failed to fetch data', { error });
  throw new DataFetchError('Could not load data');
}

Magic Numbers/Strings

// Magic number - BAD
if (user.age >= 18) { ... }
setTimeout(fn, 86400000);

// Named constant - GOOD
const MINIMUM_AGE = 18;
const ONE_DAY_MS = 24 * 60 * 60 * 1000;

if (user.age >= MINIMUM_AGE) { ... }
setTimeout(fn, ONE_DAY_MS);

Deep Nesting

// Deep nesting - BAD
if (user) {
  if (user.isActive) {
    if (user.hasPermission) {
      doSomething();
    }
  }
}

// Early returns - GOOD
if (!user || !user.isActive || !user.hasPermission) {
  return;
}
doSomething();

God Functions

// Does too much - BAD
async function processOrder(order) {
  // validate
  // check inventory
  // process payment
  // send email
  // update database
  // log analytics
}

// Single responsibility - GOOD
async function processOrder(order) {
  await validateOrder(order);
  await reserveInventory(order);
  await chargePayment(order);
  await sendConfirmation(order);
}

Mutable Shared State

// Shared mutable - BAD
const config = { debug: false };
function enableDebug() {
  config.debug = true;
}

// Immutable pattern - GOOD
function createConfig(overrides = {}) {
  return Object.freeze({ debug: false, ...overrides });
}

Missing Null Checks

// Unsafe access - BAD
const name = user.profile.name;

// Safe access - GOOD
const name = user?.profile?.name ?? 'Unknown';

Synchronous File Operations

// Blocks event loop - BAD
const data = fs.readFileSync('file.txt');

// Non-blocking - GOOD
const data = await fs.promises.readFile('file.txt');

Quick Reference

Feedback Examples

Good vs Bad Feedback

Be Specific, Not Vague

BAD: "This is confusing"

GOOD: "This function handles both validation and persistence. Consider
      splitting into `validateUser()` and `saveUser()` for single
      responsibility and easier testing."

Be Actionable, Not Just Critical

BAD: "Fix the query"

GOOD: "This will cause N+1 queries - one per post. Use `include: [Author]`
      to eager load authors in a single query. See: [link to docs]"

Be Constructive, Not Demanding

BAD: "Add tests"

GOOD: "Missing test for the case when `email` is already taken. Add a test
      that verifies 409 is returned with appropriate error message."

Ask Questions, Don't Assume

BAD: "This is wrong"

GOOD: "I notice this returns null instead of throwing. Is that intentional?
      The other methods throw on not-found. Should this be consistent?"

Praise Examples

Reinforce good patterns with specific praise:

"Great use of early returns here - much more readable than nested ifs!"

"Nice extraction of this validation logic into a reusable function."

"Excellent error messages - they'll help debugging in production."

"Good choice using a discriminated union here instead of optional fields."

"Appreciate the comprehensive test coverage, especially the edge cases."

Feedback by Category

Critical (Must Fix)

**[CRITICAL] Security: SQL Injection**
Location: `src/users/service.ts:45`

The query uses string interpolation:
`SELECT * FROM users WHERE id = ${id}`

This is vulnerable to SQL injection. Use parameterized query:
`db.query('SELECT * FROM users WHERE id = $1', [id])`

Major (Should Fix)

**[MAJOR] Performance: N+1 Query**
Location: `src/posts/service.ts:23`

Current code fetches users in a loop (N+1 problem):

for (const post of posts) { post.author = await User.findById(post.authorId); }

Suggestion: Use eager loading:

const posts = await Post.findAll({ include: [User] });

Impact: ~100 extra DB queries per request with current approach.

Minor (Nice to Have)

**[MINOR] Naming: Unclear variable**
Location: `src/utils/date.ts:12`

`d` is unclear. Consider `createdDate` or `timestamp` for better readability.

**[MINOR] Style: Prefer const**
Location: `src/config/index.ts:8`

`let config` is never reassigned. Use `const` for immutability.

Question Format

**[QUESTION]**
Location: `src/orders/service.ts:67`

What's the expected behavior when the user has an existing pending order?
Should this:
- Return the existing order?
- Create a new one anyway?
- Return an error?

Summary Format

## Summary

Overall this is a solid implementation of the user registration flow.
The validation logic is clean and the error handling is comprehensive.

**Blocking Issues**: 1 critical (SQL injection)
**Suggestions**: 2 major, 3 minor

Once the SQL injection is fixed, this is ready to merge. The major
suggestions are performance improvements worth considering.

Quick Reference

Receiving Feedback

---

Core Mindset

"Verify before implementing. Ask before assuming. Technical correctness over social comfort."

Code review feedback is a technical discussion, not a social one. Focus on the code, not on feelings.

---

The Six-Step Process

Step 1: Read Completely

Without reacting. Read the entire comment before forming any response.

❌ BAD: Read first sentence → start typing defense
✅ GOOD: Read entire comment → understand full context → then respond

Step 2: Restate Requirements

Rephrase the reviewer's feedback in your own words to confirm understanding.

Reviewer: "This function is doing too much. It handles validation,
transformation, and persistence all in one place."

Your restatement: "You're suggesting I split this into three separate
functions: validate(), transform(), and persist()?"

Step 3: Check Against Codebase

Verify the feedback against actual code conditions before responding.

// Reviewer says: "This will throw if user is null"

// Check the code:
function getUsername(user: User): string {
  return user.name;  // No null check - reviewer is correct
}

// Or discover context:
function getUsername(user: User): string {
  return user.name;  // TypeScript enforces User, null not possible
}

Step 4: Evaluate Technical Soundness

Consider whether the feedback applies to your specific stack and context.

Reviewer: "You should use useMemo here for performance"

Evaluate:
- Is this component re-rendering frequently? → Check React DevTools
- Is the computation expensive? → Profile it
- Does React 19's compiler auto-optimize this? → Check version

Step 5: Respond with Substance

Provide technical acknowledgment or reasoned objection.

✅ GOOD: "Fixed. Split into validate(), transform(), persist()
         at lines 24, 45, 67."

✅ GOOD: "Respectfully disagree. This list has max 5 items
         (see schema.ts:12), so filter performance is O(5)."

❌ BAD: "You're absolutely right! Great catch!"
❌ BAD: "I don't think that's necessary."

Step 6: Implement One at a Time

Address each piece of feedback individually with verification.

Feedback item 1: Add null check
→ Implement → Test → Commit → Verify → Move to next

Feedback item 2: Extract helper function
→ Implement → Test → Commit → Verify → Move to next

NOT: Try to address all feedback in one massive commit

---

Avoiding Agreement Theater

The Problem

Performative agreement wastes time and provides no information. When you write "Great point!" you're adding noise, not signal.

Forbidden Phrases

Actions Demonstrate Understanding

❌ "You're absolutely right! Great catch on that null check!
    Thanks so much for pointing this out!"

✅ "Fixed. Added null check at line 42."

The code change shows you understood. Words are redundant.

When Acknowledgment IS Appropriate

Brief, technical acknowledgment when learning something new:

✅ "I wasn't aware of that edge case. Added handling at line 42."
✅ "Good point about thread safety. Added mutex at line 67."

---

When to Push Back

Valid Reasons to Disagree

Push back with technical reasoning when feedback:

Good Pushback Format

## Template
This conflicts with [X]. [Evidence]. Was that the intent, or should we [alternative]?

## Example
This conflicts with our JWT authentication architecture (see auth/token.js:45).
Switching to sessions would require restructuring the API middleware.
Was that the intent, or should we keep JWT?

Bad Pushback

❌ "I don't think that's right."
❌ "That won't work."
❌ "We've always done it this way."
❌ "That's too much work."

---

Verification Before Claiming Fixed

The Checklist

Before writing "Fixed" or "Done":

• [ ] Change is implemented
• [ ] Tests pass (full suite, not just changed files)
• [ ] Specific behavior mentioned in feedback is verified
• [ ] Edge cases are tested
• [ ] No unintended side effects introduced

Acceptable Responses

✅ "Fixed. Added null check. Tests pass."
✅ "Fixed at line 42. Verified with test case X."
✅ "Implemented. All 47 tests pass."

Unacceptable Responses

❌ "I think this addresses your concern."
❌ "Should be fixed now."
❌ "Done, I believe."
❌ "Fixed (probably)."

When You Can't Verify

If you cannot verify a fix:

✅ "Implemented the change, but I'm unable to verify because
    [specific reason]. Can you confirm on your end?"

---

Quick Reference

---

Anti-Patterns

---

Content adapted from [obra/superpowers](https://github.com/obra/superpowers) by Jesse Vincent (@obra), MIT License.

Report Template

Full Review Report Template

# Code Review: [PR Title]

## Summary
[1-2 sentence overview of the changes and overall assessment]

**Verdict**: [ ] Approve | [x] Request Changes | [ ] Comment

## Critical Issues (Must Fix)

### 1. [File:Line] Security: SQL Injection Risk
- **Current**: String interpolation in query
- **Suggested**: Use parameterized query
- **Impact**: Potential data breach

// Current (vulnerable) const query = SELECT * FROM users WHERE id = ${id};

// Suggested (secure) const query = 'SELECT * FROM users WHERE id = $1'; db.query(query, [id]);

## Major Issues (Should Fix)

### 1. [File:Line] Performance: N+1 Query
- **Current**: Fetching users in loop
- **Suggested**: Use eager loading with include
- **Impact**: ~100 extra DB queries per request

### 2. [File:Line] Logic: Missing edge case
- **Current**: No handling for empty array
- **Suggested**: Add guard clause
- **Impact**: Potential runtime error

## Minor Issues (Nice to Have)

### 1. [File:Line] Naming: Unclear variable name
- **Current**: `d`
- **Suggested**: `createdDate`

### 2. [File:Line] Style: Inconsistent formatting
- **Current**: Mixed quotes
- **Suggested**: Use single quotes consistently

## Positive Feedback
- Clean separation of concerns in service layer
- Comprehensive input validation on DTOs
- Good test coverage for edge cases
- Excellent error messages

## Questions for Author
- What's the expected behavior when X happens?
- Should this support pagination for large datasets?
- Is the retry logic intentional or accidental?

## Test Coverage Assessment
- [ ] Happy path tested
- [x] Error cases tested
- [ ] Edge cases tested (missing empty array test)
- [x] Integration tests present

## Checklist
- [x] No security vulnerabilities
- [ ] Performance is acceptable (N+1 issue)
- [x] Code is readable
- [x] Tests are adequate
- [x] Documentation is present

Verdict Guidelines

Severity Definitions

Time Boxing

Quick Checks Before Submitting

• [ ] All critical issues have clear remediation
• [ ] Major issues explain the impact
• [ ] At least one positive comment included
• [ ] Questions are specific and answerable
• [ ] Verdict matches the issues found

Review Checklist

Comprehensive Review Checklist

Review Process

1. Context (5 min)

• [ ] Read PR description
• [ ] Understand the problem being solved
• [ ] Check linked issues/tickets
• [ ] Note expected changes

2. Structure (10 min)

• [ ] Review file organization
• [ ] Check architectural fit
• [ ] Verify design patterns used
• [ ] Note any breaking changes

3. Code Details (20 min)

• [ ] Review logic correctness
• [ ] Check edge cases
• [ ] Verify error handling
• [ ] Look for security issues
• [ ] Check performance concerns
• [ ] Review naming clarity

4. Tests (10 min)

• [ ] Verify test coverage
• [ ] Check test quality
• [ ] Look for edge case tests
• [ ] Ensure mocks are appropriate

5. Final Pass (5 min)

• [ ] Note positive patterns
• [ ] Prioritize feedback
• [ ] Write summary

Category Deep Dive

Design Questions

• Does this change belong in this file/module?
• Is the abstraction level appropriate?
• Could this be simpler?
• Does it follow existing patterns?
• Is it extensible without modification?

Logic Questions

• What happens with null/undefined inputs?
• Are boundary conditions handled?
• Could there be race conditions?
• Is the order of operations correct?
• Are all code paths tested?

Security Questions

• Is all user input validated?
• Are SQL queries parameterized?
• Is output properly encoded?
• Are secrets handled safely?
• Is authentication checked?
• Is authorization enforced?

Performance Questions

• Are there N+1 query patterns?
• Is data fetched efficiently?
• Are expensive operations cached?
• Could this cause memory leaks?
• Is pagination implemented?

Quick Reference

Spec Compliance Review

---

Two-Stage Review Architecture

                    ┌─────────────────────┐
                    │   Implementation    │
                    └──────────┬──────────┘
                               │
                    ┌──────────▼──────────┐
                    │  STAGE 1: Spec      │
                    │  Compliance Review  │
                    └──────────┬──────────┘
                               │
              ┌────────────────┴────────────────┐
              │                                  │
      ┌───────▼───────┐                ┌────────▼────────┐
      │   ✗ Issues    │                │   ✓ Compliant   │
      │     Found     │                │                 │
      └───────┬───────┘                └────────┬────────┘
              │                                  │
              │                        ┌────────▼────────┐
              │                        │  STAGE 2: Code  │
              │                        │  Quality Review │
              │                        └────────┬────────┘
              │                                  │
              │                    ┌─────────────┴─────────────┐
              │                    │                           │
              │            ┌───────▼───────┐         ┌────────▼────────┐
              │            │   ✗ Issues    │         │   ✓ Approved    │
              │            │     Found     │         │                 │
              │            └───────┬───────┘         └─────────────────┘
              │                    │
              └────────────────────┴────────────────────┐
                                                        │
                                              ┌─────────▼─────────┐
                                              │ Return to Author  │
                                              └───────────────────┘

Critical: Complete Stage 1 (spec compliance) BEFORE Stage 2 (code quality). Never review code quality for functionality that doesn't meet the specification.

---

Stage 1: Spec Compliance Review

Core Directive

"The implementer finished suspiciously quickly. Their report may be incomplete, inaccurate, or optimistic."

Approach every review with professional skepticism. Verify claims independently.

The Three Verification Categories

Category 1: Missing Requirements

Check for features that were requested but not implemented.

## Example Review Finding

**Missing Requirement:** Issue #42 requested "password must be at least 8 characters"

**Found in code:**

// No length validation present function validatePassword(password: string) { return password.length > 0; // Only checks non-empty }

**Status:** ❌ Incomplete - minimum length validation missing

Category 2: Unnecessary Additions

Check for scope creep and over-engineering.

## Example Review Finding

**Unnecessary Addition:** Added caching layer not in requirements

**Found in code:**

// Original requirement: "Fetch user by ID" // Actual implementation: class CachedUserRepository { // Not requested private cache = new Map(); private ttl = 60000;

async getUser(id: string) { if (this.cache.has(id)) { ... } // 50 lines of cache logic } }

**Status:** ⚠️ Scope creep - discuss before merging

Category 3: Interpretation Gaps

Check for misunderstandings of requirements.

## Example Review Finding

**Interpretation Gap:** "Sort by date" implemented as ascending

**Requirement stated:** "Sort by date" (ambiguous)

**Author implemented:** Oldest first (ascending)

**Expected:** Most recent first is typical UX pattern

**Status:** ❓ Clarify - which sort order was intended?

---

Why Order Matters

Stage 1 Must Come First

Separation of Concerns

• Stage 1 (Spec): Does it do the right thing?
• Stage 2 (Quality): Does it do the thing right?

Code quality review is meaningless if the code doesn't implement the correct functionality.

---

Spec Compliance Checklist

Before You Start

• [ ] Read the original issue/ticket completely
• [ ] Identify all explicit requirements
• [ ] Identify implicit requirements from context
• [ ] Note any acceptance criteria listed

During Review

Missing Requirements:

• [ ] All required features present
• [ ] Edge cases covered (empty, null, max values)
• [ ] Error handling as specified
• [ ] Happy path fully functional
• [ ] UI matches mockups/specs if provided

Unnecessary Additions:

• [ ] No unrequested features
• [ ] No speculative abstractions
• [ ] No premature optimizations
• [ ] Scope matches requirements exactly

Interpretation Gaps:

• [ ] Author's understanding matches spec
• [ ] Ambiguities resolved correctly
• [ ] Assumptions are documented and valid
• [ ] Behavior matches similar existing features

After Review

• [ ] Document all findings with file:line references
• [ ] Categorize as missing/unnecessary/interpretation
• [ ] Prioritize: blocking vs. non-blocking issues

---

Output Format

Compliant Result

## Spec Compliance Review: ✅ PASS

All requirements verified:
- ✅ User can upload profile image (req #1)
- ✅ Image resized to 200x200 (req #2)
- ✅ Invalid formats rejected with error message (req #3)
- ✅ Progress indicator during upload (req #4)

**Proceed to:** Code Quality Review

Issues Found

## Spec Compliance Review: ❌ ISSUES FOUND

### Missing Requirements

1. **Progress indicator not implemented** (req #4)
   - File: `ProfileUpload.tsx`
   - Expected: Progress bar during upload
   - Found: No progress indication

2. **Error messages not user-friendly** (req #3)
   - File: `ProfileUpload.tsx:45`
   - Expected: "Please upload a JPG or PNG file"
   - Found: "Error: INVALID_FORMAT"

### Unnecessary Additions

1. **Image cropping feature not requested**
   - File: `ImageCropper.tsx` (new file, 150 lines)
   - Impact: Adds complexity, delays delivery
   - Recommendation: Remove or create separate PR

**Action Required:** Address missing requirements before code quality review

---

Common Mistakes to Avoid

---

Content adapted from [obra/superpowers](https://github.com/obra/superpowers) by Jesse Vincent (@obra), MIT License.