diff --git a/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift b/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift index ca87a188f9..8465682d90 100644 --- a/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift +++ b/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift @@ -477,6 +477,19 @@ extension RunnerTests { } if let element = match.element { let frame = element.frame + // XCTest reports closed-drawer/off-viewport items as hittable, then + // "taps" coordinates outside the visible window as a silent no-op. + // Refuse instead; the daemon falls back to tree-based resolution, + // which can prefer an on-screen candidate or explain the off-screen + // state. The check uses the main window frame, not app.frame: on RN + // apps app.frame unions transformed subtrees (a closed drawer at + // negative x), so it happily "contains" unreachable coordinates. + if !match.usedNonHittableFallback + && !onScreenWindowFrame(app: activeApp).contains(CGPoint(x: frame.midX, y: frame.midY)) { + return Response(ok: false, error: ErrorPayload( + code: "ELEMENT_OFFSCREEN", + message: "element resolved off-screen at (\(Int(frame.midX)), \(Int(frame.midY)))")) + } let isTextEntry = isTextEntryElement(element) let touchFrame = frame.isEmpty ? nil diff --git a/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift b/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift index b505f682ce..0c7713d905 100644 --- a/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift +++ b/apple-runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift @@ -212,6 +212,21 @@ extension RunnerTests { return appFrame.contains(CGPoint(x: frame.midX, y: frame.midY)) } + // The tappable on-screen viewport. app.frame is unsuitable: it unions + // transformed subtrees, so a closed drawer at negative x inflates it and + // out-of-window coordinates still pass containment. Falls back to app.frame + // when no window frame is readable. + func onScreenWindowFrame(app: XCUIApplication) -> CGRect { + let window = app.windows.element(boundBy: 0) + if window.exists { + let frame = window.frame + if !frame.isEmpty { + return frame + } + } + return app.frame + } + func queryElement(app: XCUIApplication, selectorKey: String, selectorValue: String) -> Response { let match = findElement(app: app, selectorKey: selectorKey, selectorValue: selectorValue) if match.isAmbiguous { diff --git a/src/commands/interaction/runtime/interactions.test.ts b/src/commands/interaction/runtime/interactions.test.ts index bd4400a7ae..b0f8748511 100644 --- a/src/commands/interaction/runtime/interactions.test.ts +++ b/src/commands/interaction/runtime/interactions.test.ts @@ -212,6 +212,128 @@ test('runtime selector interactions fall back to a full snapshot when interactiv ]); }); +test('runtime press refuses a selector that resolves to an off-screen element', async () => { + // Closed-drawer shape: the only match sits fully left of the viewport. The + // @ref path already refuses this; the selector path must not silently tap + // out-of-viewport coordinates. + const offscreenSnapshot = makeSnapshotState([ + { + index: 0, + depth: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 400, height: 800 }, + hittable: true, + }, + { + index: 1, + depth: 2, + parentIndex: 0, + type: 'Button', + label: 'Explore', + rect: { x: -320, y: 240, width: 300, height: 50 }, + hittable: true, + }, + ]); + const taps: unknown[] = []; + const device = createInteractionDevice(offscreenSnapshot, { + tap: async (_context, point) => { + taps.push(point); + }, + }); + + await assert.rejects( + () => device.interactions.press(selector('label=Explore'), { session: 'default' }), + (error: unknown) => { + assert.ok(error instanceof Error); + assert.match(error.message, /off-screen element and is not safe to press/); + const details = (error as { details?: Record }).details; + assert.equal(details?.reason, 'offscreen_selector'); + assert.ok(typeof details?.hint === 'string'); + return true; + }, + ); + assert.equal(taps.length, 0); +}); + +test('runtime press with verify drops the non-hittable hint when evidence proves a change', async () => { + let captureCount = 0; + const nonHittableSnapshot = () => + makeSnapshotState([ + { + index: 0, + depth: 0, + type: 'Button', + label: 'Continue', + rect: { x: 10, y: 20, width: 100, height: 40 }, + hittable: false, + }, + ]); + const changedSnapshot = makeSnapshotState([ + { + index: 0, + depth: 0, + type: 'Button', + label: 'Next screen', + rect: { x: 10, y: 20, width: 100, height: 40 }, + hittable: true, + }, + ]); + const device = createInteractionDevice(nonHittableSnapshot(), { + captureSnapshot: async () => { + captureCount += 1; + return { snapshot: captureCount === 1 ? nonHittableSnapshot() : changedSnapshot }; + }, + tap: async () => ({ ok: true }), + }); + + const result = await device.interactions.press(selector('label=Continue'), { + session: 'default', + verify: true, + }); + + assert.equal(result.kind, 'selector'); + if (result.kind !== 'selector') return; + assert.equal(result.targetHittable, false); + assert.equal(result.evidence?.changedFromBefore, true); + // The "may have had no visible effect" warning is contradicted by the + // evidence sitting next to it — it must be dropped. + assert.equal('hint' in result, false); +}); + +test('runtime press keeps the non-hittable hint when evidence shows no change', async () => { + const nonHittableSnapshot = makeSnapshotState([ + { + index: 0, + depth: 0, + type: 'Button', + label: 'Continue', + rect: { x: 10, y: 20, width: 100, height: 40 }, + hittable: false, + }, + ]); + const device = createInteractionDevice(nonHittableSnapshot, { + tap: async () => ({ ok: true }), + }); + + const verified = await device.interactions.press(selector('label=Continue'), { + session: 'default', + verify: true, + }); + assert.equal(verified.evidence?.changedFromBefore, false); + assert.match( + ('hint' in verified ? verified.hint : undefined) ?? '', + /may have had no visible effect/, + ); + + const unverified = await device.interactions.press(selector('label=Continue'), { + session: 'default', + }); + assert.match( + ('hint' in unverified ? unverified.hint : undefined) ?? '', + /may have had no visible effect/, + ); +}); + test('runtime press without verify omits evidence entirely', async () => { const device = createInteractionDevice(selectorSnapshot(), { tap: async () => ({ ok: true }), diff --git a/src/commands/interaction/runtime/interactions.ts b/src/commands/interaction/runtime/interactions.ts index 0d922d1c92..958f0b4609 100644 --- a/src/commands/interaction/runtime/interactions.ts +++ b/src/commands/interaction/runtime/interactions.ts @@ -127,13 +127,13 @@ export const fillCommand: RuntimeCommand ? `fill target ${formatTargetForWarning(resolved)} resolved to "${nodeType}", attempting fill anyway.` : undefined; const evidence = verify ? await captureVerifyEvidence(runtime, options, resolved) : undefined; - return { + return reconcileNonHittableHintWithEvidence({ ...resolved, text: options.text, ...(warning ? { warning } : {}), ...(formattedBackendResult ? { backendResult: formattedBackendResult } : {}), ...(evidence ? { evidence } : {}), - }; + }); }; export const typeTextCommand: RuntimeCommand< @@ -198,11 +198,11 @@ async function tapCommand( }); const formattedBackendResult = toBackendResult(backendResult); const evidence = verify ? await captureVerifyEvidence(runtime, options, resolved) : undefined; - return { + return reconcileNonHittableHintWithEvidence({ ...resolved, ...(formattedBackendResult ? { backendResult: formattedBackendResult } : {}), ...(evidence ? { evidence } : {}), - }; + }); } /** @@ -233,6 +233,29 @@ async function captureVerifyEvidence( } } +// The resolution-time non-hittable hint warns the action "may have had no +// visible effect". When --verify evidence proves the interactive tree changed, +// that warning is contradicted by data sitting next to it in the same response +// — drop it and let targetHittable + evidence speak for themselves. +function reconcileNonHittableHintWithEvidence(result: T): T { + // Widened view: point-target results carry none of these fields, which is + // exactly the no-op path. + const view = result as { + targetHittable?: boolean; + hint?: string; + evidence?: InteractionEvidence; + }; + if ( + view.targetHittable !== false || + view.evidence?.changedFromBefore !== true || + view.hint === undefined + ) { + return result; + } + const { hint: _hint, ...rest } = view; + return rest as T; +} + function requireResolvedPoint(result: { point?: Point }): Point { if (!result.point) { throw new AppError('COMMAND_FAILED', 'Interaction target resolved without coordinates'); diff --git a/src/commands/interaction/runtime/resolution.ts b/src/commands/interaction/runtime/resolution.ts index 47e82aa97e..81725eed6a 100644 --- a/src/commands/interaction/runtime/resolution.ts +++ b/src/commands/interaction/runtime/resolution.ts @@ -13,7 +13,7 @@ import { import { buildSelectorChainForNode } from '../../../utils/selector-build.ts'; import { findNodeByLabel, resolveRefLabel } from '../../../snapshot/snapshot-processing.ts'; import { - isNodeVisibleInEffectiveViewport, + isNodeVisibleOnScreen, resolveEffectiveViewportRect, } from '../../../snapshot/mobile-snapshot-semantics.ts'; import { isSnapshotNodeInteractionBlocked } from '../../../snapshot/snapshot-occlusion.ts'; @@ -127,7 +127,7 @@ async function resolveRefInteractionTarget( kind: 'ref', point, target: { kind: 'ref', ref: `@${resolved.ref}` }, - ...describeResolvedNode(runtime, capture.snapshot.nodes, node, params.action), + ...describeResolvedInteractionNode(runtime, node, capture.snapshot.nodes, params.action), }; } @@ -182,6 +182,7 @@ async function resolveSelectorInteractionTarget( }) : resolved.node; assertInteractionNotBlocked(node, `Selector ${resolved.selector.raw}`, params.action); + assertVisibleSelectorTarget(node, capture.snapshot.nodes, resolved.selector.raw, params.action); const point = resolveNodeCenter( node, `Selector ${resolved.selector.raw} resolved to invalid bounds`, @@ -190,22 +191,24 @@ async function resolveSelectorInteractionTarget( kind: 'selector', point, target: { kind: 'selector', selector: resolved.selector.raw }, - ...describeResolvedNode(runtime, capture.snapshot.nodes, node, params.action), + ...describeResolvedInteractionNode(runtime, node, capture.snapshot.nodes, params.action), }; } -function describeResolvedNode( +// Shared tail of a resolved ref/selector interaction target: the node itself +// plus everything derived from it for the response. +function describeResolvedInteractionNode( runtime: AgentDeviceRuntime, - nodes: SnapshotState['nodes'], node: SnapshotNode, + nodes: SnapshotState['nodes'], action: InteractionAction, ): { node: SnapshotNode; selectorChain: string[]; - refLabel?: string; + refLabel: string | undefined; targetHittable?: boolean; hint?: string; - preActionNodes: SnapshotNode[]; + preActionNodes: SnapshotState['nodes']; } { return { node, @@ -410,19 +413,50 @@ function isUsableResolvedNode(node: SnapshotNode | null | undefined): node is Sn return resolveRectCenter(node.rect) !== null; } +// Selector parity for the @ref off-screen guard: without it, a selector +// resolving to a closed drawer/carousel item "succeeds" by tapping coordinates +// outside the viewport (observed as `Tapped (-161, 265)` against Bluesky's +// closed drawer) while the same node via @ref is refused. +function assertVisibleSelectorTarget( + node: SnapshotNode, + nodes: SnapshotState['nodes'], + selector: string, + action: InteractionAction, +): void { + throwIfOffscreenInteractionTarget(node, nodes, { + message: `Selector ${selector} resolved to an off-screen element and is not safe to ${action}`, + details: { reason: 'offscreen_selector', selector }, + hint: `The element is outside the visible viewport — likely inside a closed drawer, another tab, or scrolled content. Scroll toward it or open its container, take a fresh snapshot, then retry ${action}.`, + }); +} + function assertVisibleRefTarget( node: SnapshotNode, nodes: SnapshotState['nodes'], refInput: string, action: InteractionAction, +): void { + throwIfOffscreenInteractionTarget(node, nodes, { + message: `Ref ${refInput} is off-screen and not safe to ${action}`, + details: { reason: 'offscreen_ref', ref: normalizeRef(refInput) }, + hint: `Use scroll with the direction from the off-screen summary, take a fresh snapshot, then retry ${action} with the new ref or a selector.`, + }); +} + +// isNodeVisibleOnScreen (not the effective-viewport form): items inside an +// off-screen scrollable container (closed drawer) must also count as +// off-screen, not just items scrolled out of an on-screen container. +function throwIfOffscreenInteractionTarget( + node: SnapshotNode, + nodes: SnapshotState['nodes'], + failure: { message: string; details: Record; hint: string }, ): void { const viewport = node.rect ? resolveEffectiveViewportRect(node, nodes) : null; - if (!node.rect || !viewport || isNodeVisibleInEffectiveViewport(node, nodes)) return; - throw new AppError('COMMAND_FAILED', `Ref ${refInput} is off-screen and not safe to ${action}`, { - reason: 'offscreen_ref', - ref: normalizeRef(refInput), + if (!node.rect || !viewport || isNodeVisibleOnScreen(node, nodes)) return; + throw new AppError('COMMAND_FAILED', failure.message, { + ...failure.details, rect: node.rect, viewport, - hint: `Use scroll with the direction from the off-screen summary, take a fresh snapshot, then retry ${action} with the new ref or a selector.`, + hint: failure.hint, }); } diff --git a/src/daemon/__tests__/direct-ios-selector.test.ts b/src/daemon/__tests__/direct-ios-selector.test.ts new file mode 100644 index 0000000000..66378effae --- /dev/null +++ b/src/daemon/__tests__/direct-ios-selector.test.ts @@ -0,0 +1,37 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import { AppError } from '../../kernel/errors.ts'; +import { isDirectIosSelectorFallbackError } from '../direct-ios-selector.ts'; + +test('runner ELEMENT_OFFSCREEN always falls back to tree-based resolution', () => { + const error = new AppError('ELEMENT_OFFSCREEN', 'element resolved off-screen at (-161, 265)'); + assert.equal(isDirectIosSelectorFallbackError(error), true); + assert.equal(isDirectIosSelectorFallbackError(error, { allowElementNotFound: false }), true); +}); + +test('runner ELEMENT_NOT_FOUND falls back only when the caller allows it', () => { + const error = new AppError('ELEMENT_NOT_FOUND', 'element not found'); + assert.equal(isDirectIosSelectorFallbackError(error), false); + assert.equal(isDirectIosSelectorFallbackError(error, { allowElementNotFound: true }), true); +}); + +test('transport-level COMMAND_FAILED errors fall back, semantic ones do not', () => { + assert.equal( + isDirectIosSelectorFallbackError(new AppError('COMMAND_FAILED', 'fetch failed')), + true, + ); + assert.equal( + isDirectIosSelectorFallbackError( + new AppError('COMMAND_FAILED', 'Runner command deadline exceeded: timed out'), + ), + true, + ); + assert.equal( + isDirectIosSelectorFallbackError(new AppError('COMMAND_FAILED', 'element covered by overlay')), + false, + ); + assert.equal( + isDirectIosSelectorFallbackError(new AppError('AMBIGUOUS_MATCH', 'multiple')), + false, + ); +}); diff --git a/src/daemon/__tests__/selectors.test.ts b/src/daemon/__tests__/selectors.test.ts index 15a7b3fc1f..14d3b34620 100644 --- a/src/daemon/__tests__/selectors.test.ts +++ b/src/daemon/__tests__/selectors.test.ts @@ -118,6 +118,210 @@ test('resolveSelectorChain disambiguates to deeper/smaller matching node when en assert.equal(resolved.matches, 2); }); +test('resolveSelectorChain disambiguation prefers on-screen candidates over off-screen ones', () => { + // Bluesky-style closed drawer: the drawer's "Profile" sits fully off-screen + // left (deeper + smaller, so pre-viewport ranking picked it) while the bottom + // tab "Profile" is visible. The visible candidate must win. + const nodes: SnapshotState['nodes'] = [ + { + ref: 'e1', + index: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 400, height: 800 }, + depth: 0, + enabled: true, + hittable: true, + }, + { + ref: 'e2', + index: 1, + parentIndex: 0, + type: 'Button', + label: 'Profile', + rect: { x: 20, y: 740, width: 200, height: 50 }, + depth: 2, + enabled: true, + hittable: true, + }, + { + ref: 'e3', + index: 2, + parentIndex: 0, + type: 'Button', + label: 'Profile', + rect: { x: -320, y: 240, width: 100, height: 20 }, + depth: 3, + enabled: true, + hittable: false, + }, + ]; + const chain = parseSelectorChain('label="Profile"'); + const resolved = resolveSelectorChain(nodes, chain, { + platform: 'ios', + requireRect: true, + requireUnique: true, + disambiguateAmbiguous: true, + }); + assert.ok(resolved); + assert.equal(resolved.node.ref, 'e2'); + assert.equal(resolved.matches, 2); +}); + +test('resolveSelectorChain disambiguation treats items inside an off-screen scroll container as off-screen', () => { + // The closed drawer carries its own ScrollView at negative x. Visibility + // relative to that (off-screen) container is not enough — the drawer item + // must lose to the on-screen candidate. + const nodes: SnapshotState['nodes'] = [ + { + ref: 'e1', + index: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 400, height: 800 }, + depth: 0, + enabled: true, + hittable: true, + }, + { + ref: 'e2', + index: 1, + parentIndex: 0, + type: 'ScrollView', + rect: { x: -320, y: 0, width: 320, height: 800 }, + depth: 1, + enabled: true, + hittable: false, + }, + { + ref: 'e3', + index: 2, + parentIndex: 1, + type: 'Button', + label: 'Profile', + rect: { x: -310, y: 240, width: 100, height: 20 }, + depth: 3, + enabled: true, + hittable: false, + }, + { + ref: 'e4', + index: 3, + parentIndex: 0, + type: 'Button', + label: 'Profile', + rect: { x: 20, y: 740, width: 200, height: 50 }, + depth: 2, + enabled: true, + hittable: true, + }, + ]; + const chain = parseSelectorChain('label="Profile"'); + const resolved = resolveSelectorChain(nodes, chain, { + platform: 'ios', + requireRect: true, + requireUnique: true, + disambiguateAmbiguous: true, + }); + assert.ok(resolved); + assert.equal(resolved.node.ref, 'e4'); +}); + +test('resolveSelectorChain disambiguation treats an edge-grazing off-screen container as off-screen', () => { + // Bluesky regression: the closed drawer's overlay container pokes a fraction + // of a pixel into the viewport (float rounding), but its center — the tap + // point — is far off-screen. Edge overlap must not count as on-screen, so + // with no other candidates the deeper drawer button still wins (and the + // interaction guard then refuses it). + const nodes: SnapshotState['nodes'] = [ + { + ref: 'e1', + index: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 402, height: 874 }, + depth: 0, + enabled: true, + hittable: true, + }, + { + ref: 'e2', + index: 1, + parentIndex: 0, + type: 'Other', + label: 'Explore', + rect: { x: -321.6, y: 0, width: 321.67, height: 874 }, + depth: 1, + enabled: true, + hittable: false, + }, + { + ref: 'e3', + index: 2, + parentIndex: 1, + type: 'Button', + label: 'Explore', + rect: { x: -321.6, y: 240, width: 321.33, height: 50 }, + depth: 3, + enabled: true, + hittable: false, + }, + ]; + const chain = parseSelectorChain('label="Explore"'); + const resolved = resolveSelectorChain(nodes, chain, { + platform: 'ios', + requireRect: true, + requireUnique: true, + disambiguateAmbiguous: true, + }); + assert.ok(resolved); + // Neither candidate counts as on-screen, so the deepest-smallest tiebreak + // applies — NOT a preference for the edge-grazing container. + assert.equal(resolved.node.ref, 'e3'); +}); + +test('resolveSelectorChain disambiguation keeps deepest-smallest when all candidates are off-screen', () => { + const nodes: SnapshotState['nodes'] = [ + { + ref: 'e1', + index: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 400, height: 800 }, + depth: 0, + enabled: true, + hittable: true, + }, + { + ref: 'e2', + index: 1, + parentIndex: 0, + type: 'Other', + label: 'Drawer item', + rect: { x: -320, y: 200, width: 300, height: 300 }, + depth: 2, + enabled: true, + hittable: false, + }, + { + ref: 'e3', + index: 2, + parentIndex: 1, + type: 'Button', + label: 'Drawer item', + rect: { x: -310, y: 240, width: 100, height: 20 }, + depth: 3, + enabled: true, + hittable: false, + }, + ]; + const chain = parseSelectorChain('label="Drawer item"'); + const resolved = resolveSelectorChain(nodes, chain, { + platform: 'ios', + requireRect: true, + requireUnique: true, + disambiguateAmbiguous: true, + }); + assert.ok(resolved); + assert.equal(resolved.node.ref, 'e3'); +}); + test('resolveSelectorChain disambiguation tie falls back to next selector', () => { const tieNodes: SnapshotState['nodes'] = [ { diff --git a/src/daemon/client/daemon-client-timeout.ts b/src/daemon/client/daemon-client-timeout.ts index 365d138867..37f42c1709 100644 --- a/src/daemon/client/daemon-client-timeout.ts +++ b/src/daemon/client/daemon-client-timeout.ts @@ -52,11 +52,21 @@ export function handleRequestTimeout( }); } +// Read-only capture/polling commands that can block in platform accessibility +// bridges while the app is crashed or never idle. `wait` and `find` are repeated +// snapshot captures, so they share snapshot's failure mode. Keep the +// daemon/session alive on their timeouts so callers can still collect +// screenshot/perf/log evidence and close the session after the runner abort +// path has been triggered — resetting the daemon here turned one timed-out wait +// into a lost session for every session the daemon owned. +const DAEMON_PRESERVING_TIMEOUT_COMMANDS: ReadonlySet = new Set([ + PUBLIC_COMMANDS.snapshot, + PUBLIC_COMMANDS.wait, + PUBLIC_COMMANDS.find, +]); + export function shouldResetDaemonAfterRequestTimeout(command: string | undefined): boolean { - // Snapshot can block in platform accessibility bridges while the app is crashed or never idle. - // Keep the daemon/session alive so callers can still collect screenshot/perf/log evidence - // and close the session after the runner abort path has been triggered. - return command !== 'snapshot'; + return command === undefined || !DAEMON_PRESERVING_TIMEOUT_COMMANDS.has(command); } function resolveRequestTimeoutHint(params: { diff --git a/src/daemon/client/daemon-client.ts b/src/daemon/client/daemon-client.ts index 0559e5f9af..7db474ec95 100644 --- a/src/daemon/client/daemon-client.ts +++ b/src/daemon/client/daemon-client.ts @@ -5,6 +5,7 @@ import type { import type { RequestProgressSink } from '../request-progress.ts'; import { createRequestId, emitDiagnostic, withDiagnosticTimer } from '../../utils/diagnostics.ts'; import { INTERNAL_COMMANDS, PUBLIC_COMMANDS } from '../../command-catalog.ts'; +import { parseWaitPositionals } from '../../core/wait-positionals.ts'; import { prepareRemoteRequestArtifacts } from '../../remote/daemon-artifacts.ts'; import { cleanupDaemonAfterRequest, @@ -125,6 +126,15 @@ export function resolveDaemonRequestTimeoutMs( req: Omit, ): number | undefined { if (req.command === PUBLIC_COMMANDS.test) return undefined; + if (req.command === PUBLIC_COMMANDS.wait) { + // The wait budget travels as a positional, not a flag, so parse it the same + // way the daemon will. Without this, a `wait ... 180000` dies at the default + // request timeout with the runner/daemon torn down as collateral. + const waitBudgetMs = resolveWaitRequestBudgetMs(req.positionals); + if (waitBudgetMs !== null) { + return Math.max(DAEMON_REQUEST_TIMEOUT_MS, waitBudgetMs + WAIT_REQUEST_TIMEOUT_MARGIN_MS); + } + } if (typeof req.flags?.timeoutMs === 'number' && isExplicitTimeoutCommand(req.command)) { return req.flags.timeoutMs; } @@ -140,3 +150,14 @@ function isExplicitTimeoutCommand(command: string | undefined): boolean { command === PUBLIC_COMMANDS.snapshot ); } + +// Margin over the user-supplied wait budget so the daemon-side timeout result +// (with its stable/wait diagnostics) wins the race against the client envelope. +const WAIT_REQUEST_TIMEOUT_MARGIN_MS = 30_000; + +function resolveWaitRequestBudgetMs(positionals: string[] | undefined): number | null { + const parsed = parseWaitPositionals(positionals ?? []); + if (!parsed) return null; + if (parsed.kind === 'sleep') return parsed.durationMs; + return parsed.timeoutMs; +} diff --git a/src/daemon/direct-ios-selector.ts b/src/daemon/direct-ios-selector.ts index edcf0bb7d0..3175718359 100644 --- a/src/daemon/direct-ios-selector.ts +++ b/src/daemon/direct-ios-selector.ts @@ -35,6 +35,11 @@ export function isDirectIosSelectorFallbackError( ): boolean { const appError = asAppError(error); if (appError.code === 'ELEMENT_NOT_FOUND') return options.allowElementNotFound === true; + // The runner refuses to tap a hittable match whose frame is outside the app + // frame (closed drawer, off-viewport carousel). The tree-based path either + // prefers an on-screen candidate or raises the actionable offscreen_selector + // error, so always fall back. + if (appError.code === 'ELEMENT_OFFSCREEN') return true; if (appError.code !== 'COMMAND_FAILED') return false; const message = appError.message.toLowerCase(); return ( diff --git a/src/daemon/selectors-resolve.ts b/src/daemon/selectors-resolve.ts index 390c946e20..84adfd61d2 100644 --- a/src/daemon/selectors-resolve.ts +++ b/src/daemon/selectors-resolve.ts @@ -1,5 +1,7 @@ import type { Platform, PublicPlatform } from '../kernel/device.ts'; import type { SnapshotNode, SnapshotState } from '../kernel/snapshot.ts'; +import { isNodeVisibleOnScreen } from '../snapshot/mobile-snapshot-semantics.ts'; +import { buildSnapshotNodeMap } from '../snapshot/snapshot-tree.ts'; import { matchesSelector } from './selectors-match.ts'; import type { Selector, SelectorChain } from './selectors-parse.ts'; @@ -108,6 +110,12 @@ export function formatSelectorFailure( : `Selector did not match (${summary})`; } +type DisambiguationState = { + best: SnapshotNode | null; + bestVisible: boolean; + tie: boolean; +}; + function analyzeSelectorMatches( nodes: SnapshotState['nodes'], selector: Selector, @@ -116,32 +124,60 @@ function analyzeSelectorMatches( ): { count: number; firstNode: SnapshotNode | null; disambiguated: SnapshotNode | null } { let count = 0; let firstNode: SnapshotNode | null = null; - let best: SnapshotNode | null = null; - let tie = false; + const state: DisambiguationState = { best: null, bestVisible: false, tie: false }; + // Lazily built: only ambiguous matches pay for viewport inference. + let byIndex: Map | undefined; + const isVisible = (node: SnapshotNode): boolean => { + byIndex ??= buildSnapshotNodeMap(nodes); + return isNodeVisibleOnScreen(node, nodes, byIndex); + }; for (const node of nodes) { if (requireRect && !node.rect) continue; if (!matchesSelector(node, selector, platform)) continue; count += 1; firstNode ??= node; - if (!best) { - best = node; - continue; - } - const comparison = compareDisambiguationCandidates(node, best); - if (comparison > 0) { - best = node; - tie = false; - } else if (comparison === 0) { - tie = true; - } + accumulateDisambiguationCandidate(state, node, isVisible); } return { count, firstNode, - disambiguated: tie ? null : best, + disambiguated: state.tie ? null : state.best, }; } +// A closed drawer or off-viewport carousel keeps its items in the tree at +// out-of-bounds rects; picking one silently taps coordinates that cannot land. +// Prefer candidates visible on screen before the deepest-then-smallest +// tiebreak (visibility is evaluated only once matches are ambiguous, so +// unique resolutions never pay for viewport inference). +function accumulateDisambiguationCandidate( + state: DisambiguationState, + node: SnapshotNode, + isVisible: (node: SnapshotNode) => boolean, +): void { + if (!state.best) { + state.best = node; + return; + } + state.bestVisible ||= isVisible(state.best); + const nodeVisible = isVisible(node); + if (nodeVisible !== state.bestVisible) { + if (nodeVisible) { + state.best = node; + state.bestVisible = true; + state.tie = false; + } + return; + } + const comparison = compareDisambiguationCandidates(node, state.best); + if (comparison > 0) { + state.best = node; + state.tie = false; + } else if (comparison === 0) { + state.tie = true; + } +} + function countSelectorMatchesOnly( nodes: SnapshotState['nodes'], selector: Selector, diff --git a/src/snapshot/mobile-snapshot-semantics.ts b/src/snapshot/mobile-snapshot-semantics.ts index c7f007457f..f0a91261c4 100644 --- a/src/snapshot/mobile-snapshot-semantics.ts +++ b/src/snapshot/mobile-snapshot-semantics.ts @@ -1,6 +1,11 @@ import { isRectVisibleInViewport, resolveViewportRect } from '../utils/rect-visibility.ts'; import { inferVerticalScrollIndicatorDirections } from '../utils/scroll-indicator.ts'; -import type { HiddenContentHint, Rect, SnapshotNode } from '../kernel/snapshot.ts'; +import { + centerOfRect, + type HiddenContentHint, + type Rect, + type SnapshotNode, +} from '../kernel/snapshot.ts'; import { buildSnapshotNodeMap, displayNodeLabel } from './snapshot-tree.ts'; import { isScrollableNodeLike } from '../utils/scrollable.ts'; @@ -92,6 +97,39 @@ export function isNodeVisibleInEffectiveViewport( return isRectVisibleInViewport(node.rect, viewport); } +// Effective-viewport visibility measures a node against its nearest scrollable +// ancestor, so items inside an off-screen container (e.g. a closed drawer's own +// ScrollView at negative x) still read as "visible" within that container. +// On-screen visibility additionally requires the node's CENTER — the point an +// interaction would tap — to sit inside the root Application/Window viewport. +// Edge overlap is not enough: a mostly-off-screen drawer container can graze +// the viewport by a fraction of a pixel while its center (the tap point) is +// far off-screen. Interaction guards and selector disambiguation use this +// stricter form; scroll-direction summaries keep the effective form. +export function isNodeVisibleOnScreen( + node: Pick, + nodes: SnapshotNode[], + byIndex: Map = buildSnapshotNodeMap(nodes), +): boolean { + if (!node.rect) { + return true; + } + if (!isNodeVisibleInEffectiveViewport(node, nodes, byIndex)) { + return false; + } + const rootViewport = resolveViewportRect(nodes, node.rect); + if (!rootViewport) { + return true; + } + const center = centerOfRect(node.rect); + return ( + center.x >= rootViewport.x && + center.x <= rootViewport.x + rootViewport.width && + center.y >= rootViewport.y && + center.y <= rootViewport.y + rootViewport.height + ); +} + export function resolveEffectiveViewportRect( node: Pick, nodes: SnapshotNode[], diff --git a/src/utils/__tests__/daemon-client.test.ts b/src/utils/__tests__/daemon-client.test.ts index 1f3f64d77e..0e94dd776e 100644 --- a/src/utils/__tests__/daemon-client.test.ts +++ b/src/utils/__tests__/daemon-client.test.ts @@ -207,6 +207,61 @@ test('snapshot request timeout preserves daemon metadata for follow-up evidence assert.equal(shouldResetDaemonAfterRequestTimeout(undefined), true); }); +test('read-only polling command timeouts preserve the daemon like snapshot', () => { + // wait/find are repeated snapshot captures: a stalled accessibility bridge + // must not turn one timed-out poll into a daemon reset that loses every session. + assert.equal(shouldResetDaemonAfterRequestTimeout('wait'), false); + assert.equal(shouldResetDaemonAfterRequestTimeout('find'), false); + assert.equal(shouldResetDaemonAfterRequestTimeout('press'), true); +}); + +test('wait request timeout extends past the user-supplied wait budget', () => { + const base = { + session: 'default', + positionals: [] as string[], + flags: {}, + meta: {}, + }; + + // Explicit budgets beyond the default envelope extend it (budget + margin). + assert.equal( + resolveDaemonRequestTimeoutMs({ + ...base, + command: 'wait', + positionals: ['text', 'Ready', '180000'], + }), + 210_000, + ); + assert.equal( + resolveDaemonRequestTimeoutMs({ + ...base, + command: 'wait', + positionals: ['stable', '500', '120000'], + }), + 150_000, + ); + // Sleep waits block for their full duration and get the same treatment. + assert.equal( + resolveDaemonRequestTimeoutMs({ ...base, command: 'wait', positionals: ['120000'] }), + 150_000, + ); + // Small budgets never shrink the envelope below the default. + assert.equal( + resolveDaemonRequestTimeoutMs({ + ...base, + command: 'wait', + positionals: ['text', 'Ready', '5000'], + }), + 90_000, + ); + // No explicit budget → default envelope. + assert.equal( + resolveDaemonRequestTimeoutMs({ ...base, command: 'wait', positionals: ['text', 'Ready'] }), + 90_000, + ); + assert.equal(resolveDaemonRequestTimeoutMs({ ...base, command: 'wait' }), 90_000); +}); + test('snapshot uses the standard daemon request timeout with an explicit override', () => { const base = { session: 'default',