fix(review): Pass 1 ranks by catalogue tier, and a duplicate never lowers its group

Review finding bc7f4e23 (library side).

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

agents/review-coordinator.md

@@ -88,13 +88,21 @@ re-reading, and fresh finding generation are all forbidden.
 
 Two findings collide when their `(file, line, rule_key)` triplets are
 identical. When findings collide:
-- Keep the finding with the highest catalogue severity (BLOCKER >
-  MAJOR > MINOR > SUGGESTION).
-- If the severity ties, prefer a reviewer-agent finding over an engine
-  finding (one from `claude-code-review` / `claude-security-review`), so an
-  engine's brief-conflict mark never lowers what a reviewer agent asserted;
-  among reviewer-agent findings, prefer the finding from
-  `brief-conformance-reviewer` (its findings are anchored to the brief).
+- 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
+     not in the catalogue (BLOCKER > MAJOR > MINOR > SUGGESTION);
+  2. if the catalogue tier ties (for a catalogue key it always does,
+     since the triplet shares the `rule_key`), prefer a reviewer-agent
+     finding over an engine finding (one from `claude-code-review` /
+     `claude-security-review`);
+  3. among findings of one kind, the higher asserted severity;
+  4. among reviewer-agent findings, prefer the finding from
+     `brief-conformance-reviewer` (its findings are anchored to the brief);
+  5. then the first seen.
+- Write the kept finding with the highest severity any finding in its
+  group asserted, so dropping a duplicate never lowers the group and an
+  engine's brief-conflict mark never lowers what a reviewer agent asserted.
 - Concatenate the kept finding's `detail` with a one-line note: "Also
   flagged by {other reviewer}: {their title}." This preserves
   attribution without duplicating the row.
@@ -408,8 +416,9 @@ for the ID list.
 - **Severity floor is the catalogue.** It holds for every finding
   except a lowerable brief conflict, which is MINOR (Pass 3, `brief_lowered_from: MAJOR`).
   Pass 3 corrects mismatches by
-  resetting to the catalogue tier — never by dropping. Pass 1's severity
-  tiebreak uses the catalogue tier, not the reviewer's emitted value.
+  resetting to the catalogue tier — never by dropping. Pass 1 ranks by
+  the catalogue tier first, in the order Pass 1 gives, and the kept finding
+  carries the highest severity its group asserted.
 - **Block-style YAML for findings list.** The frontmatter parser
   (`lib/util/frontmatter.mjs`) does not support flow-style arrays.
 - **IDs are computed by code, not by you.** You have no shell, so you

docs/HANDOVER-CONTRACTS.md

