diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index b8f8e04f482..dc90c77756b 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -20,6 +20,7 @@ import { promptIdContext, OutputFormat, InputFormat, + LoopType, uiTelemetryService, parseAndFormatApiError, createDebugLogger, @@ -51,6 +52,38 @@ import { computeUsageFromMetrics, } from './utils/nonInteractiveHelpers.js'; +// Human-readable labels for the detectors that can fire mid-stream. +// Surfaced to stderr in TEXT mode so a headless run that halts on a loop +// doesn't exit with empty stdout and no explanation — see PR #3236 review. +const LOOP_TYPE_LABELS: Record = { + [LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS]: + 'the model repeated the same tool call with identical arguments', + [LoopType.CHANTING_IDENTICAL_SENTENCES]: + 'the model repeated the same sentence in its output', + [LoopType.REPETITIVE_THOUGHTS]: + 'the model repeated the same reasoning thought', + [LoopType.READ_FILE_LOOP]: + 'the model spent too many consecutive calls reading files without making progress', + [LoopType.ACTION_STAGNATION]: + 'the model kept calling the same tool without making progress', +}; + +function emitLoopDetectedMessage( + config: Config, + loopType: LoopType | undefined, +): void { + // In TEXT mode the adapter swallows LoopDetected, so we print here. In + // JSON modes the adapter emits a structured result, which is enough. + if (config.getOutputFormat() !== OutputFormat.TEXT) { + return; + } + const reason = loopType ? LOOP_TYPE_LABELS[loopType] : undefined; + const detail = reason ? ` (${loopType}: ${reason})` : ''; + process.stderr.write( + `Loop detection halted the run${detail}. Set the \`model.skipLoopDetection\` setting to true to disable.\n`, + ); +} + /** * Emits a final message for slash command results. * Note: systemMessage should already be emitted before calling this function. @@ -340,6 +373,9 @@ export async function runNonInteractive( if (event.type === GeminiEventType.ToolCallRequest) { toolCallRequests.push(event.value); } + if (event.type === GeminiEventType.LoopDetected) { + emitLoopDetectedMessage(config, event.value?.loopType); + } if ( outputFormat === OutputFormat.TEXT && event.type === GeminiEventType.Error @@ -506,6 +542,9 @@ export async function runNonInteractive( if (event.type === GeminiEventType.ToolCallRequest) { itemToolCallRequests.push(event.value); } + if (event.type === GeminiEventType.LoopDetected) { + emitLoopDetectedMessage(config, event.value?.loopType); + } if ( outputFormat === OutputFormat.TEXT && event.type === GeminiEventType.Error diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index e3a93e7847e..c58b3db731b 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -883,7 +883,11 @@ export class GeminiClient { for await (const event of resultStream) { if (!this.config.getSkipLoopDetection()) { if (this.loopDetector.addAndCheck(event)) { - yield { type: GeminiEventType.LoopDetected }; + const loopType = this.loopDetector.getLastLoopType(); + yield { + type: GeminiEventType.LoopDetected, + ...(loopType && { value: { loopType } }), + }; if (arenaAgentClient) { await arenaAgentClient.reportError('Loop detected'); } diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index f7be11c5a0b..1c2f9fa2a10 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -4110,4 +4110,150 @@ describe('CoreToolScheduler validation retry loop detection', () => { expect(msg).toBeDefined(); expect(msg).not.toContain(RETRY_LOOP_STOP_DIRECTIVE); }); + + it('should isolate retry counters per-tool across batches', async () => { + // Regression: the batch-level continues-loop check used to keep *all* + // retry state whenever any current request matched a previously failing + // tool. That let stale counts for an unrelated tool survive long enough + // to fire RETRY LOOP DETECTED prematurely the next time that tool was + // called. The correct behaviour prunes counters per-tool: keep only + // counters whose tool name actually appears in the current batch. + class StrictToolAlt extends BaseDeclarativeTool< + { other: string }, + ToolResult + > { + static readonly Name = 'strictStringToolAlt'; + constructor() { + super( + StrictToolAlt.Name, + 'StrictStringToolAlt', + 'Alt tool requiring string other param.', + Kind.Other, + { + type: 'object', + properties: { other: { type: 'string' } }, + required: ['other'], + }, + ); + } + protected createInvocation(params: { + other: string; + }): ToolInvocation<{ other: string }, ToolResult> { + return new (class extends BaseToolInvocation< + { other: string }, + ToolResult + > { + constructor(p: { other: string }) { + super(p); + } + getDescription() { + return 'strictStringToolAlt invocation'; + } + async execute(): Promise { + return { llmContent: 'ok', returnDisplay: 'ok' }; + } + })(params); + } + } + + const toolA = new StrictStringTool(); + const toolB = new StrictToolAlt(); + const mockToolRegistry = { + ensureTool: async (name: string) => + name === StrictStringTool.Name + ? toolA + : name === StrictToolAlt.Name + ? toolB + : undefined, + getTool: (name: string) => + name === StrictStringTool.Name + ? toolA + : name === StrictToolAlt.Name + ? toolB + : undefined, + getFunctionDeclarations: () => [], + tools: new Map(), + discovery: {}, + registerTool: () => {}, + getToolByName: (name: string) => + name === StrictStringTool.Name + ? toolA + : name === StrictToolAlt.Name + ? toolB + : undefined, + getToolByDisplayName: () => undefined, + getTools: () => [], + discoverTools: async () => {}, + getAllTools: () => [], + getAllToolNames: () => [StrictStringTool.Name, StrictToolAlt.Name], + getToolsByServer: () => [], + } as unknown as ToolRegistry; + + const mockConfig = { + getSessionId: () => 'test-session-id', + getUsageStatisticsEnabled: () => true, + getDebugMode: () => false, + getApprovalMode: () => ApprovalMode.YOLO, + getPermissionsAllow: () => [], + getContentGeneratorConfig: () => ({ + model: 'test-model', + authType: 'gemini', + }), + getShellExecutionConfig: () => ({ + terminalWidth: 90, + terminalHeight: 30, + }), + storage: { getProjectTempDir: () => '/tmp' }, + getTruncateToolOutputThreshold: () => 100, + getTruncateToolOutputLines: () => 10, + getToolRegistry: () => mockToolRegistry, + getUseModelRouter: () => false, + getGeminiClient: () => null, + isInteractive: () => true, + getIdeMode: () => false, + getExperimentalZedIntegration: () => false, + getChatRecordingService: () => undefined, + getMessageBus: vi.fn().mockReturnValue(undefined), + getDisableAllHooks: vi.fn().mockReturnValue(true), + setApprovalMode: vi.fn(), + } as unknown as Config; + + const onToolCallsUpdate = vi.fn(); + const scheduler = new CoreToolScheduler({ + config: mockConfig, + onAllToolCallsComplete: vi.fn(), + onToolCallsUpdate, + getPreferredEditor: () => 'vscode', + onEditorClose: vi.fn(), + }); + + // Tool A fails twice, accumulating a retry count of 2. + await scheduler.schedule( + [makeRequest('a1', StrictStringTool.Name, { value: 123 })], + new AbortController().signal, + ); + await scheduler.schedule( + [makeRequest('a2', StrictStringTool.Name, { value: 123 })], + new AbortController().signal, + ); + + // Now a batch for tool B only — tool A's counter must be pruned because + // A is not present in this batch. + await scheduler.schedule( + [makeRequest('b1', StrictToolAlt.Name, { other: 456 })], + new AbortController().signal, + ); + + // Tool A fails once more. Under the old wholesale-keep behaviour this + // would be the third consecutive A failure and would trip the directive. + // Under per-tool pruning the counter starts fresh at 1 and no directive + // should be emitted. + await scheduler.schedule( + [makeRequest('a3', StrictStringTool.Name, { value: 123 })], + new AbortController().signal, + ); + const msg = getLastErrorMessage(onToolCallsUpdate); + expect(msg).toBeDefined(); + expect(msg).not.toContain(RETRY_LOOP_STOP_DIRECTIVE); + }); }); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 99944fd5d76..7b78ee351b9 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -794,20 +794,20 @@ export class CoreToolScheduler { } const requestsToProcess = Array.isArray(request) ? request : [request]; - // Check if this batch continues a validation retry loop. - // Keys are ":"; if no request reuses a tool name - // that previously failed validation, reset the tracker. + // Prune validation retry state per-tool, not wholesale. Keys are + // ":"; retain counters only for tools actually + // present in the current batch. Keeping every tracked tool's counters + // whenever any current request matched caused stale counts for + // unrelated tools to survive and fire RETRY LOOP DETECTED prematurely + // the next time those tools were used. if (this.validationRetryCounts.size > 0) { - const prevTools = new Set(); - for (const key of this.validationRetryCounts.keys()) { + const currentToolNames = new Set(requestsToProcess.map((r) => r.name)); + for (const key of [...this.validationRetryCounts.keys()]) { const sep = key.indexOf(':'); - prevTools.add(sep === -1 ? key : key.slice(0, sep)); - } - const hasPrevFailingTool = requestsToProcess.some((r) => - prevTools.has(r.name), - ); - if (!hasPrevFailingTool) { - this.validationRetryCounts.clear(); + const toolName = sep === -1 ? key : key.slice(0, sep); + if (!currentToolNames.has(toolName)) { + this.validationRetryCounts.delete(key); + } } } diff --git a/packages/core/src/core/turn.ts b/packages/core/src/core/turn.ts index 5e918c72cb2..78e8dac24f6 100644 --- a/packages/core/src/core/turn.ts +++ b/packages/core/src/core/turn.ts @@ -33,6 +33,7 @@ import { parseThought, type ThoughtSummary, } from '../utils/thoughtUtils.js'; +import type { LoopType } from '../telemetry/types.js'; // Define a structure for tools passed to the server export interface ServerTool { @@ -194,6 +195,12 @@ export type ServerGeminiFinishedEvent = { export type ServerGeminiLoopDetectedEvent = { type: GeminiEventType.LoopDetected; + // The loop type is optional so historical call sites that don't produce one + // (tests, fixtures) stay valid. Real emissions in client.ts always populate + // it so downstream consumers can surface a concrete reason to the user. + value?: { + loopType: LoopType; + }; }; export type ServerGeminiCitationEvent = { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b4eead082d3..5d3b632476c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -227,6 +227,7 @@ export { ExtensionUninstallEvent, IdeConnectionEvent, IdeConnectionType, + LoopType, ModelSlashCommandEvent, PromptSuggestionEvent, SpeculationEvent, diff --git a/packages/core/src/services/loopDetectionService.test.ts b/packages/core/src/services/loopDetectionService.test.ts index 31a8699dc4b..2d0ba5e3d67 100644 --- a/packages/core/src/services/loopDetectionService.test.ts +++ b/packages/core/src/services/loopDetectionService.test.ts @@ -9,6 +9,7 @@ import type { Config } from '../config/config.js'; import type { ServerGeminiContentEvent, ServerGeminiStreamEvent, + ServerGeminiThoughtEvent, ServerGeminiToolCallRequestEvent, } from '../core/turn.js'; import { GeminiEventType } from '../core/turn.js'; @@ -23,6 +24,9 @@ vi.mock('../telemetry/loggers.js', () => ({ const TOOL_CALL_LOOP_THRESHOLD = 5; const CONTENT_LOOP_THRESHOLD = 10; const CONTENT_CHUNK_SIZE = 50; +// Mirrored from loopDetectionService.ts. Kept local so the test is +// self-describing and failures point to the constant that changed. +const FILE_READ_WINDOW = 15; describe('LoopDetectionService', () => { let service: LoopDetectionService; @@ -55,6 +59,14 @@ describe('LoopDetectionService', () => { value: content, }); + const createThoughtEvent = ( + subject: string, + description = '', + ): ServerGeminiThoughtEvent => ({ + type: GeminiEventType.Thought, + value: { subject, description }, + }); + const createRepetitiveContent = (id: number, length: number): string => { const baseString = `This is a unique sentence, id=${id}. `; let content = ''; @@ -114,7 +126,7 @@ describe('LoopDetectionService', () => { param: 'value', }); const otherEvent = { - type: 'thought', + type: GeminiEventType.UserCancelled, } as unknown as ServerGeminiStreamEvent; // Send events just below the threshold @@ -619,4 +631,361 @@ describe('LoopDetectionService', () => { expect(service.addAndCheck(otherEvent)).toBe(false); }); }); + + describe('Repetitive Thoughts Detection', () => { + it('should detect repetitive thoughts pattern', () => { + service.reset(''); + + for (let i = 0; i < 3; i++) { + service.addAndCheck( + createThoughtEvent('Plan', 'Inspect the migration script.'), + ); + } + + expect(loggers.logLoopDetected).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: 'repetitive_thoughts', + }), + ); + }); + + it('should not detect loop with varied thoughts', () => { + service.reset(''); + + service.addAndCheck(createThoughtEvent('Plan', 'Inspect the schema.')); + service.addAndCheck( + createThoughtEvent('Analysis', 'Check migration risks.'), + ); + service.addAndCheck( + createThoughtEvent('Plan', 'Evaluate rollout alternatives.'), + ); + + const isLoop = service.addAndCheck( + createThoughtEvent('Next', 'Draft the fix.'), + ); + expect(isLoop).toBe(false); + expect(loggers.logLoopDetected).not.toHaveBeenCalled(); + }); + + it('should not detect a loop when an earlier thought reappears after progress', () => { + service.reset(''); + + // Regression: earlier counting-based implementation fired as soon as + // any thought appeared >= THRESHOLD times anywhere in the retained + // history. A healthy long-running session where the model revisits + // the same phrase after making progress on unrelated steps should + // *not* trip this detector — only a sustained consecutive run does. + service.addAndCheck(createThoughtEvent('Plan', 'Inspect the schema.')); + service.addAndCheck( + createThoughtEvent('Analysis', 'Consider migration.'), + ); + service.addAndCheck(createThoughtEvent('Analysis', 'Review indexes.')); + service.addAndCheck(createThoughtEvent('Plan', 'Inspect the schema.')); + service.addAndCheck( + createThoughtEvent('Analysis', 'Consider rollout risks.'), + ); + const isLoop = service.addAndCheck( + createThoughtEvent('Plan', 'Inspect the schema.'), + ); + expect(isLoop).toBe(false); + expect(loggers.logLoopDetected).not.toHaveBeenCalled(); + }); + + it('clears thought history across tool-call roundtrips within a turn', () => { + service.reset(''); + + // Regression: thoughtHistory previously persisted across ToolCallRequest + // events within a single prompt. Three identical thoughts separated by + // real tool-call progress would incorrectly fire REPETITIVE_THOUGHTS. + service.addAndCheck(createThoughtEvent('Plan', 'Inspect the schema.')); + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'a.sql' }), + ); + service.addAndCheck(createThoughtEvent('Plan', 'Inspect the schema.')); + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'b.sql' }), + ); + const isLoop = service.addAndCheck( + createThoughtEvent('Plan', 'Inspect the schema.'), + ); + expect(isLoop).toBe(false); + expect(loggers.logLoopDetected).not.toHaveBeenCalled(); + }); + + it('ignores hedge phrases in Content events (thought detection is Thought-only)', () => { + service.reset(''); + + // Content events used to feed a substring-matched hedge-phrase list + // into thoughtHistory, which conflated prose with the model's actual + // reasoning channel. Thought detection now runs only on Thought events. + for (let i = 0; i < 5; i++) { + service.addAndCheck( + createContentEvent('I should check the config, maybe it helps.'), + ); + } + expect(loggers.logLoopDetected).not.toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ loop_type: 'repetitive_thoughts' }), + ); + }); + }); + + describe('Read File Loop Detection', () => { + // Cold-start exemption: a prompt that has not yet fired any non-read-like + // tool is still in its opening-exploration phase, so the detector gives + // it an initial pass. Tests that want to exercise the detector must + // fire a non-read tool first so subsequent reads are judged normally. + const primeNonReadTool = () => { + service.addAndCheck( + createToolCallRequestEvent('write_file', { + path: 'prime.txt', + content: '', + }), + ); + }; + + it('should detect excessive file read operations', () => { + service.reset(''); + primeNonReadTool(); + + // FILE_READ_THRESHOLD reads in the window trigger the loop. The first + // (THRESHOLD - 1) reads must not fire; the THRESHOLD-th does. + for (let i = 0; i < 7; i++) { + const event = createToolCallRequestEvent('read_file', { + path: `file${i}.txt`, + }); + const isLoop = service.addAndCheck(event); + expect(isLoop).toBe(false); + } + + const event = createToolCallRequestEvent('read_file', { + path: 'file7.txt', + }); + const isLoop = service.addAndCheck(event); + expect(isLoop).toBe(true); + expect(loggers.logLoopDetected).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: 'read_file_loop', + }), + ); + }); + + it('should exempt opening exploration from READ_FILE_LOOP (cold start)', () => { + service.reset(''); + + // Regression for PR #3236 review: a prompt like "summarize this + // project" opens with parallel read_file / list_directory calls and + // must not trip READ_FILE_LOOP before any write/execute action has + // fired. This exercises FILE_READ_WINDOW+ consecutive reads with no + // prior non-read tool — nothing should fire. + for (let i = 0; i < 20; i++) { + const name = i % 2 === 0 ? 'read_file' : 'list_directory'; + const isLoop = service.addAndCheck( + createToolCallRequestEvent(name, { path: `f${i}` }), + ); + expect(isLoop).toBe(false); + } + expect(loggers.logLoopDetected).not.toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ loop_type: 'read_file_loop' }), + ); + }); + + it('should activate READ_FILE_LOOP once a non-read tool lands mid-prompt', () => { + service.reset(''); + + // No firing before the cold-start gate flips. + for (let i = 0; i < 7; i++) { + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: `pre${i}.txt` }), + ); + } + expect(loggers.logLoopDetected).not.toHaveBeenCalled(); + + // A non-read tool lands — gate opens. + service.addAndCheck( + createToolCallRequestEvent('write_file', { + path: 'out.txt', + content: 'x', + }), + ); + + // Now a window of reads should eventually trip READ_FILE_LOOP. As new + // reads push the write_file out of the FILE_READ_WINDOW-sized history + // and FILE_READ_THRESHOLD read-likes accumulate, detection fires. + let detected = false; + for (let i = 0; i < FILE_READ_WINDOW + 2 && !detected; i++) { + detected = service.addAndCheck( + createToolCallRequestEvent('read_file', { path: `post${i}.txt` }), + ); + } + expect(detected).toBe(true); + expect(loggers.logLoopDetected).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ loop_type: 'read_file_loop' }), + ); + }); + + it('should detect other read-like operations (exact names + read_/list_ prefixes)', () => { + service.reset(''); + primeNonReadTool(); + + // Mix of read-like tool names that either appear in the exact allowlist + // (read_file, read_many_files, list_directory) or match the read_/list_ + // prefix fallback used for MCP-provided tools. + service.addAndCheck( + createToolCallRequestEvent('read_many_files', { + paths: ['file1.txt'], + }), + ); + service.addAndCheck( + createToolCallRequestEvent('list_directory', { path: '.' }), + ); + service.addAndCheck( + createToolCallRequestEvent('read_resource', { uri: 'a' }), + ); + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'file3.txt' }), + ); + service.addAndCheck(createToolCallRequestEvent('list_projects', {})); + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'file5.txt' }), + ); + service.addAndCheck( + createToolCallRequestEvent('read_many_files', { + paths: ['file6.txt'], + }), + ); + + const isLoop = service.addAndCheck( + createToolCallRequestEvent('list_directory', { path: 'nested' }), + ); + expect(isLoop).toBe(true); + expect(loggers.logLoopDetected).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: 'read_file_loop', + }), + ); + }); + + it('should not treat tools that merely contain read-like substrings as file reads', () => { + service.reset(''); + primeNonReadTool(); + + // Regression: the earlier substring heuristic treated any name + // containing 'read'/'cat'/'view'/'list' as a file read, so `review` + // (contains 'view') and `concat_chunks` (contains 'cat') contributed + // to READ_FILE_LOOP even though no file-read loop was happening. + const nonReadLikeNames = [ + 'review', + 'concat_chunks', + 'viewport_set', + 'listener_bind', + ]; + for (let i = 0; i < 6; i++) { + const name = nonReadLikeNames[i % nonReadLikeNames.length]; + const isLoop = service.addAndCheck( + createToolCallRequestEvent(name, { i }), + ); + expect(isLoop).toBe(false); + } + expect(loggers.logLoopDetected).not.toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ loop_type: 'read_file_loop' }), + ); + }); + + it('should not detect loop with mixed operations', () => { + service.reset(''); + primeNonReadTool(); + + // Mix of read and non-read operations + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'file1.txt' }), + ); + service.addAndCheck( + createToolCallRequestEvent('write_file', { + path: 'file2.txt', + content: 'test', + }), + ); + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'file3.txt' }), + ); + service.addAndCheck( + createToolCallRequestEvent('execute', { command: 'ls' }), + ); + service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'file4.txt' }), + ); + + const isLoop = service.addAndCheck( + createToolCallRequestEvent('read_file', { path: 'file5.txt' }), + ); + expect(isLoop).toBe(false); + expect(loggers.logLoopDetected).not.toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ loop_type: 'read_file_loop' }), + ); + }); + }); + + describe('Action Stagnation Detection', () => { + // Stagnation fires when the same tool *name* is called STAGNATION_THRESHOLD + // times consecutively regardless of arguments. This is distinct from + // CONSECUTIVE_IDENTICAL_TOOL_CALLS (same name AND args) and from + // READ_FILE_LOOP (high proportion of read-like tools in the window), + // so we exercise it with a non-read-like tool and varying args. + it('should detect action stagnation when the same tool is repeated with varying args', () => { + service.reset(''); + + // STAGNATION_THRESHOLD - 1 calls must not fire + for (let i = 0; i < 7; i++) { + const isLoop = service.addAndCheck( + createToolCallRequestEvent('search_code', { query: `term${i}` }), + ); + expect(isLoop).toBe(false); + } + + // THRESHOLD-th consecutive same-name call triggers stagnation + const isLoop = service.addAndCheck( + createToolCallRequestEvent('search_code', { query: 'term7' }), + ); + expect(isLoop).toBe(true); + expect(loggers.logLoopDetected).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ loop_type: 'action_stagnation' }), + ); + }); + + it('should reset stagnation streak when a different tool is called', () => { + service.reset(''); + + // Accumulate 5 consecutive same-name calls (below threshold) + for (let i = 0; i < 5; i++) { + service.addAndCheck( + createToolCallRequestEvent('search_code', { query: `a${i}` }), + ); + } + + // A different tool resets the streak + service.addAndCheck( + createToolCallRequestEvent('write_file', { + path: 'out.txt', + content: 'x', + }), + ); + + // 5 more calls of the original tool: streak only reaches 5, below threshold + for (let i = 0; i < 5; i++) { + const isLoop = service.addAndCheck( + createToolCallRequestEvent('search_code', { query: `b${i}` }), + ); + expect(isLoop).toBe(false); + } + }); + }); }); diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index d14e4223e85..ed0006733b0 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -7,6 +7,7 @@ import { createHash } from 'node:crypto'; import type { ServerGeminiStreamEvent } from '../core/turn.js'; import { GeminiEventType } from '../core/turn.js'; +import type { ThoughtSummary } from '../utils/thoughtUtils.js'; import { logLoopDetected, logLoopDetectionDisabled, @@ -23,6 +24,26 @@ const CONTENT_LOOP_THRESHOLD = 10; const CONTENT_CHUNK_SIZE = 50; const MAX_HISTORY_LENGTH = 1000; +// Thought tracking +const THOUGHT_REPEAT_THRESHOLD = 3; +const MAX_THOUGHT_HISTORY = 50; + +// File read tracking. +// +// Thresholds were raised from 5/10 because a prompt like "summarize this +// project" legitimately opens with `list_directory` + several parallel +// `read_file` calls in a single turn, which previously tripped the detector +// on its first productive move. 8/15 leaves enough headroom for that shape +// while still catching pathological read-only churn. Combined with the +// cold-start exemption below (see `hasSeenNonReadTool`), a turn that has +// only ever performed read-like actions is treated as exploration, not a +// loop — once any non-read tool lands, the detector activates. +const FILE_READ_THRESHOLD = 8; +const FILE_READ_WINDOW = 15; + +// Action stagnation tracking +const STAGNATION_THRESHOLD = 8; + /** * Service for detecting and preventing infinite loops in AI responses. * Monitors tool call repetitions and content sentence repetitions. @@ -45,10 +66,42 @@ export class LoopDetectionService { // Session-level disable flag private disabledForSession = false; + // Thought tracking + private thoughtHistory: string[] = []; + + // Tool call tracking (for read-file loop + stagnation detection) + private recentToolCalls: Array<{ name: string; args: object }> = []; + + // Action stagnation tracking: consecutive calls to the same tool *name* + // (regardless of args). Distinct from checkToolCallLoop, which requires + // identical name AND args. This catches parameter-thrashing loops where + // the model keeps calling one tool with varying arguments. + private sameNameStreak = 0; + private lastSeenToolName: string | null = null; + + // Cold-start gate for READ_FILE_LOOP: the opening exploration of a prompt + // is almost always read-heavy (list + parallel reads). Until at least one + // non-read-like tool fires, a window full of reads is treated as legitimate + // exploration rather than loop evidence. Resets per-prompt in reset(). + private hasSeenNonReadTool = false; + + // 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. + private lastLoopType: LoopType | null = null; + constructor(config: Config) { this.config = config; } + /** + * Returns the LoopType of the most recent detection, or null if no loop + * has been detected in the current prompt. + */ + getLastLoopType(): LoopType | null { + return this.lastLoopType; + } + /** * Disables loop detection for the current session. */ @@ -77,15 +130,32 @@ export class LoopDetectionService { } switch (event.type) { - case GeminiEventType.ToolCallRequest: + case GeminiEventType.ToolCallRequest: { // content chanting only happens in one single stream, reset if there // is a tool call in between this.resetContentTracking(); - this.loopDetected = this.checkToolCallLoop(event.value); + // Thought repetition is only meaningful within a single contiguous + // reasoning stream. Once a tool call lands, the model has made + // observable progress — any prior thoughts should not carry over. + this.thoughtHistory = []; + + const toolCallLoop = this.checkToolCallLoop(event.value); + this.trackToolCall(event.value); + const readFileLoop = this.checkReadFileLoop(); + const actionStagnation = this.checkActionStagnation(); + + this.loopDetected = toolCallLoop || readFileLoop || actionStagnation; break; - case GeminiEventType.Content: + } + case GeminiEventType.Content: { this.loopDetected = this.checkContentLoop(event.value); break; + } + case GeminiEventType.Thought: { + this.trackThought(event.value); + this.loopDetected = this.checkRepetitiveThoughts(); + break; + } default: break; } @@ -101,6 +171,7 @@ export class LoopDetectionService { this.toolCallRepetitionCount = 1; } if (this.toolCallRepetitionCount >= TOOL_CALL_LOOP_THRESHOLD) { + this.lastLoopType = LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS; logLoopDetected( this.config, new LoopDetectedEvent( @@ -214,6 +285,7 @@ export class LoopDetectionService { const chunkHash = createHash('sha256').update(currentChunk).digest('hex'); if (this.isLoopDetectedForChunk(currentChunk, chunkHash)) { + this.lastLoopType = LoopType.CHANTING_IDENTICAL_SENTENCES; logLoopDetected( this.config, new LoopDetectedEvent( @@ -291,6 +363,157 @@ export class LoopDetectionService { return originalChunk === currentChunk; } + /** + * Records a structured thought summary for repetition detection. Uses both + * subject and description so two thoughts with the same subject but + * diverging descriptions are correctly treated as distinct progress. + */ + private trackThought(summary: ThoughtSummary): void { + const subject = summary.subject.trim().toLowerCase(); + const description = summary.description + .trim() + .toLowerCase() + .substring(0, 200); + const signature = `${subject}|${description}`; + this.thoughtHistory.push(signature); + if (this.thoughtHistory.length > MAX_THOUGHT_HISTORY) { + this.thoughtHistory.shift(); + } + } + + /** + * Checks for repetitive thoughts pattern. + * + * Only fires when the last `THOUGHT_REPEAT_THRESHOLD` thoughts are the same + * string. Earlier implementations counted repeats across the full retained + * history, which caused false positives whenever the model revisited an + * earlier phrase after making progress on an unrelated step. + */ + private checkRepetitiveThoughts(): boolean { + if (this.thoughtHistory.length < THOUGHT_REPEAT_THRESHOLD) { + return false; + } + + const recentThoughts = this.thoughtHistory.slice(-THOUGHT_REPEAT_THRESHOLD); + const firstThought = recentThoughts[0]; + if (recentThoughts.every((thought) => thought === firstThought)) { + this.lastLoopType = LoopType.REPETITIVE_THOUGHTS; + logLoopDetected( + this.config, + new LoopDetectedEvent(LoopType.REPETITIVE_THOUGHTS, this.promptId), + ); + return true; + } + return false; + } + + // Exact tool names that read content from the filesystem. A plain substring + // match on tokens like "view" or "list" is unsafe because unrelated tools + // (e.g. "review", "checklist_update") can incidentally contain those + // tokens and get miscounted as file reads. + private static readonly READ_LIKE_TOOL_NAMES: ReadonlySet = new Set([ + 'read_file', + 'read_many_files', + 'list_directory', + ]); + + // Prefix fallback for MCP-provided tools that follow the same naming + // convention (e.g. `read_resource`, `list_projects`). The trailing + // underscore anchors the match to a name segment so "review" and + // "listener" are not treated as read-like. + private static readonly READ_LIKE_NAME_PREFIXES: readonly string[] = [ + 'read_', + 'list_', + ]; + + private isReadLikeTool(toolName: string): boolean { + if (LoopDetectionService.READ_LIKE_TOOL_NAMES.has(toolName)) { + return true; + } + return LoopDetectionService.READ_LIKE_NAME_PREFIXES.some((prefix) => + toolName.startsWith(prefix), + ); + } + + /** + * Tracks tool calls for subsequent loop detection. + */ + private trackToolCall(toolCall: { name: string; args: object }): void { + // Add to recent tool calls history + this.recentToolCalls.push(toolCall); + + // Keep bounded history + if (this.recentToolCalls.length > FILE_READ_WINDOW) { + this.recentToolCalls.shift(); + } + + // Flip the cold-start gate once any non-read-like tool has been observed. + // Opening exploration (list_directory + several read_file calls) should + // not count as loop evidence on its own. + if (!this.hasSeenNonReadTool && !this.isReadLikeTool(toolCall.name)) { + this.hasSeenNonReadTool = true; + } + + // Track same-name streak for action stagnation. Distinct from + // checkToolCallLoop which requires identical args; this detector catches + // "thrashing" where the same tool is called with varying arguments. + if (this.lastSeenToolName === toolCall.name) { + this.sameNameStreak++; + } else { + this.lastSeenToolName = toolCall.name; + this.sameNameStreak = 1; + } + } + + /** + * Checks for excessive file read operations without meaningful progress. + */ + private checkReadFileLoop(): boolean { + // Cold-start exemption: if no non-read-like tool has ever fired in this + // prompt, the model is still in its opening exploration phase. Treat a + // run of reads as legitimate discovery rather than a loop. Once any + // write/execute/other tool lands, normal detection resumes. + if (!this.hasSeenNonReadTool) { + return false; + } + + if (this.recentToolCalls.length < FILE_READ_THRESHOLD) { + return false; + } + + // Count how many of the recent tool calls were file reads + const fileReadCount = this.recentToolCalls.filter((call) => + this.isReadLikeTool(call.name), + ).length; + + if (fileReadCount >= FILE_READ_THRESHOLD) { + this.lastLoopType = LoopType.READ_FILE_LOOP; + logLoopDetected( + this.config, + new LoopDetectedEvent(LoopType.READ_FILE_LOOP, this.promptId), + ); + return true; + } + + return false; + } + + /** + * Checks for action stagnation where the model performs different but equally unproductive actions. + */ + private checkActionStagnation(): boolean { + if (this.sameNameStreak >= STAGNATION_THRESHOLD) { + this.lastLoopType = LoopType.ACTION_STAGNATION; + logLoopDetected( + this.config, + new LoopDetectedEvent(LoopType.ACTION_STAGNATION, this.promptId), + ); + return true; + } + + return false; + } + /** * Resets all loop detection state. */ @@ -299,6 +522,14 @@ export class LoopDetectionService { this.resetToolCallCount(); this.resetContentTracking(); this.loopDetected = false; + + // Reset new tracking variables + this.thoughtHistory = []; + this.recentToolCalls = []; + this.sameNameStreak = 0; + this.lastSeenToolName = null; + this.hasSeenNonReadTool = false; + this.lastLoopType = null; } private resetToolCallCount(): void { diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index 9cef68ce49a..c3666ae9a94 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -383,6 +383,9 @@ export class RipgrepFallbackEvent implements BaseTelemetryEvent { export enum LoopType { CONSECUTIVE_IDENTICAL_TOOL_CALLS = 'consecutive_identical_tool_calls', CHANTING_IDENTICAL_SENTENCES = 'chanting_identical_sentences', + REPETITIVE_THOUGHTS = 'repetitive_thoughts', + READ_FILE_LOOP = 'read_file_loop', + ACTION_STAGNATION = 'action_stagnation', } export class LoopDetectedEvent implements BaseTelemetryEvent {