From 796c7c8d958d96c1e6e765854bbdb16dd21d3335 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 01:48:17 -0700 Subject: [PATCH] fix(review): stop logging raw unparseable AI output --- src/services/ai-review.ts | 17 ++--------------- test/unit/ai-review.test.ts | 27 +++++++++++++++++---------- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 0cd4faa72c..5465b1dfa8 100644 --- a/src/services/ai-review.ts +++ b/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, }), ); } diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index bb7c02fa65..1544b28599 100644 --- a/test/unit/ai-review.test.ts +++ b/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(); });