diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 6b1deaa72ec..27e14c930e3 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -871,6 +871,11 @@ jobs: pnpm exec vitest run --config config/vitest.config.ts config/scripts/rebuild-native-deps.test.mjs config/scripts/rebuild-native-deps-windows-process-tree.test.mjs + config/scripts/rebuild-native-deps-node-pty.test.mjs + config/scripts/ensure-native-runtime-job-ownership.test.mjs + config/scripts/verify-packaged-node-pty-job-ownership.test.mjs + config/scripts/windows-pe-machine.test.mjs + config/scripts/script-module-dependencies.test.mjs src/main/windows-registry-addon.test.ts config/scripts/windows-process-tree-gyp-path.test.mjs config/scripts/windows-process-tree-gyp-rebuild.test.mjs diff --git a/.gitignore b/.gitignore index 97fc6affcac..424cf5a2a0c 100644 --- a/.gitignore +++ b/.gitignore @@ -122,6 +122,7 @@ docs/** !docs/reference/windows-cmd-shim-resolution.md !docs/reference/windows-daemon-host-relocation.md !docs/reference/windows-edr-posture.md +!docs/reference/windows-msys-job-breakaway.md !docs/reference/windows-process-enumeration.md !docs/reference/wsl-runner-verification.md !docs/reference/remote-wire-compatibility.md diff --git a/AGENTS.md b/AGENTS.md index 99c56e74108..29c50458e76 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,6 +76,7 @@ Orca targets macOS, Linux, and Windows. Keep all platform-dependent behavior beh - **Windows setup scripts**: the setup/issue-command runner is a `.cmd` batch file unless the script starts with a `#!` line — never derive that from the user's terminal-shell preference, and never launch a `.cmd` runner with a bare `cmd.exe /c` from a Git Bash pane (MSYS rewrites the `/c`). See [`docs/reference/windows-setup-shell.md`](./docs/reference/windows-setup-shell.md). - **Windows child processes**: start them through `runProcess`/`spawnProcess` in `src/shared/child-process/` — never `child_process` directly. It pins `windowsHide`, refuses `shell: true`, and encodes `.cmd`/`.bat` arguments so neither `CommandLineToArgvW` nor `cmd.exe` mangles them. A ratchet test fails on any new direct import. Recognised npm/pnpm `.cmd` shims are resolved to their real target so the spawn skips `cmd.exe` entirely; see [`docs/reference/windows-cmd-shim-resolution.md`](./docs/reference/windows-cmd-shim-resolution.md) before adding a shim shape or debugging one. - **Windows process enumeration**: read the table through `src/main/windows/windows-process-table.ts`, never by forking `powershell.exe`. See [`docs/reference/windows-process-enumeration.md`](./docs/reference/windows-process-enumeration.md). +- **Windows MSYS/Git Bash panes**: their children break away from the per-PTY job unless it is created without `JOB_OBJECT_LIMIT_BREAKAWAY_OK`, and a `conpty.node` built before that fix passes every existing gate. Before changing the per-PTY job or debugging `windows-msys-job.win32.test.ts`, read [`docs/reference/windows-msys-job-breakaway.md`](./docs/reference/windows-msys-job-breakaway.md). - **Windows daemon-host relocation**: the terminal daemon runs from a copy of the app runtime under `%LOCALAPPDATA%`, which is what survives an auto-update. Before touching that copy, its exe name, or the NSIS uninstall macro, read [`docs/reference/windows-daemon-host-relocation.md`](./docs/reference/windows-daemon-host-relocation.md). - **Windows EDR signal**: don't add `-ExecutionPolicy Bypass`, `-EncodedCommand`, `cmd.exe /c` with escaped free text, per-operation interpreter spawning, or runtime `Add-Type` compilation without reading [`docs/reference/windows-edr-posture.md`](./docs/reference/windows-edr-posture.md) first — behavioural EDR scores each of those, and being signed does not clear them. - **WSL commands**: build argv with `buildWslExecArgs` (always `--exec` — under `--`, `wsl.exe` expands `$name` in every argument and silently rewrites the script), and fence anything whose stdout you parse with `buildWslCapturedLoginShellCommand`, because the interactive login shell prints the distro banner to stdout. See [`docs/reference/wsl-command-execution.md`](./docs/reference/wsl-command-execution.md). diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index 8a5f8c27475..c26d1b4858a 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -16,7 +16,7 @@ const { verifyLinuxGlibcFloor } = require('./scripts/verify-linux-glibc-floor.cj const { writeMacBuildCompatibility } = require('./scripts/mac-build-compatibility.cjs') const { verifyPackagedPluginResources } = require('./scripts/verify-packaged-plugin-resources.cjs') const { - verifyPackagedNodePtyJobOwnership + verifyPackagedWindowsNodePty } = require('./scripts/verify-packaged-node-pty-job-ownership.cjs') const { verifySkillsCliRuntime } = require('./scripts/verify-skills-cli-runtime.cjs') const { verifyStaticAppImagePackage } = require('./scripts/static-appimage-package-contract.cjs') @@ -353,11 +353,7 @@ module.exports = { const hostArchEnum = archEnumByNodeArch[process.arch] const canExecuteTargetArch = context.arch === hostArchEnum || context.arch === 4 if (context.electronPlatformName === 'win32') { - if (process.platform === 'win32' && canExecuteTargetArch) { - verifyPackagedNodePtyJobOwnership(resourcesDir) - } else { - console.log('[verify-packaged-node-pty] skipped cross-platform or cross-arch package') - } + verifyPackagedWindowsNodePty(resourcesDir, context.arch, { canExecuteTargetArch }) } verifySkillsCliRuntime(join(resourcesDir, 'app.asar.unpacked', 'out'), resourcesDir, { executeCommands: canExecuteTargetArch diff --git a/config/packaged-runtime-node-modules.cjs b/config/packaged-runtime-node-modules.cjs index 30784e79f5c..3d4a968ea23 100644 --- a/config/packaged-runtime-node-modules.cjs +++ b/config/packaged-runtime-node-modules.cjs @@ -607,6 +607,7 @@ module.exports = { createPackagedRuntimeNodeModuleResources, findAsarEntry, isPackagedExternalSpecifier, + normalizeNodePtyWindowsArch, packageNameFromSpecifier, prunePackagedNodePty, prunePackagedParcelWatcher, diff --git a/config/scripts/build-windows-process-tree-relay-addon.mjs b/config/scripts/build-windows-process-tree-relay-addon.mjs index 912bbd3c174..526400be8f2 100644 --- a/config/scripts/build-windows-process-tree-relay-addon.mjs +++ b/config/scripts/build-windows-process-tree-relay-addon.mjs @@ -19,16 +19,8 @@ * node config/scripts/build-windows-process-tree-relay-addon.mjs --arch=arm64 */ import { execFileSync } from 'node:child_process' -import { - closeSync, - copyFileSync, - existsSync, - mkdirSync, - openSync, - readFileSync, - readSync, - writeFileSync -} from 'node:fs' +import { copyFileSync, existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import { createRequire } from 'node:module' import { join, resolve } from 'node:path' import { RELAY_WINDOWS_PROCESS_TREE_FILENAME } from '../../src/shared/relay-artifacts.ts' import { @@ -39,12 +31,13 @@ import { WINDOWS_PROCESS_TREE_PACKAGE_DIR as PACKAGE_DIR } from './windows-process-tree-gyp-rebuild.mjs' +const { PE_MACHINE, describePeMachine, readPeMachine } = createRequire(import.meta.url)( + './windows-pe-machine.cjs' +) + const ROOT = resolve(import.meta.dirname, '..', '..') const SUPPORTED_ARCHES = ['x64', 'arm64'] -/** PE `IMAGE_FILE_HEADER.Machine` values, so a cross-build cannot silently emit host arch. */ -const PE_MACHINE = { x64: 0x8664, arm64: 0xaa64 } - function parseArgs(argv) { const arch = argv.find((a) => a.startsWith('--arch='))?.slice('--arch='.length) ?? process.arch const outDir = argv.find((a) => a.startsWith('--out='))?.slice('--out='.length) @@ -363,21 +356,6 @@ function applyWindowsProcessTreeBuildFixes() { } } -/** Read the PE machine field, so an arm64 request cannot ship an x64 binary. */ -function readPeMachine(binaryPath) { - const fd = openSync(binaryPath, 'r') - try { - const header = Buffer.alloc(4) - readSync(fd, header, 0, 4, 0x3c) - const peOffset = header.readUInt32LE(0) - const machine = Buffer.alloc(2) - readSync(fd, machine, 0, 2, peOffset + 4) - return machine.readUInt16LE(0) - } finally { - closeSync(fd) - } -} - function main() { const { arch, outDir } = parseArgs(process.argv.slice(2)) if (process.platform !== 'win32') { @@ -410,9 +388,12 @@ function main() { } const machine = readPeMachine(built) if (machine !== PE_MACHINE[arch]) { + const cause = + machine === null + ? 'A truncated or quarantined build output looks like this; a relay would get a binary no host can load.' + : 'node-gyp ignored --arch; a relay would get a binary its host cannot load.' throw new Error( - `Built binary is machine 0x${machine.toString(16)}, expected 0x${PE_MACHINE[arch].toString(16)} for ${arch}. ` + - 'node-gyp ignored --arch; a relay would get a binary its host cannot load.' + `Built binary is ${describePeMachine(machine)}, expected 0x${PE_MACHINE[arch].toString(16)} for ${arch}. ${cause}` ) } diff --git a/config/scripts/ensure-native-runtime-job-ownership.test.mjs b/config/scripts/ensure-native-runtime-job-ownership.test.mjs index 91b4c5d328d..960c87e362b 100644 --- a/config/scripts/ensure-native-runtime-job-ownership.test.mjs +++ b/config/scripts/ensure-native-runtime-job-ownership.test.mjs @@ -1,22 +1,48 @@ -import { readFileSync } from 'node:fs' +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' import { createRequire } from 'node:module' -import { describe, expect, it } from 'vitest' +import { tmpdir } from 'node:os' +import { dirname, join, resolve } from 'node:path' +import { describe, expect, it, vi } from 'vitest' +import { peImage } from './windows-pe-image-fixture.mjs' const require = createRequire(import.meta.url) -const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs') +const { + CYGWIN_BREAKAWAY_MARKER, + CYGWIN_BREAKAWAY_MARKER_TEXT, + assertNodePtyJobOwnership, + assertRebuiltConptyDeniesMsysBreakaway, + nodePtyAddonPath +} = require('./node-pty-job-ownership.cjs') const NODE_PTY_PATCH = readFileSync( new URL('../patches/node-pty@1.1.0.patch', import.meta.url), 'utf8' ) -const PATCHED = { - dir: 'build/Release/', - module: { - listJobProcessIds: () => [], - terminateJob: () => true, - assignCurrentProcessToJob: () => true - } +const JOB_EXPORTS = { + listJobProcessIds: () => [], + terminateJob: () => true, + assignCurrentProcessToJob: () => true } + +const fixtureDir = mkdtempSync(join(tmpdir(), 'node-pty-job-ownership-')) + +/** A stand-in addon; only the wide literal the gate reads has to be real. */ +function writeAddon(name, { cygwinBreakawayDenied }) { + const path = join(fixtureDir, name) + writeFileSync( + path, + Buffer.concat([ + Buffer.from('MZ fake addon '), + cygwinBreakawayDenied ? CYGWIN_BREAKAWAY_MARKER : Buffer.alloc(0) + ]) + ) + return path +} + +const CURRENT_ADDON = writeAddon('current.node', { cygwinBreakawayDenied: true }) +const PRE_MSYS_ADDON = writeAddon('pre-msys.node', { cygwinBreakawayDenied: false }) + +const PATCHED = { dir: 'build/Release/', module: JOB_EXPORTS } const PREBUILD = { dir: 'prebuilds/win32-x64/', module: { @@ -28,6 +54,13 @@ const PREBUILD = { } } +const onWindows = (native, addonPath) => ({ + platform: 'win32', + nativeName: 'conpty', + native, + addonPath +}) + describe('assertNodePtyJobOwnership', () => { it('keeps node-addon-api project paths absolute during Windows source builds', () => { expect(NODE_PTY_PATCH).toContain( @@ -40,28 +73,227 @@ describe('assertNodePtyJobOwnership', () => { expect(NODE_PTY_PATCH).toContain("- 'target_name': 'pty'") }) + // The gate's whole case rests on this literal, and nothing else ties the + // constant to the C++ that compiles it in. Drift either way has to fail HERE: + // otherwise it fails every correctly rebuilt addon, and no rebuild can fix it. + it('sniffs for a literal the patch really adds to conpty.cc', () => { + const conptyHunk = NODE_PTY_PATCH.split(/^diff --git /m).find((section) => + section.startsWith('a/src/win/conpty.cc ') + ) + expect(conptyHunk, 'the patch no longer touches src/win/conpty.cc').toBeDefined() + const addedCode = conptyHunk + .split('\n') + .filter((line) => line.startsWith('+') && !/^\+\s*(\/\/|\*)/.test(line)) + expect( + addedCode.some((line) => line.includes(`L"${CYGWIN_BREAKAWAY_MARKER_TEXT}"`)), + `No added conpty.cc line carries L"${CYGWIN_BREAKAWAY_MARKER_TEXT}". Either the patch ` + + 'stopped adding it or CYGWIN_BREAKAWAY_MARKER_TEXT drifted; until they agree the gate ' + + 'rejects every correctly rebuilt addon.' + ).toBe(true) + }) + + // MSVC compiles L"" to UTF-16LE; reading the addon as anything else finds nothing. + it('looks for that literal in the encoding the compiler stores it in', () => { + expect(CYGWIN_BREAKAWAY_MARKER.toString('utf16le')).toBe(CYGWIN_BREAKAWAY_MARKER_TEXT) + expect(CYGWIN_BREAKAWAY_MARKER.length).toBe(CYGWIN_BREAKAWAY_MARKER_TEXT.length * 2) + }) + it('rejects the prebuild that shipped without the job exports', () => { - expect(() => - assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PREBUILD }) - ).toThrow(/listJobProcessIds, terminateJob, assignCurrentProcessToJob/) + expect(() => assertNodePtyJobOwnership(onWindows(PREBUILD, CURRENT_ADDON))).toThrow( + /listJobProcessIds, terminateJob, assignCurrentProcessToJob/ + ) }) it('names where the bad native came from, so the fix is obvious', () => { - expect(() => - assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PREBUILD }) - ).toThrow(/prebuilds\/win32-x64/) + expect(() => assertNodePtyJobOwnership(onWindows(PREBUILD, CURRENT_ADDON))).toThrow( + /prebuilds\/win32-x64/ + ) }) it('accepts a source build carrying the patch', () => { - expect(() => - assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PATCHED }) - ).not.toThrow() + expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, CURRENT_ADDON))).not.toThrow() + }) + + // The reason this gate reads the binary at all: every export above predates + // the Cygwin/MSYS breakaway denial, so a build that leaks every Git Bash + // child out of its pane's job satisfies all of them. + it('rejects a source build that predates the Cygwin/MSYS breakaway denial', () => { + expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, PRE_MSYS_ADDON))).toThrow( + /predates the Cygwin\/MSYS job-breakaway denial/ + ) + }) + + it('tells that build apart by path, and says to rebuild', () => { + expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, PRE_MSYS_ADDON))).toThrow( + /pre-msys\.node[\s\S]*Rebuild node-pty from source/ + ) }) it.each([ - ['non-Windows hosts', { platform: 'darwin', nativeName: 'pty' }], + ['no path at all', undefined], + ['a path that is not there', join(fixtureDir, 'absent.node')] + ])('refuses rather than skip when the addon cannot be read: %s', (_case, addonPath) => { + expect(() => assertNodePtyJobOwnership(onWindows(PATCHED, addonPath))).toThrow( + /Cannot read node-pty's conpty native/ + ) + }) + + // Passing the conpty name and no readable addon: on win32 every remaining + // branch throws, so only the platform gate can keep these quiet. The MSYS + // breakaway denial is a Windows concern and must cost other hosts nothing. + it.each([ + ['non-Windows hosts', { platform: 'darwin', nativeName: 'conpty' }], + ['non-Windows hosts building for one', { platform: 'linux', nativeName: 'conpty' }], ['the pre-ConPTY winpty backend', { platform: 'win32', nativeName: 'pty' }] ])('stays out of the way on %s', (_case, spec) => { expect(() => assertNodePtyJobOwnership({ ...spec, native: PREBUILD })).not.toThrow() }) + + it('would have thrown on Windows for the very same input', () => { + expect(() => + assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PREBUILD }) + ).toThrow() + }) +}) + +describe('nodePtyAddonPath', () => { + // Built from segments rather than a POSIX string: on Windows `resolve` returns + // a drive letter and backslashes, so a literal only ever passed off Windows. + it('resolves the addon against node-pty lib, which is the only base callers share', () => { + expect( + nodePtyAddonPath( + resolve('/app/node_modules/node-pty/lib/utils.js'), + { dir: '../build/Release/' }, + 'conpty' + ) + ).toBe(join(resolve('/app/node_modules/node-pty'), 'build', 'Release', 'conpty.node')) + }) + + it('handles the bundled layout, where the addon sits beside lib', () => { + expect( + nodePtyAddonPath( + resolve('/app/resources/node-pty/lib/utils.js'), + { dir: './build/Release/' }, + 'conpty' + ) + ).toBe(join(resolve('/app/resources/node-pty/lib'), 'build', 'Release', 'conpty.node')) + }) +}) + +describe('assertRebuiltConptyDeniesMsysBreakaway', () => { + const rebuiltInto = (files) => { + const nodePtyDir = join(mkdtempSync(join(fixtureDir, 'rebuild-')), 'node-pty') + for (const [relativePath, options] of Object.entries(files)) { + const { arch = 'x64', cygwinBreakawayDenied = true } = options + const addonPath = join(nodePtyDir, ...relativePath.split('/')) + mkdirSync(dirname(addonPath), { recursive: true }) + writeFileSync( + addonPath, + Buffer.concat([ + peImage({ arch }), + cygwinBreakawayDenied ? CYGWIN_BREAKAWAY_MARKER : Buffer.alloc(0) + ]) + ) + } + return nodePtyDir + } + + it('accepts the addon a good same-host rebuild leaves behind', () => { + const nodePtyDir = rebuiltInto({ 'build/Release/conpty.node': {} }) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'x64', crossHost: false }) + ).not.toThrow() + }) + + it('rejects one that predates the denial, wherever the rebuild ran', () => { + const nodePtyDir = rebuiltInto({ + 'build/Release/conpty.node': { cygwinBreakawayDenied: false } + }) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'x64', crossHost: true }) + ).toThrow(/predates the Cygwin\/MSYS job-breakaway denial/) + }) + + // On the host that will run this install, no addon means loadNativeModule + // falls through to the published prebuild -- the binary that leaks every MSYS + // pane child. That is a broken build, not an absence to shrug at. + it('refuses a same-host rebuild that reported success and produced nothing', () => { + const nodePtyDir = rebuiltInto({ 'package.json': {} }) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'x64', crossHost: false }) + ).toThrow(/the rebuild reported success/) + }) + + it('names both the addon it wanted and the prebuild that would load instead', () => { + const nodePtyDir = rebuiltInto({ 'package.json': {} }) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'arm64', crossHost: false }) + ).toThrow(/build[\\/]Release[\s\S]*prebuilds[\\/]win32-arm64/) + }) + + // A rebuild that ignored --arch leaves a binary the target cannot load, so + // node-pty falls back to the prebuild. Saying so here is two steps closer to + // the command that fixes it than saying so at packaging time. + it('rejects an addon of an architecture this rebuild did not target', () => { + const nodePtyDir = rebuiltInto({ 'build/Release/conpty.node': { arch: 'x64' } }) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'arm64', crossHost: true }) + ).toThrow(/machine 0x8664, but this rebuild targets win32-arm64/) + }) + + // "node-gyp ignored --arch" is a guess when the file is not a PE at all: that + // is a truncated or quarantined artifact, and saying otherwise sends the + // reader to the wrong command. + it('does not blame --arch for a file that is not a PE image', () => { + const nodePtyDir = join(mkdtempSync(join(fixtureDir, 'rebuild-')), 'node-pty') + mkdirSync(join(nodePtyDir, 'build', 'Release'), { recursive: true }) + writeFileSync(join(nodePtyDir, 'build', 'Release', 'conpty.node'), Buffer.alloc(0x200)) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'x64', crossHost: false }) + ).toThrow(/is not a PE image[\s\S]*truncated or quarantined/) + }) + + it('still names the consequence for that file, which is the prebuild', () => { + const nodePtyDir = join(mkdtempSync(join(fixtureDir, 'rebuild-')), 'node-pty') + mkdirSync(join(nodePtyDir, 'build', 'Release'), { recursive: true }) + writeFileSync(join(nodePtyDir, 'build', 'Release', 'conpty.node'), Buffer.alloc(0x200)) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'x64', crossHost: false }) + ).toThrow(/fall back to the published prebuild/) + }) + + it('accepts one a cross-arch rebuild really did emit for the target', () => { + const nodePtyDir = rebuiltInto({ 'build/Release/conpty.node': { arch: 'arm64' } }) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'arm64', crossHost: true }) + ).not.toThrow() + }) + + // PE_MACHINE covers what Orca ships; an arch it does not know is not one this + // can judge, and guessing would fail a rebuild that was fine. + it('does not judge an architecture it has no machine value for', () => { + const nodePtyDir = rebuiltInto({ 'build/Release/conpty.node': { arch: 'x64' } }) + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ nodePtyDir, rebuildArch: 'ia32', crossHost: true }) + ).not.toThrow() + }) + + it.each([ + ['a cross-host rebuild need not leave a win32 addon here', { crossHost: true }, true], + ['no node-pty on this disk is not a bad build', { crossHost: false }, false] + ])('warns instead: %s', (_case, verdict, nodePtyInstalled) => { + const nodePtyDir = nodePtyInstalled + ? rebuiltInto({ 'package.json': {} }) + : join(fixtureDir, 'no-node-pty-here') + const warn = vi.fn() + expect(() => + assertRebuiltConptyDeniesMsysBreakaway({ + nodePtyDir, + rebuildArch: 'x64', + ...verdict, + warn + }) + ).not.toThrow() + expect(warn).toHaveBeenCalledWith(expect.stringContaining('could not check the MSYS')) + }) }) diff --git a/config/scripts/ensure-native-runtime.mjs b/config/scripts/ensure-native-runtime.mjs index 6278a1e0b41..c93b6a19d62 100644 --- a/config/scripts/ensure-native-runtime.mjs +++ b/config/scripts/ensure-native-runtime.mjs @@ -13,7 +13,7 @@ import { } from './windows-process-tree-gyp-rebuild.mjs' const require = createRequire(import.meta.url) -const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs') +const { assertNodePtyJobOwnership, nodePtyAddonPath } = require('./node-pty-job-ownership.cjs') const { assertWindowsProcessTreeCreationTime } = require('./windows-process-tree-creation-time.cjs') const scriptPath = import.meta.filename const projectDir = resolve(import.meta.dirname, '../..') @@ -298,7 +298,11 @@ function loadNodePtyNativeModule() { // terminal is created, so require('node-pty') alone can miss ABI mismatches. const native = loadNativeModule(nativeName) assertNodePtyWindowsConptyRuntime(native?.dir) - assertNodePtyJobOwnership({ nativeName, native }) + assertNodePtyJobOwnership({ + nativeName, + native, + addonPath: nodePtyAddonPath(require.resolve('node-pty/lib/utils'), native, nativeName) + }) if (requiresPatchedNodePtySourceBuild() && !isNodePtyReleaseBuildDir(native?.dir)) { throw new Error( `node-pty resolved to ${native.dir}; expected build/Release so Orca's node-pty patch is active` diff --git a/config/scripts/node-pty-job-ownership.cjs b/config/scripts/node-pty-job-ownership.cjs index 5ad578fd74a..8e085e69221 100644 --- a/config/scripts/node-pty-job-ownership.cjs +++ b/config/scripts/node-pty-job-ownership.cjs @@ -1,25 +1,188 @@ 'use strict' +const { existsSync, readFileSync } = require('node:fs') +const { dirname, join, resolve } = require('node:path') +const { PE_MACHINE, describePeMachine, readPeMachine } = require('./windows-pe-machine.cjs') + const NODE_PTY_JOB_EXPORTS = ['listJobProcessIds', 'terminateJob', 'assignCurrentProcessToJob'] -function assertNodePtyJobOwnership({ nativeName, native, platform = process.platform }) { +/** + * The wide literal `usesCygwinRuntime` probes for in conpty.cc, as it sits in + * the compiled addon. + * + * Why sniff the binary rather than trust the exports: all three job exports + * predate the Cygwin/MSYS breakaway denial, so symbol presence cannot tell a + * current build from one whose per-PTY job still carries + * JOB_OBJECT_LIMIT_BREAKAWAY_OK. Measured on Windows 11: such a build passes + * every export check, reports isPtyJobOwnershipAvailable() true, and passes + * windows-pty-job.win32.test.ts 6/6, while every child of a Git Bash pane is + * created outside the pane's job and survives terminatePtyJob. See + * docs/reference/windows-msys-job-breakaway.md. + * + * Same shape as stagedRelayAddonIsUnpatched() in + * src/main/windows/windows-process-table.ts, which already tells a patched + * addon from a published one by a binary import name. + */ +const CYGWIN_BREAKAWAY_MARKER_TEXT = 'msys-2.0.dll' +const CYGWIN_BREAKAWAY_MARKER = Buffer.from(CYGWIN_BREAKAWAY_MARKER_TEXT, 'utf16le') + +/** True when the addon carries the denial. Read errors propagate: callers that cannot read it must not pass. */ +function conptyDeniesCygwinBreakaway(addonPath) { + return readFileSync(addonPath).includes(CYGWIN_BREAKAWAY_MARKER) +} + +/** + * Why here and not only at packaging: a rebuild that did not honour `--arch` + * leaves a binary the target cannot load, the app falls back to the published + * prebuild, and the packaged gate then reports it two steps from the command + * that could fix it. `PE_MACHINE` covers the Windows arches Orca ships; anything + * else this cannot judge, so it does not pretend to. + */ +function assertRebuiltConptyMatchesArch(addonPath, rebuildArch) { + const expected = PE_MACHINE[rebuildArch] + if (expected === undefined) { + return + } + const machine = readPeMachine(addonPath) + if (machine === expected) { + return + } + const consequence = [ + ', so node-pty would fall back to the published prebuild, which predates the', + 'Cygwin/MSYS job-breakaway denial and leaks every MSYS pane child out of its job.' + ].join(' ') + throw new Error( + machine === null + ? `${addonPath} is not a PE image${consequence} Check the ` + + `node-pty build output above; a truncated or quarantined artifact looks like this.` + : `${addonPath} is ${describePeMachine(machine)}, but this rebuild targets ` + + `win32-${rebuildArch} (0x${expected.toString(16)}): node-gyp did not honour ` + + `--arch${consequence}` + ) +} + +/** + * The verdict on the addon a Windows rebuild just claimed to produce. + * + * Takes the host as arguments rather than reading `process`, because the branch + * that matters -- a rebuild for the very host running it -- is otherwise + * reachable only from Windows, and a gate nobody can run is a gate nobody + * checks. + * + * Absent is fatal on that host: `loadNativeModule` falls through to + * prebuilds/win32-, and the published prebuild predates the denial, so + * the app would load it with nothing said. A cross-host rebuild need not leave + * a win32 addon on this disk, and node-pty may not be installed at all -- + * neither is evidence of a bad build. + */ +function assertRebuiltConptyDeniesMsysBreakaway({ + nodePtyDir, + rebuildArch, + crossHost, + warn = console.warn +}) { + const addonPath = join(nodePtyDir, 'build', 'Release', 'conpty.node') + if (existsSync(addonPath)) { + assertRebuiltConptyMatchesArch(addonPath, rebuildArch) + assertCygwinBreakawayDenied(addonPath, { dir: addonPath }) + return + } + if (crossHost || !existsSync(nodePtyDir)) { + warn(`[rebuild] no addon at ${addonPath}; could not check the MSYS job-breakaway denial.`) + return + } + const prebuildPath = join(nodePtyDir, 'prebuilds', `win32-${rebuildArch}`, 'conpty.node') + throw new Error( + `the rebuild reported success but ${addonPath} is not there, so node-pty would fall through ` + + `to ${prebuildPath}. That published prebuild predates the Cygwin/MSYS ` + + 'job-breakaway denial: every Git Bash pane child would be created outside its job and ' + + 'survive terminatePtyJob. Check the node-pty build output above; a same-host source ' + + 'build must leave conpty.node in build/Release.' + ) +} + +/** + * Absolute path of the addon `loadNativeModule` just resolved. + * + * `native.dir` is relative to node-pty's own `lib/`, which is the only base + * every caller shares -- the project install, a staged rebuild and the packaged + * resources tree all reach the addon through a different root. + */ +function nodePtyAddonPath(nodePtyUtilsPath, native, nativeName) { + return resolve(dirname(nodePtyUtilsPath), native.dir, `${nativeName}.node`) +} + +function assertNodePtyJobOwnership({ nativeName, native, addonPath, platform = process.platform }) { if (platform !== 'win32' || nativeName !== 'conpty') { return } const exported = native?.module ?? native const missing = NODE_PTY_JOB_EXPORTS.filter((name) => typeof exported?.[name] !== 'function') - if (missing.length === 0) { + if (missing.length > 0) { + throw new Error( + [ + `node-pty's conpty native is missing ${missing.join(', ')}.`, + `Resolved from: ${native?.dir ?? 'unknown'}`, + 'That build cannot own a PTY tree, so terminatePtyJob degrades to "unavailable"', + 'and pane teardown falls back to guessing by PID ancestry.', + 'Rebuild node-pty from source so config/patches/node-pty@1.1.0.patch applies.' + ].join(' ') + ) + } + assertCygwinBreakawayDenied(addonPath, native) +} + +/** + * Why this refuses instead of skipping when the addon cannot be read: an + * unreadable binary is exactly the state that used to pass. `loadNativeModule` + * has already required this file, so "cannot read it" means the caller did not + * say which file it loaded, and a gate that cannot see its subject is not a + * gate. + */ +function assertCygwinBreakawayDenied(addonPath, native) { + let binary + try { + binary = readFileSync(addonPath) + } catch (error) { + throw new Error( + [ + `Cannot read node-pty's conpty native at ${addonPath ?? ''}`, + `(resolved from ${native?.dir ?? 'unknown'}): ${error.message}.`, + 'Without the binary this cannot tell a current build from one that leaks', + 'every MSYS pane child out of its job, so it refuses rather than assume.' + ].join(' ') + ) + } + if (binary.includes(CYGWIN_BREAKAWAY_MARKER)) { return } - throw new Error( + throw staleConptySourceBuildError(addonPath) +} + +/** The verdict on a source build that is simply out of date: rebuild it here. */ +function staleConptySourceBuildError(addonPath) { + return new Error( [ - `node-pty's conpty native is missing ${missing.join(', ')}.`, - `Resolved from: ${native?.dir ?? 'unknown'}`, - 'That build cannot own a PTY tree, so terminatePtyJob degrades to "unavailable"', - 'and pane teardown falls back to guessing by PID ancestry.', - 'Rebuild node-pty from source so config/patches/node-pty@1.1.0.patch applies.' + `node-pty's conpty native at ${addonPath} predates the Cygwin/MSYS job-breakaway denial.`, + 'It exports the job functions, so it looks patched, but its per-PTY job still carries', + 'JOB_OBJECT_LIMIT_BREAKAWAY_OK and every Git Bash child is created outside the job:', + 'terminatePtyJob reports "terminated" and leaves the tree running.', + 'Rebuild node-pty from source so the current config/patches/node-pty@1.1.0.patch applies', + '(a worktree sharing node_modules with its main checkout shares that stale addon).', + `If that patch no longer adds L"${CYGWIN_BREAKAWAY_MARKER_TEXT}" to conpty.cc then this marker is`, + 'stale, not the addon, and no rebuild can satisfy it.', + 'See docs/reference/windows-msys-job-breakaway.md.' ].join(' ') ) } -module.exports = { assertNodePtyJobOwnership } +module.exports = { + CYGWIN_BREAKAWAY_MARKER, + CYGWIN_BREAKAWAY_MARKER_TEXT, + assertNodePtyJobOwnership, + assertCygwinBreakawayDenied, + assertRebuiltConptyDeniesMsysBreakaway, + conptyDeniesCygwinBreakaway, + nodePtyAddonPath, + staleConptySourceBuildError +} diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index 428379bd28e..0d31e58f9bc 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -144,6 +144,9 @@ const NATIVE_RUNTIME_PREFIXES = [ 'config/scripts/ensure-native-runtime', 'config/scripts/rebuild-native-deps', 'config/scripts/node-pty-job-ownership', + 'config/scripts/windows-pe-machine', + 'config/scripts/windows-pe-image-fixture', + 'config/scripts/script-module-dependencies', 'config/scripts/windows-process-tree-creation-time', 'config/scripts/windows-process-tree-gyp-rebuild', 'config/scripts/electron-builder-native-rebuild', @@ -220,6 +223,11 @@ const WINDOWS_PACKAGE_TESTS = [ ...LINUX_PACKAGE_TESTS, 'config/scripts/rebuild-native-deps.test.mjs', 'config/scripts/rebuild-native-deps-windows-process-tree.test.mjs', + 'config/scripts/rebuild-native-deps-node-pty.test.mjs', + 'config/scripts/ensure-native-runtime-job-ownership.test.mjs', + 'config/scripts/verify-packaged-node-pty-job-ownership.test.mjs', + 'config/scripts/windows-pe-machine.test.mjs', + 'config/scripts/script-module-dependencies.test.mjs', 'src/main/windows-registry-addon.test.ts', 'src/main/providers/windows-conpty-wide-char-duplication.node-pty.test.ts', 'src/main/providers/pty-repaint-wide-char-buffer.node-pty.test.ts', diff --git a/config/scripts/rebuild-native-deps-node-pty.test.mjs b/config/scripts/rebuild-native-deps-node-pty.test.mjs index 27fb7e3651e..a1321878742 100644 --- a/config/scripts/rebuild-native-deps-node-pty.test.mjs +++ b/config/scripts/rebuild-native-deps-node-pty.test.mjs @@ -260,6 +260,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => { writeFakeLoadableNodePty(projectDir, { ownsPtyJob: false }) writeFakeWindowsRegistry(projectDir) writeFakeWindowsProcessTree(projectDir) + writeFakeNodePtyConptyPayload(projectDir, process.arch) const result = runRebuildScript(projectDir, { ORCA_REBUILD_TEST_LOG: rebuildLogPath, @@ -399,4 +400,106 @@ describe('rebuild-native-deps patched node-pty rebuild', () => { } }) } + + // The Electron probe carries this check too, but it is skipped whenever the + // Electron package binary is unusable. Every job export predates the MSYS + // breakaway denial, so without reading the binary this step would hand the + // packaged app one that leaks every Git Bash child out of its pane's job. + it('fails a Windows rebuild that leaves an addon predating the MSYS breakaway denial', () => { + const projectDir = mkTempProject() + + try { + writeFakeUsableElectronPackage(projectDir, { platform: 'win32' }) + writeFakeElectronRebuild(projectDir) + writeFakeNodePtyConptyPayload(projectDir, 'x64', { cygwinBreakawayDenied: false }) + writeFakeWindowsProcessTreeWithNodeAddonApi(projectDir) + + const result = runRebuildScript( + projectDir, + { npm_config_platform: 'win32', npm_config_arch: 'x64' }, + ['--platform=win32', '--arch=x64', '--force'] + ) + + expect(result.status).not.toBe(0) + expect(result.stderr).toContain('predates the Cygwin/MSYS job-breakaway denial') + } finally { + removeTreeSync(projectDir) + } + }) + + it('accepts a Windows rebuild whose addon carries the denial', () => { + const projectDir = mkTempProject() + + try { + writeFakeUsableElectronPackage(projectDir, { platform: 'win32' }) + writeFakeElectronRebuild(projectDir) + writeFakeNodePtyConptyPayload(projectDir, 'x64') + writeFakeWindowsProcessTreeWithNodeAddonApi(projectDir) + + const result = runRebuildScript( + projectDir, + { npm_config_platform: 'win32', npm_config_arch: 'x64' }, + ['--platform=win32', '--arch=x64', '--force'] + ) + + expect(result.status, result.stderr).toBe(0) + expect(result.stderr).not.toContain('job-breakaway denial') + } finally { + removeTreeSync(projectDir) + } + }) + + // A cross-host rebuild does not necessarily leave a win32 addon on this disk, + // and neither does a tree with no node-pty in it. That must warn, not fail an + // install that was working. + it('warns rather than fails when no addon is expected on this disk', () => { + const projectDir = mkTempProject() + + try { + writeFakeUsableElectronPackage(projectDir, { platform: 'win32' }) + writeFakeElectronRebuild(projectDir) + writeFakeWindowsProcessTreeWithNodeAddonApi(projectDir) + + const result = runRebuildScript( + projectDir, + { npm_config_platform: 'win32', npm_config_arch: 'x64' }, + ['--platform=win32', '--arch=x64', '--force'] + ) + + expect(result.status, result.stderr).toBe(0) + expect(result.stderr + result.stdout).toContain('could not check the MSYS job-breakaway') + } finally { + removeTreeSync(projectDir) + } + }) + + // The other half: on the host that will run this install, a missing addon is + // not an absence to shrug at. loadNativeModule falls through to the published + // prebuild, which is the binary that leaks every MSYS pane child. + // Runs only on Windows -- nothing else can make a win32 rebuild same-host. + it.skipIf(process.platform !== 'win32')( + 'fails a same-host Windows rebuild that left no addon, naming the prebuild that would load', + () => { + const projectDir = mkTempProject() + + try { + writeFakeUsableElectronPackage(projectDir, { platform: 'win32' }) + writeFakeElectronRebuild(projectDir) + writeFakeWindowsProcessTreeWithNodeAddonApi(projectDir) + writeFakeLoadableNodePty(projectDir, { nativeDir: `prebuilds/win32-${process.arch}` }) + + const result = runRebuildScript( + projectDir, + { npm_config_platform: 'win32', npm_config_arch: process.arch }, + ['--platform=win32', `--arch=${process.arch}`, '--force'] + ) + + expect(result.status).not.toBe(0) + expect(result.stderr).toContain(join('build', 'Release', 'conpty.node')) + expect(result.stderr).toContain(join('prebuilds', `win32-${process.arch}`, 'conpty.node')) + } finally { + removeTreeSync(projectDir) + } + } + ) }) diff --git a/config/scripts/rebuild-native-deps-test-fixtures.mjs b/config/scripts/rebuild-native-deps-test-fixtures.mjs index ad9bb4c1ce2..dec35d4c143 100644 --- a/config/scripts/rebuild-native-deps-test-fixtures.mjs +++ b/config/scripts/rebuild-native-deps-test-fixtures.mjs @@ -7,10 +7,25 @@ import { readFileSync, writeFileSync } from 'node:fs' +import { createRequire } from 'node:module' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { copyScriptWithLocalModules } from './script-module-dependencies.mjs' +import { peImage } from './windows-pe-image-fixture.mjs' + +/** + * The wide literal `usesCygwinRuntime` holds, as it sits in a real addon. A + * fixture addon without it is a build that predates the MSYS breakaway denial, + * which is what these tests need to be able to represent. + * + * Taken from the gate itself: a re-typed copy agrees with a stale gate by + * construction, which is the one thing these fixtures must not do. + */ +const { CYGWIN_BREAKAWAY_MARKER } = createRequire(import.meta.url)('./node-pty-job-ownership.cjs') +const { CREATION_TIME_FLAG } = createRequire(import.meta.url)( + './windows-process-tree-creation-time.cjs' +) const sourceScriptPath = fileURLToPath(new URL('./rebuild-native-deps.mjs', import.meta.url)) const sourceInstallScriptPath = fileURLToPath( @@ -22,6 +37,11 @@ const sourceNodePtyJobOwnershipPath = fileURLToPath( const sourceWindowsProcessTreeGypRebuildPath = fileURLToPath( new URL('./windows-process-tree-gyp-rebuild.mjs', import.meta.url) ) +// Reached through projectRequire, so the module walker cannot see it: that +// specifier resolves against the project root, not against the script. +const sourceWindowsProcessTreeCreationTimePath = fileURLToPath( + new URL('./windows-process-tree-creation-time.cjs', import.meta.url) +) const sourceWindowsProcessTreePatchPath = fileURLToPath( new URL('../patches/@vscode__windows-process-tree@0.8.0.patch', import.meta.url) ) @@ -90,9 +110,10 @@ export function mkTempProject() { mkdirSync(join(projectDir, 'config', 'scripts'), { recursive: true }) copyFileSync(sourceScriptPath, join(projectDir, 'config', 'scripts', 'rebuild-native-deps.mjs')) copyScriptWithLocalModules(sourceInstallScriptPath, join(projectDir, 'config', 'scripts')) + copyScriptWithLocalModules(sourceNodePtyJobOwnershipPath, join(projectDir, 'config', 'scripts')) copyFileSync( - sourceNodePtyJobOwnershipPath, - join(projectDir, 'config', 'scripts', 'node-pty-job-ownership.cjs') + sourceWindowsProcessTreeCreationTimePath, + join(projectDir, 'config', 'scripts', 'windows-process-tree-creation-time.cjs') ) copyFileSync( sourceWindowsProcessTreeGypRebuildPath, @@ -310,10 +331,20 @@ process.exit(result.status ?? 0) } } -export function writeFakeNodePtyConptyPayload(projectDir, arch) { +export function writeFakeNodePtyConptyPayload( + projectDir, + arch, + { cygwinBreakawayDenied = true } = {} +) { const releaseDir = join(projectDir, 'node_modules', 'node-pty', 'build', 'Release') mkdirSync(releaseDir, { recursive: true }) - writeFileSync(join(releaseDir, 'conpty.node'), 'native addon') + writeFileSync( + join(releaseDir, 'conpty.node'), + Buffer.concat([ + peImage({ arch }), + cygwinBreakawayDenied ? CYGWIN_BREAKAWAY_MARKER : Buffer.alloc(0) + ]) + ) const sourceDir = join( projectDir, 'node_modules', @@ -328,12 +359,30 @@ export function writeFakeNodePtyConptyPayload(projectDir, arch) { writeFileSync(join(sourceDir, 'OpenConsole.exe'), `OpenConsole.exe ${arch}`) } +function writeFakeNodePtyAddon(nodePtyDir, nativeDir, { cygwinBreakawayDenied }) { + const addonDir = resolve(join(nodePtyDir, 'lib'), nativeDir) + mkdirSync(addonDir, { recursive: true }) + for (const nativeName of ['conpty', 'pty']) { + writeFileSync( + join(addonDir, `${nativeName}.node`), + Buffer.concat([ + peImage({ arch: process.arch === 'arm64' ? 'arm64' : 'x64' }), + cygwinBreakawayDenied ? CYGWIN_BREAKAWAY_MARKER : Buffer.alloc(0) + ]) + ) + } +} + export function writeFakeLoadableNodePty( projectDir, - { nativeDir = 'prebuilds/pty', ownsPtyJob = true } = {} + { nativeDir = 'prebuilds/pty', ownsPtyJob = true, cygwinBreakawayDenied = true } = {} ) { const nodePtyDir = join(projectDir, 'node_modules', 'node-pty') mkdirSync(join(nodePtyDir, 'lib'), { recursive: true }) + // Why a real file: the job-ownership gate reads the addon it was told about, + // because every job export predates the MSYS breakaway denial and so cannot + // distinguish a current build from one that leaks Git Bash children. + writeFakeNodePtyAddon(nodePtyDir, nativeDir, { cygwinBreakawayDenied }) writeFileSync(join(nodePtyDir, 'index.js'), 'module.exports = {}\n') writeFileSync( join(nodePtyDir, 'lib', 'utils.js'), @@ -366,10 +415,18 @@ export function writeFakeWindowsRegistry(projectDir) { ) } +/** + * A healthy one: the addon reports CreationTime, which is what a build of the + * patched source does and what the probe has required since the creation-time + * gate landed. Exporting nothing means "the tarball prebuilt" to that gate. + */ export function writeFakeWindowsProcessTree(projectDir) { const processTreeDir = join(projectDir, 'node_modules', '@vscode', 'windows-process-tree') mkdirSync(processTreeDir, { recursive: true }) - writeFileSync(join(processTreeDir, 'index.js'), 'module.exports = {}\n') + writeFileSync( + join(processTreeDir, 'index.js'), + `module.exports = { supportedProcessDataFlags: ${CREATION_TIME_FLAG} }\n` + ) } export function writeFakeWindowsProcessTreeWithNodeAddonApi( @@ -434,11 +491,17 @@ export function writeNodePtyPatchFile(projectDir) { writeFileSync(join(projectDir, 'config', 'patches', 'node-pty@1.1.0.patch'), 'patch marker\n') } -export function writePatchedNodePtyBuildArtifacts(projectDir) { +export function writePatchedNodePtyBuildArtifacts( + projectDir, + { cygwinBreakawayDenied = true } = {} +) { const buildDir = join(projectDir, 'node_modules', 'node-pty', 'build', 'Release') mkdirSync(buildDir, { recursive: true }) if (process.platform === 'win32') { - writeFileSync(join(buildDir, 'conpty.node'), '') + writeFileSync( + join(buildDir, 'conpty.node'), + cygwinBreakawayDenied ? CYGWIN_BREAKAWAY_MARKER : Buffer.alloc(0) + ) mkdirSync(join(buildDir, 'conpty'), { recursive: true }) writeFileSync(join(buildDir, 'conpty', 'conpty.dll'), '') writeFileSync(join(buildDir, 'conpty', 'OpenConsole.exe'), '') diff --git a/config/scripts/rebuild-native-deps.mjs b/config/scripts/rebuild-native-deps.mjs index deec8186f89..57482e3ac91 100644 --- a/config/scripts/rebuild-native-deps.mjs +++ b/config/scripts/rebuild-native-deps.mjs @@ -35,9 +35,12 @@ import { readFileSync, writeFileSync } from 'node:fs' +import { createRequire } from 'node:module' import { platform as osPlatform } from 'node:os' import { join, resolve } from 'node:path' +const requireLocal = createRequire(import.meta.url) + const projectDir = process.cwd() let cliOptions try { @@ -79,11 +82,10 @@ const NATIVE_MODULES = [ ...(rebuildPlatform === 'win32' ? ['@orca/windows-registry', '@vscode/windows-process-tree'] : []) ] const onlyModules = NATIVE_MODULES.filter((m) => !ignoreModules.includes(m)) +/** Whether this rebuild targets something other than the machine running it. */ +const isCrossHostRebuild = rebuildPlatform !== osPlatform() || rebuildArch !== process.arch const forceRebuild = - process.env.ORCA_FORCE_NATIVE_REBUILD === '1' || - cliOptions.force || - rebuildPlatform !== osPlatform() || - rebuildArch !== process.arch + process.env.ORCA_FORCE_NATIVE_REBUILD === '1' || cliOptions.force || isCrossHostRebuild let modulesToRebuild = onlyModules ensureElectronPackageInstalled() @@ -175,6 +177,7 @@ try { }) restoreNodePtyWindowsConptyRuntime() assertWindowsProcessTreeAddonIsPatched() + assertNodePtyConptyDeniesMsysBreakaway() } catch (/** @type {any} */ err) { console.error('[rebuild] Native module rebuild failed:', err?.message ?? err) if (isWindowsNativeLockError(err)) { @@ -228,6 +231,32 @@ function assertWindowsProcessTreeAddonIsPatched() { ) } +/** + * The other half of the same problem, for the addon this rebuild just produced. + * + * The Electron probe below carries the marker check too, but it is skipped + * whenever the Electron package binary is unusable -- and "covered by another + * path" is not "this path checks". Reading the binary needs neither a loadable + * Electron nor an executable target arch, so it runs here regardless. + * + * Absent is fatal on the host that will run this install: loadNativeModule + * falls through to prebuilds/win32-, and the published prebuild predates + * the denial, so the app would load it with nothing said. A cross-host rebuild + * does not necessarily leave a win32 addon on this disk, and that must not fail + * an install that was working. + */ +function assertNodePtyConptyDeniesMsysBreakaway() { + if (rebuildPlatform !== 'win32' || !modulesToRebuild.includes('node-pty')) { + return + } + const { assertRebuiltConptyDeniesMsysBreakaway } = requireLocal('./node-pty-job-ownership.cjs') + assertRebuiltConptyDeniesMsysBreakaway({ + nodePtyDir: resolve(projectDir, 'node_modules', 'node-pty'), + rebuildArch, + crossHost: isCrossHostRebuild + }) +} + function restoreNodePtyWindowsConptyRuntime() { if (rebuildPlatform !== 'win32' || !onlyModules.includes('node-pty')) { return @@ -548,14 +577,22 @@ function loadNativeModule(moduleName) { } if (moduleName === 'node-pty') { projectRequire('node-pty') - const { assertNodePtyJobOwnership } = projectRequire( + const { assertNodePtyJobOwnership, nodePtyAddonPath } = projectRequire( './config/scripts/node-pty-job-ownership.cjs' ) const { loadNativeModule } = projectRequire('node-pty/lib/utils') const nativeName = getNodePtyNativeModuleName() const native = loadNativeModule(nativeName) assertNodePtyWindowsConptyRuntime(native.dir) - assertNodePtyJobOwnership({ nativeName, native }) + assertNodePtyJobOwnership({ + nativeName, + native, + addonPath: nodePtyAddonPath( + projectRequire.resolve('node-pty/lib/utils'), + native, + nativeName + ) + }) if (requirePatchedNodePtySourceBuild && !isNodePtyReleaseBuildDir(native.dir)) { throw new Error( 'node-pty resolved to ' + diff --git a/config/scripts/script-module-dependencies.mjs b/config/scripts/script-module-dependencies.mjs index 02e9e174283..b92db587a81 100644 --- a/config/scripts/script-module-dependencies.mjs +++ b/config/scripts/script-module-dependencies.mjs @@ -19,7 +19,17 @@ function collectScriptModules(scriptPath, seen = new Set()) { return seen } seen.add(scriptPath) - for (const [, specifier] of readFileSync(scriptPath, 'utf8').matchAll(/from '(\.\/[^']+)'/g)) { + // `from`, bare and dynamic `import`, and plain `require` -- the Windows gates + // are .cjs, and a module reached only by require or by a side-effect import is + // the one nobody notices is missing until a subprocess fails with a + // resolution error instead. Deliberately not `projectRequire`/`requireLocal` + // wrappers: those specifiers are resolved against the project root at runtime, + // not against this file, so following them would stage the wrong path. + const source = readFileSync(scriptPath, 'utf8') + const specifiers = source.matchAll( + /(?:\bfrom|\brequire\s*\(|\bimport\s*\(|\bimport)\s*'(\.\/[^']+)'/g + ) + for (const [, specifier] of specifiers) { collectScriptModules(join(dirname(scriptPath), specifier), seen) } return seen diff --git a/config/scripts/script-module-dependencies.test.mjs b/config/scripts/script-module-dependencies.test.mjs new file mode 100644 index 00000000000..998226e0427 --- /dev/null +++ b/config/scripts/script-module-dependencies.test.mjs @@ -0,0 +1,101 @@ +import { existsSync, mkdtempSync, readdirSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' +import { copyScriptWithLocalModules } from './script-module-dependencies.mjs' + +const fixtureDir = mkdtempSync(join(tmpdir(), 'script-module-dependencies-')) + +function sourceTree(files) { + const sourceDir = mkdtempSync(join(fixtureDir, 'source-')) + for (const [name, contents] of Object.entries(files)) { + writeFileSync(join(sourceDir, name), contents) + } + return sourceDir +} + +function copiedNames(files, entryName) { + const sourceDir = sourceTree(files) + const destinationDir = join(mkdtempSync(join(fixtureDir, 'dest-')), 'scripts') + copyScriptWithLocalModules(join(sourceDir, entryName), destinationDir) + return readdirSync(destinationDir).sort() +} + +describe('copyScriptWithLocalModules', () => { + it('takes the entry script itself', () => { + expect(copiedNames({ 'entry.mjs': 'export const a = 1\n' }, 'entry.mjs')).toEqual(['entry.mjs']) + }) + + it('follows a co-located import', () => { + expect( + copiedNames( + { 'entry.mjs': "import { a } from './dep.mjs'\n", 'dep.mjs': 'export const a = 1\n' }, + 'entry.mjs' + ) + ).toEqual(['dep.mjs', 'entry.mjs']) + }) + + // The Windows addon gates are .cjs and reach each other by require. A module + // pulled in only that way used to be left behind, and the subprocess then + // failed with a resolution error that looks nothing like the defect it hides. + it('follows a co-located require, not only an import', () => { + expect( + copiedNames( + { + 'entry.cjs': "const { a } = require('./dep.cjs')\nmodule.exports = { a }\n", + 'dep.cjs': 'module.exports = { a: 1 }\n' + }, + 'entry.cjs' + ) + ).toEqual(['dep.cjs', 'entry.cjs']) + }) + + it('follows a require reached only through an imported module', () => { + const names = copiedNames( + { + 'entry.mjs': "import './middle.cjs'\n", + 'middle.cjs': "require('./leaf.cjs')\n", + 'leaf.cjs': 'module.exports = {}\n' + }, + 'entry.mjs' + ) + expect(names).toContain('leaf.cjs') + }) + + it('leaves package and builtin specifiers alone', () => { + const sourceDir = sourceTree({ + 'entry.mjs': "import { join } from 'node:path'\nimport x from 'some-package'\n" + }) + const destinationDir = join(mkdtempSync(join(fixtureDir, 'dest-')), 'scripts') + copyScriptWithLocalModules(join(sourceDir, 'entry.mjs'), destinationDir) + expect(readdirSync(destinationDir)).toEqual(['entry.mjs']) + }) + + it('terminates on a cycle rather than recursing forever', () => { + expect( + copiedNames({ 'a.mjs': "import './b.mjs'\n", 'b.mjs': "import './a.mjs'\n" }, 'a.mjs') + ).toEqual(['a.mjs', 'b.mjs']) + }) + + it('creates the destination directory it was handed', () => { + const sourceDir = sourceTree({ 'entry.mjs': 'export const a = 1\n' }) + const destinationDir = join(mkdtempSync(join(fixtureDir, 'dest-')), 'nested', 'scripts') + copyScriptWithLocalModules(join(sourceDir, 'entry.mjs'), destinationDir) + expect(existsSync(join(destinationDir, 'entry.mjs'))).toBe(true) + }) + + // The real tree this stages: the packaged-addon gate reaches its PE reader by + // require, so a walker that missed it would break every rebuild fixture. + it('stages the node-pty job-ownership gate with everything it requires', () => { + const destinationDir = join(mkdtempSync(join(fixtureDir, 'dest-')), 'scripts') + copyScriptWithLocalModules( + fileURLToPath(new URL('./node-pty-job-ownership.cjs', import.meta.url)), + destinationDir + ) + expect(readdirSync(destinationDir).sort()).toEqual([ + 'node-pty-job-ownership.cjs', + 'windows-pe-machine.cjs' + ]) + }) +}) diff --git a/config/scripts/verify-packaged-node-pty-job-ownership.cjs b/config/scripts/verify-packaged-node-pty-job-ownership.cjs index 5ba21cee8ec..2d746d23661 100644 --- a/config/scripts/verify-packaged-node-pty-job-ownership.cjs +++ b/config/scripts/verify-packaged-node-pty-job-ownership.cjs @@ -1,11 +1,52 @@ +const { existsSync } = require('node:fs') const { createRequire } = require('node:module') const { join } = require('node:path') -const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs') +const { + assertNodePtyJobOwnership, + conptyDeniesCygwinBreakaway, + nodePtyAddonPath, + staleConptySourceBuildError +} = require('./node-pty-job-ownership.cjs') +const { normalizeNodePtyWindowsArch } = require('../packaged-runtime-node-modules.cjs') +const { PE_MACHINE, describePeMachine, readPeMachine } = require('./windows-pe-machine.cjs') + +/** + * Every conpty.node the packaged tree can hand `loadNativeModule`, in its order. + * + * Why the order matters: the loader swallows each require failure and falls + * through, so a wrong-arch or otherwise unloadable build hands the pane to the + * next candidate. First loadable wins, and the published prebuild is always the + * last one standing. + */ +function packagedConptyCandidates(resourcesDir, targetArch) { + const nodePtyDir = join(resourcesDir, 'node_modules', 'node-pty') + const layouts = [ + { segments: ['build', 'Release'], prebuilt: false }, + { segments: ['build', 'Debug'], prebuilt: false }, + { segments: ['prebuilds', `win32-${targetArch}`], prebuilt: true } + ] + // Each layout is tried relative to node-pty's root, then to lib/, before the + // next layout -- the unbundled then bundled pair node-pty's loader walks. + return layouts.flatMap(({ segments, prebuilt }) => + [nodePtyDir, join(nodePtyDir, 'lib')].map((root) => ({ + path: join(root, ...segments, 'conpty.node'), + prebuilt + })) + ) +} + +function describeCandidates(candidates) { + return candidates + .map((candidate) => `${candidate.path} (${describePeMachine(candidate.machine)})`) + .join(', ') +} function loadPackagedConpty(resourcesDir) { const packagedRequire = createRequire(join(resourcesDir, 'package.json')) - const { loadNativeModule } = packagedRequire('./node_modules/node-pty/lib/utils') - return loadNativeModule('conpty') + const utilsPath = packagedRequire.resolve('./node_modules/node-pty/lib/utils') + const { loadNativeModule } = packagedRequire(utilsPath) + const native = loadNativeModule('conpty') + return { native, addonPath: nodePtyAddonPath(utilsPath, native, 'conpty') } } function verifyPackagedNodePtyJobOwnership(resourcesDir, options = {}) { @@ -14,12 +55,145 @@ function verifyPackagedNodePtyJobOwnership(resourcesDir, options = {}) { return } - const native = (options.loadNative ?? loadPackagedConpty)(resourcesDir) - assertNodePtyJobOwnership({ platform, nativeName: 'conpty', native }) + const { native, addonPath } = (options.loadNative ?? loadPackagedConpty)(resourcesDir) + assertNodePtyJobOwnership({ platform, nativeName: 'conpty', native, addonPath }) if (!native.dir.replace(/\\/g, '/').includes('build/Release/')) { throw new Error(`Packaged node-pty resolved to ${native.dir}; expected patched build/Release`) } console.log('[verify-packaged-node-pty] OK — packaged ConPTY owns process trees') } -module.exports = { verifyPackagedNodePtyJobOwnership } +/** + * The half of the packaged check that survives a cross-host build. + * + * The export check has to load the addon, so it cannot run when the packaging + * host is not the target platform/arch -- and that skip is how a Windows + * release built elsewhere could ship a node-pty that leaks every MSYS pane + * child out of its job. Reading the binary needs neither. + * + * It resolves the addon the way the loader does rather than reading one path: + * only the PE machine field separates a cross-arch package that built correctly + * from one whose rebuild silently emitted the host's arch, and the first is a + * correct package whose leftover prebuild is never reached. See the table in + * docs/reference/windows-msys-job-breakaway.md. + * + * Nothing loadable is fatal, not skipped: that package has no ConPTY backend, + * which a gate must not shrug at. + */ +function verifyPackagedConptyBreakawayMarker(resourcesDir, targetArch, options = {}) { + // Deliberately no host-platform gate: the caller has already established that + // the *target* is Windows, and gating on the host is the very skip this + // closes. + const architecture = normalizeNodePtyWindowsArch(targetArch) + const candidates = packagedConptyCandidates(resourcesDir, architecture) + const exists = options.exists ?? existsSync + const present = candidates.filter((candidate) => exists(candidate.path)) + if (present.length === 0) { + throw new Error( + [ + `Packaged node-pty for win32-${architecture} has no conpty.node on any path its loader`, + `tries (${candidates.map((c) => c.path).join(', ')}), so the packaged app has no`, + 'ConPTY backend at all.', + 'Nothing here can be checked for the Cygwin/MSYS job-breakaway denial, and a gate that', + 'cannot see its subject refuses rather than assume.' + ].join(' ') + ) + } + // Read once: the same header answers "which one loads" and "what did we find". + const inspected = present.map((candidate) => ({ + ...candidate, + machine: readPeMachine(candidate.path) + })) + const loaded = inspected.find((candidate) => candidate.machine === PE_MACHINE[architecture]) + if (!loaded) { + throw new Error( + [ + `Packaged node-pty for win32-${architecture} has conpty.node at`, + `${describeCandidates(inspected)},`, + 'and the app can load none of them: a Windows process only loads a PE of its own', + `machine, which for win32-${architecture} is`, + `0x${PE_MACHINE[architecture].toString(16)}.`, + 'Rebuild node-pty for the target architecture and repackage.' + ].join(' ') + ) + } + const addonPath = loaded.path + if (conptyDeniesCygwinBreakaway(addonPath)) { + console.log( + `[verify-packaged-node-pty] OK — win32-${architecture} loads ${addonPath}, which denies ` + + 'MSYS job breakaway' + ) + return + } + if (!loaded.prebuilt) { + throw staleConptySourceBuildError(addonPath) + } + // Past here the app falls back to the published prebuild, which never carries + // the patch. Why it fell back decides the remedy, and the three are different + // enough that naming the wrong one wastes the reader's build. + const unusableSourceBuilds = inspected.filter((candidate) => !candidate.prebuilt) + if (unusableSourceBuilds.some((candidate) => candidate.machine === null)) { + throw new Error( + [ + `Packaged node-pty for win32-${architecture} falls back to ${addonPath}, which predates`, + 'the Cygwin/MSYS job-breakaway denial, because the source build beside it is not a PE', + `image at all: ${describeCandidates(unusableSourceBuilds)}.`, + 'A truncated, empty or quarantined build artifact looks like this. Rebuild node-pty and', + 'repackage. See docs/reference/windows-msys-job-breakaway.md.' + ].join(' ') + ) + } + if (unusableSourceBuilds.length > 0) { + throw new Error( + [ + `Packaged node-pty for win32-${architecture} falls back to ${addonPath}, which predates`, + 'the Cygwin/MSYS job-breakaway denial, because the source build beside it is the wrong', + `architecture: ${describeCandidates(unusableSourceBuilds)}.`, + 'A cross-arch rebuild that did not honour --arch looks exactly like this. Re-run', + `config/scripts/rebuild-native-deps.mjs --platform=win32 --arch=${architecture},`, + 'confirm it emitted a conpty.node of that machine, and repackage.', + 'See docs/reference/windows-msys-job-breakaway.md.' + ].join(' ') + ) + } + throw new Error( + [ + `Packaged node-pty for win32-${architecture} loads ${addonPath}, the published prebuilt`, + 'fallback, which predates the Cygwin/MSYS job-breakaway denial: its per-PTY job still', + 'carries JOB_OBJECT_LIMIT_BREAKAWAY_OK, so every Git Bash pane child is created outside', + 'the job and survives terminatePtyJob.', + 'It is here because this package holds no node-pty source build at all for', + 'prunePackagedNodePty to have replaced it with, and only a host that can build node-pty', + `for win32-${architecture} produces one.`, + `If this IS a Windows ${architecture} host, the rebuild did not leave one -- check the`, + 'beforeBuild output above. Otherwise package this Windows slice on a host that can.', + 'See docs/reference/windows-msys-job-breakaway.md.' + ].join(' ') + ) +} + +/** + * The whole Windows verdict for one packaged slice. + * + * Both halves live here rather than in the afterPack hook so that "the marker + * sweep runs even when the export check cannot" is a tested claim instead of + * the shape of an if/else somebody could re-nest. + */ +function verifyPackagedWindowsNodePty(resourcesDir, targetArch, options = {}) { + ;(options.verifyMarker ?? verifyPackagedConptyBreakawayMarker)(resourcesDir, targetArch) + const hostPlatform = options.hostPlatform ?? process.platform + if (hostPlatform !== 'win32' || !options.canExecuteTargetArch) { + console.log( + '[verify-packaged-node-pty] skipped the export check on a cross-platform or cross-arch package' + ) + return + } + ;(options.verifyExports ?? verifyPackagedNodePtyJobOwnership)(resourcesDir) +} + +module.exports = { + packagedConptyCandidates, + verifyPackagedConptyBreakawayMarker, + verifyPackagedNodePtyJobOwnership, + verifyPackagedWindowsNodePty +} diff --git a/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs b/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs index 26a33cfbbd6..9d2317c8a33 100644 --- a/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs +++ b/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs @@ -1,10 +1,53 @@ +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' import { createRequire } from 'node:module' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' import { describe, expect, it, vi } from 'vitest' +import { peImage } from './windows-pe-image-fixture.mjs' const require = createRequire(import.meta.url) const { - verifyPackagedNodePtyJobOwnership + packagedConptyCandidates, + verifyPackagedConptyBreakawayMarker, + verifyPackagedNodePtyJobOwnership, + verifyPackagedWindowsNodePty } = require('./verify-packaged-node-pty-job-ownership.cjs') +const { CYGWIN_BREAKAWAY_MARKER } = require('./node-pty-job-ownership.cjs') + +const fixtureDir = mkdtempSync(join(tmpdir(), 'packaged-node-pty-job-')) +const ELECTRON_BUILDER_CONFIG = readFileSync( + new URL('../electron-builder.config.cjs', import.meta.url), + 'utf8' +) + +/** A real enough addon: a machine field the arch check reads, and the marker. */ +function conptyImage({ arch = 'x64', cygwinBreakawayDenied = true } = {}) { + return Buffer.concat([ + peImage({ arch }), + cygwinBreakawayDenied ? CYGWIN_BREAKAWAY_MARKER : Buffer.alloc(0) + ]) +} + +function writeAddon(name, options) { + const path = join(fixtureDir, name) + writeFileSync(path, conptyImage(options)) + return path +} + +/** A packaged resources tree carrying exactly the conpty.node files named. */ +function packagedResources(addons) { + const resourcesDir = mkdtempSync(join(fixtureDir, 'resources-')) + const nodePtyDir = join(resourcesDir, 'node_modules', 'node-pty') + for (const [relativePath, options] of Object.entries(addons)) { + const addonPath = join(nodePtyDir, ...relativePath.split('/')) + mkdirSync(dirname(addonPath), { recursive: true }) + writeFileSync(addonPath, conptyImage(options)) + } + return resourcesDir +} + +const CURRENT_ADDON = writeAddon('current.node', { cygwinBreakawayDenied: true }) +const PRE_MSYS_ADDON = writeAddon('pre-msys.node', { cygwinBreakawayDenied: false }) const PATCHED = { dir: '../build/Release/', @@ -15,31 +58,39 @@ const PATCHED = { } } +const packaged = (native, addonPath = CURRENT_ADDON) => ({ + platform: 'win32', + loadNative: () => ({ native, addonPath }) +}) + describe('verifyPackagedNodePtyJobOwnership', () => { it('accepts the packaged patched ConPTY binding', () => { - expect(() => - verifyPackagedNodePtyJobOwnership('resources', { - platform: 'win32', - loadNative: () => PATCHED - }) - ).not.toThrow() + expect(() => verifyPackagedNodePtyJobOwnership('resources', packaged(PATCHED))).not.toThrow() }) it('rejects a packaged upstream prebuild', () => { expect(() => - verifyPackagedNodePtyJobOwnership('resources', { - platform: 'win32', - loadNative: () => ({ dir: '../prebuilds/win32-x64/', module: {} }) - }) + verifyPackagedNodePtyJobOwnership( + 'resources', + packaged({ dir: '../prebuilds/win32-x64/', module: {} }) + ) ).toThrow(/missing listJobProcessIds, terminateJob, assignCurrentProcessToJob/) }) + // A release built against a stale native cache ships the MSYS orphan bug + // while exporting every job function, so packaging has to read the binary. + it('rejects a packaged build that predates the Cygwin/MSYS breakaway denial', () => { + expect(() => + verifyPackagedNodePtyJobOwnership('resources', packaged(PATCHED, PRE_MSYS_ADDON)) + ).toThrow(/predates the Cygwin\/MSYS job-breakaway denial/) + }) + it('requires the patched source-build directory', () => { expect(() => - verifyPackagedNodePtyJobOwnership('resources', { - platform: 'win32', - loadNative: () => ({ ...PATCHED, dir: '../prebuilds/win32-x64/' }) - }) + verifyPackagedNodePtyJobOwnership( + 'resources', + packaged({ ...PATCHED, dir: '../prebuilds/win32-x64/' }) + ) ).toThrow(/expected patched build\/Release/) }) @@ -49,3 +100,331 @@ describe('verifyPackagedNodePtyJobOwnership', () => { expect(loadNative).not.toHaveBeenCalled() }) }) + +describe('packagedConptyCandidates', () => { + // Pinned because the gate resolves the addon by walking this list in order: + // a wrong order blesses a binary the app would never reach. + it('walks the paths node-pty tries, in node-pty order', () => { + expect(packagedConptyCandidates('RES', 'arm64').map((candidate) => candidate.path)).toEqual([ + join('RES', 'node_modules', 'node-pty', 'build', 'Release', 'conpty.node'), + join('RES', 'node_modules', 'node-pty', 'lib', 'build', 'Release', 'conpty.node'), + join('RES', 'node_modules', 'node-pty', 'build', 'Debug', 'conpty.node'), + join('RES', 'node_modules', 'node-pty', 'lib', 'build', 'Debug', 'conpty.node'), + join('RES', 'node_modules', 'node-pty', 'prebuilds', 'win32-arm64', 'conpty.node'), + join('RES', 'node_modules', 'node-pty', 'lib', 'prebuilds', 'win32-arm64', 'conpty.node') + ]) + }) + + // Only the published prebuild gets the "no rebuild here can fix this" advice. + it('knows which of them node-pty publishes prebuilt', () => { + expect(packagedConptyCandidates('RES', 'x64').map((candidate) => candidate.prebuilt)).toEqual([ + false, + false, + false, + false, + true, + true + ]) + }) +}) + +describe('verifyPackagedConptyBreakawayMarker', () => { + // The release as built today: prunePackagedNodePty already dropped the + // same-arch prebuild because a patched source build replaced it. + it('passes a package whose only ConPTY load path carries the denial', () => { + const resourcesDir = packagedResources({ 'build/Release/conpty.node': {} }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).not.toThrow() + }) + + // The cross-host package. No host but Windows can build conpty.node, so there + // is no build/Release for prune to have replaced the prebuild with -- and the + // published prebuild is exactly the binary that leaks every MSYS pane child. + it('fails a cross-host package left holding the published prebuild', () => { + const resourcesDir = packagedResources({ + 'prebuilds/win32-x64/conpty.node': { cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow( + /predates the Cygwin\/MSYS job-breakaway denial/ + ) + }) + + it('tells that package how to fix it, which is not a rebuild it can run', () => { + const resourcesDir = packagedResources({ + 'prebuilds/win32-x64/conpty.node': { cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow( + /this package holds no node-pty source build at all[\s\S]*Otherwise package this Windows slice on a host that can/ + ) + }) + + // The same state reaches this from a capable host too, when the rebuild left + // nothing: telling that packager to change hosts would send them nowhere. + it('does not assume the packaging host is the wrong one', () => { + const resourcesDir = packagedResources({ + 'prebuilds/win32-x64/conpty.node': { cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow( + /If this IS a Windows x64 host, the rebuild did not leave one/ + ) + }) + + // The cross-arch package that worked: beforeBuild rebuilds node-pty for the + // TARGET arch, so build/Release is patched and loadable and the prebuild + // prune left behind is never reached. Failing this would be a false positive + // whose advice -- change hosts -- is both wrong and impossible. + it('passes a cross-arch package whose build/Release really is the target arch', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': { arch: 'arm64' }, + 'prebuilds/win32-arm64/conpty.node': { arch: 'arm64', cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).not.toThrow() + }) + + // The cross-arch package that silently did not: build/Release is the + // packaging host's own arch, the target cannot load it, and the loader falls + // through to the unpatched prebuild underneath. + it('fails a cross-arch package whose build/Release is the packaging host arch', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': { arch: 'x64' }, + 'prebuilds/win32-arm64/conpty.node': { arch: 'arm64', cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).toThrow( + /prebuilds[\\/]win32-arm64/ + ) + }) + + it('refuses a package whose every conpty.node is the wrong architecture', () => { + const resourcesDir = packagedResources({ 'build/Release/conpty.node': { arch: 'x64' } }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).toThrow( + /the app can load none of them/ + ) + }) + + // Naming the machine it found is what separates a cross-arch build from a + // truncated download, which are the same "cannot load this" to the loader. + it('names what it found rather than guessing why', () => { + const resourcesDir = packagedResources({ 'build/Release/conpty.node': { arch: 'x64' } }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).toThrow( + /machine 0x8664[\s\S]*0xaa64/ + ) + }) + + it('calls a candidate that is not a PE image what it is', () => { + const resourcesDir = packagedResources({}) + const addonPath = join( + resourcesDir, + 'node_modules', + 'node-pty', + 'build', + 'Release', + 'conpty.node' + ) + mkdirSync(dirname(addonPath), { recursive: true }) + writeFileSync(addonPath, Buffer.alloc(0x200)) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow(/not a PE image/) + }) + + // A truncated or quarantined artifact reaches the gate looking exactly like a + // cross-arch build, and "re-run with --arch" is not the command that fixes it. + it('does not blame --arch for a source build that is not a PE image', () => { + const resourcesDir = packagedResources({ + 'prebuilds/win32-x64/conpty.node': { cygwinBreakawayDenied: false } + }) + const addonPath = join( + resourcesDir, + 'node_modules', + 'node-pty', + 'build', + 'Release', + 'conpty.node' + ) + mkdirSync(dirname(addonPath), { recursive: true }) + writeFileSync(addonPath, Buffer.alloc(0x200)) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow( + /is not a PE image at all[\s\S]*truncated, empty or quarantined/ + ) + }) + + // The remedy for this one is a rebuild, not a different host, and the + // difference is a build somebody has to run twice to find out. + it('blames the wrong-arch source build rather than the host, when there is one', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': { arch: 'x64' }, + 'prebuilds/win32-arm64/conpty.node': { arch: 'arm64', cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).toThrow( + /the source build beside it is the wrong architecture[\s\S]*machine 0x8664/ + ) + }) + + it('tells that build the command that would fix it', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': { arch: 'x64' }, + 'prebuilds/win32-arm64/conpty.node': { arch: 'arm64', cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).toThrow( + /rebuild-native-deps\.mjs --platform=win32 --arch=arm64/ + ) + }) + + it('does not tell it to change hosts, which would not help', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': { arch: 'x64' }, + 'prebuilds/win32-arm64/conpty.node': { arch: 'arm64', cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).toThrow( + /^(?![\s\S]*Package this Windows slice on such a host)[\s\S]*$/ + ) + }) + + it('ignores a prebuild for an arch this slice will never load', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': {}, + 'prebuilds/win32-arm64/conpty.node': { arch: 'arm64', cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).not.toThrow() + }) + + // build/Debug sits between Release and the prebuilds in the load order, and + // nothing prunes it, so it wins whenever Release cannot be loaded. + it('resolves past a Release build the target cannot load', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': { arch: 'x64' }, + 'build/Debug/conpty.node': { arch: 'arm64', cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'arm64')).toThrow( + /build[\\/]Debug/ + ) + }) + + // A stale source build is the packaging host's own to rebuild, so it gets the + // advice that actually works rather than the cross-host one. + it('tells a stale source build to rebuild, not to change hosts', () => { + const resourcesDir = packagedResources({ + 'build/Release/conpty.node': { cygwinBreakawayDenied: false } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow( + /Rebuild node-pty from source/ + ) + }) + + // Nothing to load is not "a layout we do not recognise", it is a package with + // no ConPTY backend, and a gate that cannot see its subject is not a gate. + it('refuses rather than skip a package with no conpty.node at all', () => { + const resourcesDir = packagedResources({}) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow( + /no conpty\.node on any path its loader tries/ + ) + }) + + it('names every path it looked at when it finds none', () => { + const resourcesDir = packagedResources({}) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow( + /build[\\/]Release[\s\S]*build[\\/]Debug[\s\S]*prebuilds[\\/]win32-x64/ + ) + }) + + // Present but unreadable is the state that used to pass, so it must not warn. + // The read error itself is the message; the point is that it does not return. + it('fails rather than pass a candidate it cannot read', () => { + const resourcesDir = packagedResources({}) + mkdirSync(join(resourcesDir, 'node_modules', 'node-pty', 'build', 'Release', 'conpty.node'), { + recursive: true + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'x64')).toThrow() + }) + + it('accepts the electron-builder Arch enum the afterPack hook passes', () => { + const resourcesDir = packagedResources({ + 'prebuilds/win32-arm64/conpty.node': { arch: 'arm64' } + }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 3)).not.toThrow() + }) + + it('refuses a target arch no Windows slice ships', () => { + const resourcesDir = packagedResources({ 'build/Release/conpty.node': {} }) + expect(() => verifyPackagedConptyBreakawayMarker(resourcesDir, 'ia32')).toThrow( + /Unsupported packaged node-pty Windows architecture/ + ) + }) + + it('looks where electron-builder actually lands the addon', () => { + const exists = vi.fn().mockReturnValue(false) + expect(() => + verifyPackagedConptyBreakawayMarker(join('out', 'win-unpacked', 'resources'), 'x64', { + exists + }) + ).toThrow() + expect(exists).toHaveBeenCalledWith( + join( + 'out', + 'win-unpacked', + 'resources', + 'node_modules', + 'node-pty', + 'build', + 'Release', + 'conpty.node' + ) + ) + }) +}) + +describe('verifyPackagedWindowsNodePty', () => { + const spies = () => ({ verifyMarker: vi.fn(), verifyExports: vi.fn() }) + + // The bug this replaced: the marker check sat in the else of the host gate, so + // the cross-host package it exists for was the one package it never checked. + it.each([ + ['a cross-platform host', { hostPlatform: 'darwin', canExecuteTargetArch: true }], + ['a cross-arch slice', { hostPlatform: 'win32', canExecuteTargetArch: false }], + ['both', { hostPlatform: 'linux', canExecuteTargetArch: false }], + ['neither', { hostPlatform: 'win32', canExecuteTargetArch: true }] + ])('checks the marker on %s', (_case, host) => { + const { verifyMarker, verifyExports } = spies() + verifyPackagedWindowsNodePty('resources', 'x64', { ...host, verifyMarker, verifyExports }) + expect(verifyMarker).toHaveBeenCalledWith('resources', 'x64') + }) + + it('loads the addon for the export check only where that can work', () => { + const { verifyMarker, verifyExports } = spies() + verifyPackagedWindowsNodePty('resources', 'x64', { + hostPlatform: 'win32', + canExecuteTargetArch: true, + verifyMarker, + verifyExports + }) + expect(verifyExports).toHaveBeenCalledWith('resources') + }) + + it.each([ + ['a cross-platform host', { hostPlatform: 'darwin', canExecuteTargetArch: true }], + ['a cross-arch slice', { hostPlatform: 'win32', canExecuteTargetArch: false }] + ])('skips the export check on %s', (_case, host) => { + const { verifyMarker, verifyExports } = spies() + verifyPackagedWindowsNodePty('resources', 'x64', { ...host, verifyMarker, verifyExports }) + expect(verifyExports).not.toHaveBeenCalled() + }) + + // Swallowing the marker verdict would leave a gate that runs and decides + // nothing, which is the failure mode this whole change is about. + it('lets the marker verdict fail the package', () => { + const verifyMarker = vi.fn(() => { + throw new Error('predates the Cygwin/MSYS job-breakaway denial') + }) + expect(() => + verifyPackagedWindowsNodePty('resources', 'x64', { + hostPlatform: 'win32', + canExecuteTargetArch: true, + verifyMarker, + verifyExports: vi.fn() + }) + ).toThrow(/predates the Cygwin\/MSYS job-breakaway denial/) + }) + + it('is what the afterPack hook calls for a Windows slice', () => { + expect(ELECTRON_BUILDER_CONFIG).toContain( + 'verifyPackagedWindowsNodePty(resourcesDir, context.arch, { canExecuteTargetArch })' + ) + }) +}) diff --git a/config/scripts/windows-pe-image-fixture.mjs b/config/scripts/windows-pe-image-fixture.mjs new file mode 100644 index 00000000000..84d4e322ca4 --- /dev/null +++ b/config/scripts/windows-pe-image-fixture.mjs @@ -0,0 +1,22 @@ +import { createRequire } from 'node:module' + +const { PE_MACHINE } = createRequire(import.meta.url)('./windows-pe-machine.cjs') + +/** + * A PE image with nothing in it but a readable `IMAGE_FILE_HEADER.Machine`. + * + * Fixtures need this because the Windows addon gates read the binary: one that + * is not a PE cannot stand in for an addon whose architecture decides whether + * the app loads it at all. + */ +export function peImage({ arch = 'x64', machine, peOffset = 0x80, signature = 'PE\0\0' } = {}) { + if (machine === undefined && PE_MACHINE[arch] === undefined) { + throw new Error(`No PE machine value for ${arch}; a fixture must not invent one.`) + } + const image = Buffer.alloc(peOffset + 8) + image.write('MZ', 0, 'latin1') + image.writeUInt32LE(peOffset, 0x3c) + image.write(signature, peOffset, 'latin1') + image.writeUInt16LE(machine ?? PE_MACHINE[arch], peOffset + 4) + return image +} diff --git a/config/scripts/windows-pe-machine.cjs b/config/scripts/windows-pe-machine.cjs new file mode 100644 index 00000000000..6eb2c440a66 --- /dev/null +++ b/config/scripts/windows-pe-machine.cjs @@ -0,0 +1,40 @@ +'use strict' + +const { closeSync, openSync, readSync } = require('node:fs') + +/** PE `IMAGE_FILE_HEADER.Machine` values, so a cross-build cannot silently emit host arch. */ +const PE_MACHINE = { x64: 0x8664, arm64: 0xaa64 } + +/** + * `IMAGE_FILE_HEADER.Machine`, or null when the file is not a PE image. + * + * Null rather than a throw because callers ask this of files they did not + * produce: a truncated or non-PE binary is a thing to decide about, not a crash. + */ +function readPeMachine(binaryPath) { + const fd = openSync(binaryPath, 'r') + try { + const dosHeader = Buffer.alloc(0x40) + if (readSync(fd, dosHeader, 0, 0x40, 0) < 0x40 || dosHeader.toString('latin1', 0, 2) !== 'MZ') { + return null + } + const peOffset = dosHeader.readUInt32LE(0x3c) + const peHeader = Buffer.alloc(6) + if ( + readSync(fd, peHeader, 0, 6, peOffset) < 6 || + peHeader.toString('latin1', 0, 4) !== 'PE\0\0' + ) { + return null + } + return peHeader.readUInt16LE(4) + } finally { + closeSync(fd) + } +} + +/** How to name a machine field in an error, including the file that has none. */ +function describePeMachine(machine) { + return machine === null ? 'not a PE image' : `machine 0x${machine.toString(16)}` +} + +module.exports = { PE_MACHINE, describePeMachine, readPeMachine } diff --git a/config/scripts/windows-pe-machine.test.mjs b/config/scripts/windows-pe-machine.test.mjs new file mode 100644 index 00000000000..6d907451d39 --- /dev/null +++ b/config/scripts/windows-pe-machine.test.mjs @@ -0,0 +1,85 @@ +import { mkdtempSync, writeFileSync } from 'node:fs' +import { createRequire } from 'node:module' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { peImage } from './windows-pe-image-fixture.mjs' + +const require = createRequire(import.meta.url) +const { PE_MACHINE, describePeMachine, readPeMachine } = require('./windows-pe-machine.cjs') + +const fixtureDir = mkdtempSync(join(tmpdir(), 'windows-pe-machine-')) + +function writeImage(name, build) { + const path = join(fixtureDir, name) + writeFileSync(path, build()) + return path +} + +const X64 = writeImage('x64.node', () => peImage({ machine: PE_MACHINE.x64 })) +const ARM64 = writeImage('arm64.node', () => peImage({ machine: PE_MACHINE.arm64 })) + +describe('PE_MACHINE', () => { + // Spelled out rather than taken from the module: the fixtures below build + // their headers from these, so a table that is wrong in both entries would + // otherwise agree with itself. + it('holds the IMAGE_FILE_MACHINE values Windows actually stamps', () => { + expect(PE_MACHINE).toEqual({ x64: 0x8664, arm64: 0xaa64 }) + }) +}) + +describe('readPeMachine', () => { + it.each([ + ['x64', X64, PE_MACHINE.x64], + ['arm64', ARM64, PE_MACHINE.arm64] + ])('reads the machine field of a %s image', (_case, path, expected) => { + expect(readPeMachine(path)).toBe(expected) + }) + + // Callers ask this of files they did not produce, so anything that is not a + // PE has to be an answer rather than a crash. + it.each([ + ['a Mach-O or ELF binary', () => Buffer.alloc(0x200)], + ['a file too short to hold a DOS header', () => Buffer.from('MZ')], + [ + 'a DOS stub whose PE offset points nowhere', + () => peImage({ machine: 0x8664, peOffset: 0x8000 }).subarray(0, 0x88) + ], + ['a file with no PE signature', () => peImage({ machine: 0x8664, signature: 'XX\0\0' })] + ])('returns null for %s', (_case, build) => { + expect(readPeMachine(writeImage(`not-pe-${Math.random()}.bin`, build))).toBeNull() + }) + + it('respects the DOS header pointer rather than a fixed offset', () => { + const path = writeImage('shifted.node', () => + peImage({ machine: PE_MACHINE.arm64, peOffset: 0x120 }) + ) + expect(readPeMachine(path)).toBe(PE_MACHINE.arm64) + }) +}) + +describe('peImage fixture', () => { + // A fixture that quietly stamps machine 0x0000 for an arch it does not know + // is the same species of silent lie these gates exist to catch. + it('refuses to invent a machine value for an arch it has none for', () => { + expect(() => peImage({ arch: 'ia32' })).toThrow(/must not invent one/) + }) + + it('still takes an explicit machine, which is how the non-PE cases are built', () => { + expect(readPeMachine(writeImage('explicit.node', () => peImage({ machine: 0x1234 })))).toBe( + 0x1234 + ) + }) +}) + +describe('describePeMachine', () => { + // The callers put this straight into an error, and "not a PE image" is a + // different problem from a cross-arch build. + it('names a machine field it read', () => { + expect(describePeMachine(PE_MACHINE.arm64)).toBe('machine 0xaa64') + }) + + it('says so when there was none, rather than throwing on null', () => { + expect(describePeMachine(null)).toBe('not a PE image') + }) +}) diff --git a/docs/reference/windows-msys-job-breakaway.md b/docs/reference/windows-msys-job-breakaway.md new file mode 100644 index 00000000000..f4221078afb --- /dev/null +++ b/docs/reference/windows-msys-job-breakaway.md @@ -0,0 +1,137 @@ +# Why an MSYS pane's children escape the per-PTY job + +Every child started from a Git Bash / MSYS2 / Cygwin pane leaves the pane's job +object unless the job is created **without** `JOB_OBJECT_LIMIT_BREAKAWAY_OK`. +`terminatePtyJob` then reports `terminated` and leaves the child running — the +orphan that holds a worktree directory open. + +The denial is already in `config/patches/node-pty@1.1.0.patch` +(`usesCygwinRuntime`, added in #19068). This page records the measurement +behind it, because the failure mode it prevents is indistinguishable from a +stale native addon and the gates of the day could not tell the two apart. + +## The mechanism + +The MSYS/Cygwin runtime asks for `CREATE_BREAKAWAY_FROM_JOB` on the +`CreateProcessW` inside its `spawn`/`exec` path. A job that carries +`JOB_OBJECT_LIMIT_BREAKAWAY_OK` grants it, so the child is created outside the +job; a job without that limit denies it with `ERROR_ACCESS_DENIED`, and the +runtime retries without the flag rather than failing the spawn. `fork` is not +affected — forked Cygwin processes stay in the job either way. + +Measured on Windows 11 `10.0.26200.9168`, Git `2.55.0.windows.3`, +bash `5.3.15(1)-release`, node `v24.18.0`, `useConptyDll: true`, for +`node-pty.spawn('C:\Program Files\Git\bin\bash.exe', ['--noprofile','--norc','-i'])` +— `+J` / `-J` is membership of the per-PTY job, read with +`QueryInformationJobObject(JobObjectBasicProcessIdList)`: + +``` +bin\bash.exe +J ConPTY shell (assigned by node-pty) + └ ..\usr\bin\bash.exe +J launcher hand-off, plain CreateProcess + └ usr\bin\bash.exe +J Cygwin fork for the typed command + └ node.exe -J Cygwin exec -- ESCAPES HERE +``` + +`bin\bash.exe` is a 47 KB launcher, not an MSYS binary: `C:\Program Files\Git\bin` +holds only `bash.exe`, `git.exe` and `sh.exe`, with no `msys-2.0.dll`. Its +hand-off to `bin\..\usr\bin\bash.exe` is an ordinary `CreateProcess` and keeps +job membership. Only the MSYS runtime's own spawn breaks away. + +The shell-replacement shape (`bash -c 'exec "$BASH" --noprofile --norc -i'`) +loses membership one step earlier, at the `exec`, and everything below inherits +the loss: + +``` +bin\bash.exe +J + └ ..\usr\bin\bash.exe +J + └ usr\bin\bash.exe -J Cygwin exec -- ESCAPES HERE + └ usr\bin\bash -J + └ node.exe -J +``` + +Both shapes leak. The `exec` is not the cause; it only moves the escape earlier. + +## The A/B that pins it + +One source tree, one toolchain, one variable — `usesCygwinRuntime` forced to +`false` so the per-PTY job keeps `JOB_OBJECT_LIMIT_BREAKAWAY_OK`: + +| per-PTY job limit | `listPtyJobProcessIds` | child reaped by `terminatePtyJob` | runs | +| ---------------------- | ---------------------- | --------------------------------- | ---- | +| `BREAKAWAY_OK` set | 2 pids, child absent | no | 0/2 | +| `BREAKAWAY_OK` cleared | 5 pids, child present | yes | 4/4 | + +The job **is** the right boundary. With breakaway denied it holds the whole MSYS +tree, including the child that detached from the console, and one +`terminateJob` reaps all of it. No alternative tracking mechanism is needed. + +Denying breakaway did not break ordinary launches from the pane: `git`, +`cmd //c`, an absolute-path `node`, a `&`-backgrounded job with `disown`, and +`where.exe` all returned 0 with no `Access is denied`, identically to the +breakaway-allowed control. Untested: a **non-Cygwin** program that itself passes +`CREATE_BREAKAWAY_FROM_JOB` (installers, updaters) and therefore has no runtime +to retry for it. That needs a helper that calls `CreateProcess` with the flag; +`start /b` does not exercise it (it uses `CREATE_NEW_CONSOLE`). + +## A stale addon looks exactly like the bug + +`config/scripts/node-pty-job-ownership.cjs` used to assert only that +`terminateJob`, `listJobProcessIds` and `assignCurrentProcessToJob` are +exported. All three predate #19068, so a `conpty.node` built before it passed +every gate: `isPtyJobOwnershipAvailable()` returned true and +`windows-pty-job.win32.test.ts` passed 6/6, while +`windows-msys-job.win32.test.ts` failed with a two-pid job list that read as a +source defect rather than a build-freshness one. + +When that test fails, check the binary before the code: + +```js +// UTF-16LE, because usesCygwinRuntime holds the literals +readFileSync(conptyNodePath).includes(Buffer.from('msys-2.0.dll', 'utf16le')) +``` + +False means the addon predates the fix; rebuild node-pty from patched source. +Note that a git worktree sharing `node_modules` with its main checkout shares +that checkout's `build/Release/conpty.node`, so pinning the _source_ to a commit +does not pin the _addon_. + +The gate asserts that marker, the way `stagedRelayAddonIsUnpatched()` in +`src/main/windows/windows-process-table.ts` already sniffs a patched addon by a +binary import name. Symbol presence cannot distinguish patch revisions; a marker +can. + +Because the marker is a literal in `conpty.cc` and the gate's copy of it is a +separate constant, `ensure-native-runtime-job-ownership.test.mjs` asserts the +patch still adds `L"msys-2.0.dll"` to that file. Without that, editing the patch +would turn the gate into a permanent false positive that fails every correctly +rebuilt addon and tells the developer to do the one thing that cannot help. + +## Every path the loader can fall through to + +`loadNativeModule` tries `build/Release`, then `build/Debug`, then +`prebuilds/win32-`, each relative to node-pty's root and then to `lib/`, +swallowing every failure in between. A require of a wrong-architecture `.node` +is one of those failures, so the candidate that runs is the first one the target +arch can actually load. The published prebuild is always the last candidate and +never carries the patch: + +| package | `build/Release` | prebuild pruned? | what the app loads | +| -------------------- | ----------------------------- | ---------------- | ------------------ | +| same host, same arch | patched | yes | `build/Release` | +| cross host | absent, cannot be cross-built | no | the prebuild | +| cross arch, built | patched, target arch | no | `build/Release` | +| cross arch, failed | the host's arch | no | the prebuild | + +`beforeBuild` runs `rebuild-native-deps.mjs --platform=win32 --arch=`, so +a cross-arch slice normally does get a patched `build/Release` for the target — +row three is a correct package whose leftover prebuild is never reached. +`prunePackagedNodePty` keeps that prebuild anyway, because its guard is +`electronArch === process.arch` rather than the arch of the binary. + +So presence alone cannot separate row three from row four, and failing on any +unmarked file present would reject a correct package with advice its builder +could not act on. `verifyPackagedConptyBreakawayMarker` instead resolves the +addon the way the loader does — first candidate whose PE `IMAGE_FILE_HEADER` +machine matches the target — and checks the marker on that one. A package with +no candidate at all, or none of the target's architecture, is refused: it has no +ConPTY backend to load.