feat: Improve terminal reuse logic to prevent proliferation

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.

Author: google-labs-jules[bot]

src/integrations/terminal/BaseTerminal.ts

@@ -20,6 +20,7 @@ export abstract class BaseTerminal implements RooTerminal {
 	protected streamClosed: boolean
 
 	public taskId?: string
+	public requestedCwd?: string
 	public process?: RooTerminalProcess
 	public completedProcesses: RooTerminalProcess[] = []
 

src/integrations/terminal/Terminal.ts

@@ -1,6 +1,7 @@
 import * as vscode from "vscode"
 import pWaitFor from "p-wait-for"
 
+import { arePathsEqual } from "../../utils/path"
 import type { RooTerminalCallbacks, RooTerminalProcessResultPromise } from "./types"
 import { BaseTerminal } from "./BaseTerminal"
 import { TerminalProcess } from "./TerminalProcess"
@@ -47,7 +48,14 @@ export class Terminal extends BaseTerminal {
 		this.busy = true
 
 		const process = new TerminalProcess(this)
-		process.command = command
+		const currentCwd = this.getCurrentWorkingDirectory()
+		let commandToRun = command
+
+		if (this.requestedCwd && !arePathsEqual(this.requestedCwd, currentCwd)) {
+			// Wrap path in quotes to handle spaces
+			commandToRun = `cd "${this.requestedCwd}" && ${command}`
+		}
+		process.command = commandToRun
 		this.process = process
 
 		// Set up event handlers from callbacks before starting process.
@@ -76,7 +84,7 @@ export class Terminal extends BaseTerminal {
 					ShellIntegrationManager.zshCleanupTmpDir(this.id)
 
 					// Run the command in the terminal
-					process.run(command)
+					process.run(commandToRun)
 				})
 				.catch(() => {
 					console.log(`[Terminal ${this.id}] Shell integration not available. Command execution aborted.`)

src/integrations/terminal/TerminalRegistry.ts

@@ -192,12 +192,22 @@ export class TerminalRegistry {
 			})
 		}
 
+		// Third priority: find any idle terminal with the same provider,
+		// preferably one with the same task ID.
+		if (!terminal) {
+			const idleTerminals = terminals.filter((t) => !t.busy && t.provider === provider)
+			if (idleTerminals.length > 0) {
+				terminal = idleTerminals.find((t) => t.taskId === taskId) ?? idleTerminals[0]
+			}
+		}
+
 		// If no suitable terminal found, create a new one.
 		if (!terminal) {
 			terminal = this.createTerminal(cwd, provider)
 		}
 
 		terminal.taskId = taskId
+		terminal.requestedCwd = cwd
 
 		return terminal
 	}

src/integrations/terminal/__tests__/Terminal.spec.ts

@@ -0,0 +1,103 @@
+// npx vitest run src/integrations/terminal/__tests__/Terminal.spec.ts
+
+import * as vscode from "vscode"
+import { Terminal } from "../Terminal"
+import { TerminalProcess } from "../TerminalProcess"
+import { vi } from "vitest"
+
+// Mock dependencies
+vi.mock("vscode", () => ({
+	window: {
+		createTerminal: vi.fn().mockReturnValue({
+			shellIntegration: {
+				cwd: { fsPath: "/initial/cwd" },
+			},
+			sendText: vi.fn(),
+			exitStatus: undefined,
+		}),
+	},
+	ThemeIcon: vi.fn(),
+}))
+
+vi.mock("p-wait-for", () => ({
+	default: vi.fn().mockResolvedValue(true),
+}))
+
+vi.mock("../TerminalProcess")
+vi.mock("../../../utils/path", () => ({
+	arePathsEqual: vi.fn((a, b) => a === b),
+}))
+
+describe("Terminal", () => {
+	describe("runCommand", () => {
+		let terminal: Terminal
+		let mockRun: any
+		let mockProcess: any
+
+		beforeEach(() => {
+			terminal = new Terminal(1, undefined, "/initial/cwd")
+			mockRun = vi.fn()
+			mockProcess = {
+				run: mockRun,
+				on: vi.fn(),
+				once: vi.fn(),
+				emit: vi.fn(),
+			}
+			vi.mocked(TerminalProcess).mockImplementation((): any => {
+				return mockProcess
+			})
+		})
+
+		afterEach(() => {
+			vi.clearAllMocks()
+		})
+
+		it("should prepend cd command if requestedCwd differs from currentCwd", () => {
+			terminal.requestedCwd = "/requested/cwd"
+			vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd")
+
+			const callbacks = {
+				onLine: vi.fn(),
+				onCompleted: vi.fn(),
+				onShellExecutionStarted: vi.fn(),
+				onShellExecutionComplete: vi.fn(),
+			}
+
+			terminal.runCommand("ls", callbacks)
+
+			expect(mockProcess.command).toBe('cd "/requested/cwd" && ls')
+			expect(mockRun).toHaveBeenCalledWith('cd "/requested/cwd" && ls')
+		})
+
+		it("should not prepend cd command if requestedCwd is the same as currentCwd", () => {
+			terminal.requestedCwd = "/initial/cwd"
+			vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd")
+			const callbacks = {
+				onLine: vi.fn(),
+				onCompleted: vi.fn(),
+				onShellExecutionStarted: vi.fn(),
+				onShellExecutionComplete: vi.fn(),
+			}
+
+			terminal.runCommand("ls", callbacks)
+
+			expect(mockProcess.command).toBe("ls")
+			expect(mockRun).toHaveBeenCalledWith("ls")
+		})
+
+		it("should not prepend cd command if requestedCwd is not set", () => {
+			vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd")
+			const callbacks = {
+				onLine: vi.fn(),
+				onCompleted: vi.fn(),
+				onShellExecutionStarted: vi.fn(),
+				onShellExecutionComplete: vi.fn(),
+			}
+
+			terminal.runCommand("ls", callbacks)
+
+			expect(mockProcess.command).toBe("ls")
+			expect(mockRun).toHaveBeenCalledWith("ls")
+		})
+	})
+})

