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); + }); +});