From bed795d261620e6d5113e427a7196d94e66e7034 Mon Sep 17 00:00:00 2001 From: Nacho Lopez Date: Mon, 26 Oct 2026 11:28:57 +0100 Subject: [PATCH] fix: settle advisor tool results before completing turns (#1247) When Claude uses the advisor server tool, a completed turn can fail with `ACP error -32603: Internal error: Claude ended the turn without returning results for tool calls: srvtoolu_...`. `server_tool_use` is tracked as a foreground call, but the matching `advisor_tool_result` is discarded, leaving that call unfinished at settlement. Route advisor results through the existing tool-result handler so they clear the outstanding call and publish a terminal update. Plaintext advice is displayed, redacted advice gets a readable completion message without exposing ciphertext, and advisor error results publish a failed tool status with the error code. Release the advisor's hook callback immediately because the server tool has no later client-side `PostToolUse` hook. Addresses the result-lifecycle part of #914. Validation: - Regression coverage through the real prompt consumer for plaintext, redacted, and error results, including the complete assistant message that dispatches the advisor call. All three cases fail without the fix. A dispatched advisor call without a result still fails with `incomplete_tool_call`. - `npm run test:run -- src/tests/incomplete-tools.test.ts`: 59 passed. - `npm run test:run`: 3,055 passed, 30 skipped. - `npm run build` and `npm run check`: passed. - Exact-head Linux CI passed in the fork: https://github.com/mrmans0n/claude-agent-acp/actions/runs/38595756003. The upstream PR workflows are waiting for maintainer approval to run. The required upstream `Build` check remains pending; the fork CI result does not satisfy that merge gate. --- src/acp-agent.ts | 33 ++++++++- src/tests/incomplete-tools.test.ts | 108 +++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/src/acp-agent.ts b/src/acp-agent.ts index e4d615ec..4236bb04 100644 --- a/src/acp-agent.ts +++ b/src/acp-agent.ts @@ -13638,6 +13638,38 @@ export function toAcpNotifications( break; } + case "advisor_tool_result": { + // Advisor results arrive in assistant content, including encrypted advice. + // Resolve the server tool through the normal result path without rendering + // opaque ciphertext. Server tools have no later PostToolUse hook to await. + const result = chunk.content; + unregisterHookCallback(chunk.tool_use_id); + output.push( + ...toAcpNotifications( + [ + { + type: "tool_result", + tool_use_id: chunk.tool_use_id, + is_error: result.type === "advisor_tool_result_error", + content: + result.type === "advisor_result" + ? result.text + : result.type === "advisor_redacted_result" + ? "Advisor guidance applied server-side." + : `Advisor error: ${result.error_code}`, + }, + ], + role, + sessionId, + toolUseCache, + client, + logger, + options, + ), + ); + break; + } + case "tool_result": case "tool_search_tool_result": case "web_fetch_tool_result": @@ -13807,7 +13839,6 @@ export function toAcpNotifications( case "container_upload": case "compaction": case "compaction_delta": - case "advisor_tool_result": case "fallback": case "mcp_tool_listing": break; diff --git a/src/tests/incomplete-tools.test.ts b/src/tests/incomplete-tools.test.ts index 2d630f4c..84ff060b 100644 --- a/src/tests/incomplete-tools.test.ts +++ b/src/tests/incomplete-tools.test.ts @@ -97,6 +97,76 @@ describe("incomplete foreground tools", () => { expect(agent.sessions[sessionId].toolCallParents?.size ?? 0).toBe(0); }); + it.each([ + { + content: { type: "advisor_result", text: "Check the queue ordering." }, + status: "completed", + text: "Check the queue ordering.", + }, + { + content: { type: "advisor_redacted_result", encrypted_content: "opaque-advice" }, + status: "completed", + text: "Advisor guidance applied server-side.", + }, + { + content: { type: "advisor_tool_result_error", error_code: "overloaded" }, + status: "failed", + text: "Advisor error: overloaded", + }, + ] as const)( + "settles an advisor call with a $content.type result", + async ({ content, status, text }) => { + const advisorId = "srvtoolu_advisor"; + const { prompt, updates, logError, agent } = createTestSession(async function* (input) { + yield* echoNextPrompt(input); + yield advisorStart(advisorId); + yield advisorToolUse(advisorId); + yield advisorResult(advisorId, content); + expect(hasHookCallback(advisorId)).toBe(false); + yield successfulResultMessage(); + }); + + await expect(prompt()).resolves.toMatchObject({ stopReason: "end_turn" }); + const terminal = updates.filter( + (update) => + update.toolCallId === advisorId && + (update.status === "completed" || update.status === "failed"), + ); + expect(terminal).toHaveLength(1); + expect(terminal[0]).toMatchObject({ + sessionUpdate: "tool_call_update", + toolCallId: advisorId, + status, + }); + expect(JSON.stringify(terminal[0])).toContain(text); + expect(JSON.stringify(updates)).not.toContain("opaque-advice"); + expect(agent.sessions[sessionId].emittedToolCalls.has(advisorId)).toBe(false); + expect(agent.sessions[sessionId].dispatchedToolCalls?.has(advisorId)).toBe(false); + expect(logError).not.toHaveBeenCalled(); + }, + ); + + it("fails a dispatched advisor call without a result", async () => { + const advisorId = "srvtoolu_advisor"; + const { prompt, updates, agent } = createTestSession(async function* (input) { + yield* echoNextPrompt(input); + yield advisorStart(advisorId); + yield advisorToolUse(advisorId); + yield successfulResultMessage(); + }); + + await expect(prompt()).rejects.toMatchObject({ data: { errorKind: "incomplete_tool_call" } }); + const terminal = updates.filter( + (update) => + update.toolCallId === advisorId && + (update.status === "completed" || update.status === "failed"), + ); + expect(terminal).toHaveLength(1); + expect(terminal[0]).toMatchObject({ status: "failed" }); + expect(agent.sessions[sessionId].emittedToolCalls.has(advisorId)).toBe(false); + expect(agent.sessions[sessionId].dispatchedToolCalls?.has(advisorId)).toBe(false); + }); + it("allows a tool explicitly handed off to a background task", async () => { const { prompt, updates } = createTestSession(backgroundToolMessages); @@ -1123,6 +1193,44 @@ function toolUse( }; } +function advisorStart(id: string) { + return { + ...toolStart(id), + event: { + type: "content_block_start", + index: 0, + content_block: { type: "server_tool_use", id, name: "advisor", input: {} }, + }, + }; +} + +/** The complete assistant message that dispatches the advisor server tool. */ +function advisorToolUse(id: string) { + return { + type: "assistant", + session_id: sessionId, + parent_tool_use_id: null, + message: { + role: "assistant", + usage: successfulResultMessage().usage, + content: [{ type: "server_tool_use", id, name: "advisor", input: {} }], + }, + }; +} + +function advisorResult(id: string, content: unknown) { + return { + type: "assistant", + session_id: sessionId, + parent_tool_use_id: null, + message: { + role: "assistant", + usage: successfulResultMessage().usage, + content: [{ type: "advisor_tool_result", tool_use_id: id, content }], + }, + }; +} + function assistantText(text: string) { return { type: "assistant",