diff --git a/.claude/skills/deliver/SKILL.md b/.claude/skills/deliver/SKILL.md index b3b2919..7b1bdb5 100644 --- a/.claude/skills/deliver/SKILL.md +++ b/.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 "(): " --body-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 diff --git a/.claude/skills/review/SKILL.md b/.claude/skills/review/SKILL.md new file mode 100644 index 0000000..356e668 --- /dev/null +++ b/.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 `/` 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 --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: . Diff: . + +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] . Evidence: `:`. + +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: . Diff: . 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] . Evidence: `:`, 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, ` — `, 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: findings, fixed. Code review: findings, fixed. + +Rejected: + +- — +``` + +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.*`. diff --git a/AGENTS.md b/AGENTS.md index 7eeea3b..01e2707 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,7 +29,8 @@ changes in this repo: spec and queue: one `:` 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 - `/`, handoffs are one `## Handoff` comment per stop. + `/`, 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 diff --git a/docs/internal/DELIVERY.md b/docs/internal/DELIVERY.md index ef009f3..0cfb60d 100644 --- a/docs/internal/DELIVERY.md +++ b/docs/internal/DELIVERY.md @@ -56,8 +56,9 @@ request, closed — not from a label. The branch for issue `` 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/` - 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 `` 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/`, 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/`, 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,8 +102,9 @@ 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/`. As tasks close, the ones they - unblocked become ready on their own. +7. Agents claim `task:ready` issues one PR each, on `task/`, 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 @@ -110,6 +112,31 @@ of a Technical spec section on the feature, it produces sub-issues. 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 `/` branch must close `#` -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` diff --git a/docs/internal/agent-rules/delivery.md b/docs/internal/agent-rules/delivery.md index 090dc75..97318db 100644 --- a/docs/internal/agent-rules/delivery.md +++ b/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/` 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 --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