Skip to content

fix: reject distinct ambiguous mutation targets - #1667

Open
thymikee wants to merge 2 commits into
mainfrom
agent/fail-fast-ambiguous-mutations
Open

fix: reject distinct ambiguous mutation targets#1667
thymikee wants to merge 2 commits into
mainfrom
agent/fail-fast-ambiguous-mutations

Conversation

@thymikee

@thymikee thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

Fail mutating selectors closed when same-label matches belong to distinct accessibility subtrees, while preserving benign React Native wrapper chains by collapsing only a single ancestor-descendant chain that resolves to one actionable node.

The direct iOS path now counts raw exact matches before hittability and delegates ambiguous matches to the shared runtime. Rejections publish a bounded partial ref frame across CLI and MCP so an agent can immediately retry with a pinned @ref.

Root cause: the mutation policy used geometric disambiguation, so element-14 silently selected one of the distinct Team Standup targets. The #1597 candidate renderer had shipped, but this interaction path neither rejected nor published candidates.

The scope crosses selector policy, runtime and direct-iOS guarantees, CLI/MCP recovery, contract coverage, ADRs, help, and changelog.

Validation

  • Exact pushed head edc2c96c: VITEST_MAX_WORKERS=2 pnpm check:affected --run passed all selected checks and 4,014 tests; fresh iOS and macOS XCTest builds succeeded. The worker cap only reduced host contention; no timeout or assertion was relaxed.
  • Live Element-14 proof on the immediately preceding rebased head 64d08193 (the selector patch is unchanged in edc2c96c; the final rebase only incorporates fix(test): clean Swift toolchain temporary directories #1664's Swift test temporary-directory cleanup): in-room press 'label="Team Standup"' failed closed before mutation with AMBIGUOUS_MATCH, five distinct candidates, and refsGeneration: 440518. Retrying press @e13~s440518 succeeded and opened the intended room-history screen.
  • Replay shadow on rebased head 01b3c7478: the 39-flow React Navigation Maestro suite passed 39/39 in 1092.3s, each on one attempt with zero heals.
  • A repeat replay on 64d08193 was invalidated by host infrastructure: CoreSimulatorService/simdiskimaged crashed after 22 passing flows (connection invalid, BUILD INTERRUPTED), then the target shut down and the remaining flows failed at stopApp. This is not claimed as a product pass. Repeated read-only simctl list calls continued to reproduce the host service crash, preventing a trustworthy final-head rerun without disrupting other simulator work.
  • Element sessions were closed and disposable Element clones deleted. No simulator named bench-golden* was booted or targeted. One disposable React Navigation simulator remains pending deletion once CoreSimulatorService stabilizes.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.99 MB 1.99 MB +2.2 kB
JS gzip 642.6 kB 643.4 kB +751 B
npm tarball 770.6 kB 772.6 kB +2.0 kB
npm unpacked 2.70 MB 2.70 MB +5.6 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.7 ms 27.3 ms +0.7 ms
CLI --help 65.0 ms 65.4 ms +0.4 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/prepare-kind.js +1.5 kB +499 B
dist/src/cli-help.js +269 B +110 B
dist/src/interaction.js +279 B +106 B
dist/src/internal/daemon.js 0 B -2 B
dist/src/runtime.js 0 B -2 B

@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Review: code is clean. The fail-closed structural policy, candidate publication/MCP pinning, and direct-iOS fallback align with ADRs 0010, 0011, 0012, and 0014; all CI checks are green.\n\nThis is not merge-ready yet. The required exact-head 39-flow replay/device evidence against the actual Element-14 motivating state is still missing, including both the ambiguous rejection and retry using a returned pinned ref. The reported synthetic gearshape.fill simulator run does not validate that motivating path. Please attach that replay/device evidence, then request re-review.

@thymikee
thymikee force-pushed the agent/fail-fast-ambiguous-mutations branch from 388a007 to 01b3c74 Compare August 7, 2026 12:56
@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Required exact-head evidence is now complete on 01b3c7478 after rebasing onto current main:

  • Element-14 motivating state: on a disposable non-golden clone, the in-room mutation press 'label="Team Standup"' returned AMBIGUOUS_MATCH with five distinct candidates (text, disabled text-field, table, cell, button) and refsGeneration: 805389. The screen did not mutate.
  • Pinned recovery: retrying the returned button candidate as press @e13~s805389 succeeded and opened the intended Team Standup room-history screen.
  • Replay shadow: the 39-flow React Navigation Maestro suite passed 39/39 in 1092.3s on the non-golden iPhone 17 Pro simulator. JUnit records failures="0", skipped="0"; every flow used one attempt and zero heals.
  • Local gate: VITEST_MAX_WORKERS=4 pnpm check:affected --run passed all selected checks and 3,995 tests. The worker cap only reduced host contention; no timeout or assertion was relaxed.
  • Cleanup: sessions/daemons closed, delegated XCTest runners terminated, disposable Element clone deleted. No simulator named bench-golden* was booted or targeted.

The earlier pre-rebase replay and the initial runner-handoff attempt are not claimed as exact-head evidence. This result is from the pushed rebased head. Ready for re-review once the refreshed GitHub checks complete.

@thymikee
thymikee marked this pull request as ready for review August 7, 2026 13:46
@thymikee thymikee closed this Aug 7, 2026
@thymikee thymikee reopened this Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-07 13:47 UTC

@thymikee
thymikee force-pushed the agent/fail-fast-ambiguous-mutations branch 2 times, most recently from 388a007 to 01b3c74 Compare August 7, 2026 13:50
@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 01b3c7478. The substantive patch is unchanged apart from expected rebase reconciliation, and the prior evidence blocker is closed: exact-head Element-14 ambiguity rejected with five candidates and no mutation, the pinned returned ref opened the intended room, and the replay shadow passed 39/39 with zero heals. Code and practical evidence are clean. The remaining blocker is branch state: GitHub reports DIRTY/CONFLICTING against current main (the merge-tree conflict is additive CHANGELOG.md). Rebase/resolve, then request a changed-head re-review.

@thymikee
thymikee force-pushed the agent/fail-fast-ambiguous-mutations branch from 01b3c74 to 64d0819 Compare August 7, 2026 14:24
@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 64d081931: range-diff shows the substantive selector patch is identical to reviewed head 01b3c747, so the code verdict remains clean and the existing Element-14/pinned-ref/39-flow evidence remains materially probative. The branch is still DIRTY/CONFLICTING against current main; merge-tree shows the additive CHANGELOG.md conflict. Resolve/rebase and request changed-head review. The device replay need not be repeated if the next range-diff remains patch-identical and base reconciliation is orthogonal.

@thymikee
thymikee force-pushed the agent/fail-fast-ambiguous-mutations branch from 64d0819 to edc2c96 Compare August 7, 2026 14:33
@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Final rebase is published at edc2c96c and is mergeable against current main. Exact-head affected gates pass 4,014/4,014 and fresh iOS/macOS XCTest builds pass. The Element-14 fail-fast + pinned-ref recovery proof was repeated on the immediately preceding rebased head 64d08193; the selector patch is byte-for-byte unchanged, and the final rebase only adds #1664 Swift test temp-directory cleanup. The PR body now records the prior 39/39 replay and the later CoreSimulatorService/simdiskimaged crash transparently. Fresh GitHub checks, including all four CodeQL languages, are running. Ready for re-review once they complete.

@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

CI babysitting update: every check except iOS smoke is green. iOS smoke failed three times in the pre-existing hidden-keyboard XCTest area, never in selector ambiguity code: (1) partial text "h" vs "hardware-keyboard"; (2) a different Xcode runner-app launch timeout while the first test passed; (3) the same partial-text assertion. PR #1676 (fix(test): wait for typed text to settle in the hidden-keyboard runner test) already fixes this exact assertion and is fully green/mergeable. I am not copying that unrelated fix into this PR; once #1676 lands on main, this branch can rebase and rerun cleanly.

@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Correction to the prior note: #1676 identifies the same failing test but is not an acceptable unblock as written; its post-response polling weakens the production-route oracle (see the P1 review there). #1673 attempted the production fix but was closed with work continuing elsewhere. The important classification is unchanged: this PR does not touch RunnerTests+CommandExecution.swift or synthesized-first-responder typing, and the only red check is exposing that known baseline production timing bug. No unrelated workaround belongs in #1667.

@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

P2 — scope raw-match rejection to mutations; it currently changes direct read behavior too.

RunnerTests+Interaction.findElement now applies classifyDirectSelectorCandidates(... allowNonHittableFallback: false) to querySelector as well as press/type. With one visible/hittable match and one non-hittable same-selector duplicate, the query now returns AMBIGUOUS_MATCH; before this change it selected the hittable element. queryDirectIosSelectorOrFallback preserves AMBIGUOUS_MATCH for read callers, so wait and is surface the error instead of using their prior behavior/tree path.

Keep the new raw-match rule on mutation dispatches only, or give querySelector its prior read policy (alternatively fall back to runtime resolution for the read). Please add coverage for that one-hittable + one-non-hittable read case.

`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.

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

P2 fixed in 9e1ffb2 — the raw-match rejection is now scoped to mutating dispatches.

classifyDirectSelectorCandidates takes a rawMatchPolicy:

  • .rejectDistinctMatches (the default, so no mutation call site changed) keeps the fail-closed rule: every raw exact match counts before hittability can pick a winner.
  • .preferHittableMatch restores the prior read rule — prefer the single hittable match, ambiguous only when hittable matches compete, and never adopt the Maestro coordinate fallback.

queryElement passes .preferHittableMatch. The Maestro expected-point / non-hittable path is untouched: when allowNonHittableFallback is true the policy is not consulted, so that branch is byte-identical in behavior.

Coverage in RunnerTests+SelectorMatchPolicyTests.swift, including the case you named:

  • one hittable + one non-hittable same-selector duplicate → .selected(index: 0), not .ambiguous
  • two hittable matches → still .ambiguous
  • non-hittable only → .noMatch, i.e. a read does not borrow the coordinate fallback

ADR 0011's 2026-08-07 amendment previously read as if the raw-match rule covered the whole direct XCTest path; it now says mutating dispatches only and states the read rule and why (get/is/wait sit behind querySelector, and a read has no side effect to guard).

Honest limits on this head. This was authored in a Linux container: there is no Swift toolchain and no Xcode, so the change is not compiled or run locally — the iOS Smoke lane is the first build. There is no simulator here either, so no device evidence: the one-hittable + one-non-hittable read has not been observed end-to-end through get/is/wait on a real screen. check:fallow passes on the 40 changed files; the TS side is unchanged by this commit.


Generated by Claude Code

@thymikee

thymikee commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Code routing is corrected at 9e1ffb2, but not ready yet. The three new SelectorMatchPolicy Swift regressions are compile-only: ios.yml runs an explicit -only-testing list and none is included. They also test only the classifier, so deleting queryElement’s .preferHittableMatch wiring leaves them green. Add an executed route-level querySelector/get/is/wait regression with red-before proof (and include it in the targeted XCTest gate), then attach exact-head iOS evidence for the one-hittable + non-hittable duplicate read path. Mutation routes correctly retain fail-closed behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants