fix(review): surface per-model terminal errors and readable diagnostics in review-failure telemetry

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,

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<string, string> = {};
   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,
       }),
     );
   }

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",

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" }));