-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle compound commands in terminal integration #7432
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import type { | |
| RooTerminalProcess, | ||
| RooTerminalProcessResultPromise, | ||
| ExitCodeDetails, | ||
| CompoundProcessCompletion, | ||
| } from "./types" | ||
|
|
||
| export abstract class BaseTerminal implements RooTerminal { | ||
|
|
@@ -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 { | ||
|
|
@@ -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)) | ||
|
|
||
| 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) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the negative lookbehind |
||
|
|
||
| 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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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 { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| } | ||
|
|
||
|
|
@@ -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 | ||
| }, | ||
|
|
||
There was a problem hiding this comment.
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?