| name | code-review-playbook |
| description | Use this skill when conducting or improving code reviews. Provides structured review processes, conventional comments patterns, language-specific checklists, and feedback templates. Ensures consistent, constructive, and thorough code reviews across teams. |
| version | 1.0.0 |
| author | AI Agent Hub |
| tags | ["code-review","quality","collaboration","best-practices"] |
Code Review Playbook
Overview
This skill provides a comprehensive framework for effective code reviews that improve code quality, share knowledge, and foster collaboration. Whether you're a reviewer giving feedback or an author preparing code for review, this playbook ensures reviews are thorough, consistent, and constructive.
When to use this skill:
- Reviewing pull requests or merge requests
- Preparing code for review (self-review)
- Establishing code review standards for teams
- Training new developers on review best practices
- Resolving disagreements about code quality
- Improving review processes and efficiency
Code Review Philosophy
Purpose of Code Reviews
Code reviews serve multiple purposes:
- Quality Assurance: Catch bugs, logic errors, and edge cases
- Knowledge Sharing: Spread domain knowledge across the team
- Consistency: Ensure codebase follows conventions and patterns
- Mentorship: Help developers improve their skills
- Collective Ownership: Build shared responsibility for code
- Documentation: Create discussion history for future reference
Principles
Be Kind and Respectful:
- Review the code, not the person
- Assume positive intent
- Praise good solutions
- Frame feedback constructively
Be Specific and Actionable:
- Point to specific lines of code
- Explain why something should change
- Suggest concrete improvements
- Provide examples when helpful
Balance Speed with Thoroughness:
- Aim for timely feedback (< 24 hours)
- Don't rush critical reviews
- Use automation for routine checks
- Focus human review on logic and design
Distinguish Must-Fix from Nice-to-Have:
- Use conventional comments to indicate severity
- Block merges only for critical issues
- Allow authors to defer minor improvements
- Capture deferred work in follow-up tickets
Conventional Comments
A standardized format for review comments that makes intent clear.
Format
<label> [decorations]: <subject>
[discussion]
Labels
| Label | Meaning | Blocks Merge? |
|---|
| praise | Highlight something positive | No |
| nitpick | Minor, optional suggestion | No |
| suggestion | Propose an improvement | No |
| issue | Problem that should be addressed | Usually |
| question | Request clarification | No |
| thought | Idea to consider | No |
| chore | Routine task (formatting, deps) | No |
| note | Informational comment | No |
| todo | Follow-up work needed | Maybe |
| security | Security concern | Yes |
| bug | Potential bug | Yes |
| breaking | Breaking change | Yes |
Decorations
Optional modifiers in square brackets:
| Decoration | Meaning |
|---|
| [blocking] | Must be addressed before merge |
| [non-blocking] | Optional, can be deferred |
| [if-minor] | Only if it's a quick fix |
Examples
praise: Excellent use of TypeScript generics here!
This makes the function much more reusable while maintaining type safety.
---
nitpick [non-blocking]: Consider using const instead of let
This variable is never reassigned, so `const` would be more appropriate:
```typescript
const MAX_RETRIES = 3;
issue: Missing error handling for API call
If the API returns a 500 error, this will crash the application.
Add a try/catch block:
try {
const data = await fetchUser(userId);
} catch (error) {
logger.error('Failed to fetch user', { userId, error });
throw new UserNotFoundError(userId);
}
question: Why use a Map instead of an object here?
Is there a specific reason for this data structure choice?
If it's for performance, could you add a comment explaining?
security [blocking]: API endpoint is not authenticated
The /api/admin/users endpoint is missing authentication middleware.
This allows unauthenticated access to sensitive user data.
Add the auth middleware:
router.get('/api/admin/users', requireAdmin, getUsers);
suggestion [if-minor]: Extract magic number to named constant
Consider extracting this value:
const CACHE_TTL_SECONDS = 3600;
cache.set(key, value, CACHE_TTL_SECONDS);
---
## Review Process
### 1. Before Reviewing
**Check Context:**
- Read the PR/MR description
- Understand the purpose and scope
- Review linked tickets or issues
- Check CI/CD pipeline status
**Verify Automated Checks:**
- [ ] Tests are passing
- [ ] Linting has no errors
- [ ] Type checking passes
- [ ] Code coverage meets targets
- [ ] No merge conflicts
**Set Aside Time:**
- Small PR (< 200 lines): 15-30 minutes
- Medium PR (200-500 lines): 30-60 minutes
- Large PR (> 500 lines): 1-2 hours (or ask to split)
### 2. During Review
**Follow a Pattern:**
1. **High-Level Review** (5-10 minutes)
- Read PR description and understand intent
- Skim all changed files to get overview
- Verify approach makes sense architecturally
- Check that changes align with stated purpose
2. **Detailed Review** (20-45 minutes)
- Line-by-line code review
- Check logic, edge cases, error handling
- Verify tests cover new code
- Look for security vulnerabilities
- Ensure code follows team conventions
3. **Testing Considerations** (5-10 minutes)
- Are tests comprehensive?
- Do tests test the right things?
- Are edge cases covered?
- Is test data realistic?
4. **Documentation Check** (5 minutes)
- Are complex sections commented?
- Is public API documented?
- Are breaking changes noted?
- Is README updated if needed?
### 3. After Reviewing
**Provide Clear Decision:**
- ✅ **Approve**: Code is ready to merge
- 💬 **Comment**: Feedback provided, no action required
- 🔄 **Request Changes**: Issues must be addressed before merge
**Respond to Author:**
- Answer questions promptly
- Re-review after changes made
- Approve when issues resolved
- Thank author for addressing feedback
---
## Review Checklists
### General Code Quality
- [ ] **Readability**: Code is easy to understand
- [ ] **Naming**: Variables and functions have clear, descriptive names
- [ ] **Comments**: Complex logic is explained
- [ ] **Formatting**: Code follows team style guide
- [ ] **DRY**: No unnecessary duplication
- [ ] **SOLID Principles**: Code follows SOLID where applicable
- [ ] **Function Size**: Functions are focused and < 50 lines
- [ ] **Cyclomatic Complexity**: Functions have complexity < 10
### Functionality
- [ ] **Correctness**: Code does what it's supposed to do
- [ ] **Edge Cases**: Boundary conditions handled (null, empty, min/max)
- [ ] **Error Handling**: Errors caught and handled appropriately
- [ ] **Logging**: Appropriate log levels and messages
- [ ] **Input Validation**: User input is validated and sanitized
- [ ] **Output Validation**: Responses match expected schema
### Testing
- [ ] **Test Coverage**: New code has tests
- [ ] **Test Quality**: Tests actually test the right things
- [ ] **Edge Cases Tested**: Tests cover boundary conditions
- [ ] **Error Paths Tested**: Error handling is tested
- [ ] **Test Isolation**: Tests don't depend on each other
- [ ] **Test Naming**: Test names describe what's being tested
### Performance
- [ ] **Database Queries**: N+1 queries avoided
- [ ] **Caching**: Appropriate caching used
- [ ] **Algorithm Efficiency**: No unnecessarily slow algorithms (O(n²) when O(n) possible)
- [ ] **Resource Cleanup**: Files, connections, memory released
- [ ] **Lazy Loading**: Heavy operations deferred when possible
### Security
- [ ] **Authentication**: Protected endpoints require auth
- [ ] **Authorization**: Users can only access their own data
- [ ] **Input Sanitization**: SQL injection, XSS prevented
- [ ] **Secrets Management**: No hardcoded credentials or API keys
- [ ] **Encryption**: Sensitive data encrypted at rest and in transit
- [ ] **HTTPS Only**: Production traffic uses HTTPS
- [ ] **Rate Limiting**: Endpoints protected from abuse
### Language-Specific (JavaScript/TypeScript)
- [ ] **Async/Await**: Promises handled correctly
- [ ] **Type Safety**: TypeScript types are specific (not `any`)
- [ ] **Nullability**: Null checks where needed (`?.` operator)
- [ ] **Array Methods**: Using map/filter/reduce appropriately
- [ ] **Const vs Let**: Using const for immutable values
- [ ] **Arrow Functions**: Appropriate use of arrow vs regular functions
### Language-Specific (Python)
- [ ] **PEP 8**: Follows Python style guide
- [ ] **Type Hints**: Functions have type annotations
- [ ] **List Comprehensions**: Used where appropriate (not overused)
- [ ] **Context Managers**: Using `with` for file/connection handling
- [ ] **Exception Handling**: Specific exceptions caught (not bare `except:`)
- [ ] **F-Strings**: Modern string formatting used
### API Design
- [ ] **RESTful**: Follows REST conventions (if REST API)
- [ ] **Consistent Naming**: Endpoints follow naming patterns
- [ ] **HTTP Methods**: Correct methods used (GET, POST, PUT, DELETE)
- [ ] **Status Codes**: Appropriate HTTP status codes returned
- [ ] **Error Responses**: Consistent error format
- [ ] **Pagination**: Large lists are paginated
- [ ] **Versioning**: API version strategy followed
### Database
- [ ] **Migrations**: Database changes have migrations
- [ ] **Indexes**: Appropriate indexes created
- [ ] **Transactions**: ACID properties maintained
- [ ] **Cascades**: Delete cascades handled correctly
- [ ] **Constraints**: Foreign keys, unique constraints defined
- [ ] **N+1 Queries**: Eager loading used where needed
### Documentation
- [ ] **PR Description**: Clear explanation of changes
- [ ] **Code Comments**: Complex logic explained
- [ ] **API Docs**: Public API documented (JSDoc, docstrings)
- [ ] **README**: Updated if functionality changed
- [ ] **CHANGELOG**: Breaking changes documented
---
## Review Feedback Patterns
### How to Give Constructive Feedback
#### ✅ Good Feedback
issue: This function doesn't handle the case where the user is null
If getUserById() returns null (user not found), this will throw:
Cannot read property 'email' of null
Add a null check:
const user = await getUserById(userId);
if (!user) {
throw new UserNotFoundError(userId);
}
return user.email;
**Why it's good:**
- Specific (points to exact problem)
- Explains the impact (what will happen)
- Suggests a concrete solution
- Provides example code
#### ❌ Bad Feedback
This is wrong. Fix it.
**Why it's bad:**
- Not specific (what's wrong?)
- Not helpful (how to fix?)
- Sounds harsh (not constructive)
---
### How to Receive Feedback
**As an Author:**
1. **Assume Good Intent**: Reviewers are trying to help
2. **Ask Questions**: If feedback is unclear, ask for clarification
3. **Acknowledge Valid Points**: Accept feedback graciously
4. **Explain Your Reasoning**: If you disagree, explain why
5. **Make Changes Promptly**: Address feedback quickly
6. **Say Thank You**: Appreciate the reviewer's time
**Responding to Feedback:**
```markdown
✅ Good Response:
> Good catch! I didn't consider the null case.
> I've added the null check in commit abc123.
✅ Good Response (Disagreement):
> I chose a Map here because we need O(1) lookups on a large dataset.
> An object would work but performs worse at scale (n > 10,000).
> I added a comment explaining this tradeoff.
❌ Bad Response:
> This works fine. Not changing it.
Code Review Anti-Patterns
For Reviewers
❌ Nitpicking Without Clear Value
nitpick: Add a space here
nitpick: Rename this variable to `userInfo` instead of `userData`
nitpick: Move this import to the top
Better: Use automated tools (Prettier, linters) for formatting.
❌ Reviewing Line-by-Line Without Understanding Context
Better: Read PR description first, understand the big picture.
❌ Blocking on Personal Preferences
This should be a class instead of functions.
Better: Only block on objective issues (bugs, security). Discuss preferences separately.
❌ Rewriting Code in Comments
Better: Suggest improvements, don't provide full implementations (unless very helpful).
❌ Review Fatigue (Approving Without Reading)
Better: If you don't have time, say so. Don't rubber-stamp.
For Authors
❌ Giant Pull Requests (> 1000 lines)
Better: Break into smaller, focused PRs.
❌ No Description
Better: Write clear PR description with context and testing notes.
❌ Submitting Failing Tests
Better: Ensure all automated checks pass before requesting review.
❌ Getting Defensive About Feedback
Better: Accept feedback gracefully, discuss respectfully.
❌ Force-Pushing After Review
Better: Add new commits so reviewers can see what changed.
Review Templates
Standard PR Template
## Description
Brief summary of what changed and why.
Fixes #[issue number]
## Type of Change
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Refactoring (no functional changes)
- [ ] Documentation update
## How Has This Been Tested?
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Tested manually in local environment
- [ ] Tested in staging environment
## Checklist
- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published
## Screenshots (if applicable)
[Add screenshots for UI changes]
## Additional Notes
[Any additional context or notes for reviewers]
Security Review Template
## Security Review Checklist
### Authentication & Authorization
- [ ] Endpoints require appropriate authentication
- [ ] User permissions are checked before actions
- [ ] JWT tokens are validated and not expired
- [ ] Session management is secure
### Input Validation
- [ ] All user inputs are validated
- [ ] SQL injection prevented (parameterized queries)
- [ ] XSS prevented (input sanitization, CSP headers)
- [ ] CSRF protection in place for state-changing operations
### Data Protection
- [ ] Sensitive data is encrypted at rest
- [ ] TLS/HTTPS used for data in transit
- [ ] API keys and secrets not hardcoded
- [ ] PII is properly handled (GDPR/CCPA compliance)
### Dependencies
- [ ] No known vulnerabilities in dependencies (npm audit / pip-audit)
- [ ] Dependencies from trusted sources
- [ ] Dependency versions pinned
### Logging & Monitoring
- [ ] Sensitive data not logged (passwords, tokens)
- [ ] Security events logged (failed auth attempts)
- [ ] Rate limiting in place for public endpoints
**Security Concerns Identified:**
[List any security issues found]
**Reviewer Signature:**
[Name, Date]
Review Metrics and Goals
Healthy Review Metrics
| Metric | Target | Purpose |
|---|
| Review Time | < 24 hours | Fast feedback loop |
| PR Size | < 400 lines | Manageable reviews |
| Approval Rate (first review) | 20-30% | Balance speed vs quality |
| Comments per PR | 3-10 | Engaged but not overwhelming |
| Back-and-Forth Rounds | 1-2 | Efficient communication |
| Time to Merge (after approval) | < 2 hours | Avoid stale branches |
Warning Signs
- ⚠️ PRs sitting unreviewed for > 3 days (review capacity issue)
- ⚠️ 90%+ approval rate on first review (rubber-stamping)
- ⚠️ Average PR size > 800 lines (PRs too large)
- ⚠️ 15+ comments per PR (overly nitpicky or unclear requirements)
- ⚠️ 5+ review rounds (poor communication or unclear standards)
Integration with Agents
Code Quality Reviewer Agent
- Uses this playbook when reviewing code
- Applies conventional comments format
- Follows language-specific checklists
- Provides structured, actionable feedback
Backend System Architect
- Reviews for architectural soundness
- Checks design patterns and scalability
- Validates API design against best practices
Frontend UI Developer
- Reviews component structure and patterns
- Checks accessibility and responsive design
- Validates UI/UX implementation
Security Reviewer (Future Agent)
- Focuses on security checklist
- Identifies vulnerabilities
- Validates compliance requirements
Advanced Pattern Detection (Opus 4.5)
Extended Thinking for Complex Reviews
For large PRs or systemic analysis, leverage Opus 4.5's extended thinking to perform deep pattern detection:
When to Use Extended Thinking:
- PR touches > 10 files across multiple modules
- Reviewing security-critical code paths
- Analyzing architectural consistency
- Detecting cross-file code smells
- Evaluating breaking change impact
Extended Thinking Review Pattern:
Based on Anthropic's Extended Thinking API:
import Anthropic from '@anthropic-ai/sdk';
interface ReviewResult {
issues: ReviewIssue[];
patterns: DetectedPattern[];
recommendations: string[];
riskScore: number;
}
async function performDeepCodeReview(
prDiff: string,
codebaseContext: string
): Promise<ReviewResult> {
const anthropic = new Anthropic();
const response = await anthropic.messages.create({
model: 'claude-opus-4-5-20251101',
max_tokens: 16000,
thinking: {
type: 'enabled',
budget_tokens: 8000
},
messages: [{
role: 'user',
content: `
Perform a comprehensive code review analyzing:
## PR Changes
${prDiff}
## Codebase Context
## Analysis Requirements
1. **Systemic Pattern Detection**: Identify recurring patterns (good and bad)
2. **Cross-File Impact**: Trace how changes affect other modules
3. **Security Analysis**: Deep scan for vulnerabilities (OWASP Top 10)
4. **Architectural Consistency**: Verify alignment with existing patterns
5. **Technical Debt Assessment**: Identify debt being added or resolved
Provide structured output with:
- Critical issues (must fix)
- Warnings (should fix)
- Suggestions (nice to have)
- Detected patterns (positive and negative)
- Risk score (1-10)
`
}]
});
(response);
}
Systemic Code Smell Detection
Detect patterns that span multiple files:
interface CodeSmellPattern {
type: 'duplication' | 'coupling' | 'complexity' | 'naming' | 'architecture';
severity: 'critical' | 'warning' | 'info';
locations: FileLocation[];
description: string;
suggestion: string;
}
const SMELL_PATTERNS = {
duplication: {
triggers: ['similar logic in 3+ files', 'copy-paste patterns', 'repeated error handling'],
action: 'Extract to shared utility or base class'
},
coupling: {
triggers: ['circular imports', 'god objects', 'feature envy'],
action: 'Apply dependency injection or interface segregation'
},
complexity: {
triggers: ['nested callbacks > 3 levels', 'functions > 50 lines', 'cyclomatic complexity > 10'],
action: 'Decompose into smaller, focused functions'
},
naming: {
triggers: ['mixed conventions', , ],
:
},
: {
: [, , ],
:
}
};
Security Deep Analysis
Extended thinking enables comprehensive security review:
interface SecurityFinding {
category: 'injection' | 'auth' | 'crypto' | 'exposure' | 'config';
severity: 'critical' | 'high' | 'medium' | 'low';
cwe: string;
location: FileLocation;
description: string;
remediation: string;
}
async function performSecurityReview(
code: string,
context: SecurityContext
): Promise<SecurityFinding[]> {
const anthropic = new Anthropic();
const response = await anthropic.messages.create({
model: 'claude-opus-4-5-20251101',
max_tokens: 12000,
thinking: {
type: 'enabled',
budget_tokens: 6000
},
messages: [{
role: 'user',
:
}]
});
(response);
}
Cross-File Impact Analysis
Trace how changes ripple through the codebase:
interface ImpactAnalysis {
directImpact: FileImpact[];
indirectImpact: FileImpact[];
breakingChanges: BreakingChange[];
testCoverage: TestCoverageGap[];
}
async function analyzeChangeImpact(
changedFiles: string[],
dependencyGraph: DependencyGraph
): Promise<ImpactAnalysis> {
const directlyAffected = new Set<string>();
const indirectlyAffected = new Set<string>();
for (const file of changedFiles) {
const dependents = dependencyGraph.getDependents(file);
dependents.forEach(d => directlyAffected.add(d));
for (const dependent of dependents) {
const secondLevel = dependencyGraph.getDependents(dependent);
secondLevel.forEach(d => {
(!directlyAffected.(d)) {
indirectlyAffected.(d);
}
});
}
}
breakingAnalysis = (
changedFiles,
.(directlyAffected)
);
{
: .(directlyAffected).( ({ : f, : })),
: .(indirectlyAffected).( ({ : f, : })),
: breakingAnalysis.,
: breakingAnalysis.
};
}
Review Automation with Model Tiering
Optimize review costs with intelligent model selection:
type ModelTier = 'opus' | 'sonnet' | 'haiku';
interface ReviewConfig {
model: ModelTier;
thinkingBudget?: number;
focus: string[];
}
function selectReviewModel(pr: PullRequest): ReviewConfig {
if (pr.touchesCriticalPath || pr.isSecurityRelated) {
return {
model: 'opus',
thinkingBudget: 8000,
focus: ['security', 'architecture', 'breaking-changes']
};
}
if (pr.filesChanged > 10 || pr.linesChanged > 500) {
return {
model: 'opus',
thinkingBudget: 5000,
focus: ['patterns', 'impact', 'consistency']
};
}
if (pr.linesChanged > 100) {
{
: ,
: [, , ]
};
}
{
: ,
: [, ]
};
}
Review Comment Generation
Generate conventional comments automatically:
interface GeneratedComment {
label: 'praise' | 'nitpick' | 'suggestion' | 'issue' | 'question' | 'security' | 'bug';
decoration?: 'blocking' | 'non-blocking' | 'if-minor';
subject: string;
discussion: string;
location: { file: string; line: number };
}
function formatConventionalComment(comment: GeneratedComment): string {
const decoration = comment.decoration ? ` [${comment.decoration}]` : '';
return `${comment.label}${decoration}: ${comment.subject}\n\n${comment.discussion}`;
}
Quick Start Guide
For Reviewers:
- Read PR description and understand intent
- Check that automated checks pass
- Do high-level review (architecture, approach)
- Do detailed review (logic, edge cases, tests)
- Use conventional comments for clear communication
- Provide decision: Approve, Comment, or Request Changes
For Authors:
- Write clear PR description
- Perform self-review before requesting review
- Ensure all automated checks pass
- Keep PR focused and reasonably sized (< 400 lines)
- Respond to feedback promptly and respectfully
- Make requested changes or explain reasoning
Skill Version: 2.0.0
Last Updated: 2025-11-27
Maintained by: AI Agent Hub Team
