From dd84c2fb837ac6d84b90416b5388341b8d83ab87 Mon Sep 17 00:00:00 2001 From: Sandy Tao Date: Sun, 4 Jan 2026 18:58:34 -0800 Subject: [PATCH 001/591] feat(hooks): implement granular stop and block behavior for agent hooks (#15824) --- packages/cli/src/nonInteractiveCli.test.ts | 59 ++++++++ packages/cli/src/nonInteractiveCli.ts | 25 ++++ .../cli/src/ui/hooks/useGeminiStream.test.tsx | 57 ++++++++ packages/cli/src/ui/hooks/useGeminiStream.ts | 49 +++++++ packages/core/src/core/client.test.ts | 131 ++++++++++++++++++ packages/core/src/core/client.ts | 60 ++++++-- packages/core/src/core/turn.ts | 20 ++- 7 files changed, 388 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index f2a2a43592..c171d95a74 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -1797,4 +1797,63 @@ describe('runNonInteractive', () => { // The key assertion: sendMessageStream should have been called ONLY ONCE (initial user input). expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); }); + + describe('Agent Execution Events', () => { + it('should handle AgentExecutionStopped event', async () => { + const events: ServerGeminiStreamEvent[] = [ + { + type: GeminiEventType.AgentExecutionStopped, + value: { reason: 'Stopped by hook' }, + }, + ]; + mockGeminiClient.sendMessageStream.mockReturnValue( + createStreamFromEvents(events), + ); + + await runNonInteractive({ + config: mockConfig, + settings: mockSettings, + input: 'test stop', + prompt_id: 'prompt-id-stop', + }); + + expect(processStderrSpy).toHaveBeenCalledWith( + 'Agent execution stopped: Stopped by hook\n', + ); + // Should exit without calling sendMessageStream again + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); + }); + + it('should handle AgentExecutionBlocked event', async () => { + const allEvents: ServerGeminiStreamEvent[] = [ + { + type: GeminiEventType.AgentExecutionBlocked, + value: { reason: 'Blocked by hook' }, + }, + { type: GeminiEventType.Content, value: 'Final answer' }, + { + type: GeminiEventType.Finished, + value: { reason: undefined, usageMetadata: { totalTokenCount: 10 } }, + }, + ]; + + mockGeminiClient.sendMessageStream.mockReturnValue( + createStreamFromEvents(allEvents), + ); + + await runNonInteractive({ + config: mockConfig, + settings: mockSettings, + input: 'test block', + prompt_id: 'prompt-id-block', + }); + + expect(processStderrSpy).toHaveBeenCalledWith( + '[WARNING] Agent execution blocked: Blocked by hook\n', + ); + // sendMessageStream is called once, recursion is internal to it and transparent to the caller + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1); + expect(getWrittenOutput()).toBe('Final answer\n'); + }); + }); }); diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index c81efd72f5..d1f468ef39 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -348,6 +348,31 @@ export async function runNonInteractive({ } } else if (event.type === GeminiEventType.Error) { throw event.value.error; + } else if (event.type === GeminiEventType.AgentExecutionStopped) { + const stopMessage = `Agent execution stopped: ${event.value.reason}`; + if (config.getOutputFormat() === OutputFormat.TEXT) { + process.stderr.write(`${stopMessage}\n`); + } + // Emit final result event for streaming JSON if needed + if (streamFormatter) { + const metrics = uiTelemetryService.getMetrics(); + const durationMs = Date.now() - startTime; + streamFormatter.emitEvent({ + type: JsonStreamEventType.RESULT, + timestamp: new Date().toISOString(), + status: 'success', + stats: streamFormatter.convertToStreamStats( + metrics, + durationMs, + ), + }); + } + return; + } else if (event.type === GeminiEventType.AgentExecutionBlocked) { + const blockMessage = `Agent execution blocked: ${event.value.reason}`; + if (config.getOutputFormat() === OutputFormat.TEXT) { + process.stderr.write(`[WARNING] ${blockMessage}\n`); + } } } diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 7c1b3a7dc9..2414c340f4 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -2798,4 +2798,61 @@ describe('useGeminiStream', () => { }); }); }); + + describe('Agent Execution Events', () => { + it('should handle AgentExecutionStopped event', async () => { + mockSendMessageStream.mockReturnValue( + (async function* () { + yield { + type: ServerGeminiEventType.AgentExecutionStopped, + value: { reason: 'Stopped by hook' }, + }; + })(), + ); + + const { result } = renderTestHook(); + + await act(async () => { + await result.current.submitQuery('test stop'); + }); + + await waitFor(() => { + expect(mockAddItem).toHaveBeenCalledWith( + { + type: MessageType.INFO, + text: 'Agent execution stopped: Stopped by hook', + }, + expect.any(Number), + ); + expect(result.current.streamingState).toBe(StreamingState.Idle); + }); + }); + + it('should handle AgentExecutionBlocked event', async () => { + mockSendMessageStream.mockReturnValue( + (async function* () { + yield { + type: ServerGeminiEventType.AgentExecutionBlocked, + value: { reason: 'Blocked by hook' }, + }; + })(), + ); + + const { result } = renderTestHook(); + + await act(async () => { + await result.current.submitQuery('test block'); + }); + + await waitFor(() => { + expect(mockAddItem).toHaveBeenCalledWith( + { + type: MessageType.WARNING, + text: 'Agent execution blocked: Blocked by hook', + }, + expect.any(Number), + ); + }); + }); + }); }); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index d36d9f57ed..4522af13c7 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -793,6 +793,41 @@ export const useGeminiStream = ( [addItem, pendingHistoryItemRef, setPendingHistoryItem, settings], ); + const handleAgentExecutionStoppedEvent = useCallback( + (reason: string, userMessageTimestamp: number) => { + if (pendingHistoryItemRef.current) { + addItem(pendingHistoryItemRef.current, userMessageTimestamp); + setPendingHistoryItem(null); + } + addItem( + { + type: MessageType.INFO, + text: `Agent execution stopped: ${reason}`, + }, + userMessageTimestamp, + ); + setIsResponding(false); + }, + [addItem, pendingHistoryItemRef, setPendingHistoryItem, setIsResponding], + ); + + const handleAgentExecutionBlockedEvent = useCallback( + (reason: string, userMessageTimestamp: number) => { + if (pendingHistoryItemRef.current) { + addItem(pendingHistoryItemRef.current, userMessageTimestamp); + setPendingHistoryItem(null); + } + addItem( + { + type: MessageType.WARNING, + text: `Agent execution blocked: ${reason}`, + }, + userMessageTimestamp, + ); + }, + [addItem, pendingHistoryItemRef, setPendingHistoryItem], + ); + const processGeminiStreamEvents = useCallback( async ( stream: AsyncIterable, @@ -822,6 +857,18 @@ export const useGeminiStream = ( case ServerGeminiEventType.Error: handleErrorEvent(event.value, userMessageTimestamp); break; + case ServerGeminiEventType.AgentExecutionStopped: + handleAgentExecutionStoppedEvent( + event.value.reason, + userMessageTimestamp, + ); + break; + case ServerGeminiEventType.AgentExecutionBlocked: + handleAgentExecutionBlockedEvent( + event.value.reason, + userMessageTimestamp, + ); + break; case ServerGeminiEventType.ChatCompressed: handleChatCompressionEvent(event.value, userMessageTimestamp); break; @@ -879,6 +926,8 @@ export const useGeminiStream = ( handleContextWindowWillOverflowEvent, handleCitationEvent, handleChatModelEvent, + handleAgentExecutionStoppedEvent, + handleAgentExecutionBlockedEvent, ], ); const submitQuery = useCallback( diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 34facc4737..6045088c04 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -46,6 +46,7 @@ import type { } from '../services/modelConfigService.js'; import { ClearcutLogger } from '../telemetry/clearcut-logger/clearcut-logger.js'; import { HookSystem } from '../hooks/hookSystem.js'; +import type { DefaultHookOutput } from '../hooks/types.js'; import * as policyCatalog from '../availability/policyCatalog.js'; vi.mock('../services/chatCompressionService.js'); @@ -2781,6 +2782,136 @@ ${JSON.stringify( expect(client['hookStateMap'].has('old-id')).toBe(false); expect(client['hookStateMap'].has('new-id')).toBe(true); }); + + it('should stop execution in BeforeAgent when hook returns continue: false', async () => { + const { fireBeforeAgentHook } = await import('./clientHookTriggers.js'); + vi.mocked(fireBeforeAgentHook).mockResolvedValue({ + shouldStopExecution: () => true, + getEffectiveReason: () => 'Stopped by hook', + } as DefaultHookOutput); + + const mockChat: Partial = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue([]), + getLastPromptTokenCount: vi.fn(), + }; + client['chat'] = mockChat as GeminiChat; + + const request = [{ text: 'Hello' }]; + const stream = client.sendMessageStream( + request, + new AbortController().signal, + 'test-prompt', + ); + const events = await fromAsync(stream); + + expect(events).toContainEqual({ + type: GeminiEventType.AgentExecutionStopped, + value: { reason: 'Stopped by hook' }, + }); + expect(mockChat.addHistory).toHaveBeenCalledWith({ + role: 'user', + parts: request, + }); + expect(mockTurnRunFn).not.toHaveBeenCalled(); + }); + + it('should block execution in BeforeAgent when hook returns decision: block', async () => { + const { fireBeforeAgentHook } = await import('./clientHookTriggers.js'); + vi.mocked(fireBeforeAgentHook).mockResolvedValue({ + shouldStopExecution: () => false, + isBlockingDecision: () => true, + getEffectiveReason: () => 'Blocked by hook', + } as DefaultHookOutput); + + const mockChat: Partial = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue([]), + getLastPromptTokenCount: vi.fn(), + }; + client['chat'] = mockChat as GeminiChat; + + const request = [{ text: 'Hello' }]; + const stream = client.sendMessageStream( + request, + new AbortController().signal, + 'test-prompt', + ); + const events = await fromAsync(stream); + + expect(events).toContainEqual({ + type: GeminiEventType.AgentExecutionBlocked, + value: { + reason: 'Blocked by hook', + }, + }); + expect(mockChat.addHistory).not.toHaveBeenCalled(); + expect(mockTurnRunFn).not.toHaveBeenCalled(); + }); + + it('should stop execution in AfterAgent when hook returns continue: false', async () => { + const { fireAfterAgentHook } = await import('./clientHookTriggers.js'); + vi.mocked(fireAfterAgentHook).mockResolvedValue({ + shouldStopExecution: () => true, + getEffectiveReason: () => 'Stopped after agent', + } as DefaultHookOutput); + + mockTurnRunFn.mockImplementation(async function* () { + yield { type: GeminiEventType.Content, value: 'Hello' }; + }); + + const stream = client.sendMessageStream( + { text: 'Hi' }, + new AbortController().signal, + 'test-prompt', + ); + const events = await fromAsync(stream); + + expect(events).toContainEqual({ + type: GeminiEventType.AgentExecutionStopped, + value: { reason: 'Stopped after agent' }, + }); + // sendMessageStream should not recurse + expect(mockTurnRunFn).toHaveBeenCalledTimes(1); + }); + + it('should yield AgentExecutionBlocked and recurse in AfterAgent when hook returns decision: block', async () => { + const { fireAfterAgentHook } = await import('./clientHookTriggers.js'); + vi.mocked(fireAfterAgentHook) + .mockResolvedValueOnce({ + shouldStopExecution: () => false, + isBlockingDecision: () => true, + getEffectiveReason: () => 'Please explain', + } as DefaultHookOutput) + .mockResolvedValueOnce({ + shouldStopExecution: () => false, + isBlockingDecision: () => false, + } as DefaultHookOutput); + + mockTurnRunFn.mockImplementation(async function* () { + yield { type: GeminiEventType.Content, value: 'Response' }; + }); + + const stream = client.sendMessageStream( + { text: 'Hi' }, + new AbortController().signal, + 'test-prompt', + ); + const events = await fromAsync(stream); + + expect(events).toContainEqual({ + type: GeminiEventType.AgentExecutionBlocked, + value: { reason: 'Please explain' }, + }); + // Should have called turn run twice (original + re-prompt) + expect(mockTurnRunFn).toHaveBeenCalledTimes(2); + expect(mockTurnRunFn).toHaveBeenNthCalledWith( + 2, + expect.anything(), + [{ text: 'Please explain' }], + expect.anything(), + ); + }); }); }); }); diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 448dd99310..ecd1eff471 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -11,6 +11,7 @@ import type { Tool, GenerateContentResponse, } from '@google/genai'; +import { createUserContent } from '@google/genai'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { getDirectoryContextString, @@ -65,8 +66,12 @@ const MAX_TURNS = 100; type BeforeAgentHookReturn = | { - type: GeminiEventType.Error; - value: { error: Error }; + type: GeminiEventType.AgentExecutionStopped; + value: { reason: string }; + } + | { + type: GeminiEventType.AgentExecutionBlocked; + value: { reason: string }; } | { additionalContext: string | undefined } | undefined; @@ -135,13 +140,20 @@ export class GeminiClient { const hookOutput = await fireBeforeAgentHook(messageBus, request); hookState.hasFiredBeforeAgent = true; - if (hookOutput?.isBlockingDecision() || hookOutput?.shouldStopExecution()) { + if (hookOutput?.shouldStopExecution()) { return { - type: GeminiEventType.Error, + type: GeminiEventType.AgentExecutionStopped, value: { - error: new Error( - `BeforeAgent hook blocked processing: ${hookOutput.getEffectiveReason()}`, - ), + reason: hookOutput.getEffectiveReason(), + }, + }; + } + + if (hookOutput?.isBlockingDecision()) { + return { + type: GeminiEventType.AgentExecutionBlocked, + value: { + reason: hookOutput.getEffectiveReason(), }, }; } @@ -747,7 +759,18 @@ export class GeminiClient { prompt_id, ); if (hookResult) { - if ('type' in hookResult && hookResult.type === GeminiEventType.Error) { + if ( + 'type' in hookResult && + hookResult.type === GeminiEventType.AgentExecutionStopped + ) { + // Add user message to history before returning so it's kept in the transcript + this.getChat().addHistory(createUserContent(request)); + yield hookResult; + return new Turn(this.getChat(), prompt_id); + } else if ( + 'type' in hookResult && + hookResult.type === GeminiEventType.AgentExecutionBlocked + ) { yield hookResult; return new Turn(this.getChat(), prompt_id); } else if ('additionalContext' in hookResult) { @@ -781,11 +804,24 @@ export class GeminiClient { turn, ); - if ( - hookOutput?.isBlockingDecision() || - hookOutput?.shouldStopExecution() - ) { + if (hookOutput?.shouldStopExecution()) { + yield { + type: GeminiEventType.AgentExecutionStopped, + value: { + reason: hookOutput.getEffectiveReason(), + }, + }; + return turn; + } + + if (hookOutput?.isBlockingDecision()) { const continueReason = hookOutput.getEffectiveReason(); + yield { + type: GeminiEventType.AgentExecutionBlocked, + value: { + reason: continueReason, + }, + }; const continueRequest = [{ text: continueReason }]; yield* this.sendMessageStream( continueRequest, diff --git a/packages/core/src/core/turn.ts b/packages/core/src/core/turn.ts index 21191b34f2..11825d9d7b 100644 --- a/packages/core/src/core/turn.ts +++ b/packages/core/src/core/turn.ts @@ -66,12 +66,28 @@ export enum GeminiEventType { ContextWindowWillOverflow = 'context_window_will_overflow', InvalidStream = 'invalid_stream', ModelInfo = 'model_info', + AgentExecutionStopped = 'agent_execution_stopped', + AgentExecutionBlocked = 'agent_execution_blocked', } export type ServerGeminiRetryEvent = { type: GeminiEventType.Retry; }; +export type ServerGeminiAgentExecutionStoppedEvent = { + type: GeminiEventType.AgentExecutionStopped; + value: { + reason: string; + }; +}; + +export type ServerGeminiAgentExecutionBlockedEvent = { + type: GeminiEventType.AgentExecutionBlocked; + value: { + reason: string; + }; +}; + export type ServerGeminiContextWindowWillOverflowEvent = { type: GeminiEventType.ContextWindowWillOverflow; value: { @@ -204,7 +220,9 @@ export type ServerGeminiStreamEvent = | ServerGeminiRetryEvent | ServerGeminiContextWindowWillOverflowEvent | ServerGeminiInvalidStreamEvent - | ServerGeminiModelInfoEvent; + | ServerGeminiModelInfoEvent + | ServerGeminiAgentExecutionStoppedEvent + | ServerGeminiAgentExecutionBlockedEvent; // A turn manages the agentic loop turn within the server context. export class Turn { From d3563e2f0eb1f2e6f4e11bd28b04f1ac36bc589b Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Sun, 4 Jan 2026 20:40:21 -0800 Subject: [PATCH 002/591] Agent Skills: Add gemini skills CLI management command (#15837) --- packages/cli/src/commands/skills.test.tsx | 56 +++++++ packages/cli/src/commands/skills.tsx | 29 ++++ .../cli/src/commands/skills/disable.test.ts | 115 +++++++++++++++ packages/cli/src/commands/skills/disable.ts | 65 +++++++++ .../cli/src/commands/skills/enable.test.ts | 115 +++++++++++++++ packages/cli/src/commands/skills/enable.ts | 65 +++++++++ packages/cli/src/commands/skills/list.test.ts | 138 ++++++++++++++++++ packages/cli/src/commands/skills/list.ts | 61 ++++++++ packages/cli/src/config/config.ts | 5 + packages/cli/src/config/settings.ts | 1 + 10 files changed, 650 insertions(+) create mode 100644 packages/cli/src/commands/skills.test.tsx create mode 100644 packages/cli/src/commands/skills.tsx create mode 100644 packages/cli/src/commands/skills/disable.test.ts create mode 100644 packages/cli/src/commands/skills/disable.ts create mode 100644 packages/cli/src/commands/skills/enable.test.ts create mode 100644 packages/cli/src/commands/skills/enable.ts create mode 100644 packages/cli/src/commands/skills/list.test.ts create mode 100644 packages/cli/src/commands/skills/list.ts diff --git a/packages/cli/src/commands/skills.test.tsx b/packages/cli/src/commands/skills.test.tsx new file mode 100644 index 0000000000..5a76ab0d95 --- /dev/null +++ b/packages/cli/src/commands/skills.test.tsx @@ -0,0 +1,56 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi } from 'vitest'; +import { skillsCommand } from './skills.js'; + +vi.mock('./skills/list.js', () => ({ listCommand: { command: 'list' } })); +vi.mock('./skills/enable.js', () => ({ + enableCommand: { command: 'enable ' }, +})); +vi.mock('./skills/disable.js', () => ({ + disableCommand: { command: 'disable ' }, +})); + +vi.mock('../gemini.js', () => ({ + initializeOutputListenersAndFlush: vi.fn(), +})); + +describe('skillsCommand', () => { + it('should have correct command and aliases', () => { + expect(skillsCommand.command).toBe('skills '); + expect(skillsCommand.aliases).toEqual(['skill']); + expect(skillsCommand.describe).toBe('Manage agent skills.'); + }); + + it('should register all subcommands in builder', () => { + const mockYargs = { + middleware: vi.fn().mockReturnThis(), + command: vi.fn().mockReturnThis(), + demandCommand: vi.fn().mockReturnThis(), + version: vi.fn().mockReturnThis(), + }; + + // @ts-expect-error - Mocking yargs + skillsCommand.builder(mockYargs); + + expect(mockYargs.middleware).toHaveBeenCalled(); + expect(mockYargs.command).toHaveBeenCalledWith({ command: 'list' }); + expect(mockYargs.command).toHaveBeenCalledWith({ + command: 'enable ', + }); + expect(mockYargs.command).toHaveBeenCalledWith({ + command: 'disable ', + }); + expect(mockYargs.demandCommand).toHaveBeenCalledWith(1, expect.any(String)); + expect(mockYargs.version).toHaveBeenCalledWith(false); + }); + + it('should have a handler that does nothing', () => { + // @ts-expect-error - Handler doesn't take arguments in this case + expect(skillsCommand.handler()).toBeUndefined(); + }); +}); diff --git a/packages/cli/src/commands/skills.tsx b/packages/cli/src/commands/skills.tsx new file mode 100644 index 0000000000..2178456481 --- /dev/null +++ b/packages/cli/src/commands/skills.tsx @@ -0,0 +1,29 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { CommandModule } from 'yargs'; +import { listCommand } from './skills/list.js'; +import { enableCommand } from './skills/enable.js'; +import { disableCommand } from './skills/disable.js'; +import { initializeOutputListenersAndFlush } from '../gemini.js'; + +export const skillsCommand: CommandModule = { + command: 'skills ', + aliases: ['skill'], + describe: 'Manage agent skills.', + builder: (yargs) => + yargs + .middleware(() => initializeOutputListenersAndFlush()) + .command(listCommand) + .command(enableCommand) + .command(disableCommand) + .demandCommand(1, 'You need at least one command before continuing.') + .version(false), + handler: () => { + // This handler is not called when a subcommand is provided. + // Yargs will show the help menu. + }, +}; diff --git a/packages/cli/src/commands/skills/disable.test.ts b/packages/cli/src/commands/skills/disable.test.ts new file mode 100644 index 0000000000..20850c6ecc --- /dev/null +++ b/packages/cli/src/commands/skills/disable.test.ts @@ -0,0 +1,115 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { format } from 'node:util'; +import { handleDisable, disableCommand } from './disable.js'; +import { + loadSettings, + SettingScope, + type LoadedSettings, + type LoadableSettingScope, +} from '../../config/settings.js'; + +const emitConsoleLog = vi.hoisted(() => vi.fn()); +const debugLogger = vi.hoisted(() => ({ + log: vi.fn((message, ...args) => { + emitConsoleLog('log', format(message, ...args)); + }), +})); + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + debugLogger, + }; +}); + +vi.mock('../../config/settings.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + loadSettings: vi.fn(), + }; +}); + +vi.mock('../utils.js', () => ({ + exitCli: vi.fn(), +})); + +describe('skills disable command', () => { + const mockLoadSettings = vi.mocked(loadSettings); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('handleDisable', () => { + it('should disable an enabled skill in user scope', async () => { + const mockSettings = { + forScope: vi.fn().mockReturnValue({ + settings: { skills: { disabled: [] } }, + }), + setValue: vi.fn(), + }; + mockLoadSettings.mockReturnValue( + mockSettings as unknown as LoadedSettings, + ); + + await handleDisable({ + name: 'skill1', + scope: SettingScope.User as LoadableSettingScope, + }); + + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'skills.disabled', + ['skill1'], + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + 'Skill "skill1" successfully disabled in scope "User".', + ); + }); + + it('should log a message if the skill is already disabled', async () => { + const mockSettings = { + forScope: vi.fn().mockReturnValue({ + settings: { skills: { disabled: ['skill1'] } }, + }), + setValue: vi.fn(), + }; + mockLoadSettings.mockReturnValue( + mockSettings as unknown as LoadedSettings, + ); + + await handleDisable({ + name: 'skill1', + scope: SettingScope.User as LoadableSettingScope, + }); + + expect(mockSettings.setValue).not.toHaveBeenCalled(); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + 'Skill "skill1" is already disabled in scope "User".', + ); + }); + }); + + describe('disableCommand', () => { + it('should have correct command and describe', () => { + expect(disableCommand.command).toBe('disable '); + expect(disableCommand.describe).toBe('Disables an agent skill.'); + }); + }); +}); diff --git a/packages/cli/src/commands/skills/disable.ts b/packages/cli/src/commands/skills/disable.ts new file mode 100644 index 0000000000..1923d0e989 --- /dev/null +++ b/packages/cli/src/commands/skills/disable.ts @@ -0,0 +1,65 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { CommandModule } from 'yargs'; +import { + loadSettings, + SettingScope, + type LoadableSettingScope, +} from '../../config/settings.js'; +import { debugLogger } from '@google/gemini-cli-core'; +import { exitCli } from '../utils.js'; + +interface DisableArgs { + name: string; + scope: LoadableSettingScope; +} + +export async function handleDisable(args: DisableArgs) { + const { name, scope } = args; + const workspaceDir = process.cwd(); + const settings = loadSettings(workspaceDir); + + const currentDisabled = + settings.forScope(scope).settings.skills?.disabled || []; + + if (currentDisabled.includes(name)) { + debugLogger.log(`Skill "${name}" is already disabled in scope "${scope}".`); + return; + } + + const newDisabled = [...currentDisabled, name]; + settings.setValue(scope, 'skills.disabled', newDisabled); + debugLogger.log(`Skill "${name}" successfully disabled in scope "${scope}".`); +} + +export const disableCommand: CommandModule = { + command: 'disable ', + describe: 'Disables an agent skill.', + builder: (yargs) => + yargs + .positional('name', { + describe: 'The name of the skill to disable.', + type: 'string', + demandOption: true, + }) + .option('scope', { + alias: 's', + describe: 'The scope to disable the skill in (user or project).', + type: 'string', + default: 'user', + choices: ['user', 'project'], + }), + handler: async (argv) => { + const scope: LoadableSettingScope = + argv['scope'] === 'project' ? SettingScope.Workspace : SettingScope.User; + await handleDisable({ + name: argv['name'] as string, + scope, + }); + await exitCli(); + }, +}; diff --git a/packages/cli/src/commands/skills/enable.test.ts b/packages/cli/src/commands/skills/enable.test.ts new file mode 100644 index 0000000000..a720bb0ca9 --- /dev/null +++ b/packages/cli/src/commands/skills/enable.test.ts @@ -0,0 +1,115 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { format } from 'node:util'; +import { handleEnable, enableCommand } from './enable.js'; +import { + loadSettings, + SettingScope, + type LoadedSettings, + type LoadableSettingScope, +} from '../../config/settings.js'; + +const emitConsoleLog = vi.hoisted(() => vi.fn()); +const debugLogger = vi.hoisted(() => ({ + log: vi.fn((message, ...args) => { + emitConsoleLog('log', format(message, ...args)); + }), +})); + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + debugLogger, + }; +}); + +vi.mock('../../config/settings.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + loadSettings: vi.fn(), + }; +}); + +vi.mock('../utils.js', () => ({ + exitCli: vi.fn(), +})); + +describe('skills enable command', () => { + const mockLoadSettings = vi.mocked(loadSettings); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('handleEnable', () => { + it('should enable a disabled skill in user scope', async () => { + const mockSettings = { + forScope: vi.fn().mockReturnValue({ + settings: { skills: { disabled: ['skill1'] } }, + }), + setValue: vi.fn(), + }; + mockLoadSettings.mockReturnValue( + mockSettings as unknown as LoadedSettings, + ); + + await handleEnable({ + name: 'skill1', + scope: SettingScope.User as LoadableSettingScope, + }); + + expect(mockSettings.setValue).toHaveBeenCalledWith( + SettingScope.User, + 'skills.disabled', + [], + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + 'Skill "skill1" successfully enabled in scope "User".', + ); + }); + + it('should log a message if the skill is already enabled', async () => { + const mockSettings = { + forScope: vi.fn().mockReturnValue({ + settings: { skills: { disabled: [] } }, + }), + setValue: vi.fn(), + }; + mockLoadSettings.mockReturnValue( + mockSettings as unknown as LoadedSettings, + ); + + await handleEnable({ + name: 'skill1', + scope: SettingScope.User as LoadableSettingScope, + }); + + expect(mockSettings.setValue).not.toHaveBeenCalled(); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + 'Skill "skill1" is already enabled in scope "User".', + ); + }); + }); + + describe('enableCommand', () => { + it('should have correct command and describe', () => { + expect(enableCommand.command).toBe('enable '); + expect(enableCommand.describe).toBe('Enables an agent skill.'); + }); + }); +}); diff --git a/packages/cli/src/commands/skills/enable.ts b/packages/cli/src/commands/skills/enable.ts new file mode 100644 index 0000000000..7b5e19d88f --- /dev/null +++ b/packages/cli/src/commands/skills/enable.ts @@ -0,0 +1,65 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { CommandModule } from 'yargs'; +import { + loadSettings, + SettingScope, + type LoadableSettingScope, +} from '../../config/settings.js'; +import { debugLogger } from '@google/gemini-cli-core'; +import { exitCli } from '../utils.js'; + +interface EnableArgs { + name: string; + scope: LoadableSettingScope; +} + +export async function handleEnable(args: EnableArgs) { + const { name, scope } = args; + const workspaceDir = process.cwd(); + const settings = loadSettings(workspaceDir); + + const currentDisabled = + settings.forScope(scope).settings.skills?.disabled || []; + const newDisabled = currentDisabled.filter((d) => d !== name); + + if (currentDisabled.length === newDisabled.length) { + debugLogger.log(`Skill "${name}" is already enabled in scope "${scope}".`); + return; + } + + settings.setValue(scope, 'skills.disabled', newDisabled); + debugLogger.log(`Skill "${name}" successfully enabled in scope "${scope}".`); +} + +export const enableCommand: CommandModule = { + command: 'enable ', + describe: 'Enables an agent skill.', + builder: (yargs) => + yargs + .positional('name', { + describe: 'The name of the skill to enable.', + type: 'string', + demandOption: true, + }) + .option('scope', { + alias: 's', + describe: 'The scope to enable the skill in (user or project).', + type: 'string', + default: 'user', + choices: ['user', 'project'], + }), + handler: async (argv) => { + const scope: LoadableSettingScope = + argv['scope'] === 'project' ? SettingScope.Workspace : SettingScope.User; + await handleEnable({ + name: argv['name'] as string, + scope, + }); + await exitCli(); + }, +}; diff --git a/packages/cli/src/commands/skills/list.test.ts b/packages/cli/src/commands/skills/list.test.ts new file mode 100644 index 0000000000..ce5a5d0cf8 --- /dev/null +++ b/packages/cli/src/commands/skills/list.test.ts @@ -0,0 +1,138 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { format } from 'node:util'; +import { handleList, listCommand } from './list.js'; +import { loadSettings, type LoadedSettings } from '../../config/settings.js'; +import { loadCliConfig } from '../../config/config.js'; +import type { Config } from '@google/gemini-cli-core'; +import chalk from 'chalk'; + +const emitConsoleLog = vi.hoisted(() => vi.fn()); +const debugLogger = vi.hoisted(() => ({ + log: vi.fn((message, ...args) => { + emitConsoleLog('log', format(message, ...args)); + }), + error: vi.fn((message, ...args) => { + emitConsoleLog('error', format(message, ...args)); + }), +})); + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + coreEvents: { + emitConsoleLog, + }, + debugLogger, + }; +}); + +vi.mock('../../config/settings.js'); +vi.mock('../../config/config.js'); +vi.mock('../utils.js', () => ({ + exitCli: vi.fn(), +})); + +describe('skills list command', () => { + const mockLoadSettings = vi.mocked(loadSettings); + const mockLoadCliConfig = vi.mocked(loadCliConfig); + + beforeEach(async () => { + vi.clearAllMocks(); + mockLoadSettings.mockReturnValue({ + merged: {}, + } as unknown as LoadedSettings); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('handleList', () => { + it('should log a message if no skills are discovered', async () => { + const mockConfig = { + initialize: vi.fn().mockResolvedValue(undefined), + getSkillManager: vi.fn().mockReturnValue({ + getAllSkills: vi.fn().mockReturnValue([]), + }), + }; + mockLoadCliConfig.mockResolvedValue(mockConfig as unknown as Config); + + await handleList(); + + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + 'No skills discovered.', + ); + }); + + it('should list all discovered skills', async () => { + const skills = [ + { + name: 'skill1', + description: 'desc1', + disabled: false, + location: '/path/to/skill1', + }, + { + name: 'skill2', + description: 'desc2', + disabled: true, + location: '/path/to/skill2', + }, + ]; + const mockConfig = { + initialize: vi.fn().mockResolvedValue(undefined), + getSkillManager: vi.fn().mockReturnValue({ + getAllSkills: vi.fn().mockReturnValue(skills), + }), + }; + mockLoadCliConfig.mockResolvedValue(mockConfig as unknown as Config); + + await handleList(); + + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + chalk.bold('Discovered Agent Skills:'), + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining('skill1'), + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining(chalk.green('[Enabled]')), + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining('skill2'), + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining(chalk.red('[Disabled]')), + ); + }); + + it('should throw an error when listing fails', async () => { + mockLoadCliConfig.mockRejectedValue(new Error('List failed')); + + await expect(handleList()).rejects.toThrow('List failed'); + }); + }); + + describe('listCommand', () => { + const command = listCommand; + + it('should have correct command and describe', () => { + expect(command.command).toBe('list'); + expect(command.describe).toBe('Lists discovered agent skills.'); + }); + }); +}); diff --git a/packages/cli/src/commands/skills/list.ts b/packages/cli/src/commands/skills/list.ts new file mode 100644 index 0000000000..29b234df98 --- /dev/null +++ b/packages/cli/src/commands/skills/list.ts @@ -0,0 +1,61 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { CommandModule } from 'yargs'; +import { debugLogger } from '@google/gemini-cli-core'; +import { loadSettings } from '../../config/settings.js'; +import { loadCliConfig, type CliArgs } from '../../config/config.js'; +import { exitCli } from '../utils.js'; +import chalk from 'chalk'; + +export async function handleList() { + const workspaceDir = process.cwd(); + const settings = loadSettings(workspaceDir); + + const config = await loadCliConfig( + settings.merged, + 'skills-list-session', + { + debug: false, + } as Partial as CliArgs, + { cwd: workspaceDir }, + ); + + // Initialize to trigger extension loading and skill discovery + await config.initialize(); + + const skillManager = config.getSkillManager(); + const skills = skillManager.getAllSkills(); + + if (skills.length === 0) { + debugLogger.log('No skills discovered.'); + return; + } + + debugLogger.log(chalk.bold('Discovered Agent Skills:')); + debugLogger.log(''); + + for (const skill of skills) { + const status = skill.disabled + ? chalk.red('[Disabled]') + : chalk.green('[Enabled]'); + + debugLogger.log(`${chalk.bold(skill.name)} ${status}`); + debugLogger.log(` Description: ${skill.description}`); + debugLogger.log(` Location: ${skill.location}`); + debugLogger.log(''); + } +} + +export const listCommand: CommandModule = { + command: 'list', + describe: 'Lists discovered agent skills.', + builder: (yargs) => yargs, + handler: async () => { + await handleList(); + await exitCli(); + }, +}; diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 8763f95dbf..858a929aff 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -9,6 +9,7 @@ import { hideBin } from 'yargs/helpers'; import process from 'node:process'; import { mcpCommand } from '../commands/mcp.js'; import { extensionsCommand } from '../commands/extensions.js'; +import { skillsCommand } from '../commands/skills.js'; import { hooksCommand } from '../commands/hooks.js'; import { Config, @@ -285,6 +286,10 @@ export async function parseArguments(settings: Settings): Promise { yargsInstance.command(extensionsCommand); } + if (settings?.experimental?.skills ?? false) { + yargsInstance.command(skillsCommand); + } + // Register hooks command if hooks are enabled if (settings?.tools?.enableHooks) { yargsInstance.command(hooksCommand); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 31d5f70733..e0a24cbebf 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -91,6 +91,7 @@ const MIGRATION_MAP: Record = { excludeTools: 'tools.exclude', excludeMCPServers: 'mcp.excluded', excludedProjectEnvVars: 'advanced.excludedEnvVars', + experimentalSkills: 'experimental.skills', extensionManagement: 'experimental.extensionManagement', extensions: 'extensions', fileFiltering: 'context.fileFiltering', From f3625aab1396280792b48e2ebe1ead3b3abd23e4 Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Sun, 4 Jan 2026 23:52:14 -0500 Subject: [PATCH 003/591] refactor: consolidate EditTool and SmartEditTool (#15857) --- packages/core/src/index.ts | 2 +- packages/core/src/telemetry/loggers.test.ts | 8 +- .../src/tools/confirmation-policy.test.ts | 10 - packages/core/src/tools/edit.test.ts | 1198 ----------------- packages/core/src/tools/edit.ts | 629 --------- packages/core/src/tools/smart-edit.test.ts | 106 +- packages/core/src/tools/smart-edit.ts | 33 +- packages/core/src/tools/write-file.test.ts | 2 +- packages/core/src/utils/editCorrector.ts | 2 +- 9 files changed, 139 insertions(+), 1851 deletions(-) delete mode 100644 packages/core/src/tools/edit.test.ts delete mode 100644 packages/core/src/tools/edit.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 4cf9df113a..b0eecd55d0 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -123,7 +123,7 @@ export * from './tools/ls.js'; export * from './tools/grep.js'; export * from './tools/ripGrep.js'; export * from './tools/glob.js'; -export * from './tools/edit.js'; +export * from './tools/smart-edit.js'; export * from './tools/write-file.js'; export * from './tools/web-fetch.js'; export * from './tools/memoryTool.js'; diff --git a/packages/core/src/telemetry/loggers.test.ts b/packages/core/src/telemetry/loggers.test.ts index 3dabc4a89d..84f1ba06b6 100644 --- a/packages/core/src/telemetry/loggers.test.ts +++ b/packages/core/src/telemetry/loggers.test.ts @@ -12,7 +12,7 @@ import type { } from '../index.js'; import { AuthType, - EditTool, + SmartEditTool, GeminiClient, ToolConfirmationOutcome, ToolErrorType, @@ -1034,7 +1034,7 @@ describe('loggers', () => { }); it('should log a tool call with all fields', () => { - const tool = new EditTool(mockConfig, createMockMessageBus()); + const tool = new SmartEditTool(mockConfig, createMockMessageBus()); const call: CompletedToolCall = { status: 'success', request: { @@ -1250,7 +1250,7 @@ describe('loggers', () => { contentLength: 13, }, outcome: ToolConfirmationOutcome.ModifyWithEditor, - tool: new EditTool(mockConfig, createMockMessageBus()), + tool: new SmartEditTool(mockConfig, createMockMessageBus()), invocation: {} as AnyToolInvocation, durationMs: 100, }; @@ -1329,7 +1329,7 @@ describe('loggers', () => { errorType: undefined, contentLength: 13, }, - tool: new EditTool(mockConfig, createMockMessageBus()), + tool: new SmartEditTool(mockConfig, createMockMessageBus()), invocation: {} as AnyToolInvocation, durationMs: 100, }; diff --git a/packages/core/src/tools/confirmation-policy.test.ts b/packages/core/src/tools/confirmation-policy.test.ts index 53e04f6dec..aede2f6e7c 100644 --- a/packages/core/src/tools/confirmation-policy.test.ts +++ b/packages/core/src/tools/confirmation-policy.test.ts @@ -7,7 +7,6 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { EditTool } from './edit.js'; import { SmartEditTool } from './smart-edit.js'; import { WriteFileTool } from './write-file.js'; import { WebFetchTool } from './web-fetch.js'; @@ -81,15 +80,6 @@ describe('Tool Confirmation Policy Updates', () => { }); const tools = [ - { - name: 'EditTool', - create: (config: Config, bus: MessageBus) => new EditTool(config, bus), - params: { - file_path: 'test.txt', - old_string: 'existing', - new_string: 'new', - }, - }, { name: 'SmartEditTool', create: (config: Config, bus: MessageBus) => diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts deleted file mode 100644 index ca1505a2c4..0000000000 --- a/packages/core/src/tools/edit.test.ts +++ /dev/null @@ -1,1198 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -/* eslint-disable @typescript-eslint/no-explicit-any */ - -const mockEnsureCorrectEdit = vi.hoisted(() => vi.fn()); -const mockGenerateJson = vi.hoisted(() => vi.fn()); -const mockOpenDiff = vi.hoisted(() => vi.fn()); - -import { IdeClient } from '../ide/ide-client.js'; - -vi.mock('../ide/ide-client.js', () => ({ - IdeClient: { - getInstance: vi.fn(), - }, -})); - -vi.mock('../utils/editCorrector.js', () => ({ - ensureCorrectEdit: mockEnsureCorrectEdit, -})); - -vi.mock('../core/client.js', () => ({ - GeminiClient: vi.fn().mockImplementation(() => ({ - generateJson: mockGenerateJson, - })), -})); - -vi.mock('../utils/editor.js', () => ({ - openDiff: mockOpenDiff, -})); - -vi.mock('../telemetry/loggers.js', () => ({ - logFileOperation: vi.fn(), -})); - -interface EditFileParameterSchema { - properties: { - file_path: { - description: string; - }; - }; -} - -import type { Mock } from 'vitest'; -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import type { EditToolParams } from './edit.js'; -import { applyReplacement, EditTool } from './edit.js'; -import type { FileDiff } from './tools.js'; -import { ToolConfirmationOutcome } from './tools.js'; -import { ToolErrorType } from './tool-error.js'; -import path from 'node:path'; -import fs from 'node:fs'; -import os from 'node:os'; -import type { Config } from '../config/config.js'; -import { ApprovalMode } from '../policy/types.js'; -import type { Content, Part, SchemaUnion } from '@google/genai'; -import { StandardFileSystemService } from '../services/fileSystemService.js'; -import { WorkspaceContext } from '../utils/workspaceContext.js'; -import { - createMockMessageBus, - getMockMessageBusInstance, -} from '../test-utils/mock-message-bus.js'; - -describe('EditTool', () => { - let tool: EditTool; - let tempDir: string; - let rootDir: string; - let mockConfig: Config; - let geminiClient: any; - let baseLlmClient: any; - - beforeEach(() => { - vi.restoreAllMocks(); - tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'edit-tool-test-')); - rootDir = path.join(tempDir, 'root'); - fs.mkdirSync(rootDir); - - geminiClient = { - generateJson: mockGenerateJson, // mockGenerateJson is already defined and hoisted - }; - - baseLlmClient = { - generateJson: vi.fn(), - }; - - mockConfig = { - getGeminiClient: vi.fn().mockReturnValue(geminiClient), - getBaseLlmClient: vi.fn().mockReturnValue(baseLlmClient), - getTargetDir: () => rootDir, - getApprovalMode: vi.fn(), - setApprovalMode: vi.fn(), - getWorkspaceContext: () => new WorkspaceContext(rootDir), - getFileSystemService: () => new StandardFileSystemService(), - getIdeMode: () => false, - // getGeminiConfig: () => ({ apiKey: 'test-api-key' }), // This was not a real Config method - // Add other properties/methods of Config if EditTool uses them - // Minimal other methods to satisfy Config type if needed by EditTool constructor or other direct uses: - getApiKey: () => 'test-api-key', - getModel: () => 'test-model', - getSandbox: () => false, - getDebugMode: () => false, - getQuestion: () => undefined, - - getToolDiscoveryCommand: () => undefined, - getToolCallCommand: () => undefined, - getMcpServerCommand: () => undefined, - getMcpServers: () => undefined, - getUserAgent: () => 'test-agent', - getUserMemory: () => '', - setUserMemory: vi.fn(), - getGeminiMdFileCount: () => 0, - setGeminiMdFileCount: vi.fn(), - getToolRegistry: () => ({}) as any, // Minimal mock for ToolRegistry - isInteractive: () => false, - } as unknown as Config; - - // Reset mocks before each test - (mockConfig.getApprovalMode as Mock).mockClear(); - // Default to not skipping confirmation - (mockConfig.getApprovalMode as Mock).mockReturnValue(ApprovalMode.DEFAULT); - - // Reset mocks and set default implementation for ensureCorrectEdit - mockEnsureCorrectEdit.mockReset(); - mockEnsureCorrectEdit.mockImplementation( - async (_, currentContent, params) => { - let occurrences = 0; - if (params.old_string && currentContent) { - // Simple string counting for the mock - let index = currentContent.indexOf(params.old_string); - while (index !== -1) { - occurrences++; - index = currentContent.indexOf(params.old_string, index + 1); - } - } else if (params.old_string === '') { - occurrences = 0; // Creating a new file - } - return Promise.resolve({ params, occurrences }); - }, - ); - - // Default mock for generateJson to return the snippet unchanged - mockGenerateJson.mockReset(); - mockGenerateJson.mockImplementation( - async (contents: Content[], schema: SchemaUnion) => { - // The problematic_snippet is the last part of the user's content - const userContent = contents.find((c: Content) => c.role === 'user'); - let promptText = ''; - if (userContent && userContent.parts) { - promptText = userContent.parts - .filter((p: Part) => typeof (p as any).text === 'string') - .map((p: Part) => (p as any).text) - .join('\n'); - } - const snippetMatch = promptText.match( - /Problematic target snippet:\n```\n([\s\S]*?)\n```/, - ); - const problematicSnippet = - snippetMatch && snippetMatch[1] ? snippetMatch[1] : ''; - - if ((schema as any).properties?.corrected_target_snippet) { - return Promise.resolve({ - corrected_target_snippet: problematicSnippet, - }); - } - if ((schema as any).properties?.corrected_new_string) { - // For new_string correction, we might need more sophisticated logic, - // but for now, returning original is a safe default if not specified by a test. - const originalNewStringMatch = promptText.match( - /original_new_string \(what was intended to replace original_old_string\):\n```\n([\s\S]*?)\n```/, - ); - const originalNewString = - originalNewStringMatch && originalNewStringMatch[1] - ? originalNewStringMatch[1] - : ''; - return Promise.resolve({ corrected_new_string: originalNewString }); - } - return Promise.resolve({}); // Default empty object if schema doesn't match - }, - ); - - const bus = createMockMessageBus(); - getMockMessageBusInstance(bus).defaultToolDecision = 'ask_user'; - tool = new EditTool(mockConfig, bus); - }); - - afterEach(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); - }); - - describe('applyReplacement', () => { - it('should return newString if isNewFile is true', () => { - expect(applyReplacement(null, 'old', 'new', true)).toBe('new'); - expect(applyReplacement('existing', 'old', 'new', true)).toBe('new'); - }); - - it('should return newString if currentContent is null and oldString is empty (defensive)', () => { - expect(applyReplacement(null, '', 'new', false)).toBe('new'); - }); - - it('should return empty string if currentContent is null and oldString is not empty (defensive)', () => { - expect(applyReplacement(null, 'old', 'new', false)).toBe(''); - }); - - it('should replace oldString with newString in currentContent', () => { - expect(applyReplacement('hello old world old', 'old', 'new', false)).toBe( - 'hello new world new', - ); - }); - - it('should return currentContent if oldString is empty and not a new file', () => { - expect(applyReplacement('hello world', '', 'new', false)).toBe( - 'hello world', - ); - }); - - it.each([ - { - name: '$ literal', - current: "price is $100 and pattern end is ' '", - oldStr: 'price is $100', - newStr: 'price is $200', - expected: "price is $200 and pattern end is ' '", - }, - { - name: "$' literal", - current: 'foo', - oldStr: 'foo', - newStr: "bar$'baz", - expected: "bar$'baz", - }, - { - name: '$& literal', - current: 'hello world', - oldStr: 'hello', - newStr: '$&-replacement', - expected: '$&-replacement world', - }, - { - name: '$` literal', - current: 'prefix-middle-suffix', - oldStr: 'middle', - newStr: 'new$`content', - expected: 'prefix-new$`content-suffix', - }, - { - name: '$1, $2 capture groups literal', - current: 'test string', - oldStr: 'test', - newStr: '$1$2replacement', - expected: '$1$2replacement string', - }, - { - name: 'normal strings without problematic $', - current: 'normal text replacement', - oldStr: 'text', - newStr: 'string', - expected: 'normal string replacement', - }, - { - name: 'multiple occurrences with $ sequences', - current: 'foo bar foo baz', - oldStr: 'foo', - newStr: "test$'end", - expected: "test$'end bar test$'end baz", - }, - { - name: 'complex regex patterns with $ at end', - current: "| select('match', '^[sv]d[a-z]$')", - oldStr: "'^[sv]d[a-z]$'", - newStr: "'^[sv]d[a-z]$' # updated", - expected: "| select('match', '^[sv]d[a-z]$' # updated)", - }, - { - name: 'empty replacement with problematic $', - current: 'test content', - oldStr: 'nothing', - newStr: "replacement$'text", - expected: 'test content', - }, - { - name: '$$ (escaped dollar)', - current: 'price value', - oldStr: 'value', - newStr: '$$100', - expected: 'price $$100', - }, - ])('should handle $name', ({ current, oldStr, newStr, expected }) => { - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe(expected); - }); - }); - - describe('validateToolParams', () => { - it('should return null for valid params', () => { - const params: EditToolParams = { - file_path: path.join(rootDir, 'test.txt'), - old_string: 'old', - new_string: 'new', - }; - expect(tool.validateToolParams(params)).toBeNull(); - }); - - it('should return error for path outside root', () => { - const params: EditToolParams = { - file_path: path.join(tempDir, 'outside-root.txt'), - old_string: 'old', - new_string: 'new', - }; - const error = tool.validateToolParams(params); - expect(error).toContain( - 'File path must be within one of the workspace directories', - ); - }); - }); - - describe('shouldConfirmExecute', () => { - const testFile = 'edit_me.txt'; - let filePath: string; - - beforeEach(() => { - filePath = path.join(rootDir, testFile); - }); - - it('should resolve relative path and request confirmation', async () => { - fs.writeFileSync(filePath, 'some old content here'); - const params: EditToolParams = { - file_path: testFile, // relative path - old_string: 'old', - new_string: 'new', - }; - // ensureCorrectEdit will be called by shouldConfirmExecute - mockEnsureCorrectEdit.mockResolvedValueOnce({ - params: { ...params, file_path: filePath }, - occurrences: 1, - }); - const invocation = tool.build(params); - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).toEqual( - expect.objectContaining({ - title: `Confirm Edit: ${testFile}`, - fileName: testFile, - fileDiff: expect.any(String), - }), - ); - }); - - it('should request confirmation for valid edit', async () => { - fs.writeFileSync(filePath, 'some old content here'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }; - // ensureCorrectEdit will be called by shouldConfirmExecute - mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 1 }); - const invocation = tool.build(params); - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).toEqual( - expect.objectContaining({ - title: `Confirm Edit: ${testFile}`, - fileName: testFile, - fileDiff: expect.any(String), - }), - ); - }); - - it('should return false if old_string is not found (ensureCorrectEdit returns 0)', async () => { - fs.writeFileSync(filePath, 'some content here'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'not_found', - new_string: 'new', - }; - mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); - const invocation = tool.build(params); - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).toBe(false); - }); - - it('should return false if multiple occurrences of old_string are found (ensureCorrectEdit returns > 1)', async () => { - fs.writeFileSync(filePath, 'old old content here'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }; - mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 2 }); - const invocation = tool.build(params); - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).toBe(false); - }); - - it('should request confirmation for creating a new file (empty old_string)', async () => { - const newFileName = 'new_file.txt'; - const newFilePath = path.join(rootDir, newFileName); - const params: EditToolParams = { - file_path: newFilePath, - old_string: '', - new_string: 'new file content', - }; - // ensureCorrectEdit might not be called if old_string is empty, - // as shouldConfirmExecute handles this for diff generation. - // If it is called, it should return 0 occurrences for a new file. - mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); - const invocation = tool.build(params); - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).toEqual( - expect.objectContaining({ - title: `Confirm Edit: ${newFileName}`, - fileName: newFileName, - fileDiff: expect.any(String), - }), - ); - }); - - it('should use corrected params from ensureCorrectEdit for diff generation', async () => { - const originalContent = 'This is the original string to be replaced.'; - const originalOldString = 'original string'; - const originalNewString = 'new string'; - - const correctedOldString = 'original string to be replaced'; // More specific - const correctedNewString = 'completely new string'; // Different replacement - const expectedFinalContent = 'This is the completely new string.'; - - fs.writeFileSync(filePath, originalContent); - const params: EditToolParams = { - file_path: filePath, - old_string: originalOldString, - new_string: originalNewString, - }; - - // The main beforeEach already calls mockEnsureCorrectEdit.mockReset() - // Set a specific mock for this test case - let mockCalled = false; - mockEnsureCorrectEdit.mockImplementationOnce( - async (_, content, p, client, baseClient) => { - mockCalled = true; - expect(content).toBe(originalContent); - expect(p).toBe(params); - expect(client).toBe(geminiClient); - expect(baseClient).toBe(baseLlmClient); - return { - params: { - file_path: filePath, - old_string: correctedOldString, - new_string: correctedNewString, - }, - occurrences: 1, - }; - }, - ); - const invocation = tool.build(params); - const confirmation = (await invocation.shouldConfirmExecute( - new AbortController().signal, - )) as FileDiff; - - expect(mockCalled).toBe(true); // Check if the mock implementation was run - // expect(mockEnsureCorrectEdit).toHaveBeenCalledWith(originalContent, params, expect.anything()); // Keep this commented for now - expect(confirmation).toEqual( - expect.objectContaining({ - title: `Confirm Edit: ${testFile}`, - fileName: testFile, - }), - ); - // Check that the diff is based on the corrected strings leading to the new state - expect(confirmation.fileDiff).toContain(`-${originalContent}`); - expect(confirmation.fileDiff).toContain(`+${expectedFinalContent}`); - - // Verify that applying the correctedOldString and correctedNewString to originalContent - // indeed produces the expectedFinalContent, which is what the diff should reflect. - const patchedContent = originalContent.replace( - correctedOldString, // This was the string identified by ensureCorrectEdit for replacement - correctedNewString, // This was the string identified by ensureCorrectEdit as the replacement - ); - expect(patchedContent).toBe(expectedFinalContent); - }); - - it('should rethrow calculateEdit errors when the abort signal is triggered', async () => { - const filePath = path.join(rootDir, 'abort-confirmation.txt'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }; - - const invocation = tool.build(params); - const abortController = new AbortController(); - const abortError = new Error('Abort requested'); - - const calculateSpy = vi - .spyOn(invocation as any, 'calculateEdit') - .mockImplementation(async () => { - if (!abortController.signal.aborted) { - abortController.abort(); - } - throw abortError; - }); - - await expect( - invocation.shouldConfirmExecute(abortController.signal), - ).rejects.toBe(abortError); - - calculateSpy.mockRestore(); - }); - }); - - describe('execute', () => { - const testFile = 'execute_me.txt'; - let filePath: string; - - beforeEach(() => { - filePath = path.join(rootDir, testFile); - // Default for execute tests, can be overridden - mockEnsureCorrectEdit.mockImplementation(async (_, content, params) => { - let occurrences = 0; - if (params.old_string && content) { - let index = content.indexOf(params.old_string); - while (index !== -1) { - occurrences++; - index = content.indexOf(params.old_string, index + 1); - } - } else if (params.old_string === '') { - occurrences = 0; - } - return { params, occurrences }; - }); - }); - - it('should resolve relative path and execute successfully', async () => { - const initialContent = 'This is some old text.'; - const newContent = 'This is some new text.'; - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: testFile, // relative path - old_string: 'old', - new_string: 'new', - }; - - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toMatch(/Successfully modified file/); - expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent); - }); - - it('should throw error if file path is empty', async () => { - const params: EditToolParams = { - file_path: '', - old_string: 'old', - new_string: 'new', - }; - expect(() => tool.build(params)).toThrow( - /The 'file_path' parameter must be non-empty./, - ); - }); - - it('should reject when calculateEdit fails after an abort signal', async () => { - const params: EditToolParams = { - file_path: path.join(rootDir, 'abort-execute.txt'), - old_string: 'old', - new_string: 'new', - }; - - const invocation = tool.build(params); - const abortController = new AbortController(); - const abortError = new Error('Abort requested during execute'); - - const calculateSpy = vi - .spyOn(invocation as any, 'calculateEdit') - .mockImplementation(async () => { - if (!abortController.signal.aborted) { - abortController.abort(); - } - throw abortError; - }); - - await expect(invocation.execute(abortController.signal)).rejects.toBe( - abortError, - ); - - calculateSpy.mockRestore(); - }); - - it('should edit an existing file and return diff with fileName', async () => { - const initialContent = 'This is some old text.'; - const newContent = 'This is some new text.'; // old -> new - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }; - - // Specific mock for this test's execution path in calculateEdit - // ensureCorrectEdit is NOT called by calculateEdit, only by shouldConfirmExecute - // So, the default mockEnsureCorrectEdit should correctly return 1 occurrence for 'old' in initialContent - - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toMatch(/Successfully modified file/); - expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent); - const display = result.returnDisplay as FileDiff; - expect(display.fileDiff).toMatch(initialContent); - expect(display.fileDiff).toMatch(newContent); - expect(display.fileName).toBe(testFile); - }); - - it('should create a new file if old_string is empty and file does not exist, and return created message', async () => { - const newFileName = 'brand_new_file.txt'; - const newFilePath = path.join(rootDir, newFileName); - const fileContent = 'Content for the new file.'; - const params: EditToolParams = { - file_path: newFilePath, - old_string: '', - new_string: fileContent, - }; - - (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( - ApprovalMode.AUTO_EDIT, - ); - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toMatch(/Created new file/); - expect(fs.existsSync(newFilePath)).toBe(true); - expect(fs.readFileSync(newFilePath, 'utf8')).toBe(fileContent); - - const display = result.returnDisplay as FileDiff; - expect(display.fileDiff).toMatch(/\+Content for the new file\./); - expect(display.fileName).toBe(newFileName); - expect((result.returnDisplay as FileDiff).diffStat).toStrictEqual({ - model_added_lines: 1, - model_removed_lines: 0, - model_added_chars: 25, - model_removed_chars: 0, - user_added_lines: 0, - user_removed_lines: 0, - user_added_chars: 0, - user_removed_chars: 0, - }); - }); - - it('should return error if old_string is not found in file', async () => { - fs.writeFileSync(filePath, 'Some content.', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'nonexistent', - new_string: 'replacement', - }; - // The default mockEnsureCorrectEdit will return 0 occurrences for 'nonexistent' - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch( - /0 occurrences found for old_string in/, - ); - expect(result.returnDisplay).toMatch( - /Failed to edit, could not find the string to replace./, - ); - }); - - it('should return error if multiple occurrences of old_string are found', async () => { - fs.writeFileSync(filePath, 'multiple old old strings', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }; - // The default mockEnsureCorrectEdit will return 2 occurrences for 'old' - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch( - /Expected 1 occurrence but found 2 for old_string in file/, - ); - expect(result.returnDisplay).toMatch( - /Failed to edit, expected 1 occurrence but found 2/, - ); - }); - - it('should successfully replace multiple occurrences when expected_replacements specified', async () => { - fs.writeFileSync(filePath, 'old text\nold text\nold text', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - expected_replacements: 3, - }; - - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toMatch(/Successfully modified file/); - expect(fs.readFileSync(filePath, 'utf8')).toBe( - 'new text\nnew text\nnew text', - ); - const display = result.returnDisplay as FileDiff; - - expect(display.fileDiff).toMatch(/-old text\n-old text\n-old text/); - expect(display.fileDiff).toMatch(/\+new text\n\+new text\n\+new text/); - expect(display.fileName).toBe(testFile); - expect((result.returnDisplay as FileDiff).diffStat).toStrictEqual({ - model_added_lines: 3, - model_removed_lines: 3, - model_added_chars: 24, - model_removed_chars: 24, - user_added_lines: 0, - user_removed_lines: 0, - user_added_chars: 0, - user_removed_chars: 0, - }); - }); - - it('should return error if expected_replacements does not match actual occurrences', async () => { - fs.writeFileSync(filePath, 'old text old text', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - expected_replacements: 3, // Expecting 3 but only 2 exist - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch( - /Expected 3 occurrences but found 2 for old_string in file/, - ); - expect(result.returnDisplay).toMatch( - /Failed to edit, expected 3 occurrences but found 2/, - ); - }); - - it('should return error if trying to create a file that already exists (empty old_string)', async () => { - fs.writeFileSync(filePath, 'Existing content', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: '', - new_string: 'new content', - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch(/File already exists, cannot create/); - expect(result.returnDisplay).toMatch( - /Attempted to create a file that already exists/, - ); - }); - - it('should include modification message when proposed content is modified', async () => { - const initialContent = 'Line 1\nold line\nLine 3\nLine 4\nLine 5\n'; - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - modified_by_user: true, - ai_proposed_content: 'Line 1\nAI line\nLine 3\nLine 4\nLine 5\n', - }; - - (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( - ApprovalMode.AUTO_EDIT, - ); - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toMatch( - /User modified the `new_string` content/, - ); - expect((result.returnDisplay as FileDiff).diffStat).toStrictEqual({ - model_added_lines: 1, - model_removed_lines: 1, - model_added_chars: 7, - model_removed_chars: 8, - user_added_lines: 1, - user_removed_lines: 1, - user_added_chars: 8, - user_removed_chars: 7, - }); - }); - - it.each([ - { - name: 'modified_by_user is false', - modifiedByUser: false, - }, - { - name: 'modified_by_user is not provided', - modifiedByUser: undefined, - }, - ])( - 'should not include modification message when $name', - async ({ modifiedByUser }) => { - const initialContent = 'This is some old text.'; - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - ...(modifiedByUser !== undefined && { - modified_by_user: modifiedByUser, - }), - }; - - (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( - ApprovalMode.AUTO_EDIT, - ); - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).not.toMatch( - /User modified the `new_string` content/, - ); - }, - ); - - it('should return error if old_string and new_string are identical', async () => { - const initialContent = 'This is some identical text.'; - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'identical', - new_string: 'identical', - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch(/No changes to apply/); - expect(result.returnDisplay).toMatch(/No changes to apply/); - }); - - it('should return EDIT_NO_CHANGE error if replacement results in identical content', async () => { - // This can happen if ensureCorrectEdit finds a fuzzy match, but the literal - // string replacement with `replaceAll` results in no change. - const initialContent = 'line 1\nline 2\nline 3'; // Note the double space - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - // old_string has a single space, so it won't be found by replaceAll - old_string: 'line 1\nline 2\nline 3', - new_string: 'line 1\nnew line 2\nline 3', - }; - - // Mock ensureCorrectEdit to simulate it finding a match (e.g., via fuzzy matching) - // but it doesn't correct the old_string to the literal content. - mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 1 }); - - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_CHANGE); - expect(result.returnDisplay).toMatch( - /No changes to apply. The new content is identical to the current content./, - ); - // Ensure the file was not actually changed - expect(fs.readFileSync(filePath, 'utf8')).toBe(initialContent); - }); - }); - - describe('Error Scenarios', () => { - const testFile = 'error_test.txt'; - let filePath: string; - - beforeEach(() => { - filePath = path.join(rootDir, testFile); - }); - - it.each([ - { - name: 'FILE_NOT_FOUND error', - setup: () => {}, - params: { file_path: '', old_string: 'any', new_string: 'new' }, - expectedError: ToolErrorType.FILE_NOT_FOUND, - isAsyncTest: true, - }, - { - name: 'ATTEMPT_TO_CREATE_EXISTING_FILE error', - setup: (fp: string) => fs.writeFileSync(fp, 'existing content', 'utf8'), - params: { file_path: '', old_string: '', new_string: 'new content' }, - expectedError: ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE, - isAsyncTest: true, - }, - { - name: 'NO_OCCURRENCE_FOUND error', - setup: (fp: string) => fs.writeFileSync(fp, 'content', 'utf8'), - params: { file_path: '', old_string: 'not-found', new_string: 'new' }, - expectedError: ToolErrorType.EDIT_NO_OCCURRENCE_FOUND, - isAsyncTest: true, - }, - { - name: 'EXPECTED_OCCURRENCE_MISMATCH error', - setup: (fp: string) => fs.writeFileSync(fp, 'one one two', 'utf8'), - params: { - file_path: '', - old_string: 'one', - new_string: 'new', - expected_replacements: 3, - }, - expectedError: ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH, - isAsyncTest: true, - }, - { - name: 'NO_CHANGE error', - setup: (fp: string) => fs.writeFileSync(fp, 'content', 'utf8'), - params: { file_path: '', old_string: 'content', new_string: 'content' }, - expectedError: ToolErrorType.EDIT_NO_CHANGE, - isAsyncTest: true, - }, - { - name: 'relative path (should not throw)', - setup: () => {}, - params: { - file_path: 'relative/path.txt', - old_string: 'a', - new_string: 'b', - }, - expectedError: null, - isAsyncTest: false, - }, - { - name: 'FILE_WRITE_FAILURE on write error', - setup: (fp: string) => { - fs.writeFileSync(fp, 'content', 'utf8'); - fs.chmodSync(fp, '444'); - }, - params: { - file_path: '', - old_string: 'content', - new_string: 'new content', - }, - expectedError: ToolErrorType.FILE_WRITE_FAILURE, - isAsyncTest: true, - }, - ])( - 'should return $name', - async ({ setup, params, expectedError, isAsyncTest }) => { - const testParams = { - ...params, - file_path: params.file_path || filePath, - }; - setup(filePath); - - if (!isAsyncTest) { - expect(() => tool.build(testParams)).not.toThrow(); - } else { - const invocation = tool.build(testParams); - const result = await invocation.execute(new AbortController().signal); - expect(result.error?.type).toBe(expectedError); - } - }, - ); - }); - - describe('getDescription', () => { - it.each([ - { - name: 'identical strings (no change)', - fileName: 'test.txt', - oldStr: 'identical_string', - newStr: 'identical_string', - expected: 'No file changes to test.txt', - }, - { - name: 'different strings (full)', - fileName: 'test.txt', - oldStr: 'this is the old string value', - newStr: 'this is the new string value', - expected: - 'test.txt: this is the old string value => this is the new string value', - }, - { - name: 'very short strings', - fileName: 'short.txt', - oldStr: 'old', - newStr: 'new', - expected: 'short.txt: old => new', - }, - { - name: 'long strings (truncated)', - fileName: 'long.txt', - oldStr: - 'this is a very long old string that will definitely be truncated', - newStr: 'this is a very long new string that will also be truncated', - expected: - 'long.txt: this is a very long old string... => this is a very long new string...', - }, - ])('should handle $name', ({ fileName, oldStr, newStr, expected }) => { - const params: EditToolParams = { - file_path: path.join(rootDir, fileName), - old_string: oldStr, - new_string: newStr, - }; - const invocation = tool.build(params); - expect(invocation.getDescription()).toBe(expected); - }); - }); - - describe('workspace boundary validation', () => { - it('should validate paths are within workspace root', () => { - const validPath = { - file_path: path.join(rootDir, 'file.txt'), - old_string: 'old', - new_string: 'new', - }; - expect(tool.validateToolParams(validPath)).toBeNull(); - }); - - it('should reject paths outside workspace root', () => { - const invalidPath = { - file_path: '/etc/passwd', - old_string: 'root', - new_string: 'hacked', - }; - const error = tool.validateToolParams(invalidPath); - expect(error).toContain( - 'File path must be within one of the workspace directories', - ); - expect(error).toContain(rootDir); - }); - }); - - describe('constructor', () => { - afterEach(() => { - vi.restoreAllMocks(); - }); - - it('should use windows-style path examples on windows', () => { - vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); - - const tool = new EditTool( - {} as unknown as Config, - createMockMessageBus(), - ); - const schema = tool.schema; - expect( - (schema.parametersJsonSchema as EditFileParameterSchema).properties - .file_path.description, - ).toBe('The path to the file to modify.'); - }); - - it('should use unix-style path examples on non-windows platforms', () => { - vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); - - const tool = new EditTool( - {} as unknown as Config, - createMockMessageBus(), - ); - const schema = tool.schema; - expect( - (schema.parametersJsonSchema as EditFileParameterSchema).properties - .file_path.description, - ).toBe('The path to the file to modify.'); - }); - }); - - describe('IDE mode', () => { - const testFile = 'edit_me.txt'; - let filePath: string; - let ideClient: any; - - beforeEach(() => { - filePath = path.join(rootDir, testFile); - ideClient = { - openDiff: vi.fn(), - isDiffingEnabled: vi.fn().mockReturnValue(true), - }; - vi.mocked(IdeClient.getInstance).mockResolvedValue(ideClient); - (mockConfig as any).getIdeMode = () => true; - }); - - it('should call ideClient.openDiff and update params on confirmation', async () => { - const initialContent = 'some old content here'; - const newContent = 'some new content here'; - const modifiedContent = 'some modified content here'; - fs.writeFileSync(filePath, initialContent); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }; - mockEnsureCorrectEdit.mockResolvedValueOnce({ - params: { ...params, old_string: 'old', new_string: 'new' }, - occurrences: 1, - }); - ideClient.openDiff.mockResolvedValueOnce({ - status: 'accepted', - content: modifiedContent, - }); - - const invocation = tool.build(params); - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - - expect(ideClient.openDiff).toHaveBeenCalledWith(filePath, newContent); - - if (confirmation && 'onConfirm' in confirmation) { - await confirmation.onConfirm(ToolConfirmationOutcome.ProceedOnce); - } - - expect(params.old_string).toBe(initialContent); - expect(params.new_string).toBe(modifiedContent); - }); - }); - - describe('multiple file edits', () => { - it('should perform multiple removals and report correct diff stats', async () => { - const numFiles = 10; - const files: Array<{ - path: string; - initialContent: string; - toRemove: string; - }> = []; - const expectedLinesRemoved: number[] = []; - const actualLinesRemoved: number[] = []; - - // 1. Create 10 files with 5-10 lines each - for (let i = 0; i < numFiles; i++) { - const fileName = `test-file-${i}.txt`; - const filePath = path.join(rootDir, fileName); - const numLines = Math.floor(Math.random() * 6) + 5; // 5 to 10 lines - const lines = Array.from( - { length: numLines }, - (_, j) => `File ${i}, Line ${j + 1}`, - ); - const content = lines.join('\n') + '\n'; - - // Determine which lines to remove (2 or 3 lines) - const numLinesToRemove = Math.floor(Math.random() * 2) + 2; // 2 or 3 - expectedLinesRemoved.push(numLinesToRemove); - const startLineToRemove = 1; // Start removing from the second line - const linesToRemove = lines.slice( - startLineToRemove, - startLineToRemove + numLinesToRemove, - ); - const toRemove = linesToRemove.join('\n') + '\n'; - - fs.writeFileSync(filePath, content, 'utf8'); - files.push({ - path: filePath, - initialContent: content, - toRemove, - }); - } - - // 2. Create and execute 10 tool calls for removal - for (const file of files) { - const params: EditToolParams = { - file_path: file.path, - old_string: file.toRemove, - new_string: '', // Removing the content - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - if ( - result.returnDisplay && - typeof result.returnDisplay === 'object' && - 'diffStat' in result.returnDisplay && - result.returnDisplay.diffStat - ) { - actualLinesRemoved.push( - result.returnDisplay.diffStat?.model_removed_lines, - ); - } else if (result.error) { - throw result.error; - } - } - - // 3. Assert that the content was removed from each file - for (const file of files) { - const finalContent = fs.readFileSync(file.path, 'utf8'); - const expectedContent = file.initialContent.replace(file.toRemove, ''); - expect(finalContent).toBe(expectedContent); - expect(finalContent).not.toContain(file.toRemove); - } - - // 4. Assert that the total number of removed lines matches the diffStat total - const totalExpectedRemoved = expectedLinesRemoved.reduce( - (sum, current) => sum + current, - 0, - ); - const totalActualRemoved = actualLinesRemoved.reduce( - (sum, current) => sum + current, - 0, - ); - expect(totalActualRemoved).toBe(totalExpectedRemoved); - }); - }); -}); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts deleted file mode 100644 index 838bbc6c6e..0000000000 --- a/packages/core/src/tools/edit.ts +++ /dev/null @@ -1,629 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import * as fs from 'node:fs'; -import * as path from 'node:path'; -import * as Diff from 'diff'; -import type { - ToolCallConfirmationDetails, - ToolEditConfirmationDetails, - ToolInvocation, - ToolLocation, - ToolResult, -} from './tools.js'; -import { - BaseDeclarativeTool, - BaseToolInvocation, - Kind, - ToolConfirmationOutcome, -} from './tools.js'; -import type { MessageBus } from '../confirmation-bus/message-bus.js'; -import { ToolErrorType } from './tool-error.js'; -import { makeRelative, shortenPath } from '../utils/paths.js'; -import { isNodeError } from '../utils/errors.js'; -import type { Config } from '../config/config.js'; -import { ApprovalMode } from '../policy/types.js'; - -import { ensureCorrectEdit } from '../utils/editCorrector.js'; -import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; -import { logFileOperation } from '../telemetry/loggers.js'; -import { FileOperationEvent } from '../telemetry/types.js'; -import { FileOperation } from '../telemetry/metrics.js'; -import { getSpecificMimeType } from '../utils/fileUtils.js'; -import { getLanguageFromFilePath } from '../utils/language-detection.js'; -import type { - ModifiableDeclarativeTool, - ModifyContext, -} from './modifiable-tool.js'; -import { IdeClient } from '../ide/ide-client.js'; -import { safeLiteralReplace } from '../utils/textUtils.js'; -import { EDIT_TOOL_NAME, READ_FILE_TOOL_NAME } from './tool-names.js'; -import { debugLogger } from '../utils/debugLogger.js'; - -export function applyReplacement( - currentContent: string | null, - oldString: string, - newString: string, - isNewFile: boolean, -): string { - if (isNewFile) { - return newString; - } - if (currentContent === null) { - // Should not happen if not a new file, but defensively return empty or newString if oldString is also empty - return oldString === '' ? newString : ''; - } - // If oldString is empty and it's not a new file, do not modify the content. - if (oldString === '' && !isNewFile) { - return currentContent; - } - - // Use intelligent replacement that handles $ sequences safely - return safeLiteralReplace(currentContent, oldString, newString); -} - -/** - * Parameters for the Edit tool - */ -export interface EditToolParams { - /** - * The path to the file to modify - */ - file_path: string; - - /** - * The text to replace - */ - old_string: string; - - /** - * The text to replace it with - */ - new_string: string; - - /** - * Number of replacements expected. Defaults to 1 if not specified. - * Use when you want to replace multiple occurrences. - */ - expected_replacements?: number; - - /** - * Whether the edit was modified manually by the user. - */ - modified_by_user?: boolean; - - /** - * Initially proposed content. - */ - ai_proposed_content?: string; -} - -interface CalculatedEdit { - currentContent: string | null; - newContent: string; - occurrences: number; - error?: { display: string; raw: string; type: ToolErrorType }; - isNewFile: boolean; -} - -class EditToolInvocation - extends BaseToolInvocation - implements ToolInvocation -{ - private readonly resolvedPath: string; - - constructor( - private readonly config: Config, - params: EditToolParams, - messageBus: MessageBus, - toolName?: string, - displayName?: string, - ) { - super(params, messageBus, toolName, displayName); - this.resolvedPath = path.resolve( - this.config.getTargetDir(), - this.params.file_path, - ); - } - - override toolLocations(): ToolLocation[] { - return [{ path: this.resolvedPath }]; - } - - /** - * Calculates the potential outcome of an edit operation. - * @param params Parameters for the edit operation - * @returns An object describing the potential edit outcome - * @throws File system errors if reading the file fails unexpectedly (e.g., permissions) - */ - private async calculateEdit( - params: EditToolParams, - abortSignal: AbortSignal, - ): Promise { - const expectedReplacements = params.expected_replacements ?? 1; - let currentContent: string | null = null; - let fileExists = false; - let isNewFile = false; - let finalNewString = params.new_string; - let finalOldString = params.old_string; - let occurrences = 0; - let error: - | { display: string; raw: string; type: ToolErrorType } - | undefined = undefined; - - try { - currentContent = await this.config - .getFileSystemService() - .readTextFile(this.resolvedPath); - // Normalize line endings to LF for consistent processing. - currentContent = currentContent.replace(/\r\n/g, '\n'); - fileExists = true; - } catch (err: unknown) { - if (!isNodeError(err) || err.code !== 'ENOENT') { - // Rethrow unexpected FS errors (permissions, etc.) - throw err; - } - fileExists = false; - } - - if (params.old_string === '' && !fileExists) { - // Creating a new file - isNewFile = true; - } else if (!fileExists) { - // Trying to edit a nonexistent file (and old_string is not empty) - error = { - display: `File not found. Cannot apply edit. Use an empty old_string to create a new file.`, - raw: `File not found: ${this.resolvedPath}`, - type: ToolErrorType.FILE_NOT_FOUND, - }; - } else if (currentContent !== null) { - // Editing an existing file - const correctedEdit = await ensureCorrectEdit( - this.resolvedPath, - currentContent, - params, - this.config.getGeminiClient(), - this.config.getBaseLlmClient(), - abortSignal, - ); - finalOldString = correctedEdit.params.old_string; - finalNewString = correctedEdit.params.new_string; - occurrences = correctedEdit.occurrences; - - if (params.old_string === '') { - // Error: Trying to create a file that already exists - error = { - display: `Failed to edit. Attempted to create a file that already exists.`, - raw: `File already exists, cannot create: ${this.resolvedPath}`, - type: ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE, - }; - } else if (occurrences === 0) { - error = { - display: `Failed to edit, could not find the string to replace.`, - raw: `Failed to edit, 0 occurrences found for old_string in ${this.resolvedPath}. No edits made. The exact text in old_string was not found. Ensure you're not escaping content incorrectly and check whitespace, indentation, and context. Use ${READ_FILE_TOOL_NAME} tool to verify.`, - type: ToolErrorType.EDIT_NO_OCCURRENCE_FOUND, - }; - } else if (occurrences !== expectedReplacements) { - const occurrenceTerm = - expectedReplacements === 1 ? 'occurrence' : 'occurrences'; - - error = { - display: `Failed to edit, expected ${expectedReplacements} ${occurrenceTerm} but found ${occurrences}.`, - raw: `Failed to edit, Expected ${expectedReplacements} ${occurrenceTerm} but found ${occurrences} for old_string in file: ${this.resolvedPath}`, - type: ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH, - }; - } else if (finalOldString === finalNewString) { - error = { - display: `No changes to apply. The old_string and new_string are identical.`, - raw: `No changes to apply. The old_string and new_string are identical in file: ${this.resolvedPath}`, - type: ToolErrorType.EDIT_NO_CHANGE, - }; - } - } else { - // Should not happen if fileExists and no exception was thrown, but defensively: - error = { - display: `Failed to read content of file.`, - raw: `Failed to read content of existing file: ${this.resolvedPath}`, - type: ToolErrorType.READ_CONTENT_FAILURE, - }; - } - - const newContent = !error - ? applyReplacement( - currentContent, - finalOldString, - finalNewString, - isNewFile, - ) - : (currentContent ?? ''); - - if (!error && fileExists && currentContent === newContent) { - error = { - display: - 'No changes to apply. The new content is identical to the current content.', - raw: `No changes to apply. The new content is identical to the current content in file: ${this.resolvedPath}`, - type: ToolErrorType.EDIT_NO_CHANGE, - }; - } - - return { - currentContent, - newContent, - occurrences, - error, - isNewFile, - }; - } - - /** - * Handles the confirmation prompt for the Edit tool in the CLI. - * It needs to calculate the diff to show the user. - */ - protected override async getConfirmationDetails( - abortSignal: AbortSignal, - ): Promise { - if (this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) { - return false; - } - - let editData: CalculatedEdit; - try { - editData = await this.calculateEdit(this.params, abortSignal); - } catch (error) { - if (abortSignal.aborted) { - throw error; - } - const errorMsg = error instanceof Error ? error.message : String(error); - debugLogger.log(`Error preparing edit: ${errorMsg}`); - return false; - } - - if (editData.error) { - debugLogger.log(`Error: ${editData.error.display}`); - return false; - } - - const fileName = path.basename(this.resolvedPath); - const fileDiff = Diff.createPatch( - fileName, - editData.currentContent ?? '', - editData.newContent, - 'Current', - 'Proposed', - DEFAULT_DIFF_OPTIONS, - ); - const ideClient = await IdeClient.getInstance(); - const ideConfirmation = - this.config.getIdeMode() && ideClient.isDiffingEnabled() - ? ideClient.openDiff(this.resolvedPath, editData.newContent) - : undefined; - - const confirmationDetails: ToolEditConfirmationDetails = { - type: 'edit', - title: `Confirm Edit: ${shortenPath(makeRelative(this.params.file_path, this.config.getTargetDir()))}`, - fileName, - filePath: this.resolvedPath, - fileDiff, - originalContent: editData.currentContent, - newContent: editData.newContent, - onConfirm: async (outcome: ToolConfirmationOutcome) => { - if (outcome === ToolConfirmationOutcome.ProceedAlways) { - // No need to publish a policy update as the default policy for - // AUTO_EDIT already reflects always approving edit. - this.config.setApprovalMode(ApprovalMode.AUTO_EDIT); - } else { - await this.publishPolicyUpdate(outcome); - } - - if (ideConfirmation) { - const result = await ideConfirmation; - if (result.status === 'accepted' && result.content) { - // TODO(chrstn): See https://github.com/google-gemini/gemini-cli/pull/5618#discussion_r2255413084 - // for info on a possible race condition where the file is modified on disk while being edited. - this.params.old_string = editData.currentContent ?? ''; - this.params.new_string = result.content; - } - } - }, - ideConfirmation, - }; - return confirmationDetails; - } - - getDescription(): string { - const relativePath = makeRelative( - this.params.file_path, - this.config.getTargetDir(), - ); - if (this.params.old_string === '') { - return `Create ${shortenPath(relativePath)}`; - } - - const oldStringSnippet = - this.params.old_string.split('\n')[0].substring(0, 30) + - (this.params.old_string.length > 30 ? '...' : ''); - const newStringSnippet = - this.params.new_string.split('\n')[0].substring(0, 30) + - (this.params.new_string.length > 30 ? '...' : ''); - - if (this.params.old_string === this.params.new_string) { - return `No file changes to ${shortenPath(relativePath)}`; - } - return `${shortenPath(relativePath)}: ${oldStringSnippet} => ${newStringSnippet}`; - } - - /** - * Executes the edit operation with the given parameters. - * @param params Parameters for the edit operation - * @returns Result of the edit operation - */ - async execute(signal: AbortSignal): Promise { - let editData: CalculatedEdit; - try { - editData = await this.calculateEdit(this.params, signal); - } catch (error) { - if (signal.aborted) { - throw error; - } - const errorMsg = error instanceof Error ? error.message : String(error); - return { - llmContent: `Error preparing edit: ${errorMsg}`, - returnDisplay: `Error preparing edit: ${errorMsg}`, - error: { - message: errorMsg, - type: ToolErrorType.EDIT_PREPARATION_FAILURE, - }, - }; - } - - if (editData.error) { - return { - llmContent: editData.error.raw, - returnDisplay: `Error: ${editData.error.display}`, - error: { - message: editData.error.raw, - type: editData.error.type, - }, - }; - } - - try { - this.ensureParentDirectoriesExist(this.resolvedPath); - await this.config - .getFileSystemService() - .writeTextFile(this.resolvedPath, editData.newContent); - - const fileName = path.basename(this.resolvedPath); - const originallyProposedContent = - this.params.ai_proposed_content || editData.newContent; - const diffStat = getDiffStat( - fileName, - editData.currentContent ?? '', - originallyProposedContent, - editData.newContent, - ); - - const fileDiff = Diff.createPatch( - fileName, - editData.currentContent ?? '', // Should not be null here if not isNewFile - editData.newContent, - 'Current', - 'Proposed', - DEFAULT_DIFF_OPTIONS, - ); - const displayResult = { - fileDiff, - fileName, - originalContent: editData.currentContent, - newContent: editData.newContent, - diffStat, - }; - - // Log file operation for telemetry (without diff_stat to avoid double-counting) - const mimetype = getSpecificMimeType(this.resolvedPath); - const programmingLanguage = getLanguageFromFilePath(this.resolvedPath); - const extension = path.extname(this.resolvedPath); - const operation = editData.isNewFile - ? FileOperation.CREATE - : FileOperation.UPDATE; - - logFileOperation( - this.config, - new FileOperationEvent( - EDIT_TOOL_NAME, - operation, - editData.newContent.split('\n').length, - mimetype, - extension, - programmingLanguage, - ), - ); - - const llmSuccessMessageParts = [ - editData.isNewFile - ? `Created new file: ${this.resolvedPath} with provided content.` - : `Successfully modified file: ${this.resolvedPath} (${editData.occurrences} replacements).`, - ]; - if (this.params.modified_by_user) { - llmSuccessMessageParts.push( - `User modified the \`new_string\` content to be: ${this.params.new_string}.`, - ); - } - - return { - llmContent: llmSuccessMessageParts.join(' '), - returnDisplay: displayResult, - }; - } catch (error) { - const errorMsg = error instanceof Error ? error.message : String(error); - return { - llmContent: `Error executing edit: ${errorMsg}`, - returnDisplay: `Error writing file: ${errorMsg}`, - error: { - message: errorMsg, - type: ToolErrorType.FILE_WRITE_FAILURE, - }, - }; - } - } - - /** - * Creates parent directories if they don't exist - */ - private ensureParentDirectoriesExist(filePath: string): void { - const dirName = path.dirname(filePath); - if (!fs.existsSync(dirName)) { - fs.mkdirSync(dirName, { recursive: true }); - } - } -} - -/** - * Implementation of the Edit tool logic - */ -export class EditTool - extends BaseDeclarativeTool - implements ModifiableDeclarativeTool -{ - static readonly Name = EDIT_TOOL_NAME; - - constructor( - private readonly config: Config, - messageBus: MessageBus, - ) { - super( - EditTool.Name, - 'Edit', - `Replaces text within a file. By default, replaces a single occurrence, but can replace multiple occurrences when \`expected_replacements\` is specified. This tool requires providing significant context around the change to ensure precise targeting. Always use the ${READ_FILE_TOOL_NAME} tool to examine the file's current content before attempting a text replacement. - - The user has the ability to modify the \`new_string\` content. If modified, this will be stated in the response. - -Expectation for required parameters: -1. \`file_path\` is the path to the file to modify. -2. \`old_string\` MUST be the exact literal text to replace (including all whitespace, indentation, newlines, and surrounding code etc.). -3. \`new_string\` MUST be the exact literal text to replace \`old_string\` with (also including all whitespace, indentation, newlines, and surrounding code etc.). Ensure the resulting code is correct and idiomatic. -4. NEVER escape \`old_string\` or \`new_string\`, that would break the exact literal text requirement. -**Important:** If ANY of the above are not satisfied, the tool will fail. CRITICAL for \`old_string\`: Must uniquely identify the single instance to change. Include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. If this string matches multiple locations, or does not match exactly, the tool will fail. -**Multiple replacements:** Set \`expected_replacements\` to the number of occurrences you want to replace. The tool will replace ALL occurrences that match \`old_string\` exactly. Ensure the number of replacements matches your expectation.`, - Kind.Edit, - { - properties: { - file_path: { - description: 'The path to the file to modify.', - type: 'string', - }, - old_string: { - description: - 'The exact literal text to replace, preferably unescaped. For single replacements (default), include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. For multiple replacements, specify expected_replacements parameter. If this string is not the exact literal text (i.e. you escaped it) or does not match exactly, the tool will fail.', - type: 'string', - }, - new_string: { - description: - 'The exact literal text to replace `old_string` with, preferably unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic.', - type: 'string', - }, - expected_replacements: { - type: 'number', - description: - 'Number of replacements expected. Defaults to 1 if not specified. Use when you want to replace multiple occurrences.', - minimum: 1, - }, - }, - required: ['file_path', 'old_string', 'new_string'], - type: 'object', - }, - messageBus, - true, // isOutputMarkdown - false, // canUpdateOutput - ); - } - - /** - * Validates the parameters for the Edit tool - * @param params Parameters to validate - * @returns Error message string or null if valid - */ - protected override validateToolParamValues( - params: EditToolParams, - ): string | null { - if (!params.file_path) { - return "The 'file_path' parameter must be non-empty."; - } - - const resolvedPath = path.resolve( - this.config.getTargetDir(), - params.file_path, - ); - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(resolvedPath)) { - const directories = workspaceContext.getDirectories(); - return `File path must be within one of the workspace directories: ${directories.join(', ')}`; - } - - return null; - } - - protected createInvocation( - params: EditToolParams, - messageBus: MessageBus, - toolName?: string, - displayName?: string, - ): ToolInvocation { - return new EditToolInvocation( - this.config, - params, - messageBus, - toolName ?? this.name, - displayName ?? this.displayName, - ); - } - - getModifyContext(_: AbortSignal): ModifyContext { - const resolvePath = (filePath: string) => - path.resolve(this.config.getTargetDir(), filePath); - - return { - getFilePath: (params: EditToolParams) => params.file_path, - getCurrentContent: async (params: EditToolParams): Promise => { - try { - return await this.config - .getFileSystemService() - .readTextFile(resolvePath(params.file_path)); - } catch (err) { - if (!isNodeError(err) || err.code !== 'ENOENT') throw err; - return ''; - } - }, - getProposedContent: async (params: EditToolParams): Promise => { - try { - const currentContent = await this.config - .getFileSystemService() - .readTextFile(resolvePath(params.file_path)); - return applyReplacement( - currentContent, - params.old_string, - params.new_string, - params.old_string === '' && currentContent === '', - ); - } catch (err) { - if (!isNodeError(err) || err.code !== 'ENOENT') throw err; - return ''; - } - }, - createUpdatedParams: ( - oldContent: string, - modifiedProposedContent: string, - originalParams: EditToolParams, - ): EditToolParams => ({ - ...originalParams, - ai_proposed_content: oldContent, - old_string: oldContent, - new_string: modifiedProposedContent, - modified_by_user: true, - }), - }; - } -} diff --git a/packages/core/src/tools/smart-edit.test.ts b/packages/core/src/tools/smart-edit.test.ts index 9c76d77ee4..41bd3d0379 100644 --- a/packages/core/src/tools/smart-edit.test.ts +++ b/packages/core/src/tools/smart-edit.test.ts @@ -45,6 +45,7 @@ import { import { SmartEditTool, type EditToolParams, + applyReplacement, calculateReplacement, } from './smart-edit.js'; import { type FileDiff, ToolConfirmationOutcome } from './tools.js'; @@ -178,6 +179,109 @@ describe('SmartEditTool', () => { fs.rmSync(tempDir, { recursive: true, force: true }); }); + describe('applyReplacement', () => { + it('should return newString if isNewFile is true', () => { + expect(applyReplacement(null, 'old', 'new', true)).toBe('new'); + expect(applyReplacement('existing', 'old', 'new', true)).toBe('new'); + }); + + it('should return newString if currentContent is null and oldString is empty (defensive)', () => { + expect(applyReplacement(null, '', 'new', false)).toBe('new'); + }); + + it('should return empty string if currentContent is null and oldString is not empty (defensive)', () => { + expect(applyReplacement(null, 'old', 'new', false)).toBe(''); + }); + + it('should replace oldString with newString in currentContent', () => { + expect(applyReplacement('hello old world old', 'old', 'new', false)).toBe( + 'hello new world new', + ); + }); + + it('should return currentContent if oldString is empty and not a new file', () => { + expect(applyReplacement('hello world', '', 'new', false)).toBe( + 'hello world', + ); + }); + + it.each([ + { + name: '$ literal', + current: "price is $100 and pattern end is ' '", + oldStr: 'price is $100', + newStr: 'price is $200', + expected: "price is $200 and pattern end is ' '", + }, + { + name: "$' literal", + current: 'foo', + oldStr: 'foo', + newStr: "bar$'baz", + expected: "bar$'baz", + }, + { + name: '$& literal', + current: 'hello world', + oldStr: 'hello', + newStr: '$&-replacement', + expected: '$&-replacement world', + }, + { + name: '$` literal', + current: 'prefix-middle-suffix', + oldStr: 'middle', + newStr: 'new$`content', + expected: 'prefix-new$`content-suffix', + }, + { + name: '$1, $2 capture groups literal', + current: 'test string', + oldStr: 'test', + newStr: '$1$2replacement', + expected: '$1$2replacement string', + }, + { + name: 'normal strings without problematic $', + current: 'normal text replacement', + oldStr: 'text', + newStr: 'string', + expected: 'normal string replacement', + }, + { + name: 'multiple occurrences with $ sequences', + current: 'foo bar foo baz', + oldStr: 'foo', + newStr: "test$'end", + expected: "test$'end bar test$'end baz", + }, + { + name: 'complex regex patterns with $ at end', + current: "| select('match', '^[sv]d[a-z]$')", + oldStr: "'^[sv]d[a-z]$'", + newStr: "'^[sv]d[a-z]$' # updated", + expected: "| select('match', '^[sv]d[a-z]$' # updated)", + }, + { + name: 'empty replacement with problematic $', + current: 'test content', + oldStr: 'nothing', + newStr: "replacement$'text", + expected: 'test content', + }, + { + name: '$$ (escaped dollar)', + current: 'price value', + oldStr: 'value', + newStr: '$$100', + expected: 'price $$100', + }, + ])('should handle $name', ({ current, oldStr, newStr, expected }) => { + const result = applyReplacement(current, oldStr, newStr, false); + expect(result).toBe(expected); + }); + }); + describe('calculateReplacement', () => { const abortSignal = new AbortController().signal; @@ -719,7 +823,7 @@ describe('SmartEditTool', () => { instruction: `Remove lines from the file`, old_string: file.toRemove, new_string: '', // Removing the content - ai_proposed_string: '', + ai_proposed_content: '', }; const invocation = tool.build(params); const result = await invocation.execute(new AbortController().signal); diff --git a/packages/core/src/tools/smart-edit.ts b/packages/core/src/tools/smart-edit.ts index aee3a115f8..dbff323381 100644 --- a/packages/core/src/tools/smart-edit.ts +++ b/packages/core/src/tools/smart-edit.ts @@ -34,7 +34,6 @@ import { } from './modifiable-tool.js'; import { IdeClient } from '../ide/ide-client.js'; import { FixLLMEditWithInstruction } from '../utils/llm-edit-fixer.js'; -import { applyReplacement } from './edit.js'; import { safeLiteralReplace } from '../utils/textUtils.js'; import { SmartEditStrategyEvent } from '../telemetry/types.js'; import { logSmartEditStrategy } from '../telemetry/loggers.js'; @@ -57,6 +56,28 @@ interface ReplacementResult { finalNewString: string; } +export function applyReplacement( + currentContent: string | null, + oldString: string, + newString: string, + isNewFile: boolean, +): string { + if (isNewFile) { + return newString; + } + if (currentContent === null) { + // Should not happen if not a new file, but defensively return empty or newString if oldString is also empty + return oldString === '' ? newString : ''; + } + // If oldString is empty and it's not a new file, do not modify the content. + if (oldString === '' && !isNewFile) { + return currentContent; + } + + // Use intelligent replacement that handles $ sequences safely + return safeLiteralReplace(currentContent, oldString, newString); +} + /** * Creates a SHA256 hash of the given content. * @param content The string content to hash. @@ -357,7 +378,7 @@ export interface EditToolParams { /** * The instruction for what needs to be done. */ - instruction: string; + instruction?: string; /** * Whether the edit was modified manually by the user. @@ -365,9 +386,9 @@ export interface EditToolParams { modified_by_user?: boolean; /** - * Initially proposed string. + * Initially proposed content. */ - ai_proposed_string?: string; + ai_proposed_content?: string; } interface CalculatedEdit { @@ -423,7 +444,7 @@ class EditToolInvocation } const fixedEdit = await FixLLMEditWithInstruction( - params.instruction, + params.instruction ?? 'Apply the requested edit.', params.old_string, params.new_string, errorForLlmEditFixer, @@ -1003,7 +1024,7 @@ A good instruction should concisely answer: const content = originalParams.new_string; return { ...originalParams, - ai_proposed_string: content, + ai_proposed_content: content, old_string: oldContent, new_string: modifiedProposedContent, modified_by_user: true, diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index cd5436e7be..85f0b6837a 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -23,7 +23,7 @@ import type { ToolResult, } from './tools.js'; import { ToolConfirmationOutcome } from './tools.js'; -import { type EditToolParams } from './edit.js'; +import { type EditToolParams } from './smart-edit.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../policy/types.js'; import type { ToolRegistry } from './tool-registry.js'; diff --git a/packages/core/src/utils/editCorrector.ts b/packages/core/src/utils/editCorrector.ts index 99019e2b60..1851a8df87 100644 --- a/packages/core/src/utils/editCorrector.ts +++ b/packages/core/src/utils/editCorrector.ts @@ -7,7 +7,7 @@ import type { Content } from '@google/genai'; import type { GeminiClient } from '../core/client.js'; import type { BaseLlmClient } from '../core/baseLlmClient.js'; -import type { EditToolParams } from '../tools/edit.js'; +import type { EditToolParams } from '../tools/smart-edit.js'; import { EDIT_TOOL_NAME, GREP_TOOL_NAME, From 615b218ff7026a6e41ff22967c548fa4ed2e3408 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Sun, 4 Jan 2026 21:19:33 -0800 Subject: [PATCH 004/591] fix(cli): mock fs.readdir in consent tests for Windows compatibility (#15904) --- .../cli/src/config/extensions/consent.test.ts | 61 +++++++++++++------ .../core/src/utils/shell-permissions.test.ts | 31 ++++++++++ 2 files changed, 72 insertions(+), 20 deletions(-) diff --git a/packages/cli/src/config/extensions/consent.test.ts b/packages/cli/src/config/extensions/consent.test.ts index 72a0b79fb6..ccb569f43e 100644 --- a/packages/cli/src/config/extensions/consent.test.ts +++ b/packages/cli/src/config/extensions/consent.test.ts @@ -27,12 +27,26 @@ const mockReadline = vi.hoisted(() => ({ }), })); +const mockReaddir = vi.hoisted(() => vi.fn()); +const originalReaddir = vi.hoisted(() => ({ + current: null as typeof fs.readdir | null, +})); + // Mocking readline for non-interactive prompts vi.mock('node:readline', () => ({ default: mockReadline, createInterface: mockReadline.createInterface, })); +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + originalReaddir.current = actual.readdir; + return { + ...actual, + readdir: mockReaddir, + }; +}); + vi.mock('@google/gemini-cli-core', async (importOriginal) => { const actual = await importOriginal(); @@ -49,6 +63,10 @@ describe('consent', () => { beforeEach(async () => { vi.clearAllMocks(); + if (originalReaddir.current) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + mockReaddir.mockImplementation(originalReaddir.current as any); + } tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'consent-test-')); }); @@ -328,7 +346,7 @@ describe('consent', () => { it('should show a warning if the skill directory cannot be read', async () => { const lockedDir = path.join(tempDir, 'locked'); - await fs.mkdir(lockedDir, { recursive: true, mode: 0o000 }); + await fs.mkdir(lockedDir, { recursive: true }); const skill: SkillDefinition = { name: 'locked-skill', @@ -337,26 +355,29 @@ describe('consent', () => { body: 'body', }; - const requestConsent = vi.fn().mockResolvedValue(true); - try { - await maybeRequestConsentOrFail( - baseConfig, - requestConsent, - false, - undefined, - false, - [skill], - ); + // Mock readdir to simulate a permission error. + // We do this instead of using fs.mkdir(..., { mode: 0o000 }) because + // directory permissions work differently on Windows and 0o000 doesn't + // effectively block access there, leading to test failures in Windows CI. + mockReaddir.mockRejectedValueOnce( + new Error('EACCES: permission denied, scandir'), + ); - expect(requestConsent).toHaveBeenCalledWith( - expect.stringContaining( - ` (Location: ${skill.location}) ${chalk.red('⚠️ (Could not count items in directory)')}`, - ), - ); - } finally { - // Restore permissions so cleanup works - await fs.chmod(lockedDir, 0o700); - } + const requestConsent = vi.fn().mockResolvedValue(true); + await maybeRequestConsentOrFail( + baseConfig, + requestConsent, + false, + undefined, + false, + [skill], + ); + + expect(requestConsent).toHaveBeenCalledWith( + expect.stringContaining( + ` (Location: ${skill.location}) ${chalk.red('⚠️ (Could not count items in directory)')}`, + ), + ); }); }); }); diff --git a/packages/core/src/utils/shell-permissions.test.ts b/packages/core/src/utils/shell-permissions.test.ts index 7f7cf1f46e..a80afdbd7a 100644 --- a/packages/core/src/utils/shell-permissions.test.ts +++ b/packages/core/src/utils/shell-permissions.test.ts @@ -33,6 +33,15 @@ vi.mock('os', () => ({ homedir: mockHomedir, })); +const mockSpawnSync = vi.hoisted(() => vi.fn()); +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawnSync: mockSpawnSync, + }; +}); + const mockQuote = vi.hoisted(() => vi.fn()); vi.mock('shell-quote', () => ({ quote: mockQuote, @@ -464,12 +473,34 @@ describeWindowsOnly('PowerShell integration', () => { }); it('should block commands when PowerShell parser reports errors', () => { + // Mock spawnSync to avoid the overhead of spawning a real PowerShell process, + // which can lead to timeouts in CI environments even on Windows. + mockSpawnSync.mockReturnValue({ + status: 0, + stdout: JSON.stringify({ success: false }), + }); + const { allowed, reason } = isCommandAllowed('Get-ChildItem |', config); expect(allowed).toBe(false); expect(reason).toBe( 'Command rejected because it could not be parsed safely', ); }); + + it('should allow valid commands through PowerShell parser', () => { + // Mock spawnSync to avoid the overhead of spawning a real PowerShell process, + // which can lead to timeouts in CI environments even on Windows. + mockSpawnSync.mockReturnValue({ + status: 0, + stdout: JSON.stringify({ + success: true, + commands: [{ name: 'Get-ChildItem', text: 'Get-ChildItem' }], + }), + }); + + const { allowed } = isCommandAllowed('Get-ChildItem', config); + expect(allowed).toBe(true); + }); }); describe('isShellInvocationAllowlisted', () => { From b4b49e7029d3afb602557599e7b85137d9c0dc61 Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Mon, 5 Jan 2026 00:48:41 -0500 Subject: [PATCH 005/591] refactor(core): Extract and integrate ToolExecutor (#15900) --- packages/core/src/core/coreToolScheduler.ts | 240 +++----------- packages/core/src/index.ts | 1 + .../core/src/scheduler/tool-executor.test.ts | 299 +++++++++++++++++ packages/core/src/scheduler/tool-executor.ts | 310 ++++++++++++++++++ 4 files changed, 657 insertions(+), 193 deletions(-) create mode 100644 packages/core/src/scheduler/tool-executor.test.ts create mode 100644 packages/core/src/scheduler/tool-executor.ts diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index cace893f4b..20afc07b2c 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -5,7 +5,6 @@ */ import { - type ToolResult, type ToolResultDisplay, type AnyDeclarativeTool, type AnyToolInvocation, @@ -15,13 +14,11 @@ import { } from '../tools/tools.js'; import type { EditorType } from '../utils/editor.js'; import type { Config } from '../config/config.js'; -import type { AnsiOutput } from '../utils/terminalSerializer.js'; import { ApprovalMode } from '../policy/types.js'; -import { logToolCall, logToolOutputTruncated } from '../telemetry/loggers.js'; +import { logToolCall } from '../telemetry/loggers.js'; import { ToolErrorType } from '../tools/tool-error.js'; -import { ToolCallEvent, ToolOutputTruncatedEvent } from '../telemetry/types.js'; +import { ToolCallEvent } from '../telemetry/types.js'; import { runInDevTraceSpan } from '../telemetry/trace.js'; -import { SHELL_TOOL_NAME } from '../tools/tool-names.js'; import type { ModifyContext } from '../tools/modifiable-tool.js'; import { isModifiableDeclarativeTool, @@ -34,14 +31,10 @@ import { getToolSuggestion, } from '../utils/tool-utils.js'; import { isShellInvocationAllowlisted } from '../utils/shell-permissions.js'; -import { ShellToolInvocation } from '../tools/shell.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'; -import { - fireToolNotificationHook, - executeToolWithHooks, -} from './coreToolHookTriggers.js'; +import { fireToolNotificationHook } from './coreToolHookTriggers.js'; import { type ToolCall, type ValidatingToolCall, @@ -60,8 +53,7 @@ import { type ToolCallRequestInfo, type ToolCallResponseInfo, } from '../scheduler/types.js'; -import { saveTruncatedContent } from '../utils/fileUtils.js'; -import { convertToFunctionResponse } from '../utils/generateContentResponseUtilities.js'; +import { ToolExecutor } from '../scheduler/tool-executor.js'; export type { ToolCall, @@ -136,6 +128,7 @@ export class CoreToolScheduler { }> = []; private toolCallQueue: ToolCall[] = []; private completedToolCallsForBatch: CompletedToolCall[] = []; + private toolExecutor: ToolExecutor; constructor(options: CoreToolSchedulerOptions) { this.config = options.config; @@ -143,6 +136,7 @@ export class CoreToolScheduler { this.onAllToolCallsComplete = options.onAllToolCallsComplete; this.onToolCallsUpdate = options.onToolCallsUpdate; this.getPreferredEditor = options.getPreferredEditor; + this.toolExecutor = new ToolExecutor(this.config); // Subscribe to message bus for ASK_USER policy decisions // Use a static WeakMap to ensure we only subscribe ONCE per MessageBus instance @@ -847,188 +841,48 @@ export class CoreToolScheduler { for (const toolCall of callsToExecute) { if (toolCall.status !== 'scheduled') continue; - const scheduledCall = toolCall; - const { callId, name: toolName } = scheduledCall.request; - const invocation = scheduledCall.invocation; - this.setStatusInternal(callId, 'executing', signal); - - const liveOutputCallback = - scheduledCall.tool.canUpdateOutput && this.outputUpdateHandler - ? (outputChunk: string | AnsiOutput) => { - if (this.outputUpdateHandler) { - this.outputUpdateHandler(callId, outputChunk); - } - this.toolCalls = this.toolCalls.map((tc) => - tc.request.callId === callId && tc.status === 'executing' - ? { ...tc, liveOutput: outputChunk } - : tc, - ); - this.notifyToolCallsUpdate(); - } - : undefined; - - const shellExecutionConfig = this.config.getShellExecutionConfig(); - const hooksEnabled = this.config.getEnableHooks(); - const messageBus = this.config.getMessageBus(); - - await runInDevTraceSpan( - { - name: toolCall.tool.name, - attributes: { type: 'tool-call' }, - }, - async ({ metadata: spanMetadata }) => { - spanMetadata.input = { - request: toolCall.request, - }; - // TODO: Refactor to remove special casing for ShellToolInvocation. - // Introduce a generic callbacks object for the execute method to handle - // things like `onPid` and `onLiveOutput`. This will make the scheduler - // agnostic to the invocation type. - let promise: Promise; - if (invocation instanceof ShellToolInvocation) { - const setPidCallback = (pid: number) => { - this.toolCalls = this.toolCalls.map((tc) => - tc.request.callId === callId && tc.status === 'executing' - ? { ...tc, pid } - : tc, - ); - this.notifyToolCallsUpdate(); - }; - promise = executeToolWithHooks( - invocation, - toolName, - signal, - messageBus, - hooksEnabled, - toolCall.tool, - liveOutputCallback, - shellExecutionConfig, - setPidCallback, - ); - } else { - promise = executeToolWithHooks( - invocation, - toolName, - signal, - messageBus, - hooksEnabled, - toolCall.tool, - liveOutputCallback, - shellExecutionConfig, - ); - } - - try { - const toolResult: ToolResult = await promise; - spanMetadata.output = toolResult; - if (signal.aborted) { - this.setStatusInternal( - callId, - 'cancelled', - signal, - 'User cancelled tool execution.', - ); - } else if (toolResult.error === undefined) { - let content = toolResult.llmContent; - let outputFile: string | undefined = undefined; - const contentLength = - typeof content === 'string' ? content.length : undefined; - if ( - typeof content === 'string' && - toolName === SHELL_TOOL_NAME && - this.config.getEnableToolOutputTruncation() && - this.config.getTruncateToolOutputThreshold() > 0 && - this.config.getTruncateToolOutputLines() > 0 - ) { - const originalContentLength = content.length; - const threshold = - this.config.getTruncateToolOutputThreshold(); - const lines = this.config.getTruncateToolOutputLines(); - const truncatedResult = await saveTruncatedContent( - content, - callId, - this.config.storage.getProjectTempDir(), - threshold, - lines, - ); - content = truncatedResult.content; - outputFile = truncatedResult.outputFile; - - if (outputFile) { - logToolOutputTruncated( - this.config, - new ToolOutputTruncatedEvent( - scheduledCall.request.prompt_id, - { - toolName, - originalContentLength, - truncatedContentLength: content.length, - threshold, - lines, - }, - ), - ); - } - } - - const response = convertToFunctionResponse( - toolName, - callId, - content, - this.config.getActiveModel(), - ); - const successResponse: ToolCallResponseInfo = { - callId, - responseParts: response, - resultDisplay: toolResult.returnDisplay, - error: undefined, - errorType: undefined, - outputFile, - contentLength, - }; - this.setStatusInternal( - callId, - 'success', - signal, - successResponse, - ); - } else { - // It is a failure - const error = new Error(toolResult.error.message); - const errorResponse = createErrorResponse( - scheduledCall.request, - error, - toolResult.error.type, - ); - this.setStatusInternal(callId, 'error', signal, errorResponse); - } - } catch (executionError: unknown) { - spanMetadata.error = executionError; - if (signal.aborted) { - this.setStatusInternal( - callId, - 'cancelled', - signal, - 'User cancelled tool execution.', - ); - } else { - this.setStatusInternal( - callId, - 'error', - signal, - createErrorResponse( - scheduledCall.request, - executionError instanceof Error - ? executionError - : new Error(String(executionError)), - ToolErrorType.UNHANDLED_EXCEPTION, - ), - ); - } - } - await this.checkAndNotifyCompletion(signal); - }, + this.setStatusInternal(toolCall.request.callId, 'executing', signal); + const executingCall = this.toolCalls.find( + (c) => c.request.callId === toolCall.request.callId, ); + + if (!executingCall) { + // Should not happen, but safe guard + continue; + } + + const completedCall = await this.toolExecutor.execute({ + call: executingCall, + signal, + outputUpdateHandler: (callId, output) => { + if (this.outputUpdateHandler) { + this.outputUpdateHandler(callId, output); + } + this.toolCalls = this.toolCalls.map((tc) => + tc.request.callId === callId && tc.status === 'executing' + ? { ...tc, liveOutput: output } + : tc, + ); + this.notifyToolCallsUpdate(); + }, + onUpdateToolCall: (updatedCall) => { + this.toolCalls = this.toolCalls.map((tc) => + tc.request.callId === updatedCall.request.callId + ? updatedCall + : tc, + ); + this.notifyToolCallsUpdate(); + }, + }); + + this.toolCalls = this.toolCalls.map((tc) => + tc.request.callId === completedCall.request.callId + ? completedCall + : tc, + ); + this.notifyToolCallsUpdate(); + + await this.checkAndNotifyCompletion(signal); } } } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b0eecd55d0..c7166e2c5a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -36,6 +36,7 @@ export * from './core/turn.js'; export * from './core/geminiRequest.js'; export * from './core/coreToolScheduler.js'; export * from './scheduler/types.js'; +export * from './scheduler/tool-executor.js'; export * from './core/nonInteractiveToolExecutor.js'; export * from './core/recordingContentGenerator.js'; diff --git a/packages/core/src/scheduler/tool-executor.test.ts b/packages/core/src/scheduler/tool-executor.test.ts new file mode 100644 index 0000000000..426b5abd30 --- /dev/null +++ b/packages/core/src/scheduler/tool-executor.test.ts @@ -0,0 +1,299 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { ToolExecutor } from './tool-executor.js'; +import type { Config } from '../index.js'; +import type { ToolResult } from '../tools/tools.js'; +import { makeFakeConfig } from '../test-utils/config.js'; +import { MockTool } from '../test-utils/mock-tool.js'; +import type { ScheduledToolCall } from './types.js'; +import type { AnyToolInvocation } from '../index.js'; +import { SHELL_TOOL_NAME } from '../tools/tool-names.js'; +import * as fileUtils from '../utils/fileUtils.js'; +import * as coreToolHookTriggers from '../core/coreToolHookTriggers.js'; +import { ShellToolInvocation } from '../tools/shell.js'; +import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; + +// Mock file utils +vi.mock('../utils/fileUtils.js', () => ({ + saveTruncatedContent: vi.fn(), +})); + +// Mock executeToolWithHooks +vi.mock('../core/coreToolHookTriggers.js', () => ({ + executeToolWithHooks: vi.fn(), +})); + +describe('ToolExecutor', () => { + let config: Config; + let executor: ToolExecutor; + + beforeEach(() => { + // Use the standard fake config factory + config = makeFakeConfig(); + executor = new ToolExecutor(config); + + // Reset mocks + vi.resetAllMocks(); + + // Default mock implementation for saveTruncatedContent + vi.mocked(fileUtils.saveTruncatedContent).mockImplementation( + async (_content, _callId, _tempDir, _threshold, _lines) => ({ + content: 'TruncatedContent...', + outputFile: '/tmp/truncated_output.txt', + }), + ); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should execute a tool successfully', async () => { + const mockTool = new MockTool({ + name: 'testTool', + execute: async () => ({ + llmContent: 'Tool output', + returnDisplay: 'Tool output', + }), + }); + const invocation = mockTool.build({}); + + // Mock executeToolWithHooks to return success + vi.mocked(coreToolHookTriggers.executeToolWithHooks).mockResolvedValue({ + llmContent: 'Tool output', + returnDisplay: 'Tool output', + } as ToolResult); + + const scheduledCall: ScheduledToolCall = { + status: 'scheduled', + request: { + callId: 'call-1', + name: 'testTool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-1', + }, + tool: mockTool, + invocation: invocation as unknown as AnyToolInvocation, + startTime: Date.now(), + }; + + const onUpdateToolCall = vi.fn(); + const result = await executor.execute({ + call: scheduledCall, + signal: new AbortController().signal, + onUpdateToolCall, + }); + + expect(result.status).toBe('success'); + if (result.status === 'success') { + const response = result.response.responseParts[0]?.functionResponse + ?.response as Record; + expect(response).toEqual({ output: 'Tool output' }); + } + }); + + it('should handle execution errors', async () => { + const mockTool = new MockTool({ + name: 'failTool', + }); + const invocation = mockTool.build({}); + + // Mock executeToolWithHooks to throw + vi.mocked(coreToolHookTriggers.executeToolWithHooks).mockRejectedValue( + new Error('Tool Failed'), + ); + + const scheduledCall: ScheduledToolCall = { + status: 'scheduled', + request: { + callId: 'call-2', + name: 'failTool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-2', + }, + tool: mockTool, + invocation: invocation as unknown as AnyToolInvocation, + startTime: Date.now(), + }; + + const result = await executor.execute({ + call: scheduledCall, + signal: new AbortController().signal, + onUpdateToolCall: vi.fn(), + }); + + expect(result.status).toBe('error'); + if (result.status === 'error') { + expect(result.response.error?.message).toBe('Tool Failed'); + } + }); + + it('should return cancelled result when signal is aborted', async () => { + const mockTool = new MockTool({ + name: 'slowTool', + }); + const invocation = mockTool.build({}); + + // Mock executeToolWithHooks to simulate slow execution or cancellation check + vi.mocked(coreToolHookTriggers.executeToolWithHooks).mockImplementation( + async () => { + await new Promise((r) => setTimeout(r, 100)); + return { llmContent: 'Done', returnDisplay: 'Done' }; + }, + ); + + const scheduledCall: ScheduledToolCall = { + status: 'scheduled', + request: { + callId: 'call-3', + name: 'slowTool', + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-3', + }, + tool: mockTool, + invocation: invocation as unknown as AnyToolInvocation, + startTime: Date.now(), + }; + + const controller = new AbortController(); + const promise = executor.execute({ + call: scheduledCall, + signal: controller.signal, + onUpdateToolCall: vi.fn(), + }); + + controller.abort(); + const result = await promise; + + expect(result.status).toBe('cancelled'); + }); + + it('should truncate large shell output', async () => { + // 1. Setup Config for Truncation + vi.spyOn(config, 'getEnableToolOutputTruncation').mockReturnValue(true); + vi.spyOn(config, 'getTruncateToolOutputThreshold').mockReturnValue(10); + vi.spyOn(config, 'getTruncateToolOutputLines').mockReturnValue(5); + + const mockTool = new MockTool({ name: SHELL_TOOL_NAME }); + const invocation = mockTool.build({}); + const longOutput = 'This is a very long output that should be truncated.'; + + // 2. Mock execution returning long content + vi.mocked(coreToolHookTriggers.executeToolWithHooks).mockResolvedValue({ + llmContent: longOutput, + returnDisplay: longOutput, + }); + + const scheduledCall: ScheduledToolCall = { + status: 'scheduled', + request: { + callId: 'call-trunc', + name: SHELL_TOOL_NAME, + args: { command: 'echo long' }, + isClientInitiated: false, + prompt_id: 'prompt-trunc', + }, + tool: mockTool, + invocation: invocation as unknown as AnyToolInvocation, + startTime: Date.now(), + }; + + // 3. Execute + const result = await executor.execute({ + call: scheduledCall, + signal: new AbortController().signal, + onUpdateToolCall: vi.fn(), + }); + + // 4. Verify Truncation Logic + expect(fileUtils.saveTruncatedContent).toHaveBeenCalledWith( + longOutput, + 'call-trunc', + expect.any(String), // temp dir + 10, // threshold + 5, // lines + ); + + expect(result.status).toBe('success'); + if (result.status === 'success') { + const response = result.response.responseParts[0]?.functionResponse + ?.response as Record; + // The content should be the *truncated* version returned by the mock saveTruncatedContent + expect(response).toEqual({ output: 'TruncatedContent...' }); + expect(result.response.outputFile).toBe('/tmp/truncated_output.txt'); + } + }); + + it('should report PID updates for shell tools', async () => { + // 1. Setup ShellToolInvocation + const messageBus = createMockMessageBus(); + const shellInvocation = new ShellToolInvocation( + config, + { command: 'sleep 10' }, + messageBus, + ); + // We need a dummy tool that matches the invocation just for structure + const mockTool = new MockTool({ name: SHELL_TOOL_NAME }); + + // 2. Mock executeToolWithHooks to trigger the PID callback + const testPid = 12345; + vi.mocked(coreToolHookTriggers.executeToolWithHooks).mockImplementation( + async ( + _inv, + _name, + _sig, + _bus, + _hooks, + _tool, + _liveCb, + _shellCfg, + setPidCallback, + ) => { + // Simulate the shell tool reporting a PID + if (setPidCallback) { + setPidCallback(testPid); + } + return { llmContent: 'done', returnDisplay: 'done' }; + }, + ); + + const scheduledCall: ScheduledToolCall = { + status: 'scheduled', + request: { + callId: 'call-pid', + name: SHELL_TOOL_NAME, + args: { command: 'sleep 10' }, + isClientInitiated: false, + prompt_id: 'prompt-pid', + }, + tool: mockTool, + invocation: shellInvocation, + startTime: Date.now(), + }; + + const onUpdateToolCall = vi.fn(); + + // 3. Execute + await executor.execute({ + call: scheduledCall, + signal: new AbortController().signal, + onUpdateToolCall, + }); + + // 4. Verify PID was reported + expect(onUpdateToolCall).toHaveBeenCalledWith( + expect.objectContaining({ + status: 'executing', + pid: testPid, + }), + ); + }); +}); diff --git a/packages/core/src/scheduler/tool-executor.ts b/packages/core/src/scheduler/tool-executor.ts new file mode 100644 index 0000000000..8334168b93 --- /dev/null +++ b/packages/core/src/scheduler/tool-executor.ts @@ -0,0 +1,310 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { + ToolCallRequestInfo, + ToolCallResponseInfo, + ToolResult, + Config, + AnsiOutput, +} from '../index.js'; +import { + ToolErrorType, + ToolOutputTruncatedEvent, + logToolOutputTruncated, + runInDevTraceSpan, +} from '../index.js'; +import { SHELL_TOOL_NAME } from '../tools/tool-names.js'; +import { ShellToolInvocation } from '../tools/shell.js'; +import { executeToolWithHooks } from '../core/coreToolHookTriggers.js'; +import { saveTruncatedContent } from '../utils/fileUtils.js'; +import { convertToFunctionResponse } from '../utils/generateContentResponseUtilities.js'; +import type { + CompletedToolCall, + ToolCall, + ExecutingToolCall, + ErroredToolCall, + SuccessfulToolCall, + CancelledToolCall, +} from './types.js'; + +export interface ToolExecutionContext { + call: ToolCall; + signal: AbortSignal; + outputUpdateHandler?: (callId: string, output: string | AnsiOutput) => void; + onUpdateToolCall: (updatedCall: ToolCall) => void; +} + +export class ToolExecutor { + constructor(private readonly config: Config) {} + + async execute(context: ToolExecutionContext): Promise { + const { call, signal, outputUpdateHandler, onUpdateToolCall } = context; + const { request } = call; + const toolName = request.name; + const callId = request.callId; + + if (!('tool' in call) || !call.tool || !('invocation' in call)) { + throw new Error( + `Cannot execute tool call ${callId}: Tool or Invocation missing.`, + ); + } + const { tool, invocation } = call; + + // Setup live output handling + const liveOutputCallback = + tool.canUpdateOutput && outputUpdateHandler + ? (outputChunk: string | AnsiOutput) => { + outputUpdateHandler(callId, outputChunk); + } + : undefined; + + const shellExecutionConfig = this.config.getShellExecutionConfig(); + const hooksEnabled = this.config.getEnableHooks(); + const messageBus = this.config.getMessageBus(); + + return runInDevTraceSpan( + { + name: tool.name, + attributes: { type: 'tool-call' }, + }, + async ({ metadata: spanMetadata }) => { + spanMetadata.input = { request }; + + try { + let promise: Promise; + if (invocation instanceof ShellToolInvocation) { + const setPidCallback = (pid: number) => { + const executingCall: ExecutingToolCall = { + ...call, + status: 'executing', + tool, + invocation, + pid, + startTime: 'startTime' in call ? call.startTime : undefined, + }; + onUpdateToolCall(executingCall); + }; + promise = executeToolWithHooks( + invocation, + toolName, + signal, + messageBus, + hooksEnabled, + tool, + liveOutputCallback, + shellExecutionConfig, + setPidCallback, + ); + } else { + promise = executeToolWithHooks( + invocation, + toolName, + signal, + messageBus, + hooksEnabled, + tool, + liveOutputCallback, + shellExecutionConfig, + ); + } + + const toolResult: ToolResult = await promise; + spanMetadata.output = toolResult; + + if (signal.aborted) { + return this.createCancelledResult( + call, + 'User cancelled tool execution.', + ); + } else if (toolResult.error === undefined) { + return await this.createSuccessResult(call, toolResult); + } else { + return this.createErrorResult( + call, + new Error(toolResult.error.message), + toolResult.error.type, + ); + } + } catch (executionError: unknown) { + spanMetadata.error = executionError; + if (signal.aborted) { + return this.createCancelledResult( + call, + 'User cancelled tool execution.', + ); + } + const error = + executionError instanceof Error + ? executionError + : new Error(String(executionError)); + return this.createErrorResult( + call, + error, + ToolErrorType.UNHANDLED_EXCEPTION, + ); + } + }, + ); + } + + private createCancelledResult( + call: ToolCall, + reason: string, + ): CancelledToolCall { + const errorMessage = `[Operation Cancelled] ${reason}`; + const startTime = 'startTime' in call ? call.startTime : undefined; + + if (!('tool' in call) || !('invocation' in call)) { + // This should effectively never happen in execution phase, but we handle + // it safely + throw new Error('Cancelled tool call missing tool/invocation references'); + } + + return { + status: 'cancelled', + request: call.request, + response: { + callId: call.request.callId, + responseParts: [ + { + functionResponse: { + id: call.request.callId, + name: call.request.name, + response: { error: errorMessage }, + }, + }, + ], + resultDisplay: undefined, + error: undefined, + errorType: undefined, + contentLength: errorMessage.length, + }, + tool: call.tool, + invocation: call.invocation, + durationMs: startTime ? Date.now() - startTime : undefined, + outcome: call.outcome, + }; + } + + private async createSuccessResult( + call: ToolCall, + toolResult: ToolResult, + ): Promise { + let content = toolResult.llmContent; + let outputFile: string | undefined; + const toolName = call.request.name; + const callId = call.request.callId; + + if ( + typeof content === 'string' && + toolName === SHELL_TOOL_NAME && + this.config.getEnableToolOutputTruncation() && + this.config.getTruncateToolOutputThreshold() > 0 && + this.config.getTruncateToolOutputLines() > 0 + ) { + const originalContentLength = content.length; + const threshold = this.config.getTruncateToolOutputThreshold(); + const lines = this.config.getTruncateToolOutputLines(); + const truncatedResult = await saveTruncatedContent( + content, + callId, + this.config.storage.getProjectTempDir(), + threshold, + lines, + ); + content = truncatedResult.content; + outputFile = truncatedResult.outputFile; + + if (outputFile) { + logToolOutputTruncated( + this.config, + new ToolOutputTruncatedEvent(call.request.prompt_id, { + toolName, + originalContentLength, + truncatedContentLength: content.length, + threshold, + lines, + }), + ); + } + } + + const response = convertToFunctionResponse( + toolName, + callId, + content, + this.config.getActiveModel(), + ); + + const successResponse: ToolCallResponseInfo = { + callId, + responseParts: response, + resultDisplay: toolResult.returnDisplay, + error: undefined, + errorType: undefined, + outputFile, + contentLength: typeof content === 'string' ? content.length : undefined, + }; + + const startTime = 'startTime' in call ? call.startTime : undefined; + // Ensure we have tool and invocation + if (!('tool' in call) || !('invocation' in call)) { + throw new Error('Successful tool call missing tool or invocation'); + } + + return { + status: 'success', + request: call.request, + tool: call.tool, + response: successResponse, + invocation: call.invocation, + durationMs: startTime ? Date.now() - startTime : undefined, + outcome: call.outcome, + }; + } + + private createErrorResult( + call: ToolCall, + error: Error, + errorType?: ToolErrorType, + ): ErroredToolCall { + const response = this.createErrorResponse(call.request, error, errorType); + const startTime = 'startTime' in call ? call.startTime : undefined; + + return { + status: 'error', + request: call.request, + response, + tool: call.tool, + durationMs: startTime ? Date.now() - startTime : undefined, + outcome: call.outcome, + }; + } + + private createErrorResponse( + request: ToolCallRequestInfo, + error: Error, + errorType: ToolErrorType | undefined, + ): ToolCallResponseInfo { + return { + callId: request.callId, + error, + responseParts: [ + { + functionResponse: { + id: request.callId, + name: request.name, + response: { error: error.message }, + }, + }, + ], + resultDisplay: error.message, + errorType, + contentLength: error.message.length, + }; + } +} From 3997c7ff803c4694732ce097eb1d41bc024c0f80 Mon Sep 17 00:00:00 2001 From: Christian Gunderman Date: Mon, 5 Jan 2026 11:02:55 -0800 Subject: [PATCH 006/591] Fix terminal hang when user exits browser without logging in (#15748) --- packages/core/src/code_assist/oauth2.test.ts | 135 +++++++++++++++++++ packages/core/src/code_assist/oauth2.ts | 36 ++++- 2 files changed, 170 insertions(+), 1 deletion(-) diff --git a/packages/core/src/code_assist/oauth2.test.ts b/packages/core/src/code_assist/oauth2.test.ts index f4b12f8cf2..8e37537763 100644 --- a/packages/core/src/code_assist/oauth2.test.ts +++ b/packages/core/src/code_assist/oauth2.test.ts @@ -29,6 +29,7 @@ import { FORCE_ENCRYPTED_FILE_ENV_VAR } from '../mcp/token-storage/index.js'; import { GEMINI_DIR } from '../utils/paths.js'; import { debugLogger } from '../utils/debugLogger.js'; import { writeToStdout } from '../utils/stdio.js'; +import { FatalCancellationError } from '../utils/errors.js'; vi.mock('os', async (importOriginal) => { const os = await importOriginal(); @@ -296,6 +297,7 @@ describe('oauth2', () => { generateAuthUrl: mockGenerateAuthUrl, getToken: mockGetToken, generateCodeVerifierAsync: mockGenerateCodeVerifierAsync, + getAccessToken: vi.fn().mockResolvedValue({ token: 'test-token' }), on: vi.fn(), credentials: {}, } as unknown as OAuth2Client; @@ -1100,6 +1102,139 @@ describe('oauth2', () => { }); }); + describe('cancellation', () => { + it('should cancel when SIGINT is received', async () => { + const mockAuthUrl = 'https://example.com/auth'; + const mockState = 'test-state'; + const mockOAuth2Client = { + generateAuthUrl: vi.fn().mockReturnValue(mockAuthUrl), + on: vi.fn(), + } as unknown as OAuth2Client; + vi.mocked(OAuth2Client).mockImplementation(() => mockOAuth2Client); + + vi.spyOn(crypto, 'randomBytes').mockReturnValue(mockState as never); + vi.mocked(open).mockImplementation( + async () => ({ on: vi.fn() }) as never, + ); + + // Mock createServer to return a server that doesn't do anything (keeps promise pending) + const mockHttpServer = { + listen: vi.fn(), + close: vi.fn(), + on: vi.fn(), + address: () => ({ port: 3000 }), + }; + (http.createServer as Mock).mockImplementation( + () => mockHttpServer as unknown as http.Server, + ); + + // Spy on process.on to capture the SIGINT handler + let sigIntHandler: (() => void) | undefined; + const originalOn = process.on; + const processOnSpy = vi + .spyOn(process, 'on') + .mockImplementation( + ( + event: string | symbol, + listener: (...args: unknown[]) => void, + ) => { + if (event === 'SIGINT') { + sigIntHandler = listener as () => void; + } + return originalOn.call(process, event, listener); + }, + ); + const processRemoveListenerSpy = vi.spyOn(process, 'removeListener'); + + const clientPromise = getOauthClient( + AuthType.LOGIN_WITH_GOOGLE, + mockConfig, + ); + + // Wait a tick to ensure the SIGINT handler is registered + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(sigIntHandler).toBeDefined(); + + // Trigger SIGINT + if (sigIntHandler) { + sigIntHandler(); + } + + await expect(clientPromise).rejects.toThrow(FatalCancellationError); + expect(processRemoveListenerSpy).toHaveBeenCalledWith( + 'SIGINT', + expect.any(Function), + ); + + processOnSpy.mockRestore(); + processRemoveListenerSpy.mockRestore(); + }); + + it('should cancel when Ctrl+C (0x03) is received on stdin', async () => { + const mockAuthUrl = 'https://example.com/auth'; + const mockState = 'test-state'; + const mockOAuth2Client = { + generateAuthUrl: vi.fn().mockReturnValue(mockAuthUrl), + on: vi.fn(), + } as unknown as OAuth2Client; + vi.mocked(OAuth2Client).mockImplementation(() => mockOAuth2Client); + + vi.spyOn(crypto, 'randomBytes').mockReturnValue(mockState as never); + vi.mocked(open).mockImplementation( + async () => ({ on: vi.fn() }) as never, + ); + + const mockHttpServer = { + listen: vi.fn(), + close: vi.fn(), + on: vi.fn(), + address: () => ({ port: 3000 }), + }; + (http.createServer as Mock).mockImplementation( + () => mockHttpServer as unknown as http.Server, + ); + + // Spy on process.stdin.on + let dataHandler: ((data: Buffer) => void) | undefined; + const stdinOnSpy = vi + .spyOn(process.stdin, 'on') + .mockImplementation((event: string | symbol, listener) => { + if (event === 'data') { + dataHandler = listener as (data: Buffer) => void; + } + return process.stdin; + }); + const stdinRemoveListenerSpy = vi.spyOn( + process.stdin, + 'removeListener', + ); + + const clientPromise = getOauthClient( + AuthType.LOGIN_WITH_GOOGLE, + mockConfig, + ); + + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(dataHandler).toBeDefined(); + + // Trigger Ctrl+C + if (dataHandler) { + dataHandler(Buffer.from([0x03])); + } + + await expect(clientPromise).rejects.toThrow(FatalCancellationError); + expect(stdinRemoveListenerSpy).toHaveBeenCalledWith( + 'data', + expect.any(Function), + ); + + stdinOnSpy.mockRestore(); + stdinRemoveListenerSpy.mockRestore(); + }); + }); + describe('clearCachedCredentialFile', () => { it('should clear cached credentials and Google account', async () => { const cachedCreds = { refresh_token: 'test-token' }; diff --git a/packages/core/src/code_assist/oauth2.ts b/packages/core/src/code_assist/oauth2.ts index 8c3cd8828f..406e054f1e 100644 --- a/packages/core/src/code_assist/oauth2.ts +++ b/packages/core/src/code_assist/oauth2.ts @@ -325,7 +325,41 @@ async function initOauthClient( }, authTimeout); }); - await Promise.race([webLogin.loginCompletePromise, timeoutPromise]); + // Listen for SIGINT to stop waiting for auth so the terminal doesn't hang + // if the user chooses not to auth. + let sigIntHandler: (() => void) | undefined; + let stdinHandler: ((data: Buffer) => void) | undefined; + const cancellationPromise = new Promise((_, reject) => { + sigIntHandler = () => + reject(new FatalCancellationError('Authentication cancelled by user.')); + process.on('SIGINT', sigIntHandler); + + // Note that SIGINT might not get raised on Ctrl+C in raw mode + // so we also need to look for Ctrl+C directly in stdin. + stdinHandler = (data) => { + if (data.includes(0x03)) { + reject( + new FatalCancellationError('Authentication cancelled by user.'), + ); + } + }; + process.stdin.on('data', stdinHandler); + }); + + try { + await Promise.race([ + webLogin.loginCompletePromise, + timeoutPromise, + cancellationPromise, + ]); + } finally { + if (sigIntHandler) { + process.removeListener('SIGINT', sigIntHandler); + } + if (stdinHandler) { + process.stdin.removeListener('data', stdinHandler); + } + } coreEvents.emit(CoreEvent.UserFeedback, { severity: 'info', From dc6dda5c3796ae6e72cdb2def3159c3512661c38 Mon Sep 17 00:00:00 2001 From: Vedant Mahajan Date: Tue, 6 Jan 2026 00:33:00 +0530 Subject: [PATCH 007/591] fix: avoid SDK warning by not accessing .text getter in logging (#15706) Co-authored-by: Ishaan Gupta --- .../core/src/core/loggingContentGenerator.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/core/src/core/loggingContentGenerator.ts b/packages/core/src/core/loggingContentGenerator.ts index de16b8de69..b8cf49a091 100644 --- a/packages/core/src/core/loggingContentGenerator.ts +++ b/packages/core/src/core/loggingContentGenerator.ts @@ -213,7 +213,13 @@ export class LoggingContentGenerator implements ContentGenerator { response.responseId, response.candidates, response.usageMetadata, - JSON.stringify(response), + JSON.stringify({ + candidates: response.candidates, + usageMetadata: response.usageMetadata, + responseId: response.responseId, + modelVersion: response.modelVersion, + promptFeedback: response.promptFeedback, + }), req.config, serverDetails, ); @@ -319,7 +325,15 @@ export class LoggingContentGenerator implements ContentGenerator { responses[0]?.responseId, responses.flatMap((response) => response.candidates || []), lastUsageMetadata, - JSON.stringify(responses), + JSON.stringify( + responses.map((r) => ({ + candidates: r.candidates, + usageMetadata: r.usageMetadata, + responseId: r.responseId, + modelVersion: r.modelVersion, + promptFeedback: r.promptFeedback, + })), + ), req.config, serverDetails, ); From 3c92666ec298b6ddc47268cb9dce5cc12d598dfa Mon Sep 17 00:00:00 2001 From: Dev Randalpura Date: Mon, 5 Jan 2026 15:00:20 -0500 Subject: [PATCH 008/591] Make default settings apply (#15354) --- packages/cli/src/config/config.ts | 13 +- packages/cli/src/config/settings.test.ts | 124 +++++++++++++++----- packages/cli/src/config/settings.ts | 30 ++++- packages/cli/src/ui/components/Footer.tsx | 9 +- packages/cli/src/ui/utils/ui-sizing.test.ts | 4 +- packages/cli/src/ui/utils/ui-sizing.ts | 2 +- packages/cli/src/utils/settingsUtils.ts | 6 +- 7 files changed, 135 insertions(+), 53 deletions(-) diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 858a929aff..e08a50893f 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -666,7 +666,7 @@ export async function loadCliConfig( screenReader, }, telemetry: telemetrySettings, - usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled ?? true, + usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled, fileFiltering, checkpointing: settings.general?.checkpointing?.enabled, proxy: @@ -678,7 +678,7 @@ export async function loadCliConfig( fileDiscoveryService: fileService, bugCommand: settings.advanced?.bugCommand, model: resolvedModel, - maxSessionTurns: settings.model?.maxSessionTurns ?? -1, + maxSessionTurns: settings.model?.maxSessionTurns, experimentalZedIntegration: argv.experimentalAcp || false, listExtensions: argv.listExtensions || false, listSessions: argv.listSessions || false, @@ -698,13 +698,12 @@ export async function loadCliConfig( interactive, trustedFolder, useRipgrep: settings.tools?.useRipgrep, - enableInteractiveShell: - settings.tools?.shell?.enableInteractiveShell ?? true, + enableInteractiveShell: settings.tools?.shell?.enableInteractiveShell, shellToolInactivityTimeout: settings.tools?.shell?.inactivityTimeout, enableShellOutputEfficiency: settings.tools?.shell?.enableShellOutputEfficiency ?? true, skipNextSpeakerCheck: settings.model?.skipNextSpeakerCheck, - enablePromptCompletion: settings.general?.enablePromptCompletion ?? false, + enablePromptCompletion: settings.general?.enablePromptCompletion, truncateToolOutputThreshold: settings.tools?.truncateToolOutputThreshold, truncateToolOutputLines: settings.tools?.truncateToolOutputLines, enableToolOutputTruncation: settings.tools?.enableToolOutputTruncation, @@ -719,11 +718,11 @@ export async function loadCliConfig( settings.experimental?.introspectionAgentSettings, fakeResponses: argv.fakeResponses, recordResponses: argv.recordResponses, - retryFetchErrors: settings.general?.retryFetchErrors ?? false, + retryFetchErrors: settings.general?.retryFetchErrors, ptyInfo: ptyInfo?.name, modelConfigServiceConfig: settings.modelConfigs, // TODO: loading of hooks based on workspace trust - enableHooks: settings.tools?.enableHooks ?? false, + enableHooks: settings.tools?.enableHooks, hooks: settings.hooks || {}, projectHooks: projectHooks || {}, onModelChange: (model: string) => saveModelChange(loadedSettings, model), diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 1559cbe78c..133b50daae 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -35,6 +35,14 @@ vi.mock('./trustedFolders.js', () => ({ .mockReturnValue({ isTrusted: true, source: 'file' }), })); +vi.mock('./settingsSchema.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getSettingsSchema: vi.fn(actual.getSettingsSchema), + }; +}); + // NOW import everything else, including the (now effectively re-exported) settings.js import path, * as pathActual from 'node:path'; // Restored for MOCK_WORKSPACE_SETTINGS_PATH import { @@ -65,10 +73,16 @@ import { SettingScope, saveSettings, type SettingsFile, + getDefaultsFromSchema, } from './settings.js'; import { FatalConfigError, GEMINI_DIR } from '@google/gemini-cli-core'; import { ExtensionManager } from './extension-manager.js'; import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js'; +import { + getSettingsSchema, + MergeStrategy, + type SettingsSchema, +} from './settingsSchema.js'; const MOCK_WORKSPACE_DIR = '/mock/workspace'; // Use the (mocked) GEMINI_DIR for consistency @@ -149,14 +163,6 @@ describe('Settings Loading and Merging', () => { }); describe('loadSettings', () => { - it('should load empty settings if no files exist', () => { - const settings = loadSettings(MOCK_WORKSPACE_DIR); - expect(settings.system.settings).toEqual({}); - expect(settings.user.settings).toEqual({}); - expect(settings.workspace.settings).toEqual({}); - expect(settings.merged).toEqual({}); - }); - it.each([ { scope: 'system', @@ -201,7 +207,7 @@ describe('Settings Loading and Merging', () => { expect( settings[scope as 'system' | 'user' | 'workspace'].settings, ).toEqual(content); - expect(settings.merged).toEqual(content); + expect(settings.merged).toMatchObject(content); }, ); @@ -265,7 +271,7 @@ describe('Settings Loading and Merging', () => { expect(settings.system.settings).toEqual(systemSettingsContent); expect(settings.user.settings).toEqual(userSettingsContent); expect(settings.workspace.settings).toEqual(workspaceSettingsContent); - expect(settings.merged).toEqual({ + expect(settings.merged).toMatchObject({ ui: { theme: 'system-theme', }, @@ -318,7 +324,7 @@ describe('Settings Loading and Merging', () => { const settings = loadSettings(MOCK_WORKSPACE_DIR); - expect(settings.merged).toEqual({ + expect(settings.merged).toMatchObject({ ui: { theme: 'legacy-dark', }, @@ -443,10 +449,33 @@ describe('Settings Loading and Merging', () => { expect(settings.merged.advanced?.excludedEnvVars).toEqual( expect.arrayContaining(['USER_VAR', 'WORKSPACE_VAR']), ); - expect(settings.merged.advanced?.excludedEnvVars).toHaveLength(2); + expect(settings.merged.advanced?.excludedEnvVars).toHaveLength(4); }); it('should merge all settings files with the correct precedence', () => { + // Mock schema to test defaults application + const mockSchema = { + ui: { type: 'object', default: {}, properties: {} }, + tools: { type: 'object', default: {}, properties: {} }, + context: { + type: 'object', + default: {}, + properties: { + discoveryMaxDirs: { type: 'number', default: 200 }, + includeDirectories: { + type: 'array', + default: [], + mergeStrategy: MergeStrategy.CONCAT, + }, + }, + }, + mcpServers: { type: 'object', default: {} }, + }; + + (getSettingsSchema as Mock).mockReturnValue( + mockSchema as unknown as SettingsSchema, + ); + (mockFsExistsSync as Mock).mockReturnValue(true); const systemDefaultsContent = { ui: { @@ -510,7 +539,7 @@ describe('Settings Loading and Merging', () => { expect(settings.workspace.settings).toEqual(workspaceSettingsContent); expect(settings.merged).toEqual({ context: { - fileName: 'WORKSPACE_CONTEXT.md', + discoveryMaxDirs: 200, includeDirectories: [ '/system/defaults/dir', '/user/dir1', @@ -518,14 +547,12 @@ describe('Settings Loading and Merging', () => { '/workspace/dir', '/system/dir', ], + fileName: 'WORKSPACE_CONTEXT.md', }, + mcpServers: {}, + ui: { theme: 'system-theme' }, + tools: { sandbox: false }, telemetry: false, - tools: { - sandbox: false, - }, - ui: { - theme: 'system-theme', - }, }); }); @@ -660,7 +687,7 @@ describe('Settings Loading and Merging', () => { }, expected: { key: 'advanced.excludedEnvVars', - value: ['DEBUG', 'NODE_ENV', 'CUSTOM_VAR'], + value: ['DEBUG', 'DEBUG_MODE', 'NODE_ENV', 'CUSTOM_VAR'], }, }, { @@ -671,7 +698,7 @@ describe('Settings Loading and Merging', () => { }, expected: { key: 'advanced.excludedEnvVars', - value: ['WORKSPACE_DEBUG', 'WORKSPACE_VAR'], + value: ['DEBUG', 'DEBUG_MODE', 'WORKSPACE_DEBUG', 'WORKSPACE_VAR'], }, }, ])( @@ -734,6 +761,7 @@ describe('Settings Loading and Merging', () => { ]); expect(settings.merged.advanced?.excludedEnvVars).toEqual([ 'DEBUG', + 'DEBUG_MODE', 'NODE_ENV', 'USER_VAR', 'WORKSPACE_DEBUG', @@ -814,8 +842,8 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockReturnValue('{}'); const settings = loadSettings(MOCK_WORKSPACE_DIR); expect(settings.merged.telemetry).toBeUndefined(); - expect(settings.merged.ui).toBeUndefined(); - expect(settings.merged.mcpServers).toBeUndefined(); + expect(settings.merged.ui).toBeDefined(); + expect(settings.merged.mcpServers).toEqual({}); }); it('should merge MCP servers correctly, with workspace taking precedence', () => { @@ -941,7 +969,7 @@ describe('Settings Loading and Merging', () => { (mockFsExistsSync as Mock).mockReturnValue(false); // No settings files exist (fs.readFileSync as Mock).mockReturnValue('{}'); const settings = loadSettings(MOCK_WORKSPACE_DIR); - expect(settings.merged.mcpServers).toBeUndefined(); + expect(settings.merged.mcpServers).toEqual({}); }); it('should merge MCP servers from system, user, and workspace with system taking precedence', () => { @@ -1075,10 +1103,10 @@ describe('Settings Loading and Merging', () => { expected: 0.8, }, { - description: 'should be undefined if not in any settings file', + description: 'should be default if not in any settings file', userContent: {}, workspaceContent: {}, - expected: undefined, + expected: 0.5, }, ])('$description', ({ userContent, workspaceContent, expected }) => { (mockFsExistsSync as Mock).mockReturnValue(true); @@ -1590,7 +1618,7 @@ describe('Settings Loading and Merging', () => { ); expect(settings.system.path).toBe(MOCK_ENV_SYSTEM_SETTINGS_PATH); expect(settings.system.settings).toEqual(systemSettingsContent); - expect(settings.merged).toEqual({ + expect(settings.merged).toMatchObject({ ...systemSettingsContent, }); }); @@ -1692,8 +1720,9 @@ describe('Settings Loading and Merging', () => { 'DEBUG', ]); expect(settings.merged.advanced?.excludedEnvVars).toEqual([ - 'NODE_ENV', 'DEBUG', + 'DEBUG_MODE', + 'NODE_ENV', ]); }); @@ -1732,6 +1761,7 @@ describe('Settings Loading and Merging', () => { ]); expect(settings.merged.advanced?.excludedEnvVars).toEqual([ 'DEBUG', + 'DEBUG_MODE', 'NODE_ENV', 'USER_VAR', 'WORKSPACE_DEBUG', @@ -2444,4 +2474,42 @@ describe('Settings Loading and Merging', () => { ); }); }); + + describe('getDefaultsFromSchema', () => { + it('should extract defaults from a schema', () => { + const mockSchema = { + prop1: { + type: 'string', + default: 'default1', + label: 'Prop 1', + category: 'General', + requiresRestart: false, + }, + nested: { + type: 'object', + label: 'Nested', + category: 'General', + requiresRestart: false, + default: {}, + properties: { + prop2: { + type: 'number', + default: 42, + label: 'Prop 2', + category: 'General', + requiresRestart: false, + }, + }, + }, + }; + + const defaults = getDefaultsFromSchema(mockSchema as SettingsSchema); + expect(defaults).toEqual({ + prop1: 'default1', + nested: { + prop2: 42, + }, + }); + }); + }); }); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index e0a24cbebf..f347a04be2 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -424,6 +424,24 @@ export function migrateSettingsToV1( return v1Settings; } +export function getDefaultsFromSchema( + schema: SettingsSchema = getSettingsSchema(), +): Settings { + const defaults: Record = {}; + for (const key in schema) { + const definition = schema[key]; + if (definition.properties) { + const childDefaults = getDefaultsFromSchema(definition.properties); + if (Object.keys(childDefaults).length > 0) { + defaults[key] = childDefaults; + } + } else if (definition.default !== undefined) { + defaults[key] = definition.default; + } + } + return defaults as Settings; +} + function mergeSettings( system: Settings, systemDefaults: Settings, @@ -432,16 +450,18 @@ function mergeSettings( isTrusted: boolean, ): Settings { const safeWorkspace = isTrusted ? workspace : ({} as Settings); + const schemaDefaults = getDefaultsFromSchema(); // Settings are merged with the following precedence (last one wins for // single values): - // 1. System Defaults - // 2. User Settings - // 3. Workspace Settings - // 4. System Settings (as overrides) + // 1. Schema Defaults (Built-in) + // 2. System Defaults + // 3. User Settings + // 4. Workspace Settings + // 5. System Settings (as overrides) return customDeepMerge( getMergeStrategyForPath, - {}, // Start with an empty object + schemaDefaults, systemDefaults, user, safeWorkspace, diff --git a/packages/cli/src/ui/components/Footer.tsx b/packages/cli/src/ui/components/Footer.tsx index 358342faa1..7ab087fd99 100644 --- a/packages/cli/src/ui/components/Footer.tsx +++ b/packages/cli/src/ui/components/Footer.tsx @@ -60,12 +60,11 @@ export const Footer: React.FC = () => { const showMemoryUsage = config.getDebugMode() || settings.merged.ui?.showMemoryUsage || false; - const hideCWD = settings.merged.ui?.footer?.hideCWD || false; - const hideSandboxStatus = - settings.merged.ui?.footer?.hideSandboxStatus || false; - const hideModelInfo = settings.merged.ui?.footer?.hideModelInfo || false; + const hideCWD = settings.merged.ui?.footer?.hideCWD; + const hideSandboxStatus = settings.merged.ui?.footer?.hideSandboxStatus; + const hideModelInfo = settings.merged.ui?.footer?.hideModelInfo; const hideContextPercentage = - settings.merged.ui?.footer?.hideContextPercentage ?? true; + settings.merged.ui?.footer?.hideContextPercentage; const pathLength = Math.max(20, Math.floor(mainAreaWidth * 0.25)); const displayPath = shortenPath(tildeifyPath(targetDir), pathLength); diff --git a/packages/cli/src/ui/utils/ui-sizing.test.ts b/packages/cli/src/ui/utils/ui-sizing.test.ts index 331c416b7a..7611abbaa3 100644 --- a/packages/cli/src/ui/utils/ui-sizing.test.ts +++ b/packages/cli/src/ui/utils/ui-sizing.test.ts @@ -35,8 +35,8 @@ describe('ui-sizing', () => { [80, true, true, 79], // -1 for alternate buffer [100, true, true, 99], - // Default behavior (useFullWidth undefined or true) - [100, undefined, false, 100], + // Default behavior (useFullWidth true) + [100, true, false, 100], // useFullWidth: false (Smart sizing) [80, false, false, 78], // 98% of 80 diff --git a/packages/cli/src/ui/utils/ui-sizing.ts b/packages/cli/src/ui/utils/ui-sizing.ts index be85fe88b9..26d28fefa4 100644 --- a/packages/cli/src/ui/utils/ui-sizing.ts +++ b/packages/cli/src/ui/utils/ui-sizing.ts @@ -27,7 +27,7 @@ export const calculateMainAreaWidth = ( terminalWidth: number, settings: LoadedSettings, ): number => { - if (settings.merged.ui?.useFullWidth !== false) { + if (settings.merged.ui?.useFullWidth) { if (isAlternateBufferEnabled(settings)) { return terminalWidth - 1; } diff --git a/packages/cli/src/utils/settingsUtils.ts b/packages/cli/src/utils/settingsUtils.ts index 7ec5fd5885..69530e6dda 100644 --- a/packages/cli/src/utils/settingsUtils.ts +++ b/packages/cli/src/utils/settingsUtils.ts @@ -279,11 +279,7 @@ export function getSettingValue( if (typeof value === 'boolean') { return value; } - // Fall back to default value, ensuring it's a boolean - const defaultValue = definition.default; - if (typeof defaultValue === 'boolean') { - return defaultValue; - } + return false; // Final fallback } From b13c6b57ae99f882b82a7e6a5a899fe889fb3cfd Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Mon, 5 Jan 2026 15:25:54 -0500 Subject: [PATCH 009/591] chore: rename smart-edit to edit (#15923) --- docs/cli/telemetry.md | 6 ++-- packages/cli/src/ui/constants/tips.ts | 1 - packages/core/src/config/config.ts | 4 +-- packages/core/src/index.ts | 2 +- .../clearcut-logger/clearcut-logger.ts | 24 ++++++------- .../clearcut-logger/event-metadata-key.ts | 8 ++--- packages/core/src/telemetry/loggers.test.ts | 8 ++--- packages/core/src/telemetry/loggers.ts | 16 ++++----- packages/core/src/telemetry/types.ts | 28 ++++++++------- .../src/tools/confirmation-policy.test.ts | 11 +++--- .../{smart-edit.test.ts => edit.test.ts} | 20 +++++------ .../core/src/tools/{smart-edit.ts => edit.ts} | 34 +++++++++---------- packages/core/src/tools/write-file.test.ts | 2 +- packages/core/src/utils/editCorrector.ts | 2 +- 14 files changed, 79 insertions(+), 87 deletions(-) rename packages/core/src/tools/{smart-edit.test.ts => edit.test.ts} (98%) rename packages/core/src/tools/{smart-edit.ts => edit.ts} (97%) diff --git a/docs/cli/telemetry.md b/docs/cli/telemetry.md index 8fb2fd179e..4e0ceb0f94 100644 --- a/docs/cli/telemetry.md +++ b/docs/cli/telemetry.md @@ -297,7 +297,7 @@ Captures startup configuration and user prompt submissions. #### Tools -Captures tool executions, output truncation, and Smart Edit behavior. +Captures tool executions, output truncation, and Edit behavior. - `gemini_cli.tool_call`: Emitted for each tool (function) call. - **Attributes**: @@ -325,11 +325,11 @@ Captures tool executions, output truncation, and Smart Edit behavior. - `lines` (int) - `prompt_id` (string) -- `gemini_cli.smart_edit_strategy`: Smart Edit strategy chosen. +- `gemini_cli.edit_strategy`: Edit strategy chosen. - **Attributes**: - `strategy` (string) -- `gemini_cli.smart_edit_correction`: Smart Edit correction result. +- `gemini_cli.edit_correction`: Edit correction result. - **Attributes**: - `correction` ("success" | "failure") diff --git a/packages/cli/src/ui/constants/tips.ts b/packages/cli/src/ui/constants/tips.ts index 7200d3af4e..a18205ff36 100644 --- a/packages/cli/src/ui/constants/tips.ts +++ b/packages/cli/src/ui/constants/tips.ts @@ -75,7 +75,6 @@ export const INFORMATIVE_TIPS = [ 'Set the character threshold for truncating tool outputs (/settings)...', 'Set the number of lines to keep when truncating outputs (/settings)...', 'Enable policy-based tool confirmation via message bus (/settings)...', - 'Enable smart-edit tool for more precise editing (/settings)...', 'Enable write_todos_list tool to generate task lists (/settings)...', 'Enable model routing based on complexity (/settings)...', 'Enable experimental subagents for task delegation (/settings)...', diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index cceae41b14..f295cda720 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -25,7 +25,7 @@ import { GrepTool } from '../tools/grep.js'; import { canUseRipgrep, RipGrepTool } from '../tools/ripGrep.js'; import { GlobTool } from '../tools/glob.js'; import { ActivateSkillTool } from '../tools/activate-skill.js'; -import { SmartEditTool } from '../tools/smart-edit.js'; +import { EditTool } from '../tools/edit.js'; import { ShellTool } from '../tools/shell.js'; import { WriteFileTool } from '../tools/write-file.js'; import { WebFetchTool } from '../tools/web-fetch.js'; @@ -1690,7 +1690,7 @@ export class Config { registerCoreTool(GlobTool, this); registerCoreTool(ActivateSkillTool, this); - registerCoreTool(SmartEditTool, this); + registerCoreTool(EditTool, this); registerCoreTool(WriteFileTool, this); registerCoreTool(WebFetchTool, this); registerCoreTool(ShellTool, this); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c7166e2c5a..e20ed7f0a5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -124,7 +124,7 @@ export * from './tools/ls.js'; export * from './tools/grep.js'; export * from './tools/ripGrep.js'; export * from './tools/glob.js'; -export * from './tools/smart-edit.js'; +export * from './tools/edit.js'; export * from './tools/write-file.js'; export * from './tools/web-fetch.js'; export * from './tools/memoryTool.js'; diff --git a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts index 4ac02642a7..443f9365c9 100644 --- a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts +++ b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts @@ -31,8 +31,8 @@ import type { ExtensionEnableEvent, ModelSlashCommandEvent, ExtensionDisableEvent, - SmartEditStrategyEvent, - SmartEditCorrectionEvent, + EditStrategyEvent, + EditCorrectionEvent, AgentStartEvent, AgentFinishEvent, RecoveryAttemptEvent, @@ -89,8 +89,8 @@ export enum EventNames { TOOL_OUTPUT_TRUNCATED = 'tool_output_truncated', MODEL_ROUTING = 'model_routing', MODEL_SLASH_COMMAND = 'model_slash_command', - SMART_EDIT_STRATEGY = 'smart_edit_strategy', - SMART_EDIT_CORRECTION = 'smart_edit_correction', + EDIT_STRATEGY = 'edit_strategy', + EDIT_CORRECTION = 'edit_correction', AGENT_START = 'agent_start', AGENT_FINISH = 'agent_finish', RECOVERY_ATTEMPT = 'recovery_attempt', @@ -1235,31 +1235,27 @@ export class ClearcutLogger { }); } - logSmartEditStrategyEvent(event: SmartEditStrategyEvent): void { + logEditStrategyEvent(event: EditStrategyEvent): void { const data: EventValue[] = [ { - gemini_cli_key: EventMetadataKey.GEMINI_CLI_SMART_EDIT_STRATEGY, + gemini_cli_key: EventMetadataKey.GEMINI_CLI_EDIT_STRATEGY, value: event.strategy, }, ]; - this.enqueueLogEvent( - this.createLogEvent(EventNames.SMART_EDIT_STRATEGY, data), - ); + this.enqueueLogEvent(this.createLogEvent(EventNames.EDIT_STRATEGY, data)); this.flushIfNeeded(); } - logSmartEditCorrectionEvent(event: SmartEditCorrectionEvent): void { + logEditCorrectionEvent(event: EditCorrectionEvent): void { const data: EventValue[] = [ { - gemini_cli_key: EventMetadataKey.GEMINI_CLI_SMART_EDIT_CORRECTION, + gemini_cli_key: EventMetadataKey.GEMINI_CLI_EDIT_CORRECTION, value: event.correction, }, ]; - this.enqueueLogEvent( - this.createLogEvent(EventNames.SMART_EDIT_CORRECTION, data), - ); + this.enqueueLogEvent(this.createLogEvent(EventNames.EDIT_CORRECTION, data)); this.flushIfNeeded(); } diff --git a/packages/core/src/telemetry/clearcut-logger/event-metadata-key.ts b/packages/core/src/telemetry/clearcut-logger/event-metadata-key.ts index ce3e3ee10c..e53ae71ae9 100644 --- a/packages/core/src/telemetry/clearcut-logger/event-metadata-key.ts +++ b/packages/core/src/telemetry/clearcut-logger/event-metadata-key.ts @@ -93,11 +93,11 @@ export enum EventMetadataKey { // Replace Tool Call Event Keys // =========================================================================== - // Logs a smart edit tool strategy choice. - GEMINI_CLI_SMART_EDIT_STRATEGY = 109, + // Logs a edit tool strategy choice. + GEMINI_CLI_EDIT_STRATEGY = 109, - // Logs a smart edit correction event. - GEMINI_CLI_SMART_EDIT_CORRECTION = 110, + // Logs a edit correction event. + GEMINI_CLI_EDIT_CORRECTION = 110, // Logs the reason for web fetch fallback. GEMINI_CLI_WEB_FETCH_FALLBACK_REASON = 116, diff --git a/packages/core/src/telemetry/loggers.test.ts b/packages/core/src/telemetry/loggers.test.ts index 84f1ba06b6..3dabc4a89d 100644 --- a/packages/core/src/telemetry/loggers.test.ts +++ b/packages/core/src/telemetry/loggers.test.ts @@ -12,7 +12,7 @@ import type { } from '../index.js'; import { AuthType, - SmartEditTool, + EditTool, GeminiClient, ToolConfirmationOutcome, ToolErrorType, @@ -1034,7 +1034,7 @@ describe('loggers', () => { }); it('should log a tool call with all fields', () => { - const tool = new SmartEditTool(mockConfig, createMockMessageBus()); + const tool = new EditTool(mockConfig, createMockMessageBus()); const call: CompletedToolCall = { status: 'success', request: { @@ -1250,7 +1250,7 @@ describe('loggers', () => { contentLength: 13, }, outcome: ToolConfirmationOutcome.ModifyWithEditor, - tool: new SmartEditTool(mockConfig, createMockMessageBus()), + tool: new EditTool(mockConfig, createMockMessageBus()), invocation: {} as AnyToolInvocation, durationMs: 100, }; @@ -1329,7 +1329,7 @@ describe('loggers', () => { errorType: undefined, contentLength: 13, }, - tool: new SmartEditTool(mockConfig, createMockMessageBus()), + tool: new EditTool(mockConfig, createMockMessageBus()), invocation: {} as AnyToolInvocation, durationMs: 100, }; diff --git a/packages/core/src/telemetry/loggers.ts b/packages/core/src/telemetry/loggers.ts index 11858c9117..7ab974213f 100644 --- a/packages/core/src/telemetry/loggers.ts +++ b/packages/core/src/telemetry/loggers.ts @@ -41,8 +41,8 @@ import type { ExtensionUninstallEvent, ExtensionInstallEvent, ModelSlashCommandEvent, - SmartEditStrategyEvent, - SmartEditCorrectionEvent, + EditStrategyEvent, + EditCorrectionEvent, AgentStartEvent, AgentFinishEvent, RecoveryAttemptEvent, @@ -568,11 +568,11 @@ export async function logExtensionDisable( }); } -export function logSmartEditStrategy( +export function logEditStrategy( config: Config, - event: SmartEditStrategyEvent, + event: EditStrategyEvent, ): void { - ClearcutLogger.getInstance(config)?.logSmartEditStrategyEvent(event); + ClearcutLogger.getInstance(config)?.logEditStrategyEvent(event); bufferTelemetryEvent(() => { const logger = logs.getLogger(SERVICE_NAME); const logRecord: LogRecord = { @@ -583,11 +583,11 @@ export function logSmartEditStrategy( }); } -export function logSmartEditCorrectionEvent( +export function logEditCorrectionEvent( config: Config, - event: SmartEditCorrectionEvent, + event: EditCorrectionEvent, ): void { - ClearcutLogger.getInstance(config)?.logSmartEditCorrectionEvent(event); + ClearcutLogger.getInstance(config)?.logEditCorrectionEvent(event); bufferTelemetryEvent(() => { const logger = logs.getLogger(SERVICE_NAME); const logRecord: LogRecord = { diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index 95893f34d0..3ff143335f 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -1548,7 +1548,9 @@ export type TelemetryEvent = | RecoveryAttemptEvent | LlmLoopCheckEvent | StartupStatsEvent - | WebFetchFallbackAttemptEvent; + | WebFetchFallbackAttemptEvent + | EditStrategyEvent + | EditCorrectionEvent; export const EVENT_EXTENSION_DISABLE = 'gemini_cli.extension_disable'; export class ExtensionDisableEvent implements BaseTelemetryEvent { @@ -1588,14 +1590,14 @@ export class ExtensionDisableEvent implements BaseTelemetryEvent { } } -export const EVENT_SMART_EDIT_STRATEGY = 'gemini_cli.smart_edit_strategy'; -export class SmartEditStrategyEvent implements BaseTelemetryEvent { - 'event.name': 'smart_edit_strategy'; +export const EVENT_EDIT_STRATEGY = 'gemini_cli.edit_strategy'; +export class EditStrategyEvent implements BaseTelemetryEvent { + 'event.name': 'edit_strategy'; 'event.timestamp': string; strategy: string; constructor(strategy: string) { - this['event.name'] = 'smart_edit_strategy'; + this['event.name'] = 'edit_strategy'; this['event.timestamp'] = new Date().toISOString(); this.strategy = strategy; } @@ -1603,25 +1605,25 @@ export class SmartEditStrategyEvent implements BaseTelemetryEvent { toOpenTelemetryAttributes(config: Config): LogAttributes { return { ...getCommonAttributes(config), - 'event.name': EVENT_SMART_EDIT_STRATEGY, + 'event.name': EVENT_EDIT_STRATEGY, 'event.timestamp': this['event.timestamp'], strategy: this.strategy, }; } toLogBody(): string { - return `Smart Edit Tool Strategy: ${this.strategy}`; + return `Edit Tool Strategy: ${this.strategy}`; } } -export const EVENT_SMART_EDIT_CORRECTION = 'gemini_cli.smart_edit_correction'; -export class SmartEditCorrectionEvent implements BaseTelemetryEvent { - 'event.name': 'smart_edit_correction'; +export const EVENT_EDIT_CORRECTION = 'gemini_cli.edit_correction'; +export class EditCorrectionEvent implements BaseTelemetryEvent { + 'event.name': 'edit_correction'; 'event.timestamp': string; correction: 'success' | 'failure'; constructor(correction: 'success' | 'failure') { - this['event.name'] = 'smart_edit_correction'; + this['event.name'] = 'edit_correction'; this['event.timestamp'] = new Date().toISOString(); this.correction = correction; } @@ -1629,14 +1631,14 @@ export class SmartEditCorrectionEvent implements BaseTelemetryEvent { toOpenTelemetryAttributes(config: Config): LogAttributes { return { ...getCommonAttributes(config), - 'event.name': EVENT_SMART_EDIT_CORRECTION, + 'event.name': EVENT_EDIT_CORRECTION, 'event.timestamp': this['event.timestamp'], correction: this.correction, }; } toLogBody(): string { - return `Smart Edit Tool Correction: ${this.correction}`; + return `Edit Tool Correction: ${this.correction}`; } } diff --git a/packages/core/src/tools/confirmation-policy.test.ts b/packages/core/src/tools/confirmation-policy.test.ts index aede2f6e7c..df14bbfb63 100644 --- a/packages/core/src/tools/confirmation-policy.test.ts +++ b/packages/core/src/tools/confirmation-policy.test.ts @@ -7,7 +7,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { SmartEditTool } from './smart-edit.js'; +import { EditTool } from './edit.js'; import { WriteFileTool } from './write-file.js'; import { WebFetchTool } from './web-fetch.js'; import { ToolConfirmationOutcome } from './tools.js'; @@ -21,8 +21,8 @@ import os from 'node:os'; // Mock telemetry loggers to avoid failures vi.mock('../telemetry/loggers.js', () => ({ - logSmartEditStrategy: vi.fn(), - logSmartEditCorrectionEvent: vi.fn(), + logEditStrategy: vi.fn(), + logEditCorrectionEvent: vi.fn(), logFileOperation: vi.fn(), })); @@ -81,9 +81,8 @@ describe('Tool Confirmation Policy Updates', () => { const tools = [ { - name: 'SmartEditTool', - create: (config: Config, bus: MessageBus) => - new SmartEditTool(config, bus), + name: 'EditTool', + create: (config: Config, bus: MessageBus) => new EditTool(config, bus), params: { file_path: 'test.txt', instruction: 'change content', diff --git a/packages/core/src/tools/smart-edit.test.ts b/packages/core/src/tools/edit.test.ts similarity index 98% rename from packages/core/src/tools/smart-edit.test.ts rename to packages/core/src/tools/edit.test.ts index 41bd3d0379..14d520456b 100644 --- a/packages/core/src/tools/smart-edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -43,11 +43,11 @@ import { type Mock, } from 'vitest'; import { - SmartEditTool, + EditTool, type EditToolParams, applyReplacement, calculateReplacement, -} from './smart-edit.js'; +} from './edit.js'; import { type FileDiff, ToolConfirmationOutcome } from './tools.js'; import { ToolErrorType } from './tool-error.js'; import { @@ -64,8 +64,8 @@ import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.j import { StandardFileSystemService } from '../services/fileSystemService.js'; import type { BaseLlmClient } from '../core/baseLlmClient.js'; -describe('SmartEditTool', () => { - let tool: SmartEditTool; +describe('EditTool', () => { + let tool: EditTool; let tempDir: string; let rootDir: string; let mockConfig: Config; @@ -75,7 +75,7 @@ describe('SmartEditTool', () => { beforeEach(() => { vi.restoreAllMocks(); - tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'smart-edit-tool-test-')); + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'edit-tool-test-')); rootDir = path.join(tempDir, 'root'); fs.mkdirSync(rootDir); @@ -172,7 +172,7 @@ describe('SmartEditTool', () => { const bus = createMockMessageBus(); getMockMessageBusInstance(bus).defaultToolDecision = 'ask_user'; - tool = new SmartEditTool(mockConfig, bus); + tool = new EditTool(mockConfig, bus); }); afterEach(() => { @@ -393,9 +393,7 @@ describe('SmartEditTool', () => { const invocation = tool.build(params); const abortController = new AbortController(); - const abortError = new Error( - 'Abort requested during smart edit execution', - ); + const abortError = new Error('Abort requested during edit execution'); const calculateSpy = vi .spyOn(invocation as any, 'calculateEdit') @@ -755,9 +753,7 @@ describe('SmartEditTool', () => { const invocation = tool.build(params); const abortController = new AbortController(); - const abortError = new Error( - 'Abort requested during smart edit confirmation', - ); + const abortError = new Error('Abort requested during edit confirmation'); const calculateSpy = vi .spyOn(invocation as any, 'calculateEdit') diff --git a/packages/core/src/tools/smart-edit.ts b/packages/core/src/tools/edit.ts similarity index 97% rename from packages/core/src/tools/smart-edit.ts rename to packages/core/src/tools/edit.ts index dbff323381..3f71bdaad0 100644 --- a/packages/core/src/tools/smart-edit.ts +++ b/packages/core/src/tools/edit.ts @@ -35,10 +35,10 @@ import { import { IdeClient } from '../ide/ide-client.js'; import { FixLLMEditWithInstruction } from '../utils/llm-edit-fixer.js'; import { safeLiteralReplace } from '../utils/textUtils.js'; -import { SmartEditStrategyEvent } from '../telemetry/types.js'; -import { logSmartEditStrategy } from '../telemetry/loggers.js'; -import { SmartEditCorrectionEvent } from '../telemetry/types.js'; -import { logSmartEditCorrectionEvent } from '../telemetry/loggers.js'; +import { EditStrategyEvent } from '../telemetry/types.js'; +import { logEditStrategy } from '../telemetry/loggers.js'; +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'; @@ -289,22 +289,22 @@ export async function calculateReplacement( const exactResult = await calculateExactReplacement(context); if (exactResult) { - const event = new SmartEditStrategyEvent('exact'); - logSmartEditStrategy(config, event); + const event = new EditStrategyEvent('exact'); + logEditStrategy(config, event); return exactResult; } const flexibleResult = await calculateFlexibleReplacement(context); if (flexibleResult) { - const event = new SmartEditStrategyEvent('flexible'); - logSmartEditStrategy(config, event); + const event = new EditStrategyEvent('flexible'); + logEditStrategy(config, event); return flexibleResult; } const regexResult = await calculateRegexReplacement(context); if (regexResult) { - const event = new SmartEditStrategyEvent('regex'); - logSmartEditStrategy(config, event); + const event = new EditStrategyEvent('regex'); + logEditStrategy(config, event); return regexResult; } @@ -500,8 +500,8 @@ class EditToolInvocation if (secondError) { // The fix failed, log failure and return the original error - const event = new SmartEditCorrectionEvent('failure'); - logSmartEditCorrectionEvent(this.config, event); + const event = new EditCorrectionEvent('failure'); + logEditCorrectionEvent(this.config, event); return { currentContent: contentForLlmEditFixer, @@ -513,8 +513,8 @@ class EditToolInvocation }; } - const event = new SmartEditCorrectionEvent('success'); - logSmartEditCorrectionEvent(this.config, event); + const event = new EditCorrectionEvent('success'); + logEditCorrectionEvent(this.config, event); return { currentContent: contentForLlmEditFixer, @@ -703,7 +703,7 @@ class EditToolInvocation onConfirm: async (outcome: ToolConfirmationOutcome) => { if (outcome === ToolConfirmationOutcome.ProceedAlways) { // No need to publish a policy update as the default policy for - // AUTO_EDIT already reflects always approving smart-edit. + // AUTO_EDIT already reflects always approving edit. this.config.setApprovalMode(ApprovalMode.AUTO_EDIT); } else { await this.publishPolicyUpdate(outcome); @@ -866,7 +866,7 @@ class EditToolInvocation /** * Implementation of the Edit tool logic */ -export class SmartEditTool +export class EditTool extends BaseDeclarativeTool implements ModifiableDeclarativeTool { @@ -877,7 +877,7 @@ export class SmartEditTool messageBus: MessageBus, ) { super( - SmartEditTool.Name, + EditTool.Name, 'Edit', `Replaces text within a file. By default, replaces a single occurrence, but can replace multiple occurrences when \`expected_replacements\` is specified. This tool requires providing significant context around the change to ensure precise targeting. Always use the ${READ_FILE_TOOL_NAME} tool to examine the file's current content before attempting a text replacement. diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 85f0b6837a..cd5436e7be 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -23,7 +23,7 @@ import type { ToolResult, } from './tools.js'; import { ToolConfirmationOutcome } from './tools.js'; -import { type EditToolParams } from './smart-edit.js'; +import { type EditToolParams } from './edit.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../policy/types.js'; import type { ToolRegistry } from './tool-registry.js'; diff --git a/packages/core/src/utils/editCorrector.ts b/packages/core/src/utils/editCorrector.ts index 1851a8df87..99019e2b60 100644 --- a/packages/core/src/utils/editCorrector.ts +++ b/packages/core/src/utils/editCorrector.ts @@ -7,7 +7,7 @@ import type { Content } from '@google/genai'; import type { GeminiClient } from '../core/client.js'; import type { BaseLlmClient } from '../core/baseLlmClient.js'; -import type { EditToolParams } from '../tools/smart-edit.js'; +import type { EditToolParams } from '../tools/edit.js'; import { EDIT_TOOL_NAME, GREP_TOOL_NAME, From e5d183031acf0ea3a8f7e2edd604c9352577c6b1 Mon Sep 17 00:00:00 2001 From: Sehoon Shon Date: Mon, 5 Jan 2026 15:31:13 -0500 Subject: [PATCH 010/591] Opt-in to persist model from /model (#15820) --- .../src/ui/components/ModelDialog.test.tsx | 35 +++++++++++++++---- .../cli/src/ui/components/ModelDialog.tsx | 20 ++++++++--- packages/core/src/config/config.test.ts | 6 ++-- packages/core/src/config/config.ts | 4 +-- 4 files changed, 49 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/ui/components/ModelDialog.test.tsx b/packages/cli/src/ui/components/ModelDialog.test.tsx index f73f1cb012..fbfddbfad1 100644 --- a/packages/cli/src/ui/components/ModelDialog.test.tsx +++ b/packages/cli/src/ui/components/ModelDialog.test.tsx @@ -47,7 +47,7 @@ describe('', () => { const mockGetHasAccessToPreviewModel = vi.fn(); interface MockConfig extends Partial { - setModel: (model: string) => void; + setModel: (model: string, isTemporary?: boolean) => void; getModel: () => string; getPreviewFeatures: () => boolean; getHasAccessToPreviewModel: () => boolean; @@ -89,9 +89,7 @@ describe('', () => { it('renders the initial "main" view correctly', () => { const { lastFrame } = renderComponent(); expect(lastFrame()).toContain('Select Model'); - expect(lastFrame()).toContain( - 'Applies to this session and future Gemini CLI sessions.', - ); + expect(lastFrame()).toContain('Remember model for future sessions: false'); expect(lastFrame()).toContain('Auto'); expect(lastFrame()).toContain('Manual'); }); @@ -148,7 +146,10 @@ describe('', () => { stdin.write('\r'); await waitForUpdate(); - expect(mockSetModel).toHaveBeenCalledWith(DEFAULT_GEMINI_MODEL_AUTO); + expect(mockSetModel).toHaveBeenCalledWith( + DEFAULT_GEMINI_MODEL_AUTO, + true, // Session only by default + ); expect(mockOnClose).toHaveBeenCalled(); }); @@ -165,7 +166,29 @@ describe('', () => { stdin.write('\r'); await waitForUpdate(); - expect(mockSetModel).toHaveBeenCalledWith(DEFAULT_GEMINI_MODEL); + expect(mockSetModel).toHaveBeenCalledWith(DEFAULT_GEMINI_MODEL, true); + expect(mockOnClose).toHaveBeenCalled(); + }); + + it('toggles persist mode with Tab key', async () => { + const { lastFrame, stdin } = renderComponent(); + + expect(lastFrame()).toContain('Remember model for future sessions: false'); + + // Press Tab to toggle persist mode + stdin.write('\t'); + await waitForUpdate(); + + expect(lastFrame()).toContain('Remember model for future sessions: true'); + + // Select "Auto" (index 0) + stdin.write('\r'); + await waitForUpdate(); + + expect(mockSetModel).toHaveBeenCalledWith( + DEFAULT_GEMINI_MODEL_AUTO, + false, // Persist enabled + ); expect(mockOnClose).toHaveBeenCalled(); }); diff --git a/packages/cli/src/ui/components/ModelDialog.tsx b/packages/cli/src/ui/components/ModelDialog.tsx index fa5af46390..d1c12af8ce 100644 --- a/packages/cli/src/ui/components/ModelDialog.tsx +++ b/packages/cli/src/ui/components/ModelDialog.tsx @@ -32,6 +32,7 @@ interface ModelDialogProps { export function ModelDialog({ onClose }: ModelDialogProps): React.JSX.Element { const config = useContext(ConfigContext); const [view, setView] = useState<'main' | 'manual'>('main'); + const [persistMode, setPersistMode] = useState(false); // Determine the Preferred Model (read once when the dialog opens). const preferredModel = config?.getModel() || DEFAULT_GEMINI_MODEL_AUTO; @@ -62,6 +63,9 @@ export function ModelDialog({ onClose }: ModelDialogProps): React.JSX.Element { onClose(); } } + if (key.name === 'tab') { + setPersistMode((prev) => !prev); + } }, { isActive: true }, ); @@ -157,13 +161,13 @@ export function ModelDialog({ onClose }: ModelDialogProps): React.JSX.Element { } if (config) { - config.setModel(model); + config.setModel(model, persistMode ? false : true); const event = new ModelSlashCommandEvent(model); logModelSlashCommand(config, event); } onClose(); }, - [config, onClose], + [config, onClose, persistMode], ); let header; @@ -213,9 +217,15 @@ export function ModelDialog({ onClose }: ModelDialogProps): React.JSX.Element { /> - - Applies to this session and future Gemini CLI sessions. - + + + Remember model for future sessions:{' '} + + + {persistMode ? 'true' : 'false'} + + + (Press Tab to toggle) diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 1f389cfaa0..ea357d690a 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -1630,19 +1630,19 @@ describe('Config getHooks', () => { expect(config.getActiveModel()).toBe(originalModel); }); - it('should call onModelChange when a new model is set', () => { + it('should call onModelChange when a new model is set and should persist', () => { const onModelChange = vi.fn(); const config = new Config({ ...baseParams, onModelChange, }); - config.setModel(DEFAULT_GEMINI_MODEL); + config.setModel(DEFAULT_GEMINI_MODEL, false); expect(onModelChange).toHaveBeenCalledWith(DEFAULT_GEMINI_MODEL); }); - it('should NOT call onModelChange when a new model is set as a fallback', () => { + it('should NOT call onModelChange when a new model is temporary', () => { const onModelChange = vi.fn(); const config = new Config({ ...baseParams, diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index f295cda720..3d9aba2bb8 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -909,13 +909,13 @@ export class Config { return this.model; } - setModel(newModel: string, isFallbackModel: boolean = false): void { + setModel(newModel: string, isTemporary: boolean = true): void { if (this.model !== newModel || this._activeModel !== newModel) { this.model = newModel; // When the user explicitly sets a model, that becomes the active model. this._activeModel = newModel; coreEvents.emitModelChanged(newModel); - if (this.onModelChange && !isFallbackModel) { + if (this.onModelChange && !isTemporary) { this.onModelChange(newModel); } } From 2da911e4a02e1da58f5df969ba86a4d2ebf38f46 Mon Sep 17 00:00:00 2001 From: Manoj Naik <68473696+ManojINaik@users.noreply.github.com> Date: Tue, 6 Jan 2026 02:03:03 +0530 Subject: [PATCH 011/591] fix: prevent /copy crash on Windows by skipping /dev/tty (#15657) Co-authored-by: Jack Wotherspoon --- .../cli/src/ui/utils/commandUtils.test.ts | 32 +++++++++++++++++++ packages/cli/src/ui/utils/commandUtils.ts | 18 +++++++---- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/ui/utils/commandUtils.test.ts b/packages/cli/src/ui/utils/commandUtils.test.ts index 0694a7715d..7a2e62a947 100644 --- a/packages/cli/src/ui/utils/commandUtils.test.ts +++ b/packages/cli/src/ui/utils/commandUtils.test.ts @@ -98,6 +98,9 @@ describe('commandUtils', () => { beforeEach(async () => { vi.clearAllMocks(); + // Reset platform to default for test isolation + mockProcess.platform = 'darwin'; + // Dynamically import and set up spawn mock const { spawn } = await import('node:child_process'); mockSpawn = spawn as Mock; @@ -354,6 +357,35 @@ describe('commandUtils', () => { expect(tty.write).not.toHaveBeenCalled(); expect(tty.end).not.toHaveBeenCalled(); }); + + it('skips /dev/tty on Windows and uses stderr fallback for OSC-52', async () => { + mockProcess.platform = 'win32'; + const stderrStream = makeWritable({ isTTY: true }); + Object.defineProperty(process, 'stderr', { + value: stderrStream, + configurable: true, + }); + + // Set SSH environment to trigger OSC-52 path + process.env['SSH_CONNECTION'] = '1'; + + await copyToClipboard('windows-ssh-test'); + + expect(mockFs.createWriteStream).not.toHaveBeenCalled(); + expect(stderrStream.write).toHaveBeenCalled(); + expect(mockClipboardyWrite).not.toHaveBeenCalled(); + }); + + it('uses clipboardy on native Windows without SSH/WSL', async () => { + mockProcess.platform = 'win32'; + mockClipboardyWrite.mockResolvedValue(undefined); + + await copyToClipboard('windows-native-test'); + + // Fallback to clipboardy and not /dev/tty + expect(mockClipboardyWrite).toHaveBeenCalledWith('windows-native-test'); + expect(mockFs.createWriteStream).not.toHaveBeenCalled(); + }); }); describe('getUrlOpenCommand', () => { diff --git a/packages/cli/src/ui/utils/commandUtils.ts b/packages/cli/src/ui/utils/commandUtils.ts index d16e108423..c1bd755221 100644 --- a/packages/cli/src/ui/utils/commandUtils.ts +++ b/packages/cli/src/ui/utils/commandUtils.ts @@ -66,13 +66,19 @@ const SCREEN_DCS_CHUNK_SIZE = 240; type TtyTarget = { stream: Writable; closeAfter: boolean } | null; const pickTty = (): TtyTarget => { - // Prefer the controlling TTY to avoid interleaving escape sequences with piped stdout. - try { - const devTty = fs.createWriteStream('/dev/tty'); - return { stream: devTty, closeAfter: true }; - } catch { - // fall through + // /dev/tty is only available on Unix-like systems (Linux, macOS, BSD, etc.) + if (process.platform !== 'win32') { + // Prefer the controlling TTY to avoid interleaving escape sequences with piped stdout. + try { + const devTty = fs.createWriteStream('/dev/tty'); + // Prevent unhandled 'error' events from crashing the process. + devTty.on('error', () => {}); + return { stream: devTty, closeAfter: true }; + } catch { + // fall through - /dev/tty not accessible + } } + if (process.stderr?.isTTY) return { stream: process.stderr, closeAfter: false }; if (process.stdout?.isTTY) From 6d1e27633a32ce778127f1cf8fd88845284ae507 Mon Sep 17 00:00:00 2001 From: Christian Gunderman Date: Mon, 5 Jan 2026 13:27:53 -0800 Subject: [PATCH 012/591] Support context injection via SessionStart hook. (#15746) --- integration-tests/hooks-system.test.ts | 164 ++++++++++++++++++ packages/cli/src/gemini.tsx | 36 +++- packages/cli/src/ui/AppContainer.tsx | 32 +++- packages/cli/src/ui/commands/clearCommand.ts | 15 +- .../core/src/core/contentGenerator.test.ts | 4 +- packages/core/src/core/contentGenerator.ts | 5 +- packages/core/src/core/sessionHookTriggers.ts | 24 ++- 7 files changed, 261 insertions(+), 19 deletions(-) diff --git a/integration-tests/hooks-system.test.ts b/integration-tests/hooks-system.test.ts index bb7d2cd565..c5ab2b024d 100644 --- a/integration-tests/hooks-system.test.ts +++ b/integration-tests/hooks-system.test.ts @@ -950,6 +950,170 @@ try { } }); + it('should fire SessionStart hook and inject context', async () => { + // Create hook script that outputs JSON with additionalContext + const hookScript = `const fs = require('fs'); +console.log(JSON.stringify({ + decision: 'allow', + systemMessage: 'Context injected via SessionStart hook', + hookSpecificOutput: { + hookEventName: 'SessionStart', + additionalContext: 'If asked about your identity, you must respond with the following: I am a protocol droid that speaks the bacchi.' + } +}));`; + + await rig.setup('should fire SessionStart hook and inject context', { + fakeResponsesPath: join( + import.meta.dirname, + 'hooks-system.session-startup.responses', + ), + }); + + const scriptPath = join(rig.testDir!, 'session_start_context_hook.cjs'); + writeFileSync(scriptPath, hookScript); + + await rig.setup('should fire SessionStart hook and inject context', { + settings: { + tools: { + enableHooks: true, + }, + hooks: { + SessionStart: [ + { + matcher: 'startup', + hooks: [ + { + type: 'command', + command: `node "${scriptPath}"`, + timeout: 5000, + }, + ], + }, + ], + }, + }, + }); + + // Run a query - the SessionStart hook will fire during app initialization + const result = await rig.run({ args: 'Who are you?' }); + + // Check if systemMessage was displayed (in stderr, which rig.run captures) + expect(result).toContain('Context injected via SessionStart hook'); + + // Check if additionalContext influenced the model response + // Note: We use fake responses, but the rig records interactions. + // If we are using fake responses, the model won't actually respond unless we provide a fake response for the injected context. + // But the test rig setup uses 'hooks-system.session-startup.responses'. + // If I'm adding a new test, I might need to generate new fake responses or expect the context to be sent to the model (verify API logs). + + // Verify hook executed + const hookLogs = rig.readHookLogs(); + const sessionStartLog = hookLogs.find( + (log) => log.hookCall.hook_event_name === 'SessionStart', + ); + + expect(sessionStartLog).toBeDefined(); + + // Verify the API request contained the injected context + // rig.readAllApiRequest() gives us telemetry on API requests. + const apiRequests = rig.readAllApiRequest(); + // We expect at least one API request + expect(apiRequests.length).toBeGreaterThan(0); + + // The injected context should be in the request text + // For non-interactive mode, I prepended it to input: "context\n\ninput" + // The telemetry `request_text` should contain it. + const requestText = apiRequests[0].attributes?.request_text || ''; + expect(requestText).toContain('protocol droid'); + }); + + it('should fire SessionStart hook and display systemMessage in interactive mode', async () => { + // Create hook script that outputs JSON with systemMessage and additionalContext + const hookScript = `const fs = require('fs'); +console.log(JSON.stringify({ + decision: 'allow', + systemMessage: 'Interactive Session Start Message', + hookSpecificOutput: { + hookEventName: 'SessionStart', + additionalContext: 'The user is a Jedi Master.' + } +}));`; + + await rig.setup( + 'should fire SessionStart hook and display systemMessage in interactive mode', + { + fakeResponsesPath: join( + import.meta.dirname, + 'hooks-system.session-startup.responses', + ), + }, + ); + + const scriptPath = join( + rig.testDir!, + 'session_start_interactive_hook.cjs', + ); + writeFileSync(scriptPath, hookScript); + + await rig.setup( + 'should fire SessionStart hook and display systemMessage in interactive mode', + { + settings: { + tools: { + enableHooks: true, + }, + hooks: { + SessionStart: [ + { + matcher: 'startup', + hooks: [ + { + type: 'command', + command: `node "${scriptPath}"`, + timeout: 5000, + }, + ], + }, + ], + }, + }, + }, + ); + + const run = await rig.runInteractive(); + + // Verify systemMessage is displayed + await run.expectText('Interactive Session Start Message', 10000); + + // Send a prompt to establish a session and trigger an API call + await run.sendKeys('Hello'); + await run.sendKeys('\r'); + + // Wait for response to ensure API call happened + await run.expectText('Hello', 15000); + + // Wait for telemetry to be written to disk + await rig.waitForTelemetryReady(); + + // Verify the API request contained the injected context + // We may need to poll for API requests as they are written asynchronously + const pollResult = await poll( + () => { + const apiRequests = rig.readAllApiRequest(); + return apiRequests.length > 0; + }, + 15000, + 500, + ); + + expect(pollResult).toBe(true); + + const apiRequests = rig.readAllApiRequest(); + // The injected context should be in the request_text of the API request + const requestText = apiRequests[0].attributes?.request_text || ''; + expect(requestText).toContain('Jedi Master'); + }); + it('should fire SessionEnd and SessionStart hooks on /clear command', async () => { // Create inline hook commands for both SessionEnd and SessionStart const sessionEndCommand = diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index eacef49cb3..4a0096150a 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -634,6 +634,16 @@ export async function main() { await config.initialize(); startupProfiler.flush(config); + // If not a TTY, read from stdin + // This is for cases where the user pipes input directly into the command + let stdinData: string | undefined = undefined; + if (!process.stdin.isTTY) { + stdinData = await readStdin(); + if (stdinData) { + input = input ? `${stdinData}\n\n${input}` : stdinData; + } + } + // Fire SessionStart hook through MessageBus (only if hooks are enabled) // Must be called AFTER config.initialize() to ensure HookRegistry is loaded const hooksEnabled = config.getEnableHooks(); @@ -642,7 +652,23 @@ export async function main() { const sessionStartSource = resumedSessionData ? SessionStartSource.Resume : SessionStartSource.Startup; - await fireSessionStartHook(hookMessageBus, sessionStartSource); + const result = await fireSessionStartHook( + hookMessageBus, + sessionStartSource, + ); + + if (result) { + if (result.systemMessage) { + writeToStderr(result.systemMessage + '\n'); + } + const additionalContext = result.getAdditionalContext(); + if (additionalContext) { + // Prepend context to input (System Context -> Stdin -> Question) + input = input + ? `${additionalContext}\n\n${input}` + : additionalContext; + } + } // Register SessionEnd hook for graceful exit registerCleanup(async () => { @@ -650,14 +676,6 @@ export async function main() { }); } - // If not a TTY, read from stdin - // This is for cases where the user pipes input directly into the command - if (!process.stdin.isTTY) { - const stdinData = await readStdin(); - if (stdinData) { - input = `${stdinData}\n\n${input}`; - } - } if (!input) { debugLogger.error( `No input provided via stdin. Input can be provided by piping data into gemini or using the --prompt option.`, diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index bba0f1fd4e..a7ac42ce66 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -300,7 +300,31 @@ export const AppContainer = (props: AppContainerProps) => { const sessionStartSource = resumedSessionData ? SessionStartSource.Resume : SessionStartSource.Startup; - await fireSessionStartHook(hookMessageBus, sessionStartSource); + const result = await fireSessionStartHook( + hookMessageBus, + sessionStartSource, + ); + + if (result) { + if (result.systemMessage) { + historyManager.addItem( + { + type: MessageType.INFO, + text: result.systemMessage, + }, + Date.now(), + ); + } + + const additionalContext = result.getAdditionalContext(); + const geminiClient = config.getGeminiClient(); + if (additionalContext && geminiClient) { + await geminiClient.addHistory({ + role: 'user', + parts: [{ text: additionalContext }], + }); + } + } } // Fire-and-forget: generate summary for previous session in background @@ -321,6 +345,12 @@ export const AppContainer = (props: AppContainerProps) => { await fireSessionEndHook(hookMessageBus, SessionEndReason.Exit); } }); + // Disable the dependencies check here. historyManager gets flagged + // but we don't want to react to changes to it because each new history + // item, including the ones from the start session hook will cause a + // re-render and an error when we try to reload config. + // + // eslint-disable-next-line react-hooks/exhaustive-deps }, [config, resumedSessionData]); useEffect( diff --git a/packages/cli/src/ui/commands/clearCommand.ts b/packages/cli/src/ui/commands/clearCommand.ts index d2edbebbf2..ec8b7a52ef 100644 --- a/packages/cli/src/ui/commands/clearCommand.ts +++ b/packages/cli/src/ui/commands/clearCommand.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { DefaultHookOutput } from '@google/gemini-cli-core'; import { uiTelemetryService, fireSessionEndHook, @@ -14,6 +15,7 @@ import { } from '@google/gemini-cli-core'; import type { SlashCommand } from './types.js'; import { CommandKind } from './types.js'; +import { MessageType } from '../types.js'; import { randomUUID } from 'node:crypto'; export const clearCommand: SlashCommand = { @@ -52,8 +54,9 @@ export const clearCommand: SlashCommand = { } // Fire SessionStart hook after clearing + let result: DefaultHookOutput | undefined; if (config?.getEnableHooks() && messageBus) { - await fireSessionStartHook(messageBus, SessionStartSource.Clear); + result = await fireSessionStartHook(messageBus, SessionStartSource.Clear); } // Give the event loop a chance to process any pending telemetry operations @@ -68,5 +71,15 @@ export const clearCommand: SlashCommand = { uiTelemetryService.setLastPromptTokenCount(0); context.ui.clear(); + + if (result?.systemMessage) { + context.ui.addItem( + { + type: MessageType.INFO, + text: result.systemMessage, + }, + Date.now(), + ); + } }, }; diff --git a/packages/core/src/core/contentGenerator.test.ts b/packages/core/src/core/contentGenerator.test.ts index 6acae4f57e..9e10558a18 100644 --- a/packages/core/src/core/contentGenerator.test.ts +++ b/packages/core/src/core/contentGenerator.test.ts @@ -61,7 +61,9 @@ describe('createContentGenerator', () => { expect(FakeContentGenerator.fromFile).toHaveBeenCalledWith( fakeResponsesFile, ); - expect(generator).toEqual(mockGenerator); + expect(generator).toEqual( + new LoggingContentGenerator(mockGenerator, mockConfigWithFake), + ); }); it('should create a RecordingContentGenerator', async () => { diff --git a/packages/core/src/core/contentGenerator.ts b/packages/core/src/core/contentGenerator.ts index e4b568b871..12e07790cc 100644 --- a/packages/core/src/core/contentGenerator.ts +++ b/packages/core/src/core/contentGenerator.ts @@ -114,7 +114,10 @@ export async function createContentGenerator( ): Promise { const generator = await (async () => { if (gcConfig.fakeResponses) { - return FakeContentGenerator.fromFile(gcConfig.fakeResponses); + return new LoggingContentGenerator( + await FakeContentGenerator.fromFile(gcConfig.fakeResponses), + gcConfig, + ); } const version = await getVersion(); const model = resolveModel( diff --git a/packages/core/src/core/sessionHookTriggers.ts b/packages/core/src/core/sessionHookTriggers.ts index 524fe9beb9..149a84edbd 100644 --- a/packages/core/src/core/sessionHookTriggers.ts +++ b/packages/core/src/core/sessionHookTriggers.ts @@ -10,10 +10,12 @@ import { type HookExecutionRequest, type HookExecutionResponse, } from '../confirmation-bus/types.js'; -import type { - SessionStartSource, - SessionEndReason, - PreCompressTrigger, +import { + type SessionStartSource, + type SessionEndReason, + type PreCompressTrigger, + createHookOutput, + type DefaultHookOutput, } from '../hooks/types.js'; import { debugLogger } from '../utils/debugLogger.js'; @@ -22,13 +24,17 @@ import { debugLogger } from '../utils/debugLogger.js'; * * @param messageBus The message bus to use for hook communication * @param source The source/trigger of the session start + * @returns The output from the SessionStart hook, or undefined if failed/no output */ export async function fireSessionStartHook( messageBus: MessageBus, source: SessionStartSource, -): Promise { +): Promise { try { - await messageBus.request( + const response = await messageBus.request< + HookExecutionRequest, + HookExecutionResponse + >( { type: MessageBusType.HOOK_EXECUTION_REQUEST, eventName: 'SessionStart', @@ -38,8 +44,14 @@ export async function fireSessionStartHook( }, MessageBusType.HOOK_EXECUTION_RESPONSE, ); + + if (response.output) { + return createHookOutput('SessionStart', response.output); + } + return undefined; } catch (error) { debugLogger.debug(`SessionStart hook failed:`, error); + return undefined; } } From ed8bad8c26ef1134de9794c8983fd912315605be Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Mon, 5 Jan 2026 13:32:02 -0800 Subject: [PATCH 013/591] Fix order of preflight (#15941) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 306652221d..5190f4ae23 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,7 @@ "lint:all": "node scripts/lint.js", "format": "prettier --experimental-cli --write .", "typecheck": "npm run typecheck --workspaces --if-present", - "preflight": "npm run clean && npm ci && npm run format && npm run lint:ci && npm run build && npm run typecheck && npm run test:ci", + "preflight": "npm run clean && npm ci && npm run format && npm run build && npm run lint:ci && npm run typecheck && npm run test:ci", "prepare": "husky && npm run bundle", "prepare:package": "node scripts/prepare-package.js", "release:version": "node scripts/version.js", From fd7b6bf40a9c274f92dbfc576f4157217b0101f8 Mon Sep 17 00:00:00 2001 From: Christian Gunderman Date: Mon, 5 Jan 2026 13:41:35 -0800 Subject: [PATCH 014/591] Fix failing unit tests (#15940) --- packages/core/src/code_assist/oauth2.test.ts | 38 ++++++++------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/packages/core/src/code_assist/oauth2.test.ts b/packages/core/src/code_assist/oauth2.test.ts index 8e37537763..63e63c897c 100644 --- a/packages/core/src/code_assist/oauth2.test.ts +++ b/packages/core/src/code_assist/oauth2.test.ts @@ -1129,21 +1129,7 @@ describe('oauth2', () => { ); // Spy on process.on to capture the SIGINT handler - let sigIntHandler: (() => void) | undefined; - const originalOn = process.on; - const processOnSpy = vi - .spyOn(process, 'on') - .mockImplementation( - ( - event: string | symbol, - listener: (...args: unknown[]) => void, - ) => { - if (event === 'SIGINT') { - sigIntHandler = listener as () => void; - } - return originalOn.call(process, event, listener); - }, - ); + const processOnSpy = vi.spyOn(process, 'on'); const processRemoveListenerSpy = vi.spyOn(process, 'removeListener'); const clientPromise = getOauthClient( @@ -1154,6 +1140,11 @@ describe('oauth2', () => { // Wait a tick to ensure the SIGINT handler is registered await new Promise((resolve) => setTimeout(resolve, 0)); + const sigintCall = processOnSpy.mock.calls.find( + (call) => call[0] === 'SIGINT', + ); + const sigIntHandler = sigintCall?.[1] as (() => void) | undefined; + expect(sigIntHandler).toBeDefined(); // Trigger SIGINT @@ -1196,15 +1187,7 @@ describe('oauth2', () => { ); // Spy on process.stdin.on - let dataHandler: ((data: Buffer) => void) | undefined; - const stdinOnSpy = vi - .spyOn(process.stdin, 'on') - .mockImplementation((event: string | symbol, listener) => { - if (event === 'data') { - dataHandler = listener as (data: Buffer) => void; - } - return process.stdin; - }); + const stdinOnSpy = vi.spyOn(process.stdin, 'on'); const stdinRemoveListenerSpy = vi.spyOn( process.stdin, 'removeListener', @@ -1217,6 +1200,13 @@ describe('oauth2', () => { await new Promise((resolve) => setTimeout(resolve, 0)); + const dataCall = stdinOnSpy.mock.calls.find( + (call: [string, ...unknown[]]) => call[0] === 'data', + ); + const dataHandler = dataCall?.[1] as + | ((data: Buffer) => void) + | undefined; + expect(dataHandler).toBeDefined(); // Trigger Ctrl+C From 8f0324d868904bece4d370d06d5a3ed275ae9ade Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Mon, 5 Jan 2026 14:46:23 -0800 Subject: [PATCH 015/591] fix(cli): resolve paste issue on Windows terminals. (#15932) --- .../context-compress-interactive.test.ts | 2 +- integration-tests/extensions-reload.test.ts | 21 ++- .../file-system-interactive.test.ts | 4 +- integration-tests/hooks-system.test.ts | 8 +- .../mcp_server_cyclic_schema.test.ts | 2 +- integration-tests/test-helper.ts | 6 + packages/cli/src/ui/AppContainer.tsx | 6 +- .../src/ui/components/InputPrompt.test.tsx | 5 + .../src/ui/components/SettingsDialog.test.tsx | 5 + .../cli/src/ui/contexts/KeypressContext.tsx | 47 +++++-- .../cli/src/ui/hooks/useBracketedPaste.ts | 38 ------ packages/cli/src/ui/utils/bracketedPaste.ts | 18 --- .../utils/terminalCapabilityManager.test.ts | 44 ++++++ .../src/ui/utils/terminalCapabilityManager.ts | 125 +++++++++++++----- packages/core/src/utils/terminal.ts | 8 ++ 15 files changed, 224 insertions(+), 115 deletions(-) delete mode 100644 packages/cli/src/ui/hooks/useBracketedPaste.ts delete mode 100644 packages/cli/src/ui/utils/bracketedPaste.ts diff --git a/integration-tests/context-compress-interactive.test.ts b/integration-tests/context-compress-interactive.test.ts index 49f5e2aa7c..c7e04c6c23 100644 --- a/integration-tests/context-compress-interactive.test.ts +++ b/integration-tests/context-compress-interactive.test.ts @@ -32,7 +32,7 @@ describe('Interactive Mode', () => { await run.sendKeys( 'Write a 200 word story about a robot. The story MUST end with the text THE_END followed by a period.', ); - await run.sendKeys('\r'); + await run.type('\r'); // Wait for the specific end marker. await run.expectText('THE_END.', 30000); diff --git a/integration-tests/extensions-reload.test.ts b/integration-tests/extensions-reload.test.ts index 29db8522ad..520076d7c6 100644 --- a/integration-tests/extensions-reload.test.ts +++ b/integration-tests/extensions-reload.test.ts @@ -95,7 +95,10 @@ describe('extension reloading', () => { // Poll for the updated list await rig.pollCommand( - () => run.sendKeys('\u0015/mcp list\r'), + async () => { + await run.sendText('/mcp list'); + await run.type('\r'); + }, () => { const output = stripAnsi(run.output); return ( @@ -110,9 +113,9 @@ describe('extension reloading', () => { // Update the extension, expect the list to update, and mcp servers as well. await run.sendKeys('\u0015/extensions update test-extension'); await run.expectText('/extensions update test-extension'); - await run.sendKeys('\r'); + await run.type('\r'); await new Promise((resolve) => setTimeout(resolve, 500)); - await run.sendKeys('\r'); + await run.type('\r'); await run.expectText( ` * test-server (remote): http://localhost:${portB}/mcp`, ); @@ -123,7 +126,10 @@ describe('extension reloading', () => { // Poll for the updated extension version await rig.pollCommand( - () => run.sendKeys('\u0015/extensions list\r'), + async () => { + await run.sendText('/extensions list'); + await run.type('\r'); + }, () => stripAnsi(run.output).includes( 'test-extension (v0.0.2) - active (updated)', @@ -133,7 +139,10 @@ describe('extension reloading', () => { // Poll for the updated mcp tool await rig.pollCommand( - () => run.sendKeys('\u0015/mcp list\r'), + async () => { + await run.sendText('/mcp list'); + await run.type('\r'); + }, () => { const output = stripAnsi(run.output); return ( @@ -146,7 +155,7 @@ describe('extension reloading', () => { ); await run.sendText('/quit'); - await run.sendKeys('\r'); + await run.type('\r'); // Clean things up. await serverA.stop(); diff --git a/integration-tests/file-system-interactive.test.ts b/integration-tests/file-system-interactive.test.ts index d7ad73fd0d..6f955a1378 100644 --- a/integration-tests/file-system-interactive.test.ts +++ b/integration-tests/file-system-interactive.test.ts @@ -37,7 +37,7 @@ describe('Interactive file system', () => { // Step 1: Read the file const readPrompt = `Read the version from ${fileName}`; await run.type(readPrompt); - await run.sendKeys('\r'); + await run.type('\r'); const readCall = await rig.waitForToolCall('read_file', 30000); expect(readCall, 'Expected to find a read_file tool call').toBe(true); @@ -45,7 +45,7 @@ describe('Interactive file system', () => { // Step 2: Write the file const writePrompt = `now change the version to 1.0.1 in the file`; await run.type(writePrompt); - await run.sendKeys('\r'); + await run.type('\r'); // Check tool calls made with right args await rig.expectToolCallSuccess( diff --git a/integration-tests/hooks-system.test.ts b/integration-tests/hooks-system.test.ts index c5ab2b024d..34827a5f7c 100644 --- a/integration-tests/hooks-system.test.ts +++ b/integration-tests/hooks-system.test.ts @@ -1087,7 +1087,7 @@ console.log(JSON.stringify({ // Send a prompt to establish a session and trigger an API call await run.sendKeys('Hello'); - await run.sendKeys('\r'); + await run.type('\r'); // Wait for response to ensure API call happened await run.expectText('Hello', 15000); @@ -1166,7 +1166,7 @@ console.log(JSON.stringify({ // Send an initial prompt to establish a session await run.sendKeys('Say hello'); - await run.sendKeys('\r'); + await run.type('\r'); // Wait for the response await run.expectText('Hello', 10000); @@ -1176,14 +1176,14 @@ console.log(JSON.stringify({ const numClears = 3; for (let i = 0; i < numClears; i++) { await run.sendKeys('/clear'); - await run.sendKeys('\r'); + await run.type('\r'); // Wait a bit for clear to complete await new Promise((resolve) => setTimeout(resolve, 2000)); // Send a prompt to establish an active session before next clear await run.sendKeys('Say hello'); - await run.sendKeys('\r'); + await run.type('\r'); // Wait for response await run.expectText('Hello', 10000); diff --git a/integration-tests/mcp_server_cyclic_schema.test.ts b/integration-tests/mcp_server_cyclic_schema.test.ts index 742a35fe78..29373dbac4 100644 --- a/integration-tests/mcp_server_cyclic_schema.test.ts +++ b/integration-tests/mcp_server_cyclic_schema.test.ts @@ -200,7 +200,7 @@ describe('mcp server with cyclic tool schema is detected', () => { const run = await rig.runInteractive(); await run.type('/mcp list'); - await run.sendKeys('\r'); + await run.type('\r'); await run.expectText('tool_with_cyclic_schema'); }); diff --git a/integration-tests/test-helper.ts b/integration-tests/test-helper.ts index e197c724a5..8fc4208ebf 100644 --- a/integration-tests/test-helper.ts +++ b/integration-tests/test-helper.ts @@ -209,6 +209,12 @@ export class InteractiveRun { async type(text: string) { let typedSoFar = ''; for (const char of text) { + if (char === '\r') { + // wait >30ms before `enter` to avoid fast return conversion + // from bufferFastReturn() in KeypressContent.tsx + await new Promise((resolve) => setTimeout(resolve, 50)); + } + this.ptyProcess.write(char); typedSoFar += char; diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index a7ac42ce66..ce5654b443 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -91,7 +91,6 @@ import { useVim } from './hooks/vim.js'; import { type LoadableSettingScope, SettingScope } from '../config/settings.js'; import { type InitializationResult } from '../core/initializer.js'; import { useFocus } from './hooks/useFocus.js'; -import { useBracketedPaste } from './hooks/useBracketedPaste.js'; import { useKeypress, type Key } from './hooks/useKeypress.js'; import { keyMatchers, Command } from './keyMatchers.js'; import { useLoadingIndicator } from './hooks/useLoadingIndicator.js'; @@ -123,7 +122,6 @@ import { useAlternateBuffer } from './hooks/useAlternateBuffer.js'; import { useSettings } from './contexts/SettingsContext.js'; import { terminalCapabilityManager } from './utils/terminalCapabilityManager.js'; import { useInputHistoryStore } from './hooks/useInputHistoryStore.js'; -import { enableBracketedPaste } from './utils/bracketedPaste.js'; import { useBanner } from './hooks/useBanner.js'; const WARNING_PROMPT_DURATION_MS = 1000; @@ -424,8 +422,7 @@ export const AppContainer = (props: AppContainerProps) => { disableLineWrapping(); app.rerender(); } - enableBracketedPaste(); - terminalCapabilityManager.enableKittyProtocol(); + terminalCapabilityManager.enableSupportedModes(); refreshStatic(); }, [refreshStatic, isAlternateBuffer, app, config]); @@ -925,7 +922,6 @@ Logging in with Google... Restarting Gemini CLI to continue. }); const isFocused = useFocus(); - useBracketedPaste(); // Context file names computation const contextFileNames = useMemo(() => { diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index 35ac7d4a71..0c273cac86 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -31,6 +31,7 @@ import { useReverseSearchCompletion } from '../hooks/useReverseSearchCompletion. import clipboardy from 'clipboardy'; import * as clipboardUtils from '../utils/clipboardUtils.js'; import { useKittyKeyboardProtocol } from '../hooks/useKittyKeyboardProtocol.js'; +import { terminalCapabilityManager } from '../utils/terminalCapabilityManager.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import stripAnsi from 'strip-ansi'; import chalk from 'chalk'; @@ -121,6 +122,10 @@ describe('InputPrompt', () => { beforeEach(() => { vi.resetAllMocks(); + vi.spyOn( + terminalCapabilityManager, + 'isBracketedPasteEnabled', + ).mockReturnValue(true); mockCommandContext = createMockCommandContext(); diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index 66ad349a9a..82750be0ee 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -35,6 +35,7 @@ import { type SettingDefinition, type SettingsSchemaType, } from '../../config/settingsSchema.js'; +import { terminalCapabilityManager } from '../../ui/utils/terminalCapabilityManager.js'; // Mock the VimModeContext const mockToggleVimEnabled = vi.fn(); @@ -253,6 +254,10 @@ const renderDialog = ( describe('SettingsDialog', () => { beforeEach(() => { + vi.spyOn( + terminalCapabilityManager, + 'isBracketedPasteEnabled', + ).mockReturnValue(true); mockToggleVimEnabled.mockResolvedValue(true); }); diff --git a/packages/cli/src/ui/contexts/KeypressContext.tsx b/packages/cli/src/ui/contexts/KeypressContext.tsx index 1c88e128c7..8bfe51694f 100644 --- a/packages/cli/src/ui/contexts/KeypressContext.tsx +++ b/packages/cli/src/ui/contexts/KeypressContext.tsx @@ -19,10 +19,12 @@ import { ESC } from '../utils/input.js'; import { parseMouseEvent } from '../utils/mouse.js'; import { FOCUS_IN, FOCUS_OUT } from '../hooks/useFocus.js'; import { appEvents, AppEvent } from '../../utils/events.js'; +import { terminalCapabilityManager } from '../utils/terminalCapabilityManager.js'; export const BACKSLASH_ENTER_TIMEOUT = 5; export const ESC_TIMEOUT = 50; export const PASTE_TIMEOUT = 30_000; +export const FAST_RETURN_TIMEOUT = 30; // Parse the key itself const KEY_INFO_MAP: Record< @@ -148,7 +150,7 @@ function nonKeyboardEventFilter( */ function bufferBackslashEnter( keypressHandler: KeypressHandler, -): (key: Key | null) => void { +): KeypressHandler { const bufferer = (function* (): Generator { while (true) { const key = yield; @@ -184,7 +186,31 @@ function bufferBackslashEnter( bufferer.next(); // prime the generator so it starts listening. - return (key: Key | null) => bufferer.next(key); + return (key: Key) => bufferer.next(key); +} + +/** + * Converts return keys pressed quickly after other keys into plain + * insertable return characters. + * + * This is to accomodate older terminals that paste text without bracketing. + */ +function bufferFastReturn(keypressHandler: KeypressHandler): KeypressHandler { + let lastKeyTime = 0; + return (key: Key) => { + const now = Date.now(); + if (key.name === 'return' && now - lastKeyTime <= FAST_RETURN_TIMEOUT) { + keypressHandler({ + ...key, + name: '', + sequence: '\r', + insertable: true, + }); + } else { + keypressHandler(key); + } + lastKeyTime = now; + }; } /** @@ -192,9 +218,7 @@ function bufferBackslashEnter( * Will flush the buffer if no data is received for PASTE_TIMEOUT ms or * when a null key is received. */ -function bufferPaste( - keypressHandler: KeypressHandler, -): (key: Key | null) => void { +function bufferPaste(keypressHandler: KeypressHandler): KeypressHandler { const bufferer = (function* (): Generator { while (true) { let key = yield; @@ -238,7 +262,7 @@ function bufferPaste( })(); bufferer.next(); // prime the generator so it starts listening. - return (key: Key | null) => bufferer.next(key); + return (key: Key) => bufferer.next(key); } /** @@ -592,10 +616,13 @@ export function KeypressProvider({ process.stdin.setEncoding('utf8'); // Make data events emit strings - const mouseFilterer = nonKeyboardEventFilter(broadcast); - const backslashBufferer = bufferBackslashEnter(mouseFilterer); - const pasteBufferer = bufferPaste(backslashBufferer); - let dataListener = createDataListener(pasteBufferer); + let processor = nonKeyboardEventFilter(broadcast); + if (!terminalCapabilityManager.isBracketedPasteEnabled()) { + processor = bufferFastReturn(processor); + } + processor = bufferBackslashEnter(processor); + processor = bufferPaste(processor); + let dataListener = createDataListener(processor); if (debugKeystrokeLogging) { const old = dataListener; diff --git a/packages/cli/src/ui/hooks/useBracketedPaste.ts b/packages/cli/src/ui/hooks/useBracketedPaste.ts deleted file mode 100644 index 1e9cbbebcf..0000000000 --- a/packages/cli/src/ui/hooks/useBracketedPaste.ts +++ /dev/null @@ -1,38 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { useEffect } from 'react'; -import { - disableBracketedPaste, - enableBracketedPaste, -} from '../utils/bracketedPaste.js'; - -/** - * Enables and disables bracketed paste mode in the terminal. - * - * This hook ensures that bracketed paste mode is enabled when the component - * mounts and disabled when it unmounts or when the process exits. - */ -export const useBracketedPaste = () => { - const cleanup = () => { - disableBracketedPaste(); - }; - - useEffect(() => { - enableBracketedPaste(); - - process.on('exit', cleanup); - process.on('SIGINT', cleanup); - process.on('SIGTERM', cleanup); - - return () => { - cleanup(); - process.removeListener('exit', cleanup); - process.removeListener('SIGINT', cleanup); - process.removeListener('SIGTERM', cleanup); - }; - }, []); -}; diff --git a/packages/cli/src/ui/utils/bracketedPaste.ts b/packages/cli/src/ui/utils/bracketedPaste.ts deleted file mode 100644 index 26bb0e08fa..0000000000 --- a/packages/cli/src/ui/utils/bracketedPaste.ts +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { writeToStdout } from '@google/gemini-cli-core'; - -const ENABLE_BRACKETED_PASTE = '\x1b[?2004h'; -const DISABLE_BRACKETED_PASTE = '\x1b[?2004l'; - -export const enableBracketedPaste = () => { - writeToStdout(ENABLE_BRACKETED_PASTE); -}; - -export const disableBracketedPaste = () => { - writeToStdout(DISABLE_BRACKETED_PASTE); -}; diff --git a/packages/cli/src/ui/utils/terminalCapabilityManager.test.ts b/packages/cli/src/ui/utils/terminalCapabilityManager.test.ts index 42f59f95a1..bd58fc5cab 100644 --- a/packages/cli/src/ui/utils/terminalCapabilityManager.test.ts +++ b/packages/cli/src/ui/utils/terminalCapabilityManager.test.ts @@ -23,6 +23,8 @@ vi.mock('@google/gemini-cli-core', () => ({ disableKittyKeyboardProtocol: vi.fn(), enableModifyOtherKeys: vi.fn(), disableModifyOtherKeys: vi.fn(), + enableBracketedPasteMode: vi.fn(), + disableBracketedPasteMode: vi.fn(), })); describe('TerminalCapabilityManager', () => { @@ -264,4 +266,46 @@ describe('TerminalCapabilityManager', () => { expect(manager.isModifyOtherKeysEnabled()).toBe(true); }); }); + + describe('bracketed paste detection', () => { + it('should detect bracketed paste support (mode set)', async () => { + const manager = TerminalCapabilityManager.getInstance(); + const promise = manager.detectCapabilities(); + + // Simulate bracketed paste response: \x1b[?2004;1$y + stdin.emit('data', Buffer.from('\x1b[?2004;1$y')); + // Complete detection with DA1 + stdin.emit('data', Buffer.from('\x1b[?62c')); + + await promise; + expect(manager.isBracketedPasteSupported()).toBe(true); + expect(manager.isBracketedPasteEnabled()).toBe(true); + }); + + it('should detect bracketed paste support (mode reset)', async () => { + const manager = TerminalCapabilityManager.getInstance(); + const promise = manager.detectCapabilities(); + + // Simulate bracketed paste response: \x1b[?2004;2$y + stdin.emit('data', Buffer.from('\x1b[?2004;2$y')); + // Complete detection with DA1 + stdin.emit('data', Buffer.from('\x1b[?62c')); + + await promise; + expect(manager.isBracketedPasteSupported()).toBe(true); + expect(manager.isBracketedPasteEnabled()).toBe(true); + }); + + it('should not enable bracketed paste if not supported', async () => { + const manager = TerminalCapabilityManager.getInstance(); + const promise = manager.detectCapabilities(); + + // Complete detection with DA1 only + stdin.emit('data', Buffer.from('\x1b[?62c')); + + await promise; + expect(manager.isBracketedPasteSupported()).toBe(false); + expect(manager.isBracketedPasteEnabled()).toBe(false); + }); + }); }); diff --git a/packages/cli/src/ui/utils/terminalCapabilityManager.ts b/packages/cli/src/ui/utils/terminalCapabilityManager.ts index c893c46043..f6838a79a0 100644 --- a/packages/cli/src/ui/utils/terminalCapabilityManager.ts +++ b/packages/cli/src/ui/utils/terminalCapabilityManager.ts @@ -11,6 +11,8 @@ import { disableKittyKeyboardProtocol, enableModifyOtherKeys, disableModifyOtherKeys, + enableBracketedPasteMode, + disableBracketedPasteMode, } from '@google/gemini-cli-core'; export type TerminalBackgroundColor = string | undefined; @@ -23,6 +25,7 @@ export class TerminalCapabilityManager { private static readonly TERMINAL_NAME_QUERY = '\x1b[>q'; private static readonly DEVICE_ATTRIBUTES_QUERY = '\x1b[c'; private static readonly MODIFY_OTHER_KEYS_QUERY = '\x1b[>4;?m'; + private static readonly BRACKETED_PASTE_QUERY = '\x1b[?2004$p'; // Kitty keyboard flags: CSI ? flags u // eslint-disable-next-line no-control-regex @@ -40,6 +43,10 @@ export class TerminalCapabilityManager { // modifyOtherKeys response: CSI > 4 ; level m // eslint-disable-next-line no-control-regex private static readonly MODIFY_OTHER_KEYS_REGEX = /\x1b\[>4;(\d+)m/; + // DECRQM response for bracketed paste: CSI ? 2004 ; Ps $ y + // Ps = 1 (set), 2 (reset), 3 (permanently set), 4 (permanently reset) + // eslint-disable-next-line no-control-regex + private static readonly BRACKETED_PASTE_REGEX = /\x1b\[\?2004;([1-4])\$y/; private terminalBackgroundColor: TerminalBackgroundColor; private kittySupported = false; @@ -48,6 +55,8 @@ export class TerminalCapabilityManager { private terminalName: string | undefined; private modifyOtherKeysSupported = false; private modifyOtherKeysEnabled = false; + private bracketedPasteSupported = false; + private bracketedPasteEnabled = false; private constructor() {} @@ -75,6 +84,21 @@ export class TerminalCapabilityManager { return; } + const cleanupOnExit = () => { + if (this.kittySupported) { + this.disableKittyProtocol(); + } + if (this.modifyOtherKeysSupported) { + this.disableModifyOtherKeys(); + } + if (this.bracketedPasteSupported) { + this.disableBracketedPaste(); + } + }; + process.on('exit', () => cleanupOnExit); + process.on('SIGTERM', () => cleanupOnExit); + process.on('SIGINT', cleanupOnExit); + return new Promise((resolve) => { const originalRawMode = process.stdin.isRaw; if (!originalRawMode) { @@ -87,6 +111,7 @@ export class TerminalCapabilityManager { let deviceAttributesReceived = false; let bgReceived = false; let modifyOtherKeysReceived = false; + let bracketedPasteReceived = false; // eslint-disable-next-line prefer-const let timeoutId: NodeJS.Timeout; @@ -100,27 +125,14 @@ export class TerminalCapabilityManager { } this.detectionComplete = true; - // Auto-enable kitty if supported - if (this.kittySupported) { - this.enableKittyProtocol(); - process.on('exit', () => this.disableKittyProtocol()); - process.on('SIGTERM', () => this.disableKittyProtocol()); - } else if (this.modifyOtherKeysSupported) { - this.enableModifyOtherKeys(); - process.on('exit', () => this.disableModifyOtherKeys()); - process.on('SIGTERM', () => this.disableModifyOtherKeys()); - } + this.enableSupportedModes(); resolve(); }; - const onTimeout = () => { - cleanup(); - }; - // A somewhat long timeout is acceptable as all terminals should respond // to the device attributes query used as a sentinel. - timeoutId = setTimeout(onTimeout, 1000); + timeoutId = setTimeout(cleanup, 1000); const onData = (data: Buffer) => { buffer += data.toString(); @@ -149,6 +161,32 @@ export class TerminalCapabilityManager { this.kittySupported = true; } + // check for modifyOtherKeys support + if (!modifyOtherKeysReceived) { + const match = buffer.match( + TerminalCapabilityManager.MODIFY_OTHER_KEYS_REGEX, + ); + if (match) { + modifyOtherKeysReceived = true; + const level = parseInt(match[1], 10); + this.modifyOtherKeysSupported = level >= 2; + debugLogger.log( + `Detected modifyOtherKeys support: ${this.modifyOtherKeysSupported} (level ${level})`, + ); + } + } + + // check for bracketed paste support + if (!bracketedPasteReceived) { + const match = buffer.match( + TerminalCapabilityManager.BRACKETED_PASTE_REGEX, + ); + if (match) { + bracketedPasteReceived = true; + this.bracketedPasteSupported = true; + } + } + // Check for Terminal Name/Version response. if (!terminalNameReceived) { const match = buffer.match( @@ -174,21 +212,6 @@ export class TerminalCapabilityManager { cleanup(); } } - - // check for modifyOtherKeys support - if (!modifyOtherKeysReceived) { - const match = buffer.match( - TerminalCapabilityManager.MODIFY_OTHER_KEYS_REGEX, - ); - if (match) { - modifyOtherKeysReceived = true; - const level = parseInt(match[1], 10); - this.modifyOtherKeysSupported = level >= 2; - debugLogger.log( - `Detected modifyOtherKeys support: ${this.modifyOtherKeysSupported} (level ${level})`, - ); - } - } }; process.stdin.on('data', onData); @@ -200,6 +223,7 @@ export class TerminalCapabilityManager { TerminalCapabilityManager.OSC_11_QUERY + TerminalCapabilityManager.TERMINAL_NAME_QUERY + TerminalCapabilityManager.MODIFY_OTHER_KEYS_QUERY + + TerminalCapabilityManager.BRACKETED_PASTE_QUERY + TerminalCapabilityManager.DEVICE_ATTRIBUTES_QUERY, ); } catch (e) { @@ -209,6 +233,17 @@ export class TerminalCapabilityManager { }); } + enableSupportedModes() { + if (this.kittySupported) { + this.enableKittyProtocol(); + } else if (this.modifyOtherKeysSupported) { + this.enableModifyOtherKeys(); + } + if (this.bracketedPasteSupported) { + this.enableBracketedPaste(); + } + } + getTerminalBackgroundColor(): TerminalBackgroundColor { return this.terminalBackgroundColor; } @@ -221,6 +256,36 @@ export class TerminalCapabilityManager { return this.kittyEnabled; } + isBracketedPasteSupported(): boolean { + return this.bracketedPasteSupported; + } + + isBracketedPasteEnabled(): boolean { + return this.bracketedPasteEnabled; + } + + enableBracketedPaste(): void { + try { + if (this.bracketedPasteSupported) { + enableBracketedPasteMode(); + this.bracketedPasteEnabled = true; + } + } catch (e) { + debugLogger.warn('Failed to enable bracketed paste mode:', e); + } + } + + disableBracketedPaste(): void { + try { + if (this.bracketedPasteEnabled) { + disableBracketedPasteMode(); + this.bracketedPasteEnabled = false; + } + } catch (e) { + debugLogger.warn('Failed to disable bracketed paste mode:', e); + } + } + enableKittyProtocol(): void { try { if (this.kittySupported) { diff --git a/packages/core/src/utils/terminal.ts b/packages/core/src/utils/terminal.ts index f8070a4a37..5e2fdb8bf0 100644 --- a/packages/core/src/utils/terminal.ts +++ b/packages/core/src/utils/terminal.ts @@ -34,6 +34,14 @@ export function disableModifyOtherKeys() { writeToStdout('\x1b[>4;0m'); } +export function enableBracketedPasteMode() { + writeToStdout('\x1b[?2004h'); +} + +export function disableBracketedPasteMode() { + writeToStdout('\x1b[?2004l'); +} + export function enableLineWrapping() { writeToStdout('\x1b[?7h'); } From 2cb33b2f764b19b4c7ce3ce1f9f3359542a4656d Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Mon, 5 Jan 2026 15:12:51 -0800 Subject: [PATCH 016/591] Agent Skills: Implement /skills reload (#15865) --- packages/cli/src/config/config.ts | 6 + packages/cli/src/config/settings.test.ts | 1 + packages/cli/src/config/settings.ts | 1 + packages/cli/src/ui/AppContainer.tsx | 14 ++ .../cli/src/ui/commands/skillsCommand.test.ts | 177 +++++++++++++++++- packages/cli/src/ui/commands/skillsCommand.ts | 116 +++++++++++- .../cli/src/ui/contexts/UIStateContext.tsx | 1 + packages/core/src/config/config.test.ts | 104 ++++++++++ packages/core/src/config/config.ts | 39 +++- packages/core/src/tools/tool-registry.ts | 9 + packages/core/src/utils/events.ts | 9 + 11 files changed, 468 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index e08a50893f..1aee75940b 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -726,6 +726,12 @@ export async function loadCliConfig( hooks: settings.hooks || {}, projectHooks: projectHooks || {}, onModelChange: (model: string) => saveModelChange(loadedSettings, model), + onReload: async () => { + const refreshedSettings = loadSettings(cwd); + return { + disabledSkills: refreshedSettings.merged.skills?.disabled, + }; + }, }); } diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 133b50daae..df3fdbe9ea 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -114,6 +114,7 @@ vi.mock('./extension.js'); const mockCoreEvents = vi.hoisted(() => ({ emitFeedback: vi.fn(), + emitSettingsChanged: vi.fn(), })); vi.mock('@google/gemini-cli-core', async (importOriginal) => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index f347a04be2..5cba3dd637 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -534,6 +534,7 @@ export class LoadedSettings { setNestedProperty(settingsFile.originalSettings, key, value); this._merged = this.computeMergedSettings(); saveSettings(settingsFile); + coreEvents.emitSettingsChanged(); } } diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index ce5654b443..f352556b06 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -188,6 +188,7 @@ export const AppContainer = (props: AppContainerProps) => { const [modelSwitchedFromQuotaError, setModelSwitchedFromQuotaError] = useState(false); const [historyRemountKey, setHistoryRemountKey] = useState(0); + const [settingsNonce, setSettingsNonce] = useState(0); const [updateInfo, setUpdateInfo] = useState(null); const [isTrustedFolder, setIsTrustedFolder] = useState( isWorkspaceTrusted(settings.merged).isTrusted, @@ -368,6 +369,17 @@ export const AppContainer = (props: AppContainerProps) => { }; }, [config]); + useEffect(() => { + const handleSettingsChanged = () => { + setSettingsNonce((prev) => prev + 1); + }; + + coreEvents.on(CoreEvent.SettingsChanged, handleSettingsChanged); + return () => { + coreEvents.off(CoreEvent.SettingsChanged, handleSettingsChanged); + }; + }, []); + const { consoleMessages, clearConsoleMessages: clearConsoleMessagesState } = useConsoleMessages(); @@ -1546,6 +1558,7 @@ Logging in with Google... Restarting Gemini CLI to continue. bannerData, bannerVisible, terminalBackgroundColor: config.getTerminalBackground(), + settingsNonce, }), [ isThemeDialogOpen, @@ -1638,6 +1651,7 @@ Logging in with Google... Restarting Gemini CLI to continue. bannerData, bannerVisible, config, + settingsNonce, ], ); diff --git a/packages/cli/src/ui/commands/skillsCommand.test.ts b/packages/cli/src/ui/commands/skillsCommand.test.ts index 39339f8226..cba9c9ff4e 100644 --- a/packages/cli/src/ui/commands/skillsCommand.test.ts +++ b/packages/cli/src/ui/commands/skillsCommand.test.ts @@ -1,21 +1,22 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { skillsCommand } from './skillsCommand.js'; import { MessageType } from '../types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import type { CommandContext } from './types.js'; -import type { Config } from '@google/gemini-cli-core'; +import type { Config, SkillDefinition } from '@google/gemini-cli-core'; import { SettingScope, type LoadedSettings } from '../../config/settings.js'; describe('skillsCommand', () => { let context: CommandContext; beforeEach(() => { + vi.useFakeTimers(); const skills = [ { name: 'skill1', @@ -35,6 +36,7 @@ describe('skillsCommand', () => { config: { getSkillManager: vi.fn().mockReturnValue({ getAllSkills: vi.fn().mockReturnValue(skills), + getSkills: vi.fn().mockReturnValue(skills), getSkill: vi .fn() .mockImplementation( @@ -51,6 +53,11 @@ describe('skillsCommand', () => { }); }); + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + it('should add a SKILLS_LIST item to UI with descriptions by default', async () => { await skillsCommand.action!(context, ''); @@ -187,6 +194,170 @@ describe('skillsCommand', () => { }); }); + describe('reload', () => { + it('should reload skills successfully and show success message', async () => { + const reloadCmd = skillsCommand.subCommands!.find( + (s) => s.name === 'reload', + )!; + // Make reload take some time so timer can fire + const reloadSkillsMock = vi.fn().mockImplementation(async () => { + await new Promise((resolve) => setTimeout(resolve, 200)); + }); + // @ts-expect-error Mocking reloadSkills + context.services.config.reloadSkills = reloadSkillsMock; + + const actionPromise = reloadCmd.action!(context, ''); + + // Initially, no pending item (flicker prevention) + expect(context.ui.setPendingItem).not.toHaveBeenCalled(); + + // Fast forward 100ms to trigger the pending item + await vi.advanceTimersByTimeAsync(100); + expect(context.ui.setPendingItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.INFO, + text: 'Reloading agent skills...', + }), + ); + + // Fast forward another 100ms (reload complete), but pending item should stay + await vi.advanceTimersByTimeAsync(100); + expect(context.ui.setPendingItem).not.toHaveBeenCalledWith(null); + + // Fast forward to reach 500ms total + await vi.advanceTimersByTimeAsync(300); + await actionPromise; + + expect(reloadSkillsMock).toHaveBeenCalled(); + expect(context.ui.setPendingItem).toHaveBeenCalledWith(null); + expect(context.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.INFO, + text: 'Agent skills reloaded successfully.', + }), + expect.any(Number), + ); + }); + + it('should show new skills count after reload', async () => { + const reloadCmd = skillsCommand.subCommands!.find( + (s) => s.name === 'reload', + )!; + const reloadSkillsMock = vi.fn().mockImplementation(async () => { + const skillManager = context.services.config!.getSkillManager(); + vi.mocked(skillManager.getSkills).mockReturnValue([ + { name: 'skill1' }, + { name: 'skill2' }, + { name: 'skill3' }, + ] as SkillDefinition[]); + }); + // @ts-expect-error Mocking reloadSkills + context.services.config.reloadSkills = reloadSkillsMock; + + await reloadCmd.action!(context, ''); + + expect(context.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.INFO, + text: 'Agent skills reloaded successfully. 1 newly available skill.', + }), + expect.any(Number), + ); + }); + + it('should show removed skills count after reload', async () => { + const reloadCmd = skillsCommand.subCommands!.find( + (s) => s.name === 'reload', + )!; + const reloadSkillsMock = vi.fn().mockImplementation(async () => { + const skillManager = context.services.config!.getSkillManager(); + vi.mocked(skillManager.getSkills).mockReturnValue([ + { name: 'skill1' }, + ] as SkillDefinition[]); + }); + // @ts-expect-error Mocking reloadSkills + context.services.config.reloadSkills = reloadSkillsMock; + + await reloadCmd.action!(context, ''); + + expect(context.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.INFO, + text: 'Agent skills reloaded successfully. 1 skill no longer available.', + }), + expect.any(Number), + ); + }); + + it('should show both added and removed skills count after reload', async () => { + const reloadCmd = skillsCommand.subCommands!.find( + (s) => s.name === 'reload', + )!; + const reloadSkillsMock = vi.fn().mockImplementation(async () => { + const skillManager = context.services.config!.getSkillManager(); + vi.mocked(skillManager.getSkills).mockReturnValue([ + { name: 'skill2' }, // skill1 removed, skill3 added + { name: 'skill3' }, + ] as SkillDefinition[]); + }); + // @ts-expect-error Mocking reloadSkills + context.services.config.reloadSkills = reloadSkillsMock; + + await reloadCmd.action!(context, ''); + + expect(context.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.INFO, + text: 'Agent skills reloaded successfully. 1 newly available skill and 1 skill no longer available.', + }), + expect.any(Number), + ); + }); + + it('should show error if configuration is missing', async () => { + const reloadCmd = skillsCommand.subCommands!.find( + (s) => s.name === 'reload', + )!; + context.services.config = null; + + await reloadCmd.action!(context, ''); + + expect(context.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.ERROR, + text: 'Could not retrieve configuration.', + }), + expect.any(Number), + ); + }); + + it('should show error if reload fails', async () => { + const reloadCmd = skillsCommand.subCommands!.find( + (s) => s.name === 'reload', + )!; + const error = new Error('Reload failed'); + const reloadSkillsMock = vi.fn().mockImplementation(async () => { + await new Promise((_, reject) => setTimeout(() => reject(error), 200)); + }); + // @ts-expect-error Mocking reloadSkills + context.services.config.reloadSkills = reloadSkillsMock; + + const actionPromise = reloadCmd.action!(context, ''); + await vi.advanceTimersByTimeAsync(100); + await vi.advanceTimersByTimeAsync(400); + await actionPromise; + + expect(context.ui.setPendingItem).toHaveBeenCalledWith(null); + expect(context.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.ERROR, + text: 'Failed to reload skills: Reload failed', + }), + expect.any(Number), + ); + }); + }); + describe('completions', () => { it('should provide completions for disable (only enabled skills)', async () => { const disableCmd = skillsCommand.subCommands!.find( diff --git a/packages/cli/src/ui/commands/skillsCommand.ts b/packages/cli/src/ui/commands/skillsCommand.ts index e3cbc568a1..156516e9ea 100644 --- a/packages/cli/src/ui/commands/skillsCommand.ts +++ b/packages/cli/src/ui/commands/skillsCommand.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -10,7 +10,11 @@ import { type SlashCommandActionReturn, CommandKind, } from './types.js'; -import { MessageType, type HistoryItemSkillsList } from '../types.js'; +import { + MessageType, + type HistoryItemSkillsList, + type HistoryItemInfo, +} from '../types.js'; import { SettingScope } from '../../config/settings.js'; async function listAction( @@ -104,7 +108,7 @@ async function disableAction( context.ui.addItem( { type: MessageType.INFO, - text: `Skill "${skillName}" disabled in ${scope} settings. Restart required to take effect.`, + text: `Skill "${skillName}" disabled in ${scope} settings. Use "/skills reload" for it to take effect.`, }, Date.now(), ); @@ -148,12 +152,107 @@ async function enableAction( context.ui.addItem( { type: MessageType.INFO, - text: `Skill "${skillName}" enabled in ${scope} settings. Restart required to take effect.`, + text: `Skill "${skillName}" enabled in ${scope} settings. Use "/skills reload" for it to take effect.`, }, Date.now(), ); } +async function reloadAction( + context: CommandContext, +): Promise { + const config = context.services.config; + if (!config) { + context.ui.addItem( + { + type: MessageType.ERROR, + text: 'Could not retrieve configuration.', + }, + Date.now(), + ); + return; + } + + const skillManager = config.getSkillManager(); + const beforeNames = new Set(skillManager.getSkills().map((s) => s.name)); + + const startTime = Date.now(); + let pendingItemSet = false; + const pendingTimeout = setTimeout(() => { + context.ui.setPendingItem({ + type: MessageType.INFO, + text: 'Reloading agent skills...', + }); + pendingItemSet = true; + }, 100); + + try { + await config.reloadSkills(); + + clearTimeout(pendingTimeout); + if (pendingItemSet) { + // If we showed the pending item, make sure it stays for at least 500ms + // total to avoid a "flicker" where it appears and immediately disappears. + const elapsed = Date.now() - startTime; + const minVisibleDuration = 500; + if (elapsed < minVisibleDuration) { + await new Promise((resolve) => + setTimeout(resolve, minVisibleDuration - elapsed), + ); + } + context.ui.setPendingItem(null); + } + + const afterSkills = skillManager.getSkills(); + const afterNames = new Set(afterSkills.map((s) => s.name)); + + const added = afterSkills.filter((s) => !beforeNames.has(s.name)); + const removedCount = [...beforeNames].filter( + (name) => !afterNames.has(name), + ).length; + + let successText = 'Agent skills reloaded successfully.'; + const details: string[] = []; + + if (added.length > 0) { + details.push( + `${added.length} newly available skill${added.length > 1 ? 's' : ''}`, + ); + } + if (removedCount > 0) { + details.push( + `${removedCount} skill${removedCount > 1 ? 's' : ''} no longer available`, + ); + } + + if (details.length > 0) { + successText += ` ${details.join(' and ')}.`; + } + + context.ui.addItem( + { + type: 'info', + text: successText, + icon: '✓ ', + color: 'green', + } as HistoryItemInfo, + Date.now(), + ); + } catch (error) { + clearTimeout(pendingTimeout); + if (pendingItemSet) { + context.ui.setPendingItem(null); + } + context.ui.addItem( + { + type: MessageType.ERROR, + text: `Failed to reload skills: ${error instanceof Error ? error.message : String(error)}`, + }, + Date.now(), + ); + } +} + function disableCompletion( context: CommandContext, partialArg: string, @@ -185,7 +284,7 @@ function enableCompletion( export const skillsCommand: SlashCommand = { name: 'skills', description: - 'List, enable, or disable Gemini CLI agent skills. Usage: /skills [list | disable | enable ]', + 'List, enable, disable, or reload Gemini CLI agent skills. Usage: /skills [list | disable | enable | reload]', kind: CommandKind.BUILT_IN, autoExecute: false, subCommands: [ @@ -210,6 +309,13 @@ export const skillsCommand: SlashCommand = { action: enableAction, completion: enableCompletion, }, + { + name: 'reload', + description: + 'Reload the list of discovered skills. Usage: /skills reload', + kind: CommandKind.BUILT_IN, + action: reloadAction, + }, ], action: listAction, }; diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index c0f0eb0c2e..d9f74e59e0 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -138,6 +138,7 @@ export interface UIState { bannerVisible: boolean; customDialog: React.ReactNode | null; terminalBackgroundColor: TerminalBackgroundColor; + settingsNonce: number; } export const UIStateContext = createContext(null); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index ea357d690a..e16ef982fc 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -33,6 +33,8 @@ import { RipGrepTool, canUseRipgrep } from '../tools/ripGrep.js'; import { logRipgrepFallback } from '../telemetry/loggers.js'; import { RipgrepFallbackEvent } from '../telemetry/types.js'; import { ToolRegistry } from '../tools/tool-registry.js'; +import { ACTIVATE_SKILL_TOOL_NAME } from '../tools/tool-names.js'; +import type { SkillDefinition } from '../skills/skillLoader.js'; import { DEFAULT_MODEL_CONFIGS } from './defaultModelConfigs.js'; import { DEFAULT_GEMINI_MODEL, @@ -57,6 +59,7 @@ vi.mock('fs', async (importOriginal) => { vi.mock('../tools/tool-registry', () => { const ToolRegistryMock = vi.fn(); ToolRegistryMock.prototype.registerTool = vi.fn(); + ToolRegistryMock.prototype.unregisterTool = vi.fn(); ToolRegistryMock.prototype.discoverAllTools = vi.fn(); ToolRegistryMock.prototype.sortTools = vi.fn(); ToolRegistryMock.prototype.getAllTools = vi.fn(() => []); // Mock methods if needed @@ -104,6 +107,7 @@ vi.mock('../core/client.js', () => ({ GeminiClient: vi.fn().mockImplementation(() => ({ initialize: vi.fn().mockResolvedValue(undefined), stripThoughtsFromHistory: vi.fn(), + isInitialized: vi.fn().mockReturnValue(false), })), })); @@ -1978,4 +1982,104 @@ describe('Config JIT Initialization', () => { expect(ContextManager).not.toHaveBeenCalled(); expect(config.getUserMemory()).toBe('Initial Memory'); }); + + describe('reloadSkills', () => { + it('should refresh disabledSkills and re-register ActivateSkillTool when skills exist', async () => { + const mockOnReload = vi.fn().mockResolvedValue({ + disabledSkills: ['skill2'], + }); + const params: ConfigParameters = { + sessionId: 'test-session', + targetDir: '/tmp/test', + debugMode: false, + model: 'test-model', + cwd: '/tmp/test', + skillsSupport: true, + onReload: mockOnReload, + }; + + config = new Config(params); + await config.initialize(); + + const skillManager = config.getSkillManager(); + const toolRegistry = config.getToolRegistry(); + + vi.spyOn(skillManager, 'discoverSkills').mockResolvedValue(undefined); + vi.spyOn(skillManager, 'setDisabledSkills'); + vi.spyOn(toolRegistry, 'registerTool'); + vi.spyOn(toolRegistry, 'unregisterTool'); + + const mockSkills = [{ name: 'skill1' }]; + vi.spyOn(skillManager, 'getSkills').mockReturnValue( + mockSkills as SkillDefinition[], + ); + + await config.reloadSkills(); + + expect(mockOnReload).toHaveBeenCalled(); + expect(skillManager.setDisabledSkills).toHaveBeenCalledWith(['skill2']); + expect(toolRegistry.registerTool).toHaveBeenCalled(); + expect(toolRegistry.unregisterTool).not.toHaveBeenCalledWith( + ACTIVATE_SKILL_TOOL_NAME, + ); + }); + + it('should unregister ActivateSkillTool when no skills exist after reload', async () => { + const params: ConfigParameters = { + sessionId: 'test-session', + targetDir: '/tmp/test', + debugMode: false, + model: 'test-model', + cwd: '/tmp/test', + skillsSupport: true, + }; + + config = new Config(params); + await config.initialize(); + + const skillManager = config.getSkillManager(); + const toolRegistry = config.getToolRegistry(); + + vi.spyOn(skillManager, 'discoverSkills').mockResolvedValue(undefined); + vi.spyOn(toolRegistry, 'registerTool'); + vi.spyOn(toolRegistry, 'unregisterTool'); + + vi.spyOn(skillManager, 'getSkills').mockReturnValue([]); + + await config.reloadSkills(); + + expect(toolRegistry.unregisterTool).toHaveBeenCalledWith( + ACTIVATE_SKILL_TOOL_NAME, + ); + }); + + it('should clear disabledSkills when onReload returns undefined for them', async () => { + const mockOnReload = vi.fn().mockResolvedValue({ + disabledSkills: undefined, + }); + const params: ConfigParameters = { + sessionId: 'test-session', + targetDir: '/tmp/test', + debugMode: false, + model: 'test-model', + cwd: '/tmp/test', + skillsSupport: true, + onReload: mockOnReload, + }; + + config = new Config(params); + // Initially set some disabled skills + // @ts-expect-error - accessing private + config.disabledSkills = ['skill1']; + await config.initialize(); + + const skillManager = config.getSkillManager(); + vi.spyOn(skillManager, 'discoverSkills').mockResolvedValue(undefined); + vi.spyOn(skillManager, 'setDisabledSkills'); + + await config.reloadSkills(); + + expect(skillManager.setDisabledSkills).toHaveBeenCalledWith([]); + }); + }); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 3d9aba2bb8..aceea0efef 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -356,6 +356,7 @@ export interface ConfigParameters { disabledSkills?: string[]; experimentalJitContext?: boolean; onModelChange?: (model: string) => void; + onReload?: () => Promise<{ disabledSkills?: string[] }>; } export class Config { @@ -479,10 +480,13 @@ export class Config { private experimentsPromise: Promise | undefined; private hookSystem?: HookSystem; private readonly onModelChange: ((model: string) => void) | undefined; + private readonly onReload: + | (() => Promise<{ disabledSkills?: string[] }>) + | undefined; private readonly enableAgents: boolean; private readonly skillsSupport: boolean; - private readonly disabledSkills: string[]; + private disabledSkills: string[]; private readonly experimentalJitContext: boolean; private contextManager?: ContextManager; @@ -643,6 +647,7 @@ export class Config { this.projectHooks = params.projectHooks; this.experiments = params.experiments; this.onModelChange = params.onModelChange; + this.onReload = params.onReload; if (params.contextFileName) { setGeminiMdFilename(params.contextFileName); @@ -1520,6 +1525,38 @@ export class Config { return this.skillsSupport; } + /** + * Reloads skills by re-discovering them from extensions and local directories. + */ + async reloadSkills(): Promise { + if (!this.skillsSupport) { + return; + } + + if (this.onReload) { + const refreshed = await this.onReload(); + this.disabledSkills = refreshed.disabledSkills ?? []; + } + + await this.getSkillManager().discoverSkills( + this.storage, + this.getExtensions(), + ); + this.getSkillManager().setDisabledSkills(this.disabledSkills); + + // Re-register ActivateSkillTool to update its schema with the newly discovered skills + if (this.getSkillManager().getSkills().length > 0) { + this.getToolRegistry().registerTool( + new ActivateSkillTool(this, this.messageBus), + ); + } else { + this.getToolRegistry().unregisterTool(ActivateSkillTool.Name); + } + + // Notify the client that system instructions might need updating + await this.updateSystemInstructionIfInitialized(); + } + isInteractive(): boolean { return this.interactive; } diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 18c30c5f76..6179e5a068 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -225,6 +225,15 @@ export class ToolRegistry { this.allKnownTools.set(tool.name, tool); } + /** + * Unregisters a tool definition by name. + * + * @param name - The name of the tool to unregister. + */ + unregisterTool(name: string): void { + this.allKnownTools.delete(name); + } + /** * Sorts tools as: * 1. Built in tools. diff --git a/packages/core/src/utils/events.ts b/packages/core/src/utils/events.ts index 402b59940c..aebc1901a2 100644 --- a/packages/core/src/utils/events.ts +++ b/packages/core/src/utils/events.ts @@ -74,6 +74,7 @@ export enum CoreEvent { Output = 'output', MemoryChanged = 'memory-changed', ExternalEditorClosed = 'external-editor-closed', + SettingsChanged = 'settings-changed', } export interface CoreEvents { @@ -83,6 +84,7 @@ export interface CoreEvents { [CoreEvent.Output]: [OutputPayload]; [CoreEvent.MemoryChanged]: [MemoryChangedPayload]; [CoreEvent.ExternalEditorClosed]: never[]; + [CoreEvent.SettingsChanged]: never[]; } type EventBacklogItem = { @@ -163,6 +165,13 @@ export class CoreEventEmitter extends EventEmitter { this.emit(CoreEvent.ModelChanged, payload); } + /** + * Notifies subscribers that settings have been modified. + */ + emitSettingsChanged(): void { + this.emit(CoreEvent.SettingsChanged); + } + /** * Flushes buffered messages. Call this immediately after primary UI listener * subscribes. From 384fb6a465bb576f67daa1ee459db973bd554ba8 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Mon, 5 Jan 2026 16:11:50 -0800 Subject: [PATCH 017/591] Add setting to support OSC 52 paste (#15336) --- docs/get-started/configuration.md | 5 + packages/cli/src/config/settingsSchema.ts | 10 ++ .../src/ui/components/InputPrompt.test.tsx | 32 +++++- .../cli/src/ui/components/InputPrompt.tsx | 19 +++- .../src/ui/contexts/KeypressContext.test.tsx | 105 ++++++++++++++++++ .../cli/src/ui/contexts/KeypressContext.tsx | 48 +++++++- schemas/settings.schema.json | 7 ++ 7 files changed, 216 insertions(+), 10 deletions(-) diff --git a/docs/get-started/configuration.md b/docs/get-started/configuration.md index 246617854c..4ac0ac0764 100644 --- a/docs/get-started/configuration.md +++ b/docs/get-started/configuration.md @@ -848,6 +848,11 @@ their corresponding top-level category object in your `settings.json` file. - **Default:** `"auto"` - **Requires restart:** Yes +- **`experimental.useOSC52Paste`** (boolean): + - **Description:** Use OSC 52 sequence for pasting instead of clipboardy + (useful for remote sessions). + - **Default:** `false` + - **`experimental.introspectionAgentSettings.enabled`** (boolean): - **Description:** Enable the Introspection Agent. - **Default:** `false` diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index ff9ce70b78..a2d7b2a008 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -1443,6 +1443,16 @@ const SETTINGS_SCHEMA = { }, }, }, + useOSC52Paste: { + type: 'boolean', + label: 'Use OSC 52 Paste', + category: 'Experimental', + requiresRestart: false, + default: false, + description: + 'Use OSC 52 sequence for pasting instead of clipboardy (useful for remote sessions).', + showInDialog: true, + }, introspectionAgentSettings: { type: 'object', label: 'Introspection Agent Settings', diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index 0c273cac86..7717b1e2f9 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -4,7 +4,10 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { renderWithProviders } from '../../test-utils/render.js'; +import { + renderWithProviders, + createMockSettings, +} from '../../test-utils/render.js'; import { waitFor } from '../../test-utils/async.js'; import { act } from 'react'; import type { InputPromptProps } from './InputPrompt.js'; @@ -598,7 +601,7 @@ describe('InputPrompt', () => { }); await waitFor(() => { expect(debugLoggerErrorSpy).toHaveBeenCalledWith( - 'Error handling clipboard image:', + 'Error handling paste:', expect.any(Error), ); }); @@ -633,6 +636,31 @@ describe('InputPrompt', () => { }); unmount(); }); + + it('should use OSC 52 when useOSC52Paste setting is enabled', async () => { + vi.mocked(clipboardUtils.clipboardHasImage).mockResolvedValue(false); + const settings = createMockSettings({ + experimental: { useOSC52Paste: true }, + }); + + const { stdout, stdin, unmount } = renderWithProviders( + , + { settings }, + ); + + const writeSpy = vi.spyOn(stdout, 'write'); + + await act(async () => { + stdin.write('\x16'); // Ctrl+V + }); + + await waitFor(() => { + expect(writeSpy).toHaveBeenCalledWith('\x1b]52;c;?\x07'); + }); + // Should NOT call clipboardy.read() + expect(clipboardy.read).not.toHaveBeenCalled(); + unmount(); + }); }); it.each([ diff --git a/packages/cli/src/ui/components/InputPrompt.tsx b/packages/cli/src/ui/components/InputPrompt.tsx index 8aac3d0fd4..78031a8a9e 100644 --- a/packages/cli/src/ui/components/InputPrompt.tsx +++ b/packages/cli/src/ui/components/InputPrompt.tsx @@ -7,7 +7,7 @@ import type React from 'react'; import clipboardy from 'clipboardy'; import { useCallback, useEffect, useState, useRef } from 'react'; -import { Box, Text, type DOMElement } from 'ink'; +import { Box, Text, useStdout, type DOMElement } from 'ink'; import { SuggestionsDisplay, MAX_WIDTH } from './SuggestionsDisplay.js'; import { theme } from '../semantic-colors.js'; import { useInputHistory } from '../hooks/useInputHistory.js'; @@ -43,6 +43,7 @@ import * as path from 'node:path'; import { SCREEN_READER_USER_PREFIX } from '../textConstants.js'; import { useShellFocusState } from '../contexts/ShellFocusContext.js'; import { useUIState } from '../contexts/UIStateContext.js'; +import { useSettings } from '../contexts/SettingsContext.js'; import { StreamingState } from '../types.js'; import { useMouseClick } from '../hooks/useMouseClick.js'; import { useMouse, type MouseEvent } from '../contexts/MouseContext.js'; @@ -129,6 +130,8 @@ export const InputPrompt: React.FC = ({ suggestionsPosition = 'below', setBannerVisible, }) => { + const { stdout } = useStdout(); + const { merged: settings } = useSettings(); const kittyProtocol = useKittyKeyboardProtocol(); const isShellFocused = useShellFocusState(); const { setEmbeddedShellFocused } = useUIActions(); @@ -350,13 +353,17 @@ export const InputPrompt: React.FC = ({ } } - const textToInsert = await clipboardy.read(); - const offset = buffer.getOffset(); - buffer.replaceRangeByOffset(offset, offset, textToInsert); + if (settings.experimental?.useOSC52Paste) { + stdout.write('\x1b]52;c;?\x07'); + } else { + const textToInsert = await clipboardy.read(); + const offset = buffer.getOffset(); + buffer.replaceRangeByOffset(offset, offset, textToInsert); + } } catch (error) { - debugLogger.error('Error handling clipboard image:', error); + debugLogger.error('Error handling paste:', error); } - }, [buffer, config]); + }, [buffer, config, stdout, settings]); useMouseClick( innerBoxRef, diff --git a/packages/cli/src/ui/contexts/KeypressContext.test.tsx b/packages/cli/src/ui/contexts/KeypressContext.test.tsx index c78ccf11a5..c4ce92ee52 100644 --- a/packages/cli/src/ui/contexts/KeypressContext.test.tsx +++ b/packages/cli/src/ui/contexts/KeypressContext.test.tsx @@ -320,6 +320,111 @@ describe('KeypressContext', () => { }), ); }); + + it('should parse valid OSC 52 response', async () => { + const keyHandler = vi.fn(); + const { result } = renderHook(() => useKeypressContext(), { wrapper }); + + act(() => result.current.subscribe(keyHandler)); + + const base64Data = Buffer.from('Hello OSC 52').toString('base64'); + const sequence = `\x1b]52;c;${base64Data}\x07`; + + act(() => stdin.write(sequence)); + + await waitFor(() => { + expect(keyHandler).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'paste', + paste: true, + sequence: 'Hello OSC 52', + }), + ); + }); + }); + + it('should handle split OSC 52 response', async () => { + const keyHandler = vi.fn(); + const { result } = renderHook(() => useKeypressContext(), { wrapper }); + + act(() => result.current.subscribe(keyHandler)); + + const base64Data = Buffer.from('Split Paste').toString('base64'); + const sequence = `\x1b]52;c;${base64Data}\x07`; + + // Split the sequence + const part1 = sequence.slice(0, 5); + const part2 = sequence.slice(5); + + act(() => stdin.write(part1)); + act(() => stdin.write(part2)); + + await waitFor(() => { + expect(keyHandler).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'paste', + paste: true, + sequence: 'Split Paste', + }), + ); + }); + }); + + it('should handle OSC 52 response terminated by ESC \\', async () => { + const keyHandler = vi.fn(); + const { result } = renderHook(() => useKeypressContext(), { wrapper }); + + act(() => result.current.subscribe(keyHandler)); + + const base64Data = Buffer.from('Terminated by ST').toString('base64'); + const sequence = `\x1b]52;c;${base64Data}\x1b\\`; + + act(() => stdin.write(sequence)); + + await waitFor(() => { + expect(keyHandler).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'paste', + paste: true, + sequence: 'Terminated by ST', + }), + ); + }); + }); + + it('should ignore unknown OSC sequences', async () => { + const keyHandler = vi.fn(); + const { result } = renderHook(() => useKeypressContext(), { wrapper }); + + act(() => result.current.subscribe(keyHandler)); + + const sequence = `\x1b]1337;File=name=Zm9vCg==\x07`; + + act(() => stdin.write(sequence)); + + await act(async () => { + vi.advanceTimersByTime(0); + }); + + expect(keyHandler).not.toHaveBeenCalled(); + }); + + it('should ignore invalid OSC 52 format', async () => { + const keyHandler = vi.fn(); + const { result } = renderHook(() => useKeypressContext(), { wrapper }); + + act(() => result.current.subscribe(keyHandler)); + + const sequence = `\x1b]52;x;notbase64\x07`; + + act(() => stdin.write(sequence)); + + await act(async () => { + vi.advanceTimersByTime(0); + }); + + expect(keyHandler).not.toHaveBeenCalled(); + }); }); describe('debug keystroke logging', () => { diff --git a/packages/cli/src/ui/contexts/KeypressContext.tsx b/packages/cli/src/ui/contexts/KeypressContext.tsx index 8bfe51694f..a5ff7e92a2 100644 --- a/packages/cli/src/ui/contexts/KeypressContext.tsx +++ b/packages/cli/src/ui/contexts/KeypressContext.tsx @@ -317,12 +317,56 @@ function* emitKeys( } } - if (escaped && (ch === 'O' || ch === '[')) { + if (escaped && (ch === 'O' || ch === '[' || ch === ']')) { // ANSI escape sequence code = ch; let modifier = 0; - if (ch === 'O') { + if (ch === ']') { + // OSC sequence + // ESC ] ; BEL + // ESC ] ; ESC \ + let buffer = ''; + + // Read until BEL, `ESC \`, or timeout (empty string) + while (true) { + const next = yield; + if (next === '' || next === '\u0007') { + break; + } else if (next === ESC) { + const afterEsc = yield; + if (afterEsc === '' || afterEsc === '\\') { + break; + } + buffer += next + afterEsc; + continue; + } + buffer += next; + } + + // Check for OSC 52 (Clipboard) response + // Format: 52;c; or 52;p; + const match = /^52;[cp];(.*)$/.exec(buffer); + if (match) { + try { + const base64Data = match[1]; + const decoded = Buffer.from(base64Data, 'base64').toString('utf-8'); + keypressHandler({ + name: 'paste', + ctrl: false, + meta: false, + shift: false, + paste: true, + insertable: true, + sequence: decoded, + }); + } catch (_e) { + debugLogger.log('Failed to decode OSC 52 clipboard data'); + } + } + + continue; // resume main loop + } else if (ch === 'O') { // ESC O letter // ESC O modifier letter ch = yield; diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index 4e556269dd..4900fa25d6 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -1418,6 +1418,13 @@ }, "additionalProperties": false }, + "useOSC52Paste": { + "title": "Use OSC 52 Paste", + "description": "Use OSC 52 sequence for pasting instead of clipboardy (useful for remote sessions).", + "markdownDescription": "Use OSC 52 sequence for pasting instead of clipboardy (useful for remote sessions).\n\n- Category: `Experimental`\n- Requires restart: `no`\n- Default: `false`", + "default": false, + "type": "boolean" + }, "introspectionAgentSettings": { "title": "Introspection Agent Settings", "description": "Configuration for Introspection Agent.", From cbd2eee9c46749a3df138ffd2004cf139c76e91b Mon Sep 17 00:00:00 2001 From: Sehoon Shon Date: Mon, 5 Jan 2026 23:07:44 -0500 Subject: [PATCH 018/591] remove manual string when displaying manual model in the footer (#15967) --- .../__snapshots__/Footer.test.tsx.snap | 4 +- packages/core/src/config/models.test.ts | 38 +++++++++++++++++++ packages/core/src/config/models.ts | 16 ++++---- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/ui/components/__snapshots__/Footer.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/Footer.test.tsx.snap index be6f152e9f..495446eb0a 100644 --- a/packages/cli/src/ui/components/__snapshots__/Footer.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/Footer.test.tsx.snap @@ -1,8 +1,8 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html -exports[`