Code Review

Code Review

Systematic code review across six dimensions with confidence-scored findings.

Core principle: A finding without a severity and a concrete fix is noise. Every issue needs: what, why it matters, and how to fix it.

When to Use

• Reviewing a PR before merge
• Auditing existing code for quality problems
• Giving structured feedback on someone's implementation
• Self-reviewing before submitting

When NOT to Use

• Security-only audit (use security-review skill)
• Performance-only audit (use frontend-performance or backend-performance skill)
• Refactoring existing code (use refactoring skill)

Rule #1: Review tests first. Tests reveal intent and coverage before you read the implementation. Check: Do tests exist? Do they test the right things? Are edge cases covered?

The 6-Axis Framework

Rate each dimension and note specific findings:

Risk Classification

HIGH — Block merge, fix before shipping

• Incorrect behavior on valid input
• Security vulnerability (injection, auth bypass, data exposure)
• Data loss or corruption possibility
• Crash on expected inputs
• Breaking change without versioning

MEDIUM — Fix before or shortly after shipping

• Unhandled edge cases (error, empty, concurrent)
• Misleading names that will cause future bugs
• Missing tests for critical paths
• Performance problem that will matter at scale
• Inconsistent with established patterns

LOW — Fix when convenient

• Naming improvements
• Redundant code
• Minor style inconsistencies
• Optimization in non-critical paths
• Comment improvements

Adversarial Analysis (3 Personas)

After the 6-axis review, think from these perspectives:

Persona 1: The Attacker

• What happens if every input is malicious?
• Can I bypass authentication or authorization?
• Can I inject code via any input field?
• Can I cause a denial of service?
• Can I read data I shouldn't?

Persona 2: The User

• What happens when the network is slow or fails?
• What error does the user see when this fails?
• What if the user does something unexpected (refresh mid-flow, double-click)?
• What if the user's data is empty, very large, or unusual?

Persona 3: The Future Maintainer

• If I come back to this in 6 months, will I understand it?
• If I need to add a feature, where do I make the change?
• If this breaks at 3am, will the logs tell me what happened?
• If requirements change, how many places need updating?

Confidence Scoring

Score each finding before reporting it:

Rule: Do not report a finding you haven't traced. Pattern matching without code tracing is noise.

Multi-Model Review Pattern

For critical code, run independent reviews from multiple perspectives:

Pass 1: Correctness — does it do what it says?
Pass 2: Security — can it be exploited?
Pass 3: Architecture — does it fit the system?

Separate passes catch different things. A reviewer focused on architecture misses security; one focused on security misses architecture.

Finding Format

Acknowledge what's done well — always include at least one specific positive observation. Specific praise motivates good practices and builds trust.

Each finding:

[SEVERITY] [Axis] Short title

Location: file.ts:42

Problem: [What is wrong and why it matters]

Evidence: [Specific code that demonstrates the issue]

Fix: [Concrete recommendation]

What's Done Well:

• [Specific positive observation]

Review Checklist

Correctness:

• Logic handles empty, null, and boundary values
• Concurrent access is safe (no race conditions)
• Error paths return/throw appropriate values
• Function does what its name says

Security:

• All user input validated server-side
• No SQL/command/template injection vectors
• Auth and authz checks present on all sensitive operations
• No secrets in code or logs

Performance:

• No N+1 query patterns
• No unbounded data fetches
• No synchronous blocking in async context
• Memory not growing unboundedly

Readability:

• Names reveal intent (no single-letter variables except loop indices)
• Functions do one thing and are <50 lines
• No magic numbers (use named constants)
• Complex logic has explanatory comments

Maintainability:

• Tests cover happy path and important edge cases
• Loading, error, and empty states all handled
• No copy-paste duplication
• No dead code

Architecture:

• Follows established patterns in the codebase
• Dependencies point the right direction
• Coupling is minimal (changes here don't cascade everywhere)
• Abstraction level is appropriate (not over- or under-engineered)