From 96162bac198aa4b6cdb5e0b3ad784a260307f8ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 13:11:05 +0200 Subject: [PATCH 1/5] fix(replay): bounded retry on pre-dispatch capture racing app launch (#1385) A step right after `open --relaunch` (e.g. a step-2 press) can have its pre-dispatch target-verification capture land while the app is still launching/mounting, producing a transient capture-failed/sparse-snapshot verdict that isn't a real divergence and fails the step closed before it ever dispatches. captureDivergenceObservation now takes an opt-in retryLaunchRace flag that bounds-retries that specific capture (fixed backoff, 12s deadline), mirroring wait's keep-polling landmark semantics (#1349) on this pre-dispatch path. Only verifyReplayActionTarget's gate opts in; the post-failure diagnostic capture and post-resolution guard-mismatch capture stay single-shot since they follow an already-real failure. --- docs/adr/0012-interactive-replay.md | 24 ++++- ...replay-target-verification-runtime.test.ts | 101 ++++++++++++++++++ .../handlers/session-replay-divergence.ts | 60 ++++++++++- .../session-replay-target-verification.ts | 6 ++ 4 files changed, 186 insertions(+), 5 deletions(-) diff --git a/docs/adr/0012-interactive-replay.md b/docs/adr/0012-interactive-replay.md index 8f3e55b3b3..bd2db32e92 100644 --- a/docs/adr/0012-interactive-replay.md +++ b/docs/adr/0012-interactive-replay.md @@ -3,9 +3,9 @@ ## Status Accepted (2026-07-10). Implemented on `main`, including the amendments folded into the decisions -below (#1264, #1269, #1271 stage 2, #1280, #1349); only decision 5's replay benchmark extension -remains deferred. The per-step landing record (PRs #1193-#1349) and the pre-acceptance migration -plan live in this file's git history. +below (#1264, #1269, #1271 stage 2, #1280, #1349, #1385); only decision 5's replay benchmark +extension remains deferred. The per-step landing record (PRs #1193-#1349) and the pre-acceptance +migration plan live in this file's git history. ## Rules at a glance @@ -29,7 +29,9 @@ Normative summary, one entry per decision. The binding contracts, amendments, an `identity-unverifiable` divergence, never a silent pick. Amendments: non-unique ids demote to role+label (#1269); an identity-empty pressed container records its first labeled descendant, double-guarded fail-closed (#1280); `wait` landmark identity and `is` coverage dispatch on the - `targetIdentityVerification` descriptor trait (#1349). + `targetIdentityVerification` descriptor trait (#1349); the pre-dispatch verification capture + bounds-retries a content-quality `capture-failed`/`sparse-snapshot` verdict on an app-launch race, + opt-in per call site (#1385). 4. **Divergence is a structured error, resumable by plan ordinal.** `ok:false` with code `REPLAY_DIVERGENCE` and `details.divergence` v1: `kind` (`action-failure` | `selector-miss` | `identity-mismatch` | `identity-unverifiable`), a bounded `screen` (same capture scope as plain @@ -369,6 +371,20 @@ An old unannotated action remains executable without this check. All three diver target-binding divergences reported before the device action. This is not general outcome verification: `--verify` remains post-action change evidence with a different contract. +> **Amendment (#1385).** The capture this verification runs against (`captureDivergenceObservation`) is +> itself the pre-dispatch gate a step right after `open --relaunch` races: the app can still be +> launching/mounting when it lands, producing a transient content-quality verdict — `capture-failed` +> (Android's snapshot helper returns "insufficient foreground app content" while the app is mounting, and +> the capture path throws) or `sparse-snapshot` (iOS's private-AX fallback under load) — that is not a +> real divergence, only an unlucky capture. This capture now retries with a bounded backoff (fixed delay +> list, capped at a 12s wall-clock deadline) before falling through to `identity-unverifiable`, mirroring +> the keep-polling semantics `wait`'s recorded-landmark identity verification (#1349) already applies on +> its own (post-resolution) path. The retry is opt-in per call site (`retryLaunchRace`), not a change to +> every `captureDivergenceObservation` caller: only the pre-dispatch verification gate in +> `verifyReplayActionTarget` races a launch this way — the post-failure diagnostic capture +> (`buildReplayFailureDivergence`) and the post-resolution guard-mismatch capture both follow an +> already-real failure, where retrying would only delay an already-decided divergence. + ### 4. Divergence wire contract and replay-only resume **Divergence is a structured error, not success data.** The daemon returns `ok:false` with code diff --git a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts index b3fb0f2cdd..15d4e97c53 100644 --- a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts @@ -13,6 +13,16 @@ vi.mock('../../../core/dispatch.ts', async (importOriginal) => { return { ...actual, dispatchCommand: vi.fn(async () => ({})), resolveTargetDevice: vi.fn() }; }); +// #1385's pre-dispatch launch-race retry (captureDivergenceObservation's +// `retryLaunchRace`) awaits a real `sleep` between attempts; stub it to a +// no-op so the retry tests below exercise the retry BRANCH (still runs each +// mocked capture attempt) without a real wall-clock wait (repo guidance: unit +// tests must not wait real time). +vi.mock('../../../utils/timeouts.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, sleep: vi.fn(async () => {}) }; +}); + import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -486,6 +496,97 @@ test('a target-binding divergence carries a real computed resume (step 5 wiring, expect(resume.reason).toBeUndefined(); }); +// #1385: a step-2 press right after `open --relaunch` can capture while the +// app is still launching/mounting — the pre-dispatch verification capture +// throws (Android's "insufficient foreground app content" surfaces as a +// thrown capture failure) or returns sparse on the first attempt(s), then +// recovers once the app finishes mounting. The bounded retry in +// `captureDivergenceObservation` (`retryLaunchRace`) must ride that out +// instead of failing closed on the first unlucky capture. +test('a transient pre-dispatch capture failure right after launch recovers within the bounded retry, and the action still dispatches', async () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-recover-'), + ); + const { sessionStore, sessionName } = setupSession(root); + const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); + + // First two captures fail closed (mirrors a mid-launch/mid-mount capture); + // the third lands after the app settles and finds the recorded target. + mockDispatchCommand + .mockImplementationOnce(async () => { + throw new Error('Android snapshot helper returned insufficient foreground app content'); + }) + .mockImplementationOnce(async () => { + throw new Error('Android snapshot helper returned insufficient foreground app content'); + }) + .mockResolvedValue({ + nodes: [ + { + index: 0, + depth: 0, + type: 'Button', + identifier: 'save', + label: 'Save', + rect: { x: 10, y: 10, width: 40, height: 20 }, + }, + ], + truncated: false, + backend: 'xctest', + }); + + const invoked: DaemonRequest[] = []; + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + invoked.push(req); + return { ok: true, data: {} }; + }, + }); + + expect(response.ok).toBe(true); + expect(invoked.map((req) => req.command)).toEqual(['click']); + expect(mockDispatchCommand).toHaveBeenCalledTimes(3); +}); + +test('a pre-dispatch capture failure that never recovers still fails closed as identity-unverifiable once the bounded retry is exhausted', async () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-exhausted-'), + ); + const { sessionStore, sessionName } = setupSession(root); + const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); + + mockDispatchCommand.mockImplementation(async () => { + throw new Error('Android snapshot helper returned insufficient foreground app content'); + }); + + const invoked: DaemonRequest[] = []; + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + invoked.push(req); + return { ok: true, data: {} }; + }, + }); + + expect(invoked.length).toBe(0); + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('REPLAY_DIVERGENCE'); + const divergence = response.error.details?.divergence as Record; + expect(divergence.kind).toBe('identity-unverifiable'); + const cause = divergence.cause as { code: string; message: string }; + expect(cause.code).toBe('IDENTITY_UNVERIFIABLE'); + expect(cause.message).toContain('capture-failed'); + // Bounded: the retry gives up after its fixed delay list, not forever. + expect(mockDispatchCommand.mock.calls.length).toBeGreaterThan(1); +}); + // --------------------------------------------------------------------------- // #1349: wait's post-resolution landmark verification. An annotated selector // wait must NEVER enter the generic pre-dispatch resolution path — polling is diff --git a/src/daemon/handlers/session-replay-divergence.ts b/src/daemon/handlers/session-replay-divergence.ts index eb51475d1b..721b00e8b3 100644 --- a/src/daemon/handlers/session-replay-divergence.ts +++ b/src/daemon/handlers/session-replay-divergence.ts @@ -1,5 +1,6 @@ import fs from 'node:fs'; import path from 'node:path'; +import { sleep } from '../../utils/timeouts.ts'; import { markSessionPartialRefsIssued, setSessionSnapshot } from '../session-snapshot.ts'; import { isSparseSnapshotQualityVerdict } from '../../snapshot/snapshot-quality.ts'; import { displayLabel, formatRole } from '../../snapshot/snapshot-lines.ts'; @@ -216,15 +217,72 @@ export function toReplayRepairHintCapture( * (`collectSettleChromeRefs`) and the meaningful-target filter stay layered ON * TOP of this full capture as FILTERS, never as a narrower scoping. */ +// #1385: a `capture-failed` / `sparse-snapshot` verdict on the PRE-DISPATCH +// target-verification capture (`verifyReplayActionTarget`) right after an +// `open --relaunch` step is often just the app still launching/mounting, not +// a real divergence — the SAME transition `wait`'s keep-polling landmark +// verification (#1349) rides out on its own (post-resolution) path. Bounded +// by BOTH a wall-clock deadline and a fixed delay-list length, so a +// mocked-instant `sleep` in tests cannot turn this into a busy-loop — it just +// runs the list once (mirrors the Android freshness retry shape in +// `snapshot-capture.ts`). +// +// Opt-in via `retryLaunchRace`, NOT the default for every caller: the +// post-failure diagnostic capture (`buildReplayFailureDivergence`) and the +// post-resolution guard-mismatch capture follow an ALREADY-REAL failure, not +// a launch race, and plenty of unit tests stub a throwing `snapshot` dispatch +// there expecting an immediate `capture-failed` result (repo convention: unit +// tests must not wait real time). Retrying unconditionally would force real +// wall-clock waits onto every one of those. +const DIVERGENCE_CAPTURE_RETRY_DEADLINE_MS = 12_000; +const DIVERGENCE_CAPTURE_RETRY_DELAYS_MS = [300, 500, 800, 1200, 2000, 3000, 4000] as const; + export async function captureDivergenceObservation(params: { session: SessionState; sessionName: string; sessionStore: SessionStore; logPath: string; action: ReplayReportAction; + retryLaunchRace?: boolean; }): Promise { - const { session, sessionName, sessionStore, logPath, action } = params; + const { session, sessionName, sessionStore, logPath, action, retryLaunchRace = false } = params; const flags = divergenceCaptureFlags(action); + + let observation = await captureDivergenceObservationAttempt({ + session, + sessionName, + sessionStore, + logPath, + flags, + }); + if (!retryLaunchRace) return observation; + + const deadline = Date.now() + DIVERGENCE_CAPTURE_RETRY_DEADLINE_MS; + for (const delayMs of DIVERGENCE_CAPTURE_RETRY_DELAYS_MS) { + if (observation.state === 'available') break; + const remainingMs = deadline - Date.now(); + if (remainingMs <= 0) break; + await sleep(Math.min(delayMs, remainingMs)); + observation = await captureDivergenceObservationAttempt({ + session, + sessionName, + sessionStore, + logPath, + flags, + }); + } + + return observation; +} + +async function captureDivergenceObservationAttempt(params: { + session: SessionState; + sessionName: string; + sessionStore: SessionStore; + logPath: string; + flags: CommandFlags; +}): Promise { + const { session, sessionName, sessionStore, logPath, flags } = params; try { const capture = await captureSnapshot({ device: session.device, diff --git a/src/daemon/handlers/session-replay-target-verification.ts b/src/daemon/handlers/session-replay-target-verification.ts index 82fc361630..ca2da1d7fc 100644 --- a/src/daemon/handlers/session-replay-target-verification.ts +++ b/src/daemon/handlers/session-replay-target-verification.ts @@ -317,12 +317,18 @@ export async function verifyReplayActionTarget(params: { return { verified: false, response: await buildRecordedUnverifiableResponse() }; } + // #1385: this is the pre-dispatch gate a step right after `open --relaunch` + // can race — the app may still be launching/mounting when this capture + // lands, producing a transient `capture-failed` / `sparse-snapshot` + // verdict that is not a real divergence. Bounded retry rides out that + // transition instead of failing closed on the first unlucky capture. const observation = await captureDivergenceObservation({ session, sessionName, sessionStore, logPath, action, + retryLaunchRace: true, }); if (observation.state !== 'available') { return { From 712bada5a55bce5774b272c494ff9a20507517e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 13:27:28 +0200 Subject: [PATCH 2/5] fix(replay): gate #1385 launch-race retry on content-quality vs mechanism failure Address review feedback on #1386: the bounded retry was retrying every thrown capture-failed, including permanent mechanism failures (e.g. a missing helper artifact) that a retry can never fix, at the cost of the full backoff budget before the identical divergence. Gate the retry on the same signal Android's helper capture path already emits for this exact distinction: retriable:true on a content-poor rejection (rejectAndroidHelperContentUnavailable), unset on a permanent one (androidSnapshotHelperUnavailableError). The non-throwing sparse-snapshot verdict still always retries. Mirrors #1381's isUnreadableCaptureContentError taxonomy for the wait keep-poll loop. Also update the two #1385 tests to use a retriable-tagged error and add a case proving a permanent failure fails on the first attempt. --- docs/adr/0012-interactive-replay.md | 10 +++ ...replay-target-verification-runtime.test.ts | 83 ++++++++++++++----- .../handlers/session-replay-divergence.ts | 62 ++++++++++---- 3 files changed, 120 insertions(+), 35 deletions(-) diff --git a/docs/adr/0012-interactive-replay.md b/docs/adr/0012-interactive-replay.md index bd2db32e92..faf17aed11 100644 --- a/docs/adr/0012-interactive-replay.md +++ b/docs/adr/0012-interactive-replay.md @@ -384,6 +384,16 @@ target-binding divergences reported before the device action. This is not genera > `verifyReplayActionTarget` races a launch this way — the post-failure diagnostic capture > (`buildReplayFailureDivergence`) and the post-resolution guard-mismatch capture both follow an > already-real failure, where retrying would only delay an already-decided divergence. +> +> The retry loop is further gated on the SAME content-quality-vs-mechanism-failure taxonomy #1381 draws +> for the wait keep-poll loop (`isUnreadableCaptureContentError`): the non-throwing `sparse-snapshot` +> verdict always retries (it is already a content-quality signal), but a thrown `capture-failed` only +> retries when the underlying error is explicitly marked `retriable: true` — the SAME signal Android's +> helper capture path already emits for a content-poor/system-window-only rejection +> (`rejectAndroidHelperContentUnavailable`, `platforms/android/snapshot.ts`) and leaves unset for a +> permanent one (the helper artifact itself missing, `androidSnapshotHelperUnavailableError`). A +> mechanism failure — helper artifact missing, device offline — therefore still fails on the first +> attempt rather than spending the full 12s retry budget on a foregone conclusion. ### 4. Divergence wire contract and replay-only resume diff --git a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts index 15d4e97c53..29f6e98702 100644 --- a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts @@ -30,6 +30,7 @@ import { runReplayScriptFile } from '../session-replay-runtime.ts'; import { SessionStore } from '../../session-store.ts'; import type { DaemonRequest } from '../../types.ts'; import { dispatchCommand } from '../../../core/dispatch.ts'; +import { AppError } from '../../../kernel/errors.ts'; import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; const mockDispatchCommand = vi.mocked(dispatchCommand); @@ -497,28 +498,42 @@ test('a target-binding divergence carries a real computed resume (step 5 wiring, }); // #1385: a step-2 press right after `open --relaunch` can capture while the -// app is still launching/mounting — the pre-dispatch verification capture -// throws (Android's "insufficient foreground app content" surfaces as a -// thrown capture failure) or returns sparse on the first attempt(s), then -// recovers once the app finishes mounting. The bounded retry in -// `captureDivergenceObservation` (`retryLaunchRace`) must ride that out -// instead of failing closed on the first unlucky capture. -test('a transient pre-dispatch capture failure right after launch recovers within the bounded retry, and the action still dispatches', async () => { +// app is still launching/mounting. The bounded retry in +// `captureDivergenceObservation` (`retryLaunchRace`) only rides out a +// content-quality verdict — the same `retriable: true` signal Android's +// helper capture path already emits for a content-poor/system-window-only +// rejection (`rejectAndroidHelperContentUnavailable`) — not a mechanism +// failure (helper artifact missing, device offline), which must still fail +// fast. `throwRetriableCaptureFailure`/`throwPermanentCaptureFailure` below +// build the two shapes. +function throwRetriableCaptureFailure(): never { + throw new AppError( + 'COMMAND_FAILED', + 'Android snapshot helper returned insufficient foreground app content', + { retriable: true }, + ); +} + +function throwPermanentCaptureFailure(): never { + throw new AppError( + 'COMMAND_FAILED', + 'Android snapshot helper is unavailable: the bundled helper artifact was not found', + ); +} + +test('a transient content-quality capture failure right after launch recovers within the bounded retry, and the action still dispatches', async () => { const root = fs.mkdtempSync( path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-recover-'), ); const { sessionStore, sessionName } = setupSession(root); const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); - // First two captures fail closed (mirrors a mid-launch/mid-mount capture); - // the third lands after the app settles and finds the recorded target. + // First two captures fail closed with a content-quality (retriable) verdict + // (mirrors a mid-launch/mid-mount capture); the third lands after the app + // settles and finds the recorded target. mockDispatchCommand - .mockImplementationOnce(async () => { - throw new Error('Android snapshot helper returned insufficient foreground app content'); - }) - .mockImplementationOnce(async () => { - throw new Error('Android snapshot helper returned insufficient foreground app content'); - }) + .mockImplementationOnce(async () => throwRetriableCaptureFailure()) + .mockImplementationOnce(async () => throwRetriableCaptureFailure()) .mockResolvedValue({ nodes: [ { @@ -551,16 +566,14 @@ test('a transient pre-dispatch capture failure right after launch recovers withi expect(mockDispatchCommand).toHaveBeenCalledTimes(3); }); -test('a pre-dispatch capture failure that never recovers still fails closed as identity-unverifiable once the bounded retry is exhausted', async () => { +test('a content-quality capture failure that never recovers still fails closed as identity-unverifiable once the bounded retry is exhausted', async () => { const root = fs.mkdtempSync( path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-exhausted-'), ); const { sessionStore, sessionName } = setupSession(root); const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); - mockDispatchCommand.mockImplementation(async () => { - throw new Error('Android snapshot helper returned insufficient foreground app content'); - }); + mockDispatchCommand.mockImplementation(async () => throwRetriableCaptureFailure()); const invoked: DaemonRequest[] = []; const response = await runReplayScriptFile({ @@ -587,6 +600,38 @@ test('a pre-dispatch capture failure that never recovers still fails closed as i expect(mockDispatchCommand.mock.calls.length).toBeGreaterThan(1); }); +test('a permanent (non-content-quality) capture failure fails fast without retrying', async () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-permanent-'), + ); + const { sessionStore, sessionName } = setupSession(root); + const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); + + // A missing helper artifact is not something a launch-race retry can fix — + // it never carries `retriable: true`, so the capture must fail on the + // FIRST attempt, not spend the retry budget on a foregone conclusion. + mockDispatchCommand.mockImplementation(async () => throwPermanentCaptureFailure()); + + const invoked: DaemonRequest[] = []; + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + invoked.push(req); + return { ok: true, data: {} }; + }, + }); + + expect(invoked.length).toBe(0); + expect(response.ok).toBe(false); + if (response.ok) return; + const divergence = response.error.details?.divergence as Record; + expect(divergence.kind).toBe('identity-unverifiable'); + expect(mockDispatchCommand).toHaveBeenCalledTimes(1); +}); + // --------------------------------------------------------------------------- // #1349: wait's post-resolution landmark verification. An annotated selector // wait must NEVER enter the generic pre-dispatch resolution path — polling is diff --git a/src/daemon/handlers/session-replay-divergence.ts b/src/daemon/handlers/session-replay-divergence.ts index 721b00e8b3..057876a29f 100644 --- a/src/daemon/handlers/session-replay-divergence.ts +++ b/src/daemon/handlers/session-replay-divergence.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { sleep } from '../../utils/timeouts.ts'; +import { normalizeError } from '../../kernel/errors.ts'; import { markSessionPartialRefsIssued, setSessionSnapshot } from '../session-snapshot.ts'; import { isSparseSnapshotQualityVerdict } from '../../snapshot/snapshot-quality.ts'; import { displayLabel, formatRole } from '../../snapshot/snapshot-lines.ts'; @@ -248,22 +249,22 @@ export async function captureDivergenceObservation(params: { const { session, sessionName, sessionStore, logPath, action, retryLaunchRace = false } = params; const flags = divergenceCaptureFlags(action); - let observation = await captureDivergenceObservationAttempt({ + let attempt = await captureDivergenceObservationAttempt({ session, sessionName, sessionStore, logPath, flags, }); - if (!retryLaunchRace) return observation; + if (!retryLaunchRace) return attempt.observation; const deadline = Date.now() + DIVERGENCE_CAPTURE_RETRY_DEADLINE_MS; for (const delayMs of DIVERGENCE_CAPTURE_RETRY_DELAYS_MS) { - if (observation.state === 'available') break; + if (attempt.observation.state === 'available' || !attempt.retryable) break; const remainingMs = deadline - Date.now(); if (remainingMs <= 0) break; await sleep(Math.min(delayMs, remainingMs)); - observation = await captureDivergenceObservationAttempt({ + attempt = await captureDivergenceObservationAttempt({ session, sessionName, sessionStore, @@ -272,16 +273,36 @@ export async function captureDivergenceObservation(params: { }); } - return observation; + return attempt.observation; } +type DivergenceCaptureAttempt = { + observation: DivergenceObservation; + /** + * Meaningful only when `observation.state === 'unavailable'`: whether this + * particular failure is a content-quality verdict a launch-race retry + * should ride out, versus a mechanism failure that will not resolve itself + * (helper artifact missing, device offline). Mirrors #1381's + * `isUnreadableCaptureContentError` taxonomy for the wait keep-poll loop — + * content verdict retries, mechanism failure fails fast — via the SAME + * signal Android's helper capture path already emits for exactly this + * distinction: `retriable: true` on a content-poor/system-window-only + * rejection (`rejectAndroidHelperContentUnavailable`, + * `platforms/android/snapshot.ts`), left unset on a permanent one (the + * helper artifact itself missing, `androidSnapshotHelperUnavailableError`). + * The non-throwing `sparse-snapshot` verdict is always retryable — it is + * already a content-quality signal, never a mechanism failure. + */ + retryable: boolean; +}; + async function captureDivergenceObservationAttempt(params: { session: SessionState; sessionName: string; sessionStore: SessionStore; logPath: string; flags: CommandFlags; -}): Promise { +}): Promise { const { session, sessionName, sessionStore, logPath, flags } = params; try { const capture = await captureSnapshot({ @@ -293,9 +314,12 @@ async function captureDivergenceObservationAttempt(params: { const snapshot = capture.snapshot; if (isSparseSnapshotQualityVerdict(snapshot.snapshotQuality)) { return { - state: 'unavailable', - reason: 'sparse-snapshot', - hint: 'The post-failure snapshot was sparse or unavailable; run snapshot -i to observe the current screen.', + observation: { + state: 'unavailable', + reason: 'sparse-snapshot', + hint: 'The post-failure snapshot was sparse or unavailable; run snapshot -i to observe the current screen.', + }, + retryable: true, }; } setSessionSnapshot(session, snapshot); @@ -313,16 +337,22 @@ async function captureDivergenceObservationAttempt(params: { markSessionPartialRefsIssued(session, digestBodies); sessionStore.set(sessionName, session); return { - state: 'available', - nodes: snapshot.nodes, - refsGeneration: session.snapshotGeneration ?? 0, - appBundleId: session.appBundleId, + observation: { + state: 'available', + nodes: snapshot.nodes, + refsGeneration: session.snapshotGeneration ?? 0, + appBundleId: session.appBundleId, + }, + retryable: false, }; } catch (error) { return { - state: 'unavailable', - reason: 'capture-failed', - hint: `Post-failure snapshot capture failed (${error instanceof Error ? error.message : String(error)}); the original replay failure is unaffected.`, + observation: { + state: 'unavailable', + reason: 'capture-failed', + hint: `Post-failure snapshot capture failed (${error instanceof Error ? error.message : String(error)}); the original replay failure is unaffected.`, + }, + retryable: normalizeError(error).retriable === true, }; } } From 4de8628b8ad172e4fb55403fe2d283d117a78002 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 16:23:17 +0200 Subject: [PATCH 3/5] fix(replay): narrow #1385 retry to a real content-quality taxonomy, anchor the deadline Address second review pass on #1386: P1 - `retriable === true` was too broad: Android's adb layer (adb-executor.ts) marks genuine transport mechanism failures retriable too (connection_dropped, device_offline, server_version_mismatch - an unchanged retry of the SAME adb command can succeed there), so those consumed the launch-race retry budget despite being exactly the mechanism failures the ADR says must fail fast. Replace the check with the narrow discriminator: the thrown error's androidSnapshotHelperFailureReason must be one of the three literal content verdicts rejectAndroidHelperContentUnavailable attaches (empty-helper-output, system-window-only, content-poor-app-window) - never the free-form message androidSnapshotHelperCaptureError sets for a crashed/timed-out helper, and never present at all for a permanently missing helper artifact. Added a regression test for an adb mechanism failure marked retriable at the transport level, proving it still fails on the first attempt. P2 - the 12s deadline started AFTER the first (unbounded) capture attempt, so the effective wall-clock cost was "first capture + 12s of retries", undocumented and untested as such. Anchor the deadline before the first attempt instead, and rewrite the comments/ADR to state precisely what is and is not bounded: the delay list caps attempt count, the deadline caps sleep time from entry, neither caps an individual capture's own duration. Added a fake-timer test proving the deadline (not just the delay array's length) is what stops retries when captures themselves consume real time. --- docs/adr/0012-interactive-replay.md | 33 +++++--- .../session-replay-divergence.test.ts | 62 +++++++++++++- ...replay-target-verification-runtime.test.ts | 82 +++++++++++++++---- .../handlers/session-replay-divergence.ts | 72 ++++++++++++---- 4 files changed, 205 insertions(+), 44 deletions(-) diff --git a/docs/adr/0012-interactive-replay.md b/docs/adr/0012-interactive-replay.md index faf17aed11..077c0c4d33 100644 --- a/docs/adr/0012-interactive-replay.md +++ b/docs/adr/0012-interactive-replay.md @@ -376,11 +376,11 @@ target-binding divergences reported before the device action. This is not genera > launching/mounting when it lands, producing a transient content-quality verdict — `capture-failed` > (Android's snapshot helper returns "insufficient foreground app content" while the app is mounting, and > the capture path throws) or `sparse-snapshot` (iOS's private-AX fallback under load) — that is not a -> real divergence, only an unlucky capture. This capture now retries with a bounded backoff (fixed delay -> list, capped at a 12s wall-clock deadline) before falling through to `identity-unverifiable`, mirroring -> the keep-polling semantics `wait`'s recorded-landmark identity verification (#1349) already applies on -> its own (post-resolution) path. The retry is opt-in per call site (`retryLaunchRace`), not a change to -> every `captureDivergenceObservation` caller: only the pre-dispatch verification gate in +> real divergence, only an unlucky capture. This capture now retries with a bounded backoff (fixed +> 7-entry delay list, 12s DELAY-ONLY budget — see below) before falling through to `identity-unverifiable`, +> mirroring the keep-polling semantics `wait`'s recorded-landmark identity verification (#1349) already +> applies on its own (post-resolution) path. The retry is opt-in per call site (`retryLaunchRace`), not a +> change to every `captureDivergenceObservation` caller: only the pre-dispatch verification gate in > `verifyReplayActionTarget` races a launch this way — the post-failure diagnostic capture > (`buildReplayFailureDivergence`) and the post-resolution guard-mismatch capture both follow an > already-real failure, where retrying would only delay an already-decided divergence. @@ -388,12 +388,23 @@ target-binding divergences reported before the device action. This is not genera > The retry loop is further gated on the SAME content-quality-vs-mechanism-failure taxonomy #1381 draws > for the wait keep-poll loop (`isUnreadableCaptureContentError`): the non-throwing `sparse-snapshot` > verdict always retries (it is already a content-quality signal), but a thrown `capture-failed` only -> retries when the underlying error is explicitly marked `retriable: true` — the SAME signal Android's -> helper capture path already emits for a content-poor/system-window-only rejection -> (`rejectAndroidHelperContentUnavailable`, `platforms/android/snapshot.ts`) and leaves unset for a -> permanent one (the helper artifact itself missing, `androidSnapshotHelperUnavailableError`). A -> mechanism failure — helper artifact missing, device offline — therefore still fails on the first -> attempt rather than spending the full 12s retry budget on a foregone conclusion. +> retries when the underlying error's `androidSnapshotHelperFailureReason` is one of the three literal +> codes `rejectAndroidHelperContentUnavailable` (`platforms/android/snapshot.ts`) attaches to a +> content-poor/system-window-only rejection — `empty-helper-output`, `system-window-only`, +> `content-poor-app-window` (mirroring `AndroidHelperContentRecoveryDecision['reason']`, +> `platforms/android/snapshot-content-recovery.ts`). This is deliberately narrower than the error's own +> generic `retriable` flag: Android's adb layer separately marks true mechanism failures retriable too +> (`connection_dropped`, `device_offline`, `server_version_mismatch` — an unchanged retry of the SAME adb +> command can succeed there), and a helper artifact permanently missing +> (`androidSnapshotHelperUnavailableError`) carries neither signal. A mechanism failure therefore still +> fails on the first attempt rather than spending the retry budget on a foregone conclusion, regardless of +> what its own `retriable` flag says. +> +> The 12s budget is a DELAY-ONLY bound, anchored at the first capture attempt: it caps how long this loop +> sleeps between retries, not how long any individual capture attempt itself may run (a capture already +> carries its own platform-level timeouts this loop does not shorten or re-implement). The fixed-length +> delay array is a separate, independent bound on attempt COUNT, so a mocked-instant `sleep` in unit tests +> cannot turn this into a real-time busy-loop. ### 4. Divergence wire contract and replay-only resume diff --git a/src/daemon/handlers/__tests__/session-replay-divergence.test.ts b/src/daemon/handlers/__tests__/session-replay-divergence.test.ts index 825253edff..33dff3b8b1 100644 --- a/src/daemon/handlers/__tests__/session-replay-divergence.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-divergence.test.ts @@ -14,7 +14,8 @@ vi.mock('../../../core/dispatch.ts', async (importOriginal) => { // in `snapshot-capture.ts` (`capturePostActionAwareSnapshot`) awaits; making it // instant does not change control flow — the loop still runs, retries, and // re-captures — so the test still proves two dispatches and use of the retried -// tree. No other test in this file triggers a sleep path. +// tree. The #1385 `retryLaunchRace` retry loop (`captureDivergenceObservation`) +// awaits this SAME `sleep`, further down in this file. vi.mock('../../../utils/timeouts.ts', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, sleep: vi.fn(async () => {}) }; @@ -22,6 +23,7 @@ vi.mock('../../../utils/timeouts.ts', async (importOriginal) => { import { dispatchCommand } from '../../../core/dispatch.ts'; import type { RawSnapshotNode } from '../../../kernel/snapshot.ts'; +import { AppError } from '../../../kernel/errors.ts'; import { makeAndroidSession, makeIosSession, @@ -32,7 +34,10 @@ import { walkNonRawAndroidFixture, } from '../../../__tests__/test-utils/android-ui-hierarchy-fixtures.ts'; import { SessionStore } from '../../session-store.ts'; -import { buildReplayFailureDivergence } from '../session-replay-divergence.ts'; +import { + buildReplayFailureDivergence, + captureDivergenceObservation, +} from '../session-replay-divergence.ts'; const mockDispatchCommand = vi.mocked(dispatchCommand); @@ -1135,3 +1140,56 @@ test.each([ expect(screen.refs.some((ref) => ref.label === 'Battery charging, 100 percent.')).toBe(false); expect(screen.refs.some((ref) => ref.label === 'Wifi signal full.')).toBe(false); }); + +// #1385 P2: the retry deadline is a DELAY-ONLY budget, not a per-attempt +// capture timeout — this loop does not itself bound how long a single +// capture attempt runs, only how much SLEEP time it spends between attempts +// (measured from the first attempt). This test proves that bound is real +// wall-clock behavior, not merely "run the fixed 7-entry delay list": each +// mocked capture attempt here advances the (fake) system clock by 5s to +// stand in for a slow capture, so the 12s deadline is exhausted after just 3 +// attempts — far short of the 8 attempts (1 + 7 delays) the array alone would +// allow — proving the deadline, not the array length, is what stopped it. +test('captureDivergenceObservation retryLaunchRace: the 12s deadline bounds retries even when captures themselves consume wall-clock time', async () => { + vi.useFakeTimers(); + try { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replay-divergence-deadline-')); + const sessionStore = new SessionStore(path.join(root, 'sessions')); + const sessionName = 'default'; + sessionStore.set(sessionName, makeIosSession(sessionName, { appBundleId: 'com.example.app' })); + + mockDispatchCommand.mockImplementation(async () => { + vi.setSystemTime(new Date(Date.now() + 5_000)); + throw new AppError( + 'COMMAND_FAILED', + 'Android snapshot helper returned insufficient foreground app content', + { androidSnapshotHelperFailureReason: 'content-poor-app-window', retriable: true }, + ); + }); + + const action = { + ts: 0, + command: 'click', + positionals: ['label="Save"'], + flags: {}, + result: { selectorChain: ['label="Save"', 'id="save"'] }, + }; + + const observation = await captureDivergenceObservation({ + session: sessionStore.get(sessionName)!, + sessionName, + sessionStore, + logPath: path.join(root, 'daemon.log'), + action, + retryLaunchRace: true, + }); + + expect(observation.state).toBe('unavailable'); + // 1 initial attempt + 2 retries: the 3rd retry's pre-sleep remaining-budget + // check sees the deadline already passed (3 x 5s = 15s > 12s) and breaks + // BEFORE a 4th capture — not the 8 attempts the delay array alone permits. + expect(mockDispatchCommand).toHaveBeenCalledTimes(3); + } finally { + vi.useRealTimers(); + } +}); diff --git a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts index 29f6e98702..3b056c9857 100644 --- a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts @@ -500,17 +500,20 @@ test('a target-binding divergence carries a real computed resume (step 5 wiring, // #1385: a step-2 press right after `open --relaunch` can capture while the // app is still launching/mounting. The bounded retry in // `captureDivergenceObservation` (`retryLaunchRace`) only rides out a -// content-quality verdict — the same `retriable: true` signal Android's -// helper capture path already emits for a content-poor/system-window-only -// rejection (`rejectAndroidHelperContentUnavailable`) — not a mechanism -// failure (helper artifact missing, device offline), which must still fail -// fast. `throwRetriableCaptureFailure`/`throwPermanentCaptureFailure` below -// build the two shapes. -function throwRetriableCaptureFailure(): never { +// content-quality verdict — Android's helper path attaches +// `androidSnapshotHelperFailureReason` as one of exactly three literals +// ('empty-helper-output' | 'system-window-only' | 'content-poor-app-window') +// on that verdict (`rejectAndroidHelperContentUnavailable`) — never a +// mechanism failure (helper artifact missing, device offline, adb connection +// dropped), which must still fail fast even when the underlying error +// separately carries `retriable: true` for its own (adb-level, same-command) +// retry semantics. The three `throw*` helpers below build the three shapes +// this distinction must tell apart. +function throwContentQualityCaptureFailure(): never { throw new AppError( 'COMMAND_FAILED', 'Android snapshot helper returned insufficient foreground app content', - { retriable: true }, + { androidSnapshotHelperFailureReason: 'content-poor-app-window', retriable: true }, ); } @@ -521,6 +524,19 @@ function throwPermanentCaptureFailure(): never { ); } +function throwAdbMechanismFailureMarkedRetriable(): never { + // Mirrors `classifyAdbFailure`'s `connection_dropped` entry + // (adb-executor.ts): `retriable: true` at the adb-transport level (retrying + // the SAME adb command can succeed), but not evidence of a content-quality + // capture verdict — a launch-race retry has no reason to believe THIS + // failure resolves itself, and must not be fooled by the bare `retriable` + // flag into treating it as one. + throw new AppError('COMMAND_FAILED', 'adb: transport error', { + adbFailure: 'connection_dropped', + retriable: true, + }); +} + test('a transient content-quality capture failure right after launch recovers within the bounded retry, and the action still dispatches', async () => { const root = fs.mkdtempSync( path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-recover-'), @@ -528,12 +544,12 @@ test('a transient content-quality capture failure right after launch recovers wi const { sessionStore, sessionName } = setupSession(root); const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); - // First two captures fail closed with a content-quality (retriable) verdict - // (mirrors a mid-launch/mid-mount capture); the third lands after the app - // settles and finds the recorded target. + // First two captures fail closed with a content-quality verdict (mirrors a + // mid-launch/mid-mount capture); the third lands after the app settles and + // finds the recorded target. mockDispatchCommand - .mockImplementationOnce(async () => throwRetriableCaptureFailure()) - .mockImplementationOnce(async () => throwRetriableCaptureFailure()) + .mockImplementationOnce(async () => throwContentQualityCaptureFailure()) + .mockImplementationOnce(async () => throwContentQualityCaptureFailure()) .mockResolvedValue({ nodes: [ { @@ -573,7 +589,7 @@ test('a content-quality capture failure that never recovers still fails closed a const { sessionStore, sessionName } = setupSession(root); const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); - mockDispatchCommand.mockImplementation(async () => throwRetriableCaptureFailure()); + mockDispatchCommand.mockImplementation(async () => throwContentQualityCaptureFailure()); const invoked: DaemonRequest[] = []; const response = await runReplayScriptFile({ @@ -608,8 +624,9 @@ test('a permanent (non-content-quality) capture failure fails fast without retry const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); // A missing helper artifact is not something a launch-race retry can fix — - // it never carries `retriable: true`, so the capture must fail on the - // FIRST attempt, not spend the retry budget on a foregone conclusion. + // it never carries `androidSnapshotHelperFailureReason` at all, so the + // capture must fail on the FIRST attempt, not spend the retry budget on a + // foregone conclusion. mockDispatchCommand.mockImplementation(async () => throwPermanentCaptureFailure()); const invoked: DaemonRequest[] = []; @@ -632,6 +649,39 @@ test('a permanent (non-content-quality) capture failure fails fast without retry expect(mockDispatchCommand).toHaveBeenCalledTimes(1); }); +test('an adb mechanism failure marked retriable at the transport level still fails fast (retriable is not a content-quality signal)', async () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-adb-mechanism-'), + ); + const { sessionStore, sessionName } = setupSession(root); + const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); + + // A dropped adb connection carries `retriable: true` (retrying the SAME adb + // command can succeed) but no `androidSnapshotHelperFailureReason` content + // verdict — the launch-race retry must not treat the bare `retriable` flag + // as proof this is a "the app is still mounting" content-quality failure. + mockDispatchCommand.mockImplementation(async () => throwAdbMechanismFailureMarkedRetriable()); + + const invoked: DaemonRequest[] = []; + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + invoked.push(req); + return { ok: true, data: {} }; + }, + }); + + expect(invoked.length).toBe(0); + expect(response.ok).toBe(false); + if (response.ok) return; + const divergence = response.error.details?.divergence as Record; + expect(divergence.kind).toBe('identity-unverifiable'); + expect(mockDispatchCommand).toHaveBeenCalledTimes(1); +}); + // --------------------------------------------------------------------------- // #1349: wait's post-resolution landmark verification. An annotated selector // wait must NEVER enter the generic pre-dispatch resolution path — polling is diff --git a/src/daemon/handlers/session-replay-divergence.ts b/src/daemon/handlers/session-replay-divergence.ts index 057876a29f..9b517b2397 100644 --- a/src/daemon/handlers/session-replay-divergence.ts +++ b/src/daemon/handlers/session-replay-divergence.ts @@ -2,6 +2,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { sleep } from '../../utils/timeouts.ts'; import { normalizeError } from '../../kernel/errors.ts'; +import type { AndroidHelperContentRecoveryDecision } from '../../platforms/android/snapshot-content-recovery.ts'; import { markSessionPartialRefsIssued, setSessionSnapshot } from '../session-snapshot.ts'; import { isSparseSnapshotQualityVerdict } from '../../snapshot/snapshot-quality.ts'; import { displayLabel, formatRole } from '../../snapshot/snapshot-lines.ts'; @@ -222,11 +223,19 @@ export function toReplayRepairHintCapture( // target-verification capture (`verifyReplayActionTarget`) right after an // `open --relaunch` step is often just the app still launching/mounting, not // a real divergence — the SAME transition `wait`'s keep-polling landmark -// verification (#1349) rides out on its own (post-resolution) path. Bounded -// by BOTH a wall-clock deadline and a fixed delay-list length, so a -// mocked-instant `sleep` in tests cannot turn this into a busy-loop — it just -// runs the list once (mirrors the Android freshness retry shape in -// `snapshot-capture.ts`). +// verification (#1349) rides out on its own (post-resolution) path. +// +// The `DIVERGENCE_CAPTURE_RETRY_DEADLINE_MS` budget is a DELAY-ONLY bound: it +// caps how long this loop SLEEPS between attempts (measured from the first +// attempt, so a slow first capture eats into the same budget rather than +// getting a free 12s on top), not how long any individual capture attempt +// itself may run — a capture already carries its own platform-level bounds +// (adb/xcodebuild command timeouts) this loop does not re-implement or +// shorten. The fixed-length `DIVERGENCE_CAPTURE_RETRY_DELAYS_MS` array is a +// SEPARATE, independent bound: it caps the attempt COUNT regardless of the +// deadline, so a mocked-instant `sleep` in tests cannot turn this into a +// busy-loop racing real time — it just runs the list once (mirrors the +// Android freshness retry shape in `snapshot-capture.ts`). // // Opt-in via `retryLaunchRace`, NOT the default for every caller: the // post-failure diagnostic capture (`buildReplayFailureDivergence`) and the @@ -248,6 +257,10 @@ export async function captureDivergenceObservation(params: { }): Promise { const { session, sessionName, sessionStore, logPath, action, retryLaunchRace = false } = params; const flags = divergenceCaptureFlags(action); + // Anchored BEFORE the first attempt, not after: a slow first capture + // shrinks the retry budget rather than getting a free `DEADLINE_MS` on top + // of however long it took. + const deadline = Date.now() + DIVERGENCE_CAPTURE_RETRY_DEADLINE_MS; let attempt = await captureDivergenceObservationAttempt({ session, @@ -258,7 +271,6 @@ export async function captureDivergenceObservation(params: { }); if (!retryLaunchRace) return attempt.observation; - const deadline = Date.now() + DIVERGENCE_CAPTURE_RETRY_DEADLINE_MS; for (const delayMs of DIVERGENCE_CAPTURE_RETRY_DELAYS_MS) { if (attempt.observation.state === 'available' || !attempt.retryable) break; const remainingMs = deadline - Date.now(); @@ -276,20 +288,50 @@ export async function captureDivergenceObservation(params: { return attempt.observation; } +// The ONLY three reason codes `rejectAndroidHelperContentUnavailable` +// (`platforms/android/snapshot.ts`) attaches as +// `androidSnapshotHelperFailureReason` — mirrors +// `AndroidHelperContentRecoveryDecision['reason']` +// (`platforms/android/snapshot-content-recovery.ts`), the SOLE site that +// classifies a structurally-succeeded helper capture as too thin/foreign to +// trust: the helper produced accessibility XML, but its content doesn't meet +// the bar. This is deliberately narrower than the error's own `retriable` +// flag: Android's adb layer (`adb-executor.ts`) ALSO marks transport-level +// mechanism failures retriable (`connection_dropped`, `device_offline`, +// `server_version_mismatch`) because an unchanged retry of the SAME adb +// command can succeed there — a true statement about adb, but not evidence +// this specific capture failure is a content-quality verdict rather than a +// genuine mechanism failure (a crashed/timed-out helper invocation, a +// dropped adb connection, a missing helper artifact) a launch-race retry +// cannot fix. `androidSnapshotHelperCaptureError`'s generic capture-failure +// wrap (`rejectAndroidHelperCaptureFailure`) ALSO sets +// `androidSnapshotHelperFailureReason` — but to an arbitrary formatted +// failure message, never one of these three literals, so this exact-value +// membership check does not also catch that unrelated, non-content-quality +// case. +const ANDROID_CAPTURE_CONTENT_QUALITY_REASONS: ReadonlySet< + AndroidHelperContentRecoveryDecision['reason'] +> = new Set(['empty-helper-output', 'system-window-only', 'content-poor-app-window']); + +function isCaptureContentQualityFailure(error: unknown): boolean { + const reason = normalizeError(error).details?.androidSnapshotHelperFailureReason; + return ( + typeof reason === 'string' && + ANDROID_CAPTURE_CONTENT_QUALITY_REASONS.has( + reason as AndroidHelperContentRecoveryDecision['reason'], + ) + ); +} + type DivergenceCaptureAttempt = { observation: DivergenceObservation; /** * Meaningful only when `observation.state === 'unavailable'`: whether this * particular failure is a content-quality verdict a launch-race retry * should ride out, versus a mechanism failure that will not resolve itself - * (helper artifact missing, device offline). Mirrors #1381's - * `isUnreadableCaptureContentError` taxonomy for the wait keep-poll loop — - * content verdict retries, mechanism failure fails fast — via the SAME - * signal Android's helper capture path already emits for exactly this - * distinction: `retriable: true` on a content-poor/system-window-only - * rejection (`rejectAndroidHelperContentUnavailable`, - * `platforms/android/snapshot.ts`), left unset on a permanent one (the - * helper artifact itself missing, `androidSnapshotHelperUnavailableError`). + * (helper artifact missing, device offline, adb connection dropped). + * Mirrors #1381's `isUnreadableCaptureContentError` taxonomy for the wait + * keep-poll loop — content verdict retries, mechanism failure fails fast. * The non-throwing `sparse-snapshot` verdict is always retryable — it is * already a content-quality signal, never a mechanism failure. */ @@ -352,7 +394,7 @@ async function captureDivergenceObservationAttempt(params: { reason: 'capture-failed', hint: `Post-failure snapshot capture failed (${error instanceof Error ? error.message : String(error)}); the original replay failure is unaffected.`, }, - retryable: normalizeError(error).retriable === true, + retryable: isCaptureContentQualityFailure(error), }; } } From fc60fd5b540f00879a57dbf4e65588d2df622e9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 11:43:27 +0200 Subject: [PATCH 4/5] test(replay): add targeted iOS sparse-snapshot retry regressions Address the reviewer's second finding on #1386: the Android capture-failed retry path had recover/exhaustion regressions, but the iOS sparse-snapshot verdict branch (isSparseSnapshotQualityVerdict, always-retryable, no thrown error involved) had no test of its own - only incidental coverage by way of the Android-shaped tests. Add the mirror pair: a sparse verdict that recovers within the bounded retry and dispatches, and one that never recovers and fails closed as identity-unverifiable once the retry is exhausted. --- ...replay-target-verification-runtime.test.ts | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts index 3b056c9857..efd7811406 100644 --- a/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts @@ -616,6 +616,105 @@ test('a content-quality capture failure that never recovers still fails closed a expect(mockDispatchCommand.mock.calls.length).toBeGreaterThan(1); }); +// #1385: the iOS side of the same launch race — the capture doesn't throw, +// it returns a structurally-valid tree flagged `sparse` (the private-AX +// fallback engaging under load). `isSparseSnapshotQualityVerdict` treats this +// as always-retryable, unlike the Android thrown-error branch, which is +// gated on the narrower content-quality taxonomy — so this needs its own +// regression, not just coverage by the Android capture-failed tests above. +function iosSparseCapture(): { + nodes: never[]; + truncated: false; + backend: 'xctest'; + quality: object; +} { + return { + nodes: [], + truncated: false, + backend: 'xctest', + quality: { state: 'sparse', backend: 'private-ax', reason: 'AX query rejected mid-transition' }, + }; +} + +test('an iOS sparse-snapshot verdict right after launch recovers within the bounded retry, and the action still dispatches', async () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-sparse-recover-'), + ); + const { sessionStore, sessionName } = setupSession(root); + const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); + + // First two captures return a structurally-valid but sparse-flagged tree + // (mirrors the private-AX fallback engaging mid-launch); the third lands + // after the app settles and finds the recorded target. + mockDispatchCommand + .mockImplementationOnce(async () => iosSparseCapture()) + .mockImplementationOnce(async () => iosSparseCapture()) + .mockResolvedValue({ + nodes: [ + { + index: 0, + depth: 0, + type: 'Button', + identifier: 'save', + label: 'Save', + rect: { x: 10, y: 10, width: 40, height: 20 }, + }, + ], + truncated: false, + backend: 'xctest', + }); + + const invoked: DaemonRequest[] = []; + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + invoked.push(req); + return { ok: true, data: {} }; + }, + }); + + expect(response.ok).toBe(true); + expect(invoked.map((req) => req.command)).toEqual(['click']); + expect(mockDispatchCommand).toHaveBeenCalledTimes(3); +}); + +test('an iOS sparse-snapshot verdict that never recovers still fails closed as identity-unverifiable once the bounded retry is exhausted', async () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-sparse-exhausted-'), + ); + const { sessionStore, sessionName } = setupSession(root); + const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']); + + mockDispatchCommand.mockImplementation(async () => iosSparseCapture()); + + const invoked: DaemonRequest[] = []; + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + invoked.push(req); + return { ok: true, data: {} }; + }, + }); + + expect(invoked.length).toBe(0); + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('REPLAY_DIVERGENCE'); + const divergence = response.error.details?.divergence as Record; + expect(divergence.kind).toBe('identity-unverifiable'); + const cause = divergence.cause as { code: string; message: string }; + expect(cause.code).toBe('IDENTITY_UNVERIFIABLE'); + expect(cause.message).toContain('sparse-snapshot'); + // Bounded: the retry gives up after its fixed delay list, not forever. + expect(mockDispatchCommand.mock.calls.length).toBeGreaterThan(1); +}); + test('a permanent (non-content-quality) capture failure fails fast without retrying', async () => { const root = fs.mkdtempSync( path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-permanent-'), From 4a10b4e520a20531368680b31a8b70f87ce19845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 12:32:35 +0200 Subject: [PATCH 5/5] fix(replay): re-consume shared #1381 predicate after rebase reintroduced local one The branch's rebase onto main re-resolved the #1381 conflict without carrying forward the earlier fix, reintroducing a local ANDROID_CAPTURE_CONTENT_QUALITY_REASONS/isCaptureContentQualityFailure duplicate of the shared isUnreadableCaptureContentError predicate (src/snapshot/snapshot-quality.ts, landed by #1381). Two parallel copies of the same taxonomy can silently drift. Drop the local duplicate and its Android-platform-type import; consume the shared predicate directly, matching what ADR 0012 already documents. --- .../handlers/session-replay-divergence.ts | 44 +++---------------- 1 file changed, 5 insertions(+), 39 deletions(-) diff --git a/src/daemon/handlers/session-replay-divergence.ts b/src/daemon/handlers/session-replay-divergence.ts index 9b517b2397..3ebf5ee182 100644 --- a/src/daemon/handlers/session-replay-divergence.ts +++ b/src/daemon/handlers/session-replay-divergence.ts @@ -1,10 +1,11 @@ import fs from 'node:fs'; import path from 'node:path'; import { sleep } from '../../utils/timeouts.ts'; -import { normalizeError } from '../../kernel/errors.ts'; -import type { AndroidHelperContentRecoveryDecision } from '../../platforms/android/snapshot-content-recovery.ts'; import { markSessionPartialRefsIssued, setSessionSnapshot } from '../session-snapshot.ts'; -import { isSparseSnapshotQualityVerdict } from '../../snapshot/snapshot-quality.ts'; +import { + isSparseSnapshotQualityVerdict, + isUnreadableCaptureContentError, +} from '../../snapshot/snapshot-quality.ts'; import { displayLabel, formatRole } from '../../snapshot/snapshot-lines.ts'; import { redactDiagnosticData } from '../../kernel/redaction.ts'; import type { CommandFlags } from '../../core/dispatch.ts'; @@ -288,41 +289,6 @@ export async function captureDivergenceObservation(params: { return attempt.observation; } -// The ONLY three reason codes `rejectAndroidHelperContentUnavailable` -// (`platforms/android/snapshot.ts`) attaches as -// `androidSnapshotHelperFailureReason` — mirrors -// `AndroidHelperContentRecoveryDecision['reason']` -// (`platforms/android/snapshot-content-recovery.ts`), the SOLE site that -// classifies a structurally-succeeded helper capture as too thin/foreign to -// trust: the helper produced accessibility XML, but its content doesn't meet -// the bar. This is deliberately narrower than the error's own `retriable` -// flag: Android's adb layer (`adb-executor.ts`) ALSO marks transport-level -// mechanism failures retriable (`connection_dropped`, `device_offline`, -// `server_version_mismatch`) because an unchanged retry of the SAME adb -// command can succeed there — a true statement about adb, but not evidence -// this specific capture failure is a content-quality verdict rather than a -// genuine mechanism failure (a crashed/timed-out helper invocation, a -// dropped adb connection, a missing helper artifact) a launch-race retry -// cannot fix. `androidSnapshotHelperCaptureError`'s generic capture-failure -// wrap (`rejectAndroidHelperCaptureFailure`) ALSO sets -// `androidSnapshotHelperFailureReason` — but to an arbitrary formatted -// failure message, never one of these three literals, so this exact-value -// membership check does not also catch that unrelated, non-content-quality -// case. -const ANDROID_CAPTURE_CONTENT_QUALITY_REASONS: ReadonlySet< - AndroidHelperContentRecoveryDecision['reason'] -> = new Set(['empty-helper-output', 'system-window-only', 'content-poor-app-window']); - -function isCaptureContentQualityFailure(error: unknown): boolean { - const reason = normalizeError(error).details?.androidSnapshotHelperFailureReason; - return ( - typeof reason === 'string' && - ANDROID_CAPTURE_CONTENT_QUALITY_REASONS.has( - reason as AndroidHelperContentRecoveryDecision['reason'], - ) - ); -} - type DivergenceCaptureAttempt = { observation: DivergenceObservation; /** @@ -394,7 +360,7 @@ async function captureDivergenceObservationAttempt(params: { reason: 'capture-failed', hint: `Post-failure snapshot capture failed (${error instanceof Error ? error.message : String(error)}); the original replay failure is unaffected.`, }, - retryable: isCaptureContentQualityFailure(error), + retryable: isUnreadableCaptureContentError(error), }; } }