Skip to content

Commit 16ab7d6

Browse files
committed
fix: B116/B117/B118 — multi-platform correctness follow-ups
Three benchmark-surfaced bugs from Phase 88 Round 2 Android, all fixed in this batch with unit tests. 213 -> 225 tests passing. B118/D637 — platform-aware CDP timeouts Android emulator JS thread is 50-170× slower than iOS simulator for Runtime.evaluate. A single 5s CDP_TIMEOUT_MS constant produced false-negatives on typeText (timed out AFTER the mutation succeeded). Introduced Platform type + defaultTimeout(platform) + timeoutForMethod(method, platform) in cdp/timeout-config.ts. Android gets 2×; iOS unchanged. CDPClient routes both evaluate paths and sendWithTimeout fallbacks through the new helpers using this._connectedTarget?.platform. 10 new unit tests. B117/D638 — device_* tools inherit platform from CDP target runAgentDevice(cliArgs, opts) now accepts optional platform; appends --platform <p> to agent-device CLI when no session is open. Sessions still carry device identity so session-happy path unchanged. createDeviceScreenshotHandler accepts optional getClient and derives platform from client.connectedTarget?.platform with an explicit args.platform override. Added `platform` to device_screenshot tool schema. Back-compat preserved (existing zero-arg call still works). 3 new unit tests. B116/D639 — simctl listapps cross-check in platform inference inferPlatforms now reads BOTH adb pm list AND xcrun simctl listapps booted (via parseSimctlListapps, which extracts top-level bundle IDs from the NeXTSTEP plist format, skipping nested GroupContainers). Logic: Android-only→android, iOS-only→ios, both→ios+ambiguousPlatform=true, neither→ios. Readers are injectable for unit testing without subprocesses. HermesTarget gains ambiguousPlatform flag. 9 new unit tests. All 225 tests pass; TypeScript clean; dist/ rebuilt.
1 parent 8434f8e commit 16ab7d6

16 files changed

Lines changed: 452 additions & 43 deletions

