diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts index ee7708e69..79e5c2e95 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts @@ -219,36 +219,39 @@ describe('handleAtCommand', () => { }); }); - it('should correctly unescape paths with escaped spaces', async () => { - const fileContent = 'This is the file content.'; - const filePath = await createTestFile( - path.join(testRootDir, 'path', 'to', 'my file.txt'), - fileContent, - ); - const escapedpath = path.join(testRootDir, 'path', 'to', 'my\\ file.txt'); - const query = `@${escapedpath}`; + it.skipIf(process.platform === 'win32')( + 'should correctly unescape paths with escaped spaces', + async () => { + const fileContent = 'This is the file content.'; + const filePath = await createTestFile( + path.join(testRootDir, 'path', 'to', 'my file.txt'), + fileContent, + ); + const escapedpath = path.join(testRootDir, 'path', 'to', 'my\\ file.txt'); + const query = `@${escapedpath}`; - const result = await handleAtCommand({ - query, - config: mockConfig, - onDebugMessage: mockOnDebugMessage, - messageId: 125, - signal: abortController.signal, - }); + const result = await handleAtCommand({ + query, + config: mockConfig, + onDebugMessage: mockOnDebugMessage, + messageId: 125, + signal: abortController.signal, + }); - expect(result.processedQuery).toEqual([ - { text: `@${filePath}` }, - { text: '\n--- Content from referenced files ---' }, - { text: `\nContent from ${filePath}:\n` }, - { text: fileContent }, - { text: '\n--- End of content ---' }, - ]); - expect(result.shouldProceed).toBe(true); - // toolDisplays should be returned for caller to add to UI history - expect(result.toolDisplays).toBeDefined(); - expect(result.toolDisplays).toHaveLength(1); - expect(result.toolDisplays![0].status).toBe(ToolCallStatus.Success); - }); + expect(result.processedQuery).toEqual([ + { text: `@${filePath}` }, + { text: '\n--- Content from referenced files ---' }, + { text: `\nContent from ${filePath}:\n` }, + { text: fileContent }, + { text: '\n--- End of content ---' }, + ]); + expect(result.shouldProceed).toBe(true); + // toolDisplays should be returned for caller to add to UI history + expect(result.toolDisplays).toBeDefined(); + expect(result.toolDisplays).toHaveLength(1); + expect(result.toolDisplays![0].status).toBe(ToolCallStatus.Success); + }, + ); it('should handle multiple @file references', async () => { const content1 = 'Content file1'; @@ -782,34 +785,37 @@ describe('handleAtCommand', () => { }); }); - it('should still handle escaped spaces in paths before punctuation', async () => { - const fileContent = 'Spaced file content'; - const filePath = await createTestFile( - path.join(testRootDir, 'spaced file.txt'), - fileContent, - ); - const escapedPath = path.join(testRootDir, 'spaced\\ file.txt'); - const query = `Check @${escapedPath}, it has spaces.`; + it.skipIf(process.platform === 'win32')( + 'should still handle escaped spaces in paths before punctuation', + async () => { + const fileContent = 'Spaced file content'; + const filePath = await createTestFile( + path.join(testRootDir, 'spaced file.txt'), + fileContent, + ); + const escapedPath = path.join(testRootDir, 'spaced\\ file.txt'); + const query = `Check @${escapedPath}, it has spaces.`; - const result = await handleAtCommand({ - query, - config: mockConfig, - onDebugMessage: mockOnDebugMessage, - messageId: 412, - signal: abortController.signal, - }); + const result = await handleAtCommand({ + query, + config: mockConfig, + onDebugMessage: mockOnDebugMessage, + messageId: 412, + signal: abortController.signal, + }); - expect(result).toMatchObject({ - processedQuery: [ - { text: `Check @${filePath}, it has spaces.` }, - { text: '\n--- Content from referenced files ---' }, - { text: `\nContent from ${filePath}:\n` }, - { text: fileContent }, - { text: '\n--- End of content ---' }, - ], - shouldProceed: true, - }); - }); + expect(result).toMatchObject({ + processedQuery: [ + { text: `Check @${filePath}, it has spaces.` }, + { text: '\n--- Content from referenced files ---' }, + { text: `\nContent from ${filePath}:\n` }, + { text: fileContent }, + { text: '\n--- End of content ---' }, + ], + shouldProceed: true, + }); + }, + ); it('should not break file paths with periods in extensions', async () => { const fileContent = 'TypeScript content'; diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 8d2f0ba3f..4ddc8c7b1 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -50,6 +50,7 @@ import type { import { fileURLToPath } from 'node:url'; import { ToolNames, ToolNamesMigration } from '../tools/tool-names.js'; import { escapeXml } from '../utils/xml.js'; +import { unescapePath, PATH_ARG_KEYS } from '../utils/paths.js'; import { CONCURRENCY_SAFE_KINDS } from '../tools/tools.js'; import { isShellCommandReadOnly } from '../utils/shellReadOnlyChecker.js'; import { stripShellWrapper } from '../utils/shell-utils.js'; @@ -1532,10 +1533,23 @@ export class CoreToolScheduler { isModifying: true, } as ToolCallConfirmationDetails); + // Normalize shell-escaped paths so the editor receives actual + // filesystem paths (request.args may still hold escaped values + // since buildInvocation normalizes a structuredClone). + const normalizedArgs = { + ...waitingToolCall.request.args, + } as typeof waitingToolCall.request.args; + for (const key of PATH_ARG_KEYS) { + if (typeof normalizedArgs[key] === 'string') { + (normalizedArgs as Record)[key] = unescapePath( + String(normalizedArgs[key]).trim(), + ); + } + } const { updatedParams, updatedDiff } = await modifyWithEditor< typeof waitingToolCall.request.args >( - waitingToolCall.request.args, + normalizedArgs, modifyContext as ModifyContext, editorType, signal, @@ -1746,6 +1760,14 @@ export class CoreToolScheduler { const invocation = scheduledCall.invocation; const toolInput = scheduledCall.request.args as Record; + // Normalize shell-escaped path params so hooks operate on actual filesystem + // paths, matching the normalization done in tool validation. + for (const key of PATH_ARG_KEYS) { + if (typeof toolInput[key] === 'string') { + toolInput[key] = unescapePath(String(toolInput[key]).trim()); + } + } + // Generate unique tool_use_id for hook tracking const toolUseId = generateToolUseId(); @@ -1908,7 +1930,9 @@ export class CoreToolScheduler { // FS_PATH_TOOL_NAMES) so MCP / non-FS tools that reuse those // parameter names with different semantics never enter the // activation pipeline. - const candidatePaths = extractToolFilePaths(toolName, toolInput); + const candidatePaths = extractToolFilePaths(toolName, toolInput).map( + (p) => unescapePath(p), + ); if (candidatePaths.length > 0) { const rulesRegistry = this.config.getConditionalRulesRegistry(); diff --git a/packages/core/src/followup/speculationToolGate.ts b/packages/core/src/followup/speculationToolGate.ts index f7dd402cd..6a1e61ac6 100644 --- a/packages/core/src/followup/speculationToolGate.ts +++ b/packages/core/src/followup/speculationToolGate.ts @@ -18,6 +18,7 @@ import { ToolNames } from '../tools/tool-names.js'; import { isShellCommandReadOnlyAST } from '../utils/shellAstParser.js'; import { ApprovalMode } from '../config/config.js'; +import { unescapePath, PATH_ARG_KEYS } from '../utils/paths.js'; import type { OverlayFs } from './overlayFs.js'; export interface ToolGateResult { @@ -117,10 +118,11 @@ async function resolveReadPaths( args: Record, overlayFs: OverlayFs, ): Promise { - const pathKeys = ['file_path', 'filePath', 'path', 'notebook_path']; - for (const key of pathKeys) { + for (const key of PATH_ARG_KEYS) { if (typeof args[key] === 'string') { - args[key] = overlayFs.resolveReadPath(args[key] as string); + args[key] = overlayFs.resolveReadPath( + unescapePath(String(args[key]).trim()), + ); return; } } @@ -134,11 +136,11 @@ export async function rewritePathArgs( args: Record, overlayFs: OverlayFs, ): Promise { - // Common path argument names used by Edit and WriteFile tools - const pathKeys = ['file_path', 'filePath', 'path', 'notebook_path']; - for (const key of pathKeys) { + for (const key of PATH_ARG_KEYS) { if (typeof args[key] === 'string') { - args[key] = await overlayFs.redirectWrite(args[key] as string); + args[key] = await overlayFs.redirectWrite( + unescapePath(String(args[key]).trim()), + ); return; } } diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 2b78956d9..2c890394f 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -233,6 +233,60 @@ describe('EditTool', () => { const error = tool.validateToolParams(params); expect(error).toBeNull(); }); + + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped spaces in file_path', + () => { + const escapedPath = path.join(rootDir, 'my\\ file.txt'); + const params: EditToolParams = { + file_path: escapedPath, + old_string: 'old', + new_string: 'new', + }; + expect(tool.validateToolParams(params)).toBeNull(); + expect(params.file_path).toBe(path.join(rootDir, 'my file.txt')); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'should unescape multiple shell-escaped characters in file_path', + () => { + const escapedPath = path.join( + rootDir, + 'project\\ \\(v2\\)\\ \\&\\ more.txt', + ); + const params: EditToolParams = { + file_path: escapedPath, + old_string: 'old', + new_string: 'new', + }; + expect(tool.validateToolParams(params)).toBeNull(); + expect(params.file_path).toBe( + path.join(rootDir, 'project (v2) & more.txt'), + ); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'should preserve literal backslashes in file_path', + () => { + // On Windows, backslashes are path separators, and unescapePath is a + // no-op. This test only validates literal-backslash preservation on + // platforms where backslashes are not path separators. + const pathWithBackslash = path.join( + rootDir, + 'path\\\\with\\\\slashes.txt', + ); + const params: EditToolParams = { + file_path: pathWithBackslash, + old_string: 'old', + new_string: 'new', + }; + expect(tool.validateToolParams(params)).toBeNull(); + // Double backslashes (literal) should be preserved + expect(params.file_path).toBe(pathWithBackslash); + }, + ); }); describe('getConfirmationDetails', () => { @@ -905,6 +959,52 @@ describe('EditTool', () => { }); }); + describe.skipIf(process.platform === 'win32')( + 'escaped paths with spaces (end-to-end)', + () => { + it('should read and edit a file whose name contains spaces when given an escaped path', async () => { + // Create a file with spaces in its name on disk + const realFileName = 'my spaced file.txt'; + const realPath = path.join(rootDir, realFileName); + fs.writeFileSync(realPath, 'Hello old world!', 'utf8'); + + // Pass an ESCAPED path (as the LLM might from at-completion) + const escapedPath = path.join(rootDir, 'my\\ spaced\\ file.txt'); + const params: EditToolParams = { + file_path: escapedPath, + old_string: 'old', + new_string: 'new', + }; + + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); + + // Should succeed — not fail with file-not-found + expect(result.llmContent).toMatch(/Showing lines \d+-\d+ of \d+/); + expect(fs.readFileSync(realPath, 'utf8')).toBe('Hello new world!'); + }); + + it('should fail gracefully when escaped path points to nonexistent file', async () => { + const escapedPath = path.join(rootDir, 'nonexistent\\ file.txt'); + const params: EditToolParams = { + file_path: escapedPath, + old_string: 'old', + new_string: 'new', + }; + + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); + + // Should report file-not-found (unescaped path used, file truly doesn't exist) + expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); + }); + }, + ); + describe('workspace boundary validation', () => { it('should validate paths are within workspace root', () => { const validPath = { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index fef091bf6..567c63a71 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -17,7 +17,7 @@ import type { import type { PermissionDecision } from '../permissions/types.js'; import { BaseDeclarativeTool, Kind, ToolConfirmationOutcome } from './tools.js'; import { ToolErrorType } from './tool-error.js'; -import { makeRelative, shortenPath } from '../utils/paths.js'; +import { makeRelative, shortenPath, unescapePath } from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../config/config.js'; @@ -591,6 +591,10 @@ Expectation for required parameters: protected override validateToolParamValues( params: EditToolParams, ): string | null { + // Normalize shell-escaped paths (e.g. "my\ file.txt" → "my file.txt") + // that may reach the LLM via at-completion or manual typing. + params.file_path = unescapePath(params.file_path.trim()); + if (!params.file_path) { return "The 'file_path' parameter must be non-empty."; } diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 566c4c32b..448953ee1 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -361,6 +361,22 @@ describe('GlobTool', () => { 'Path is not a directory', ); }); + + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped path', + async () => { + // Create a directory with a space so the unescaped path exists + const dirWithSpace = path.join(tempRootDir, 'sub dir'); + await fs.mkdir(dirWithSpace); + const params: GlobToolParams = { + pattern: '*.ts', + path: path.join(tempRootDir, 'sub\\ dir'), + }; + expect(globTool.validateToolParams(params)).toBeNull(); + // Path should be normalized in place + expect(params.path).toBe(dirWithSpace); + }, + ); }); describe('workspace boundary validation', () => { diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 5e13e5b1a..a352e8706 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -10,7 +10,11 @@ import { glob, escape } from 'glob'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; -import { resolveAndValidatePath, isSubpath } from '../utils/paths.js'; +import { + resolveAndValidatePath, + isSubpath, + unescapePath, +} from '../utils/paths.js'; import { getMemoryBaseDir } from '../memory/paths.js'; import { type Config } from '../config/config.js'; import type { PermissionDecision } from '../permissions/types.js'; @@ -348,6 +352,7 @@ export class GlobTool extends BaseDeclarativeTool { // Only validate path if one is provided if (params.path) { + params.path = unescapePath(params.path.trim()); try { resolveAndValidatePath(this.config, params.path, { allowExternalPaths: true, diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 4af6d8d7c..14da1223a 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -174,6 +174,21 @@ describe('GrepTool', () => { `Path is not a directory: ${filePath}`, ); }); + + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped path', + async () => { + // Create a directory with a space so the unescaped path exists + const dirWithSpace = path.join(tempRootDir, 'sub dir'); + await fs.mkdir(dirWithSpace); + const params: GrepToolParams = { + pattern: 'hello', + path: path.join(tempRootDir, 'sub\\ dir'), + }; + expect(grepTool.validateToolParams(params)).toBeNull(); + expect(params.path).toBe(dirWithSpace); + }, + ); }); describe('execute', () => { diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 53500022f..f61f66bf0 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -13,9 +13,12 @@ import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { createDebugLogger } from '../utils/debugLogger.js'; +import { + resolveAndValidatePath, + isSubpath, + unescapePath, +} from '../utils/paths.js'; -const debugLogger = createDebugLogger('GREP'); -import { resolveAndValidatePath, isSubpath } from '../utils/paths.js'; import { getMemoryBaseDir } from '../memory/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { isGitRepository } from '../utils/gitUtils.js'; @@ -25,6 +28,8 @@ import type { FileExclusions } from '../utils/ignorePatterns.js'; import { ToolErrorType } from './tool-error.js'; import { isCommandAvailable } from '../utils/shell-utils.js'; +const debugLogger = createDebugLogger('GREP'); + // --- Interfaces --- /** @@ -615,6 +620,7 @@ export class GrepTool extends BaseDeclarativeTool { // Only validate path if one is provided if (params.path) { + params.path = unescapePath(params.path.trim()); try { resolveAndValidatePath(this.config, params.path, { allowExternalPaths: true, diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index 445092a6d..f9552de60 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -9,6 +9,7 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import os from 'node:os'; import { LSTool } from './ls.js'; +import type { LSToolParams } from './ls.js'; import type { Config } from '../config/config.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import { ToolErrorType } from './tool-error.js'; @@ -393,4 +394,21 @@ describe('LSTool', () => { expect(result.returnDisplay).toBe('Listed 1 item(s)'); }); }); + + describe('validateToolParams', () => { + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped path', + async () => { + // Create a directory with a space so the unescaped path exists + const dirWithSpace = path.join(tempRootDir, 'sub dir'); + await fs.mkdir(dirWithSpace); + const params: LSToolParams = { + path: path.join(tempRootDir, 'sub\\ dir'), + }; + const result = lsTool.validateToolParams(params); + expect(result).toBeNull(); + expect(params.path).toBe(dirWithSpace); + }, + ); + }); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 2c2c1eb17..ceb147ba6 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -8,8 +8,13 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import { makeRelative, shortenPath } from '../utils/paths.js'; -import { isSubpaths, isSubpath } from '../utils/paths.js'; +import { + makeRelative, + shortenPath, + unescapePath, + isSubpaths, + isSubpath, +} from '../utils/paths.js'; import type { Config } from '../config/config.js'; import type { PermissionDecision } from '../permissions/types.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; @@ -355,6 +360,8 @@ export class LSTool extends BaseDeclarativeTool { protected override validateToolParamValues( params: LSToolParams, ): string | null { + params.path = unescapePath(params.path.trim()); + if (!path.isAbsolute(params.path)) { return `Path must be absolute: ${params.path}`; } diff --git a/packages/core/src/tools/lsp.test.ts b/packages/core/src/tools/lsp.test.ts index a74f5453c..d045ddcb3 100644 --- a/packages/core/src/tools/lsp.test.ts +++ b/packages/core/src/tools/lsp.test.ts @@ -277,6 +277,21 @@ describe('LspTool', () => { } as LspToolParams); expect(result).toBe('query is required for workspaceSymbol.'); }); + + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped filePath', + () => { + const params: LspToolParams = { + operation: 'goToDefinition', + filePath: 'src/app\\ file.ts', + line: 10, + character: 5, + }; + const result = tool.validateToolParams(params); + expect(result).toBeNull(); + expect(params.filePath).toBe('src/app file.ts'); + }, + ); }); }); diff --git a/packages/core/src/tools/lsp.ts b/packages/core/src/tools/lsp.ts index 27711a080..aba81d579 100644 --- a/packages/core/src/tools/lsp.ts +++ b/packages/core/src/tools/lsp.ts @@ -9,6 +9,7 @@ import { fileURLToPath, pathToFileURL } from 'node:url'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { ToolDisplayNames, ToolNames } from './tool-names.js'; +import { unescapePath } from '../utils/paths.js'; import type { Config } from '../config/config.js'; import type { LspCallHierarchyIncomingCall, @@ -1155,8 +1156,13 @@ export class LspTool extends BaseDeclarativeTool { ): string | null { const operation = params.operation; + // Normalize shell-escaped paths (e.g. "my\ file.txt" → "my file.txt") + if (params.filePath) { + params.filePath = unescapePath(params.filePath.trim()); + } + if (LOCATION_REQUIRED_OPERATIONS.has(operation)) { - if (!params.filePath || params.filePath.trim() === '') { + if (!params.filePath) { return `filePath is required for ${operation}.`; } if (typeof params.line !== 'number') { @@ -1165,7 +1171,7 @@ export class LspTool extends BaseDeclarativeTool { } if (FILE_REQUIRED_OPERATIONS.has(operation)) { - if (!params.filePath || params.filePath.trim() === '') { + if (!params.filePath) { return `filePath is required for ${operation}.`; } } @@ -1183,7 +1189,7 @@ export class LspTool extends BaseDeclarativeTool { } if (RANGE_REQUIRED_OPERATIONS.has(operation)) { - if (!params.filePath || params.filePath.trim() === '') { + if (!params.filePath) { return `filePath is required for ${operation}.`; } if (typeof params.line !== 'number') { diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 19bacd3e2..00ed0e8ce 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -82,6 +82,21 @@ describe('ReadFileTool', () => { ); }); + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped spaces in file_path', + () => { + const escapedPath = path.join(tempRootDir, 'my\\ file.txt'); + const params: ReadFileToolParams = { + file_path: escapedPath, + }; + const invocation = tool.build(params); + expect(invocation).toBeDefined(); + expect(invocation.params.file_path).toBe( + path.join(tempRootDir, 'my file.txt'), + ); + }, + ); + it('should allow path outside root (external path support)', () => { const params: ReadFileToolParams = { file_path: '/outside/root.txt', @@ -268,6 +283,29 @@ describe('ReadFileTool', () => { }); }); + it.skipIf(process.platform === 'win32')( + 'should read a file with spaces in its name when given an escaped path', + async () => { + const realFileName = 'my spaced read.txt'; + const realPath = path.join(tempRootDir, realFileName); + const fileContent = 'Content with spaces in filename.'; + await fsp.writeFile(realPath, fileContent, 'utf-8'); + + // Pass an ESCAPED path (as the LLM might from at-completion) + const escapedPath = path.join(tempRootDir, 'my\\ spaced\\ read.txt'); + const params: ReadFileToolParams = { file_path: escapedPath }; + const invocation = tool.build(params) as ToolInvocation< + ReadFileToolParams, + ToolResult + >; + + expect(await invocation.execute(abortSignal)).toEqual({ + llmContent: fileContent, + returnDisplay: '', + }); + }, + ); + it('should return error if path is a directory', async () => { const dirPath = path.join(tempRootDir, 'directory'); await fsp.mkdir(dirPath); diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index bfb4aeabc..099e22a9c 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -8,7 +8,7 @@ import os from 'node:os'; import path from 'node:path'; import fs from 'node:fs/promises'; import type { Stats } from 'node:fs'; -import { makeRelative, shortenPath } from '../utils/paths.js'; +import { makeRelative, shortenPath, unescapePath } from '../utils/paths.js'; import type { ToolInvocation, ToolLocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; @@ -383,8 +383,12 @@ export class ReadFileTool extends BaseDeclarativeTool< protected override validateToolParamValues( params: ReadFileToolParams, ): string | null { - const filePath = params.file_path; - if (params.file_path.trim() === '') { + // Normalize shell-escaped paths (e.g. "my\ file.txt" → "my file.txt") + // that may reach the LLM via at-completion or manual typing. + const filePath = unescapePath(params.file_path.trim()); + params.file_path = filePath; + + if (!filePath) { return "The 'file_path' parameter must be non-empty."; } diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 5ed8d0e78..c02ccdee7 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -140,6 +140,21 @@ describe('RipGrepTool', () => { const params: RipGrepToolParams = { pattern: 'hello', path: filePath }; expect(grepTool.validateToolParams(params)).toBeNull(); }); + + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped path', + async () => { + // Create a directory with a space so the unescaped path exists + const dirWithSpace = path.join(tempRootDir, 'sub dir'); + await fs.mkdir(dirWithSpace); + const params: RipGrepToolParams = { + pattern: 'hello', + path: path.join(tempRootDir, 'sub\\ dir'), + }; + expect(grepTool.validateToolParams(params)).toBeNull(); + expect(params.path).toBe(dirWithSpace); + }, + ); }); describe('execute', () => { diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index b0d01aa4c..1c218b019 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -9,7 +9,7 @@ import path from 'node:path'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { ToolNames } from './tool-names.js'; -import { resolveAndValidatePath } from '../utils/paths.js'; +import { resolveAndValidatePath, unescapePath } from '../utils/paths.js'; import { getErrorMessage } from '../utils/errors.js'; import type { Config } from '../config/config.js'; import { runRipgrep } from '../utils/ripgrepUtils.js'; @@ -424,6 +424,7 @@ export class RipGrepTool extends BaseDeclarativeTool< // Only validate path if one is provided if (params.path) { + params.path = unescapePath(params.path.trim()); try { resolveAndValidatePath(this.config, params.path, { allowFiles: true, diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index e5f972b00..3a1de60b0 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -178,6 +178,24 @@ describe('WriteFileTool', () => { }; expect(() => tool.build(params)).toThrow(`Missing or empty "file_path"`); }); + + it.skipIf(process.platform === 'win32')( + 'should unescape shell-escaped spaces in file_path', + () => { + // On Windows, unescapePath is a no-op and backslashes are path + // separators, so the expected unescape behavior doesn't apply. + const escapedPath = path.join(rootDir, 'my\\ file.txt'); + const params = { + file_path: escapedPath, + content: 'hello', + }; + const invocation = tool.build(params); + expect(invocation).toBeDefined(); + expect(invocation.params.file_path).toBe( + path.join(rootDir, 'my file.txt'), + ); + }, + ); }); describe('shouldConfirmExecute', () => { @@ -458,6 +476,37 @@ describe('WriteFileTool', () => { expect(result.llmContent).not.toMatch(/User modified the `content`/); }); + + it.skipIf(process.platform === 'win32')( + 'should write to a file with spaces in its name when given an escaped path', + async () => { + // On Windows, unescapePath is a no-op and backslashes are path + // separators, so shell-escaping behavior doesn't apply. + const realPath = path.join(rootDir, 'my spaced write.txt'); + const escapedPath = path.join(rootDir, 'my\\ spaced\\ write.txt'); + const content = 'Written via escaped path.'; + + const params = { file_path: escapedPath, content }; + const invocation = tool.build(params); + + const confirmDetails = + await invocation.getConfirmationDetails(abortSignal); + if ( + typeof confirmDetails === 'object' && + 'onConfirm' in confirmDetails && + confirmDetails.onConfirm + ) { + await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); + } + + const result = await invocation.execute(abortSignal); + + // Should succeed — file created at the unescaped (real) path + expect(result.llmContent).toMatch(/Successfully created and wrote/); + expect(fs.existsSync(realPath)).toBe(true); + expect(fs.readFileSync(realPath, 'utf8')).toBe(content); + }, + ); }); describe('workspace boundary validation', () => { diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 0e7c979f7..1134920f8 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -32,7 +32,7 @@ import { detectLineEnding, } from '../services/fileSystemService.js'; import type { LineEnding } from '../services/fileSystemService.js'; -import { makeRelative, shortenPath } from '../utils/paths.js'; +import { makeRelative, shortenPath, unescapePath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; @@ -404,7 +404,10 @@ export class WriteFileTool protected override validateToolParamValues( params: WriteFileToolParams, ): string | null { - const filePath = params.file_path; + // Normalize shell-escaped paths (e.g. "my\ file.txt" → "my file.txt") + // that may reach the LLM via at-completion or manual typing. + const filePath = unescapePath(params.file_path.trim()); + params.file_path = filePath; if (!filePath) { return `Missing or empty "file_path"`; diff --git a/packages/core/src/utils/filesearch/fileSearch.test.ts b/packages/core/src/utils/filesearch/fileSearch.test.ts index 265e9cfc9..aa094039f 100644 --- a/packages/core/src/utils/filesearch/fileSearch.test.ts +++ b/packages/core/src/utils/filesearch/fileSearch.test.ts @@ -642,35 +642,38 @@ describe('FileSearch', () => { expect(limitedResults).toEqual(['file1.js', 'file2.js']); }); - it('should handle file paths with special characters that need escaping', async () => { - tmpDir = await createTmpDir({ - src: { - 'file with (special) chars.txt': '', - 'another-file.txt': '', - }, - }); + it.skipIf(process.platform === 'win32')( + 'should handle file paths with special characters that need escaping', + async () => { + tmpDir = await createTmpDir({ + src: { + 'file with (special) chars.txt': '', + 'another-file.txt': '', + }, + }); - const fileSearch = FileSearchFactory.create({ - projectRoot: tmpDir, - useGitignore: false, - useQwenignore: false, - ignoreDirs: [], - cache: false, - cacheTtl: 0, - enableRecursiveFileSearch: true, - enableFuzzySearch: true, - }); + const fileSearch = FileSearchFactory.create({ + projectRoot: tmpDir, + useGitignore: false, + useQwenignore: false, + ignoreDirs: [], + cache: false, + cacheTtl: 0, + enableRecursiveFileSearch: true, + enableFuzzySearch: true, + }); - await fileSearch.initialize(); + await fileSearch.initialize(); - // Search for the file using a pattern that contains special characters. - // The `unescapePath` function should handle the escaped path correctly. - const results = await fileSearch.search( - 'src/file with \\(special\\) chars.txt', - ); + // Search for the file using a pattern that contains special characters. + // The `unescapePath` function should handle the escaped path correctly. + const results = await fileSearch.search( + 'src/file with \\(special\\) chars.txt', + ); - expect(results).toEqual(['src/file with (special) chars.txt']); - }); + expect(results).toEqual(['src/file with (special) chars.txt']); + }, + ); describe('DirectoryFileSearch', () => { it('should search for files in the current directory', async () => { diff --git a/packages/core/src/utils/paths.test.ts b/packages/core/src/utils/paths.test.ts index c8b5c0343..ab6c20479 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/packages/core/src/utils/paths.test.ts @@ -192,81 +192,97 @@ describe('escapePath', () => { }); describe('unescapePath', () => { - it('should unescape spaces', () => { - expect(unescapePath('my\\ file.txt')).toBe('my file.txt'); - }); + const isWindows = process.platform === 'win32'; - it('should unescape tabs', () => { - expect(unescapePath('file\\\twith\\\ttabs.txt')).toBe( - 'file\twith\ttabs.txt', + // On Windows, backslashes are path separators, not shell escape chars. + // unescapePath is intentionally a no-op on win32. + it.skipIf(!isWindows)('should be a no-op on Windows', () => { + expect(unescapePath('C:\\Users\\my file.txt')).toBe( + 'C:\\Users\\my file.txt', + ); + expect(unescapePath('C:\\(v2)\\file.txt')).toBe('C:\\(v2)\\file.txt'); + expect(unescapePath('path\\to\\file\\ name.txt')).toBe( + 'path\\to\\file\\ name.txt', ); }); - it('should unescape parentheses', () => { - expect(unescapePath('file\\(1\\).txt')).toBe('file(1).txt'); - }); - - it('should unescape square brackets', () => { - expect(unescapePath('file\\[backup\\].txt')).toBe('file[backup].txt'); - }); - - it('should unescape curly braces', () => { - expect(unescapePath('file\\{temp\\}.txt')).toBe('file{temp}.txt'); - }); - - it('should unescape multiple special characters', () => { - expect(unescapePath('my\\ file\\ \\(backup\\)\\ \\[v1.2\\].txt')).toBe( - 'my file (backup) [v1.2].txt', - ); - }); - - it('should handle paths without escaped characters', () => { - expect(unescapePath('normalfile.txt')).toBe('normalfile.txt'); - expect(unescapePath('path/to/normalfile.txt')).toBe( - 'path/to/normalfile.txt', - ); - }); - - it('should handle all special characters', () => { - expect( - unescapePath( - '\\ \\(\\)\\[\\]\\{\\}\\;\\&\\|\\*\\?\\$\\`\\\'\\"\\#\\!\\~\\<\\>', - ), - ).toBe(' ()[]{};&|*?$`\'"#!~<>'); - }); - - it('should be the inverse of escapePath', () => { - const testCases = [ - 'my file.txt', - 'file(1).txt', - 'file[backup].txt', - 'My Documents/Project (2024)/file [backup].txt', - 'file with $special &chars!.txt', - ' ()[]{};&|*?$`\'"#!~<>', - 'file\twith\ttabs.txt', - ]; - - testCases.forEach((testCase) => { - expect(unescapePath(escapePath(testCase))).toBe(testCase); + describe.skipIf(isWindows)('on Unix', () => { + it('should unescape spaces', () => { + expect(unescapePath('my\\ file.txt')).toBe('my file.txt'); }); - }); - it('should handle empty strings', () => { - expect(unescapePath('')).toBe(''); - }); + it('should unescape tabs', () => { + expect(unescapePath('file\\\twith\\\ttabs.txt')).toBe( + 'file\twith\ttabs.txt', + ); + }); - it('should not affect backslashes not followed by special characters', () => { - expect(unescapePath('file\\name.txt')).toBe('file\\name.txt'); - expect(unescapePath('path\\to\\file.txt')).toBe('path\\to\\file.txt'); - }); + it('should unescape parentheses', () => { + expect(unescapePath('file\\(1\\).txt')).toBe('file(1).txt'); + }); - it('should handle escaped backslashes in unescaping', () => { - // Should correctly unescape when there are escaped backslashes - expect(unescapePath('path\\\\\\ file.txt')).toBe('path\\\\ file.txt'); - expect(unescapePath('path\\\\\\\\\\ file.txt')).toBe( - 'path\\\\\\\\ file.txt', - ); - expect(unescapePath('file\\\\\\(test\\).txt')).toBe('file\\\\(test).txt'); + it('should unescape square brackets', () => { + expect(unescapePath('file\\[backup\\].txt')).toBe('file[backup].txt'); + }); + + it('should unescape curly braces', () => { + expect(unescapePath('file\\{temp\\}.txt')).toBe('file{temp}.txt'); + }); + + it('should unescape multiple special characters', () => { + expect(unescapePath('my\\ file\\ \\(backup\\)\\ \\[v1.2\\].txt')).toBe( + 'my file (backup) [v1.2].txt', + ); + }); + + it('should handle paths without escaped characters', () => { + expect(unescapePath('normalfile.txt')).toBe('normalfile.txt'); + expect(unescapePath('path/to/normalfile.txt')).toBe( + 'path/to/normalfile.txt', + ); + }); + + it('should handle all special characters', () => { + expect( + unescapePath( + '\\ \\(\\)\\[\\]\\{\\}\\;\\&\\|\\*\\?\\$\\`\\\'\\"\\#\\!\\~\\<\\>', + ), + ).toBe(' ()[]{};&|*?$`\'"#!~<>'); + }); + + it('should be the inverse of escapePath', () => { + const testCases = [ + 'my file.txt', + 'file(1).txt', + 'file[backup].txt', + 'My Documents/Project (2024)/file [backup].txt', + 'file with $special &chars!.txt', + ' ()[]{};&|*?$`\'"#!~<>', + 'file\twith\ttabs.txt', + ]; + + testCases.forEach((testCase) => { + expect(unescapePath(escapePath(testCase))).toBe(testCase); + }); + }); + + it('should handle empty strings', () => { + expect(unescapePath('')).toBe(''); + }); + + it('should not affect backslashes not followed by special characters', () => { + expect(unescapePath('file\\name.txt')).toBe('file\\name.txt'); + expect(unescapePath('path\\to\\file.txt')).toBe('path\\to\\file.txt'); + }); + + it('should handle escaped backslashes in unescaping', () => { + // Should correctly unescape when there are escaped backslashes + expect(unescapePath('path\\\\\\ file.txt')).toBe('path\\\\ file.txt'); + expect(unescapePath('path\\\\\\\\\\ file.txt')).toBe( + 'path\\\\\\\\ file.txt', + ); + expect(unescapePath('file\\\\\\(test\\).txt')).toBe('file\\\\(test).txt'); + }); }); }); diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index 0a75d7048..ef858cdfa 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -46,6 +46,22 @@ export function _resetValidatePathCacheForTest(): void { */ export const SHELL_SPECIAL_CHARS = /[ \t()[\]{};|*?$`'"#&<>!~]/; +// Single shared list of path-argument keys used across file tools. +// file_path (Edit, ReadFile, WriteFile), path (Glob, Grep, Ls, RipGrep), +// filePath (Lsp), notebook_path. +export const PATH_ARG_KEYS = [ + 'file_path', + 'path', + 'filePath', + 'notebook_path', +] as const; + +/** Compiled regex for unescapePath — hoisted to avoid re-compilation per call. */ +const UNESCAPE_REGEX = (() => { + const inner = SHELL_SPECIAL_CHARS.source.slice(1, -1); + return new RegExp(`\\\\([${inner}])`, 'g'); +})(); + /** * Replaces the home directory with a tilde. * @param path - The path to tildeify. @@ -205,12 +221,17 @@ export function escapePath(filePath: string): string { /** * Unescapes special characters in a file path. * Removes backslash escaping from shell metacharacters. + * + * On Windows, backslashes are path separators, not shell escape characters + * (PowerShell uses backtick, cmd.exe uses caret). Skipping unescaping on + * win32 avoids corrupting valid absolute paths like C:\(v2)\file.txt. */ export function unescapePath(filePath: string): string { - return filePath.replace( - new RegExp(`\\\\([${SHELL_SPECIAL_CHARS.source.slice(1, -1)}])`, 'g'), - '$1', - ); + if (os.platform() === 'win32') { + return filePath; + } + const unescaped = filePath.replace(UNESCAPE_REGEX, '$1'); + return unescaped; } /** diff --git a/packages/core/src/utils/rulesDiscovery.test.ts b/packages/core/src/utils/rulesDiscovery.test.ts index 81735e4ca..f46f81385 100644 --- a/packages/core/src/utils/rulesDiscovery.test.ts +++ b/packages/core/src/utils/rulesDiscovery.test.ts @@ -13,7 +13,7 @@ import { loadRules, ConditionalRulesRegistry, } from './rulesDiscovery.js'; -import { QWEN_DIR } from './paths.js'; +import { QWEN_DIR, unescapePath } from './paths.js'; vi.mock('os', async (importOriginal) => { const actualOs = await importOriginal(); @@ -457,5 +457,24 @@ Use hooks.`, reg.matchAndConsume('/totally/other/place/file.ts'), ).toBeUndefined(); }); + + it.skipIf(process.platform === 'win32')( + 'should match shell-escaped file paths after unescaping', + () => { + // On Windows, unescapePath is a no-op (backslash is a path + // separator, not a shell escape character). + const reg = new ConditionalRulesRegistry( + [rule('/r/ts.md', ['src/**/*.tsx'], 'Use hooks.')], + '/project', + ); + const escapedPath = 'src/App\\ file.tsx'; + const normalizedPath = unescapePath(escapedPath.trim()); + expect(normalizedPath).toBe('src/App file.tsx'); + const result = reg.matchAndConsume( + path.resolve('/project', normalizedPath), + ); + expect(result).toContain('Use hooks.'); + }, + ); }); });