From ecc02f1d7834a7c00c47d20e209611dd33005bc8 Mon Sep 17 00:00:00 2001 From: Mao Nakamoto <41178744+maonakamoto@users.noreply.github.com> Date: Tue, 1 Sep 2026 03:01:50 +0200 Subject: [PATCH] fix(ai): an empty completion was returned as a successful answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `completeWithOpenAICompat` ended with `content ?? ''`, so a response carrying `content: null` came back as the empty string — a SUCCESS. Three things followed from that, none of them visible: - `withProviderFallback` called `recordAIHealthSuccess()`, so /api/health reported the AI chain `ok` on a request that produced nothing - the chain never fell through, so the second provider was never asked — the fallback existed and did not engage on the one failure it could not see - the caller got '' and had to invent its own meaning for it The trigger is ordinary, not exotic: a reasoning model spends the whole `max_tokens` budget on reasoning tokens and stops with `finish_reason: "length"` before emitting any content. Observed on an OpenRouter auto-routed model at a small budget. The app's real budgets are far from that today — but which model sits behind a provider is not ours to pin forever, and this repo has already been bitten by model ids rotating underneath it. An empty completion is now an AIProviderError with status 502, chosen so `shouldTryNextProvider` falls through on `>= 500`: an unusable response from one vendor should be retried at the next, which is the whole point of having a chain. `finish_reason` rides on the error body so the log says WHY it was empty, following the existing rule that the body is for logs and never for a browser. This is the shape the codebase keeps meeting — absence read as an answer. The health signal saying `ok` while returning nothing is the same failure as a green gate that tests nothing. Mutation-proven: deleting the guard fails six tests. Verified with `SESSION_SECRET=… npm run build` (exit 0), the gate npm run verify does not cover. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Cd183M6472xBgTKWA2is6h --- src/lib/ai/__tests__/provider.test.ts | 86 +++++++++++++++++++++++++++ src/lib/ai/provider.ts | 36 ++++++++++- 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/src/lib/ai/__tests__/provider.test.ts b/src/lib/ai/__tests__/provider.test.ts index d0e0a7a8..98c54ae1 100644 --- a/src/lib/ai/__tests__/provider.test.ts +++ b/src/lib/ai/__tests__/provider.test.ts @@ -171,6 +171,92 @@ describe('the Groq call', () => { }) }) +describe('an empty completion is a failure, not an answer', () => { + /** + * `content ?? ''` returned the empty string as a SUCCESS: the fallback chain + * recorded `recordAIHealthSuccess()`, never tried the next provider, and the + * health endpoint said `ok` while the caller got nothing. + * + * The trigger is ordinary — a reasoning model spends the whole `max_tokens` + * budget on reasoning and stops with `finish_reason: "length"` before writing + * any content. The model behind a provider is not ours to pin forever. + */ + const emptyResponse = (content: string | null, finish_reason: string) => + ({ + ok: true, + json: async () => ({ choices: [{ message: { content }, finish_reason }] }), + }) as Response + + it.each([ + ['null content, truncated', null, 'length'], + ['empty string, truncated', '', 'length'], + ['whitespace only', ' \n ', 'stop'], + ['empty string, stopped', '', 'stop'], + ])('rejects %s rather than returning it', async (_name, content, finish) => { + const { completeText } = await loadProvider({ GROQ_API_KEY: 'gsk_test' }) + jest.spyOn(global, 'fetch').mockResolvedValue(emptyResponse(content, finish)) + + const failure = await completeText({ + system: 's', + prompt: 'p', + maxTokens: 20, + temperature: 0, + }).catch((error: unknown) => error) + + const { AIChainExhaustedError } = await import('@/lib/ai/errors') + expect(failure).toBeInstanceOf(AIChainExhaustedError) + }) + + it('falls through to the next provider instead of returning blank', async () => { + // The whole point of a chain: one provider returning something unusable + // must not end the attempt. 502 is chosen so `shouldTryNextProvider` passes. + const { completeText } = await loadProvider({ + GROQ_API_KEY: 'gsk_test', + OPENROUTER_API_KEY: 'sk-or_test', + }) + const fetchMock = jest + .spyOn(global, 'fetch') + .mockResolvedValueOnce(emptyResponse(null, 'length')) + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ choices: [{ message: { content: 'real answer' } }] }), + } as Response) + + const text = await completeText({ system: 's', prompt: 'p', maxTokens: 20, temperature: 0 }) + + expect(text).toBe('real answer') + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + + it('names finish_reason on the error, so the log says WHY it was empty', async () => { + const { completeText } = await loadProvider({ GROQ_API_KEY: 'gsk_test' }) + jest.spyOn(global, 'fetch').mockResolvedValue(emptyResponse(null, 'length')) + + const failure = await completeText({ + system: 's', + prompt: 'p', + maxTokens: 20, + temperature: 0, + }).catch((error: unknown) => error) + + const { AIChainExhaustedError } = await import('@/lib/ai/errors') + const last = (failure as InstanceType).last + expect(last?.body).toContain('length') + }) + + it('still returns a real answer untouched', async () => { + const { completeText } = await loadProvider({ GROQ_API_KEY: 'gsk_test' }) + jest.spyOn(global, 'fetch').mockResolvedValue({ + ok: true, + json: async () => ({ choices: [{ message: { content: '{"values":{}}' } }] }), + } as Response) + + await expect( + completeText({ system: 's', prompt: 'p', maxTokens: 20, temperature: 0 }), + ).resolves.toBe('{"values":{}}') + }) +}) + describe('the OpenRouter call', () => { it('uses OpenRouter when Groq is absent', async () => { const { completeText } = await loadProvider({ diff --git a/src/lib/ai/provider.ts b/src/lib/ai/provider.ts index a103563c..caf0f99a 100644 --- a/src/lib/ai/provider.ts +++ b/src/lib/ai/provider.ts @@ -214,8 +214,40 @@ async function completeWithOpenAICompat( throw new AIProviderError(config.provider, res.status, detail) } - const body = (await res.json()) as { choices?: Array<{ message?: { content?: string } }> } - return body.choices?.[0]?.message?.content ?? '' + const body = (await res.json()) as { + choices?: Array<{ message?: { content?: string | null }; finish_reason?: string }> + } + const choice = body.choices?.[0] + const content = choice?.message?.content ?? '' + + // An empty completion is a FAILURE, not an answer. + // + // This used to be `content ?? ''`, so a response carrying `content: null` + // returned the empty string as a success — `withProviderFallback` then called + // `recordAIHealthSuccess()`, the chain never fell through to the next + // provider, and the health endpoint reported `ok`. A blank answer, recorded + // as a healthy one. + // + // The way it happens is not exotic: a reasoning model spends the whole + // `max_tokens` budget on reasoning tokens and stops with + // `finish_reason: "length"` before emitting any content. Observed on an + // OpenRouter auto-routed model at a small budget. The app's real budgets + // (2000 for form fill) are far from that today — but the model behind a + // provider is not ours to pin forever, and this repo has already been bitten + // by model ids rotating underneath it. + // + // 502 rather than a bespoke status because `shouldTryNextProvider` falls + // through on `>= 500`: an unusable response from one provider should be + // retried on the next, which is the entire point of having a chain. + if (content.trim() === '') { + throw new AIProviderError( + config.provider, + 502, + `empty completion (finish_reason: ${choice?.finish_reason ?? 'unknown'})`, + ) + } + + return content } /**