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 0000000..1c68d8d --- /dev/null +++ b/src/agent/native-tool-call-execution-integrity.test.ts @@ -0,0 +1,421 @@ +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: {} } } }, +]; + +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 { + 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 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) + + 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, + 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; +} + +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, + options?: { + taggedToolCompatibility?: "qwen"; + requestTools?: NonNullable; + }, +): Promise<{ outcomeOrError: unknown; completion: CompletionResult }> { + const completion = await drainCompleteStream(fetchImpl, options); + 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: provider derives ambiguous EOF and neither call executes", async () => { + const toolA = countingTool("os.fs.read"); + const toolB = countingTool("os.fs.grep"); + 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); + }); + + 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 () => { + 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); + }); +}); diff --git a/src/llm/provider/completion-types.ts b/src/llm/provider/completion-types.ts index d6cd564..6acc50d 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 974943f..e20ae8e 100644 --- a/src/llm/provider/openai/openai-provider.ts +++ b/src/llm/provider/openai/openai-provider.ts @@ -160,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 { @@ -252,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, + }; +} diff --git a/src/llm/provider/openai/openai-stream-consumer.ts b/src/llm/provider/openai/openai-stream-consumer.ts index a866e3c..1b72cfd 100644 --- a/src/llm/provider/openai/openai-stream-consumer.ts +++ b/src/llm/provider/openai/openai-stream-consumer.ts @@ -37,20 +37,36 @@ 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) { 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; + if (chunk.finishReason !== null) terminalObserved = true; finishReason = chunk.finishReason ?? finishReason; modelId = chunk.modelId ?? modelId; usage = normaliseUsage(chunk.usage) ?? usage; @@ -64,6 +80,7 @@ export function createOpenAiStreamConsumer( modelId, usage, toolCalls, + terminalObserved: true, }); } if (chunk.toolArgsDelta !== undefined) { @@ -93,6 +110,7 @@ export function createOpenAiStreamConsumer( } boundary = buffer.indexOf("\n\n"); } + if (done) break; } } finally { reader.releaseLock(); @@ -105,6 +123,7 @@ export function createOpenAiStreamConsumer( modelId, usage, toolCalls, + terminalObserved, }); }, }; @@ -135,6 +154,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 +165,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 a773464..ed1227d 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 d8ac0e5..76e0cbb 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,18 @@ 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 (err) { + if (err instanceof SyntaxError) { + throw new ToolCallArgumentsParseError(name); + } + throw err; + } calls.push({ tool: name, - args: parseArguments(tc.function.arguments), + args, ...(reasoningText ? { reasoning: reasoningText } : {}), }); }