fix: fail turns with unfinished tools (#1212)

Fixing silent agent turn completion in case of tools failure

src/acp-agent.ts

@@ -590,6 +590,10 @@ type Turn = {
   /** uuid stamped on the pushed `SDKUserMessage`; the SDK echoes it back so the
    *  consumer can match the replayed user message to this turn. */
   promptUuid: string;
+  /** Tools surfaced during this turn, awaiting completion or an explicit
+   *  background handoff. Intersect with emittedToolCalls at settlement so
+   *  tool_result counts even when a presentation hook is still pending. */
+  foregroundToolCallIds?: Set<string>;
   /** Local-only slash commands (e.g. `/clear`) return a result without an echo,
    *  so the consumer can't promote them via the replay; it falls back to
    *  promoting the queue head when the result arrives. */
@@ -3681,6 +3685,26 @@ export class ClaudeAcpAgent {
         }
         return;
       }
+      if (
+        toolCallId &&
+        (update.sessionUpdate === "tool_call" || update.sessionUpdate === "tool_call_update")
+      ) {
+        if (
+          claudeMeta?.parentToolUseId ||
+          routedNotification.sessionId !== params.sessionId ||
+          update.status === "completed" ||
+          update.status === "failed"
+        ) {
+          // A later stream frame can attribute an eager permission call to a
+          // child. It must no longer count as this turn's foreground work.
+          forgetForegroundToolCall(session, toolCallId);
+        } else if (
+          update.sessionUpdate === "tool_call" &&
+          session.emittedToolCalls.has(toolCallId)
+        ) {
+          recordForegroundToolCall(session, toolCallId);
+        }
+      }
       if (update.sessionUpdate === "agent_message_chunk") {
         if (
           !claudeMeta?.parentToolUseId &&
@@ -4095,6 +4119,7 @@ export class ClaudeAcpAgent {
       isSubagent: boolean,
     ) => {
       session.liveBackgroundTasks.set(taskId, { parentToolUseId, isSubagent });
+      if (parentToolUseId) forgetForegroundToolCall(session, parentToolUseId);
       session.resumableSubagents?.delete(taskId);
       if (isSubagent && session.activeTurn && !session.activeTurn.settled) {
         (session.activeTurn.spawnedTaskIds ??= new Set()).add(taskId);
@@ -4186,6 +4211,62 @@ export class ClaudeAcpAgent {
       if (!turn || turn.settled || turn.settling) {
         return;
       }
+      // Both a result and EOF can end the turn here. Preserve cancellation
+      // and existing errors instead of replacing them with this check.
+      if (result.stopReason === "end_turn" && reportReason === "notReported") {
+        // A confirmed background task is allowed to outlive the turn.
+        const backgroundTools = new Set(
+          [...session.liveBackgroundTasks.values()].map((task) => task.parentToolUseId),
+        );
+        // Check only this turn's tools. A tool_result removes its id from
+        // emittedToolCalls even when its PostToolUse hook has not arrived yet.
+        const unfinished = [...(turn.foregroundToolCallIds ?? [])].filter(
+          (id) => session.emittedToolCalls.has(id) && !backgroundTools.has(id),
+        );
+        if (unfinished.length > 0) {
+          const message = `Claude ended the turn without returning results for tool calls: ${unfinished.join(", ")}`;
+          this.logger.error(
+            `Session ${params.sessionId}, turn ${turn.promptUuid}, stopReason=${result.stopReason}: ${message}`,
+          );
+          // Fail every unfinished tool before reporting one error for the prompt.
+          for (const toolCallId of unfinished) {
+            // A late hook must not overwrite the failure we are about to send.
+            unregisterHookCallback(toolCallId);
+            session.emittedToolCalls.delete(toolCallId);
+            delete session.toolUseCache[toolCallId];
+            session.toolCallFields?.delete(toolCallId);
+            await sendUpdate({
+              sessionId: params.sessionId,
+              update: {
+                sessionUpdate: "tool_call_update",
+                toolCallId,
+                status: "failed",
+                content: [
+                  {
+                    type: "content",
+                    content: {
+                      type: "text",
+                      text: "Claude ended the turn without returning a result for this tool.",
+                    },
+                  },
+                ],
+              },
+            });
+            // Cancellation can arrive while we await the client update.
+            if (turn.settled || session.activeTurn !== turn) return;
+            if (session.cancelled) {
+              await settleActive({ ...result, stopReason: "cancelled" });
+              return;
+            }
+          }
+          await failActiveWithSessionFailure(
+            "internal_error",
+            RequestError.internalError(errorKindData("incomplete_tool_call"), message),
+            message,
+          );
+          return;
+        }
+      }
       turn.settling = true;
       turn.settlingOutcome = result;
       if (reportReason === "notReported") {
@@ -7643,6 +7724,15 @@ export class ClaudeAcpAgent {
       return;
     }
     session.emittedToolCalls.add(toolCallId);
+    // SandboxNetworkAccess is a permission-only callback, not an executing
+    // tool: the SDK does not send a tool_result for its synthetic id.
+    if (
+      !parentToolUseId &&
+      notificationSessionId === sessionId &&
+      toolName !== "SandboxNetworkAccess"
+    ) {
+      recordForegroundToolCall(session, toolCallId);
+    }
     (session.eagerToolCallSessions ??= new Map()).set(toolCallId, notificationSessionId);
     const update = new AcpToolCallRenderer(this.toolCallCapabilities).toolCall(
       { id: toolCallId, name: toolName, input: toolInput },
@@ -9495,7 +9585,7 @@ function totalTokens(usage: UsageSnapshot): number {
 /** Error kinds this adapter invents itself, alongside the SDK's categorical
  *  `SDKAssistantMessageError` kinds: `no_result` marks a turn the SDK declared
  *  over without ever emitting its result (issue #825). */
-type AgentErrorKind = SDKAssistantMessageError | "no_result";
+type AgentErrorKind = SDKAssistantMessageError | "no_result" | "incomplete_tool_call";
 
 /**
  * Build the `data` payload attached to a `RequestError.internalError` when we
@@ -10031,6 +10121,17 @@ function shouldEmitToolCall(toolName: string): boolean {
   return toolName !== "TodoWrite" && !isTaskTool(toolName);
 }
 
+/** Streamed and permission-surfaced tools can precede the SDK's user echo. */
+function recordForegroundToolCall(session: Session, toolCallId: string): void {
+  const turn = session.activeTurn ?? session.turnQueue?.find((queued) => !queued.settled);
+  if (turn && !turn.settled) (turn.foregroundToolCallIds ??= new Set()).add(toolCallId);
+}
+
+function forgetForegroundToolCall(session: Session, toolCallId: string): void {
+  session.activeTurn?.foregroundToolCallIds?.delete(toolCallId);
+  for (const turn of session.turnQueue ?? []) turn.foregroundToolCallIds?.delete(toolCallId);
+}
+
 /** The tool-call field tracker of a session, created on first use. */
 function toolCallFieldsOf(session: {
   toolCallFields?: ToolCallFieldTracker;

src/tests/incomplete-tools.test.ts

@@ -0,0 +1,321 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { ClaudeAcpAgent, type AcpClient } from "../acp-agent.js";
+import { clearHookCallbacks, createPostToolUseHook, hasHookCallback } from "../tools.js";
+import { Pushable } from "../utils.js";
+import {
+  mockSessionState,
+  successfulResultMessage,
+  userEcho,
+  wrapQuery,
+} from "./session-doubles.js";
+
+const sessionId = "test-session";
+const toolCallId = "unfinished-bash";
+
+afterEach(() => clearHookCallbacks(sessionId));
+
+describe("incomplete foreground tools", () => {
+  it.each(["result", "EOF"] as const)(
+    "fails the tool and prompt at %s even after the input stream closes",
+    async (ending) => {
+      const { prompt, updates, logError } = createTestSession((input) =>
+        unfinishedToolMessages(input, ending),
+      );
+
+      await expect(prompt()).rejects.toMatchObject({ data: { errorKind: "incomplete_tool_call" } });
+      expect(updates).toContainEqual(
+        expect.objectContaining({
+          sessionUpdate: "tool_call_update",
+          toolCallId,
+          status: "failed",
+        }),
+      );
+      expect(logError).toHaveBeenCalledExactlyOnceWith(
+        expect.stringMatching(
+          /Session test-session, turn .+, stopReason=end_turn:.*unfinished-bash/,
+        ),
+      );
+      const failed = updates.find((u) => u.toolCallId === toolCallId && u.status === "failed");
+      expect(failed.content[0].content.text).toContain("without returning a result");
+      expect(failed).not.toHaveProperty("rawOutput");
+      expect(hasHookCallback(toolCallId)).toBe(false);
+    },
+  );
+
+  it("detects a tool emitted before the user echo", async () => {
+    const { prompt } = createTestSession(toolBeforeUserEchoMessages);
+
+    await expect(prompt()).rejects.toMatchObject({ data: { errorKind: "incomplete_tool_call" } });
+  });
+
+  it("detects a permission tool whose streamed tool_use never arrived", async () => {
+    const { prompt, updates } = createTestSession(permissionOnlyToolMessages);
+
+    await expect(prompt()).rejects.toMatchObject({ data: { errorKind: "incomplete_tool_call" } });
+    expect(updates).toContainEqual(expect.objectContaining({ toolCallId, status: "failed" }));
+  });
+
+  it("allows a completed tool with a late PostToolUse hook", async () => {
+    const { prompt, logError } = createTestSession(completedToolMessages);
+
+    await expect(prompt()).resolves.toMatchObject({ stopReason: "end_turn" });
+    expect(hasHookCallback(toolCallId)).toBe(true);
+    await createPostToolUseHook()(
+      {
+        hook_event_name: "PostToolUse",
+        tool_name: "Bash",
+        tool_input: { command: "echo test" },
+        tool_response: { stdout: "test", stderr: "", interrupted: false },
+      } as any,
+      toolCallId,
+      { signal: new AbortController().signal },
+    );
+    expect(logError).not.toHaveBeenCalled();
+  });
+
+  it("allows a tool explicitly handed off to a background task", async () => {
+    const { prompt, updates } = createTestSession(backgroundToolMessages);
+
+    await expect(prompt()).resolves.toMatchObject({ stopReason: "end_turn" });
+    expect(updates.some((u) => u.toolCallId === toolCallId && u.status === "failed")).toBe(false);
+  });
+
+  it("does not claim a subagent's tool as a foreground tool", async () => {
+    const { prompt } = createTestSession(subagentToolMessages);
+
+    await expect(prompt()).resolves.toMatchObject({ stopReason: "end_turn" });
+  });
+
+  it("does not treat a request to run in the background as a confirmed handoff", async () => {
+    const { prompt } = createTestSession(unconfirmedBackgroundToolMessages);
+
+    await expect(prompt()).rejects.toMatchObject({ data: { errorKind: "incomplete_tool_call" } });
+  });
+
+  it("waits for deferred settlement before checking unfinished tools", async () => {
+    const { prompt, logError } = createTestSession(deferredSettlementMessages);
+
+    await expect(prompt()).resolves.toMatchObject({ stopReason: "end_turn" });
+    expect(logError).not.toHaveBeenCalled();
+  });
+
+  it("fails every unfinished tool and allows a later prompt to succeed", async () => {
+    const { prompt, updates, agent } = createTestSession(incompleteThenSuccessfulTurnMessages);
+
+    await expect(prompt()).rejects.toMatchObject({ data: { errorKind: "incomplete_tool_call" } });
+    expect(updates.filter((u) => u.status === "failed").map((u) => u.toolCallId)).toEqual([
+      toolCallId,
+      "second-tool",
+    ]);
+    expect(agent.sessions[sessionId].emittedToolCalls.size).toBe(0);
+    expect(agent.sessions[sessionId].toolUseCache).toEqual({});
+    await expect(prompt()).resolves.toMatchObject({ stopReason: "end_turn" });
+  });
+
+  it("preserves cancellation and does not attribute its unfinished tool to the next turn", async () => {
+    const { prompt, updates } = createTestSession(cancelledThenSuccessfulTurnMessages);
+
+    await expect(prompt()).resolves.toMatchObject({ stopReason: "cancelled" });
+    await expect(prompt()).resolves.toMatchObject({ stopReason: "end_turn" });
+    expect(updates.some((u) => u.toolCallId === toolCallId && u.status === "failed")).toBe(false);
+  });
+
+  it("preserves an existing SDK failure", async () => {
+    const { prompt } = createTestSession(sdkFailureMessages);
+
+    await expect(prompt()).rejects.toThrow("original failure");
+  });
+});
+
+function toolStart(id = toolCallId, parent: string | null = null) {
+  return {
+    type: "stream_event",
+    session_id: sessionId,
+    uuid: "stream-message",
+    parent_tool_use_id: parent,
+    event: {
+      type: "content_block_start",
+      index: 0,
+      content_block: { type: "tool_use", id, name: "Bash", input: {} },
+    },
+  };
+}
+
+function toolResult(id = toolCallId) {
+  return {
+    type: "user",
+    session_id: sessionId,
+    parent_tool_use_id: null,
+    message: {
+      role: "user",
+      content: [{ type: "tool_result", tool_use_id: id, content: "done" }],
+    },
+  };
+}
+
+function createTestSession(
+  createSdkMessages: (input: Pushable<any>, agent: ClaudeAcpAgent) => AsyncGenerator<any>,
+) {
+  const updates: any[] = [];
+  const logError = vi.fn();
+  const input = new Pushable<any>();
+  const agent = new ClaudeAcpAgent(
+    {
+      sessionUpdate: async (notification: any) => {
+        updates.push(notification.update);
+      },
+      requestPermission: async () => ({ outcome: { outcome: "selected", optionId: "allow-once" } }),
+    } as unknown as AcpClient,
+    { log: () => {}, error: logError },
+  );
+  agent.sessions[sessionId] = mockSessionState({
+    input,
+    // The real consumer reads the scripted SDK events from this generator.
+    query: wrapQuery(createSdkMessages(input, agent)),
+  });
+  const prompt = () => agent.prompt({ sessionId, prompt: [{ type: "text", text: "continue" }] });
+  return { agent, updates, logError, prompt };
+}
+
+// Wait for prompt() to submit input, then echo it as the SDK would.
+async function* echoNextPrompt(input: Pushable<any>) {
+  const { value } = await input[Symbol.asyncIterator]().next();
+  yield userEcho(value);
+}
+
+async function* unfinishedToolMessages(input: Pushable<any>, ending: "result" | "EOF") {
+  yield* echoNextPrompt(input);
+  yield toolStart();
+  yield {
+    ...toolStart(),
+    event: { type: "content_block_stop", index: 0 },
+  };
+  // The input block closed, but no tool_result arrived. Returning ends the stream.
+  if (ending === "result") yield successfulResultMessage();
+}
+
+async function* toolBeforeUserEchoMessages(input: Pushable<any>) {
+  const { value } = await input[Symbol.asyncIterator]().next();
+  yield toolStart();
+  yield userEcho(value);
+  yield successfulResultMessage();
+}
+
+async function* permissionOnlyToolMessages(input: Pushable<any>, agent: ClaudeAcpAgent) {
+  yield* echoNextPrompt(input);
+  await agent.canUseTool(sessionId)("Bash", { command: "echo test" }, {
+    toolUseID: toolCallId,
+    signal: new AbortController().signal,
+    suggestions: [],
+  } as any);
+  yield successfulResultMessage();
+}
+
+async function* completedToolMessages(input: Pushable<any>) {
+  yield* echoNextPrompt(input);
+  yield toolStart();
+  yield toolResult();
+  yield successfulResultMessage();
+}
+
+async function* backgroundToolMessages(input: Pushable<any>) {
+  yield* echoNextPrompt(input);
+  yield toolStart();
+  yield {
+    type: "system",
+    subtype: "task_started",
+    session_id: sessionId,
+    task_id: "background-shell",
+    tool_use_id: toolCallId,
+    description: "dev server",
+  };
+  yield successfulResultMessage();
+}
+
+async function* subagentToolMessages(input: Pushable<any>) {
+  yield* echoNextPrompt(input);
+  yield toolStart("child-tool", "parent-agent");
+  yield successfulResultMessage();
+}
+
+async function* unconfirmedBackgroundToolMessages(input: Pushable<any>) {
+  yield* echoNextPrompt(input);
+  yield {
+    type: "assistant",
+    parent_tool_use_id: null,
+    message: {
+      role: "assistant",
+      usage: successfulResultMessage().usage,
+      content: [
+        {
+          type: "tool_use",
+          id: toolCallId,
+          name: "Bash",
+          input: { command: "sleep 100", run_in_background: true },
+        },
+      ],
+    },
+  };
+  yield successfulResultMessage();
+}
+
+async function* deferredSettlementMessages(input: Pushable<any>) {
+  yield* echoNextPrompt(input);
+  yield toolStart();
+  yield {
+    type: "system",
+    subtype: "task_started",
+    session_id: sessionId,
+    task_id: "child",
+    tool_use_id: "parent-agent",
+    subagent_type: "Explore",
+    description: "investigate",
+  };
+  // The result arrives while the subagent is still running, so settlement waits.
+  yield successfulResultMessage();
+  yield toolResult();
+  yield {
+    type: "system",
+    subtype: "task_notification",
+    session_id: sessionId,
+    task_id: "child",
+    tool_use_id: "parent-agent",
+    status: "completed",
+    summary: "done",
+  };
+  yield { type: "system", subtype: "session_state_changed", state: "idle" };
+}
+
+async function* incompleteThenSuccessfulTurnMessages(input: Pushable<any>) {
+  const messages = input[Symbol.asyncIterator]();
+  yield userEcho((await messages.next()).value);
+  yield toolStart();
+  yield toolStart("second-tool");
+  yield successfulResultMessage();
+  yield { type: "system", subtype: "session_state_changed", state: "idle" };
+
+  // Keep the SDK stream open for the next prompt in the same session.
+  yield userEcho((await messages.next()).value);
+  yield successfulResultMessage();
+}
+
+async function* cancelledThenSuccessfulTurnMessages(input: Pushable<any>, agent: ClaudeAcpAgent) {
+  const messages = input[Symbol.asyncIterator]();
+  yield userEcho((await messages.next()).value);
+  yield toolStart();
+  await agent.cancel({ sessionId });
+  yield { type: "system", subtype: "session_state_changed", state: "idle" };
+
+  yield userEcho((await messages.next()).value);
+  yield successfulResultMessage();
+}
+
+async function* sdkFailureMessages(input: Pushable<any>) {
+  yield* echoNextPrompt(input);
+  yield toolStart();
+  yield successfulResultMessage({
+    subtype: "error_during_execution",
+    is_error: true,
+    errors: ["original failure"],
+  });
+}