feat: Improve terminal reuse logic to prevent proliferation #8525
Conversation
This change improves the terminal reuse logic to prevent the creation of unnecessary terminals. The previous implementation was too strict and would create new terminals even when idle ones were available. The new logic introduces a fallback mechanism to reuse idle terminals even if their current working directory has drifted. When a terminal with a drifted CWD is reused, a `cd` command is prepended to the command being executed to ensure it runs in the correct directory. This change also includes: - Updates to the `RooTerminal` interface and `BaseTerminal` class to support the new logic. - Comprehensive unit tests to validate the new terminal reuse behavior.
dosubot
bot
added
size:L
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
|
||
| if (this.requestedCwd && !arePathsEqual(this.requestedCwd, currentCwd)) { | ||
| // Wrap path in quotes to handle spaces | ||
| commandToRun = `cd "${this.requestedCwd}" && ${command}` |
There was a problem hiding this comment.
When constructing the command (e.g., cd "${this.requestedCwd}" && ...), consider sanitizing 'requestedCwd' further to escape embedded quotes or other special characters.
| commandToRun = `cd "${this.requestedCwd}" && ${command}` | |
| commandToRun = `cd "${this.requestedCwd.replace(/\"/g, '\\"')}" && ${command}` |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
|
|
||
| if (this.requestedCwd && !arePathsEqual(this.requestedCwd, currentCwd)) { | ||
| // Wrap path in quotes to handle spaces | ||
| commandToRun = `cd "${this.requestedCwd}" && ${command}` |
There was a problem hiding this comment.
[P1] Path escaping and shell portability: requestedCwd is interpolated into a shell command without escaping. If it contains a double quote, this will break command parsing. Also, using '&&' assumes a POSIX-like shell; Windows PowerShell 5 doesn't support '&&'. Escape quotes and consider shell-aware composition.
| commandToRun = `cd "${this.requestedCwd}" && ${command}` | |
| commandToRun = `cd "${this.requestedCwd.replace(/"/g, '\\"')}" && ${command}` |
| } | ||
|
|
||
| terminal.taskId = taskId | ||
| terminal.requestedCwd = cwd |
There was a problem hiding this comment.
[P2] Normalize CWD for consistency: requestedCwd is assigned directly from input. Other comparisons normalize to fsPath; assign the normalized path here to avoid mismatches across platforms.
| terminal.requestedCwd = cwd | |
| terminal.requestedCwd = vscode.Uri.file(cwd).fsPath |
hannesrudolph
added
the
Issue/PR - Triage
2 participants
This change improves the terminal reuse logic to prevent the creation of unnecessary terminals. The previous implementation was too strict and would create new terminals even when idle ones were available.
The new logic introduces a fallback mechanism to reuse idle terminals even if their current working directory has drifted. When a terminal with a drifted CWD is reused, a
cdcommand is prepended to the command being executed to ensure it runs in the correct directory.This change also includes:
RooTerminalinterface andBaseTerminalclass to support the new logic.Related GitHub Issue
Closes: #
Roo Code Task Context (Optional)
Description
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Enhance terminal reuse logic to allow reusing idle terminals with drifted CWD by prepending a
cdcommand.cdcommand inTerminal.TerminalRegistryto prioritize reusing terminals with the same task ID and provider.requestedCwdtoRooTerminalinterface andBaseTerminalclass.runCommand()inTerminalto handle CWD drift.Terminal.spec.tsandTerminalRegistry.spec.tsto validate terminal reuse logic.This description was created by
for 214b606. You can customize this summary. It will automatically update as commits are pushed.