scripts/cdp-bridge/dist/agent-device-wrapper.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,13 @@ export async function runAgentDevice(cliArgs, opts = {}) {
378378
if (sessionName) {
379379
args.push('--session', sessionName);
380380
}
381+
else if (opts.platform) {
382+
// B117/D638: when no session is open but a platform hint is provided (e.g. from
383+
// CDPClient.connectedTarget.platform), pass --platform so agent-device doesn't
384+
// default to whichever booted device it finds first. Avoids wrong-device
385+
// screenshots when both iOS sim and Android emulator are booted.
386+
args.push('--platform', opts.platform);
387+
}
381388
try {
382389
const { stdout } = await execFile('agent-device', args, {
383390
timeout: EXEC_TIMEOUT,

scripts/cdp-bridge/dist/cdp-client.js

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { detectBridge } from './bridge-detector.js';
44
import { logger } from './logger.js';
55
import { performSetup, reinjectHelpers as reinjectHelpersFn } from './cdp/setup.js';
66
import { resetState, setActiveFlag, clearActiveFlag, sleep } from './cdp/state.js';
7-
import { CDP_TIMEOUT_MS, timeoutForMethod } from './cdp/timeout-config.js';
7+
import { defaultTimeout, timeoutForMethod } from './cdp/timeout-config.js';
88
import { sendWithTimeout as sendMsg, rejectAllPending as rejectPending, handleMessage as handleMsg } from './cdp/transport.js';
99
import { wireEventHandlers, parseNetworkHookMessage as parseNetHook } from './cdp/event-handlers.js';
1010
import { discoverForList } from './cdp/discovery.js';
@@ -114,14 +114,18 @@ export class CDPClient {
114114
}
115115
this.rejectAllPending(new Error('Client disconnected'));
116116
}
117+
get effectivePlatform() {
118+
return this._connectedTarget?.platform ?? null;
119+
}
117120
async evaluate(expression, awaitPromise = false) {
118121
if (awaitPromise) {
119122
return this.evaluateAsync(expression);
120123
}
124+
const timeout = defaultTimeout(this.effectivePlatform);
121125
const result = await this.sendWithTimeout('Runtime.evaluate', {
122126
expression,
123127
returnByValue: true,
124-
}, CDP_TIMEOUT_MS);
128+
}, timeout);
125129
if (result?.exceptionDetails) {
126130
return {
127131
error: result.exceptionDetails.text ??
@@ -135,8 +139,9 @@ export class CDPClient {
135139
// Hermes CDP doesn't support awaitPromise — use global slot + polling
136140
// Values are JSON-serialized inside Hermes to handle non-serializable objects
137141
// A deferred cleanup timer ensures the slot is removed even if the caller times out
142+
const timeout = defaultTimeout(this.effectivePlatform);
138143
const slot = '__rn_agent_async_' + (++this.slotId) + '_' + Date.now();
139-
const ASYNC_CLEANUP_MS = CDP_TIMEOUT_MS * 2;
144+
const ASYNC_CLEANUP_MS = timeout * 2;
140145
const wrapper = `(function() {
141146
function safeVal(v) {
142147
try { return JSON.stringify(v); } catch(e) { return JSON.stringify(String(v)); }
@@ -153,17 +158,17 @@ export class CDPClient {
153158
const initResult = await this.sendWithTimeout('Runtime.evaluate', {
154159
expression: wrapper,
155160
returnByValue: true,
156-
}, CDP_TIMEOUT_MS);
161+
}, timeout);
157162
if (initResult?.exceptionDetails) {
158163
return {
159164
error: initResult.exceptionDetails.text ??
160165
initResult.exceptionDetails.exception?.description ??
161166
'Unknown evaluation error',
162167
};
163168
}
164-
// B45 fix: Use absolute deadline to guarantee total wall-clock stays within CDP_TIMEOUT_MS.
169+
// B45 fix: Use absolute deadline to guarantee total wall-clock stays within timeout.
165170
// Each poll gets only the remaining time (min 500ms) to avoid overshooting.
166-
const deadline = Date.now() + CDP_TIMEOUT_MS;
171+
const deadline = Date.now() + timeout;
167172
while (Date.now() < deadline) {
168173
const remaining = deadline - Date.now();
169174
if (remaining < 500)
@@ -194,10 +199,10 @@ export class CDPClient {
194199
expression: `delete globalThis['${slot}']`,
195200
returnByValue: true,
196201
}, 1000).catch(() => { });
197-
return { error: 'Promise did not resolve within ' + CDP_TIMEOUT_MS + 'ms' };
202+
return { error: 'Promise did not resolve within ' + timeout + 'ms' };
198203
}
199204
async send(method, params) {
200-
return this.sendWithTimeout(method, params, timeoutForMethod(method));
205+
return this.sendWithTimeout(method, params, timeoutForMethod(method, this.effectivePlatform));
201206
}
202207
handleMessage(data) {
203208
handleMsg(data, this.pending, this.eventHandlers, (params) => this.parseNetworkHookMessage(params));
@@ -207,7 +212,7 @@ export class CDPClient {
207212
}
208213
async setup() {
209214
const result = await performSetup({
210-
send: (method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method)),
215+
send: (method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method, this.effectivePlatform)),
211216
evaluate: (expr) => this.evaluate(expr),
212217
port: this._port,
213218
connectedTarget: this._connectedTarget,
@@ -226,7 +231,7 @@ export class CDPClient {
226231
}
227232
}
228233
setupEventHandlers() {
229-
wireEventHandlers(this.eventHandlers, { console: this._consoleBuffer, network: this._networkBuffer, log: this._logBuffer, scripts: this._scripts }, (method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method)), () => this._isPaused, (v) => { this._isPaused = v; });
234+
wireEventHandlers(this.eventHandlers, { console: this._consoleBuffer, network: this._networkBuffer, log: this._logBuffer, scripts: this._scripts }, (method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method, this.effectivePlatform)), () => this._isPaused, (v) => { this._isPaused = v; });
230235
}
231236
handleClose(code) {
232237
handleCloseFn(this.buildReconnectCtx(), code);

scripts/cdp-bridge/dist/cdp/discovery.js

Lines changed: 62 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,26 +51,83 @@ export function filterValidTargets(targets) {
5151
?.replace(/\[::\]/g, '127.0.0.1'),
5252
}));
5353
}
54-
export function inferPlatforms(targets) {
55-
let androidPackages = null;
54+
/**
55+
* B116 (D639): extract top-level bundle IDs from `xcrun simctl listapps booted`.
56+
* Output is NeXTSTEP plist; top-level keys are quoted bundle IDs at exactly
57+
* 4-space indentation, e.g. ` "com.foo.bar" = {`. We match that pattern
58+
* explicitly so we don't pick up nested keys like GroupContainers entries.
59+
*/
60+
export function parseSimctlListapps(stdout) {
61+
const ids = new Set();
62+
const TOP_LEVEL = /^ "([A-Za-z0-9._-]+)"\s*=\s*\{/;
63+
for (const line of stdout.split('\n')) {
64+
const m = line.match(TOP_LEVEL);
65+
if (m)
66+
ids.add(m[1]);
67+
}
68+
return ids;
69+
}
70+
function readAndroidPackages() {
5671
try {
5772
const out = execFileSync('adb', ['shell', 'pm', 'list', 'packages'], {
5873
timeout: 3000,
5974
encoding: 'utf8',
6075
stdio: ['ignore', 'pipe', 'ignore'],
6176
});
62-
androidPackages = new Set(out.split('\n')
77+
return new Set(out.split('\n')
6378
.map(line => line.replace('package:', '').trim())
6479
.filter(Boolean));
6580
}
6681
catch {
67-
// adb not available or no device — all targets treated as iOS
82+
return null;
83+
}
84+
}
85+
function readIOSPackages() {
86+
try {
87+
const out = execFileSync('xcrun', ['simctl', 'listapps', 'booted'], {
88+
timeout: 5000,
89+
encoding: 'utf8',
90+
stdio: ['ignore', 'pipe', 'ignore'],
91+
});
92+
return parseSimctlListapps(out);
6893
}
94+
catch {
95+
return null;
96+
}
97+
}
98+
/**
99+
* B116 (D639): look up each target's description against BOTH iOS simctl and
100+
* Android adb installed-package sets. If present in only one, tag that
101+
* platform. If present in both (same bundleId installed on both devices),
102+
* we can't disambiguate from bundleId alone — leave the target ambiguous
103+
* (callers must pass `targetId` or `bundleId` + `platform`).
104+
* If present in neither (or lookup failed), fall back to iOS (matches prior
105+
* behavior — iOS-first bias).
106+
*
107+
* Readers are injectable for unit testing without spawning subprocesses.
108+
*/
109+
export function inferPlatforms(targets, readers = {}) {
110+
const androidPackages = (readers.readAndroid ?? readAndroidPackages)();
111+
const iosPackages = (readers.readIOS ?? readIOSPackages)();
69112
for (const t of targets) {
70-
if (androidPackages?.has(t.description ?? '')) {
113+
const desc = t.description ?? '';
114+
const inAndroid = androidPackages?.has(desc) ?? false;
115+
const inIOS = iosPackages?.has(desc) ?? false;
116+
if (inAndroid && !inIOS) {
71117
t.platform = 'android';
72118
}
119+
else if (inIOS && !inAndroid) {
120+
t.platform = 'ios';
121+
}
122+
else if (inAndroid && inIOS) {
123+
// Ambiguous — same bundleId installed on both. Default to iOS but mark
124+
// for downstream so callers can notice and pass targetId/bundleId filter.
125+
t.platform = 'ios';
126+
t.ambiguousPlatform = true;
127+
}
73128
else {
129+
// No information (adb/simctl both failed, or target bundle unknown) —
130+
// default to iOS to preserve prior behavior for iOS-only setups.
74131
t.platform = 'ios';
75132
}
76133
}

scripts/cdp-bridge/dist/cdp/timeout-config.js

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
export const CDP_TIMEOUT_FAST = 1500;
22
export const CDP_TIMEOUT_MS = 5000;
33
export const CDP_TIMEOUT_SLOW = 30000;
4+
// D637/B118: Android emulator JS thread runs Hermes operations 50-170× slower
5+
// than iOS simulator (Phase 88 benchmark). A single 5s default gave false-negatives
6+
// on typeText / complex store reads — the mutation succeeded but the Runtime.evaluate
7+
// round-trip missed the timeout. 2× multiplier for Android puts p95 back inside budget.
8+
const ANDROID_MULTIPLIER = 2;
49
const METHOD_TIMEOUTS = new Map([
510
['Runtime.getHeapUsage', CDP_TIMEOUT_FAST],
611
['Log.enable', CDP_TIMEOUT_FAST],
@@ -11,6 +16,13 @@ const METHOD_TIMEOUTS = new Map([
1116
['Profiler.stop', CDP_TIMEOUT_SLOW],
1217
['Network.getResponseBody', CDP_TIMEOUT_SLOW],
1318
]);
14-
export function timeoutForMethod(method) {
15-
return METHOD_TIMEOUTS.get(method) ?? CDP_TIMEOUT_MS;
19+
function applyPlatformMultiplier(base, platform) {
20+
return platform === 'android' ? base * ANDROID_MULTIPLIER : base;
21+
}
22+
export function timeoutForMethod(method, platform) {
23+
const base = METHOD_TIMEOUTS.get(method) ?? CDP_TIMEOUT_MS;
24+
return applyPlatformMultiplier(base, platform);
25+
}
26+
export function defaultTimeout(platform) {
27+
return applyPlatformMultiplier(CDP_TIMEOUT_MS, platform);
1628
}

scripts/cdp-bridge/dist/index.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,10 +277,11 @@ trackedTool('collect_logs', 'Collect logs from multiple sources in parallel: JS
277277
}, createCollectLogsHandler(getClient));
278278
// --- agent-device tools (native device interaction) ---
279279
trackedTool('device_list', 'List all available iOS simulators and Android emulators. Returns device name, UDID, platform, and status. Use before device_snapshot action=open to confirm the target device.', {}, createDeviceListHandler());
280-
trackedTool('device_screenshot', 'Capture a screenshot of the active device screen. Returns image data or file path. Prefer JPEG for faster capture.', {
280+
trackedTool('device_screenshot', 'Capture a screenshot of the active device screen. Returns image data or file path. Prefer JPEG for faster capture. When both iOS sim and Android emulator are booted, defaults to the platform of the currently connected CDP target; override with `platform` if needed.', {
281281
path: z.string().optional().describe('Output file path (default: auto-generated in /tmp). Use .jpg extension for JPEG.'),
282282
format: z.enum(['jpeg', 'png']).optional().describe('Image format (default: auto-detect from path extension, or jpeg)'),
283-
}, createDeviceScreenshotHandler());
283+
platform: z.enum(['ios', 'android']).optional().describe('Target device platform. Defaults to the currently-connected CDP target platform.'),
284+
}, createDeviceScreenshotHandler(getClient));
284285
trackedTool('device_snapshot', 'Manage device sessions and capture UI snapshots. action=open starts a session (required before other device_ tools). action=snapshot returns the accessibility tree with @ref identifiers for device_press/device_fill. action=close ends the session.', {
285286
action: z.enum(['open', 'close', 'snapshot']).default('snapshot').describe('open: start session for an app. snapshot: capture UI tree with element refs. close: end session.'),
286287
appId: z.string().optional().describe('App bundle ID — required for action=open (e.g. "com.example.app")'),

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ export function buildScreenshotArgs(args, now = Date.now) {
1818
}
1919
return ['screenshot', '--out', outputPath];
2020
}
21-
export function createDeviceScreenshotHandler() {
22-
return async (args) => runAgentDevice(buildScreenshotArgs(args));
21+
/**
22+
* B117/D638: device_screenshot now accepts an optional `platform` and, when not
23+
* provided, falls back to the current CDP target's platform. Prevents
24+
* wrong-device screenshots when both iOS sim and Android emulator are booted.
25+
* getClient is optional so existing callers/tests still compile.
26+
*/
27+
export function createDeviceScreenshotHandler(getClient) {
28+
return async (args) => {
29+
const platform = args.platform ?? getClient?.()?.connectedTarget?.platform ?? null;
30+
return runAgentDevice(buildScreenshotArgs(args), { platform });
31+
};
2332
}

scripts/cdp-bridge/src/agent-device-wrapper.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,7 @@ function cacheRefMapFromResult(result: ToolResult): void {
368368

369369
export async function runAgentDevice(
370370
cliArgs: string[],
371-
opts: { skipSession?: boolean } = {},
371+
opts: { skipSession?: boolean; platform?: 'ios' | 'android' | null } = {},
372372
): Promise<ToolResult> {
373373
const sessionName = (!opts.skipSession && activeSession) ? activeSession.name : '';
374374
const isSnapshotCmd = cliArgs[0] === 'snapshot';
@@ -402,6 +402,12 @@ export async function runAgentDevice(
402402
const args = [...cliArgs, '--json'];
403403
if (sessionName) {
404404
args.push('--session', sessionName);
405+
} else if (opts.platform) {
406+
// B117/D638: when no session is open but a platform hint is provided (e.g. from
407+
// CDPClient.connectedTarget.platform), pass --platform so agent-device doesn't
408+
// default to whichever booted device it finds first. Avoids wrong-device
409+
// screenshots when both iOS sim and Android emulator are booted.
410+
args.push('--platform', opts.platform);
405411
}
406412

407413
try {

scripts/cdp-bridge/src/cdp-client.ts

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ import { logger } from './logger.js';
55
import { performSetup, reinjectHelpers as reinjectHelpersFn } from './cdp/setup.js';
66
import { resetState, setActiveFlag, clearActiveFlag, sleep } from './cdp/state.js';
77
import type { ResettableState } from './cdp/state.js';
8-
import { CDP_TIMEOUT_MS, timeoutForMethod } from './cdp/timeout-config.js';
8+
import { defaultTimeout, timeoutForMethod } from './cdp/timeout-config.js';
9+
import type { Platform } from './cdp/timeout-config.js';
910
import { sendWithTimeout as sendMsg, rejectAllPending as rejectPending, handleMessage as handleMsg } from './cdp/transport.js';
1011
import { wireEventHandlers, parseNetworkHookMessage as parseNetHook } from './cdp/event-handlers.js';
1112
import { discover, discoverForList } from './cdp/discovery.js';
@@ -154,15 +155,20 @@ export class CDPClient {
154155
this.rejectAllPending(new Error('Client disconnected'));
155156
}
156157

158+
private get effectivePlatform(): Platform {
159+
return this._connectedTarget?.platform ?? null;
160+
}
161+
157162
async evaluate(expression: string, awaitPromise = false): Promise<EvaluateResult> {
158163
if (awaitPromise) {
159164
return this.evaluateAsync(expression);
160165
}
161166

167+
const timeout = defaultTimeout(this.effectivePlatform);
162168
const result = await this.sendWithTimeout('Runtime.evaluate', {
163169
expression,
164170
returnByValue: true,
165-
}, CDP_TIMEOUT_MS) as { result?: { value?: unknown }; exceptionDetails?: { text?: string; exception?: { description?: string } } };
171+
}, timeout) as { result?: { value?: unknown }; exceptionDetails?: { text?: string; exception?: { description?: string } } };
166172

167173
if (result?.exceptionDetails) {
168174
return {
@@ -178,8 +184,9 @@ export class CDPClient {
178184
// Hermes CDP doesn't support awaitPromise — use global slot + polling
179185
// Values are JSON-serialized inside Hermes to handle non-serializable objects
180186
// A deferred cleanup timer ensures the slot is removed even if the caller times out
187+
const timeout = defaultTimeout(this.effectivePlatform);
181188
const slot = '__rn_agent_async_' + (++this.slotId) + '_' + Date.now();
182-
const ASYNC_CLEANUP_MS = CDP_TIMEOUT_MS * 2;
189+
const ASYNC_CLEANUP_MS = timeout * 2;
183190
const wrapper = `(function() {
184191
function safeVal(v) {
185192
try { return JSON.stringify(v); } catch(e) { return JSON.stringify(String(v)); }
@@ -197,7 +204,7 @@ export class CDPClient {
197204
const initResult = await this.sendWithTimeout('Runtime.evaluate', {
198205
expression: wrapper,
199206
returnByValue: true,
200-
}, CDP_TIMEOUT_MS) as { exceptionDetails?: { text?: string; exception?: { description?: string } } };
207+
}, timeout) as { exceptionDetails?: { text?: string; exception?: { description?: string } } };
201208

202209
if (initResult?.exceptionDetails) {
203210
return {
@@ -207,9 +214,9 @@ export class CDPClient {
207214
};
208215
}
209216

210-
// B45 fix: Use absolute deadline to guarantee total wall-clock stays within CDP_TIMEOUT_MS.
217+
// B45 fix: Use absolute deadline to guarantee total wall-clock stays within timeout.
211218
// Each poll gets only the remaining time (min 500ms) to avoid overshooting.
212-
const deadline = Date.now() + CDP_TIMEOUT_MS;
219+
const deadline = Date.now() + timeout;
213220
while (Date.now() < deadline) {
214221
const remaining = deadline - Date.now();
215222
if (remaining < 500) break;
@@ -241,11 +248,11 @@ export class CDPClient {
241248
expression: `delete globalThis['${slot}']`,
242249
returnByValue: true,
243250
}, 1000).catch(() => {});
244-
return { error: 'Promise did not resolve within ' + CDP_TIMEOUT_MS + 'ms' };
251+
return { error: 'Promise did not resolve within ' + timeout + 'ms' };
245252
}
246253

247254
async send(method: string, params?: unknown): Promise<unknown> {
248-
return this.sendWithTimeout(method, params, timeoutForMethod(method));
255+
return this.sendWithTimeout(method, params, timeoutForMethod(method, this.effectivePlatform));
249256
}
250257

251258
private handleMessage(data: WebSocket.RawData): void {
@@ -258,7 +265,7 @@ export class CDPClient {
258265

259266
private async setup(): Promise<void> {
260267
const result = await performSetup({
261-
send: (method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method)),
268+
send: (method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method, this.effectivePlatform)),
262269
evaluate: (expr) => this.evaluate(expr),
263270
port: this._port,
264271
connectedTarget: this._connectedTarget,
@@ -281,7 +288,7 @@ export class CDPClient {
281288
wireEventHandlers(
282289
this.eventHandlers,
283290
{ console: this._consoleBuffer, network: this._networkBuffer, log: this._logBuffer, scripts: this._scripts },
284-
(method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method)),
291+
(method, params, ms) => this.sendWithTimeout(method, params, ms ?? timeoutForMethod(method, this.effectivePlatform)),
285292
() => this._isPaused,
286293
(v) => { this._isPaused = v; },
287294
);

0 commit comments

Comments
 (0)