---

name: code-review description: Comprehensive code review for commits and pull requests. Covers security, TDD, code quality, and documentation standards.

Code Review Skill

Thorough review with emphasis on Security and TDD.

Review Flow

CopyCode Changes -> Security Scan (P1) -> TDD Validation (P2) -> Code Quality -> Verdict

Security Checklist (Critical)

OWASP Top 10

Security Code Patterns

Copy// CRITICAL: SQL Injection
// BAD:
const query = `SELECT * FROM users WHERE id = ${userId}`;

// GOOD:
const query = 'SELECT * FROM users WHERE id = ?';
db.query(query, [userId]);

// CRITICAL: XSS
// BAD:
element.innerHTML = userInput;

// GOOD:
element.textContent = userInput;

// CRITICAL: Command injection
// BAD:
exec(`ls ${userPath}`);

// GOOD:
fs.readdir(userPath);

// CRITICAL: Hardcoded secrets
// BAD:
const apiKey = 'sk-1234567890';

// GOOD:
const apiKey = process.env.API_KEY;

Security Review Questions

• ALL user input validated/sanitized?
• Queries parameterized?
• Secrets in environment variables?
• Auth checked on protected routes?
• Error messages safe (no stack traces)?

TDD Checklist (Priority)

Test Quality Patterns

Copy// BAD: Testing implementation details
test('calls internal method', () => {
  const spy = jest.spyOn(service, '_privateMethod');
  service.doThing();
  expect(spy).toHaveBeenCalled();  // Testing HOW, not WHAT
});

// GOOD: Testing behavior/outcome
test('returns processed result', () => {
  const result = service.doThing();
  expect(result).toEqual(expectedOutput);  // Testing WHAT
});

// BAD: Unrealistic mock data
const mockUser = { id: 1, name: 'test' };

// GOOD: Realistic test data
const mockUser = {
  id: 'usr_abc123',
  name: 'Jane Smith',
  email: 'jane@example.com',
  createdAt: new Date('2024-01-15'),
  roles: ['user', 'admin']
};

// BAD: Only happy path
test('creates user', () => {
  const user = createUser(validData);
  expect(user).toBeDefined();
});

// GOOD: Edge cases covered
test('creates user with valid data', () => { /* ... */ });
test('throws on missing email', () => { /* ... */ });
test('throws on duplicate username', () => { /* ... */ });
test('handles unicode names', () => { /* ... */ });

Code Quality Checklists

No Shortcuts

No Hardcoded Values

Root Cause Fixes

Documentation (Mandatory)

Copy// BAD: No documentation
export function createUser(data) {
  return db.create(data);
}

// GOOD: Full documentation
/**
 * Creates a new user account with validation.
 *
 * @param data - User creation input
 * @returns Created user object
 * @throws {ValidationError} If email invalid
 *
 * @related
 *   - ./UserRepository.ts - Database persistence
 *   - ../validators/email.ts - Email validation
 */
export async function createUser(data: CreateUserInput): Promise<User>

Red Flags

Output Format

Copy# Code Review: [Branch/PR]

## Summary
| Metric | Value |
|--------|-------|
| Files reviewed | X |
| Security issues | X (Y critical) |
| TDD compliance | Y/N |
| Verdict | Ready/Review/Rework |

## Security Issues
### Critical
1. **[Type]** - `file:line`
   - Problem: [desc]
   - Impact: [potential damage]
   - Fix: [solution]

## TDD Issues
1. **[Issue]** - `file:line`
   - Fix: [tests to add]

## Code Quality Issues
[issues]

## Verdict
[decision + reasoning]

### Required Before Merge
- [ ] [action item]

Review Workflow

Step 1: Security Scan First

Copy# Check for secrets
grep -r "password\|secret\|api_key\|token" --include="*.ts"

# Check for SQL injection
grep -r "SELECT.*\${" --include="*.ts"

# Check for XSS
grep -r "innerHTML\|dangerouslySetInnerHTML" --include="*.tsx"

Step 2: TDD Validation

Copy# Check test coverage
npm test -- --coverage

# Verify tests exist for changed files

Verdict Rules

Example: Transparency Block

Copy<decision-transparency>
**Decision:** Approve with minor suggestions (Ready)

**Reasoning:**
- **Security**: No vulnerabilities found
- **Testing**: All new code has tests
- **Quality**: Minor naming suggestions only

**Issues Found:**
1. Minor: Variable name `d` could be `data` (line 42)
2. Minor: Consider extracting magic number 5 to constant

**Confidence:** High - Standard approval scenario
</decision-transparency>

Example: Debate Invitation

Copy<debate-invitation>
**Topic:** Handling of deprecated API usage

**Option A: Block Until Fixed**
- Pros: No technical debt
- Cons: Delays release

**Option B: Approve with Follow-up Task**
- Pros: Pragmatic, allows progress
- Cons: Debt may linger

**My Lean:** Option B - Create ticket, set deadline

**Your Input Needed:** Is there a release deadline? How critical is this code path?
</debate-invitation>