mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-10 22:21:22 -07:00
refactor: consolidate EditTool and SmartEditTool (#15857)
This commit is contained in:
Abhi
@@ -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';
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
@@ -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) =>
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -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<EditToolParams, ToolResult>
|
||||
implements ToolInvocation<EditToolParams, ToolResult>
|
||||
{
|
||||
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<CalculatedEdit> {
|
||||
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<ToolCallConfirmationDetails | false> {
|
||||
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<ToolResult> {
|
||||
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<EditToolParams, ToolResult>
|
||||
implements ModifiableDeclarativeTool<EditToolParams>
|
||||
{
|
||||
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<EditToolParams, ToolResult> {
|
||||
return new EditToolInvocation(
|
||||
this.config,
|
||||
params,
|
||||
messageBus,
|
||||
toolName ?? this.name,
|
||||
displayName ?? this.displayName,
|
||||
);
|
||||
}
|
||||
|
||||
getModifyContext(_: AbortSignal): ModifyContext<EditToolParams> {
|
||||
const resolvePath = (filePath: string) =>
|
||||
path.resolve(this.config.getTargetDir(), filePath);
|
||||
|
||||
return {
|
||||
getFilePath: (params: EditToolParams) => params.file_path,
|
||||
getCurrentContent: async (params: EditToolParams): Promise<string> => {
|
||||
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<string> => {
|
||||
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,
|
||||
}),
|
||||
};
|
||||
}
|
||||
}
|
||||
@@ -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);
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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';
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user