From ccc1b47ca5e9a1c26f02f1b4d2a62275722e4e7d Mon Sep 17 00:00:00 2001 From: Anton Lykhoyda Date: Wed, 20 May 2026 13:11:25 +0200 Subject: [PATCH] fix(device-record): refuse multi-sim ambiguity, thread deviceId through script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #173 (sub-issue 1). Symptom: device_record action=start silently picked one of multiple booted simulators non-deterministically, costing the user a 175s recording when it captured iPhone 15 Pro's home screen while iPhone 17 Pro was being driven by Maestro. The shell script's `xcrun simctl io booted recordVideo` and `adb shell screenrecord` both use implicit device resolution that breaks under multi-device conditions. Fix shape: 1. New `deviceId` arg on device_record start (iOS UDID or Android serial). 2. Pre-flight resolver in the TS handler: - 0 candidates → existing NO_DEVICE path - 1 candidate, no deviceId → auto-select; mark autoSelected: true - explicit deviceId (any candidate count) → must match a candidate or refuse with code=DEVICE_AMBIGUOUS listing all candidates. An explicit selection is authoritative — a stale or typo'd UDID is a failure mode, not a hint. - >1 candidates, no deviceId → DEVICE_AMBIGUOUS with the candidate list so the caller knows the valid identifiers (the GH #173 surface) 3. Echo `deviceId` and `autoSelected` in the start response so callers can verify they got the device they meant. 4. record_proof.sh accepts `--udid ` (iOS) and `--serial ` (Android) flags, threads them into `xcrun simctl io ` and `adb -s `. Falls back to `booted`/auto when no flag passed (single-device case, backward-compatible). 5. Android stop path now reads the persisted serial sidecar and uses `adb -s ` for pkill/pull/rm. Without this, multi-device stop would hit "ambiguous device" or operate on the wrong target — codex-pair caught the gap during review. 6. Defensive: on every Android start, unconditionally write OR clear the serial sidecar. Leaving a stale sidecar from a prior recording would misroute the next stop to a now-disconnected device — also codex-pair. Implementation details: - parseAllBootedIosDevices: returns ALL booted simulators (not just first) - parseAllAdbDevices: returns ALL connected devices including physical (parseAdbDevicesEmu in device-screenshot-raw.ts only matches emulator-*; for device_record's ambiguity check we need physical devices too) - resolveTargetDevice: pure function exported for unit tests - Backward-compatible: single-device callers see no change (autoSelected, no flag forwarded to the script) Tests (1481/1481 passing, +10 net new): - parseAllBootedIosDevices: returns all booted, skips Shutdown - parseAllBootedIosDevices: empty/malformed JSON → [] - parseAllAdbDevices: returns emulators AND physical devices (key distinction from device-screenshot-raw's emu-only parser) - parseAllAdbDevices: skips header and empty lines - resolveTargetDevice: 1 candidate, no deviceId → auto-select - resolveTargetDevice: 1 candidate + mismatched deviceId → AMBIGUOUS (the design fix codex-pair pushed back on — explicit-selection-is- authoritative) - resolveTargetDevice: 1 candidate + matching deviceId → use it (not auto-selected) - resolveTargetDevice: >1 candidates + matching deviceId → use it - resolveTargetDevice: >1 candidates + no deviceId → AMBIGUOUS with full candidate list (THE GH #173 fix surface) - resolveTargetDevice: >1 candidates + non-matching deviceId → AMBIGUOUS (typo surfaces fast) Out of scope (not introduced by this PR; flagged by codex-pair as pre-existing issues in record_proof.sh): - /tmp PID file used for `kill` without strict validation (HIGH) - /tmp venv path predictable (HIGH) - Literal `$` instead of `$$` in raw_file/device_path names — typo'd unique suffix (MED) These should be addressed in a separate hardening pass. Co-Authored-By: Claude Opus 4.7 --- scripts/cdp-bridge/dist/index.js | 3 +- .../cdp-bridge/dist/tools/device-record.js | 124 ++++++++++++- scripts/cdp-bridge/src/index.ts | 3 +- scripts/cdp-bridge/src/tools/device-record.ts | 171 +++++++++++++++++- .../gh-173-device-record-multi-device.test.js | 142 +++++++++++++++ scripts/record_proof.sh | 59 +++++- 6 files changed, 493 insertions(+), 9 deletions(-) create mode 100644 scripts/cdp-bridge/test/unit/gh-173-device-record-multi-device.test.js diff --git a/scripts/cdp-bridge/dist/index.js b/scripts/cdp-bridge/dist/index.js index d2e28ae3..259f6f94 100644 --- a/scripts/cdp-bridge/dist/index.js +++ b/scripts/cdp-bridge/dist/index.js @@ -447,10 +447,11 @@ trackedTool('device_dismiss_system_dialog', 'Tap an OS-level system dialog dismi platform: z.enum(['ios', 'android']).optional().describe('Force platform. Auto-detected from the active session or booted devices if omitted.'), timeoutMs: z.number().int().min(1000).max(60000).optional().describe('Maestro invocation timeout (default 15000ms).'), }, createDeviceDismissSystemDialogHandler()); -trackedTool('device_record', 'Cross-platform screen recording for proof captures. Wraps xcrun simctl io recordVideo (iOS) and adb shell screenrecord (Android), auto-pulls Android files to the host, converts to MP4 with faststart via ffmpeg. Three actions: action="start" begins a background recording (returns pid + output path); action="stop" finalizes ALL active recordings (returns saved files; pass gif=true to also produce GIFs via ffmpeg); action="status" lists active recordings. Android caps at 180s per recording. iOS may stall on long captures via xcrun simctl. Session-less.', { +trackedTool('device_record', 'Cross-platform screen recording for proof captures. Wraps xcrun simctl io recordVideo (iOS) and adb shell screenrecord (Android), auto-pulls Android files to the host, converts to MP4 with faststart via ffmpeg. Three actions: action="start" begins a background recording (returns pid + output path + the deviceId actually used); action="stop" finalizes ALL active recordings (returns saved files; pass gif=true to also produce GIFs via ffmpeg); action="status" lists active recordings. Android caps at 180s per recording. iOS may stall on long captures via xcrun simctl. GH #173: when more than one simulator is booted (or more than one Android device connected), start refuses to auto-pick to avoid recording the wrong device — pass deviceId= to disambiguate; the response echoes the deviceId actually used so you can verify. Session-less.', { action: z.enum(['start', 'stop', 'status']).describe('start: begin recording. stop: finalize and save (all active recordings). status: list active recordings.'), platform: z.enum(['ios', 'android']).optional().describe('(start only) Force platform. Auto-detected from booted devices if omitted.'), outputPath: z.string().optional().describe('(start only) Absolute output path. Defaults to /tmp/rn-dev-agent-proof--.mp4.'), + deviceId: z.string().optional().describe('(start only) Explicit target identifier (iOS UDID or Android serial). Required when more than one device of the same platform is booted/connected — without it, start fails with code=DEVICE_AMBIGUOUS and lists the candidates. Auto-selected when exactly one device is available.'), gif: z.boolean().optional().describe('(stop only) When true, also convert each saved recording to GIF via ffmpeg.'), gifPath: z.string().optional().describe('(stop only) Override GIF output path. Defaults to the recording path with .gif extension.'), }, createDeviceRecordHandler()); diff --git a/scripts/cdp-bridge/dist/tools/device-record.js b/scripts/cdp-bridge/dist/tools/device-record.js index 82f4c5c9..a804bd49 100644 --- a/scripts/cdp-bridge/dist/tools/device-record.js +++ b/scripts/cdp-bridge/dist/tools/device-record.js @@ -11,6 +11,97 @@ const START_TIMEOUT_MS = 10_000; const STOP_TIMEOUT_MS = 60_000; const STATUS_TIMEOUT_MS = 5_000; const GIF_TIMEOUT_MS = 60_000; +export function parseAllBootedIosDevices(jsonText) { + let data; + try { + data = JSON.parse(jsonText); + } + catch { + return []; + } + const runtimes = data?.devices; + if (!runtimes || typeof runtimes !== 'object') + return []; + const out = []; + for (const list of Object.values(runtimes)) { + if (!Array.isArray(list)) + continue; + for (const device of list) { + if (device && device.state === 'Booted' && typeof device.udid === 'string' && device.udid.length > 0) { + out.push({ udid: device.udid, state: device.state, name: device.name }); + } + } + } + return out; +} +export function parseAllAdbDevices(stdout) { + const out = []; + for (const raw of stdout.split('\n')) { + const line = raw.trim(); + if (!line || line.startsWith('List of devices')) + continue; + // Match any serial — not just `emulator-NNNN` — so physical devices count + // toward multi-device ambiguity detection. + const m = line.match(/^(\S+)\s+(device|offline|unauthorized)\b/); + if (!m) + continue; + out.push({ serial: m[1], state: m[2] }); + } + return out; +} +async function listBootedIosUdids() { + try { + const { stdout } = await execFileAsync('xcrun', ['simctl', 'list', '-j', 'devices', 'booted'], { + timeout: 5000, + maxBuffer: 1024 * 1024, + }); + return parseAllBootedIosDevices(stdout); + } + catch { + return []; + } +} +async function listConnectedAndroidDevices() { + try { + const { stdout } = await execFileAsync('adb', ['devices'], { + timeout: 5000, + maxBuffer: 1024 * 1024, + }); + return parseAllAdbDevices(stdout).filter((d) => d.state === 'device'); + } + catch { + return []; + } +} +/** + * Pre-flight target resolution for `device_record start`. Returns the + * device id to use, or a structured ambiguity error listing the + * candidates the caller must pick from. Pure: takes the candidate list + * as input so the unit tests don't need to spawn xcrun/adb. + * + * Rules: + * - 0 candidates → caller's NO_DEVICE path handles it (we don't fire here) + * - 1 candidate → auto-select, mark autoSelected: true + * - >1 + explicit deviceId matches a candidate → use it + * - >1 + explicit deviceId does NOT match → AMBIGUOUS with the full list + * (so caller sees the exact valid ids — typos surface fast) + * - >1 + no deviceId → AMBIGUOUS (the GH #173 bug fix surface) + */ +export function resolveTargetDevice(candidates, deviceId) { + // An explicit deviceId is authoritative regardless of candidate count. + // If the user said "record on X", we must record on X or refuse — silently + // picking a different device is the exact bug GH #173 reports. + if (deviceId) { + if (candidates.some((c) => c.id === deviceId)) { + return { ok: true, deviceId, autoSelected: false, totalAvailable: candidates.length }; + } + return { ok: false, reason: 'AMBIGUOUS', candidates }; + } + if (candidates.length === 1) { + return { ok: true, deviceId: candidates[0].id, autoSelected: true, totalAvailable: 1 }; + } + return { ok: false, reason: 'AMBIGUOUS', candidates }; +} function getPluginRoot() { if (process.env.CLAUDE_PLUGIN_ROOT) return process.env.CLAUDE_PLUGIN_ROOT; @@ -66,8 +157,37 @@ async function runStart(args) { return failResult(`Unknown platform: "${platform}". Expected ios or android.`); } const outputPath = args.outputPath ?? defaultOutputPath(platform); + // GH #173 sub-issue 1: pre-flight multi-device disambiguation. The shell + // script's `simctl io booted` / `adb devices` resolution picks + // non-deterministically when more than one device is booted/connected, + // and silently captures the wrong one. Refuse to start until the + // caller pins a target with `deviceId`. + const candidates = platform === 'ios' + ? (await listBootedIosUdids()).map((d) => ({ id: d.udid, label: d.name })) + : (await listConnectedAndroidDevices()).map((d) => ({ id: d.serial })); + if (candidates.length === 0) { + return failResult(platform === 'ios' ? 'No iOS simulator booted.' : 'No Android device connected.', { code: 'NO_DEVICE' }); + } + const resolution = resolveTargetDevice(candidates, args.deviceId); + if (!resolution.ok) { + const list = resolution.candidates + .map((c) => ` - ${c.id}${c.label ? ` (${c.label})` : ''}`) + .join('\n'); + const argName = platform === 'ios' ? 'UDID' : 'serial'; + return failResult(`device_record: ${resolution.candidates.length} ${platform} ${argName === 'UDID' ? 'simulators booted' : 'devices connected'} — refusing to auto-pick to avoid recording the wrong device. ` + + `Pass deviceId=<${argName}> to disambiguate:\n${list}`, { code: 'DEVICE_AMBIGUOUS', platform, candidates: resolution.candidates }); + } + const scriptArgs = ['start', platform, outputPath]; + // Only forward an explicit id when we're picking from >1 candidate; the + // single-device case keeps the script's existing `booted`/auto path so + // we don't regress any environment where simctl's `booted` shorthand + // works differently than passing the literal UDID (defensive — both + // should be equivalent on Apple's side). + if (!resolution.autoSelected) { + scriptArgs.push(platform === 'ios' ? '--udid' : '--serial', resolution.deviceId); + } try { - const { stdout } = await execFileAsync(getRecordScript(), ['start', platform, outputPath], { timeout: START_TIMEOUT_MS }); + const { stdout } = await execFileAsync(getRecordScript(), scriptArgs, { timeout: START_TIMEOUT_MS }); const parsed = parseStartOutput(stdout); if (!parsed) { return failResult(`Recording started but could not parse PID/output. Raw: ${stdout.trim()}`); @@ -75,6 +195,8 @@ async function runStart(args) { return okResult({ action: 'start', platform, + deviceId: resolution.deviceId, + autoSelected: resolution.autoSelected, output: parsed.output, pid: parsed.pid, note: 'Call device_record action=stop to finalize. Android caps at 180s; iOS has no inherent cap but xcrun simctl io may stall on long captures.', diff --git a/scripts/cdp-bridge/src/index.ts b/scripts/cdp-bridge/src/index.ts index 242e6c2a..e1fee1fc 100644 --- a/scripts/cdp-bridge/src/index.ts +++ b/scripts/cdp-bridge/src/index.ts @@ -743,11 +743,12 @@ trackedTool( trackedTool( 'device_record', - 'Cross-platform screen recording for proof captures. Wraps xcrun simctl io recordVideo (iOS) and adb shell screenrecord (Android), auto-pulls Android files to the host, converts to MP4 with faststart via ffmpeg. Three actions: action="start" begins a background recording (returns pid + output path); action="stop" finalizes ALL active recordings (returns saved files; pass gif=true to also produce GIFs via ffmpeg); action="status" lists active recordings. Android caps at 180s per recording. iOS may stall on long captures via xcrun simctl. Session-less.', + 'Cross-platform screen recording for proof captures. Wraps xcrun simctl io recordVideo (iOS) and adb shell screenrecord (Android), auto-pulls Android files to the host, converts to MP4 with faststart via ffmpeg. Three actions: action="start" begins a background recording (returns pid + output path + the deviceId actually used); action="stop" finalizes ALL active recordings (returns saved files; pass gif=true to also produce GIFs via ffmpeg); action="status" lists active recordings. Android caps at 180s per recording. iOS may stall on long captures via xcrun simctl. GH #173: when more than one simulator is booted (or more than one Android device connected), start refuses to auto-pick to avoid recording the wrong device — pass deviceId= to disambiguate; the response echoes the deviceId actually used so you can verify. Session-less.', { action: z.enum(['start', 'stop', 'status']).describe('start: begin recording. stop: finalize and save (all active recordings). status: list active recordings.'), platform: z.enum(['ios', 'android']).optional().describe('(start only) Force platform. Auto-detected from booted devices if omitted.'), outputPath: z.string().optional().describe('(start only) Absolute output path. Defaults to /tmp/rn-dev-agent-proof--.mp4.'), + deviceId: z.string().optional().describe('(start only) Explicit target identifier (iOS UDID or Android serial). Required when more than one device of the same platform is booted/connected — without it, start fails with code=DEVICE_AMBIGUOUS and lists the candidates. Auto-selected when exactly one device is available.'), gif: z.boolean().optional().describe('(stop only) When true, also convert each saved recording to GIF via ffmpeg.'), gifPath: z.string().optional().describe('(stop only) Override GIF output path. Defaults to the recording path with .gif extension.'), }, diff --git a/scripts/cdp-bridge/src/tools/device-record.ts b/scripts/cdp-bridge/src/tools/device-record.ts index 4dd671c3..00d1004d 100644 --- a/scripts/cdp-bridge/src/tools/device-record.ts +++ b/scripts/cdp-bridge/src/tools/device-record.ts @@ -21,6 +21,134 @@ export interface DeviceRecordArgs { outputPath?: string; gif?: boolean; gifPath?: string; + /** + * GH #173 (sub-issue 1): explicit target identifier for multi-device + * scenarios. iOS UDID for `simctl io recordVideo`, Android + * serial for `adb -s shell screenrecord`. Required when more + * than one device of the same platform is booted/connected — without + * it, `simctl io booted` and `adb devices` pick non-deterministically + * and silently capture the wrong device (the user's reported pain). + */ + deviceId?: string; +} + +interface SimctlDevice { + udid: string; + state: string; + name?: string; +} +interface SimctlListPayload { + devices?: Record; +} + +export function parseAllBootedIosDevices(jsonText: string): SimctlDevice[] { + let data: SimctlListPayload; + try { + data = JSON.parse(jsonText) as SimctlListPayload; + } catch { + return []; + } + const runtimes = data?.devices; + if (!runtimes || typeof runtimes !== 'object') return []; + const out: SimctlDevice[] = []; + for (const list of Object.values(runtimes)) { + if (!Array.isArray(list)) continue; + for (const device of list) { + if (device && device.state === 'Booted' && typeof device.udid === 'string' && device.udid.length > 0) { + out.push({ udid: device.udid, state: device.state, name: device.name }); + } + } + } + return out; +} + +export interface AdbDevice { + serial: string; + state: 'device' | 'offline' | 'unauthorized'; +} + +export function parseAllAdbDevices(stdout: string): AdbDevice[] { + const out: AdbDevice[] = []; + for (const raw of stdout.split('\n')) { + const line = raw.trim(); + if (!line || line.startsWith('List of devices')) continue; + // Match any serial — not just `emulator-NNNN` — so physical devices count + // toward multi-device ambiguity detection. + const m = line.match(/^(\S+)\s+(device|offline|unauthorized)\b/); + if (!m) continue; + out.push({ serial: m[1], state: m[2] as 'device' | 'offline' | 'unauthorized' }); + } + return out; +} + +async function listBootedIosUdids(): Promise { + try { + const { stdout } = await execFileAsync('xcrun', ['simctl', 'list', '-j', 'devices', 'booted'], { + timeout: 5000, + maxBuffer: 1024 * 1024, + }); + return parseAllBootedIosDevices(stdout); + } catch { + return []; + } +} + +async function listConnectedAndroidDevices(): Promise { + try { + const { stdout } = await execFileAsync('adb', ['devices'], { + timeout: 5000, + maxBuffer: 1024 * 1024, + }); + return parseAllAdbDevices(stdout).filter((d) => d.state === 'device'); + } catch { + return []; + } +} + +export interface DeviceResolution { + ok: true; + deviceId: string; + autoSelected: boolean; + totalAvailable: number; +} + +export interface DeviceResolutionAmbiguous { + ok: false; + reason: 'AMBIGUOUS'; + candidates: Array<{ id: string; label?: string }>; +} + +/** + * Pre-flight target resolution for `device_record start`. Returns the + * device id to use, or a structured ambiguity error listing the + * candidates the caller must pick from. Pure: takes the candidate list + * as input so the unit tests don't need to spawn xcrun/adb. + * + * Rules: + * - 0 candidates → caller's NO_DEVICE path handles it (we don't fire here) + * - 1 candidate → auto-select, mark autoSelected: true + * - >1 + explicit deviceId matches a candidate → use it + * - >1 + explicit deviceId does NOT match → AMBIGUOUS with the full list + * (so caller sees the exact valid ids — typos surface fast) + * - >1 + no deviceId → AMBIGUOUS (the GH #173 bug fix surface) + */ +export function resolveTargetDevice( + candidates: Array<{ id: string; label?: string }>, + deviceId: string | undefined, +): DeviceResolution | DeviceResolutionAmbiguous { + // An explicit deviceId is authoritative regardless of candidate count. + // If the user said "record on X", we must record on X or refuse — silently + // picking a different device is the exact bug GH #173 reports. + if (deviceId) { + if (candidates.some((c) => c.id === deviceId)) { + return { ok: true, deviceId, autoSelected: false, totalAvailable: candidates.length }; + } + return { ok: false, reason: 'AMBIGUOUS', candidates }; + } + if (candidates.length === 1) { + return { ok: true, deviceId: candidates[0].id, autoSelected: true, totalAvailable: 1 }; + } + return { ok: false, reason: 'AMBIGUOUS', candidates }; } function getPluginRoot(): string { @@ -97,10 +225,49 @@ async function runStart(args: DeviceRecordArgs): Promise { } const outputPath = args.outputPath ?? defaultOutputPath(platform); + // GH #173 sub-issue 1: pre-flight multi-device disambiguation. The shell + // script's `simctl io booted` / `adb devices` resolution picks + // non-deterministically when more than one device is booted/connected, + // and silently captures the wrong one. Refuse to start until the + // caller pins a target with `deviceId`. + const candidates = platform === 'ios' + ? (await listBootedIosUdids()).map((d) => ({ id: d.udid, label: d.name })) + : (await listConnectedAndroidDevices()).map((d) => ({ id: d.serial })); + + if (candidates.length === 0) { + return failResult( + platform === 'ios' ? 'No iOS simulator booted.' : 'No Android device connected.', + { code: 'NO_DEVICE' }, + ); + } + + const resolution = resolveTargetDevice(candidates, args.deviceId); + if (!resolution.ok) { + const list = resolution.candidates + .map((c) => ` - ${c.id}${c.label ? ` (${c.label})` : ''}`) + .join('\n'); + const argName = platform === 'ios' ? 'UDID' : 'serial'; + return failResult( + `device_record: ${resolution.candidates.length} ${platform} ${argName === 'UDID' ? 'simulators booted' : 'devices connected'} — refusing to auto-pick to avoid recording the wrong device. ` + + `Pass deviceId=<${argName}> to disambiguate:\n${list}`, + { code: 'DEVICE_AMBIGUOUS', platform, candidates: resolution.candidates }, + ); + } + + const scriptArgs = ['start', platform, outputPath]; + // Only forward an explicit id when we're picking from >1 candidate; the + // single-device case keeps the script's existing `booted`/auto path so + // we don't regress any environment where simctl's `booted` shorthand + // works differently than passing the literal UDID (defensive — both + // should be equivalent on Apple's side). + if (!resolution.autoSelected) { + scriptArgs.push(platform === 'ios' ? '--udid' : '--serial', resolution.deviceId); + } + try { const { stdout } = await execFileAsync( getRecordScript(), - ['start', platform, outputPath], + scriptArgs, { timeout: START_TIMEOUT_MS }, ); const parsed = parseStartOutput(stdout); @@ -110,6 +277,8 @@ async function runStart(args: DeviceRecordArgs): Promise { return okResult({ action: 'start', platform, + deviceId: resolution.deviceId, + autoSelected: resolution.autoSelected, output: parsed.output, pid: parsed.pid, note: 'Call device_record action=stop to finalize. Android caps at 180s; iOS has no inherent cap but xcrun simctl io may stall on long captures.', diff --git a/scripts/cdp-bridge/test/unit/gh-173-device-record-multi-device.test.js b/scripts/cdp-bridge/test/unit/gh-173-device-record-multi-device.test.js new file mode 100644 index 00000000..a5099d87 --- /dev/null +++ b/scripts/cdp-bridge/test/unit/gh-173-device-record-multi-device.test.js @@ -0,0 +1,142 @@ +// GH #173 sub-issue 1: device_record silently captured the wrong simulator +// when more than one was booted, costing the reporter a 175s recording. +// PR-1 adds a pre-flight resolver — refuse to auto-pick when 2+ candidates, +// require explicit deviceId to disambiguate, echo the picked id back so +// callers can verify they got the device they meant. + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + parseAllBootedIosDevices, + parseAllAdbDevices, + resolveTargetDevice, +} from '../../dist/tools/device-record.js'; + +// ───────────────────────────────────────────────────────────────────────────── +// parseAllBootedIosDevices +// ───────────────────────────────────────────────────────────────────────────── + +test('parseAllBootedIosDevices: returns every Booted device, skips Shutdown', () => { + const json = JSON.stringify({ + devices: { + 'com.apple.CoreSimulator.SimRuntime.iOS-18-0': [ + { udid: 'AAA-SHUT', state: 'Shutdown', name: 'iPhone 16' }, + { udid: 'BBB-BOOTED', state: 'Booted', name: 'iPhone 17 Pro' }, + { udid: 'CCC-BOOTED', state: 'Booted', name: 'iPhone 15 Pro' }, + ], + }, + }); + const out = parseAllBootedIosDevices(json); + assert.equal(out.length, 2, 'should return both booted devices'); + assert.deepEqual(out.map((d) => d.udid).sort(), ['BBB-BOOTED', 'CCC-BOOTED']); + // Friendly name preserved for the ambiguity error message. + const bbb = out.find((d) => d.udid === 'BBB-BOOTED'); + assert.equal(bbb.name, 'iPhone 17 Pro'); +}); + +test('parseAllBootedIosDevices: empty / malformed JSON yields empty list', () => { + assert.deepEqual(parseAllBootedIosDevices('not-json'), []); + assert.deepEqual(parseAllBootedIosDevices('{}'), []); + assert.deepEqual(parseAllBootedIosDevices(JSON.stringify({ devices: {} })), []); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// parseAllAdbDevices +// ───────────────────────────────────────────────────────────────────────────── + +test('parseAllAdbDevices: returns emulators AND physical devices (multi-device disambiguation)', () => { + // Note: parseAdbDevicesEmu in device-screenshot-raw.ts only matches + // emulator-* — for device_record's multi-device check we need physical + // devices to count too. This test pins that distinction. + const stdout = + 'List of devices attached\n' + + 'emulator-5554\tdevice\n' + + 'R3CW70BFGAA\tdevice\n' + + 'emulator-5556\toffline\n' + + 'emulator-5558\tunauthorized\n'; + const out = parseAllAdbDevices(stdout); + // All four lines are surfaced — the resolver caller filters by state. + assert.equal(out.length, 4); + const states = out.reduce((acc, d) => { acc[d.serial] = d.state; return acc; }, {}); + assert.equal(states['emulator-5554'], 'device'); + assert.equal(states['R3CW70BFGAA'], 'device'); + assert.equal(states['emulator-5556'], 'offline'); + assert.equal(states['emulator-5558'], 'unauthorized'); +}); + +test('parseAllAdbDevices: skips the header line and empty lines', () => { + assert.deepEqual(parseAllAdbDevices(''), []); + assert.deepEqual(parseAllAdbDevices('List of devices attached\n'), []); + assert.deepEqual(parseAllAdbDevices('List of devices attached\n\n\n'), []); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// resolveTargetDevice +// ───────────────────────────────────────────────────────────────────────────── + +test('resolveTargetDevice: 1 candidate, no deviceId → auto-select', () => { + const r = resolveTargetDevice([{ id: 'BBB-BOOTED' }], undefined); + assert.equal(r.ok, true); + assert.equal(r.deviceId, 'BBB-BOOTED'); + assert.equal(r.autoSelected, true); + assert.equal(r.totalAvailable, 1); +}); + +test('resolveTargetDevice: 1 candidate + mismatched deviceId → AMBIGUOUS (explicit selection is authoritative)', () => { + // Even with only one device available, an explicit deviceId must + // match or we refuse. Silently picking the only device when the + // caller asked for a different one is the same class of wrong-device + // behavior GH #173 is trying to prevent — a stale UDID from a saved + // workflow would silently record the wrong sim. + const r = resolveTargetDevice([{ id: 'BBB-BOOTED' }], 'something-else'); + assert.equal(r.ok, false); + assert.equal(r.reason, 'AMBIGUOUS'); + assert.equal(r.candidates.length, 1); + assert.equal(r.candidates[0].id, 'BBB-BOOTED'); +}); + +test('resolveTargetDevice: 1 candidate + matching deviceId → use it (not auto-selected)', () => { + const r = resolveTargetDevice([{ id: 'BBB-BOOTED' }], 'BBB-BOOTED'); + assert.equal(r.ok, true); + assert.equal(r.deviceId, 'BBB-BOOTED'); + assert.equal(r.autoSelected, false, 'explicit match is not auto-selected even with only one candidate'); +}); + +test('resolveTargetDevice: >1 candidates, deviceId matches one → use it', () => { + const r = resolveTargetDevice( + [{ id: 'AAA' }, { id: 'BBB' }, { id: 'CCC' }], + 'BBB', + ); + assert.equal(r.ok, true); + assert.equal(r.deviceId, 'BBB'); + assert.equal(r.autoSelected, false); + assert.equal(r.totalAvailable, 3); +}); + +test('resolveTargetDevice: >1 candidates, no deviceId → AMBIGUOUS with full list (THE GH #173 fix surface)', () => { + const r = resolveTargetDevice( + [ + { id: 'BBB-BOOTED', label: 'iPhone 17 Pro' }, + { id: 'CCC-BOOTED', label: 'iPhone 15 Pro' }, + ], + undefined, + ); + assert.equal(r.ok, false); + assert.equal(r.reason, 'AMBIGUOUS'); + assert.deepEqual( + r.candidates.map((c) => c.id).sort(), + ['BBB-BOOTED', 'CCC-BOOTED'], + ); + // Labels preserved so the error message can name what each id is. + assert.equal(r.candidates.find((c) => c.id === 'BBB-BOOTED').label, 'iPhone 17 Pro'); +}); + +test('resolveTargetDevice: >1 candidates, deviceId does NOT match → AMBIGUOUS (typo surfaces fast)', () => { + const r = resolveTargetDevice( + [{ id: 'AAA' }, { id: 'BBB' }], + 'typo-here', + ); + assert.equal(r.ok, false); + assert.equal(r.reason, 'AMBIGUOUS'); + assert.equal(r.candidates.length, 2); +}); diff --git a/scripts/record_proof.sh b/scripts/record_proof.sh index 3ebfc1d0..b475fd88 100755 --- a/scripts/record_proof.sh +++ b/scripts/record_proof.sh @@ -35,10 +35,32 @@ is_alive() { cmd_start() { local platform="${1:-}" local output_path="${2:-}" + shift 2 2>/dev/null || true [[ -z "$platform" || -z "$output_path" ]] && { echo "Error: start requires " >&2; exit 1; } [[ "$platform" != "ios" && "$platform" != "android" ]] && { echo "Error: platform must be ios or android" >&2; exit 1; } + # GH #173 sub-issue 1: optional explicit target identifier for multi-device + # scenarios. `--udid ` for iOS (passed verbatim to `simctl io`), + # `--serial ` for Android (passed to `adb -s`). The TS handler at + # device-record.ts:resolveTargetDevice does pre-flight ambiguity detection + # and only forwards an identifier when there are 2+ candidates; the + # single-device case still uses the implicit `booted`/auto resolver below. + local target_id="" + while [[ $# -gt 0 ]]; do + case "$1" in + --udid|--serial) + target_id="${2:-}" + [[ -z "$target_id" ]] && { echo "Error: $1 requires a value" >&2; exit 1; } + shift 2 + ;; + *) + echo "Error: unknown flag '$1' for start" >&2 + exit 1 + ;; + esac + done + local pf pf="$(pid_file "$platform")" if [[ -f "$pf" ]] && is_alive "$(cat "$pf")"; then @@ -57,7 +79,8 @@ cmd_start() { echo "Error: No iOS simulator booted" >&2 exit 1 fi - xcrun simctl io booted recordVideo --force "$raw_file" & + local ios_target="${target_id:-booted}" + xcrun simctl io "$ios_target" recordVideo --force "$raw_file" & local rec_pid=$! else raw_file="${RAW_PREFIX}-${platform}-$$.mp4" @@ -66,7 +89,11 @@ cmd_start() { exit 1 fi local device_path="/sdcard/rn-dev-agent-proof-$$.mp4" - adb shell screenrecord "$device_path" & + if [[ -n "$target_id" ]]; then + adb -s "$target_id" shell screenrecord "$device_path" & + else + adb shell screenrecord "$device_path" & + fi local rec_pid=$! fi @@ -81,6 +108,17 @@ cmd_start() { echo "$output_path" > "$(path_file "$platform")" echo "$raw_file" > "${PID_PREFIX}-${platform}.raw-path" [[ "$platform" == "android" ]] && echo "$device_path" > "${PID_PREFIX}-${platform}.device-path" + # Persist the Android serial so the stop path scopes pkill/pull/rm to + # the same device. Unconditionally write or clear — leaving a stale + # sidecar from a prior recording would misroute this stop to a now- + # disconnected device. + if [[ "$platform" == "android" ]]; then + if [[ -n "$target_id" ]]; then + echo "$target_id" > "${PID_PREFIX}-${platform}.serial" + else + rm -f "${PID_PREFIX}-${platform}.serial" + fi + fi echo "Recording started: platform=$platform pid=$rec_pid output=$output_path" } @@ -128,17 +166,28 @@ cmd_stop() { # --- Pull Android recording from device --- if [[ "$platform" == "android" ]]; then + # GH #173 sub-issue 1: scope every adb call to the serial that + # cmd_start recorded against, so multi-device stop doesn't hit + # "ambiguous device" or operate on the wrong target. Falls back to + # implicit selection when no serial sidecar exists (single-device + # case, or recording started before this change). + local -a adb_args=() + local serialf="${PID_PREFIX}-${platform}.serial" + if [[ -f "$serialf" ]]; then + adb_args+=(-s "$(cat "$serialf")") + fi # Ensure remote screenrecord is stopped (local SIGINT may not propagate) - adb shell pkill -2 screenrecord 2>/dev/null || true + adb "${adb_args[@]}" shell pkill -2 screenrecord 2>/dev/null || true local device_pathf="${PID_PREFIX}-${platform}.device-path" if [[ -f "$device_pathf" ]]; then local device_path device_path="$(cat "$device_pathf")" sleep 2 - adb pull "$device_path" "$raw_file" >/dev/null 2>&1 || echo "Warning: Failed to pull recording from device" >&2 - adb shell rm -f "$device_path" 2>/dev/null || true + adb "${adb_args[@]}" pull "$device_path" "$raw_file" >/dev/null 2>&1 || echo "Warning: Failed to pull recording from device" >&2 + adb "${adb_args[@]}" shell rm -f "$device_path" 2>/dev/null || true rm -f "$device_pathf" fi + rm -f "$serialf" fi # --- Convert to MP4 with faststart + validate ---