refactor(macos): probe press-and-hold through the shared spawn chokepoint

`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.
This commit is contained in:
Neil
2026-08-27 02:18:54 -07:00
parent b8e552e4f7
commit 614476cbb5
3 changed files with 127 additions and 44 deletions
@@ -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<typeof runProcessSync> =>
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', () => {
@@ -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>): () => 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[] = []
+41 -22
View File
@@ -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 <domain> <key>` 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
}