docs(delivery),chore(skills): two blind reviews before every PR — rule 14 and the review skill

.claude/skills/deliver/SKILL.md

@@ -5,9 +5,9 @@ description: Claim and implement a ready issue (task:ready, bug:ready, or featur
 
 # Deliver a ready issue
 
-Rules 1, 2, 3 and 9 in `docs/internal/agent-rules/delivery.md` govern this skill: act
-only on ready labels, the assignee is the claim, build from the body, done is
-defined per kind.
+Rules 1, 2, 3, 9 and 14 in `docs/internal/agent-rules/delivery.md` govern this
+skill: act only on ready labels, the assignee is the claim, build from the
+body, done is defined per kind, and the PR is reviewed before it is opened.
 
 Argument: an issue number. Without one, take the oldest ready issue, tasks
 before bugs before features:
@@ -89,12 +89,21 @@ text, error messages, HTTP error bodies) for claims your change makes
 false, and fix them in the same PR. A behaviour that changed while its
 description stayed put is a bug you shipped.
 
-Run `pnpm check` before opening the PR.
+Run `pnpm check` before moving on.
 
 If something in the spec turns out to be wrong or impossible, do not work
 around it: push what you have, leave a handoff (step 6), and stop. The
 maintainer reopens a spec session.
 
+## 4a. Review
+
+Run the `review` skill on the branch (rule 14). It spawns two reviewers
+that have not seen this session, one holding the spec and one holding the
+rules, and it ends in one of two states: fixes on the branch plus a
+`## Review` section for the PR body, or a blocking finding that survived two
+rounds, in which case it tells you to stop and hand off (step 6). Do not
+open a PR from the second state.
+
 ## 5. Open the PR
 
 If a person is present in this session, show the text first and wait for a
@@ -110,10 +119,11 @@ Beyond that:
   the last one.
 - **bug**: name the regression test; it is the triage test, now passing.
 - **feature**: walk every Completion condition and say how each was checked.
+- **all kinds**: the `## Review` section from step 4a, after the checklist.
 
-Rule 12 applies to the PR body: 200 words plus the checklist, what changed
-and why, no narration of how you got there, and `*Written by an agent.*` as
-the last line.
+Rule 12 applies to the PR body: 200 words plus the checklist and the Review
+section, what changed and why, no narration of how you got there, and
+`*Written by an agent.*` as the last line.
 
 ```bash
 gh pr create --title "<type>(<scope>): <summary>" --body-file <file>
@@ -124,7 +134,8 @@ Leave the issue assigned and labelled as it is. Merge closes it.
 ## 6. Stopping early
 
 If you stop for any reason before the PR is merged — blocked, out of
-context, told to stop, spec turned out wrong — push the branch, then leave
+context, told to stop, spec turned out wrong, a blocking review finding
+open after two rounds — push the branch, then leave
 exactly one comment and release the claim. If a person is present in this session, show the text first and wait for a yes before posting. Running unattended, post directly.
 
 ```markdown

.claude/skills/review/SKILL.md

