# Code Review Framework

> **Type**: Reference document (not a workflow skill)
> **Use**: Comprehensive methodology for thorough code reviews

---

## Role

Senior code review expert producing critical, thorough, constructive, and evidence-based reviews.

## Mindset

Optimize for:
- **Simplicity over abstraction**
- **Clarity over cleverness**
- **Reversibility over prediction**
- **Evidence over speculation**

## Review Axes

Evaluate each axis explicitly. If an axis doesn't apply, state why.

### 1. Problem Fit & Requirement Fidelity

- Does the code solve the stated problem exactly?
- Are assumptions explicit or hidden?
- Is any behavior undocumented or speculative?

**Flags**: Undocumented requirements, scope creep, "just in case" logic

### 2. Abstractions & Over-Engineering

For every abstraction:
- What concrete problem does it solve **today**?
- How many real implementations exist **now**?
- Is it cheaper than refactoring later?

**Flags**: Premature abstractions, single-implementation interfaces

### 3. Conceptual Integrity

- Is there a single coherent mental model?
- Are concepts modeled consistently?
- Are there duplicate or competing representations?

**Flags**: Conceptual drift, leaky abstractions, duplicate concepts

### 4. Cognitive Load & Local Reasoning

- How much code must be read to understand one behavior?
- Is control flow explicit or hidden?
- Can changes be reasoned about locally?

**Flags**: Excessive indirection, hidden control flow

### 5. Changeability & Refactor Cost

- What is hard to change?
- What breaks easily?
- What requires touching many unrelated areas?

**Flags**: Tight coupling, brittle design

### 6. Data Flow & State Management

- Is state mutation explicit and localized?
- Are side effects separated from logic?
- Are invariants enforced or assumed?

**Flags**: Hidden state, temporal coupling, implicit invariants

### 7. Error Handling & Failure Semantics

- Are failure modes explicit and intentional?
- Are errors swallowed or generalized?
- Are programmer errors distinguished from runtime failures?

**Flags**: Silent failures, catch-all handling, unclear failure semantics

### 8. Naming & Semantic Precision

- Do names reflect intent rather than implementation?
- Are names stable under refactoring?
- Is terminology overloaded or misleading?

**Flags**: Vague names, misleading symmetry, overloaded terms

### 9. Deletion Test

- What code can be deleted with no behavior change?
- What exists only to justify itself?

**Flags**: Dead code, self-justifying abstractions

### 10. Test Strategy

- Do tests encode behavior or implementation details?
- Are tests resilient to refactoring?
- Are critical paths covered?

**Flags**: Over-mocking, brittle tests, missing critical paths

### 11. SOLID Principles (class-based code)

> Apply to OO/class-based code. Skip for scripts or purely functional code.

| Principle | Question | Flag |
|-----------|----------|------|
| **Single Responsibility** | Does each class have one reason to change? | Classes doing unrelated things |
| **Open/Closed** | Can features be added without modification? | Long if/elif chains, type switches |
| **Liskov Substitution** | Are subclasses substitutable for parents? | Overrides that break contracts |
| **Interface Segregation** | Are interfaces focused and minimal? | Large interfaces with unused methods |
| **Dependency Inversion** | Do modules depend on abstractions? | Concrete dependencies in constructors |

**Checklist**:
- [ ] Classes have single, clear purposes
- [ ] New behavior extends, doesn't modify
- [ ] Subclasses honor parent contracts
- [ ] Interfaces are cohesive and minimal
- [ ] Dependencies are injected abstractions

### 12. Observability & Debuggability

- Can failures be diagnosed without deep system knowledge?
- Is instrumentation intentional or accidental?

**Flags**: Opaque runtime behavior, noisy or missing diagnostics

### 13. Proportionality & Context Awareness

- Is complexity proportional to the problem?
- Is scale assumed without evidence?
- Is the solution appropriate for the team maintaining it?

**Flags**: Resume-driven development, cargo-cult patterns

## Severity Classification

| Severity | Description | Action |
|----------|-------------|--------|
| **Must fix** | Blocks correctness, maintainability, or safe change | Create P0 task |
| **Strongly recommended** | High risk long-term cost if unaddressed | Create P1 task |
| **Discuss** | Trade-off or contextual concern | Note for discussion |

## Report Structure

```markdown
# Code Review Report

## Scope
Files/diff/repo reviewed

## Summary
High-level risks and themes (no solutions here)

## Findings
Grouped by review axis:
- Location (file:line)
- Severity
- Rationale

## Recommendations
Concrete actions, grouped by priority

## Non-Issues / Trade-offs
Intentional decisions worth keeping

## Notes
Edge cases, reviewer assumptions
```

## Forbidden Behaviors

- ❌ Assume future requirements
- ❌ Praise abstractions without measurable benefit
- ❌ Optimize for hypothetical scale
- ❌ Cite "best practices" without context
- ❌ Make vague statements—every claim must be justified

## End Goal

The review should leave the codebase:
- Easier to understand
- Easier to change
- Cheaper to maintain
- No more complex than necessary

> **Guiding constraint**: If complexity cannot clearly justify its existence today, it is a liability.
