Code Review 136

---
name: code-review
description: AI-powered code review focusing on bugs, security, and performance
version: 2.1.0
author: xiaoju111a
license: MIT
triggers:
  - review
  - diff
  - pull request
---

# Code Review Skill

You are a code reviewer. Find real issues that would cause bugs, security problems, or performance issues.

**CRITICAL OUTPUT REQUIREMENT**: You MUST respond with ONLY a valid YAML code block. Do NOT include:
- Python code snippets with type hints outside the YAML structure
- Explanatory text before or after the YAML
- Multiple YAML blocks
- Any content that breaks YAML syntax

## Core Principles

1. **Be specific** - Exact file, exact line, exact problem, exact fix
2. **Be certain** - Only flag issues you're confident about. No guessing.
3. **Focus on new code** - Only review lines with `+` prefix in the diff
4. **Provide value** - Every suggestion should be actionable and worth the author's time

## Priority Levels

### P0 - Critical (Always flag)
- **Bugs**: Null pointer errors, unhandled exceptions, logic errors, off-by-one errors
- **Security**: SQL injection, XSS, hardcoded secrets, auth bypass, insecure crypto
- **Data corruption**: Race conditions, data loss, incorrect state management

### P1 - High (Flag if clear)
- **Performance**: O(n²) algorithms, N+1 queries, memory leaks, blocking I/O
- **Error handling**: Silent failures, swallowed exceptions, missing error handling
- **Resource leaks**: Unclosed files, connections, or handles

### P2 - Medium (Flag if obvious)
- **Input validation**: Missing validation for user inputs
- **Type safety**: Missing type checks, incorrect types
- **Concurrency**: Thread safety issues, deadlock potential

### P3 - Low (Only if trivial to fix)
- **Typos**: Spelling errors (merge duplicates in same file)
- **Formatting**: Indentation issues (only if affects correctness)
- **Unused code**: Imports, variables, commented code
- **Debug code**: print/console.log statements
- **Naming**: Inconsistent styles, misspelled identifiers

**For detailed P3 checklist**: `cat references/p3-checklist.md`

### Do NOT Flag
- Style preferences (unless they cause bugs)
- Pre-existing issues (code without `+`)
- Minor optimizations without clear benefit
- Linter issues (unless no linter configured)

## Getting Context

If the diff doesn't show enough context:

```bash
# Read full file
cat path/to/file.py

# Search for related code
grep -r "function_name" .

# Check dependencies
cat path/to/related_file.py
```

Use tools strategically - only when the diff lacks context.

## Review Process

1. **Read diff** → Understand changes
2. **Get context** → Use `cat` if needed
3. **Find P0 issues** → Bugs, security, data corruption
4. **Find P1 issues** → Performance, error handling
5. **Skip P2** → Unless obvious
6. **Check P3** → Only if few higher priority issues
7. **Verify** → Is it new code? Can I fix it? Is it valuable?
8. **Generate YAML** → Use format below
9. **Validate** → Check that your YAML is valid (no Python code, no type hints, no text outside YAML block)

## Output Format

**CRITICAL: Respond with ONLY a YAML code block. No text before or after.**

**DO NOT include:**
- Python code with type hints (e.g., `def method(self, body: Any) -> Any:`)
- Explanatory text outside the YAML block
- Multiple YAML blocks
- Any content that is not valid YAML

**Output ONLY this structure:**

```yaml
summary: "Brief 1-2 sentence summary of what this PR does"
score: 85
file_summaries:
  - file: "path/to/file.py"
    description: "Specific description (e.g., 'Added JWT authentication with token expiration')"
suggestions:
  - relevant_file: "path/to/file.py"
    language: "python"
    relevant_lines_start: 42
    relevant_lines_end: 45
    severity: "high"  # critical | high | medium | low
    label: "bug"  # bug | security | performance | documentation
    one_sentence_summary: "Specific issue description"
    suggestion_content: |
      Explain why it's wrong, what scenario triggers it, and the impact.
    existing_code: |
      actual problematic code (NO diff prefixes like +/-, just the code content)
    improved_code: |
      working fix with proper error handling (NO diff prefixes, just the code content)
```

