unified-code-review

Unified Code Review

Three core passes (1 Risk → 2 Agent-authored when applicable → 3 Structure), plus 1b (repo overlay), §2b (always — you are the LLM reviewer), and §2c (when wiring is at stake — tiered, not always Full). Do not run structure-only review.

Output is a sensor, not a merge verdict. Human owns merge, especially on HIGH-risk paths. Queue bots may map LOW + PASS to an approve signal outside this skill — that is policy, not a substitute for review depth. Do not invent numeric auto-approve score bands here.

Pipeline contract: ### Advisory and ### Nits are for humans to triage. Agent loops / autofix pipelines must act only on ### Blockers (or an explicit user ask) — do not implement advisories unprompted. Reviewer: emit the sensor and stop; do not start fixing unless asked.

Eval (when reviews are logged): sample whether the risk tier and recommendation matched a human judgment. Same discipline as product evals — logged scores without sampling drift into theater.

Verdict tokens (byte-identical everywhere): PASS | ADVISORY | BLOCKERS. Section headings ### Blockers, ### Advisory, ### Nits are fixed strings — omit any section that would be empty; never emit placeholders.

TL;DR — quick start

0. Pass 0 — establish the change set (merge-base / gh pr diff); record base/head; note out-of-scope paths.
1. Pass 1 — classify risk by blast radius; name journeys at risk; answer the Pass 1 questions; decide line-by-line vs skim.
2. Pass 1b — only if the repo overlay defines enforceable workflow laws (task/deploy/issue). Else skip.
3. Pass 2 — if agent-authored: intent evidence, test hunks first.
4. §2b — always (you are the reviewing LLM): trace one level deeper before BLOCKERS.
5. §2c — when wiring is at stake; tier per table, default Lite; HIGH never Skip.
6. Pass 3 — structural bar (code judo, presumptive blockers) on Pass-1-flagged hunks.
7. Emit the output template → verdict, then run the pre-send checklist. Dual-ask (ready / next / roadmap) → see Output format.

Prefer all passes in one thread. When this session authored the diff, an optional fresh thread/subagent may run the review — see Fresh context. Use a structure subagent only for Pass 3.

Definitions

Code judo example: a 40-line “adapter” that only forwards kwargs to one function → delete the adapter; call the function at the call sites.

Pincer catch example: callers treat getUser(id) as “throws if missing”; callee returns null. Silent NPE / wrong branch downstream. Reconcile: role hypothesis fails — fix contract or call sites (do not emit BLOCKERS on “throws” without opening the callee).

Pass overview

Pass 0 — Establish the change set (always)

• Branch audit: git diff $(git merge-base origin/<default> HEAD)..HEAD (merge-base semantics). Do not use a plain two-dot diff against a moved base (git diff origin/main HEAD) — it silently includes unrelated mainline history.
• PR: gh pr diff <n> or gh pr diff --patch.
• Whole-tree audit only when the user asks for one — say so explicitly in the report.
• Record base and head SHAs (or PR number + head SHA) in the report.
• Size ≠ risk. Note a large change set if it changes how you read; file/line count does not set the Pass 1 tier.
• Out of scope for findings unless they encode a code invariant the diff violates, or they are evidence that a newly added dependency is missing/unresolved: lockfiles, generated output, vendored trees, and spec/task .md files (gaps in those docs → omit or Nit, never Advisory/Blocker).

Pass 1 — Risk triage (always, after Pass 0)

Classify by blast radius, not diff size. The examples below are the portable default — a repo REVIEWS.md replaces them and may override thresholds (see Repo overlay).

When a change spans levels, report the highest and map hunks to levels.

Portable default laws (changed files only)

HIGH already names auth/secrets as blast radius. These are the line-level flags. Same shape as overlay laws: applies-to + flag + want. Quote the law under the finding.

Repo REVIEWS.md may replace or narrow these laws; it does not skip them unless it states a stricter equivalent.

Answer before deep review

