fix(ios): double-check off-screen click refusals against a direct element read - #1566
Conversation
…ment read #1542: after an AX-free scroll on iOS, the off-screen interaction guard can refuse a click even though the target is genuinely on-screen, because it trusts a scroll-container ancestor's rect from the bulk accessibility tree, which a keyboard-dismiss content-offset correction can leave stale/corrupted while the target's own rect is already correct. When the guard is about to refuse on iOS, it now takes a single fresh, tree-independent XCUITest read of the target element (querySelector) and trusts that read's live `hittable` + rect-vs-root-viewport signal instead, if it positively confirms on-screen. Any failure to unambiguously re-resolve the element (no id/label, not found, ambiguous, transport error) fails closed exactly as before. Genuinely off-screen targets, and every other platform, are unchanged: the backend method is gated to local (non-provider) iOS sessions only, and only ever runs on the about-to-fail path. The decision itself is a pure function (decideOffscreenRefusalDoubleCheck in mobile-snapshot-semantics.ts) with counterfactual-proven tests: hardcoding it to always trust the bulk verdict turns the rescue test red, and hardcoding it to always trust the direct read (including on "unavailable") turns the fail-closed/genuine-refusal test red. Live-validated on a fresh-boot iOS simulator: checkout-form.ad 2/2 passes (previously failing at step 11), gesture-lab.ad 2/2 (regression), and the Android checkout-form/gesture-lab suite passes unchanged, proving no cross-platform behavior change.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
…se the double-check to one backend hook Review blockers 1+2 (interleaved by design — the soundness fix is expressed through the collapsed hook's contract): 1. SOUNDNESS: a rescued refusal now returns the node PATCHED WITH THE LIVE RECT the backend confirmed, and every downstream use (tap point, response) reads from that returned node — never the original. In the frozen-tree manifestation (the whole bulk tree pinned at pre-gesture values), the original rect can be stale even when the rescue verdict is correct; tapping it would have silently landed at the wrong coordinate. New regression: offscreen-double-check.test.ts's frozen-tree case, with a counterfactual (revert to computing the point from the pre-guard node) proven red then reverted. 2. SURFACE: collapsed to ONE optional backend hook, `confirmOffscreenTargetVisible?(context, node, rootViewport): Promise<Rect | null>` — conceptually a boolean, but returns the live rect so item 1's fix has something to act on. Deleted decideOffscreenRefusalDoubleCheck, the OffscreenRefusalDoubleCheckSignal/Reading ADT, and resolution.ts's dual-signal reconciliation shell: the bulk side was hardcoded 'off-screen' at the only call site, so the two-signal model was dead weight. The shared guard is now: bulk-off-screen -> ask the hook -> a live rect proceeds (patched), anything else (including no hook) throws exactly as before. The pure geometry boundary that decision reduces to (`isConfirmedOnScreenProbe` in mobile-snapshot-semantics.ts, replacing the deleted ADT) is unit-tested with two counterfactuals: ignoring `hittable` and ignoring the viewport containment check each turn a test red (proved, then reverted). `throwIfOffscreenInteractionTarget` is now exported (ADR 0011 registry honesty, see the contracts commit) and directly unit-tested in resolution.test.ts, mirroring the existing tryResolveRefNode pattern.
…queryDirectIosSelector
Review blocker 3 (BOUNDARIES):
- direct-ios-selector.ts no longer does any runner I/O — it's back to pure
gate/parse (readSimpleIosSelectorTarget, deriveDirectIosNodeSelector,
isDirectIosSelectorFallbackError) plus the ONE shared eligibility
predicate, isLocalIosRunnerSession(session, { skipPendingPostGestureStabilization
}). Both the direct-selector tap fast path and the new offscreen
double-check probe call this same function; the one behavioral difference
between them (the tap fast path skips a session with a pending
postGestureStabilization, the double-check does not) is now an explicit
parameter instead of two separately-written gates.
- The probe I/O moved to a new sibling, src/daemon/offscreen-target-probe.ts,
which reuses selector-runtime.ts's `queryDirectIosSelector` (now exported
and decoupled from SelectorRuntimeParams — it takes a session + a bare
{key, value} selector + AppleRunnerRequestOptions) rather than opening a
second querySelector client. Node extraction (`readDirectIosSelectorNode`,
the one `as SnapshotNode` cast) stays singular, inside selector-runtime.ts.
- interaction-runtime.ts wires confirmOffscreenTargetVisible only when
isLocalIosRunnerSession(session, { skipPendingPostGestureStabilization:
false }) — deliberately NOT skipping a pending post-gesture stabilization,
since that is exactly the window the double-check exists to cover.
…arantee matrix Review blocker 4 (GUARANTEE HONESTY): the shared offscreen cell (RUNTIME_TREE_SHARED_GUARANTEES.offscreen, used by runtime-selector and runtime-ref) and the native-ref path's offscreen cell still named isNodeVisibleOnScreen as sole enforcement after #1542's double-check landed — that understates what actually enforces the guarantee now. Both cells' `via` now point at throwIfOffscreenInteractionTarget (exported from resolution.ts in the prior commit for exactly this), the real end-to-end enforcement point: isNodeVisibleOnScreen is the bulk-tree decision it starts from, and on iOS a would-be refusal can still be confirmed via the optional AgentDeviceBackend.confirmOffscreenTargetVisible hook before erroring. The cell's comment states the rescue-only, fail-closed shape explicitly per ADR 0011's matrix rules — this does not weaken the cell, it extends its description to match reality. iOS rescue policy stays OUT of resolution.ts's shared docstrings (the "spine"): this registry file is where per-path enforcement detail belongs, and the optional-method wiring in interaction-runtime.ts remains the only cross-platform touch. The registry's own gate test (interaction-guarantees.test.ts) still passes: every `via` resolves to a real exported symbol.
….test.ts Review blocker 5 (TEST HOMES): AGENTS.md forbids adding to daemon/handlers/__tests__/interaction.test.ts (it predates the test-mirrors-source-topology rule and shrinks opportunistically). Reverts the 172 lines added there in the original PR version; interaction.test.ts is back to its pre-#1542 baseline (81 tests, unchanged). The same assertions now live in their proper homes (see the prior three commits for the sources they cover): - pure decision pin: src/utils/__tests__/mobile-snapshot-semantics.test.ts (isConfirmedOnScreenProbe, with the two counterfactuals) - direct-guard pin: src/commands/interaction/runtime/resolution.test.ts (throwIfOffscreenInteractionTarget, mirroring tryResolveRefNode) - probe unit tests: src/daemon/__tests__/selector-runtime.test.ts (queryDirectIosSelector) and src/daemon/__tests__/direct-ios-selector.test.ts (isLocalIosRunnerSession, deriveDirectIosNodeSelector) - probe integration: src/daemon/__tests__/offscreen-target-probe.test.ts (confirmIosOffscreenTargetVisible, mocked runner) - end-to-end rescue/refuse, including the frozen-tree live-geometry regression + its counterfactual: new sibling src/commands/interaction/runtime/offscreen-double-check.test.ts (next to resolution.ts, using the same createInteractionDevice harness resolution.test.ts already uses)
Reshape complete — all five blockers addressedRecreated the worktree from Note on process: I couldn't locate the underlying review comment via 1. SOUNDNESS — live geometry after rescueConfirmed real: my original code returned early from the guard on a rescue but callers kept using the pre-guard Chose (a), not (b): New regression + counterfactual ( (Mutation applied: reverted 2. SURFACE — one hookCollapsed to const liveRect = await runtime.backend.confirmOffscreenTargetVisible?.(ctx, node, rootViewport);
if (liveRect) return { ...node, rect: liveRect };
throw new AppError(...); // fail-closed, unchangedThe pure geometry boundary the hook's decision reduces to ( Both applied, watched red, reverted. 3. BOUNDARIES
The probe I/O moved to a new sibling, 4. GUARANTEE HONESTYBoth 5. TEST HOMES
LOC table
Exit bar (fresh-boot iPhone 17 Pro / iOS 26.2, re-run in full against the reshaped code)
Gates: Generated by Claude Code |
|
Re-reviewed updated exact head |
thymikee
added
the
ready-for-human
|
1 participant
Summary
#1542 (the remaining blocker on PR #1559, which fixed the "scroll-inert" defect) is fixed here: the iOS
checkout-form.adcorpus leg now passes 2/2 fresh-boot.Root cause
click id="shipping-pickup"was refused withresolved to an off-screen elementeven though the button was visibly on-screen. Live evidence (/private/tmp/ad-defect2-artifacts/manual-probe/):rect: {x:18, y:62, w:366, h:729}(correct).rect: {x:18, y:381, w:366, h:109}— squeezed down to a sliver, a stale artifact of the keyboard-dismiss content-offset correction PR fix(ios): keyboard-dismiss content settle race (#1542) — partial, defect 2 needs a decision #1559 fixed the cause of but not this downstream symptom.y:136.67) is correct in both — but the off-screen guard (resolveEffectiveViewportRect/findNearestScrollableAncestorRectinsrc/snapshot/mobile-snapshot-semantics.ts) measures the button against its (corrupted) ancestor's clip rect and refuses.Fix
When — and only when — the off-screen guard is about to refuse a click/tap/gesture-target resolution on iOS, it now takes one extra, tree-independent read of the target element straight from the local XCTest runner (
querySelector, the same primitive the existing direct-iOS-selector fast path uses) and trusts that read's livehittable+ rect-vs-root-viewport signal if it positively confirms on-screen.AgentDeviceBackend.verifyOffscreenClickTargetmethod, attached only for local (non-provider) iOS sessions insrc/daemon/handlers/interaction-runtime.ts; every other platform/backend omits it, soruntime.backend.verifyOffscreenClickTargetisundefinedthere and the guard's decision is byte-for-byte unchanged. It runs only insidethrowIfOffscreenInteractionTarget's about-to-throw branch insrc/commands/interaction/runtime/resolution.ts— never on the accept path.isHittableon a fresh, single-element query is already computed against the element's current clip/window state, so it captures ancestor-clipping correctness without a second, fragile query. This is a live-validation-driven refinement over the original ancestor-rect-swap design.readSimpleIosSelectorTarget) skips itself whilesession.postGestureStabilizationis pending — exactly the window where the bulk tree is stale. The double-check deliberately does not inherit that gate; it needs to work precisely in that window.The pure decision function + counterfactual proof
decideOffscreenRefusalDoubleCheck(src/snapshot/mobile-snapshot-semantics.ts) is the whole decision: bulk-says-offscreen + direct-says-onscreen → proceed; both agree → refuse; direct-read-unavailable → refuse (fail-closed). It is pure and covered bysrc/utils/__tests__/mobile-snapshot-semantics.test.ts, proved with two counterfactual mutations (revert-and-watch-fail per docs/agents/testing.md):Counterfactual 1 — "always trust bulk" (hardcode
return 'refuse'when bulk is off-screen, ignoringdirect):Counterfactual 2 — "always trust direct" (
return direct.status === 'off-screen' ? 'refuse' : 'proceed', treating "unavailable" as license to proceed instead of falling back to the bulk guard's refusal):Both mutations were applied, watched red, then reverted; the real implementation is back to 19/19 green in that file.
Live validation (fresh-boot iPhone 17 Pro / iOS 26.2)
checkout-form.adfresh-boot run 1/2checkout-form.adfresh-boot run 2/2gesture-lab.adfresh-boot run 1/2 (regression)gesture-lab.adfresh-boot run 2/2 (regression)checkout-form-android.ad+gesture-lab-android.ad(Pixel_7_CI, app freezer disabled)click id="shipping-pickup"replayed against the Home screen, no navigation) — true-refusal proofREPLAY_DIVERGENCEfires:Replay failed at step 3 (click "id=\"shipping-pickup\""): Selector did not match: id="shipping-pickup"The debug trace of a passing run shows the rescue firing live: a
querySelectorforshipping-pickupimmediately precedes thetapat the step that used to fail — confirming the direct read, not a lucky bulk-tree state, is what let the click through.Artifacts (videos, ndjson request logs, divergence reports) preserved outside the repo at
/private/tmp/ad-refusal-artifacts/on the machine this was developed on.Gates
pnpm typecheck && pnpm lint && pnpm format:check && pnpm check:layering && pnpm check:replay-compat— all clean.npx vitest run src/daemon src/snapshot src/commands/interaction/runtime— 218 files / 1900 tests pass. Fullnpx vitest run— 637/638 files clean; the one flaky test (runner-client.test.ts's xctestrun-abort test, unrelated to this change) passed cleanly on an isolated rerun — contention flake, not an assertion failure.Fixes #1542
Generated by Claude Code