Code Review Playbook

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:

1. Quality Assurance: Catch bugs, logic errors, and edge cases 2. Knowledge Sharing: Spread domain knowledge across the team 3. Consistency: Ensure codebase follows conventions and patterns 4. Mentorship: Help developers improve their skills 5. Collective Ownership: Build shared responsibility for code 6. 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

issue [blocking]: Missing error handling for API call
If the API returns a 500 error, this will crash. Add try/catch.

security [blocking]: API endpoint is not authenticated
The /api/admin/users endpoint is missing auth middleware.

Load Read("${CLAUDE_SKILL_DIR}/references/conventional-comments.md") for the full format, labels (praise, nitpick, suggestion, issue, question, security, bug, breaking), decorations ([blocking], [non-blocking], [if-minor]), and examples.

---

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

---

CC Built-in Review Commands (2.1.152+)

This playbook is the manual framework; Claude Code ships built-in commands that automate parts of it:

• `/code-review` — reviews the current diff for correctness bugs and reuse/simplification/efficiency cleanups.
• `/code-review --fix` (CC 2.1.152+) — runs the review then applies the findings to your working tree (a bug-hunting review covering correctness plus reuse/simplification/efficiency).
• `/code-review --comment` — posts findings as inline PR comments.
• `/simplify` — CC 2.1.154 changed this: it now runs a cleanup-only review (reuse, simplification, efficiency, altitude) and applies the fixes — it no longer invokes the full /code-review --fix bug-hunt. Reach for /simplify for tidy-ups, /code-review --fix for bug-finding-plus-fix.

Use the built-ins for fast diff-scoped passes; use ork:review-pr for the multi-agent, full-PR review (security + testing + architecture). See #1940 for the overlap analysis between /code-review --comment and ork:review-pr.

---

Skill Version: 2.0.0 Last Updated: 2026-01-08 Maintained by: AI Agent Hub Team

Related Skills

• ork:architecture-patterns - Enforce testing and architectural best practices during code review
• security-scanning - Automated security checks to complement manual review
• ork:testing-unit - Unit testing patterns to verify during review

Rules

Each category has individual rule files in rules/ loaded on-demand:

Total: 5 rules across 4 categories

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: /ork: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

Pull Request Template

Description

<!-- Provide a brief summary of the changes and the motivation behind them. Link to relevant issues. -->

What changed:

Why it changed:

Related issues:

• Fixes #[issue number]
• Relates to #[issue number]

---

Type of Change

<!-- Mark the relevant option(s) with an [x] -->

• [ ] 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, code improvement)
• [ ] Documentation (changes to documentation only)
• [ ] Performance improvement (non-breaking change that improves performance)
• [ ] Test coverage (adding or improving tests)
• [ ] Dependency update (updating libraries or packages)
• [ ] Configuration change (CI/CD, build, or environment config)

---

How Has This Been Tested?

<!-- Describe the tests you ran to verify your changes. Provide instructions so reviewers can reproduce. -->

Automated Tests

• [ ] Unit tests added/updated
• [ ] Integration tests added/updated
• [ ] End-to-end tests added/updated
• [ ] All existing tests pass locally

Manual Testing

• [ ] Tested manually in local environment
• [ ] Tested in staging environment
• [ ] Tested on multiple browsers (if frontend changes)
• [ ] Tested on mobile devices (if applicable)

Test Coverage

Before: X% coverage After: Y% coverage Coverage change: +Z%

---

Testing Instructions for Reviewers

<!-- Provide step-by-step instructions for reviewers to test your changes -->

1. Checkout this branch: git checkout feature/branch-name 2. Install dependencies: npm install (if package.json changed) 3. Run migrations: npm run migrate (if database changed) 4. Start the application: npm run dev 5. Navigate to: http://localhost:3000/feature-path 6. Test scenario:

• Step 1: [Description]
• Step 2: [Description]
• Expected result: [Description]

---

Database Changes

<!-- If this PR includes database changes, describe them here -->

• [ ] No database changes
• [ ] Migration created: migrations/YYYYMMDD_description.sql
• [ ] Rollback migration created
• [ ] Tested migration on staging data
• [ ] Data backfill needed: [Describe]

Schema changes:

• Added table: table_name
• Added column: table_name.column_name
• Modified column: table_name.column_name
• Added index: table_name(column_name)

---

Performance Impact

<!-- Describe any performance implications of your changes -->

• [ ] No performance impact
• [ ] Performance improvement (describe below)
• [ ] Potential performance degradation (describe mitigation below)

Benchmarks:

• Before: [metric] = X
• After: [metric] = Y
• Change: ±Z%

Load test results: (if applicable)

• Requests per second: X
• p95 latency: Xms
• Error rate: X%

---

Security Considerations

<!-- Describe security implications and how they're addressed -->

• [ ] No security impact
• [ ] Security improvement (describe below)
• [ ] New authentication/authorization added
• [ ] Input validation implemented
• [ ] Secrets management updated (using environment variables, not hardcoded)

Security review needed for:

• [ ] Payment processing
• [ ] User authentication
• [ ] Data access controls
• [ ] API endpoint protection
• [ ] Sensitive data handling

---

Breaking Changes

<!-- If this PR introduces breaking changes, describe them and provide migration path -->

• [ ] No breaking changes
• [ ] Breaking changes (describe below)

What breaks:

Migration path for users:

Deprecation timeline:

• Version X.Y: Deprecation warning added
• Version X.Y+1: Breaking change introduced
• Version X.Y+2: Old functionality removed

---

Deployment Notes

<!-- Special instructions for deploying this change -->

• [ ] Standard deployment (no special steps)
• [ ] Requires configuration changes (describe below)
• [ ] Requires manual steps (describe below)
• [ ] Feature flag recommended

Pre-deployment steps: 1. [Step description] 2. [Step description]

Post-deployment steps: 1. [Step description] 2. [Step description]

Rollback plan:

• [ ] Can rollback via standard deployment revert
• [ ] Requires manual rollback steps (describe below)

---

Checklist

<!-- Ensure all items are checked before requesting review -->

Code Quality

• [ ] 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 removed commented-out code and debug statements
• [ ] I have removed console.log / print statements (unless intentional)
• [ ] My changes generate no new warnings (linter, TypeScript, etc.)

Testing

• [ ] 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
• [ ] I have tested edge cases and error scenarios
• [ ] I have tested backwards compatibility (if applicable)

Documentation

• [ ] I have made corresponding changes to the documentation
• [ ] I have updated the README (if applicable)
• [ ] I have updated API docs (if API changed)
• [ ] I have updated inline comments for complex logic
• [ ] I have updated CHANGELOG (if applicable)

Dependencies

• [ ] Any dependent changes have been merged and published in downstream modules
• [ ] I have checked for dependency vulnerabilities (npm audit / pip-audit)
• [ ] I have updated lock files (package-lock.json, Pipfile.lock)

Security

