Code Review Skill

Review in this order, stopping only to record findings:

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

Deep examples live in references/. Do not re-derive them here:

• references/security-patterns.md - per-language vulnerable/safe pairs
• references/tdd-patterns.md - TDD process + test quality examples
• references/review-templates.md - copy-paste review templates
• references/iso27001-guidelines.md - compliance checklist
• references/validation-false-positives.md - anti-patterns when reviewing validation/audit code

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

Risk	BAD	GOOD
SQL Injection	const query = `SELECT * FROM users WHERE id = ${userId}`;	const query = 'SELECT * FROM users WHERE id = ?'; db.query(query, [userId]);
XSS	element.innerHTML = userInput;	element.textContent = userInput;
Command injection	exec(`ls ${userPath}`);	fs.readdir(userPath);
Hardcoded secrets	const apiKey = 'sk-1234567890';	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

Rule	BAD	GOOD
Test behavior, not internals	jest.spyOn(service, '_privateMethod') then expect(spy).toHaveBeenCalled() (HOW)	const result = service.doThing(); expect(result).toEqual(expectedOutput); (WHAT)
Realistic test data	const mockUser = { id: 1, name: 'test' };	const mockUser = { id: 'usr_abc123', name: 'Jane Smith', email: 'jane@example.com', createdAt: new Date('2024-01-15'), roles: ['user', 'admin'] };
Cover edge cases, not just happy path	only test('creates user', ...)	plus throws on missing email, throws on duplicate username, 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>