Code Review

Code review — standalone, spec-less diff judgment

You are reviewing a concrete change — a git diff, a branch, a GitHub PR, a pasted patch — on its own merits. No rsc-SDD spec/plan/constitution chain is required and you should not pretend one exists. This is the doctrine behind the executable /code-review slash command: same evidence bar, written as a discipline you run by hand. If the user is mid-SDD and wants to process incoming comments against 02-DOCS/wiki/sdd/, that is ../review/SKILL.md; a naked diff or an inbound third-party PR is this skill.

The north star is signal-to-noise. Report only findings you would stake your name on. A clean diff is APPROVE, not a manufactured nit. High-false-positive review gets tuned out by humans in about two weeks; the bar to aim for is the logic-error review where under 1% of findings come back marked wrong. Padding does not make you look thorough — it trains the reader to ignore you.

Get the change and its intent first

Three inputs, in this order: the diff, its stated purpose, and the touched surface (the files around the hunks, not just the hunks).

bashCopy code
# A GitHub PR
gh pr diff 1432
gh pr view 1432 --json title,body,files,additions,deletions

# A local branch against the trunk
git diff main...HEAD
git diff --stat main...HEAD   # see blast radius before reading

# A pasted patch — read it as given

A review with no notion of intent is a review of vibes. If no purpose is stated, infer it from the diff and say what you assumed ("Assuming this is meant to add idempotency to the webhook handler…") so the reader can correct a wrong premise — a silent wrong premise produces a confidently wrong review. Then read the whole changed file, not just the green/red lines: the structural failure of standalone review is judging a hunk without its context and shipping generic pattern-matched suggestions.

The pass order

Run these in order. Passes 1–5 are correctness/safety and are blocking-eligible; pass 6 is cleanup and is usually [should-fix] or [nit]. A clean pass is a reportable result ("contracts: nothing changed shape, no finding"), not a pass you silently skip.

Pass 4 is a boundary pass — trace untrusted input to its sink and flag the reachable ones. For a real STRIDE/OWASP threat model with exploitability ranking and vulnerable→fixed diffs, hand off to ../secure-coding/SKILL.md.

Adjacent jobs, delegated by name: running the lint/type/test gates until they are green is ../verify/SKILL.md (this skill judges whether green is correct); root-causing one confirmed failure is ../debug/SKILL.md; cross-checking spec/plan/tasks before code exists is ../analyze/SKILL.md.

Confidence floor and the false-positive skip-list

The 80% rule: if you are not at least ~80% sure a finding is real, you have two moves — trace the code until you are sure, or downgrade it to [question]. Never ship a guess dressed as a defect.

Skip these common false positives outright (or demote to [question]/[nit]):

• Guarded upstream — the "missing" check happens in the caller you can see; trace before you flag.
• Framework-enforced — the framework already does it (e.g. an ORM that parameterizes, a router that validates).
• Behind an off-everywhere flag — real but unreachable in any deployed config → [nit], not blocking.
• Test-only / generated code held to the prod bar — don't demand prod-grade error handling in a fixture or a generated client.
• Style the linter owns — quotes, import order, line length. If a tool enforces it, don't spend a finding on it.

No severity inflation. Rank by blast radius × reachability, not by how clever the catch was. A typo in a log string is a nit even if it took effort to spot.

Severity and finding format

• [blocking] — wrong/unsafe; merging causes a real defect. Must be fixed.
• [should-fix] — a real problem with bounded blast radius; fix it or consciously accept it.
• [nit] — minor; the reader may ignore it without consequence.
• [question] — you suspect an issue but cannot prove reachability; asking, not asserting.

Every finding carries where / why / repro / fix:

textCopy code
[should-fix] api/orders.py:88 — duplicated total logic
  where:  `subtotal = sum(i.price * i.qty for i in items)` re-implements
          `cart.compute_subtotal()` (cart/totals.py:14), which also applies
          per-item discounts this copy silently drops.
  why:    discounted items now bill at full price on this path only;
          the two implementations will drift on the next discount change.
  repro:  order containing any item with `discount_pct > 0` → charged the
          undiscounted amount; covered by no test.
  fix:    call `cart.compute_subtotal(items)` instead of inlining the sum.

Rule: no repro or stated mechanism → it is a [question], not a blocker. "This could overflow" with no path is a question; "n*1000 with n up to 3M exceeds int32 at orders.py:51" is a finding.

Verify before you flag

Read the surrounding code, trace the value, confirm the path is reachable.

• Bad: "Looks like SQL injection." (pattern-match)
• Good: "search() interpolates req.query.q straight into db.execute(\… WHERE name='${q}'`)at search.ts:22;q` is unvalidated user input → injection." (traced)

If you cannot trace it to a concrete value and a reachable sink, you do not yet have a finding.

When several reviewers looked at the same diff, merge before reporting. Have each one end with a ```json findings block, run npx @ericrisco/rsc review consolidate <reports…>, and send each blocker it marks verify to the finding-verifier agent. That is the review skill's step, and it applies here unchanged.

The verdict

End every review with exactly one, plainly — no mushy middle:

• APPROVE — no blockers, no should-fix. Point the user to ../ship/SKILL.md to merge.
• APPROVE WITH NITS — mergeable; nits listed but none gate the merge.
• CHANGES REQUESTED — at least one [blocking]. List precisely what unblocks it, so the author knows when they are done.

Effort dial

Mirror the slash command's effort level: low/medium → fewer, high-confidence findings (raise the confidence floor, focus on passes 1–4). high/max → broader coverage; uncertain findings are allowed but must be labelled [question], never inflated into blockers. This is coverage vs precision, not the user's register — it changes what you look at, not how you word it.

Emitting comments and applying fixes

Read-only by default. You produce findings + a verdict and stop there. Two opt-in modes:

• --comment → post the findings as an inline-anchored review on the PR.
• --fix → apply the agreed findings to the working tree.

bashCopy code
# Summary review (the verdict)
gh pr review 1432 --request-changes -b "CHANGES REQUESTED — see inline. Blocker: orders.py:88 …"
gh pr review 1432 --approve -b "APPROVE — correctness and contracts clean."

Inline line-anchored comments go through the GitHub REST API — see references/pr-workflow.md for the JSON shape, fork-PR handling, and large-diff strategy. If --fix puts you on the default branch, branch first; commit or push only when the user asks; git authorship is Eric (no Claude co-author or generated footer).

Anti-patterns