From 688da6d3a59ba13c76a5d298db2aa27da36013cd Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Mon, 14 Apr 2025 22:02:50 -0700 Subject: [PATCH] fix: race in short-running command output capture 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 --- src/core/Cline.ts | 75 ++++++++++++++++----------- src/integrations/terminal/Terminal.ts | 30 ++++++++++- 2 files changed, 73 insertions(+), 32 deletions(-) diff --git a/src/core/Cline.ts b/src/core/Cline.ts index ea5e231a18a..3e7e1810156 100644 --- a/src/core/Cline.ts +++ b/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" @@ -969,11 +969,14 @@ export class Cline extends EventEmitter { 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 => { + 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 => { try { const { response, text, images } = await this.ask("command_output", line) if (response === "yesButtonClicked") { @@ -982,37 +985,30 @@ export class Cline extends EventEmitter { 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 @@ -1026,6 +1022,25 @@ export class Cline extends EventEmitter { 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 [ diff --git a/src/integrations/terminal/Terminal.ts b/src/integrations/terminal/Terminal.ts index e17d01fa48f..2faa57cfffe 100644 --- a/src/integrations/terminal/Terminal.ts +++ b/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((resolve, reject) => { // Set up event handlers process.once("continue", () => resolve())