voyage/lib/review/coordinator-contract.mjs

// 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.
//   - Nothing about ids beyond the triplet: `computeFindingId(file, line,
//     rule_key)` is the one id. 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).
//
// What IS implemented, purely: Pass 1 (triplet dedup → highest-severity-wins
// survivor + conformance tiebreak + 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),
// Pass 4 verdict thresholds — fail-closed: a suppression that did not REFUTE
// the finding, and a reviewer that never reported, forbid ALLOW (see
// classifySuppression). 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';
import { isMain } from '../util/is-main.mjs';

export const JUDGE_TITLE_MAX = 100;
export const JUDGE_DETAIL_MAX = 800;

// ---- Suppression classification (fail-closed) --------------------------------
//
// A removal is `dropped` ONLY when the test refuted the finding as a claim
// about this codebase. Every other removal is `unverified`: the coordinator
// took the finding out of the count without ever establishing it was unreal,
// so it may not be spent as evidence of a clean review.

/**
 * Reasons that REFUTE. `no-citation` is the only one this deterministic subset
 * can emit: a finding whose `file` is empty or whose `line` is negative names
 * no location, so it makes no checkable claim at all
 * (agents/review-coordinator.md Pass 3 — "Speculative 'code might break
 * somewhere' findings have no anchor").
 *
 * `accuracy:refuted` (Pass 2 Accuracy — a citation escaping the repo root) and
 * `file-existence:refuted` (Pass 3 — absent from both working tree and diff)
 * are emitted by the LLM coordinator, whose fs/judgement branches this module
 * excludes. They are declared here anyway: the vocabulary is owned in one
 * place so prose and lib cannot drift.
 */
export const REFUTING_REASONS = Object.freeze(new Set([
  'no-citation',
  'accuracy:refuted',
  'file-existence:refuted',
]));

/**
 * The reason vocabulary on the unverified side. Length is not in it: since
 * ordre 5 (P11) succinctness trims MINOR/SUGGESTION text and never removes a
 * finding, so `succinctness:*` is no longer emitted (a stray one still fails
 * closed through `classifySuppression`). `file-existence:indeterminate`
 * is emitted by the LLM coordinator's Pass 3 (which runs the fs Glob this
 * module deliberately excludes); the vocabulary is owned here so prose and lib
 * cannot drift.
 */
export const UNVERIFIED_REASONS = Object.freeze([
  'actionability:empty',
  'engine:out-of-scope',
  'unknown-rule_key',
  'file-existence:indeterminate',
]);

/**
 * Classify a suppression reason. Anything not declared refuting is
 * `unverified` — the default is fail-CLOSED, so a reason introduced later
 * without a decision cannot silently move the verdict toward ALLOW.
 * @param {string} reason
 * @returns {'refuted'|'unverified'}
 */
export function classifySuppression(reason) {
  return REFUTING_REASONS.has(reason) ? 'refuted' : 'unverified';
}

/**
 * Tag a finding with its suppression reason and route it to the refuted
 * (`dropped`) or the `unverified` bucket.
 * @param {object} finding
 * @param {string} reason
 * @param {object[]} dropped
 * @param {object[]} unverified
 */
function suppress(finding, reason, dropped, unverified) {
  const tagged = { ...finding, suppressed_reason: reason };
  if (classifySuppression(reason) === 'refuted') dropped.push(tagged);
  else unverified.push(tagged);
}

/**
 * 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;
}

/**
 * The tiers whose over-long text may be trimmed. Length is packaging, never a
 * claim: a BLOCKER or MAJOR keeps every character, and nothing is ever removed
 * for length. Borrowed: "cap only the nits, and count what was cut"
 * (ai-native-sdlc `assets/REVIEW.md:7-23`, MIT).
 */
export const TRIMMABLE_SEVERITIES = Object.freeze(['MINOR', 'SUGGESTION']);

export const TRIM_MARKER = ' […trimmed]';

