fix(review): stop logging raw unparseable AI output

src/services/ai-review.ts

@@ -1007,11 +1007,6 @@ function isSubscriptionCliTimeout(error: unknown): boolean {
   return error instanceof Error && error.message === "subscription_cli_timeout";
 }
 
-/** Cap on the diagnostic prefix logged for an unparseable model response (#observability-unparseable) -- long
- *  enough to tell a markdown-fenced/truncated-mid-JSON/plain-prose response apart, short enough to never dump
- *  a large chunk of model output into Sentry/audit context. */
-const UNPARSEABLE_RESPONSE_SNIPPET_MAX_CHARS = 400;
-
 /** One reviewer opinion (whichever provider `env.AI` resolves to — self-host Codex/Claude Code/etc, or the
  *  legacy Workers-AI pair) with a per-slot reliable fallback and a 3× retry on the primary. */
 async function runWorkersOpinion(
@@ -1041,9 +1036,7 @@ 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;
-  let lastUnparseable:
-    | { model: string; attempt: number; responseChars: number; hasJsonObject: boolean; responseSnippet: string }
-    | undefined;
+  let lastUnparseable: { model: string; attempt: number; responseChars: number; hasJsonObject: boolean } | undefined;
   const models = fallback && fallback !== primary ? [primary, fallback] : [primary];
   for (const [modelIndex, model] of models.entries()) {
     if (modelIndex > 0) {
@@ -1094,11 +1087,7 @@ async function runWorkersOpinion(
         const status = trimmedText ? "unparseable_output" : "empty_output";
         diagnostics.push({ model, attempt, status, responseChars: text.length, hasJsonObject, ...usageFields });
         if (trimmedText) {
-          // NOT added to the diagnostics entry above: reviewDiagnostics flows into result/Sentry context that
-          // must never carry raw provider text (see the "withholds unsafe provider and reviewer fallback text"
-          // test) -- logged here instead, which reaches only the structured-log Sentry forwarder, never `result`.
-          const responseSnippet = trimmedText.slice(0, UNPARSEABLE_RESPONSE_SNIPPET_MAX_CHARS);
-          lastUnparseable = { model, attempt, responseChars: text.length, hasJsonObject, responseSnippet };
+          lastUnparseable = { model, attempt, responseChars: text.length, hasJsonObject };
           console.warn(
             JSON.stringify({
               level: "warn",
@@ -1107,7 +1096,6 @@ async function runWorkersOpinion(
               attempt,
               responseChars: text.length,
               hasJsonObject,
-              responseSnippet,
             }),
           );
         }
@@ -1160,7 +1148,6 @@ async function runWorkersOpinion(
         attempt: lastUnparseable.attempt,
         responseChars: lastUnparseable.responseChars,
         hasJsonObject: lastUnparseable.hasJsonObject,
-        responseSnippet: lastUnparseable.responseSnippet,
       }),
     );
   }

test/unit/ai-review.test.ts

@@ -3082,7 +3082,7 @@ describe("pure helpers", () => {
     warnSpy.mockRestore();
   });
 
-  it("logs unparseable exhaustion separately when the model runs but returns unparseable output, including a response snippet for diagnosis (#observability-unparseable)", async () => {
+  it("logs unparseable exhaustion separately without raw provider output (#observability-unparseable)", async () => {
     const logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
     const run = vi.fn(async () => ({ response: "not json at all" }));
     const env = createTestEnv({ AI: { run } as unknown as Ai });
@@ -3097,28 +3097,35 @@ describe("pure helpers", () => {
       .map((c) => c[0])
       .find((l) => typeof l === "string" && l.includes("ai_review_provider_unparseable_exhausted"));
     expect(exhausted).toBeDefined();
-    expect(JSON.parse(exhausted as string)).toMatchObject({
+    const payload = JSON.parse(exhausted as string);
+    expect(payload).toMatchObject({
       event: "ai_review_provider_unparseable_exhausted",
-      responseSnippet: "not json at all",
+      responseChars: "not json at all".length,
+      hasJsonObject: false,
     });
+    expect(payload).not.toHaveProperty("responseSnippet");
     logSpy.mockRestore();
   });
 
-  it("truncates the unparseable-output response snippet to 400 chars instead of logging the full response (#observability-unparseable), and never puts it on the returned diagnostics (#4111-style public/private boundary)", async () => {
+  it("withholds raw unparseable provider output from diagnostics and structured logs (#observability-unparseable)", async () => {
     const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
-    const longResponse = "not json, ".repeat(60); // 600 chars, well over the 400-char cap
-    const run = vi.fn(async () => ({ response: longResponse }));
+    const rawResponse = "not json with private repo context and alice@example.com";
+    const run = vi.fn(async () => ({ response: rawResponse }));
     const env = createTestEnv({ AI: { run } as unknown as Ai });
     const diagnostics: AiReviewDiagnostic[] = [];
     await runWorkersOpinion(env, "primary-model", "primary-model", "sys", "user", 256, diagnostics);
-    // reviewDiagnostics flows into result/Sentry context that must never carry raw provider text (see the
-    // "withholds unsafe provider and reviewer fallback text" test) -- the snippet only ever reaches the log.
     expect(diagnostics[0]).not.toHaveProperty("responseSnippet");
     const firstWarn = warnSpy.mock.calls
       .map((c) => c[0])
       .find((l) => typeof l === "string" && l.includes("ai_review_provider_unparseable_output"));
-    expect(JSON.parse(firstWarn as string).responseSnippet).toBe(longResponse.slice(0, 400));
-    expect(JSON.parse(firstWarn as string).responseSnippet.length).toBe(400);
+    const payload = JSON.parse(firstWarn as string);
+    expect(payload).toMatchObject({
+      event: "ai_review_provider_unparseable_output",
+      responseChars: rawResponse.length,
+      hasJsonObject: false,
+    });
+    expect(payload).not.toHaveProperty("responseSnippet");
+    expect(firstWarn).not.toContain(rawResponse);
     warnSpy.mockRestore();
   });