pr-review

# Pull request review queue The maintainer side of the review pipeline. CI posts the pinned review (the v3 author card + reviewer brief, or a legacy `<!-- CLAUDE_REVIEW N/M -->` monolith); this skill reads it as the source of truth for every open PR at once, computes one verdict per PR, and turns the approver's decisions into a batch of GitHub actions. Deterministic scripts do the collecting, judging-by-rule and rendering; the model writes only the judgment calls on rows that earned one; a shared act primitive executes. Everything lives in `scripts/review-v3/` (covered by `make test-review-pipeline`): | Step | Script | Model? | |---|---|---| | collect | `collect.py` → `.pr-review-queue.json` | no | | analyze | `analyze.py` (verdict, reason codes, cross-PR) | no | | judge | this skill, judge rows only → `.pr-review-judgments.json` | **yes** | | render | `render.py` (board Artifact / detail / `--terminal`) | no | | act | `act.py` (plan → preview → execute) — **only on an explicit `--act`** | no | ## Usage ```text /pr-review # the queue: my lanes, rendered as the board Artifact /pr-review 21598 # one row, expanded (findings, preview links, actions) /pr-review --verdict stamp --terminal # just the stampable set, with the act command /pr-review --owner any --domain blog # what marketing has to decide /pr-review --act --stamp 21550,21577 --route 21431:@cnunciato --unblock 21525 flags: --owner me|any|<role>|@login --domain docs,blog,website,programs,infra,frontend,other --include-handed-off --verdict stamp,judge,route,blocked --author app/workprentice,pulumi-bot,any --since 7d --include-infra --strict-stances --board|--terminal --ai|--no-ai --act --stamp N[:merge|:no-merge],N --route N[:@target] --request-changes N --chain C1 --unblock N --fix N --close N [--superseded-by M | N:M] --ask-fix N --refresh N --rerun N --rerun-checks N --render N --deploy N --reason "TEXT" | "N=TEXT" --approve-note "one sentence" [--merge-humans] [--no-merge] [--force] [--dry-run] ``` Every `--act` flag that takes a PR is repeatable (`--close 1 --close 2`); `--stamp` also takes a comma list, and its `:merge` / `:no-merge` suffix is per PR. `--reason "N=text"` scopes a reason to one PR; bare text is accepted only when exactly one step in the plan takes a reason. `--superseded-by M` needs exactly one `--close`; with several, say which as `--superseded-by N:M`. `--approve-note` is one sentence appended to every approval body in the batch. `act.py --help` is authoritative. `--board` is the default, always: render the page and publish it with the Artifact tool. Only an explicit `--terminal` in the person's own invocation prints the table instead — a queue with no judge rows in it is still a board, and "the rows were easy to describe" is not a reason to fall back to text. Both are renderings of the same `queue.json`; there is no separate code path. ## How a run ends A run invoked without `--act` ends at the published board: collect → analyze → judge → render → publish → stop. The last thing you say is one line — the board link and the command the page composed. Do not plan, do not ask whether to plan, and do not ask whether to execute. The board *is* the deliverable. The person reads each row, sets the decisions themselves, and copies the command the page composed for them; **that copy-paste is what authorizes the writes**, and a batch that reaches GitHub without it has skipped the reading the board exists to make possible. **A write is never the default answer to a question.** `AskUserQuestion` may carry one — a single named action on a row the person just walked through is a fair thing to offer, and never offering it is its own kind of unhelpful — but three things have to hold together: 1. **It is neither the first option nor the recommended one.** The default sits on the choice that changes nothing, because the cost of a mis-click is asymmetric: the wrong read-only answer costs a round trip, the wrong write answer costs an approval nobody can take back. 2. **The label says the write out loud, and how far it reaches.** "Approve and squash-merge #21622" is a label. "Proceed", "Yes, go ahead" and "Run it" are not: from the option alone, the person has to be able to tell that GitHub is about to change and by how much. 3. **They have already seen the thing itself** — that row, its findings, its diff — in this turn, not just a count of rows. A question is consent to what was read; it cannot manufacture the reading. A batch fails the third one by construction. "Approve & merge these 7" is one keystroke over seven PRs nobody opened, so the stampable set goes to the board and comes back as a command the person invokes, never as a menu option. Scale is the line: one reviewed row may be a question, a set is always the board. The corollary, so this doesn't swing back the other way: the question you must not ask at all is *"want me to plan it?"* Planning is read-only, so proposing it wastes a round trip — and on a queue run it isn't yours to propose in the first place, because step 5 starts with the person, not with you. Publish and stop; their next invocation is the yes. ## Config: `~/.pr-review.yml` Local, never committed. It says which routing lanes are *yours*; routing itself (lane → owning team) comes from `.github/review-routing.yml`. ```yaml me: [docs, infra, frontend, other] # lanes I approve for; blog/website route to marketing stamp_max_lines: 40 # a diff at or over this is never a stamp stale_date_days: 3 # a blog `date:` older than this is stale link_fixes: mine # mine (default) | route: a link-only diff is yours whatever its lane ``` `.github/review-routing.yml` carries `link_only: { approval: any-team }`, which says any review team may approve a diff whose every changed line differs only in a link. Those rows get `gate:any-team` and are any approver's, and the Sentinel agrees, so `link_fixes` only matters where that policy is `lane`. `link_fixes: mine` is then for the redirect sweeps: a diff where every changed line is the same sentence with only a link rewritten (text or target; a word's casing may change too) gets `shape:link-only` and skips the lane check, since the lane owner's review buys nothing there. It's the queue's own bar, narrower than the Sentinel's mechanical bar, which counts any link edit as substantive. The row still has to clear the stamp bar; a sweep with open ⚠️ rows is a judge row with "approve as-is" as its primary. ### What makes a row "mine" Ownership is per **lane** (a content domain), never per author or per repo area, and it is computed, not declared on the PR: 1. **Every changed path gets a subject** from `classify_path()` in triage-classify.py — the same classifier that writes the `domain:*` labels, so the board and triage can't disagree. The closed set is `docs`, `blog`, `website`, `programs`, `infra`, `frontend`, `other`; a path nothing else claims (`.claude/`, `styles/`, generated `data/`) lands in `other`. 2. **The PR's domains are the union of its files' subjects.** A PR that touches a blog post and a doc is in both lanes at once. 3. **Every row has a lane team on the hook.** `resolve_lanes` always comes back with at least one required role: `none` cells are a config error in `.github/review-routing.yml`, and a mechanical docs/blog edit routes to its lane exactly like a substantive one — `mechanical` skips the *model* review, never the approver. The `gate:none` reason code that used to mark an ungated row is gone with the state it described. 4. **Otherwise a row is mine if any of its domains is in `me`.** Intersection, not containment: one file in a lane you own makes the whole PR yours to approve, because the queue is asking "can I act on this?", not "is all of it mine?". With `me: [docs, infra, frontend, other]`, that means everything except a PR whose files are *entirely* blog and website. 5. **The rest routes**, and `.github/review-routing.yml` picks the target: the matrix maps subject × change type (mechanical vs substantive) to a role, the role maps to a GitHub team, and the team is the target unless GitHub says it doesn't exist (`route:no-team`, which falls back to that role's `sla.escalate_to` person; `route:team-unverified` means the token couldn't read teams, so the configured team is used unchecked). Blog is the blog team's, website and frontend marketing's, docs and programs the docs guild's, infra and other tools'. 6. **Two overrides, both visible as reason chips.** `link-fixes:mine` makes a `shape:link-only` diff yours whatever lane it sits in (config above). `handed-off:@who` takes a row out of your list entirely — someone else is already requested as reviewer — and parks it under "Waiting on others" until they act. 7. **`--owner` reinterprets step 4 only.** `me` is the default, `any` drops the lane check, and a role name or `@login` substitutes someone else's lanes so you can see the queue through their eyes. The `owner:<domain>:<role>` chip on each row shows the computation, one chip per domain the PR touches. The chips that explain why a row is on your board at all — `gate:any-team`, `link-fixes:mine`, `route:no-team`, `route:team-unverified` — render as plain words, with the code kept in the chip's tooltip. Missing file: the lanes come from your GitHub team memberships — every team in `.github/review-routing.yml` you belong to contributes the subjects its role owns, so a first run with no local setup already shows route rows. Only when those memberships can't be read (a token without `read:org`) does every lane count as mine, and the analyzer says which of the two happened. `python3 scripts/review-v3/pr_review_config.py` prints the effective config. The AI-suspect allowlist stays at `~/.claude/pr-review/ai-suspect-authors.txt` (see `pr-review:references:trust-and-scrutiny`). ## The flow Run the scripts from the repo root. Each is idempotent; re-running after acting updates the board rather than rewriting it. ### 1. Collect ```bash python3 scripts/review-v3/collect.py --out .pr-review-queue.json [--pr N] [--author A] [--since 7d] [--ai|--no-ai] ``` Every open, non-draft PR (an explicit `--pr N` also collects a draft): title, author with the trust axes and risk tier, labels, files with patches, head/base SHAs, `mergeable_state` (re-asked while GitHub still says `unknown`), the check rollup, reviews, requested reviewers, the parsed pinned review (`review-worklist.py`, both surfaces), its `REVIEW_STATE`, triage's `<!-- TRIAGE_PROSE -->` comment, the reviewed-head SHA from `<!-- CLAUDE_REVIEW_HEAD … -->`, and the preview URL plus per-page links. Responses that only change when the PR does are cached under `/.pr-review-cache/<pr>/` per (head SHA, updated_at). GitHub access: `gh` when installed, otherwise the REST API with `GITHUB_TOKEN` / `GH_TOKEN` (a Claude Code web session has the token but not `gh`). `--snapshot-dir DIR` reads endpoint JSON from files instead — the test backend, and the way to hand the scripts data fetched by the GitHub MCP tools: write each response to `DIR/GET/<endpoint path>.json` (`gh_client.py` documents the layout). GitHub App authors search as `author:app/<slug>` (`author:WorkPrentice` returns nothing); `--author` accepts any spelling. A legacy (v2) review over one comment's size limit is split across several, each stamped `<!-- CLAUDE_REVIEW k/N -->` on its first line; the collector gathers every page and joins them in page order. Its findings sections are the *tail* of the document, so a page-1-only read reported a review with no findings at all — which on #21490 turned a `blocked` row into a `judge` row wearing an "approve as-is & merge" button and passed the Sentinel's G2 gate as "legacy review clean". Review status per PR, label-independent: `CURRENT` needs the card's `CLAUDE_REVIEW_HEAD` to prefix-match the live head — pushes made with `GITHUB_TOKEN` never fire `synchronize`, so a PR can sit at `review:no-blockers` with a review describing content a later commit replaced. `STALE` / `IN_PROGRESS` / `ERROR` / `ABSENT` / `TRIAGE_PROSE` otherwise. > **v3 surface guard.** If a PR's review comments contain `<!-- CLAUDE_REVIEW_AUTHOR -->`, the PR is on the v3 surface and no local refresh machinery applies to it. The findings are the author card's blocking rows (🚨 / ❓, each with an `F<n>` id; dispositions live in its `REVIEW_STATE` block) plus the reviewer's guide's `### ⚠️ Check these before approving` rows — `collect.py` reads both. **Never run the local `docs-review:references:update` refresh or `pinned-comment.sh upsert` on a v3 PR** — they write a legacy monolith beside the cards. When a v3 review is `STALE`, the row is *blocked* with a `--refresh N` action that comments `@claude <reason> #update-review`; when it is `ERROR`, with a `--rerun N` action that comments `@claude <reason> #new-review` (the pipeline's from-scratch escape hatch); wait for the card to re-render (the label returns to `review:outstanding-issues` / `review:no-blockers`), then re-collect. The Sentinel check on the PR is the merge gate; stamp only when it is green or would be. A legacy (v2) PR that is `STALE` gets the same `--refresh` action; the CI update lane rewrites the monolith. This skill never refreshes a review locally. ### 2. Analyze ```bash python3 scripts/review-v3/analyze.py --in .pr-review-queue.json [--owner me] [--include-infra] [--strict-stances] [--domain …] [--verdict …] [--author …] ``` Per PR: `domains` (`classify_path` via `routing.resolve_lanes`), the owning role and team per domain, exactly one `verdict`, a `reasons` list of machine-readable codes, `gate_fails` (every stamp gate this row missed, which is how the opening tells a row that needs reading from one that merely has to merge after another), `actions` (the `--act` fragments that apply), and `handoffs` (interactive runs that are not `act.py` fragments — today just "fix it yourself" on a workflow-authored row with open findings). Cross-PR, computed over the full set before any filter: **collision clusters** (union-find over shared paths; a pair is `overlap` when the hunks intersect on the base side, else `same-file`, with a suggested merge order), **directional conflicts** (a PR adds links to a URL that exists only as a Hugo `aliases:` entry while another open PR removes links from it), **duplicate candidates** (shared file, similar title, opened within 10 minutes), **stale blog dates**, **stale reviews**, and **stale brief summaries** (a "What this PR changes" bullet naming a value the diff no longer contains). | Verdict | Meaning | Board | |---|---|---| | `stamp` | Every gate passes; approving asserts nothing beyond what's machine-verified. | "Approve & merge" starts selected. One command approves and squash-merges the set. | | `judge` | Decidable from the board; something needs a human's call. | Judgment box: the lines, the question, a deep link, a recommended disposition. | | `route` | Not this approver's lane per the routing matrix. | Batch "request review from owner"; any defect found rides along. | | `blocked` | Can't merge regardless: unanswered 🚨 review findings, conflict, red CI, stale or errored review, changes requested by someone else, your own PR. | Names the blocker and offers the mechanical unblock (merge base, refresh, re-run the review, re-run the failed checks, send back / close for open 🚨, route for another lane's). A row with none says "no action available". | **Handed off.** A PR whose requested reviewers include a human who isn't you (and don't include you) is waiting on them, not you. It keeps its verdict in `queue.json` but leaves the groups: the board and `--terminal` list it once, compactly, under "Waiting on others" (who, how old, a ✗ for red CI or ⚠ for a conflict), and `--include-handed-off` brings the full rows back when you need to act on one. A requested team counts as yours when `review-routing.yml` maps it to a lane in `me:`. The review request is the memory: it lives on the PR, every session sees it, and GitHub clears it when the reviewer acts, which is when the row returns. `--route N:@login` is therefore also the "don't show me this again" button. Collisions and directional conflicts against a handed-off PR are advisory (`:theirs`): they no longer gate your stamp, and a cluster with at most one of your PRs in it drops out of the pinned slots. The stamp bar, **all required**: label `review:no-blockers` (`review:trivial` is not enough — no review ran; after a base merge the card itself saying nothing blocks, with no open ⚠️ row, also counts); zero ⚠️ rows on the brief, whatever disposition one carries (zero low-confidence items on a legacy review); review `CURRENT`; `mergeable_state` in {clean, blocked} with checks green, the Sentinel left out of that rollup as `act.py` leaves it out; no changes-requested review from anyone but you; no overlap collision, directional conflict or duplicate; one of the PR's domains is in `me`; not your own PR; no new file under `content/blog/`; no blog `date:` this PR sets that is older than `stale_date_days` (`blog:stale-date:<date>`; a link fix in an old post is not a backdate, only an added post or a `+date:` line in the diff counts); no `layouts/` or `.github/` change unless `--include-infra`; changed lines under `stamp_max_lines`; scrutiny not heightened. Editorial stances on the brief block only with `--strict-stances`. Heightened scrutiny (AI-suspect on a human author) caps a row at `judge`. Precedence: blocked > route > stamp/judge. **A review that did not arrive whole is blocked, never judged.** Two signals, either one enough: GitHub did not return a page the `k/N` markers promise (`review.pages_missing`), or the card's own tally table declares more findings than its sections parsed into (`counts_shortfall` — the tally and the sections are rendered by the same pass, so the table is the one thing that can notice a truncated body from the outside). The row wears `review:unreadable:<why>`, its unblock is a fresh review, and `act.py` refuses the merge in its own preflight rather than trusting the queue. A review that merely parsed into nothing with nothing to corroborate it — a v2 card with no tally table, a v3 card missing its head sentinel or carrying a broken `REVIEW_STATE` — is not blocked (nothing says there *are* findings) but is never stampable either: `review:parse-confidence:low`. One thing that looks like a blocker is not. Your **own** changes-requested review does not block your stamp: the approval you are about to post supersedes it (`act.py` skips it in the preflight, and the analyzer reads it as `sent-back:<date>` — the row is waiting on the author, not on you).