fix(review): one reviewer's two findings on one triplet keep a row each

Pass 1 merged two different /code-review findings on (bin/slug.mjs, 46, CODE_REVIEW_DEFECT) and the second lost its row and its action. A reviewer never duplicates itself: its second finding on a triplet under another title stays its own row, with the id over rule_key#2 (the form critic-diff already uses); fill-finding-ids counts the same way (voyage-lekerepo trial 5, finding 7).

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

agents/review-coordinator.md

@@ -76,7 +76,11 @@ re-reading, and fresh finding generation are all forbidden.
 ### Pass 1 — Dedup by `(file, line, rule_key)` triplet
 
 Two findings collide when their `(file, line, rule_key)` triplets are
-identical. When findings collide:
+identical, with one exception: **one reviewer never duplicates itself.** Two
+findings from the same reviewer on one triplet under different titles are two
+findings (an engine's catch-all `rule_key` makes this common): keep both rows,
+in input order. A finding from another reviewer joins the row whose title it
+shares, else the first one. When findings collide:
 - Rank the colliding findings and keep the first:
   1. the higher catalogue tier: the severity `lib/review/rule-catalogue.mjs`
      gives the `rule_key`, or the finding's own severity when the key is
@@ -420,8 +424,9 @@ for the ID list.
   where n is its 1-based position in the JSON block's `findings` array.
   `/trekreview` Phase 7 runs `lib/review/fill-finding-ids.mjs`, which
   replaces each placeholder with the id computed over `(file, line,
-  rule_key)` (`lib/parsers/finding-id.mjs`) and refuses a placeholder with
-  no finding behind it.
+  rule_key)` (`lib/parsers/finding-id.mjs`; the k-th row on one triplet
+  gets it over `rule_key#k`) and refuses a placeholder with no finding
+  behind it.
 - **Suppressed findings are accountable.** When you remove a finding via
   Pass 2 or Pass 3, log it in `## Suppressed Findings` with the reason and
   its fate (`[dropped]` / `[unverified]`), and list every `[unverified]` one

lib/parsers/finding-id.mjs

@@ -1,7 +1,8 @@
 // lib/parsers/finding-id.mjs
 // Stable finding-ID for /trekreview v1.0.
 //
-// id = sha1(file:line:rule_key) → 40-char hex.
+// id = sha1(file:line:rule_key) → 40-char hex; a second finding on the same
+// triplet gets it over `rule_key#2` (computeFindingIds).
 // Same input always produces same output (determinism floor SC4).
 // node:crypto is built-in (zero-deps invariant).
 
@@ -43,6 +44,36 @@ export function computeFindingId(filePath, line, ruleKey) {
   return createHash('sha1').update(composite).digest('hex');
 }
 
