open/voyage
1
0
Fork 0
Code Issues Releases 13 Activity

fix(review): the critic-loop matcher needs overlapping texts, and a new finding never takes an old id

Browse source 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-16 16:29:58 +02:00
commit 30ca7078e9
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 187 additions and 14 deletions
Download patch file Download diff file Expand all files Collapse all files

42
lib/review/critic-diff.mjs
View file

@ -31,10 +31,14 @@
// verdict's plan_sha256 and differs from the plan now. Otherwise the round is
// counted as before (each finding at its reported severity, plus the rule-B
// record) and `diffNote` says why in one line. With rule A on, a current
// finding outside the diff is matched to a previous one (same id, its mapped
// line, or the same wording in the same file class); a previous finding stays
// counted until every reviewer that raised it marks it resolved, no reviewer
// says it is not, and nobody re-reports it.
// finding outside the diff is matched to a previous one only when their
// texts overlap (`contentOverlap` ≥ `DEFAULT_THRESHOLD`) and the current one
// sits at the previous one's mapped line, or (for a previous finding with no
// plan line) has its id, or both texts hold `ANCHOR_MIN_CONTENT_WORDS`
// content words in the same file class; a new finding never carries a
// previous finding's id: a repeated id gets `<rule_key>#n`, as in dedup.
// A previous finding stays counted until every reviewer that raised it marks
// it resolved, no reviewer says it is not, and nobody re-reports it.
// Carried findings come first (previous order), then new ones (dedup order).
//
// Trust limit: anything that can write the plan can rewrite the previous
@ -53,6 +57,7 @@ import {
normalizeSeverity, contentOverlap, contentTokens, DEFAULT_THRESHOLD, ANCHOR_MIN_CONTENT_WORDS, SEVERITIES,
} from './plan-review-dedup.mjs';
import { getRule } from './rule-catalogue.mjs';
import { computeFindingId } from '../parsers/finding-id.mjs';
export const RULE_A = 'Rule A: from round 2 on, a new major finding that cites a plan line outside the diff since the last reviewed text counts as minor; a blocker, a security finding and a finding with no plan line count at full severity, and a previous finding stays counted until it is marked resolved.';
export const RULE_B = 'Rule B: a major that a named test, `Regression:` command or gate in the plan will fail on during execution is minor — never a blocker, never a security finding.';
@ -303,19 +308,25 @@ function placeOf(f, fullToOp, planSet) {
const contentSize = t => new Set(contentTokens(typeof t === 'string' ? t : '')).size;
/** Does current finding `f` (placed at `pl`) sit at previous finding `p`'s mapped line? */
function atSpot(pl, p, d) {
if (pl === null) return false;
if (!hasOwn(p, 'op_line') || !Number.isInteger(p.op_line)) return false;
const n = d.oldToNew[p.op_line] ?? null;
return n !== null && n >= pl.start && n <= pl.end;
}
/** Does current finding `f` (placed at `pl`, outside the diff) re-report previous finding `p`? */
function sameFinding(f, pl, p, d) {
if (typeof f.id === 'string' && f.id === p.id) return true;
if (contentOverlap(f.text, p.text) < DEFAULT_THRESHOLD) return false;
if (atSpot(pl, p, d)) return true;
const pCites = hasOwn(p, 'op_line');
if (pl && pCites && Number.isInteger(p.op_line)) {
const n = d.oldToNew[p.op_line] ?? null;
if (n !== null && n >= pl.start && n <= pl.end) return true;
}
// The id is compared only for a previous finding with no plan line.
if (!pCites && typeof f.id === 'string' && f.id === p.id) return true;
const sameClass = (pl !== null && pCites) ||
(typeof f.file === 'string' && f.file !== '' && f.file === p.file);
if (!sameClass) return false;
if (contentSize(f.text) < ANCHOR_MIN_CONTENT_WORDS || contentSize(p.text) < ANCHOR_MIN_CONTENT_WORDS) return false;
return contentOverlap(f.text, p.text) >= DEFAULT_THRESHOLD;
return contentSize(f.text) >= ANCHOR_MIN_CONTENT_WORDS && contentSize(p.text) >= ANCHOR_MIN_CONTENT_WORDS;
}
/** A previous finding leaves the count only on its raisers' true, nobody's false. */
@ -427,6 +438,8 @@ export function countRound({ round, previous, base, baseNote, current, planPaths
const markList = Array.isArray(marks) ? marks : [];
const matchedTo = new Map(prevFindings.map(p => [p, []]));
// A new finding never carries a previous finding's id (dedup's `<rule_key>#n` scheme).
const takenIds = new Set(prevFindings.map(p => p.id).filter(id => typeof id === 'string'));
const fresh = [];
for (const f of Array.isArray(findings) ? findings : []) {
const o = project(f);
@ -454,6 +467,13 @@ export function countRound({ round, previous, base, baseNote, current, planPaths
o.original_severity = 'major';
o.lowered_by = `rule A: outside the diff since round ${prevRound}`;
}
if (typeof o.id === 'string' && takenIds.has(o.id)) {
let n = 2;
let id;
while (takenIds.has(id = computeFindingId(String(o.file || 'unknown'), Number.isInteger(o.line) ? o.line : 0, `${o.rule_key ?? 'unknown'}#${n}`))) n += 1;
o.id = id;
}
if (typeof o.id === 'string') takenIds.add(o.id);
fresh.push(o);
}

159
tests/validators/plan-gate-diff-mode.test.mjs
View file

@ -17,6 +17,7 @@ import { resolve } from 'node:path';
import { gatePlan, recordVerdict, planHash, operativeText, acceptOpen } from '../../lib/validators/plan-gate.mjs';
import * as diffMod from '../../lib/review/critic-diff.mjs';
import { dedupFindings, contentOverlap } from '../../lib/review/plan-review-dedup.mjs';
import { computeFindingId } from '../../lib/parsers/finding-id.mjs';
import { buildPlan } from '../helpers/plan-v18-builder.mjs';
import { spawnSync } from 'node:child_process';
import {
@ -289,10 +290,14 @@ test('8: a round-1 major that is not resolved keeps its severity (raiser silent;
const tb = editC(r1.text);
const shifted = at(tb, S1_CHANGES);
assert.equal(shifted, at(p0, S1_CHANGES) + 1);
const vb = rec(tb, dd({ pc: [guard(tb), f('major', shifted, 'For an empty input file the first step leaves the run1 result unstated')] }), { base: baseOf(r1.text) }).verdict;
const REWORDED = 'For an empty input file the first step leaves the run1 result unstated';
const vb = rec(tb, dd({ pc: [guard(tb), f('major', shifted, REWORDED)] }), { base: baseOf(r1.text) }).verdict;
assertGuardLowered(vb, 1);
const rw = byText(vb, REWORDED);
assert.equal(rw.severity, 'minor', 'a reworded re-report is its own finding; the old one keeps its severity');
assert.equal(rw.lowered_by, 'rule A: outside the diff since round 1');
const cb = byId(vb, id);
assert.equal(cb.length, 1, 'the reworded re-report is the old finding, not a new one');
assert.equal(cb.length, 1, 'a reworded re-report is its own finding; the old one keeps its severity');
assert.equal(cb[0].severity, 'major');
assert.equal(cb[0].text, M1);
assert.equal(cb[0].line, shifted);
@ -551,9 +556,11 @@ test('16: security on carry — a lowered finding re-reported with a security ke
assert.equal(byText(r2.verdict, L).severity, 'minor');
const t3 = editB(r2.text);
const NEW3 = 'The verify command for step one names no expected failing output';
const RE = 'Step 1 change gives run1 no documented failure mode at all, and the raw input reaches a shell';
assert.equal(contentOverlap(L, RE), 1, 'the re-report holds all of L\'s content words');
const v3 = rec(t3, dd({
pc: [guard(t3, NEW3, S1_VERIFY)],
sg: [f('major', at(t3, S1_CHANGES), 'Run one feeds the unescaped input path into a shell command line', { rule_key: 'SECURITY_INJECTION' })],
sg: [f('major', at(t3, S1_CHANGES), RE, { rule_key: 'SECURITY_INJECTION' })],
}), { base: baseOf(r2.text) }).verdict;
assertGuardLowered(v3, 2, NEW3);
const c = byId(v3, id);
@ -563,6 +570,19 @@ test('16: security on carry — a lowered finding re-reported with a security ke
assert.deepEqual([...c[0].raised_by].sort(), ['plan-critic', 'scope-guardian']);
assert.equal(v3.verdict, 'REVISE');
// An unrelated security finding at the lowered finding's place stands alone.
const UNRELATED = 'Run one feeds the unescaped input path into a shell command line';
const vu = rec(t3, dd({
pc: [guard(t3, NEW3, S1_VERIFY)],
sg: [f('major', at(t3, S1_CHANGES), UNRELATED, { rule_key: 'SECURITY_INJECTION' })],
}), { base: baseOf(r2.text) }).verdict;
const u = byText(vu, UNRELATED);
assert.equal(u.severity, 'major', 'an unrelated security finding at a lowered finding\'s place is its own finding');
assert.equal(u.lowered_by, undefined);
const cu = byId(vu, id);
assert.equal(cu.length, 1, JSON.stringify(vu.findings));
assert.equal(cu[0].severity, 'minor');
// A hand-built dedup finding with a security rule_key and no rule_keys.
const t2b = editA(r1.text);
const hand = {
@ -924,3 +944,136 @@ test('24: the e2e script is ASCII only and bash -n clean', () => {
const r = spawnSync('/bin/bash', ['-n', E2E], { encoding: 'utf-8', env: childEnv() });
assert.equal(r.status, 0, r.stderr);
});
// ---- the matcher needs overlapping texts; a new id never repeats an old one ----
const M1 = 'Step 1 never says what run1 returns on an empty input file';
test('25: an unrelated new major on an old finding\'s former line number after a shift is new, with a distinct id', () => {
const p0 = buildPlan();
const X = at(p0, S1_CHANGES);
const r1 = round1({ pc: [f('major', X, M1)] });
const id = r1.verdict.findings[0].id;
const t2 = editC(r1.text);
assert.equal(at(t2, S1_CHANGES), X + 1, 'the edit shifts the old place down by one');
const NEW = 'The previous step leaves its input encoding unstated for callers';
const v = rec(t2, dd({
pc: [f('major', X, NEW), guard(t2)],
pcPrev: [{ id, resolved: true }], sgPrev: [{ id, resolved: true }],
}), { base: baseOf(r1.text) }).verdict;
const n = byText(v, NEW);
assert.equal(n.severity, 'minor');
assert.equal(n.lowered_by, 'rule A: outside the diff since round 1');
assert.notEqual(n.id, id, 'a new finding never carries an old finding\'s id');
assert.equal(n.id, computeFindingId('plan.md', X, 'step-quality#2'));
assert.deepEqual(v.resolved, [id]);
assert.equal(v.findings.filter(x => x.text === M1).length, 0, 'no finding reads M1');
assert.equal(v.verdict, 'PASS');
});
test('26: an unrelated major at an old finding\'s place, from the other reviewer with another rule_key, is its own finding', () => {
const p0 = buildPlan();
const r1 = round1({ pc: [f('major', at(p0, S1_CHANGES), M1)] });
const id = r1.verdict.findings[0].id;
const t2 = editA(r1.text);
const NEW = 'Nothing in the plan says who owns the generated module after delivery';
const v = rec(t2, dd({
pc: [guard(t2)],
sg: [f('major', at(t2, S1_CHANGES), NEW, { rule_key: 'scope-drift' })],
pcPrev: [{ id, resolved: true }], sgPrev: [{ id, resolved: true }],
}), { base: baseOf(r1.text) }).verdict;
const n = byText(v, NEW);
assert.equal(n.severity, 'minor');
assert.equal(n.lowered_by, 'rule A: outside the diff since round 1');
assert.deepEqual(v.resolved, [id]);
assert.equal(v.findings.filter(x => x.text === M1).length, 0, 'no finding reads M1');
assertGuardLowered(v, 1);
assert.equal(v.verdict, 'PASS');
});
test('27: a new file-level major with an old rule-B-lowered finding\'s id counts at major with its own text and id, and survives the old one\'s resolution', () => {
const OLD = 'The plan names no owner for the generated fixtures';
const r1 = round1({
pc: [{ file: 'plan.md', line: 0, rule_key: 'gap', severity: 'minor', lowered_from: 'major', caught_by: 'npm test', text: OLD }],
});
const o1 = byText(r1.verdict, OLD);
assert.equal(o1.severity, 'minor');
assert.equal(o1.lowered_by, 'rule B: npm test');
const id1 = o1.id;
const t2 = editA(r1.text);
const NEW2 = 'The rollout order of the two migrations is never stated';
const r2 = rec(t2, dd({
pc: [{ file: 'plan.md', line: 0, rule_key: 'gap', severity: 'major', text: NEW2 }, guard(t2)],
}), { base: baseOf(r1.text) });
const v2 = r2.verdict;
const n2 = byText(v2, NEW2);
assert.equal(n2.severity, 'major');
assert.equal(n2.original_severity, undefined);
assert.notEqual(n2.id, id1);
assert.equal(byText(v2, OLD).severity, 'minor');
assert.deepEqual(v2.counts, { blocker: 0, major: 1, minor: 2 });
assert.equal(v2.verdict, 'REVISE');
const t3 = editB(r2.text);
const NEW3 = 'No step says how the cache directory is cleaned after a failed run';
const v3 = rec(t3, dd({
pc: [{ file: 'plan.md', line: 0, rule_key: 'gap', severity: 'major', text: NEW3 }],
pcPrev: [{ id: id1, resolved: true }],
}), { base: baseOf(r2.text) }).verdict;
assert.deepEqual(v3.resolved, [id1]);
const c2 = byText(v3, NEW2);
assert.equal(c2.severity, 'major');
assert.equal(c2.id, computeFindingId('plan.md', 0, 'gap#2'));
assert.equal(byText(v3, NEW3).id, computeFindingId('plan.md', 0, 'gap#3'));
assert.equal(v3.verdict, 'REVISE');
});
test('28: the clauses with short texts (fewer than 6 content words): at its place, no id match for a plan-line finding, id match with no plan line', () => {
const SHORT = 'Run1 output unstated';
const p0 = buildPlan();
const X = at(p0, S1_CHANGES);
const reads = (v, s) => v.findings.filter(x => x.text === s);
// (i) clause (a): the same short text at the old finding's place (shifted).
{
const r1 = round1({ pc: [f('major', X, SHORT)] });
const id = r1.verdict.findings[0].id;
const t2 = editC(r1.text);
const v = rec(t2, dd({ pc: [f('major', at(t2, S1_CHANGES), SHORT), guard(t2)] }), { base: baseOf(r1.text) }).verdict;
const hit = reads(v, SHORT);
assert.equal(hit.length, 1, JSON.stringify(v.findings));
assert.equal(hit[0].id, id);
assert.equal(hit[0].severity, 'major');
assert.equal(hit[0].carried_from, 1);
}
// (ii) a plan-line finding is never matched by id: the same text at the old full-file line number.
{
const r1 = round1({ pc: [f('major', X, SHORT)] });
const id = r1.verdict.findings[0].id;
const t2 = editC(r1.text);
const v = rec(t2, dd({ pc: [f('major', X, SHORT), guard(t2)] }), { base: baseOf(r1.text) }).verdict;
const hit = reads(v, SHORT);
assert.equal(hit.length, 2, JSON.stringify(v.findings));
const carried = hit.filter(x => x.id === id);
assert.equal(carried.length, 1);
assert.equal(carried[0].severity, 'major');
const fresh = hit.filter(x => x.id !== id);
assert.equal(fresh.length, 1);
assert.equal(fresh[0].severity, 'minor');
assert.equal(fresh[0].lowered_by, 'rule A: outside the diff since round 1');
}
// (iii) clause (b): no plan line, the same id and text.
{
const MOD = 'Missing null check';
const r1 = round1({ pc: [{ file: 'src/mod1.mjs', line: 12, rule_key: 'step-quality', severity: 'major', text: MOD }] });
const t2 = editA(r1.text);
const v = rec(t2, dd({ pc: [{ file: 'src/mod1.mjs', line: 12, rule_key: 'step-quality', severity: 'major', text: MOD }, guard(t2)] }), { base: baseOf(r1.text) }).verdict;
const hit = reads(v, MOD);
assert.equal(hit.length, 1, JSON.stringify(v.findings));
assert.equal(hit[0].carried_from, 1);
assert.equal(hit[0].severity, 'major');
}
});