-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle compound terminal commands by preserving shell context #7431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
37dc97a
9a8634e
7a09564
add8ecb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,19 +16,29 @@ import { Terminal } from "./Terminal" | |
|
|
||
| export class TerminalProcess extends BaseTerminalProcess { | ||
| private terminalRef: WeakRef<Terminal> | ||
| private shellExecutionCompleteCount: number = 0 | ||
| private hasCompleted: boolean = false | ||
|
|
||
| constructor(terminal: Terminal) { | ||
| super() | ||
|
|
||
| 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", "<no shell integration>") | ||
| 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() | ||
| }) | ||
|
|
@@ -72,6 +82,27 @@ export class TerminalProcess extends BaseTerminalProcess { | |
| 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 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) | ||
| } | ||
|
|
||
| private lastExitCode?: number | ||
|
|
||
| /** | ||
| * Executes a command through VSCode's shell integration. | ||
| * Handles both simple and compound commands. | ||
| */ | ||
| private async runSingleCommand(command: string): Promise<string> { | ||
| const terminal = this.terminal.terminal | ||
|
|
||
| // Create a promise that resolves when the stream becomes available | ||
| const streamAvailable = new Promise<AsyncIterable<string>>((resolve, reject) => { | ||
| const timeoutId = setTimeout(() => { | ||
|
|
@@ -101,7 +132,15 @@ export class TerminalProcess extends BaseTerminalProcess { | |
|
|
||
| // Create promise that resolves when shell execution completes for this terminal | ||
| const shellExecutionComplete = new Promise<ExitCodeDetails>((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 | ||
|
|
@@ -127,9 +166,9 @@ export class TerminalProcess extends BaseTerminalProcess { | |
| commandToExecute += ` ; start-sleep -milliseconds ${Terminal.getCommandDelay()}` | ||
| } | ||
|
|
||
| terminal.shellIntegration.executeCommand(commandToExecute) | ||
| terminal.shellIntegration!.executeCommand(commandToExecute) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using non-null assertion here, but at line 53 we properly check for null. Could we add a guard check or at least a comment explaining why we're certain is non-null at this point? The check at line 53 suggests it could be null. |
||
| } else { | ||
| terminal.shellIntegration.executeCommand(command) | ||
| terminal.shellIntegration!.executeCommand(command) | ||
| } | ||
|
|
||
| this.isHot = true | ||
|
|
@@ -153,7 +192,7 @@ export class TerminalProcess extends BaseTerminalProcess { | |
|
|
||
| // Emit continue event to allow execution to proceed | ||
| this.emit("continue") | ||
| return | ||
| return "" | ||
| } | ||
|
|
||
| let preOutput = "" | ||
|
|
@@ -209,7 +248,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 +277,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 +293,10 @@ 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() | ||
|
|
||
| this.emit("completed", this.removeEscapeSequences(this.fullOutput)) | ||
| this.emit("continue") | ||
| return this.removeEscapeSequences(this.fullOutput) | ||
| } | ||
|
|
||
| public override continue() { | ||
|
|
@@ -464,4 +506,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 | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -165,6 +165,97 @@ 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) | ||
| }) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the tests cover the happy path well, there's no test for what happens if the deprecated method is accidentally called. Should we add a test to ensure it throws the expected error, or is the method going to be removed? |
||
|
|
||
| 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", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is set at line 254 but never read anywhere. Is this intended for future use, or can it be removed? If it's for future use, maybe add a comment explaining the planned usage?