From bc8cfc919f1bf8b806c783395919369209494c45 Mon Sep 17 00:00:00 2001 From: Eric Wheeler Date: Wed, 5 Mar 2025 19:19:30 -0800 Subject: [PATCH] fix: prevent terminal sharing between Roo tasks This change improves terminal management by tracking which task owns each terminal and prioritizing terminal selection based on task ownership. Terminal selection now follows a priority order: 1. First try to find a terminal already assigned to this task with matching directory 2. If not found, try to find any available terminal with matching directory 3. If still not found, try to find any non-busy terminal 4. Only create a new terminal as a last resort When a task ends, all terminals associated with it are released for use by other tasks. This prevents the issue where multiple Roo task instances could inadvertently share terminals, which could lead to confusion when terminal output from one task appears in another task's context. Signed-off-by: Eric Wheeler --- src/core/Cline.ts | 13 ++---- src/integrations/terminal/Terminal.ts | 1 + src/integrations/terminal/TerminalRegistry.ts | 44 +++++++++++++++++-- 3 files changed, 45 insertions(+), 13 deletions(-) diff --git a/src/core/Cline.ts b/src/core/Cline.ts index 2a2ea88033..79faedb48a 100644 --- a/src/core/Cline.ts +++ b/src/core/Cline.ts @@ -27,7 +27,6 @@ import { everyLineHasLineNumbers, truncateOutput, } from "../integrations/misc/extract-text" -import { TerminalManager } from "../integrations/terminal/TerminalManager" import { ExitCodeDetails } from "../integrations/terminal/TerminalProcess" import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry" import { UrlContentFetcher } from "../services/browser/UrlContentFetcher" @@ -111,7 +110,6 @@ export class Cline { private rootTask: Cline | undefined = undefined readonly apiConfiguration: ApiConfiguration api: ApiHandler - private terminalManager: TerminalManager private urlContentFetcher: UrlContentFetcher private browserSession: BrowserSession private didEditFile: boolean = false @@ -182,7 +180,6 @@ export class Cline { this.taskNumber = -1 this.apiConfiguration = apiConfiguration this.api = buildApiHandler(apiConfiguration) - this.terminalManager = new TerminalManager() this.urlContentFetcher = new UrlContentFetcher(provider.context) this.browserSession = new BrowserSession(provider.context) this.customInstructions = customInstructions @@ -906,7 +903,9 @@ export class Cline { this.abort = true - this.terminalManager.disposeAll() + // Release any terminals associated with this task + TerminalRegistry.releaseTerminalsForTask(this.taskId) + this.urlContentFetcher.closeBrowser() this.browserSession.closeBrowser() this.rooIgnoreController?.dispose() @@ -921,7 +920,7 @@ export class Cline { // Tools async executeCommandTool(command: string): Promise<[boolean, ToolResponse]> { - const terminalInfo = await this.terminalManager.getOrCreateTerminal(cwd) + const terminalInfo = await TerminalRegistry.getOrCreateTerminal(cwd, this.taskId) terminalInfo.terminal.show() // weird visual bug when creating new terminals (even manually) where there's an empty space at the top. const process = terminalInfo.runCommand(command) @@ -3475,17 +3474,13 @@ export class Cline { const busyTerminals = TerminalRegistry.getTerminals(true) const inactiveTerminals = TerminalRegistry.getTerminals(false) - // const allTerminals = [...busyTerminals, ...inactiveTerminals] if (busyTerminals.length > 0 && this.didEditFile) { - // || this.didEditFile await delay(300) // delay after saving file to let terminals catch up } - // let terminalWasBusy = false if (busyTerminals.length > 0) { // wait for terminals to cool down - // terminalWasBusy = allTerminals.some((t) => this.terminalManager.isProcessHot(t.id)) await pWaitFor(() => busyTerminals.every((t) => !TerminalRegistry.isProcessHot(t.id)), { interval: 100, timeout: 15_000, diff --git a/src/integrations/terminal/Terminal.ts b/src/integrations/terminal/Terminal.ts index ac6ad99233..1132eb55cc 100644 --- a/src/integrations/terminal/Terminal.ts +++ b/src/integrations/terminal/Terminal.ts @@ -11,6 +11,7 @@ export class Terminal { public running: boolean private streamClosed: boolean public process?: TerminalProcess + public taskId?: string constructor(id: number, terminal: vscode.Terminal) { this.id = id diff --git a/src/integrations/terminal/TerminalRegistry.ts b/src/integrations/terminal/TerminalRegistry.ts index 3f0d2170f5..5e9123c45a 100644 --- a/src/integrations/terminal/TerminalRegistry.ts +++ b/src/integrations/terminal/TerminalRegistry.ts @@ -176,15 +176,47 @@ export class TerminalRegistry { this.disposables = [] } + /** + * Releases all terminals associated with a task + * @param taskId The task ID + */ + static releaseTerminalsForTask(taskId?: string): void { + if (!taskId) return + + this.terminals.forEach((terminal) => { + if (terminal.taskId === taskId) { + terminal.taskId = undefined + } + }) + } + /** * Gets an existing terminal or creates a new one for the given working directory * @param cwd The working directory path + * @param taskId Optional task ID to associate with the terminal * @returns A Terminal instance */ - static async getOrCreateTerminal(cwd: string): Promise { + static async getOrCreateTerminal(cwd: string, taskId?: string): Promise { const terminals = this.getAllTerminals() - // Find available terminal from our pool first (created for this task) + // First priority: Find a terminal already assigned to this task with matching directory + if (taskId) { + const taskTerminal = terminals.find((t) => { + if (t.busy || t.taskId !== taskId) { + return false + } + const terminalCwd = t.terminal.shellIntegration?.cwd + if (!terminalCwd) { + return false + } + return arePathsEqual(vscode.Uri.file(cwd).fsPath, terminalCwd.fsPath) + }) + if (taskTerminal) { + return taskTerminal + } + } + + // Second priority: Find any available terminal with matching directory const matchingTerminal = terminals.find((t) => { if (t.busy) { return false @@ -196,18 +228,22 @@ export class TerminalRegistry { return arePathsEqual(vscode.Uri.file(cwd).fsPath, terminalCwd.fsPath) }) if (matchingTerminal) { + matchingTerminal.taskId = taskId return matchingTerminal } - // If no matching terminal exists, try to find any non-busy terminal + // Third priority: Find any non-busy terminal const availableTerminal = terminals.find((t) => !t.busy) if (availableTerminal) { // Navigate back to the desired directory await availableTerminal.runCommand(`cd "${cwd}"`) + availableTerminal.taskId = taskId return availableTerminal } // If all terminals are busy, create a new one - return this.createTerminal(cwd) + const newTerminal = this.createTerminal(cwd) + newTerminal.taskId = taskId + return newTerminal } }