Code Reviewer

Code Reviewer

A comprehensive code review skill that analyzes pull requests and code changes for quality, security, maintainability, and best practices.

When This Skill Activates

This skill activates when you:

• Ask for a code review
• Request a PR review
• Mention reviewing changes
• Say "review this" or "check this code"

Review Process

Phase 1: Context Gathering

1. Get changed files

   git diff main...HEAD --name-only
   git log main...HEAD --oneline

2. Get the diff

   git diff main...HEAD

3. Understand project context

• Read relevant documentation
• Check existing patterns in similar files
• Identify project-specific conventions

Phase 2: Analysis Categories

1. Correctness

• [ ] Logic is sound and matches requirements
• [ ] Edge cases are handled
• [ ] Error handling is appropriate
• [ ] No obvious bugs or typos

2. Security

• [ ] No hardcoded secrets or credentials
• [ ] Input validation and sanitization
• [ ] SQL injection prevention
• [ ] XSS prevention (for frontend)
• [ ] Authentication/authorization checks
• [ ] Safe handling of user data

3. Performance

• [ ] No N+1 queries
• [ ] Appropriate caching
• [ ] Efficient algorithms
• [ ] No unnecessary computations
• [ ] Memory efficiency

4. Code Quality

• [ ] Follows DRY principle
• [ ] Follows KISS principle
• [ ] Appropriate abstractions
• [ ] Clear naming conventions
• [ ] Proper typing (if TypeScript)
• [ ] No commented-out code

5. Testing

• [ ] Tests cover new functionality
• [ ] Tests cover edge cases
• [ ] Test assertions are meaningful
• [ ] No brittle tests

6. Documentation

• [ ] Complex logic is explained
• [ ] Public APIs have documentation
• [ ] JSDoc/TSDoc for functions
• [ ] README updated if needed

7. Maintainability

• [ ] Code is readable
• [ ] Consistent style
• [ ] Modular design
• [ ] Separation of concerns

Phase 3: Output Format

Use this structured format for review feedback:

# Code Review

## Summary
Brief overview of the changes (2-3 sentences).

## Issues by Severity

### Critical
Must fix before merge.

- [ ] **Issue Title**: Description with file:line reference

### High
Should fix before merge unless there's a good reason.

- [ ] **Issue Title**: Description with file:line reference

### Medium
Consider fixing, can be done in follow-up.

- [ ] **Issue Title**: Description with file:line reference

### Low
Nice to have improvements.

- [ ] **Issue Title**: Description with file:line reference

## Positive Highlights
What was done well in this PR.

## Suggestions
Optional improvements that don't require immediate action.

## Approval Status
- [ ] Approved
- [ ] Approved with suggestions
- [ ] Request changes

Common Issues to Check

Security Issues

Performance Issues

Code Quality Issues

Testing Issues

Language-Specific Guidelines

TypeScript

• Use unknown instead of any for untyped values
• Prefer interface for public APIs, type for unions
• Use strict mode settings
• Avoid as assertions when possible

React

• Follow Hooks rules
• Use useCallback/useMemo appropriately (not prematurely)
• Prefer function components
• Use proper key props in lists
• Avoid prop drilling with Context

Python

• Follow PEP 8 style guide
• Use type hints
• Use f-strings for formatting
• Prefer list comprehensions over map/filter
• Use context managers for resources

Go

• Handle errors explicitly
• Use named returns for clarity
• Keep goroutines simple
• Use channels for communication
• Avoid package-level state

Before Approving

Confirm the following:

• [ ] All critical issues are addressed
• [ ] Tests pass locally
• [ ] No merge conflicts
• [ ] Commit messages are clear
• [ ] Documentation is updated
• [ ] Breaking changes are documented

Scripts

Run the review checklist script:

python scripts/review_checklist.py <pr-number>

References

• references/checklist.md - Complete review checklist
• references/security.md - Security review guidelines
• references/patterns.md - Common patterns and anti-patterns

Code Reviewer

A Claude Code skill for comprehensive code review of pull requests and code changes.

Installation

This skill is part of the agent-playbook collection.

Usage

When reviewing code, simply ask:

