-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: use automation profile for terminal creation #7893
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
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 |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import { BaseTerminal } from "./BaseTerminal" | |
| import { TerminalProcess } from "./TerminalProcess" | ||
| import { ShellIntegrationManager } from "./ShellIntegrationManager" | ||
| import { mergePromise } from "./mergePromise" | ||
| import { getAutomationShell } from "../../utils/shell" | ||
|
|
||
| export class Terminal extends BaseTerminal { | ||
| public terminal: vscode.Terminal | ||
|
|
@@ -17,7 +18,19 @@ export class Terminal extends BaseTerminal { | |
|
|
||
| const env = Terminal.getEnv() | ||
| const iconPath = new vscode.ThemeIcon("rocket") | ||
| this.terminal = terminal ?? vscode.window.createTerminal({ cwd, name: "Roo Code", iconPath, env }) | ||
|
|
||
| // Try to get the automation shell first, fall back to default behavior if not configured | ||
| const shellPath = getAutomationShell() || undefined | ||
|
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. Could we simplify this to just |
||
|
|
||
| const terminalOptions: vscode.TerminalOptions = { | ||
| cwd, | ||
| name: "Roo Code", | ||
| iconPath, | ||
| env, | ||
| shellPath, | ||
| } | ||
|
|
||
| this.terminal = terminal ?? vscode.window.createTerminal(terminalOptions) | ||
|
|
||
| if (Terminal.getTerminalZdotdir()) { | ||
| ShellIntegrationManager.terminalTmpDirs.set(id, env.ZDOTDIR) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,18 @@ | |
| import * as vscode from "vscode" | ||
| import { Terminal } from "../Terminal" | ||
| import { TerminalRegistry } from "../TerminalRegistry" | ||
| import * as shellUtils from "../../../utils/shell" | ||
|
|
||
| const PAGER = process.platform === "win32" ? "" : "cat" | ||
|
|
||
| vi.mock("execa", () => ({ | ||
| execa: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock("../../../utils/shell", () => ({ | ||
| getAutomationShell: vi.fn(() => null), | ||
| })) | ||
|
|
||
| describe("TerminalRegistry", () => { | ||
| let mockCreateTerminal: any | ||
|
|
||
|
|
@@ -118,5 +123,80 @@ describe("TerminalRegistry", () => { | |
| Terminal.setTerminalZshP10k(false) | ||
| } | ||
| }) | ||
|
|
||
| it("uses automation profile shell when configured", () => { | ||
|
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. Should we add a test case for when getAutomationShell() throws an exception? It would be good to ensure the Terminal constructor handles this gracefully and falls back to default behavior. |
||
| // Mock getAutomationShell to return a specific shell path | ||
| const getAutomationShellMock = shellUtils.getAutomationShell as any | ||
| getAutomationShellMock.mockReturnValue("/bin/bash") | ||
|
|
||
| try { | ||
| TerminalRegistry.createTerminal("/test/path", "vscode") | ||
|
|
||
| expect(mockCreateTerminal).toHaveBeenCalledWith({ | ||
| cwd: "/test/path", | ||
| name: "Roo Code", | ||
| iconPath: expect.any(Object), | ||
| env: { | ||
| PAGER, | ||
| VTE_VERSION: "0", | ||
| PROMPT_EOL_MARK: "", | ||
| }, | ||
| shellPath: "/bin/bash", | ||
| }) | ||
| } finally { | ||
| // Reset mock | ||
| getAutomationShellMock.mockReturnValue(null) | ||
| } | ||
| }) | ||
|
|
||
| it("does not set shellPath when automation profile is not configured", () => { | ||
| // Mock getAutomationShell to return null (no automation profile) | ||
| const getAutomationShellMock = shellUtils.getAutomationShell as any | ||
| getAutomationShellMock.mockReturnValue(null) | ||
|
|
||
| TerminalRegistry.createTerminal("/test/path", "vscode") | ||
|
|
||
| expect(mockCreateTerminal).toHaveBeenCalledWith({ | ||
| cwd: "/test/path", | ||
| name: "Roo Code", | ||
| iconPath: expect.any(Object), | ||
| env: { | ||
| PAGER, | ||
| VTE_VERSION: "0", | ||
| PROMPT_EOL_MARK: "", | ||
| }, | ||
| shellPath: undefined, | ||
| }) | ||
| }) | ||
|
|
||
| it("uses Windows automation profile when configured", () => { | ||
| // Mock for Windows platform | ||
| const originalPlatform = process.platform | ||
| Object.defineProperty(process, "platform", { value: "win32", configurable: true }) | ||
|
|
||
| // Mock getAutomationShell to return PowerShell 7 path | ||
| const getAutomationShellMock = shellUtils.getAutomationShell as any | ||
| getAutomationShellMock.mockReturnValue("C:\\Program Files\\PowerShell\\7\\pwsh.exe") | ||
|
|
||
| try { | ||
| TerminalRegistry.createTerminal("/test/path", "vscode") | ||
|
|
||
| expect(mockCreateTerminal).toHaveBeenCalledWith({ | ||
| cwd: "/test/path", | ||
| name: "Roo Code", | ||
| iconPath: expect.any(Object), | ||
| env: { | ||
| PAGER: "", | ||
| VTE_VERSION: "0", | ||
| PROMPT_EOL_MARK: "", | ||
| }, | ||
| shellPath: "C:\\Program Files\\PowerShell\\7\\pwsh.exe", | ||
| }) | ||
| } finally { | ||
| // Restore platform and mock | ||
| Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true }) | ||
| getAutomationShellMock.mockReturnValue(null) | ||
| } | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" | ||
| import * as vscode from "vscode" | ||
| import { userInfo } from "os" | ||
| import { getShell } from "../shell" | ||
| import { getShell, getAutomationShell } from "../shell" | ||
|
|
||
| // Mock vscode module | ||
| vi.mock("vscode", () => ({ | ||
|
|
@@ -485,4 +485,213 @@ describe("Shell Detection Tests", () => { | |
| expect(result).toBe("/bin/bash") // Should fall back to safe default | ||
| }) | ||
| }) | ||
|
|
||
| // -------------------------------------------------------------------------- | ||
| // Automation Shell Detection Tests | ||
| // -------------------------------------------------------------------------- | ||
| describe("Automation Shell Detection", () => { | ||
|
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. The automation shell tests have great coverage! Though for consistency with the main shell detection tests above, we could consider reorganizing these into more structured platform-specific describe blocks. Not critical, but might improve readability. |
||
| describe("Windows Automation Shell", () => { | ||
| beforeEach(() => { | ||
| Object.defineProperty(process, "platform", { value: "win32" }) | ||
| }) | ||
|
|
||
| it("uses automation profile path when configured", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.windows") { | ||
| return { path: "C:\\Program Files\\PowerShell\\7\\pwsh.exe" } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe") | ||
| }) | ||
|
|
||
| it("handles array path in automation profile", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.windows") { | ||
| return { path: ["C:\\Program Files\\Git\\bin\\bash.exe", "bash.exe"] } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBe("C:\\Program Files\\Git\\bin\\bash.exe") | ||
| }) | ||
|
|
||
| it("returns null when no automation profile is configured", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.windows") { | ||
| return null | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
|
|
||
| it("returns null when automation profile has no path", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.windows") { | ||
| return { source: "PowerShell" } // No path property | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
|
|
||
| it("returns null when automation profile path is not in allowlist", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.windows") { | ||
| return { path: "C:\\malicious\\shell.exe" } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| describe("macOS Automation Shell", () => { | ||
| beforeEach(() => { | ||
| Object.defineProperty(process, "platform", { value: "darwin" }) | ||
| }) | ||
|
|
||
| it("uses automation profile path when configured", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.osx") { | ||
| return { path: "/bin/bash" } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBe("/bin/bash") | ||
| }) | ||
|
|
||
| it("handles array path in automation profile", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.osx") { | ||
| return { path: ["/usr/local/bin/bash", "/bin/bash"] } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBe("/usr/local/bin/bash") | ||
| }) | ||
|
|
||
| it("returns null when no automation profile is configured", () => { | ||
| const mockConfig = { | ||
| get: vi.fn(() => undefined), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| describe("Linux Automation Shell", () => { | ||
| beforeEach(() => { | ||
| Object.defineProperty(process, "platform", { value: "linux" }) | ||
| }) | ||
|
|
||
| it("uses automation profile path when configured", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.linux") { | ||
| return { path: "/bin/bash" } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBe("/bin/bash") | ||
| }) | ||
|
|
||
| it("handles array path in automation profile", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.linux") { | ||
| return { path: ["/usr/bin/zsh", "/bin/zsh"] } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBe("/usr/bin/zsh") | ||
| }) | ||
|
|
||
| it("returns null when automation profile path is empty array", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.linux") { | ||
| return { path: [] } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
|
|
||
| it("validates automation shell against allowlist", () => { | ||
| const mockConfig = { | ||
| get: vi.fn((key: string) => { | ||
| if (key === "automationProfile.linux") { | ||
| return { path: "/usr/bin/evil-shell" } | ||
| } | ||
| return undefined | ||
| }), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| describe("Unknown Platform Automation Shell", () => { | ||
| it("returns null for unknown platforms", () => { | ||
| Object.defineProperty(process, "platform", { value: "sunos" }) | ||
| const mockConfig = { | ||
| get: vi.fn(() => undefined), | ||
| } | ||
| vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
| }) | ||
|
|
||
| describe("Error Handling in Automation Shell", () => { | ||
| it("handles configuration errors gracefully", () => { | ||
| Object.defineProperty(process, "platform", { value: "win32" }) | ||
| vscode.workspace.getConfiguration = () => { | ||
| throw new Error("Configuration error") | ||
| } | ||
|
|
||
| expect(getAutomationShell()).toBeNull() | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
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.
Is this comment accurate? The fallback to default behavior actually happens in VSCode's createTerminal when shellPath is undefined, not in our getAutomationShell() function. Maybe we should clarify this to avoid confusion?