From 2d500278b4ce326cb54407845e0d02b06aee433e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:48:47 -0700 Subject: [PATCH] build(windows): refuse unpatched node-pty prebuilds Merged after clean CI, Windows packaging verification, and readiness review. --- .github/workflows/pr.yml | 1 + config/electron-builder.config.cjs | 10 +++ config/patches/node-pty@1.1.0.patch | 40 ++++++++++- ...sure-native-runtime-job-ownership.test.mjs | 67 +++++++++++++++++++ config/scripts/ensure-native-runtime.mjs | 2 + config/scripts/ensure-native-runtime.test.mjs | 7 ++ config/scripts/node-pty-job-ownership.cjs | 29 ++++++++ config/scripts/rebuild-native-deps.mjs | 7 +- config/scripts/rebuild-native-deps.test.mjs | 64 +++++++++++++++++- ...verify-packaged-node-pty-job-ownership.cjs | 25 +++++++ ...y-packaged-node-pty-job-ownership.test.mjs | 51 ++++++++++++++ pnpm-lock.yaml | 6 +- 12 files changed, 302 insertions(+), 7 deletions(-) create mode 100644 config/scripts/ensure-native-runtime-job-ownership.test.mjs create mode 100644 config/scripts/node-pty-job-ownership.cjs create mode 100644 config/scripts/verify-packaged-node-pty-job-ownership.cjs create mode 100644 config/scripts/verify-packaged-node-pty-job-ownership.test.mjs diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 22fb956a1a5..06e30b1d39c 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -642,6 +642,7 @@ jobs: - name: Test Windows-specific boundaries run: >- pnpm exec vitest run --config config/vitest.config.ts + config/scripts/rebuild-native-deps.test.mjs src/main/providers/windows-conpty-wide-char-duplication.node-pty.test.ts src/main/providers/pty-repaint-wide-char-buffer.node-pty.test.ts src/shared/child-process/windows-command-line.win32.test.ts diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index 33e31aa2605..bf7d5b1382c 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -14,6 +14,9 @@ const { const { verifyLinuxGlibcFloor } = require('./scripts/verify-linux-glibc-floor.cjs') const { writeMacBuildCompatibility } = require('./scripts/mac-build-compatibility.cjs') const { verifyPackagedPluginResources } = require('./scripts/verify-packaged-plugin-resources.cjs') +const { + verifyPackagedNodePtyJobOwnership +} = require('./scripts/verify-packaged-node-pty-job-ownership.cjs') const { verifySkillsCliRuntime } = require('./scripts/verify-skills-cli-runtime.cjs') // Why: dev-channel builds must carry the *release* identity — same bundle id, @@ -266,6 +269,13 @@ module.exports = { const archEnumByNodeArch = { ia32: 0, x64: 1, armv7l: 2, arm64: 3 } 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') + } + } verifySkillsCliRuntime(join(resourcesDir, 'app.asar.unpacked', 'out'), resourcesDir, { executeCommands: canExecuteTargetArch }) diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 3daad4fde68..c5c35191f35 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -2,7 +2,12 @@ diff --git a/binding.gyp b/binding.gyp index 5f63978b07ab50aaf7523219a2170ec737a6b5db..bbd9e06136e8922f40b5779e35d4fc835f1479ab 100644 --- a/binding.gyp +++ b/binding.gyp -@@ -5,9 +5,6 @@ +@@ -1,13 +1,10 @@ + { + 'target_defaults': { + 'dependencies': [ +- " [], + terminateJob: () => true, + assignCurrentProcessToJob: () => true + } +} +const PREBUILD = { + dir: 'prebuilds/win32-x64/', + module: { + startProcess: () => {}, + connect: () => {}, + resize: () => {}, + clear: () => {}, + kill: () => {} + } +} + +describe('assertNodePtyJobOwnership', () => { + it('keeps node-addon-api project paths absolute during Windows source builds', () => { + expect(NODE_PTY_PATCH).toContain( + `+ " { + expect(NODE_PTY_PATCH).toContain("- 'target_name': 'conpty_console_list'") + expect(NODE_PTY_PATCH).toContain("- 'target_name': 'pty'") + }) + + it('rejects the prebuild that shipped without the job exports', () => { + expect(() => + assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PREBUILD }) + ).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/) + }) + + it('accepts a source build carrying the patch', () => { + expect(() => + assertNodePtyJobOwnership({ platform: 'win32', nativeName: 'conpty', native: PATCHED }) + ).not.toThrow() + }) + + it.each([ + ['non-Windows hosts', { platform: 'darwin', nativeName: 'pty' }], + ['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() + }) +}) diff --git a/config/scripts/ensure-native-runtime.mjs b/config/scripts/ensure-native-runtime.mjs index 941049472fb..b570ac0e7ae 100644 --- a/config/scripts/ensure-native-runtime.mjs +++ b/config/scripts/ensure-native-runtime.mjs @@ -7,6 +7,7 @@ import { release } from 'node:os' import { basename, resolve } from 'node:path' const require = createRequire(import.meta.url) +const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs') const scriptPath = import.meta.filename const projectDir = resolve(import.meta.dirname, '../..') const runtime = readRuntimeArg() @@ -277,6 +278,7 @@ function loadNodePtyNativeModule() { // terminal is created, so require('node-pty') alone can miss ABI mismatches. const native = loadNativeModule(nativeName) assertNodePtyWindowsConptyRuntime(native?.dir) + assertNodePtyJobOwnership({ nativeName, native }) 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/ensure-native-runtime.test.mjs b/config/scripts/ensure-native-runtime.test.mjs index 0bc8e5ae077..d1ea104b863 100644 --- a/config/scripts/ensure-native-runtime.test.mjs +++ b/config/scripts/ensure-native-runtime.test.mjs @@ -14,6 +14,9 @@ import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' const sourceScriptPath = fileURLToPath(new URL('./ensure-native-runtime.mjs', import.meta.url)) +const sourceNodePtyJobOwnershipPath = fileURLToPath( + new URL('./node-pty-job-ownership.cjs', import.meta.url) +) describe('ensure-native-runtime', () => { it('rechecks Node native modules in fresh child processes after rebuilding', () => { @@ -156,6 +159,10 @@ describe('ensure-native-runtime', () => { function mkTempProject() { const projectDir = mkdtempSync(join(tmpdir(), 'orca-native-runtime-')) mkdirSync(join(projectDir, 'config', 'scripts'), { recursive: true }) + copyFileSync( + sourceNodePtyJobOwnershipPath, + join(projectDir, 'config', 'scripts', 'node-pty-job-ownership.cjs') + ) return projectDir } diff --git a/config/scripts/node-pty-job-ownership.cjs b/config/scripts/node-pty-job-ownership.cjs new file mode 100644 index 00000000000..35884c1414a --- /dev/null +++ b/config/scripts/node-pty-job-ownership.cjs @@ -0,0 +1,29 @@ +'use strict' + +const NODE_PTY_JOB_EXPORTS = [ + 'listJobProcessIds', + 'terminateJob', + 'assignCurrentProcessToJob' +] + +function assertNodePtyJobOwnership({ nativeName, native, 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) { + return + } + 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(' ') + ) +} + +module.exports = { assertNodePtyJobOwnership } diff --git a/config/scripts/rebuild-native-deps.mjs b/config/scripts/rebuild-native-deps.mjs index 6d776abd1ca..78e25d3634c 100644 --- a/config/scripts/rebuild-native-deps.mjs +++ b/config/scripts/rebuild-native-deps.mjs @@ -475,9 +475,14 @@ function loadNativeModule(moduleName) { } if (moduleName === 'node-pty') { projectRequire('node-pty') + const { assertNodePtyJobOwnership } = projectRequire( + './config/scripts/node-pty-job-ownership.cjs' + ) const { loadNativeModule } = projectRequire('node-pty/lib/utils') - const native = loadNativeModule(getNodePtyNativeModuleName()) + const nativeName = getNodePtyNativeModuleName() + const native = loadNativeModule(nativeName) assertNodePtyWindowsConptyRuntime(native.dir) + assertNodePtyJobOwnership({ nativeName, native }) if (requirePatchedNodePtySourceBuild && !isNodePtyReleaseBuildDir(native.dir)) { throw new Error( 'node-pty resolved to ' + diff --git a/config/scripts/rebuild-native-deps.test.mjs b/config/scripts/rebuild-native-deps.test.mjs index 0ab28de7a65..b158ee57f21 100644 --- a/config/scripts/rebuild-native-deps.test.mjs +++ b/config/scripts/rebuild-native-deps.test.mjs @@ -18,6 +18,9 @@ const sourceScriptPath = fileURLToPath(new URL('./rebuild-native-deps.mjs', impo const sourceInstallScriptPath = fileURLToPath( new URL('./install-electron-package-binary.mjs', import.meta.url) ) +const sourceNodePtyJobOwnershipPath = fileURLToPath( + new URL('./node-pty-job-ownership.cjs', import.meta.url) +) describe('rebuild-native-deps Electron install fallback', () => { it('continues non-strict postinstall when Electron retry download fails', () => { @@ -135,6 +138,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => { writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) writeFakeLoadableNodePty(projectDir, { nativeDir: '../build/Release/' }) writeFakeWindowsRegistry(projectDir) + writeFakeWindowsProcessTree(projectDir) writeFakeNodePtyConptyPayload(projectDir, process.arch) const result = runRebuildScript(projectDir, { @@ -189,6 +193,7 @@ describe('rebuild-native-deps patched node-pty rebuild', () => { writeFakeUsableElectronPackage(projectDir, { platform: 'win32' }) writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) writeFakeLoadableNodePty(projectDir) + writeFakeWindowsProcessTree(projectDir) writeFakeNodePtyConptyPayload(projectDir, process.arch) const result = runRebuildScript(projectDir, { @@ -207,6 +212,36 @@ describe('rebuild-native-deps patched node-pty rebuild', () => { } ) + it.skipIf(process.platform !== 'win32')( + 'rebuilds a loadable ConPTY native that lacks Orca job ownership', + () => { + const projectDir = mkTempProject() + + try { + const rebuildLogPath = join(projectDir, 'electron-rebuild.log') + writeFakeUsableElectronPackage(projectDir, { platform: 'win32' }) + writeFakeElectronRebuild(projectDir, { logPathEnv: 'ORCA_REBUILD_TEST_LOG' }) + writeFakeLoadableNodePty(projectDir, { ownsPtyJob: false }) + writeFakeWindowsRegistry(projectDir) + writeFakeWindowsProcessTree(projectDir) + + const result = runRebuildScript(projectDir, { + ORCA_REBUILD_TEST_LOG: rebuildLogPath, + npm_config_platform: 'win32', + npm_config_arch: process.arch + }) + + expect(result.status, result.stderr).toBe(0) + expect(result.stdout).toContain('Rebuilding failed native modules: node-pty') + expect(result.stdout).toContain('missing listJobProcessIds') + const rebuildCall = JSON.parse(readFileSync(rebuildLogPath, 'utf8').trim()) + expect(rebuildCall.onlyModules).toEqual(['node-pty']) + } finally { + rmSync(projectDir, { recursive: true, force: true }) + } + } + ) + it.skipIf(process.platform === 'win32')( 'rebuilds when Electron can load node-pty but patched build artifacts are missing', () => { @@ -305,6 +340,10 @@ function mkTempProject() { sourceInstallScriptPath, join(projectDir, 'config', 'scripts', 'install-electron-package-binary.mjs') ) + copyFileSync( + sourceNodePtyJobOwnershipPath, + join(projectDir, 'config', 'scripts', 'node-pty-job-ownership.cjs') + ) return projectDir } @@ -495,7 +534,10 @@ function writeFakeNodePtyConptyPayload(projectDir, arch) { writeFileSync(join(sourceDir, 'OpenConsole.exe'), `OpenConsole.exe ${arch}`) } -function writeFakeLoadableNodePty(projectDir, { nativeDir = 'prebuilds/pty' } = {}) { +function writeFakeLoadableNodePty( + projectDir, + { nativeDir = 'prebuilds/pty', ownsPtyJob = true } = {} +) { const nodePtyDir = join(projectDir, 'node_modules', 'node-pty') mkdirSync(join(nodePtyDir, 'lib'), { recursive: true }) writeFileSync(join(nodePtyDir, 'index.js'), 'module.exports = {}\n') @@ -503,7 +545,19 @@ function writeFakeLoadableNodePty(projectDir, { nativeDir = 'prebuilds/pty' } = join(nodePtyDir, 'lib', 'utils.js'), ` exports.loadNativeModule = function loadNativeModule(nativeName) { - return { dir: ${JSON.stringify(nativeDir)}, module: { nativeName } } + return { + dir: ${JSON.stringify(nativeDir)}, + module: { + nativeName, + ...(${JSON.stringify(ownsPtyJob)} + ? { + listJobProcessIds() {}, + terminateJob() {}, + assignCurrentProcessToJob() {} + } + : {}) + } + } } ` ) @@ -518,6 +572,12 @@ function writeFakeWindowsRegistry(projectDir) { ) } +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') +} + function writeNodePtyPatchFile(projectDir) { mkdirSync(join(projectDir, 'config', 'patches'), { recursive: true }) writeFileSync(join(projectDir, 'config', 'patches', 'node-pty@1.1.0.patch'), 'patch marker\n') diff --git a/config/scripts/verify-packaged-node-pty-job-ownership.cjs b/config/scripts/verify-packaged-node-pty-job-ownership.cjs new file mode 100644 index 00000000000..5ba21cee8ec --- /dev/null +++ b/config/scripts/verify-packaged-node-pty-job-ownership.cjs @@ -0,0 +1,25 @@ +const { createRequire } = require('node:module') +const { join } = require('node:path') +const { assertNodePtyJobOwnership } = require('./node-pty-job-ownership.cjs') + +function loadPackagedConpty(resourcesDir) { + const packagedRequire = createRequire(join(resourcesDir, 'package.json')) + const { loadNativeModule } = packagedRequire('./node_modules/node-pty/lib/utils') + return loadNativeModule('conpty') +} + +function verifyPackagedNodePtyJobOwnership(resourcesDir, options = {}) { + const platform = options.platform ?? process.platform + if (platform !== 'win32') { + return + } + + const native = (options.loadNative ?? loadPackagedConpty)(resourcesDir) + assertNodePtyJobOwnership({ platform, nativeName: 'conpty', native }) + 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 } diff --git a/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs b/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs new file mode 100644 index 00000000000..26a33cfbbd6 --- /dev/null +++ b/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs @@ -0,0 +1,51 @@ +import { createRequire } from 'node:module' +import { describe, expect, it, vi } from 'vitest' + +const require = createRequire(import.meta.url) +const { + verifyPackagedNodePtyJobOwnership +} = require('./verify-packaged-node-pty-job-ownership.cjs') + +const PATCHED = { + dir: '../build/Release/', + module: { + listJobProcessIds: () => [], + terminateJob: () => true, + assignCurrentProcessToJob: () => true + } +} + +describe('verifyPackagedNodePtyJobOwnership', () => { + it('accepts the packaged patched ConPTY binding', () => { + expect(() => + verifyPackagedNodePtyJobOwnership('resources', { + platform: 'win32', + loadNative: () => PATCHED + }) + ).not.toThrow() + }) + + it('rejects a packaged upstream prebuild', () => { + expect(() => + verifyPackagedNodePtyJobOwnership('resources', { + platform: 'win32', + loadNative: () => ({ dir: '../prebuilds/win32-x64/', module: {} }) + }) + ).toThrow(/missing listJobProcessIds, terminateJob, assignCurrentProcessToJob/) + }) + + it('requires the patched source-build directory', () => { + expect(() => + verifyPackagedNodePtyJobOwnership('resources', { + platform: 'win32', + loadNative: () => ({ ...PATCHED, dir: '../prebuilds/win32-x64/' }) + }) + ).toThrow(/expected patched build\/Release/) + }) + + it('does not load Windows natives for other targets', () => { + const loadNative = vi.fn() + verifyPackagedNodePtyJobOwnership('resources', { platform: 'linux', loadNative }) + expect(loadNative).not.toHaveBeenCalled() + }) +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1ee3caf6950..6d739a42d25 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -27,7 +27,7 @@ patchedDependencies: hash: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 path: config/patches/lint-staged@16.4.0.patch node-pty@1.1.0: - hash: 710d5c74d1b738c41316e997e7cb860b61147604f98380ccf4f5bb20b6ed0ac5 + hash: 9c665d9e41571341cb8986607d531b953a7f32c0dd60e1e74e633c098d80f2c1 path: config/patches/node-pty@1.1.0.patch importers: @@ -69,7 +69,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=710d5c74d1b738c41316e997e7cb860b61147604f98380ccf4f5bb20b6ed0ac5) + version: 1.1.0(patch_hash=9c665d9e41571341cb8986607d531b953a7f32c0dd60e1e74e633c098d80f2c1) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12046,7 +12046,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=710d5c74d1b738c41316e997e7cb860b61147604f98380ccf4f5bb20b6ed0ac5): + node-pty@1.1.0(patch_hash=9c665d9e41571341cb8986607d531b953a7f32c0dd60e1e74e633c098d80f2c1): dependencies: node-addon-api: 7.1.1