test: refuse foreign-pid signals from unit-test workers (Coverage fork death, #1824)

docs/agents/testing.md

@@ -100,6 +100,23 @@ the runtime seam existed; retiring `dispatchCommand('snapshot')` surfaced them o
 time. Do not add to that set, and when a command migrates (`docs/agents/adr-0019-unit.md`), its
 tests move to the runtime seam in the same PR.
 
+Signals are hermetic too. A vitest worker may signal only itself and its own **direct children**;
+`src/__tests__/hermetic-signal-setup.ts` refuses every other process-table write and fails the
+sending test by name. It covers both ways out of the process, because the runner-disposal family
+uses both: `process.kill` (signal 0, the liveness probe, stays free) and a spawned
+`kill`/`pkill`/`killall`, whose `-P` and `-f` forms reach processes the worker never spawned at all.
+The refused pid is usually one the test made up (`child: { pid: 4242 }`), and on a real host that
+number can belong to anyone — on CI it periodically belonged to a sibling fork, which died mid-file
+with no test attributed ("Worker exited unexpectedly", #1824); a `pkill -f 'xcodebuild.*'` from a
+unit test would find a developer's live runner.
+
+If a test drives a real kill path against a fabricated pid, mock the seam where it already mocks the
+liveness reads: `signalPidsBestEffort` / `signalProcessGroupBestEffort` in `src/utils/host-process.ts`
+for direct writes, the exec or tool-provider seam for a spawned `pkill`. Killing a daemon or Metro
+fixture the test itself spawned is fine — that pid is the worker's own. *Direct* is literal: a
+grandchild started through a shell or `npx` wrapper is not tracked, so signal the direct child, or
+its process group through the negative pid, rather than the grandchild's pid.
+
 Keep tests behavioral. Do not assert shapes or cases TypeScript already proves.
 
 A test added as a regression pin must be shown to fail without the change it pins — vacuity is the

src/__tests__/hermetic-signal-setup.test.ts

@@ -0,0 +1,94 @@
+import assert from 'node:assert/strict';
+import { execFile, spawn } from 'node:child_process';
+import { once } from 'node:events';
+import { promisify } from 'node:util';
+import { afterEach, test } from 'vitest';
+import { drainRefusedWritesForTest } from './hermetic-signal-setup.ts';
+
+// The guard in hermetic-signal-setup.ts is what keeps a unit test from reaching
+// a process it does not own (#1824). These drive its refusal paths deliberately,
+// so each one drains the record the guard would otherwise fail the test with.
+
+const execFileAsync = promisify(execFile);
+
+afterEach(() => {
+  assert.deepEqual(drainRefusedWritesForTest(), [], 'a test left an unasserted refusal behind');
+});
+
+function spawnSleeper() {
+  return spawn(process.execPath, ['-e', 'setTimeout(() => {}, 10_000)'], { stdio: 'ignore' });
+}
+
+test('a live direct child may be signalled, and its process group with it', async () => {
+  const child = spawnSleeper();
+  const pid = child.pid;
+  assert.ok(pid);
+
+  process.kill(pid, 'SIGTERM');
+  await once(child, 'exit');
+
+  assert.deepEqual(drainRefusedWritesForTest(), []);
+});
+
+test('authority ends when a child is reaped, so its recycled pid is refused', async () => {
+  const child = spawnSleeper();
+  const pid = child.pid;
+  assert.ok(pid);
+  child.kill('SIGKILL');
+  await once(child, 'exit');
+
+  // The pid is now free for the kernel to hand to an unrelated process, so the
+  // worker no longer owns it — even though this worker did spawn it once.
+  assert.throws(
+    () => process.kill(pid, 'SIGTERM'),
+    (error: NodeJS.ErrnoException) =>
+      error.code === 'ESRCH' && /did not spawn it/.test(error.message),
+  );
+
+  const [record] = drainRefusedWritesForTest();
+  assert.match(record ?? '', new RegExp(`SIGTERM -> pid ${pid}`));
+});
+
+test('a live process this worker never spawned is refused', () => {
+  // The vitest main process: alive, not ours.
+  assert.throws(
+    () => process.kill(process.ppid, 'SIGTERM'),
+    (error: NodeJS.ErrnoException) => error.code === 'ESRCH',
+  );
+  assert.equal(drainRefusedWritesForTest().length, 1);
+});
+
+test('a liveness probe stays free', () => {
+  assert.equal(process.kill(process.ppid, 0), true);
+  assert.deepEqual(drainRefusedWritesForTest(), []);
+});
+
+test('a promisified execFile cannot smuggle a pkill past the guard', async () => {
+  // promisify() resolves through the custom-promisify symbol rather than calling
+  // the wrapper, so this path needs its own interception.
+  await assert.rejects(
+    execFileAsync('pkill', ['-f', 'xcodebuild.*AgentDeviceRunner']),
+    (error: NodeJS.ErrnoException) =>
+      error.code === 'ENOENT' && /Refusing to spawn/.test(error.message),
+  );
+
+  const [record] = drainRefusedWritesForTest();
+  assert.match(record ?? '', /spawn pkill -f xcodebuild/);
+});
+
+test('a promisified execFile still tracks the child it starts', async () => {
+  const pending = execFileAsync(process.execPath, ['-e', 'setTimeout(() => {}, 10_000)']);
+  const pid = pending.child.pid;
+  assert.ok(pid);
+
+  process.kill(pid, 'SIGKILL');
+  await pending.catch(() => {});
+
+  assert.deepEqual(drainRefusedWritesForTest(), []);
+});
+
+test('a spawned kill binary is refused whichever entry point starts it', () => {
+  assert.throws(() => spawn('pkill', ['-TERM', '-P', '4242']), /Refusing to spawn/);
+  assert.throws(() => execFile('/usr/bin/killall', ['node']), /Refusing to spawn/);
+  assert.equal(drainRefusedWritesForTest().length, 2);
+});

src/__tests__/hermetic-signal-setup.ts

@@ -0,0 +1,170 @@
+import childProcess from 'node:child_process';
+import module from 'node:module';
+import util from 'node:util';
+import { afterAll, afterEach, expect } from 'vitest';
+
+// Unit tests must be hermetic with respect to the host's process table: a
+// worker may signal only itself and its own direct children. Anything else is
+// a pid the test made up (`child: { pid: 4242 }`), and on a real host that
+// number can belong to anyone — on CI it periodically belonged to a sibling
+// vitest fork, which died mid-file with no test attributed
+// ("Worker exited unexpectedly", #1824). Refusing the write here, in every
+// worker, turns that silent fork death into a named failure of the test that
+// sent it, on any host, deterministically.
+//
+// Both ways out of the process are covered, because the runner-disposal family
+// uses both: `process.kill` (including the negative pid that addresses a
+// child's process group) and a spawned `kill`/`pkill`/`killall`, whose `-P`
+// and `-f` forms reach processes this worker never spawned at all.
+//
+// Signal 0 is a liveness probe (`isProcessAlive`), not a signal; it stays free.
+// Tests that drive a real kill path against a fabricated pid mock the seam —
+// `signalPidsBestEffort` / `signalProcessGroupBestEffort` in
+// `src/utils/host-process.ts` for the direct writes, the exec/tool-provider
+// seam for a spawned `pkill` — the same way they already mock the liveness
+// reads.
+
+/**
+ * Direct, *live* children only.
+ *
+ * A grandchild (a server started through a shell or `npx` wrapper) is not
+ * tracked, so signalling its pid is refused even though the test legitimately
+ * owns it; signal the direct child, or its process group via the negative pid,
+ * or mock the seam.
+ *
+ * Authority ends when the child is reaped: a pid is only ever a claim on a slot
+ * in the host's process table, and the kernel hands that slot to someone else
+ * once it is free. So synchronous spawns (`spawnSync`, `execFileSync`) are never
+ * remembered — they have already exited when the call returns — and async
+ * children are dropped on `exit`. Cleanup that signals a child must gate on
+ * `isProcessAlive` (as `stopProcess` in `client-metro.test.ts` already does);
+ * "I spawned this pid once" is not ownership of whatever holds it now, which is
+ * the exact hazard this file exists to close.
+ */
+const ownPids = new Set<number>([process.pid]);
+
+const KILL_BINARIES = new Set(['kill', 'pkill', 'killall']);
+
+const refused: string[] = [];
+
+function record(what: string): void {
+  const state = expect.getState();
+  const where = new Error().stack?.split('\n').slice(3, 7).join('\n') ?? '';
+  refused.push(`${what} (${state.currentTestName ?? 'outside a test'})\n${where}`);
+}
+
+function rememberChild(child: unknown): void {
+  const handle = child as { pid?: unknown; once?: (event: string, cb: () => void) => void } | null;
+  const pid = handle?.pid;
+  if (typeof pid !== 'number') return;
+  ownPids.add(pid);
+  // Both events are attached because a child that is never spawned at all
+  // ('error') emits close without exit.
+  handle?.once?.('exit', () => ownPids.delete(pid));
+  handle?.once?.('close', () => ownPids.delete(pid));
+}
+
+function killBinaryName(command: unknown): string | undefined {
+  // `exec` takes a shell command line; the rest take an executable path.
+  const first = String(command).trim().split(/\s+/)[0] ?? '';
+  const name = first.split('/').pop() ?? '';
+  return KILL_BINARIES.has(name) ? name : undefined;
+}
+
+function refuseSpawnedKill(command: unknown, args: unknown): Error | undefined {
+  const name = killBinaryName(command);
+  if (!name) return undefined;
+  const rendered = [String(command), ...(Array.isArray(args) ? args.map(String) : [])].join(' ');
+  record(`spawn ${rendered}`);
+  // Shaped like the binary being unavailable, which every caller of these
+  // best-effort kills already tolerates.
+  const error = new Error(
+    `Refusing to spawn \`${rendered}\`: a unit test may not signal processes through ${name} (see afterEach failure).`,
+  ) as NodeJS.ErrnoException;
+  error.code = 'ENOENT';
+  return error;
+}
+
+function wrapSpawner<T extends (...args: never[]) => unknown>(real: T, remember: boolean): T {
+  const wrapped = ((...args: Parameters<T>) => {
+    const refusal = refuseSpawnedKill(args[0], args[1]);
+    if (refusal) throw refusal;
+    const child = real(...args);
+    if (remember) rememberChild(child);
+    return child;
+  }) as unknown as T;
+  // `util.promisify(execFile)` does not call the function at all: it resolves
+  // through the custom-promisify symbol, so copying the original across would
+  // hand every promisified caller an unguarded execFile. Wrap that path too.
+  const custom = (real as unknown as Record<symbol, unknown>)[util.promisify.custom] as
+    | ((...args: unknown[]) => unknown)
+    | undefined;
+  if (custom) {
+    const wrappedCustom = (...args: unknown[]) => {
+      const refusal = refuseSpawnedKill(args[0], args[1]);
+      if (refusal) return Promise.reject(refusal);
+      const promise = custom(...args);
+      if (remember) rememberChild((promise as { child?: unknown } | null)?.child);
+      return promise;
+    };
+    Object.defineProperty(wrapped, util.promisify.custom, { value: wrappedCustom });
+  }
+  return wrapped;
+}
+
+for (const name of ['spawn', 'fork', 'exec', 'execFile'] as const) {
+  (childProcess as unknown as Record<string, unknown>)[name] = wrapSpawner(
+    childProcess[name],
+    true,
+  );
+}
+for (const name of ['spawnSync', 'execFileSync', 'execSync'] as const) {
+  (childProcess as unknown as Record<string, unknown>)[name] = wrapSpawner(
+    childProcess[name],
+    false,
+  );
+}
+// Named ESM imports of the builtin (`import { spawn } from 'node:child_process'`)
+// bind to the CJS exports only after they are re-synced.
+module.syncBuiltinESMExports();
+
+const realKill = process.kill.bind(process);
+process.kill = ((pid: number, signal: string | number = 'SIGTERM') => {
+  if (signal === 0 || ownPids.has(Math.abs(pid))) return realKill(pid, signal as NodeJS.Signals);
+  record(`${String(signal)} -> pid ${pid}`);
+  // Behave like a dead pid: the caller's ESRCH handling runs, no signal leaves
+  // this worker, and the record above fails the test in afterEach below.
+  const error = new Error(
+    `Refusing to send ${String(signal)} to pid ${pid}: this vitest worker did not spawn it (see afterEach failure).`,
+  ) as NodeJS.ErrnoException;
+  error.code = 'ESRCH';
+  throw error;
+}) as typeof process.kill;
+
+// Best-effort kill sites swallow the errors above, so the refusal is reported
+// here, attributed to the test (or file) that caused it, and never lost.
+function failOnRefusedSignals(scope: string): void {
+  if (refused.length === 0) return;
+  const count = refused.length;
+  const report = refused.splice(0).join('\n');
+  throw new Error(
+    `${scope} tried to signal ${count} process(es) this vitest worker did not spawn. ` +
+      'A unit test may signal only itself and its own direct children (and their process ' +
+      'groups); mock the seam instead — signalPidsBestEffort / signalProcessGroupBestEffort in ' +
+      'src/utils/host-process.ts, the exec or tool-provider seam for a spawned pkill, or ' +
+      `vi.spyOn(process, 'kill'). If the pid is a descendant this test really owns, signal the ` +
+      `direct child (or its group via the negative pid) rather than the grandchild.\n${report}`,
+  );
+}
+
+afterEach(() => failOnRefusedSignals('This test'));
+afterAll(() => failOnRefusedSignals('This file'));
+
+/**
+ * Takes the pending refusals so a test can assert on them without the hooks
+ * above failing it. Only `hermetic-signal-setup.test.ts`, which drives the
+ * guard's own refusal paths on purpose, should call this.
+ */
+export function drainRefusedWritesForTest(): string[] {
+  return refused.splice(0);
+}

src/__tests__/test-file-size-ratchet.test.ts

@@ -36,8 +36,8 @@ const PINNED_TEST_FILE_LINES: Readonly<Record<string, number>> = Object.freeze({
   'src/__tests__/remote-connection.test.ts': 2973,
   'src/daemon/handlers/__tests__/snapshot-handler.test.ts': 2654,
   'src/commands/interaction/runtime/settle.test.ts': 2361,
-  'src/platforms/apple/core/__tests__/runner-session.test.ts': 2083,
   'src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts': 2031,
+  'src/platforms/apple/core/__tests__/runner-session.test.ts': 2001,
   'src/utils/__tests__/daemon-client.test.ts': 1910,
   'src/utils/__tests__/output.test.ts': 1861,
   'src/platforms/android/__tests__/snapshot.test.ts': 1660,

src/daemon/__tests__/request-router-open.test.ts

@@ -12,6 +12,19 @@ vi.mock('../../utils/host-process.ts', async (importOriginal) => {
   const actual = await importOriginal<typeof import('../../utils/host-process.ts')>();
   return { ...actual, readProcessStartTime: vi.fn(() => 'test-process-start') };
 });
+// Opening a session runs the owned-lease cleanup, which pattern-kills stale
+// xcodebuild runners with a real `pkill -f`. The session id here is fabricated,
+// so on a host with a live Apple runner that write would reach a process this
+// test does not own; stub the tool seam the way the runner tests stub the
+// signal seam (#1824).
+vi.mock('../../platforms/apple/core/tool-provider.ts', async (importOriginal) => {
+  const actual =
+    await importOriginal<typeof import('../../platforms/apple/core/tool-provider.ts')>();
+  return {
+    ...actual,
+    runAppleToolCommand: vi.fn(async () => ({ exitCode: 0, stdout: '', stderr: '' })),
+  };
+});
 
 import { dispatchCommand } from '../../core/dispatch.ts';
 import {

src/platforms/apple/core/__tests__/runner-disposal.test.ts

@@ -14,13 +14,15 @@ const {
   mockRunAppleToolCommand,
   mockRunXcrun,
   mockSignalPidsBestEffort,
+  mockSignalProcessGroupBestEffort,
 } = vi.hoisted(() => ({
   mockCleanupTempFile: vi.fn(),
   mockIsProcessAlive: vi.fn(),
   mockIsProcessGroupAlive: vi.fn(),
   mockRunAppleToolCommand: vi.fn(),
   mockRunXcrun: vi.fn(),
   mockSignalPidsBestEffort: vi.fn(),
+  mockSignalProcessGroupBestEffort: vi.fn(),
 }));
 
 vi.mock('../../../../utils/host-process.ts', async (importOriginal) => {
@@ -30,6 +32,7 @@ vi.mock('../../../../utils/host-process.ts', async (importOriginal) => {
     isProcessAlive: mockIsProcessAlive,
     isProcessGroupAlive: mockIsProcessGroupAlive,
     signalPidsBestEffort: mockSignalPidsBestEffort,
+    signalProcessGroupBestEffort: mockSignalProcessGroupBestEffort,
   };
 });
 
@@ -51,7 +54,6 @@ import { abortRunnerSessionsAndPrepProcesses } from '../runner/runner-disposal.t
 
 beforeEach(() => {
   vi.useFakeTimers();
-  vi.spyOn(process, 'kill').mockImplementation(() => true);
   mockIsProcessAlive.mockReturnValue(true);
   mockIsProcessGroupAlive.mockReturnValue(false);
   mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' });
@@ -144,9 +146,8 @@ function makeRunnerSession(
 }
 
 function runnerSignals(session: RunnerSession): NodeJS.Signals[] {
-  return vi
-    .mocked(process.kill)
-    .mock.calls.filter(([pid]) => pid === -(session.child.pid ?? 0))
+  return mockSignalProcessGroupBestEffort.mock.calls
+    .filter(([pid]) => pid === session.child.pid)
     .map(([, signal]) => signal as NodeJS.Signals);
 }
 

src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts

@@ -18,6 +18,8 @@ const {
   mockRunAppleToolCommand,
   mockRunCmdBackground,
   mockRunXcrun,
+  mockSignalPidsBestEffort,
+  mockSignalProcessGroupBestEffort,
   mockWaitForRunner,
   mockRedirectRelease,
 } = vi.hoisted(() => ({
@@ -32,6 +34,9 @@ const {
   mockRunAppleToolCommand: vi.fn(),
   mockRunCmdBackground: vi.fn(),
   mockRunXcrun: vi.fn(),
+  // Runner child pids here are fabricated (4141..4444); see runner-session.test.ts.
+  mockSignalPidsBestEffort: vi.fn(),
+  mockSignalProcessGroupBestEffort: vi.fn(),
   mockWaitForRunner: vi.fn(),
   mockRedirectRelease: vi.fn(),
 }));
@@ -54,6 +59,8 @@ vi.mock('../../../../utils/host-process.ts', async () => {
     ...actual,
     isProcessAlive: mockIsProcessAlive,
     isProcessGroupAlive: mockIsProcessGroupAlive,
+    signalPidsBestEffort: mockSignalPidsBestEffort,
+    signalProcessGroupBestEffort: mockSignalProcessGroupBestEffort,
   };
 });