docs(review): stop rule in the plan loop, review loop and critic prompts (decision 217, 220)

/trekplan Phase 9: when --record or the gate reports the stop rule there
is no new round; AskUserQuestion with the class and counts, "Known issue"
(--accept-open, a security finding refused) or "New mechanism" (stop, the
plan stays unapproved); the round-3 blocker path never offers
--extra-round after the stop rule. Phase 10 prints round of
max_rounds + extra_rounds.

/trekexecute Phase 7.2 step 4: after a round-2 BLOCK or fixed-pending,
before any R-step, review-rounds.mjs prepare --dry-run; review-stop-rule:
gives the same question ("Known issue" = /trekreview --answers, a BLOCKER
keeps the merge locked; "New mechanism" = stop). /trekreview Phase 3 tells
the stop rule's exit 1 from the cap's. plan-critic and scope-guardian:
rule_key names the class, never the instance.

New pins in review-loop-docs, critic-severity-rules and review-prose-pins.

Part 1, step 1.4 of the E plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-11-08 07:16:05 +01:00
commit 08e8aa1c66
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
8 changed files with 134 additions and 12 deletions

View file

@ -392,6 +392,11 @@ this block inline *is* the hand-off; do not attempt to write a file.
One object per finding. `file`/`line` point at the plan section (use the plan
path + the step's line) or the source the finding concerns; `rule_key` is a
short, stable id for the finding class (used for exact-match dedup).
`rule_key` names the class, never the instance: a catalogue key when one
fits, else `<dimension>/<short-id>` with the same dimension for every finding
of that kind. The plan gate's stop rule counts findings by class (the
catalogue key, else the part before the first `/`), so a key made for one
instance hides a class that keeps coming back.
`severity` is exactly one of `blocker`, `major`, `minor`. Emit
`"findings": []` when the plan is clean.
`previous` (from round 2; omit it in round 1) holds one `{id, resolved}` per

View file

@ -182,6 +182,11 @@ this block inline *is* the hand-off; do not attempt to write a file.
One object per scope-creep / gap / dependency finding. `file`/`line` point at the
plan step (or the brief requirement) the finding concerns; `rule_key` is a short,
stable id for the finding class (used for exact-match dedup against plan-critic).
`rule_key` names the class, never the instance: a catalogue key when one fits,
else the kind (`creep`, `gap`, `dependency`, or `<kind>/<short-id>`), the same
kind for every finding of that kind. The plan gate's stop rule counts findings
by class (the catalogue key, else the part before the first `/`), so a key
made for one instance hides a class that keeps coming back.
Emit `"findings": []` when the plan is fully aligned.
`previous` (from round 2; omit it in round 1) holds one `{id, resolved}` per
finding the current `## Critic Verdict` lists; `resolved` is `true` or

View file