You: Review this PR
You: Check my changes
You: Review the code in src/auth/

The skill will: 1. Analyze the changes 2. Check against security best practices 3. Evaluate code quality 4. Review test coverage 5. Provide structured feedback

Review Categories

Output Format

Reviews are structured with severity levels:

• Critical: Must fix before merge
• High: Should fix before merge
• Medium: Consider fixing
• Low: Nice to have improvements

Scripts

Generate a review checklist:

python scripts/review_checklist.py

References

• OWASP Top 10
• Google Engineering Practices
• Clean Code

Code Review Checklist

Use this checklist for systematic code reviews.

Pre-Review

• [ ] I understand what this PR is trying to achieve
• [ ] I have read the linked issues/tickets
• [ ] I have checked the base branch is correct
• [ ] I have verified the PR is not a draft

Code Review

Correctness

• [ ] Code implements the stated requirements
• [ ] Edge cases are handled
• [ ] Error handling is appropriate
• [ ] No obvious bugs
• [ ] Input validation is present

Security

• [ ] No hardcoded secrets/credentials
• [ ] User input is validated/sanitized
• [ ] SQL/NoSQL injection prevention
• [ ] XSS prevention (for web)
• [ ] CSRF protection (for state-changing operations)
• [ ] Authentication/authorization is correct
• [ ] Sensitive data is handled securely

Performance

• [ ] No N+1 queries
• [ ] Appropriate caching (if applicable)
• [ ] Efficient algorithm/data structure choice
• [ ] No unnecessary database/network calls
• [ ] Pagination for large datasets
• [ ] Indexes used where appropriate

Code Quality

• [ ] Code is readable and understandable
• [ ] Naming is clear and consistent
• [ ] No dead/commented-out code
• [ ] No duplicate code
• [ ] Appropriate abstractions
• [ ] Follows DRY, KISS, YAGNI
• [ ] Type definitions are accurate (if typed)

Testing

• [ ] Tests cover new functionality
• [ ] Tests cover edge cases
• [ ] Tests are meaningful (not tautologies)
• [ ] No hardcoded test data that makes tests brittle
• [ ] All tests pass
• [ ] Test coverage not decreased

Documentation

• [ ] Complex logic has comments
• [ ] Public APIs are documented
• [ ] Breaking changes are noted
• [ ] README/API docs updated if needed
• [ ] Migration guide provided for breaking changes

Maintainability

• [ ] Code is modular
• [ ] Separation of concerns
• [ ] Easy to modify
• [ ] Easy to test
• [ ] Follows project conventions

Style

• [ ] Consistent formatting
• [ ] Follows project style guide
• [ ] No lint errors
• [ ] No console.log/debugger left in

Post-Review

• [ ] Provided clear, actionable feedback
• [ ] Explained reasoning for suggestions
• [ ] Flagged blocking issues separately from nice-to-haves
• [ ] Recognized good work in the PR

Common Patterns and Anti-Patterns

Patterns to Encourage

Error Handling

Good:

async function getUser(id: string) {
  const user = await db.users.findById(id);
  if (!user) {
    throw new NotFoundError(`User ${id} not found`);
  }
  return user;
}

Bad:

async function getUser(id: string) {
  return await db.users.findById(id); // Returns null, not handled
}

Async/Await

Good:

const result = await fetch(url);
const data = await result.json();

Bad:

fetch(url).then(r => r.json()).then(data => {
  // Nested callbacks
});

Early Returns

Good:

function process(user) {
  if (!user) return null;
  if (!user.active) return null;
  return user.data;
}

Bad:

function process(user) {
  if (user) {
    if (user.active) {
      return user.data;
    }
  }
  return null;
}

Destructuring

Good:

const { name, email } = user;

Bad:

const name = user.name;
const email = user.email;

Anti-Patterns to Catch

Magic Numbers

Bad:

if (user.role === 5) { ... }

Good:

const Role = { ADMIN: 5, USER: 1 };
if (user.role === Role.ADMIN) { ... }

Neglected Promise Rejection

Bad:

fetch(url).then(data => processData(data));

Good:

fetch(url)
  .then(data => processData(data))
  .catch(error => logError(error));

