Code Review

Code Review

When to Use

• The user shares code (file, snippet, or diff) and asks for feedback
• They paste a pull request or want to know if code is production-ready
• Pre-merge quality gate or bug hunting
• Reviewing architectural decisions in a PR

Context Required

From startup-context: tech stack, product stage, team size. Also need from the user:

• The code or diff to review
• What the code is supposed to do (PR purpose or feature context)
• Any specific concerns (performance, security, correctness)
• Language and framework if not obvious from the code

Workflow

Follow a structured five-step methodology. Each step must be completed before moving to the next.

1. Context — Understand and summarize the PR's purpose before any analysis. Recap the intent back to the user in 1-2 sentences. If unclear, ask before proceeding. Never start reviewing without understanding what the code is trying to accomplish.
2. Structure — Evaluate architectural decisions and design patterns:
   • Does the code belong in the right module/layer?
   • Are abstractions appropriate (not too many, not too few)?
   • Does this change align with the existing codebase patterns?
   • For non-obvious design choices, acknowledge the author's reasoning before proposing alternatives.
3. Details — Assess code quality across multiple dimensions:
   • Correctness: Logic errors, off-by-one bugs, null/undefined handling, race conditions, edge cases
   • Security: OWASP Top 10 baseline — injection, broken auth, data exposure, XSS, access control, misconfig, insecure deserialization, vulnerable components
   • Performance: N+1 queries, unnecessary re-renders, O(n^2) on large datasets, missing caching, memory leaks
   • Naming and clarity: Do names communicate intent? Are functions focused on a single responsibility?
4. Tests — Validate test coverage with equal rigor as code review:
   • Are behavioral assertions present (not just implementation testing)?
   • Are edge cases and error paths covered?
   • Are tests brittle or resilient to refactoring?
   • What test cases are missing?
5. Feedback — Generate a prioritized, categorized report with specific code examples and concrete improvements. Recognize strong patterns and good decisions explicitly.

Output Format

# Code Review: [Feature/File Name]

## Summary
One-paragraph assessment: what the PR does, whether it is ready to merge, needs minor fixes, or needs rework.

## Findings

### Critical (must fix before merge)
- **[CRT-1] Title** — file:line — description, why it matters, suggested fix with code example

### Major (should fix before merge)
- **[MAJ-1] Title** — file:line — description, why it matters, suggested fix

### Minor (fix when convenient)
- **[MIN-1] Title** — file:line — description, suggestion

### Positive (things done well)
- **[POS-1] Title** — file:line — what was done well and why it matters

## Questions
Clarifying questions about non-obvious design choices before blocking on them.

## Suggested Tests
- Test case 1
- Test case 2

Frameworks & Best Practices

Severity Definitions

Review Principles

• Always ground feedback in specifics. Every finding must reference a file, line, and include a concrete improvement — not just "this could be better."
• Recognize good work explicitly. Call out strong patterns, clean abstractions, and thoughtful error handling. Reviews that only flag problems are demoralizing and incomplete.
• Acknowledge author reasoning. For non-obvious choices, assume the author had a reason. Ask before overriding. Phrase as "I see you chose X — was that because of Y? If so, consider Z as an alternative."
• Do not block on style when automated tooling handles it. Linting and formatting are the job of CI, not reviewers. Focus on logic, architecture, and correctness.
• Treat test review with equal weight. Tests are not an afterthought. Missing tests for critical paths is a major finding, not a minor one.

OWASP Top 10 Quick Checks

1. Injection — Are user inputs parameterized? Check SQL, NoSQL, OS command, LDAP
2. Broken Auth — Sessions secure? Tokens rotated? Passwords hashed (bcrypt/argon2)?
3. Sensitive Data Exposure — Secrets in env vars (not code)? PII encrypted at rest?
4. XXE — XML parsing disabled or configured to reject external entities?
5. Broken Access Control — Every endpoint checks authorization, not just authentication?
6. Misconfiguration — Debug modes off? CORS restrictive? Security headers set?
7. XSS — Outputs encoded? No dangerouslySetInnerHTML / v-html with user data?
8. Insecure Deserialization — Serialized objects from untrusted sources validated?
9. Vulnerable Components — Dependencies up to date? Any known CVEs?
10. Insufficient Logging — Auth failures, access violations, and errors logged?

N+1 Query Detection

• Looping over a collection and making a DB call per item
• ORM lazy loading inside a loop (e.g., user.posts in a for user in users loop)
• GraphQL resolvers fetching related data per parent without DataLoader
• Fix: Eager loading, batch queries, or DataLoader pattern

Language-Agnostic Red Flags

• Functions longer than 40 lines or more than 3 levels of nesting
• Boolean params that change behavior (use separate functions)
• Catch blocks that swallow exceptions silently
• Hardcoded values that should be configuration
• Missing input validation on public API boundaries

Related Skills

• security-review — chain for a deep-dive security audit when critical security findings surface
• architecture-design — chain when code review reveals structural problems that need architectural rethinking

Examples

Example prompt: "Review this Express route handler for creating users."

Good output snippet:

# Code Review: POST /api/users Handler

## Summary
The handler creates users but has a critical SQL injection vulnerability and missing
input validation. Needs fixes before merge. Clean async/await usage is a positive.

## Findings
### Critical
- **[CRT-1] SQL Injection** — routes/users.js:14 — Email interpolated
  directly into SQL string. Use parameterized queries:
  `db.query('INSERT INTO users (email) VALUES ($1)', [email])`

### Major
- **[MAJ-1] No input validation** — routes/users.js:12 — Email from
  `req.body` used with no validation. Add zod/joi schema validation.
- **[MAJ-2] Missing error handling** — routes/users.js:18 — DB errors
  propagate as 500 with stack trace. Wrap in try/catch, return generic error.

### Positive
- **[POS-1] Clean async/await** — routes/users.js:11 — Easy to follow,
  no callback nesting.

## Questions
- Is there a validation middleware already in the project that should be reused here?