fix: race in short-running command output capture (RooCodeInc#2624)

Previously, handlers for terminal output were registered after starting the process,
which could cause output to be missed for fast-executing commands that complete
before handlers are set up.

- Adds CommandCallbacks interface to register handlers upfront
- Moves handler registration before process start to ensure no output is missed
- Provides process instance in callbacks for direct control
- Adds debug logging to help diagnose timing issues

Signed-off-by: Eric Wheeler <[email protected]>
Co-authored-by: Eric Wheeler <[email protected]>

Author: 2 people

src/core/Cline.ts

@@ -25,7 +25,7 @@ import { findToolName, formatContentBlockToMarkdown } from "../integrations/misc
 import { fetchInstructionsTool } from "./tools/fetchInstructionsTool"
 import { listFilesTool } from "./tools/listFilesTool"
 import { readFileTool } from "./tools/readFileTool"
-import { ExitCodeDetails } from "../integrations/terminal/TerminalProcess"
+import { ExitCodeDetails, TerminalProcess } from "../integrations/terminal/TerminalProcess"
 import { Terminal } from "../integrations/terminal/Terminal"
 import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry"
 import { UrlContentFetcher } from "../services/browser/UrlContentFetcher"
@@ -966,11 +966,14 @@ export class Cline extends EventEmitter<ClineEvents> {
 
 		const workingDirInfo = workingDir ? ` from '${workingDir.toPosix()}'` : ""
 		terminalInfo.terminal.show() // weird visual bug when creating new terminals (even manually) where there's an empty space at the top.
-		const process = terminalInfo.runCommand(command)
-
 		let userFeedback: { text?: string; images?: string[] } | undefined
 		let didContinue = false
-		const sendCommandOutput = async (line: string): Promise<void> => {
+		let completed = false
+		let result: string = ""
+		let exitDetails: ExitCodeDetails | undefined
+		const { terminalOutputLineLimit = 500 } = (await this.providerRef.deref()?.getState()) ?? {}
+
+		const sendCommandOutput = async (line: string, terminalProcess: TerminalProcess): Promise<void> => {
 			try {
 				const { response, text, images } = await this.ask("command_output", line)
 				if (response === "yesButtonClicked") {
@@ -979,37 +982,30 @@ export class Cline extends EventEmitter<ClineEvents> {
 					userFeedback = { text, images }
 				}
 				didContinue = true
-				process.continue() // continue past the await
+				terminalProcess.continue() // continue past the await
 			} catch {
 				// This can only happen if this ask promise was ignored, so ignore this error
 			}
 		}
 
-		const { terminalOutputLineLimit = 500 } = (await this.providerRef.deref()?.getState()) ?? {}
-
-		process.on("line", (line) => {
-			if (!didContinue) {
-				sendCommandOutput(Terminal.compressTerminalOutput(line, terminalOutputLineLimit))
-			} else {
-				this.say("command_output", Terminal.compressTerminalOutput(line, terminalOutputLineLimit))
-			}
-		})
-
-		let completed = false
-		let result: string = ""
-		let exitDetails: ExitCodeDetails | undefined
-		process.once("completed", (output?: string) => {
-			// Use provided output if available, otherwise keep existing result.
-			result = output ?? ""
-			completed = true
-		})
-
-		process.once("shell_execution_complete", (details: ExitCodeDetails) => {
-			exitDetails = details
-		})
-
-		process.once("no_shell_integration", async (message: string) => {
-			await this.say("shell_integration_warning", message)
+		const process = terminalInfo.runCommand(command, {
+			onLine: (line, process) => {
+				if (!didContinue) {
+					sendCommandOutput(Terminal.compressTerminalOutput(line, terminalOutputLineLimit), process)
+				} else {
+					this.say("command_output", Terminal.compressTerminalOutput(line, terminalOutputLineLimit))
+				}
+			},
+			onCompleted: (output) => {
+				result = output ?? ""
+				completed = true
+			},
+			onShellExecutionComplete: (details) => {
+				exitDetails = details
+			},
+			onNoShellIntegration: async (message) => {
+				await this.say("shell_integration_warning", message)
+			},
 		})
 
 		await process
@@ -1023,6 +1019,25 @@ export class Cline extends EventEmitter<ClineEvents> {
 
 		result = Terminal.compressTerminalOutput(result, terminalOutputLineLimit)
 
+		// keep in case we need it to troubleshoot user issues, but this should be removed in the future
+		// if everything looks good:
+		console.debug(
+			"[execute_command status]",
+			JSON.stringify(
+				{
+					completed,
+					userFeedback,
+					hasResult: result.length > 0,
+					exitDetails,
+					terminalId: terminalInfo.id,
+					workingDir: workingDirInfo,
+					isTerminalBusy: terminalInfo.busy,
+				},
+				null,
+				2,
+			),
+		)
+
 		if (userFeedback) {
 			await this.say("user_feedback", userFeedback.text, userFeedback.images)
 			return [

src/integrations/terminal/Terminal.ts

@@ -7,6 +7,13 @@ const { TerminalRegistry } = require("./TerminalRegistry")
 
 export const TERMINAL_SHELL_INTEGRATION_TIMEOUT = 5000
 
+export interface CommandCallbacks {
+	onLine?: (line: string, process: TerminalProcess) => void
+	onCompleted?: (output: string | undefined, process: TerminalProcess) => void
+	onShellExecutionComplete?: (details: ExitCodeDetails, process: TerminalProcess) => void
+	onNoShellIntegration?: (message: string, process: TerminalProcess) => void
+}
+
 export class Terminal {
 	private static shellIntegrationTimeout: number = TERMINAL_SHELL_INTEGRATION_TIMEOUT
 	private static commandDelay: number = 0
@@ -161,7 +168,7 @@ export class Terminal {
 		return output
 	}
 
-	public runCommand(command: string): TerminalProcessResultPromise {
+	public runCommand(command: string, callbacks?: CommandCallbacks): TerminalProcessResultPromise {
 		// We set busy before the command is running because the terminal may be waiting
 		// on terminal integration, and we must prevent another instance from selecting
 		// the terminal for use during that time.
@@ -176,7 +183,26 @@ export class Terminal {
 		// Set process on terminal
 		this.process = process
 
-		// Create a promise for command completion
+		// Set up event handlers from callbacks before starting process
+		// This ensures that we don't miss any events because they are
+		// configured before the process starts.
+		if (callbacks) {
+			if (callbacks.onLine) {
+				process.on("line", (line) => callbacks.onLine!(line, process))
+			}
+			if (callbacks.onCompleted) {
+				process.once("completed", (output) => callbacks.onCompleted!(output, process))
+			}
+			if (callbacks.onShellExecutionComplete) {
+				process.once("shell_execution_complete", (details) =>
+					callbacks.onShellExecutionComplete!(details, process),
+				)
+			}
+			if (callbacks.onNoShellIntegration) {
+				process.once("no_shell_integration", (msg) => callbacks.onNoShellIntegration!(msg, process))
+			}
+		}
+
 		const promise = new Promise<void>((resolve, reject) => {
 			// Set up event handlers
 			process.once("continue", () => resolve())