From a8523bcecd0e46fa35b53d1c0bbc1cfa504eae35 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 31 Jul 2026 05:25:46 -0700 Subject: [PATCH] fix(agents): surface malformed tools and approval delivery failures (#116831) Co-authored-by: Peter Steinberger --- .../agent-tool-definition-adapter.test.ts | 66 +++++++++++++++-- src/agents/agent-tool-definition-adapter.ts | 70 ++++++++++++------- ...ded-agent-subscribe.handlers.tools.test.ts | 52 +++++++++++++- ...embedded-agent-subscribe.handlers.tools.ts | 23 ++++-- .../reply/agent-runner-embedded-candidate.ts | 31 ++++---- .../agent-runner-execution-results.test.ts | 45 ++++++++++++ 6 files changed, 233 insertions(+), 54 deletions(-) diff --git a/src/agents/agent-tool-definition-adapter.test.ts b/src/agents/agent-tool-definition-adapter.test.ts index 9a0279c95152..29fea7a48733 100644 --- a/src/agents/agent-tool-definition-adapter.test.ts +++ b/src/agents/agent-tool-definition-adapter.test.ts @@ -421,10 +421,19 @@ describe("toClientToolDefinitions โ€“ param coercion", () => { expect(calledWith).toEqual({ query: "hello" }); }); - it("falls back to empty object for invalid JSON string", async () => { - const { calledWith } = await executeClientTool("not-json"); - expect(calledWith).toStrictEqual({}); - }); + it.each(["not-json", "[1,2,3]", "42", '"query"'])( + "returns a visible error instead of dispatching malformed client arguments: %s", + async (params) => { + const { calledWith, result } = await executeClientTool(params); + expect(calledWith).toBeUndefined(); + expect(result.details).toMatchObject({ + status: "error", + tool: "search", + error: expect.stringContaining("client tool arguments"), + }); + expect(result.terminate).not.toBe(true); + }, + ); it("falls back to empty object for empty string", async () => { const { calledWith } = await executeClientTool(""); @@ -441,9 +450,52 @@ describe("toClientToolDefinitions โ€“ param coercion", () => { expect(calledWith).toStrictEqual({}); }); - it("falls back to empty object for a JSON array string", async () => { - const { calledWith } = await executeClientTool("[1,2,3]"); - expect(calledWith).toStrictEqual({}); + it.each([null, undefined, "", {}])( + "rejects missing required client arguments without reserving a completed call: %s", + async (params) => { + const clientTool = makeClientTool("search"); + clientTool.function.parameters = { + type: "object", + properties: { query: { type: "string" } }, + required: ["query"], + }; + const reserve = vi.fn(); + const complete = vi.fn(); + const discard = vi.fn(); + const [definition] = toClientToolDefinitions([clientTool], { reserve, complete, discard }); + const result = await expectDefined(definition, "client tool definition").execute( + "call-required-client-args", + params, + undefined, + undefined, + extensionContext, + ); + + expect(result.details).toMatchObject({ + status: "error", + error: expect.stringContaining("query"), + }); + expect(result.terminate).not.toBe(true); + expect(complete).not.toHaveBeenCalled(); + expect(discard).toHaveBeenCalledWith("call-required-client-args", "search"); + }, + ); + + it("keeps absent arguments valid for a parameterless client tool", async () => { + const clientTool = makeClientTool("ping"); + clientTool.function.parameters = { type: "object", properties: {} }; + const complete = vi.fn(); + const [definition] = toClientToolDefinitions([clientTool], { complete }); + const result = await expectDefined(definition, "client tool definition").execute( + "call-parameterless-client-tool", + undefined, + undefined, + undefined, + extensionContext, + ); + + expect(complete).toHaveBeenCalledWith("call-parameterless-client-tool", "ping", {}); + expect(result.terminate).toBe(true); }); it("handles nested JSON string correctly", async () => { diff --git a/src/agents/agent-tool-definition-adapter.ts b/src/agents/agent-tool-definition-adapter.ts index c573ea57d3ac..fc737a456cb6 100644 --- a/src/agents/agent-tool-definition-adapter.ts +++ b/src/agents/agent-tool-definition-adapter.ts @@ -30,7 +30,7 @@ import type { ClientToolDefinition } from "./embedded-agent-runner/run/params.js import type { AgentTool, AgentToolResult, AgentToolUpdateCallback } from "./runtime/index.js"; import type { ToolDefinition } from "./sessions/index.js"; import { normalizeToolName } from "./tool-policy.js"; -import { jsonResult, payloadTextResult } from "./tools/common.js"; +import { jsonResult, payloadTextResult, ToolInputError } from "./tools/common.js"; type AnyAgentTool = AgentTool; @@ -447,36 +447,48 @@ export function toToolDefinitions( }); } -/** - * Coerce tool-call params into a plain object. - * - * Some providers (e.g. Gemini) stream tool-call arguments as incremental - * string deltas. By the time the framework invokes the tool's `execute` - * callback the accumulated value may still be a JSON **string** rather than - * a parsed object. `isPlainObject()` returns `false` for strings, which - * caused the params to be silently replaced with `{}`. - * - * This helper tries `JSON.parse` when the value is a string and falls back - * to an empty object only when parsing genuinely fails. - */ -function coerceParamsRecord(value: unknown): Record { +function coerceParamsRecord( + value: unknown, + schema: ClientToolDefinition["function"]["parameters"], +): Record { + let record: Record; if (isPlainObject(value)) { - return value; - } - if (typeof value === "string") { + record = value; + } else if (value === undefined || value === null) { + record = {}; + } else if (typeof value === "string") { const trimmed = value.trim(); - if (trimmed.length > 0) { + if (!trimmed) { + record = {}; + } else { + let parsed: unknown; try { - const parsed: unknown = JSON.parse(trimmed); - if (isPlainObject(parsed)) { - return parsed; - } + parsed = JSON.parse(trimmed); } catch { - // not valid JSON โ€“ fall through to empty object + throw new ToolInputError("Invalid client tool arguments: expected a JSON object"); + } + if (parsed === null) { + record = {}; + } else if (isPlainObject(parsed)) { + record = parsed; + } else { + throw new ToolInputError("Invalid client tool arguments: expected a JSON object"); } } + } else { + throw new ToolInputError("Invalid client tool arguments: expected a JSON object"); } - return {}; + + const required = Array.isArray(schema?.required) + ? schema.required.filter((key): key is string => typeof key === "string") + : []; + const missing = required.filter((key) => !Object.hasOwn(record, key)); + if (missing.length > 0) { + throw new ToolInputError( + `Invalid client tool arguments: missing required ${missing.join(", ")}`, + ); + } + return record; } /** Convert client-hosted tools into pending session definitions. */ @@ -497,8 +509,8 @@ export function toClientToolDefinitions( if (onClientToolCall && typeof onClientToolCall !== "function") { onClientToolCall.reserve?.(toolCallId, func.name); } - const initialParamsRecord = coerceParamsRecord(params); try { + const initialParamsRecord = coerceParamsRecord(params, func.parameters); const outcome = await runBeforeToolCallHook({ toolName: func.name, params: initialParamsRecord, @@ -521,7 +533,7 @@ export function toClientToolDefinitions( throw new Error(outcome.reason); } const adjustedParams = outcome.params; - const paramsRecord = coerceParamsRecord(adjustedParams); + const paramsRecord = coerceParamsRecord(adjustedParams, func.parameters); // Client-hosted tools have no tool-owned finalizer, so hook reconciliation // produces the canonical execution shape consumed here. const voiceConfirmation = consumeFinalClientVoiceToolConfirmation({ @@ -552,6 +564,12 @@ export function toClientToolDefinitions( if (onClientToolCall && typeof onClientToolCall !== "function") { onClientToolCall.discard?.(toolCallId, func.name); } + if (err instanceof ToolInputError) { + return buildToolExecutionErrorResult({ + toolName: func.name, + message: err.message, + }); + } throw err; } // Return a terminal pending result; the client will execute the tool. diff --git a/src/agents/embedded-agent-subscribe.handlers.tools.test.ts b/src/agents/embedded-agent-subscribe.handlers.tools.test.ts index 9b969e994dfb..15dc7a5511ca 100644 --- a/src/agents/embedded-agent-subscribe.handlers.tools.test.ts +++ b/src/agents/embedded-agent-subscribe.handlers.tools.test.ts @@ -2211,8 +2211,8 @@ describe("handleToolExecutionEnd exec approval prompts", () => { expect(ctx.state.deterministicApprovalPromptSent).toBe(true); }); - it("does not suppress assistant output when deterministic prompt delivery rejects", async () => { - const { ctx } = createTestContext(); + it("records an actionable failure when deterministic approval delivery rejects", async () => { + const { ctx, warn } = createTestContext(); ctx.params.onToolResult = vi.fn(async () => { throw new Error("delivery failed"); }); @@ -2235,6 +2235,54 @@ describe("handleToolExecutionEnd exec approval prompts", () => { }); expect(ctx.state.deterministicApprovalPromptSent).toBe(false); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("failed to deliver exec approval prompt: delivery failed"), + ); + expect(ctx.state.lastToolError).toMatchObject({ + toolName: "exec", + error: "Approval prompt delivery failed: delivery failed", + mutatingAction: false, + }); + const payloads = buildEmbeddedRunPayloads({ + assistantTexts: [], + toolMetas: requirePayloadToolMetas(ctx.state.toolMetas), + lastAssistant: undefined, + lastToolError: ctx.state.lastToolError, + sessionKey: "agent:unit-session", + toolResultFormat: "markdown", + inlineToolResultsAllowed: false, + }); + expect(payloads[0]?.text).toContain("approval prompt delivery"); + }); + + it("records an actionable failure when unavailable-approval notice delivery rejects", async () => { + const { ctx, warn } = createTestContext(); + ctx.params.onToolResult = vi.fn(async () => { + throw new Error("notice delivery failed"); + }); + + await endTool(ctx, { + toolName: "exec", + toolCallId: "tool-exec-unavailable-reject", + isError: false, + result: { + details: { + status: "approval-unavailable", + reason: "no-approval-route", + channelLabel: "Discord", + }, + }, + }); + + expect(ctx.state.deterministicApprovalPromptSent).toBe(false); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("failed to deliver exec approval prompt: notice delivery failed"), + ); + expect(ctx.state.lastToolError).toMatchObject({ + toolName: "exec", + error: "Approval prompt delivery failed: notice delivery failed", + mutatingAction: false, + }); }); it("emits approval + blocked command item events when exec needs approval", async () => { diff --git a/src/agents/embedded-agent-subscribe.handlers.tools.ts b/src/agents/embedded-agent-subscribe.handlers.tools.ts index 52aa4dd2525f..b40868f82161 100644 --- a/src/agents/embedded-agent-subscribe.handlers.tools.ts +++ b/src/agents/embedded-agent-subscribe.handlers.tools.ts @@ -861,6 +861,21 @@ async function emitToolResultOutput(params: { sanitizedResult: unknown; }) { const { ctx, toolName, rawToolName, meta, isToolError, result, sanitizedResult } = params; + const recordApprovalPromptDeliveryFailure = (error: unknown) => { + const message = error instanceof Error ? error.message : String(error); + ctx.log.warn(`failed to deliver exec approval prompt: ${message}`); + const approvalMeta = meta ? `${meta} ยท approval prompt delivery` : "approval prompt delivery"; + ctx.state.lastToolError = ( + ctx.params.observeToolTerminal ?? resolveFallbackToolTerminalObserver(ctx) + )({ + toolName, + meta: approvalMeta, + executionStarted: false, + outcome: "failure", + failure: { error: `Approval prompt delivery failed: ${message}` }, + }).lastToolError; + ctx.state.deterministicApprovalPromptSent = false; + }; const hasStructuredMedia = Boolean( result && typeof result === "object" && @@ -893,8 +908,8 @@ async function emitToolResultOutput(params: { }), ); ctx.state.deterministicApprovalPromptSent = true; - } catch { - ctx.state.deterministicApprovalPromptSent = false; + } catch (error) { + recordApprovalPromptDeliveryFailure(error); } finally { ctx.state.deterministicApprovalPromptPending = false; } @@ -922,8 +937,8 @@ async function emitToolResultOutput(params: { }), ); ctx.state.deterministicApprovalPromptSent = true; - } catch { - ctx.state.deterministicApprovalPromptSent = false; + } catch (error) { + recordApprovalPromptDeliveryFailure(error); } finally { ctx.state.deterministicApprovalPromptPending = false; } diff --git a/src/auto-reply/reply/agent-runner-embedded-candidate.ts b/src/auto-reply/reply/agent-runner-embedded-candidate.ts index 8e5d8c098f7f..99115170c39a 100644 --- a/src/auto-reply/reply/agent-runner-embedded-candidate.ts +++ b/src/auto-reply/reply/agent-runner-embedded-candidate.ts @@ -354,25 +354,26 @@ export async function runEmbeddedFallbackCandidate(params: { // Serialized delivery preserves tool result order across detached callbacks. let toolResultChain: Promise = Promise.resolve(); return (payload: ReplyPayload) => { - toolResultChain = toolResultChain - .then(async () => { - turn.replyOperation?.recordActivity(); - const { text, skip } = params.presentation.normalizeStreamingText(payload); - if (skip) { - return; - } - if (text !== undefined) { - await turn.typingSignals.signalTextDelta(text); - } - await turn.opts?.onToolResult?.({ ...payload, text }); - }) - .catch((err: unknown) => { - logVerbose(`tool result delivery failed: ${String(err)}`); - }); + const delivery = toolResultChain.then(async () => { + turn.replyOperation?.recordActivity(); + const { text, skip } = params.presentation.normalizeStreamingText(payload); + if (skip) { + return; + } + if (text !== undefined) { + await turn.typingSignals.signalTextDelta(text); + } + await turn.opts?.onToolResult?.({ ...payload, text }); + }); + // Keep later results best-effort while exposing this delivery to awaiting owners. + toolResultChain = delivery.catch((err: unknown) => { + logVerbose(`tool result delivery failed: ${String(err)}`); + }); const task = toolResultChain.finally(() => { turn.pendingToolTasks.delete(task); }); turn.pendingToolTasks.add(task); + return delivery; }; })() : undefined, diff --git a/src/auto-reply/reply/agent-runner-execution-results.test.ts b/src/auto-reply/reply/agent-runner-execution-results.test.ts index ab256aa4b1bd..6208d75d84c4 100644 --- a/src/auto-reply/reply/agent-runner-execution-results.test.ts +++ b/src/auto-reply/reply/agent-runner-execution-results.test.ts @@ -480,6 +480,51 @@ describe("executeAgentTurn: result and tool delivery", () => { expect(delivered).toEqual(["second"]); }); + it.each([ + { + label: "typed approval prompt", + payload: { + text: "Approval required.", + channelData: { + execApproval: { + approvalId: "approval-1", + approvalSlug: "approval", + }, + }, + }, + }, + { + label: "unavailable approval notice", + payload: { + text: "Exec approval is required, but no interactive approval client is currently available.", + }, + }, + ])( + "propagates rejected $label delivery while preserving later best-effort results", + async ({ payload }) => { + const delivered: string[] = []; + const onToolResult = vi.fn(async (result: { text?: string }) => { + if (result.text !== "later") { + throw new Error("delivery failed"); + } + delivered.push(result.text); + }); + state.runEmbeddedAgentMock.mockImplementationOnce(async (params: EmbeddedAgentParams) => { + await expect(params.onToolResult?.(payload)).rejects.toThrow("delivery failed"); + await expect(params.onToolResult?.({ text: "later" })).resolves.toBeUndefined(); + return { payloads: [{ text: "final" }], meta: {} }; + }); + + const executeAgentTurn = await getExecuteAgentTurnForTest(); + const input = createMinimalRunAgentTurnParams({ opts: { onToolResult } }); + const result = await executeAgentTurn(input); + await Promise.all(input.pendingToolTasks); + + expect(result.kind).toBe("success"); + expect(delivered).toEqual(["later"]); + }, + ); + it("delivers streamed tool results in callback order even when dispatch latency differs", async () => { const deliveryOrder: string[] = []; const onToolResult = vi.fn(async (payload: { text?: string }) => {