From 9defa5183b38d7382f6d9ca4e053822981f421f2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:45:25 -0700 Subject: [PATCH] ci(windows): build node-pty from source before the win32 suites CI proved the encoder fix on real cmd.exe -- 26/26 -- and in the same run proved the job suite had been testing an unpatched binary. node-pty prefers its upstream prebuild, which does not contain this patch, so every job-object export was absent and isPtyJobOwnershipAvailable() was false. That guard is why the failure was loud rather than a vacuous pass, and it is the reason the assertion exists. Packaging was never affected: rebuild-native-deps.mjs already builds node-pty from source for Electron and restores the ConPTY runtime files. The gap was the node-runtime test environment only. Not changing requiresPatchedNodePtySourceBuild's win32 exemption here. Its premise -- that the patch is Unix-only -- is now false, but lifting it also needs pnpm rebuild to force a source build, and I cannot validate that on macOS and Linux from here. Recorded as a follow-up instead of changed blind. --- .github/workflows/pr.yml | 10 ++++++++ docs/reference/windows-process-enumeration.md | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 1adcb723c0e..810f732d6be 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -618,6 +618,16 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile + # Why: node-pty prefers its upstream prebuild, which does not contain + # Orca's Windows patch, so the job-object exports would be absent and the + # suite below would test an unpatched binary. build_from_source removes + # the prebuild, and the package's postinstall restores the ConPTY runtime + # files that a bare node-gyp rebuild would miss. + - name: Rebuild node-pty from patched source + env: + npm_config_build_from_source: 'true' + run: pnpm rebuild node-pty + - name: Test Windows-specific boundaries run: >- pnpm exec vitest run --config config/vitest.config.ts diff --git a/docs/reference/windows-process-enumeration.md b/docs/reference/windows-process-enumeration.md index d21538a1383..f34940143b5 100644 --- a/docs/reference/windows-process-enumeration.md +++ b/docs/reference/windows-process-enumeration.md @@ -157,3 +157,27 @@ and falls back. The case that would not be benign is a recycled `HANDLE` value, where the call could reach a different job in the same process. Fixing it properly means synchronising node-pty's handle table rather than adding a lock around one accessor, so it is deliberately left alone here. + +### The patch must actually be compiled + +node-pty prefers its upstream prebuild and only builds from source when +`npm_config_build_from_source` is set or no prebuild exists for the platform. +The Windows prebuild does **not** contain this patch, so a plain `pnpm install` +on Windows yields a node-pty without the job-object exports — and +`terminatePtyJob` then reports `unavailable` on every call, which is +indistinguishable from a correctly degraded build. + +Packaging is unaffected: `rebuild-native-deps.mjs` rebuilds node-pty from source +for Electron and restores the ConPTY runtime files that a bare `node-gyp +rebuild` skips. The gap is the **node-runtime test environment**, which is why +the Windows CI job rebuilds from source before running the win32 suites. + +`isPtyJobOwnershipAvailable()` exists for exactly this: the win32 suite asserts +it is true before asserting anything else, so an unpatched binary fails loudly +instead of passing every case vacuously. That guard is what caught this. + +`requiresPatchedNodePtySourceBuild()` in `ensure-native-runtime.mjs` still +exempts win32, on the premise that the patch is Unix-only. That premise is now +false, but lifting the exemption also needs `pnpm rebuild` to force a source +build — otherwise the assertion fires and the remedy does not fix it. Left as a +follow-up rather than changed blind.