diff --git a/README.md b/README.md index 75f14ffe..cfec5ed1 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,8 @@ Task subagent as its own child session, following the [subagents RFD](https://gi - `subagent_update` on the parent announces the child before any of its traffic, with the parent's `title` and `description` for it, and reports its work state: `running`, - `requires_action` while one of its requests is open, and `idle` when its work ends, with + `requires_action` while one of its requests is open (and `running` again when the last one ends, + unless a cancel withdrew it: the child is stopping then), and `idle` when its work ends, with `end_turn` or `cancelled` when the SDK gives the reason. A failure is `idle` with the `error` stop reason and the SDK's error text as its `error`. The child keeps its session for every delegation to it. diff --git a/src/acp-agent.ts b/src/acp-agent.ts index 06b60eab..bf429ab4 100644 --- a/src/acp-agent.ts +++ b/src/acp-agent.ts @@ -9035,15 +9035,17 @@ export class ClaudeAcpAgent { /** Runs `request`, a request that `requestSessionId` sends for the session * `ownerSessionId`. When that is a subagent's child session, the child * reports that it awaits the user while the request is open (the RFD form, - * see `NativeSubagentRuntime.awaitingUser`). */ + * see `NativeSubagentRuntime.awaitingUser`). `signal` aborts when Claude + * Code withdraws the request. */ private withChildAwaitingUser( ownerSessionId: string, requestSessionId: string, + signal: AbortSignal, request: () => Promise, ): Promise { const runtime = this.sessions[ownerSessionId]?.nativeSubagentRuntime; return runtime && requestSessionId !== ownerSessionId - ? runtime.awaitingUser(requestSessionId, request) + ? runtime.awaitingUser(requestSessionId, request, signal) : request(); } @@ -9270,7 +9272,7 @@ export class ClaudeAcpAgent { // The request goes to a subagent's child session for a tool call of that // subagent, but it blocks the turn of the owner session. try { - return await this.withChildAwaitingUser(ownerSessionId, params.sessionId, () => + return await this.withChildAwaitingUser(ownerSessionId, params.sessionId, signal, () => this.withPendingUserInput( ownerSessionId, () => raceWithAbort(this.client.requestPermission(params, signal), signal), @@ -9759,7 +9761,7 @@ export class ClaudeAcpAgent { ); let response; try { - response = await this.withChildAwaitingUser(sessionId, requestSessionId, () => + response = await this.withChildAwaitingUser(sessionId, requestSessionId, signal, () => this.withPendingUserInput( sessionId, () => this.client.createElicitation(createRequest, signal), diff --git a/src/native-subagents.ts b/src/native-subagents.ts index 06667100..a5c6d37f 100644 --- a/src/native-subagents.ts +++ b/src/native-subagents.ts @@ -138,6 +138,8 @@ export class NativeSubagentRuntime { private readonly generationByTaskId = new Map(); private readonly pending = new Map(); private pendingCount = 0; + /** The RFD form: how many times the parent was cancelled (see {@link awaitingUser}). */ + private parentCancels = 0; /** * `form` is undefined for a client without native subagent sessions. The RFD @@ -515,20 +517,36 @@ export class NativeSubagentRuntime { * foreground children that the turn waits on and, currently, background * children too, and the SDK's task events report each as cancelled. A child * that keeps running keeps its open requests - * (agentclientprotocol/agent-client-protocol#2308). + * (agentclientprotocol/agent-client-protocol#2308). The RFD form only counts + * the cancel, for {@link awaitingUser}. */ async parentCancelled(deliver: Publish): Promise { if (this.form === "air") await this.finishAll("cancelled", deliver); + else this.parentCancels++; } /** * Runs `request`, a permission or question request of the session * `sessionId`. In the RFD form, a child waiting on it is `requires_action` - * until its last open request ends. + * until its last open request ends, and then `running` again. + * + * `signal` aborts when Claude Code withdraws the request, which is not an + * answer. If the parent was cancelled while the request was open, Claude + * Code's interrupt withdrew it as it stopped the child, and the SDK's events + * report the child `idle` next. The end of the request then reports no + * `running`, which would show a child that is being stopped as working. + * Without a cancel, something else answered the request in the user's place, + * such as a `PermissionRequest` hook, and the child runs on, so the end + * reports `running`. */ - async awaitingUser(sessionId: string, request: () => Promise): Promise { + async awaitingUser( + sessionId: string, + request: () => Promise, + signal?: AbortSignal, + ): Promise { const child = this.form === "rfd" ? this.childOfSession(sessionId) : undefined; if (!child?.announced) return request(); + const cancelsAtOpen = this.parentCancels; child.openRequests = (child.openRequests ?? 0) + 1; if (child.openRequests === 1 && child.workState === "running") { await this.reportState(child, { state: "requires_action" }).catch((error) => @@ -539,7 +557,9 @@ export class NativeSubagentRuntime { return await request(); } finally { child.openRequests = (child.openRequests ?? 1) - 1; - if (child.openRequests === 0 && child.workState === "requires_action") { + // Withdrawn after a cancel that came while the request was open. + const stopped = signal?.aborted === true && this.parentCancels !== cancelsAtOpen; + if (child.openRequests === 0 && child.workState === "requires_action" && !stopped) { await this.reportState(child, { state: "running" }).catch((error) => this.logger.log(`Failed to report that subagent ${child.sessionId} runs: ${error}`), ); diff --git a/src/tests/acp-agent.test.ts b/src/tests/acp-agent.test.ts index 43d88608..1b3249fd 100644 --- a/src/tests/acp-agent.test.ts +++ b/src/tests/acp-agent.test.ts @@ -5846,6 +5846,116 @@ describe("subagent permission attribution (issue #851)", () => { expect(requests[0].sessionId).toBe("agent-42"); }); + describe("a child's permission request that Claude Code withdraws (the subagents RFD)", () => { + /** + * An agent for an RFD client, whose child `agent-42` runs and has asked for + * permission. The client holds the request and ignores `$/cancel_request`. + * `withdrawal` is the request's signal, which Claude Code aborts when it + * withdraws the request. + */ + async function childWaitingForPermission() { + const updates: AcpSessionNotification[] = []; + const withdrawal = new AbortController(); + const agent = new ClaudeAcpAgent( + { + sessionUpdate: async (notification: AcpSessionNotification) => { + updates.push(notification); + }, + requestPermission: () => new Promise(() => {}), + } as unknown as AcpClient, + { log: () => {}, error: () => {} }, + ); + await initializeClient(agent, { subagents: {} } as ClientCapabilities); + const query = wrapQuery((async function* () {})()); + const session = mockSessionState({ query, input: new Pushable() }); + agent.sessions["session-1"] = session; + session.liveBackgroundTasks.set("agent-42", { + parentToolUseId: "toolu_parent", + isSubagent: true, + }); + session.nativeSubagentsByTaskId = new Map([ + [ + "agent-42", + { + sessionId: "agent-42", + parentSessionId: "session-1", + parentToolUseId: "toolu_parent", + name: "Explore", + task: "Investigate", + announced: true, + workState: "running", + }, + ], + ]); + const runtime = new NativeSubagentRuntime( + "rfd", + "session-1", + session, + async (notification) => { + updates.push(notification); + }, + { log: () => {} }, + ); + session.nativeSubagentRuntime = runtime; + const asking = agent + .canUseTool("session-1")("Bash", { command: "ls" }, { + signal: withdrawal.signal, + suggestions: [], + toolUseID: "toolu_sub", + agentID: "agent-42", + } as any) + .catch((error: Error) => error.message); + /** The states that the child reported, such as `idle cancelled`. */ + const childStates = () => + updates.flatMap(({ update }) => + update.sessionUpdate === "subagent_update" && update.state + ? [ + [update.state.state, (update.state as { stopReason?: string }).stopReason] + .filter(Boolean) + .join(" "), + ] + : [], + ); + await vi.waitFor(() => expect(childStates()).toEqual(["requires_action"])); + return { agent, query, runtime, withdrawal, asking, childStates }; + } + + it("reports no running when a cancel withdraws it, and leaves the child's end to the SDK", async () => { + const { agent, query, runtime, withdrawal, asking, childStates } = + await childWaitingForPermission(); + // Claude Code's interrupt withdraws the request of the child that it stops. + query.interrupt.mockImplementation(async () => withdrawal.abort()); + + await agent.cancel({ sessionId: "session-1" }); + await expect(asking).resolves.toBe("Tool use aborted"); + expect(childStates()).toEqual(["requires_action"]); + + await runtime.finishTask("agent-42", "killed", async () => {}); + expect(childStates()).toEqual(["requires_action", "idle cancelled"]); + }); + + it("reports no running when a session/close withdraws it", async () => { + const { agent, query, withdrawal, asking, childStates } = await childWaitingForPermission(); + // The SDK's cleanup of a closed query aborts every request that is open. + query.close.mockImplementation(() => withdrawal.abort()); + + await agent.closeSession({ sessionId: "session-1" }); + await expect(asking).resolves.toBe("Tool use aborted"); + + expect(childStates()).toEqual(["requires_action"]); + }); + + it("reports running when Claude Code withdraws it on its own, since the child goes on", async () => { + const { withdrawal, asking, childStates } = await childWaitingForPermission(); + + // A hook answered the request in the user's place. + withdrawal.abort(); + await expect(asking).resolves.toBe("Tool use aborted"); + + expect(childStates()).toEqual(["requires_action", "running"]); + }); + }); + it("sends the PostToolUse hook update of a child's Edit to the child session", async () => { const updates: AcpSessionNotification[] = []; const agent = new ClaudeAcpAgent( diff --git a/src/tests/acp-v2.test.ts b/src/tests/acp-v2.test.ts index 9906552f..d0a09711 100644 --- a/src/tests/acp-v2.test.ts +++ b/src/tests/acp-v2.test.ts @@ -3734,6 +3734,71 @@ describe("ACP v2 requests that subagents make", () => { "running", ]); }); + + it("reports no running before idle cancelled for a child's request that a cancel withdraws", async () => { + // Claude Code's interrupt withdraws the request, which the client holds and + // would not settle, then it stops the child. + const withdrawal = new AbortController(); + sdk.interrupt = async () => withdrawal.abort(); + const client = v2Client({ + answerPermission: () => + new Promise(() => {}) as unknown as v2.RequestPermissionResponse, + }); + scriptTurns(async function* (options) { + yield toolUseMessage(options, "toolu_agent-1", "Agent", { + description: "Explore the project", + prompt: "Look around", + subagent_type: "Explore", + }); + yield taskStarted(options, "agent-1", { is_backgrounded: false }); + await options.canUseTool!("Bash", { command: "ls" }, { + signal: withdrawal.signal, + suggestions: [], + toolUseID: "toolu_ls", + agentID: "agent-1", + requestId: randomUUID(), + } as Parameters>[2]).catch(() => undefined); + yield { + type: "system", + subtype: "task_updated", + task_id: "agent-1", + patch: { status: "killed" }, + uuid: randomUUID(), + session_id: options.sessionId, + }; + yield result(options, ABORTED_TOOLS); + yield idleState(options); + }); + let root = ""; + await client.app.connectWith(connectRouter(), async (agent) => { + await initializeV2(agent); + ({ sessionId: root } = await agent.request(v2.methods.agent.session.new, { cwd })); + await agent.request(v2.methods.agent.session.prompt, { sessionId: root, prompt: text("go") }); + await vi.waitFor(() => expect(client.permissionRequests).toHaveLength(1)); + await agent.notify(v2.methods.agent.session.cancel, { sessionId: root }); + await vi.waitFor(() => + expect( + turnTrace(client.sessionUpdates.filter(({ sessionId }) => sessionId === root)), + ).toContain("idle cancelled"), + ); + await client.authUpdate(1); + }); + + expect(client.permissionRequests[0].sessionId).toBe("agent-1"); + // The child's own stream, and the parent's copy of its states. + expect( + turnTrace(client.sessionUpdates.filter(({ sessionId }) => sessionId === "agent-1")), + ).toEqual(["running", "requires_action", "idle cancelled"]); + expect( + client.sessionUpdates.flatMap(({ sessionId, update }) => { + if (sessionId !== root || update.sessionUpdate !== "subagent_update" || !update.state) { + return []; + } + const { state, stopReason } = update.state as { state: string; stopReason?: string }; + return [stopReason ? `${state} ${stopReason}` : state]; + }), + ).toEqual(["running", "requires_action", "idle cancelled"]); + }); }); // A Bash call that sends its command to the background returns at once, while diff --git a/src/tests/native-subagents.test.ts b/src/tests/native-subagents.test.ts index c1415c5a..cd602a0e 100644 --- a/src/tests/native-subagents.test.ts +++ b/src/tests/native-subagents.test.ts @@ -1442,4 +1442,132 @@ describe("NativeSubagentRuntime in the RFD form", () => { expect(states(published).at(-1)).toEqual({ state: "idle", stopReason: "end_turn" }); }); + + describe("a request that Claude Code withdraws", () => { + /** + * A request of `worker-1` that fails once `withdrawal` aborts, as the + * agent's requests do. It resolves with how it ended. + */ + const withdrawable = (runtime: NativeSubagentRuntime, withdrawal: AbortController) => + runtime + .awaitingUser( + "worker-1", + () => + new Promise((_answer, reject) => { + const fail = () => reject(new Error("Tool use aborted")); + if (withdrawal.signal.aborted) fail(); + else withdrawal.signal.addEventListener("abort", fail); + }), + withdrawal.signal, + ) + .catch((error: Error) => error.message); + + it("does not report running for a request that a cancel withdraws, and leaves the child's end to the SDK", async () => { + const { runtime, published } = await runningWorker(); + const withdrawal = new AbortController(); + const request = withdrawable(runtime, withdrawal); + await vi.waitFor(() => expect(states(published)).toHaveLength(2)); + + // Claude Code's interrupt withdraws the request of the child it stops, + // then the SDK reports that it killed the child. + await runtime.parentCancelled(async () => {}); + withdrawal.abort(); + await expect(request).resolves.toBe("Tool use aborted"); + expect(states(published)).toEqual([{ state: "running" }, { state: "requires_action" }]); + + await runtime.finishTask("worker-1", "killed", async () => {}); + expect(states(published)).toEqual([ + { state: "running" }, + { state: "requires_action" }, + { state: "idle", stopReason: "cancelled" }, + ]); + }); + + it("reports running for a request that Claude Code withdraws without a cancel, since the child goes on", async () => { + const { runtime, published } = await runningWorker(); + const withdrawal = new AbortController(); + const request = withdrawable(runtime, withdrawal); + await vi.waitFor(() => expect(states(published)).toHaveLength(2)); + + // A hook answered the request in the user's place. + withdrawal.abort(); + await expect(request).resolves.toBe("Tool use aborted"); + + expect(states(published)).toEqual([ + { state: "running" }, + { state: "requires_action" }, + { state: "running" }, + ]); + }); + + it("keeps the open request of a child that outlives a cancel, and reports running once the user answers it", async () => { + const { runtime, published } = await runningWorker(); + const answer = Promise.withResolvers(); + const request = runtime.awaitingUser("worker-1", () => answer.promise); + await vi.waitFor(() => expect(states(published)).toHaveLength(2)); + + await runtime.parentCancelled(async () => {}); + expect(states(published)).toEqual([{ state: "running" }, { state: "requires_action" }]); + + // The cancel did not resolve the request: the user can still answer it. + answer.resolve("allow"); + await request; + expect(states(published)).toEqual([ + { state: "running" }, + { state: "requires_action" }, + { state: "running" }, + ]); + }); + + it("does not count a cancel that came before the request opened", async () => { + const { runtime, published } = await runningWorker(); + await runtime.parentCancelled(async () => {}); + const withdrawal = new AbortController(); + const request = withdrawable(runtime, withdrawal); + await vi.waitFor(() => expect(states(published)).toHaveLength(2)); + + withdrawal.abort(); + await request; + + expect(states(published).at(-1)).toEqual({ state: "running" }); + }); + + it.each([ + { first: "withdrawn", last: "answered", running: true }, + { first: "answered", last: "withdrawn", running: false }, + { first: "withdrawn", last: "withdrawn", running: false }, + ])( + "lets the last of two requests decide: $first, then $last", + async ({ first, last, running }) => { + const { runtime, published } = await runningWorker(); + const withdrawals = [new AbortController(), new AbortController()]; + const answers = [Promise.withResolvers(), Promise.withResolvers()]; + const requests = withdrawals.map((withdrawal, index) => + runtime + .awaitingUser("worker-1", () => answers[index].promise, withdrawal.signal) + .catch((error: Error) => error.message), + ); + await vi.waitFor(() => expect(states(published)).toHaveLength(2)); + await runtime.parentCancelled(async () => {}); + + const end = async (index: number, how: string) => { + if (how === "answered") { + answers[index].resolve("allow"); + } else { + withdrawals[index].abort(); + answers[index].reject(new Error("Tool use aborted")); + } + await requests[index]; + }; + await end(0, first); + // Only the last request to end reports. + expect(states(published)).toHaveLength(2); + await end(1, last); + + expect(states(published).at(-1)).toEqual({ + state: running ? "running" : "requires_action", + }); + }, + ); + }); });