feat(ui): Add confirmation dialog for disabling loop detection for current session (#8231)

This commit is contained in:
Sandy Tao
2025-09-10 22:20:13 -07:00
committed by GitHub
parent 5b2176770e
commit 78744cfbca
12 changed files with 498 additions and 39 deletions
+17 -29
View File
@@ -505,6 +505,7 @@ Logging in with Google... Please restart Gemini CLI to continue.
pendingHistoryItems: pendingGeminiHistoryItems, pendingHistoryItems: pendingGeminiHistoryItems,
thought, thought,
cancelOngoingRequest, cancelOngoingRequest,
loopDetectionConfirmationRequest,
} = useGeminiStream( } = useGeminiStream(
config.getGeminiClient(), config.getGeminiClient(),
historyManager.history, historyManager.history,
@@ -896,35 +897,20 @@ Logging in with Google... Please restart Gemini CLI to continue.
const nightly = props.version.includes('nightly'); const nightly = props.version.includes('nightly');
const dialogsVisible = useMemo( const dialogsVisible =
() => showWorkspaceMigrationDialog ||
showWorkspaceMigrationDialog || shouldShowIdePrompt ||
shouldShowIdePrompt || isFolderTrustDialogOpen ||
isFolderTrustDialogOpen || !!shellConfirmationRequest ||
!!shellConfirmationRequest || !!confirmationRequest ||
!!confirmationRequest || !!loopDetectionConfirmationRequest ||
isThemeDialogOpen || isThemeDialogOpen ||
isSettingsDialogOpen || isSettingsDialogOpen ||
isAuthenticating || isAuthenticating ||
isAuthDialogOpen || isAuthDialogOpen ||
isEditorDialogOpen || isEditorDialogOpen ||
showPrivacyNotice || showPrivacyNotice ||
!!proQuotaRequest, !!proQuotaRequest;
[
showWorkspaceMigrationDialog,
shouldShowIdePrompt,
isFolderTrustDialogOpen,
shellConfirmationRequest,
confirmationRequest,
isThemeDialogOpen,
isSettingsDialogOpen,
isAuthenticating,
isAuthDialogOpen,
isEditorDialogOpen,
showPrivacyNotice,
proQuotaRequest,
],
);
const pendingHistoryItems = useMemo( const pendingHistoryItems = useMemo(
() => [...pendingSlashCommandHistoryItems, ...pendingGeminiHistoryItems], () => [...pendingSlashCommandHistoryItems, ...pendingGeminiHistoryItems],
@@ -952,6 +938,7 @@ Logging in with Google... Please restart Gemini CLI to continue.
commandContext, commandContext,
shellConfirmationRequest, shellConfirmationRequest,
confirmationRequest, confirmationRequest,
loopDetectionConfirmationRequest,
geminiMdFileCount, geminiMdFileCount,
streamingState, streamingState,
initError, initError,
@@ -1024,6 +1011,7 @@ Logging in with Google... Please restart Gemini CLI to continue.
commandContext, commandContext,
shellConfirmationRequest, shellConfirmationRequest,
confirmationRequest, confirmationRequest,
loopDetectionConfirmationRequest,
geminiMdFileCount, geminiMdFileCount,
streamingState, streamingState,
initError, initError,
@@ -6,6 +6,7 @@
import { Box, Text } from 'ink'; import { Box, Text } from 'ink';
import { IdeIntegrationNudge } from '../IdeIntegrationNudge.js'; import { IdeIntegrationNudge } from '../IdeIntegrationNudge.js';
import { LoopDetectionConfirmation } from './LoopDetectionConfirmation.js';
import { FolderTrustDialog } from './FolderTrustDialog.js'; import { FolderTrustDialog } from './FolderTrustDialog.js';
import { ShellConfirmationDialog } from './ShellConfirmationDialog.js'; import { ShellConfirmationDialog } from './ShellConfirmationDialog.js';
import { RadioButtonSelect } from './shared/RadioButtonSelect.js'; import { RadioButtonSelect } from './shared/RadioButtonSelect.js';
@@ -83,6 +84,13 @@ export const DialogManager = () => {
<ShellConfirmationDialog request={uiState.shellConfirmationRequest} /> <ShellConfirmationDialog request={uiState.shellConfirmationRequest} />
); );
} }
if (uiState.loopDetectionConfirmationRequest) {
return (
<LoopDetectionConfirmation
onComplete={uiState.loopDetectionConfirmationRequest.onComplete}
/>
);
}
if (uiState.confirmationRequest) { if (uiState.confirmationRequest) {
return ( return (
<Box flexDirection="column"> <Box flexDirection="column">
@@ -0,0 +1,34 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { renderWithProviders } from '../../test-utils/render.js';
import { describe, it, expect, vi } from 'vitest';
import { LoopDetectionConfirmation } from './LoopDetectionConfirmation.js';
describe('LoopDetectionConfirmation', () => {
const onComplete = vi.fn();
it('renders correctly', () => {
const { lastFrame } = renderWithProviders(
<LoopDetectionConfirmation onComplete={onComplete} />,
);
expect(lastFrame()).toMatchSnapshot();
});
it('contains the expected options', () => {
const { lastFrame } = renderWithProviders(
<LoopDetectionConfirmation onComplete={onComplete} />,
);
const output = lastFrame()!.toString();
expect(output).toContain('A potential loop was detected');
expect(output).toContain('Keep loop detection enabled (esc)');
expect(output).toContain('Disable loop detection for this session');
expect(output).toContain(
'This can happen due to repetitive tool calls or other model behavior',
);
});
});
@@ -0,0 +1,88 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { Box, Text } from 'ink';
import type { RadioSelectItem } from './shared/RadioButtonSelect.js';
import { RadioButtonSelect } from './shared/RadioButtonSelect.js';
import { useKeypress } from '../hooks/useKeypress.js';
import { theme } from '../semantic-colors.js';
export type LoopDetectionConfirmationResult = {
userSelection: 'disable' | 'keep';
};
interface LoopDetectionConfirmationProps {
onComplete: (result: LoopDetectionConfirmationResult) => void;
}
export function LoopDetectionConfirmation({
onComplete,
}: LoopDetectionConfirmationProps) {
useKeypress(
(key) => {
if (key.name === 'escape') {
onComplete({
userSelection: 'keep',
});
}
},
{ isActive: true },
);
const OPTIONS: Array<RadioSelectItem<LoopDetectionConfirmationResult>> = [
{
label: 'Keep loop detection enabled (esc)',
value: {
userSelection: 'keep',
},
},
{
label: 'Disable loop detection for this session',
value: {
userSelection: 'disable',
},
},
];
return (
<Box
flexDirection="column"
borderStyle="round"
borderColor={theme.status.warning}
width="100%"
marginLeft={1}
>
<Box paddingX={1} paddingY={0} flexDirection="column">
<Box minHeight={1}>
<Box minWidth={3}>
<Text color={theme.status.warning} aria-label="Loop detected:">
?
</Text>
</Box>
<Box>
<Text wrap="truncate-end">
<Text color={theme.text.primary} bold>
A potential loop was detected
</Text>{' '}
</Text>
</Box>
</Box>
<Box width="100%" marginTop={1}>
<Box flexDirection="column">
<Text color={theme.text.secondary}>
This can happen due to repetitive tool calls or other model
behavior. Do you want to keep loop detection enabled or disable it
for this session?
</Text>
<Box marginTop={1}>
<RadioButtonSelect items={OPTIONS} onSelect={onComplete} />
</Box>
</Box>
</Box>
</Box>
</Box>
);
}
@@ -0,0 +1,13 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
exports[`LoopDetectionConfirmation > renders correctly 1`] = `
" ╭──────────────────────────────────────────────────────────────────────────────────────────────────╮
│ ? A potential loop was detected │
│ │
│ This can happen due to repetitive tool calls or other model behavior. Do you want to keep loop │
│ detection enabled or disable it for this session? │
│ │
│ ● 1. Keep loop detection enabled (esc) │
│ 2. Disable loop detection for this session │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯"
`;
@@ -11,6 +11,7 @@ import type {
ConsoleMessageItem, ConsoleMessageItem,
ShellConfirmationRequest, ShellConfirmationRequest,
ConfirmationRequest, ConfirmationRequest,
LoopDetectionConfirmationRequest,
HistoryItemWithoutId, HistoryItemWithoutId,
StreamingState, StreamingState,
} from '../types.js'; } from '../types.js';
@@ -53,6 +54,7 @@ export interface UIState {
commandContext: CommandContext; commandContext: CommandContext;
shellConfirmationRequest: ShellConfirmationRequest | null; shellConfirmationRequest: ShellConfirmationRequest | null;
confirmationRequest: ConfirmationRequest | null; confirmationRequest: ConfirmationRequest | null;
loopDetectionConfirmationRequest: LoopDetectionConfirmationRequest | null;
geminiMdFileCount: number; geminiMdFileCount: number;
streamingState: StreamingState; streamingState: StreamingState;
initError: string | null; initError: string | null;
@@ -1789,4 +1789,262 @@ describe('useGeminiStream', () => {
); );
}); });
}); });
describe('Loop Detection Confirmation', () => {
beforeEach(() => {
// Add mock for getLoopDetectionService to the config
const mockLoopDetectionService = {
disableForSession: vi.fn(),
};
mockConfig.getGeminiClient = vi.fn().mockReturnValue({
...new MockedGeminiClientClass(mockConfig),
getLoopDetectionService: () => mockLoopDetectionService,
});
});
it('should set loopDetectionConfirmationRequest when LoopDetected event is received', async () => {
mockSendMessageStream.mockReturnValue(
(async function* () {
yield {
type: ServerGeminiEventType.Content,
value: 'Some content',
};
yield {
type: ServerGeminiEventType.LoopDetected,
};
})(),
);
const { result } = renderTestHook();
await act(async () => {
await result.current.submitQuery('test query');
});
await waitFor(() => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
expect(
typeof result.current.loopDetectionConfirmationRequest?.onComplete,
).toBe('function');
});
});
it('should disable loop detection and show message when user selects "disable"', async () => {
const mockLoopDetectionService = {
disableForSession: vi.fn(),
};
const mockClient = {
...new MockedGeminiClientClass(mockConfig),
getLoopDetectionService: () => mockLoopDetectionService,
};
mockConfig.getGeminiClient = vi.fn().mockReturnValue(mockClient);
mockSendMessageStream.mockReturnValue(
(async function* () {
yield {
type: ServerGeminiEventType.LoopDetected,
};
})(),
);
const { result } = renderTestHook();
await act(async () => {
await result.current.submitQuery('test query');
});
// Wait for confirmation request to be set
await waitFor(() => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
});
// Simulate user selecting "disable"
await act(async () => {
result.current.loopDetectionConfirmationRequest?.onComplete({
userSelection: 'disable',
});
});
// Verify loop detection was disabled
expect(mockLoopDetectionService.disableForSession).toHaveBeenCalledTimes(
1,
);
// Verify confirmation request was cleared
expect(result.current.loopDetectionConfirmationRequest).toBeNull();
// Verify appropriate message was added
expect(mockAddItem).toHaveBeenCalledWith(
{
type: 'info',
text: 'Loop detection has been disabled for this session. Please try your request again.',
},
expect.any(Number),
);
});
it('should keep loop detection enabled and show message when user selects "keep"', async () => {
const mockLoopDetectionService = {
disableForSession: vi.fn(),
};
const mockClient = {
...new MockedGeminiClientClass(mockConfig),
getLoopDetectionService: () => mockLoopDetectionService,
};
mockConfig.getGeminiClient = vi.fn().mockReturnValue(mockClient);
mockSendMessageStream.mockReturnValue(
(async function* () {
yield {
type: ServerGeminiEventType.LoopDetected,
};
})(),
);
const { result } = renderTestHook();
await act(async () => {
await result.current.submitQuery('test query');
});
// Wait for confirmation request to be set
await waitFor(() => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
});
// Simulate user selecting "keep"
await act(async () => {
result.current.loopDetectionConfirmationRequest?.onComplete({
userSelection: 'keep',
});
});
// Verify loop detection was NOT disabled
expect(mockLoopDetectionService.disableForSession).not.toHaveBeenCalled();
// Verify confirmation request was cleared
expect(result.current.loopDetectionConfirmationRequest).toBeNull();
// Verify appropriate message was added
expect(mockAddItem).toHaveBeenCalledWith(
{
type: 'info',
text: 'A potential loop was detected. This can happen due to repetitive tool calls or other model behavior. The request has been halted.',
},
expect.any(Number),
);
});
it('should handle multiple loop detection events properly', async () => {
const { result } = renderTestHook();
// First loop detection - set up fresh mock for first call
mockSendMessageStream.mockReturnValueOnce(
(async function* () {
yield {
type: ServerGeminiEventType.LoopDetected,
};
})(),
);
// First loop detection
await act(async () => {
await result.current.submitQuery('first query');
});
await waitFor(() => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
});
// Simulate user selecting "keep" for first request
await act(async () => {
result.current.loopDetectionConfirmationRequest?.onComplete({
userSelection: 'keep',
});
});
expect(result.current.loopDetectionConfirmationRequest).toBeNull();
// Verify first message was added
expect(mockAddItem).toHaveBeenCalledWith(
{
type: 'info',
text: 'A potential loop was detected. This can happen due to repetitive tool calls or other model behavior. The request has been halted.',
},
expect.any(Number),
);
// Second loop detection - set up fresh mock for second call
mockSendMessageStream.mockReturnValueOnce(
(async function* () {
yield {
type: ServerGeminiEventType.LoopDetected,
};
})(),
);
// Second loop detection
await act(async () => {
await result.current.submitQuery('second query');
});
await waitFor(() => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
});
// Simulate user selecting "disable" for second request
await act(async () => {
result.current.loopDetectionConfirmationRequest?.onComplete({
userSelection: 'disable',
});
});
expect(result.current.loopDetectionConfirmationRequest).toBeNull();
// Verify second message was added
expect(mockAddItem).toHaveBeenCalledWith(
{
type: 'info',
text: 'Loop detection has been disabled for this session. Please try your request again.',
},
expect.any(Number),
);
});
it('should process LoopDetected event after moving pending history to history', async () => {
mockSendMessageStream.mockReturnValue(
(async function* () {
yield {
type: ServerGeminiEventType.Content,
value: 'Some response content',
};
yield {
type: ServerGeminiEventType.LoopDetected,
};
})(),
);
const { result } = renderTestHook();
await act(async () => {
await result.current.submitQuery('test query');
});
// Verify that the content was added to history before the loop detection dialog
await waitFor(() => {
expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({
type: 'gemini',
text: 'Some response content',
}),
expect.any(Number),
);
});
// Then verify loop detection confirmation request was set
await waitFor(() => {
expect(result.current.loopDetectionConfirmationRequest).not.toBeNull();
});
});
});
}); });
+38 -8
View File
@@ -153,6 +153,12 @@ export const useGeminiStream = (
); );
const loopDetectedRef = useRef(false); const loopDetectedRef = useRef(false);
const [
loopDetectionConfirmationRequest,
setLoopDetectionConfirmationRequest,
] = useState<{
onComplete: (result: { userSelection: 'disable' | 'keep' }) => void;
} | null>(null);
const onExec = useCallback(async (done: Promise<void>) => { const onExec = useCallback(async (done: Promise<void>) => {
setIsResponding(true); setIsResponding(true);
@@ -588,15 +594,38 @@ export const useGeminiStream = (
[addItem, config], [addItem, config],
); );
const handleLoopDetectionConfirmation = useCallback(
(result: { userSelection: 'disable' | 'keep' }) => {
setLoopDetectionConfirmationRequest(null);
if (result.userSelection === 'disable') {
config.getGeminiClient().getLoopDetectionService().disableForSession();
addItem(
{
type: 'info',
text: `Loop detection has been disabled for this session. Please try your request again.`,
},
Date.now(),
);
} else {
addItem(
{
type: 'info',
text: `A potential loop was detected. This can happen due to repetitive tool calls or other model behavior. The request has been halted.`,
},
Date.now(),
);
}
},
[config, addItem],
);
const handleLoopDetectedEvent = useCallback(() => { const handleLoopDetectedEvent = useCallback(() => {
addItem( // Show the confirmation dialog to choose whether to disable loop detection
{ setLoopDetectionConfirmationRequest({
type: 'info', onComplete: handleLoopDetectionConfirmation,
text: `A potential loop was detected. This can happen due to repetitive tool calls or other model behavior. The request has been halted.`, });
}, }, [handleLoopDetectionConfirmation]);
Date.now(),
);
}, [addItem]);
const processGeminiStreamEvents = useCallback( const processGeminiStreamEvents = useCallback(
async ( async (
@@ -1045,5 +1074,6 @@ export const useGeminiStream = (
pendingHistoryItems, pendingHistoryItems,
thought, thought,
cancelOngoingRequest, cancelOngoingRequest,
loopDetectionConfirmationRequest,
}; };
}; };
+4
View File
@@ -284,3 +284,7 @@ export interface ConfirmationRequest {
prompt: ReactNode; prompt: ReactNode;
onConfirm: (confirm: boolean) => void; onConfirm: (confirm: boolean) => void;
} }
export interface LoopDetectionConfirmationRequest {
onComplete: (result: { userSelection: 'disable' | 'keep' }) => void;
}
+4
View File
@@ -186,6 +186,10 @@ export class GeminiClient {
return this.chat?.getChatRecordingService(); return this.chat?.getChatRecordingService();
} }
getLoopDetectionService(): LoopDetectionService {
return this.loopDetector;
}
async addDirectoryContext(): Promise<void> { async addDirectoryContext(): Promise<void> {
if (!this.chat) { if (!this.chat) {
return; return;
@@ -130,6 +130,15 @@ describe('LoopDetectionService', () => {
expect(service.addAndCheck(toolCallEvent)).toBe(true); expect(service.addAndCheck(toolCallEvent)).toBe(true);
expect(loggers.logLoopDetected).toHaveBeenCalledTimes(1); expect(loggers.logLoopDetected).toHaveBeenCalledTimes(1);
}); });
it('should not detect a loop when disabled for session', () => {
service.disableForSession();
const event = createToolCallRequestEvent('testTool', { param: 'value' });
for (let i = 0; i < TOOL_CALL_LOOP_THRESHOLD; i++) {
expect(service.addAndCheck(event)).toBe(false);
}
expect(loggers.logLoopDetected).not.toHaveBeenCalled();
});
}); });
describe('Content Loop Detection', () => { describe('Content Loop Detection', () => {
@@ -719,4 +728,12 @@ describe('LoopDetectionService LLM Checks', () => {
expect(result).toBe(false); expect(result).toBe(false);
expect(loggers.logLoopDetected).not.toHaveBeenCalled(); expect(loggers.logLoopDetected).not.toHaveBeenCalled();
}); });
it('should not trigger LLM check when disabled for session', async () => {
service.disableForSession();
await advanceTurns(30);
const result = await service.turnStarted(abortController.signal);
expect(result).toBe(false);
expect(mockGeminiClient.generateJson).not.toHaveBeenCalled();
});
}); });
@@ -74,10 +74,20 @@ export class LoopDetectionService {
private llmCheckInterval = DEFAULT_LLM_CHECK_INTERVAL; private llmCheckInterval = DEFAULT_LLM_CHECK_INTERVAL;
private lastCheckTurn = 0; private lastCheckTurn = 0;
// Session-level disable flag
private disabledForSession = false;
constructor(config: Config) { constructor(config: Config) {
this.config = config; this.config = config;
} }
/**
* Disables loop detection for the current session.
*/
disableForSession(): void {
this.disabledForSession = true;
}
private getToolCallKey(toolCall: { name: string; args: object }): string { private getToolCallKey(toolCall: { name: string; args: object }): string {
const argsString = JSON.stringify(toolCall.args); const argsString = JSON.stringify(toolCall.args);
const keyString = `${toolCall.name}:${argsString}`; const keyString = `${toolCall.name}:${argsString}`;
@@ -90,8 +100,8 @@ export class LoopDetectionService {
* @returns true if a loop is detected, false otherwise * @returns true if a loop is detected, false otherwise
*/ */
addAndCheck(event: ServerGeminiStreamEvent): boolean { addAndCheck(event: ServerGeminiStreamEvent): boolean {
if (this.loopDetected) { if (this.loopDetected || this.disabledForSession) {
return true; return this.loopDetected;
} }
switch (event.type) { switch (event.type) {
@@ -121,6 +131,9 @@ export class LoopDetectionService {
* @returns A promise that resolves to `true` if a loop is detected, and `false` otherwise. * @returns A promise that resolves to `true` if a loop is detected, and `false` otherwise.
*/ */
async turnStarted(signal: AbortSignal) { async turnStarted(signal: AbortSignal) {
if (this.disabledForSession) {
return false;
}
this.turnsInCurrentPrompt++; this.turnsInCurrentPrompt++;
if ( if (