From 14c4f4907995fe0c6831609fdb7d8b4854bda1d4 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 15 Aug 2026 20:15:53 +0000 Subject: [PATCH 1/2] 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/2] 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)", () => {