From d6a74ce13390b601753aedcc07a16fe39b1be86d Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 30 Jun 2026 23:05:44 +0800 Subject: [PATCH 1/8] fix(acp): address daemon loop review comments --- .../acp-integration/session/Session.test.ts | 215 +++++++++++++++++- .../src/acp-integration/session/Session.ts | 162 +++++++++++-- 2 files changed, 354 insertions(+), 23 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index a26db79e356..f73a1d472ba 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,189 @@ 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('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' }, + }, + ]; + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrorCount: 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(mockToolRegistry.getTool).toHaveBeenCalledTimes(3); + 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: 101 }, (_, index) => ({ + id: `read_${index}`, + name: 'read_file', + args: { file_path: `file_${index}.ts` }, + })); + 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(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}` }, + }), + ); + const toolLoopState = { + totalToolCalls: 0, + invalidToolParamErrorCount: 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.', + ]); expect(debugLoggerWarnSpy).toHaveBeenCalledWith( expect.stringContaining( 'Stopping ACP turn after repeated tool parameter errors', diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 7d98e025ad3..75fd387a5ab 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -217,6 +217,7 @@ type RunToolResult = { type DaemonToolLoopState = { totalToolCalls: number; + invalidToolParamErrorCount: number; invalidToolParamErrors: Map; loopDetected: boolean; }; @@ -226,10 +227,15 @@ 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 { totalToolCalls: 0, + invalidToolParamErrorCount: 0, invalidToolParamErrors: new Map(), loopDetected: false, }; @@ -278,7 +284,8 @@ function recordDaemonInvalidToolParams( ): boolean { if (!loopState || loopState.loopDetected) return loopState?.loopDetected ?? false; - const key = `${toolName}\0${error.message}`; + const key = toolName; + loopState.invalidToolParamErrorCount++; const count = (loopState.invalidToolParamErrors.get(key) ?? 0) + 1; loopState.invalidToolParamErrors.set(key, count); if (count < DAEMON_INVALID_TOOL_PARAMS_THRESHOLD) return false; @@ -1901,7 +1908,7 @@ export class Session implements SessionContext { toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { - await this.#preserveCancelledPermissionToolRun( + await this.#preserveStoppedToolRun( toolRun, pendingSend.signal, ); @@ -1911,6 +1918,13 @@ export class Session implements SessionContext { toolRun, pendingSend.signal, ); + if (toolRun.loopDetected) { + await this.#preserveStoppedToolRun( + toolRun, + pendingSend.signal, + ); + return { stopReason: 'end_turn' }; + } } } @@ -2179,16 +2193,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 +2368,7 @@ export class Session implements SessionContext { } } - async #preserveCancelledPermissionToolRun( + async #preserveStoppedToolRun( toolRun: RunToolResult, abortSignal: AbortSignal, ): Promise { @@ -2362,6 +2377,9 @@ export class Session implements SessionContext { role: 'user', parts: [ ...toolRun.parts, + ...(toolRun.loopDetected + ? [{ text: LOOP_DETECTED_CONTEXT_MESSAGE }] + : []), ...(await this.#drainMidTurnUserMessages(abortSignal)), ], }, @@ -3071,16 +3089,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 +3407,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 +3415,11 @@ export class Session implements SessionContext { toolRun, ac.signal, ); + if (toolRun.loopDetected) { + await this.#preserveStoppedToolRun(toolRun, ac.signal); + await this.#emitBackgroundNotificationEndTurn('end_turn'); + return; + } } } @@ -3882,17 +3903,20 @@ export class Session implements SessionContext { } let skippedToolCallCounter = 0; - const recordSkippedToolCall = async (fc: FunctionCall): Promise => { + const recordSkippedToolCall = async ( + fc: FunctionCall, + message = PERMISSION_CANCEL_SKIP_MESSAGE, + ): 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 }, + response: { error: message }, }, }; - const error = new Error(PERMISSION_CANCEL_SKIP_MESSAGE); + const error = new Error(message); try { this.config.getChatRecordingService()?.recordToolResult([part], { callId, @@ -3931,14 +3955,70 @@ export class Session implements SessionContext { onStopAfterPermissionCancel?: () => 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++) { + const r = await this.runTool( + runAbortSignal, + promptId, + calls[i], + onStopAfterPermissionCancel, + toolLoopState, + ); + 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])], @@ -3962,6 +4042,22 @@ export class Session implements SessionContext { executing.add(p); if (executing.size >= maxConcurrency) { await Promise.race(executing); + if (results.some((result) => result?.loopDetected)) { + await Promise.all(executing); + await fillLoopSkippedFrom(idx + 1); + return results; + } + if ( + toolLoopState && + toolLoopState.invalidToolParamErrorCount > 0 && + executing.size > 0 + ) { + await Promise.all(executing); + if (results.some((result) => result?.loopDetected)) { + await fillLoopSkippedFrom(idx + 1); + return results; + } + } } } await Promise.all(executing); @@ -4104,6 +4200,21 @@ export class Session implements SessionContext { ): Promise { const callId = fc.id ?? `${fc.name}-${Date.now()}`; let args = (fc.args ?? {}) as Record; + if (toolLoopState?.loopDetected) { + return { + parts: [ + { + 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; @@ -4169,9 +4280,20 @@ export class Session implements SessionContext { error, errorType: undefined, }); + const loopDetected = + !activeToolAbortSignal.aborted && + !opts?.stopAfterPermissionCancel && + recordDaemonInvalidToolParams( + this.config, + promptId, + toolLoopState, + toolName, + error, + ); return { parts: errorParts, stopAfterPermissionCancel: opts?.stopAfterPermissionCancel ?? false, + loopDetected, }; }; From 85af8c7f319edd3900581ccdf5788208151b66b6 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 1 Jul 2026 00:36:52 +0800 Subject: [PATCH 2/8] fix(acp): skip remaining calls after loop detection --- .../acp-integration/session/Session.test.ts | 35 ++++++++++++++++++- .../src/acp-integration/session/Session.ts | 2 ++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index f73a1d472ba..ae21d3daeea 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2502,6 +2502,16 @@ describe('Session', () => { 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, @@ -2531,7 +2541,16 @@ describe('Session', () => { ); expect(result.loopDetected).toBe(true); - expect(mockToolRegistry.getTool).toHaveBeenCalledTimes(3); + 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 a permission request was cancelled before the user answered; user input is required before continuing.', + ); expect(debugLoggerWarnSpy).toHaveBeenCalledWith( expect.stringContaining( 'Stopping ACP turn after repeated tool parameter errors from missing_tool', @@ -2595,6 +2614,11 @@ describe('Session', () => { args: { subagent_type: `bad_${index}` }, }), ); + functionCalls.push({ + id: 'read_after_loop', + name: 'read_file', + args: { file_path: 'after-loop.ts' }, + }); const toolLoopState = { totalToolCalls: 0, invalidToolParamErrorCount: 0, @@ -2629,6 +2653,15 @@ describe('Session', () => { ).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 a permission request was cancelled before the user answered; user input is required before continuing.', + ]); + 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( diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 700adc72b2f..4d0ada16aa8 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -4107,6 +4107,7 @@ export class Session implements SessionContext { shouldStopForLoop ||= r.loopDetected === true; } if (shouldStopForLoop) { + await appendSkippedAfter(parts, batch.calls[batch.calls.length - 1]); return { parts, stopAfterPermissionCancel: false, @@ -4132,6 +4133,7 @@ export class Session implements SessionContext { ); parts.push(...r.parts); if (r.loopDetected) { + await appendSkippedAfter(parts, fc); return { parts, stopAfterPermissionCancel: false, From 730f9f0bcb84048f6445dae8c6ab32b11c6f7be4 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 1 Jul 2026 00:45:21 +0800 Subject: [PATCH 3/8] fix(acp): use loop skip reason after detection --- .../src/acp-integration/session/Session.test.ts | 4 ++-- .../cli/src/acp-integration/session/Session.ts | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index ae21d3daeea..c988fa29cc6 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2549,7 +2549,7 @@ describe('Session', () => { 'read_after_loop', ]); expect(result.parts[4].functionResponse?.response?.['error']).toEqual( - 'Skipped because a permission request was cancelled before the user answered; user input is required before continuing.', + 'Skipped because loop detection stopped the current turn before this tool call could run.', ); expect(debugLoggerWarnSpy).toHaveBeenCalledWith( expect.stringContaining( @@ -2653,7 +2653,7 @@ describe('Session', () => { ).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 a permission request was cancelled before the user answered; user input is required before continuing.', + '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', diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 4d0ada16aa8..73769400c56 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -3938,10 +3938,14 @@ export class Session implements SessionContext { 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)); } }; @@ -4107,7 +4111,11 @@ export class Session implements SessionContext { shouldStopForLoop ||= r.loopDetected === true; } if (shouldStopForLoop) { - await appendSkippedAfter(parts, batch.calls[batch.calls.length - 1]); + await appendSkippedAfter( + parts, + batch.calls[batch.calls.length - 1], + LOOP_DETECTED_SKIP_MESSAGE, + ); return { parts, stopAfterPermissionCancel: false, @@ -4133,7 +4141,7 @@ export class Session implements SessionContext { ); parts.push(...r.parts); if (r.loopDetected) { - await appendSkippedAfter(parts, fc); + await appendSkippedAfter(parts, fc, LOOP_DETECTED_SKIP_MESSAGE); return { parts, stopAfterPermissionCancel: false, From 004ddf5fb0c8010d2476e757bf98cfd643b8e90d Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 1 Jul 2026 02:33:11 +0800 Subject: [PATCH 4/8] fix(cli): resolve acp loop review comments --- .../acp-integration/session/Session.test.ts | 78 ++++++++++++++++++- .../src/acp-integration/session/Session.ts | 13 ++-- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index c988fa29cc6..f69f94be164 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2515,7 +2515,6 @@ describe('Session', () => { ]; const toolLoopState = { totalToolCalls: 0, - invalidToolParamErrorCount: 0, invalidToolParamErrors: new Map(), loopDetected: false, }; @@ -2581,6 +2580,25 @@ describe('Session', () => { 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', + ), + }), + ]), + }); expect(debugLoggerWarnSpy).toHaveBeenCalledWith( expect.stringContaining( 'Stopping ACP turn after 101 tool calls in one turn.', @@ -2621,7 +2639,6 @@ describe('Session', () => { }); const toolLoopState = { totalToolCalls: 0, - invalidToolParamErrorCount: 0, invalidToolParamErrors: new Map(), loopDetected: false, }; @@ -2670,6 +2687,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( diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 73769400c56..4e4d8eff90a 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -217,7 +217,6 @@ type RunToolResult = { type DaemonToolLoopState = { totalToolCalls: number; - invalidToolParamErrorCount: number; invalidToolParamErrors: Map; loopDetected: boolean; }; @@ -235,7 +234,6 @@ const LOOP_DETECTED_CONTEXT_MESSAGE = function createDaemonToolLoopState(): DaemonToolLoopState { return { totalToolCalls: 0, - invalidToolParamErrorCount: 0, invalidToolParamErrors: new Map(), loopDetected: false, }; @@ -285,7 +283,6 @@ function recordDaemonInvalidToolParams( if (!loopState || loopState.loopDetected) return loopState?.loopDetected ?? false; const key = toolName; - loopState.invalidToolParamErrorCount++; const count = (loopState.invalidToolParamErrors.get(key) ?? 0) + 1; loopState.invalidToolParamErrors.set(key, count); if (count < DAEMON_INVALID_TOOL_PARAMS_THRESHOLD) return false; @@ -3771,7 +3768,13 @@ export class Session implements SessionContext { ) ) { return { - parts: [], + parts: functionCalls.map((fc) => ({ + functionResponse: { + id: fc.id ?? `${fc.name}-${Date.now()}`, + name: fc.name ?? 'unknown_tool', + response: { error: LOOP_DETECTED_SKIP_MESSAGE }, + }, + })), stopAfterPermissionCancel: false, loopDetected: true, }; @@ -4053,7 +4056,7 @@ export class Session implements SessionContext { } if ( toolLoopState && - toolLoopState.invalidToolParamErrorCount > 0 && + toolLoopState.invalidToolParamErrors.size > 0 && executing.size > 0 ) { await Promise.all(executing); From d4aff4d167ea1a5e33e42d744f6c1ebe104e4fbc Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 1 Jul 2026 03:32:35 +0800 Subject: [PATCH 5/8] fix(cli): narrow daemon invalid tool bucketing --- .../acp-integration/session/Session.test.ts | 46 +++++++++++++++++++ .../src/acp-integration/session/Session.ts | 12 ++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index f69f94be164..f8fc8c1725e 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2483,6 +2483,52 @@ describe('Session', () => { }); }); + 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); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 4e4d8eff90a..b7c56999e1e 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -4277,7 +4277,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(); @@ -4294,6 +4297,7 @@ export class Session implements SessionContext { errorType: undefined, }); const loopDetected = + opts?.recordInvalidToolParams === true && !activeToolAbortSignal.aborted && !opts?.stopAfterPermissionCancel && recordDaemonInvalidToolParams( @@ -4311,7 +4315,9 @@ export class Session implements SessionContext { }; if (!fc.name) { - return earlyErrorResponse(new Error('Missing function name')); + return earlyErrorResponse(new Error('Missing function name'), undefined, { + recordInvalidToolParams: true, + }); } const toolName = fc.name; @@ -4321,6 +4327,8 @@ export class Session implements SessionContext { if (!tool) { return earlyErrorResponse( new Error(`Tool "${toolName}" not found in registry.`), + toolName, + { recordInvalidToolParams: true }, ); } From e403d2f9efec4fb846157b38885574b2873e6aa5 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 1 Jul 2026 04:34:42 +0800 Subject: [PATCH 6/8] fix(cli): abort agent batch after loop detection --- .../acp-integration/session/Session.test.ts | 91 +++++++++++++++++++ .../src/acp-integration/session/Session.ts | 4 + 2 files changed, 95 insertions(+) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index f8fc8c1725e..506d8a23143 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -9299,9 +9299,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; }>; }; @@ -10117,6 +10123,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 b7c56999e1e..949fc645a91 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -3960,6 +3960,7 @@ export class Session implements SessionContext { calls: FunctionCall[], runAbortSignal: AbortSignal, onStopAfterPermissionCancel?: () => void, + onStopAfterLoopDetected?: () => void, shouldSkipUnstarted?: () => boolean, ): Promise => { const configuredMaxConcurrency = parsePositiveIntegerEnv( @@ -4050,6 +4051,7 @@ export class Session implements SessionContext { 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; @@ -4061,6 +4063,7 @@ export class Session implements SessionContext { ) { await Promise.all(executing); if (results.some((result) => result?.loopDetected)) { + onStopAfterLoopDetected?.(); await fillLoopSkippedFrom(idx + 1); return results; } @@ -4101,6 +4104,7 @@ export class Session implements SessionContext { batch.calls, batchAbortController.signal, stopBatchAfterPermissionCancel, + () => batchAbortController.abort('loop_detected'), () => batchStopAfterPermissionCancel, ); } finally { From 6b4eb8e5a28dac25c641f2277dc3648d42f31f9f Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 1 Jul 2026 05:38:48 +0800 Subject: [PATCH 7/8] fix(cli): record loop-detected skipped tools --- .../acp-integration/session/Session.test.ts | 17 ++- .../src/acp-integration/session/Session.ts | 122 ++++++++++-------- 2 files changed, 86 insertions(+), 53 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 506d8a23143..af6c5000618 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2605,11 +2605,12 @@ describe('Session', () => { 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: 101 }, (_, index) => ({ + 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([ { @@ -2645,9 +2646,21 @@ describe('Session', () => { }), ]), }); + 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.', + 'Stopping ACP turn after 102 tool calls in one turn.', ), ); }); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 949fc645a91..a55695cef72 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -282,6 +282,8 @@ function recordDaemonInvalidToolParams( ): boolean { if (!loopState || loopState.loopDetected) return loopState?.loopDetected ?? false; + // 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); @@ -3759,6 +3761,46 @@ 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, @@ -3768,19 +3810,16 @@ export class Session implements SessionContext { ) ) { return { - parts: functionCalls.map((fc) => ({ - functionResponse: { - id: fc.id ?? `${fc.name}-${Date.now()}`, - name: fc.name ?? 'unknown_tool', - response: { error: LOOP_DETECTED_SKIP_MESSAGE }, - }, - })), + 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; @@ -3905,42 +3944,6 @@ export class Session implements SessionContext { } } - let skippedToolCallCounter = 0; - const recordSkippedToolCall = async ( - fc: FunctionCall, - message = PERMISSION_CANCEL_SKIP_MESSAGE, - ): 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, - }); - 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, @@ -4003,12 +4006,20 @@ export class Session implements SessionContext { ) { 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) { @@ -4040,6 +4051,7 @@ export class Session implements SessionContext { calls[idx], onStopAfterPermissionCancel, toolLoopState, + recordSkippedToolCall, ) .then((r) => { results[idx] = r; @@ -4145,6 +4157,7 @@ export class Session implements SessionContext { fc, undefined, toolLoopState, + recordSkippedToolCall, ); parts.push(...r.parts); if (r.loopDetected) { @@ -4214,19 +4227,26 @@ 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: [ - { - functionResponse: { - id: callId, - name: fc.name ?? 'unknown_tool', - response: { error: LOOP_DETECTED_SKIP_MESSAGE }, - }, - }, + 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, From 05e8c14f229aacb3b6a036d5dea347968e9ea0e3 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 1 Jul 2026 07:37:50 +0800 Subject: [PATCH 8/8] fix(cli): resolve acp loop review follow-ups --- .../acp-integration/session/Session.test.ts | 79 ++++++++++++++++++- .../src/acp-integration/session/Session.ts | 11 +-- 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index af6c5000618..5c10c8d203f 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2660,7 +2660,7 @@ describe('Session', () => { ).toEqual(preservedResponses); expect(debugLoggerWarnSpy).toHaveBeenCalledWith( expect.stringContaining( - 'Stopping ACP turn after 102 tool calls in one turn.', + 'Stopping ACP turn after 101 tool calls in one turn.', ), ); }); @@ -9654,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({ diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index a55695cef72..168523ec920 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -3806,7 +3806,7 @@ export class Session implements SessionContext { this.config, promptId, toolLoopState, - functionCalls.length, + dedupedFunctionCalls.length, ) ) { return { @@ -4068,11 +4068,12 @@ export class Session implements SessionContext { await fillLoopSkippedFrom(idx + 1); return results; } - if ( + const invalidToolErrorNearThreshold = toolLoopState && - toolLoopState.invalidToolParamErrors.size > 0 && - executing.size > 0 - ) { + [...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?.();