feat(review): review rounds carry findings until resolved, one cap of three

This commit is contained in:
Kjell Tore Guttormsen 2026-11-02 17:44:23 +01:00
commit 635003e8aa
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
13 changed files with 2021 additions and 100 deletions

View file

@ -2348,10 +2348,12 @@ the run is `stopped`, and the merge stays closed.
`{report dir}/review-round-{R}-steps.md`, and stop. A later `--resume`
finds the plan as approved and comes back to this review.
**The cap:** `TREKEXECUTE_MAX_REVIEW_ROUNDS` reviews per run, default **3**
(the first review and two fix loops; env-overridable).
A BLOCK at the cap goes to the operator: stop, and the report names every
BLOCKER still open. Each loop also takes 1 from `iterations_remaining`, which
**The cap:** `MAX_REVIEW_ROUNDS` in `lib/review/review-rounds.mjs`, **3**
reviews per run (the first and two fix loops), read with
`node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-rounds.mjs --cap`; `/trekreview`
reads the same constant and refuses a fourth round.
A review at the cap with BLOCKERs or MAJORs answered "fixed" still open goes to the operator: stop, and the report names every one.
Each loop also takes 1 from `iterations_remaining`, which
bounds it a second time (the completion gate's clause 3 counts it).
Borrowed: the order (run the thing, simplify, review) is Cherny's; a verdict in
@ -3235,7 +3237,7 @@ set from the effort.
itself (Phase 7.2). Exit 1 (BLOCK) or 2 (not measured) from the verdict
gate keeps the merge into main closed (Phase 8). A BLOCKER is fixed by new
numbered plan steps the operator approves, in a loop capped at
`TREKEXECUTE_MAX_REVIEW_ROUNDS`, never by improvising. When the lock
`MAX_REVIEW_ROUNDS` (`lib/review/review-rounds.mjs`), never by improvising. When the lock
opens, the guard asks before the merge, and the executor never asks again.
Exit 2 from the step check (K9), the architecture rules (K5) or the
plan-file sweep (K6) is "not measured", never green.

View file

@ -155,6 +155,28 @@ Determine the "before" SHA that bounds the review:
Compute the "after" SHA: `after_sha=$(git rev-parse HEAD)`.
**Then the round.** Rounds converge, at most `MAX_REVIEW_ROUNDS` (**3**, in
`lib/review/review-rounds.mjs`) per run:
```bash
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-rounds.mjs prepare --project "{project_dir}" --since {before_sha} --json
```
Add `--workflow` when `workflow_substrate == true` and `--dry-run` when
`mode == dry-run`. It prints `{round, round_base, previous}`; keep all three for
Phases 5 and 7. A later round is one where `{project_dir}/review.md` has this
`scope_sha_start` (the same run): `prepare` counts the round from the run's
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), or a round 2 or later was asked on
`--workflow` (re-run without 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.
Capture working-tree changes (uncommitted at review time):
```bash
git diff --name-only "$before_sha".."$after_sha"
@ -406,6 +428,21 @@ Each reviewer prompt includes:
- **Brief path** — `{brief_path}` (read on demand; do not inline).
- **Rule catalogue** — reference to `lib/review/rule-catalogue.mjs`.
**From round 2** (`round` ≥ 2 from Phase 3), each reviewer prompt also gets
the round input:
- the round diff, `git diff {round_base}..{after_sha}`, beside the run's diff;
- the previous findings, from the trailing JSON block of
`{project_dir}/{previous}`, each with its id, to mark in the reviewer's own
JSON block as `previous: [{id, resolved}]` (`resolved: true` only when the
round diff fixes it);
- the line "you may read the files around it; a previous finding is never
re-raised as new; flag every occurrence of a finding class".
Finalize (Phase 7) carries a previous finding until every reviewer that ran in
its round marks it resolved, and an engine finding until its engine ran this
round and did not raise it again. The engines (Phase 4.7) stay on the run range
(`/security-review` takes no range).
`brief-conformance-reviewer` additionally receives `sc_evidence_block` from
Phase 4.5, pasted verbatim — the command, exit code and first line of output for
every Success Criterion. It is the ONLY evidence that agent has about whether a
@ -628,13 +665,30 @@ parser at `lib/util/frontmatter.mjs` does not support flow-style arrays.
**Then finalize the review** — one deterministic pass after `fill-finding-ids`:
```bash
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-finalize.mjs --review "{project_dir}/review.md" --repo . --project "{project_dir}" --round 1
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-finalize.mjs --review "{project_dir}/review.md" --repo . --project "{project_dir}" --round {round}
```
On exit 1 or 2 the file stays as `fill-finding-ids` wrote it; print the reason in one line and continue.
From round 2 it also gets the round base and the previous review, as `prepare`
printed them, and the carried answers follow it:
```bash
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-finalize.mjs --review "{project_dir}/review.md" --repo . --project "{project_dir}" --round {round} --round-base {round_base} --previous "{project_dir}/{previous}"
node ${CLAUDE_PLUGIN_ROOT}/lib/review/review-answers.mjs carry --review "{project_dir}/review.md" --previous "{project_dir}/{previous}" --repo .
```
In round 1, on exit 1 or 2 the file stays as `fill-finding-ids` wrote it; print the reason in one line and continue.
From round 2, finalize exit 1 or 2 stops the review: print the reason and "not measured", and stop. The previous
findings reach review.md only through finalize's carry, so the verdict gate reads a round 2+ review without
`finalized: true` as exit 2 (`review-not-measured: round 2+ review not finalized`), never green.
Finalize writes `round`, `round_base`, `finalized: true`, `ran` (what ran this round, read from the
`.review-out-*.md` and `.review-engine-payload-*.json` files) and, from round 2, `resolved` with a
`## Resolved since round <n>` section; a carried BLOCKER gets its `### Step R<n>`. `review-answers.mjs carry`
writes an `## Answers` row with `Carried from` for each carried MAJOR the operator accepted in an
earlier round; on exit 1 or 2 print the reason in one line and continue: no answer is carried.
A finding is pre-existing when it sits on a line the range `scope_sha_start..scope_sha_end` did not add or
change; finalize marks it from git, the verdict gate re-derives the mark, and a BLOCKER or security finding
counts in full whatever the mark.
counts in full whatever the mark. A carried finding (`carried_from`) keeps its line from the previous round's end,
so it is never pre-existing, whatever the mark.
**The verdict is in the file (K7).** The frontmatter carries
`verdict: ALLOW | WARN | BLOCK`, equal to the trailing JSON block's