Code Review Patterns

Overview

Code reviews catch bugs before they ship. But reviewing code quality before functionality is backwards.

Core principle: First verify it works, THEN verify it’s good.

Quick Review Checklist (Reference Pattern)

For rapid reviews, check these 8 items:

• Code is simple and readable
• Functions and variables are well-named
• No duplicated code
• Proper error handling
• No exposed secrets or API keys
• Input validation implemented
• Good test coverage
• Performance considerations addressed

The Iron Law

NO CODE QUALITY REVIEW BEFORE SPEC COMPLIANCE

If you haven’t verified the code meets requirements, you cannot review code quality.

Two-Stage Review Process

Stage 1: Spec Compliance Review

Does it do what was asked?

1. Read the Requirements

   • What was requested?
   • What are the acceptance criteria?
   • What are the edge cases?

2. Trace the Implementation

   • Does the code implement each requirement?
   • Are all edge cases handled?
   • Does it match the spec exactly?

3. Test Functionality

   • Run the tests
   • Manual test if needed
   • Verify outputs match expectations

Gate: Only proceed to Stage 2 if Stage 1 passes.

Stage 2: Code Quality Review

Is it well-written?

Review in priority order:

1. Security – Vulnerabilities that could be exploited
2. Correctness – Logic errors, edge cases missed
3. Performance – Unnecessary slowness
4. Maintainability – Hard to understand or modify
5. UX – User experience issues (if UI involved)
6. Accessibility – A11y issues (if UI involved)

Security Review Checklist

Reference: OWASP Top 10 – Check against industry standard vulnerabilities.

Security Quick-Scan Commands

Run before any review:

# Check for hardcoded secrets
grep -rE "(api[_-]?key|password|secret|token)\s*[:=]" --include="*.ts" --include="*.js" src/

# Check for SQL injection risk
grep -rE "(query|exec)\s*\(" --include="*.ts" src/ | grep -v "parameterized"

# Check for dangerous patterns
grep -rE "(eval\(|innerHTML\s*=|dangerouslySetInnerHTML)" --include="*.ts" --include="*.tsx" src/

# Check for console.log (remove before production)
grep -rn "console\.log" --include="*.ts" --include="*.tsx" src/

LSP-Powered Code Analysis

Use LSP for semantic understanding during reviews:

Review Workflow with LSP:

1. localSearchCode → find symbol + get lineHint
2. lspGotoDefinition(lineHint=N) → understand implementation
3. lspFindReferences(lineHint=N) → check all usages for consistency
4. lspCallHierarchy(incoming) → verify callers handle changes

CRITICAL: Always get lineHint from localSearchCode first. Never guess line numbers.

Critical Security Patterns:

OWASP Top 10 Quick Reference:

1. Injection (SQL, Command, XSS)
2. Broken Authentication
3. Sensitive Data Exposure
4. XXE (XML External Entities)
5. Broken Access Control
6. Security Misconfiguration
7. Cross-Site Scripting (XSS)
8. Insecure Deserialization
9. Using Vulnerable Components
10. Insufficient Logging

Full security review: See OWASP Top 10

For each security issue found:

- [CRITICAL] SQL injection at `src/api/users.ts:45`
  - Problem: User input concatenated into query
  - Fix: Use parameterized query
  - Code: `db.query(\`SELECT * FROM users WHERE id = ?\`, [userId])`

Quality Review Checklist

Pattern Recognition Criteria

During reviews, identify patterns worth documenting:

If you spot these during review:

1. Note the pattern in review feedback
2. Include in your Memory Notes (Patterns section) – router will persist to patterns.md via Memory Update task
3. Flag inconsistencies from established patterns

Performance Review Checklist

UX Review Checklist (UI Code)

Accessibility Review Checklist (UI Code)

Severity Classification

Priority Output Format (Feedback Grouping)

Organize feedback by priority (from reference pattern):

## Code Review Feedback

### Critical (must fix before merge)
- [95] SQL injection at `src/api/users.ts:45`
  → Fix: Use parameterized query `db.query('SELECT...', [userId])`

### Warnings (should fix)
- [85] N+1 query at `src/services/posts.ts:23`
  → Fix: Batch query with WHERE IN clause

### Suggestions (consider improving)
- [70] Function `calc()` could be renamed to `calculateTotal()`
  → More descriptive naming

ALWAYS include specific examples of how to fix each issue. Don’t just say “this is wrong” – show the correct approach.

Red Flags – STOP and Re-review

If you find yourself:

• Reviewing code style before checking functionality
• Not running the tests
• Skipping the security checklist
• Giving generic feedback (“looks good”)
• Not providing file:line citations
• Not explaining WHY something is wrong
• Not providing fix recommendations

STOP. Start over with Stage 1.

Rationalization Prevention

Output Format

## Code Review: [PR Title/Component]

### Stage 1: Spec Compliance ✅/❌

**Requirements:**
- [x] Requirement 1 - implemented at `file:line`
- [x] Requirement 2 - implemented at `file:line`
- [ ] Requirement 3 - NOT IMPLEMENTED

**Tests:** PASS (24/24)

**Verdict:** [Meets spec / Missing requirements]

---

### Stage 2: Code Quality

**Security:**
- [CRITICAL] Issue at `file:line` - Fix: [recommendation]
- No issues found ✅

**Performance:**
- [MAJOR] N+1 query at `file:line` - Fix: Use batch query
- No issues found ✅

**Quality:**
- [MINOR] Unclear naming at `file:line` - Suggestion: rename to X
- No issues found ✅

**UX/A11y:** (if UI code)
- [MAJOR] Missing loading state - Fix: Add spinner
- No issues found ✅

---

### Summary

**Decision:** Approve / Request Changes

**Critical:** [count]
**Major:** [count]
**Minor:** [count]

**Required fixes before merge:**
1. [Most important fix]
2. [Second fix]

Review Loop Protocol

After requesting changes:

1. Wait for fixes – Developer addresses issues
2. Re-review – Check that fixes actually fix the issues
3. Verify no regressions – Run tests again
4. Approve or request more changes – Repeat if needed

Never approve without verifying fixes work.

Final Check

Before approving:

• Stage 1 complete (spec compliance verified)
• Stage 2 complete (all checklists reviewed)
• All critical/major issues addressed
• Tests pass
• No regressions introduced
• Evidence captured for each claim