Skip to content

Commit 289492d

Browse files
committed
fix(apple): type the alert-absence retry instead of matching error prose
Review blocker 1 on #2021. The Apple alert legs decided retry and hint eligibility by substring-matching error messages for "alert not found" / "no alert", and `alert wait` swallowed *every* read failure. A dead runner, an unreachable macOS helper or a canceled request was therefore spent as poll budget and finally reported as `alert wait timed out`, hiding the real cause. Both backends now state absence as typed evidence: - The XCTest runner answers `ErrorPayload(code: "ALERT_NOT_FOUND", ...)`. It is diagnostic-only, so it stays `COMMAND_FAILED` on the wire and surfaces as `details.runnerErrorCode` — the same shape `RUNNER_BUSY` already used. - The macOS helper adds `reason: "alert-not-found"` to its JSON error details, which the helper client already forwards verbatim. `isAlertNotFoundError` reads only those two fields. `awaitAppleAlert` re-throws anything that is not a typed absence instead of polling through it, and the scoped-snapshot fallback hint attaches to typed absence alone. The three tests the review asked for, plus coverage the daemon-altitude copies could not express: a non-absence failure propagates immediately from `wait`; an action does not retry a failure whose message merely reads like an absence; the macOS helper's typed reason is retried like the runner's. The daemon-level non-absence test moved to the family suite that owns this policy since R59, lowering that file's size pin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RpfS12XApXqasuAWZJaEX
1 parent 95d609c commit 289492d

9 files changed

Lines changed: 166 additions & 50 deletions

File tree