@@ -476,7 +476,7 @@ and an engine finding below its tier, marked or not (a BLOCK review exits 1 as b
 The gate cannot check an `engine` label on the prose path: there the coordinator writes the label, so a finding wrongly labelled `engine` is judged as an engine finding;
 the workflow path (`--workflow`) takes `engine` from the payload source and writes it into the returned finding.
 `runContract` (`lib/review/coordinator-contract.mjs`) likewise takes `engine` from the payload a finding arrived in, never from the finding's own field,
-and on a severity tie in dedup a reviewer-agent finding survives over an engine finding (both paths), so an engine's mark never lowers what a reviewer agent asserted.
+and in dedup the survivor is ranked by catalogue tier (for a catalogue key it always ties inside one triplet), so a reviewer-agent finding survives over an engine finding and carries the highest severity the group asserted; an engine's mark never lowers what a reviewer agent asserted.
 A plan made from the review skips a lowered finding: it is MINOR, and `/trekplan` takes BLOCKER and MAJOR findings only.
 
 **Validation strategy:**

lib/review/coordinator-contract.mjs

@@ -16,8 +16,10 @@
 //     placeholders and lib/review/fill-finding-ids.mjs computes them (the
 //     old doc named a 4-tuple with `title` the code never computed).
 //
-// What IS implemented, purely: Pass 1 (triplet dedup → highest-severity-wins
-// survivor + conformance tiebreak + detail concat + raised_by provenance),
+// What IS implemented, purely: Pass 1 (triplet dedup → survivor ranked by
+// catalogue tier, then reviewer over engine, asserted severity, conformance,
+// first seen; the survivor carries its group's highest asserted severity
+// (review bc7f4e23) + detail concat + raised_by provenance),
 // Pass 2 succinctness (trims MINOR/SUGGESTION text, never removes) +
 // actionability-presence, Pass 3 reasonableness
 // (citation / unknown-rule_key suppression, severity-mismatch correction),
@@ -238,11 +240,16 @@ export function ingest(reviewerPayloads) {
 }
 
 /**
- * Pass 1 — dedup by (file, line, rule_key) triplet. Survivor = highest
- * catalogue severity; severity tie → prefer a reviewer-agent finding over an
- * engine finding, then the conformance reviewer; carries
- * raised_by provenance, concatenates other reviewers' attribution into detail,
- * and recomputes the id over the triplet.
+ * Pass 1 — dedup by (file, line, rule_key) triplet. The survivor is ranked by
+ * (1) the catalogue tier (`getRule(rule_key)?.severity`, else the finding's own
+ * severity; lower `severityRank` wins), (2) a reviewer-agent finding over an
+ * engine finding, (3) among findings of one kind the higher asserted severity,
+ * (4) the conformance reviewer, (5) first seen. It is written with the highest
+ * severity any finding in its group asserted, so dropping a duplicate never
+ * lowers the group (review bc7f4e23; plan 2026-10-11-files-backticks-trial4,
+ * ADR "Pass 1 survivor order and group severity"). Carries raised_by
+ * provenance, concatenates other reviewers' attribution into detail, and
+ * recomputes the id over the triplet.
  * @param {object[]} findings
  * @returns {object[]}
  */
@@ -256,21 +263,43 @@ export function dedupByTriplet(findings) {
   const out = [];
   for (const group of groups.values()) {
     // A brief-conflict mark is not carried over from a group member to the
-    // survivor: when the higher-severity survivor is unmarked, the group stays
-    // unmarked and is judged at its full severity. On a severity tie a
-    // reviewer-agent finding survives over an engine finding (decision 13,
-    // operator 12.10), so an engine's mark never lowers what a reviewer agent
-    // asserted, whatever the payload order; among findings of one kind the
-    // conformance reviewer wins the tie, else the first-seen one stays.
+    // survivor: when the survivor is unmarked, the group stays unmarked and is
+    // judged at its full severity. The survivor is ranked by catalogue tier
+    // first; for a catalogue key the tier always ties inside one triplet (the
+    // rule_key is shared), so a reviewer-agent finding survives over an engine
+    // finding (decision 13, operator 12.10; review bc7f4e23), then the higher
+    // asserted severity among findings of one kind, then the conformance
+    // reviewer, else the first-seen one stays. The survivor is written with the
+    // highest severity any finding in its group asserted, so an engine's mark
+    // never lowers what a reviewer agent asserted, whatever the payload order
+    // (plan 2026-10-11-files-backticks-trial4, ADR "Pass 1 survivor order and
+    // group severity").
+    const tier = (f) => severityRank(getRule(f.rule_key)?.severity ?? f.severity);
     let survivor = group[0];
     for (const f of group.slice(1)) {
+      const tf = tier(f);
+      const ts = tier(survivor);
+      if (tf !== ts) {
+        if (tf < ts) survivor = f;
+        continue;
+      }
+      const ef = isEngineFinding(f);
+      const es = isEngineFinding(survivor);
+      if (ef !== es) {
+        if (!ef) survivor = f;
+        continue;
+      }
       const rf = severityRank(f.severity);
       const rs = severityRank(survivor.severity);
-      const tieToReviewer = rf === rs && !isEngineFinding(f) && isEngineFinding(survivor);
-      const tieToConformance =
-        rf === rs && isEngineFinding(f) === isEngineFinding(survivor) &&
-        isConformance(f.reviewer) && !isConformance(survivor.reviewer);
-      if (rf < rs || tieToReviewer || tieToConformance) survivor = f;
+      if (rf !== rs) {
+        if (rf < rs) survivor = f;
+        continue;
+      }
+      if (isConformance(f.reviewer) && !isConformance(survivor.reviewer)) survivor = f;
+    }
+    let severity = survivor.severity;
+    for (const f of group) {
+      if (severityRank(f.severity) < severityRank(severity)) severity = f.severity;
     }
     const raised_by = [...new Set(group.map((f) => f.reviewer).filter(Boolean))];
     const others = group.filter((f) => f !== survivor);
@@ -282,7 +311,10 @@ export function dedupByTriplet(findings) {
       }
     }
     const id = computeFindingId(survivor.file, survivor.line, survivor.rule_key);
-    out.push({ ...survivor, id, ...(detail !== undefined ? { detail } : {}), raised_by });
+    out.push({
+      ...survivor, ...(severity !== undefined ? { severity } : {}), id,
+      ...(detail !== undefined ? { detail } : {}), raised_by,
+    });
   }
   return out;
 }