1. What could go wrong? — concrete failure modes, plus the user journeys this change puts at risk (login, checkout, webhook replay) — not only which files changed. Copy/docs/internal-only → none.
2. Line-by-line vs skim? — which files/hunks need careful reading.
3. Empirical checks? — specific tests, CI command, manual steps (name them here; run only per Empirical checks). On MEDIUM+ user-facing diffs, name at least one check per at-risk journey.
4. Reversibility? — easy rollback (docs, flag-gated) vs hard/one-way (migrations, irreversible data writes, breaking API removals). Hard to reverse → raise effective risk even when the surface is small.
5. Verification gap? — do tests cover this change class, or only adjacent paths? Green CI ≠ coverage for the hunk under review. Token: covered | adjacent-only | named-unrun | none. Use named-unrun when this review listed checks but did not execute them (the default).
6. Release guardrails? — feature flag, staging-only, shadow mode.
7. Faster-merge guardrails? — tests to add, rollback plan.

Routing

• HIGH → default BLOCKERS until questions answered; never PASS on structure alone; §2c per tier table (Standard or Full — never Skip).
• MEDIUM → line-by-line on boundaries; §2c per tier table when the diff touches shared helpers or multi-route behavior; tests required for behavior changes. Open product / API-shape / irreversible-data judgement → cannot PASS (see Verdict rules).
• LOW → structure + spot-check; lean on CI; §2c Skip unless a cross-module smell is obvious (then Lite). Style, copy, and docs nits do not by themselves block PASS.

Repo overlay — which rubric wins

Check in this order; stop at the first that exists:

