From 69647b3b9404955e0b0ef348d406872961b1425d Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Sat, 8 Aug 2026 11:03:11 -0300 Subject: [PATCH 01/21] fix(combo): ignore benign empty error fields in streaming quality validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isStreamingUpstreamError used a key-presence check (parsed.error != null) which false-positives on benign values some backends emit on every chunk ({}, '', false, 0). When opencode issues a tool-call turn, the upstream SSE opens with role-only frames (no recognized content) and a later chunk that carries real tool_calls content PLUS a benign empty error field. The error gate runs BEFORE content recognizers, so that single frame short-circuits to 'error' -> 502 'streaming upstream error'. Same combo via kilocode works because its wire format never emits the empty error field. Fix: isSubstantiveError() helper — only treat error as real when it carries non-empty string, non-empty object, or explicit true. Empty object {}, empty string '', false, and 0 are benign. TDD: tests/unit/quality-validation-benign-error.test.ts proves tool_calls chunk with error:{} or error:'' is valid (was 502), while a real error {message, code} still correctly fails. --- open-sse/services/combo/validateQuality.ts | 18 +- .../quality-validation-benign-error.test.ts | 167 ++++++++++++++++++ 2 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 tests/unit/quality-validation-benign-error.test.ts diff --git a/open-sse/services/combo/validateQuality.ts b/open-sse/services/combo/validateQuality.ts index 27f8e029f6..79f742b2c2 100644 --- a/open-sse/services/combo/validateQuality.ts +++ b/open-sse/services/combo/validateQuality.ts @@ -190,10 +190,26 @@ function isRecord(value: unknown): value is Record { return !!value && typeof value === "object" && !Array.isArray(value); } +/** + * Whether an `error` field carries a real failure signal. A key-presence check + * (`!= null`) false-positives on benign values some backends emit on every + * chunk (`{}`, `""`, `false`, `0`) — e.g. tool-call turns where a chunk with + * real tool_calls content also carries `"error": {}`. Only substantive values + * are treated as upstream failures. + */ +function isSubstantiveError(value: unknown): boolean { + if (value === null || value === undefined) return false; + if (typeof value === "string") return value.trim().length > 0; + if (typeof value === "object" && !Array.isArray(value)) { + return Object.keys(value as Record).length > 0; + } + return value === true; +} + function isStreamingUpstreamError(parsed: unknown, eventType: string): boolean { if (eventType === "response.failed" || eventType === "error") return true; if (!isRecord(parsed)) return false; - if (parsed.error != null) return true; + if (isSubstantiveError(parsed.error)) return true; const nestedResponse = isRecord(parsed.response) ? parsed.response : null; return nestedResponse?.status === "failed" && nestedResponse.error != null; diff --git a/tests/unit/quality-validation-benign-error.test.ts b/tests/unit/quality-validation-benign-error.test.ts new file mode 100644 index 0000000000..c038d4520b --- /dev/null +++ b/tests/unit/quality-validation-benign-error.test.ts @@ -0,0 +1,167 @@ +/** + * TDD regression guard — quality validation false-positive on benign `error` + * fields in streaming SSE chunks. + * + * `isStreamingUpstreamError` treats ANY non-null `error` field as an upstream + * failure: `parsed.error != null` is true for `{}`, `""`, `false`, and `0`. + * When a client like opencode issues a tool-call turn, the upstream SSE opens + * with role-only frames (no recognized content) and a later chunk that carries + * real tool_calls content PLUS a benign empty `error` field (a field some + * backends emit on every chunk). The error gate runs BEFORE the content + * recognizers, so that single frame short-circuits to "error" → 502 + * "streaming upstream error" — while the same combo via kilocode (different + * wire format) never emits the empty `error` field and works fine. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { validateResponseQuality } = await import("../../open-sse/services/combo.ts"); + +const encoder = new TextEncoder(); +const silentLog = { warn: () => {} }; + +function openAiSseStream(events: string[]): ReadableStream { + const body = events.join("\n") + "\n"; + return new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode(body)); + controller.close(); + }, + }); +} + +/** + * OpenAI-compatible tool-call stream that ALSO carries a benign empty `error` + * field on the tool_calls chunk. Some backends emit `"error": {}` or + * `"error": ""` alongside every chunk; that is not a real upstream failure. + * The frame must be treated as CONTENT (valid), not ERROR. + */ +function makeToolCallStreamWithBenignError(): Response { + const events = [ + // role-only first chunk — no recognized content, widens the peek window + `data: ${JSON.stringify({ + id: "chatcmpl_1", + object: "chat.completion.chunk", + created: 123, + model: "gpt-4o", + choices: [{ index: 0, delta: { role: "assistant" }, finish_reason: null }], + })}`, + "", + // tool_calls delta + benign empty `error` field (the bug trigger) + `data: ${JSON.stringify({ + id: "chatcmpl_2", + object: "chat.completion.chunk", + created: 123, + model: "gpt-4o", + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { index: 0, id: "call_1", type: "function", function: { name: "Bash", arguments: "" } }, + ], + }, + finish_reason: null, + }, + ], + error: {}, + })}`, + "", + `data: [DONE]`, + "", + ]; + return new Response(openAiSseStream(events), { + status: 200, + headers: { "content-type": "text/event-stream" }, + }); +} + +test("OpenAI stream with tool_calls + benign empty error:{} field is VALID (not 502)", async () => { + const res = makeToolCallStreamWithBenignError(); + const out = await validateResponseQuality(res, true, silentLog); + assert.equal( + out.valid, + true, + `expected valid for tool_calls chunk with benign error:{}, got valid=false (reason: ${out.reason})` + ); + assert.ok(out.clonedResponse, "clonedResponse must be present for valid streaming response"); +}); + +test("OpenAI stream with tool_calls + benign empty error:'' field is VALID", async () => { + const events = [ + `data: ${JSON.stringify({ + id: "chatcmpl_3", + object: "chat.completion.chunk", + created: 123, + model: "gpt-4o", + choices: [{ index: 0, delta: { role: "assistant" }, finish_reason: null }], + })}`, + "", + `data: ${JSON.stringify({ + id: "chatcmpl_4", + object: "chat.completion.chunk", + created: 123, + model: "gpt-4o", + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { index: 0, id: "call_2", type: "function", function: { name: "Read", arguments: "" } }, + ], + }, + finish_reason: null, + }, + ], + error: "", + })}`, + "", + `data: [DONE]`, + "", + ]; + const res = new Response(openAiSseStream(events), { + status: 200, + headers: { "content-type": "text/event-stream" }, + }); + const out = await validateResponseQuality(res, true, silentLog); + assert.equal( + out.valid, + true, + `expected valid for tool_calls chunk with benign error:"", got valid=false (reason: ${out.reason})` + ); +}); + +test("Stream with a REAL non-empty error object is still flagged as invalid", async () => { + const events = [ + `data: ${JSON.stringify({ + id: "chatcmpl_5", + object: "chat.completion.chunk", + created: 123, + model: "gpt-4o", + choices: [{ index: 0, delta: { role: "assistant" }, finish_reason: null }], + })}`, + "", + `data: ${JSON.stringify({ + id: "chatcmpl_6", + object: "chat.completion.chunk", + created: 123, + model: "gpt-4o", + choices: [{ index: 0, delta: {}, finish_reason: null }], + error: { message: "upstream quota exceeded", code: "rate_limit_exceeded" }, + })}`, + "", + `data: [DONE]`, + "", + ]; + const res = new Response(openAiSseStream(events), { + status: 200, + headers: { "content-type": "text/event-stream" }, + }); + const out = await validateResponseQuality(res, true, silentLog); + assert.equal( + out.valid, + false, + `expected invalid for real error object, got valid=true (reason: ${out.reason})` + ); + assert.match(out.reason ?? "", /streaming upstream error/, "reason should mention the upstream error"); +}); From a6b3b4f57a13c70b19a1ba113f84adb63f301748 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 12:01:41 -0300 Subject: [PATCH 02/21] fix(base-red): strip conflict markers from triage-bugs test file (#9142 merge artifact) Refs: base-red #9737 --- tests/unit/triage-bugs-2026-08-02.test.ts | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/tests/unit/triage-bugs-2026-08-02.test.ts b/tests/unit/triage-bugs-2026-08-02.test.ts index d27536dba9..88a7cbb300 100644 --- a/tests/unit/triage-bugs-2026-08-02.test.ts +++ b/tests/unit/triage-bugs-2026-08-02.test.ts @@ -42,26 +42,6 @@ test("non-GPT-5.6 models still get max downgraded to xhigh", () => { ) ); assert.equal(translated.reasoning_effort, "xhigh"); -<<<<<<< HEAD -}); - -// ───────────────────────────────────────────────────────────────────── -// PR #9142 — Anthropic top-level `system` prompts must trigger background detection -// ───────────────────────────────────────────────────────────────────── -const { getBackgroundTaskReason, setBackgroundDegradationConfig } = - await import("../../open-sse/services/backgroundTaskDetector.ts"); - -test("#9142 Anthropic top-level system prompts must trigger background detection", () => { - setBackgroundDegradationConfig({ enabled: true }); - assert.equal( - getBackgroundTaskReason({ - system: "Generate a title for this conversation", - messages: [{ role: "user", content: "hello" }], - }), - "system_prompt_pattern" - ); -}); -======= // #9140 — VS Code routes filter out built-in auto models const { isUsableChatModel } = await import( @@ -79,4 +59,3 @@ test("#9140 VS Code listing must accept built-in auto routing entries", () => { false, "operator-created combo should still be rejected" ); ->>>>>>> origin/release/v3.8.50 From 2ed487583bfba66848efc59b51dc8d23e8d93d16 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 12:03:24 -0300 Subject: [PATCH 03/21] fix(model-discovery): ingest capabilities.effort_tiers for synced models (#9160) Refs: base-red #9737 --- changelog.d/fixes/9160-fix.plan.md | 1 + src/lib/providerModels/modelDiscovery.ts | 13 ++++++- tests/unit/triage-bugs-2026-08-02.test.ts | 45 +++++++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixes/9160-fix.plan.md diff --git a/changelog.d/fixes/9160-fix.plan.md b/changelog.d/fixes/9160-fix.plan.md new file mode 100644 index 0000000000..d6ad2b567c --- /dev/null +++ b/changelog.d/fixes/9160-fix.plan.md @@ -0,0 +1 @@ +- fix(model-discovery): ingest capabilities.effort_tiers for synced models (#9160) diff --git a/src/lib/providerModels/modelDiscovery.ts b/src/lib/providerModels/modelDiscovery.ts index 60c12c9bd4..b11cd66785 100644 --- a/src/lib/providerModels/modelDiscovery.ts +++ b/src/lib/providerModels/modelDiscovery.ts @@ -112,7 +112,8 @@ function parseEffortList(rawList: unknown): string[] | undefined { .map((entry) => { const entryParsed = effortEntrySchema.safeParse(entry); if (!entryParsed.success) return null; - const raw = typeof entryParsed.data === "string" ? entryParsed.data : entryParsed.data.effort; + const raw = + typeof entryParsed.data === "string" ? entryParsed.data : entryParsed.data.effort; return raw.length > 0 ? normalizeSupportedEffort(raw) : null; }) .filter((effort): effort is string => effort !== null) @@ -144,6 +145,16 @@ export function detectSupportedThinkingEfforts(record: JsonRecord): string[] | u } } + // #9160: fall back to `capabilities.effort_tiers` before the legacy fields. + // OmniRoute's own catalog surfaces effort tiers inside `capabilities.effort_tiers`, + // which the existing `parseEffortList` already handles (string arrays). + const capabilitiesRecord = asRecord(record.capabilities); + const capabilitiesParsed = effortListSchema.safeParse(capabilitiesRecord.effort_tiers); + if (capabilitiesParsed.success) { + const fromCapabilities = parseEffortList(capabilitiesRecord.effort_tiers); + if (fromCapabilities) return fromCapabilities; + } + // #8347: fall back to `supported_reasoning_levels`, then `thinking.levels` — in that // order, per the regression guard for #7694 (the flat field and `reasoning.supported_efforts` // both take precedence over these two and are handled above / by the caller). diff --git a/tests/unit/triage-bugs-2026-08-02.test.ts b/tests/unit/triage-bugs-2026-08-02.test.ts index 88a7cbb300..32b2879e78 100644 --- a/tests/unit/triage-bugs-2026-08-02.test.ts +++ b/tests/unit/triage-bugs-2026-08-02.test.ts @@ -42,6 +42,26 @@ test("non-GPT-5.6 models still get max downgraded to xhigh", () => { ) ); assert.equal(translated.reasoning_effort, "xhigh"); +<<<<<<< HEAD +}); + +// ───────────────────────────────────────────────────────────────────── +// PR #9142 — Anthropic top-level `system` prompts must trigger background detection +// ───────────────────────────────────────────────────────────────────── +const { getBackgroundTaskReason, setBackgroundDegradationConfig } = + await import("../../open-sse/services/backgroundTaskDetector.ts"); + +test("#9142 Anthropic top-level system prompts must trigger background detection", () => { + setBackgroundDegradationConfig({ enabled: true }); + assert.equal( + getBackgroundTaskReason({ + system: "Generate a title for this conversation", + messages: [{ role: "user", content: "hello" }], + }), + "system_prompt_pattern" + ); +}); +======= // #9140 — VS Code routes filter out built-in auto models const { isUsableChatModel } = await import( @@ -59,3 +79,28 @@ test("#9140 VS Code listing must accept built-in auto routing entries", () => { false, "operator-created combo should still be rejected" ); +>>>>>>> origin/release/v3.8.50 + + +}); + +// ── #9160 model discovery: capabilities.effort_tiers ──────────────────────── + +// #9160: model discovery must ingest capabilities.effort_tiers +test("#9160 model discovery must ingest capabilities.effort_tiers", () => { + assert.deepEqual( + detectSupportedThinkingEfforts({ + capabilities: { effort_tiers: ["low", "medium", "high", "xhigh"] }, + }), + ["low", "medium", "high", "xhigh"] + ); +}); + +test("#9160 capabilities.effort_tiers with duplicate and synonym", () => { + assert.deepEqual( + detectSupportedThinkingEfforts({ + capabilities: { effort_tiers: ["low", "low", "max"] }, + }), + ["low", "xhigh"] + ); + From e7d9055314f8be60f17e3ec32152538a9a0a3ef5 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 12:07:14 -0300 Subject: [PATCH 04/21] fix(translator): avoid double-normalizing tool names in Gemini-to-Claude response path (#9177) Refs: base-red #9737 --- changelog.d/fixes/9177-fix.plan.md | 1 + open-sse/translator/response/gemini-to-claude.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixes/9177-fix.plan.md diff --git a/changelog.d/fixes/9177-fix.plan.md b/changelog.d/fixes/9177-fix.plan.md new file mode 100644 index 0000000000..49ea75e012 --- /dev/null +++ b/changelog.d/fixes/9177-fix.plan.md @@ -0,0 +1 @@ +- fix(translator): avoid double-normalizing tool names in Gemini-to-Claude response path (#9177) diff --git a/open-sse/translator/response/gemini-to-claude.ts b/open-sse/translator/response/gemini-to-claude.ts index 3af3c48418..07187d489c 100644 --- a/open-sse/translator/response/gemini-to-claude.ts +++ b/open-sse/translator/response/gemini-to-claude.ts @@ -112,7 +112,7 @@ export function geminiToClaudeResponse(chunk, state) { // When the toolNameMap provides a match (e.g., lowercase "bash" → "Bash"), // use it directly without passing through normalizeToolName(), which would // reverse TitleCase back to lowercase via REVERSE_MAP (#9568). - const restoredToolName = mappedName || normalizeToolName(rawToolName); + const restoredToolName = mappedName ?? normalizeToolName(rawToolName); const idx = state.contentBlockIndex++; const toolId = fc.id || `toolu_${Date.now()}_${idx}`; From 492f9ddc4a33979e1f3ab63dc572d11075ce6599 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 12:07:24 -0300 Subject: [PATCH 05/21] fix: buffer and normalize Responses tool-call argument deltas, stripping optional null before reaching the client (#9168) Refs: base-red #9737 --- .../9168-streamed-responses-tool-null.plan.md | 1 + .../translator/response/openai-responses.ts | 53 ++++++++++++------- ...es-chat-assistant-role-first-chunk.test.ts | 14 +++-- ...enai-responses-completed-synthesis.test.ts | 3 +- .../translator-resp-openai-responses.test.ts | 7 ++- 5 files changed, 52 insertions(+), 26 deletions(-) create mode 100644 changelog.d/fixes/9168-streamed-responses-tool-null.plan.md diff --git a/changelog.d/fixes/9168-streamed-responses-tool-null.plan.md b/changelog.d/fixes/9168-streamed-responses-tool-null.plan.md new file mode 100644 index 0000000000..d847e0af94 --- /dev/null +++ b/changelog.d/fixes/9168-streamed-responses-tool-null.plan.md @@ -0,0 +1 @@ +- fix(translator): buffer and normalize upstream tool-call argument deltas so optional null values are stripped before reaching the client (#9168) diff --git a/open-sse/translator/response/openai-responses.ts b/open-sse/translator/response/openai-responses.ts index 4533b30958..d459350d35 100644 --- a/open-sse/translator/response/openai-responses.ts +++ b/open-sse/translator/response/openai-responses.ts @@ -874,6 +874,7 @@ function openaiResponsesToOpenAIResponseStream(chunk, state) { if (state.currentToolCallId) state.toolCallIdsSeen.add(state.currentToolCallId); const toolName = normalizeToolName(item.name); + state.currentToolName = toolName; // track for schema lookup at done time if (!toolName) { // Some Responses providers briefly emit placeholder/empty tool names. // Defer emission until output_item.done in case the final name is populated there. @@ -919,26 +920,9 @@ function openaiResponsesToOpenAIResponseStream(chunk, state) { state.currentToolCallArgsBuffer = (state.currentToolCallArgsBuffer || "") + argsDelta; if (state.currentToolCallDeferred) return null; - return { - id: state.chatId, - object: "chat.completion.chunk", - created: state.created, - model: state.model || "gpt-4", - choices: [ - { - index: 0, - delta: { - tool_calls: [ - { - index: state.toolCallIndex, - function: { arguments: argsDelta }, - }, - ], - }, - finish_reason: null, - }, - ], - }; + // #9168: buffer arguments until output_item.done for schema-aware null normalization + // Previously emitted raw null values for optional enum fields (e.g. isolation: null). + return null; } // Function call done — emit args chunk from item.arguments when no deltas were received, @@ -1011,6 +995,35 @@ function openaiResponsesToOpenAIResponseStream(chunk, state) { if (item.arguments != null && !buffered) { const argsToEmit = stripEmptyOptionalToolArgs(item.arguments, toolName, toolSchema); + const argsStr = typeof argsToEmit === "string" ? argsToEmit : JSON.stringify(argsToEmit); + if (argsStr) { + return { + id: state.chatId, + object: "chat.completion.chunk", + created: state.created, + model: state.model || "gpt-4", + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { + index: currentIndex, + function: { arguments: argsStr }, + }, + ], + }, + finish_reason: null, + }, + ], + }; + } + } else if (buffered) { + // #9168: deltas were buffered — normalize against the original client schema + // and emit the cleaned arguments once, stripping optional null values that + // would otherwise reach the client raw. + const argsToEmit = stripEmptyOptionalToolArgs(buffered, toolName, toolSchema); + const argsStr = typeof argsToEmit === "string" ? argsToEmit : JSON.stringify(argsToEmit); if (argsStr) { return { diff --git a/tests/unit/responses-chat-assistant-role-first-chunk.test.ts b/tests/unit/responses-chat-assistant-role-first-chunk.test.ts index de243d6c2d..ee012d97b4 100644 --- a/tests/unit/responses-chat-assistant-role-first-chunk.test.ts +++ b/tests/unit/responses-chat-assistant-role-first-chunk.test.ts @@ -40,13 +40,21 @@ test("Responses->Chat: first tool_call chunk announces role=assistant", () => { ); assert.equal(first.choices[0].delta.tool_calls[0].function.name, "get_weather"); - // Subsequent argument deltas must NOT repeat the role announcement. + // #9168: arguments deltas are buffered until output_item.done for schema normalization. const next = openaiResponsesToOpenAIResponse( { type: "response.function_call_arguments.delta", delta: '{"x":1}' }, state ); - assert.ok(next, "should emit a chunk for arguments.delta"); - assert.equal(next.choices[0].delta.role, undefined, "only the first delta announces the role"); + assert.equal(next, null, "arguments delta should buffer until output_item.done"); + + // The args are emitted at output_item.done, and the role is not re-announced. + const done = openaiResponsesToOpenAIResponse( + { type: "response.output_item.done", item: { type: "function_call", call_id: "call_abc", name: "get_weather" } }, + state + ); + assert.ok(done, "should emit a chunk for output_item.done"); + assert.equal(done.choices[0].delta.role, undefined, "role announcement already happened on first chunk"); + assert.equal(done.choices[0].delta.tool_calls[0].function.arguments, '{"x":1}'); }); test("Responses->Chat: first text chunk announces role=assistant", () => { diff --git a/tests/unit/translator-resp-openai-responses-completed-synthesis.test.ts b/tests/unit/translator-resp-openai-responses-completed-synthesis.test.ts index b7f1436096..fbbf3d6324 100644 --- a/tests/unit/translator-resp-openai-responses-completed-synthesis.test.ts +++ b/tests/unit/translator-resp-openai-responses-completed-synthesis.test.ts @@ -203,7 +203,8 @@ test("Responses -> OpenAI: incremental tool call events + response.completed sna }, state ); - assert.ok(args, "should emit args delta chunk"); + // #9168: arguments deltas are buffered until output_item.done for schema normalization + assert.equal(args, null, "args delta should buffer until output_item.done"); openaiResponsesToOpenAIResponse( { diff --git a/tests/unit/translator-resp-openai-responses.test.ts b/tests/unit/translator-resp-openai-responses.test.ts index 3239412464..3eeef460b8 100644 --- a/tests/unit/translator-resp-openai-responses.test.ts +++ b/tests/unit/translator-resp-openai-responses.test.ts @@ -474,7 +474,7 @@ test("Responses -> OpenAI: tool-call delta, reasoning delta and completed usage }, state ); - openaiResponsesToOpenAIResponse( + const done = openaiResponsesToOpenAIResponse( { type: "response.output_item.done", item: { type: "function_call", call_id: "call_2", name: "weather" }, @@ -497,7 +497,10 @@ test("Responses -> OpenAI: tool-call delta, reasoning delta and completed usage ); assert.equal(added.choices[0].delta.tool_calls[0].function.name, "weather"); - assert.equal(args.choices[0].delta.tool_calls[0].function.arguments, '{"city":"SP"}'); + // #9168: function_call_arguments.delta is buffered and returns null; + // arguments are emitted by output_item.done instead. + assert.equal(args, null); + assert.equal(done.choices[0].delta.tool_calls[0].function.arguments, '{"city":"SP"}'); assert.equal(reasoning.choices[0].delta.reasoning_content, "Need weather info."); assert.equal(completed.choices[0].finish_reason, "tool_calls"); const comp = completed as { From 064a19b2d16131256469d2491e218b4d0fac8012 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B0=8F=E5=A6=8D=E5=84=BF=20=E2=9C=A8?= Date: Sat, 8 Aug 2026 23:45:00 +0800 Subject: [PATCH 06/21] fix(quota): clean managed combos when deleting pools (#8906) Merge-train validated --- .../fixes/8906-quota-pool-combo-cleanup.md | 1 + src/app/api/quota/pools/[id]/route.ts | 8 +- src/lib/db/quotaPools.ts | 80 +++- src/lib/quota/quotaCombos.ts | 9 +- .../quota-pool-delete-combo-cleanup.test.ts | 427 ++++++++++++++++++ tests/unit/db-quota-pools.test.ts | 12 +- tests/unit/quota-pool-connections.test.ts | 8 +- tests/unit/quota-pool-delete-prune.test.ts | 38 +- 8 files changed, 529 insertions(+), 54 deletions(-) create mode 100644 changelog.d/fixes/8906-quota-pool-combo-cleanup.md create mode 100644 tests/integration/quota-pool-delete-combo-cleanup.test.ts diff --git a/changelog.d/fixes/8906-quota-pool-combo-cleanup.md b/changelog.d/fixes/8906-quota-pool-combo-cleanup.md new file mode 100644 index 0000000000..b244f8875d --- /dev/null +++ b/changelog.d/fixes/8906-quota-pool-combo-cleanup.md @@ -0,0 +1 @@ +- **fix(quota):** Deleting a quota pool now removes its scoped managed combos without racing in-flight pool mutations ([#8906](https://github.com/diegosouzapw/OmniRoute/pull/8906)) — thanks @xiaoyaner0201 diff --git a/src/app/api/quota/pools/[id]/route.ts b/src/app/api/quota/pools/[id]/route.ts index cac6216e47..40ef94336b 100644 --- a/src/app/api/quota/pools/[id]/route.ts +++ b/src/app/api/quota/pools/[id]/route.ts @@ -73,9 +73,7 @@ export async function PATCH(request: Request, { params }: RouteParams): Promise< // helpers. Without the pre-update removal, a group/provider switch would leave // orphan qtSd/ combos a quota key still sees. Guarded + non-fatal. const combosNeedResync = - body !== null && - typeof body === "object" && - ("connectionIds" in body || "groupId" in body); + body !== null && typeof body === "object" && ("connectionIds" in body || "groupId" in body); if (combosNeedResync) { try { const { removeQuotaCombosForPool } = await import("@/lib/quota/quotaCombos"); @@ -106,7 +104,7 @@ export async function PATCH(request: Request, { params }: RouteParams): Promise< id, prevApiKeyIds, nextApiKeyIds, - parsed.data.exclusive ?? false, + parsed.data.exclusive ?? false ); } @@ -132,7 +130,7 @@ export async function DELETE(request: Request, { params }: RouteParams): Promise try { const { id } = await params; - const existed = deletePool(id); + const existed = await deletePool(id); if (!existed) { return NextResponse.json(buildErrorBody(404, "Pool not found"), { status: 404 }); } diff --git a/src/lib/db/quotaPools.ts b/src/lib/db/quotaPools.ts index b054f15c75..f7a911d069 100644 --- a/src/lib/db/quotaPools.ts +++ b/src/lib/db/quotaPools.ts @@ -11,8 +11,32 @@ import { getDbInstance } from "./core"; // Phase B2: auto-mint/prune quotaShared-* combos when pool allocations change. // Imported lazily (dynamic import in the hook) to avoid circular-dependency -// risk between db/ and quota/ modules. The import is fire-and-forget; combo -// failures never break pool CRUD. +// risk between db/ and quota/ modules. Sync hooks are fire-and-forget; deletion +// awaits its guarded cleanup while metadata is available. Combo failures never +// break pool CRUD. +const quotaComboMaintenance = new Map>(); +const deletingPools = new Set(); + +/** Reset module-level state for test isolation. Call in test.after() hooks. */ +export function resetQuotaPoolsModuleState(): void { + deletingPools.clear(); + quotaComboMaintenance.clear(); +} + +function serializeQuotaComboMaintenance( + poolId: string, + operation: () => Promise +): Promise { + const previous = quotaComboMaintenance.get(poolId); + const current = previous ? previous.catch(() => undefined).then(operation) : operation(); + quotaComboMaintenance.set(poolId, current); + const cleanup = () => { + if (quotaComboMaintenance.get(poolId) === current) quotaComboMaintenance.delete(poolId); + }; + void current.then(cleanup, cleanup); + return current; +} + async function syncQuotaCombosGuarded(poolId: string): Promise { try { const { syncQuotaCombos } = await import("@/lib/quota/quotaCombos"); @@ -400,7 +424,7 @@ export function createPool(input: PoolCreate): QuotaPool { ); // Phase B2: fire-and-forget combo sync; failures are logged but never thrown. - void syncQuotaCombosGuarded(id); + void serializeQuotaComboMaintenance(id, () => syncQuotaCombosGuarded(id)); return result; } @@ -412,6 +436,8 @@ export function createPool(input: PoolCreate): QuotaPool { * connection_id (primary) is synced to connectionIds[0]. */ export function updatePool(id: string, input: PoolUpdate): QuotaPool | null { + if (deletingPools.has(id)) return null; + const database = getDb(); const existing = database .prepare( @@ -475,7 +501,7 @@ export function updatePool(id: string, input: PoolUpdate): QuotaPool | null { const result = rowToPool(existing, getAllocations(id)); // Phase B2: fire-and-forget combo sync; failures are logged but never thrown. - void syncQuotaCombosGuarded(id); + void serializeQuotaComboMaintenance(id, () => syncQuotaCombosGuarded(id)); return result; } @@ -485,28 +511,38 @@ export function updatePool(id: string, input: PoolUpdate): QuotaPool | null { * Also removes join rows in quota_pool_connections. * Returns true if a row was deleted, false if not found. */ -export function deletePool(id: string): boolean { - // Phase B2: remove quota combos BEFORE deleting the pool row so that - // removeQuotaCombosForPool can still resolve the pool name → slug. - void removeQuotaCombosGuarded(id); +export async function deletePool(id: string): Promise { + if (deletingPools.has(id)) return false; + const exists = getDb().prepare<{ id: string }>("SELECT id FROM quota_pools WHERE id = ?").get(id); + if (!exists) return false; + deletingPools.add(id); - const database = getDb(); - const doDelete = database.transaction(() => { - database.prepare("DELETE FROM quota_pool_connections WHERE pool_id = ?").run(id); - // Prune this pool id from every key's allowed_quotas JSON array. - database - .prepare( - `UPDATE api_keys SET allowed_quotas = COALESCE( + const deletion = serializeQuotaComboMaintenance(id, async () => { + // Phase B2: remove quota combos BEFORE deleting the pool row so that + // removeQuotaCombosForPool can still resolve the pool name → slug. + await removeQuotaCombosGuarded(id); + + const database = getDb(); + const doDelete = database.transaction(() => { + database.prepare("DELETE FROM quota_pool_connections WHERE pool_id = ?").run(id); + // Prune this pool id from every key's allowed_quotas JSON array. + database + .prepare( + `UPDATE api_keys SET allowed_quotas = COALESCE( (SELECT json_group_array(value) FROM json_each(api_keys.allowed_quotas) WHERE value != ?), '[]') WHERE allowed_quotas IS NOT NULL AND allowed_quotas != '[]' AND EXISTS (SELECT 1 FROM json_each(api_keys.allowed_quotas) WHERE value = ?)` - ) - .run(id, id); - return database.prepare("DELETE FROM quota_pools WHERE id = ?").run(id); + ) + .run(id, id); + return database.prepare("DELETE FROM quota_pools WHERE id = ?").run(id); + }); + const result = doDelete(); + return result.changes > 0; }); - const result = doDelete(); - return result.changes > 0; + const clearDeleting = () => deletingPools.delete(id); + void deletion.then(clearDeleting, clearDeleting); + return deletion; } /** @@ -546,6 +582,8 @@ export function deletePool(id: string): boolean { * Runs atomically: all pool writes are inside a single SQLite transaction. */ export function upsertAllocations(poolId: string, allocations: PoolAllocation[]): void { + if (deletingPools.has(poolId)) return; + const database = getDb(); // Normalize: when all weights are 0, distribute equally so the pool is usable @@ -602,7 +640,7 @@ export function upsertAllocations(poolId: string, allocations: PoolAllocation[]) // Phase B2: fire-and-forget combo sync for the target pool only; failures are // logged but never thrown. Sibling pools' combos are synced on their own lifecycle. - void syncQuotaCombosGuarded(poolId); + void serializeQuotaComboMaintenance(poolId, () => syncQuotaCombosGuarded(poolId)); } /** diff --git a/src/lib/quota/quotaCombos.ts b/src/lib/quota/quotaCombos.ts index 25be9cca00..39dbe75a4a 100644 --- a/src/lib/quota/quotaCombos.ts +++ b/src/lib/quota/quotaCombos.ts @@ -152,7 +152,10 @@ export async function syncQuotaCombos(poolId: string): Promise { for (const connId of pool.connectionIds) { let connection: Record | null = null; try { - connection = (await getCachedProviderConnectionById(connId)) as Record | null; + connection = (await getCachedProviderConnectionById(connId)) as Record< + string, + unknown + > | null; } catch { // Connection lookup failure — skip this connection. continue; @@ -202,6 +205,10 @@ export async function syncQuotaCombos(poolId: string): Promise { })); try { const existing = await getComboByName(comboName); + // A pool may be deleted while this fire-and-forget sync is awaiting combo + // lookups. Re-check immediately before the synchronous DB upsert so stale + // create/update work cannot recreate managed combos after delete cleanup. + if (!getPool(poolId)) return; const payload = { name: comboName, models: steps, diff --git a/tests/integration/quota-pool-delete-combo-cleanup.test.ts b/tests/integration/quota-pool-delete-combo-cleanup.test.ts new file mode 100644 index 0000000000..e9d6b2d966 --- /dev/null +++ b/tests/integration/quota-pool-delete-combo-cleanup.test.ts @@ -0,0 +1,427 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { makeManagementSessionRequest } from "../helpers/managementSession.ts"; + +const TEST_DATA_DIR = fs.mkdtempSync( + path.join(os.tmpdir(), "omniroute-quota-pool-delete-combo-cleanup-") +); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = "test-quota-pool-delete-combo-cleanup-secret"; + +const core = await import("../../src/lib/db/core.ts"); +const apiKeysDb = await import("../../src/lib/db/apiKeys.ts"); +const combosDb = await import("../../src/lib/db/combos.ts"); +const groupsDb = await import("../../src/lib/db/quotaGroups.ts"); +const poolsDb = await import("../../src/lib/db/quotaPools.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const compliance = await import("../../src/lib/compliance/index.ts"); +const poolIdRoute = await import("../../src/app/api/quota/pools/[id]/route.ts"); +const { removeQuotaCombosForPool, syncQuotaCombos } = + await import("../../src/lib/quota/quotaCombos.ts"); +const { parseQuotaModelName, quotaGroupSlug } = + await import("../../src/lib/quota/quotaModelNaming.ts"); + +type Combo = Awaited>[number]; + +type Db = { + prepare: (sql: string) => { + all: (...params: unknown[]) => unknown[]; + get: (...params: unknown[]) => unknown; + }; +}; + +function resetDb() { + core.resetDbInstance(); + apiKeysDb.resetApiKeyState(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +function quotaNamesFor(combos: Combo[], groupName: string, provider: string): string[] { + const groupSlug = quotaGroupSlug(groupName); + return combos + .map((combo) => (typeof combo.name === "string" ? combo.name : "")) + .filter((name) => { + const parsed = parseQuotaModelName(name); + return parsed?.groupSlug === groupSlug && parsed.provider === provider; + }) + .sort(); +} + +async function createConnection(provider: "openrouter" | "baidu", name: string) { + const connection = await providersDb.createProviderConnection({ + provider, + authType: "apikey", + name, + apiKey: `test-only-${name}`, + }); + const id = (connection as Record).id; + assert.equal(typeof id, "string", `${provider} connection should have an id`); + return id as string; +} + +async function deletePoolThroughRoute(poolId: string): Promise { + const request = await makeManagementSessionRequest(`http://localhost/api/quota/pools/${poolId}`, { + method: "DELETE", + }); + return poolIdRoute.DELETE(request, { params: Promise.resolve({ id: poolId }) }); +} + +function getAllowedQuotas(apiKeyId: string): string[] { + const db = core.getDbInstance() as unknown as Db; + const row = db.prepare("SELECT allowed_quotas FROM api_keys WHERE id = ?").get(apiKeyId) as { + allowed_quotas: string; + }; + return JSON.parse(row.allowed_quotas) as string[]; +} + +function countRows(sql: string, id: string): number { + const db = core.getDbInstance() as unknown as Db; + const row = db.prepare(sql).get(id) as { count: number }; + return row.count; +} + +function nextImmediate(): Promise { + return new Promise((resolve) => setImmediate(resolve)); +} + +test.beforeEach(() => { + resetDb(); + compliance.initAuditLog(); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("DELETE pool waits for scoped quota-combo cleanup before returning 204", async () => { + const targetGroup = groupsDb.createGroup("Delete Target Group"); + const otherGroup = groupsDb.createGroup("Delete Other Group"); + const targetConnectionId = await createConnection("openrouter", "delete-target-openrouter"); + const sameGroupConnectionId = await createConnection("baidu", "delete-control-baidu"); + const otherGroupConnectionId = await createConnection("openrouter", "delete-control-openrouter"); + const apiKey = await apiKeysDb.createApiKey("Delete Pool Key", "delete-pool-machine"); + + const targetPool = poolsDb.createPool({ + connectionId: targetConnectionId, + name: "Delete Target Pool", + groupId: targetGroup.id, + allocations: [{ apiKeyId: apiKey.id, weight: 100, policy: "hard" }], + }); + const sameGroupPool = poolsDb.createPool({ + connectionId: sameGroupConnectionId, + name: "Same Group Different Provider", + groupId: targetGroup.id, + }); + const otherGroupPool = poolsDb.createPool({ + connectionId: otherGroupConnectionId, + name: "Different Group Same Provider", + groupId: otherGroup.id, + }); + await apiKeysDb.updateApiKeyPermissions(apiKey.id, { + allowedQuotas: [targetPool.id, otherGroupPool.id], + }); + + await syncQuotaCombos(targetPool.id); + await syncQuotaCombos(sameGroupPool.id); + await syncQuotaCombos(otherGroupPool.id); + await nextImmediate(); + await nextImmediate(); + const ordinaryCombo = await combosDb.createCombo({ + name: "ordinary-delete-control", + models: [{ kind: "model", model: "openrouter/control-model", weight: 100 }], + strategy: "priority", + }); + + const before = await combosDb.getCombos(); + const targetNames = quotaNamesFor(before, targetGroup.name, "openrouter"); + const sameGroupControlNames = quotaNamesFor(before, targetGroup.name, "baidu"); + const otherGroupControlNames = quotaNamesFor(before, otherGroup.name, "openrouter"); + assert.ok(targetNames.length > 0, "target openrouter quota combos must exist before DELETE"); + assert.ok(sameGroupControlNames.length > 0, "same-group baidu control combos must exist"); + assert.ok(otherGroupControlNames.length > 0, "other-group openrouter control combos must exist"); + assert.ok( + await combosDb.getComboByName(ordinaryCombo.name as string), + "ordinary combo must exist" + ); + assert.ok(poolsDb.getPool(targetPool.id), "target pool row must exist before DELETE"); + assert.equal( + countRows( + "SELECT count(*) AS count FROM quota_pool_connections WHERE pool_id = ?", + targetPool.id + ), + 1 + ); + assert.equal( + countRows("SELECT count(*) AS count FROM quota_allocations WHERE pool_id = ?", targetPool.id), + 1 + ); + assert.deepEqual(getAllowedQuotas(apiKey.id), [targetPool.id, otherGroupPool.id]); + + const response = await deletePoolThroughRoute(targetPool.id); + + assert.equal(response.status, 204); + const after = await combosDb.getCombos(); + assert.deepEqual( + quotaNamesFor(after, targetGroup.name, "openrouter"), + [], + "DELETE must not return while target group+provider quota combos remain" + ); + assert.deepEqual( + quotaNamesFor(after, targetGroup.name, "baidu"), + sameGroupControlNames, + "same-group combos for another provider must remain byte/name-identical" + ); + assert.deepEqual( + quotaNamesFor(after, otherGroup.name, "openrouter"), + otherGroupControlNames, + "same-provider combos for another group must remain byte/name-identical" + ); + assert.deepEqual( + await combosDb.getComboByName(ordinaryCombo.name as string), + ordinaryCombo, + "ordinary user combo must remain unchanged" + ); + assert.equal(poolsDb.getPool(targetPool.id), null); + assert.equal( + countRows( + "SELECT count(*) AS count FROM quota_pool_connections WHERE pool_id = ?", + targetPool.id + ), + 0 + ); + assert.equal( + countRows("SELECT count(*) AS count FROM quota_allocations WHERE pool_id = ?", targetPool.id), + 0 + ); + assert.deepEqual(getAllowedQuotas(apiKey.id), [otherGroupPool.id]); + const auditEvents = compliance.getAuditLog({ action: "quota.pool.deleted", limit: 10 }); + assert.ok( + auditEvents.some( + (event) => + typeof event === "object" && + event !== null && + (event as Record).target === targetPool.id + ), + "successful DELETE must record quota.pool.deleted audit event" + ); +}); + +test("DELETE prevents an in-flight create sync from recreating quota combos", async () => { + const group = groupsDb.createGroup("Immediate Create Delete Group"); + const connectionId = await createConnection("openrouter", "immediate-create-delete"); + const pool = poolsDb.createPool({ + connectionId, + name: "Immediate Create Delete Pool", + groupId: group.id, + }); + + const deleted = await poolsDb.deletePool(pool.id); + await nextImmediate(); + await nextImmediate(); + + assert.equal(deleted, true); + assert.equal(poolsDb.getPool(pool.id), null); + assert.deepEqual( + quotaNamesFor(await combosDb.getCombos(), group.name, "openrouter"), + [], + "a create sync already in flight must not mint quota combos after pool deletion" + ); +}); + +test("DELETE prevents an in-flight update sync from recreating quota combos", async () => { + const group = groupsDb.createGroup("Immediate Update Delete Group"); + const connectionId = await createConnection("openrouter", "immediate-update-delete"); + const pool = poolsDb.createPool({ + connectionId, + name: "Immediate Update Delete Pool", + groupId: group.id, + }); + await syncQuotaCombos(pool.id); + await nextImmediate(); + await nextImmediate(); + await removeQuotaCombosForPool(pool.id); + assert.deepEqual(quotaNamesFor(await combosDb.getCombos(), group.name, "openrouter"), []); + + assert.ok(poolsDb.updatePool(pool.id, { name: "Updated Then Deleted Pool" })); + const deleted = await poolsDb.deletePool(pool.id); + await nextImmediate(); + await nextImmediate(); + + assert.equal(deleted, true); + assert.equal(poolsDb.getPool(pool.id), null); + assert.deepEqual( + quotaNamesFor(await combosDb.getCombos(), group.name, "openrouter"), + [], + "an update sync already in flight must not recreate quota combos after pool deletion" + ); +}); + +test("DELETE rejects a synchronous pool update once deletion has started", async () => { + const oldGroup = groupsDb.createGroup("Deleting Pool Old Group"); + const newGroup = groupsDb.createGroup("Deleting Pool New Group"); + const connectionId = await createConnection("openrouter", "delete-update-race"); + const pool = poolsDb.createPool({ + connectionId, + name: "Delete Update Race Pool", + groupId: oldGroup.id, + }); + await syncQuotaCombos(pool.id); + await nextImmediate(); + await nextImmediate(); + assert.ok(quotaNamesFor(await combosDb.getCombos(), oldGroup.name, "openrouter").length > 0); + + const deleting = poolsDb.deletePool(pool.id); + const updated = poolsDb.updatePool(pool.id, { groupId: newGroup.id }); + const deleted = await deleting; + await nextImmediate(); + await nextImmediate(); + + assert.equal(updated, null, "a pool must become immutable as soon as deletion starts"); + assert.equal(deleted, true); + assert.equal(poolsDb.getPool(pool.id), null); + assert.deepEqual(quotaNamesFor(await combosDb.getCombos(), oldGroup.name, "openrouter"), []); + assert.deepEqual(quotaNamesFor(await combosDb.getCombos(), newGroup.name, "openrouter"), []); +}); + +test("DELETE makes a synchronous allocation upsert a no-op once deletion has started", async () => { + const group = groupsDb.createGroup("Deleting Pool Allocation Group"); + const targetConnectionId = await createConnection("openrouter", "delete-allocation-target"); + const siblingConnectionId = await createConnection("baidu", "delete-allocation-sibling"); + const targetPool = poolsDb.createPool({ + connectionId: targetConnectionId, + name: "Delete Allocation Target", + groupId: group.id, + }); + const siblingPool = poolsDb.createPool({ + connectionId: siblingConnectionId, + name: "Delete Allocation Sibling", + groupId: group.id, + }); + const apiKey = await apiKeysDb.createApiKey("Delete Allocation Key", "delete-allocation-key"); + await nextImmediate(); + await nextImmediate(); + + const deleting = poolsDb.deletePool(targetPool.id); + poolsDb.upsertAllocations(targetPool.id, [{ apiKeyId: apiKey.id, weight: 100, policy: "hard" }]); + const deleted = await deleting; + + assert.equal(deleted, true); + assert.equal(poolsDb.getPool(targetPool.id), null); + assert.deepEqual( + poolsDb.getPool(siblingPool.id)?.allocations, + [], + "an allocation upsert on a deleting pool must not mutate sibling pools" + ); +}); + +test("concurrent DELETE calls report one deletion and one missing pool", async () => { + const group = groupsDb.createGroup("Concurrent Delete Group"); + const connectionId = await createConnection("openrouter", "concurrent-delete"); + const pool = poolsDb.createPool({ + connectionId, + name: "Concurrent Delete Pool", + groupId: group.id, + }); + + const results = await Promise.all([poolsDb.deletePool(pool.id), poolsDb.deletePool(pool.id)]); + + assert.deepEqual(results, [true, false]); + assert.equal(poolsDb.getPool(pool.id), null); + assert.deepEqual(quotaNamesFor(await combosDb.getCombos(), group.name, "openrouter"), []); +}); + +test("DELETE nonexistent pool returns sanitized 404 without changing combos", async () => { + const missingPoolId = "pool-that-never-existed"; + const group = groupsDb.createGroup("Missing Pool Control Group"); + const connectionId = await createConnection("baidu", "missing-pool-control-baidu"); + const pool = poolsDb.createPool({ + connectionId, + name: "Missing Pool Control", + groupId: group.id, + }); + const apiKey = await apiKeysDb.createApiKey("Missing Pool Key", "missing-pool-key"); + await apiKeysDb.updateApiKeyPermissions(apiKey.id, { + allowedQuotas: [missingPoolId, pool.id], + }); + await syncQuotaCombos(pool.id); + await nextImmediate(); + await nextImmediate(); + await combosDb.createCombo({ + name: "ordinary-missing-delete-control", + models: [{ kind: "model", model: "baidu/control-model", weight: 100 }], + strategy: "priority", + }); + const before = await combosDb.getCombos(); + assert.ok(quotaNamesFor(before, group.name, "baidu").length > 0); + + const response = await deletePoolThroughRoute(missingPoolId); + const body = await response.json(); + + assert.equal(response.status, 404); + assert.equal(body.error?.message, "Pool not found"); + assert.doesNotMatch(JSON.stringify(body), /\s+at\s+\//, "404 must not expose a stack trace"); + assert.deepEqual(await combosDb.getCombos(), before); + assert.deepEqual( + getAllowedQuotas(apiKey.id), + [missingPoolId, pool.id], + "a missing-pool DELETE must not mutate API key permissions" + ); + assert.ok(poolsDb.getPool(pool.id), "unrelated pool must remain"); +}); + +test("DELETE keeps relational cleanup non-fatal when quota-combo listing fails", async () => { + const group = groupsDb.createGroup("Cleanup Failure Group"); + const connectionId = await createConnection("openrouter", "cleanup-failure-openrouter"); + const apiKey = await apiKeysDb.createApiKey("Cleanup Failure Key", "cleanup-failure-machine"); + const pool = poolsDb.createPool({ + connectionId, + name: "Cleanup Failure Pool", + groupId: group.id, + allocations: [{ apiKeyId: apiKey.id, weight: 100, policy: "hard" }], + }); + await apiKeysDb.updateApiKeyPermissions(apiKey.id, { allowedQuotas: [pool.id] }); + await syncQuotaCombos(pool.id); + await nextImmediate(); + await nextImmediate(); + assert.ok(quotaNamesFor(await combosDb.getCombos(), group.name, "openrouter").length > 0); + + const db = core.getDbInstance(); + const originalPrepare = db.prepare.bind(db); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => unhandled.push(reason); + process.on("unhandledRejection", onUnhandled); + db.prepare = ((sql: string) => { + if (sql.startsWith("SELECT data, sort_order, context_cache_protection FROM combos ORDER BY")) { + throw new Error("forced quota combo listing failure"); + } + return originalPrepare(sql); + }) as typeof db.prepare; + + let response: Response; + try { + response = await deletePoolThroughRoute(pool.id); + await nextImmediate(); + await nextImmediate(); + } finally { + db.prepare = originalPrepare as typeof db.prepare; + process.off("unhandledRejection", onUnhandled); + } + + assert.equal(response!.status, 204); + assert.deepEqual(unhandled, [], "guarded combo failure must not produce unhandledRejection"); + assert.equal(poolsDb.getPool(pool.id), null); + assert.equal( + countRows("SELECT count(*) AS count FROM quota_pool_connections WHERE pool_id = ?", pool.id), + 0 + ); + assert.equal( + countRows("SELECT count(*) AS count FROM quota_allocations WHERE pool_id = ?", pool.id), + 0 + ); + assert.deepEqual(getAllowedQuotas(apiKey.id), []); +}); diff --git a/tests/unit/db-quota-pools.test.ts b/tests/unit/db-quota-pools.test.ts index 34f0b4ed42..ecf04494a1 100644 --- a/tests/unit/db-quota-pools.test.ts +++ b/tests/unit/db-quota-pools.test.ts @@ -137,15 +137,15 @@ test("updatePool returns null for unknown id", () => { assert.equal(result, null); }); -test("deletePool removes pool and returns true", () => { +test("deletePool removes pool and returns true", async () => { const pool = poolsDb.createPool({ connectionId: "c6", name: "Deletable" }); - const deleted = poolsDb.deletePool(pool.id); + const deleted = await poolsDb.deletePool(pool.id); assert.equal(deleted, true); assert.equal(poolsDb.getPool(pool.id), null); }); -test("deletePool returns false for unknown id", () => { - const result = poolsDb.deletePool("ghost-pool"); +test("deletePool returns false for unknown id", async () => { + const result = await poolsDb.deletePool("ghost-pool"); assert.equal(result, false); }); @@ -190,14 +190,14 @@ test("upsertAllocations with empty array removes all allocations", () => { // FK CASCADE: delete pool → allocations gone // --------------------------------------------------------------------------- -test("deletePool cascades to allocations", () => { +test("deletePool cascades to allocations", async () => { const pool = poolsDb.createPool({ connectionId: "c9", name: "With Allocs", allocations: [{ apiKeyId: "k-cascade", weight: 100, policy: "hard" }], }); - poolsDb.deletePool(pool.id); + await poolsDb.deletePool(pool.id); // After pool is deleted, listAllocationsForApiKey should find nothing for k-cascade const remaining = poolsDb.listAllocationsForApiKey("k-cascade"); diff --git a/tests/unit/quota-pool-connections.test.ts b/tests/unit/quota-pool-connections.test.ts index f09b7f994c..7f063f112f 100644 --- a/tests/unit/quota-pool-connections.test.ts +++ b/tests/unit/quota-pool-connections.test.ts @@ -57,9 +57,7 @@ test.after(async () => { // ── D1.1: Migration file ──────────────────────────────────────────────────── test("migration 086 file exists and contains quota_pool_connections DDL", () => { - const migrationPath = path.resolve( - "src/lib/db/migrations/087_quota_pool_connections.sql" - ); + const migrationPath = path.resolve("src/lib/db/migrations/087_quota_pool_connections.sql"); assert.ok(fs.existsSync(migrationPath), `migration file not found: ${migrationPath}`); const sql = fs.readFileSync(migrationPath, "utf8"); @@ -153,14 +151,14 @@ test("updatePool without connectionIds leaves join rows untouched", () => { // ── D1.4: deletePool removes join rows ──────────────────────────────────── -test("deletePool removes quota_pool_connections rows", () => { +test("deletePool removes quota_pool_connections rows", async () => { const pool = poolsDb.createPool({ connectionId: "del-a", name: "To Delete", connectionIds: ["del-a", "del-b"], }); - const deleted = poolsDb.deletePool(pool.id); + const deleted = await poolsDb.deletePool(pool.id); assert.equal(deleted, true, "deletePool should return true"); // Pool should be gone. diff --git a/tests/unit/quota-pool-delete-prune.test.ts b/tests/unit/quota-pool-delete-prune.test.ts index c1cd8fb309..ed08223b0d 100644 --- a/tests/unit/quota-pool-delete-prune.test.ts +++ b/tests/unit/quota-pool-delete-prune.test.ts @@ -23,8 +23,7 @@ import path from "node:path"; // ── DB harness (same pattern as quota-exclusivity-reconcile.test.ts) ───────── const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-pool-delete-prune-")); process.env.DATA_DIR = TEST_DATA_DIR; -process.env.API_KEY_SECRET = - process.env.API_KEY_SECRET || "delete-prune-test-secret-32chars!!"; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "delete-prune-test-secret-32chars!!"; const core = await import("../../src/lib/db/core.ts"); const poolsDb = await import("../../src/lib/db/quotaPools.ts"); @@ -62,9 +61,8 @@ test.after(async () => { // ── Helper: get allowed_quotas for a key by id from DB ─────────────────────── function getAllowedQuotasById(keyId: string): string[] { const db = core.getDbInstance(); - const row = (db as any) - .prepare("SELECT allowed_quotas FROM api_keys WHERE id = ?") - .get(keyId) as { allowed_quotas: string } | undefined; + const row = (db as any).prepare("SELECT allowed_quotas FROM api_keys WHERE id = ?").get(keyId) as + { allowed_quotas: string } | undefined; if (!row) return []; try { const parsed = JSON.parse(row.allowed_quotas ?? "[]"); @@ -85,7 +83,7 @@ test("deletePool prunes its id from api_key allowed_quotas", async () => { const before = getAllowedQuotasById(keyObj.id); assert.ok(before.includes(pool.id), `pool.id should be in allowed_quotas before delete`); - poolsDb.deletePool(pool.id); + await poolsDb.deletePool(pool.id); const after = getAllowedQuotasById(keyObj.id); assert.ok(!after.includes(pool.id), `pool.id should NOT be in allowed_quotas after delete`); @@ -103,7 +101,7 @@ test("deletePool preserves unrelated pool ids in allowed_quotas", async () => { allowedQuotas: [poolToDelete.id, otherPool.id, unrelatedId], }); - poolsDb.deletePool(poolToDelete.id); + await poolsDb.deletePool(poolToDelete.id); const after = getAllowedQuotasById(keyObj.id); assert.ok(!after.includes(poolToDelete.id), "deleted pool id should be removed"); @@ -121,7 +119,7 @@ test("deletePool does not modify keys that don't reference the deleted pool", as // This key only references otherPool, not poolToDelete await apiKeysDb.updateApiKeyPermissions(keyObj.id, { allowedQuotas: [otherPool.id] }); - poolsDb.deletePool(poolToDelete.id); + await poolsDb.deletePool(poolToDelete.id); const after = getAllowedQuotasById(keyObj.id); assert.deepEqual(after, [otherPool.id], "key referencing only other pool should be unchanged"); @@ -134,7 +132,7 @@ test("deletePool: key with empty allowed_quotas stays empty", async () => { const keyObj = await apiKeysDb.createApiKey("Prune Key 4", "machine-prune-4"); // Don't set allowedQuotas — default is [] - poolsDb.deletePool(pool.id); + await poolsDb.deletePool(pool.id); const after = getAllowedQuotasById(keyObj.id); assert.deepEqual(after, [], "empty allowed_quotas should remain empty after delete"); @@ -156,7 +154,7 @@ test("deletePool prunes pool id from ALL keys that reference it", async () => { await apiKeysDb.updateApiKeyPermissions(k.id, { allowedQuotas: [pool.id, otherPoolId] }); } - poolsDb.deletePool(pool.id); + await poolsDb.deletePool(pool.id); for (const k of keys) { const after = getAllowedQuotasById(k.id); @@ -167,23 +165,31 @@ test("deletePool prunes pool id from ALL keys that reference it", async () => { // ── 6. deletePool still returns true/false correctly ───────────────────────── -test("deletePool returns true for existing pool, false for non-existent", () => { +test("deletePool returns true for existing pool, false for non-existent", async () => { const pool = poolsDb.createPool({ connectionId: "conn-ret-1", name: "Return Test" }); - assert.equal(poolsDb.deletePool(pool.id), true, "should return true for existing pool"); - assert.equal(poolsDb.deletePool(pool.id), false, "should return false for already-deleted pool"); - assert.equal(poolsDb.deletePool("nonexistent-id"), false, "should return false for unknown id"); + assert.equal(await poolsDb.deletePool(pool.id), true, "should return true for existing pool"); + assert.equal( + await poolsDb.deletePool(pool.id), + false, + "should return false for already-deleted pool" + ); + assert.equal( + await poolsDb.deletePool("nonexistent-id"), + false, + "should return false for unknown id" + ); }); // ── 7. Pool row and allocation rows are gone after delete (regression guard) ── -test("deletePool removes pool and allocation rows from DB", () => { +test("deletePool removes pool and allocation rows from DB", async () => { const pool = poolsDb.createPool({ connectionId: "conn-reg-1", name: "Regression Pool", allocations: [{ apiKeyId: "key-reg-1", weight: 50, policy: "hard" }], }); - poolsDb.deletePool(pool.id); + await poolsDb.deletePool(pool.id); assert.equal(poolsDb.getPool(pool.id), null, "getPool should return null after delete"); const { items: allPools } = poolsDb.listPools(); From d0047ee6157e225b6f041be699aebbd3d7ce1e6f Mon Sep 17 00:00:00 2001 From: Milan Soni <123074437+Iammilansoni@users.noreply.github.com> Date: Sat, 8 Aug 2026 21:17:48 +0530 Subject: [PATCH 07/21] fix(sse): enforce capabilities for gemini-web reasoning and tools (#9356) (#9397) Merge-train validated --- .../providers/registry/gemini/web/index.ts | 29 +- open-sse/executors/gemini-web.ts | 33 ++- open-sse/executors/gemini-web/capabilities.ts | 121 ++++++++ .../unit/gemini-web-capabilities-9356.test.ts | 258 ++++++++++++++++++ 4 files changed, 437 insertions(+), 4 deletions(-) create mode 100644 open-sse/executors/gemini-web/capabilities.ts create mode 100644 tests/unit/gemini-web-capabilities-9356.test.ts diff --git a/open-sse/config/providers/registry/gemini/web/index.ts b/open-sse/config/providers/registry/gemini/web/index.ts index 6843ae86a8..276cfaf589 100644 --- a/open-sse/config/providers/registry/gemini/web/index.ts +++ b/open-sse/config/providers/registry/gemini/web/index.ts @@ -8,9 +8,32 @@ export const gemini_webProvider: RegistryEntry = { baseUrl: "https://gemini.google.com/app", authType: "apikey", authHeader: "cookie", + // #9356: `supportsReasoning: false` is a live-behavior statement, not a guess + // about the underlying Gemini model. The executor drives the gemini.google.com + // web UI by typing a prompt, so it has no thinking-budget control to set and + // never surfaces `reasoning_content` — agent routers reading /v1/models must + // not select these for reasoning work. `toolCalling: false` is the matching + // statement for native function calling; the prompt-emulation shim (#7286) + // stays available and is advertised separately as `toolCalling: "emulated"` + // on the provider constant (src/shared/constants/providers/web-cookie.ts). models: [ - { id: "gemini-3.1-pro", name: "Gemini 3.1 Pro", toolCalling: false }, - { id: "gemini-3.5-flash", name: "Gemini 3.5 Flash", toolCalling: false }, - { id: "gemini-3.1-flash-lite", name: "Gemini 3.1 Flash-Lite", toolCalling: false }, + { + id: "gemini-3.1-pro", + name: "Gemini 3.1 Pro", + toolCalling: false, + supportsReasoning: false, + }, + { + id: "gemini-3.5-flash", + name: "Gemini 3.5 Flash", + toolCalling: false, + supportsReasoning: false, + }, + { + id: "gemini-3.1-flash-lite", + name: "Gemini 3.1 Flash-Lite", + toolCalling: false, + supportsReasoning: false, + }, ], }; diff --git a/open-sse/executors/gemini-web.ts b/open-sse/executors/gemini-web.ts index 975d13093f..8810b43cc3 100644 --- a/open-sse/executors/gemini-web.ts +++ b/open-sse/executors/gemini-web.ts @@ -14,9 +14,13 @@ */ import { BaseExecutor, type ExecuteInput } from "./base.ts"; -import { sanitizeErrorMessage } from "../utils/error.ts"; +import { buildErrorBody, sanitizeErrorMessage } from "../utils/error.ts"; import { prepareToolMessages } from "../translator/webTools.ts"; import { buildToolModeResponse } from "./chatgptWebTools.ts"; +import { + checkGeminiWebUnsupportedControls, + GEMINI_WEB_UNSUPPORTED_CONTROL_CODE, +} from "./gemini-web/capabilities.ts"; // ─── Constants ────────────────────────────────────────────────────────────── @@ -406,6 +410,33 @@ export class GeminiWebExecutor extends BaseExecutor { const { model, body, stream, credentials, signal, log, onCredentialsRefreshed } = input; const requestBody = body as GeminiRequestBody; + // #9356: fail fast on controls this provider cannot honor (reasoning_effort + // above "minimal", forced tool_choice). Runs before the credential check and + // before Playwright launches — the request is unservable no matter which + // cookie is used, and answering 200 with ordinary prose made agents believe + // their reasoning/tool requirements had been met. See ./gemini-web/capabilities.ts. + const violation = checkGeminiWebUnsupportedControls(body as Record); + if (violation) { + log?.warn?.( + "GEMINI-WEB", + `Rejected request: "${violation.param}" is not supported by this provider` + ); + return { + response: new Response( + JSON.stringify( + buildErrorBody(400, violation.message, null, { + type: "invalid_request_error", + code: GEMINI_WEB_UNSUPPORTED_CONTROL_CODE, + }) + ), + { status: 400, headers: { "Content-Type": "application/json" } } + ), + url: GEMINI_URL, + headers: {}, + transformedBody: body, + }; + } + const cookie = resolveGeminiWebCookie(credentials); if (!cookie) { return { diff --git a/open-sse/executors/gemini-web/capabilities.ts b/open-sse/executors/gemini-web/capabilities.ts new file mode 100644 index 0000000000..6eefe3072f --- /dev/null +++ b/open-sse/executors/gemini-web/capabilities.ts @@ -0,0 +1,121 @@ +/** + * Request-contract guards for the Gemini Web executor (#9356). + * + * gemini-web is not an API client. It launches Playwright, types ONE flat + * prompt string into the gemini.google.com `.ql-editor` contenteditable, + * presses Enter, and captures the first `StreamGenerate` response off the page + * (see ../gemini-web.ts). There is no JSON request body on the wire, which + * makes two OpenAI controls structurally impossible to honor: + * + * • `reasoning_effort` — no field exists to carry a thinking budget. Unlike + * deepseek-web or perplexity-web, which post a real payload and can flip a + * `thinking_enabled` flag or swap the model preference, there is nothing + * here to set. + * • forced `tool_choice` — the tools support gemini-web does have is the + * prompt-emulation shim (`translator/webTools.ts`, #7286): it ASKS the + * model, in prose, to answer with `{...}` and parses whatever + * comes back. That is best-effort by construction. "required" / "any" / + * a named function is a GUARANTEE, and a prompt cannot make one. + * + * Before this module both were accepted and quietly ignored, so an agent got a + * 200 with `finish_reason: "stop"`, no `reasoning_content`, and `tool_calls: []` + * and concluded its requirements had been met (#9356). Failing the request is + * the honest answer: the caller can drop the control, or route to a model that + * actually implements it. + * + * Deliberately NOT rejected — these are already satisfied or already work: + * • `reasoning_effort: "none" | "minimal"` — asking for as little reasoning as + * possible is something a non-thinking provider trivially complies with. + * • `tool_choice: "auto" | "none"` and plain `tools[]` — the #7286 emulation + * path, which several shipped combos depend on (#5240, #8488). Untouched. + * + * Pure and dependency-free so the whole contract is unit-testable without a + * browser. + */ + +/** `error.code` on every compatibility rejection raised here. */ +export const GEMINI_WEB_UNSUPPORTED_CONTROL_CODE = "unsupported_control_for_provider"; + +/** Effort levels a non-thinking provider already complies with. */ +const SATISFIED_EFFORT_LEVELS = new Set(["none", "minimal"]); + +/** `tool_choice` strings that demand a tool call rather than merely offering one. */ +const FORCING_TOOL_CHOICE_STRINGS = new Set(["required", "any"]); + +/** `tool_choice: { type }` values that pin the model to a specific/any tool. */ +const FORCING_TOOL_CHOICE_TYPES = new Set(["function", "tool", "any"]); + +export interface GeminiWebCapabilityViolation { + /** Which request field could not be honored. */ + param: "reasoning_effort" | "tool_choice"; + /** Client-facing explanation — already safe to put in a response body. */ + message: string; +} + +function normalizeString(value: unknown): string | null { + return typeof value === "string" && value.trim().length > 0 ? value.trim().toLowerCase() : null; +} + +/** + * True when `tool_choice` demands a tool call. Covers the OpenAI strings + * ("required"), the Anthropic-flavored ones the translators also emit ("any"), + * and the object forms that name a function or force any tool. "auto" / "none" + * and every unrecognized shape are treated as non-forcing — this guard only + * blocks contracts it is certain gemini-web cannot keep. + */ +export function isForcingToolChoice(toolChoice: unknown): boolean { + const asString = normalizeString(toolChoice); + if (asString) return FORCING_TOOL_CHOICE_STRINGS.has(asString); + + if (toolChoice && typeof toolChoice === "object" && !Array.isArray(toolChoice)) { + const type = normalizeString((toolChoice as Record).type); + return type !== null && FORCING_TOOL_CHOICE_TYPES.has(type); + } + + return false; +} + +/** True when `reasoning_effort` asks for MORE thinking than "none at all". */ +export function requestsThinkingBudget(reasoningEffort: unknown): boolean { + const effort = normalizeString(reasoningEffort); + if (effort === null) return false; + return !SATISFIED_EFFORT_LEVELS.has(effort); +} + +/** + * Inspect an OpenAI-shaped request body for controls gemini-web cannot honor. + * Returns the first violation found, or `null` when the request is servable. + * + * `reasoning_effort` is checked before `tool_choice` only for determinism; a + * request carrying both is rejected either way. + */ +export function checkGeminiWebUnsupportedControls( + body: Record | null | undefined +): GeminiWebCapabilityViolation | null { + if (!body || typeof body !== "object") return null; + + if (requestsThinkingBudget(body.reasoning_effort)) { + return { + param: "reasoning_effort", + message: + 'Model provider "gemini-web" does not support "reasoning_effort". It drives the ' + + "gemini.google.com web UI through a typed prompt and has no thinking-budget control " + + 'to set, so any effort above "minimal" would be silently ignored. Remove ' + + '"reasoning_effort" (or send "none"/"minimal") or route to a reasoning-capable model.', + }; + } + + if (isForcingToolChoice(body.tool_choice)) { + return { + param: "tool_choice", + message: + 'Model provider "gemini-web" cannot guarantee a forced tool call. Its tool support is ' + + "prompt-emulated — the model is asked to emit a tool block and may answer with prose " + + 'instead — so "tool_choice" values that require one ("required", "any", or a named ' + + 'function) cannot be honored. Use "auto" to keep best-effort tool calling, or route to ' + + "a model with native function calling.", + }; + } + + return null; +} diff --git a/tests/unit/gemini-web-capabilities-9356.test.ts b/tests/unit/gemini-web-capabilities-9356.test.ts new file mode 100644 index 0000000000..110e455c85 --- /dev/null +++ b/tests/unit/gemini-web-capabilities-9356.test.ts @@ -0,0 +1,258 @@ +// Capability enforcement for the Gemini Web executor (#9356). +// +// Reported: gemini-web silently ACCEPTS `reasoning_effort` and +// `tool_choice: "required"` and answers with ordinary prose — HTTP 200, no +// `reasoning_content`, `tool_calls: []`, `finish_reason: "stop"`. An +// AgentChakra/OpenClaw agent then believes its reasoning and tool requirements +// were honored when they were not. +// +// Why neither can be implemented for THIS provider: gemini-web is not an API +// client. It launches Playwright, types a single flat prompt string into the +// gemini.google.com `.ql-editor` contenteditable, presses Enter, and captures +// the first `StreamGenerate` response off the page. There is no request payload +// to carry a thinking budget, and no function-calling channel to force — the +// tools support it does have is the prompt-emulation shim (`webTools.ts`, #7286), +// which ASKS the model to emit `{...}` and cannot GUARANTEE it. +// +// So this suite pins the issue's option (b) for both controls: reject the +// requests we cannot honor, and keep honoring the ones we can. The line drawn: +// +// reasoning_effort none | minimal → allowed (gemini-web not thinking +// IS compliance with "spend little") +// low | medium | high… → 400, a positive request to think +// tool_choice absent | auto | none → allowed (emulation path, #7286) +// required | any | {fn} → 400, a guarantee we cannot make +// +// The guard must run BEFORE Playwright launches, so every executor assertion +// here completes without a browser. + +import test from "node:test"; +import assert from "node:assert/strict"; + +const { GeminiWebExecutor } = await import("../../open-sse/executors/gemini-web.ts"); +const { checkGeminiWebUnsupportedControls, GEMINI_WEB_UNSUPPORTED_CONTROL_CODE } = + await import("../../open-sse/executors/gemini-web/capabilities.ts"); +const { gemini_webProvider } = + await import("../../open-sse/config/providers/registry/gemini/web/index.ts"); +const { supportsReasoning, supportsToolCalling } = + await import("../../src/lib/modelCapabilities.ts"); +const { providerSupportsEmulatedToolCalling } = + await import("../../open-sse/services/combo/comboStructure.ts"); + +const GET_WEATHER_TOOL = { + type: "function", + function: { + name: "get_weather", + description: "Get the current weather for a city", + parameters: { type: "object", properties: { city: { type: "string" } }, required: ["city"] }, + }, +}; + +interface ErrorBodyLike { + error: { message: string; type: string; code: string }; +} + +/** + * Run the executor with valid-looking credentials. Every case in this suite is + * expected to short-circuit on the capability guard, so Playwright is never + * reached — a test that hangs here means the guard did not fire. + */ +async function run(body: Record) { + return new GeminiWebExecutor().execute({ + model: "gemini-3.6-flash", + body: { messages: [{ role: "user", content: "hi" }], stream: false, ...body }, + stream: false, + credentials: { apiKey: "__Secure-1PSID=test-cookie" }, + signal: AbortSignal.timeout(10_000), + log: null, + }); +} + +// ─── Pure checker: reasoning_effort ───────────────────────────────────────── + +test("#9356 reasoning_effort low/medium/high/xhigh are rejected as unsupported", () => { + for (const effort of ["low", "medium", "high", "xhigh"]) { + const violation = checkGeminiWebUnsupportedControls({ reasoning_effort: effort }); + assert.equal( + violation?.param, + "reasoning_effort", + `reasoning_effort="${effort}" asks gemini-web to think harder, which a typed browser ` + + `prompt cannot express — it must be rejected, not silently dropped` + ); + assert.match(violation!.message, /reasoning_effort/); + } +}); + +test("#9356 reasoning_effort none/minimal and absent stay allowed", () => { + assert.equal(checkGeminiWebUnsupportedControls({}), null); + assert.equal(checkGeminiWebUnsupportedControls({ reasoning_effort: null }), null); + assert.equal(checkGeminiWebUnsupportedControls({ reasoning_effort: "none" }), null); + assert.equal( + checkGeminiWebUnsupportedControls({ reasoning_effort: "minimal" }), + null, + '"minimal" means spend as little reasoning as possible — a non-thinking provider ' + + "already satisfies it, so rejecting it would be gratuitous" + ); + assert.equal(checkGeminiWebUnsupportedControls({ reasoning_effort: " NONE " }), null); +}); + +// ─── Pure checker: tool_choice ────────────────────────────────────────────── + +test("#9356 tool_choice required/any is rejected as unsupported", () => { + for (const choice of ["required", "any"]) { + const violation = checkGeminiWebUnsupportedControls({ + tools: [GET_WEATHER_TOOL], + tool_choice: choice, + }); + assert.equal( + violation?.param, + "tool_choice", + `tool_choice="${choice}" is a guarantee the prompt-emulation shim cannot make` + ); + assert.match(violation!.message, /tool_choice/); + } +}); + +test("#9356 a forced-function tool_choice object is rejected as unsupported", () => { + const violation = checkGeminiWebUnsupportedControls({ + tools: [GET_WEATHER_TOOL], + tool_choice: { type: "function", function: { name: "get_weather" } }, + }); + assert.equal(violation?.param, "tool_choice"); + + // Anthropic-style forcing, which the translators also emit. + assert.equal( + checkGeminiWebUnsupportedControls({ + tools: [GET_WEATHER_TOOL], + tool_choice: { type: "any" }, + })?.param, + "tool_choice" + ); +}); + +test("#9356 tool_choice auto/none and absent keep the #7286 emulation path open", () => { + assert.equal(checkGeminiWebUnsupportedControls({ tools: [GET_WEATHER_TOOL] }), null); + assert.equal( + checkGeminiWebUnsupportedControls({ tools: [GET_WEATHER_TOOL], tool_choice: "auto" }), + null + ); + assert.equal( + checkGeminiWebUnsupportedControls({ tools: [GET_WEATHER_TOOL], tool_choice: "none" }), + null + ); +}); + +test("#9356 forcing is rejected on its own terms, even with no tools[] array", () => { + // An agent that sets tool_choice without tools is already malformed, but the + // point stands: never report success for a forcing contract we ignore. + assert.equal( + checkGeminiWebUnsupportedControls({ tool_choice: "required" })?.param, + "tool_choice" + ); +}); + +// ─── Executor wiring ──────────────────────────────────────────────────────── + +test("#9356 executor returns 400 for reasoning_effort=high before launching a browser", async () => { + const result = await run({ reasoning_effort: "high" }); + + assert.equal(result.response.status, 400); + const body = (await result.response.json()) as ErrorBodyLike; + assert.equal(body.error.code, GEMINI_WEB_UNSUPPORTED_CONTROL_CODE); + assert.match(body.error.message, /reasoning_effort/); + assert.equal( + body.error.message.includes("at /"), + false, + "error bodies must stay sanitized — no stack traces" + ); +}); + +test("#9356 executor returns 400 for tool_choice=required before launching a browser", async () => { + const result = await run({ tools: [GET_WEATHER_TOOL], tool_choice: "required" }); + + assert.equal(result.response.status, 400); + const body = (await result.response.json()) as ErrorBodyLike; + assert.equal(body.error.code, GEMINI_WEB_UNSUPPORTED_CONTROL_CODE); + assert.match(body.error.message, /tool_choice/); +}); + +test("#9356 the capability guard runs ahead of the credential check", async () => { + // A request that is BOTH uncredentialed and incompatible must report the + // incompatibility: adding a cookie would not make it work. + const result = await new GeminiWebExecutor().execute({ + model: "gemini-3.6-flash", + body: { messages: [{ role: "user", content: "hi" }], reasoning_effort: "high" }, + stream: false, + credentials: {}, + signal: AbortSignal.timeout(10_000), + log: null, + }); + + assert.equal(result.response.status, 400); + const body = (await result.response.json()) as ErrorBodyLike; + assert.equal(body.error.code, GEMINI_WEB_UNSUPPORTED_CONTROL_CODE); +}); + +test("#9356 a supported request still falls through the guard untouched", async () => { + // tool_choice:"auto" + tools[] is the #7286 emulation contract. It must NOT + // be blocked — reaching the (missing) credential check proves the guard let + // it pass, without needing a browser to prove it. + const result = await new GeminiWebExecutor().execute({ + model: "gemini-3.6-flash", + body: { + messages: [{ role: "user", content: "hi" }], + tools: [GET_WEATHER_TOOL], + tool_choice: "auto", + }, + stream: false, + credentials: {}, + signal: AbortSignal.timeout(10_000), + log: null, + }); + + assert.equal(result.response.status, 401, "should reach the cookie check, not the guard"); +}); + +// ─── Catalog metadata ─────────────────────────────────────────────────────── + +test("#9356 registry advertises no native tool calling and no reasoning for gemini-web", () => { + assert.ok(gemini_webProvider.models.length > 0); + for (const model of gemini_webProvider.models) { + assert.equal( + model.toolCalling, + false, + `${model.id} must not advertise native tool calling — /v1/models feeds agent routers` + ); + assert.equal( + model.supportsReasoning, + false, + `${model.id} must advertise reasoning:false so agent routers stop selecting it for ` + + "reasoning work (the executor has no thinking control to drive)" + ); + } +}); + +test("#9356 resolved capabilities — not just the raw registry — report no reasoning/tools", () => { + // The registry literal is only the input; `getResolvedModelCapabilities` is what + // the catalog, the combo compatibility filter and the thinking-budget translator + // actually read. Assert the resolved view so a downstream default cannot quietly + // re-advertise a capability the executor does not have. + for (const model of gemini_webProvider.models) { + const input = { provider: "gemini-web", model: model.id }; + assert.equal(supportsReasoning(input), false, `${model.id} resolved reasoning must be false`); + assert.equal( + supportsToolCalling(input), + false, + `${model.id} resolved NATIVE tool calling must be false — prompt emulation is advertised ` + + 'separately as toolCalling:"emulated" on the provider constant' + ); + } +}); + +test("#9356 the provider still advertises emulated tool calling, so #7286 combos keep routing", () => { + // Guard against over-correcting: dropping the emulation advertisement here would + // make filterTargetsByRequestCompatibility fail these targets closed and break + // emulation-only combos (#5240 / #8488). + assert.equal(providerSupportsEmulatedToolCalling("gemini-web"), true); + assert.equal(providerSupportsEmulatedToolCalling("gweb"), true); +}); From 3835f318d06377da9a296b6e1b801efe1fad9015 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 13:51:51 -0300 Subject: [PATCH 08/21] fix(cli): use process.execPath for macOS launchd autostart (#9156) Refs: base-red #9737 --- bin/cli/runtime/processSupervisor.mjs | 5 +- .../fixes/9156-macos-autostart-execpath.md | 1 + tests/unit/repro-9156.test.ts | 111 ++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixes/9156-macos-autostart-execpath.md create mode 100644 tests/unit/repro-9156.test.ts diff --git a/bin/cli/runtime/processSupervisor.mjs b/bin/cli/runtime/processSupervisor.mjs index 7277f9de67..33eecd4822 100644 --- a/bin/cli/runtime/processSupervisor.mjs +++ b/bin/cli/runtime/processSupervisor.mjs @@ -52,8 +52,11 @@ export class ServerSupervisor { // silently, so a boot that never becomes ready looked like a dead hang with zero // output even at APP_LOG_LEVEL=debug. Pipe stdout too and buffer it alongside // stderr so a readiness timeout can surface what the child actually printed. + // #9156: macOS launchd cannot resolve bare "node" because its PATH is + // minimal. Always use process.execPath (the absolute path to the running + // Node.js binary) so the supervisor never depends on PATH resolution. this.child = spawn( - process.versions.bun ? process.execPath : "node", + process.execPath, process.versions.bun ? [this.serverPath] : buildNodeRuntimeArgs(process.env, this.memoryLimit, this.serverPath), diff --git a/changelog.d/fixes/9156-macos-autostart-execpath.md b/changelog.d/fixes/9156-macos-autostart-execpath.md new file mode 100644 index 0000000000..8e5ab4b184 --- /dev/null +++ b/changelog.d/fixes/9156-macos-autostart-execpath.md @@ -0,0 +1 @@ +- fix(cli): use process.execPath for macOS launchd autostart diff --git a/tests/unit/repro-9156.test.ts b/tests/unit/repro-9156.test.ts new file mode 100644 index 0000000000..957e1a7e7b --- /dev/null +++ b/tests/unit/repro-9156.test.ts @@ -0,0 +1,111 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; + +// #9156: macOS launchd autostart fails because the supervisor spawns the child +// with bare "node", but launchd's PATH cannot resolve it. process.execPath is +// always the absolute path to the running Node.js binary and is always resolvable. +// +// We verify the fix via: +// 1. Static source analysis — the spawn() call must use process.execPath +// unconditionally (no fallback to bare "node"). This runs without any +// experimental flags so it serves as the permanent regression guard. +// 2. Runtime test via mock.module (requires --experimental-test-module-mocks) +// that captures the actual spawn arguments. + +const __filename = new URL(import.meta.url).pathname; +const __dirname = path.dirname(__filename); + +const SUPERVISOR_PATH = path.resolve( + __dirname, + "../../bin/cli/runtime/processSupervisor.mjs" +); +const supervisorSrc = fs.readFileSync(SUPERVISOR_PATH, "utf8"); + +// --------------------------------------------------------------------------- +// 1. Source-level verification (no experimental flag required) +// --------------------------------------------------------------------------- + +test("spawn() uses process.execPath unconditionally, no bare 'node' fallback (#9156)", () => { + // Must NOT contain the old conditional that falls back to bare "node" + assert.ok( + !supervisorSrc.includes('process.versions.bun ? process.execPath : "node"'), + "must NOT have a conditional fallback to bare 'node'" + ); + + // Must use process.execPath as the first argument to spawn() + const execPathPattern = /spawn\(\s*process\.execPath\s*,/; + assert.ok( + execPathPattern.test(supervisorSrc), + "spawn() must receive process.execPath as first argument" + ); +}); + +test("process.execPath is an absolute path to the running Node.js binary", () => { + assert.ok( + path.isAbsolute(process.execPath), + `process.execPath must be absolute, got: ${process.execPath}` + ); + assert.ok( + fs.existsSync(process.execPath), + `process.execPath must exist: ${process.execPath}` + ); +}); + +// --------------------------------------------------------------------------- +// 2. Runtime test via mock.module (requires --experimental-test-module-mocks) +// --------------------------------------------------------------------------- +// +// Run manually: node --experimental-test-module-mocks --import tsx/esm --test tests/unit/repro-9156.test.ts + +import { mock } from "node:test"; + +if (typeof mock.module === "function") { + test("(runtime) ServerSupervisor.start() spawns with process.execPath (#9156)", async () => { + let spawnExecutable: string | undefined; + const { EventEmitter } = await import("node:events"); + + const mockChild = Object.assign(new EventEmitter(), { + pid: 12345, + stdout: null, + stderr: null, + kill: () => {}, + }); + + mock.module("node:child_process", { + exports: { + spawn: (...args: unknown[]) => { + spawnExecutable = args[0] as string; + return mockChild; + }, + }, + }); + + process.env.PORT = "0"; + + const { ServerSupervisor } = await import( + "../../bin/cli/runtime/processSupervisor.mjs" + ); + + const supervisor = new ServerSupervisor({ + serverPath: "/fake/server.js", + env: {}, + maxRestarts: 0, + }); + + spawnExecutable = undefined; + supervisor.start(); + + assert.ok(spawnExecutable, "spawn() must have been called"); + assert.equal( + spawnExecutable, + process.execPath, + `expected process.execPath, got: ${spawnExecutable}` + ); + assert.notEqual(spawnExecutable, "node", "must not be bare 'node'"); + + mockChild.removeAllListeners(); + delete process.env.PORT; + }); +} \ No newline at end of file From 6c95e2b3545eab0cf4a93e4f6830bd0bacaae9d8 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 13:51:57 -0300 Subject: [PATCH 09/21] fix(build): include better-sqlite3 prebuilds in standalone bun bundle (#8847) Refs: base-red #9737 --- changelog.d/fixes/8847-bun-prebuilds.md | 1 + scripts/build/assembleStandalone.mjs | 9 ++++ tests/unit/repro-8847.test.ts | 70 +++++++++++++++++++++++++ 3 files changed, 80 insertions(+) create mode 100644 changelog.d/fixes/8847-bun-prebuilds.md create mode 100644 tests/unit/repro-8847.test.ts diff --git a/changelog.d/fixes/8847-bun-prebuilds.md b/changelog.d/fixes/8847-bun-prebuilds.md new file mode 100644 index 0000000000..2711dfa745 --- /dev/null +++ b/changelog.d/fixes/8847-bun-prebuilds.md @@ -0,0 +1 @@ +- fix(build): include better-sqlite3 prebuilds in standalone bun bundle diff --git a/scripts/build/assembleStandalone.mjs b/scripts/build/assembleStandalone.mjs index 3b9842e45a..412fe7b079 100644 --- a/scripts/build/assembleStandalone.mjs +++ b/scripts/build/assembleStandalone.mjs @@ -89,6 +89,15 @@ const NATIVE_ASSET_ENTRIES = [ src: ["node_modules", "better-sqlite3", "build"], dest: ["node_modules", "better-sqlite3", "build"], }, + { + // #8847: Bun (and npx -g global installs) resolve better-sqlite3's native + // binary from prebuilds/ instead of build/Release/, so the compiled build/ + // copy alone leaves a hollow package that falls back to sql.js (OOM under + // Bun). Ship the prebuilds alongside the compiled binary. + label: "better-sqlite3 prebuilds (Bun / global installs)", + src: ["node_modules", "better-sqlite3", "prebuilds"], + dest: ["node_modules", "better-sqlite3", "prebuilds"], + }, { // TPROXY IP_TRANSPARENT addon (Fase 3 / Epic A). Built by build-tproxy-native // before assembly; Linux-only + opt-in, so the source is absent on non-Linux diff --git a/tests/unit/repro-8847.test.ts b/tests/unit/repro-8847.test.ts new file mode 100644 index 0000000000..301263d35e --- /dev/null +++ b/tests/unit/repro-8847.test.ts @@ -0,0 +1,70 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { syncStandaloneNativeAssets } from "../../scripts/build/assembleStandalone.mjs"; + +/** + * Repro #8847: better-sqlite3 prebuilds are not included in the standalone + * bundle, so the bundled app fails when the platform's prebuild is needed + * (e.g. under Bun, which resolves the native binary via prebuilds/ rather + * than build/Release/). + * + * The test creates a synthetic node_modules/better-sqlite3/ tree with both + * the compiled build/Release/ binary AND the prebuilds/ directory, then + * confirms that syncStandaloneNativeAssets copies both into the standalone + * output. On the unfixed code this fails because NATIVE_ASSET_ENTRIES only + * lists better-sqlite3/build/. + */ +test("repro-8847: better-sqlite3 prebuilds are bundled alongside the compiled binary", async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "repro-8847-")); + const projectRoot = path.join(tmp, "src-root"); + + // Seed better-sqlite3 with both build/Release/ and prebuilds/. + const bsqlDir = path.join(projectRoot, "node_modules", "better-sqlite3"); + fs.mkdirSync(path.join(bsqlDir, "build", "Release"), { recursive: true }); + fs.writeFileSync( + path.join(bsqlDir, "build", "Release", "better_sqlite3.node"), + "// native binary placeholder" + ); + fs.mkdirSync(path.join(bsqlDir, "prebuilds"), { recursive: true }); + for (const target of [ + "darwin-arm64.node", + "darwin-x64.node", + "linux-arm64.node", + "linux-x64.node", + "linuxmusl-arm64.node", + "linuxmusl-x64.node", + "win32-arm64.node", + "win32-x64.node", + ]) { + fs.writeFileSync(path.join(bsqlDir, "prebuilds", target), `// ${target}`); + } + + const outDir = path.join(tmp, "standalone"); + fs.mkdirSync(outDir, { recursive: true }); + + // Act: copy native assets into the standalone output. + await syncStandaloneNativeAssets(projectRoot, fs.promises, { log() {} }, outDir); + + // Assert: the compiled build/Release/ binary was copied. + assert.ok( + fs.existsSync( + path.join(outDir, "node_modules", "better-sqlite3", "build", "Release", "better_sqlite3.node") + ), + "compiled native binary (build/Release/) must be in the standalone bundle" + ); + + // Assert: the prebuilds/ directory was also copied. + const prebuildsDir = path.join(outDir, "node_modules", "better-sqlite3", "prebuilds"); + assert.ok(fs.existsSync(prebuildsDir), "prebuilds/ directory must be in the standalone bundle"); + + // Assert: at least one prebuild file was copied. + assert.ok( + fs.existsSync(path.join(prebuildsDir, "linux-x64.node")), + "linux-x64 prebuild must be in the standalone bundle" + ); + + fs.rmSync(tmp, { recursive: true, force: true }); +}); \ No newline at end of file From 93ee4dce9f40e6631c19ac1b67bf8e3677edb275 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 13:52:04 -0300 Subject: [PATCH 10/21] fix(build): add build-next-isolated.mjs sibling imports to package.json files array (#9633) Refs: base-red #9737 --- changelog.d/fixes/9633-npm-build-files.md | 1 + package.json | 5 ++++- tests/unit/repro-9633.test.ts | 27 +++++++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixes/9633-npm-build-files.md create mode 100644 tests/unit/repro-9633.test.ts diff --git a/changelog.d/fixes/9633-npm-build-files.md b/changelog.d/fixes/9633-npm-build-files.md new file mode 100644 index 0000000000..c1dc5128da --- /dev/null +++ b/changelog.d/fixes/9633-npm-build-files.md @@ -0,0 +1 @@ +- fix(build): add build-next-isolated.mjs sibling imports to package.json files array diff --git a/package.json b/package.json index ba0b667569..b1430ec223 100644 --- a/package.json +++ b/package.json @@ -34,8 +34,11 @@ "scripts/dev/tls-options.mjs", "scripts/check/check-supported-node-runtime.ts", "scripts/dev/sync-env.mjs", - "scripts/build/native-binary-compat.mjs", + "scripts/build/assembleStandalone.mjs", + "scripts/build/backendOnlyPages.mjs", "scripts/build/build-next-isolated.mjs", + "scripts/build/build-tproxy-native.mjs", + "scripts/build/native-binary-compat.mjs", "scripts/build/runtime-env.mjs", "README.md", "LICENSE", diff --git a/tests/unit/repro-9633.test.ts b/tests/unit/repro-9633.test.ts new file mode 100644 index 0000000000..7acb750e5a --- /dev/null +++ b/tests/unit/repro-9633.test.ts @@ -0,0 +1,27 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; + +const pkg = JSON.parse(readFileSync("package.json", "utf-8")); +const files = pkg.files || []; + +// #9633: `build-next-isolated.mjs` is published (fixed in #1126), but three of +// its sibling modules it imports were missing from the `files` whitelist, so +// `npm run build` on a globally-installed package crashed with ERR_MODULE_NOT_FOUND. +// The dynamic import of `build-tproxy-native.mjs` (~line 308) and the static +// imports of `assembleStandalone.mjs` / `backendOnlyPages.mjs` must ship too. +const NEEDED = [ + "scripts/build/assembleStandalone.mjs", + "scripts/build/backendOnlyPages.mjs", + "scripts/build/build-tproxy-native.mjs", + "scripts/build/colocateOptionals.mjs", +]; + +test("#9633: build-next-isolated.mjs sibling imports present in package.json files[]", () => { + for (const needed of NEEDED) { + assert.ok( + files.some((f) => typeof f === "string" && f === needed), + `${needed} is not in package.json files[]` + ); + } +}); From c88b96244fcf7e6235db220c758f5af5cc90552d Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 13:52:11 -0300 Subject: [PATCH 11/21] fix(providers): classify 400 out of extra usage as quota_exhausted for Anthropic OAuth (#9486) Refs: base-red #9737 --- changelog.d/fixes/9486-claude-400-quota.md | 1 + open-sse/config/errorConfig.ts | 12 ++++ tests/unit/repro-9486.test.ts | 71 ++++++++++++++++++++++ 3 files changed, 84 insertions(+) create mode 100644 changelog.d/fixes/9486-claude-400-quota.md create mode 100644 tests/unit/repro-9486.test.ts diff --git a/changelog.d/fixes/9486-claude-400-quota.md b/changelog.d/fixes/9486-claude-400-quota.md new file mode 100644 index 0000000000..b410faf7d1 --- /dev/null +++ b/changelog.d/fixes/9486-claude-400-quota.md @@ -0,0 +1 @@ +- fix(providers): classify 400 out of extra usage as quota_exhausted for Anthropic OAuth diff --git a/open-sse/config/errorConfig.ts b/open-sse/config/errorConfig.ts index 8124c93f43..dcdf3bae9c 100644 --- a/open-sse/config/errorConfig.ts +++ b/open-sse/config/errorConfig.ts @@ -149,6 +149,18 @@ export const ERROR_RULES: ErrorRule[] = [ backoff: true, reason: "quota_exhausted", }, + { + id: "out_of_extra_usage", + text: "out of extra usage", + backoff: true, + reason: "quota_exhausted", + }, + { + id: "extra_usage_required", + text: "extra usage required", + backoff: true, + reason: "quota_exhausted", + }, { id: "capacity", text: "capacity", backoff: true, reason: "model_capacity" }, { id: "overloaded", text: "overloaded", backoff: true, reason: "model_capacity" }, { id: "high_demand", text: "high demand", backoff: true, reason: "model_capacity" }, diff --git a/tests/unit/repro-9486.test.ts b/tests/unit/repro-9486.test.ts new file mode 100644 index 0000000000..3df80de635 --- /dev/null +++ b/tests/unit/repro-9486.test.ts @@ -0,0 +1,71 @@ +/** + * Issue #9486 — Anthropic OAuth returns HTTP 400 with "out of extra usage" in + * the error body when a tool-carrying request exceeds the account's usage quota. + * This should be classified as quota_exhausted (not generic bad_request), so the + * account fallback mechanism applies a proper cooldown and combo routing can + * skip to another target. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { matchErrorRuleByText, findMatchingErrorRule, ERROR_RULES } = + await import("../../open-sse/config/errorConfig.ts"); +const { checkFallbackError, classifyErrorText } = + await import("../../open-sse/services/accountFallback.ts"); +const { RateLimitReason } = await import("../../open-sse/config/constants.ts"); + +test("#9486 ERROR_RULES has a text rule for 'out of extra usage' → quota_exhausted", () => { + const rule = ERROR_RULES.find((r) => r.text === "out of extra usage"); + assert.ok(rule, "expected a rule for 'out of extra usage'"); + assert.equal(rule!.reason, "quota_exhausted"); + // Should use backoff so the fallback path applies exponential scaling + assert.equal(rule!.backoff, true); +}); + +test("#9486 matchErrorRuleByText finds 'out of extra usage' rule", () => { + const rule = matchErrorRuleByText("out of extra usage"); + assert.ok(rule, "expected a matching rule"); + assert.equal(rule!.reason, "quota_exhausted"); +}); + +test("#9486 matchErrorRuleByText finds rule in a longer error message", () => { + const rule = matchErrorRuleByText( + "Error: 400 - out of extra usage. You have exceeded your usage quota for this billing period." + ); + assert.ok(rule, "expected a matching rule from longer message"); + assert.equal(rule!.reason, "quota_exhausted"); +}); + +test("#9486 findMatchingErrorRule with 400 + 'out of extra usage' returns quota_exhausted", () => { + const rule = findMatchingErrorRule(400, "out of extra usage"); + assert.ok(rule, "expected a matching rule"); + assert.equal(rule!.reason, "quota_exhausted"); +}); + +test("#9486 checkFallbackError returns quota_exhausted for 400 + 'out of extra usage'", () => { + const out = checkFallbackError(400, "out of extra usage", 0, null, "claude"); + assert.equal(out.shouldFallback, true); + assert.equal(out.reason, RateLimitReason.QUOTA_EXHAUSTED); + // Should get a non-zero cooldown (quota exhaustion is not transient) + assert.ok(out.cooldownMs > 0, `expected positive cooldown, got ${out.cooldownMs}ms`); +}); + +test("#9486 checkFallbackError handles 'Extra usage required' (same class)", () => { + // Anthropic sometimes returns "Extra usage required" instead of "out of extra usage" + const out = checkFallbackError(400, "Extra usage required", 0, null, "claude"); + assert.equal(out.shouldFallback, true); + assert.equal(out.reason, RateLimitReason.QUOTA_EXHAUSTED); +}); + +test("#9486 classifyErrorText flags 'out of extra usage' as QUOTA_EXHAUSTED", () => { + const out = classifyErrorText("out of extra usage"); + assert.equal(out, RateLimitReason.QUOTA_EXHAUSTED); +}); + +test("#9486 generic 400 without quota text still gets no fallback (regression guard)", () => { + // Regression guard: a plain 400 with no quota-related text must NOT trigger + // fallback, preserving the existing behavior for non-quota 400 errors. + const out = checkFallbackError(400, "Bad request: invalid JSON", 0, null, "claude"); + assert.equal(out.shouldFallback, false); + assert.equal(out.reason, RateLimitReason.UNKNOWN); +}); \ No newline at end of file From a90c5e5aba7c16ccd470176e7c3691d0b8463b01 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 13:52:17 -0300 Subject: [PATCH 12/21] fix(db): wire telemetry cleanup scheduler in Next.js startup path (#9624) Refs: base-red #9737 --- .../fixes/9624-telemetry-cleanup-wiring.md | 1 + src/instrumentation-node.ts | 13 +++++ tests/unit/repro-9624.test.ts | 52 +++++++++++++++++++ 3 files changed, 66 insertions(+) create mode 100644 changelog.d/fixes/9624-telemetry-cleanup-wiring.md create mode 100644 tests/unit/repro-9624.test.ts diff --git a/changelog.d/fixes/9624-telemetry-cleanup-wiring.md b/changelog.d/fixes/9624-telemetry-cleanup-wiring.md new file mode 100644 index 0000000000..01e389606e --- /dev/null +++ b/changelog.d/fixes/9624-telemetry-cleanup-wiring.md @@ -0,0 +1 @@ +- fix(db): wire telemetry cleanup scheduler in Next.js startup path (#9624) diff --git a/src/instrumentation-node.ts b/src/instrumentation-node.ts index d0407e437a..7be56fb919 100755 --- a/src/instrumentation-node.ts +++ b/src/instrumentation-node.ts @@ -306,6 +306,7 @@ export async function registerNodejs(): Promise { { applyRuntimeSettings }, { startRuntimeConfigHotReload }, { startSpendBatchWriter }, + { startCleanupScheduler }, { registerDefaultGuardrails }, { ensurePersistentManagementPasswordHash }, { skillExecutor }, @@ -320,6 +321,7 @@ export async function registerNodejs(): Promise { import("@/lib/config/runtimeSettings"), import("@/lib/config/hotReload"), import("@/lib/spend/batchWriter"), + import("@/lib/db/cleanup"), import("@/lib/guardrails"), import("@/lib/auth/managementPassword"), import("@/lib/skills/executor"), @@ -489,6 +491,17 @@ export async function registerNodejs(): Promise { console.warn("[STARTUP] Could not initialize vacuum scheduler (non-fatal):", msg); } + // Retention cleanup scheduler (#4691/#6988, #9624): runs the general retention + // cleanup once after startup and then every 6 hours. Previously this was only + // wired into the unused src/server-init.ts, so telemetry tables grew unboundedly + // even with retention.autoCleanupEnabled=true. Idempotent (guarded internally). + try { + startCleanupScheduler(); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + console.warn("[STARTUP] Could not start cleanup scheduler (non-fatal):", msg); + } + // Warm the model catalog's durable, apiKey-independent sub-caches at // startup — see warmModelCatalogCache() for why the top-level Response // cache alone doesn't deliver this. Fire-and-forget, non-fatal. diff --git a/tests/unit/repro-9624.test.ts b/tests/unit/repro-9624.test.ts new file mode 100644 index 0000000000..a73e83eddf --- /dev/null +++ b/tests/unit/repro-9624.test.ts @@ -0,0 +1,52 @@ +import { describe, it } from "node:test"; +import { strict as assert } from "node:assert"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, resolve } from "node:path"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); + +const INSTRUMENTATION_NODE_PATH = resolve( + __dirname, + "../../src/instrumentation-node.ts" +); + +describe("repro-9624: startCleanupScheduler wired in Next.js startup path", () => { + it("should import startCleanupScheduler from cleanup", () => { + const source = readFileSync(INSTRUMENTATION_NODE_PATH, "utf-8"); + + // instrumentation-node.ts loads all startup modules via dynamic imports in a + // Promise.all destructure, e.g.: + // const [{ startCleanupScheduler }, ...] = await Promise.all([ + // import("@/lib/db/cleanup"), ... + // ]); + // So the binding and the module import appear separately in the file. + const cleanupModuleImported = /import\(\s*["']@\/lib\/db\/cleanup["']\s*\)/.test( + source + ); + const schedulerBound = /\bstartCleanupScheduler\b/.test(source); + + assert.ok( + cleanupModuleImported, + "@/lib/db/cleanup should be imported (dynamic import) in instrumentation-node.ts" + ); + assert.ok( + schedulerBound, + "startCleanupScheduler should be bound in instrumentation-node.ts" + ); + }); + + it("should call startCleanupScheduler() during startup", () => { + const source = readFileSync(INSTRUMENTATION_NODE_PATH, "utf-8"); + + // Check that startCleanupScheduler is called (as a function call). + // It can be called directly or as part of a conditional. + const hasCall = /\bstartCleanupScheduler\s*\(/.test(source); + + assert.ok( + hasCall, + "startCleanupScheduler() should be called in instrumentation-node.ts" + ); + }); +}); \ No newline at end of file From df1ea5bd77fe8e128caf46f8342277ac5f67ba8b Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 13:52:23 -0300 Subject: [PATCH 13/21] fix(db): align domain_cost_history cleanup cutoff with millisecond column (#9625) Refs: base-red #9737 --- changelog.d/fixes/9625-domain-cost-ms.md | 1 + src/lib/db/cleanup.ts | 5 +- tests/unit/repro-9625.test.ts | 92 ++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 changelog.d/fixes/9625-domain-cost-ms.md create mode 100644 tests/unit/repro-9625.test.ts diff --git a/changelog.d/fixes/9625-domain-cost-ms.md b/changelog.d/fixes/9625-domain-cost-ms.md new file mode 100644 index 0000000000..37c3e1e7e6 --- /dev/null +++ b/changelog.d/fixes/9625-domain-cost-ms.md @@ -0,0 +1 @@ +- fix(db): align domain_cost_history cleanup cutoff with millisecond column (#9625) diff --git a/src/lib/db/cleanup.ts b/src/lib/db/cleanup.ts index 20d0358ce4..94d5995afe 100644 --- a/src/lib/db/cleanup.ts +++ b/src/lib/db/cleanup.ts @@ -253,14 +253,15 @@ export async function cleanupMemoryEntries(): Promise { /** * Clean up old domain_cost_history based on retention settings. (#6848) - * Uses unix-epoch `timestamp` column (INTEGER). + * The `timestamp` column stores epoch milliseconds (saveCostEntry default + * is Date.now()), so the cutoff must be in milliseconds to match. (#9625) */ export async function cleanupDomainCostHistory(): Promise { const db = getDbInstance(); const retention = getRetentionSettings(); const retentionDays = retention.domainCostHistory; - const cutoffEpoch = Math.floor(Date.now() / 1000) - retentionDays * 86_400; + const cutoffEpoch = Date.now() - retentionDays * 86_400_000; const result: CleanupResult = { deleted: 0, errors: 0 }; diff --git a/tests/unit/repro-9625.test.ts b/tests/unit/repro-9625.test.ts new file mode 100644 index 0000000000..f6ff2f68cf --- /dev/null +++ b/tests/unit/repro-9625.test.ts @@ -0,0 +1,92 @@ +/** + * Issue #9625 — domain_cost_history cleanup cutoff unit mismatch. + * + * cleanupDomainCostHistory() computes the cutoff in epoch seconds + * (Math.floor(Date.now() / 1000)) but the timestamp column stores + * epoch milliseconds (Date.now()), as inserted by saveCostEntry(). + * + * This test seeds data using the same format as the production code + * (milliseconds), then asserts that cleanupDomainCostHistory() correctly + * deletes rows older than the retention window. + * + * Before the fix, the cutoff in seconds was ~1000× smaller than the + * stored timestamps, so the DELETE WHERE timestamp < cutoff would + * never match old rows — the cleanup was effectively a no-op. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-9625-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const { cleanupDomainCostHistory } = await import("../../src/lib/db/cleanup.ts"); +const { getDbInstance, resetDbInstance } = await import("../../src/lib/db/core.ts"); + +test.after(() => { + resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +const DAY_MS = 86_400_000; // milliseconds + +test("#9625 cleanupDomainCostHistory: cutoff in ms matches production timestamps", async () => { + const db = getDbInstance()!; + const now = Date.now(); // milliseconds — same as saveCostEntry() default + + const insert = db.prepare( + "INSERT INTO domain_cost_history (api_key_id, cost, timestamp) VALUES (?, ?, ?)" + ); + + // Seed data using millisecond timestamps (production format). + // 3 old rows: 40 days ago (should be deleted) + // 2 recent rows: 5 days ago (should be kept) + insert.run("key1", 1.0, now - 40 * DAY_MS); + insert.run("key1", 2.0, now - 40 * DAY_MS); + insert.run("key1", 3.0, now - 40 * DAY_MS); + insert.run("key1", 4.0, now - 5 * DAY_MS); + insert.run("key1", 5.0, now - 5 * DAY_MS); + + const result = await cleanupDomainCostHistory(); + + // Before the fix, cutoff was in seconds (~1.7e9) while timestamps + // are in milliseconds (~1.7e12). The comparison `WHERE ts < 1.7e9` + // would never match rows with ts ~1.7e12, so nothing was deleted. + assert.strictEqual(result.deleted, 3, "Should delete 3 old rows (40 days old)"); + assert.strictEqual(result.errors, 0); + + const remaining = db.prepare("SELECT COUNT(*) as cnt FROM domain_cost_history").get() as { + cnt: number; + }; + assert.strictEqual(remaining.cnt, 2, "Should keep 2 recent rows (5 days old)"); +}); + +test("#9625 unit mismatch: seconds cutoff would NOT match ms timestamps", () => { + // Demonstrate the arithmetic bug: a cutoff in seconds is ~1000× + // smaller than a millisecond timestamp, so the WHERE clause never + // matches production data. + const nowMs = Date.now(); + const nowSec = Math.floor(nowMs / 1000); + const retentionDays = 30; + const cutoffSec = nowSec - retentionDays * 86_400; // seconds + const cutoffMs = nowMs - retentionDays * 86_400_000; // milliseconds + + // A row inserted 40 days ago with a millisecond timestamp: + const oldRowMs = nowMs - 40 * 86_400_000; // ~1.7e12 + + // With seconds cutoff: oldRowMs (1.7e12) < cutoffSec (1.7e9) is FALSE + // because 1.7e12 > 1.7e9 — the row is never matched. + assert.ok( + oldRowMs > cutoffSec, + "Bug: ms timestamp is NOT less than seconds cutoff, so row is never deleted" + ); + + // With milliseconds cutoff: oldRowMs (1.7e12) < cutoffMs (1.7e12) is TRUE + assert.ok( + oldRowMs < cutoffMs, + "Fix: ms timestamp IS less than ms cutoff, so row is correctly deleted" + ); +}); \ No newline at end of file From aefa2b665bca3ea515ddfdb34b5eddccd2744600 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 8 Aug 2026 13:52:30 -0300 Subject: [PATCH 14/21] fix(playground): surface provider model loading errors in LlmChatCard (#9626) Refs: base-red #9737 --- changelog.d/fixes/9626-playground-errors.md | 1 + .../components/LlmChatCard.tsx | 22 ++++++- .../providers/hooks/useProviderModels.ts | 45 ++++++++++---- tests/unit/repro-9626.test.ts | 62 +++++++++++++++++++ 4 files changed, 116 insertions(+), 14 deletions(-) create mode 100644 changelog.d/fixes/9626-playground-errors.md create mode 100644 tests/unit/repro-9626.test.ts diff --git a/changelog.d/fixes/9626-playground-errors.md b/changelog.d/fixes/9626-playground-errors.md new file mode 100644 index 0000000000..3bf1ee0343 --- /dev/null +++ b/changelog.d/fixes/9626-playground-errors.md @@ -0,0 +1 @@ +- fix(playground): surface provider model loading errors and offer retry (#9626) diff --git a/src/app/(dashboard)/dashboard/media-providers/components/LlmChatCard.tsx b/src/app/(dashboard)/dashboard/media-providers/components/LlmChatCard.tsx index 27a900c541..53700db68a 100644 --- a/src/app/(dashboard)/dashboard/media-providers/components/LlmChatCard.tsx +++ b/src/app/(dashboard)/dashboard/media-providers/components/LlmChatCard.tsx @@ -134,7 +134,7 @@ export function LlmChatCard({ }: Props) { const t = useTranslations("miniPlayground"); const { keys } = useApiKey(); - const { models } = useProviderModels(providerId); + const { models, loading, error, retry } = useProviderModels(providerId); const [internalSelectedKey, setInternalSelectedKey] = useState(""); const [internalModel, setInternalModel] = useState(initialModel ?? ""); @@ -392,15 +392,31 @@ export function LlmChatCard({ + {error && ( + + + {String(error)} + + + + )} {/* Key select */} {keys.length > 0 && ( diff --git a/src/app/(dashboard)/dashboard/providers/hooks/useProviderModels.ts b/src/app/(dashboard)/dashboard/providers/hooks/useProviderModels.ts index b2b2ec62d4..c3997f813d 100644 --- a/src/app/(dashboard)/dashboard/providers/hooks/useProviderModels.ts +++ b/src/app/(dashboard)/dashboard/providers/hooks/useProviderModels.ts @@ -1,6 +1,6 @@ "use client"; -import { useState, useEffect } from "react"; +import { useState, useEffect, useCallback, useRef } from "react"; export interface ProviderModel { id: string; @@ -18,6 +18,8 @@ interface UseProviderModelsResult { models: ProviderModel[]; loading: boolean; error: string | null; + /** Re-runs the model fetch for the current provider. Useful for a Retry action. */ + retry: () => void; } /** @@ -32,15 +34,14 @@ export function useProviderModels(providerId: string): UseProviderModelsResult { const [models, setModels] = useState([]); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); + // Cancels any in-flight load (component unmount or a retry superseding the + // previous request) so a stale response never overwrites a newer one. + const cleanupRef = useRef<(() => void) | null>(null); - useEffect(() => { - if (!providerId) { - setLoading(false); - return; - } - + const load = useCallback(() => { + cleanupRef.current?.(); let cancelled = false; - const load = async () => { + const run = async () => { setLoading(true); setError(null); try { @@ -109,11 +110,33 @@ export function useProviderModels(providerId: string): UseProviderModelsResult { if (!cancelled) setLoading(false); } }; - void load(); - return () => { + void run(); + const cleanup = () => { cancelled = true; }; + cleanupRef.current = cleanup; + return cleanup; }, [providerId]); - return { models, loading, error }; + useEffect(() => { + if (!providerId) { + setLoading(false); + return; + } + return load(); + }, [providerId, load]); + + // Release the current in-flight cleanup on unmount so no state updates leak. + useEffect(() => { + return () => { + cleanupRef.current?.(); + }; + }, []); + + const retry = useCallback(() => { + if (!providerId) return; + load(); + }, [providerId, load]); + + return { models, loading, error, retry }; } diff --git a/tests/unit/repro-9626.test.ts b/tests/unit/repro-9626.test.ts new file mode 100644 index 0000000000..022bca6771 --- /dev/null +++ b/tests/unit/repro-9626.test.ts @@ -0,0 +1,62 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +const root = join(import.meta.dirname, "../.."); +const llmChatCardPath = + "src/app/(dashboard)/dashboard/media-providers/components/LlmChatCard.tsx"; +const src = readFileSync(join(root, llmChatCardPath), "utf8"); + +const DISABLED_ON_LOADING = /disabled\s*=\s*\{\s*loading\s*\}/; +const MODELS_LOADING_MARKER = /modelsLoading|Loading…|Loading\.\.\./; +const ERROR_BRANCH = /error\s*&&/; +const RETRY_ACTION = /onClick\s*=\s*\{[^}]*retry|retry[A-Za-z]*\s*\(\)|const\s+\[reload/i; +const NO_MODELS_AFTER_EMPTY = /modelOptions\.length\s*===?\s*0|models\.length\s*===?\s*0/; + +test("LlmChatCard destructures loading and error from useProviderModels (#9626)", () => { + const match = src.match(/const\s*\{\s*([^}]+)\s*\}\s*=\s*useProviderModels\(/); + assert.ok(match, "Expected to find a destructuring of useProviderModels"); + + const destructured = match[1]; + assert.ok( + destructured.includes("loading"), + "loading state must be destructured from useProviderModels" + ); + assert.ok(destructured.includes("error"), "error state must be destructured from useProviderModels"); +}); + +test("LlmChatCard disables the model selector while models are loading (#9626)", () => { + assert.ok( + DISABLED_ON_LOADING.test(src), + "The model