git:20261028.d5f1bde to git:20261029.8d68eef

+ 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, reads only the rules.md rules the diff can trigger, forces a semantic and core-file risk assessment before any finding is written, refutes every finding twice, and publishes an English card of two parts only — what the PR does, and its blocking issues — plus the PR's risk, the decisions a person must make, and the code the author can delete in the same PR."

+ An agent executes this top to bottom. Every step writes an artifact into `$WORK` (default `/tmp/hl-review-<PR>`;

+ keep the one `fetch.sh` prints): a step that leaves nothing behind can be skipped unseen.

+ The card has two parts, what the PR does and its blocking issues; beside them stand only the PR's risk, the decisions

+ a person must make (Step 3, Step 8) and Step 6's deletions — none of them a finding. Nothing else. Non-blocking

+ observations are deleted, not softened into a "note", "FYI", "non-blocking but", "for reference" or a postscript

+ under the conclusion: one that survives only by relabelling fails the bar. 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 kinds, equal in weight:

+ | Desync | an operator-observable change the description never states; a title or description that does not match the diff at the current head |

+ | Contract violation | the diff breaks a rule `AGENTS.md` states outright; the finding names the bullet and, for an ownership bullet, 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; an E finding that names none of them is taste and goes.

+ The fourth kind is narrow. "Could be simpler", "I would put it elsewhere", "extract a helper" are taste and go. It

+ survives only where the repo has already decided — a detour around an existing path, a concern implemented outside

+ its owner — in writing both sides can read; a finding that cannot name the bullet and the owner is taste wearing a

+ citation. With no blocking issue, say so explicitly; never manufacture small ones.

+ Read and cite, never restate, the authoring contract: `AGENTS.md`, `.github/copilot-instructions.md` (with its

+ do-not-flag list), `docs/contributing/style-guide.md`, `.github/PULL_REQUEST_TEMPLATE.md`. Whatever ruff, pylint,

+ mypy, CodeQL, gitleaks, bandit, REUSE, pre-commit or the coverage floor gates is out of scope: re-flagging it is a

+ review defect.

+ Read `diff.txt`, `body.txt` and `comments.txt` first. 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,

+ as [`references/re-review.md`](references/re-review.md) says.

+ | file | for |

+ | `title.txt`, `body.txt` | the title and the description; X2 checks both against the diff |

+ | `diff.txt`, `files.txt` | the full diff against the merge base; its paths, one per line, the input to rule selection |

+ | `commits.txt` | 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 |

+ | `comments.txt` | review and issue comments, each headed by author and time; an earlier card is settled (re-review.md), any other point already made is not repeated |

+ | `testfiles.txt`, `docfiles.txt` | changed paths under a `tests/` directory; changed docs, prompts, `.md`/`.rst` — an empty `docfiles.txt` beside a `src/` change is X3's shape |

+ | `openprs.txt` | other open PRs touching the same files; a `# PARTIAL:` line names what was never compared, reported as `SKIPPED: V4` |

+ | `skill.txt` | this skill's version (its 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 |

+ | `risk.txt` | the PR's risk, graded from its changed paths by `risk.py` with `references/tiers.md` as its data, then one line per path; the floor of the card's `Risk:` |

+ Take every row of the index atop [`rules.md`](rules.md) whose trigger matches `files.txt`, a skim of `diff.txt`, or a

+ `label:` line in `meta.txt`; write the union of their ids into `$WORK/rules.txt`, one per line, and read only those

+ bodies. **Never read `rules.md` whole**: a reviewer told to attend to every rule 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 earlier findings that still hold.

+ Answer all six from the diff, not the description, one line each into `$WORK/answers.txt` as `Q1:` … `Q6:`. An answer that

+ anchors in no path, symbol or condition is rewritten before going on.

+ - **Q1 — Root cause in one sentence, and what pins it.** Not the symptom or 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 says

+ nobody found the cause.

+ - **Q2 — 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 an operator now observes.** A behaviour, interface, default, flag, artifact path, metric, log line.

+ "Nothing" needs its reason: the `AGENTS.md` exemption rests on it.

+ - **Q4 — Every consumer of a changed value.** For each new or changed status, enum member, field, keyword or return

+ shape: membership tests, call sites, readers of the persisted document, prompts naming it. Grep, never guess.

+ - **Q5 — What would make it wrong.** The input, configuration, phase or race. "Nothing" is not an answer, nor is a

+ - **Q6 — The scenario, and whether it ends.** The input, configuration or sequence that produces the failure, in

+ actual values and 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". 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:

+ One line per changed non-test `src/` file, Tier 3 included — check the list against `files.txt` — into

