Skip to content

Commit b3324a7

Browse files
Lykhoydaclaude
andauthored
fix(device-record): refuse multi-sim ambiguity, thread deviceId through script (#177)
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 <UDID>` (iOS) and `--serial <SERIAL>` (Android) flags, threads them into `xcrun simctl io <UDID>` and `adb -s <SERIAL>`. 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 <serial>` 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 <noreply@anthropic.com>
1 parent a18669f commit b3324a7

6 files changed

Lines changed: 493 additions & 9 deletions

File tree

scripts/cdp-bridge/dist/index.js

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

scripts/cdp-bridge/dist/tools/device-record.js

Lines changed: 123 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,97 @@ const START_TIMEOUT_MS = 10_000;
1111
const STOP_TIMEOUT_MS = 60_000;
1212
const STATUS_TIMEOUT_MS = 5_000;
1313
const GIF_TIMEOUT_MS = 60_000;
14+
export function parseAllBootedIosDevices(jsonText) {
15+
let data;
16+
try {
17+
data = JSON.parse(jsonText);
18+
}
19+
catch {
20+
return [];
21+
}
22+
const runtimes = data?.devices;
23+
if (!runtimes || typeof runtimes !== 'object')
24+
return [];
25+
const out = [];
26+
for (const list of Object.values(runtimes)) {
27+
if (!Array.isArray(list))
28+
continue;
29+
for (const device of list) {
30+
if (device && device.state === 'Booted' && typeof device.udid === 'string' && device.udid.length > 0) {
31+
out.push({ udid: device.udid, state: device.state, name: device.name });
32+
}
33+
}
34+
}
35+
return out;
36+
}
37+
export function parseAllAdbDevices(stdout) {
38+
const out = [];
39+
for (const raw of stdout.split('\n')) {
40+
const line = raw.trim();
41+
if (!line || line.startsWith('List of devices'))
42+
continue;
43+
// Match any serial — not just `emulator-NNNN` — so physical devices count
44+
// toward multi-device ambiguity detection.
45+
const m = line.match(/^(\S+)\s+(device|offline|unauthorized)\b/);
46+
if (!m)
47+
continue;
48+
out.push({ serial: m[1], state: m[2] });
49+
}
50+
return out;
51+
}
52+
async function listBootedIosUdids() {
53+
try {
54+
const { stdout } = await execFileAsync('xcrun', ['simctl', 'list', '-j', 'devices', 'booted'], {
55+
timeout: 5000,
56+
maxBuffer: 1024 * 1024,
57+
});
58+
return parseAllBootedIosDevices(stdout);
59+
}
60+
catch {
61+
return [];
62+
}
63+
}
64+
async function listConnectedAndroidDevices() {
65+
try {
66+
const { stdout } = await execFileAsync('adb', ['devices'], {
67+
timeout: 5000,
68+
maxBuffer: 1024 * 1024,
69+
});
70+
return parseAllAdbDevices(stdout).filter((d) => d.state === 'device');
71+
}
72+
catch {
73+
return [];
74+
}
75+
}
76+
/**
77+
* Pre-flight target resolution for `device_record start`. Returns the
78+
* device id to use, or a structured ambiguity error listing the
79+
* candidates the caller must pick from. Pure: takes the candidate list
80+
* as input so the unit tests don't need to spawn xcrun/adb.
81+
*
82+
* Rules:
83+
* - 0 candidates → caller's NO_DEVICE path handles it (we don't fire here)
84+
* - 1 candidate → auto-select, mark autoSelected: true
85+
* - >1 + explicit deviceId matches a candidate → use it
86+
* - >1 + explicit deviceId does NOT match → AMBIGUOUS with the full list
87+
* (so caller sees the exact valid ids — typos surface fast)
88+
* - >1 + no deviceId → AMBIGUOUS (the GH #173 bug fix surface)
89+
*/
90+
export function resolveTargetDevice(candidates, deviceId) {
91+
// An explicit deviceId is authoritative regardless of candidate count.
92+
// If the user said "record on X", we must record on X or refuse — silently
93+
// picking a different device is the exact bug GH #173 reports.
94+
if (deviceId) {
95+
if (candidates.some((c) => c.id === deviceId)) {
96+
return { ok: true, deviceId, autoSelected: false, totalAvailable: candidates.length };
97+
}
98+
return { ok: false, reason: 'AMBIGUOUS', candidates };
99+
}
100+
if (candidates.length === 1) {
101+
return { ok: true, deviceId: candidates[0].id, autoSelected: true, totalAvailable: 1 };
102+
}
103+
return { ok: false, reason: 'AMBIGUOUS', candidates };
104+
}
14105
function getPluginRoot() {
15106
if (process.env.CLAUDE_PLUGIN_ROOT)
16107
return process.env.CLAUDE_PLUGIN_ROOT;
@@ -66,15 +157,46 @@ async function runStart(args) {
66157
return failResult(`Unknown platform: "${platform}". Expected ios or android.`);
67158
}
68159
const outputPath = args.outputPath ?? defaultOutputPath(platform);
160+
// GH #173 sub-issue 1: pre-flight multi-device disambiguation. The shell
161+
// script's `simctl io booted` / `adb devices` resolution picks
162+
// non-deterministically when more than one device is booted/connected,
163+
// and silently captures the wrong one. Refuse to start until the
164+
// caller pins a target with `deviceId`.
165+
const candidates = platform === 'ios'
166+
? (await listBootedIosUdids()).map((d) => ({ id: d.udid, label: d.name }))
167+
: (await listConnectedAndroidDevices()).map((d) => ({ id: d.serial }));
168+
if (candidates.length === 0) {
169+
return failResult(platform === 'ios' ? 'No iOS simulator booted.' : 'No Android device connected.', { code: 'NO_DEVICE' });
170+
}
171+
const resolution = resolveTargetDevice(candidates, args.deviceId);
172+
if (!resolution.ok) {
173+
const list = resolution.candidates
174+
.map((c) => ` - ${c.id}${c.label ? ` (${c.label})` : ''}`)
175+
.join('\n');
176+
const argName = platform === 'ios' ? 'UDID' : 'serial';
177+
return failResult(`device_record: ${resolution.candidates.length} ${platform} ${argName === 'UDID' ? 'simulators booted' : 'devices connected'} — refusing to auto-pick to avoid recording the wrong device. ` +
178+
`Pass deviceId=<${argName}> to disambiguate:\n${list}`, { code: 'DEVICE_AMBIGUOUS', platform, candidates: resolution.candidates });
179+
}
180+
const scriptArgs = ['start', platform, outputPath];
181+
// Only forward an explicit id when we're picking from >1 candidate; the
182+
// single-device case keeps the script's existing `booted`/auto path so
183+
// we don't regress any environment where simctl's `booted` shorthand
184+
// works differently than passing the literal UDID (defensive — both
185+
// should be equivalent on Apple's side).
186+
if (!resolution.autoSelected) {
187+
scriptArgs.push(platform === 'ios' ? '--udid' : '--serial', resolution.deviceId);
188+
}
69189
try {
70-
const { stdout } = await execFileAsync(getRecordScript(), ['start', platform, outputPath], { timeout: START_TIMEOUT_MS });
190+
const { stdout } = await execFileAsync(getRecordScript(), scriptArgs, { timeout: START_TIMEOUT_MS });
71191
const parsed = parseStartOutput(stdout);
72192
if (!parsed) {
73193
return failResult(`Recording started but could not parse PID/output. Raw: ${stdout.trim()}`);
74194
}
75195
return okResult({
76196
action: 'start',
77197
platform,
198+
deviceId: resolution.deviceId,
199+
autoSelected: resolution.autoSelected,
78200
output: parsed.output,
79201
pid: parsed.pid,
80202
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.',

scripts/cdp-bridge/src/index.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -743,11 +743,12 @@ trackedTool(
743743

744744
trackedTool(
745745
'device_record',
746-
'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.',
746+
'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=<UDID|serial> to disambiguate; the response echoes the deviceId actually used so you can verify. Session-less.',
747747
{
748748
action: z.enum(['start', 'stop', 'status']).describe('start: begin recording. stop: finalize and save (all active recordings). status: list active recordings.'),
749749
platform: z.enum(['ios', 'android']).optional().describe('(start only) Force platform. Auto-detected from booted devices if omitted.'),
750750
outputPath: z.string().optional().describe('(start only) Absolute output path. Defaults to /tmp/rn-dev-agent-proof-<platform>-<timestamp>.mp4.'),
751+
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.'),
751752
gif: z.boolean().optional().describe('(stop only) When true, also convert each saved recording to GIF via ffmpeg.'),
752753
gifPath: z.string().optional().describe('(stop only) Override GIF output path. Defaults to the recording path with .gif extension.'),
753754
},

scripts/cdp-bridge/src/tools/device-record.ts

Lines changed: 170 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,134 @@ export interface DeviceRecordArgs {
2121
outputPath?: string;
2222
gif?: boolean;
2323
gifPath?: string;
24+
/**
25+
* GH #173 (sub-issue 1): explicit target identifier for multi-device
26+
* scenarios. iOS UDID for `simctl io <UDID> recordVideo`, Android
27+
* serial for `adb -s <SERIAL> shell screenrecord`. Required when more
28+
* than one device of the same platform is booted/connected — without
29+
* it, `simctl io booted` and `adb devices` pick non-deterministically
30+
* and silently capture the wrong device (the user's reported pain).
31+
*/
32+
deviceId?: string;
33+
}
34+
35+
interface SimctlDevice {
36+
udid: string;
37+
state: string;
38+
name?: string;
39+
}
40+
interface SimctlListPayload {
41+
devices?: Record<string, SimctlDevice[]>;
42+
}
43+
44+
export function parseAllBootedIosDevices(jsonText: string): SimctlDevice[] {
45+
let data: SimctlListPayload;
46+
try {
47+
data = JSON.parse(jsonText) as SimctlListPayload;
48+
} catch {
49+
return [];
50+
}
51+
const runtimes = data?.devices;
52+
if (!runtimes || typeof runtimes !== 'object') return [];
53+
const out: SimctlDevice[] = [];
54+
for (const list of Object.values(runtimes)) {
55+
if (!Array.isArray(list)) continue;
56+
for (const device of list) {
57+
if (device && device.state === 'Booted' && typeof device.udid === 'string' && device.udid.length > 0) {
58+
out.push({ udid: device.udid, state: device.state, name: device.name });
59+
}
60+
}
61+
}
62+
return out;
63+
}
64+
65+
export interface AdbDevice {
66+
serial: string;
67+
state: 'device' | 'offline' | 'unauthorized';
68+
}
69+
70+
export function parseAllAdbDevices(stdout: string): AdbDevice[] {
71+
const out: AdbDevice[] = [];
72+
for (const raw of stdout.split('\n')) {
73+
const line = raw.trim();
74+
if (!line || line.startsWith('List of devices')) continue;
75+
// Match any serial — not just `emulator-NNNN` — so physical devices count
76+
// toward multi-device ambiguity detection.
77+
const m = line.match(/^(\S+)\s+(device|offline|unauthorized)\b/);
78+
if (!m) continue;
79+
out.push({ serial: m[1], state: m[2] as 'device' | 'offline' | 'unauthorized' });
80+
}
81+
return out;
82+
}
83+
84+
async function listBootedIosUdids(): Promise<SimctlDevice[]> {
85+
try {
86+
const { stdout } = await execFileAsync('xcrun', ['simctl', 'list', '-j', 'devices', 'booted'], {
87+
timeout: 5000,
88+
maxBuffer: 1024 * 1024,
89+
});
90+
return parseAllBootedIosDevices(stdout);
91+
} catch {
92+
return [];
93+
}
94+
}
95+
96+
async function listConnectedAndroidDevices(): Promise<AdbDevice[]> {
97+
try {
98+
const { stdout } = await execFileAsync('adb', ['devices'], {
99+
timeout: 5000,
100+
maxBuffer: 1024 * 1024,
101+
});
102+
return parseAllAdbDevices(stdout).filter((d) => d.state === 'device');
103+
} catch {
104+
return [];
105+
}
106+
}
107+
108+
export interface DeviceResolution {
109+
ok: true;
110+
deviceId: string;
111+
autoSelected: boolean;
112+
totalAvailable: number;
113+
}
114+
115+
export interface DeviceResolutionAmbiguous {
116+
ok: false;
117+
reason: 'AMBIGUOUS';
118+
candidates: Array<{ id: string; label?: string }>;
119+
}
120+
121+
/**
122+
* Pre-flight target resolution for `device_record start`. Returns the
123+
* device id to use, or a structured ambiguity error listing the
124+
* candidates the caller must pick from. Pure: takes the candidate list
125+
* as input so the unit tests don't need to spawn xcrun/adb.
126+
*
127+
* Rules:
128+
* - 0 candidates → caller's NO_DEVICE path handles it (we don't fire here)
129+
* - 1 candidate → auto-select, mark autoSelected: true
130+
* - >1 + explicit deviceId matches a candidate → use it
131+
* - >1 + explicit deviceId does NOT match → AMBIGUOUS with the full list
132+
* (so caller sees the exact valid ids — typos surface fast)
133+
* - >1 + no deviceId → AMBIGUOUS (the GH #173 bug fix surface)
134+
*/
135+
export function resolveTargetDevice(
136+
candidates: Array<{ id: string; label?: string }>,
137+
deviceId: string | undefined,
138+
): DeviceResolution | DeviceResolutionAmbiguous {
139+
// An explicit deviceId is authoritative regardless of candidate count.
140+
// If the user said "record on X", we must record on X or refuse — silently
141+
// picking a different device is the exact bug GH #173 reports.
142+
if (deviceId) {
143+
if (candidates.some((c) => c.id === deviceId)) {
144+
return { ok: true, deviceId, autoSelected: false, totalAvailable: candidates.length };
145+
}
146+
return { ok: false, reason: 'AMBIGUOUS', candidates };
147+
}
148+
if (candidates.length === 1) {
149+
return { ok: true, deviceId: candidates[0].id, autoSelected: true, totalAvailable: 1 };
150+
}
151+
return { ok: false, reason: 'AMBIGUOUS', candidates };
24152
}
25153

26154
function getPluginRoot(): string {
@@ -97,10 +225,49 @@ async function runStart(args: DeviceRecordArgs): Promise<ToolResult> {
97225
}
98226
const outputPath = args.outputPath ?? defaultOutputPath(platform);
99227

228+
// GH #173 sub-issue 1: pre-flight multi-device disambiguation. The shell
229+
// script's `simctl io booted` / `adb devices` resolution picks
230+
// non-deterministically when more than one device is booted/connected,
231+
// and silently captures the wrong one. Refuse to start until the
232+
// caller pins a target with `deviceId`.
233+
const candidates = platform === 'ios'
234+
? (await listBootedIosUdids()).map((d) => ({ id: d.udid, label: d.name }))
235+
: (await listConnectedAndroidDevices()).map((d) => ({ id: d.serial }));
236+
237+
if (candidates.length === 0) {
238+
return failResult(
239+
platform === 'ios' ? 'No iOS simulator booted.' : 'No Android device connected.',
240+
{ code: 'NO_DEVICE' },
241+
);
242+
}
243+
244+
const resolution = resolveTargetDevice(candidates, args.deviceId);
245+
if (!resolution.ok) {
246+
const list = resolution.candidates
247+
.map((c) => ` - ${c.id}${c.label ? ` (${c.label})` : ''}`)
248+
.join('\n');
249+
const argName = platform === 'ios' ? 'UDID' : 'serial';
250+
return failResult(
251+
`device_record: ${resolution.candidates.length} ${platform} ${argName === 'UDID' ? 'simulators booted' : 'devices connected'} — refusing to auto-pick to avoid recording the wrong device. ` +
252+
`Pass deviceId=<${argName}> to disambiguate:\n${list}`,
253+
{ code: 'DEVICE_AMBIGUOUS', platform, candidates: resolution.candidates },
254+
);
255+
}
256+
257+
const scriptArgs = ['start', platform, outputPath];
258+
// Only forward an explicit id when we're picking from >1 candidate; the
259+
// single-device case keeps the script's existing `booted`/auto path so
260+
// we don't regress any environment where simctl's `booted` shorthand
261+
// works differently than passing the literal UDID (defensive — both
262+
// should be equivalent on Apple's side).
263+
if (!resolution.autoSelected) {
264+
scriptArgs.push(platform === 'ios' ? '--udid' : '--serial', resolution.deviceId);
265+
}
266+
100267
try {
101268
const { stdout } = await execFileAsync(
102269
getRecordScript(),
103-
['start', platform, outputPath],
270+
scriptArgs,
104271
{ timeout: START_TIMEOUT_MS },
105272
);
106273
const parsed = parseStartOutput(stdout);
@@ -110,6 +277,8 @@ async function runStart(args: DeviceRecordArgs): Promise<ToolResult> {
110277
return okResult({
111278
action: 'start',
112279
platform,
280+
deviceId: resolution.deviceId,
281+
autoSelected: resolution.autoSelected,
113282
output: parsed.output,
114283
pid: parsed.pid,
115284
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.',

0 commit comments

Comments
 (0)