From 614476cbb590a6f89f1859d732f3ca49c083d6a5 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 02:18:54 -0700 Subject: [PATCH] refactor(macos): probe press-and-hold through the shared spawn chokepoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/shared/child-process/child-process-import-boundary.test.ts` forbids a direct `node:child_process` import outside its allowlist, and the allowlist only shrinks — so this module moves to `runProcessSync`, which exists for callers that genuinely cannot await. This one runs before `app.whenReady()`. `runProcessSync` returns a non-zero exit instead of throwing it, so the three-way read decision is re-expressed against `ProcessResult`: exit 0 is an explicit value, exit 1 is a missing key, and a timeout, a signal kill, any other exit, or a child that never started all stay 'unknown'. The throw path is now inside `interpretDefaultsRead` so a spawn failure is reachable from a test rather than hidden in an untested catch, and the write checks the exit code — a refused `defaults write` no longer looks like success. Both boundary-test failures were the same import: with it gone the offender count returns to 155, so no ratchet baseline is bumped. --- ...s-and-hold-default.defaults-domain.test.ts | 62 +++++++++++------- src/main/macos-press-and-hold-default.test.ts | 46 ++++++++++++++ src/main/macos-press-and-hold-default.ts | 63 ++++++++++++------- 3 files changed, 127 insertions(+), 44 deletions(-) diff --git a/src/main/macos-press-and-hold-default.defaults-domain.test.ts b/src/main/macos-press-and-hold-default.defaults-domain.test.ts index 97fbdef57aa..0e33883df34 100644 --- a/src/main/macos-press-and-hold-default.defaults-domain.test.ts +++ b/src/main/macos-press-and-hold-default.defaults-domain.test.ts @@ -1,14 +1,15 @@ import { execFileSync } from 'node:child_process' import { randomUUID } from 'node:crypto' -import { rmSync } from 'node:fs' -import { homedir } from 'node:os' +import { chmodSync, mkdirSync, mkdtempSync, rmSync } from 'node:fs' +import { homedir, tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' +import { runProcessSync } from '../shared/child-process/run-process' import { PRESS_AND_HOLD_KEY, applyMacPressAndHoldPreference, ensureMacPressAndHoldDefault, - interpretDefaultsReadFailure, + interpretDefaultsRead, readDomainPressAndHoldPreference, writeDomainPressAndHoldPreference, type PressAndHoldHost, @@ -30,6 +31,9 @@ import { // would exercise a different branch than production. const domains: string[] = [] +/** Pinned against the real binary below, then reused by the platform-agnostic unit tests. */ +const DEFAULTS_MISSING_EXIT_CODE = 1 + function throwawayDomain(): string { const domain = `com.stablyai.orca.defaults-domain-test.${randomUUID()}` domains.push(domain) @@ -113,27 +117,41 @@ describe.skipIf(process.platform !== 'darwin')( }) it('refuses to call a failed probe "unset"', () => { - // A real spawn failure, not a hand-built object: `execFileSync` reports it with no exit status, - // which is precisely what must not be mistaken for "the key does not exist". - let spawnFailure: unknown - try { - execFileSync('/usr/bin/defaults-does-not-exist', ['read'], { stdio: 'ignore' }) - } catch (error) { - spawnFailure = error - } - expect((spawnFailure as { status?: number | null }).status ?? null).toBe(null) - expect(interpretDefaultsReadFailure(spawnFailure)).toBe('unknown') - - let missingKeyFailure: unknown - try { - execFileSync('/usr/bin/defaults', ['read', throwawayDomain(), PRESS_AND_HOLD_KEY], { - stdio: 'ignore' + // Real runs, not hand-built results: a binary that is not there must come back 'unknown' + // through the same path production takes, not be mistaken for "the key does not exist". + const spawnFailure = (): ReturnType => + runProcessSync({ + program: '/usr/bin/defaults-does-not-exist', + args: ['read'], + stdio: ['ignore', 'ignore', 'ignore'] }) - } catch (error) { - missingKeyFailure = error + expect(spawnFailure).toThrow() + expect(interpretDefaultsRead(spawnFailure)).toBe('unknown') + + const missingKey = runProcessSync({ + program: '/usr/bin/defaults', + args: ['read', throwawayDomain(), PRESS_AND_HOLD_KEY], + stdio: ['ignore', 'ignore', 'ignore'] + }) + // The one exit code the whole design reads as unset; every other outcome is 'unknown'. + expect(missingKey.code).toBe(DEFAULTS_MISSING_EXIT_CODE) + expect(missingKey.timedOut).toBe(false) + expect(interpretDefaultsRead(() => missingKey)).toBe('unset') + }) + + // Skipped as root, where the read-only directory below would still be writable. + it.skipIf(process.getuid?.() === 0)('reports a write the binary refused', () => { + // Why a real refusal: `defaults write` exits non-zero instead of throwing, so a caller that + // only caught exceptions would record 'applied' for a value that never reached the plist. + const readOnly = join(mkdtempSync(join(tmpdir(), 'orca-press-hold-ro-')), 'locked') + mkdirSync(readOnly) + chmodSync(readOnly, 0o500) + try { + expect(writeDomainPressAndHoldPreference(join(readOnly, 'domain'), false)).toBe(false) + } finally { + chmodSync(readOnly, 0o700) + rmSync(readOnly, { recursive: true, force: true }) } - expect((missingKeyFailure as { status?: number }).status).toBe(1) - expect(interpretDefaultsReadFailure(missingKeyFailure)).toBe('unset') }) it('lands the accent-menu setting in the domain, in either direction', () => { diff --git a/src/main/macos-press-and-hold-default.test.ts b/src/main/macos-press-and-hold-default.test.ts index 30a87f56104..fca46f78174 100644 --- a/src/main/macos-press-and-hold-default.test.ts +++ b/src/main/macos-press-and-hold-default.test.ts @@ -2,9 +2,11 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'nod import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' +import type { ProcessResult } from '../shared/child-process/run-process' import { applyMacPressAndHoldPreference, ensureMacPressAndHoldDefault, + interpretDefaultsRead, isOrcaPreferencesDomain, parsePressAndHoldRecord, readBundleIdentifierFromExecutablePath, @@ -295,6 +297,50 @@ describe('applyMacPressAndHoldPreference', () => { }) }) +/** + * Why here and not only in the .defaults-domain file: that file is macOS-only and CI has no macOS + * runner, so this three-way rule would otherwise be enforced nowhere in CI. These cases need no + * `defaults` binary — the sibling file pins that the real binary produces them. + */ +describe('interpretDefaultsRead', () => { + function probe(overrides: Partial): () => ProcessResult { + return () => ({ code: 0, signal: null, stdout: '', stderr: '', timedOut: false, ...overrides }) + } + + it('reads a clean exit as an explicit value', () => { + expect(interpretDefaultsRead(probe({ code: 0 }))).toBe('set') + }) + + it('reads only "does not exist" as unset', () => { + expect(interpretDefaultsRead(probe({ code: 1 }))).toBe('unset') + }) + + describe('refuses to call a failed probe unset', () => { + // Each of these would otherwise overwrite a value the user deliberately set. + const failures: [string, () => ProcessResult][] = [ + ['killed before it exited', probe({ code: null, signal: 'SIGTERM' })], + ['timed out', probe({ code: null, timedOut: true })], + [ + 'timed out after somehow reporting the missing-key code', + probe({ code: 1, timedOut: true }) + ], + ['exited with some other error', probe({ code: 2 })], + ['exited with a negative status', probe({ code: -1 })], + [ + 'never started at all', + () => { + throw Object.assign(new Error('spawn ENOENT'), { code: 'ENOENT' }) + } + ] + ] + for (const [name, failing] of failures) { + it(name, () => { + expect(interpretDefaultsRead(failing)).toBe('unknown') + }) + } + }) +}) + describe('readBundleIdentifierFromExecutablePath', () => { const roots: string[] = [] diff --git a/src/main/macos-press-and-hold-default.ts b/src/main/macos-press-and-hold-default.ts index f037fcb80a7..40c3526790d 100644 --- a/src/main/macos-press-and-hold-default.ts +++ b/src/main/macos-press-and-hold-default.ts @@ -1,6 +1,6 @@ -import { execFileSync } from 'node:child_process' import { mkdirSync, readFileSync } from 'node:fs' import { dirname, join } from 'node:path' +import { runProcessSync, type ProcessResult } from '../shared/child-process/run-process' import { writeFileAtomically } from './codex-accounts/fs-utils' /** @@ -40,8 +40,11 @@ export const PRESS_AND_HOLD_RECORD_VERSION = 1 * and the system default is unset, so it reports `false` on a Mac where press-and-hold is on. * `defaults read ` is domain-scoped and exits 1 when the key is absent. */ const DEFAULTS_BINARY = '/usr/bin/defaults' +/** A hang cutoff, not a latency budget: the probe measures ~6ms, and cutting a slow-but-working + * `defaults` short costs the user the feature for that launch. The shared 30s default is what + * must not apply — this runs before `ready`, so a 30s block is a hung app. */ const DEFAULTS_TIMEOUT_MS = 5_000 -/** Why: `defaults` exits 1 for "does not exist"; other statuses mean the probe itself failed. */ +/** Why: `defaults` exits 1 for "does not exist"; anything else means the probe itself failed. */ const DEFAULTS_MISSING_STATUS = 1 const ORCA_BUNDLE_ID = 'com.stablyai.orca' @@ -114,36 +117,52 @@ export function readBundleIdentifierFromExecutablePath(execPath: string): string /** * Why this distinction carries the whole design: only a real "does not exist" answer may be read as - * unset. A spawn failure or a timeout leaves `status` unset, and calling that unset would let a - * broken probe overwrite a value the user chose. + * unset. A killed or timed-out probe never answered, and calling that unset would let a broken + * probe overwrite a value the user chose. */ -export function interpretDefaultsReadFailure(error: unknown): 'unset' | 'unknown' { - return (error as { status?: number | null }).status === DEFAULTS_MISSING_STATUS - ? 'unset' - : 'unknown' +export function interpretDefaultsRead(probe: () => ProcessResult): 'set' | 'unset' | 'unknown' { + let result: ProcessResult + try { + result = probe() + } catch { + // `runProcessSync` throws only when the child never started, which answers nothing at all. + return 'unknown' + } + // Why before the code: a timeout kills mid-run, so any exit it leaves behind is not an answer. + if (result.timedOut) { + return 'unknown' + } + if (result.code === 0) { + return 'set' + } + return result.code === DEFAULTS_MISSING_STATUS ? 'unset' : 'unknown' } export function readDomainPressAndHoldPreference(domain: string): 'set' | 'unset' | 'unknown' { - try { - execFileSync(DEFAULTS_BINARY, ['read', domain, PRESS_AND_HOLD_KEY], { - encoding: 'utf8', - timeout: DEFAULTS_TIMEOUT_MS, - stdio: ['ignore', 'pipe', 'ignore'] + // Why no captured output: the exit code is the whole answer, and reading only presence is what + // makes an explicit `true` and an explicit `false` both count as the user's. + return interpretDefaultsRead(() => + runProcessSync({ + program: DEFAULTS_BINARY, + args: ['read', domain, PRESS_AND_HOLD_KEY], + timeoutMs: DEFAULTS_TIMEOUT_MS, + stdio: ['ignore', 'ignore', 'ignore'] }) - return 'set' - } catch (error) { - return interpretDefaultsReadFailure(error) - } + ) } export function writeDomainPressAndHoldPreference(domain: string, value: boolean): boolean { try { - execFileSync( - DEFAULTS_BINARY, - ['write', domain, PRESS_AND_HOLD_KEY, '-bool', value ? 'true' : 'false'], - { encoding: 'utf8', timeout: DEFAULTS_TIMEOUT_MS, stdio: ['ignore', 'ignore', 'ignore'] } + // Why the code check: a non-zero `defaults write` returns rather than throwing, so treating a + // completed run as success would record 'applied' for a write that never landed. + return ( + runProcessSync({ + program: DEFAULTS_BINARY, + args: ['write', domain, PRESS_AND_HOLD_KEY, '-bool', value ? 'true' : 'false'], + timeoutMs: DEFAULTS_TIMEOUT_MS, + stdio: ['ignore', 'ignore', 'ignore'] + }).code === 0 ) - return true } catch { return false }