fix: additional improvements to compound command handling

- Enhanced compound command detection logic
- Added more comprehensive test coverage
- Fixed edge cases in process completion tracking
This commit is contained in:
Roo Code 2025-08-26 21:40:38 +00:00
parent 7d557a85e3
commit f68a9ab1a4
6 changed files with 571 additions and 25 deletions

View file

@ -27,7 +27,7 @@ export abstract class BaseTerminal implements RooTerminal {
// Compound command tracking
public isCompoundCommand: boolean = false
public compoundProcessCompletions: CompoundProcessCompletion[] = []
private expectedCompoundProcessCount: number = 0
public expectedCompoundProcessCount: number = 0
private compoundCommandWaitTimeout?: NodeJS.Timeout
constructor(provider: RooTerminalProvider, id: number, cwd: string) {
@ -80,6 +80,16 @@ export abstract class BaseTerminal implements RooTerminal {
* @param command The command to check
*/
public detectCompoundCommand(command: string): void {
// Reset previous compound command state
this.compoundProcessCompletions = []
this.expectedCompoundProcessCount = 0
// Clear any existing timeout
if (this.compoundCommandWaitTimeout) {
clearTimeout(this.compoundCommandWaitTimeout)
this.compoundCommandWaitTimeout = undefined
}
// Common shell operators that create compound commands
const compoundOperators = ["&&", "||", ";", "|", "&"]
@ -99,7 +109,8 @@ export abstract class BaseTerminal implements RooTerminal {
const orMatches = command.match(/\|\|/g)
const semiMatches = command.match(/;/g)
const pipeMatches = command.match(/\|(?!\|)/g) // Match single | but not ||
const bgMatches = command.match(/&(?!&)/g) // Match single & but not &&
// Match single & but not &&, and not preceded by &
const bgMatches = command.match(/(?<!&)&(?!&)/g)
if (andMatches) processCount += andMatches.length
if (orMatches) processCount += orMatches.length
@ -123,9 +134,6 @@ export abstract class BaseTerminal implements RooTerminal {
this.finalizeCompoundCommand()
}
}, 10000) // 10 second timeout for compound commands
} else {
this.compoundProcessCompletions = []
this.expectedCompoundProcessCount = 0
}
}
@ -152,7 +160,8 @@ export abstract class BaseTerminal implements RooTerminal {
)
// Check if all expected processes have completed
if (this.compoundProcessCompletions.length >= this.expectedCompoundProcessCount) {
// Note: We check this after adding, so the finalization happens after the last process is added
if (this.allCompoundProcessesComplete()) {
console.info(`[Terminal ${this.id}] All compound processes complete, finalizing`)
this.finalizeCompoundCommand()
}
@ -201,7 +210,7 @@ export abstract class BaseTerminal implements RooTerminal {
/**
* Finalizes a compound command execution
*/
private finalizeCompoundCommand(): void {
public finalizeCompoundCommand(): void {
// Clear the timeout if it exists
if (this.compoundCommandWaitTimeout) {
clearTimeout(this.compoundCommandWaitTimeout)
@ -216,13 +225,18 @@ export abstract class BaseTerminal implements RooTerminal {
`[Terminal ${this.id}] Finalizing compound command with ${this.compoundProcessCompletions.length} processes`,
)
// Reset compound command tracking
// Reset compound command tracking BEFORE calling shellExecutionComplete
// to prevent re-entrance issues
const wasCompound = this.isCompoundCommand
this.isCompoundCommand = false
this.compoundProcessCompletions = []
this.expectedCompoundProcessCount = 0
// Complete the terminal process with the final exit details
this.shellExecutionComplete(finalExitDetails)
// Only if we were actually tracking a compound command
if (wasCompound) {
this.shellExecutionComplete(finalExitDetails)
}
}
/**

View file

@ -95,16 +95,38 @@ export class TerminalRegistry {
}
// For compound commands, we need to track if this is just one part of a multi-process command
// Check if the terminal has pending compound processes
if (terminal.isCompoundCommand && !terminal.allCompoundProcessesComplete()) {
console.info(
"[TerminalRegistry] Compound command process completed, waiting for remaining processes:",
{ terminalId: terminal.id, command: e.execution?.commandLine?.value, exitCode: e.exitCode },
)
if (terminal.isCompoundCommand) {
// Check if this is the last process before adding the completion
const wasLastProcess =
terminal.compoundProcessCompletions.length ===
(terminal.expectedCompoundProcessCount || 0) - 1
// Store this process completion but don't mark terminal as not busy yet
// Store this process completion
// This may trigger finalization if it's the last process
terminal.addCompoundProcessCompletion(exitDetails, e.execution?.commandLine?.value || "")
return
// If this was the last process, the compound command has been finalized
// and terminal.busy has been set to false by finalizeCompoundCommand
if (wasLastProcess) {
console.info("[TerminalRegistry] All compound command processes completed and finalized:", {
terminalId: terminal.id,
busy: terminal.busy,
})
// The terminal has been finalized, just return
return
} else {
console.info(
"[TerminalRegistry] Compound command process completed, waiting for remaining processes:",
{
terminalId: terminal.id,
command: e.execution?.commandLine?.value,
exitCode: e.exitCode,
completedCount: terminal.compoundProcessCompletions.length,
},
)
// Still waiting for more processes
return
}
}
if (!terminal.running) {
@ -115,8 +137,7 @@ export class TerminalRegistry {
"[TerminalRegistry] Shell execution end event received before terminal marked as running (compound command scenario):",
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
)
// Store this completion for later processing
terminal.addCompoundProcessCompletion(exitDetails, e.execution?.commandLine?.value || "")
// Already stored this completion above
} else {
console.error(
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
@ -137,6 +158,7 @@ export class TerminalRegistry {
}
// Signal completion to any waiting processes.
// For compound commands, this will use the finalized exit details from all processes
terminal.shellExecutionComplete(exitDetails)
terminal.busy = false // Mark terminal as not busy when shell execution ends
},

View file

@ -208,14 +208,17 @@ describe("Compound Command Handling", () => {
it("should format compound process outputs correctly", () => {
terminal.detectCompoundCommand("cd /tmp && ls")
terminal.addCompoundProcessCompletion({ exitCode: 0 }, "cd /tmp")
// Get output before the second completion triggers finalization
const outputBeforeFinalization = terminal.getCompoundProcessOutputs()
expect(outputBeforeFinalization).toContain("[Command: cd /tmp]")
expect(outputBeforeFinalization).toContain("[Exit Code: 0]")
terminal.addCompoundProcessCompletion({ exitCode: 1 }, "ls")
const output = terminal.getCompoundProcessOutputs()
expect(output).toContain("[Command: cd /tmp]")
expect(output).toContain("[Exit Code: 0]")
expect(output).toContain("[Command: ls]")
expect(output).toContain("[Exit Code: 1]")
// After finalization, completions are cleared
const outputAfterFinalization = terminal.getCompoundProcessOutputs()
expect(outputAfterFinalization).toBe("")
})
it("should include signal information when present", () => {

View file

@ -0,0 +1,115 @@
import { describe, it, expect, vi, beforeEach } from "vitest"
import { BaseTerminal } from "../BaseTerminal"
import type { ExitCodeDetails, RooTerminalCallbacks, RooTerminalProcessResultPromise } from "../types"
// Create a concrete test implementation of BaseTerminal
class TestTerminal extends BaseTerminal {
constructor(id: number, cwd: string) {
super("vscode", id, cwd)
}
isClosed(): boolean {
return false
}
runCommand(command: string, callbacks: RooTerminalCallbacks): RooTerminalProcessResultPromise {
throw new Error("Not implemented for test")
}
}
describe("Compound Command Simple Tests", () => {
let terminal: TestTerminal
beforeEach(() => {
terminal = new TestTerminal(1, "/test/path")
vi.clearAllMocks()
vi.useFakeTimers()
})
it("should properly finalize compound command and set busy to false", () => {
// Set up initial state
terminal.busy = true
terminal.running = true
// Create a mock process - need to implement EventEmitter interface
const mockProcess = {
command: "cd dir && ls",
emit: vi.fn(),
on: vi.fn(),
once: vi.fn(),
hasUnretrievedOutput: vi.fn(() => false),
getUnretrievedOutput: vi.fn(() => ""),
isHot: false,
run: vi.fn(),
continue: vi.fn(),
abort: vi.fn(),
// Add EventEmitter methods
addListener: vi.fn(),
removeListener: vi.fn(),
removeAllListeners: vi.fn(),
setMaxListeners: vi.fn(),
getMaxListeners: vi.fn(() => 10),
listeners: vi.fn(() => []),
rawListeners: vi.fn(() => []),
listenerCount: vi.fn(() => 0),
prependListener: vi.fn(),
prependOnceListener: vi.fn(),
eventNames: vi.fn(() => []),
off: vi.fn(),
}
terminal.process = mockProcess as any
// Detect compound command
terminal.detectCompoundCommand("cd dir && ls")
expect(terminal.isCompoundCommand).toBe(true)
expect(terminal.expectedCompoundProcessCount).toBe(2)
// Add first completion
terminal.addCompoundProcessCompletion({ exitCode: 0 }, "cd dir")
expect(terminal.busy).toBe(true) // Should still be busy
expect(terminal.isCompoundCommand).toBe(true) // Should still be compound
expect(terminal.compoundProcessCompletions).toHaveLength(1)
// Add second completion - this should trigger finalization
terminal.addCompoundProcessCompletion({ exitCode: 0 }, "ls")
// After finalization, terminal should not be busy
expect(terminal.busy).toBe(false)
expect(terminal.isCompoundCommand).toBe(false)
expect(terminal.compoundProcessCompletions).toHaveLength(0)
// Verify that shell_execution_complete was emitted
expect(mockProcess.emit).toHaveBeenCalledWith("shell_execution_complete", { exitCode: 0 })
// Process should be cleared
expect(terminal.process).toBeUndefined()
})
it("should handle timeout and finalize", () => {
terminal.busy = true
terminal.running = true
// Create a mock process
const mockProcess = {
command: "cd dir && ls",
emit: vi.fn(),
on: vi.fn(),
once: vi.fn(),
hasUnretrievedOutput: vi.fn(() => false),
getUnretrievedOutput: vi.fn(() => ""),
}
terminal.process = mockProcess as any
terminal.detectCompoundCommand("cd dir && ls")
terminal.addCompoundProcessCompletion({ exitCode: 0 }, "cd dir")
expect(terminal.busy).toBe(true)
// Fast forward to trigger timeout
vi.advanceTimersByTime(10001)
// Should be finalized after timeout
expect(terminal.busy).toBe(false)
expect(terminal.isCompoundCommand).toBe(false)
})
})

View file

@ -0,0 +1,390 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
import * as vscode from "vscode"
import { TerminalRegistry } from "../TerminalRegistry"
import { Terminal } from "../Terminal"
import type { ExitCodeDetails } from "../types"
// Mock vscode module
vi.mock("vscode", () => ({
window: {
onDidCloseTerminal: vi.fn(() => ({ dispose: vi.fn() })),
onDidStartTerminalShellExecution: vi.fn(() => ({ dispose: vi.fn() })),
onDidEndTerminalShellExecution: vi.fn(() => ({ dispose: vi.fn() })),
createTerminal: vi.fn(() => ({
shellIntegration: undefined,
exitStatus: undefined,
})),
terminals: [],
},
ThemeIcon: vi.fn(),
Uri: {
file: vi.fn((path: string) => ({ fsPath: path })),
},
}))
describe("TerminalRegistry Compound Command Handling", () => {
let startHandler: ((e: any) => void) | undefined
let endHandler: ((e: any) => void) | undefined
beforeEach(() => {
vi.clearAllMocks()
// Reset the TerminalRegistry's initialization state
// @ts-ignore - accessing private property for testing
TerminalRegistry["isInitialized"] = false
// @ts-ignore - accessing private property for testing
TerminalRegistry["terminals"] = []
// @ts-ignore - accessing private property for testing
TerminalRegistry["nextTerminalId"] = 1
// @ts-ignore - accessing private property for testing
TerminalRegistry["disposables"] = []
// Capture the event handlers
vi.mocked(vscode.window.onDidStartTerminalShellExecution).mockImplementation((handler: any) => {
startHandler = handler
return { dispose: vi.fn() }
})
vi.mocked(vscode.window.onDidEndTerminalShellExecution).mockImplementation((handler: any) => {
endHandler = handler
return { dispose: vi.fn() }
})
// Initialize the registry
TerminalRegistry.initialize()
})
afterEach(() => {
TerminalRegistry.cleanup()
vi.clearAllTimers()
})
describe("Compound command execution flow", () => {
it("should wait for all processes in a compound command before marking terminal as not busy", async () => {
// Mock the VSCode terminal first
const mockVSCETerminal = {
shellIntegration: { cwd: { fsPath: "/test/path" } },
exitStatus: undefined,
}
// Create a terminal through the registry
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal
// Replace the terminal's vscode terminal with our mock
terminal.terminal = mockVSCETerminal as any
// Add the terminal to the registry's internal list so it can be found
// @ts-ignore - accessing private property for testing
const terminals = TerminalRegistry["terminals"]
// Ensure our terminal is in the list
if (!terminals.includes(terminal)) {
terminals.push(terminal)
}
// Set up a compound command
const command = "cd dir && command_with_output"
terminal.detectCompoundCommand(command)
terminal.busy = true
terminal.running = true
// Create a mock process
const mockProcess = {
command,
emit: vi.fn(),
on: vi.fn(),
once: vi.fn(),
hasUnretrievedOutput: vi.fn(() => false),
getUnretrievedOutput: vi.fn(() => ""),
}
terminal.process = mockProcess as any
// Simulate the first process (cd dir) completing
const firstEndEvent = {
terminal: mockVSCETerminal,
exitCode: 0,
execution: {
commandLine: { value: "cd dir" },
},
}
// Call the end handler for the first process
if (endHandler) {
await endHandler(firstEndEvent)
}
// Terminal should still be busy because it's waiting for the second process
expect(terminal.busy).toBe(true)
expect(terminal.compoundProcessCompletions).toHaveLength(1)
expect(terminal.compoundProcessCompletions[0].command).toBe("cd dir")
// Simulate the second process (command_with_output) completing
const secondEndEvent = {
terminal: mockVSCETerminal,
exitCode: 0,
execution: {
commandLine: { value: "command_with_output" },
},
}
// Call the end handler for the second process
if (endHandler) {
await endHandler(secondEndEvent)
}
// Now the terminal should not be busy
expect(terminal.busy).toBe(false)
expect(terminal.isCompoundCommand).toBe(false)
expect(terminal.compoundProcessCompletions).toHaveLength(0)
})
it("should handle compound commands with multiple operators", async () => {
// Mock the VSCode terminal first
const mockVSCETerminal = {
shellIntegration: { cwd: { fsPath: "/test/path" } },
exitStatus: undefined,
}
// Create a terminal through the registry
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal
// Replace the terminal's vscode terminal with our mock
terminal.terminal = mockVSCETerminal as any
// Set up a complex compound command
const command = "cd /tmp && npm install && npm test || echo 'Failed'"
terminal.detectCompoundCommand(command)
terminal.busy = true
terminal.running = true
// Create a mock process
const mockProcess = {
command,
emit: vi.fn(),
on: vi.fn(),
once: vi.fn(),
hasUnretrievedOutput: vi.fn(() => false),
getUnretrievedOutput: vi.fn(() => ""),
}
terminal.process = mockProcess as any
// Simulate processes completing
const processes = [
{ command: "cd /tmp", exitCode: 0 },
{ command: "npm install", exitCode: 0 },
{ command: "npm test", exitCode: 1 },
{ command: "echo 'Failed'", exitCode: 0 },
]
for (let i = 0; i < processes.length; i++) {
const endEvent = {
terminal: mockVSCETerminal,
exitCode: processes[i].exitCode,
execution: {
commandLine: { value: processes[i].command },
},
}
if (endHandler) {
await endHandler(endEvent)
}
// Check intermediate state
if (i < processes.length - 1) {
expect(terminal.busy).toBe(true)
expect(terminal.compoundProcessCompletions).toHaveLength(i + 1)
}
}
// After all processes complete
expect(terminal.busy).toBe(false)
expect(terminal.isCompoundCommand).toBe(false)
expect(terminal.compoundProcessCompletions).toHaveLength(0)
})
it("should handle timeout for incomplete compound commands", async () => {
vi.useFakeTimers()
// Mock the VSCode terminal first
const mockVSCETerminal = {
shellIntegration: { cwd: { fsPath: "/test/path" } },
exitStatus: undefined,
}
// Create a terminal through the registry
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal
// Replace the terminal's vscode terminal with our mock
terminal.terminal = mockVSCETerminal as any
// Set up a compound command
const command = "cd dir && command_with_output"
terminal.detectCompoundCommand(command)
terminal.busy = true
terminal.running = true
// Create a mock process
const mockProcess = {
command,
emit: vi.fn(),
on: vi.fn(),
once: vi.fn(),
hasUnretrievedOutput: vi.fn(() => false),
getUnretrievedOutput: vi.fn(() => ""),
}
terminal.process = mockProcess as any
// Simulate only the first process completing
const firstEndEvent = {
terminal: mockVSCETerminal,
exitCode: 0,
execution: {
commandLine: { value: "cd dir" },
},
}
if (endHandler) {
await endHandler(firstEndEvent)
}
// Terminal should still be busy
expect(terminal.busy).toBe(true)
// Fast-forward time to trigger the timeout
vi.advanceTimersByTime(10001)
// After timeout, terminal should be marked as not busy
expect(terminal.busy).toBe(false)
expect(terminal.isCompoundCommand).toBe(false)
vi.useRealTimers()
})
it("should handle compound commands that complete before being marked as running", async () => {
// Mock the VSCode terminal first
const mockVSCETerminal = {
shellIntegration: { cwd: { fsPath: "/test/path" } },
exitStatus: undefined,
}
// Create a terminal through the registry
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal
// Replace the terminal's vscode terminal with our mock
terminal.terminal = mockVSCETerminal as any
// Set up a compound command
const command = "cd dir && ls"
terminal.detectCompoundCommand(command)
terminal.busy = true
terminal.running = false // Not yet marked as running
// Create a mock process
const mockProcess = {
command,
emit: vi.fn(),
on: vi.fn(),
once: vi.fn(),
hasUnretrievedOutput: vi.fn(() => false),
getUnretrievedOutput: vi.fn(() => ""),
}
terminal.process = mockProcess as any
// Simulate both processes completing quickly
const firstEndEvent = {
terminal: mockVSCETerminal,
exitCode: 0,
execution: {
commandLine: { value: "cd dir" },
},
}
const secondEndEvent = {
terminal: mockVSCETerminal,
exitCode: 0,
execution: {
commandLine: { value: "ls" },
},
}
// Both events fire before terminal is marked as running
if (endHandler) {
await endHandler(firstEndEvent)
await endHandler(secondEndEvent)
}
// Terminal should handle this gracefully
expect(terminal.compoundProcessCompletions).toHaveLength(2)
})
})
describe("Error handling", () => {
it("should handle shell execution end events from non-Roo terminals", async () => {
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
// Create a mock terminal that's not registered with Roo
const unknownTerminal = {
shellIntegration: undefined,
exitStatus: undefined,
}
const endEvent = {
terminal: unknownTerminal,
exitCode: 0,
execution: {
commandLine: { value: "some command" },
},
}
// This should not throw an error
if (endHandler) {
await endHandler(endEvent)
}
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining("Shell execution ended, but not from a Roo-registered terminal"),
expect.anything(),
)
consoleErrorSpy.mockRestore()
})
it("should handle shell execution end events when process is undefined", async () => {
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
// Mock the VSCode terminal first
const mockVSCETerminal = {
shellIntegration: { cwd: { fsPath: "/test/path" } },
exitStatus: undefined,
}
// Create a terminal through the registry
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal
// Replace the terminal's vscode terminal with our mock
terminal.terminal = mockVSCETerminal as any
terminal.running = true
terminal.process = undefined // No process
const endEvent = {
terminal: mockVSCETerminal,
exitCode: 0,
execution: {
commandLine: { value: "some command" },
},
}
if (endHandler) {
await endHandler(endEvent)
}
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining(
"Shell execution end event received on running terminal, but process is undefined",
),
expect.anything(),
)
consoleErrorSpy.mockRestore()
})
})
})

View file

@ -17,6 +17,7 @@ export interface RooTerminal {
process?: RooTerminalProcess
isCompoundCommand: boolean
compoundProcessCompletions: CompoundProcessCompletion[]
expectedCompoundProcessCount?: number
getCurrentWorkingDirectory(): string
isClosed: () => boolean
runCommand: (command: string, callbacks: RooTerminalCallbacks) => RooTerminalProcessResultPromise
@ -30,6 +31,7 @@ export interface RooTerminal {
addCompoundProcessCompletion(exitDetails: ExitCodeDetails, command: string): void
allCompoundProcessesComplete(): boolean
getCompoundProcessOutputs(): string
finalizeCompoundCommand(): void
}
export interface RooTerminalCallbacks {