Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mrubens merged 1 commit into from
Apr 15, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 45 additions & 30 deletions src/core/Cline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -969,11 +969,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") {
Expand All @@ -982,37 +985,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
Expand All @@ -1026,6 +1022,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(
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug logging is quite detailed (using JSON.stringify with indentation) and logs several internal state fields. Consider gating this under a debug flag or proper logging level check to avoid performance overhead or inadvertent leakage of sensitive/internal information in production.

"[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 [
Expand Down
30 changes: 28 additions & 2 deletions src/integrations/terminal/Terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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())
Expand Down