code-review-patterns

What this skill does

# 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.

## Reference Files

Read only the references needed for the current review:

- `references/review-order-and-checkpoints.md` for concern-first reading order, review checkpoints, zero-finding halts, and re-review loops
- `references/security-review-checklist.md` for auth, input/output, secrets, network, storage, and dependency checks
- `references/code-review-heuristics.md` for maintainability, performance, hidden-failure, edge-case, sloppy-pattern, and UI quick scans

## Signal Quality Rule

**Flag ONLY when certain. False positives erode trust and waste remediation cycles.**

| Flag | Do NOT Flag |
|------|-------------|
| Will fail to compile/parse (syntax, type, import errors) | Style preferences not in project guidelines |
| Logic error producing wrong results for all inputs | Potential issues dependent on specific inputs/state |
| Clear guideline violation (quote the exact rule) | Subjective improvements or nitpicks |

## 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)

## Review Order

Before scanning code line-by-line, read
`references/review-order-and-checkpoints.md` and reconstruct the change by
concern, not by raw diff order.

## Security Review

For auth, data, network, storage, or externally reachable code, read
`references/security-review-checklist.md` before forming findings.

## LSP-Powered Code Analysis

**Use LSP for semantic understanding during reviews:**

| Task | LSP Tool | Why Better Than Grep |
|------|----------|---------------------|
| Find all callers of a function | `lspCallHierarchy(incoming)` | Finds actual calls, not string matches |
| Find all usages of a type/variable | `lspFindReferences` | Semantic, not text-based |
| Navigate to definition | `lspGotoDefinition` | Jumps to actual definition |
| Understand what function calls | `lspCallHierarchy(outgoing)` | Maps call chain |

**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.

## Review Heuristics

For performance, maintainability, edge cases, hidden failures, type-design
drift, or UI-specific checks, read `references/code-review-heuristics.md`.

**Wrong/Right — Silent optional chaining:**
```typescript
// WRONG: silently swallows null user
const name = user?.profile?.name ?? 'Unknown';

// RIGHT: log the gap, then degrade
const name = user?.profile?.name;
if (!name) {
  logger.warn('user profile missing name', { userId: user?.id });
}
return name ?? 'Unknown';
```

## Edge Case Classification Taxonomy

Reference checklist for systematic edge case scanning during review:

| Category | Examples | Detection |
|----------|----------|-----------|
| Missing else/default | Switch without default, if without else for nullable | Check switch/if exhaustiveness |
| Unguarded inputs | No validation on user input, missing null checks | Direct parameter use without validation |
| Off-by-one | Loop bounds, array indexing, pagination | Review all `<` vs `<=`, `array[length]` vs `array[length-1]` |
| Arithmetic edge cases | Division by zero, integer overflow, floating point | `/` operator without divisor validation |
| Implicit type coercion | `==` instead of `===`, string-to-number, truthy/falsy | `==` (not `===`), `+` with mixed types |
| Race conditions | Shared mutable state, async without locking | Shared variables modified in async paths |
| Timeout/retry gaps | No timeout on network calls, no retry exhaustion | fetch/axios without timeout config |

Use during Stage 2 Quality Review. Check only categories relevant to the changed code.

## Clarity Over Brevity

- Nested ternary `a ? b ? c : d : e` → Use if/else or switch
- Dense one-liner saving 2 lines → 3 clear lines over 1 clever line
- Chained `.map().filter().reduce()` with complex callbacks → Named intermediates

## Pattern Recognition Criteria

**During reviews, identify patterns worth documenting:**

| Criteria | What to Look For | Example |
|----------|------------------|---------|
| **Tribal** | Knowledge new devs wouldn't know | "All API responses use envelope structure" |
| **Opinionated** | Specific choices that could differ | "We use snake_case for DB, camelCase for JS" |
| **Unusual** | Not standard framework patterns | "Custom retry logic with backoff" |
| **Consistent** | Repeated across multiple files | "All services have health check endpoint" |

**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

## Severity Classification

| Severity | Definition | Action |
|----------|------------|--------|
| **CRITICAL** | Security vulnerability or blocks functionality | Must fix before merge |
| **MAJOR** | Affects functionality or significant quality issue | Should fix before merge |
| **MINOR** | Style issues, small improvements | Can merge, fix later |
| **NIT** | Purely stylistic preferences | Optional |

## Multi-Signal Review Methodology

**Each Stage 2 pass produces an independent signal. Score each dimension separately.**

**HARD signals** (any failure blocks approval):
- **Security:** One real vulnerability = dimension score 0
- **Correctness:** One logic error producing wrong output = dimension score 0

**SOFT signals** (concerns noted, don't block alone):
- **Performance:** Scaling concern without immediate impact
- **Maintainability:** Complex but functional code
- **UX/A11y:** Missing states but core flow works

**Aggregation rule:**
1. If ANY HARD signal = 0 → STATUS: CHANGES_REQUESTED (non-negotiable)
2. CONFIDENCE = min(HARD scores), reduced by max 10 if SOFT signals are low
3. Include per-signal breakdown in Router Handoff for targeted remediation

### Zero-Finding Halt

If a review produces ZERO findings across ALL dimensions (security, correctness, performance, maintainability, UX/A11y), the review MUST halt and re-examine. Zero findings in a non-trivial change is a signal of insufficient review depth, not perfect code. Action: Re-read every changed file. Re-run the heuristic scans in `references/code-review-heuristics.m