diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index a26db79e356..5c10c8d203f 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2349,9 +2349,35 @@ describe('Session', () => { it('stops an ACP prompt after repeated invalid tool parameters with fresh ids', async () => { mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); - const build = vi.fn().mockImplementation(() => { - throw new Error('Parameter "questions" must be an array.'); - }); + const messageBus = { + request: vi.fn().mockResolvedValue({ + success: true, + output: { + decision: 'block', + reason: 'Continue after Stop hook', + }, + }), + }; + 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' }] }]); + mockChat.getLastModelMessageText = vi.fn().mockReturnValue('response'); + const build = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('Parameter "questions" must be an array: value 1.'); + }) + .mockImplementationOnce(() => { + throw new Error('Parameter "questions" must be an array: value 2.'); + }) + .mockImplementationOnce(() => { + throw new Error('Parameter "questions" must be an array: value 3.'); + }); mockToolRegistry.getTool.mockReturnValue({ name: 'ask_user_question', kind: core.Kind.Other, @@ -2421,6 +2447,298 @@ describe('Session', () => { expect(build).toHaveBeenCalledTimes(3); expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(3); + const stopHookCalls = messageBus.request.mock.calls.filter( + ([request]) => + typeof request === 'object' && + request !== null && + 'eventName' in request && + request.eventName === 'Stop', + ); + expect(stopHookCalls).toHaveLength(0); + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'Stopping ACP turn after repeated tool parameter errors', + ), + ); + expect(mockChat.addHistory).toHaveBeenCalledWith({ + role: 'user', + parts: [ + expect.objectContaining({ + functionResponse: expect.objectContaining({ + id: 'ask_3', + name: 'ask_user_question', + response: expect.objectContaining({ + error: expect.stringContaining( + 'Parameter "questions" must be an array', + ), + }), + }), + }), + expect.objectContaining({ + text: expect.stringContaining( + 'terminated because the model exceeded tool-call safety limits', + ), + }), + ], + }); + }); + + it('does not stop disabled tools as repeated invalid parameter calls', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockConfig.getPermissionManager = vi.fn().mockReturnValue({ + isToolEnabled: vi.fn().mockResolvedValue(false), + }); + mockToolRegistry.getTool.mockReturnValue({ + name: 'write_file', + kind: core.Kind.Edit, + build: vi.fn(), + }); + const functionCalls: FunctionCall[] = [ + { id: 'write_1', name: 'write_file', args: {} }, + { id: 'write_2', name: 'write_file', args: {} }, + { id: 'write_3', name: 'write_file', args: {} }, + ]; + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrors: new Map(), + loopDetected: false, + }; + + const result = await ( + session as unknown as { + runToolCalls: ( + abortSignal: AbortSignal, + promptId: string, + calls: FunctionCall[], + loopState: typeof toolLoopState, + ) => Promise<{ + parts: Part[]; + stopAfterPermissionCancel: boolean; + loopDetected?: boolean; + }>; + } + ).runToolCalls( + new AbortController().signal, + 'prompt-disabled-tool', + functionCalls, + toolLoopState, + ); + + expect(result.loopDetected).not.toBe(true); + expect(toolLoopState.invalidToolParamErrors.size).toBe(0); + expect(result.parts).toHaveLength(3); + }); + + it('stops early tool lookup errors after repeated invalid tool calls', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockToolRegistry.getTool.mockReturnValue(undefined); + const functionCalls: FunctionCall[] = [ + { + id: 'missing_1', + name: 'missing_tool', + args: { value: 'one' }, + }, + { + id: 'missing_2', + name: 'missing_tool', + args: { value: 'two' }, + }, + { + id: 'missing_3', + name: 'missing_tool', + args: { value: 'three' }, + }, + { + id: 'missing_4', + name: 'missing_tool', + args: { value: 'four' }, + }, + { + id: 'read_after_loop', + name: 'read_file', + args: { file_path: 'after-loop.ts' }, + }, + ]; + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrors: new Map(), + loopDetected: false, + }; + + const result = await ( + session as unknown as { + runToolCalls: ( + abortSignal: AbortSignal, + promptId: string, + calls: FunctionCall[], + loopState: typeof toolLoopState, + ) => Promise<{ + parts: Part[]; + stopAfterPermissionCancel: boolean; + loopDetected?: boolean; + }>; + } + ).runToolCalls( + new AbortController().signal, + 'prompt-missing-tool-loop', + functionCalls, + toolLoopState, + ); + + expect(result.loopDetected).toBe(true); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'missing_1', + 'missing_2', + 'missing_3', + 'missing_4', + 'read_after_loop', + ]); + expect(result.parts[4].functionResponse?.response?.['error']).toEqual( + 'Skipped because loop detection stopped the current turn before this tool call could run.', + ); + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'Stopping ACP turn after repeated tool parameter errors from missing_tool', + ), + ); + }); + + it('stops an ACP prompt after exceeding the daemon tool-call cap', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + const functionCalls = Array.from({ length: 102 }, (_, index) => ({ + id: `read_${index}`, + name: 'read_file', + args: { file_path: `file_${index}.ts` }, + })); + functionCalls[101].id = 'read_0'; + mockChat.sendMessageStream = vi.fn().mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { functionCalls }, + }, + ]), + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'read many files' }], + }); + + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(1); + expect(mockToolRegistry.getTool).not.toHaveBeenCalled(); + expect(mockChat.addHistory).toHaveBeenCalledWith({ + role: 'user', + parts: expect.arrayContaining([ + expect.objectContaining({ + functionResponse: expect.objectContaining({ + id: 'read_0', + name: 'read_file', + response: { + error: expect.stringContaining('loop detection'), + }, + }), + }), + expect.objectContaining({ + text: expect.stringContaining( + 'terminated because the model exceeded tool-call safety limits', + ), + }), + ]), + }); + const preservedResponses = vi + .mocked(mockChat.addHistory) + .mock.calls.flatMap(([content]) => content.parts ?? []) + .filter((part) => part.functionResponse) + .map((part) => part.functionResponse?.id); + expect(preservedResponses).toHaveLength(101); + expect(new Set(preservedResponses).size).toBe(101); + expect( + mockChatRecordingService.recordToolResult.mock.calls.map( + ([parts]) => parts[0]?.functionResponse?.id, + ), + ).toEqual(preservedResponses); + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'Stopping ACP turn after 101 tool calls in one turn.', + ), + ); + }); + + it('does not start unstarted concurrent Agent calls after invalid parameter loop detection', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + const build = vi.fn().mockImplementation(() => { + throw new Error('Invalid subagent_type: bad'); + }); + mockToolRegistry.getTool.mockImplementation((name: string) => + name === core.ToolNames.AGENT + ? { + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build, + canUpdateOutput: false, + isOutputMarkdown: true, + } + : undefined, + ); + const functionCalls: FunctionCall[] = Array.from( + { length: 5 }, + (_, index) => ({ + id: `agent_${index}`, + name: core.ToolNames.AGENT, + args: { subagent_type: `bad_${index}` }, + }), + ); + functionCalls.push({ + id: 'read_after_loop', + name: 'read_file', + args: { file_path: 'after-loop.ts' }, + }); + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrors: new Map(), + loopDetected: false, + }; + const result = await ( + session as unknown as { + runToolCalls: ( + abortSignal: AbortSignal, + promptId: string, + calls: FunctionCall[], + loopState: typeof toolLoopState, + ) => Promise<{ + parts: Part[]; + stopAfterPermissionCancel: boolean; + loopDetected?: boolean; + }>; + } + ).runToolCalls( + new AbortController().signal, + 'prompt-agent-invalid-loop', + functionCalls, + toolLoopState, + ); + + expect(result.loopDetected).toBe(true); + expect( + result.parts + .slice(3) + .map((part) => part.functionResponse?.response?.['error']), + ).toEqual([ + 'Skipped because loop detection stopped the current turn before this tool call could run.', + 'Skipped because loop detection stopped the current turn before this tool call could run.', + 'Skipped because loop detection stopped the current turn before this tool call could run.', + ]); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'agent_0', + 'agent_1', + 'agent_2', + 'agent_3', + 'agent_4', + 'read_after_loop', + ]); expect(debugLoggerWarnSpy).toHaveBeenCalledWith( expect.stringContaining( 'Stopping ACP turn after repeated tool parameter errors', @@ -2428,6 +2746,63 @@ describe('Session', () => { ); }); + it('stops concurrent Agent batches after Promise.race observes loop detection', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + const build = vi.fn().mockImplementation(() => { + throw new Error('Invalid subagent_type: bad'); + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build, + canUpdateOutput: false, + isOutputMarkdown: true, + }); + const functionCalls: FunctionCall[] = Array.from( + { length: 3 }, + (_, index) => ({ + id: `agent_${index}`, + name: core.ToolNames.AGENT, + args: { subagent_type: `bad_${index}` }, + }), + ); + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrors: new Map(), + loopDetected: false, + }; + + const result = await ( + session as unknown as { + runToolCalls: ( + abortSignal: AbortSignal, + promptId: string, + calls: FunctionCall[], + loopState: typeof toolLoopState, + ) => Promise<{ + parts: Part[]; + stopAfterPermissionCancel: boolean; + loopDetected?: boolean; + }>; + } + ).runToolCalls( + new AbortController().signal, + 'prompt-agent-race-loop', + functionCalls, + toolLoopState, + ); + + expect(result.loopDetected).toBe(true); + expect(build).toHaveBeenCalledTimes(3); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'agent_0', + 'agent_1', + 'agent_2', + ]); + }); + it('clears duplicate provider id tracking between ACP prompts', async () => { mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); vi.mocked(mockChat.getHistoryFunctionResponseIds).mockReturnValue( @@ -8937,9 +9312,15 @@ describe('Session', () => { abortSignal: AbortSignal, promptId: string, functionCalls: FunctionCall[], + toolLoopState?: { + totalToolCalls: number; + invalidToolParamErrors: Map; + loopDetected: boolean; + }, ) => Promise<{ parts: Part[]; stopAfterPermissionCancel: boolean; + loopDetected?: boolean; repeatedDuplicateProviderToolCall?: boolean; }>; }; @@ -9273,6 +9654,83 @@ describe('Session', () => { expect(laterExecute).not.toHaveBeenCalled(); }); + it('skips later pre-loop tools after non-question permission cancellation', async () => { + const cancelledExecute = vi.fn(); + const laterExecute = vi.fn().mockResolvedValue({ + llmContent: 'should not execute', + returnDisplay: 'should not execute', + }); + mockToolRegistry.getTool.mockImplementation((name: string) => + name === core.ToolNames.SHELL + ? mockConfirmingTool(name, cancelledExecute, 'exec') + : mockAllowedTool(name, laterExecute), + ); + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: { outcome: 'cancelled' }, + }); + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrors: new Map(), + loopDetected: false, + }; + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls( + new AbortController().signal, + 'prompt-pre-loop-shell-cancel', + [ + { + id: 'shell_call', + name: core.ToolNames.SHELL, + args: { command: 'echo denied' }, + }, + { + id: 'read_1', + name: core.ToolNames.READ_FILE, + args: { file_path: '/tmp/one' }, + }, + { + id: 'read_2', + name: core.ToolNames.READ_FILE, + args: { file_path: '/tmp/two' }, + }, + { + id: 'read_3', + name: core.ToolNames.READ_FILE, + args: { file_path: '/tmp/three' }, + }, + ], + toolLoopState, + ); + + expect(result.stopAfterPermissionCancel).toBe(true); + expect(result.parts.map((part) => part.functionResponse?.id)).toEqual([ + 'shell_call', + 'read_1', + 'read_2', + 'read_3', + ]); + expect( + result.parts.slice(1).map((part) => part.functionResponse?.response), + ).toEqual([ + { + error: + 'Skipped because a permission request was cancelled before the user answered; user input is required before continuing.', + }, + { + error: + 'Skipped because a permission request was cancelled before the user answered; user input is required before continuing.', + }, + { + error: + 'Skipped because a permission request was cancelled before the user answered; user input is required before continuing.', + }, + ]); + expect(cancelledExecute).not.toHaveBeenCalled(); + expect(laterExecute).not.toHaveBeenCalled(); + }); + it('skips later tools after selecting the reject permission option', async () => { const rejectedExecute = vi.fn(); const laterExecute = vi.fn().mockResolvedValue({ @@ -9755,6 +10213,91 @@ describe('Session', () => { expect(siblingSignal?.aborted).toBe(true); }); + it('aborts sibling Agent calls in the same batch after loop detection', async () => { + let firstSiblingSignal: AbortSignal | undefined; + let secondSiblingSignal: AbortSignal | undefined; + const firstSiblingExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + firstSiblingSignal = signal; + await waitForAbortOrTick(signal); + return { + llmContent: 'first sibling stopped', + returnDisplay: 'first sibling stopped', + }; + }); + const secondSiblingExecute = vi + .fn() + .mockImplementation(async (signal: AbortSignal) => { + secondSiblingSignal = signal; + await waitForAbortOrTick(signal); + return { + llmContent: 'second sibling stopped', + returnDisplay: 'second sibling stopped', + }; + }); + mockToolRegistry.getTool.mockReturnValue({ + name: core.ToolNames.AGENT, + kind: core.Kind.Think, + displayName: 'Agent', + description: 'Agent', + build: vi.fn().mockImplementation((args: Record) => { + if (args['_test_id'] === 'invalid') { + throw new Error('Invalid subagent_type: bad'); + } + const isFirstSibling = args['_test_id'] === 'sibling_1'; + return { + params: { subagent_type: 'explore', ...args }, + eventEmitter: new EventEmitter(), + execute: isFirstSibling + ? firstSiblingExecute + : secondSiblingExecute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }; + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrors: new Map([[core.ToolNames.AGENT, 2]]), + loopDetected: false, + }; + + const result = await ( + session as unknown as ToolCallInternals + ).runToolCalls( + new AbortController().signal, + 'prompt-agent-loop-abort', + [ + { + id: 'agent_invalid', + name: core.ToolNames.AGENT, + args: { _test_id: 'invalid', subagent_type: 'bad' }, + }, + { + id: 'agent_sibling_1', + name: core.ToolNames.AGENT, + args: { _test_id: 'sibling_1', subagent_type: 'explore' }, + }, + { + id: 'agent_sibling_2', + name: core.ToolNames.AGENT, + args: { _test_id: 'sibling_2', subagent_type: 'explore' }, + }, + ], + toolLoopState, + ); + + expect(result.loopDetected).toBe(true); + expect(firstSiblingExecute).toHaveBeenCalledOnce(); + expect(secondSiblingExecute).toHaveBeenCalledOnce(); + expect(firstSiblingSignal?.aborted).toBe(true); + expect(secondSiblingSignal?.aborted).toBe(true); + }); + it('passes an already-aborted parent signal to Agent batches', async () => { const eventEmitter = new EventEmitter(); const receivedAbortStates: boolean[] = []; diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 43a888bf7ee..168523ec920 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -226,6 +226,10 @@ const DAEMON_INVALID_TOOL_PARAMS_THRESHOLD = 3; const PERMISSION_CANCEL_SKIP_MESSAGE = 'Skipped because a permission request was cancelled before the user answered; user input is required before continuing.'; +const LOOP_DETECTED_SKIP_MESSAGE = + 'Skipped because loop detection stopped the current turn before this tool call could run.'; +const LOOP_DETECTED_CONTEXT_MESSAGE = + 'System: this turn was terminated because the model exceeded tool-call safety limits. Try a different approach on the next turn.'; function createDaemonToolLoopState(): DaemonToolLoopState { return { @@ -278,7 +282,9 @@ function recordDaemonInvalidToolParams( ): boolean { if (!loopState || loopState.loopDetected) return loopState?.loopDetected ?? false; - const key = `${toolName}\0${error.message}`; + // Intentionally bucket by tool name only: repeated parameter errors for the + // same tool mean the model is stuck on that tool's schema. + const key = toolName; const count = (loopState.invalidToolParamErrors.get(key) ?? 0) + 1; loopState.invalidToolParamErrors.set(key, count); if (count < DAEMON_INVALID_TOOL_PARAMS_THRESHOLD) return false; @@ -1901,7 +1907,7 @@ export class Session implements SessionContext { toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { - await this.#preserveCancelledPermissionToolRun( + await this.#preserveStoppedToolRun( toolRun, pendingSend.signal, ); @@ -1911,6 +1917,13 @@ export class Session implements SessionContext { toolRun, pendingSend.signal, ); + if (toolRun.loopDetected) { + await this.#preserveStoppedToolRun( + toolRun, + pendingSend.signal, + ); + return { stopReason: 'end_turn' }; + } } } @@ -2179,16 +2192,17 @@ export class Session implements SessionContext { toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { - await this.#preserveCancelledPermissionToolRun( - toolRun, - pendingSend.signal, - ); + await this.#preserveStoppedToolRun(toolRun, pendingSend.signal); return { stopReason: 'end_turn' }; } nextMessage = await this.#buildNextMessageAfterToolRun( toolRun, pendingSend.signal, ); + if (toolRun.loopDetected) { + await this.#preserveStoppedToolRun(toolRun, pendingSend.signal); + return { stopReason: 'end_turn' }; + } } } @@ -2353,7 +2367,7 @@ export class Session implements SessionContext { } } - async #preserveCancelledPermissionToolRun( + async #preserveStoppedToolRun( toolRun: RunToolResult, abortSignal: AbortSignal, ): Promise { @@ -2362,6 +2376,9 @@ export class Session implements SessionContext { role: 'user', parts: [ ...toolRun.parts, + ...(toolRun.loopDetected + ? [{ text: LOOP_DETECTED_CONTEXT_MESSAGE }] + : []), ...(await this.#drainMidTurnUserMessages(abortSignal)), ], }, @@ -3071,16 +3088,17 @@ export class Session implements SessionContext { toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { - await this.#preserveCancelledPermissionToolRun( - toolRun, - ac.signal, - ); + await this.#preserveStoppedToolRun(toolRun, ac.signal); return; } nextMessage = await this.#buildNextMessageAfterToolRun( toolRun, ac.signal, ); + if (toolRun.loopDetected) { + await this.#preserveStoppedToolRun(toolRun, ac.signal); + return; + } } } } catch (error) { @@ -3388,10 +3406,7 @@ export class Session implements SessionContext { toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { - await this.#preserveCancelledPermissionToolRun( - toolRun, - ac.signal, - ); + await this.#preserveStoppedToolRun(toolRun, ac.signal); await this.#emitBackgroundNotificationEndTurn('end_turn'); return; } @@ -3399,6 +3414,11 @@ export class Session implements SessionContext { toolRun, ac.signal, ); + if (toolRun.loopDetected) { + await this.#preserveStoppedToolRun(toolRun, ac.signal); + await this.#emitBackgroundNotificationEndTurn('end_turn'); + return; + } } } @@ -3741,22 +3761,65 @@ export class Session implements SessionContext { functionCalls: FunctionCall[], toolLoopState?: DaemonToolLoopState, ): Promise { + const dedupedFunctionCalls = dedupeToolCallsById(functionCalls); + let skippedToolCallCounter = 0; + const recordSkippedToolCall = async ( + fc: FunctionCall, + message = PERMISSION_CANCEL_SKIP_MESSAGE, + emitStart = true, + ): Promise => { + const toolName = fc.name ?? 'unknown_tool'; + const callId = fc.id ?? `${toolName}-skip-${++skippedToolCallCounter}`; + const part: Part = { + functionResponse: { + id: callId, + name: toolName, + response: { error: message }, + }, + }; + const error = new Error(message); + try { + this.config.getChatRecordingService()?.recordToolResult([part], { + callId, + status: 'error', + resultDisplay: undefined, + error, + errorType: undefined, + }); + if (emitStart) { + 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; + }; + if ( recordDaemonToolCalls( this.config, promptId, toolLoopState, - functionCalls.length, + dedupedFunctionCalls.length, ) ) { return { - parts: [], + parts: await Promise.all( + dedupedFunctionCalls.map((fc) => + recordSkippedToolCall(fc, LOOP_DETECTED_SKIP_MESSAGE, false), + ), + ), stopAfterPermissionCancel: false, loopDetected: true, }; } - const dedupedFunctionCalls = dedupeToolCallsById(functionCalls); type ExecutableBatch = { kind: 'execute'; concurrent: boolean; @@ -3881,43 +3944,14 @@ 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: PERMISSION_CANCEL_SKIP_MESSAGE }, - }, - }; - const error = new Error(PERMISSION_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 appendSkippedAfter = async ( + parts: Part[], + fc: FunctionCall, + message = PERMISSION_CANCEL_SKIP_MESSAGE, + ) => { const startIndex = dedupedFunctionCalls.indexOf(fc) + 1; for (const remainingCall of dedupedFunctionCalls.slice(startIndex)) { - parts.push(await recordSkippedToolCall(remainingCall)); + parts.push(await recordSkippedToolCall(remainingCall, message)); } }; @@ -3929,16 +3963,81 @@ export class Session implements SessionContext { calls: FunctionCall[], runAbortSignal: AbortSignal, onStopAfterPermissionCancel?: () => void, + onStopAfterLoopDetected?: () => void, shouldSkipUnstarted?: () => boolean, ): Promise => { - const maxConcurrency = parsePositiveIntegerEnv( + const configuredMaxConcurrency = parsePositiveIntegerEnv( process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'], 10, ); + const maxConcurrency = toolLoopState + ? Math.min( + configuredMaxConcurrency, + DAEMON_INVALID_TOOL_PARAMS_THRESHOLD, + ) + : configuredMaxConcurrency; const results: RunToolResult[] = new Array(calls.length); const executing = new Set>(); - for (let i = 0; i < calls.length; i++) { + const fillLoopSkippedFrom = async (startIndex: number) => { + for (let i = startIndex; i < calls.length; i++) { + if (results[i]) continue; + results[i] = { + parts: [ + await recordSkippedToolCall(calls[i], LOOP_DETECTED_SKIP_MESSAGE), + ], + stopAfterPermissionCancel: false, + loopDetected: true, + }; + } + }; + const fillPermissionSkippedFrom = async (startIndex: number) => { + for (let i = startIndex; i < calls.length; i++) { + if (results[i]) continue; + results[i] = { + parts: [await recordSkippedToolCall(calls[i])], + stopAfterPermissionCancel: false, + }; + } + }; + let startIndex = 0; + if ( + toolLoopState && + calls.length > DAEMON_INVALID_TOOL_PARAMS_THRESHOLD + ) { + startIndex = DAEMON_INVALID_TOOL_PARAMS_THRESHOLD; + for (let i = 0; i < startIndex; i++) { + if (runAbortSignal.aborted && shouldSkipUnstarted?.()) { + results[i] = { + parts: [await recordSkippedToolCall(calls[i])], + stopAfterPermissionCancel: false, + }; + continue; + } + const r = await this.runTool( + runAbortSignal, + promptId, + calls[i], + onStopAfterPermissionCancel, + toolLoopState, + recordSkippedToolCall, + ); + results[i] = r; + if (r.loopDetected) { + await fillLoopSkippedFrom(i + 1); + return results; + } + if (r.stopAfterPermissionCancel) { + await fillPermissionSkippedFrom(i + 1); + return results; + } + } + } + for (let i = startIndex; i < calls.length; i++) { const idx = i; + if (toolLoopState?.loopDetected) { + await fillLoopSkippedFrom(idx); + return results; + } if (runAbortSignal.aborted && shouldSkipUnstarted?.()) { results[idx] = { parts: [await recordSkippedToolCall(calls[idx])], @@ -3952,6 +4051,7 @@ export class Session implements SessionContext { calls[idx], onStopAfterPermissionCancel, toolLoopState, + recordSkippedToolCall, ) .then((r) => { results[idx] = r; @@ -3962,6 +4062,25 @@ export class Session implements SessionContext { executing.add(p); if (executing.size >= maxConcurrency) { await Promise.race(executing); + if (results.some((result) => result?.loopDetected)) { + onStopAfterLoopDetected?.(); + await Promise.all(executing); + await fillLoopSkippedFrom(idx + 1); + return results; + } + const invalidToolErrorNearThreshold = + toolLoopState && + [...toolLoopState.invalidToolParamErrors.values()].some( + (count) => count >= DAEMON_INVALID_TOOL_PARAMS_THRESHOLD - 1, + ); + if (invalidToolErrorNearThreshold && executing.size > 0) { + await Promise.all(executing); + if (results.some((result) => result?.loopDetected)) { + onStopAfterLoopDetected?.(); + await fillLoopSkippedFrom(idx + 1); + return results; + } + } } } await Promise.all(executing); @@ -3998,6 +4117,7 @@ export class Session implements SessionContext { batch.calls, batchAbortController.signal, stopBatchAfterPermissionCancel, + () => batchAbortController.abort('loop_detected'), () => batchStopAfterPermissionCancel, ); } finally { @@ -4011,6 +4131,11 @@ export class Session implements SessionContext { shouldStopForLoop ||= r.loopDetected === true; } if (shouldStopForLoop) { + await appendSkippedAfter( + parts, + batch.calls[batch.calls.length - 1], + LOOP_DETECTED_SKIP_MESSAGE, + ); return { parts, stopAfterPermissionCancel: false, @@ -4033,9 +4158,11 @@ export class Session implements SessionContext { fc, undefined, toolLoopState, + recordSkippedToolCall, ); parts.push(...r.parts); if (r.loopDetected) { + await appendSkippedAfter(parts, fc, LOOP_DETECTED_SKIP_MESSAGE); return { parts, stopAfterPermissionCancel: false, @@ -4101,9 +4228,31 @@ export class Session implements SessionContext { fc: FunctionCall, onStopAfterPermissionCancel?: () => void, toolLoopState?: DaemonToolLoopState, + recordSkippedToolCall?: ( + fc: FunctionCall, + message?: string, + emitStart?: boolean, + ) => Promise, ): Promise { const callId = fc.id ?? `${fc.name}-${Date.now()}`; let args = (fc.args ?? {}) as Record; + if (toolLoopState?.loopDetected) { + return { + parts: [ + recordSkippedToolCall + ? await recordSkippedToolCall(fc, LOOP_DETECTED_SKIP_MESSAGE, false) + : { + functionResponse: { + id: callId, + name: fc.name ?? 'unknown_tool', + response: { error: LOOP_DETECTED_SKIP_MESSAGE }, + }, + }, + ], + stopAfterPermissionCancel: false, + loopDetected: true, + }; + } const startTime = Date.now(); let spanError: string | undefined; @@ -4153,7 +4302,10 @@ export class Session implements SessionContext { const earlyErrorResponse = async ( error: Error, toolName = fc.name ?? 'unknown_tool', - opts?: { stopAfterPermissionCancel?: boolean }, + opts?: { + recordInvalidToolParams?: boolean; + stopAfterPermissionCancel?: boolean; + }, ) => { spanError = error.message; cleanupAgentToolResources(); @@ -4169,14 +4321,28 @@ export class Session implements SessionContext { error, errorType: undefined, }); + const loopDetected = + opts?.recordInvalidToolParams === true && + !activeToolAbortSignal.aborted && + !opts?.stopAfterPermissionCancel && + recordDaemonInvalidToolParams( + this.config, + promptId, + toolLoopState, + toolName, + error, + ); return { parts: errorParts, stopAfterPermissionCancel: opts?.stopAfterPermissionCancel ?? false, + loopDetected, }; }; if (!fc.name) { - return earlyErrorResponse(new Error('Missing function name')); + return earlyErrorResponse(new Error('Missing function name'), undefined, { + recordInvalidToolParams: true, + }); } const toolName = fc.name; @@ -4186,6 +4352,8 @@ export class Session implements SessionContext { if (!tool) { return earlyErrorResponse( new Error(`Tool "${toolName}" not found in registry.`), + toolName, + { recordInvalidToolParams: true }, ); }