-
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
Conversation
- Add commandMaxWaitTime setting (default 30 seconds) to allow Roo to continue with other tasks when commands exceed timeout - Add autoSkippedCommands setting with common dev server patterns that should run in background - Update TerminalProcess to implement timeout logic and background command detection - Add UI components in Terminal Settings for both new settings - Add comprehensive tests for timeout and background command functionality - Update ClineProvider and executeCommandTool to pass settings through Fixes #8459
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.
Running diagnostics on my own code; if I disagree with me, at least I know who to blame.
| const lowerPattern = pattern.toLowerCase() | ||
| // Support wildcards in patterns | ||
| if (lowerPattern.includes("*")) { | ||
| const regex = new RegExp(lowerPattern.replace(/\*/g, ".*")) |
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.
[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:
| const regex = new RegExp(lowerPattern.replace(/\*/g, ".*")) | |
| const escaped = lowerPattern | |
| .split("*") | |
| .map((s) => s.replace(/[.+?^${}()|[\]\\]/g, "\\$&")) | |
| .join(".*") | |
| const regex = new RegExp(`^${escaped}$`) | |
| return regex.test(lowerCommand) |
| allowedCommands: ["*"], | ||
| commandExecutionTimeout: 20, | ||
| commandTimeoutAllowlist: [], | ||
| commandMaxWaitTime: 30, |
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:
| commandMaxWaitTime: 30, | |
| autoSkippedCommands: [ | |
| "npm run dev", | |
| "npm run start", | |
| "npm run watch", | |
| "yarn dev", | |
| "yarn start", | |
| "yarn watch", | |
| "pnpm dev", | |
| "pnpm start", | |
| "pnpm watch", | |
| "python -m http.server*", | |
| "python manage.py runserver*", | |
| "php -S *", | |
| "rails server", | |
| "dotnet watch*", | |
| "docker compose up", | |
| "docker-compose up" | |
| ], |
| } | ||
|
|
||
| public override runCommand(command: string, callbacks: RooTerminalCallbacks): RooTerminalProcessResultPromise { | ||
| public override runCommand( |
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.
[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.
This PR addresses Issue #8459 by implementing command timeout and auto-skipped commands functionality.
Summary
Adds two new terminal settings to help Roo handle long-running commands more efficiently:
Changes
Core Implementation
commandMaxWaitTimeandautoSkippedCommandsto global settings schemaTerminalProcessto implement timeout logic and background command detectionUI Components
Testing
Default Configuration
The feature ships with sensible defaults:
npm run devnpm run startyarn devyarn startpnpm devpnpm startnpm run watchyarn watchpnpm watchpython -m http.server*python manage.py runserver*php -S *rails serverdotnet watch*docker compose updocker-compose upBenefits
Fixes #8459
Important
Adds command timeout and auto-skipped commands settings to improve terminal command handling, with UI integration and comprehensive testing.
commandMaxWaitTimeandautoSkippedCommandsto global settings schema inglobal-settings.ts.TerminalProcess.executeCommandToolto pass new settings from provider state.commandMaxWaitTimeand list editor forautoSkippedCommandsinTerminalSettings.tsx.settings.jsonfor new terminal settings descriptions.TerminalProcessTimeout.spec.tsto test timeout and background command functionality.ClineProvider.tsto handle new settings in state management.Terminal.tsandExecaTerminal.tsto support new command execution parameters.This description was created by
for a3ccac0. You can customize this summary. It will automatically update as commits are pushed.