Any Type

Bad:

function parse(data: any) { ... }

Good:

function parse(data: unknown): Result { ... }

Deep Nesting

Bad:

if (a) {
  if (b) {
    if (c) {
      doSomething();
    }
  }
}

Good:

if (!a) return;
if (!b) return;
if (!c) return;
doSomething();

Large Functions

Bad: Functions > 50 lines

Good: Split into smaller, focused functions

God Objects

Bad: Classes/methods that do everything

Good: Single Responsibility Principle

Shotgun Surgery

Bad: Adding a feature requires changing many files

Good: Good separation of concerns

React Specific

Hooks Dependencies

Bad:

useEffect(() => {
  fetchData(userId);
}, []); // Missing userId dependency

Good:

useEffect(() => {
  fetchData(userId);
}, [userId]);

State Updates

Bad:

setCount(count + 1);
setCount(count + 1);

Good:

setCount(c => c + 2);

Key Props

Bad:

items.map((item, i) => <Item key={i} />)

Good:

items.map(item => <Item key={item.id} />)

Backend Specific

N+1 Query

Bad:

for user in users:
  posts = db.query("SELECT * FROM posts WHERE user_id = ?", user.id)

Good:

user_ids = [u.id for u in users]
posts = db.query("SELECT * FROM posts WHERE user_id IN ?", user_ids)

Transaction Handling

Bad:

db.transfer(a, b, amount)  # No transaction

Good:

with db.transaction():
  db.transfer(a, b, amount)

Security Review Guidelines

OWASP Top 10 Coverage

A01:2021 – Broken Access Control

• [ ] Users can only access their own data
• [ ] API endpoints have proper authentication
• [ ] Admin actions require admin role
• [ ] No IDOR (Insecure Direct Object References)
• [ ] Proper authorization checks on all endpoints

A02:2021 – Cryptographic Failures

• [ ] Passwords are hashed (bcrypt/argon2)
• [ ] HTTPS is enforced
• [ ] Sensitive data is encrypted at rest
• [ ] No weak cipher suites
• [ ] Proper key management

A03:2021 – Injection

• [ ] Parameterized queries for SQL
• [ ] Input validation and sanitization
• [ ] ORM used safely
• [ ] No command injection from user input
• [ ] No LDAP injection

A04:2021 – Insecure Design

• [ ] Rate limiting on auth endpoints
• [ ] Proper logout functionality
• [ ] Session timeout is reasonable
• [ ] No security through obscurity

A05:2021 – Security Misconfiguration

• [ ] Debug mode off in production
• [ ] Error messages don't leak information
• [ ] Default credentials changed
• [ ] Security headers configured
• [ ] CORS configured correctly

A06:2021 – Vulnerable Components

• [ ] Dependencies up to date
• [ ] No known vulnerabilities in deps
• [ ] Unused dependencies removed

A07:2021 – Auth Failures

• [ ] Strong password policy
• [ ] No brute force protection needed (rate limiting)
• [ ] MFA implemented for sensitive operations
• [ ] Session IDs are random

A08:2021 – Software/Data Integrity

• [ ] Dependencies from trusted sources
• [ ] CI/CD has integrity checks
• [ ] Verify data integrity

A09:2021 – Logging Failures

• [ ] Security events logged
• [ ] Logs don't contain sensitive data
• [ ] Log tampering protection
• [ ] Audit trail for critical operations

A10:2021 – SSRF

• [ ] No arbitrary URL fetching from user input
• [ ] Allowlist for external calls
• [ ] Network segmentation

Frontend Security

• [ ] XSS prevention
• [ ] CSRF tokens
• [ ] Content Security Policy
• [ ] Subresource Integrity
• [ ] No dangerouslySetInnerHTML with user content

Backend Security

• [ ] Input validation on all endpoints
• [ ] Output encoding
• [ ] Prepared statements
• [ ] Principle of least privilege
• [ ] Secure file upload handling

Infrastructure Security

• [ ] Secrets in environment variables
• [ ] No secrets in code
• [ ] Proper RBAC
• [ ] Network security rules
• [ ] Regular security updates