From e9ace26e64586261ca1e65278c620a36d78f924f Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 26 Oct 2025 20:45:00 +0000 Subject: [PATCH 1/4] Fix: Use runtime.normalizePath for file tool path resolution The file editing tools (file_edit_replace_string, file_edit_insert, file_read) were incorrectly using Node.js's path.resolve() to resolve relative file paths. This caused bugs with SSH runtimes because path.resolve() uses LOCAL filesystem semantics instead of REMOTE filesystem semantics. Bug: - path.resolve() resolves paths relative to the local machine's filesystem - For SSH runtimes, paths need to be resolved relative to the remote filesystem - This caused file operations to fail or access wrong paths on SSH remotes Fix: - Replace path.resolve() with runtime.normalizePath() in all file tools - normalizePath() handles path resolution correctly for both local and SSH runtimes - LocalRuntime: uses Node.js path.resolve() (correct for local files) - SSHRuntime: uses POSIX-style path joining (correct for remote filesystems) Tests: - Added unit test to verify runtime.normalizePath() is used for path resolution - Added integration test for relative path handling in both local and SSH runtimes - All existing tests continue to pass Files changed: - src/services/tools/file_edit_operation.ts - src/services/tools/file_edit_insert.ts - src/services/tools/file_read.ts - src/services/tools/file_edit_operation.test.ts - tests/ipcMain/runtimeFileEditing.test.ts --- src/services/tools/file_edit_insert.ts | 6 +- .../tools/file_edit_operation.test.ts | 58 +++++++++++- src/services/tools/file_edit_operation.ts | 5 +- src/services/tools/file_read.ts | 7 +- tests/ipcMain/runtimeFileEditing.test.ts | 93 +++++++++++++++++++ 5 files changed, 155 insertions(+), 14 deletions(-) diff --git a/src/services/tools/file_edit_insert.ts b/src/services/tools/file_edit_insert.ts index 12a5599c61..00fdb2b147 100644 --- a/src/services/tools/file_edit_insert.ts +++ b/src/services/tools/file_edit_insert.ts @@ -1,5 +1,4 @@ import { tool } from "ai"; -import * as path from "path"; import type { FileEditInsertToolResult } from "@/types/tools"; import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; @@ -41,9 +40,8 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) }; } - const resolvedPath = path.isAbsolute(file_path) - ? file_path - : path.resolve(config.cwd, file_path); + // Use runtime's normalizePath method to resolve paths correctly for both local and SSH runtimes + const resolvedPath = config.runtime.normalizePath(file_path, config.cwd); // Check if file exists using runtime const exists = await fileExists(config.runtime, resolvedPath); diff --git a/src/services/tools/file_edit_operation.test.ts b/src/services/tools/file_edit_operation.test.ts index 927aae83be..23462ecbbb 100644 --- a/src/services/tools/file_edit_operation.test.ts +++ b/src/services/tools/file_edit_operation.test.ts @@ -1,14 +1,15 @@ -import { describe, test, expect } from "@jest/globals"; +import { describe, test, expect, jest, beforeEach } from "@jest/globals"; import { executeFileEditOperation } from "./file_edit_operation"; import { WRITE_DENIED_PREFIX } from "@/types/tools"; import { createRuntime } from "@/runtime/runtimeFactory"; +import type { Runtime } from "@/runtime/Runtime"; const TEST_CWD = "/tmp"; -function createConfig() { +function createConfig(runtime?: Runtime) { return { cwd: TEST_CWD, - runtime: createRuntime({ type: "local", srcBaseDir: TEST_CWD }), + runtime: runtime || createRuntime({ type: "local", srcBaseDir: TEST_CWD }), tempDir: "/tmp", }; } @@ -26,4 +27,55 @@ describe("executeFileEditOperation", () => { expect(result.error.startsWith(WRITE_DENIED_PREFIX)).toBe(true); } }); + + test("should use runtime.normalizePath for path resolution, not Node's path.resolve", async () => { + // This test exposes a bug where file_edit_operation.ts uses path.resolve() + // instead of runtime.normalizePath() for resolving file paths. + // + // The bug: path.resolve() uses LOCAL filesystem semantics (Node.js path module), + // which normalizes paths differently than the remote filesystem expects. + // For example, path.resolve() on Windows uses backslashes, and path normalization + // can behave differently across platforms. + + const normalizePathCalls: Array<{ targetPath: string; basePath: string }> = []; + + const mockRuntime = { + stat: jest.fn().mockRejectedValue(new Error("File not found")), + normalizePath: jest.fn((targetPath: string, basePath: string) => { + normalizePathCalls.push({ targetPath, basePath }); + // Mock SSH-style path normalization + if (targetPath.startsWith("/")) return targetPath; + return `${basePath}/${targetPath}`; + }), + } as unknown as Runtime; + + const testFilePath = "relative/path/to/file.txt"; + const testCwd = "/remote/workspace/dir"; + + await executeFileEditOperation({ + config: { + cwd: testCwd, + runtime: mockRuntime, + tempDir: "/tmp", + }, + filePath: testFilePath, + operation: () => ({ success: true, newContent: "test", metadata: {} }), + }); + + // BUG: The code uses path.resolve() directly instead of runtime.normalizePath() + // This means path resolution uses LOCAL filesystem semantics instead of runtime-specific logic + + // Check if normalizePath was called for path resolution + const normalizeCallForFilePath = normalizePathCalls.find( + (call) => call.targetPath === testFilePath + ); + + // This will FAIL because file_edit_operation.ts doesn't use runtime.normalizePath() + // for resolving the file path - it uses path.resolve() directly + expect(normalizeCallForFilePath).toBeDefined(); + + if (normalizeCallForFilePath) { + expect(normalizeCallForFilePath.basePath).toBe(testCwd); + } + }); }); diff --git a/src/services/tools/file_edit_operation.ts b/src/services/tools/file_edit_operation.ts index 14b922357f..5b16d08c6a 100644 --- a/src/services/tools/file_edit_operation.ts +++ b/src/services/tools/file_edit_operation.ts @@ -1,4 +1,3 @@ -import * as path from "path"; import type { FileEditDiffSuccessBase, FileEditErrorResult } from "@/types/tools"; import { WRITE_DENIED_PREFIX } from "@/types/tools"; import type { ToolConfiguration } from "@/utils/tools/tools"; @@ -45,7 +44,9 @@ export async function executeFileEditOperation({ }; } - const resolvedPath = path.isAbsolute(filePath) ? filePath : path.resolve(config.cwd, filePath); + // Use runtime's normalizePath method to resolve paths correctly for both local and SSH runtimes + // This ensures path resolution uses runtime-specific semantics instead of Node.js path module + const resolvedPath = config.runtime.normalizePath(filePath, config.cwd); // Check if file exists and get stats using runtime let fileStat; diff --git a/src/services/tools/file_read.ts b/src/services/tools/file_read.ts index 9bf9c6d1c7..34f172169f 100644 --- a/src/services/tools/file_read.ts +++ b/src/services/tools/file_read.ts @@ -1,5 +1,4 @@ import { tool } from "ai"; -import * as path from "path"; import type { FileReadToolResult } from "@/types/tools"; import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; @@ -31,10 +30,8 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => { }; } - // Resolve relative paths from configured working directory - const resolvedPath = path.isAbsolute(filePath) - ? filePath - : path.resolve(config.cwd, filePath); + // Use runtime's normalizePath method to resolve paths correctly for both local and SSH runtimes + const resolvedPath = config.runtime.normalizePath(filePath, config.cwd); // Check if file exists using runtime let fileStat; diff --git a/tests/ipcMain/runtimeFileEditing.test.ts b/tests/ipcMain/runtimeFileEditing.test.ts index c93dacdd0f..03849d2c27 100644 --- a/tests/ipcMain/runtimeFileEditing.test.ts +++ b/tests/ipcMain/runtimeFileEditing.test.ts @@ -484,6 +484,99 @@ describeIntegration("Runtime File Editing Tools", () => { }, type === "ssh" ? TEST_TIMEOUT_SSH_MS : TEST_TIMEOUT_LOCAL_MS ); + + test.concurrent( + "should handle relative paths correctly when editing files", + async () => { + const env = await createTestEnvironment(); + const tempGitRepo = await createTempGitRepo(); + + try { + // Setup provider + await setupProviders(env.mockIpcRenderer, { + anthropic: { + apiKey: getApiKey("ANTHROPIC_API_KEY"), + }, + }); + + // Create workspace + const branchName = generateBranchName("relative-path-test"); + const runtimeConfig = getRuntimeConfig(branchName); + const { workspaceId, cleanup } = await createWorkspaceHelper( + env, + tempGitRepo, + branchName, + runtimeConfig, + type === "ssh" + ); + + try { + const streamTimeout = + type === "ssh" ? STREAM_TIMEOUT_SSH_MS : STREAM_TIMEOUT_LOCAL_MS; + + // Create a file using AI with a relative path + const relativeTestFile = "subdir/relative_test.txt"; + const createEvents = await sendMessageAndWait( + env, + workspaceId, + `Create a file at path "${relativeTestFile}" with content: "Original content"`, + streamTimeout + ); + + // Verify file was created successfully + const createStreamEnd = createEvents.find( + (e) => "type" in e && e.type === "stream-end" + ); + expect(createStreamEnd).toBeDefined(); + expect((createStreamEnd as any).error).toBeUndefined(); + + // Now edit the file using a relative path + const editEvents = await sendMessageAndWait( + env, + workspaceId, + `Replace the text in ${relativeTestFile}: change "Original" to "Modified"`, + streamTimeout + ); + + // Verify edit was successful + const editStreamEnd = editEvents.find((e) => "type" in e && e.type === "stream-end"); + expect(editStreamEnd).toBeDefined(); + expect((editStreamEnd as any).error).toBeUndefined(); + + // Verify file_edit_replace_string tool was called + const toolCalls = editEvents.filter( + (e) => "type" in e && e.type === "tool-call-start" + ); + const editCall = toolCalls.find( + (e: any) => e.toolName === "file_edit_replace_string" + ); + expect(editCall).toBeDefined(); + + // Read the file to verify the edit was applied + const readEvents = await sendMessageAndWait( + env, + workspaceId, + `Read the file ${relativeTestFile} and tell me its content`, + streamTimeout + ); + + const responseText = extractTextFromEvents(readEvents); + // The file should contain "Modified" not "Original" + expect(responseText.toLowerCase()).toContain("modified"); + + // If this is SSH, the bug would cause the edit to fail because + // path.resolve() would resolve relative to the LOCAL filesystem + // instead of the REMOTE filesystem + } finally { + await cleanup(); + } + } finally { + await cleanupTestEnvironment(env); + await cleanupTempGitRepo(tempGitRepo); + } + }, + type === "ssh" ? TEST_TIMEOUT_SSH_MS : TEST_TIMEOUT_LOCAL_MS + ); } ); }); From 64b0fad67ca3188edd55bef8ebcca06d13a19882 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 26 Oct 2025 20:47:56 +0000 Subject: [PATCH 2/4] Fix linting errors in file_edit_operation.test.ts - Remove unused import (beforeEach) - Use nullish coalescing operator (??) instead of logical or (||) - Properly type jest.fn() mocks to avoid any types --- src/services/tools/file_edit_operation.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/services/tools/file_edit_operation.test.ts b/src/services/tools/file_edit_operation.test.ts index 23462ecbbb..9297717e54 100644 --- a/src/services/tools/file_edit_operation.test.ts +++ b/src/services/tools/file_edit_operation.test.ts @@ -1,4 +1,4 @@ -import { describe, test, expect, jest, beforeEach } from "@jest/globals"; +import { describe, test, expect, jest } from "@jest/globals"; import { executeFileEditOperation } from "./file_edit_operation"; import { WRITE_DENIED_PREFIX } from "@/types/tools"; import { createRuntime } from "@/runtime/runtimeFactory"; @@ -9,7 +9,7 @@ const TEST_CWD = "/tmp"; function createConfig(runtime?: Runtime) { return { cwd: TEST_CWD, - runtime: runtime || createRuntime({ type: "local", srcBaseDir: TEST_CWD }), + runtime: runtime ?? createRuntime({ type: "local", srcBaseDir: TEST_CWD }), tempDir: "/tmp", }; } @@ -40,8 +40,8 @@ describe("executeFileEditOperation", () => { const normalizePathCalls: Array<{ targetPath: string; basePath: string }> = []; const mockRuntime = { - stat: jest.fn().mockRejectedValue(new Error("File not found")), - normalizePath: jest.fn((targetPath: string, basePath: string) => { + stat: jest.fn<() => Promise>().mockRejectedValue(new Error("File not found")), + normalizePath: jest.fn<(targetPath: string, basePath: string) => string>((targetPath: string, basePath: string) => { normalizePathCalls.push({ targetPath, basePath }); // Mock SSH-style path normalization if (targetPath.startsWith("/")) return targetPath; From 4ef9b9d729192c2b3f520c983ba18cc88d6ac5d1 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 26 Oct 2025 20:48:41 +0000 Subject: [PATCH 3/4] Fix test: Mock runtime methods properly to avoid rejection The test was mocking runtime.stat to reject, which caused the test to fail before assertions could run. Now properly mocks stat, readFile, and writeFile to return successful values so the test can verify normalizePath is called. --- .../tools/file_edit_operation.test.ts | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/services/tools/file_edit_operation.test.ts b/src/services/tools/file_edit_operation.test.ts index 9297717e54..1826683bac 100644 --- a/src/services/tools/file_edit_operation.test.ts +++ b/src/services/tools/file_edit_operation.test.ts @@ -29,10 +29,10 @@ describe("executeFileEditOperation", () => { }); test("should use runtime.normalizePath for path resolution, not Node's path.resolve", async () => { - // This test exposes a bug where file_edit_operation.ts uses path.resolve() - // instead of runtime.normalizePath() for resolving file paths. + // This test verifies that executeFileEditOperation uses runtime.normalizePath() + // instead of path.resolve() for resolving file paths. // - // The bug: path.resolve() uses LOCAL filesystem semantics (Node.js path module), + // Why this matters: path.resolve() uses LOCAL filesystem semantics (Node.js path module), // which normalizes paths differently than the remote filesystem expects. // For example, path.resolve() on Windows uses backslashes, and path normalization // can behave differently across platforms. @@ -40,7 +40,13 @@ describe("executeFileEditOperation", () => { const normalizePathCalls: Array<{ targetPath: string; basePath: string }> = []; const mockRuntime = { - stat: jest.fn<() => Promise>().mockRejectedValue(new Error("File not found")), + stat: jest.fn<() => Promise<{ size: number; modifiedTime: Date; isDirectory: boolean }>>().mockResolvedValue({ + size: 100, + modifiedTime: new Date(), + isDirectory: false, + }), + readFile: jest.fn<() => Promise>().mockResolvedValue(new Uint8Array()), + writeFile: jest.fn<() => Promise>().mockResolvedValue(undefined), normalizePath: jest.fn<(targetPath: string, basePath: string) => string>((targetPath: string, basePath: string) => { normalizePathCalls.push({ targetPath, basePath }); // Mock SSH-style path normalization @@ -62,16 +68,11 @@ describe("executeFileEditOperation", () => { operation: () => ({ success: true, newContent: "test", metadata: {} }), }); - // BUG: The code uses path.resolve() directly instead of runtime.normalizePath() - // This means path resolution uses LOCAL filesystem semantics instead of runtime-specific logic - - // Check if normalizePath was called for path resolution + // Verify that runtime.normalizePath() was called for path resolution const normalizeCallForFilePath = normalizePathCalls.find( (call) => call.targetPath === testFilePath ); - // This will FAIL because file_edit_operation.ts doesn't use runtime.normalizePath() - // for resolving the file path - it uses path.resolve() directly expect(normalizeCallForFilePath).toBeDefined(); if (normalizeCallForFilePath) { From 4dac181e2c810ca7a9ecefbbbb3eb7a12a554003 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 26 Oct 2025 20:51:35 +0000 Subject: [PATCH 4/4] Fix formatting (prettier) --- .../tools/file_edit_operation.test.ts | 36 ++++++++++--------- tests/ipcMain/runtimeFileEditing.test.ts | 2 +- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/services/tools/file_edit_operation.test.ts b/src/services/tools/file_edit_operation.test.ts index 1826683bac..ffb90a1afe 100644 --- a/src/services/tools/file_edit_operation.test.ts +++ b/src/services/tools/file_edit_operation.test.ts @@ -31,28 +31,32 @@ describe("executeFileEditOperation", () => { test("should use runtime.normalizePath for path resolution, not Node's path.resolve", async () => { // This test verifies that executeFileEditOperation uses runtime.normalizePath() // instead of path.resolve() for resolving file paths. - // + // // Why this matters: path.resolve() uses LOCAL filesystem semantics (Node.js path module), // which normalizes paths differently than the remote filesystem expects. // For example, path.resolve() on Windows uses backslashes, and path normalization // can behave differently across platforms. - + const normalizePathCalls: Array<{ targetPath: string; basePath: string }> = []; - + const mockRuntime = { - stat: jest.fn<() => Promise<{ size: number; modifiedTime: Date; isDirectory: boolean }>>().mockResolvedValue({ - size: 100, - modifiedTime: new Date(), - isDirectory: false, - }), + stat: jest + .fn<() => Promise<{ size: number; modifiedTime: Date; isDirectory: boolean }>>() + .mockResolvedValue({ + size: 100, + modifiedTime: new Date(), + isDirectory: false, + }), readFile: jest.fn<() => Promise>().mockResolvedValue(new Uint8Array()), writeFile: jest.fn<() => Promise>().mockResolvedValue(undefined), - normalizePath: jest.fn<(targetPath: string, basePath: string) => string>((targetPath: string, basePath: string) => { - normalizePathCalls.push({ targetPath, basePath }); - // Mock SSH-style path normalization - if (targetPath.startsWith("/")) return targetPath; - return `${basePath}/${targetPath}`; - }), + normalizePath: jest.fn<(targetPath: string, basePath: string) => string>( + (targetPath: string, basePath: string) => { + normalizePathCalls.push({ targetPath, basePath }); + // Mock SSH-style path normalization + if (targetPath.startsWith("/")) return targetPath; + return `${basePath}/${targetPath}`; + } + ), } as unknown as Runtime; const testFilePath = "relative/path/to/file.txt"; @@ -72,9 +76,9 @@ describe("executeFileEditOperation", () => { const normalizeCallForFilePath = normalizePathCalls.find( (call) => call.targetPath === testFilePath ); - + expect(normalizeCallForFilePath).toBeDefined(); - + if (normalizeCallForFilePath) { expect(normalizeCallForFilePath.basePath).toBe(testCwd); } diff --git a/tests/ipcMain/runtimeFileEditing.test.ts b/tests/ipcMain/runtimeFileEditing.test.ts index 03849d2c27..2143a574cd 100644 --- a/tests/ipcMain/runtimeFileEditing.test.ts +++ b/tests/ipcMain/runtimeFileEditing.test.ts @@ -563,7 +563,7 @@ describeIntegration("Runtime File Editing Tools", () => { const responseText = extractTextFromEvents(readEvents); // The file should contain "Modified" not "Original" expect(responseText.toLowerCase()).toContain("modified"); - + // If this is SSH, the bug would cause the edit to fail because // path.resolve() would resolve relative to the LOCAL filesystem // instead of the REMOTE filesystem