fix(review): an engine finding raised again outside findings stays counted (decision 222)

A Claude Security finding counted at confidence high in round N that Claude
Security raises again at a lower confidence in round N+1 landed in that
payload's below_threshold, never in findings. Finalize passed only findings
to carry, the engine was in ran.engines, so the round-N finding was marked
resolved and the verdict fell from BLOCK to WARN (part 3 verifier, F1).

carry takes a new option, raised: the engine entries a round raised outside
findings. A previous engine finding that no current finding matches stays
carried, never resolved, when an entry of its own engine matches it (the
same file, then the same rule_key or content overlap); each entry holds at
most one previous finding. Without the option carry behaves as before.
Finalize's carry pass builds raised from every engine payload's
below_threshold and out_of_scope, the engine taken from the payload's
reviewer, never from the entry.

The carry rule is restated in the review-rounds and review-finalize headers,
/trekreview's round prose and Handover 6.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-11-08 18:22:49 +01:00
commit 3be2bd3386
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
5 changed files with 179 additions and 14 deletions

View file

@ -514,7 +514,10 @@ the round input:
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
round and did not raise it again — in its payload's `findings`, or outside
them in `below_threshold` or `out_of_scope` (decision 222: a Claude Security
finding counted at confidence `high` that comes back below it stays
counted). The engines (Phase 4.7) stay on the run range
(`/security-review` takes no range; Claude Security scans from `{before_sha}`).
`brief-conformance-reviewer` additionally receives `sc_evidence_block` from

View file

@ -462,7 +462,7 @@ Since 1.1 (ordre 5):
- `## Remediation Steps` — required at `review_version` ≥ 1.1 when there is a BLOCKER: one `### Step R<n>` per BLOCKER carrying `Source finding: <id>` (strict error `REVIEW_MISSING_REMEDIATION_STEP`, soft warning). Steps are appended and numbered, never reworded across iterations (Spec Kit `/converge`).
- `## Declined to Judge` — required at `review_version` ≥ 1.1 (strict error, soft warning; 1.0 reviews exempt): every finding set aside without a ruling, with its reason (including engine findings outside the review scope, `engine:out-of-scope`, and Claude Security findings below confidence `high`, `engine:below-threshold`), plus `Trimmed: N`. **No finding is struck for length**: only MINOR/SUGGESTION text is trimmed, and counted (`trimmed` in the JSON block); a long BLOCKER/MAJOR keeps every character.
- Findings from the engines — Claude Code's own `/code-review` and one security engine, chosen by `lib/review/security-engine.mjs choose` (Claude Security where it is installed and the range has committed changes, else `/security-review`) — enter under the catalogue keys `CODE_REVIEW_DEFECT` (MAJOR), `CODE_REVIEW_IMPROVEMENT` (MINOR: a `/code-review` finding whose category is simplification, efficiency, reuse, style or readability) and `SECURITY_REVIEW_VULNERABILITY` (BLOCKER), or a narrower key, with `reviewer` `claude-code-review` / `claude-security` (engine `claude-security`) / `claude-security-review`. A Claude Security finding counts only at confidence `high` (decision 219); the rest stand under `## Declined to Judge` as `engine:below-threshold`, an unverified reason: not counted, it forbids `ALLOW`. With no security engine (`choose` gives `no-engine`, `no-origin` or `empty-diff`, or the chosen engine did not run) the expected reviewer `security-engine` never reports: `missing-reviewer:security-engine`, verdict gate exit 2 — security not measured, never 0.
- Working files of one review run, in the project folder (not part of `review.md`): the engine transcriptions `.review-engine-{code-review|security-review}.json` (`/trekreview` Phase 4.7) and, beside them, the converted payloads `.review-engine-payload-{engine}.json` (the `engine-findings.mjs` output, `{reviewer, findings, out_of_scope}`). Claude Security's is never transcribed: `.review-engine-payload-claude-security.json` is the `security-engine.mjs convert` output from its own report, `{reviewer, findings, out_of_scope, below_threshold, report}`; `below_threshold` is never compared or put back. `review-finalize.mjs` reads the payloads to record which engines ran and to put back, at its own severity, an engine finding at MAJOR or BLOCKER that no finding of `review.md` carries (the standard-path comparison); the verdict gate reads the payloads too, to compare them with `review.md`: an engine finding at MAJOR or BLOCKER missing from it is exit 2 (`review-not-measured: engine finding not in review`). `review-rounds.mjs prepare` deletes the payloads at the start of each round.
- Working files of one review run, in the project folder (not part of `review.md`): the engine transcriptions `.review-engine-{code-review|security-review}.json` (`/trekreview` Phase 4.7) and, beside them, the converted payloads `.review-engine-payload-{engine}.json` (the `engine-findings.mjs` output, `{reviewer, findings, out_of_scope}`). Claude Security's is never transcribed: `.review-engine-payload-claude-security.json` is the `security-engine.mjs convert` output from its own report, `{reviewer, findings, out_of_scope, below_threshold, report}`; `below_threshold` is never put back and never compared with `review.md`; from round 2, an entry of it (or of any engine payload's `out_of_scope`) that matches a previous finding of the same engine keeps that finding counted: carried, never resolved (decision 222). `review-finalize.mjs` reads the payloads to record which engines ran and to put back, at its own severity, an engine finding at MAJOR or BLOCKER that no finding of `review.md` carries (the standard-path comparison); the verdict gate reads the payloads too, to compare them with `review.md`: an engine finding at MAJOR or BLOCKER missing from it is exit 2 (`review-not-measured: engine finding not in review`). `review-rounds.mjs prepare` deletes the payloads at the start of each round.
Since 1.2 (every field and section optional; a malformed one present is a strict error, a soft warning):
- Trailing JSON block, additive: per finding `pre_existing` (boolean: the range did not add or change the cited line and `scope_sha_start` is bound to the fork point from main (the fork point itself, or a later commit reached through process files only); otherwise it is never true), `carried_from` (the previous round's finding id) and `instruction_ref` (the repo path of the instruction file a CLAUDE.md-key finding cites); top level `pre_existing_counts` (`{SEVERITY: n}`), `resolved` (the previous round's finding ids no longer raised) and `ran` (`{engines: [code-review|security-review|claude-security], reviewers: [string]}`, written with `finalized: true`). Codes `REVIEW_PRE_EXISTING_MALFORMED`, `REVIEW_ROUND_MALFORMED`, `REVIEW_POLICY_MALFORMED`.

View file

@ -58,9 +58,13 @@
// a top-level `reviewer` that maps back through `ENGINES`
// (lib/review/engine-findings.mjs), and a reviewer ran when its regular
// `.review-out-<reviewer>.md` passes the findings schema; that output's
// `previous: [{id, resolved}]` are its marks. The previous review's `ran`
// counts only when it is `finalized: true`. Writes `resolved` and a
// `## Resolved since round <n>` section just before `## Declined to Judge`.
// `previous: [{id, resolved}]` are its marks. A previous engine finding
// its own engine raised again this round outside `findings` (an entry of
// that payload's `below_threshold` or `out_of_scope`, the engine taken
// from the payload's `reviewer`) stays carried, never resolved (decision
// 222). The previous review's `ran` counts only when it is
// `finalized: true`. Writes `resolved` and a `## Resolved since round <n>`
// section just before `## Declined to Judge`.
// - pre-existing: `markFindings` (lib/review/pre-existing.mjs) sets
// `pre_existing` on every JSON finding from git over
// `scope_sha_start..scope_sha_end` (read raw, as `reviewedSha` reads; a
@ -421,6 +425,27 @@ function resolvedSection(n, list) {
return `## Resolved since round ${n}\n\nPrevious findings this round no longer raises: ${list.length}.\n\n${items.join('\n')}\n\n`;
}
/** The engine a payload's top-level `reviewer` maps back to through ENGINES, or null. */
const payloadEngine = (p) => (isPlainObject(p) && typeof p.reviewer === 'string' && Object.hasOwn(ENGINE_OF_REVIEWER, p.reviewer)
? ENGINE_OF_REVIEWER[p.reviewer] : null);
/**
* The entries each engine raised this round outside its `findings` (decision 222):
* `below_threshold` and `out_of_scope`, plain objects only, each with the
* engine of its payload (never the entry's own label).
*/
function raisedOutside(payloads) {
const out = [];
for (const p of Array.isArray(payloads) ? payloads : []) {
const engine = payloadEngine(p);
if (!engine) continue;
for (const list of [p.below_threshold, p.out_of_scope]) {
for (const e of Array.isArray(list) ? list : []) if (isPlainObject(e)) out.push({ ...e, engine });
}
}
return out;
}
/** Pass `carry`: only with a previous review. */
function passCarry(payload, ctx) {
if (!ctx.previous) return {};
@ -428,6 +453,7 @@ function passCarry(payload, ctx) {
const current = payload.findings;
const r = carry(ctx.previous.findings, current, {
marks: ctx.evidence.marks, ran: ctx.evidence.ran, previousRan: ctx.previous.ran,
raised: raisedOutside(ctx.evidence.payloads),
});
// carry returns a copy of each current finding at its index: a finding an
// earlier pass added (an engine put-back) stays added under its copy.
@ -887,8 +913,7 @@ export function roundEvidence(dir) {
if (PAYLOAD_RE.test(name)) {
let payload;
try { payload = JSON.parse(readFileSync(p, 'utf8')); } catch { continue; }
const engine = isPlainObject(payload) && typeof payload.reviewer === 'string' && Object.hasOwn(ENGINE_OF_REVIEWER, payload.reviewer)
? ENGINE_OF_REVIEWER[payload.reviewer] : null;
const engine = payloadEngine(payload);
if (engine && Array.isArray(payload.findings)) {
engines.add(engine);
payloads.push(payload);

View file

@ -45,7 +45,7 @@
// - `dryRun` computes the same result and writes, copies and deletes nothing.
// Nothing is written before every check has passed.
//
// carry(previous, current, {marks, ran, previousRan}): a current finding
// carry(previous, current, {marks, ran, previousRan, raised}): a current finding
// matches a previous one in the same file with the same rule_key, or with
// `contentOverlap` >= DEFAULT_THRESHOLD (lib/review/plan-review-dedup.mjs); ids
// are never compared, and each finding matches at most one other. A previous
@ -53,8 +53,18 @@
// rule of lib/review/critic-diff.mjs, with "every reviewer that ran then"
// standing in for the raisers):
// - a matched previous finding is carried;
// - an engine finding no current finding matches is carried when its own
// engine raised it again this round outside `findings` (decision 222):
// `raised` holds that round's engine entries from each payload's
// `below_threshold` and `out_of_scope`, each with the `engine` of the
// payload it came from; an entry matches by the same rule (its `engine`
// equal to the finding's) and holds at most one previous finding. A
// Claude Security finding counted at confidence `high` that comes back
// below it is still raised, so it stays counted. Without `raised`,
// nothing is held this way;
// - an engine finding is resolved only when its engine is in `ran.engines`,
// it was not raised again, and no reviewer marks it `resolved: false`;
// it was not raised again (in `findings` or, through `raised`, outside
// them), and no reviewer marks it `resolved: false`;
// - a reviewer-only finding is resolved only when `previousRan.reviewers` is
// non-empty, every reviewer in it is in `ran.reviewers`, each marked it
// `resolved: true`, and none marked it `resolved: false`;
@ -309,12 +319,15 @@ function before(a, b) {
return false;
}
/** The current finding `p` folds into: same rule_key first, then the nearest line, then the larger overlap, then order. */
function bestMatch(p, current, taken) {
/**
* The current finding `p` folds into: same rule_key first, then the nearest line, then the larger overlap, then order.
* Only entries `eligible` accepts are considered.
*/
function bestMatch(p, current, taken, eligible = () => true) {
let best = -1;
let key = null;
current.forEach((c, i) => {
if (taken.has(i) || !sameFinding(p, c)) return;
if (taken.has(i) || !eligible(c) || !sameFinding(p, c)) return;
const k = [
c.rule_key === p.rule_key ? 0 : 1,
Number.isInteger(c.line) && Number.isInteger(p.line) ? Math.abs(c.line - p.line) : Number.MAX_SAFE_INTEGER,
@ -371,14 +384,18 @@ function resolvedNow(p, { marks, ran, previousRan }) {
* Carry the previous round's findings into this round's (see the header).
* @param {object[]} previous - the previous review's JSON findings
* @param {object[]} current - this round's JSON findings
* @param {{ marks?: Array<{id, resolved, reviewer}>, ran?: {engines, reviewers}, previousRan?: {engines, reviewers}|null }} ctx
* @param {{ marks?: Array<{id, resolved, reviewer}>, ran?: {engines, reviewers}, previousRan?: {engines, reviewers}|null,
* raised?: object[] }} ctx - `raised`: the engine entries this round raised outside `findings`
* (`below_threshold`, `out_of_scope`), each with the `engine` of the payload it came from
* @returns {{ findings: object[], resolved: string[], folded: number }}
*/
export function carry(previous, current, { marks = [], ran = { engines: [], reviewers: [] }, previousRan = null } = {}) {
export function carry(previous, current, { marks = [], ran = { engines: [], reviewers: [] }, previousRan = null, raised = [] } = {}) {
const prev = (Array.isArray(previous) ? previous : []).filter((p) => p && typeof p === 'object');
const cur = (Array.isArray(current) ? current : []).filter((c) => c && typeof c === 'object');
const again = (Array.isArray(raised) ? raised : []).filter((e) => e && typeof e === 'object' && !Array.isArray(e));
const findings = cur.map((c) => ({ ...c }));
const taken = new Set();
const raisedTaken = new Set();
const appended = [];
const resolved = [];
for (const p of prev) {
@ -388,6 +405,13 @@ export function carry(previous, current, { marks = [], ran = { engines: [], revi
findings[i] = fold(p, cur[i]);
continue;
}
// Raised again by its own engine outside `findings`: no evidence it is resolved (decision 222).
const j = typeof p.engine === 'string' && p.engine !== '' ? bestMatch(p, again, raisedTaken, (e) => e.engine === p.engine) : -1;
if (j >= 0) {
raisedTaken.add(j);
appended.push(carried(p));
continue;
}
if (resolvedNow(p, { marks, ran, previousRan })) {
if (typeof p.id === 'string') resolved.push(p.id);
continue;

View file

@ -27,7 +27,9 @@ import { tmpdir } from 'node:os';
import { dirname, join } from 'node:path';
import { fileURLToPath, pathToFileURL } from 'node:url';
import { computeFindingId, computeFindingIds } from '../../lib/parsers/finding-id.mjs';
import { convertReport } from '../../lib/review/security-engine.mjs';
import { childEnv } from '../helpers/child-env.mjs';
import { record, writeReport } from '../helpers/claude-security-report.mjs';
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
const ROUNDS = join(ROOT, 'lib', 'review', 'review-rounds.mjs');
@ -35,6 +37,7 @@ const FINALIZE = join(ROOT, 'lib', 'review', 'review-finalize.mjs');
const ANSWERS = join(ROOT, 'lib', 'review', 'review-answers.mjs');
const ENGINE = join(ROOT, 'lib', 'review', 'engine-findings.mjs');
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
const SECURITY = join(ROOT, 'lib', 'review', 'security-engine.mjs');
const VALIDATOR = join(ROOT, 'lib', 'validators', 'review-validator.mjs');
const PROJECT_REL = '.claude/projects/2026-11-02-r';
const TRAILER = 'Co-Authored-By: Claude Test <noreply@example.invalid>';
@ -1086,3 +1089,113 @@ test('[E3] isSecurityFinding: a finding from Claude Security is a security findi
assert.equal(rounds.isSecurityFinding({ engine: 'security-review', rule_key: 'CODE_REVIEW_DEFECT' }), true);
assert.equal(rounds.isSecurityFinding({ engine: 'code-review', rule_key: 'CODE_REVIEW_DEFECT' }), false, 'control: /code-review is not a security engine');
});
// ---- E 3, decision 222: an engine finding its engine raises again outside `findings` stays counted ----
// A Claude Security BLOCKER counted in round 1, at b.mjs:3.
const CS_PREV = { severity: 'BLOCKER', file: 'b.mjs', line: 3, rule_key: 'SECURITY_REVIEW_VULNERABILITY', engine: 'claude-security',
title: 'Shell command built from the branch name', detail: 'checkout() interpolates the branch name into a shell string.' };
const CS_RAN = { engines: ['claude-security', 'code-review'], reviewers: [CCR, BCR] };
/** Claude Security's payload of `records` (b.mjs:3 unless changed), converted by the real convertReport. */
function csPayload(records, scopeFiles) {
const dir = mkdtempSync(join(tmpdir(), 'review-rounds-cs-'));
made.push(dir);
const base = 'a'.repeat(40);
const head = 'b'.repeat(40);
writeReport(dir, { head, base, records: records.map((over) => record({ file: 'b.mjs', line: 3, title: CS_PREV.title, ...over })) });
const r = convertReport({ reportDir: dir, base, head, scopeFiles });
assert.equal(r.exit, 0, r.reason);
return r.payload;
}
/** A payload list's entries as finalize passes them to carry: plain objects, `engine` from the payload's reviewer. */
const raisedBy = (engine, list) => list.map((e) => ({ ...e, engine }));
test('[E3] decision 222: a counted Claude Security finding its engine raises again below confidence high (below_threshold) stays counted', () => {
const p = P(CS_PREV);
const pl = csPayload([{ confidence: 'medium' }]);
assert.deepEqual([pl.findings.length, pl.below_threshold.length], [0, 1], 'Control: the raise is in below_threshold only');
const ctx = { marks: [], ran: CS_RAN, previousRan: CS_RAN };
// Control: without `raised`, carry behaves as before: the engine ran and its findings do not hold it.
const before = rounds.carry([p], pl.findings, ctx);
assert.deepEqual([before.findings.length, before.resolved], [0, [p.id]], 'Control: without raised it resolves');
const held = rounds.carry([p], pl.findings, { ...ctx, raised: raisedBy('claude-security', pl.below_threshold) });
assert.deepEqual(held.resolved, [], 'a below_threshold raise is no evidence that it is resolved');
assert.deepEqual(held.findings.map((x) => [x.severity, x.carried_from, x.engine]), [['BLOCKER', p.id, 'claude-security']],
'it is carried as it was, at its own severity');
// Control: another engine's entry does not hold it.
const other = rounds.carry([p], pl.findings, { ...ctx, raised: raisedBy('security-review', pl.below_threshold) });
assert.deepEqual([other.findings.length, other.resolved], [0, [p.id]], 'Control: an entry of another engine holds nothing');
});
test('[E3] decision 222: a counted engine finding its engine raises again outside the review scope (out_of_scope) stays counted', () => {
const p = P(CS_PREV);
const pl = csPayload([{}], ['a.mjs']);
assert.deepEqual([pl.findings.length, pl.out_of_scope.length], [0, 1], 'Control: the raise is in out_of_scope only');
const held = rounds.carry([p], pl.findings, { marks: [], ran: CS_RAN, previousRan: CS_RAN, raised: raisedBy('claude-security', pl.out_of_scope) });
assert.deepEqual(held.resolved, [], 'an out_of_scope raise is no evidence that it is resolved');
assert.deepEqual(held.findings.map((x) => [x.severity, x.carried_from]), [['BLOCKER', p.id]]);
});
test('[E3] decision 222: each raised entry holds at most one previous finding', () => {
const p3 = P(CS_PREV);
const p8 = P({ ...CS_PREV, line: 8 });
const pl = csPayload([{ confidence: 'low' }]);
const r = rounds.carry([p3, p8], pl.findings, { marks: [], ran: CS_RAN, previousRan: CS_RAN, raised: raisedBy('claude-security', pl.below_threshold) });
assert.deepEqual(r.findings.map((x) => x.carried_from), [p3.id], 'the nearer previous finding is held');
assert.deepEqual(r.resolved, [p8.id], 'the one entry holds no second finding');
});
test('[E3] decision 222: round 2 whose Claude Security payload holds the round-1 BLOCKER only in below_threshold keeps it counted: BLOCK, never under Resolved since round 1', () => {
const fx = rndRepo();
const { prev } = roundOne(fx, [CS_PREV], { engines: ['code-review', 'claude-security'] });
const [id] = prev.findings.map((x) => x.id);
/** Round 2's Claude Security report of start..c2, converted by the real CLI and saved as Phase 4.7 saves it. */
const claudeSecurity = (records) => {
const dir = mkdtempSync(join(tmpdir(), 'review-rounds-cs-report-'));
made.push(dir);
writeReport(dir, { head: fx.c2, base: fx.start, records });
const scope = join(dir, 'scope.txt');
writeFileSync(scope, 'a.mjs\nb.mjs\n');
const r = run([SECURITY, 'convert', '--report-dir', dir, '--base', fx.start, '--head', fx.c2, '--scope', scope]);
assert.equal(r.status, 0, `security-engine convert: ${r.stdout}${r.stderr}`);
writeFileSync(join(fx.project, '.review-engine-payload-claude-security.json'), r.stdout);
return JSON.parse(r.stdout);
};
const resolvedPart = (text) => text.slice(text.indexOf('## Resolved since round 1'), text.indexOf('## Declined to Judge'));
// The coordinator declined the below-threshold raise: no finding, WARN.
roundTwo(fx, [], { outs: { [CCR]: [], [BCR]: [] }, engines: ['code-review'], verdict: 'WARN' });
const pl = claudeSecurity([record({ file: 'b.mjs', line: 3, title: CS_PREV.title, confidence: 'medium' })]);
assert.deepEqual([pl.findings.length, pl.below_threshold.length], [0, 1], 'Control: Claude Security raised it again, below high only');
const f = fx.finalize2();
assert.equal(f.status, 0, f.stdout + f.stderr);
const text = fx.read();
const payload = jsonOf(text);
assert.deepEqual(payload.ran.engines, ['claude-security', 'code-review'], 'Claude Security ran this round');
assert.deepEqual(payload.resolved, [], 'the round-1 BLOCKER is not resolved');
assert.deepEqual(payload.findings.map((x) => [x.severity, x.carried_from]), [['BLOCKER', id]], 'it stays counted, carried');
assert.equal(payload.verdict, 'BLOCK');
assert.match(fmOf(text), /^verdict: BLOCK$/m);
assert.match(text, /^## Resolved since round 1$/m, text);
assert.ok(!resolvedPart(text).includes(CS_PREV.title) && !resolvedPart(text).includes(id), `not under Resolved since round 1:\n${resolvedPart(text)}`);
const gate = fx.gate();
assert.equal(gate.status, 1, gate.stdout + gate.stderr);
assert.equal(parse(gate).reason, 'verdict BLOCK', gate.stdout);
// Control: Claude Security ran and did not raise it at all: resolved.
roundTwo(fx, [], { outs: { [CCR]: [], [BCR]: [] }, engines: ['code-review'], verdict: 'WARN' });
claudeSecurity([]);
const c = fx.finalize2();
assert.equal(c.status, 0, c.stdout + c.stderr);
assert.deepEqual(jsonOf(fx.read()).resolved, [id], 'Control: a clean Claude Security run resolves it');
assert.ok(resolvedPart(fx.read()).includes(CS_PREV.title), 'Control: listed under Resolved since round 1');
// Control: an entry in another engine's payload never holds it, whatever engine the entry names.
roundTwo(fx, [], { outs: { [CCR]: [], [BCR]: [] }, engines: [], verdict: 'WARN' });
claudeSecurity([]);
writeFileSync(join(fx.project, '.review-engine-payload-code-review.json'), JSON.stringify({
reviewer: 'claude-code-review', findings: [], out_of_scope: [], below_threshold: pl.below_threshold,
}));
assert.equal(pl.below_threshold[0].engine, 'claude-security', 'Control: the planted entry names claude-security');
const o = fx.finalize2();
assert.equal(o.status, 0, o.stdout + o.stderr);
assert.deepEqual(jsonOf(fx.read()).resolved, [id], 'Control: the engine is the payload\'s, never the entry\'s');
});