fix(review): close the gate's fail-open paths an independent review found

An adversarial read of d88245c reproduced 15 problems; all fixed here, each
with a test that fails on the old code.

- verdict-gate: a greener verdict must be backed by the LAST fence being a
  `json` block of the right shape, with no BLOCKER in the JSON (any case),
  in `## Findings (BLOCKER)`, or a differing finding-id list; two verdict
  lines and more than one file argument are exit 2. ({}, null, lowercase
  "blocker", a trailing ```jsonc and a later json fence all gave exit 0.)
- engine-findings: a transcribed "narrower" key can no longer lower an
  engine tier; absolute paths under --repo-root match the scope; "12" lines
  parse; an empty scope is an error; out-of-scope engine findings are
  unverified (`engine:out-of-scope`) and forbid ALLOW.
- recurrence: the catch-all engine keys and keys normalised from unknown
  ones are never proposed; the same unfixed finding in two iterations is not
  a recurrence; proposals are validated before they are offered; \r/U+2028
  and damaged markers are refused; writes go through a symlinked CLAUDE.md.
  Prose points --repo at the session directory, not the git toplevel.
- Workflow substrate: engines are counted from what arrived, and the
  verdict's floor is recomputed from the coordinator's findings; now tested
  by running the script with stubbed Workflow globals. Two literal NUL bytes
  written as \u0000 (git saw the file as binary).
- Trimming keeps Pass 1's "Also flagged by" provenance; `## Declined to
  Judge` and remediation steps are required from review_version 1.1 only
  (N-1); the zero-diff review now carries the JSON block the gate needs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-06 20:03:16 +02:00
commit 40f6ae1177
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
17 changed files with 581 additions and 78 deletions

View file

@ -36,7 +36,7 @@ part that stops (Phase 5–6); the verdict is written in `review.md` and one
command decides on it (`lib/review/verdict-gate.mjs`, Phase 8: 0 = ALLOW/WARN,
1 = BLOCK, 2 = could not decide, never green). No finding is struck for
length. A finding class seen in two reviews becomes one proposal for the
project's memory, approved line by line (Phase 9.5). Order credited to Boris
project's memory, approved line by line (Phase 8.7). Order credited to Boris
Cherny's public workflow (engine → brief control); the gate to Kiro and Spec
Kit — see `docs/HANDOVER-CONTRACTS.md` § Handover 6.
@ -163,10 +163,15 @@ explicitly in the Coverage table.
If the file count is `0`, write a minimal review.md per the template:
`verdict: ALLOW`, `findings: []` (the empty list is the one inline form the
parser reads — a bare `findings:` is null and fails validation), and the
three required sections, each saying "No diff between {before_sha} and
{after_sha}; nothing to review." Skip Phases 4–7. Continue to Phase 8
(validate + stats).
parser reads — a bare `findings:` is null and fails validation), the
required sections (`## Executive Summary`, `## Coverage`, `## Declined to
Judge` with `None.` and `Trimmed: 0`, `## Remediation Summary`), each saying
"No diff between {before_sha} and {after_sha}; nothing to review.", and the
trailing block `{"verdict": "ALLOW", "counts": {"BLOCKER": 0, "MAJOR": 0,
"MINOR": 0, "SUGGESTION": 0}, "findings": []}` — without it the verdict gate
cannot back the ALLOW and exits 2. There is nothing for the Phase 4.7 engine
to review, so its absence does not withhold ALLOW here. Skip Phases 4–7.
Continue to Phase 8 (validate + stats).
## Phase 4 — Triage gate (deterministic path-pattern classifier)
@ -311,16 +316,26 @@ drop an engine finding on the way. Then convert it:
git diff --name-only "$before_sha".."$after_sha" > "{project_dir}/.review-scope.txt"
git diff --name-only HEAD >> "{project_dir}/.review-scope.txt"
node ${CLAUDE_PLUGIN_ROOT}/lib/review/engine-findings.mjs \
--scope "{project_dir}/.review-scope.txt" "{project_dir}/.review-engine-code-review.json"
--scope "{project_dir}/.review-scope.txt" --repo-root "$(git rev-parse --show-toplevel)" \
"{project_dir}/.review-engine-code-review.json"
```
Exit 0 prints a reviewer payload (`claude-code-review` /
`claude-security-review`) for Phase 6 plus `out_of_scope`: engine findings on
files outside the review's SHA range (`/security-review` diffs against
origin's default branch, which can be wider). Those go in the Coverage
section, one line each — never silently dropped. Exit 1 = the transcription is
malformed: fix it from the engine's own text and re-run. Exit 2 = could not
run.
origin's default branch, which can be wider). Pass `out_of_scope` to the
coordinator as well: each one is `unverified` (`engine:out-of-scope`), is
listed under `## Declined to Judge` and in Coverage, and forbids `ALLOW` —
never silently dropped. Absolute paths under the repo root are made relative
before the scope match (`--repo-root`). Exit 1 = the transcription is
malformed, or the scope list is empty: fix it from the engine's own text (or
the scope from Phase 3) and re-run; never feed a payload that did not pass.
Exit 2 = could not run — record that engine as not run.
A "narrower" `rule_key` you transcribe is used only when its catalogue tier is
at least the engine's own (MAJOR for `/code-review`, BLOCKER for
`/security-review`); the adapter refuses to let a transcription lower an
engine finding.
**An engine that did not run** (Claude Code older than v2.1.246, the Skill
call refused, no `origin` remote, an `ambiguous argument` error, or no findings
@ -576,8 +591,12 @@ node ${CLAUDE_PLUGIN_ROOT}/lib/review/verdict-gate.mjs "{review_path}"
```
Exit 0 = ALLOW or WARN; exit 1 = BLOCK; exit 2 = the gate could not decide
(missing verdict, a verdict the file contradicts, unreadable file) — **never
green**. Capture the exit code as `gate_exit` and print it in Phase 9 exactly
(missing verdict, a verdict the file contradicts — in its JSON block, its
`## Findings (BLOCKER)` section or its finding-id list — a JSON block that is
not the last fence or has the wrong shape, two verdict lines, unreadable
file) — **never green**. It takes exactly one file. On exit 0 with WARN,
`--json` also returns `warn_reasons` (the JSON block's `allow_blocked_by`);
print them. Capture the exit code as `gate_exit` and print it in Phase 9 exactly
as returned. This is the command that later stops a merge or a release on
BLOCK; `/trekreview` reports it and never overrides it.
@ -628,9 +647,14 @@ repo already has (every `.claude/projects/*/review.md`, plus earlier committed
versions of them) — no new store.
```bash
node ${CLAUDE_PLUGIN_ROOT}/lib/review/recurrence.mjs propose --repo "$(git rev-parse --show-toplevel)"
node ${CLAUDE_PLUGIN_ROOT}/lib/review/recurrence.mjs propose --repo "$PWD"
```
`--repo` is the session's working directory — the directory `project_dir`
paths are relative to, and the project Claude Code loads `.claude/rules/`
from. Not the git toplevel: for a project in a subdirectory that would read
no reviews and write the lessons in the wrong place.
It prints `{"proposals": [...]}` (exit 0) or exits 2 (could not run — say so
in the summary; never read 2 as "nothing recurred"). Each proposal names its
`rule_key`, its route and its evidence (the finding IDs from two reviews), and
@ -652,7 +676,7 @@ to a line it was not shown.
On yes:
```bash
node ${CLAUDE_PLUGIN_ROOT}/lib/review/recurrence.mjs apply --repo "$(git rev-parse --show-toplevel)" \
node ${CLAUDE_PLUGIN_ROOT}/lib/review/recurrence.mjs apply --repo "$PWD" \
--rule-key <RULE_KEY> [--line "<the operator's edited line>"]
```
@ -661,8 +685,14 @@ It writes ONLY between Voyage's own markers in
gives it a one-line pointer once (never the lesson itself). Exit 1 with
`over-budget` means the managed block is full: tell the operator the lessons
belong in a skill now — that is the operator's decision, not this command's.
Exit 1 with `invalid-line` means an edited line lacks the three parts: show
why and ask again. Report every outcome in the summary; when no class
Exit 1 with `invalid-line` means the line was refused by the schema check
(missing one of the three parts, a line break or control character, marker
text, or over 700 characters) — show the `detail` and ask again. Exit 1 with
`markers-damaged` means Voyage's markers in the lessons file are not one
start before one end: say so; the operator fixes the file by hand. A
finding class only counts when two reviews hold two DIFFERENT findings of it
— one unfixed finding seen again is a persisting defect, not a lesson — and
the two catch-all engine keys are never proposed. Report every outcome in the summary; when no class
recurred, say "no recurring finding class" — do not pad.
Borrowed: routing (Pocock `retro`), the three-part line (ai-native-sdlc),