CONTEXT:
- Issue/PR: [link or number]
- Author: [who made the changes]
- Scope: [files/directories affected]
- Purpose: [what problem it solves]
- Dependencies: [any related PRs or changes]

Questions to Answer:

What problem does this solve?
Is this the right approach?
Are there alternative solutions?
What's the impact on existing code?

Phase 2: Review the Diff

Goal: Examine actual code changes

Steps:

Review files changed (start with most important)
Check for unexpected changes
Examine each diff hunk carefully
Note questions and concerns

Diff Review Template:

## Files Changed

### Critical Files
- `file1:line` - [concern or observation]
- `file2:line` - [concern or observation]

### Supporting Files
- `file3:line` - [concern or observation]

### Test Files
- `test_file:line` - [concern or observation]

## Observations
1. [observation with file:line reference]
2. [observation with file:line reference]

Phase 3: Quality Checks

Categories to Evaluate:

3.1 Correctness

CORRECTNESS:
- [ ] Logic is correct
- [ ] Handles edge cases
- [ ] Error handling is appropriate
- [ ] No off-by-one errors
- [ ] Null/None handling is proper
- [ ] Type conversions are safe

ISSUES:
- `file:line` - [description of issue]

3.2 Security

SECURITY:
- [ ] No SQL injection vulnerabilities
- [ ] Input validation is proper
- [ ] No hardcoded secrets
- [ ] Authentication/authorization checked
- [ ] No XSS vulnerabilities (if web)
- [ ] Sensitive data is protected
- [ ] Rate limiting considered
- [ ] Dependencies are safe

ISSUES:
- `file:line` - [description of security concern]

Security Patterns to Check:

// ❌ Bad: SQL injection risk
let query = format!("SELECT * FROM users WHERE id = {}", user_input);

// ✅ Good: Parameterized query
let query = "SELECT * FROM users WHERE id = ?1";
conn.query_row(query, params![user_input], |row| { ... });

// ❌ Bad: Hardcoded secret
let api_key = "sk-abc123...";

// ✅ Good: Environment variable
let api_key = std::env::var("API_KEY")?;

// ❌ Bad: No input validation
fn process_input(input: &str) { ... }

// ✅ Good: Validated input
fn process_input(input: &str) -> Result<()> {
    if input.len() > MAX_LENGTH {
        return Err(Error::InputTooLong);
    }
    // Additional validation...
    Ok(())
}

3.3 Performance

PERFORMANCE:
- [ ] No N+1 queries
- [ ] Efficient algorithms
- [ ] Proper use of caching
- [ ] No unnecessary allocations
- [ ] Async operations used correctly
- [ ] Database queries optimized

ISSUES:
- `file:line` - [description of performance concern]

Performance Patterns:

// ❌ Bad: N+1 query problem
for user in users {
    let posts = get_posts_for_user(user.id).await?;  // N queries!
}

// ✅ Good: Batch query
let user_ids: Vec<i64> = users.iter().map(|u| u.id).collect();
let posts = get_posts_for_users(&user_ids).await?;  // 1 query

// ❌ Bad: Unnecessary clone
let data = large_vec.clone();
process_data(&data);

// ✅ Good: Reference when possible
process_data(&large_vec);

// ❌ Bad: Inefficient string concatenation
let mut s = String::new();
for part in parts {
    s += part;  // O(n²) complexity
}

// ✅ Good: Use join or with_capacity
let s = parts.join("");
// or
let mut s = String::with_capacity(estimated_size);
for part in parts {
    s.push_str(part);
}

3.4 Maintainability

MAINTAINABILITY:
- [ ] Code is readable
- [ ] Functions are focused
- [ ] Names are descriptive
- [ ] No code duplication
- [ ] Proper abstraction levels
- [ ] Easy to modify
- [ ] Follows project conventions

ISSUES:
- `file:line` - [description of maintainability concern]

Maintainability Patterns:

// ❌ Bad: Magic numbers
if status == 200 { ... }

// ✅ Good: Named constants
const HTTP_OK: u16 = 200;
if status == HTTP_OK { ... }

// ❌ Bad: Long function doing multiple things
fn process_order(order: Order) -> Result<()> {
    // Validate order (20 lines)
    // Calculate pricing (30 lines)
    // Update inventory (25 lines)
    // Send notifications (15 lines)
    // Log everything (10 lines)
}

// ✅ Good: Focused functions
fn process_order(order: Order) -> Result<()> {
    validate_order(&order)?;
    let total = calculate_total(&order)?;
    update_inventory(&order)?;
    send_notifications(&order)?;
    log_order(&order, total)?;
    Ok(())
}

// ❌ Bad: Duplicated logic
fn process_user(u: &User) { /* duplicate validation */ }
fn process_admin(u: &Admin) { /* duplicate validation */ }

// ✅ Good: Extracted common logic
trait Validatable {
    fn validate(&self) -> Result<()>;
}

fn process_entity<T: Validatable>(entity: &T) -> Result<()> {
    entity.validate()?;
    // ...
}

3.5 Testing

TESTING:
- [ ] New code is tested
- [ ] Tests are meaningful
- [ ] Edge cases covered
- [ ] Tests are maintainable
- [ ] Mocks used appropriately
- [ ] Test names are descriptive

ISSUES:
- `file:line` - [description of testing concern]

Testing Patterns:

// ❌ Bad: Vague test name
#[test]
fn test_process() { ... }

