## Pre-Review Context Checklist

1. **Purpose Understanding**
   - [ ] Read PR/MR description thoroughly
   - [ ] Check linked issues/tickets/stories
   - [ ] Understand the business requirement
   - [ ] Identify the scope of changes

2. **Codebase Context**
   - [ ] Review project README and CONTRIBUTING.md
   - [ ] Identify coding standards and style guides
   - [ ] Check for existing linting/formatting rules
   - [ ] Note architectural patterns in use

3. **Change Scope Analysis**
   - [ ] List all modified files
   - [ ] Categorize by type (feature, bug fix, refactor, config)
   - [ ] Identify high-risk areas (auth, payments, data handling)
   - [ ] Note dependencies affected

## Correctness Checklist

### Functional Correctness
- [ ] Code accomplishes stated purpose
- [ ] Logic flow is correct and complete
- [ ] All requirements are addressed
- [ ] Acceptance criteria are met

### Edge Cases
- [ ] Null/undefined values handled
- [ ] Empty collections handled
- [ ] Boundary conditions tested
- [ ] Invalid input handled gracefully
- [ ] Maximum/minimum values considered

### Error Handling
- [ ] All exceptions caught appropriately
- [ ] Error messages are informative
- [ ] Errors don't expose sensitive information
- [ ] Recovery paths exist where appropriate
- [ ] Fallback behavior is defined

### Data Integrity
- [ ] Data types are correct
- [ ] Conversions are safe
- [ ] Precision is maintained (floats, dates)
- [ ] Character encoding handled correctly
- [ ] Data validation at boundaries

## Security Checklist

### Input Validation
- [ ] All user inputs validated
- [ ] Input length limits enforced
- [ ] Input type validation performed
- [ ] Whitelist validation preferred over blacklist
- [ ] File uploads validated (type, size, content)

### Injection Prevention
- [ ] SQL queries use parameterized statements
- [ ] NoSQL queries properly escaped
- [ ] Command injection prevented
- [ ] LDAP injection prevented
- [ ] XPath injection prevented

### Authentication & Authorization
- [ ] Authentication checked on all protected endpoints
- [ ] Authorization checked for resource access
- [ ] Session management is secure
- [ ] Password handling follows best practices
- [ ] Multi-factor authentication where required

### Data Protection
- [ ] Sensitive data encrypted at rest
- [ ] Sensitive data encrypted in transit
- [ ] No secrets in code or logs
- [ ] PII handled according to policy
- [ ] Data retention policies followed

### Web Security
- [ ] XSS prevention implemented
- [ ] CSRF protection in place
- [ ] Security headers configured
- [ ] CORS properly restricted
- [ ] Cookie security attributes set

### Dependency Security
- [ ] No known vulnerable dependencies
- [ ] Dependencies from trusted sources
- [ ] Lock files committed
- [ ] Minimal dependency footprint

## Performance Checklist

### Algorithmic Efficiency
- [ ] Time complexity is acceptable
- [ ] Space complexity is acceptable
- [ ] No unnecessary nested loops
- [ ] Appropriate data structures used
- [ ] Sorting/searching algorithms optimal

### Database Performance
- [ ] N+1 query problems avoided
- [ ] Appropriate indexes exist
- [ ] Queries are optimized
- [ ] Batch operations used where appropriate
- [ ] Connection pooling configured
- [ ] Query result caching considered

### Memory Management
- [ ] No memory leaks
- [ ] Large objects disposed properly
- [ ] Streams closed after use
- [ ] Buffers sized appropriately
- [ ] Lazy loading where beneficial

### Network Efficiency
- [ ] API calls minimized
- [ ] Request/response payloads optimized
- [ ] Pagination implemented for large datasets
- [ ] Caching headers utilized
- [ ] Compression enabled

### Concurrency
- [ ] Thread safety maintained
- [ ] Race conditions prevented
- [ ] Deadlocks avoided
- [ ] Async operations used appropriately
- [ ] Resource contention minimized

