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] 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,