fix: use VS Code default shell for command execution instead of /bin/sh#509
fix: use VS Code default shell for command execution instead of /bin/sh#509p12tic wants to merge 1 commit into
Conversation
When terminalShellIntegrationDisabled is true (the default), commands are executed via execa. Previously, execa used shell: true which defaults to /bin/sh on Linux, ignoring the user's VS Code terminal shell settings. Now execa falls back to getShell() which respects: - VS Code terminal.integrated.defaultProfile.linux - os.userInfo().shell - $SHELL environment variable - Platform-specific defaults (/bin/bash on Linux, /bin/zsh on macOS)
p12tic
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
📝 WalkthroughWalkthrough
ChangesShell Resolution via getShell()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts (1)
102-123: 💤 Low valueConsider organizing shell selection tests into a separate describe block.
Tests at Lines 102-123 verify shell selection behavior but are nested under the "UTF-8 encoding fix" describe block. A dedicated describe block (e.g., "shell selection") would improve test organization and make the test structure clearer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts` around lines 102 - 123, Move the two tests that assert shell selection into a new describe block named "shell selection" to improve organization: create a describe("shell selection", () => { ... }) and place the it("should use execaShellPath when set", ...) and it("should fall back to getShell() when execaShellPath is undefined", ...) inside it; keep the test bodies unchanged and preserve references to BaseTerminal.setExecaShellPath, mockGetShell, terminalProcess.run, and the execa mock so behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts`:
- Around line 113-123: The test leaks mock behavior because
mockGetShell.mockReturnValue("/usr/bin/zsh") permanently changes the mock;
change the test to use mockGetShell.mockReturnValueOnce("/usr/bin/zsh") (or
restore/reset the mock implementation) so
BaseTerminal.setExecaShellPath(undefined) test only affects this test; update
the call in the "should fall back to getShell() when execaShellPath is
undefined" spec to use mockReturnValueOnce on mockGetShell to ensure isolation
and prevent interfering with other tests that expect "/bin/bash".
---
Nitpick comments:
In `@src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts`:
- Around line 102-123: Move the two tests that assert shell selection into a new
describe block named "shell selection" to improve organization: create a
describe("shell selection", () => { ... }) and place the it("should use
execaShellPath when set", ...) and it("should fall back to getShell() when
execaShellPath is undefined", ...) inside it; keep the test bodies unchanged and
preserve references to BaseTerminal.setExecaShellPath, mockGetShell,
terminalProcess.run, and the execa mock so behavior is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c75a69c7-78ea-4c02-8ad0-f4e70ea5b3c2
📒 Files selected for processing (2)
src/integrations/terminal/ExecaTerminalProcess.tssrc/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts
| it("should fall back to getShell() when execaShellPath is undefined", async () => { | ||
| BaseTerminal.setExecaShellPath(undefined) | ||
| mockGetShell.mockReturnValue("/usr/bin/zsh") | ||
| await terminalProcess.run("echo test") | ||
| const execaMock = vitest.mocked(execa) | ||
| expect(execaMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| shell: true, | ||
| shell: "/usr/bin/zsh", | ||
| }), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Fix test isolation issue to prevent flakiness.
Using mockReturnValue() on Line 115 permanently changes the mock's behavior until explicitly reset. Since clearAllMocks() (Line 62) only clears call history but not mock implementations, the "/usr/bin/zsh" return value will persist if this test runs before others, causing the test at Line 66-80 to fail when it expects "/bin/bash".
🔧 Proposed fix using mockReturnValueOnce
it("should fall back to getShell() when execaShellPath is undefined", async () => {
BaseTerminal.setExecaShellPath(undefined)
- mockGetShell.mockReturnValue("/usr/bin/zsh")
+ mockGetShell.mockReturnValueOnce("/usr/bin/zsh")
await terminalProcess.run("echo test")
const execaMock = vitest.mocked(execa)
expect(execaMock).toHaveBeenCalledWith(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts` around
lines 113 - 123, The test leaks mock behavior because
mockGetShell.mockReturnValue("/usr/bin/zsh") permanently changes the mock;
change the test to use mockGetShell.mockReturnValueOnce("/usr/bin/zsh") (or
restore/reset the mock implementation) so
BaseTerminal.setExecaShellPath(undefined) test only affects this test; update
the call in the "should fall back to getShell() when execaShellPath is
undefined" spec to use mockReturnValueOnce on mockGetShell to ensure isolation
and prevent interfering with other tests that expect "/bin/bash".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks for the contribution! We have a broader PR open at #333 that covers the same underlying issue (#321) and goes further — it fixes shell selection and includes handling for Windows/WSL edge cases that were surfaced during review. Could you coordinate with @F915 to see if there's anything here worth folding in |
|
After review I'm going to reopen this PR since the other one is still in draft, if you'd like to review #333 to incorporate any existing feedback into your PR that would help with future review. |
|
I have an open PR (#333) that relates to the same issue (#321 / #508). Noted in a review comment on #333, changing shell: true to shell: getShell() has a known limitation on Windows with WSL — getShell() can return /bin/bash, which is not a valid Windows shellPath. The current version of #333 takes a different approach, using vscode.env.shell for shell resolution instead of modifying shell: true alone. It's still in draft — testing across platforms and terminal modes is not complete yet. Would love to hear your thoughts if you have any. |
3 participants
Related GitHub Issue
Closes: #508
Description
When terminalShellIntegrationDisabled is true (the default), commands are executed via execa. Previously, execa used shell: true which defaults to /bin/sh on Linux, ignoring the user's VS Code terminal shell settings.
Now execa falls back to getShell() which respects:
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
n/a
Documentation Updates
not needed
Additional Notes
n/a
Get in Touch
povilas_78868
Summary by CodeRabbit