refactor: harden exec failure wrapping end to end

src/__tests__/exec-wrap-guard.test.ts

@@ -0,0 +1,107 @@
+import assert from 'node:assert/strict';
+import fs from 'node:fs';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
+import { test } from 'vitest';
+
+// ---------------------------------------------------------------------------
+// Source guard: hand-rolled exec failure wraps must not rebuild the
+// stdout/stderr/exitCode details trio inline.
+//
+// normalizeError only surfaces the stderr excerpt for COMMAND_FAILED errors
+// whose details carry `processExitError: true`. Before PR #1072 ~49 call sites
+// spread the trio by hand and silently missed the flag, so users saw
+// "<tool> exited with code N" with no cause. The fix is centralized in
+// `execFailureDetails` / `requireExecSuccess` (src/utils/exec.ts); this guard
+// keeps new call sites from drifting back to inline spreads.
+//
+// A site that intentionally rebuilds the trio WITHOUT the flag (reachable at
+// exit 0, timeout errors where the message beats the excerpt, nested tool
+// payloads) opts out with a `// exec-guard-allow: <reason>` comment directly
+// above the `new AppError(` — the reason doubles as documentation.
+// ---------------------------------------------------------------------------
+
+const SRC_ROOT = path.join(path.dirname(fileURLToPath(import.meta.url)), '..');
+const ALLOW_MARKER = 'exec-guard-allow:';
+// Longhand `stdout: x.stdout` and shorthand `stdout,` property forms alike.
+const TRIO_KEYS = [/\bstdout\s*[:,}]/, /\bstderr\s*[:,}]/, /\bexitCode\s*[:,}]/] as const;
+const MAX_CALL_WINDOW = 1_600;
+
+function listSourceFiles(dir: string, files: string[] = []): string[] {
+  for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
+    if (entry.name === '__tests__' || entry.name === 'node_modules') continue;
+    const fullPath = path.join(dir, entry.name);
+    if (entry.isDirectory()) {
+      listSourceFiles(fullPath, files);
+      continue;
+    }
+    if (!entry.name.endsWith('.ts')) continue;
+    if (entry.name.endsWith('.test.ts') || entry.name.endsWith('.d.ts')) continue;
+    files.push(fullPath);
+  }
+  return files;
+}
+
+// The argument window of a `new AppError(...)` call: from its opening paren to
+// the balancing close, capped defensively in case a string literal unbalances
+// the parens.
+function appErrorCallWindow(source: string, callStart: number): string {
+  const openParen = source.indexOf('(', callStart);
+  if (openParen === -1) return '';
+  let depth = 0;
+  const end = Math.min(source.length, openParen + MAX_CALL_WINDOW);
+  for (let i = openParen; i < end; i += 1) {
+    const char = source[i];
+    if (char === '(') depth += 1;
+    if (char === ')') {
+      depth -= 1;
+      if (depth === 0) return source.slice(openParen, i + 1);
+    }
+  }
+  return source.slice(openParen, end);
+}
+
+function hasAllowMarker(source: string, callStart: number): boolean {
+  const precedingLines = source.slice(Math.max(0, callStart - 300), callStart);
+  return precedingLines.includes(ALLOW_MARKER);
+}
+
+function isInlineTrioViolation(source: string, callStart: number): boolean {
+  const window = appErrorCallWindow(source, callStart);
+  // Stderr enrichment only fires for COMMAND_FAILED; the trio is plain
+  // diagnostic context under other codes (and under dynamic code
+  // expressions, which are runner/daemon pass-throughs, not exec wraps).
+  if (!/^\(\s*'COMMAND_FAILED'/.test(window)) return false;
+  if (!TRIO_KEYS.every((key) => key.test(window))) return false;
+  if (window.includes('execFailureDetails') || window.includes('requireExecSuccess')) return false;
+  return !hasAllowMarker(source, callStart);
+}
+
+function collectFileViolations(filePath: string): string[] {
+  const source = fs.readFileSync(filePath, 'utf8');
+  const violations: string[] = [];
+  let searchFrom = 0;
+  for (;;) {
+    const callStart = source.indexOf('new AppError(', searchFrom);
+    if (callStart === -1) return violations;
+    searchFrom = callStart + 1;
+    if (!isInlineTrioViolation(source, callStart)) continue;
+    const line = source.slice(0, callStart).split('\n').length;
+    violations.push(`${path.relative(SRC_ROOT, filePath)}:${line}`);
+  }
+}
+
+test('AppError details never rebuild the exec stdout/stderr/exitCode trio inline', () => {
+  const violations = listSourceFiles(SRC_ROOT).flatMap(collectFileViolations);
+
+  assert.deepEqual(
+    violations,
+    [],
+    `Inline exec stdout/stderr/exitCode spreads in AppError details miss the processExitError flag, ` +
+      `so normalizeError cannot surface the stderr excerpt. Build the details with ` +
+      `execFailureDetails()/requireExecSuccess() from src/utils/exec.ts, or — when the throw is ` +
+      `reachable at exit 0 or the excerpt would degrade the message — opt out with a ` +
+      `"// ${ALLOW_MARKER} <reason>" comment above the new AppError(...) call. Violations:\n` +
+      violations.join('\n'),
+  );
+});

src/daemon/device-ready.ts

@@ -9,6 +9,7 @@ import {
 } from '../platforms/apple/core/devicectl.ts';
 import { runXcrun } from '../platforms/apple/core/tool-provider.ts';
 import { isActiveProviderDevice } from '../provider-device-runtime.ts';
+import { execFailureDetails } from '../utils/exec.ts';
 import { createTtlMemo } from '../utils/ttl-memo.ts';
 
 const IOS_DEVICE_READY_TIMEOUT_MS = 15_000;
@@ -102,11 +103,12 @@ async function ensureIosDeviceReady(deviceId: string): Promise<void> {
         timeoutMs: IOS_DEVICE_READY_TIMEOUT_MS + IOS_DEVICE_READY_COMMAND_TIMEOUT_BUFFER_MS,
       },
     );
