From 1883bdae393e74aef532d4470b49a4fe4468fc83 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 23 Jul 2026 19:00:26 -0700 Subject: [PATCH] fix(review): surface per-model terminal errors and readable diagnostics in review-failure telemetry The ai_review_provider_exhausted summary only carried the LAST error across all models x attempts, so a fallback's circuit_open masked the primary's distinct terminal failure (a rate-limit 429) during the 2026-07-23 outage (LOOPOVER-2A) -- the single Sentry event pointed at the wrong provider. runWorkersOpinion now also tracks each model's own terminal error and logs the map alongside the unchanged last-error field. Both captureReviewFailure sites passed raw AiReviewDiagnostic[] into Sentry context, where the SDK's default normalizeDepth flattened every entry to the literal string "[Object]" (LOOPOVER-2B), erasing the model/attempt/status/ error detail. formatReviewDiagnosticsForCapture renders them as compact model#attempt:status[:error] strings that survive normalization. --- src/queue/ai-review-orchestration.ts | 8 ++++-- src/services/ai-review.ts | 21 ++++++++++++++ test/unit/ai-review-advisory.test.ts | 4 ++- test/unit/ai-review.test.ts | 42 ++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 3 deletions(-) diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index ad0c4ac201..527bab9c15 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -35,6 +35,7 @@ import { } from "../signals/focus-manifest"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import { + formatReviewDiagnosticsForCapture, hasPublicReviewAssessment, isEnabled, runLoopOverAiReview, @@ -801,8 +802,10 @@ export async function runAiReviewForAdvisory( ai_review_mode: args.settings.aiReviewMode, reviewer_count: result.reviewerCount, public_notes: hasPublicReviewAssessment(result.advisoryNotes), + // Compact strings, not the raw objects -- Sentry's normalizeDepth flattens nested entries to "[Object]" + // and destroys the per-attempt detail (LOOPOVER-2B); see formatReviewDiagnosticsForCapture. /* v8 ignore next -- current review runner always supplies diagnostics for completed AI attempts. */ - review_diagnostics: result.reviewDiagnostics ?? [], + review_diagnostics: formatReviewDiagnosticsForCapture(result.reviewDiagnostics ?? []), }, "ai_review_inconclusive"); } args.advisory.findings.push(...findings); @@ -872,8 +875,9 @@ export async function runAiReviewForAdvisory( head_sha: args.advisory.headSha, ai_review_mode: args.settings.aiReviewMode, reviewer_count: result.reviewerCount, + // Same "[Object]" flattening hazard as the inconclusive capture above (LOOPOVER-2B). /* v8 ignore next -- current review runner always supplies diagnostics for completed AI attempts. */ - review_diagnostics: result.reviewDiagnostics ?? [], + review_diagnostics: formatReviewDiagnosticsForCapture(result.reviewDiagnostics ?? []), configured_reviewers: env.AI_REVIEW_PLAN?.reviewers?.map((reviewer) => reviewer.model) ?? null, diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 1ea7c357c9..6cd71a00e0 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -488,6 +488,21 @@ export type AiReviewActualUsage = { costUsd?: number | undefined; }; +/** Render diagnostics as compact `model#attempt:status[:error]` strings for Sentry capture context. Passing the + * raw objects loses everything: Sentry's normalizeDepth flattens each nested entry to the literal string + * "[Object]", which erased exactly the model/attempt/status/error detail these entries exist to carry (the + * 2026-07-23 outage, LOOPOVER-2B, was diagnosable only from separate provider-failure events because of this). + * Strings survive normalization verbatim. The `error` field is errorMessage() output, never raw provider text, + * so including it here keeps the "withholds unsafe provider text" boundary intact. */ +export function formatReviewDiagnosticsForCapture( + diagnostics: readonly AiReviewDiagnostic[], +): string[] { + return diagnostics.map( + (diagnostic) => + `${diagnostic.model}#${diagnostic.attempt}:${diagnostic.status}${diagnostic.error ? `:${diagnostic.error}` : ""}`, + ); +} + type ReviewerOpinionOutcome = { review: ModelReview | null; fallbackNote?: string | undefined; @@ -1142,6 +1157,10 @@ async function runWorkersOpinion( // Track the last provider error so we can fail-LOUD once ALL models × attempts are exhausted (below). Per-attempt // logs are warn (noisy retries, skipped by the central Sentry forwarder); the exhausted summary is error (#26). let lastError: unknown; + // ALSO track each model's own terminal error: `lastError` alone lets the fallback's failure MASK the primary's + // distinct one in the exhausted summary -- during the 2026-07-23 outage (LOOPOVER-2A) the fallback's + // circuit_open hid the primary's rate-limit 429, so the single Sentry event pointed at the wrong provider. + const errorsByModel: Record = {}; let lastUnparseable: | { model: string; attempt: number; responseChars: number; hasJsonObject: boolean; responseSnippet: string } | undefined; @@ -1257,6 +1276,7 @@ async function runWorkersOpinion( }), ); lastError = error; + errorsByModel[model] = errorMessage(error); // A CLI timeout is not transient -- the same model retrying the same oversized/complex diff will almost // certainly time out again. Stop retrying THIS model (the fallback below still gets its own full retry // budget, since a different model/config may not share the same timeout) instead of burning up to 3x @@ -1284,6 +1304,7 @@ async function runWorkersOpinion( primary, fallback, error: errorMessage(lastError), + errorsByModel, }), ); } diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index a169ae9899..abd32c0bea 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -456,8 +456,10 @@ describe("runAiReviewForAdvisory", () => { head_sha: "sha3", public_notes: true, reviewer_count: 1, + // Compact strings, not objects -- raw diagnostic objects flatten to "[Object]" in Sentry context + // (LOOPOVER-2B); formatReviewDiagnosticsForCapture renders model#attempt:status[:error]. review_diagnostics: expect.arrayContaining([ - expect.objectContaining({ status: "unparseable_output" }), + expect.stringContaining(":unparseable_output"), ]), }), "ai_review_inconclusive", diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 9e582cc859..0d9621e129 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -4,6 +4,7 @@ import { BEST_REVIEW_MODELS, buildTestEvidencePromptSection, callAiProvider, + formatReviewDiagnosticsForCapture, INCOHERENT_DIFF_ASSESSMENT, isIncoherentDiffBail, isStructuralProviderConfigError, @@ -3366,6 +3367,47 @@ describe("pure helpers", () => { warnSpy.mockRestore(); }); + it("REGRESSION (LOOPOVER-2A): the exhausted log carries each model's OWN terminal error, so the fallback's failure cannot mask the primary's", async () => { + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + // The 2026-07-23 outage shape: the primary rate-limits (429 → no same-model retry), the fallback fails + // structurally (circuit_open). `error` alone reported only the fallback's message, hiding the 429. + const run = vi.fn(async (model: string) => { + throw new Error(model === "primary-model" ? "claude_code_error_429" : "circuit_open: provider down"); + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const result = await runWorkersOpinion(env, "primary-model", "fallback-model", "sys", "user", 256); + expect(result).toEqual({ review: null }); + const exhausted = logSpy.mock.calls + .map((c) => c[0]) + .find((l) => typeof l === "string" && l.includes("ai_review_provider_exhausted")); + expect(exhausted).toBeDefined(); + expect(JSON.parse(exhausted as string)).toMatchObject({ + event: "ai_review_provider_exhausted", + // Still the last error overall (unchanged Sentry grouping)… + error: expect.stringContaining("circuit_open"), + // …but now ALSO each model's own terminal failure, keyed by model. + errorsByModel: { + "primary-model": "claude_code_error_429", + "fallback-model": "circuit_open: provider down", + }, + }); + logSpy.mockRestore(); + warnSpy.mockRestore(); + }); + + it("formatReviewDiagnosticsForCapture renders compact model#attempt:status[:error] strings (raw objects flatten to \"[Object]\" in Sentry context — LOOPOVER-2B)", () => { + const diagnostics: AiReviewDiagnostic[] = [ + { model: "claude-code", attempt: 0, status: "provider_error", error: "claude_code_error_429" }, + { model: "codex", attempt: 1, status: "unparseable_output", responseChars: 12, hasJsonObject: false }, + ]; + expect(formatReviewDiagnosticsForCapture(diagnostics)).toEqual([ + "claude-code#0:provider_error:claude_code_error_429", + "codex#1:unparseable_output", + ]); + expect(formatReviewDiagnosticsForCapture([])).toEqual([]); + }); + it("logs unparseable exhaustion separately when the model runs but returns unparseable output, including a response snippet for diagnosis (#observability-unparseable)", async () => { const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); const run = vi.fn(async () => ({ response: "not json at all" }));