@@ -0,0 +1,158 @@
+---
+name: review
+description: Run the two pre-PR reviews from delivery rule 14 on a branch or an open PR — a spec review blind to the rules and a code review blind to the issue, each by a fresh sub-agent — then verify every finding, fix or reject each, and write the Review section for the PR body. Use when the user says "review #N", "review this branch", "review PR N", or from the deliver skill before opening a PR.
+---
+
+# Review a change before its PR
+
+Rule 14 in `docs/internal/agent-rules/delivery.md` governs this skill: two
+reviews, each blind to the other and to you; every finding verified, then
+fixed or rejected with a reason; two rounds at most. Rule 12 governs
+everything you write.
+
+Argument: an issue number, a PR number, or nothing. With nothing, review the
+current branch; the issue is the second segment of its `<kind>/<n>` name.
+With a PR number, the branch is the PR's head and the issue is the one its
+body closes. If the PR is not yours, do not push to it: run steps 1 to 4 and
+post the confirmed findings as one comment (step 6).
+
+## 1. Gather the inputs
+
+Every input goes into a directory the reviewers read from, so what they see
+is exactly what you put there and nothing else.
+
+```bash
+git fetch origin main
+R=$(mktemp -d)
+git diff origin/main...HEAD > "$R/diff.patch"
+gh issue view <N> --json body -q .body > "$R/issue.md"
+```
+
+For a task, add the parent's body as `$R/feature.md`. Copy every ADR listed
+under Decisions and every file listed under Rules in play into `$R/spec/`.
+For a bug, add the triage report as `$R/triage.md`; the Simplest fix section
+is the agreed approach. Copy all of `docs/internal/agent-rules/` and
+`docs/internal/adr/README.md` into `$R/rules/`.
+
+Do not add the PR body, your commit messages, your handoff draft, or any
+note from this session. The reviewers must not know what you believe the
+diff does.
+
+## 2. Spawn the spec review
+
+A fresh sub-agent on the most capable model available, never a smaller one
+chosen for speed. Read-only: it reads `$R/diff.patch`, `$R/issue.md`,
+`$R/feature.md`, `$R/triage.md` and `$R/spec/`, and nothing under
+`$R/rules/`. Give it this brief, verbatim, with the paths filled in:
+
+```markdown
+You are reviewing a diff against a specification. You have not seen the
+specification before and you have no other context. Read only the files
+named here; do not open the repository and do not run anything.
+
+Specification: <paths>. Diff: <path>.
+
+Answer three questions, and only these:
+
+1. Is every line of Scope and Done when (for a bug: the Simplest fix; for a
+   feature: every Completion condition) delivered by the diff? For each
+   line, name the hunk that delivers it or say "not delivered".
+2. Does the diff do anything the specification did not ask for? Name it.
+3. For every test the diff adds or changes: does the title state a claim
+   the specification made, and does the body assert that claim? A title
+   that promises more than the body proves is a defect.
+
+Report one finding per defect, in this form and no other:
+
+- [blocking|note] <claim in one sentence>. Evidence: `<file>:<line>`.
+
+Blocking means the PR should not merge as is. Note means a reviewer should
+know. Do not suggest fixes. Do not praise. If there are no findings, say
+"No findings." and stop.
+```
+
+## 3. Spawn the code review
+
+In parallel with step 2. A fresh sub-agent on the same class of model, in
+its own worktree of the branch under review, so it can run and break things
+without touching yours. It reads `$R/diff.patch` and `$R/rules/`, and
+nothing else under `$R`. Give it this brief, verbatim:
+
+```markdown
+You are reviewing a diff for correctness and for conformance to the rules
+of this repository. You have no other context and you have not seen the
+issue this diff implements; judge the code, not the intent.
+
+Rules: <path to $R/rules/>. Diff: <path>. Repository: your working
+directory, checked out at the reviewed commit. You may run `pnpm check`,
+`pnpm test`, and any command that helps you answer; you may edit code to
+see what the suite catches, as long as `git checkout .` restores it before
+you report.
+
+Answer two questions, in this order:
+
+1. For every function the diff adds or changes: what input, state, error
+   path, or interleaving makes it return the wrong thing or leave the
+   wrong state? Where a test exists for it, break the code it covers and
+   run the test; if the test stays green, that is a finding. Where you
+   suspect a changed path is unreached by any test, delete it and run the
+   suite; if nothing goes red, that is a finding.
+2. Does the diff break any rule in the rules directory? Cite the file and
+   the rule number.
+
+Report one finding per defect, in this form and no other:
+
+- [blocking|note] <claim in one sentence>. Evidence: `<file>:<line>`, or a
+  fenced block with the command you ran and its output.
+
+Blocking means the PR should not merge as is. Note means a reviewer should
+know. Do not suggest fixes. Do not praise. If there are no findings, say
+"No findings." and stop.
+```
+
+## 4. Verify every finding
+
+A finding is a claim, not a fact. For each one, reproduce its evidence
+yourself: read the cited lines, or run the cited command. Then decide:
+
+- **Confirmed**: fix it on the branch. Do not note it anywhere; the diff is
+  the record.
+- **Rejected**: write one line for the PR body, `<claim> — <why it is
+wrong>`, in plain words.
+
+Two findings that disagree with each other, one from each review, usually
+mean the spec is missing a line. Resolve it in favour of the rules, reject
+the other with that reason, and say "spec needs: ..." in the PR body.
+
+## 5. Second round
+
+If any confirmed finding changed the diff, regenerate `$R/diff.patch` and
+run again only the review whose findings you fixed, with a fresh sub-agent.
+Verify as in step 4. That is the last round.
+
+A blocking finding still open after the second round, one you could neither
+fix nor reject with a reason, means the change is contested. Do not open
+the PR. Push the branch, hand off with the finding under Findings (deliver
+skill, step 6), and stop.
+
+## 6. Write the Review section
+
+Into the PR body, after the checklist:
+
+```markdown
+## Review
+
+Spec review: <n> findings, <m> fixed. Code review: <n> findings, <m> fixed.
+
+Rejected:
+
+- <claim> — <reason>
+```
+
+Omit "Rejected:" when nothing was. This section is outside the 200-word
+budget, like the checklist, and it still obeys rule 12: one line per
+rejected finding, no narration of what was fixed.
+
+On a PR that is not yours, post the same text as one comment instead, with
+the confirmed findings listed under "Confirmed:" since nobody has fixed
+them, ending with `*Written by an agent.*`.

