fix(shell): report PowerShell on Windows when no profile is configured (#82)#239
fix(shell): report PowerShell on Windows when no profile is configured (#82)#239proyectoauraorg wants to merge 1 commit into
Conversation
… is set (Zoo-Code-Org#82) getShell() fell through to COMSPEC (cmd.exe) when VS Code had no explicit Windows terminal profile, so the system prompt advertised cmd.exe even though the integrated terminal actually launches PowerShell (VS Code's modern default). Return Windows PowerShell from getWindowsShellFromVSCode() in that case so the prompt and rules match the real shell. Explicitly configured cmd/WSL/custom profiles are unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
📝 WalkthroughWalkthroughWindows shell detection behavior is updated so that ChangesWindows Shell Detection Default
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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.
🧹 Nitpick comments (1)
src/utils/shell.ts (1)
188-198: 💤 Low valueConsider narrowing the return type to
string.With this change, every branch of
getWindowsShellFromVSCode()now returns a non-null string (the early-return defaults toPOWERSHELL_LEGACY, and the remaining branches all return concreteSHELL_PATHS.*values ornormalizedPathfrom a profile). The declaredstring | nullreturn type is therefore misleading and may push callers to add unreachable null checks. The peer macOS/Linux helpers still legitimately returnnull, so this divergence is also worth documenting if you keep the union.♻️ Optional narrowing
-function getWindowsShellFromVSCode(): string | null { +function getWindowsShellFromVSCode(): string {🤖 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/utils/shell.ts` around lines 188 - 198, The function getWindowsShellFromVSCode currently declares a return type of string | null but all execution paths return a concrete string (the early exit returns SHELL_PATHS.POWERSHELL_LEGACY, and the other branches return specific SHELL_PATHS.* values or the profile's normalizedPath), so update the function signature to return string (not string | null) and adjust any callers if they were guarding against null; keep references to defaultProfileName, profiles, normalizedPath and SHELL_PATHS.* when locating the logic to change and, if desired, add a short comment noting that unlike the macOS/Linux helpers this helper never returns null.
🤖 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.
Nitpick comments:
In `@src/utils/shell.ts`:
- Around line 188-198: The function getWindowsShellFromVSCode currently declares
a return type of string | null but all execution paths return a concrete string
(the early exit returns SHELL_PATHS.POWERSHELL_LEGACY, and the other branches
return specific SHELL_PATHS.* values or the profile's normalizedPath), so update
the function signature to return string (not string | null) and adjust any
callers if they were guarding against null; keep references to
defaultProfileName, profiles, normalizedPath and SHELL_PATHS.* when locating the
logic to change and, if desired, add a short comment noting that unlike the
macOS/Linux helpers this helper never returns null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ccac09b-b670-490b-b222-1b427d4a60c0
📒 Files selected for processing (2)
src/utils/__tests__/shell.spec.tssrc/utils/shell.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| // No explicit Windows terminal profile is configured. VS Code's built-in | ||
| // default on modern Windows is PowerShell (not cmd.exe), and that is the | ||
| // shell the integrated terminal actually launches. Mirror it here so the | ||
| // system prompt advertises the real shell instead of falling through to | ||
| // COMSPEC (cmd.exe). Windows PowerShell is always present, so it is a safe | ||
| // allowlisted default. See issue #82. | ||
| return SHELL_PATHS.POWERSHELL_LEGACY |
There was a problem hiding this comment.
On a machine with PowerShell 7 installed, VS Code's auto-detection will typically select PS7 as the default terminal — not legacy 5.1. Commit 1a2c45cb fixed the analogous case (profile name includes "powershell" but no path/source) by switching from POWERSHELL_LEGACY → POWERSHELL_7 for exactly this reason (UTF-8 mojibake). Should this path apply the same heuristic — e.g. probe for PS7 at its known path before falling back to legacy?
| // Modern VS Code launches PowerShell by default on Windows (issue #82), so | ||
| // getShell() should report PowerShell rather than falling through to cmd.exe. | ||
| vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any | ||
| vi.mocked(userInfo).mockReturnValue({ shell: "C:\\Program Files\\PowerShell\\7\\pwsh.exe" } as any) |
There was a problem hiding this comment.
Is this mock still needed? With the early return at shell.ts:197, getShellFromUserInfo() is never called in this code path — so this mock value is never read. Removing it would make the test clearer about what's actually being exercised.
| vi.mocked(userInfo).mockReturnValue({ shell: "C:\\Program Files\\PowerShell\\7\\pwsh.exe" } as any) |
2 participants
Summary
Fixes #82. On Windows the system prompt told the model the shell was cmd.exe, but the integrated terminal actually runs PowerShell — VS Code's default on modern Windows. The two shell-detection paths disagreed.
getShell()(used by the system prompt insystem-info.tsand shell-specificrules.ts) calledgetWindowsShellFromVSCode(), which returnednullwhen no explicitterminal.integrated.defaultProfile.windowswas set.getShell()then fell through toCOMSPEC(cmd.exe), which is not what VS Code launches.Fix
When no Windows terminal profile is configured,
getWindowsShellFromVSCode()now returns Windows PowerShell (VS Code's built-in default) instead ofnull. Windows PowerShell is always present and allowlisted, so it's a safe default.Explicitly configured profiles are unchanged: PowerShell 7 / legacy / WSL / custom paths and an explicit Command Prompt profile all still resolve as before.
Testing
Updated the no-profile Windows tests to assert PowerShell, kept allowlist-safety coverage (non-allowlisted profile path → safe fallback), and added a test confirming an explicit Command Prompt profile still yields cmd.exe.
Summary by CodeRabbit