diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 3bfa023e..3e0dba61 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -27,7 +27,7 @@ vi.mock('../utils/editor.js', () => ({ })); import { describe, it, expect, beforeEach, afterEach, vi, Mock } from 'vitest'; -import { EditTool, EditToolParams } from './edit.js'; +import { applyReplacement, EditTool, EditToolParams } from './edit.js'; import { FileDiff, ToolConfirmationOutcome } from './tools.js'; import { ToolErrorType } from './tool-error.js'; import path from 'path'; @@ -155,45 +155,30 @@ describe('EditTool', () => { fs.rmSync(tempDir, { recursive: true, force: true }); }); - describe('_applyReplacement', () => { - // Access private method for testing - // Note: `tool` is initialized in `beforeEach` of the parent describe block + describe('applyReplacement', () => { it('should return newString if isNewFile is true', () => { - expect((tool as any)._applyReplacement(null, 'old', 'new', true)).toBe( - 'new', - ); - expect( - (tool as any)._applyReplacement('existing', 'old', 'new', true), - ).toBe('new'); + 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((tool as any)._applyReplacement(null, '', 'new', false)).toBe( - 'new', - ); + expect(applyReplacement(null, '', 'new', false)).toBe('new'); }); it('should return empty string if currentContent is null and oldString is not empty (defensive)', () => { - expect((tool as any)._applyReplacement(null, 'old', 'new', false)).toBe( - '', - ); + expect(applyReplacement(null, 'old', 'new', false)).toBe(''); }); it('should replace oldString with newString in currentContent', () => { - expect( - (tool as any)._applyReplacement( - 'hello old world old', - 'old', - 'new', - false, - ), - ).toBe('hello new world new'); + 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( - (tool as any)._applyReplacement('hello world', '', 'new', false), - ).toBe('hello world'); + expect(applyReplacement('hello world', '', 'new', false)).toBe( + 'hello world', + ); }); }); @@ -239,15 +224,13 @@ describe('EditTool', () => { filePath = path.join(rootDir, testFile); }); - it('should return false if params are invalid', async () => { + it('should throw an error if params are invalid', async () => { const params: EditToolParams = { file_path: 'relative.txt', old_string: 'old', new_string: 'new', }; - expect( - await tool.shouldConfirmExecute(params, new AbortController().signal), - ).toBe(false); + expect(() => tool.build(params)).toThrow(); }); it('should request confirmation for valid edit', async () => { @@ -259,8 +242,8 @@ describe('EditTool', () => { }; // ensureCorrectEdit will be called by shouldConfirmExecute mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 1 }); - const confirmation = await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); expect(confirmation).toEqual( @@ -280,9 +263,11 @@ describe('EditTool', () => { new_string: 'new', }; mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); - expect( - await tool.shouldConfirmExecute(params, new AbortController().signal), - ).toBe(false); + 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 () => { @@ -293,9 +278,11 @@ describe('EditTool', () => { new_string: 'new', }; mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 2 }); - expect( - await tool.shouldConfirmExecute(params, new AbortController().signal), - ).toBe(false); + 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 () => { @@ -310,8 +297,8 @@ describe('EditTool', () => { // 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 confirmation = await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); expect(confirmation).toEqual( @@ -358,9 +345,8 @@ describe('EditTool', () => { }; }, ); - - const confirmation = (await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = (await invocation.shouldConfirmExecute( new AbortController().signal, )) as FileDiff; @@ -408,15 +394,13 @@ describe('EditTool', () => { }); }); - it('should return error if params are invalid', async () => { + it('should throw error if params are invalid', async () => { const params: EditToolParams = { file_path: 'relative.txt', old_string: 'old', new_string: 'new', }; - const result = await tool.execute(params, new AbortController().signal); - expect(result.llmContent).toMatch(/Error: Invalid parameters provided/); - expect(result.returnDisplay).toMatch(/Error: File path must be absolute/); + expect(() => tool.build(params)).toThrow(/File path must be absolute/); }); it('should edit an existing file and return diff with fileName', async () => { @@ -433,12 +417,8 @@ describe('EditTool', () => { // ensureCorrectEdit is NOT called by calculateEdit, only by shouldConfirmExecute // So, the default mockEnsureCorrectEdit should correctly return 1 occurrence for 'old' in initialContent - // Simulate confirmation by setting shouldAlwaysEdit - (tool as any).shouldAlwaysEdit = true; - - const result = await tool.execute(params, new AbortController().signal); - - (tool as any).shouldAlwaysEdit = false; // Reset for other tests + 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); @@ -461,7 +441,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + 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); @@ -477,7 +458,8 @@ describe('EditTool', () => { new_string: 'replacement', }; // The default mockEnsureCorrectEdit will return 0 occurrences for 'nonexistent' - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch( /0 occurrences found for old_string in/, ); @@ -494,7 +476,8 @@ describe('EditTool', () => { new_string: 'new', }; // The default mockEnsureCorrectEdit will return 2 occurrences for 'old' - const result = await tool.execute(params, new AbortController().signal); + 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/, ); @@ -512,12 +495,8 @@ describe('EditTool', () => { expected_replacements: 3, }; - // Simulate confirmation by setting shouldAlwaysEdit - (tool as any).shouldAlwaysEdit = true; - - const result = await tool.execute(params, new AbortController().signal); - - (tool as any).shouldAlwaysEdit = false; // Reset for other tests + 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( @@ -537,7 +516,8 @@ describe('EditTool', () => { new_string: 'new', expected_replacements: 3, // Expecting 3 but only 2 exist }; - const result = await tool.execute(params, new AbortController().signal); + 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/, ); @@ -553,7 +533,8 @@ describe('EditTool', () => { old_string: '', new_string: 'new content', }; - const result = await tool.execute(params, new AbortController().signal); + 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/, @@ -573,7 +554,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch( /User modified the `new_string` content/, @@ -593,7 +575,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).not.toMatch( /User modified the `new_string` content/, @@ -612,7 +595,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).not.toMatch( /User modified the `new_string` content/, @@ -627,7 +611,8 @@ describe('EditTool', () => { old_string: 'identical', new_string: 'identical', }; - const result = await tool.execute(params, new AbortController().signal); + 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/); }); @@ -647,7 +632,8 @@ describe('EditTool', () => { old_string: 'any', new_string: 'new', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); }); @@ -658,7 +644,8 @@ describe('EditTool', () => { old_string: '', new_string: 'new content', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe( ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE, ); @@ -671,7 +658,8 @@ describe('EditTool', () => { old_string: 'not-found', new_string: 'new', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_OCCURRENCE_FOUND); }); @@ -683,7 +671,8 @@ describe('EditTool', () => { new_string: 'new', expected_replacements: 3, }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe( ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH, ); @@ -696,18 +685,18 @@ describe('EditTool', () => { old_string: 'content', new_string: 'content', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_CHANGE); }); - it('should return INVALID_PARAMETERS error for relative path', async () => { + it('should throw INVALID_PARAMETERS error for relative path', async () => { const params: EditToolParams = { file_path: 'relative/path.txt', old_string: 'a', new_string: 'b', }; - const result = await tool.execute(params, new AbortController().signal); - expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS); + expect(() => tool.build(params)).toThrow(); }); it('should return FILE_WRITE_FAILURE on write error', async () => { @@ -720,7 +709,8 @@ describe('EditTool', () => { old_string: 'content', new_string: 'new content', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); }); }); @@ -733,8 +723,9 @@ describe('EditTool', () => { old_string: 'identical_string', new_string: 'identical_string', }; + const invocation = tool.build(params); // shortenPath will be called internally, resulting in just the file name - expect(tool.getDescription(params)).toBe( + expect(invocation.getDescription()).toBe( `No file changes to ${testFileName}`, ); }); @@ -746,9 +737,10 @@ describe('EditTool', () => { old_string: 'this is the old string value', new_string: 'this is the new string value', }; + const invocation = tool.build(params); // shortenPath will be called internally, resulting in just the file name // The snippets are truncated at 30 chars + '...' - expect(tool.getDescription(params)).toBe( + expect(invocation.getDescription()).toBe( `${testFileName}: this is the old string value => this is the new string value`, ); }); @@ -760,7 +752,8 @@ describe('EditTool', () => { old_string: 'old', new_string: 'new', }; - expect(tool.getDescription(params)).toBe(`${testFileName}: old => new`); + const invocation = tool.build(params); + expect(invocation.getDescription()).toBe(`${testFileName}: old => new`); }); it('should truncate long strings in the description', () => { @@ -772,7 +765,8 @@ describe('EditTool', () => { new_string: 'this is a very long new string that will also be truncated', }; - expect(tool.getDescription(params)).toBe( + const invocation = tool.build(params); + expect(invocation.getDescription()).toBe( `${testFileName}: this is a very long old string... => this is a very long new string...`, ); }); @@ -839,8 +833,8 @@ describe('EditTool', () => { content: modifiedContent, }); - const confirmation = await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 43505182..f1d0498a 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -8,11 +8,12 @@ import * as fs from 'fs'; import * as path from 'path'; import * as Diff from 'diff'; import { - BaseTool, + BaseDeclarativeTool, Icon, ToolCallConfirmationDetails, ToolConfirmationOutcome, ToolEditConfirmationDetails, + ToolInvocation, ToolLocation, ToolResult, ToolResultDisplay, @@ -29,6 +30,26 @@ import { ReadFileTool } from './read-file.js'; import { ModifiableDeclarativeTool, ModifyContext } from './modifiable-tool.js'; import { IDEConnectionStatus } from '../ide/ide-client.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; + } + return currentContent.replaceAll(oldString, newString); +} + /** * Parameters for the Edit tool */ @@ -68,112 +89,14 @@ interface CalculatedEdit { isNewFile: boolean; } -/** - * Implementation of the Edit tool logic - */ -export class EditTool - extends BaseTool - implements ModifiableDeclarativeTool -{ - static readonly Name = 'replace'; +class EditToolInvocation implements ToolInvocation { + constructor( + private readonly config: Config, + public params: EditToolParams, + ) {} - constructor(private readonly config: Config) { - 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 ${ReadFileTool.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\` MUST be an absolute path; otherwise an error will be thrown. -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.`, - Icon.Pencil, - { - properties: { - file_path: { - description: - "The absolute path to the file to modify. Must start with '/'.", - type: 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: 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: Type.STRING, - }, - expected_replacements: { - type: 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: Type.OBJECT, - }, - ); - } - - /** - * Validates the parameters for the Edit tool - * @param params Parameters to validate - * @returns Error message string or null if valid - */ - validateToolParams(params: EditToolParams): string | null { - const errors = SchemaValidator.validate(this.schema.parameters, params); - if (errors) { - return errors; - } - - if (!path.isAbsolute(params.file_path)) { - return `File path must be absolute: ${params.file_path}`; - } - - const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(params.file_path)) { - const directories = workspaceContext.getDirectories(); - return `File path must be within one of the workspace directories: ${directories.join(', ')}`; - } - - return null; - } - - /** - * Determines any file locations affected by the tool execution - * @param params Parameters for the tool execution - * @returns A list of such paths - */ - toolLocations(params: EditToolParams): ToolLocation[] { - return [{ path: params.file_path }]; - } - - private _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; - } - return currentContent.replaceAll(oldString, newString); + toolLocations(): ToolLocation[] { + return [{ path: this.params.file_path }]; } /** @@ -271,7 +194,7 @@ Expectation for required parameters: }; } - const newContent = this._applyReplacement( + const newContent = applyReplacement( currentContent, finalOldString, finalNewString, @@ -292,23 +215,15 @@ Expectation for required parameters: * It needs to calculate the diff to show the user. */ async shouldConfirmExecute( - params: EditToolParams, abortSignal: AbortSignal, ): Promise { if (this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) { return false; } - const validationError = this.validateToolParams(params); - if (validationError) { - console.error( - `[EditTool Wrapper] Attempted confirmation with invalid parameters: ${validationError}`, - ); - return false; - } let editData: CalculatedEdit; try { - editData = await this.calculateEdit(params, abortSignal); + editData = await this.calculateEdit(this.params, abortSignal); } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error); console.log(`Error preparing edit: ${errorMsg}`); @@ -320,7 +235,7 @@ Expectation for required parameters: return false; } - const fileName = path.basename(params.file_path); + const fileName = path.basename(this.params.file_path); const fileDiff = Diff.createPatch( fileName, editData.currentContent ?? '', @@ -334,14 +249,14 @@ Expectation for required parameters: this.config.getIdeModeFeature() && this.config.getIdeMode() && ideClient?.getConnectionStatus().status === IDEConnectionStatus.Connected - ? ideClient.openDiff(params.file_path, editData.newContent) + ? ideClient.openDiff(this.params.file_path, editData.newContent) : undefined; const confirmationDetails: ToolEditConfirmationDetails = { type: 'edit', - title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`, + title: `Confirm Edit: ${shortenPath(makeRelative(this.params.file_path, this.config.getTargetDir()))}`, fileName, - filePath: params.file_path, + filePath: this.params.file_path, fileDiff, originalContent: editData.currentContent, newContent: editData.newContent, @@ -355,8 +270,8 @@ Expectation for required parameters: 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. - params.old_string = editData.currentContent ?? ''; - params.new_string = result.content; + this.params.old_string = editData.currentContent ?? ''; + this.params.new_string = result.content; } } }, @@ -365,26 +280,23 @@ Expectation for required parameters: return confirmationDetails; } - getDescription(params: EditToolParams): string { - if (!params.file_path || !params.old_string || !params.new_string) { - return `Model did not provide valid parameters for edit tool`; - } + getDescription(): string { const relativePath = makeRelative( - params.file_path, + this.params.file_path, this.config.getTargetDir(), ); - if (params.old_string === '') { + if (this.params.old_string === '') { return `Create ${shortenPath(relativePath)}`; } const oldStringSnippet = - params.old_string.split('\n')[0].substring(0, 30) + - (params.old_string.length > 30 ? '...' : ''); + this.params.old_string.split('\n')[0].substring(0, 30) + + (this.params.old_string.length > 30 ? '...' : ''); const newStringSnippet = - params.new_string.split('\n')[0].substring(0, 30) + - (params.new_string.length > 30 ? '...' : ''); + this.params.new_string.split('\n')[0].substring(0, 30) + + (this.params.new_string.length > 30 ? '...' : ''); - if (params.old_string === params.new_string) { + if (this.params.old_string === this.params.new_string) { return `No file changes to ${shortenPath(relativePath)}`; } return `${shortenPath(relativePath)}: ${oldStringSnippet} => ${newStringSnippet}`; @@ -395,25 +307,10 @@ Expectation for required parameters: * @param params Parameters for the edit operation * @returns Result of the edit operation */ - async execute( - params: EditToolParams, - signal: AbortSignal, - ): Promise { - const validationError = this.validateToolParams(params); - if (validationError) { - return { - llmContent: `Error: Invalid parameters provided. Reason: ${validationError}`, - returnDisplay: `Error: ${validationError}`, - error: { - message: validationError, - type: ToolErrorType.INVALID_TOOL_PARAMS, - }, - }; - } - + async execute(signal: AbortSignal): Promise { let editData: CalculatedEdit; try { - editData = await this.calculateEdit(params, signal); + editData = await this.calculateEdit(this.params, signal); } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error); return { @@ -438,16 +335,16 @@ Expectation for required parameters: } try { - this.ensureParentDirectoriesExist(params.file_path); - fs.writeFileSync(params.file_path, editData.newContent, 'utf8'); + this.ensureParentDirectoriesExist(this.params.file_path); + fs.writeFileSync(this.params.file_path, editData.newContent, 'utf8'); let displayResult: ToolResultDisplay; if (editData.isNewFile) { - displayResult = `Created ${shortenPath(makeRelative(params.file_path, this.config.getTargetDir()))}`; + displayResult = `Created ${shortenPath(makeRelative(this.params.file_path, this.config.getTargetDir()))}`; } else { // Generate diff for display, even though core logic doesn't technically need it // The CLI wrapper will use this part of the ToolResult - const fileName = path.basename(params.file_path); + const fileName = path.basename(this.params.file_path); const fileDiff = Diff.createPatch( fileName, editData.currentContent ?? '', // Should not be null here if not isNewFile @@ -466,12 +363,12 @@ Expectation for required parameters: const llmSuccessMessageParts = [ editData.isNewFile - ? `Created new file: ${params.file_path} with provided content.` - : `Successfully modified file: ${params.file_path} (${editData.occurrences} replacements).`, + ? `Created new file: ${this.params.file_path} with provided content.` + : `Successfully modified file: ${this.params.file_path} (${editData.occurrences} replacements).`, ]; - if (params.modified_by_user) { + if (this.params.modified_by_user) { llmSuccessMessageParts.push( - `User modified the \`new_string\` content to be: ${params.new_string}.`, + `User modified the \`new_string\` content to be: ${this.params.new_string}.`, ); } @@ -501,6 +398,91 @@ Expectation for required parameters: fs.mkdirSync(dirName, { recursive: true }); } } +} + +/** + * Implementation of the Edit tool logic + */ +export class EditTool + extends BaseDeclarativeTool + implements ModifiableDeclarativeTool +{ + static readonly Name = 'replace'; + constructor(private readonly config: Config) { + 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 ${ReadFileTool.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\` MUST be an absolute path; otherwise an error will be thrown. +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.`, + Icon.Pencil, + { + properties: { + file_path: { + description: + "The absolute path to the file to modify. Must start with '/'.", + type: 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: 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: Type.STRING, + }, + expected_replacements: { + type: 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: Type.OBJECT, + }, + ); + } + + /** + * Validates the parameters for the Edit tool + * @param params Parameters to validate + * @returns Error message string or null if valid + */ + validateToolParams(params: EditToolParams): string | null { + const errors = SchemaValidator.validate(this.schema.parameters, params); + if (errors) { + return errors; + } + + if (!path.isAbsolute(params.file_path)) { + return `File path must be absolute: ${params.file_path}`; + } + + const workspaceContext = this.config.getWorkspaceContext(); + if (!workspaceContext.isPathWithinWorkspace(params.file_path)) { + const directories = workspaceContext.getDirectories(); + return `File path must be within one of the workspace directories: ${directories.join(', ')}`; + } + + return null; + } + + protected createInvocation( + params: EditToolParams, + ): ToolInvocation { + return new EditToolInvocation(this.config, params); + } getModifyContext(_: AbortSignal): ModifyContext { return { @@ -516,7 +498,7 @@ Expectation for required parameters: getProposedContent: async (params: EditToolParams): Promise => { try { const currentContent = fs.readFileSync(params.file_path, 'utf8'); - return this._applyReplacement( + return applyReplacement( currentContent, params.old_string, params.new_string, diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 0ee6c0ee..934b7ce7 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -64,7 +64,8 @@ describe('GlobTool', () => { describe('execute', () => { it('should find files matching a simple pattern in the root', async () => { const params: GlobToolParams = { pattern: '*.txt' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 2 file(s)'); expect(result.llmContent).toContain(path.join(tempRootDir, 'fileA.txt')); expect(result.llmContent).toContain(path.join(tempRootDir, 'FileB.TXT')); @@ -73,7 +74,8 @@ describe('GlobTool', () => { it('should find files case-sensitively when case_sensitive is true', async () => { const params: GlobToolParams = { pattern: '*.txt', case_sensitive: true }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 1 file(s)'); expect(result.llmContent).toContain(path.join(tempRootDir, 'fileA.txt')); expect(result.llmContent).not.toContain( @@ -83,7 +85,8 @@ describe('GlobTool', () => { it('should find files case-insensitively by default (pattern: *.TXT)', async () => { const params: GlobToolParams = { pattern: '*.TXT' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 2 file(s)'); expect(result.llmContent).toContain(path.join(tempRootDir, 'fileA.txt')); expect(result.llmContent).toContain(path.join(tempRootDir, 'FileB.TXT')); @@ -94,7 +97,8 @@ describe('GlobTool', () => { pattern: '*.TXT', case_sensitive: false, }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 2 file(s)'); expect(result.llmContent).toContain(path.join(tempRootDir, 'fileA.txt')); expect(result.llmContent).toContain(path.join(tempRootDir, 'FileB.TXT')); @@ -102,7 +106,8 @@ describe('GlobTool', () => { it('should find files using a pattern that includes a subdirectory', async () => { const params: GlobToolParams = { pattern: 'sub/*.md' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 2 file(s)'); expect(result.llmContent).toContain( path.join(tempRootDir, 'sub', 'fileC.md'), @@ -114,7 +119,8 @@ describe('GlobTool', () => { it('should find files in a specified relative path (relative to rootDir)', async () => { const params: GlobToolParams = { pattern: '*.md', path: 'sub' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 2 file(s)'); expect(result.llmContent).toContain( path.join(tempRootDir, 'sub', 'fileC.md'), @@ -126,7 +132,8 @@ describe('GlobTool', () => { it('should find files using a deep globstar pattern (e.g., **/*.log)', async () => { const params: GlobToolParams = { pattern: '**/*.log' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 1 file(s)'); expect(result.llmContent).toContain( path.join(tempRootDir, 'sub', 'deep', 'fileE.log'), @@ -135,7 +142,8 @@ describe('GlobTool', () => { it('should return "No files found" message when pattern matches nothing', async () => { const params: GlobToolParams = { pattern: '*.nonexistent' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'No files found matching pattern "*.nonexistent"', ); @@ -144,7 +152,8 @@ describe('GlobTool', () => { it('should correctly sort files by modification time (newest first)', async () => { const params: GlobToolParams = { pattern: '*.sortme' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); const llmContent = partListUnionToString(result.llmContent); expect(llmContent).toContain('Found 2 file(s)'); @@ -242,8 +251,8 @@ describe('GlobTool', () => { // Let's try to go further up. const paramsOutside: GlobToolParams = { pattern: '*.txt', - path: '../../../../../../../../../../tmp', - }; // Definitely outside + path: '../../../../../../../../../../tmp', // Definitely outside + }; expect(specificGlobTool.validateToolParams(paramsOutside)).toContain( 'resolves outside the allowed workspace directories', ); @@ -290,7 +299,8 @@ describe('GlobTool', () => { it('should work with paths in workspace subdirectories', async () => { const params: GlobToolParams = { pattern: '*.md', path: 'sub' }; - const result = await globTool.execute(params, abortSignal); + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 2 file(s)'); expect(result.llmContent).toContain('fileC.md'); diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 5bcb9778..df0cc348 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -8,7 +8,13 @@ import fs from 'fs'; import path from 'path'; import { glob } from 'glob'; import { SchemaValidator } from '../utils/schemaValidator.js'; -import { BaseTool, Icon, ToolResult } from './tools.js'; +import { + BaseDeclarativeTool, + BaseToolInvocation, + Icon, + ToolInvocation, + ToolResult, +} from './tools.js'; import { Type } from '@google/genai'; import { shortenPath, makeRelative } from '../utils/paths.js'; import { Config } from '../config/config.js'; @@ -74,10 +80,168 @@ export interface GlobToolParams { respect_git_ignore?: boolean; } +class GlobToolInvocation extends BaseToolInvocation< + GlobToolParams, + ToolResult +> { + constructor( + private config: Config, + params: GlobToolParams, + ) { + super(params); + } + + getDescription(): string { + let description = `'${this.params.pattern}'`; + if (this.params.path) { + const searchDir = path.resolve( + this.config.getTargetDir(), + this.params.path || '.', + ); + const relativePath = makeRelative(searchDir, this.config.getTargetDir()); + description += ` within ${shortenPath(relativePath)}`; + } + return description; + } + + async execute(signal: AbortSignal): Promise { + try { + const workspaceContext = this.config.getWorkspaceContext(); + const workspaceDirectories = workspaceContext.getDirectories(); + + // If a specific path is provided, resolve it and check if it's within workspace + let searchDirectories: readonly string[]; + if (this.params.path) { + const searchDirAbsolute = path.resolve( + this.config.getTargetDir(), + this.params.path, + ); + if (!workspaceContext.isPathWithinWorkspace(searchDirAbsolute)) { + return { + llmContent: `Error: Path "${this.params.path}" is not within any workspace directory`, + returnDisplay: `Path is not within workspace`, + }; + } + searchDirectories = [searchDirAbsolute]; + } else { + // Search across all workspace directories + searchDirectories = workspaceDirectories; + } + + // Get centralized file discovery service + const respectGitIgnore = + this.params.respect_git_ignore ?? + this.config.getFileFilteringRespectGitIgnore(); + const fileDiscovery = this.config.getFileService(); + + // Collect entries from all search directories + let allEntries: GlobPath[] = []; + + for (const searchDir of searchDirectories) { + const entries = (await glob(this.params.pattern, { + cwd: searchDir, + withFileTypes: true, + nodir: true, + stat: true, + nocase: !this.params.case_sensitive, + dot: true, + ignore: ['**/node_modules/**', '**/.git/**'], + follow: false, + signal, + })) as GlobPath[]; + + allEntries = allEntries.concat(entries); + } + + const entries = allEntries; + + // Apply git-aware filtering if enabled and in git repository + let filteredEntries = entries; + let gitIgnoredCount = 0; + + if (respectGitIgnore) { + const relativePaths = entries.map((p) => + path.relative(this.config.getTargetDir(), p.fullpath()), + ); + const filteredRelativePaths = fileDiscovery.filterFiles(relativePaths, { + respectGitIgnore, + }); + const filteredAbsolutePaths = new Set( + filteredRelativePaths.map((p) => + path.resolve(this.config.getTargetDir(), p), + ), + ); + + filteredEntries = entries.filter((entry) => + filteredAbsolutePaths.has(entry.fullpath()), + ); + gitIgnoredCount = entries.length - filteredEntries.length; + } + + if (!filteredEntries || filteredEntries.length === 0) { + let message = `No files found matching pattern "${this.params.pattern}"`; + if (searchDirectories.length === 1) { + message += ` within ${searchDirectories[0]}`; + } else { + message += ` within ${searchDirectories.length} workspace directories`; + } + if (gitIgnoredCount > 0) { + message += ` (${gitIgnoredCount} files were git-ignored)`; + } + return { + llmContent: message, + returnDisplay: `No files found`, + }; + } + + // Set filtering such that we first show the most recent files + const oneDayInMs = 24 * 60 * 60 * 1000; + const nowTimestamp = new Date().getTime(); + + // Sort the filtered entries using the new helper function + const sortedEntries = sortFileEntries( + filteredEntries, + nowTimestamp, + oneDayInMs, + ); + + const sortedAbsolutePaths = sortedEntries.map((entry) => + entry.fullpath(), + ); + const fileListDescription = sortedAbsolutePaths.join('\n'); + const fileCount = sortedAbsolutePaths.length; + + let resultMessage = `Found ${fileCount} file(s) matching "${this.params.pattern}"`; + if (searchDirectories.length === 1) { + resultMessage += ` within ${searchDirectories[0]}`; + } else { + resultMessage += ` across ${searchDirectories.length} workspace directories`; + } + if (gitIgnoredCount > 0) { + resultMessage += ` (${gitIgnoredCount} additional files were git-ignored)`; + } + resultMessage += `, sorted by modification time (newest first):\n${fileListDescription}`; + + return { + llmContent: resultMessage, + returnDisplay: `Found ${fileCount} matching file(s)`, + }; + } catch (error) { + const errorMessage = + error instanceof Error ? error.message : String(error); + console.error(`GlobLogic execute Error: ${errorMessage}`, error); + return { + llmContent: `Error during glob search operation: ${errorMessage}`, + returnDisplay: `Error: An unexpected error occurred.`, + }; + } + } +} + /** * Implementation of the Glob tool logic */ -export class GlobTool extends BaseTool { +export class GlobTool extends BaseDeclarativeTool { static readonly Name = 'glob'; constructor(private config: Config) { @@ -158,166 +322,9 @@ export class GlobTool extends BaseTool { return null; } - /** - * Gets a description of the glob operation. - */ - getDescription(params: GlobToolParams): string { - let description = `'${params.pattern}'`; - if (params.path) { - const searchDir = path.resolve( - this.config.getTargetDir(), - params.path || '.', - ); - const relativePath = makeRelative(searchDir, this.config.getTargetDir()); - description += ` within ${shortenPath(relativePath)}`; - } - return description; - } - - /** - * Executes the glob search with the given parameters - */ - async execute( + protected createInvocation( params: GlobToolParams, - signal: AbortSignal, - ): Promise { - const validationError = this.validateToolParams(params); - if (validationError) { - return { - llmContent: `Error: Invalid parameters provided. Reason: ${validationError}`, - returnDisplay: validationError, - }; - } - - try { - const workspaceContext = this.config.getWorkspaceContext(); - const workspaceDirectories = workspaceContext.getDirectories(); - - // If a specific path is provided, resolve it and check if it's within workspace - let searchDirectories: readonly string[]; - if (params.path) { - const searchDirAbsolute = path.resolve( - this.config.getTargetDir(), - params.path, - ); - if (!workspaceContext.isPathWithinWorkspace(searchDirAbsolute)) { - return { - llmContent: `Error: Path "${params.path}" is not within any workspace directory`, - returnDisplay: `Path is not within workspace`, - }; - } - searchDirectories = [searchDirAbsolute]; - } else { - // Search across all workspace directories - searchDirectories = workspaceDirectories; - } - - // Get centralized file discovery service - const respectGitIgnore = - params.respect_git_ignore ?? - this.config.getFileFilteringRespectGitIgnore(); - const fileDiscovery = this.config.getFileService(); - - // Collect entries from all search directories - let allEntries: GlobPath[] = []; - - for (const searchDir of searchDirectories) { - const entries = (await glob(params.pattern, { - cwd: searchDir, - withFileTypes: true, - nodir: true, - stat: true, - nocase: !params.case_sensitive, - dot: true, - ignore: ['**/node_modules/**', '**/.git/**'], - follow: false, - signal, - })) as GlobPath[]; - - allEntries = allEntries.concat(entries); - } - - const entries = allEntries; - - // Apply git-aware filtering if enabled and in git repository - let filteredEntries = entries; - let gitIgnoredCount = 0; - - if (respectGitIgnore) { - const relativePaths = entries.map((p) => - path.relative(this.config.getTargetDir(), p.fullpath()), - ); - const filteredRelativePaths = fileDiscovery.filterFiles(relativePaths, { - respectGitIgnore, - }); - const filteredAbsolutePaths = new Set( - filteredRelativePaths.map((p) => - path.resolve(this.config.getTargetDir(), p), - ), - ); - - filteredEntries = entries.filter((entry) => - filteredAbsolutePaths.has(entry.fullpath()), - ); - gitIgnoredCount = entries.length - filteredEntries.length; - } - - if (!filteredEntries || filteredEntries.length === 0) { - let message = `No files found matching pattern "${params.pattern}"`; - if (searchDirectories.length === 1) { - message += ` within ${searchDirectories[0]}`; - } else { - message += ` within ${searchDirectories.length} workspace directories`; - } - if (gitIgnoredCount > 0) { - message += ` (${gitIgnoredCount} files were git-ignored)`; - } - return { - llmContent: message, - returnDisplay: `No files found`, - }; - } - - // Set filtering such that we first show the most recent files - const oneDayInMs = 24 * 60 * 60 * 1000; - const nowTimestamp = new Date().getTime(); - - // Sort the filtered entries using the new helper function - const sortedEntries = sortFileEntries( - filteredEntries, - nowTimestamp, - oneDayInMs, - ); - - const sortedAbsolutePaths = sortedEntries.map((entry) => - entry.fullpath(), - ); - const fileListDescription = sortedAbsolutePaths.join('\n'); - const fileCount = sortedAbsolutePaths.length; - - let resultMessage = `Found ${fileCount} file(s) matching "${params.pattern}"`; - if (searchDirectories.length === 1) { - resultMessage += ` within ${searchDirectories[0]}`; - } else { - resultMessage += ` across ${searchDirectories.length} workspace directories`; - } - if (gitIgnoredCount > 0) { - resultMessage += ` (${gitIgnoredCount} additional files were git-ignored)`; - } - resultMessage += `, sorted by modification time (newest first):\n${fileListDescription}`; - - return { - llmContent: resultMessage, - returnDisplay: `Found ${fileCount} matching file(s)`, - }; - } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); - console.error(`GlobLogic execute Error: ${errorMessage}`, error); - return { - llmContent: `Error during glob search operation: ${errorMessage}`, - returnDisplay: `Error: An unexpected error occurred.`, - }; - } + ): ToolInvocation { + return new GlobToolInvocation(this.config, params); } } diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index aadc93ae..4bb59115 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -120,7 +120,8 @@ describe('GrepTool', () => { describe('execute', () => { it('should find matches for a simple pattern in all files', async () => { const params: GrepToolParams = { pattern: 'world' }; - const result = await grepTool.execute(params, abortSignal); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'Found 3 matches for pattern "world" in the workspace directory', ); @@ -136,7 +137,8 @@ describe('GrepTool', () => { it('should find matches in a specific path', async () => { const params: GrepToolParams = { pattern: 'world', path: 'sub' }; - const result = await grepTool.execute(params, abortSignal); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'Found 1 match for pattern "world" in path "sub"', ); @@ -147,7 +149,8 @@ describe('GrepTool', () => { it('should find matches with an include glob', async () => { const params: GrepToolParams = { pattern: 'hello', include: '*.js' }; - const result = await grepTool.execute(params, abortSignal); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'Found 1 match for pattern "hello" in the workspace directory (filter: "*.js"):', ); @@ -168,7 +171,8 @@ describe('GrepTool', () => { path: 'sub', include: '*.js', }; - const result = await grepTool.execute(params, abortSignal); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'Found 1 match for pattern "hello" in path "sub" (filter: "*.js")', ); @@ -179,7 +183,8 @@ describe('GrepTool', () => { it('should return "No matches found" when pattern does not exist', async () => { const params: GrepToolParams = { pattern: 'nonexistentpattern' }; - const result = await grepTool.execute(params, abortSignal); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'No matches found for pattern "nonexistentpattern" in the workspace directory.', ); @@ -188,7 +193,8 @@ describe('GrepTool', () => { it('should handle regex special characters correctly', async () => { const params: GrepToolParams = { pattern: 'foo.*bar' }; // Matches 'const foo = "bar";' - const result = await grepTool.execute(params, abortSignal); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'Found 1 match for pattern "foo.*bar" in the workspace directory:', ); @@ -198,7 +204,8 @@ describe('GrepTool', () => { it('should be case-insensitive by default (JS fallback)', async () => { const params: GrepToolParams = { pattern: 'HELLO' }; - const result = await grepTool.execute(params, abortSignal); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain( 'Found 2 matches for pattern "HELLO" in the workspace directory:', ); @@ -210,14 +217,10 @@ describe('GrepTool', () => { ); }); - it('should return an error if params are invalid', async () => { + it('should throw an error if params are invalid', async () => { const params = { path: '.' } as unknown as GrepToolParams; // Invalid: pattern missing - const result = await grepTool.execute(params, abortSignal); - expect(result.llmContent).toBe( - "Error: Invalid parameters provided. Reason: params must have required property 'pattern'", - ); - expect(result.returnDisplay).toBe( - "Model provided invalid parameters. Error: params must have required property 'pattern'", + expect(() => grepTool.build(params)).toThrow( + /params must have required property 'pattern'/, ); }); }); @@ -246,7 +249,8 @@ describe('GrepTool', () => { const multiDirGrepTool = new GrepTool(multiDirConfig); const params: GrepToolParams = { pattern: 'world' }; - const result = await multiDirGrepTool.execute(params, abortSignal); + const invocation = multiDirGrepTool.build(params); + const result = await invocation.execute(abortSignal); // Should find matches in both directories expect(result.llmContent).toContain( @@ -297,7 +301,8 @@ describe('GrepTool', () => { // Search only in the 'sub' directory of the first workspace const params: GrepToolParams = { pattern: 'world', path: 'sub' }; - const result = await multiDirGrepTool.execute(params, abortSignal); + const invocation = multiDirGrepTool.build(params); + const result = await invocation.execute(abortSignal); // Should only find matches in the specified sub directory expect(result.llmContent).toContain( @@ -317,7 +322,8 @@ describe('GrepTool', () => { describe('getDescription', () => { it('should generate correct description with pattern only', () => { const params: GrepToolParams = { pattern: 'testPattern' }; - expect(grepTool.getDescription(params)).toBe("'testPattern'"); + const invocation = grepTool.build(params); + expect(invocation.getDescription()).toBe("'testPattern'"); }); it('should generate correct description with pattern and include', () => { @@ -325,19 +331,21 @@ describe('GrepTool', () => { pattern: 'testPattern', include: '*.ts', }; - expect(grepTool.getDescription(params)).toBe("'testPattern' in *.ts"); + const invocation = grepTool.build(params); + expect(invocation.getDescription()).toBe("'testPattern' in *.ts"); }); - it('should generate correct description with pattern and path', () => { + it('should generate correct description with pattern and path', async () => { + const dirPath = path.join(tempRootDir, 'src', 'app'); + await fs.mkdir(dirPath, { recursive: true }); const params: GrepToolParams = { pattern: 'testPattern', path: path.join('src', 'app'), }; + const invocation = grepTool.build(params); // The path will be relative to the tempRootDir, so we check for containment. - expect(grepTool.getDescription(params)).toContain("'testPattern' within"); - expect(grepTool.getDescription(params)).toContain( - path.join('src', 'app'), - ); + expect(invocation.getDescription()).toContain("'testPattern' within"); + expect(invocation.getDescription()).toContain(path.join('src', 'app')); }); it('should indicate searching across all workspace directories when no path specified', () => { @@ -350,28 +358,31 @@ describe('GrepTool', () => { const multiDirGrepTool = new GrepTool(multiDirConfig); const params: GrepToolParams = { pattern: 'testPattern' }; - expect(multiDirGrepTool.getDescription(params)).toBe( + const invocation = multiDirGrepTool.build(params); + expect(invocation.getDescription()).toBe( "'testPattern' across all workspace directories", ); }); - it('should generate correct description with pattern, include, and path', () => { + it('should generate correct description with pattern, include, and path', async () => { + const dirPath = path.join(tempRootDir, 'src', 'app'); + await fs.mkdir(dirPath, { recursive: true }); const params: GrepToolParams = { pattern: 'testPattern', include: '*.ts', path: path.join('src', 'app'), }; - expect(grepTool.getDescription(params)).toContain( + const invocation = grepTool.build(params); + expect(invocation.getDescription()).toContain( "'testPattern' in *.ts within", ); - expect(grepTool.getDescription(params)).toContain( - path.join('src', 'app'), - ); + expect(invocation.getDescription()).toContain(path.join('src', 'app')); }); it('should use ./ for root path in description', () => { const params: GrepToolParams = { pattern: 'testPattern', path: '.' }; - expect(grepTool.getDescription(params)).toBe("'testPattern' within ./"); + const invocation = grepTool.build(params); + expect(invocation.getDescription()).toBe("'testPattern' within ./"); }); }); }); diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 027ab1b1..8e2b84f1 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -10,7 +10,13 @@ import path from 'path'; import { EOL } from 'os'; import { spawn } from 'child_process'; import { globStream } from 'glob'; -import { BaseTool, Icon, ToolResult } from './tools.js'; +import { + BaseDeclarativeTool, + BaseToolInvocation, + Icon, + ToolInvocation, + ToolResult, +} from './tools.js'; import { Type } from '@google/genai'; import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; @@ -49,46 +55,17 @@ interface GrepMatch { line: string; } -// --- GrepLogic Class --- - -/** - * Implementation of the Grep tool logic (moved from CLI) - */ -export class GrepTool extends BaseTool { - static readonly Name = 'search_file_content'; // Keep static name - - constructor(private readonly config: Config) { - super( - GrepTool.Name, - 'SearchText', - 'Searches for a regular expression pattern within the content of files in a specified directory (or current working directory). Can filter files by a glob pattern. Returns the lines containing matches, along with their file paths and line numbers.', - Icon.Regex, - { - properties: { - pattern: { - description: - "The regular expression (regex) pattern to search for within file contents (e.g., 'function\\s+myFunction', 'import\\s+\\{.*\\}\\s+from\\s+.*').", - type: Type.STRING, - }, - path: { - description: - 'Optional: The absolute path to the directory to search within. If omitted, searches the current working directory.', - type: Type.STRING, - }, - include: { - description: - "Optional: A glob pattern to filter which files are searched (e.g., '*.js', '*.{ts,tsx}', 'src/**'). If omitted, searches all files (respecting potential global ignores).", - type: Type.STRING, - }, - }, - required: ['pattern'], - type: Type.OBJECT, - }, - ); +class GrepToolInvocation extends BaseToolInvocation< + GrepToolParams, + ToolResult +> { + constructor( + private readonly config: Config, + params: GrepToolParams, + ) { + super(params); } - // --- Validation Methods --- - /** * Checks if a path is within the root directory and resolves it. * @param relativePath Path relative to the root directory (or undefined for root). @@ -130,58 +107,11 @@ export class GrepTool extends BaseTool { return targetPath; } - /** - * Validates the parameters for the tool - * @param params Parameters to validate - * @returns An error message string if invalid, null otherwise - */ - validateToolParams(params: GrepToolParams): string | null { - const errors = SchemaValidator.validate(this.schema.parameters, params); - if (errors) { - return errors; - } - - try { - new RegExp(params.pattern); - } catch (error) { - return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`; - } - - // Only validate path if one is provided - if (params.path) { - try { - this.resolveAndValidatePath(params.path); - } catch (error) { - return getErrorMessage(error); - } - } - - return null; // Parameters are valid - } - - // --- Core Execution --- - - /** - * Executes the grep search with the given parameters - * @param params Parameters for the grep search - * @returns Result of the grep search - */ - async execute( - params: GrepToolParams, - signal: AbortSignal, - ): Promise { - const validationError = this.validateToolParams(params); - if (validationError) { - return { - llmContent: `Error: Invalid parameters provided. Reason: ${validationError}`, - returnDisplay: `Model provided invalid parameters. Error: ${validationError}`, - }; - } - + async execute(signal: AbortSignal): Promise { try { const workspaceContext = this.config.getWorkspaceContext(); - const searchDirAbs = this.resolveAndValidatePath(params.path); - const searchDirDisplay = params.path || '.'; + const searchDirAbs = this.resolveAndValidatePath(this.params.path); + const searchDirDisplay = this.params.path || '.'; // Determine which directories to search let searchDirectories: readonly string[]; @@ -197,9 +127,9 @@ export class GrepTool extends BaseTool { let allMatches: GrepMatch[] = []; for (const searchDir of searchDirectories) { const matches = await this.performGrepSearch({ - pattern: params.pattern, + pattern: this.params.pattern, path: searchDir, - include: params.include, + include: this.params.include, signal, }); @@ -226,7 +156,7 @@ export class GrepTool extends BaseTool { } if (allMatches.length === 0) { - const noMatchMsg = `No matches found for pattern "${params.pattern}" ${searchLocationDescription}${params.include ? ` (filter: "${params.include}")` : ''}.`; + const noMatchMsg = `No matches found for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}.`; return { llmContent: noMatchMsg, returnDisplay: `No matches found` }; } @@ -247,7 +177,7 @@ export class GrepTool extends BaseTool { const matchCount = allMatches.length; const matchTerm = matchCount === 1 ? 'match' : 'matches'; - let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${params.pattern}" ${searchLocationDescription}${params.include ? ` (filter: "${params.include}")` : ''}: + let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}: --- `; @@ -274,8 +204,6 @@ export class GrepTool extends BaseTool { } } - // --- Grep Implementation Logic --- - /** * Checks if a command is available in the system's PATH. * @param {string} command The command name (e.g., 'git', 'grep'). @@ -353,17 +281,20 @@ export class GrepTool extends BaseTool { * @param params Parameters for the grep operation * @returns A string describing the grep */ - getDescription(params: GrepToolParams): string { - let description = `'${params.pattern}'`; - if (params.include) { - description += ` in ${params.include}`; + getDescription(): string { + let description = `'${this.params.pattern}'`; + if (this.params.include) { + description += ` in ${this.params.include}`; } - if (params.path) { + if (this.params.path) { const resolvedPath = path.resolve( this.config.getTargetDir(), - params.path, + this.params.path, ); - if (resolvedPath === this.config.getTargetDir() || params.path === '.') { + if ( + resolvedPath === this.config.getTargetDir() || + this.params.path === '.' + ) { description += ` within ./`; } else { const relativePath = makeRelative( @@ -445,7 +376,9 @@ export class GrepTool extends BaseTool { return this.parseGrepOutput(output, absolutePath); } catch (gitError: unknown) { console.debug( - `GrepLogic: git grep failed: ${getErrorMessage(gitError)}. Falling back...`, + `GrepLogic: git grep failed: ${getErrorMessage( + gitError, + )}. Falling back...`, ); } } @@ -525,7 +458,9 @@ export class GrepTool extends BaseTool { return this.parseGrepOutput(output, absolutePath); } catch (grepError: unknown) { console.debug( - `GrepLogic: System grep failed: ${getErrorMessage(grepError)}. Falling back...`, + `GrepLogic: System grep failed: ${getErrorMessage( + grepError, + )}. Falling back...`, ); } } @@ -576,7 +511,9 @@ export class GrepTool extends BaseTool { // Ignore errors like permission denied or file gone during read if (!isNodeError(readError) || readError.code !== 'ENOENT') { console.debug( - `GrepLogic: Could not read/process ${fileAbsolutePath}: ${getErrorMessage(readError)}`, + `GrepLogic: Could not read/process ${fileAbsolutePath}: ${getErrorMessage( + readError, + )}`, ); } } @@ -585,9 +522,126 @@ export class GrepTool extends BaseTool { return allMatches; } catch (error: unknown) { console.error( - `GrepLogic: Error in performGrepSearch (Strategy: ${strategyUsed}): ${getErrorMessage(error)}`, + `GrepLogic: Error in performGrepSearch (Strategy: ${strategyUsed}): ${getErrorMessage( + error, + )}`, ); throw error; // Re-throw } } } + +// --- GrepLogic Class --- + +/** + * Implementation of the Grep tool logic (moved from CLI) + */ +export class GrepTool extends BaseDeclarativeTool { + static readonly Name = 'search_file_content'; // Keep static name + + constructor(private readonly config: Config) { + super( + GrepTool.Name, + 'SearchText', + 'Searches for a regular expression pattern within the content of files in a specified directory (or current working directory). Can filter files by a glob pattern. Returns the lines containing matches, along with their file paths and line numbers.', + Icon.Regex, + { + properties: { + pattern: { + description: + "The regular expression (regex) pattern to search for within file contents (e.g., 'function\\s+myFunction', 'import\\s+\\{.*\\}\\s+from\\s+.*').", + type: Type.STRING, + }, + path: { + description: + 'Optional: The absolute path to the directory to search within. If omitted, searches the current working directory.', + type: Type.STRING, + }, + include: { + description: + "Optional: A glob pattern to filter which files are searched (e.g., '*.js', '*.{ts,tsx}', 'src/**'). If omitted, searches all files (respecting potential global ignores).", + type: Type.STRING, + }, + }, + required: ['pattern'], + type: Type.OBJECT, + }, + ); + } + + /** + * Checks if a path is within the root directory and resolves it. + * @param relativePath Path relative to the root directory (or undefined for root). + * @returns The absolute path if valid and exists, or null if no path specified (to search all directories). + * @throws {Error} If path is outside root, doesn't exist, or isn't a directory. + */ + private resolveAndValidatePath(relativePath?: string): string | null { + // If no path specified, return null to indicate searching all workspace directories + if (!relativePath) { + return null; + } + + const targetPath = path.resolve(this.config.getTargetDir(), relativePath); + + // Security Check: Ensure the resolved path is within workspace boundaries + const workspaceContext = this.config.getWorkspaceContext(); + if (!workspaceContext.isPathWithinWorkspace(targetPath)) { + const directories = workspaceContext.getDirectories(); + throw new Error( + `Path validation failed: Attempted path "${relativePath}" resolves outside the allowed workspace directories: ${directories.join(', ')}`, + ); + } + + // Check existence and type after resolving + try { + const stats = fs.statSync(targetPath); + if (!stats.isDirectory()) { + throw new Error(`Path is not a directory: ${targetPath}`); + } + } catch (error: unknown) { + if (isNodeError(error) && error.code !== 'ENOENT') { + throw new Error(`Path does not exist: ${targetPath}`); + } + throw new Error( + `Failed to access path stats for ${targetPath}: ${error}`, + ); + } + + return targetPath; + } + + /** + * Validates the parameters for the tool + * @param params Parameters to validate + * @returns An error message string if invalid, null otherwise + */ + validateToolParams(params: GrepToolParams): string | null { + const errors = SchemaValidator.validate(this.schema.parameters, params); + if (errors) { + return errors; + } + + try { + new RegExp(params.pattern); + } catch (error) { + return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`; + } + + // Only validate path if one is provided + if (params.path) { + try { + this.resolveAndValidatePath(params.path); + } catch (error) { + return getErrorMessage(error); + } + } + + return null; // Parameters are valid + } + + protected createInvocation( + params: GrepToolParams, + ): ToolInvocation { + return new GrepToolInvocation(this.config, params); + } +} diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 7ef9d2b5..4c1d044c 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -9,6 +9,7 @@ import { SchemaValidator } from '../utils/schemaValidator.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { BaseDeclarativeTool, + BaseToolInvocation, Icon, ToolInvocation, ToolLocation, @@ -45,13 +46,16 @@ export interface ReadFileToolParams { limit?: number; } -class ReadFileToolInvocation - implements ToolInvocation -{ +class ReadFileToolInvocation extends BaseToolInvocation< + ReadFileToolParams, + ToolResult +> { constructor( private config: Config, - public params: ReadFileToolParams, - ) {} + params: ReadFileToolParams, + ) { + super(params); + } getDescription(): string { const relativePath = makeRelative( @@ -61,14 +65,10 @@ class ReadFileToolInvocation return shortenPath(relativePath); } - toolLocations(): ToolLocation[] { + override toolLocations(): ToolLocation[] { return [{ path: this.params.absolute_path, line: this.params.offset }]; } - shouldConfirmExecute(): Promise { - return Promise.resolve(false); - } - async execute(): Promise { const result = await processSingleFileContent( this.params.absolute_path, diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 79e6f010..ceacd6ca 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -53,6 +53,34 @@ export interface ToolInvocation< ): Promise; } +/** + * A convenience base class for ToolInvocation. + */ +export abstract class BaseToolInvocation< + TParams extends object, + TResult extends ToolResult, +> implements ToolInvocation +{ + constructor(readonly params: TParams) {} + + abstract getDescription(): string; + + toolLocations(): ToolLocation[] { + return []; + } + + shouldConfirmExecute( + _abortSignal: AbortSignal, + ): Promise { + return Promise.resolve(false); + } + + abstract execute( + signal: AbortSignal, + updateOutput?: (output: string) => void, + ): Promise; +} + /** * A type alias for a tool invocation where the specific parameter and result types are not known. */