Code Review

Code Review

Structured code review that catches what matters. Not style nitpicks - real bugs, security holes, and logic errors.

Announce: "Using code-review to analyze these changes."

Quick Start (Self-Review)

Before committing your own code, check these 3 things:

1. Does it break existing tests? Run the full test suite. Not just your new tests.
2. Does it handle the error case? For every happy path, ask: "What if this fails?"
3. Does it leak secrets or PII? Search your diff for API keys, passwords, tokens, email addresses.

If all 3 pass, your code is ready for a proper review.

The Full System

Step 1: Understand the Context

Before reading a single line of code:

• What is this change supposed to do? (Read the PR description or commit message)
• What files are involved? (Get the scope)
• Are there unaddressed comments from previous reviewers?

# For PRs:
gh pr view <number> --json title,body,files
gh pr diff <number>

Step 2: Set Up Safe Review (PRs Only)

For reviewing other people's PRs, use a git worktree for isolation:

# Create isolated workspace
git worktree add .worktrees/pr-<number> -b review/pr-<number>
cd .worktrees/pr-<number>

# Check out the PR
gh pr checkout <number>

Why isolation? The PR may contain untrusted code. Never run code from a PR you haven't reviewed. A worktree keeps your working directory clean.

Step 3: Analyze the Changes

Read the diff file by file. For each file, check:

Correctness:

• Does the logic do what the description says?
• Are edge cases handled? (null, empty, zero, negative, overflow)
• Are error paths handled? (network failure, disk full, permission denied)
• Do types match? (especially at boundaries - API responses, user input)

Security:

• Input validation on user-provided data?
• SQL/NoSQL injection possible?
• XSS possible in rendered output?
• Auth checks on new endpoints?
• Secrets hardcoded anywhere?
• Unsafe deserialization?

Quality:

• Does the change follow existing patterns in the codebase?
• Are there obvious simplifications?
• Is there dead code or unused imports?
• Are variable names clear enough to understand without comments?

Step 4: Categorize Findings

Every finding gets a severity:

Each finding needs:

• File and line reference (src/auth.ts:45)
• What the issue is (specific, not vague)
• Why it matters (impact)
• Suggested fix (actionable, not "please improve")

Step 5: Write the Review

Structure your review:

## Review: [PR Title or Change Description]

### Summary
[1-2 sentences: what this change does and overall assessment]

### Critical / High
[List issues that must or should be fixed]
- `file:line` - [Issue]. [Why it matters]. Fix: [suggestion].

### Medium
[Issues worth addressing]
- `file:line` - [Issue]. Fix: [suggestion].

### Low
[Optional improvements]
- `file:line` - [Suggestion].

### Positive Notes
[What's done well - this matters for morale and learning]

### Verdict
[Approve / Request Changes / Comment]

Step 6: Clean Up (PRs Only)

# Return to main workspace
cd ../..
git worktree remove .worktrees/pr-<number>
git branch -D review/pr-<number>

When to Use

• Reviewing a pull request (yours or someone else's)
• Before committing code to a shared branch
• After implementing a feature, before marking it done
• When you suspect code quality issues in a section of the codebase

When NOT to Use

• Reviewing documentation-only changes (just read them)
• Reviewing auto-generated code (CI configs, lock files)
• When the user explicitly says "skip review"
• For exploratory or prototype code that will be rewritten

Delegation Pattern

Large PRs (500+ lines) benefit from delegation:

## 1. TASK
Review the diff of PR #[NUMBER] for bugs, security issues, and code quality.

## 2. EXPECTED OUTCOME
Categorized findings (Critical/High/Medium/Low) with file:line references and suggested fixes.

## 3. REQUIRED TOOLS
Read, Grep, Glob, Bash (gh pr view, gh pr diff only)

## 4. MUST DO
- Read every changed file
- Check for security vulnerabilities (OWASP top 10)
- Check for logic errors and unhandled edge cases
- Categorize findings by severity
- Include specific file:line references

## 5. MUST NOT DO
- Don't run any code from the PR
- Don't modify any files
- Don't approve or merge the PR
- Don't report style-only issues as High/Critical

## 6. CONTEXT
PR: [URL or number]
Description: [What the PR does]
Key files: [Most important files to focus on]

Agent: general-purpose | Model: Sonnet | Effort: High (12 turns)

Security Focus Areas

These are the highest-value things to check. A review that catches one of these pays for itself:

Anti-Patterns

With Starter System

If you have the Starter System installed:

• Use /remember to log review patterns specific to your codebase
• "Always check for X in Y files" becomes institutional knowledge
• Use /handoff to pass review context across sessions

With Kairn (Course)

When Kairn is configured, code review gains:

• Cross-project patterns - "This auth pattern was flagged in 3 other projects"
• Review history - tracks which types of issues your codebase tends to have
• Codebase-specific rules - learns your project's conventions over time

Without Kairn: review quality depends on the reviewer's memory. Still structured, still valuable, just not adaptive.