voyage/lib/review/coordinator-contract.mjs
Kjell Tore Guttormsen d88245c06e
feat(review): the review is a gate - verdict in the file, CC engine, project memory
Ordre 5 (wave 1, P11 + P5), review_version 1.1.

- K7: `verdict: ALLOW | WARN | BLOCK` is required in review.md (strict
  error, soft warning for 1.0 reviews). New CLI lib/review/verdict-gate.mjs:
  0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide (never green); a verdict
  greener than the file's own BLOCKERs or JSON block is exit 2. Realpath
  main guard (the old file:// guard exits 0 under a symlink - measured).
- No finding is struck for length: succinctness trims only MINOR/SUGGESTION
  text (catalogue tier), keeps BLOCKER/MAJOR whole, counts trims; the
  coordinator lists what it declined to judge.
- Claude Code's /code-review and /security-review run as the engine via the
  Skill tool (never --fix/--comment/--post/ultra); lib/review/engine-findings.mjs
  turns their findings into a reviewer payload; two catalogue keys
  (CODE_REVIEW_DEFECT, SECURITY_REVIEW_VULNERABILITY), catalogue 12 -> 14.
  An engine that did not run forbids ALLOW.
- Every BLOCKER is written as a numbered `### Step R<n>` with its source
  finding (strict error REVIEW_MISSING_REMEDIATION_STEP).
- P5: lib/review/recurrence.mjs - a finding class in two review.md files
  (current + git history, no new store) becomes one proposal, routed by the
  catalogue's memory_route; written only between Voyage markers in
  .claude/rules/voyage-review-lessons.md under a size budget, CLAUDE.md gets
  a pointer only; the operator approves every line (Phase 8.7).
- Workflow substrate: catalogue enum in step; a missing reviewer or engine
  forbids ALLOW there too (it returned ALLOW on zero findings before).
- Fixtures review-run-A/B said WARN over a BLOCKER; now BLOCK.

Borrowed and credited in code: Superpowers (declined-to-judge), ai-native-sdlc
(cap only nits; mistake/fix/check), Spec Kit (managed block; converge steps),
Pocock (retro routing), Kiro (verdict file + gate; evidence per lesson),
Cherny (engine -> brief control order; review -> memory; budget).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-06 19:45:52 +02:00

457 lines
19 KiB
JavaScript

// 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 (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 { realpathSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
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',
'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;
}
/**
* 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 = trimText(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[] }} [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);
const unverified = [...judged.unverified, ...reasoned.unverified];
// 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.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 ----------------------------------------------------------------
function isMain() {
try {
return realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url));
} catch {
return false;
}
}
if (isMain()) {
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);
}