Fix fresh Windows and Linux runtime setup - #49
Conversation
Signed-off-by: Joseph Yaksich <gitcommit90@users.noreply.github.com>
📝 WalkthroughWalkthroughThis release updates v0.0.33 references, improves Windows WSL setup error handling, and adds SHA-256-verified x64 and arm64 Cloudflared binaries. Runtime resolution, installation, updates, and tests now validate the matching architecture-specific connector. ChangesRelease hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant packageLinuxHost
participant ReleaseArchive
participant install.sh
participant update-host.sh
participant connectorBinary
packageLinuxHost->>ReleaseArchive: Add verified executable x64 and arm64 cloudflared assets
install.sh->>ReleaseArchive: Validate the host-architecture connector
update-host.sh->>ReleaseArchive: Validate the CONNECTOR_ARCH connector
connectorBinary->>ReleaseArchive: Resolve the packaged host-architecture connector
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
test/desktop.mjs (1)
194-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound this assertion to the HostSetup block.
The
[\s\S]*segments are unbounded. The script-leveltryat line 186 andcatchat line 300 ofscripts/install-wsl-runtime.ps1already satisfy this pattern. The test therefore still passes if thetry/catchinsideif ($HostSetup)is removed, so it does not guard the behavior it describes. Anchor the match onexit 0immediately before the HostSetupcatch.💚 Proposed tighter assertion
- assert.match(windowsRuntime, /if \(\$HostSetup\)[\s\S]*try \{[\s\S]*catch \{[\s\S]*Fail-Setup \$message/, "unexpected elevated host-setup errors are written to shared status instead of collapsing to an unexplained exit code"); + assert.match(windowsRuntime, /if \(\$HostSetup\) \{\s*try \{[\s\S]*?\n\s*exit 0\r?\n\s*\} catch \{[\s\S]*?Fail-Setup \$message/, "unexpected elevated host-setup errors are written to shared status instead of collapsing to an unexplained exit code");🤖 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 `@test/desktop.mjs` at line 194, Update the windowsRuntime assertion in the desktop test to scope the try/catch match to the if ($HostSetup) block by requiring exit 0 immediately before its catch. Keep the assertion checking that the HostSetup catch invokes Fail-Setup $message, while preventing script-level try/catch blocks from satisfying the pattern.test/connectors.mjs (1)
8-20: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftAdd behavioral coverage for the packaging and digest contract.
This test checks source text only. It does not prove that both binaries enter the archive, retain executable mode, or fail on a digest mismatch. It also does not exercise resolution from
appRoot/resources.Add a deterministic fixture or mocked packaging test, plus a runtime resolver test for the matching architecture.
🤖 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 `@test/connectors.mjs` around lines 8 - 20, Add behavioral coverage beyond the source-text assertions in the packaging and resolver tests: use a deterministic fixture or mocked packaging flow to verify both architecture-specific cloudflared binaries enter the Linux archive with executable mode and reject digest mismatches, and add a runtime test exercising connector resolution from appRoot/resources for the host architecture. Anchor changes around the Linux packaging entry point and the connector resolver used by the existing test.
🤖 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 `@scripts/install-wsl-runtime.ps1`:
- Around line 171-173: Update the finally cleanup block around $hostTemporary so
Remove-Item is best-effort and cannot replace the original failure under the
script-wide Stop policy. Suppress or locally handle cleanup errors while
retaining the existing Test-Path, recursive, and forced removal behavior,
allowing the original elevated setup error to reach catch and Fail-Setup
unchanged.
- Line 147: Update the Start-Process invocation assigning $installer to
explicitly quote the $msi path in the -ArgumentList passed to msiexec.exe,
matching the quoting approach already used near line 196 so paths containing
spaces install correctly.
- Around line 45-52: Update the WSL invocation block around `$raw` and `$code`
to initialize `$code` to a nonzero failure value before calling `& $wsl`, then
use `$?` to confirm the command launched successfully before accepting
`$LASTEXITCODE`. Preserve the existing exit-code handling for successfully
launched `wsl.exe`, while ensuring missing or blocked executables fail closed
and cannot reuse a stale `$LASTEXITCODE`.
---
Nitpick comments:
In `@test/connectors.mjs`:
- Around line 8-20: Add behavioral coverage beyond the source-text assertions in
the packaging and resolver tests: use a deterministic fixture or mocked
packaging flow to verify both architecture-specific cloudflared binaries enter
the Linux archive with executable mode and reject digest mismatches, and add a
runtime test exercising connector resolution from appRoot/resources for the host
architecture. Anchor changes around the Linux packaging entry point and the
connector resolver used by the existing test.
In `@test/desktop.mjs`:
- Line 194: Update the windowsRuntime assertion in the desktop test to scope the
try/catch match to the if ($HostSetup) block by requiring exit 0 immediately
before its catch. Keep the assertion checking that the HostSetup catch invokes
Fail-Setup $message, while preventing script-level try/catch blocks from
satisfying the pattern.
🪄 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: a5617a05-665e-4bc7-a305-a1324ae378c6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
CHANGELOG.mdREADME.mdpackage.jsonscripts/install-wsl-runtime.ps1scripts/package-linux-host.mjssite/public/install.shsite/public/update-host.shsrc/server/channel-computers.tssrc/server/connectors.tssrc/server/db.tstest/channel-computers.mjstest/connectors.mjstest/desktop.mjstest/site.mjs
| $previousErrorAction = $ErrorActionPreference | ||
| try { | ||
| $ErrorActionPreference = "Continue" | ||
| $raw = & $wsl @ArgumentList 2>&1 | Out-String | ||
| $code = $LASTEXITCODE | ||
| } finally { | ||
| $ErrorActionPreference = $previousErrorAction | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not trust $LASTEXITCODE when wsl.exe never runs.
$code is set only from $LASTEXITCODE. If & $wsl cannot start (wsl.exe absent from System32, or launch blocked), no native exit code is produced. $LASTEXITCODE then holds a stale value from an earlier native call, or $null on first use. A stale 0 makes the probe report success and makes Get-WslDistributionNames return the error text as distribution names. Default $code to a nonzero value and check $?.
🛡️ Proposed fix to fail closed on launch failures
$previousErrorAction = $ErrorActionPreference
+ $raw = ""
+ $code = 1
try {
$ErrorActionPreference = "Continue"
$raw = & $wsl `@ArgumentList` 2>&1 | Out-String
- $code = $LASTEXITCODE
+ $launched = $?
+ $code = $LASTEXITCODE
+ if ($null -eq $code -or (-not $launched -and $code -eq 0)) { $code = 1 }
} finally {
$ErrorActionPreference = $previousErrorAction
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $previousErrorAction = $ErrorActionPreference | |
| try { | |
| $ErrorActionPreference = "Continue" | |
| $raw = & $wsl @ArgumentList 2>&1 | Out-String | |
| $code = $LASTEXITCODE | |
| } finally { | |
| $ErrorActionPreference = $previousErrorAction | |
| } | |
| $previousErrorAction = $ErrorActionPreference | |
| $raw = "" | |
| $code = 1 | |
| try { | |
| $ErrorActionPreference = "Continue" | |
| $raw = & $wsl `@ArgumentList` 2>&1 | Out-String | |
| $launched = $? | |
| $code = $LASTEXITCODE | |
| if ($null -eq $code -or (-not $launched -and $code -eq 0)) { $code = 1 } | |
| } finally { | |
| $ErrorActionPreference = $previousErrorAction | |
| } |
🤖 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 `@scripts/install-wsl-runtime.ps1` around lines 45 - 52, Update the WSL
invocation block around `$raw` and `$code` to initialize `$code` to a nonzero
failure value before calling `& $wsl`, then use `$?` to confirm the command
launched successfully before accepting `$LASTEXITCODE`. Preserve the existing
exit-code handling for successfully launched `wsl.exe`, while ensuring missing
or blocked executables fail closed and cannot reuse a stale `$LASTEXITCODE`.
| Fail-Setup "Microsoft WSL installer did not have a valid Microsoft Corporation signature." | ||
| } | ||
| Write-SetupStatus -Status "running" -Step "Installing Microsoft WSL $wslVersion..." -Progress 18 | ||
| $installer = Start-Process -FilePath "$env:SystemRoot\System32\msiexec.exe" -ArgumentList @("/i", $msi, "/qn", "/norestart") -Wait -PassThru |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Windows PowerShell 5.1 Start-Process ArgumentList array quoting spaces in path
💡 Result:
In Windows PowerShell 5.1, the Start-Process cmdlet's -ArgumentList parameter accepts either a single string or an array of strings [1][2]. When you provide an array, the cmdlet joins the elements into a single string by inserting a space between each element [1][3]. Crucially, Start-Process does not automatically handle quoting for array elements that contain spaces [4][5][6]. If an argument contains spaces, the resulting joined string may cause the target process to interpret the arguments incorrectly [7][5]. To pass arguments containing spaces correctly in PowerShell 5.1, you must manually include escaped double quotes around the specific argument within your string or array element [1][2][8]. Example using an array: Start-Process -FilePath "C:\Path\To\Executable.exe" -ArgumentList "FirstArg", ""C:\Path With Spaces\File.txt"", "ThirdArg" Example using a single string: Start-Process -FilePath "C:\Path\To\Executable.exe" -ArgumentList "/arg1 "C:\Path With Spaces\File.txt" /arg3" Official documentation recommends that for the most reliable results, you should use a single string for -ArgumentList and explicitly manage all necessary quotes and escaping [1][2][8]. This avoids potential issues with the automatic space-joining behavior of the array-based approach [5][6]. When using nested quotes, you can use the backtick (`) to escape the double quotes for the target process [1][8].
Citations:
- 1: https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/5.1/Microsoft.PowerShell.Management/Start-Process.md
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/start-process?view=powershell-7.4
- 3: https://github.com/PowerShell/PowerShell/blob/7a55bf98b2370ab4817ab2533cb67673053ee446/src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs
- 4: Start-Process doesn't pass arguments with embedded whitespace and double quotes correctly PowerShell/PowerShell#5576
- 5: https://stackoverflow.com/questions/79388953/running-a-powershell-start-process-command-with-arguments-containing-spaces-fr
- 6: Passing arguments with the Start-Process cmdlet PowerShell/PowerShell#18906
- 7: Start-Process doesn't pass arguments with embedded whitespace and double quotes correctly PowerShell/PowerShell#5576
- 8: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/start-process?view=powershell-7.6
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate script ---'
sed -n '1,230p' scripts/install-wsl-runtime.ps1
printf '%s\n' '--- PowerShell version references ---'
rg -n -i --glob '!node_modules' --glob '!dist' --glob '!build' 'PowerShell|pwsh|powershell.exe|Windows PowerShell|PSVersion' .
printf '%s\n' '--- installer-related references ---'
rg -n -C 3 'Start-Process|GetTempPath|wsl\.candidate\.msi|Test-PinnedWslRuntime|msiexec' scriptsRepository: gitcommit90/1Helm
Length of output: 19099
🌐 Web query:
Microsoft documentation Windows PowerShell 5.1 Start-Process -ArgumentList joins array elements with spaces does not add quotation marks
💡 Result:
In Windows PowerShell 5.1, the Start-Process cmdlet does not automatically add quotation marks to elements in an ArgumentList array [1][2]. Instead, it flattens the array by joining all elements into a single string, separated by a single space [1][3][2]. Because this process does not preserve the structure of the original array or apply automatic quoting, arguments containing spaces or special characters will be misinterpreted by the target executable unless you manually handle the quoting [1][3]. To ensure that arguments containing spaces are correctly passed to a child process, you must include escaped double quotes around those specific elements within your PowerShell code [3][2]. For example, if you need to pass a file path that contains spaces: # Incorrect: The executable will see 'C:\Program' and 'Files\app.txt' as two separate arguments Start-Process -FilePath "app.exe" -ArgumentList "C:\Program Files\app.txt" # Correct: Escaped quotes ensure the path is treated as a single argument Start-Process -FilePath "app.exe" -ArgumentList ""C:\Program Files\app.txt"" As noted in official documentation, for the most predictable results, you can use a single string for ArgumentList that includes all arguments and any necessary escaped quotes, rather than relying on an array [3][2]. Keep in mind that the backtick (`) is used in PowerShell to escape characters (like double quotes) for the command-line string construction [3][2].
Citations:
- 1: Document that Start-Process -ArgumentList does not preserve the argument vector or do any quoting MicrosoftDocs/PowerShell-Docs#7701
- 2: https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/5.1/Microsoft.PowerShell.Management/Start-Process.md
- 3: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/start-process?view=powershell-7.6
Quote the MSI path passed to msiexec.exe.
Windows PowerShell 5.1 joins -ArgumentList elements with spaces and does not add quotes. If the elevated account's temporary path contains spaces, msiexec.exe parses the MSI path as separate arguments and the fresh install fails. Use explicit quotes, as at line 196.
🤖 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 `@scripts/install-wsl-runtime.ps1` at line 147, Update the Start-Process
invocation assigning $installer to explicitly quote the $msi path in the
-ArgumentList passed to msiexec.exe, matching the quoting approach already used
near line 196 so paths containing spaces install correctly.
| } finally { | ||
| if (Test-Path -LiteralPath $hostTemporary) { Remove-Item -LiteralPath $hostTemporary -Recurse -Force } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make temporary cleanup best-effort so it cannot mask the real error.
The script-wide policy is Stop. If Remove-Item fails, for example because msiexec still holds the MSI, the cleanup error becomes the active exception, reaches the new catch, and Fail-Setup overwrites the status file with the cleanup message. The original elevated failure is then lost in the UI.
♻️ Proposed fix
} finally {
- if (Test-Path -LiteralPath $hostTemporary) { Remove-Item -LiteralPath $hostTemporary -Recurse -Force }
+ if (Test-Path -LiteralPath $hostTemporary) {
+ Remove-Item -LiteralPath $hostTemporary -Recurse -Force -ErrorAction SilentlyContinue
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } finally { | |
| if (Test-Path -LiteralPath $hostTemporary) { Remove-Item -LiteralPath $hostTemporary -Recurse -Force } | |
| } | |
| } finally { | |
| if (Test-Path -LiteralPath $hostTemporary) { | |
| Remove-Item -LiteralPath $hostTemporary -Recurse -Force -ErrorAction SilentlyContinue | |
| } | |
| } |
🤖 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 `@scripts/install-wsl-runtime.ps1` around lines 171 - 173, Update the finally
cleanup block around $hostTemporary so Remove-Item is best-effort and cannot
replace the original failure under the script-wide Stop policy. Suppress or
locally handle cleanup errors while retaining the existing Test-Path, recursive,
and forced removal behavior, allowing the original elevated setup error to reach
catch and Fail-Setup unchanged.
1 participant
Outcome
Fixes the two fresh-install failures found during 0.0.32 end-user acceptance:
wsl.exe --versionfailure, and elevated host-setup errors are preserved in shared setup status.Bumps the synchronized desktop candidate to 0.0.33.
Verification
npm run typechecknpm run buildnpm run test:onboarding-browser(20 passed)npm test(112 passed, 2 documented skips)bash -n site/public/install.sh site/public/update-host.shgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation