From 772d375fed252f838c41b9315cf88046f64d192a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 09:57:10 +0900 Subject: [PATCH] fix(openai-chat): treat a non-string repeat as padding once that field is known Some OpenAI-compatible streamers repeat an already-sent id, name, or arguments as a non-string placeholder on a continuation delta rather than as null. Validation ran before the pending-call lookup, so the whole turn died with a 502 and the tool never ran -- even though the value being repeated was already held in canonical form. The lookup now happens first and tolerance is per field, keyed on that field's own provenance. Two corrections on top of @waw4303's #2155. It gated arguments acceptance on the call having a canonical NAME. A name says nothing about whether arguments was ever sent as a string, so a real argument payload could be silently dropped. PendingToolCall now carries sawArgumentsString; an empty string counts, because it proves the upstream sent the field with the right wire type. It also left a non-string repeated id unconditionally terminal even after a canonical id was stored. Ids now follow the same rule as the other two. Diagnostics are passed from the rejection site instead of rescanned. A stateless rescan stops at the first structurally odd value, so a stream carrying accepted padding on call 0 and a real defect on call 1 blamed call 0. --- src/adapters/openai-chat.ts | 141 +++++++++++++++++++--------- tests/openai-chat-hardening.test.ts | 96 +++++++++++++++++++ 2 files changed, 191 insertions(+), 46 deletions(-) diff --git a/src/adapters/openai-chat.ts b/src/adapters/openai-chat.ts index c5338d5eaa..8e59559094 100644 --- a/src/adapters/openai-chat.ts +++ b/src/adapters/openai-chat.ts @@ -308,8 +308,13 @@ function invalidToolCallsEvent( rawToolCalls: unknown, mode: "stream" | "response", usage?: OcxUsage, + diagnosticOverride?: InvalidToolCallDiagnostic, ): Extract { - const diagnostic = diagnoseInvalidToolCalls(rawToolCalls, mode); + // The streamed accumulator knows things a rescan cannot: which field on which pending call + // was actually rejected. Without the override, a stream carrying accepted padding on call 0 + // and a real defect on call 1 blames call 0, because the stateless scan stops at the first + // structurally odd value it sees. + const diagnostic = diagnosticOverride ?? diagnoseInvalidToolCalls(rawToolCalls, mode); const detail = diagnostic ? ` (${diagnostic.reason}${diagnostic.callIndex !== undefined ? `; callIndex=${diagnostic.callIndex}` : ""}; valueType=${diagnostic.valueType})` : ""; @@ -527,9 +532,13 @@ function diagnoseInvalidToolCalls( return undefined; } -function logInvalidToolCalls(mode: "stream" | "response", rawToolCalls: unknown): void { +function logInvalidToolCalls( + mode: "stream" | "response", + rawToolCalls: unknown, + diagnosticOverride?: InvalidToolCallDiagnostic, +): void { if (!isDebugEnabled()) return; - const diagnostic = diagnoseInvalidToolCalls(rawToolCalls, mode); + const diagnostic = diagnosticOverride ?? diagnoseInvalidToolCalls(rawToolCalls, mode); if (!diagnostic) return; const fieldShape = fingerprintInvalidField(invalidToolCallField(rawToolCalls, diagnostic)); debugProviderDiagnostic("openai-chat", "invalid-tool-calls", { @@ -1499,7 +1508,20 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd const budgetEncoder = new TextEncoder(); let buffer = ""; let bufferBytes = 0; - interface PendingToolCall { key: string; id: string; name: string; args: string; argsBytes: number } + interface PendingToolCall { + key: string; + id: string; + name: string; + args: string; + argsBytes: number; + /** + * Whether this call has ever received `arguments` as an actual string, empty included. + * An empty string still counts: it proves the upstream sent the field with the right + * wire type, which is what a later malformed repeat of that field would be padding for. + * A canonical NAME is not evidence about the ARGUMENTS field and must not stand in. + */ + sawArgumentsString: boolean; + } const pendingToolCalls: PendingToolCall[] = []; let toolCallSeq = 0; const closeToolCalls = (): PendingToolCall[] => { @@ -1613,61 +1635,88 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd logInvalidToolCalls("stream", rawToolCalls); return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage)); } - for (const rawToolCall of rawToolCalls) { + for (let callIndex = 0; callIndex < rawToolCalls.length; callIndex++) { + const rawToolCall: unknown = rawToolCalls[callIndex]; if (!isRecord(rawToolCall)) { - logInvalidToolCalls("stream", rawToolCalls); - return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage)); + const diagnostic: InvalidToolCallDiagnostic = { + reason: "tool_call_not_object", + callIndex, + valueType: rawToolCall === null ? "null" : Array.isArray(rawToolCall) ? "array" : typeof rawToolCall, + }; + logInvalidToolCalls("stream", rawToolCalls, diagnostic); + return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage, diagnostic)); } - const tc = rawToolCall as { - index?: number; - id?: string; - function?: { name?: string; arguments?: string }; - }; - // That cast is a TypeScript convenience, not a runtime guarantee: this is - // upstream JSON. Validate the fields before they are stored, so a non-string - // name or arguments value fails closed through the #1325 channel here rather - // than escaping later as a TypeError from string handling at flush time. - const rawFunction = (rawToolCall as { function?: unknown }).function; - if (rawFunction !== undefined && rawFunction !== null) { - if (!isRecord(rawFunction)) { - logInvalidToolCalls("stream", rawToolCalls); - return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage)); - } - const rawName = rawFunction.name; - const rawArguments = rawFunction.arguments; - // Some OpenAI-compatible streamers repeat already-sent fields as null on - // continuation deltas. Treat only null/undefined as absent; every other - // non-string value still fails closed before entering the accumulator. - if (isInvalidStreamStringField(rawName) || isInvalidStreamStringField(rawArguments)) { - logInvalidToolCalls("stream", rawToolCalls); - return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage)); - } + // This is upstream JSON, so every field is validated before it is stored: a + // malformed value must fail closed through the #1325 channel here rather than + // escaping later as a TypeError from string handling at flush time. + const rawFunction = rawToolCall.function; + if (rawFunction !== undefined && rawFunction !== null && !isRecord(rawFunction)) { + const diagnostic: InvalidToolCallDiagnostic = { + reason: "tool_call_function_not_object", + callIndex, + valueType: Array.isArray(rawFunction) ? "array" : typeof rawFunction, + }; + logInvalidToolCalls("stream", rawToolCalls, diagnostic); + return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage, diagnostic)); } - if (isInvalidStreamStringField(tc.id)) { - logInvalidToolCalls("stream", rawToolCalls); - return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage)); - } - const key = typeof tc.index === "number" - ? `i:${tc.index}` - : tc.id - ? `id:${tc.id}` + const fnRecord = isRecord(rawFunction) ? rawFunction : undefined; + const rawName = fnRecord?.name; + const rawArguments = fnRecord?.arguments; + const rawId = rawToolCall.id; + const idDelta = typeof rawId === "string" ? rawId : ""; + const rawIndex = rawToolCall.index; + + // Resolve the pending call BEFORE judging the fields. Some OpenAI-compatible + // streamers repeat an already-sent field as a non-string placeholder on a + // continuation delta; judging first meant the whole stream died with a 502 even + // though the value being repeated was already held in canonical form. + const key = typeof rawIndex === "number" + ? `i:${rawIndex}` + : idDelta + ? `id:${idDelta}` : pendingToolCalls[pendingToolCalls.length - 1]?.key; let call = key !== undefined ? pendingToolCalls.find(c => c.key === key) : undefined; - if (!call && tc.id) call = pendingToolCalls.find(c => c.id === tc.id); + if (!call && idDelta) call = pendingToolCalls.find(c => c.id === idDelta); if (!call) { - call = { key: key ?? `seq:${pendingToolCalls.length}`, id: "", name: "", args: "", argsBytes: 0 }; + call = { + key: key ?? `seq:${pendingToolCalls.length}`, + id: "", + name: "", + args: "", + argsBytes: 0, + sawArgumentsString: false, + }; pendingToolCalls.push(call); budget.openCall(call.key); } - if (tc.id && !call.id) call.id = tc.id; - if (tc.function?.name && !call.name) call.name = tc.function.name; - if (tc.function?.arguments) { + + // Tolerance is per FIELD, keyed on that field's own provenance. A canonical name + // says nothing about whether `arguments` was ever sent as a string, so it cannot + // authorize a malformed arguments value — that would silently drop a real + // argument payload the model intended to send. + const rejection: InvalidToolCallDiagnostic | undefined = + isInvalidStreamStringField(rawName) && call.name.trim() === "" + ? { reason: "tool_call_function_name_invalid", callIndex, valueType: typeof rawName } + : isInvalidStreamStringField(rawArguments) && !call.sawArgumentsString + ? { reason: "tool_call_function_arguments_invalid", callIndex, valueType: typeof rawArguments } + : isInvalidStreamStringField(rawId) && call.id === "" + ? { reason: "tool_call_id_invalid", callIndex, valueType: typeof rawId } + : undefined; + if (rejection) { + logInvalidToolCalls("stream", rawToolCalls, rejection); + return yield* terminateWithError(invalidToolCallsEvent(rawToolCalls, "stream", pendingUsage, rejection)); + } + + if (idDelta && !call.id) call.id = idDelta; + if (typeof rawName === "string" && rawName && !call.name) call.name = rawName; + if (typeof rawArguments === "string") call.sawArgumentsString = true; + if (typeof rawArguments === "string" && rawArguments) { const previousBytes = call.argsBytes; - const nextBytes = previousBytes + budgetEncoder.encode(tc.function.arguments).byteLength; + const nextBytes = previousBytes + budgetEncoder.encode(rawArguments).byteLength; const scope = { kind: "tool_args" as const, callId: call.key }; const reservation = budget.reserveTransient(nextBytes, scope); try { - call.args += tc.function.arguments; + call.args += rawArguments; reservation.commitRetained(); budget.releaseRetained(previousBytes, scope); call.argsBytes = nextBytes; diff --git a/tests/openai-chat-hardening.test.ts b/tests/openai-chat-hardening.test.ts index f7435e0a28..81671b0968 100644 --- a/tests/openai-chat-hardening.test.ts +++ b/tests/openai-chat-hardening.test.ts @@ -476,6 +476,102 @@ describe("openai-chat stream response hardening", () => { expect(lines).toContain('"callIndex":1'); expect(lines).not.toContain('"tool_call_function_name_invalid"'); }); + + // Some OpenAI-compatible streamers repeat an already-sent field as a non-string placeholder + // instead of null. Before #2155 that killed the whole turn with a 502 even though the value + // being repeated was already held in canonical form, so the tool never ran. + test("a non-string repeat is padding once that field has string provenance (#2155)", async () => { + const adapter = createOpenAIChatAdapter(provider()); + const response = new Response([ + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, id: "call_a", function: { name: "shell", arguments: "" } }, + ] } }] })}\n\n`, + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, id: { padding: true }, function: { name: { padding: true }, arguments: { padding: true } } }, + ] } }] })}\n\n`, + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, function: { arguments: "{}" } }, + ] } }] })}\n\n`, + 'data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + + const events = await collect(adapter.parseStream(response)); + expect(events.some(event => event.type === "error")).toBe(false); + expect(events).toContainEqual({ type: "tool_call_start", id: "call_a", name: "shell" }); + expect(events).toContainEqual({ type: "tool_call_delta", arguments: "{}" }); + }); + + // Tolerance is per field. A canonical NAME is not evidence that `arguments` was ever sent + // as a string, and accepting a malformed object here would silently discard an argument + // payload the model meant to send. + test("a canonical name does not authorize a malformed arguments value (#2155)", async () => { + const adapter = createOpenAIChatAdapter(provider()); + const response = new Response([ + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, id: "call_a", function: { name: "shell" } }, + ] } }] })}\n\n`, + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, function: { arguments: { bad: true } } }, + ] } }] })}\n\n`, + "data: [DONE]\n\n", + ].join("")); + + expect(await collect(adapter.parseStream(response))).toEqual([{ + type: "error", + status: 502, + errorType: "upstream_error", + message: "upstream response contained invalid tool calls (tool_call_function_arguments_invalid; callIndex=0; valueType=object)", + }]); + }); + + test("a malformed id stays terminal until that call has a canonical id (#2155)", async () => { + const adapter = createOpenAIChatAdapter(provider()); + const response = new Response([ + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, function: { name: "shell", arguments: "{}" } }, + ] } }] })}\n\n`, + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, id: { bad: true } }, + ] } }] })}\n\n`, + "data: [DONE]\n\n", + ].join("")); + + expect(await collect(adapter.parseStream(response))).toEqual([{ + type: "error", + status: 502, + errorType: "upstream_error", + message: "upstream response contained invalid tool calls (tool_call_id_invalid; callIndex=0; valueType=object)", + }]); + }); + + // The reason the diagnostic is passed from the rejection site rather than rescanned: a + // stateless rescan stops at the first structurally odd value, which here is the ACCEPTED + // padding on call 0, and would blame the wrong call for the real defect on call 1. + test("parallel calls blame the unresolved call, not the accepted padding (#2155)", async () => { + process.env.OCX_DEBUG = "1"; + const adapter = createOpenAIChatAdapter(provider()); + const response = new Response([ + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, id: "call_a", function: { name: "alpha", arguments: "" } }, + { index: 1, id: "call_b", function: { name: "beta" } }, + ] } }] })}\n\n`, + `data: ${JSON.stringify({ choices: [{ delta: { tool_calls: [ + { index: 0, function: { arguments: { padding: true } } }, + { index: 1, function: { arguments: { bad: true } } }, + ] } }] })}\n\n`, + "data: [DONE]\n\n", + ].join("")); + + expect(await collect(adapter.parseStream(response))).toEqual([{ + type: "error", + status: 502, + errorType: "upstream_error", + message: "upstream response contained invalid tool calls (tool_call_function_arguments_invalid; callIndex=1; valueType=object)", + }]); + const lines = getDebugLogEntries().map(entry => entry.line).join("\n"); + expect(lines).toContain('"callIndex":1'); + }); }); describe("openai-chat credential hardening", () => {