fix: reject distinct ambiguous mutation targets (#1667)

* fix: reject distinct ambiguous mutation targets

* fix(ios): scope the raw-match rejection to mutating dispatches

`RunnerTests+Interaction.findElement` applied the new fail-closed
classification to `querySelector` as well as press/type, because the read
call site takes the default `allowNonHittableFallback: false`. With one
visible/hittable match and one non-hittable same-selector duplicate the
query started returning AMBIGUOUS_MATCH where it previously selected the
hittable element, and `queryDirectIosSelectorOrFallback` preserves that
error for read callers — so `get`, `is`, and `wait` surfaced an error
instead of their prior answer.

`classifyDirectSelectorCandidates` now takes a `rawMatchPolicy`. Mutations
keep `.rejectDistinctMatches` (the default, so no mutation call site
changes); `queryElement` passes `.preferHittableMatch`, restoring the
prior read rule: prefer the single hittable match, ambiguous only when
hittable matches compete, and never adopt the Maestro coordinate fallback.
The Maestro expected-point path is untouched.

Covers the one-hittable + one-non-hittable read, competing hittable reads,
and the non-hittable-only read. ADR 0011's amendment now states the scope.

* test(ios): execute selector read ambiguity regression

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: thymikee

.github/workflows/ios.yml

@@ -97,6 +97,7 @@ jobs:
             -only-testing:AgentDeviceRunnerUITests/RunnerTests/testBareDelayedTypeFailsWhenTappedInputDisappearsMidCommand \
             -only-testing:AgentDeviceRunnerUITests/RunnerTests/testSynthesizedTextCommitProgressWalksExpectedPrefixOnly \
             -only-testing:AgentDeviceRunnerUITests/RunnerTests/testTextEntryTapWitnessIsBoundToTargetIdentity \
+            -only-testing:AgentDeviceRunnerUITests/RunnerTests/testQuerySelectorPrefersHittableMatchOverNonHittableDuplicate \
             -only-testing:AgentDeviceRunnerUITests/RunnerTests/testActivateTargetSkipsForegroundAndActivatesNonForegroundApplication \
             -only-testing:AgentDeviceRunnerUITests/RunnerTests/testMissingBundleCommandInvalidatesCompleteCachedTargetState \
             -only-testing:AgentDeviceRunnerUITests/RunnerTests/testCachedTargetInvalidationClearsProcessBoundState \

CHANGELOG.md

@@ -5,6 +5,7 @@
 - `agent-device help workflow` is now a compact ~8KB card instead of a ~41KB dump; the same depth still exists, split into `help scripting` (save-script, secret-safe fills, batch JSON, replay divergence/repair, recording) and `help gestures` (multi-touch shapes and platform quirks), plus a few paragraphs folded into the topics that already owned the subject (`help debugging`, `help physical-device`, `help validate`). Every `help <topic>` first line is now `agent-device <version> — <topic>` so an agent can read the installed version from its mandatory first help read instead of a separate `agent-device --version` call.
 - Cloud iOS (BrowserStack, AWS Device Farm): `snapshot` and `diff` no longer fail with `SESSION_NOT_FOUND` on a live provider session (#1658). The app-session guard they ran belongs to the local XCUITest runner, which must attach to a target app; a cloud capture reads the provider's own driver session and needs no app identity, so the guard now applies to local Apple targets only. Relatedly, a cloud iOS `open com.example.app` now records that bundle id on the session — the provider path skips local app resolution (no simctl/devicectl reaches a hosted device), and used to drop an explicitly spelled bundle id along with it, leaving the session with no app identity at all. Opening a second bundle id replaces the first, matching the local path, where an explicitly spelled target always wins over the session's current app; deep links, display names, and bare `open` still keep the app already tracked.
 - Cloud `fill` (BrowserStack, AWS Device Farm) now witnesses that the field it tapped actually holds text-entry focus before sending its keys, instead of dispatching tap and keys in back-to-back requests (#1658). A WebView input — an OAuth/SSO page in a Safari view controller, for example — takes first responder asynchronously, so the keys used to land with nothing focused while `fill` still answered "Filled N chars"; tapping and filling as two separate commands worked only because the round trip between them gave the field time to focus. The witness is the focused element's own geometry: `fill` polls the active element and proceeds only once it contains the point it tapped, which is the one signal that identifies *which* field took focus. Keyboard visibility cannot — it reads the same before and after a second fill into an already-open form, so it could not tell a focused password field from the email field the previous fill left focused. The response discloses `textEntryReadiness`: `focused-element`, or `keyboard-shown` when the driver has no active-element route but the keyboard rose from hidden after the tap. Both describe a fill that witnessed focus before typing; there is deliberately no value for typing without evidence, because nothing renders this field and such a value would reach a caller as an ordinary success. Breaking: when focus cannot be witnessed, cloud `fill` now FAILS with `COMMAND_FAILED` / `text_entry_focus_not_observed` and sends no keys, instead of typing into whatever holds first responder and answering "Filled N chars" — a fill with no witness must not read as a filled field. That covers a tap that focused nothing, a keyboard already up on a driver that cannot name the focused field, and a driver that reports neither (`text_entry_focus_unobservable`, which points at `press` + `type` as the deliberate way to enter text unwitnessed). Only a positively classified unimplemented route counts as unsupported, so a dead session, an auth rejection, or a grid outage surfaces instead of degrading into a blind text entry.
+- Changed mutating selector ambiguity semantics (press/click/fill/longpress): duplicate accessibility wrappers collapse only when every match forms one ancestor-descendant chain resolving to the same actionable node. Matches in distinct subtrees now fail fast with `AMBIGUOUS_MATCH` and a bounded, immediately reusable candidate-ref frame; visible/depth/area geometry no longer silently picks a mutation target. The direct iOS XCTest path now counts raw exact matches before hittability and delegates ambiguity to the same runtime rule. AppControlBench provenance: element-14 ran on 0.20.5; this change is intended for 0.20.7+, and comparative benchmark reports should note that it can replace a wrong-success recovery loop with one candidate-pick turn while occasionally adding that turn for genuinely distinct duplicates.
 - `scroll` and `back` now accept `--settle` (with `--settle-quiet` and `--timeout`), collapsing scroll-then-observe and back-then-observe into one call (#1638). The response carries the same settled payload the touch commands return — verdict, changed-lines diff with fresh refs on added lines, the unchanged-interactive tail, and `refsGeneration` when the settled tree was stored — and is best-effort: it never fails the action. One difference is deliberate: `scroll`/`back` resolve no element, so the diff baseline is the session's stored pre-action tree ("the last tree you observed") rather than a freshly resolved pre-action capture. Both commands now also preserve the daemon on timeout, like the other settle-capable commands.
 - Security: repository `./agent-device.json` now accepts only project-safe automation defaults. It rejects daemon endpoint/auth/transport/server settings, tenant/run/lease selectors, provider/cloud and Metro connection fields, headers, executable reporter modules, local write destinations, and other operator-controlled values before local module loading or any daemon health/RPC request. Put remote endpoint and token together in protected CI environment variables, user config, an explicit `--config` file, or the existing `connect`/`--remote-config` workflow. Daemon auth tokens no longer travel in serialized command flags.
 - `viewport` is now rejected during capability admission on Apple targets instead of reaching the device and failing inside dispatch. No Apple backend can resize a screen — simulator and device geometry is fixed by the selected device type — so `viewport` on iOS/iPadOS/tvOS/macOS now fails with `UNSUPPORTED_OPERATION`, `viewport is not supported on this device`, and a hint pointing at `--platform web` and at picking a different simulator. `capabilities` no longer advertises `viewport` on Apple targets. Web viewport resizing (`agent-device viewport 1280 900 --platform web`) is unchanged, and Android was already denied.

apple/runner/AgentDeviceRunner/AgentDeviceRunner/AgentDeviceRunnerApp.m

@@ -105,6 +105,26 @@ - (void)viewDidLoad {
       [textField.heightAnchor constraintEqualToConstant:44],
     ]];
   }
+
+  if ([NSProcessInfo.processInfo.arguments containsObject:@"--agent-device-selector-read-regression"]) {
+    NSString *const duplicateIdentifier = @"agent-device-selector-read-duplicate";
+
+    UIButton *visibleButton = [UIButton buttonWithType:UIButtonTypeSystem];
+    visibleButton.accessibilityIdentifier = duplicateIdentifier;
+    [visibleButton setTitle:@"Readable target" forState:UIControlStateNormal];
+    visibleButton.translatesAutoresizingMaskIntoConstraints = NO;
+    [self.view addSubview:visibleButton];
+
+    UILabel *offscreenLabel = [[UILabel alloc] initWithFrame:CGRectMake(-200, -200, 100, 40)];
+    offscreenLabel.accessibilityIdentifier = duplicateIdentifier;
+    offscreenLabel.text = @"Decorative duplicate";
+    [self.view addSubview:offscreenLabel];
+
+    [NSLayoutConstraint activateConstraints:@[
+      [visibleButton.centerXAnchor constraintEqualToAnchor:self.view.centerXAnchor],
+      [visibleButton.topAnchor constraintEqualToAnchor:label.bottomAnchor constant:24],
+    ]];
+  }
 #endif
 }
 

apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerSelectorMatchPolicy.swift

@@ -0,0 +1,78 @@
+struct SelectorCandidateFacts {
+  let isHittable: Bool
+  let hasTappableFrame: Bool
+  let containsExpectedPoint: Bool
+
+  init(
+    isHittable: Bool,
+    hasTappableFrame: Bool,
+    containsExpectedPoint: Bool = true
+  ) {
+    self.isHittable = isHittable
+    self.hasTappableFrame = hasTappableFrame
+    self.containsExpectedPoint = containsExpectedPoint
+  }
+}
+
+enum DirectSelectorCandidateDecision: Equatable {
+  case noMatch
+  case selected(index: Int, usedNonHittableFallback: Bool)
+  case ambiguous
+}
+
+/// How many raw exact matches a dispatch may discard before hittability is
+/// allowed to pick a winner. The two rows differ because the cost of guessing
+/// wrong differs, not because the matching differs.
+enum DirectSelectorRawMatchPolicy: Equatable {
+  /// Mutations fail closed: every raw exact match counts, so a hittable
+  /// element can never silently win over a same-selector duplicate the caller
+  /// never saw and act on the wrong one.
+  case rejectDistinctMatches
+  /// Reads prefer the single hittable match and ignore non-hittable
+  /// same-selector duplicates. A read has no side effect to guard, and
+  /// `querySelector` backs `get`/`is`/`wait` — failing those closed turns a
+  /// decorative duplicate into an error where the reader previously got its
+  /// answer.
+  case preferHittableMatch
+}
+
+/// Normal direct selector mutations count every raw exact match before
+/// hittability can choose a winner. Reads keep the hittable-preference rule,
+/// and Maestro's explicitly requested coordinate fallback keeps its
+/// point-filtered compatibility behavior.
+func classifyDirectSelectorCandidates(
+  _ candidates: [SelectorCandidateFacts],
+  allowNonHittableFallback: Bool,
+  filtersByExpectedPoint: Bool = false,
+  rawMatchPolicy: DirectSelectorRawMatchPolicy = .rejectDistinctMatches
+) -> DirectSelectorCandidateDecision {
+  let eligible = candidates.indices.filter { index in
+    !filtersByExpectedPoint || candidates[index].containsExpectedPoint
+  }
+
+  if !allowNonHittableFallback && rawMatchPolicy == .rejectDistinctMatches {
+    guard eligible.count <= 1 else { return .ambiguous }
+    guard let index = eligible.first, candidates[index].isHittable else { return .noMatch }
+    return .selected(index: index, usedNonHittableFallback: false)
+  }
+
+  var hittableIndex: Int?
+  var fallbackIndex: Int?
+  for index in eligible {
+    let candidate = candidates[index]
+    if candidate.isHittable {
+      guard hittableIndex == nil else { return .ambiguous }
+      hittableIndex = index
+    } else if allowNonHittableFallback && candidate.hasTappableFrame {
+      guard fallbackIndex == nil else { return .ambiguous }
+      fallbackIndex = index
+    }
+  }
+  if let hittableIndex {
+    return .selected(index: hittableIndex, usedNonHittableFallback: false)
+  }
+  if let fallbackIndex {
+    return .selected(index: fallbackIndex, usedNonHittableFallback: true)
+  }
+  return .noMatch
+}

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

@@ -1653,7 +1653,7 @@ extension RunnerTests {
     return .context(ActiveCommandContext(app: activeApp))
   }
 
-  private func executeOnMainPrepared(
+  func executeOnMainPrepared(
     command: Command,
     activeApp: XCUIApplication,
     alertDeadline: Date? = nil

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

@@ -129,7 +129,8 @@ extension RunnerTests {
     selectorKey: String,
     selectorValue: String,
     allowNonHittableFallback: Bool = false,
-    expectedPoint: CGPoint? = nil
+    expectedPoint: CGPoint? = nil,
+    rawMatchPolicy: DirectSelectorRawMatchPolicy = .rejectDistinctMatches
   ) -> SelectorElementMatch {
     let value = selectorValue.trimmingCharacters(in: .whitespacesAndNewlines)
     guard !value.isEmpty else {
@@ -149,35 +150,32 @@ extension RunnerTests {
       return SelectorElementMatch(element: nil, isAmbiguous: false, usedNonHittableFallback: false)
     }
 
-    var matchedElement: XCUIElement?
-    var nonHittableElement: XCUIElement?
     let matches = app.descendants(matching: .any).matching(predicate).allElementsBoundByIndex
-    for element in matches where element.exists {
-      if let expectedPoint, !element.frame.contains(expectedPoint) {
-        continue
-      }
-      if !element.isHittable {
-        if allowNonHittableFallback && hasTappableFrame(app: app, element: element) {
-          guard nonHittableElement == nil else {
-            return SelectorElementMatch(element: nil, isAmbiguous: true, usedNonHittableFallback: false)
-          }
-          nonHittableElement = element
-        }
-        continue
-      }
-      guard matchedElement == nil else {
-        return SelectorElementMatch(element: nil, isAmbiguous: true, usedNonHittableFallback: false)
-      }
-      matchedElement = element
+      .filter(\.exists)
+    let facts = matches.map { element in
+      SelectorCandidateFacts(
+        isHittable: element.isHittable,
+        hasTappableFrame: hasTappableFrame(app: app, element: element),
+        containsExpectedPoint: expectedPoint.map(element.frame.contains) ?? true
+      )
     }
-    if let matchedElement {
-      return SelectorElementMatch(element: matchedElement, isAmbiguous: false, usedNonHittableFallback: false)
+    switch classifyDirectSelectorCandidates(
+      facts,
+      allowNonHittableFallback: allowNonHittableFallback,
+      filtersByExpectedPoint: expectedPoint != nil,
+      rawMatchPolicy: rawMatchPolicy
+    ) {
+    case .noMatch:
+      return SelectorElementMatch(element: nil, isAmbiguous: false, usedNonHittableFallback: false)
+    case .ambiguous:
+      return SelectorElementMatch(element: nil, isAmbiguous: true, usedNonHittableFallback: false)
+    case let .selected(index, usedNonHittableFallback):
+      return SelectorElementMatch(
+        element: matches[index],
+        isAmbiguous: false,
+        usedNonHittableFallback: usedNonHittableFallback
+      )
     }
-    return SelectorElementMatch(
-      element: nonHittableElement,
-      isAmbiguous: false,
-      usedNonHittableFallback: nonHittableElement != nil
-    )
   }
 
   // Maestro-compat gate for the non-hittable coordinate fallback: an element
@@ -209,7 +207,17 @@ extension RunnerTests {
   }
 
   func queryElement(app: XCUIApplication, selectorKey: String, selectorValue: String) -> Response {
-    let match = findElement(app: app, selectorKey: selectorKey, selectorValue: selectorValue)
+    // querySelector is a read — it backs get/is/wait and the offscreen-refusal
+    // double-check, none of which mutate. The fail-closed raw-match rule exists
+    // to stop a mutation acting on an unseen duplicate; applying it here would
+    // instead turn a decorative non-hittable duplicate into an AMBIGUOUS_MATCH
+    // for readers that previously resolved the hittable element.
+    let match = findElement(
+      app: app,
+      selectorKey: selectorKey,
+      selectorValue: selectorValue,
+      rawMatchPolicy: .preferHittableMatch
+    )
     if match.isAmbiguous {
       return Response(ok: false, error: ErrorPayload(code: "AMBIGUOUS_MATCH", message: "selector matched multiple elements"))
     }

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

@@ -0,0 +1,122 @@
+import XCTest
+
+extension RunnerTests {
+#if AGENT_DEVICE_RUNNER_UNIT_TESTS
+  func testDirectSelectorRejectsTwoRawMatchesBeforeHittabilityPreference() {
+    let decision = classifyDirectSelectorCandidates(
+      [
+        SelectorCandidateFacts(isHittable: true, hasTappableFrame: true),
+        SelectorCandidateFacts(isHittable: false, hasTappableFrame: true),
+      ],
+      allowNonHittableFallback: false
+    )
+
+    XCTAssertEqual(decision, .ambiguous)
+  }
+
+  func testDirectSelectorAcceptsOneRawHittableMatch() {
+    XCTAssertEqual(
+      classifyDirectSelectorCandidates(
+        [SelectorCandidateFacts(isHittable: true, hasTappableFrame: true)],
+        allowNonHittableFallback: false
+      ),
+      .selected(index: 0, usedNonHittableFallback: false)
+    )
+  }
+
+  // The read rows below are the regression guard for scoping the fail-closed
+  // rule to mutations: querySelector backs get/is/wait, so the exact shape
+  // that must stay resolvable is one hittable match beside a non-hittable
+  // same-selector duplicate.
+  func testReadSelectorPrefersTheHittableMatchOverANonHittableDuplicate() {
+    let decision = classifyDirectSelectorCandidates(
+      [
+        SelectorCandidateFacts(isHittable: true, hasTappableFrame: true),
+        SelectorCandidateFacts(isHittable: false, hasTappableFrame: true),
+      ],
+      allowNonHittableFallback: false,
+      rawMatchPolicy: .preferHittableMatch
+    )
+
+    XCTAssertEqual(decision, .selected(index: 0, usedNonHittableFallback: false))
+  }
+
+  func testReadSelectorStillRejectsTwoHittableMatches() {
+    XCTAssertEqual(
+      classifyDirectSelectorCandidates(
+        [
+          SelectorCandidateFacts(isHittable: true, hasTappableFrame: true),
+          SelectorCandidateFacts(isHittable: true, hasTappableFrame: true),
+        ],
+        allowNonHittableFallback: false,
+        rawMatchPolicy: .preferHittableMatch
+      ),
+      .ambiguous
+    )
+  }
+
+  // A read never coordinate-taps, so a non-hittable-only match stays a miss
+  // rather than borrowing the Maestro fallback.
+  func testReadSelectorDoesNotAdoptTheNonHittableCoordinateFallback() {
+    XCTAssertEqual(
+      classifyDirectSelectorCandidates(
+        [SelectorCandidateFacts(isHittable: false, hasTappableFrame: true)],
+        allowNonHittableFallback: false,
+        rawMatchPolicy: .preferHittableMatch
+      ),
+      .noMatch
+    )
+  }
+
+  func testMaestroSelectorKeepsExpectedPointAndNonHittableFallbackSemantics() {
+    XCTAssertEqual(
+      classifyDirectSelectorCandidates(
+        [
+          SelectorCandidateFacts(isHittable: false, hasTappableFrame: true, containsExpectedPoint: false),
+          SelectorCandidateFacts(isHittable: false, hasTappableFrame: true, containsExpectedPoint: true),
+        ],
+        allowNonHittableFallback: true,
+        filtersByExpectedPoint: true
+      ),
+      .selected(index: 1, usedNonHittableFallback: true)
+    )
+  }
+
+#if os(iOS)
+  func testQuerySelectorPrefersHittableMatchOverNonHittableDuplicate() throws {
+    let duplicateIdentifier = "agent-device-selector-read-duplicate"
+    app.launchArguments = ["--agent-device-selector-read-regression"]
+    app.launch()
+    defer {
+      invalidateCachedTarget(reason: "unit_test_cleanup")
+      app.terminate()
+    }
+    XCTAssertTrue(app.waitForExistence(timeout: appExistenceTimeout))
+
+    let matches = app.descendants(matching: .any)
+      .matching(identifier: duplicateIdentifier)
+      .allElementsBoundByIndex
+      .filter(\.exists)
+    XCTAssertEqual(matches.count, 2, "fixture must expose two raw identifier matches")
+    XCTAssertEqual(matches.filter(\.isHittable).count, 1, "fixture must expose exactly one hittable match")
+
+    let command = try JSONDecoder().decode(
+      Command.self,
+      from: Data(
+        #"{"command":"querySelector","commandId":"query-selector-duplicate","selectorKey":"id","selectorValue":"agent-device-selector-read-duplicate"}"#.utf8
+      )
+    )
+    let response = try executeOnMainPrepared(command: command, activeApp: app)
+
+    guard response.ok else {
+      XCTFail(String(describing: response.error))
+      return
+    }
+    XCTAssertEqual(response.data?.found, true)
+    XCTAssertEqual(response.data?.nodes?.count, 1)
+    XCTAssertEqual(response.data?.nodes?.first?.identifier, duplicateIdentifier)
+    XCTAssertEqual(response.data?.nodes?.first?.hittable, true)
+  }
+#endif
+#endif
+}