loop detection in chat #3013
Draft
loop detection in chat #3013
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements loop detection for chat tool calling to prevent infinite loops, particularly for the Gemini model family. The feature detects two types of loops: repeated tool calls with the same arguments, and repeated text in model responses.
Changes:
- Added loop detection functions with configurable thresholds for tool call loops and text loops
- Integrated loop detection into the main tool calling loop and inline chat, only activating for Gemini family models
- Added telemetry tracking and new metadata fields to distinguish between loop-detected exits and iteration-limit exits
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/extension/prompt/common/toolCallRound.ts | Adds detectToolCallLoop and detectTextLoop functions with heuristic-based loop detection logic |
| src/extension/intents/node/toolCallingLoop.ts | Integrates loop detection into the main tool calling loop, adds telemetry events and handlers for detected loops |
| src/extension/inlineChat/node/inlineChatIntent.ts | Integrates tool call loop detection into inline chat with Gemini-specific handling |
| src/extension/prompt/common/conversation.ts | Extends IResultMetadata interface with new fields for tracking loop detection exit reasons |
| src/extension/prompt/node/chatParticipantTelemetry.ts | Updates telemetry signature to support new 'toolLoop' response type |
| src/extension/prompt/node/defaultIntentRequestHandler.ts | Updates telemetry handling to use the new toolCallExitReason metadata field |
Comments suppressed due to low confidence (1)
src/extension/intents/node/toolCallingLoop.ts:204
- This use of variable 'lastResult' always evaluates to true.
if (textLoopDetection && lastResult) {
|
|
||
| function splitSentences(text: string): string[] { | ||
| return text | ||
| .split(/[\.\!\?\n\r]+/g) |
There was a problem hiding this comment.
The regex pattern contains unescaped special characters in a character class. In regex character classes, the exclamation mark and question mark don't need escaping, but the period does. The current pattern /[\.\!\?\n\r]+/g should be /[.!?\n\r]+/g - the backslashes before ! and ? are unnecessary (though harmless), but the period is correctly escaped. However, for clarity and consistency with regex best practices, consider using /[.!?\n\r]+/g to remove unnecessary escapes.
Suggested change
| .split(/[\.\!\?\n\r]+/g) | |
| .split(/[.!?\n\r]+/g) |
Comment on lines
+66
to
+185
| export function detectToolCallLoop(toolCallRounds: readonly IToolCallRound[]): ToolCallLoopDetectionResult | undefined { | ||
| const allCalls: IToolCall[] = []; | ||
| for (const round of toolCallRounds) { | ||
| if (!round.toolCalls.length) { | ||
| continue; | ||
| } | ||
| for (const call of round.toolCalls) { | ||
| allCalls.push(call); | ||
| } | ||
| } | ||
|
|
||
| // Require a minimum number of calls overall before we even consider this a loop. | ||
| const minTotalCalls = 12; | ||
| if (allCalls.length < minTotalCalls) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Look at a sliding window of the most recent calls to see if | ||
| // the model is bouncing between the same one or two tool invocations. | ||
| const windowSize = 20; | ||
| const recent = allCalls.slice(-Math.min(windowSize, allCalls.length)); | ||
| if (recent.length < minTotalCalls) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const toolCountsWindow: Record<string, number> = Object.create(null); | ||
| for (const call of recent) { | ||
| const key = `${call.name}:${call.arguments}`; | ||
| toolCountsWindow[key] = (toolCountsWindow[key] || 0) + 1; | ||
| } | ||
|
|
||
| const keys = Object.keys(toolCountsWindow); | ||
| const uniqueToolKeyCount = keys.length; | ||
| if (uniqueToolKeyCount === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // We only consider it a loop if the recent window is dominated by | ||
| // one or two repeating tool+argument combinations. | ||
| const maxKeyCount = keys.reduce((max, key) => Math.max(max, toolCountsWindow[key]), 0); | ||
| const maxDistinctKeys = 2; | ||
| const minRepeatsForLoop = 6; | ||
| if (uniqueToolKeyCount <= maxDistinctKeys && maxKeyCount >= minRepeatsForLoop) { | ||
| return { | ||
| toolCountsWindow, | ||
| windowSize: recent.length, | ||
| uniqueToolKeyCount, | ||
| maxKeyCount, | ||
| totalToolCallRounds: toolCallRounds.length, | ||
| totalToolCalls: allCalls.length, | ||
| }; | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| export interface ITextLoopDetectionResult { | ||
| readonly repeatCount: number; | ||
| readonly totalSentences: number; | ||
| readonly totalRounds: number; | ||
| readonly responseLength: number; | ||
| } | ||
|
|
||
| function splitSentences(text: string): string[] { | ||
| return text | ||
| .split(/[\.\!\?\n\r]+/g) | ||
| .map(s => s.trim()) | ||
| .filter(s => s.length > 0); | ||
| } | ||
|
|
||
| function normalizeSentence(sentence: string): string { | ||
| return sentence | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, ' ') | ||
| .trim(); | ||
| } | ||
|
|
||
| export function detectTextLoop(toolCallRounds: readonly IToolCallRound[]): ITextLoopDetectionResult | undefined { | ||
| const lastRound = toolCallRounds.at(-1); | ||
| if (!lastRound) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const response = lastRound.response; | ||
| const minResponseLength = 200; | ||
| if (!response || response.length < minResponseLength) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const sentences = splitSentences(response); | ||
| if (sentences.length < 3) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const sentenceCounts: Record<string, number> = Object.create(null); | ||
| let maxCount = 0; | ||
| for (const sentence of sentences) { | ||
| const normalized = normalizeSentence(sentence); | ||
| if (normalized.length < 30) { | ||
| continue; | ||
| } | ||
| const count = (sentenceCounts[normalized] || 0) + 1; | ||
| sentenceCounts[normalized] = count; | ||
| if (count > maxCount) { | ||
| maxCount = count; | ||
| } | ||
| } | ||
|
|
||
| const minRepeatsForTextLoop = 3; | ||
| if (maxCount >= minRepeatsForTextLoop) { | ||
| return { | ||
| repeatCount: maxCount, | ||
| totalSentences: sentences.length, | ||
| totalRounds: toolCallRounds.length, | ||
| responseLength: response.length, | ||
| }; | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The new loop detection functions detectToolCallLoop and detectTextLoop contain complex heuristics with multiple thresholds and conditions, but lack test coverage. Consider adding unit tests to verify the loop detection logic works correctly across various scenarios, such as:
- Detecting loops with different numbers of repeated tool calls
- Not triggering false positives with legitimate tool call sequences
- Correctly identifying repeated sentences in text
- Handling edge cases like empty inputs, short sequences, etc.
|
|
||
| this.toolCallRounds.push(result.round); | ||
| const loopDetection = isGeminiFamily ? detectToolCallLoop(this.toolCallRounds) : undefined; | ||
| if (loopDetection && lastResult) { |
There was a problem hiding this comment.
This use of variable 'lastResult' always evaluates to true.
This issue also appears in the following locations of the same file:
- line 204
See below for a potential fix:
if (loopDetection) {
lastResult = this.hitToolCallLoop(outputStream, lastResult, loopDetection);
break;
}
const textLoopDetection = isGeminiFamily ? detectTextLoop(this.toolCallRounds) : undefined;
if (textLoopDetection) {
lastResult = this.hitTextLoop(outputStream, lastResult, textLoopDetection);
break;
}
if (!result.round.toolCalls.length || result.response.type !== ChatFetchResponseType.Success) {
break;
}
} catch (e) {
if (isCancellationError(e) && lastResult) {
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.