Code Review Skill

Triggers

Use this skill when you see:

• review, code review, PR review
• audit, quality check, security review
• feedback, suggestions, improvements

Instructions

Review Philosophy

Goals of Code Review:

1. Catch bugs before production
2. Improve code quality
3. Share knowledge across team
4. Maintain consistency
5. Document decisions

Mindset:

• Be constructive, not critical
• Assume good intent
• Focus on the code, not the person
• Ask questions to understand
• Acknowledge good work

Review Checklist

1. Correctness

• Does the code do what it's supposed to do?
• Are edge cases handled?
• Are error conditions handled gracefully?
• Do the tests cover the requirements?
• Are there any logic errors?

2. Security

• No hardcoded secrets or credentials
• Input validation on all user inputs
• SQL queries use parameterized statements
• Authentication/authorization checked
• Sensitive data properly encrypted
• No exposure of sensitive info in logs
• CSRF/XSS protections in place
• Dependencies checked for vulnerabilities

3. Performance

• No N+1 query problems
• Appropriate indexing for queries
• No unnecessary loops or iterations
• Large datasets paginated
• Caching used where appropriate
• No memory leaks
• Async operations where beneficial

4. Maintainability

• Code is readable and self-documenting
• Functions/methods have single responsibility
• No excessive complexity (cyclomatic)
• Magic numbers/strings extracted to constants
• Dead code removed
• Consistent naming conventions
• Appropriate comments for complex logic

5. Testing

• Unit tests for new functionality
• Edge cases tested
• Error paths tested
• Tests are readable and maintainable
• No flaky tests
• Integration tests where appropriate
• Test coverage adequate

6. Architecture

• Follows established patterns
• Proper separation of concerns
• Dependencies flow correctly
• No circular dependencies
• API contracts maintained
• Breaking changes documented

Review Process

Before Reviewing

1. Understand the context
   - Read the PR description
   - Check linked issues/tickets
   - Understand the requirements

2. Get the big picture
   - Look at files changed
   - Identify the scope
   - Note architectural changes

3. Run the code (if possible)
   - Check out the branch
   - Run tests locally
   - Try the feature manually

During Review

1. First pass: Overview
   - Skim all changes
   - Note major concerns
   - Identify areas needing deep review

2. Second pass: Details
   - Review each file carefully
   - Check logic and edge cases
   - Verify test coverage

3. Third pass: Integration
   - Consider system impact
   - Check for breaking changes
   - Verify documentation

Giving Feedback

Feedback Categories

Blocking (Must Fix)

[BLOCKING] SQL injection vulnerability in user input handling.

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

This should use parameterized queries:

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

**Suggestion (Should Consider)**

[SUGGESTION] Consider extracting this to a separate function.

This block handles three distinct concerns. Separating them would improve:

• Testability
• Reusability
• Readability

Something like:

def validate_input(data): ...
def transform_data(data): ...
def persist_data(data): ...

**Nitpick (Take It or Leave It)**

[NIT] Consider using a more descriptive variable name.

x could be user_count for clarity.

**Praise (Acknowledgment)**

[PRAISE] Great use of the strategy pattern here! This makes it easy to add new payment providers.

**Question (Clarification)**

[QUESTION] What happens if response is null here?

I don't see null handling - is this guaranteed by the caller?

### Common Issues by Language

#### Python
```python
# Missing type hints
def process(data):  # -> def process(data: dict) -> bool:

# Mutable default arguments
def func(items=[]):  # -> def func(items=None): items = items or []

# Not using context managers
f = open('file.txt')  # -> with open('file.txt') as f:

# Catching too broad exceptions
except Exception:  # -> except SpecificError:

# String concatenation in loops
result = ""
for item in items:
    result += str(item)  # Use ''.join() instead

TypeScript/JavaScript

// Missing null checks
user.name.toLowerCase();  // -> user?.name?.toLowerCase()

// Using any
function process(data: any)  // -> proper typing

// Not awaiting promises
async function fetch() {
    fetchData();  // -> await fetchData()
}

// == instead of ===
if (value == null)  // -> if (value === null)

// Missing error handling in async
try { await riskyOp(); } catch (e) { }  // -> handle error

SQL

-- Missing indexes for WHERE clauses
SELECT * FROM orders WHERE customer_id = 123;
-- Ensure customer_id is indexed

-- SELECT * in production
SELECT * FROM users;  -- -> SELECT id, name, email FROM users

-- No LIMIT on potentially large results
SELECT * FROM logs;  -- -> SELECT * FROM logs LIMIT 1000

-- String concatenation (SQL injection)
"SELECT * FROM users WHERE name = '" + name + "'"
-- -> Parameterized query

Review Response Template

## Summary
[1-2 sentence overview of the changes and your assessment]

## Blocking Issues
[List any issues that must be fixed before merge]

## Suggestions
[Recommended improvements, not blocking]

## Questions
[Clarifications needed]

## Positive Notes
[What was done well]

## Decision
- [ ] Approved
- [ ] Request Changes
- [ ] Need Discussion

Best Practices

1. Review Promptly: Don't let PRs sit for days
2. Limit Review Size: 400 lines max per review session
3. Use Checklists: Consistent review quality
4. Be Specific: Point to exact lines with fixes
5. Explain Why: Not just what's wrong, but why
6. Offer Solutions: Don't just criticize
7. Follow Up: Check that feedback was addressed
8. Learn from Reviews: Each review is a learning opportunity