Skip to content

Commit ab70d8f

Browse files
committed
fix(terminal): clean up pending command_output ask on completion
- Supersede a still-pending command_output ask in onCompleted so it resolves immediately instead of lingering until the next interactive message, and clear the Proceed/Kill controls in the webview when the final non-partial command_output say arrives. - Continue the process for any ask answer, not just typed messages, so a non-message response actually backgrounds the command and the tool resolves before the process completes. - Strengthen ask-policy tests: assert the rescheduled ask still fires at the re-anchored deadline, assert early resolution for non-message answers, and cover ask supersession on completion.
1 parent ac20ae6 commit ab70d8f

4 files changed

Lines changed: 140 additions & 11 deletions

File tree

src/core/tools/ExecuteCommandTool.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,8 +374,12 @@ export async function executeCommandInTerminal(
374374

375375
if (response === "messageResponse") {
376376
message = { text, images }
377-
process.continue()
378377
}
378+
379+
// Any answer means the command should keep running in the background;
380+
// continue the process so the tool resolves now instead of blocking
381+
// until the command actually completes.
382+
process.continue()
379383
} catch (_error) {
380384
// Silently handle ask errors (e.g., "Current ask promise was ignored")
381385
}
@@ -422,6 +426,13 @@ export async function executeCommandInTerminal(
422426
clearTimeout(commandOutputAskTimer)
423427
commandOutputAskTimer = undefined
424428

429+
// If an interactive command_output ask is still pending, supersede it
430+
// so it resolves immediately instead of lingering until the next
431+
// interactive message bumps lastMessageTs.
432+
if (hasAskedForCommandOutput && !runInBackground) {
433+
task.supersedePendingAsk()
434+
}
435+
425436
clearTimeout(pendingCommandOutputEmitTimer)
426437
pendingCommandOutputEmitTimer = undefined
427438

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

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ describe("executeCommandTool", () => {
7878
},
7979
recordToolUsage: vitest.fn().mockReturnValue({} as ToolUsage),
8080
recordToolError: vitest.fn(),
81+
supersedePendingAsk: vitest.fn(),
8182
providerRef: {
8283
deref: vitest.fn().mockResolvedValue({
8384
getState: vitest.fn().mockResolvedValue({
@@ -357,7 +358,12 @@ describe("executeCommandTool", () => {
357358
const processPromise = new Promise<void>((resolve) => {
358359
state.resolveProcess = resolve
359360
})
360-
state.proc = Object.assign(processPromise, { continue: vitest.fn(), abort: vitest.fn() })
361+
// Mirror real terminal behavior: continue() resolves the wait early
362+
// while the command keeps running in the background.
363+
state.proc = Object.assign(processPromise, {
364+
continue: vitest.fn(() => state.resolveProcess()),
365+
abort: vitest.fn(),
366+
})
361367
;(TerminalRegistry.getOrCreateTerminal as ReturnType<typeof vitest.fn>).mockResolvedValue({
362368
runCommand: vitest.fn((_cmd: string, callbacks: RooTerminalCallbacks) => {
363369
state.callbacks = callbacks
@@ -480,16 +486,17 @@ describe("executeCommandTool", () => {
480486

481487
it("re-anchors a pending ask when execution start is reported after early output", async () => {
482488
vitest.useFakeTimers()
489+
mockCline.ask.mockResolvedValue({ response: "messageResponse", text: "keep going", images: undefined })
483490
const terminal = await setupControllableTerminal()
484491

485-
const handlePromise = handleCommand("echo hello")
492+
const handlePromise = handleCommand("sleep 60")
486493

487494
await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined())
488495
const callbacks = terminal.callbacks!
489496
const proc = terminal.proc as unknown as RooTerminalProcess
490497

491498
// Output arrives before the execution-started event (defensive case).
492-
await callbacks.onLine("hello\n", proc)
499+
await callbacks.onLine("working...\n", proc)
493500
await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS - 2_000)
494501
callbacks.onShellExecutionStarted!(1234, proc)
495502

@@ -498,19 +505,23 @@ describe("executeCommandTool", () => {
498505
await vitest.advanceTimersByTimeAsync(2_500)
499506
expect(mockCline.ask).not.toHaveBeenCalled()
500507

501-
await callbacks.onCompleted!("hello\n", proc)
508+
// The ask must still fire at the re-anchored deadline — a version
509+
// that cleared the old timer without rescheduling would fail here.
510+
await vitest.advanceTimersByTimeAsync(2_500)
511+
expect(mockCline.ask).toHaveBeenCalledWith("command_output", "")
512+
513+
// Let the command finish so the tool can resolve.
514+
await callbacks.onCompleted!("working...\n", proc)
502515
callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc)
503-
terminal.resolveProcess()
504-
await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS + 1_000)
516+
await vitest.advanceTimersByTimeAsync(100)
505517

506518
await handlePromise
507519

508-
expect(mockCline.ask).not.toHaveBeenCalled()
520+
expect(mockPushToolResult).toHaveBeenCalled()
509521
})
510522

511523
it("cancels a pending ask when the agent timeout moves the command to the background", async () => {
512524
vitest.useFakeTimers()
513-
mockCline.supersedePendingAsk = vitest.fn()
514525
const terminal = await setupControllableTerminal()
515526

516527
const handlePromise = handleCommand("npm run dev", 2)
@@ -594,7 +605,7 @@ describe("executeCommandTool", () => {
594605
expect(mockPushToolResult.mock.calls[0][0]).toContain("Exit code: 0")
595606
})
596607

597-
it("does not continue the process when the ask is answered without a message", async () => {
608+
it("resolves before completion when the ask is answered without a message", async () => {
598609
vitest.useFakeTimers()
599610
mockCline.ask.mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined })
600611
const terminal = await setupControllableTerminal()
@@ -609,17 +620,50 @@ describe("executeCommandTool", () => {
609620
await callbacks.onLine("working...\n", proc)
610621
await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS)
611622

623+
// Any ask answer backgrounds the command: the process is continued and
624+
// the tool resolves without waiting for the command to complete.
625+
// Note the process promise is never resolved in this test.
626+
await vitest.advanceTimersByTimeAsync(100)
627+
await handlePromise
628+
612629
expect(mockCline.ask).toHaveBeenCalledWith("command_output", "")
613-
expect(terminal.proc.continue).not.toHaveBeenCalled()
630+
expect(terminal.proc.continue).toHaveBeenCalled()
631+
expect(mockPushToolResult).toHaveBeenCalled()
632+
expect(mockPushToolResult.mock.calls[0][0]).toContain("still running")
633+
634+
// Cleanup: let the command finish.
635+
await callbacks.onCompleted!("working...\n", proc)
636+
callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc)
637+
})
638+
639+
it("supersedes a pending ask when the command completes", async () => {
640+
vitest.useFakeTimers()
641+
mockCline.ask.mockReturnValue(new Promise(() => {}))
642+
const terminal = await setupControllableTerminal()
643+
644+
const handlePromise = handleCommand("sleep 5")
645+
646+
await vitest.waitFor(() => expect(terminal.callbacks).toBeDefined())
647+
const callbacks = terminal.callbacks!
648+
const proc = terminal.proc as unknown as RooTerminalProcess
614649

650+
callbacks.onShellExecutionStarted!(1234, proc)
651+
await callbacks.onLine("working...\n", proc)
652+
await vitest.advanceTimersByTimeAsync(executeCommandModule.COMMAND_OUTPUT_ASK_DELAY_MS)
653+
654+
expect(mockCline.ask).toHaveBeenCalledWith("command_output", "")
655+
656+
// The command completes while the ask is still pending.
615657
await callbacks.onCompleted!("working...\n", proc)
616658
callbacks.onShellExecutionComplete!({ exitCode: 0 }, proc)
617659
terminal.resolveProcess()
618660
await vitest.advanceTimersByTimeAsync(100)
619661

620662
await handlePromise
621663

664+
expect(mockCline.supersedePendingAsk).toHaveBeenCalled()
622665
expect(mockPushToolResult).toHaveBeenCalled()
666+
expect(mockPushToolResult.mock.calls[0][0]).toContain("Exit code: 0")
623667
})
624668
})
625669
})

webview-ui/src/components/chat/ChatView.tsx

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,6 +462,17 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
462462
case "error":
463463
case "text":
464464
case "command_output":
465+
// A non-partial command_output say means the command
466+
// finished; clear any lingering Proceed/Kill controls
467+
// from the interactive ask so they don't stay up after
468+
// completion.
469+
if (lastMessage.partial !== true && clineAskRef.current === "command_output") {
470+
setClineAsk(undefined)
471+
setEnableButtons(false)
472+
setPrimaryButtonText(undefined)
473+
setSecondaryButtonText(undefined)
474+
}
475+
break
465476
case "mcp_server_request_started":
466477
case "mcp_server_response":
467478
case "completion_result":

webview-ui/src/components/chat/__tests__/ChatView.spec.tsx

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1134,6 +1134,69 @@ describe("ChatView - Message Queueing Tests", () => {
11341134
}),
11351135
)
11361136
})
1137+
1138+
it("clears the command_output controls when the final output arrives", async () => {
1139+
const { getByTestId, getByRole, queryByRole } = renderChatView()
1140+
1141+
// Hydrate state with a command_output ask (Proceed/Kill controls visible)
1142+
mockPostMessage({
1143+
clineMessages: [
1144+
{
1145+
type: "say",
1146+
say: "task",
1147+
ts: Date.now() - 2000,
1148+
text: "Initial task",
1149+
},
1150+
{
1151+
type: "ask",
1152+
ask: "command_output",
1153+
ts: Date.now() - 1000,
1154+
text: "",
1155+
partial: false,
1156+
},
1157+
],
1158+
})
1159+
1160+
await waitFor(() => {
1161+
expect(getByTestId("chat-textarea")).toBeInTheDocument()
1162+
})
1163+
1164+
await act(async () => {
1165+
await new Promise((resolve) => setTimeout(resolve, 50))
1166+
})
1167+
1168+
expect(getByRole("button", { name: "chat:proceedWhileRunning.title" })).toBeInTheDocument()
1169+
1170+
// The command completes: the final non-partial command_output say arrives.
1171+
mockPostMessage({
1172+
clineMessages: [
1173+
{
1174+
type: "say",
1175+
say: "task",
1176+
ts: Date.now() - 2000,
1177+
text: "Initial task",
1178+
},
1179+
{
1180+
type: "ask",
1181+
ask: "command_output",
1182+
ts: Date.now() - 1000,
1183+
text: "",
1184+
partial: false,
1185+
},
1186+
{
1187+
type: "say",
1188+
say: "command_output",
1189+
ts: Date.now(),
1190+
text: "done\n",
1191+
partial: false,
1192+
},
1193+
],
1194+
})
1195+
1196+
await waitFor(() => {
1197+
expect(queryByRole("button", { name: "chat:proceedWhileRunning.title" })).not.toBeInTheDocument()
1198+
})
1199+
})
11371200
})
11381201

11391202
describe("ChatView - Follow-up Suggestions", () => {

0 commit comments

Comments
 (0)