feat: Add flow to allow modifying edits during edit tool call (#808)

This commit is contained in:
Leo 2025-06-08 18:56:58 +01:00 committed by GitHub
parent 584286cfd9
commit 9efca40dae
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 520 additions and 6 deletions

View File

@ -172,6 +172,7 @@ export async function loadCliConfig(
fileFilteringRespectGitIgnore: settings.fileFiltering?.respectGitIgnore,
fileFilteringAllowBuildArtifacts:
settings.fileFiltering?.allowBuildArtifacts,
enableModifyWithExternalEditors: settings.enableModifyWithExternalEditors,
});
}

View File

@ -37,6 +37,7 @@ export interface Settings {
contextFileName?: string;
accessibility?: AccessibilitySettings;
telemetry?: boolean;
enableModifyWithExternalEditors?: boolean;
// Git-aware file filtering settings
fileFiltering?: {

View File

@ -355,6 +355,7 @@ export const App = ({ config, settings, startupWarnings = [] }: AppProps) => {
key={h.id}
item={h}
isPending={false}
config={config}
/>
)),
]}
@ -370,6 +371,7 @@ export const App = ({ config, settings, startupWarnings = [] }: AppProps) => {
// HistoryItemDisplay. Refactor later. Use a fake id for now.
item={{ ...item, id: 0 }}
isPending={true}
config={config}
/>
))}
</Box>

View File

