From 8f67b6a92bec24ae43b3dfbd087fe35df0531857 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Mon, 27 Jul 2026 12:49:30 +0200 Subject: [PATCH] fix: Only resolve a denied tool call the client was told about (#923) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `permission_denied` forwarded an SDK frame as a `tool_call_update` without checking that the referenced `tool_call` was ever announced, leaving it the last emitter without that check after #916. The id is the executing call's own, and the frame is enqueued from inside canUseTool, so it lands between the call's `tool_use` and the `tool_result` carrying the rejection — verified against the bundled CLI, where the deny short-circuit only emits for pre-ask decisions (rules, mode, classifier, asyncAgent) and fires exactly once per denial. In that flow the call is announced and in flight, so the guard changes nothing. It changes the denials that arrive outside that window. The cancelled-turn guard drops the assistant message carrying the tool_use, so no `tool_call` is emitted, and a denial for it can still land afterwards — the same premise the `tool_result` fallback in `toAcpNotifications` gates on `wasEmitted` for. Ids leave `emittedToolCalls` at `tool_result`, so the check also stops a denial trailing its own result from flipping a call the client has seen finish back to failed. While here: a denial raised inside a subagent identifies it by `agent_id`, never by the Agent/Task call that spawned it, so the update went out unattributed while the `tool_call` it resolves sits in the subagent's transcript. Resolve the parent through `liveBackgroundTasks`, as the canUseTool path already does, and leave the update unattributed rather than dropped when `task_started` was never seen for that agent id. `permission_denied` had no test coverage. Add cases for the in-flight resolve, the reason fallback, the never-announced and already-resolved drops, and subagent attribution with and without a recorded parent. Fixes #918 Co-authored-by: Claude Opus 5 (1M context) --- src/acp-agent.ts | 23 ++++ src/tests/acp-agent.test.ts | 234 ++++++++++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+) diff --git a/src/acp-agent.ts b/src/acp-agent.ts index 8c83126d..7cd77f64 100644 --- a/src/acp-agent.ts +++ b/src/acp-agent.ts @@ -2768,6 +2768,28 @@ export class ClaudeAcpAgent { // already emitted as a `tool_call`, so mark it failed with the // rejection reason — otherwise the client shows a tool call // that silently never resolves. + // + // The id is the executing call's own, and the frame lands + // between its `tool_use` and its `tool_result` (the SDK enqueues + // it from inside canUseTool), so the call is normally in flight + // here. Not always: the assistant message carrying the tool_use + // is dropped by the cancelled-turn guard below, and a denial for + // it can still arrive afterwards — the case the `tool_result` + // fallback in `toAcpNotifications` gates on `wasEmitted` for. + // Drop the update rather than reference a tool call the client + // was never given (see `ensureToolCallEmitted`, issue #851). + if (!session.emittedToolCalls.has(message.tool_use_id)) { + break; + } + // A denial inside a subagent identifies the subagent by + // `agent_id` (as canUseTool does with `agentID`), never by the + // Agent/Task call that spawned it. Resolve it the same way so + // the update lands in the subagent's transcript alongside the + // `tool_call` it resolves, which carries the parent stamped from + // `parent_tool_use_id` (see `liveBackgroundTasks`). + const parentToolUseId = message.agent_id + ? session.liveBackgroundTasks.get(message.agent_id)?.parentToolUseId + : undefined; const reason = message.decision_reason ?? message.message; await sendUpdate({ sessionId: message.session_id, @@ -2784,6 +2806,7 @@ export class ClaudeAcpAgent { _meta: { claudeCode: { toolName: message.tool_name, + ...(parentToolUseId ? { parentToolUseId } : {}), toolResponse: { decisionReasonType: message.decision_reason_type, decisionReason: message.decision_reason, diff --git a/src/tests/acp-agent.test.ts b/src/tests/acp-agent.test.ts index fbb6155c..95090e3d 100644 --- a/src/tests/acp-agent.test.ts +++ b/src/tests/acp-agent.test.ts @@ -11444,3 +11444,237 @@ describe("tool_progress heartbeats", () => { expect(sent.map((u) => u.toolCallId)).toEqual(["toolu_inner"]); }); }); + +describe("permission_denied", () => { + // The SDK enqueues this frame from inside canUseTool, so it lands between the + // denied call's `tool_use` and the `tool_result` carrying the rejection: the + // call is announced and still in flight. The handler forwarded it without + // checking, which mattered for the denials that arrive outside that window. + type ToolCallUpdate = Extract< + SessionNotification["update"], + { sessionUpdate: "tool_call_update" } + >; + + /** Run a turn carrying `messages` and return the updates it produced. */ + async function run(messages: any[], overrides: Record = {}) { + const updates: SessionNotification["update"][] = []; + const mockClient = { + sessionUpdate: async (n: SessionNotification) => { + updates.push(n.update); + }, + } as unknown as AcpClient; + const agent = new ClaudeAcpAgent(mockClient, { log: () => {}, error: () => {} }); + const input = new Pushable(); + async function* messageGenerator() { + const { value, done } = await input[Symbol.asyncIterator]().next(); + if (!done && value) { + yield { + type: "user", + message: value.message, + parent_tool_use_id: null, + uuid: value.uuid, + session_id: "test-session", + isReplay: true, + }; + } + yield* messages; + yield { + type: "result", + subtype: "success", + stop_reason: null, + is_error: false, + result: "", + usage: {}, + uuid: randomUUID(), + session_id: "test-session", + }; + yield { type: "system", subtype: "session_state_changed", state: "idle" }; + } + agent.sessions["test-session"] = mockSessionState({ + query: wrapQuery(messageGenerator()), + input, + ...overrides, + }); + await agent.prompt({ + sessionId: "test-session", + prompt: [{ type: "text", text: "test" }], + }); + return updates; + } + + /** The assistant message announcing `toolUseId`, as the streamed tool_use + * arrives when partial messages are off. */ + function toolUse(toolUseId: string, parentToolUseId: string | null = null) { + return { + type: "assistant", + message: { + role: "assistant", + content: [ + { + type: "tool_use", + id: toolUseId, + name: "Write", + input: { file_path: "/tmp/denied.txt", content: "hi" }, + }, + ], + usage: {}, + }, + parent_tool_use_id: parentToolUseId, + uuid: randomUUID(), + session_id: "test-session", + }; + } + + /** The rejection the model sees, which follows the denial frame. */ + function toolResult(toolUseId: string, parentToolUseId: string | null = null) { + return { + type: "user", + message: { + role: "user", + content: [ + { + type: "tool_result", + tool_use_id: toolUseId, + content: "Permission to use Write has been denied.", + is_error: true, + }, + ], + }, + parent_tool_use_id: parentToolUseId, + uuid: randomUUID(), + session_id: "test-session", + }; + } + + function denial(toolUseId: string, extra: Record = {}) { + return { + type: "system", + subtype: "permission_denied", + tool_name: "Write", + tool_use_id: toolUseId, + decision_reason_type: "mode", + decision_reason: "dontAsk mode denies tools that require approval", + message: "Permission to use Write has been denied.", + uuid: randomUUID(), + session_id: "test-session", + ...extra, + }; + } + + const denials = (updates: SessionNotification["update"][]) => + updates.filter( + (u): u is ToolCallUpdate => + u.sessionUpdate === "tool_call_update" && + JSON.stringify(u.content ?? "").includes("Permission denied"), + ); + + it("marks the announced tool call failed with the denial reason", async () => { + const updates = await run([ + toolUse("toolu_denied"), + denial("toolu_denied"), + toolResult("toolu_denied"), + ]); + + // The denial resolves the call the preceding tool_call announced, before the + // tool_result's own failed update lands. + expect(updates[0]).toMatchObject({ sessionUpdate: "tool_call", toolCallId: "toolu_denied" }); + const sent = denials(updates); + expect(sent).toHaveLength(1); + expect(sent[0]).toMatchObject({ + toolCallId: "toolu_denied", + status: "failed", + content: [ + { + type: "content", + content: { + type: "text", + text: "Permission denied: dontAsk mode denies tools that require approval", + }, + }, + ], + }); + expect((sent[0]._meta as any).claudeCode).toMatchObject({ + toolName: "Write", + toolResponse: { decisionReasonType: "mode" }, + }); + // Top-level denial: nothing to attribute it to. + expect((sent[0]._meta as any).claudeCode).not.toHaveProperty("parentToolUseId"); + }); + + it("falls back to the SDK's rejection message when there is no decision reason", async () => { + const updates = await run([ + toolUse("toolu_denied"), + denial("toolu_denied", { decision_reason: undefined }), + ]); + + expect(denials(updates)[0].content).toEqual([ + { + type: "content", + content: { + type: "text", + text: "Permission denied: Permission to use Write has been denied.", + }, + }, + ]); + }); + + // A cancel drops the assistant message carrying the tool_use, so no tool_call + // is emitted; a denial for it still arrives. Forwarding that left the client + // resolving an id it was never given — the case the tool_result fallback in + // `toAcpNotifications` already gates on `wasEmitted` for. + it("drops a denial for a tool call that was never announced", async () => { + expect(denials(await run([denial("toolu_ghost")]))).toHaveLength(0); + }); + + // Ids leave `emittedToolCalls` at `tool_result`, so a denial that somehow + // trails its own result can't flip a call the client has seen finish back to + // failed. + it("drops a denial for a tool call that already resolved", async () => { + const updates = await run([ + toolUse("toolu_denied"), + toolResult("toolu_denied"), + denial("toolu_denied"), + ]); + + expect(denials(updates)).toHaveLength(0); + }); + + // A subagent's denial names the subagent (`agent_id`), never the Agent/Task + // call that spawned it, so without the `liveBackgroundTasks` lookup the update + // lands at the top level while the tool_call it resolves sits in the + // subagent's transcript. + it("attributes a subagent denial to the Agent call that spawned it", async () => { + const updates = await run([ + { + type: "system", + subtype: "task_started", + task_id: "agent-1", + tool_use_id: "toolu_agent", + subagent_type: "general-purpose", + uuid: randomUUID(), + session_id: "test-session", + }, + toolUse("toolu_inner", "toolu_agent"), + denial("toolu_inner", { agent_id: "agent-1" }), + ]); + + expect((denials(updates)[0]._meta as any).claudeCode).toMatchObject({ + toolName: "Write", + parentToolUseId: "toolu_agent", + }); + }); + + // The attribution rests on `task_started` having been seen for the agent id. + // If it wasn't, the denial still resolves the call — unattributed beats + // dropped. + it("still resolves a subagent denial when the parent is unknown", async () => { + const updates = await run([ + toolUse("toolu_inner", "toolu_agent"), + denial("toolu_inner", { agent_id: "agent-unknown" }), + ]); + + const sent = denials(updates); + expect(sent).toHaveLength(1); + expect((sent[0]._meta as any).claudeCode).not.toHaveProperty("parentToolUseId"); + }); +});