• [ ] I have reviewed for security vulnerabilities
• [ ] I have not introduced hardcoded secrets or API keys
• [ ] I have validated and sanitized user inputs
• [ ] I have added appropriate authentication/authorization checks

---

Screenshots

<!-- Add screenshots for UI changes. Before/after comparisons are especially helpful. -->

Before

[Screenshot or N/A]

After

[Screenshot or N/A]

---

Additional Context

<!-- Add any other context about the PR here -->

Related PRs:

• [Link to related PR in another repo]

Follow-up work:

• [ ] [Description of follow-up task] (#issue-number)

Known limitations:

• [Describe any known issues or limitations]

Questions for reviewers:

• [Any specific questions or areas you'd like reviewers to focus on]

---

Reviewer Checklist

<!-- For reviewers: Use this checklist during review -->

• [ ] Code changes make sense and align with PR description
• [ ] Tests adequately cover new code
• [ ] No obvious bugs or logic errors
• [ ] Performance implications are acceptable
• [ ] Security considerations are addressed
• [ ] Documentation is updated
• [ ] Breaking changes are clearly communicated
• [ ] Code follows project conventions and style

---

PR Created: [Date] Ready for Review: [Date] Target Merge Date: [Date]

Code Review Feedback Template

Review Summary

PR Title: [Title] PR Number: #[Number] Author: [Author Name] Reviewer: [Your Name] Review Date: YYYY-MM-DD

Overall Assessment: ✅ Approve / 💬 Comment / 🔄 Request Changes

---

Executive Summary

<!-- High-level assessment in 2-3 sentences -->

[Brief summary of what the PR does well and main areas for improvement]

---

Detailed Feedback

🎉 Strengths

<!-- Highlight positive aspects of the code -->

1. [Aspect]: [Specific praise]

• Example: Excellent error handling in processPayment function
• The try/catch blocks handle all edge cases gracefully

2. [Aspect]: [Specific praise]

3. [Aspect]: [Specific praise]

---

🔴 Blocking Issues

<!-- Issues that must be fixed before merge -->

Issue 1: [Title]

Severity: 🔴 Critical / 🟠 High Location: file/path.ts:123-145 Label: bug / security / breaking

Problem: [Detailed description of the issue]

Impact: [What happens if this is not fixed]

Suggested Fix:

// Before (current code)
const user = await getUser(userId);
return user.email; // Crashes if user is null

// After (suggested fix)
const user = await getUser(userId);
if (!user) {
  throw new UserNotFoundError(userId);
}
return user.email;

Additional Context: [Any relevant background information]

---

Issue 2: [Title]

[Same structure as Issue 1]

---

🟡 Suggestions

<!-- Improvements that enhance quality but are not blocking -->

Suggestion 1: [Title]

Severity: 🟡 Medium / 🟢 Low Location: file/path.ts:67-89 Label: suggestion / refactor

Current Code:

// Example of current implementation

Suggested Improvement:

// Proposed improvement

Reasoning: [Why this change improves the code - readability, performance, maintainability, etc.]

Effort: Low / Medium / High

---

❓ Questions

<!-- Clarification questions for the author -->

1. Q: Why did you choose to use a Map instead of an object in UserCache?

• Context: [Relevant context]
• Concern: [What you're unsure about]

2. Q: [Question text]

---

💡 Nitpicks (Optional)

<!-- Minor, non-blocking suggestions -->

1. nitpick [non-blocking]: Consider renaming userData to userProfile

• Location: src/user/service.ts:45
• Reason: More specific and aligns with domain language

2. nitpick [non-blocking]: Add blank line between imports and code

• Location: Multiple files
• Reason: Improves readability

---

Specific File Reviews

src/services/payment.ts

Overall: ✅ Looks good / ⚠️ Needs work

Line-by-Line Comments:

Lines 23-45: processPayment function

praise: Excellent use of async/await here!

The error handling is comprehensive and the retry logic
is a great addition for handling transient failures.

Lines 67-89: calculateTotal function

issue: Missing validation for negative prices

If a product has a negative price (data corruption scenario),
this will calculate an incorrect total.

Add validation:

if (items.some(item => item.price < 0)) { throw new InvalidPriceError('Product price cannot be negative'); }

Lines 120-135: refundPayment function

question: Should we add idempotency here?

If this function is called multiple times for the same payment,
will it create duplicate refunds? Consider adding an idempotency
check using the payment ID.

---

src/tests/payment.test.ts

Overall: ✅ Looks good / ⚠️ Needs work

Comments:

• ✅ Good coverage of happy path scenarios
• ⚠️ Missing tests for error scenarios (API timeout, invalid response)
• 💡 Consider adding parameterized tests for different payment amounts

---

Test Coverage Review

Current Coverage: X% Target Coverage: Y% Gap: ±Z%

Well-Tested:

• ✅ Happy path scenarios
• ✅ Input validation
• ✅ [Other areas]

Needs More Tests:

• ⚠️ Error handling paths
• ⚠️ Edge cases (null, empty, max values)
• ⚠️ Integration with payment gateway

Suggested Tests:

describe('processPayment', () => {
  it('should retry on transient failures', async () => {
    // Test implementation
  });

  it('should throw on permanent failures', async () => {
    // Test implementation
  });

  it('should handle timeout gracefully', async () => {
    // Test implementation
  });
});

---

Performance Review

Concerns: ✅ None / ⚠️ Some / 🔴 Major

Observations:

• [ ] No obvious performance issues
• [ ] Database queries are optimized (no N+1)
• [ ] Caching is used appropriately
• [ ] No memory leaks detected

Performance Notes: [Specific observations about performance]

Suggested Improvements: [If any performance optimizations are recommended]

---

Security Review

Concerns: ✅ None / ⚠️ Some / 🔴 Critical

Security Checklist:

• [ ] Input validation and sanitization
• [ ] SQL injection prevention (parameterized queries)
• [ ] XSS prevention (output encoding)
• [ ] Authentication checks on protected routes
• [ ] Authorization checks (users can only access their own data)
• [ ] No hardcoded secrets or API keys
• [ ] Sensitive data encrypted
• [ ] HTTPS enforced

Security Notes: [Specific security observations]

---

Documentation Review

Documentation Quality: ✅ Good / ⚠️ Needs Work / 🔴 Missing

Checklist:

• [ ] Code has inline comments for complex logic
• [ ] Public APIs are documented (JSDoc/docstrings)
• [ ] README updated (if applicable)
• [ ] CHANGELOG updated (if applicable)
• [ ] Migration guide provided (if breaking changes)

Documentation Gaps: [List any missing or unclear documentation]

---

Architecture & Design

Alignment with Architecture: ✅ Good / ⚠️ Concerns / 🔴 Conflicts

Observations:

• Does this PR follow existing patterns?
• Are there any architectural concerns?
• Does it introduce new patterns (are they justified)?
• Is there unnecessary coupling?

Suggestions: [Any architectural improvements]

---

Checklist for Author

Before marking as resolved, ensure:

• [ ] All blocking issues addressed
• [ ] Questions answered
• [ ] Tests added for new code
• [ ] Documentation updated
• [ ] Self-review completed
• [ ] CI/CD checks passing

---

Next Steps

Immediate Actions Required

1. [Action 1]: [Description]

• Owner: Author
• Priority: High
• Estimated time: [X hours/days]

2. [Action 2]: [Description]

Follow-Up Work (Optional)

• [ ] [Follow-up item 1] - Create issue #[number]
• [ ] [Follow-up item 2] - Create issue #[number]

---

Timeline

• Review Requested: [Date]
• Initial Review Completed: [Date]
• Changes Requested: [Date]
• Re-Review Needed: Yes / No
• Target Merge Date: [Date]

---

Additional Notes

[Any other context, thoughts, or suggestions for the author]

---

Review Decision

<!-- Choose one -->

✅ Approve

Reasoning: [Why this PR is ready to merge]

Conditions (if any):

• [ ] [Condition that must be met before merge]

---

💬 Comment

Reasoning: [Why you're providing comments without blocking]

Non-Blocking Suggestions: X Questions: Y

---

🔄 Request Changes

Reasoning: [Why changes are required before merge]

Blocking Issues: X Required Changes: [Summary]

Estimated Rework Time: [X hours/days]

---

Reviewer Signature: [Your Name] Date: YYYY-MM-DD

---

For Re-Review

<!-- Fill this out when re-reviewing after changes -->

Changes Reviewed: [Date] Status: ✅ All issues addressed / 🔄 Some issues remain

Remaining Issues:

• [ ] [Issue description]

New Comments: [Any new feedback after reviewing changes]

Final Decision: ✅ Approve / 🔄 Request additional changes

Code Review Checklist

Use this comprehensive checklist when reviewing code to ensure thorough and consistent reviews.

---

Pre-Review Setup

• [ ] Read PR Description: Understand intent and scope
• [ ] Check CI Status: All automated checks passing (tests, linting, type checking)
• [ ] Review Size: PR is manageable (< 400 lines preferred, flag if > 800)
• [ ] Linked Issues: PR references relevant tickets/issues
• [ ] No Merge Conflicts: Branch is up to date with target branch

---

High-Level Review (Architecture & Design)

Overall Approach

• [ ] Problem-Solution Alignment: Changes solve the stated problem
• [ ] Scope Appropriate: No unrelated changes included
• [ ] Architecture Consistency: Follows existing patterns and conventions
• [ ] Design Patterns: Appropriate patterns used (not over-engineered)
• [ ] Separation of Concerns: Each module/function has single responsibility

Code Organization

• [ ] File Structure: New files in appropriate directories
• [ ] Module Boundaries: Clear separation between modules
• [ ] Coupling: Low coupling between modules
• [ ] Cohesion: High cohesion within modules

---

Code Quality

Readability

• [ ] Clear Intent: Code purpose is obvious from reading
• [ ] Naming Conventions: Variables, functions, classes have descriptive names
• [ ] Consistent Style: Follows team style guide
• [ ] Comments: Complex logic explained (not what, but why)
• [ ] Magic Numbers: Constants extracted and named
• [ ] Code Formatting: Properly formatted (via Prettier/Black)

Maintainability

• [ ] DRY Principle: No unnecessary code duplication
• [ ] Function Size: Functions < 50 lines, focused on single task
• [ ] Cyclomatic Complexity: Functions have complexity < 10
• [ ] Nesting Depth: No deeply nested code (< 4 levels)
• [ ] Dead Code: No commented-out code or unused variables
• [ ] TODO Comments: Tracked in issue tracker, not just in code

---

Functionality

Correctness

• [ ] Logic Errors: No obvious bugs or logic errors
• [ ] Edge Cases: Boundary conditions handled
• [ ] Null/undefined/None handled
• [ ] Empty arrays/strings handled
• [ ] Zero values handled
• [ ] Negative numbers (if applicable)
• [ ] Maximum/minimum values
• [ ] Data Types: Correct data types used
• [ ] Off-by-One Errors: Array indices and loops correct

Error Handling

• [ ] Try-Catch Blocks: Errors caught where appropriate
• [ ] Specific Exceptions: Catching specific errors, not generic catch-all
• [ ] Error Messages: Clear, actionable error messages
• [ ] Error Logging: Errors logged with appropriate context
• [ ] Error Propagation: Errors bubble up or handled at right level
• [ ] Graceful Degradation: System handles failures gracefully
• [ ] User Feedback: Users see helpful error messages (not stack traces)

Input Validation

• [ ] Required Fields: Required inputs validated
• [ ] Data Types: Input types validated
• [ ] Ranges: Min/max values enforced
• [ ] Format Validation: Email, phone, URL formats validated
• [ ] Sanitization: User input sanitized (XSS prevention)
• [ ] SQL Injection Prevention: Parameterized queries used

---

Testing

Test Coverage

• [ ] Tests Exist: New code has tests
• [ ] Coverage Metrics: Code coverage meets targets (80%+)
• [ ] Happy Path: Main functionality tested
• [ ] Error Paths: Error scenarios tested
• [ ] Edge Cases: Boundary conditions tested
• [ ] Regression Tests: Previous bugs have tests

Test Quality

• [ ] Test Names: Clearly describe what's being tested
• [ ] AAA Pattern: Arrange-Act-Assert structure
• [ ] Test Isolation: Tests don't depend on each other
• [ ] No Flaky Tests: Tests pass consistently
• [ ] Realistic Data: Test data resembles production data
• [ ] Mocking Strategy: Appropriate use of mocks vs real dependencies

Test Types

• [ ] Unit Tests: Business logic tested in isolation
• [ ] Integration Tests: Component interactions tested
• [ ] E2E Tests: Critical user flows tested (if applicable)
• [ ] Performance Tests: Performance benchmarks (if applicable)

---

Performance

Efficiency

• [ ] Algorithm Complexity: Efficient algorithms used
• [ ] No O(n²) where O(n) is possible
• [ ] No unnecessary nested loops
• [ ] Database Queries: Optimized queries
• [ ] No N+1 query problems
• [ ] Eager loading where needed
• [ ] Appropriate indexes exist
• [ ] Caching: Used where appropriate
• [ ] Lazy Loading: Heavy operations deferred when possible

Resource Management

• [ ] Memory Leaks: No obvious memory leaks
• [ ] Event listeners cleaned up
• [ ] Subscriptions unsubscribed
• [ ] Timers/intervals cleared
• [ ] File Handles: Files closed after use
• [ ] Database Connections: Connections properly managed (pooling)
• [ ] Large Collections: Large arrays/lists handled efficiently

---

Security

Authentication & Authorization

• [ ] Auth Required: Protected endpoints require authentication
• [ ] Permission Checks: User permissions verified before actions
• [ ] JWT Validation: Tokens validated and not expired
• [ ] Session Security: Sessions managed securely
• [ ] Password Requirements: Password complexity enforced

Data Protection

• [ ] Sensitive Data: No secrets in code (API keys, passwords)
• [ ] Environment Variables: Secrets in environment, not hardcoded
• [ ] Encryption: Sensitive data encrypted (passwords, PII)
• [ ] HTTPS Only: Production uses HTTPS
• [ ] Secure Headers: Security headers set (CSP, X-Frame-Options)

Common Vulnerabilities

• [ ] SQL Injection: Parameterized queries used
• [ ] XSS: User input sanitized, output encoded
• [ ] CSRF: CSRF tokens for state-changing operations
• [ ] Insecure Dependencies: No known vulnerabilities (npm audit)
• [ ] Rate Limiting: Public endpoints rate-limited
• [ ] File Upload Security: File type/size restrictions

---

API Design (if applicable)

REST Principles

• [ ] Resource Naming: Plural nouns, kebab-case
• [ ] HTTP Methods: Correct methods (GET, POST, PUT, DELETE)
• [ ] Status Codes: Appropriate status codes (200, 201, 400, 404, 500)
• [ ] Idempotency: PUT/DELETE are idempotent
• [ ] Pagination: Large lists paginated
• [ ] Versioning: API version strategy followed

Request/Response

• [ ] Request Validation: Request body validated
• [ ] Response Format: Consistent response structure
• [ ] Error Format: Standardized error responses
• [ ] Field Naming: Consistent naming (camelCase or snake_case)
• [ ] Timestamps: ISO 8601 format

---

Database (if applicable)

Schema Design

• [ ] Migrations: Database changes have migrations
• [ ] Rollback Migrations: Rollback scripts provided
• [ ] Indexes: Appropriate indexes created
• [ ] Foreign Keys: Relationships properly defined
• [ ] Constraints: Unique, not-null constraints defined
• [ ] Normalization: Appropriate normalization level

Queries

• [ ] Parameterized Queries: No SQL injection risk
• [ ] Query Optimization: Queries are efficient
• [ ] Transaction Management: ACID properties maintained
• [ ] Connection Pooling: Database connections pooled

---

Frontend (if applicable)

React/Component Best Practices

• [ ] Component Size: Components < 300 lines
• [ ] Props Validation: PropTypes or TypeScript interfaces
• [ ] Key Props: Proper keys in lists (not index)
• [ ] State Management: State lifted appropriately
• [ ] Side Effects: useEffect dependencies correct
• [ ] Memoization: Expensive calculations memoized

Accessibility

• [ ] Keyboard Navigation: All interactive elements keyboard accessible
• [ ] ARIA Labels: Screen reader support
• [ ] Color Contrast: WCAG AA compliance (4.5:1 ratio)
• [ ] Focus Indicators: Visible focus states
• [ ] Semantic HTML: Using proper HTML elements

Performance

• [ ] Bundle Size: No unnecessary dependencies
• [ ] Code Splitting: Large components lazy-loaded
• [ ] Image Optimization: Images compressed and sized appropriately
• [ ] Render Optimization: No unnecessary re-renders

---

Documentation

Code Documentation

• [ ] Inline Comments: Complex logic explained
• [ ] JSDoc/Docstrings: Public APIs documented
• [ ] Type Annotations: TypeScript/Python type hints used
• [ ] Examples: Usage examples provided (if library/utility)

Project Documentation

• [ ] README Updated: If functionality changed
• [ ] API Docs Updated: If API changed
• [ ] CHANGELOG Updated: Breaking changes documented
• [ ] Migration Guide: If breaking changes
• [ ] Architecture Diagrams: Updated (if relevant)

---

Language-Specific Checks

JavaScript/TypeScript

• [ ] Async/Await: Promises handled with async/await
• [ ] Error Handling: Async errors caught
• [ ] Type Safety: No any types (TypeScript)
• [ ] Null Safety: Optional chaining (?.) used
• [ ] Const vs Let: Immutable values use const
• [ ] Arrow Functions: Used appropriately
• [ ] Template Literals: Used instead of string concatenation
• [ ] Destructuring: Used for readability
• [ ] Modern Syntax: ES6+ features used appropriately

Python

• [ ] PEP 8: Follows Python style guide
• [ ] Type Hints: Function annotations provided
• [ ] F-Strings: Modern string formatting
• [ ] List Comprehensions: Used appropriately (not overused)
• [ ] Context Managers: with for file/connection handling
• [ ] Exception Handling: Specific exceptions, no bare except:
• [ ] Generator Expressions: Used for memory efficiency
• [ ] Dataclasses: Used for data structures (Python 3.7+)

---

Dependencies

Dependency Management

• [ ] Necessary Dependencies: New dependencies justified
• [ ] Version Pinning: Dependencies pinned to specific versions
• [ ] Lock Files Updated: package-lock.json / Pipfile.lock updated
• [ ] No Vulnerabilities: npm audit / pip-audit clean
• [ ] License Compatibility: Dependency licenses compatible
• [ ] Bundle Size Impact: Large dependencies justified

---

CI/CD & Deployment

Continuous Integration

• [ ] All Checks Pass: Linting, tests, type checking
• [ ] Build Succeeds: Application builds successfully
• [ ] No Warnings: Build generates no warnings

Deployment Considerations

• [ ] Feature Flags: New features behind flags (if applicable)
• [ ] Database Migrations: Safe for zero-downtime deployment
• [ ] Backward Compatibility: No breaking changes to API contracts
• [ ] Rollback Plan: Can revert if issues arise
• [ ] Environment Variables: New env vars documented

---

Breaking Changes

Impact Assessment

• [ ] No Breaking Changes (or marked as breaking change)
• [ ] Migration Path: Clear upgrade path provided
• [ ] Deprecation Warnings: Old APIs deprecated, not removed immediately
• [ ] Version Bump: Semantic versioning followed (major version bump)
• [ ] Stakeholder Notification: Affected teams notified

---

Follow-Up Work

Technical Debt

• [ ] TODO Items: Tracked in issue tracker
• [ ] Known Limitations: Documented
• [ ] Refactoring Needed: Follow-up issues created
• [ ] Performance Optimizations: Future work identified

---

Final Checks

Before Approving

• [ ] All Blocking Issues Addressed: Critical problems resolved
• [ ] Questions Answered: Author responded to clarifications
• [ ] Tests Pass: All automated checks green
• [ ] Documentation Complete: Necessary docs updated
• [ ] Security Reviewed: No security vulnerabilities
• [ ] Performance Acceptable: No significant regressions

Approval Decision

• ✅ Approve: Ready to merge, no blocking issues
• 💬 Comment: Feedback provided, no action required
• 🔄 Request Changes: Blocking issues must be addressed

---

Common Issues to Watch For

Frequent Problems

• ❌ Missing null/undefined checks
• ❌ No error handling in async operations
• ❌ N+1 database queries
• ❌ Hardcoded secrets or API keys
• ❌ Missing input validation
• ❌ Commented-out code
• ❌ console.log / print statements
• ❌ TODO comments without issue references
• ❌ Magic numbers (unnamed constants)
• ❌ Deep nesting (> 4 levels)
• ❌ Large functions (> 50 lines)
• ❌ Missing tests for new code

---

Review Etiquette

For Reviewers

• [ ] Use conventional comments (praise, issue, suggestion, question)
• [ ] Be specific and actionable
• [ ] Explain the "why" behind suggestions
• [ ] Acknowledge good work (use "praise" labels)
• [ ] Assume positive intent
• [ ] Provide timely feedback (< 24 hours)

For Authors

• [ ] Respond to all comments
• [ ] Ask for clarification if needed
• [ ] Accept feedback gracefully
• [ ] Make changes promptly
• [ ] Thank reviewers for their time

---

Checklist Version: 1.0.0 Skill: code-review-playbook v1.0.0 Last Updated: 2025-10-31

Conventional Comments Examples

Real-world examples of conventional comments for different scenarios.

Comment Format

<label> (<category>): <subject>

<discussion>

Labels & When to Use

🔴 issue - Must Fix (Blocking)

Security vulnerability:

issue (security): SQL injection vulnerability in user lookup.

The `user_id` parameter is concatenated directly into the query.

Instead of:
  cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")

Use parameterized queries:
  cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,))

Breaking bug:

issue (bug): This will crash when `items` is empty.

`items[0]` throws IndexError. Add a guard:
  if not items:
      return default_value

🟡 suggestion - Should Consider

Better approach:

suggestion (performance): Consider using `dict.get()` for O(1) lookup.

Current loop is O(n) for each check:
  for item in items:
      if item['id'] == target_id: ...

With a dict:
  items_by_id = {item['id']: item for item in items}
  result = items_by_id.get(target_id)

Readability improvement:

suggestion (readability): Extract this into a well-named function.

This 15-line block calculates shipping cost. A function like
`calculate_shipping_cost(order, destination)` would make the
caller's intent clearer and enable reuse.

⚪ nitpick - Non-blocking Polish

Style:

nitpick (style): Prefer `is None` over `== None` per PEP 8.

  if value is None:  # ✓
  if value == None:  # ✗

Naming:

nitpick (naming): `data` is generic. Consider `user_profile` or `response_payload`.

🟢 praise - Positive Reinforcement

praise: Excellent test coverage! These edge cases would have caught
real bugs. The property-based test for serialization roundtrips is
particularly clever.

praise: This refactor reduced complexity from 15 to 4. Much easier
to reason about now. Great work!

🔵 question - Clarification Needed

question (design): Why did we choose Redis over PostgreSQL for sessions?

Not blocking, just want to understand the tradeoff for the ADR.

question: Is `timeout=30` intentional? Other endpoints use 60s.

📝 thought - Non-blocking Observation

thought: We might want to add rate limiting here eventually.
Not for this PR, but worth a follow-up issue.

Anti-Patterns to Avoid

❌ Vague criticism:

This code is bad.

✅ Specific and actionable:

issue (complexity): This function has 6 levels of nesting.
Consider early returns or extracting helper functions.

❌ Demanding tone:

You need to fix this. This is wrong.

✅ Collaborative tone:

suggestion: Consider using X because Y. What do you think?

PR Review Walkthrough Example

A complete example of reviewing a PR that adds user authentication.

The PR

Title: feat(auth): Add JWT authentication middleware Files Changed: 4 files (+180, -12)

File 1: auth/middleware.py

def authenticate_request(request):
    token = request.headers.get('Authorization')
    if not token:
        raise AuthError('Missing token')

    payload = jwt.decode(token, SECRET_KEY)  # ← Issue: No algorithm specified
    request.user = payload['user_id']
    return request

Review Comments (Conventional Format)

1. Security Issue (Blocking)

🔴 **issue (security):** JWT decode without algorithm is vulnerable to algorithm confusion attacks.

An attacker could forge tokens by switching to 'none' algorithm.

**Suggestion:**
​```python
payload = jwt.decode(token, SECRET_KEY, algorithms=['HS256'])
​```

Reference: https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/

2. Error Handling (Suggestion)

🟡 **suggestion (error-handling):** Consider catching specific JWT exceptions.

​```python
try:
    payload = jwt.decode(token, SECRET_KEY, algorithms=['HS256'])
except jwt.ExpiredSignatureError:
    raise AuthError('Token expired', code=401)
except jwt.InvalidTokenError:
    raise AuthError('Invalid token', code=401)
​```

This gives users actionable error messages.

3. Praise

🟢 **praise:** Clean separation of auth logic into middleware. This will make testing much easier!

4. Nitpick (Non-blocking)

⚪ **nitpick (style):** Consider using `get()` with default for cleaner None check:

​```python
token = request.headers.get('Authorization', '')
if not token:
​```

Summary Comment

## Review Summary

Thanks for adding auth! The implementation is clean and well-structured.

### Must Fix (Blocking)
- [ ] Add `algorithms` parameter to `jwt.decode()` (security)

### Should Consider
- [ ] Specific JWT exception handling for better UX

### Nice to Have
- [ ] Minor style improvements

LGTM once the security issue is addressed! 🔐

Key Takeaways

1. Lead with praise - Acknowledge good work first 2. Use conventional comments - Clear severity (issue/suggestion/nitpick) 3. Provide context - Explain why, not just what 4. Include examples - Show the fix, don't just describe it 5. Summarize - Group by priority for easy action

Conventional Comments

A standardized format for review comments that makes intent clear.

Format

<label> [decorations]: <subject>

[discussion]

Labels

Decorations

Examples

// 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.

---

issue: Missing error handling for API call
If the API returns a 500 error, this will crash the application.
Add a try/catch block with proper error logging.

---

security [blocking]: API endpoint is not authenticated
The `/api/admin/users` endpoint is missing authentication middleware.

---

suggestion [if-minor]: Extract magic number to named constant

Code Review Patterns & Practices

This reference guide covers common review patterns, conventional comments, depth levels, and security focus areas.

---

Conventional Comments

Use structured comment prefixes to set clear expectations for the author. This pattern originated from conventionalcomments.org.

Standard Prefixes

Decorators (Optional)

Add decorators to clarify urgency:

• blocking: Must be fixed before merge
• non-blocking: Can be addressed later
• if-minor: Only if it's a quick fix

Examples:

issue (blocking): SQL injection risk - sanitize input
suggestion (non-blocking): Consider memoizing this calculation
nitpick (if-minor): Add newline at end of file

---

Review Depth Levels

Adjust review depth based on change type, author experience, and risk.

Level 1: Light Review (5-10 minutes)

When to use:

• Documentation-only changes
• Test-only additions
• Config/dependency updates (low risk)
• Experienced author, small change

What to check:

• [ ] CI passes (lint, tests, type check)
• [ ] No obvious security issues (secrets, SQL injection)
• [ ] Commit message follows convention
• [ ] PR description explains "why"

Example comment:

LGTM! CI green, change is isolated.

nitpick: Consider adding a test case for edge case X

---

Level 2: Standard Review (15-30 minutes)

When to use:

• New features (medium complexity)
• Bug fixes with logic changes
• Refactoring existing code
• Moderate author experience

What to check:

• [ ] Correctness: Logic handles edge cases (empty arrays, null, undefined)
• [ ] Tests: Unit tests cover happy path + edge cases (80%+ coverage)
• [ ] Type safety: No any types, proper null checks
• [ ] Error handling: Try/catch for async, meaningful error messages
• [ ] Performance: No obvious N+1 queries, unnecessary re-renders
• [ ] Security: Input validation, no hardcoded secrets
• [ ] Documentation: JSDoc for public APIs, README updates if needed

Example comment:

issue (blocking): Missing error handling in fetchAnalysis()
- What happens if API returns 500?
- Add try/catch and show user-friendly error message

suggestion: Consider extracting validation logic to Zod schema
- Reusable across API + frontend
- Centralized validation rules

praise: Great test coverage! Love the edge case tests.

---

Level 3: Deep Review (30-60 minutes)

When to use:

• Critical path features (auth, payments, data loss risk)
• Architecture changes (new patterns, major refactors)
• Security-sensitive code (authentication, authorization, PII)
• Junior author or unfamiliar codebase area

What to check (includes Level 2, plus):

• [ ] Architecture: Fits existing patterns, doesn't introduce coupling
• [ ] Scalability: Handles 10x growth (users, data volume)
• [ ] Maintainability: Code is readable, well-documented, DRY
• [ ] Security (OWASP): Injection, auth, XSS, CSRF, exposure, misconfiguration
• [ ] Observability: Logging, error tracking, metrics
• [ ] Migration path: Database migrations, backward compatibility
• [ ] Rollback plan: Feature flags, circuit breakers
• [ ] E2E tests: Critical flows tested end-to-end

Example comment:

security (blocking): Multiple OWASP Top 10 violations detected

1. SQL Injection (A03):
   - Line 45: User input concatenated into SQL query
   - Fix: Use parameterized queries (SQLAlchemy already supports this)

2. Broken Access Control (A01):
   - Line 78: No check if user owns this analysis
   - Fix: Add ownership check before allowing update

3. Security Logging Failures (A09):
   - No audit log for data deletion
   - Fix: Log user_id, analysis_id, timestamp to audit table

performance: Potential N+1 query in loop (lines 102-110)
- Fetching chunks individually instead of batch query
- Fix: Use `SELECT WHERE id IN (...)` to fetch all at once
- Expected impact: 50ms → 5ms for 10 chunks

suggestion: Add feature flag for this rollout
- New analysis pipeline is high-risk change
- Allows gradual rollout + quick rollback
- Example: `if feature_flags.is_enabled('new_pipeline', user_id):`

question: How does this handle concurrent updates?
- Two users editing same analysis simultaneously
- Do we need optimistic locking (version field)?

praise: Excellent error messages! These will make debugging much easier.

---

Security Focus Areas

OWASP Top 10 (2021) Checklist

Use this checklist for every security-sensitive change:

A01: Broken Access Control

• [ ] Check user authentication before accessing resources
• [ ] Verify user owns the resource (analysis, artifact, etc.)
• [ ] Validate permissions for create/read/update/delete
• [ ] No direct object references without authorization (e.g., /api/analyses/123)

Example violation:

# BAD: Anyone can delete any analysis
@router.delete("/analyses/{analysis_id}")
async def delete_analysis(analysis_id: int):
    await repo.delete(analysis_id)  # ❌ No auth check!

# GOOD: Check ownership first
@router.delete("/analyses/{analysis_id}")
async def delete_analysis(
    analysis_id: int,
    current_user: User = Depends(get_current_user)
):
    analysis = await repo.get(analysis_id)
    if analysis.user_id != current_user.id:
        raise HTTPException(403, "Not authorized")
    await repo.delete(analysis_id)

---

A02: Cryptographic Failures

• [ ] No passwords/API keys in plaintext (use bcrypt, environment variables)
• [ ] Sensitive data encrypted at rest (PII, payment info)
• [ ] HTTPS enforced for all endpoints
• [ ] No sensitive data in logs or error messages

Example violation:

# BAD: Password in plaintext
user = User(email=email, password=password)  # ❌

# GOOD: Hash password
from passlib.context import CryptContext
pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
user = User(email=email, password_hash=pwd_context.hash(password))

---

A03: Injection (SQL, NoSQL, Command)

• [ ] Use parameterized queries (never string concatenation)
• [ ] Validate/sanitize all user input
• [ ] Use ORM (SQLAlchemy) instead of raw SQL when possible
• [ ] Escape HTML output to prevent XSS

Example violation:

# BAD: SQL injection risk
query = f"SELECT * FROM analyses WHERE url = '{user_url}'"  # ❌
results = db.execute(query)

# GOOD: Parameterized query
query = "SELECT * FROM analyses WHERE url = :url"
results = db.execute(query, {"url": user_url})

# BEST: Use ORM
results = db.query(Analysis).filter(Analysis.url == user_url).all()

---

Remember: Code review is about collaboration, not gatekeeping. Explain reasoning, suggest alternatives, and celebrate good work. Every review is a learning opportunity for both author and reviewer.

Rule Categories

1. TypeScript Quality (typescript) — HIGH — 1 rule

TypeScript-specific review patterns: strict types, Zod validation, exhaustive checks, React 19 APIs.

• typescript-quality.md — Type safety, no any, Zod for API responses, assertNever in switches

2. Python Quality (python) — HIGH — 1 rule

Python-specific review patterns: Pydantic v2, ruff formatting, async safety, type hints.

• python-quality.md — Pydantic validators, ruff compliance, mypy strict, async timeout protection

3. Security Baseline (security) — CRITICAL — 1 rule

Security review baseline that applies to all languages: OWASP Top 10, secrets, authentication.

• security-baseline.md — No hardcoded secrets, auth on all endpoints, input validation, dependency audit

4. Linting (linting) — HIGH — 2 rules

Biome 2.0+ unified linting and formatting, ESLint migration, CI integration.

• linting-biome-setup.md — Biome installation, ESLint migration, gradual adoption with overrides
• linting-biome-rules.md — Production config, type-aware rules, CI integration, 421 built-in rules

[Rule Name]

[Brief description — 1-2 sentences.]

Incorrect:

// Bad pattern

Correct:

// Good pattern

Key rules:

• [Rule 1]
• [Rule 2]
• [Rule 3]

Reference: [link]

Biome Rule Configuration and CI Integration

Incorrect — default config without key rules enabled:

{
  "linter": { "enabled": true }
  // Missing: noUnusedVariables, noUnusedImports, noExplicitAny
  // Missing: type-aware rules (Biome 2.0+)
}

Correct — production Biome configuration:

{
  "$schema": "https://biomejs.dev/schemas/2.0.0/schema.json",
  "formatter": {
    "enabled": true,
    "indentStyle": "space",
    "indentWidth": 2,
    "lineWidth": 100
  },
  "linter": {
    "enabled": true,
    "domains": {
      "types": "recommended"
    },
    "rules": {
      "recommended": true,
      "correctness": {
        "noUnusedVariables": "error",
        "noUnusedImports": "error"
      },
      "suspicious": {
        "noExplicitAny": "warn"
      }
    }
  },
  "javascript": {
    "formatter": {
      "quoteStyle": "single",
      "trailingCommas": "all"
    }
  }
}

Biome 2.0+ type inference features:

• Reads .d.ts from node_modules for type-aware rules
• noFloatingPromises: Catches unhandled promises — Biome 2.4 moved this to the types domain, so set linter.domains.types: "recommended" or the rule silently under-functions
• Multi-file analysis: Cross-module diagnostics

Correct — CI integration (GitHub Actions):

# .github/workflows/lint.yml
name: Lint
on: [push, pull_request]
jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: biomejs/setup-biome@v2
      - run: biome ci .

Biome vs ESLint comparison:

Key decisions:

• Start with recommended rules, tighten over time
• Enable noUnusedVariables and noUnusedImports as errors
• Enable noFloatingPromises for TypeScript projects (v2.0+)
• Use biome ci in CI (strict), biome check locally
• Config strictness: recommended -> warn -> error progression

Biome Linting Setup and Migration

Incorrect — complex multi-tool setup:

// 4+ config files: .eslintrc, .prettierrc, .prettierignore, .editorconfig
// 127+ npm packages for ESLint + Prettier + plugins
// 3-5s lint time for 10k lines

Correct — Biome single-tool setup:

# Install (single binary, no plugins needed)
npm install --save-dev --save-exact @biomejs/biome

# Initialize config
npx @biomejs/biome init

# Check (lint + format in one command)
npx @biomejs/biome check .

# Fix all auto-fixable issues
npx @biomejs/biome check --write .

# CI mode (strict, fails on errors)
npx @biomejs/biome ci .

Correct — ESLint migration:

# Auto-migrate ESLint configuration
npx @biomejs/biome migrate eslint --write

Common rule mappings:

Correct — gradual adoption with overrides:

{
  "overrides": [
    {
      "include": ["*.test.ts", "*.spec.ts"],
      "linter": {
        "rules": {
          "suspicious": { "noExplicitAny": "off" }
        }
      }
    },
    {
      "include": ["legacy/**"],
      "linter": { "enabled": false }
    }
  ]
}

Key decisions:

• New projects: Start with Biome directly
• Existing projects: Migrate gradually with overrides
• CI: Use biome ci for strict mode, biome check for local dev
• Speed: ~200ms for 10k lines vs 3-5s with ESLint+Prettier

Python Quality Review Rules

Review rules for Python code. Focused on Pydantic v2, async safety, and type strictness.

Pydantic v2 Patterns

# VIOLATION: No validation on input models
class UserInput(BaseModel):
    email: str      # Accepts any string
    age: int        # Accepts negative numbers

# CORRECT: Constrained fields + validators
from pydantic import BaseModel, Field, model_validator

class UserInput(BaseModel):
    email: str = Field(pattern=r'^[\w\.-]+@[\w\.-]+\.\w+$')
    age: int = Field(ge=0, le=150)

    @model_validator(mode='after')
    def validate_fields(self) -> 'UserInput':
        if self.age < 13 and '@' not in self.email:
            raise ValueError('Minors require valid parent email')
        return self

Type Hints (mypy Strict)

# VIOLATION: Missing type hints
def process(data):
    result = []
    for item in data:
        result.append(item.name)
    return result

# CORRECT: Full type hints
def process(data: list[UserModel]) -> list[str]:
    result: list[str] = []
    for item in data:
        result.append(item.name)
    return result

Async Safety

# VIOLATION: No timeout on external calls
async def fetch_user(user_id: str) -> User:
    response = await httpx.get(f"/users/{user_id}")
    return User(**response.json())

# CORRECT: Timeout protection
import asyncio

async def fetch_user(user_id: str) -> User:
    async with asyncio.timeout(10):
        response = await httpx.get(f"/users/{user_id}")
        response.raise_for_status()
        return User.model_validate(response.json())

Ruff Compliance

# All Python files must pass:
# ruff check --select ALL
# ruff format --check

# Key rules enforced:
# - No unused imports
# - No f-strings in logging (use % formatting)
# - No bare except clauses
# - No mutable default arguments

Incorrect — missing validation and timeout:

class UserInput(BaseModel):
    email: str  # No validation!
    age: int    # Accepts negative

async def fetch_user(user_id: str) -> User:
    # No timeout! May hang forever
    response = await httpx.get(f"/users/{user_id}")
    return User(**response.json())

Correct — constrained fields with timeout protection:

from pydantic import BaseModel, Field, model_validator
import asyncio

class UserInput(BaseModel):
    email: str = Field(pattern=r'^[\w\.-]+@[\w\.-]+\.\w+$')
    age: int = Field(ge=0, le=150)

    @model_validator(mode='after')
    def validate_fields(self) -> 'UserInput':
        if self.age < 13 and '@' not in self.email:
            raise ValueError('Minors require valid parent email')
        return self

async def fetch_user(user_id: str) -> User:
    async with asyncio.timeout(10):  # Timeout protection
        response = await httpx.get(f"/users/{user_id}")
        response.raise_for_status()
        return User.model_validate(response.json())

Review Checklist

Security Baseline Review Rules

Security checks that apply to ALL languages. These are merge-blocking findings.

No Hardcoded Secrets

# VIOLATION: Secrets in code
API_KEY = "sk-1234567890abcdef"
DB_PASSWORD = "admin123"
JWT_SECRET = "mysecret"

# CORRECT: Environment variables
API_KEY = os.environ["API_KEY"]
DB_PASSWORD = os.environ.get("DB_PASSWORD")

// VIOLATION: Secrets in code
const apiKey = "sk-1234567890abcdef";

// CORRECT: Environment variables
const apiKey = process.env.API_KEY;

Detection patterns: Look for variables named *_KEY, *_SECRET, *_PASSWORD, *_TOKEN with string literal values.

Authentication on All Endpoints

# VIOLATION: Unprotected endpoint
@app.get("/api/admin/users")
async def list_users():
    return await db.get_all_users()

# CORRECT: Auth middleware
@app.get("/api/admin/users")
async def list_users(user: User = Depends(require_admin)):
    return await db.get_all_users()

// VIOLATION: No auth
router.get('/api/users', getUsers);

// CORRECT: Auth middleware
router.get('/api/users', requireAuth, getUsers);

Input Validation

Violation — SQL injection via f-string interpolation:

# VIOLATION: SQL injection
query = f"SELECT * FROM users WHERE id = {user_id}"

Correct — parameterized query prevents injection:

query = "SELECT * FROM users WHERE id = $1"
await db.execute(query, user_id)

Violation — XSS via raw innerHTML assignment:

// VIOLATION: XSS — raw HTML insertion
element.innerHTML = userInput;

Correct — textContent auto-escapes HTML entities:

element.textContent = userInput;

Dependency Audit

# Must run before merge:
npm audit          # JavaScript/TypeScript
pip-audit          # Python

Debug/Development Code

# VIOLATION: Debug code in production
import pdb; pdb.set_trace()
print(f"DEBUG: user password is {password}")
set -x  # In scripts with secrets in scope

# CORRECT: Remove before commit
logger.debug("User authenticated", extra={"user_id": user.id})

Incorrect — hardcoded secrets, no auth, SQL injection:

# Hardcoded secret
API_KEY = "sk-1234567890abcdef"

# No auth protection
@app.get("/api/admin/users")
async def list_users():
    return await db.get_all_users()

# SQL injection vulnerability
query = f"SELECT * FROM users WHERE id = {user_id}"

Correct — env vars, auth middleware, parameterized queries:

# Environment variables
API_KEY = os.environ["API_KEY"]

# Auth middleware
@app.get("/api/admin/users")
async def list_users(user: User = Depends(require_admin)):
    return await db.get_all_users()

# Parameterized query
query = "SELECT * FROM users WHERE id = $1"
await db.execute(query, user_id)

Review Checklist

TypeScript Quality Review Rules

Review rules for TypeScript and React code. Flag violations, suggest fixes.

No any Types

// VIOLATION: any defeats the type system
function processData(data: any) { ... }
const result: any = await fetch(url);

// CORRECT: Use proper types or unknown
function processData(data: UserInput) { ... }
const result: unknown = await fetch(url);

Zod Runtime Validation

All API responses MUST be validated with Zod at the boundary:

// VIOLATION: Trust the network
const data = await response.json();
const data = await response.json() as User;  // Type assertion, not validation

// CORRECT: Validate at boundary
import { z } from 'zod';
const UserSchema = z.object({
  id: z.uuid(),       // z.guid() for the permissive (non-RFC9562) variant
  email: z.email(),
  role: z.enum(['admin', 'user']),
});
const data = UserSchema.parse(await response.json());

Exhaustive Switch Statements

All switch statements MUST have assertNever default:

// VIOLATION: Non-exhaustive — adding a new status silently falls through
switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
}

// CORRECT: Compiler catches missing cases
function assertNever(x: never): never {
  throw new Error(`Unexpected value: ${x}`);
}

switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
  default: return assertNever(status);
}

