ci: run a real npm ci on Windows for the desktop app - #173
Merged
Conversation
Every JS/TS lane runs on ubuntu-latest, so the entire Windows dependency path — Electron's postinstall unpacking a dist through @electron/get and its native @electron-internal/extract-zip addon — has never been exercised by CI. The one Windows job that exists, installer-tests.yml, runs PowerShell tests against install.ps1 and installs nothing. The job installs with scripts enabled (the postinstall is the thing under test), then asserts node_modules/electron/dist/electron.exe actually exists: npm ci exits 0 when a postinstall is skipped, so its exit code alone would let a silently missing unpack read as green. It then runs the addon-load guard and the `electron` vitest project — the pin test plus the scripts build guards — on the platform that pin contract is about. Gated on the same `frontend` lane as js-tests, so Python-only PRs skip it. Not using .github/actions/retry: that composite is `shell: bash` and no Windows job here has ever used it, so this job would be its first — an unproven path inside a job that is itself new. Replaced with an inline pwsh loop matching the action's defaults; an Electron dist download is exactly the flaky-network case retries exist for. What this does NOT catch is stated in the workflow header rather than left for someone to discover: it cannot reproduce the ERR_DLOPEN_FAILED failure that motivated assert-win-vcruntime.mjs, because windows-latest ships Visual Studio Build Tools and so always has VCRUNTIME140.dll. The job stays green on a regression that breaks every clean developer machine. Reproducing that needs a runner image without the redistributable. Verification is structural only and that is the honest limit: both workflows parse, the job graph, step shells, pinned SHAs, the frontend gate, the npm script and every referenced path were checked. There is no Windows runner here, so the job's first real execution is on this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d9f50bb41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # does and does not cover — notably it cannot reproduce the missing-VC++ | ||
| # runtime failure, because the runner image always has it. | ||
| if: needs.detect.outputs.frontend == 'true' | ||
| uses: ./.github/workflows/desktop-install-windows.yml |
There was a problem hiding this comment.
When a frontend PR makes this called workflow fail, all-checks-pass neither waits for nor evaluates it because the aggregate's needs list at lines 254–269 omits desktop-install-windows; ci.yml explicitly states that branch protection requires only that aggregate. It can therefore report success—and permit merging—while the new Windows lane is still running or has failed, defeating the intended regression protection. Add this caller job to the aggregate's needs list.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
The job added in #173 ran for the first time on that PR and did its job immediately, then merged red because it is not a required check. The good news is the answer it was added to get: steps 1-7 passed. `npm ci` took 63 seconds, electron.exe was present, and the VC++ guard was satisfied — the Windows install path is healthy. Step 8 ran the whole `electron` vitest project, which was scope beyond the install path, and it failed in three groups: * ssh-connection.test.ts asserts OpenSSH ControlMaster multiplexing, which Windows OpenSSH never implemented. ssh-connection.ts knows this and deliberately runs a no-mux path there, so the suite asserts mux behaviour against a no-mux object. Not a product bug, and making it cross-platform is a separate change. * update-relaunch.test.ts:54 roots its fixture at the POSIX literal /home/u/…; on Windows path.resolve prepends a drive letter, so the resolved exec path can never equal the driveless expectation. * windows-hermes-path.test.ts:177 hard-codes a POSIX literal where its Windows sibling three tests above already builds the expectation with path.join. So the job now runs test:desktop:win-install — the pin contract plus the five scripts/*.test.mjs build guards, six files named explicitly. A directory filter would have to survive backslash separators, and a named list makes "what runs on Windows" reviewable in package.json. It is a test:* script, not check:*, because the js-tests matrix auto-discovers check:* and would otherwise run it on ubuntu as a second matrix entry. Checked those six for the same defect class rather than assuming: three carry POSIX literals and all three are safe, because they assert missingness (true on any OS) or pass the path only to a stub that matches on the command string. stage-native-deps.test.mjs already carries its own skipIf for win32. The six filters were verified to match exactly six of the project's 80 test files, with all three failing files excluded. Recorded as follow-up: Windows' no-mux SSH path has no coverage at all — the well-covered mux path is the one Windows never takes. Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 Co-authored-by: Claude <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
The narrowed job ran and got 21 tests passed, 0 test failures — but two of the six suites failed to *load*, so it was still red. Both #173 and #174 merged before their job finished (it is not a required check), so main carried a red job through both. Observed green on a Windows runner, quoted rather than inferred: desktop-electron-pin 4 tests, assert-win-vcruntime 6, assert-dist-built 5, write-build-stamp 6. The pin contract and the VC++ guard both hold on the platform they are about, which is the point of the lane. The two failures share one cause: stage-native-deps.mjs does `import { Arch } from 'electron-builder'`, and before-pack.mjs imports from stage-native-deps.mjs, so both pull electron-builder into the vitest module graph. Both fail with the same `SyntaxError: Invalid or unexpected token` on Windows while loading cleanly on ubuntu; the four that pass import no such thing. BOM, CRLF and stray control characters were compared across all five .mjs suites first and showed no difference, which is what pointed at the import graph. Not chased further on purpose: it is a vitest/vite transform problem with a third-party package, not this repo's code, and those two are packaging-stage guards rather than install-path checks — including them was scope creep on my part. The diagnosis is recorded in the workflow header so the next person starts there rather than at the symptom. 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.
Adds the Windows desktop-install job discussed in #172.
The gap
Every JS/TS lane runs on
ubuntu-latest. So the entire Windows dependency path — Electron's postinstall unpacking a dist through@electron/getand its native@electron-internal/extract-zipaddon — has never been exercised by CI. The one Windows job that exists,installer-tests.yml, runs two PowerShell tests againstinstall.ps1and installs nothing.What the job does
npm ciwith scriptsnode_modules/electron/dist/electron.exeexistsnpm ciexits 0 when a postinstall is skipped, so its exit code alone would let a silently missing unpack read as greennode scripts/assert-win-vcruntime.mjscheck:test:desktop:platformsdesktop-electron-pin.test.ts+ thescripts/*.test.mjsguards, on the platform that pin contract is actually aboutGated on the same
frontendlane asjs-tests, so Python-only PRs don't pay for a Windows runner. 40-minute timeout — Windows runners are slower andnpm ciis the long pole.What it does not catch — stated in the workflow header, not buried
It cannot reproduce the
ERR_DLOPEN_FAILEDfailure that motivated #172.windows-latestships Visual Studio Build Tools, soVCRUNTIME140.dllis always present. This job would stay green on a regression that breaks every clean developer machine. Reproducing that needs a runner image without the redistributable.I said this twice before agreeing to add the job, and I've put it in the workflow header so the next person doesn't have to rediscover it. The value here is the other Windows failure modes: platform-specific resolution, lockfile drift, a postinstall that breaks only on Windows.
One deliberate deviation from repo convention
I did not use
.github/actions/retry. It'sshell: bash, and no Windows job in this repo has ever used it — this job would be its first. That's an unproven path inside a job that is itself new and that I cannot execute. Replaced with an inlinepwshloop matching the action's defaults (3 attempts, 10s apart). An Electron dist download is precisely the flaky-network case retries exist for, so dropping retry entirely wasn't the right trade.Verification — structural only, and that's the honest limit
There is no Windows runner in my sandbox, so the job has never been executed. Its first real run is on this PR. What I did check:
frontendgate resolve as intended;check:test:desktop:platformsexists inapps/desktop/package.json;--project electronincludesscripts/**.test.{ts,mjs}, so the fix(desktop): name the missing Windows runtime instead of failing on dlopen #172 test runs there;If the first run is red, my ordered guesses are the
npm ciduration against the 40-minute timeout, then Windows-specific behaviour in a postinstall this repo has never run there. I'll drive it to green.Generated by Claude Code