ci: put the SSH suite on the Windows lane - #180
Merged
Conversation
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
11 tasks
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
…it (#183) The same defect turned up three times across #177 and #180: a hard-coded absolute POSIX path meeting a path the code builds with path.join. On Windows path.join emits backslashes and path.resolve prepends a drive, so the two sides can never be equal and the assertion is dead on that platform — silently, because the file had only ever run on Linux. Two were found by the Windows lane going red; the third by reading a file before adding it. One at a time, after the fact, on a required check. The rule does NOT ban leading-slash literals. A blanket ban is unusable: the seven lane files hold 71 absolute POSIX literals, and all but the three above are inputs — a fake execPath, a ControlPath handed to ssh, a path the stub filesystem is asked about. Coping with those is the job of the code under test. The defect is where a literal meets a computed path, so the rule keys on that meeting: path.join onto a POSIX root (fix: wrap in path.resolve, a no-op on POSIX), a comparison against a POSIX literal, and a regex that opens with a path separator. "Opens with" rather than "contains" keeps the shebang check and the remote-shell-command assertion — both legitimately POSIX, both in lane files today — quiet. Scoped to the suites in test:desktop:win-install, and that list is parsed from package.json rather than restated in the config. A second copy fails silently: a suite added to the lane but missing from the config gets no cross-platform linting at exactly the moment it starts needing it. Verified by observation, not assertion. Run over the pre-fix files (1770376^, 50e2b2b^) the rule reports exactly the three known defects and nothing else, and none of them on the current files. The self-maintenance claim was proved the same way: a probe file with a known defect drew no report until its name alone was added to the lane script — no config edit — after which it did. Two false positives are kept and annotated rather than engineered away. windows-hermes-path compares against /root/venv/Scripts/python.exe twice, and both are safe because every path helper that resolver touches is injected by makeDeps and joins with '/'. The disables carry that reason, which is the rule earning its keep — "why is this POSIX literal safe on Windows?" is what the next person adding a suite needs answered. The rule's own tests caught two bugs in it first: expect(x).toBe(v) puts the expected value at argument 0, not 1, so expect-style assertions were never checked; and the lane parser consumed the token after every flag, so a boolean -u swallowed the suite name behind it. It now consumes values only for known value-taking flags, erring towards a glob that matches nothing rather than silently dropping a suite. Separately, the first disable comments did not work at all — eslint-disable-next-line followed by more comment lines applies to the next comment. Found by running lint, not by reading it. lint now covers eslint-rules/ and scripts/*.test.mjs, because three lane suites live in scripts/ and were outside `eslint src/ electron/`. The rest of scripts/ stays unlinted; it carries 10 pre-existing errors that are not this change. eslint becomes a declared root devDependency — it was only ever an auto-installed peer, and the rule's tests import RuleTester from it. The lockfile delta is mechanical peer-marker removal with no version movement. check:lint 0 errors (89 warnings, unchanged), test:desktop:platforms 976 passed, lane script 117 passed. 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
2 participants
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.
The last open item from the Windows-lane work. Seven files in the lane now.
Inspected before adding, not added and hoped
Over-including unverified files caused rounds #174 and #175, and since #176 this lane is a required check — so being wrong now blocks everyone's PRs, not 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:
Now asserted on
path.basename(a)— separator-independent, and it's what thesun_pathlimit is actually about. Verified under both flavours.Everything else is safe for a stated reason
Not "looks fine" — each one has a specific reason:
!p.includes('/var/folders/')ControlPath=/tmp/x.sockbaseSshOptionsverbatim; it does no path joiningcd '/home/me/project'path.join(os.tmpdir(), …)×6fs.symlinkSync,mode: 0o755, mode assertionsAlso corrects a header #179 made false
The workflow still said ssh-connection was POSIX-only and that making it cross-platform was "tracked as follow-up" — which is exactly what #179 did. Both comment blocks rewritten so the file doesn't misdescribe the repo.
The risk, plainly
The remaining exposure is unknown-unknowns across ~40 tests I can't execute from this sandbox. The inspection was systematic and everything it surfaced is fixed, but a Windows-only failure would land on a required check.
If the lane goes red on this, the fix is to delete
ssh-connectionfrom the list — one word. The test repairs in #179 and here stand on their own regardless; nothing needs reverting.Generated by Claude Code