+/**
+ * The ids of a list of findings, in order. The first finding on a
+ * (file, line, rule_key) gets computeFindingId of it, as always; the k-th
+ * (k >= 2) gets it over `${rule_key}#k`, the form critic-diff.mjs already uses,
+ * so two different findings one reviewer put on one triplet keep a row each
+ * (voyage-lekerepo trial 5, finding 7).
+ * @param {Array<{file: string, line: number|string, rule_key: string}>} findings
+ * @param {(index: number, error: Error) => void} [onError] - when given, a
+ *   finding whose id cannot be computed gets null and is reported here;
+ *   without it the TypeError is thrown
+ * @returns {Array<string|null>} one id per finding
+ */
+export function computeFindingIds(findings, onError) {
+  const seen = new Map();
+  return findings.map((f, i) => {
+    const key = `${f?.file}:${f?.line}:${f?.rule_key}`;
+    const k = (seen.get(key) ?? 0) + 1;
+    seen.set(key, k);
+    try {
+      // A missing rule_key must still throw, never become "undefined#2".
+      const ruleKey = k === 1 || typeof f?.rule_key !== 'string' || f.rule_key === '' ? f?.rule_key : `${f.rule_key}#${k}`;
+      return computeFindingId(f?.file, f?.line, ruleKey);
+    } catch (e) {
+      if (!onError) throw e;
+      onError(i, e);
+      return null;
+    }
+  });
+}
+
 /**
  * Validate a finding-ID's shape (40-char lowercase hex).
  * @param {string} id

lib/review/coordinator-contract.mjs

@@ -12,7 +12,8 @@
 //     contract being mirrored, so only the deterministic "recommended_action is
 //     present-and-non-empty when supplied" half is kept.
 //   - Nothing about ids beyond the triplet: `computeFindingId(file, line,
-//     rule_key)` is the one id. The LLM coordinator writes `@@ID_<n>@@`
+//     rule_key)` is the one id, over `rule_key#k` for the k-th row on one
+//     triplet (computeFindingIds). The LLM coordinator writes `@@ID_<n>@@`
 //     placeholders and lib/review/fill-finding-ids.mjs computes them (the
 //     old doc named a 4-tuple with `title` the code never computed).
 //
@@ -32,13 +33,13 @@
 // classifySuppression). No LLM, no network, no time, no randomness.
 //
 // Reuses: SEVERITY_VALUES / RULE_KEYS / getRule (rule-catalogue.mjs),
-// computeFindingId (finding-id.mjs, triplet), validateFindings
+// computeFindingIds (finding-id.mjs, triplet), validateFindings
 // (findings-schema.mjs). Triplet key format mirrors
 // scripts/bakeoff-armA-merge.mjs `dedupByTriplet`; raised_by provenance mirrors
 // lib/review/plan-review-dedup.mjs.
 
 import { SEVERITY_VALUES, RULE_KEYS, getRule } from './rule-catalogue.mjs';
-import { computeFindingId } from '../parsers/finding-id.mjs';
+import { computeFindingIds } from '../parsers/finding-id.mjs';
 import { validateFindings } from './findings-schema.mjs';
 import { ENGINES } from './engine-findings.mjs';
 import { isMain } from '../util/is-main.mjs';
@@ -259,14 +260,28 @@ export function ingest(reviewerPayloads) {
  * @returns {object[]}
  */
 export function dedupByTriplet(findings) {
-  const groups = new Map();
+  // One reviewer never duplicates itself: its second finding on a triplet under
+  // another title is another finding and opens a group of its own, which gets
+  // its own row and the id over `rule_key#2` (voyage-lekerepo trial 5,
+  // finding 7). Another reviewer's finding joins the group whose title it
+  // shares, else the first one it can join.
+  const tripletGroups = new Map();
   for (const f of findings) {
     const key = tripletKey(f);
-    if (!groups.has(key)) groups.set(key, []);
-    groups.get(key).push(f);
+    if (!tripletGroups.has(key)) tripletGroups.set(key, []);
+    const subs = tripletGroups.get(key);
+    const fits = (g) => g.every((m) => m.reviewer !== f.reviewer || sameTitle(m, f));
+    let sub = subs.find((g) => fits(g) && g.some((m) => sameTitle(m, f) && m.title !== undefined && f.title !== undefined))
+      ?? subs.find(fits);
+    if (!sub) {
+      sub = [];
+      subs.push(sub);
+    }
+    sub.push(f);
   }
+  const groups = [...tripletGroups.values()].flat();
   const out = [];
-  for (const group of groups.values()) {
+  for (const group of groups) {
     // Ranked as the JSDoc above says. A brief-conflict mark is not carried over
     // from a group member to the survivor: when the survivor is unmarked, the
     // group stays unmarked and is judged at its full severity. For a catalogue
@@ -307,17 +322,24 @@ export function dedupByTriplet(findings) {
         detail += `\nAlso flagged by ${o.reviewer ?? 'unknown'}: ${o.title ?? o.rule_key}.`;
       }
     }
-    const id = computeFindingId(survivor.file, survivor.line, survivor.rule_key);
     // When the group raised the survivor, Pass 3 still needs what the survivor
     // itself asserted for `original_severity` (review 08deae68).
     const raised = severityRank(severity) < severityRank(survivor.severity);
     out.push({
-      ...survivor, ...(severity !== undefined ? { severity } : {}), id,
+      ...survivor, ...(severity !== undefined ? { severity } : {}),
       ...(detail !== undefined ? { detail } : {}), raised_by,
       ...(raised ? { pass1_raised_from: survivor.severity } : {}),
     });
   }
-  return out;
+  // The groups of one triplet sit next to each other, so the k-th row on a
+  // triplet here is the k-th in review.md, where fill-finding-ids counts the same way.
+  const ids = computeFindingIds(out);
+  return out.map((f, i) => ({ ...f, id: ids[i] }));
+}
+
+function sameTitle(a, b) {
+  if (a.title === undefined || b.title === undefined) return true;
+  return String(a.title).trim().toLowerCase() === String(b.title).trim().toLowerCase();
 }
 
 /**

lib/review/fill-finding-ids.mjs

@@ -7,7 +7,8 @@
 // step no document named. Now the contract says it: the coordinator writes
 // `@@ID_<n>@@` wherever finding n's id belongs (n = 1-based position in the
 // trailing JSON block's `findings` array), and this tool replaces every
-// placeholder with computeFindingId(file, line, rule_key) of that finding.
+// placeholder with computeFindingId(file, line, rule_key) of that finding
+// (`rule_key#k` for the k-th finding on one triplet: computeFindingIds).
 //
 // Fail-closed: a placeholder with no JSON finding behind it, or a 40-hex id
 // in the JSON block that is not the computed one, is an error -- never a guess.
@@ -19,7 +20,7 @@
 //   2 = could not run (unreadable file, no JSON block).
 
 import { readFileSync, writeFileSync, renameSync } from 'node:fs';
-import { computeFindingId } from '../parsers/finding-id.mjs';
+import { computeFindingIds } from '../parsers/finding-id.mjs';
 import { extractFindingsBlock } from './findings-schema.mjs';
 import { isMain } from '../util/is-main.mjs';
 
@@ -41,13 +42,8 @@ export function fillFindingIds(text) {
   }
   const findings = Array.isArray(parsed?.findings) ? parsed.findings : [];
   const errors = [];
-  const ids = findings.map((f, i) => {
-    try {
-      return computeFindingId(f.file, f.line, f.rule_key);
-    } catch (e) {
-      errors.push(`findings[${i}]: cannot compute an id (${e.message})`);
-      return null;
-    }
+  const ids = computeFindingIds(findings, (i, e) => {
+    errors.push(`findings[${i}]: cannot compute an id (${e.message})`);
   });
   findings.forEach((f, i) => {
     if (typeof f.id === 'string' && HEX40.test(f.id) && ids[i] && f.id !== ids[i]) {

tests/lib/coordinator-contract.test.mjs

@@ -9,6 +9,7 @@ import { strict as assert } from 'node:assert';
 import { readFileSync } from 'node:fs';
 import { join, dirname } from 'node:path';
 import { fileURLToPath } from 'node:url';
+import { computeFindingId } from '../../lib/parsers/finding-id.mjs';
 import {
   severityRank,
   ingest,
@@ -58,6 +59,21 @@ test('dedupByTriplet — survivor is the highest-severity finding in the group',
   assert.equal(out[0].severity, 'BLOCKER');
 });
 
+test('dedupByTriplet — two DIFFERENT findings from one reviewer on one triplet stay two rows with their own ids (trial 5, finding 7)', () => {
+  // /code-review gave both the catch-all key at bin/slug.mjs:46; one merged row lost the second finding's action.
+  const findings = [
+    { file: 'bin/slug.mjs', line: 46, rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR', title: 'EAGAIN on a non-blocking stdin', recommended_action: 'Read stdin with a stream.', reviewer: 'claude-code-review' },
+    { file: 'bin/slug.mjs', line: 46, rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR', title: 'A lone \\r does not split lines', recommended_action: 'Split on \\r too.', reviewer: 'claude-code-review' },
+    { file: 'bin/slug.mjs', line: 46, rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR', title: 'EAGAIN on a non-blocking stdin', recommended_action: 'Read stdin with a stream.', reviewer: 'code-correctness-reviewer' },
+  ];
+  const out = dedupByTriplet(findings);
+  assert.equal(out.length, 2, JSON.stringify(out.map((f) => f.title)));
+  assert.deepEqual(out.map((f) => f.recommended_action), ['Read stdin with a stream.', 'Split on \\r too.']);
+  assert.equal(out[0].id, computeFindingId('bin/slug.mjs', 46, 'CODE_REVIEW_DEFECT'));
+  assert.equal(out[1].id, computeFindingId('bin/slug.mjs', 46, 'CODE_REVIEW_DEFECT#2'));
+  assert.deepEqual([...out[0].raised_by].sort(), ['claude-code-review', 'code-correctness-reviewer']);
+});
+
 test('dedupByTriplet — severity tie breaks toward the conformance reviewer', () => {
   const findings = [
     { file: 'x.mjs', line: 1, rule_key: 'NON_GOAL_VIOLATED', severity: 'BLOCKER', title: 'corr', reviewer: 'correctness' },
@@ -403,3 +419,11 @@ test('runContract — a BLOCKER with a 900-character detail survives and yields
   assert.equal(result.findings[0].detail, detail, 'its detail is not cut');
   assert.equal(result.unverified.length, 0);
 });
+
+test('the coordinator prose says one reviewer never duplicates itself, as dedupByTriplet does (trial 5, finding 7)', () => {
+  const prose = readFileSync(join(ROOT, 'agents/review-coordinator.md'), 'utf8');
+  const pass1 = prose.slice(prose.indexOf('### Pass 1'), prose.indexOf('### Pass 2'));
+  assert.match(pass1, /one reviewer never duplicates itself/i);
+  assert.match(pass1, /different titles are two\s+findings/);
+  assert.match(prose, /k-th row on one triplet\s+gets it over `rule_key#k`/);
+});

tests/lib/review-findings-kept.test.mjs

@@ -145,6 +145,17 @@ test('fill-finding-ids: every @@ID_n@@ becomes the id computed over (file, line,
   assert.match(r.text, new RegExp(`"id": "${id1}"`));
 });
 
+test('fill-finding-ids: a second finding on the same (file, line, rule_key) gets its own id, `#2` on the rule (trial 5, finding 7)', () => {
+  const G = { ...F1, title: 'A lone \\r does not split lines' };
+  const r = fillFindingIds(review([F1, G]));
+  assert.equal(r.ok, true, r.errors.join('\n'));
+  const id1 = computeFindingId(F1.file, F1.line, F1.rule_key);
+  const id2 = computeFindingId(F1.file, F1.line, `${F1.rule_key}#2`);
+  assert.notEqual(id1, id2);
+  assert.match(r.text, new RegExp(`findings:\\n  - ${id1}\\n  - ${id2}\\n`));
+  assert.equal(fillFindingIds(r.text).ok, true, 'the filled review checks out again');
+});
+
 test('fill-finding-ids: a placeholder with no JSON finding behind it is an error, not a guess', () => {
   const r = fillFindingIds(review([F1], 'See also @@ID_7@@.'));
   assert.equal(r.ok, false);