// ✅ Good: Descriptive test name
#[test]
fn test_process_returns_error_when_input_is_empty() { ... }

// ❌ Bad: Testing implementation details
#[test]
fn test_internal_helper_function() { ... }

// ✅ Good: Testing public behavior
#[test]
fn test_public_api_returns_expected_result() { ... }

// ❌ Bad: Not testing edge cases
#[test]
fn test_happy_path() { ... }

// ✅ Good: Comprehensive coverage
#[test]
fn test_happy_path() { ... }

#[test]
fn test_empty_input() { ... }

#[test]
fn test_max_length_input() { ... }

#[test]
fn test_invalid_input_returns_error() { ... }

Phase 4: Provide Feedback

Goal: Communicate findings clearly and constructively

Feedback Categories:

🔴 MUST FIX: Critical issues that block merge
🟡 SHOULD FIX: Important improvements recommended
🟢 SUGGESTION: Nice-to-have improvements
💬 QUESTION: Clarification needed
ℹ️ NOTE: Information for awareness

Feedback Template:

## Review Summary

**Overall**: [Approve | Request Changes | Comment]
**Confidence**: [High | Medium | Low]

### 🔴 Must Fix
1. `file:line` - [issue description]
   **Why**: [explanation]
   **Suggestion**: [how to fix]

### 🟡 Should Fix
1. `file:line` - [issue description]
   **Why**: [explanation]
   **Suggestion**: [how to fix]

### 🟢 Suggestions
1. `file:line` - [improvement suggestion]
   **Benefit**: [what it improves]

### 💬 Questions
1. `file:line` - [question]
   **Context**: [why you're asking]

### ✅ What's Good
- [positive observation 1]
- [positive observation 2]

Writing Good Feedback:

# ❌ Bad: Vague and unhelpful
"This code is wrong."

# ✅ Good: Specific and actionable
"file:42 - This function doesn't handle the case where `users` is empty.
ACTUAL: Panics with index out of bounds
EXPECTED: Should return an empty vec or error
SUGGESTION: Add check: `if users.is_empty() { return Ok(vec![]); }`"

Specialized Review Checklists

Security Review Checklist

## Security Review

### Input Validation
- [ ] All user inputs are validated
- [ ] Input length limits enforced
- [ ] Input type checking
- [ ] Input sanitization for display

### Authentication & Authorization
- [ ] Auth checks are present
- [ ] Auth checks are correct
- [ ] Role-based access control verified
- [ ] Session management is secure

### Data Protection
- [ ] Sensitive data encrypted at rest
- [ ] Sensitive data encrypted in transit
- [ ] No secrets in logs
- [ ] No secrets in error messages

### Injection Prevention
- [ ] SQL queries use parameterized statements
- [ ] No command injection
- [ ] No path traversal vulnerabilities
- [ ] Proper output encoding

### Dependencies
- [ ] New dependencies are necessary
- [ ] New dependencies are trusted
- [ ] No known vulnerabilities in dependencies

Performance Review Checklist

## Performance Review

### Database
- [ ] Queries are optimized
- [ ] Indexes used appropriately
- [ ] N+1 queries avoided
- [ ] Transactions used correctly

### Memory
- [ ] No memory leaks
- [ ] Large allocations justified
- [ ] References used instead of clones
- [ ] Buffers sized appropriately

### Concurrency
- [ ] Async used correctly
- [ ] No race conditions
- [ ] Locks are necessary and minimal
- [ ] Deadlock potential analyzed

### Caching
- [ ] Caching strategy is appropriate
- [ ] Cache invalidation is correct
- [ ] Cache keys are well-designed

API Design Review Checklist

## API Design Review

### Consistency
- [ ] Naming follows conventions
- [ ] Parameter ordering is consistent
- [ ] Response format is consistent
- [ ] Error format is consistent

### Documentation
- [ ] All endpoints documented
- [ ] Parameters documented
- [ ] Response schema documented
- [ ] Error codes documented

### Versioning
- [ ] Breaking changes handled correctly
- [ ] Version in URL or header
- [ ] Backwards compatibility maintained

### Error Handling
- [ ] Errors are informative
- [ ] Errors are actionable
- [ ] Error codes are consistent
- [ ] HTTP status codes are correct

Review Workflow Integration

Pre-Commit Review

# Run before committing
cargo fmt -- --check
cargo clippy -- -D warnings
cargo test

# Review your own changes
git diff

PR Review Process

# Fetch PR branch
git fetch origin pull/123/head:pr-123
git checkout pr-123

# Run checks
cargo test
cargo clippy

# Review specific files
git diff main...HEAD -- path/to/file

Continuous Integration

Ensure CI runs:

Lint checks
Type checks
Test suite
Security scans
Performance benchmarks (if applicable)

Anti-Patterns to Avoid

❌ Don't:

Nitpick style preferences
Block PRs for minor improvements
Be vague or unhelpful
Focus only on finding faults
Review without understanding context

✅ Do:

Focus on correctness and maintainability
Balance thoroughness with pragmatism
Provide specific, actionable feedback
Acknowledge good practices
Understand the problem being solved
Consider the author's perspective

Integration with Kilo

This skill integrates with Kilo's workflow:

Evidence-Based: All feedback includes file:line references
Minimal Changes: Focus on what matters most
Constructive: Provide solutions, not just problems
Systematic: Follow structured review process

Use with other skills:

systematic-debugging: When review reveals bugs
test-driven-development: When suggesting new tests
documentation: When docs need updates