React 19 APIs

// REQUIRE: useOptimistic for mutations
const [optimistic, addOptimistic] = useOptimistic(state, reducer);

// REQUIRE: useFormStatus in form submit buttons
const { pending } = useFormStatus();

// REQUIRE: use() for Suspense-aware data fetching
const data = use(promise);

// REQUIRE: Skeleton loading, not spinners
function CardSkeleton() {
  return <div className="animate-pulse">...</div>;
}

Incorrect — any types, no validation, non-exhaustive switch:

// Defeats type system
function processData(data: any) { return data.email; }

// Trust the network - no validation!
const data = await response.json();

// Non-exhaustive switch
switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
}  // Adding 'pending' silently breaks

Correct — proper types, Zod validation, exhaustive switch:

import { z } from 'zod';

// Proper types
const UserSchema = z.object({
  id: z.uuid(),
  email: z.email(),
});
function processData(data: z.infer<typeof UserSchema>) { return data.email; }

// Validate at boundary
const data = UserSchema.parse(await response.json());

// Exhaustive switch with assertNever
function assertNever(x: never): never {
  throw new Error(`Unexpected: ${x}`);
}
switch (status) {
  case 'active': return 'Active';
  case 'inactive': return 'Inactive';
  default: return assertNever(status);  // Compiler catches missing cases
}