tests/review/brief-conflict-findings.test.mjs

@@ -463,9 +463,12 @@ test('Handover 6 — names the fields, engine findings only, the one lowering an
 // ---- Decision 13 (operator 12.10): the source decides, a tie keeps the reviewer --
 //
 // "Came from an engine" is read from the payload a finding arrived in, never
-// from an `engine` field a reviewer wrote on its own finding; and on a severity
-// tie a reviewer-agent finding survives over an engine finding, so an engine's
-// mark can never lower what a reviewer agent asserted, whatever the order.
+// from an `engine` field a reviewer wrote on its own finding; and Pass 1 ranks
+// a triplet's findings by catalogue tier first (for a catalogue key the tier
+// always ties inside one triplet), then a reviewer-agent finding over an engine
+// finding, and the survivor carries the highest severity its group asserted,
+// so an engine's mark can never lower what a reviewer agent asserted, whatever
+// the order (review bc7f4e23).
 
 function reviewerCopy(extra = {}) {
   const { engine: _e, ...rest } = codeReviewDefect({ title: 'Reviewer: no retry', ...extra });
@@ -513,18 +516,36 @@ for (const order of ['reviewer first', 'engine first']) {
   });
 }
 
-test('decision 13 — an engine MAJOR over a reviewer MINOR on the same triplet still drops to MINOR (the engine wins on severity)', () => {
-  const result = m.runContract([
-    { reviewer: 'code-correctness-reviewer', findings: [reviewerCopy({ rule_key: 'MISSING_TEST', severity: 'MINOR' })] },
-    { reviewer: 'claude-code-review', findings: [codeReviewDefect({ rule_key: 'MISSING_TEST', contradicts_brief: NON_GOAL })] },
-  ]);
-  const out = result.findings[0];
-  assert.equal(out.engine, 'code-review');
-  assert.equal(out.severity, 'MINOR');
-  assert.equal(out.brief_lowered_from, 'MAJOR');
-  assert.equal(result.verdict, 'ALLOW');
-  assertAllOutputLegal(result);
-});
+function minorReviewerAndMarkedEngine(order) {
+  const reviewerPayload = { reviewer: 'code-correctness-reviewer', findings: [reviewerCopy({ rule_key: 'MISSING_TEST', severity: 'MINOR' })] };
+  const enginePayload = { reviewer: 'claude-code-review', findings: [codeReviewDefect({ rule_key: 'MISSING_TEST', contradicts_brief: NON_GOAL })] };
+  return order === 'reviewer first' ? [reviewerPayload, enginePayload] : [enginePayload, reviewerPayload];
+}
+
+for (const order of ['reviewer first', 'engine first']) {
+  test(`bc7f4e23 — a marked /code-review MAJOR over a reviewer MINOR on the same triplet keeps the reviewer at MAJOR (${order}) → WARN`, () => {
+    const result = m.runContract(minorReviewerAndMarkedEngine(order));
+    assert.equal(result.findings.length, 1);
+    const out = result.findings[0];
+    assert.equal('engine' in out, false, 'the survivor is the reviewer finding');
+    assert.equal(out.severity, 'MAJOR', 'the group asserted MAJOR; dropping the engine duplicate never lowers it');
+    assert.equal('brief_lowered_from' in out, false, 'the engine mark never lowers what a reviewer agent asserted');
+    assert.equal('contradicts_brief' in out, false);
+    assert.equal(result.verdict, 'WARN');
+    assertAllOutputLegal(result);
+  });
+
+  test(`bc7f4e23 — dedupByTriplet(ingest(...)) keeps the reviewer finding at the group's MAJOR over a marked /code-review MAJOR (${order})`, () => {
+    const { findings } = m.ingest(minorReviewerAndMarkedEngine(order));
+    const out = m.dedupByTriplet(findings);
+    assert.equal(out.length, 1);
+    assert.equal('engine' in out[0], false, 'the reviewer-agent finding survives the tier tie');
+    assert.equal(out[0].reviewer, 'code-correctness-reviewer');
+    assert.equal(out[0].severity, 'MAJOR', 'the survivor carries the highest severity its group asserted');
+    assert.equal('contradicts_brief' in out[0], false, 'the engine mark is not carried to the survivor');
+    assert.deepEqual([...out[0].raised_by].sort(), ['claude-code-review', 'code-correctness-reviewer']);
+  });
+}
 
 test('decision 13 — the prose states the tie rule (coordinator Pass 1, Handover 6)', () => {
   const coordinator = readDoc('agents', 'review-coordinator.md');
@@ -534,7 +555,34 @@ test('decision 13 — the prose states the tie rule (coordinator Pass 1, Handove
   assert.match(pass1, /never\s+lowers\s+what\s+a\s+reviewer\s+agent\s+asserted/);
   const h6 = cut(readDoc('docs', 'HANDOVER-CONTRACTS.md'), '## Handover 6 — `review.md` → plan', '\n## Handover 7', 'Handover 6');
   assert.match(h6, /takes? `engine` from the payload a finding arrived in/);
-  assert.match(h6, /on a severity tie[^.]*a reviewer-agent finding survives[^.]*over an engine finding/i);
+  assert.match(h6, /ranked by catalogue tier[^.]*a reviewer-agent finding survives over an engine finding[^.]*highest severity the group asserted/i,
+    'Handover 6: Pass 1 ranks by catalogue tier and the survivor carries the group severity (bc7f4e23)');
+});
+
+test('b406a628 — coordinator Pass 1 ranks in the library order and writes the group severity (decision 24)', () => {
+  const coordinator = readDoc('agents', 'review-coordinator.md');
+  const pass1 = cut(coordinator, '### Pass 1', '### Pass 2', 'coordinator Pass 1');
+  assert.doesNotMatch(pass1, /highest\s+catalogue\s+severity/, 'Pass 1 no longer keeps the highest catalogue severity');
+  const ranks = [
+    /higher\s+catalogue\s+tier/,
+    /prefer\s+a\s+reviewer-agent\s+finding\s+over\s+an\s+engine\s+finding/,
+    /higher\s+asserted\s+severity/,
+    /brief-conformance-reviewer/,
+    /first\s+seen/,
+  ];
+  let previous = -1;
+  for (const rank of ranks) {
+    const at = pass1.search(rank);
+    assert.ok(at >= 0, `Pass 1 names the rank ${rank}`);
+    assert.ok(at > previous, `Pass 1 names the rank ${rank} after the one before it`);
+    previous = at;
+  }
+  assert.match(pass1, /not\s+in\s+the\s+catalogue/, 'Pass 1 names the unknown-key fallback');
+  assert.match(pass1, /highest\s+severity\s+any\s+finding\s+in\s+its\s+group\s+asserted/,
+    'Pass 1 writes the group severity');
+  assert.doesNotMatch(coordinator, /not\s+the\s+reviewer's\s+emitted\s+value/,
+    'no Hard rule still says the tiebreak ignores the emitted value');
+  assert.match(coordinator, /Pass\s+1\s+ranks\s+by\s+the\s+catalogue\s+tier\s+first/);
 });
 
 // ---- The engine of a payload is read from an own property (review 3799ea6c) --