734 lines
33 KiB
JavaScript
734 lines
33 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.
|
|
// - Nothing about ids beyond the triplet: `computeFindingId(file, line,
|
|
// rule_key)` is the one id, over `rule_key#k` for the k-th row on one
|
|
// triplet (computeFindingIds). 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 → survivor ranked by
|
|
// catalogue tier, then reviewer over engine, asserted severity, conformance,
|
|
// first in the group's order; the survivor carries its group's highest asserted severity
|
|
// (review bc7f4e23) + detail concat + raised_by provenance; when that raises
|
|
// it, `pass1_raised_from` keeps what the survivor itself asserted, read by
|
|
// Pass 3 and never returned: `original_severity` is what the kept finding
|
|
// itself asserted, before Pass 1 raised it to its group's severity (review
|
|
// 08deae68)),
|
|
// 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),
|
|
// computeFindingIds (finding-id.mjs, triplet), validateFindings
|
|
// (findings-schema.mjs). Triplet key format mirrors
|
|
// scripts/bakeoff-armA-merge.mjs `dedupByTriplet`; raised_by provenance mirrors
|
|
// lib/review/plan-review-dedup.mjs.
|
|
|
|
import { SEVERITY_VALUES, RULE_KEYS, getRule } from './rule-catalogue.mjs';
|
|
import { computeFindingIds } from '../parsers/finding-id.mjs';
|
|
import { validateFindings } from './findings-schema.mjs';
|
|
import { ENGINES } from './engine-findings.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, or
|
|
* a negative line; a line past the end of the file does not refute) 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), and `accuracy:line-out-of-range` by its
|
|
* Pass 2 Accuracy (a line past the end of the file: the citation is wrong, the
|
|
* claim is unchecked, so a line number alone never refutes); 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',
|
|
'accuracy:line-out-of-range',
|
|
]);
|
|
// The brief-conflict mark (`contradicts_brief`) is in neither set on purpose:
|
|
// it is a field on a KEPT finding, never a `suppressed_reason`. As a reason it
|
|
// would fail closed through `classifySuppression`, land the finding in
|
|
// `unverified` and forbid ALLOW — the opposite of what the mark is for.
|
|
|
|
// ---- The brief-conflict rule ---------------------------------------------------
|
|
//
|
|
// The one place that decides when a finding marked as contradicting the brief
|
|
// (`contradicts_brief: "<the Non-Goal or stated choice, verbatim>"`) may drop
|
|
// to MINOR. Only an engine finding (`engine` `code-review` or `security-review`)
|
|
// is ever marked; only a `/code-review` finding at the MAJOR tier ever drops,
|
|
// to MINOR, writing `brief_lowered_from: 'MAJOR'`. A BLOCKER and any
|
|
// `/security-review` finding keep their severity and only carry the mark.
|
|
// The verdict gate (verdict-gate.mjs) imports `illegalBriefLowering`; the
|
|
// opt-in workflow review mirrors it. `original_severity` (what the kept finding
|
|
// itself asserted before Pass 1 raised it, review 08deae68) is never read here.
|
|
|
|
// The payload `reviewer` names lib/review/engine-findings.mjs writes, and the
|
|
// engine each one stands for.
|
|
const ENGINE_OF_PAYLOAD = Object.freeze(Object.fromEntries(
|
|
Object.entries(ENGINES).map(([engine, spec]) => [spec.reviewer, engine])));
|
|
|
|
const isEngineFinding = (f) => Object.hasOwn(ENGINES, f.engine ?? '');
|
|
|
|
function carries(f, field) {
|
|
return f?.[field] !== undefined && f?.[field] !== null;
|
|
}
|
|
|
|
function isMark(v) {
|
|
return typeof v === 'string' && v.trim().length > 0;
|
|
}
|
|
|
|
/**
|
|
* True when a finding, before the lowering, may drop to MINOR: a non-empty
|
|
* string mark, `engine: 'code-review'`, a known `rule_key` whose catalogue tier
|
|
* is MAJOR, and severity MAJOR. Anything else (missing engine, unknown key,
|
|
* non-string mark, a BLOCKER-tier or MINOR-tier key) → false.
|
|
* @param {object} f
|
|
* @returns {boolean}
|
|
*/
|
|
export function lowerableBriefConflict(f) {
|
|
return markedMajorCodeReview(f) && f.severity === 'MAJOR';
|
|
}
|
|
|
|
function markedMajorCodeReview(f) {
|
|
return isMark(f?.contradicts_brief) &&
|
|
f.engine === 'code-review' &&
|
|
getRule(f.rule_key)?.severity === 'MAJOR';
|
|
}
|
|
|
|
function isLegalBriefLowering(f) {
|
|
return markedMajorCodeReview(f) &&
|
|
f.severity === 'MINOR' &&
|
|
f.brief_lowered_from === 'MAJOR';
|
|
}
|
|
|
|
/**
|
|
* True when what the coordinator wrote is not the one legal shape and (a)
|
|
* carries `brief_lowered_from`, or (b) carries a mark without an engine, or
|
|
* (c) carries a mark below its catalogue tier, or (d) carries a mark with a
|
|
* missing or unknown `rule_key` (fail-closed), or (e) is an engine finding
|
|
* below its catalogue tier, marked or not. An unmarked reviewer finding (no
|
|
* `engine`) without `brief_lowered_from` is never judged by this rule.
|
|
* @param {object} f
|
|
* @returns {boolean}
|
|
*/
|
|
export function illegalBriefLowering(f) {
|
|
if (f === null || typeof f !== 'object') return false;
|
|
if (isLegalBriefLowering(f)) return false;
|
|
const marked = carries(f, 'contradicts_brief');
|
|
const engine = isEngineFinding(f);
|
|
const rule = getRule(f.rule_key);
|
|
const belowTier = rule !== null && severityRank(f.severity) > severityRank(rule.severity);
|
|
if (carries(f, 'brief_lowered_from')) return true; // (a)
|
|
if (marked && !engine) return true; // (b)
|
|
if (marked && rule === null) return true; // (d)
|
|
if (engine && belowTier) return true; // (c) and (e): a marked finding is an engine one past (b)
|
|
return false;
|
|
}
|
|
|
|
/**
|
|
* 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;
|
|
}
|
|
// "Came from an engine" is read from the payload, never from an `engine`
|
|
// field a reviewer wrote on its own finding (decision 13, operator 12.10):
|
|
// an engine payload's findings get `engine` from its `reviewer`, every other
|
|
// payload's findings lose any `engine` they carry.
|
|
const eng = Object.hasOwn(ENGINE_OF_PAYLOAD, payload.reviewer) ? ENGINE_OF_PAYLOAD[payload.reviewer] : null;
|
|
for (const f of payload.findings) {
|
|
const { engine: _claimed, ...rest } = f;
|
|
findings.push({
|
|
...rest,
|
|
...(eng ? { engine: eng } : {}),
|
|
reviewer: f.reviewer ?? payload.reviewer ?? f.owner_reviewer ?? null,
|
|
});
|
|
}
|
|
}
|
|
return { findings, skipped };
|
|
}
|
|
|
|
/**
|
|
* Pass 1 — dedup by (file, line, rule_key) triplet. The survivor is ranked by
|
|
* (1) the catalogue tier (`getRule(rule_key)?.severity`, else the finding's own
|
|
* severity; lower `severityRank` wins), (2) a reviewer-agent finding over an
|
|
* engine finding, (3) among findings of one kind the higher asserted severity,
|
|
* (4) the conformance reviewer, (5) the first in the group's order (the order
|
|
* the two passes below place the members in, never the arrival order). It is
|
|
* written with the highest severity any finding in its group asserted, so
|
|
* dropping a duplicate never lowers the group (review bc7f4e23; plan 2026-10-11-files-backticks-trial4,
|
|
* ADR "Pass 1 survivor order and group severity"). 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) {
|
|
// One reviewer never duplicates itself: its second finding on a triplet under
|
|
// another title is another finding and opens a group of its own, which gets
|
|
// its own row and the id over `rule_key#2` (voyage-lekerepo trial 5,
|
|
// finding 7). Inside one triplet the findings are grouped in two passes, so
|
|
// the order the reviewers' findings arrive in never changes which row a
|
|
// finding joins (plan 2026-10-20-fix-round-serious-findings, ADR "ORD: two
|
|
// passes over the triplet, title first"): (1) every finding with a title
|
|
// joins the class of its normalized title, over all of the triplet's
|
|
// findings; (2) the title classes, in alphabetical order of the normalized
|
|
// title, each join the first earlier group whose every member fits every
|
|
// finding of the class (another reviewer, or the same title), else they form
|
|
// a group of their own; findings without a title come last, in canonical
|
|
// order (reviewer, detail, the whole finding), each joining the first group
|
|
// it fits. Members inside a group keep the order the passes place them in:
|
|
// the classes alphabetically, each class's findings in canonical order, the
|
|
// untitled findings last in canonical order. Rank (5) of the survivor reads
|
|
// that order, so a full tie never follows arrival order either: the survivor
|
|
// decides Pass 2's actionability and Pass 3's brief-conflict lowering, and so
|
|
// the verdict. A triplet's groups keep the order of their earliest input
|
|
// index, so the rows and `#k` ids stay in input order.
|
|
const tripletGroups = new Map();
|
|
findings.forEach((f, i) => {
|
|
const key = tripletKey(f);
|
|
if (!tripletGroups.has(key)) tripletGroups.set(key, []);
|
|
tripletGroups.get(key).push({ f, i });
|
|
});
|
|
const groups = [...tripletGroups.values()].flatMap((entries) => groupTriplet(entries));
|
|
const out = [];
|
|
for (const group of groups) {
|
|
// Ranked as the JSDoc above says. A brief-conflict mark is not carried over
|
|
// from a group member to the survivor: when the survivor is unmarked, the
|
|
// group stays unmarked and is judged at its full severity. For a catalogue
|
|
// key the tier always ties inside one triplet (the rule_key is shared).
|
|
const tier = (f) => severityRank(getRule(f.rule_key)?.severity ?? f.severity);
|
|
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 = isEngineFinding(f);
|
|
const es = isEngineFinding(survivor);
|
|
if (ef !== es) {
|
|
if (!ef) survivor = f;
|
|
continue;
|
|
}
|
|
const rf = severityRank(f.severity);
|
|
const rs = severityRank(survivor.severity);
|
|
if (rf !== rs) {
|
|
if (rf < rs) survivor = f;
|
|
continue;
|
|
}
|
|
if (isConformance(f.reviewer) && !isConformance(survivor.reviewer)) survivor = f;
|
|
}
|
|
let severity = survivor.severity;
|
|
for (const f of group) {
|
|
if (severityRank(f.severity) < severityRank(severity)) severity = f.severity;
|
|
}
|
|
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}.`;
|
|
}
|
|
}
|
|
// When the group raised the survivor, Pass 3 still needs what the survivor
|
|
// itself asserted for `original_severity` (review 08deae68).
|
|
const raised = severityRank(severity) < severityRank(survivor.severity);
|
|
out.push({
|
|
...survivor, ...(severity !== undefined ? { severity } : {}),
|
|
...(detail !== undefined ? { detail } : {}), raised_by,
|
|
...(raised ? { pass1_raised_from: survivor.severity } : {}),
|
|
});
|
|
}
|
|
// One id per row here; runContract counts the written ids again over the
|
|
// final list (after Pass 2 and 3), the way fill-finding-ids counts review.md.
|
|
const ids = computeFindingIds(out);
|
|
return out.map((f, i) => ({ ...f, id: ids[i] }));
|
|
}
|
|
|
|
/** A finding's title as the grouping compares it: trimmed, lower case. */
|
|
function normTitle(f) {
|
|
return String(f.title).trim().toLowerCase();
|
|
}
|
|
|
|
function sameTitle(a, b) {
|
|
if (a.title === undefined || b.title === undefined) return true;
|
|
return normTitle(a) === normTitle(b);
|
|
}
|
|
|
|
/** Code-unit order of two strings (never the locale's). */
|
|
function cmpStr(a, b) {
|
|
return a < b ? -1 : a > b ? 1 : 0;
|
|
}
|
|
|
|
/**
|
|
* The groups of one triplet's findings, in the two passes and the order
|
|
* `dedupByTriplet` describes. `entries` are `{ f, i }` (the finding and its
|
|
* input index). Returns arrays of findings.
|
|
* @param {Array<{f: object, i: number}>} entries
|
|
* @returns {object[][]}
|
|
*/
|
|
function groupTriplet(entries) {
|
|
const fits = (m, f) => m.reviewer !== f.reviewer || sameTitle(m, f);
|
|
const canon = (x, y) => cmpStr(String(x.f.reviewer ?? ''), String(y.f.reviewer ?? ''))
|
|
|| cmpStr(String(x.f.detail ?? ''), String(y.f.detail ?? ''))
|
|
|| cmpStr(JSON.stringify(x.f), JSON.stringify(y.f));
|
|
const classes = new Map();
|
|
const untitled = [];
|
|
for (const e of entries) {
|
|
if (e.f.title === undefined) {
|
|
untitled.push(e);
|
|
continue;
|
|
}
|
|
const t = normTitle(e.f);
|
|
if (!classes.has(t)) classes.set(t, []);
|
|
classes.get(t).push(e);
|
|
}
|
|
const groups = [];
|
|
const place = (members) => {
|
|
let g = groups.find((grp) => grp.every((m) => members.every((e) => fits(m.f, e.f))));
|
|
if (!g) {
|
|
g = [];
|
|
groups.push(g);
|
|
}
|
|
g.push(...members);
|
|
};
|
|
for (const t of [...classes.keys()].sort(cmpStr)) place(classes.get(t).sort(canon));
|
|
for (const e of untitled.sort(canon)) place([e]);
|
|
return groups
|
|
.map((g) => ({ first: Math.min(...g.map((e) => e.i)), members: g.map((e) => e.f) }))
|
|
.sort((a, b) => a.first - b.first)
|
|
.map((g) => g.members);
|
|
}
|
|
|
|
/**
|
|
* 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. A finding the brief rule will lower (Pass 3) is
|
|
* judged at MINOR, the tier it will be counted at.
|
|
* @param {object} f
|
|
* @returns {string}
|
|
*/
|
|
function effectiveSeverity(f) {
|
|
if (lowerableBriefConflict(f)) return 'MINOR';
|
|
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) and
|
|
* route an unknown rule_key to `unverified`; 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`.
|
|
*
|
|
* The brief rule runs here too: a finding `lowerableBriefConflict` accepts is
|
|
* set to MINOR with `brief_lowered_from: 'MAJOR'`; every other finding gets
|
|
* the catalogue reset above.
|
|
* @param {object[]} findings
|
|
* @returns {{ kept: object[], dropped: object[], unverified: object[] }}
|
|
*/
|
|
export function reasonablenessFilter(findings) {
|
|
const kept = [];
|
|
const dropped = [];
|
|
const unverified = [];
|
|
for (const incoming of findings) {
|
|
// The brief rule, first: only this pass writes `brief_lowered_from`, so an
|
|
// incoming one is removed; only an engine finding is ever marked, so a mark
|
|
// on any other finding is removed. Every finding written here then makes
|
|
// `illegalBriefLowering` false.
|
|
// `pass1_raised_from` (Pass 1) names what the finding itself asserted
|
|
// before its group raised it; it is read here and never written out.
|
|
const { brief_lowered_from: _lowered, pass1_raised_from: raisedFrom, ...rest } = incoming;
|
|
const own = raisedFrom ?? rest.severity;
|
|
let f = rest;
|
|
if (!isEngineFinding(f) && 'contradicts_brief' in f) {
|
|
const { contradicts_brief: _mark, ...unmarked } = f;
|
|
f = unmarked;
|
|
}
|
|
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); // never null: RULE_KEYS checked above
|
|
if (lowerableBriefConflict(f)) {
|
|
// The one legal lowering: a marked /code-review MAJOR on a MAJOR-tier key.
|
|
kept.push({ ...f, severity: 'MINOR', brief_lowered_from: 'MAJOR' });
|
|
} else {
|
|
// Today's catalogue reset. A marked engine finding arrives at its tier
|
|
// (engine-findings.mjs), so it lands here only when its severity was
|
|
// changed on the way, and the reset puts it back with its mark kept.
|
|
// `original_severity` names what the kept finding itself asserted
|
|
// (`own`), never the severity Pass 1 lent it (review 08deae68).
|
|
kept.push({
|
|
...f, severity: rule.severity,
|
|
...(own !== rule.severity ? { original_severity: own } : {}),
|
|
});
|
|
}
|
|
}
|
|
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);
|
|
// Pass 2's drops and unverified come straight from Pass 1: its
|
|
// `pass1_raised_from` bookkeeping is removed there too, so no returned
|
|
// finding carries it (Pass 3 removes it from everything it sees).
|
|
const withoutPass1 = ({ pass1_raised_from: _raised, ...f }) => f;
|
|
const judgedDropped = judged.dropped.map(withoutPass1);
|
|
const judgedUnverified = judged.unverified.map(withoutPass1);
|
|
// 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 = [...judgedUnverified, ...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.
|
|
// Matched on Pass 1's id, which is one per row (a triplet can hold two rows).
|
|
const keptSet = new Set(reasoned.kept.map((f) => f.id));
|
|
const trimmed_count = judged.trimmed.filter((f) => keptSet.has(f.id)).length;
|
|
// The ids written are counted over the final list, as fill-finding-ids counts
|
|
// review.md's JSON: a row whose triplet sibling was set aside gets the plain id.
|
|
const finalIds = computeFindingIds(reasoned.kept);
|
|
const kept = reasoned.kept.map((f, i) => ({ ...f, id: finalIds[i] }));
|
|
|
|
// 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: kept,
|
|
suppressed: [...judgedDropped, ...judgedUnverified, ...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);
|
|
}
|