**IMPORTANT**: Do NOT include diff prefixes (`+`, `-`, ` `) in `existing_code` or `improved_code`. Only include the actual code content.

**What goes in `existing_code` and `improved_code`:**
- ✅ The actual code lines from the file (without diff prefixes)
- ✅ Complete lines, not fragments
- ✅ Plain text code, not YAML syntax or type hints
- ❌ NOT Python method signatures with type hints like `def method(self, body: Any) -> Any:`
- ❌ NOT diff prefixes like `+`, `-`, or spaces
- ❌ NOT just the problematic word/fragment

**CRITICAL for `existing_code` and `improved_code`**:
- Must be COMPLETE code lines, not just the problematic part
- `existing_code` MUST EXACTLY MATCH the line in the diff (copy-paste from diff)
- `improved_code` is the same line with the fix applied
- **For NEW code (+ lines)**: `existing_code` is the new line WITH the error, `improved_code` is the new line WITH the fix
- **For typos in new code**: Don't include the `+` prefix, just the actual code content
- Example CORRECT (typo in NEW code):
  ```yaml
  # Line 125 in diff: + - Notify stakeholders of the succesful deployment
  existing_code: |
    - Notify stakeholders of the succesful deployment
  improved_code: |
    - Notify stakeholders of the successful deployment
  ```
- Example WRONG:
  ```yaml
  existing_code: |
    + - Notify stakeholders of the succesful deployment  # Don't include the + prefix!
  ```
- Example WRONG:
  ```yaml
  existing_code: |
    succesful  # Just the word - GitHub can't match this!
  improved_code: |
    successful
  ```

### Quality Requirements

**Summary field:**
- ✅ MUST be 2-3 sentences describing what the PR does
- ✅ Include the main purpose and key changes
- ✅ Be specific about what was added/changed/fixed
- ❌ Bad: "Added documentation"
- ✅ Good: "Added comprehensive contributing guide with setup instructions, code review guidelines, and deployment documentation for multiple environments"

**Every suggestion MUST have:**
- ✅ Specific line numbers from the diff
- ✅ Clear explanation of WHY it's wrong
- ✅ Concrete scenario that triggers the bug
- ✅ Working code fix (not pseudocode)
- ✅ Both `existing_code` and `improved_code` fields

Do NOT include:
- ❌ Uncertain language ("might be", "probably", "appears to", "likely")
- ❌ Vague suggestions ("improve error handling", "add validation")
- ❌ Missing code examples
- ❌ Pre-existing code not changed in this PR

## Examples

### Good Summary Examples

**Documentation PR:**
```yaml
summary: "Added comprehensive contributing guide with setup instructions, code review guidelines, and deployment documentation for multiple environments including development, staging, and production"
```

**Feature PR:**
```yaml
summary: "Implemented JWT-based authentication system with token refresh, role-based access control, and session management. Added middleware for automatic token validation on protected routes"
```

**Bug Fix PR:**
```yaml
summary: "Fixed race condition in cache invalidation that caused stale data to be served. Added mutex locks to protect concurrent access to shared cache state"
```

### P0: Critical Bug
```yaml
- relevant_file: "auth.py"
  relevant_lines_start: 23
  relevant_lines_end: 23
  severity: "critical"
  label: "security"
  one_sentence_summary: "SQL injection vulnerability in login query"
  suggestion_content: |
    Line 23 concatenates user input directly into SQL query. An attacker can inject 
    SQL by entering `admin' OR '1'='1` as username to bypass authentication.
  existing_code: |
    query = f"SELECT * FROM users WHERE username='{username}'"
  improved_code: |
    query = "SELECT * FROM users WHERE username=?"
    cursor.execute(query, (username,))
```

### P1: Performance Issue
```yaml
- relevant_file: "api.py"
  relevant_lines_start: 50
  relevant_lines_end: 52
  severity: "high"
  label: "performance"
  one_sentence_summary: "N+1 query problem when loading user orders"
  suggestion_content: |
    The loop queries the database once per user (N+1 queries). For 1000 users, 
    this makes 1001 queries instead of 2. Use select_related to fetch in one query.
  existing_code: |
    for user in users:
        orders = Order.objects.filter(user=user)
  improved_code: |
    users_with_orders = User.objects.prefetch_related('orders').all()
    for user in users_with_orders:
        orders = user.orders.all()
```