## Maintainability Checklist

### Code Readability
- [ ] Variable names are descriptive
- [ ] Function names describe behavior
- [ ] Code is self-documenting
- [ ] Complex logic has explanatory comments
- [ ] Magic numbers/strings extracted to constants
- [ ] Consistent formatting applied

### Code Structure
- [ ] Functions have single responsibility
- [ ] Classes are cohesive
- [ ] Methods are appropriately sized (<30 lines ideal)
- [ ] Nesting depth is reasonable (<4 levels)
- [ ] Cyclomatic complexity is acceptable (<10)

### DRY Principle
- [ ] No duplicated code blocks
- [ ] Common patterns extracted
- [ ] Utilities reused appropriately
- [ ] Configuration centralized

### SOLID Principles
- [ ] Single Responsibility followed
- [ ] Open/Closed principle considered
- [ ] Liskov Substitution maintained
- [ ] Interface Segregation applied
- [ ] Dependency Inversion used

### Documentation
- [ ] Public APIs documented
- [ ] Complex algorithms explained
- [ ] Configuration options documented
- [ ] README updated if needed
- [ ] CHANGELOG updated if needed

## Testing Checklist

### Test Coverage
- [ ] New code has tests
- [ ] Modified code has updated tests
- [ ] Happy path tested
- [ ] Edge cases tested
- [ ] Error cases tested
- [ ] Coverage meets project threshold

### Test Quality
- [ ] Tests are readable
- [ ] Tests have clear assertions
- [ ] Tests are independent
- [ ] Tests are deterministic (not flaky)
- [ ] Test data is appropriate

### Test Types Present
- [ ] Unit tests for business logic
- [ ] Integration tests for components
- [ ] API tests for endpoints
- [ ] E2E tests for critical paths
- [ ] Performance tests where needed

### Test Maintenance
- [ ] No commented-out tests
- [ ] No skipped tests without reason
- [ ] Test utilities maintained
- [ ] Mocks/stubs appropriate
- [ ] Test fixtures organized

## Architecture Checklist

### Design Patterns
- [ ] Appropriate patterns used
- [ ] Patterns implemented correctly
- [ ] No anti-patterns introduced
- [ ] Consistency with existing architecture

### Separation of Concerns
- [ ] Layers properly separated
- [ ] Business logic isolated
- [ ] Presentation logic separated
- [ ] Data access abstracted

### Dependencies
- [ ] Dependencies flow correctly
- [ ] No circular dependencies
- [ ] Dependency injection used
- [ ] Interfaces used appropriately
- [ ] Coupling minimized

### API Design
- [ ] Contracts are clear
- [ ] Breaking changes avoided/documented
- [ ] Versioning considered
- [ ] Error responses consistent

### Scalability
- [ ] Stateless where possible
- [ ] Horizontal scaling considered
- [ ] Resource limits defined
- [ ] Bottlenecks identified

## Severity Classification

### BLOCKING (Must Fix Before Merge)
Criteria:
- Security vulnerabilities
- Data loss potential
- System crashes or hangs
- Breaking production functionality
- Compliance violations

Format:
[BLOCKING] Issue description
- Why it's critical
- Suggested fix
- Impact if not fixed

### MAJOR (Should Fix Before Merge)
Criteria:
- Performance regressions
- Missing error handling for likely scenarios
- Significant maintainability issues
- Missing critical tests
- Design pattern violations

Format:
[MAJOR] Issue description
- Why it matters
- Suggested improvement
- Trade-offs to consider

### MINOR (Fix or Acknowledge)
Criteria:
- Code style inconsistencies
- Minor optimization opportunities
- Small readability improvements
- Minor test gaps
- Documentation improvements

Format:
[MINOR] Issue description
- Suggestion

### NITPICK (Optional Improvement)
Criteria:
- Personal preferences
- Micro-optimizations
- Stylistic alternatives
- Future considerations

Format:
[NIT] Comment

