diff --git a/src/adapters/openai-chat.ts b/src/adapters/openai-chat.ts index 99b62af5e5..b79d73fa9c 100644 --- a/src/adapters/openai-chat.ts +++ b/src/adapters/openai-chat.ts @@ -205,6 +205,98 @@ function isRecord(value: unknown): value is Record { return value !== null && typeof value === "object" && !Array.isArray(value); } +type InvalidToolCallReason = + | "tool_calls_not_array" + | "tool_call_not_object" + | "tool_call_id_invalid" + | "tool_call_function_not_object" + | "tool_call_function_name_invalid" + | "tool_call_function_name_blank" + | "tool_call_function_arguments_invalid"; + +/** + * Explain only the rejected wire shape, never its values. This diagnostic exists so provider + * compatibility can be tightened from evidence without retaining tool arguments or credentials. + */ +function diagnoseInvalidToolCalls( + rawToolCalls: unknown, + mode: "stream" | "response", +): { reason: InvalidToolCallReason; callIndex?: number; valueType: string } | undefined { + if (!Array.isArray(rawToolCalls)) { + return { reason: "tool_calls_not_array", valueType: rawToolCalls === null ? "null" : typeof rawToolCalls }; + } + for (let callIndex = 0; callIndex < rawToolCalls.length; callIndex++) { + const rawToolCall = rawToolCalls[callIndex]; + if (!isRecord(rawToolCall)) { + return { + reason: "tool_call_not_object", + callIndex, + valueType: rawToolCall === null ? "null" : Array.isArray(rawToolCall) ? "array" : typeof rawToolCall, + }; + } + if (mode === "stream") { + // The streamed path validates the pieces it is about to store (#1531): a present + // `function` must be a record, and a present `name`/`arguments`/`id` must be a string. + // Blank names are caught later at flush, not here, so they are not diagnosed on this + // branch. Describe exactly that boundary rather than tightening compatibility in a + // diagnostic change. + const streamFunction = (rawToolCall as { function?: unknown }).function; + if (streamFunction !== undefined && streamFunction !== null) { + if (!isRecord(streamFunction)) { + return { + reason: "tool_call_function_not_object", + callIndex, + valueType: Array.isArray(streamFunction) ? "array" : typeof streamFunction, + }; + } + if (streamFunction.name !== undefined && typeof streamFunction.name !== "string") { + return { reason: "tool_call_function_name_invalid", callIndex, valueType: typeof streamFunction.name }; + } + if (streamFunction.arguments !== undefined && typeof streamFunction.arguments !== "string") { + return { reason: "tool_call_function_arguments_invalid", callIndex, valueType: typeof streamFunction.arguments }; + } + } + if (rawToolCall.id !== undefined && typeof rawToolCall.id !== "string") { + return { reason: "tool_call_id_invalid", callIndex, valueType: typeof rawToolCall.id }; + } + continue; + } + // Precedence must mirror the buffered validator below, or a payload with more than one + // problem is reported under the wrong reason and sends compatibility work after the wrong + // shape. That validator checks the `function` container first (`!isRecord(rawToolCall) || + // !isRecord(rawToolCall.function)`), then id/name/arguments types together, and only then + // the blank name. + if (!isRecord(rawToolCall.function)) { + return { + reason: "tool_call_function_not_object", + callIndex, + valueType: rawToolCall.function === null ? "null" : Array.isArray(rawToolCall.function) ? "array" : typeof rawToolCall.function, + }; + } + if (typeof rawToolCall.id !== "string") { + return { reason: "tool_call_id_invalid", callIndex, valueType: typeof rawToolCall.id }; + } + if (typeof rawToolCall.function.name !== "string") { + return { reason: "tool_call_function_name_invalid", callIndex, valueType: typeof rawToolCall.function.name }; + } + if (typeof rawToolCall.function.arguments !== "string") { + return { reason: "tool_call_function_arguments_invalid", callIndex, valueType: typeof rawToolCall.function.arguments }; + } + // Last, matching the validator: #1531 also rejects a blank or whitespace-only name here, + // because such a call cannot select a dispatch target. Reporting it as `name_invalid` + // would claim a type problem for a correctly-typed value, so it gets its own code. + if (rawToolCall.function.name.trim().length === 0) { + return { reason: "tool_call_function_name_blank", callIndex, valueType: "string" }; + } + } + return undefined; +} + +function logInvalidToolCalls(mode: "stream" | "response", rawToolCalls: unknown): void { + const diagnostic = diagnoseInvalidToolCalls(rawToolCalls, mode); + if (diagnostic) debugProviderDiagnostic("openai-chat", "invalid-tool-calls", { mode, ...diagnostic }); +} + function developerSystemText(message: OcxMessage): string | undefined { if (message.role !== "developer") return undefined; if (typeof message.content === "string") return message.content; @@ -1018,10 +1110,12 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd // through the adapter error channel instead of escaping as TypeError (#1325). Null is // tolerated as absent because OpenAI-compatible providers may emit it as stream padding. if (!Array.isArray(rawToolCalls)) { + logInvalidToolCalls("stream", rawToolCalls); return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); } for (const rawToolCall of rawToolCalls) { if (!isRecord(rawToolCall)) { + logInvalidToolCalls("stream", rawToolCalls); return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); } const tc = rawToolCall as { @@ -1036,16 +1130,19 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd const rawFunction = (rawToolCall as { function?: unknown }).function; if (rawFunction !== undefined && rawFunction !== null) { if (!isRecord(rawFunction)) { + logInvalidToolCalls("stream", rawToolCalls); return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); } const rawName = rawFunction.name; const rawArguments = rawFunction.arguments; if ((rawName !== undefined && typeof rawName !== "string") || (rawArguments !== undefined && typeof rawArguments !== "string")) { + logInvalidToolCalls("stream", rawToolCalls); return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); } } if (tc.id !== undefined && typeof tc.id !== "string") { + logInvalidToolCalls("stream", rawToolCalls); return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); } const key = typeof tc.index === "number" @@ -1206,9 +1303,13 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd if (typeof msg.content === "string") events.push({ type: "text_delta", text: msg.content }); const rawToolCalls = msg.tool_calls; if (rawToolCalls !== undefined && rawToolCalls !== null) { - if (!Array.isArray(rawToolCalls)) return [invalidToolCallsEvent(usage)]; + if (!Array.isArray(rawToolCalls)) { + logInvalidToolCalls("response", rawToolCalls); + return [invalidToolCallsEvent(usage)]; + } for (const rawToolCall of rawToolCalls) { if (!isRecord(rawToolCall) || !isRecord(rawToolCall.function)) { + logInvalidToolCalls("response", rawToolCalls); return [invalidToolCallsEvent(usage)]; } const id = rawToolCall.id; @@ -1219,6 +1320,7 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd // a whitespace-only function name is not a legitimate tool-call shape either. if (typeof id !== "string" || typeof name !== "string" || typeof args !== "string" || name.trim().length === 0) { + logInvalidToolCalls("response", rawToolCalls); return [invalidToolCallsEvent(usage)]; } events.push({ type: "tool_call_start", id, name }); diff --git a/tests/openai-chat-hardening.test.ts b/tests/openai-chat-hardening.test.ts index b78c3102b8..9a2117f4a4 100644 --- a/tests/openai-chat-hardening.test.ts +++ b/tests/openai-chat-hardening.test.ts @@ -1,5 +1,7 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, test } from "bun:test"; import { createOpenAIChatAdapter as createOpenAIChatAdapterProduction } from "../src/adapters/openai-chat"; +import { getDebugLogEntries, resetDebugLogBufferForTests } from "../src/lib/debug-log-buffer"; +import { resetDebugSettingsForTests } from "../src/lib/debug-settings"; import { routeModel } from "../src/router"; import type { AdapterEvent, OcxConfig, OcxParsedRequest, OcxProviderConfig } from "../src/types"; import { withTestTranslatorBudget } from "./helpers/translator-budget"; @@ -7,6 +9,15 @@ import { withTestTranslatorBudget } from "./helpers/translator-budget"; const createOpenAIChatAdapter = (...args: Parameters) => withTestTranslatorBudget(createOpenAIChatAdapterProduction(...args)); +const previousDebug = process.env.OCX_DEBUG; + +afterEach(() => { + resetDebugSettingsForTests(); + resetDebugLogBufferForTests(); + if (previousDebug === undefined) delete process.env.OCX_DEBUG; + else process.env.OCX_DEBUG = previousDebug; +}); + function parsed(): OcxParsedRequest { return { modelId: "test-model", @@ -148,6 +159,77 @@ describe("openai-chat non-stream response hardening", () => { }]); } }); + + test("debug mode records only the non-stream tool-call shape failure", async () => { + process.env.OCX_DEBUG = "1"; + const secretArguments = "private-tool-arguments"; + const adapter = createOpenAIChatAdapter(provider()); + const events = await adapter.parseResponse!(new Response(JSON.stringify({ + choices: [{ message: { role: "assistant", tool_calls: [{ + id: "call_1", + function: { name: "tool", arguments: { secretArguments } }, + }] } }], + }))); + + expect(events).toEqual([{ type: "error", message: "upstream response contained invalid tool calls" }]); + const lines = getDebugLogEntries().map(entry => entry.line).join("\n"); + expect(lines).toContain("[ocx:openai-chat:invalid-tool-calls]"); + expect(lines).toContain('"mode":"response"'); + expect(lines).toContain('"reason":"tool_call_function_arguments_invalid"'); + expect(lines).toContain('"valueType":"object"'); + expect(lines).not.toContain(secretArguments); + expect(lines).not.toContain("call_1"); + }); + + test("tool-call structural diagnostics stay disabled by default", async () => { + delete process.env.OCX_DEBUG; + const adapter = createOpenAIChatAdapter(provider()); + await adapter.parseResponse!(new Response(JSON.stringify({ + choices: [{ message: { role: "assistant", tool_calls: { privateArguments: "secret" } } }], + }))); + + expect(getDebugLogEntries()).toHaveLength(0); + }); + + // The diagnostic's job is to say WHICH check rejected the payload. If its precedence drifts + // from the validator's, a payload with more than one problem is reported under the wrong + // reason and sends provider-compatibility work after the wrong shape. These cases each carry + // two defects at once, so only the matching order produces the expected reason. + describe("diagnostic precedence matches the buffered validator", () => { + async function reasonFor(toolCall: unknown): Promise { + process.env.OCX_DEBUG = "1"; + const adapter = createOpenAIChatAdapter(provider()); + await adapter.parseResponse!(new Response(JSON.stringify({ + choices: [{ message: { role: "assistant", tool_calls: [toolCall] } }], + }))); + const lines = getDebugLogEntries().map(entry => entry.line).join("\n"); + const match = /"reason":"([a-z_]+)"/.exec(lines); + return match?.[1] ?? ""; + } + + test("a bad function container outranks a bad id", async () => { + // Validator checks `!isRecord(rawToolCall.function)` before it reads `id`. + expect(await reasonFor({ id: 7, function: "not-an-object" })) + .toBe("tool_call_function_not_object"); + }); + + test("a bad id outranks a bad name", async () => { + expect(await reasonFor({ id: 7, function: { name: 9, arguments: "{}" } })) + .toBe("tool_call_id_invalid"); + }); + + test("a bad arguments type outranks a blank name", async () => { + // Both are rejected by the same validator condition; arguments is checked first there, + // so a blank name must not shadow it. + expect(await reasonFor({ id: "call_1", function: { name: " ", arguments: 5 } })) + .toBe("tool_call_function_arguments_invalid"); + }); + + test("a blank name is reported as blank, not as a type problem", async () => { + expect(await reasonFor({ id: "call_1", function: { name: " ", arguments: "{}" } })) + .toBe("tool_call_function_name_blank"); + }); + }); }); describe("openai-chat stream response hardening", () => { @@ -227,6 +309,29 @@ describe("openai-chat stream response hardening", () => { }]); } }); + + test("debug mode classifies streaming tool-call structure without retaining values", async () => { + process.env.OCX_DEBUG = "1"; + const privateName = "private-tool-name"; + const adapter = createOpenAIChatAdapter(provider()); + const response = new Response([ + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [{ + privateName, + privateArguments: "private arguments", + }, null] } }] })}\n\n`, + "data: [DONE]\n\n", + ].join("")); + + const events = await collect(adapter.parseStream(response)); + expect(events).toEqual([{ type: "error", message: "upstream response contained invalid tool calls" }]); + const lines = getDebugLogEntries().map(entry => entry.line).join("\n"); + expect(lines).toContain('"mode":"stream"'); + expect(lines).toContain('"reason":"tool_call_not_object"'); + expect(lines).toContain('"callIndex":1'); + expect(lines).toContain('"valueType":"null"'); + expect(lines).not.toContain(privateName); + expect(lines).not.toContain("private arguments"); + }); }); describe("openai-chat credential hardening", () => {