From 33a3450e4b60169edcd9b977c6e19770a6524d9b Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 14:28:39 +0000 Subject: [PATCH 01/21] Expand tilde in LocalRuntime srcBaseDir at construction time Previously, LocalRuntime would store srcBaseDir paths with tildes unexpanded, leading to incorrect path construction when using path.join(). This simplifies logic by ensuring all local paths are fully expanded at the IPC boundary. Changes: - Add expandTilde() utility for local file system path expansion - Update LocalRuntime constructor to expand tilde in srcBaseDir - Add comprehensive tests for both utilities For SSH runtime, tilde paths are preserved and expanded using expandTildeForSSH() when constructing remote commands, which is the correct behavior for remote paths. --- src/runtime/LocalRuntime.test.ts | 31 ++++++++++++++++++++++++++ src/runtime/LocalRuntime.ts | 4 +++- src/runtime/tildeExpansion.test.ts | 33 ++++++++++++++++++++++++++++ src/runtime/tildeExpansion.ts | 35 ++++++++++++++++++++++++++++-- 4 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 src/runtime/LocalRuntime.test.ts create mode 100644 src/runtime/tildeExpansion.test.ts diff --git a/src/runtime/LocalRuntime.test.ts b/src/runtime/LocalRuntime.test.ts new file mode 100644 index 000000000..8c348400a --- /dev/null +++ b/src/runtime/LocalRuntime.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "bun:test"; +import * as os from "os"; +import * as path from "path"; +import { LocalRuntime } from "./LocalRuntime"; + +describe("LocalRuntime constructor", () => { + it("should expand tilde in srcBaseDir", () => { + const runtime = new LocalRuntime("~/workspace"); + const workspacePath = runtime.getWorkspacePath("/home/user/project", "branch"); + + // The workspace path should use the expanded home directory + const expected = path.join(os.homedir(), "workspace", "project", "branch"); + expect(workspacePath).toBe(expected); + }); + + it("should handle absolute paths without expansion", () => { + const runtime = new LocalRuntime("/absolute/path"); + const workspacePath = runtime.getWorkspacePath("/home/user/project", "branch"); + + const expected = path.join("/absolute/path", "project", "branch"); + expect(workspacePath).toBe(expected); + }); + + it("should handle bare tilde", () => { + const runtime = new LocalRuntime("~"); + const workspacePath = runtime.getWorkspacePath("/home/user/project", "branch"); + + const expected = path.join(os.homedir(), "project", "branch"); + expect(workspacePath).toBe(expected); + }); +}); diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index 8edb8d5d8..945eb2c97 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -22,6 +22,7 @@ import { checkInitHookExists, getInitHookPath, createLineBufferedLoggers } from import { execAsync } from "../utils/disposableExec"; import { getProjectName } from "../utils/runtime/helpers"; import { getErrorMessage } from "../utils/errors"; +import { expandTilde } from "./tildeExpansion"; /** * Local runtime implementation that executes commands and file operations @@ -31,7 +32,8 @@ export class LocalRuntime implements Runtime { private readonly srcBaseDir: string; constructor(srcBaseDir: string) { - this.srcBaseDir = srcBaseDir; + // Expand tilde to actual home directory path for local file system operations + this.srcBaseDir = expandTilde(srcBaseDir); } async exec(command: string, options: ExecOptions): Promise { diff --git a/src/runtime/tildeExpansion.test.ts b/src/runtime/tildeExpansion.test.ts new file mode 100644 index 000000000..25042eb9a --- /dev/null +++ b/src/runtime/tildeExpansion.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from "bun:test"; +import * as os from "os"; +import * as path from "path"; +import { expandTilde } from "./tildeExpansion"; + +describe("expandTilde", () => { + it("should expand ~ to home directory", () => { + const result = expandTilde("~"); + expect(result).toBe(os.homedir()); + }); + + it("should expand ~/path to home directory + path", () => { + const result = expandTilde("~/workspace"); + expect(result).toBe(path.join(os.homedir(), "workspace")); + }); + + it("should leave absolute paths unchanged", () => { + const absolutePath = "/abs/path/to/dir"; + const result = expandTilde(absolutePath); + expect(result).toBe(absolutePath); + }); + + it("should leave relative paths unchanged", () => { + const relativePath = "relative/path"; + const result = expandTilde(relativePath); + expect(result).toBe(relativePath); + }); + + it("should handle nested paths correctly", () => { + const result = expandTilde("~/workspace/project/subdir"); + expect(result).toBe(path.join(os.homedir(), "workspace/project/subdir")); + }); +}); diff --git a/src/runtime/tildeExpansion.ts b/src/runtime/tildeExpansion.ts index be37c0810..e5084b6ca 100644 --- a/src/runtime/tildeExpansion.ts +++ b/src/runtime/tildeExpansion.ts @@ -1,11 +1,42 @@ /** - * Utilities for handling tilde path expansion in SSH commands + * Utilities for handling tilde path expansion * - * When running commands over SSH, tilde paths need special handling: + * For SSH commands, tilde paths need special handling: * - Quoted tildes won't expand: `cd '~'` fails, but `cd "$HOME"` works * - Must escape special shell characters when using $HOME expansion + * + * For local paths, tildes should be expanded to actual file system paths. */ +import * as os from "os"; +import * as path from "path"; + +/** + * Expand tilde to actual home directory path for local file system operations. + * + * Converts: + * - "~" → "/home/user" (actual home directory) + * - "~/path" → "/home/user/path" + * - "/abs/path" → "/abs/path" (unchanged) + * + * @param filePath - Path that may contain tilde prefix + * @returns Fully expanded absolute path + * + * @example + * expandTilde("~") // => "/home/user" + * expandTilde("~/workspace") // => "/home/user/workspace" + * expandTilde("/abs/path") // => "/abs/path" + */ +export function expandTilde(filePath: string): string { + if (filePath === "~") { + return os.homedir(); + } else if (filePath.startsWith("~/")) { + return path.join(os.homedir(), filePath.slice(2)); + } else { + return filePath; + } +} + /** * Expand tilde path to $HOME-based path for use in SSH commands. * From 827d5989ccbb69e1e5e33e63d0da848a089fac99 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 14:54:57 +0000 Subject: [PATCH 02/21] Reject tilde paths in SSH runtime srcBaseDir SSH runtime now requires absolute paths for srcBaseDir to simplify logic and avoid ambiguity about which user's home directory is being referenced. Changes: - SSHRuntime constructor validates srcBaseDir doesn't start with tilde - Updated default SSH srcBaseDir from ~/cmux to /home/cmux - Updated all tests to use absolute paths or sshConfig.workdir - Replaced tilde-handling tests with tilde-rejection tests - Updated Runtime.ts documentation to clarify path requirements LocalRuntime continues to expand tilde paths at construction (previous commit), ensuring all runtime paths are fully resolved at the IPC boundary. --- src/runtime/Runtime.ts | 10 +-- src/runtime/SSHRuntime.test.ts | 31 ++++++++ src/runtime/SSHRuntime.ts | 9 +++ src/utils/chatCommands.test.ts | 10 +-- src/utils/chatCommands.ts | 2 +- tests/ipcMain/createWorkspace.test.ts | 105 +++++++++----------------- 6 files changed, 85 insertions(+), 82 deletions(-) create mode 100644 src/runtime/SSHRuntime.test.ts diff --git a/src/runtime/Runtime.ts b/src/runtime/Runtime.ts index 28f8fd9b4..47c5e83ad 100644 --- a/src/runtime/Runtime.ts +++ b/src/runtime/Runtime.ts @@ -14,8 +14,8 @@ * * srcBaseDir (base directory for all workspaces): * - Where cmux stores ALL workspace directories - * - Local: ~/.cmux/src - * - SSH: /home/user/workspace (or custom remote path) + * - Local: ~/.cmux/src (tilde expanded to full path by LocalRuntime) + * - SSH: /home/user/workspace (must be absolute path, no tilde allowed) * * Workspace Path Computation: * {srcBaseDir}/{projectName}/{workspaceName} @@ -27,14 +27,14 @@ * Example: "feature-123" or "main" * * Full Example (Local): - * srcBaseDir: ~/.cmux/src + * srcBaseDir: ~/.cmux/src (expanded to /home/user/.cmux/src) * projectPath: /Users/me/git/my-project (local git repo) * projectName: my-project (extracted) * workspaceName: feature-123 - * → Workspace: ~/.cmux/src/my-project/feature-123 + * → Workspace: /home/user/.cmux/src/my-project/feature-123 * * Full Example (SSH): - * srcBaseDir: /home/user/workspace + * srcBaseDir: /home/user/workspace (absolute path required) * projectPath: /Users/me/git/my-project (local git repo) * projectName: my-project (extracted) * workspaceName: feature-123 diff --git a/src/runtime/SSHRuntime.test.ts b/src/runtime/SSHRuntime.test.ts new file mode 100644 index 000000000..2f65c5223 --- /dev/null +++ b/src/runtime/SSHRuntime.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "bun:test"; +import { SSHRuntime } from "./SSHRuntime"; + +describe("SSHRuntime constructor", () => { + it("should reject tilde in srcBaseDir", () => { + expect(() => { + new SSHRuntime({ + host: "example.com", + srcBaseDir: "~/cmux", + }); + }).toThrow(/cannot start with tilde/); + }); + + it("should reject bare tilde in srcBaseDir", () => { + expect(() => { + new SSHRuntime({ + host: "example.com", + srcBaseDir: "~", + }); + }).toThrow(/cannot start with tilde/); + }); + + it("should accept absolute paths in srcBaseDir", () => { + expect(() => { + new SSHRuntime({ + host: "example.com", + srcBaseDir: "/home/user/cmux", + }); + }).not.toThrow(); + }); +}); diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index 44ac8e336..b2bbc37ee 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -59,6 +59,15 @@ export class SSHRuntime implements Runtime { private readonly controlPath: string; constructor(config: SSHRuntimeConfig) { + // Reject tilde paths - require explicit full paths for SSH + // Rationale: Simplifies logic and avoids ambiguity about which user's home directory + if (config.srcBaseDir.startsWith("~")) { + throw new Error( + `SSH runtime srcBaseDir cannot start with tilde. ` + + `Use full path (e.g., /home/username/cmux instead of ~/cmux)` + ); + } + this.config = config; // Get deterministic controlPath from connection pool // Multiple SSHRuntime instances with same config share the same controlPath, diff --git a/src/utils/chatCommands.test.ts b/src/utils/chatCommands.test.ts index 304031dd1..15a456def 100644 --- a/src/utils/chatCommands.test.ts +++ b/src/utils/chatCommands.test.ts @@ -18,7 +18,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "user@host", - srcBaseDir: "~/cmux", + srcBaseDir: "/home/cmux", }); }); @@ -27,7 +27,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "User@Host.Example.Com", - srcBaseDir: "~/cmux", + srcBaseDir: "/home/cmux", }); }); @@ -36,7 +36,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "user@host", - srcBaseDir: "~/cmux", + srcBaseDir: "/home/cmux", }); }); @@ -50,7 +50,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "hostname", - srcBaseDir: "~/cmux", + srcBaseDir: "/home/cmux", }); }); @@ -59,7 +59,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "dev.example.com", - srcBaseDir: "~/cmux", + srcBaseDir: "/home/cmux", }); }); diff --git a/src/utils/chatCommands.ts b/src/utils/chatCommands.ts index 3c19b4901..5629fdb88 100644 --- a/src/utils/chatCommands.ts +++ b/src/utils/chatCommands.ts @@ -56,7 +56,7 @@ export function parseRuntimeString( return { type: RUNTIME_MODE.SSH, host: hostPart, - srcBaseDir: "~/cmux", // Default remote base directory (NOT including workspace name) + srcBaseDir: "/home/cmux", // Default remote base directory (NOT including workspace name) }; } diff --git a/tests/ipcMain/createWorkspace.test.ts b/tests/ipcMain/createWorkspace.test.ts index 44fcbb07a..ba300fe20 100644 --- a/tests/ipcMain/createWorkspace.test.ts +++ b/tests/ipcMain/createWorkspace.test.ts @@ -590,45 +590,41 @@ exit 1 ); test.concurrent( - "handles tilde (~/) paths correctly (SSH only)", + "rejects tilde paths in srcBaseDir (SSH only)", async () => { + // Skip if SSH server not available + if (!sshConfig) { + console.log("Skipping SSH-specific test: SSH server not available"); + return; + } + const env = await createTestEnvironment(); const tempGitRepo = await createTempGitRepo(); try { - const branchName = generateBranchName("tilde-test"); + const branchName = generateBranchName("tilde-rejection-test"); const trunkBranch = await detectDefaultTrunkBranch(tempGitRepo); - // Use ~/workspace/... path instead of absolute path + // Try to use tilde path - should be rejected const tildeRuntimeConfig: RuntimeConfig = { type: "ssh", host: `testuser@localhost`, srcBaseDir: `~/workspace`, - identityFile: sshConfig!.privateKeyPath, - port: sshConfig!.port, + identityFile: sshConfig.privateKeyPath, + port: sshConfig.port, }; - const { result, cleanup } = await createWorkspaceWithCleanup( - env, + const result = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, tempGitRepo, branchName, trunkBranch, tildeRuntimeConfig ); - expect(result.success).toBe(true); - if (!result.success) { - throw new Error(`Failed to create workspace with tilde path: ${result.error}`); - } - - // Wait for init to complete - await new Promise((resolve) => setTimeout(resolve, getInitWaitTime())); - - // Verify workspace exists - expect(result.metadata.id).toBeDefined(); - expect(result.metadata.namedWorkspacePath).toBeDefined(); - - await cleanup(); + // Should fail with error about tilde + expect(result.success).toBe(false); + expect(result.error).toMatch(/cannot start with tilde/i); } finally { await cleanupTestEnvironment(env); await cleanupTempGitRepo(tempGitRepo); @@ -638,74 +634,41 @@ exit 1 ); test.concurrent( - "handles tilde paths with init hooks (SSH only)", + "rejects bare tilde in srcBaseDir (SSH only)", async () => { + // Skip if SSH server not available + if (!sshConfig) { + console.log("Skipping SSH-specific test: SSH server not available"); + return; + } + const env = await createTestEnvironment(); const tempGitRepo = await createTempGitRepo(); try { - // Add init hook to repo - await createInitHook( - tempGitRepo, - `#!/bin/bash -echo "Init hook executed with tilde path" -` - ); - await commitChanges(tempGitRepo, "Add init hook for tilde test"); - - const branchName = generateBranchName("tilde-init-test"); + const branchName = generateBranchName("bare-tilde-rejection"); const trunkBranch = await detectDefaultTrunkBranch(tempGitRepo); - // Use ~/workspace/... path instead of absolute path + // Try to use bare tilde - should be rejected const tildeRuntimeConfig: RuntimeConfig = { type: "ssh", host: `testuser@localhost`, - srcBaseDir: `~/workspace`, - identityFile: sshConfig!.privateKeyPath, - port: sshConfig!.port, + srcBaseDir: `~`, + identityFile: sshConfig.privateKeyPath, + port: sshConfig.port, }; - // Capture init events to verify hook output - const initEvents = setupInitEventCapture(env); - - const { result, cleanup } = await createWorkspaceWithCleanup( - env, + const result = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, tempGitRepo, branchName, trunkBranch, tildeRuntimeConfig ); - expect(result.success).toBe(true); - if (!result.success) { - throw new Error( - `Failed to create workspace with tilde path + init hook: ${result.error}` - ); - } - - // Wait for init to complete (including hook) - await new Promise((resolve) => setTimeout(resolve, getInitWaitTime())); - - // Verify init hook was executed - const outputEvents = filterEventsByType(initEvents, EVENT_TYPE_INIT_OUTPUT); - const outputLines = outputEvents.map((e) => { - const data = e.data as { line?: string }; - return data.line ?? ""; - }); - - // Debug: Print all output including errors - console.log("=== TILDE INIT HOOK OUTPUT ==="); - outputEvents.forEach((e) => { - const data = e.data as { line?: string; isError?: boolean }; - const prefix = data.isError ? "[ERROR]" : "[INFO] "; - console.log(prefix + (data.line ?? "")); - }); - console.log("=== END TILDE INIT HOOK OUTPUT ==="); - - expect(outputLines.some((line) => line.includes("Running init hook"))).toBe(true); - expect(outputLines.some((line) => line.includes("Init hook executed"))).toBe(true); - - await cleanup(); + // Should fail with error about tilde + expect(result.success).toBe(false); + expect(result.error).toMatch(/cannot start with tilde/i); } finally { await cleanupTestEnvironment(env); await cleanupTempGitRepo(tempGitRepo); @@ -848,7 +811,7 @@ echo "Init hook executed with tilde path" const runtimeConfig: RuntimeConfig = { type: "ssh", host: "testuser@localhost", - srcBaseDir: "~/workspace", + srcBaseDir: sshConfig.workdir, identityFile: sshConfig.privateKeyPath, port: sshConfig.port, }; From 164622a7e7e1e214df94a2543dad797713a1b40e Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 14:58:10 +0000 Subject: [PATCH 03/21] Fix SSH runtime redundant cd detection tests Updated tests to use absolute paths instead of tilde paths for SSH runtime, consistent with the requirement that SSH srcBaseDir must be absolute. --- src/services/tools/bash.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/tools/bash.test.ts b/src/services/tools/bash.test.ts index 40c287825..9b426d731 100644 --- a/src/services/tools/bash.test.ts +++ b/src/services/tools/bash.test.ts @@ -1174,12 +1174,12 @@ describe("SSH runtime redundant cd detection", () => { } it("should add educational note when command starts with cd", async () => { - const remoteCwd = "~/workspace/project/branch"; + const remoteCwd = "/remote/workspace/project/branch"; using testEnv = createTestBashToolWithSSH(remoteCwd); const tool = testEnv.tool; const args: BashToolArgs = { - script: "cd ~/workspace/project/branch && echo test", + script: "cd /remote/workspace/project/branch && echo test", timeout_secs: 5, }; @@ -1194,7 +1194,7 @@ describe("SSH runtime redundant cd detection", () => { }); it("should not add note when command does not start with cd", async () => { - const remoteCwd = "~/workspace/project/branch"; + const remoteCwd = "/remote/workspace/project/branch"; using testEnv = createTestBashToolWithSSH(remoteCwd); const tool = testEnv.tool; From 8c49ea5eb3663ea7101e5f3622ae999a195a4375 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:02:45 +0000 Subject: [PATCH 04/21] Restore redundant cd rejection in bash tool Reverted from educational note back to rejection behavior. When a command tries to cd into the current working directory (e.g., 'cd /workspace && command'), the bash tool now rejects it with a clear error message. Changes: - Replaced educational note with redundant cd detection and rejection - Uses runtime.normalizePath() for cross-runtime path comparison - Works correctly for both local and SSH runtimes - Updated tests to verify rejection behavior --- src/services/tools/bash.test.ts | 26 ++++++++++++++++---------- src/services/tools/bash.ts | 26 +++++++++++++++++++------- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/services/tools/bash.test.ts b/src/services/tools/bash.test.ts index 9b426d731..c860cfaa3 100644 --- a/src/services/tools/bash.test.ts +++ b/src/services/tools/bash.test.ts @@ -1173,7 +1173,7 @@ describe("SSH runtime redundant cd detection", () => { }; } - it("should add educational note when command starts with cd", async () => { + it("should reject redundant cd when command cds to working directory", async () => { const remoteCwd = "/remote/workspace/project/branch"; using testEnv = createTestBashToolWithSSH(remoteCwd); const tool = testEnv.tool; @@ -1185,27 +1185,33 @@ describe("SSH runtime redundant cd detection", () => { const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; - // Command should execute (not blocked) - // But should include a note about cd behavior - if (result.success && "note" in result) { - expect(result.note).toContain("bash command starts in"); - expect(result.note).toContain("do not persist"); + // Should reject the redundant cd + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant cd to working directory"); + expect(result.error).toContain("no cd needed"); + expect(result.exitCode).toBe(-1); } }); - it("should not add note when command does not start with cd", async () => { + it("should allow cd to different directory", async () => { const remoteCwd = "/remote/workspace/project/branch"; using testEnv = createTestBashToolWithSSH(remoteCwd); const tool = testEnv.tool; const args: BashToolArgs = { - script: "echo test", + script: "cd /tmp && echo test", timeout_secs: 5, }; const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; - // Should not have a note field - expect(result).not.toHaveProperty("note"); + // Should not be blocked (cd to a different directory is allowed) + // Note: Test runs locally so actual cd might fail, but we check it's not rejected + expect(result.exitCode).not.toBe(-1); // Not a rejection (-1) + if (!result.success) { + // If it failed, it should not be due to redundant cd detection + expect(result.error).not.toContain("Redundant cd"); + } }); }); diff --git a/src/services/tools/bash.ts b/src/services/tools/bash.ts index 2e6375624..5673175ee 100644 --- a/src/services/tools/bash.ts +++ b/src/services/tools/bash.ts @@ -76,11 +76,25 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => { let displayTruncated = false; // Hit 16KB display limit let fileTruncated = false; // Hit 100KB file limit - // Detect if command starts with cd - we'll add an educational note for the agent - const scriptStartsWithCd = /^\s*cd\s/.test(script); - const cdNote = scriptStartsWithCd - ? `Note: Each bash command starts in ${config.cwd}. Directory changes (cd) do not persist between commands.` - : undefined; + // Detect redundant cd to working directory + // Match patterns like: "cd /path &&", "cd /path;", "cd '/path' &&", "cd \"/path\" &&" + const cdPattern = /^\s*cd\s+['"]?([^'";&|]+)['"]?\s*[;&|]/; + const match = cdPattern.exec(script); + if (match) { + const targetPath = match[1].trim(); + // Normalize paths for comparison using runtime's path resolution + const normalizedTarget = config.runtime.normalizePath(targetPath, config.cwd); + const normalizedCwd = config.runtime.normalizePath(".", config.cwd); + + if (normalizedTarget === normalizedCwd) { + return { + success: false, + error: `Redundant cd to working directory detected. The tool already runs in ${config.cwd} - no cd needed. Remove the 'cd ${targetPath}' prefix.`, + exitCode: -1, + wall_duration_ms: 0, + }; + } + } // Execute using runtime interface (works for both local and SSH) const execStream = await config.runtime.exec(script, { @@ -377,7 +391,6 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => { output, exitCode: 0, wall_duration_ms, - ...(cdNote && { note: cdNote }), truncated: { reason: overflowReason ?? "unknown reason", totalLines: lines.length, @@ -465,7 +478,6 @@ File will be automatically cleaned up when stream ends.`; output: lines.join("\n"), exitCode: 0, wall_duration_ms, - ...(cdNote && { note: cdNote }), }); } else { resolveOnce({ From 242485f555e7b25147824adc3623e868a95812b2 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:04:44 +0000 Subject: [PATCH 05/21] Format LocalRuntime.test.ts --- src/runtime/LocalRuntime.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/runtime/LocalRuntime.test.ts b/src/runtime/LocalRuntime.test.ts index 8c348400a..03abda812 100644 --- a/src/runtime/LocalRuntime.test.ts +++ b/src/runtime/LocalRuntime.test.ts @@ -7,7 +7,7 @@ describe("LocalRuntime constructor", () => { it("should expand tilde in srcBaseDir", () => { const runtime = new LocalRuntime("~/workspace"); const workspacePath = runtime.getWorkspacePath("/home/user/project", "branch"); - + // The workspace path should use the expanded home directory const expected = path.join(os.homedir(), "workspace", "project", "branch"); expect(workspacePath).toBe(expected); @@ -16,7 +16,7 @@ describe("LocalRuntime constructor", () => { it("should handle absolute paths without expansion", () => { const runtime = new LocalRuntime("/absolute/path"); const workspacePath = runtime.getWorkspacePath("/home/user/project", "branch"); - + const expected = path.join("/absolute/path", "project", "branch"); expect(workspacePath).toBe(expected); }); @@ -24,7 +24,7 @@ describe("LocalRuntime constructor", () => { it("should handle bare tilde", () => { const runtime = new LocalRuntime("~"); const workspacePath = runtime.getWorkspacePath("/home/user/project", "branch"); - + const expected = path.join(os.homedir(), "project", "branch"); expect(workspacePath).toBe(expected); }); From 9102feda5d3012862cd84b946283baff49a3469e Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:05:43 +0000 Subject: [PATCH 06/21] Derive SSH srcBaseDir from username When parsing SSH runtime strings, extract the username from user@host format and use it to construct the srcBaseDir path. This ensures workspace creation targets the correct user's home directory. - 'ssh user@host' -> srcBaseDir: /home/user/cmux - 'ssh hostname' -> srcBaseDir: /home/$USER/cmux (current user) Resolves Codex review comment about hard-coded /home/cmux. --- src/utils/chatCommands.test.ts | 14 +++++++++----- src/utils/chatCommands.ts | 6 +++++- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/utils/chatCommands.test.ts b/src/utils/chatCommands.test.ts index 15a456def..95ad42995 100644 --- a/src/utils/chatCommands.test.ts +++ b/src/utils/chatCommands.test.ts @@ -18,7 +18,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "user@host", - srcBaseDir: "/home/cmux", + srcBaseDir: "/home/user/cmux", }); }); @@ -27,7 +27,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "User@Host.Example.Com", - srcBaseDir: "/home/cmux", + srcBaseDir: "/home/User/cmux", }); }); @@ -36,7 +36,7 @@ describe("parseRuntimeString", () => { expect(result).toEqual({ type: "ssh", host: "user@host", - srcBaseDir: "/home/cmux", + srcBaseDir: "/home/user/cmux", }); }); @@ -47,19 +47,23 @@ describe("parseRuntimeString", () => { test("accepts SSH with hostname only (user will be inferred)", () => { const result = parseRuntimeString("ssh hostname", workspaceName); + // When no user is specified, uses current user (process.env.USER) + const expectedUser = process.env.USER || "user"; expect(result).toEqual({ type: "ssh", host: "hostname", - srcBaseDir: "/home/cmux", + srcBaseDir: `/home/${expectedUser}/cmux`, }); }); test("accepts SSH with hostname.domain only", () => { const result = parseRuntimeString("ssh dev.example.com", workspaceName); + // When no user is specified, uses current user (process.env.USER) + const expectedUser = process.env.USER || "user"; expect(result).toEqual({ type: "ssh", host: "dev.example.com", - srcBaseDir: "/home/cmux", + srcBaseDir: `/home/${expectedUser}/cmux`, }); }); diff --git a/src/utils/chatCommands.ts b/src/utils/chatCommands.ts index 5629fdb88..d6ce66d16 100644 --- a/src/utils/chatCommands.ts +++ b/src/utils/chatCommands.ts @@ -51,12 +51,16 @@ export function parseRuntimeString( throw new Error("SSH runtime requires host (e.g., 'ssh hostname' or 'ssh user@host')"); } + // Extract username from user@host format, or default to current user + const atIndex = hostPart.indexOf("@"); + const user = atIndex > 0 ? hostPart.substring(0, atIndex) : process.env.USER || "user"; + // Accept both "hostname" and "user@hostname" formats // SSH will use current user or ~/.ssh/config if user not specified return { type: RUNTIME_MODE.SSH, host: hostPart, - srcBaseDir: "/home/cmux", // Default remote base directory (NOT including workspace name) + srcBaseDir: `/home/${user}/cmux`, // Default remote base directory (NOT including workspace name) }; } From 42b793221e123e6797a6eb870cc44cb1cc48095e Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:07:29 +0000 Subject: [PATCH 07/21] Fix lint: use nullish coalescing --- src/utils/chatCommands.test.ts | 4 ++-- src/utils/chatCommands.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/utils/chatCommands.test.ts b/src/utils/chatCommands.test.ts index 95ad42995..fa17f2593 100644 --- a/src/utils/chatCommands.test.ts +++ b/src/utils/chatCommands.test.ts @@ -48,7 +48,7 @@ describe("parseRuntimeString", () => { test("accepts SSH with hostname only (user will be inferred)", () => { const result = parseRuntimeString("ssh hostname", workspaceName); // When no user is specified, uses current user (process.env.USER) - const expectedUser = process.env.USER || "user"; + const expectedUser = process.env.USER ?? "user"; expect(result).toEqual({ type: "ssh", host: "hostname", @@ -59,7 +59,7 @@ describe("parseRuntimeString", () => { test("accepts SSH with hostname.domain only", () => { const result = parseRuntimeString("ssh dev.example.com", workspaceName); // When no user is specified, uses current user (process.env.USER) - const expectedUser = process.env.USER || "user"; + const expectedUser = process.env.USER ?? "user"; expect(result).toEqual({ type: "ssh", host: "dev.example.com", diff --git a/src/utils/chatCommands.ts b/src/utils/chatCommands.ts index d6ce66d16..9c40c5cb1 100644 --- a/src/utils/chatCommands.ts +++ b/src/utils/chatCommands.ts @@ -53,7 +53,7 @@ export function parseRuntimeString( // Extract username from user@host format, or default to current user const atIndex = hostPart.indexOf("@"); - const user = atIndex > 0 ? hostPart.substring(0, atIndex) : process.env.USER || "user"; + const user = atIndex > 0 ? hostPart.substring(0, atIndex) : process.env.USER ?? "user"; // Accept both "hostname" and "user@hostname" formats // SSH will use current user or ~/.ssh/config if user not specified From 0ab0930a7a6581d85990631e7f3008d474c06d4a Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:09:36 +0000 Subject: [PATCH 08/21] Use /root for root user in SSH srcBaseDir When parsing SSH runtime strings with user 'root', use /root as the home directory instead of /home/root. --- src/utils/chatCommands.test.ts | 15 +++++++++++++-- src/utils/chatCommands.ts | 6 +++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/utils/chatCommands.test.ts b/src/utils/chatCommands.test.ts index fa17f2593..306838a5f 100644 --- a/src/utils/chatCommands.test.ts +++ b/src/utils/chatCommands.test.ts @@ -49,10 +49,11 @@ describe("parseRuntimeString", () => { const result = parseRuntimeString("ssh hostname", workspaceName); // When no user is specified, uses current user (process.env.USER) const expectedUser = process.env.USER ?? "user"; + const expectedHomeDir = expectedUser === "root" ? "/root" : `/home/${expectedUser}`; expect(result).toEqual({ type: "ssh", host: "hostname", - srcBaseDir: `/home/${expectedUser}/cmux`, + srcBaseDir: `${expectedHomeDir}/cmux`, }); }); @@ -60,10 +61,20 @@ describe("parseRuntimeString", () => { const result = parseRuntimeString("ssh dev.example.com", workspaceName); // When no user is specified, uses current user (process.env.USER) const expectedUser = process.env.USER ?? "user"; + const expectedHomeDir = expectedUser === "root" ? "/root" : `/home/${expectedUser}`; expect(result).toEqual({ type: "ssh", host: "dev.example.com", - srcBaseDir: `/home/${expectedUser}/cmux`, + srcBaseDir: `${expectedHomeDir}/cmux`, + }); + }); + + test("uses /root for root user", () => { + const result = parseRuntimeString("ssh root@hostname", workspaceName); + expect(result).toEqual({ + type: "ssh", + host: "root@hostname", + srcBaseDir: "/root/cmux", }); }); diff --git a/src/utils/chatCommands.ts b/src/utils/chatCommands.ts index 9c40c5cb1..164a2f948 100644 --- a/src/utils/chatCommands.ts +++ b/src/utils/chatCommands.ts @@ -55,12 +55,16 @@ export function parseRuntimeString( const atIndex = hostPart.indexOf("@"); const user = atIndex > 0 ? hostPart.substring(0, atIndex) : process.env.USER ?? "user"; + // Determine home directory path based on user + // root user's home is /root, all others are /home/ + const homeDir = user === "root" ? "/root" : `/home/${user}`; + // Accept both "hostname" and "user@hostname" formats // SSH will use current user or ~/.ssh/config if user not specified return { type: RUNTIME_MODE.SSH, host: hostPart, - srcBaseDir: `/home/${user}/cmux`, // Default remote base directory (NOT including workspace name) + srcBaseDir: `${homeDir}/cmux`, // Default remote base directory (NOT including workspace name) }; } From c5915d410bf6933ffeab0fc6d426664bd7eaf3dd Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:11:45 +0000 Subject: [PATCH 09/21] Format chatCommands.ts --- src/utils/chatCommands.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/chatCommands.ts b/src/utils/chatCommands.ts index 164a2f948..dbd9fa140 100644 --- a/src/utils/chatCommands.ts +++ b/src/utils/chatCommands.ts @@ -53,7 +53,7 @@ export function parseRuntimeString( // Extract username from user@host format, or default to current user const atIndex = hostPart.indexOf("@"); - const user = atIndex > 0 ? hostPart.substring(0, atIndex) : process.env.USER ?? "user"; + const user = atIndex > 0 ? hostPart.substring(0, atIndex) : (process.env.USER ?? "user"); // Determine home directory path based on user // root user's home is /root, all others are /home/ From 83f2e2d3a9cec2bc8be9ea893066fdd0caa6ad52 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:21:34 +0000 Subject: [PATCH 10/21] =?UTF-8?q?=F0=9F=A4=96=20Block=20redundant=20path?= =?UTF-8?q?=20prefixes=20in=20file=5F*=20tools=20to=20save=20tokens?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add validation to reject absolute paths that contain the workspace directory prefix, encouraging agents to use relative paths instead. This helps save tokens by avoiding redundant path prefixes in tool calls. Changes: - Add validateNoRedundantPrefix() to fileCommon.ts to detect absolute paths containing cwd - Integrate validation into all file_* tools (file_read, file_edit_insert, file_edit_operation) - Update all tool tests to use relative paths instead of absolute paths - Add comprehensive test coverage for redundant prefix detection The validation provides helpful error messages suggesting the relative path equivalent: "Redundant path prefix detected. The path '/workspace/project/src/file.ts' contains the workspace directory. Please use relative paths to save tokens: 'src/file.ts'" _Generated with `cmux`_ --- src/services/tools/fileCommon.test.ts | 66 +++++++++++++++++++- src/services/tools/fileCommon.ts | 40 ++++++++++++ src/services/tools/file_edit_insert.test.ts | 34 +++++----- src/services/tools/file_edit_insert.ts | 15 ++++- src/services/tools/file_edit_operation.ts | 20 +++++- src/services/tools/file_edit_replace.test.ts | 4 +- src/services/tools/file_read.test.ts | 49 ++++++++++----- src/services/tools/file_read.ts | 15 ++++- 8 files changed, 203 insertions(+), 40 deletions(-) diff --git a/src/services/tools/fileCommon.test.ts b/src/services/tools/fileCommon.test.ts index 835f9a514..2b3e99ec0 100644 --- a/src/services/tools/fileCommon.test.ts +++ b/src/services/tools/fileCommon.test.ts @@ -1,6 +1,11 @@ import { describe, it, expect } from "bun:test"; import type { FileStat } from "@/runtime/Runtime"; -import { validatePathInCwd, validateFileSize, MAX_FILE_SIZE } from "./fileCommon"; +import { + validatePathInCwd, + validateFileSize, + validateNoRedundantPrefix, + MAX_FILE_SIZE, +} from "./fileCommon"; import { createRuntime } from "@/runtime/runtimeFactory"; describe("fileCommon", () => { @@ -131,4 +136,63 @@ describe("fileCommon", () => { expect(result).not.toBeNull(); }); }); + + describe("validateNoRedundantPrefix", () => { + const cwd = "/workspace/project"; + const runtime = createRuntime({ type: "local", srcBaseDir: cwd }); + + it("should allow relative paths", () => { + expect(validateNoRedundantPrefix("src/file.ts", cwd, runtime)).toBeNull(); + expect(validateNoRedundantPrefix("./src/file.ts", cwd, runtime)).toBeNull(); + expect(validateNoRedundantPrefix("file.ts", cwd, runtime)).toBeNull(); + }); + + it("should reject absolute paths that contain the cwd prefix", () => { + const result = validateNoRedundantPrefix("/workspace/project/src/file.ts", cwd, runtime); + expect(result).not.toBeNull(); + expect(result?.error).toContain("Redundant path prefix detected"); + expect(result?.error).toContain("Please use relative paths to save tokens"); + expect(result?.error).toContain("src/file.ts"); // Should suggest the relative path + }); + + it("should reject absolute paths at the cwd root", () => { + const result = validateNoRedundantPrefix("/workspace/project/file.ts", cwd, runtime); + expect(result).not.toBeNull(); + expect(result?.error).toContain("Redundant path prefix detected"); + expect(result?.error).toContain("file.ts"); // Should suggest the relative path + }); + + it("should allow absolute paths outside cwd (they will be caught by validatePathInCwd)", () => { + // This validation only catches redundant prefixes, not paths outside cwd + expect(validateNoRedundantPrefix("/etc/passwd", cwd, runtime)).toBeNull(); + expect(validateNoRedundantPrefix("/home/user/file.ts", cwd, runtime)).toBeNull(); + }); + + it("should handle paths with ..", () => { + // Relative paths with .. are fine for this check + expect(validateNoRedundantPrefix("../outside.ts", cwd, runtime)).toBeNull(); + expect(validateNoRedundantPrefix("src/../../outside.ts", cwd, runtime)).toBeNull(); + }); + + it("should work with cwd that has trailing slash", () => { + const cwdWithSlash = "/workspace/project/"; + const result = validateNoRedundantPrefix( + "/workspace/project/src/file.ts", + cwdWithSlash, + runtime + ); + expect(result).not.toBeNull(); + expect(result?.error).toContain("src/file.ts"); + }); + + it("should handle nested paths correctly", () => { + const result = validateNoRedundantPrefix( + "/workspace/project/src/components/Button/index.ts", + cwd, + runtime + ); + expect(result).not.toBeNull(); + expect(result?.error).toContain("src/components/Button/index.ts"); + }); + }); }); diff --git a/src/services/tools/fileCommon.ts b/src/services/tools/fileCommon.ts index e5f616337..1c752895b 100644 --- a/src/services/tools/fileCommon.ts +++ b/src/services/tools/fileCommon.ts @@ -48,6 +48,46 @@ export function validateFileSize(stats: FileStat): { error: string } | null { return null; } +/** + * Validates that a file path doesn't contain redundant workspace prefix. + * Returns an error object if the path contains the cwd prefix, null if valid. + * This helps save tokens by encouraging relative paths. + * + * @param filePath - The file path to validate + * @param cwd - The working directory + * @param runtime - The runtime (skip check for SSH since paths are remote) + * @returns Error object if redundant prefix found, null if valid + */ +export function validateNoRedundantPrefix( + filePath: string, + cwd: string, + runtime: Runtime +): { error: string } | null { + // Skip for SSH runtimes - remote paths don't apply + if (runtime instanceof SSHRuntime) { + return null; + } + + // Check if the path contains the cwd as a prefix (indicating redundancy) + // This catches cases like "/workspace/project/src/file.ts" when cwd is "/workspace/project" + const normalizedPath = path.normalize(filePath); + const normalizedCwd = path.normalize(cwd); + + // Only check absolute paths - relative paths are fine + if (path.isAbsolute(normalizedPath)) { + // Check if the absolute path starts with the cwd + if (normalizedPath.startsWith(normalizedCwd)) { + // Calculate what the relative path would be + const relativePath = path.relative(normalizedCwd, normalizedPath); + return { + error: `Redundant path prefix detected. The path '${filePath}' contains the workspace directory. Please use relative paths to save tokens: '${relativePath}'`, + }; + } + } + + return null; +} + /** * Validates that a file path is within the allowed working directory. * Returns an error object if the path is outside cwd, null if valid. diff --git a/src/services/tools/file_edit_insert.test.ts b/src/services/tools/file_edit_insert.test.ts index 562f50be1..2c429c777 100644 --- a/src/services/tools/file_edit_insert.test.ts +++ b/src/services/tools/file_edit_insert.test.ts @@ -55,7 +55,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 0, content: "INSERTED", }; @@ -78,7 +78,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 1, content: "INSERTED", }; @@ -101,7 +101,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 2, content: "INSERTED", }; @@ -124,7 +124,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 3, content: "INSERTED", }; @@ -147,7 +147,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 1, content: "INSERTED1\nINSERTED2", }; @@ -169,7 +169,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 0, content: "INSERTED", }; @@ -186,12 +186,10 @@ describe("file_edit_insert tool", () => { it("should fail when file does not exist and create is not set", async () => { // Setup - const nonExistentPath = path.join(testDir, "nonexistent.txt"); - using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: nonExistentPath, + file_path: "nonexistent.txt", // Use relative path line_offset: 0, content: "INSERTED", }; @@ -209,15 +207,13 @@ describe("file_edit_insert tool", () => { it("should create file when create is true and file does not exist", async () => { // Setup - const nonExistentPath = path.join(testDir, "newfile.txt"); - const tool = createFileEditInsertTool({ cwd: testDir, runtime: createRuntime({ type: "local", srcBaseDir: "/tmp" }), runtimeTempDir: "/tmp", }); const args: FileEditInsertToolArgs = { - file_path: nonExistentPath, + file_path: "newfile.txt", // Use relative path line_offset: 0, content: "INSERTED", create: true, @@ -229,21 +225,19 @@ describe("file_edit_insert tool", () => { // Assert expect(result.success).toBe(true); - const fileContent = await fs.readFile(nonExistentPath, "utf-8"); + const fileContent = await fs.readFile(path.join(testDir, "newfile.txt"), "utf-8"); expect(fileContent).toBe("INSERTED\n"); }); it("should create parent directories when create is true", async () => { // Setup - const nestedPath = path.join(testDir, "nested", "dir", "newfile.txt"); - const tool = createFileEditInsertTool({ cwd: testDir, runtime: createRuntime({ type: "local", srcBaseDir: "/tmp" }), runtimeTempDir: "/tmp", }); const args: FileEditInsertToolArgs = { - file_path: nestedPath, + file_path: "nested/dir/newfile.txt", // Use relative path line_offset: 0, content: "INSERTED", create: true, @@ -255,7 +249,7 @@ describe("file_edit_insert tool", () => { // Assert expect(result.success).toBe(true); - const fileContent = await fs.readFile(nestedPath, "utf-8"); + const fileContent = await fs.readFile(path.join(testDir, "nested/dir/newfile.txt"), "utf-8"); expect(fileContent).toBe("INSERTED\n"); }); @@ -270,7 +264,7 @@ describe("file_edit_insert tool", () => { runtimeTempDir: "/tmp", }); const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 1, content: "INSERTED", create: true, @@ -294,7 +288,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: -1, content: "INSERTED", }; @@ -317,7 +311,7 @@ describe("file_edit_insert tool", () => { using testEnv = createTestFileEditInsertTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileEditInsertToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path line_offset: 10, // File only has 2 lines content: "INSERTED", }; diff --git a/src/services/tools/file_edit_insert.ts b/src/services/tools/file_edit_insert.ts index 00fdb2b14..e65a376a9 100644 --- a/src/services/tools/file_edit_insert.ts +++ b/src/services/tools/file_edit_insert.ts @@ -2,7 +2,7 @@ import { tool } from "ai"; import type { FileEditInsertToolResult } from "@/types/tools"; import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; -import { validatePathInCwd } from "./fileCommon"; +import { validatePathInCwd, validateNoRedundantPrefix } from "./fileCommon"; import { WRITE_DENIED_PREFIX } from "@/types/tools"; import { executeFileEditOperation } from "./file_edit_operation"; import { RuntimeError } from "@/runtime/Runtime"; @@ -25,6 +25,19 @@ export const createFileEditInsertTool: ToolFactory = (config: ToolConfiguration) create, }): Promise => { try { + // Validate no redundant path prefix (must come first to catch absolute paths) + const redundantPrefixValidation = validateNoRedundantPrefix( + file_path, + config.cwd, + config.runtime + ); + if (redundantPrefixValidation) { + return { + success: false, + error: redundantPrefixValidation.error, + }; + } + const pathValidation = validatePathInCwd(file_path, config.cwd, config.runtime); if (pathValidation) { return { diff --git a/src/services/tools/file_edit_operation.ts b/src/services/tools/file_edit_operation.ts index 5b16d08c6..5c06e3902 100644 --- a/src/services/tools/file_edit_operation.ts +++ b/src/services/tools/file_edit_operation.ts @@ -1,7 +1,12 @@ import type { FileEditDiffSuccessBase, FileEditErrorResult } from "@/types/tools"; import { WRITE_DENIED_PREFIX } from "@/types/tools"; import type { ToolConfiguration } from "@/utils/tools/tools"; -import { generateDiff, validateFileSize, validatePathInCwd } from "./fileCommon"; +import { + generateDiff, + validateFileSize, + validatePathInCwd, + validateNoRedundantPrefix, +} from "./fileCommon"; import { RuntimeError } from "@/runtime/Runtime"; import { readFileString, writeFileString } from "@/utils/runtime/helpers"; @@ -36,6 +41,19 @@ export async function executeFileEditOperation({ FileEditErrorResult | (FileEditDiffSuccessBase & TMetadata) > { try { + // Validate no redundant path prefix (must come first to catch absolute paths) + const redundantPrefixValidation = validateNoRedundantPrefix( + filePath, + config.cwd, + config.runtime + ); + if (redundantPrefixValidation) { + return { + success: false, + error: `${WRITE_DENIED_PREFIX} ${redundantPrefixValidation.error}`, + }; + } + const pathValidation = validatePathInCwd(filePath, config.cwd, config.runtime); if (pathValidation) { return { diff --git a/src/services/tools/file_edit_replace.test.ts b/src/services/tools/file_edit_replace.test.ts index 6f3d62217..da76ac87f 100644 --- a/src/services/tools/file_edit_replace.test.ts +++ b/src/services/tools/file_edit_replace.test.ts @@ -64,7 +64,7 @@ describe("file_edit_replace_string tool", () => { }); const payload: FileEditReplaceStringToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path old_string: "Hello world", new_string: "Hello universe", }; @@ -102,7 +102,7 @@ describe("file_edit_replace_lines tool", () => { }); const payload: FileEditReplaceLinesToolArgs = { - file_path: testFilePath, + file_path: "test.txt", // Use relative path start_line: 2, end_line: 3, new_lines: ["LINE2", "LINE3"], diff --git a/src/services/tools/file_read.test.ts b/src/services/tools/file_read.test.ts index 5bd855ade..bd061552e 100644 --- a/src/services/tools/file_read.test.ts +++ b/src/services/tools/file_read.test.ts @@ -55,7 +55,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path }; // Execute @@ -78,7 +78,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path offset: 3, // Start from line 3 }; @@ -101,7 +101,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path limit: 2, // Read only first 2 lines }; @@ -124,7 +124,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path offset: 2, // Start from line 2 limit: 2, // Read 2 lines }; @@ -148,7 +148,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path }; // Execute @@ -169,7 +169,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path }; // Execute @@ -185,12 +185,10 @@ describe("file_read tool", () => { it("should fail when file does not exist", async () => { // Setup - const nonExistentPath = path.join(testDir, "nonexistent.txt"); - using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: nonExistentPath, + filePath: "nonexistent.txt", // Use relative path }; // Execute @@ -211,7 +209,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path offset: 10, // Beyond file length }; @@ -234,7 +232,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path }; // Execute @@ -261,7 +259,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path }; // Execute @@ -285,7 +283,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path }; // Execute @@ -308,7 +306,7 @@ describe("file_read tool", () => { using testEnv = createTestFileReadTool({ cwd: testDir }); const tool = testEnv.tool; const args: FileReadToolArgs = { - filePath: testFilePath, + filePath: "test.txt", // Use relative path limit: 500, // Read only 500 lines }; @@ -322,6 +320,29 @@ describe("file_read tool", () => { } }); + it("should reject absolute paths containing the workspace directory", async () => { + // Setup + const content = "test content"; + await fs.writeFile(testFilePath, content); + + using testEnv = createTestFileReadTool({ cwd: testDir }); + const tool = testEnv.tool; + const args: FileReadToolArgs = { + filePath: testFilePath, // Absolute path containing cwd prefix + }; + + // Execute + const result = (await tool.execute!(args, mockToolCallOptions)) as FileReadToolResult; + + // Assert + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant path prefix detected"); + expect(result.error).toContain("Please use relative paths to save tokens"); + expect(result.error).toContain("test.txt"); // Should suggest the relative path + } + }); + it("should reject reading files outside cwd using ..", async () => { // Setup - create a file in testDir const content = "secret content"; diff --git a/src/services/tools/file_read.ts b/src/services/tools/file_read.ts index 34f172169..bba7b8a46 100644 --- a/src/services/tools/file_read.ts +++ b/src/services/tools/file_read.ts @@ -2,7 +2,7 @@ import { tool } from "ai"; import type { FileReadToolResult } from "@/types/tools"; import type { ToolConfiguration, ToolFactory } from "@/utils/tools/tools"; import { TOOL_DEFINITIONS } from "@/utils/tools/toolDefinitions"; -import { validatePathInCwd, validateFileSize } from "./fileCommon"; +import { validatePathInCwd, validateFileSize, validateNoRedundantPrefix } from "./fileCommon"; import { RuntimeError } from "@/runtime/Runtime"; import { readFileString } from "@/utils/runtime/helpers"; @@ -21,6 +21,19 @@ export const createFileReadTool: ToolFactory = (config: ToolConfiguration) => { ): Promise => { // Note: abortSignal available but not used - file reads are fast and complete quickly try { + // Validate no redundant path prefix (must come first to catch absolute paths) + const redundantPrefixValidation = validateNoRedundantPrefix( + filePath, + config.cwd, + config.runtime + ); + if (redundantPrefixValidation) { + return { + success: false, + error: redundantPrefixValidation.error, + }; + } + // Validate that the path is within the working directory const pathValidation = validatePathInCwd(filePath, config.cwd, config.runtime); if (pathValidation) { From c354cd8008b360b4f1b3b3b0a721f964b20b9b3d Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:37:27 +0000 Subject: [PATCH 11/21] =?UTF-8?q?=F0=9F=A4=96=20Fix:=20Catch=20runtime=20c?= =?UTF-8?q?reation=20errors=20in=20WORKSPACE=5FCREATE=20handler?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The WORKSPACE_CREATE IPC handler wasn't catching errors thrown during runtime creation (e.g., validation errors for tilde paths in SSH runtime). This caused tests to fail with unhandled exceptions instead of returning { success: false, error: ... } as expected. Wrap createRuntime() call in try-catch to properly handle validation errors and return them through the IPC boundary. Fixes integration test: 'rejects tilde paths in srcBaseDir (SSH only)' _Generated with `cmux`_ --- src/services/ipcMain.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/services/ipcMain.ts b/src/services/ipcMain.ts index 3bfcdf2a3..77b40dc93 100644 --- a/src/services/ipcMain.ts +++ b/src/services/ipcMain.ts @@ -283,7 +283,15 @@ export class IpcMain { type: "local", srcBaseDir: this.config.srcDir, }; - const runtime = createRuntime(finalRuntimeConfig); + + // Create runtime - catch validation errors (e.g., tilde paths in SSH) + let runtime; + try { + runtime = createRuntime(finalRuntimeConfig); + } catch (error) { + const errorMsg = error instanceof Error ? error.message : String(error); + return { success: false, error: errorMsg }; + } // Create session BEFORE starting init so events can be forwarded const session = this.getOrCreateSession(workspaceId); From b644868d3181bbe160f0573d083fe7dedde6a68c Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:46:07 +0000 Subject: [PATCH 12/21] =?UTF-8?q?=F0=9F=A4=96=20Enable=20redundant=20path?= =?UTF-8?q?=20prefix=20validation=20for=20SSH=20runtime?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, validateNoRedundantPrefix() skipped validation for SSH runtimes under the assumption that remote paths needed different handling. However, the validation is equally valuable for SSH - absolute paths like '/home/user/cmux/project/branch/src/file.ts' are just as redundant as '/workspace/project/src/file.ts' on local runtime. Changes: - Remove SSH runtime bypass in validateNoRedundantPrefix() - Use simple string matching instead of Node's path module (works for both) - Handle trailing slashes and partial directory name matches correctly - Add comprehensive tests for SSH runtime validation - Add edge case tests (path equals cwd, partial name matches) The validation now saves tokens on both local and SSH runtimes by encouraging agents to use relative paths consistently. _Generated with `cmux`_ --- src/services/tools/fileCommon.test.ts | 36 ++++++++++++++++++++++++ src/services/tools/fileCommon.ts | 40 +++++++++++++++------------ 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/src/services/tools/fileCommon.test.ts b/src/services/tools/fileCommon.test.ts index 2b3e99ec0..a0f5f5822 100644 --- a/src/services/tools/fileCommon.test.ts +++ b/src/services/tools/fileCommon.test.ts @@ -194,5 +194,41 @@ describe("fileCommon", () => { expect(result).not.toBeNull(); expect(result?.error).toContain("src/components/Button/index.ts"); }); + + it("should reject path that equals cwd exactly", () => { + const result = validateNoRedundantPrefix("/workspace/project", cwd, runtime); + expect(result).not.toBeNull(); + expect(result?.error).toContain("Redundant path prefix detected"); + expect(result?.error).toContain("."); // Should suggest current directory + }); + + it("should not match partial directory names", () => { + // /workspace/project2 should NOT match /workspace/project + expect(validateNoRedundantPrefix("/workspace/project2/file.ts", cwd, runtime)).toBeNull(); + expect(validateNoRedundantPrefix("/workspace/project-old/file.ts", cwd, runtime)).toBeNull(); + }); + + it("should work with SSH runtime", () => { + const sshRuntime = createRuntime({ + type: "ssh", + host: "user@localhost", + srcBaseDir: "/home/user/cmux", + identityFile: "/tmp/fake-key", + }); + const sshCwd = "/home/user/cmux/project/branch"; + + // Should reject absolute paths with redundant prefix on SSH too + const result = validateNoRedundantPrefix( + "/home/user/cmux/project/branch/src/file.ts", + sshCwd, + sshRuntime + ); + expect(result).not.toBeNull(); + expect(result?.error).toContain("Redundant path prefix detected"); + expect(result?.error).toContain("src/file.ts"); + + // Should allow relative paths on SSH + expect(validateNoRedundantPrefix("src/file.ts", sshCwd, sshRuntime)).toBeNull(); + }); }); }); diff --git a/src/services/tools/fileCommon.ts b/src/services/tools/fileCommon.ts index 1c752895b..4bdbb1b37 100644 --- a/src/services/tools/fileCommon.ts +++ b/src/services/tools/fileCommon.ts @@ -53,9 +53,12 @@ export function validateFileSize(stats: FileStat): { error: string } | null { * Returns an error object if the path contains the cwd prefix, null if valid. * This helps save tokens by encouraging relative paths. * + * Works for both local and SSH runtimes by using simple string matching + * for absolute paths instead of Node's path module (which only handles local paths). + * * @param filePath - The file path to validate * @param cwd - The working directory - * @param runtime - The runtime (skip check for SSH since paths are remote) + * @param runtime - The runtime (unused, kept for consistency) * @returns Error object if redundant prefix found, null if valid */ export function validateNoRedundantPrefix( @@ -63,26 +66,29 @@ export function validateNoRedundantPrefix( cwd: string, runtime: Runtime ): { error: string } | null { - // Skip for SSH runtimes - remote paths don't apply - if (runtime instanceof SSHRuntime) { + // Only check absolute paths (start with /) - relative paths are fine + // This works for both local and SSH since both use Unix-style paths + if (!filePath.startsWith("/")) { return null; } - // Check if the path contains the cwd as a prefix (indicating redundancy) - // This catches cases like "/workspace/project/src/file.ts" when cwd is "/workspace/project" - const normalizedPath = path.normalize(filePath); - const normalizedCwd = path.normalize(cwd); + // Normalize both paths: remove trailing slashes for consistent comparison + const normalizedPath = filePath.replace(/\/+$/, ""); + const normalizedCwd = cwd.replace(/\/+$/, ""); - // Only check absolute paths - relative paths are fine - if (path.isAbsolute(normalizedPath)) { - // Check if the absolute path starts with the cwd - if (normalizedPath.startsWith(normalizedCwd)) { - // Calculate what the relative path would be - const relativePath = path.relative(normalizedCwd, normalizedPath); - return { - error: `Redundant path prefix detected. The path '${filePath}' contains the workspace directory. Please use relative paths to save tokens: '${relativePath}'`, - }; - } + // Check if the absolute path starts with the cwd + // Use startsWith + check for path separator to avoid partial matches + // e.g., /workspace/project should match /workspace/project/src but not /workspace/project2 + if ( + normalizedPath === normalizedCwd || + normalizedPath.startsWith(normalizedCwd + "/") + ) { + // Calculate what the relative path would be + const relativePath = + normalizedPath === normalizedCwd ? "." : normalizedPath.substring(normalizedCwd.length + 1); + return { + error: `Redundant path prefix detected. The path '${filePath}' contains the workspace directory. Please use relative paths to save tokens: '${relativePath}'`, + }; } return null; From 302cbe7136ad133f9c0f71637150680f6fc748d5 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:48:52 +0000 Subject: [PATCH 13/21] =?UTF-8?q?=F0=9F=A4=96=20Fix:=20Mark=20unused=20run?= =?UTF-8?q?time=20parameter=20with=20underscore=20prefix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ESLint requires unused parameters to be prefixed with underscore. The runtime parameter is kept for API consistency even though it's not used after removing the SSH bypass. _Generated with `cmux`_ --- src/services/tools/fileCommon.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/services/tools/fileCommon.ts b/src/services/tools/fileCommon.ts index 4bdbb1b37..a1e877a6f 100644 --- a/src/services/tools/fileCommon.ts +++ b/src/services/tools/fileCommon.ts @@ -64,7 +64,7 @@ export function validateFileSize(stats: FileStat): { error: string } | null { export function validateNoRedundantPrefix( filePath: string, cwd: string, - runtime: Runtime + _runtime: Runtime ): { error: string } | null { // Only check absolute paths (start with /) - relative paths are fine // This works for both local and SSH since both use Unix-style paths @@ -79,10 +79,7 @@ export function validateNoRedundantPrefix( // Check if the absolute path starts with the cwd // Use startsWith + check for path separator to avoid partial matches // e.g., /workspace/project should match /workspace/project/src but not /workspace/project2 - if ( - normalizedPath === normalizedCwd || - normalizedPath.startsWith(normalizedCwd + "/") - ) { + if (normalizedPath === normalizedCwd || normalizedPath.startsWith(normalizedCwd + "/")) { // Calculate what the relative path would be const relativePath = normalizedPath === normalizedCwd ? "." : normalizedPath.substring(normalizedCwd.length + 1); From b49b5a904d5342e0969bbc6bcd1904dfca51f767 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 15:53:12 +0000 Subject: [PATCH 14/21] =?UTF-8?q?=F0=9F=A4=96=20Use=20runtime.normalizePat?= =?UTF-8?q?h()=20for=20DRY=20in=20validateNoRedundantPrefix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of manually normalizing paths with string operations, leverage the runtime's existing normalizePath() method for consistent path handling. This reduces duplication and ensures we handle paths the same way the runtime does. Changes: - Use runtime.normalizePath(".", cwd) to normalize the working directory - Keeps manual trailing slash cleanup for absolute paths (can't use normalizePath directly since it's designed for relative path resolution) - Update documentation to reflect use of runtime method - Remove underscore prefix from runtime param (now used) _Generated with `cmux`_ --- src/services/tools/fileCommon.ts | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/services/tools/fileCommon.ts b/src/services/tools/fileCommon.ts index a1e877a6f..7b726d33a 100644 --- a/src/services/tools/fileCommon.ts +++ b/src/services/tools/fileCommon.ts @@ -53,18 +53,18 @@ export function validateFileSize(stats: FileStat): { error: string } | null { * Returns an error object if the path contains the cwd prefix, null if valid. * This helps save tokens by encouraging relative paths. * - * Works for both local and SSH runtimes by using simple string matching - * for absolute paths instead of Node's path module (which only handles local paths). + * Works for both local and SSH runtimes by using runtime.normalizePath() + * for consistent path handling across different runtime types. * * @param filePath - The file path to validate * @param cwd - The working directory - * @param runtime - The runtime (unused, kept for consistency) + * @param runtime - The runtime to use for path normalization * @returns Error object if redundant prefix found, null if valid */ export function validateNoRedundantPrefix( filePath: string, cwd: string, - _runtime: Runtime + runtime: Runtime ): { error: string } | null { // Only check absolute paths (start with /) - relative paths are fine // This works for both local and SSH since both use Unix-style paths @@ -72,17 +72,22 @@ export function validateNoRedundantPrefix( return null; } - // Normalize both paths: remove trailing slashes for consistent comparison + // Use runtime's normalizePath to ensure consistent handling across local and SSH + // Normalize the cwd to get canonical form (removes trailing slashes, etc.) + const normalizedCwd = runtime.normalizePath(".", cwd); + + // For absolute paths, we can't use normalizePath directly (it resolves relative paths) + // so just clean up trailing slashes manually const normalizedPath = filePath.replace(/\/+$/, ""); - const normalizedCwd = cwd.replace(/\/+$/, ""); + const cleanCwd = normalizedCwd.replace(/\/+$/, ""); // Check if the absolute path starts with the cwd // Use startsWith + check for path separator to avoid partial matches // e.g., /workspace/project should match /workspace/project/src but not /workspace/project2 - if (normalizedPath === normalizedCwd || normalizedPath.startsWith(normalizedCwd + "/")) { + if (normalizedPath === cleanCwd || normalizedPath.startsWith(cleanCwd + "/")) { // Calculate what the relative path would be const relativePath = - normalizedPath === normalizedCwd ? "." : normalizedPath.substring(normalizedCwd.length + 1); + normalizedPath === cleanCwd ? "." : normalizedPath.substring(cleanCwd.length + 1); return { error: `Redundant path prefix detected. The path '${filePath}' contains the workspace directory. Please use relative paths to save tokens: '${relativePath}'`, }; From 2956db093a07dca183fe0ae3a84076852150c7af Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 16:16:58 +0000 Subject: [PATCH 15/21] =?UTF-8?q?=F0=9F=A4=96=20Add=20resolvePath()=20to?= =?UTF-8?q?=20Runtime=20interface=20for=20tilde=20expansion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add resolvePath() method to Runtime interface - LocalRuntime: Expands tildes using expandTilde() and verifies path exists - SSHRuntime: Uses remote 'realpath' command to expand and verify paths - WORKSPACE_CREATE: Resolves srcBaseDir before creating runtime and storing in config - Remove tilde validation from SSHRuntime constructor (now handled by resolvePath) - Both tilde (~) and non-tilde paths now work for workspace creation - Add comprehensive unit tests for both runtimes This enables storing resolved paths in config, ensuring consistency across local and SSH runtimes regardless of how the path was originally specified. Generated with `cmux` --- src/runtime/LocalRuntime.test.ts | 37 +++++++++++++++++ src/runtime/LocalRuntime.ts | 20 +++++++++ src/runtime/Runtime.ts | 18 ++++++++ src/runtime/SSHRuntime.test.ts | 70 ++++++++++++++++++++++++++++++-- src/runtime/SSHRuntime.ts | 69 +++++++++++++++++++++++++++---- src/services/gitService.test.ts | 3 ++ src/services/ipcMain.ts | 18 +++++++- 7 files changed, 221 insertions(+), 14 deletions(-) diff --git a/src/runtime/LocalRuntime.test.ts b/src/runtime/LocalRuntime.test.ts index 03abda812..f191a167d 100644 --- a/src/runtime/LocalRuntime.test.ts +++ b/src/runtime/LocalRuntime.test.ts @@ -29,3 +29,40 @@ describe("LocalRuntime constructor", () => { expect(workspacePath).toBe(expected); }); }); + +describe("LocalRuntime.resolvePath", () => { + it("should expand tilde to home directory", async () => { + const runtime = new LocalRuntime("/tmp"); + const resolved = await runtime.resolvePath("~"); + expect(resolved).toBe(os.homedir()); + }); + + it("should expand tilde with path", async () => { + const runtime = new LocalRuntime("/tmp"); + // Use a path that likely exists (or use /tmp if ~ doesn't have subdirs) + const resolved = await runtime.resolvePath("~/.."); + const expected = path.dirname(os.homedir()); + expect(resolved).toBe(expected); + }); + + it("should resolve absolute paths", async () => { + const runtime = new LocalRuntime("/tmp"); + const resolved = await runtime.resolvePath("/tmp"); + expect(resolved).toBe("/tmp"); + }); + + it("should reject non-existent paths", async () => { + const runtime = new LocalRuntime("/tmp"); + // eslint-disable-next-line @typescript-eslint/await-thenable + await expect(runtime.resolvePath("/this/path/does/not/exist/12345")).rejects.toThrow( + /Cannot resolve path/ + ); + }); + + it("should resolve relative paths from cwd", async () => { + const runtime = new LocalRuntime("/tmp"); + const resolved = await runtime.resolvePath("."); + // Should resolve to absolute path + expect(path.isAbsolute(resolved)).toBe(true); + }); +}); diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index 945eb2c97..10217aa08 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -301,6 +301,26 @@ export class LocalRuntime implements Runtime { } } + async resolvePath(filePath: string): Promise { + // Expand tilde to actual home directory path + const expanded = expandTilde(filePath); + + // Resolve to absolute path (handles relative paths like "./foo") + const resolved = path.resolve(expanded); + + // Verify path exists + try { + await fsPromises.access(resolved); + return resolved; + } catch (err) { + throw new RuntimeErrorClass( + `Cannot resolve path '${filePath}': ${getErrorMessage(err)}`, + "file_io", + err instanceof Error ? err : undefined + ); + } + } + normalizePath(targetPath: string, basePath: string): string { // For local runtime, use Node.js path resolution // Handle special case: current directory diff --git a/src/runtime/Runtime.ts b/src/runtime/Runtime.ts index 47c5e83ad..18f11ade5 100644 --- a/src/runtime/Runtime.ts +++ b/src/runtime/Runtime.ts @@ -195,6 +195,24 @@ export interface Runtime { */ stat(path: string): Promise; + /** + * Resolve a path to its absolute, canonical form (expanding tildes, resolving symlinks, etc.). + * This is used at workspace creation time to normalize srcBaseDir paths in config. + * + * @param path Path to resolve (may contain tildes or be relative) + * @returns Promise resolving to absolute path + * @throws RuntimeError if path cannot be resolved (e.g., doesn't exist, permission denied) + * + * @example + * // LocalRuntime + * await runtime.resolvePath("~/cmux") // => "/home/user/cmux" + * await runtime.resolvePath("./relative") // => "/current/dir/relative" + * + * // SSHRuntime + * await runtime.resolvePath("~/cmux") // => "/home/user/cmux" (via SSH shell expansion) + */ + resolvePath(path: string): Promise; + /** * Normalize a path for comparison purposes within this runtime's context. * Handles runtime-specific path semantics (local vs remote). diff --git a/src/runtime/SSHRuntime.test.ts b/src/runtime/SSHRuntime.test.ts index 2f65c5223..57f2e6a16 100644 --- a/src/runtime/SSHRuntime.test.ts +++ b/src/runtime/SSHRuntime.test.ts @@ -2,22 +2,24 @@ import { describe, expect, it } from "bun:test"; import { SSHRuntime } from "./SSHRuntime"; describe("SSHRuntime constructor", () => { - it("should reject tilde in srcBaseDir", () => { + it("should accept tilde in srcBaseDir", () => { + // Tildes are now allowed - they will be resolved via resolvePath() expect(() => { new SSHRuntime({ host: "example.com", srcBaseDir: "~/cmux", }); - }).toThrow(/cannot start with tilde/); + }).not.toThrow(); }); - it("should reject bare tilde in srcBaseDir", () => { + it("should accept bare tilde in srcBaseDir", () => { + // Tildes are now allowed - they will be resolved via resolvePath() expect(() => { new SSHRuntime({ host: "example.com", srcBaseDir: "~", }); - }).toThrow(/cannot start with tilde/); + }).not.toThrow(); }); it("should accept absolute paths in srcBaseDir", () => { @@ -29,3 +31,63 @@ describe("SSHRuntime constructor", () => { }).not.toThrow(); }); }); + +describe("SSHRuntime.resolvePath", () => { + // Note: These tests require TEST_INTEGRATION=1 to run with actual SSH connection + const isIntegrationTest = process.env.TEST_INTEGRATION === "1"; + const describeIfIntegration = isIntegrationTest ? describe : describe.skip; + + describeIfIntegration("with SSH connection", () => { + it("should expand tilde to home directory", async () => { + const runtime = new SSHRuntime({ + host: process.env.SSH_HOST ?? "localhost", + port: parseInt(process.env.SSH_PORT ?? "2222"), + identityFile: process.env.SSH_IDENTITY_FILE, + srcBaseDir: "~/test-workspace", + }); + + const resolved = await runtime.resolvePath("~"); + // Should be an absolute path + expect(resolved).toMatch(/^\/home\//); + }); + + it("should expand tilde with path", async () => { + const runtime = new SSHRuntime({ + host: process.env.SSH_HOST ?? "localhost", + port: parseInt(process.env.SSH_PORT ?? "2222"), + identityFile: process.env.SSH_IDENTITY_FILE, + srcBaseDir: "~/test-workspace", + }); + + const resolved = await runtime.resolvePath("~/.."); + // Should be parent of home directory + expect(resolved).toBe("/home"); + }); + + it("should resolve absolute paths", async () => { + const runtime = new SSHRuntime({ + host: process.env.SSH_HOST ?? "localhost", + port: parseInt(process.env.SSH_PORT ?? "2222"), + identityFile: process.env.SSH_IDENTITY_FILE, + srcBaseDir: "/tmp", + }); + + const resolved = await runtime.resolvePath("/tmp"); + expect(resolved).toBe("/tmp"); + }); + + it("should reject non-existent paths", async () => { + const runtime = new SSHRuntime({ + host: process.env.SSH_HOST ?? "localhost", + port: parseInt(process.env.SSH_PORT ?? "2222"), + identityFile: process.env.SSH_IDENTITY_FILE, + srcBaseDir: "/tmp", + }); + + // eslint-disable-next-line @typescript-eslint/await-thenable + await expect(runtime.resolvePath("/this/path/does/not/exist/12345")).rejects.toThrow( + /Failed to resolve path/ + ); + }); + }); +}); diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index b2bbc37ee..66e308804 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -59,15 +59,8 @@ export class SSHRuntime implements Runtime { private readonly controlPath: string; constructor(config: SSHRuntimeConfig) { - // Reject tilde paths - require explicit full paths for SSH - // Rationale: Simplifies logic and avoids ambiguity about which user's home directory - if (config.srcBaseDir.startsWith("~")) { - throw new Error( - `SSH runtime srcBaseDir cannot start with tilde. ` + - `Use full path (e.g., /home/username/cmux instead of ~/cmux)` - ); - } - + // Note: srcBaseDir may contain tildes - they will be resolved via resolvePath() before use + // The WORKSPACE_CREATE IPC handler resolves paths before storing in config this.config = config; // Get deterministic controlPath from connection pool // Multiple SSHRuntime instances with same config share the same controlPath, @@ -324,6 +317,64 @@ export class SSHRuntime implements Runtime { isDirectory: fileType === "directory", }; } + async resolvePath(filePath: string): Promise { + // Use shell to expand tildes and resolve path on remote system + // We use `realpath` which resolves symlinks and gives canonical path + // Note: realpath also verifies the path exists + const command = `realpath ${shescape.quote(filePath)}`; + const sshArgs = this.buildSSHArgs(); + sshArgs.push(this.config.host, command); + + return new Promise((resolve, reject) => { + const proc = spawn("ssh", sshArgs); + let stdout = ""; + let stderr = ""; + + proc.stdout?.on("data", (data: Buffer) => { + stdout += data.toString(); + }); + + proc.stderr?.on("data", (data: Buffer) => { + stderr += data.toString(); + }); + + proc.on("close", (code) => { + if (code !== 0) { + reject( + new RuntimeErrorClass( + `Failed to resolve path '${filePath}' on remote host: ${stderr.trim()}`, + "file_io" + ) + ); + return; + } + + const resolved = stdout.trim(); + if (!resolved?.startsWith("/")) { + reject( + new RuntimeErrorClass( + `Invalid resolved path '${resolved}' for '${filePath}' (expected absolute path)`, + "file_io" + ) + ); + return; + } + + resolve(resolved); + }); + + proc.on("error", (err) => { + reject( + new RuntimeErrorClass( + `Cannot resolve path '${filePath}' on remote host: ${getErrorMessage(err)}`, + "network", + err instanceof Error ? err : undefined + ) + ); + }); + }); + } + normalizePath(targetPath: string, basePath: string): string { // For SSH, handle paths in a POSIX-like manner without accessing the remote filesystem const target = targetPath.trim(); diff --git a/src/services/gitService.test.ts b/src/services/gitService.test.ts index 237b68989..7a578e01b 100644 --- a/src/services/gitService.test.ts +++ b/src/services/gitService.test.ts @@ -91,6 +91,9 @@ describe("removeWorktreeSafe", () => { const result = await createWorktree(mockConfig, repoPath, "dirty-branch", { trunkBranch: defaultBranch, }); + if (!result.success) { + console.error("createWorktree failed:", result.error); + } expect(result.success).toBe(true); const worktreePath = result.path!; diff --git a/src/services/ipcMain.ts b/src/services/ipcMain.ts index 77b40dc93..5b5c37380 100644 --- a/src/services/ipcMain.ts +++ b/src/services/ipcMain.ts @@ -284,10 +284,26 @@ export class IpcMain { srcBaseDir: this.config.srcDir, }; - // Create runtime - catch validation errors (e.g., tilde paths in SSH) + // Create temporary runtime to resolve srcBaseDir path + // This allows tilde paths to work for both local and SSH runtimes let runtime; + let resolvedSrcBaseDir: string; try { runtime = createRuntime(finalRuntimeConfig); + + // Resolve srcBaseDir to absolute path (expanding tildes, etc.) + resolvedSrcBaseDir = await runtime.resolvePath(finalRuntimeConfig.srcBaseDir); + + // If path was resolved to something different, recreate runtime with resolved path + if (resolvedSrcBaseDir !== finalRuntimeConfig.srcBaseDir) { + const resolvedRuntimeConfig: RuntimeConfig = { + ...finalRuntimeConfig, + srcBaseDir: resolvedSrcBaseDir, + }; + runtime = createRuntime(resolvedRuntimeConfig); + // Update finalRuntimeConfig to store resolved path in config + finalRuntimeConfig.srcBaseDir = resolvedSrcBaseDir; + } } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error); return { success: false, error: errorMsg }; From d27fdc9e1aa51353c49159defd2ea7425f814e6d Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 16:33:46 +0000 Subject: [PATCH 16/21] =?UTF-8?q?=F0=9F=A4=96=20Fix=20resolvePath()=20to?= =?UTF-8?q?=20separate=20path=20resolution=20from=20existence=20checking?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove existence checking from LocalRuntime.resolvePath() - Now just expands tildes and resolves to absolute path - No longer uses fsPromises.access() to verify path exists - Update SSHRuntime.resolvePath() to use readlink -m instead of realpath - readlink -m normalizes paths without requiring them to exist - Properly separates path resolution from validation - Extract SSH command spawning pattern into execSSHCommand() helper - Reduces duplication in SSHRuntime - Reusable for simple SSH commands that return stdout - Remove SSH integration tests for resolvePath() - Will rely on createWorkspace matrix tests for end-to-end verification - Keep only constructor unit tests that don't require SSH - Update LocalRuntime test to expect non-existent paths to resolve successfully - Revert unrelated gitService.test.ts debug logging This improves separation of concerns: resolvePath() now purely resolves paths without side effects, allowing callers to decide whether to validate existence. Generated with `cmux` --- src/runtime/LocalRuntime.test.ts | 9 +++-- src/runtime/LocalRuntime.ts | 14 +------- src/runtime/SSHRuntime.test.ts | 60 -------------------------------- src/runtime/SSHRuntime.ts | 39 +++++++++------------ src/services/gitService.test.ts | 6 ---- 5 files changed, 21 insertions(+), 107 deletions(-) diff --git a/src/runtime/LocalRuntime.test.ts b/src/runtime/LocalRuntime.test.ts index f191a167d..a3cbdd053 100644 --- a/src/runtime/LocalRuntime.test.ts +++ b/src/runtime/LocalRuntime.test.ts @@ -51,12 +51,11 @@ describe("LocalRuntime.resolvePath", () => { expect(resolved).toBe("/tmp"); }); - it("should reject non-existent paths", async () => { + it("should resolve non-existent paths without checking existence", async () => { const runtime = new LocalRuntime("/tmp"); - // eslint-disable-next-line @typescript-eslint/await-thenable - await expect(runtime.resolvePath("/this/path/does/not/exist/12345")).rejects.toThrow( - /Cannot resolve path/ - ); + const resolved = await runtime.resolvePath("/this/path/does/not/exist/12345"); + // Should resolve to absolute path without checking if it exists + expect(resolved).toBe("/this/path/does/not/exist/12345"); }); it("should resolve relative paths from cwd", async () => { diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index 10217aa08..3e2d7dd29 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -306,19 +306,7 @@ export class LocalRuntime implements Runtime { const expanded = expandTilde(filePath); // Resolve to absolute path (handles relative paths like "./foo") - const resolved = path.resolve(expanded); - - // Verify path exists - try { - await fsPromises.access(resolved); - return resolved; - } catch (err) { - throw new RuntimeErrorClass( - `Cannot resolve path '${filePath}': ${getErrorMessage(err)}`, - "file_io", - err instanceof Error ? err : undefined - ); - } + return path.resolve(expanded); } normalizePath(targetPath: string, basePath: string): string { diff --git a/src/runtime/SSHRuntime.test.ts b/src/runtime/SSHRuntime.test.ts index 57f2e6a16..4ee0f8ccc 100644 --- a/src/runtime/SSHRuntime.test.ts +++ b/src/runtime/SSHRuntime.test.ts @@ -31,63 +31,3 @@ describe("SSHRuntime constructor", () => { }).not.toThrow(); }); }); - -describe("SSHRuntime.resolvePath", () => { - // Note: These tests require TEST_INTEGRATION=1 to run with actual SSH connection - const isIntegrationTest = process.env.TEST_INTEGRATION === "1"; - const describeIfIntegration = isIntegrationTest ? describe : describe.skip; - - describeIfIntegration("with SSH connection", () => { - it("should expand tilde to home directory", async () => { - const runtime = new SSHRuntime({ - host: process.env.SSH_HOST ?? "localhost", - port: parseInt(process.env.SSH_PORT ?? "2222"), - identityFile: process.env.SSH_IDENTITY_FILE, - srcBaseDir: "~/test-workspace", - }); - - const resolved = await runtime.resolvePath("~"); - // Should be an absolute path - expect(resolved).toMatch(/^\/home\//); - }); - - it("should expand tilde with path", async () => { - const runtime = new SSHRuntime({ - host: process.env.SSH_HOST ?? "localhost", - port: parseInt(process.env.SSH_PORT ?? "2222"), - identityFile: process.env.SSH_IDENTITY_FILE, - srcBaseDir: "~/test-workspace", - }); - - const resolved = await runtime.resolvePath("~/.."); - // Should be parent of home directory - expect(resolved).toBe("/home"); - }); - - it("should resolve absolute paths", async () => { - const runtime = new SSHRuntime({ - host: process.env.SSH_HOST ?? "localhost", - port: parseInt(process.env.SSH_PORT ?? "2222"), - identityFile: process.env.SSH_IDENTITY_FILE, - srcBaseDir: "/tmp", - }); - - const resolved = await runtime.resolvePath("/tmp"); - expect(resolved).toBe("/tmp"); - }); - - it("should reject non-existent paths", async () => { - const runtime = new SSHRuntime({ - host: process.env.SSH_HOST ?? "localhost", - port: parseInt(process.env.SSH_PORT ?? "2222"), - identityFile: process.env.SSH_IDENTITY_FILE, - srcBaseDir: "/tmp", - }); - - // eslint-disable-next-line @typescript-eslint/await-thenable - await expect(runtime.resolvePath("/this/path/does/not/exist/12345")).rejects.toThrow( - /Failed to resolve path/ - ); - }); - }); -}); diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index 66e308804..7a5ac9da2 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -318,10 +318,18 @@ export class SSHRuntime implements Runtime { }; } async resolvePath(filePath: string): Promise { - // Use shell to expand tildes and resolve path on remote system - // We use `realpath` which resolves symlinks and gives canonical path - // Note: realpath also verifies the path exists - const command = `realpath ${shescape.quote(filePath)}`; + // Use shell to expand tildes and normalize path on remote system + // Uses bash to expand ~ and readlink -m to normalize without checking existence + // readlink -m canonicalizes the path (handles .., ., //) without requiring it to exist + const command = `bash -c 'readlink -m ${shescape.quote(filePath)}'`; + return this.execSSHCommand(command); + } + + /** + * Execute a simple SSH command and return stdout + * @private + */ + private async execSSHCommand(command: string): Promise { const sshArgs = this.buildSSHArgs(); sshArgs.push(this.config.host, command); @@ -340,33 +348,18 @@ export class SSHRuntime implements Runtime { proc.on("close", (code) => { if (code !== 0) { - reject( - new RuntimeErrorClass( - `Failed to resolve path '${filePath}' on remote host: ${stderr.trim()}`, - "file_io" - ) - ); - return; - } - - const resolved = stdout.trim(); - if (!resolved?.startsWith("/")) { - reject( - new RuntimeErrorClass( - `Invalid resolved path '${resolved}' for '${filePath}' (expected absolute path)`, - "file_io" - ) - ); + reject(new RuntimeErrorClass(`SSH command failed: ${stderr.trim()}`, "network")); return; } - resolve(resolved); + const output = stdout.trim(); + resolve(output); }); proc.on("error", (err) => { reject( new RuntimeErrorClass( - `Cannot resolve path '${filePath}' on remote host: ${getErrorMessage(err)}`, + `Cannot execute SSH command: ${getErrorMessage(err)}`, "network", err instanceof Error ? err : undefined ) diff --git a/src/services/gitService.test.ts b/src/services/gitService.test.ts index 7a578e01b..4f7cb665e 100644 --- a/src/services/gitService.test.ts +++ b/src/services/gitService.test.ts @@ -55,9 +55,6 @@ describe("removeWorktreeSafe", () => { const result = await createWorktree(mockConfig, repoPath, "test-branch", { trunkBranch: defaultBranch, }); - if (!result.success) { - console.error("createWorktree failed:", result.error); - } expect(result.success).toBe(true); const worktreePath = result.path!; @@ -91,9 +88,6 @@ describe("removeWorktreeSafe", () => { const result = await createWorktree(mockConfig, repoPath, "dirty-branch", { trunkBranch: defaultBranch, }); - if (!result.success) { - console.error("createWorktree failed:", result.error); - } expect(result.success).toBe(true); const worktreePath = result.path!; From 839ff279a3a385b9fb8e8f320a796cd6160bfc34 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 16:36:34 +0000 Subject: [PATCH 17/21] =?UTF-8?q?=F0=9F=A4=96=20Fix:=20Remove=20async=20fr?= =?UTF-8?q?om=20LocalRuntime.resolvePath()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Method no longer uses await, so removing async keyword to satisfy linter. Generated with `cmux` --- src/runtime/LocalRuntime.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index 3e2d7dd29..8c54701bc 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -301,12 +301,12 @@ export class LocalRuntime implements Runtime { } } - async resolvePath(filePath: string): Promise { + resolvePath(filePath: string): Promise { // Expand tilde to actual home directory path const expanded = expandTilde(filePath); // Resolve to absolute path (handles relative paths like "./foo") - return path.resolve(expanded); + return Promise.resolve(path.resolve(expanded)); } normalizePath(targetPath: string, basePath: string): string { From dd9f92f3fd0d13fb443849e7957f621e2c09ca1c Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 16:43:17 +0000 Subject: [PATCH 18/21] =?UTF-8?q?=F0=9F=A4=96=20Add=20timeout=20requiremen?= =?UTF-8?q?t=20to=20SSHRuntime.execSSHCommand()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add timeoutMs parameter to execSSHCommand() to prevent network hangs - Implement timeout with clearTimeout cleanup on success/error - Update resolvePath() to use 5 second timeout (should be near-instant) - Add class-level comment documenting timeout requirement for all SSH operations - Timeouts should be either set literally or forwarded from upstream This prevents indefinite hangs on network issues, dropped connections, or unresponsive SSH servers. Generated with `cmux` --- src/runtime/SSHRuntime.ts | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index 7a5ac9da2..922b56806 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -53,6 +53,10 @@ export interface SSHRuntimeConfig { * - Supports SSH config aliases, ProxyJump, ControlMaster, etc. * - No password prompts (assumes key-based auth or ssh-agent) * - Atomic file writes via temp + rename + * + * IMPORTANT: All SSH operations MUST include a timeout to prevent hangs from network issues. + * Timeouts should be either set literally for internal operations or forwarded from upstream + * for user-initiated operations. */ export class SSHRuntime implements Runtime { private readonly config: SSHRuntimeConfig; @@ -322,14 +326,17 @@ export class SSHRuntime implements Runtime { // Uses bash to expand ~ and readlink -m to normalize without checking existence // readlink -m canonicalizes the path (handles .., ., //) without requiring it to exist const command = `bash -c 'readlink -m ${shescape.quote(filePath)}'`; - return this.execSSHCommand(command); + // Use 5 second timeout for path resolution (should be near-instant) + return this.execSSHCommand(command, 5000); } /** * Execute a simple SSH command and return stdout + * @param command - The command to execute on the remote host + * @param timeoutMs - Timeout in milliseconds (required to prevent network hangs) * @private */ - private async execSSHCommand(command: string): Promise { + private async execSSHCommand(command: string, timeoutMs: number): Promise { const sshArgs = this.buildSSHArgs(); sshArgs.push(this.config.host, command); @@ -337,6 +344,16 @@ export class SSHRuntime implements Runtime { const proc = spawn("ssh", sshArgs); let stdout = ""; let stderr = ""; + let timedOut = false; + + // Set timeout to prevent hanging on network issues + const timer = setTimeout(() => { + timedOut = true; + proc.kill(); + reject( + new RuntimeErrorClass(`SSH command timed out after ${timeoutMs}ms: ${command}`, "network") + ); + }, timeoutMs); proc.stdout?.on("data", (data: Buffer) => { stdout += data.toString(); @@ -347,6 +364,9 @@ export class SSHRuntime implements Runtime { }); proc.on("close", (code) => { + clearTimeout(timer); + if (timedOut) return; // Already rejected + if (code !== 0) { reject(new RuntimeErrorClass(`SSH command failed: ${stderr.trim()}`, "network")); return; @@ -357,6 +377,9 @@ export class SSHRuntime implements Runtime { }); proc.on("error", (err) => { + clearTimeout(timer); + if (timedOut) return; // Already rejected + reject( new RuntimeErrorClass( `Cannot execute SSH command: ${getErrorMessage(err)}`, From 4da5eb97f3f292586b8cba78953bea2521256c5a Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 16:47:24 +0000 Subject: [PATCH 19/21] =?UTF-8?q?=F0=9F=A4=96=20Update=20createWorkspace?= =?UTF-8?q?=20integration=20tests=20for=20new=20tilde=20resolution=20logic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change tests from expecting tilde rejection to expecting tilde resolution - Update "rejects tilde paths" → "resolves tilde paths to absolute paths" - Update "rejects bare tilde" → "resolves bare tilde to home directory" - Verify that resolved paths are stored in config (not tilde paths) - Remove skip logic for missing SSH server - SSH server is now required - Change skip checks to throw errors if SSH server unavailable - Fix type errors: use workspace.name instead of workspace.branch Tests now verify that: 1. Tildes are accepted in srcBaseDir for both local and SSH runtimes 2. Tildes are resolved to absolute paths via runtime.resolvePath() 3. Resolved paths are stored in config (ensuring consistency) 4. SSH server must be available for SSH tests (no silent skips) Generated with `cmux` --- tests/ipcMain/createWorkspace.test.ts | 74 ++++++++++++++++++--------- 1 file changed, 49 insertions(+), 25 deletions(-) diff --git a/tests/ipcMain/createWorkspace.test.ts b/tests/ipcMain/createWorkspace.test.ts index ba300fe20..892f70ec7 100644 --- a/tests/ipcMain/createWorkspace.test.ts +++ b/tests/ipcMain/createWorkspace.test.ts @@ -590,22 +590,20 @@ exit 1 ); test.concurrent( - "rejects tilde paths in srcBaseDir (SSH only)", + "resolves tilde paths in srcBaseDir to absolute paths (SSH only)", async () => { - // Skip if SSH server not available if (!sshConfig) { - console.log("Skipping SSH-specific test: SSH server not available"); - return; + throw new Error("SSH server is required for SSH integration tests"); } const env = await createTestEnvironment(); const tempGitRepo = await createTempGitRepo(); try { - const branchName = generateBranchName("tilde-rejection-test"); + const branchName = generateBranchName("tilde-resolution-test"); const trunkBranch = await detectDefaultTrunkBranch(tempGitRepo); - // Try to use tilde path - should be rejected + // Use tilde path - should be accepted and resolved const tildeRuntimeConfig: RuntimeConfig = { type: "ssh", host: `testuser@localhost`, @@ -614,17 +612,32 @@ exit 1 port: sshConfig.port, }; - const result = await env.mockIpcRenderer.invoke( - IPC_CHANNELS.WORKSPACE_CREATE, + const { result, cleanup } = await createWorkspaceWithCleanup( + env, tempGitRepo, branchName, trunkBranch, tildeRuntimeConfig ); - // Should fail with error about tilde - expect(result.success).toBe(false); - expect(result.error).toMatch(/cannot start with tilde/i); + // Should succeed and resolve tilde to absolute path + expect(result.success).toBe(true); + if (!result.success) { + throw new Error(`Failed to create workspace: ${result.error}`); + } + + // Verify the stored runtimeConfig has resolved path (not tilde) + const projectsConfig = env.config.loadConfigOrDefault(); + const projectWorkspaces = + projectsConfig.projects.get(tempGitRepo)?.workspaces ?? []; + const workspace = projectWorkspaces.find((w) => w.name === branchName); + + expect(workspace).toBeDefined(); + expect(workspace?.runtimeConfig?.srcBaseDir).toBeDefined(); + expect(workspace?.runtimeConfig?.srcBaseDir).toMatch(/^\/home\//); + expect(workspace?.runtimeConfig?.srcBaseDir).not.toContain("~"); + + await cleanup(); } finally { await cleanupTestEnvironment(env); await cleanupTempGitRepo(tempGitRepo); @@ -634,22 +647,20 @@ exit 1 ); test.concurrent( - "rejects bare tilde in srcBaseDir (SSH only)", + "resolves bare tilde in srcBaseDir to home directory (SSH only)", async () => { - // Skip if SSH server not available if (!sshConfig) { - console.log("Skipping SSH-specific test: SSH server not available"); - return; + throw new Error("SSH server is required for SSH integration tests"); } const env = await createTestEnvironment(); const tempGitRepo = await createTempGitRepo(); try { - const branchName = generateBranchName("bare-tilde-rejection"); + const branchName = generateBranchName("bare-tilde-resolution"); const trunkBranch = await detectDefaultTrunkBranch(tempGitRepo); - // Try to use bare tilde - should be rejected + // Use bare tilde - should be accepted and resolved to home directory const tildeRuntimeConfig: RuntimeConfig = { type: "ssh", host: `testuser@localhost`, @@ -658,17 +669,32 @@ exit 1 port: sshConfig.port, }; - const result = await env.mockIpcRenderer.invoke( - IPC_CHANNELS.WORKSPACE_CREATE, + const { result, cleanup } = await createWorkspaceWithCleanup( + env, tempGitRepo, branchName, trunkBranch, tildeRuntimeConfig ); - // Should fail with error about tilde - expect(result.success).toBe(false); - expect(result.error).toMatch(/cannot start with tilde/i); + // Should succeed and resolve tilde to home directory + expect(result.success).toBe(true); + if (!result.success) { + throw new Error(`Failed to create workspace: ${result.error}`); + } + + // Verify the stored runtimeConfig has resolved path (not tilde) + const projectsConfig = env.config.loadConfigOrDefault(); + const projectWorkspaces = + projectsConfig.projects.get(tempGitRepo)?.workspaces ?? []; + const workspace = projectWorkspaces.find((w) => w.name === branchName); + + expect(workspace).toBeDefined(); + expect(workspace?.runtimeConfig?.srcBaseDir).toBeDefined(); + expect(workspace?.runtimeConfig?.srcBaseDir).toMatch(/^\/home\//); + expect(workspace?.runtimeConfig?.srcBaseDir).not.toContain("~"); + + await cleanup(); } finally { await cleanupTestEnvironment(env); await cleanupTempGitRepo(tempGitRepo); @@ -783,10 +809,8 @@ exit 1 test.concurrent( "forwards origin remote instead of bundle path", async () => { - // Skip if SSH server not available if (!sshConfig) { - console.log("Skipping SSH-specific test: SSH server not available"); - return; + throw new Error("SSH server is required for SSH integration tests"); } const env = await createTestEnvironment(); From 8aa211e26ff8b83af5aaeeba3c24d384ca7fbde3 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 16:52:41 +0000 Subject: [PATCH 20/21] =?UTF-8?q?=F0=9F=93=9D=20Clarify=20that=20integrati?= =?UTF-8?q?on=20tests=20require=20bun=20x=20jest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add note to Testing section that tests in tests/ folder must be run with 'bun x jest' (not 'bun test') since they use Jest as the test runner. Unit tests in src/ use bun test, integration tests in tests/ use Jest. Generated with `cmux` --- docs/AGENTS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/AGENTS.md b/docs/AGENTS.md index 3a6dd36cc..824ea2c0f 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -203,8 +203,10 @@ This project uses **Make** as the primary build orchestrator. See `Makefile` for - utils should be either pure functions or easily isolated (e.g. if they operate on the FS they accept a path). Testing them should not require complex mocks or setup. - **Integration tests:** + - **⚠️ IMPORTANT: Use `bun x jest` to run tests in the `tests/` folder** - Integration tests use Jest (not bun test), so you must run them with `bun x jest` or `TEST_INTEGRATION=1 bun x jest` - Run specific integration test: `TEST_INTEGRATION=1 bun x jest tests/ipcMain/sendMessage.test.ts -t "test name pattern"` - Run all integration tests: `TEST_INTEGRATION=1 bun x jest tests` (~35 seconds, runs 40 tests) + - Unit tests in `src/` use bun test: `bun test src/path/to/file.test.ts` - **⚠️ Running `tests/ipcMain` locally takes a very long time.** Prefer running specific test files or use `-t` to filter to specific tests. - **Performance**: Tests use `test.concurrent()` to run in parallel within each file - **NEVER bypass IPC in integration tests** - Integration tests must use the real IPC communication paths (e.g., `mockIpcRenderer.invoke()`) even when it's harder. Directly accessing services (HistoryService, PartialService, etc.) or manipulating config/state directly bypasses the integration layer and defeats the purpose of the test. From 39bae689c380577c5952bc14dacfbab1777ddf85 Mon Sep 17 00:00:00 2001 From: Ammar Date: Mon, 27 Oct 2025 16:56:37 +0000 Subject: [PATCH 21/21] =?UTF-8?q?=F0=9F=A4=96=20Fix=20SSHRuntime.resolvePa?= =?UTF-8?q?th()=20to=20work=20with=20BusyBox?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use simple bash echo to expand tildes instead of readlink -m, which is not available in BusyBox (used in SSH Docker test container). The command `bash -c 'p=$path; echo $p'` lets bash expand ~ naturally without requiring GNU coreutils or python3. This fixes runtimeFileEditing integration tests that were failing with: readlink: unrecognized option: m All tests now pass (8/8 in runtimeFileEditing). Generated with `cmux` --- src/runtime/SSHRuntime.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index 922b56806..04aa3726e 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -322,10 +322,10 @@ export class SSHRuntime implements Runtime { }; } async resolvePath(filePath: string): Promise { - // Use shell to expand tildes and normalize path on remote system - // Uses bash to expand ~ and readlink -m to normalize without checking existence - // readlink -m canonicalizes the path (handles .., ., //) without requiring it to exist - const command = `bash -c 'readlink -m ${shescape.quote(filePath)}'`; + // Use shell to expand tildes on remote system + // Bash will expand ~ automatically when we echo the unquoted variable + // This works with BusyBox (doesn't require GNU coreutils) + const command = `bash -c 'p=${shescape.quote(filePath)}; echo $p'`; // Use 5 second timeout for path resolution (should be near-instant) return this.execSSHCommand(command, 5000); }