From ae02236c63fa605f5e473b81efe922025e5b140f Mon Sep 17 00:00:00 2001 From: anthony bushong Date: Thu, 9 Oct 2025 13:51:16 -0700 Subject: [PATCH] feat(core): generalize path correction for use across tools (#10612) --- packages/core/src/tools/smart-edit.test.ts | 71 ------------ packages/core/src/tools/smart-edit.ts | 65 ++--------- packages/core/src/utils/pathCorrector.test.ts | 101 ++++++++++++++++++ packages/core/src/utils/pathCorrector.ts | 63 +++++++++++ 4 files changed, 174 insertions(+), 126 deletions(-) create mode 100644 packages/core/src/utils/pathCorrector.test.ts create mode 100644 packages/core/src/utils/pathCorrector.ts diff --git a/packages/core/src/tools/smart-edit.test.ts b/packages/core/src/tools/smart-edit.test.ts index f6c4c107c8..292ae31b25 100644 --- a/packages/core/src/tools/smart-edit.test.ts +++ b/packages/core/src/tools/smart-edit.test.ts @@ -274,77 +274,6 @@ describe('SmartEditTool', () => { expect(result.occurrences).toBe(1); }); }); - 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', () => { diff --git a/packages/core/src/tools/smart-edit.ts b/packages/core/src/tools/smart-edit.ts index 11c5c45d8d..34586a829a 100644 --- a/packages/core/src/tools/smart-edit.ts +++ b/packages/core/src/tools/smart-edit.ts @@ -38,6 +38,7 @@ import { logSmartEditStrategy } from '../telemetry/loggers.js'; import { SmartEditCorrectionEvent } from '../telemetry/types.js'; import { logSmartEditCorrectionEvent } from '../telemetry/loggers.js'; +import { correctPath } from '../utils/pathCorrector.js'; interface ReplacementContext { params: EditToolParams; currentContent: string; @@ -876,58 +877,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 @@ -940,11 +889,17 @@ A good instruction should concisely answer: return "The 'file_path' parameter must be non-empty."; } - if (!path.isAbsolute(params.file_path)) { + let filePath = params.file_path; + if (!path.isAbsolute(filePath)) { // Attempt to auto-correct to an absolute path - const error = this.correctPath(params); - if (error) return error; + const result = correctPath(filePath, this.config); + if (result.success) { + filePath = result.correctedPath; + } else { + return result.error; + } } + params.file_path = filePath; const workspaceContext = this.config.getWorkspaceContext(); if (!workspaceContext.isPathWithinWorkspace(params.file_path)) { diff --git a/packages/core/src/utils/pathCorrector.test.ts b/packages/core/src/utils/pathCorrector.test.ts new file mode 100644 index 0000000000..3516183695 --- /dev/null +++ b/packages/core/src/utils/pathCorrector.test.ts @@ -0,0 +1,101 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import type { Config } from '../config/config.js'; +import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; +import { StandardFileSystemService } from '../services/fileSystemService.js'; +import { correctPath } from './pathCorrector.js'; + +describe('pathCorrector', () => { + let tempDir: string; + let rootDir: string; + let otherWorkspaceDir: string; + let mockConfig: Config; + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-corrector-test-')); + rootDir = path.join(tempDir, 'root'); + otherWorkspaceDir = path.join(tempDir, 'other'); + fs.mkdirSync(rootDir, { recursive: true }); + fs.mkdirSync(otherWorkspaceDir, { recursive: true }); + + mockConfig = { + getTargetDir: () => rootDir, + getWorkspaceContext: () => + createMockWorkspaceContext(rootDir, [otherWorkspaceDir]), + getFileSystemService: () => new StandardFileSystemService(), + } as unknown as Config; + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + it('should correct a relative path if it is unambiguous in the target dir', () => { + const testFile = 'unique.txt'; + fs.writeFileSync(path.join(rootDir, testFile), 'content'); + + const result = correctPath(testFile, mockConfig); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.correctedPath).toBe(path.join(rootDir, testFile)); + } + }); + + it('should correct a partial relative path if it is unambiguous in another workspace dir', () => { + const subDir = path.join(otherWorkspaceDir, 'sub'); + fs.mkdirSync(subDir); + const testFile = 'file.txt'; + const fullPath = path.join(subDir, testFile); + fs.writeFileSync(fullPath, 'content'); + + const result = correctPath(testFile, mockConfig); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.correctedPath).toBe(fullPath); + } + }); + + it('should return an error for a relative path that does not exist', () => { + const result = correctPath('nonexistent.txt', mockConfig); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toMatch( + /File not found for 'nonexistent.txt' and path is not absolute./, + ); + } else { + expect.fail('Expected path correction to fail.'); + } + }); + + it('should return an error for an ambiguous path', () => { + const ambiguousFile = 'component.ts'; + const subDir1 = path.join(rootDir, 'module1'); + const subDir2 = path.join(otherWorkspaceDir, 'module2'); + fs.mkdirSync(subDir1, { recursive: true }); + fs.mkdirSync(subDir2, { recursive: true }); + fs.writeFileSync(path.join(subDir1, ambiguousFile), 'content 1'); + fs.writeFileSync(path.join(subDir2, ambiguousFile), 'content 2'); + + const result = correctPath(ambiguousFile, mockConfig); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toMatch( + /The file path 'component.ts' is ambiguous and matches multiple files./, + ); + } else { + expect.fail('Expected path correction to fail.'); + } + }); +}); diff --git a/packages/core/src/utils/pathCorrector.ts b/packages/core/src/utils/pathCorrector.ts new file mode 100644 index 0000000000..94576c43e5 --- /dev/null +++ b/packages/core/src/utils/pathCorrector.ts @@ -0,0 +1,63 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import type { Config } from '../config/config.js'; + +type SuccessfulPathCorrection = { + success: true; + correctedPath: string; +}; + +type FailedPathCorrection = { + success: false; + error: string; +}; + +/** + * Attempts to correct a relative or ambiguous file path to a single, absolute path + * within the workspace. + * + * @param filePath The file path to correct. + * @param config The application configuration. + * @returns A `PathCorrectionResult` object with either a `correctedPath` or an `error`. + */ +export type PathCorrectionResult = + | SuccessfulPathCorrection + | FailedPathCorrection; +export function correctPath( + filePath: string, + config: Config, +): PathCorrectionResult { + // Check for direct path relative to the primary target directory. + const directPath = path.join(config.getTargetDir(), filePath); + if (fs.existsSync(directPath)) { + return { success: true, correctedPath: directPath }; + } + + // If not found directly, search across all workspace directories for ambiguous matches. + const workspaceContext = config.getWorkspaceContext(); + const fileSystem = config.getFileSystemService(); + const searchPaths = workspaceContext.getDirectories(); + const foundFiles = fileSystem.findFiles(filePath, searchPaths); + + if (foundFiles.length === 0) { + return { + success: false, + error: `File not found for '${filePath}' and path is not absolute.`, + }; + } + + if (foundFiles.length > 1) { + return { + success: false, + error: `The file path '${filePath}' is ambiguous and matches multiple files. Please provide a more specific path. Matches: ${foundFiles.join(', ')}`, + }; + } + + return { success: true, correctedPath: foundFiles[0] }; +}