From 9df604b01b0220024e78fd6513fac4ad242bf714 Mon Sep 17 00:00:00 2001 From: Jerop Kipruto Date: Fri, 13 Feb 2026 18:15:21 -0500 Subject: [PATCH] feat(plan): hide plan write and edit operations on plans in Plan Mode (#19012) --- .../messages/ToolGroupMessage.test.tsx | 45 ++++++++- .../components/messages/ToolGroupMessage.tsx | 17 ++-- packages/cli/src/ui/hooks/toolMapping.ts | 1 + packages/cli/src/ui/types.ts | 2 + .../core/src/core/coreToolScheduler.test.ts | 83 ++++++++++++++++ packages/core/src/core/coreToolScheduler.ts | 16 +++- packages/core/src/scheduler/scheduler.test.ts | 14 +++ packages/core/src/scheduler/scheduler.ts | 12 ++- .../core/src/scheduler/state-manager.test.ts | 13 ++- packages/core/src/scheduler/state-manager.ts | 7 ++ packages/core/src/scheduler/types.ts | 8 ++ packages/core/src/tools/ask-user.test.ts | 54 +---------- packages/core/src/tools/ask-user.ts | 32 ------- packages/core/src/tools/edit.ts | 8 +- packages/core/src/tools/glob.ts | 4 +- packages/core/src/tools/read-file.ts | 4 +- packages/core/src/tools/tool-names.ts | 8 +- packages/core/src/tools/write-file.ts | 4 +- packages/core/src/utils/tool-utils.test.ts | 94 ++++++++++++++++++- packages/core/src/utils/tool-utils.ts | 55 +++++++++++ 20 files changed, 373 insertions(+), 108 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx index 43825c3168..deef0cf91f 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx @@ -10,9 +10,14 @@ import { ToolGroupMessage } from './ToolGroupMessage.js'; import type { IndividualToolCallDisplay } from '../../types.js'; import { Scrollable } from '../shared/Scrollable.js'; import { - ASK_USER_DISPLAY_NAME, makeFakeConfig, CoreToolCallStatus, + ApprovalMode, + ASK_USER_DISPLAY_NAME, + WRITE_FILE_DISPLAY_NAME, + EDIT_DISPLAY_NAME, + READ_FILE_DISPLAY_NAME, + GLOB_DISPLAY_NAME, } from '@google/gemini-cli-core'; import os from 'node:os'; @@ -511,4 +516,42 @@ describe('', () => { unmount(); }); }); + + describe('Plan Mode Filtering', () => { + it.each([ + { + name: WRITE_FILE_DISPLAY_NAME, + mode: ApprovalMode.PLAN, + visible: false, + }, + { name: EDIT_DISPLAY_NAME, mode: ApprovalMode.PLAN, visible: false }, + { + name: WRITE_FILE_DISPLAY_NAME, + mode: ApprovalMode.DEFAULT, + visible: true, + }, + { name: READ_FILE_DISPLAY_NAME, mode: ApprovalMode.PLAN, visible: true }, + { name: GLOB_DISPLAY_NAME, mode: ApprovalMode.PLAN, visible: true }, + ])('filtering logic for $name in $mode mode', ({ name, mode, visible }) => { + const toolCalls = [ + createToolCall({ + callId: 'test-call', + name, + approvalMode: mode, + }), + ]; + + const { lastFrame, unmount } = renderWithProviders( + , + { config: baseMockConfig }, + ); + + if (visible) { + expect(lastFrame()).toContain(name); + } else { + expect(lastFrame()).toBe(''); + } + unmount(); + }); + }); }); diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 35e6c33b9a..18179b6a92 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -15,8 +15,8 @@ import { theme } from '../../semantic-colors.js'; import { useConfig } from '../../contexts/ConfigContext.js'; import { isShellTool, isThisShellFocused } from './ToolShared.js'; import { - shouldHideAskUserTool, CoreToolCallStatus, + shouldHideToolCall, } from '@google/gemini-cli-core'; import { ShowMoreLines } from '../ShowMoreLines.js'; import { useUIState } from '../../contexts/UIStateContext.js'; @@ -45,13 +45,18 @@ export const ToolGroupMessage: React.FC = ({ borderTop: borderTopOverride, borderBottom: borderBottomOverride, }) => { - // Filter out Ask User tools that should be hidden (e.g. in-progress or errors without result) + // Filter out tool calls that should be hidden (e.g. in-progress Ask User, or Plan Mode operations). const toolCalls = useMemo( () => - allToolCalls.filter((t) => { - const displayStatus = mapCoreStatusToDisplayStatus(t.status); - return !shouldHideAskUserTool(t.name, displayStatus, !!t.resultDisplay); - }), + allToolCalls.filter( + (t) => + !shouldHideToolCall({ + displayName: t.name, + status: t.status, + approvalMode: t.approvalMode, + hasResultDisplay: !!t.resultDisplay, + }), + ), [allToolCalls], ); diff --git a/packages/cli/src/ui/hooks/toolMapping.ts b/packages/cli/src/ui/hooks/toolMapping.ts index 46b374070d..d921651e51 100644 --- a/packages/cli/src/ui/hooks/toolMapping.ts +++ b/packages/cli/src/ui/hooks/toolMapping.ts @@ -95,6 +95,7 @@ export function mapToDisplay( outputFile, ptyId, correlationId, + approvalMode: call.approvalMode, }; }); diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index 562fcbcb4c..8481cca71f 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -14,6 +14,7 @@ import { type RetrieveUserQuotaResponse, type SkillDefinition, type AgentDefinition, + type ApprovalMode, CoreToolCallStatus, checkExhaustive, } from '@google/gemini-cli-core'; @@ -106,6 +107,7 @@ export interface IndividualToolCallDisplay { ptyId?: number; outputFile?: string; correlationId?: string; + approvalMode?: ApprovalMode; } export interface CompressionProps { diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index b8862b2b31..3c18b3daa2 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -2274,4 +2274,87 @@ describe('CoreToolScheduler Sequential Execution', () => { ); }); }); + + describe('ApprovalMode Preservation', () => { + it('should preserve approvalMode throughout tool lifecycle', async () => { + // Arrange + const executeFn = vi.fn().mockResolvedValue({ + llmContent: 'Tool executed', + returnDisplay: 'Tool executed', + }); + const mockTool = new MockTool({ + name: 'mockTool', + execute: executeFn, + shouldConfirmExecute: MOCK_TOOL_SHOULD_CONFIRM_EXECUTE, + }); + + const mockToolRegistry = { + getTool: () => mockTool, + getAllToolNames: () => ['mockTool'], + } as unknown as ToolRegistry; + + const onAllToolCallsComplete = vi.fn(); + const onToolCallsUpdate = vi.fn(); + + // Set approval mode to PLAN + const mockConfig = createMockConfig({ + getToolRegistry: () => mockToolRegistry, + getApprovalMode: () => ApprovalMode.PLAN, + // Ensure policy engine returns ASK_USER to trigger AwaitingApproval state + getPolicyEngine: () => + ({ + check: async () => ({ decision: PolicyDecision.ASK_USER }), + }) as unknown as PolicyEngine, + }); + mockConfig.getHookSystem = vi.fn().mockReturnValue(undefined); + + const scheduler = new CoreToolScheduler({ + config: mockConfig, + onAllToolCallsComplete, + onToolCallsUpdate, + getPreferredEditor: () => 'vscode', + }); + + const abortController = new AbortController(); + const request = { + callId: '1', + name: 'mockTool', + args: { param: 'value' }, + isClientInitiated: false, + prompt_id: 'test-prompt', + }; + + // Act - Schedule + const schedulePromise = scheduler.schedule( + request, + abortController.signal, + ); + + // Assert - Check AwaitingApproval state + const awaitingCall = (await waitForStatus( + onToolCallsUpdate, + CoreToolCallStatus.AwaitingApproval, + )) as WaitingToolCall; + + expect(awaitingCall).toBeDefined(); + expect(awaitingCall.approvalMode).toBe(ApprovalMode.PLAN); + + // Act - Confirm + + await ( + awaitingCall.confirmationDetails as ToolCallConfirmationDetails + ).onConfirm(ToolConfirmationOutcome.ProceedOnce); + + // Wait for completion + await schedulePromise; + + // Assert - Check Success state + expect(onAllToolCallsComplete).toHaveBeenCalled(); + const completedCalls = onAllToolCallsComplete.mock + .calls[0][0] as ToolCall[]; + expect(completedCalls).toHaveLength(1); + expect(completedCalls[0].status).toBe(CoreToolCallStatus.Success); + expect(completedCalls[0].approvalMode).toBe(ApprovalMode.PLAN); + }); + }); }); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 2c4b18b578..ea2cdb7015 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -217,6 +217,7 @@ export class CoreToolScheduler { const invocation = currentCall.invocation; const outcome = currentCall.outcome; + const approvalMode = currentCall.approvalMode; switch (newStatus) { case CoreToolCallStatus.Success: { @@ -232,6 +233,7 @@ export class CoreToolScheduler { response: auxiliaryData as ToolCallResponseInfo, durationMs, outcome, + approvalMode, } as SuccessfulToolCall; } case CoreToolCallStatus.Error: { @@ -246,6 +248,7 @@ export class CoreToolScheduler { response: auxiliaryData as ToolCallResponseInfo, durationMs, outcome, + approvalMode, } as ErroredToolCall; } case CoreToolCallStatus.AwaitingApproval: @@ -259,6 +262,7 @@ export class CoreToolScheduler { startTime: existingStartTime, outcome, invocation, + approvalMode, } as WaitingToolCall; case CoreToolCallStatus.Scheduled: return { @@ -268,6 +272,7 @@ export class CoreToolScheduler { startTime: existingStartTime, outcome, invocation, + approvalMode, } as ScheduledToolCall; case CoreToolCallStatus.Cancelled: { const durationMs = existingStartTime @@ -316,6 +321,7 @@ export class CoreToolScheduler { }, durationMs, outcome, + approvalMode, } as CancelledToolCall; } case CoreToolCallStatus.Validating: @@ -326,6 +332,7 @@ export class CoreToolScheduler { startTime: existingStartTime, outcome, invocation, + approvalMode, } as ValidatingToolCall; case CoreToolCallStatus.Executing: return { @@ -335,6 +342,7 @@ export class CoreToolScheduler { startTime: existingStartTime, outcome, invocation, + approvalMode, } as ExecutingToolCall; default: { const exhaustiveCheck: never = newStatus; @@ -373,6 +381,7 @@ export class CoreToolScheduler { status: CoreToolCallStatus.Error, tool: call.tool, response, + approvalMode: call.approvalMode, } as ErroredToolCall; } @@ -496,6 +505,7 @@ export class CoreToolScheduler { ); } const requestsToProcess = Array.isArray(request) ? request : [request]; + const currentApprovalMode = this.config.getApprovalMode(); this.completedToolCallsForBatch = []; const newToolCalls: ToolCall[] = requestsToProcess.map( @@ -518,6 +528,7 @@ export class CoreToolScheduler { ToolErrorType.TOOL_NOT_REGISTERED, ), durationMs: 0, + approvalMode: currentApprovalMode, }; } @@ -536,6 +547,7 @@ export class CoreToolScheduler { ToolErrorType.INVALID_TOOL_PARAMS, ), durationMs: 0, + approvalMode: currentApprovalMode, }; } @@ -545,6 +557,7 @@ export class CoreToolScheduler { tool: toolInstance, invocation: invocationOrError, startTime: Date.now(), + approvalMode: currentApprovalMode, }; }, ); @@ -920,7 +933,7 @@ export class CoreToolScheduler { this.toolCalls = this.toolCalls.map((tc) => tc.request.callId === completedCall.request.callId - ? completedCall + ? { ...completedCall, approvalMode: tc.approvalMode } : tc, ); this.notifyToolCallsUpdate(); @@ -1049,6 +1062,7 @@ export class CoreToolScheduler { }, durationMs, outcome: ToolConfirmationOutcome.Cancel, + approvalMode: queuedCall.approvalMode, }); } } diff --git a/packages/core/src/scheduler/scheduler.test.ts b/packages/core/src/scheduler/scheduler.test.ts index 222b4993b9..ad2d094b4e 100644 --- a/packages/core/src/scheduler/scheduler.test.ts +++ b/packages/core/src/scheduler/scheduler.test.ts @@ -320,6 +320,20 @@ describe('Scheduler (Orchestrator)', () => { ]), ); }); + + it('should set approvalMode to PLAN when config returns PLAN', async () => { + mockConfig.getApprovalMode.mockReturnValue(ApprovalMode.PLAN); + await scheduler.schedule(req1, signal); + + expect(mockStateManager.enqueue).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + status: CoreToolCallStatus.Validating, + approvalMode: ApprovalMode.PLAN, + }), + ]), + ); + }); }); describe('Phase 2: Queue Management', () => { diff --git a/packages/core/src/scheduler/scheduler.ts b/packages/core/src/scheduler/scheduler.ts index a8c68290f1..b177fe0318 100644 --- a/packages/core/src/scheduler/scheduler.ts +++ b/packages/core/src/scheduler/scheduler.ts @@ -22,6 +22,7 @@ import { CoreToolCallStatus, } from './types.js'; import { ToolErrorType } from '../tools/tool-error.js'; +import type { ApprovalMode } from '../policy/types.js'; import { PolicyDecision } from '../policy/types.js'; import { ToolConfirmationOutcome, @@ -243,6 +244,7 @@ export class Scheduler { this.isProcessing = true; this.isCancelling = false; this.state.clearBatch(); + const currentApprovalMode = this.config.getApprovalMode(); try { const toolRegistry = this.config.getToolRegistry(); @@ -260,10 +262,15 @@ export class Scheduler { enrichedRequest, toolRegistry.getAllToolNames(), ), + approvalMode: currentApprovalMode, }; } - return this._validateAndCreateToolCall(enrichedRequest, tool); + return this._validateAndCreateToolCall( + enrichedRequest, + tool, + currentApprovalMode, + ); }); this.state.enqueue(newCalls); @@ -297,6 +304,7 @@ export class Scheduler { private _validateAndCreateToolCall( request: ToolCallRequestInfo, tool: AnyDeclarativeTool, + approvalMode: ApprovalMode, ): ValidatingToolCall | ErroredToolCall { return runWithToolCallContext( { @@ -314,6 +322,7 @@ export class Scheduler { invocation, startTime: Date.now(), schedulerId: this.schedulerId, + approvalMode, }; } catch (e) { return { @@ -327,6 +336,7 @@ export class Scheduler { ), durationMs: 0, schedulerId: this.schedulerId, + approvalMode, }; } }, diff --git a/packages/core/src/scheduler/state-manager.test.ts b/packages/core/src/scheduler/state-manager.test.ts index 9314d70f41..758ff354c0 100644 --- a/packages/core/src/scheduler/state-manager.test.ts +++ b/packages/core/src/scheduler/state-manager.test.ts @@ -24,6 +24,7 @@ import { } from '../tools/tools.js'; import { MessageBusType } from '../confirmation-bus/types.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; +import { ApprovalMode } from '../policy/types.js'; describe('SchedulerStateManager', () => { const mockRequest: ToolCallRequestInfo = { @@ -43,12 +44,16 @@ describe('SchedulerStateManager', () => { shouldConfirmExecute: vi.fn(), } as unknown as AnyToolInvocation; - const createValidatingCall = (id = 'call-1'): ValidatingToolCall => ({ + const createValidatingCall = ( + id = 'call-1', + mode: ApprovalMode = ApprovalMode.DEFAULT, + ): ValidatingToolCall => ({ status: CoreToolCallStatus.Validating, request: { ...mockRequest, callId: id }, tool: mockTool, invocation: mockInvocation, startTime: Date.now(), + approvalMode: mode, }); const createMockResponse = (id: string): ToolCallResponseInfo => ({ @@ -208,7 +213,7 @@ describe('SchedulerStateManager', () => { describe('Status Transitions', () => { it('should transition validating to scheduled', () => { - const call = createValidatingCall(); + const call = createValidatingCall('call-1', ApprovalMode.PLAN); stateManager.enqueue([call]); stateManager.dequeue(); @@ -220,6 +225,7 @@ describe('SchedulerStateManager', () => { const snapshot = stateManager.getSnapshot(); expect(snapshot[0].status).toBe(CoreToolCallStatus.Scheduled); expect(snapshot[0].request.callId).toBe(call.request.callId); + expect(snapshot[0].approvalMode).toBe(ApprovalMode.PLAN); }); it('should transition scheduled to executing', () => { @@ -242,7 +248,7 @@ describe('SchedulerStateManager', () => { }); it('should transition to success and move to completed batch', () => { - const call = createValidatingCall(); + const call = createValidatingCall('call-1', ApprovalMode.PLAN); stateManager.enqueue([call]); stateManager.dequeue(); @@ -272,6 +278,7 @@ describe('SchedulerStateManager', () => { expect(completed.status).toBe(CoreToolCallStatus.Success); expect(completed.response).toEqual(response); expect(completed.durationMs).toBeDefined(); + expect(completed.approvalMode).toBe(ApprovalMode.PLAN); }); it('should transition to error and move to completed batch', () => { diff --git a/packages/core/src/scheduler/state-manager.ts b/packages/core/src/scheduler/state-manager.ts index 170873bc83..6a473ad47c 100644 --- a/packages/core/src/scheduler/state-manager.ts +++ b/packages/core/src/scheduler/state-manager.ts @@ -350,6 +350,7 @@ export class SchedulerStateManager { durationMs: startTime ? Date.now() - startTime : undefined, outcome: call.outcome, schedulerId: call.schedulerId, + approvalMode: call.approvalMode, }; } @@ -366,6 +367,7 @@ export class SchedulerStateManager { durationMs: startTime ? Date.now() - startTime : undefined, outcome: call.outcome, schedulerId: call.schedulerId, + approvalMode: call.approvalMode, }; } @@ -399,6 +401,7 @@ export class SchedulerStateManager { outcome: call.outcome, invocation: call.invocation, schedulerId: call.schedulerId, + approvalMode: call.approvalMode, }; } @@ -424,6 +427,7 @@ export class SchedulerStateManager { outcome: call.outcome, invocation: call.invocation, schedulerId: call.schedulerId, + approvalMode: call.approvalMode, }; } @@ -479,6 +483,7 @@ export class SchedulerStateManager { durationMs: startTime ? Date.now() - startTime : undefined, outcome: call.outcome, schedulerId: call.schedulerId, + approvalMode: call.approvalMode, }; } @@ -500,6 +505,7 @@ export class SchedulerStateManager { outcome: call.outcome, invocation: call.invocation, schedulerId: call.schedulerId, + approvalMode: call.approvalMode, }; } @@ -522,6 +528,7 @@ export class SchedulerStateManager { liveOutput, pid, schedulerId: call.schedulerId, + approvalMode: call.approvalMode, }; } } diff --git a/packages/core/src/scheduler/types.ts b/packages/core/src/scheduler/types.ts index c63d6d3c8e..b09c42fe51 100644 --- a/packages/core/src/scheduler/types.ts +++ b/packages/core/src/scheduler/types.ts @@ -15,6 +15,7 @@ import type { import type { AnsiOutput } from '../utils/terminalSerializer.js'; import type { ToolErrorType } from '../tools/tool-error.js'; import type { SerializableConfirmationDetails } from '../confirmation-bus/types.js'; +import { type ApprovalMode } from '../policy/types.js'; export const ROOT_SCHEDULER_ID = 'root'; @@ -65,6 +66,7 @@ export type ValidatingToolCall = { startTime?: number; outcome?: ToolConfirmationOutcome; schedulerId?: string; + approvalMode?: ApprovalMode; }; export type ScheduledToolCall = { @@ -75,6 +77,7 @@ export type ScheduledToolCall = { startTime?: number; outcome?: ToolConfirmationOutcome; schedulerId?: string; + approvalMode?: ApprovalMode; }; export type ErroredToolCall = { @@ -85,6 +88,7 @@ export type ErroredToolCall = { durationMs?: number; outcome?: ToolConfirmationOutcome; schedulerId?: string; + approvalMode?: ApprovalMode; }; export type SuccessfulToolCall = { @@ -96,6 +100,7 @@ export type SuccessfulToolCall = { durationMs?: number; outcome?: ToolConfirmationOutcome; schedulerId?: string; + approvalMode?: ApprovalMode; }; export type ExecutingToolCall = { @@ -108,6 +113,7 @@ export type ExecutingToolCall = { outcome?: ToolConfirmationOutcome; pid?: number; schedulerId?: string; + approvalMode?: ApprovalMode; }; export type CancelledToolCall = { @@ -119,6 +125,7 @@ export type CancelledToolCall = { durationMs?: number; outcome?: ToolConfirmationOutcome; schedulerId?: string; + approvalMode?: ApprovalMode; }; export type WaitingToolCall = { @@ -141,6 +148,7 @@ export type WaitingToolCall = { startTime?: number; outcome?: ToolConfirmationOutcome; schedulerId?: string; + approvalMode?: ApprovalMode; }; export type Status = ToolCall['status']; diff --git a/packages/core/src/tools/ask-user.test.ts b/packages/core/src/tools/ask-user.test.ts index 0273e2fc0d..df206890c3 100644 --- a/packages/core/src/tools/ask-user.test.ts +++ b/packages/core/src/tools/ask-user.test.ts @@ -5,11 +5,7 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { - AskUserTool, - shouldHideAskUserTool, - isCompletedAskUserTool, -} from './ask-user.js'; +import { AskUserTool, 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'; @@ -17,54 +13,6 @@ 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); diff --git a/packages/core/src/tools/ask-user.ts b/packages/core/src/tools/ask-user.ts index f941af5d4c..78e518afb1 100644 --- a/packages/core/src/tools/ask-user.ts +++ b/packages/core/src/tools/ask-user.ts @@ -259,38 +259,6 @@ 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. */ diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 2e79ebcb6b..41f895f5cd 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -43,7 +43,11 @@ import { EditCorrectionEvent } from '../telemetry/types.js'; import { logEditCorrectionEvent } from '../telemetry/loggers.js'; import { correctPath } from '../utils/pathCorrector.js'; -import { EDIT_TOOL_NAME, READ_FILE_TOOL_NAME } from './tool-names.js'; +import { + EDIT_TOOL_NAME, + READ_FILE_TOOL_NAME, + EDIT_DISPLAY_NAME, +} from './tool-names.js'; import { debugLogger } from '../utils/debugLogger.js'; import { EDIT_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; @@ -908,7 +912,7 @@ export class EditTool ) { super( EditTool.Name, - 'Edit', + EDIT_DISPLAY_NAME, EDIT_DEFINITION.base.description!, Kind.Edit, EDIT_DEFINITION.base.parametersJsonSchema, diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index ea1ec994e5..78b445e762 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -14,7 +14,7 @@ import { shortenPath, makeRelative } from '../utils/paths.js'; import { type Config } from '../config/config.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; import { ToolErrorType } from './tool-error.js'; -import { GLOB_TOOL_NAME } from './tool-names.js'; +import { GLOB_TOOL_NAME, GLOB_DISPLAY_NAME } from './tool-names.js'; import { getErrorMessage } from '../utils/errors.js'; import { debugLogger } from '../utils/debugLogger.js'; import { GLOB_DEFINITION } from './definitions/coreTools.js'; @@ -271,7 +271,7 @@ export class GlobTool extends BaseDeclarativeTool { ) { super( GlobTool.Name, - 'FindFiles', + GLOB_DISPLAY_NAME, GLOB_DEFINITION.base.description!, Kind.Search, GLOB_DEFINITION.base.parametersJsonSchema, diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 62209c4d2e..c43f79ded0 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -21,7 +21,7 @@ import { FileOperation } from '../telemetry/metrics.js'; import { getProgrammingLanguage } from '../telemetry/telemetry-utils.js'; import { logFileOperation } from '../telemetry/loggers.js'; import { FileOperationEvent } from '../telemetry/types.js'; -import { READ_FILE_TOOL_NAME } from './tool-names.js'; +import { READ_FILE_TOOL_NAME, READ_FILE_DISPLAY_NAME } from './tool-names.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import { READ_FILE_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; @@ -173,7 +173,7 @@ export class ReadFileTool extends BaseDeclarativeTool< ) { super( ReadFileTool.Name, - 'ReadFile', + READ_FILE_DISPLAY_NAME, READ_FILE_DEFINITION.base.description!, Kind.Read, READ_FILE_DEFINITION.base.parametersJsonSchema, diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index 88041ec7fe..191d667ea3 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -40,10 +40,16 @@ export const GET_INTERNAL_DOCS_TOOL_NAME = 'get_internal_docs'; export const ACTIVATE_SKILL_TOOL_NAME = 'activate_skill'; export const EDIT_TOOL_NAMES = new Set([EDIT_TOOL_NAME, WRITE_FILE_TOOL_NAME]); export const ASK_USER_TOOL_NAME = 'ask_user'; -export const ASK_USER_DISPLAY_NAME = 'Ask User'; export const EXIT_PLAN_MODE_TOOL_NAME = 'exit_plan_mode'; export const ENTER_PLAN_MODE_TOOL_NAME = 'enter_plan_mode'; +// Tool Display Names +export const WRITE_FILE_DISPLAY_NAME = 'WriteFile'; +export const EDIT_DISPLAY_NAME = 'Edit'; +export const ASK_USER_DISPLAY_NAME = 'Ask User'; +export const READ_FILE_DISPLAY_NAME = 'ReadFile'; +export const GLOB_DISPLAY_NAME = 'FindFiles'; + /** * Mapping of legacy tool names to their current names. * This ensures backward compatibility for user-defined policies, skills, and hooks. diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 467bee663e..9f4abfdf55 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -9,7 +9,7 @@ import fsPromises from 'node:fs/promises'; import path from 'node:path'; import os from 'node:os'; import * as Diff from 'diff'; -import { WRITE_FILE_TOOL_NAME } from './tool-names.js'; +import { WRITE_FILE_TOOL_NAME, WRITE_FILE_DISPLAY_NAME } from './tool-names.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../policy/types.js'; @@ -446,7 +446,7 @@ export class WriteFileTool ) { super( WriteFileTool.Name, - 'WriteFile', + WRITE_FILE_DISPLAY_NAME, WRITE_FILE_DEFINITION.base.description!, Kind.Edit, WRITE_FILE_DEFINITION.base.parametersJsonSchema, diff --git a/packages/core/src/utils/tool-utils.test.ts b/packages/core/src/utils/tool-utils.test.ts index 0ded23f2e3..225889d53a 100644 --- a/packages/core/src/utils/tool-utils.test.ts +++ b/packages/core/src/utils/tool-utils.test.ts @@ -5,11 +5,101 @@ */ import { expect, describe, it } from 'vitest'; -import { doesToolInvocationMatch, getToolSuggestion } from './tool-utils.js'; +import { + doesToolInvocationMatch, + getToolSuggestion, + shouldHideToolCall, +} from './tool-utils.js'; import type { AnyToolInvocation, Config } from '../index.js'; -import { ReadFileTool } from '../index.js'; +import { + ReadFileTool, + ApprovalMode, + CoreToolCallStatus, + ASK_USER_DISPLAY_NAME, + WRITE_FILE_DISPLAY_NAME, + EDIT_DISPLAY_NAME, + READ_FILE_DISPLAY_NAME, +} from '../index.js'; import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; +describe('shouldHideToolCall', () => { + it.each([ + { + status: CoreToolCallStatus.Scheduled, + hasResult: true, + shouldHide: true, + }, + { + status: CoreToolCallStatus.Executing, + hasResult: true, + shouldHide: true, + }, + { + status: CoreToolCallStatus.AwaitingApproval, + hasResult: true, + shouldHide: true, + }, + { + status: CoreToolCallStatus.Validating, + hasResult: true, + shouldHide: true, + }, + { + status: CoreToolCallStatus.Success, + hasResult: true, + shouldHide: false, + }, + { + status: CoreToolCallStatus.Error, + hasResult: false, + shouldHide: true, + }, + { + status: CoreToolCallStatus.Error, + hasResult: true, + shouldHide: false, + }, + ])( + 'AskUser: status=$status, hasResult=$hasResult -> hide=$shouldHide', + ({ status, hasResult, shouldHide }) => { + expect( + shouldHideToolCall({ + displayName: ASK_USER_DISPLAY_NAME, + status, + hasResultDisplay: hasResult, + }), + ).toBe(shouldHide); + }, + ); + + it.each([ + { + name: WRITE_FILE_DISPLAY_NAME, + mode: ApprovalMode.PLAN, + visible: false, + }, + { name: EDIT_DISPLAY_NAME, mode: ApprovalMode.PLAN, visible: false }, + { + name: WRITE_FILE_DISPLAY_NAME, + mode: ApprovalMode.DEFAULT, + visible: true, + }, + { name: READ_FILE_DISPLAY_NAME, mode: ApprovalMode.PLAN, visible: true }, + ])( + 'Plan Mode: tool=$name, mode=$mode -> visible=$visible', + ({ name, mode, visible }) => { + expect( + shouldHideToolCall({ + displayName: name, + status: CoreToolCallStatus.Success, + approvalMode: mode, + hasResultDisplay: true, + }), + ).toBe(!visible); + }, + ); +}); + describe('getToolSuggestion', () => { it('should suggest the top N closest tool names for a typo', () => { const allToolNames = ['list_files', 'read_file', 'write_file']; diff --git a/packages/core/src/utils/tool-utils.ts b/packages/core/src/utils/tool-utils.ts index ed9c11f34e..b8e60fe4ce 100644 --- a/packages/core/src/utils/tool-utils.ts +++ b/packages/core/src/utils/tool-utils.ts @@ -8,6 +8,61 @@ import type { AnyDeclarativeTool, AnyToolInvocation } from '../index.js'; import { isTool } from '../index.js'; import { SHELL_TOOL_NAMES } from './shell-utils.js'; import levenshtein from 'fast-levenshtein'; +import { ApprovalMode } from '../policy/types.js'; +import { CoreToolCallStatus } from '../scheduler/types.js'; +import { + ASK_USER_DISPLAY_NAME, + WRITE_FILE_DISPLAY_NAME, + EDIT_DISPLAY_NAME, +} from '../tools/tool-names.js'; + +/** + * Options for determining if a tool call should be hidden in the CLI history. + */ +export interface ShouldHideToolCallParams { + /** The display name of the tool. */ + displayName: string; + /** The current status of the tool call. */ + status: CoreToolCallStatus; + /** The approval mode active when the tool was called. */ + approvalMode?: ApprovalMode; + /** Whether the tool has produced a result for display. */ + hasResultDisplay: boolean; +} + +/** + * Determines if a tool call should be hidden from the standard tool history UI. + * + * We hide tools in several cases: + * 1. Ask User tools that are in progress, displayed via specialized UI. + * 2. Ask User tools that errored without result display, typically param + * validation errors that the agent automatically recovers from. + * 3. WriteFile and Edit tools when in Plan Mode, redundant because the + * resulting plans are displayed separately upon exiting plan mode. + */ +export function shouldHideToolCall(params: ShouldHideToolCallParams): boolean { + const { displayName, status, approvalMode, hasResultDisplay } = params; + + switch (displayName) { + case ASK_USER_DISPLAY_NAME: + switch (status) { + case CoreToolCallStatus.Scheduled: + case CoreToolCallStatus.Validating: + case CoreToolCallStatus.Executing: + case CoreToolCallStatus.AwaitingApproval: + return true; + case CoreToolCallStatus.Error: + return !hasResultDisplay; + default: + return false; + } + case WRITE_FILE_DISPLAY_NAME: + case EDIT_DISPLAY_NAME: + return approvalMode === ApprovalMode.PLAN; + default: + return false; + } +} /** * Generates a suggestion string for a tool name that was not found in the registry.