From 827587196368dc688913b8ce79503762f83d0555 Mon Sep 17 00:00:00 2001 From: Jerop Kipruto Date: Thu, 12 Feb 2026 16:49:07 -0500 Subject: [PATCH] Hide AskUser tool validation errors from UI (agent self-corrects) (#18954) --- .../messages/ToolGroupMessage.test.tsx | 60 ++++---- .../components/messages/ToolGroupMessage.tsx | 18 +-- .../src/ui/components/messages/ToolShared.tsx | 10 +- .../ToolGroupMessage.test.tsx.snap | 14 +- packages/core/src/index.ts | 1 + packages/core/src/tools/ask-user.test.ts | 138 +++++++++++++++++- packages/core/src/tools/ask-user.ts | 60 ++++++++ 7 files changed, 245 insertions(+), 56 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx index d2d3cd277a..d2ada4d659 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx @@ -409,35 +409,41 @@ describe('', () => { describe('Ask User Filtering', () => { it.each([ - ToolCallStatus.Pending, - ToolCallStatus.Executing, - ToolCallStatus.Confirming, - ])('filters out ask_user when status is %s', (status) => { - const toolCalls = [ - createToolCall({ - callId: `ask-user-${status}`, - name: ASK_USER_DISPLAY_NAME, - status, - }), - ]; - - const { lastFrame, unmount } = renderWithProviders( - , - { config: baseMockConfig }, - ); - - expect(lastFrame()).toMatchSnapshot(); - unmount(); - }); - - it.each([ToolCallStatus.Success, ToolCallStatus.Error])( - 'does NOT filter out ask_user when status is %s', - (status) => { + { + status: ToolCallStatus.Pending, + resultDisplay: 'test result', + shouldHide: true, + }, + { + status: ToolCallStatus.Executing, + resultDisplay: 'test result', + shouldHide: true, + }, + { + status: ToolCallStatus.Confirming, + resultDisplay: 'test result', + shouldHide: true, + }, + { + status: ToolCallStatus.Success, + resultDisplay: 'test result', + shouldHide: false, + }, + { status: ToolCallStatus.Error, resultDisplay: '', shouldHide: true }, + { + status: ToolCallStatus.Error, + resultDisplay: 'error message', + shouldHide: false, + }, + ])( + 'filtering logic for status=$status and hasResult=$resultDisplay', + ({ status, resultDisplay, shouldHide }) => { const toolCalls = [ createToolCall({ callId: `ask-user-${status}`, name: ASK_USER_DISPLAY_NAME, status, + resultDisplay, }), ]; @@ -446,7 +452,11 @@ describe('', () => { { config: baseMockConfig }, ); - expect(lastFrame()).toMatchSnapshot(); + if (shouldHide) { + expect(lastFrame()).toBe(''); + } else { + expect(lastFrame()).toMatchSnapshot(); + } unmount(); }, ); diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 07ae280558..a553679562 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -14,7 +14,7 @@ import { ShellToolMessage } from './ShellToolMessage.js'; import { theme } from '../../semantic-colors.js'; import { useConfig } from '../../contexts/ConfigContext.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 { useUIState } from '../../contexts/UIStateContext.js'; @@ -30,15 +30,6 @@ interface ToolGroupMessageProps { 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 const TOOL_MESSAGE_HORIZONTAL_MARGIN = 4; @@ -51,9 +42,12 @@ export const ToolGroupMessage: React.FC = ({ borderTop: borderTopOverride, 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( - () => allToolCalls.filter((t) => !isAskUserInProgress(t)), + () => + allToolCalls.filter( + (t) => !shouldHideAskUserTool(t.name, t.status, !!t.resultDisplay), + ), [allToolCalls], ); diff --git a/packages/cli/src/ui/components/messages/ToolShared.tsx b/packages/cli/src/ui/components/messages/ToolShared.tsx index a48aefdc7c..eaf3c73bfb 100644 --- a/packages/cli/src/ui/components/messages/ToolShared.tsx +++ b/packages/cli/src/ui/components/messages/ToolShared.tsx @@ -18,7 +18,7 @@ import { theme } from '../../semantic-colors.js'; import { type Config, SHELL_TOOL_NAME, - ASK_USER_DISPLAY_NAME, + isCompletedAskUserTool, type ToolResultDisplay, } from '@google/gemini-cli-core'; import { useInactivityTimer } from '../../hooks/useInactivityTimer.js'; @@ -205,13 +205,7 @@ export const ToolInfo: React.FC = ({ }, [emphasis]); // Hide description for completed Ask User tools (the result display speaks for itself) - const isCompletedAskUser = - name === ASK_USER_DISPLAY_NAME && - [ - ToolCallStatus.Success, - ToolCallStatus.Error, - ToolCallStatus.Canceled, - ].includes(status); + const isCompletedAskUser = isCompletedAskUserTool(name, status); return ( diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ToolGroupMessage.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/ToolGroupMessage.test.tsx.snap index 3586b32c21..7cda673c07 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/ToolGroupMessage.test.tsx.snap +++ b/packages/cli/src/ui/components/messages/__snapshots__/ToolGroupMessage.test.tsx.snap @@ -1,27 +1,21 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html -exports[` > Ask User Filtering > does NOT filter out ask_user when status is Error 1`] = ` +exports[` > Ask User Filtering > filtering logic for status='Error' and hasResult='error message' 1`] = ` "╭──────────────────────────────────────────────────────────────────────────╮ │ x Ask User │ │ │ -│ Test result │ +│ error message │ ╰──────────────────────────────────────────────────────────────────────────╯" `; -exports[` > Ask User Filtering > does NOT filter out ask_user when status is Success 1`] = ` +exports[` > Ask User Filtering > filtering logic for status='Success' and hasResult='test result' 1`] = ` "╭──────────────────────────────────────────────────────────────────────────╮ │ ✓ Ask User │ │ │ -│ Test result │ +│ test result │ ╰──────────────────────────────────────────────────────────────────────────╯" `; -exports[` > Ask User Filtering > filters out ask_user when status is Confirming 1`] = `""`; - -exports[` > Ask User Filtering > filters out ask_user when status is Executing 1`] = `""`; - -exports[` > Ask User Filtering > filters out ask_user when status is Pending 1`] = `""`; - exports[` > Ask User Filtering > shows other tools when ask_user is filtered out 1`] = ` "╭──────────────────────────────────────────────────────────────────────────╮ │ ✓ other-tool A tool for testing │ diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8232f73570..448e555df4 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -157,6 +157,7 @@ export * from './tools/read-many-files.js'; export * from './tools/mcp-client.js'; export * from './tools/mcp-tool.js'; export * from './tools/write-todos.js'; +export * from './tools/ask-user.js'; // MCP OAuth export { MCPOAuthProvider } from './mcp/oauth-provider.js'; diff --git a/packages/core/src/tools/ask-user.test.ts b/packages/core/src/tools/ask-user.test.ts index c7d64eae6e..19c98fbc6b 100644 --- a/packages/core/src/tools/ask-user.test.ts +++ b/packages/core/src/tools/ask-user.test.ts @@ -5,10 +5,97 @@ */ 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 type { MessageBus } from '../confirmation-bus/message-bus.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', () => { 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', () => { it('should return confirmation details with normalized questions', async () => { const questions = [ diff --git a/packages/core/src/tools/ask-user.ts b/packages/core/src/tools/ask-user.ts index db9103c720..951094d9ad 100644 --- a/packages/core/src/tools/ask-user.ts +++ b/packages/core/src/tools/ask-user.ts @@ -13,6 +13,7 @@ import { type ToolConfirmationPayload, ToolConfirmationOutcome, } from './tools.js'; +import { ToolErrorType } from './tool-error.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { QuestionType, type Question } from '../confirmation-bus/types.js'; import { ASK_USER_TOOL_NAME, ASK_USER_DISPLAY_NAME } from './tool-names.js'; @@ -154,6 +155,23 @@ export class AskUserTool extends BaseDeclarativeTool< ): AskUserInvocation { return new AskUserInvocation(params, messageBus, toolName, toolDisplayName); } + + override async validateBuildAndExecute( + params: AskUserParams, + abortSignal: AbortSignal, + ): Promise { + 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< @@ -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) + ); +}