Code Review Guidelines

A comprehensive guide for AI agents performing code reviews, organized by priority and impact.

Table of Contents

Security — CRITICAL

1. SQL Injection Prevention
2. XSS Prevention

Performance — HIGH

3. Avoid N+1 Query Problem

Correctness — HIGH

4. Proper Error Handling

Maintainability — MEDIUM

5. Use Meaningful Variable Names
6. Add Type Hints

Security

SQL Injection Prevention

Impact: CRITICAL | Category: security | Tags: sql, security, injection, database

Never construct SQL queries with string concatenation or f-strings. Always use parameterized queries to prevent SQL injection attacks.

Why This Matters

SQL injection is one of the most common and dangerous web vulnerabilities. Attackers can:

• Access unauthorized data
• Modify or delete database records
• Execute admin operations on the database
• In some cases, issue commands to the OS

❌ Incorrect

def get_user(user_id):
    query = f"SELECT * FROM users WHERE id = {user_id}"
    result = db.execute(query)
    return result

# Vulnerable to: get_user("1 OR 1=1")
# Returns all users!

✅ Correct

def get_user(user_id: int) -> Optional[Dict[str, Any]]:
    query = "SELECT * FROM users WHERE id = ?"
    result = db.execute(query, (user_id,))
    return result.fetchone() if result else None

➡️ Full details: security-sql-injection.md

XSS Prevention

Impact: CRITICAL | Category: security | Tags: xss, security, html, javascript

Never insert unsanitized user input into HTML. Always escape output or use frameworks that auto-escape by default.

❌ Incorrect

// Dangerous!
document.getElementById('username').innerHTML = userInput;

✅ Correct

// Safe: use textContent
element.textContent = userInput;

// Or sanitize if HTML needed
import DOMPurify from 'dompurify';
element.innerHTML = DOMPurify.sanitize(userHtml);

➡️ Full details: security-xss-prevention.md

Performance

Avoid N+1 Query Problem

Impact: HIGH | Category: performance | Tags: database, performance, orm, queries

The N+1 query problem occurs when code executes 1 query to fetch a list, then N additional queries to fetch related data for each item.

❌ Incorrect

# 101 queries for 100 posts!
posts = Post.objects.all()  # 1 query
for post in posts:
    print(f"{post.title} by {post.author.name}")  # N queries

✅ Correct

# 1 query with JOIN
posts = Post.objects.select_related('author').all()
for post in posts:
    print(f"{post.title} by {post.author.name}")  # No extra queries!

➡️ Full details: performance-n-plus-one.md

Correctness

Proper Error Handling

Impact: HIGH | Category: correctness | Tags: errors, exceptions, reliability

Always handle errors explicitly. Don't use bare except clauses or ignore errors silently.

❌ Incorrect

try:
    result = risky_operation()
except:
    pass  # Silent failure!

✅ Correct

try:
    config = json.loads(config_file.read())
except json.JSONDecodeError as e:
    logger.error(f"Invalid JSON in config file: {e}")
    config = get_default_config()
except FileNotFoundError:
    logger.warning("Config file not found, using defaults")
    config = get_default_config()

➡️ Full details: correctness-error-handling.md

Maintainability

Use Meaningful Variable Names

Impact: MEDIUM | Category: maintainability | Tags: naming, readability, code-quality

Choose descriptive, intention-revealing names. Avoid single letters (except loop counters), abbreviations, and generic names.

❌ Incorrect

def calc(x, y, z):
    tmp = x * y
    res = tmp + z
    return res

✅ Correct

def calculate_total_price(item_price: float, quantity: int, tax_rate: float) -> float:
    subtotal = item_price * quantity
    total_with_tax = subtotal + (subtotal * tax_rate)
    return total_with_tax

➡️ Full details: maintainability-naming.md

Add Type Hints

Impact: MEDIUM | Category: maintainability | Tags: types, python, typescript, type-safety

Use type annotations to make code self-documenting and catch errors early.

❌ Incorrect

def get_user(id):
    return users.get(id)

✅ Correct

def get_user(id: int) -> Optional[Dict[str, Any]]:
    """Fetch user by ID."""
    return users.get(id)

➡️ Full details: maintainability-type-hints.md

Quick Reference

Review Checklist

Security (CRITICAL - review first)

• No SQL injection vulnerabilities
• No XSS vulnerabilities
• Secrets not hardcoded
• Authentication/authorization checks present

Performance (HIGH)

• No N+1 queries
• Appropriate caching
• No unnecessary database calls
• Efficient algorithms

Correctness (HIGH) - [ ] Proper error handling

• Edge cases handled
• Input validation
• No race conditions

Maintainability (MEDIUM)

• Clear variable/function names
• Type hints present
• Code is DRY (Don't Repeat Yourself)
• Functions are single-purpose

Testing

• Tests cover new code
• Edge cases tested
• Error paths tested

Severity Levels

Review Output Format

When performing reviews, structure as:

## Security Issues (X found)

### CRITICAL: SQL Injection in `get_user()`
**File:** `api/users.py:45`
**Issue:** User input interpolated directly into SQL query
**Fix:** Use parameterized query

## Performance Issues (X found)

### HIGH: N+1 Query in `list_posts()`
**File:** `views/posts.py:23`
**Issue:** Fetching author in loop
**Fix:** Add `.select_related('author')`

## Summary
- 🔴 CRITICAL: 1
- 🟠 HIGH: 1
- 🟡 MEDIUM: 3
- ⚪ LOW: 2

**Recommendation:** Address CRITICAL and HIGH issues before merging.

References

• Individual rule files in rules/ directory
• OWASP Top 10
• Clean Code by Robert Martin