diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 7b74a94176b..05f972a1e67 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EventEmitter } from 'node:events'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -226,6 +227,38 @@ describe('Session', () => { getTool: ReturnType; ensureTool: ReturnType; }; + + function mockConfirmingTool( + name: string, + execute: ReturnType, + type: core.ToolCallConfirmationDetails['type'] = 'ask_user_question', + ) { + return { + name, + kind: core.Kind.Other, + displayName: name, + description: name, + build: vi.fn().mockReturnValue({ + params: {}, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type, + title: name, + questions: + type === 'ask_user_question' + ? [{ header: 'Continue?', question: 'Continue?' }] + : undefined, + onConfirm: vi.fn().mockResolvedValue(undefined), + }), + getDescription: vi.fn().mockReturnValue(name), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }; + } + beforeEach(() => { currentModel = 'qwen3-code-plus'; currentAuthType = AuthType.USE_OPENAI; @@ -331,6 +364,7 @@ describe('Session', () => { requestPermission: vi.fn().mockResolvedValue({ outcome: { outcome: 'selected', optionId: 'proceed_once' }, }), + extMethod: vi.fn().mockResolvedValue({ messages: [] }), extNotification: vi.fn().mockResolvedValue(undefined), } as unknown as AgentSideConnection; @@ -5071,6 +5105,299 @@ describe('Session', () => { expect(hasPlanReminder).toBe(false); }); }); + + describe('ask_user_question cancellation turn stop', () => { + function createAskUserQuestionResponseStream() { + return createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + usageMetadata: { + totalTokenCount: 10, + promptTokenCount: 5, + }, + functionCalls: [ + { + id: 'ask-user-question-call', + name: core.ToolNames.ASK_USER_QUESTION, + args: { + questions: [{ header: 'Continue?', question: 'Continue?' }], + }, + }, + ], + }, + }, + ]); + } + + it('waits for pending rewrites before ending after cancelled ask_user_question', async () => { + let releaseRewrite!: () => void; + const flushTurn = vi.fn().mockResolvedValue(undefined); + const waitForPendingRewrites = vi.fn( + () => + new Promise((resolve) => { + releaseRewrite = resolve; + }), + ); + session.messageRewriter = { + interceptUpdate: vi.fn().mockResolvedValue(undefined), + flushTurn, + waitForPendingRewrites, + } as unknown as Session['messageRewriter']; + mockToolRegistry.getTool.mockReturnValue( + mockConfirmingTool(core.ToolNames.ASK_USER_QUESTION, vi.fn()), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createAskUserQuestionResponseStream()); + vi.mocked(mockClient.extMethod).mockResolvedValueOnce({ + messages: ['follow-up while waiting'], + }); + + const promptPromise = session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'question' }], + }); + let promptSettled = false; + void promptPromise.then(() => { + promptSettled = true; + }); + + await vi.waitFor(() => { + expect(waitForPendingRewrites).toHaveBeenCalledTimes(1); + }); + await Promise.resolve(); + + expect(flushTurn).toHaveBeenCalledTimes(1); + expect(promptSettled).toBe(false); + + releaseRewrite(); + await expect(promptPromise).resolves.toEqual({ + stopReason: 'end_turn', + }); + expect(mockChat.addHistory).toHaveBeenCalledWith({ + role: 'user', + parts: [ + expect.objectContaining({ + functionResponse: expect.objectContaining({ + id: 'ask-user-question-call', + name: core.ToolNames.ASK_USER_QUESTION, + }), + }), + { + text: '\n[User message received during tool execution]: follow-up while waiting', + }, + ], + }); + expect( + mockChatRecordingService.recordMidTurnUserMessage, + ).toHaveBeenCalledWith( + [ + { + text: '\n[User message received during tool execution]: follow-up while waiting', + }, + ], + 'follow-up while waiting', + ); + }); + + it('waits for pending rewrites before cron stops after cancelled ask_user_question', async () => { + const scheduler = { + size: 1, + hasPendingWork: true, + start: vi.fn((callback: (job: { prompt: string }) => void) => { + callback({ prompt: 'scheduled question' }); + }), + stop: vi.fn(), + getExitSummary: vi.fn().mockReturnValue(undefined), + }; + mockConfig.isCronEnabled = vi.fn().mockReturnValue(true); + mockConfig.getCronScheduler = vi.fn().mockReturnValue(scheduler); + + let releaseCronRewrite!: () => void; + const waitForPendingRewrites = vi + .fn() + .mockResolvedValueOnce(undefined) + .mockImplementationOnce( + () => + new Promise((resolve) => { + releaseCronRewrite = resolve; + }), + ); + session.messageRewriter = { + interceptUpdate: vi.fn().mockResolvedValue(undefined), + flushTurn: vi.fn().mockResolvedValue(undefined), + waitForPendingRewrites, + } as unknown as Session['messageRewriter']; + mockToolRegistry.getTool.mockReturnValue( + mockConfirmingTool(core.ToolNames.ASK_USER_QUESTION, vi.fn()), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce(createAskUserQuestionResponseStream()); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'start cron' }], + }); + + await vi.waitFor(() => { + expect(waitForPendingRewrites).toHaveBeenCalledTimes(2); + }); + + const internals = session as unknown as { + cronCompletion: Promise | null; + }; + const cronCompletion = internals.cronCompletion; + expect(cronCompletion).toBeTruthy(); + let cronSettled = false; + void cronCompletion?.then(() => { + cronSettled = true; + }); + await Promise.resolve(); + + expect(cronSettled).toBe(false); + + releaseCronRewrite(); + await vi.waitFor(() => { + expect(internals.cronCompletion).toBeNull(); + }); + }); + + it('ends Stop-hook continuation after cancelled ask_user_question', async () => { + const execute = vi.fn(); + mockToolRegistry.getTool.mockReturnValue( + mockConfirmingTool(core.ToolNames.ASK_USER_QUESTION, execute), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + const messageBus = { + request: vi + .fn() + .mockResolvedValueOnce({ + success: true, + output: { + decision: 'block', + reason: 'Continue after Stop hook', + }, + }) + .mockResolvedValueOnce({ + success: true, + output: {}, + }), + }; + mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + mockConfig.hasHooksForEvent = vi + .fn() + .mockImplementation((eventName: string) => eventName === 'Stop'); + mockChat.getHistory = vi + .fn() + .mockReturnValue([ + { role: 'model', parts: [{ text: 'response text' }] }, + ]); + mockChat.getLastModelMessageText = vi + .fn() + .mockReturnValue('response text'); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce(createAskUserQuestionResponseStream()); + + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'hello' }], + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(execute).not.toHaveBeenCalled(); + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2); + expect( + messageBus.request.mock.calls.filter( + ([request]) => + typeof request === 'object' && + request !== null && + 'eventName' in request && + request.eventName === 'Stop', + ), + ).toHaveLength(1); + expect(mockChat.addHistory).toHaveBeenCalledWith({ + role: 'user', + parts: [ + expect.objectContaining({ + functionResponse: expect.objectContaining({ + id: 'ask-user-question-call', + name: core.ToolNames.ASK_USER_QUESTION, + }), + }), + ], + }); + }); + + it('ends background notification processing after cancelled ask_user_question', async () => { + const execute = vi.fn(); + mockToolRegistry.getTool.mockReturnValue( + mockConfirmingTool(core.ToolNames.ASK_USER_QUESTION, execute), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce(createAskUserQuestionResponseStream()); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'start background work' }], + }); + + const callback = mockBackgroundTaskRegistry.setNotificationCallback.mock + .calls[0][0] as ( + displayText: string, + modelText: string, + meta: { agentId: string; status: string; toolUseId?: string }, + ) => void; + + callback('done', '', { + agentId: 'agent-1', + status: 'completed', + }); + + await vi.waitFor(() => { + expect(mockClient.extNotification).toHaveBeenCalledWith( + '_qwencode/end_turn', + { + sessionId: 'test-session-id', + reason: 'end_turn', + source: 'background_notification', + }, + ); + }); + + expect(execute).not.toHaveBeenCalled(); + expect(mockChat.addHistory).toHaveBeenCalledWith({ + role: 'user', + parts: [ + expect.objectContaining({ + functionResponse: expect.objectContaining({ + id: 'ask-user-question-call', + name: core.ToolNames.ASK_USER_QUESTION, + }), + }), + ], + }); + }); + }); }); describe('runToolCalls', () => { @@ -5079,51 +5406,1026 @@ describe('Session', () => { abortSignal: AbortSignal, promptId: string, functionCalls: FunctionCall[], - ) => Promise; + ) => Promise<{ + parts: Part[]; + stopAfterUserQuestionCancel: boolean; + }>; }; - it('executes only the first duplicate functionCall id in one batch', async () => { - const execute = vi.fn().mockResolvedValue({ - llmContent: 'first result', - returnDisplay: 'first result', + function emitNestedAskUserQuestion( + eventEmitter: EventEmitter, + respond: ReturnType, + ) { + eventEmitter.emit(core.AgentEventType.TOOL_WAITING_APPROVAL, { + subagentId: 'subagent-1', + round: 1, + callId: 'nested_question', + name: core.ToolNames.ASK_USER_QUESTION, + description: 'Ask user', + args: {}, + confirmationDetails: { + type: 'ask_user_question', + title: 'Question', + questions: [{ header: 'Continue?', question: 'Continue?' }], + }, + respond, + timestamp: Date.now(), }); - mockToolRegistry.getTool.mockReturnValue({ - name: 'read_file', + } + + function waitForAbortOrTick(signal: AbortSignal): Promise { + return new Promise((resolve) => { + if (signal.aborted) { + resolve(); + return; + } + const timeout = setTimeout(resolve, 10); + signal.addEventListener( + 'abort', + () => { + clearTimeout(timeout); + resolve(); + }, + { once: true }, + ); + }); + } + + function mockAllowedTool(name: string, execute: ReturnType) { + return { + name, kind: core.Kind.Read, - displayName: 'Read File', - description: 'Read file', + displayName: name, + description: name, build: vi.fn().mockReturnValue({ - params: { file_path: 'a.ts' }, + params: {}, execute, getDefaultPermission: vi.fn().mockResolvedValue('allow'), - getDescription: vi.fn().mockReturnValue('Read file'), + getDescription: vi.fn().mockReturnValue(name), toolLocations: vi.fn().mockReturnValue([]), }), canUpdateOutput: false, isOutputMarkdown: true, + }; + } + + it('marks cancelled ask_user_question as a turn stop', async () => { + const execute = vi.fn().mockResolvedValue({ + llmContent: 'should not execute', + returnDisplay: 'should not execute', + }); + mockToolRegistry.getTool.mockReturnValue( + mockConfirmingTool(core.ToolNames.ASK_USER_QUESTION, execute), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-question-cancel', [ + { + id: 'question_call', + name: core.ToolNames.ASK_USER_QUESTION, + args: { questions: [{ header: 'Continue?', question: 'Continue?' }] }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(result.parts).toHaveLength(1); + expect(result.parts[0]?.functionResponse?.id).toBe('question_call'); + expect(result.parts[0]?.functionResponse?.response).toEqual({ + error: `Tool "${core.ToolNames.ASK_USER_QUESTION}" was canceled by the user.`, + }); + expect(execute).not.toHaveBeenCalled(); + }); + + it('skips later sequential tools after cancelled ask_user_question', async () => { + const questionExecute = vi.fn(); + const shellExecute = vi.fn().mockResolvedValue({ + llmContent: 'shell result', + returnDisplay: 'shell result', + }); + mockToolRegistry.getTool.mockImplementation((name: string) => + name === core.ToolNames.ASK_USER_QUESTION + ? mockConfirmingTool(name, questionExecute) + : mockAllowedTool(name, shellExecute), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-question-shell', [ + { + id: 'question_call', + name: core.ToolNames.ASK_USER_QUESTION, + args: { questions: [{ header: 'Continue?', question: 'Continue?' }] }, + }, + { + id: 'shell_call', + name: core.ToolNames.SHELL, + args: { command: 'echo should-not-run' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(questionExecute).not.toHaveBeenCalled(); + expect(shellExecute).not.toHaveBeenCalled(); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'question_call', + 'shell_call', + ]); + expect(result.parts[1]?.functionResponse?.response).toEqual({ + error: + 'Skipped because ask_user_question was cancelled before the user answered; user input is required before continuing.', + }); + expect(mockChatRecordingService.recordToolResult).toHaveBeenCalledWith( + [result.parts[1]], + expect.objectContaining({ + callId: 'shell_call', + status: 'error', + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith({ + sessionId: 'test-session-id', + update: expect.objectContaining({ + sessionUpdate: 'tool_call_update', + toolCallId: 'shell_call', + status: 'failed', + _meta: expect.objectContaining({ + toolName: core.ToolNames.SHELL, + }), + }), + }); + const shellUpdates = vi + .mocked(mockClient.sessionUpdate) + .mock.calls.map(([params]) => params.update) + .filter( + (update) => + 'toolCallId' in update && update.toolCallId === 'shell_call', + ); + expect( + shellUpdates.map((update) => ({ + sessionUpdate: update.sessionUpdate, + status: 'status' in update ? update.status : undefined, + })), + ).toEqual([ + { sessionUpdate: 'tool_call', status: 'pending' }, + { sessionUpdate: 'tool_call_update', status: 'failed' }, + ]); + }); + + it('preserves skipped tool responses when skipped tool updates fail', async () => { + const questionExecute = vi.fn(); + const shellExecute = vi.fn().mockResolvedValue({ + llmContent: 'shell result', + returnDisplay: 'shell result', }); + mockToolRegistry.getTool.mockImplementation((name: string) => + name === core.ToolNames.ASK_USER_QUESTION + ? mockConfirmingTool(name, questionExecute) + : mockAllowedTool(name, shellExecute), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + vi.mocked(mockClient.sessionUpdate).mockImplementation( + async ({ update }) => { + if ( + 'toolCallId' in update && + update.toolCallId === 'shell_call' && + update.sessionUpdate === 'tool_call' + ) { + throw new Error('client disconnected'); + } + }, + ); - const parts = await (session as unknown as ToolCallInternals).runToolCalls( + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls( new AbortController().signal, - 'prompt-dup', + 'prompt-question-shell-disconnect', [ { - id: 'dup_id_0001', - name: 'read_file', - args: { file_path: 'a.ts' }, + id: 'question_call', + name: core.ToolNames.ASK_USER_QUESTION, + args: { + questions: [{ header: 'Continue?', question: 'Continue?' }], + }, }, { - id: 'dup_id_0001', - name: 'read_file', - args: { file_path: 'b.ts' }, + id: 'shell_call', + name: core.ToolNames.SHELL, + args: { command: 'echo should-not-run' }, }, ], ); + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(shellExecute).not.toHaveBeenCalled(); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'question_call', + 'shell_call', + ]); + expect(result.parts[1]?.functionResponse?.response).toEqual({ + error: + 'Skipped because ask_user_question was cancelled before the user answered; user input is required before continuing.', + }); + }); + + it('uses stable unique ids for skipped tool calls without ids', async () => { + const questionExecute = vi.fn(); + const shellExecute = vi.fn().mockResolvedValue({ + llmContent: 'shell result', + returnDisplay: 'shell result', + }); + mockToolRegistry.getTool.mockImplementation((name: string) => + name === core.ToolNames.ASK_USER_QUESTION + ? mockConfirmingTool(name, questionExecute) + : mockAllowedTool(name, shellExecute), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-skip-no-ids', [ + { + id: 'question_call', + name: core.ToolNames.ASK_USER_QUESTION, + args: { questions: [{ header: 'Continue?', question: 'Continue?' }] }, + }, + { + name: core.ToolNames.SHELL, + args: { command: 'echo first' }, + }, + { + name: core.ToolNames.SHELL, + args: { command: 'echo second' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'question_call', + `${core.ToolNames.SHELL}-skip-1`, + `${core.ToolNames.SHELL}-skip-2`, + ]); + }); + + it('does not stop the turn for non-question permission cancellation', async () => { + const execute = vi.fn(); + mockToolRegistry.getTool.mockReturnValue( + mockConfirmingTool(core.ToolNames.SHELL, execute, 'exec'), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-shell-cancel', [ + { + id: 'shell_call', + name: core.ToolNames.SHELL, + args: { command: 'echo denied' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(false); + expect(result.parts).toHaveLength(1); + expect(result.parts[0]?.functionResponse?.id).toBe('shell_call'); + expect(execute).not.toHaveBeenCalled(); + }); + + it('stops and aborts Agent tool execution after nested ask_user_question cancellation', async () => { + const eventEmitter = new EventEmitter(); + let executeSignal: AbortSignal | undefined; + const respond = vi.fn().mockResolvedValue(undefined); + const execute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + executeSignal = signal; + emitNestedAskUserQuestion(eventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + }; + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockReturnValue({ + params: { subagent_type: 'explore' }, + eventEmitter, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-agent-question', [ + { + id: 'agent_call', + name: core.ToolNames.AGENT, + args: { subagent_type: 'explore' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(executeSignal?.aborted).toBe(true); + expect(respond).toHaveBeenCalledWith( + core.ToolConfirmationOutcome.Cancel, + { + answers: undefined, + }, + ); + expect(result.parts[0]?.functionResponse?.id).toBe('agent_call'); + }); + + it('ignores later subagent tool events after nested ask_user_question cancellation', async () => { + const eventEmitter = new EventEmitter(); + const respond = vi.fn().mockResolvedValue(undefined); + const execute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + emitNestedAskUserQuestion(eventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + eventEmitter.emit(core.AgentEventType.TOOL_RESULT, { + subagentId: 'subagent-1', + round: 1, + callId: 'late_tool', + name: core.ToolNames.SHELL, + success: true, + responseParts: [{ text: 'late result' }], + resultDisplay: 'late result', + timestamp: Date.now(), + }); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + }; + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockReturnValue({ + params: { subagent_type: 'explore' }, + eventEmitter, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-agent-late-event', [ + { + id: 'agent_call', + name: core.ToolNames.AGENT, + args: { subagent_type: 'explore' }, + }, + ]); + + await Promise.resolve(); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + const subagentUpdates = vi + .mocked(mockClient.sessionUpdate) + .mock.calls.map(([params]) => params.update) + .filter( + (update) => + update.sessionUpdate === 'tool_call_update' && + update._meta?.provenance === 'subagent', + ); + expect(subagentUpdates).toEqual([]); + }); + + it('aborts sibling Agent calls in the same batch after nested ask_user_question cancellation', async () => { + const questionEventEmitter = new EventEmitter(); + const siblingEventEmitter = new EventEmitter(); + let siblingSignal: AbortSignal | undefined; + const respond = vi.fn().mockResolvedValue(undefined); + const questionExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + emitNestedAskUserQuestion(questionEventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + }; + }); + const siblingExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + siblingSignal = signal; + await waitForAbortOrTick(signal); + return { + llmContent: 'sibling stopped', + returnDisplay: 'sibling stopped', + }; + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockImplementation((args: Record) => { + const isQuestionAgent = args['_test_id'] === 'question'; + return { + params: { subagent_type: 'explore', ...args }, + eventEmitter: isQuestionAgent + ? questionEventEmitter + : siblingEventEmitter, + execute: isQuestionAgent ? questionExecute : siblingExecute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }; + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-agent-siblings', [ + { + id: 'agent_question', + name: core.ToolNames.AGENT, + args: { _test_id: 'question', subagent_type: 'explore' }, + }, + { + id: 'agent_sibling', + name: core.ToolNames.AGENT, + args: { _test_id: 'sibling', subagent_type: 'explore' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(questionExecute).toHaveBeenCalledOnce(); + expect(siblingExecute).toHaveBeenCalledOnce(); + expect(siblingSignal?.aborted).toBe(true); + }); + + it('passes an already-aborted parent signal to Agent batches', async () => { + const eventEmitter = new EventEmitter(); + const receivedAbortStates: boolean[] = []; + const execute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + receivedAbortStates.push(signal.aborted); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + }; + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockReturnValue({ + params: { subagent_type: 'explore' }, + eventEmitter, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + const parentAbort = new AbortController(); + parentAbort.abort('parent cancelled'); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(parentAbort.signal, 'prompt-agent-pre-aborted', [ + { + id: 'agent_first', + name: core.ToolNames.AGENT, + args: { subagent_type: 'explore' }, + }, + { + id: 'agent_second', + name: core.ToolNames.AGENT, + args: { subagent_type: 'explore' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(false); + expect(execute).toHaveBeenCalledTimes(2); + expect(receivedAbortStates).toEqual([true, true]); + }); + + it('skips unstarted Agent calls after nested ask_user_question cancellation', async () => { + const previousMaxConcurrency = + process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY']; + process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'] = '1'; + try { + const eventEmitter = new EventEmitter(); + const respond = vi.fn().mockResolvedValue(undefined); + const questionExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + emitNestedAskUserQuestion(eventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + }; + }); + const secondExecute = vi.fn(); + const thirdExecute = vi.fn(); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockImplementation((args: Record) => { + const id = args['_test_id']; + return { + params: { subagent_type: 'explore', ...args }, + eventEmitter, + execute: + id === 'question' + ? questionExecute + : id === 'second' + ? secondExecute + : thirdExecute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }; + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-agent-unstarted', [ + { + id: 'agent_question', + name: core.ToolNames.AGENT, + args: { _test_id: 'question', subagent_type: 'explore' }, + }, + { + id: 'agent_second', + name: core.ToolNames.AGENT, + args: { _test_id: 'second', subagent_type: 'explore' }, + }, + { + id: 'agent_third', + name: core.ToolNames.AGENT, + args: { _test_id: 'third', subagent_type: 'explore' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(questionExecute).toHaveBeenCalledOnce(); + expect(secondExecute).not.toHaveBeenCalled(); + expect(thirdExecute).not.toHaveBeenCalled(); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'agent_question', + 'agent_second', + 'agent_third', + ]); + expect(result.parts[1]?.functionResponse?.response).toEqual({ + error: + 'Skipped because ask_user_question was cancelled before the user answered; user input is required before continuing.', + }); + expect(result.parts[2]?.functionResponse?.response).toEqual({ + error: + 'Skipped because ask_user_question was cancelled before the user answered; user input is required before continuing.', + }); + } finally { + if (previousMaxConcurrency === undefined) { + delete process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY']; + } else { + process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'] = + previousMaxConcurrency; + } + } + }); + + it('skips later sequential batches after nested ask_user_question cancellation', async () => { + const questionEventEmitter = new EventEmitter(); + const siblingEventEmitter = new EventEmitter(); + const respond = vi.fn().mockResolvedValue(undefined); + const questionExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + emitNestedAskUserQuestion(questionEventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + }; + }); + const siblingExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + await waitForAbortOrTick(signal); + return { + llmContent: 'sibling stopped', + returnDisplay: 'sibling stopped', + }; + }); + const shellExecute = vi.fn().mockResolvedValue({ + llmContent: 'shell result', + returnDisplay: 'shell result', + }); + mockToolRegistry.getTool.mockImplementation((name: string) => { + if (name !== core.ToolNames.AGENT) { + return mockAllowedTool(name, shellExecute); + } + return { + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockImplementation((args: Record) => { + const isQuestionAgent = args['_test_id'] === 'question'; + return { + params: { subagent_type: 'explore', ...args }, + eventEmitter: isQuestionAgent + ? questionEventEmitter + : siblingEventEmitter, + execute: isQuestionAgent ? questionExecute : siblingExecute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }; + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }; + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-agent-then-shell', [ + { + id: 'agent_question', + name: core.ToolNames.AGENT, + args: { _test_id: 'question', subagent_type: 'explore' }, + }, + { + id: 'agent_sibling', + name: core.ToolNames.AGENT, + args: { _test_id: 'sibling', subagent_type: 'explore' }, + }, + { + id: 'shell_after', + name: core.ToolNames.SHELL, + args: { command: 'echo should-not-run' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(questionExecute).toHaveBeenCalledOnce(); + expect(siblingExecute).toHaveBeenCalledOnce(); + expect(shellExecute).not.toHaveBeenCalled(); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'agent_question', + 'agent_sibling', + 'shell_after', + ]); + expect(result.parts[2]?.functionResponse?.response).toEqual({ + error: + 'Skipped because ask_user_question was cancelled before the user answered; user input is required before continuing.', + }); + }); + + it('does not fire success hooks for sibling Agents aborted by nested ask_user_question cancellation', async () => { + const messageBus = { + request: vi.fn().mockResolvedValue({ + success: true, + output: {}, + }), + }; + mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + const questionEventEmitter = new EventEmitter(); + const siblingEventEmitter = new EventEmitter(); + const respond = vi.fn().mockResolvedValue(undefined); + const questionExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + emitNestedAskUserQuestion(questionEventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + }; + }); + const siblingExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + await waitForAbortOrTick(signal); + return { + llmContent: 'sibling stopped', + returnDisplay: 'sibling stopped', + }; + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockImplementation((args: Record) => { + const isQuestionAgent = args['_test_id'] === 'question'; + return { + params: { subagent_type: 'explore', ...args }, + eventEmitter: isQuestionAgent + ? questionEventEmitter + : siblingEventEmitter, + execute: isQuestionAgent ? questionExecute : siblingExecute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }; + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls( + new AbortController().signal, + 'prompt-agent-sibling-hooks', + [ + { + id: 'agent_question', + name: core.ToolNames.AGENT, + args: { _test_id: 'question', subagent_type: 'explore' }, + }, + { + id: 'agent_sibling', + name: core.ToolNames.AGENT, + args: { _test_id: 'sibling', subagent_type: 'explore' }, + }, + ], + ); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + const hookRequests = messageBus.request.mock.calls.map(([request]) => { + const eventName = + typeof request === 'object' && + request !== null && + 'eventName' in request + ? request.eventName + : undefined; + const input = + typeof request === 'object' && request !== null && 'input' in request + ? request.input + : undefined; + return { eventName, input }; + }); + expect( + hookRequests.filter(({ eventName }) => eventName === 'PostToolUse'), + ).toEqual([]); + expect(hookRequests).toContainEqual( + expect.objectContaining({ + eventName: 'PostToolUseFailure', + input: expect.objectContaining({ + is_interrupt: true, + }), + }), + ); + }); + + it('marks Agent exceptions after nested ask_user_question cancellation as interrupts', async () => { + const messageBus = { + request: vi.fn().mockResolvedValue({ + success: true, + output: {}, + }), + }; + mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + const eventEmitter = new EventEmitter(); + const respond = vi.fn().mockResolvedValue(undefined); + const execute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + emitNestedAskUserQuestion(eventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + throw new Error('agent aborted after question cancel'); + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockReturnValue({ + params: { subagent_type: 'explore' }, + eventEmitter, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-agent-interrupt', [ + { + id: 'agent_call', + name: core.ToolNames.AGENT, + args: { subagent_type: 'explore' }, + }, + ]); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(messageBus.request).toHaveBeenCalledWith( + expect.objectContaining({ + eventName: 'PostToolUseFailure', + input: expect.objectContaining({ + is_interrupt: true, + }), + signal: expect.objectContaining({ + aborted: true, + }), + }), + expect.anything(), + ); + }); + + it('marks Agent soft errors after nested ask_user_question cancellation as interrupts', async () => { + const messageBus = { + request: vi.fn().mockResolvedValue({ + success: true, + output: {}, + }), + }; + mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + const eventEmitter = new EventEmitter(); + const respond = vi.fn().mockResolvedValue(undefined); + const execute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + emitNestedAskUserQuestion(eventEmitter, respond); + await vi.waitFor(() => { + expect(signal.aborted).toBe(true); + }); + return { + llmContent: 'agent stopped', + returnDisplay: 'agent stopped', + error: { message: 'agent aborted after question cancel' }, + }; + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockReturnValue({ + params: { subagent_type: 'explore' }, + eventEmitter, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls( + new AbortController().signal, + 'prompt-agent-soft-interrupt', + [ + { + id: 'agent_call', + name: core.ToolNames.AGENT, + args: { subagent_type: 'explore' }, + }, + ], + ); + + expect(result.stopAfterUserQuestionCancel).toBe(true); + expect(messageBus.request).toHaveBeenCalledWith( + expect.objectContaining({ + eventName: 'PostToolUseFailure', + input: expect.objectContaining({ + is_interrupt: true, + }), + signal: expect.objectContaining({ + aborted: true, + }), + }), + expect.anything(), + ); + }); + + it('executes only the first duplicate functionCall id in one batch', async () => { + const execute = vi.fn().mockResolvedValue({ + llmContent: 'first result', + returnDisplay: 'first result', + }); + mockToolRegistry.getTool.mockReturnValue({ + name: 'read_file', + kind: core.Kind.Read, + displayName: 'Read File', + description: 'Read file', + build: vi.fn().mockReturnValue({ + params: { file_path: 'a.ts' }, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Read file'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-dup', [ + { + id: 'dup_id_0001', + name: 'read_file', + args: { file_path: 'a.ts' }, + }, + { + id: 'dup_id_0001', + name: 'read_file', + args: { file_path: 'b.ts' }, + }, + ]); + expect(execute).toHaveBeenCalledOnce(); - expect(parts.map((part) => part.functionResponse?.id)).toEqual([ + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ 'dup_id_0001', ]); + expect(result.stopAfterUserQuestionCancel).toBe(false); expect(mockChatRecordingService.recordToolResult).toHaveBeenCalledOnce(); }); @@ -5148,25 +6450,24 @@ describe('Session', () => { isOutputMarkdown: true, }); - const parts = await (session as unknown as ToolCallInternals).runToolCalls( - new AbortController().signal, - 'prompt-empty', - [ - { - id: '', - name: 'read_file', - args: { file_path: 'a.ts' }, - }, - { - id: '', - name: 'read_file', - args: { file_path: 'b.ts' }, - }, - ], - ); + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls(new AbortController().signal, 'prompt-empty', [ + { + id: '', + name: 'read_file', + args: { file_path: 'a.ts' }, + }, + { + id: '', + name: 'read_file', + args: { file_path: 'b.ts' }, + }, + ]); expect(execute).toHaveBeenCalledTimes(2); - expect(parts).toHaveLength(2); + expect(result.parts).toHaveLength(2); + expect(result.stopAfterUserQuestionCancel).toBe(false); expect(mockChatRecordingService.recordToolResult).toHaveBeenCalledTimes( 2, ); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 12d791d8c8f..d1e3b799f69 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -171,6 +171,14 @@ type AutoCompressionSendResult = | { responseStream: AsyncGenerator; stopReason?: never } | { responseStream: null; stopReason: PromptResponse['stopReason'] }; +type RunToolResult = { + parts: Part[]; + stopAfterUserQuestionCancel: boolean; +}; + +const ASK_USER_QUESTION_CANCEL_SKIP_MESSAGE = + 'Skipped because ask_user_question was cancelled before the user answered; user input is required before continuing.'; + // The drain is served from an in-memory queue, so a conforming client answers // near-instantly (or rejects with -32601). No response within this window // means the client silently drops unknown methods; without a deadline the @@ -1318,15 +1326,21 @@ export class Session implements SessionContext { } if (functionCalls.length > 0) { - const toolResponseParts = await this.runToolCalls( + const toolRun = await this.runToolCalls( pendingSend.signal, promptId, functionCalls, ); + if (toolRun.stopAfterUserQuestionCancel) { + await this.#preserveCancelledAskUserQuestionToolRun( + toolRun, + ); + return { stopReason: 'end_turn' }; + } nextMessage = { role: 'user', parts: [ - ...toolResponseParts, + ...toolRun.parts, ...(await this.#drainMidTurnUserMessages()), ], }; @@ -1583,15 +1597,19 @@ export class Session implements SessionContext { // Process tool calls from the follow-up message if (functionCalls.length > 0) { - const toolResponseParts = await this.runToolCalls( + const toolRun = await this.runToolCalls( pendingSend.signal, promptId, functionCalls, ); + if (toolRun.stopAfterUserQuestionCancel) { + await this.#preserveCancelledAskUserQuestionToolRun(toolRun); + return { stopReason: 'end_turn' }; + } nextMessage = { role: 'user', parts: [ - ...toolResponseParts, + ...toolRun.parts, ...(await this.#drainMidTurnUserMessages()), ], }; @@ -1755,6 +1773,19 @@ export class Session implements SessionContext { } } + async #preserveCancelledAskUserQuestionToolRun( + toolRun: RunToolResult, + ): Promise { + this.#preserveUnsentMessageHistory( + { + role: 'user', + parts: [...toolRun.parts, ...(await this.#drainMidTurnUserMessages())], + }, + true, + ); + await this.messageRewriter?.waitForPendingRewrites(); + } + #recordCompressionTokenCount(info: ChatCompressionInfo): void { this.#syncPromptTokenCountWithCurrentChat(); const tokenCount = this.#extractCompressionTokenCount(info); @@ -2157,15 +2188,21 @@ export class Session implements SessionContext { } if (functionCalls.length > 0) { - const toolResponseParts = await this.runToolCalls( + const toolRun = await this.runToolCalls( ac.signal, promptId, functionCalls, ); + if (toolRun.stopAfterUserQuestionCancel) { + await this.#preserveCancelledAskUserQuestionToolRun( + toolRun, + ); + return; + } nextMessage = { role: 'user', parts: [ - ...toolResponseParts, + ...toolRun.parts, ...(await this.#drainMidTurnUserMessages()), ], }; @@ -2463,15 +2500,20 @@ export class Session implements SessionContext { } if (functionCalls.length > 0) { - const toolResponseParts = await this.runToolCalls( + const toolRun = await this.runToolCalls( ac.signal, promptId, functionCalls, ); + if (toolRun.stopAfterUserQuestionCancel) { + await this.#preserveCancelledAskUserQuestionToolRun(toolRun); + await this.#emitBackgroundNotificationEndTurn('end_turn'); + return; + } nextMessage = { role: 'user', parts: [ - ...toolResponseParts, + ...toolRun.parts, ...(await this.#drainMidTurnUserMessages()), ], }; @@ -2805,10 +2847,11 @@ export class Session implements SessionContext { abortSignal: AbortSignal, promptId: string, functionCalls: FunctionCall[], - ): Promise { + ): Promise { + const dedupedFunctionCalls = dedupeToolCallsById(functionCalls); type Batch = { concurrent: boolean; calls: FunctionCall[] }; const batches: Batch[] = []; - for (const fc of dedupeToolCallsById(functionCalls)) { + for (const fc of dedupedFunctionCalls) { const isAgent = fc.name === ToolNames.AGENT; const last = batches[batches.length - 1]; if (isAgent && last?.concurrent) { @@ -2818,22 +2861,79 @@ export class Session implements SessionContext { } } + let skippedToolCallCounter = 0; + const recordSkippedToolCall = async (fc: FunctionCall): Promise => { + const toolName = fc.name ?? 'unknown_tool'; + const callId = fc.id ?? `${toolName}-skip-${++skippedToolCallCounter}`; + const part: Part = { + functionResponse: { + id: callId, + name: toolName, + response: { error: ASK_USER_QUESTION_CANCEL_SKIP_MESSAGE }, + }, + }; + const error = new Error(ASK_USER_QUESTION_CANCEL_SKIP_MESSAGE); + try { + this.config.getChatRecordingService()?.recordToolResult([part], { + callId, + status: 'error', + resultDisplay: undefined, + error, + errorType: undefined, + }); + await this.toolCallEmitter.emitStart({ + callId, + toolName, + args: (fc.args ?? {}) as Record, + status: 'pending', + }); + await this.toolCallEmitter.emitError(callId, toolName, error); + } catch (recordError) { + debugLogger.error('Failed to record skipped tool call:', recordError); + } + return part; + }; + + const appendSkippedAfter = async (parts: Part[], fc: FunctionCall) => { + const startIndex = dedupedFunctionCalls.indexOf(fc) + 1; + for (const remainingCall of dedupedFunctionCalls.slice(startIndex)) { + parts.push(await recordSkippedToolCall(remainingCall)); + } + }; + // Bounded-concurrency runner: matches core's `runConcurrently` // behaviour (`coreToolScheduler.ts:1506`), capped by // `QWEN_CODE_MAX_TOOL_CONCURRENCY` (default 10). Results are returned // in input order regardless of resolution order. - const runBounded = async (calls: FunctionCall[]): Promise => { + const runBounded = async ( + calls: FunctionCall[], + runAbortSignal: AbortSignal, + onStopAfterUserQuestionCancel?: () => void, + shouldSkipUnstarted?: () => boolean, + ): Promise => { const parsed = parseInt( process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'] || '', 10, ); const maxConcurrency = Number.isFinite(parsed) && parsed >= 1 ? parsed : 10; - const results: Part[][] = new Array(calls.length); + const results: RunToolResult[] = new Array(calls.length); const executing = new Set>(); for (let i = 0; i < calls.length; i++) { const idx = i; - const p = this.runTool(abortSignal, promptId, calls[idx]) + if (runAbortSignal.aborted && shouldSkipUnstarted?.()) { + results[idx] = { + parts: [await recordSkippedToolCall(calls[idx])], + stopAfterUserQuestionCancel: false, + }; + continue; + } + const p = this.runTool( + runAbortSignal, + promptId, + calls[idx], + onStopAfterUserQuestionCancel, + ) .then((r) => { results[idx] = r; }) @@ -2852,16 +2952,54 @@ export class Session implements SessionContext { const parts: Part[] = []; for (const batch of batches) { if (batch.concurrent && batch.calls.length > 1) { - const results = await runBounded(batch.calls); - for (const r of results) parts.push(...r); + const batchAbortController = new AbortController(); + let batchStopAfterUserQuestionCancel = false; + const propagateAbort = () => { + batchAbortController.abort(abortSignal.reason); + }; + if (abortSignal.aborted) { + propagateAbort(); + } else { + abortSignal.addEventListener('abort', propagateAbort, { + once: true, + }); + } + const stopBatchAfterUserQuestionCancel = () => { + batchStopAfterUserQuestionCancel = true; + batchAbortController.abort(USER_CANCEL_ABORT_REASON); + }; + let results: RunToolResult[]; + try { + results = await runBounded( + batch.calls, + batchAbortController.signal, + stopBatchAfterUserQuestionCancel, + () => batchStopAfterUserQuestionCancel, + ); + } finally { + abortSignal.removeEventListener('abort', propagateAbort); + } + let shouldStop = false; + for (const r of results) { + parts.push(...r.parts); + shouldStop ||= r.stopAfterUserQuestionCancel; + } + if (shouldStop) { + await appendSkippedAfter(parts, batch.calls[batch.calls.length - 1]); + return { parts, stopAfterUserQuestionCancel: true }; + } } else { for (const fc of batch.calls) { const r = await this.runTool(abortSignal, promptId, fc); - parts.push(...r); + parts.push(...r.parts); + if (r.stopAfterUserQuestionCancel) { + await appendSkippedAfter(parts, fc); + return { parts, stopAfterUserQuestionCancel: true }; + } } } } - return parts; + return { parts, stopAfterUserQuestionCancel: false }; } /** @@ -2903,12 +3041,17 @@ export class Session implements SessionContext { abortSignal: AbortSignal, promptId: string, fc: FunctionCall, - ): Promise { + onStopAfterUserQuestionCancel?: () => void, + ): Promise { const callId = fc.id ?? `${fc.name}-${Date.now()}`; let args = (fc.args ?? {}) as Record; const startTime = Date.now(); let spanError: string | undefined; + let activeToolAbortSignal = abortSignal; + let nestedAskUserQuestionCancelled = false; + let agentToolAbortController: AbortController | undefined; + let removeAgentToolAbortPropagation: (() => void) | undefined; const errorResponse = (error: Error) => { const durationMs = Date.now() - startTime; @@ -2920,7 +3063,7 @@ export class Session implements SessionContext { function_args: args, duration_ms: durationMs, // An aborted signal means the call was cancelled, not a genuine error. - status: abortSignal.aborted ? 'cancelled' : 'error', + status: activeToolAbortSignal.aborted ? 'cancelled' : 'error', success: false, error: error.message, tool_type: @@ -2943,8 +3086,10 @@ export class Session implements SessionContext { const earlyErrorResponse = async ( error: Error, toolName = fc.name ?? 'unknown_tool', + opts?: { stopAfterUserQuestionCancel?: boolean }, ) => { spanError = error.message; + removeAgentToolAbortPropagation?.(); if (toolName !== ToolNames.TODO_WRITE) { await this.toolCallEmitter.emitError(callId, toolName, error); } @@ -2957,7 +3102,10 @@ export class Session implements SessionContext { error, errorType: undefined, }); - return errorParts; + return { + parts: errorParts, + stopAfterUserQuestionCancel: opts?.stopAfterUserQuestionCancel ?? false, + }; }; if (!fc.name) { @@ -3000,6 +3148,23 @@ export class Session implements SessionContext { const isAgentTool = tool.name === ToolNames.AGENT; const isExitPlanModeTool = tool.name === ToolNames.EXIT_PLAN_MODE; const isEnterPlanModeTool = tool.name === ToolNames.ENTER_PLAN_MODE; + if (isAgentTool) { + agentToolAbortController = new AbortController(); + activeToolAbortSignal = agentToolAbortController.signal; + const propagateAbort = () => { + agentToolAbortController?.abort(abortSignal.reason); + }; + if (abortSignal.aborted) { + propagateAbort(); + } else { + abortSignal.addEventListener('abort', propagateAbort, { + once: true, + }); + removeAgentToolAbortPropagation = () => { + abortSignal.removeEventListener('abort', propagateAbort); + }; + } + } // Track cleanup functions for sub-agent event listeners let subAgentCleanupFunctions: Array<() => void> = []; @@ -3036,12 +3201,17 @@ export class Session implements SessionContext { this.client, parentToolCallId, subagentType, + () => { + nestedAskUserQuestionCancelled = true; + agentToolAbortController?.abort(USER_CANCEL_ABORT_REASON); + onStopAfterUserQuestionCancel?.(); + }, ); // Set up sub-agent tool tracking subAgentCleanupFunctions = subSubAgentTracker.setup( taskEventEmitter, - abortSignal, + activeToolAbortSignal, ); } @@ -3388,6 +3558,9 @@ export class Session implements SessionContext { switch (outcome) { case ToolConfirmationOutcome.Cancel: + if (toolName === ToolNames.ASK_USER_QUESTION) { + onStopAfterUserQuestionCancel?.(); + } // Route through earlyErrorResponse so spanError carries the // cancellation reason (plain errorResponse leaves it unset, // which makes endToolSpan fall back to the generic 'tool @@ -3395,6 +3568,10 @@ export class Session implements SessionContext { return earlyErrorResponse( new Error(`Tool "${toolName}" was canceled by the user.`), toolName, + { + stopAfterUserQuestionCancel: + toolName === ToolNames.ASK_USER_QUESTION, + }, ); case ToolConfirmationOutcome.ProceedOnce: case ToolConfirmationOutcome.ProceedAlways: @@ -3437,7 +3614,7 @@ export class Session implements SessionContext { args, toolUseId, permissionMode, - abortSignal, + activeToolAbortSignal, ); if (!preHookResult.shouldProceed) { @@ -3468,11 +3645,11 @@ export class Session implements SessionContext { `Qwen Code is executing tool ${toolName}`, ); try { - toolResult = await invocation.execute(abortSignal); + toolResult = await invocation.execute(activeToolAbortSignal); } finally { sleepInhibitorHandle.release(); } - const aborted = abortSignal.aborted; + const aborted = activeToolAbortSignal.aborted; endToolExecutionSpan(execSpan, { success: !toolResult.error && !aborted, error: aborted @@ -3485,14 +3662,17 @@ export class Session implements SessionContext { } catch (execError) { endToolExecutionSpan(execSpan, { success: false, - error: abortSignal.aborted ? 'tool_cancelled' : 'tool_exception', - cancelled: abortSignal.aborted, + error: activeToolAbortSignal.aborted + ? 'tool_cancelled' + : 'tool_exception', + cancelled: activeToolAbortSignal.aborted, }); throw execError; } // Clean up event listeners subAgentCleanupFunctions.forEach((cleanup) => cleanup()); + removeAgentToolAbortPropagation?.(); // enter_plan_mode and the AUTO/YOLO gate path of exit_plan_mode change the // approval mode inside execute() without going through the user-confirmation @@ -3518,8 +3698,29 @@ export class Session implements SessionContext { toolResult.llmContent, ); + // A tool can fail "softly" by returning toolResult.error without + // throwing, and can be cancelled mid-flight. Compute the real outcome + // once and reflect it on hooks, the client-facing emitResult, + // logToolCall / recordToolResult / the tool span, instead of + // hardcoding success — otherwise failed/cancelled daemon/ACP tools + // are mislabeled as successful in telemetry, session replay, and the + // client UI. + const aborted = activeToolAbortSignal.aborted; + const status: 'success' | 'error' | 'cancelled' = aborted + ? 'cancelled' + : toolResult.error + ? 'error' + : 'success'; + const succeeded = status === 'success'; + // Fire PostToolUse hook on successful execution (aligned with core path) - if (hooksEnabledForTool && messageBusForTool && !toolResult.error) { + if ( + hooksEnabledForTool && + messageBusForTool && + !toolResult.error && + !aborted && + !nestedAskUserQuestionCancelled + ) { // Use the same response shape as core (llmContent/returnDisplay) const toolResponse = { llmContent: toolResult.llmContent, @@ -3532,7 +3733,7 @@ export class Session implements SessionContext { toolResponse, toolUseId, permissionMode, - abortSignal, + activeToolAbortSignal, ); // If hook indicates to stop, return an error response @@ -3555,18 +3756,19 @@ export class Session implements SessionContext { } else if ( hooksEnabledForTool && messageBusForTool && - toolResult.error + (toolResult.error || aborted) ) { - // Fire PostToolUseFailure hook when tool returns an error (aligned with core path) + const isInterrupt = aborted; + // Fire PostToolUseFailure hook when a tool errors or resolves after cancellation. const failureHookResult = await firePostToolUseFailureHook( messageBusForTool, toolUseId, toolName, args, - toolResult.error.message, - false, // not an interrupt + toolResult.error?.message ?? 'Tool execution was cancelled', + isInterrupt, permissionMode, - abortSignal, + activeToolAbortSignal, ); // Log additional context if provided @@ -3577,21 +3779,6 @@ export class Session implements SessionContext { } } - // A tool can fail "softly" by returning toolResult.error without - // throwing, and can be cancelled mid-flight. Compute the real outcome - // once and reflect it on the client-facing emitResult as well as - // logToolCall / recordToolResult / the tool span, instead of - // hardcoding success — otherwise failed/cancelled daemon/ACP tools - // are mislabeled as successful in telemetry, session replay, and the - // client UI. - const aborted = abortSignal.aborted; - const status: 'success' | 'error' | 'cancelled' = aborted - ? 'cancelled' - : toolResult.error - ? 'error' - : 'success'; - const succeeded = status === 'success'; - // Handle TodoWriteTool: extract todos and send plan update if (isTodoWriteTool) { const todos = this.planEmitter.extractTodos( @@ -3662,10 +3849,14 @@ export class Session implements SessionContext { } else if (aborted) { spanError = 'Tool execution was cancelled'; } - return responseParts; + return { + parts: responseParts, + stopAfterUserQuestionCancel: nestedAskUserQuestionCancelled, + }; } catch (e) { // Ensure cleanup on error subAgentCleanupFunctions.forEach((cleanup) => cleanup()); + removeAgentToolAbortPropagation?.(); const error = e instanceof Error ? e : new Error(String(e)); spanError = error.message; @@ -3673,7 +3864,7 @@ export class Session implements SessionContext { // Fire PostToolUseFailure hook (aligned with core path in coreToolScheduler.ts) const hooksEnabledForError = !this.config.getDisableAllHooks?.(); const messageBusForError = this.config.getMessageBus?.(); - const isInterrupt = abortSignal.aborted; + const isInterrupt = activeToolAbortSignal.aborted; if (hooksEnabledForError && messageBusForError) { const failureHookResult = await firePostToolUseFailureHook( @@ -3684,7 +3875,7 @@ export class Session implements SessionContext { error.message, isInterrupt, String(approvalMode), - abortSignal, + activeToolAbortSignal, ); // Log additional context if provided @@ -3712,13 +3903,16 @@ export class Session implements SessionContext { callId, // A throw caused by abort (e.g. AbortError) is a cancellation, not // a genuine tool error — keep it consistent with the success path. - status: abortSignal.aborted ? 'cancelled' : 'error', + status: activeToolAbortSignal.aborted ? 'cancelled' : 'error', resultDisplay: undefined, error, errorType: undefined, }); - return errorResponse(error); + return { + parts: errorResponse(error), + stopAfterUserQuestionCancel: nestedAskUserQuestionCancelled, + }; } }); // end runInToolSpanContext } finally { diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts index 243d2a4ff45..43d909380f5 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts @@ -545,6 +545,150 @@ describe('SubAgentTracker', () => { }); }); + it('notifies when nested ask_user_question is cancelled', async () => { + requestPermissionSpy.mockResolvedValue({ + outcome: { outcome: 'cancelled' }, + }); + const onAskUserQuestionCancel = vi.fn(); + tracker = new SubAgentTracker( + mockContext, + mockClient, + 'parent-call-123', + 'test-subagent', + onAskUserQuestionCancel, + ); + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockResolvedValue(undefined); + const event = createApprovalEvent({ + name: ToolNames.ASK_USER_QUESTION, + callId: 'call-ask', + confirmationDetails: { + type: 'ask_user_question', + title: 'Question', + questions: [{ question: 'Continue?', header: 'Question' }], + } as AgentApprovalRequestEvent['confirmationDetails'], + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(respondSpy).toHaveBeenCalledWith( + ToolConfirmationOutcome.Cancel, + { + answers: undefined, + }, + ); + }); + expect(onAskUserQuestionCancel).toHaveBeenCalledOnce(); + expect(respondSpy.mock.invocationCallOrder[0]).toBeLessThan( + onAskUserQuestionCancel.mock.invocationCallOrder[0], + ); + }); + + it('does not notify when a non-question subagent tool is cancelled', async () => { + requestPermissionSpy.mockResolvedValue({ + outcome: { outcome: 'cancelled' }, + }); + const onAskUserQuestionCancel = vi.fn(); + tracker = new SubAgentTracker( + mockContext, + mockClient, + 'parent-call-123', + 'test-subagent', + onAskUserQuestionCancel, + ); + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockResolvedValue(undefined); + const event = createApprovalEvent({ + name: 'shell', + callId: 'call-shell', + confirmationDetails: createInfoConfirmation(), + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(respondSpy).toHaveBeenCalledWith( + ToolConfirmationOutcome.Cancel, + { + answers: undefined, + }, + ); + }); + expect(onAskUserQuestionCancel).not.toHaveBeenCalled(); + }); + + it('notifies when nested ask_user_question permission request fails', async () => { + requestPermissionSpy.mockRejectedValue(new Error('Network error')); + const onAskUserQuestionCancel = vi.fn(); + tracker = new SubAgentTracker( + mockContext, + mockClient, + 'parent-call-123', + 'test-subagent', + onAskUserQuestionCancel, + ); + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockResolvedValue(undefined); + const event = createApprovalEvent({ + name: ToolNames.ASK_USER_QUESTION, + callId: 'call-ask', + confirmationDetails: { + type: 'ask_user_question', + title: 'Question', + questions: [{ question: 'Continue?', header: 'Question' }], + } as AgentApprovalRequestEvent['confirmationDetails'], + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(respondSpy).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); + }); + expect(onAskUserQuestionCancel).toHaveBeenCalledOnce(); + expect(onAskUserQuestionCancel.mock.invocationCallOrder[0]).toBeLessThan( + respondSpy.mock.invocationCallOrder[0], + ); + }); + + it('notifies when nested ask_user_question permission failure cannot respond', async () => { + requestPermissionSpy.mockRejectedValue(new Error('Network error')); + const onAskUserQuestionCancel = vi.fn(); + tracker = new SubAgentTracker( + mockContext, + mockClient, + 'parent-call-123', + 'test-subagent', + onAskUserQuestionCancel, + ); + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockRejectedValue(new Error('Already closed')); + const event = createApprovalEvent({ + name: ToolNames.ASK_USER_QUESTION, + callId: 'call-ask', + confirmationDetails: { + type: 'ask_user_question', + title: 'Question', + questions: [{ question: 'Continue?', header: 'Question' }], + } as AgentApprovalRequestEvent['confirmationDetails'], + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(onAskUserQuestionCancel).toHaveBeenCalledOnce(); + }); + expect(respondSpy).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); + }); + it('should forward answers payload from ACP permission responses', async () => { requestPermissionSpy.mockResolvedValue({ outcome: { diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.ts index 1cebb156f53..22d9b842b08 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.ts @@ -18,6 +18,7 @@ import type { import { AgentEventType, ToolConfirmationOutcome, + ToolNames, createDebugLogger, } from '@qwen-code/qwen-code-core'; import { z } from 'zod'; @@ -63,6 +64,7 @@ export class SubAgentTracker { private readonly client: AgentSideConnection, parentToolCallId: string, subagentType: string, + private readonly onAskUserQuestionCancel?: () => void, ) { this.toolCallEmitter = new ToolCallEmitter(ctx); this.messageEmitter = new MessageEmitter(ctx); @@ -226,18 +228,36 @@ export class SubAgentTracker { : z .nativeEnum(ToolConfirmationOutcome) .parse(output.outcome.optionId); - // Respond to subagent with the outcome await event.respond(outcome, { answers: 'answers' in output ? output.answers : undefined, }); + if ( + outcome === ToolConfirmationOutcome.Cancel && + event.name === ToolNames.ASK_USER_QUESTION + ) { + this.onAskUserQuestionCancel?.(); + } } catch (error) { // If permission request fails, cancel the tool call debugLogger.error( `Permission request failed for subagent tool ${event.name}:`, error, ); - await event.respond(ToolConfirmationOutcome.Cancel); + if (event.name === ToolNames.ASK_USER_QUESTION) { + // Fail closed: if the client cannot answer a nested user question, + // stop the parent turn instead of letting later tools run without the + // required user input. + this.onAskUserQuestionCancel?.(); + } + try { + await event.respond(ToolConfirmationOutcome.Cancel); + } catch (respondError) { + debugLogger.error( + `Failed to cancel subagent tool ${event.name} after permission request failure:`, + respondError, + ); + } } }; }