[EVAL] feat: Improve terminal reuse logic to prevent proliferation #17
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.
![roo-code-dan[bot]](https://avatars.githubusercontent.com/u/211522643?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.
The path quoting here is insufficient to prevent shell injection. If requestedCwd contains a double quote or other shell metacharacters (e.g., "/tmp/test"ing" or /tmp/test;rm -rf /), the command could be broken or exploited. Consider using a shell escaping function or switching to a safer approach like using the terminal's built-in cd command through shell integration APIs if available, or properly escaping the path using a library designed for shell escaping.
|
|
||
| vi.mock("../TerminalProcess") | ||
| vi.mock("../../../utils/path", () => ({ | ||
| arePathsEqual: vi.fn((a, b) => a === b), |
There was a problem hiding this comment.
The mock for arePathsEqual uses simple string equality ((a, b) => a === b), which doesn't match the actual implementation that normalizes paths and handles case-insensitivity on Windows. This could cause tests to pass even when the real implementation would fail. Consider using a more accurate mock that at least handles basic normalization, or test against the real arePathsEqual function to ensure the terminal reuse logic works correctly with actual path comparison behavior.
🧪 Evaluation PR for Testing
This is an automated test PR created for evaluation purposes.
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: