diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 6e3d213236..8007879fc9 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -793,6 +793,23 @@ export function parseModelReview(text: string): ModelReview | null { } } +/** True when the model's raw response is specifically the deliberate INCOHERENT_DIFF_ASSESSMENT bail (see that + * constant's own prompt text) rather than a generic parse failure. parseModelReview collapses both into the + * same `null` -- correct for its own contract, since neither yields a usable review -- but the retry loop needs + * to tell them apart: an incoherent-diff bail is the model's deliberate, confident answer about THIS diff and + * will not change on a same-model retry, unlike a truncated/malformed response that might parse fine next time. + * Mirrors parseModelReview's own extraction so this can never disagree with what that function actually parsed. */ +export function isIncoherentDiffBail(text: string): boolean { + const jsonText = extractLastJsonObject(text); + if (!jsonText) return false; + try { + const obj = JSON.parse(jsonText) as Record; + return typeof obj.assessment === "string" && obj.assessment.trim() === INCOHERENT_DIFF_ASSESSMENT; + } catch { + return false; + } +} + // Aggregate ceiling across ALL optional context sections combined (#3900). Each section below already // enforces its OWN per-section cap (FILE_CONTENT_BUDGET, MAX_CONTEXT_CHARS, MAX_PROMPT_CHARS, // MAX_ENRICHMENT_PROMPT_SECTION_CHARS...), but nothing previously bounded the COMBINED total: with every @@ -1141,6 +1158,11 @@ async function runWorkersOpinion( }), ); } + // #ops-review-burst: an INCOHERENT_DIFF_ASSESSMENT bail is the model's deliberate, confident answer about + // THIS diff -- not a truncated/malformed response that might parse fine on a same-model retry. Stop + // retrying this model (same reasoning as the CLI-timeout/429/structural-config breaks below); the + // fallback model below still gets its own full retry budget, since it may reach a different verdict. + if (isIncoherentDiffBail(text)) break; } catch (error) { // Fail-LOUD (#1566): a provider/CLI failure (e.g. the claude-code CLI absent → spawn ENOENT, or an auth/API // error) must be VISIBLE, not silently swallowed into a "no usable output" review. Log every failed attempt; diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 0c7f0593ea..4b43f37bc0 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -4,6 +4,8 @@ import { BEST_REVIEW_MODELS, buildTestEvidencePromptSection, callAiProvider, + INCOHERENT_DIFF_ASSESSMENT, + isIncoherentDiffBail, isStructuralProviderConfigError, resolveEffectiveAiReviewOnMerge, resolveEffectiveAiReviewPlan, @@ -3074,6 +3076,49 @@ describe("pure helpers", () => { expect(run).toHaveBeenCalledTimes(2); // 1 primary (structural failure) + 1 fallback (succeeded on its first try). }); + it("runWorkersOpinion stops retrying a model after ONE INCOHERENT_DIFF_ASSESSMENT bail, but the fallback still gets its full retry budget (#ops-review-burst)", async () => { + let primaryAttempts = 0; + const run = vi.fn(async (model: string) => { + if (model === "fallback") return { response: reviewJson() }; + primaryAttempts += 1; + return { response: reviewJson({ assessment: INCOHERENT_DIFF_ASSESSMENT, blockers: [], nits: [], suggestions: [] }) }; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const diagnostics: Array<{ status: string; model: string }> = []; + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + expect(parsed.review?.assessment).toContain("reasonable"); + expect(primaryAttempts).toBe(1); // NOT 3 -- the model's own deliberate bail will not change on a same-model retry. + expect(run).toHaveBeenCalledTimes(2); // 1 primary (incoherent-diff bail) + 1 fallback (succeeded on its first try). + expect(diagnostics[0]).toMatchObject({ model: "primary", attempt: 0, status: "unparseable_output" }); + }); + + it("runWorkersOpinion exhausts all providers when EVERY model bails on INCOHERENT_DIFF_ASSESSMENT, without burning the full retry budget on either", async () => { + let totalAttempts = 0; + const run = vi.fn(async () => { + totalAttempts += 1; + return { response: reviewJson({ assessment: INCOHERENT_DIFF_ASSESSMENT, blockers: [], nits: [], suggestions: [] }) }; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256); + expect(parsed.review).toBeNull(); + expect(totalAttempts).toBe(2); // 1 per model, NOT 3 per model (6 total) -- each model's own bail is deliberate. + }); + + it("isIncoherentDiffBail recognizes exactly the model's own INCOHERENT_DIFF_ASSESSMENT text, not a generic parse failure or a look-alike assessment", () => { + expect(isIncoherentDiffBail(reviewJson({ assessment: INCOHERENT_DIFF_ASSESSMENT }))).toBe(true); + // Real-world shape (LOOPOVER-29): empty blockers/nits/suggestions alongside the bail assessment. + expect(isIncoherentDiffBail(reviewJson({ assessment: INCOHERENT_DIFF_ASSESSMENT, blockers: [], nits: [], suggestions: [] }))).toBe(true); + expect(isIncoherentDiffBail(reviewJson({ assessment: "The change looks reasonable and focused." }))).toBe(false); + expect(isIncoherentDiffBail(reviewJson({ assessment: `${INCOHERENT_DIFF_ASSESSMENT} (extra prose)` }))).toBe(false); + expect(isIncoherentDiffBail("not json at all")).toBe(false); + expect(isIncoherentDiffBail("")).toBe(false); + // A JSON object whose assessment isn't a string at all (extractLastJsonObject still finds the object). + expect(isIncoherentDiffBail(JSON.stringify({ assessment: 42 }))).toBe(false); + // Brace-balanced (extractLastJsonObject finds it) but not valid JSON (single-quoted, not double-quoted) -- + // exercises the try/catch around JSON.parse, not just the "no JSON object at all" early return above. + expect(isIncoherentDiffBail("{ 'assessment': 'not valid JSON' }")).toBe(false); + }); + it("isStructuralProviderConfigError matches only codex's own structural-config error messages, not other Errors or non-Error throws (GITTENSORY-K/8)", () => { expect(isStructuralProviderConfigError(new Error("codex_auth_not_configured: ~/.codex/auth.json not found"))).toBe(true); expect(isStructuralProviderConfigError(new Error("codex_no_auth: auth.json missing or expired"))).toBe(true);