fix(review): the verdict gate refuses duplicate finding ids

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

lib/review/review-answers.mjs

@@ -339,6 +339,12 @@ export function decideAnswers(text, { records = [], reviewSubject, preExisting =
   if (!payload) {
     return { ok: false, unanswered: [], fixedPending: [], reason: 'review-not-measured: no readable trailing JSON block' };
   }
+  // One id twice would let one bound answer clear two MAJORs (fail closed).
+  const ids = (Array.isArray(payload.findings) ? payload.findings : []).filter((f) => f && typeof f === 'object').map((f) => f.id);
+  const dup = ids.find((id, i) => ids.indexOf(id) !== i);
+  if (dup !== undefined) {
+    return { ok: false, unanswered: [], fixedPending: [], reason: `review-not-measured: duplicate finding id ${dup}` };
+  }
   const majors = countingMajors(payload, { preExisting });
   if (majors.length === 0) return { ok: true };
   const { rows } = parseAnswers(bodyOf(text));
@@ -498,6 +504,9 @@ export function recordAnswer({ reviewPath, finding, answer, repo, round, store,
   try { text = readFileSync(reviewPath, 'utf8'); } catch (e) { return cannot(`cannot read ${reviewPath}: ${e.message}`); }
   const payload = payloadOf(text);
   if (!payload || !Array.isArray(payload.findings)) return cannot(`${reviewPath} has no readable trailing JSON block`);
+  const ids = payload.findings.filter((x) => x && typeof x === 'object').map((x) => x.id);
+  const dup = ids.find((x, i) => ids.indexOf(x) !== i);
+  if (dup !== undefined) return cannot(`${reviewPath} holds a duplicate finding id ${dup}; nothing appended`);
   const f = payload.findings.find((x) => x && x.id === id);
   if (f && isBlocker(f)) return refuse(`${id} is a BLOCKER: a BLOCKER is never answered, it is fixed`);
   if (!f || !countingMajors(payload).some((x) => x.id === id)) return refuse(`${id} is not a counting MAJOR of this review`);

lib/review/verdict-gate.mjs

@@ -48,6 +48,9 @@
 //         engine finding not in review: <severity> <rule_key> at
 //         <file>:<line>[, …]`; `droppedEngineFindings`): finalize's put-back
 //         did not run, or `finalized: true` was written by hand.
+//         An ALLOW/WARN review that holds one finding id twice, in the
+//         frontmatter `findings:` list or in the trailing JSON block, is exit
+//         2 (reason `duplicate finding id <id> in <where>`).
 //         Exit 2 is NEVER green.
 //
 // The pure `decideVerdict` stays text-only: it never reads the answers. The
@@ -159,6 +162,13 @@ export function decideVerdict(text) {
   if (illegal.length > 0) {
     return undecided(`verdict ${verdict} over a brief-conflict lowering outside the one legal shape (finding ${illegal.join(', ')})`, verdict);
   }
+  // One id twice would let one bound answer clear two MAJORs: not measured.
+  const fmList = Array.isArray(fm.findings) ? fm.findings : [];
+  const fmDup = fmList.find((id, i) => fmList.indexOf(id) !== i);
+  if (fmDup !== undefined) return undecided(`duplicate finding id ${fmDup} in the frontmatter findings: list`, verdict);
+  const jsonList = payload.findings.map((f) => f.id);
+  const jsonDup = jsonList.find((id, i) => jsonList.indexOf(id) !== i);
+  if (jsonDup !== undefined) return undecided(`duplicate finding id ${jsonDup} in the trailing JSON block`, verdict);
   const fmIds = new Set(Array.isArray(fm.findings) ? fm.findings : []);
   const jsonIds = new Set(payload.findings.map((f) => f.id));
   if (fmIds.size !== jsonIds.size || [...fmIds].some((id) => !jsonIds.has(id))) {

tests/lib/unique-finding-ids.test.mjs

@@ -0,0 +1,207 @@
+// tests/lib/unique-finding-ids.test.mjs
+// [DUP] Every finding id in a review is unique. Two MAJORs that share one id
+// would let one bound `accepted` answer clear both, so the verdict gate
+// (`decideVerdict`), the answers check (`decideAnswers`) and the answer CLI
+// (`recordAnswer`) refuse duplicates: the gate and the check as "not
+// measured" (exit 2), the CLI by appending nothing.
+// `keep:` cases hold at the base already and stay untagged.
+// Modules load through `import * as m`, so a missing export is a failed
+// assertion at the red base, never a crash.
+
+import { after, test } from 'node:test';
+import { strict as assert } from 'node:assert';
+import { spawnSync } from 'node:child_process';
+import { appendFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { dirname, join } from 'node:path';
+import { fileURLToPath } from 'node:url';
+import * as gateMod from '../../lib/review/verdict-gate.mjs';
+import * as answersMod from '../../lib/review/review-answers.mjs';
+import { LOG_RELPATH } from '../../lib/util/approval-log.mjs';
+import { childEnv } from '../helpers/child-env.mjs';
+
+const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
+const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
+const ANSWERS = join(ROOT, 'lib', 'review', 'review-answers.mjs');
+const TRAILER = 'Co-Authored-By: Claude Test <noreply@example.invalid>';
+const ANS_REL = '.claude/projects/2026-11-03-d/review.md';
+const fid = (n) => String(n).padStart(40, 'b');
+const parse = (r) => { try { return JSON.parse(r.stdout); } catch { return {}; } };
+
+/**
+ * A review of `sha` (shape copied from tests/lib/review-answers.test.mjs
+ * `ansReview`). `findings` are [{severity, id?}]; an absent id is fid(i + 1).
+ * `fmIds` is the frontmatter `findings:` list (absent = the JSON ids, each
+ * once, in order).
+ */
+function dupReview({ sha = 'a'.repeat(40), verdict = 'WARN', findings = [{ severity: 'MAJOR' }], fmIds } = {}) {
+  const list = findings.map((f, i) => ({
+    id: f.id ?? fid(i + 1), severity: f.severity, rule_key: 'MISSING_TEST',
+    file: 'a.mjs', line: i + 1, title: `t${i + 1}`, detail: `d${i + 1}`, recommended_action: 'Add a test.',
+  }));
+  const counts = { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
+  for (const f of list) counts[f.severity] += 1;
+  const json = JSON.stringify({ verdict, counts, allow_blocked_by: [], findings: list }, null, 2);
+  const ids = fmIds ?? [...new Set(list.map((f) => f.id))];
+  return `---
+type: trekreview
+review_version: "1.2"
+task: "t"
+slug: s
+project_dir: .claude/projects/2026-11-03-d/
+brief_path: .claude/projects/2026-11-03-d/brief.md
+scope_sha_end: ${sha}
+reviewed_files_count: 1
+verdict: ${verdict}
+findings:
+${ids.map((id) => `  - ${id}`).join('\n')}
+---
+
+# Review: t
+
+## Executive Summary
+
+x
+
+## Coverage
+
+| File | Treatment | Reason |
+|------|-----------|--------|
+| a.mjs | deep-review | default |
+
+## Remediation Summary
+
+x
+
+## Remediation Steps
+
+None.
+
+## Declined to Judge
+
+None.
+
+\`\`\`json
+${json}
+\`\`\`
+`;
+}
+
+/** git in `cwd` (copied from tests/lib/review-answers.test.mjs `g`). */
+function g(cwd, ...args) {
+  const r = spawnSync('git', args, { cwd, encoding: 'utf8', env: childEnv() });
+  if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`);
+  return r.stdout.trim();
+}
+
+function putIn(dir, rel, text) {
+  mkdirSync(dirname(join(dir, rel)), { recursive: true });
+  writeFileSync(join(dir, rel), text);
+}
+
+const made = [];
+after(() => { for (const b of made) rmSync(b, { recursive: true, force: true }); });
+
+/**
+ * A throwaway repo with one reviewed commit and a review of it at ANS_REL
+ * (pattern of tests/lib/review-answers.test.mjs `newRepo` + `ansFixture`).
+ */
+function fixture(reviewOpts = {}) {
+  const base = mkdtempSync(join(tmpdir(), 'unique-finding-ids-'));
+  made.push(base);
+  const repo = join(base, 'repo');
+  mkdirSync(repo, { recursive: true });
+  mkdirSync(join(base, 'hooks-none'));
+  g(repo, 'init', '-q', '-b', 'main');
+  appendFileSync(join(repo, '.git', 'config'), '[user]\n\tname = Test\n\temail = test@example.invalid\n'
+    + `[commit]\n\tgpgsign = false\n[core]\n\thooksPath = "${join(base, 'hooks-none')}"\n`);
+  appendFileSync(join(repo, '.git', 'info', 'exclude'), '.claude/projects/\n');
+  putIn(repo, 'a.mjs', 'export const a = 1;\n');
+  g(repo, 'add', '-A');
+  g(repo, 'commit', '-q', '-m', 'feat: the work');
+  const reviewed = g(repo, 'rev-parse', 'HEAD');
+  const reviewPath = join(repo, ...ANS_REL.split('/'));
+  putIn(repo, ANS_REL, dupReview({ sha: reviewed, ...reviewOpts }));
+  const logFile = join(repo, ...LOG_RELPATH.split('/'));
+  return {
+    repo, reviewPath,
+    readLog: () => (existsSync(logFile) ? readFileSync(logFile, 'utf8').split('\n').filter(Boolean) : []),
+    read: () => readFileSync(reviewPath, 'utf8'),
+    record: ({ finding = fid(1), answer = 'accepted', reason = 'the old behaviour is kept on purpose' } = {}) => spawnSync(
+      process.execPath,
+      [ANSWERS, 'record', '--review', reviewPath, '--finding', finding, '--answer', answer,
+        '--repo', repo, '--round', '1', '--store', 'local', '--trailer', TRAILER],
+      { input: reason, encoding: 'utf8', env: childEnv() },
+    ),
+    gate: () => spawnSync(process.execPath, [GATE, '--json', reviewPath, '--repo', repo], { encoding: 'utf8', env: childEnv() }),
+  };
+}
+
+const TWO_MAJORS_ONE_ID = [{ severity: 'MAJOR', id: fid(1) }, { severity: 'MAJOR', id: fid(1) }];
+
+function assertNotMeasuredDup(reason) {
+  assert.match(String(reason), /duplicate finding id/, String(reason));
+  assert.doesNotMatch(String(reason), /^review-unanswered:/);
+  assert.doesNotMatch(String(reason), /^review-stale/);
+}
+
+test('[DUP] two MAJORs that share one id keep the gate at exit 2 although one accepted row is bound', () => {
+  const fx = fixture({ findings: TWO_MAJORS_ONE_ID });
+  // Succeeds at the base, refused after the fix: its exit is not the point.
+  fx.record();
+  const r = fx.gate();
+  assert.equal(r.status, 2, r.stdout + r.stderr);
+  assertNotMeasuredDup(parse(r).reason);
+  assert.match(String(parse(r).reason), new RegExp(`duplicate finding id ${fid(1)} in the trailing JSON block`));
+});
+
+test('[DUP] the frontmatter list with one id twice exits 2', () => {
+  assert.equal(typeof gateMod.decideVerdict, 'function');
+  const text = dupReview({ findings: [{ severity: 'MINOR' }], fmIds: [fid(1), fid(1)] });
+  const d = gateMod.decideVerdict(text);
+  assert.equal(d.exit, 2, JSON.stringify(d));
+  assertNotMeasuredDup(d.reason);
+  assert.equal(d.reason, `duplicate finding id ${fid(1)} in the frontmatter findings: list`);
+  assert.equal(d.verdict, 'WARN');
+});
+
+test('[DUP] decideAnswers refuses duplicate ids', () => {
+  assert.equal(typeof answersMod.decideAnswers, 'function');
+  const d = answersMod.decideAnswers(dupReview({ findings: TWO_MAJORS_ONE_ID }), {});
+  assert.deepEqual(d, {
+    ok: false, unanswered: [], fixedPending: [], reason: `review-not-measured: duplicate finding id ${fid(1)}`,
+  });
+});
+
+test('[DUP] record refuses an answer on a review with duplicate ids and appends nothing', () => {
+  const fx = fixture({ findings: TWO_MAJORS_ONE_ID });
+  const before = fx.read();
+  const r = fx.record();
+  assert.equal(r.status, 2, r.stdout + r.stderr);
+  assert.equal(parse(r).recorded, false, r.stdout);
+  assert.match(String(parse(r).reason), new RegExp(`holds a duplicate finding id ${fid(1)}; nothing appended`), r.stdout);
+  assert.deepEqual(fx.readLog(), []);
+  assert.equal(fx.read(), before);
+});
+
+test('keep: a BLOCK review with duplicate ids still exits 1', () => {
+  assert.equal(typeof gateMod.decideVerdict, 'function');
+  const d = gateMod.decideVerdict(dupReview({ verdict: 'BLOCK', findings: TWO_MAJORS_ONE_ID, fmIds: [fid(1), fid(1)] }));
+  assert.equal(d.exit, 1, JSON.stringify(d));
+  const fx = fixture({ verdict: 'BLOCK', findings: TWO_MAJORS_ONE_ID });
+  const r = fx.gate();
+  assert.equal(r.status, 1, r.stdout + r.stderr);
+});
+
+test('keep: a review with unique ids decides as before', () => {
+  assert.equal(typeof gateMod.decideVerdict, 'function');
+  assert.equal(gateMod.decideVerdict(dupReview({ findings: [{ severity: 'MINOR' }, { severity: 'MINOR' }] })).exit, 0);
+  const fx = fixture({ findings: [{ severity: 'MAJOR' }, { severity: 'MINOR' }] });
+  const open = fx.gate();
+  assert.equal(open.status, 2, open.stdout + open.stderr);
+  assert.match(String(parse(open).reason), /^review-unanswered: 1 MAJOR without an answer/);
+  const rec = fx.record();
+  assert.equal(rec.status, 0, rec.stdout + rec.stderr);
+  const r = fx.gate();
+  assert.equal(r.status, 0, r.stdout + r.stderr);
+});