code-review

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

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

Security Checklist (Critical)

OWASP Top 10

Vulnerability	Red Flag
Injection	String concatenation in queries
Broken Auth	Plain text passwords, weak tokens
Sensitive Data Exposure	Secrets in logs
XSS	innerHTML, dangerouslySetInnerHTML
Broken Access Control	Missing permission checks

Security Code Patterns

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

Question	Expected	Red Flag
Tests written first?	Yes	Implementation without tests
Tests define behavior?	WHAT not HOW	Internal implementation tested
Coverage adequate?	Critical paths covered	Only happy path
Tests independent?	Run in isolation	Order-dependent

Test Quality Patterns

// 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: '[e-mail]',
  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

Pattern	Problem
catch (e) { }	Silent exception
TODO, FIXME, HACK	Incomplete work
Commented-out code	Dead code
any overuse	Type safety bypassed

No Hardcoded Values

Bad	Good
if (status === 3)	if (status === Status.APPROVED)
setTimeout(fn, 5000)	setTimeout(fn, TIMEOUT_MS)
'http://localhost:3000'	process.env.API_URL

Root Cause Fixes

Symptom Fix (Bad)	Root Cause Fix (Good)
Add null check where crash occurs	Validate data at entry point
Retry failed request 3 times	Fix why request fails
Catch and ignore error	Handle error appropriately
Add delay to avoid race condition	Fix the race condition

Documentation (Mandatory)

Level	Required
File	@file, @description, @related
Class	Purpose, responsibilities
Function	@param, @returns, @throws

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

Flag	Severity	Action
SQL/Command injection	CRITICAL	Block
Hardcoded secrets	CRITICAL	Block
XSS vulnerability	CRITICAL	Block
No tests for new code	MAJOR	Request tests
Tests modified to pass	MAJOR	Investigate
Silent exception catch	MAJOR	Require logging
Missing file header	MAJOR	Add docs

Output Format

# 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

# 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

# Check test coverage
npm test -- --coverage

# Verify tests exist for changed files

Verdict Rules

Condition	Verdict
Any critical security	Needs Rework
No tests for new functionality	Needs Review
Minor issues only	Ready (with suggestions)
No issues	Ready to merge

Example: Transparency Block

<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

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