Review Checklist

Review PR $ARGUMENTS

PR Context (Auto-Fetched)

• Recent PRs: !gh pr list --limit 10 --json number,title,author,state,createdAt,updatedAt --jq '.[] | "\(.number): \(.title) [\(.state)] by \(.author.login)"' 2>/dev/null || echo "Unable to fetch PR list"
• Current Branch PRs: !gh pr list --head "$(git branch --show-current 2>/dev/null || echo '')" --json number,title,state --jq '.[] | "\(.number): \(.title)"' 2>/dev/null | head -5 || echo "No PRs found for current branch"
• GitHub CLI Available: !which gh >/dev/null 2>&1 && echo "✅ Yes" || echo "❌ Not found - install GitHub CLI"

Your Task

Review pull request #$ARGUMENTS. Use the PR list above to locate the PR, then:

1. Fetch PR details using: gh pr view $ARGUMENTS --json title,author,state,createdAt,updatedAt,comments 2. Review the diff: gh pr diff $ARGUMENTS 3. Check changed files: gh pr diff $ARGUMENTS --name-only 4. Review comments: gh pr view $ARGUMENTS --comments

Review Checklist

Code Quality

• [ ] Code follows project style guide
• [ ] No obvious bugs or logic errors
• [ ] Error handling is appropriate
• [ ] Edge cases are considered
• [ ] Code is readable and maintainable

