jerry/claude-agent-acp
1
0
Fork 0
mirror of https://github.com/agentclientprotocol/claude-agent-acp.git synced 2026-10-27 12:40:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Emit tool_call before permission request (#820)

Browse Source 
The Claude Agent SDK changed the timing of `canUseTool` — it now runs
*before* the assistant message's `tool_use` block streams to us. As a
result we sent a permission request (and `AskUserQuestion` elicitation)
referencing a `toolCallId` the client had not yet seen via a `tool_call`
update, so clients couldn't associate the prompt with a tool call.

This supersedes #815, which fixed the ordering by blocking `canUseTool`
until the stream emitted the `tool_call`. That approach could deadlock
on cancellation (the `await` had no signal/timeout) and woke the waiter
from only one branch, so suppressed tools never resolved it.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ben Brandt
2026-06-29 15:48:22 +02:00
committed by GitHub
parent 5dd8746237
commit c95fc884e8
3 changed files with 313 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

199
src/acp-agent.ts
View File

@@ -280,6 +280,14 @@ type Session = {
* `tool_result` time so a long-running session doesn't accumulate every
* tool call for its whole lifetime. */
toolUseCache: ToolUseCache;
/** Tracks which tool_use ids we've already emitted a `tool_call` for, so the
* second source to encounter a tool call sends a `tool_call_update` instead
* of a duplicate `tool_call`. The SDK can invoke `canUseTool` (→ a permission
* request, which emits the tool_call eagerly so the client has it before
* being asked to approve it) either before or after the assistant message's
* tool_use block streams; this set makes the two paths converge regardless of
* order. Pruned at `tool_result` time alongside `toolUseCache`. */
emittedToolCalls: Set<string>;
/** Maps the ACP `messageId` we expose to clients (see `messageIdForGrouping`)
* to the SDK message uuid that the Agent SDK's rewind/resume APIs key on
* (`Query.rewindFiles` takes a user-message uuid; `resumeSessionAt` takes an
@@ -1897,6 +1905,7 @@ export class ClaudeAcpAgent {
clientCapabilities: this.clientCapabilities,
cwd: session.cwd,
taskState: session.taskState,
emittedToolCalls: session.emittedToolCalls,
messageId: currentStreamMessageId,
},
)) {
@@ -2128,6 +2137,7 @@ export class ClaudeAcpAgent {
parentToolUseId: message.parent_tool_use_id,
cwd: session.cwd,
taskState: session.taskState,
emittedToolCalls: session.emittedToolCalls,
messageId: messageIdForGrouping(message),
},
)) {
@@ -2550,8 +2560,21 @@ export class ClaudeAcpAgent {
* longer leaves the `await` hanging. */
private async requestPermissionFromClient(
params: RequestPermissionRequest,
toolName: string,
signal: AbortSignal,
): Promise<RequestPermissionResponse> {
// The SDK may invoke `canUseTool` (and therefore this permission request)
// before the assistant message's tool_use block streams to us. Some ACP clients
// expect the `tool_call` a permission request references to already exist,
// so emit it now if it hasn't been sent yet. The streamed tool_use chunk
// later refines it with a `tool_call_update` rather than emitting a
// duplicate (see `emittedToolCalls` in `toAcpNotifications`).
await this.ensureToolCallEmitted(
params.sessionId,
toolName,
params.toolCall.toolCallId,
params.toolCall.rawInput,
);
try {
return await this.client.requestPermission(params, signal);
} catch (error) {
@@ -2562,6 +2585,39 @@ export class ClaudeAcpAgent {
}
}
/** Emit the `tool_call` a permission request references if it hasn't been sent
* yet, so the client has the tool call before being asked to approve it. The
* matching streamed tool_use chunk later refines it with a `tool_call_update`
* instead of emitting a duplicate (see `emittedToolCalls`). Built via the same
* `toolCallNotification` helper as the streamed path so the two are identical.
* Tools the stream renders as a plan (TodoWrite) or suppresses (Task*) are
* skipped so a permission prompt for them never surfaces a stray tool_call. */
private async ensureToolCallEmitted(
sessionId: string,
toolName: string,
toolCallId: string,
toolInput: unknown,
): Promise<void> {
const session = this.sessions[sessionId];
if (!session || !shouldEmitToolCall(toolName)) {
return;
}
if (session.emittedToolCalls.has(toolCallId)) {
return;
}
session.emittedToolCalls.add(toolCallId);
const supportsTerminalOutput = this.clientCapabilities?._meta?.["terminal_output"] === true;
await this.client.sessionUpdate({
sessionId,
update: toolCallNotification(
{ id: toolCallId, name: toolName, input: toolInput },
toolInput,
supportsTerminalOutput,
session.cwd,
),
});
}
canUseTool(sessionId: string): CanUseTool {
return async (toolName, toolInput, { signal, suggestions, toolUseID }) => {
const alwaysAllowLabel = describeAlwaysAllow(suggestions, toolName);
@@ -2579,6 +2635,9 @@ export class ClaudeAcpAgent {
// than the interactive dialog). Present it as an ACP form elicitation and
// feed the answers back as updatedInput for the tool's own call() to read.
if (toolName === "AskUserQuestion" && this.clientCapabilities?.elicitation?.form) {
// Like permission requests, the elicitation references this toolUseID, so
// make sure the tool_call has surfaced to the client before we send it.
await this.ensureToolCallEmitted(sessionId, toolName, toolUseID, toolInput);
return this.handleAskUserQuestion(sessionId, toolInput, toolUseID, signal);
}
@@ -2623,6 +2682,7 @@ export class ClaudeAcpAgent {
),
},
},
toolName,
signal,
);
@@ -2695,6 +2755,7 @@ export class ClaudeAcpAgent {
),
},
},
toolName,
signal,
);
if (signal.aborted || response.outcome?.outcome === "cancelled") {
@@ -3433,6 +3494,7 @@ export class ClaudeAcpAgent {
) ?? DEFAULT_CONTEXT_WINDOW,
taskState,
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
};
@@ -4149,6 +4211,64 @@ function applyMessageId(
}
}
/** Built-in tools that drive the task list (headless/SDK sessions use these
* instead of TodoWrite). Their tool_use/tool_result are surfaced as `plan`
* snapshots rather than as tool_calls. */
function isTaskTool(toolName: string): boolean {
return (
toolName === "TaskCreate" ||
toolName === "TaskUpdate" ||
toolName === "TaskList" ||
toolName === "TaskGet"
);
}
/** Whether a tool's tool_use surfaces to the client as a standalone
* `tool_call`. TodoWrite is rendered as a `plan` and Task* tools are
* suppressed (their plan snapshot is emitted at tool_result time), so neither
* produces a tool_call. */
function shouldEmitToolCall(toolName: string): boolean {
return toolName !== "TodoWrite" && !isTaskTool(toolName);
}
/** Build the `tool_call` (or, with `refine`, the `tool_call_update`)
* notification for a tool_use. Shared by every site that surfaces a tool call:
* the streamed tool_use path (first encounter → tool_call, later encounter →
* refine) and the permission flow (`ensureToolCallEmitted`), so they can't
* drift. The initial `tool_call` carries `status: "pending"` and, for Bash, the
* `terminal_info` _meta that the later `terminal_output`/`terminal_exit`
* updates key off of; a refining `tool_call_update` carries neither. */
function toolCallNotification(
toolUse: { id: string; name: string; input: unknown },
rawInput: unknown,
supportsTerminalOutput: boolean,
cwd?: string,
refine = false,
): SessionNotification["update"] {
if (refine) {
return {
_meta: { claudeCode: { toolName: toolUse.name } } satisfies ToolUpdateMeta,
toolCallId: toolUse.id,
sessionUpdate: "tool_call_update",
rawInput,
...toolInfoFromToolUse(toolUse, supportsTerminalOutput, cwd),
};
}
return {
_meta: {
claudeCode: { toolName: toolUse.name },
...(toolUse.name === "Bash" && supportsTerminalOutput
? { terminal_info: { terminal_id: toolUse.id } }
: {}),
} satisfies ToolUpdateMeta,
toolCallId: toolUse.id,
sessionUpdate: "tool_call",
rawInput,
status: "pending",
...toolInfoFromToolUse(toolUse, supportsTerminalOutput, cwd),
};
}
/**
* Convert an SDKAssistantMessage (Claude) to a SessionNotification (ACP).
* Only handles text, image, and thinking chunks for now.
@@ -4166,6 +4286,13 @@ export function toAcpNotifications(
parentToolUseId?: string | null;
cwd?: string;
taskState?: TaskState;
// Tracks tool_use ids already emitted as a `tool_call` so a permission
// request (which emits the tool_call eagerly) and the streamed tool_use
// chunk don't both emit one — whichever arrives second emits a
// `tool_call_update` instead. Mutated in place. When omitted, the
// tool_call/update decision falls back to `toolUseCache` presence (the
// historical single-source behavior).
emittedToolCalls?: Set<string>;
// Opaque id identifying the message these chunks belong to (ACP message ids
// are opaque strings — no particular format is required). Attached to
// user/agent message and thought chunks so clients can group streamed chunks
@@ -4253,12 +4380,7 @@ export function toAcpNotifications(
entries: planEntries(chunk.input as { todos: ClaudePlanEntry[] }),
};
}
} else if (
chunk.name === "TaskCreate" ||
chunk.name === "TaskUpdate" ||
chunk.name === "TaskList" ||
chunk.name === "TaskGet"
) {
} else if (isTaskTool(chunk.name)) {
// Task* tool_use is suppressed; the plan update is emitted at
// tool_result time once we have the task ID (for TaskCreate) and
// confirmation that the change took effect.
@@ -4311,39 +4433,31 @@ export function toAcpNotifications(
// ignore if we can't turn it to JSON
}
if (alreadyCached) {
// Second encounter (full assistant message after streaming) —
// send as tool_call_update to refine the existing tool_call
// rather than emitting a duplicate tool_call.
update = {
_meta: {
claudeCode: {
toolName: chunk.name,
},
} satisfies ToolUpdateMeta,
toolCallId: chunk.id,
sessionUpdate: "tool_call_update",
// Emit a `tool_call` only the first time this id surfaces to the
// client; afterwards refine it with a `tool_call_update`. The first
// surface may be this stream chunk OR an earlier permission request
// (see `ensureToolCallEmitted`), so emission is tracked separately
// from `toolUseCache`. Without an `emittedToolCalls` set we fall back
// to cache presence — the historical streaming-only behavior.
const emittedToolCalls = options?.emittedToolCalls;
const alreadyEmitted = emittedToolCalls ? emittedToolCalls.has(chunk.id) : alreadyCached;
emittedToolCalls?.add(chunk.id);
if (alreadyEmitted) {
// Already surfaced (full assistant message after streaming, or a
// permission request emitted it first) — refine with a
// tool_call_update rather than emitting a duplicate tool_call.
update = toolCallNotification(
chunk,
rawInput,
...toolInfoFromToolUse(chunk, supportsTerminalOutput, options?.cwd),
};
supportsTerminalOutput,
options?.cwd,
true,
);
} else {
// First encounter (streaming content_block_start or replay) —
// send as tool_call with terminal_info for Bash tools.
update = {
_meta: {
claudeCode: {
toolName: chunk.name,
},
...(chunk.name === "Bash" && supportsTerminalOutput
? { terminal_info: { terminal_id: chunk.id } }
: {}),
} satisfies ToolUpdateMeta,
toolCallId: chunk.id,
sessionUpdate: "tool_call",
rawInput,
status: "pending",
...toolInfoFromToolUse(chunk, supportsTerminalOutput, options?.cwd),
};
// First surface (streaming content_block_start or replay) — send as
// tool_call (with terminal_info for Bash).
update = toolCallNotification(chunk, rawInput, supportsTerminalOutput, options?.cwd);
}
}
break;
@@ -4357,6 +4471,7 @@ export function toAcpNotifications(
case "bash_code_execution_tool_result":
case "text_editor_code_execution_tool_result":
case "mcp_tool_result": {
options?.emittedToolCalls?.delete(chunk.tool_use_id);
const toolUse = toolUseCache[chunk.tool_use_id];
if (!toolUse) {
logger.error(
@@ -4365,12 +4480,7 @@ export function toAcpNotifications(
break;
}
if (
toolUse.name === "TaskCreate" ||
toolUse.name === "TaskUpdate" ||
toolUse.name === "TaskList" ||
toolUse.name === "TaskGet"
) {
if (isTaskTool(toolUse.name)) {
// Headless/SDK sessions emit Task* tools instead of TodoWrite.
// TaskCreate / TaskUpdate mutate the accumulated task list; TaskList
// and TaskGet are read-only so we just suppress their tool_call /
@@ -4490,6 +4600,7 @@ export function streamEventToAcpNotifications(
clientCapabilities?: ClientCapabilities;
cwd?: string;
taskState?: TaskState;
emittedToolCalls?: Set<string>;
messageId?: string;
},
): SessionNotification[] {
@@ -4508,6 +4619,7 @@ export function streamEventToAcpNotifications(
parentToolUseId: message.parent_tool_use_id,
cwd: options?.cwd,
taskState: options?.taskState,
emittedToolCalls: options?.emittedToolCalls,
messageId: options?.messageId,
},
);
@@ -4524,6 +4636,7 @@ export function streamEventToAcpNotifications(
parentToolUseId: message.parent_tool_use_id,
cwd: options?.cwd,
taskState: options?.taskState,
emittedToolCalls: options?.emittedToolCalls,
messageId: options?.messageId,
},
);

144
src/tests/acp-agent.test.ts
View File

@@ -118,6 +118,7 @@ function mockSessionState(overrides: Record<string, any> = {}) {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
...overrides,
} as any;
@@ -1814,6 +1815,7 @@ describe("permission request cancellation", () => {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
} as any;
return agent.sessions[sessionId]!;
@@ -1835,7 +1837,11 @@ describe("permission request cancellation", () => {
},
} as unknown as AcpClient;
const agent = new ClaudeAcpAgent(mockClient, { log: () => {}, error: () => {} });
injectSession(agent, "session-1");
const session = injectSession(agent, "session-1");
// The tool_call was already surfaced by the streamed tool_use chunk, so the
// permission request goes straight to requestPermission without first
// emitting one.
session.emittedToolCalls.add("tool-1");
const controller = new AbortController();
const pending = agent.canUseTool("session-1")("Bash", { command: "ls" }, {
@@ -1872,6 +1878,136 @@ describe("permission request cancellation", () => {
});
});
describe("tool_call emitted before permission request", () => {
// The SDK can invoke canUseTool before the assistant message's tool_use block
// streams to us. ACP clients expect the tool_call a permission request
// references to already exist, so the permission flow emits it eagerly and the
// streamed chunk later refines it with a tool_call_update (deduped via
// session.emittedToolCalls) rather than emitting a duplicate.
function setup(overrides: Record<string, any> = {}) {
const events: string[] = [];
const updates: SessionNotification[] = [];
const mockClient = {
sessionUpdate: async (n: SessionNotification) => {
events.push(`update:${n.update.sessionUpdate}`);
updates.push(n);
},
requestPermission: async () => {
events.push("permission");
return { outcome: { outcome: "selected", optionId: "allow" } };
},
} as unknown as AcpClient;
const agent = new ClaudeAcpAgent(mockClient, { log: () => {}, error: () => {} });
agent.sessions["session-1"] = mockSessionState(overrides);
return { agent, events, updates, session: agent.sessions["session-1"]! };
}
it("emits the tool_call (then asks permission) when the stream hasn't yet", async () => {
const { agent, events, updates, session } = setup();
const result = await agent.canUseTool("session-1")("Bash", { command: "ls" }, {
signal: new AbortController().signal,
suggestions: [],
toolUseID: "tool-1",
} as any);
// tool_call is sent before the permission request is raised.
expect(events).toEqual(["update:tool_call", "permission"]);
expect(updates[0].update).toMatchObject({
sessionUpdate: "tool_call",
toolCallId: "tool-1",
status: "pending",
});
expect(session.emittedToolCalls.has("tool-1")).toBe(true);
expect(result).toMatchObject({ behavior: "allow" });
});
it("does not re-emit the tool_call when the stream already surfaced it", async () => {
const { agent, events } = setup();
agent.sessions["session-1"]!.emittedToolCalls.add("tool-1");
await agent.canUseTool("session-1")("Bash", { command: "ls" }, {
signal: new AbortController().signal,
suggestions: [],
toolUseID: "tool-1",
} as any);
expect(events).toEqual(["permission"]);
});
it("refines the eagerly-emitted tool_call with a tool_call_update when the chunk streams", () => {
const { session } = setup();
// Permission flow already emitted the tool_call for this id.
session.emittedToolCalls.add("tool-1");
const notifications = toAcpNotifications(
[{ type: "tool_use", id: "tool-1", name: "Bash", input: { command: "ls" } }],
"assistant",
"session-1",
session.toolUseCache,
{} as AcpClient,
console,
{ emittedToolCalls: session.emittedToolCalls },
);
expect(notifications).toHaveLength(1);
expect(notifications[0].update.sessionUpdate).toBe("tool_call_update");
});
it("does not emit a tool_call for suppressed tools (TodoWrite) on a permission request", async () => {
const { agent, events, session } = setup();
await agent.canUseTool("session-1")(
"TodoWrite",
{ todos: [{ content: "x", status: "pending" }] },
{ signal: new AbortController().signal, suggestions: [], toolUseID: "todo-1" } as any,
);
expect(events).toEqual(["permission"]);
expect(session.emittedToolCalls.has("todo-1")).toBe(false);
});
it("includes Bash terminal_info _meta in the eager tool_call so terminal output can attach", async () => {
const { agent, updates } = setup();
// Terminal-capable client (e.g. Zed). The eager tool_call must carry
// terminal_info.terminal_id, otherwise the later terminal_output/terminal_exit
// updates (keyed by terminal_id) have nothing to attach to.
(agent as any).clientCapabilities = { _meta: { terminal_output: true } };
await agent.canUseTool("session-1")("Bash", { command: "ls" }, {
signal: new AbortController().signal,
suggestions: [],
toolUseID: "tool-1",
} as any);
expect(updates[0].update).toMatchObject({
sessionUpdate: "tool_call",
toolCallId: "tool-1",
_meta: { terminal_info: { terminal_id: "tool-1" } },
});
});
it("prunes the emission marker on a tool_result even when the tool_use was never cached", () => {
const { session } = setup();
// Eager-emitted via the permission flow, but the tool_use chunk never
// streamed (e.g. cancelled), so toolUseCache has no entry for it.
session.emittedToolCalls.add("tool-1");
toAcpNotifications(
[{ type: "tool_result", tool_use_id: "tool-1", content: [{ type: "text", text: "x" }] }],
"user",
"session-1",
session.toolUseCache,
{} as AcpClient,
// Silence the expected "tool result for tool use that wasn't tracked" log.
{ log: () => {}, error: () => {} },
{ emittedToolCalls: session.emittedToolCalls },
);
expect(session.emittedToolCalls.has("tool-1")).toBe(false);
});
});
describe("runPromptWithCancellation", () => {
function deferred<T>() {
let resolve!: (value: T) => void;
@@ -2582,6 +2718,7 @@ describe("session/close", () => {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
};
return agent.sessions[sessionId]!;
@@ -2667,6 +2804,7 @@ describe("session/delete", () => {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
};
return agent.sessions[sessionId]!;
@@ -2769,6 +2907,7 @@ describe("getOrCreateSession param change detection", () => {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
};
return agent.sessions[sessionId]!;
@@ -4983,6 +5122,7 @@ describe("post-error recovery", () => {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
};
return { interrupt };
@@ -5553,6 +5693,7 @@ describe("session/cancel wedge recovery (issue #680)", () => {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
};
return { interrupt };
@@ -6006,6 +6147,7 @@ describe("agent selection config option", () => {
contextWindowSize: 200000,
taskState: new Map(),
toolUseCache: {},
emittedToolCalls: new Set(),
messageIdToUuid: new Map(),
};
return { session: agent.sessions[sessionId]!, applyFlagSettings };

14
src/tests/session-config-options.test.ts
View File

@@ -122,6 +122,8 @@ describe("session config options", () => {
})),
configOptions: structuredClone(MOCK_CONFIG_OPTIONS),
contextWindowSize: 200000,
toolUseCache: {},
emittedToolCalls: new Set(),
};
}
@@ -1111,6 +1113,10 @@ describe("session config options", () => {
],
};
// The tool_call was already surfaced (by the streamed tool_use chunk), so
// the permission request won't re-emit one — keep this focused on options.
session.emittedToolCalls.add("toolu_1");
const canUseTool = (agent as any).canUseTool(SESSION_ID);
const signal = new AbortController().signal;
try {
@@ -1142,6 +1148,10 @@ describe("session config options", () => {
],
};
permissionResponse = { outcome: { outcome: "selected", optionId: "auto" } };
// The tool_call was already surfaced (by the streamed tool_use chunk), so
// the permission request won't re-emit one — the deny path below should
// produce no session updates at all.
session.emittedToolCalls.add("toolu_2");
const canUseTool = (agent as any).canUseTool(SESSION_ID);
const result = await canUseTool(
@@ -1170,6 +1180,10 @@ describe("session config options", () => {
],
};
// The tool_call was already surfaced (by the streamed tool_use chunk), so
// the permission request won't re-emit one — keep this focused on options.
session.emittedToolCalls.add("toolu_3");
const canUseTool = (agent as any).canUseTool(SESSION_ID);
const signal = new AbortController().signal;
try {