Skip to content

fix: return null from getShellFromEnv when environment variables are not set #8579

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

fix: return null from getShellFromEnv when environment variables are not set #8579

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
6 changes: 6 additions & 0 deletions src/utils/__tests__/shell.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,8 @@ describe("Shell Detection Tests", () => {

it("falls back to /bin/zsh if no config, userInfo, or env variable is set", () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
// With the fix, getShellFromEnv() returns null when SHELL is not set,
// so it falls back to the safe default from getSafeFallbackShell()
expect(getShell()).toBe("/bin/zsh")
})
})
Expand Down Expand Up @@ -303,6 +305,8 @@ describe("Shell Detection Tests", () => {

it("falls back to /bin/bash if nothing is set", () => {
vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
// With the fix, getShellFromEnv() returns null when SHELL is not set,
// so it falls back to the safe default from getSafeFallbackShell()
expect(getShell()).toBe("/bin/bash")
})
})
Expand Down Expand Up @@ -345,6 +349,8 @@ describe("Shell Detection Tests", () => {
throw new Error("userInfo error")
})
delete process.env.SHELL
// With the fix, getShellFromEnv() returns null when SHELL is not set,
// so it falls back to the safe default from getSafeFallbackShell()
expect(getShell()).toBe("/bin/bash")
})
})
Expand Down
12 changes: 4 additions & 8 deletions src/utils/shell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,18 +269,14 @@ function getShellFromEnv(): string | null {

if (process.platform === "win32") {
// On Windows, COMSPEC typically holds cmd.exe
return env.COMSPEC || "C:\\Windows\\System32\\cmd.exe"
return env.COMSPEC || null
}

if (process.platform === "darwin") {
// On macOS/Linux, SHELL is commonly the environment variable
return env.SHELL || "/bin/zsh"
// On Unix-like systems (macOS, Linux), SHELL is the environment variable
if (process.platform === "darwin" || process.platform === "linux") {
return env.SHELL || null
}

if (process.platform === "linux") {
// On Linux, SHELL is commonly the environment variable
return env.SHELL || "/bin/bash"
}
return null
Copy link
Author

@roomote roomote bot Oct 9, 2025

Choose a reason for hiding this comment

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

P1: Potential regression for unknown Unix-like platforms

The current implementation will return null for platforms that are not win32, darwin, or linux (e.g., freebsd, openbsd, sunos, aix). This means these platforms won't check the SHELL environment variable at all.

Before this PR, these platforms would have gotten a hardcoded fallback (/bin/bash for Linux path). While returning null allows the fallback chain to work, it skips checking SHELL entirely for these platforms.

Consider this alternative that checks SHELL for all Unix-like platforms:

Suggested change
return null
// On Unix-like systems, SHELL is the environment variable
return env.SHELL || null
}

This way, any Unix-like platform (including FreeBSD, OpenBSD, etc.) will check SHELL, and only return null if it's not set, allowing the proper fallback chain to work.

}

Expand Down
Loading