diff --git a/lib/review/coordinator-contract.mjs b/lib/review/coordinator-contract.mjs new file mode 100644 index 0000000..f2afd8d --- /dev/null +++ b/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 }} + */ +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, 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] \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); +} diff --git a/tests/lib/coordinator-contract.test.mjs b/tests/lib/coordinator-contract.test.mjs new file mode 100644 index 0000000..08daa3b --- /dev/null +++ b/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)); +});