From 79cd9ab493897eeca96f21531509667791466a23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 24 Oct 2026 20:24:50 +0200 Subject: [PATCH] fix(ai): stop asking a model with none once none did not help The #1267 rule asks a reply that reasoned without answering once more with reasoning_effort "none", and asks every later call that way for an hour. That is what Gemma 4 on Ollama needs. gpt-oss behind vLLM accepts only low / medium / high and ignores "none", and a GLM chat template reads an effort it does not know as its maximum, so for those models the repeat and every later "none" were wasted requests or longer thoughts. When a reply sent as "none" comes back without an answer as well, the Local and gateway clients now learn that too: later calls go out as the call asks, with the thinking room #1276 keeps, and an answerless reply is final. One call costs two requests at most and one after that, and the log gets one line for each lesson instead of one per call. A failed answerless call now carries what it cost on its error, and a repeat that answers reports both requests, so the day's token ledger can charge them. The learned lessons are pinned on globalThis, so the Coach's route handlers and the worker's jobs share one copy per process instead of learning the same thing twice. Refs #1279 --- .../gateway-unswitchable-reasoning.test.ts | 212 ++++++++++++++++++ ...ools-loop-always-reasoning-gateway.test.ts | 8 +- src/lib/ai/local-client.ts | 63 ++++-- src/lib/ai/openai-client.ts | 51 +++-- src/lib/ai/reasoning/answerless.ts | 110 +++++++++ src/lib/ai/reasoning/dialect-cache.ts | 25 ++- 6 files changed, 430 insertions(+), 39 deletions(-) create mode 100644 src/lib/ai/__tests__/gateway-unswitchable-reasoning.test.ts create mode 100644 src/lib/ai/reasoning/answerless.ts diff --git a/src/lib/ai/__tests__/gateway-unswitchable-reasoning.test.ts b/src/lib/ai/__tests__/gateway-unswitchable-reasoning.test.ts new file mode 100644 index 000000000..8e415bd80 --- /dev/null +++ b/src/lib/ai/__tests__/gateway-unswitchable-reasoning.test.ts @@ -0,0 +1,212 @@ +/** + * #1279 — a model behind an OpenAI-compatible gateway or a local server that + * reasons however it is asked: gpt-oss behind vLLM accepts only low / medium / + * high and ignores `reasoning_effort: "none"`, and a GLM chat template reads an + * effort it does not know as its maximum. Every reply is the thought alone, + * cut off at the token ceiling (`finish_reason: "length"`). + * + * The #1267 repeat with `none` is right for Gemma on Ollama and wasted on + * these. Driven through the real clients, the real `safeFetch` and the pinned + * dispatcher against a loopback server, these cases pin the rule: one repeat + * per call, never `none` again once a `none` reply was answerless too, the + * thinking room on every later call, and what each failed call cost carried + * on its error so the day's ledger can charge it. + */ +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +const emitSignal = vi.hoisted(() => vi.fn()); +vi.mock("@/lib/logging/signal", () => ({ + emitSignal: (...a: unknown[]) => emitSignal(...a), +})); + +import { LocalOpenAICompatibleClient } from "../local-client"; +import { OpenAIClient } from "../openai-client"; +import { resetReasoningDialectCache } from "../reasoning/dialect-cache"; +import { singleUserTurn, type CompletionParams } from "../types"; +import { _resetPinnedDispatcherForTests } from "@/lib/safe-fetch-dispatcher"; + +interface SeenRequest { + reasoningEffort: unknown; + maxTokens: unknown; +} + +/** + * `always`: the thought fills the ceiling whatever the effort (gpt-oss, GLM). + * `answers-unreasoned`: the same, except asked with `none` it answers + * (Gemma 4 on Ollama). + */ +let mode: "always" | "answers-unreasoned" = "always"; +let server: http.Server; +let origin: string; +let seen: SeenRequest[] = []; + +const ANSWER = JSON.stringify({ summary: "Steady." }); + +function reply(effort: unknown) { + const answers = mode === "answers-unreasoned" && effort === "none"; + return { + id: "chatcmpl-1279", + object: "chat.completion", + model: "gpt-oss-120b", + choices: [ + { + index: 0, + message: answers + ? { role: "assistant", content: ANSWER } + : { + role: "assistant", + content: null, + reasoning_content: "We need to look at the readings. ".repeat(40), + }, + finish_reason: answers ? "stop" : "length", + }, + ], + usage: { prompt_tokens: 2400, completion_tokens: 300, total_tokens: 2700 }, + }; +} + +beforeAll(async () => { + server = http.createServer((req, res) => { + let raw = ""; + req.on("data", (c) => (raw += c)); + req.on("end", () => { + const body = JSON.parse(raw) as Record; + seen.push({ + reasoningEffort: body.reasoning_effort, + maxTokens: body.max_tokens ?? body.max_completion_tokens, + }); + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify(reply(body.reasoning_effort))); + }); + }); + await new Promise((r) => server.listen(0, "127.0.0.1", r)); + origin = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterAll(async () => { + _resetPinnedDispatcherForTests(); + await new Promise((r) => server.close(() => r())); +}); + +const savedOrigins = process.env.AI_PRIVATE_ORIGINS; + +beforeEach(() => { + process.env.AI_PRIVATE_ORIGINS = origin; + mode = "always"; + seen = []; + emitSignal.mockClear(); + resetReasoningDialectCache(); +}); + +afterEach(() => { + if (savedOrigins === undefined) delete process.env.AI_PRIVATE_ORIGINS; + else process.env.AI_PRIVATE_ORIGINS = savedOrigins; +}); + +function gateway() { + return new OpenAIClient({ + apiKey: "", + model: "gpt-oss-120b", + baseUrl: `${origin}/v1`, + providerType: "openai-compatible", + }); +} + +function local() { + return new LocalOpenAICompatibleClient({ + baseUrl: `${origin}/v1`, + model: "gpt-oss-120b", + }); +} + +/** A status card call as `runStatusCompletion` shapes it: no level. */ +function statusParams(): CompletionParams { + return singleUserTurn({ + system: "You write the status note.", + user: "Snapshot: …", + maxTokens: 300, + timeoutMs: 30_000, + responseFormat: "json", + }); +} + +function actions(): string[] { + return emitSignal.mock.calls.map((c) => (c[0] as { action: string }).action); +} + +async function failures( + client: { generateCompletion(p: CompletionParams): Promise }, + calls: number, +) { + const errors: Array> = []; + for (let i = 0; i < calls; i += 1) { + errors.push( + (await client + .generateCompletion(statusParams()) + .catch((e) => e)) as Record, + ); + } + return errors; +} + +describe.each([ + ["the OpenAI-compatible gateway", gateway, "ai.gateway"], + ["the Local client", local, "ai.local"], +] as const)( + "%s, a model that reasons whatever it is asked", + (_, make, prefix) => { + it("repeats one call once, then never sends `none` again", async () => { + const errors = await failures(make(), 5); + + expect(seen.map((s) => s.reasoningEffort)).toEqual([ + undefined, + "none", + undefined, + undefined, + undefined, + undefined, + ]); + for (const err of errors) { + expect(err).toBeInstanceOf(Error); + expect(err.kind).toBe("reasoning_only"); + expect(err.httpStatus).toBe(0); + } + // One line when the repeat is learned, one when it is learned useless, + // nothing per call after that. + expect(actions()).toEqual([ + `${prefix}.reasoning_answerless`, + `${prefix}.reasoning_unswitchable`, + ]); + }); + + it("leaves the thinking room on every call after the first", async () => { + await failures(make(), 3); + expect(seen.map((s) => s.maxTokens)).toEqual([300, 1324, 1324, 1324]); + }); + + it("carries what a failed call cost on its error, both requests included", async () => { + const [first, second] = await failures(make(), 2); + expect(first.tokensUsed).toBe(5400); + expect(second.tokensUsed).toBe(2700); + }); + + it("still repeats with `none` for a model that answers unreasoned, and bills both requests", async () => { + mode = "answers-unreasoned"; + const result = await make().generateCompletion(statusParams()); + expect(result.content).toBe(ANSWER); + expect(result.tokensUsed).toBe(5400); + expect(seen.map((s) => s.reasoningEffort)).toEqual([undefined, "none"]); + }); + }, +); diff --git a/src/lib/ai/coach/__tests__/tools-loop-always-reasoning-gateway.test.ts b/src/lib/ai/coach/__tests__/tools-loop-always-reasoning-gateway.test.ts index 9b7a56d34..e66828695 100644 --- a/src/lib/ai/coach/__tests__/tools-loop-always-reasoning-gateway.test.ts +++ b/src/lib/ai/coach/__tests__/tools-loop-always-reasoning-gateway.test.ts @@ -251,7 +251,7 @@ describe("#1276: a gateway model that always reasons", () => { }); it.each(["fielded", "template-opened", "raw"])( - "keeps room to reason after one answerless reply taught it `none` (%s)", + "keeps room to reason after the `none` repeat came back answerless too (%s)", async (mode) => { parser = mode; // A long round reasons past the whole ceiling once; the #1267 rule @@ -281,8 +281,10 @@ describe("#1276: a gateway model that always reasons", () => { expect(out.result.content).toBe(ANSWER); expect(executeCoachTool).toHaveBeenCalledTimes(1); for (const body of bodies()) { - expect(body.reasoning_effort).toBe("none"); - // `none` is a request, not a promise: the thinking room stays. + // #1279 — the `none` repeat came back answerless too, so the model + // is not asked with `none` again: the turn goes out at its own level. + expect(body.reasoning_effort).toBe("medium"); + // The thinking room stays. expect(body.max_tokens).toBe(600 + 4096); } }, diff --git a/src/lib/ai/local-client.ts b/src/lib/ai/local-client.ts index a8e253993..c10b7c623 100644 --- a/src/lib/ai/local-client.ts +++ b/src/lib/ai/local-client.ts @@ -34,6 +34,11 @@ import { rememberReasoningDialect, } from "./reasoning/dialect-cache"; import { uncontrolledThinkingRoom } from "./reasoning/levels"; +import { + nextAnswerlessStep, + reasonsUnasked, + withEarlierSpend, +} from "./reasoning/answerless"; import { ReasoningCollector, separateMessage, @@ -108,7 +113,7 @@ function withStrictJsonPrefix(messages: AiMessage[]): AiMessage[] { * treats as a hard failure; `kind` says whether the reply held reasoning and * no answer, or nothing at all, so the log names it instead of "network". */ -function emptyReplyError(hadReasoning: boolean): Error { +function emptyReplyError(hadReasoning: boolean, spent: number | null): Error { const err = new Error( hadReasoning ? "Local AI returned reasoning but no answer" @@ -118,6 +123,9 @@ function emptyReplyError(hadReasoning: boolean): Error { httpStatus: 0, kind: hadReasoning ? "reasoning_only" : "empty_response", upstream: "local", + // #1279 — what the reply cost, so the day's ledger charges it instead of + // refunding a call that was paid for. + ...(spent !== null ? { tokensUsed: spent } : {}), }); return err; } @@ -204,8 +212,7 @@ export class LocalOpenAICompatibleClient implements AIProvider { // room once the model has shown it reasons unasked. const thinkingTokens = params.reasoning ? uncontrolledThinkingRoom(params.reasoning.effort) - : learnedReasoningDialect(this.reasoningDialectKey()) === - ANSWERS_ONLY_UNREASONED + : reasonsUnasked(this.reasoningDialectKey()) ? uncontrolledThinkingRoom("off") : 0; const body = JSON.stringify({ @@ -299,6 +306,10 @@ export class LocalOpenAICompatibleClient implements AIProvider { * `none` comes back empty with reasoning (or after asking to reason), learn * that this endpoint and model answer only unreasoned, and tell the caller * to ask once more. Returns false when the empty reply is final. + * + * #1279 — once per call, and a model whose `none` reply was answerless too + * is not asked with `none` again while the lesson holds + * (`reasoning/answerless.ts`). */ private learnAnswerlessReasoning( params: CompletionParams, @@ -306,22 +317,32 @@ export class LocalOpenAICompatibleClient implements AIProvider { finishReason: string | undefined, ): boolean { const sent = this.sentReasoningEffort(params); - if (sent === "none") return false; - if (!hadReasoning && sent === null) return false; - rememberReasoningDialect( + const finish = + finishReason === "length" || finishReason === "stop" + ? finishReason + : "other"; + const step = nextAnswerlessStep( this.reasoningDialectKey(), - ANSWERS_ONLY_UNREASONED, + sent, + hadReasoning, ); + if (step.kind === "final") { + if (step.learnedRegardless) { + emitSignal({ + action: "ai.local.reasoning_unswitchable", + level: "warn", + meta: { finish }, + }); + } + return false; + } annotateReasoningDowngrade("local", sent ?? "default", "none"); emitSignal({ action: "ai.local.reasoning_answerless", level: "warn", meta: { effort: sent ?? "default", - finish: - finishReason === "length" || finishReason === "stop" - ? finishReason - : "other", + finish, reasoning: hadReasoning, }, }); @@ -470,8 +491,9 @@ export class LocalOpenAICompatibleClient implements AIProvider { room, ); if (!content) { + const spent = json.usage?.total_tokens ?? null; if (this.learnAnswerlessReasoning(params, hadReasoning, finish)) { - return this.generateCompletion(params); + return withEarlierSpend(this.generateCompletion(params), spent); } // The reasoning that did arrive still reaches `onReasoning`, as before. collector.result( @@ -480,7 +502,7 @@ export class LocalOpenAICompatibleClient implements AIProvider { // v1.20.1 — sentinel httpStatus + kind so the chain classifier can tell // an empty 200-OK reply apart from a transport failure. Cascade unchanged // (`status <= 0` is already a hard failure). - throw emptyReplyError(hadReasoning); + throw emptyReplyError(hadReasoning, spent); } const reasoning = collector.result( json.usage?.completion_tokens_details?.reasoning_tokens ?? null, @@ -597,6 +619,7 @@ export class LocalOpenAICompatibleClient implements AIProvider { room, ); if (!content) { + const spent = json?.usage?.total_tokens ?? null; if ( this.learnAnswerlessReasoning( params, @@ -604,10 +627,13 @@ export class LocalOpenAICompatibleClient implements AIProvider { json?.choices?.[0]?.finish_reason, ) ) { - return this.generateCompletionStream(params, onDelta); + return withEarlierSpend( + this.generateCompletionStream(params, onDelta), + spent, + ); } collector.result(null); - throw emptyReplyError(hadReasoning); + throw emptyReplyError(hadReasoning, spent); } const reasoning = collector.result(null); onDelta(content); @@ -804,10 +830,13 @@ export class LocalOpenAICompatibleClient implements AIProvider { const hadReasoning = streamedReasoning || endState === "inside" || tail.took; if (this.learnAnswerlessReasoning(params, hadReasoning, finishReason)) { - return this.generateCompletionStream(params, onDelta); + return withEarlierSpend( + this.generateCompletionStream(params, onDelta), + tokensUsed, + ); } collector.result(reasoningTokens); - throw emptyReplyError(hadReasoning); + throw emptyReplyError(hadReasoning, tokensUsed); } const reasoning = collector.result(reasoningTokens); diff --git a/src/lib/ai/openai-client.ts b/src/lib/ai/openai-client.ts index b5fe23a83..5b515406b 100644 --- a/src/lib/ai/openai-client.ts +++ b/src/lib/ai/openai-client.ts @@ -20,6 +20,11 @@ import { learnedReasoningDialect, rememberReasoningDialect, } from "./reasoning/dialect-cache"; +import { + nextAnswerlessStep, + reasonsUnasked, + withEarlierSpend, +} from "./reasoning/answerless"; import { REASONING_THINKING_BUDGET, uncontrolledThinkingRoom, @@ -437,34 +442,42 @@ export class OpenAIClient implements AIProvider { // #1267 — a gateway in front of a thinking model (Ollama serving // Gemma 4, say) can answer with the thought in `reasoning` and no // content. Asked with `none`, the same model answers; learn that once - // and ask again. Same rule as the Local client. + // and ask again. #1279 — once, and never `none` again for a model the + // repeat showed ignores it. Same rule as the Local client. const sentEffort = reasoningWire.kind !== "none" ? reasoningWire.effort : entryEffort; const hadReasoning = reply.hadReasoning; - if ( - this.isGateway && - sentEffort !== "none" && - (hadReasoning || sentEffort !== null) - ) { - rememberReasoningDialect( - this.reasoningDialectKey(), - ANSWERS_ONLY_UNREASONED, - ); + const spent = json.usage?.total_tokens ?? null; + const finish = + choice?.finish_reason === "length" || choice?.finish_reason === "stop" + ? choice.finish_reason + : "other"; + const step = this.isGateway + ? nextAnswerlessStep( + this.reasoningDialectKey(), + sentEffort, + hadReasoning, + ) + : null; + if (step?.kind === "ask_unreasoned") { annotateReasoningDowngrade(this.type, sentEffort ?? "default", "none"); emitSignal({ action: "ai.gateway.reasoning_answerless", level: "warn", meta: { effort: sentEffort ?? "default", - finish: - choice?.finish_reason === "length" || - choice?.finish_reason === "stop" - ? choice.finish_reason - : "other", + finish, reasoning: hadReasoning, }, }); - return this.generateCompletion(params); + return withEarlierSpend(this.generateCompletion(params), spent); + } + if (step?.kind === "final" && step.learnedRegardless) { + emitSignal({ + action: "ai.gateway.reasoning_unswitchable", + level: "warn", + meta: { finish }, + }); } // v1.20.1 — tag the empty-reply throw with a sentinel `httpStatus: 0` + // `kind` so the provider-chain classifier can tell an empty 200-OK reply @@ -479,6 +492,9 @@ export class OpenAIClient implements AIProvider { httpStatus: 0, kind: hadReasoning ? "reasoning_only" : "empty_response", upstream: this.isGateway ? "openai-compatible" : "openai", + // #1279 — what the reply cost upstream, so the day's ledger charges + // it instead of refunding a call that was paid for. + ...(spent !== null ? { tokensUsed: spent } : {}), }); throw err; } @@ -561,8 +577,7 @@ export class OpenAIClient implements AIProvider { if (params.reasoning) { return uncontrolledThinkingRoom(params.reasoning.effort); } - return learnedReasoningDialect(this.reasoningDialectKey()) === - ANSWERS_ONLY_UNREASONED + return reasonsUnasked(this.reasoningDialectKey()) ? uncontrolledThinkingRoom("off") : 0; } diff --git a/src/lib/ai/reasoning/answerless.ts b/src/lib/ai/reasoning/answerless.ts new file mode 100644 index 000000000..7d1de828b --- /dev/null +++ b/src/lib/ai/reasoning/answerless.ts @@ -0,0 +1,110 @@ +/** + * #1279 — what the Local and the OpenAI-compatible gateway clients do with a + * reply that reached the model and came back without an answer. + * + * A reply with reasoning and no answer is asked once more with + * `reasoning_effort: "none"` (#1267): Ollama serves Gemma 4 that way, and the + * repeat answers. The lesson holds for the hour (`ANSWERS_ONLY_UNREASONED`). + * + * When the repeat, or any later call sent as `none`, comes back the same way, + * `none` did nothing for this model: gpt-oss accepts only low / medium / high, + * and a GLM chat template reads an effort it does not know as its maximum + * (vllm-project/vllm#54744). That is learned too (`REASONS_REGARDLESS`): later + * calls go out as the call asks, with the thinking room #1276 keeps, and an + * answerless reply is final. So one call costs two requests at most, and + * after the first such call, one. + * + * What bounds the calls themselves is not here: the chain keeps background + * work off a provider that keeps failing (`provider-runner.ts`), and each job + * backs off the record it failed on. + */ +import { + ANSWERS_ONLY_UNREASONED, + REASONS_REGARDLESS, + learnedReasoningDialect, + rememberReasoningDialect, + type ReasoningDialectKey, +} from "./dialect-cache"; + +/** + * What the client does next with an answerless reply. + * - `ask_unreasoned`: learned that the model answers only unreasoned; send + * the call once more (it now goes out as `none`). + * - `final`: throw the empty reply. `learnedRegardless` is true when this + * reply taught that `none` does not help. + */ +export type AnswerlessStep = + { kind: "ask_unreasoned" } | { kind: "final"; learnedRegardless: boolean }; + +/** + * The next step for a reply with no answer. `sent` is the effort the request + * carried (`null`: no field); `hadReasoning` whether the reply reasoned. + */ +export function nextAnswerlessStep( + key: ReasoningDialectKey, + sent: string | null, + hadReasoning: boolean, +): AnswerlessStep { + if (learnedReasoningDialect(key) === REASONS_REGARDLESS) { + return { kind: "final", learnedRegardless: false }; + } + if (sent === "none") { + if (!hadReasoning) return { kind: "final", learnedRegardless: false }; + rememberReasoningDialect(key, REASONS_REGARDLESS); + return { kind: "final", learnedRegardless: true }; + } + if (!hadReasoning && sent === null) { + return { kind: "final", learnedRegardless: false }; + } + rememberReasoningDialect(key, ANSWERS_ONLY_UNREASONED); + return { kind: "ask_unreasoned" }; +} + +/** + * True when the model has shown it reasons without being asked to, so a call + * without a level of its own gets the thinking room (#1276). + */ +export function reasonsUnasked(key: ReasoningDialectKey): boolean { + const learned = learnedReasoningDialect(key); + return learned === ANSWERS_ONLY_UNREASONED || learned === REASONS_REGARDLESS; +} + +/** + * The repeat's outcome with the first request's tokens added: they were spent + * upstream whether the repeat answers or fails too, and the day's ledger + * charges what a call cost, not its last request. + */ +export async function withEarlierSpend( + repeat: Promise, + earlier: number | null, +): Promise { + if (earlier === null || earlier <= 0) return repeat; + try { + const result = await repeat; + return { + ...result, + tokensUsed: (result.tokensUsed ?? 0) + earlier, + }; + } catch (err) { + addSpentTokens(err, earlier); + throw err; + } +} + +/** + * Tokens a failed call spent upstream, as its error carries them + * (`tokensUsed`), or `null` when it carries none. + */ +export function spentTokensOf(err: unknown): number | null { + const value = (err as { tokensUsed?: unknown } | null)?.tokensUsed; + return typeof value === "number" && Number.isFinite(value) && value > 0 + ? value + : null; +} + +/** Add `tokens` to what the error `err` says its call spent. */ +export function addSpentTokens(err: unknown, tokens: number | null): void { + if (tokens === null || tokens <= 0) return; + if (err === null || typeof err !== "object") return; + Object.assign(err, { tokensUsed: (spentTokensOf(err) ?? 0) + tokens }); +} diff --git a/src/lib/ai/reasoning/dialect-cache.ts b/src/lib/ai/reasoning/dialect-cache.ts index 207efada6..60151707a 100644 --- a/src/lib/ai/reasoning/dialect-cache.ts +++ b/src/lib/ai/reasoning/dialect-cache.ts @@ -47,10 +47,33 @@ export interface ReasoningDialectKey { */ export const ANSWERS_ONLY_UNREASONED = "off"; +/** + * #1279 — the learned value for an endpoint and model that answered with + * reasoning alone even when asked with `none`: gpt-oss behind vLLM accepts + * only low / medium / high and ignores it, and a GLM chat template reads an + * effort it does not know as its maximum. Asking such a model with `none` is + * a wasted call, or a longer thought. Both clients send what the call asks + * for (with the thinking room) and never repeat an answerless reply while + * this holds. + */ +export const REASONS_REGARDLESS = "unswitchable"; + /** How long a learned downgrade holds before the full request is tried again. */ export const DIALECT_TTL_MS = 60 * 60 * 1000; -const learned = new Map(); +/** + * Pinned on `globalThis`: a module imported from more than one bundler layer + * is instantiated once per layer (see `caches` in `server-cache.ts`), so the + * Coach's route handlers and the worker's jobs each kept a copy and paid for + * the same lesson separately. One map per process. + */ +const LEARNED_SLOT = Symbol.for("healthlog.ai.reasoningDialects"); +const learned: Map = (( + globalThis as Record +)[LEARNED_SLOT] ??= new Map()) as Map< + string, + { value: string; expiresAt: number } +>; function cacheKey(key: ReasoningDialectKey): string { return `${key.provider}\u0000${key.endpoint}\u0000${key.model}`;