Testing

• [ ] Tests are included for new functionality
• [ ] Existing tests still pass
• [ ] Test coverage is adequate
• [ ] Integration tests updated if needed

Security

• [ ] No sensitive data exposed
• [ ] Input validation present
• [ ] Authentication/authorization correct
• [ ] No SQL injection or XSS vulnerabilities

Performance

• [ ] No obvious performance issues
• [ ] Database queries are optimized
• [ ] No unnecessary API calls
• [ ] Caching considered where appropriate

Documentation

• [ ] Code is well-commented
• [ ] README/docs updated if needed
• [ ] API changes documented
• [ ] Breaking changes noted

Review Feedback Template

Use conventional comments format:

<label> [decorations]: <subject>

[discussion]

Labels: praise, nitpick, suggestion, issue, question, thought, chore, note, todo, security, bug, breaking

Example Comments:

• praise: Great use of TypeScript types here
• suggestion [non-blocking]: Consider extracting this into a helper function
• issue [blocking]: This will fail if the array is empty
• security [blocking]: API key should not be logged

Review Process

1. Read the PR description - Understand the goal 2. Review changed files - Focus on the diff 3. Check tests - Ensure coverage and correctness 4. Test locally (if possible) - Verify it works 5. Provide constructive feedback - Be specific and kind 6. Approve or request changes - Based on blocking issues

Common Issues to Watch For

• Breaking changes without migration path
• Performance regressions in critical paths
• Security vulnerabilities (SQL injection, XSS, etc.)
• Missing error handling for edge cases
• Inconsistent patterns with existing codebase
• Over-engineering simple solutions
• Under-testing complex logic

Approval Criteria

Approve if:

• Code quality is good
• Tests are adequate
• No blocking issues
• Follows project conventions

Request changes if:

• Blocking bugs or security issues
• Missing critical tests
• Significant style violations
• Breaking changes without discussion