From 37dc97a5c3df1d83016fb9230a5f424931d07809 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 26 Aug 2025 20:58:36 +0000 Subject: [PATCH 1/4] fix: handle compound terminal commands by sequential execution - Add commandParser module to detect and split compound commands - Parse commands with &&, ||, ;, and | operators - Execute compound command segments sequentially with proper operator logic - Respect exit codes for && and || operators - Add comprehensive tests for command parsing logic Fixes #7430 --- src/integrations/terminal/TerminalProcess.ts | 97 +++++++- .../terminal/__tests__/commandParser.spec.ts | 230 ++++++++++++++++++ src/integrations/terminal/commandParser.ts | 187 ++++++++++++++ 3 files changed, 509 insertions(+), 5 deletions(-) create mode 100644 src/integrations/terminal/__tests__/commandParser.spec.ts create mode 100644 src/integrations/terminal/commandParser.ts diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index eb0424fe8d..da054b8920 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -13,6 +13,7 @@ import { inspect } from "util" import type { ExitCodeDetails } from "./types" import { BaseTerminalProcess } from "./BaseTerminalProcess" import { Terminal } from "./Terminal" +import { parseCommand, CommandSegment } from "./commandParser" export class TerminalProcess extends BaseTerminalProcess { private terminalRef: WeakRef @@ -72,6 +73,83 @@ export class TerminalProcess extends BaseTerminalProcess { return } + // Parse the command to check for compound operators + const parsedCommand = parseCommand(command) + + if (parsedCommand.isCompound) { + console.info(`[TerminalProcess] Detected compound command with ${parsedCommand.segments.length} segments`) + await this.runCompoundCommand(parsedCommand.segments) + return + } + + // Execute single command as before + await this.runSingleCommand(command) + } + + /** + * Executes a compound command by running each segment sequentially + * based on the operator logic (&&, ||, ;, |) + */ + private async runCompoundCommand(segments: CommandSegment[]) { + let lastExitCode = 0 + let accumulatedOutput = "" + let finalExitCode = 0 + + for (let i = 0; i < segments.length; i++) { + const segment = segments[i] + + // Check if this segment should execute based on previous exit code + if (i > 0 && !segment.shouldExecute(lastExitCode)) { + console.info(`[TerminalProcess] Skipping segment "${segment.command}" due to operator logic`) + continue + } + + console.info( + `[TerminalProcess] Executing compound segment ${i + 1}/${segments.length}: "${segment.command}"`, + ) + + // For pipe operators, we need special handling (future enhancement) + if (segment.operator === "|" && i < segments.length - 1) { + // For now, we'll execute pipes as a single command + // This is a limitation we can document + const pipeCommand = segments + .slice(i) + .map((s) => s.command) + .join(" | ") + await this.runSingleCommand(pipeCommand) + return + } + + // Execute the segment + const segmentOutput = await this.runSingleCommand(segment.command, i === 0) + + if (segmentOutput) { + if (accumulatedOutput && !accumulatedOutput.endsWith("\n")) { + accumulatedOutput += "\n" + } + accumulatedOutput += segmentOutput + } + + // Get the exit code from the last execution + lastExitCode = this.lastExitCode ?? 0 + finalExitCode = lastExitCode + } + + // Emit the final accumulated output + this.fullOutput = accumulatedOutput + this.emit("completed", this.removeEscapeSequences(accumulatedOutput)) + this.emit("shell_execution_complete", { exitCode: finalExitCode }) + this.emit("continue") + } + + private lastExitCode?: number + + /** + * Executes a single command (extracted from the original run method) + */ + private async runSingleCommand(command: string, emitStarted: boolean = true): Promise { + const terminal = this.terminal.terminal + // Create a promise that resolves when the stream becomes available const streamAvailable = new Promise>((resolve, reject) => { const timeoutId = setTimeout(() => { @@ -127,9 +205,9 @@ export class TerminalProcess extends BaseTerminalProcess { commandToExecute += ` ; start-sleep -milliseconds ${Terminal.getCommandDelay()}` } - terminal.shellIntegration.executeCommand(commandToExecute) + terminal.shellIntegration!.executeCommand(commandToExecute) } else { - terminal.shellIntegration.executeCommand(command) + terminal.shellIntegration!.executeCommand(command) } this.isHot = true @@ -153,7 +231,7 @@ export class TerminalProcess extends BaseTerminalProcess { // Emit continue event to allow execution to proceed this.emit("continue") - return + return "" } let preOutput = "" @@ -209,7 +287,8 @@ export class TerminalProcess extends BaseTerminalProcess { this.terminal.setActiveStream(undefined) // Wait for shell execution to complete. - await shellExecutionComplete + const exitDetails = await shellExecutionComplete + this.lastExitCode = exitDetails.exitCode this.isHot = false @@ -237,7 +316,7 @@ export class TerminalProcess extends BaseTerminalProcess { this.continue() // Return early since we can't process output without shell integration markers - return + return "" } // fullOutput begins after C marker so we only need to trim off D marker @@ -253,8 +332,16 @@ export class TerminalProcess extends BaseTerminalProcess { // command is finished, we still want to consider it 'hot' in case // so that api request stalls to let diagnostics catch up"). this.stopHotTimer() + + // For compound commands, we handle completion differently + if (!emitStarted) { + // Return output without emitting completion (handled by runCompoundCommand) + return this.removeEscapeSequences(this.fullOutput) + } + this.emit("completed", this.removeEscapeSequences(this.fullOutput)) this.emit("continue") + return this.removeEscapeSequences(this.fullOutput) } public override continue() { diff --git a/src/integrations/terminal/__tests__/commandParser.spec.ts b/src/integrations/terminal/__tests__/commandParser.spec.ts new file mode 100644 index 0000000000..6a82781521 --- /dev/null +++ b/src/integrations/terminal/__tests__/commandParser.spec.ts @@ -0,0 +1,230 @@ +import { describe, it, expect } from "vitest" +import { parseCommand, isCompoundCommand, splitCompoundCommand } from "../commandParser" + +describe("commandParser", () => { + describe("parseCommand", () => { + it("should identify simple commands as non-compound", () => { + const result = parseCommand("ls -la") + expect(result.isCompound).toBe(false) + expect(result.segments).toHaveLength(1) + expect(result.segments[0].command).toBe("ls -la") + expect(result.segments[0].operator).toBeUndefined() + }) + + it("should parse && operator correctly", () => { + const result = parseCommand("cd foo && npm test") + expect(result.isCompound).toBe(true) + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe("cd foo") + expect(result.segments[0].operator).toBe("&&") + expect(result.segments[1].command).toBe("npm test") + expect(result.segments[1].operator).toBeUndefined() + }) + + it("should parse || operator correctly", () => { + const result = parseCommand("npm test || echo 'Tests failed'") + expect(result.isCompound).toBe(true) + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe("npm test") + expect(result.segments[0].operator).toBe("||") + expect(result.segments[1].command).toBe("echo 'Tests failed'") + }) + + it("should parse semicolon operator correctly", () => { + const result = parseCommand("echo 'Starting'; npm test; echo 'Done'") + expect(result.isCompound).toBe(true) + expect(result.segments).toHaveLength(3) + expect(result.segments[0].command).toBe("echo 'Starting'") + expect(result.segments[0].operator).toBe(";") + expect(result.segments[1].command).toBe("npm test") + expect(result.segments[1].operator).toBe(";") + expect(result.segments[2].command).toBe("echo 'Done'") + expect(result.segments[2].operator).toBeUndefined() + }) + + it("should parse pipe operator correctly", () => { + const result = parseCommand("ls -la | grep test") + expect(result.isCompound).toBe(true) + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe("ls -la") + expect(result.segments[0].operator).toBe("|") + expect(result.segments[1].command).toBe("grep test") + }) + + it("should handle mixed operators", () => { + const result = parseCommand("cd app && npm install || echo 'Install failed'; npm test") + expect(result.isCompound).toBe(true) + expect(result.segments).toHaveLength(4) + expect(result.segments[0].operator).toBe("&&") + expect(result.segments[1].operator).toBe("||") + expect(result.segments[2].operator).toBe(";") + expect(result.segments[3].operator).toBeUndefined() + }) + + it("should respect single quotes", () => { + const result = parseCommand("echo 'test && test'") + expect(result.isCompound).toBe(false) + expect(result.segments).toHaveLength(1) + expect(result.segments[0].command).toBe("echo 'test && test'") + }) + + it("should respect double quotes", () => { + const result = parseCommand('echo "test || test"') + expect(result.isCompound).toBe(false) + expect(result.segments).toHaveLength(1) + expect(result.segments[0].command).toBe('echo "test || test"') + }) + + it("should handle escaped characters", () => { + const result = parseCommand("echo test \\&& echo test2") + expect(result.isCompound).toBe(false) + expect(result.segments).toHaveLength(1) + expect(result.segments[0].command).toBe("echo test \\&& echo test2") + }) + + it("should handle complex quoted strings", () => { + const result = parseCommand(`echo "It's a test" && echo 'He said "hello"'`) + expect(result.isCompound).toBe(true) + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe(`echo "It's a test"`) + expect(result.segments[1].command).toBe(`echo 'He said "hello"'`) + }) + + it("should handle empty segments gracefully", () => { + const result = parseCommand("&& npm test") + expect(result.segments).toHaveLength(1) + expect(result.segments[0].command).toBe("npm test") + }) + + it("should handle trailing operators", () => { + const result = parseCommand("npm test &&") + expect(result.segments).toHaveLength(1) + expect(result.segments[0].command).toBe("npm test") + expect(result.segments[0].operator).toBe("&&") + }) + }) + + describe("isCompoundCommand", () => { + it("should return false for simple commands", () => { + expect(isCompoundCommand("ls -la")).toBe(false) + expect(isCompoundCommand("npm test")).toBe(false) + expect(isCompoundCommand("echo 'test && test'")).toBe(false) + }) + + it("should return true for compound commands", () => { + expect(isCompoundCommand("cd foo && npm test")).toBe(true) + expect(isCompoundCommand("npm test || echo failed")).toBe(true) + expect(isCompoundCommand("echo start; npm test")).toBe(true) + expect(isCompoundCommand("ls | grep test")).toBe(true) + }) + }) + + describe("splitCompoundCommand", () => { + it("should return single segment for simple commands", () => { + const segments = splitCompoundCommand("npm test") + expect(segments).toHaveLength(1) + expect(segments[0].command).toBe("npm test") + }) + + it("should split compound commands correctly", () => { + const segments = splitCompoundCommand("cd app && npm install && npm test") + expect(segments).toHaveLength(3) + expect(segments[0].command).toBe("cd app") + expect(segments[1].command).toBe("npm install") + expect(segments[2].command).toBe("npm test") + }) + }) + + describe("CommandSegment.shouldExecute", () => { + it("should handle && operator logic correctly", () => { + const result = parseCommand("cmd1 && cmd2") + const segment2 = result.segments[1] + + // Should execute if previous command succeeded (exit code 0) + expect(segment2.shouldExecute(0)).toBe(true) + // Should not execute if previous command failed + expect(segment2.shouldExecute(1)).toBe(false) + expect(segment2.shouldExecute(127)).toBe(false) + }) + + it("should handle || operator logic correctly", () => { + const result = parseCommand("cmd1 || cmd2") + const segment2 = result.segments[1] + + // Should not execute if previous command succeeded + expect(segment2.shouldExecute(0)).toBe(false) + // Should execute if previous command failed + expect(segment2.shouldExecute(1)).toBe(true) + expect(segment2.shouldExecute(127)).toBe(true) + }) + + it("should handle ; operator logic correctly", () => { + const result = parseCommand("cmd1; cmd2") + const segment2 = result.segments[1] + + // Should always execute regardless of previous exit code + expect(segment2.shouldExecute(0)).toBe(true) + expect(segment2.shouldExecute(1)).toBe(true) + expect(segment2.shouldExecute(127)).toBe(true) + }) + + it("should handle | operator logic correctly", () => { + const result = parseCommand("cmd1 | cmd2") + const segment2 = result.segments[1] + + // Should always execute (pipes always run) + expect(segment2.shouldExecute(0)).toBe(true) + expect(segment2.shouldExecute(1)).toBe(true) + }) + + it("should handle first segment correctly", () => { + const result = parseCommand("cmd1 && cmd2 || cmd3") + const segment1 = result.segments[0] + + // First segment should always execute + expect(segment1.shouldExecute(0)).toBe(true) + expect(segment1.shouldExecute(1)).toBe(true) + }) + }) + + describe("edge cases", () => { + it("should handle multiple spaces between commands and operators", () => { + const result = parseCommand("cmd1 && cmd2") + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe("cmd1") + expect(result.segments[1].command).toBe("cmd2") + }) + + it("should handle tabs and other whitespace", () => { + const result = parseCommand("cmd1\t&&\tcmd2") + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe("cmd1") + expect(result.segments[1].command).toBe("cmd2") + }) + + it("should handle complex real-world commands", () => { + const cmd = "git add . && git commit -m 'feat: add feature' && git push origin main || echo 'Push failed'" + const result = parseCommand(cmd) + expect(result.isCompound).toBe(true) + expect(result.segments).toHaveLength(4) + expect(result.segments[0].command).toBe("git add .") + expect(result.segments[1].command).toBe("git commit -m 'feat: add feature'") + expect(result.segments[2].command).toBe("git push origin main") + expect(result.segments[3].command).toBe("echo 'Push failed'") + }) + + it("should handle commands with redirections", () => { + const result = parseCommand("echo test > file.txt && cat file.txt") + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe("echo test > file.txt") + expect(result.segments[1].command).toBe("cat file.txt") + }) + + it("should handle commands with environment variables", () => { + const result = parseCommand("NODE_ENV=test npm test && echo $NODE_ENV") + expect(result.segments).toHaveLength(2) + expect(result.segments[0].command).toBe("NODE_ENV=test npm test") + expect(result.segments[1].command).toBe("echo $NODE_ENV") + }) + }) +}) diff --git a/src/integrations/terminal/commandParser.ts b/src/integrations/terminal/commandParser.ts new file mode 100644 index 0000000000..61c702c1d0 --- /dev/null +++ b/src/integrations/terminal/commandParser.ts @@ -0,0 +1,187 @@ +/** + * Utility module for parsing and handling compound shell commands. + * Detects and splits commands with operators like &&, ||, ;, and | + * to enable sequential execution and proper process tracking. + */ + +export interface ParsedCommand { + /** The original full command string */ + original: string + /** Whether this is a compound command with operators */ + isCompound: boolean + /** Individual command segments if compound, otherwise array with single command */ + segments: CommandSegment[] +} + +export interface CommandSegment { + /** The command text */ + command: string + /** The operator that follows this command (if any) */ + operator?: "&&" | "||" | ";" | "|" + /** Whether execution should continue based on previous exit code */ + shouldExecute: (previousExitCode: number) => boolean +} + +/** + * Parses a command string to detect compound command operators. + * Handles &&, ||, ;, and | operators while respecting quotes and escapes. + * + * @param command The command string to parse + * @returns Parsed command information + */ +export function parseCommand(command: string): ParsedCommand { + const segments: CommandSegment[] = [] + let current = "" + let inSingleQuote = false + let inDoubleQuote = false + let escaped = false + let i = 0 + + while (i < command.length) { + const char = command[i] + const nextChar = command[i + 1] + + // Handle escape sequences + if (escaped) { + current += char + escaped = false + i++ + continue + } + + if (char === "\\" && !inSingleQuote) { + escaped = true + current += char + i++ + continue + } + + // Handle quotes + if (char === "'" && !inDoubleQuote) { + inSingleQuote = !inSingleQuote + current += char + i++ + continue + } + + if (char === '"' && !inSingleQuote) { + inDoubleQuote = !inDoubleQuote + current += char + i++ + continue + } + + // If we're inside quotes, just add the character + if (inSingleQuote || inDoubleQuote) { + current += char + i++ + continue + } + + // Check for operators (only outside quotes) + let operator: CommandSegment["operator"] | undefined + let operatorLength = 0 + + if (char === "&" && nextChar === "&") { + operator = "&&" + operatorLength = 2 + } else if (char === "|" && nextChar === "|") { + operator = "||" + operatorLength = 2 + } else if (char === ";") { + operator = ";" + operatorLength = 1 + } else if (char === "|" && nextChar !== "|") { + operator = "|" + operatorLength = 1 + } + + if (operator) { + // Found an operator, save current segment + const trimmedCommand = current.trim() + if (trimmedCommand) { + const segment = createSegment(trimmedCommand, operator) + segments.push(segment) + } + current = "" + i += operatorLength + } else { + current += char + i++ + } + } + + // Add the last segment + const trimmedCommand = current.trim() + if (trimmedCommand) { + segments.push(createSegment(trimmedCommand, undefined)) + } + + // Now set the shouldExecute logic based on the PREVIOUS segment's operator + for (let i = 1; i < segments.length; i++) { + const prevOperator = segments[i - 1].operator + + switch (prevOperator) { + case "&&": + // Execute only if previous command succeeded (exit code 0) + segments[i].shouldExecute = (exitCode) => exitCode === 0 + break + case "||": + // Execute only if previous command failed (non-zero exit code) + segments[i].shouldExecute = (exitCode) => exitCode !== 0 + break + case ";": + case "|": + default: + // Always execute regardless of previous exit code + segments[i].shouldExecute = () => true + break + } + } + + // If we only have one segment with no operator, it's not compound + const isCompound = segments.length > 1 || (segments.length === 1 && segments[0].operator !== undefined) + + return { + original: command, + isCompound, + segments, + } +} + +/** + * Creates a command segment with appropriate execution logic based on operator + */ +function createSegment(command: string, operator: CommandSegment["operator"]): CommandSegment { + // The shouldExecute function determines if THIS segment should execute + // based on the PREVIOUS segment's exit code and the PREVIOUS segment's operator + // By default, segments always execute (first segment or after semicolon) + let shouldExecute: (previousExitCode: number) => boolean = () => true + + return { + command, + operator, + shouldExecute, + } +} + +/** + * Checks if a command contains compound operators that would spawn multiple processes + * + * @param command The command to check + * @returns True if the command contains compound operators + */ +export function isCompoundCommand(command: string): boolean { + return parseCommand(command).isCompound +} + +/** + * Splits a compound command into individual commands for sequential execution. + * Preserves the execution logic of operators like && and ||. + * + * @param command The compound command to split + * @returns Array of individual commands with execution conditions + */ +export function splitCompoundCommand(command: string): CommandSegment[] { + return parseCommand(command).segments +} From 9a8634e1ce500ce0ab417b8d71632cb73b372122 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 26 Aug 2025 21:06:22 +0000 Subject: [PATCH 2/4] refactor: simplify compound command handling to preserve shell context - Execute compound commands as single shell commands instead of splitting - Preserves shell context between command segments (e.g., cd affects subsequent commands) - Deprecate the segment-splitting approach in favor of simpler solution - Add comprehensive tests for compound command behavior This approach is simpler and more reliable as it lets the shell naturally handle operator logic and state preservation. --- src/integrations/terminal/TerminalProcess.ts | 79 +++---------- .../__tests__/TerminalProcess.spec.ts | 109 ++++++++++++++++++ 2 files changed, 127 insertions(+), 61 deletions(-) diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index da054b8920..cb2b117f59 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -78,7 +78,10 @@ export class TerminalProcess extends BaseTerminalProcess { if (parsedCommand.isCompound) { console.info(`[TerminalProcess] Detected compound command with ${parsedCommand.segments.length} segments`) - await this.runCompoundCommand(parsedCommand.segments) + console.info(`[TerminalProcess] Executing compound command as a single shell command to preserve context`) + // Execute compound commands as a single command to preserve shell context + // This ensures operators like && and || work correctly and state is maintained + await this.runSingleCommand(command) return } @@ -87,67 +90,27 @@ export class TerminalProcess extends BaseTerminalProcess { } /** - * Executes a compound command by running each segment sequentially - * based on the operator logic (&&, ||, ;, |) + * @deprecated This method is no longer used. Compound commands are now executed + * as a single shell command to preserve context and proper operator behavior. + * Keeping for reference only. + * + * Previously attempted to execute compound commands by splitting them into segments, + * but this approach lost shell context between commands (e.g., cd wouldn't affect + * subsequent commands). */ - private async runCompoundCommand(segments: CommandSegment[]) { - let lastExitCode = 0 - let accumulatedOutput = "" - let finalExitCode = 0 - - for (let i = 0; i < segments.length; i++) { - const segment = segments[i] - - // Check if this segment should execute based on previous exit code - if (i > 0 && !segment.shouldExecute(lastExitCode)) { - console.info(`[TerminalProcess] Skipping segment "${segment.command}" due to operator logic`) - continue - } - - console.info( - `[TerminalProcess] Executing compound segment ${i + 1}/${segments.length}: "${segment.command}"`, - ) - - // For pipe operators, we need special handling (future enhancement) - if (segment.operator === "|" && i < segments.length - 1) { - // For now, we'll execute pipes as a single command - // This is a limitation we can document - const pipeCommand = segments - .slice(i) - .map((s) => s.command) - .join(" | ") - await this.runSingleCommand(pipeCommand) - return - } - - // Execute the segment - const segmentOutput = await this.runSingleCommand(segment.command, i === 0) - - if (segmentOutput) { - if (accumulatedOutput && !accumulatedOutput.endsWith("\n")) { - accumulatedOutput += "\n" - } - accumulatedOutput += segmentOutput - } - - // Get the exit code from the last execution - lastExitCode = this.lastExitCode ?? 0 - finalExitCode = lastExitCode - } - - // Emit the final accumulated output - this.fullOutput = accumulatedOutput - this.emit("completed", this.removeEscapeSequences(accumulatedOutput)) - this.emit("shell_execution_complete", { exitCode: finalExitCode }) - this.emit("continue") + private async runCompoundCommand_DEPRECATED(segments: CommandSegment[]) { + // This method is intentionally left empty and deprecated + // Compound commands are now handled by executing them as a single command + throw new Error("runCompoundCommand is deprecated. Compound commands should be executed as a single command.") } private lastExitCode?: number /** - * Executes a single command (extracted from the original run method) + * Executes a command through VSCode's shell integration. + * Handles both simple and compound commands. */ - private async runSingleCommand(command: string, emitStarted: boolean = true): Promise { + private async runSingleCommand(command: string): Promise { const terminal = this.terminal.terminal // Create a promise that resolves when the stream becomes available @@ -333,12 +296,6 @@ export class TerminalProcess extends BaseTerminalProcess { // so that api request stalls to let diagnostics catch up"). this.stopHotTimer() - // For compound commands, we handle completion differently - if (!emitStarted) { - // Return output without emitting completion (handled by runCompoundCommand) - return this.removeEscapeSequences(this.fullOutput) - } - this.emit("completed", this.removeEscapeSequences(this.fullOutput)) this.emit("continue") return this.removeEscapeSequences(this.fullOutput) diff --git a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts index 04c31bd93a..80f6461548 100644 --- a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts +++ b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts @@ -6,6 +6,7 @@ import { mergePromise } from "../mergePromise" import { TerminalProcess } from "../TerminalProcess" import { Terminal } from "../Terminal" import { TerminalRegistry } from "../TerminalRegistry" +import { parseCommand } from "../commandParser" vi.mock("execa", () => ({ execa: vi.fn(), @@ -165,6 +166,114 @@ describe("TerminalProcess", () => { await completePromise expect(terminalProcess.isHot).toBe(false) }) + + it("handles compound commands as single command", async () => { + // Test that compound commands are executed as a single command + // to preserve shell context between segments + const compoundCommand = "cd foo && npm test" + + let executedCommand = "" + + mockStream = (async function* () { + yield "\x1b]633;C\x07" + yield "Changed directory to foo\n" + yield "Running tests...\n" + yield "Tests passed" + yield "\x1b]633;D\x07" + terminalProcess.emit("shell_execution_complete", { exitCode: 0 }) + })() + + mockTerminal.shellIntegration.executeCommand.mockImplementation((cmd: string) => { + executedCommand = cmd + return { + read: vi.fn().mockReturnValue(mockStream), + } + }) + + const runPromise = terminalProcess.run(compoundCommand) + + // Emit stream available with the mock stream + terminalProcess.emit("stream_available", mockStream) + + await runPromise + + // Verify the compound command was executed as a single command + expect(executedCommand).toBe(compoundCommand) + expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledTimes(1) + expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(compoundCommand) + }) + + it("detects compound commands correctly", async () => { + // Test various compound command patterns + const testCases = [ + { command: "cd foo && npm test", isCompound: true }, + { command: "npm test || echo 'Tests failed'", isCompound: true }, + { command: "echo start; npm test; echo done", isCompound: true }, + { command: "ls -la | grep test", isCompound: true }, + { command: "npm test", isCompound: false }, + { command: "echo 'test && test'", isCompound: false }, // Quoted operators shouldn't be detected + ] + + for (const testCase of testCases) { + const parsed = parseCommand(testCase.command) + expect(parsed.isCompound).toBe(testCase.isCompound) + } + }) + + it("preserves shell context in compound commands", async () => { + // This test verifies that compound commands maintain context + // For example, cd in the first part affects the second part + const command = "cd /tmp && pwd" + + mockStream = (async function* () { + yield "\x1b]633;C\x07" + yield "/tmp\n" // pwd should output /tmp since cd was in same shell context + yield "\x1b]633;D\x07" + terminalProcess.emit("shell_execution_complete", { exitCode: 0 }) + })() + + mockTerminal.shellIntegration.executeCommand.mockReturnValue({ + read: vi.fn().mockReturnValue(mockStream), + }) + + let output = "" + terminalProcess.on("completed", (result) => { + output = result || "" + }) + + const runPromise = terminalProcess.run(command) + terminalProcess.emit("stream_available", mockStream) + await runPromise + + expect(output.trim()).toBe("/tmp") + // Verify it was executed as a single command + expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(command) + }) + + it("handles complex compound commands with multiple operators", async () => { + const complexCommand = "git add . && git commit -m 'test' && git push || echo 'Push failed'" + + mockStream = (async function* () { + yield "\x1b]633;C\x07" + yield "Files added\n" + yield "Committed\n" + yield "Pushed successfully\n" + yield "\x1b]633;D\x07" + terminalProcess.emit("shell_execution_complete", { exitCode: 0 }) + })() + + mockTerminal.shellIntegration.executeCommand.mockReturnValue({ + read: vi.fn().mockReturnValue(mockStream), + }) + + const runPromise = terminalProcess.run(complexCommand) + terminalProcess.emit("stream_available", mockStream) + await runPromise + + // Should execute as single command + expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledTimes(1) + expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(complexCommand) + }) }) describe("continue", () => { From 7a095647edcb9d3193cef8d04f4b0fd4be6d6bce Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 26 Aug 2025 21:26:59 +0000 Subject: [PATCH 3/4] refactor: remove unused commandParser module and simplify compound command detection - Removed the entire commandParser.ts module (187 lines) and its tests (230 lines) - Moved compound command detection inline into TerminalProcess.ts as a simple private method - Removed deprecated runCompoundCommand_DEPRECATED method - Simplified the approach to just detect and execute compound commands as single shell commands - This eliminates the conflicting dual-approach issue where one commit added complex parsing and the next deprecated it The functionality remains the same - compound commands are still detected and executed as single shell commands to preserve context, but now with a much cleaner and simpler implementation. --- src/integrations/terminal/TerminalProcess.ts | 93 +++++-- .../__tests__/TerminalProcess.spec.ts | 18 -- .../terminal/__tests__/commandParser.spec.ts | 230 ------------------ src/integrations/terminal/commandParser.ts | 187 -------------- 4 files changed, 66 insertions(+), 462 deletions(-) delete mode 100644 src/integrations/terminal/__tests__/commandParser.spec.ts delete mode 100644 src/integrations/terminal/commandParser.ts diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index cb2b117f59..70917ff1e6 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -13,7 +13,6 @@ import { inspect } from "util" import type { ExitCodeDetails } from "./types" import { BaseTerminalProcess } from "./BaseTerminalProcess" import { Terminal } from "./Terminal" -import { parseCommand, CommandSegment } from "./commandParser" export class TerminalProcess extends BaseTerminalProcess { private terminalRef: WeakRef @@ -73,37 +72,18 @@ export class TerminalProcess extends BaseTerminalProcess { return } - // Parse the command to check for compound operators - const parsedCommand = parseCommand(command) - - if (parsedCommand.isCompound) { - console.info(`[TerminalProcess] Detected compound command with ${parsedCommand.segments.length} segments`) - console.info(`[TerminalProcess] Executing compound command as a single shell command to preserve context`) - // Execute compound commands as a single command to preserve shell context - // This ensures operators like && and || work correctly and state is maintained - await this.runSingleCommand(command) - return + // Check if command contains compound operators (&&, ||, ;, |) + if (this.isCompoundCommand(command)) { + console.info( + `[TerminalProcess] Detected compound command, executing as single shell command to preserve context`, + ) } - // Execute single command as before + // Execute all commands (simple or compound) through shell integration + // This preserves shell context for compound commands (e.g., cd affects subsequent commands) await this.runSingleCommand(command) } - /** - * @deprecated This method is no longer used. Compound commands are now executed - * as a single shell command to preserve context and proper operator behavior. - * Keeping for reference only. - * - * Previously attempted to execute compound commands by splitting them into segments, - * but this approach lost shell context between commands (e.g., cd wouldn't affect - * subsequent commands). - */ - private async runCompoundCommand_DEPRECATED(segments: CommandSegment[]) { - // This method is intentionally left empty and deprecated - // Compound commands are now handled by executing them as a single command - throw new Error("runCompoundCommand is deprecated. Compound commands should be executed as a single command.") - } - private lastExitCode?: number /** @@ -508,4 +488,63 @@ export class TerminalProcess extends BaseTerminalProcess { return match133 !== undefined ? match133 : match633 } + + /** + * Checks if a command contains compound operators (&&, ||, ;, |) + * that would typically spawn multiple processes. + * + * @param command The command string to check + * @returns True if the command contains compound operators + */ + private isCompoundCommand(command: string): boolean { + // Quick check for compound operators outside of quotes + let inSingleQuote = false + let inDoubleQuote = false + let escaped = false + + for (let i = 0; i < command.length; i++) { + const char = command[i] + const nextChar = command[i + 1] + + // Handle escape sequences + if (escaped) { + escaped = false + continue + } + + if (char === "\\" && !inSingleQuote) { + escaped = true + continue + } + + // Handle quotes + if (char === "'" && !inDoubleQuote) { + inSingleQuote = !inSingleQuote + continue + } + + if (char === '"' && !inSingleQuote) { + inDoubleQuote = !inDoubleQuote + continue + } + + // If we're inside quotes, skip operator detection + if (inSingleQuote || inDoubleQuote) { + continue + } + + // Check for operators (only outside quotes) + if (char === "&" && nextChar === "&") { + return true + } else if (char === "|" && nextChar === "|") { + return true + } else if (char === ";") { + return true + } else if (char === "|" && nextChar !== "|") { + return true + } + } + + return false + } } diff --git a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts index 80f6461548..39e444c56a 100644 --- a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts +++ b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts @@ -6,7 +6,6 @@ import { mergePromise } from "../mergePromise" import { TerminalProcess } from "../TerminalProcess" import { Terminal } from "../Terminal" import { TerminalRegistry } from "../TerminalRegistry" -import { parseCommand } from "../commandParser" vi.mock("execa", () => ({ execa: vi.fn(), @@ -203,23 +202,6 @@ describe("TerminalProcess", () => { expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(compoundCommand) }) - it("detects compound commands correctly", async () => { - // Test various compound command patterns - const testCases = [ - { command: "cd foo && npm test", isCompound: true }, - { command: "npm test || echo 'Tests failed'", isCompound: true }, - { command: "echo start; npm test; echo done", isCompound: true }, - { command: "ls -la | grep test", isCompound: true }, - { command: "npm test", isCompound: false }, - { command: "echo 'test && test'", isCompound: false }, // Quoted operators shouldn't be detected - ] - - for (const testCase of testCases) { - const parsed = parseCommand(testCase.command) - expect(parsed.isCompound).toBe(testCase.isCompound) - } - }) - it("preserves shell context in compound commands", async () => { // This test verifies that compound commands maintain context // For example, cd in the first part affects the second part diff --git a/src/integrations/terminal/__tests__/commandParser.spec.ts b/src/integrations/terminal/__tests__/commandParser.spec.ts deleted file mode 100644 index 6a82781521..0000000000 --- a/src/integrations/terminal/__tests__/commandParser.spec.ts +++ /dev/null @@ -1,230 +0,0 @@ -import { describe, it, expect } from "vitest" -import { parseCommand, isCompoundCommand, splitCompoundCommand } from "../commandParser" - -describe("commandParser", () => { - describe("parseCommand", () => { - it("should identify simple commands as non-compound", () => { - const result = parseCommand("ls -la") - expect(result.isCompound).toBe(false) - expect(result.segments).toHaveLength(1) - expect(result.segments[0].command).toBe("ls -la") - expect(result.segments[0].operator).toBeUndefined() - }) - - it("should parse && operator correctly", () => { - const result = parseCommand("cd foo && npm test") - expect(result.isCompound).toBe(true) - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe("cd foo") - expect(result.segments[0].operator).toBe("&&") - expect(result.segments[1].command).toBe("npm test") - expect(result.segments[1].operator).toBeUndefined() - }) - - it("should parse || operator correctly", () => { - const result = parseCommand("npm test || echo 'Tests failed'") - expect(result.isCompound).toBe(true) - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe("npm test") - expect(result.segments[0].operator).toBe("||") - expect(result.segments[1].command).toBe("echo 'Tests failed'") - }) - - it("should parse semicolon operator correctly", () => { - const result = parseCommand("echo 'Starting'; npm test; echo 'Done'") - expect(result.isCompound).toBe(true) - expect(result.segments).toHaveLength(3) - expect(result.segments[0].command).toBe("echo 'Starting'") - expect(result.segments[0].operator).toBe(";") - expect(result.segments[1].command).toBe("npm test") - expect(result.segments[1].operator).toBe(";") - expect(result.segments[2].command).toBe("echo 'Done'") - expect(result.segments[2].operator).toBeUndefined() - }) - - it("should parse pipe operator correctly", () => { - const result = parseCommand("ls -la | grep test") - expect(result.isCompound).toBe(true) - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe("ls -la") - expect(result.segments[0].operator).toBe("|") - expect(result.segments[1].command).toBe("grep test") - }) - - it("should handle mixed operators", () => { - const result = parseCommand("cd app && npm install || echo 'Install failed'; npm test") - expect(result.isCompound).toBe(true) - expect(result.segments).toHaveLength(4) - expect(result.segments[0].operator).toBe("&&") - expect(result.segments[1].operator).toBe("||") - expect(result.segments[2].operator).toBe(";") - expect(result.segments[3].operator).toBeUndefined() - }) - - it("should respect single quotes", () => { - const result = parseCommand("echo 'test && test'") - expect(result.isCompound).toBe(false) - expect(result.segments).toHaveLength(1) - expect(result.segments[0].command).toBe("echo 'test && test'") - }) - - it("should respect double quotes", () => { - const result = parseCommand('echo "test || test"') - expect(result.isCompound).toBe(false) - expect(result.segments).toHaveLength(1) - expect(result.segments[0].command).toBe('echo "test || test"') - }) - - it("should handle escaped characters", () => { - const result = parseCommand("echo test \\&& echo test2") - expect(result.isCompound).toBe(false) - expect(result.segments).toHaveLength(1) - expect(result.segments[0].command).toBe("echo test \\&& echo test2") - }) - - it("should handle complex quoted strings", () => { - const result = parseCommand(`echo "It's a test" && echo 'He said "hello"'`) - expect(result.isCompound).toBe(true) - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe(`echo "It's a test"`) - expect(result.segments[1].command).toBe(`echo 'He said "hello"'`) - }) - - it("should handle empty segments gracefully", () => { - const result = parseCommand("&& npm test") - expect(result.segments).toHaveLength(1) - expect(result.segments[0].command).toBe("npm test") - }) - - it("should handle trailing operators", () => { - const result = parseCommand("npm test &&") - expect(result.segments).toHaveLength(1) - expect(result.segments[0].command).toBe("npm test") - expect(result.segments[0].operator).toBe("&&") - }) - }) - - describe("isCompoundCommand", () => { - it("should return false for simple commands", () => { - expect(isCompoundCommand("ls -la")).toBe(false) - expect(isCompoundCommand("npm test")).toBe(false) - expect(isCompoundCommand("echo 'test && test'")).toBe(false) - }) - - it("should return true for compound commands", () => { - expect(isCompoundCommand("cd foo && npm test")).toBe(true) - expect(isCompoundCommand("npm test || echo failed")).toBe(true) - expect(isCompoundCommand("echo start; npm test")).toBe(true) - expect(isCompoundCommand("ls | grep test")).toBe(true) - }) - }) - - describe("splitCompoundCommand", () => { - it("should return single segment for simple commands", () => { - const segments = splitCompoundCommand("npm test") - expect(segments).toHaveLength(1) - expect(segments[0].command).toBe("npm test") - }) - - it("should split compound commands correctly", () => { - const segments = splitCompoundCommand("cd app && npm install && npm test") - expect(segments).toHaveLength(3) - expect(segments[0].command).toBe("cd app") - expect(segments[1].command).toBe("npm install") - expect(segments[2].command).toBe("npm test") - }) - }) - - describe("CommandSegment.shouldExecute", () => { - it("should handle && operator logic correctly", () => { - const result = parseCommand("cmd1 && cmd2") - const segment2 = result.segments[1] - - // Should execute if previous command succeeded (exit code 0) - expect(segment2.shouldExecute(0)).toBe(true) - // Should not execute if previous command failed - expect(segment2.shouldExecute(1)).toBe(false) - expect(segment2.shouldExecute(127)).toBe(false) - }) - - it("should handle || operator logic correctly", () => { - const result = parseCommand("cmd1 || cmd2") - const segment2 = result.segments[1] - - // Should not execute if previous command succeeded - expect(segment2.shouldExecute(0)).toBe(false) - // Should execute if previous command failed - expect(segment2.shouldExecute(1)).toBe(true) - expect(segment2.shouldExecute(127)).toBe(true) - }) - - it("should handle ; operator logic correctly", () => { - const result = parseCommand("cmd1; cmd2") - const segment2 = result.segments[1] - - // Should always execute regardless of previous exit code - expect(segment2.shouldExecute(0)).toBe(true) - expect(segment2.shouldExecute(1)).toBe(true) - expect(segment2.shouldExecute(127)).toBe(true) - }) - - it("should handle | operator logic correctly", () => { - const result = parseCommand("cmd1 | cmd2") - const segment2 = result.segments[1] - - // Should always execute (pipes always run) - expect(segment2.shouldExecute(0)).toBe(true) - expect(segment2.shouldExecute(1)).toBe(true) - }) - - it("should handle first segment correctly", () => { - const result = parseCommand("cmd1 && cmd2 || cmd3") - const segment1 = result.segments[0] - - // First segment should always execute - expect(segment1.shouldExecute(0)).toBe(true) - expect(segment1.shouldExecute(1)).toBe(true) - }) - }) - - describe("edge cases", () => { - it("should handle multiple spaces between commands and operators", () => { - const result = parseCommand("cmd1 && cmd2") - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe("cmd1") - expect(result.segments[1].command).toBe("cmd2") - }) - - it("should handle tabs and other whitespace", () => { - const result = parseCommand("cmd1\t&&\tcmd2") - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe("cmd1") - expect(result.segments[1].command).toBe("cmd2") - }) - - it("should handle complex real-world commands", () => { - const cmd = "git add . && git commit -m 'feat: add feature' && git push origin main || echo 'Push failed'" - const result = parseCommand(cmd) - expect(result.isCompound).toBe(true) - expect(result.segments).toHaveLength(4) - expect(result.segments[0].command).toBe("git add .") - expect(result.segments[1].command).toBe("git commit -m 'feat: add feature'") - expect(result.segments[2].command).toBe("git push origin main") - expect(result.segments[3].command).toBe("echo 'Push failed'") - }) - - it("should handle commands with redirections", () => { - const result = parseCommand("echo test > file.txt && cat file.txt") - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe("echo test > file.txt") - expect(result.segments[1].command).toBe("cat file.txt") - }) - - it("should handle commands with environment variables", () => { - const result = parseCommand("NODE_ENV=test npm test && echo $NODE_ENV") - expect(result.segments).toHaveLength(2) - expect(result.segments[0].command).toBe("NODE_ENV=test npm test") - expect(result.segments[1].command).toBe("echo $NODE_ENV") - }) - }) -}) diff --git a/src/integrations/terminal/commandParser.ts b/src/integrations/terminal/commandParser.ts deleted file mode 100644 index 61c702c1d0..0000000000 --- a/src/integrations/terminal/commandParser.ts +++ /dev/null @@ -1,187 +0,0 @@ -/** - * Utility module for parsing and handling compound shell commands. - * Detects and splits commands with operators like &&, ||, ;, and | - * to enable sequential execution and proper process tracking. - */ - -export interface ParsedCommand { - /** The original full command string */ - original: string - /** Whether this is a compound command with operators */ - isCompound: boolean - /** Individual command segments if compound, otherwise array with single command */ - segments: CommandSegment[] -} - -export interface CommandSegment { - /** The command text */ - command: string - /** The operator that follows this command (if any) */ - operator?: "&&" | "||" | ";" | "|" - /** Whether execution should continue based on previous exit code */ - shouldExecute: (previousExitCode: number) => boolean -} - -/** - * Parses a command string to detect compound command operators. - * Handles &&, ||, ;, and | operators while respecting quotes and escapes. - * - * @param command The command string to parse - * @returns Parsed command information - */ -export function parseCommand(command: string): ParsedCommand { - const segments: CommandSegment[] = [] - let current = "" - let inSingleQuote = false - let inDoubleQuote = false - let escaped = false - let i = 0 - - while (i < command.length) { - const char = command[i] - const nextChar = command[i + 1] - - // Handle escape sequences - if (escaped) { - current += char - escaped = false - i++ - continue - } - - if (char === "\\" && !inSingleQuote) { - escaped = true - current += char - i++ - continue - } - - // Handle quotes - if (char === "'" && !inDoubleQuote) { - inSingleQuote = !inSingleQuote - current += char - i++ - continue - } - - if (char === '"' && !inSingleQuote) { - inDoubleQuote = !inDoubleQuote - current += char - i++ - continue - } - - // If we're inside quotes, just add the character - if (inSingleQuote || inDoubleQuote) { - current += char - i++ - continue - } - - // Check for operators (only outside quotes) - let operator: CommandSegment["operator"] | undefined - let operatorLength = 0 - - if (char === "&" && nextChar === "&") { - operator = "&&" - operatorLength = 2 - } else if (char === "|" && nextChar === "|") { - operator = "||" - operatorLength = 2 - } else if (char === ";") { - operator = ";" - operatorLength = 1 - } else if (char === "|" && nextChar !== "|") { - operator = "|" - operatorLength = 1 - } - - if (operator) { - // Found an operator, save current segment - const trimmedCommand = current.trim() - if (trimmedCommand) { - const segment = createSegment(trimmedCommand, operator) - segments.push(segment) - } - current = "" - i += operatorLength - } else { - current += char - i++ - } - } - - // Add the last segment - const trimmedCommand = current.trim() - if (trimmedCommand) { - segments.push(createSegment(trimmedCommand, undefined)) - } - - // Now set the shouldExecute logic based on the PREVIOUS segment's operator - for (let i = 1; i < segments.length; i++) { - const prevOperator = segments[i - 1].operator - - switch (prevOperator) { - case "&&": - // Execute only if previous command succeeded (exit code 0) - segments[i].shouldExecute = (exitCode) => exitCode === 0 - break - case "||": - // Execute only if previous command failed (non-zero exit code) - segments[i].shouldExecute = (exitCode) => exitCode !== 0 - break - case ";": - case "|": - default: - // Always execute regardless of previous exit code - segments[i].shouldExecute = () => true - break - } - } - - // If we only have one segment with no operator, it's not compound - const isCompound = segments.length > 1 || (segments.length === 1 && segments[0].operator !== undefined) - - return { - original: command, - isCompound, - segments, - } -} - -/** - * Creates a command segment with appropriate execution logic based on operator - */ -function createSegment(command: string, operator: CommandSegment["operator"]): CommandSegment { - // The shouldExecute function determines if THIS segment should execute - // based on the PREVIOUS segment's exit code and the PREVIOUS segment's operator - // By default, segments always execute (first segment or after semicolon) - let shouldExecute: (previousExitCode: number) => boolean = () => true - - return { - command, - operator, - shouldExecute, - } -} - -/** - * Checks if a command contains compound operators that would spawn multiple processes - * - * @param command The command to check - * @returns True if the command contains compound operators - */ -export function isCompoundCommand(command: string): boolean { - return parseCommand(command).isCompound -} - -/** - * Splits a compound command into individual commands for sequential execution. - * Preserves the execution logic of operators like && and ||. - * - * @param command The compound command to split - * @returns Array of individual commands with execution conditions - */ -export function splitCompoundCommand(command: string): CommandSegment[] { - return parseCommand(command).segments -} From add8ecb860b8f41e10016e90f93f897b1ad09c64 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 26 Aug 2025 22:06:33 +0000 Subject: [PATCH 4/4] fix: improve busy state management for compound terminal commands - Add tracking for duplicate shell_execution_complete events - Prevent race conditions in busy state management by using hasCompleted flag - Move busy state management to TerminalProcess to ensure proper coordination - Fix callback handling in Terminal.ts to ensure busy is always cleared on completion - Remove duplicate busy state clearing in BaseTerminal.shellExecutionComplete This addresses the issue where compound commands could cause the terminal busy state to be incorrectly managed due to duplicate shell_execution_complete events or race conditions between different components. --- src/integrations/terminal/BaseTerminal.ts | 14 +++++++++--- src/integrations/terminal/Terminal.ts | 11 +++++++-- src/integrations/terminal/TerminalProcess.ts | 24 +++++++++++++++++--- 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/integrations/terminal/BaseTerminal.ts b/src/integrations/terminal/BaseTerminal.ts index a79d417b07..4e4c47a557 100644 --- a/src/integrations/terminal/BaseTerminal.ts +++ b/src/integrations/terminal/BaseTerminal.ts @@ -71,17 +71,25 @@ export abstract class BaseTerminal implements RooTerminal { * @param exitDetails The exit details of the shell execution */ public shellExecutionComplete(exitDetails: ExitCodeDetails) { - this.busy = false - this.running = false - + // Only update state if we have an active process + // This prevents duplicate calls from affecting the state if (this.process) { + this.running = false + // Add to the front of the queue (most recent first). if (this.process.hasUnretrievedOutput()) { this.completedProcesses.unshift(this.process) } + // Emit the event before clearing the process reference this.process.emit("shell_execution_complete", exitDetails) + + // Clear the process reference + const completedProcess = this.process this.process = undefined + + // The busy state will be managed by the TerminalProcess itself + // to prevent race conditions with compound commands } } diff --git a/src/integrations/terminal/Terminal.ts b/src/integrations/terminal/Terminal.ts index 8bf2072f3d..24acfc6ffa 100644 --- a/src/integrations/terminal/Terminal.ts +++ b/src/integrations/terminal/Terminal.ts @@ -54,9 +54,16 @@ export class Terminal extends BaseTerminal { // This ensures that we don't miss any events because they are // configured before the process starts. process.on("line", (line) => callbacks.onLine(line, process)) - process.once("completed", (output) => callbacks.onCompleted(output, process)) + process.once("completed", (output) => { + // Ensure busy is set to false when completed + this.busy = false + callbacks.onCompleted(output, process) + }) process.once("shell_execution_started", (pid) => callbacks.onShellExecutionStarted(pid, process)) - process.once("shell_execution_complete", (details) => callbacks.onShellExecutionComplete(details, process)) + process.once("shell_execution_complete", (details) => { + // Note: busy state is managed by TerminalProcess and BaseTerminal + callbacks.onShellExecutionComplete(details, process) + }) process.once("no_shell_integration", (msg) => callbacks.onNoShellIntegration?.(msg, process)) const promise = new Promise((resolve, reject) => { diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index 70917ff1e6..d1d59979e4 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -16,6 +16,8 @@ import { Terminal } from "./Terminal" export class TerminalProcess extends BaseTerminalProcess { private terminalRef: WeakRef + private shellExecutionCompleteCount: number = 0 + private hasCompleted: boolean = false constructor(terminal: Terminal) { super() @@ -23,12 +25,20 @@ export class TerminalProcess extends BaseTerminalProcess { this.terminalRef = new WeakRef(terminal) this.once("completed", () => { - this.terminal.busy = false + // Only set busy to false if not already done + if (!this.hasCompleted) { + this.hasCompleted = true + this.terminal.busy = false + } }) this.once("no_shell_integration", () => { this.emit("completed", "") - this.terminal.busy = false + // Only set busy to false if not already done + if (!this.hasCompleted) { + this.hasCompleted = true + this.terminal.busy = false + } this.terminal.setActiveStream(undefined) this.continue() }) @@ -122,7 +132,15 @@ export class TerminalProcess extends BaseTerminalProcess { // Create promise that resolves when shell execution completes for this terminal const shellExecutionComplete = new Promise((resolve) => { - this.once("shell_execution_complete", (details: ExitCodeDetails) => resolve(details)) + this.once("shell_execution_complete", (details: ExitCodeDetails) => { + this.shellExecutionCompleteCount++ + if (this.shellExecutionCompleteCount > 1) { + console.warn( + `[TerminalProcess] shell_execution_complete fired ${this.shellExecutionCompleteCount} times for command: ${command}`, + ) + } + resolve(details) + }) }) // Execute command