/**
 * The tier a finding will be judged at: the catalogue tier when the rule_key is
 * known (Pass 3 resets to it anyway), else the reviewer-asserted severity. The
 * trim decision reads this, so a reviewer cannot get a MAJOR trimmed by
 * mislabelling it MINOR.
 * @param {object} f
 * @returns {string}
 */
function effectiveSeverity(f) {
  return getRule(f.rule_key)?.severity ?? f.severity;
}

function trimText(text, max) {
  return text.slice(0, max - TRIM_MARKER.length) + TRIM_MARKER;
}

/**
 * Trim a detail but keep Pass 1's "Also flagged by …" provenance lines: the
 * reviewer's own text is what gets cut. When the provenance alone is too long
 * it is cut too — `raised_by` still names every reviewer.
 */
function trimDetail(detail, max) {
  const at = detail.indexOf('\nAlso flagged by ');
  if (at === -1) return trimText(detail, max);
  const head = detail.slice(0, at);
  const tail = detail.slice(at);
  const room = max - tail.length - TRIM_MARKER.length;
  if (room < 40) return trimText(detail, max);
  return head.slice(0, room) + TRIM_MARKER + tail;
}

/**
 * Pass 2 — HubSpot Judge (deterministic subset): succinctness and
 * actionability (recommended_action, when present, must be a non-empty
 * string). The imperative-verb test is excluded (LLM judgement).
 *
 * Succinctness never removes a finding (ordre 5, P11 — S118's most serious
 * finding was struck for a long `detail`). A MINOR/SUGGESTION whose title is
 * over 100 or detail over 800 characters is TRIMMED, kept, and returned in
 * `trimmed` so the count is visible; a BLOCKER/MAJOR keeps its full text.
 *
 * Actionability reads a `.length`, never the claim, so it cannot establish the
 * finding is unreal: it routes to `unverified`. `dropped` stays in the
 * signature for the refuting Pass-2 filter this subset excludes (Accuracy: a
 * path-traversal escape IS a refutation).
 * @param {object[]} findings
 * @returns {{ kept: object[], dropped: object[], unverified: object[], trimmed: object[] }}
 */
export function judgeFilter(findings) {
  const kept = [];
  const dropped = [];
  const unverified = [];
  const trimmed = [];
  for (const f of findings) {
    if ('recommended_action' in f &&
      (typeof f.recommended_action !== 'string' || f.recommended_action.trim().length === 0)) {
      suppress(f, 'actionability:empty', dropped, unverified);
      continue;
    }
    const title = f.title ?? '';
    const detail = f.detail ?? '';
    const longTitle = title.length > JUDGE_TITLE_MAX;
    const longDetail = detail.length > JUDGE_DETAIL_MAX;
    if ((longTitle || longDetail) && TRIMMABLE_SEVERITIES.includes(effectiveSeverity(f))) {
      const out = { ...f };
      const fields = [];
      if (longTitle) { out.title = trimText(title, JUDGE_TITLE_MAX); fields.push('title'); }
      if (longDetail) { out.detail = trimDetail(detail, JUDGE_DETAIL_MAX); fields.push('detail'); }
      out.trimmed_fields = fields;
      kept.push(out);
      trimmed.push(out);
    } else {
      kept.push(f);
    }
  }
  return { kept, dropped, unverified, trimmed };
}

/**
 * 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) — its indeterminate branch is
 * prose-side, tokenised as `file-existence:indeterminate`.
 *
 * `no-citation` REFUTES (the finding names no location, so it makes no
 * checkable claim) and is dropped. `unknown-rule_key` does not: an ad-hoc key
 * is a real defect wearing the wrong label — v5.1.1 high-effort mode already
 * KEEPS these, normalised to PLAN_EXECUTE_DRIFT — so it routes to `unverified`.
 * @param {object[]} findings
 * @returns {{ kept: object[], dropped: object[], unverified: object[] }}
 */
