Skip to content

Commit ded8054

Browse files
committed
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.
1 parent 2ee355f commit ded8054

File tree

5 files changed

+205
-7
lines changed

5 files changed

+205
-7
lines changed

src/runtime/LocalRuntime.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,16 @@ export class LocalRuntime implements Runtime {
303303
}
304304
}
305305

306+
normalizePath(targetPath: string, basePath: string): string {
307+
// For local runtime, use Node.js path resolution
308+
// Handle special case: current directory
309+
const target = targetPath.trim();
310+
if (target === ".") {
311+
return path.resolve(basePath);
312+
}
313+
return path.resolve(basePath, target);
314+
}
315+
306316
getWorkspacePath(projectPath: string, workspaceName: string): string {
307317
const projectName = getProjectName(projectPath);
308318
return path.join(this.srcBaseDir, projectName, workspaceName);

src/runtime/Runtime.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,25 @@ export interface Runtime {
195195
*/
196196
stat(path: string): Promise<FileStat>;
197197

198+
/**
199+
* Normalize a path for comparison purposes within this runtime's context.
200+
* Handles runtime-specific path semantics (local vs remote).
201+
*
202+
* @param targetPath Path to normalize (may be relative or absolute)
203+
* @param basePath Base path to resolve relative paths against
204+
* @returns Normalized path suitable for string comparison
205+
*
206+
* @example
207+
* // LocalRuntime
208+
* runtime.normalizePath(".", "/home/user") // => "/home/user"
209+
* runtime.normalizePath("../other", "/home/user/project") // => "/home/user/other"
210+
*
211+
* // SSHRuntime
212+
* runtime.normalizePath(".", "/home/user") // => "/home/user"
213+
* runtime.normalizePath("~/project", "~") // => "~/project"
214+
*/
215+
normalizePath(targetPath: string, basePath: string): string;
216+
198217
/**
199218
* Compute absolute workspace path from project and workspace name.
200219
* This is the SINGLE source of truth for workspace path computation.

src/runtime/SSHRuntime.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,41 @@ export class SSHRuntime implements Runtime {
317317
};
318318
}
319319

320+
normalizePath(targetPath: string, basePath: string): string {
321+
// For SSH, handle paths in a POSIX-like manner without accessing the remote filesystem
322+
const target = targetPath.trim();
323+
let base = basePath.trim();
324+
325+
// Normalize base path - remove trailing slash (except for root "/")
326+
if (base.length > 1 && base.endsWith("/")) {
327+
base = base.slice(0, -1);
328+
}
329+
330+
// Handle special case: current directory
331+
if (target === ".") {
332+
return base;
333+
}
334+
335+
// Handle tilde expansion - keep as-is for comparison
336+
let normalizedTarget = target;
337+
if (target === "~" || target.startsWith("~/")) {
338+
normalizedTarget = target;
339+
} else if (target.startsWith("/")) {
340+
// Absolute path - use as-is
341+
normalizedTarget = target;
342+
} else {
343+
// Relative path - resolve against base using POSIX path joining
344+
normalizedTarget = base.endsWith("/") ? base + target : base + "/" + target;
345+
}
346+
347+
// Remove trailing slash for comparison (except for root "/")
348+
if (normalizedTarget.length > 1 && normalizedTarget.endsWith("/")) {
349+
normalizedTarget = normalizedTarget.slice(0, -1);
350+
}
351+
352+
return normalizedTarget;
353+
}
354+
320355
/**
321356
* Build common SSH arguments based on runtime config
322357
* @param includeHost - Whether to include the host in the args (for direct ssh commands)

src/services/tools/bash.test.ts

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1235,3 +1235,139 @@ fi
12351235
expect(remainingProcesses).toBe(0);
12361236
});
12371237
});
1238+
1239+
describe("SSH runtime redundant cd detection", () => {
1240+
// Helper to create bash tool with SSH runtime configuration
1241+
// Note: These tests check redundant cd detection logic only - they don't actually execute via SSH
1242+
function createTestBashToolWithSSH(cwd: string) {
1243+
const tempDir = new TestTempDir("test-bash-ssh");
1244+
const sshRuntime = createRuntime({
1245+
type: "ssh",
1246+
host: "test-host",
1247+
srcBaseDir: "/remote/base",
1248+
});
1249+
1250+
const tool = createBashTool({
1251+
cwd,
1252+
runtime: sshRuntime,
1253+
tempDir: tempDir.path,
1254+
});
1255+
1256+
return {
1257+
tool,
1258+
[Symbol.dispose]() {
1259+
tempDir[Symbol.dispose]();
1260+
},
1261+
};
1262+
}
1263+
1264+
it("should reject redundant cd to absolute path on SSH runtime", async () => {
1265+
const remoteCwd = "/home/user/project";
1266+
using testEnv = createTestBashToolWithSSH(remoteCwd);
1267+
const tool = testEnv.tool;
1268+
1269+
const args: BashToolArgs = {
1270+
script: `cd ${remoteCwd} && echo test`,
1271+
timeout_secs: 5,
1272+
};
1273+
1274+
const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult;
1275+
1276+
expect(result.success).toBe(false);
1277+
if (!result.success) {
1278+
expect(result.error).toContain("Redundant cd");
1279+
expect(result.error).toContain("already runs in");
1280+
}
1281+
});
1282+
1283+
it("should reject redundant cd with relative path (.) on SSH runtime", async () => {
1284+
const remoteCwd = "/home/user/project";
1285+
using testEnv = createTestBashToolWithSSH(remoteCwd);
1286+
const tool = testEnv.tool;
1287+
1288+
const args: BashToolArgs = {
1289+
script: "cd . && echo test",
1290+
timeout_secs: 5,
1291+
};
1292+
1293+
const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult;
1294+
1295+
expect(result.success).toBe(false);
1296+
if (!result.success) {
1297+
expect(result.error).toContain("Redundant cd");
1298+
}
1299+
});
1300+
1301+
it("should reject redundant cd with tilde path on SSH runtime", async () => {
1302+
const remoteCwd = "~/project";
1303+
using testEnv = createTestBashToolWithSSH(remoteCwd);
1304+
const tool = testEnv.tool;
1305+
1306+
const args: BashToolArgs = {
1307+
script: "cd ~/project && echo test",
1308+
timeout_secs: 5,
1309+
};
1310+
1311+
const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult;
1312+
1313+
expect(result.success).toBe(false);
1314+
if (!result.success) {
1315+
expect(result.error).toContain("Redundant cd");
1316+
}
1317+
});
1318+
1319+
it("should reject redundant cd with single tilde on SSH runtime", async () => {
1320+
const remoteCwd = "~";
1321+
using testEnv = createTestBashToolWithSSH(remoteCwd);
1322+
const tool = testEnv.tool;
1323+
1324+
const args: BashToolArgs = {
1325+
script: "cd ~ && echo test",
1326+
timeout_secs: 5,
1327+
};
1328+
1329+
const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult;
1330+
1331+
expect(result.success).toBe(false);
1332+
if (!result.success) {
1333+
expect(result.error).toContain("Redundant cd");
1334+
}
1335+
});
1336+
1337+
it("should handle trailing slashes in path comparison on SSH runtime", async () => {
1338+
const remoteCwd = "/home/user/project";
1339+
using testEnv = createTestBashToolWithSSH(remoteCwd);
1340+
const tool = testEnv.tool;
1341+
1342+
const args: BashToolArgs = {
1343+
script: "cd /home/user/project/ && echo test",
1344+
timeout_secs: 5,
1345+
};
1346+
1347+
const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult;
1348+
1349+
expect(result.success).toBe(false);
1350+
if (!result.success) {
1351+
expect(result.error).toContain("Redundant cd");
1352+
}
1353+
});
1354+
1355+
it("should handle cwd with trailing slash on SSH runtime", async () => {
1356+
const remoteCwd = "/home/user/project/";
1357+
using testEnv = createTestBashToolWithSSH(remoteCwd);
1358+
const tool = testEnv.tool;
1359+
1360+
const args: BashToolArgs = {
1361+
script: "cd /home/user/project && echo test",
1362+
timeout_secs: 5,
1363+
};
1364+
1365+
const result = (await tool.execute!(args, mockToolCallOptions)) as BashToolResult;
1366+
1367+
expect(result.success).toBe(false);
1368+
if (!result.success) {
1369+
expect(result.error).toContain("Redundant cd");
1370+
}
1371+
});
1372+
});
1373+

src/services/tools/bash.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -77,17 +77,15 @@ export const createBashTool: ToolFactory = (config: ToolConfiguration) => {
7777
let fileTruncated = false; // Hit 100KB file limit
7878

7979
// Detect redundant cd to working directory
80-
// Note: config.cwd is the actual execution path (local for LocalRuntime, remote for SSHRuntime)
81-
// Match patterns like: "cd /path &&", "cd /path;", "cd '/path' &&", "cd "/path" &&"
80+
// Delegate path normalization to the runtime for proper handling of local vs remote paths
8281
const cdPattern = /^\s*cd\s+['"]?([^'";\\&|]+)['"]?\s*[;&|]/;
8382
const match = cdPattern.exec(script);
8483
if (match) {
8584
const targetPath = match[1].trim();
86-
// For SSH runtime, config.cwd might use $HOME - need to handle this
87-
// Normalize paths for comparison (resolve to absolute where possible)
88-
// Note: This check is best-effort - it won't catch all cases on SSH (e.g., ~/path vs $HOME/path)
89-
const normalizedTarget = path.resolve(config.cwd, targetPath);
90-
const normalizedCwd = path.resolve(config.cwd);
85+
86+
// Use runtime's normalizePath method to handle path comparison correctly
87+
const normalizedTarget = config.runtime.normalizePath(targetPath, config.cwd);
88+
const normalizedCwd = config.runtime.normalizePath(".", config.cwd);
9189

9290
if (normalizedTarget === normalizedCwd) {
9391
return {

0 commit comments

Comments
 (0)