+ `$WORK/core_files.txt`:

+ Tiers come from [`references/tiers.md`](references/tiers.md): its table, then its Q1–Q4 for any other file, new ones

+ included. Q1b bites hardest: a `Coordinator` attribute rename passes imports, lint and collection and fails hours into

+ a session — grep the class name, not just the symbol. A lower tier than it gives is no route past the real tier's

+ checks. `COVERED`: this PR's tests exercise the blast radius, or the change cannot reach it. `GAP`: they do not, and

+ it goes on the card. `N/A`: the change cannot reach it. A reason naming nothing this PR changed is worthless — "core

+ file, large blast radius" is true of every PR against it. A docs-, CI- or test-only diff writes one

+ The card's `Risk:` is `risk.txt`'s level, or higher when the diff does what paths cannot show — deletes or weakens a

+ test, changes a default, a persisted shape or a gate, widens what an agent may do — written `(raised from <level>)`.

+ Never lower: paths are the floor, and only a person changes what they mean (`references/tiers.md`).

+ One line per id in `rules.txt` into `$WORK/verdicts.txt`:

+ `CLEAR` claims you looked and it does not apply to this diff; the reason makes that checkable ("ok", "n/a", "fine"

+ are not reasons). Step 1b already cut the list, so no rule is skipped for length. A `FIRE` must cite a file this PR

+ changes; the contract's header, the prompt naming a flag, the doc that did not move are evidence beside it.

+ No line goes to a rule whose `Severity:` is advisory in every shape it names, nor to one whose body says it cannot be

+ a finding — V2 is the output contract and V7 is Q4, each recorded by the step that runs it. The card cannot publish

+ an advisory rule, so adjudicate one 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.

+ A clean description is cheap for an AI; these are where a diff that reads well hides defects. One line per check into

+ `$WORK/ai_diagnostic.txt` as `<check>: CLEAN|HIT|N/A -- <what you looked at and found>`; "clean" alone, or a reason

+ naming nothing in the diff, is no answer.

+ 1. `claims` — **claim against code.** Does the code enforce what the description, docstring, comment and prompt

+ assert? Every number, flag, path and default the description names must be greppable in the head tree; a number

+ traced to no output is `[unverified]` and never repeated as fact. A command or 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: one naming the

+ 2. `silent-failure` — **safety theater.** For each new guard, `try`, `except` or default: is it reachable, does it

+ ever fire, does it turn a failed measurement, patch apply or artifact write into a plausible value? A fail-closed

+ gate that coalesces a missing input to a passing default is the blocking shape.

+ 3. `test-falsifies` — **calibrated to pass, not to falsify, or not needed.** Does the test drive the production entry

+ point or a mock, assert behaviour or restate the implementation, fail on the merge base? A regression test passing

+ on base pins nothing. A test that repeats another, pins plumbing, or re-pins a contract at each layer below it is a

+ deletion ([`references/subtract.md`](references/subtract.md)).

+ 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 block 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 permissive branch that produced the symptom survives unchanged, with the new branch added above it —

