Uh oh!
There was an error while loading. Please reload this page.
fix(shared): prefer Windows PowerShell 5.1 over optional pwsh - #6260
fix(shared): prefer Windows PowerShell 5.1 over optional pwsh#6260RioPlay wants to merge 1 commit into
Conversation
Windows env probes preferred pwsh first, which skips the host every Windows install has and makes tooling depend on an optional install. Try powershell.exe first, then pwsh. Share one candidate list from shared/shell; desktop hydration and terminal fallback follow it.
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
ApprovabilityVerdict: Needs human review This PR changes the default Windows shell preference order across multiple apps, affecting which shell is spawned for all Windows users. Runtime behavior changes to shell selection logic warrant human verification, especially from a first-time contributor. You can customize Macroscope's approvability policy. Learn more. |
CDVolvik
left a comment
There was a problem hiding this comment.
Reordering the probe candidates makes sense on its own: powershell.exe is always present, so trying pwsh.exe first costs a failed spawn on every machine without PowerShell 7. But the two editions do not emit the same bytes, and the probe script does not pin an encoding.
captureWindowsEnvironmentCommand just does Write-Output $value, while the Node side reads with { encoding: "utf8" } (shell.ts:386). pwsh defaults [Console]::OutputEncoding to UTF-8, so that pairing works. Windows PowerShell 5.1 defaults it to the console codepage, which on a stock install is OEM 437 or ANSI 1252.
Running the real read path (execFileSync with encoding: "utf8") against the emitted probe script, with PATH set to C:\Users\José\AppData\Roaming\npm:
expected : "C:\Users\José\AppData\Roaming\npm"
UTF-8 console mode : "C:\Users\José\AppData\Roaming\npm" MATCH
stock OEM cp437 : "C:\Users\Jos?\AppData\Roaming\npm" CORRUPTED
stock ANSI cp1252 : "C:\Users\Jos?\AppData\Roaming\npm" CORRUPTED
explicit UTF-8 : "C:\Users\José\AppData\Roaming\npm" MATCH
Today 5.1 is the fallback, so this only bites when pwsh is missing. After this change it is the first candidate everywhere, so the corrupting path becomes the default for any user whose profile directory or tool paths contain non-ASCII characters. It fails quietly: the markers are ASCII so extraction still succeeds, and you get a PATH with mangled entries rather than an error.
Worth noting this also reaches FNM_DIR and FNM_MULTISHELL_PATH, which go through the same capture.
One line at the top of captureWindowsEnvironmentCommand covers it and makes 5.1-first safe:
"[Console]::OutputEncoding = [System.Text.Encoding]::UTF8",The same applies to the shared readEnvironmentFromWindowsShell command in packages/shared/src/shell.ts.
Separate concern in the same diff: defaultShellResolver and resolveShellCandidates in apps/server/src/terminal/Manager.ts are not probes, they pick the user's interactive terminal. Moving pwsh.exe below powershell.exe there means someone who installed PowerShell 7 now opens terminals in 5.1. The always-present argument is right for a headless probe and backwards for a shell the user chose to install. Those two orderings probably want to be separate constants rather than one shared list.
Problem
On Windows, shell env probes preferred
pwshfirst. That skips the PowerShell every Windows install already has (5.1 /powershell.exe) and makes tooling depend on an optional install.Fix
Try
powershell.exefirst, thenpwsh.exe. Share one candidate list fromshared/shellso desktop hydration and terminal fallback follow the same order.Verification
In-app T3 Terminal on this branch (not the agent outer shell):
System.Collections.Hashtable.PSVersion→ 5.1.26100.8655(Get-Process -Id $PID).Path→C:\WINDOWS\System32\WindowsPowerShell\v1.0\powershell.exeTest plan
Model: Grok 4.5 · Harness: Grok Build
Note
Medium Risk
Changes the default Windows shell used for terminals and environment probing, which can affect Windows startup and shell behavior. Fallback to pwsh/cmd remains, so risk is moderate rather than high.
Overview
Prefers built-in Windows PowerShell 5.1 (
powershell.exe) over optional PowerShell 7 (pwsh.exe) for Windows shell env probes and terminal startup.Exports a shared
WINDOWS_POWERSHELL_CANDIDATESlist fromshared/shelland uses it in desktop env hydration. Terminal fallback order now tries the absolute 5.1 path /powershell.exebeforepwsh.exe, so Windows installs no longer depend on an optionalpwshinstall.Reviewed by Cursor Bugbot for commit 99da444. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Prefer Windows PowerShell 5.1 (
powershell.exe) overpwshfor shell resolution on Windowspowershell.exebeforepwsh.exe, since PowerShell 5.1 is built-in whilepwshis optional.WINDOWS_POWERSHELL_CANDIDATESfrompackages/shared/src/shell.tsand uses it inapps/desktop/src/shell/DesktopShellEnvironment.tsto replace local literals.readEnvironmentFromWindowsShellto probepowershell.exefirst, falling back topwsh.exe.powershell.exeinstead ofpwsh.exe; systems where onlypwshis installed will fall back correctly.Macroscope summarized 99da444.