diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index a69d4517c01..3aa9d45ce29 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -44,6 +44,7 @@ import { type ToolCallRequestInfo, } from '../../core/turn.js'; import { LoopDetectionService } from '../../services/loopDetectionService.js'; +import type { LoopType } from '../../telemetry/types.js'; import { CoreToolScheduler, type ToolCall, @@ -317,6 +318,12 @@ export interface ReasoningLoopResult { terminateMode: AgentTerminateMode | null; /** Number of model round-trips completed. */ turnsUsed: number; + /** + * Which loop detector fired, when terminateMode is LOOP_DETECTED (issue + * #9450 — attribution for stops that all render as one generic message + * otherwise). null otherwise. + */ + loopType?: LoopType | null; } /** @@ -993,6 +1000,13 @@ export class AgentCore { } as AgentRoundEvent); const functionCalls: FunctionCall[] = []; + // callIds already streamed to the loop guard this attempt. Mirrors + // dedupeToolCallsById (which collapses execution to one call per + // id): a provider can emit the same call id twice in one response, + // and counting both emissions would leave the request counters one + // ahead of the executed result evidence (one recordToolResult per + // executed call), fail-safe-halting a productive stateful poller. + const loopGuardStreamedCallIds = new Set(); let roundText = ''; let roundThoughtText = ''; let lastUsage: GenerateContentResponseUsageMetadata | undefined = @@ -1030,6 +1044,7 @@ export class AgentCore { stickyMaxOutputTokens = streamEvent.maxOutputTokensEscalated; } functionCalls.length = 0; + loopGuardStreamedCallIds.clear(); roundText = ''; roundThoughtText = ''; lastUsage = undefined; @@ -1111,6 +1126,17 @@ export class AgentCore { for (const fc of chunkFunctionCalls) { const toolName = String(fc.name); + // Provider-duplicate emissions of an already-streamed call id + // execute once (dedupeToolCallsById collapses them), so feed + // the loop guard once — request counts and result evidence + // must stay the same population. Id-less calls are never + // deduped, mirroring dedupeToolCallsById. + if (fc.id) { + if (loopGuardStreamedCallIds.has(fc.id)) { + continue; + } + loopGuardStreamedCallIds.add(fc.id); + } if ( checkSubagentLoop({ type: LlmEventType.ToolCallRequest, @@ -1200,6 +1226,24 @@ export class AgentCore { terminateMode = AgentTerminateMode.LOOP_DETECTED; break; } + // Result-aware loop guards (issue #9450): stateful reads like + // task_list may legitimately repeat with identical arguments while + // the shared task board changes, so the detector must see each + // executed result before the next round re-emits the call. + for (const toolResult of toolCallResult.results) { + if ( + loopDetector.recordToolResult( + { name: toolResult.toolName, args: toolResult.args }, + toolResult.responseParts, + ) + ) { + terminateMode = AgentTerminateMode.LOOP_DETECTED; + break; + } + } + if (terminateMode === AgentTerminateMode.LOOP_DETECTED) { + break; + } currentMessages = toolCallResult.messages; const externalInputs = this.drainExternalInputs(options); @@ -1317,6 +1361,9 @@ export class AgentCore { text: finalText, terminateMode, turnsUsed: turnCounter, + ...(terminateMode === AgentTerminateMode.LOOP_DETECTED + ? { loopType: loopDetector.getLastLoopType() } + : {}), }; } @@ -1625,6 +1672,14 @@ export class AgentCore { ): Promise<{ messages: Content[]; repeatedDuplicateProviderToolCall: boolean; + /** Executed calls with their model-visible results, in call order. + * Consumed by the loop detector for result-aware stateful-read guards + * (issue #9450). */ + results: Array<{ + toolName: string; + args: Record; + responseParts: Part[]; + }>; }> { const responseByCallId = new Map< string, @@ -1678,6 +1733,7 @@ export class AgentCore { return { messages: [{ role: 'user', parts: [] }], repeatedDuplicateProviderToolCall: true, + results: [], }; } @@ -2267,9 +2323,31 @@ export class AgentCore { timestamp: Date.now(), }); + // Pair each executed call with its model-visible (finalized) result so + // the reasoning loop can feed the loop detector's result-aware guards. + const finalizedByCallId = new Map( + finalizedResponses.map((response) => [response.callId, response]), + ); + const results: Array<{ + toolName: string; + args: Record; + responseParts: Part[]; + }> = []; + for (const fc of uniqueFunctionCalls) { + const callId = callIdByFunctionCall.get(fc) ?? fc.id ?? ''; + const finalized = finalizedByCallId.get(callId); + if (!finalized) continue; + results.push({ + toolName: String(fc.name ?? ''), + args: (fc.args ?? {}) as Record, + responseParts: finalized.responseParts, + }); + } + return { messages: [{ role: 'user', parts: toolResponseParts }], repeatedDuplicateProviderToolCall: false, + results, }; } diff --git a/packages/core/src/agents/runtime/agent-events.ts b/packages/core/src/agents/runtime/agent-events.ts index 67da0b8c1e1..0736216d48a 100644 --- a/packages/core/src/agents/runtime/agent-events.ts +++ b/packages/core/src/agents/runtime/agent-events.ts @@ -204,6 +204,12 @@ export interface AgentExternalMessageEvent { export interface AgentFinishEvent { subagentId: string; terminateReason: string; + /** + * Which loop detector fired when terminateReason is LOOP_DETECTED + * (issue #9450), so stops are attributable in journals/telemetry instead + * of collapsing into one generic label. + */ + loopType?: string; timestamp: number; rounds?: number; totalDurationMs?: number; diff --git a/packages/core/src/agents/runtime/agent-headless.test.ts b/packages/core/src/agents/runtime/agent-headless.test.ts index ac86bd7baf6..ef40d8c6945 100644 --- a/packages/core/src/agents/runtime/agent-headless.test.ts +++ b/packages/core/src/agents/runtime/agent-headless.test.ts @@ -63,6 +63,8 @@ import { WriteFileTool } from '../../tools/write-file.js'; import { ToolNames } from '../../tools/tool-names.js'; import { normalizeToolNameForProvider } from '../../utils/tool-name-utils.js'; import { LoopDetectionService } from '../../services/loopDetectionService.js'; +import { logSubagentExecution } from '../../telemetry/loggers.js'; +import type { SubagentExecutionEvent } from '../../telemetry/types.js'; vi.mock('../../core/llm-chat.js'); vi.mock('../../core/contentGenerator.js', async (importOriginal) => { @@ -107,6 +109,10 @@ vi.mock('../../core/environmentContext.js', () => ({ vi.mock('../../core/nonInteractiveToolExecutor.js'); vi.mock('../../ide/ide-client.js'); vi.mock('../../core/client.js'); +vi.mock('../../telemetry/loggers.js', async (importOriginal) => ({ + ...(await importOriginal()), + logSubagentExecution: vi.fn(), +})); vi.mock('../../skills/skill-manager.js', () => { const SkillManagerMock = vi.fn(); @@ -2236,6 +2242,397 @@ describe('subagent.ts', () => { expect(scope.getTerminateMode()).toBe(AgentTerminateMode.LOOP_DETECTED); }); + it('keeps polling task_list while the task board changes (issue #9450)', async () => { + // Identical task_list arguments do not imply an identical result: + // teammates mutate the shared board between calls. The agent must + // not be halted while the observed results keep changing. + const taskListToolDef: FunctionDeclaration = { + name: 'task_list', + description: 'Lists team tasks', + parameters: { type: Type.OBJECT, properties: {} }, + }; + + const { config } = await createMockConfig({ + getFunctionDeclarationsFiltered: vi + .fn() + .mockReturnValue([taskListToolDef]), + getTool: vi.fn().mockReturnValue(undefined), + }); + const toolConfig: ToolConfig = { tools: ['task_list'] }; + const pollCount = 8; // well past the consecutive-identical threshold + const taskListArgs = { + status: 'in_progress', + owner: 'peer-a', + blockedBy: '', + }; + + mockSendMessageStream.mockImplementation( + createMockStream([ + ...Array.from({ length: pollCount }, (_, index) => [ + { + id: `poll_${index + 1}`, + name: 'task_list', + args: taskListArgs, + }, + ]), + 'stop', + ]), + ); + + let boardVersion = 0; + const taskListInvocation = { + params: taskListArgs, + getDescription: vi.fn().mockReturnValue('List tasks'), + toolLocations: vi.fn().mockReturnValue([]), + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + // A peer completes/claims a task between polls, so every result + // differs even though the arguments are identical. + execute: vi.fn().mockImplementation(async () => { + boardVersion += 1; + const status = boardVersion % 2 === 0 ? 'completed' : 'in_progress'; + return { + llmContent: `#7 [${status}] @peer-a — task (v${boardVersion})`, + returnDisplay: 'Listed tasks', + }; + }), + }; + const taskListTool = { + name: 'task_list', + displayName: 'Task List', + description: 'List tasks in the team task list', + kind: 'READ' as const, + schema: taskListToolDef, + build: vi.fn().mockImplementation(() => taskListInvocation), + canUpdateOutput: false, + isOutputMarkdown: false, + } as unknown as AnyDeclarativeTool; + vi.mocked( + (config.getToolRegistry() as unknown as ToolRegistry).getTool, + ).mockImplementation((name: string) => + name === 'task_list' ? taskListTool : undefined, + ); + + const scope = await AgentHeadless.create( + 'test-agent', + config, + promptConfig, + defaultModelConfig, + defaultRunConfig, + toolConfig, + ); + + await scope.execute(new ContextState()); + + expect(taskListInvocation.execute).toHaveBeenCalledTimes(pollCount); + expect(mockSendMessageStream).toHaveBeenCalledTimes(pollCount + 1); + expect(scope.getTerminateMode()).not.toBe( + AgentTerminateMode.LOOP_DETECTED, + ); + }); + + it('still halts task_list polling when the board is frozen (issue #9450)', async () => { + const taskListToolDef: FunctionDeclaration = { + name: 'task_list', + description: 'Lists team tasks', + parameters: { type: Type.OBJECT, properties: {} }, + }; + + const { config } = await createMockConfig({ + getFunctionDeclarationsFiltered: vi + .fn() + .mockReturnValue([taskListToolDef]), + getTool: vi.fn().mockReturnValue(undefined), + }); + const toolConfig: ToolConfig = { tools: ['task_list'] }; + const taskListArgs = { + status: 'in_progress', + owner: 'peer-a', + blockedBy: '', + }; + + mockSendMessageStream.mockImplementation( + createMockStream([ + ...Array.from({ length: 5 }, (_, index) => [ + { + id: `poll_${index + 1}`, + name: 'task_list', + args: taskListArgs, + }, + ]), + 'stop', + ]), + ); + + const taskListInvocation = { + params: taskListArgs, + getDescription: vi.fn().mockReturnValue('List tasks'), + toolLocations: vi.fn().mockReturnValue([]), + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + // No teammate activity: every poll returns the identical board. + execute: vi.fn().mockResolvedValue({ + llmContent: '#7 [in_progress] @peer-a — task', + returnDisplay: 'Listed tasks', + }), + }; + const taskListTool = { + name: 'task_list', + displayName: 'Task List', + description: 'List tasks in the team task list', + kind: 'READ' as const, + schema: taskListToolDef, + build: vi.fn().mockImplementation(() => taskListInvocation), + canUpdateOutput: false, + isOutputMarkdown: false, + } as unknown as AnyDeclarativeTool; + vi.mocked( + (config.getToolRegistry() as unknown as ToolRegistry).getTool, + ).mockImplementation((name: string) => + name === 'task_list' ? taskListTool : undefined, + ); + + const finishEvents: Array<{ loopType?: string }> = []; + const eventEmitter = new AgentEventEmitter(); + eventEmitter.on(AgentEventType.FINISH, (event: unknown) => { + finishEvents.push(event as { loopType?: string }); + }); + + const scope = await AgentHeadless.create( + 'test-agent', + config, + promptConfig, + defaultModelConfig, + defaultRunConfig, + toolConfig, + eventEmitter, + ); + + await scope.execute(new ContextState()); + + expect(mockSendMessageStream).toHaveBeenCalledTimes(5); + expect(taskListInvocation.execute).toHaveBeenCalledTimes(4); + expect(scope.getTerminateMode()).toBe(AgentTerminateMode.LOOP_DETECTED); + // The exact detector is attributable in the finish event (#9450). + expect(finishEvents).toHaveLength(1); + expect(finishEvents[0].loopType).toBe( + 'consecutive_identical_tool_calls', + ); + // The telemetry completion record carries the same attribution; a + // SubagentExecutionEvent without loop_type would silently drop the + // spread and journal the stop as unattributable. + const completionEvents = vi + .mocked(logSubagentExecution) + .mock.calls.map((call) => call[1]) + .filter( + (event): event is SubagentExecutionEvent => + event.status !== 'started', + ); + expect(completionEvents).toHaveLength(1); + expect(completionEvents[0]?.loop_type).toBe( + 'consecutive_identical_tool_calls', + ); + }); + + it('counts a provider-duplicate call id once so result evidence stays in sync (issue #9450)', async () => { + // A provider can stream the SAME call id twice in one response — the + // exact pathology dedupeToolCallsById exists for. Execution collapses + // the pair to one call (one recorded result), so the loop guard must + // also count one request; otherwise the request counter runs one + // ahead of the result evidence and the result-aware exemption + // fails safe, halting a fully productive poller. + const taskListToolDef: FunctionDeclaration = { + name: 'task_list', + description: 'Lists team tasks', + parameters: { type: Type.OBJECT, properties: {} }, + }; + + const { config } = await createMockConfig({ + getFunctionDeclarationsFiltered: vi + .fn() + .mockReturnValue([taskListToolDef]), + getTool: vi.fn().mockReturnValue(undefined), + }); + const toolConfig: ToolConfig = { tools: ['task_list'] }; + const taskListArgs = { + status: 'in_progress', + owner: 'peer-a', + blockedBy: '', + }; + + // Round 1 emits the same call id twice (the provider duplicate); the + // remaining rounds emit one call each, the board changing every time. + const duplicateId = 'dup_call_0'; + mockSendMessageStream.mockImplementation( + createMockStream([ + [ + { id: duplicateId, name: 'task_list', args: taskListArgs }, + { id: duplicateId, name: 'task_list', args: taskListArgs }, + ], + ...Array.from({ length: 5 }, (_, index) => [ + { + id: `poll_${index + 1}`, + name: 'task_list', + args: taskListArgs, + }, + ]), + 'stop', + ]), + ); + + let boardVersion = 0; + const taskListInvocation = { + params: taskListArgs, + getDescription: vi.fn().mockReturnValue('List tasks'), + toolLocations: vi.fn().mockReturnValue([]), + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + // Every executed poll returns a changed board. + execute: vi.fn().mockImplementation(async () => { + boardVersion += 1; + return { + llmContent: `#7 [in_progress] @peer-a — task (v${boardVersion})`, + returnDisplay: 'Listed tasks', + }; + }), + }; + const taskListTool = { + name: 'task_list', + displayName: 'Task List', + description: 'List tasks in the team task list', + kind: 'READ' as const, + schema: taskListToolDef, + build: vi.fn().mockImplementation(() => taskListInvocation), + canUpdateOutput: false, + isOutputMarkdown: false, + } as unknown as AnyDeclarativeTool; + vi.mocked( + (config.getToolRegistry() as unknown as ToolRegistry).getTool, + ).mockImplementation((name: string) => + name === 'task_list' ? taskListTool : undefined, + ); + + const scope = await AgentHeadless.create( + 'test-agent', + config, + promptConfig, + defaultModelConfig, + { ...defaultRunConfig, max_turns: 20 }, + toolConfig, + ); + + await scope.execute(new ContextState()); + + // The duplicate id executes once (dedupeToolCallsById), so 6 executed + // polls across 7 model turns; the changed board must carry the agent + // to goal instead of a false loop halt. + expect(taskListInvocation.execute).toHaveBeenCalledTimes(6); + expect(mockSendMessageStream).toHaveBeenCalledTimes(7); + expect(scope.getTerminateMode()).not.toBe( + AgentTerminateMode.LOOP_DETECTED, + ); + }); + + it('does not carry a stale loop attribution into a re-executed run (issue #9450)', async () => { + const taskListToolDef: FunctionDeclaration = { + name: 'task_list', + description: 'Lists team tasks', + parameters: { type: Type.OBJECT, properties: {} }, + }; + + const { config } = await createMockConfig({ + getFunctionDeclarationsFiltered: vi + .fn() + .mockReturnValue([taskListToolDef]), + getTool: vi.fn().mockReturnValue(undefined), + }); + const toolConfig: ToolConfig = { tools: ['task_list'] }; + const taskListArgs = { + status: 'in_progress', + owner: 'peer-a', + blockedBy: '', + }; + + mockSendMessageStream.mockImplementation( + createMockStream([ + ...Array.from({ length: 5 }, (_, index) => [ + { + id: `poll_${index + 1}`, + name: 'task_list', + args: taskListArgs, + }, + ]), + 'stop', + ]), + ); + + const taskListInvocation = { + params: taskListArgs, + getDescription: vi.fn().mockReturnValue('List tasks'), + toolLocations: vi.fn().mockReturnValue([]), + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + execute: vi.fn().mockResolvedValue({ + llmContent: '#7 [in_progress] @peer-a — task', + returnDisplay: 'Listed tasks', + }), + }; + const taskListTool = { + name: 'task_list', + displayName: 'Task List', + description: 'List tasks in the team task list', + kind: 'READ' as const, + schema: taskListToolDef, + build: vi.fn().mockImplementation(() => taskListInvocation), + canUpdateOutput: false, + isOutputMarkdown: false, + } as unknown as AnyDeclarativeTool; + vi.mocked( + (config.getToolRegistry() as unknown as ToolRegistry).getTool, + ).mockImplementation((name: string) => + name === 'task_list' ? taskListTool : undefined, + ); + + const finishEvents: Array<{ + loopType?: string; + terminateReason?: string; + }> = []; + const eventEmitter = new AgentEventEmitter(); + eventEmitter.on(AgentEventType.FINISH, (event: unknown) => { + finishEvents.push( + event as { loopType?: string; terminateReason?: string }, + ); + }); + eventEmitter.on(AgentEventType.ERROR, () => undefined); + + const scope = await AgentHeadless.create( + 'test-agent', + config, + promptConfig, + defaultModelConfig, + defaultRunConfig, + toolConfig, + eventEmitter, + ); + + // Run 1 halts on the frozen board with an attribution. + await scope.execute(new ContextState()); + expect(scope.getTerminateMode()).toBe(AgentTerminateMode.LOOP_DETECTED); + + // Run 2 on the same instance (stop-hook continuation / resident + // turns) errors before any loop fires: it must not carry run 1's + // loopType into its FINISH/telemetry. + mockSendMessageStream.mockRejectedValueOnce( + new Error('simulated model error'), + ); + await expect(scope.execute(new ContextState())).rejects.toThrow( + 'simulated model error', + ); + expect(scope.getTerminateMode()).toBe(AgentTerminateMode.ERROR); + + expect(finishEvents).toHaveLength(2); + expect(finishEvents[0].loopType).toBe( + 'consecutive_identical_tool_calls', + ); + expect(finishEvents[1].loopType).toBeUndefined(); + }); + it('should ignore duplicate provider tool-call ids already present in chat history', async () => { const listFilesToolDef: FunctionDeclaration = { name: 'list_files', diff --git a/packages/core/src/agents/runtime/agent-headless.ts b/packages/core/src/agents/runtime/agent-headless.ts index 899023758db..08a00fc3382 100644 --- a/packages/core/src/agents/runtime/agent-headless.ts +++ b/packages/core/src/agents/runtime/agent-headless.ts @@ -139,6 +139,8 @@ export class AgentHeadless { private readonly core: AgentCore; private finalText: string = ''; private terminateMode: AgentTerminateMode = AgentTerminateMode.ERROR; + // Which loop detector fired when terminateMode is LOOP_DETECTED (#9450). + private loopType: string | null = null; private chat?: LlmChat; private toolsList?: FunctionDeclaration[]; private executing = false; @@ -225,6 +227,10 @@ export class AgentHeadless { this.executing = true; this.finalText = ''; this.terminateMode = AgentTerminateMode.ERROR; + // A re-executed instance (stop-hook continuation, resident turns) must + // not carry the previous run's loop attribution into an ERROR/FINISH + // spread; the field is only meaningful for a LOOP_DETECTED stop. + this.loopType = null; const resetStats = options.resetStats !== false; if (resetStats) { this.core.resetExecutionStats(); @@ -385,6 +391,7 @@ export class AgentHeadless { this.finalText = result.text; this.terminateMode = result.terminateMode ?? AgentTerminateMode.GOAL; + this.loopType = result.loopType ?? null; } catch (error) { debugLogger.error('Error during subagent execution:', error); this.terminateMode = AgentTerminateMode.ERROR; @@ -402,6 +409,7 @@ export class AgentHeadless { this.core.eventEmitter?.emit(AgentEventType.FINISH, { subagentId: this.core.subagentId, terminateReason: this.terminateMode, + ...(this.loopType ? { loopType: this.loopType } : {}), timestamp: Date.now(), rounds: summary.rounds, totalDurationMs: summary.totalDurationMs, @@ -420,6 +428,7 @@ export class AgentHeadless { : 'failed', { terminate_reason: this.terminateMode, + ...(this.loopType ? { loop_type: this.loopType } : {}), result: this.finalText, execution_summary: this.core.stats.formatCompact( 'Subagent execution completed', diff --git a/packages/core/src/agents/runtime/agent-interactive.test.ts b/packages/core/src/agents/runtime/agent-interactive.test.ts index 2ff6d965bf6..920bf868a41 100644 --- a/packages/core/src/agents/runtime/agent-interactive.test.ts +++ b/packages/core/src/agents/runtime/agent-interactive.test.ts @@ -16,7 +16,8 @@ import type { } from './agent-events.js'; import { ContextState } from './agent-headless.js'; import type { AgentInteractiveConfig } from './agent-types.js'; -import { AgentStatus } from './agent-types.js'; +import { AgentStatus, AgentTerminateMode } from './agent-types.js'; +import { LoopType } from '../../telemetry/types.js'; import { getCurrentAgentDepth, getCurrentAgentId, @@ -33,7 +34,12 @@ function createMockCore( overrides: { chatValue?: unknown; nullChat?: boolean; - loopResult?: { text: string; terminateMode: null; turnsUsed: number }; + loopResult?: { + text: string; + terminateMode: AgentTerminateMode | null; + turnsUsed: number; + loopType?: LoopType; + }; } = {}, ) { const emitter = new AgentEventEmitter(); @@ -324,6 +330,45 @@ describe('AgentInteractive', () => { await agent.shutdown(); }); + it('surfaces the exact loop detector in the interactive stop message (issue #9450)', async () => { + // A loop stop must name its detector (issue #9450 requirement #7): the + // visible info message and lastRoundError both carry the LoopType, so a + // future regression collapsing stops back into the generic label fails + // here instead of shipping unattributable stops. + const { core } = createMockCore({ + loopResult: { + text: '', + terminateMode: AgentTerminateMode.LOOP_DETECTED, + turnsUsed: 3, + loopType: LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, + }, + }); + const agent = new AgentInteractive( + createConfig({ initialTask: 'go' }), + core, + ); + + await agent.start(context); + // A loop-detected round settles the agent as failed (the round error + // path); the stop message must already have been pushed by then. + await vi.waitFor(() => { + expect(['idle', 'failed']).toContain(agent.getStatus()); + }); + await vi.waitFor(() => { + expect(agent.getMessages().some((m) => m.role === 'info')).toBe(true); + }); + + const stopMessages = agent + .getMessages() + .filter((m) => m.role === 'info') + .map((m) => String(m.content)); + expect(stopMessages).toContain( + 'Agent stopped: duplicate tool-call loop detected (consecutive_identical_tool_calls).', + ); + + await agent.shutdown(); + }); + it('should set status to failed when chat creation fails', async () => { const { core } = createMockCore({ nullChat: true }); const config = createConfig(); diff --git a/packages/core/src/agents/runtime/agent-interactive.ts b/packages/core/src/agents/runtime/agent-interactive.ts index abf94cd6c9d..9270cb4f986 100644 --- a/packages/core/src/agents/runtime/agent-interactive.ts +++ b/packages/core/src/agents/runtime/agent-interactive.ts @@ -41,6 +41,7 @@ import { type AgentInteractiveConfig, type AgentMessage, } from './agent-types.js'; +import type { LoopType } from '../../telemetry/types.js'; const debugLogger = createDebugLogger('AGENT_INTERACTIVE'); @@ -239,11 +240,13 @@ export class AgentInteractive { result.terminateMode && result.terminateMode !== AgentTerminateMode.GOAL ) { - const msg = terminateModeMessage(result.terminateMode); + const msg = terminateModeMessage(result.terminateMode, result.loopType); if (msg) { this.addMessage('info', msg.text, { metadata: { level: msg.level } }); } - this.lastRoundError = `Terminated: ${result.terminateMode}`; + this.lastRoundError = result.loopType + ? `Terminated: ${result.terminateMode} (${result.loopType})` + : `Terminated: ${result.terminateMode}`; } } catch (err) { // User-initiated cancellation already logged by cancelCurrentRound(). @@ -520,6 +523,7 @@ export class AgentInteractive { */ function terminateModeMessage( mode: AgentTerminateMode, + loopType?: LoopType | null, ): { text: string; level: 'info' | 'warning' | 'error' } | null { switch (mode) { case AgentTerminateMode.MAX_TURNS: @@ -533,7 +537,11 @@ function terminateModeMessage( return { text: 'Agent stopped due to an error.', level: 'error' }; case AgentTerminateMode.LOOP_DETECTED: return { - text: 'Agent stopped: duplicate tool-call loop detected.', + // Name the exact detector so a stop is attributable (issue #9450) + // instead of collapsing every loop type into one generic label. + text: loopType + ? `Agent stopped: duplicate tool-call loop detected (${loopType}).` + : 'Agent stopped: duplicate tool-call loop detected.', level: 'error', }; case AgentTerminateMode.CANCELLED: diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 49be3dde597..db4bcd2394d 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -8174,6 +8174,214 @@ hello expect(client['pendingMemoryPrefetch']).toBeUndefined(); }); + // Drives sendMessageStream with ToolResult messages whose + // functionResponse ids match previously streamed ToolCallRequest + // callIds, exercising the result-aware recording branch on the main + // interactive path (issue #9450). + async function runTaskListPollTurns( + board: (round: number) => string, + maxRounds = 9, + ) { + const promptId = 'prompt-task-list-poll'; + const taskListArgs = { status: 'in_progress', owner: 'peer-a' }; + const allEvents: Array<{ type: string; value?: unknown }> = []; + for (let round = 0; round <= maxRounds; round++) { + mockTurnRunFn.mockReturnValueOnce( + (async function* () { + yield { + type: LlmEventType.ToolCallRequest, + value: { + callId: `tl-${round}`, + name: 'task_list', + args: taskListArgs, + isClientInitiated: false, + prompt_id: promptId, + }, + }; + yield { + type: LlmEventType.ToolCallRequest, + value: { + callId: `other-${round}`, + name: 'tool_b', + args: { step: round }, + isClientInitiated: false, + prompt_id: promptId, + }, + }; + })(), + ); + const contents = + round === 0 + ? [{ text: 'poll the board' }] + : [ + { + functionResponse: { + id: `tl-${round - 1}`, + name: 'task_list', + response: { output: board(round - 1) }, + }, + }, + { + functionResponse: { + id: `other-${round - 1}`, + name: 'tool_b', + response: { output: `step ${round - 1}` }, + }, + }, + ]; + const events = await fromAsync( + client.sendMessageStream( + contents as never, + new AbortController().signal, + promptId, + { + type: + round === 0 + ? SendMessageType.UserQuery + : SendMessageType.ToolResult, + }, + ), + ); + allEvents.push(...(events as Array<{ type: string; value?: unknown }>)); + if ( + allEvents.some((e) => e.type === LlmEventType.LoopDetected) || + !events.some((e) => e.type === LlmEventType.ToolCallRequest) + ) { + return allEvents; + } + } + return allEvents; + } + + it('halts the interactive turn when paired ToolResults show a frozen stateful board (#9450)', async () => { + const events = await runTaskListPollTurns(() => 'frozen board'); + const loopEvent = events.find( + (e) => e.type === LlmEventType.LoopDetected, + ); + expect(loopEvent).toBeDefined(); + expect( + (loopEvent?.value as { loopType?: string } | undefined)?.loopType, + ).toBe('global_tool_call_duplicate'); + }); + + it('keeps the interactive turn alive while paired ToolResults keep changing (#9450)', async () => { + const events = await runTaskListPollTurns((round) => `board v${round}`); + expect(events.some((e) => e.type === LlmEventType.LoopDetected)).toBe( + false, + ); + }); + + // Variant of runTaskListPollTurns that polls ONLY task_list (no + // interleaved tool), so identical (name, args) build one unbroken + // consecutive streak across rounds. Round 0 streams the same call id + // twice — execution collapses it into one executed call and one + // functionResponse — so request counts and result evidence desync unless + // the loop-guard feed counts one event per call id per attempt + // (issue #9450). + async function runDuplicateIdTaskListPollTurns( + board: (round: number) => string, + maxRounds = 6, + ) { + const promptId = 'prompt-task-list-dup-poll'; + const taskListArgs = { status: 'in_progress', owner: 'peer-a' }; + const allEvents: Array<{ type: string; value?: unknown }> = []; + for (let round = 0; round <= maxRounds; round++) { + const request = (callId: string) => ({ + type: LlmEventType.ToolCallRequest, + value: { + callId, + name: 'task_list', + args: taskListArgs, + isClientInitiated: false, + prompt_id: promptId, + }, + }); + mockTurnRunFn.mockReturnValueOnce( + (async function* () { + yield request(`tl-${round}`); + if (round === 0) { + // Provider-duplicate emission of the same call id: execution + // collapses it (one functionResponse comes back below), so the + // loop-guard feed must count it once. + yield request(`tl-${round}`); + } + })(), + ); + const contents = + round === 0 + ? [{ text: 'poll the board' }] + : [ + { + functionResponse: { + id: `tl-${round - 1}`, + name: 'task_list', + response: { output: board(round - 1) }, + }, + }, + ]; + const events = await fromAsync( + client.sendMessageStream( + contents as never, + new AbortController().signal, + promptId, + { + type: + round === 0 + ? SendMessageType.UserQuery + : SendMessageType.ToolResult, + }, + ), + ); + allEvents.push(...(events as Array<{ type: string; value?: unknown }>)); + if ( + allEvents.some((e) => e.type === LlmEventType.LoopDetected) || + !events.some((e) => e.type === LlmEventType.ToolCallRequest) + ) { + return allEvents; + } + } + return allEvents; + } + + it('counts a provider-duplicate call id once so changed-board polls never halt (#9450)', async () => { + const events = await runDuplicateIdTaskListPollTurns( + (round) => `board v${round}`, + ); + expect(events.some((e) => e.type === LlmEventType.LoopDetected)).toBe( + false, + ); + // The turn kept polling through every round: 7 rounds, 7 unique call + // ids, 8 streamed events (round 0's id is emitted twice and both + // emissions still reach consumers — only the guard feed is deduped). + // Without the feed dedup the duplicate round-0 emission desyncs the + // request counter one ahead of the result evidence and the guard halts + // the streak mid-poll. + const taskListRequests = events.filter( + (e) => + e.type === LlmEventType.ToolCallRequest && + (e.value as { name?: string }).name === 'task_list', + ); + expect(taskListRequests).toHaveLength(8); + expect( + new Set( + taskListRequests.map((e) => (e.value as { callId: string }).callId), + ).size, + ).toBe(7); + }); + + it('still halts a frozen board despite the duplicate-call-id feed dedup (#9450)', async () => { + const events = await runDuplicateIdTaskListPollTurns( + () => 'frozen board', + ); + const loopEvent = events.find( + (e) => e.type === LlmEventType.LoopDetected, + ); + expect(loopEvent).toBeDefined(); + expect( + (loopEvent?.value as { loopType?: string } | undefined)?.loopType, + ).toBe('consecutive_identical_tool_calls'); + }); + it('should halt via the always-on turn cap before the skipLoopDetection gate', async () => { let abortHandlerInvoked = false; mockMemoryManager.recall.mockImplementation((_root, _query, opts) => { diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index e27817332b0..64c325783d5 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -3696,6 +3696,43 @@ export class LlmClient { } if (messageType === SendMessageType.ToolResult) { + // Record executed tool results for stateful read tools (task_list) + // so the loop guards can distinguish productive re-polling — the + // shared task board changed between identical calls — from a stuck + // loop (issue #9450). A detection here (the result-aware global + // duplicate count) halts the turn exactly like the event-loop + // guards below. + for (const part of requestToSend) { + if ( + typeof part !== 'object' || + part === null || + !('functionResponse' in part) + ) { + continue; + } + const functionResponseId = (part as Part).functionResponse?.id; + if (!functionResponseId) continue; + if ( + this.loopDetector.recordToolResultByCallId(functionResponseId, [ + part as Part, + ]) + ) { + for (const goalEvent of await finalizeInterruptedGoalTurn()) { + yield goalEvent; + } + const loopType = this.loopDetector.getLastLoopType(); + yield { + type: LlmEventType.LoopDetected, + ...(loopType && { value: { loopType } }), + }; + await arenaAgentClient?.reportError('Loop detected'); + this.lastApiCompletionTimestamp = Date.now(); + endCurrentInteraction('error', 'loop detected', 'loop_detected'); + this.cancelPendingMemoryPrefetch('no_safe_delivery_point'); + this.fireLoopDetectedStopFailure(loopType); + return turn; + } + } const toolResultMemory = await this.consumeManagedAutoMemoryRecall('tool_result'); if (toolResultMemory?.prompt) { @@ -3794,6 +3831,15 @@ export class LlmClient { const resultStream = turn.run(model, requestToSend, signal); let didUpdateIdeContextState = false; let steerInputSettled = false; + // callIds already fed to the loop guards this attempt. Mirrors the + // execution-side dedup (coreToolScheduler.dedupeRequestsByCallId / the + // interactive duplicate-call-id suppression), which collapses + // provider-duplicate emissions into one executed call and one result: + // feeding the guards once per call id keeps request counts and result + // evidence on the same population (main-session twin of the agent-core + // fix, issue #9450). Id-less requests are never deduped. Cleared on + // retry/fallback alongside the attempt's accumulated state. + const loopGuardFedCallIds = new Set(); try { for await (const event of resultStream) { if (!steerInputSettled) { @@ -3813,6 +3859,7 @@ export class LlmClient { event.type === LlmEventType.ModelFallback ) { hasToolCalls = false; + loopGuardFedCallIds.clear(); agentOutput.restartAttempt( event.type === LlmEventType.Retry && event.isContinuation === true, @@ -3832,11 +3879,28 @@ export class LlmClient { didUpdateIdeContextState = true; } + // A provider-duplicate emission of an already-fed call id executes + // once (the schedulers collapse it), so feed the loop guards once — + // counting both emissions would leave the request counters one ahead + // of the executed result evidence and fail-safe-halt a productive + // stateful poller (issue #9450). The event itself still flows to + // consumers below; only the guard feed is deduped. + let duplicateLoopGuardRequest = false; + if (event.type === LlmEventType.ToolCallRequest) { + const fedCallId = event.value.callId; + if (fedCallId) { + duplicateLoopGuardRequest = loopGuardFedCallIds.has(fedCallId); + loopGuardFedCallIds.add(fedCallId); + } + } + // Always-on safety checks (consecutive-identical tool-call guard, // shell inspection stagnation, and per-turn tool-call cap). These fire // before the skipLoopDetection gate so they cannot be bypassed by // configuration. - const alwaysOnLoop = this.loopDetector.checkAlwaysOnSafeties(event); + const alwaysOnLoop = + !duplicateLoopGuardRequest && + this.loopDetector.checkAlwaysOnSafeties(event); if (alwaysOnLoop) { // Drop every tool call collected before the guard fired so the run // halts here instead of spawning a continuation that re-trips it. @@ -3874,6 +3938,7 @@ export class LlmClient { // relaxes the heuristics (see nonInteractiveCli.ts). const skipLoopDetection = this.config.getSkipLoopDetection(); const heuristicLoop = + !duplicateLoopGuardRequest && !skipLoopDetection && this.loopDetector.addAndCheckHeuristicLoops(event); if (heuristicLoop) { diff --git a/packages/core/src/services/loopDetectionService.test.ts b/packages/core/src/services/loopDetectionService.test.ts index 59ba5929537..c1ffc192343 100644 --- a/packages/core/src/services/loopDetectionService.test.ts +++ b/packages/core/src/services/loopDetectionService.test.ts @@ -5,6 +5,8 @@ */ import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { createHash } from 'node:crypto'; +import type { Part } from '@google/genai'; import type { Config } from '../config/config.js'; import type { ServerLlmContentEvent, @@ -18,6 +20,7 @@ import { LlmEventType } from '../core/turn.js'; import * as loggers from '../telemetry/loggers.js'; import { LoopType } from '../telemetry/types.js'; import type { DebugLogger } from '../utils/debugLogger.js'; +import { FULL_OUTPUT_DIGEST_LABEL } from '../tools/truncation.js'; import { DEFAULT_MAX_TOOL_CALLS_PER_TURN, LoopDetectionService, @@ -50,12 +53,14 @@ describe('LoopDetectionService', () => { const makeConfig = ( cap: number = DEFAULT_MAX_TOOL_CALLS_PER_TURN, explicit = false, + skipLoopDetection = true, ): Config => ({ getTelemetryEnabled: () => true, getMaxToolCallsPerTurn: () => cap, isMaxToolCallsPerTurnExplicit: () => explicit, getDebugLogger: () => mockDebugLogger, + getSkipLoopDetection: () => skipLoopDetection, }) as unknown as Config; beforeEach(() => { @@ -2696,4 +2701,596 @@ describe('LoopDetectionService', () => { ); }); }); + + describe('Result-aware guards for stateful read tools (issue #9450)', () => { + // Identical `task_list` arguments do not imply an identical result: + // teammates mutate the shared task board between calls. These tests pin + // the fix for the false positive where a polling teammate was halted by + // the argument-only guards while the board kept changing. + const TASK_LIST_ARGS = { + status: 'in_progress', + owner: 'peer-a', + blockedBy: '', + }; + + const taskListEvent = ( + callId: string, + args: Record = TASK_LIST_ARGS, + ): ServerLlmToolCallRequestEvent => ({ + type: LlmEventType.ToolCallRequest, + value: { + name: 'task_list', + args, + callId, + isClientInitiated: false, + prompt_id: 'test-prompt-id', + }, + }); + + const taskListResult = (boardState: string, callId = 'call-x'): Part[] => [ + { + functionResponse: { + id: callId, + name: 'task_list', + response: { output: boardState }, + }, + }, + ]; + + it('still halts at the threshold when no results were recorded (fail-safe)', () => { + // A wiring gap must never loosen the DashScope protection (#5019): + // without result evidence the guard behaves exactly as pre-fix. + const event = taskListEvent('call-1'); + for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) { + expect(service.checkAlwaysOnSafeties(event)).toBe(false); + } + expect(service.checkAlwaysOnSafeties(event)).toBe(true); + expect(service.getLastLoopType()).toBe( + LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, + ); + }); + + it('still halts when result evidence is only partial (fail-safe)', () => { + // Incomplete evidence (a dropped/failed execution records no result) + // must never grant the result-aware exemption: the guard needs one + // recorded result per preceding request before it trusts a streak. + let fired = false; + for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD; i++) { + fired = service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + if (i !== 2) { + // Skip one recording mid-streak: 4 results for 5 requests. + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult('frozen board'), + ); + } + } + expect(fired).toBe(true); + expect(service.getLastLoopType()).toBe( + LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, + ); + }); + + it('still halts at the threshold when every result is unchanged', () => { + const unchanged = '#1 [in_progress] @peer-a — task'; + let fired = false; + for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD; i++) { + fired = service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(unchanged), + ); + } + expect(fired).toBe(true); + expect(service.getConsecutiveToolCallCount()).toBe( + TOOL_CALL_LOOP_THRESHOLD, + ); + expect(service.getLastLoopType()).toBe( + LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, + ); + }); + + it('does not halt while the task board keeps changing between identical calls', () => { + let fired = false; + // Well past the argument-only threshold: every poll returns a changed + // board (a peer completed/claimed a task between calls), which is the + // productive polling pattern the team prompt encourages. + for (let i = 0; i < 4 * TOOL_CALL_LOOP_THRESHOLD; i++) { + fired = service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(`board state v${i}`), + ); + } + expect(fired).toBe(false); + expect(loggers.logLoopDetected).not.toHaveBeenCalled(); + }); + + it('keeps productive polling alive past the adaptive per-turn cap', () => { + // With the default (adaptive) cap, a turn beyond the soft cap halts + // only on a stuck-repetition signal. Changed results must not build + // that signal, so polling continues past the soft cap. + const defaultCapService = new LoopDetectionService(makeConfig()); + defaultCapService.reset('cap-prompt'); + let fired = false; + for (let i = 0; i < DEFAULT_MAX_TOOL_CALLS_PER_TURN + 20; i++) { + fired = defaultCapService.checkAlwaysOnSafeties( + taskListEvent(`call-${i}`), + ); + if (fired) break; + defaultCapService.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(`board state v${i}`), + ); + } + expect(fired).toBe(false); + }); + + it('does not accumulate an oscillating board toward the global-duplicate halt (heuristics on)', () => { + // A board flipping between two byte-identical states returns a result + // different from its predecessor on EVERY poll — changed-state + // progress — even though each (call, result) pair recurs across the + // turn. Turn-wide pair totals would reach the threshold here; the + // consecutive identical-result count must not. + const heuristicService = new LoopDetectionService( + makeConfig(DEFAULT_MAX_TOOL_CALLS_PER_TURN, false, false), + ); + heuristicService.reset('oscillating-global'); + let detected = false; + for (let i = 0; i < 4 * GLOBAL_DUPLICATE_THRESHOLD && !detected; i++) { + detected = heuristicService.addAndCheck(taskListEvent(`call-${i}`)); + if (detected) break; + detected = heuristicService.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(i % 2 === 0 ? 'board A' : 'board B'), + ); + } + expect(detected).toBe(false); + expect(loggers.logLoopDetected).not.toHaveBeenCalled(); + }); + + it('keeps an oscillating board alive past the adaptive cap (skipLoopDetection default)', () => { + // CLI default (skipLoopDetection=true): the pair totals previously + // fed capMaxKeyRepeat, so an oscillating board past the 100-call soft + // cap was halted by the always-on adaptive cap. Every poll changing + // the result must keep the stuck signal at bay instead. + const defaultCapService = new LoopDetectionService(makeConfig()); + defaultCapService.reset('oscillating-cap'); + let fired = false; + for (let i = 0; i < DEFAULT_MAX_TOOL_CALLS_PER_TURN + 20; i++) { + fired = defaultCapService.checkAlwaysOnSafeties( + taskListEvent(`call-${i}`), + ); + if (fired) break; + fired = defaultCapService.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(i % 2 === 0 ? 'board A' : 'board B'), + ); + if (fired) break; + } + expect(fired).toBe(false); + expect(loggers.logLoopDetected).not.toHaveBeenCalled(); + }); + + it('still halts an interleaved frozen board via the adaptive cap', () => { + // The other direction under the CLI default: a genuinely frozen board + // (same result on every poll) interleaved with other calls must still + // build the stuck signal and trip the adaptive cap past the soft cap. + const svc = new LoopDetectionService(makeConfig()); + svc.reset('frozen-cap'); + let fired = false; + for (let i = 0; i < GLOBAL_DUPLICATE_THRESHOLD && !fired; i++) { + fired = svc.checkAlwaysOnSafeties( + createToolCallRequestEvent('filler', { i }), + ); + if (fired) break; + fired = svc.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + fired = svc.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult('frozen board'), + ); + } + expect(fired).toBe(false); + // Diverse filler calls push the turn past the soft cap; the frozen + // result streak (>= threshold) is the stuck signal that halts it. + for (let i = 0; i < DEFAULT_MAX_TOOL_CALLS_PER_TURN + 20 && !fired; i++) { + fired = svc.checkAlwaysOnSafeties( + createToolCallRequestEvent('filler', { j: i }), + ); + } + expect(fired).toBe(true); + expect(svc.getLastLoopType()).toBe(LoopType.TURN_TOOL_CALL_CAP); + }); + + describe('oversized (persisted) results fingerprint as stubs (issue #9450)', () => { + // Results over the persistence threshold are rewritten into stubs + // whose envelope embeds a per-call unique path. Hashing the envelope + // verbatim would fingerprint uniquely every poll, silently disabling + // every result-aware guard for exactly the largest results. + const digestOf = (content: string): string => + createHash('sha256').update(content).digest('hex'); + + const persistedStub = ( + boardState: string, + opts: { digest?: string; path?: string } = {}, + ): string => { + const digestLine = + opts.digest !== undefined + ? `\n${FULL_OUTPUT_DIGEST_LABEL}${opts.digest}` + : ''; + return ` +Output too large (42 KB). Full output saved to: ${opts.path ?? '/tool-results/call-x.txt'}${digestLine} +Note: this file may be cleaned up after 24 hours. + +Preview (up to 2000 chars): +${boardState} +`; + }; + + const driveUntilFireOrEnd = ( + results: () => Part[], + rounds: number, + ): boolean => { + let fired = false; + for (let i = 0; i < rounds && !fired; i++) { + fired = service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + results(), + ); + } + return fired; + }; + + it('halts a frozen oversized board despite per-call unique stub paths', () => { + // Same frozen content persisted to a DIFFERENT per-call path each + // poll: the envelope varies, the digest does not, so the guard must + // still see five unchanged results and halt at the same threshold. + let callCounter = 0; + const fired = driveUntilFireOrEnd(() => { + callCounter += 1; + return taskListResult( + persistedStub('frozen oversized board', { + digest: digestOf('frozen oversized board'), + path: `/tool-results/call-${callCounter}.txt`, + }), + `call-${callCounter}`, + ); + }, 8); + expect(fired).toBe(true); + expect(service.getLastLoopType()).toBe( + LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, + ); + }); + + it('keeps an oversized board alive when mutations land beyond the preview window', () => { + // The preview covers only the first chars; the full-output digest is + // what keeps the fingerprint sensitive to mutations past it. + let version = 0; + const fired = driveUntilFireOrEnd(() => { + version += 1; + const content = `board head\n${'x'.repeat(3000)}\ntail v${version}`; + return taskListResult( + persistedStub('board head', { digest: digestOf(content) }), + ); + }, 4 * TOOL_CALL_LOOP_THRESHOLD); + expect(fired).toBe(false); + }); + + it('falls back to the path-free preview for digest-less stubs', () => { + // Stubs produced before the digest line existed: identical previews + // in different envelopes must still collide (halt), changed previews + // must not. + let callCounter = 0; + const frozen = driveUntilFireOrEnd(() => { + callCounter += 1; + return taskListResult( + persistedStub('legacy frozen preview', { + path: `/tool-results/legacy-${callCounter}.txt`, + }), + `call-${callCounter}`, + ); + }, 8); + expect(frozen).toBe(true); + + const svc = new LoopDetectionService(makeConfig()); + svc.reset('legacy-changed'); + let fired = false; + for (let i = 0; i < 4 * TOOL_CALL_LOOP_THRESHOLD && !fired; i++) { + fired = svc.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + fired = svc.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult( + persistedStub(`legacy preview v${i}`, { + path: `/tool-results/legacy-${i}.txt`, + }), + ), + ); + } + expect(fired).toBe(false); + }); + + it('fingerprints quoted stub markers mid-content as ordinary text', () => { + // Board content can QUOTE a stub (label + hex); only LEADING + // producer shapes are stubs, so two boards differing only in quoted + // content must still count as changed. + const quoted = (hex: string) => + `peer said:\n${FULL_OUTPUT_DIGEST_LABEL}${hex}\nend`; + let fired = false; + for (let i = 0; i < 4 * TOOL_CALL_LOOP_THRESHOLD && !fired; i++) { + fired = service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + fired = service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(quoted(digestOf(`content ${i}`))), + ); + } + expect(fired).toBe(false); + }); + }); + + it('restarts the streak when a result changed, then halts on a fresh unchanged streak', () => { + const args = TASK_LIST_ARGS; + // R1..R4: the board changes once mid-streak (v2), so R5 must NOT halt. + const states = ['v1', 'v1', 'v2', 'v1']; + for (let i = 0; i < 4; i++) { + expect(service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`))).toBe( + false, + ); + service.recordToolResult( + { name: 'task_list', args }, + taskListResult(states[i]), + ); + } + expect(service.checkAlwaysOnSafeties(taskListEvent('call-4'))).toBe( + false, + ); + service.recordToolResult( + { name: 'task_list', args }, + taskListResult('v1'), + ); + + // The streak restarted at call-4 (the reset made it request #1 of the + // new streak): call-5..call-7 stay below the threshold, and their + // unchanged results corroborate the loop, so call-8 — the 5th request + // of the restarted streak — halts. + for (let i = 5; i <= 7; i++) { + expect(service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`))).toBe( + false, + ); + service.recordToolResult( + { name: 'task_list', args }, + taskListResult('v1'), + ); + } + expect(service.checkAlwaysOnSafeties(taskListEvent('call-8'))).toBe(true); + expect(service.getLastLoopType()).toBe( + LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, + ); + }); + + it('does not change behavior for deterministic (non-stateful) tools', () => { + const event = createToolCallRequestEvent('read_file', { + file_path: '/a', + }); + for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) { + service.checkAlwaysOnSafeties(event); + // Results are recorded but ignored for non-stateful tools: identical + // args still mean an identical result, so the argument-only guard + // must fire unchanged. + service.recordToolResult( + { name: 'read_file', args: { file_path: '/a' } }, + [ + { + functionResponse: { + id: `call-${i}`, + name: 'read_file', + response: { output: `content v${i}` }, + }, + }, + ], + ); + } + expect(service.checkAlwaysOnSafeties(event)).toBe(true); + expect(service.getLastLoopType()).toBe( + LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, + ); + }); + + it('records results by callId pairing from ToolCallRequest events', () => { + for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) { + expect(service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`))).toBe( + false, + ); + expect( + service.recordToolResultByCallId( + `call-${i}`, + taskListResult(`board state v${i}`, `call-${i}`), + ), + ).toBe(false); + } + // Changed results arrived through the callId pairing, so the + // threshold-th identical request is accepted. + expect( + service.checkAlwaysOnSafeties( + taskListEvent(`call-${TOOL_CALL_LOOP_THRESHOLD - 1}`), + ), + ).toBe(false); + // Unknown callIds (never streamed through this service) are ignored. + expect( + service.recordToolResultByCallId('never-seen', taskListResult('x')), + ).toBe(false); + }); + + it('counts global duplicates on (call, result) pairs when heuristics run', () => { + const heuristicService = new LoopDetectionService( + makeConfig(DEFAULT_MAX_TOOL_CALLS_PER_TURN, false, false), + ); + heuristicService.reset('global-dup'); + + // Identical task_list calls whose results CHANGE never reach the + // global-duplicate threshold, no matter how they are interleaved. + const interleaved = ['task_list', 'tool_b', 'tool_c']; + let stateOrdinal = 0; + for (let round = 0; round < 3; round++) { + for (const name of interleaved) { + const args = name === 'task_list' ? TASK_LIST_ARGS : { step: round }; + expect( + heuristicService.addAndCheck( + createToolCallRequestEvent(name, args), + ), + ).toBe(false); + if (name === 'task_list') { + expect( + heuristicService.recordToolResult( + { name, args }, + taskListResult(`state-${stateOrdinal++}`), + ), + ).toBe(false); + } + } + } + + // A genuinely stuck poll — same call, SAME result, interleaved so the + // consecutive guard never fires — trips the result-aware global + // duplicate at the threshold. + const stuckService = new LoopDetectionService( + makeConfig(DEFAULT_MAX_TOOL_CALLS_PER_TURN, false, false), + ); + stuckService.reset('global-dup-stuck'); + let detected = false; + for ( + let round = 0; + round < GLOBAL_DUPLICATE_THRESHOLD && !detected; + round++ + ) { + for (const name of interleaved) { + const args = name === 'task_list' ? TASK_LIST_ARGS : { step: round }; + if ( + stuckService.addAndCheck(createToolCallRequestEvent(name, args)) + ) { + detected = true; + break; + } + if (name === 'task_list') { + detected = stuckService.recordToolResult( + { name, args }, + taskListResult('frozen board'), + ); + if (detected) break; + } + } + } + expect(detected).toBe(true); + expect(stuckService.getLastLoopType()).toBe( + LoopType.GLOBAL_TOOL_CALL_DUPLICATE, + ); + }); + + it('treats changed results as progress for action stagnation', () => { + const heuristicService = new LoopDetectionService( + makeConfig(DEFAULT_MAX_TOOL_CALLS_PER_TURN, false, false), + ); + heuristicService.reset('stagnation'); + + // 8+ same-name task_list calls with VARYING args (the consecutive + // guard never fires) and CHANGING results: productive polling, no + // ACTION_STAGNATION halt. + for (let i = 0; i < 12; i++) { + const args = { owner: `peer-${i % 3}` }; + expect( + heuristicService.addAndCheck( + createToolCallRequestEvent('task_list', args), + ), + ).toBe(false); + expect( + heuristicService.recordToolResult( + { name: 'task_list', args }, + taskListResult(`state v${i}`), + ), + ).toBe(false); + } + + // Same shape but the board is FROZEN: the same-name streak is not + // reset and stagnation fires. + const frozenService = new LoopDetectionService( + makeConfig(DEFAULT_MAX_TOOL_CALLS_PER_TURN, false, false), + ); + frozenService.reset('stagnation-frozen'); + let fired = false; + for (let i = 0; i < 12; i++) { + const args = { owner: `peer-${i % 3}` }; + fired = frozenService.addAndCheck( + createToolCallRequestEvent('task_list', args), + ); + if (fired) break; + frozenService.recordToolResult( + { name: 'task_list', args }, + taskListResult('frozen board'), + ); + } + expect(fired).toBe(true); + expect(frozenService.getLastLoopType()).toBe(LoopType.ACTION_STAGNATION); + }); + + it('resets result evidence on retry so a replay is judged on its own results', () => { + const unchanged = '#1 [in_progress] @peer-a — task'; + for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) { + service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(unchanged), + ); + } + expect( + service.checkAlwaysOnSafeties({ + type: LlmEventType.Retry, + } as ServerLlmStreamEvent), + ).toBe(false); + + // After the retry the replayed attempt starts with fresh evidence: + // four unchanged results are not yet enough to halt. + for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD - 1; i++) { + expect( + service.checkAlwaysOnSafeties(taskListEvent(`replay-${i}`)), + ).toBe(false); + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(unchanged), + ); + } + expect(service.checkAlwaysOnSafeties(taskListEvent('replay-4'))).toBe( + true, + ); + }); + + it('clears stateful tracking on reset()', () => { + const unchanged = '#1 [in_progress] @peer-a — task'; + service.checkAlwaysOnSafeties(taskListEvent('call-0')); + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(unchanged), + ); + service.reset('fresh-prompt'); + + // Changed results in the fresh prompt must not be compared against + // the previous prompt's fingerprint. + let fired = false; + for (let i = 0; i < 4 * TOOL_CALL_LOOP_THRESHOLD; i++) { + fired = service.checkAlwaysOnSafeties(taskListEvent(`call-${i}`)); + if (fired) break; + service.recordToolResult( + { name: 'task_list', args: TASK_LIST_ARGS }, + taskListResult(`fresh v${i}`), + ); + } + expect(fired).toBe(false); + }); + }); }); diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index 3f1173387db..6c4cd6ab74a 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -5,6 +5,7 @@ */ import { createHash } from 'node:crypto'; +import type { Part } from '@google/genai'; import type { ServerLlmStreamEvent } from '../core/turn.js'; import { LlmEventType } from '../core/turn.js'; import type { ThoughtSummary } from '../utils/thoughtUtils.js'; @@ -19,6 +20,11 @@ import { } from '../telemetry/types.js'; import type { Config } from '../config/config.js'; import { getToolCallRepeatKey } from '../tools/tool-call-repeat-key.js'; +import { + FULL_OUTPUT_DIGEST_LABEL, + PREVIEW_SIZE_CHARS, + TOOL_OUTPUT_TRUNCATED_PREFIX, +} from '../tools/truncation.js'; // Re-exported for existing importers (daemon turn-loop guard); the // implementation lives in a leaf module so replay detection in @@ -84,6 +90,21 @@ const PERIODIC_MIN_TRUNCATED_OCCURRENCES = 3; // (~5th repetition for the ~300-char block in the report). const MIN_PERIODIC_REGION_LENGTH = 1000; +// Tools whose identical arguments do NOT imply an identical result: they +// read shared state that other agents can mutate between calls (issue +// #9450 — a teammate polling `task_list` while peers keep completing tasks +// was halted by the argument-only guards). For these tools the guards below +// become result-aware: repetition only counts as a loop when the observed +// results are unchanged too. Intentionally narrow — deterministic tools keep +// the argument-only behavior, and other team tools (`send_message`, +// `task_update`) have different mutation/delivery semantics and stay out. +const STATEFUL_READ_TOOLS: ReadonlySet = new Set(['task_list']); + +// Bound for the callId → request map used to pair tool results with their +// requests (recordToolResultByCallId). Parallel tool batches are far smaller; +// the cap only protects against unpaired entries accumulating. +const MAX_TRACKED_TOOL_REQUESTS = 500; + // Thought tracking const THOUGHT_REPEAT_THRESHOLD = 3; const MAX_THOUGHT_HISTORY = 50; @@ -170,6 +191,82 @@ export function shouldHaltOnTurnToolCallCap( return isExplicitCap || totalCalls > hardCap || stuck; } +// Producer shapes of the oversized-result stubs (see tools/truncation.ts). +// Recognition is anchored on these LEADING prefixes: results like task_list +// embed peer-authored text verbatim, and that text can quote stub markers — +// honoring a marker found mid-string would let quoted content collapse or +// vary the fingerprint, so only shapes that START with a producer prefix are +// treated as stubs (issue #9450). +const STUB_PRODUCER_PREFIXES: readonly string[] = [ + '', + 'Output too large (', + TOOL_OUTPUT_TRUNCATED_PREFIX, +]; + +const STUB_PREVIEW_MARKER = `Preview (up to ${PREVIEW_SIZE_CHARS} chars):`; +const STUB_TRUNCATED_PART_MARKER = 'Truncated part of the output:\n'; + +/** + * Reads the sha256 digest a stub producer embedded for the FULL + * pre-truncation output: the label must START its line and be followed by + * exactly 64 hex chars ending the line. A mid-line mention of the label + * (quoted content) never matches (issue #9450). + */ +function extractAnchoredStubDigest(value: string): string | null { + let searchFrom = 0; + for (;;) { + const index = value.indexOf(FULL_OUTPUT_DIGEST_LABEL, searchFrom); + if (index === -1) return null; + if (index === 0 || value[index - 1] === '\n') { + const digestStart = index + FULL_OUTPUT_DIGEST_LABEL.length; + const digest = value.slice(digestStart, digestStart + 64); + const terminator = value[digestStart + 64]; + if ( + /^[0-9a-f]{64}$/.test(digest) && + (terminator === undefined || terminator === '\n' || terminator === '\r') + ) { + return digest; + } + } + searchFrom = index + FULL_OUTPUT_DIGEST_LABEL.length; + } +} + +/** + * Reduces an oversized-result stub to a stable fingerprint payload. + * Oversized tool results are rewritten into truncation stubs whose envelope + * embeds a per-call unique artifact path (`/.txt`, + * a random temp file); hashing the envelope verbatim would fingerprint + * uniquely every poll — silently disabling every result-aware guard for + * exactly the largest results (a frozen board would read as "changed every + * time", issue #9450). + * + * Prefer the producers' sha256 of the full pre-truncation output + * (FULL_OUTPUT_DIGEST_LABEL): stable across calls for identical content and + * sensitive to mutations anywhere, including beyond the preview window. + * Stubs without a digest line fall back to their path-free visible payload + * (preview, or head+tail after the truncation marker). Non-stub text passes + * through unchanged. + */ +function stripPersistenceEnvelope(value: string): string { + if (!STUB_PRODUCER_PREFIXES.some((prefix) => value.startsWith(prefix))) { + return value; + } + const digest = extractAnchoredStubDigest(value); + if (digest !== null) { + return `sha256:${digest}`; + } + for (const marker of [STUB_PREVIEW_MARKER, STUB_TRUNCATED_PART_MARKER]) { + const payloadStart = value.indexOf(marker); + if (payloadStart !== -1) { + const payload = value.slice(payloadStart + marker.length); + const closeTag = payload.indexOf(''); + return `payload:${closeTag === -1 ? payload : payload.slice(0, closeTag)}`; + } + } + return `raw:${value}`; +} + /** * Service for detecting and preventing infinite loops in AI responses. * Monitors tool call repetitions and content sentence repetitions. @@ -245,9 +342,46 @@ export class LoopDetectionService { // skipLoopDetection. capMaxKeyRepeat is the running max count of any single // (tool,args) key this turn — the stuck-repetition signal that decides // whether exceeding the soft cap halts (stuck) or is allowed (productive). + // Stateful read tools feed their consecutive identical-result count instead + // (recordToolResult), so changed-state polling never builds the signal. private capKeyCounts = new Map(); private capMaxKeyRepeat = 0; + // Result-aware tracking for stateful read tools (see STATEFUL_READ_TOOLS). + // Keyed by the (tool, args) repeat key. `resultsObserved` / + // `unchangedStreak` count results within the CURRENT consecutive-identical + // streak (restarted when the streak breaks); `lastFingerprint` survives + // streak breaks so a state change is still visible across interleaved + // calls (used by the action-stagnation reset). + private statefulRepeatState = new Map< + string, + { + resultsObserved: number; + unchangedStreak: number; + lastFingerprint: string | undefined; + } + >(); + + // Consecutive identical-result counts per repeat key for stateful read + // tools, recorded post-execution. Replaces the request-time + // global-duplicate counting and the cap's stuck-repetition counting for + // these tools: the same call returning changed state is productive and + // must not accumulate toward either halt. The count restarts at 1 + // whenever the result differs from the key's predecessor, so a board + // oscillating between two byte-identical states is changed-state progress + // on every poll and never accumulates — even though the same + // (call, result) pair recurs across the turn — while a frozen board keeps + // accumulating even when other calls are interleaved. + private statefulConsecutiveResults = new Map< + string, + { fingerprint: string; count: number } + >(); + + // callId → request pairing so results can be matched to their calls when + // the runtime only has the response (populated on ToolCallRequest events, + // consumed by recordToolResultByCallId). + private requestByCallId = new Map(); + // Loop type of the most recent firing. Bubbled up through the // LoopDetected event so callers (non-interactive CLI, telemetry) can tell // the user which detector actually fired. @@ -288,6 +422,151 @@ export class LoopDetectionService { ); } + /** + * Records the executed result of a tool call so the guards can treat + * stateful read tools (see STATEFUL_READ_TOOLS) result-aware: identical + * arguments whose results keep changing are productive polling, not a + * loop (issue #9450). Call this once per executed call, after execution + * and before the model is re-prompted with the result. Runtime paths that + * only hold the response (no name/args) can use recordToolResultByCallId. + * + * Returns true when the recorded result itself trips a detector (the + * result-aware global-duplicate count); callers must then halt the turn + * the same way they do for an event-detected loop. + */ + recordToolResult( + toolCall: { name: string; args: object }, + responseParts: readonly Part[], + ): boolean { + if (this.loopDetected) return true; + if (this.disabledForSession) return false; + if (!this.isStatefulReadTool(toolCall.name)) return false; + + const resultText = LoopDetectionService.extractResultText(responseParts); + if (resultText === null) return false; + const fingerprint = createHash('sha256').update(resultText).digest('hex'); + const key = this.getToolCallKey(toolCall); + + // Consecutive-streak evidence for the always-on guard. The state entry + // can predate the streak (lastFingerprint survives streak breaks), so + // create it lazily but only count results while a streak exists. + let state = this.statefulRepeatState.get(key); + if (!state) { + state = { + resultsObserved: 0, + unchangedStreak: 0, + lastFingerprint: undefined, + }; + this.statefulRepeatState.set(key, state); + } + const firstResult = state.lastFingerprint === undefined; + const fingerprintChanged = + !firstResult && state.lastFingerprint !== fingerprint; + if (this.lastToolCallKey === key) { + state.resultsObserved++; + if (firstResult) { + state.lastFingerprint = fingerprint; + } else if (state.lastFingerprint === fingerprint) { + state.unchangedStreak++; + } else { + state.unchangedStreak = 0; + state.lastFingerprint = fingerprint; + } + } else { + state.lastFingerprint = fingerprint; + } + + // A changed result is observable progress: restart the same-name streak + // so ACTION_STAGNATION does not fire on productive polling. + if (fingerprintChanged && this.lastSeenToolName === toolCall.name) { + this.sameNameStreak = 1; + } + + // Consecutive identical-result counting: replaces the request-time + // global-duplicate and cap stuck-repetition counting for stateful + // tools. The count restarts at 1 whenever the result differs from the + // key's predecessor, so an oscillating board (changed state on every + // poll) never accumulates toward either halt while a frozen board — + // same result on every poll, even interleaved with other calls — does. + const prior = this.statefulConsecutiveResults.get(key); + const consecutiveCount = + prior && prior.fingerprint === fingerprint ? prior.count + 1 : 1; + this.statefulConsecutiveResults.set(key, { + fingerprint, + count: consecutiveCount, + }); + if (consecutiveCount > this.capMaxKeyRepeat) { + this.capMaxKeyRepeat = consecutiveCount; + } + + // The global-duplicate detector is gated (skipLoopDetection) exactly as + // its request-time counterpart in addAndCheckHeuristicLoops. + if ( + !this.config.getSkipLoopDetection() && + consecutiveCount >= GLOBAL_DUPLICATE_THRESHOLD + ) { + this.lastLoopType = LoopType.GLOBAL_TOOL_CALL_DUPLICATE; + logLoopDetected( + this.config, + new LoopDetectedEvent( + LoopType.GLOBAL_TOOL_CALL_DUPLICATE, + this.promptId, + ), + ); + this.loopDetected = true; + return true; + } + return false; + } + + /** + * Variant of recordToolResult for runtimes that only have the response: + * the request is resolved through the callId pairing populated on + * ToolCallRequest events. Unknown callIds (e.g. client-initiated calls + * that never streamed through this service) are ignored. + */ + recordToolResultByCallId( + callId: string, + responseParts: readonly Part[], + ): boolean { + const request = this.requestByCallId.get(callId); + if (!request) return false; + this.requestByCallId.delete(callId); + return this.recordToolResult( + { name: request.name, args: request.args }, + responseParts, + ); + } + + private isStatefulReadTool(toolName: string): boolean { + return STATEFUL_READ_TOOLS.has(toolName); + } + + /** + * Reconstructs the model-visible result text from tool response parts. + * Only the fingerprint of this text is retained, never the text itself. + * Oversized results arrive as persistence stubs whose envelope embeds a + * per-call unique file path; each string value is reduced to its stable + * payload first (see stripPersistenceEnvelope) so identical underlying + * results fingerprint identically no matter where they were persisted. + * Returns null when the parts carry no functionResponse content. + */ + private static extractResultText( + responseParts: readonly Part[], + ): string | null { + const chunks: string[] = []; + for (const part of responseParts) { + const functionResponse = part.functionResponse; + if (!functionResponse) continue; + chunks.push( + JSON.stringify(functionResponse.response ?? {}, (_key, value) => + typeof value === 'string' ? stripPersistenceEnvelope(value) : value, + ), + ); + } + return chunks.length > 0 ? chunks.join('\n') : null; + } + private getToolCallKey(toolCall: { name: string; args: object }): string { return getToolCallRepeatKey(toolCall.name, toolCall.args); } @@ -329,7 +608,12 @@ export class LoopDetectionService { this.trackToolCall(event.value); const toolCallKey = this.getToolCallKey(event.value); - const globalDup = this.checkGlobalDuplicate(toolCallKey); + // Stateful read tools are counted post-execution in + // recordToolResult, on consecutive identical results instead of + // args alone (issue #9450). + const globalDup = this.isStatefulReadTool(event.value.name) + ? false + : this.checkGlobalDuplicate(toolCallKey); const alternating = this.checkAlternatingPattern(toolCallKey); const readFileLoop = this.checkReadFileLoop(); const actionStagnation = this.checkActionStagnation(); @@ -446,6 +730,15 @@ export class LoopDetectionService { this.resetToolCallCount(); this.capKeyCounts.clear(); this.capMaxKeyRepeat = 0; + // A retry replays the failed attempt's tool calls; drop the stateful + // result evidence too so the replayed attempt is judged on its own + // results (consecutive counts re-accumulate as results land, consistent + // with the capKeyCounts/globalToolCallCounts clears). + this.statefulConsecutiveResults.clear(); + for (const state of this.statefulRepeatState.values()) { + state.resultsObserved = 0; + state.unchangedStreak = 0; + } return false; } @@ -464,20 +757,42 @@ export class LoopDetectionService { // it (consecutive-identical and the adaptive cap's stuck tracker). Args // can be large (e.g. write_file content), so avoid recomputing per guard. const key = this.getToolCallKey(event.value); + const stateful = this.isStatefulReadTool(event.value.name); + + // Pair requests with their later results (recordToolResultByCallId). + // Only stateful read tools participate: recordToolResult rejects every + // other tool, so tracking them would just accumulate full args objects + // (write_file args can carry whole file contents) until eviction. + if (event.value.callId && stateful) { + this.requestByCallId.set(event.value.callId, { + name: event.value.name, + args: event.value.args, + }); + if (this.requestByCallId.size > MAX_TRACKED_TOOL_REQUESTS) { + const oldest = this.requestByCallId.keys().next().value; + if (oldest !== undefined) this.requestByCallId.delete(oldest); + } + } // Always-on stuck-repetition tracking for the adaptive cap (see // checkTurnToolCallCap): lets the cap tell a productive turn from a stuck - // one, regardless of skipLoopDetection. - this.trackCapKeyRepeat(key); + // one, regardless of skipLoopDetection. Stateful read tools are counted + // post-execution instead (recordToolResult): the same call returning + // changed state is productive and must not build the stuck signal. + if (!stateful) { + this.trackCapKeyRepeat(key); + } // Consecutive identical tool calls (same name AND identical args) are the - // one repetition signal precise enough to halt unconditionally — an - // identical call returns an identical result, so it is never productive. - // Promoted here from the opt-in tier so it protects every user regardless - // of the `skipLoopDetection` config default: the DashScope server rejects - // this pattern with a 400 (issue #5019) far below the per-turn cap, so - // the gated default left users unprotected. - if (this.checkToolCallLoop(key)) { + // one repetition signal precise enough to halt unconditionally — for + // deterministic tools an identical call returns an identical result, so + // it is never productive. Promoted here from the opt-in tier so it + // protects every user regardless of the `skipLoopDetection` config + // default: the DashScope server rejects this pattern with a 400 (issue + // #5019) far below the per-turn cap, so the gated default left users + // unprotected. For stateful read tools the guard additionally requires + // the observed results to be unchanged (issue #9450). + if (this.checkToolCallLoop(event.value, key)) { this.loopDetected = true; return true; } @@ -494,14 +809,50 @@ export class LoopDetectionService { return false; } - private checkToolCallLoop(key: string): boolean { + private checkToolCallLoop( + toolCall: { name: string; args: object }, + key: string, + ): boolean { if (this.lastToolCallKey === key) { this.toolCallRepetitionCount++; } else { + // The streak moved on: restart the result evidence for both the old + // and the new key so each consecutive streak is judged on the results + // observed within it. + for (const streakKey of [this.lastToolCallKey, key]) { + if (!streakKey) continue; + const state = this.statefulRepeatState.get(streakKey); + if (state) { + state.resultsObserved = 0; + state.unchangedStreak = 0; + } + } this.lastToolCallKey = key; this.toolCallRepetitionCount = 1; } if (this.toolCallRepetitionCount >= TOOL_CALL_LOOP_THRESHOLD) { + if (this.isStatefulReadTool(toolCall.name)) { + // Result-aware guard (issue #9450): identical arguments to a stateful + // read do not imply an identical result, so only halt when the + // executed results corroborate the loop. By the Nth identical request + // the prior N-1 results have been recorded; if they were ALL observed + // and unchanged, the repetition is genuinely unproductive. If some + // result changed, the model's re-poll was productive — restart the + // streak instead of halting. Missing result evidence (results never + // recorded for this streak) fails safe and keeps the pre-#9450 + // behavior, so the DashScope protection (#5019) is never loosened by + // a wiring gap. + const state = this.statefulRepeatState.get(key); + const expectedResults = this.toolCallRepetitionCount - 1; + if (state && state.resultsObserved >= expectedResults) { + if (state.unchangedStreak < expectedResults - 1) { + this.toolCallRepetitionCount = 1; + state.resultsObserved = 0; + state.unchangedStreak = 0; + return false; + } + } + } this.lastLoopType = LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS; logLoopDetected( this.config, @@ -1324,6 +1675,9 @@ export class LoopDetectionService { this.turnToolCallTotalCommitted = 0; this.capKeyCounts.clear(); this.capMaxKeyRepeat = 0; + this.statefulRepeatState.clear(); + this.statefulConsecutiveResults.clear(); + this.requestByCallId.clear(); } private resetToolCallCount(): void { diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts index d494f12cf62..bb33ef6d16d 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts @@ -27,6 +27,7 @@ import { SkillLaunchEvent, ProtocolTagSanitizedEvent, RipgrepRuntimeRecoveryEvent, + SubagentExecutionEvent, type ToolCallEvent, } from '../types.js'; import type { RumEvent, RumPayload } from './event-types.js'; @@ -394,6 +395,50 @@ describe('QwenLogger', () => { ); }); + it('journals the loop detector attribution on subagent loop stops', () => { + // A loop stop must stay attributable in the journal (issue #9450 + // requirement #7): dropping the loop_type spread would record the + // stop as an unattributable failure. + const logger = QwenLogger.getInstance(mockConfig)!; + const enqueueSpy = vi.spyOn(logger, 'enqueueLogEvent'); + const event = new SubagentExecutionEvent('worker-a', 'failed', { + terminate_reason: 'loop_detected', + loop_type: 'consecutive_identical_tool_calls', + }); + + logger.logSubagentExecutionEvent(event); + + expect(enqueueSpy).toHaveBeenCalledWith( + expect.objectContaining({ + event_type: 'action', + type: 'tool', + name: 'subagent_execution', + properties: expect.objectContaining({ + subagent_name: 'worker-a', + status: 'failed', + terminate_reason: 'loop_detected', + loop_type: 'consecutive_identical_tool_calls', + }), + }), + ); + }); + + it('omits loop_type from subagent journals when no loop fired', () => { + const logger = QwenLogger.getInstance(mockConfig)!; + const enqueueSpy = vi.spyOn(logger, 'enqueueLogEvent'); + const event = new SubagentExecutionEvent('worker-b', 'completed'); + + logger.logSubagentExecutionEvent(event); + + expect(enqueueSpy).toHaveBeenCalledWith( + expect.objectContaining({ + properties: expect.not.objectContaining({ + loop_type: expect.anything(), + }), + }), + ); + }); + it('logs protocol tag sanitization without model content', () => { const logger = QwenLogger.getInstance(mockConfig)!; const enqueueSpy = vi.spyOn(logger, 'enqueueLogEvent'); diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts index 678b16bedd4..d89edc9f82c 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts @@ -584,6 +584,7 @@ export class QwenLogger { subagent_name: event.subagent_name, status: event.status, terminate_reason: event.terminate_reason, + ...(event.loop_type ? { loop_type: event.loop_type } : {}), }, snapshots: JSON.stringify({ ...(event.execution_summary diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index b662acb37fd..2215e369f3b 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -1076,6 +1076,7 @@ export class SubagentExecutionEvent implements BaseTelemetryEvent { terminate_reason?: string; result?: string; execution_summary?: string; + loop_type?: string; constructor( subagent_name: string, @@ -1084,6 +1085,7 @@ export class SubagentExecutionEvent implements BaseTelemetryEvent { terminate_reason?: string; result?: string; execution_summary?: string; + loop_type?: string; }, ) { this['event.name'] = 'subagent_execution'; @@ -1093,6 +1095,7 @@ export class SubagentExecutionEvent implements BaseTelemetryEvent { this.terminate_reason = options?.terminate_reason; this.result = options?.result; this.execution_summary = options?.execution_summary; + this.loop_type = options?.loop_type; } } diff --git a/packages/core/src/tools/truncation.ts b/packages/core/src/tools/truncation.ts index c89a9127a70..1057f4ab1ab 100644 --- a/packages/core/src/tools/truncation.ts +++ b/packages/core/src/tools/truncation.ts @@ -17,10 +17,22 @@ import { ToolOutputTruncatedEvent } from '../telemetry/types.js'; const debugLogger = createDebugLogger('TRUNCATION'); -const PREVIEW_SIZE_CHARS = 2000; +export const PREVIEW_SIZE_CHARS = 2000; const MAX_FILE_SIZE_BYTES = 50 * 1024 * 1024; // 50MB export const MAX_SESSION_BYTES = 500 * 1024 * 1024; // 500MB +/** + * Label of the line `buildStub` embeds carrying a sha256 of the FULL + * pre-truncation output. The preview only covers the first + * PREVIEW_SIZE_CHARS chars and the envelope embeds a per-call unique file + * path, so consumers that fingerprint results (the loop guards in + * services/loopDetectionService.ts) read this digest instead of hashing the + * envelope: a board mutating beyond the preview window still fingerprints + * differently each poll, and a frozen board identically no matter where it + * was persisted (issue #9450). + */ +export const FULL_OUTPUT_DIGEST_LABEL = 'Full output sha256: '; + /** * Stable prefix every truncated tool output starts with. Used as an * idempotency sentinel so content that was already truncated (by a tool's own @@ -513,10 +525,16 @@ function buildStub( const preview = generatePreview(content); const sizeKb = Math.round(byteSize / 1024); const isFilePath = path.isAbsolute(filePathOrNote); + // sha256 of the FULL pre-truncation output (see FULL_OUTPUT_DIGEST_LABEL): + // the envelope's per-call unique path would otherwise fingerprint uniquely + // every poll, silently disabling every result-aware loop guard for exactly + // the largest results (issue #9450). + const fullDigest = crypto.createHash('sha256').update(content).digest('hex'); if (isFilePath) { return ` Output too large (${sizeKb} KB). Full output saved to: ${filePathOrNote} +${FULL_OUTPUT_DIGEST_LABEL}${fullDigest} Note: this file may be cleaned up after 24 hours. To read the complete output, use the ${ReadFileTool.Name} tool with the absolute file path above. @@ -526,6 +544,7 @@ ${preview} } return `Output too large (${sizeKb} KB). ${filePathOrNote} +${FULL_OUTPUT_DIGEST_LABEL}${fullDigest} Preview (up to ${PREVIEW_SIZE_CHARS} chars): ${preview}`;