Code Review Skill

Reviews code through battle-tested engineering principles with comprehensive quality gates.

Usage

Tell me what to review:

• "Review src/auth.ts"
• "Review the changes in git diff HEAD~3"
• "Review this PR for merge readiness"

Review Process

1. Read the code — Understand what it does
2. Verify functionality — Check for bugs, edge cases, correctness
3. Assess test coverage — Ensure changed code has adequate tests
4. Evaluate test quality — Check tests follow best practices (not flaky)
5. Check guideline compliance — Verify AGENTS.md/CLAUDE.md adherence
6. Apply philosophy lenses — Check against engineering principles
7. Run QA checks — Execute lint, typecheck, tests (after fixes)
8. Output structured findings — Categorized, actionable feedback

---

1. Functionality Verification

Correctness Checklist

• Does the code do what it's supposed to do?
• Are all requirements/acceptance criteria met?
• Are edge cases handled? (null, empty, boundary values, overflow)
• Are error conditions handled gracefully?
• Is the happy path tested AND working?
• Are race conditions possible? (async, concurrent access)
• Is state managed correctly? (no stale data, proper initialization)

Bug Detection Signals

Signal	Question
Off-by-one	Loop bounds, array indices, comparisons (< vs <=)
Null/undefined	Can any value be null when accessed?
Type coercion	Implicit conversions causing bugs? (JS: "5" + 3)
Resource leaks	Files, connections, memory properly closed?
Infinite loops	Can loop conditions fail to terminate?
Integer overflow	Large numbers handled safely?
Security holes	SQL injection, XSS, path traversal, secrets exposed?

---

2. Test Coverage Assessment

Coverage Requirements

• New code: Must have tests covering primary functionality
• Bug fixes: Must have regression test proving fix
• Changed code: Tests should cover modified behavior
• Critical paths: Auth, payments, data mutations require high coverage

Coverage Analysis

Is the changed code tested? → NO → Block: Add tests
     ↓ YES
Are edge cases covered? → NO → Request edge case tests
     ↓ YES
Are error paths tested? → NO → Request error handling tests
     ↓ YES
Coverage adequate ✓

Google's Coverage Guidelines

• 60%: Acceptable minimum
• 75%: Commendable
• 90%: Exemplary
• Per-commit: 90%+ for changed lines is reasonable

Focus on what's NOT covered rather than hitting arbitrary numbers.

---

3. Test Quality (Non-Flaky Tests)

FIRST Principles for Good Tests

Principle	Meaning
Fast	Run quickly (milliseconds for unit tests)
Isolated	No dependencies on other tests or external state
Repeatable	Same result every time, any environment
Self-validating	Pass or fail, no manual interpretation
Timely	Written close to the code (ideally before - TDD)

Flaky Test Detection

Tests are flaky if they fail intermittently without code changes.

Common Causes:

• Time/date dependencies (use fixed/mocked time)
• Random data without seed control
• Shared mutable state between tests
• Network/external service calls
• Race conditions in async code
• File system dependencies
• Database state leakage
• Order-dependent tests

Test Quality Checklist

• Tests are deterministic (same input → same output)
• Tests clean up after themselves (no side effects)
• Tests don't depend on execution order
• Async operations properly awaited
• External dependencies mocked/stubbed
• Time-sensitive tests use controlled time
• Random values use seeded generators
• Tests have meaningful assertions (not just expect(true))
• Test names describe behavior being tested
• One logical assertion per test

Red Flags in Tests

• sleep() or arbitrary delays
• Commented-out assertions
• Empty catch blocks swallowing failures
• Tests that pass when logic is deleted (useless tests)
• expect(result).toBeDefined() without checking value
• Shared state between it() blocks
• Network calls without mocking

---

4. QA Checks

Before Approving, Verify:

1. Lint passes: npm run lint, eslint, etc.
2. Types check: tsc --noEmit, npm run typecheck, etc.
3. Tests pass: npm test, pytest, etc.
4. Build succeeds: npm run build, cargo build, etc.

Automated QA Flow

Run lint → FAIL → Must fix before merge
    ↓ PASS
Run typecheck → FAIL → Must fix before merge
    ↓ PASS
Run tests → FAIL → Must fix (unless pre-existing)
    ↓ PASS
Build → FAIL → Must fix before merge
    ↓ PASS
QA Passed ✓

---

5. Guidelines Compliance

AGENTS.md / CLAUDE.md Adherence

Check if the codebase has guidance files:

• AGENTS.md or CLAUDE.md in root or relevant directories
• Project-specific coding standards
• Architecture decisions and patterns

Compliance Checklist

• Follows documented code style
• Uses approved libraries/patterns
• Adheres to naming conventions
• Follows directory structure guidelines
• Respects forbidden patterns (if documented)
• Matches documented architectural decisions

---

6. Engineering Philosophy Lenses

Grug Brain Check (Complexity Demon Detection)

• Is this the simplest solution?
• Could a newcomer understand it in 30 seconds?
• Are there unnecessary abstractions?

Code Smell Detection

Smell	Question
Long Method	>50 lines? One sentence description?
Long Params	>3 parameters? Group them?
Feature Envy	Uses another class more than its own?
Shotgun Surgery	One change = many file edits?
Dead Code	Unreachable or unused?

Unix Philosophy Check

• Does each function do ONE thing well?
• Can components be tested in isolation?
• Are there side effects in "pure" functions?

DRY/KISS

• Is logic duplicated?
• Is there a simpler solution?

---

Output Format

## Summary
[One-line verdict: APPROVE / NEEDS WORK / RETHINK]

## QA Status
- Lint: ✓/✗
- Typecheck: ✓/✗
- Tests: ✓/✗ (X passed, Y failed)
- Build: ✓/✗

## Functionality Issues
[Bugs, incorrect behavior, edge cases]

## Test Coverage Gaps
[Untested code paths, missing tests]

## Test Quality Issues
[Flaky tests, bad patterns]

## Guidelines Violations
[AGENTS.md/CLAUDE.md non-compliance]

## Code Quality
[Design issues, maintainability]

## Nitpicks
[Minor suggestions]

## What's Good
[Acknowledge solid work]

Severity Levels

Level	Meaning	Action
🔴 Critical	Bugs, security, data loss, broken tests	Block merge
🟠 Important	Missing tests, design issues	Should fix
🟡 Suggestion	Could be better	Consider
🟢 Nitpick	Style, minor preference	Optional

Master Decision Tree

Does it work correctly? → NO → 🔴 Fix bugs first
     ↓ YES
Does it have tests? → NO → 🔴 Add tests
     ↓ YES
Are tests good (not flaky)? → NO → 🟠 Fix test quality
     ↓ YES
Does QA pass? → NO → 🔴 Fix QA failures
     ↓ YES
Follows guidelines? → NO → 🟠 Align with standards
     ↓ YES
Is it simple? → NO → 🟡 Consider simplifying
     ↓ YES
APPROVE ✓

What NOT to Do

• Don't nitpick formatting (use formatters)
• Don't demand unnecessary abstractions
• Don't block on style preferences
• Don't rewrite working code for elegance alone
• Don't ignore test failures as "probably flaky"