Skip to content

fix(phase4): preserve Windows WSL commands over stdin - #81

Merged
gitcommit90 merged 1 commit into
mainfrom
fix/windows-wsl-stdin-transport
Aug 5, 2026
Merged

fix(phase4): preserve Windows WSL commands over stdin#81
gitcommit90 merged 1 commit into
mainfrom
fix/windows-wsl-stdin-transport

Conversation

@gitcommit90

@gitcommit90 gitcommit90 commented Aug 5, 2026

Copy link
Copy Markdown
Owner

What changed

PowerShell 5 re-quotes native command arguments before invoking wsl.exe, which mangled the otherwise-literal service/version assertion into active) = active. The Windows acceptance helper now sends all in-distribution scripts over stdin to bash -s rather than as a bash -lc argument.

Verification

  • isolated transport proof passed under the actual limited helm-ph4 account on VM 115 and cleaned its temporary WSL distribution;
  • updated script parsed in the actual VM 115 Windows PowerShell;
  • focused Phase 4 suite: 8 passed, 0 failed;
  • full npm run ci: 177 tests, 175 passed, 2 skipped, 0 failed.

This changes only ops/platform-acceptance/windows.ps1 and its regression test. No version bump, tag, release, Stable promotion, website deploy, or production change.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved Windows command execution through WSL, preserving command text and preventing quoting or substitution errors.
    • Ensured commands run consistently through the expected root Bash process.
  • Tests

    • Added acceptance coverage to verify the updated WSL execution behavior and prevent regression to the previous invocation method.

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>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Windows acceptance commands now pass through WSL standard input to root /bin/bash -s. Acceptance tests verify this invocation and reject the previous /bin/bash -lc pattern.

Changes

Windows WSL command piping

Layer / File(s) Summary
Command piping and validation
ops/platform-acceptance/windows.ps1, test/phase4-platform-acceptance.mjs
Invoke-Distro pipes command text to /bin/bash -s. Windows acceptance checks verify the piped root invocation and reject /bin/bash -lc.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

  • gitcommit90/1Helm#59: Refines the same Windows WSL command invocation path and adds acceptance tests.
  • gitcommit90/1Helm#70: Introduces the Windows WSL acceptance runner that this change further refines.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the Windows WSL stdin transport fix and matches the main change.
Description check ✅ Passed The description explains the problem, implementation, scope, and verification results; omitted template checkboxes are non-critical for this single-item fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/windows-wsl-stdin-transport

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2bf7aaf and 0c674d6.

📒 Files selected for processing (2)
  • ops/platform-acceptance/windows.ps1
  • test/phase4-platform-acceptance.mjs

Comment on lines +215 to +216
assert.match(windows, /\$Command \| & \$Wsl -d \$Distro -u root --exec \/bin\/bash -s/);
assert.doesNotMatch(windows, /\/bin\/bash -lc \$Command/);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

@gitcommit90
gitcommit90 merged commit 2606bef into main Aug 5, 2026
6 checks passed
@gitcommit90
gitcommit90 deleted the fix/windows-wsl-stdin-transport branch August 5, 2026 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant