--- name: review-pr description: "Review a Hyperloom pull request. Invoke with a PR number whenever asked to review, re-review, or sanity-check a PR in this repository. Collects the PR into a scratch directory, selects from rules.md only the rules this diff can trigger, forces a semantic and core-file risk assessment before any finding is written, refutes every finding twice, and publishes an English conclusion to the PR. The output has two parts only: what the PR does, and blocking issues." --- # Hyperloom PR review Written for an agent to execute top to bottom. Every step writes an artifact into `$WORK`; a step that leaves nothing behind is one a review can skip without the skip being visible. `$WORK` defaults to `/tmp/hl-review-` and is printed by `fetch.sh`. ## Output contract The published review has exactly two parts: what the PR does, and blocking issues. Nothing else. Non-blocking observations are deleted, not softened. Not "note", not "FYI", not "non-blocking but", not "for reference", not a postscript under the conclusion. If the only way to keep an observation is to relabel it, it fails the bar and goes. The bar is two questions: **if this is not changed, is the current behaviour wrong?** and **was the reason for the change true, and does the change end the failure it names?** Blocking means one of four things, and the repo treats them as equal: | Kind | What counts | |---|---| | Wrong behaviour | correctness, crash, data or precision error, compatibility break, security, performance regression | | Ineffective | the premise does not reproduce at the merge base, or the failure the PR names still happens at the head, or a check is dropped as covered by something that decides less — rules E1-E5 | | Desync | an operator-observable change the PR description never states; PR title or description does not match the diff at the current head | | Contract violation | the diff breaks a rule `AGENTS.md` states outright, and the finding names the bullet it breaks plus, where that bullet is about ownership, the module in this repo that already owns the concern | The second kind is the one a correct diff passes: right line by line, tested, accurately described, and still resting on a measurement that does not reproduce or leaving the reported failure happening. E1-E5 each name the tool to re-run, the branch to trace or the predicate to read; one that names neither is taste and goes like any other. The fourth kind is the narrow one. "This could be simpler", "I would have put it elsewhere" and "consider extracting a helper" are reviewer taste and are deleted like anything else. What survives is the case where the repo has already decided — a detour around a path that exists, or a concern implemented outside the module that owns it — and the decision is written down where both sides can read it. A finding that cannot name the bullet and the owner is taste wearing a citation, and it goes. If there are no blocking issues, say so explicitly. Do not manufacture small ones to fill space. The card closes with its `Checked:` line: what the review read and ran, the skill version and model that ran it, and on a re-review what became of the earlier card's findings. ## Links out, not restatement The authoring contract lives in the repo and is not copied here. Read and cite, do not restate: `AGENTS.md`, `.github/copilot-instructions.md` (including its explicit do-not-flag list), `docs/contributing/style-guide.md`, `.github/PULL_REQUEST_TEMPLATE.md`. Anything already gated by ruff, pylint, mypy, CodeQL, gitleaks, bandit, REUSE, pre-commit or the coverage floor is out of scope: re-flagging a gated rule is a defect in the review, not a finding. --- ## Step 1 — Fetch ```bash bash .claude/skills/review-pr/fetch.sh ``` Keep the `$WORK` it prints. Read `diff.txt`, `body.txt` and `comments.txt` before going on. When `comments.txt` holds an earlier card for this PR, or a human's blocking objection the author has not answered in code, settle those first and still review the whole diff: follow [`references/re-review.md`](references/re-review.md) beside the steps below. | file | what it is for | |---|---| | `meta.txt` | number, title, author, state, head sha, base ref, url, mergeable | | `title.txt`, `body.txt` | the title, one line, and the description — rule X2 checks both against the diff | | `diff.txt` | the full diff against the merge base | | `files.txt` | changed paths, one per line — the input to rule selection | | `numstat.txt`, `netlines.txt` | added, deleted, path; and E3's production total, `src/` outside `tests/`, binary rows excluded | | `commits.txt` | commit subjects, oldest first; a late commit is where a description goes stale | | `base.txt` | merge-base sha — every "is this pre-existing" question is answered against it | | `ci.txt` | check runs at the current head: name, conclusion, url | | `comments.txt` | existing review and issue comments, each headed by its author and time — an earlier card is settled as [`references/re-review.md`](references/re-review.md) says; any other point already made is not repeated (Step 7) | | `testfiles.txt`, `docfiles.txt` | changed paths under a `tests/` directory, and changed docs, prompts and `.md`/`.rst` paths — an empty `docfiles.txt` beside a `src/` change is X3's shape | | `openprs.txt` | other open PRs touching the same files — conflicting in-flight work; a `# PARTIAL:` line names what was never compared, reported as `SKIPPED: V4` (rule V4) | | `skill.txt` | this skill's version (its directory's tree hash, `+dirty` with uncommitted edits) and the environment's model — the card's `Skill:` is the version, its `Model:` the model your system prompt names, else this one | ## Step 1b — Select the rules Open the index at the top of [`rules.md`](rules.md) and take every row whose trigger matches `files.txt`, a skim of `diff.txt`, or a `label:` line in `meta.txt`. Write the union of their rule ids into `$WORK/rules.txt`, one per line, then read only those bodies. **Never read `rules.md` whole** — it holds 78 rules across 11 families, and a reviewer told to attend to all of them attends to none. Match rows generously: a row you are unsure about is taken, never dropped. V1-V8 and X2 are on every list, V2 and V7 without a verdict line. On a re-review, so are the rules of the earlier findings that still hold. ## Step 2 — Semantic understanding Answer all six from the diff, not the description. One line each into `$WORK/answers.txt`, prefixed `Q1:` … `Q6:`. An answer that anchors in nothing — no path, symbol or condition — is not an answer; rewrite it before going on. - **Q1 — State the root cause in one sentence, and say what pins it.** Not the symptom, not a list of mitigations. What pins it is evidence outside the description — failing output, a session artifact, a measurement re-run at `base.txt`; sourced to the body alone it is `[unverified]` and E1 decides what that costs. A body listing three improvements is the tell nobody found the cause. - **Q2 — Name the route, and the layer at each end.** Place two frames on this upstream-first vocabulary — `admission` (could refuse the input), `producer` (computes the value), `intermediate` (passes it on), `consumer` (acts on it): the frame this diff changes, and the earliest frame that could have refused the bad value. Both go on the card; strictly downstream with no hunk upstream is E4. - **Q3 — What does this make observable to an operator?** A behaviour, an interface, a default, a flag, an artifact path, a metric, a log line. If the answer is "nothing", say what makes that true — that claim is what the exemption in `AGENTS.md` rests on. - **Q4 — Name every consumer of a changed value.** For each new or changed status string, enum member, field, keyword argument or return shape: the membership tests, the call sites, the readers of the persisted document, the prompt that names it. Grep, do not guess. - **Q5 — What would it take for this change to be wrong?** Name the input, configuration, phase or race that would make it produce a wrong answer. "Nothing" is not an answer, and neither is a differential run that never built the shapes whose early return the diff moved (E1). - **Q6 — Name the scenario, then say whether it ends.** The scenario is the input, configuration or sequence that produces the failure, in actual values and actual order — "the interpreter already carries ray 2.44.1 with click 8.5.0 and pip cannot write to it", never "when the config is wrong". Then trace the branch the head runs under exactly that, and say what the outcome becomes. It goes on the card. Where the failure recurs the diff may still be worth landing, but it closes nothing — that is E2. ## Step 3 — Core-file risk Write one line per backbone file the diff touches into `$WORK/core_files.txt`: ``` TIER1|TIER2|TIER3 COVERED|GAP|N/A -- ``` Every changed non-test file under `src/` gets a line, Tier 3 included — check the list against `files.txt`. Writing a tier lower than `references/tiers.md` gives it is not a route past the checks the real tier requires. `COVERED` = the blast radius is exercised by this PR's tests or is unreachable from the change. `GAP` = it is not, and that goes on the card. `N/A` = the change cannot reach it. A reason that names no file or symbol this PR changes is worthless — "core file, large blast radius" is equally true of every PR ever opened against that file. A docs-, CI- or test-only diff touches nothing under `src/`: write one `NONE -- ` line. Tiers come from [`references/tiers.md`](references/tiers.md): a table of the backbone files, and Q1–Q4 for anything not in it, including new files. Q1b is the one that bites — a `Coordinator` attribute rename passes imports, lint and collection and fails hours into a session. Grep the class name, not just the symbol. ## Step 4 — Rule checklist Adjudicate every rule id in `$WORK/rules.txt`. One line each into `$WORK/verdicts.txt`: ``` FIRE|CLEAR|N/A -- ``` `CLEAR` is a claim that you looked and it does not apply *to this diff*; the reason is what makes it checkable. "ok", "n/a", "fine" are not reasons. Step 1b already cut the list to what this diff can trigger, so there is no rule here you may pass over because the list looked long. A `FIRE` must cite a file this PR changes; everything else — the header stating the contract, the prompt naming the flag, the doc that did not move — is evidence and welcome beside it. A rule whose `Severity:` is advisory in every shape it names takes no line here, and neither does one whose own body says it cannot be a finding -- V2 is the output contract and V7 is Q4, both recorded by the step that runs them. The contract cannot publish it, so adjudicate it only when it fires, as a note outside the card. Across 23 recorded reviews 815 verdict lines carried 12 `FIRE`s — spend the `CLEAR` reasons where a finding could come out. ## Step 5 — AI-code diagnostic A clean, well-written description is something AI produces easily; these are the structural places a diff that reads well hides its defects. One line per check into `$WORK/ai_diagnostic.txt`: ``` : CLEAN|HIT|N/A -- ``` "clean" alone is not an answer; a reason that names nothing in the diff is not one either. 1. `claims` — **Claim against code.** Does the code enforce what the description, the docstring, the comment and the prompt assert? Every number, flag name, path and default in the description must be greppable in the head tree; a number you cannot trace to an output is `[unverified]` and is never repeated as fact. A command or an artifact name the diff adds to an instruction — a rule's evidence step, a prompt, a runbook — is run here, once, and its output read: a step naming the wrong artifact or a deprecated form prints nothing and fails silently forever. 2. `silent-failure` — **Safety theater and silent failure.** For each new guard, `try`, `except` or default: is it reachable, will it ever fire, does it convert a failed measurement, a failed patch apply or a failed artifact write into a plausible-looking value? A fail-closed gate that coalesces a missing input to a passing default is the blocking shape. 3. `test-falsifies` — **Test calibrated to pass, not to falsify.** Does the test drive the production entry point or a mock of it? Does it assert behaviour or restate the implementation? Would it fail on the merge base? A regression test that passes on base pins nothing. 4. `patch-shape` — **A fix that cannot reach its own cause.** Run on a fix PR as the card contract defines one, against Q2's two frames, each shape naming its file:line; elsewhere write `N/A`. Any hit is an E4 candidate, and the first two are blocking on their own: - every production hunk lands in failure classification, naming or reporting — a diff whose whole footprint is what the failure is *called* cannot stop it happening (#2015); - the error class, message or identifier the body quotes appears in neither the changed files nor the code that writes it — then the diff cannot be on that path at all (#1981); - the new guard reads a value its own function did not produce, and no hunk touches it (#1982); - the permissive branch that produced the symptom survives unchanged, with the new branch added above it — narrowing a default is not changing it (#2099); - state is cleared at a reader's entry instead of where it is written or named (#2188); - the body or a new test concedes the rejected input is a reasonable thing to emit, and the diff rejects it anyway — the cause is a missing capability, not a bad caller (#1983). ## Step 6 — Free-form pass, then the build-it-again line Read the diff as someone who knows this system. Does the approach belong at this layer? Any correctness risk the rules missed — a phase entered twice, a resume landing on a state shape the new code cannot read, a destructive sweep scoped by pattern rather than by what this run owns, a number compared against one another measurement system produced? This step is where most findings come from; the rule list is a prompt for it, not a substitute. Append a `BUILD:` line to `$WORK/answers.txt`: **"If none of this code existed, what would you build — and does every mechanism this diff adds need to exist?"** Name what you would build, then say which of the diff's parts it does not contain. A part that survives only because it is already written is what this question exists to catch; where the diff is a refactor it settles under E3 on the net line count, otherwise it carries an `AGENTS.md` bullet and an owner module, or stops here. ## Step 7 — Refutation Try to kill each candidate finding before it reaches the card. One block per finding in `$WORK/refutations.txt`: ``` FINDING: ATTEMPT: OUTCOME: SURVIVES|DROPPED -- ``` The key is the same one the card will use, and every `FIRE` in `verdicts.txt` needs a block. An `ATTEMPT` that names nothing openable is not a refutation: nobody can repeat it. With nothing to refute, write a single `FINDING: none -- ` line; an empty file reads the same as a skipped step. Attack in this order: is the line added by this PR or pre-existing context around an added line (compare against `base.txt`); does the symbol resolve somewhere the diff did not show; is the trigger reachable in a real configuration; is the point already made in `comments.txt` by anything other than an earlier review-pr card. A finding on an earlier card is not "already made": it is settled in `prior_verdicts.txt`, kept if it still holds and dropped if it was fixed. Then hand the surviving findings as they will appear, the diff and `base.txt` to a reader who has not seen your reasoning — a second agent or a person — with every finding false until defended. `$WORK/independent.txt` takes the same `FINDING` / `ATTEMPT` / `OUTCOME` blocks, one per finding that survived above. With no such reader, write `FINDING: none -- no independent reader available` and say so on the card. ## Step 8 — Verdict Before writing the card, walk `verdicts.txt` in both directions: nothing goes on the card that is anchored in no changed file, and nothing marked `FIRE` quietly vanishes from it — report it, change the verdict, or write `-- not reported: `. ### Card contract The bracketed key joins the finding back to its `FIRE` line in `verdicts.txt` and to its surviving blocks in `refutations.txt` and `independent.txt`. A finding with no key cannot be cross-checked. ``` ## PR #NNN -- What it does: <one or two sentences: the wrong behaviour or missing capability, and the route taken. Written for someone who has not read the diff.> Scenario: <Q6's values and order, then what the head does under them. Fix PRs only.> Layer: <Q2's two frames: the one this diff changes -> the earliest that could have refused.> Blocking issues: <N | none> 1. [<RULE-ID|free:slug>] <one-line summary> [verified] Problem: <what is wrong> path/to/file.py:123 Impact: <what it costs at runtime> Action: <verb> <what to change> 2. ... deferred: <key> -- <reason> Checked: <paths and symbols read> | Ran: <commands> | Skill: <skill> | Model: <model> | Prior: <KEY status, ...> | Base: <merge-base sha> | Head: <head sha> ``` `Scenario:` and `Layer:` are written when `title.txt` carries a `fix` prefix or the body names a concrete failure — an error string, a session id, an RSI problem, `Fixes #N` — and only then; that is E2's trigger. "when the config is invalid" is not a scenario: if neither the body nor a surviving artifact pins it, write `Scenario: [unverified] -- <where you looked>`, E2's finding. At most 5 findings, ranked most-severe first by (severity, then blast radius). A sixth that survived both refutation passes is not dropped in silence — it goes on a `deferred:` line, which is the only thing that may stand in place of reporting it. A finding names one shape. Where the diff has it in several places, `Problem:` lists every `path:line` and `Action:` covers them all. `Prior` is written only on a re-review, as [`references/re-review.md`](references/re-review.md) says. Use `free:<slug>` as the key for a Step 6 free-form finding; it is held to the same two refutation passes as a rule finding. A clean review writes `Blocking issues: none` and no numbered findings — but the surviving-but-unreported scan still runs, so `none` cannot be used to bury a `FIRE` that survived refutation. ### Evidence threshold - `[verified]` — you opened the code, followed the chain, and can name the input, configuration or phase that triggers it; for an E1 or E2 evidence gap, naming where you looked — body, artifacts, commits — and what was not there is what verifies it. - `[inferred]` — plausible, not confirmed. An inferred finding is not blocking: verify it or delete it. `pr-review-publish.sh` refuses a card whose blocking findings are not all `[verified]`. A finding with no `path:line` does not go on the card, and one that stops at "likely the root cause" with no chain is not shippable. ### CI failure classification Before quoting `ci.txt` or `meta.txt`'s `mergeable`, read [`references/ci.md`](references/ci.md). ### SKIPPED protocol When a step genuinely cannot run — no network for `openprs.txt`, no second reader for Step 7, a diff too large to read — write `SKIPPED: <step> -- <why, and what is therefore unchecked>` into that step's artifact and carry the same line onto the card. A `SKIPPED` artifact means that axis was **not checked**; silence there is never read as clean. Skipping a step without recording it is itself a review defect. ## Publishing Post the conclusion to the PR in English. No Chinese, no emoji, no signature. ```bash # blocking issues gh pr review <n> --request-changes --body "<conclusion>" # no blocking issues: first dismiss your own earlier CHANGES_REQUESTED review, then gh pr comment <n> --body "<conclusion>"$'\n\nLGTM' ``` Never approve and never merge: both belong to the human reviewer the author brings in after LGTM. The clean comment still carries the `Checked:` / `Ran:` line so the author sees the depth of the review rather than a bare LGTM; it has two parts and the `LGTM` line, and stops there. ## Verifying a sub-agent's finding A finding produced by a sub-agent is a hypothesis. Before publishing it, do all four: 1. Open the cited code and read it. Confirm the described logic is what is there. 2. Reproduce with the smallest possible run — call the changed function directly, or run the single test that covers it. Never the full local suite; CI runs that. 3. Diff against `base.txt` to confirm this PR introduced it. 4. Judge whether the trigger is reachable in a real configuration, not only in a constructed one. A finding that fails any of these is deleted, not downgraded. Report one verified finding rather than five unconfirmed ones. Nothing in the output says "a sub-agent found" — you own every line. **Nothing new goes in this file.** SKILL.md is budgeted at 365 lines and holds only what every review needs. Past that length a skill stops being read in full, and an unread instruction is worth less than no instruction because it looks like coverage. Conditional content goes in `rules.md` and reaches the reviewer through the index; lookup tables go in `references/` and are opened at the step that needs them; executable content goes in a script and gets called. Raising the budget is a decision to make this file less likely to be read — make it in a commit that says why.