code-review

name	code-review
description	Deep code review methodology for thorough quality assessment

Code Review

Identify defects, security risks, and maintainability problems in code changes before they merge.

Quick Reference

Aspect	Detail
Input	PR diff, file contents, PR title/description
Output	Structured findings (see Finding Format below)
Scope	Changed lines + immediate context; follow references 1 level deep when a change touches a public API
Skip	Formatting, import order, whitespace — defer to linters
Depth	Read every changed line; skim unchanged context for broken assumptions

Finding Format

Each finding MUST follow this structure:

**[severity]** `file:line` — Description of the issue.
Suggestion: concrete fix or direction.

Severity levels:

Level	Meaning	Gate
error	Bugs, security holes, data loss, crashes	MUST fix before merge
warning	Wrong abstraction, missing validation, performance trap	SHOULD fix
info	Naming, style, minor simplification	Nice to have

Review Procedure

Execute these phases in order. Each phase produces findings or nothing.

Phase 1 — Understand Intent

Read the PR title, description, and linked issues. Determine what the change is supposed to do. If intent is unclear, report as warning before proceeding.

Phase 2 — Correctness

For each changed function/block:

• Trace inputs through the logic. Identify domain boundaries (null, empty, zero, negative, max-length).
• For each boundary, verify the code handles or explicitly rejects it. Unhandled → error.
• Check return values: can a caller confuse a failure return with a success? Silent failures → error.

Phase 3 — Security

At every system boundary (user input, HTTP params, DB queries, shell commands, file paths):

• Verify input is validated or sanitized before use. Missing → error.
• Check for secrets in code, logs, or error messages. Present → error.
• Verify auth checks exist for protected operations. Missing → error.

Phase 4 — Performance

• Identify loops over collections: is work inside the loop that could be batched or hoisted? Report as warning.
• Look for N+1 patterns: a query inside a loop that iterates query results. Report as warning.
• Flag unbounded lists or payloads with no pagination/limit. Report as warning.

Phase 5 — Maintainability

• Flag functions doing more than one job (needs "and" to describe) → warning.
• Flag duplicated logic across the diff (same pattern 2+ times) → info.
• Flag misleading names (function name promises X, body does Y) → warning.

Phase 6 — Tests

• If the change adds behavior, verify a test covers the happy path. Missing → warning.
• If the change fixes a bug, verify a regression test exists. Missing → warning.
• Flag non-deterministic tests (time-dependent, random, order-dependent) → warning.

Examples

Bug: unhandled null at domain boundary

// PR diff
function getUser(id: string) {
  const row = db.query("SELECT * FROM users WHERE id = ?", [id]);
  return { name: row.name, email: row.email };
}

Finding:

**[error]** `src/users.ts:3` — `db.query` returns `null` when no row matches,
but the next line unconditionally accesses `.name` on the result.
Suggestion: Guard with `if (!row) return null` or throw a NotFoundError.

Security: unsanitized input in shell command

# PR diff
def export_report(filename):
    os.system(f"tar czf /tmp/{filename}.tar.gz /data/reports")

Finding:

**[error]** `reports/export.py:3` — `filename` is interpolated into a shell
command without sanitization. An attacker passing `; rm -rf /` exploits this.
Suggestion: Use `subprocess.run(["tar", "czf", ...])` with a list to avoid shell injection,
and validate `filename` against an allowlist pattern.

N+1 query in loop

// PR diff
const orders = await db.orders.findMany({ where: { status: "open" } });
for (const order of orders) {
  const customer = await db.customers.findUnique({ where: { id: order.customerId } });
  order.customerName = customer.name;
}

Finding:

**[warning]** `src/orders.ts:2-5` — Each loop iteration issues a separate
DB query for the customer. With N open orders this is N+1 queries.
Suggestion: Use `include: { customer: true }` in the initial query, or
batch-fetch customers with `findMany({ where: { id: { in: customerIds } } })`.

MUST DO / MUST NOT DO

MUST DO	MUST NOT DO
Report every finding with file, line, severity, and suggestion	Report vague findings without location or fix direction
Prioritize errors first, then warnings, then info	Bury a critical bug under 10 style nits
Read the full diff before writing findings	Review only the first file and stop
Verify claims by reading the referenced code	Assume a pattern is wrong without checking the implementation
Limit info-level findings to 5 max	Flood the review with cosmetic suggestions

Final Checklist

Before submitting your review, verify:

• Every error finding includes a concrete reproduction scenario or input
• Every finding has file:line, severity, description, and suggestion
• Findings are grouped by severity (errors first)
• No duplicate findings (same root cause reported once, not per-occurrence)
• If zero findings: explicitly state "No issues found" — do not return empty output