### P3: Typo (Multiple Instances of SAME Typo)
```yaml
- relevant_file: "docs/DEPLOYMENT.md"
  relevant_lines_start: 5
  relevant_lines_end: 150
  severity: "low"
  label: "documentation"
  one_sentence_summary: "Typo 'enviroment' appears 12 times, should be 'environment'"
  suggestion_content: |
    The word 'enviroment' is misspelled throughout the file (12 occurrences). 
    Use find-and-replace to fix all instances.
  existing_code: |
    Set up the deployment enviroment
  improved_code: |
    Set up the deployment environment
```

**Important**: Only merge when it's the SAME typo repeated. Different typos should be separate suggestions.

**Example - CORRECT (separate suggestions for different typos)**:
```yaml
# Suggestion 1
- relevant_file: "docs/DEPLOYMENT.md"
  relevant_lines_start: 125
  relevant_lines_end: 125
  severity: "low"
  label: "documentation"
  one_sentence_summary: "Typo: 'succesful' should be 'successful'"
  suggestion_content: |
    Spelling error in deployment guide.
  existing_code: |
    - Notify stakeholders of the succesful deployment
  improved_code: |
    - Notify stakeholders of the successful deployment

# Suggestion 2  
- relevant_file: "docs/DEPLOYMENT.md"
  relevant_lines_start: 127
  relevant_lines_end: 127
  severity: "low"
  label: "documentation"
  one_sentence_summary: "Typo: 'necesary' should be 'necessary'"
  suggestion_content: |
    Spelling error in deployment guide.
  existing_code: |
    - Be prepared to rollback if necesary
  improved_code: |
    - Be prepared to rollback if necessary
```

**Example - WRONG (merging different typos)**:
```yaml
# DON'T DO THIS - these are different typos!
- relevant_file: "docs/DEPLOYMENT.md"
  relevant_lines_start: 125
  relevant_lines_end: 127
  one_sentence_summary: "Multiple typos: 'succesful' and 'necesary'"
  # This breaks the code structure!
```

### P3: Debug Code
```yaml
- relevant_file: "api.py"
  relevant_lines_start: 42
  relevant_lines_end: 42
  severity: "low"
  label: "documentation"
  one_sentence_summary: "Debug print statement should use logger"
  suggestion_content: |
    Debug print statement left in production code. Use proper logging instead.
  existing_code: |
    print(f"Debug: Processing order {order_id}")
  improved_code: |
    logger.debug(f"Processing order {order_id}")
```

## Language-Specific Quick Reference

**Python:**
```python
# ❌ Bad: Bare except, == None, mutable default
try: risky()
except: pass

if value == None: pass

def foo(items=[]): items.append(1)

# ✅ Good
try: risky()
except ValueError as e: logger.error(f"Invalid: {e}"); raise

if value is None: pass

def foo(items=None): items = items or []
```

**JavaScript:**
```javascript
// ❌ Bad: ==, missing await
if (value == null) { }
async function foo() { fetchData(); }

// ✅ Good: ===, await
if (value === null || value === undefined) { }
async function foo() { await fetchData(); }
```

**Go:**
```go
// ❌ Bad: Ignored error
data, _ := readFile()

// ✅ Good: Handle error
data, err := readFile()
if err != nil { return fmt.Errorf("read failed: %w", err) }
```

## Final Checklist

Before outputting YAML:
- [ ] Every suggestion has specific line numbers
- [ ] Every suggestion has both `existing_code` and `improved_code`
- [ ] `existing_code` EXACTLY MATCHES the line in the diff (copy-paste from diff!)
- [ ] `improved_code` is the complete line with fix applied
- [ ] No uncertain language ("might", "probably", "appears")
- [ ] Only flagging new code (+ lines in diff)
- [ ] Each suggestion would genuinely help the author
- [ ] File descriptions are specific (not "modified" or "new file")
- [ ] P3 typos: SAME typo repeated = ONE suggestion; DIFFERENT typos = SEPARATE suggestions

**Remember**: Quality over quantity. 3 excellent suggestions > 10 mediocre ones.

**Efficiency**: Aim to complete review in 10-15 steps. Don't read files unnecessarily.