Code Review Playbook
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.
Overview
- •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
code
<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
typescript
// ✅ Good: Clear, specific, actionable 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:
typescript
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:
typescript
router.get('/api/admin/users', requireAdmin, getUsers);
suggestion [if-minor]: Extract magic number to named constant
Consider extracting this value:
typescript
const CACHE_TTL_SECONDS = 3600; cache.set(key, value, CACHE_TTL_SECONDS);
code
--- ## 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 ### 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 - [ ] **Rate Limiting**: Endpoints protected from abuse --- ## Quick Start Guide **For Reviewers:** 1. Read PR description and understand intent 2. Check that automated checks pass 3. Do high-level review (architecture, approach) 4. Do detailed review (logic, edge cases, tests) 5. Use conventional comments for clear communication 6. Provide decision: Approve, Comment, or Request Changes **For Authors:** 1. Write clear PR description 2. Perform self-review before requesting review 3. Ensure all automated checks pass 4. Keep PR focused and reasonably sized (< 400 lines) 5. Respond to feedback promptly and respectfully 6. Make requested changes or explain reasoning --- **Skill Version**: 2.0.0 **Last Updated**: 2026-01-08 **Maintained by**: AI Agent Hub Team ## Related Skills - `test-standards-enforcer` - Enforce testing best practices during code review - `security-scanning` - Automated security checks to complement manual review - `unit-testing` - Unit test patterns to verify during review - `a11y-testing` - Accessibility testing requirements for UI code reviews ## Capability Details ### review-process **Keywords:** code review, pr review, review process, feedback **Solves:** - How to review PRs - Conventional comments format - Review best practices ### quality-checks **Keywords:** readability, solid, dry, complexity, naming **Solves:** - Check code quality - SOLID principles review - Cyclomatic complexity ### security-review **Keywords:** security, authentication, authorization, injection, xss **Solves:** - Security review checklist - Find vulnerabilities - Auth validation ### language-specific **Keywords:** typescript, python, type hints, async await, pep8 **Solves:** - TypeScript review - Python review - Language-specific patterns ### pr-template **Keywords:** pr template, pull request, description **Solves:** - PR description format - Review checklist ## Available Scripts - **`scripts/review-pr.md`** - Dynamic PR review with auto-fetched GitHub data - Auto-fetches: PR title, author, state, changed files, diff stats, comments count - Usage: `/review-pr [PR-number]` - Requires: GitHub CLI (`gh`) - Uses `$ARGUMENTS` and `!command` for live PR data - **`assets/review-feedback-template.md`** - Static review feedback template - **`assets/pr-template.md`** - PR description template