Requesting Code Review

Requesting Code Review

Dispatch code-reviewer subagent to catch issues before they cascade.

Core principle: Review early, review often.

When to Request Review

Mandatory:

• After each task in subagent-driven development
• After completing major feature
• Before merge to main

Optional but valuable:

• When stuck (fresh perspective)
• Before refactoring (baseline check)
• After fixing complex bug

Quick Start

1. Get git SHAs:

BASE_SHA=$(git rev-parse HEAD~1)  # or origin/main
HEAD_SHA=$(git rev-parse HEAD)

2. Dispatch code-reviewer subagent:

Use Task tool with code-reviewer type, fill template at Code Reviewer Template

Required placeholders:

• {WHAT_WAS_IMPLEMENTED} - What you just built
• {PLAN_OR_REQUIREMENTS} - What it should do
• {BASE_SHA} - Starting commit
• {HEAD_SHA} - Ending commit
• {DESCRIPTION} - Brief summary

3. Act on feedback:

See severity guidelines for details.

Integration with Workflows

Subagent-Driven Development:

• Review after EACH task
• Catch issues before they compound
• Fix before moving to next task

Executing Plans:

• Review after each batch (3 tasks)
• Get feedback, apply, continue

Ad-Hoc Development:

• Review before merge
• Review when stuck

Pushing Back on Reviews

If reviewer wrong:

• Push back with technical reasoning
• Show code/tests that prove it works
• Reference plan requirements
• Request clarification

See pushing back examples

Common Mistakes

Never:

• Skip review because "it's simple"
• Ignore Critical issues
• Proceed with unfixed Important issues
• Argue without technical justification

Always:

• Provide full context in review request
• Fix Critical issues immediately
• Document disagreements with technical justification
• Re-review after fixing Critical issues

Examples

Need examples? See Review Examples & Workflows for:

• Complete review output examples
• Good vs bad review requests
• Review workflows for different scenarios
• How to act on different severity levels
• When and how to push back

Need template? See Code Reviewer Template for the complete subagent dispatch template.

Code Review Agent

You are reviewing code changes for production readiness.

Your task: 1. Review {WHAT_WAS_IMPLEMENTED} 2. Compare against {PLAN_OR_REQUIREMENTS} 3. Check code quality, architecture, testing 4. Categorize issues by severity 5. Assess production readiness

What Was Implemented

{DESCRIPTION}

Requirements/Plan

{PLAN_REFERENCE}

Git Range to Review

Base: {BASE_SHA} Head: {HEAD_SHA}

git diff --stat {BASE_SHA}..{HEAD_SHA}
git diff {BASE_SHA}..{HEAD_SHA}

Review Checklist

Code Quality:

• Clean separation of concerns?
• Proper error handling?
• Type safety (if applicable)?
• DRY principle followed?
• Edge cases handled?

Architecture:

• Sound design decisions?
• Scalability considerations?
• Performance implications?
• Security concerns?

Testing:

• Tests actually test logic (not mocks)?
• Edge cases covered?
• Integration tests where needed?
• All tests passing?

Requirements:

• All plan requirements met?
• Implementation matches spec?
• No scope creep?
• Breaking changes documented?

Production Readiness:

• Migration strategy (if schema changes)?
• Backward compatibility considered?
• Documentation complete?
• No obvious bugs?

Output Format

Strengths

