code-review

Code Review

When to use this skill

Use this skill when the user needs to:

• Review code for correctness, bugs, or logic errors
• Audit code for security vulnerabilities
• Check for performance issues or anti-patterns
• Review a pull request or diff
• Get feedback on code style and maintainability
• Validate error handling and edge cases

Review Process

Follow this checklist for every review. Check categories in order of severity.

1. Security (Critical)

• [ ] SQL injection: All queries use parameterized statements, never string concatenation
• [ ] XSS: User input is escaped/sanitized before rendering in HTML
• [ ] Authentication: Every endpoint that needs auth has it; tokens are validated properly
• [ ] Authorization: Users can only access their own resources; IDOR checks are in place
• [ ] Secrets: No hardcoded passwords, API keys, or tokens in source code
• [ ] Input validation: All external input is validated at system boundaries
• [ ] Error messages: Errors don't leak internal details (stack traces, DB schemas, file paths)
• [ ] Dependencies: No known vulnerable versions (check CVEs for major deps)

2. Correctness (High)

• [ ] Logic errors: Conditions, loops, and branches produce correct results for all inputs
• [ ] Edge cases: Empty inputs, null values, boundary conditions, zero-length collections
• [ ] Off-by-one: Array indices, loop bounds, pagination offsets
• [ ] Race conditions: Shared state in concurrent code is properly synchronized
• [ ] Error handling: Errors are caught at the right level; resources are cleaned up (files, connections)
• [ ] Type safety: No implicit type coercions that could cause silent failures

3. Performance (Medium)

• [ ] N+1 queries: Database queries inside loops; should be batched or joined
• [ ] Unnecessary computation: Work done inside loops that could be hoisted out
• [ ] Memory: Large datasets loaded entirely into memory when streaming would work
• [ ] Caching: Repeated expensive computations that could be cached
• [ ] Algorithmic complexity: O(n²) or worse where O(n log n) or O(n) is possible

4. Maintainability (Low)

• [ ] Naming: Variables and functions have clear, descriptive names
• [ ] Dead code: Unused imports, unreachable branches, commented-out code
• [ ] Duplication: Repeated logic that should be extracted into a function
• [ ] Complexity: Functions doing too many things; deeply nested conditionals
• [ ] Consistency: Follows the project's existing patterns and conventions

Output Format

For each finding, use this structure:

### [SEVERITY] Brief title

**File**: `path/to/file.py` line 42-48
**Category**: Security | Correctness | Performance | Maintainability

**Problem**: Clear description of what's wrong and why it matters.

**Suggestion**:
```python
# Before (problematic)
query = f"SELECT * FROM users WHERE id = {user_id}"

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

Severity levels:
- **CRITICAL**: Security vulnerability or data loss risk. Must fix before merge.
- **HIGH**: Bug that will cause incorrect behavior. Should fix before merge.
- **MEDIUM**: Performance issue or code smell. Fix soon.
- **LOW**: Style or maintainability suggestion. Nice to have.

## Review Summary Template

After reviewing all code, provide a summary:

```markdown
## Review Summary

**Overall**: APPROVE / REQUEST CHANGES / NEEDS DISCUSSION

**Findings**: X critical, Y high, Z medium, W low

### Critical/High Items (must address)
1. [Brief description + location]

### Medium Items (should address)
1. [Brief description + location]

### Low Items (optional)
1. [Brief description + location]

### Positive Observations
- [Good patterns or practices noticed]

Language-Specific Checks

Python

• Mutable default arguments (def f(items=[]) — use None instead)
• Bare except: clauses (catch specific exceptions)
• == vs is for None checks (use is None)
• f-strings with user input in SQL/shell commands

JavaScript/TypeScript

• == vs === (prefer strict equality)
• Missing await on async functions
• Prototype pollution via unchecked object spread
• innerHTML with user content (use textContent)

Go

• Unchecked errors (err assigned but not checked)
• Goroutine leaks (unbuffered channels, missing context cancellation)
• Race conditions on shared maps

Rust

• unwrap() in production code (use ? or handle the error)
• Missing Send/Sync bounds for concurrent types

Gotchas

• Review the change, not the whole file: Focus on what's new or modified. Don't nitpick existing code unless it interacts with the change.
• Severity matters more than count: One critical security issue outweighs ten style suggestions. Lead with the most important findings.
• Suggest, don't rewrite: Provide enough of a fix to be clear, but don't refactor their entire approach unless asked.
• Check the tests: If there are tests, verify they actually test the behavior, not just run without errors. Missing tests for new behavior is a finding.
• Consider the context: A quick prototype has different standards than production code. Calibrate accordingly.

Validation

After completing a review:

1. Every finding has a file location and a clear suggestion
2. Severity levels are consistent (don't call style issues "critical")
3. The summary accurately reflects the findings
4. Positive patterns are acknowledged, not just problems