fix(review): the workflow review ranks by tier and puts back every engine finding

Operator decision 23 (13.10); review findings ab1b5d9f and bc7f4e23 (workflow side).

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

commands/trekreview.md

@@ -519,8 +519,9 @@ block. Do NOT re-run the reviewers based on the coordinator's output.
 ## Phase 5–6 via the Workflow substrate (opt-in `--workflow`)
 
 Runs **only** when `workflow_substrate == true`. This is the **NW2 port**: it expresses
-the SAME Phase 5–6 pipeline (parallel reviewers → triplet-dedup → coordinator verdict) as
-a single Workflow, reusing the NW1 findings schema. The S10 bake-off found it
+the same Phase 5–6 pipeline shape (parallel reviewers → triplet-dedup → coordinator verdict)
+as a single Workflow, reusing the NW1 findings schema; its verdict floor is stricter than
+the prose path's (decision 23, below). The S10 bake-off found it
 **fidelity-equivalent** to the prose path — see `docs/T2-bakeoff-results.md` (verdict
 **POSITIVE**: verdict-match 1.0, issue-coverage 100%, `(file,rule_key)` jaccard ≥
 within-arm, tokens +4.4%). It stays **opt-in**, not the default, because the Workflow tool
@@ -545,6 +546,11 @@ Workflow({
 
 A reviewer that returns nothing, or an engine listed in `engineNotRun`,
 forbids `ALLOW` on this path too (`missing_reviewers` in the result).
+On this path dedup ranks as Pass 1 does (both paths), every engine finding the
+coordinator did not return at its exact `(file, line, rule_key)` is put back at its own
+severity (operator decision 23), and a returned finding is never below the lower of its
+reviewer counterpart's severity and its catalogue tier (floor (b)), so the workflow path
+is stricter than the prose path.
 
 Contract (verified in S10 part B — follow exactly):
 - **Pass `args` as a JSON object.** The script defensively re-parses a JSON *string*, but

docs/HANDOVER-CONTRACTS.md

@@ -477,6 +477,7 @@ The gate cannot check an `engine` label on the prose path: there the coordinator
 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 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.
+On the workflow path dedup ranks as Pass 1 does (both paths), every engine finding the coordinator did not return at its exact `(file, line, rule_key)` is put back at its own severity (operator decision 23), and a returned finding is never below the lower of its reviewer counterpart's severity and its catalogue tier (floor (b)), so the workflow path is stricter than the prose path.
 A plan made from the review skips a lowered finding: it is MINOR, and `/trekplan` takes BLOCKER and MAJOR findings only.
 
 **Validation strategy:**

scripts/bakeoff-armA-merge.mjs

@@ -10,9 +10,10 @@
 //   for each reviewer raw-output file:
 //     validateReviewerOutput()  (NW1 — extract last json fence, parse, schema-check)
 //   collect findings from VALID reviewers
-//   triplet-dedup by (file,line,rule_key)   [SAME logic as Arm B's dedupByTriplet,
-//                                             so the two arms dedup identically —
-//                                             triplet-only, no jaccard pass-2]
+//   triplet-dedup by (file,line,rule_key)   [same key as Arm B's dedupByTriplet,
+//                                             first-wins here; Arm B ranks by tier
+//                                             and keeps the reviewer finding on a
+//                                             tie — triplet-only, no jaccard pass-2]
 //   → emit { merged, validation, raw_finding_count, deduped_count }
 //
 // The merged findings are then handed to the review-coordinator (spawned by main),
@@ -28,9 +29,11 @@ import { readFileSync, existsSync } from 'node:fs';
 import { validateReviewerOutput } from '../lib/review/findings-schema.mjs';
 import { isMain } from '../lib/util/is-main.mjs';
 
-// Triplet dedup — byte-for-byte the same key + first-wins policy as
-// scripts/trekreview-armB.workflow.mjs `dedupByTriplet`, so neither arm gets a
-// dedup advantage. Triplet-only (file,line,rule_key); NO jaccard pass-2.
+// Triplet dedup — the same (file,line,rule_key) key as
+// scripts/trekreview-armB.workflow.mjs `dedupByTriplet`, but not the same
+// policy: Arm A keeps the first-seen finding, while Arm B ranks by catalogue
+// tier and keeps the reviewer finding on a tie (review bc7f4e23). Triplet-only;
+// NO jaccard pass-2.
 export function dedupByTriplet(findings) {
   const seen = new Map();
   for (const f of findings) {

scripts/trekreview-armB.workflow.mjs

@@ -114,22 +114,47 @@ const VERDICT_SCHEMA = {
 
 // Inlined dedup by (file,line,rule_key) — the JS-side replacement for the
 // coordinator's pass-1 triplet dedup (T2 §5 / lib/review/plan-review-dedup.mjs
-// pattern; scripts cannot import, so the triplet key is inlined). On a
-// repeated triplet the higher severity stays; at equal severity a reviewer
-// finding (no `engine`) stays over an engine finding, so an engine's mark never
-// lowers what a reviewer agent asserted (decision 13, operator 12.10, as
-// coordinator-contract Pass 1); otherwise the first-seen one stays.
+// pattern; scripts cannot import, so the triplet key is inlined). The survivor
+// order is coordinator-contract Pass 1's (review bc7f4e23): the catalogue tier
+// first (for a catalogue key it always ties inside one triplet, since the
+// rule_key is shared), then a reviewer finding (no `engine`) over an engine
+// finding (decision 13, operator 12.10), then the higher asserted severity;
+// the workflow has no conformance rule, so otherwise the first-seen one stays.
+// The survivor carries the highest severity any finding in its group asserted,
+// so an engine's mark never lowers what a reviewer agent asserted.
 function dedupByTriplet(findings) {
-  const seen = new Map();
+  const groups = new Map();
   for (const f of findings) {
     const key = `${f.file}\u0000${f.line}\u0000${f.rule_key}`;
-    if (!seen.has(key)) { seen.set(key, f); continue; }
-    const kept = seen.get(key);
-    const higher = sevRank(f.severity) < sevRank(kept.severity);
-    const tieToReviewer = sevRank(f.severity) === sevRank(kept.severity) && !f.engine && Boolean(kept.engine);
-    if (higher || tieToReviewer) seen.set(key, f);
+    if (!groups.has(key)) groups.set(key, []);
+    groups.get(key).push(f);
   }
-  return [...seen.values()];
+  const tier = (f) => sevRank(tierOf(f.rule_key) ?? f.severity);
+  const out = [];
+  for (const group of groups.values()) {
+    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 = Boolean(f.engine);
+      const es = Boolean(survivor.engine);
+      if (ef !== es) {
+        if (!ef) survivor = f;
+        continue;
+      }
+      if (sevRank(f.severity) < sevRank(survivor.severity)) survivor = f;
+    }
+    let severity = survivor.severity;
+    for (const f of group) {
+      if (sevRank(f.severity) < sevRank(severity)) severity = f.severity;
+    }
+    out.push(severity === survivor.severity ? survivor : { ...survivor, severity });
+  }
+  return out;
 }
 
 function reviewerPrompt(kind, a) {
@@ -237,33 +262,68 @@ if (merged.length === 0) {
 // The verdict is mechanical: recompute its floor from the findings the
 // coordinator returned, so an LLM-written ALLOW over a BLOCKER cannot pass.
 const returned = Array.isArray(coord?.findings) ? coord.findings : merged;
-// The brief-conflict floor: every returned finding is judged from its
-// counterpart in the coordinator's input (`merged`, same file/line/rule_key),
-// never from the labels the coordinator wrote. The raise is written into
-// `returned`, the mark kept, so the rendered review.md carries it.
+// The floor reads two inputs, never the labels the coordinator wrote:
+// - `merged` (the coordinator's input, same file/line/rule_key) for each
+//   returned finding's counterpart: (a) an engine counterpart raises it to the
+//   engine's severity, unless the one legal lowering; (b) a reviewer
+//   counterpart, labelled or not, raises it to the lower of the counterpart's
+//   asserted severity and its catalogue tier (the coordinator may reset a
+//   finding to its tier, never below; for an unknown key the bound is the
+//   counterpart's severity), and a labelled finding is raised as before.
+// - `allFindings` (before dedup, so an engine finding a reviewer finding
+//   shadowed in its triplet counts too) for the put-back: (c) every engine
+//   finding with no returned finding at its exact triplet is appended at its
+//   own severity, one copy per triplet (the higher severity, else the first
+//   seen) (operator decision 23, review ab1b5d9f). So the coordinator's drop,
+//   move or relabel of an engine finding no longer lowers the verdict.
+// The raises and put-backs are written into `returned`, the marks kept, so the
+// rendered review.md carries them.
+const tripletOf = (f) => `${f.file}\u0000${f.line}\u0000${f.rule_key}`;
 const counterparts = new Map(merged
   .filter((m) => m && typeof m === 'object')
-  .map((m) => [`${m.file}\u0000${m.line}\u0000${m.rule_key}`, m]));
+  .map((m) => [tripletOf(m), m]));
 const present = (f, field) => f[field] !== undefined && f[field] !== null;
 const raiseTo = (f, sev) => { if (sevRank(sev) < sevRank(f.severity)) f.severity = sev; };
+const lowerOf = (s1, s2) => (sevRank(s1) >= sevRank(s2) ? s1 : s2);
 for (const f of returned) {
   if (!f || typeof f !== 'object') continue;
-  const c = counterparts.get(`${f.file}\u0000${f.line}\u0000${f.rule_key}`);
+  const c = counterparts.get(tripletOf(f));
   const labelled = present(f, 'engine') || present(f, 'contradicts_brief') || present(f, 'brief_lowered_from');
   if (c && c.engine) {
-    // (1) an engine counterpart: raised to its severity, unless the one legal lowering.
+    // (a) an engine counterpart: raised to its severity, unless the one legal lowering.
     const legal = c.engine === 'code-review' && c.severity === 'MAJOR' && tierOf(c.rule_key) === 'MAJOR' &&
       f.severity === 'MINOR' && typeof f.contradicts_brief === 'string' && f.contradicts_brief.trim().length > 0 &&
       f.brief_lowered_from === 'MAJOR';
     if (!legal) raiseTo(f, c.severity);
     f.engine = c.engine;
-  } else if (labelled) {
-    // (2) a reviewer counterpart, (3) no counterpart: an unknown key raises to BLOCKER.
+    continue;
+  }
+  if (c) {
+    // (b) a reviewer counterpart: never below the lower of its asserted severity and the tier.
+    const tier = tierOf(c.rule_key);
+    raiseTo(f, tier === null ? c.severity : lowerOf(c.severity, tier));
+  }
+  if (labelled) {
+    // A labelled reviewer finding, or one with no counterpart: an unknown key raises to BLOCKER.
     raiseTo(f, tierOf(f.rule_key) ?? 'BLOCKER');
     if (c) raiseTo(f, c.severity);
     delete f.engine;
   }
 }
+// (c) the put-back (decision 23).
+const returnedTriplets = new Set(returned.filter((f) => f && typeof f === 'object').map(tripletOf));
+const putBack = new Map();
+for (const f of allFindings) {
+  if (!f || typeof f !== 'object' || !f.engine) continue;
+  const key = tripletOf(f);
+  if (returnedTriplets.has(key)) continue;
+  const kept = putBack.get(key);
+  if (!kept || sevRank(f.severity) < sevRank(kept.severity)) putBack.set(key, f);
+}
+for (const f of putBack.values()) {
+  const { brief_lowered_from: _lowered, ...copy } = f;
+  returned.push(copy);
+}
 const has = (sev) => returned.some((f) => f && f.severity === sev);
 let verdict = coord?.verdict ?? null;
 if (has('BLOCKER')) verdict = 'BLOCK';

tests/scripts/trekreview-armb-workflow.test.mjs

@@ -114,7 +114,9 @@ async function floorCase({ reviewerFindings = [], codeFindings = [], securityFin
   const prompt = s.calls.coordinatorPrompt;
   assert.ok(typeof prompt === 'string', 'the coordinator ran');
   const merged = JSON.parse(prompt.slice(prompt.indexOf(PROMPT_JSON_MARK) + PROMPT_JSON_MARK.length));
-  const raised = r.findings.some((f, i) => rank(f.severity) < rank(before[i].severity));
+  // Only indexes present in `before` are compared: the floor may append engine
+  // findings it puts back (decision 23), so `returned` can be longer.
+  const raised = r.findings.some((f, i) => i < before.length && rank(f.severity) < rank(before[i].severity));
   return { r, merged, before, raised, s };
 }
 
@@ -243,7 +245,7 @@ test('armB brief rule (10) — a marked BLOCKER-tier /code-review finding return
   checkRaise(c, true);
 });
 
-test('armB brief rule (11) — dedup keeps the /code-review MAJOR over a first-seen reviewer MINOR on the same triplet → WARN', async () => {
+test('armB brief rule (11) — bc7f4e23: dedup ranks by tier and keeps the reviewer over a /code-review MAJOR on the same triplet, at the group\'s MAJOR → WARN', async () => {
   const c = await floorCase({
     reviewerFindings: [F('CODE_REVIEW_DEFECT', 'MINOR')],
     codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
@@ -251,12 +253,12 @@ test('armB brief rule (11) — dedup keeps the /code-review MAJOR over a first-s
     verdict: 'ALLOW',
   });
   assert.equal(c.merged.length, 1);
-  assert.equal(c.merged[0].severity, 'MAJOR');
-  assert.equal(c.merged[0].engine, 'code-review');
+  assert.equal(c.merged[0].severity, 'MAJOR', 'the survivor carries the highest severity its group asserted');
+  assert.equal('engine' in c.merged[0], false, 'the reviewer finding survives the tier tie');
   assert.equal(c.r.deduped_count, 1);
   assert.equal(c.r.raw_finding_count, 2);
   assert.equal(c.r.verdict, 'WARN');
-  checkRaise(c, true);
+  checkRaise(c, false, true);
 });
 
 test('armB brief rule (12) — an unlabelled reviewer MISSING_TEST asserted BLOCKER, returned at MAJOR (catalogue reset) with WARN stays WARN', async () => {
@@ -330,9 +332,11 @@ test('armB brief rule — VERDICT_SCHEMA declares engine, contradicts_brief and
   assert.ok(/kept and marked, never removed/i.test(prompt));
 });
 
-// Decision 13 (operator 12.10): on a severity tie a reviewer-agent finding
-// survives dedup over an engine finding, so the engine's mark can never lower
-// what a reviewer agent asserted (the same rule as coordinator-contract Pass 1).
+// Decision 13 (operator 12.10), bc7f4e23: dedup ranks by catalogue tier, which
+// always ties inside a catalogue triplet, so a reviewer-agent finding survives
+// over an engine finding and carries the group's highest asserted severity; the
+// engine's mark can never lower what a reviewer agent asserted (the same order
+// as coordinator-contract Pass 1).
 test('armB brief rule (15) — a tie between a reviewer MAJOR and a /code-review MAJOR keeps the reviewer: the legal shape returned with ALLOW → WARN, no engine', async () => {
   const c = await floorCase({
     reviewerFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
@@ -348,3 +352,149 @@ test('armB brief rule (15) — a tie between a reviewer MAJOR and a /code-review
   assert.equal('engine' in c.r.findings[0], false);
   checkRaise(c, true);
 });
+
+// ---- Decision 23 (operator 13.10, review ab1b5d9f): no engine finding leaves ----
+// the workflow review unreturned. Every engine finding of the input (before
+// dedup) with no returned finding at its exact (file, line, rule_key) is put
+// back at its own severity; a returned finding is never below the lower of its
+// reviewer counterpart's severity and its catalogue tier.
+
+const at = (findings, file, line, rule_key) =>
+  findings.filter((f) => f && f.file === file && f.line === line && f.rule_key === rule_key);
+
+test('decision 23 — a /security-review BLOCKER returned as a CODE_REVIEW_IMPROVEMENT MINOR on the same line → BLOCK, the BLOCKER is put back', async () => {
+  const c = await floorCase({
+    securityFindings: [F('SECURITY_REVIEW_VULNERABILITY', 'BLOCKER')],
+    returned: [F('CODE_REVIEW_IMPROVEMENT', 'MINOR')],
+    verdict: 'ALLOW',
+  });
+  assert.equal(c.r.verdict, 'BLOCK');
+  const back = at(c.r.findings, FILE, LINE, 'SECURITY_REVIEW_VULNERABILITY');
+  assert.equal(back.length, 1, 'the BLOCKER is in the findings');
+  assert.equal(back[0].severity, 'BLOCKER');
+  assert.equal(back[0].engine, 'security-review');
+  assert.equal('brief_lowered_from' in back[0], false);
+});
+
+test('decision 23 — the same BLOCKER returned one line lower at MINOR → BLOCK, the BLOCKER is back at its own line', async () => {
+  const c = await floorCase({
+    securityFindings: [F('SECURITY_REVIEW_VULNERABILITY', 'BLOCKER')],
+    returned: [F('SECURITY_REVIEW_VULNERABILITY', 'MINOR', { line: LINE + 1 })],
+    verdict: 'ALLOW',
+  });
+  assert.equal(c.r.verdict, 'BLOCK');
+  const back = at(c.r.findings, FILE, LINE, 'SECURITY_REVIEW_VULNERABILITY');
+  assert.equal(back.length, 1, 'the BLOCKER at its own line is in the findings');
+  assert.equal(back[0].severity, 'BLOCKER');
+  assert.equal(back[0].engine, 'security-review');
+});
+
+test('decision 23 — the same BLOCKER dropped (returned empty with ALLOW) → BLOCK', async () => {
+  const c = await floorCase({
+    securityFindings: [F('SECURITY_REVIEW_VULNERABILITY', 'BLOCKER')],
+    returned: [],
+    verdict: 'ALLOW',
+  });
+  assert.equal(c.r.verdict, 'BLOCK');
+  const back = at(c.r.findings, FILE, LINE, 'SECURITY_REVIEW_VULNERABILITY');
+  assert.equal(back.length, 1);
+  assert.equal(back[0].severity, 'BLOCKER');
+  assert.equal(back[0].engine, 'security-review');
+});
+
+test('decision 23 — a /security-review BLOCKER shadowed in dedup by a reviewer MINOR on the same triplet, returned empty → BLOCK', async () => {
+  const c = await floorCase({
+    reviewerFindings: [F('SECURITY_REVIEW_VULNERABILITY', 'MINOR')],
+    securityFindings: [F('SECURITY_REVIEW_VULNERABILITY', 'BLOCKER')],
+    returned: [],
+    verdict: 'ALLOW',
+  });
+  assert.equal(c.r.verdict, 'BLOCK');
+  const back = at(c.r.findings, FILE, LINE, 'SECURITY_REVIEW_VULNERABILITY');
+  assert.equal(back.length, 1);
+  assert.equal(back[0].severity, 'BLOCKER');
+  assert.equal(back[0].engine, 'security-review');
+});
+
+test('decision 23 — a /code-review MAJOR dropped → WARN', async () => {
+  const c = await floorCase({
+    codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
+    returned: [],
+    verdict: 'ALLOW',
+  });
+  assert.equal(c.r.verdict, 'WARN');
+  const back = at(c.r.findings, FILE, LINE, 'CODE_REVIEW_DEFECT');
+  assert.equal(back.length, 1);
+  assert.equal(back[0].severity, 'MAJOR');
+  assert.equal(back[0].engine, 'code-review');
+});
+
+for (const order of ['MAJOR first', 'BLOCKER first']) {
+  test(`decision 23 — two /code-review findings on one triplet (${order}), returned empty → BLOCK, one copy, at BLOCKER`, async () => {
+    const pair = [F('CODE_REVIEW_DEFECT', 'MAJOR', { title: 'major' }), F('CODE_REVIEW_DEFECT', 'BLOCKER', { title: 'blocker' })];
+    const c = await floorCase({
+      codeFindings: order === 'MAJOR first' ? pair : [pair[1], pair[0]],
+      returned: [],
+      verdict: 'ALLOW',
+    });
+    assert.equal(c.r.verdict, 'BLOCK');
+    const back = at(c.r.findings, FILE, LINE, 'CODE_REVIEW_DEFECT');
+    assert.equal(back.length, 1, 'exactly one finding at the triplet');
+    assert.equal(back[0].severity, 'BLOCKER', 'the higher-severity copy is put back');
+    assert.equal(back[0].title, 'blocker');
+    assert.equal(back[0].engine, 'code-review');
+  });
+}
+
+test('decision 23 floor (b) — a reviewer finding with a rule_key outside the catalogue asserted MAJOR, returned at MINOR → raised to MAJOR, WARN', async () => {
+  const c = await floorCase({
+    reviewerFindings: [F('NOT_IN_THE_CATALOGUE', 'MAJOR')],
+    returned: [F('NOT_IN_THE_CATALOGUE', 'MINOR')],
+    verdict: 'ALLOW',
+  });
+  assert.equal(c.r.verdict, 'WARN');
+  assert.equal(c.r.findings.length, 1);
+  assert.equal(c.r.findings[0].severity, 'MAJOR', "the bound for an unknown key is the counterpart's asserted severity");
+});
+
+test('decision 23 — the one legal lowering (marked /code-review MAJOR returned as MINOR with the mark) still gives ALLOW', async () => {
+  const c = await floorCase({
+    codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
+    returned: [F('CODE_REVIEW_DEFECT', 'MINOR', { contradicts_brief: MARK, brief_lowered_from: 'MAJOR' })],
+    verdict: 'ALLOW',
+  });
+  assert.equal(c.r.verdict, 'ALLOW');
+  assert.equal(c.r.findings.length, 1, 'nothing is put back over a returned triplet');
+  assert.equal(c.r.findings[0].severity, 'MINOR');
+  assert.equal(c.r.findings[0].engine, 'code-review');
+});
+
+// ---- the docs state the workflow floor ------------------------------------------
+
+// Slice from the first `start` to the next `end` after it; fail when either is missing.
+function section(text, start, end) {
+  const i = text.indexOf(start);
+  assert.ok(i !== -1, `section start not found: ${start}`);
+  const j = text.indexOf(end, i + start.length);
+  assert.ok(j !== -1, `section end not found after ${start}: ${JSON.stringify(end)}`);
+  return text.slice(i, j);
+}
+
+const CMD_TEXT = readFileSync(join(ROOT, 'commands', 'trekreview.md'), 'utf-8');
+const HANDOVER_TEXT = readFileSync(join(ROOT, 'docs', 'HANDOVER-CONTRACTS.md'), 'utf-8');
+const PUT_BACK = /put\s+back\s+at\s+its\s+own\s+severity/;
+const FLOOR_B = /lower\s+of\s+its\s+reviewer\s+counterpart's\s+severity\s+and\s+its\s+catalogue\s+tier/;
+
+test('decision 23 — commands/trekreview.md workflow section states the put-back and floor (b); the port is no longer the SAME pipeline', () => {
+  const wf = section(CMD_TEXT, '## Phase 5–6 via the Workflow substrate', '## Phase 7');
+  assert.match(wf, PUT_BACK);
+  assert.match(wf, FLOOR_B);
+  assert.doesNotMatch(CMD_TEXT, /the\s+SAME\s+Phase/);
+});
+
+test('decision 23 — Handover 6 states the dedup rank on both paths, the put-back and floor (b)', () => {
+  const h6 = section(HANDOVER_TEXT, '## Handover 6 — `review.md` → plan', '\n## Handover 7');
+  assert.match(h6, /dedup\s+ranks\s+as\s+Pass\s+1\s+does\s+\(both\s+paths\)/);
+  assert.match(h6, PUT_BACK);
+  assert.match(h6, FLOOR_B);
+});