Review Diff
Review the working-tree changes against a fixed rubric and emit prioritized, actionable findings. This is the day-to-day review skill that [[assess-quality]] (the quality framework) and [[review-design]] (the design “why” layer) both explicitly hand off to. They set the bar; this skill checks one concrete diff against it.
Why a rubric is mandatory
A review with no rubric drifts into a vibe check and produces a near-empty, low-confidence result (in one observed run, a rubric-less review emitted almost nothing at 0.42 confidence). The five dimensions below are loaded before reading the diff so every change is checked against the same axes. If you cannot load the rubric, say so and stop — do not emit a confident-looking empty review.
Gather the diff
Review everything in the working tree, not just what is staged:
git status --short # the full picture: staged, unstaged, untracked
git diff --staged # staged changes
git diff # unstaged changes to tracked files
git ls-files --others --exclude-standard # untracked files (read each in full)If a scope base ref is given (e.g. main), use git diff main...HEAD plus the
working-tree diffs. Read untracked files in full with Read — they have no “old”
side, so the entire file is the change.
The rubric
Check every change against all five dimensions, in priority order:
| Dimension | Ask | Example findings |
|---|---|---|
| Correctness | Does it do what it claims, including edge cases? | off-by-one, null/None deref, unhandled error path, wrong operator, race, resource leak |
| Security | Could this be abused or leak? | injected input in a query/shell, hardcoded secret, missing authz check, unsafe deserialization, path traversal |
| Tests | Is the new behavior covered? | new branch with no test, asserted-nothing test, deleted test, fixture not updated |
| Scope-creep | Does the diff match its stated intent? | unrelated refactor mixed in, drive-by rename, formatting churn that hides the real change, commented-out code |
| Conventions | Does it match the codebase’s existing style? | naming, error-handling pattern, logging style, public-API shape, missing doc on an exported symbol |
Security and correctness findings outrank style. A real bug at low priority
buried under nits is a failed review.
Severity
Assign one of three levels and order findings by it:
| Severity | Meaning | Examples |
|---|---|---|
| blocker | Must fix before merge | correctness bug, security hole, deleted/disabled test, secret in the diff |
| should-fix | Fix unless there is a stated reason not to | missing test for new behavior, scope creep, error swallowed |
| nit | Optional polish | naming, wording, formatting the linter would catch anyway |
Output
Emit a single prioritized list, blockers first. Every finding carries
file:line, a severity tag, what is wrong, and a concrete suggested fix:
## Review: <branch> (<N> files, +<adds>/-<dels>)
### Blockers
- [blocker] src/auth.py:42 -- user-supplied `next` is passed to `redirect()`
without an allowlist (open redirect). Fix: validate against a known set of
paths, or strip to a relative path.
### Should-fix
- [should-fix] src/parser.rs:88 -- new `parse_header` branch has no test.
Fix: add a case covering a header with no colon.
- [should-fix] src/app.ts:1-200 -- unrelated reformatting of the whole file
obscures the 3-line behavior change (scope creep). Fix: revert the formatting
or split it into its own commit.
### Nits
- [nit] src/util.go:12 -- exported `DoThing` lacks a doc comment.
### Summary
2 blockers, 1 should-fix, 1 nit. Recommend: do not merge until blockers resolved.If the diff is clean, say so plainly and list the dimensions checked — a clean review is a real result, but only after the rubric was actually applied.
Rules
- Read changed files in full when context matters. A diff hunk hides the function it lives in; a correctness call often needs the surrounding code.
- Report only; do not edit, stage, or commit. This skill recommends. For a failing pipeline, route to [[diagnose-ci]]; to set or arbitrate the project’s quality bar rather than review one diff, route to [[assess-quality]].
- Cite a real
file:line. A finding without a location is not actionable. - No empty confident reviews. If nothing loaded or the diff is empty, say that explicitly instead of emitting a hollow pass.