[What's well done? Be specific.]

Issues

Critical (Must Fix)

[Bugs, security issues, data loss risks, broken functionality]

Important (Should Fix)

[Architecture problems, missing features, poor error handling, test gaps]

Minor (Nice to Have)

[Code style, optimization opportunities, documentation improvements]

For each issue:

• File:line reference
• What's wrong
• Why it matters
• How to fix (if not obvious)

Recommendations

[Improvements for code quality, architecture, or process]

Assessment

Ready to merge? [Yes/No/With fixes]

Reasoning: [Technical assessment in 1-2 sentences]

Critical Rules

DO:

• Categorize by actual severity (not everything is Critical)
• Be specific (file:line, not vague)
• Explain WHY issues matter
• Acknowledge strengths
• Give clear verdict

DON'T:

• Say "looks good" without checking
• Mark nitpicks as Critical
• Give feedback on code you didn't review
• Be vague ("improve error handling")
• Avoid giving a clear verdict

Example Output

### Strengths
- Clean database schema with proper migrations (db.ts:15-42)
- Comprehensive test coverage (18 tests, all edge cases)
- Good error handling with fallbacks (summarizer.ts:85-92)

### Issues

#### Important
1. **Missing help text in CLI wrapper**
   - File: index-conversations:1-31
   - Issue: No --help flag, users won't discover --concurrency
   - Fix: Add --help case with usage examples

2. **Date validation missing**
   - File: search.ts:25-27
   - Issue: Invalid dates silently return no results
   - Fix: Validate ISO format, throw error with example

#### Minor
1. **Progress indicators**
   - File: indexer.ts:130
   - Issue: No "X of Y" counter for long operations
   - Impact: Users don't know how long to wait

### Recommendations
- Add progress reporting for user experience
- Consider config file for excluded projects (portability)

### Assessment

**Ready to merge: With fixes**

**Reasoning:** Core implementation is solid with good architecture and tests. Important issues (help text, date validation) are easily fixed and don't affect core functionality.

Code Review Examples

Complete Review Example

Context

Task 2 completed: Added verification and repair functions for conversation index

Git range: a7981ec..3df7661

What was implemented: Verification and repair functions for conversation index

Plan reference: Task 2 from docs/plans/deployment-plan.md

Good Review Output

### Strengths
- Clean database schema with proper migrations (db.ts:15-42)
- Comprehensive test coverage (18 tests, all edge cases)
- Good error handling with fallbacks (summarizer.ts:85-92)
- Real integration tests, not just mocks (test-integration.ts)
- Clear separation of concerns (verification vs repair logic)

### Issues

#### Important (Should Fix)
1. **Missing help text in CLI wrapper**
   - File: index-conversations:1-31
   - Issue: No --help flag, users won't discover --concurrency
   - Why it matters: Poor user experience, hidden features
   - Fix: Add --help case with usage examples showing all options

2. **Date validation missing**
   - File: search.ts:25-27
   - Issue: Invalid dates silently return no results
   - Why it matters: Silent failures confuse users
   - Fix: Validate ISO format, throw error with example format

3. **Progress indicators missing**
   - File: indexer.ts:130
   - Issue: No "X of Y" counter for long operations
   - Why it matters: Users don't know how long to wait
   - Fix: Add `console.log(\`Processing \${i+1}/\${total}\`)` in loop

#### Minor (Nice to Have)
1. **Magic number for reporting interval**
   - File: indexer.ts:145
   - Issue: Hardcoded 100 for progress reporting
   - Impact: Low, but config would be cleaner
   - Fix: Extract to constant `PROGRESS_INTERVAL = 100`

### Recommendations
- Add progress reporting for better user experience
- Consider config file for excluded projects (improves portability)
- Documentation looks good, but add troubleshooting section

### Assessment

**Ready to merge: With fixes**

**Reasoning:** Core implementation is solid with good architecture and comprehensive tests. Important issues (help text, date validation, progress indicators) are easily fixed in 10-15 minutes and don't affect core functionality. Minor issues can be addressed later.

Acting on This Review

Fix Important issues:

# Fix 1: Add help text
# Edit index-conversations, add help flag

# Fix 2: Add date validation
# Edit search.ts:25-27, add format check

# Fix 3: Add progress indicators
# Edit indexer.ts:130, add counter

# Test fixes
pytest tests/ -v

# Commit
git add .
git commit -m "fix: add help text, date validation, and progress indicators"

Continue to next task after fixes verified.

Good vs Bad Review Requests

❌ Bad Request: Vague

Can you review my code?

BASE_SHA: abc123
HEAD_SHA: def456

Problems:

• No context on what was implemented
• No requirements to check against
• No description of changes
• Reviewer has to reverse-engineer everything

✅ Good Request: Specific

Review Task 3: User Authentication Implementation

WHAT_WAS_IMPLEMENTED:
- JWT-based authentication middleware
- Login/logout endpoints
- Password hashing with bcrypt
- Session management

PLAN_OR_REQUIREMENTS:
See Task 3 in docs/plans/2025-01-15-user-auth.md
Key requirements:
- Secure password storage (bcrypt)
- JWT tokens with 24hr expiration
- Logout invalidates token
- All endpoints require auth except /login

BASE_SHA: a7981ec
HEAD_SHA: 3df7661

DESCRIPTION:
Added authentication system with JWT tokens. Implemented password hashing,
login/logout endpoints, and auth middleware. All tests passing.

Why it's good:

• Clear context on what was built
• Specific requirements to verify
• Git range for diff
• Brief summary of changes

Review Workflow Examples

Example 1: Subagent-Driven Development (After Each Task)

[Task 2 completed: Email validation]

PM: Let me request code review before proceeding.

BASE_SHA=$(git log --oneline | grep "Task 1" | head -1 | awk '{print $1}')
HEAD_SHA=$(git rev-parse HEAD)

[Dispatch code-reviewer subagent]
  WHAT_WAS_IMPLEMENTED: Email validation in User model
  PLAN_OR_REQUIREMENTS: Task 2 from docs/plans/user-system-plan.md
  BASE_SHA: abc123
  HEAD_SHA: def456
  DESCRIPTION: Added validate_email() utility and integrated into User model

[Subagent returns]:
  Strengths: Clean validator, good tests
  Issues:
    Important: Missing edge case for emails with + signs
    Minor: Could extract regex to constant
  Assessment: Fix + sign handling, then proceed

PM: [Fix + sign edge case]
[Add test for alice+spam@example.com]
[Verify all tests pass]
[Commit fix]

PM: Ready to proceed to Task 3

Example 2: Before Merging Major Feature

[Feature branch: user-authentication complete]

PM: Let me get a final review before merging to main.

BASE_SHA=$(git merge-base origin/main HEAD)
HEAD_SHA=$(git rev-parse HEAD)

[Dispatch code-reviewer subagent]
  WHAT_WAS_IMPLEMENTED: Complete user authentication system
  PLAN_OR_REQUIREMENTS: docs/plans/2025-01-15-user-auth.md (all tasks)
  BASE_SHA: a7981ec (merge-base with main)
  HEAD_SHA: 3df7661
  DESCRIPTION: Full auth system: registration, login, JWT, password hashing, session management

[Subagent returns]:
  Strengths: Complete implementation, excellent test coverage, secure
  Issues:
    Critical: JWT secret in code, should use environment variable
    Important: No rate limiting on login endpoint
    Minor: Could add password strength requirements
  Assessment: NOT ready to merge - fix Critical immediately

PM: [Move JWT secret to environment variable]
[Add .env.example with JWT_SECRET]
[Update config to read from env]
[Verify tests still pass]
[Commit: "fix: move JWT secret to environment variable"]

PM: Rate limiting is Important but can be separate task.
PM: Password strength can be future enhancement.

[Request review again with fix]
  Issues: None
  Assessment: Ready to merge

PM: [Merge to main]

Example 3: When Stuck (Fresh Perspective)

[Debugging weird test failure for 2 hours]

PM: I'm stuck. Let me get a code review for fresh eyes.

BASE_SHA=$(git rev-parse HEAD~3)  # Before I started debugging
HEAD_SHA=$(git rev-parse HEAD)

[Dispatch code-reviewer subagent]
  WHAT_WAS_IMPLEMENTED: Attempted fix for test_user_creation flakiness
  PLAN_OR_REQUIREMENTS: Test should pass consistently
  BASE_SHA: abc123
  HEAD_SHA: def456
  DESCRIPTION: Test fails intermittently, added logging, tried various fixes

[Subagent returns]:
  Issue found: test_user_creation:15 - race condition
  The test creates user and immediately queries, but database write is async
  Fix: Add await or use synchronous test database

PM: Ah! That's it. The async write wasn't flushing.
[Add await db.flush() before query]
[Test passes consistently]

Common Review Scenarios

Scenario: All Critical Issues

### Issues

#### Critical (Must Fix)
1. **SQL injection vulnerability**
   - File: users.py:45
   - Issue: Raw string interpolation in query
   - Fix: Use parameterized query

2. **Plaintext password storage**
   - File: models.py:23
   - Issue: Passwords stored without hashing
   - Fix: Use bcrypt before saving

### Assessment

**Ready to merge: NO**

**Reasoning:** Critical security issues must be fixed before any deployment.

Action: Fix immediately, don't proceed.

Scenario: Mix of Severities

### Issues

#### Important (Should Fix)
1. **Missing error handling**
   - File: api.py:67
   - Issue: Network call can throw, not caught
   - Fix: Wrap in try/except, return 500

#### Minor (Nice to Have)
1. **Magic number**
   - File: config.py:12
   - Issue: Hardcoded timeout value
   - Fix: Extract to named constant

### Assessment

**Ready to merge: With fixes**

**Reasoning:** Error handling is important but localized fix. Magic number can be addressed later.

Action: Fix Important, note Minor for future, proceed.

Scenario: Clean Implementation

### Strengths
- Excellent test coverage (95%)
- Clean separation of concerns
- Good error handling throughout
- Well-documented edge cases

### Issues

#### Minor (Nice to Have)
1. **Could add type hints**
   - File: helpers.py
   - Impact: Very low, code is clear
   - Fix: Add -> str type hints

### Recommendations
- Consider adding performance tests for large datasets
- Documentation is great, maybe add architecture diagram

### Assessment

**Ready to merge: YES**

**Reasoning:** Implementation is production-ready. Minor improvements can be separate tasks.

Action: Merge, create tickets for nice-to-haves.

Severity Guidelines

Critical (Must Fix)

• Security vulnerabilities
• Data loss risks
• Broken core functionality
• Production crashes
• Memory leaks

Action: Stop everything, fix immediately, re-review.

Important (Should Fix)

• Missing features from requirements
• Poor error handling
• Architectural issues
• Test coverage gaps
• Performance problems

Action: Fix before proceeding to next major task.

Minor (Nice to Have)

• Code style inconsistencies
• Optimization opportunities
• Documentation improvements
• Magic numbers
• Variable naming

Action: Note for later, proceed if time-constrained.

Pushing Back on Reviews

Sometimes reviewers are wrong. Push back with technical reasoning:

Example: Reviewer Wrong About Performance

Reviewer: "This loop is inefficient, use map() instead"

Your response:
"I tested both approaches with 10k items:
- Current loop: 45ms
- Map approach: 43ms (2ms faster)

The readability gain of explicit loop outweighs 2ms difference.
Performance requirement is <100ms, we're at 45ms.
Keeping current implementation."

Example: Reviewer Misunderstood Requirements

Reviewer: "Missing validation for phone numbers"

Your response:
"Plan requirement (docs/plans/user-plan.md:45):
'Phone number is optional and will be validated in Phase 2'

Current task only requires email validation.
Phone validation is tracked in Task 7."

Example: Reviewer Right, You Missed Something

Reviewer: "No test for empty email"

Your response:
"You're right, I missed that edge case.
Adding test now:

def test_create_user_empty_email():
    with pytest.raises(ValueError, match="Email required"):
        create_user("alice", "")

Fixed and committed."

Red Flags in Your Own Code

Before requesting review, self-check:

• [ ] All tests passing locally
• [ ] No commented-out code
• [ ] No debug print statements
• [ ] No hardcoded secrets or credentials
• [ ] All requirements from plan implemented
• [ ] Error handling in place
• [ ] Edge cases tested
• [ ] Commit messages clear

If any ❌, fix before requesting review.