diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index f2fc4889f8..8383d9a4e1 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -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" @@ -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 @@ -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 @@ -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, @@ -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 @@ -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) => { @@ -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 }, } @@ -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 diff --git a/src/core/tools/UpdateTodoListTool.ts b/src/core/tools/UpdateTodoListTool.ts index 7414b713cf..4317696da6 100644 --- a/src/core/tools/UpdateTodoListTool.ts +++ b/src/core/tools/UpdateTodoListTool.ts @@ -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) diff --git a/src/core/tools/UseMcpToolTool.ts b/src/core/tools/UseMcpToolTool.ts index 9b2870060c..cd7c3d469d 100644 --- a/src/core/tools/UseMcpToolTool.ts +++ b/src/core/tools/UseMcpToolTool.ts @@ -287,7 +287,8 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> { private async sendExecutionStatus(task: Task, status: McpExecutionStatus): Promise { 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), }) diff --git a/src/core/tools/__tests__/executeCommand.spec.ts b/src/core/tools/__tests__/executeCommand.spec.ts index fd85beb0f4..7146fa930b 100644 --- a/src/core/tools/__tests__/executeCommand.spec.ts +++ b/src/core/tools/__tests__/executeCommand.spec.ts @@ -38,7 +38,7 @@ describe("executeCommand", () => { // Create mock provider mockProvider = { - postMessageToWebview: vitest.fn(), + postMessageToWebview: vitest.fn().mockResolvedValue(undefined), getState: vitest.fn().mockResolvedValue({ terminalShellIntegrationDisabled: false, }), @@ -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", () => { @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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(() => {}), { + 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 @@ -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 @@ -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, @@ -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 diff --git a/src/core/tools/__tests__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index 41b22a0e5f..a856b180ca 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -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 @@ -97,7 +98,7 @@ describe("executeCommandTool", () => { terminalOutputCharacterLimit: 100000, terminalShellIntegrationDisabled: true, }), - postMessageToWebview: vitest.fn(), + postMessageToWebview: vitest.fn().mockResolvedValue(undefined), }), }, lastMessageTs: Date.now(), @@ -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"], @@ -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() diff --git a/src/core/tools/__tests__/updateTodoListTool.spec.ts b/src/core/tools/__tests__/updateTodoListTool.spec.ts index ebe0500d66..6700764418 100644 --- a/src/core/tools/__tests__/updateTodoListTool.spec.ts +++ b/src/core/tools/__tests__/updateTodoListTool.spec.ts @@ -1,6 +1,43 @@ import { describe, it, expect, beforeEach, vi } from "vitest" -import { parseMarkdownChecklist } from "../UpdateTodoListTool" +import { parseMarkdownChecklist, setPendingTodoList, updateTodoListTool } from "../UpdateTodoListTool" import { TodoItem } from "@roo-code/types" +import type { Task } from "../../task/Task" +import type { ToolCallbacks } from "../BaseTool" + +describe("UpdateTodoListTool", () => { + it("persists the edited todo list even if the say notification fails", async () => { + const editedTodos: TodoItem[] = [{ id: "edited", content: "Edited task", status: "in_progress" }] + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined) + const task = { + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + didToolFailInCurrentTurn: false, + todoList: [], + say: vi.fn().mockRejectedValue(new Error("say failed")), + } as unknown as Task + const callbacks = { + pushToolResult: vi.fn(), + handleError: vi.fn(), + askApproval: vi.fn().mockImplementation(async () => { + setPendingTodoList(editedTodos) + return true + }), + } as unknown as ToolCallbacks + + await updateTodoListTool.execute({ todos: "[ ] Original task" }, task, callbacks) + await new Promise((resolve) => setImmediate(resolve)) + + // Notification is fire-and-forget: persistence happens regardless, and the + // rejection is logged rather than routed to handleError (which would abort). + expect(task.todoList).toEqual(editedTodos) + expect(callbacks.handleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[UpdateTodoListTool] Failed to post user_edit_todos:", + expect.any(Error), + ) + consoleErrorSpy.mockRestore() + }) +}) describe("parseMarkdownChecklist", () => { describe("standard checkbox format (without dash prefix)", () => { diff --git a/src/eslint.config.mjs b/src/eslint.config.mjs index 65965eb8d5..9c08a7fe39 100644 --- a/src/eslint.config.mjs +++ b/src/eslint.config.mjs @@ -34,7 +34,7 @@ export default [ { // Ratchet: enforce no-floating-promises directory by directory. Each // directory is added here once its floating promises are resolved. - files: ["activate/**/*.ts", "core/task/**/*.ts", "core/webview/**/*.ts"], + files: ["activate/**/*.ts", "core/task/**/*.ts", "core/tools/**/*.ts", "core/webview/**/*.ts"], languageOptions: { parserOptions: { project: true,