open/voyage
1
0
Fork 0
Code Issues Releases 2 Activity

fix(review): an anonymous invalid payload is unattributable, not a reviewer named "unnamed reviewer"

Browse source 
Follow-up defect in the reviewer accounting added by e2aec01, found by review
and confirmed by probe before fixing.

validateFindings only WARNS on a missing `reviewer` field, so a payload can
fail schema while carrying no name. ingest then records `reviewer: null`, and
runContract turned that null into the literal reviewer name "unnamed reviewer".

MEASURED before the fix:
  runContract([{findings:[{file:'x.mjs',line:1,rule_key:'NOPE',severity:'MAJOR'}]}],
              {expectedReviewers:['code-correctness-reviewer']})
  -> missing_reviewers = ["unnamed reviewer", "code-correctness-reviewer"]
One failure, two entries, one of them an agent nobody launched. The
`reported.delete(s.reviewer)` line was also inert for that case, since a null
name was never in the set to begin with.

Fix: skipped payloads are split by whether they carry a name. Named ones go to
missing_reviewers as before; anonymous ones increment the new
`unattributable_payloads` count, which forbids ALLOW on its own - so stripping
a reviewer name from a payload cannot restore ALLOW, and the floor does not
depend on the caller passing expectedReviewers. `allow_blocked_by` reports the
two facts separately: `missing-reviewer:<name>` and `unattributable-payload (n)`.

The old behaviour never produced a false ALLOW - it failed in the safe
direction - but it named a reviewer that did not exist, which is the kind of
output an operator would chase.

Iron Law: two failing tests first (double entry; anonymous-payload-alone must
forbid ALLOW), then the fix.

Also verified in this pass, by temporarily adding a fake reason to
UNVERIFIED_REASONS: the prose-vocabulary pin does go red when a reason is
declared in the lib but missing from agents/review-coordinator.md. A pin that
cannot fail is not a pin.

Suite 1034 (1032/0/2) -> 1036 (1034/0/2), 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-09-01 22:54:31 +02:00
commit 20cdc22803
4 changed files with 60 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

41
lib/review/coordinator-contract.mjs
View file

@ -267,7 +267,7 @@ export function reasonablenessFilter(findings) {
* severity is reviewer-asserted and was never substantiated.
*
* @param {object[]} findings
* @param {{ unverified?: object[], missingReviewers?: string[] }} [options]
* @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 = {}) {
@ -278,6 +278,7 @@ export function computeVerdict(findings, options = {}) {
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) {
@ -286,6 +287,7 @@ export function computeVerdict(findings, options = {}) {
}
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';
@ -299,11 +301,12 @@ export function computeVerdict(findings, options = {}) {
* 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 payload
* failed schema validation and was thrown away at ingest, lands in
* `missing_reviewers` and 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.
* 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
@ -311,7 +314,7 @@ export function computeVerdict(findings, options = {}) {
*
* @param {Array<{reviewer?: string, findings: object[]}>} reviewerPayloads
* @param {{ expectedReviewers?: string[] }} [options]
* @returns {{ verdict: string, counts: Record<string, number>, findings: object[], suppressed: object[], unverified: object[], skipped: object[], missing_reviewers: string[], allow_blocked_by: string[] }}
* @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[] }}
*/
export function runContract(reviewerPayloads, options = {}) {
const { findings: ingested, skipped } = ingest(reviewerPayloads);
@ -320,16 +323,24 @@ export function runContract(reviewerPayloads, options = {}) {
const reasoned = reasonablenessFilter(judged.kept);
const unverified = [...judged.unverified, ...reasoned.unverified];
// 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) {
if (typeof payload?.reviewer === 'string' && payload.reviewer.length > 0) reported.add(payload.reviewer);
}
for (const s of skipped) reported.delete(s.reviewer);
const missing_reviewers = [];
for (const s of skipped) {
const name = s.reviewer ?? 'unnamed reviewer';
if (!missing_reviewers.includes(name)) missing_reviewers.push(name);
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);
}
@ -337,6 +348,7 @@ export function runContract(reviewerPayloads, options = {}) {
const { verdict, counts, allow_blocked_by } = computeVerdict(reasoned.kept, {
unverified,
missingReviewers: missing_reviewers,
unattributablePayloads: unattributable_payloads,
});
return {
verdict,
@ -346,6 +358,7 @@ export function runContract(reviewerPayloads, options = {}) {
unverified,
skipped,
missing_reviewers,
unattributable_payloads,
allow_blocked_by,
};
}