Skip to content

Commit e209c7c

Browse files
committed
fix(terminal): delay command_output ask for short foreground commands
The command_output ask fired on the first output chunk of any foreground command, prompting users even for commands about to complete. Schedule the ask instead so it only fires when the command is still running after a 5s delay, preserving the interrupt/feedback path for long-running commands while letting short commands finish without prompting. Closes #1042
1 parent d27153a commit e209c7c

2 files changed

Lines changed: 168 additions & 18 deletions

File tree

src/core/tools/ExecuteCommandTool.ts

Lines changed: 62 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,15 @@ export function canRetryShellIntegrationError(error: unknown): error is ShellInt
3535
return error instanceof ShellIntegrationError && !error.commandSubmitted
3636
}
3737

38+
/**
39+
* Grace period before a foreground command may trigger a `command_output` ask.
40+
* Short commands that emit output and exit within this window never prompt the
41+
* user; the ask only fires when the command is still running once the delay
42+
* elapses, so users can still interrupt or provide feedback on long-running
43+
* commands.
44+
*/
45+
export const COMMAND_OUTPUT_ASK_DELAY_MS = 5_000
46+
3847
export function getTerminalProviderForExecution(terminalShellIntegrationDisabled: boolean): {
3948
terminalProvider: RooTerminalProvider
4049
isCmdExeFallback: boolean
@@ -340,6 +349,51 @@ export async function executeCommandInTerminal(
340349
resolveOnCompleted = resolve
341350
})
342351