src/integrations/terminal/__tests__/TerminalRegistry.spec.ts

@@ -3,9 +3,14 @@
 import * as vscode from "vscode"
 import { Terminal } from "../Terminal"
 import { TerminalRegistry } from "../TerminalRegistry"
+import { arePathsEqual } from "../../../utils/path"
 
 const PAGER = process.platform === "win32" ? "" : "cat"
 
+vi.mock("../../../utils/path", () => ({
+	arePathsEqual: vi.fn((a, b) => a === b),
+}))
+
 vi.mock("execa", () => ({
 	execa: vi.fn(),
 }))
@@ -119,4 +124,78 @@ describe("TerminalRegistry", () => {
 			}
 		})
 	})
+
+	describe("getOrCreateTerminal", () => {
+		let createTerminalSpy: any
+
+		beforeEach(() => {
+			// Reset terminals before each test
+			;(TerminalRegistry as any).terminals = []
+			createTerminalSpy = vi.spyOn(TerminalRegistry, "createTerminal")
+		})
+
+		afterEach(() => {
+			createTerminalSpy.mockRestore()
+		})
+
+		it("should create a new terminal if none exist", async () => {
+			await TerminalRegistry.getOrCreateTerminal("/test/path", "task1")
+			expect(createTerminalSpy).toHaveBeenCalledWith("/test/path", "vscode")
+		})
+
+		it("should reuse a terminal with the same task ID and cwd", async () => {
+			const existingTerminal = new Terminal(1, undefined, "/test/path")
+			existingTerminal.taskId = "task1"
+			;(TerminalRegistry as any).terminals.push(existingTerminal)
+
+			const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1")
+			expect(terminal).toBe(existingTerminal)
+			expect(createTerminalSpy).not.toHaveBeenCalled()
+		})
+
+		it("should reuse an idle terminal with the same cwd", async () => {
+			const existingTerminal = new Terminal(1, undefined, "/test/path")
+			;(TerminalRegistry as any).terminals.push(existingTerminal)
+
+			const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task2")
+			expect(terminal).toBe(existingTerminal)
+			expect(createTerminalSpy).not.toHaveBeenCalled()
+		})
+
+		it("should reuse an idle terminal with a different cwd if no better option is available", async () => {
+			const existingTerminal = new Terminal(1, undefined, "/other/path")
+			;(TerminalRegistry as any).terminals.push(existingTerminal)
+
+			const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1")
+			expect(terminal).toBe(existingTerminal)
+			expect(createTerminalSpy).not.toHaveBeenCalled()
+			expect(terminal.requestedCwd).toBe("/test/path")
+		})
+
+		it("should prioritize reusing a terminal with the same task ID when cwd has drifted", async () => {
+			const terminal1 = new Terminal(1, undefined, "/other/path1")
+			terminal1.taskId = "task1"
+			const terminal2 = new Terminal(2, undefined, "/other/path2")
+			terminal2.taskId = "task2"
+			;(TerminalRegistry as any).terminals.push(terminal1, terminal2)
+
+			const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1")
+			expect(terminal).toBe(terminal1)
+			expect(createTerminalSpy).not.toHaveBeenCalled()
+		})
+
+		it("should create a new terminal if all existing terminals are busy", async () => {
+			const existingTerminal = new Terminal(1, undefined, "/test/path")
+			existingTerminal.busy = true
+			;(TerminalRegistry as any).terminals.push(existingTerminal)
+
+			await TerminalRegistry.getOrCreateTerminal("/test/path", "task1")
+			expect(createTerminalSpy).toHaveBeenCalledWith("/test/path", "vscode")
+		})
+
+		it("should set the requestedCwd on the returned terminal", async () => {
+			const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1")
+			expect(terminal.requestedCwd).toBe("/test/path")
+		})
+	})
 })

src/integrations/terminal/types.ts

@@ -9,6 +9,7 @@ export interface RooTerminal {
 	running: boolean
 	taskId?: string
 	process?: RooTerminalProcess
+	requestedCwd?: string
 	getCurrentWorkingDirectory(): string
 	isClosed: () => boolean
 	runCommand: (command: string, callbacks: RooTerminalCallbacks) => RooTerminalProcessResultPromise