Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions src/core/tools/ExecuteCommandTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { CommandExecutionStatus, DEFAULT_TERMINAL_OUTPUT_PREVIEW_SIZE, Persisted
import { TelemetryService } from "@roo-code/telemetry"

import { Task } from "../task/Task"
import type { ClineProvider } from "../webview/ClineProvider"

import { ToolUse, ToolResponse } from "../../shared/tools"
import { formatResponse } from "../prompts/responses"
Expand Down Expand Up @@ -75,6 +76,12 @@ export function resolveAgentTimeoutMs(timeoutSeconds: number | null | undefined)
return process.env.ROO_CLI_RUNTIME === "1" ? 0 : requestedAgentTimeout
}

// Fire-and-forget: some call sites are synchronous terminal callbacks that cannot await,
// and postMessageToWebview swallows its own errors, so void is enough.
function postCommandExecutionStatus(provider: ClineProvider | undefined, status: CommandExecutionStatus): void {
void provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
}

export class ExecuteCommandTool extends BaseTool<"execute_command"> {
readonly name = "execute_command" as const

Expand Down Expand Up @@ -115,7 +122,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
status: "error",
message: parseError.message,
}
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(errorStatus) })
postCommandExecutionStatus(provider, errorStatus)
task.didToolFailInCurrentTurn = true
pushToolResult(formatResponse.toolError(parseError.message))
return
Expand Down Expand Up @@ -203,7 +210,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
if (canRetryShellIntegrationError(error)) {
// Silent retry via execa — shell startup race, command was not submitted.
const status: CommandExecutionStatus = { executionId, status: "fallback" }
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
postCommandExecutionStatus(provider, status)

const [rejected, result] = await executeCommandInTerminal(task, {
...options,
Expand Down Expand Up @@ -294,7 +301,7 @@ export async function executeCommandInTerminal(
// panel immediately (same effect as the retry-fallback path).
if (isCmdExeFallback) {
const status: CommandExecutionStatus = { executionId, status: "fallback" }
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
postCommandExecutionStatus(provider, status)
}

// Get global storage path for persisted output artifacts
Expand Down Expand Up @@ -394,7 +401,7 @@ export async function executeCommandInTerminal(
const compressedOutput = Terminal.compressTerminalOutput(accumulatedOutput)
latestCompressedOutput = compressedOutput
const status: CommandExecutionStatus = { executionId, status: "output", output: compressedOutput }
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
postCommandExecutionStatus(provider, status)
schedulePartialCommandOutputUpdate()
},
onCompleted: async (output: string | undefined) => {
Expand Down Expand Up @@ -433,11 +440,11 @@ export async function executeCommandInTerminal(
},
onShellExecutionStarted: (pid: number | undefined) => {
const status: CommandExecutionStatus = { executionId, status: "started", pid, command }
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
postCommandExecutionStatus(provider, status)
},
onShellExecutionComplete: (details: ExitCodeDetails) => {
const status: CommandExecutionStatus = { executionId, status: "exited", exitCode: details.exitCode }
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
postCommandExecutionStatus(provider, status)
exitDetails = details
},
}
Expand Down Expand Up @@ -506,7 +513,7 @@ export async function executeCommandInTerminal(
} catch (error) {
if (isUserTimedOut) {
const status: CommandExecutionStatus = { executionId, status: "timeout" }
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
postCommandExecutionStatus(provider, status)
await task.say("error", t("common:errors:command_timeout", { seconds: commandExecutionTimeoutSeconds }))
task.didToolFailInCurrentTurn = true
task.terminalProcess = undefined
Expand Down
19 changes: 12 additions & 7 deletions src/core/tools/UpdateTodoListTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,18 @@ export class UpdateTodoListTool extends BaseTool<"update_todo_list"> {
approvedTodoList !== undefined && JSON.stringify(normalizedTodos) !== JSON.stringify(approvedTodoList)
if (isTodoListChanged) {
normalizedTodos = approvedTodoList ?? []
task.say(
"user_edit_todos",
JSON.stringify({
tool: "updateTodoList",
todos: normalizedTodos,
}),
)
// Non-blocking: a failed notification must not abort persisting the edited list.
void task
.say(
"user_edit_todos",
JSON.stringify({
tool: "updateTodoList",
todos: normalizedTodos,
}),
)
.catch((error) => {
console.error("[UpdateTodoListTool] Failed to post user_edit_todos:", error)
})
}

await setTodoListForTask(task, normalizedTodos)
Expand Down
3 changes: 2 additions & 1 deletion src/core/tools/UseMcpToolTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,8 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {

private async sendExecutionStatus(task: Task, status: McpExecutionStatus): Promise<void> {
const clineProvider = await task.providerRef.deref()
clineProvider?.postMessageToWebview({
// Fire-and-forget: postMessageToWebview swallows its own errors, so void is enough.
void clineProvider?.postMessageToWebview({
type: "mcpExecutionStatus",
text: JSON.stringify(status),
})
Expand Down
82 changes: 70 additions & 12 deletions src/core/tools/__tests__/executeCommand.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe("executeCommand", () => {

// Create mock provider
mockProvider = {
postMessageToWebview: vitest.fn(),
postMessageToWebview: vitest.fn().mockResolvedValue(undefined),
getState: vitest.fn().mockResolvedValue({
terminalShellIntegrationDisabled: false,
}),
Expand Down Expand Up @@ -73,6 +73,12 @@ describe("executeCommand", () => {

// Mock TerminalRegistry.getOrCreateTerminal
;(TerminalRegistry.getOrCreateTerminal as any).mockResolvedValue(mockTerminal)
vitest.mocked(Terminal.isActiveShellCmdExe).mockReturnValue(false)
})

afterEach(() => {
vitest.useRealTimers()
vitest.restoreAllMocks()
})

describe("Working Directory Behavior", () => {
Expand All @@ -89,7 +95,7 @@ describe("executeCommand", () => {
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
// Simulate command completion
setTimeout(() => {
callbacks.onCompleted("Command output", mockProcess)
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand Down Expand Up @@ -128,7 +134,7 @@ describe("executeCommand", () => {
.fn()
.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command output", mockProcess)
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand Down Expand Up @@ -160,7 +166,7 @@ describe("executeCommand", () => {
.fn()
.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command output", mockProcess)
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand Down Expand Up @@ -190,7 +196,7 @@ describe("executeCommand", () => {
mockTerminal.getCurrentWorkingDirectory.mockReturnValue(customCwd)
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command output", mockProcess)
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand Down Expand Up @@ -219,7 +225,7 @@ describe("executeCommand", () => {
mockTerminal.getCurrentWorkingDirectory.mockReturnValue(resolvedCwd)
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command output", mockProcess)
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand Down Expand Up @@ -265,10 +271,34 @@ describe("executeCommand", () => {
})

describe("Terminal Provider Selection", () => {
it("posts fallback status when cmd.exe requires the Execa provider", async () => {
vitest.spyOn(Terminal, "isActiveShellCmdExe").mockReturnValue(true)
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
})

await executeCommandInTerminal(mockTask, {
executionId: "test-123",
command: "echo test",
terminalShellIntegrationDisabled: false,
})

expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
expect.objectContaining({
type: "commandExecutionStatus",
text: expect.stringContaining('"status":"fallback"'),
}),
)
})

it("should use vscode provider when shell integration is enabled", async () => {
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command output", mockProcess)
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand All @@ -290,7 +320,7 @@ describe("executeCommand", () => {
it("should use execa provider when shell integration is disabled", async () => {
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command output", mockProcess)
void callbacks.onCompleted("Command output", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand All @@ -311,11 +341,39 @@ describe("executeCommand", () => {
})

describe("Command Execution States", () => {
it("posts timeout status when command execution exceeds the user limit", async () => {
vitest.useFakeTimers()
const pendingProcess = Object.assign(new Promise<void>(() => {}), {
continue: vitest.fn(),
abort: vitest.fn(),
})
mockTerminal.runCommand.mockReturnValue(pendingProcess)

const executionPromise = executeCommandInTerminal(mockTask, {
executionId: "test-123",
command: "sleep 10",
terminalShellIntegrationDisabled: false,
commandExecutionTimeout: 1_000,
})
await vitest.advanceTimersByTimeAsync(1_000)
const [rejected, result] = await executionPromise

expect(rejected).toBe(false)
expect(result).toContain("terminated after exceeding")
expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith(
expect.objectContaining({
type: "commandExecutionStatus",
text: expect.stringContaining('"status":"timeout"'),
}),
)
expect(pendingProcess.abort).toHaveBeenCalled()
})

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)
void callbacks.onCompleted("Command completed successfully", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand All @@ -340,7 +398,7 @@ describe("executeCommand", () => {
mockTerminal.getCurrentWorkingDirectory.mockReturnValue("/test/project")
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command failed", mockProcess)
void callbacks.onCompleted("Command failed", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 1 }, mockProcess)
}, 0)
return mockProcess
Expand All @@ -366,7 +424,7 @@ describe("executeCommand", () => {
mockTerminal.getCurrentWorkingDirectory.mockReturnValue("/test/project")
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Command interrupted", mockProcess)
void callbacks.onCompleted("Command interrupted", mockProcess)
callbacks.onShellExecutionComplete(
{
exitCode: undefined,
Expand Down Expand Up @@ -411,7 +469,7 @@ describe("executeCommand", () => {
getCurrentWorkingDirectory: vitest.fn().mockReturnValue(updatedCwd),
runCommand: vitest.fn().mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
callbacks.onCompleted("Directory changed", mockProcess)
void callbacks.onCompleted("Directory changed", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
Expand Down
69 changes: 67 additions & 2 deletions src/core/tools/__tests__/executeCommandTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { formatResponse } from "../../prompts/responses"
import { ToolUse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools"
import { unescapeHtmlEntities } from "../../../utils/text-normalization"
import { Terminal } from "../../../integrations/terminal/Terminal"
import { TerminalRegistry } from "../../../integrations/terminal/TerminalRegistry"
import type { RooTerminalCallbacks, RooTerminalProcess } from "../../../integrations/terminal/types"

// Mock dependencies
Expand Down Expand Up @@ -97,7 +98,7 @@ describe("executeCommandTool", () => {
terminalOutputCharacterLimit: 100000,
terminalShellIntegrationDisabled: true,
}),
postMessageToWebview: vitest.fn(),
postMessageToWebview: vitest.fn().mockResolvedValue(undefined),
}),
},
lastMessageTs: Date.now(),
Expand Down Expand Up @@ -212,6 +213,70 @@ describe("executeCommandTool", () => {
})

describe("Error handling", () => {
it("reports command parse errors to the webview", async () => {
const provider = await mockCline.providerRef.deref()
mockToolUse.params.command = 'echo "unterminated'
mockToolUse.nativeArgs = { command: 'echo "unterminated' }

await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, {
askApproval: mockAskApproval as unknown as AskApproval,
handleError: mockHandleError as unknown as HandleError,
pushToolResult: mockPushToolResult as unknown as PushToolResult,
})

expect(provider.postMessageToWebview).toHaveBeenCalledWith(
expect.objectContaining({
type: "commandExecutionStatus",
text: expect.stringContaining('"status":"error"'),
}),
)
expect(mockAskApproval).not.toHaveBeenCalled()
})

it("posts fallback status when retrying a pre-submission shell integration failure", async () => {
const provider = await mockCline.providerRef.deref()
const shellError = new executeCommandModule.ShellIntegrationError("startup failed", false)
const failedProcess = Object.assign(Promise.reject(shellError), {
continue: vitest.fn(),
abort: vitest.fn(),
})
const successfulProcess = Object.assign(Promise.resolve(), {
continue: vitest.fn(),
abort: vitest.fn(),
})
// The terminal mock only needs the Promise surface used by this execution path.
const successfulTerminalProcess = successfulProcess as unknown as RooTerminalProcess

vitest
.mocked(TerminalRegistry.getOrCreateTerminal)
.mockResolvedValueOnce({
runCommand: vitest.fn().mockReturnValue(failedProcess),
getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"),
} as never)
.mockResolvedValueOnce({
runCommand: vitest.fn().mockImplementation((_command: string, callbacks: RooTerminalCallbacks) => {
void callbacks.onCompleted?.("", successfulTerminalProcess)
callbacks.onShellExecutionComplete?.({ exitCode: 0 }, successfulTerminalProcess)
return successfulProcess
}),
getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"),
} as never)

await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, {
askApproval: mockAskApproval as unknown as AskApproval,
handleError: mockHandleError as unknown as HandleError,
pushToolResult: mockPushToolResult as unknown as PushToolResult,
})

expect(provider.postMessageToWebview).toHaveBeenCalledWith(
expect.objectContaining({
type: "commandExecutionStatus",
text: expect.stringContaining('"status":"fallback"'),
}),
)
expect(TerminalRegistry.getOrCreateTerminal).toHaveBeenCalledTimes(2)
})

it.each([
[undefined, undefined, "executeCommand.destructiveCommandGuard.blocked"],
["matches a destructive pattern", undefined, "executeCommand.destructiveCommandGuard.blockedWithReason"],
Expand Down Expand Up @@ -580,7 +645,7 @@ describe("executeCommandTool", () => {
mockCline.providerRef.deref.mockResolvedValue({
contextProxy: { getValue: vitest.fn().mockReturnValue(false) },
getState: vitest.fn().mockResolvedValue({ terminalShellIntegrationDisabled: false }),
postMessageToWebview: vitest.fn(),
postMessageToWebview: vitest.fn().mockResolvedValue(undefined),
})
vitest.spyOn(Terminal, "isActiveShellCmdExe").mockReturnValue(false)
const terminal = await setupControllableTerminal()
Expand Down
Loading
Loading