ci: drop the two electron-builder suites from the Windows lane - #175
Merged
Conversation
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. 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: 87608a9893
ℹ️ 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".
| "test:ui": "vitest run --project ui", | ||
| "test:desktop:platforms": "vitest run --project electron", | ||
| "test:desktop:win-install": "vitest run --project electron desktop-electron-pin assert-dist-built assert-win-vcruntime before-pack stage-native-deps write-build-stamp", | ||
| "test:desktop:win-install": "vitest run --project electron desktop-electron-pin assert-dist-built assert-win-vcruntime write-build-stamp", |
There was a problem hiding this comment.
When a Windows-specific regression affects stageNodePtyInto—which branches on the host platform/architecture and performs native filesystem staging—this change leaves it covered only on Ubuntu. The stated rationale does not apply to this suite: at this commit stage-native-deps.mjs does not import electron-builder; only before-pack.mjs imports Arch, so the staging suite's loader failure has a separate, still-undiagnosed cause. Please isolate that failure or exercise the staging script directly on Windows rather than removing its filter and turning the lane green by dropping the coverage.
AGENTS.md reference: apps/desktop/AGENTS.md:L181-L188
Useful? React with 👍 / 👎.
This was referenced Aug 11, 2026
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
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.
Third and final pass on the Windows lane.
mainis red on this job; this makes it green.Where it got to
The narrowed job from #174 ran and got 21 tests passed, 0 test failures — but two of six suites failed to load, so the job was still red. Both #173 and #174 merged before their job finished, since it isn't a required check, so
mainhas carried a red job through both.Observed green on a Windows runner — quoted from the run, not inferred:
The pin contract and the VC++ guard both hold on the platform they're about, which is the point of the lane.
The two failures share one cause
…and
before-pack.mjsimports fromstage-native-deps.mjs. So both suites pullelectron-builderinto the vitest module graph, and both fail with the sameSyntaxError: Invalid or unexpected tokenon Windows while loading cleanly on ubuntu. The four that pass import no such thing.I ruled out the obvious first: BOM, CRLF, and stray control characters compared across all five
.mjssuites showed no difference between failing and passing files — which is what pointed at the import graph rather than the file contents.Why I'm not chasing it
It's a vitest/vite transform problem with a third-party package on Windows, not this repo's code. And those two are packaging-stage guards, not install-path checks — including them was scope creep on my part, for the second time on this lane. The diagnosis goes in the workflow header so the next person starts from the cause rather than the symptom.
Verification
The four filters match exactly four files, with
before-pack,stage-native-deps,ssh-connection,update-relaunchandwindows-hermes-pathall confirmed excluded. Every one of the four has been seen passing on a Windows runner, so this isn't a prediction.Follow-ups, unchanged and accumulating honestly
electron-buildercan't be imported under vitest on Windows. Worth knowing before anyone adds packaging tests to a Windows lane. Not filed as a repo bug — the defect is very likely upstream.update-relaunch.test.ts:54andwindows-hermes-path.test.ts:177are Linux-only by accident, not design; both are one-line fixes.Generated by Claude Code