菜单
Provides comprehensive code review checklist following team standards. Use when reviewing pull requests or preparing code for review.
用 Codex 或 Claude 帮你安装 复制这段 Prompt，粘贴到 Codex、Claude 或其他助手里，让它检查 Skill 页面并帮你完成安装。
基于 SOC 职业分类
Integration with AppFlowy project management tool for task tracking, database management, and workspace organization. Use when working with AppFlowy, managing project tasks, creating databases, organizing workspaces, syncing agent work with project tracking, syncing documentation or tasks to AppFlowy, setting up automated sync workflows, or when the user mentions AppFlowy, project tracking, task management, or sync automation. Includes generic sync script that works with ANY project. Supports rich text formatting (bold, italic, code, links, strikethrough) and git pushsync workflow for automated syncing.
Create hierarchical project plans optimized for solo agentic development. Use when planning projects, phases, or tasks that Claude will execute. Produces Claude-executable plans with verification criteria, not enterprise documentation. Handles briefs, roadmaps, phase plans, and context handoffs.
Guides standardized REST API endpoint creation following team conventions. Use when creating new API endpoints.
Safe database migration procedures with backward compatibility, backups, and rollback strategies. Use when creating, modifying, or dropping database schemas. Covers migration creation, testing, execution, and rollback.
Guides production deployment workflow with safety checks and rollback procedures. Use when deploying applications to staging or production environments.
Structured incident response workflow for production issues. Use when handling outages, performance degradation, or user-impacting problems. Covers triage, communication, mitigation, resolution, and post-incident review.
| name | code-review-checklist |
| description | Provides comprehensive code review checklist following team standards. Use when reviewing pull requests or preparing code for review. |
| version | 1.0.0 |
| author | Engineering Team |
| category | custom |
| token_estimate | ~2800 |
<when_to_use> Use this skill when:
Do NOT use this skill when:
Before diving into code, understand what and why:
Review PR Description:
# Fetch PR details
gh pr view <pr-number>
# Check linked issues
gh issue view <issue-number>
Understand the change:
Check CI/CD results:
# View check status
gh pr checks <pr-number>
# Review test results
# All checks should be passing before detailed review
Review code for readability, maintainability, and best practices:
Readability:
Structure:
Error Handling:
Example Quality Check:
# Bad: Unclear, poor error handling
def p(u):
r = requests.get(u)
return r.json()['data']
# Good: Clear, with error handling
def fetch_user_profile(user_id: str) -> dict:
"""Fetch user profile from API.
Args:
user_id: Unique identifier for the user
Returns:
User profile dictionary
Raises:
UserNotFoundError: If user doesn't exist
APIError: If API request fails
"""
try:
response = requests.get(f"{API_BASE}/users/{user_id}")
response.raise_for_status()
return response.json()['data']
except requests.HTTPError as e:
if e.response.status_code == 404:
raise UserNotFoundError(f"User {user_id} not found")
raise APIError(f"Failed to fetch user profile: {e}")
Check for common security vulnerabilities:
Authentication & Authorization:
Input Validation:
Data Protection:
Common Vulnerabilities:
Security Checklist Example:
# Bad: SQL injection vulnerability
def get_user(username):
query = f"SELECT * FROM users WHERE username = '{username}'"
return db.execute(query)
# Good: Parameterized query
def get_user(username: str) -> Optional[User]:
query = "SELECT * FROM users WHERE username = ?"
result = db.execute(query, (username,))
return User.from_db(result) if result else None
# Bad: Sensitive data in logs
logger.info(f"User logged in: {user.email}, password: {user.password}")
# Good: No sensitive data in logs
logger.info(f"User logged in: user_id={user.id}")
Assess performance implications:
Efficiency:
Scalability:
Resource Usage:
Performance Example:
# Bad: N+1 query problem
def get_users_with_posts():
users = User.query.all()
for user in users:
user.posts = Post.query.filter_by(user_id=user.id).all() # N queries!
return users
# Good: Single query with join
def get_users_with_posts():
return User.query.options(joinedload(User.posts)).all()
# Bad: Loading entire dataset into memory
def process_all_records():
records = Record.query.all() # Could be millions!
for record in records:
process(record)
# Good: Batch processing
def process_all_records():
batch_size = 1000
offset = 0
while True:
records = Record.query.limit(batch_size).offset(offset).all()
if not records:
break
for record in records:
process(record)
offset += batch_size
Verify adequate test coverage and quality:
Test Coverage:
Test Quality:
Test Organization:
# Check test coverage
pytest --cov=myapp --cov-report=html tests/
# Review coverage report
open htmlcov/index.html
# Look for:
# - Uncovered critical paths
# - Missing edge case tests
# - Low coverage modules
Test Example:
# Good test: Clear name, arrange-act-assert pattern
def test_user_registration_with_valid_data_creates_user():
# Arrange
user_data = {
"email": "test@example.com",
"password": "SecurePass123!",
"name": "Test User"
}
# Act
result = register_user(user_data)
# Assert
assert result.success is True
assert result.user.email == "test@example.com"
assert User.query.filter_by(email="test@example.com").first() is not None
# Good test: Edge case
def test_user_registration_with_duplicate_email_returns_error():
# Arrange
existing_user = create_user(email="test@example.com")
new_user_data = {"email": "test@example.com", "password": "pass"}
# Act
result = register_user(new_user_data)
# Assert
assert result.success is False
assert "already exists" in result.error_message.lower()
Ensure code is properly documented:
Code Documentation:
External Documentation:
Documentation Example:
# Good: Comprehensive docstring
def calculate_user_score(
user: User,
time_period: timedelta,
weights: Optional[Dict[str, float]] = None
) -> float:
"""Calculate weighted activity score for a user.
Score is based on various user activities weighted by importance.
Higher scores indicate more engaged users.
Args:
user: User object to calculate score for
time_period: How far back to look for activities
weights: Optional custom weights for each activity type.
Defaults to standard weights if not provided.
Returns:
Float between 0.0 and 100.0 representing user engagement score
Raises:
ValueError: If time_period is negative
Example:
>>> user = User.get(123)
>>> score = calculate_user_score(user, timedelta(days=30))
>>> print(f"User engagement score: {score:.2f}")
User engagement score: 78.50
"""
if time_period.total_seconds() < 0:
raise ValueError("time_period must be positive")
weights = weights or DEFAULT_WEIGHTS
# ... implementation
Complete final checks and provide constructive feedback:
Final Checks:
Provide Feedback:
Use constructive, specific feedback:
Good feedback:
**Security Concern**: Line 45 is vulnerable to SQL injection.
Please use parameterized queries instead of string interpolation.
Example:
```python
# Instead of:
query = f"SELECT * FROM users WHERE id = {user_id}"
# Use:
query = "SELECT * FROM users WHERE id = ?"
db.execute(query, (user_id,))
Performance Issue: Lines 120-135 have an N+1 query problem.
Consider using joinedload to fetch posts in a single query.
Nice!: The error handling in process_payment() is excellent.
Clear error messages and proper logging.
**Poor feedback:**
```markdown
❌ "This is bad"
❌ "Rewrite this"
❌ "I don't like this approach"
Approval Criteria:
Approve if:
Request changes if:
<best_practices>
Review in Small BatchesRationale: Review effectiveness drops significantly for PRs over 400 lines. Cognitive load makes it hard to catch issues in large reviews.
Implementation:
Rationale: Systematic checklists catch more issues than ad-hoc reviews.
Implementation:
Rationale: Fast feedback enables team velocity, but thoroughness prevents bugs.
Implementation:
Medium Freedom: Core quality and security checks must be performed, but:
This skill uses approximately 2,800 tokens when fully loaded.
Optimization Strategy:
</best_practices>
<common_pitfalls> Focusing Only on Style
What Happens: Review catches formatting issues but misses logic bugs, security flaws, or performance problems.
Why It Happens: Style issues are easiest to spot; deeper issues require more analysis.
How to Avoid:
Recovery: If approved with issues, create follow-up tickets for missed concerns.
Rubber-Stamp ApprovalWhat Happens: Approve PR without thorough review, trusting tests will catch issues.
Why It Happens: Time pressure, trust in author, or reviewer fatigue.
How to Avoid:
Warning Signs:
What Happens: Review focuses on minor preferences, blocking PRs for subjective style choices.
Why It Happens: Different coding preferences, perfectionism, or miscommunication.
How to Avoid:
Recovery: If feedback was too harsh, follow up with positive message acknowledging good aspects. </common_pitfalls>
Reviewing a Simple Bug FixContext: Small PR fixing a null pointer exception in user profile page.
PR Details:
Review Process:
gh pr view 1234
# Issue: Users without profile pictures get 500 error
# Fix: Add null check before accessing picture URL
# Changed code:
def get_profile_picture_url(user):
if user.profile_picture is None: # ✓ Good: null check added
return DEFAULT_AVATAR_URL
return user.profile_picture.url
# Test added:
def test_user_without_profile_picture_returns_default_avatar():
user = User(profile_picture=None)
url = get_profile_picture_url(user)
assert url == DEFAULT_AVATAR_URL
✓ Edge case tested
Feedback:
LGTM! ✓
Good fix. The null check is clear and the test covers the edge case well.
One suggestion (optional): Consider adding a docstring to clarify when
DEFAULT_AVATAR_URL is returned, for future maintainers.
Outcome: Approved. Simple, well-tested fix. Review took 5 minutes.
Reviewing a New API EndpointContext: PR adds new REST API endpoint for creating user posts.
PR Details:
Review Process:
gh pr view 1235
# Feature: Users can create posts via API
# Includes: Authentication, validation, rate limiting
# Tests: Unit and integration tests included
# Controller code (simplified):
@app.route('/api/v1/posts', methods=['POST'])
@require_auth # ✓ Good: auth required
def create_post():
data = request.get_json()
# ✓ Good: input validation
if not data or 'content' not in data:
return jsonify({'error': 'Content required'}), 400
if len(data['content']) > 10000: # ✓ Good: size limit
return jsonify({'error': 'Content too long'}), 400
# ✓ Good: uses current user from auth
post = Post(
user_id=current_user.id,
content=data['content']
)
db.session.add(post)
db.session.commit()
return jsonify(post.to_dict()), 201
Quality: Good structure, clear logic ✓
Feedback on security:
**Security Issue**: Line 45 - User content is not sanitized before storage.
This could allow XSS attacks if content is rendered without escaping.
Recommendation: Use bleach or similar library to sanitize HTML:
```python
import bleach
content = bleach.clean(
data['content'],
tags=['p', 'br', 'strong', 'em'], # Allow safe tags only
strip=True
)
Or ensure frontend always escapes content when rendering.
4. **Performance review:**
- ✓ Single database insert
- ✓ No N+1 queries
- ⚠️ **Issue found**: No rate limiting implemented despite PR description
**Feedback on performance:**
```markdown
**Missing Feature**: PR description mentions rate limiting, but I don't see
it implemented. This endpoint could be abused to spam posts.
Suggestion: Add rate limiter decorator:
```python
@app.route('/api/v1/posts', methods=['POST'])
@require_auth
@rate_limit(max=10, period=60) # 10 posts per minute
def create_post():
...
5. **Testing review:**
```python
# Tests included:
def test_create_post_with_valid_data():
"""Test successful post creation."""
# ✓ Good: tests happy path
def test_create_post_without_auth():
"""Test auth required."""
# ✓ Good: tests security
def test_create_post_with_empty_content():
"""Test validation."""
# ✓ Good: tests validation
# ⚠️ Missing: test for content length limit
# ⚠️ Missing: test for XSS prevention (once implemented)
Feedback on testing:
**Testing Gap**: Missing test for content length validation (line 47).
Please add:
```python
def test_create_post_with_too_long_content_returns_400():
content = "x" * 10001 # Exceeds 10000 char limit
response = client.post('/api/v1/posts',
json={'content': content},
headers=auth_headers)
assert response.status_code == 400
assert 'too long' in response.json['error'].lower()
6. **Documentation review:**
```yaml
# OpenAPI spec updated:
/api/v1/posts:
post:
summary: Create a new post
requestBody:
content:
application/json:
schema:
type: object
properties:
content:
type: string
maxLength: 10000
responses:
201:
description: Post created successfully
400:
description: Invalid input
401:
description: Authentication required
✓ Good: API docs updated
Final Feedback:
Nice work on the endpoint structure and tests! A few issues to address:
## Required Changes:
1. **Security**: Add XSS protection on user content (see comment on line 45)
2. **Feature**: Implement rate limiting as mentioned in PR description
3. **Testing**: Add test for content length validation
## Suggestions (optional):
- Consider adding a `published_at` timestamp for posts
- Might want to return created post with full details including user info
Once these are addressed, this will be ready to merge. Let me know if you
have questions!
Outcome: Requested changes. Issues found before they reached production. Author addressed concerns and PR was approved on second review.
Reviewing Database MigrationContext: PR adds database migration for new feature.
PR Details:
Review Process:
# Migration: add_notifications_table.py
def upgrade():
op.create_table(
'notifications',
sa.Column('id', sa.Integer(), primary_key=True),
sa.Column('user_id', sa.Integer(), sa.ForeignKey('users.id'), nullable=False),
sa.Column('type', sa.String(50), nullable=False),
sa.Column('content', sa.Text(), nullable=False),
sa.Column('read', sa.Boolean(), default=False),
sa.Column('created_at', sa.DateTime(), default=datetime.utcnow),
)
op.create_index('idx_user_notifications', 'notifications', ['user_id', 'created_at'])
def downgrade():
op.drop_index('idx_user_notifications')
op.drop_table('notifications')
read column might be needed for querying unread notificationstype if notifications are filtered by typeFeedback:
**Performance Suggestion**: Consider adding index on `read` column if you'll
query for unread notifications frequently:
```python
op.create_index('idx_unread_notifications', 'notifications',
['user_id', 'read', 'created_at'])
This composite index will help queries like:
SELECT * FROM notifications WHERE user_id = ? AND read = false ORDER BY created_at DESC
Question: Will notifications be filtered by type frequently? If so,
consider including type in the index as well.
3. **Model review:**
```python
class Notification(db.Model):
id = db.Column(db.Integer, primary_key=True)
user_id = db.Column(db.Integer, db.ForeignKey('users.id'), nullable=False)
type = db.Column(db.String(50), nullable=False)
content = db.Column(db.Text, nullable=False)
read = db.Column(db.Boolean, default=False)
created_at = db.Column(db.DateTime, default=datetime.utcnow)
user = db.relationship('User', backref='notifications')
✓ Model matches migration
Feedback:
**Testing Gap**: Please add test to verify migration runs successfully and
model works as expected:
```python
def test_notification_model():
user = User(email="test@example.com")
notification = Notification(
user=user,
type="mention",
content="You were mentioned in a post"
)
db.session.add_all([user, notification])
db.session.commit()
assert notification.id is not None
assert notification.read is False
assert notification.user == user
**Outcome:** Suggested improvements. Migration is safe but could be optimized. After addressing performance suggestion and adding tests, approved.
</example>
</examples>
<troubleshooting>
<issue>
<name>Unsure If Security Issue Is Serious</name>
**Symptoms:**
- Code looks potentially vulnerable but you're not certain
- Author claims it's safe but you have doubts
**Solution:**
1. Research the specific vulnerability (SQLi, XSS, etc.)
2. Try to write an exploit in test environment
3. Consult security team or senior engineer
4. When in doubt, flag it and discuss
**Better safe than sorry** - flag potential security issues even if uncertain.
</issue>
<issue>
<name>PR Too Large to Review Effectively</name>
**Symptoms:**
- PR has 500+ lines changed
- Multiple unrelated changes
- Difficulty understanding the full impact
**Solution:**
1. Ask author to split into smaller PRs
2. Review in multiple sessions if splitting not feasible
3. Focus on critical paths and security issues first
4. Consider pair reviewing with another engineer
</issue>
<issue>
<name>Tests Pass But Code Seems Wrong</name>
**Symptoms:**
- All tests green but logic looks suspicious
- Tests might not cover the actual usage
**Solution:**
1. Check out the branch locally
2. Run the code with realistic data
3. Add tests for the scenario you're concerned about
4. Discuss your concerns with author
**Trust your instincts** - if something feels wrong, investigate further.
</issue>
</troubleshooting>
<related_skills>
This skill works well with:
- **security-audit**: For deep security review of sensitive changes
- **api-design**: When reviewing new API endpoints
- **database-design**: When reviewing database schema changes
- **testing-strategy**: For evaluating test coverage approach
</related_skills>
<notes>
<limitations>
- Checklist focuses on web application code; adapt for other domains
- Security section covers common issues but not comprehensive security audit
- Performance section provides general guidance; may need profiling for specific concerns
</limitations>
<assumptions>
- Code is in a pull request or branch
- CI/CD pipeline is set up and running
- Team has agreed-upon coding standards
- Author has provided adequate context in PR description
</assumptions>
<version_history>
### Version 1.0.0 (2025-01-20)
- Initial creation
- Comprehensive checklist covering quality, security, performance, testing, documentation
- Examples for different PR types
- Troubleshooting guide
</version_history>
<additional_resources>
- [OWASP Top 10](https://owasp.org/www-project-top-ten/) - Common security vulnerabilities
- [Code Review Best Practices](https://google.github.io/eng-practices/review/)
- Internal: Team coding standards at [internal wiki]
</additional_resources>
</notes>
<success_criteria>
Code review is considered complete and successful when:
1. **Context Understanding Achieved**
- PR description reviewed and understood
- Linked issues examined
- CI/CD checks verified passing
- Change rationale is clear
2. **Quality Standards Met**
- Code is readable and maintainable
- Follows team style guide
- Error handling is appropriate
- No code smells or anti-patterns
3. **Security Verified**
- Authentication/authorization properly implemented
- Input validation present
- No injection vulnerabilities
- Sensitive data properly handled
- Dependencies have no known CVEs
4. **Performance Acceptable**
- No obvious performance issues (N+1 queries, memory leaks)
- Scalability considerations addressed
- Resource usage is reasonable
5. **Testing Adequate**
- Test coverage meets team threshold
- Critical paths tested
- Edge cases covered
- Tests are well-written and independent
6. **Documentation Complete**
- Code is appropriately documented
- Public APIs have docstrings
- External documentation updated if needed
7. **Feedback Provided**
- Constructive, specific feedback given
- Critical issues clearly identified
- Suggestions for improvements offered
- Positive aspects acknowledged
8. **Decision Made**
- Clear approval or request for changes
- No blocking issues remain for approved PRs
</success_criteria>