From fd6eaa0a1aceb9132956812fdbb90bcda59db3b7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:15:13 -0700 Subject: [PATCH] fix(selfhost): force JSON mode on OpenAI-compatible review calls and stop retrying deterministic-identical outputs (#8790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirmed live 2026-07-26 (the back-to-back inconclusive-review incident): the ollama fallback answered the review prompt with the same 2,814-char markdown prose on all 3 attempts — hasJsonObject:false every time — so any primary-model bail became a guaranteed "no usable verdict" manual hold, and the retry budget was pure waste (reviews run at temperature 0; identical input yields identical output). - AiRunOptions gains responseFormat: "json_object"; createOpenAiCompatibleAi forwards it as OpenAI's response_format (Ollama/vLLM honor it), with a single 400-fallback retry stripping the parameter for older servers that reject it (degrade to ask-nicely, never fail the call). Non-400 failures and 400s without the declared contract throw exactly as before. - runWorkersOpinion declares the JSON contract on every review call (other providers ignore the field; the subscription CLIs already comply via the prompt) and stops a model's retries when an attempt returns byte-identical output to the previous one — the same stop-retrying-this-model reasoning as the deliberate-bail/timeout/429 breaks. The next model keeps its full budget. New diagnostic status: identical_retry_skipped. --- src/selfhost/ai.ts | 31 ++++++++++++++++---- src/services/ai-review.ts | 28 +++++++++++++++++- test/unit/ai-review-advisory.test.ts | 43 ++++++++++++++++++++++++++++ test/unit/ai-review.test.ts | 10 +++++-- test/unit/selfhost-ai.test.ts | 42 +++++++++++++++++++++++++++ 5 files changed, 145 insertions(+), 9 deletions(-) diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index 427c2a82e6..fda1a99f6f 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -33,6 +33,12 @@ interface AiRunOptions { // (embeddings, the subscription CLIs, Anthropic) ignores it, so it is safe to set unconditionally on a // call that ONLY ever targets an Ollama-backed binding (e.g. AI_VISION). providerOptions?: Record; + // #8790: the caller expects a JSON object back (the PR-review prompt's contract). Only + // createOpenAiCompatibleAi's chat path reads this — forwarded as OpenAI's `response_format` so a local + // model (confirmed live: an Ollama fallback answering the review prompt in markdown prose on every + // attempt) is FORCED into JSON mode instead of merely asked. Every other provider ignores it (the + // subscription CLIs already comply via the prompt), so it is safe to set unconditionally on review calls. + responseFormat?: "json_object"; // Correlation context for a provider-failure log (#codex-timeout-fields): purely observational, never read by a // provider's own request logic. The caller (runWorkersOpinion) passes whatever of these it already has in scope // for THIS review — job id and attempt are per-attempt, repoFullName/pullNumber identify the PR being reviewed — @@ -366,18 +372,33 @@ export function createOpenAiCompatibleAi(opts: { } const repoOverride = opts.providerName ? resolveOpenAiCompatibleRepoOverride(opts.providerName, options) : undefined; const resolvedModel = resolveModel(firstConfigured(repoOverride, opts.model), model, opts.defaultModel ?? DEFAULT_OPENAI_COMPATIBLE_CHAT_MODEL); - const res = await fetch(`${base}/chat/completions`, { - method: "POST", - headers: headers(), - body: JSON.stringify({ + const chatBody = (withResponseFormat: boolean): string => + JSON.stringify({ model: resolvedModel, messages: toMessages(options).map((message) => ({ role: message.role, content: toOpenAiMessageContent(message.content) })), max_tokens: options.max_tokens, temperature: options.temperature, ...(options.providerOptions ? { options: options.providerOptions } : {}), - }), + // #8790: force JSON mode when the caller declared a JSON contract — Ollama's and vLLM's + // OpenAI-compatible layers both honor it; servers that don't get the 400-fallback below. + ...(withResponseFormat && options.responseFormat === "json_object" ? { response_format: { type: "json_object" } } : {}), + }); + let res = await fetch(`${base}/chat/completions`, { + method: "POST", + headers: headers(), + body: chatBody(true), signal: AbortSignal.timeout(120_000), }); + // #8790: an older OpenAI-compatible server may reject the response_format parameter outright with a + // 400. Retry once without it — degrading to the pre-#8790 ask-nicely behavior beats failing the call. + if (!res.ok && res.status === 400 && options.responseFormat === "json_object") { + res = await fetch(`${base}/chat/completions`, { + method: "POST", + headers: headers(), + body: chatBody(false), + signal: AbortSignal.timeout(120_000), + }); + } if (!res.ok) throw new Error(`ai_http_${res.status}`); const data = (await res.json()) as { choices?: Array<{ message?: { content?: string } }> }; const usage = extractCliUsage(JSON.stringify(data)); diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 25bed55d97..7ceebf9070 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -476,7 +476,7 @@ export type ModelReview = { export type AiReviewDiagnostic = { model: string; attempt: number; - status: "parsed" | "empty_output" | "unparseable_output" | "provider_error" | "missing_assessment"; + status: "parsed" | "empty_output" | "unparseable_output" | "provider_error" | "missing_assessment" | "identical_retry_skipped"; responseChars?: number | undefined; hasJsonObject?: boolean | undefined; error?: string | undefined; @@ -1189,6 +1189,12 @@ async function runWorkersOpinion( if (modelIndex > 0) { incr("loopover_ai_review_model_fallback_total", { primary, fallback: model }); } + // #8790: the previous attempt's raw output for THIS model. Reviews run at temperature 0, so a + // byte-identical repeat is deterministic — the remaining retries are provably useless (confirmed live + // 2026-07-26: a fallback returned the same 2,814-char markdown response on all 3 attempts). Same + // stop-retrying-this-model reasoning as the deliberate-bail/timeout/429 breaks below; the next model + // still gets its own full budget. + let lastRawText: string | undefined; for (let attempt = 0; attempt < 3; attempt += 1) { try { const cliSystemAppend = selfHostCliSystemAppend(model, systemAppend); @@ -1197,6 +1203,10 @@ async function runWorkersOpinion( { max_tokens: maxTokens, temperature: 0, + // #8790: the review prompt's contract is a JSON object — declare it so an OpenAI-compatible + // provider is forced into JSON mode (response_format) instead of merely asked. Ignored by every + // other provider (the subscription CLIs already comply via the prompt). + responseFormat: "json_object", messages: [ { role: "system", content: system }, { role: "user", content: toContentBlocks(user, images) }, @@ -1232,6 +1242,22 @@ async function runWorkersOpinion( const text = coerceAiText(result); const usage = coerceAiUsage(result); const usageFields = usage ? { usage } : {}; + // #8790: byte-identical to the previous attempt's (necessarily failed — a success returns) output → + // deterministic repeat; stop this model's retries instead of burning the rest of the budget on it. + if (attempt > 0 && text.trim() !== "" && text === lastRawText) { + diagnostics.push({ model, attempt, status: "identical_retry_skipped", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); + console.warn( + JSON.stringify({ + level: "warn", + event: "ai_review_provider_identical_retry_skipped", + model, + attempt, + responseChars: text.length, + }), + ); + break; + } + lastRawText = text; const parsed = parseModelReview(text); if (parsed && parsed.assessment.trim() !== "") { diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index de611d8b46..524ad3ba4a 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -760,6 +760,49 @@ describe("runAiReviewForAdvisory", () => { captureSpy.mockRestore(); }); + 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; + // The same unparseable markdown every time — the confirmed 2026-07-26 production shape. Attempt 0 fails, + // attempt 1 comes back byte-identical → the remaining retry budget is provably useless and is skipped. + const env = aiEnv(async () => { + aiCalls += 1; + return { response: "### Review of Code Changes\n\nProse, not JSON." }; + }); + const result = await runAiReviewForAdvisory(env, { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + expect(result?.findings).toEqual([expect.objectContaining({ code: "ai_review_inconclusive" })]); + // This harness's plan resolves 4 model slots (two reviewers x primary+fallback); the invariant under + // test is per-slot: exactly 2 attempts each (attempt 0 + the identical attempt 1), never a third. + expect(aiCalls).toBe(8); + }); + + it("#8790: a model whose failed outputs DIFFER between attempts keeps its full retry budget (only determinism short-circuits)", async () => { + const adv = advisory(); + let aiCalls = 0; + const env = aiEnv(async () => { + aiCalls += 1; + return { response: `### Prose attempt ${aiCalls}` }; + }); + await runAiReviewForAdvisory(env, { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + expect(aiCalls).toBe(12); // 3 attempts x the same 4 model slots — pre-#8790 behavior pinned for varying output + }); + it("uses the non-cacheable block-mode inconclusive note when no reviewer returns public text", async () => { const adv = advisory(); const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: "" })), { diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 31cfbbb6dc..79b8c3acc9 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -1449,8 +1449,9 @@ describe("Workers AI fallback + degraded output", () => { const result = await runLoopOverAiReview(env, baseInput); expect(result.status === "ok" && result.advisoryNotes).toBeNull(); expect(result.status === "ok" && result.inconclusive).toBe(true); - // primary 3× + fallback 3× retries, all unparseable. - expect(run).toHaveBeenCalledTimes(6); + // #8790: identical unparseable output on every call → each model stops after its byte-identical + // attempt 1 (2 calls per model) instead of burning the full 3-attempt budget on a deterministic repeat. + expect(run).toHaveBeenCalledTimes(4); }); }); @@ -3259,7 +3260,10 @@ describe("pure helpers", () => { // attempt ever produced the required assessment field. expect(parsed.review?.assessment).toBe(""); expect(parsed.review?.nits).toEqual(["Edge case on empty input is untested.", "Naming could be clearer."]); - expect(run).toHaveBeenCalledTimes(6); // 3 attempts x 2 models -- the full budget, since nothing here is a deliberate bail. + // #8790: the mock returns byte-identical output every call, so each model stops after its identical + // attempt 1 (2 calls per model). The incomplete-review fallback + exhausted log below are unaffected — + // attempt 0 already captured bestIncompleteReview. + expect(run).toHaveBeenCalledTimes(4); const exhausted = logSpy.mock.calls .map((c) => c[0]) .find((l) => typeof l === "string" && l.includes("ai_review_missing_assessment_exhausted")); diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index 38026ef5ea..9c243d121b 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -178,6 +178,48 @@ describe("createOpenAiCompatibleAi (#979)", () => { expect(first?.body.model).toBe("llama3.1"); }); + it("#8790: forwards response_format json_object when the caller declares a JSON contract, and omits it otherwise", async () => { + const bodies: Array> = []; + vi.stubGlobal("fetch", vi.fn(async (_url: string, init: { body: string }) => { + bodies.push(JSON.parse(init.body)); + return new Response(JSON.stringify({ choices: [{ message: { content: "{}" } }] }), { status: 200 }); + })); + const ai = createOpenAiCompatibleAi({ baseUrl: "http://o/v1" }); + await ai.run("m", { prompt: "x", responseFormat: "json_object" }); + expect(bodies[0]).toMatchObject({ response_format: { type: "json_object" } }); + await ai.run("m", { prompt: "x" }); + expect("response_format" in bodies[1]!).toBe(false); + }); + + it("#8790: retries ONCE without response_format when the server 400s on it — degrade to ask-nicely, never fail the call", async () => { + const bodies: Array> = []; + vi.stubGlobal("fetch", vi.fn(async (_url: string, init: { body: string }) => { + const body = JSON.parse(init.body) as Record; + bodies.push(body); + if ("response_format" in body) return new Response("unknown parameter", { status: 400 }); + return new Response(JSON.stringify({ choices: [{ message: { content: "ok" } }] }), { status: 200 }); + })); + const ai = createOpenAiCompatibleAi({ baseUrl: "http://o/v1" }); + const out = await ai.run("m", { prompt: "x", responseFormat: "json_object" }); + expect(out.response).toBe("ok"); + expect(bodies).toHaveLength(2); + expect("response_format" in bodies[1]!).toBe(false); + }); + + it("#8790: a 400 WITHOUT a declared JSON contract still throws ai_http_400 — the retry is format-rejection-specific", async () => { + vi.stubGlobal("fetch", vi.fn(async () => new Response("bad request", { status: 400 }))); + const ai = createOpenAiCompatibleAi({ baseUrl: "http://o/v1" }); + await expect(ai.run("m", { prompt: "x" })).rejects.toThrow("ai_http_400"); + }); + + it("#8790: a NON-400 failure with the JSON contract set throws without a format-stripping retry (only 400 means parameter rejection)", async () => { + const fetchMock = vi.fn(async () => new Response("upstream broke", { status: 500 })); + vi.stubGlobal("fetch", fetchMock); + const ai = createOpenAiCompatibleAi({ baseUrl: "http://o/v1" }); + await expect(ai.run("m", { prompt: "x", responseFormat: "json_object" })).rejects.toThrow("ai_http_500"); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + it("attributes usage.provider from its own configured providerName (#ai-usage-provider-attribution) since an HTTP chat-completions response never reports one itself", async () => { vi.stubGlobal("fetch", vi.fn(async () => new Response(JSON.stringify({ choices: [{ message: { content: "ok" } }] }), { status: 200 }),