code-review-and-quality

Code Review and Quality

Overview

Five-axis review: correctness, contract compliance, readability, security, performance. Each finding is categorized as Critical / Important / Suggestion. Contract compliance is the forge-specific axis — implementations must match .forge/contracts/ exactly.

When to Use

• A task from .forge/tasks.yaml is marked done and needs review
• A PR is ready for merge review
• User asks to review a specific change or file

When NOT to Use

• Code hasn't been tested yet — run tdd first
• Looking for performance profiling — that's a separate investigation
• Security audit with threat modeling — use security-auditor agent persona

Common Rationalizations

Thought	Reality
"It passes tests so it's correct"	Tests don't prove correctness — they prove what was tested
"I'll leave style comments but not block"	Style inconsistencies compound into readability debt
"The contract is close enough"	Contracts exist precisely to prevent "close enough"
"Security issues are edge cases"	Edge cases are where attackers operate
"We can optimize later"	Performance regressions rarely get addressed later

Red Flags

• Implementation ignores an error type defined in the contract
• Function signature differs from contract's typed schema
• Invariant in contract is not enforced in implementation
• Input validation missing at module boundary (contract says it's not caller's job)
• Magic numbers with no explanation
• Error paths return 200 with error message in body

Five-Axis Review

Axis 1: Contract Compliance (forge-specific)

For each module touched, check against .forge/contracts/<module>.md:

• Input types match contract schema exactly
• Output types match contract schema exactly
• Every error type in the contract is handled
• Every invariant in the contract is enforced
• "Not responsible for" items are not implemented here

Axis 2: Correctness

• All acceptance criteria from the task are satisfied
• Edge cases and error paths handled
• No off-by-one, null deref, or race condition visible
• Side effects are intentional and documented

Axis 3: Readability

• Names describe what, not how
• Complex logic has a comment explaining WHY (not WHAT)
• No dead code or commented-out blocks
• Function length: if it doesn't fit in one screen, it should be decomposed

Axis 4: Security

• All user input validated before use
• No SQL/command/HTML injection vectors
• Secrets not logged, not in error messages
• Authentication checked before authorization checked before business logic

Axis 5: Performance

• No N+1 queries introduced
• No synchronous blocking in async paths
• No unbounded operations on user-controlled input size

Finding Format

[CRITICAL] Contract violation: UserService.create() returns `userId: string` per contract
           but implementation returns `id: number`. Callers will break at runtime.
           File: src/user/service.ts:47

[IMPORTANT] Missing error handling: `RateLimitError` is defined in AuthService contract
            but never caught — will surface as unhandled exception.
            File: src/auth/middleware.ts:23

[SUGGESTION] Variable name `d` on line 12 — consider `durationMs` for clarity.

Verification

• All five axes reviewed
• Contract compliance checked against .forge/contracts/ for each touched module
• Every Critical finding listed (must be fixed before merge)
• Every Important finding listed (should be fixed or explicitly deferred)
• Findings cite file:line for each issue
• Suggestions separated from required changes