Code Review Skill | Agent Skills

Code Review

When to use this skill

Reviewing pull requests
Checking code quality
Providing feedback on implementations
Identifying potential bugs
Suggesting improvements
Security audits
Performance analysis

Instructions

Step 1: Understand the context

Read the PR description:

What is the goal of this change?
Which issues does it address?
Are there any special considerations?

Check the scope:

How many files changed?
What type of changes? (feature, bugfix, refactor)
Are tests included?

Step 2: High-level review

Architecture and design:

Does the approach make sense?
Is it consistent with existing patterns?
Are there simpler alternatives?
Is the code in the right place?

Code organization:

Clear separation of concerns?
Appropriate abstraction levels?
Logical file/folder structure?

Step 3: Detailed code review

Naming:

[ ] Variables: descriptive, meaningful names
[ ] Functions: verb-based, clear purpose
[ ] Classes: noun-based, single responsibility
[ ] Constants: UPPER_CASE for true constants
[ ] Avoid abbreviations unless widely known

Functions:

[ ] Single responsibility
[ ] Reasonable length (< 50 lines ideally)
[ ] Clear inputs and outputs
[ ] Minimal side effects
[ ] Proper error handling

Classes and objects:

[ ] Single responsibility principle
[ ] Open/closed principle
[ ] Liskov substitution principle
[ ] Interface segregation
[ ] Dependency inversion

Error handling:

[ ] All errors caught and handled
[ ] Meaningful error messages
[ ] Proper logging
[ ] No silent failures
[ ] User-friendly errors for UI

Code quality:

[ ] No code duplication (DRY)
[ ] No dead code
[ ] No commented-out code
[ ] No magic numbers
[ ] Consistent formatting

Step 4: Security review

Input validation:

[ ] All user inputs validated
[ ] Type checking
[ ] Range checking
[ ] Format validation

Authentication & Authorization:

[ ] Proper authentication checks
[ ] Authorization for sensitive operations
[ ] Session management
[ ] Password handling (hashing, salting)

Data protection:

[ ] No hardcoded secrets
[ ] Sensitive data encrypted
[ ] SQL injection prevention
[ ] XSS prevention
[ ] CSRF protection

Dependencies:

[ ] No vulnerable packages
[ ] Dependencies up-to-date
[ ] Minimal dependency usage

Step 5: Performance review

Algorithms:

[ ] Appropriate algorithm choice
[ ] Reasonable time complexity
[ ] Reasonable space complexity
[ ] No unnecessary loops

Database:

[ ] Efficient queries
[ ] Proper indexing
[ ] N+1 query prevention
[ ] Connection pooling

Caching:

[ ] Appropriate caching strategy
[ ] Cache invalidation handled
[ ] Memory usage reasonable

Resource management:

[ ] Files properly closed
[ ] Connections released
[ ] Memory leaks prevented

Step 6: Testing review

Test coverage:

[ ] Unit tests for new code
[ ] Integration tests if needed
[ ] Edge cases covered
[ ] Error cases tested

Test quality:

[ ] Tests are readable
[ ] Tests are maintainable
[ ] Tests are deterministic
[ ] No test interdependencies
[ ] Proper test data setup/teardown

Test naming:

# Good
def test_user_creation_with_valid_data_succeeds():
    pass

# Bad
def test1():
    pass

Step 7: Documentation review

Code comments:

[ ] Complex logic explained
[ ] No obvious comments
[ ] TODOs have tickets
[ ] Comments are accurate

Function documentation:

def calculate_total(items: List[Item], tax_rate: float) -> Decimal:
    """
    Calculate the total price including tax.

    Args:
        items: List of items to calculate total for
        tax_rate: Tax rate as decimal (e.g., 0.1 for 10%)

    Returns:
        Total price including tax

    Raises:
        ValueError: If tax_rate is negative
    """
    pass

README/docs:

[ ] README updated if needed
[ ] API docs updated
[ ] Migration guide if breaking changes

Step 8: Provide feedback

Be constructive:

✅ Good:
"Consider extracting this logic into a separate function for better
testability and reusability:

def validate_email(email: str) -> bool:
    return '@' in email and '.' in email.split('@')[1]

This would make it easier to test and reuse across the codebase."

❌ Bad:
"This is wrong. Rewrite it."

Be specific:

✅ Good:
"On line 45, this query could cause N+1 problem. Consider using
.select_related('author') to fetch related objects in a single query."

❌ Bad:
"Performance issues here."

Prioritize issues:

🔴 Critical: Security, data loss, major bugs
🟡 Important: Performance, maintainability
🟢 Nice-to-have: Style, minor improvements

Acknowledge good work:

"Nice use of the strategy pattern here! This makes it easy to add
new payment methods in the future."

Review checklist

Functionality

[ ] Code does what it's supposed to do
[ ] Edge cases handled
[ ] Error cases handled
[ ] No obvious bugs

Code Quality

[ ] Clear, descriptive naming
[ ] Functions are small and focused
[ ] No code duplication
[ ] Consistent with codebase style
[ ] No code smells

Security

[ ] Input validation
[ ] No hardcoded secrets
[ ] Authentication/authorization
[ ] No SQL injection vulnerabilities
[ ] No XSS vulnerabilities

Performance

[ ] No obvious bottlenecks
[ ] Efficient algorithms
[ ] Proper database queries
[ ] Resource management

Testing

[ ] Tests included
[ ] Good test coverage
[ ] Tests are maintainable
[ ] Edge cases tested

Documentation

[ ] Code is self-documenting
[ ] Comments where needed
[ ] Docs updated
[ ] Breaking changes documented

Common issues

Anti-patterns

God class:

# Bad: One class doing everything
class UserManager:
    def create_user(self): pass
    def send_email(self): pass
    def process_payment(self): pass
    def generate_report(self): pass

Magic numbers:

# Bad
if user.age > 18:
    pass

# Good
MINIMUM_AGE = 18
if user.age > MINIMUM_AGE:
    pass

Deep nesting:

# Bad
if condition1:
    if condition2:
        if condition3:
            if condition4:
                # deeply nested code

# Good (early returns)
if not condition1:
    return
if not condition2:
    return
if not condition3:
    return
if not condition4:
    return
# flat code

Security vulnerabilities

SQL Injection:

# Bad
query = f"SELECT * FROM users WHERE id = {user_id}"

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

XSS:

// Bad
element.innerHTML = userInput;

// Good
element.textContent = userInput;

Hardcoded secrets:

# Bad
API_KEY = "sk-1234567890abcdef"

# Good
API_KEY = os.environ.get("API_KEY")

Best practices

Review promptly: Don't make authors wait
Be respectful: Focus on code, not the person
Explain why: Don't just say what's wrong
Suggest alternatives: Show better approaches
Use examples: Code examples clarify feedback
Pick your battles: Focus on important issues
Acknowledge good work: Positive feedback matters
Review your own code first: Catch obvious issues
Use automated tools: Let tools catch style issues
Be consistent: Apply same standards to all code

Tools to use

Linters:

Python: pylint, flake8, black
JavaScript: eslint, prettier
Go: golint, gofmt
Rust: clippy, rustfmt

Security:

Bandit (Python)
npm audit (Node.js)
OWASP Dependency-Check