code-review-standards

Code Review Standards

When to Use

• Reviewing pull requests
• Performing code reviews
• Defining review criteria
• Establishing review process

Overview

Code review standards ensure consistent, thorough reviews that catch bugs before they reach production. This skill aggregates criteria from specialized skills.

Review Framework

4-Level Severity Classification

1. CRITICAL 🔴 - Must fix before merge
2. Security vulnerabilities
3. Data loss risks
4. Authentication bypasses
5. SQL injection risks

1. HIGH 🟠 - Should fix before merge
2. TypeScript strict mode violations
3. Missing error handling
4. Performance issues (N+1 queries)
5. Missing input validation

1. MEDIUM 🟡 - Fix soon (can merge with plan)
2. Code quality issues
3. Missing tests
4. Poor naming
5. Missing documentation

1. LOW 🟢 - Nice to have
2. Style suggestions
3. Optimization opportunities
4. Refactoring ideas

Review Checklist

1. Correctness

→ See: [correctness-criteria.md](./correctness-criteria.md)

• [ ] Logic is correct for all test cases
• [ ] Edge cases handled (null, empty, max, min)
• [ ] Error conditions properly handled
• [ ] Return types match function signatures
• [ ] Async operations properly awaited
• [ ] No race conditions
• [ ] No off-by-one errors

2. Security

→ See: [security-sentinel skill](../security-sentinel/SKILL.md) → See: [security-checklist.md](./security-checklist.md)

CRITICAL - Must check every review:

• [ ] No hardcoded secrets
• [ ] Input validation with Zod (ALL inputs)
• [ ] Authentication checked on protected routes
• [ ] Authorization enforced (resource ownership)
• [ ] SQL injection prevented (using Drizzle)
• [ ] XSS prevented (no dangerouslySetInnerHTML without sanitization)
• [ ] CSRF protection on state-changing operations
• [ ] No sensitive data in logs
• [ ] Passwords hashed (bcrypt, 12+ rounds)
• [ ] JWTs properly verified

For complete security criteria: → [security-sentinel/SKILL.md](../security-sentinel/SKILL.md)

3. TypeScript Quality

→ See: [typescript-strict-guard skill](../typescript-strict-guard/SKILL.md)

• [ ] No any types
• [ ] No @ts-ignore without extensive comment
• [ ] No ! non-null assertions without comment
• [ ] Explicit types on all function parameters
• [ ] Explicit return types on all functions
• [ ] Type guards used for unknown types
• [ ] Proper use of generics
• [ ] No implicit any

4. Testing

→ See: [quality-gates/test-patterns.md](../quality-gates/test-patterns.md)

• [ ] Tests exist for new code
• [ ] Tests follow AAA pattern
• [ ] Coverage meets thresholds (75%/90%)
• [ ] UI tests verify DOM state (not just mocks)
• [ ] E2E tests for visual changes
• [ ] No skipped tests without reason
• [ ] Tests are independent
• [ ] Tests clean up after themselves

5. Performance

→ See: [performance-criteria.md](./performance-criteria.md)

• [ ] No N+1 query problems
• [ ] Database queries optimized
• [ ] Async operations parallelized where possible
• [ ] Large datasets paginated
• [ ] Images optimized
• [ ] No unnecessary re-renders
• [ ] Expensive calculations memoized

6. Code Quality

→ See: [maintainability-rules.md](./maintainability-rules.md)

• [ ] No console.log in production code
• [ ] No commented-out code
• [ ] No TODO without GitHub issue
• [ ] Functions have single responsibility
• [ ] Variable names are descriptive
• [ ] No dead code
• [ ] No duplicated logic
• [ ] Proper error messages

7. Architecture Compliance

→ See: [architecture-patterns skill](../architecture-patterns/SKILL.md)

• [ ] Correct pattern chosen for problem
• [ ] Pattern implemented correctly
• [ ] No pattern violations
• [ ] Follows Next.js best practices
• [ ] Server vs Client Components correct
• [ ] State management appropriate

Review Process

Step 1: Pre-Review (2 minutes)

