MansoorMajeed/review

Code Review

You are reviewing code you did NOT write. You have no context about implementation decisions. This is intentional — it prevents the "marking your own homework" problem.

Setup

Identify the base branch and get the diff:

git log --oneline main..HEAD
git diff main...HEAD --stat
git diff main...HEAD

If there's a plan file referenced in recent commits or in .scratch/plans/, read it to understand the intended scope.

Review Checklist

Check 1: Scope

Does the diff match the stated goal?

• Scope creep: Are there unrelated changes? Files modified that don't connect to the goal?
• Missing pieces: Are stated goals not addressed? Incomplete implementations?
• Over-building: Is there code for features or edge cases that weren't requested?

Check 2: Critical Issues

Look for:

• SQL injection, command injection, XSS
• Race conditions or shared mutable state
• Unvalidated external input (user input, API responses)
• Hardcoded secrets, credentials, or API keys
• Missing error handling on external calls (network, file I/O, database)
• Resource leaks (unclosed connections, file handles)

Check 3: Tests

• Are the tests testing real behavior? (Not just that a function was called)
• Would these tests catch a regression? (If the code broke, would the test fail?)
• Are there obvious untested paths? (Error cases, edge cases, empty inputs)
• Are tests too tightly coupled to implementation? (Will they break on a refactor?)

Check 4: Simplicity

• Over-engineering? Abstractions that wrap a single implementation?
• Unnecessary configurability? Parameters nobody will change?
• Defensive code for impossible scenarios?
• Could any of this be simpler while achieving the same result?

Check 5: Assumptions

• What assumptions does this code make about its inputs?
• What assumptions does it make about the environment?
• Are these documented or validated at the boundary?

Check 6: Operational Safety

• Idempotency: is this safe to re-run?
• Graceful degradation: what happens when a dependency is down?
• On-call impact: does this change alert behavior or add noise?
• Incident resilience: would this break during a Sev 0?

Output

Severity levels

• [P1] — Must fix. Security issues, data loss risk, correctness bugs.
• [P2] — Should fix. Logic errors, missing error handling, test gaps, regressions.
• [P3] — Consider fixing. Style, naming, minor simplification opportunities.

Format for each finding

### N. [P1/P2/P3] Short description — `file:line`

Explain the problem. Show the problematic code if helpful.

**Fix:** What to do instead (with code suggestion if applicable).

Be direct and specific. Don't soften feedback. The goal is to catch problems, not to be polite.

Write the review file

Determine the current branch name:

git branch --show-current

Write findings to .scratch/reviews/YYYY-MM-DD-<branch>.md. This is the exchange point — the implementer reads this file using the address-review skill.

The file should contain:

1. A summary section (what was reviewed, branch, base)
2. All findings with the format above
3. A verdict: "Ship it", "Needs attention", or "Needs rework"
4. A "Human Reviewer Callouts" section for non-blocking items the human should be aware of (dependency changes, permission changes, etc.)

Also present the findings in conversation — the review file is for the implementer session, the conversation output is for the current session's user.