### PRAISE (Positive Feedback)
Criteria:
- Excellent solutions
- Good use of patterns
- Improved clarity
- Thorough testing

Format:
[PRAISE] Positive observation

# Code Review Report

## Summary
- **PR/MR:** [Title and ID]
- **Reviewer:** [Agent/Name]
- **Date:** [Date]
- **Overall Assessment:** [Approved/Changes Requested/Needs Discussion]

## Changes Overview
[Brief description of what the code does]

## Review Statistics
- Files reviewed: X
- Lines added: X
- Lines removed: X
- Blocking issues: X
- Major issues: X
- Minor issues: X

## Findings by Category

### Security
[List security-related findings]

### Performance
[List performance-related findings]

### Maintainability
[List maintainability-related findings]

### Testing
[List testing-related findings]

### Architecture
[List architecture-related findings]

## Blocking Issues (Must Fix)
1. [Issue with full details]
2. [Issue with full details]

## Major Issues (Should Fix)
1. [Issue with details]
2. [Issue with details]

## Minor Issues
1. [Brief description]
2. [Brief description]

## Positive Observations
- [What was done well]
- [Clever solutions noticed]

## Recommendations
1. [Actionable recommendation]
2. [Actionable recommendation]

## Decision
- [ ] Approved
- [ ] Approved with minor changes
- [ ] Request changes (blocking issues exist)
- [ ] Needs discussion

# Common Issues to Check

# 1. Mutable default arguments
def bad_func(items=[]):  # BUG: Shared mutable default
    pass

def good_func(items=None):  # CORRECT: None default
    items = items or []

# 2. Context managers
f = open('file.txt')  # BAD: Not using context manager
data = f.read()
f.close()

with open('file.txt') as f:  # GOOD: Context manager
    data = f.read()

# 3. Exception handling
except Exception:  # TOO BROAD
    pass

except (ValueError, TypeError) as e:  # SPECIFIC
    logger.error(f"Validation error: {e}")
    raise

# 4. Type hints
def process(data):  # MISSING TYPES
    pass

def process(data: dict[str, Any]) -> ProcessResult:  # TYPED
    pass

# 5. String formatting
msg = "Hello " + name + "!"  # BAD: Concatenation
msg = f"Hello {name}!"  # GOOD: f-string

# 6. List comprehension vs loop
result = []
for item in items:
    if item.valid:
        result.append(item.value)

result = [item.value for item in items if item.valid]  # BETTER

// Common Issues to Check

// 1. Null/undefined handling
user.name.toLowerCase();  // UNSAFE
user?.name?.toLowerCase();  // SAFE

// 2. Type assertions
const data = response as UserData;  // UNSAFE assertion
// Better: Use type guards
function isUserData(obj: unknown): obj is UserData {
    return obj !== null && typeof obj === 'object' && 'name' in obj;
}

// 3. Async/await errors
async function fetch() {
    doAsyncThing();  // MISSING await
    await doAsyncThing();  // CORRECT
}

// 4. Error handling in async
try {
    await riskyOperation();
} catch (e) {
    // Empty catch block - BAD
}

try {
    await riskyOperation();
} catch (e) {
    logger.error('Operation failed', { error: e });
    throw new ServiceError('Operation failed', { cause: e });
}

// 5. Strict equality
if (value == null) { }  // LOOSE equality
if (value === null || value === undefined) { }  // STRICT

// 6. Array methods
const found = items.find(i => i.id === id);
found.name;  // UNSAFE: find can return undefined
found?.name;  // SAFE: optional chaining

// 7. Import order and organization
import { z } from 'zod';
import React from 'react';
import { localThing } from './local';
import { Button } from '@/components';

// Should be organized: external, internal, local

// Common Issues to Check

// 1. Error handling
result, err := doSomething()
// Missing error check - BAD

result, err := doSomething()
if err != nil {
    return fmt.Errorf("doing something: %w", err)
}

// 2. Defer in loops
for _, file := range files {
    f, _ := os.Open(file)
    defer f.Close()  // BAD: Defers accumulate
}

