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

   • Security vulnerabilities
   • Data loss risks
   • Authentication bypasses
   • SQL injection risks

2. HIGH 🟠 - Should fix before merge

   • TypeScript strict mode violations
   • Missing error handling
   • Performance issues (N+1 queries)
   • Missing input validation

3. MEDIUM 🟡 - Fix soon (can merge with plan)

   • Code quality issues
   • Missing tests
   • Poor naming
   • Missing documentation

4. LOW 🟢 - Nice to have

   • Style suggestions
   • Optimization opportunities
   • Refactoring ideas

---

Review Checklist

1. Correctness

→ See: 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 → See: 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

---

3. TypeScript Quality

→ See: typescript-strict-guard skill

• 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

• 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

• 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

• 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

• 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

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

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**:
```typescript
// Suggested fix

Reference: [OWASP link or skill reference]

### TypeScript Issue Template

```markdown
🟠 **[TypeScript] [Issue Type]**

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

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

**Fix**:
```typescript
// Current (bad)
function process(data: any) { }

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

Reference: typescript-strict-guard skill

### Performance Issue Template

```markdown
🟡 **[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**:
```typescript
// 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**
```typescript
// 🔴 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 - Code quality rules
5. review-templates.md - Feedback templates

---

Integration with Other Skills

Code review aggregates criteria from:

• security-sentinel - Security vulnerability checks
• typescript-strict-guard - Type safety validation
• quality-gates - Quality checkpoint framework
• architecture-patterns - Pattern compliance
• nextjs-15-specialist - Next.js best practices

---

Example Review

# Code Review: Add User Authentication

## Summary
Adds JWT-based authentication with login/logout endpoints.

## 🔴 CRITICAL Issues

### 1. Hardcoded JWT Secret
**File**: `src/lib/auth.ts:12`
**Issue**: JWT secret is hardcoded as "secret123"
**Risk**: Anyone can forge JWTs
**Fix**:
```typescript
- const secret = "secret123"
+ const secret = process.env.JWT_SECRET
+ if (!secret) throw new Error('JWT_SECRET not set')

🟠 HIGH Issues

2. Missing Input Validation

File: src/app/api/auth/login/route.ts:15 Issue: User input not validated before use Fix: Add Zod schema validation

const loginSchema = z.object({
  email: z.string().email(),
  password: z.string().min(8),
})

const validated = loginSchema.parse(body)

🟡 MEDIUM Issues

3. Missing Tests

File: tests/integration/auth.test.ts Issue: No tests for error cases Needed:

• Test invalid email format
• Test wrong password
• Test expired JWT

Verdict

🔄 REQUEST CHANGES - Fix critical and high issues before merge.

Once fixed, this will be a solid authentication implementation.

---

## See Also

- security-checklist.md - OWASP Top 10 checklist
- performance-criteria.md - Performance review guide
- maintainability-rules.md - Code quality rules
- review-templates.md - Feedback templates
- ../security-sentinel/SKILL.md - Security patterns
- ../quality-gates/SKILL.md - Quality framework