apple/macos-helper/Sources/AgentDeviceMacOSHelper/main.swift

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,9 +287,14 @@ struct AgentDeviceMacOSHelper {
287287
let surface = optionValue(arguments: Array(arguments.dropFirst()), name: "--surface")
288288
let app = try resolveTargetApplication(bundleId: bundleId, surface: surface)
289289
guard let alertElement = findAlertElement(appElement: AXUIElementCreateApplication(app.processIdentifier)) else {
290+
// `reason` is the typed channel the host retries on; the message is for humans only.
290291
throw HelperError.commandFailed(
291292
"alert not found",
292-
details: ["bundleId": app.bundleIdentifier ?? "", "appName": app.localizedName ?? ""]
293+
details: [
294+
"reason": "alert-not-found",
295+
"bundleId": app.bundleIdentifier ?? "",
296+
"appName": app.localizedName ?? "",
297+
]
293298
)
294299
}
295300
let buttons = collectButtons(root: alertElement)

apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2190,7 +2190,12 @@ extension RunnerTests {
21902190
Self.alertCommandTimeout(timeoutMs: command.timeoutMs)
21912191
)
21922192
guard let alert = resolveAlert(app: activeApp, deadline: deadline) else {
2193-
return Response(ok: false, error: ErrorPayload(message: "alert not found"))
2193+
// Typed so the host retries on absence alone: a transport or runner failure carries no
2194+
// code and must not be mistaken for "no alert yet" (ALERT_NOT_FOUND_RUNNER_CODE).
2195+
return Response(
2196+
ok: false,
2197+
error: ErrorPayload(code: "ALERT_NOT_FOUND", message: "alert not found")
2198+
)
21942199
}
21952200
return handleAlert(alert, action: action, deadline: deadline)
21962201
case .gesture:

packages/contracts/src/alert-contract.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,16 @@ export const ALERT_POLL_INTERVAL_MS = 300;
22
export const DEFAULT_ALERT_TIMEOUT_MS = 10_000;
33
export const ALERT_ACTION_RETRY_MS = 2_000;
44

5+
/**
6+
* The one alert failure the family retries on: the backend looked and there was no alert *yet*.
7+
* Both Apple backends state it in typed form — the XCTest runner as this `ErrorPayload.code`
8+
* (surfacing as `details.runnerErrorCode`, like `RUNNER_BUSY`, without changing the wire error
9+
* code), the macOS helper as `details.reason`. Retry and the fallback hint key on these, never on
10+
* the message text: a transport, runner or helper failure must never read as an absent alert.
11+
*/
12+
export const ALERT_NOT_FOUND_RUNNER_CODE = 'ALERT_NOT_FOUND';
13+
export const ALERT_NOT_FOUND_REASON = 'alert-not-found';
14+
515
export const ALERT_ACTIONS = ['get', 'accept', 'dismiss', 'wait'] as const;
616
export type AlertAction = (typeof ALERT_ACTIONS)[number];
717

packages/contracts/src/facades/interaction.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
export {
22
ALERT_ACTIONS,
33
ALERT_ACTION_RETRY_MS,
4+
ALERT_NOT_FOUND_REASON,
5+
ALERT_NOT_FOUND_RUNNER_CODE,
46
ALERT_POLL_INTERVAL_MS,
57
DEFAULT_ALERT_TIMEOUT_MS,
68
} from '../alert-contract.ts';

scripts/__tests__/test-file-size-ratchet.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ const TRIPWIRE_LINES = 1_000;
3434
// Exact current lengths. Lower a pin when its file shrinks; never raise one — extract instead.
3535
const PINNED_TEST_FILE_LINES: Readonly<Record<string, number>> = Object.freeze({
3636
'src/__tests__/remote-connection.test.ts': 2973,
37-
'src/daemon/handlers/__tests__/snapshot-handler.test.ts': 2301,
37+
'src/daemon/handlers/__tests__/snapshot-handler.test.ts': 2284,
3838
'src/commands/interaction/runtime/settle.test.ts': 2359,
3939
'src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts': 2020,
4040
'src/platforms/apple/core/__tests__/runner-session.test.ts': 2001,

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

Lines changed: 16 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -2134,15 +2134,23 @@ test('wait selector bypasses a fresh matching session snapshot', async () => {
21342134
);
21352135
});
21362136

2137-
test('alert accept retries on "alert not found" and succeeds on second attempt', async () => {
2137+
/**
2138+
* Absence as the XCTest runner states it (`ALERT_NOT_FOUND` surfaces as `details.runnerErrorCode`).
2139+
* The retry and the fallback hint key on that evidence, never on the message text.
2140+
*/
2141+
function alertAbsence(message = 'alert not found'): AppError {
2142+
return new AppError('COMMAND_FAILED', message, { runnerErrorCode: 'ALERT_NOT_FOUND' });
2143+
}
2144+
2145+
test('alert accept retries a typed alert absence and succeeds on the second attempt', async () => {
21382146
const sessionStore = makeSessionStore();
21392147
const sessionName = 'ios-sim';
21402148
sessionStore.set(sessionName, makeSession(sessionName, iosSimulatorDevice));
21412149

21422150
let calls = 0;
21432151
mockRunnerCommand.mockImplementation(async () => {
21442152
calls += 1;
2145-
if (calls === 1) throw new AppError('COMMAND_FAILED', 'alert not found');
2153+
if (calls === 1) throw alertAbsence();
21462154
return { accepted: true };
21472155
});
21482156

@@ -2163,41 +2171,16 @@ test('alert accept retries on "alert not found" and succeeds on second attempt',
21632171
});
21642172
});
21652173

2166-
test('alert accept does not retry on non-alert errors', async () => {
2167-
const sessionStore = makeSessionStore();
2168-
const sessionName = 'ios-sim';
2169-
sessionStore.set(sessionName, makeSession(sessionName, iosSimulatorDevice));
2170-
2171-
let calls = 0;
2172-
mockRunnerCommand.mockImplementation(async () => {
2173-
calls += 1;
2174-
throw new AppError('COMMAND_FAILED', 'runner crashed');
2175-
});
2176-
2177-
await expect(
2178-
handleSnapshotCommands({
2179-
req: {
2180-
token: 't',
2181-
session: sessionName,
2182-
command: 'alert',
2183-
positionals: ['accept'],
2184-
flags: {},
2185-
},
2186-
sessionName,
2187-
logPath: '/tmp/daemon.log',
2188-
sessionStore,
2189-
}),
2190-
).rejects.toThrow('runner crashed');
2191-
2192-
expect(calls).toBe(1);
2193-
});
2174+
// The non-absence case moved to `src/platforms/apple/__tests__/alert.test.ts` with the retry policy
2175+
// itself (R59), where it also covers a failure whose message merely reads like an absence — the
2176+
// case this daemon-altitude copy could not distinguish.
21942177

21952178
test('alert accept adds a scoped-snapshot hint after retrying alert-not-found failures', async () => {
21962179
const sessionStore = makeSessionStore();
21972180
const sessionName = 'ios-sim';
21982181
sessionStore.set(sessionName, makeSession(sessionName, iosSimulatorDevice));
21992182

2200-
mockRunnerCommand.mockRejectedValue(new AppError('COMMAND_FAILED', 'alert not found'));
2183+
mockRunnerCommand.mockRejectedValue(alertAbsence());
22012184

22022185
let thrown: unknown;
22032186
try {
@@ -2222,15 +2205,15 @@ test('alert accept adds a scoped-snapshot hint after retrying alert-not-found fa
22222205
expect((thrown as AppError).details?.hint).toMatch(/scoped snapshot/i);
22232206
});
22242207

2225-
test('alert dismiss retries on "no alert" message', async () => {
2208+
test('alert dismiss retries a typed absence whatever the message says', async () => {
22262209
const sessionStore = makeSessionStore();
22272210
const sessionName = 'ios-sim';
22282211
sessionStore.set(sessionName, makeSession(sessionName, iosSimulatorDevice));
22292212

22302213
let calls = 0;
22312214
mockRunnerCommand.mockImplementation(async () => {
22322215
calls += 1;
2233-
if (calls < 3) throw new AppError('COMMAND_FAILED', 'no alert present');
2216+
if (calls < 3) throw alertAbsence('no alert present');
22342217
return { dismissed: true };
22352218
});
22362219

src/platforms/apple/__tests__/alert.test.ts

Lines changed: 82 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ import { afterEach, test, vi } from 'vitest';
44
vi.mock('../core/runner/runner-client.ts', () => ({ runAppleRunnerCommand: vi.fn() }));
55
vi.mock('../os/macos/helper.ts', () => ({ runMacOsAlertAction: vi.fn() }));
66

7+
import {
8+
ALERT_NOT_FOUND_REASON,
9+
ALERT_NOT_FOUND_RUNNER_CODE,
10+
} from '@agent-device/contracts/alert-contract';
711
import { AppError } from '@agent-device/kernel/errors';
812
import { IOS_SIMULATOR, MACOS_DEVICE } from '../../../__tests__/test-utils/device-fixtures.ts';
913
import { runAppleRunnerCommand } from '../core/runner/runner-client.ts';
@@ -14,6 +18,20 @@ const mockRunner = vi.mocked(runAppleRunnerCommand);
1418
const mockHelper = vi.mocked(runMacOsAlertAction);
1519
const runnerOptions = {};
1620

21+
/**
22+
* Absence as each backend states it. The message is deliberately the same prose the old predicate
23+
* matched on, so a test that passes here is passing on the typed evidence and nothing else.
24+
*/
25+
function runnerAbsence(): AppError {
26+
return new AppError('COMMAND_FAILED', 'alert not found', {
27+
runnerErrorCode: ALERT_NOT_FOUND_RUNNER_CODE,
28+
});
29+
}
30+
31+
function helperAbsence(): AppError {
32+
return new AppError('COMMAND_FAILED', 'alert not found', { reason: ALERT_NOT_FOUND_REASON });
33+
}
34+
1735
afterEach(() => {
1836
vi.useRealTimers();
1937
mockRunner.mockReset();
@@ -40,7 +58,7 @@ test('a wait polls until one attempt answers, and the first attempt gets the who
4058
let calls = 0;
4159
mockRunner.mockImplementation(async () => {
4260
calls += 1;
43-
if (calls === 1) throw new AppError('COMMAND_FAILED', 'alert not found');
61+
if (calls === 1) throw runnerAbsence();
4462
return { title: 'Camera Access' };
4563
});
4664

@@ -56,7 +74,7 @@ test('a wait polls until one attempt answers, and the first attempt gets the who
5674

5775
test('a wait that never sees an alert reports the timeout rather than the last attempt error', async () => {
5876
vi.useFakeTimers();
59-
mockRunner.mockRejectedValue(new AppError('COMMAND_FAILED', 'alert not found'));
77+
mockRunner.mockRejectedValue(runnerAbsence());
6078

6179
const outcome = awaitAppleAlert(IOS_SIMULATOR, runnerOptions, { timeoutMs: 900 }).then(
6280
() => undefined,
@@ -87,7 +105,7 @@ test('an accept retries only while the backend says the alert is not there yet',
87105

88106
test('an exhausted accept carries the scoped-snapshot fallback the agent needs next', async () => {
89107
vi.useFakeTimers();
90-
mockRunner.mockRejectedValue(new AppError('COMMAND_FAILED', 'alert not found'));
108+
mockRunner.mockRejectedValue(runnerAbsence());
91109

92110
const outcome = actOnAppleAlert(IOS_SIMULATOR, runnerOptions, 'accept').then(
93111
() => undefined,
@@ -100,6 +118,67 @@ test('an exhausted accept carries the scoped-snapshot fallback the agent needs n
100118
assert.match(String(error.details?.hint), /scoped snapshot/i);
101119
});
102120

121+
// The whole point of typing absence: a backend that failed for any other reason must not be
122+
// retried until the window expires and then reported as a timeout. These three pin that the
123+
// evidence — not the message text — is what decides.
124+
test('a wait propagates a non-absence failure instead of spending it as poll budget', async () => {
125+
vi.useFakeTimers();
126+
let calls = 0;
127+
// The message deliberately says "alert not found"; only the missing typed evidence matters.
128+
mockRunner.mockImplementation(async () => {
129+
calls += 1;
130+
throw new AppError('COMMAND_FAILED', 'runner transport closed: alert not found');
131+
});
132+
133+
const outcome = awaitAppleAlert(IOS_SIMULATOR, runnerOptions, { timeoutMs: 5_000 }).then(
134+
() => undefined,
135+
(error: unknown) => error,
136+
);
137+
await vi.advanceTimersByTimeAsync(6_000);
138+
139+
assert.match(String(((await outcome) as Error).message), /runner transport closed/);
140+
assert.equal(calls, 1);
141+
});
142+
143+
test('an action does not retry a failure that only reads like an absence', async () => {
144+
vi.useFakeTimers();
145+
let calls = 0;
146+
mockRunner.mockImplementation(async () => {
147+
calls += 1;
148+
throw new AppError('COMMAND_FAILED', 'no alert service on this runner');
149+
});
150+
151+
const outcome = actOnAppleAlert(IOS_SIMULATOR, runnerOptions, 'dismiss').then(
152+
() => undefined,
153+
(error: unknown) => error,
154+
);
155+
await vi.advanceTimersByTimeAsync(3_500);
156+
const error = (await outcome) as AppError;
157+
158+
assert.equal(calls, 1);
159+
// The fallback hint is absence-only advice, so an untyped failure must not carry it either.
160+
assert.equal(error.details?.hint, undefined);
161+
});
162+
163+
test('the macOS helper states absence as a typed reason, and the family retries it', async () => {
164+
vi.useFakeTimers();
165+
let calls = 0;
166+
mockHelper.mockImplementation(async () => {
167+
calls += 1;
168+
throw helperAbsence();
169+
});
170+
171+
const outcome = actOnAppleAlert(MACOS_DEVICE, runnerOptions, 'accept').then(
172+
() => undefined,
173+
(error: unknown) => error,
174+
);
175+
await vi.advanceTimersByTimeAsync(3_500);
176+
const error = (await outcome) as AppError;
177+
178+
assert.ok(calls > 1, 'a typed absence is retried');
179+
assert.match(String(error.details?.hint), /scoped snapshot/i);
180+
});
181+
103182
// The macOS host answers through its helper, and a frontmost-app session names no bundle at all.
104183
test('the macOS host reads through its helper, and a frontmost-app session names no bundle', async () => {
105184
mockHelper.mockResolvedValue({ title: 'Allow access' });

src/platforms/apple/alert.ts

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import {
22
ALERT_ACTION_RETRY_MS,
3+
ALERT_NOT_FOUND_REASON,
4+
ALERT_NOT_FOUND_RUNNER_CODE,
35
ALERT_POLL_INTERVAL_MS,
46
DEFAULT_ALERT_TIMEOUT_MS,
57
} from '@agent-device/contracts/alert-contract';
@@ -15,9 +17,14 @@ import { runMacOsAlertAction } from './os/macos/helper.ts';
1517

1618
/**
1719
* Apple's four alert legs. R59 moved them here from the daemon: how long to look for a transient
18-
* alert, how many times to re-ask a runner that answers "alert not found", and which of the two
19-
* Apple backends answers at all are family mechanics, not request policy. What the caller supplies
20-
* is the window it allows and the session's target; everything else is this family's.
20+
* alert, how many times to re-ask a backend that reports the alert is not there yet, and which of
21+
* the two Apple backends answers at all are family mechanics, not request policy. What the caller
22+
* supplies is the window it allows and the session's target; everything else is this family's.
23+
*
24+
* Absence is the one retriable outcome, and both backends state it as typed evidence — the XCTest
25+
* runner as `ALERT_NOT_FOUND`, the macOS helper as `reason: 'alert-not-found'`. Nothing here reads
26+
* an error message: a transport, runner or helper failure that happened to mention an alert would
27+
* otherwise be retried until the window expired and then reported as a timeout, hiding the cause.
2128
*/
2229
type NativeAlertAction = 'get' | 'accept' | 'dismiss';
2330

@@ -75,18 +82,21 @@ export async function awaitAppleAlert(
7582
const budgetMs = firstAttempt ? timeout : remainingBudgetMs(start, timeout);
7683
firstAttempt = false;
7784
return await runAlert('get', budgetMs);
78-
} catch {
79-
// keep waiting
85+
} catch (error) {
86+
// Only a typed absence is worth waiting out. Anything else — a dead runner, an unreachable
87+
// helper, a canceled request — is reported as itself rather than spent as poll budget and
88+
// relabeled `alert wait timed out`.
89+
if (!isAlertNotFoundError(error)) throw error;
8090
}
8191
await sleep(ALERT_POLL_INTERVAL_MS);
8292
}
8393
throw new AppError('COMMAND_FAILED', 'alert wait timed out');
8494
}
8595

8696
/**
87-
* Accept and dismiss retry only while the backend keeps saying the alert is not there yet — a
88-
* sheet that is still animating in. Any other failure is reported on its first occurrence, and
89-
* an exhausted retry window carries the scoped-snapshot fallback the agent needs next.
97+
* Accept and dismiss retry only while the backend keeps reporting a typed absence — a sheet that
98+
* is still animating in. Any other failure is reported on its first occurrence, and an exhausted
99+
* retry window carries the scoped-snapshot fallback the agent needs next.
90100
*/
91101
export async function actOnAppleAlert(
92102
device: DeviceInfo,
@@ -127,7 +137,16 @@ function withAlertFallbackHint(error: unknown): unknown {
127137
});
128138
}
129139

140+
/**
141+
* Absence, as the backend itself classified it. The XCTest runner's `ALERT_NOT_FOUND` arrives as
142+
* `details.runnerErrorCode` (it stays `COMMAND_FAILED` on the wire, like `RUNNER_BUSY`); the macOS
143+
* helper's arrives as `details.reason`, forwarded verbatim from its JSON error envelope.
144+
*/
130145
function isAlertNotFoundError(error: unknown): boolean {
131-
const message = String((error as { message?: unknown })?.message ?? '').toLowerCase();
132-
return message.includes('alert not found') || message.includes('no alert');
146+
if (!(error instanceof AppError)) return false;
147+
const details = error.details ?? {};
148+
return (
149+
details['runnerErrorCode'] === ALERT_NOT_FOUND_RUNNER_CODE ||
150+
details['reason'] === ALERT_NOT_FOUND_REASON
151+
);
133152
}

src/platforms/apple/core/runner/runner-session.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { AppError, toAppErrorCode, createRequestCanceledError } from '@agent-device/kernel/errors';
2+
import { ALERT_NOT_FOUND_RUNNER_CODE } from '@agent-device/contracts/alert-contract';
23
import { type ExecResult } from '../../../../utils/exec.ts';
34
import { withKeyedLock } from '../../../../utils/keyed-lock.ts';
45
import { Deadline } from '../../../../utils/retry.ts';
@@ -891,8 +892,20 @@ function readRunnerErrorCode(rawCode: unknown): string | undefined {
891892
return typeof rawCode === 'string' && rawCode.trim().length > 0 ? rawCode.trim() : undefined;
892893
}
893894

895+
/**
896+
* Runner codes that classify a failure for the host without renaming it on the wire. They stay
897+
* `COMMAND_FAILED` and survive as `details.runnerErrorCode`, which is what family policy reads:
898+
* `RUNNER_BUSY` for retriable contention, `ALERT_NOT_FOUND` for an alert that is not there yet.
899+
*/
900+
const DIAGNOSTIC_ONLY_RUNNER_ERROR_CODES: ReadonlySet<string> = new Set([
901+
'RUNNER_BUSY',
902+
ALERT_NOT_FOUND_RUNNER_CODE,
903+
]);
904+
894905
function runnerAppErrorCode(runnerErrorCode: string | undefined): AppError['code'] {
895-
if (runnerErrorCode === 'RUNNER_BUSY') return 'COMMAND_FAILED';
906+
if (runnerErrorCode !== undefined && DIAGNOSTIC_ONLY_RUNNER_ERROR_CODES.has(runnerErrorCode)) {
907+
return 'COMMAND_FAILED';
908+
}
896909
return runnerErrorCode ? toAppErrorCode(runnerErrorCode) : 'COMMAND_FAILED';
897910
}
898911

0 commit comments

Comments
 (0)