review-code

name	review-code
description	Use when reviewing changed code before committing or after completing a feature — checks performance, architecture, type safety, and code quality against project standards

Code Review

Review all changed files for performance, architecture violations, type safety, and code quality. Fix every real issue found. Severity-ranked — critical issues first.

Verification rule: Every fix must be verified. Run tests after fixing. Never say "this should fix it" — show the test output proving it does.

Phase 1: Identify Changes & Detect Stack

Run git diff (or git diff HEAD if there are staged changes) to see what changed. If no git changes, review files edited in this conversation.

Auto-detect stack from changed files:

• .go files → Go review rules
• .py files → Python review rules
• .tsx/.ts files → React review rules
• Mixed → apply all relevant rule sets

Phase 2: Launch Four Review Agents in Parallel

Use the Agent tool to launch all four agents concurrently. Pass each agent the full diff plus the detected stack context.

Agent 1: Performance & Efficiency Review (CRITICAL)

This is the highest priority review. Check every change against these rules:

Database (Go/pgx):

• N+1 queries — any query inside a loop is a bug. Must use batch query, JOIN, or pgx.Batch
• Unbounded queries — every SELECT must have LIMIT. No SELECT * FROM table without pagination
• Missing indexes — new WHERE/JOIN/ORDER BY columns on large tables need index migrations
• Sequential queries — independent queries that should use errgroup for parallel execution
• Missing batch inserts — multiple INSERTs in a loop → use pgx.Batch or multi-value INSERT
• Inefficient COUNT — separate COUNT query when it could be COUNT(*) OVER() window function

Database (Python/SQLAlchemy):

• N+1 queries — lazy-loaded relationships accessed in loops. Must use selectinload() or joinedload()
• Missing eager loading — related models accessed but not loaded in the query options
• Sequential async I/O — independent awaits that should use asyncio.gather()
• Unbounded queries — missing .limit() on queries that return lists

Algorithm & Data Structures (all stacks):

• O(n²) when O(n) possible — nested loops on same data, list membership in loop (use map/set)
• Redundant allocations — slice/list creation inside loops without pre-allocation
• Linear search for lookup — using list/slice where map/set gives O(1)
• String concatenation in loop — use strings.Builder (Go) or "".join() (Python)
• Unnecessary copies — passing large structs by value in Go where pointer suffices

React:

• Unbounded list rendering — >50 items without virtualization
• Missing debounce — search/filter inputs firing API on every keystroke
• Fetch in render — API calls outside of hooks/React Query
• Missing pagination — fetching all items from API without limit/offset

General:

• Redundant computation — same value computed multiple times, repeated file reads, duplicate API calls
• Hot-path bloat — expensive work in startup, per-request, or per-render paths
• Memory leaks — unbounded caches, missing cleanup, event listener leaks
• TOCTOU — checking existence before operating instead of operating and handling error

Agent 2: Architecture & Structure Review

Clean architecture boundaries:

• Domain imports external — internal/domain/ (Go) or src/domain/ (Python) must not import infrastructure, interfaces, fiber, pgx, sqlalchemy, fastapi, or any framework
• Application imports infrastructure — internal/application/ or src/application/ must not import infrastructure packages directly
• Handler contains business logic — logic beyond parsing request and calling use case belongs in application layer
• Domain entity exposed in API response — handlers must map to DTOs, never return domain types directly
• Repository returns non-domain types — repos must return domain entities, not ORM models

Go-specific:

• Missing swaggo annotations — every handler must have // FuncName godoc block with @Summary, @Tags, @Param, @Success, @Failure, @Router
• Manual body parsing — must use pkg.ValidateBody[T], pkg.ValidateQuery[T], pkg.ValidateParams[T] instead of manual BodyParser + validation
• Missing example tags — DTO/response structs should have example tags for Swagger UI
• Error handling — naked error returns without wrapping context. Use fmt.Errorf("operation: %w", err)
• Exported function without comment — all exported functions need doc comments

Python-specific:

• Missing type annotations — all functions must have return types, all parameters typed
• ABC instead of Protocol — repository interfaces should use Protocol for structural typing
• Missing Mapped[] types — SQLAlchemy models must use Mapped[type] with mapped_column()
• Untyped dict — passing dict where a Pydantic model or dataclass should be used

React-specific:

• Prop drilling >3 levels — should use Context or state library
• Business logic in component — extract to custom hook
• Missing loading/error/empty states — every data-fetching component needs all three

