Skip to content

Validate the default shell #7680

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Validate the default shell #7680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions src/core/prompts/sections/system-info.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ import osName from "os-name"

import { getShell } from "../../../utils/shell"

export function getSystemInfoSection(cwd: string): string {
export async function getSystemInfoSection(cwd: string): Promise<string> {
const shell = await getShell()
let details = `====

SYSTEM INFORMATION

Operating System: ${osName()}
Default Shell: ${getShell()}
Default Shell: ${shell}
Home Directory: ${os.homedir().toPosix()}
Current Workspace Directory: ${cwd.toPosix()}

Expand Down
2 changes: 1 addition & 1 deletion src/core/prompts/system.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ ${modesSection}

${getRulesSection(cwd, supportsComputerUse, effectiveDiffStrategy, codeIndexManager)}

${getSystemInfoSection(cwd)}
${await getSystemInfoSection(cwd)}

${getObjectiveSection(codeIndexManager, experiments)}

Expand Down
2 changes: 2 additions & 0 deletions src/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,7 @@
"uuid": "^11.1.0",
"vscode-material-icons": "^0.1.1",
"web-tree-sitter": "^0.25.6",
"which": "^5.0.0",
"workerpool": "^9.2.0",
"yaml": "^2.8.0",
"zod": "^3.25.61"
Expand All @@ -522,6 +523,7 @@
"@types/tmp": "^0.2.6",
"@types/turndown": "^5.0.5",
"@types/vscode": "^1.84.0",
"@types/which": "^3.0.4",
"@vscode/test-electron": "^2.5.2",
"@vscode/vsce": "3.3.2",
"esbuild": "^0.25.0",
Expand Down
97 changes: 55 additions & 42 deletions src/utils/__tests__/shell.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ vi.mock("os", () => ({
userInfo: vi.fn(() => ({ shell: null })),
}))

// Mock the which module
vi.mock("which", () => ({
default: vi.fn(),
}))

import which from "which"

describe("Shell Detection Tests", () => {
let originalPlatform: string
let originalEnv: NodeJS.ProcessEnv
Expand Down Expand Up @@ -40,6 +47,12 @@ describe("Shell Detection Tests", () => {

// Reset userInfo mock to default
vi.mocked(userInfo).mockReturnValue({ shell: null } as any)

// Mock which to always resolve paths successfully for tests
vi.mocked(which).mockImplementation(async (cmd: string) => {
Copy link
Contributor

@roomote roomote bot Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be valuable to add test cases where which returns null to ensure the fallback logic works correctly? Currently, the mock always returns the command successfully.

Example test case:

Suggested change
vi.mocked(which).mockImplementation(async (cmd: string) => {
it("falls back through all options when validation fails", async () => {
// Mock which to return null for all paths
vi.mocked(which).mockResolvedValue(null)
mockVsCodeConfig("windows", "PowerShell", {
PowerShell: { path: "C:\Invalid\Path\pwsh.exe" },
})
vi.mocked(userInfo).mockReturnValue({ shell: "C:\Invalid\Shell.exe" } as any)
process.env.COMSPEC = "C:\Invalid\cmd.exe"
// Should fall back to the default CMD path
expect(await getShell()).toBe("C:\Windows\System32\cmd.exe")
})

// Return the command as-is to simulate successful resolution
return cmd
})
})

afterEach(() => {
Expand All @@ -58,66 +71,66 @@ describe("Shell Detection Tests", () => {
Object.defineProperty(process, "platform", { value: "win32" })
})

it("uses explicit PowerShell 7 path from VS Code config (profile path)", () => {
it("uses explicit PowerShell 7 path from VS Code config (profile path)", async () => {
mockVsCodeConfig("windows", "PowerShell", {
PowerShell: { path: "C:\\Program Files\\PowerShell\\7\\pwsh.exe" },
})
expect(getShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
expect(await getShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
})

it("uses PowerShell 7 path if source is 'PowerShell' but no explicit path", () => {
it("uses PowerShell 7 path if source is 'PowerShell' but no explicit path", async () => {
mockVsCodeConfig("windows", "PowerShell", {
PowerShell: { source: "PowerShell" },
})
expect(getShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
expect(await getShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
})

it("falls back to legacy PowerShell if profile includes 'powershell' but no path/source", () => {
it("falls back to legacy PowerShell if profile includes 'powershell' but no path/source", async () => {
mockVsCodeConfig("windows", "PowerShell", {
PowerShell: {},
})
expect(getShell()).toBe("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe")
expect(await getShell()).toBe("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe")
})

it("uses WSL bash when profile indicates WSL source", () => {
it("uses WSL bash when profile indicates WSL source", async () => {
mockVsCodeConfig("windows", "WSL", {
WSL: { source: "WSL" },
})
expect(getShell()).toBe("/bin/bash")
expect(await getShell()).toBe("/bin/bash")
})

it("uses WSL bash when profile name includes 'wsl'", () => {
it("uses WSL bash when profile name includes 'wsl'", async () => {
mockVsCodeConfig("windows", "Ubuntu WSL", {
"Ubuntu WSL": {},
})
expect(getShell()).toBe("/bin/bash")
expect(await getShell()).toBe("/bin/bash")
})

it("defaults to cmd.exe if no special profile is matched", () => {
it("defaults to cmd.exe if no special profile is matched", async () => {
mockVsCodeConfig("windows", "CommandPrompt", {
CommandPrompt: {},
})
expect(getShell()).toBe("C:\\Windows\\System32\\cmd.exe")
expect(await getShell()).toBe("C:\\Windows\\System32\\cmd.exe")
})

it("handles undefined profile gracefully", () => {
it("handles undefined profile gracefully", async () => {
// Mock a case where defaultProfileName exists but the profile doesn't
mockVsCodeConfig("windows", "NonexistentProfile", {})
expect(getShell()).toBe("C:\\Windows\\System32\\cmd.exe")
expect(await getShell()).toBe("C:\\Windows\\System32\\cmd.exe")
})

it("respects userInfo() if no VS Code config is available", () => {
it("respects userInfo() if no VS Code config is available", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
vi.mocked(userInfo).mockReturnValue({ shell: "C:\\Custom\\PowerShell.exe" } as any)

expect(getShell()).toBe("C:\\Custom\\PowerShell.exe")
expect(await getShell()).toBe("C:\\Custom\\PowerShell.exe")
})

it("respects an odd COMSPEC if no userInfo shell is available", () => {
it("respects an odd COMSPEC if no userInfo shell is available", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
process.env.COMSPEC = "D:\\CustomCmd\\cmd.exe"

expect(getShell()).toBe("D:\\CustomCmd\\cmd.exe")
expect(await getShell()).toBe("D:\\CustomCmd\\cmd.exe")
})
})

Expand All @@ -129,28 +142,28 @@ describe("Shell Detection Tests", () => {
Object.defineProperty(process, "platform", { value: "darwin" })
})

it("uses VS Code profile path if available", () => {
it("uses VS Code profile path if available", async () => {
mockVsCodeConfig("osx", "MyCustomShell", {
MyCustomShell: { path: "/usr/local/bin/fish" },
})
expect(getShell()).toBe("/usr/local/bin/fish")
expect(await getShell()).toBe("/usr/local/bin/fish")
})

it("falls back to userInfo().shell if no VS Code config is available", () => {
it("falls back to userInfo().shell if no VS Code config is available", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
vi.mocked(userInfo).mockReturnValue({ shell: "/opt/homebrew/bin/zsh" } as any)
expect(getShell()).toBe("/opt/homebrew/bin/zsh")
expect(await getShell()).toBe("/opt/homebrew/bin/zsh")
})

it("falls back to SHELL env var if no userInfo shell is found", () => {
it("falls back to SHELL env var if no userInfo shell is found", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
process.env.SHELL = "/usr/local/bin/zsh"
expect(getShell()).toBe("/usr/local/bin/zsh")
expect(await getShell()).toBe("/usr/local/bin/zsh")
})

it("falls back to /bin/zsh if no config, userInfo, or env variable is set", () => {
it("falls back to /bin/zsh if no config, userInfo, or env variable is set", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
expect(getShell()).toBe("/bin/zsh")
expect(await getShell()).toBe("/bin/zsh")
})
})

Expand All @@ -162,61 +175,61 @@ describe("Shell Detection Tests", () => {
Object.defineProperty(process, "platform", { value: "linux" })
})

it("uses VS Code profile path if available", () => {
it("uses VS Code profile path if available", async () => {
mockVsCodeConfig("linux", "CustomProfile", {
CustomProfile: { path: "/usr/bin/fish" },
})
expect(getShell()).toBe("/usr/bin/fish")
expect(await getShell()).toBe("/usr/bin/fish")
})

it("falls back to userInfo().shell if no VS Code config is available", () => {
it("falls back to userInfo().shell if no VS Code config is available", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
vi.mocked(userInfo).mockReturnValue({ shell: "/usr/bin/zsh" } as any)
expect(getShell()).toBe("/usr/bin/zsh")
expect(await getShell()).toBe("/usr/bin/zsh")
})

it("falls back to SHELL env var if no userInfo shell is found", () => {
it("falls back to SHELL env var if no userInfo shell is found", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
process.env.SHELL = "/usr/bin/fish"
expect(getShell()).toBe("/usr/bin/fish")
expect(await getShell()).toBe("/usr/bin/fish")
})

it("falls back to /bin/bash if nothing is set", () => {
it("falls back to /bin/bash if nothing is set", async () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
expect(getShell()).toBe("/bin/bash")
expect(await getShell()).toBe("/bin/bash")
})
})

// --------------------------------------------------------------------------
// Unknown Platform & Error Handling
// --------------------------------------------------------------------------
describe("Unknown Platform / Error Handling", () => {
it("falls back to /bin/sh for unknown platforms", () => {
it("falls back to /bin/sh for unknown platforms", async () => {
Object.defineProperty(process, "platform", { value: "sunos" })
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
expect(getShell()).toBe("/bin/sh")
expect(await getShell()).toBe("/bin/sh")
})

it("handles VS Code config errors gracefully, falling back to userInfo shell if present", () => {
it("handles VS Code config errors gracefully, falling back to userInfo shell if present", async () => {
Object.defineProperty(process, "platform", { value: "linux" })
vscode.workspace.getConfiguration = () => {
throw new Error("Configuration error")
}
vi.mocked(userInfo).mockReturnValue({ shell: "/bin/bash" } as any)
expect(getShell()).toBe("/bin/bash")
expect(await getShell()).toBe("/bin/bash")
})

it("handles userInfo errors gracefully, falling back to environment variable if present", () => {
it("handles userInfo errors gracefully, falling back to environment variable if present", async () => {
Object.defineProperty(process, "platform", { value: "darwin" })
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
vi.mocked(userInfo).mockImplementation(() => {
throw new Error("userInfo error")
})
process.env.SHELL = "/bin/zsh"
expect(getShell()).toBe("/bin/zsh")
expect(await getShell()).toBe("/bin/zsh")
})

it("falls back fully to default shell paths if everything fails", () => {
it("falls back fully to default shell paths if everything fails", async () => {
Object.defineProperty(process, "platform", { value: "linux" })
vscode.workspace.getConfiguration = () => {
throw new Error("Configuration error")
Expand All @@ -225,7 +238,7 @@ describe("Shell Detection Tests", () => {
throw new Error("userInfo error")
})
delete process.env.SHELL
expect(getShell()).toBe("/bin/bash")
expect(await getShell()).toBe("/bin/bash")
})
})
})
Loading
Loading