feat(eval): add review-coordinator contract reference impl + deterministic test

lib/review/coordinator-contract.mjs

@@ -0,0 +1,234 @@
+// lib/review/coordinator-contract.mjs
+// SKAL-1·4a — deterministic reference implementation of the review-coordinator
+// 4-pass contract (agents/review-coordinator.md §"Your 4-pass process").
+//
+// This is a DETERMINISTIC SUBSET, not a full mirror of the LLM coordinator.
+// It implements the pure, hermetic passes and DELIBERATELY EXCLUDES the parts
+// that need a live filesystem or LLM judgement (which belong to the 4c
+// LLM-in-the-loop eval, not this all-agree foundation tier):
+//   - Pass 2 "Accuracy" file-existence / line-plausibility glob (fs I/O).
+//   - Pass 2 "Actionability" imperative-verb heuristic — the real coordinator
+//     uses LLM judgement here; a verb-list approximation would DIVERGE from the
+//     contract being mirrored, so only the deterministic "recommended_action is
+//     present-and-non-empty when supplied" half is kept.
+//   - The doc's 4-tuple `(file,line,rule_key,title)` id recompute — the shipped
+//     `computeFindingId` is 3-arg `(file,line,rule_key)`; this module follows
+//     the shipped code and flags the doc divergence (the 4-tuple id is not
+//     producible by the current helper).
+//
+// What IS implemented, purely: Pass 1 (triplet dedup → highest-severity-wins
+// survivor + conformance tiebreak + detail concat + raised_by provenance),
+// Pass 2 succinctness + actionability-presence, Pass 3 reasonableness
+// (citation / unknown-rule_key drop, severity-mismatch correction), Pass 4
+// verdict thresholds. No LLM, no network, no time, no randomness.
+//
+// Reuses: SEVERITY_VALUES / RULE_KEYS / getRule (rule-catalogue.mjs),
+// computeFindingId (finding-id.mjs, triplet), validateFindings
+// (findings-schema.mjs). Triplet key format mirrors
+// scripts/bakeoff-armA-merge.mjs:33; 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 { validateFindings } from './findings-schema.mjs';
+
+export const JUDGE_TITLE_MAX = 100;
+export const JUDGE_DETAIL_MAX = 800;
+
+/**
+ * Catalogue-tier rank of a severity: lower number = higher severity.
+ * BLOCKER=0 … SUGGESTION=3; an unknown severity ranks last.
+ * @param {string} severity
+ * @returns {number}
+ */
+export function severityRank(severity) {
+  const i = SEVERITY_VALUES.indexOf(severity);
+  return i === -1 ? SEVERITY_VALUES.length : i;
+}
+
+function isConformance(reviewer) {
+  return typeof reviewer === 'string' && reviewer.toLowerCase().includes('conformance');
+}
+
+function tripletKey(f) {
+  return `${f.file} ${f.line} ${f.rule_key}`;
+}
+
+/**
+ * Validate each reviewer payload and collect findings from the VALID ones,
+ * tagging each finding with its source reviewer (mirrors mergeArmA — invalid
+ * payloads are skipped, not crashed-on).
+ * @param {Array<{reviewer?: string, findings: object[]}>} reviewerPayloads
+ * @returns {{ findings: object[], skipped: Array<{reviewer: string|null, error_codes: string[]}> }}
+ */
+export function ingest(reviewerPayloads) {
+  const findings = [];
+  const skipped = [];
+  for (const payload of reviewerPayloads) {
+    const r = validateFindings(payload);
+    if (!r.valid) {
+      skipped.push({ reviewer: payload?.reviewer ?? null, error_codes: r.errors.map((e) => e.code) });
+      continue;
+    }
+    for (const f of payload.findings) {
+      findings.push({ ...f, reviewer: f.reviewer ?? payload.reviewer ?? f.owner_reviewer ?? null });
+    }
+  }
+  return { findings, skipped };
+}
+
+/**
+ * Pass 1 — dedup by (file, line, rule_key) triplet. Survivor = highest
+ * catalogue severity; severity tie → prefer the conformance reviewer; carries
+ * raised_by provenance, concatenates other reviewers' attribution into detail,
+ * and recomputes the id over the triplet.
+ * @param {object[]} findings
+ * @returns {object[]}
+ */
+export function dedupByTriplet(findings) {
+  const groups = new Map();
+  for (const f of findings) {
+    const key = tripletKey(f);
+    if (!groups.has(key)) groups.set(key, []);
+    groups.get(key).push(f);
+  }
+  const out = [];
+  for (const group of groups.values()) {
+    let survivor = group[0];
+    for (const f of group.slice(1)) {
+      const higher = severityRank(f.severity) < severityRank(survivor.severity);
+      const tieToConformance =
+        severityRank(f.severity) === severityRank(survivor.severity) &&
+        isConformance(f.reviewer) && !isConformance(survivor.reviewer);
+      if (higher || tieToConformance) survivor = f;
+    }
+    const raised_by = [...new Set(group.map((f) => f.reviewer).filter(Boolean))];
+    const others = group.filter((f) => f !== survivor);
+    let detail = survivor.detail;
+    if (others.length > 0) {
+      detail = survivor.detail ?? '';
+      for (const o of others) {
+        detail += `\nAlso flagged by ${o.reviewer ?? 'unknown'}: ${o.title ?? o.rule_key}.`;
+      }
+    }
+    const id = computeFindingId(survivor.file, survivor.line, survivor.rule_key);
+    out.push({ ...survivor, id, ...(detail !== undefined ? { detail } : {}), raised_by });
+  }
+  return out;
+}
+
+/**
+ * Pass 2 — HubSpot Judge (deterministic subset): drop on succinctness
+ * (title > 100 or detail > 800 chars) and actionability (recommended_action,
+ * when present, must be a non-empty string). The imperative-verb test is
+ * excluded (LLM judgement).
+ * @param {object[]} findings
+ * @returns {{ kept: object[], dropped: object[] }}
+ */
+export function judgeFilter(findings) {
+  const kept = [];
+  const dropped = [];
+  for (const f of findings) {
+    const titleLen = (f.title ?? '').length;
+    const detailLen = (f.detail ?? '').length;
+    let reason = null;
+    if (titleLen > JUDGE_TITLE_MAX) reason = 'succinctness:title';
+    else if (detailLen > JUDGE_DETAIL_MAX) reason = 'succinctness:detail';
+    else if ('recommended_action' in f &&
+      (typeof f.recommended_action !== 'string' || f.recommended_action.trim().length === 0)) {
+      reason = 'actionability:empty';
+    }
+    if (reason) dropped.push({ ...f, suppressed_reason: reason });
+    else kept.push(f);
+  }
+  return { kept, dropped };
+}
+
+/**
+ * Pass 3 — Cloudflare reasonableness (deterministic subset): drop findings
+ * with no citation (empty file / line < 0) or an unknown rule_key; CORRECT a
+ * severity that does not match the catalogue tier (a correction, not a drop).
+ * The fs file-existence glob is excluded (I/O).
+ * @param {object[]} findings
+ * @returns {{ kept: object[], dropped: object[] }}
+ */
+export function reasonablenessFilter(findings) {
+  const kept = [];
+  const dropped = [];
+  for (const f of findings) {
+    if (typeof f.file !== 'string' || f.file.length === 0 ||
+      (typeof f.line === 'number' && f.line < 0)) {
+      dropped.push({ ...f, suppressed_reason: 'no-citation' });
+      continue;
+    }
+    if (!RULE_KEYS.has(f.rule_key)) {
+      dropped.push({ ...f, suppressed_reason: 'unknown-rule_key' });
+      continue;
+    }
+    const rule = getRule(f.rule_key);
+    if (rule && f.severity !== rule.severity) {
+      kept.push({ ...f, severity: rule.severity, original_severity: f.severity });
+    } else {
+      kept.push(f);
+    }
+  }
+  return { kept, dropped };
+}
+
+/**
+ * Pass 4 — compute the verdict from severity counts (after dedup + filtering).
+ * BLOCKER ≥ 1 → BLOCK; else MAJOR ≥ 1 → WARN; else ALLOW.
+ * @param {object[]} findings
+ * @returns {{ verdict: 'BLOCK'|'WARN'|'ALLOW', counts: Record<string, number> }}
+ */
+export function computeVerdict(findings) {
+  const counts = { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
+  for (const f of findings) {
+    if (counts[f.severity] !== undefined) counts[f.severity] += 1;
+  }
+  let verdict;
+  if (counts.BLOCKER >= 1) verdict = 'BLOCK';
+  else if (counts.MAJOR >= 1) verdict = 'WARN';
+  else verdict = 'ALLOW';
+  return { verdict, counts };
+}
+
+/**
+ * Run the full deterministic contract: ingest → Pass 1 → Pass 2 → Pass 3 → Pass 4.
+ * @param {Array<{reviewer?: string, findings: object[]}>} reviewerPayloads
+ * @returns {{ verdict: string, counts: Record<string, number>, findings: object[], suppressed: object[], skipped: object[] }}
+ */
+export function runContract(reviewerPayloads) {
+  const { findings: ingested, skipped } = ingest(reviewerPayloads);
+  const deduped = dedupByTriplet(ingested);
+  const judged = judgeFilter(deduped);
+  const reasoned = reasonablenessFilter(judged.kept);
+  const { verdict, counts } = computeVerdict(reasoned.kept);
+  return {
+    verdict,
+    counts,
+    findings: reasoned.kept,
+    suppressed: [...judged.dropped, ...reasoned.dropped],
+    skipped,
+  };
+}
+
+// ---- CLI shim ----------------------------------------------------------------
+
+if (import.meta.url === `file://${process.argv[1]}`) {
+  const args = process.argv.slice(2);
+  const filePath = args.find((a) => !a.startsWith('--'));
+  if (!filePath) {
+    process.stderr.write('Usage: coordinator-contract.mjs [--json] <reviewer-payloads.json>\n');
+    process.exit(2);
+  }
+  const { readFileSync } = await import('node:fs');
+  const payloads = JSON.parse(readFileSync(filePath, 'utf-8'));
+  const result = runContract(Array.isArray(payloads) ? payloads : [payloads]);
+  if (args.includes('--json')) {
+    process.stdout.write(JSON.stringify(result, null, 2) + '\n');
+  } else {
+    process.stdout.write(`coordinator-contract: ${result.verdict} (${result.findings.length} findings, ${result.suppressed.length} suppressed)\n`);
+  }
+  process.exit(0);
+}

tests/lib/coordinator-contract.test.mjs

@@ -0,0 +1,163 @@
+// tests/lib/coordinator-contract.test.mjs
+// SKAL-1·4a — deterministic test of the review-coordinator contract subset.
+//
+// Inline reviewer-JSON fixtures (idiom: tests/validators/brief-validator.test.mjs).
+// Hermetic: no LLM, network, time, or randomness.
+
+import { test } from 'node:test';
+import { strict as assert } from 'node:assert';
+import {
+  severityRank,
+  ingest,
+  dedupByTriplet,
+  judgeFilter,
+  reasonablenessFilter,
+  computeVerdict,
+  runContract,
+} from '../../lib/review/coordinator-contract.mjs';
+
+// ---- Pass 1 — dedup --------------------------------------------------------
+
+test('dedupByTriplet — genuine cross-reviewer collapse (identical triplet) → 1, raised_by both', () => {
+  // Both reviewers flag the SAME (file,line,rule_key) triplet — this is the
+  // real collapse the coordinator performs.
+  const findings = [
+    { file: 'lib/auth/jwt.mjs', line: 19, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', title: 'algo from header', reviewer: 'correctness' },
+    { file: 'lib/auth/jwt.mjs', line: 19, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', title: 'NG1 violated', reviewer: 'conformance' },
+  ];
+  const out = dedupByTriplet(findings);
+  assert.equal(out.length, 1, 'identical triplets must collapse to one');
+  assert.deepEqual([...out[0].raised_by].sort(), ['conformance', 'correctness']);
+});
+
+test('dedupByTriplet — README #2 two DIFFERENT rule_keys at same file:line do NOT collapse', () => {
+  // The bakeoff-rich README marks issue #2 dual-flaggable via SECURITY_INJECTION
+  // AND NON_GOAL_VIOLATED. Those are DIFFERENT triplets → distinct defects → kept.
+  const findings = [
+    { file: 'lib/auth/jwt.mjs', line: 19, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', reviewer: 'correctness' },
+    { file: 'lib/auth/jwt.mjs', line: 19, rule_key: 'NON_GOAL_VIOLATED', severity: 'BLOCKER', reviewer: 'conformance' },
+  ];
+  assert.equal(dedupByTriplet(findings).length, 2);
+});
+
+test('dedupByTriplet — survivor is the highest-severity finding in the group', () => {
+  const findings = [
+    { file: 'x.mjs', line: 1, rule_key: 'SECURITY_INJECTION', severity: 'MINOR', reviewer: 'correctness' },
+    { file: 'x.mjs', line: 1, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', reviewer: 'correctness' },
+  ];
+  const out = dedupByTriplet(findings);
+  assert.equal(out.length, 1);
+  assert.equal(out[0].severity, 'BLOCKER');
+});
+
+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' },
+    { file: 'x.mjs', line: 1, rule_key: 'NON_GOAL_VIOLATED', severity: 'BLOCKER', title: 'conf', reviewer: 'conformance' },
+  ];
+  const out = dedupByTriplet(findings);
+  assert.equal(out.length, 1);
+  assert.equal(out[0].reviewer, 'conformance');
+});
+
+// ---- severity ranking (4-tier, incl. synthetic SUGGESTION) -----------------
+
+test('severityRank — orders the full 4-tier SEVERITY_VALUES incl. SUGGESTION', () => {
+  assert.ok(severityRank('BLOCKER') < severityRank('MAJOR'));
+  assert.ok(severityRank('MAJOR') < severityRank('MINOR'));
+  assert.ok(severityRank('MINOR') < severityRank('SUGGESTION'));
+});
+
+test('dedupByTriplet — SUGGESTION vs MINOR (synthetic) keeps the MINOR survivor', () => {
+  // gold corpus has no SUGGESTION; exercise the 4th tier synthetically.
+  const findings = [
+    { file: 'x.mjs', line: 2, rule_key: 'MISSING_ERROR_HANDLING', severity: 'SUGGESTION', reviewer: 'correctness' },
+    { file: 'x.mjs', line: 2, rule_key: 'MISSING_ERROR_HANDLING', severity: 'MINOR', reviewer: 'correctness' },
+  ];
+  const out = dedupByTriplet(findings);
+  assert.equal(out.length, 1);
+  assert.equal(out[0].severity, 'MINOR');
+});
+
+// ---- Pass 4 — verdict thresholds -------------------------------------------
+
+test('computeVerdict — BLOCKER>=1 → BLOCK, MAJOR-only → WARN, else ALLOW', () => {
+  assert.equal(computeVerdict([{ severity: 'BLOCKER' }, { severity: 'MAJOR' }]).verdict, 'BLOCK');
+  assert.equal(computeVerdict([{ severity: 'MAJOR' }, { severity: 'MINOR' }]).verdict, 'WARN');
+  assert.equal(computeVerdict([{ severity: 'MINOR' }]).verdict, 'ALLOW');
+  assert.equal(computeVerdict([{ severity: 'SUGGESTION' }]).verdict, 'ALLOW');
+  assert.equal(computeVerdict([]).verdict, 'ALLOW');
+});
+
+test('computeVerdict — counts each severity tier', () => {
+  const { counts } = computeVerdict([
+    { severity: 'BLOCKER' }, { severity: 'BLOCKER' }, { severity: 'MAJOR' }, { severity: 'MINOR' },
+  ]);
+  assert.deepEqual(counts, { BLOCKER: 2, MAJOR: 1, MINOR: 1, SUGGESTION: 0 });
+});
+
+// ---- Pass 3 — reasonableness -----------------------------------------------
+
+test('reasonablenessFilter — drops unknown rule_key + citation-less, corrects severity mismatch', () => {
+  const r = reasonablenessFilter([
+    { file: 'x.mjs', line: 1, rule_key: 'NOPE_KEY', severity: 'BLOCKER' },        // unknown → drop
+    { file: '', line: 1, rule_key: 'MISSING_TEST', severity: 'MAJOR' },           // no file → drop
+    { file: 'x.mjs', line: -1, rule_key: 'MISSING_TEST', severity: 'MAJOR' },     // line < 0 → drop
+    { file: 'x.mjs', line: 1, rule_key: 'MISSING_TEST', severity: 'MINOR' },      // catalogue is MAJOR → correct, keep
+  ]);
+  assert.equal(r.kept.length, 1);
+  assert.equal(r.dropped.length, 3);
+  assert.equal(r.kept[0].severity, 'MAJOR');
+  assert.equal(r.kept[0].original_severity, 'MINOR');
+});
+
+// ---- Pass 2 — judge --------------------------------------------------------
+
+test('judgeFilter — drops over-long title and empty recommended_action', () => {
+  const j = judgeFilter([
+    { file: 'x.mjs', line: 1, rule_key: 'MISSING_TEST', severity: 'MAJOR', title: 'x'.repeat(101) },          // too long → drop
+    { file: 'x.mjs', line: 2, rule_key: 'MISSING_TEST', severity: 'MAJOR', title: 'ok', recommended_action: '   ' }, // empty action → drop
+    { file: 'x.mjs', line: 3, rule_key: 'MISSING_TEST', severity: 'MAJOR', title: 'ok' },                     // keep (no action field is fine)
+  ]);
+  assert.equal(j.kept.length, 1);
+  assert.equal(j.dropped.length, 2);
+});
+
+// ---- ingest ----------------------------------------------------------------
+
+test('ingest — collects findings from valid payloads, skips invalid ones', () => {
+  const { findings, skipped } = ingest([
+    { reviewer: 'correctness', findings: [{ file: 'x.mjs', line: 1, rule_key: 'MISSING_TEST', severity: 'MAJOR' }] },
+    { reviewer: 'bad', findings: [{ line: 1, rule_key: 'MISSING_TEST', severity: 'MAJOR' }] }, // missing file → invalid → skipped
+  ]);
+  assert.equal(findings.length, 1);
+  assert.equal(findings[0].reviewer, 'correctness');
+  assert.equal(skipped.length, 1);
+});
+
+// ---- end-to-end ------------------------------------------------------------
+
+test('runContract — two reviewers, dual-flag triplet collapses, verdict BLOCK', () => {
+  const result = runContract([
+    { reviewer: 'correctness', findings: [
+      { file: 'lib/auth/jwt.mjs', line: 19, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', title: 'algo from header' },
+      { file: 'lib/auth/refresh.mjs', line: 0, rule_key: 'MISSING_TEST', severity: 'MAJOR', title: 'no concurrent test' },
+    ] },
+    { reviewer: 'conformance', findings: [
+      { file: 'lib/auth/jwt.mjs', line: 19, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', title: 'NG1' }, // collapses with correctness's
+      { file: 'lib/handlers/login.mjs', line: 17, rule_key: 'UNIMPLEMENTED_CRITERION', severity: 'BLOCKER', title: '200 not 401' },
+    ] },
+  ]);
+  assert.equal(result.verdict, 'BLOCK');
+  assert.equal(result.findings.length, 3, '4 raw findings, jwt:19 SECURITY_INJECTION collapses → 3');
+  const jwt = result.findings.find((f) => f.file === 'lib/auth/jwt.mjs');
+  assert.deepEqual([...jwt.raised_by].sort(), ['conformance', 'correctness']);
+});
+
+test('runContract — deterministic: identical input yields identical output', () => {
+  const input = [
+    { reviewer: 'correctness', findings: [{ file: 'a.mjs', line: 1, rule_key: 'MISSING_TEST', severity: 'MAJOR', title: 't' }] },
+    { reviewer: 'conformance', findings: [{ file: 'b.mjs', line: 2, rule_key: 'UNIMPLEMENTED_CRITERION', severity: 'BLOCKER', title: 'u' }] },
+  ];
+  assert.deepEqual(runContract(input), runContract(input));
+});