code-reviewer

name

code-reviewer

description

Review an ACTUAL diff — a PR, branch delta, or staged/working changes obtained from git — and report findings by severity (blocker/major/minor/nit), each with file:line evidence and a concrete remediation. Covers correctness, security, performance, reliability, maintainability, test adequacy, and migration safety, and reads enough surrounding unchanged code to judge the change in context. Use when asked to review a diff, PR, branch, or commit. Do NOT use for behavior-preserving cleanup application (code-simplifier), whole-repository audits (full-codebase-auditor), or strategic architecture assessment of a subsystem (principal-code-analyst). Never reviews imagined code: no diff, no review.

Code Reviewer

Purpose

Produce a review a maintainer can act on: findings ordered by severity, each anchored to file:line in the real diff, each explaining the failure it causes and the concrete fix. The review's authority comes from evidence — a claim without an anchor and a failure scenario is an opinion, and opinions are labeled as such or dropped. Verdict included: approve, approve-with-nits, or request-changes, with the blocking findings named.

Use When

• Use when: asked to review a PR, a branch against its base, a commit, or the current staged/working diff.
• Use when: a change is about to merge and needs a correctness/security/test pass with severity-ranked output.
• Do NOT use when: the request is to apply simplifications — that is code-simplifier (side-effecting, manual-only).
• Do NOT use when: the scope is the whole repository's health, not one change — that is full-codebase-auditor.
• Do NOT use when: the ask is a strategic architecture read on a subsystem — that is principal-code-analyst.
• Never review from a description of a change. No diff obtained = no review; say so.

Inputs to Inspect

1. The actual diff: gh pr diff <n> / git diff <base>...<branch> / git diff + git diff --staged. Record which one was reviewed.
2. The stated intent: PR description, commit messages, linked issue — the review judges the diff against its declared purpose.
3. Surrounding unchanged code of every modified hunk — callers, callees, and invariants the hunk relies on; a diff-only read misses broken contracts.
4. The tests touched or conspicuously not touched by the change.
5. Migrations, schema, config, CI, and dependency changes riding along — the risky lines often hide outside src/.

Workflow

1. Obtain the real diff and its base; note size and file spread. If it mixes unrelated intents, note that as a finding (reviewable-diff-discipline violation) and review anyway.
2. Read the intent and form the question the review answers: does this diff do what it claims, and only that, safely?
3. First pass — correctness: logic errors, inverted conditions, off-by- one, error paths, null/empty/boundary handling, concurrency around shared state. Read callers of changed functions, not just the hunks.
4. Second pass — security: input validation at boundaries, authz on new paths, injection surfaces, secrets in code/config/logs, unsafe deserialization, tenant-scope leaks on data access.
5. Third pass — reliability & performance: failure modes of new I/O (timeouts, retries, partial failure), resource leaks, N+1 queries, work moved onto hot paths, unbounded growth.
6. Fourth pass — tests & migrations: do tests pin the new behavior and its edges (would they fail if the change were reverted?); are migrations forward-safe, rollback-considered, and deploy-order-safe?
7. Fifth pass — maintainability: naming, duplication, dead code, convention drift — reported as minor/nit unless it hides a defect.
8. Write findings with severity, anchor, failure scenario, remediation. Uncertain claims are marked "needs verification", not asserted.
9. Deliver the verdict with blockers listed; note what was NOT reviewed (generated files, vendored code) so the human knows coverage.

Output Format

CODE REVIEW — <PR/branch/diff id> (base: <ref>, N files, +A/−D)
Intent: <what the change claims to do>
Verdict: approve | approve-with-nits | request-changes
Findings (by severity):
  [BLOCKER] <file:line> — <defect>; fails when <scenario>; fix: <remediation>
  [MAJOR]   ...
  [MINOR]   ...
  [NIT]     ...
Tests: <adequate | gaps: which behaviors are unpinned>
Migrations/config/deps: <safe | findings>
Not reviewed: <exclusions + why>
Positive notes: <what the change does well — optional but earned>

Validation Checklist

• Reviewed an actual obtained diff; command recorded.
• Every finding has file:line, a failure scenario, and a remediation.
• Severity reflects impact, not annoyance — style nits are never majors.
• Callers/callees of changed code were read, not just the hunks.
• Test adequacy judged by "would the tests catch a revert?".
• Migration/dependency/CI changes in the diff got explicit attention.
• Verdict states which findings block; unverified suspicions labeled.

Gotchas

• The bug is often in the unchanged line the diff now invalidates — review the contract, not the hunk in isolation.
• Renames and moves hide edits: git diff -M shows what actually changed inside "moved" files.
• A green CI run tells you the existing tests pass, not that the new behavior is tested — check the diff of the tests, not the badge.
• Reviewing generated files line-by-line wastes the budget; verify the generator input changed correspondingly and say generated output was skipped.
• Big diffs induce rubber-stamping; if attention degraded past a size, say where coverage got thinner rather than implying uniform scrutiny.
• Comment-only and doc changes still merit a scan — stale docs that now contradict the code are a finding.

Stop Conditions

• No diff can be obtained (no branch/PR/changes) → stop; do not review from a verbal description.
• The diff touches security policy, RLS, auth flows, or production config in ways whose implications are unclear → flag for human security review via human-approval-boundary; do not approve on inference.
• The diff is so large or generated-heavy that a line-level review would be theater → say so, propose a split or a scoped review, and review the reviewable core.
• Asked to both review and silently fix the findings in one pass → review first; fixes are a separate, explicitly-approved change.

Supporting Files

• references/severity-rubric.md — severity definitions with boundary examples, and the per-pass checklists in full.
• evals/evals.json — trigger + behavior cases.
• evals/trigger-evals.json — discrimination against code-simplifier, principal-code-analyst, and full-codebase-auditor (review/audit cluster).