Skip to content

Commit 387ca89

Browse files
F915F915
committed
fix(terminal): delegate shell detection to vscode.env.shell (#321)
Replace manual VS Code config parsing and allowlist validation in shell.ts with vscode.env.shell, the API VS Code provides for extension shell detection since version 1.37. - shell.ts: remove SHELL_ALLOWLIST (~95 entries) and manual config parsing functions (getWindowsShellFromVSCode, getMacShellFromVSCode, getLinuxShellFromVSCode). getShell() reads vscode.env.shell directly, falling back through os.userInfo(), COMSPEC/SHELL environment variables, and platform defaults. Remove getShellFallbackOccurred(). - Terminal.ts: construct terminal without explicit shellPath when the user has not set execaShellPath or configured WSL, letting VS Code's profile system determine the shell. When execaShellPath is set, pass it explicitly. For WSL, omit shellPath so VS Code uses its WSL profile for shell integration. Start the shell-integration-ready promise in the constructor. - TerminalProcess.ts: remove the getShellFallbackOccurred check that previously guarded against mismatch between the terminal's shell and the detected shell. After the shell.ts refactoring, both paths use the same VS Code resolution. When shell integration is unavailable, emit no_shell_integration without calling sendText first. In the stream processing path, treat missing OSC 633/133 markers as complete output instead of an error. - ExecaTerminalProcess.ts: use BaseTerminal.getExecaShellPath() || getShell() to match Terminal's shell resolution. When the shell is wsl.exe, spawn it directly with array arguments instead of wrapping through a shell. - tests: update shell.spec.ts for the new getShell() fallback chain. Update terminal test files to mock vscode.env.shell instead of stubbing getShell(). Extend vscode.js mock with onDidChangeTerminalShellIntegration event and env.shell. Update TerminalProcess.spec.ts to verify sendText is not called before no_shell_integration (preventing double execution). Update TerminalRegistry.spec.ts to drop the getShell spy and shellPath assertions now that Terminal no longer passes an explicit shellPath by default. Add WSL direct-spawn test in ExecaTerminalProcess.spec.ts. Fixes #321
1 parent 3ed991e commit 387ca89

13 files changed

Lines changed: 394 additions & 728 deletions

‎src/__mocks__/vscode.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ export const window = {
8181
sendText: () => {},
8282
}),
8383
onDidCloseTerminal: () => mockDisposable,
84+
onDidChangeTerminalShellIntegration: () => mockDisposable,
8485
createTextEditorDecorationType: () => ({ dispose: () => {} }),
8586
}
8687

