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

fix(review): original_severity names what the kept finding asserted

Browse source 
Review finding 08deae68.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-14 11:09:01 +02:00
commit 9529df7665
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 93 additions and 6 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -19,7 +19,11 @@
// What IS implemented, purely: Pass 1 (triplet dedup → survivor ranked by
// catalogue tier, then reviewer over engine, asserted severity, conformance,
// first seen; the survivor carries its group's highest asserted severity
// (review bc7f4e23) + detail concat + raised_by provenance),
// (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),
@ -98,6 +102,8 @@ export const UNVERIFIED_REASONS = Object.freeze([
// `/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` is never read here.
// `original_severity` is what the kept finding itself asserted, before Pass 1
// raised it to its group's severity (review 08deae68).
// The payload `reviewer` names lib/review/engine-findings.mjs writes, and the
// engine each one stands for.
@ -303,9 +309,13 @@ export function dedupByTriplet(findings) {
}
}
const id = computeFindingId(survivor.file, survivor.line, survivor.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 } : {}), id,
...(detail !== undefined ? { detail } : {}), raised_by,
...(raised ? { pass1_raised_from: survivor.severity } : {}),
});
}
return out;
@ -428,7 +438,10 @@ export function reasonablenessFilter(findings) {
// 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.
const { brief_lowered_from: _lowered, ...rest } = incoming;
// `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;
@ -447,11 +460,16 @@ export function reasonablenessFilter(findings) {
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 if (rule && f.severity !== rule.severity) {
} else if (rule) {
// 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.
kept.push({ ...f, severity: rule.severity, original_severity: f.severity });
// `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 } : {}),
});
} else {
kept.push(f);
}
@ -529,6 +547,12 @@ export function runContract(reviewerPayloads, options = {}) {
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
@ -543,7 +567,7 @@ export function runContract(reviewerPayloads, options = {}) {
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];
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"
@ -591,7 +615,7 @@ export function runContract(reviewerPayloads, options = {}) {
verdict,
counts,
findings: reasoned.kept,
suppressed: [...judged.dropped, ...judged.unverified, ...reasoned.dropped, ...reasoned.unverified],
suppressed: [...judgedDropped, ...judgedUnverified, ...reasoned.dropped, ...reasoned.unverified],
unverified,
skipped,
missing_reviewers,