1. Read PR description
2. Understand what changed and why
3. Check CI/CD status (tests, build, coverage)
4. Identify high-risk areas (auth, payments, data handling)

Step 2: Security Review (5 minutes)

For ALL PRs:

• Check for hardcoded secrets
• Verify input validation
• Check authentication/authorization

For Auth/API/Data PRs:

• Run security-sentinel skill
• Review OWASP Top 10 criteria
• Check for injection risks

→ [security-checklist.md](./security-checklist.md)

Step 3: Code Review (10-20 minutes)

1. Correctness: Does it work as intended?
2. TypeScript: Strict mode compliance?
3. Testing: Adequate coverage and quality?
4. Performance: Any obvious issues?
5. Quality: Readable, maintainable code?
6. Architecture: Follows established patterns?

Step 4: Write Feedback (5 minutes)

Use severity levels and templates: → [review-templates.md](./review-templates.md)

Format:

## 🔴 CRITICAL Issues

- [ ] [Security] Hardcoded API key in auth.ts:45
  - **Risk**: API key exposed in version control
  - **Fix**: Move to environment variable
  - **File**: src/lib/auth.ts:45

## 🟠 HIGH Issues

- [ ] [TypeScript] Using `any` type in processData()
  - **Issue**: No type safety
  - **Fix**: Define explicit interface
  - **File**: src/utils/process.ts:12

## 🟡 MEDIUM Issues

- [ ] [Testing] Missing tests for error cases
  - **Coverage**: Only happy path tested
  - **Needed**: Test null input, invalid format
  - **File**: tests/unit/process.test.ts

## 🟢 LOW Issues / Suggestions

- Consider extracting helper function for readability

Step 5: Verdict

Choose one:

• ✅ APPROVE - No critical/high issues
• 🔄 REQUEST CHANGES - Critical or multiple high issues
• 💬 COMMENT - Questions or low/medium issues only

Review Templates

Security Issue Template

🔴 **[Security] [Vulnerability Type]**

**Location**: `src/path/file.ts:123`

**Issue**: [Description of vulnerability]

**Risk**: [What could go wrong]

**Fix**:

// Suggested fix

**Reference**: [OWASP link or skill reference]

TypeScript Issue Template

🟠 **[TypeScript] [Issue Type]**

**Location**: `src/path/file.ts:45`

**Issue**: [What's wrong]

**Fix**:

// Current (bad) function process(data: any) { }

// Suggested (good) function process(data: ProcessData): ProcessedResult { }

**Reference**: typescript-strict-guard skill

Performance Issue Template

🟡 **[Performance] [Issue Type]**

**Location**: `src/path/file.ts:78`

**Issue**: N+1 query problem in getUserProjects()

**Impact**: Linear time complexity, slow for large datasets

**Fix**:

// Use join instead of separate queries const projects = await db .select() .from(projectsTable) .leftJoin(usersTable, eq(projectsTable.userId, usersTable.id))

Common Review Patterns

Code Smells

Long Functions

// 🔴 BAD: 100+ line function
function processEverything() {
  // ... 100 lines
}

// ✅ GOOD: Extracted helpers
function processEverything() {
  const validated = validateInput()
  const processed = processData(validated)
  const saved = saveToDatabase(processed)
  return saved
}

Deeply Nested Logic

// 🔴 BAD: 4+ levels of nesting
if (user) {
  if (user.projects) {
    if (user.projects.length > 0) {
      if (user.projects[0].status === 'active') {
        // ...
      }
    }
  }
}

// ✅ GOOD: Early returns
if (!user) return
if (!user.projects || user.projects.length === 0) return
if (user.projects[0].status !== 'active') return
// ...

Magic Numbers

// 🔴 BAD: Unexplained numbers
setTimeout(callback, 3600000)

// ✅ GOOD: Named constants
const ONE_HOUR_MS = 60 * 60 * 1000
setTimeout(callback, ONE_HOUR_MS)

Progressive Disclosure

1. SKILL.md (this file) - Review framework overview
2. security-checklist.md - OWASP Top 10 checklist
3. performance-criteria.md - Performance review criteria
4. maintainability-rules.md -