Skip to content

[Chore] Enforce safe async handling in core tools #1255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zoomote wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Chore] Enforce safe async handling in core tools #1255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14c4f49
chore(lint): enforce no-floating-promises in core tools
roomote Aug 15, 2026
8701129
test: cover core tool promise branches
roomote Aug 15, 2026
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
23 changes: 16 additions & 7 deletions src/core/tools/ExecuteCommandTool.ts
View file
Open in desktop
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,14 @@ export function resolveAgentTimeoutMs(timeoutSeconds: number | null | undefined)
return process.env.ROO_CLI_RUNTIME === "1" ? 0 : requestedAgentTimeout
}

function postCommandExecutionStatus(provider: ClineProvider | undefined, status: CommandExecutionStatus): void {
void provider
?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
.catch((error) => {
console.error(`[ExecuteCommandTool] Failed to post ${status.status} command status:`, error)
})
}

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

Expand Down Expand Up @@ -115,7 +124,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 +212,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 +303,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 +403,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 +442,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 +515,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
2 changes: 1 addition & 1 deletion src/core/tools/UpdateTodoListTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export class UpdateTodoListTool extends BaseTool<"update_todo_list"> {
approvedTodoList !== undefined && JSON.stringify(normalizedTodos) !== JSON.stringify(approvedTodoList)
if (isTodoListChanged) {
normalizedTodos = approvedTodoList ?? []
task.say(
await task.say(
"user_edit_todos",
JSON.stringify({
tool: "updateTodoList",
Expand Down
12 changes: 8 additions & 4 deletions src/core/tools/UseMcpToolTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,14 @@ 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({
type: "mcpExecutionStatus",
text: JSON.stringify(status),
})
try {
await clineProvider?.postMessageToWebview({
type: "mcpExecutionStatus",
text: JSON.stringify(status),
})
} catch (error) {
console.error(`[UseMcpToolTool] Failed to post ${status.status} execution status:`, error)
}
}

private processToolContent(toolResult: any): { text: string; images: string[] } {
Expand Down
110 changes: 98 additions & 12 deletions src/core/tools/__tests__/executeCommand.spec.ts
View file
Open in desktop
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,67 @@ 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("logs rejected command status updates without failing the command", async () => {
const postError = new Error("webview unavailable")
const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(() => undefined)
mockProvider.postMessageToWebview.mockRejectedValue(postError)
mockTerminal.runCommand.mockImplementation((command: string, callbacks: RooTerminalCallbacks) => {
setTimeout(() => {
void callbacks.onCompleted("Command completed successfully", mockProcess)
callbacks.onShellExecutionComplete({ exitCode: 0 }, mockProcess)
}, 0)
return mockProcess
})

const [rejected, result] = await executeCommandInTerminal(mockTask, {
executionId: "test-123",
command: "echo success",
terminalShellIntegrationDisabled: false,
})
await new Promise<void>((resolve) => setImmediate(resolve))

expect(rejected).toBe(false)
expect(result).toContain("Exit code: 0")
expect(consoleErrorSpy).toHaveBeenCalledWith(
"[ExecuteCommandTool] Failed to post exited command status:",
postError,
)
consoleErrorSpy.mockRestore()
})

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 +426,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 +452,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 +497,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
Loading
Loading