AGENTS.md

@@ -29,7 +29,8 @@ changes in this repo:
   spec and queue: one `<kind>:<state>` label per issue, agents act only on
   `*:ready` and `bug:triage`, the body is the spec and comments are
   discussion, reporters' issues are never rewritten, branches are
-  `<kind>/<n>`, handoffs are one `## Handoff` comment per stop.
+  `<kind>/<n>`, handoffs are one `## Handoff` comment per stop, every PR
+  gets a spec review and a code review before it opens.
 
 So are the accepted records in [docs/internal/adr/](docs/internal/adr/). An ADR marked
 _Accepted — not yet implemented_ means the documentation already describes the

docs/internal/DELIVERY.md

@@ -56,8 +56,9 @@ request, closed — not from a label. The branch for issue `<n>` is always
    code, the message, the field. To reject the shape but keep the
    reproduction, reply and re-add `bug:triage`.
 6. An agent running `deliver` claims the `bug:ready` issue, creates `bug/<n>`
-   from the repro branch, and opens a PR that closes the issue. The triage test is now
-   the regression test. Merge closes the bug.
+   from the repro branch, has the change reviewed (below), and opens a PR
+   that closes the issue. The triage test is now the regression test. Merge
+   closes the bug.
 
 ## A feature, delivered as one PR
 
@@ -83,9 +84,9 @@ request, closed — not from a label. The branch for issue `<n>` is always
 5. Once Open questions is empty and every linked ADR is accepted, the
    maintainer adds `feature:ready`. The ADRs move to _Accepted — not yet
    implemented_.
-6. An agent running `deliver` claims it, works on `feature/<n>`, and opens a
-   PR whose body walks every completion condition. Merge closes the feature, and its ADRs flip to
-   _Accepted_.
+6. An agent running `deliver` claims it, works on `feature/<n>`, has the
+   change reviewed (below), and opens a PR whose body walks every completion
+   condition. Merge closes the feature, and its ADRs flip to _Accepted_.
 
 ## A feature, split into tasks
 
@@ -101,15 +102,41 @@ of a Technical spec section on the feature, it produces sub-issues.
    time. From then on the automation promotes a task to `task:ready` the
    moment its box is ticked, its Technical spec has content, and every issue
    under Depends on is closed. Nobody re-reads the dependency graph by hand.
-7. Agents claim `task:ready` issues one PR each, on `task/<n>`. As tasks close, the ones they
-   unblocked become ready on their own.
+7. Agents claim `task:ready` issues one PR each, on `task/<n>`, each PR
+   reviewed before it opens (below). As tasks close, the ones they unblocked
+   become ready on their own.
 8. Verification is part of delivery. Every task PR walks its Done when, and
    the PR that closes the last open sub-issue also walks the feature's
    Completion conditions. When that last sub-issue closes, the automation
    comments on the feature, and the maintainer closes it. If the feature
    came from a request, the request closes on its own with a pointer to the
    feature.
 
+## Review before the PR
+
+A PR arrives reviewed; it is not reviewed on arrival. Before opening one,
+the delivering agent runs the `review` skill, which spawns two reviewers on
+the most capable model available. Neither has seen the delivering session,
+and neither sees what the other sees. The spec reviewer gets the issue, its
+parent, its ADRs and the diff, and answers whether every line of the spec
+is delivered, whether the diff does anything the spec did not ask for, and
+whether each test proves the claim in its title. The code reviewer gets the
+agent rules, the ADR index and the diff, never the issue, and answers what
+input or interleaving makes each changed function wrong and whether a rule
+is broken; it works in its own worktree and may break code to see what the
+suite catches. The two are blind to each other on purpose: a reviewer
+holding both the spec and the rules resolves a conflict between them
+silently, and the maintainer wants to see that conflict, because it usually
+means the spec is missing a line.
+
+Findings are claims. The agent verifies each against the code, fixes what
+it confirms, and lists what it rejects in the PR body under `## Review`, one
+line each with the reason. A confirmed fix re-runs the review that raised
+it, once. A blocking finding still open after that is a contested change:
+the agent hands off with it instead of opening the PR. The same two reviews
+run on a person's PR when the maintainer asks; there the agent posts the
+findings as a comment and pushes nothing.
+
 ## Handoffs between agents
 
 An agent that stops before its PR is merged — out of context, blocked, or
