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).

# 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.