mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-12 12:54:07 -07:00
Hide AskUser tool validation errors from UI (agent self-corrects) (#18954)
This commit is contained in:
@@ -409,35 +409,41 @@ describe('<ToolGroupMessage />', () => {
|
|||||||
|
|
||||||
describe('Ask User Filtering', () => {
|
describe('Ask User Filtering', () => {
|
||||||
it.each([
|
it.each([
|
||||||
ToolCallStatus.Pending,
|
{
|
||||||
ToolCallStatus.Executing,
|
status: ToolCallStatus.Pending,
|
||||||
ToolCallStatus.Confirming,
|
resultDisplay: 'test result',
|
||||||
])('filters out ask_user when status is %s', (status) => {
|
shouldHide: true,
|
||||||
const toolCalls = [
|
},
|
||||||
createToolCall({
|
{
|
||||||
callId: `ask-user-${status}`,
|
status: ToolCallStatus.Executing,
|
||||||
name: ASK_USER_DISPLAY_NAME,
|
resultDisplay: 'test result',
|
||||||
status,
|
shouldHide: true,
|
||||||
}),
|
},
|
||||||
];
|
{
|
||||||
|
status: ToolCallStatus.Confirming,
|
||||||
const { lastFrame, unmount } = renderWithProviders(
|
resultDisplay: 'test result',
|
||||||
<ToolGroupMessage {...baseProps} toolCalls={toolCalls} />,
|
shouldHide: true,
|
||||||
{ config: baseMockConfig },
|
},
|
||||||
);
|
{
|
||||||
|
status: ToolCallStatus.Success,
|
||||||
expect(lastFrame()).toMatchSnapshot();
|
resultDisplay: 'test result',
|
||||||
unmount();
|
shouldHide: false,
|
||||||
});
|
},
|
||||||
|
{ status: ToolCallStatus.Error, resultDisplay: '', shouldHide: true },
|
||||||
it.each([ToolCallStatus.Success, ToolCallStatus.Error])(
|
{
|
||||||
'does NOT filter out ask_user when status is %s',
|
status: ToolCallStatus.Error,
|
||||||
(status) => {
|
resultDisplay: 'error message',
|
||||||
|
shouldHide: false,
|
||||||
|
},
|
||||||
|
])(
|
||||||
|
'filtering logic for status=$status and hasResult=$resultDisplay',
|
||||||
|
({ status, resultDisplay, shouldHide }) => {
|
||||||
const toolCalls = [
|
const toolCalls = [
|
||||||
createToolCall({
|
createToolCall({
|
||||||
callId: `ask-user-${status}`,
|
callId: `ask-user-${status}`,
|
||||||
name: ASK_USER_DISPLAY_NAME,
|
name: ASK_USER_DISPLAY_NAME,
|
||||||
status,
|
status,
|
||||||
|
resultDisplay,
|
||||||
}),
|
}),
|
||||||
];
|
];
|
||||||
|
|
||||||
@@ -446,7 +452,11 @@ describe('<ToolGroupMessage />', () => {
|
|||||||
{ config: baseMockConfig },
|
{ config: baseMockConfig },
|
||||||
);
|
);
|
||||||
|
|
||||||
expect(lastFrame()).toMatchSnapshot();
|
if (shouldHide) {
|
||||||
|
expect(lastFrame()).toBe('');
|
||||||
|
} else {
|
||||||
|
expect(lastFrame()).toMatchSnapshot();
|
||||||
|
}
|
||||||
unmount();
|
unmount();
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -14,7 +14,7 @@ import { ShellToolMessage } from './ShellToolMessage.js';
|
|||||||
import { theme } from '../../semantic-colors.js';
|
import { theme } from '../../semantic-colors.js';
|
||||||
import { useConfig } from '../../contexts/ConfigContext.js';
|
import { useConfig } from '../../contexts/ConfigContext.js';
|
||||||
import { isShellTool, isThisShellFocused } from './ToolShared.js';
|
import { isShellTool, isThisShellFocused } from './ToolShared.js';
|
||||||
import { ASK_USER_DISPLAY_NAME } from '@google/gemini-cli-core';
|
import { shouldHideAskUserTool } from '@google/gemini-cli-core';
|
||||||
import { ShowMoreLines } from '../ShowMoreLines.js';
|
import { ShowMoreLines } from '../ShowMoreLines.js';
|
||||||
import { useUIState } from '../../contexts/UIStateContext.js';
|
import { useUIState } from '../../contexts/UIStateContext.js';
|
||||||
|
|
||||||
@@ -30,15 +30,6 @@ interface ToolGroupMessageProps {
|
|||||||
borderBottom?: boolean;
|
borderBottom?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Helper to identify Ask User tools that are in progress (have their own dialog UI)
|
|
||||||
const isAskUserInProgress = (t: IndividualToolCallDisplay): boolean =>
|
|
||||||
t.name === ASK_USER_DISPLAY_NAME &&
|
|
||||||
[
|
|
||||||
ToolCallStatus.Pending,
|
|
||||||
ToolCallStatus.Executing,
|
|
||||||
ToolCallStatus.Confirming,
|
|
||||||
].includes(t.status);
|
|
||||||
|
|
||||||
// Main component renders the border and maps the tools using ToolMessage
|
// Main component renders the border and maps the tools using ToolMessage
|
||||||
const TOOL_MESSAGE_HORIZONTAL_MARGIN = 4;
|
const TOOL_MESSAGE_HORIZONTAL_MARGIN = 4;
|
||||||
|
|
||||||
@@ -51,9 +42,12 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
|
|||||||
borderTop: borderTopOverride,
|
borderTop: borderTopOverride,
|
||||||
borderBottom: borderBottomOverride,
|
borderBottom: borderBottomOverride,
|
||||||
}) => {
|
}) => {
|
||||||
// Filter out in-progress Ask User tools (they have their own AskUserDialog UI)
|
// Filter out Ask User tools that should be hidden (e.g. in-progress or errors without result)
|
||||||
const toolCalls = useMemo(
|
const toolCalls = useMemo(
|
||||||
() => allToolCalls.filter((t) => !isAskUserInProgress(t)),
|
() =>
|
||||||
|
allToolCalls.filter(
|
||||||
|
(t) => !shouldHideAskUserTool(t.name, t.status, !!t.resultDisplay),
|
||||||
|
),
|
||||||
[allToolCalls],
|
[allToolCalls],
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|||||||
@@ -18,7 +18,7 @@ import { theme } from '../../semantic-colors.js';
|
|||||||
import {
|
import {
|
||||||
type Config,
|
type Config,
|
||||||
SHELL_TOOL_NAME,
|
SHELL_TOOL_NAME,
|
||||||
ASK_USER_DISPLAY_NAME,
|
isCompletedAskUserTool,
|
||||||
type ToolResultDisplay,
|
type ToolResultDisplay,
|
||||||
} from '@google/gemini-cli-core';
|
} from '@google/gemini-cli-core';
|
||||||
import { useInactivityTimer } from '../../hooks/useInactivityTimer.js';
|
import { useInactivityTimer } from '../../hooks/useInactivityTimer.js';
|
||||||
@@ -205,13 +205,7 @@ export const ToolInfo: React.FC<ToolInfoProps> = ({
|
|||||||
}, [emphasis]);
|
}, [emphasis]);
|
||||||
|
|
||||||
// Hide description for completed Ask User tools (the result display speaks for itself)
|
// Hide description for completed Ask User tools (the result display speaks for itself)
|
||||||
const isCompletedAskUser =
|
const isCompletedAskUser = isCompletedAskUserTool(name, status);
|
||||||
name === ASK_USER_DISPLAY_NAME &&
|
|
||||||
[
|
|
||||||
ToolCallStatus.Success,
|
|
||||||
ToolCallStatus.Error,
|
|
||||||
ToolCallStatus.Canceled,
|
|
||||||
].includes(status);
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<Box overflow="hidden" height={1} flexGrow={1} flexShrink={1}>
|
<Box overflow="hidden" height={1} flexGrow={1} flexShrink={1}>
|
||||||
|
|||||||
+4
-10
@@ -1,27 +1,21 @@
|
|||||||
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
|
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
|
||||||
|
|
||||||
exports[`<ToolGroupMessage /> > Ask User Filtering > does NOT filter out ask_user when status is Error 1`] = `
|
exports[`<ToolGroupMessage /> > Ask User Filtering > filtering logic for status='Error' and hasResult='error message' 1`] = `
|
||||||
"╭──────────────────────────────────────────────────────────────────────────╮
|
"╭──────────────────────────────────────────────────────────────────────────╮
|
||||||
│ x Ask User │
|
│ x Ask User │
|
||||||
│ │
|
│ │
|
||||||
│ Test result │
|
│ error message │
|
||||||
╰──────────────────────────────────────────────────────────────────────────╯"
|
╰──────────────────────────────────────────────────────────────────────────╯"
|
||||||
`;
|
`;
|
||||||
|
|
||||||
exports[`<ToolGroupMessage /> > Ask User Filtering > does NOT filter out ask_user when status is Success 1`] = `
|
exports[`<ToolGroupMessage /> > Ask User Filtering > filtering logic for status='Success' and hasResult='test result' 1`] = `
|
||||||
"╭──────────────────────────────────────────────────────────────────────────╮
|
"╭──────────────────────────────────────────────────────────────────────────╮
|
||||||
│ ✓ Ask User │
|
│ ✓ Ask User │
|
||||||
│ │
|
│ │
|
||||||
│ Test result │
|
│ test result │
|
||||||
╰──────────────────────────────────────────────────────────────────────────╯"
|
╰──────────────────────────────────────────────────────────────────────────╯"
|
||||||
`;
|
`;
|
||||||
|
|
||||||
exports[`<ToolGroupMessage /> > Ask User Filtering > filters out ask_user when status is Confirming 1`] = `""`;
|
|
||||||
|
|
||||||
exports[`<ToolGroupMessage /> > Ask User Filtering > filters out ask_user when status is Executing 1`] = `""`;
|
|
||||||
|
|
||||||
exports[`<ToolGroupMessage /> > Ask User Filtering > filters out ask_user when status is Pending 1`] = `""`;
|
|
||||||
|
|
||||||
exports[`<ToolGroupMessage /> > Ask User Filtering > shows other tools when ask_user is filtered out 1`] = `
|
exports[`<ToolGroupMessage /> > Ask User Filtering > shows other tools when ask_user is filtered out 1`] = `
|
||||||
"╭──────────────────────────────────────────────────────────────────────────╮
|
"╭──────────────────────────────────────────────────────────────────────────╮
|
||||||
│ ✓ other-tool A tool for testing │
|
│ ✓ other-tool A tool for testing │
|
||||||
|
|||||||
@@ -157,6 +157,7 @@ export * from './tools/read-many-files.js';
|
|||||||
export * from './tools/mcp-client.js';
|
export * from './tools/mcp-client.js';
|
||||||
export * from './tools/mcp-tool.js';
|
export * from './tools/mcp-tool.js';
|
||||||
export * from './tools/write-todos.js';
|
export * from './tools/write-todos.js';
|
||||||
|
export * from './tools/ask-user.js';
|
||||||
|
|
||||||
// MCP OAuth
|
// MCP OAuth
|
||||||
export { MCPOAuthProvider } from './mcp/oauth-provider.js';
|
export { MCPOAuthProvider } from './mcp/oauth-provider.js';
|
||||||
|
|||||||
@@ -5,10 +5,97 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||||
import { AskUserTool } from './ask-user.js';
|
import {
|
||||||
|
AskUserTool,
|
||||||
|
shouldHideAskUserTool,
|
||||||
|
isCompletedAskUserTool,
|
||||||
|
} from './ask-user.js';
|
||||||
import { QuestionType, type Question } from '../confirmation-bus/types.js';
|
import { QuestionType, type Question } from '../confirmation-bus/types.js';
|
||||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||||
import { ToolConfirmationOutcome } from './tools.js';
|
import { ToolConfirmationOutcome } from './tools.js';
|
||||||
|
import { ToolErrorType } from './tool-error.js';
|
||||||
|
import { ASK_USER_DISPLAY_NAME } from './tool-names.js';
|
||||||
|
|
||||||
|
describe('AskUserTool Helpers', () => {
|
||||||
|
describe('shouldHideAskUserTool', () => {
|
||||||
|
it('returns false for non-AskUser tools', () => {
|
||||||
|
expect(shouldHideAskUserTool('other-tool', 'Success', true)).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('hides Pending AskUser tool', () => {
|
||||||
|
expect(
|
||||||
|
shouldHideAskUserTool(ASK_USER_DISPLAY_NAME, 'Pending', false),
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('hides Executing AskUser tool', () => {
|
||||||
|
expect(
|
||||||
|
shouldHideAskUserTool(ASK_USER_DISPLAY_NAME, 'Executing', false),
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('hides Confirming AskUser tool', () => {
|
||||||
|
expect(
|
||||||
|
shouldHideAskUserTool(ASK_USER_DISPLAY_NAME, 'Confirming', false),
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('shows Success AskUser tool', () => {
|
||||||
|
expect(
|
||||||
|
shouldHideAskUserTool(ASK_USER_DISPLAY_NAME, 'Success', true),
|
||||||
|
).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('shows Canceled AskUser tool', () => {
|
||||||
|
expect(
|
||||||
|
shouldHideAskUserTool(ASK_USER_DISPLAY_NAME, 'Canceled', true),
|
||||||
|
).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('hides Error AskUser tool without result display', () => {
|
||||||
|
expect(shouldHideAskUserTool(ASK_USER_DISPLAY_NAME, 'Error', false)).toBe(
|
||||||
|
true,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('shows Error AskUser tool with result display', () => {
|
||||||
|
expect(shouldHideAskUserTool(ASK_USER_DISPLAY_NAME, 'Error', true)).toBe(
|
||||||
|
false,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('isCompletedAskUserTool', () => {
|
||||||
|
it('returns false for non-AskUser tools', () => {
|
||||||
|
expect(isCompletedAskUserTool('other-tool', 'Success')).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns true for Success status', () => {
|
||||||
|
expect(isCompletedAskUserTool(ASK_USER_DISPLAY_NAME, 'Success')).toBe(
|
||||||
|
true,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns true for Error status', () => {
|
||||||
|
expect(isCompletedAskUserTool(ASK_USER_DISPLAY_NAME, 'Error')).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns true for Canceled status', () => {
|
||||||
|
expect(isCompletedAskUserTool(ASK_USER_DISPLAY_NAME, 'Canceled')).toBe(
|
||||||
|
true,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false for in-progress statuses', () => {
|
||||||
|
expect(isCompletedAskUserTool(ASK_USER_DISPLAY_NAME, 'Executing')).toBe(
|
||||||
|
false,
|
||||||
|
);
|
||||||
|
expect(isCompletedAskUserTool(ASK_USER_DISPLAY_NAME, 'Pending')).toBe(
|
||||||
|
false,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('AskUserTool', () => {
|
describe('AskUserTool', () => {
|
||||||
let mockMessageBus: MessageBus;
|
let mockMessageBus: MessageBus;
|
||||||
@@ -228,6 +315,55 @@ describe('AskUserTool', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('validateBuildAndExecute', () => {
|
||||||
|
it('should hide validation errors from returnDisplay', async () => {
|
||||||
|
const params = {
|
||||||
|
questions: [{ question: 'Test?', header: 'This is way too long' }],
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = await tool.validateBuildAndExecute(
|
||||||
|
params,
|
||||||
|
new AbortController().signal,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(result.error).toBeDefined();
|
||||||
|
expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS);
|
||||||
|
expect(result.returnDisplay).toBe('');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should NOT hide non-validation errors (if any were to occur)', async () => {
|
||||||
|
const validateParamsSpy = vi
|
||||||
|
.spyOn(tool, 'validateToolParams')
|
||||||
|
.mockReturnValue(null);
|
||||||
|
|
||||||
|
const params = {
|
||||||
|
questions: [{ question: 'Valid?', header: 'Valid' }],
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockInvocation = {
|
||||||
|
execute: vi.fn().mockRejectedValue(new Error('Some execution error')),
|
||||||
|
params,
|
||||||
|
getDescription: vi.fn().mockReturnValue(''),
|
||||||
|
toolLocations: vi.fn().mockReturnValue([]),
|
||||||
|
shouldConfirmExecute: vi.fn().mockResolvedValue(false),
|
||||||
|
};
|
||||||
|
|
||||||
|
const buildSpy = vi.spyOn(tool, 'build').mockReturnValue(mockInvocation);
|
||||||
|
|
||||||
|
const result = await tool.validateBuildAndExecute(
|
||||||
|
params,
|
||||||
|
new AbortController().signal,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(result.error).toBeDefined();
|
||||||
|
expect(result.error?.type).toBe(ToolErrorType.EXECUTION_FAILED);
|
||||||
|
expect(result.returnDisplay).toBe('Some execution error');
|
||||||
|
|
||||||
|
buildSpy.mockRestore();
|
||||||
|
validateParamsSpy.mockRestore();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('shouldConfirmExecute', () => {
|
describe('shouldConfirmExecute', () => {
|
||||||
it('should return confirmation details with normalized questions', async () => {
|
it('should return confirmation details with normalized questions', async () => {
|
||||||
const questions = [
|
const questions = [
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ import {
|
|||||||
type ToolConfirmationPayload,
|
type ToolConfirmationPayload,
|
||||||
ToolConfirmationOutcome,
|
ToolConfirmationOutcome,
|
||||||
} from './tools.js';
|
} from './tools.js';
|
||||||
|
import { ToolErrorType } from './tool-error.js';
|
||||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||||
import { QuestionType, type Question } from '../confirmation-bus/types.js';
|
import { QuestionType, type Question } from '../confirmation-bus/types.js';
|
||||||
import { ASK_USER_TOOL_NAME, ASK_USER_DISPLAY_NAME } from './tool-names.js';
|
import { ASK_USER_TOOL_NAME, ASK_USER_DISPLAY_NAME } from './tool-names.js';
|
||||||
@@ -154,6 +155,23 @@ export class AskUserTool extends BaseDeclarativeTool<
|
|||||||
): AskUserInvocation {
|
): AskUserInvocation {
|
||||||
return new AskUserInvocation(params, messageBus, toolName, toolDisplayName);
|
return new AskUserInvocation(params, messageBus, toolName, toolDisplayName);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
override async validateBuildAndExecute(
|
||||||
|
params: AskUserParams,
|
||||||
|
abortSignal: AbortSignal,
|
||||||
|
): Promise<ToolResult> {
|
||||||
|
const result = await super.validateBuildAndExecute(params, abortSignal);
|
||||||
|
if (
|
||||||
|
result.error &&
|
||||||
|
result.error.type === ToolErrorType.INVALID_TOOL_PARAMS
|
||||||
|
) {
|
||||||
|
return {
|
||||||
|
...result,
|
||||||
|
returnDisplay: '',
|
||||||
|
};
|
||||||
|
}
|
||||||
|
return result;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
export class AskUserInvocation extends BaseToolInvocation<
|
export class AskUserInvocation extends BaseToolInvocation<
|
||||||
@@ -242,3 +260,45 @@ export class AskUserInvocation extends BaseToolInvocation<
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Determines if an 'Ask User' tool call should be hidden from the standard tool history UI.
|
||||||
|
*
|
||||||
|
* We hide Ask User tools in two cases:
|
||||||
|
* 1. They are in progress because they are displayed using a specialized UI (AskUserDialog).
|
||||||
|
* 2. They have errored without a result display (e.g. validation errors), in which case
|
||||||
|
* the agent self-corrects and we don't want to clutter the UI.
|
||||||
|
*
|
||||||
|
* NOTE: The 'status' parameter values are intended to match the CLI's ToolCallStatus enum.
|
||||||
|
*/
|
||||||
|
export function shouldHideAskUserTool(
|
||||||
|
name: string,
|
||||||
|
status: string,
|
||||||
|
hasResultDisplay: boolean,
|
||||||
|
): boolean {
|
||||||
|
if (name !== ASK_USER_DISPLAY_NAME) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Case 1: In-progress tools (Pending, Executing, Confirming)
|
||||||
|
if (['Pending', 'Executing', 'Confirming'].includes(status)) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Case 2: Error without result display
|
||||||
|
if (status === 'Error' && !hasResultDisplay) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Returns true if the tool name and status correspond to a completed 'Ask User' tool call.
|
||||||
|
*/
|
||||||
|
export function isCompletedAskUserTool(name: string, status: string): boolean {
|
||||||
|
return (
|
||||||
|
name === ASK_USER_DISPLAY_NAME &&
|
||||||
|
['Success', 'Error', 'Canceled'].includes(status)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user