code-review

# Code Review

Perform a structured, multi-dimensional code review. Act as a senior engineer reviewing a colleague's work — be thorough but constructive.

## When to Offer This Workflow

**Trigger conditions:**
- User pastes code and asks for review or feedback
- User mentions "review", "check", "audit", "any issues", "bugs"
- User shares a pull request or diff
- User asks about code quality, security, or performance

**Initial approach:**
Before diving in, understand the context:
1. What language/framework is this?
2. What does this code do? (Read it first, don't ask unless unclear)
3. Is this a snippet or full module?
4. Any specific concerns the user mentioned?

## Review Process

### Phase 1: Understand Before Judging

Read the entire code before making any comments. Understand:
- The purpose and intent of the code
- The architecture and design patterns being used
- The constraints the author may be working under
- Whether this is production code, a prototype, or a learning exercise

This context matters. A quick prototype doesn't need the same scrutiny as a payment processing module.

### Phase 2: Systematic Review

Review across five dimensions, in priority order:

#### 1. Correctness (Highest Priority)
Does the code do what it's supposed to?

Check for:
- Logic errors and off-by-one mistakes
- Null/undefined/nil handling — what happens when data is missing?
- Edge cases: empty inputs, zero values, maximum values, concurrent access
- Error handling: are exceptions caught appropriately? Are errors swallowed silently?
- Race conditions in concurrent code
- Resource leaks (unclosed files, connections, streams)
- Type mismatches or implicit conversions that could cause bugs

#### 2. Security
Could this code be exploited?

Check for:
- **Injection**: SQL injection, XSS, command injection, path traversal
- **Authentication/Authorization**: Missing auth checks, privilege escalation
- **Data exposure**: Secrets in code, verbose error messages leaking internals, PII in logs
- **Input validation**: Trusting user input without sanitization
- **Cryptography**: Weak algorithms, hardcoded keys, improper random number generation
- **Dependencies**: Known vulnerable libraries, outdated packages
- **OWASP Top 10**: Systematically check against common vulnerability classes

#### 3. Performance
Will this code perform well under load?

Check for:
- **Algorithmic complexity**: O(n^2) where O(n) is possible, unnecessary nested loops
- **Database**: N+1 queries, missing indexes, full table scans, unbounded result sets
- **Memory**: Unnecessary allocations, loading entire files into memory, unbounded caches
- **I/O**: Synchronous calls that should be async, missing connection pooling
- **Caching**: Repeated expensive computations that could be cached
- **Batching**: Individual operations that could be batched

#### 4. Maintainability
Can another developer understand and modify this code?

Check for:
- **Naming**: Are variables, functions, and classes named clearly? Could a reader guess what they do?
- **Size**: Functions over 30 lines or classes over 300 lines usually need splitting
- **Single Responsibility**: Does each function/class do one thing well?
- **DRY**: Is logic duplicated? Could shared utilities reduce repetition?
- **Error messages**: Are they actionable? Do they help debugging?
- **Documentation**: Are complex algorithms or business rules explained?
- **Testability**: Could this code be unit tested easily? Are dependencies injectable?

#### 5. Style and Conventions (Lowest Priority)
Does the code follow project and language conventions?

Check for:
- Consistent formatting (indentation, spacing, line length)
- Idiomatic language usage (e.g., list comprehensions in Python, streams in Java)
- Naming conventions (camelCase vs snake_case per language norms)
- Import organization
- Consistent error handling patterns

### Phase 3: Prioritize and Report

## Reference Materials

- Load `severity-guide.md` to understand how to classify issue severity (Critical / Warning / Suggestion) with examples for each level
- Load `owasp-top-10.md` when doing security-focused reviews for a comprehensive checklist of vulnerability categories

## Output Format

Structure your review as follows:

### Critical Issues
Issues that must be fixed — bugs, security vulnerabilities, data loss risks.

For each issue:
- **Location**: File and line number(s)
- **What's wrong**: Clear explanation of the problem
- **Why it matters**: The real-world impact (data loss, security breach, crash)
- **Fix**: Concrete code suggestion

### Warnings
Issues that should be fixed — performance problems, potential bugs, poor error handling.

Same format as critical issues.

### Suggestions
Nice-to-have improvements — readability, style, minor refactors.

Keep these brief. Don't nitpick.

### What's Done Well
Acknowledge good patterns, clean abstractions, thorough error handling, or clever solutions. This matters — it reinforces good practices and shows you read the code carefully.

## Guidelines

- **Lead with the most important issues.** If there's a SQL injection vulnerability, that matters more than variable naming.
- **Be specific.** "This could be improved" is useless. "Line 42: this SQL query concatenates user input directly — use parameterized queries to prevent injection" is actionable.
- **Show, don't just tell.** When suggesting a fix, include a code snippet showing the improvement.
- **Explain the why.** Don't just say "use a Set instead of Array" — explain that membership checks are O(1) vs O(n), and it matters here because the array is checked inside a loop.
- **Consider the author's intent.** If code looks intentionally structured a certain way, ask about it before suggesting changes.
- **Scale your review.** A 10-line utility function doesn't need 50 comments. A payment processing module deserves deep scrutiny.
- **Don't flag things that are clearly intentional.** If someone uses `# type: ignore`, they probably have a reason.