Browse Source

fix: [5424] return the cwd in the exec tool's response so that the model is not lost after subsequent calls (#5667)

chris-garrett 5 months ago
parent
commit
a163053430
2 changed files with 475 additions and 2 deletions
  1. 474 0
      src/core/tools/__tests__/executeCommand.spec.ts
  2. 1 2
      src/core/tools/executeCommandTool.ts

+ 474 - 0
src/core/tools/__tests__/executeCommand.spec.ts

@@ -0,0 +1,474 @@
+//
+// Tests the ExecuteCommand tool itself vs calling the tool where the tool is mocked.
+//
+import * as path from "path"
+import * as fs from "fs/promises"
+
+import { ExecuteCommandOptions } from "../executeCommandTool"
+import { TerminalRegistry } from "../../../integrations/terminal/TerminalRegistry"
+import { Terminal } from "../../../integrations/terminal/Terminal"
+import { ExecaTerminal } from "../../../integrations/terminal/ExecaTerminal"
+import type { RooTerminalCallbacks } from "../../../integrations/terminal/types"
+
+// Mock fs to control directory existence checks
+vitest.mock("fs/promises")
+
+// Mock TerminalRegistry to control terminal creation
+vitest.mock("../../../integrations/terminal/TerminalRegistry")
+
+// Mock Terminal and ExecaTerminal classes
+vitest.mock("../../../integrations/terminal/Terminal")
+vitest.mock("../../../integrations/terminal/ExecaTerminal")
+
+// Import the actual executeCommand function (not mocked)
+import { executeCommand } from "../executeCommandTool"
+
+// Tests for the executeCommand function
+describe("executeCommand", () => {
+	let mockTask: any
+	let mockTerminal: any
+	let mockProcess: any
+	let mockProvider: any
+
+	beforeEach(() => {
+		vitest.clearAllMocks()
+
+		// Mock fs.access to simulate directory existence
+		;(fs.access as any).mockResolvedValue(undefined)
+
+		// Create mock provider
+		mockProvider = {
+			postMessageToWebview: vitest.fn(),
+			getState: vitest.fn().mockResolvedValue({
+				terminalOutputLineLimit: 500,
+				terminalShellIntegrationDisabled: false,
+			}),
+		}
+
+		// Create mock task
+		mockTask = {
+			cwd: "/test/project",
+			taskId: "test-task-123",
+			providerRef: {
+				deref: vitest.fn().mockResolvedValue(mockProvider),
+			},
+			say: vitest.fn().mockResolvedValue(undefined),
+			terminalProcess: undefined,
+		}
+
+		// Create mock process that resolves immediately
+		mockProcess = Promise.resolve()
+		mockProcess.continue = vitest.fn()
+
+		// Create mock terminal with getCurrentWorkingDirectory method
+		mockTerminal = {
+			provider: "vscode",
+			id: 1,
+			initialCwd: "/test/project",
+			getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/project"),
+			runCommand: vitest.fn().mockReturnValue(mockProcess),
+			terminal: {
+				show: vitest.fn(),
+			},
+		}
+
+		// Mock TerminalRegistry.getOrCreateTerminal
+		;(TerminalRegistry.getOrCreateTerminal as any).mockResolvedValue(mockTerminal)
+	})
+
+	describe("Working Directory Behavior", () => {
+		it("should use terminal.getCurrentWorkingDirectory() in the output message for completed commands", async () => {
+			// Setup: Mock terminal to return a different current working directory
+			const initialCwd = "/test/project"
+			const currentCwd = "/test/project/subdirectory"
+
+			mockTask.cwd = initialCwd
+			mockTerminal.initialCwd = initialCwd
+			mockTerminal.getCurrentWorkingDirectory.mockReturnValue(currentCwd)
+
+			// Mock the terminal process to complete successfully
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				// Simulate command completion
+				setTimeout(() => {
+					callbacks.onCompleted("Command output", mockProcess)
+					callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(mockTerminal.getCurrentWorkingDirectory).toHaveBeenCalled()
+			expect(result).toContain(`within working directory '${currentCwd}'`)
+			expect(result).not.toContain(`within working directory '${initialCwd}'`)
+		})
+
+		it("should use terminal.getCurrentWorkingDirectory() for VSCode Terminal with shell integration", async () => {
+			// Setup: Mock VSCode Terminal instance
+			const vscodeTerminal = new Terminal(1, undefined, "/test/project")
+			const mockVSCodeTerminal = vscodeTerminal as any
+
+			// Mock shell integration providing different cwd
+			mockVSCodeTerminal.terminal = {
+				show: vitest.fn(),
+				shellIntegration: {
+					cwd: { fsPath: "/test/project/changed-dir" },
+				},
+			}
+			mockVSCodeTerminal.getCurrentWorkingDirectory = vitest.fn().mockReturnValue("/test/project/changed-dir")
+			mockVSCodeTerminal.runCommand = vitest
+				.fn()
+				.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+					setTimeout(() => {
+						callbacks.onCompleted("Command output", mockProcess)
+						callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+					}, 0)
+					return mockProcess
+				})
+			;(TerminalRegistry.getOrCreateTerminal as any).mockResolvedValue(mockVSCodeTerminal)
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(result).toContain("within working directory '/test/project/changed-dir'")
+		})
+
+		it("should use terminal.getCurrentWorkingDirectory() for ExecaTerminal (always returns initialCwd)", async () => {
+			// Setup: Mock ExecaTerminal instance
+			const execaTerminal = new ExecaTerminal(1, "/test/project")
+			const mockExecaTerminal = execaTerminal as any
+
+			// ExecaTerminal always returns initialCwd
+			mockExecaTerminal.getCurrentWorkingDirectory = vitest.fn().mockReturnValue("/test/project")
+			mockExecaTerminal.runCommand = vitest
+				.fn()
+				.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+					setTimeout(() => {
+						callbacks.onCompleted("Command output", mockProcess)
+						callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+					}, 0)
+					return mockProcess
+				})
+			;(TerminalRegistry.getOrCreateTerminal as any).mockResolvedValue(mockExecaTerminal)
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				terminalShellIntegrationDisabled: true, // Forces ExecaTerminal
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(mockExecaTerminal.getCurrentWorkingDirectory).toHaveBeenCalled()
+			expect(result).toContain("within working directory '/test/project'")
+		})
+	})
+
+	describe("Custom Working Directory", () => {
+		it("should handle absolute custom cwd and use terminal.getCurrentWorkingDirectory() in output", async () => {
+			const customCwd = "/custom/absolute/path"
+
+			mockTerminal.getCurrentWorkingDirectory.mockReturnValue(customCwd)
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				setTimeout(() => {
+					callbacks.onCompleted("Command output", mockProcess)
+					callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				customCwd,
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(TerminalRegistry.getOrCreateTerminal).toHaveBeenCalledWith(
+				customCwd,
+				true, // customCwd provided
+				mockTask.taskId,
+				"vscode",
+			)
+			expect(result).toContain(`within working directory '${customCwd}'`)
+		})
+
+		it("should handle relative custom cwd and use terminal.getCurrentWorkingDirectory() in output", async () => {
+			const relativeCwd = "subdirectory"
+			const resolvedCwd = path.resolve(mockTask.cwd, relativeCwd)
+
+			mockTerminal.getCurrentWorkingDirectory.mockReturnValue(resolvedCwd)
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				setTimeout(() => {
+					callbacks.onCompleted("Command output", mockProcess)
+					callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				customCwd: relativeCwd,
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(TerminalRegistry.getOrCreateTerminal).toHaveBeenCalledWith(
+				resolvedCwd,
+				true, // customCwd provided
+				mockTask.taskId,
+				"vscode",
+			)
+			expect(result).toContain(`within working directory '${resolvedCwd.toPosix()}'`)
+		})
+
+		it("should return error when custom working directory does not exist", async () => {
+			const nonExistentCwd = "/non/existent/path"
+
+			// Mock fs.access to throw error for non-existent directory
+			;(fs.access as any).mockRejectedValue(new Error("Directory does not exist"))
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				customCwd: nonExistentCwd,
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(result).toBe(`Working directory '${nonExistentCwd}' does not exist.`)
+			expect(TerminalRegistry.getOrCreateTerminal).not.toHaveBeenCalled()
+		})
+	})
+
+	describe("Terminal Provider Selection", () => {
+		it("should use vscode provider when shell integration is enabled", async () => {
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				setTimeout(() => {
+					callbacks.onCompleted("Command output", mockProcess)
+					callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			await executeCommand(mockTask, options)
+
+			// Verify
+			expect(TerminalRegistry.getOrCreateTerminal).toHaveBeenCalledWith(
+				mockTask.cwd,
+				false, // no customCwd
+				mockTask.taskId,
+				"vscode",
+			)
+		})
+
+		it("should use execa provider when shell integration is disabled", async () => {
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				setTimeout(() => {
+					callbacks.onCompleted("Command output", mockProcess)
+					callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo test",
+				terminalShellIntegrationDisabled: true,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			await executeCommand(mockTask, options)
+
+			// Verify
+			expect(TerminalRegistry.getOrCreateTerminal).toHaveBeenCalledWith(
+				mockTask.cwd,
+				false, // no customCwd
+				mockTask.taskId,
+				"execa",
+			)
+		})
+	})
+
+	describe("Command Execution States", () => {
+		it("should handle completed command with exit code 0", async () => {
+			mockTerminal.getCurrentWorkingDirectory.mockReturnValue("/test/project")
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				setTimeout(() => {
+					callbacks.onCompleted("Command completed successfully", mockProcess)
+					callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "echo success",
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(result).toContain("Exit code: 0")
+			expect(result).toContain("within working directory '/test/project'")
+		})
+
+		it("should handle completed command with non-zero exit code", async () => {
+			mockTerminal.getCurrentWorkingDirectory.mockReturnValue("/test/project")
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				setTimeout(() => {
+					callbacks.onCompleted("Command failed", mockProcess)
+					callbacks.onShellExecutionComplete({ exitCode: 1 }, mockProcess)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "exit 1",
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(result).toContain("Command execution was not successful")
+			expect(result).toContain("Exit code: 1")
+			expect(result).toContain("within working directory '/test/project'")
+		})
+
+		it("should handle command terminated by signal", async () => {
+			mockTerminal.getCurrentWorkingDirectory.mockReturnValue("/test/project")
+			mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+				setTimeout(() => {
+					callbacks.onCompleted("Command interrupted", mockProcess)
+					callbacks.onShellExecutionComplete(
+						{
+							exitCode: undefined,
+							signalName: "SIGINT",
+							coreDumpPossible: false,
+						},
+						mockProcess,
+					)
+				}, 0)
+				return mockProcess
+			})
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "long-running-command",
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify
+			expect(rejected).toBe(false)
+			expect(result).toContain("Process terminated by signal SIGINT")
+			expect(result).toContain("within working directory '/test/project'")
+		})
+	})
+
+	describe("Terminal Working Directory Updates", () => {
+		it("should update working directory when terminal returns different cwd", async () => {
+			// Setup: Terminal initially at project root, but getCurrentWorkingDirectory returns different path
+			const initialCwd = "/test/project"
+			const updatedCwd = "/test/project/src"
+
+			mockTask.cwd = initialCwd
+			mockTerminal.initialCwd = initialCwd
+
+			// Mock Terminal instance behavior
+			const mockTerminalInstance = {
+				...mockTerminal,
+				terminal: { show: vitest.fn() },
+				getCurrentWorkingDirectory: vitest.fn().mockReturnValue(updatedCwd),
+				runCommand: vitest.fn().mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
+					setTimeout(() => {
+						callbacks.onCompleted("Directory changed", mockProcess)
+						callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
+					}, 0)
+					return mockProcess
+				}),
+			}
+
+			;(TerminalRegistry.getOrCreateTerminal as any).mockResolvedValue(mockTerminalInstance)
+
+			const options: ExecuteCommandOptions = {
+				executionId: "test-123",
+				command: "cd src && pwd",
+				terminalShellIntegrationDisabled: false,
+				terminalOutputLineLimit: 500,
+			}
+
+			// Execute
+			const [rejected, result] = await executeCommand(mockTask, options)
+
+			// Verify the result uses the updated working directory
+			expect(rejected).toBe(false)
+			expect(result).toContain(`within working directory '${updatedCwd}'`)
+			expect(result).not.toContain(`within working directory '${initialCwd}'`)
+
+			// Verify the terminal's getCurrentWorkingDirectory was called
+			expect(mockTerminalInstance.getCurrentWorkingDirectory).toHaveBeenCalled()
+		})
+	})
+})

+ 1 - 2
src/core/tools/executeCommandTool.ts

@@ -266,8 +266,7 @@ export async function executeCommand(
 			exitStatus = `Exit code: <undefined, notify user>`
 		}
 
-		let workingDirInfo = ` within working directory '${workingDir.toPosix()}'`
-		const newWorkingDir = terminal.getCurrentWorkingDirectory()
+		let workingDirInfo = ` within working directory '${terminal.getCurrentWorkingDirectory().toPosix()}'`
 
 		return [false, `Command executed in terminal ${workingDirInfo}. ${exitStatus}\nOutput:\n${result}`]
 	} else {