export function reasonablenessFilter(findings) {
  const kept = [];
  const dropped = [];
  const unverified = [];
  for (const f of findings) {
    if (typeof f.file !== 'string' || f.file.length === 0 ||
      (typeof f.line === 'number' && f.line < 0)) {
      suppress(f, 'no-citation', dropped, unverified);
      continue;
    }
    if (!RULE_KEYS.has(f.rule_key)) {
      suppress(f, 'unknown-rule_key', dropped, unverified);
      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, unverified };
}

/**
 * Pass 4 — compute the verdict from severity counts (after dedup + filtering).
 * BLOCKER ≥ 1 → BLOCK; else MAJOR ≥ 1 → WARN; else ALLOW.
 *
 * FAIL-CLOSED: ALLOW additionally requires that nothing is `unverified` and
 * that every expected reviewer reported. Neither ever RAISES a verdict — the
 * severity thresholds are untouched — they only forbid the clean one, so the
 * worst case of a false unverified is WARN plus a stated reason, never a
 * silent pass. Unverified findings are NOT counted into a severity tier: their
 * severity is reviewer-asserted and was never substantiated.
 *
 * @param {object[]} findings
 * @param {{ unverified?: object[], missingReviewers?: string[], unattributablePayloads?: number }} [options]
 * @returns {{ verdict: 'BLOCK'|'WARN'|'ALLOW', counts: Record<string, number>, allow_blocked_by: string[] }}
 */
export function computeVerdict(findings, options = {}) {
  const counts = { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
  for (const f of findings) {
    if (counts[f.severity] !== undefined) counts[f.severity] += 1;
  }

  const unverified = options.unverified ?? [];
  const missingReviewers = options.missingReviewers ?? [];
  const unattributablePayloads = options.unattributablePayloads ?? 0;
  const allow_blocked_by = [];
  const byReason = new Map();
  for (const f of unverified) {
    const reason = f?.suppressed_reason ?? 'unspecified';
    byReason.set(reason, (byReason.get(reason) ?? 0) + 1);
  }
  for (const [reason, n] of byReason) allow_blocked_by.push(`unverified:${reason} (${n})`);
  for (const r of missingReviewers) allow_blocked_by.push(`missing-reviewer:${r}`);
  if (unattributablePayloads > 0) allow_blocked_by.push(`unattributable-payload (${unattributablePayloads})`);

  let verdict;
  if (counts.BLOCKER >= 1) verdict = 'BLOCK';
  else if (counts.MAJOR >= 1) verdict = 'WARN';
  else if (allow_blocked_by.length > 0) verdict = 'WARN';
  else verdict = 'ALLOW';
  return { verdict, counts, allow_blocked_by };
}

/**
 * Run the full deterministic contract: ingest → Pass 1 → Pass 2 → Pass 3 → Pass 4.
 *
 * `options.expectedReviewers` names the reviewers this review was supposed to
 * hear from. A reviewer that is absent from the payloads, or whose named
 * payload failed schema validation and was thrown away at ingest, lands in
 * `missing_reviewers`; a payload that failed schema WITHOUT a reviewer name is
 * counted in `unattributable_payloads` instead. Either forbids ALLOW: an
 * unread reviewer is an absent one, and zero findings from a silent reviewer
 * must not read like zero findings from a clean diff.
 *
 * `suppressed` stays the UNION of `dropped` (refuted) and `unverified` so
 * existing consumers keep their meaning; `unverified` is the subset that
 * forbids ALLOW. Do not iterate both and count twice.
 *
 * @param {Array<{reviewer?: string, findings: object[]}>} reviewerPayloads
 * @param {{ expectedReviewers?: string[], engineOutOfScope?: object[] }} [options]
 * `declined_to_judge` lists every unverified finding with its reason, and
 * `trimmed_count` counts the MINOR/SUGGESTION findings whose text was trimmed
 * (kept, judged, and counted — never removed for length).
 *
 * @returns {{ verdict: string, counts: Record<string, number>, findings: object[], suppressed: object[], unverified: object[], skipped: object[], missing_reviewers: string[], unattributable_payloads: number, allow_blocked_by: string[], declined_to_judge: object[], trimmed_count: number }}
 */
export function runContract(reviewerPayloads, options = {}) {
  const { findings: ingested, skipped } = ingest(reviewerPayloads);
  const deduped = dedupByTriplet(ingested);
  const judged = judgeFilter(deduped);
  const reasoned = reasonablenessFilter(judged.kept);
  // Engine findings on files outside the review's SHA range (engine-findings.mjs
  // `out_of_scope`). /security-review diffs against origin's default branch,
  // wider than this review: a real claim nobody here judged, so it forbids
  // ALLOW. /code-review was given exactly this range: its MINOR or SUGGESTION
  // finding outside it (an improvement to code the review did not change) is
  // declined and listed, never dropped, but does not hold ALLOW back
  // (voyage-lekerepo trial 2, 2026-10-08). A MAJOR+ one still forbids ALLOW:
  // a changed signature that breaks an unchanged caller is cited AT the
  // unchanged file (independent review, 2026-10-08).
  const outOfScope = (options.engineOutOfScope ?? [])
    .map((f) => ({ ...f, suppressed_reason: 'engine:out-of-scope' }));
  const minorOnly = (f) => f.engine === 'code-review' && (f.severity === 'MINOR' || f.severity === 'SUGGESTION');
  const outOfScopeBlocking = outOfScope.filter((f) => !minorOnly(f));
  const outOfScopeNoted = outOfScope.filter(minorOnly);
  const unverified = [...judged.unverified, ...reasoned.unverified, ...outOfScopeBlocking];
  // What the coordinator set aside without ruling on it, one line each, so a
  // reader sees every finding that did not reach a verdict tier. Borrowed:
  // "Declined to judge … nothing you set aside is dropped silently"
  // (Superpowers `skills/requesting-code-review/code-reviewer.md:43-48`, MIT).
  const declined_to_judge = [...unverified, ...outOfScopeNoted].map((f) => ({
    file: f.file ?? null,
    line: f.line ?? null,
    rule_key: f.rule_key ?? null,
    severity: f.severity ?? null,
    title: f.title ?? null,
    reason: f.suppressed_reason,
  }));
  // A trimmed finding that Pass 3 later removed is accounted for there, not here.
  const keptSet = new Set(reasoned.kept.map((f) => tripletKey(f)));
  const trimmed_count = judged.trimmed.filter((f) => keptSet.has(tripletKey(f))).length;

  // A reviewer counts as REPORTED only when a payload carrying its name
  // validated. `validateFindings` merely warns on a missing `reviewer`, so a
  // payload can fail schema anonymously: that is an unattributable payload, not
  // a reviewer called "unnamed reviewer". Naming one would invent an agent
  // nobody launched, and would double-count with expectedReviewers when the two
  // are in fact the same failure.
  const skippedNames = new Set();
  let unattributable_payloads = 0;
  for (const s of skipped) {
    if (typeof s.reviewer === 'string' && s.reviewer.length > 0) skippedNames.add(s.reviewer);
    else unattributable_payloads += 1;
  }
  const reported = new Set();
  for (const payload of reviewerPayloads) {
    const name = payload?.reviewer;
    if (typeof name === 'string' && name.length > 0 && !skippedNames.has(name)) reported.add(name);
  }
  const missing_reviewers = [...skippedNames];
  for (const r of options.expectedReviewers ?? []) {
    if (!reported.has(r) && !missing_reviewers.includes(r)) missing_reviewers.push(r);
  }

  const { verdict, counts, allow_blocked_by } = computeVerdict(reasoned.kept, {
    unverified,
    missingReviewers: missing_reviewers,
    unattributablePayloads: unattributable_payloads,
  });
  return {
    verdict,
    counts,
    findings: reasoned.kept,
    suppressed: [...judged.dropped, ...judged.unverified, ...reasoned.dropped, ...reasoned.unverified],
    unverified,
    skipped,
    missing_reviewers,
    unattributable_payloads,
    allow_blocked_by,
    declined_to_judge,
    trimmed_count,
  };
}

// ---- CLI shim ----------------------------------------------------------------

if (isMain(import.meta.url)) {
  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);
}