From ded80547ab7df6f1b691c701ec175ccc9970b3ae Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 26 Oct 2025 19:01:27 +0000 Subject: [PATCH 1/3] Fix redundant CD detection for SSH runtime Delegate path normalization to Runtime interface instead of having bash tool understand different runtime types. - Add normalizePath() method to Runtime interface - Implement in LocalRuntime using Node.js path.resolve() - Implement in SSHRuntime using POSIX path semantics - Update bash.ts to delegate to runtime.normalizePath() - Add 6 comprehensive tests for SSH runtime - All 52 bash tool tests pass This approach maintains better separation of concerns - the bash tool doesn't need to know about SSH vs Local runtime specifics. --- src/runtime/LocalRuntime.ts | 10 +++ src/runtime/Runtime.ts | 19 +++++ src/runtime/SSHRuntime.ts | 35 ++++++++ src/services/tools/bash.test.ts | 136 ++++++++++++++++++++++++++++++++ src/services/tools/bash.ts | 12 ++- 5 files changed, 205 insertions(+), 7 deletions(-) diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index 4b01216e8..95818b758 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -303,6 +303,16 @@ export class LocalRuntime implements Runtime { } } + normalizePath(targetPath: string, basePath: string): string { + // For local runtime, use Node.js path resolution + // Handle special case: current directory + const target = targetPath.trim(); + if (target === ".") { + return path.resolve(basePath); + } + return path.resolve(basePath, target); + } + getWorkspacePath(projectPath: string, workspaceName: string): string { const projectName = getProjectName(projectPath); return path.join(this.srcBaseDir, projectName, workspaceName); diff --git a/src/runtime/Runtime.ts b/src/runtime/Runtime.ts index 0bfd1af25..4cf89f2ca 100644 --- a/src/runtime/Runtime.ts +++ b/src/runtime/Runtime.ts @@ -195,6 +195,25 @@ export interface Runtime { */ stat(path: string): Promise; + /** + * Normalize a path for comparison purposes within this runtime's context. + * Handles runtime-specific path semantics (local vs remote). + * + * @param targetPath Path to normalize (may be relative or absolute) + * @param basePath Base path to resolve relative paths against + * @returns Normalized path suitable for string comparison + * + * @example + * // LocalRuntime + * runtime.normalizePath(".", "/home/user") // => "/home/user" + * runtime.normalizePath("../other", "/home/user/project") // => "/home/user/other" + * + * // SSHRuntime + * runtime.normalizePath(".", "/home/user") // => "/home/user" + * runtime.normalizePath("~/project", "~") // => "~/project" + */ + normalizePath(targetPath: string, basePath: string): string; + /** * Compute absolute workspace path from project and workspace name. * This is the SINGLE source of truth for workspace path computation. diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index 7c19f3d27..4b86d916b 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -317,6 +317,41 @@ export class SSHRuntime implements Runtime { }; } + normalizePath(targetPath: string, basePath: string): string { + // For SSH, handle paths in a POSIX-like manner without accessing the remote filesystem + const target = targetPath.trim(); + let base = basePath.trim(); + + // Normalize base path - remove trailing slash (except for root "/") + if (base.length > 1 && base.endsWith("/")) { + base = base.slice(0, -1); + } + + // Handle special case: current directory + if (target === ".") { + return base; + } + + // Handle tilde expansion - keep as-is for comparison + let normalizedTarget = target; + if (target === "~" || target.startsWith("~/")) { + normalizedTarget = target; + } else if (target.startsWith("/")) { + // Absolute path - use as-is + normalizedTarget = target; + } else { + // Relative path - resolve against base using POSIX path joining + normalizedTarget = base.endsWith("/") ? base + target : base + "/" + target; + } + + // Remove trailing slash for comparison (except for root "/") + if (normalizedTarget.length > 1 && normalizedTarget.endsWith("/")) { + normalizedTarget = normalizedTarget.slice(0, -1); + } + + return normalizedTarget; + } + /** * Build common SSH arguments based on runtime config * @param includeHost - Whether to include the host in the args (for direct ssh commands) diff --git a/src/services/tools/bash.test.ts b/src/services/tools/bash.test.ts index fc58458e6..8ff76cc34 100644 --- a/src/services/tools/bash.test.ts +++ b/src/services/tools/bash.test.ts @@ -1235,3 +1235,139 @@ fi expect(remainingProcesses).toBe(0); }); }); + +describe("SSH runtime redundant cd detection", () => { + // Helper to create bash tool with SSH runtime configuration + // Note: These tests check redundant cd detection logic only - they don't actually execute via SSH + function createTestBashToolWithSSH(cwd: string) { + const tempDir = new TestTempDir("test-bash-ssh"); + const sshRuntime = createRuntime({ + type: "ssh", + host: "test-host", + srcBaseDir: "/remote/base", + }); + + const tool = createBashTool({ + cwd, + runtime: sshRuntime, + tempDir: tempDir.path, + }); + + return { + tool, + [Symbol.dispose]() { + tempDir[Symbol.dispose](); + }, + }; + } + + it("should reject redundant cd to absolute path on SSH runtime", async () => { + const remoteCwd = "/home/user/project"; + using testEnv = createTestBashToolWithSSH(remoteCwd); + const tool = testEnv.tool; + + const args: BashToolArgs = { + script: `cd ${remoteCwd} && echo test`, + timeout_secs: 5, + }; + + const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant cd"); + expect(result.error).toContain("already runs in"); + } + }); + + it("should reject redundant cd with relative path (.) on SSH runtime", async () => { + const remoteCwd = "/home/user/project"; + using testEnv = createTestBashToolWithSSH(remoteCwd); + const tool = testEnv.tool; + + const args: BashToolArgs = { + script: "cd . && echo test", + timeout_secs: 5, + }; + + const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant cd"); + } + }); + + it("should reject redundant cd with tilde path on SSH runtime", async () => { + const remoteCwd = "~/project"; + using testEnv = createTestBashToolWithSSH(remoteCwd); + const tool = testEnv.tool; + + const args: BashToolArgs = { + script: "cd ~/project && echo test", + timeout_secs: 5, + }; + + const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant cd"); + } + }); + + it("should reject redundant cd with single tilde on SSH runtime", async () => { + const remoteCwd = "~"; + using testEnv = createTestBashToolWithSSH(remoteCwd); + const tool = testEnv.tool; + + const args: BashToolArgs = { + script: "cd ~ && echo test", + timeout_secs: 5, + }; + + const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant cd"); + } + }); + + it("should handle trailing slashes in path comparison on SSH runtime", async () => { + const remoteCwd = "/home/user/project"; + using testEnv = createTestBashToolWithSSH(remoteCwd); + const tool = testEnv.tool; + + const args: BashToolArgs = { + script: "cd /home/user/project/ && echo test", + timeout_secs: 5, + }; + + const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant cd"); + } + }); + + it("should handle cwd with trailing slash on SSH runtime", async () => { + const remoteCwd = "/home/user/project/"; + using testEnv = createTestBashToolWithSSH(remoteCwd); + const tool = testEnv.tool; + + const args: BashToolArgs = { + script: "cd /home/user/project && echo test", + timeout_secs: 5, + }; + + const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult; + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("Redundant cd"); + } + }); +}); + diff --git a/src/services/tools/bash.ts b/src/services/tools/bash.ts index 23dbcabfa..491b9cbdf 100644 --- a/src/services/tools/bash.ts +++ b/src/services/tools/bash.ts @@ -77,17 +77,15 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => { let fileTruncated = false; // Hit 100KB file limit // Detect redundant cd to working directory - // Note: config.cwd is the actual execution path (local for LocalRuntime, remote for SSHRuntime) - // Match patterns like: "cd /path &&", "cd /path;", "cd '/path' &&", "cd "/path" &&" + // Delegate path normalization to the runtime for proper handling of local vs remote paths const cdPattern = /^\s*cd\s+['"]?([^'";\\&|]+)['"]?\s*[;&|]/; const match = cdPattern.exec(script); if (match) { const targetPath = match[1].trim(); - // For SSH runtime, config.cwd might use $HOME - need to handle this - // Normalize paths for comparison (resolve to absolute where possible) - // Note: This check is best-effort - it won't catch all cases on SSH (e.g., ~/path vs $HOME/path) - const normalizedTarget = path.resolve(config.cwd, targetPath); - const normalizedCwd = path.resolve(config.cwd); + + // Use runtime's normalizePath method to handle path comparison correctly + const normalizedTarget = config.runtime.normalizePath(targetPath, config.cwd); + const normalizedCwd = config.runtime.normalizePath(".", config.cwd); if (normalizedTarget === normalizedCwd) { return { From b5f1a9132f1748ce817ec9d10270e9ac7f2991c3 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 26 Oct 2025 19:16:33 +0000 Subject: [PATCH 2/3] Fix prettier formatting --- src/services/tools/bash.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/services/tools/bash.test.ts b/src/services/tools/bash.test.ts index 8ff76cc34..0542280b2 100644 --- a/src/services/tools/bash.test.ts +++ b/src/services/tools/bash.test.ts @@ -1246,7 +1246,7 @@ describe("SSH runtime redundant cd detection", () => { host: "test-host", srcBaseDir: "/remote/base", }); - + const tool = createBashTool({ cwd, runtime: sshRuntime, @@ -1370,4 +1370,3 @@ describe("SSH runtime redundant cd detection", () => { } }); }); - From 80cd8dd248bec88c83b44e651ccf41279288d105 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 26 Oct 2025 19:21:57 +0000 Subject: [PATCH 3/3] Fix trailing whitespace in comments --- src/runtime/Runtime.ts | 6 +++--- src/services/tools/bash.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/runtime/Runtime.ts b/src/runtime/Runtime.ts index 4cf89f2ca..28f8fd9b4 100644 --- a/src/runtime/Runtime.ts +++ b/src/runtime/Runtime.ts @@ -198,16 +198,16 @@ export interface Runtime { /** * Normalize a path for comparison purposes within this runtime's context. * Handles runtime-specific path semantics (local vs remote). - * + * * @param targetPath Path to normalize (may be relative or absolute) * @param basePath Base path to resolve relative paths against * @returns Normalized path suitable for string comparison - * + * * @example * // LocalRuntime * runtime.normalizePath(".", "/home/user") // => "/home/user" * runtime.normalizePath("../other", "/home/user/project") // => "/home/user/other" - * + * * // SSHRuntime * runtime.normalizePath(".", "/home/user") // => "/home/user" * runtime.normalizePath("~/project", "~") // => "~/project" diff --git a/src/services/tools/bash.ts b/src/services/tools/bash.ts index 491b9cbdf..8def72db6 100644 --- a/src/services/tools/bash.ts +++ b/src/services/tools/bash.ts @@ -82,7 +82,7 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => { const match = cdPattern.exec(script); if (match) { const targetPath = match[1].trim(); - + // Use runtime's normalizePath method to handle path comparison correctly const normalizedTarget = config.runtime.normalizePath(targetPath, config.cwd); const normalizedCwd = config.runtime.normalizePath(".", config.cwd);