1. REVIEWS.md at repo root — canonical for that project.
2. AGENTS.md / CONTRIBUTING.md / .cursor/rules/* — repo laws and review hints.
3. This skill — portable default when nothing else is defined.

If the repo has REVIEWS.md, load it instead of the generic risk examples above. Still apply this skill’s process order (change set → risk → operational laws when defined → agent-authored when applicable → §2b always → §2c when wiring → structure → verdict) and the portable default laws (authz / slopsquat / instruction-injection) unless the overlay states a stricter equivalent. Overlays commonly add project-specific cross-module invariants (data-boundary rules, tier/serialization contracts) or task traceability laws — use those when present, and let repo thresholds (file size, verdict tiers) override this skill’s defaults.

Overlay law shape (apply when the overlay states a law this way; unstructured overlay text still counts if it is enforceable): path glob + what to flag + what you want instead. A sentence of good intentions is not a law. Check changed files matching the glob only — not the rest of the repo. Overlay laws add to this skill’s passes; they do not skip Pass 1–3. When a finding comes from an overlay law, quote the law under the finding.

Pass 1b — Operational laws (repo overlay only)

Run only when the overlay defines enforceable workflow laws (task trackers, issue keys, deploy gates). Skip otherwise — even if REVIEWS.md exists. Do not invent Taskwarrior (or similar) checks for repos that do not use them.

When the overlay defines task traceability (in REVIEWS.md, an AGENTS.md core law, or a contributor doc):

1. Load the repo’s task-session skill if the overlay references one (path per overlay).
2. Run the overlay’s falsifiable checks before Pass 3 — intent link, scope reconciliation (done | pending | task add), no orphan non-trivial code hunks.
3. Apply the overlay’s verdict tier — a repo may make agent-authored src/** gaps a presumptive blocker; portable default is ADVISORY unless the repo says otherwise.
4. List exact task add … command strings for follow-ups, even if not executed in-session.

Empirical pattern: task list / task <uuid> info (or the repo’s equivalent CLI) — cite UUID in Task coverage; numeric ID is not a stable link.

If the repo rubric already contains a full task-traceability section, that section wins over this summary.

Pass 2 — Agent-authored changes (when applicable)

Run only when the diff is agent-authored (or this session authored it). §2b and §2c are not gated on this pass — see their triggers.

Treat agent output as unreviewed external contribution — plausible code, missing intent.

Require intent evidence before deep review:

• PR/MR description, issue link, or short what / why / out of scope.
• Agent loops: frozen goal spec + verifier output — not the agent’s “done” message.

Presentation ≠ proof: polished description, screenshots, or “tested” claims without verifier output, linked CI, or a named runnable path → Advisory [unverified_claim] (Blocker on HIGH).

Trust / spray: repeated human evidence of intent + tests → skim more (still risk-triage). First-touch + many PRs in one burst → require issue-first intent package before line-by-line.

Review order:

1. Test hunks first — assertion gaming is common; green CI ≠ correct until test edits are justified.
2. Implementation — line-by-line on Pass-1-flagged hunks.
3. CI / guard diffs — skipped tests, lowered coverage, disabled lint.

Small diffs: review works best on chunks you would throw away if derailed.

Example-bound fixes: flag when a change handles the demonstrated case (fixture, repro, sample path) but not the general class (other callers, inputs, error modes). Emit as Advisory [example_bound_fix] unless it leaves a MEDIUM+ failure mode open — then Blocker.

Decision audit (optional — MEDIUM+ agent-authored, or when the same agent authored and reviews): short dump of product/API/error/scope/test choices only — not style nits. For each: why, alternative considered (or “none”), confidence high | medium | low + what would falsify it. End with: stand behind in prod? yes | no — if no, exact gaps. Do not rewrite code in this step; surface decisions for the human.

Fresh context (optional)

When this session authored the diff and the host can start a subagent or new thread, prefer that over same-session self-review. Hand it only a review package: Pass 0 change set (base/head SHAs or the diff), Pass 1 risk summary, and pointers to REVIEWS.md / overlay paths — not prior justifications or session history. The reviewer runs this same UCR rubric; do not invent a second review system. If no subagent/new thread is available, stay in-thread and apply §2b to yourself.

Pass 2b — Agent-as-reviewer (always)

Trigger: always. This agent running the skill is the LLM reviewer — apply these rules to yourself, not only to a nested subagent. Independent of who authored the diff.

Review prompt (agent or human): ask for behavior and consistency across routes/modules, not only “block production PR” or a security checklist. Name cross-cutting invariants to verify (from REVIEWS.md, ADRs, or architecture docs when present).

Before BLOCKERS on a HIGH finding:

1. Trace one level deeper — open the callee/import the finding cites; confirm the failure mode (throw vs return null vs early exit). This is the Lite pincer.
2. Live-path gate — before [must-fix] on a helper, schema, or validator: cite ≥1 production call site (not tests-only) that this diff can reach. Unused / tests-only drift → Advisory [latent_contract]. A failure mode already present on the base branch and not newly exposed or widened by this diff → Advisory [preexisting], not a blocker.
3. Cross-module claims — list affected files; line-by-line each boundary; prefer targeted tests / empirical checks over single-pass inference.
4. Second pass when stakes are high — consistency-focused re-run or stronger model when correctness lives in wiring (fallback chains, deploy pipelines, auth middleware, event → side-effect paths).

Do not treat more reasoning effort or a longer prompt as a substitute for (1)–(4). Prompt framing and call-chain depth move results more than “think harder.”

Parent agent duty: when delegating Pass 3, complete Pass 1 + §2b + §2c at the appropriate tier on HIGH-risk wiring before trusting a subagent PASS — see Workflow with subagents.

Pass 2c — Pincer review (call-graph)

Trigger: when correctness lives in wiring — shared helpers, middleware, SDK/webhook clients, event → side-effect paths, MEDIUM/HIGH hunks from Pass 1, or any cross-module BLOCKERS claim from §2b. Not gated on Pass 2 (authorship). Tier from the table below; default Lite.

Bidirectional pass for integration defects — bugs in assumptions between modules, invisible in single-file review. Origin: pincer / pinch (Roma, пинцер манёвр) — situation model ↑, falsifiable role hypothesis ↓, reconcile (pinch).

Scale to the diff. This table is the only place that defines tier criteria:

Heuristic: PR fits one chat context, <5 changed symbols across <3 modules → Lite. Token redistribution on a landing page → Lite. Middleware used in twelve routes → Standard. Auth/session rewrite → Full.

Context Guard

If the diff / context window is truncated, downgrade §2c to Lite and explicitly list unseen modules as unverified integration risks. Do not claim Full/Standard coverage you did not read.

If the tier table selects Full but FULL-PINCER.md is unavailable (partial install), run Standard and mark the Full tier unverified in the report.

Between-file prompts (Lite and above)

• Caller/callee assumptions — Who creates this instance, how long does it live, can it be recreated?
• Persistence boundary — Mutable state: survive restart / crash / deploy / scale-out? In-memory vs disk/queue/DB mismatches are canonical pinch material.
• Two mental models — “What A does” vs “what B expects”; reconcile is the diff — start there, not after line-by-line confirmation.

Reconcile verdicts (Standard / Full)

Hard rule: confirmed is exclusive with findings on that edge. If you emit any finding (blocker, advisory, or nit) for the edge, reconcile is revise or abandon — never confirmed.

Lens: semantic compression — repeated meaning through one path; unique meaning stays local. Wrong abstraction costs more than a little duplication — prefer leave duplicated when in doubt.

Finding bar: actionable only; every finding needs evidence at a real line; categories include duplication, wrong_abstraction_level, boundary_violation, hidden_side_effect, error_handling, complexity, naming, dead_code.

How to execute (Lite / Standard)

1. List changed symbols and call edges (grep, LSP, graph tool).
2. Lite: open the callee once (§2b step 1); run between-file prompts on that edge; stop.
3. Standard: bottom-up summary for touched callees → top-down role hypothesis from call sites → reconcile (pinch). Isolation between passes is ideal, not mandatory.
4. Empirical check — one per non-confirmed reconcile (see below).

Empirical checks

Name concrete checks in Pass 1 and after non-confirmed reconciles. On MEDIUM+ user-facing diffs, name at least one check per at-risk journey (happy path is enough; add an edge or adversarial case only for auth, payments, or state-machine changes). Run a check only if the host permits and the user asked for verification; otherwise emit it as a required check under Empirical checks in the report and leave execution to the human. Do not start fixing, inventing tests, or spinning up the app as part of the sensor emit (pipeline contract).

Pass 3 — Structural bar (thermo-nuclear)

After Pass 1 (and 2 when agent-authored), audit for code judo: whole branches, helpers, or layers that can disappear.

Perform a deep code quality audit of the change. Rethink structure so behavior stays the same but the implementation becomes simpler, smaller, and more direct. Measure twice, cut once.

Named anti-patterns (same bar, not new blocker rows): throwaway code — a prototype merged as the permanent path; piecemeal growth — expedient patches that erode the layer. Prefer deletion over another patch.

Ambition over politeness. Do not rubber-stamp “it works.”

Presumptive blockers

Block unless clearly justified:

Also load repo-specific laws from AGENTS.md / lint rules when present. Repo REVIEWS.md may elevate #14 to a hard blocker; repo thresholds override #2's default.

Primary questions

• Code-judo move that deletes branches/layers?
• Right file and layer?
• Abstraction earning its keep?
• Types explicit at boundaries?
• Orchestration unnecessarily sequential?

Finding priority

1. Risk + unanswered Pass 1 questions (HIGH/MEDIUM)
2. Pass 1b operational-law violations
3. §2c pinch findings (contract mismatch on wiring)
4. Presumptive blockers
5. Missed dramatic simplification
6. Spaghetti / branching
7. Boundary / type problems
8. File size / decomposition
9. Style nits (only if nothing above)

Deprioritize: import order, line length, pre-existing warnings in untouched files. Speculative DoS / rate-limit / unproven validation stay off BLOCKERS (see noise filter).

Output format

Dual-ask: if the user also wants product status (ready / next / roadmap / progress), answer that first, then emit the UCR sensor. Do not let the rubric crowd out the human's ask.

### Ready
- …

### Next (per docs)
- … (cite ROADMAP / docs; mark gaps vs code)

Core block (always emit)

### Risk
HIGH | MEDIUM | LOW

### What could go wrong?
- …

### Review depth
- **Journeys at risk:** … | none
- **Line-by-line:** …
- **Empirical checks:** … (required checks named here; run only if host permits and user asked)
- **Reversibility:** easy | hard/one-way — …
- **Verification gap:** covered | adjacent-only | named-unrun | none — …
- **Release guardrails:** feature flag | staging-only | shadow | none
- **Change set:** base `…` → head `…` (or PR #N)

### Verdict
PASS | ADVISORY | BLOCKERS

Then, only when non-empty. Behavioral [must-fix] uses given / when / then plus a live path this diff can reach. Structural presumptive blockers (#1–#7, #14) cite a line only — do not invent a repro.

### Blockers
- [must-fix] … — given … / when … / then … (live path: …)

### Advisory
- [should-fix] … (use `[example_bound_fix]`, `[latent_contract]`, `[preexisting]`, `[unverified_claim]`, `[needs_judgement]`, `[authz]`, `[slopsquat]`, or `[instruction_injection]` when applicable)

Add-on block (emit only if the corresponding pass ran)

### Task coverage
- **Claimed task(s):** `<uuid>` — description (not numeric ID alone)
- **Reconciled:** done | pending | new (`task add` listed below) | missing
- **New tasks needed:** exact `task add …` commands (or “none”)
- **Task verdict** (section-scoped — feeds the sensor verdict, is not it): pass | advisory | blocker

### Pincer (note tier: Lite | Standard | Full)
- **Situation model (bottom-up):** … *(Standard/Full)*
- **Role hypothesis (top-down):** …
- **Reconcile:** confirmed | revise | abandon — … *(Standard/Full)*
- **Consolidation:** extract | leave | inline/split — … *(Full only)*

### Decision audit
- **Choices:** … (product / API / error / scope / test only)
- **Example-bound?** yes | no — …
- **Open debt:** …
- **Stand behind in prod?** yes | no — if no, exact gaps

### Code judo
- High-impact structural simplifications only.

### Nits
- Only if verdict is `PASS` or `ADVISORY`, and the list is short. (Omit on `BLOCKERS`.)

Verdict rules:

• BLOCKERS — HIGH with open Pass 1 questions, any presumptive blocker, or repo law violated. Proven [authz] (live path to IDOR / open storage / leaked secret) and proven [slopsquat] (unresolved or typosquat dep on a shipped path) count.
• ADVISORY — no blockers; meaningful simplification still recommended, or a product / API-shape / irreversible-data choice still needs a person's judgement ([needs_judgement]). [instruction_injection] stays here until a live path is shown.
• PASS — risk acceptable; no structural regression; no open product / API-shape / irreversible-data judgement. “It works” is not enough alone. HIGH never PASS on structure alone. Style, copy, and docs nits do not by themselves block PASS.
• Noise filter — do not emit BLOCKERS for speculative DoS, missing rate-limits, open-redirect without a session/token steal, memory/CPU exhaustion, or input-validation gaps without a proven impact path. Those stay Advisory or omit. This does not relax [authz], secret leak, data-loss, or false-closure.

Who acts on what:

• Blockers → safe input for agent fix loops (when the harness enables them)
• Advisory / Nits → human decision only; out of scope for unprompted agent follow-up

Consistency lock (non-negotiable):

• ### Verdict is the only sensor verdict; section-scoped verdicts (Task coverage, reconcile) never substitute for it
• Non-empty ### Blockers ⇒ ### Verdict must be BLOCKERS
• ADVISORY or PASS ⇒ omit ### Blockers entirely (move items to Advisory / Nits)
• Open product / API-shape / irreversible-data judgement (including [needs_judgement] or decision audit Stand behind in prod? no) ⇒ cannot be PASS; if the gaps are must-fix, verdict is BLOCKERS

Pre-send checklist (run before finishing — especially on smaller / faster models):

1. ### Blockers omitted iff verdict ≠ BLOCKERS; no empty or placeholder-filled sections
2. Each behavioral [must-fix] cites a live production path this diff can reach (not tests-only, not base-only) and a given / when / then repro. Structural blockers cite a line. [preexisting] stays Advisory.
3. Pincer confirmed ⇒ no findings on that edge
4. Dual-ask answered first when the user asked ready / next / roadmap
5. Change set (base/head) recorded; HIGH used §2c per tier table (never Skip)
6. PASS ⇒ no open product / API-shape / irreversible-data judgement and no [needs_judgement]

Workflow with subagents

Fresh UCR reviewer (optional): see Fresh context — same rubric, review package only, no alternate checklist.

Structure subagent (Pass 3): when a structure-only subagent exists (e.g. Cursor Task subagent_type: "thermo-nuclear-code-quality-review"), or a sibling thermo-nuclear skill on hosts without that subagent:

1. Parent loads this skill (and repo REVIEWS.md if it exists).
2. Parent completes Pass 0 + Pass 1 (+ Pass 1b when the overlay defines operational laws; Pass 2 if agent-authored; §2b always; §2c when wiring is at stake).
3. Invoke the subagent with diff + file contents and explicit instruction: apply Pass 3 only on Pass-1-flagged hunks; output must include the Pass 1 summary + this skill’s verdict. Subagent output alone is not enough for HIGH-risk cross-module wiring — the parent verifies §2b and §2c when triggered.

If no structure subagent is available: run Pass 3 in this same thread. Never omit the structural bar.

Further reading (not loaded by default)

• FULL-PINCER.md — §2c Full tier harness only.
• SOURCES.md — provenance and credits; not required to execute this skill.