Skip to content

Commit 4807acd

Browse files
thymikeethymikee
committed
Address review findings for boot diagnostics and command gating
1 parent 358271f commit 4807acd

8 files changed

Lines changed: 185 additions & 17 deletions

File tree

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ dist/
44
.DS_Store
55
*.log
66
test/screenshots/*.png
7+
test/artifacts/
78
.build/
89
.swiftpm/
910
DerivedData/

‎src/daemon/handlers/__tests__/session.test.ts‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
import test from 'node:test';
2+
import assert from 'node:assert/strict';
3+
import fs from 'node:fs';
4+
import os from 'node:os';
5+
import path from 'node:path';
6+
import { handleSessionCommands } from '../session.ts';
7+
import { SessionStore } from '../../session-store.ts';
8+
import type { DaemonRequest, DaemonResponse, SessionState } from '../../types.ts';
9+
10+
function makeSessionStore(): SessionStore {
11+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-session-handler-'));
12+
return new SessionStore(path.join(root, 'sessions'));
13+
}
14+
15+
function makeSession(name: string, device: SessionState['device']): SessionState {
16+
return {
17+
name,
18+
device,
19+
createdAt: Date.now(),
20+
actions: [],
21+
};
22+
}
23+
24+
const noopInvoke = async (_req: DaemonRequest): Promise<DaemonResponse> => ({ ok: true, data: {} });
25+
26+
test('boot requires session or explicit selector', async () => {
27+
const sessionStore = makeSessionStore();
28+
const response = await handleSessionCommands({
29+
req: {
30+
token: 't',
31+
session: 'default',
32+
command: 'boot',
33+
positionals: [],
34+
flags: {},
35+
},
36+
sessionName: 'default',
37+
logPath: path.join(os.tmpdir(), 'daemon.log'),
38+
sessionStore,
39+
invoke: noopInvoke,
40+
ensureReady: async () => {},
41+
});
42+
assert.ok(response);
43+
assert.equal(response?.ok, false);
44+
if (response && !response.ok) {
45+
assert.equal(response.error.code, 'INVALID_ARGS');
46+
}
47+
});
48+
49+
test('boot rejects unsupported iOS device kind', async () => {
50+
const sessionStore = makeSessionStore();
51+
const sessionName = 'ios-device-session';
52+
sessionStore.set(
53+
sessionName,
54+
makeSession(sessionName, {
55+
platform: 'ios',
56+
id: 'ios-device-1',
57+
name: 'iPhone Device',
58+
kind: 'device',
59+
booted: true,
60+
}),
61+
);
62+
const response = await handleSessionCommands({
63+
req: {
64+
token: 't',
65+
session: sessionName,
66+
command: 'boot',
67+
positionals: [],
68+
flags: {},
69+
},
70+
sessionName,
71+
logPath: path.join(os.tmpdir(), 'daemon.log'),
72+
sessionStore,
73+
invoke: noopInvoke,
74+
ensureReady: async () => {
75+
throw new Error('ensureReady should not be called for unsupported boot');
76+
},
77+
});
78+
assert.ok(response);
79+
assert.equal(response?.ok, false);
80+
if (response && !response.ok) {
81+
assert.equal(response.error.code, 'UNSUPPORTED_OPERATION');
82+
}
83+
});
84+
85+
test('boot succeeds for supported device in session', async () => {
86+
const sessionStore = makeSessionStore();
87+
const sessionName = 'android-session';
88+
sessionStore.set(
89+
sessionName,
90+
makeSession(sessionName, {
91+
platform: 'android',
92+
id: 'emulator-5554',
93+
name: 'Pixel Emulator',
94+
kind: 'emulator',
95+
booted: true,
96+
}),
97+
);
98+
let ensureCalls = 0;
99+
const response = await handleSessionCommands({
100+
req: {
101+
token: 't',
102+
session: sessionName,
103+
command: 'boot',
104+
positionals: [],
105+
flags: {},
106+
},
107+
sessionName,
108+
logPath: path.join(os.tmpdir(), 'daemon.log'),
109+
sessionStore,
110+
invoke: noopInvoke,
111+
ensureReady: async () => {
112+
ensureCalls += 1;
113+
},
114+
});
115+
assert.ok(response);
116+
assert.equal(response?.ok, true);
117+
assert.equal(ensureCalls, 1);
118+
if (response && response.ok) {
119+
assert.equal(response.data?.platform, 'android');
120+
assert.equal(response.data?.booted, true);
121+
}
122+
});

‎src/daemon/handlers/session.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,19 @@ export async function handleSessionCommands(params: {
2121
sessionStore: SessionStore;
2222
invoke: (req: DaemonRequest) => Promise<DaemonResponse>;
2323
dispatch?: typeof dispatchCommand;
24+
ensureReady?: typeof ensureDeviceReady;
2425
}): Promise<DaemonResponse | null> {
25-
const { req, sessionName, logPath, sessionStore, invoke, dispatch: dispatchOverride } = params;
26+
const {
27+
req,
28+
sessionName,
29+
logPath,
30+
sessionStore,
31+
invoke,
32+
dispatch: dispatchOverride,
33+
ensureReady: ensureReadyOverride,
34+
} = params;
2635
const dispatch = dispatchOverride ?? dispatchCommand;
36+
const ensureReady = ensureReadyOverride ?? ensureDeviceReady;
2737
const command = req.command;
2838

2939
if (command === 'session_list') {
@@ -82,7 +92,7 @@ export async function handleSessionCommands(params: {
8292
};
8393
}
8494
const device = session?.device ?? (await resolveTargetDevice(flags));
85-
await ensureDeviceReady(device);
95+
await ensureReady(device);
8696
if (!isCommandSupportedOnDevice('apps', device)) {
8797
return { ok: false, error: { code: 'UNSUPPORTED_OPERATION', message: 'apps is not supported on this device' } };
8898
}
@@ -119,7 +129,10 @@ export async function handleSessionCommands(params: {
119129
};
120130
}
121131
const device = session?.device ?? (await resolveTargetDevice(flags));
122-
await ensureDeviceReady(device);
132+
if (!isCommandSupportedOnDevice('boot', device)) {
133+
return { ok: false, error: { code: 'UNSUPPORTED_OPERATION', message: 'boot is not supported on this device' } };
134+
}
135+
await ensureReady(device);
123136
return {
124137
ok: true,
125138
data: {
@@ -136,7 +149,7 @@ export async function handleSessionCommands(params: {
136149
const session = sessionStore.get(sessionName);
137150
const flags = req.flags ?? {};
138151
const device = session?.device ?? (await resolveTargetDevice(flags));
139-
await ensureDeviceReady(device);
152+
await ensureReady(device);
140153
if (device.platform === 'ios') {
141154
if (session?.appBundleId) {
142155
return {

‎src/platforms/__tests__/boot-diagnostics.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,3 +41,11 @@ test('bootFailureHint returns actionable guidance', () => {
4141
const hint = bootFailureHint('IOS_RUNNER_CONNECT_TIMEOUT');
4242
assert.equal(hint.includes('xcodebuild logs'), true);
4343
});
44+
45+
test('connect phase does not classify non-timeout errors as connect timeout', () => {
46+
const reason = classifyBootFailure({
47+
message: 'Runner returned malformed JSON payload',
48+
context: { platform: 'ios', phase: 'connect' },
49+
});
50+
assert.equal(reason, 'BOOT_COMMAND_FAILED');
51+
});

‎src/platforms/android/devices.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,12 +145,15 @@ export async function waitForAndroidBoot(serial: string, timeoutMs = 60000): Pro
145145
const stdout = lastBootResult?.stdout;
146146
const stderr = lastBootResult?.stderr;
147147
const exitCode = lastBootResult?.exitCode;
148-
const reason = classifyBootFailure({
148+
let reason = classifyBootFailure({
149149
error,
150150
stdout,
151151
stderr,
152152
context: { platform: 'android', phase: 'boot' },
153153
});
154+
if (reason === 'BOOT_COMMAND_FAILED' && appErr.message === 'Android device is still booting') {
155+
reason = 'ANDROID_BOOT_TIMEOUT';
156+
}
154157
const baseDetails = {
155158
serial,
156159
timeoutMs: timeoutBudget,

‎src/platforms/boot-diagnostics.ts‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,21 @@ export function classifyBootFailure(input: {
5555
.join('\n')
5656
.toLowerCase();
5757

58-
if (platform === 'ios' && (phase === 'connect' || haystack.includes('runner did not accept connection'))) {
58+
if (
59+
platform === 'ios' &&
60+
(
61+
haystack.includes('runner did not accept connection') ||
62+
(phase === 'connect' &&
63+
(
64+
haystack.includes('timed out') ||
65+
haystack.includes('timeout') ||
66+
haystack.includes('econnrefused') ||
67+
haystack.includes('connection refused') ||
68+
haystack.includes('fetch failed') ||
69+
haystack.includes('socket hang up')
70+
))
71+
)
72+
) {
5973
return 'IOS_RUNNER_CONNECT_TIMEOUT';
6074
}
6175
if (platform === 'ios' && phase === 'boot' && (haystack.includes('timed out') || haystack.includes('timeout'))) {
@@ -73,15 +87,16 @@ export function classifyBootFailure(input: {
7387
return 'CI_RESOURCE_STARVATION_SUSPECTED';
7488
}
7589
if (
76-
platform === 'android' && (
77-
haystack.includes('device not found') ||
78-
haystack.includes('no devices') ||
79-
haystack.includes('device offline') ||
80-
haystack.includes('offline') ||
81-
haystack.includes('unauthorized') ||
82-
haystack.includes('not authorized') ||
83-
haystack.includes('unable to locate device') ||
84-
haystack.includes('invalid device')
90+
platform === 'android' &&
91+
(
92+
haystack.includes('device not found') ||
93+
haystack.includes('no devices') ||
94+
haystack.includes('device offline') ||
95+
haystack.includes('offline') ||
96+
haystack.includes('unauthorized') ||
97+
haystack.includes('not authorized') ||
98+
haystack.includes('unable to locate device') ||
99+
haystack.includes('invalid device')
85100
)
86101
) {
87102
return 'ADB_TRANSPORT_UNAVAILABLE';

‎src/platforms/ios/index.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,8 +225,6 @@ export async function ensureBootedSimulator(device: DeviceInfo): Promise<void> {
225225
try {
226226
await retryWithPolicy(
227227
async () => {
228-
const currentState = await getSimulatorState(device.id);
229-
if (currentState === 'Booted') return;
230228
bootResult = await runCmd('xcrun', ['simctl', 'boot', device.id], { allowFailure: true });
231229
const bootOutput = `${bootResult.stdout}\n${bootResult.stderr}`.toLowerCase();
232230
const bootAlreadyDone =

‎src/platforms/ios/runner-client.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,11 +485,19 @@ async function postCommandViaSimulator(
485485
);
486486
const body = result.stdout as string;
487487
if (result.exitCode !== 0) {
488+
const reason = classifyBootFailure({
489+
message: 'Runner did not accept connection (simctl spawn)',
490+
stdout: result.stdout,
491+
stderr: result.stderr,
492+
context: { platform: 'ios', phase: 'connect' },
493+
});
488494
throw new AppError('COMMAND_FAILED', 'Runner did not accept connection (simctl spawn)', {
489495
port,
490496
stdout: result.stdout,
491497
stderr: result.stderr,
492498
exitCode: result.exitCode,
499+
reason,
500+
hint: bootFailureHint(reason),
493501
});
494502
}
495503
return { status: 200, body };

0 commit comments

Comments
 (0)