Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
173 changes: 173 additions & 0 deletions src/integrations/terminal/BaseTerminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
RooTerminalProcess,
RooTerminalProcessResultPromise,
ExitCodeDetails,
CompoundProcessCompletion,
} from "./types"

export abstract class BaseTerminal implements RooTerminal {
Expand All @@ -23,13 +24,21 @@ export abstract class BaseTerminal implements RooTerminal {
public process?: RooTerminalProcess
public completedProcesses: RooTerminalProcess[] = []

// Compound command tracking
public isCompoundCommand: boolean = false
public compoundProcessCompletions: CompoundProcessCompletion[] = []
public expectedCompoundProcessCount: number = 0
private compoundCommandWaitTimeout?: NodeJS.Timeout

constructor(provider: RooTerminalProvider, id: number, cwd: string) {
this.provider = provider
this.id = id
this.initialCwd = cwd
this.busy = false
this.running = false
this.streamClosed = false
this.isCompoundCommand = false
this.compoundProcessCompletions = []
}

public getCurrentWorkingDirectory(): string {
Expand Down Expand Up @@ -66,6 +75,170 @@ export abstract class BaseTerminal implements RooTerminal {
}
}

/**
* Detects if a command is a compound command (contains operators like &&, ||, ;)
* @param command The command to check
*/
public detectCompoundCommand(command: string): void {
// Reset previous compound command state
this.compoundProcessCompletions = []
this.expectedCompoundProcessCount = 0

// Clear any existing timeout
if (this.compoundCommandWaitTimeout) {
clearTimeout(this.compoundCommandWaitTimeout)
this.compoundCommandWaitTimeout = undefined
}

// Common shell operators that create compound commands
const compoundOperators = ["&&", "||", ";", "|", "&"]

// Check if command contains any compound operators
this.isCompoundCommand = compoundOperators.some((op) => command.includes(op))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compound command detection using simple string matching could incorrectly identify operators within quoted strings. For example, echo "test && test" would be incorrectly flagged as a compound command. Could we consider using a more robust parsing approach that respects shell quoting rules?


if (this.isCompoundCommand) {
// Estimate the number of processes (this is a heuristic, not exact)
// For && and ||, each operator adds one process
// For ;, each semicolon adds one process
// For |, each pipe adds one process
// For &, it's a background process indicator
let processCount = 1

// Count && and || operators
const andMatches = command.match(/&&/g)
const orMatches = command.match(/\|\|/g)
const semiMatches = command.match(/;/g)
const pipeMatches = command.match(/\|(?!\|)/g) // Match single | but not ||
// Match single & but not &&, and not preceded by &
const bgMatches = command.match(/(?<!&)&(?!&)/g)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the negative lookbehind (?<!&)&(?!&) supported in all JavaScript environments we target? Some older environments might not support lookbehind assertions. Would a more compatible regex pattern work here?


if (andMatches) processCount += andMatches.length
if (orMatches) processCount += orMatches.length
if (semiMatches) processCount += semiMatches.length
if (pipeMatches) processCount += pipeMatches.length
if (bgMatches) processCount += bgMatches.length

this.expectedCompoundProcessCount = processCount

console.info(
`[Terminal ${this.id}] Detected compound command with estimated ${processCount} processes:`,
command,
)

// Set a timeout to handle cases where we don't receive all expected completions
this.compoundCommandWaitTimeout = setTimeout(() => {
if (this.compoundProcessCompletions.length > 0) {
console.warn(
`[Terminal ${this.id}] Compound command timeout - processing ${this.compoundProcessCompletions.length} completions`,
)
this.finalizeCompoundCommand()
}
}, 10000) // 10 second timeout for compound commands
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 10-second timeout is hardcoded here. Could we extract this as a named constant like COMPOUND_COMMAND_TIMEOUT_MS or make it configurable? This would make it easier to adjust if needed and improve code readability.

}
}

/**
* Adds a compound process completion
* @param exitDetails The exit details of the completed process
* @param command The command that completed
*/
public addCompoundProcessCompletion(exitDetails: ExitCodeDetails, command: string): void {
if (!this.isCompoundCommand) {
console.warn(`[Terminal ${this.id}] Received compound process completion but not tracking compound command`)
return
}

this.compoundProcessCompletions.push({
exitDetails,
command,
timestamp: Date.now(),
})

console.info(
`[Terminal ${this.id}] Added compound process completion ${this.compoundProcessCompletions.length}/${this.expectedCompoundProcessCount}:`,
command,
)

// Check if all expected processes have completed
// Note: We check this after adding, so the finalization happens after the last process is added
if (this.allCompoundProcessesComplete()) {
console.info(`[Terminal ${this.id}] All compound processes complete, finalizing`)
this.finalizeCompoundCommand()
}
}

/**
* Checks if all compound processes have completed
* @returns True if all expected processes have completed
*/
public allCompoundProcessesComplete(): boolean {
// If we're not tracking a compound command, consider it complete
if (!this.isCompoundCommand) {
return true
}

// Check if we've received completions for all expected processes
// We use >= because sometimes we might get more completions than expected
const isComplete = this.compoundProcessCompletions.length >= this.expectedCompoundProcessCount

console.info(
`[Terminal ${this.id}] Checking compound completion: ${this.compoundProcessCompletions.length}/${this.expectedCompoundProcessCount} = ${isComplete}`,
)

return isComplete
}

/**
* Gets the combined output from all compound processes
* @returns The combined output string
*/
public getCompoundProcessOutputs(): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method only returns metadata about the commands and exit codes, but doesn't capture the actual command output. Since the original issue was about missing output from subsequent processes, should we also be collecting and returning the actual output from each process?

// Combine outputs from all completed processes
const outputs: string[] = []

for (const completion of this.compoundProcessCompletions) {
outputs.push(`[Command: ${completion.command}]`)
outputs.push(`[Exit Code: ${completion.exitDetails.exitCode}]`)
if (completion.exitDetails.signalName) {
outputs.push(`[Signal: ${completion.exitDetails.signalName}]`)
}
}

return outputs.join("\n")
}

/**
* Finalizes a compound command execution
*/
public finalizeCompoundCommand(): void {
// Clear the timeout if it exists
if (this.compoundCommandWaitTimeout) {
clearTimeout(this.compoundCommandWaitTimeout)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an error occurs during compound command execution, we might not reach this cleanup code. Should we wrap the compound command execution in a try-finally block to ensure the timeout is always cleared?

this.compoundCommandWaitTimeout = undefined
}

// Get the last exit details (from the final process in the chain)
const lastCompletion = this.compoundProcessCompletions[this.compoundProcessCompletions.length - 1]
const finalExitDetails = lastCompletion?.exitDetails || { exitCode: 0 }

console.info(
`[Terminal ${this.id}] Finalizing compound command with ${this.compoundProcessCompletions.length} processes`,
)

// Reset compound command tracking BEFORE calling shellExecutionComplete
// to prevent re-entrance issues
const wasCompound = this.isCompoundCommand
this.isCompoundCommand = false
this.compoundProcessCompletions = []
this.expectedCompoundProcessCount = 0

// Complete the terminal process with the final exit details
// Only if we were actually tracking a compound command
if (wasCompound) {
this.shellExecutionComplete(finalExitDetails)
}
}

/**
* Handles shell execution completion for this terminal.
* @param exitDetails The exit details of the shell execution
Expand Down
3 changes: 3 additions & 0 deletions src/integrations/terminal/ExecaTerminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ export class ExecaTerminal extends BaseTerminal {
public override runCommand(command: string, callbacks: RooTerminalCallbacks): RooTerminalProcessResultPromise {
this.busy = true

// Detect if this is a compound command before creating the process
this.detectCompoundCommand(command)

const process = new ExecaTerminalProcess(this)
process.command = command
this.process = process
Expand Down
3 changes: 3 additions & 0 deletions src/integrations/terminal/Terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ export class Terminal extends BaseTerminal {
// from selecting the terminal for use during that time.
this.busy = true

// Detect if this is a compound command before creating the process
this.detectCompoundCommand(command)

const process = new TerminalProcess(this)
process.command = command
this.process = process
Expand Down
57 changes: 51 additions & 6 deletions src/integrations/terminal/TerminalRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,57 @@ export class TerminalRegistry {
return
}

if (!terminal.running) {
console.error(
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
)
// For compound commands, we need to track if this is just one part of a multi-process command
if (terminal.isCompoundCommand) {
// Check if this is the last process before adding the completion
const wasLastProcess =
terminal.compoundProcessCompletions.length ===
(terminal.expectedCompoundProcessCount || 0) - 1

// Store this process completion
// This may trigger finalization if it's the last process
terminal.addCompoundProcessCompletion(exitDetails, e.execution?.commandLine?.value || "")

// If this was the last process, the compound command has been finalized
// and terminal.busy has been set to false by finalizeCompoundCommand
if (wasLastProcess) {
console.info("[TerminalRegistry] All compound command processes completed and finalized:", {
terminalId: terminal.id,
busy: terminal.busy,
})
// The terminal has been finalized, just return
return
} else {
console.info(
"[TerminalRegistry] Compound command process completed, waiting for remaining processes:",
{
terminalId: terminal.id,
command: e.execution?.commandLine?.value,
exitCode: e.exitCode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extensive console.info logging here and throughout the compound command handling might be too verbose for production. Could we use debug-level logging or make the verbosity configurable?

completedCount: terminal.compoundProcessCompletions.length,
},
)
// Still waiting for more processes
return
}
}

terminal.busy = false
if (!terminal.running) {
// For compound commands that spawn processes quickly, we might get the end event
// before the terminal is marked as running. Handle this gracefully.
if (terminal.process && terminal.isCompoundCommand) {
console.warn(
"[TerminalRegistry] Shell execution end event received before terminal marked as running (compound command scenario):",
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
)
// Already stored this completion above
} else {
console.error(
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
)
terminal.busy = false
}
return
}

Expand All @@ -114,6 +158,7 @@ export class TerminalRegistry {
}

// Signal completion to any waiting processes.
// For compound commands, this will use the finalized exit details from all processes
terminal.shellExecutionComplete(exitDetails)
terminal.busy = false // Mark terminal as not busy when shell execution ends
},
Expand Down
Loading
Loading