@@ -104,6 +105,7 @@ export const extensions = {
104105

105106
export const env = {
106107
openExternal: () => Promise.resolve(),
108+
shell: "/bin/bash", // vscode.env.shell mock — resolved default shell
107109
}
108110

109111
export const Uri = mockUri

‎src/integrations/terminal/ExecaTerminalProcess.ts‎

Lines changed: 47 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import process from "process"
55
import type { RooTerminal } from "./types"
66
import { BaseTerminal } from "./BaseTerminal"
77
import { BaseTerminalProcess } from "./BaseTerminalProcess"
8-
import { getShell } from "../../utils/shell"
8+
import { getShell, WSL_EXE_PATH } from "../../utils/shell"
99

1010
export class ExecaTerminalProcess extends BaseTerminalProcess {
1111
private terminalRef: WeakRef<RooTerminal>
@@ -40,19 +40,52 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
4040
try {
4141
this.isHot = true
4242

43-
this.subprocess = execa({
44-
shell: BaseTerminal.getExecaShellPath() || getShell(),
45-
cwd: this.terminal.getCurrentWorkingDirectory(),
46-
all: true,
47-
// Ignore stdin to ensure non-interactive mode and prevent hanging
48-
stdin: "ignore",
49-
env: {
50-
...process.env,
51-
// Ensure UTF-8 encoding for Ruby, CocoaPods, etc.
52-
LANG: "en_US.UTF-8",
53-
LC_ALL: "en_US.UTF-8",
54-
},
55-
})`${command}`
43+
const resolvedShell = BaseTerminal.getExecaShellPath() || getShell()
44+
const isWslShell = resolvedShell === WSL_EXE_PATH
45+
46+
let effectiveShell: string | boolean = resolvedShell
47+
let effectiveCommand = command
48+
49+
if (isWslShell) {
50+
// Spawn wsl.exe directly (not through cmd.exe) to avoid nested-quoting issues.
51+
// execa(file, args, options) passes args as an array — no shell interpretation.
52+
const windowsCwd = this.terminal.getCurrentWorkingDirectory()
53+
const forwardSlashedCwd = windowsCwd.replace(/\\/g, "/")
54+
const wslCwd = forwardSlashedCwd.replace(
55+
/^([A-Za-z]):\//,
56+
(_, drive: string) => `/mnt/${drive.toLowerCase()}/`,
57+
)
58+
59+
const wslArgs = ["--", "bash", "-c", command]
60+
61+
if (wslCwd !== forwardSlashedCwd) {
62+
// Drive path successfully converted — use --cd to set WSL working directory
63+
wslArgs.unshift("--cd", wslCwd)
64+
}
65+
66+
this.subprocess = execa(WSL_EXE_PATH, wslArgs, {
67+
cwd: undefined,
68+
all: true,
69+
stdin: "ignore",
70+
env: {
71+
...process.env,
72+
LANG: "en_US.UTF-8",
73+
LC_ALL: "en_US.UTF-8",
74+
},
75+
})
76+
} else {
77+
this.subprocess = execa({
78+
shell: effectiveShell,
79+
cwd: this.terminal.getCurrentWorkingDirectory(),
80+
all: true,
81+
stdin: "ignore",
82+
env: {
83+
...process.env,
84+
LANG: "en_US.UTF-8",
85+
LC_ALL: "en_US.UTF-8",
86+
},
87+
})`${effectiveCommand}`
88+
}
5689

5790
this.pid = this.subprocess.pid
5891

‎src/integrations/terminal/Terminal.ts‎

Lines changed: 82 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,102 @@
11
import * as vscode from "vscode"
2-
import pWaitFor from "p-wait-for"
32

43
import type { RooTerminalCallbacks, RooTerminalProcessResultPromise } from "./types"
54
import { BaseTerminal } from "./BaseTerminal"
65
import { TerminalProcess } from "./TerminalProcess"
76
import { ShellIntegrationManager } from "./ShellIntegrationManager"
87
import { mergePromise } from "./mergePromise"
9-
import { getShell } from "../../utils/shell"
8+
import { getWslProfile } from "../../utils/shell"
109

1110
export class Terminal extends BaseTerminal {
1211
public terminal: vscode.Terminal
1312

1413
public cmdCounter: number = 0
1514

15+
// Promise that resolves once shell integration is ready (or times out).
16+
// Uses the onDidChangeTerminalShellIntegration event for instant detection
17+
// when shell integration activates, with a timeout as safety net.
18+
private shellIntegrationReady: Promise<void>
19+
1620
constructor(id: number, terminal: vscode.Terminal | undefined, cwd: string) {
1721
super("vscode", id, cwd)
1822

1923
const env = Terminal.getEnv()
2024
const iconPath = new vscode.ThemeIcon("rocket")
21-
this.terminal = terminal ?? vscode.window.createTerminal({ cwd, name: "Roo Code", iconPath, env, shellPath: getShell() })
25+
26+
const wslProfile = getWslProfile()
27+
28+
// For WSL, do NOT pass explicit shellPath/shellArgs — let VS Code use
29+
// its default profile which has source:"WSL" (or auto-detects WSL via
30+
// the profile path). Explicitly passing shellPath bypasses the profile
31+
// system and prevents VS Code from injecting WSL shell integration.
32+
if (wslProfile) {
33+
this.terminal = terminal ?? vscode.window.createTerminal({ cwd, name: "Roo Code", iconPath, env })
34+
} else if (BaseTerminal.getExecaShellPath()) {
35+
const shell = BaseTerminal.getExecaShellPath()!
36+
this.terminal = terminal ?? vscode.window.createTerminal({ cwd, name: "Roo Code", iconPath, env, shellPath: shell })
37+
} else {
38+
this.terminal = terminal ?? vscode.window.createTerminal({ cwd, name: "Roo Code", iconPath, env })
39+
}
2240

2341
if (Terminal.getTerminalZdotdir()) {
2442
ShellIntegrationManager.terminalTmpDirs.set(id, env.ZDOTDIR)
2543
}
44+
45+
// Wait for shell integration using both the VS Code event (instant)
46+
// and polling. Both run within the user-configured timeout — no hidden
47+
// extension. WSL terminals also follow this path: shell integration is
48+
// not supported for WSL (OSC 633 sequences don't traverse the PTY
49+
// bridge), so the timeout will fire naturally and runCommand falls
50+
// through to the execa fallback.
51+
this.shellIntegrationReady = new Promise<void>((resolve) => {
52+
// Already ready?
53+
if (this.terminal.shellIntegration) {
54+
resolve()
55+
return
56+
}
57+
58+
const timeout = Terminal.getShellIntegrationTimeout()
59+
60+
let settled = false
61+
const done = () => {
62+
if (settled) return
63+
settled = true
64+
clearTimeout(timeoutId)
65+
clearInterval(pollInterval)
66+
eventDisposable.dispose()
67+
resolve()
68+
}
69+
70+
// Event-based detection: fires instantly when shell integration activates.
71+
// Check shellIntegration (not .executeCommand) — matching original
72+
// pWaitFor behavior. If we check .executeCommand, a brief window between
73+
// shellIntegration being set and executeCommand being ready would cause us
74+
// to miss the activation entirely (no more events fire, only timeout).
75+
const eventDisposable = vscode.window.onDidChangeTerminalShellIntegration((e) => {
76+
if (e.terminal === this.terminal && this.terminal.shellIntegration) {
77+
done()
78+
}
79+
})
80+
81+
// Polling fallback: same loose check as original pWaitFor so that
82+
// non-WSL shells (Git Bash, pwsh) correctly detect shell integration
83+
// activation even when events fire out of order.
84+
const pollInterval = setInterval(() => {
85+
if (this.terminal.shellIntegration) {
86+
done()
87+
}
88+
}, 500)
89+
90+
// Safety-net timeout: if shell integration never activates within the
91+
// configured time (e.g. WSL), resolve anyway.
92+
const timeoutId = setTimeout(() => done(), timeout)
93+
})
94+
95+
// Clean up ZDOTDIR temp directory once shell integration is ready
96+
// (or on timeout). Covers all shell types including WSL.
97+
this.shellIntegrationReady.finally(() => {
98+
ShellIntegrationManager.zshCleanupTmpDir(this.id)
99+
})
26100
}
27101

28102
/**
@@ -61,35 +135,18 @@ export class Terminal extends BaseTerminal {
61135
process.once("no_shell_integration", (msg) => callbacks.onNoShellIntegration?.(msg, process))
62136

63137
const promise = new Promise<void>((resolve, reject) => {
64-
// Set up event handlers
65138
process.once("continue", () => resolve())
66139
process.once("error", (error) => {
67140
console.error(`[Terminal ${this.id}] error:`, error)
68141
reject(error)
69142
})
70143

71-
// Wait for shell integration before executing the command
72-
pWaitFor(() => this.terminal.shellIntegration !== undefined, {
73-
timeout: Terminal.getShellIntegrationTimeout(),
144+
// Reuse the shell-integration-ready promise started in the
145+
// constructor — the wait has already been running since
146+
// terminal creation, so by now it may already be resolved.
147+
this.shellIntegrationReady.then(() => {
148+
process.run(command)
74149
})
75-
.then(() => {
76-
// Clean up temporary directory if shell integration is available, zsh did its job:
77-
ShellIntegrationManager.zshCleanupTmpDir(this.id)
78-
79-
// Run the command in the terminal
80-
process.run(command)
81-
})
82-
.catch(() => {
83-
console.log(`[Terminal ${this.id}] Shell integration not available. Command execution aborted.`)
84-
85-
// Clean up temporary directory if shell integration is not available
86-
ShellIntegrationManager.zshCleanupTmpDir(this.id)
87-
88-
process.emit(
89-
"no_shell_integration",
90-
`Shell integration initialization sequence '\\x1b]633;A' was not received within ${Terminal.getShellIntegrationTimeout() / 1000}s. Shell integration has been disabled for this terminal instance. Increase the timeout in the settings if necessary.`,
91-
)
92-
})
93150
})
94151

95152
return mergePromise(process, promise)

‎src/integrations/terminal/TerminalProcess.ts‎

Lines changed: 17 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
import * as vscode from "vscode"
2-
import { inspect } from "util"
3-
42
import type { ExitCodeDetails } from "./types"
53
import { BaseTerminalProcess } from "./BaseTerminalProcess"
64
import { Terminal } from "./Terminal"
7-
85
export class TerminalProcess extends BaseTerminalProcess {
96
// #266: Some processes (interactive tools, programs that trap SIGINT and
107
// prompt for confirmation) need more than one Ctrl+C to actually exit. We
@@ -56,23 +53,16 @@ export class TerminalProcess extends BaseTerminalProcess {
5653
const isShellIntegrationAvailable = terminal.shellIntegration && terminal.shellIntegration.executeCommand
5754

5855
if (!isShellIntegrationAvailable) {
59-
terminal.sendText(command, true)
60-
6156
console.warn(
62-
"[TerminalProcess] Shell integration not available. Command sent without knowledge of response.",
57+
"[TerminalProcess] Shell integration not available — NOT using sendText " +
58+
"(would execute without output capture). Falling back to inline (execa) execution.",
6359
)
6460

6561
this.emit(
6662
"no_shell_integration",
67-
"Command was submitted; output is not available, as shell integration is inactive.",
63+
"Shell integration is inactive; falling back to inline (execa) execution.",
6864
)
6965

70-
this.emit(
71-
"completed",
72-
"<shell integration is not available, so terminal output and command execution status is unknown>",
73-
)
74-
75-
this.emit("continue")
7666
return
7767
}
7868

@@ -82,11 +72,10 @@ export class TerminalProcess extends BaseTerminalProcess {
8272
// Remove event listener to prevent memory leaks
8373
this.removeAllListeners("stream_available")
8474

85-
// Emit no_shell_integration event with descriptive message
86-
this.emit(
87-
"no_shell_integration",
88-
`VSCE shell integration stream did not start within ${Terminal.getShellIntegrationTimeout() / 1000} seconds. Terminal problem?`,
89-
)
75+
// NOTE: Do NOT emit "no_shell_integration" here — the command was
76+
// already submitted via executeCommand() and will run (or is running)
77+
// in the external terminal. Emitting "no_shell_integration" would
78+
// cause ExecuteCommandTool to re-execute via execa (double execution).
9079

9180
// Reject with descriptive error
9281
reject(
@@ -221,27 +210,16 @@ export class TerminalProcess extends BaseTerminalProcess {
221210
// Emit any remaining output before completing
222211
this.emitRemainingBufferIfListening()
223212
} else {
224-
const errorMsg =
225-
"VSCE output start escape sequence (]633;C or ]133;C) not received, but the stream has started. Upstream VSCE Bug?"
226-
227-
const inspectPreOutput = inspect(preOutput, { colors: false, breakLength: Infinity })
228-
console.error(`[Terminal Process] ${errorMsg} preOutput: ${inspectPreOutput}`)
229-
230-
// Emit no_shell_integration event
231-
this.emit("no_shell_integration", errorMsg)
232-
233-
// Emit completed event with error message
234-
this.emit(
235-
"completed",
236-
"<VSCE shell integration markers not found: terminal output and command execution status is unknown>\n" +
237-
`<preOutput>${inspectPreOutput}</preOutput>\n` +
238-
"AI MODEL: You MUST notify the user with the information above so they can open a bug report.",
239-
)
240-
241-
this.continue()
242-
243-
// Return early since we can't process output without shell integration markers
244-
return
213+
// The command executed via executeCommand() (shell integration IS available),
214+
// but the stream didn't contain the expected ]633;C / ]133;C start marker.
215+
// This happens on WSL where the PTY bridge can reorder or omit markers.
216+
// Since the command already ran in the external terminal, falling back to
217+
// execa would cause double execution. Instead, treat all stream data as
218+
// command output and complete normally.
219+
// Use the preOutput as the command output — it contains the actual
220+
// command result even if the markers were stripped by the WSL PTY bridge.
221+
this.fullOutput = preOutput
222+
this.emitRemainingBufferIfListening()
245223
}
246224

247225
// fullOutput begins after C marker so we only need to trim off D marker

0 commit comments

Comments
 (0)