Review Diff

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:  ( files, +/-)

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

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: urmzd
• Source: urmzd/dotfiles
• License: Apache-2.0

Install and usage instructions live in the source repository linked above.