Skip to content

Commit 7497419

Browse files
thymikeethymikee
committed
fix(daemon): bound and catch the hint probe, pin the printed command
Two precision fixes from review: - The probe was best-effort in spirit but not in fact: allowFailure only suppresses a non-zero exit code, not a timeout or spawn-failure rejection (exec.ts always rejects on those). listSimulatorApps' listapps call had no timeout at all, so it could hang the error path indefinitely; the launchctl probe had no catch, so a rejection would propagate and replace the deterministic SESSION_NOT_FOUND with a probe failure. Threaded an optional timeoutMs through listSimulatorApps/listSimulatorAppMetadata (including its plutil fallback) and wrapped both detectSoleRunningIosSimulatorApp and buildIosOpenCommandHint in try/catch so any throw falls back to the generic hint, never propagates. - The printed command now always pins --udid (the sole-booted-device check only proves uniqueness within the probed device's own simulator set, not that a fresh CLI invocation without --ios-simulator-device-set would resolve to the same device) and echoes --ios-simulator-device-set when the detected device carries one, so the hint is the exact command that was live-validated, not a shorter one that happens to work by coincidence. Added a length guard: wire-level error details are redacted before send and silently truncate any string over 400 chars, so a hint that would exceed a safe budget (a long device-set path pushes it there) now falls back instead of printing a truncated, non-copy-pasteable command. Also: "in the foreground" overclaimed what the probe verifies (a resident but backgrounded app would match the same running-process check) — reworded to "running".
1 parent d0b8157 commit 7497419

5 files changed

Lines changed: 177 additions & 29 deletions

File tree

