review-rounds
Run one review round of CLAUDE.md on this branch
SKILL.md.claude/skills/review-rounds/SKILL.md
GitHub!target/debug/xtask review start $0 $1
The lines above are the output of xtask review start. This skill runs one review round of CLAUDE.md on this branch. Do the steps below in order.
Arguments
- The base is
$0. Thebase:line above gives its merge base withHEAD. - The round is $1.
- The criteria are: $2
- The flags are in the argument string
$ARGUMENTS. A flag is given only if it is a word of that string:--review-only: stop after step 9.--baseline: also run thebaselineagent (steps 2, 5 and 7).
Before the steps
- Run this skill only after
cargo run -p xtask -- check --tier pushandcargo build -p xtaskpass. - A refusal of
review start(exit 3) stops the skill before this text. Claude Code shows its lines, such as "commit first", in the session. Aclaude -prun in text mode shows nothing. - A
warning:line ofreview startdoes not stop the round. Copy it to the findings file (step 9). allowed-toolslets thextask reviewcommands run with no approval only in the turn in which the skill loads. A new turn ends that permission. A message of a person starts a new turn. So does the notice that a background agent or command ended.- The notices of the background reviewers and of the mutants run start new turns. So in the default permission mode,
review checkof step 8 asks for approval once in each round. A latereview mutantscan also ask for approval.review log-checkandreview cleanof step 11 also ask once in each round. They run after the findings file, so the counts of the round do not include them. Approve each one once. Do not approve with "don't ask again". That changes the local settings file of Claude Code, and thenreview checkfails the round. - In an agent-run phase, "the person" in this skill is the orchestrator.
- If you are a subagent, do not give your final answer before the last step that you do. Your run ends with your final answer, and a nested agent or a background command then has no reader.
- Do not pass
modeloreffortto the Agent tool. The agent file decides them (decision 0025). - If the Write tool refuses a file of this skill to a subagent, do not write it in another way. Give the path and the whole text of each such file in your final answer. Also say at which step you stopped.
- In that case, keep the text of each refused file, and go on. Do the fixes and the commit of step 10. Do step 11 up to the log, and write the log's text. Leave the approvals fields as step 9 wrote them. If you can write a scratch file, run
target/debug/xtask review log-checkon a copy of the log there. Then stop. With--review-only, stop after step 9. - The caller writes each file at its path. Also give the command
target/debug/xtask review clean $1in your final answer. The caller runs it after it writes the files. If the caller writes the log, it runstarget/debug/xtask review log-check <run>/log.mdbeforereview clean. Then the caller decides the next step: the next round, or the steps after the last round.
Names
Take each value from the lines of review start. Do not make a path from the branch name, because review start writes / in a branch name as -.
<run>: the absolute path on therun folder:line.<target>: the absolute path on thetarget folder:line.<base>: the full SHA on thebase:line.<since>: the full SHA on thesince:line.- The branch route: the reviewer lines below the line
branch <base>..<head>:. - The since route: the reviewer lines below the line
since <since>..<head>:. The line "no changes" means that the since range is empty. - The reasons of a reviewer: the text after its name and the colon on its line of a route, in
<run>/$1/route.txt. That file holds the lines of the two routes with all their reasons. The lines above show at most 5 paths of a reviewer, then "(+n more)".
The round's folder is <run>/$1. review start wrote branch.diff, since.diff, route.txt and state.json there. The mutants run writes mutants.txt, missed.txt, known.txt and mutants.out/ there. Step 9 writes the findings file <run>/$1/findings.md. The log of the branch is <run>/log.md.
Steps
1. Start the mutants run
Run this command with the Bash tool, with run_in_background: true and timeout: 3900000:
target/debug/xtask review mutants $1
The tool result comes at once. The command stops itself after 60 minutes. When it ends, it prints one line, and writes the line to <run>/$1/mutants.txt. The notice that the command ended holds the line. Do not wait for it now. If the command refuses to run (exit 3), it prints an error line, no mutants: line, and no mutants.txt.
2. Start the baseline (--baseline only)
Start the baseline agent in the same message as the reviewers of step 3. Its range is <base>..HEAD. Fill in this prompt template, and give the agent only that text:
Review this commit range as your instructions say.
Range: <base>..HEAD
Criteria: $2
Round: $1
Target folder: <target>
3. Start the reviewers
The six reviewers are reviewer, gpu-reviewer, memory-reviewer, wire-reviewer, supply-chain-reviewer and docs-reviewer. Choose the reviewers and their ranges:
- Round 1: start each reviewer of the branch route. The range of each is
<base>..HEAD. Its reasons come from the branch route. - Rounds 2 and 3: start
revieweron<base>..HEAD, with its reasons from the branch route. Also start each other reviewer of the since route on<since>..HEAD, with its reasons from the since route. If the since range is empty, startrevieweronly. - From round 4:
- Start each reviewer of the since route on
<since>..HEAD,reviewerincluded. - Its reasons come from the since route.
- It reads the fix range and the files that it touches (decision 0024).
- If the since range is empty, start no reviewer. The round has no findings, so it is the last round.
- Start each reviewer of the since route on
Start each reviewer, and the baseline, in one message. Use the Agent tool, with subagent_type set to the name of the reviewer. Each one is a new subagent. Never start a fork, because a fork gets your context.
Fill in this prompt template for each reviewer, and give the reviewer only that text. Do not add your reasoning, or anything from an earlier round:
Review this commit range as your instructions say.
Range: <the range of the reviewer>
Criteria: $2
Reasons of the route: <the reasons of the reviewer>
Round: $1
Target folder: <target>
From round 4, put this line after the Range: line of the template:
Scope: read the diff of the range and the whole of each file that it touches. Do not review the rest of the branch.
A change to .claude/agents/ reaches the agents only in a new session. The Scope: line reaches them in any session.
4. Collect and merge the findings
Wait for each reviewer, and for the baseline with --baseline. A background agent gives its output in the notice that it ended. A foreground agent gives its output in its tool result. For each agent, record the total tokens and the duration of its <usage>.
Each finding of a reviewer is one block with the fields severity, kind, where, rule, claim, evidence and fix. "No findings" gives no block. Merge the blocks:
- Two blocks are the same finding when they have the same
whereand the same claim. The samewhereis the same file, at the same line or in the same function. The same claim is the same defect, in other words. - Merge each set of blocks that are the same finding into one finding. Its sources are each reviewer that reported it. Its severity is the highest severity of the blocks:
blocker, thenshould-fix, thennit. - Give each finding the ID
$1-<n>, with<n>from 1. - Do the
repeat-ofcheck:- Read
<run>/<r>/findings.mdfor each round<r>below $1. Find each finding there with the verdictrefuted. - Also read
<run>/log.md,docs/reviews/<branch>.mdand eachdocs/reviews/<branch>-m<n>.md.<branch>is the last part of the<run>path. - Find each row of these logs whose Action is a TD ID or
known: <TD>. Skip a TD thatdocs/tech-debt.mdlists under## Closed. - If a finding of this round is the same finding as a
refutedone, give itrepeat-of:and the ID of that finding. Give it the verdictrefuted. - If it is the same finding as a row with a TD, give it
repeat-of:and that TD. Give it the verdictknown. Its action in the log isknown: <TD>. - From round 4, also find each row of
<run>/log.mdwhose Action isreported, not fixed. It can be a row of any earlier round, a nit of rounds 1 to 3 included. Read no other log for these rows, since a row of another log can have the same ID. - A finding of this round that is the same finding as such a row is a reported repeat, unless one of these is true:
- its severity is higher than the row's;
- it names a strict rule that the row's Rule cell does not name;
- its sources include
cargo-mutants, and the row's Reviewers do not.
- Give a reported repeat
repeat-of:and the ID of that row. If more than one such row fits, use the row of the latest round. If that row's Repeat of holds an ID, use that ID. - A reported repeat that is a blocker or should-fix gets the row's verdict and Significant value. A nit keeps the verdict
unvalidatedand-. Its action in the log isreported, not fixed. - If one of the three is true, the finding is a new finding. Give it no
repeat-of. Steps 5 and 6 validate it as any finding. - A finding with
repeat-ofgets no validator.
- Read
- Keep the baseline's findings in a separate list. Merge them only with each other. Give each the ID
$1-b<n>, with<n>from 1. - If a baseline finding is the same finding as a reviewer's finding, give it
same-as:and the ID of the reviewer's finding.
5. Validate the findings
Start one validator for each of these:
- each
blockerandshould-fixof the reviewers, with norepeat-of; - each
blockerandshould-fixof the baseline, but not one withsame-asa finding that has a validator.
A baseline finding with same-as a finding that has a validator gets the verdict of that finding, from its validator. Each other nit gets the verdict unvalidated, with no validator.
Start the validators in one message, at most 15 at once. If there are more than 15, start them in batches of 15. Start the next batch when each validator of the last batch has ended. Use the Agent tool, with subagent_type set to validator.
The range of a finding is <base>..HEAD if a source of the finding reviewed that range, or the finding is the baseline's. Else it is <since>..HEAD. Fill in this prompt template for each finding, and give the validator only that text:
Validate this finding as your instructions say.
ID: <the ID of the finding>
Finding:
<the block of the finding: severity, kind, where, rule, claim, evidence and fix, as a source reported it>
Range: <the range of the finding>
Criteria: $2
Round: $1
Target folder: <target>
Each validator gives one block with the fields id, verdict, evidence and reason. Record the verdict of each finding, and its evidence in a few words. Record the tokens and the duration of each validator from its <usage>.
Wait for each validator before step 6. Step 6 takes its wait for the mutants run off the wall minutes, so that wait must not overlap a validator.
6. Validate the missed mutants
If the notice that the mutants command ended has come, the wait for it is 0 seconds. Else, run
jq -n now, and record its output as the start of the wait. Then wait for the notice. When it comes, runjq -n nowagain, and record the end of the wait.Read the mutants line in the notice. If the notice does not show it, read
<run>/$1/mutants.txt. Read that file only after the notice has come, because the run writes the file when it ends. The line starts withmutants: full:,mutants: partial:ormutants: none:. If the notice shows nomutants:line and the file does not exist, the command refused to run. Then record its error line as the mutants line, and go to item 9.Read
<run>/$1/missed.txtand<run>/$1/known.txt. If neither file exists, the run is from an olderxtask. Then read<run>/$1/mutants.out/missed.txtasmissed.txt. If both files are empty or missing, go to item 9.Each line of
missed.txtbecomes a finding with the next ID$1-<n>. Its source iscargo-mutants, its severityshould-fixand its kindweak-test. Itswhereis the file and line at the start of the line. Its rule isVERIFY-1, and its claim is "no test fails with this mutant", with the line. Make no finding of a line ofknown.txtin this item.Each line of
known.txtbecomes a finding with the next ID$1-<n>. Its fields are those of item 4, from the text before the tab. Its verdict isknown, and itsrepeat-ofis the TD after the tab. It gets no validator. Its action in the log is "known: <TD>".Do the
repeat-ofcheck of step 4, item 4, for each finding of item 4. A finding whoserepeat-ofis arefutedfinding gets the verdictrefuted. A finding whoserepeat-ofis a TD gets the verdictknown, and its action in the log is "known: <TD>". A missed mutant is a reported repeat only of a row whose Reviewers namecargo-mutants. It gets the row's verdict and Significant value, and its action in the log isreported, not fixed. None of these gets a validator. Leave its line out of the validator's prompt. If no line is left, go to item 8.Start one
validatorwith this prompt template, filled in. The prompt holds each line that has norepeat-of. The range is<since>..HEAD, because the mutants run tests the since diff:Validate these missed mutants as your instructions say. Give one block for each line, with the line as its id. Missed mutants, from cargo-mutants: <each line of missed.txt with no repeat-of> Range: <since>..HEAD Criteria: $2 Round: $1 Target folder: <target>The verdict of each of these findings is the verdict of the validator for its line.
If a finding of a reviewer and a finding of item 4 are the same finding, as step 4 says, merge the two. The merged finding keeps the reviewer's ID, and gets
cargo-mutantsin its sources. Its rule also names VERIFY-1. If its two verdicts differ, it keepsconfirmedorpre-existingover the others. If the reviewer's finding is a reported repeat of a row whose Reviewers do not namecargo-mutants, the merged finding is a new finding. It loses itsrepeat-ofand the values that it copied from the row. Its verdict is the verdict of the finding of item 4.Run
jq -n now, and record the output as the end of the round. Runjq .started <run>/$1/state.jsonfor the start of the round.
7. Compare with the baseline (--baseline only)
A finding is found when its verdict is confirmed or pre-existing. If the baseline has no found finding, skip this step. Record "of 0 baseline findings, the round found 0".
Else start one new subagent with subagent_type set to general-purpose. Fill in this prompt template, and give the subagent only that text. List A holds each found finding of the baseline. List B holds each found finding of the reviewers and of cargo-mutants, and each known finding. Give each finding as its ID, its where and its claim:
Compare two lists of review findings. Use only the two lists below. Do not read files, and do not run commands.
List A holds the found findings of a baseline review. List B holds the found findings and the `known` findings of a review round on the same commit.
For each finding in list A, find a finding in list B that reports the same defect at the same place. A finding in list B can match more than one finding in list A.
Give one block for each finding in list A:
id: the ID of the finding in list A
mark: found or missed
match: the ID of the matching finding in list B, and a quote of its claim; or none
reason: one sentence
List A:
<the found findings of the baseline: ID, where and claim of each>
List B:
<the found findings and each `known` finding of the round: ID, where and claim of each>
Record the mark of each baseline finding, with the match. Count x, the baseline findings marked found, and y, all found baseline findings. Record "of y baseline findings, the round found x", with the numbers.
8. Check the state of the repo
Run this command:
target/debug/xtask review check $1
- If it prints "no change", go to step 9.
- If it exits 3, stop. Do not write findings. Give the person each line of its output. Its lines name each change since
review start. - If the only change is to the local settings file of Claude Code, from an approval with "don't ask again", the person confirms that change. Then the person starts the round again.
9. Write the findings
Write <run>/$1/findings.md with the Write tool, in the form below. If the Write tool refuses it, do as "Before the steps" says.
# Review round $1
* mutants: <the mutants line, as the command printed it>
* base: <base>; since: <since>; criteria: <the criteria>
* flags: <the flags, or none>
* warnings: <each warning: line of review start, or none>
* times: started <started>, wait <seconds>, end <end>; wall minutes <(end - started - wait) / 60, to 1 decimal>
* interventions: <the number of messages of a person since review start, but not the message that gives the approval counts of step 11. An approval is not an intervention>
* late review commands: <the number of `target/debug/xtask review` commands that the skill ran or tried to run after the turn in which it loaded, until you write this file>
* approvals: <not asked. Step 11 writes the count that the person gives for the round>
* baseline approvals: <not asked. Step 11 writes the count that the person gives for the baseline. Without --baseline, write "-">
## Agents
| Agent | Range | Findings | Tokens | Minutes |
| - | - | - | - | - |
| reviewer | <base>..HEAD | <IDs of its findings, or no findings> | <tokens> | <minutes> |
| validator | <range> | verdicts for <IDs> | <tokens> | <minutes> |
## Findings
### <ID>
* sources: <each reviewer, cargo-mutants>
* severity: <blocker, should-fix or nit>
* significant: <yes or no; or ->
* kind: <bug, weak-test, rule, doc or other>
* where: <file:line>
* rule: <ID and quote, or none>
* claim: <the claim>
* verdict: <confirmed, pre-existing, refuted, unproven, unvalidated or known>
* repeat-of: <ID, TD, or none>
* evidence: <the evidence of the validator, in a few words>
## Baseline findings
### $1-b<n>
* (the same fields, with sources: baseline, and same-as: <ID, or none>)
* mark: <found or missed, with the match and its quote; or not compared>
## Baseline comparison
of <y> baseline findings, the round found <x>
- Fill the
significantfield yourself. A reviewer does not fill it. - A blocker or should-fix with the verdict
confirmed,pre-existingorunprovengetsyesorno, by the test of decision 0024. - The test: a rule or decision fails in a case that a user can reach, or later work goes wrong. A contradiction or a misleading measure is significant too.
- Each other finding gets
-. This includes eachknownfinding. - List each agent that the round started: the reviewers, the baseline, and each validator. A validator's Findings cell names the IDs that it judged.
- Without
--baseline, leave out the two baseline sections. - With
--review-only, runtarget/debug/xtask review clean $1. Then stop here. If auto mode or a person refuses it, give the command to the person. Give the person the path of the findings file and a summary: the mutants line, and the count of findings for each verdict.
10. Fix the findings
The fix rule is decision 0024:
- Rounds 1 to 3: fix each found finding, the baseline's included. A found finding has the verdict
confirmedorpre-existing. - From round 4, a round fixes blockers, significant should-fix and every confirmed finding on a strict rule.
- From round 4, log each other finding as
reported, not fixed, nits included. These are eachconfirmed,pre-existingorunprovenfinding of none of the three kinds, and eachunvalidatedfinding. Do not fix them. - From round 4, fix each
pre-existingfinding that is a blocker, hasyes, or is on a strict rule. Or send it to a TD or to its own PR. - From round 4, fix an
unprovenfinding only if it is one of those three kinds. - A significant should-fix has
yesin itssignificantfield. - The strict rules are PROTO-1, PROTO-2, MEM-1 to MEM-5, RT-1, GPU-1 to GPU-3, VERIFY-1, GIT-1, DEPS-1, ASSET-1.
- A finding is on a strict rule when its rule names one of them. Each finding of
cargo-mutantsis on VERIFY-1.
Then:
- Where the finding is about behaviour, first write a test that fails before the fix.
- A
pre-existingfinding can go todocs/tech-debt.md, or to its own PR, in place of a fix. - A mutant that the team sends to tech debt gets a
KNOWN_MISSEDentry in the milestone's last commit, not in a round's fixes. Reason: a change toxtask/src/review.rsroutes all six reviewers (ROUTE_ALL). - Do not fix a
refutedfinding. - Do not fix a
knownfinding. Its gap is in its TD. - Do not fix a reported repeat (step 4, item 4). Its action is
reported, not fixed. - Decide each
unprovenfinding, and record the reason. - A nit has the verdict
unvalidated. In rounds 1 to 3, fix it only if you agree with it. - Before the commit, run
cargo run -p xtask -- check --tier commit(GIT-2). - Commit the fixes, signed, with the subject "review round $1 fixes".
11. Add the round to the log
First, get the approval counts of the round. They are cost, not interventions. Use the first of the next three bullets that applies. Then do the two bullets after them, unless that bullet tells you to stop. Under the orchestrator bullet, use the first mode sub-bullet that applies, and always the last sub-bullet.
- If you are the orchestrator of an agent-run phase, you are "the person" (see "Before the steps"). Do not ask yourself.
- In a
claude -prun where nobody answers permission prompts, write each count as 0 with the mode, in any mode. For example, write0 (auto mode). A mode that asks for approval gets none in that run, so its count is 0 too, for example0 (default mode). Where a host or a prompt tool answers prompts (--permission-prompts host), write the approvals that it gave, if the run records them. Otherwise writenot countedwith the reason. - In
dontAskmode in an interactive session, write each count as0 (dontAsk mode). - In auto mode in an interactive session, the mode can fall back to a prompt that waits for a person. You do not see an approval of that prompt (TD-38). So write each count as
0 (auto mode; a fallback prompt is not counted). - In another mode in an interactive session, ask the maintainer the questions below.
- Without
--baseline, keep "-" in thebaseline approvalsfield. This applies in each mode.
- In a
- If you are not the orchestrator, and you are a subagent or no person can answer, as in a
claude -prun, do not ask. Leave the approvals fields as step 9 wrote them. Give the questions below in your final answer. The caller asks them, and writes the counts. If the Write tool refused a file to you, write the log's text, as "Before the steps" says. Else stop here, and the caller does the rest of this step. - In any other case, ask the person:
- "How many tool calls did you approve for the round's agents and for me, from
review startuntil the findings file? Do not count thebaselineagent and its validators." - With
--baseline: "How many tool calls did you approve for thebaselineagent and its validators in that time?" - Also say: "Give one total if you cannot split them. In a mode that asks for no approval, each count is 0."
- "How many tool calls did you approve for the round's agents and for me, from
- If you asked, wait for the answer. Do not guess the counts.
- Write the first count in the
approvalsfield of<run>/$1/findings.md. Write the second count in itsbaseline approvalsfield, in place of "not asked". With one total, theapprovalsfield holds the total, which then includes the baseline's, and thebaseline approvalsfield holds "in approvals".
The log <run>/log.md has two tables. If the Write tool refuses it, do not write it in another way. Give the path and the whole text of the file in your final answer. Also give two commands: target/debug/xtask review log-check <run>/log.md and target/debug/xtask review clean $1. Then stop. The caller writes the log, then runs review log-check, then review clean. If the file does not exist, write it in this form first:
# Review log
## Findings
| Date | Round | Commit | Finding | Reviewers | Severity | Significant | Kind | Rule | Where | Claim | Verdict | Repeat of | Action |
| - | - | - | - | - | - | - | - | - | - | - | - | - | - |
## Rounds
| Round | Agents | Tokens | Wall minutes | Interventions | Late review commands | Approvals | Mutants | Baseline agents | Baseline tokens | Baseline wall minutes | Baseline interventions | Baseline approvals | Baseline found |
| - | - | - | - | - | - | - | - | - | - | - | - | - | - |
Add one row to the findings table for each finding of the round, the baseline's included:
- Date: today, as
YYYY-MM-DD. Commit: the short SHA ofHEADatreview start. - Reviewers: the sources of the finding. Rule: the rule ID, or none. Claim: one line.
- If the rule of a finding is a TD, also write the rule that the TD concerns in the Rule cell, such as
PROTO-1for TD-16. - Significant: the
significantfield of the finding. - Action: one of these forms:
- the fix commit and its subject, or the diff that holds the fix;
reported, not fixed;refuted;known: <TD>; a TD ID;no change:and the reason, such as its own PR;merged into <ID>;, then the action of that finding.
- A
no change:reason names no commit SHA. - A
knownfinding has the actionknown: <TD>.
Add one row to the rounds table:
- Agents: the number of agents that the round started, but the baseline and its own validators. The validators count with the agents that they validate. Step 7's comparer counts neither with the round nor with the baseline.
- Tokens: the sum of the tokens of those agents.
- Wall minutes: the wall minutes of the findings file. They run from
review startto the last verdict, without the wait for the mutants run. - Interventions: the number of messages of a person in the round, but not the message that gives the approval counts. An approval is not an intervention.
- Late review commands: the field of the findings file. In the default permission mode, each one asked a person for approval (TD-36). Only in that mode does the count equal the approvals of those commands.
- Approvals: the count that the person gave for the round, as cost. It is the field of the findings file.
- Mutants: the mutants line.
- With
--baseline: the baseline agents are the baseline and the validators that step 5 started for its findings. Their tokens are the sum of their tokens. Their wall minutes are the duration of the baseline, plus the longest duration of those validators. Their interventions are the messages of a person for those agents. Baseline approvals: the count that the person gave for the baseline, or "in approvals" if the person gave one total. Baseline found is "of y baseline findings, the round found x". Without--baseline, write "-" in each baseline cell.
Then check the log:
- Run
target/debug/xtask review log-check <run>/log.md. - If it names a problem, correct the row or the fix before the next round.
- It runs after the findings file, so the counts of the round do not include its approval.
Then remove the worktrees and folders of the round:
- Run
target/debug/xtask review clean $1. It removes each worktree or folder undertarget/review/whose name starts with$1-, also with changes. - It runs after the findings file, where the count of approvals of this step ends. So the counts of the round do not include its approval.
- If auto mode or a person refuses it, give the command to the person, and go on.
Then decide if this round is the last round (decision 0024). The round is the last round when all of these are true:
- Its number is 3 or more.
- It has no
confirmedblocker, noconfirmedshould-fix withyes, and noconfirmedfinding on a strict rule. - It fixed, or sent to a TD, no
pre-existingorunprovenblocker, should-fix withyes, or finding on a strict rule.
A known finding does not count. A confirmed finding of the three kinds counts whatever its action, a TD included. There is no cap. If the round is not the last round, the next round follows.
After the last round:
- Copy
<run>/log.mdto the reviews folder underdocs, as<branch>.md.<branch>is the last part of the<run>path. In a run of theorchestrateskill, the copy is<branch>-m<n>.md, for milestone<n>. - Write a Progress line for each round in the plan of the work.
- Run the checks with
cargo run -p xtask -- check --tier push, and commit, signed. - Give the person the text for the PR description. It lists each
refutedfinding, eachunprovenblocker or should-fix, and eachrepeat-ofof a finding that is notknown. Then it lists eachknownfinding once, with its TD. This includes eachknownmutant. In its<details>section, it lists eachreported, not fixedfinding. It also gives the mutants line of each round.