fix(agents): surface malformed tools and approval delivery failures (#116831)

Co-authored-by: Peter Steinberger <steipete@macos.shared>

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 () => {

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<string, unknown> {
+function coerceParamsRecord(
+  value: unknown,
+  schema: ClientToolDefinition["function"]["parameters"],
+): Record<string, unknown> {
+  let record: Record<string, unknown>;
   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.

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 () => {

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;
     }

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<void> = 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,

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 }) => {