Code Review
When to Use
- Reviewing pull requests before merge
- Auditing code changes after a feature branch is complete
- Self-reviewing your own changes before committing
- Investigating code quality concerns raised by teammates
Prerequisites
- Changes are committed or staged in git
- Access to the repository and its test suite
- Understanding of the project's coding standards
Workflow
1. Understand the Scope
# See what files changed
git --no-pager diff --stat main...HEAD
# Get a summary of the diff
git --no-pager diff main...HEAD --shortstat
For PR reviews, use the code-review agent type which is purpose-built for this:
task agent_type: "code-review"
prompt: "Review the staged changes in this repository"
2. Review Checklist
Evaluate each changed file against these categories:
| Priority |
Category |
What to Check |
| 🔴 Critical |
Correctness |
Logic errors, off-by-one, null handling, race conditions |
| 🔴 Critical |
Security |
Injection, auth bypass, secret exposure, unsafe deserialization |
| 🟡 Important |
Error handling |
Missing try/catch, unhandled promise rejections, error propagation |
| 🟡 Important |
Edge cases |
Empty inputs, large inputs, unicode, concurrent access |
| 🟢 Minor |
Performance |
N+1 queries, unnecessary re-renders, missing indexes |
| 🟢 Minor |
Maintainability |
Dead code, unclear naming, missing types |
3. Investigate Suspicious Patterns
# Find TODO/FIXME/HACK left in changed files
git --no-pager diff main...HEAD | Select-String "TODO|FIXME|HACK"
# Check for console.log or debug statements
git --no-pager diff main...HEAD | Select-String "console\.log|debugger|print\("
4. Verify Tests
# Ensure tests exist for changed source files
git --no-pager diff --name-only main...HEAD | Select-String "\.(ts|js|py|go)$"
# Run the test suite
npm test 2>&1 | Select-Object -Last 20
5. Severity Levels for Findings
- 🔴 Blocker — Must fix before merge (bugs, security issues, data loss)
- 🟡 Warning — Should fix, but not a merge blocker (error handling gaps, missing tests)
- 🟢 Suggestion — Nice to have (naming, style, minor optimization)
- 💡 Nitpick — Optional, low priority (formatting, comment wording)
6. Check for Breaking Changes
# Look for changed function signatures or removed exports
git --no-pager diff main...HEAD -- "*.ts" | Select-String "^[-+].*(export|public|function)"
# Check for changed API routes or database schemas
grep -rn "router\.\|app\.\|migration" --include="*.ts" src/
Examples
Quick Self-Review Before Commit
# Stage changes and review
git add -A
git --no-pager diff --cached --stat
git --no-pager diff --cached
PR Review with code-review Agent
The code-review agent provides high signal-to-noise analysis — it only surfaces
issues that genuinely matter (bugs, security, logic errors), never style or formatting.
task agent_type: "code-review"
prompt: "Review changes between main and the current branch. Focus on correctness and security."
Common Rationalizations
| Rationalization |
Reality |
| "LGTM, it's a simple change" |
Simple-looking changes can break implicit dependencies. (Hyrum's Law) |
| "Tests pass, so it's fine" |
Tests only verify what's explicitly tested. Reviews catch what tests don't. |
| "I trust the author" |
Reviews aren't distrust — they're a second pair of eyes. Authors miss their own bugs. |
| "It's a big PR, I'll skim it" |
Large PRs need more thorough review. The size itself is the first piece of feedback. |
| "Security is for the security team later" |
Cost to fix in development < cost to fix in production × 100. |
Reviewing AI-Generated Code
AI-generated code requires the same review standard as human-written code — often a stricter one.
Core principle: Treat the AI as a junior engineer. The first output is a draft, not a finished product.
Verify, Don't Trust.
Review agent output exactly as you would review a code submission from a new contributor.
LLM-Specific Checklist
| Risk |
What to look for |
| Plausible but wrong logic |
Code that looks correct but contains subtle semantic errors — AI optimizes for appearance |
| Hallucinated APIs |
Method names, library versions, or options that don't exist |
| Missing edge cases |
AI often generates the happy path only; check null, empty, concurrent, and boundary conditions |
| Scope creep |
AI may change code beyond what was asked — diff carefully |
| Test quality |
AI-written tests often assert the implementation, not the behavior; verify they would actually catch regressions |
| Security assumptions |
AI may apply patterns from its training data that are outdated or contextually wrong |
Quality Gate Before Merging AI Output
Red Flags
- "LGTM" approval within 2 minutes of a 500-line PR
- All review comments are style/formatting related (no logic review)
- No findings on a diff that touches authentication or payment code
- "I wrote this code, review not needed"
- Business logic added without corresponding tests
Verification
Tips
- Review tests first — they document the intended behavior
- Read the PR description/issue before the code to understand intent
- Check the boundaries between changed and unchanged code
- For large PRs, review file-by-file using
view tool rather than reading raw diffs
- Use
explore agent to understand unfamiliar code paths before commenting
- If a change is too large to review effectively, that itself is feedback worth giving
1---2name: code-review3description: Use when reviewing code changes for quality, correctness, and security — runs a structured checklist with severity-rated findings4---56# Code Review78## When to Use910- Reviewing pull requests before merge11- Auditing code changes after a feature branch is complete12- Self-reviewing your own changes before committing13- Investigating code quality concerns raised by teammates1415## Prerequisites1617- Changes are committed or staged in git18- Access to the repository and its test suite19- Understanding of the project's coding standards2021## Workflow2223### 1. Understand the Scope2425```powershell26# See what files changed27git --no-pager diff --stat main...HEAD2829# Get a summary of the diff30git --no-pager diff main...HEAD --shortstat31```3233For PR reviews, use the `code-review` agent type which is purpose-built for this:3435```text36task agent_type: "code-review"37prompt: "Review the staged changes in this repository"38```3940### 2. Review Checklist4142Evaluate each changed file against these categories:4344| Priority | Category | What to Check |45|----------|----------|---------------|46| 🔴 Critical | **Correctness** | Logic errors, off-by-one, null handling, race conditions |47| 🔴 Critical | **Security** | Injection, auth bypass, secret exposure, unsafe deserialization |48| 🟡 Important | **Error handling** | Missing try/catch, unhandled promise rejections, error propagation |49| 🟡 Important | **Edge cases** | Empty inputs, large inputs, unicode, concurrent access |50| 🟢 Minor | **Performance** | N+1 queries, unnecessary re-renders, missing indexes |51| 🟢 Minor | **Maintainability** | Dead code, unclear naming, missing types |5253### 3. Investigate Suspicious Patterns5455```powershell56# Find TODO/FIXME/HACK left in changed files57git --no-pager diff main...HEAD | Select-String "TODO|FIXME|HACK"5859# Check for console.log or debug statements60git --no-pager diff main...HEAD | Select-String "console\.log|debugger|print\("61```6263### 4. Verify Tests6465```powershell66# Ensure tests exist for changed source files67git --no-pager diff --name-only main...HEAD | Select-String "\.(ts|js|py|go)$"6869# Run the test suite70npm test 2>&1 | Select-Object -Last 2071```7273### 5. Severity Levels for Findings7475- **🔴 Blocker** — Must fix before merge (bugs, security issues, data loss)76- **🟡 Warning** — Should fix, but not a merge blocker (error handling gaps, missing tests)77- **🟢 Suggestion** — Nice to have (naming, style, minor optimization)78- **💡 Nitpick** — Optional, low priority (formatting, comment wording)7980### 6. Check for Breaking Changes8182```powershell83# Look for changed function signatures or removed exports84git --no-pager diff main...HEAD -- "*.ts" | Select-String "^[-+].*(export|public|function)"8586# Check for changed API routes or database schemas87grep -rn "router\.\|app\.\|migration" --include="*.ts" src/88```8990## Examples9192### Quick Self-Review Before Commit9394```powershell95# Stage changes and review96git add -A97git --no-pager diff --cached --stat98git --no-pager diff --cached99```100101### PR Review with code-review Agent102103The `code-review` agent provides high signal-to-noise analysis — it only surfaces104issues that genuinely matter (bugs, security, logic errors), never style or formatting.105106```text107task agent_type: "code-review"108prompt: "Review changes between main and the current branch. Focus on correctness and security."109```110111## Common Rationalizations112113| Rationalization | Reality |114|----------------|---------|115| "LGTM, it's a simple change" | Simple-looking changes can break implicit dependencies. (Hyrum's Law) |116| "Tests pass, so it's fine" | Tests only verify what's explicitly tested. Reviews catch what tests don't. |117| "I trust the author" | Reviews aren't distrust — they're a second pair of eyes. Authors miss their own bugs. |118| "It's a big PR, I'll skim it" | Large PRs need more thorough review. The size itself is the first piece of feedback. |119| "Security is for the security team later" | Cost to fix in development < cost to fix in production × 100. |120121## Reviewing AI-Generated Code122123AI-generated code requires the same review standard as human-written code — often a stricter one.124125**Core principle**: Treat the AI as a junior engineer. The first output is a draft, not a finished product.126127```text128Verify, Don't Trust.129Review agent output exactly as you would review a code submission from a new contributor.130```131132### LLM-Specific Checklist133134| Risk | What to look for |135|------|-----------------|136| **Plausible but wrong logic** | Code that looks correct but contains subtle semantic errors — AI optimizes for appearance |137| **Hallucinated APIs** | Method names, library versions, or options that don't exist |138| **Missing edge cases** | AI often generates the happy path only; check null, empty, concurrent, and boundary conditions |139| **Scope creep** | AI may change code beyond what was asked — diff carefully |140| **Test quality** | AI-written tests often assert the implementation, not the behavior; verify they would actually catch regressions |141| **Security assumptions** | AI may apply patterns from its training data that are outdated or contextually wrong |142143### Quality Gate Before Merging AI Output144145- [ ] Linter passes with no suppressions added146- [ ] Type checker passes147- [ ] Tests pass, and at minimum one test was in a failing state before the fix148- [ ] Manual smoke test on the core path149- [ ] Edge cases (null, empty, large input, concurrent access) are handled150- [ ] No new technical debt introduced silently (TODOs, skipped tests, magic values)151152## Red Flags153154- "LGTM" approval within 2 minutes of a 500-line PR155- All review comments are style/formatting related (no logic review)156- No findings on a diff that touches authentication or payment code157- "I wrote this code, review not needed"158- Business logic added without corresponding tests159160## Verification161162- [ ] Actually opened every changed file (didn't just read `git diff --stat`)163- [ ] 🔴 Critical findings are explicitly marked as Blockers164- [ ] Auth/authorization code was reviewed from a security perspective165- [ ] New logic has corresponding tests166- [ ] Reviewed the full `git --no-pager diff main...HEAD`167- [ ] If any code was AI-generated, the Quality Gate Before Merging AI Output checklist was applied168169## Tips170171- Review tests first — they document the intended behavior172- Read the PR description/issue before the code to understand intent173- Check the **boundaries** between changed and unchanged code174- For large PRs, review file-by-file using `view` tool rather than reading raw diffs175- Use `explore` agent to understand unfamiliar code paths before commenting176- If a change is too large to review effectively, that itself is feedback worth giving
drvoss