refactor: establish natural terminal hierarchy

Move terminal state access from TerminalManager to TerminalRegistry to establish
a clear hierarchical relationship between components. This change centralizes
terminal management in TerminalRegistry and eliminates duplicate state tracking
in TerminalManager.

The hierarchy flows from TerminalRegistry (managing all terminals) to Terminal
(encapsulating a terminal instance) to TerminalProcess (running within a terminal).

Key changes:
- Remove `processes` map from TerminalManager
- Add static getUnretrievedOutput and isProcessHot methods to TerminalRegistry, which manages all terminals globally

Test updates:
- Modify test setup to create Terminal instances
- Remove processes map usage from tests
- Update process creation and command execution flow in tests

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>
This commit is contained in:
Eric Wheeler 2025-03-05 16:40:08 -08:00
parent 304fcf2fbc
commit 59745a7058
8 changed files with 216 additions and 152 deletions

View file

@ -29,6 +29,7 @@ import {
} 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"
import { listFiles } from "../services/glob/list-files"
import { regexSearchFiles } from "../services/ripgrep"
@ -3472,8 +3473,8 @@ export class Cline {
details += "\n(No open tabs)"
}
const busyTerminals = this.terminalManager.getTerminals(true)
const inactiveTerminals = this.terminalManager.getTerminals(false)
const busyTerminals = TerminalRegistry.getTerminals(true)
const inactiveTerminals = TerminalRegistry.getTerminals(false)
// const allTerminals = [...busyTerminals, ...inactiveTerminals]
if (busyTerminals.length > 0 && this.didEditFile) {
@ -3485,7 +3486,7 @@ export class Cline {
if (busyTerminals.length > 0) {
// wait for terminals to cool down
// terminalWasBusy = allTerminals.some((t) => this.terminalManager.isProcessHot(t.id))
await pWaitFor(() => busyTerminals.every((t) => !this.terminalManager.isProcessHot(t.id)), {
await pWaitFor(() => busyTerminals.every((t) => !TerminalRegistry.isProcessHot(t.id)), {
interval: 100,
timeout: 15_000,
}).catch(() => {})
@ -3517,7 +3518,7 @@ export class Cline {
terminalDetails += "\n\n# Actively Running Terminals"
for (const busyTerminal of busyTerminals) {
terminalDetails += `\n## Original command: \`${busyTerminal.lastCommand}\``
const newOutput = this.terminalManager.getUnretrievedOutput(busyTerminal.id)
const newOutput = TerminalRegistry.getUnretrievedOutput(busyTerminal.id)
if (newOutput) {
terminalDetails += `\n### New Output\n${newOutput}`
} else {
@ -3529,7 +3530,7 @@ export class Cline {
if (inactiveTerminals.length > 0) {
const inactiveTerminalOutputs = new Map<number, string>()
for (const inactiveTerminal of inactiveTerminals) {
const newOutput = this.terminalManager.getUnretrievedOutput(inactiveTerminal.id)
const newOutput = TerminalRegistry.getUnretrievedOutput(inactiveTerminal.id)
if (newOutput) {
inactiveTerminalOutputs.set(inactiveTerminal.id, newOutput)
}

View file

@ -1,13 +1,15 @@
import * as vscode from "vscode"
import { ExitCodeDetails, TerminalProcess } from "./TerminalProcess"
export class Terminal {
public terminal: vscode.Terminal
public busy: boolean
public lastCommand: string
public id: number
public stream?: AsyncIterable<string>
private stream?: AsyncIterable<string>
public running: boolean
public streamClosed: boolean
private streamClosed: boolean
public process?: TerminalProcess
constructor(id: number, terminal: vscode.Terminal) {
this.id = id
@ -18,6 +20,57 @@ export class Terminal {
this.streamClosed = false
}
/**
* Gets the terminal's stream
*/
public getStream(): AsyncIterable<string> | undefined {
return this.stream
}
/**
* Checks if the stream is closed
*/
public isStreamClosed(): boolean {
return this.streamClosed
}
/**
* Sets the active stream for this terminal and notifies the process
* @param stream The stream to set, or undefined to clean up
* @throws Error if process is undefined when a stream is provided
*/
public setActiveStream(stream: AsyncIterable<string> | undefined): void {
this.stream = stream
if (stream) {
// New stream is available
if (!this.process) {
throw new Error(`Cannot set active stream on terminal ${this.id} because process is undefined`)
}
this.streamClosed = false
this.running = true
this.process.emit("stream_available", this.id, stream)
} else {
// Stream is being closed
this.streamClosed = true
this.running = false
}
}
/**
* Handles shell execution completion for this terminal
* @param exitDetails The exit details of the shell execution
*/
public shellExecutionComplete(exitDetails: ExitCodeDetails): void {
this.running = false
if (this.process) {
this.process.emit("shell_execution_complete", this.id, exitDetails)
this.process = undefined
}
}
/**
* Gets the terminal contents based on the number of commands to include
* @param commands Number of previous commands to include (-1 for all)

View file

@ -97,7 +97,6 @@ declare module "vscode" {
export class TerminalManager {
private terminalIds: Set<number> = new Set()
private processes: Map<number, TerminalProcess> = new Map()
private disposables: vscode.Disposable[] = []
constructor() {
@ -108,15 +107,9 @@ export class TerminalManager {
startDisposable = (vscode.window as vscode.Window).onDidStartTerminalShellExecution?.(async (e) => {
// Get a handle to the stream as early as possible:
const stream = e?.execution.read()
const terminalInfo = TerminalRegistry.getTerminalInfoByTerminal(e.terminal)
if (stream && terminalInfo) {
const process = this.processes.get(terminalInfo.id)
if (process) {
terminalInfo.stream = stream
terminalInfo.running = true
terminalInfo.streamClosed = false
process.emit("stream_available", terminalInfo.id, stream)
}
const terminalInfo = TerminalRegistry.getTerminalByVSCETerminal(e.terminal)
if (terminalInfo) {
terminalInfo.setActiveStream(stream)
} else {
console.error("[TerminalManager] Stream failed, not registered for terminal")
}
@ -130,25 +123,16 @@ export class TerminalManager {
// onDidEndTerminalShellExecution
endDisposable = (vscode.window as vscode.Window).onDidEndTerminalShellExecution?.(async (e) => {
// Find the terminal ID by the VSCode terminal instance
const terminalId = this.findTerminalIdByVscodeTerminal(e.terminal)
const process = terminalId !== undefined ? this.processes.get(terminalId) : undefined
const terminalInfo = TerminalRegistry.getTerminalByVSCETerminal(e.terminal)
const process = terminalInfo?.process
const exitDetails = process ? process.interpretExitCode(e?.exitCode) : { exitCode: e?.exitCode }
console.info("[TerminalManager] Shell execution ended:", {
...exitDetails,
})
// Signal completion to any waiting processes
for (const id of this.terminalIds) {
const info = TerminalRegistry.getTerminal(id)
if (info && info.terminal === e.terminal) {
info.running = false
const process = this.processes.get(id)
if (process) {
process.emit("shell_execution_complete", id, exitDetails)
}
break
}
if (terminalInfo && this.terminalIds.has(terminalInfo.id)) {
terminalInfo.shellExecutionComplete(exitDetails)
}
})
} catch (error) {
@ -165,50 +149,32 @@ export class TerminalManager {
runCommand(terminalInfo: Terminal, command: string): TerminalProcessResultPromise {
terminalInfo.busy = true
terminalInfo.lastCommand = command
const process = new TerminalProcess()
this.processes.set(terminalInfo.id, process)
process.once("completed", () => {
terminalInfo.busy = false
})
// Create process immediately
const process = new TerminalProcess(terminalInfo)
// if shell integration is not available, remove terminal so it does not get reused as it may be running a long-running process
process.once("no_shell_integration", () => {
console.log(`no_shell_integration received for terminal ${terminalInfo.id}`)
// Remove the terminal so we can't reuse it (in case it's running a long-running process)
TerminalRegistry.removeTerminal(terminalInfo.id)
this.terminalIds.delete(terminalInfo.id)
this.processes.delete(terminalInfo.id)
})
// Set process on terminal
terminalInfo.process = process
// Create a promise for command completion
const promise = new Promise<void>((resolve, reject) => {
process.once("continue", () => {
resolve()
})
// Set up event handlers
process.once("continue", () => resolve())
process.once("error", (error) => {
console.error(`Error in terminal ${terminalInfo.id}:`, error)
reject(error)
})
})
// Always use pWaitFor, which resolves immediately if shell integration is already available
pWaitFor(() => terminalInfo.terminal.shellIntegration !== undefined, { timeout: 4000 })
.then(() => {
const existingProcess = this.processes.get(terminalInfo.id)
if (existingProcess) {
existingProcess.run(terminalInfo.terminal, command)
} else {
console.error("[TerminalManager] existingProcess not found for terminal", terminalInfo.id)
}
})
.catch(() => {
// Shell integration did not become available within timeout
const existingProcess = this.processes.get(terminalInfo.id)
if (existingProcess) {
// Wait for shell integration before executing the command
pWaitFor(() => terminalInfo.terminal.shellIntegration !== undefined, { timeout: 4000 })
.then(() => {
process.run(command)
})
.catch(() => {
console.log("[TerminalManager] Shell integration not available. Command execution aborted.")
existingProcess.emit("no_shell_integration")
}
})
process.emit("no_shell_integration")
})
})
return mergePromise(process, promise)
}
@ -247,47 +213,11 @@ export class TerminalManager {
return newTerminalInfo
}
getTerminals(busy: boolean): { id: number; lastCommand: string }[] {
return Array.from(this.terminalIds)
.map((id) => TerminalRegistry.getTerminal(id))
.filter((t): t is Terminal => t !== undefined && t.busy === busy)
.map((t) => ({ id: t.id, lastCommand: t.lastCommand }))
}
getUnretrievedOutput(terminalId: number): string {
if (!this.terminalIds.has(terminalId)) {
return ""
}
const process = this.processes.get(terminalId)
return process ? process.getUnretrievedOutput() : ""
}
/**
* Finds the terminal ID by the VSCode terminal instance
* @param terminal The VSCode terminal instance
* @returns The terminal ID or undefined if not found
*/
private findTerminalIdByVscodeTerminal(terminal: vscode.Terminal): number | undefined {
for (const id of this.terminalIds) {
const info = TerminalRegistry.getTerminal(id)
if (info && info.terminal === terminal) {
return id
}
}
return undefined
}
isProcessHot(terminalId: number): boolean {
const process = this.processes.get(terminalId)
return process ? process.isHot : false
}
disposeAll() {
// for (const info of this.terminals) {
// //info.terminal.dispose() // dont want to dispose terminals when task is aborted
// }
this.terminalIds.clear()
this.processes.clear()
this.disposables.forEach((disposable) => disposable.dispose())
this.disposables = []
}

View file

@ -33,11 +33,32 @@ const PROCESS_HOT_TIMEOUT_COMPILING = 15_000
export class TerminalProcess extends EventEmitter<TerminalProcessEvents> {
private isListening: boolean = true
private terminalInfo: Terminal | undefined
private terminalInfo: Terminal
private lastEmitTime_ms: number = 0
private fullOutput: string = ""
private lastRetrievedIndex: number = 0
isHot: boolean = false
constructor(terminal: Terminal) {
super()
// Store terminal info for later use
this.terminalInfo = terminal
// Set up event handlers
this.once("completed", () => {
if (this.terminalInfo) {
this.terminalInfo.busy = false
}
})
this.once("no_shell_integration", () => {
if (this.terminalInfo) {
console.log(`no_shell_integration received for terminal ${this.terminalInfo.id}`)
TerminalRegistry.removeTerminal(this.terminalInfo.id)
// Note: TerminalManager.terminalIds cleanup would need to be handled
}
})
}
interpretExitCode(exitCode: number | undefined): ExitCodeDetails {
if (exitCode === undefined) {
@ -134,23 +155,15 @@ export class TerminalProcess extends EventEmitter<TerminalProcessEvents> {
}
private hotTimer: NodeJS.Timeout | null = null
async run(terminal: vscode.Terminal, command: string) {
if (terminal.shellIntegration && terminal.shellIntegration.executeCommand) {
// Get terminal info to access stream
const terminalInfo = TerminalRegistry.getTerminalInfoByTerminal(terminal)
if (!terminalInfo) {
console.error("[TerminalProcess] Terminal not found in registry")
this.emit("no_shell_integration")
this.emit("completed")
this.emit("continue")
return
}
async run(command: string) {
const terminal = this.terminalInfo.terminal
if (terminal.shellIntegration && terminal.shellIntegration.executeCommand) {
// When executeCommand() is called, onDidStartTerminalShellExecution will fire in TerminalManager
// which creates a new stream via execution.read() and emits 'stream_available'
const streamAvailable = new Promise<AsyncIterable<string>>((resolve) => {
this.once("stream_available", (id: number, stream: AsyncIterable<string>) => {
if (id === terminalInfo.id) {
if (id === this.terminalInfo.id) {
resolve(stream)
}
})
@ -159,15 +172,12 @@ export class TerminalProcess extends EventEmitter<TerminalProcessEvents> {
// Create promise that resolves when shell execution completes for this terminal
const shellExecutionComplete = new Promise<ExitCodeDetails>((resolve) => {
this.once("shell_execution_complete", (id: number, exitDetails: ExitCodeDetails) => {
if (id === terminalInfo.id) {
if (id === this.terminalInfo.id) {
resolve(exitDetails)
}
})
})
// getUnretrievedOutput needs to know if streamClosed, so store this for later
this.terminalInfo = terminalInfo
// Execute command
terminal.shellIntegration.executeCommand(command)
this.isHot = true
@ -253,7 +263,7 @@ export class TerminalProcess extends EventEmitter<TerminalProcessEvents> {
// Set streamClosed immediately after stream ends
if (this.terminalInfo) {
this.terminalInfo.streamClosed = true
this.terminalInfo.setActiveStream(undefined)
}
// Wait for shell execution to complete and handle exit details
@ -346,7 +356,7 @@ export class TerminalProcess extends EventEmitter<TerminalProcessEvents> {
// For active streams: return only complete lines (up to last \n).
// For closed streams: return all remaining content.
if (endIndex === -1) {
if (!this.terminalInfo?.streamClosed) {
if (this.terminalInfo && !this.terminalInfo.isStreamClosed()) {
// Stream still running - only process complete lines
endIndex = outputToProcess.lastIndexOf("\n")
if (endIndex === -1) {

View file

@ -51,7 +51,12 @@ export class TerminalRegistry {
}
}
static getTerminalInfoByTerminal(terminal: vscode.Terminal): Terminal | undefined {
/**
* Gets a terminal by its VSCode terminal instance
* @param terminal The VSCode terminal instance
* @returns The Terminal object, or undefined if not found
*/
static getTerminalByVSCETerminal(terminal: vscode.Terminal): Terminal | undefined {
const terminalInfo = this.terminals.find((t) => t.terminal === terminal)
if (terminalInfo && this.isTerminalClosed(terminalInfo.terminal)) {
@ -75,4 +80,39 @@ export class TerminalRegistry {
private static isTerminalClosed(terminal: vscode.Terminal): boolean {
return terminal.exitStatus !== undefined
}
/**
* Gets unretrieved output from a terminal process
* @param terminalId The terminal ID
* @returns The unretrieved output as a string, or empty string if terminal not found
*/
static getUnretrievedOutput(terminalId: number): string {
const terminal = this.getTerminal(terminalId)
if (!terminal) {
return ""
}
return terminal.process ? terminal.process.getUnretrievedOutput() : ""
}
/**
* Checks if a terminal process is "hot" (recently active)
* @param terminalId The terminal ID
* @returns True if the process is hot, false otherwise
*/
static isProcessHot(terminalId: number): boolean {
const terminal = this.getTerminal(terminalId)
if (!terminal) {
return false
}
return terminal.process ? terminal.process.isHot : false
}
/**
* Gets terminals filtered by busy state
* @param busy Whether to get busy or non-busy terminals
* @returns Array of Terminal objects
*/
static getTerminals(busy: boolean): Terminal[] {
return this.getAllTerminals().filter((t) => t.busy === busy)
}
}

View file

@ -35,8 +35,6 @@ describe("TerminalProcess", () => {
let mockStream: AsyncIterableIterator<string>
beforeEach(() => {
terminalProcess = new TerminalProcess()
// Create properly typed mock terminal
mockTerminal = {
shellIntegration: {
@ -61,6 +59,9 @@ describe("TerminalProcess", () => {
mockTerminalInfo = new Terminal(1, mockTerminal)
// Create a process for testing
terminalProcess = new TerminalProcess(mockTerminalInfo)
TerminalRegistry["terminals"].push(mockTerminalInfo)
// Reset event listeners
@ -93,7 +94,7 @@ describe("TerminalProcess", () => {
mockTerminal.shellIntegration.executeCommand.mockReturnValue(mockExecution)
const runPromise = terminalProcess.run(mockTerminal, "test command")
const runPromise = terminalProcess.run("test command")
terminalProcess.emit("stream_available", mockTerminalInfo.id, mockStream)
await runPromise
@ -102,28 +103,38 @@ describe("TerminalProcess", () => {
})
it("handles terminals without shell integration", async () => {
// Create a terminal without shell integration
const noShellTerminal = {
sendText: jest.fn(),
shellIntegration: undefined,
name: "No Shell Terminal",
processId: Promise.resolve(456),
creationOptions: {},
exitStatus: undefined,
state: { isInteractedWith: true },
dispose: jest.fn(),
hide: jest.fn(),
show: jest.fn(),
} as unknown as vscode.Terminal
// Create new terminal info with the no-shell terminal
const noShellTerminalInfo = new Terminal(2, noShellTerminal)
// Create new process with the no-shell terminal
const noShellProcess = new TerminalProcess(noShellTerminalInfo)
// Set up event listeners to verify events are emitted
const noShellPromise = new Promise<void>((resolve) => {
terminalProcess.once("no_shell_integration", resolve)
})
const completedPromise = new Promise<void>((resolve) => {
terminalProcess.once("completed", (_output?: string) => resolve())
})
const continuePromise = new Promise<void>((resolve) => {
terminalProcess.once("continue", resolve)
})
const eventPromises = Promise.all([
new Promise<void>((resolve) => noShellProcess.once("no_shell_integration", resolve)),
new Promise<void>((resolve) => noShellProcess.once("completed", (_output?: string) => resolve())),
new Promise<void>((resolve) => noShellProcess.once("continue", resolve)),
])
await terminalProcess.run(noShellTerminal, "test command")
// Run command and wait for all events
await noShellProcess.run("test command")
await eventPromises
// Verify all expected events are emitted
await Promise.all([noShellPromise, completedPromise, continuePromise])
// Verify sendText is called with the command
// Verify sendText was called with the command
expect(noShellTerminal.sendText).toHaveBeenCalledWith("test command", true)
})
@ -153,7 +164,7 @@ describe("TerminalProcess", () => {
read: jest.fn().mockReturnValue(mockStream),
})
const runPromise = terminalProcess.run(mockTerminal, "npm run build")
const runPromise = terminalProcess.run("npm run build")
terminalProcess.emit("stream_available", mockTerminalInfo.id, mockStream)
expect(terminalProcess.isHot).toBe(true)
@ -192,7 +203,7 @@ describe("TerminalProcess", () => {
describe("mergePromise", () => {
it("merges promise methods with terminal process", async () => {
const process = new TerminalProcess()
const process = new TerminalProcess(mockTerminalInfo)
const promise = Promise.resolve()
const merged = mergePromise(process, promise)

View file

@ -106,13 +106,13 @@ async function testTerminalCommand(
// Add the terminal to the registry
TerminalRegistry["terminals"] = [mockTerminalInfo]
// Create a new terminal process
startTime = process.hrtime.bigint() // Start timing from terminal process creation
const terminalProcess = new TerminalProcess()
// Create a terminal manager (this will set up the event handlers)
const terminalManager = new TerminalManager()
// Create a new terminal process for testing
startTime = process.hrtime.bigint() // Start timing from terminal process creation
const terminalProcess = new TerminalProcess(mockTerminalInfo)
try {
// Set up the mock stream with real command output
const mockStream = createRealCommandStream(command)
@ -124,6 +124,9 @@ async function testTerminalCommand(
}
})
// Execute the command
terminalProcess.run(command)
// Set up event listeners to capture output
let capturedOutput = ""
terminalProcess.on("completed", (output) => {
@ -143,13 +146,12 @@ async function testTerminalCommand(
})
})
// Store the process in the manager's processes map
// This is needed for the TerminalManager to find the process when events are triggered
terminalManager["processes"].set(mockTerminalInfo.id, terminalProcess)
// Set the process on the terminal and add terminal ID to manager
mockTerminalInfo.process = terminalProcess
terminalManager["terminalIds"].add(mockTerminalInfo.id)
// Run the command
const runPromise = terminalProcess.run(mockTerminal, command)
// Run the command (now handled by constructor)
// We've already created the process, so we'll trigger the events manually
// Get the event handlers from the mock
const eventHandlers = (vscode as any).__eventHandlers
@ -185,8 +187,6 @@ async function testTerminalCommand(
// Wait for the command to complete or timeout
await Promise.race([completedPromise, timeoutPromise])
await runPromise
// Calculate execution time in microseconds
// If endTime wasn't set (unlikely but possible), set it now
if (!timeRecorded) {

View file

@ -1,11 +1,28 @@
import { TerminalProcess } from "../TerminalProcess"
import { execSync } from "child_process"
import { Terminal } from "../Terminal"
import * as vscode from "vscode"
// Mock vscode.Terminal for testing
const mockTerminal = {
name: "Test Terminal",
processId: Promise.resolve(123),
creationOptions: {},
exitStatus: undefined,
state: { isInteractedWith: true },
dispose: jest.fn(),
hide: jest.fn(),
show: jest.fn(),
sendText: jest.fn(),
} as unknown as vscode.Terminal
describe("TerminalProcess.interpretExitCode", () => {
let terminalProcess: TerminalProcess
let mockTerminalInfo: Terminal
beforeEach(() => {
terminalProcess = new TerminalProcess()
mockTerminalInfo = new Terminal(1, mockTerminal)
terminalProcess = new TerminalProcess(mockTerminalInfo)
})
it("should handle undefined exit code", () => {
@ -89,9 +106,11 @@ describe("TerminalProcess.interpretExitCode", () => {
describe("TerminalProcess.interpretExitCode with real commands", () => {
let terminalProcess: TerminalProcess
let mockTerminalInfo: Terminal
beforeEach(() => {
terminalProcess = new TerminalProcess()
mockTerminalInfo = new Terminal(1, mockTerminal)
terminalProcess = new TerminalProcess(mockTerminalInfo)
})
it("should correctly interpret exit code 0 from successful command", () => {