mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: prevent conda auto-activation from hijacking terminal command output
When a terminal is created with conda auto-activation (or any shell initialization that runs commands), a race condition could occur where VSCode fires onDidStartTerminalShellExecution events for BOTH the conda activation AND Roo's command. The previous implementation blindly set the stream when the event fired, causing Roo to pick up the wrong stream and miss its own command's output. This fix: 1. Adds an optional eventCommand parameter to setActiveStream() 2. Passes the command from the shell execution event for verification 3. Only emits stream_available if the event command matches Roo's command 4. Uses flexible prefix matching to handle PowerShell workarounds Fixes #11148
This commit is contained in:
parent
44fd975b17
commit
fb720e4204
4 changed files with 276 additions and 5 deletions
|
|
@ -40,11 +40,16 @@ export abstract class BaseTerminal implements RooTerminal {
|
|||
abstract runCommand(command: string, callbacks: RooTerminalCallbacks): RooTerminalProcessResultPromise
|
||||
|
||||
/**
|
||||
* Sets the active stream for this terminal and notifies the process
|
||||
* Sets the active stream for this terminal and notifies the process.
|
||||
* When eventCommand is provided, the stream is only set if it matches the process's command.
|
||||
* This prevents conda/other auto-activation commands from interfering with Roo's command execution.
|
||||
*
|
||||
* @param stream The stream to set, or undefined to clean up
|
||||
* @param pid The process ID from the shell execution event
|
||||
* @param eventCommand The command from the shell execution event, used to verify this is Roo's command
|
||||
* @throws Error if process is undefined when a stream is provided
|
||||
*/
|
||||
public setActiveStream(stream: AsyncIterable<string> | undefined, pid?: number): void {
|
||||
public setActiveStream(stream: AsyncIterable<string> | undefined, pid?: number, eventCommand?: string): void {
|
||||
if (stream) {
|
||||
if (!this.process) {
|
||||
this.running = false
|
||||
|
|
@ -56,6 +61,18 @@ export abstract class BaseTerminal implements RooTerminal {
|
|||
return
|
||||
}
|
||||
|
||||
// If eventCommand is provided, verify it matches the process's expected command
|
||||
// This prevents conda/pyenv/other auto-activation commands from hijacking the stream
|
||||
if (eventCommand !== undefined && this.process.command) {
|
||||
if (!BaseTerminal.commandsMatch(this.process.command, eventCommand)) {
|
||||
console.warn(
|
||||
`[Terminal ${this.provider}/${this.id}] Ignoring shell execution for non-matching command. ` +
|
||||
`Expected: "${this.process.command}", Got: "${eventCommand}"`,
|
||||
)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
this.running = true
|
||||
this.streamClosed = false
|
||||
this.process.emit("shell_execution_started", pid)
|
||||
|
|
@ -65,6 +82,39 @@ export abstract class BaseTerminal implements RooTerminal {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if two commands match. Uses flexible matching to handle cases where
|
||||
* the executed command might be modified (e.g., PowerShell counter workaround).
|
||||
*
|
||||
* @param expectedCommand The command Roo is trying to execute
|
||||
* @param actualCommand The command from the shell execution event
|
||||
* @returns true if the commands match
|
||||
*/
|
||||
public static commandsMatch(expectedCommand: string, actualCommand: string): boolean {
|
||||
// Normalize commands by trimming whitespace
|
||||
const expected = expectedCommand.trim()
|
||||
const actual = actualCommand.trim()
|
||||
|
||||
// Exact match (handles both empty string case and exact matches)
|
||||
if (expected === actual) {
|
||||
return true
|
||||
}
|
||||
|
||||
// If expected is empty but actual is not, they don't match
|
||||
// (this prevents empty process command from matching any event command)
|
||||
if (expected.length === 0) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Check if expected is a prefix of actual (handles PowerShell counter workaround
|
||||
// which appends extra commands like ` ; "(Roo/PS Workaround: N)" > $null`)
|
||||
if (actual.startsWith(expected)) {
|
||||
return true
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
/**
|
||||
* Handles shell execution completion for this terminal.
|
||||
* @param exitDetails The exit details of the shell execution
|
||||
|
|
|
|||
|
|
@ -51,14 +51,19 @@ export class TerminalRegistry {
|
|||
// Get a handle to the stream as early as possible:
|
||||
const stream = e.execution.read()
|
||||
const terminal = this.getTerminalByVSCETerminal(e.terminal)
|
||||
const eventCommand = e.execution?.commandLine?.value
|
||||
|
||||
console.info("[onDidStartTerminalShellExecution]", {
|
||||
command: e.execution?.commandLine?.value,
|
||||
command: eventCommand,
|
||||
terminalId: terminal?.id,
|
||||
processCommand: terminal?.process?.command,
|
||||
})
|
||||
|
||||
if (terminal) {
|
||||
terminal.setActiveStream(stream)
|
||||
// Pass the eventCommand to setActiveStream for verification.
|
||||
// This prevents conda/pyenv/other auto-activation commands from
|
||||
// hijacking the stream when the terminal first opens.
|
||||
terminal.setActiveStream(stream, undefined, eventCommand)
|
||||
terminal.busy = true // Mark terminal as busy when shell execution starts
|
||||
} else {
|
||||
console.error(
|
||||
|
|
|
|||
216
src/integrations/terminal/__tests__/BaseTerminal.spec.ts
Normal file
216
src/integrations/terminal/__tests__/BaseTerminal.spec.ts
Normal file
|
|
@ -0,0 +1,216 @@
|
|||
// npx vitest run src/integrations/terminal/__tests__/BaseTerminal.spec.ts
|
||||
|
||||
import { BaseTerminal } from "../BaseTerminal"
|
||||
import type { RooTerminalProcess } from "../types"
|
||||
|
||||
// Create a concrete implementation of BaseTerminal for testing
|
||||
class TestTerminal extends BaseTerminal {
|
||||
constructor(id: number = 1, cwd: string = "/test") {
|
||||
super("vscode", id, cwd)
|
||||
}
|
||||
|
||||
isClosed(): boolean {
|
||||
return false
|
||||
}
|
||||
|
||||
runCommand(): never {
|
||||
throw new Error("Not implemented")
|
||||
}
|
||||
}
|
||||
|
||||
// Create a mock process for testing
|
||||
function createMockProcess(command: string): RooTerminalProcess {
|
||||
const events: Record<string, ((...args: any[]) => void)[]> = {}
|
||||
|
||||
return {
|
||||
command,
|
||||
isHot: false,
|
||||
run: vi.fn(),
|
||||
continue: vi.fn(),
|
||||
abort: vi.fn(),
|
||||
hasUnretrievedOutput: vi.fn().mockReturnValue(false),
|
||||
getUnretrievedOutput: vi.fn().mockReturnValue(""),
|
||||
trimRetrievedOutput: vi.fn(),
|
||||
on: vi.fn((event: string, handler: (...args: any[]) => void) => {
|
||||
if (!events[event]) {
|
||||
events[event] = []
|
||||
}
|
||||
events[event].push(handler)
|
||||
}),
|
||||
once: vi.fn((event: string, handler: (...args: any[]) => void) => {
|
||||
if (!events[event]) {
|
||||
events[event] = []
|
||||
}
|
||||
events[event].push(handler)
|
||||
}),
|
||||
emit: vi.fn((event: string, ...args: any[]) => {
|
||||
const handlers = events[event] || []
|
||||
handlers.forEach((handler) => handler(...args))
|
||||
return true
|
||||
}),
|
||||
off: vi.fn(),
|
||||
removeListener: vi.fn(),
|
||||
removeAllListeners: vi.fn(),
|
||||
listeners: vi.fn(),
|
||||
rawListeners: vi.fn(),
|
||||
listenerCount: vi.fn(),
|
||||
prependListener: vi.fn(),
|
||||
prependOnceListener: vi.fn(),
|
||||
eventNames: vi.fn(),
|
||||
addListener: vi.fn(),
|
||||
setMaxListeners: vi.fn(),
|
||||
getMaxListeners: vi.fn(),
|
||||
} as unknown as RooTerminalProcess
|
||||
}
|
||||
|
||||
// Create a mock async iterable stream for testing
|
||||
async function* createMockStream(): AsyncGenerator<string> {
|
||||
yield "test output"
|
||||
}
|
||||
|
||||
describe("BaseTerminal", () => {
|
||||
describe("commandsMatch", () => {
|
||||
it("returns true for exact match", () => {
|
||||
expect(BaseTerminal.commandsMatch("npm test", "npm test")).toBe(true)
|
||||
})
|
||||
|
||||
it("returns true for exact match with whitespace trimming", () => {
|
||||
expect(BaseTerminal.commandsMatch(" npm test ", "npm test")).toBe(true)
|
||||
expect(BaseTerminal.commandsMatch("npm test", " npm test ")).toBe(true)
|
||||
})
|
||||
|
||||
it("returns true when actual command starts with expected command (PowerShell workaround)", () => {
|
||||
// PowerShell counter workaround appends extra commands
|
||||
const expected = "npm test"
|
||||
const actual = 'npm test ; "(Roo/PS Workaround: 1)" > $null'
|
||||
expect(BaseTerminal.commandsMatch(expected, actual)).toBe(true)
|
||||
})
|
||||
|
||||
it("returns true when actual command has trailing sleep command", () => {
|
||||
const expected = "npm test"
|
||||
const actual = "npm test ; start-sleep -milliseconds 50"
|
||||
expect(BaseTerminal.commandsMatch(expected, actual)).toBe(true)
|
||||
})
|
||||
|
||||
it("returns false for completely different commands", () => {
|
||||
expect(BaseTerminal.commandsMatch("npm test", "conda activate base")).toBe(false)
|
||||
})
|
||||
|
||||
it("returns false when commands are similar but not matching", () => {
|
||||
expect(BaseTerminal.commandsMatch("npm install", "npm run build")).toBe(false)
|
||||
})
|
||||
|
||||
it("returns false for reversed prefix (actual is prefix of expected)", () => {
|
||||
// This case should not match - the expected command should be a prefix of actual
|
||||
expect(BaseTerminal.commandsMatch("npm test --coverage", "npm test")).toBe(false)
|
||||
})
|
||||
|
||||
it("handles empty strings", () => {
|
||||
expect(BaseTerminal.commandsMatch("", "")).toBe(true)
|
||||
expect(BaseTerminal.commandsMatch("npm test", "")).toBe(false)
|
||||
expect(BaseTerminal.commandsMatch("", "npm test")).toBe(false)
|
||||
})
|
||||
|
||||
it("handles commands with special characters", () => {
|
||||
const expected = 'echo "hello world"'
|
||||
const actual = 'echo "hello world"'
|
||||
expect(BaseTerminal.commandsMatch(expected, actual)).toBe(true)
|
||||
})
|
||||
|
||||
it("handles conda activation commands correctly", () => {
|
||||
// Roo's command should not match conda's auto-activation
|
||||
expect(BaseTerminal.commandsMatch("npm test", "conda activate base")).toBe(false)
|
||||
expect(BaseTerminal.commandsMatch("npm test", "source activate myenv")).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe("setActiveStream", () => {
|
||||
let terminal: TestTerminal
|
||||
let mockProcess: RooTerminalProcess
|
||||
|
||||
beforeEach(() => {
|
||||
terminal = new TestTerminal()
|
||||
mockProcess = createMockProcess("npm test")
|
||||
terminal.process = mockProcess
|
||||
})
|
||||
|
||||
it("sets stream when no eventCommand is provided (backwards compatibility)", () => {
|
||||
const stream = createMockStream()
|
||||
terminal.setActiveStream(stream)
|
||||
|
||||
expect(terminal.running).toBe(true)
|
||||
expect(mockProcess.emit).toHaveBeenCalledWith("shell_execution_started", undefined)
|
||||
expect(mockProcess.emit).toHaveBeenCalledWith("stream_available", stream)
|
||||
})
|
||||
|
||||
it("sets stream when eventCommand matches process command", () => {
|
||||
const stream = createMockStream()
|
||||
terminal.setActiveStream(stream, undefined, "npm test")
|
||||
|
||||
expect(terminal.running).toBe(true)
|
||||
expect(mockProcess.emit).toHaveBeenCalledWith("stream_available", stream)
|
||||
})
|
||||
|
||||
it("sets stream when eventCommand starts with process command (PowerShell case)", () => {
|
||||
const stream = createMockStream()
|
||||
terminal.setActiveStream(stream, undefined, 'npm test ; "(Roo/PS Workaround: 1)" > $null')
|
||||
|
||||
expect(terminal.running).toBe(true)
|
||||
expect(mockProcess.emit).toHaveBeenCalledWith("stream_available", stream)
|
||||
})
|
||||
|
||||
it("ignores stream when eventCommand does not match process command", () => {
|
||||
const stream = createMockStream()
|
||||
const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
|
||||
|
||||
terminal.setActiveStream(stream, undefined, "conda activate base")
|
||||
|
||||
expect(terminal.running).toBe(false)
|
||||
expect(mockProcess.emit).not.toHaveBeenCalledWith("stream_available", expect.anything())
|
||||
expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining("Ignoring shell execution"))
|
||||
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
|
||||
it("accepts stream when process has no command set (backward compatibility)", () => {
|
||||
mockProcess.command = ""
|
||||
const stream = createMockStream()
|
||||
|
||||
// When process.command is empty string, the command verification is skipped
|
||||
// for backward compatibility. The process should accept any stream.
|
||||
terminal.setActiveStream(stream, undefined, "conda activate base")
|
||||
|
||||
// Since process.command is empty, no verification is done and stream is accepted
|
||||
expect(terminal.running).toBe(true)
|
||||
expect(mockProcess.emit).toHaveBeenCalledWith("stream_available", stream)
|
||||
})
|
||||
|
||||
it("cleans up when stream is undefined", () => {
|
||||
terminal.setActiveStream(undefined)
|
||||
|
||||
expect(terminal.isStreamClosed).toBe(true)
|
||||
})
|
||||
|
||||
it("handles missing process gracefully", () => {
|
||||
terminal.process = undefined
|
||||
const stream = createMockStream()
|
||||
const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
|
||||
|
||||
terminal.setActiveStream(stream)
|
||||
|
||||
expect(terminal.running).toBe(false)
|
||||
expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining("process is undefined"))
|
||||
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
|
||||
it("passes pid to shell_execution_started event", () => {
|
||||
const stream = createMockStream()
|
||||
const pid = 12345
|
||||
|
||||
terminal.setActiveStream(stream, pid, "npm test")
|
||||
|
||||
expect(mockProcess.emit).toHaveBeenCalledWith("shell_execution_started", pid)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -12,7 +12,7 @@ export interface RooTerminal {
|
|||
getCurrentWorkingDirectory(): string
|
||||
isClosed: () => boolean
|
||||
runCommand: (command: string, callbacks: RooTerminalCallbacks) => RooTerminalProcessResultPromise
|
||||
setActiveStream(stream: AsyncIterable<string> | undefined, pid?: number): void
|
||||
setActiveStream(stream: AsyncIterable<string> | undefined, pid?: number, eventCommand?: string): void
|
||||
shellExecutionComplete(exitDetails: ExitCodeDetails): void
|
||||
getProcessesWithOutput(): RooTerminalProcess[]
|
||||
getUnretrievedOutput(): string
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue