diff --git a/packages/core/src/tools/at-reference-resolution.test.ts b/packages/core/src/tools/at-reference-resolution.test.ts deleted file mode 100644 index 26f260c226..0000000000 --- a/packages/core/src/tools/at-reference-resolution.test.ts +++ /dev/null @@ -1,591 +0,0 @@ -/** - * @license - * Copyright 2026 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { ReadFileTool } from './read-file.js'; -import { WriteFileTool, getCorrectedFileContent } from './write-file.js'; -import { EditTool } from './edit.js'; -import { correctPath } from '../utils/pathCorrector.js'; -import path from 'node:path'; -import os from 'node:os'; -import fs from 'node:fs'; -import fsp from 'node:fs/promises'; -import type { Config } from '../config/config.js'; -import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; -import { StandardFileSystemService } from '../services/fileSystemService.js'; -import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; -import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; -import { isSubpath } from '../utils/paths.js'; - -vi.mock('../telemetry/loggers.js', () => ({ - logFileOperation: vi.fn(), - logEditStrategy: vi.fn(), - logEditCorrectionEvent: vi.fn(), -})); - -vi.mock('./jit-context.js', () => ({ - discoverJitContext: vi.fn().mockResolvedValue(''), - appendJitContext: vi.fn().mockImplementation((content) => content), - appendJitContextToParts: vi.fn().mockImplementation((content) => content), -})); - -describe('Consolidated At-Reference Path Resolution Tests (b-495551283)', () => { - let tempRootDir: string; - let mockConfigInstance: Config; - const abortSignal = new AbortController().signal; - - beforeEach(async () => { - // Create a unique temporary root directory for each test run - const realTmp = await fsp.realpath(os.tmpdir()); - tempRootDir = await fsp.mkdtemp( - path.join(realTmp, 'at-ref-resolution-root-'), - ); - - mockConfigInstance = { - getFileService: () => new FileDiscoveryService(tempRootDir), - getFileSystemService: () => new StandardFileSystemService(), - getTargetDir: () => tempRootDir, - getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), - getFileFilteringOptions: () => ({ - respectGitIgnore: true, - respectGeminiIgnore: true, - }), - storage: { - getProjectTempDir: () => path.join(tempRootDir, '.temp'), - }, - isInteractive: () => false, - isPlanMode: () => false, - getActiveModel: () => undefined, - getBaseLlmClient: () => undefined, - getDisableLLMCorrection: () => true, - isPathAllowed(this: Config, absolutePath: string): boolean { - const workspaceContext = this.getWorkspaceContext(); - if (workspaceContext.isPathWithinWorkspace(absolutePath)) { - return true; - } - const projectTempDir = this.storage.getProjectTempDir(); - return isSubpath(path.resolve(projectTempDir), absolutePath); - }, - validatePathAccess(this: Config, absolutePath: string): string | null { - if (this.isPathAllowed(absolutePath)) { - return null; - } - const workspaceDirs = this.getWorkspaceContext().getDirectories(); - const projectTempDir = this.storage.getProjectTempDir(); - return `Path not in workspace: Attempted path "${absolutePath}" resolves outside the allowed workspace directories: ${workspaceDirs.join(', ')} or the project temp directory: ${projectTempDir}`; - }, - } as unknown as Config; - - // Create the policies directory and new-policies.txt file - await fsp.mkdir(path.join(tempRootDir, 'policies'), { recursive: true }); - await fsp.writeFile( - path.join(tempRootDir, 'policies', 'new-policies.txt'), - '[[rule]]\ntoolName = "run_shell_command"\ndecision = "allow"\n', - 'utf8', - ); - }); - - afterEach(async () => { - // Clean up the temporary root directory - if (fs.existsSync(tempRootDir)) { - await fsp.rm(tempRootDir, { recursive: true, force: true }); - } - }); - - it('ReadFileTool successfully reads a file when the path is prefixed with @', async () => { - const readFileTool = new ReadFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = readFileTool.build({ - file_path: '@policies/new-policies.txt', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed because it defensively strips the leading '@' - expect(result.error).toBeUndefined(); - expect(result.llmContent).toContain('toolName = "run_shell_command"'); - }); - - it('ReadFileTool successfully reads a file when the path is prefixed with @/', async () => { - const readFileTool = new ReadFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = readFileTool.build({ - file_path: '@/policies/new-policies.txt', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed because it defensively strips the leading '@/' - expect(result.error).toBeUndefined(); - expect(result.llmContent).toContain('toolName = "run_shell_command"'); - }); - - it('WriteFileTool successfully writes to/updates a file when the path is prefixed with @', async () => { - const writeFileTool = new WriteFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = writeFileTool.build({ - file_path: '@policies/new-policies.txt', - content: '[[rule]]\nupdated_content = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and update the correct file - expect(result.error).toBeUndefined(); - - const incorrectFilePath = path.join( - tempRootDir, - '@policies', - 'new-policies.txt', - ); - const correctFilePath = path.join( - tempRootDir, - 'policies', - 'new-policies.txt', - ); - - // It should NOT have created a literal "@policies" directory - expect(fs.existsSync(incorrectFilePath)).toBe(false); - - // It should have updated the correct file under "policies" - const updatedContent = await fsp.readFile(correctFilePath, 'utf8'); - expect(updatedContent).toContain('updated_content = true'); - }); - - it('WriteFileTool successfully creates a new file when the path is prefixed with @ and the parent directory exists', async () => { - const writeFileTool = new WriteFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = writeFileTool.build({ - file_path: '@policies/brand-new-file.txt', - content: '[[rule]]\nbrand_new_file = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and create the correct file - expect(result.error).toBeUndefined(); - - const incorrectFilePath = path.join( - tempRootDir, - '@policies', - 'brand-new-file.txt', - ); - const correctFilePath = path.join( - tempRootDir, - 'policies', - 'brand-new-file.txt', - ); - - // It should NOT have created a literal "@policies" directory - expect(fs.existsSync(incorrectFilePath)).toBe(false); - - // It should have created the correct file under "policies" - const createdContent = await fsp.readFile(correctFilePath, 'utf8'); - expect(createdContent).toContain('brand_new_file = true'); - }); - - it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @ and the first segment exists', async () => { - const writeFileTool = new WriteFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = writeFileTool.build({ - file_path: '@policies/sub/brand-new-file.txt', - content: '[[rule]]\nnested_brand_new_file = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and create the correct file - expect(result.error).toBeUndefined(); - - const incorrectFilePath = path.join( - tempRootDir, - '@policies', - 'sub', - 'brand-new-file.txt', - ); - const correctFilePath = path.join( - tempRootDir, - 'policies', - 'sub', - 'brand-new-file.txt', - ); - - // It should NOT have created a literal "@policies" directory - expect(fs.existsSync(incorrectFilePath)).toBe(false); - - // It should have created the correct file under "policies/sub" - const createdContent = await fsp.readFile(correctFilePath, 'utf8'); - expect(createdContent).toContain('nested_brand_new_file = true'); - }); - - it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @ and the first segment does NOT exist', async () => { - const writeFileTool = new WriteFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = writeFileTool.build({ - file_path: '@new-policies/sub/brand-new-file.txt', - content: '[[rule]]\nnested_brand_new_file = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and create the correct file - expect(result.error).toBeUndefined(); - - const incorrectFilePath = path.join( - tempRootDir, - '@new-policies', - 'sub', - 'brand-new-file.txt', - ); - const correctFilePath = path.join( - tempRootDir, - 'new-policies', - 'sub', - 'brand-new-file.txt', - ); - - // It SHOULD have created a literal "@new-policies" directory because neither exists - expect(fs.existsSync(incorrectFilePath)).toBe(true); - - // It should NOT have created the file under "new-policies/sub" - expect(fs.existsSync(correctFilePath)).toBe(false); - - // Verify the content of the created file - const createdContent = await fsp.readFile(incorrectFilePath, 'utf8'); - expect(createdContent).toContain('nested_brand_new_file = true'); - }); - - it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @/ and the first segment does NOT exist', async () => { - const writeFileTool = new WriteFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = writeFileTool.build({ - file_path: '@/new-policies-alias/sub/brand-new-file.txt', - content: '[[rule]]\nnested_brand_new_file_alias = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and create the correct file - expect(result.error).toBeUndefined(); - - const literalAtFilePath = path.join( - tempRootDir, - '@', - 'new-policies-alias', - 'sub', - 'brand-new-file.txt', - ); - const correctFilePath = path.join( - tempRootDir, - 'new-policies-alias', - 'sub', - 'brand-new-file.txt', - ); - - // It should NOT have created a literal "@" directory - expect(fs.existsSync(literalAtFilePath)).toBe(false); - expect(fs.existsSync(path.join(tempRootDir, '@'))).toBe(false); - - // It should have created the file under "new-policies-alias/sub" - expect(fs.existsSync(correctFilePath)).toBe(true); - - // Verify the content of the created file - const createdContent = await fsp.readFile(correctFilePath, 'utf8'); - expect(createdContent).toContain('nested_brand_new_file_alias = true'); - }); - - it('WriteFileTool successfully creates a new file in a nested subdirectory when the path is prefixed with @\\ and the first segment does NOT exist', async () => { - const writeFileTool = new WriteFileTool( - mockConfigInstance, - createMockMessageBus(), - ); - const invocation = writeFileTool.build({ - file_path: '@\\new-policies-alias-win\\sub\\brand-new-file.txt', - content: '[[rule]]\nnested_brand_new_file_alias_win = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and create the correct file - expect(result.error).toBeUndefined(); - - const isWindows = process.platform === 'win32'; - const literalAtFilePath = isWindows - ? path.join( - tempRootDir, - '@', - 'new-policies-alias-win', - 'sub', - 'brand-new-file.txt', - ) - : path.join( - tempRootDir, - '@\\new-policies-alias-win\\sub\\brand-new-file.txt', - ); - const correctFilePath = isWindows - ? path.join( - tempRootDir, - 'new-policies-alias-win', - 'sub', - 'brand-new-file.txt', - ) - : path.join( - tempRootDir, - 'new-policies-alias-win\\sub\\brand-new-file.txt', - ); - - // It should NOT have created a literal "@" directory - expect(fs.existsSync(literalAtFilePath)).toBe(false); - expect(fs.existsSync(path.join(tempRootDir, '@'))).toBe(false); - - // It should have created the file under "new-policies-alias-win/sub" - expect(fs.existsSync(correctFilePath)).toBe(true); - - // Verify the content of the created file - const createdContent = await fsp.readFile(correctFilePath, 'utf8'); - expect(createdContent).toContain('nested_brand_new_file_alias_win = true'); - }); - - it('getCorrectedFileContent blocks path traversal outside the workspace', async () => { - const result = await getCorrectedFileContent( - mockConfigInstance, - '../../etc/passwd', - 'malicious content', - abortSignal, - ); - - // The utility should fail with a path validation error - expect(result.error).toBeDefined(); - expect(result.error?.message).toContain('Path not in workspace'); - }); - - it('EditTool.getModifyContext blocks path traversal outside the workspace', async () => { - const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); - const modifyContext = editTool.getModifyContext(abortSignal); - - // The getCurrentContent method should throw a path validation error - await expect( - modifyContext.getCurrentContent({ - file_path: '../../etc/passwd', - instruction: 'read file', - old_string: '', - new_string: '', - }), - ).rejects.toThrow('Path not in workspace'); - - // The getProposedContent method should throw a path validation error - await expect( - modifyContext.getProposedContent({ - file_path: '../../etc/passwd', - instruction: 'read file', - old_string: '', - new_string: '', - }), - ).rejects.toThrow('Path not in workspace'); - }); - - it('getCorrectedFileContent handles symlink loops gracefully', async () => { - const symlinkPath1 = path.join(tempRootDir, 'symlink1'); - const symlinkPath2 = path.join(tempRootDir, 'symlink2'); - await fsp.symlink(symlinkPath2, symlinkPath1); - await fsp.symlink(symlinkPath1, symlinkPath2); - - const result = await getCorrectedFileContent( - mockConfigInstance, - 'symlink1', - 'content', - abortSignal, - ); - - // The utility should fail gracefully with a resolution error - expect(result.error).toBeDefined(); - expect(result.error?.message).toContain('Failed to resolve path'); - }); - - it('EditTool.getModifyContext handles symlink loops gracefully by throwing a descriptive error', async () => { - const symlinkPath1 = path.join(tempRootDir, 'symlink1'); - const symlinkPath2 = path.join(tempRootDir, 'symlink2'); - await fsp.symlink(symlinkPath2, symlinkPath1); - await fsp.symlink(symlinkPath1, symlinkPath2); - - const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); - const modifyContext = editTool.getModifyContext(abortSignal); - - // The getCurrentContent method should throw a path resolution error - await expect( - modifyContext.getCurrentContent({ - file_path: 'symlink1', - instruction: 'read file', - old_string: '', - new_string: '', - }), - ).rejects.toThrow('Failed to resolve path'); - - // The getProposedContent method should throw a path resolution error - await expect( - modifyContext.getProposedContent({ - file_path: 'symlink1', - instruction: 'read file', - old_string: '', - new_string: '', - }), - ).rejects.toThrow('Failed to resolve path'); - }); - - it('getCorrectedFileContent successfully resolves paths in Plan Mode', async () => { - const plansDir = path.join(tempRootDir, '.plans'); - await fsp.mkdir(plansDir, { recursive: true }); - await fsp.writeFile( - path.join(plansDir, 'plan-file.txt'), - 'plan content', - 'utf8', - ); - - const planConfigInstance = Object.assign({}, mockConfigInstance, { - isPlanMode: () => true, - getProjectRoot: () => tempRootDir, - storage: { - getProjectTempDir: () => path.join(tempRootDir, '.temp'), - getPlansDir: () => plansDir, - }, - }) as unknown as Config; - - const result = await getCorrectedFileContent( - planConfigInstance, - 'plan-file.txt', - 'new plan content', - abortSignal, - ); - - expect(result.error).toBeUndefined(); - expect(result.originalContent).toBe('plan content'); - }); - - it('EditTool successfully edits an existing file when the path is prefixed with @', async () => { - const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); - const invocation = editTool.build({ - file_path: '@policies/new-policies.txt', - instruction: 'update decision rule', - old_string: 'decision = "allow"', - new_string: 'decision = "deny"', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and update the correct file - expect(result.error).toBeUndefined(); - - const correctFilePath = path.join( - tempRootDir, - 'policies', - 'new-policies.txt', - ); - const updatedContent = await fsp.readFile(correctFilePath, 'utf8'); - expect(updatedContent).toContain('decision = "deny"'); - }); - - it('EditTool successfully creates a new file when the path is prefixed with @ and the parent directory exists', async () => { - const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); - const invocation = editTool.build({ - file_path: '@policies/brand-new-edit-file.txt', - instruction: 'create new file', - old_string: '', - new_string: '[[rule]]\nbrand_new_edit_file = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and create the correct file - expect(result.error).toBeUndefined(); - - const incorrectFilePath = path.join( - tempRootDir, - '@policies', - 'brand-new-edit-file.txt', - ); - const correctFilePath = path.join( - tempRootDir, - 'policies', - 'brand-new-edit-file.txt', - ); - - // It should NOT have created a literal "@policies" directory - expect(fs.existsSync(incorrectFilePath)).toBe(false); - - // It should have created the correct file under "policies" - const createdContent = await fsp.readFile(correctFilePath, 'utf8'); - expect(createdContent).toContain('brand_new_edit_file = true'); - }); - - it('EditTool successfully creates a new file in a nested subdirectory when the path is prefixed with @ and the first segment does NOT exist', async () => { - const editTool = new EditTool(mockConfigInstance, createMockMessageBus()); - const invocation = editTool.build({ - file_path: '@new-policies-edit/sub/brand-new-file.txt', - instruction: 'create new file in nested subdirectory', - old_string: '', - new_string: '[[rule]]\nnested_brand_new_edit_file = true\n', - }); - - const result = await invocation.execute({ abortSignal }); - - // The tool should succeed and create the correct file - expect(result.error).toBeUndefined(); - - const incorrectFilePath = path.join( - tempRootDir, - '@new-policies-edit', - 'sub', - 'brand-new-file.txt', - ); - const correctFilePath = path.join( - tempRootDir, - 'new-policies-edit', - 'sub', - 'brand-new-file.txt', - ); - - // It SHOULD have created a literal "@new-policies-edit" directory because neither exists - expect(fs.existsSync(incorrectFilePath)).toBe(true); - - // It should NOT have created the file under "new-policies-edit/sub" - expect(fs.existsSync(correctFilePath)).toBe(false); - - // Verify the content of the created file - const createdContent = await fsp.readFile(incorrectFilePath, 'utf8'); - expect(createdContent).toContain('nested_brand_new_edit_file = true'); - }); - - it('correctPath successfully resolves a path prefixed with @ to its clean counterpart', () => { - const result = correctPath( - '@policies/new-policies.txt', - mockConfigInstance, - ); - - expect(result.success).toBe(true); - if (result.success) { - const expectedPath = path.join( - tempRootDir, - 'policies', - 'new-policies.txt', - ); - expect(result.correctedPath).toBe(expectedPath); - } - }); -}); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 1e3bffa4e5..c00ea4c0da 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -27,12 +27,7 @@ import { import { buildFilePathArgsPattern } from '../policy/utils.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { ToolErrorType } from './tool-error.js'; -import { - makeRelative, - shortenPath, - resolveDefensiveToolPath, - resolveToRealPath, -} from '../utils/paths.js'; +import { makeRelative, shortenPath } from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; import { correctPath } from '../utils/pathCorrector.js'; import type { Config } from '../config/config.js'; @@ -483,12 +478,11 @@ class EditToolInvocation ); if (this.config.isPlanMode()) { try { - const planPath = resolveAndValidatePlanPath( + this.resolvedPath = resolveAndValidatePlanPath( this.params.file_path, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); - this.resolvedPath = resolveToRealPath(planPath); } catch (e) { debugLogger.error( 'Failed to resolve plan path during EditTool invocation setup', @@ -501,33 +495,15 @@ class EditToolInvocation } else if (!path.isAbsolute(this.params.file_path)) { const result = correctPath(this.params.file_path, this.config); if (result.success) { - try { - this.resolvedPath = resolveToRealPath(result.correctedPath); - } catch { - this.resolvedPath = result.correctedPath; - } + this.resolvedPath = result.correctedPath; } else { - const sanitizedPath = resolveDefensiveToolPath( - this.params.file_path, + this.resolvedPath = path.resolve( this.config.getTargetDir(), + this.params.file_path, ); - try { - this.resolvedPath = resolveToRealPath( - path.resolve(this.config.getTargetDir(), sanitizedPath), - ); - } catch { - this.resolvedPath = path.resolve( - this.config.getTargetDir(), - sanitizedPath, - ); - } } } else { - try { - this.resolvedPath = resolveToRealPath(this.params.file_path); - } catch { - this.resolvedPath = this.params.file_path; - } + this.resolvedPath = this.params.file_path; } } @@ -1118,43 +1094,28 @@ export class EditTool let resolvedPath: string; if (this.config.isPlanMode()) { try { - const planPath = resolveAndValidatePlanPath( + resolvedPath = resolveAndValidatePlanPath( params.file_path, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); - resolvedPath = resolveToRealPath(planPath); } catch (err) { return err instanceof Error ? err.message : String(err); } } else if (!path.isAbsolute(params.file_path)) { const result = correctPath(params.file_path, this.config); if (result.success) { - try { - resolvedPath = resolveToRealPath(result.correctedPath); - } catch (err) { - return err instanceof Error ? err.message : String(err); - } + resolvedPath = result.correctedPath; } else { - const sanitizedPath = resolveDefensiveToolPath( - params.file_path, + resolvedPath = path.resolve( this.config.getTargetDir(), + params.file_path, ); - try { - resolvedPath = resolveToRealPath( - path.resolve(this.config.getTargetDir(), sanitizedPath), - ); - } catch (err) { - return err instanceof Error ? err.message : String(err); - } } } else { - try { - resolvedPath = resolveToRealPath(params.file_path); - } catch (err) { - return err instanceof Error ? err.message : String(err); - } + resolvedPath = params.file_path; } + const newPlaceholders = detectOmissionPlaceholders(params.new_string); if (newPlaceholders.length > 0) { const oldPlaceholders = new Set( @@ -1189,65 +1150,13 @@ export class EditTool } getModifyContext(_: AbortSignal): ModifyContext { - const resolvePath = (params: EditToolParams): string => { - let pathBeforeRealResolve: string; - - try { - if (this.config.isPlanMode()) { - pathBeforeRealResolve = resolveAndValidatePlanPath( - params.file_path, - this.config.storage.getPlansDir(), - this.config.getProjectRoot(), - ); - } else if (!path.isAbsolute(params.file_path)) { - const result = correctPath(params.file_path, this.config); - if (result.success) { - pathBeforeRealResolve = result.correctedPath; - } else { - const sanitizedPath = resolveDefensiveToolPath( - params.file_path, - this.config.getTargetDir(), - ); - pathBeforeRealResolve = path.resolve( - this.config.getTargetDir(), - sanitizedPath, - ); - } - } else { - pathBeforeRealResolve = params.file_path; - } - } catch (err) { - throw new Error( - 'Failed to resolve path: ' + - (err instanceof Error ? err.message : String(err)), - ); - } - - let resolved: string; - try { - resolved = resolveToRealPath(pathBeforeRealResolve); - } catch (err) { - throw new Error( - 'Failed to resolve path: ' + - (err instanceof Error ? err.message : String(err)), - ); - } - - const validationError = this.config.validatePathAccess(resolved); - if (validationError) { - throw new Error(validationError); - } - return resolved; - }; - return { getFilePath: (params: EditToolParams) => params.file_path, getCurrentContent: async (params: EditToolParams): Promise => { try { - const resolvedPath = resolvePath(params); return await this.config .getFileSystemService() - .readTextFile(resolvedPath); + .readTextFile(params.file_path); } catch (err) { if (!isNodeError(err) || err.code !== 'ENOENT') throw err; return ''; @@ -1255,10 +1164,9 @@ export class EditTool }, getProposedContent: async (params: EditToolParams): Promise => { try { - const resolvedPath = resolvePath(params); const currentContent = await this.config .getFileSystemService() - .readTextFile(resolvedPath); + .readTextFile(params.file_path); return applyReplacement( currentContent, params.old_string, diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 47bdc86de9..778f8967eb 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -6,12 +6,7 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import path from 'node:path'; -import { - makeRelative, - shortenPath, - resolveDefensiveToolPath, - resolveToRealPath, -} from '../utils/paths.js'; +import { makeRelative, shortenPath } from '../utils/paths.js'; import { BaseDeclarativeTool, BaseToolInvocation, @@ -79,20 +74,10 @@ class ReadFileToolInvocation extends BaseToolInvocation< _toolDisplayName?: string, ) { super(params, messageBus, _toolName, _toolDisplayName); - const sanitizedPath = resolveDefensiveToolPath( - this.params.file_path, + this.resolvedPath = path.resolve( this.config.getTargetDir(), + this.params.file_path, ); - try { - this.resolvedPath = resolveToRealPath( - path.resolve(this.config.getTargetDir(), sanitizedPath), - ); - } catch { - this.resolvedPath = path.resolve( - this.config.getTargetDir(), - sanitizedPath, - ); - } } getDescription(): string { @@ -257,20 +242,11 @@ export class ReadFileTool extends BaseDeclarativeTool< return "The 'file_path' parameter must be non-empty."; } - const sanitizedPath = resolveDefensiveToolPath( - params.file_path, + const resolvedPath = path.resolve( this.config.getTargetDir(), + params.file_path, ); - let resolvedPath: string; - try { - resolvedPath = resolveToRealPath( - path.resolve(this.config.getTargetDir(), sanitizedPath), - ); - } catch (err) { - return `Failed to resolve path: ${err instanceof Error ? err.message : String(err)}`; - } - const validationError = this.config.validatePathAccess( resolvedPath, 'read', diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 94ec4f3852..9ec19b879f 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -28,12 +28,7 @@ import { } from './tools.js'; import { buildFilePathArgsPattern } from '../policy/utils.js'; import { ToolErrorType } from './tool-error.js'; -import { - makeRelative, - shortenPath, - resolveDefensiveToolPath, - resolveToRealPath, -} from '../utils/paths.js'; +import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { ensureCorrectFileContent } from '../utils/editCorrector.js'; import { detectLineEnding } from '../utils/textUtils.js'; @@ -114,67 +109,10 @@ export async function getCorrectedFileContent( let fileExists = false; let correctedContent = proposedContent; - let resolvedPath: string; - if (config.isPlanMode()) { - try { - const planPath = path.isAbsolute(filePath) - ? filePath - : resolveAndValidatePlanPath( - filePath, - config.storage.getPlansDir(), - config.getProjectRoot(), - ); - resolvedPath = resolveToRealPath(planPath); - } catch (err) { - return { - originalContent: '', - correctedContent: proposedContent, - fileExists: false, - error: { - message: - 'Failed to resolve plan path: ' + - (err instanceof Error ? err.message : String(err)), - code: 'EINVAL', - }, - }; - } - } else { - const sanitizedPath = path.isAbsolute(filePath) - ? filePath - : resolveDefensiveToolPath(filePath, config.getTargetDir()); - try { - resolvedPath = resolveToRealPath( - path.resolve(config.getTargetDir(), sanitizedPath), - ); - } catch (err) { - return { - originalContent: '', - correctedContent: proposedContent, - fileExists: false, - error: { - message: - 'Failed to resolve path: ' + - (err instanceof Error ? err.message : String(err)), - code: 'EINVAL', - }, - }; - } - } - - const validationError = config.validatePathAccess(resolvedPath); - if (validationError) { - return { - originalContent: '', - correctedContent: proposedContent, - fileExists: false, - error: { message: validationError, code: 'EACCES' }, - }; - } - try { originalContent = await config .getFileSystemService() - .readTextFile(resolvedPath); + .readTextFile(filePath); fileExists = true; // File exists and was read } catch (err) { if (isNodeError(err) && err.code === 'ENOENT') { @@ -232,12 +170,11 @@ class WriteFileToolInvocation extends BaseToolInvocation< if (this.config.isPlanMode()) { try { - const planPath = resolveAndValidatePlanPath( + this.resolvedPath = resolveAndValidatePlanPath( this.params.file_path, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); - this.resolvedPath = resolveToRealPath(planPath); } catch (e) { debugLogger.error( 'Failed to resolve plan path during WriteFileTool invocation setup', @@ -247,20 +184,10 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.resolvedPath = this.params.file_path; } } else { - const sanitizedPath = resolveDefensiveToolPath( - this.params.file_path, + this.resolvedPath = path.resolve( this.config.getTargetDir(), + this.params.file_path, ); - try { - this.resolvedPath = resolveToRealPath( - path.resolve(this.config.getTargetDir(), sanitizedPath), - ); - } catch { - this.resolvedPath = path.resolve( - this.config.getTargetDir(), - sanitizedPath, - ); - } } } @@ -598,27 +525,16 @@ export class WriteFileTool let resolvedPath: string; if (this.config.isPlanMode()) { try { - const planPath = resolveAndValidatePlanPath( + resolvedPath = resolveAndValidatePlanPath( filePath, this.config.storage.getPlansDir(), this.config.getProjectRoot(), ); - resolvedPath = resolveToRealPath(planPath); } catch (err) { return err instanceof Error ? err.message : String(err); } } else { - const sanitizedPath = resolveDefensiveToolPath( - filePath, - this.config.getTargetDir(), - ); - try { - resolvedPath = resolveToRealPath( - path.resolve(this.config.getTargetDir(), sanitizedPath), - ); - } catch (err) { - return err instanceof Error ? err.message : String(err); - } + resolvedPath = path.resolve(this.config.getTargetDir(), filePath); } const validationError = this.config.validatePathAccess(resolvedPath); diff --git a/packages/core/src/utils/pathCorrector.ts b/packages/core/src/utils/pathCorrector.ts index 632368ff20..d6538ff056 100644 --- a/packages/core/src/utils/pathCorrector.ts +++ b/packages/core/src/utils/pathCorrector.ts @@ -8,7 +8,6 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import type { Config } from '../config/config.js'; import { bfsFileSearchSync } from './bfsFileSearch.js'; -import { resolveDefensiveToolPath } from './paths.js'; type SuccessfulPathCorrection = { success: true; @@ -35,13 +34,8 @@ export function correctPath( filePath: string, config: Config, ): PathCorrectionResult { - const sanitizedPath = resolveDefensiveToolPath( - filePath, - config.getTargetDir(), - ); - // Check for direct path relative to the primary target directory. - const directPath = path.join(config.getTargetDir(), sanitizedPath); + const directPath = path.join(config.getTargetDir(), filePath); if (fs.existsSync(directPath)) { return { success: true, correctedPath: directPath }; } @@ -49,8 +43,8 @@ export function correctPath( // If not found directly, search across all workspace directories for ambiguous matches. const workspaceContext = config.getWorkspaceContext(); const searchPaths = workspaceContext.getDirectories(); - const basename = path.basename(sanitizedPath); - const normalizedTarget = sanitizedPath.replace(/\\/g, '/'); + const basename = path.basename(filePath); + const normalizedTarget = filePath.replace(/\\/g, '/'); // Normalize path for matching and check if it ends with the provided relative path const foundFiles = searchPaths diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index 0b9bdd21e1..d9d543a682 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -572,59 +572,3 @@ export function isTrustedSystemPath(filePath: string): boolean { ); } } - -/** - * Defensively resolves and sanitizes a file path generated by the LLM, - * stripping user-facing reference prefixes if necessary. - */ -export function resolveDefensiveToolPath( - filePath: string, - targetDir: string, -): string { - if (filePath.includes('\0')) { - return filePath; - } - - try { - const literalPath = path.resolve(targetDir, filePath); - - // If the file literally exists on disk as-is, return the resolved literal path immediately - if (fs.existsSync(literalPath)) { - return filePath; - } - - // If the model supplied a leading @ prefix and the literal path doesn't exist: - if (filePath.startsWith('@') && filePath.length > 1) { - if (filePath.startsWith('@/') || filePath.startsWith('@\\')) { - return filePath.substring(1).replace(/^[\\/]+/, ''); - } - - const strippedPath = filePath.substring(1).replace(/^[\\/]+/, ''); - - // Check if a literal directory/file starting with '@' exists for the first segment. - // If it does, we should preserve the '@' prefix. - const parts = strippedPath.split(/[\\/]/); - const firstSegment = parts[0]; - if (firstSegment) { - const literalFirstSegment = path.resolve(targetDir, '@' + firstSegment); - if (fs.existsSync(literalFirstSegment)) { - return filePath; - } - - // Only strip the '@' prefix if the stripped path's first segment actually exists - const strippedFirstSegment = path.resolve(targetDir, firstSegment); - if (fs.existsSync(strippedFirstSegment)) { - return strippedPath; - } - } - - // Otherwise, preserve the '@' prefix to allow creating new files/directories starting with '@' - return filePath; - } - } catch { - // Fallback to original path if any filesystem or resolution error occurs - } - - // Fallback: return the original path - return filePath; -}