mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: handle race condition for compound commands in new terminals
- Add compound command detection to identify commands with &&, ||, ;, |, & operators - Implement delay mechanism for compound commands on newly spawned terminals - Ensure shell integration is fully attached before executing compound commands - Improve error handling in TerminalRegistry for race conditions - Add comprehensive tests for compound command handling Fixes #7430
This commit is contained in:
parent
11c454ffa2
commit
182ecc0e53
3 changed files with 288 additions and 5 deletions
|
|
@ -11,6 +11,7 @@ export class Terminal extends BaseTerminal {
|
|||
public terminal: vscode.Terminal
|
||||
|
||||
public cmdCounter: number = 0
|
||||
private shellIntegrationReady: boolean = false
|
||||
|
||||
constructor(id: number, terminal: vscode.Terminal | undefined, cwd: string) {
|
||||
super("vscode", id, cwd)
|
||||
|
|
@ -67,14 +68,30 @@ export class Terminal extends BaseTerminal {
|
|||
reject(error)
|
||||
})
|
||||
|
||||
// For compound commands or when shell integration is not ready,
|
||||
// we need to ensure shell integration is fully attached
|
||||
const isCompoundCommand = Terminal.isCompoundCommand(command)
|
||||
const needsIntegrationWait = isCompoundCommand && !this.shellIntegrationReady
|
||||
|
||||
// Wait for shell integration before executing the command
|
||||
pWaitFor(() => this.terminal.shellIntegration !== undefined, {
|
||||
timeout: Terminal.getShellIntegrationTimeout(),
|
||||
})
|
||||
.then(() => {
|
||||
.then(async () => {
|
||||
// Clean up temporary directory if shell integration is available, zsh did its job:
|
||||
ShellIntegrationManager.zshCleanupTmpDir(this.id)
|
||||
|
||||
// For compound commands on newly spawned terminals, add a small delay
|
||||
// to ensure shell integration is fully attached after the terminal is created
|
||||
if (needsIntegrationWait) {
|
||||
console.info(
|
||||
`[Terminal ${this.id}] Compound command detected on new terminal, ensuring shell integration is ready`,
|
||||
)
|
||||
await new Promise((resolve) => setTimeout(resolve, 100))
|
||||
}
|
||||
|
||||
this.shellIntegrationReady = true
|
||||
|
||||
// Run the command in the terminal
|
||||
process.run(command)
|
||||
})
|
||||
|
|
@ -190,4 +207,44 @@ export class Terminal extends BaseTerminal {
|
|||
|
||||
return env
|
||||
}
|
||||
|
||||
/**
|
||||
* Detects if a command is a compound command (contains operators like &&, ||, ;, |, &)
|
||||
* @param command The command to check
|
||||
* @returns True if the command contains compound operators
|
||||
*/
|
||||
public static isCompoundCommand(command: string): boolean {
|
||||
// Check for common shell operators that create compound commands
|
||||
// These operators can cause multiple processes to be spawned
|
||||
const compoundOperators = [
|
||||
"&&", // AND operator
|
||||
"||", // OR operator
|
||||
";", // Sequential operator
|
||||
"|", // Pipe operator
|
||||
"&", // Background operator (at end of command)
|
||||
]
|
||||
|
||||
// Check if command contains any compound operators
|
||||
// Be careful with pipe operator to avoid false positives in strings
|
||||
for (const operator of compoundOperators) {
|
||||
if (operator === "&") {
|
||||
// Check for background operator at the end (not &&)
|
||||
if (command.trimEnd().endsWith("&") && !command.trimEnd().endsWith("&&")) {
|
||||
return true
|
||||
}
|
||||
} else if (operator === "|") {
|
||||
// Check for pipe operator (not ||)
|
||||
const pipeRegex = /(?<![|])\|(?![|])/
|
||||
if (pipeRegex.test(command)) {
|
||||
return true
|
||||
}
|
||||
} else {
|
||||
if (command.includes(operator)) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -95,10 +95,26 @@ export class TerminalRegistry {
|
|||
}
|
||||
|
||||
if (!terminal.running) {
|
||||
console.error(
|
||||
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
|
||||
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
|
||||
)
|
||||
// This can happen with compound commands where shell integration
|
||||
// attaches after the first command completes
|
||||
const isCompoundCmd = process?.command && this.isCompoundCommand(process.command)
|
||||
|
||||
if (isCompoundCmd) {
|
||||
console.warn(
|
||||
"[TerminalRegistry] Shell execution end event received for compound command before terminal marked as running (race condition):",
|
||||
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
|
||||
)
|
||||
|
||||
// If we have a process, complete it to prevent hanging
|
||||
if (process) {
|
||||
terminal.shellExecutionComplete(exitDetails)
|
||||
}
|
||||
} else {
|
||||
console.error(
|
||||
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
|
||||
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
|
||||
)
|
||||
}
|
||||
|
||||
terminal.busy = false
|
||||
return
|
||||
|
|
@ -325,4 +341,28 @@ export class TerminalRegistry {
|
|||
ShellIntegrationManager.zshCleanupTmpDir(id)
|
||||
this.terminals = this.terminals.filter((t) => t.id !== id)
|
||||
}
|
||||
|
||||
/**
|
||||
* Detects if a command is a compound command (contains operators like &&, ||, ;, |, &)
|
||||
* @param command The command to check
|
||||
* @returns True if the command contains compound operators
|
||||
*/
|
||||
private static isCompoundCommand(command: string): boolean {
|
||||
// Check for common shell operators that create compound commands
|
||||
const compoundOperators = ["&&", "||", ";", "|"]
|
||||
|
||||
// Also check for background operator at the end
|
||||
if (command.trimEnd().endsWith("&") && !command.trimEnd().endsWith("&&")) {
|
||||
return true
|
||||
}
|
||||
|
||||
return compoundOperators.some((op) => {
|
||||
if (op === "|") {
|
||||
// Check for pipe operator (not ||)
|
||||
const pipeRegex = /(?<![|])\|(?![|])/
|
||||
return pipeRegex.test(command)
|
||||
}
|
||||
return command.includes(op)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
186
src/integrations/terminal/__tests__/Terminal.compound.spec.ts
Normal file
186
src/integrations/terminal/__tests__/Terminal.compound.spec.ts
Normal file
|
|
@ -0,0 +1,186 @@
|
|||
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"
|
||||
import * as vscode from "vscode"
|
||||
|
||||
import { Terminal } from "../Terminal"
|
||||
import { TerminalRegistry } from "../TerminalRegistry"
|
||||
|
||||
describe("Terminal - Compound Command Handling", () => {
|
||||
beforeEach(() => {
|
||||
// Initialize the registry for tests
|
||||
vi.spyOn(TerminalRegistry, "initialize").mockImplementation(() => {})
|
||||
TerminalRegistry.initialize()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
describe("isCompoundCommand", () => {
|
||||
it("should detect && operator", () => {
|
||||
expect(Terminal.isCompoundCommand("cd /tmp && ls")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("echo hello && echo world")).toBe(true)
|
||||
})
|
||||
|
||||
it("should detect || operator", () => {
|
||||
expect(Terminal.isCompoundCommand("cd /nonexistent || echo 'failed'")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("test -f file.txt || touch file.txt")).toBe(true)
|
||||
})
|
||||
|
||||
it("should detect ; operator", () => {
|
||||
expect(Terminal.isCompoundCommand("cd /tmp; ls")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("echo first; echo second; echo third")).toBe(true)
|
||||
})
|
||||
|
||||
it("should detect | pipe operator", () => {
|
||||
expect(Terminal.isCompoundCommand("ls | grep test")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("cat file.txt | head -10")).toBe(true)
|
||||
})
|
||||
|
||||
it("should detect & background operator", () => {
|
||||
expect(Terminal.isCompoundCommand("npm start &")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("python server.py &")).toBe(true)
|
||||
})
|
||||
|
||||
it("should not detect && in strings or comments", () => {
|
||||
// These are still detected as compound commands because we check for the operator presence
|
||||
// This is intentional to err on the side of caution
|
||||
expect(Terminal.isCompoundCommand('echo "&&"')).toBe(true)
|
||||
})
|
||||
|
||||
it("should not detect single & in the middle of command", () => {
|
||||
expect(Terminal.isCompoundCommand("echo 'this & that'")).toBe(false)
|
||||
expect(Terminal.isCompoundCommand("url?param1=a¶m2=b")).toBe(false)
|
||||
})
|
||||
|
||||
it("should handle complex compound commands", () => {
|
||||
expect(Terminal.isCompoundCommand("cd /tmp && npm install || echo 'failed'")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("test -d dir && (cd dir; make) || mkdir dir")).toBe(true)
|
||||
})
|
||||
|
||||
it("should return false for simple commands", () => {
|
||||
expect(Terminal.isCompoundCommand("ls")).toBe(false)
|
||||
expect(Terminal.isCompoundCommand("cd /tmp")).toBe(false)
|
||||
expect(Terminal.isCompoundCommand("echo hello")).toBe(false)
|
||||
expect(Terminal.isCompoundCommand("npm install")).toBe(false)
|
||||
})
|
||||
|
||||
it("should handle edge cases", () => {
|
||||
expect(Terminal.isCompoundCommand("")).toBe(false)
|
||||
expect(Terminal.isCompoundCommand(" ")).toBe(false)
|
||||
expect(Terminal.isCompoundCommand("&")).toBe(true) // Background operator
|
||||
expect(Terminal.isCompoundCommand("&&")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("||")).toBe(true)
|
||||
expect(Terminal.isCompoundCommand("|")).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("Compound command execution with shell integration", () => {
|
||||
let mockTerminal: any
|
||||
let terminal: Terminal
|
||||
|
||||
beforeEach(() => {
|
||||
// Create a mock VSCode terminal
|
||||
mockTerminal = {
|
||||
shellIntegration: undefined,
|
||||
sendText: vi.fn(),
|
||||
show: vi.fn(),
|
||||
hide: vi.fn(),
|
||||
dispose: vi.fn(),
|
||||
exitStatus: undefined,
|
||||
state: { isInteractedWith: false },
|
||||
creationOptions: {},
|
||||
name: "Test Terminal",
|
||||
processId: Promise.resolve(1234),
|
||||
}
|
||||
|
||||
// Mock vscode.window.createTerminal
|
||||
vi.spyOn(vscode.window, "createTerminal").mockReturnValue(mockTerminal as any)
|
||||
|
||||
// Create a Terminal instance
|
||||
terminal = new Terminal(1, undefined, "/tmp")
|
||||
})
|
||||
|
||||
it("should add delay for compound commands on new terminals", async () => {
|
||||
const command = "cd /tmp && npm test"
|
||||
const callbacks = {
|
||||
onLine: vi.fn(),
|
||||
onCompleted: vi.fn(),
|
||||
onShellExecutionStarted: vi.fn(),
|
||||
onShellExecutionComplete: vi.fn(),
|
||||
onNoShellIntegration: vi.fn(),
|
||||
}
|
||||
|
||||
// Mock shell integration becoming available
|
||||
setTimeout(() => {
|
||||
mockTerminal.shellIntegration = {
|
||||
executeCommand: vi.fn(),
|
||||
cwd: { fsPath: "/tmp" },
|
||||
}
|
||||
}, 50)
|
||||
|
||||
const processPromise = terminal.runCommand(command, callbacks)
|
||||
|
||||
// Wait a bit for the command to be processed
|
||||
await new Promise((resolve) => setTimeout(resolve, 200))
|
||||
|
||||
// Verify that the terminal is marked as busy initially
|
||||
expect(terminal.busy).toBe(true)
|
||||
|
||||
// The shellIntegrationReady flag should be set after the delay
|
||||
expect((terminal as any).shellIntegrationReady).toBe(true)
|
||||
})
|
||||
|
||||
it("should not add delay for simple commands", async () => {
|
||||
const command = "ls -la"
|
||||
const callbacks = {
|
||||
onLine: vi.fn(),
|
||||
onCompleted: vi.fn(),
|
||||
onShellExecutionStarted: vi.fn(),
|
||||
onShellExecutionComplete: vi.fn(),
|
||||
onNoShellIntegration: vi.fn(),
|
||||
}
|
||||
|
||||
// Mock shell integration being immediately available
|
||||
mockTerminal.shellIntegration = {
|
||||
executeCommand: vi.fn(),
|
||||
cwd: { fsPath: "/tmp" },
|
||||
}
|
||||
|
||||
const processPromise = terminal.runCommand(command, callbacks)
|
||||
|
||||
// Wait a bit for the command to be processed
|
||||
await new Promise((resolve) => setTimeout(resolve, 100))
|
||||
|
||||
// Should execute without additional delay
|
||||
expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(command)
|
||||
})
|
||||
|
||||
it("should not add delay for compound commands on terminals with ready shell integration", async () => {
|
||||
const command = "cd /tmp && npm test"
|
||||
const callbacks = {
|
||||
onLine: vi.fn(),
|
||||
onCompleted: vi.fn(),
|
||||
onShellExecutionStarted: vi.fn(),
|
||||
onShellExecutionComplete: vi.fn(),
|
||||
onNoShellIntegration: vi.fn(),
|
||||
}
|
||||
|
||||
// Mock shell integration being immediately available
|
||||
mockTerminal.shellIntegration = {
|
||||
executeCommand: vi.fn(),
|
||||
cwd: { fsPath: "/tmp" },
|
||||
}
|
||||
|
||||
// Mark shell integration as ready (simulating a reused terminal)
|
||||
;(terminal as any).shellIntegrationReady = true
|
||||
|
||||
const processPromise = terminal.runCommand(command, callbacks)
|
||||
|
||||
// Wait a bit for the command to be processed
|
||||
await new Promise((resolve) => setTimeout(resolve, 100))
|
||||
|
||||
// Should execute without additional delay since shellIntegrationReady is true
|
||||
expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(command)
|
||||
})
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue