diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index e372a58aa7..a26db79e35 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -2347,6 +2347,87 @@ describe('Session', () => { ).toContain('Duplicate provider tool call id "shell_1"'); }); + 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.'); + }); + mockToolRegistry.getTool.mockReturnValue({ + name: 'ask_user_question', + kind: core.Kind.Other, + displayName: 'Ask User Question', + description: 'Ask user question', + build, + canUpdateOutput: false, + isOutputMarkdown: true, + }); + + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'ask_1', + name: 'ask_user_question', + args: { questions: '[{"question":"Continue?"}]' }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'ask_2', + name: 'ask_user_question', + args: { questions: '[{"question":"Continue?"}]' }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'ask_3', + name: 'ask_user_question', + args: { questions: '[{"question":"Continue?"}]' }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValueOnce(createEmptyStream()); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'ask me before continuing' }], + }); + + expect(build).toHaveBeenCalledTimes(3); + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(3); + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'Stopping ACP turn after repeated tool parameter errors', + ), + ); + }); + 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 bd79077261..7d98e025ad 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -106,6 +106,9 @@ import { endToolExecutionSpan, logConversationFinishedEvent, ConversationFinishedEvent, + logLoopDetected, + LoopDetectedEvent, + LoopType, acquireSleepInhibitor, clearGoalTerminalObserver, setGoalTerminalObserver, @@ -209,11 +212,85 @@ type RunToolResult = { parts: Part[]; stopAfterPermissionCancel: boolean; repeatedDuplicateProviderToolCall?: boolean; + loopDetected?: boolean; }; +type DaemonToolLoopState = { + totalToolCalls: number; + invalidToolParamErrors: Map; + loopDetected: boolean; +}; + +const DAEMON_TURN_TOOL_CALL_CAP = 100; +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.'; +function createDaemonToolLoopState(): DaemonToolLoopState { + return { + totalToolCalls: 0, + invalidToolParamErrors: new Map(), + loopDetected: false, + }; +} + +function recordDaemonLoopDetected( + config: Config, + promptId: string, + loopType: LoopType, + message: string, + loopState: DaemonToolLoopState, +): true { + if (!loopState.loopDetected) { + loopState.loopDetected = true; + debugLogger.warn(message); + logLoopDetected(config, new LoopDetectedEvent(loopType, promptId)); + } + return true; +} + +function recordDaemonToolCalls( + config: Config, + promptId: string, + loopState: DaemonToolLoopState | undefined, + count: number, +): boolean { + if (!loopState || loopState.loopDetected) + return loopState?.loopDetected ?? false; + loopState.totalToolCalls += count; + if (loopState.totalToolCalls <= DAEMON_TURN_TOOL_CALL_CAP) return false; + return recordDaemonLoopDetected( + config, + promptId, + LoopType.TURN_TOOL_CALL_CAP, + `Stopping ACP turn after ${loopState.totalToolCalls} tool calls in one turn.`, + loopState, + ); +} + +function recordDaemonInvalidToolParams( + config: Config, + promptId: string, + loopState: DaemonToolLoopState | undefined, + toolName: string, + error: Error, +): boolean { + if (!loopState || loopState.loopDetected) + return loopState?.loopDetected ?? false; + const key = `${toolName}\0${error.message}`; + const count = (loopState.invalidToolParamErrors.get(key) ?? 0) + 1; + loopState.invalidToolParamErrors.set(key, count); + if (count < DAEMON_INVALID_TOOL_PARAMS_THRESHOLD) return false; + return recordDaemonLoopDetected( + config, + promptId, + LoopType.INVALID_TOOL_PARAMS_STAGNATION, + `Stopping ACP turn after repeated tool parameter errors from ${toolName}: ${error.message}`, + loopState, + ); +} + // 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 @@ -1653,6 +1730,7 @@ export class Session implements SessionContext { let nextMessage: Content | null = { role: 'user', parts }; let turnCount = 0; + const toolLoopState = createDaemonToolLoopState(); // conversation_finished must fire on every terminal path of the // turn — the loop below has cancel/abort/no-stream early-returns @@ -1820,6 +1898,7 @@ export class Session implements SessionContext { pendingSend.signal, promptId, functionCalls, + toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { await this.#preserveCancelledPermissionToolRun( @@ -1981,6 +2060,7 @@ export class Session implements SessionContext { role: 'user', parts: continueParts, }; + const toolLoopState = createDaemonToolLoopState(); // Process the follow-up message and any tool calls that result while (nextMessage !== null) { @@ -2096,6 +2176,7 @@ export class Session implements SessionContext { pendingSend.signal, promptId, functionCalls, + toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { await this.#preserveCancelledPermissionToolRun( @@ -2293,6 +2374,10 @@ export class Session implements SessionContext { toolRun: RunToolResult, abortSignal: AbortSignal, ): Promise { + if (toolRun.loopDetected) { + debugLogger.debug('Stopping ACP turn after daemon loop detection.'); + return null; + } if (toolRun.repeatedDuplicateProviderToolCall) { debugLogger.debug( 'Stopping ACP turn after dropping repeated duplicate provider tool-call response.', @@ -2894,6 +2979,7 @@ export class Session implements SessionContext { role: 'user', parts: [...cronReminders, { text: modelText }], }; + const toolLoopState = createDaemonToolLoopState(); while (nextMessage !== null) { turnCount++; @@ -2982,6 +3068,7 @@ export class Session implements SessionContext { ac.signal, promptId, functionCalls, + toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { await this.#preserveCancelledPermissionToolRun( @@ -3197,6 +3284,7 @@ export class Session implements SessionContext { role: 'user', parts: [...notificationReminders, ...notificationParts], }; + const toolLoopState = createDaemonToolLoopState(); while (nextMessage !== null) { if (ac.signal.aborted) { @@ -3297,6 +3385,7 @@ export class Session implements SessionContext { ac.signal, promptId, functionCalls, + toolLoopState, ); if (toolRun.stopAfterPermissionCancel) { await this.#preserveCancelledPermissionToolRun( @@ -3650,7 +3739,23 @@ export class Session implements SessionContext { abortSignal: AbortSignal, promptId: string, functionCalls: FunctionCall[], + toolLoopState?: DaemonToolLoopState, ): Promise { + if ( + recordDaemonToolCalls( + this.config, + promptId, + toolLoopState, + functionCalls.length, + ) + ) { + return { + parts: [], + stopAfterPermissionCancel: false, + loopDetected: true, + }; + } + const dedupedFunctionCalls = dedupeToolCallsById(functionCalls); type ExecutableBatch = { kind: 'execute'; @@ -3846,6 +3951,7 @@ export class Session implements SessionContext { promptId, calls[idx], onStopAfterPermissionCancel, + toolLoopState, ) .then((r) => { results[idx] = r; @@ -3898,9 +4004,18 @@ export class Session implements SessionContext { abortSignal.removeEventListener('abort', propagateAbort); } let shouldStop = false; + let shouldStopForLoop = false; for (const r of results) { parts.push(...r.parts); shouldStop ||= r.stopAfterPermissionCancel; + shouldStopForLoop ||= r.loopDetected === true; + } + if (shouldStopForLoop) { + return { + parts, + stopAfterPermissionCancel: false, + loopDetected: true, + }; } if (shouldStop) { await appendSkippedAfter(parts, batch.calls[batch.calls.length - 1]); @@ -3912,8 +4027,21 @@ export class Session implements SessionContext { } } else { for (const fc of batch.calls) { - const r = await this.runTool(abortSignal, promptId, fc); + const r = await this.runTool( + abortSignal, + promptId, + fc, + undefined, + toolLoopState, + ); parts.push(...r.parts); + if (r.loopDetected) { + return { + parts, + stopAfterPermissionCancel: false, + loopDetected: true, + }; + } if (r.stopAfterPermissionCancel) { await appendSkippedAfter(parts, fc); return { @@ -3972,6 +4100,7 @@ export class Session implements SessionContext { promptId: string, fc: FunctionCall, onStopAfterPermissionCancel?: () => void, + toolLoopState?: DaemonToolLoopState, ): Promise { const callId = fc.id ?? `${fc.name}-${Date.now()}`; let args = (fc.args ?? {}) as Record; @@ -4110,8 +4239,10 @@ export class Session implements SessionContext { // Get approval mode for hook context (defined outside try for catch block access) const approvalMode = this.config.getApprovalMode(); + let toolBuildSucceeded = false; try { const invocation = tool.build(args); + toolBuildSucceeded = true; // Production AgentTool always initializes `eventEmitter` on its // invocation (`agent.ts:392`). Be defensive about the `undefined` @@ -4886,9 +5017,21 @@ export class Session implements SessionContext { errorType: undefined, }); + const loopDetected = + !activeToolAbortSignal.aborted && + !toolBuildSucceeded && + recordDaemonInvalidToolParams( + this.config, + promptId, + toolLoopState, + toolName, + error, + ); + return { parts: errorResponse(error), stopAfterPermissionCancel: nestedPermissionCancelled, + loopDetected, }; } }); // end runInToolSpanContext diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index 26e3d2ebf1..627a684c73 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -138,6 +138,8 @@ const LOOP_TYPE_LABELS: Record = { 'the model alternated between the same two tool calls in a repeating pattern', [LoopType.TURN_TOOL_CALL_CAP]: 'the model exceeded the maximum number of tool calls allowed in a single turn', + [LoopType.INVALID_TOOL_PARAMS_STAGNATION]: + 'the model repeatedly sent invalid tool parameters without correcting them', }; function formatLoopDetectedMessage(loopType: LoopType | undefined): string { @@ -150,7 +152,8 @@ function formatLoopDetectedMessage(loopType: LoopType | undefined): string { loopType === LoopType.TURN_TOOL_CALL_CAP || loopType === LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS || loopType === LoopType.SHELL_COMMAND_STAGNATION || - loopType === LoopType.GLOBAL_TOOL_CALL_DUPLICATE; + loopType === LoopType.GLOBAL_TOOL_CALL_DUPLICATE || + loopType === LoopType.INVALID_TOOL_PARAMS_STAGNATION; const hint = isAlwaysOn ? ' This is an always-on guard and cannot be disabled via `model.skipLoopDetection`.' : ' Set the `model.skipLoopDetection` setting to true to disable.'; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 94cf10afb0..57db2d099a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -346,6 +346,7 @@ export { logExtensionDisable, logExtensionEnable, logIdeConnection, + logLoopDetected, logModelSlashCommand, logPromptSuggestion, logSpeculation, @@ -360,6 +361,7 @@ export { ExtensionUninstallEvent, IdeConnectionEvent, IdeConnectionType, + LoopDetectedEvent, LoopType, ModelSlashCommandEvent, PromptSuggestionEvent, diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index a2a2b2f49f..bce64ddb55 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -443,6 +443,8 @@ export enum LoopType { ALTERNATING_TOOL_CALL_PATTERN = 'alternating_tool_call_pattern', /** Total tool calls in a single turn exceeded the always-on hard cap, regardless of pattern. */ TURN_TOOL_CALL_CAP = 'turn_tool_call_cap', + /** The same tool repeatedly failed schema validation with fresh tool-call ids. */ + INVALID_TOOL_PARAMS_STAGNATION = 'invalid_tool_params_stagnation', } export class LoopDetectedEvent implements BaseTelemetryEvent {