mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
fix(core): honor skipLoopDetection for the deterministic tool-call loop (#5128)
#5036 carved the deterministic identical-tool-call check out of the `model.skipLoopDetection` gate, turning it into a hard-stop that fires even when loop detection is disabled. Because `skipLoopDetection` defaults to true (settingsSchema: "to avoid false-positive interruptions"), this silently re-enabled loop halts for the default configuration and broke the documented escape hatch — the non-interactive guidance in nonInteractiveCli.ts told users to set `model.skipLoopDetection: true`, which no longer disabled the halt and is unreachable in non-interactive mode (no disable dialog). Gate both the deterministic and heuristic detector paths behind the single flag again. The deterministic split, retry-reset, and pending tool-call splice introduced by #5036 still apply once detection is explicitly enabled (skipLoopDetection: false), so the runaway guard remains available as opt-in without overriding the default-off contract.
This commit is contained in:
1 parent
02bd82abfd
commit
a74f70b3d6
2 files changed
+55
-6
No files matched your search
@@ -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);
|
||||
@@ -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(
|
||||
@@ -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 },
|
||||
|
||||
@@ -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`
|
||||
// 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();
|
||||
|
||||
Reference in new issue
Block a user