@ -15,17 +15,20 @@ import { ToolGroupMessage } from './messages/ToolGroupMessage.js';
import { GeminiMessageContent } from './messages/GeminiMessageContent.js';
import { Box } from 'ink';
import { AboutBox } from './AboutBox.js';
import { Config } from '@gemini-cli/core';
interface HistoryItemDisplayProps {
item: HistoryItem;
availableTerminalHeight: number;
isPending: boolean;
config?: Config;
}
export const HistoryItemDisplay: React.FC<HistoryItemDisplayProps> = ({
item,
availableTerminalHeight,
isPending,
config,
}) => (
<Box flexDirection="column" key={item.id}>
{/* Render standard message types */}
@ -60,6 +63,7 @@ export const HistoryItemDisplay: React.FC<HistoryItemDisplayProps> = ({
toolCalls={item.tools}
groupId={item.id}
availableTerminalHeight={availableTerminalHeight}
config={config}
/>
)}
</Box>

View File

@ -13,6 +13,8 @@ import {
ToolConfirmationOutcome,
ToolExecuteConfirmationDetails,
ToolMcpConfirmationDetails,
checkHasEditor,
Config,
} from '@gemini-cli/core';
import {
RadioButtonSelect,
@ -21,11 +23,12 @@ import {
export interface ToolConfirmationMessageProps {
confirmationDetails: ToolCallConfirmationDetails;
config?: Config;
}
export const ToolConfirmationMessage: React.FC<
ToolConfirmationMessageProps
> = ({ confirmationDetails }) => {
> = ({ confirmationDetails, config }) => {
const { onConfirm } = confirmationDetails;
useInput((_, key) => {
@ -44,6 +47,24 @@ export const ToolConfirmationMessage: React.FC<
>();
if (confirmationDetails.type === 'edit') {
if (confirmationDetails.isModifying) {
return (
<Box
minWidth="90%"
borderStyle="round"
borderColor={Colors.Gray}
justifyContent="space-around"
padding={1}
overflow="hidden"
>
<Text>Modify in progress: </Text>
<Text color={Colors.AccentGreen}>
Save and close external editor to continue
</Text>
</Box>
);
}
// Body content is now the DiffRenderer, passing filename to it
// The bordered box is removed from here and handled within DiffRenderer
bodyContent = (
@ -63,8 +84,28 @@ export const ToolConfirmationMessage: React.FC<
label: 'Yes, allow always',
value: ToolConfirmationOutcome.ProceedAlways,
},
{ label: 'No (esc)', value: ToolConfirmationOutcome.Cancel },
);
// Conditionally add editor options if editors are installed
const notUsingSandbox = !process.env.SANDBOX;
const externalEditorsEnabled =
config?.getEnableModifyWithExternalEditors() ?? false;
if (checkHasEditor('vscode') && notUsingSandbox && externalEditorsEnabled) {
options.push({
label: 'Modify with VS Code',
value: ToolConfirmationOutcome.ModifyVSCode,
});
}
if (checkHasEditor('vim') && externalEditorsEnabled) {
options.push({
label: 'Modify with vim',
value: ToolConfirmationOutcome.ModifyVim,
});
}
options.push({ label: 'No (esc)', value: ToolConfirmationOutcome.Cancel });
} else if (confirmationDetails.type === 'exec') {
const executionProps =
confirmationDetails as ToolExecuteConfirmationDetails;

View File

@ -10,17 +10,20 @@ import { IndividualToolCallDisplay, ToolCallStatus } from '../../types.js';
import { ToolMessage } from './ToolMessage.js';
import { ToolConfirmationMessage } from './ToolConfirmationMessage.js';
import { Colors } from '../../colors.js';
import { Config } from '@gemini-cli/core';
interface ToolGroupMessageProps {
groupId: number;
toolCalls: IndividualToolCallDisplay[];
availableTerminalHeight: number;
config?: Config;
}
// Main component renders the border and maps the tools using ToolMessage
export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
toolCalls,
availableTerminalHeight,
config,
}) => {
const hasPending = !toolCalls.every(
(t) => t.status === ToolCallStatus.Success,
@ -80,6 +83,7 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
tool.confirmationDetails && (
<ToolConfirmationMessage
confirmationDetails={tool.confirmationDetails}
config={config}
/>
)}
</Box>

View File

@ -1440,7 +1440,7 @@ export interface TextBuffer {
key: Record<string, boolean>,
) => boolean;
/**
* Opens the current buffer contents in the users preferred terminal text
* Opens the current buffer contents in the user's preferred terminal text
* editor ($VISUAL or $EDITOR, falling back to "vi"). The method blocks
* until the editor exits, then reloads the file and replaces the inmemory
* buffer with whatever the user saved.
@ -1451,7 +1451,7 @@ export interface TextBuffer {
*
* Note: We purposefully rely on the *synchronous* spawn API so that the
* calling process genuinely waits for the editor to close before
* continuing. This mirrors Gits behaviour and simplifies downstream
* continuing. This mirrors Git's behaviour and simplifies downstream
* controlflow (callers can simply `await` the Promise).
*/
openInExternalEditor: (opts?: { editor?: string }) => Promise<void>;

View File

@ -78,6 +78,7 @@ export interface ConfigParameters {
telemetryLogUserPromptsEnabled?: boolean;
fileFilteringRespectGitIgnore?: boolean;
fileFilteringAllowBuildArtifacts?: boolean;
enableModifyWithExternalEditors?: boolean;
}
export class Config {
@ -106,6 +107,7 @@ export class Config {
private readonly geminiIgnorePatterns: string[] = [];
private readonly fileFilteringRespectGitIgnore: boolean;
private readonly fileFilteringAllowBuildArtifacts: boolean;
private readonly enableModifyWithExternalEditors: boolean;
private fileDiscoveryService: FileDiscoveryService | null = null;
constructor(params: ConfigParameters) {
@ -135,6 +137,8 @@ export class Config {
params.fileFilteringRespectGitIgnore ?? true;
this.fileFilteringAllowBuildArtifacts =
params.fileFilteringAllowBuildArtifacts ?? false;
this.enableModifyWithExternalEditors =
params.enableModifyWithExternalEditors ?? false;
if (params.contextFileName) {
setGeminiMdFilename(params.contextFileName);
@ -266,6 +270,10 @@ export class Config {
return this.fileFilteringAllowBuildArtifacts;
}
getEnableModifyWithExternalEditors(): boolean {
return this.enableModifyWithExternalEditors;
}
async getFileService(): Promise<FileDiscoveryService> {
if (!this.fileDiscoveryService) {
this.fileDiscoveryService = new FileDiscoveryService(this.targetDir);

View File

@ -13,6 +13,8 @@ import {
ToolResult,
ToolRegistry,
ApprovalMode,
EditTool,
EditToolParams,
} from '../index.js';
import { Part, PartListUnion } from '@google/genai';
import { getResponseTextFromParts } from '../utils/generateContentResponseUtilities.js';
@ -358,6 +360,16 @@ export class CoreToolScheduler {
this.checkAndNotifyCompletion();
}
private setArgsInternal(targetCallId: string, args: unknown): void {
this.toolCalls = this.toolCalls.map((call) => {
if (call.request.callId !== targetCallId) return call;
return {
...call,
request: { ...call.request, args: args as Record<string, unknown> },
};
});
}
private isRunning(): boolean {
return this.toolCalls.some(
(call) =>
@ -471,6 +483,33 @@ export class CoreToolScheduler {
'cancelled',
'User did not allow tool call',
);
} else if (
outcome === ToolConfirmationOutcome.ModifyVSCode ||
outcome === ToolConfirmationOutcome.ModifyVim
) {
const waitingToolCall = toolCall as WaitingToolCall;
if (waitingToolCall?.confirmationDetails?.type === 'edit') {
const editTool = waitingToolCall.tool as EditTool;
this.setStatusInternal(callId, 'awaiting_approval', {
...waitingToolCall.confirmationDetails,
isModifying: true,
});
const modifyResults = await editTool.onModify(
waitingToolCall.request.args as unknown as EditToolParams,
this.abortController.signal,
outcome,
);
if (modifyResults) {
this.setArgsInternal(callId, modifyResults.updatedParams);
this.setStatusInternal(callId, 'awaiting_approval', {
...waitingToolCall.confirmationDetails,
fileDiff: modifyResults.updatedDiff,
isModifying: false,
});
}
}
} else {
this.setStatusInternal(callId, 'scheduled');
}

View File

@ -25,6 +25,7 @@ export * from './utils/errors.js';
export * from './utils/getFolderStructure.js';
export * from './utils/memoryDiscovery.js';
export * from './utils/gitIgnoreParser.js';
export * from './utils/editor.js';
// Export services
export * from './services/fileDiscoveryService.js';

View File

@ -8,6 +8,7 @@
const mockEnsureCorrectEdit = vi.hoisted(() => vi.fn());
const mockGenerateJson = vi.hoisted(() => vi.fn());
const mockOpenDiff = vi.hoisted(() => vi.fn());
vi.mock('../utils/editCorrector.js', () => ({
ensureCorrectEdit: mockEnsureCorrectEdit,
@ -19,6 +20,10 @@ vi.mock('../core/client.js', () => ({
})),
}));
vi.mock('../utils/editor.js', () => ({
openDiff: mockOpenDiff,
}));
import { describe, it, expect, beforeEach, afterEach, vi, Mock } from 'vitest';
import { EditTool, EditToolParams } from './edit.js';
import { FileDiff } from './tools.js';
@ -27,6 +32,7 @@ import fs from 'fs';
import os from 'os';
import { ApprovalMode, Config } from '../config/config.js';
import { Content, Part, SchemaUnion } from '@google/genai';
import { ToolConfirmationOutcome } from './tools.js';
describe('EditTool', () => {
let tool: EditTool;
@ -715,4 +721,140 @@ function makeRequest() {
);
});
});
describe('onModify', () => {
const testFile = 'some_file.txt';
let filePath: string;
const diffDir = path.join(os.tmpdir(), 'gemini-cli-edit-tool-diffs');
beforeEach(() => {
filePath = path.join(rootDir, testFile);
mockOpenDiff.mockClear();
});
afterEach(() => {
fs.rmSync(diffDir, { recursive: true, force: true });
});
it('should create temporary files, call openDiff, and return updated params with diff', async () => {
const originalContent = 'original content';
const params: EditToolParams = {
file_path: filePath,
edits: [
{ old_string: originalContent, new_string: 'modified content' },
],
};
fs.writeFileSync(filePath, originalContent, 'utf8');
const result = await tool.onModify(
params,
new AbortController().signal,
ToolConfirmationOutcome.ModifyVSCode,
);
expect(mockOpenDiff).toHaveBeenCalledTimes(1);
const [oldPath, newPath] = mockOpenDiff.mock.calls[0];
expect(oldPath).toMatch(
/gemini-cli-edit-tool-diffs[/\\]gemini-cli-edit-some_file\.txt-old-\d+/,
);
expect(newPath).toMatch(
/gemini-cli-edit-tool-diffs[/\\]gemini-cli-edit-some_file\.txt-new-\d+/,
);
expect(result).toBeDefined();
expect(result!.updatedParams).toEqual({
file_path: filePath,
edits: [
{ old_string: originalContent, new_string: 'modified content' },
],
});
expect(result!.updatedDiff).toEqual(`Index: some_file.txt
===================================================================
--- some_file.txt\tCurrent
+++ some_file.txt\tProposed
@@ -1,1 +1,1 @@
-original content
\\ No newline at end of file
+modified content
\\ No newline at end of file
`);
// Verify temp files are cleaned up
expect(fs.existsSync(oldPath)).toBe(false);
expect(fs.existsSync(newPath)).toBe(false);
});
it('should handle non-existent files and return updated params', async () => {
const params: EditToolParams = {
file_path: filePath,
edits: [{ old_string: '', new_string: 'new file content' }],
};
const result = await tool.onModify(
params,
new AbortController().signal,
ToolConfirmationOutcome.ModifyVSCode,
);
expect(mockOpenDiff).toHaveBeenCalledTimes(1);
const [oldPath, newPath] = mockOpenDiff.mock.calls[0];
expect(result).toBeDefined();
expect(result!.updatedParams).toEqual({
file_path: filePath,
edits: [{ old_string: '', new_string: 'new file content' }],
});
expect(result!.updatedDiff).toContain('new file content');
// Verify temp files are cleaned up
expect(fs.existsSync(oldPath)).toBe(false);
expect(fs.existsSync(newPath)).toBe(false);
});
it('should clean up previous temp files before creating new ones', async () => {
const params: EditToolParams = {
file_path: filePath,
edits: [{ old_string: 'old', new_string: 'new' }],
};
fs.writeFileSync(filePath, 'some old content', 'utf8');
// Call onModify first time
const result1 = await tool.onModify(
params,
new AbortController().signal,
ToolConfirmationOutcome.ModifyVSCode,
);
const firstCall = mockOpenDiff.mock.calls[0];
const firstOldPath = firstCall[0];
const firstNewPath = firstCall[1];
expect(result1).toBeDefined();
expect(fs.existsSync(firstOldPath)).toBe(false);
expect(fs.existsSync(firstNewPath)).toBe(false);
// Ensure different timestamps so that the file names are different for testing.
await new Promise((resolve) => setTimeout(resolve, 2));
const result2 = await tool.onModify(
params,
new AbortController().signal,
ToolConfirmationOutcome.ModifyVSCode,
);
const secondCall = mockOpenDiff.mock.calls[1];
const secondOldPath = secondCall[0];
const secondNewPath = secondCall[1];
// Call onModify second time
expect(result2).toBeDefined();
expect(fs.existsSync(secondOldPath)).toBe(false);
expect(fs.existsSync(secondNewPath)).toBe(false);
// Verify different file names were used
expect(firstOldPath).not.toBe(secondOldPath);
expect(firstNewPath).not.toBe(secondNewPath);
});
});
});

View File

@ -4,8 +4,9 @@
* SPDX-License-Identifier: Apache-2.0
*/
import fs from 'fs';
import path from 'path';
import * as fs from 'fs';
import * as path from 'path';
import * as os from 'os';
import * as Diff from 'diff';
import {
BaseTool,
@ -22,6 +23,7 @@ import { GeminiClient } from '../core/client.js';
import { Config, ApprovalMode } from '../config/config.js';
import { ensureCorrectEdit } from '../utils/editCorrector.js';
import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js';
import { openDiff } from '../utils/editor.js';
import { ReadFileTool } from './read-file.js';
/**
@ -75,6 +77,8 @@ export class EditTool extends BaseTool<EditToolParams, EditResult> {
private readonly config: Config;
private readonly rootDirectory: string;
private readonly client: GeminiClient;
private tempOldDiffPath?: string;
private tempNewDiffPath?: string;
/**
* Creates a new instance of the EditLogic
@ -514,6 +518,142 @@ Expectation for required parameters:
}
}
/**
* Creates temp files for the current and proposed file contents and opens a diff tool.
* When the diff tool is closed, the tool will check if the file has been modified and provide the updated params.
* @returns Updated params and diff if the file has been modified, undefined otherwise.
*/
async onModify(
params: EditToolParams,
_abortSignal: AbortSignal,
outcome: ToolConfirmationOutcome,
): Promise<
{ updatedParams: EditToolParams; updatedDiff: string } | undefined
> {
const { oldPath, newPath } = this.createTempFiles(params);
this.tempOldDiffPath = oldPath;
this.tempNewDiffPath = newPath;
await openDiff(
this.tempOldDiffPath,
this.tempNewDiffPath,
outcome === ToolConfirmationOutcome.ModifyVSCode ? 'vscode' : 'vim',
);
return await this.getUpdatedParamsIfModified(params, _abortSignal);
}
private async getUpdatedParamsIfModified(
params: EditToolParams,
_abortSignal: AbortSignal,
): Promise<
{ updatedParams: EditToolParams; updatedDiff: string } | undefined
> {
if (!this.tempOldDiffPath || !this.tempNewDiffPath) return undefined;
let oldContent = '';
let newContent = '';
try {
oldContent = fs.readFileSync(this.tempOldDiffPath, 'utf8');
} catch (err) {
if (!isNodeError(err) || err.code !== 'ENOENT') throw err;
oldContent = '';
}
try {
newContent = fs.readFileSync(this.tempNewDiffPath, 'utf8');
} catch (err) {
if (!isNodeError(err) || err.code !== 'ENOENT') throw err;
newContent = '';
}
// Combine the edits into a single edit
const updatedParams: EditToolParams = {
...params,
edits: [
{
old_string: oldContent,
new_string: newContent,
},
],
};
const updatedDiff = Diff.createPatch(
path.basename(params.file_path),
oldContent,
newContent,
'Current',
'Proposed',
DEFAULT_DIFF_OPTIONS,
);
this.deleteTempFiles();
return { updatedParams, updatedDiff };
}
private createTempFiles(params: EditToolParams): Record<string, string> {
this.deleteTempFiles();
const tempDir = os.tmpdir();
const diffDir = path.join(tempDir, 'gemini-cli-edit-tool-diffs');
if (!fs.existsSync(diffDir)) {
fs.mkdirSync(diffDir, { recursive: true });
}
const fileName = path.basename(params.file_path);
const timestamp = Date.now();
const tempOldPath = path.join(
diffDir,
`gemini-cli-edit-${fileName}-old-${timestamp}`,
);
const tempNewPath = path.join(
diffDir,
`gemini-cli-edit-${fileName}-new-${timestamp}`,
);
let currentContent = '';
try {
currentContent = fs.readFileSync(params.file_path, 'utf8');
} catch (err) {
if (!isNodeError(err) || err.code !== 'ENOENT') throw err;
currentContent = '';
}
let proposedContent = currentContent;
for (const edit of params.edits) {
proposedContent = this._applyReplacement(
proposedContent,
edit.old_string,
edit.new_string,
edit.old_string === '' && currentContent === '',
);
}
fs.writeFileSync(tempOldPath, currentContent, 'utf8');
fs.writeFileSync(tempNewPath, proposedContent, 'utf8');
return {
oldPath: tempOldPath,
newPath: tempNewPath,
};
}
private deleteTempFiles(): void {
try {
if (this.tempOldDiffPath) {
fs.unlinkSync(this.tempOldDiffPath);
this.tempOldDiffPath = undefined;
}
} catch {
console.error(`Error deleting temp diff file: `, this.tempOldDiffPath);
}
try {
if (this.tempNewDiffPath) {
fs.unlinkSync(this.tempNewDiffPath);
this.tempNewDiffPath = undefined;
}
} catch {
console.error(`Error deleting temp diff file: `, this.tempNewDiffPath);
}
}
/**
* Creates parent directories if they don't exist
*/

View File

@ -202,6 +202,7 @@ export interface ToolEditConfirmationDetails {
onConfirm: (outcome: ToolConfirmationOutcome) => Promise<void>;
fileName: string;
fileDiff: string;
isModifying?: boolean;
}
export interface ToolExecuteConfirmationDetails {
@ -231,5 +232,7 @@ export enum ToolConfirmationOutcome {
ProceedAlways = 'proceed_always',
ProceedAlwaysServer = 'proceed_always_server',
ProceedAlwaysTool = 'proceed_always_tool',
ModifyVSCode = 'modify_vscode',
ModifyVim = 'modify_vim',
Cancel = 'cancel',
}

View File

@ -0,0 +1,128 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { execSync, spawn } from 'child_process';
type EditorType = 'vscode' | 'vim';
interface DiffCommand {
command: string;
args: string[];
}
export function checkHasEditor(editor: EditorType): boolean {
const commandExists = (cmd: string): boolean => {
try {
execSync(`which ${cmd}`, { stdio: 'ignore' });
return true;
} catch {
return false;
}
};
if (editor === 'vscode') {
return commandExists('code');
} else if (editor === 'vim') {
return commandExists('vim');
}
return false;
}
/**
* Get the diff command for a specific editor.
*/
export function getDiffCommand(
oldPath: string,
newPath: string,
editor: EditorType,
): DiffCommand | null {
switch (editor) {
case 'vscode':
return {
command: 'code',
args: ['--wait', '--diff', oldPath, newPath],
};
case 'vim':
return {
command: 'vim',
args: [
'-d',
// skip viminfo file to avoid E138 errors
'-i',
'NONE',
// make the left window read-only and the right window editable
'-c',
'wincmd h | set readonly | wincmd l',
// set up colors for diffs
'-c',
'highlight DiffAdd cterm=bold ctermbg=22 guibg=#005f00 | highlight DiffChange cterm=bold ctermbg=24 guibg=#005f87 | highlight DiffText ctermbg=21 guibg=#0000af | highlight DiffDelete ctermbg=52 guibg=#5f0000',
// Show helpful messages
'-c',
'set showtabline=2 | set tabline=[Instructions]\\ :wqa(save\\ &\\ quit)\\ \\|\\ i/esc(toggle\\ edit\\ mode)',
'-c',
'wincmd h | setlocal statusline=OLD\\ FILE',
'-c',
'wincmd l | setlocal statusline=%#StatusBold#NEW\\ FILE\\ :wqa(save\\ &\\ quit)\\ \\|\\ i/esc(toggle\\ edit\\ mode)',
// Auto close all windows when one is closed
'-c',
'autocmd WinClosed * wqa',
oldPath,
newPath,
],
};
default:
return null;
}
}
/**
* Opens a diff tool to compare two files.
* Terminal-based editors by default blocks parent process until the editor exits.
* GUI-based editors requires args such as "--wait" to block parent process.
*/
export async function openDiff(
oldPath: string,
newPath: string,
editor: EditorType,
): Promise<void> {
const diffCommand = getDiffCommand(oldPath, newPath, editor);
if (!diffCommand) {
console.error('No diff tool available. Install vim or vscode.');
return;
}
try {
if (editor === 'vscode') {
// Use spawn to avoid blocking the entire process, resolve this function when editor is closed.
return new Promise((resolve, reject) => {
const process = spawn(diffCommand.command, diffCommand.args, {
stdio: 'inherit',
});
process.on('close', (code) => {
if (code === 0) {
resolve();
} else {
reject(new Error(`VS Code exited with code ${code}`));
}
});
process.on('error', (error) => {
reject(error);
});
});
} else {
// Use execSync for terminal-based editors like vim
const command = `${diffCommand.command} ${diffCommand.args.map((arg) => `"${arg}"`).join(' ')}`;
execSync(command, {
stdio: 'inherit',
encoding: 'utf8',
});
}
} catch (error) {
console.error(error);
}
}