From 710284cc3da1b61b22169b621457d6ae77029ec8 Mon Sep 17 00:00:00 2001 From: cte Date: Wed, 5 Mar 2025 21:28:42 -0800 Subject: [PATCH] Handle outputless commands --- .changeset/wise-bats-perform.md | 5 + src/core/Cline.ts | 6 + src/integrations/terminal/TerminalProcess.ts | 43 +++-- .../__tests__/TerminalProcess.test.ts | 157 +++++++++++++++++- 4 files changed, 198 insertions(+), 13 deletions(-) create mode 100644 .changeset/wise-bats-perform.md diff --git a/.changeset/wise-bats-perform.md b/.changeset/wise-bats-perform.md new file mode 100644 index 0000000000..fd6cb18a62 --- /dev/null +++ b/.changeset/wise-bats-perform.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Handle outputless commands diff --git a/src/core/Cline.ts b/src/core/Cline.ts index 38fc61f48a..0f1ffd16db 100644 --- a/src/core/Cline.ts +++ b/src/core/Cline.ts @@ -971,6 +971,12 @@ export class Cline { await this.say("shell_integration_warning") }) + process.once("stream_stalled", async (id: number) => { + if (id === terminalInfo.id && !didContinue) { + sendCommandOutput("") + } + }) + await process // Wait for a short delay to ensure all messages are sent to the webview. diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index 047465e3fe..b31185dcc5 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -41,12 +41,17 @@ export interface TerminalProcessEvents { error: [error: Error] no_shell_integration: [] /** - * Emitted when a shell execution completes + * Emitted when a shell execution completes. * @param id The terminal ID * @param exitDetails Contains exit code and signal information if process was terminated by signal */ shell_execution_complete: [id: number, exitDetails: ExitCodeDetails] stream_available: [id: number, stream: AsyncIterable] + /** + * Emitted when an execution fails to emit a "line" event for a given period of time. + * @param id The terminal ID + */ + stream_stalled: [id: number] } export class TerminalProcess extends EventEmitter { @@ -55,7 +60,7 @@ export class TerminalProcess extends EventEmitter { private isListening = true private terminalInfo: TerminalInfo | undefined - private lastEmitTime_ms = 0 + private lastEmitAt = 0 private outputBuilder?: OutputBuilder private hotTimer: NodeJS.Timeout | null = null @@ -67,14 +72,18 @@ export class TerminalProcess extends EventEmitter { this._isHot = value } - constructor(private readonly terminalOutputLimit: number) { + constructor( + private readonly terminalOutputLimit: number, + private readonly stallTimeout: number = 5_000, + ) { super() } async run(terminal: vscode.Terminal, command: string) { if (terminal.shellIntegration && terminal.shellIntegration.executeCommand) { - // Get terminal info to access stream + // Get terminal info to access stream. const terminalInfo = TerminalRegistry.getTerminalInfoByTerminal(terminal) + if (!terminalInfo) { console.error("[TerminalProcess] Terminal not found in registry") this.emit("no_shell_integration") @@ -127,11 +136,9 @@ export class TerminalProcess extends EventEmitter { this.outputBuilder = new OutputBuilder({ maxSize: this.terminalOutputLimit }) - /** - * Some commands won't result in output flushing until the command - * completes. This locks the UI. Should we set a timer to prompt - * the user to continue? - */ + let stallTimer: NodeJS.Timeout | null = setTimeout(() => { + this.emit("stream_stalled", terminalInfo.id) + }, this.stallTimeout) for await (let data of stream) { // Check for command output start marker. @@ -158,11 +165,17 @@ export class TerminalProcess extends EventEmitter { // right away but this wouldn't happen until it emits a line break, so // as soon as we get any output we emit to let webview know to show spinner. const now = Date.now() - const timeSinceLastEmit = now - this.lastEmitTime_ms + const timeSinceLastEmit = now - this.lastEmitAt if (this.isListening && timeSinceLastEmit > EMIT_INTERVAL) { - this.flushLine() - this.lastEmitTime_ms = now + if (this.flushLine()) { + if (stallTimer) { + clearTimeout(stallTimer) + stallTimer = null + } + + this.lastEmitAt = now + } } // Set isHot depending on the command. @@ -258,7 +271,10 @@ export class TerminalProcess extends EventEmitter { if (line) { this.emit("line", line) + return true } + + return false } private flushAll() { @@ -270,7 +286,10 @@ export class TerminalProcess extends EventEmitter { if (buffer) { this.emit("line", buffer) + return true } + + return false } private processOutput(outputToProcess: string) { diff --git a/src/integrations/terminal/__tests__/TerminalProcess.test.ts b/src/integrations/terminal/__tests__/TerminalProcess.test.ts index c90c517e3e..16cd85230d 100644 --- a/src/integrations/terminal/__tests__/TerminalProcess.test.ts +++ b/src/integrations/terminal/__tests__/TerminalProcess.test.ts @@ -20,6 +20,9 @@ jest.mock("vscode", () => ({ ThemeIcon: jest.fn(), })) +const TERMINAL_OUTPUT_LIMIT = 100 * 1024 +const STALL_TIMEOUT = 100 + describe("TerminalProcess", () => { let terminalProcess: TerminalProcess let mockTerminal: jest.Mocked< @@ -34,7 +37,7 @@ describe("TerminalProcess", () => { let mockStream: AsyncIterableIterator beforeEach(() => { - terminalProcess = new TerminalProcess(100 * 1024) + terminalProcess = new TerminalProcess(TERMINAL_OUTPUT_LIMIT, STALL_TIMEOUT) // Create properly typed mock terminal mockTerminal = { @@ -173,4 +176,156 @@ describe("TerminalProcess", () => { expect(terminalProcess["isListening"]).toBe(false) }) }) + + describe("stalled stream handling", () => { + it("emits stream_stalled event when no output is received within timeout", async () => { + // Create a promise that resolves when stream_stalled is emitted + const streamStalledPromise = new Promise((resolve) => { + terminalProcess.once("stream_stalled", (id: number) => { + resolve(id) + }) + }) + + // Create a stream that doesn't emit any data + mockStream = (async function* () { + yield "\x1b]633;C\x07" // Command start sequence + // No data is yielded after this, causing the stall + await new Promise((resolve) => setTimeout(resolve, STALL_TIMEOUT * 2)) + // This would normally be yielded, but the stall timer will fire first + yield "Output after stall" + yield "\x1b]633;D\x07" // Command end sequence + terminalProcess.emit("shell_execution_complete", mockTerminalInfo.id, { exitCode: 0 }) + })() + + mockExecution = { + read: jest.fn().mockReturnValue(mockStream), + } + + mockTerminal.shellIntegration.executeCommand.mockReturnValue(mockExecution) + + // Start the terminal process + const runPromise = terminalProcess.run(mockTerminal, "test command") + terminalProcess.emit("stream_available", mockTerminalInfo.id, mockStream) + + // Wait for the stream_stalled event + const stalledId = await streamStalledPromise + + // Verify the event was emitted with the correct terminal ID + expect(stalledId).toBe(mockTerminalInfo.id) + + // Complete the run + await runPromise + }) + + it("clears stall timer when output is received", async () => { + // Spy on the emit method to check if stream_stalled is emitted + const emitSpy = jest.spyOn(terminalProcess, "emit") + + // Create a stream that emits data before the stall timeout + mockStream = (async function* () { + yield "\x1b]633;C\x07" // Command start sequence + yield "Initial output\n" // This should clear the stall timer + + // Wait longer than the stall timeout + await new Promise((resolve) => setTimeout(resolve, STALL_TIMEOUT * 2)) + + yield "More output\n" + yield "\x1b]633;D\x07" // Command end sequence + terminalProcess.emit("shell_execution_complete", mockTerminalInfo.id, { exitCode: 0 }) + })() + + mockExecution = { + read: jest.fn().mockReturnValue(mockStream), + } + + mockTerminal.shellIntegration.executeCommand.mockReturnValue(mockExecution) + + // Start the terminal process + const runPromise = terminalProcess.run(mockTerminal, "test command") + terminalProcess.emit("stream_available", mockTerminalInfo.id, mockStream) + + // Wait for the run to complete + await runPromise + + // Wait a bit longer to ensure the stall timer would have fired if not cleared + await new Promise((resolve) => setTimeout(resolve, STALL_TIMEOUT * 2)) + + // Verify stream_stalled was not emitted + expect(emitSpy).not.toHaveBeenCalledWith("stream_stalled", expect.anything()) + }) + + it("returns true from flushLine when a line is emitted", async () => { + // Create a stream with output + mockStream = (async function* () { + yield "\x1b]633;C\x07" // Command start sequence + yield "Test output\n" // This should be flushed as a line + yield "\x1b]633;D\x07" // Command end sequence + terminalProcess.emit("shell_execution_complete", mockTerminalInfo.id, { exitCode: 0 }) + })() + + mockExecution = { + read: jest.fn().mockReturnValue(mockStream), + } + + mockTerminal.shellIntegration.executeCommand.mockReturnValue(mockExecution) + + // Spy on the flushLine method + const flushLineSpy = jest.spyOn(terminalProcess as any, "flushLine") + + // Spy on the emit method to check if line is emitted + const emitSpy = jest.spyOn(terminalProcess, "emit") + + // Start the terminal process + const runPromise = terminalProcess.run(mockTerminal, "test command") + terminalProcess.emit("stream_available", mockTerminalInfo.id, mockStream) + + // Wait for the run to complete + await runPromise + + // Verify flushLine was called and returned true + expect(flushLineSpy).toHaveBeenCalled() + expect(flushLineSpy.mock.results.some((result) => result.value === true)).toBe(true) + + // Verify line event was emitted + expect(emitSpy).toHaveBeenCalledWith("line", expect.any(String)) + }) + + it("returns false from flushLine when no line is emitted", async () => { + // Create a stream with no complete lines + mockStream = (async function* () { + yield "\x1b]633;C\x07" // Command start sequence + yield "Test output" // No newline, so this won't be flushed as a line yet + yield "\x1b]633;D\x07" // Command end sequence + terminalProcess.emit("shell_execution_complete", mockTerminalInfo.id, { exitCode: 0 }) + })() + + mockExecution = { + read: jest.fn().mockReturnValue(mockStream), + } + + mockTerminal.shellIntegration.executeCommand.mockReturnValue(mockExecution) + + // Create a custom implementation to test flushLine directly + const testFlushLine = async () => { + // Create a new instance with the same configuration + const testProcess = new TerminalProcess(TERMINAL_OUTPUT_LIMIT, STALL_TIMEOUT) + + // Set up the output builder with content that doesn't have a newline + testProcess["outputBuilder"] = { + readLine: jest.fn().mockReturnValue(""), + append: jest.fn(), + reset: jest.fn(), + content: "Test output", + } as any + + // Call flushLine directly + const result = testProcess["flushLine"]() + return result + } + + // Test flushLine directly + const flushLineResult = await testFlushLine() + expect(flushLineResult).toBe(false) + }) + }) })