From 79aae337de7e066ce551335351a787160e2300fb Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 13:55:06 -0700 Subject: [PATCH] fix(ai-review): verify that reviewers actually agree, drop blocker authority from a bailed review #9074 -- consensusDefectOf returned a defect whenever BOTH reviewers had a non-empty blockers list and NEVER compared the texts, while the doc comment two lines above asserted the property the code did not implement ("Requiring two independent models to AGREE is itself the precision mechanism"). Reviewer A's "SQL injection in src/db.ts" plus reviewer B's "the new helper lacks a doc comment" produced a critical finding published on the contributor's PR as "AI reviewers agree on a likely critical defect: SQL injection in src/db.ts" -- a false claim of agreement, given as the reason their work was auto-closed under one-shot rules. Agreement is now established by comparing blocker texts (normalized significant-token overlap, with a lower bar when both cite the same path and share another term), across ANY pair from the two lists since reviewers order findings differently. Two reviewers flagging DIFFERENT defects is neither consensus nor silence -- it is now a SPLIT, gating on the weaker of the two confidences. Previously `split` required exactly one side to have flagged, so both flagging produced no finding at all: strictly weaker than one doing so. A blank blocker entry no longer counts as "flagged" anywhere, which also removes the case where two empty strings manufactured a critical closing finding under the generic "AI reviewers agree" title. #9087 -- the system prompt tells a model that cannot read the diff to bail with INCOHERENT_DIFF_ASSESSMENT and return empty blockers. A model that bailed, violated that instruction, and happened to write a >=40-char rationale had its blockers promoted into a usable review by #8789's reclassification. Under `combine: "single"` (our live claude-code+ollama config) a lone blocker becomes a full ai_consensus_defect -- severity critical, published as agreement -- and a close under aiReviewGateMode: block. Blockers are now dropped on reclassification; the valueAssessment (#8789's actual purpose), nits, and suggestions are kept. The published title also stops claiming agreement when only one reviewer ran. #9085 (partial) -- adds slop_risk_above_threshold and surface_lane_reject to CONFIGURED_GATE_BLOCKER_SIGNAL_CODES. Both gate real closes but were absent, so their reversals recorded under a different id (or not at all) and the per-rule precision check in downgradeCloseToHold could never apply to them -- the same drift shape as the backtest_regression omission this list's doc comment was written about. NOT done, and the issue's premise is wrong: #9085 claims severity "never determines consequence" and is read only by presentational code. It is not -- conclusionForSeverity maps critical -> action_required, so raising these findings' severity to match their consequence flipped advisory-mode repos from neutral to action_required and broke four gate tests. Reverted; the severity mismatch needs that coupling decoupled first, which is its own change. Closes #9074 Closes #9087 Tests: 11 consensus/split cases (unrelated defects are not consensus, same defect in different words is, any-pair matching, weaker-confidence rule, disagreement becomes a split with min confidence, one-sided split unchanged, clean pair yields nothing, unverifiable text never agrees, shared-path booster, single shared token is not agreement), a reclassification test proving blockers are dropped while the valueAssessment survives, and 3 taxonomy tests. 100% line+branch coverage on all added lines; 985/985 across 9 suites. --- src/queue/ai-review-orchestration.ts | 9 +- src/rules/advisory.ts | 10 ++ src/services/ai-review.ts | 107 ++++++++++++++++--- test/unit/ai-review.test.ts | 149 +++++++++++++++++++++------ test/unit/rules.test.ts | 19 ++++ 5 files changed, 251 insertions(+), 43 deletions(-) diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 5948ab2df..0ad9e9ba4 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -763,7 +763,14 @@ export async function runAiReviewForAdvisory( findings.push({ code: "ai_consensus_defect", severity: "critical", - title: `AI reviewers agree on a likely critical defect: ${result.consensusDefect.title}`, + // #9074/#9087: only claim AGREEMENT when more than one reviewer actually ran. Under `combine: "single"` + // (the live claude-code+ollama config) a lone reviewer's blocker reaches this same finding, and the + // plural copy asserted a corroboration that was never checked — on a contributor's PR, as the stated + // reason their work was auto-closed. + title: + result.plannedReviewerCount > 1 + ? `AI reviewers agree on a likely critical defect: ${result.consensusDefect.title}` + : `AI review flagged a likely critical defect: ${result.consensusDefect.title}`, detail: result.consensusDefect.detail, action: "Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.", diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index a676751af..1703e2285 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -212,6 +212,16 @@ export const CONFIGURED_GATE_BLOCKER_SIGNAL_CODES: readonly string[] = Object.fr "backtest_regression", "lockfile_tamper_risk", CLA_CONSENT_MISSING_CODE, + // #9085 taxonomy drift: both of these gate REAL closes but were absent from this list, so their reversals + // recorded under a different id (or not at all) and the per-rule precision check in downgradeCloseToHold + // could never apply to them — the exact drift this list's own doc comment promises against, and the same + // shape as the backtest_regression omission fixed above. + // • slop_risk_above_threshold: pushed directly into `blockers`, never routed through + // resolveConfiguredGateMode, and its reversals were recorded under `slop_gate_score`. + // • surface_lane_reject: live in CONCRETE_EVIDENCE_BLOCKER_CODES but invisible to + // recordConfiguredGateBlockerSignals and therefore to the entire calibration corpus. + "slop_risk_above_threshold", + "surface_lane_reject", ...GATE_SCORE_SIGNAL_CODES, ]); diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 9ad6e002e..c77508db6 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -917,7 +917,16 @@ export function parseModelReview(text: string): ModelReview | null { if (valueAssessment && valueAssessment.rationale.length >= SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS) { return { assessment: SCOPE_MISMATCH_ASSESSMENT, - blockers, + // #9087: blockers are DROPPED on reclassification. The system prompt instructs a model that cannot + // read the diff to bail with INCOHERENT_DIFF_ASSESSMENT *and return empty blockers*; a model that + // bails, violates that instruction, and happens to write a >=40-char rationale was having its + // blockers promoted into a usable review. Under `combine: "single"` (our live claude-code+ollama + // config) combineReviews turns a lone blocker into a full ai_consensus_defect -- severity critical, + // published as "AI reviewers agree on a likely critical defect" for a ONE-reviewer run -- which is a + // close under aiReviewGateMode: block. A model that just said it could not read the diff has not + // earned blocker authority over it. The valueAssessment (the entire point of #8789) is kept, so a + // valid-but-under-described PR still gets its scope observation instead of an inconclusive hold. + blockers: [], nits, suggestions, inlineFindings, @@ -1963,22 +1972,85 @@ export function composeImprovementSignal( * sub-`aiReviewCloseConfidence`-floor confidence changes what happens next (manual-review hold by default, * non-blocking under `advisory_only`, or ignored under `one_shot`) rather than adding a second floor on top of * the block decision itself. */ +/** #9074: the two reviewers' blocker texts, normalized for comparison — lowercased, punctuation stripped, + * short/stopword tokens dropped. Deliberately content-only: model phrasing varies, so identity is judged on + * the substantive terms (identifiers, file paths, defect nouns) both reviewers used. */ +function significantBlockerTokens(text: string): Set { + const STOPWORDS = new Set([ + "this", "that", "with", "from", "have", "has", "does", "not", "the", "and", "for", "are", "but", "its", + "into", "when", "will", "would", "could", "should", "there", "here", "which", "while", "your", "you", + "code", "change", "changes", "line", "lines", "file", "files", "pull", "request", "review", + ]); + return new Set( + text + .toLowerCase() + .replace(/[^a-z0-9/._-]+/g, " ") + .split(/\s+/) + .filter((token) => token.length >= 4 && !STOPWORDS.has(token)), + ); +} + +/** #9074: do these two blocker texts describe the SAME defect? Jaccard overlap over significant tokens, with a + * lower bar when both cite the same path-like token (a shared `src/db.ts` plus any shared term is far stronger + * evidence of the same finding than raw word overlap). Neither side having significant tokens is unverifiable + * and therefore NOT agreement — this function only ever returns true on positive evidence. */ +export function blockersDescribeSameDefect(first: string, second: string): boolean { + const a = significantBlockerTokens(first); + const b = significantBlockerTokens(second); + if (a.size === 0 || b.size === 0) return false; + let shared = 0; + for (const token of a) if (b.has(token)) shared += 1; + if (shared === 0) return false; + // shared > 0 above guarantees both sets are non-empty, so the union is always >= 1 — no zero guard needed. + const jaccard = shared / (a.size + b.size - shared); + if (jaccard >= 0.4) return true; + // Same file/path cited by both, plus at least one other shared term. + const sharedPathish = [...a].some((token) => b.has(token) && (token.includes("/") || token.includes("."))); + return sharedPathish && shared >= 2; +} + +/** + * #9074: a consensus defect requires the two reviewers to actually AGREE. This previously returned a defect + * whenever BOTH reviewers had a non-empty blockers list, never comparing the texts — so reviewer A's "SQL + * injection in src/db.ts" and reviewer B's "the new helper lacks a doc comment" produced + * `{title: "SQL injection in src/db.ts", split: false}`, published on the contributor's PR as "AI reviewers + * agree on a likely critical defect: SQL injection in src/db.ts" as the reason their work was auto-closed + * under one-shot rules. That is a false claim of agreement, and the doc comment right above the old code + * already asserted the property ("Requiring two independent models to AGREE is itself the precision + * mechanism") that the code did not implement. + * + * Two reviewers flagging DIFFERENT defects is not silence and not consensus — it is a SPLIT, which the caller + * now derives from a null return plus both-flagged (see combineReviews). The split still gates; it just no + * longer claims corroboration that was never checked. + */ +/** #9074: a whitespace-only entry is not a flagged blocker. Previously `blockers: [""]` counted as "flagged", + * so two reviewers who each returned a blank entry produced a full consensus defect under the generic + * "AI reviewers agree on a likely blocking defect" title — agreement about nothing at all. */ +export function realBlockersOf(review: ModelReview): string[] { + return review.blockers.filter((blocker) => blocker.trim().length > 0); +} + export function consensusDefectOf( a: ModelReview, b: ModelReview, ): AiConsensusDefect | null { - if (a.blockers.length === 0 || b.blockers.length === 0) return null; - const title = toPublicSafe( - a.blockers[0] || - b.blockers[0] || - "AI reviewers agree on a likely blocking defect", - ); + const aBlockers = realBlockersOf(a); + const bBlockers = realBlockersOf(b); + if (aBlockers.length === 0 || bBlockers.length === 0) return null; + // Agreement is established when ANY of A's blockers matches ANY of B's — reviewers routinely order their + // findings differently, so a positional comparison would miss real consensus. + const agreedPair = aBlockers + .flatMap((first) => bBlockers.map((second) => [first, second] as const)) + .find(([first, second]) => blockersDescribeSameDefect(first, second)); + if (!agreedPair) return null; + // Cite the blocker the two reviewers actually AGREED on (#9074), not whichever happened to be first. Both + // halves of the pair came from realBlockersOf, so each is non-blank — no fallback chain is reachable here. + const agreed = agreedPair[0]; + const title = toPublicSafe(agreed); if (!title) return null; // unsafe title → drop the block entirely (fail-safe) // Cite ONLY the primary blocker (not every finding joined together) so the Gate's "why blocked" reason // stays focused on the single core defect instead of repeating the whole blockers list. (#focused-reviews) - const detail = - toPublicSafe(a.blockers[0] || b.blockers[0] || "") ?? - "Both AI reviewers independently flagged a concrete must-fix defect in this change."; + const detail = title; // The consensus is only as strong as the WEAKER reviewer: take the minimum of the two confidences (#8). return { title, detail, confidence: Math.min(a.confidence, b.confidence) }; } @@ -2371,7 +2443,10 @@ export function combineReviews( const [a, b] = reviews; if (a && b) { const defect = consensusDefectOf(a, b); - const split = !defect && a.blockers.length > 0 !== b.blockers.length > 0; + // #9074: a split is EITHER "exactly one reviewer flagged" (the historical case) OR "both flagged but they + // described different defects". Without the second arm, two reviewers each raising an uncorroborated + // concern produced no finding at all — strictly weaker than one reviewer doing so. + const split = !defect && (realBlockersOf(a).length > 0 || realBlockersOf(b).length > 0); // On a split, exactly one reviewer flagged a blocker — carry THAT reviewer's confidence so the // `ai_review_split` finding gates on the same calibrated floor a consensus defect would. return split @@ -2379,7 +2454,15 @@ export function combineReviews( defect, split, inconclusive: false, - splitConfidence: a.blockers.length > 0 ? a.confidence : b.confidence, + // #9074: when BOTH flagged (disagreeing), neither is corroborated — take the MINIMUM so the split + // gates on the weaker reviewer, matching consensusDefectOf's own min() rule. When only one flagged, + // this is unchanged: that reviewer's own confidence. + splitConfidence: + realBlockersOf(a).length > 0 && realBlockersOf(b).length > 0 + ? Math.min(a.confidence, b.confidence) + : realBlockersOf(a).length > 0 + ? a.confidence + : b.confidence, } : { defect, split, inconclusive: false }; } diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 0b149e201..69d67b785 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -7,6 +7,7 @@ import { callAiProvider, formatReviewDiagnosticsForCapture, INCOHERENT_DIFF_ASSESSMENT, + blockersDescribeSameDefect, isIncoherentDiffBail, SCOPE_MISMATCH_ASSESSMENT, SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS, @@ -2158,6 +2159,32 @@ describe("pure helpers", () => { expect(parsed?.blockers).toEqual([]); }); + // #9087: the system prompt tells a model that cannot read the diff to bail AND return empty blockers. A model + // that bails, violates that instruction, and happens to write a >=40-char rationale had its blockers promoted + // into a usable review — and under `combine: "single"` (the live claude-code+ollama config) a lone blocker + // becomes a full ai_consensus_defect: severity critical, published as agreement, and a CLOSE under + // aiReviewGateMode: block. A model that just said it could not read the diff has not earned blocker authority. + it("#9087: a reclassified bail DROPS its blockers while keeping the valueAssessment", () => { + const rationale = "The PR title describes a trivial test-fixture fix, but the diff adds needsMinerDetection: true at 7 call sites."; + const parsed = parseModelReview( + JSON.stringify({ + assessment: INCOHERENT_DIFF_ASSESSMENT, + blockers: ["This change introduces a critical security hole"], + nits: ["a nit"], + suggestions: ["a suggestion"], + confidence: 0.9, + valueAssessment: { magnitude: "unclear", rationale }, + }), + ); + expect(parsed?.assessment).toBe(SCOPE_MISMATCH_ASSESSMENT); + // The whole point: no blocker authority survives the bail. + expect(parsed?.blockers).toEqual([]); + // ...but #8789's actual purpose (the scope observation) is preserved, along with the softer channels. + expect(parsed?.valueAssessment?.rationale).toBe(rationale); + expect(parsed?.nits).toEqual(["a nit"]); + expect(parsed?.suggestions).toEqual(["a suggestion"]); + }); + it("#8789: a bail with a SHORT rationale (a bare echo) stays a bail — null parse, bail-true for the retry break", () => { const short = JSON.stringify({ assessment: INCOHERENT_DIFF_ASSESSMENT, @@ -3167,38 +3194,100 @@ describe("pure helpers", () => { ).toBeNull(); // unsafe → dropped }); - it("consensusDefectOf falls back to b's blocker when a's is blank", () => { - const a = { - assessment: "", - suggestions: [], - nits: [], - blockers: [""], - inlineFindings: [], - confidence: 1, - }; - const b = { - assessment: "", - suggestions: [], - nits: [], - blockers: ["Race condition in src/x.ts"], - inlineFindings: [], - confidence: 1, - }; - expect(consensusDefectOf(a, b)?.title).toBe("Race condition in src/x.ts"); + // #9074: consensusDefectOf returned a defect whenever BOTH reviewers had a non-empty blockers list, never + // comparing the texts — so "SQL injection in src/db.ts" and "the new helper lacks a doc comment" produced a + // critical finding published as "AI reviewers agree on a likely critical defect: SQL injection in src/db.ts", + // a false claim of agreement posted on a contributor's PR as the reason it was auto-closed. + describe("#9074: consensus requires the reviewers to actually agree", () => { + const base = { assessment: "", suggestions: [], nits: [], inlineFindings: [], confidence: 1 }; + + it("returns NO consensus when the two reviewers flagged unrelated defects", () => { + const a = { ...base, blockers: ["SQL injection in src/db.ts allows arbitrary query execution"] }; + const b = { ...base, blockers: ["the new helper lacks a doc comment"] }; + expect(consensusDefectOf(a, b)).toBeNull(); + }); + + it("returns a consensus defect when both describe the SAME defect in different words", () => { + const a = { ...base, blockers: ["Null dereference of `user.profile` in src/auth/session.ts"] }; + const b = { ...base, blockers: ["src/auth/session.ts dereferences user.profile without a null check"] }; + const out = consensusDefectOf(a, b); + expect(out).not.toBeNull(); + expect(out?.confidence).toBe(1); + }); + + it("matches ANY pair across the two lists, not just the first of each (reviewers order findings differently)", () => { + const a = { ...base, blockers: ["missing changelog entry", "Race condition in src/queue/worker.ts on shutdown"] }; + const b = { ...base, blockers: ["src/queue/worker.ts has a shutdown race condition"] }; + const out = consensusDefectOf(a, b); + expect(out).not.toBeNull(); + // ...and it cites the AGREED blocker, not a.blockers[0]. + expect(out?.title).toContain("Race condition"); + }); + + it("takes the WEAKER reviewer's confidence for the agreed defect", () => { + const a = { ...base, confidence: 0.9, blockers: ["Race condition in src/queue/worker.ts on shutdown"] }; + const b = { ...base, confidence: 0.4, blockers: ["src/queue/worker.ts has a shutdown race condition"] }; + expect(consensusDefectOf(a, b)?.confidence).toBe(0.4); + }); + + it("treats two DISAGREEING reviewers as a split, not as silence", () => { + const a = { ...base, confidence: 0.8, blockers: ["SQL injection in src/db.ts"] }; + const b = { ...base, confidence: 0.5, blockers: ["the new helper lacks a doc comment"] }; + const combined = combineReviews([a, b], { strategy: "consensus" }); + expect(combined.defect).toBeNull(); + // Before this, `split` required exactly one side to have flagged — so BOTH flagging produced no finding + // at all, strictly weaker than one reviewer flagging. + expect(combined.split).toBe(true); + // Neither is corroborated, so the split gates on the weaker reviewer. + expect(combined.splitConfidence).toBe(0.5); + }); + + it("still reports a one-sided split with that reviewer's own confidence (unchanged)", () => { + const a = { ...base, confidence: 0.7, blockers: ["SQL injection in src/db.ts"] }; + const b = { ...base, confidence: 0.2, blockers: [] }; + const combined = combineReviews([a, b], { strategy: "consensus" }); + expect(combined.split).toBe(true); + expect(combined.splitConfidence).toBe(0.7); + }); + + it("is NOT agreement when a blocker has no significant tokens to compare (unverifiable ⇒ never a defect)", () => { + // All tokens are short/stopwords, so there is nothing substantive to match on. + const vague = { ...base, blockers: ["it is bad"] }; + const real = { ...base, blockers: ["Null dereference in src/auth/session.ts"] }; + expect(blockersDescribeSameDefect("it is bad", "so is that")).toBe(false); + expect(consensusDefectOf(vague, real)).toBeNull(); + }); + + it("accepts a shared file path plus a second shared term even when raw word overlap is low", () => { + // Long, differently-worded reports that nonetheless cite the same file AND the same defect noun: the + // Jaccard ratio is dragged below 0.4 by the surrounding prose, but this is real agreement. + const first = "A subtle and intermittent deadlock arises inside src/queue/worker.ts whenever shutdown overlaps with an inflight claim operation"; + const second = "src/queue/worker.ts deadlock"; + expect(blockersDescribeSameDefect(first, second)).toBe(true); + }); + + it("does not treat a single shared path token alone as agreement", () => { + // Same file, entirely different defects — one shared token is not corroboration. + expect(blockersDescribeSameDefect("src/db.ts leaks a connection", "src/db.ts needs a comment")).toBe(false); + }); + + it("reports neither defect nor split when nobody flagged anything", () => { + const clean = { ...base, blockers: [] }; + const combined = combineReviews([clean, clean], { strategy: "consensus" }); + expect(combined).toMatchObject({ defect: null, split: false, inconclusive: false }); + }); }); - it("consensusDefectOf uses the default title + detail when BOTH reviewers' blockers are blank", () => { - const blank = { - assessment: "", - suggestions: [], - nits: [], - blockers: [""], - inlineFindings: [], - confidence: 1, - }; - const out = consensusDefectOf(blank, { ...blank, blockers: [""] }); - expect(out?.title).toContain("AI reviewers agree"); // both blockers[0] falsy → default title - expect(out?.detail).toContain("independently flagged"); // joined detail empty → default detail + // #9074: a blank blocker entry is not a flagged blocker, and two reviewers who each returned one are not in + // agreement about anything. These previously asserted the opposite — that `blockers: [""]` on both sides + // produced a full consensus defect under the generic "AI reviewers agree on a likely blocking defect" title, + // i.e. a critical, closing finding manufactured from two empty strings. + it("#9074: a blank blocker on either side is not a flagged blocker — no consensus defect", () => { + const blank = { assessment: "", suggestions: [], nits: [], blockers: [""], inlineFindings: [], confidence: 1 }; + const real = { ...blank, blockers: ["Null dereference in src/db.ts"] }; + expect(consensusDefectOf(blank, real)).toBeNull(); + expect(consensusDefectOf(real, blank)).toBeNull(); + expect(consensusDefectOf(blank, { ...blank, blockers: [""] })).toBeNull(); }); it("synthesizeDefect cites the FLAGGING reviewer's blocker + confidence, skipping an earlier clean reviewer (#8)", () => { diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index 8421a25bb..2e088e847 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -1,3 +1,4 @@ +import { CONFIGURED_GATE_BLOCKER_SIGNAL_CODES } from "../../src/rules/advisory"; import { describe, expect, it } from "vitest"; import { buildCheckRunAnnotations, @@ -1751,3 +1752,21 @@ function emptyCollisions(): CollisionReport { clusters: [], }; } + +// #9085 taxonomy drift: both of these gate REAL closes but were absent from the signal-codes list, so their +// reversals recorded under a different id (or not at all) and the per-rule precision check in +// downgradeCloseToHold could never apply to them — the same shape as the backtest_regression omission that +// this list's own doc comment was written about. +describe("CONFIGURED_GATE_BLOCKER_SIGNAL_CODES covers every code that can close a PR (#9085)", () => { + it("includes slop_risk_above_threshold, which is pushed directly into blockers", () => { + expect(CONFIGURED_GATE_BLOCKER_SIGNAL_CODES).toContain("slop_risk_above_threshold"); + }); + + it("includes surface_lane_reject, which is live in CONCRETE_EVIDENCE_BLOCKER_CODES", () => { + expect(CONFIGURED_GATE_BLOCKER_SIGNAL_CODES).toContain("surface_lane_reject"); + }); + + it("has no duplicate entries (a duplicate would double-record a single reversal)", () => { + expect(new Set(CONFIGURED_GATE_BLOCKER_SIGNAL_CODES).size).toBe(CONFIGURED_GATE_BLOCKER_SIGNAL_CODES.length); + }); +});