From eb1a6a6091155932e3584a58da8240bab12f15cd Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Fri, 26 Sep 2025 18:05:48 -0700 Subject: [PATCH] Revert "fix(core): auto-correct file paths in smart edit where possible" (#10009) --- .../src/zed-integration/fileSystemService.ts | 4 - .../core/src/services/fileSystemService.ts | 26 +------ packages/core/src/tools/smart-edit.test.ts | 78 +------------------ packages/core/src/tools/smart-edit.ts | 57 +------------- 4 files changed, 6 insertions(+), 159 deletions(-) diff --git a/packages/cli/src/zed-integration/fileSystemService.ts b/packages/cli/src/zed-integration/fileSystemService.ts index 0604c34153..a56593f8c3 100644 --- a/packages/cli/src/zed-integration/fileSystemService.ts +++ b/packages/cli/src/zed-integration/fileSystemService.ts @@ -44,8 +44,4 @@ export class AcpFileSystemService implements FileSystemService { sessionId: this.sessionId, }); } - - findFiles(fileName: string, searchPaths: readonly string[]): string[] { - return this.fallback.findFiles(fileName, searchPaths); - } } diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 8209925956..946c227ab6 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -4,9 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as fs from 'node:fs/promises'; -import * as path from 'node:path'; -import { globSync } from 'glob'; +import fs from 'node:fs/promises'; /** * Interface for file system operations that may be delegated to different implementations @@ -27,15 +25,6 @@ export interface FileSystemService { * @param content - The content to write */ writeTextFile(filePath: string, content: string): Promise; - - /** - * Finds files with a given name within specified search paths. - * - * @param fileName - The name of the file to find. - * @param searchPaths - An array of directory paths to search within. - * @returns An array of absolute paths to the found files. - */ - findFiles(fileName: string, searchPaths: readonly string[]): string[]; } /** @@ -49,17 +38,4 @@ export class StandardFileSystemService implements FileSystemService { async writeTextFile(filePath: string, content: string): Promise { await fs.writeFile(filePath, content, 'utf-8'); } - - findFiles(fileName: string, searchPaths: readonly string[]): string[] { - const foundFiles: string[] = []; - for (const searchPath of searchPaths) { - const pattern = path.join(searchPath, '**', fileName); - const matches = globSync(pattern, { - nodir: true, - absolute: true, - }); - foundFiles.push(...matches); - } - return foundFiles; - } } diff --git a/packages/core/src/tools/smart-edit.test.ts b/packages/core/src/tools/smart-edit.test.ts index bb1b37002a..f1bd9f14c6 100644 --- a/packages/core/src/tools/smart-edit.test.ts +++ b/packages/core/src/tools/smart-edit.test.ts @@ -242,78 +242,6 @@ describe('SmartEditTool', () => { }); }); - describe('correctPath', () => { - it('should correct a relative path if it is unambiguous', () => { - const testFile = 'unique.txt'; - fs.writeFileSync(path.join(rootDir, testFile), 'content'); - - const params: EditToolParams = { - file_path: testFile, - instruction: 'An instruction', - old_string: 'old', - new_string: 'new', - }; - - const validationResult = (tool as any).correctPath(params); - - expect(validationResult).toBeNull(); - expect(params.file_path).toBe(path.join(rootDir, testFile)); - }); - - it('should correct a partial relative path if it is unambiguous', () => { - const subDir = path.join(rootDir, 'sub'); - fs.mkdirSync(subDir); - const testFile = 'file.txt'; - const partialPath = path.join('sub', testFile); - const fullPath = path.join(subDir, testFile); - fs.writeFileSync(fullPath, 'content'); - - const params: EditToolParams = { - file_path: partialPath, - instruction: 'An instruction', - old_string: 'old', - new_string: 'new', - }; - - const validationResult = (tool as any).correctPath(params); - - expect(validationResult).toBeNull(); - expect(params.file_path).toBe(fullPath); - }); - - it('should return an error for a relative path that does not exist', () => { - const params: EditToolParams = { - file_path: 'test.txt', - instruction: 'An instruction', - old_string: 'old', - new_string: 'new', - }; - const result = (tool as any).correctPath(params); - expect(result).toMatch(/File not found for 'test.txt'/); - }); - - it('should return an error for an ambiguous path', () => { - const subDir1 = path.join(rootDir, 'module1'); - const subDir2 = path.join(rootDir, 'module2'); - fs.mkdirSync(subDir1, { recursive: true }); - fs.mkdirSync(subDir2, { recursive: true }); - - const ambiguousFile = 'component.ts'; - fs.writeFileSync(path.join(subDir1, ambiguousFile), 'content 1'); - fs.writeFileSync(path.join(subDir2, ambiguousFile), 'content 2'); - - const params: EditToolParams = { - file_path: ambiguousFile, - instruction: 'An instruction', - old_string: 'old', - new_string: 'new', - }; - - const validationResult = (tool as any).correctPath(params); - expect(validationResult).toMatch(/ambiguous and matches multiple files/); - }); - }); - describe('validateToolParams', () => { it('should return null for valid params', () => { const params: EditToolParams = { @@ -325,15 +253,15 @@ describe('SmartEditTool', () => { expect(tool.validateToolParams(params)).toBeNull(); }); - it('should return an error if path is outside the workspace', () => { + it('should return error for relative path', () => { const params: EditToolParams = { - file_path: path.join(os.tmpdir(), 'outside.txt'), + file_path: 'test.txt', instruction: 'An instruction', old_string: 'old', new_string: 'new', }; expect(tool.validateToolParams(params)).toMatch( - /must be within one of the workspace directories/, + /File path must be absolute/, ); }); }); diff --git a/packages/core/src/tools/smart-edit.ts b/packages/core/src/tools/smart-edit.ts index 4b7a8d4f88..3f62301ead 100644 --- a/packages/core/src/tools/smart-edit.ts +++ b/packages/core/src/tools/smart-edit.ts @@ -755,58 +755,6 @@ A good instruction should concisely answer: ); } - /** - * Quickly checks if the file path can be resolved directly against the workspace root. - * @param filePath The relative file path to check. - * @returns The absolute path if the file exists, otherwise null. - */ - private findDirectPath(filePath: string): string | null { - const directPath = path.join(this.config.getTargetDir(), filePath); - return fs.existsSync(directPath) ? directPath : null; - } - - /** - * Searches for a file across all configured workspace directories. - * @param filePath The file path (can be partial) to search for. - * @returns A list of absolute paths for all matching files found. - */ - private findAmbiguousPaths(filePath: string): string[] { - const workspaceContext = this.config.getWorkspaceContext(); - const fileSystem = this.config.getFileSystemService(); - const searchPaths = workspaceContext.getDirectories(); - return fileSystem.findFiles(filePath, searchPaths); - } - - /** - * Attempts to correct a relative file path to an absolute path. - * This function modifies `params.file_path` in place if successful. - * @param params The tool parameters containing the file_path to correct. - * @returns An error message string if correction fails, otherwise null. - */ - private correctPath(params: EditToolParams): string | null { - const directPath = this.findDirectPath(params.file_path); - if (directPath) { - params.file_path = directPath; - return null; - } - - const foundFiles = this.findAmbiguousPaths(params.file_path); - - if (foundFiles.length === 0) { - return `File not found for '${params.file_path}' and path is not absolute.`; - } - - if (foundFiles.length > 1) { - return ( - `The file path '${params.file_path}' is too ambiguous and matches multiple files. ` + - `Please provide a more specific path. Matches: ${foundFiles.join(', ')}` - ); - } - - params.file_path = foundFiles[0]; - return null; - } - /** * Validates the parameters for the Edit tool * @param params Parameters to validate @@ -820,9 +768,7 @@ A good instruction should concisely answer: } if (!path.isAbsolute(params.file_path)) { - // Attempt to auto-correct to an absolute path - const error = this.correctPath(params); - if (error) return error; + return `File path must be absolute: ${params.file_path}`; } const workspaceContext = this.config.getWorkspaceContext(); @@ -830,6 +776,7 @@ A good instruction should concisely answer: const directories = workspaceContext.getDirectories(); return `File path must be within one of the workspace directories: ${directories.join(', ')}`; } + return null; }