diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index 3d6a44ef18..6e8a9becb4 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -35,6 +35,15 @@ export function canRetryShellIntegrationError(error: unknown): error is ShellInt return error instanceof ShellIntegrationError && !error.commandSubmitted } +/** + * Grace period before a foreground command may trigger a `command_output` ask. + * Short commands that emit output and exit within this window never prompt the + * user; the ask only fires when the command is still running once the delay + * elapses, so users can still interrupt or provide feedback on long-running + * commands. + */ +export const COMMAND_OUTPUT_ASK_DELAY_MS = 5_000 + export function getTerminalProviderForExecution(terminalShellIntegrationDisabled: boolean): { terminalProvider: RooTerminalProvider isCmdExeFallback: boolean @@ -340,6 +349,58 @@ export async function executeCommandInTerminal( resolveOnCompleted = resolve }) + // Delay the `command_output` ask so short foreground commands that emit + // output and exit normally never prompt the user. The ask only fires if the + // command is still running once COMMAND_OUTPUT_ASK_DELAY_MS has elapsed + // since execution started, preserving the interrupt/feedback path for + // long-running commands. The anchor is re-based to onShellExecutionStarted + // (falling back to the pre-runCommand timestamp when that event never + // fires) so shell-integration startup on cold terminals does not consume + // the grace period. + let commandStartedAt = 0 + let commandOutputAskTimer: NodeJS.Timeout | undefined + + const askForCommandOutput = async (process: RooTerminalProcess): Promise => { + if (runInBackground || hasAskedForCommandOutput || completed) { + return + } + + // Mark that we've asked to prevent multiple concurrent asks + hasAskedForCommandOutput = true + + try { + const { response, text, images } = await task.ask("command_output", "") + runInBackground = true + + if (response === "messageResponse") { + message = { text, images } + } + + // Any answer means the command should keep running in the background; + // continue the process so the tool resolves now instead of blocking + // until the command actually completes. + process.continue() + } catch (_error) { + // Silently handle ask errors (e.g., "Current ask promise was ignored") + } + } + + const scheduleCommandOutputAsk = (process: RooTerminalProcess): void => { + if (runInBackground || hasAskedForCommandOutput || completed || commandOutputAskTimer) { + return + } + + const remainingDelay = COMMAND_OUTPUT_ASK_DELAY_MS - (Date.now() - commandStartedAt) + + commandOutputAskTimer = setTimeout( + () => { + commandOutputAskTimer = undefined + void askForCommandOutput(process) + }, + Math.max(remainingDelay, 0), + ) + } + const callbacks: RooTerminalCallbacks = { onLine: async (lines: string, process: RooTerminalProcess) => { accumulatedOutput += lines @@ -359,26 +420,19 @@ export async function executeCommandInTerminal( provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) }) schedulePartialCommandOutputUpdate() - if (runInBackground || hasAskedForCommandOutput) { - return - } - - // Mark that we've asked to prevent multiple concurrent asks - hasAskedForCommandOutput = true - - try { - const { response, text, images } = await task.ask("command_output", "") - runInBackground = true - - if (response === "messageResponse") { - message = { text, images } - process.continue() - } - } catch (_error) { - // Silently handle ask errors (e.g., "Current ask promise was ignored") - } + scheduleCommandOutputAsk(process) }, onCompleted: async (output: string | undefined) => { + clearTimeout(commandOutputAskTimer) + commandOutputAskTimer = undefined + + // If an interactive command_output ask is still pending, supersede it + // so it resolves immediately instead of lingering until the next + // interactive message bumps lastMessageTs. + if (hasAskedForCommandOutput && !runInBackground) { + task.supersedePendingAsk() + } + clearTimeout(pendingCommandOutputEmitTimer) pendingCommandOutputEmitTimer = undefined @@ -412,9 +466,21 @@ export async function executeCommandInTerminal( console.error("[ExecuteCommandTool] Failed to flush final command_output:", error) }) }, - onShellExecutionStarted: (pid: number | undefined) => { + onShellExecutionStarted: (pid: number | undefined, process: RooTerminalProcess) => { const status: CommandExecutionStatus = { executionId, status: "started", pid, command } provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) }) + + // Re-anchor the ask delay to actual execution start so the shell + // integration startup wait does not count against the grace period. + commandStartedAt = Date.now() + + // Output should not precede this event, but if it did, reschedule + // the pending ask against the corrected anchor. + if (commandOutputAskTimer) { + clearTimeout(commandOutputAskTimer) + commandOutputAskTimer = undefined + scheduleCommandOutputAsk(process) + } }, onShellExecutionComplete: (details: ExitCodeDetails) => { const status: CommandExecutionStatus = { executionId, status: "exited", exitCode: details.exitCode } @@ -441,6 +507,8 @@ export async function executeCommandInTerminal( workingDir = terminal.getCurrentWorkingDirectory() } + // Fallback anchor for providers that never fire onShellExecutionStarted. + commandStartedAt = Date.now() const process = terminal.runCommand(command, callbacks) task.terminalProcess = process @@ -462,6 +530,8 @@ export async function executeCommandInTerminal( new Promise((resolve) => { agentTimeoutId = setTimeout(() => { runInBackground = true + clearTimeout(commandOutputAskTimer) + commandOutputAskTimer = undefined process.continue() task.supersedePendingAsk() resolve() @@ -501,6 +571,7 @@ export async function executeCommandInTerminal( } finally { clearTimeout(agentTimeoutId) clearTimeout(userTimeoutId) + clearTimeout(commandOutputAskTimer) clearTimeout(pendingCommandOutputEmitTimer) task.terminalProcess = undefined } diff --git a/src/core/tools/__tests__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index 1725154328..c3236f3565 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 type { RooTerminalCallbacks, RooTerminalProcess } from "../../../integrations/terminal/types" // Mock dependencies vitest.mock("execa", () => ({ @@ -77,6 +78,7 @@ describe("executeCommandTool", () => { }, recordToolUsage: vitest.fn().mockReturnValue({} as ToolUsage), recordToolError: vitest.fn(), + supersedePendingAsk: vitest.fn(), providerRef: { deref: vitest.fn().mockResolvedValue({ getState: vitest.fn().mockResolvedValue({ @@ -333,4 +335,338 @@ describe("executeCommandTool", () => { expect(executeCommandModule.resolveAgentTimeoutMs(30)).toBe(30_000) }) }) + + describe("command_output ask policy", () => { + type MockProcess = Promise & { + continue: ReturnType + abort: ReturnType + } + + interface ControllableTerminal { + callbacks: RooTerminalCallbacks | undefined + proc: MockProcess + resolveProcess: () => void + } + + const setupControllableTerminal = async (): Promise => { + const { TerminalRegistry } = await import("../../../integrations/terminal/TerminalRegistry") + const state: ControllableTerminal = { + callbacks: undefined, + proc: undefined as unknown as MockProcess, + resolveProcess: () => {}, + } + const processPromise = new Promise((resolve) => { + state.resolveProcess = resolve + }) + // Mirror real terminal behavior: continue() resolves the wait early + // while the command keeps running in the background. + state.proc = Object.assign(processPromise, { + continue: vitest.fn(() => state.resolveProcess()), + abort: vitest.fn(), + }) + ;(TerminalRegistry.getOrCreateTerminal as ReturnType).mockResolvedValue({ + runCommand: vitest.fn((_cmd: string, callbacks: RooTerminalCallbacks) => { + state.callbacks = callbacks + return state.proc + }), + getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"), + }) + return state + } + + const handleCommand = (command: string, timeout?: number) => { + mockToolUse.params.command = command + mockToolUse.params.timeout = timeout === undefined ? undefined : String(timeout) + mockToolUse.nativeArgs = timeout === undefined ? { command } : { command, timeout } + + return 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, + }) + } + + it("does not ask about command output when a short command emits output and exits normally", async () => { + vitest.useFakeTimers() + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("echo hello") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("hello\n", proc) + await callbacks.onCompleted!("hello\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + terminal.resolveProcess() + + // Advance past the ask delay to prove the scheduled ask was cancelled + // on completion, not merely deferred beyond the test's runtime. + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS + 1_000) + + await handlePromise + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalled() + const result = mockPushToolResult.mock.calls[0][0] + expect(result).toContain("hello") + expect(result).toContain("Exit code: 0") + }) + + it("asks about command output when the command is still running after the ask delay", async () => { + vitest.useFakeTimers() + mockCline.ask.mockResolvedValue({ response: "messageResponse", text: "keep going", images: undefined }) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("sleep 60") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("working...\n", proc) + + // First output alone must not trigger the ask. + expect(mockCline.ask).not.toHaveBeenCalled() + + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS) + + expect(mockCline.ask).toHaveBeenCalledWith("command_output", "") + expect(terminal.proc.continue).toHaveBeenCalled() + + // Further output after the ask must not schedule another ask. + await callbacks.onLine("still working...\n", proc) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + // Let the command finish so the tool can resolve. + await callbacks.onCompleted!("working...\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(100) + + await handlePromise + + expect(mockPushToolResult).toHaveBeenCalled() + }) + + it("anchors the ask delay to execution start so shell integration startup does not consume it", async () => { + vitest.useFakeTimers() + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("echo hello") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + // Simulate a cold terminal spending most of the grace period waiting + // for shell integration before the command actually starts. + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS - 2_000) + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("hello\n", proc) + + // Past the pre-runCommand anchor deadline but well within the window + // measured from execution start: still no ask. + await vitest.advanceTimersByTimeAsync(2_500) + expect(mockCline.ask).not.toHaveBeenCalled() + + await callbacks.onCompleted!("hello\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS + 1_000) + + await handlePromise + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalled() + }) + + it("re-anchors a pending ask when execution start is reported after early output", async () => { + vitest.useFakeTimers() + mockCline.ask.mockResolvedValue({ response: "messageResponse", text: "keep going", images: undefined }) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("sleep 60") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + // Output arrives before the execution-started event (defensive case). + await callbacks.onLine("working...\n", proc) + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS - 2_000) + callbacks.onShellExecutionStarted!(1234, proc) + + // The pending ask was rescheduled against the new anchor, so the old + // deadline passing must not fire it. + await vitest.advanceTimersByTimeAsync(2_500) + expect(mockCline.ask).not.toHaveBeenCalled() + + // The ask must still fire at the re-anchored deadline — a version + // that cleared the old timer without rescheduling would fail here. + await vitest.advanceTimersByTimeAsync(2_500) + expect(mockCline.ask).toHaveBeenCalledWith("command_output", "") + + // Let the command finish so the tool can resolve. + await callbacks.onCompleted!("working...\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + await vitest.advanceTimersByTimeAsync(100) + + await handlePromise + + expect(mockPushToolResult).toHaveBeenCalled() + }) + + it("cancels a pending ask when the agent timeout moves the command to the background", async () => { + vitest.useFakeTimers() + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("npm run dev", 2) + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("server starting...\n", proc) + + // Agent timeout (2s) fires before the ask delay (5s). + await vitest.advanceTimersByTimeAsync(2_000) + expect(terminal.proc.continue).toHaveBeenCalled() + expect(mockCline.supersedePendingAsk).toHaveBeenCalled() + + // Output after the background transition must never schedule an ask. + await callbacks.onLine("listening...\n", proc) + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS + 1_000) + expect(mockCline.ask).not.toHaveBeenCalled() + + await handlePromise + + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockPushToolResult.mock.calls[0][0]).toContain("still running") + }) + + it("falls back to the command dispatch time when execution start is never reported", async () => { + vitest.useFakeTimers() + mockCline.ask.mockResolvedValue({ response: "messageResponse", text: "keep going", images: undefined }) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("sleep 60") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + // No onShellExecutionStarted: the pre-runCommand anchor applies. + await callbacks.onLine("working...\n", proc) + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS) + + expect(mockCline.ask).toHaveBeenCalledWith("command_output", "") + + await callbacks.onCompleted!("working...\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(100) + + await handlePromise + + expect(mockPushToolResult).toHaveBeenCalled() + }) + + it("swallows ask errors without failing the command", async () => { + vitest.useFakeTimers() + mockCline.ask.mockRejectedValue(new Error("Current ask promise was ignored")) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("sleep 60") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("working...\n", proc) + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS) + + expect(mockCline.ask).toHaveBeenCalledWith("command_output", "") + expect(terminal.proc.continue).not.toHaveBeenCalled() + + await callbacks.onCompleted!("working...\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(100) + + await handlePromise + + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockPushToolResult.mock.calls[0][0]).toContain("Exit code: 0") + }) + + it("resolves before completion when the ask is answered without a message", async () => { + // Note: in production only messageResponse answers reach a + // command_output ask (Proceed/Kill route through terminalOperation); + // yesButtonClicked is synthetic here to pin the non-message branch. + vitest.useFakeTimers() + mockCline.ask.mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("sleep 60") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("working...\n", proc) + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS) + + // Any ask answer backgrounds the command: the process is continued and + // the tool resolves without waiting for the command to complete. + // Note the process promise is never resolved in this test. + await vitest.advanceTimersByTimeAsync(100) + await handlePromise + + expect(mockCline.ask).toHaveBeenCalledWith("command_output", "") + expect(terminal.proc.continue).toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockPushToolResult.mock.calls[0][0]).toContain("still running") + + // Cleanup: let the command finish. + await callbacks.onCompleted!("working...\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + }) + + it("supersedes a pending ask when the command completes", async () => { + vitest.useFakeTimers() + mockCline.ask.mockReturnValue(new Promise(() => {})) + const terminal = await setupControllableTerminal() + + const handlePromise = handleCommand("sleep 5") + + await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined()) + const callbacks = terminal.callbacks! + const proc = terminal.proc as unknown as RooTerminalProcess + + callbacks.onShellExecutionStarted!(1234, proc) + await callbacks.onLine("working...\n", proc) + await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS) + + expect(mockCline.ask).toHaveBeenCalledWith("command_output", "") + + // The command completes while the ask is still pending. + await callbacks.onCompleted!("working...\n", proc) + callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc) + terminal.resolveProcess() + await vitest.advanceTimersByTimeAsync(100) + + await handlePromise + + expect(mockCline.supersedePendingAsk).toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockPushToolResult.mock.calls[0][0]).toContain("Exit code: 0") + }) + }) }) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index 01c4796c16..0410336609 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -462,6 +462,17 @@ const ChatViewComponent: React.ForwardRefRenderFunction { }), ) }) + + it("clears the command_output controls when the final output arrives", async () => { + const { getByTestId, getByRole, queryByRole } = renderChatView() + + // Hydrate state with a command_output ask (Proceed/Kill controls visible) + mockPostMessage({ + clineMessages: [ + { + type: "say", + say: "task", + ts: Date.now() - 2000, + text: "Initial task", + }, + { + type: "ask", + ask: "command_output", + ts: Date.now() - 1000, + text: "", + partial: false, + }, + ], + }) + + await waitFor(() => { + expect(getByTestId("chat-textarea")).toBeInTheDocument() + }) + + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)) + }) + + expect(getByRole("button", { name: "chat:proceedWhileRunning.title" })).toBeInTheDocument() + + // The command completes: the final non-partial command_output say arrives. + mockPostMessage({ + clineMessages: [ + { + type: "say", + say: "task", + ts: Date.now() - 2000, + text: "Initial task", + }, + { + type: "ask", + ask: "command_output", + ts: Date.now() - 1000, + text: "", + partial: false, + }, + { + type: "say", + say: "command_output", + ts: Date.now(), + text: "done\n", + partial: false, + }, + ], + }) + + await waitFor(() => { + expect(queryByRole("button", { name: "chat:proceedWhileRunning.title" })).not.toBeInTheDocument() + }) + }) }) describe("ChatView - Follow-up Suggestions", () => {