/diff-review

Reviews only the diff between the current branch and its merge base, never the whole tree. Scope discipline is what keeps this affordable and repeatable — do not read or comment on files the diff does not touch.

This skill exists because this repo's automated reviewer is gone, and the findings it used to catch split into two kinds: lint-shape defects (any linter or type checker gets those) and judgement-shape defects that need an adversarial reasoning pass. This skill is for the second kind only. Do not re-implement cargo clippy, tsc, or the CI test suite in prose.

Step 0 — figure out the diff

git fetch origin master 2>/dev/null || true
BASE=$(git merge-base origin/master HEAD)
git diff --stat "$BASE"...HEAD
git diff "$BASE"...HEAD

If the user names a specific target (a PR number, branch, or origin/<branch>), diff against that branch's merge-base with origin/master instead of HEAD's. State which base commit and which head you used in the report — an unstated base is an unverifiable review.

Read changed files in full where the diff alone doesn't give enough context (e.g. you need to see the function a changed line lives in), but the object of review is the diff's own lines: new logic, changed logic, changed claims. Do not go hunting for pre-existing issues on unchanged lines — that's a separate, out-of-scope task (flag it, don't fold it in).

Step 1 — classify area sensitivity per changed file

Per docs/review-severity-bar.md: strict bar (Tally read/write, master_binding.rs, book_presence.rs, money/reports, DSC/credentials, personal/customer data) vs. loose bar (scripts, dev tooling, fixtures, non-claim docs). State which bar you applied to each finding.

Step 2 — hunt the judgement classes

A lint cannot catch any of these. For each changed hunk, ask:

Overclaimed evidence / docs (was 47% P1 in the historical corpus)

• Does prose claim something was verified, observed, or measured live when the actual evidence is synthetic, simulated, or simply not attached?
• Calibration: a doc paragraph asserting "confirmed against live Tally" over a receipt that is actually a fixture generated by this repo's own simulator.
• If a correction lands in one place, check whether the same overclaim survives in a sibling doc, comment, or ADR the diff didn't touch — that's a separate finding, not proof the correction was complete.

Scope/qualification errors (was 78% P1 — the most severe class after privacy)

• Is a rule, constant, or branch that was only proven true under one configuration (one Tally product/release/license tier, one voucher type, one locale) being written with no scope tag, so it silently applies to every configuration?
• Calibration: a rule proven only under TallyPrime 7.1 Silver, education mode, written into shared code as if it held generally — GST duty-head names, license-tier gates, and SVCURRENTCOMPANY guard behavior have all had exactly this shape in this repo's history.
• Calibration: a compact 2025-2026 fiscal-year range read as an 8-digit numeric identifier and bound to the wrong ledger — a parsing rule that held for the samples on hand, applied without a bounds/format check.

Domain-specific logic / edge cases (37% of all findings; concentrated in master_binding.rs and book_presence.rs)

• For any change to matching, folding, binding, or presence logic: what is the novel counter-example? Two near-duplicate ledger names differing by one character. A truncated identifier. A missing master near-missing an existing one. Walk the actual boundary conditions the diff's logic introduces or changes — don't just confirm the happy path the tests cover.
• Does the change make an illegal state unrepresentable (AGENTS.md P2), or does it add another rule a future author has to remember?

Fail-open error handling — the judgement half

• Not "does an error path exist" (that's lint-shape) but "is this specific fallback semantically correct?" A caught error that silently downgrades a write failure to a no-op is different from one that surfaces a typed error. Ask what a human would wrongly believe if this fallback fired.

Privacy — the judgement half

• Not "is there a hardcoded secret" (lint-shape) but "is this an entirely new category of sensitive data the existing checks don't know to look for?" (a new PII field, a new machine-identifying value, a new customer-content channel).

Step 3 — apply the severity bar, state a disposition

Use docs/review-severity-bar.md exactly — do not invent a different scale. Every finding gets:

• Severity: P1 / P2 / P3, with the area bar (strict/loose) that produced it.
• Disposition (P2/P3 only): fixed / stale / rejected / accepted-follow-up, each with the evidence for that call (a commit SHA, a line reference, a stated reason). accepted-follow-up requires an actual filed issue link — do not default to it because it's the easy option; the old unwritten rule produced 27 tracking issues and two duplicate pairs from one PR by defaulting there.
• What was actually checked: one sentence naming the check performed (read the function, ran the test, compared against the ADR). Never state a conclusion stronger than what that check measured — "the parser rejects this input" is a claim about what you ran; "the parser is correct" is not something a single test run proves. If you didn't run something, say you read it, not that you verified it.

Findings with no P1s should say so plainly rather than implying silence means nothing was found — state the hunk count reviewed and the classes checked.

Step 4 — stopping rule (read this before starting round 2+)

Track two numbers across review rounds on the same PR: findings-per-round and P1 share (P1 findings ÷ total findings, this round).

• If P1 share is flat or rising across rounds, the fixes are generating new surface at least as fast as they close it. Stop. Escalate to a human reviewer instead of continuing to loop. State this plainly in the report: "P1 share has not dropped across N rounds; this needs a human, not another automated pass." Do not keep reviewing in the hope that one more round converges — the historical corpus shows PRs running 32 rounds this way, with 46% of findings landing on lines the previous fix had just added.
• If findings-per-round is flat but P1 share is dropping, that's real convergence (shrinking severity, not just shrinking count) — continue.
• Cap at 3 rounds by default even when converging. A PR that needs a 4th round on this skill needs a human to look at why the diff keeps growing new surface.
• You cannot merge by fixing faster than you can review. If you notice yourself speeding up the review to keep pace with a fast-iterating author, that is the signal to stop and say so, not to go faster.

Step 5 — model choice

Default to Sonnet for the review pass — this is a scoped diff read plus a severity classification, not a research task. Escalate to Opus only when: the diff touches master_binding.rs, book_presence.rs, or Tally read/write/money/privacy code and the change is large (roughly >300 changed lines) or restructures existing matching/binding logic rather than adding a narrow case; or when a P1 finding's correctness is itself ambiguous and needs deeper reasoning to confirm before it blocks a merge. Say in the report which model tier you used and why.

Output format

## Diff reviewed
base: <sha>  head: <sha/branch>
files changed: <n>   hunks reviewed: <n>

## Findings

### P1 — <one-line summary>
area: strict/loose — <file>:<line range>
what I checked: <one sentence>
why it's P1: <cite the class from Step 2>

### P2 — <one-line summary>
area: strict/loose — <file>:<line range>
disposition: fixed | stale | rejected | accepted-follow-up
evidence: <commit/link/reason>
what I checked: <one sentence>

## Stopping-rule status
round: <n>   findings this round: <n>   P1 share: <n>%
trend vs. previous round: <rising/flat/falling> — <continue|escalate to human>

## Model used
<sonnet|opus> — <why>

Explicitly out of scope for this skill

• Whole-tree review, or review of files the diff doesn't touch.
• Lint-shape defects a compiler/linter/CI test already catches — do not spend a finding restating a test failure.
• Editing .github/workflows/ci.yml, scripts/merge*, tally_mirror.rs, snapshot.rs, commands.rs, runtime.rs, bridge-tally-protocol/src/lib.rs, or src/main.tsx — reviewing changes to these is in scope, editing them under this skill is not.