From 14c4f4907995fe0c6831609fdb7d8b4854bda1d4 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 15 Aug 2026 20:15:53 +0000 Subject: [PATCH 1/3] chore(lint): enforce no-floating-promises in core tools --- src/core/tools/ExecuteCommandTool.ts | 23 +++++--- src/core/tools/UpdateTodoListTool.ts | 2 +- src/core/tools/UseMcpToolTool.ts | 12 +++-- .../tools/__tests__/executeCommand.spec.ts | 52 ++++++++++++++----- .../__tests__/executeCommandTool.spec.ts | 4 +- src/eslint.config.mjs | 2 +- 6 files changed, 68 insertions(+), 27 deletions(-) diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index f2fc4889f8..4838bb508c 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,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 @@ -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 @@ -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, @@ -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 @@ -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) => { @@ -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 }, } @@ -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 diff --git a/src/core/tools/UpdateTodoListTool.ts b/src/core/tools/UpdateTodoListTool.ts index 7414b713cf..9161e138ac 100644 --- a/src/core/tools/UpdateTodoListTool.ts +++ b/src/core/tools/UpdateTodoListTool.ts @@ -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", diff --git a/src/core/tools/UseMcpToolTool.ts b/src/core/tools/UseMcpToolTool.ts index 9b2870060c..4796a0fea4 100644 --- a/src/core/tools/UseMcpToolTool.ts +++ b/src/core/tools/UseMcpToolTool.ts @@ -287,10 +287,14 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> { private async sendExecutionStatus(task: Task, status: McpExecutionStatus): Promise { 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[] } { diff --git a/src/core/tools/__tests__/executeCommand.spec.ts b/src/core/tools/__tests__/executeCommand.spec.ts index fd85beb0f4..7a1b0c0fa0 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, }), @@ -89,7 +89,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 +128,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 +160,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 +190,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 +219,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 @@ -268,7 +268,7 @@ describe("executeCommand", () => { 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 +290,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 +311,39 @@ describe("executeCommand", () => { }) describe("Command Execution States", () => { + 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((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 @@ -340,7 +368,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 +394,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 +439,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..3232443128 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -97,7 +97,7 @@ describe("executeCommandTool", () => { terminalOutputCharacterLimit: 100000, terminalShellIntegrationDisabled: true, }), - postMessageToWebview: vitest.fn(), + postMessageToWebview: vitest.fn().mockResolvedValue(undefined), }), }, lastMessageTs: Date.now(), @@ -580,7 +580,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/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, From 870112959bc9a527cfbbf4a4a2ff27650c150f2d Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 15 Aug 2026 21:25:00 +0000 Subject: [PATCH 2/3] test: cover core tool promise branches --- .../tools/__tests__/executeCommand.spec.ts | 58 +++++++++++++++++ .../__tests__/executeCommandTool.spec.ts | 65 +++++++++++++++++++ .../__tests__/updateTodoListTool.spec.ts | 38 ++++++++++- 3 files changed, 160 insertions(+), 1 deletion(-) diff --git a/src/core/tools/__tests__/executeCommand.spec.ts b/src/core/tools/__tests__/executeCommand.spec.ts index 7a1b0c0fa0..717fead659 100644 --- a/src/core/tools/__tests__/executeCommand.spec.ts +++ b/src/core/tools/__tests__/executeCommand.spec.ts @@ -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", () => { @@ -265,6 +271,30 @@ 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(() => { @@ -311,6 +341,34 @@ 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("logs rejected command status updates without failing the command", async () => { const postError = new Error("webview unavailable") const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(() => undefined) diff --git a/src/core/tools/__tests__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index 3232443128..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 @@ -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"], diff --git a/src/core/tools/__tests__/updateTodoListTool.spec.ts b/src/core/tools/__tests__/updateTodoListTool.spec.ts index ebe0500d66..614a5cf46d 100644 --- a/src/core/tools/__tests__/updateTodoListTool.spec.ts +++ b/src/core/tools/__tests__/updateTodoListTool.spec.ts @@ -1,6 +1,42 @@ 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("waits for the user-edited todo message before persisting the edited list", async () => { + let resolveSay: (() => void) | undefined + const sayPromise = new Promise((resolve) => { + resolveSay = resolve + }) + const editedTodos: TodoItem[] = [{ id: "edited", content: "Edited task", status: "in_progress" }] + const task = { + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + didToolFailInCurrentTurn: false, + todoList: [], + say: vi.fn().mockReturnValue(sayPromise), + } 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 + + const executionPromise = updateTodoListTool.execute({ todos: "[ ] Original task" }, task, callbacks) + await vi.waitFor(() => expect(task.say).toHaveBeenCalled()) + + expect(task.todoList).toEqual([]) + resolveSay?.() + await executionPromise + + expect(task.todoList).toEqual(editedTodos) + }) +}) describe("parseMarkdownChecklist", () => { describe("standard checkbox format (without dash prefix)", () => { From 01541a99519376175b7f48c51e443889df47e519 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Fri, 21 Aug 2026 21:49:28 -0400 Subject: [PATCH 3/3] refactor(core/tools): drop unreachable webview-post catches, make notifications non-blocking --- src/core/tools/ExecuteCommandTool.ts | 8 ++---- src/core/tools/UpdateTodoListTool.ts | 19 ++++++++----- src/core/tools/UseMcpToolTool.ts | 13 ++++----- .../tools/__tests__/executeCommand.spec.ts | 28 ------------------- .../__tests__/updateTodoListTool.spec.ts | 25 +++++++++-------- 5 files changed, 33 insertions(+), 60 deletions(-) diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index 4838bb508c..8383d9a4e1 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -76,12 +76,10 @@ 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) }) - .catch((error) => { - console.error(`[ExecuteCommandTool] Failed to post ${status.status} command status:`, error) - }) + void provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) }) } export class ExecuteCommandTool extends BaseTool<"execute_command"> { diff --git a/src/core/tools/UpdateTodoListTool.ts b/src/core/tools/UpdateTodoListTool.ts index 9161e138ac..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 ?? [] - await 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 4796a0fea4..cd7c3d469d 100644 --- a/src/core/tools/UseMcpToolTool.ts +++ b/src/core/tools/UseMcpToolTool.ts @@ -287,14 +287,11 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> { private async sendExecutionStatus(task: Task, status: McpExecutionStatus): Promise { const clineProvider = await task.providerRef.deref() - try { - await clineProvider?.postMessageToWebview({ - type: "mcpExecutionStatus", - text: JSON.stringify(status), - }) - } catch (error) { - console.error(`[UseMcpToolTool] Failed to post ${status.status} execution status:`, error) - } + // Fire-and-forget: postMessageToWebview swallows its own errors, so void is enough. + void clineProvider?.postMessageToWebview({ + type: "mcpExecutionStatus", + text: JSON.stringify(status), + }) } private processToolContent(toolResult: any): { text: string; images: string[] } { diff --git a/src/core/tools/__tests__/executeCommand.spec.ts b/src/core/tools/__tests__/executeCommand.spec.ts index 717fead659..7146fa930b 100644 --- a/src/core/tools/__tests__/executeCommand.spec.ts +++ b/src/core/tools/__tests__/executeCommand.spec.ts @@ -369,34 +369,6 @@ describe("executeCommand", () => { 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((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) => { diff --git a/src/core/tools/__tests__/updateTodoListTool.spec.ts b/src/core/tools/__tests__/updateTodoListTool.spec.ts index 614a5cf46d..6700764418 100644 --- a/src/core/tools/__tests__/updateTodoListTool.spec.ts +++ b/src/core/tools/__tests__/updateTodoListTool.spec.ts @@ -5,18 +5,15 @@ import type { Task } from "../../task/Task" import type { ToolCallbacks } from "../BaseTool" describe("UpdateTodoListTool", () => { - it("waits for the user-edited todo message before persisting the edited list", async () => { - let resolveSay: (() => void) | undefined - const sayPromise = new Promise((resolve) => { - resolveSay = resolve - }) + 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().mockReturnValue(sayPromise), + say: vi.fn().mockRejectedValue(new Error("say failed")), } as unknown as Task const callbacks = { pushToolResult: vi.fn(), @@ -27,14 +24,18 @@ describe("UpdateTodoListTool", () => { }), } as unknown as ToolCallbacks - const executionPromise = updateTodoListTool.execute({ todos: "[ ] Original task" }, task, callbacks) - await vi.waitFor(() => expect(task.say).toHaveBeenCalled()) - - expect(task.todoList).toEqual([]) - resolveSay?.() - await executionPromise + 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() }) })