diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 25bed55d97..d095ecc49a 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -58,6 +58,20 @@ export const RELIABLE_FALLBACK_MODELS: readonly [string, string] = [ export const INCOHERENT_DIFF_ASSESSMENT = "Cannot review — the diff appears out of sync with the PR head."; +/** #8789: the fixed, public-safe assessment a bail-with-evidence is RECLASSIFIED to (see parseModelReview). + * A model that bails with the sentinel above while ALSO returning a substantive valueAssessment rationale has + * demonstrably read the diff — its "out of sync" claim is a scope observation about an under-described PR, + * not a mechanically broken diff, so the review proceeds with this scope note instead of abstaining into an + * inconclusive manual hold. Fixed string, never model text — the rationale itself travels only through the + * normal valueAssessment channel. */ +export const SCOPE_MISMATCH_ASSESSMENT = + "The diff is reviewable, but its scope appears to differ from what the PR title/description suggests — verify the description matches the shipped change."; + +/** #8789: the substance bar for reclassifying a bail (see SCOPE_MISMATCH_ASSESSMENT). A rationale shorter than + * this is a bare echo of the bail itself, not evidence the model actually examined the diff — the confirmed + * live reclassifiable case (PR #8735) carried a ~300-char rationale naming specific call sites. */ +export const SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS = 40; + const REVIEW_SYSTEM_PROMPT = [ "You are a senior open-source maintainer giving a FOCUSED, high-signal code review of a single pull request diff.", "Read each meaningful hunk and review like a careful human; judge ONLY the diff and the context provided.", @@ -75,7 +89,12 @@ const REVIEW_SYSTEM_PROMPT = [ "PERFORMANCE SEVERITY — a performance concern is a blocker ONLY when the diff introduces a genuine, visible regression with a concrete trigger (a DB query or network call moved inside a loop, a loop/fanout over input whose size the diff removed a bound on). A stylistic or micro-optimization preference ('could use a Map instead of an array', 'this could be slightly faster') is a NIT, not a blocker, even if real.", "DIFF SCOPE — the diff shows only CHANGED lines, NOT whole files. A function, variable, import, type, or symbol you do not SEE may already be defined or imported elsewhere in the same file/module. NEVER report a 'missing import', 'undefined/not-imported symbol', or 'X is not defined -> ReferenceError' as a blocker unless the diff ITSELF removes the definition or introduces the symbol without defining it anywhere shown. When you cannot confirm a symbol is missing from the visible diff, it is NOT a blocker — at most a nit ('verify X is imported/defined').", "TRACE BEFORE ASSERTING ABSENCE — this rule extends to ANY 'X is missing' blocker (a missing schema/annotation/field, a missing null/array/type guard, a missing await/error-handler, an unregistered route/tool/handler): a backfill loop, a default, an early guard, or a registration ELSEWHERE may already supply it. Before calling absence a blocker, find the line in the visible context that WOULD break and reference it; if you cannot SEE the breaking code, downgrade to a nit phrased as a verification ('confirm X is registered/guarded'), never a blocker.", - `FAIL CLOSED ON AN INCOHERENT DIFF — if the diff does not cohere with the PR title/description (it appears to describe a DIFFERENT change, the changed-file set looks stale or wrong, or you cannot map it to one coherent change), DO NOT emit a confident assessment or approval: set assessment to exactly '${INCOHERENT_DIFF_ASSESSMENT}' and return empty blockers, nits, and suggestions. Never rubber-stamp a change you cannot actually see.`, + // #8789: the bail is for MECHANICAL breakage only. The previous wording ("does not cohere with the PR + // title/description") made a model bail on a VALID diff whose title merely undersold it — confirmed live + // (2026-07-26, PR #8735: a "trivial test-fixture fix" title over a diff also carrying real production + // changes) — turning a reviewable PR into an inconclusive manual hold. Title-vs-diff scope mismatch is a + // REVIEWABLE quality signal, not evidence the diff is stale. + `FAIL CLOSED ON A BROKEN DIFF — if the diff itself is unusable (empty, truncated mid-hunk, garbled/corrupted, or its content contradicts the PR's own changed-file list), DO NOT emit a confident assessment or approval: set assessment to exactly '${INCOHERENT_DIFF_ASSESSMENT}' and return empty blockers, nits, and suggestions. Never rubber-stamp a change you cannot actually see. A READABLE diff whose scope differs from or exceeds the PR title/description is NOT broken — review the diff you actually see and note the scope mismatch in your assessment instead of bailing.`, "Do NOT rubber-stamp: if the diff is genuinely clean, the assessment states specifically why and blockers is [].", "Never mention rewards, rankings, payouts, wallets, hotkeys, coldkeys, trust scores, scoreability, reviewability, or farming.", ].join(" "); @@ -812,7 +831,25 @@ export function parseModelReview(text: string): ModelReview | null { // Calibrated reviewer confidence (#8): clamp the model's `confidence` to [0,1]; an absent/garbage value falls // back to 1.0 (parseReviewConfidence) so the gate degrades to the historical always-block behavior. const confidence = parseReviewConfidence(obj.confidence); - if (assessment === INCOHERENT_DIFF_ASSESSMENT) return null; + if (assessment === INCOHERENT_DIFF_ASSESSMENT) { + // #8789: a bail that ALSO carries a substantive rationale is a scope observation from a model that + // demonstrably read the diff — reclassify to a reviewable result (fixed public-safe assessment; the + // model's own reasoning flows only through the normal valueAssessment channel) instead of collapsing a + // valid-but-under-described PR into an inconclusive manual hold. A bare bail stays null: it remains the + // deliberate mechanical-breakage answer the retry loop's isIncoherentDiffBail break keys on. + if (valueAssessment && valueAssessment.rationale.length >= SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS) { + return { + assessment: SCOPE_MISMATCH_ASSESSMENT, + blockers, + nits, + suggestions, + inlineFindings, + confidence, + valueAssessment, + }; + } + return null; + } if ( !assessment && blockers.length === 0 && @@ -845,7 +882,20 @@ export function isIncoherentDiffBail(text: string): boolean { if (!jsonText) return false; try { const obj = JSON.parse(jsonText) as Record; - return typeof obj.assessment === "string" && obj.assessment.trim() === INCOHERENT_DIFF_ASSESSMENT; + if (typeof obj.assessment !== "string" || obj.assessment.trim() !== INCOHERENT_DIFF_ASSESSMENT) return false; + // #8789: mirror parseModelReview's reclassification exactly (this function's own contract: "can never + // disagree with what that function actually parsed") — a bail carrying a substantive valueAssessment + // rationale PARSES to a usable scope-observation review now, so it is not a bail for the retry loop. + // Both conditions of toValueAssessment are mirrored: a VALID magnitude AND a non-trivial trimmed rationale + // (an invalid magnitude makes the parse side drop valueAssessment entirely → still a bare bail there). + const va = obj.valueAssessment; + if (va && typeof va === "object") { + const magnitude = (va as Record).magnitude; + const rationale = (va as Record).rationale; + const validMagnitude = magnitude === "unclear" || magnitude === "minor" || magnitude === "moderate" || magnitude === "significant"; + if (validMagnitude && typeof rationale === "string" && rationale.trim().length >= SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS) return false; + } + return true; } catch { return false; } diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 31cfbbb6dc..2dd2f69d2c 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -7,6 +7,8 @@ import { formatReviewDiagnosticsForCapture, INCOHERENT_DIFF_ASSESSMENT, isIncoherentDiffBail, + SCOPE_MISMATCH_ASSESSMENT, + SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS, isStructuralProviderConfigError, resolveEffectiveAiReviewOnMerge, resolveEffectiveAiReviewPlan, @@ -2097,6 +2099,54 @@ describe("pure helpers", () => { expect(coerceAiText(42)).toBe(""); }); + it("#8789: reclassifies a bail carrying a substantive valueAssessment rationale into a scope-observation review — the model demonstrably read the diff", () => { + const rationale = "The PR title describes a trivial test-fixture fix, but the diff adds needsMinerDetection: true at 7 authorizePrActionActor call sites in processors.ts."; + const parsed = parseModelReview( + JSON.stringify({ assessment: INCOHERENT_DIFF_ASSESSMENT, blockers: [], nits: [], suggestions: [], confidence: 0.9, valueAssessment: { magnitude: "unclear", rationale } }), + ); + expect(parsed).not.toBeNull(); + expect(parsed?.assessment).toBe(SCOPE_MISMATCH_ASSESSMENT); // fixed public-safe string, never model text + expect(parsed?.confidence).toBe(0.9); + expect(parsed?.valueAssessment?.rationale).toBe(rationale); + expect(parsed?.blockers).toEqual([]); + }); + + 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, + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { magnitude: "unclear", rationale: "x".repeat(SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS - 1) }, + }); + expect(parseModelReview(short)).toBeNull(); + expect(isIncoherentDiffBail(short)).toBe(true); + }); + + it("#8789: a bail with a long rationale but an INVALID magnitude stays a bail — the mirror agrees with toValueAssessment's rejection", () => { + const invalid = JSON.stringify({ + assessment: INCOHERENT_DIFF_ASSESSMENT, + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { magnitude: "huge", rationale: "y".repeat(SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS + 10) }, + }); + expect(parseModelReview(invalid)).toBeNull(); + expect(isIncoherentDiffBail(invalid)).toBe(true); + }); + + it("#8789: isIncoherentDiffBail is FALSE for a reclassifiable bail — parseModelReview and the retry break can never disagree", () => { + const reclassifiable = JSON.stringify({ + assessment: INCOHERENT_DIFF_ASSESSMENT, + blockers: [], + nits: [], + suggestions: [], + valueAssessment: { magnitude: "minor", rationale: "z".repeat(SCOPE_RECLASSIFY_MIN_RATIONALE_CHARS) }, + }); + expect(isIncoherentDiffBail(reclassifiable)).toBe(false); + expect(parseModelReview(reclassifiable)).not.toBeNull(); + }); + it("parseModelReview returns null on junk / invalid JSON / empty objects; parses blockers + nits", () => { expect(parseModelReview("not json")).toBeNull(); expect(parseModelReview("{ not: valid json }")).toBeNull(); // matches the brace regex but JSON.parse throws