From 4255c0f81b50d548739dc9c9cfb813b0994dc68e Mon Sep 17 00:00:00 2001 From: Can Date: Tue, 18 Aug 2026 17:10:50 +0300 Subject: [PATCH 1/5] fix(llm): fail closed on malformed native tool calls Two behaviors combined to let a truncated or malformed native OpenAI-compatible tool call reach real execution: 1. parseArguments() caught any JSON.parse failure on a tool call's arguments and silently returned {} instead of surfacing an error - a truncated argument string was indistinguishable from an intentional empty call. 2. A tool-call stream ending by bare EOF, with no provider finish_reason and no [DONE], was treated identically to a confirmed clean completion (finishReason -> null, stop -> true, truncated -> false). Since native tool calls with empty text content are intentionally allowed through when toolCalls exist, nothing stopped the unconfirmed call from reaching dispatch. Fix: - parseArguments() now throws on genuinely non-empty malformed JSON (or JSON that parses to something other than an object) instead of substituting {}. A legitimately empty/whitespace argument string still maps to {}. The failure surfaces through openAiToolCallsToBatch() as ToolCallArgumentsParseError, which reaches tryParseToolCalls()'s existing catch block and routes through the same one-shot repair path grammar-parsed batches already use - no new error subsystem. The raw arguments are never included in the error message, since they may carry sensitive data that reaches logs. - The stream consumer now tracks whether a trustworthy terminal signal was actually observed (an explicit provider finish_reason on any chunk, or a parser-recognized [DONE]) before the stream ended. completionFromStreamFinal() folds an unconfirmed pending tool call into the existing truncated/stop computation, so it is caught by detectModelFailure's first check before parseArguments or validateBatch are ever reached. A provider that sends finish_reason without [DONE] remains accepted; explicit finish_reason: "length" is unchanged; plain-text-only responses are unaffected, since the fix only applies when a tool call is actually pending. Verified with an execution-level regression suite driving the real OpenAiProvider, step executor, and ToolRegistry with an instrumented no-op tool: a malformed or unconfirmed call now executes zero times in every case that previously executed once - malformed args under a clean terminal, malformed args under a bare-EOF stream, container- level truncation under a bare-EOF stream, syntactically complete args under a bare-EOF stream, and the same case duplicated across parallel calls. A healthy call still executes exactly once, explicit finish_reason: "length" remains zero executions, and a stream read error still propagates as a rejection rather than a silent no-op. --- ...tive-tool-call-execution-integrity.test.ts | 308 ++++++++++++++++++ src/llm/provider/completion-types.ts | 9 + src/llm/provider/openai/openai-provider.ts | 15 +- .../provider/openai/openai-stream-consumer.ts | 13 + .../openai/openai-tool-call-adapter.test.ts | 65 ++++ .../openai/openai-tool-call-adapter.ts | 43 ++- 6 files changed, 442 insertions(+), 11 deletions(-) create mode 100644 src/agent/native-tool-call-execution-integrity.test.ts diff --git a/src/agent/native-tool-call-execution-integrity.test.ts b/src/agent/native-tool-call-execution-integrity.test.ts new file mode 100644 index 00000000..d5942f47 --- /dev/null +++ b/src/agent/native-tool-call-execution-integrity.test.ts @@ -0,0 +1,308 @@ +import { describe, expect, it } from "vitest"; +import { join } from "node:path"; +import { OpenAiProvider } from "../llm/provider/openai/openai-provider.js"; +import { openAiToolCallAdapter } from "../llm/provider/openai/openai-tool-call-adapter.js"; +import type { CompletionRequest, CompletionResult } from "../llm/provider/completion-types.js"; +import { executeStep, type StepDependencies } from "./step-executor.js"; +import { ToolRegistry } from "../tools/tool-registry.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import { PLAIN_INSTRUCT_PROFILE } from "../llm/model-profile.js"; +import { buildGrammar } from "../llm/grammar/build-grammar.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import { DEFAULT_TOOL_DESCRIPTORS } from "../prompt/tool-descriptors.js"; +import { compressToolResult } from "../compressor/result-compressor.js"; +import type { CapabilitiesSummary, SkillCatalogEntry } from "../prompt/stable-prefix.js"; + +/** + * Execution-integrity regression suite for the native OpenAI-compatible + * tool-call path. + * + * Drives the real `OpenAiProvider` (SSE parsing) and the real + * `executeStep()` (tool dispatch) with an instrumented no-op tool that + * only counts invocations — never a real filesystem/network/shell effect. + * + * Proves, at the actual dispatch boundary, that: + * - a malformed or ambiguously-terminated tool call is never invoked, + * - a healthy call still executes exactly once, + * - an explicit `finish_reason: "length"` still fails closed. + */ + +const tools: NonNullable = [ + { type: "function", function: { name: "os__fs__delete", parameters: { type: "object", properties: {} } } }, +]; + +function sseFrame(obj: Record): string { + return `data: ${JSON.stringify(obj)}\n\n`; +} + +/** Streams one tool call's arguments, then the connection just ends — no + * finish_reason chunk, no `[DONE]`. */ +function eofBody(toolCallArgs: string, toolName = "os__fs__delete"): string { + return ( + sseFrame({ + choices: [ + { + index: 0, + delta: { role: "assistant", tool_calls: [{ index: 0, id: "call_1", type: "function", function: { name: toolName, arguments: "" } }] }, + finish_reason: null, + }, + ], + }) + + sseFrame({ choices: [{ index: 0, delta: { tool_calls: [{ index: 0, function: { arguments: toolCallArgs } }] }, finish_reason: null }] }) + ); +} + +function healthyBody(toolCallArgs: string, toolName = "os__fs__delete"): string { + return ( + eofBody(toolCallArgs, toolName) + + sseFrame({ choices: [{ index: 0, delta: {}, finish_reason: "tool_calls" }], usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 } }) + + "data: [DONE]\n\n" + ); +} + +function lengthTerminatedBody(toolCallArgs: string, toolName = "os__fs__delete"): string { + return ( + eofBody(toolCallArgs, toolName) + + sseFrame({ choices: [{ index: 0, delta: {}, finish_reason: "length" }], usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 } }) + + "data: [DONE]\n\n" + ); +} + +function fetchReturning(body: string) { + return (async () => new Response(body, { status: 200, headers: { "content-type": "text/event-stream" } })) as unknown as typeof fetch; +} + +function fetchErroringMidStream(prefixBody: string) { + return (async () => { + const encoder = new TextEncoder(); + const body = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode(prefixBody)); + controller.error(new Error("simulated transport read error")); + }, + }); + return new Response(body, { status: 200, headers: { "content-type": "text/event-stream" } }); + }) as unknown as typeof fetch; +} + +async function drainCompleteStream(fetchImpl: typeof fetch): Promise { + const provider = new OpenAiProvider({ id: "test", baseUrl: "https://example.invalid", apiKey: "", defaultChatModel: "m", fetchImpl }); + const gen = provider.completeStream({ prompt: "delete widget.txt", tools }); + let next = await gen.next(); + while (!next.done) next = await gen.next(); + return next.value; +} + +function makeCaps(): CapabilitiesSummary { + return { platform: "linux", arch: "x64", browserChannel: "chrome", workingDir: "/work", hasClipboard: false, hasWmctrl: false, hasNotifications: false }; +} + +async function runStepThroughLlmComplete( + llmComplete: StepDependencies["llmComplete"], + registerTools: (registry: ToolRegistry) => void, +): Promise<{ outcomeOrError: unknown }> { + const registry = new ToolRegistry(); + registerTools(registry); + registry.register({ + name: "reply", + description: "reply", + readonly: true, + async run(args: Record) { + return compressToolResult({ tool: "reply", status: "ok", output: String(args.text ?? "") }); + }, + }); + + const grammarsDir = join(process.cwd(), "grammars"); + const grammar = await buildGrammar(PLAIN_INSTRUCT_PROFILE, grammarsDir); + const session = createEmptySessionState({ id: "s-integrity", workingDir: "/w" }); + const deps: StepDependencies = { + registry, + slotManager: new SlotManager(2), + llmComplete, + grammar, + profile: PLAIN_INSTRUCT_PROFILE, + toolTransport: "native_tools", + toolCallAdapter: openAiToolCallAdapter, + supportsSlotAffinity: false, + }; + + let outcomeOrError: unknown; + try { + outcomeOrError = await executeStep( + { + session, + toolDescriptors: DEFAULT_TOOL_DESCRIPTORS, + capabilities: makeCaps(), + skillCatalog: [] as SkillCatalogEntry[], + stepIndex: 0, + signal: new AbortController().signal, + userMessage: "delete the widget", + }, + deps, + ); + } catch (err) { + outcomeOrError = err; + } + return { outcomeOrError }; +} + +/** End-to-end: drives the real SSE parser via a fake fetch, then feeds + * the resulting real CompletionResult into the real step executor. */ +async function runStepFromFetch( + fetchImpl: typeof fetch, + registerTools: (registry: ToolRegistry) => void, +): Promise<{ outcomeOrError: unknown; completion: CompletionResult }> { + const completion = await drainCompleteStream(fetchImpl); + const { outcomeOrError } = await runStepThroughLlmComplete(async () => completion, registerTools); + return { outcomeOrError, completion }; +} + +function countingTool(name: string) { + let count = 0; + return { + executions: () => count, + register: (registry: ToolRegistry) => { + registry.register({ + name, + description: "instrumented no-op test tool", + readonly: false, + async run(args: Record) { + count += 1; + return compressToolResult({ tool: name, status: "ok", output: `noop args=${JSON.stringify(args)}` }); + }, + }); + }, + }; +} + +describe("native tool-call execution integrity", () => { + it("1. healthy valid tool call: executions = 1", async () => { + const tool = countingTool("os.fs.delete"); + const { outcomeOrError } = await runStepFromFetch(fetchReturning(healthyBody('{"path":"widget.txt"}')), tool.register); + expect(tool.executions()).toBe(1); + expect(outcomeOrError).not.toBeInstanceOf(Error); + }); + + it("2. malformed JSON + clean terminal (finish_reason: tool_calls): executions = 0", async () => { + const tool = countingTool("os.fs.delete"); + const { outcomeOrError } = await runStepFromFetch(fetchReturning(healthyBody('{"path":"widget.txt')), tool.register); + expect(tool.executions()).toBe(0); + // Routed through the existing one-shot repair path, not a silent {} execute. + expect(outcomeOrError).toBeInstanceOf(Error); + }); + + it("3. malformed JSON + abrupt EOF (no finish_reason, no [DONE]): executions = 0", async () => { + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch(fetchReturning(eofBody('{"path":"widget.txt')), tool.register); + expect(completion.finishReason).toBeNull(); + expect(completion.truncated).toBe(true); // now correctly flagged + expect(tool.executions()).toBe(0); + expect(outcomeOrError).toBeInstanceOf(Error); + }); + + it("4. container-level truncated JSON + abrupt EOF: executions = 0", async () => { + const tool = countingTool("os.shell.run"); + const { outcomeOrError } = await runStepFromFetch( + fetchReturning(eofBody('{"commands":["npm install","npm test"', "os__shell__run")), + tool.register, + ); + expect(tool.executions()).toBe(0); + expect(outcomeOrError).toBeInstanceOf(Error); + }); + + it("5. syntactically COMPLETE JSON but ambiguous EOF: executions = 0", async () => { + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch(fetchReturning(eofBody('{"path":"widget.txt"}')), tool.register); + expect(completion.finishReason).toBeNull(); + expect(completion.truncated).toBe(true); + expect(tool.executions()).toBe(0); + expect(outcomeOrError).toBeInstanceOf(Error); + }); + + it("6. explicit finish_reason: length: executions = 0 (control, unchanged)", async () => { + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch(fetchReturning(lengthTerminatedBody('{"path":"widget.txt"}')), tool.register); + expect(completion.finishReason).toBe("length"); + expect(completion.truncated).toBe(true); + expect(tool.executions()).toBe(0); + expect(outcomeOrError).toBeInstanceOf(Error); + }); + + it("7. parallel calls (properly resource-classed tools): A complete + B truncated, ambiguous EOF -> neither executes", async () => { + // os.fs.read and os.fs.grep are both real, registered `pure_read` + // entries in tool-resource-class.ts, so this batch is accepted by + // validateBatch on its own merits (not rejected as "no resource + // class") — isolating the truncation-safety question cleanly. + // + // truncated:true / stop:false / finishReason:null below is exactly + // what completionFromStreamFinal() now derives for an ambiguous EOF + // with pending tool calls (see openai-provider.ts). Built directly + // here rather than via a real fetch so this test's focus stays on + // the step-executor's per-call handling once that flag is set. + const toolA = countingTool("os.fs.read"); + const toolB = countingTool("os.fs.grep"); + const completion: CompletionResult = { + content: "", + reasoningContent: "", + stop: false, + truncated: true, + timing: { promptMs: 0, predictedMs: 0, promptTokens: 1, predictedTokens: 1 }, + cacheHitTokens: 0, + slotId: -1, + modelId: "m", + usage: { promptTokens: 1, completionTokens: 1, totalTokens: 2 }, + toolCalls: [ + { id: "call_a", type: "function", function: { name: "os__fs__read", arguments: '{"path":"a.txt"}' } }, + { id: "call_b", type: "function", function: { name: "os__fs__grep", arguments: '{"path":"b.txt' } }, + ], + finishReason: null, + }; + + const { outcomeOrError } = await runStepThroughLlmComplete(async () => completion, (registry) => { + toolA.register(registry); + toolB.register(registry); + }); + expect(toolA.executions()).toBe(0); + expect(toolB.executions()).toBe(0); + expect(outcomeOrError).toBeInstanceOf(Error); + }); + + it("8. stream read error mid-stream: executions = 0", async () => { + const tool = countingTool("os.fs.delete"); + let threw = false; + try { + await drainCompleteStream(fetchErroringMidStream(eofBody('{"path":"widget.txt'))); + } catch { + threw = true; + } + expect(threw).toBe(true); + expect(tool.executions()).toBe(0); + }); + + it("9. compatibility: finish_reason sent but connection ends without [DONE] must still be trusted as a clean completion", async () => { + // Some OpenAI-compatible providers omit the [DONE] sentinel entirely + // but do send a real finish_reason on the last data chunk. That is a + // trustworthy terminal signal on its own and must NOT be treated as + // ambiguous just because [DONE] never arrived. + const bodyWithFinishReasonButNoDone = + eofBody('{"path":"widget.txt"}') + + sseFrame({ choices: [{ index: 0, delta: {}, finish_reason: "tool_calls" }], usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 } }); + // deliberately no "data: [DONE]\n\n" appended + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch(fetchReturning(bodyWithFinishReasonButNoDone), tool.register); + expect(completion.finishReason).toBe("tool_calls"); + expect(completion.truncated).toBe(false); + expect(tool.executions()).toBe(1); + expect(outcomeOrError).not.toBeInstanceOf(Error); + }); + + it("10. compatibility: plain text-only response with ambiguous EOF is unaffected by the tool-call fix", async () => { + // No tool_calls at all — hasPendingToolCalls is false, so this fix's + // truncated/stop computation must not touch this path at all. + const textOnlyEofBody = sseFrame({ choices: [{ index: 0, delta: { role: "assistant", content: "hello" }, finish_reason: null }] }); + const completion = await drainCompleteStream(fetchReturning(textOnlyEofBody)); + expect(completion.toolCalls).toBeUndefined(); + expect(completion.truncated).toBe(false); + expect(completion.stop).toBe(true); + }); +}); diff --git a/src/llm/provider/completion-types.ts b/src/llm/provider/completion-types.ts index d6cd5644..6acc50d2 100644 --- a/src/llm/provider/completion-types.ts +++ b/src/llm/provider/completion-types.ts @@ -122,4 +122,13 @@ export interface StreamFinalResult { finishReason?: string | null; usage?: CompletionUsage; modelId?: string | null; + /** + * Whether the underlying transport actually delivered a trustworthy + * terminal signal — an explicit provider `finish_reason` on any chunk, + * or a parser-recognized terminal event (e.g. `[DONE]`) — before the + * stream ended. `false` (or absent) means the connection just closed + * (bare EOF / read error) without either: not asserted, so callers must + * not treat an absent value as confirmation of a clean completion. + */ + terminalObserved?: boolean; } diff --git a/src/llm/provider/openai/openai-provider.ts b/src/llm/provider/openai/openai-provider.ts index af400e7d..12cece9c 100644 --- a/src/llm/provider/openai/openai-provider.ts +++ b/src/llm/provider/openai/openai-provider.ts @@ -219,11 +219,22 @@ function completionFromStreamFinal( totalTokens: 0, }; const finishReason = streamFinal?.finishReason ?? null; + // A native tool call whose stream ended without a trustworthy terminal + // signal (no explicit finish_reason, no [DONE]/parser terminal event — + // just the connection closing) is not a confirmed completion. Treating + // it as an ordinary `stop` would let a mid-stream-cut tool call reach + // dispatch indistinguishably from a genuinely finished one. This only + // applies when a tool call is actually pending — a plain-text response + // that races the same way already has its own `stop`/`no_stop` handling + // downstream and is left untouched here. + const hasPendingToolCalls = (streamFinal?.toolCalls?.length ?? 0) > 0; + const ambiguousToolCallTermination = + hasPendingToolCalls && streamFinal?.terminalObserved !== true; return { content: streamFinal?.content ?? accumulated, reasoningContent: streamFinal?.reasoningContent ?? accumulatedReasoning, - stop: finishReason !== "length", - truncated: finishReason === "length", + stop: finishReason !== "length" && !ambiguousToolCallTermination, + truncated: finishReason === "length" || ambiguousToolCallTermination, timing: { promptMs: 0, predictedMs: 0, diff --git a/src/llm/provider/openai/openai-stream-consumer.ts b/src/llm/provider/openai/openai-stream-consumer.ts index a866e3c6..573e611a 100644 --- a/src/llm/provider/openai/openai-stream-consumer.ts +++ b/src/llm/provider/openai/openai-stream-consumer.ts @@ -37,6 +37,14 @@ export function createOpenAiStreamConsumer( let finishReason: string | null = null; let modelId: string | null = null; let usage: CompletionUsage | undefined; + // A trustworthy terminal signal: an explicit provider finish_reason + // on any chunk, or a parser-recognized terminal event (`[DONE]`). + // Some OpenAI-compatible providers send a final finish_reason and + // then simply close the connection without ever emitting `[DONE]` — + // that still counts. A bare `reader.read()` EOF with neither must + // NOT be conflated with either, since a still-open tool call's + // arguments may be mid-stream. + let terminalObserved = false; const toolCalls = new Map(); try { while (true) { @@ -51,6 +59,7 @@ export function createOpenAiStreamConsumer( const chunk = parseOpenAiSseEvent(rawEvent, reasoning, toolArgsBuffer); content += chunk.delta; reasoningContent += chunk.reasoningDelta; + if (chunk.finishReason !== null) terminalObserved = true; finishReason = chunk.finishReason ?? finishReason; modelId = chunk.modelId ?? modelId; usage = normaliseUsage(chunk.usage) ?? usage; @@ -64,6 +73,7 @@ export function createOpenAiStreamConsumer( modelId, usage, toolCalls, + terminalObserved: true, }); } if (chunk.toolArgsDelta !== undefined) { @@ -105,6 +115,7 @@ export function createOpenAiStreamConsumer( modelId, usage, toolCalls, + terminalObserved, }); }, }; @@ -135,6 +146,7 @@ function buildFinalResult(args: { modelId: string | null; usage?: CompletionUsage; toolCalls: ReadonlyMap; + terminalObserved: boolean; }): StreamFinalResult { const sortedToolCalls = [...args.toolCalls.entries()] .sort(([a], [b]) => a - b) @@ -145,6 +157,7 @@ function buildFinalResult(args: { reasoningContent: args.reasoningContent, finishReason: args.finishReason, modelId: args.modelId, + terminalObserved: args.terminalObserved, ...(args.usage ? { usage: args.usage } : {}), ...(sortedToolCalls.length > 0 ? { toolCalls: sortedToolCalls } : {}), }; diff --git a/src/llm/provider/openai/openai-tool-call-adapter.test.ts b/src/llm/provider/openai/openai-tool-call-adapter.test.ts index a773464a..ed1227d8 100644 --- a/src/llm/provider/openai/openai-tool-call-adapter.test.ts +++ b/src/llm/provider/openai/openai-tool-call-adapter.test.ts @@ -4,6 +4,7 @@ import { nameUnescape, descriptorsToOpenAiTools, openAiToolCallsToBatch, + ToolCallArgumentsParseError, } from "./openai-tool-call-adapter.js"; describe("OpenAiToolCallAdapter", () => { @@ -32,6 +33,70 @@ describe("OpenAiToolCallAdapter", () => { expect(batch.calls[0]?.args).toMatchObject({ text: "hello" }); }); + it("maps a legitimately empty arguments string to {}", () => { + const batch = openAiToolCallsToBatch([ + { function: { name: "os__fs__list", arguments: "" } }, + ]); + expect(batch.calls[0]?.args).toEqual({}); + const whitespaceOnly = openAiToolCallsToBatch([ + { function: { name: "os__fs__list", arguments: " " } }, + ]); + expect(whitespaceOnly.calls[0]?.args).toEqual({}); + }); + + it("throws ToolCallArgumentsParseError on malformed non-empty JSON instead of substituting {}", () => { + expect(() => + openAiToolCallsToBatch([ + { function: { name: "os__fs__delete", arguments: '{"path":"widget.txt' } }, + ]), + ).toThrow(ToolCallArgumentsParseError); + }); + + it("throws on container-level truncated JSON instead of substituting {}", () => { + expect(() => + openAiToolCallsToBatch([ + { + function: { + name: "os__shell__run", + arguments: '{"commands":["npm install","npm test"', + }, + }, + ]), + ).toThrow(ToolCallArgumentsParseError); + }); + + it("throws when arguments parse to valid JSON that is not an object (array/primitive)", () => { + expect(() => + openAiToolCallsToBatch([ + { function: { name: "os__fs__delete", arguments: "[1,2,3]" } }, + ]), + ).toThrow(ToolCallArgumentsParseError); + expect(() => + openAiToolCallsToBatch([ + { function: { name: "os__fs__delete", arguments: "5" } }, + ]), + ).toThrow(ToolCallArgumentsParseError); + }); + + it("never includes the raw arguments string in the thrown error's message", () => { + const secret = '{"path":"/etc/shadow","token":"sk-super-secret-do-not-log'; + try { + openAiToolCallsToBatch([{ function: { name: "os__fs__delete", arguments: secret } }]); + expect.unreachable("expected a throw"); + } catch (err) { + expect(err).toBeInstanceOf(ToolCallArgumentsParseError); + expect((err as Error).message).not.toContain("sk-super-secret"); + expect((err as Error).message).not.toContain("/etc/shadow"); + } + }); + + it("valid object args still parse normally (control)", () => { + const batch = openAiToolCallsToBatch([ + { function: { name: "os__fs__read", arguments: '{"path":"a.txt"}' } }, + ]); + expect(batch.calls[0]?.args).toEqual({ path: "a.txt" }); + }); + it("includes reply and finish in descriptorsToOpenAiTools", () => { const tools = descriptorsToOpenAiTools([ { diff --git a/src/llm/provider/openai/openai-tool-call-adapter.ts b/src/llm/provider/openai/openai-tool-call-adapter.ts index d8ac0e5b..0bcf652a 100644 --- a/src/llm/provider/openai/openai-tool-call-adapter.ts +++ b/src/llm/provider/openai/openai-tool-call-adapter.ts @@ -97,18 +97,37 @@ export function descriptorsToOpenAiTools( return out; } +/** + * A tool call's `function.arguments` was non-empty but not valid JSON (or + * not a JSON object). Thrown rather than silently substituting `{}` so the + * failure reaches `tryParseToolCalls`'s existing catch block and routes + * through the same one-shot repair path grammar-parsed batches already + * use — never include the raw arguments here, they may carry sensitive + * user data and this message can reach logs. + */ +export class ToolCallArgumentsParseError extends Error { + constructor(toolName: string) { + super(`tool call "${toolName}" arguments are not a valid JSON object`); + this.name = "ToolCallArgumentsParseError"; + } +} + +/** + * Parses one tool call's raw argument string. A genuinely empty/whitespace + * string is a legitimate zero-arg call and maps to `{}`. Anything + * non-empty that fails to parse, or parses to something other than a JSON + * object, throws instead of falling back to `{}` — a truncated or + * malformed argument string must never be silently treated the same as an + * intentional empty call. + */ function parseArguments(raw: string): Record { const trimmed = raw.trim(); if (!trimmed) return {}; - try { - const parsed = JSON.parse(trimmed) as unknown; - if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { - return parsed as Record; - } - } catch { - // fall through + const parsed = JSON.parse(trimmed) as unknown; + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + return parsed as Record; } - return {}; + throw new SyntaxError("tool call arguments must be a JSON object"); } export function openAiToolCallsToBatch( @@ -118,9 +137,15 @@ export function openAiToolCallsToBatch( const calls: ToolCallPayload[] = []; for (const tc of toolCalls) { const name = nameUnescape(tc.function.name); + let args: Record; + try { + args = parseArguments(tc.function.arguments); + } catch { + throw new ToolCallArgumentsParseError(name); + } calls.push({ tool: name, - args: parseArguments(tc.function.arguments), + args, ...(reasoningText ? { reasoning: reasoningText } : {}), }); } From 1c0c93cc01d26e6f6930fd74c46c405954e1a325 Mon Sep 17 00:00:00 2001 From: canblmz1 <116688414+canblmz1@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:42:39 +0300 Subject: [PATCH 2/5] fix(llm): apply termination safety after tagged adaptation --- src/llm/provider/openai/openai-provider.ts | 69 ++++++++++++++-------- 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/src/llm/provider/openai/openai-provider.ts b/src/llm/provider/openai/openai-provider.ts index 12cece9c..e20ae8e1 100644 --- a/src/llm/provider/openai/openai-provider.ts +++ b/src/llm/provider/openai/openai-provider.ts @@ -36,6 +36,11 @@ export interface OpenAiProviderOptions { apiKey: string; defaultChatModel: string; headers?: Record; + /** + * Header that carries the API key when the service does not accept + * `Authorization: Bearer`. See `openai-auth-headers.ts`. + */ + apiKeyHeader?: string; supportsVision?: boolean; supportsParallelTools?: boolean; supportsPromptCache?: boolean; @@ -46,6 +51,12 @@ export interface OpenAiProviderOptions { streamConsumer?: StreamConsumer; apiPathPrefix?: string; taggedToolCompatibility?: "qwen"; + /** + * Vendor-specific fields merged into every chat completion body. + * See `RESERVED_BODY_KEYS` in `openai-build-body.ts` for the keys + * this passthrough cannot override. + */ + extraBody?: Record; } export class OpenAiProvider implements LlmProvider { @@ -59,6 +70,7 @@ export class OpenAiProvider implements LlmProvider { private readonly defaultChatModel: string; private readonly apiPathPrefix: string; private readonly taggedToolCompatibility: "qwen" | undefined; + private readonly extraBody: Record | undefined; constructor(options: OpenAiProviderOptions) { this.id = options.id; @@ -79,10 +91,12 @@ export class OpenAiProvider implements LlmProvider { this.defaultChatModel = options.defaultChatModel; this.apiPathPrefix = normalizeApiPathPrefix(options.apiPathPrefix ?? "/v1"); this.taggedToolCompatibility = options.taggedToolCompatibility; + this.extraBody = options.extraBody; this.http = { baseUrl: normalizeOpenAiBaseUrl(options.baseUrl), apiKey: options.apiKey, extraHeaders: options.headers ?? {}, + ...(options.apiKeyHeader ? { apiKeyHeader: options.apiKeyHeader } : {}), requestTimeoutMs: options.requestTimeoutMs ?? 600_000, fetchImpl: options.fetchImpl ?? fetch, label: options.id, @@ -90,7 +104,7 @@ export class OpenAiProvider implements LlmProvider { } async complete(request: CompletionRequest): Promise { - const body = buildOpenAiChatBody(request, this.defaultChatModel, false); + const body = buildOpenAiChatBody(request, this.defaultChatModel, false, this.extraBody); const json = await openAiPostJson( this.http, `${this.apiPathPrefix}/chat/completions`, @@ -107,7 +121,7 @@ export class OpenAiProvider implements LlmProvider { async *completeStream( request: CompletionRequest, ): AsyncGenerator { - const body = buildOpenAiChatBody(request, this.defaultChatModel, true); + const body = buildOpenAiChatBody(request, this.defaultChatModel, true, this.extraBody); // Opening the stream (connect + status check) happens inside the // client's bounded retry, strictly before the first chunk exists. // From here on the stream is live and failures are terminal. @@ -146,14 +160,19 @@ export class OpenAiProvider implements LlmProvider { if (accumulatedReasoning.length > 0 && final.reasoningContent.length === 0) { final.reasoningContent = accumulatedReasoning; } - if (this.taggedToolCompatibility === "qwen") { - // Buffer-then-adapt: the tagged `` payload may be split - // across deltas, so adapt only the fully-buffered message. Text and - // reasoning deltas were already yielded above for live UX; the adapt - // seam just rewrites the final result (content → tool_calls). - return adaptQwenCompletionResult(final, request); - } - return final; + // Tagged Qwen calls are synthesized only after the stream has been + // fully buffered. Apply termination safety after that adaptation seam, + // so native and tagged calls are judged from the same final dispatchable + // tool-call set. A synthetic `finishReason: "tool_calls"` from the + // adapter is not evidence that the provider actually terminated cleanly. + const adaptedFinal = + this.taggedToolCompatibility === "qwen" + ? adaptQwenCompletionResult(final, request) + : final; + return applyToolCallTerminationSafety( + adaptedFinal, + streamFinal?.terminalObserved === true, + ); } async health(): Promise { @@ -219,22 +238,11 @@ function completionFromStreamFinal( totalTokens: 0, }; const finishReason = streamFinal?.finishReason ?? null; - // A native tool call whose stream ended without a trustworthy terminal - // signal (no explicit finish_reason, no [DONE]/parser terminal event — - // just the connection closing) is not a confirmed completion. Treating - // it as an ordinary `stop` would let a mid-stream-cut tool call reach - // dispatch indistinguishably from a genuinely finished one. This only - // applies when a tool call is actually pending — a plain-text response - // that races the same way already has its own `stop`/`no_stop` handling - // downstream and is left untouched here. - const hasPendingToolCalls = (streamFinal?.toolCalls?.length ?? 0) > 0; - const ambiguousToolCallTermination = - hasPendingToolCalls && streamFinal?.terminalObserved !== true; return { content: streamFinal?.content ?? accumulated, reasoningContent: streamFinal?.reasoningContent ?? accumulatedReasoning, - stop: finishReason !== "length" && !ambiguousToolCallTermination, - truncated: finishReason === "length" || ambiguousToolCallTermination, + stop: finishReason !== "length", + truncated: finishReason === "length", timing: { promptMs: 0, predictedMs: 0, @@ -249,3 +257,18 @@ function completionFromStreamFinal( finishReason, }; } + +function applyToolCallTerminationSafety( + result: CompletionResult, + terminalObserved: boolean, +): CompletionResult { + const hasToolCalls = (result.toolCalls?.length ?? 0) > 0; + if (!hasToolCalls || terminalObserved || result.truncated) { + return result; + } + return { + ...result, + stop: false, + truncated: true, + }; +} From 315654c173155568be61e2db3674e8bc0b3bd0d7 Mon Sep 17 00:00:00 2001 From: canblmz1 <116688414+canblmz1@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:43:08 +0300 Subject: [PATCH 3/5] fix(llm): flush final undelimited SSE event at EOF --- .../provider/openai/openai-stream-consumer.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/llm/provider/openai/openai-stream-consumer.ts b/src/llm/provider/openai/openai-stream-consumer.ts index 573e611a..1b72cfdc 100644 --- a/src/llm/provider/openai/openai-stream-consumer.ts +++ b/src/llm/provider/openai/openai-stream-consumer.ts @@ -50,12 +50,19 @@ export function createOpenAiStreamConsumer( while (true) { if (signal?.aborted) break; const { done, value } = await reader.read(); - if (done) break; - buffer += decoder.decode(value, { stream: true }); + if (done) { + // Flush TextDecoder state and treat a final non-empty SSE event + // as an implicit last boundary. Some providers/proxies close the + // response immediately after the terminal event instead of + // writing the conventional trailing blank line. + buffer += decoder.decode(); + } else { + buffer += decoder.decode(value, { stream: true }); + } let boundary = buffer.indexOf("\n\n"); - while (boundary >= 0) { - const rawEvent = buffer.slice(0, boundary); - buffer = buffer.slice(boundary + 2); + while (boundary >= 0 || (done && buffer.trim().length > 0)) { + const rawEvent = boundary >= 0 ? buffer.slice(0, boundary) : buffer; + buffer = boundary >= 0 ? buffer.slice(boundary + 2) : ""; const chunk = parseOpenAiSseEvent(rawEvent, reasoning, toolArgsBuffer); content += chunk.delta; reasoningContent += chunk.reasoningDelta; @@ -103,6 +110,7 @@ export function createOpenAiStreamConsumer( } boundary = buffer.indexOf("\n\n"); } + if (done) break; } } finally { reader.releaseLock(); From 94eac428d5199627040492ccbf9b33867e21a532 Mon Sep 17 00:00:00 2001 From: canblmz1 <116688414+canblmz1@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:43:28 +0300 Subject: [PATCH 4/5] fix(llm): preserve non-parse tool-call errors --- src/llm/provider/openai/openai-tool-call-adapter.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/llm/provider/openai/openai-tool-call-adapter.ts b/src/llm/provider/openai/openai-tool-call-adapter.ts index 0bcf652a..76e0cbb7 100644 --- a/src/llm/provider/openai/openai-tool-call-adapter.ts +++ b/src/llm/provider/openai/openai-tool-call-adapter.ts @@ -140,8 +140,11 @@ export function openAiToolCallsToBatch( let args: Record; try { args = parseArguments(tc.function.arguments); - } catch { - throw new ToolCallArgumentsParseError(name); + } catch (err) { + if (err instanceof SyntaxError) { + throw new ToolCallArgumentsParseError(name); + } + throw err; } calls.push({ tool: name, From 49723cb778a9af544af806ec6920ed3b5655e13e Mon Sep 17 00:00:00 2001 From: canblmz1 <116688414+canblmz1@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:44:39 +0300 Subject: [PATCH 5/5] test(llm): cover tagged calls and EOF tail handling --- ...tive-tool-call-execution-integrity.test.ts | 189 ++++++++++++++---- 1 file changed, 151 insertions(+), 38 deletions(-) diff --git a/src/agent/native-tool-call-execution-integrity.test.ts b/src/agent/native-tool-call-execution-integrity.test.ts index d5942f47..1c68d8d9 100644 --- a/src/agent/native-tool-call-execution-integrity.test.ts +++ b/src/agent/native-tool-call-execution-integrity.test.ts @@ -31,10 +31,19 @@ const tools: NonNullable = [ { type: "function", function: { name: "os__fs__delete", parameters: { type: "object", properties: {} } } }, ]; +const parallelTools: NonNullable = [ + { type: "function", function: { name: "os__fs__read", parameters: { type: "object", properties: {} } } }, + { type: "function", function: { name: "os__fs__grep", parameters: { type: "object", properties: {} } } }, +]; + function sseFrame(obj: Record): string { return `data: ${JSON.stringify(obj)}\n\n`; } +function sseTail(obj: Record): string { + return `data: ${JSON.stringify(obj)}`; +} + /** Streams one tool call's arguments, then the connection just ends — no * finish_reason chunk, no `[DONE]`. */ function eofBody(toolCallArgs: string, toolName = "os__fs__delete"): string { @@ -52,6 +61,55 @@ function eofBody(toolCallArgs: string, toolName = "os__fs__delete"): string { ); } +function parallelEofBody(): string { + return ( + sseFrame({ + choices: [ + { + index: 0, + delta: { + role: "assistant", + tool_calls: [ + { index: 0, id: "call_a", type: "function", function: { name: "os__fs__read", arguments: "" } }, + { index: 1, id: "call_b", type: "function", function: { name: "os__fs__grep", arguments: "" } }, + ], + }, + finish_reason: null, + }, + ], + }) + + sseFrame({ + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { index: 0, function: { arguments: '{"path":"a.txt"}' } }, + { index: 1, function: { arguments: '{"path":"b.txt' } }, + ], + }, + finish_reason: null, + }, + ], + }) + ); +} + +function qwenTaggedBody(finishReason: string | null = null): string { + return sseFrame({ + choices: [ + { + index: 0, + delta: { + role: "assistant", + content: "", + }, + finish_reason: finishReason, + }, + ], + }); +} + function healthyBody(toolCallArgs: string, toolName = "os__fs__delete"): string { return ( eofBody(toolCallArgs, toolName) + @@ -85,9 +143,27 @@ function fetchErroringMidStream(prefixBody: string) { }) as unknown as typeof fetch; } -async function drainCompleteStream(fetchImpl: typeof fetch): Promise { - const provider = new OpenAiProvider({ id: "test", baseUrl: "https://example.invalid", apiKey: "", defaultChatModel: "m", fetchImpl }); - const gen = provider.completeStream({ prompt: "delete widget.txt", tools }); +async function drainCompleteStream( + fetchImpl: typeof fetch, + options?: { + taggedToolCompatibility?: "qwen"; + requestTools?: NonNullable; + }, +): Promise { + const provider = new OpenAiProvider({ + id: "test", + baseUrl: "https://example.invalid", + apiKey: "", + defaultChatModel: "m", + fetchImpl, + ...(options?.taggedToolCompatibility + ? { taggedToolCompatibility: options.taggedToolCompatibility } + : {}), + }); + const gen = provider.completeStream({ + prompt: "delete widget.txt", + tools: options?.requestTools ?? tools, + }); let next = await gen.next(); while (!next.done) next = await gen.next(); return next.value; @@ -151,8 +227,12 @@ async function runStepThroughLlmComplete( async function runStepFromFetch( fetchImpl: typeof fetch, registerTools: (registry: ToolRegistry) => void, + options?: { + taggedToolCompatibility?: "qwen"; + requestTools?: NonNullable; + }, ): Promise<{ outcomeOrError: unknown; completion: CompletionResult }> { - const completion = await drainCompleteStream(fetchImpl); + const completion = await drainCompleteStream(fetchImpl, options); const { outcomeOrError } = await runStepThroughLlmComplete(async () => completion, registerTools); return { outcomeOrError, completion }; } @@ -228,40 +308,21 @@ describe("native tool-call execution integrity", () => { expect(outcomeOrError).toBeInstanceOf(Error); }); - it("7. parallel calls (properly resource-classed tools): A complete + B truncated, ambiguous EOF -> neither executes", async () => { - // os.fs.read and os.fs.grep are both real, registered `pure_read` - // entries in tool-resource-class.ts, so this batch is accepted by - // validateBatch on its own merits (not rejected as "no resource - // class") — isolating the truncation-safety question cleanly. - // - // truncated:true / stop:false / finishReason:null below is exactly - // what completionFromStreamFinal() now derives for an ambiguous EOF - // with pending tool calls (see openai-provider.ts). Built directly - // here rather than via a real fetch so this test's focus stays on - // the step-executor's per-call handling once that flag is set. + it("7. parallel calls: provider derives ambiguous EOF and neither call executes", async () => { const toolA = countingTool("os.fs.read"); const toolB = countingTool("os.fs.grep"); - const completion: CompletionResult = { - content: "", - reasoningContent: "", - stop: false, - truncated: true, - timing: { promptMs: 0, predictedMs: 0, promptTokens: 1, predictedTokens: 1 }, - cacheHitTokens: 0, - slotId: -1, - modelId: "m", - usage: { promptTokens: 1, completionTokens: 1, totalTokens: 2 }, - toolCalls: [ - { id: "call_a", type: "function", function: { name: "os__fs__read", arguments: '{"path":"a.txt"}' } }, - { id: "call_b", type: "function", function: { name: "os__fs__grep", arguments: '{"path":"b.txt' } }, - ], - finishReason: null, - }; - - const { outcomeOrError } = await runStepThroughLlmComplete(async () => completion, (registry) => { - toolA.register(registry); - toolB.register(registry); - }); + const { outcomeOrError, completion } = await runStepFromFetch( + fetchReturning(parallelEofBody()), + (registry) => { + toolA.register(registry); + toolB.register(registry); + }, + { requestTools: parallelTools }, + ); + expect(completion.toolCalls).toHaveLength(2); + expect(completion.finishReason).toBeNull(); + expect(completion.truncated).toBe(true); + expect(completion.stop).toBe(false); expect(toolA.executions()).toBe(0); expect(toolB.executions()).toBe(0); expect(outcomeOrError).toBeInstanceOf(Error); @@ -297,12 +358,64 @@ describe("native tool-call execution integrity", () => { }); it("10. compatibility: plain text-only response with ambiguous EOF is unaffected by the tool-call fix", async () => { - // No tool_calls at all — hasPendingToolCalls is false, so this fix's - // truncated/stop computation must not touch this path at all. const textOnlyEofBody = sseFrame({ choices: [{ index: 0, delta: { role: "assistant", content: "hello" }, finish_reason: null }] }); const completion = await drainCompleteStream(fetchReturning(textOnlyEofBody)); expect(completion.toolCalls).toBeUndefined(); expect(completion.truncated).toBe(false); expect(completion.stop).toBe(true); }); + + it("11. qwen tagged tool call with ambiguous EOF is fail-closed after adaptation", async () => { + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch( + fetchReturning(qwenTaggedBody()), + tool.register, + { taggedToolCompatibility: "qwen" }, + ); + expect(completion.toolCalls).toHaveLength(1); + expect(completion.finishReason).toBe("tool_calls"); + expect(completion.truncated).toBe(true); + expect(completion.stop).toBe(false); + expect(tool.executions()).toBe(0); + expect(outcomeOrError).toBeInstanceOf(Error); + }); + + it("12. qwen tagged tool call with explicit terminal finish reason still executes", async () => { + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch( + fetchReturning(qwenTaggedBody("stop")), + tool.register, + { taggedToolCompatibility: "qwen" }, + ); + expect(completion.toolCalls).toHaveLength(1); + expect(completion.finishReason).toBe("tool_calls"); + expect(completion.truncated).toBe(false); + expect(completion.stop).toBe(true); + expect(tool.executions()).toBe(1); + expect(outcomeOrError).not.toBeInstanceOf(Error); + }); + + it("13. final finish_reason event without trailing blank line is flushed at EOF", async () => { + const body = + eofBody('{"path":"widget.txt"}') + + sseTail({ choices: [{ index: 0, delta: {}, finish_reason: "tool_calls" }], usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 } }); + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch(fetchReturning(body), tool.register); + expect(completion.finishReason).toBe("tool_calls"); + expect(completion.truncated).toBe(false); + expect(completion.stop).toBe(true); + expect(tool.executions()).toBe(1); + expect(outcomeOrError).not.toBeInstanceOf(Error); + }); + + it("14. final [DONE] event without trailing blank line is flushed at EOF", async () => { + const body = eofBody('{"path":"widget.txt"}') + "data: [DONE]"; + const tool = countingTool("os.fs.delete"); + const { outcomeOrError, completion } = await runStepFromFetch(fetchReturning(body), tool.register); + expect(completion.finishReason).toBeNull(); + expect(completion.truncated).toBe(false); + expect(completion.stop).toBe(true); + expect(tool.executions()).toBe(1); + expect(outcomeOrError).not.toBeInstanceOf(Error); + }); });