test(ssh): state mux explicitly so the ControlMaster tests are platform-stable - #179
Merged
Merged
Conversation
…rm-stable Correcting my own claim first: I recorded across five PR bodies that Windows' no-mux SSH path had no test coverage. That was false. ssh-connection.test.ts has seven dedicated no-mux tests covering every one of the five mux-conditional branches in the source. I inferred the gap from seeing the suite fail on Windows and never checked. What is actually wrong is close to the inverse. All eight Windows failures were in *mux* tests that never set mux, so they inherited `opts.mux ?? process.platform !== 'win32'`, silently became no-mux objects on Windows, and their ControlMaster assertions could not hold. So mux: true is now stated on all sixteen ControlMaster constructions. controlDir is the exact marker — it is meaningless without a master — which makes the edit mechanical and complete rather than judged case by case. Provably a no-op on Linux: the only value changed is opts.mux, and both `undefined ?? true` and `true ?? true` resolve to true there, so this cannot regress the platform CI runs today. Simulated across linux/darwin/win32: the mux tests go false → true on win32 only, and the seven no-mux tests stay false everywhere. Also found a second Windows problem that mux: true does not fix: the control-dir safety block in open() is itself wrapped in `process.platform !== 'win32'`, so the symlink-rejection test has no rejection to assert there. Added the same platform guard the 0700 test three lines below already uses. Not added to the Windows lane. This makes the suite capable of running there, but 40+ tests cannot be verified from this sandbox, and over-including unverified files caused two earlier rounds on that lane. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
The last open item from the Windows-lane work. Seven files now. Inspected before adding rather than adding and hoping: over-including unverified files caused rounds #174 and #175, and since #176 the lane is a required check, so being wrong now blocks everyone's PRs rather than just mine. The sweep covered Windows-hostile syscalls, separator-dependent assertions and shell assumptions. It found a third instance of the defect class behind #177: `assert.match(a, /\/[0-9a-f]{16}\.sock$/)` against controlSocketPath, which builds with path.join — a leading-slash regex can only match on POSIX. Now asserted on path.basename(a), which is separator-independent and is what the sun_path limit is actually about. Checked under both path flavours. Everything else in the file is safe for stated reasons rather than by assumption: the /var/folders/ check is a negative assertion; ControlPath passes through baseSshOptions verbatim, which does no path joining; the `cd '/home/me/project'` assertion is a remote shell command and remotes are always POSIX; and the path.join calls create directories inside the two tests that already carry win32 guards. Also corrects the workflow header, which #179 had made false — it still said ssh-connection was POSIX-only and that making it cross-platform was tracked as follow-up, which is precisely what #179 did. Remaining exposure is unknown-unknowns across ~40 tests that cannot be executed from this sandbox. If the lane goes red on this, the fix is to drop ssh-connection back out of the list — one word, and the test repairs stand on their own. Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 Co-authored-by: Claude <noreply@anthropic.com>
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
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.
Correcting myself first
Across #174–#178 I recorded that "Windows' no-mux SSH path has no test coverage at all." That was false.
ssh-connection.test.tshas seven dedicatedno-mux:tests, covering every one of the five mux-conditional branches inssh-connection.ts:ControlMaster/ControlPath/ControlPersistopen()verifies with a one-shot exec, no-Mopen()classifies auth failureforward()spawns the persistent-N -Lchild;cancelForward+closekill itI inferred the gap from watching the suite fail on Windows and never opened the file to check. It was cheap to check.
What's actually wrong is close to the inverse
All eight Windows failures were in mux tests that never set
mux, so they inherited:On Windows that resolves
false— the tests silently became no-mux connections, and their ControlMaster assertions couldn't hold. The suite wasn't missing no-mux coverage; its mux coverage was quietly evaporating on the one platform it was never run on.The change
mux: truestated on all sixteen ControlMaster constructions.controlDiris the exact marker — it's meaningless without a master — so the edit is mechanical and complete rather than judged case-by-case.Provably a no-op on Linux. The only value changed is
opts.mux, and bothundefined ?? trueandtrue ?? trueresolve totruethere. A regression on the platform CI runs today isn't possible from this change. Simulated across all three platforms:A second Windows problem
mux: truedoes not fixThe control-dir safety block in
open()is itself wrapped inprocess.platform !== 'win32'— soopen() rejects a control-dir that is a symlinkhas no rejection to assert on Windows. Added the sameif (process.platform === 'win32') returnguard that the 0700 test three lines below already uses. Worth knowing that explicitmuxalone would not have been sufficient.Two sub-claims from my earlier triage, also corrected
createSshProbeConnectionalready forcesmux: falseinternally — that test needed nothing.close()test I listed as already mux-explicit was a false grep hit on the string'mux: master gone'in a stderr fixture. All eight failures share one cause, which is simpler than I reported.Not included
ssh-connection.test.tsis not added to the Windows lane. This makes the suite capable of running there, but it's 40+ tests I can't execute from here, and over-including unverified files caused rounds #174 and #175. Adding it is a separate, checkable step — and given the lane is a required check since #176, the cost of being wrong is now everyone's PRs, not just mine.Generated by Claude Code