Code Reviewer

Code Reviewer Skill

Review Protocol

Execute all 8 layers in order. Report findings at each layer before moving to next. Do not skip a layer because it seems unlikely to have issues. The ordering is by severity: logic errors are caught first, style last.

A finding in Layer 3 (Security) or Layer 4 (Compliance) = BLOCK. The PR does not merge until Layer 3 and 4 findings are resolved.

Layer 1 — Logic Correctness

• Does the code do what the spec or ticket requires?
• Are there off-by-one errors, wrong comparators, or inverted conditions?
• Are all return paths handled? (Missing else, unhandled null, early return that skips cleanup)
• Are boolean conditions correctly composed? (AND/OR precedence, short-circuit behavior)
• Are async operations correctly awaited? (Missing await, unhandled promise rejection)
• Do loops terminate? Are loop counters correct?

Layer 2 — Edge Cases & Boundary Conditions

• Empty collections, null inputs, undefined, zero values, negative numbers
• String: empty string, whitespace-only, max length, special characters, unicode
• Date/time: timezone handling, DST transitions, leap year, end of month, end of year
• Concurrency: race conditions, double-submit, state mutations across async boundaries
• Large inputs: will this scale to 10,000 employees? 100,000 records?

Layer 3 — Security (BLOCK if any finding)

• SQL/NoSQL injection: any user input flowing into a query without parameterization?
• XSS: any user input rendered as HTML without escaping?
• Command injection: any user input in a shell command or exec call?
• Authentication: is this endpoint protected? JWT verified? Session validated?
• Authorization: is the right role checked? Not just "logged in" but "allowed to do this"?
• Data exposure: does any response include fields the client should not see? (SSN, full bank account, other employees' data)
• Secret management: no hardcoded credentials, API keys, connection strings, or tokens
• New dependencies: are they pinned? Audited? No known CVEs?

Layer 4 — Compliance & Regulatory (BLOCK if any finding)

FLSA:

• Overtime rules correct (40h threshold, rate calculation)?
• Exempt/non-exempt classification applied correctly?
• Recordkeeping fields present for new employee records?

IRS Publication 15:

• Withholding tables current for the tax year?
• Supplemental wage rate (22% federal) applied correctly?
• Fringe benefit treatment correct?

Privacy (CCPA/HIPAA where applicable):

• PII collected only where necessary?
• PII stored encrypted at rest?
• No PII logged in plain text (console.log, debug output)?
• Data retention rules respected?

Audit trail:

• All changes to pay-affecting records logged with who/when/what?
• Audit log entries are immutable (append-only)?

Multi-state:

• New state introduced? All state-specific rules handled?
• Apportionment logic correct for multi-state employees?

Layer 5 — Error Handling

• Every external call (DB, API, file system, third party) has explicit failure handling
• Errors are caught at the right level (not swallowed silently)
• Error logs include enough context to debug in production (request ID, employee ID, pay period)
• User-facing error messages reveal no internal state, stack traces, or system details
• Payroll fail-safe: partial payment states are worse than no payment — ensure atomicity

Layer 6 — Performance & Scalability

• N+1 query patterns: is there a DB call inside a loop? Batch it.
• Missing indexes on newly queried columns? Check EXPLAIN plan.
• Memory leaks: resources opened (connections, streams, file handles) but not closed?
• Payload size: are large collections paginated or streamed?
• Synchronous operations blocking the event loop on CPU-intensive payroll calculations?

Layer 7 — Test Coverage

• Are new behaviors covered by tests?
• Do the tests verify behavior (what the code does) vs implementation (how it does it)?
• Are edge cases from Layer 2 covered by tests?
• Are the tests brittle? (Testing internal state, mocking too much, snapshots of huge objects)
• Can edge cases be tested without standing up the entire application?

Layer 8 — Readability & Maintainability

• Naming: do names describe what the thing IS, not how it's implemented? ✅ calculateBiweeklyGross ❌ processStep2
• Function length: over 40 lines is a signal. Review for extraction opportunity.
• Comments: present only where the "why" is non-obvious (not describing what the code does — that's what code is for)
• Duplication: any logic repeated more than twice? Extract and name it.
• Complexity: cyclomatic complexity above 10 in one function? Split it.

Output Format

## Code Review Report — [Date]

**Layer 1 (Logic):**      [PASS | N findings — list below]
**Layer 2 (Edge Cases):** [PASS | N findings — list below]
**Layer 3 (Security):**   [PASS | N findings — BLOCK if any]
**Layer 4 (Compliance):** [PASS | N findings — BLOCK if any]
**Layer 5 (Errors):**     [PASS | N findings]
**Layer 6 (Performance):**[PASS | N findings]
**Layer 7 (Tests):**      [PASS | N findings]
**Layer 8 (Readability):**[PASS | N findings]

**Verdict:** APPROVE / REQUEST CHANGES / BLOCK

**Critical findings (must fix before merge):**
[Layer 3–4 findings in detail]

**Should-fix findings (strong recommendation):**
[Layer 1–2 findings in detail]

**Consider fixing (low priority):**
[Layer 5–8 findings]