‎src/daemon/handlers/__tests__/snapshot-handler.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ test('snapshot on iOS rejects sessions without a tracked app', async () => {
383383
test('snapshot on iOS without a tracked app carries the detected open command as its hint', async () => {
384384
mockBuildIosOpenCommandHint.mockResolvedValue(
385385
'One booted device found ("My iPhone Simulator", udid sim-1) with xyz.blueskyweb.app ' +
386-
'running. Run: agent-device open xyz.blueskyweb.app --platform ios',
386+
'running. Run: agent-device open xyz.blueskyweb.app --platform ios --udid sim-1',
387387
);
388388
const sessionStore = makeSessionStore();
389389
const sessionName = 'ios-sim-no-app-hinted';
@@ -407,7 +407,7 @@ test('snapshot on iOS without a tracked app carries the detected open command as
407407
expect(response.error.code).toBe('SESSION_NOT_FOUND');
408408
expect(response.error.details?.hint).toBe(
409409
'One booted device found ("My iPhone Simulator", udid sim-1) with xyz.blueskyweb.app ' +
410-
'running. Run: agent-device open xyz.blueskyweb.app --platform ios',
410+
'running. Run: agent-device open xyz.blueskyweb.app --platform ios --udid sim-1',
411411
);
412412
}
413413
expect(mockBuildIosOpenCommandHint).toHaveBeenCalledWith(iosSimulatorDevice);

‎src/daemon/ios-app-session-hint.test.ts‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,66 @@ test('an unambiguous environment gets the exact runnable open command', async ()
2626

2727
expect(hint).toBe(
2828
'One booted device found ("iPhone 16", udid booted-1) with xyz.blueskyweb.app running. ' +
29-
'Run: agent-device open xyz.blueskyweb.app --platform ios',
29+
'Run: agent-device open xyz.blueskyweb.app --platform ios --udid booted-1',
3030
);
3131
expect(detectSoleRunningIosSimulatorApp).toHaveBeenCalledWith(soleBootedDevice);
3232
});
3333

34+
test('a custom simulator set is echoed back so the printed command is the one that was run', async () => {
35+
const soleBootedDevice = {
36+
...IOS_SIMULATOR,
37+
id: 'booted-1',
38+
name: 'iPhone 16',
39+
simulatorSetPath: '/tmp/agent-device sim set',
40+
};
41+
listBootedIosSimulators.mockResolvedValue([soleBootedDevice]);
42+
detectSoleRunningIosSimulatorApp.mockResolvedValue({
43+
bundleId: 'xyz.blueskyweb.app',
44+
name: 'Bluesky',
45+
});
46+
47+
const hint = await buildIosOpenCommandHint(soleBootedDevice);
48+
49+
expect(hint).toBe(
50+
'One booted device found ("iPhone 16", udid booted-1) with xyz.blueskyweb.app running. ' +
51+
'Run: agent-device open xyz.blueskyweb.app --platform ios --udid booted-1 ' +
52+
"--ios-simulator-device-set '/tmp/agent-device sim set'",
53+
);
54+
});
55+
56+
test('a hint that would exceed the wire-redaction truncation length falls back to undefined', async () => {
57+
// A truncated mid-command hint (see redactDiagnosticData in
58+
// packages/kernel/src/redaction.ts, which silently truncates any details
59+
// string over 400 chars) is worse than the generic default — it looks
60+
// actionable but isn't copy-paste-safe.
61+
const soleBootedDevice = {
62+
...IOS_SIMULATOR,
63+
id: 'booted-1',
64+
name: 'iPhone 16',
65+
simulatorSetPath: `/very/long/simulator/set/path/${'segment/'.repeat(30)}`,
66+
};
67+
listBootedIosSimulators.mockResolvedValue([soleBootedDevice]);
68+
detectSoleRunningIosSimulatorApp.mockResolvedValue({
69+
bundleId: 'xyz.blueskyweb.app',
70+
name: 'Bluesky',
71+
});
72+
73+
await expect(buildIosOpenCommandHint(soleBootedDevice)).resolves.toBeUndefined();
74+
});
75+
76+
test('a rejecting probe (timeout or spawn failure) is caught, not propagated', async () => {
77+
listBootedIosSimulators.mockRejectedValue(new Error('simctl timed out'));
78+
79+
await expect(buildIosOpenCommandHint(IOS_SIMULATOR)).resolves.toBeUndefined();
80+
});
81+
82+
test('a rejecting foreground-app probe is caught, not propagated', async () => {
83+
listBootedIosSimulators.mockResolvedValue([{ ...IOS_SIMULATOR, id: 'booted-1' }]);
84+
detectSoleRunningIosSimulatorApp.mockRejectedValue(new Error('launchctl timed out'));
85+
86+
await expect(buildIosOpenCommandHint(IOS_SIMULATOR)).resolves.toBeUndefined();
87+
});
88+
3489
test('more than one booted simulator stays ambiguous and returns no hint', async () => {
3590
listBootedIosSimulators.mockResolvedValue([
3691
{ ...IOS_SIMULATOR, id: 'booted-1' },

‎src/daemon/ios-app-session-hint.ts‎

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { isIosFamily, type DeviceInfo } from '@agent-device/kernel/device';
22
import { detectSoleRunningIosSimulatorApp } from '../platforms/apple/core/app-resolution.ts';
33
import { listBootedIosSimulators } from '../platforms/apple/core/devices.ts';
4+
import { shellQuoteIfNeeded } from '../utils/shell-quote.ts';
45

56
/**
67
* Enriches the generic "Run open first" SESSION_NOT_FOUND hint with the exact
@@ -11,20 +12,52 @@ import { listBootedIosSimulators } from '../platforms/apple/core/devices.ts';
1112
* than one running app, or a probe failure — returns `undefined` so the
1213
* caller keeps the generic hint. This must never guess: a wrong-but-confident
1314
* command is worse than the default guidance.
15+
*
16+
* The whole probe is strictly best-effort: it runs on an error path, so a
17+
* throw here (a bounded probe still rejects on timeout or a spawn failure —
18+
* see app-resolution.ts) must fall back to `undefined`, never replace the
19+
* caller's deterministic error with a probe failure.
1420
*/
21+
// Wire-level details are redacted before send (packages/kernel/src/redaction.ts),
22+
// which silently truncates any string field over 400 chars — a truncated
23+
// mid-command hint is a worse failure mode than the generic default (it looks
24+
// actionable but isn't). A long --ios-simulator-device-set path (the
25+
// bench-clone case) can push the composed hint past that, so this stays
26+
// comfortably under it and falls back rather than risk truncation.
27+
const MAX_HINT_LENGTH = 350;
28+
1529
export async function buildIosOpenCommandHint(device: DeviceInfo): Promise<string | undefined> {
1630
if (!isIosFamily(device) || device.kind !== 'simulator') return undefined;
1731

18-
const booted = await listBootedIosSimulators({ simulatorSetPath: device.simulatorSetPath });
19-
if (booted.length !== 1) return undefined;
20-
const [soleBootedDevice] = booted;
21-
if (!soleBootedDevice) return undefined;
32+
try {
33+
const booted = await listBootedIosSimulators({ simulatorSetPath: device.simulatorSetPath });
34+
if (booted.length !== 1) return undefined;
35+
const [soleBootedDevice] = booted;
36+
if (!soleBootedDevice) return undefined;
2237

23-
const app = await detectSoleRunningIosSimulatorApp(soleBootedDevice);
24-
if (!app) return undefined;
38+
const app = await detectSoleRunningIosSimulatorApp(soleBootedDevice);
39+
if (!app) return undefined;
40+
41+
const command = buildOpenCommand(soleBootedDevice, app.bundleId);
42+
const hint =
43+
`One booted device found ("${soleBootedDevice.name}", udid ${soleBootedDevice.id}) with ` +
44+
`${app.bundleId} running. Run: ${command}`;
45+
return hint.length <= MAX_HINT_LENGTH ? hint : undefined;
46+
} catch {
47+
return undefined;
48+
}
49+
}
2550

26-
return (
27-
`One booted device found ("${soleBootedDevice.name}", udid ${soleBootedDevice.id}) with ` +
28-
`${app.bundleId} running. Run: agent-device open ${app.bundleId} --platform ios`
29-
);
51+
// Always pins --udid: the sole-booted-device check only proves there is one
52+
// booted simulator within `device`'s own simulator set, not that a fresh CLI
53+
// invocation (no --ios-simulator-device-set) would land on it too — a custom
54+
// device set (the bench-clone case) is otherwise invisible to the default
55+
// set. --ios-simulator-device-set is echoed back whenever the detected
56+
// device carries one, so the printed command is the actual command that was
57+
// live-validated, not a shorter one that happens to work by coincidence.
58+
function buildOpenCommand(device: DeviceInfo, bundleId: string): string {
59+
const deviceSetFlag = device.simulatorSetPath
60+
? ` --ios-simulator-device-set ${shellQuoteIfNeeded(device.simulatorSetPath)}`
61+
: '';
62+
return `agent-device open ${bundleId} --platform ios --udid ${device.id}${deviceSetFlag}`;
3063
}

‎src/platforms/apple/core/__tests__/app-resolution.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,42 @@ test('detectSoleRunningIosSimulatorApp treats a failed probe as inconclusive', a
109109
assert.equal(await detectSoleRunningIosSimulatorApp(bootedSimulator), undefined);
110110
});
111111

112+
test('detectSoleRunningIosSimulatorApp catches a rejecting launchctl probe (timeout/spawn failure)', async () => {
113+
mockRunSimctl.mockImplementation(async (_device: DeviceInfo, args: string[]) => {
114+
if (args[0] === 'spawn') throw new Error('xcrun timed out after 3000ms');
115+
return {
116+
stdout: JSON.stringify({ 'com.apple.Preferences': { CFBundleDisplayName: 'Settings' } }),
117+
};
118+
});
119+
120+
assert.equal(await detectSoleRunningIosSimulatorApp(bootedSimulator), undefined);
121+
});
122+
123+
test('detectSoleRunningIosSimulatorApp catches a rejecting listapps probe (timeout/spawn failure)', async () => {
124+
mockRunSimctl.mockImplementation(async (_device: DeviceInfo, args: string[]) => {
125+
if (args[0] === 'spawn') {
126+
return {
127+
exitCode: 0,
128+
stdout:
129+
'PID\tStatus\tLabel\n1000\t0\tUIKitApplication:com.apple.Preferences[abcd][rb-legacy]',
130+
};
131+
}
132+
throw new Error('xcrun timed out after 3000ms');
133+
});
134+
135+
assert.equal(await detectSoleRunningIosSimulatorApp(bootedSimulator), undefined);
136+
});
137+
138+
test('detectSoleRunningIosSimulatorApp bounds the listapps probe with a timeout', async () => {
139+
mockLaunchctlAndListapps(['com.apple.Preferences']);
140+
141+
await detectSoleRunningIosSimulatorApp(bootedSimulator);
142+
143+
const listappsCall = mockRunSimctl.mock.calls.find(([, args]) => args[0] === 'listapps');
144+
assert.equal(typeof listappsCall?.[2]?.timeoutMs, 'number');
145+
assert.ok((listappsCall?.[2]?.timeoutMs ?? 0) > 0);
146+
});
147+
112148
test('detectSoleRunningIosSimulatorApp does not probe a stopped simulator', async () => {
113149
assert.equal(
114150
await detectSoleRunningIosSimulatorApp({ ...bootedSimulator, booted: false }),

‎src/platforms/apple/core/app-resolution.ts‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -124,19 +124,28 @@ export async function detectSoleRunningIosSimulatorApp(
124124
return undefined;
125125
}
126126

127-
const [runningBundleIds, installedApps] = await Promise.all([
128-
listRunningIosSimulatorBundleIds(device),
129-
listSimulatorApps(device),
130-
]);
131-
if (runningBundleIds.length === 0) return undefined;
132-
133-
const installedById = new Map(installedApps.map((app) => [app.bundleId, app] as const));
134-
const candidates = new Map<string, IosAppInfo>();
135-
for (const bundleId of runningBundleIds) {
136-
const app = installedById.get(bundleId);
137-
if (app) candidates.set(app.bundleId, app);
127+
// Both probes are bounded, but a timeout still REJECTS (exec.ts's
128+
// `allowFailure` only suppresses non-zero exit codes, not a timeout or a
129+
// spawn failure) — this is an error-path enrichment, so any throw here
130+
// must fall back to "inconclusive", never propagate and replace the
131+
// caller's deterministic error with a probe failure.
132+
try {
133+
const [runningBundleIds, installedApps] = await Promise.all([
134+
listRunningIosSimulatorBundleIds(device),
135+
listSimulatorApps(device, { timeoutMs: IOS_FOREGROUND_APP_PROBE_TIMEOUT_MS }),
136+
]);
137+
if (runningBundleIds.length === 0) return undefined;
138+
139+
const installedById = new Map(installedApps.map((app) => [app.bundleId, app] as const));
140+
const candidates = new Map<string, IosAppInfo>();
141+
for (const bundleId of runningBundleIds) {
142+
const app = installedById.get(bundleId);
143+
if (app) candidates.set(app.bundleId, app);
144+
}
145+
return candidates.size === 1 ? [...candidates.values()][0] : undefined;
146+
} catch {
147+
return undefined;
138148
}
139-
return candidates.size === 1 ? [...candidates.values()][0] : undefined;
140149
}
141150

142151
async function listRunningIosSimulatorBundleIds(device: DeviceInfo): Promise<string[]> {
@@ -202,16 +211,30 @@ export async function listIosApps(device: DeviceInfo, filter: AppsFilter): Promi
202211
return await resolveIosPhysicalDeviceControl(device).listApps(device, filter);
203212
}
204213

205-
async function listSimulatorApps(device: DeviceInfo): Promise<IosAppInfo[]> {
206-
const apps = await listSimulatorAppMetadata(device);
214+
type SimulatorAppListOptions = {
215+
/** Unset (default) preserves prior unbounded behavior for normal command flow. */
216+
timeoutMs?: number;
217+
};
218+
219+
async function listSimulatorApps(
220+
device: DeviceInfo,
221+
options?: SimulatorAppListOptions,
222+
): Promise<IosAppInfo[]> {
223+
const apps = await listSimulatorAppMetadata(device, options);
207224
return apps.map((app) => ({
208225
bundleId: app.bundleId,
209226
name: app.name,
210227
}));
211228
}
212229

213-
async function listSimulatorAppMetadata(device: DeviceInfo): Promise<SimulatorAppMetadata[]> {
214-
const result = await runSimctl(device, ['listapps', device.id], { allowFailure: true });
230+
async function listSimulatorAppMetadata(
231+
device: DeviceInfo,
232+
options?: SimulatorAppListOptions,
233+
): Promise<SimulatorAppMetadata[]> {
234+
const result = await runSimctl(device, ['listapps', device.id], {
235+
allowFailure: true,
236+
timeoutMs: options?.timeoutMs,
237+
});
215238
const stdout = result.stdout as string;
216239
const trimmed = stdout.trim();
217240
if (!trimmed) return [];
@@ -248,6 +271,7 @@ async function listSimulatorAppMetadata(device: DeviceInfo): Promise<SimulatorAp
248271
const converted = await runAppleToolCommand('plutil', ['-convert', 'json', '-o', '-', '-'], {
249272
allowFailure: true,
250273
stdin: trimmed,
274+
timeoutMs: options?.timeoutMs,
251275
});
252276
if (converted.exitCode === 0 && converted.stdout.trim().startsWith('{')) {
253277
parsed = JSON.parse(converted.stdout) as Record<

0 commit comments

Comments
 (0)