From a99a303431b7112c3825cf795dfd01ed9028a600 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 4 Jul 2026 12:41:19 +0200 Subject: [PATCH 1/3] docs+feat: ADR 0011 interaction guarantee contract, Layer-1 registry and gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design for making interaction guarantees hold across every dispatch path (runtime selector/ref, direct iOS selector, native ref, coordinate, maestro fallback) instead of eroding at path boundaries one incident at a time — every interaction bug this week was a (path, guarantee) cell nobody was watching. Three layers (ADR 0011): declare the path x guarantee matrix as a typed registry whose completeness is a compile error; share one implementation per rule on both sides of the wire with golden fixture tables proving TS/Swift parity; prove every non-waived cell with contract scenarios generated from the registry. This lands Layer 1: the registry with an HONEST initial classification — ten cells are acknowledged gap waivers (direct-path disambiguation/ occlusion/nonHittable/responseFields/errorTaxonomy, native-ref guards, coordinate bounds) — plus the gate test that keeps entries truthful: referenced TS symbols must be exported, runner symbols must exist in the Swift sources, delegations must land on paths that actually enforce the guarantee, and the gap list is pinned so it can only change explicitly in a reviewed diff. --- .../0011-interaction-guarantee-contract.md | 216 +++++++++++++ .../__tests__/interaction-guarantees.test.ts | 144 +++++++++ src/contracts/interaction-guarantees.ts | 305 ++++++++++++++++++ 3 files changed, 665 insertions(+) create mode 100644 docs/adr/0011-interaction-guarantee-contract.md create mode 100644 src/contracts/__tests__/interaction-guarantees.test.ts create mode 100644 src/contracts/interaction-guarantees.ts diff --git a/docs/adr/0011-interaction-guarantee-contract.md b/docs/adr/0011-interaction-guarantee-contract.md new file mode 100644 index 0000000000..c799700930 --- /dev/null +++ b/docs/adr/0011-interaction-guarantee-contract.md @@ -0,0 +1,216 @@ +# ADR 0011: Interaction Guarantee Contract (path × guarantee matrix) + +## Status + +Proposed + +## Context + +Interaction commands (`press`/`click`/`fill`/`longpress`, and by extension the +read/wait surfaces built on the same resolution machinery) reach the device +through several dispatch paths, each of which chose its latency-vs-semantics +trade-offs independently: + +| Path | What it is | Why it exists | +| --- | --- | --- | +| `runtime-selector` | daemon tree capture → `resolveSelectorChain` → guards → coordinate tap | full semantics | +| `runtime-ref` | session snapshot → ref lookup → guards → coordinate tap | full semantics | +| `direct-ios-selector` | selector sent to the XCTest runner, which queries and taps natively | saves a full snapshot round trip | +| `native-ref` | `backend.tapTarget`/`fillTarget` for `click @ref` / `fill @ref` | saves resolution round trips | +| `coordinate` | raw x/y tap | escape hatch; semantics intentionally minimal | +| replay/maestro variants | `allowNonHittableCoordinateFallback`, replay-heal | third-party compat | + +Over one week of fixes, every interaction bug we shipped or caught was the same +shape — a guarantee enforced on one path and silently absent on a sibling: + +- off-screen refusal existed for `@ref` targets but not selector targets, and + not in the runner's native tap (Bluesky closed drawer: `Tapped (-161, 265)` + reported as success; fixed in #1075); +- `--verify` evidence attached on `press @ref` but was dropped by + `fill @ref`'s hand-built response branch (#1064 review); +- adb failure classification enriched thrown errors but not + `allowFailure`-result errors, and wrapped `exec` but not semantic provider + methods (#1067 review); +- the user's wait budget bounded prepare/replay/snapshot request envelopes but + not `wait` itself, and `wait` sat on the daemon-reset timeout path while + `snapshot` — same failure mode — preserved the daemon (#1075). + +The guarantees themselves are small and well-implemented. What is missing is +any structure that *knows the full set of paths and the full set of +guarantees* and can therefore notice an unfilled cell. Reviews catch cells +only when a reviewer happens to hold both axes in their head; dogfooding +catches them one production incident at a time. + +## Decision + +Make the path × guarantee matrix a first-class, machine-checked artifact, with +three enforcement layers. Types enforce **completeness** of declarations, +shared implementations prevent **drift**, and generated test coverage enforces +**truth**. + +### Layer 1 — Declare: the matrix as a typed registry with a gate + +`src/contracts/interaction-guarantees.ts` declares both axes and requires +every cell to be classified: + +```ts +export const INTERACTION_GUARANTEES = [ + 'disambiguation', // visible > deepest > smallest; ties fail + 'occlusion', // covered targets are refused + 'offscreen', // tap point (rect center) must lie in the root viewport + 'nonHittable', // promotion + targetHittable/hint annotation + 'responseFields', // refLabel/selectorChain/evidence assembled by the shared builder + 'verifyEvidence', // --verify baseline + post-action digest + 'errorTaxonomy', // no-match/ambiguous/offscreen codes, messages, hints +] as const; + +export type GuaranteeEnforcement = + | { kind: 'runtime'; via: string } // shared TS implementation (symbol name) + | { kind: 'runner'; via: string; parityTable: string } // Swift twin + golden fixture table proving parity + | { kind: 'delegated'; to: InteractionPathId } // path defers (e.g. direct → runtime on ELEMENT_OFFSCREEN) + | { kind: 'waived'; reason: string }; // explicit, reviewed waiver + +export const INTERACTION_DISPATCH_PATHS: Record< + InteractionPathId, + { + description: string; + commands: readonly string[]; + guarantees: Record; + } +> = { /* every path, every cell */ }; +``` + +Because `guarantees` is a `Record` over the guarantee union, **tsc fails** the +moment someone adds a guarantee without classifying it for every path, or adds +a path without classifying every guarantee. A unit gate +(`interaction-guarantees.test.ts`) additionally checks that: + +- every `via` string resolves to a real exported symbol (declarations cannot + rot into fiction); +- every `parityTable` names an existing fixture file; +- every `waived` reason is non-empty (waivers are visible, reviewed debt — the + initial classification will honestly contain several, and each one links an + issue). + +This is the same "make the gap declare itself" pattern already proven in this +repo by `scripts/integration-progress-model.ts` (which caught the unclassified +`--verify` flag on #1064) and the cross-command apple-leak guard from the +platform consolidation (ADR 0009). + +### Layer 2 — Share: one implementation per rule, on both sides of the wire + +Each guarantee has exactly one home that all TS paths import (most already +exist after #1075: `isNodeVisibleOnScreen`, `accumulateDisambiguationCandidate`, +`isSnapshotNodeInteractionBlocked`, `describeResolvedNode`, +`interactionResultExtra`, `reconcileNonHittableHintWithEvidence`). The +registry's `via` fields point at them; fallow's duplication gate keeps +re-implementations from creeping back. + +Rules that must run **runner-side** (the direct iOS path cannot see the daemon +tree) get pure-function Swift twins operating on plain geometry — no +`XCUIElement` — e.g. `TapPointPolicy.isAllowed(elementFrame:windowFrame:)`. +Parity is enforced by **golden fixture tables**: JSON files under +`contracts/fixtures/` consumed by three test suites — + +1. vitest asserts the TS rule over the table; +2. the runner's Swift unit tests (already compiled in CI by "Swift Runner Unit + Compile") assert the Swift twin over the same table; +3. the provider harness's fake runner derives its behavior from the same + table, so integration scenarios exercise the real contract rather than a + hand-written approximation. + +Drift between TS and Swift then turns CI red on whichever side changed, +without needing a simulator. + +For `responseFields`, one `buildInteractionResponseData(...)` becomes the only +construction site for interaction response payloads (this deletes the class of +bug where `fill @ref` rebuilt its response by hand and dropped `evidence`). A +small guard test — repo-precedented — fails if an interaction handler contains +a hand-rolled `responseData = {` literal. + +### Layer 3 — Prove: a contract suite generated from the registry + +`test/integration/interaction-contract/` holds table-driven scenarios: fixture +tree × command × forced path. The fixture trees are the real shapes that found +this week's bugs, kept permanently: + +- closed drawer (all candidates off-screen) → `offscreen_selector`/`offscreen_ref`; +- drawer item + visible twin (ambiguous on/off-screen) → visible candidate wins; +- edge-grazing container (0.07 px viewport overlap, center off-screen) → still refused; +- covered node → occlusion refusal; +- non-hittable target ± `--verify` → hint present / suppressed by evidence; +- stripped-root tree (no Application/Window) → safe-default visibility. + +Path forcing is explicit (`AGENT_DEVICE_FORCE_INTERACTION_PATH=runtime|direct|native-ref`, +test-only) so cases stay stable when path-selection heuristics change. + +The gate closes the loop: it walks the Layer-1 registry and fails when any +non-waived cell has no contract case tagged for it. Coverage of the matrix is +therefore by construction, not by reviewer memory. + +### Timeout policy joins the descriptor registry + +The `wait` timeout bug existed because request-envelope budgets and +on-timeout daemon policy lived in two hand-maintained lists in the client +(`isExplicitTimeoutCommand`, `shouldResetDaemonAfterRequestTimeout`). Both are +replaced by declarations on the command descriptors (ADR 0008 registry): + +```ts +timeoutPolicy: { + budget: { source: 'flag' | 'positional-parser' | 'none'; parser?: string }; + onTimeout: 'preserve-daemon' | 'reset-daemon'; +} +``` + +with a completeness gate over all public commands. Read-only polling commands +declare `preserve-daemon`; the client derives the envelope from the declared +budget source instead of special-casing command names. + +## Consequences + +- Adding a dispatch path or a guarantee becomes a *forced* whole-matrix + decision: tsc will not compile an unclassified cell, and the gate will not + pass an untested one. Silent erosion is structurally impossible; explicit + waivers remain possible but visible and linkable to issues. +- Fast paths keep their latency wins. Their divergence is priced and + documented instead of discovered on-device. +- The Swift/TS split stops being a parity blind spot: golden tables are the + single source of truth for cross-language rules, and the fake runner stops + being a second, drifting implementation. +- Initial cost is mostly classification honesty: the first registry will + contain `waived('gap: ...')` cells (e.g. the maestro non-hittable fallback + intentionally waives `offscreen`), which is the point — the debt becomes a + diffable list instead of folklore. +- The registry doubles as documentation input for help/skill output ("what + press guarantees"), which matters for small-model agents that only read the + contract, never the code. + +## Migration plan + +Each step lands green and independently useful: + +1. **Registry + gate** with an honest initial classification (waivers linked + to issues). No behavior change. +2. **Response builder consolidation** (`buildInteractionResponseData`) + the + hand-rolled-literal guard test. +3. **Golden fixture tables** + `TapPointPolicy` Swift extraction + the three + consumers (vitest, XCTest, fake runner). +4. **Contract scenario suite** + registry-driven coverage gate; port the + Bluesky fixtures from #1075's tests into the permanent tables. +5. **Descriptor timeout policy**; delete the two client-side command lists. + +## Alternatives considered + +- **Delete the fast paths** (single resolution spine): unacceptable — the + direct iOS path saves a full snapshot round trip per interaction and the + ref fast path is the backbone of replay throughput. The problem is not that + fast paths exist; it is that their trade-offs were implicit. +- **Typestate/branded types** ("cannot construct a response without proof the + guards ran"): cannot reach across the wire into Swift, and the proof tokens + go viral through every signature for marginal gain over the + registry-plus-gate split. Types are used where they are strongest — + completeness of the declaration — and tests where they are strongest — + truth of the declaration. +- **More integration tests without the registry**: this is the status quo + plus effort. Without the matrix as code, nothing forces a new path to + acquire the existing suite, which is exactly how this week's bugs happened. diff --git a/src/contracts/__tests__/interaction-guarantees.test.ts b/src/contracts/__tests__/interaction-guarantees.test.ts new file mode 100644 index 0000000000..55ef47e6c0 --- /dev/null +++ b/src/contracts/__tests__/interaction-guarantees.test.ts @@ -0,0 +1,144 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { + INTERACTION_DISPATCH_PATHS, + INTERACTION_GUARANTEES, + INTERACTION_PATH_IDS, +} from '../interaction-guarantees.ts'; + +// ADR 0011 Layer-1 gate: the matrix must stay complete (typed) AND honest +// (referenced implementations exist, waivers carry reasons). A cell that +// points at a deleted symbol or an empty excuse fails here, not on-device. + +const PROJECT_ROOT = path.resolve(import.meta.dirname, '..', '..', '..'); +const RUNNER_SOURCES_DIR = path.join( + PROJECT_ROOT, + 'apple-runner', + 'AgentDeviceRunner', + 'AgentDeviceRunnerUITests', +); + +test('every dispatch path classifies every guarantee', () => { + for (const pathId of INTERACTION_PATH_IDS) { + const contract = INTERACTION_DISPATCH_PATHS[pathId]; + assert.ok(contract, `missing contract for path ${pathId}`); + for (const guarantee of INTERACTION_GUARANTEES) { + assert.ok( + contract.guarantees[guarantee], + `path ${pathId} does not classify guarantee ${guarantee}`, + ); + } + } +}); + +test('runtime enforcement entries reference real exported symbols', async () => { + for (const [pathId, contract] of Object.entries(INTERACTION_DISPATCH_PATHS)) { + for (const [guarantee, enforcement] of Object.entries(contract.guarantees)) { + if (enforcement.kind !== 'runtime') continue; + const [modulePath, symbol] = enforcement.via.split('#'); + assert.ok( + modulePath && symbol, + `${pathId}/${guarantee}: runtime via must be "#", got "${enforcement.via}"`, + ); + const absolute = path.join(PROJECT_ROOT, modulePath); + assert.ok(fs.existsSync(absolute), `${pathId}/${guarantee}: module not found: ${modulePath}`); + const mod = (await import(absolute)) as Record; + assert.ok( + symbol in mod, + `${pathId}/${guarantee}: "${symbol}" is not exported from ${modulePath}`, + ); + } + } +}); + +test('runner enforcement entries reference symbols present in runner sources', () => { + for (const [pathId, contract] of Object.entries(INTERACTION_DISPATCH_PATHS)) { + for (const [guarantee, enforcement] of Object.entries(contract.guarantees)) { + if (enforcement.kind !== 'runner') continue; + const [fileName, symbol] = enforcement.via.split('#'); + assert.ok( + fileName && symbol, + `${pathId}/${guarantee}: runner via must be "#", got "${enforcement.via}"`, + ); + const absolute = path.join(RUNNER_SOURCES_DIR, fileName); + assert.ok( + fs.existsSync(absolute), + `${pathId}/${guarantee}: runner source not found: ${fileName}`, + ); + const source = fs.readFileSync(absolute, 'utf8'); + assert.ok( + source.includes(symbol), + `${pathId}/${guarantee}: "${symbol}" not found in ${fileName}`, + ); + if (enforcement.parityTable !== undefined) { + assert.ok( + fs.existsSync(path.join(PROJECT_ROOT, enforcement.parityTable)), + `${pathId}/${guarantee}: parity table not found: ${enforcement.parityTable}`, + ); + } + } + } +}); + +test('delegations point at real paths and waivers carry reasons', () => { + for (const [pathId, contract] of Object.entries(INTERACTION_DISPATCH_PATHS)) { + for (const [guarantee, enforcement] of Object.entries(contract.guarantees)) { + if (enforcement.kind === 'delegated') { + assert.ok( + (INTERACTION_PATH_IDS as readonly string[]).includes(enforcement.to), + `${pathId}/${guarantee}: delegated to unknown path ${enforcement.to}`, + ); + assert.notEqual( + enforcement.to, + pathId, + `${pathId}/${guarantee}: a path cannot delegate to itself`, + ); + assert.ok( + enforcement.via.trim().length > 0, + `${pathId}/${guarantee}: delegation must say how it triggers`, + ); + const target = + INTERACTION_DISPATCH_PATHS[enforcement.to].guarantees[ + guarantee as (typeof INTERACTION_GUARANTEES)[number] + ]; + assert.ok( + target.kind === 'runtime' || target.kind === 'runner', + `${pathId}/${guarantee}: delegates to ${enforcement.to}, which does not enforce it (${target.kind})`, + ); + } + if (enforcement.kind === 'waived' || enforcement.kind === 'inapplicable') { + assert.ok( + enforcement.reason.trim().length > 10, + `${pathId}/${guarantee}: ${enforcement.kind} requires a substantive reason`, + ); + } + } + } +}); + +test('acknowledged gaps are visible and bounded', () => { + const gaps: string[] = []; + for (const [pathId, contract] of Object.entries(INTERACTION_DISPATCH_PATHS)) { + for (const [guarantee, enforcement] of Object.entries(contract.guarantees)) { + if (enforcement.kind === 'waived' && enforcement.reason.startsWith('gap:')) { + gaps.push(`${pathId}/${guarantee}`); + } + } + } + // CONSERVATIVE: this list may only shrink, or grow in the same PR that + // updates it here with a linked issue. It is the diffable debt list. + assert.deepEqual(gaps.sort(), [ + 'coordinate/offscreen', + 'direct-ios-selector/disambiguation', + 'direct-ios-selector/errorTaxonomy', + 'direct-ios-selector/nonHittable', + 'direct-ios-selector/occlusion', + 'direct-ios-selector/responseFields', + 'maestro-non-hittable-fallback/errorTaxonomy', + 'native-ref/nonHittable', + 'native-ref/occlusion', + 'native-ref/offscreen', + ]); +}); diff --git a/src/contracts/interaction-guarantees.ts b/src/contracts/interaction-guarantees.ts new file mode 100644 index 0000000000..2a12025b40 --- /dev/null +++ b/src/contracts/interaction-guarantees.ts @@ -0,0 +1,305 @@ +/** + * The interaction guarantee matrix (ADR 0011). + * + * Every dispatch path an interaction command can take must classify EVERY + * guarantee: enforced by shared runtime code, enforced runner-side (with a + * parity table once ADR 0011 phase 3 lands), delegated to another path, + * inapplicable by construction, or explicitly waived with a reason. + * + * The `Record` over the guarantee union makes completeness a compile error: + * adding a guarantee refuses to build until every path classifies it, and a + * new path cannot omit a cell. The companion gate test keeps the entries + * honest (referenced symbols must exist, waivers must carry reasons). + * + * Waived cells with a `gap:` prefix are acknowledged debt — each should link + * an issue. They are the point of the registry: the debt is a diffable list + * a reviewer sees change, not folklore rediscovered on-device. + */ + +export const INTERACTION_GUARANTEES = [ + // Ambiguous matches resolve visible-first, then deepest, then smallest; + // remaining ties fail with "did not resolve uniquely". + 'disambiguation', + // Targets covered by another visible element are refused. + 'occlusion', + // The tap point (rect center) must lie inside the root viewport; closed + // drawers / off-viewport carousels are refused, not silently no-op tapped. + 'offscreen', + // Non-hittable targets are promoted to a hittable ancestor when possible + // and annotated (targetHittable/hint) when not. + 'nonHittable', + // Responses carry the shared field set (refLabel, selectorChain, evidence + // merge) assembled by the shared builders, never hand-rolled per branch. + 'responseFields', + // --verify captures a pre-action baseline and post-action digest. + 'verifyEvidence', + // Failures use the shared codes/messages/hints (no-match diagnostics, + // ambiguous shape, offscreen reasons). + 'errorTaxonomy', +] as const; + +export type InteractionGuarantee = (typeof INTERACTION_GUARANTEES)[number]; + +export const INTERACTION_PATH_IDS = [ + 'runtime-selector', + 'runtime-ref', + 'direct-ios-selector', + 'native-ref', + 'coordinate', + 'maestro-non-hittable-fallback', +] as const; + +export type InteractionPathId = (typeof INTERACTION_PATH_IDS)[number]; + +export type GuaranteeEnforcement = + | { + kind: 'runtime'; + /** `#` implementing the rule. */ + via: string; + } + | { + kind: 'runner'; + /** Swift symbol implementing the rule runner-side. */ + via: string; + /** Golden fixture table proving TS/Swift parity (ADR 0011 phase 3). */ + parityTable?: string; + } + | { + kind: 'delegated'; + to: InteractionPathId; + /** How the delegation is triggered (flag, error fallback, ...). */ + via: string; + } + | { + kind: 'inapplicable'; + reason: string; + } + | { + kind: 'waived'; + reason: string; + }; + +export type InteractionPathContract = { + description: string; + commands: readonly string[]; + guarantees: Record; +}; + +export const INTERACTION_DISPATCH_PATHS: Record = { + 'runtime-selector': { + description: 'Daemon tree capture, selector chain resolution, guarded coordinate tap.', + commands: ['press', 'click', 'fill', 'longpress'], + guarantees: { + disambiguation: { + kind: 'runtime', + via: 'src/daemon/selectors-resolve.ts#resolveSelectorChain', + }, + occlusion: { + kind: 'runtime', + via: 'src/snapshot/snapshot-occlusion.ts#isSnapshotNodeInteractionBlocked', + }, + offscreen: { + kind: 'runtime', + via: 'src/snapshot/mobile-snapshot-semantics.ts#isNodeVisibleOnScreen', + }, + nonHittable: { + kind: 'runtime', + via: 'src/core/interaction-targeting.ts#resolveActionableTouchResolution', + }, + responseFields: { + kind: 'runtime', + via: 'src/daemon/handlers/interaction-touch-targets.ts#interactionResultExtra', + }, + verifyEvidence: { + kind: 'runtime', + via: 'src/commands/interaction/runtime/interactions.ts#pressCommand', + }, + errorTaxonomy: { + kind: 'runtime', + via: 'src/daemon/selectors-resolve.ts#formatSelectorFailure', + }, + }, + }, + 'runtime-ref': { + description: 'Session snapshot ref lookup, guarded coordinate tap.', + commands: ['press', 'click', 'fill', 'longpress'], + guarantees: { + disambiguation: { + kind: 'inapplicable', + reason: 'Refs identify exactly one node by construction.', + }, + occlusion: { + kind: 'runtime', + via: 'src/snapshot/snapshot-occlusion.ts#isSnapshotNodeInteractionBlocked', + }, + offscreen: { + kind: 'runtime', + via: 'src/snapshot/mobile-snapshot-semantics.ts#isNodeVisibleOnScreen', + }, + nonHittable: { + kind: 'runtime', + via: 'src/core/interaction-targeting.ts#resolveActionableTouchResolution', + }, + responseFields: { + kind: 'runtime', + via: 'src/daemon/handlers/interaction-touch-targets.ts#interactionResultExtra', + }, + verifyEvidence: { + kind: 'runtime', + via: 'src/commands/interaction/runtime/interactions.ts#pressCommand', + }, + errorTaxonomy: { + kind: 'runtime', + via: 'src/daemon/selectors-resolve.ts#STALE_REF_HINT', + }, + }, + }, + 'direct-ios-selector': { + description: + 'Simple selectors on iOS are sent to the XCTest runner, which queries and taps natively without a daemon tree capture.', + commands: ['press', 'fill'], + guarantees: { + disambiguation: { + kind: 'waived', + reason: + 'gap: runner findElement uses unique-hittable-or-AMBIGUOUS_MATCH, which differs from tree rules (no visible-first/deepest-smallest preference). Needs a parity table or delegation on AMBIGUOUS_MATCH.', + }, + occlusion: { + kind: 'waived', + reason: + 'gap: XCTest isHittable approximates occlusion but there is no explicit covered-element check on the direct path.', + }, + offscreen: { + kind: 'runner', + via: 'RunnerTests+Interaction.swift#onScreenWindowFrame', + }, + nonHittable: { + kind: 'waived', + reason: + 'gap: non-hittable matches are skipped runner-side (ELEMENT_NOT_FOUND) instead of promoted/annotated like the runtime path.', + }, + responseFields: { + kind: 'waived', + reason: + 'gap: the response is built from the runner payload; refLabel/selectorChain are absent on the direct path.', + }, + verifyEvidence: { + kind: 'delegated', + to: 'runtime-selector', + via: '--verify disables the direct path (readDirectIosSelectorTapTarget / fill flags.verify check)', + }, + errorTaxonomy: { + kind: 'waived', + reason: + 'gap: ELEMENT_NOT_FOUND/AMBIGUOUS_MATCH lack the selector diagnostics and hints the runtime path attaches.', + }, + }, + }, + 'native-ref': { + description: + 'click @ref / fill @ref dispatch to backend.tapTarget/fillTarget without runtime resolution when no non-default options are set.', + commands: ['click', 'fill'], + guarantees: { + disambiguation: { + kind: 'inapplicable', + reason: 'Refs identify exactly one node by construction.', + }, + occlusion: { + kind: 'waived', + reason: 'gap: no covered-element check before the native ref tap.', + }, + offscreen: { + kind: 'waived', + reason: + 'gap: no viewport check before the native ref tap; relies on runner-side ref resolution behavior.', + }, + nonHittable: { + kind: 'waived', + reason: 'gap: no promotion/annotation on the native ref path.', + }, + responseFields: { + kind: 'runtime', + via: 'src/daemon/handlers/interaction-touch-targets.ts#interactionResultExtra', + }, + verifyEvidence: { + kind: 'delegated', + to: 'runtime-ref', + via: '--verify disables the native ref fast path (maybeTapRefTarget / maybeFillRefTarget verify check)', + }, + errorTaxonomy: { + kind: 'runtime', + via: 'src/daemon/selectors-resolve.ts#STALE_REF_HINT', + }, + }, + }, + coordinate: { + description: 'Raw x/y tap. Semantics are intentionally minimal.', + commands: ['press', 'click', 'fill', 'longpress'], + guarantees: { + disambiguation: { + kind: 'inapplicable', + reason: 'Coordinates name a point, not an element.', + }, + occlusion: { + kind: 'inapplicable', + reason: 'Coordinates bypass element semantics by design (escape hatch).', + }, + offscreen: { + kind: 'waived', + reason: + 'gap: out-of-viewport coordinates are forwarded as-is; a bounds warning would be cheap.', + }, + nonHittable: { + kind: 'inapplicable', + reason: 'No element to promote or annotate.', + }, + responseFields: { + kind: 'inapplicable', + reason: 'No resolved node, so no refLabel/selectorChain.', + }, + verifyEvidence: { + kind: 'runtime', + via: 'src/commands/interaction/runtime/resolution.ts#resolveInteractionTarget', + }, + errorTaxonomy: { + kind: 'runtime', + via: 'src/kernel/errors.ts#normalizeError', + }, + }, + }, + 'maestro-non-hittable-fallback': { + description: + 'Replay-only coordinate fallback for non-hittable elements (allowNonHittableCoordinateFallback), matching Maestro semantics.', + commands: ['press', 'fill'], + guarantees: { + disambiguation: { + kind: 'runner', + via: 'RunnerTests+Interaction.swift#findElement', + }, + occlusion: { + kind: 'waived', + reason: 'Intentional: Maestro taps resolved bounds regardless of overlay state.', + }, + offscreen: { + kind: 'runner', + via: 'RunnerTests+Interaction.swift#hasTappableFrame', + }, + nonHittable: { + kind: 'waived', + reason: 'Intentional: the entire point of this path is tapping non-hittable elements.', + }, + responseFields: { + kind: 'runtime', + via: 'src/daemon/handlers/interaction-touch.ts#handleTouchInteractionCommands', + }, + verifyEvidence: { + kind: 'inapplicable', + reason: 'Replay-only path; --verify is not part of replay semantics.', + }, + errorTaxonomy: { + kind: 'waived', + reason: 'gap: shares the direct path error shapes, including their missing hints.', + }, + }, + }, +}; From e43ce24038212d99bbb794110f7ec064009bc367 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 4 Jul 2026 13:43:20 +0200 Subject: [PATCH 2/3] refactor: apply ADR 0011 design review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Frame Layer 1 as an honesty/completeness gate, not a truth gate: it proves every path declared a stance and referenced symbols exist; behavioral parity starts with the Layer-2/3 fixture and scenario work. - Split responseFields into responseConstruction (one shared response construction site — a single Layer-2 refactor) and responseIdentity (which identity fields a path can provide — per-path capability work); note the anticipated errorTaxonomy split (codes vs diagnostics). - Encode the hybrid gap-closure strategy: runner-side parity for geometry-local rules, delegation-on-error for semantic failures (with the explicit caveat that delegation-on-error is NOT success-path parity), and a shared runtime preflight for native-ref where a silent backend success means delegation never triggers. - Gap waivers now require a trackingIssue (gate-enforced URL); all 16 pinned gaps link the umbrella issue #1081. The honest reclassification grew the pin list from 10 to 16 — responseConstruction is a gap on every path including runtime ones, which is exactly the partial progress the coarser guarantee was hiding. - Align ADR wording with the code: parityTable is optional until Layer 3, required once a runner cell claims parity. --- .../0011-interaction-guarantee-contract.md | 67 ++++++++-- .../__tests__/interaction-guarantees.test.ts | 98 +++++++++----- src/contracts/interaction-guarantees.ts | 121 ++++++++++++++---- 3 files changed, 216 insertions(+), 70 deletions(-) diff --git a/docs/adr/0011-interaction-guarantee-contract.md b/docs/adr/0011-interaction-guarantee-contract.md index c799700930..0a4c1e3822 100644 --- a/docs/adr/0011-interaction-guarantee-contract.md +++ b/docs/adr/0011-interaction-guarantee-contract.md @@ -50,25 +50,45 @@ shared implementations prevent **drift**, and generated test coverage enforces ### Layer 1 — Declare: the matrix as a typed registry with a gate +Layer 1 is an **honesty/completeness gate, not a truth gate**: it proves that +every path has declared a stance on every guarantee and that the referenced +implementations exist. It does not prove the declared behavior — behavioral +parity only starts once the fixture tables (Layer 2) and contract scenarios +(Layer 3) land. Landing Layer 1 alone still changes the failure mode: an +unwatched cell becomes impossible; an unproven cell is at least a visible, +owned claim. + `src/contracts/interaction-guarantees.ts` declares both axes and requires every cell to be classified: ```ts export const INTERACTION_GUARANTEES = [ - 'disambiguation', // visible > deepest > smallest; ties fail - 'occlusion', // covered targets are refused - 'offscreen', // tap point (rect center) must lie in the root viewport - 'nonHittable', // promotion + targetHittable/hint annotation - 'responseFields', // refLabel/selectorChain/evidence assembled by the shared builder - 'verifyEvidence', // --verify baseline + post-action digest - 'errorTaxonomy', // no-match/ambiguous/offscreen codes, messages, hints + 'disambiguation', // visible > deepest > smallest; ties fail + 'occlusion', // covered targets are refused + 'offscreen', // tap point (rect center) must lie in the root viewport + 'nonHittable', // promotion + targetHittable/hint annotation + 'responseConstruction', // one shared response construction site (Layer 2) + 'responseIdentity', // refLabel/selectorChain availability on this path + 'verifyEvidence', // --verify baseline + post-action digest + 'errorTaxonomy', // no-match/ambiguous/offscreen codes, messages, hints ] as const; +``` + +`responseConstruction` and `responseIdentity` are deliberately separate: "use +one shared construction site" and "which identity fields this path can +provide" have different closure strategies (the former is a single Layer-2 +refactor; the latter is per-path capability work). `errorTaxonomy` is expected +to split the same way later — stable codes/fallback classification vs rich +selector diagnostics and hints — because direct runner paths can close codes +long before full diagnostics. + +```ts export type GuaranteeEnforcement = | { kind: 'runtime'; via: string } // shared TS implementation (symbol name) - | { kind: 'runner'; via: string; parityTable: string } // Swift twin + golden fixture table proving parity + | { kind: 'runner'; via: string; parityTable?: string } // Swift twin; parityTable optional until Layer 3, required once the cell claims parity | { kind: 'delegated'; to: InteractionPathId } // path defers (e.g. direct → runtime on ELEMENT_OFFSCREEN) - | { kind: 'waived'; reason: string }; // explicit, reviewed waiver + | { kind: 'waived'; reason: string; trackingIssue?: string }; // explicit, reviewed waiver; gap waivers must carry a tracking issue export const INTERACTION_DISPATCH_PATHS: Record< InteractionPathId, @@ -88,9 +108,10 @@ a path without classifying every guarantee. A unit gate - every `via` string resolves to a real exported symbol (declarations cannot rot into fiction); - every `parityTable` names an existing fixture file; -- every `waived` reason is non-empty (waivers are visible, reviewed debt — the - initial classification will honestly contain several, and each one links an - issue). +- every `waived` reason is non-empty, and every `gap:` waiver carries a + `trackingIssue` — waivers must be owned, not merely visible. One umbrella + tracking issue with sub-issues split off as work is scheduled is + sufficient; the gate enforces the link, not the granularity. This is the same "make the gap declare itself" pattern already proven in this repo by `scripts/integration-progress-model.ts` (which caught the unclassified @@ -148,6 +169,28 @@ The gate closes the loop: it walks the Layer-1 registry and fails when any non-waived cell has no contract case tagged for it. Coverage of the matrix is therefore by construction, not by reviewer memory. +### Closing the gaps: a hybrid strategy, not one answer + +The acknowledged gaps close by different mechanisms depending on what the +guarantee needs: + +- **Runner-side parity for cheap geometry-local rules** (offscreen / + tappable-frame on the direct iOS path). These are pure frame math, provable + with golden tables, and keep the fast path fast. +- **Delegation-on-error for semantic and rich-runtime cases** + (`ELEMENT_OFFSCREEN`, `AMBIGUOUS_MATCH`, `ELEMENT_NOT_FOUND`, non-hittable + refusal): the fast path fails cheaply, and the runtime path supplies + disambiguation and full diagnostics only when needed. **Delegation-on-error + is not success-path parity**: it cannot catch the case where XCTest finds + one hittable candidate that runtime rules would refuse or rank differently. + Those cells stay `gap:` waivers until parity tables or contract scenarios + prove the success path too. +- **A shared runtime preflight for the native-ref path**: the ref came from a + daemon snapshot, so the node is already in hand — check offscreen / + occlusion / non-hittable against it *before* calling + `tapTarget`/`fillTarget`. A backend fast path can silently "succeed", so + delegation-on-error would never trigger there. + ### Timeout policy joins the descriptor registry The `wait` timeout bug existed because request-envelope budgets and diff --git a/src/contracts/__tests__/interaction-guarantees.test.ts b/src/contracts/__tests__/interaction-guarantees.test.ts index 55ef47e6c0..a2e786f13f 100644 --- a/src/contracts/__tests__/interaction-guarantees.test.ts +++ b/src/contracts/__tests__/interaction-guarantees.test.ts @@ -82,40 +82,65 @@ test('runner enforcement entries reference symbols present in runner sources', ( } }); -test('delegations point at real paths and waivers carry reasons', () => { +function eachEnforcement( + visit: ( + pathId: string, + guarantee: (typeof INTERACTION_GUARANTEES)[number], + enforcement: (typeof INTERACTION_DISPATCH_PATHS)[keyof typeof INTERACTION_DISPATCH_PATHS]['guarantees'][(typeof INTERACTION_GUARANTEES)[number]], + ) => void, +): void { for (const [pathId, contract] of Object.entries(INTERACTION_DISPATCH_PATHS)) { - for (const [guarantee, enforcement] of Object.entries(contract.guarantees)) { - if (enforcement.kind === 'delegated') { - assert.ok( - (INTERACTION_PATH_IDS as readonly string[]).includes(enforcement.to), - `${pathId}/${guarantee}: delegated to unknown path ${enforcement.to}`, - ); - assert.notEqual( - enforcement.to, - pathId, - `${pathId}/${guarantee}: a path cannot delegate to itself`, - ); - assert.ok( - enforcement.via.trim().length > 0, - `${pathId}/${guarantee}: delegation must say how it triggers`, - ); - const target = - INTERACTION_DISPATCH_PATHS[enforcement.to].guarantees[ - guarantee as (typeof INTERACTION_GUARANTEES)[number] - ]; - assert.ok( - target.kind === 'runtime' || target.kind === 'runner', - `${pathId}/${guarantee}: delegates to ${enforcement.to}, which does not enforce it (${target.kind})`, - ); - } - if (enforcement.kind === 'waived' || enforcement.kind === 'inapplicable') { - assert.ok( - enforcement.reason.trim().length > 10, - `${pathId}/${guarantee}: ${enforcement.kind} requires a substantive reason`, - ); - } + for (const guarantee of INTERACTION_GUARANTEES) { + visit(pathId, guarantee, contract.guarantees[guarantee]); } } +} + +test('delegations point at real paths that enforce the guarantee', () => { + eachEnforcement((pathId, guarantee, enforcement) => { + if (enforcement.kind !== 'delegated') return; + assert.ok( + (INTERACTION_PATH_IDS as readonly string[]).includes(enforcement.to), + `${pathId}/${guarantee}: delegated to unknown path ${enforcement.to}`, + ); + assert.notEqual( + enforcement.to, + pathId, + `${pathId}/${guarantee}: a path cannot delegate to itself`, + ); + assert.ok( + enforcement.via.trim().length > 0, + `${pathId}/${guarantee}: delegation must say how it triggers`, + ); + const target = INTERACTION_DISPATCH_PATHS[enforcement.to].guarantees[guarantee]; + assert.ok( + target.kind === 'runtime' || target.kind === 'runner', + `${pathId}/${guarantee}: delegates to ${enforcement.to}, which does not enforce it (${target.kind})`, + ); + }); +}); + +test('waivers and inapplicable entries carry substantive reasons', () => { + eachEnforcement((pathId, guarantee, enforcement) => { + if (enforcement.kind !== 'waived' && enforcement.kind !== 'inapplicable') return; + assert.ok( + enforcement.reason.trim().length > 10, + `${pathId}/${guarantee}: ${enforcement.kind} requires a substantive reason`, + ); + }); +}); + +test('gap waivers are owned by tracking issues', () => { + eachEnforcement((pathId, guarantee, enforcement) => { + if (enforcement.kind !== 'waived' || !enforcement.reason.startsWith('gap:')) return; + // Waivers must be owned, not just visible: every acknowledged gap links + // the umbrella tracking issue or a sub-issue split from it. + assert.match( + enforcement.trackingIssue ?? '', + /^https:\/\/github\.com\/callstack\/agent-device\/issues\/\d+$/, + `${pathId}/${guarantee}: gap waiver requires a trackingIssue URL`, + ); + }); }); test('acknowledged gaps are visible and bounded', () => { @@ -128,17 +153,24 @@ test('acknowledged gaps are visible and bounded', () => { } } // CONSERVATIVE: this list may only shrink, or grow in the same PR that - // updates it here with a linked issue. It is the diffable debt list. + // updates it here with a linked issue. It is the diffable debt list + // (umbrella: https://github.com/callstack/agent-device/issues/1081). assert.deepEqual(gaps.sort(), [ 'coordinate/offscreen', + 'coordinate/responseConstruction', 'direct-ios-selector/disambiguation', 'direct-ios-selector/errorTaxonomy', 'direct-ios-selector/nonHittable', 'direct-ios-selector/occlusion', - 'direct-ios-selector/responseFields', + 'direct-ios-selector/responseConstruction', + 'direct-ios-selector/responseIdentity', 'maestro-non-hittable-fallback/errorTaxonomy', + 'maestro-non-hittable-fallback/responseConstruction', 'native-ref/nonHittable', 'native-ref/occlusion', 'native-ref/offscreen', + 'native-ref/responseConstruction', + 'runtime-ref/responseConstruction', + 'runtime-selector/responseConstruction', ]); }); diff --git a/src/contracts/interaction-guarantees.ts b/src/contracts/interaction-guarantees.ts index 2a12025b40..b09dcb1c0a 100644 --- a/src/contracts/interaction-guarantees.ts +++ b/src/contracts/interaction-guarantees.ts @@ -6,14 +6,24 @@ * parity table once ADR 0011 phase 3 lands), delegated to another path, * inapplicable by construction, or explicitly waived with a reason. * + * This registry plus its gate test is an HONESTY/COMPLETENESS gate, not a + * truth gate: it proves every path has declared a stance and that referenced + * symbols exist. Behavioral parity is only proven once the golden fixture + * tables (Layer 2) and contract scenarios (Layer 3) land. + * * The `Record` over the guarantee union makes completeness a compile error: * adding a guarantee refuses to build until every path classifies it, and a * new path cannot omit a cell. The companion gate test keeps the entries - * honest (referenced symbols must exist, waivers must carry reasons). + * honest (referenced symbols must exist, waivers must carry reasons, and + * every `gap:` waiver must carry a tracking issue). * - * Waived cells with a `gap:` prefix are acknowledged debt — each should link - * an issue. They are the point of the registry: the debt is a diffable list - * a reviewer sees change, not folklore rediscovered on-device. + * Closure strategy for the acknowledged gaps is hybrid (see ADR 0011): + * runner-side parity for cheap geometry-local rules; delegation-on-error for + * semantic/rich-runtime failures (which is NOT success-path parity — cells + * where the fast path can succeed on a candidate the runtime rules would + * refuse stay gaps until proven); and a shared runtime preflight against the + * already-captured snapshot node for the native-ref path, because a backend + * fast path can silently succeed and delegation-on-error never triggers. */ export const INTERACTION_GUARANTEES = [ @@ -28,13 +38,21 @@ export const INTERACTION_GUARANTEES = [ // Non-hittable targets are promoted to a hittable ancestor when possible // and annotated (targetHittable/hint) when not. 'nonHittable', - // Responses carry the shared field set (refLabel, selectorChain, evidence - // merge) assembled by the shared builders, never hand-rolled per branch. - 'responseFields', + // Response payloads are assembled by a single shared construction site, + // never hand-rolled per branch (the class of bug that dropped fill @ref + // evidence). Closure is ADR 0011 Layer 2 (buildInteractionResponseData). + 'responseConstruction', + // The identity fields a path can echo back: refLabel, selectorChain, the + // resolved target. Distinct from construction — a path may build responses + // through the shared site yet be unable to provide identity fields. + 'responseIdentity', // --verify captures a pre-action baseline and post-action digest. 'verifyEvidence', // Failures use the shared codes/messages/hints (no-match diagnostics, - // ambiguous shape, offscreen reasons). + // ambiguous shape, offscreen reasons). NOTE: expected to split into + // errorCodes (stable codes / fallback classification) vs errorDiagnostics + // (rich selector diagnostics and hints) once direct runner paths close + // codes earlier than full diagnostics. 'errorTaxonomy', ] as const; @@ -61,7 +79,10 @@ export type GuaranteeEnforcement = kind: 'runner'; /** Swift symbol implementing the rule runner-side. */ via: string; - /** Golden fixture table proving TS/Swift parity (ADR 0011 phase 3). */ + /** + * Golden fixture table proving TS/Swift parity. Optional until ADR 0011 + * Layer 3 lands; required once a runner cell claims parity. + */ parityTable?: string; } | { @@ -77,6 +98,8 @@ export type GuaranteeEnforcement = | { kind: 'waived'; reason: string; + /** Required when the reason starts with `gap:` — waivers must be owned. */ + trackingIssue?: string; }; export type InteractionPathContract = { @@ -85,6 +108,8 @@ export type InteractionPathContract = { guarantees: Record; }; +const GAPS_UMBRELLA_ISSUE = 'https://github.com/callstack/agent-device/issues/1081'; + export const INTERACTION_DISPATCH_PATHS: Record = { 'runtime-selector': { description: 'Daemon tree capture, selector chain resolution, guarded coordinate tap.', @@ -106,7 +131,13 @@ export const INTERACTION_DISPATCH_PATHS: Record Date: Sat, 4 Jul 2026 13:57:03 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20address=20registry=20review=20?= =?UTF-8?q?=E2=80=94=20maestro=20disambiguation=20honesty,=20command-scope?= =?UTF-8?q?d=20verify?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. maestro-non-hittable-fallback/disambiguation was overclaimed: the guarantee is defined as visible-first/deepest/smallest ranking, but findElement only implements unique-or-ambiguous scanning. Reclassified as an intentional waiver (deliberate Maestro-semantics divergence), mirroring how the direct path keeps its success-path parity gap. 2. verifyEvidence was claimed path-wide on paths that dispatch longpress, which has no --verify. Cells can now be command-scoped via appliesTo (non-empty strict subset of the path's commands, gate-enforced), and the three affected cells scope to press/click/fill. --- .../0011-interaction-guarantee-contract.md | 6 +++++ .../__tests__/interaction-guarantees.test.ts | 23 +++++++++++++++++++ src/contracts/interaction-guarantees.ts | 20 +++++++++++++--- 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/docs/adr/0011-interaction-guarantee-contract.md b/docs/adr/0011-interaction-guarantee-contract.md index 0a4c1e3822..54da902f2a 100644 --- a/docs/adr/0011-interaction-guarantee-contract.md +++ b/docs/adr/0011-interaction-guarantee-contract.md @@ -74,6 +74,12 @@ export const INTERACTION_GUARANTEES = [ ] as const; ``` +Cells may be **command-scoped** via `appliesTo` when a guarantee only exists +for a subset of the path's commands — e.g. `verifyEvidence` applies to +`press`/`click`/`fill` but not `longpress`, and claiming it path-wide would +overstate coverage. The gate rejects `appliesTo` entries naming commands the +path does not dispatch, and rejects redundant full-coverage lists. + `responseConstruction` and `responseIdentity` are deliberately separate: "use one shared construction site" and "which identity fields this path can provide" have different closure strategies (the former is a single Layer-2 diff --git a/src/contracts/__tests__/interaction-guarantees.test.ts b/src/contracts/__tests__/interaction-guarantees.test.ts index a2e786f13f..2be2f660aa 100644 --- a/src/contracts/__tests__/interaction-guarantees.test.ts +++ b/src/contracts/__tests__/interaction-guarantees.test.ts @@ -130,6 +130,29 @@ test('waivers and inapplicable entries carry substantive reasons', () => { }); }); +test('command-scoped guarantees only name commands the path actually dispatches', () => { + for (const [pathId, contract] of Object.entries(INTERACTION_DISPATCH_PATHS)) { + for (const guarantee of INTERACTION_GUARANTEES) { + const enforcement = contract.guarantees[guarantee]; + if (enforcement.appliesTo === undefined) continue; + assert.ok( + enforcement.appliesTo.length > 0, + `${pathId}/${guarantee}: appliesTo must be non-empty when present`, + ); + for (const command of enforcement.appliesTo) { + assert.ok( + contract.commands.includes(command), + `${pathId}/${guarantee}: appliesTo names "${command}", which the path does not dispatch`, + ); + } + assert.ok( + enforcement.appliesTo.length < contract.commands.length, + `${pathId}/${guarantee}: appliesTo covers every path command — drop it, omission means all`, + ); + } + } +}); + test('gap waivers are owned by tracking issues', () => { eachEnforcement((pathId, guarantee, enforcement) => { if (enforcement.kind !== 'waived' || !enforcement.reason.startsWith('gap:')) return; diff --git a/src/contracts/interaction-guarantees.ts b/src/contracts/interaction-guarantees.ts index b09dcb1c0a..3281826bdd 100644 --- a/src/contracts/interaction-guarantees.ts +++ b/src/contracts/interaction-guarantees.ts @@ -69,7 +69,7 @@ export const INTERACTION_PATH_IDS = [ export type InteractionPathId = (typeof INTERACTION_PATH_IDS)[number]; -export type GuaranteeEnforcement = +type GuaranteeEnforcementBase = | { kind: 'runtime'; /** `#` implementing the rule. */ @@ -102,6 +102,16 @@ export type GuaranteeEnforcement = trackingIssue?: string; }; +export type GuaranteeEnforcement = GuaranteeEnforcementBase & { + /** + * Command scoping: when a guarantee only applies to a subset of the path's + * commands (e.g. --verify exists on press/click/fill but not longpress), + * the cell names that subset instead of implying path-wide coverage. Must + * be a non-empty subset of the path's `commands`; omitted = all commands. + */ + appliesTo?: readonly string[]; +}; + export type InteractionPathContract = { description: string; commands: readonly string[]; @@ -144,6 +154,7 @@ export const INTERACTION_DISPATCH_PATHS: Record