pr-review

Files (4)

• SKILL.md
• bench-format.md · references/
• report-template.md · references/
• pr-drift.sh · scripts/

SKILL.md (first lines)

# PR review v1.1

## The problem this playbook is for

A large PR cannot be read top to bottom. Even when it can, you cannot weight what matters. And
**the PR body is only a projection of what the author believes, and it goes stale before the diff does.**

So this playbook is not built around an order for reading the diff. It is built around
**leaving a record of what you decided not to look at.**

## Principles

1. **A finding you cannot refute is not a finding.** If you cannot name a concrete input or state → a
   wrong output or a loss, it does not become a finding. "Hard to read" and "this would be nicer" are outside this playbook.
2. **The body is a hypothesis. The diff is fact.** Never source a claim from the body.
3. **Split the role that finds from the role that confirms.** Tell the verifying role to *refute*, not to support.
4. **Precision over recall, by default.** A review carrying 40 nits stops being read. Raise the depth deliberately.
5. **Never wave through what you did not look at.** It is the gatekeeper's one real offence.
6. **The reviewer does not edit the code under review.** Fixing it is the author's job.

## What you do not call while this playbook is running

Review is neither creative work nor implementation, so **do not call the following even when their trigger
conditions appear to match**. Calling them turns the review into a different task.

| Do not call | Why |
|---|---|
| `plumb:graph` | A large PR looks like its territory (dependencies, parallel work, several sources of truth, verification), but it is a **pre-work design** skill. It converts the review into a design task |
| `/simplify` | "Clean this up" lands on it. It is quality-only and it applies fixes. Against principle 6 |
| plumb's `playbooks/shaping-the-work.md` | A design conversation starts. Review is not creative work |
| plumb's `playbooks/being-reviewed.md` | The **author's** playbook. Wearing both hats on one PR leaves a record of you judging your own diff. It is what the author reads after you return, not what you read |
| `/code-review --fix` / `--comment` | `--fix` is against principle 6. `--comment` sends findings that never passed stage 5's verdict straight to the author, cut loose from the approval decision (returning is concentrated in the one place that is stage 6) |

These, on the other hand, you **do pull in**.

- **principle-gate-claims-on-evidence** — it feeds directly into the CONFIRMED verdict in stage 5
- plumb's `playbooks/worktree-setup.md` — isolation for stage 3's reproduction tests
- plumb's `playbooks/fan-out.md` — stage 3's parallel execution

## Steps

### 0. Pin the revision

Start here when all you were handed is a PR URL.

1. Split the URL into `owner/repo` and the PR number
2. Clone the repo if it is not local; otherwise `git fetch origin pull/<n>/head:pr<n>`
3. Detect drift and branch protection

```bash
plumb-pr-drift <owner/repo> <PR number>
```

- Record the SHA under review. **Everything below is done against that SHA.**
- Read the protection warnings it prints. If `dismiss_stale_reviews_on_push` is false, a push after your
  approval turns the approval into a blank cheque. Decide whether to approve with that fact included.
- On a long-lived PR you are reviewing for the second time or later, re-run everything below against the
  diff from the SHA you read last time.

**Run stage 0 again immediately before you build what you return.** (a failure taken in real use)

The PR moves while you review it. Measured: 13 commits landed during a two-hour review, and:

- **5 of 9 findings had their anchor line shift** (GitHub draws the draft against the current diff, so an
  inline comment tied to an old SHA points at a different line)
- 3 findings were spent because **the document they targeted was deleted outright**
- 1 **changed shape when a fix landed**
- and those 13 commits had brought in **2 new BLOCK candidates**

Pinned at the start is not enough. **Take it again just before you submit, and if it moved, review the
difference before you return.**

**If the PR keeps moving, tell the author that fact itself.** Chasing a moving target keeps the review
permanently out of date. Treat it as a structural problem with the PR, the same as size or body drift.
- **Dump the diff to a file** (stage 1 uses it):
  `git diff <base>...<head> > <scratch>/pr<n>.diff`

**Reading the tree, in every stage below.** Three or more greps over the same tree become one
sweep script; its output goes to a file, and what comes back is the pointer plus the deciding lines
(`playbooks/batching-chatty-tools.md`). Measured on 2026-09-03: two reviews of the same PR at the
same depth cost 37 and 62 requests, every one a shell call, with the bytes returned flat
(138KB and 141KB) - the request count was the whole difference.

### 1. Bidirectional inventory

Build **two independent** sets of claims.

**(a) From the body** — pull out of the PR body and the commit messages only the claims whose truth can be
decided. Do not verify them here.

**(b) From the diff** — hand the `plumb:pr-diff-reader` agent **only the diff file you dumped in stage 0**.
It is an explorer-shaped role: when `plumb-config role.explorer.model` resolves, pass that as its
`model`; unset, it inherits the main session's.
That agent holds nothing but `Read` / `Grep` / `Glob`, so **it cannot fetch the body**.
Tool permissions enforce this, not a polite instruction.

The six bundled agents live in `agents/`: `pr-diff-reader`, `pr-invariant`, `pr-cutover`, `pr-repro`,
`pr-refuter`, `pr-blindspot`. Launch each by its plugin-qualified name (`plumb:pr-refuter` and so on).
An installed plugin does not register the bare name, and it can resolve to an unrelated personal agent.

Then **take the set difference.**

| Gap | What it means | Severity |
|---|---|---|
| In the body, not in the diff | Unimplemented, or claimed and then taken back. It misleads the approver | Medium |
| **In the diff, not in the body** | **An undeclared change** | **Always high** |

`plumb-pr-drift`'s "commits after the body was last edited" is a mechanical lower bound on the second one.
Zero does not clear you (the body can have been incomplete from the start), but non-zero means something
is there for certain.

### 2. Weighting, and the cut

Sort every claim by ⟨irreversibility⟩ × ⟨money or data lost⟩ and draw the line at your time budget.

**Write out, right there, the list of what fell below the line.** Stage 6 returns it.
The quality of a review is decided less by what you looked at than by what you decided not to look at.

Irreversibility, high to low: production DB migration / a charge or invoice becoming final / state changed

From Haruki1090/plumb, MIT.

More from Haruki1090/plumb