diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 2aa96d7fdb..ca12e53088 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -987,15 +987,36 @@ export async function runAiReviewForAdvisory( }, "ai_review_inconclusive"); } args.advisory.findings.push(...findings); + // #9432: `degraded` persists the per-attempt review diagnostics into `ai_review_cache.metadata_json`. + // Until now these existed ONLY as a PostHog capture property, which made the "why did this review produce + // no summary?" question unanswerable from the database or the container logs -- the 2026-07-27 + // investigation had to stop there, because reaching PostHog needs an interactive OAuth the incident + // responder may not have. The diagnostics are the discriminator that matters: `missing_assessment` (the + // model genuinely returned no narrative) vs `unparseable_output`/`empty_output` (it returned something + // parseModelReview could not read, i.e. no balanced JSON object) vs `provider_error` are three different + // bugs with three different fixes, and `metadata_json.inconclusive` alone cannot tell them apart. + // + // Written on the DEGRADED paths only, never on the healthy one. Every byte here lands in a D1 row, and + // the 2026-07-26 outage was this database hitting its 10 GB ceiling -- so this buys diagnosability for + // the small minority of reviews that actually failed, rather than growing all of them. The compact + // `model#attempt:status[:error]` strings come from formatReviewDiagnosticsForCapture, whose `error` field + // is errorMessage() output and never raw provider text, so the public/private boundary is unchanged. const metadataFor = ( notes: string | null | undefined, inlineFindings: InlineFinding[], + degraded?: boolean, ): Record => ({ rag: attributeReviewRagTelemetry(ragTelemetry, { notes, findings, inlineFindings, }), + ...(degraded + ? { + /* v8 ignore next -- current review runner always supplies diagnostics for completed AI attempts; the `?? []` is a type-level fallback for the optional field. */ + reviewDiagnostics: formatReviewDiagnosticsForCapture(result.reviewDiagnostics ?? []), + } + : {}), }); if (result.inconclusive && hasPublicReviewAssessment(result.advisoryNotes)) { return { @@ -1003,7 +1024,7 @@ export async function runAiReviewForAdvisory( reviewerCount: result.reviewerCount, inlineFindings: [], findings, - metadata: metadataFor(result.advisoryNotes, []), + metadata: metadataFor(result.advisoryNotes, [], true), cacheable: false, valueAssessment: result.valueAssessment ?? undefined, }; @@ -1026,7 +1047,7 @@ export async function runAiReviewForAdvisory( reviewerCount: result.reviewerCount, inlineFindings: [], findings, - metadata: metadataFor(null, []), + metadata: metadataFor(null, [], true), cacheable: false, }; } @@ -1068,7 +1089,7 @@ export async function runAiReviewForAdvisory( reviewerCount: result.reviewerCount, inlineFindings: [], findings, - metadata: metadataFor(null, []), + metadata: metadataFor(null, [], true), cacheable: false, }; } catch (error) { diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index f508e611aa..9089cee498 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -814,6 +814,46 @@ describe("runAiReviewForAdvisory", () => { captureSpy.mockRestore(); }); + it("#9432: persists the review diagnostics into metadata when no public notes were produced", async () => { + // The whole point: `metadata_json.inconclusive` says a review degraded but not WHY, and the diagnostics + // used to reach PostHog only -- unreadable during an incident without an interactive OAuth. An empty + // provider response must now be recoverable from the row itself as `empty_output`, which is what + // distinguishes "the model returned nothing" from "the model returned a narrative we then dropped". + const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: "" })), { + mode: "live", + settings: { aiReviewMode: "advisory" } as RepositorySettings, + advisory: advisory(), + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + const diagnostics = result?.metadata?.reviewDiagnostics as string[] | undefined; + expect(Array.isArray(diagnostics)).toBe(true); + expect(diagnostics?.length).toBeGreaterThan(0); + // Compact `model#attempt:status` form from formatReviewDiagnosticsForCapture -- the status is the payload. + expect(diagnostics?.every((entry) => /^[^#]+#\d+:/.test(entry))).toBe(true); + expect(diagnostics?.join("\n")).toContain("empty_output"); + }); + + it("#9432: omits the review diagnostics from metadata on a healthy review", async () => { + // The other side of the `degraded` branch. Diagnostics are deliberately NOT written on the success path: + // every byte lands in a D1 row, and this database hit its 10 GB ceiling on 2026-07-26. Diagnosability is + // bought only for the reviews that actually failed. + const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: defectJson() })), { + mode: "live", + settings: { aiReviewMode: "advisory" } as RepositorySettings, + advisory: advisory(), + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + expect(result?.notes).toBeTruthy(); + expect(result?.metadata).toBeDefined(); + expect(result?.metadata?.reviewDiagnostics).toBeUndefined(); + }); + it("#8790: stops retrying a model after a byte-identical repeat of a failed attempt (deterministic at temperature 0)", async () => { const adv = advisory(); let aiCalls = 0;