@ -2286,6 +2286,22 @@ never typed by the operator, in this order (Cherny's):
the loop below, as a BLOCK does. The loop's R-steps are then the BLOCKER
steps plus each MAJOR answered "fixed". Any other `review-unanswered:` goes
to Phase 7.5 with the merge closed.
**The stop rule first** (decisions 217, 220). After a round-2 BLOCK or
`fixed-pending` (`review.round` = 2), before any R-step is added, ask
`prepare` whether round 3 may open, without writing anything:
```bash
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-rounds.mjs prepare --project "{project_dir}" --since {session_start_sha} --repo . --dry-run --json
```
A `reason` that starts `review-stop-rule:` → no loop, and no R-step is
added. Ask the operator in one `AskUserQuestion` with that line in plain
words (the class and the blocking counts per round), recommendation first:
**Known issue** — the open MAJORs are answered `accepted` through
`/trekreview --answers --project {project_dir}` (the review is still
current, so no new round opens; a security finding cannot be accepted),
and the run goes on to Phase 7.5 with the lock as the verdict gate then
reads it: a BLOCKER keeps the merge locked (decision 220); **New mechanism** — stop (stop class 4, Phase 5.5): the run is
`stopped`, the merge stays closed, and the direction goes to the operator.
Any other result (exit 0, the cap, exit 2) → the loop below as before.
**A BLOCK is fixed by new plan steps, in a bounded loop.** The review writes
every BLOCKER as a numbered `### Step R<n>` under `## Remediation Steps`

View file

@ -928,7 +928,9 @@ verdict and decides whether the plan may go on. A BLOCKER or an open MAJOR
stops the plan: revise and run the next round. The loop is done on a round
that leaves neither, or when the operator accepts the open MAJORs after the
last round; after round 3 with a BLOCKER still open it stops and asks the
operator. It never goes on silently.
operator. A round whose blocking findings did not get fewer, or that brings
new blocking findings of the same class as the round before, ends the loop
too: the stop rule (below). It never goes on silently.
**From round 2.** Round 1 is unchanged: the reviewers read the whole plan.
From round 2 on, first print what changed since the text the last round
@ -1027,8 +1029,11 @@ snapshot is the operative text this round reviewed, kept under
`<git dir>/voyage/plan-snapshots/` and never committed; the next round's
`--diff` reads it. A plan outside any git repository (plan mode) keeps no
snapshot (`not kept: no git repository`), so its next round counts fully.
A failed snapshot write does not fail the round.
Exit 1 means the round cap is spent (see "After round 3"). Exit 2 means the
A failed snapshot write does not fail the round. `stop_rule` is this round's
stop-rule decision (see "The stop rule"), and `reason` names the code of a
refusal.
Exit 1 means the round cap is spent (see "After round 3") or the stop rule
fired after the previous round (`reason: PLAN_GATE_STOP_RULE`). Exit 2 means the
round cannot be recorded: broken input, a reviewer missing from it (an absent
review is not a clean one), or a plan that changed after `--hash` — the
verdict would bless text nobody reviewed. Show the message and rerun the round
@ -1045,9 +1050,9 @@ shorter text's content words in the longer one), and keeps the copy rule B count
duplicates. Source: research/05 R1 + R2.
**After each round**, by the recorded verdict:
- **BLOCK** or **REVISE**, rounds left: revise the plan for the merged
findings, add rows to `## Revisions` (what changed and why), and run the
next round. Any edit outside the review log — a step, the design, the
- **BLOCK** or **REVISE**, rounds left, and the stop rule did not fire:
revise the plan for the merged findings, add rows to `## Revisions` (what
changed and why), and run the next round. Any edit outside the review log — a step, the design, the
verification — makes the recorded verdict stale, so a revised plan is always
re-reviewed.
- **PASS** (minors only, or clean): fill in the quality header and
@ -1068,8 +1073,33 @@ duplicates. Source: research/05 R1 + R2.
The acceptance is bound to this round and this plan text; the majors stay
listed in the Phase 10 summary. A blocker is never accepted.
**The stop rule** (decisions 217, 220; `lib/review/stop-rule.mjs`): when the
`--record` line carries `stop_rule.fired: true`, or the gate reports
`PLAN_GATE_STOP_RULE`, there is no new round. The rule fires after round 2 or
later when one finding class has new blocking findings in this round and the
round before, or when the blocking findings did not get fewer; a class is the
catalogue `rule_key`, else the reviewer's dimension (the `rule_key` before the
first `/`). `--record` refuses any new round (exit 1, `--extra-round`
included), and the gate's STOP line replaces the open-blocker and open-major
reasons. Stop and ask the operator with AskUserQuestion: the gate's line in
plain words (the class and the blocking counts per round), recommendation
first, two options:
- **Known issue** — after this yes, record it:
`node ${CLAUDE_PLUGIN_ROOT}/lib/validators/plan-gate.mjs --accept-open "{plan_path}"`.
It accepts this round's open blockers and majors as known issues, with
rounds left too; the gate then lists them as warnings. A security finding is never accepted: `--accept-open` refuses with
`PLAN_GATE_SECURITY_OPEN`, and then only New mechanism is left.
- **New mechanism** — stop; the plan stays unapproved, `/trekexecute` is not
offered, and the direction goes to the operator.
The question never offers another round, and nothing here is revised and
recorded again.
**After round 3 with a BLOCKER open**, `plan-gate.mjs` reports
`PLAN_GATE_ROUNDS_EXHAUSTED` and `--record` refuses a fourth round. Stop and ask
`PLAN_GATE_ROUNDS_EXHAUSTED` and `--record` refuses a fourth round. This path
is only for a round 3 where the stop rule did not fire: the gate reports
`PLAN_GATE_STOP_RULE` in its place otherwise, and this path never offers
`--extra-round` after the stop rule. Stop and ask
the operator with AskUserQuestion — the open blockers listed in plain words,
recommendation first:
- **One more round** — revise for the listed blockers, then record with
@ -1084,8 +1114,9 @@ Then check the plan with the gate (K3), which every later step relies on:
node ${CLAUDE_PLUGIN_ROOT}/lib/validators/plan-gate.mjs --json "{plan_path}"
```
Exit 0 = the plan validates, the verdict matches it, no BLOCKER is open, and
every open MAJOR was accepted by the operator. Exit 1 = stop; its `reasons`
Exit 0 = the plan validates, the verdict matches it, no BLOCKER is open (or
each was accepted as a known issue under the stop rule), and every open MAJOR
was accepted by the operator. Exit 1 = stop; its `reasons`
say why. Exit 2 = it could not read the plan.
Borrowed, with credit: the bounded review loop that ends only on a recorded
@ -1127,7 +1158,7 @@ Present a summary to the user:
{Summary of external research + attached research briefs, or "No external research used."}
### Adversarial review
**Critic rounds:** {N} of {max} — last verdict {BLOCK | REVISE | PASS}
**Critic rounds:** {round} of {max_rounds + extra_rounds} — last verdict {BLOCK | REVISE | PASS}
**Plan critic:** {Summary — blockers/majors/minors found, how addressed}
**Scope guardian:** {Summary — creep/gaps found, how addressed}
**Open majors:** {list with "accepted by the operator" for each, or "none"}

View file

@ -153,8 +153,15 @@ archives, keeps the previous review as `{project_dir}/{previous}`
(`review-round-<run>-<n>.md`, `<run>` = the first 12 hex of `before_sha`), and
`round_base` is its `scope_sha_end`. Every round it deletes the previous
round's `.review-out-*.md` and `.review-engine-payload-*.json` files, so each
such file finalize reads was written this round. Exit 1 stops: the round cap is
reached (the operator re-plans). Exit 2 stops "not measured" (the round base
such file finalize reads was written this round. Exit 1 stops, for one of two
reasons, told apart by the start of `reason`: `review round cap reached` (the
operator re-plans), or `review-stop-rule:` — the stop rule (decisions 217,
220; `lib/review/stop-rule.mjs`), judged when round 3 would open: the
previous round and this one brought new BLOCKER/MAJOR findings of the same
class, or the blocking findings (BLOCKERs and counting MAJORs) did not get
fewer. Its line gives the class and the counts per round with the two
recommendations, Known issue or New mechanism, never another round; the
operator decides (`/trekexecute` Phase 7.2 asks it). Exit 2 stops "not measured" (the round base
is not an ancestor of HEAD, the round count does not match the archives, or an
archive differs). Print the reason in one line; no review is written in either
case.

View file

@ -61,6 +61,11 @@ for (const rel of AGENTS) {
assert.ok(t.includes("A security finding uses the catalogue's security `rule_key`"));
});
test(`${rel}: [STP] rule_key names the class, never the instance (the stop rule's class, decision 217)`, () => {
const t = norm(read(rel));
assert.ok(t.includes('`rule_key` names the class, never the instance'), `${rel} must say rule_key names the class, never the instance`);
});
test(`${rel}: round 2+ lists every previous finding by id and cites the exact line`, () => {
const t = norm(read(rel));
assert.ok(t.includes('From round 2 the prompt gives you the diff since the last reviewed text'), `${rel}: round-2 diff sentence missing`);

View file

@ -138,3 +138,32 @@ test('Handover 4 states the overlap match and the distinct id', () => {
assert.ok(h4.includes('only on overlapping text'), 'Handover 4 says the gate matches "only on overlapping text"');
assert.ok(h4.includes('`<rule_key>#n`'), 'Handover 4 names `<rule_key>#n`');
});
// ---- [STP] the stop rule (E plan, Part 1 step 1.4; decisions 217, 220) ----------------
const P10 = flat(phase(read('commands/trekplan.md'), 10));
/** Phase 9's stop-rule paragraph: from its bold lead to the next bold lead that opens a line. */
function stopPart() {
const at = P9.indexOf('**The stop rule**');
assert.ok(at >= 0, 'Phase 9 has a **The stop rule** paragraph');
const next = P9.indexOf('**After round 3', at);
return P9.slice(at, next < 0 ? undefined : next);
}
test('[STP] Phase 9: the stop rule gives no new round and asks Known issue or New mechanism, never one more round', () => {
const s = stopPart();
for (const want of ['stop_rule.fired', 'PLAN_GATE_STOP_RULE', 'no new round', 'AskUserQuestion',
'**Known issue**', 'plan-gate.mjs --accept-open', 'PLAN_GATE_SECURITY_OPEN',
'**New mechanism**', 'the plan stays unapproved', 'the direction goes to the operator']) {
assert.ok(s.includes(want), `the stop-rule paragraph says ${JSON.stringify(want)}`);
}
assert.doesNotMatch(s, /\*\*One more round\*\*/, 'the stop-rule question has no "One more round" option');
assert.ok(P9.includes('never offers `--extra-round` after the stop rule'),
'the round-3 blocker path says it never offers --extra-round after the stop rule');
});
test('[STP] Phase 10 prints the round of max_rounds + extra_rounds', () => {
assert.ok(P10.includes('**Critic rounds:** {round} of {max_rounds + extra_rounds}'), 'Phase 10 prints round of max_rounds + extra_rounds');
assert.ok(!P10.includes('**Critic rounds:** {N} of {max}'), 'the old "{N} of {max}" is gone');
});

View file

@ -338,3 +338,27 @@ test('[DPR] /trekreview Phase 4 says a changed instruction file at any depth, do
assert.ok(lines.includes('| `summary-only` | `docs/**`, Markdown files at the repo root other than `CLAUDE.md` and `AGENTS.md` (README, CHANGELOG), `tests/fixtures/**`. |'),
'Control: the summary-only row is unchanged');
});
// ---- [STP] the stop rule in the review rounds (E plan, Part 1 step 1.4; decisions 217, 220) ----
test('[STP] /trekreview Phase 3 tells the stop rule\'s exit 1 from the cap\'s', () => {
const p3 = flat(section(CMD, '## Phase 3 — Discover scope SHA range'));
assert.ok(p3.includes('`review-stop-rule:`'), 'Phase 3 names the review-stop-rule: reason');
assert.ok(p3.includes('`review round cap reached`'), 'Phase 3 names the cap reason');
assert.ok(p3.includes('Known issue') && p3.includes('New mechanism'), 'Phase 3 names the two recommendations');
});
test('[STP] /trekexecute Phase 7.2 step 4 runs prepare --dry-run after a round-2 BLOCK, before any R-step, and asks Known issue or New mechanism', () => {
const p72 = flat(section(read('commands/trekexecute.md'), '## Phase 7.2 — Simplify, review, verdict (P11)'));
const cmd = 'review-rounds.mjs prepare --project "{project_dir}" --since {session_start_sha} --repo . --dry-run --json';
const at = p72.indexOf(cmd);
assert.ok(at >= 0, `Phase 7.2 runs ${cmd}`);
const loop = p72.indexOf('**A BLOCK is fixed by new plan steps');
assert.ok(loop > at, 'the dry-run check comes before the loop adds any R-step');
const part = p72.slice(at, loop);
for (const want of ['`review-stop-rule:`', 'AskUserQuestion', '**Known issue**', '/trekreview --answers',
'the review is still current', 'a BLOCKER keeps the merge locked', '**New mechanism**', 'no R-step is added']) {
assert.ok(part.includes(want), `the stop-rule paragraph says ${JSON.stringify(want)}`);
}
assert.doesNotMatch(part, /one more round/i);
});