ci: run the test job on windows-latest too - #392
Conversation
The Windows-specific code in this repo - where.exe executable resolution, PATHEXT-aware lookups, and the cmd.exe routing that .cmd shims need since the CVE-2024-27980 fix - has never been exercised by CI. Every regression in it so far was found by a user on Windows or by reading the code, which is why three separate PRs were needed to fix one defect class. The matrix adds windows-latest with fail-fast disabled so a Windows-only failure does not hide the Linux result (and vice versa).
The new Windows CI leg surfaced ten failures in three groups. Six were my own tests from #391 asserting a bare `cmd.exe`. A real Windows host has COMSPEC set - the runner reports C:\Windows\system32\cmd.exe - so planShimSpawn returns that absolute path and the assertions only held on a host where COMSPEC was unset. The platform was already faked in those tests; comspec now is too, and a new case covers the COMSPEC fallback and the bare cmd.exe default that is left when it is unset. Three came from Git's CRLF conversion on Windows checkouts. generate-docs compares generated sections against the file on disk, so with CRLF on disk every section read as stale and `--check` exited 1; the plugins.txt parser kept a trailing \r on each name. .gitattributes pins text checkouts to LF, which is what those byte comparisons assume, and the plugins.txt parse tolerates CRLF so a checkout setting cannot look like a plugin mismatch. The last one asserted that writing to C:\Windows\System32 throws. The runner is elevated, so the write succeeded - and left a file in System32. It now writes through a parent that is a regular file, which no privilege level can turn into a directory.
![revuto-review[bot]](https://avatars.githubusercontent.com/u/55848801?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the Windows CI leg plus the test fixes. Both matrix legs are green on this head SHA (test (ubuntu-latest), test (windows-latest)), and the substance of the test changes checks out:
atomic-write: the new Windows branch works becausewriteFileAtomicdoesfs.existsSync(dirname)on a path that is a file, so it skipsmkdirSyncandwriteFileSyncfails with ENOTDIR regardless of elevation — a real refusal, unlike the old System32 write.comspecpinning:planShimSpawnreadsoptions.comspec || process.env.comspec || 'cmd.exe'(lib/utils/command-parser.js:137), so on a real Windows host the un-pinned tests would have compared againstC:\Windows\system32\cmd.exe. Each mutation is restored in afinallywith the correctundefinedvs. value distinction.plugins.txt/\r?\n/split matches the repo's CRLF rule, and.gitattributes(* text=auto eol=lf) is a no-op for the current index (every tracked text file is alreadyi/lf).
One operational item below.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest] |
There was a problem hiding this comment.
This is an auto review done by revuto.
Adding the matrix renames the check run, which will break any branch protection rule that requires the old name.
At the base commit (5b46d8e) the check run is named plain test. At this head the same job reports as test (ubuntu-latest) and test (windows-latest) — GitHub appends the matrix values to the job name, and there is no longer any check named test.
If test is listed as a required status check on main, every PR will sit at "Expected — Waiting for status to be reported" forever after this merges (this PR itself currently reports mergeable_state: blocked). Two ways out:
- update the branch-protection / ruleset required checks to
test (ubuntu-latest)andtest (windows-latest)at merge time, or - add a small aggregator job named
testwithneeds: [test]and keep the matrix job under a different id, so the required-check name stays stable as legs are added or removed.
Worth confirming before merge since the repo's own agnix job is referenced by name elsewhere and required checks aren't visible in-tree.
Why
The Windows-specific code in this repo has never run in CI:
where.exeexecutable resolution and PATHEXT-aware lookups (resolveExecutableForPlatform)cmd.exe /d /s /crouting that.cmdshims require since the CVE-2024-27980 fix (planShimSpawn)Every defect in that area so far - #388, #389/#390, #391 - was found by a user hitting it on Windows or by reading the code. The unit tests fake
process.platform, which proves the planner's arithmetic but never proves the plan actually spawns anything on a real Windows host.What
testbecomes a 2-leg matrix:ubuntu-latest+windows-latest,fail-fast: falseso a Windows-only failure does not mask the Linux result.No source changes. This PR's job is to tell us whether the suite passes on Windows; anything it surfaces gets fixed on top.
Test plan
The PR's own CI run is the test.