From c73c3c2cc7b30ae68bf0491b946562b2e8412838 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 20 Jul 2026 22:14:39 -0700 Subject: [PATCH] fix(review): retry when an AI review response is missing its assessment runWorkersOpinion accepted the first structurally-parseable response even when the required assessment field was empty, so a model that returned valid JSON with blockers/nits but no narrative summary was treated as a success and published with no visible review text. Now a missing-assessment response is retried like a parse failure, with the best incomplete response kept as a last-resort fallback instead of surfacing a null review. --- src/services/ai-review.ts | 46 ++++++++++++++++++++++++++++-- test/unit/ai-review.test.ts | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 8007879fc9..5a0eec447a 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -452,7 +452,7 @@ export type ModelReview = { export type AiReviewDiagnostic = { model: string; attempt: number; - status: "parsed" | "empty_output" | "unparseable_output" | "provider_error"; + status: "parsed" | "empty_output" | "unparseable_output" | "provider_error" | "missing_assessment"; responseChars?: number | undefined; hasJsonObject?: boolean | undefined; error?: string | undefined; @@ -1091,6 +1091,14 @@ async function runWorkersOpinion( let lastUnparseable: | { model: string; attempt: number; responseChars: number; hasJsonObject: boolean; responseSnippet: string } | undefined; + // #missing-assessment-retry: the system prompt declares `assessment` REQUIRED and never empty, but a model + // occasionally returns valid JSON with real blockers/nits and an empty assessment anyway -- parseModelReview + // correctly parses that (it only returns null when EVERYTHING is empty), so without this, the very first + // such response would have been accepted immediately and surfaced downstream as a misleading "did not include + // a separate narrative summary" placeholder instead of retrying for a real one. Kept as a fallback candidate + // ONLY for the case where every attempt across every model comes back this way -- degrades to exactly today's + // behavior in that (expected to be rare) worst case, never worse. + let bestIncompleteReview: ModelReview | null = null; const models = fallback && fallback !== primary ? [primary, fallback] : [primary]; for (const [modelIndex, model] of models.entries()) { if (modelIndex > 0) { @@ -1132,10 +1140,27 @@ async function runWorkersOpinion( const usage = coerceAiUsage(result); const usageFields = usage ? { usage } : {}; const parsed = parseModelReview(text); - if (parsed) { + if (parsed && parsed.assessment.trim() !== "") { diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); return { review: parsed }; } + if (parsed) { + // Valid JSON, real blockers/nits/suggestions, but the REQUIRED assessment came back empty -- + // keep it as a last-resort candidate and retry for a real one instead of accepting immediately. + bestIncompleteReview = parsed; + diagnostics.push({ model, attempt, status: "missing_assessment", responseChars: text.length, hasJsonObject: true, ...usageFields }); + console.warn( + JSON.stringify({ + level: "warn", + event: "ai_review_missing_assessment", + model, + attempt, + blockersCount: parsed.blockers.length, + nitsCount: parsed.nits.length, + }), + ); + continue; + } const hasJsonObject = Boolean(extractLastJsonObject(text)); const trimmedText = text.trim(); const status = trimmedText ? "unparseable_output" : "empty_output"; @@ -1223,6 +1248,23 @@ async function runWorkersOpinion( }), ); } + // Every attempt across every model came back with valid blockers/nits/suggestions but no assessment -- + // surface it as a real, alertable outage signal (this should be rare; the retry above exists specifically + // to make it rare) but still return the usable content rather than discarding it. Matches today's exact + // downstream degrade (fallbackPublicAssessment) as the worst case, never worse. + if (bestIncompleteReview) { + console.log( + JSON.stringify({ + level: "error", + event: "ai_review_missing_assessment_exhausted", + primary, + fallback, + blockersCount: bestIncompleteReview.blockers.length, + nitsCount: bestIncompleteReview.nits.length, + }), + ); + return { review: bestIncompleteReview }; + } return { review: null }; } diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 4b43f37bc0..2e4bac5ccb 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3104,6 +3104,62 @@ describe("pure helpers", () => { expect(totalAttempts).toBe(2); // 1 per model, NOT 3 per model (6 total) -- each model's own bail is deliberate. }); + it("REGRESSION (#missing-assessment-retry): runWorkersOpinion retries when the model returns real blockers/nits but an empty assessment, despite the prompt requiring it", async () => { + let attempts = 0; + const run = vi.fn(async () => { + attempts += 1; + if (attempts === 1) return { response: reviewJson({ assessment: "" }) }; + return { response: reviewJson({ assessment: "The change looks reasonable and focused." }) }; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const diagnostics: Array<{ status: string; model: string; attempt: number }> = []; + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never); + expect(parsed.review?.assessment).toBe("The change looks reasonable and focused."); + expect(attempts).toBe(2); // 1 missing-assessment attempt, then a real one -- same model, no fallback needed. + expect(diagnostics[0]).toMatchObject({ model: "primary", attempt: 0, status: "missing_assessment" }); + expect(diagnostics[1]).toMatchObject({ model: "primary", attempt: 1, status: "parsed" }); + }); + + it("REGRESSION (#missing-assessment-retry): falls back to the last incomplete-but-usable review when EVERY attempt across EVERY model comes back with an empty assessment", async () => { + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const run = vi.fn(async () => ({ + response: reviewJson({ assessment: "", blockers: [], nits: ["Edge case on empty input is untested.", "Naming could be clearer."] }), + })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256); + // Real content is preserved (the exact degrade this PR set out to avoid discarding) even though no + // attempt ever produced the required assessment field. + expect(parsed.review?.assessment).toBe(""); + expect(parsed.review?.nits).toEqual(["Edge case on empty input is untested.", "Naming could be clearer."]); + expect(run).toHaveBeenCalledTimes(6); // 3 attempts x 2 models -- the full budget, since nothing here is a deliberate bail. + const exhausted = logSpy.mock.calls + .map((c) => c[0]) + .find((l) => typeof l === "string" && l.includes("ai_review_missing_assessment_exhausted")); + expect(exhausted).toBeDefined(); + expect(JSON.parse(exhausted as string)).toMatchObject({ + level: "error", + event: "ai_review_missing_assessment_exhausted", + primary: "primary", + fallback: "fallback", + blockersCount: 0, + nitsCount: 2, + }); + logSpy.mockRestore(); + warnSpy.mockRestore(); + }); + + it("does not treat the deliberate INCOHERENT_DIFF_ASSESSMENT bail as a missing assessment (it's a non-empty sentinel string)", async () => { + const run = vi.fn(async () => ({ + response: reviewJson({ assessment: INCOHERENT_DIFF_ASSESSMENT, blockers: [], nits: [], suggestions: [] }), + })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const diagnostics: Array<{ status: string }> = []; + const parsed = await runWorkersOpinion(env, "m", "m", "sys", "user", 256, diagnostics as never); + expect(parsed.review).toBeNull(); // INCOHERENT_DIFF_ASSESSMENT parses to null (see parseModelReview) + expect(diagnostics.some((d) => d.status === "missing_assessment")).toBe(false); + }); + 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.