fix(phase4): preserve Windows WSL commands over stdin - #81
Conversation
Windows PowerShell 5 re-quotes native command arguments before invoking wsl.exe. That mangled the otherwise-literal service/version assertion into `active) = active` inside bash. Pass all in-distribution command scripts over stdin to `bash -s` instead of as a `bash -lc` argument. The exact transport was proven under the real limited helm-ph4 runner account on VM 115 with an isolated WSL distribution. Keep a structural regression test that requires stdin transport and rejects the argument form. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughWindows acceptance commands now pass through WSL standard input to root ChangesWindows WSL command piping
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/phase4-platform-acceptance.mjs`:
- Around line 215-216: Strengthen the negative assertion in the Windows command
test so it rejects any argument-based `-lc` invocation following `--exec
/bin/bash`, regardless of quoting, whitespace, or the specific command argument.
Keep the existing positive assertion unchanged and update only the doesNotMatch
pattern around the windows command output.
🪄 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: 2c9b9a44-0c79-46ee-9e81-1079ef34a8c8
📒 Files selected for processing (2)
ops/platform-acceptance/windows.ps1test/phase4-platform-acceptance.mjs
| assert.match(windows, /\$Command \| & \$Wsl -d \$Distro -u root --exec \/bin\/bash -s/); | ||
| assert.doesNotMatch(windows, /\/bin\/bash -lc \$Command/); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject all argument-based -lc forms.
The negative assertion matches only /bin/bash -lc $Command. A future edit could add quotes or different whitespace and still pass while using argument transport. Match the --exec /bin/bash -lc shape independently of $Command.
Proposed test adjustment
- assert.doesNotMatch(windows, /\/bin\/bash -lc \$Command/);
+ assert.doesNotMatch(windows, /--exec\s+\/bin\/bash\s+-lc\b/);📝 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.
| assert.match(windows, /\$Command \| & \$Wsl -d \$Distro -u root --exec \/bin\/bash -s/); | |
| assert.doesNotMatch(windows, /\/bin\/bash -lc \$Command/); | |
| assert.match(windows, /\$Command \| & \$Wsl -d \$Distro -u root --exec \/bin\/bash -s/); | |
| assert.doesNotMatch(windows, /--exec\s+\/bin\/bash\s+-lc\b/); |
🤖 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/phase4-platform-acceptance.mjs` around lines 215 - 216, Strengthen the
negative assertion in the Windows command test so it rejects any argument-based
`-lc` invocation following `--exec /bin/bash`, regardless of quoting,
whitespace, or the specific command argument. Keep the existing positive
assertion unchanged and update only the doesNotMatch pattern around the windows
command output.
What changed
PowerShell 5 re-quotes native command arguments before invoking
wsl.exe, which mangled the otherwise-literal service/version assertion intoactive) = active. The Windows acceptance helper now sends all in-distribution scripts over stdin tobash -srather than as abash -lcargument.Verification
helm-ph4account on VM 115 and cleaned its temporary WSL distribution;npm run ci: 177 tests, 175 passed, 2 skipped, 0 failed.This changes only
ops/platform-acceptance/windows.ps1and its regression test. No version bump, tag, release, Stable promotion, website deploy, or production change.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests