fix(phase4): execute Windows WSL scripts without BOM - #82
Merged
Conversation
PowerShell 5 adds a UTF-8 BOM when piping text to wsl.exe, so bash received `test` as the first command. A non-login `bash -s` also omitted the installed node path. Write each command to a unique runner-temp script as UTF-8 without BOM, mount that file through /mnt, and execute it from a login shell. The exact transport passed under the real limited helm-ph4 account on VM 115 with an isolated WSL distribution, including nested substitutions and cleanup. Keep structural coverage for no-BOM file transport and login-shell execution. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesWindows WSL execution
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 2
🤖 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 `@ops/platform-acceptance/windows.ps1`:
- Around line 36-45: Update the temporary script handling around $script so
creation, path conversion, and WSL execution all occur inside the try block.
Initialize $script before try, guard cleanup in finally so it only runs when a
path was assigned, and use Remove-Item -LiteralPath for deletion.
- Around line 38-43: Update the temporary script execution in the PowerShell
flow around WriteAllText and the WSL bash invocation so every command failure
propagates instead of only the final command status; use bash’s errexit behavior
or prepend set -e to the generated script. Add a regression case in
Assert-DistroVersion coverage with a failing first command followed by a
successful final command, and if changing the invocation to bash -e, update the
corresponding expectation in phase4-platform-acceptance.mjs.
🪄 Autofix
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: 02152b7a-9a7a-4b1b-97e3-a107f735c013
📒 Files selected for processing (2)
ops/platform-acceptance/windows.ps1test/phase4-platform-acceptance.mjs
Comment on lines
+36
to
+45
| $script = Join-Path $tempRoot ("1helm-distro-{0}.sh" -f [guid]::NewGuid().ToString('N')) | ||
| $encoding = New-Object System.Text.UTF8Encoding($false) | ||
| [IO.File]::WriteAllText($script, $Command + "`n", $encoding) | ||
| $drive = $script.Substring(0, 1).ToLowerInvariant() | ||
| $scriptInDistro = "/mnt/$drive/" + ($script.Substring(3) -replace '\\', '/') | ||
| try { | ||
| & $Wsl -d $Distro -u root --exec /bin/bash -lc "bash '$scriptInDistro'" | Out-Host | ||
| if ($LASTEXITCODE -ne 0) { Refuse "in-distribution command failed: $Command" } | ||
| } finally { | ||
| Remove-Item $script -Force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep cleanup active during temporary-file creation.
Line 41 starts try after WriteAllText and path conversion. If either operation fails, finally does not run and a temporary script can remain in the runner temp directory.
Move creation, path conversion, and execution inside try. Initialize $script before try, guard the cleanup, and use Remove-Item -LiteralPath.
🤖 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 `@ops/platform-acceptance/windows.ps1` around lines 36 - 45, Update the
temporary script handling around $script so creation, path conversion, and WSL
execution all occur inside the try block. Initialize $script before try, guard
cleanup in finally so it only runs when a path was assigned, and use Remove-Item
-LiteralPath for deletion.
Comment on lines
+38
to
+43
| [IO.File]::WriteAllText($script, $Command + "`n", $encoding) | ||
| $drive = $script.Substring(0, 1).ToLowerInvariant() | ||
| $scriptInDistro = "/mnt/$drive/" + ($script.Substring(3) -replace '\\', '/') | ||
| try { | ||
| & $Wsl -d $Distro -u root --exec /bin/bash -lc "bash '$scriptInDistro'" | Out-Host | ||
| if ($LASTEXITCODE -ne 0) { Refuse "in-distribution command failed: $Command" } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate failures from every command in the temporary script.
Line 42 runs bash without -e, and Line 38 does not prepend set -e. Bash returns the status of the final command. In Assert-DistroVersion, a failed service check followed by a successful version check returns 0, so Line 43 records false success.
Run the script with bash -e or write set -e as the first line. Add a regression case with a failed first command and a successful final command. If bash -e is used, update Line 217 in test/phase4-platform-acceptance.mjs.
🤖 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 `@ops/platform-acceptance/windows.ps1` around lines 38 - 43, Update the
temporary script execution in the PowerShell flow around WriteAllText and the
WSL bash invocation so every command failure propagates instead of only the
final command status; use bash’s errexit behavior or prepend set -e to the
generated script. Add a regression case in Assert-DistroVersion coverage with a
failing first command followed by a successful final command, and if changing
the invocation to bash -e, update the corresponding expectation in
phase4-platform-acceptance.mjs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The last real Windows run proved stdin preserved shell quoting, but its log exposed two PowerShell 5 boundary effects: stdin added a UTF-8 BOM (
test), and non-loginbash -somitted the installednodepath. The helper now writes each command to a unique runner-temp file as UTF-8 without BOM, mounts it into WSL, and executes it from a login shell.Verification
helm-ph4account on VM 115 with an isolated WSL distribution, nested command substitutions, login PATH, and cleanup;npm run ci: 177 tests, 175 passed, 2 skipped, 0 failed.Only the Windows acceptance helper and its regression test changed. No version bump, tag, release, Stable promotion, website deploy, or production change.
🤖 Generated with Claude Code
Summary by CodeRabbit