Initial commit
This commit is contained in:
Zhongwei Li
562
.claude/skills/code-review-framework/SKILL.md
Normal file
562
.claude/skills/code-review-framework/SKILL.md
Normal file
@@ -0,0 +1,562 @@
|
||||
---
|
||||
name: code-review-framework
|
||||
description: Automatically applies when reviewing code. Ensures structured review checklist covering correctness, security, performance, maintainability, testing, and documentation.
|
||||
category: python
|
||||
---
|
||||
|
||||
# Code Review Framework
|
||||
|
||||
When reviewing code, follow this structured framework for comprehensive, consistent reviews.
|
||||
|
||||
**Trigger Keywords**: code review, PR review, pull request, review checklist, code quality, review comments, review feedback
|
||||
|
||||
**Agent Integration**: Used by `code-reviewer`, `backend-architect`, `security-engineer`
|
||||
|
||||
## ✅ Correct Pattern: Review Checklist
|
||||
|
||||
```python
|
||||
"""
|
||||
Code Review Checklist
|
||||
===================
|
||||
|
||||
Use this checklist for every code review.
|
||||
"""
|
||||
|
||||
from typing import List, Dict
|
||||
from dataclasses import dataclass
|
||||
from enum import Enum
|
||||
|
||||
|
||||
class ReviewCategory(str, Enum):
|
||||
"""Review categories."""
|
||||
CORRECTNESS = "correctness"
|
||||
SECURITY = "security"
|
||||
PERFORMANCE = "performance"
|
||||
MAINTAINABILITY = "maintainability"
|
||||
TESTING = "testing"
|
||||
DOCUMENTATION = "documentation"
|
||||
|
||||
|
||||
class ReviewSeverity(str, Enum):
|
||||
"""Issue severity levels."""
|
||||
BLOCKING = "blocking" # Must fix before merge
|
||||
MAJOR = "major" # Should fix
|
||||
MINOR = "minor" # Nice to have
|
||||
NITPICK = "nitpick" # Style/preference
|
||||
|
||||
|
||||
@dataclass
|
||||
class ReviewComment:
|
||||
"""Single review comment."""
|
||||
category: ReviewCategory
|
||||
severity: ReviewSeverity
|
||||
file: str
|
||||
line: int
|
||||
message: str
|
||||
suggestion: str = ""
|
||||
|
||||
|
||||
class CodeReview:
|
||||
"""Structured code review."""
|
||||
|
||||
def __init__(self):
|
||||
self.comments: List[ReviewComment] = []
|
||||
|
||||
def add_comment(
|
||||
self,
|
||||
category: ReviewCategory,
|
||||
severity: ReviewSeverity,
|
||||
file: str,
|
||||
line: int,
|
||||
message: str,
|
||||
suggestion: str = ""
|
||||
):
|
||||
"""Add review comment."""
|
||||
self.comments.append(ReviewComment(
|
||||
category=category,
|
||||
severity=severity,
|
||||
file=file,
|
||||
line=line,
|
||||
message=message,
|
||||
suggestion=suggestion
|
||||
))
|
||||
|
||||
def check_correctness(self, code: str):
|
||||
"""Check correctness issues."""
|
||||
checks = [
|
||||
self._check_error_handling,
|
||||
self._check_edge_cases,
|
||||
self._check_logic_errors,
|
||||
self._check_type_safety,
|
||||
]
|
||||
for check in checks:
|
||||
check(code)
|
||||
|
||||
def check_security(self, code: str):
|
||||
"""Check security issues."""
|
||||
checks = [
|
||||
self._check_input_validation,
|
||||
self._check_sql_injection,
|
||||
self._check_secret_exposure,
|
||||
self._check_authentication,
|
||||
]
|
||||
for check in checks:
|
||||
check(code)
|
||||
|
||||
def check_performance(self, code: str):
|
||||
"""Check performance issues."""
|
||||
checks = [
|
||||
self._check_n_plus_one,
|
||||
self._check_inefficient_loops,
|
||||
self._check_memory_leaks,
|
||||
self._check_async_usage,
|
||||
]
|
||||
for check in checks:
|
||||
check(code)
|
||||
|
||||
def get_blocking_issues(self) -> List[ReviewComment]:
|
||||
"""Get blocking issues that prevent merge."""
|
||||
return [
|
||||
c for c in self.comments
|
||||
if c.severity == ReviewSeverity.BLOCKING
|
||||
]
|
||||
|
||||
def generate_summary(self) -> Dict[str, int]:
|
||||
"""Generate review summary."""
|
||||
summary = {
|
||||
"total_comments": len(self.comments),
|
||||
"blocking": 0,
|
||||
"major": 0,
|
||||
"minor": 0,
|
||||
"nitpick": 0,
|
||||
}
|
||||
|
||||
for comment in self.comments:
|
||||
summary[comment.severity.value] += 1
|
||||
|
||||
return summary
|
||||
```
|
||||
|
||||
## Correctness Review
|
||||
|
||||
```python
|
||||
"""
|
||||
Correctness Review Checklist
|
||||
===========================
|
||||
|
||||
1. Error Handling
|
||||
"""
|
||||
|
||||
# ❌ Missing error handling
|
||||
async def fetch_user(user_id: int):
|
||||
response = await http_client.get(f"/users/{user_id}")
|
||||
return response.json() # What if request fails?
|
||||
|
||||
# ✅ Proper error handling
|
||||
async def fetch_user(user_id: int) -> Dict:
|
||||
"""
|
||||
Fetch user by ID.
|
||||
|
||||
Args:
|
||||
user_id: User ID
|
||||
|
||||
Returns:
|
||||
User data dict
|
||||
|
||||
Raises:
|
||||
UserNotFoundError: If user doesn't exist
|
||||
APIError: If API request fails
|
||||
"""
|
||||
try:
|
||||
response = await http_client.get(f"/users/{user_id}")
|
||||
response.raise_for_status()
|
||||
return response.json()
|
||||
except httpx.HTTPStatusError as e:
|
||||
if e.response.status_code == 404:
|
||||
raise UserNotFoundError(f"User {user_id} not found")
|
||||
raise APIError(f"API request failed: {e}")
|
||||
except httpx.RequestError as e:
|
||||
raise APIError(f"Network error: {e}")
|
||||
|
||||
|
||||
"""
|
||||
2. Edge Cases
|
||||
"""
|
||||
|
||||
# ❌ No edge case handling
|
||||
def divide(a: float, b: float) -> float:
|
||||
return a / b # What if b is 0?
|
||||
|
||||
# ✅ Edge cases handled
|
||||
def divide(a: float, b: float) -> float:
|
||||
"""
|
||||
Divide a by b.
|
||||
|
||||
Args:
|
||||
a: Numerator
|
||||
b: Denominator
|
||||
|
||||
Returns:
|
||||
Result of division
|
||||
|
||||
Raises:
|
||||
ValueError: If b is zero
|
||||
"""
|
||||
if b == 0:
|
||||
raise ValueError("Cannot divide by zero")
|
||||
return a / b
|
||||
|
||||
|
||||
"""
|
||||
3. Logic Errors
|
||||
"""
|
||||
|
||||
# ❌ Logic error
|
||||
def calculate_discount(price: float, discount_percent: float) -> float:
|
||||
return price - price * discount_percent # Should divide by 100!
|
||||
|
||||
# ✅ Correct logic
|
||||
def calculate_discount(price: float, discount_percent: float) -> float:
|
||||
"""Calculate price after discount."""
|
||||
if not 0 <= discount_percent <= 100:
|
||||
raise ValueError("Discount must be between 0 and 100")
|
||||
return price - (price * discount_percent / 100)
|
||||
|
||||
|
||||
"""
|
||||
4. Type Safety
|
||||
"""
|
||||
|
||||
# ❌ No type hints
|
||||
def process_data(data): # What type is data?
|
||||
return data.upper()
|
||||
|
||||
# ✅ Type hints
|
||||
def process_data(data: str) -> str:
|
||||
"""Process string data."""
|
||||
return data.upper()
|
||||
```
|
||||
|
||||
## Security Review
|
||||
|
||||
```python
|
||||
"""
|
||||
Security Review Checklist
|
||||
========================
|
||||
|
||||
1. Input Validation
|
||||
"""
|
||||
|
||||
# ❌ No input validation
|
||||
@app.post("/users")
|
||||
async def create_user(email: str, password: str):
|
||||
user = User(email=email, password=password)
|
||||
db.add(user)
|
||||
return user
|
||||
|
||||
# ✅ Input validation with Pydantic
|
||||
class UserCreate(BaseModel):
|
||||
email: EmailStr # Validates email format
|
||||
password: str = Field(min_length=8, max_length=100)
|
||||
|
||||
@validator("password")
|
||||
def validate_password(cls, v):
|
||||
if not any(c.isupper() for c in v):
|
||||
raise ValueError("Password must contain uppercase")
|
||||
if not any(c.isdigit() for c in v):
|
||||
raise ValueError("Password must contain digit")
|
||||
return v
|
||||
|
||||
@app.post("/users")
|
||||
async def create_user(user: UserCreate):
|
||||
# Input is validated
|
||||
hashed_password = hash_password(user.password)
|
||||
db_user = User(email=user.email, password=hashed_password)
|
||||
db.add(db_user)
|
||||
return db_user
|
||||
|
||||
|
||||
"""
|
||||
2. SQL Injection Prevention
|
||||
"""
|
||||
|
||||
# ❌ SQL injection vulnerability
|
||||
def get_user(email: str):
|
||||
query = f"SELECT * FROM users WHERE email = '{email}'"
|
||||
return db.execute(query) # Vulnerable!
|
||||
|
||||
# ✅ Parameterized queries
|
||||
def get_user(email: str):
|
||||
query = text("SELECT * FROM users WHERE email = :email")
|
||||
return db.execute(query, {"email": email})
|
||||
|
||||
|
||||
"""
|
||||
3. Secret Exposure
|
||||
"""
|
||||
|
||||
# ❌ Hardcoded secrets
|
||||
API_KEY = "sk-1234567890abcdef" # Exposed!
|
||||
DATABASE_URL = "postgresql://user:password@localhost/db"
|
||||
|
||||
# ✅ Environment variables
|
||||
from pydantic_settings import BaseSettings
|
||||
|
||||
class Settings(BaseSettings):
|
||||
api_key: str = Field(alias="API_KEY")
|
||||
database_url: str = Field(alias="DATABASE_URL")
|
||||
|
||||
class Config:
|
||||
env_file = ".env"
|
||||
env_file_encoding = "utf-8"
|
||||
|
||||
|
||||
"""
|
||||
4. Authentication & Authorization
|
||||
"""
|
||||
|
||||
# ❌ No authentication
|
||||
@app.delete("/users/{user_id}")
|
||||
async def delete_user(user_id: int):
|
||||
db.delete(User, user_id) # Anyone can delete!
|
||||
|
||||
# ✅ Proper authentication and authorization
|
||||
@app.delete("/users/{user_id}")
|
||||
async def delete_user(
|
||||
user_id: int,
|
||||
current_user: User = Depends(get_current_user)
|
||||
):
|
||||
# Check authorization
|
||||
if not current_user.is_admin and current_user.id != user_id:
|
||||
raise HTTPException(403, "Not authorized")
|
||||
|
||||
db.delete(User, user_id)
|
||||
```
|
||||
|
||||
## Performance Review
|
||||
|
||||
```python
|
||||
"""
|
||||
Performance Review Checklist
|
||||
===========================
|
||||
|
||||
1. N+1 Queries
|
||||
"""
|
||||
|
||||
# ❌ N+1 query problem
|
||||
async def get_orders():
|
||||
orders = db.query(Order).all()
|
||||
for order in orders:
|
||||
print(order.user.email) # N queries!
|
||||
|
||||
# ✅ Eager loading
|
||||
async def get_orders():
|
||||
orders = db.query(Order).options(joinedload(Order.user)).all()
|
||||
for order in orders:
|
||||
print(order.user.email) # Single query
|
||||
|
||||
|
||||
"""
|
||||
2. Inefficient Loops
|
||||
"""
|
||||
|
||||
# ❌ Inefficient loop
|
||||
def find_duplicates(items: List[str]) -> List[str]:
|
||||
duplicates = []
|
||||
for i, item in enumerate(items):
|
||||
for j, other in enumerate(items):
|
||||
if i != j and item == other:
|
||||
duplicates.append(item) # O(n²)
|
||||
return duplicates
|
||||
|
||||
# ✅ Efficient with set
|
||||
def find_duplicates(items: List[str]) -> List[str]:
|
||||
seen = set()
|
||||
duplicates = set()
|
||||
for item in items:
|
||||
if item in seen:
|
||||
duplicates.add(item)
|
||||
seen.add(item)
|
||||
return list(duplicates) # O(n)
|
||||
|
||||
|
||||
"""
|
||||
3. Async Usage
|
||||
"""
|
||||
|
||||
# ❌ Blocking I/O in async
|
||||
async def fetch_data():
|
||||
response = requests.get("https://api.example.com") # Blocks!
|
||||
return response.json()
|
||||
|
||||
# ✅ Async I/O
|
||||
async def fetch_data():
|
||||
async with httpx.AsyncClient() as client:
|
||||
response = await client.get("https://api.example.com")
|
||||
return response.json()
|
||||
|
||||
|
||||
"""
|
||||
4. Memory Usage
|
||||
"""
|
||||
|
||||
# ❌ Loading entire file
|
||||
def process_file(filepath: str):
|
||||
with open(filepath) as f:
|
||||
content = f.read() # All in memory!
|
||||
for line in content.split('\n'):
|
||||
process_line(line)
|
||||
|
||||
# ✅ Streaming
|
||||
def process_file(filepath: str):
|
||||
with open(filepath) as f:
|
||||
for line in f: # One line at a time
|
||||
process_line(line)
|
||||
```
|
||||
|
||||
## Review Comment Template
|
||||
|
||||
```markdown
|
||||
## Review Summary
|
||||
|
||||
### Overview
|
||||
- **Files Changed**: 5
|
||||
- **Lines Changed**: +150, -30
|
||||
- **Blocking Issues**: 0
|
||||
- **Major Issues**: 2
|
||||
- **Minor Issues**: 3
|
||||
|
||||
### Blocking Issues
|
||||
None
|
||||
|
||||
### Major Issues
|
||||
|
||||
1. **File: `api/users.py`, Line 45** (Security)
|
||||
```python
|
||||
# Current
|
||||
query = f"SELECT * FROM users WHERE id = {user_id}"
|
||||
|
||||
# Suggestion
|
||||
query = text("SELECT * FROM users WHERE id = :id")
|
||||
result = db.execute(query, {"id": user_id})
|
||||
```
|
||||
**Reason**: SQL injection vulnerability. Always use parameterized queries.
|
||||
|
||||
2. **File: `services/orders.py`, Line 78** (Performance)
|
||||
```python
|
||||
# Current
|
||||
orders = db.query(Order).all()
|
||||
for order in orders:
|
||||
print(order.user.email) # N+1 query
|
||||
|
||||
# Suggestion
|
||||
orders = db.query(Order).options(joinedload(Order.user)).all()
|
||||
```
|
||||
**Reason**: N+1 query problem. Use eager loading.
|
||||
|
||||
### Minor Issues
|
||||
|
||||
1. **File: `models/user.py`, Line 12** (Type Safety)
|
||||
- Missing return type hint on `get_full_name` method
|
||||
- Add: `-> str`
|
||||
|
||||
2. **File: `tests/test_users.py`, Line 34** (Testing)
|
||||
- Missing test for edge case: empty email
|
||||
- Add test case
|
||||
|
||||
3. **File: `utils/helpers.py`, Line 56** (Documentation)
|
||||
- Missing docstring for `format_date` function
|
||||
- Add Google-style docstring
|
||||
|
||||
### Positive Feedback
|
||||
- ✅ Good use of Pydantic models for validation
|
||||
- ✅ Comprehensive error handling
|
||||
- ✅ Well-structured code
|
||||
- ✅ Good test coverage (85%)
|
||||
|
||||
### Next Steps
|
||||
1. Address major issues (SQL injection, N+1)
|
||||
2. Consider minor improvements
|
||||
3. Update tests
|
||||
4. Update documentation
|
||||
|
||||
**Overall Assessment**: Approve after addressing major issues.
|
||||
```
|
||||
|
||||
## ❌ Anti-Patterns in Reviews
|
||||
|
||||
```python
|
||||
# ❌ Vague comments
|
||||
"This doesn't look right"
|
||||
|
||||
# ✅ Better: Specific with suggestion
|
||||
"SQL injection vulnerability on line 45. Use parameterized queries:
|
||||
query = text('SELECT * FROM users WHERE id = :id')"
|
||||
|
||||
|
||||
# ❌ No severity indication
|
||||
"You should add error handling"
|
||||
|
||||
# ✅ Better: Clear severity
|
||||
"[BLOCKING] Missing error handling. API calls can fail and crash the app."
|
||||
|
||||
|
||||
# ❌ Only pointing out negatives
|
||||
"You forgot type hints, missing tests, bad variable names"
|
||||
|
||||
# ✅ Better: Balance with positives
|
||||
"Good use of Pydantic! One suggestion: add type hints to helper functions"
|
||||
|
||||
|
||||
# ❌ Style preferences as blocking
|
||||
"[BLOCKING] Use single quotes instead of double quotes"
|
||||
|
||||
# ✅ Better: Appropriate severity
|
||||
"[NITPICK] Consider single quotes for consistency"
|
||||
```
|
||||
|
||||
## Best Practices Checklist
|
||||
|
||||
### Reviewer
|
||||
- ✅ Use structured review checklist
|
||||
- ✅ Categorize comments (correctness, security, etc.)
|
||||
- ✅ Indicate severity (blocking, major, minor)
|
||||
- ✅ Provide specific suggestions with code examples
|
||||
- ✅ Balance criticism with positive feedback
|
||||
- ✅ Focus on important issues first
|
||||
- ✅ Be respectful and constructive
|
||||
- ✅ Test the code if possible
|
||||
- ✅ Check for security vulnerabilities
|
||||
- ✅ Review tests and documentation
|
||||
|
||||
### Author
|
||||
- ✅ Self-review before requesting review
|
||||
- ✅ Provide context in PR description
|
||||
- ✅ Keep PRs focused and small (<400 lines)
|
||||
- ✅ Respond to all comments
|
||||
- ✅ Don't take feedback personally
|
||||
- ✅ Ask questions if unclear
|
||||
- ✅ Mark resolved comments
|
||||
- ✅ Update tests and docs
|
||||
- ✅ Verify all blocking issues fixed
|
||||
- ✅ Request re-review after changes
|
||||
|
||||
## Auto-Apply
|
||||
|
||||
When reviewing code:
|
||||
1. Use structured checklist (correctness, security, performance)
|
||||
2. Categorize and prioritize issues
|
||||
3. Provide specific suggestions with code
|
||||
4. Mark severity (blocking, major, minor, nitpick)
|
||||
5. Include positive feedback
|
||||
6. Focus on impact, not style
|
||||
7. Be respectful and constructive
|
||||
|
||||
## Related Skills
|
||||
|
||||
- `type-safety` - For type checking
|
||||
- `async-await-checker` - For async patterns
|
||||
- `structured-errors` - For error handling
|
||||
- `pytest-patterns` - For test review
|
||||
- `fastapi-patterns` - For API review
|
||||
- `pydantic-models` - For validation review
|
||||
Reference in New Issue
Block a user