for _, file := range files {
    func() {
        f, _ := os.Open(file)
        defer f.Close()
        // process file
    }()  // GOOD: Defer executes each iteration
}

// 3. Goroutine leaks
go func() {
    for {
        // No exit condition - LEAK
    }
}()

// 4. Race conditions
var counter int
go func() { counter++ }()  // RACE
go func() { counter++ }()

var counter atomic.Int64  // SAFE
go func() { counter.Add(1) }()

// 5. Channel handling
ch := make(chan int)
// Sender doesn't close - receivers may block forever

// 6. Nil checks
if user != nil && user.Profile != nil && user.Profile.Name != "" {
    // VERBOSE but safe
}

-- Common Issues to Check

-- 1. SQL Injection
-- BAD: String interpolation
query = f"SELECT * FROM users WHERE id = {user_id}"

-- GOOD: Parameterized query
query = "SELECT * FROM users WHERE id = $1"

-- 2. SELECT *
-- BAD: Fetches all columns
SELECT * FROM large_table;

-- GOOD: Specific columns
SELECT id, name, email FROM large_table;

-- 3. Missing indexes (check EXPLAIN output)
-- Ensure WHERE, JOIN, ORDER BY columns are indexed

-- 4. N+1 patterns
-- BAD: Query in loop
for user in users:
    orders = query("SELECT * FROM orders WHERE user_id = ?", user.id)

-- GOOD: Batch query
orders = query("SELECT * FROM orders WHERE user_id IN (?)", user_ids)

-- 5. Missing LIMIT
-- BAD: Unbounded result set
SELECT * FROM logs WHERE level = 'ERROR';

-- GOOD: Bounded
SELECT * FROM logs WHERE level = 'ERROR' LIMIT 1000;

-- 6. Transaction handling
-- Ensure write operations are in transactions
-- Ensure proper isolation levels

## For Security Issues
[BLOCKING] **Security: SQL Injection Vulnerability**

The current implementation directly interpolates user input into the query:
```python
query = f"SELECT * FROM users WHERE id = {user_id}"

query = "SELECT * FROM users WHERE id = %s"
cursor.execute(query, (user_id,))

for user in users:
    orders = db.query("SELECT * FROM orders WHERE user_id = ?", user.id)

user_ids = [u.id for u in users]
orders = db.query("SELECT * FROM orders WHERE user_id IN (?)", user_ids)
orders_by_user = group_by(orders, lambda o: o.user_id)

cache_expiry = 86400

CACHE_EXPIRY_SECONDS = 86400  # 24 hours
cache_expiry = CACHE_EXPIRY_SECONDS

class PaymentProcessor(Protocol):
    def process(self, amount: Decimal) -> PaymentResult: ...

## Automated Review Integration

### Pre-Review Automated Checks

Before manual review, ensure these automated checks pass:

```yaml
# GitHub Actions Example
name: PR Checks

on: [pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Lint
        run: npm run lint

  type-check:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Type Check
        run: npm run type-check

  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Test
        run: npm run test -- --coverage
      - name: Check Coverage
        run: |
          coverage=$(cat coverage/coverage-summary.json | jq '.total.lines.pct')
          if (( $(echo "$coverage < 80" | bc -l) )); then
            echo "Coverage below 80%: $coverage%"
            exit 1
          fi

  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Security Scan
        run: npm audit --audit-level=high

## PR Review Checklist

### Automated (CI/CD)
- [ ] Linting passes
- [ ] Type checking passes
- [ ] Unit tests pass
- [ ] Integration tests pass
- [ ] Coverage threshold met
- [ ] Security scan clean
- [ ] Build succeeds

### Manual Review (This Agent)
- [ ] Correctness verified
- [ ] Security reviewed
- [ ] Performance checked
- [ ] Maintainability assessed
- [ ] Tests reviewed
- [ ] Architecture evaluated
- [ ] Documentation updated