-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add command timeout and auto-skipped commands settings #8461
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 |
|---|---|---|
|
|
@@ -15,7 +15,12 @@ export class ExecaTerminal extends BaseTerminal { | |
| return false | ||
| } | ||
|
|
||
| public override runCommand(command: string, callbacks: RooTerminalCallbacks): RooTerminalProcessResultPromise { | ||
| public override runCommand( | ||
|
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. [P3] Provider parity: the new optional params (commandMaxWaitTime, autoSkippedCommands) are accepted in the signature but not forwarded. Execa fallback will not mirror background/timeout behavior, which can surprise users when shell integration is disabled. Consider forwarding the options (even if ExecaTerminalProcess is a no-op today) to keep contracts aligned and ease future implementation. |
||
| command: string, | ||
| callbacks: RooTerminalCallbacks, | ||
| commandMaxWaitTime?: number, | ||
| autoSkippedCommands?: string[], | ||
| ): RooTerminalProcessResultPromise { | ||
| this.busy = true | ||
|
|
||
| const process = new ExecaTerminalProcess(this) | ||
|
|
@@ -30,6 +35,7 @@ export class ExecaTerminal extends BaseTerminal { | |
| const promise = new Promise<void>((resolve, reject) => { | ||
| process.once("continue", () => resolve()) | ||
| process.once("error", (error) => reject(error)) | ||
| // Note: ExecaTerminalProcess doesn't support timeout yet, but we maintain the interface | ||
| process.run(command) | ||
| }) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,10 @@ import { Terminal } from "./Terminal" | |||||||||||||||
|
|
||||||||||||||||
| export class TerminalProcess extends BaseTerminalProcess { | ||||||||||||||||
| private terminalRef: WeakRef<Terminal> | ||||||||||||||||
| private commandTimeout?: NodeJS.Timeout | ||||||||||||||||
| private commandMaxWaitTime: number = 30000 // Default 30 seconds | ||||||||||||||||
| private autoSkippedCommands: string[] = [] | ||||||||||||||||
| private isBackgroundCommand: boolean = false | ||||||||||||||||
|
|
||||||||||||||||
| constructor(terminal: Terminal) { | ||||||||||||||||
| super() | ||||||||||||||||
|
|
@@ -44,9 +48,20 @@ export class TerminalProcess extends BaseTerminalProcess { | |||||||||||||||
| return terminal | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| public override async run(command: string) { | ||||||||||||||||
| public override async run(command: string, commandMaxWaitTime?: number, autoSkippedCommands?: string[]) { | ||||||||||||||||
| this.command = command | ||||||||||||||||
|
|
||||||||||||||||
| // Update settings if provided | ||||||||||||||||
| if (commandMaxWaitTime !== undefined) { | ||||||||||||||||
| this.commandMaxWaitTime = commandMaxWaitTime * 1000 // Convert to milliseconds | ||||||||||||||||
| } | ||||||||||||||||
| if (autoSkippedCommands !== undefined) { | ||||||||||||||||
| this.autoSkippedCommands = autoSkippedCommands | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Check if this command should run in background | ||||||||||||||||
| this.isBackgroundCommand = this.shouldRunInBackground(command) | ||||||||||||||||
|
|
||||||||||||||||
| const terminal = this.terminal.terminal | ||||||||||||||||
|
|
||||||||||||||||
| const isShellIntegrationAvailable = terminal.shellIntegration && terminal.shellIntegration.executeCommand | ||||||||||||||||
|
|
@@ -134,6 +149,30 @@ export class TerminalProcess extends BaseTerminalProcess { | |||||||||||||||
|
|
||||||||||||||||
| this.isHot = true | ||||||||||||||||
|
|
||||||||||||||||
| // Set up timeout for long-running commands if not a background command | ||||||||||||||||
| if (!this.isBackgroundCommand && this.commandMaxWaitTime > 0) { | ||||||||||||||||
| this.commandTimeout = setTimeout(() => { | ||||||||||||||||
| console.log( | ||||||||||||||||
| `[TerminalProcess] Command timeout reached after ${this.commandMaxWaitTime / 1000} seconds for: ${command}`, | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| // Emit event to allow Roo to continue with other tasks | ||||||||||||||||
| this.emit("command_timeout", command) | ||||||||||||||||
|
|
||||||||||||||||
| // Don't abort the command, just allow Roo to continue | ||||||||||||||||
| // The command will continue running in the background | ||||||||||||||||
| this.isBackgroundCommand = true | ||||||||||||||||
| }, this.commandMaxWaitTime) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // If it's a background command, emit immediately to allow Roo to continue | ||||||||||||||||
| if (this.isBackgroundCommand) { | ||||||||||||||||
| console.log(`[TerminalProcess] Running command in background: ${command}`) | ||||||||||||||||
| setTimeout(() => { | ||||||||||||||||
| this.emit("background_command", command) | ||||||||||||||||
| }, 100) // Small delay to ensure command starts | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Wait for stream to be available | ||||||||||||||||
| let stream: AsyncIterable<string> | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -208,6 +247,12 @@ export class TerminalProcess extends BaseTerminalProcess { | |||||||||||||||
| // Set streamClosed immediately after stream ends. | ||||||||||||||||
| this.terminal.setActiveStream(undefined) | ||||||||||||||||
|
|
||||||||||||||||
| // Clear timeout if command completed before timeout | ||||||||||||||||
| if (this.commandTimeout) { | ||||||||||||||||
| clearTimeout(this.commandTimeout) | ||||||||||||||||
| this.commandTimeout = undefined | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Wait for shell execution to complete. | ||||||||||||||||
| await shellExecutionComplete | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -464,4 +509,24 @@ export class TerminalProcess extends BaseTerminalProcess { | |||||||||||||||
|
|
||||||||||||||||
| return match133 !== undefined ? match133 : match633 | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * Check if a command should run in the background based on patterns | ||||||||||||||||
| */ | ||||||||||||||||
| private shouldRunInBackground(command: string): boolean { | ||||||||||||||||
| if (!this.autoSkippedCommands || this.autoSkippedCommands.length === 0) { | ||||||||||||||||
| return false | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const lowerCommand = command.toLowerCase() | ||||||||||||||||
| return this.autoSkippedCommands.some((pattern) => { | ||||||||||||||||
| const lowerPattern = pattern.toLowerCase() | ||||||||||||||||
| // Support wildcards in patterns | ||||||||||||||||
| if (lowerPattern.includes("*")) { | ||||||||||||||||
| const regex = new RegExp(lowerPattern.replace(/\*/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. [P1] Wildcard pattern matching builds an unescaped RegExp and isn't anchored, so characters like '.' or '?' in user patterns change semantics (e.g., 'python -m http.server*' treats '.' as any char) and partial matches can over-trigger. Consider escaping regex metacharacters and anchoring the pattern. Suggested fix:
Suggested change
|
||||||||||||||||
| return regex.test(lowerCommand) | ||||||||||||||||
| } | ||||||||||||||||
| return lowerCommand.includes(lowerPattern) | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
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.
[P2] Default autoSkippedCommands don't align with the PR description and common long-running commands (missing yarn/pnpm watch, npm run start, wildcarded servers). Propose expanding defaults for better out-of-the-box behavior: