Skip to content

fix: harden iOS XCTest recovery paths#1158

Open
thymikee wants to merge 1 commit into
mainfrom
agent/issue-1156-ios-runner-recovery
Open

fix: harden iOS XCTest recovery paths#1158
thymikee wants to merge 1 commit into
mainfrom
agent/issue-1156-ios-runner-recovery

Conversation

@thymikee

@thymikee thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Fixes #1156.

Reframes iOS snapshot recovery around the XCTest accessibility channel instead of a simulator-only tree backend penalty. Regular snapshots now avoid all XCTest-backed tiers after slow or timed-out XCTest accessibility work and derive recovery from non-XCTest backend traits, while raw diagnostics keep tree-first behavior.

Improves back --in-app by trying native navigation buttons, then semantic top-band Back/Close/Cancel controls, before falling back to a bounded top-leading tap. This avoids false-positive coordinate success on React Native custom search headers.

Validation

  • pnpm build
  • pnpm build:xcuitest
  • git diff --check
  • Verified on thymikee-iphone with Bluesky: snapshot Explore, focus search by ref, type agent-device, run back --in-app, and confirm the follow-up snapshot returned to Explore.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.6 MB 1.6 MB -3.0 kB
JS gzip 508.6 kB 507.7 kB -850 B
npm tarball 611.7 kB 612.3 kB +664 B
npm unpacked 2.2 MB 2.2 MB +3.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.0 ms 27.0 ms -0.0 ms
CLI --help 55.3 ms 53.9 ms -1.3 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/logcat.js +16.3 kB +5.8 kB
dist/src/internal/daemon.js -778 B -169 B
dist/src/tv-remote.js -502 B -108 B
dist/src/session.js -109 B -39 B
dist/src/registry.js -26 B -10 B

@thymikee thymikee force-pushed the agent/issue-1156-ios-runner-recovery branch 2 times, most recently from 7b5b8c8 to 93b1fc3 Compare July 8, 2026 15:49
@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Code-quality review — #1158 (fix: harden iOS XCTest recovery paths)

Reviewed the full diff plus the surrounding runner code it touches. The core direction is sound and one part is genuinely elegant, but there are two behavioral/design issues in the two halves of this PR that I'd want resolved before merge, plus a couple of cleanups.

What's good (keep it)

  • The SnapshotBackendKind.usesXCTestAccessibilityChannel trait is the right move. It collapses scattered kind == .recursiveTree / kind != .privateAX checks into one semantic property, so effectiveSnapshotCapturePlan, shouldSkipSnapshotBackendForAbandonedTreeCapture, and the record helpers now all read off the same concept. The rename (tree backendXCTest channel) is accurate, and deriving the recovery plan via plan.filter { !$0.usesXCTestAccessibilityChannel } instead of a hardcoded [.privateAX, .querySweep, .recursiveTree] is the correct generalization. The !recoveryPlan.isEmpty guard is a good defensive touch. The pure static helpers on the back side are well unit-tested.

1. (Blocking-ish, behavior) On a physical device the derived recovery plan is always [.privateAX] — and privateAX is a compile-time no-op there. So the widened penalty turns every snapshot sparse for 120s, including on healthy screens.

This is the interaction of two changes in the PR:

  • The penalty gate widened from #if os(iOS) && targetEnvironment(simulator)#if os(iOS) (physical devices now reorder).
  • privateAX is the only non-XCTest backend, and privateAXSnapshotCapture is #if os(iOS) && targetEnvironment(simulator) — on device its body compiles to return nil.

So on device, effectiveSnapshotCapturePlan under penalty always yields exactly [.privateAX], which always returns nil, which falls straight through to the sparse fallback payload. Concrete sequence:

  1. First hostile snapshot grinds ~30s (unchanged), trips penalizeSnapshotXCTestChannel (120s window).
  2. Every subsequent snapshot for that bundle for 120s → plan [.privateAX]nilsparse root, instantly.
  3. Navigate to a healthy screen within that window → still [.privateAX] → still sparse, even though recursiveTree would have returned a real tree. Nothing clears the penalty early, and because the only recovery tier no-ops we never re-probe to discover the screen recovered.

On simulator this is fine (privateAX returns real nodes). On device it means "one hostile screen → 2 minutes of empty snapshots on every screen of that app." That's a real regression vs. main, where device was never penalized and would have captured the healthy screen. It also makes the state: "recovered" / AGENT_DEVICE_RUNNER_SNAPSHOT_RECOVERED stamping misleading on device — nothing is recovered, we just gave up fast.

The trait-filter is elegant precisely because it hides which backends exist per platform — but here it silently produces a plan of exclusively non-functional backends. If fail-fast-sparse is the intended device tradeoff (the ADR edit hints it is), please make it explicit rather than emergent from a compile-time no-op — e.g. detect "recovery plan has no runnable backend on this platform" and either (a) use a much shorter device penalty, or (b) re-probe with a small XCTest budget instead of blanket-sparse for the full 120s. At minimum, a device-path unit test pinning this behavior would keep the invariant honest.

2. (Blocking-ish, design contradiction + latency) back --in-app tier 2 leans right into the XCTest channel the rest of the PR treats as hostile — with no budget.

topNavigationBackElement does app.buttons.matching(predicate).allElementsBoundByIndex and then per-candidate .exists / .isHittable / .frame / .label / .identifier — every one a synchronous XCTest round-trip, ~5 per button, no deadline. That's the same enumeration channel the issue reports grinding to 30s on these RN screens. On main, back only touched navigationBars.buttons (empty → fast 399ms fail). This PR can turn that fast fail into a multi-second grind on exactly the screens #1156 is about.

There's a nice code-judo move that also unifies the two halves' model of the channel: consult the penalty you just generalized. If isSnapshotXCTestChannelPenalized(bundleId:) is set, skip tier 2 entirely and go straight to the coordinate fallback (and/or wrap the enumeration in a small runMainThreadWork budget). One half of the PR learns the XCTest AX channel is dangerous; the other half should get to use that knowledge instead of re-discovering it the slow way.

3. (Design question) Tier 3 makes back --in-app report ok: true for a blind top-leading tap that may have hit nothing — the honest "not available" signal is gone.

tapTopLeadingNavigationFallback returns true whenever the coordinate tap performs (which it essentially always does). So on iOS tapInAppBackControl now almost never returns false, and the caller almost never emits in-app back control is not available. The PR summary says the goal is to avoid false-positive coordinate success on RN custom search headers — but tier 3 just relocates that false-positive to the top-left corner (which on a search header may be the field, a logo, or empty). If "always attempt something and let the follow-up snapshot verify" is intended, fine — but it's worth a comment saying so, because as written it trades a fast honest failure for an unconditional optimistic success, which is the opposite of what the summary claims.


Cleanups (not blocking)

4. Two sources of truth for the back/close/cancel keyword set. The 6-clause predicate (label CONTAINS[c] %@ OR identifier CONTAINS[c] %@ … ×3) and navigationBackControlRank independently hardcode the same three keywords in the same priority order. Add "dismiss" to one and forget the other and they silently drift. Collapse to a single ordered table and drive both from it, e.g.:

static let navigationBackKeywords = ["back", "close", "cancel"] // index = rank

static func navigationBackControlRank(label: String, identifier: String) -> Int? {
  let text = "\(label) \(identifier)".lowercased()
  return navigationBackKeywords.firstIndex { text.contains($0) }
}

and build the predicate by flat-mapping the same list into label/identifier CONTAINS[c] clauses. Removes the duplication and the wall of repeated %@.

5. File size / decomposition. This lands the navigation-back cluster in RunnerTests+Interaction.swift, pushing it to 1625 lines. The cluster (topNavigationBackElement, navigationBackControlRank, isTopNavigationControlFrame, topLeadingNavigationFallbackPoint, tapTopLeadingNavigationFallback + 4 tests) is cohesive and mostly pure — a clean RunnerTests+Navigation.swift. This also lines up with the in-flight issue-1110-interaction-test-split work; worth extracting rather than growing the grab-bag further.

6. Uncommented magic constants. The header-band and fallback-point clamps (0.22/96/180, 0.08/28/44, 0.155/56/132) have zero rationale, in contrast to the heavily-commented snapshot side. Worth a one-liner each. Note the y term is effectively dead on standard phones: for a 932pt-tall frame, 932*0.155 = 144 → clamped to 132, so 132 is the real constant for common devices and the proportion almost never bites — which a reader can't tell without doing the arithmetic.

7. Doc nit (ADR 0004). The edited sentence reads "Argent's ax-service can describe simulator screens that XCTest recursive snapshots and typed XCUIElementQuery enumeration can degrade to no useful child nodes" — the relative clause got mangled (screens don't degrade to no child nodes; the snapshots do). Restore a clean two-clause form.


Verdict: the abstraction work (trait + rename) is a real improvement and I'd keep it as-is. But #1 and #2 are genuine behavior concerns for the physical-device case this PR is explicitly targeting (#1156), and they pull in opposite directions on the same XCTest-channel question — I'd want those two reconciled (and #3 confirmed as intended) before merge. #4#7 are straightforward cleanups.

@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Review finding: back --in-app can now report success without finding an in-app back control. In tapTopLeadingNavigationFallback, any delivered synthesized or fallback coordinate tap returns true, so tapInAppBackControl/backInApp reports success on any iOS screen with a valid window frame, including root screens or headers where the top-left tap does nothing or hits a non-navigation control. That regresses the existing contract where absence of an in-app control returned in-app back control is not available. Please gate this fallback on an observed top-band candidate or verify that navigation/state changed before returning success; otherwise keep returning false when no control can be proved.

@thymikee thymikee force-pushed the agent/issue-1156-ios-runner-recovery branch 2 times, most recently from 696515d to 57311fb Compare July 8, 2026 16:12
@thymikee thymikee force-pushed the agent/issue-1156-ios-runner-recovery branch from 57311fb to a81fa24 Compare July 8, 2026 18:29
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.

iOS physical device: latest npm 0.19.0 still hits XCTest main-thread timeouts in RN overlay/list states

1 participant