From d78334c95fbc70e0c324c0d9e401dcd034ccd2e2 Mon Sep 17 00:00:00 2001 From: Jarrod Whelan <150866123+jwhelangoog@users.noreply.github.com> Date: Fri, 20 Mar 2026 11:30:52 -0700 Subject: [PATCH] refactor(cli,core): foundational layout, identity management, and type safety This commit establishes the structural foundation and required infrastructure to support the upcoming compact tool output changes. It includes identity management improvements, layout fixes, and type-safety enhancements that stand independently. 1. Identity & History Management: - useHistoryManager: Ensure strictly increasing IDs for history items, even with identical timestamps. - acpClient: Introduced callIdCounter to prevent callId collisions during rapid execution. - MainContent: Implemented negative ID logic for pending items to ensure stable React keys and prevent collisions. - historyUtils: New file containing extracted history logic (isToolExecuting, getAllToolCalls) hoisted from AppContainer. 2. UI Infrastructure & Layout: - test-utils/render: Modernized renderWithProviders and removed legacy props. - AskUserDialog: Fixed layout, scroll visibility, and alignment issues. - toolLayoutUtils: Increased TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT for better spacing. - ShellToolMessage/ToolGroupMessage: Updated line-count expectations and snapshots to align with layout changes. 3. IDE & Diffing Improvements: - ToolActionsContext: Refactored IdeClient initialization to fix a race condition and potential memory leak. - edit/diffOptions: Improved accuracy of diff stat derivation, ensuring "full content" stats are available for the model. - coreToolScheduler: Automatically derive diff stats from patches if missing. - state-manager: Ensure diffStat preservation for rejected tool calls. 4. Type Safety & Constants: - types/tools: Added foundational types like DiffStat, FileDiff, and StructuredToolResult. - Type Guards: Added guards for isFileDiff, isTodoList, isAnsiOutput, and hasSummary. - CodeColorizer: Added function overloads to gracefully handle null language detection. - tool-names: Introduced DISPLAY_NAME constants for consistent tool labeling. This commit passes all workspace tests and builds successfully. Feature-specific logic for compact output is excluded. --- packages/cli/src/acp/acpClient.ts | 6 +- packages/cli/src/test-utils/render.tsx | 39 ++++++-- packages/cli/src/ui/AppContainer.tsx | 84 +++++----------- .../src/ui/components/AskUserDialog.test.tsx | 2 +- .../cli/src/ui/components/AskUserDialog.tsx | 10 +- .../src/ui/components/MainContent.test.tsx | 12 +-- .../cli/src/ui/components/MainContent.tsx | 12 ++- .../__snapshots__/AskUserDialog.test.tsx.snap | 27 +----- .../__snapshots__/MainContent.test.tsx.snap | 9 +- .../messages/ShellToolMessage.test.tsx | 12 +-- .../messages/ToolConfirmationMessage.test.tsx | 9 +- .../ShellToolMessage.test.tsx.snap | 15 +-- .../ToolResultDisplay.test.tsx.snap | 3 +- .../ui/contexts/ToolActionsContext.test.tsx | 23 +++-- .../src/ui/contexts/ToolActionsContext.tsx | 40 ++++---- .../src/ui/hooks/useHistoryManager.test.ts | 54 ++++++++++- .../cli/src/ui/hooks/useHistoryManager.ts | 16 +++- packages/cli/src/ui/types.ts | 95 ++++++++++++++++++- .../cli/src/ui/utils/CodeColorizer.test.tsx | 24 +++++ packages/cli/src/ui/utils/CodeColorizer.tsx | 56 ++++++++--- packages/cli/src/ui/utils/confirmingTool.ts | 6 +- packages/cli/src/ui/utils/historyUtils.ts | 83 ++++++++++++++++ .../cli/src/ui/utils/toolLayoutUtils.test.ts | 20 ++-- packages/cli/src/ui/utils/toolLayoutUtils.ts | 2 +- packages/core/src/confirmation-bus/types.ts | 2 + packages/core/src/core/coreToolScheduler.ts | 7 ++ .../core/src/scheduler/state-manager.test.ts | 24 ++--- packages/core/src/scheduler/state-manager.ts | 3 + packages/core/src/tools/diffOptions.ts | 36 +++++++ packages/core/src/tools/edit.ts | 24 ++++- packages/core/src/tools/tool-names.ts | 7 +- packages/core/src/tools/tools.ts | 16 ++++ packages/core/src/tools/web-fetch.ts | 4 +- packages/core/src/tools/web-search.ts | 4 +- 34 files changed, 566 insertions(+), 220 deletions(-) create mode 100644 packages/cli/src/ui/utils/historyUtils.ts diff --git a/packages/cli/src/acp/acpClient.ts b/packages/cli/src/acp/acpClient.ts index 5e3f3666b1..7cbfdc0d61 100644 --- a/packages/cli/src/acp/acpClient.ts +++ b/packages/cli/src/acp/acpClient.ts @@ -97,6 +97,8 @@ export async function runAcpClient( await connection.closed.finally(runExitCleanup); } +let callIdCounter = 0; + export class GeminiAgent { private sessions: Map = new Map(); private clientCapabilities: acp.ClientCapabilities | undefined; @@ -897,7 +899,7 @@ export class Session { promptId: string, fc: FunctionCall, ): Promise { - const callId = fc.id ?? `${fc.name}-${Date.now()}`; + const callId = fc.id ?? `${fc.name}-${Date.now()}-${++callIdCounter}`; const args = fc.args ?? {}; const startTime = Date.now(); @@ -1370,7 +1372,7 @@ export class Session { include: pathSpecsToRead, }; - const callId = `${readManyFilesTool.name}-${Date.now()}`; + const callId = `${readManyFilesTool.name}-${Date.now()}-${++callIdCounter}`; try { const invocation = readManyFilesTool.build(toolArgs); diff --git a/packages/cli/src/test-utils/render.tsx b/packages/cli/src/test-utils/render.tsx index 7d298b120d..a20a60ed5e 100644 --- a/packages/cli/src/test-utils/render.tsx +++ b/packages/cli/src/test-utils/render.tsx @@ -16,7 +16,7 @@ import { vi } from 'vitest'; import stripAnsi from 'strip-ansi'; import type React from 'react'; import { act, useState } from 'react'; -import type { LoadedSettings } from '../config/settings.js'; +import { LoadedSettings } from '../config/settings.js'; import { KeypressProvider } from '../ui/contexts/KeypressContext.js'; import { SettingsContext } from '../ui/contexts/SettingsContext.js'; import { ShellFocusContext } from '../ui/contexts/ShellFocusContext.js'; @@ -604,16 +604,18 @@ export const renderWithProviders = async ( uiState: providedUiState, width, mouseEventsEnabled = false, + useAlternateBuffer: explicitUseAlternateBuffer, config, uiActions, persistentState, appState = mockAppState, }: { shellFocus?: boolean; - settings?: LoadedSettings; + settings?: LoadedSettings | Partial; uiState?: Partial; width?: number; mouseEventsEnabled?: boolean; + useAlternateBuffer?: boolean; config?: Config; uiActions?: Partial; persistentState?: { @@ -660,15 +662,34 @@ export const renderWithProviders = async ( const terminalWidth = width ?? baseState.terminalWidth; + const finalSettings = + settings instanceof LoadedSettings + ? settings + : createMockSettings(settings || {}); + if (!config) { config = await loadCliConfig( - settings.merged, + finalSettings.merged, 'random-session-id', - {} as unknown as CliArgs, + {} as CliArgs, { cwd: '/' }, ); } + const useAlternateBuffer = + explicitUseAlternateBuffer ?? + finalSettings.merged.ui?.useAlternateBuffer ?? + false; + + const finalConfig = new Proxy(config, { + get(target, prop) { + if (prop === 'getUseAlternateBuffer') { + return () => useAlternateBuffer; + } + return Reflect.get(target, prop); + }, + }); + const mainAreaWidth = terminalWidth; const finalUiState = { @@ -697,8 +718,8 @@ export const renderWithProviders = async ( const wrapWithProviders = (comp: React.ReactElement) => ( - - + + @@ -709,7 +730,7 @@ export const renderWithProviders = async ( ( wrapper?: React.ComponentType<{ children: React.ReactNode }>; // Options for renderWithProviders shellFocus?: boolean; - settings?: LoadedSettings; + settings?: LoadedSettings | Partial; uiState?: Partial; width?: number; mouseEventsEnabled?: boolean; @@ -864,7 +885,7 @@ export async function renderHookWithProviders( const Wrapper = options.wrapper || (({ children }) => <>{children}); - let renderResult: ReturnType; + let renderResult: Awaited>; await act(async () => { renderResult = await renderWithProviders( diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 9d05f54347..a6f2d1d99a 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -30,8 +30,6 @@ import { import { ConfigContext } from './contexts/ConfigContext.js'; import { type HistoryItem, - type HistoryItemWithoutId, - type HistoryItemToolGroup, AuthState, type ConfirmationRequest, type PermissionConfirmationRequest, @@ -81,7 +79,6 @@ import { type AgentsDiscoveredPayload, ChangeAuthRequestedError, ProjectIdRequiredError, - CoreToolCallStatus, buildUserSteeringHintPrompt, logBillingEvent, ApiKeyUpdatedEvent, @@ -170,29 +167,11 @@ import { useIsHelpDismissKey } from './utils/shortcutsHelp.js'; import { useSuspend } from './hooks/useSuspend.js'; import { useRunEventNotifications } from './hooks/useRunEventNotifications.js'; import { isNotificationsEnabled } from '../utils/terminalNotifications.js'; - -function isToolExecuting(pendingHistoryItems: HistoryItemWithoutId[]) { - return pendingHistoryItems.some((item) => { - if (item && item.type === 'tool_group') { - return item.tools.some( - (tool) => CoreToolCallStatus.Executing === tool.status, - ); - } - return false; - }); -} - -function isToolAwaitingConfirmation( - pendingHistoryItems: HistoryItemWithoutId[], -) { - return pendingHistoryItems - .filter((item): item is HistoryItemToolGroup => item.type === 'tool_group') - .some((item) => - item.tools.some( - (tool) => CoreToolCallStatus.AwaitingApproval === tool.status, - ), - ); -} +import { + isToolExecuting, + isToolAwaitingConfirmation, + getAllToolCalls, +} from './utils/historyUtils.js'; interface AppContainerProps { config: Config; @@ -1151,6 +1130,16 @@ Logging in with Google... Restarting Gemini CLI to continue. consumePendingHints, ); + const pendingHistoryItems = useMemo( + () => [...pendingSlashCommandHistoryItems, ...pendingGeminiHistoryItems], + [pendingSlashCommandHistoryItems, pendingGeminiHistoryItems], + ); + + const hasPendingToolConfirmation = useMemo( + () => isToolAwaitingConfirmation(pendingHistoryItems), + [pendingHistoryItems], + ); + toggleBackgroundShellRef.current = toggleBackgroundShell; isBackgroundShellVisibleRef.current = isBackgroundShellVisible; backgroundShellsRef.current = backgroundShells; @@ -1222,10 +1211,6 @@ Logging in with Google... Restarting Gemini CLI to continue. cancelHandlerRef.current = useCallback( (shouldRestorePrompt: boolean = true) => { - const pendingHistoryItems = [ - ...pendingSlashCommandHistoryItems, - ...pendingGeminiHistoryItems, - ]; if (isToolAwaitingConfirmation(pendingHistoryItems)) { return; // Don't clear - user may be composing a follow-up message } @@ -1259,8 +1244,7 @@ Logging in with Google... Restarting Gemini CLI to continue. inputHistory, getQueuedMessagesText, clearQueue, - pendingSlashCommandHistoryItems, - pendingGeminiHistoryItems, + pendingHistoryItems, ], ); @@ -1296,10 +1280,7 @@ Logging in with Google... Restarting Gemini CLI to continue. const isIdle = streamingState === StreamingState.Idle; const isAgentRunning = streamingState === StreamingState.Responding || - isToolExecuting([ - ...pendingSlashCommandHistoryItems, - ...pendingGeminiHistoryItems, - ]); + isToolExecuting(pendingHistoryItems); if (isSlash && isAgentRunning) { const { commandToExecute } = parseSlashCommand( @@ -1361,8 +1342,7 @@ Logging in with Google... Restarting Gemini CLI to continue. isMcpReady, streamingState, messageQueue.length, - pendingSlashCommandHistoryItems, - pendingGeminiHistoryItems, + pendingHistoryItems, config, constrainHeight, setConstrainHeight, @@ -1684,6 +1664,11 @@ Logging in with Google... Restarting Gemini CLI to continue. const handleGlobalKeypress = useCallback( (key: Key): boolean => { + // Debug log keystrokes if enabled + if (settings.merged.general.debugKeystrokeLogging) { + debugLogger.log('[DEBUG] Keystroke:', JSON.stringify(key)); + } + if (shortcutsHelpVisible && isHelpDismissKey(key)) { setShortcutsHelpVisible(false); } @@ -1862,6 +1847,7 @@ Logging in with Google... Restarting Gemini CLI to continue. activePtyId, handleSuspend, embeddedShellFocused, + settings.merged.general.debugKeystrokeLogging, refreshStatic, setCopyModeEnabled, tabFocusTimeoutRef, @@ -2022,16 +2008,6 @@ Logging in with Google... Restarting Gemini CLI to continue. authState === AuthState.AwaitingApiKeyInput || !!newAgents; - const pendingHistoryItems = useMemo( - () => [...pendingSlashCommandHistoryItems, ...pendingGeminiHistoryItems], - [pendingSlashCommandHistoryItems, pendingGeminiHistoryItems], - ); - - const hasPendingToolConfirmation = useMemo( - () => isToolAwaitingConfirmation(pendingHistoryItems), - [pendingHistoryItems], - ); - const hasConfirmUpdateExtensionRequests = confirmUpdateExtensionRequests.length > 0; const hasLoopDetectionConfirmationRequest = @@ -2121,12 +2097,7 @@ Logging in with Google... Restarting Gemini CLI to continue. ]); const allToolCalls = useMemo( - () => - pendingHistoryItems - .filter( - (item): item is HistoryItemToolGroup => item.type === 'tool_group', - ) - .flatMap((item) => item.tools), + () => getAllToolCalls(pendingHistoryItems), [pendingHistoryItems], ); @@ -2292,10 +2263,7 @@ Logging in with Google... Restarting Gemini CLI to continue. showIsExpandableHint, hintMode: config.isModelSteeringEnabled() && - isToolExecuting([ - ...pendingSlashCommandHistoryItems, - ...pendingGeminiHistoryItems, - ]), + isToolExecuting(pendingHistoryItems), hintBuffer: '', }), [ diff --git a/packages/cli/src/ui/components/AskUserDialog.test.tsx b/packages/cli/src/ui/components/AskUserDialog.test.tsx index 8ed240389c..8369f4ec49 100644 --- a/packages/cli/src/ui/components/AskUserDialog.test.tsx +++ b/packages/cli/src/ui/components/AskUserDialog.test.tsx @@ -288,7 +288,7 @@ describe('AskUserDialog', () => { }); describe.each([ - { useAlternateBuffer: true, expectedArrows: false }, + { useAlternateBuffer: true, expectedArrows: true }, { useAlternateBuffer: false, expectedArrows: true }, ])( 'Scroll Arrows (useAlternateBuffer: $useAlternateBuffer)', diff --git a/packages/cli/src/ui/components/AskUserDialog.tsx b/packages/cli/src/ui/components/AskUserDialog.tsx index b1d23885e6..f845c3dcfd 100644 --- a/packages/cli/src/ui/components/AskUserDialog.tsx +++ b/packages/cli/src/ui/components/AskUserDialog.tsx @@ -857,8 +857,14 @@ const ChoiceQuestionView: React.FC = ({ : undefined; const maxItemsToShow = - listHeight && questionHeightLimit - ? Math.max(1, Math.floor((listHeight - questionHeightLimit) / 2)) + listHeight && (!isAlternateBuffer || availableHeight !== undefined) + ? Math.min( + selectionItems.length, + Math.max( + 1, + Math.floor((listHeight - (questionHeightLimit ?? 0)) / 2), + ), + ) : selectionItems.length; return ( diff --git a/packages/cli/src/ui/components/MainContent.test.tsx b/packages/cli/src/ui/components/MainContent.test.tsx index b2c18aa7d8..66ebc772e9 100644 --- a/packages/cli/src/ui/components/MainContent.test.tsx +++ b/packages/cli/src/ui/components/MainContent.test.tsx @@ -97,7 +97,7 @@ describe('getToolGroupBorderAppearance', () => { }); it('inspects only the last pending tool_group item if current has no tools', () => { - const item = { type: 'tool_group' as const, tools: [], id: 1 }; + const item = { type: 'tool_group' as const, tools: [], id: -1 }; const pendingItems = [ { type: 'tool_group' as const, @@ -158,7 +158,7 @@ describe('getToolGroupBorderAppearance', () => { confirmationDetails: undefined, } as IndividualToolCallDisplay, ], - id: 1, + id: -1, }; const result = getToolGroupBorderAppearance( item, @@ -187,7 +187,7 @@ describe('getToolGroupBorderAppearance', () => { confirmationDetails: undefined, } as IndividualToolCallDisplay, ], - id: 1, + id: -1, }; const result = getToolGroupBorderAppearance( item, @@ -276,7 +276,7 @@ describe('getToolGroupBorderAppearance', () => { confirmationDetails: undefined, } as IndividualToolCallDisplay, ], - id: 1, + id: -1, }; const result = getToolGroupBorderAppearance( item, @@ -292,7 +292,7 @@ describe('getToolGroupBorderAppearance', () => { }); it('handles empty tools with active shell turn (isCurrentlyInShellTurn)', () => { - const item = { type: 'tool_group' as const, tools: [], id: 1 }; + const item = { type: 'tool_group' as const, tools: [], id: -1 }; // active shell turn const result = getToolGroupBorderAppearance( @@ -693,7 +693,7 @@ describe('MainContent', () => { pendingHistoryItems: [ { type: 'tool_group', - id: 1, + id: -1, tools: [ { callId: 'call_1', diff --git a/packages/cli/src/ui/components/MainContent.tsx b/packages/cli/src/ui/components/MainContent.tsx index 0530e171b8..98b99d384f 100644 --- a/packages/cli/src/ui/components/MainContent.tsx +++ b/packages/cli/src/ui/components/MainContent.tsx @@ -88,6 +88,7 @@ export const MainContent = () => { () => augmentedHistory.map( ({ item, isExpandable, isFirstThinking, isFirstAfterThinking }) => ( + // ({ item, isExpandable, isFirstThinking, /* isFirstAfterThinking */ }) => ( { const pendingItems = useMemo( () => ( - + {pendingHistoryItems.map((item, i) => { const prevType = i === 0 @@ -140,12 +141,12 @@ export const MainContent = () => { return ( { ); })} {showConfirmationQueue && confirmingTool && ( - + )} ), diff --git a/packages/cli/src/ui/components/__snapshots__/AskUserDialog.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/AskUserDialog.test.tsx.snap index 30caf0fb40..c0fae8bf19 100644 --- a/packages/cli/src/ui/components/__snapshots__/AskUserDialog.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/AskUserDialog.test.tsx.snap @@ -39,37 +39,14 @@ Enter to select · ↑/↓ to navigate · Esc to cancel exports[`AskUserDialog > Scroll Arrows (useAlternateBuffer: true) > shows scroll arrows correctly when useAlternateBuffer is true 1`] = ` "Choose an option +▲ ● 1. Option 1 Description 1 2. Option 2 Description 2 3. Option 3 Description 3 - 4. Option 4 - Description 4 - 5. Option 5 - Description 5 - 6. Option 6 - Description 6 - 7. Option 7 - Description 7 - 8. Option 8 - Description 8 - 9. Option 9 - Description 9 - 10. Option 10 - Description 10 - 11. Option 11 - Description 11 - 12. Option 12 - Description 12 - 13. Option 13 - Description 13 - 14. Option 14 - Description 14 - 15. Option 15 - Description 15 - 16. Enter a custom value +▼ Enter to select · ↑/↓ to navigate · Esc to cancel " diff --git a/packages/cli/src/ui/components/__snapshots__/MainContent.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/MainContent.test.tsx.snap index 785dc6b6f0..4a5813f814 100644 --- a/packages/cli/src/ui/components/__snapshots__/MainContent.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/MainContent.test.tsx.snap @@ -6,12 +6,11 @@ AppHeader(full) ╭──────────────────────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command Running a long command... │ │ │ -│ Line 9 │ │ Line 10 │ │ Line 11 │ │ Line 12 │ │ Line 13 │ -│ Line 14 █ │ +│ Line 14 │ │ Line 15 █ │ │ Line 16 █ │ │ Line 17 █ │ @@ -28,12 +27,11 @@ AppHeader(full) ╭──────────────────────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command Running a long command... │ │ │ -│ Line 9 │ │ Line 10 │ │ Line 11 │ │ Line 12 │ │ Line 13 │ -│ Line 14 █ │ +│ Line 14 │ │ Line 15 █ │ │ Line 16 █ │ │ Line 17 █ │ @@ -49,8 +47,7 @@ exports[`MainContent > MainContent Tool Output Height Logic > 'Normal mode - Con ╭──────────────────────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command Running a long command... │ │ │ -│ ... first 9 lines hidden (Ctrl+O to show) ... │ -│ Line 10 │ +│ ... first 10 lines hidden (Ctrl+O to show) ... │ │ Line 11 │ │ Line 12 │ │ Line 13 │ diff --git a/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx index 6135d3574e..e5f95465ca 100644 --- a/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx @@ -185,28 +185,28 @@ describe('', () => { [ 'respects availableTerminalHeight when it is smaller than ACTIVE_SHELL_MAX_LINES', 10, - 8, + 7, false, true, ], [ 'uses ACTIVE_SHELL_MAX_LINES when availableTerminalHeight is large', 100, - ACTIVE_SHELL_MAX_LINES - 3, + ACTIVE_SHELL_MAX_LINES - 4, false, true, ], [ 'uses full availableTerminalHeight when focused in alternate buffer mode', 100, - 98, + 97, true, false, ], [ 'defaults to ACTIVE_SHELL_MAX_LINES in alternate buffer when availableTerminalHeight is undefined', undefined, - ACTIVE_SHELL_MAX_LINES - 3, + ACTIVE_SHELL_MAX_LINES - 4, false, false, ], @@ -328,8 +328,8 @@ describe('', () => { await waitUntilReady(); await waitFor(() => { const frame = lastFrame(); - // Should still be constrained to 12 (15 - 3) because isExpandable is false - expect(frame.match(/Line \d+/g)?.length).toBe(12); + // Should still be constrained to 11 (15 - 4) because isExpandable is false + expect(frame.match(/Line \d+/g)?.length).toBe(11); }); expect(lastFrame()).toMatchSnapshot(); unmount(); diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx index 5398f2c23f..4f62e3cce7 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx @@ -469,8 +469,7 @@ describe('ToolConfirmationMessage', () => { confirm: vi.fn(), cancel: vi.fn(), isDiffingEnabled: false, - }); - + }); const { lastFrame, waitUntilReady, unmount } = await renderWithProviders( { confirm: vi.fn(), cancel: vi.fn(), isDiffingEnabled: false, - }); - + }); const { lastFrame, waitUntilReady, unmount } = await renderWithProviders( { confirm: mockConfirm, cancel: vi.fn(), isDiffingEnabled: false, - }); - + }); const confirmationDetails: SerializableConfirmationDetails = { type: 'info', title: 'Confirm Web Fetch', diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ShellToolMessage.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/ShellToolMessage.test.tsx.snap index 1847b8ce67..967ea81e14 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/ShellToolMessage.test.tsx.snap +++ b/packages/cli/src/ui/components/messages/__snapshots__/ShellToolMessage.test.tsx.snap @@ -4,7 +4,6 @@ exports[` > Height Constraints > defaults to ACTIVE_SHELL_MA "╭──────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command A shell command │ │ │ -│ Line 89 │ │ Line 90 │ │ Line 91 │ │ Line 92 │ @@ -14,7 +13,7 @@ exports[` > Height Constraints > defaults to ACTIVE_SHELL_MA │ Line 96 │ │ Line 97 │ │ Line 98 │ -│ Line 99 ▄ │ +│ Line 99 │ │ Line 100 █ │ " `; @@ -130,7 +129,6 @@ exports[` > Height Constraints > respects availableTerminalH "╭──────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command A shell command │ │ │ -│ Line 93 │ │ Line 94 │ │ Line 95 │ │ Line 96 │ @@ -145,7 +143,6 @@ exports[` > Height Constraints > stays constrained in altern "╭──────────────────────────────────────────────────────────────────────────────╮ │ ✓ Shell Command A shell command │ │ │ -│ Line 89 │ │ Line 90 │ │ Line 91 │ │ Line 92 │ @@ -155,7 +152,7 @@ exports[` > Height Constraints > stays constrained in altern │ Line 96 │ │ Line 97 │ │ Line 98 │ -│ Line 99 ▄ │ +│ Line 99 │ │ Line 100 █ │ " `; @@ -164,7 +161,6 @@ exports[` > Height Constraints > uses ACTIVE_SHELL_MAX_LINES "╭──────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command A shell command │ │ │ -│ Line 89 │ │ Line 90 │ │ Line 91 │ │ Line 92 │ @@ -174,7 +170,7 @@ exports[` > Height Constraints > uses ACTIVE_SHELL_MAX_LINES │ Line 96 │ │ Line 97 │ │ Line 98 │ -│ Line 99 ▄ │ +│ Line 99 │ │ Line 100 █ │ " `; @@ -183,10 +179,9 @@ exports[` > Height Constraints > uses full availableTerminal "╭──────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command A shell command (Shift+Tab to unfocus) │ │ │ -│ Line 3 │ │ Line 4 │ -│ Line 5 █ │ -│ Line 6 █ │ +│ Line 5 │ +│ Line 6 │ │ Line 7 █ │ │ Line 8 █ │ │ Line 9 █ │ diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ToolResultDisplay.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/ToolResultDisplay.test.tsx.snap index 5e5c7ea2b0..e34e66cc48 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/ToolResultDisplay.test.tsx.snap +++ b/packages/cli/src/ui/components/messages/__snapshots__/ToolResultDisplay.test.tsx.snap @@ -37,8 +37,7 @@ exports[`ToolResultDisplay > renders string result as plain text when renderOutp `; exports[`ToolResultDisplay > truncates very long string results 1`] = ` -"... 248 hidden (Ctrl+O) ... -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +"... 249 hidden (Ctrl+O) ... aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa diff --git a/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx b/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx index 8a75bf7d57..21f2d6b1db 100644 --- a/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx +++ b/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx @@ -72,16 +72,21 @@ describe('ToolActionsContext', () => { beforeEach(() => { vi.clearAllMocks(); + // Default to a pending promise to avoid unwanted async state updates in tests + // that don't specifically test the IdeClient initialization. + vi.mocked(IdeClient.getInstance).mockReturnValue(new Promise(() => {})); }); - const wrapper = ({ children }: { children: React.ReactNode }) => ( - - {children} - - ); + const Wrapper = ({ children }: { children: React.ReactNode }) => { + return ( + + {children} + + ); + }; it('publishes to MessageBus for tools with correlationId', async () => { - const { result } = renderHook(() => useToolActions(), { wrapper }); + const { result } = renderHook(() => useToolActions(), { wrapper: Wrapper }); await result.current.confirm( 'modern-call', @@ -99,7 +104,7 @@ describe('ToolActionsContext', () => { }); it('handles cancel by calling confirm with Cancel outcome', async () => { - const { result } = renderHook(() => useToolActions(), { wrapper }); + const { result } = renderHook(() => useToolActions(), { wrapper: Wrapper }); await result.current.cancel('modern-call'); @@ -119,7 +124,7 @@ describe('ToolActionsContext', () => { vi.mocked(IdeClient.getInstance).mockResolvedValue(mockIdeClient); vi.mocked(mockConfig.getIdeMode).mockReturnValue(true); - const { result } = renderHook(() => useToolActions(), { wrapper }); + const { result } = renderHook(() => useToolActions(), { wrapper: Wrapper }); // Wait for IdeClient initialization in useEffect await act(async () => { @@ -157,7 +162,7 @@ describe('ToolActionsContext', () => { vi.mocked(IdeClient.getInstance).mockResolvedValue(mockIdeClient); vi.mocked(mockConfig.getIdeMode).mockReturnValue(true); - const { result } = renderHook(() => useToolActions(), { wrapper }); + const { result } = renderHook(() => useToolActions(), { wrapper: Wrapper }); // Wait for initialization await act(async () => { diff --git a/packages/cli/src/ui/contexts/ToolActionsContext.tsx b/packages/cli/src/ui/contexts/ToolActionsContext.tsx index 10e063e098..4168853cbb 100644 --- a/packages/cli/src/ui/contexts/ToolActionsContext.tsx +++ b/packages/cli/src/ui/contexts/ToolActionsContext.tsx @@ -5,13 +5,7 @@ */ import type React from 'react'; -import { - createContext, - useContext, - useCallback, - useState, - useEffect, -} from 'react'; +import { createContext, useContext, useCallback, useState, useEffect } from 'react'; import { IdeClient, ToolConfirmationOutcome, @@ -52,7 +46,7 @@ interface ToolActionsContextValue { const ToolActionsContext = createContext(null); -export const useToolActions = () => { +export const useToolActions = (): ToolActionsContextValue => { const context = useContext(ToolActionsContext); if (!context) { throw new Error('useToolActions must be used within a ToolActionsProvider'); @@ -77,24 +71,23 @@ export const ToolActionsProvider: React.FC = ( useEffect(() => { let isMounted = true; + let activeClient: IdeClient | null = null; + + const handleStatusChange = () => { + if (isMounted && activeClient) { + setIsDiffingEnabled(activeClient.isDiffingEnabled()); + } + }; + if (config.getIdeMode()) { IdeClient.getInstance() .then((client) => { if (!isMounted) return; + activeClient = client; setIdeClient(client); setIsDiffingEnabled(client.isDiffingEnabled()); - const handleStatusChange = () => { - if (isMounted) { - setIsDiffingEnabled(client.isDiffingEnabled()); - } - }; - client.addStatusChangeListener(handleStatusChange); - // Return a cleanup function for the listener - return () => { - client.removeStatusChangeListener(handleStatusChange); - }; }) .catch((error) => { debugLogger.error('Failed to get IdeClient instance:', error); @@ -102,6 +95,9 @@ export const ToolActionsProvider: React.FC = ( } return () => { isMounted = false; + if (activeClient) { + activeClient.removeStatusChangeListener(handleStatusChange); + } }; }, [config]); @@ -164,7 +160,13 @@ export const ToolActionsProvider: React.FC = ( ); return ( - + {children} ); diff --git a/packages/cli/src/ui/hooks/useHistoryManager.test.ts b/packages/cli/src/ui/hooks/useHistoryManager.test.ts index 696f9d60c0..b657e5d4ba 100644 --- a/packages/cli/src/ui/hooks/useHistoryManager.test.ts +++ b/packages/cli/src/ui/hooks/useHistoryManager.test.ts @@ -39,6 +39,56 @@ describe('useHistoryManager', () => { expect(result.current.history[0].id).toBeGreaterThanOrEqual(timestamp); }); + it('should generate strictly increasing IDs even if baseTimestamp goes backwards', () => { + const { result } = renderHook(() => useHistory()); + const timestamp = 1000000; + const itemData: Omit = { type: 'info', text: 'First' }; + + let id1!: number; + let id2!: number; + + act(() => { + id1 = result.current.addItem(itemData, timestamp); + // Try to add with a smaller timestamp + id2 = result.current.addItem(itemData, timestamp - 500); + }); + + expect(id1).toBe(timestamp); + expect(id2).toBe(id1 + 1); + expect(result.current.history[1].id).toBe(id2); + }); + + it('should ensure new IDs start after existing IDs when resuming a session', () => { + const initialItems: HistoryItem[] = [ + { id: 5000, type: 'info', text: 'Existing' }, + ]; + const { result } = renderHook(() => useHistory({ initialItems })); + + let newId!: number; + act(() => { + // Try to add with a timestamp smaller than the highest existing ID + newId = result.current.addItem({ type: 'info', text: 'New' }, 2000); + }); + + expect(newId).toBe(5001); + expect(result.current.history[1].id).toBe(5001); + }); + + it('should update lastIdRef when loading new history', () => { + const { result } = renderHook(() => useHistory()); + + act(() => { + result.current.loadHistory([{ id: 8000, type: 'info', text: 'Loaded' }]); + }); + + let newId!: number; + act(() => { + newId = result.current.addItem({ type: 'info', text: 'New' }, 1000); + }); + + expect(newId).toBe(8001); + }); + it('should generate unique IDs for items added with the same base timestamp', () => { const { result } = renderHook(() => useHistory()); const timestamp = Date.now(); @@ -215,8 +265,8 @@ describe('useHistoryManager', () => { const after = Date.now(); expect(result.current.history).toHaveLength(1); - // ID should be >= before + 1 (since counter starts at 0 and increments to 1) - expect(result.current.history[0].id).toBeGreaterThanOrEqual(before + 1); + // ID should be >= before (since baseTimestamp defaults to Date.now()) + expect(result.current.history[0].id).toBeGreaterThanOrEqual(before); expect(result.current.history[0].id).toBeLessThanOrEqual(after + 1); }); diff --git a/packages/cli/src/ui/hooks/useHistoryManager.ts b/packages/cli/src/ui/hooks/useHistoryManager.ts index 93f7f01f28..c6ceabb920 100644 --- a/packages/cli/src/ui/hooks/useHistoryManager.ts +++ b/packages/cli/src/ui/hooks/useHistoryManager.ts @@ -42,16 +42,22 @@ export function useHistory({ initialItems?: HistoryItem[]; } = {}): UseHistoryManagerReturn { const [history, setHistory] = useState(initialItems); - const messageIdCounterRef = useRef(0); + const lastIdRef = useRef( + initialItems.reduce((max, item) => Math.max(max, item.id), 0), + ); - // Generates a unique message ID based on a timestamp and a counter. + // Generates a unique message ID based on a timestamp, ensuring it is always + // greater than any previously assigned ID. const getNextMessageId = useCallback((baseTimestamp: number): number => { - messageIdCounterRef.current += 1; - return baseTimestamp + messageIdCounterRef.current; + const nextId = Math.max(baseTimestamp, lastIdRef.current + 1); + lastIdRef.current = nextId; + return nextId; }, []); const loadHistory = useCallback((newHistory: HistoryItem[]) => { setHistory(newHistory); + const maxId = newHistory.reduce((max, item) => Math.max(max, item.id), 0); + lastIdRef.current = Math.max(lastIdRef.current, maxId); }, []); // Adds a new item to the history state with a unique ID. @@ -153,7 +159,7 @@ export function useHistory({ // Clears the entire history state and resets the ID counter. const clearItems = useCallback(() => { setHistory([]); - messageIdCounterRef.current = 0; + lastIdRef.current = 0; }, []); return useMemo( diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index 2f8e414a83..f32d6fbc5c 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -16,13 +16,20 @@ import { type AgentDefinition, type ApprovalMode, type Kind, + type AnsiOutput, CoreToolCallStatus, checkExhaustive, } from '@google/gemini-cli-core'; import type { PartListUnion } from '@google/genai'; import { type ReactNode } from 'react'; -export type { ThoughtSummary, SkillDefinition }; +export { CoreToolCallStatus }; +export type { + ThoughtSummary, + SkillDefinition, + SerializableConfirmationDetails, + ToolResultDisplay, +}; export enum AuthState { // Attempting to authenticate or re-authenticate @@ -86,6 +93,88 @@ export function mapCoreStatusToDisplayStatus( } } +export interface DiffStat { + model_added_lines: number; + model_removed_lines: number; + model_added_chars: number; + model_removed_chars: number; + user_added_lines: number; + user_removed_lines: number; + user_added_chars: number; + user_removed_chars: number; +} + +export interface FileDiff { + fileDiff: string; + fileName: string; + filePath: string; + originalContent: string | null; + newContent: string; + diffStat?: DiffStat; + isNewFile?: boolean; +} + +export interface GrepMatch { + filePath: string; + lineNumber: number; + line: string; +} + +export interface GrepResult { + summary: string; + matches?: GrepMatch[]; + payload?: string; +} + +export interface ListDirectoryResult { + summary: string; + files?: string[]; + payload?: string; +} + +export interface ReadManyFilesResult { + summary: string; + files?: string[]; + skipped?: Array<{ path: string; reason: string }>; + include?: string[]; + excludes?: string[]; + targetDir?: string; + payload?: string; +} + +/** + * --- TYPE GUARDS --- + */ + +export const isFileDiff = (res: unknown): res is FileDiff => + typeof res === 'object' && res !== null && 'fileDiff' in res; + +export const isGrepResult = (res: unknown): res is GrepResult => + typeof res === 'object' && + res !== null && + 'summary' in res && + ('matches' in res || 'payload' in res); + +export const isListResult = ( + res: unknown, +): res is ListDirectoryResult | ReadManyFilesResult => + typeof res === 'object' && + res !== null && + 'summary' in res && + ('files' in res || 'include' in res); + +export const isTodoList = (res: unknown): res is { todos: unknown[] } => + typeof res === 'object' && res !== null && 'todos' in res; + +export const isAnsiOutput = (res: unknown): res is AnsiOutput => + Array.isArray(res) && (res.length === 0 || Array.isArray(res[0])); + +export const hasSummary = (res: unknown): res is { summary: string } => + typeof res === 'object' && + res !== null && + 'summary' in res && + typeof res.summary === 'string'; + export interface ToolCallEvent { type: 'tool_call'; status: CoreToolCallStatus; @@ -352,10 +441,6 @@ export type HistoryItemMcpStatus = HistoryItemBase & { showSchema: boolean; }; -// Using Omit seems to have some issues with typescript's -// type inference e.g. historyItem.type === 'tool_group' isn't auto-inferring that -// 'tools' in historyItem. -// Individually exported types extending HistoryItemBase export type HistoryItemWithoutId = | HistoryItemUser | HistoryItemUserShell diff --git a/packages/cli/src/ui/utils/CodeColorizer.test.tsx b/packages/cli/src/ui/utils/CodeColorizer.test.tsx index 2628a36d0a..b9d5c65877 100644 --- a/packages/cli/src/ui/utils/CodeColorizer.test.tsx +++ b/packages/cli/src/ui/utils/CodeColorizer.test.tsx @@ -82,4 +82,28 @@ describe('colorizeCode', () => { await expect(renderResult).toMatchSvgSnapshot(); renderResult.unmount(); }); + + it('returns an array of lines when returnLines is true', () => { + const code = 'line 1\nline 2\nline 3'; + const settings = new LoadedSettings( + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + true, + [], + ); + + const result = colorizeCode({ + code, + language: 'javascript', + maxWidth: 80, + settings, + hideLineNumbers: true, + returnLines: true, + }); + + expect(Array.isArray(result)).toBe(true); + expect(result).toHaveLength(3); + }); }); diff --git a/packages/cli/src/ui/utils/CodeColorizer.tsx b/packages/cli/src/ui/utils/CodeColorizer.tsx index 948a5f8988..94dda9501e 100644 --- a/packages/cli/src/ui/utils/CodeColorizer.tsx +++ b/packages/cli/src/ui/utils/CodeColorizer.tsx @@ -21,8 +21,8 @@ import { MaxSizedBox, MINIMUM_MAX_HEIGHT, } from '../components/shared/MaxSizedBox.js'; -import type { LoadedSettings } from '../../config/settings.js'; import { debugLogger } from '@google/gemini-cli-core'; +import type { LoadedSettings } from '../../config/settings.js'; // Configure theming and parsing utilities. const lowlight = createLowlight(common); @@ -117,7 +117,11 @@ export function colorizeLine( line: string, language: string | null, theme?: Theme, + disableColor = false, ): React.ReactNode { + if (disableColor) { + return {line}; + } const activeTheme = theme || themeManager.getActiveTheme(); return highlightAndRenderLine(line, language, activeTheme); } @@ -130,6 +134,8 @@ export interface ColorizeCodeOptions { theme?: Theme | null; settings: LoadedSettings; hideLineNumbers?: boolean; + disableColor?: boolean; + returnLines?: boolean; } /** @@ -138,6 +144,12 @@ export interface ColorizeCodeOptions { * @param options The options for colorizing the code. * @returns A React.ReactNode containing Ink elements for the highlighted code. */ +export function colorizeCode( + options: ColorizeCodeOptions & { returnLines: true }, +): React.ReactNode[]; +export function colorizeCode( + options: ColorizeCodeOptions & { returnLines?: false }, +): React.ReactNode; export function colorizeCode({ code, language = null, @@ -146,13 +158,16 @@ export function colorizeCode({ theme = null, settings, hideLineNumbers = false, -}: ColorizeCodeOptions): React.ReactNode { + disableColor = false, + returnLines = false, +}: ColorizeCodeOptions): React.ReactNode | React.ReactNode[] { const codeToHighlight = code.replace(/\n$/, ''); const activeTheme = theme || themeManager.getActiveTheme(); const showLineNumbers = hideLineNumbers ? false : settings.merged.ui.showLineNumbers; + const useMaxSizedBox = !settings.merged.ui.useAlternateBuffer && !returnLines; try { // Render the HAST tree using the adapted theme // Apply the theme's default foreground color to the top-level Text element @@ -162,7 +177,7 @@ export function colorizeCode({ let hiddenLinesCount = 0; // Optimization to avoid highlighting lines that cannot possibly be displayed. - if (availableHeight !== undefined) { + if (availableHeight !== undefined && useMaxSizedBox) { availableHeight = Math.max(availableHeight, MINIMUM_MAX_HEIGHT); if (lines.length > availableHeight) { const sliceIndex = lines.length - availableHeight; @@ -172,11 +187,9 @@ export function colorizeCode({ } const renderedLines = lines.map((line, index) => { - const contentToRender = highlightAndRenderLine( - line, - language, - activeTheme, - ); + const contentToRender = disableColor + ? line + : highlightAndRenderLine(line, language, activeTheme); return ( @@ -188,19 +201,26 @@ export function colorizeCode({ alignItems="flex-start" justifyContent="flex-end" > - + {`${index + 1 + hiddenLinesCount}`} )} - + {contentToRender} ); }); - if (availableHeight !== undefined) { + if (returnLines) { + return renderedLines; + } + + if (useMaxSizedBox) { return ( - {`${index + 1}`} + + {`${index + 1}`} + )} - {stripAnsi(line)} + + {stripAnsi(line)} + )); - if (availableHeight !== undefined) { + if (returnLines) { + return fallbackLines; + } + + if (useMaxSizedBox) { return ( item.type === 'tool_group') - .flatMap((group) => group.tools); + const allPendingTools = getAllToolCalls(pendingHistoryItems); const confirmingTools = allPendingTools.filter( (tool) => tool.status === CoreToolCallStatus.AwaitingApproval, diff --git a/packages/cli/src/ui/utils/historyUtils.ts b/packages/cli/src/ui/utils/historyUtils.ts new file mode 100644 index 0000000000..ee607dca96 --- /dev/null +++ b/packages/cli/src/ui/utils/historyUtils.ts @@ -0,0 +1,83 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { CoreToolCallStatus } from '../types.js'; +import type { + HistoryItem, + HistoryItemWithoutId, + HistoryItemToolGroup, + IndividualToolCallDisplay, +} from '../types.js'; + +export function getLastTurnToolCallIds( + history: HistoryItem[], + pendingHistoryItems: HistoryItemWithoutId[], +): string[] { + const targetToolCallIds: string[] = []; + + // Find the boundary of the last user prompt + let lastUserPromptIndex = -1; + for (let i = history.length - 1; i >= 0; i--) { + const type = history[i].type; + if (type === 'user' || type === 'user_shell') { + lastUserPromptIndex = i; + break; + } + } + + // Collect IDs from history after last user prompt + history.forEach((item, index) => { + if (index > lastUserPromptIndex && item.type === 'tool_group') { + item.tools.forEach((t) => { + if (t.callId) targetToolCallIds.push(t.callId); + }); + } + }); + + // Collect IDs from pending items + pendingHistoryItems.forEach((item) => { + if (item.type === 'tool_group') { + item.tools.forEach((t) => { + if (t.callId) targetToolCallIds.push(t.callId); + }); + } + }); + + return targetToolCallIds; +} + +export function isToolExecuting( + pendingHistoryItems: HistoryItemWithoutId[], +): boolean { + return pendingHistoryItems.some((item) => { + if (item && item.type === 'tool_group') { + return item.tools.some( + (tool) => CoreToolCallStatus.Executing === tool.status, + ); + } + return false; + }); +} + +export function isToolAwaitingConfirmation( + pendingHistoryItems: HistoryItemWithoutId[], +): boolean { + return pendingHistoryItems + .filter((item): item is HistoryItemToolGroup => item.type === 'tool_group') + .some((item) => + item.tools.some( + (tool) => CoreToolCallStatus.AwaitingApproval === tool.status, + ), + ); +} + +export function getAllToolCalls( + historyItems: HistoryItemWithoutId[], +): IndividualToolCallDisplay[] { + return historyItems + .filter((item): item is HistoryItemToolGroup => item.type === 'tool_group') + .flatMap((group) => group.tools); +} diff --git a/packages/cli/src/ui/utils/toolLayoutUtils.test.ts b/packages/cli/src/ui/utils/toolLayoutUtils.test.ts index 57e1e3f190..768fccc111 100644 --- a/packages/cli/src/ui/utils/toolLayoutUtils.test.ts +++ b/packages/cli/src/ui/utils/toolLayoutUtils.test.ts @@ -9,6 +9,10 @@ import { calculateToolContentMaxLines, calculateShellMaxLines, SHELL_CONTENT_OVERHEAD, + TOOL_RESULT_STATIC_HEIGHT, + TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT, + TOOL_RESULT_ASB_RESERVED_LINE_COUNT, + TOOL_RESULT_MIN_LINES_SHOWN, } from './toolLayoutUtils.js'; import { CoreToolCallStatus } from '@google/gemini-cli-core'; import { @@ -48,7 +52,7 @@ describe('toolLayoutUtils', () => { availableTerminalHeight: 2, isAlternateBuffer: false, }, - expected: 3, + expected: TOOL_RESULT_MIN_LINES_SHOWN + 1, }, { desc: 'returns available space directly in constrained terminal (ASB mode)', @@ -56,7 +60,7 @@ describe('toolLayoutUtils', () => { availableTerminalHeight: 4, isAlternateBuffer: true, }, - expected: 3, + expected: TOOL_RESULT_MIN_LINES_SHOWN + 1, }, { desc: 'returns remaining space if sufficient space exists (Standard mode)', @@ -64,7 +68,10 @@ describe('toolLayoutUtils', () => { availableTerminalHeight: 20, isAlternateBuffer: false, }, - expected: 17, + expected: + 20 - + TOOL_RESULT_STATIC_HEIGHT - + TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT, }, { desc: 'returns remaining space if sufficient space exists (ASB mode)', @@ -72,7 +79,8 @@ describe('toolLayoutUtils', () => { availableTerminalHeight: 20, isAlternateBuffer: true, }, - expected: 13, + expected: + 20 - TOOL_RESULT_STATIC_HEIGHT - TOOL_RESULT_ASB_RESERVED_LINE_COUNT, }, ]; @@ -148,7 +156,7 @@ describe('toolLayoutUtils', () => { constrainHeight: true, isExpandable: false, }, - expected: 4, + expected: 6 - TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT, }, { desc: 'handles negative availableTerminalHeight gracefully', @@ -172,7 +180,7 @@ describe('toolLayoutUtils', () => { constrainHeight: false, isExpandable: false, }, - expected: 28, + expected: 30 - TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT, }, { desc: 'falls back to COMPLETED_SHELL_MAX_LINES - SHELL_CONTENT_OVERHEAD for completed shells if space allows', diff --git a/packages/cli/src/ui/utils/toolLayoutUtils.ts b/packages/cli/src/ui/utils/toolLayoutUtils.ts index 9f391dca4e..1f140b9bc9 100644 --- a/packages/cli/src/ui/utils/toolLayoutUtils.ts +++ b/packages/cli/src/ui/utils/toolLayoutUtils.ts @@ -17,7 +17,7 @@ import { CoreToolCallStatus } from '@google/gemini-cli-core'; */ export const TOOL_RESULT_STATIC_HEIGHT = 1; export const TOOL_RESULT_ASB_RESERVED_LINE_COUNT = 6; -export const TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT = 2; +export const TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT = 3; export const TOOL_RESULT_MIN_LINES_SHOWN = 2; /** diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index 91aeab8308..3c96fdaa19 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -8,6 +8,7 @@ import { type FunctionCall } from '@google/genai'; import type { ToolConfirmationOutcome, ToolConfirmationPayload, + DiffStat, } from '../tools/tools.js'; import type { ToolCall } from '../scheduler/types.js'; @@ -88,6 +89,7 @@ export type SerializableConfirmationDetails = originalContent: string | null; newContent: string; isModifying?: boolean; + diffStat?: DiffStat; } | { type: 'exec'; diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 1ecae4ef33..4b4e6e0c54 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -23,6 +23,7 @@ import { getToolSuggestion, isToolCallResponseInfo, } from '../utils/tool-utils.js'; +import { getDiffStatFromPatch } from '../tools/diffOptions.js'; import type { ToolConfirmationRequest } from '../confirmation-bus/types.js'; import { MessageBusType } from '../confirmation-bus/types.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; @@ -312,6 +313,12 @@ export class CoreToolScheduler { waitingCall.confirmationDetails.originalContent, newContent: waitingCall.confirmationDetails.newContent, filePath: waitingCall.confirmationDetails.filePath, + // Derive stats from the patch if they aren't already present + diffStat: + waitingCall.confirmationDetails.diffStat ?? + getDiffStatFromPatch( + waitingCall.confirmationDetails.fileDiff, + ), }; } } diff --git a/packages/core/src/scheduler/state-manager.test.ts b/packages/core/src/scheduler/state-manager.test.ts index dd5071c5bf..ff69e0d207 100644 --- a/packages/core/src/scheduler/state-manager.test.ts +++ b/packages/core/src/scheduler/state-manager.test.ts @@ -22,6 +22,7 @@ import { ToolConfirmationOutcome, type AnyDeclarativeTool, type AnyToolInvocation, + type FileDiff, } from '../tools/tools.js'; import { MessageBusType } from '../confirmation-bus/types.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; @@ -359,7 +360,7 @@ describe('SchedulerStateManager', () => { expect(active.confirmationDetails).toEqual(details); }); - it('should preserve diff when cancelling an edit tool call', () => { + it('should preserve diff and derive stats when cancelling an edit tool call', () => { const call = createValidatingCall(); stateManager.enqueue([call]); stateManager.dequeue(); @@ -369,9 +370,9 @@ describe('SchedulerStateManager', () => { title: 'Edit', fileName: 'test.txt', filePath: '/path/to/test.txt', - fileDiff: 'diff', - originalContent: 'old', - newContent: 'new', + fileDiff: '@@ -1,1 +1,1 @@\n-old line\n+new line', + originalContent: 'old line', + newContent: 'new line', onConfirm: vi.fn(), }; @@ -389,13 +390,14 @@ describe('SchedulerStateManager', () => { const completed = stateManager.completedBatch[0] as CancelledToolCall; expect(completed.status).toBe(CoreToolCallStatus.Cancelled); - expect(completed.response.resultDisplay).toEqual({ - fileDiff: 'diff', - fileName: 'test.txt', - filePath: '/path/to/test.txt', - originalContent: 'old', - newContent: 'new', - }); + const result = completed.response.resultDisplay as FileDiff; + expect(result.fileDiff).toBe(details.fileDiff); + expect(result.diffStat).toEqual( + expect.objectContaining({ + model_added_lines: 1, + model_removed_lines: 1, + }), + ); }); it('should ignore status updates for non-existent callIds', () => { diff --git a/packages/core/src/scheduler/state-manager.ts b/packages/core/src/scheduler/state-manager.ts index 428b7f87a8..093aaa7308 100644 --- a/packages/core/src/scheduler/state-manager.ts +++ b/packages/core/src/scheduler/state-manager.ts @@ -32,6 +32,7 @@ import { type SerializableConfirmationDetails, } from '../confirmation-bus/types.js'; import { isToolCallResponseInfo } from '../utils/tool-utils.js'; +import { getDiffStatFromPatch } from '../tools/diffOptions.js'; /** * Handler for terminal tool calls. @@ -473,6 +474,8 @@ export class SchedulerStateManager { filePath: details.filePath, originalContent: details.originalContent, newContent: details.newContent, + // Derive stats from the patch if they aren't already present + diffStat: details.diffStat ?? getDiffStatFromPatch(details.fileDiff), }; } } diff --git a/packages/core/src/tools/diffOptions.ts b/packages/core/src/tools/diffOptions.ts index b026b14f7c..0a0e0fa49e 100644 --- a/packages/core/src/tools/diffOptions.ts +++ b/packages/core/src/tools/diffOptions.ts @@ -76,3 +76,39 @@ export function getDiffStat( user_removed_chars: userStats.removedChars, }; } + +/** + * Extracts line and character stats from a unified diff patch string. + * This is useful for reconstructing stats for rejected or errored operations + * where the full strings may no longer be easily accessible. + */ +export function getDiffStatFromPatch(patch: string): DiffStat { + let addedLines = 0; + let removedLines = 0; + let addedChars = 0; + let removedChars = 0; + + const lines = patch.split('\n'); + for (const line of lines) { + // Only count lines that are additions or removals, + // excluding the diff headers (--- and +++) and metadata (\) + if (line.startsWith('+') && !line.startsWith('+++')) { + addedLines++; + addedChars += line.length - 1; + } else if (line.startsWith('-') && !line.startsWith('---')) { + removedLines++; + removedChars += line.length - 1; + } + } + + return { + model_added_lines: addedLines, + model_removed_lines: removedLines, + model_added_chars: addedChars, + model_removed_chars: removedChars, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, + }; +} diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index bfa70565be..22d2bb3578 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -896,11 +896,33 @@ class EditToolInvocation DEFAULT_DIFF_OPTIONS, ); + // Determine the full content as originally proposed by the AI to ensure accurate diff stats. + let fullAiProposedContent = editData.newContent; + if ( + this.params.modified_by_user && + this.params.ai_proposed_content !== undefined + ) { + try { + const aiReplacement = await calculateReplacement(this.config, { + params: { + ...this.params, + new_string: this.params.ai_proposed_content, + }, + currentContent: editData.currentContent ?? '', + abortSignal: signal, + }); + fullAiProposedContent = aiReplacement.newContent; + } catch (_e) { + // Fallback to newContent if speculative calculation fails + fullAiProposedContent = editData.newContent; + } + } + const diffStat = getDiffStat( fileName, editData.currentContent ?? '', + fullAiProposedContent, editData.newContent, - this.params.new_string, ); displayResult = { fileDiff, diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index e818881662..3b55cef0a1 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -150,8 +150,6 @@ export { SKILL_PARAM_NAME, }; -export const LS_TOOL_NAME_LEGACY = 'list_directory'; // Just to be safe if anything used the old exported name directly - export const EDIT_TOOL_NAMES = new Set([EDIT_TOOL_NAME, WRITE_FILE_TOOL_NAME]); /** @@ -183,6 +181,11 @@ 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'; +export const LS_DISPLAY_NAME = 'ReadFolder'; +export const GREP_DISPLAY_NAME = 'SearchText'; +export const WEB_SEARCH_DISPLAY_NAME = 'GoogleSearch'; +export const WEB_FETCH_DISPLAY_NAME = 'WebFetch'; +export const READ_MANY_FILES_DISPLAY_NAME = 'ReadManyFiles'; /** * Mapping of legacy tool names to their current names. diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 3865aaf357..5e2d12ace2 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -816,6 +816,21 @@ export interface TodoList { export type ToolLiveOutput = string | AnsiOutput | SubagentProgress; +export interface StructuredToolResult { + summary: string; +} + +export function isStructuredToolResult( + obj: unknown, +): obj is StructuredToolResult { + return ( + typeof obj === 'object' && + obj !== null && + 'summary' in obj && + typeof obj.summary === 'string' + ); +} + export type ToolResultDisplay = | string | FileDiff @@ -869,6 +884,7 @@ export interface ToolEditConfirmationDetails { originalContent: string | null; newContent: string; isModifying?: boolean; + diffStat?: DiffStat; ideConfirmation?: Promise; } diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index 27a60c4259..4a7bf6554e 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -30,7 +30,7 @@ import { NetworkRetryAttemptEvent, } from '../telemetry/index.js'; import { LlmRole } from '../telemetry/llmRole.js'; -import { WEB_FETCH_TOOL_NAME } from './tool-names.js'; +import { WEB_FETCH_TOOL_NAME, WEB_FETCH_DISPLAY_NAME } from './tool-names.js'; import { debugLogger } from '../utils/debugLogger.js'; import { coreEvents } from '../utils/events.js'; import { retryWithBackoff, getRetryErrorType } from '../utils/retry.js'; @@ -891,7 +891,7 @@ export class WebFetchTool extends BaseDeclarativeTool< ) { super( WebFetchTool.Name, - 'WebFetch', + WEB_FETCH_DISPLAY_NAME, WEB_FETCH_DEFINITION.base.description!, Kind.Fetch, WEB_FETCH_DEFINITION.base.parametersJsonSchema, diff --git a/packages/core/src/tools/web-search.ts b/packages/core/src/tools/web-search.ts index 18132d2c35..2a29291437 100644 --- a/packages/core/src/tools/web-search.ts +++ b/packages/core/src/tools/web-search.ts @@ -5,7 +5,7 @@ */ import type { MessageBus } from '../confirmation-bus/message-bus.js'; -import { WEB_SEARCH_TOOL_NAME } from './tool-names.js'; +import { WEB_SEARCH_TOOL_NAME, WEB_SEARCH_DISPLAY_NAME } from './tool-names.js'; import type { GroundingMetadata } from '@google/genai'; import { BaseDeclarativeTool, @@ -212,7 +212,7 @@ export class WebSearchTool extends BaseDeclarativeTool< ) { super( WebSearchTool.Name, - 'GoogleSearch', + WEB_SEARCH_DISPLAY_NAME, WEB_SEARCH_DEFINITION.base.description!, Kind.Search, WEB_SEARCH_DEFINITION.base.parametersJsonSchema,