@@ -144,9 +171,9 @@ issue if needed; a silent `bug:needs-info` closes after two weeks; a feature
 whose last sub-issue closed gets a note to close it; a completed feature
 closes its request; a PR closed without merging or a claim silent for three
 days releases the claim; a PR from a `<kind>/<n>` branch must close `#<n>`
-and nothing else. The repo's
-skills — `spec-session`, `triage-bug`, `deliver` — handle the transitions an
-agent makes as part of its own procedure.
+and nothing else. The repo's skills — `spec-session`, `triage-bug`,
+`deliver`, `review` — handle the transitions an agent makes as part of its
+own procedure, and the reviews are the delivering agent's job, not CI's.
 
 Four transitions are judgments and stay manual on purpose: `bug:new` to
 `bug:triage`, `bug:triage` to `bug:ready`, `feature:spec` to
@@ -171,4 +198,4 @@ when it closes. See [adr/README.md](adr/README.md).
 - Labels: `.github/labels.json`, synced by `.github/workflows/labels.yml`
 - Automation: `.github/workflows/issue-state.yml`
 - Skills: `.claude/skills/spec-session`, `.claude/skills/triage-bug`,
-  `.claude/skills/deliver`
+  `.claude/skills/deliver`, `.claude/skills/review`

docs/internal/agent-rules/delivery.md

@@ -158,8 +158,8 @@ is open, done means closed as completed.
     narration of what the agent did or considered. Evidence goes in a code
     block or a link, never in prose. Cut anything that does not change the
     reader's next decision. Budgets, counted outside code blocks: a triage
-    report 300 words, a handoff 150, a PR body 200 plus its checklist, a
-    "Spec updated" comment one line. Text over budget is cut before it is
+    report 300 words, a handoff 150, a PR body 200 plus its checklist and
+    its Review section, a "Spec updated" comment one line. Text over budget is cut before it is
     posted, not excused after. Do not restate the issue body: confirm or
     correct what it says, then add only what is new. Every comment, issue
     body, and PR body an agent writes ends with the line
@@ -175,6 +175,39 @@ is open, done means closed as completed.
     `origin/<branch>` rather than switching to it. Nothing depends on which
     worktree a branch was made in.
 
+14. **A PR is opened only after two reviews, and every finding is answered.**
+    Review is part of delivery in the same way verification is (rule 9): the
+    PR arrives reviewed, it is not reviewed on arrival. Before opening a PR,
+    the delivering agent runs two reviews of the diff against `main`, each
+    by a fresh sub-agent on the most capable model available (never a
+    smaller one chosen for speed), and each blind to the delivering session
+    and to the other reviewer.
+    The *spec review* gets the issue body, its parent feature, the ADRs
+    under Decisions, the files under Rules in play, and the diff — nothing
+    else, and never the PR body. It answers: is every line of Scope and Done
+    when delivered, does the diff do anything the spec did not ask for, and
+    does every test title state a claim the spec made. It reads; it does not
+    run anything.
+    The *code review* gets every file under this directory, the ADR index,
+    and the diff — never the issue. It answers, in this order: for each
+    changed function, what input, state, or interleaving makes it wrong; and
+    does the diff break a rule in this directory. It works in its own
+    worktree and may run the suite, delete a changed path, or break a branch
+    to see what stays green (testing rules 2 and 3), restoring the tree
+    afterwards.
+    Each review returns findings, one per defect: a claim, the evidence as
+    `file:line` or a command and its output, and *blocking* or *note*. A
+    finding is a claim, not a fact: the delivering agent verifies each one
+    against the code before acting. A confirmed finding is fixed and the
+    review that raised it runs again on the new diff; a rejected finding is
+    listed in the PR body under `## Review`, one line each with the reason,
+    so the maintainer sees what was overruled. Accepted findings are not
+    narrated. Two rounds at most: a blocking finding the agent could neither
+    fix nor reject after the second round means it stops and hands off with
+    the finding under Findings (rule 2), rather than opening a PR it knows is
+    contested. A PR from a person gets the same two reviews when the
+    maintainer asks for them.
+
 ## Procedures
 
 **Claiming work.** Find it, claim it, branch, and finish with a PR that
@@ -202,8 +235,8 @@ command, so the one-label invariant never breaks in between:
 gh issue edit <n> --add-label bug:needs-info --remove-label bug:triage
 ```
 
-The repo's own skills (`spec-session`, `triage-bug`, `deliver`) encode these
-procedures; use them rather than retyping the steps.
+The repo's own skills (`spec-session`, `triage-bug`, `deliver`, `review`)
+encode these procedures; use them rather than retyping the steps.
 
 ## Automation