Agent 3: Code Quality & Reuse Review

Duplication & reuse:

• Duplicate logic — search codebase for existing utilities that could replace new code
• Copy-paste with variation — near-duplicate blocks that should be unified
• Inline logic for existing util — hand-rolled string manipulation, path handling, type guards when helpers exist

Code smells:

• Redundant state — state that duplicates or can be derived from other state
• Parameter sprawl — function growing too many params instead of using options struct/config
• Stringly-typed code — raw strings where constants/enums already exist in codebase
• Magic numbers — unexplained numeric literals (extract to named constants)
• Dead code — unused functions, unreachable branches, commented-out code

Error handling:

• Swallowed errors — _ = someFunc() or empty catch blocks hiding failures
• Panic in library code (Go) — use error returns, never panic
• Bare except (Python) — except Exception or except: without specific error types

Agent 4: Security & Safety Review (NON-NEGOTIABLE)

Security is a non-negotiable. Any security finding is automatically CRITICAL.

• SQL injection — string concatenation in queries instead of parameterized args
• Missing input validation — user input used without validation (should go through ValidateBody/Query/Params or Pydantic)
• Sensitive data in logs — passwords, tokens, API keys logged or included in error messages
• Missing auth check — endpoint without @Security BearerAuth annotation or missing auth middleware
• CORS misconfiguration — overly permissive origins in production config
• Hardcoded secrets — API keys, passwords, connection strings in code instead of env vars
• Path traversal — user-controlled file paths without sanitization
• Mass assignment — binding all request fields to model without allowlist
• Missing rate limiting — public endpoints without rate limiting middleware
• Insecure defaults — debug mode, verbose errors, or stack traces exposed in production config

Phase 3: Aggregate & Fix

Wait for all four agents to complete. Aggregate findings by severity:

Severity Levels

Level	Definition	Action
CRITICAL	N+1 queries, SQL injection, unbounded queries, security holes, architecture violations	Must fix before commit
HIGH	Missing indexes, O(n²) algorithms, missing validation, swallowed errors	Must fix before commit
MEDIUM	Missing swaggo annotations, type safety gaps, code duplication, missing error context	Fix now, acceptable to defer with TODO
LOW	Missing example tags, minor style issues, potential future optimization	Note and skip if time-constrained

Fix-First Classification

Every finding gets classified before action:

Classification	Definition	Action
AUTO-FIX	Mechanical issue with one correct solution (missing LIMIT, N+1 query, missing parameterized query)	Fix directly, no discussion
ASK	Judgment call with trade-offs (architecture refactor, API design choice, scope decision)	Present options to user via AskUserQuestion
SKIP	False positive or irrelevant to this codebase	Note and move on

Default to AUTO-FIX. Only use ASK when there are genuinely multiple valid approaches. Never ASK about non-negotiables (security, N+1, tests) — those are always AUTO-FIX.

Fix Process

1. Fix all CRITICAL and HIGH issues directly in code (AUTO-FIX)
2. Fix MEDIUM issues — skip only if clearly a false positive
3. Note LOW issues in summary — do not spend time on these
4. Do NOT argue with findings or add defensive commentary — fix or skip

Evidence-First Reporting

Every finding must include evidence. No vague claims.

## Code Review Results

### Status: DONE / DONE_WITH_CONCERNS

### Fixed (N issues) — AUTO-FIX
- [CRITICAL] Fixed N+1 query in `UserRepository.List` (user_repo.go:45)
  Evidence: query inside for loop, replaced with batch IN clause
- [HIGH] Added LIMIT to events query in `ActivityService` (activity.go:78)
  Evidence: SELECT without LIMIT on table with 100k+ rows
- [CRITICAL] Fixed SQL injection in `SearchHandler` (search_handler.go:23)
  Evidence: string concatenation `"WHERE name = '" + input + "'"`, replaced with parameterized query

### Asked (N issues) — ASK
- [MEDIUM] `UserService` has 8 methods — split into smaller services?
  Presented options to user: keep as-is vs split by domain

### Skipped (N issues) — SKIP
- [LOW] Missing example tag on InternalDTO — not exposed in Swagger

### Clean
- No architecture violations found
- Security review passed

Chains

• Invoked by: eng-lead (code review gate before committing)
• Complements: All *-feature and *-refactor skills as final quality gate
• REQUIRED: Fix all CRITICAL and HIGH issues before code is committed
• REQUIRED: Update CLAUDE.md if review reveals project-specific gotchas (claude-md)