Skip to content

Conversation

roomote-agent
Copy link
Collaborator

@roomote-agent roomote-agent commented Jul 5, 2025

Description

Fixes #5424

This PR resolves the issue where subsequent execute_command calls would run in the wrong directory after the LLM used commands like cd

. The problem occurred because the terminal registry was reusing terminals whose working directory had changed via cd commands, without properly tracking these changes.

Changes Made

  • Enhanced TerminalRegistry.getOrCreateTerminal() to properly track working directory changes using VSCode's terminal.shellIntegration.cwd
  • Added strict working directory matching in all three priority levels of terminal selection
  • Implemented task isolation to prevent terminals from different tasks being inappropriately reused
  • Added comprehensive test coverage with 5 new tests

Testing

  • All existing tests pass (47 terminal integration tests)
  • New tests pass (5 working directory-specific tests)
  • No linting or TypeScript errors
  • Verified fix resolves the reported issue

Files Changed

  • src/integrations/terminal/TerminalRegistry.ts - Enhanced terminal selection logic
  • src/integrations/terminal/tests/TerminalRegistry.workingDirectory.spec.ts - Test coverage

Important

Fixes terminal working directory tracking in TerminalRegistry.ts to ensure correct execution after cd commands, with new tests added.

  • Behavior:
    • Fixes terminal working directory tracking in TerminalRegistry.ts to ensure commands run in correct directory after cd.
    • Implements task isolation to prevent terminal reuse across different tasks.
  • Testing:
    • Adds 5 new tests in TerminalRegistry.workingDirectory.spec.ts to cover working directory changes and task-specific terminal reuse.
    • All existing and new tests pass, ensuring no regressions.
  • Misc:
    • Enhances getOrCreateTerminal() to track working directory using terminal.shellIntegration.cwd and match strictly by directory.

This description was created by Ellipsis for 156698f. You can customize this summary. It will automatically update as commits are pushed.

…5424)

- Enhanced TerminalRegistry.getOrCreateTerminal() to properly track working directory changes
- Added strict working directory matching in all terminal selection priorities
- Implemented task isolation to prevent inappropriate terminal reuse
- Added comprehensive test coverage for working directory scenarios
- Ensures subsequent commands run in correct directory after cd operations

Fixes #5424
@roomote-agent roomote-agent requested review from mrubens, cte and jr as code owners July 5, 2025 21:40
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 5, 2025
@dosubot dosubot bot added the bug Something isn't working label Jul 5, 2025
Copy link

delve-auditor bot commented Jul 5, 2025

No security or compliance issues detected. Reviewed everything up to 156698f.

Security Overview
  • 🔎 Scanned files: 2 changed file(s)
Detected Code Changes
Change Type Relevant files
Bug Fix ► TerminalRegistry.ts
    Improve working directory tracking in terminal selection
    Enhanced terminal reuse logic with task isolation
► TerminalRegistry.workingDirectory.spec.ts
    Add comprehensive test coverage for working directory scenarios

Reply to this PR with @delve-auditor followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

if (!terminal) {
terminal = terminals.find((t) => {
if (t.busy || t.provider !== provider) {
return false
}

// Don't reuse terminals assigned to different tasks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added check to avoid reusing terminals assigned to a different task is a good fix. Note that this same taskId check is repeated in both the second and third priority branches. Consider refactoring this logic into a helper function for clarity and maintainability.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jul 5, 2025
@hannesrudolph hannesrudolph moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jul 7, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jul 7, 2025
@daniel-lxs
Copy link
Collaborator

I feel like we can probably solve this with better instructions or maybe letting the model know the current working directory of the terminal? With this change Roo Code will potentially create new terminal instances every time it uses a command with cd.

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to Triage in Roo Code Roadmap Jul 9, 2025
@hannesrudolph hannesrudolph added Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. and removed PR - Needs Preliminary Review labels Jul 9, 2025
@daniel-lxs daniel-lxs moved this from Triage to renovate BOT in Roo Code Roadmap Jul 15, 2025
@daniel-lxs
Copy link
Collaborator

Automatically closed: PR created by @roomote-agent before 2025-07-16

@daniel-lxs daniel-lxs closed this Jul 18, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jul 18, 2025
@github-project-automation github-project-automation bot moved this from Roomote/renovate BOT to Done in Roo Code Roadmap Jul 18, 2025
Copy link

roomote bot commented Jul 18, 2025

I understand the concern about potential terminal proliferation. Let me review the current implementation and explore a better solution that addresses the working directory tracking issue without creating excessive terminal instances. I will investigate alternative approaches such as better instructions or providing current working directory information to the model.

Copy link

roomote bot commented Jul 18, 2025

Summary of Changes

I have addressed your concern about terminal proliferation while maintaining the core functionality to track working directory changes. Here is what I implemented:

Key Improvements:

  1. Smarter Terminal Reuse: Modified TerminalRegistry.getOrCreateTerminal() to prioritize reusing task-specific terminals regardless of current working directory when requiredCwd=false (the default behavior).

  2. Selective Directory Enforcement: Only create new terminals when requiredCwd=true AND the current working directory does not match the requested directory. This prevents excessive terminal creation while maintaining precision when needed.

  3. Enhanced User Feedback: Updated executeCommandTool to inform the model when the working directory has changed after command execution, providing better context without requiring new terminals.

  4. Updated Test Coverage: Revised tests to reflect the new behavior that prevents terminal proliferation while maintaining directory tracking capabilities.

How This Solves the Original Issue:

  • Before: Terminals would be reused even when their working directory had changed via cd commands, causing subsequent commands to run in wrong directories.
  • After: The model receives clear feedback about working directory changes, and terminals are only created when absolutely necessary.

Benefits:

Prevents Terminal Proliferation: Task-specific terminals are reused by default
Maintains Directory Awareness: Model gets informed about directory changes
Preserves Precision: Can still enforce specific directories when needed
All Tests Pass: Integration tests, security checks, and compilation all successful

The solution addresses the core issue from #5424 while implementing your feedback about avoiding excessive terminal creation. The model now receives proper working directory information to make informed decisions about subsequent commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Subsequent commands run in wrong directory if llm uses cd <dir>
3 participants