352+
// Delay the `command_output` ask so short foreground commands that emit
353+
// output and exit normally never prompt the user. The ask only fires if the
354+
// command is still running once COMMAND_OUTPUT_ASK_DELAY_MS has elapsed
355+
// since execution started, preserving the interrupt/feedback path for
356+
// long-running commands.
357+
let commandStartedAt = 0
358+
let commandOutputAskTimer: NodeJS.Timeout | undefined
359+
360+
const askForCommandOutput = async (process: RooTerminalProcess): Promise<void> => {
361+
if (runInBackground || hasAskedForCommandOutput || completed) {
362+
return
363+
}
364+
365+
// Mark that we've asked to prevent multiple concurrent asks
366+
hasAskedForCommandOutput = true
367+
368+
try {
369+
const { response, text, images } = await task.ask("command_output", "")
370+
runInBackground = true
371+
372+
if (response === "messageResponse") {
373+
message = { text, images }
374+
process.continue()
375+
}
376+
} catch (_error) {
377+
// Silently handle ask errors (e.g., "Current ask promise was ignored")
378+
}
379+
}
380+
381+
const scheduleCommandOutputAsk = (process: RooTerminalProcess): void => {
382+
if (runInBackground || hasAskedForCommandOutput || completed || commandOutputAskTimer) {
383+
return
384+
}
385+
386+
const remainingDelay = COMMAND_OUTPUT_ASK_DELAY_MS - (Date.now() - commandStartedAt)
387+
388+
commandOutputAskTimer = setTimeout(
389+
() => {
390+
commandOutputAskTimer = undefined
391+
void askForCommandOutput(process)
392+
},
393+
Math.max(remainingDelay, 0),
394+
)
395+
}
396+
343397
const callbacks: RooTerminalCallbacks = {
344398
onLine: async (lines: string, process: RooTerminalProcess) => {
345399
accumulatedOutput += lines
@@ -359,26 +413,12 @@ export async function executeCommandInTerminal(
359413
provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
360414
schedulePartialCommandOutputUpdate()
361415

362-
if (runInBackground || hasAskedForCommandOutput) {
363-
return
364-
}
365-
366-
// Mark that we've asked to prevent multiple concurrent asks
367-
hasAskedForCommandOutput = true
368-
369-
try {
370-
const { response, text, images } = await task.ask("command_output", "")
371-
runInBackground = true
372-
373-
if (response === "messageResponse") {
374-
message = { text, images }
375-
process.continue()
376-
}
377-
} catch (_error) {
378-
// Silently handle ask errors (e.g., "Current ask promise was ignored")
379-
}
416+
scheduleCommandOutputAsk(process)
380417
},
381418
onCompleted: async (output: string | undefined) => {
419+
clearTimeout(commandOutputAskTimer)
420+
commandOutputAskTimer = undefined
421+
382422
clearTimeout(pendingCommandOutputEmitTimer)
383423
pendingCommandOutputEmitTimer = undefined
384424

@@ -441,6 +481,7 @@ export async function executeCommandInTerminal(
441481
workingDir = terminal.getCurrentWorkingDirectory()
442482
}
443483

484+
commandStartedAt = Date.now()
444485
const process = terminal.runCommand(command, callbacks)
445486
task.terminalProcess = process
446487

@@ -462,6 +503,8 @@ export async function executeCommandInTerminal(
462503
new Promise<void>((resolve) => {
463504
agentTimeoutId = setTimeout(() => {
464505
runInBackground = true
506+
clearTimeout(commandOutputAskTimer)
507+
commandOutputAskTimer = undefined
465508
process.continue()
466509
task.supersedePendingAsk()
467510
resolve()
@@ -501,6 +544,7 @@ export async function executeCommandInTerminal(
501544
} finally {
502545
clearTimeout(agentTimeoutId)
503546
clearTimeout(userTimeoutId)
547+
clearTimeout(commandOutputAskTimer)
504548
clearTimeout(pendingCommandOutputEmitTimer)
505549
task.terminalProcess = undefined
506550
}

src/core/tools/__tests__/executeCommandTool.spec.ts

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { formatResponse } from "../../prompts/responses"
88
import { ToolUse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools"
99
import { unescapeHtmlEntities } from "../../../utils/text-normalization"
1010
import { Terminal } from "../../../integrations/terminal/Terminal"
11+
import type { RooTerminalCallbacks, RooTerminalProcess } from "../../../integrations/terminal/types"
1112

1213
// Mock dependencies
1314
vitest.mock("execa", () => ({
@@ -333,4 +334,109 @@ describe("executeCommandTool", () => {
333334
expect(executeCommandModule.resolveAgentTimeoutMs(30)).toBe(30_000)
334335
})
335336
})
337+
338+
describe("command_output ask policy", () => {
339+
type MockProcess = Promise<void> & {
340+
continue: ReturnType<typeof vitest.fn>
341+
abort: ReturnType<typeof vitest.fn>
342+
}
343+
344+
interface ControllableTerminal {
345+
callbacks: RooTerminalCallbacks | undefined
346+
proc: MockProcess
347+
resolveProcess: () => void
348+
}
349+
350+
const setupControllableTerminal = async (): Promise<ControllableTerminal> => {
351+
const { TerminalRegistry } = await import("../../../integrations/terminal/TerminalRegistry")
352+
const state: ControllableTerminal = {
353+
callbacks: undefined,
354+
proc: undefined as unknown as MockProcess,
355+
resolveProcess: () => {},
356+
}
357+
const processPromise = new Promise<void>((resolve) => {
358+
state.resolveProcess = resolve
359+
})
360+
state.proc = Object.assign(processPromise, { continue: vitest.fn(), abort: vitest.fn() })
361+
;(TerminalRegistry.getOrCreateTerminal as ReturnType<typeof vitest.fn>).mockResolvedValue({
362+
runCommand: vitest.fn((_cmd: string, callbacks: RooTerminalCallbacks) => {
363+
state.callbacks = callbacks
364+
return state.proc
365+
}),
366+
getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/workspace"),
367+
})
368+
return state
369+
}
370+
371+
const handleCommand = (command: string) => {
372+
mockToolUse.params.command = command
373+
mockToolUse.nativeArgs = { command }
374+
375+
return executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, {
376+
askApproval: mockAskApproval as unknown as AskApproval,
377+
handleError: mockHandleError as unknown as HandleError,
378+
pushToolResult: mockPushToolResult as unknown as PushToolResult,
379+
})
380+
}
381+
382+
it("does not ask about command output when a short command emits output and exits normally", async () => {
383+
vitest.useFakeTimers()
384+
const terminal = await setupControllableTerminal()
385+
386+
const handlePromise = handleCommand("echo hello")
387+
388+
await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined())
389+
const callbacks = terminal.callbacks!
390+
const proc = terminal.proc as unknown as RooTerminalProcess
391+
392+
await callbacks.onLine("hello\n", proc)
393+
await callbacks.onCompleted!("hello\n", proc)
394+
callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc)
395+
terminal.resolveProcess()
396+
397+
// Advance past the ask delay to prove the scheduled ask was cancelled
398+
// on completion, not merely deferred beyond the test's runtime.
399+
await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS + 1_000)
400+
401+
await handlePromise
402+
403+
expect(mockCline.ask).not.toHaveBeenCalled()
404+
expect(mockPushToolResult).toHaveBeenCalled()
405+
const result = mockPushToolResult.mock.calls[0][0]
406+
expect(result).toContain("hello")
407+
expect(result).toContain("Exit code: 0")
408+
})
409+
410+
it("asks about command output when the command is still running after the ask delay", async () => {
411+
vitest.useFakeTimers()
412+
mockCline.ask.mockResolvedValue({ response: "messageResponse", text: "keep going", images: undefined })
413+
const terminal = await setupControllableTerminal()
414+
415+
const handlePromise = handleCommand("sleep 60")
416+
417+
await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined())
418+
const callbacks = terminal.callbacks!
419+
const proc = terminal.proc as unknown as RooTerminalProcess
420+
421+
await callbacks.onLine("working...\n", proc)
422+
423+
// First output alone must not trigger the ask.
424+
expect(mockCline.ask).not.toHaveBeenCalled()
425+
426+
await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS)
427+
428+
expect(mockCline.ask).toHaveBeenCalledWith("command_output", "")
429+
expect(terminal.proc.continue).toHaveBeenCalled()
430+
431+
// Let the command finish so the tool can resolve.
432+
await callbacks.onCompleted!("working...\n", proc)
433+
callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc)
434+
terminal.resolveProcess()
435+
await vitest.advanceTimersByTimeAsync(100)
436+
437+
await handlePromise
438+
439+
expect(mockPushToolResult).toHaveBeenCalled()
440+
})
441+
})
336442
})

0 commit comments

Comments
 (0)