Skip to content

fix(core): honor skipLoopDetection for the deterministic tool-call loop #5128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 1 commit into from
Jun 15, 2026
Merged

fix(core): honor skipLoopDetection for the deterministic tool-call loop #5128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 45 additions & 5 deletions packages/core/src/core/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6070,7 +6070,7 @@ Other open files:
expect(mockCheckNextSpeaker).not.toHaveBeenCalled();
});

it('keeps deterministic tool-call checks when skipLoopDetection is true', async () => {
it('does not run loop checks when skipLoopDetection is true', async () => {
// Arrange
// Ensure config returns true for skipLoopDetection
vi.spyOn(client['config'], 'getSkipLoopDetection').mockReturnValue(true);
Expand Down Expand Up @@ -6106,13 +6106,14 @@ Other open files:
// consume stream
}

expect(ldMock.addAndCheckDeterministicToolCallLoop).toHaveBeenCalledTimes(
2,
);
// Assert - neither detector path runs when skipLoopDetection is true
expect(
ldMock.addAndCheckDeterministicToolCallLoop,
).not.toHaveBeenCalled();
expect(ldMock.addAndCheckHeuristicLoops).not.toHaveBeenCalled();
});

it('hard-stops identical tool calls even when skipLoopDetection is true', async () => {
it('does not hard-stop identical tool calls when skipLoopDetection is true', async () => {
vi.spyOn(client['config'], 'getSkipLoopDetection').mockReturnValue(true);

mockTurnRunFn.mockReturnValue(
Expand Down Expand Up @@ -6144,6 +6145,45 @@ Other open files:
),
);

// skipLoopDetection defaults to true, so even repeated identical calls
// must not be halted — the documented escape hatch stays effective.
expect(events.some((e) => e.type === GeminiEventType.LoopDetected)).toBe(
false,
);
});

it('hard-stops identical tool calls when loop detection is enabled', async () => {
vi.spyOn(client['config'], 'getSkipLoopDetection').mockReturnValue(false);

mockTurnRunFn.mockReturnValue(
(async function* () {
for (let i = 0; i < 5; i++) {
yield {
type: GeminiEventType.ToolCallRequest,
value: {
callId: `repeat-${i}`,
name: 'run_shell_command',
args: { command: 'echo repeated' },
},
};
}
})(),
);

const mockChat: Partial<GeminiChat> = {
addHistory: vi.fn(),
getHistory: vi.fn().mockReturnValue([]),
};
client['chat'] = mockChat as GeminiChat;

const events = await fromAsync(
client.sendMessageStream(
[{ text: 'repeat a tool' }],
new AbortController().signal,
'prompt-id-loop-identical',
),
);

expect(events.at(-1)).toEqual({
type: GeminiEventType.LoopDetected,
value: { loopType: LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS },
Expand Down
11 changes: 10 additions & 1 deletion packages/core/src/core/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2097,11 +2097,20 @@ export class GeminiClient {
didUpdateIdeContextState = true;
}

// Loop detection is opt-in: `model.skipLoopDetection` defaults to true
// (see settingsSchema) to avoid false-positive interruptions. Keep BOTH
// the deterministic identical-tool-call check and the heuristic checks
// behind this single flag so the documented `model.skipLoopDetection`

@wenshao wenshao Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This sentence claims "The deterministic split, retry-reset, and pending-call splice below still apply once detection is enabled." Of the three items, only the deterministic split and the pending-call splice are actually below this comment in client.ts. The retry-reset (resetToolCallCount()) lives inside addAndCheckDeterministicToolCallLoop in loopDetectionService.ts:184 — reachable through the call graph but not "below" in this file. Referencing unchanged code elsewhere with a positional pointer like "below" makes the comment more likely to become stale when someone refactors the service or reorders this block.

Consider dropping "retry-reset" from the list (it is an implementation detail of the deterministic path and already covered by "the deterministic split … still apply"), or rephrasing to avoid the positional claim, e.g.:

Suggested change
// behind this single flag so the documented `model.skipLoopDetection`
// Loop detection is opt-in: `model.skipLoopDetection` defaults to true
// (see settingsSchema) to avoid false-positive interruptions. Keep BOTH
// the deterministic identical-tool-call check and the heuristic checks
// behind this single flag so the documented `model.skipLoopDetection`
// escape hatch stays honest (including the non-interactive hint in
// nonInteractiveCli.ts). The deterministic split and the pending-call
// splice below still apply once detection is enabled.

— qwen3.7-max via Qwen Code /review

// escape hatch stays honest (including the non-interactive hint in
// nonInteractiveCli.ts). The deterministic split, retry-reset, and
// pending-call splice below still apply once detection is enabled.
const skipLoopDetection = this.config.getSkipLoopDetection();
const deterministicToolCallLoop =
!skipLoopDetection &&
this.loopDetector.addAndCheckDeterministicToolCallLoop(event);
const heuristicLoop =
!deterministicToolCallLoop &&
!this.config.getSkipLoopDetection() &&
!skipLoopDetection &&
this.loopDetector.addAndCheckHeuristicLoops(event);
if (deterministicToolCallLoop || heuristicLoop) {
const loopType = this.loopDetector.getLastLoopType();
Expand Down
Loading