-    const stdout = String(result.stdout ?? '');
-    const stderr = String(result.stderr ?? '');
+    const { stdout, stderr } = result;
     const parsed = await readIosReadyPayload(jsonPath);
     if (result.exitCode === 0) {
       if (!parsed.parsed) {
+        // exec-guard-allow: thrown at exit 0 (probe succeeded but the readiness
+        // JSON was missing/invalid) — not a process-exit wrap.
         throw new AppError('COMMAND_FAILED', 'iOS device readiness probe failed', {
           kind: 'probe_inconclusive',
           deviceId,
@@ -126,15 +128,16 @@ async function ensureIosDeviceReady(deviceId: string): Promise<void> {
       }
       return;
     }
-    throw new AppError('COMMAND_FAILED', 'iOS device is not ready for automation', {
-      kind: 'not_ready',
-      deviceId,
-      stdout,
-      stderr,
-      exitCode: result.exitCode,
-      tunnelState: parsed?.tunnelState,
-      hint: resolveIosReadyHint(stdout, stderr),
-    });
+    throw new AppError(
+      'COMMAND_FAILED',
+      'iOS device is not ready for automation',
+      execFailureDetails(result, {
+        kind: 'not_ready',
+        deviceId,
+        tunnelState: parsed?.tunnelState,
+        hint: resolveIosReadyHint(stdout, stderr),
+      }),
+    );
   } catch (error) {
     if (error instanceof AppError && error.code === 'COMMAND_FAILED') {
       const kind = typeof error.details?.kind === 'string' ? error.details.kind : '';

src/daemon/handlers/__tests__/session-appstate-input-perf.test.ts

@@ -441,7 +441,8 @@ test('perf preserves successful metrics and normalizes per-metric Android sampli
     const cpu = (response.data?.metrics as any)?.cpu;
     expect(startup?.available).toBe(false);
     expect(memory?.available).toBe(false);
-    expect(memory?.reason).toBe('error: device offline');
+    // The bare `error:` severity prefix is stripped by the stderr-excerpt noise filter.
+    expect(memory?.reason).toBe('device offline');
     expect(memory?.error?.code).toBe('COMMAND_FAILED');
     expect(memory?.error?.hint).toMatch(/adb reconnect/i);
     expect(memory?.error?.details?.metric).toBe('memory');

src/daemon/runtime-hints.ts

@@ -1,5 +1,6 @@
 import { isIosFamily, type DeviceInfo } from '../kernel/device.ts';
 import { AppError, asAppError } from '../kernel/errors.ts';
+import { execFailureDetails } from '../utils/exec.ts';
 import type { SessionRuntimeHints } from './types.ts';
 import {
   resolveRuntimeTransportHints,
@@ -125,15 +126,12 @@ async function writeAndroidDevPrefs(
       runAsDenied
         ? `Failed to access Android app sandbox for ${packageName}`
         : `Failed to probe Android app sandbox for ${packageName}`,
-      {
+      execFailureDetails(probeResult, {
         package: packageName,
         cmd: 'adb',
         args: probeArgs,
-        stdout: probeResult.stdout,
-        stderr: probeResult.stderr,
-        exitCode: probeResult.exitCode,
         hint: runAsDenied ? ANDROID_RUN_AS_HINT : ANDROID_PROBE_HINT,
-      },
+      }),
     );
   }
 

src/daemon/target-shutdown.ts

@@ -71,7 +71,7 @@ async function shutdownAndroidEmulator(device: DeviceInfo): Promise<DeviceTarget
   return {
     success: result.exitCode === 0,
     exitCode: result.exitCode,
-    stdout: String(result.stdout ?? ''),
-    stderr: String(result.stderr ?? ''),
+    stdout: result.stdout,
+    stderr: result.stderr,
   };
 }

src/kernel/errors.ts

@@ -30,12 +30,30 @@ export function toAppErrorCode(
   return fallback;
 }
 
-type AppErrorDetails = Record<string, unknown> & {
+/**
+ * Details bag for AppError. Free-form context is allowed, but these keys carry
+ * meaning at normalize/render time and must keep their types:
+ * - `hint` — overrides `defaultHintForCode`; re-wraps preserve an existing hint.
+ * - `diagnosticId` / `logPath` — lifted onto the normalized error, stripped from details.
+ * - `processExitError` + `stdout`/`stderr`/`exitCode` — marks a wrap of a real
+ *   process exit so normalizeError can surface the first meaningful stderr line;
+ *   build these via `execFailureDetails`/`requireExecSuccess` in src/utils/exec.ts
+ *   rather than by hand.
+ * - `retriable` — typed retry signal hoisted to the wire error shape.
+ * - `reason` — machine-dispatchable sub-classification within a code.
+ */
+export type AppErrorDetails = Record<string, unknown> & {
   hint?: string;
   diagnosticId?: string;
   logPath?: string;
   retriable?: boolean;
   supportedOn?: string;
+  processExitError?: boolean;
+  stdout?: string;
+  stderr?: string;
+  // null mirrors the raw child_process exit event: killed by signal, no code.
+  exitCode?: number | null;
+  reason?: string;
 };
 
 export type NormalizedError = {
@@ -133,18 +151,26 @@ function maybeEnrichCommandFailedMessage(
   return `${message}: ${excerpt}`;
 }
 
-function firstStderrLine(stderr: string): string | null {
-  const skipPatterns = [
-    /^an error was encountered processing the command/i,
-    /^underlying error\b/i,
-    /^simulator device failed to complete the requested operation/i,
-  ];
+// Boilerplate preamble lines and tool-name/severity prefixes carry nothing the
+// code/message don't already convey. These lists live in the kernel deliberately:
+// normalizeError renders wire-level errors after platform code has run, so
+// platforms cannot contribute patterns — revisit as a registry only if the
+// lists outgrow a handful of entries.
+const STDERR_SKIP_PATTERNS = [
+  /^an error was encountered processing the command/i,
+  /^underlying error\b/i,
+  /^simulator device failed to complete the requested operation/i,
+];
+const STDERR_NOISE_PREFIX = /^(?:(?:adb|xcrun|simctl):\s*)?(?:error:\s*)?/i;
 
+function firstStderrLine(stderr: string): string | null {
   for (const rawLine of stderr.split('\n')) {
     const line = rawLine.trim();
     if (!line) continue;
-    if (skipPatterns.some((pattern) => pattern.test(line))) continue;
-    return line.length > 200 ? `${line.slice(0, 200)}...` : line;
+    if (STDERR_SKIP_PATTERNS.some((pattern) => pattern.test(line))) continue;
+    const excerpt = line.replace(STDERR_NOISE_PREFIX, '').trim();
+    if (!excerpt) continue;
+    return excerpt.length > 200 ? `${excerpt.slice(0, 200)}...` : excerpt;
   }
   return null;
 }

src/platforms/android/__tests__/adb-executor.test.ts

@@ -492,13 +492,11 @@ test('androidAdbResultError composes the classified hint with the stderr excerpt
   });
 
   // execFailureDetails flags processExitError, so normalizeError suffixes the
-  // curated message with the stderr excerpt while the classified hint rides along.
+  // curated message with the stderr excerpt (severity prefix stripped) while
+  // the classified hint rides along.
   assert.equal(error.details?.processExitError, true);
   const normalized = normalizeError(error);
-  assert.equal(
-    normalized.message,
-    'adb uninstall failed for com.example.app: error: device offline',
-  );
+  assert.equal(normalized.message, 'adb uninstall failed for com.example.app: device offline');
   assert.match(String(normalized.hint), /adb reconnect/i);
   assert.equal(normalized.retriable, true);
 });

src/platforms/android/adb-executor.ts

@@ -2,6 +2,7 @@ import { AsyncLocalStorage } from 'node:async_hooks';
 import type { Readable, Writable } from 'node:stream';
 import type { DeviceInfo } from '../../kernel/device.ts';
 import {
+  coerceExecResult,
   execFailureDetails,
   runCmd,
   runCmdBackground,
@@ -382,9 +383,11 @@ function withAdbFailureHintProvider(provider: AndroidAdbProvider): AndroidAdbPro
   if (adbFailureHintProviders.has(provider)) return provider;
   const enriched: AndroidAdbProvider = {
     ...provider,
-    exec: withAdbFailureHints(provider.exec),
-    ...(provider.pull ? { pull: withAdbFailureHints(provider.pull) } : {}),
-    ...(provider.install ? { install: withAdbFailureHints(provider.install) } : {}),
+    exec: withAdbFailureHints(coerceAdbResults(provider.exec)),
+    ...(provider.pull ? { pull: withAdbFailureHints(coerceAdbResults(provider.pull)) } : {}),
+    ...(provider.install
+      ? { install: withAdbFailureHints(coerceAdbResults(provider.install)) }
+      : {}),
     ...(provider.installBundle
       ? { installBundle: withAdbFailureHints(provider.installBundle) }
       : {}),
@@ -393,6 +396,16 @@ function withAdbFailureHintProvider(provider: AndroidAdbProvider): AndroidAdbPro
   return enriched;
 }
 
+// Providers are SDK-supplied callbacks whose results cross an unchecked
+// boundary; coerce them once here (see coerceExecResult) so downstream code
+// can trust the ExecResult types. Wrapped inside the same enrichment pass so
+// the WeakSet memo above also prevents coercer stacking.
+function coerceAdbResults<Args extends unknown[]>(
+  call: (...args: Args) => Promise<AndroidAdbExecutorResult>,
+): (...args: Args) => Promise<AndroidAdbExecutorResult> {
+  return async (...args) => coerceExecResult(await call(...args));
+}
+
 export function createDeviceAdbExecutor(device: DeviceInfo): AndroidAdbExecutor {
   return createSerialAdbExecutor(device.id);
 }
@@ -649,14 +662,9 @@ function createExecAndroidPortReverseProvider(adb: AndroidAdbExecutor): AndroidP
         timeoutMs: options?.timeoutMs,
       });
       if (result.exitCode !== 0 && !isMissingReverseMapping(result.stdout, result.stderr)) {
-        throw attachAdbFailureHint(
-          new AppError('COMMAND_FAILED', `Failed to remove Android port reverse ${local}`, {
-            local,
-            stdout: result.stdout,
-            stderr: result.stderr,
-            exitCode: result.exitCode,
-          }),
-        );
+        throw androidAdbResultError(`Failed to remove Android port reverse ${local}`, result, {
+          local,
+        });
       }
       for (const locals of owned.values()) {
         locals.delete(local);

src/platforms/android/app-control.ts

@@ -92,6 +92,8 @@ export async function openAndroidAppWithAdb(
 
   const component = await resolveAndroidLaunchComponentWithAdb(adb, packageName, [category]);
   if (!component) {
+    // exec-guard-allow: reachable at exit 0 (am start "succeeds" with an error
+    // in its output); the trio is the primary attempt's context, not an exit wrap.
     throw new AppError(
       'COMMAND_FAILED',
       `Failed to resolve Android launch component for ${packageName}`,

src/platforms/android/devices.ts

@@ -1,4 +1,4 @@
-import { execFailureDetails, runCmd, runCmdDetached, whichCmd } from '../../utils/exec.ts';
+import { requireExecSuccess, runCmd, runCmdDetached, whichCmd } from '../../utils/exec.ts';
 import type { ExecResult } from '../../utils/exec.ts';
 import { sleep } from '../../utils/timeouts.ts';
 import { AppError, asAppError } from '../../kernel/errors.ts';
@@ -311,19 +311,16 @@ export function resolveAndroidAvdName(
 }
 
 async function listAndroidAvdNames(): Promise<string[]> {
-  const result = await runCmd('emulator', ['-list-avds'], {
-    allowFailure: true,
-    timeoutMs: ANDROID_BOOT_PROP_TIMEOUT_MS,
-  });
-  if (result.exitCode !== 0) {
-    throw new AppError(
-      'COMMAND_FAILED',
-      'Failed to list Android emulator AVDs',
-      execFailureDetails(result, {
-        hint: 'Verify Android emulator tooling is installed and available in PATH.',
-      }),
-    );
-  }
+  const result = requireExecSuccess(
+    await runCmd('emulator', ['-list-avds'], {
+      allowFailure: true,
+      timeoutMs: ANDROID_BOOT_PROP_TIMEOUT_MS,
+    }),
+    'Failed to list Android emulator AVDs',
+    {
+      hint: 'Verify Android emulator tooling is installed and available in PATH.',
+    },
+  );
   return parseAndroidAvdList(result.stdout);
 }
 
@@ -523,6 +520,8 @@ export async function waitForAndroidBoot(serial: string, timeoutMs = 60000): Pro
         );
         lastBootResult = result;
         if (result.stdout.trim() === '1') return;
+        // exec-guard-allow: getprop exits 0 while the device is still booting;
+        // the throw is a retry-loop poll miss, not a process-exit wrap.
         throw new AppError('COMMAND_FAILED', 'Android device is still booting', {
           serial,
           stdout: result.stdout,