+ narrowing a default is not changing it (#2099);

+ - 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 build-it-again

+ 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 another measurement system's? Most

+ findings come from this step; the rule list prompts it and does not replace it.

+ Then append to `$WORK/answers.txt`:

+ - `BUILD:` **"If none of this code existed, what would you build — and does every mechanism this diff adds or patches

+ need to exist?"** Name it, then the diff's parts it lacks. A part surviving only because it is written is what this

+ catches; on a fix, so is a guard added where deleting or changing existing code removes the cause. A refactor

+ settles it under E3 on the net line count; anything else carries an `AGENTS.md` bullet and an owner module, or

+ stops here.

+ - `TRIM:` lines, as [`references/subtract.md`](references/subtract.md) says.

+

+ Try to kill each candidate before it reaches the card, one block per finding in `$WORK/refutations.txt`:

+ The key is the card's; every `FIRE` needs a block; an `ATTEMPT` naming nothing openable cannot be repeated and is no

+ refutation. With nothing to refute write a single `FINDING: none -- <reason>` line: an empty file reads as a skipped

+ Attack in order: is the line added by this PR, or context around one (against `base.txt`)? Does the symbol resolve

+ where the diff did not show? Is the trigger reachable in a real configuration? Has `comments.txt` already made the

+ point? An earlier review-pr card does not count: its findings are settled in `prior_verdicts.txt`, kept if they still

+ hold and dropped if 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 blocks for each survivor. With no such reader write `FINDING: none -- no independent reader available` and say

+ so on the card.

+ Before writing the card, walk `verdicts.txt` both ways: nothing reaches the card anchored in no changed file, and no

+ `FIRE` vanishes from it — report it, change the verdict, or write `-- not reported: <reason>`.

+ What it does: <one or two sentences for someone who has not read the diff: the wrong behaviour or missing

+ capability, and the route taken>

+ Risk: <low|medium|high>[ (raised from <level>)] -- <the path or change that sets it>

+ Decisions for a human:

+ - <paths> -- <what changes for an operator or a gate> -- <the question a person answers>

+

+ Trim in this PR:

+ - path/to/other.py:40-58 -- <what to delete> -- <evidence>

+ - `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.

+ - `Decisions for a human:` is what a person must accept before merge: every operator-observable change from Q3, and

+ every `high` path in `risk.txt`, named whole or under a directory written with its trailing `/`. A medium card with

+ nothing to decide writes `Decisions for a human: none -- <why>`; a high card names at least one; a low card has

+ none, since anything a person must decide makes it medium. Decisions never count toward `Blocking issues`.

+ - The key joins a finding to its `FIRE` in `verdicts.txt` and its blocks in `refutations.txt` and `independent.txt`;

+ a finding with no key cannot be cross-checked. A Step 6 free-form finding is `free:<slug>`, held to the same two passes.

+ - At most 5 findings, most severe first (severity, then blast radius). A sixth that survived both refutation passes

+ goes on a `deferred:` line, the only thing that may stand in for reporting it.

+ - A finding names one shape: `Problem:` lists every `path:line` where it occurs, and `Action:` covers them all,

+ preferring to delete or change existing code over adding any.

+ - A clean card is `Blocking issues: none` with no numbered findings; the unreported-`FIRE` walk still runs, so `none`

+ cannot bury a `FIRE` that survived refutation.

+ - `Prior` is written only on a re-review ([`references/re-review.md`](references/re-review.md)).

+ - `[verified]`: you opened the code, followed the chain, and can name the triggering input, configuration or phase;

+ for an E1 or E2 evidence gap, naming where you looked — body, artifacts, commits — and what was not there verifies

+ it. `[inferred]` — plausible, not confirmed — is not blocking: verify it or delete it; `check-card.sh`

+ refuses a card whose blocking findings are not all `[verified]`. No `path:line`, or a chain that stops at "likely

+ the root cause", keeps a finding off the card.

+ - Before quoting `ci.txt` or `meta.txt`'s `mergeable`, read [`references/ci.md`](references/ci.md).

+ Last, run `bash .claude/skills/review-pr/check-card.sh "$WORK" <PR> <head sha> "$WORK/risk.txt"` and fix the card

+ until it prints nothing. CI publishes only a card that passes the same check against a risk it grades from GitHub.

+ A step that genuinely cannot run — no network for `openprs.txt`, no second reader, a diff too large to read — writes

+ `SKIPPED: <step> -- <why, and what is therefore unchecked>` into its artifact and onto the card. `SKIPPED` means not

+ checked; silence is never read as clean, and an unrecorded skip is itself a review defect.

+ Post `card.md` as `check-card.sh` passed it, in English: no Chinese, no emoji, no signature.

+ gh pr review <n> --request-changes --body-file "$WORK/card.md"

+ gh pr comment <n> --body "$(cat "$WORK/card.md")"$'\n\nLGTM'

+ Never `gh pr review --approve`, never merge. A local run posts under your account, so an approval would be your

+ code-owner approval of a model's verdict; both belong to the human reviewer the author brings in after LGTM. The bot

+ never approves either, whatever the risk: `Risk:` and `Decisions for a human:` show that reviewer where to look. The

+ clean comment is the whole card, `Checked:` line included so the author sees its depth, then `LGTM`.

+ A sub-agent's finding is a hypothesis. Before publishing: (1) open the cited code and confirm the logic is there;

+ (2) reproduce with the smallest run — call the changed function directly, or the one test covering it, never the

+ full local suite (CI runs that); (3) diff against `base.txt` to confirm this PR introduced it; (4) confirm the

+ trigger is reachable in a real configuration, not only a constructed one. Failing any deletes it, not downgrades it.

+ One verified finding beats five unconfirmed; nothing says "a sub-agent found" — you own every line.

+ **Nothing new goes in this file.** It is budgeted at 365 lines and holds only what every review needs: past that a

+ skill stops being read in full, and an unread instruction is worth less than none because it looks like coverage.

+ Conditional content goes in `rules.md` behind the index, lookup tables in `references/` opened at the step that needs

+ them, executable content in a script that gets called. Raising the budget makes this file less likely to be read —

+ do it in a commit that says why.