From 8deea62daff36367dbc9bf422ec36da99f516018 Mon Sep 17 00:00:00 2001 From: initguru Date: Thu, 17 Sep 2026 14:31:19 +0900 Subject: [PATCH] fix(sse): guard empty tool_calls[] and strip tool_choice without tools (#12901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related schema-compliance fixes for upstreams that enforce the OpenAI spec strictly (vLLM self-hosted, Kimi-K2.6): 1. Empty tool_calls[] guard: providers like Kimi-K2.6 attach an empty tool_calls:[] array to every content delta when tools are defined. An empty array is truthy, so guarding on 'delta.tool_calls' alone called closeMessage() after the first content delta, closing the message item prematurely. Subsequent content deltas arrived on a done item and were dropped by clients (Codex: 'OutputTextDelta without active item'), leaving only the first text fragment in the output. Guard both translation paths on 'delta.tool_calls?.length' so closeMessage runs only when at least one actual tool call is present: - open-sse/translator/response/openai-responses.ts (chatCore translate path) - open-sse/transformer/responsesTransformer.ts (/v1/responses direct path) 2. tool_choice schema guard: auxiliary/internal calls (e.g. WebSearch) legitimately send tool_choice:'auto' with no tools array, and routing may drop the tools array after the client sent it. vLLM rejects this combination with a schema 400 ('When using tool_choice, tools must be set'). Add stripToolChoiceWithoutTools() to targetRequestSanitizer.ts that removes a dangling tool_choice lacking a usable tools array at the common dispatch boundary. The guard is OpenAI-spec compliance, not provider-specific, so it fires regardless of provider. TDD: tests reproduce both bugs (Red: 5 fail → Green: 31 pass, 0 fail): - tests/unit/translator-openai-responses-empty-tool-calls.test.ts (2 tests) - tests/unit/responses-transformer.test.ts (+2 tests, 21 total pass) - tests/unit/tool-choice-schema-normalization.test.ts (8 tests) typecheck:core: 0 errors Co-authored-by: Jihyun Son --- open-sse/services/targetRequestSanitizer.ts | 35 ++++- open-sse/transformer/responsesTransformer.ts | 2 +- .../translator/response/openai-responses.ts | 2 +- tests/unit/responses-transformer.test.ts | 61 ++++++++ .../tool-choice-schema-normalization.test.ts | 138 +++++++++++++++++ ...-openai-responses-empty-tool-calls.test.ts | 146 ++++++++++++++++++ 6 files changed, 380 insertions(+), 4 deletions(-) create mode 100644 tests/unit/tool-choice-schema-normalization.test.ts create mode 100644 tests/unit/translator-openai-responses-empty-tool-calls.test.ts diff --git a/open-sse/services/targetRequestSanitizer.ts b/open-sse/services/targetRequestSanitizer.ts index 291b3051f5..01815eb21d 100644 --- a/open-sse/services/targetRequestSanitizer.ts +++ b/open-sse/services/targetRequestSanitizer.ts @@ -48,6 +48,30 @@ function stripVerbosityForTarget(body: JsonRecord, model: string): string[] { return stripped; } +/** + * Strip a `tool_choice` control that lacks a usable `tools` array. + * + * Some upstreams (vLLM self-hosted) reject the combination "tool_choice set + * without tools" with a schema 400 — "When using tool_choice, tools must be + * set." Auxiliary/internal calls (e.g. WebSearch) legitimately send + * `tool_choice:"auto"` with no tools array, and routing/fallback may have + * dropped the tools array after the client sent it. This guard removes the + * dangling `tool_choice` so the request stays schema-valid for any upstream + * that enforces the OpenAI spec. It is OpenAI-spec compliance, not + * provider-specific, so it fires regardless of provider. + */ +function stripToolChoiceWithoutTools(body: JsonRecord): boolean { + if (!Object.hasOwn(body, "tool_choice")) return false; + const tc = body.tool_choice; + // A falsy/null tool_choice carries no "use tools" intent — leave as-is. + if (!tc) return false; + const tools = body.tools; + const hasUsableTools = Array.isArray(tools) && tools.length > 0; + if (hasUsableTools) return false; + delete body.tool_choice; + return true; +} + /** * Sanitize a translated request using the concrete provider/model selected by * routing. Returns a fresh top-level object and never mutates the caller body. @@ -79,10 +103,17 @@ export function sanitizeRequestForResolvedTarget( // boundary so custom executors cannot accidentally bypass them. stripUnsupportedParams(options.provider, options.model, next); - if (stripped.length > 0) { + // Strip a dangling tool_choice that lacks a usable tools array — upstreams + // that enforce the OpenAI spec (vLLM) reject "tool_choice without tools" + // with a schema 400. + const strippedToolChoice = stripToolChoiceWithoutTools(next); + + if (stripped.length > 0 || strippedToolChoice) { + const parts = [...stripped]; + if (strippedToolChoice) parts.push("tool_choice (no tools)"); options.log?.debug?.( "TARGET_PARAMS", - `Stripped ${stripped.join(", ")} for resolved target ${options.provider || "unknown"}/${options.model}` + `Stripped ${parts.join(", ")} for resolved target ${options.provider || "unknown"}/${options.model}` ); } diff --git a/open-sse/transformer/responsesTransformer.ts b/open-sse/transformer/responsesTransformer.ts index f22f38f9ed..8b9b497464 100644 --- a/open-sse/transformer/responsesTransformer.ts +++ b/open-sse/transformer/responsesTransformer.ts @@ -795,7 +795,7 @@ export function createResponsesApiTransformStream( } // Handle tool_calls - if (delta.tool_calls) { + if (delta.tool_calls?.length) { // Close reasoning first so tool calls do not collide with an // open reasoning item, then close the message at its real index. if (state.reasoningId && !state.reasoningDone) { diff --git a/open-sse/translator/response/openai-responses.ts b/open-sse/translator/response/openai-responses.ts index 29b9a937d7..cd1310bffd 100644 --- a/open-sse/translator/response/openai-responses.ts +++ b/open-sse/translator/response/openai-responses.ts @@ -288,7 +288,7 @@ export function openaiToOpenAIResponsesResponse(chunk, state) { } // Handle tool_calls - if (delta.tool_calls) { + if (delta.tool_calls?.length) { // Close reasoning first so tool calls do not collide with an open // reasoning item, then close the message at its real index. if (state.reasoningId && !state.reasoningDone) { diff --git a/tests/unit/responses-transformer.test.ts b/tests/unit/responses-transformer.test.ts index 6a29e84e54..b3ae941a02 100644 --- a/tests/unit/responses-transformer.test.ts +++ b/tests/unit/responses-transformer.test.ts @@ -547,3 +547,64 @@ test("createResponsesApiTransformStream keepalive self-clears when enqueue fails globalThis.clearInterval = realClearInterval; } }); + +// Regression: providers (e.g. Kimi-K2.6) emit content deltas that carry an empty +// `tool_calls:[]` array in the SAME chunk when tools are defined. The empty array is +// truthy, so the old `if (delta.tool_calls)` guard entered the tool-call branch and +// called closeMessage() immediately — closing the message item after only the first +// content delta. Subsequent content deltas arrived on a done item, and Codex +// (which clears `active_item` on `output_item.done`) dropped them with +// "OutputTextDelta without active item", producing a one-character response. +// The guard must ignore an empty tool_calls array so the message stays open. +test("createResponsesApiTransformStream does not close the message on an empty tool_calls array paired with content (Kimi-K2.6 pattern)", async () => { + const output = await runTransformStream([ + 'data: {"id":"chatcmpl_1","choices":[{"index":0,"delta":{"content":"H","tool_calls":[]}}]}\n\n', + 'data: {"choices":[{"index":0,"delta":{"content":"ello","tool_calls":[]}}]}\n\n', + 'data: {"choices":[{"index":0,"delta":{"content":" world","tool_calls":[]}}]}\n\n', + 'data: {"choices":[{"index":0,"delta":{},"finish_reason":"stop"}],"usage":{"prompt_tokens":1,"completion_tokens":2,"total_tokens":3}}\n\n', + ]); + + const events = parseSseOutput(output); + const textDeltas = events + .filter((event) => event.event === "response.output_text.delta") + .map((event) => JSON.parse(event.data).delta); + const completed = JSON.parse( + events.find((event) => event.event === "response.completed").data + ).response; + + // All three content deltas must be emitted — not just the first one. + assert.deepEqual(textDeltas, ["H", "ello", " world"]); + // Exactly ONE assistant message item, carrying the full concatenated text. + const messageItems = completed.output.filter((item) => item.type === "message"); + assert.equal(messageItems.length, 1, "empty tool_calls must not split/close the message"); + assert.equal(messageItems[0].content[0].text, "Hello world"); + // No function_call items should be synthesized from the empty arrays. + const functionCallItems = completed.output.filter((item) => item.type === "function_call"); + assert.deepEqual(functionCallItems, []); +}); + +// The same fix must not regress the real tool-call path: when tool_calls carries an +// actual entry, the preceding content message must still close so the tool call is its +// own output item. +test("createResponsesApiTransformStream still closes the message and emits a real tool call when tool_calls is non-empty", async () => { + const output = await runTransformStream([ + 'data: {"id":"chatcmpl_1","choices":[{"index":0,"delta":{"content":"let me search","tool_calls":[]}}]}\n\n', + 'data: {"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"id":"call_1","function":{"name":"search","arguments":"{\\"q\\":\\"hi\\"}"}}]}}]}\n\n', + 'data: {"choices":[{"index":0,"delta":{},"finish_reason":"tool_calls"}],"usage":{"prompt_tokens":1,"completion_tokens":2,"total_tokens":3}}\n\n', + ]); + + const events = parseSseOutput(output); + const completed = JSON.parse( + events.find((event) => event.event === "response.completed").data + ).response; + + const messageItems = completed.output.filter((item) => item.type === "message"); + const functionCallItems = completed.output.filter((item) => item.type === "function_call"); + + // The text message closed with its full content, and the tool call is a separate item. + assert.equal(messageItems.length, 1); + assert.equal(messageItems[0].content[0].text, "let me search"); + assert.equal(functionCallItems.length, 1); + assert.equal(functionCallItems[0].call_id, "call_1"); + assert.equal(functionCallItems[0].arguments, '{"q":"hi"}'); +}); diff --git a/tests/unit/tool-choice-schema-normalization.test.ts b/tests/unit/tool-choice-schema-normalization.test.ts new file mode 100644 index 0000000000..ff62bf12dc --- /dev/null +++ b/tests/unit/tool-choice-schema-normalization.test.ts @@ -0,0 +1,138 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { sanitizeRequestForResolvedTarget } = + await import("../../open-sse/services/targetRequestSanitizer.ts"); + +const baseOpts = { provider: "skhynix", model: "DeepSeek-V4-Flash-0731" } as const; + +test("tool_choice schema normalization: strips tool_choice when tools absent (vLLM 400 guard)", () => { + // WebSearch-style auxiliary call: tool_choice:"auto" with NO tools array. + // vLLM (Hosted_vllmException) rejects this: "When using tool_choice, tools must be set." + const body = { + model: "DeepSeek-V4-Flash-0731", + messages: [{ role: "user", content: "search the web" }], + tool_choice: "auto", + } as Record; + + const out = sanitizeRequestForResolvedTarget(body, baseOpts); + + assert.equal( + Object.prototype.hasOwnProperty.call(out, "tool_choice"), + false, + "tool_choice must be removed when tools is absent" + ); + assert.equal( + Object.prototype.hasOwnProperty.call(out, "tools"), + false, + "tools key should not be introduced" + ); +}); + +test("tool_choice schema normalization: strips tool_choice when tools is empty array", () => { + const body = { + model: "DeepSeek-V4-Flash-0731", + messages: [{ role: "user", content: "hi" }], + tools: [], + tool_choice: "auto", + } as Record; + + const out = sanitizeRequestForResolvedTarget(body, baseOpts); + + assert.equal( + Object.prototype.hasOwnProperty.call(out, "tool_choice"), + false, + "tool_choice must be removed when tools is an empty array" + ); + // empty tools array itself can stay — only tool_choice is the schema violation +}); + +test("tool_choice schema normalization: preserves tool_choice when tools present", () => { + const body = { + model: "DeepSeek-V4-Flash-0731", + messages: [{ role: "user", content: "use a tool" }], + tools: [{ type: "function", function: { name: "get_weather", parameters: {} } }], + tool_choice: "auto", + } as Record; + + const out = sanitizeRequestForResolvedTarget(body, baseOpts); + + assert.equal(out.tool_choice, "auto", "tool_choice must be preserved when tools present"); + assert.equal(Array.isArray(out.tools), true, "tools array must be preserved"); + assert.equal((out.tools as unknown[]).length, 1); +}); + +test("tool_choice schema normalization: preserves object tool_choice with tools", () => { + const body = { + model: "DeepSeek-V4-Flash-0731", + messages: [{ role: "user", content: "x" }], + tools: [{ type: "function", function: { name: "fn", parameters: {} } }], + tool_choice: { type: "function", function: { name: "fn" } }, + } as Record; + + const out = sanitizeRequestForResolvedTarget(body, baseOpts); + + assert.ok(typeof out.tool_choice === "object", "object tool_choice preserved when tools present"); +}); + +test("tool_choice schema normalization: no-op when tool_choice absent", () => { + const body = { + model: "DeepSeek-V4-Flash-0731", + messages: [{ role: "user", content: "hi" }], + } as Record; + + const out = sanitizeRequestForResolvedTarget(body, baseOpts); + + assert.equal( + Object.prototype.hasOwnProperty.call(out, "tool_choice"), + false, + "no tool_choice key introduced when absent" + ); +}); + +test("tool_choice schema normalization: no-op for null tool_choice without tools", () => { + const body = { + model: "DeepSeek-V4-Flash-0731", + messages: [{ role: "user", content: "hi" }], + tool_choice: null, + } as Record; + + const out = sanitizeRequestForResolvedTarget(body, baseOpts); + + // null tool_choice is falsy and carries no "use tools" intent; leave as-is + // (the guard only strips a DEFINED, truthy tool_choice lacking a tools array) + assert.equal(out.tool_choice, null); +}); + +test("tool_choice schema normalization: applies regardless of provider (global schema guard)", () => { + // The guard is OpenAI-spec compliance, not provider-specific — it must fire + // for any provider whose upstream enforces "tool_choice requires tools". + for (const provider of ["skhynix", "openai", "nvidia", "deepseek"]) { + const body = { + model: "any-model", + messages: [{ role: "user", content: "x" }], + tool_choice: "required", + } as Record; + + const out = sanitizeRequestForResolvedTarget(body, { provider, model: "any-model" }); + + assert.equal( + Object.prototype.hasOwnProperty.call(out, "tool_choice"), + false, + `tool_choice must be stripped for provider=${provider} when tools absent` + ); + } +}); + +test("tool_choice schema normalization: does not mutate caller body", () => { + const body = { + model: "DeepSeek-V4-Flash-0731", + messages: [{ role: "user", content: "x" }], + tool_choice: "auto", + } as Record; + + sanitizeRequestForResolvedTarget(body, baseOpts); + + // the function returns a fresh object and must not mutate the caller's body + assert.equal(body.tool_choice, "auto", "caller body must not be mutated"); +}); diff --git a/tests/unit/translator-openai-responses-empty-tool-calls.test.ts b/tests/unit/translator-openai-responses-empty-tool-calls.test.ts new file mode 100644 index 0000000000..1916d23d2e --- /dev/null +++ b/tests/unit/translator-openai-responses-empty-tool-calls.test.ts @@ -0,0 +1,146 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { openaiToOpenAIResponsesResponse } = + await import("../../open-sse/translator/response/openai-responses.ts"); +const { initState } = await import("../../open-sse/translator/index.ts"); +const { FORMATS } = await import("../../open-sse/translator/formats.ts"); + +/** + * Reproduces the Kimi-K2.6 empty-tool_calls bug. + * + * Kimi-K2.6 attaches an EMPTY `tool_calls:[]` array to every content delta when + * tools are defined. An empty array is truthy, so `if (delta.tool_calls)` is + * true even when no actual tool call is present. The translator then calls + * closeMessage() right after the first content delta, closing the message item + * with only the first fragment of text. Subsequent content deltas arrive on a + * done item and are emitted as orphan `output_text.delta` events (no + * matching active item) — Codex CLI panics with "OutputTextDelta without + * active item" and only the first fragment reaches the final output. + * + * Expected (after fix): both content fragments accumulate into ONE message + * item, `output_text.done` carries the full text, and the final + * `response.completed.response.output` message contains the full text. + */ +function collectEvents(chunks) { + const state = initState(FORMATS.OPENAI_RESPONSES); + const events = []; + for (const chunk of chunks) { + const result = openaiToOpenAIResponsesResponse(chunk, state); + if (result) events.push(...result); + } + return events; +} + +test("Kimi-K2.6: empty tool_calls:[] on content deltas must NOT close the message item", () => { + const events = collectEvents([ + // First content delta with an EMPTY tool_calls array attached (Kimi pattern) + { + id: "chatcmpl-kimi", + model: "Kimi-K2.6", + choices: [ + { + index: 0, + delta: { content: " The", tool_calls: [] }, + finish_reason: null, + }, + ], + }, + // Second content delta — also carries an empty tool_calls array + { + id: "chatcmpl-kimi", + model: "Kimi-K2.6", + choices: [ + { + index: 0, + delta: { content: " fix works.", tool_calls: [] }, + finish_reason: null, + }, + ], + }, + // Final chunk: finish_reason (no content) + { + id: "chatcmpl-kimi", + model: "Kimi-K2.6", + choices: [{ index: 0, delta: {}, finish_reason: "stop" }], + usage: { prompt_tokens: 10, completion_tokens: 5, total_tokens: 15 }, + }, + ]); + + const textDeltas = events + .filter((e) => e.event === "response.output_text.delta") + .map((e) => e.data.delta); + const textDone = events + .filter((e) => e.event === "response.output_text.done") + .map((e) => e.data.text); + + // Both content fragments must be emitted as deltas on the SAME item + assert.deepEqual(textDeltas, [" The", " fix works."]); + + // Exactly one output_text.done carrying the FULL accumulated text + assert.equal(textDone.length, 1, "must emit exactly one output_text.done"); + assert.equal(textDone[0], " The fix works.", "output_text.done must carry full text"); + + // The final completed response output must contain the full text + const completed = events.find((e) => e.event === "response.completed"); + assert.ok(completed, "must emit response.completed"); + const msgItems = completed.data.response.output.filter((o) => o.type === "message"); + assert.equal(msgItems.length, 1, "must have exactly one message item"); + assert.equal( + msgItems[0].content[0].text, + " The fix works.", + "final output message must contain the full text" + ); +}); + +test("Kimi-K2.6: a REAL tool_call (non-empty) still closes the message before the call", () => { + const events = collectEvents([ + { + id: "chatcmpl-kimi2", + model: "Kimi-K2.6", + choices: [ + { + index: 0, + delta: { content: "thinking", tool_calls: [] }, + finish_reason: null, + }, + ], + }, + { + id: "chatcmpl-kimi2", + model: "Kimi-K2.6", + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { + index: 0, + id: "call_1", + function: { name: "get_weather", arguments: '{"city":"NYC"}' }, + }, + ], + }, + finish_reason: null, + }, + ], + }, + { + id: "chatcmpl-kimi2", + model: "Kimi-K2.6", + choices: [{ index: 0, delta: {}, finish_reason: "tool_calls" }], + usage: { prompt_tokens: 10, completion_tokens: 5, total_tokens: 15 }, + }, + ]); + + // The content "thinking" must be in the final output as a message + const completed = events.find((e) => e.event === "response.completed"); + const msgItems = completed.data.response.output.filter((o) => o.type === "message"); + assert.equal(msgItems.length, 1); + assert.equal(msgItems[0].content[0].text, "thinking"); + // And a function_call item must exist + const fnItems = completed.data.response.output.filter( + (o) => o.type === "function_call" || o.type === "custom_tool_call" + ); + assert.ok(fnItems.length >= 1, "must have a function/custom tool call item"); +});