From 9a8634e1ce500ce0ab417b8d71632cb73b372122 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 26 Aug 2025 21:06:22 +0000 Subject: [PATCH] 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", () => {