fix(cloud): unblock iOS snapshot and gate cloud fill on text-entry focus (#1658) - #1666
Conversation
|
Reviewed
Branch blocker: the PR conflicts with |
d3760b6 to
2099757
Compare
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
|
Rebased onto P1 — The stub now models the device rather than the protocol: focus lands a beat after the tap, and P2 — the budget was advertised, not enforced. Correct on both counts. Probes now carry their own bound (1.5s, threaded through the client as a per-request timeout override) instead of inheriting the 30s default, so the 2s wait cannot stretch toward it. On classification, chasing this down surfaced a sharper version of your point: I had keyed "unimplemented" partly on HTTP status, but W3C Branch/CI. Conflict is gone. The Smoke Tests failure on Still outstanding, and I want to be plain about it: there is no exact-head live BrowserStack/AWS evidence. This environment has no BrowserStack credentials and no Generated by Claude Code |
|
Re-reviewed exact head The no-keyboard path is fixed: it now refuses before sending keys. The provider scenario now models delayed focus and dropped unfocused keys, asserts the field value/snapshot, and the WebDriver error classification correctly distinguishes an unsupported route from dead-session/grid failures. Two blockers remain:
CI is now green and the branch is mergeable. Exact-head BrowserStack/AWS device evidence is still absent; the author confirms no credentials were available, so the stub scenario does not close that device-facing validation gap. |
…cus (#1658) Two bugs isolated to the cloud-webdriver iOS path. `snapshot`/`diff` refused every capture on a live BrowserStack session with SESSION_NOT_FOUND, instantly and without a driver round trip. 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 it now applies to local Apple targets only. The session was empty in the first place because the provider open path skips local app resolution wholesale — no simctl/devicectl reaches a hosted device — and dropped an explicitly spelled bundle id along with it. A dotted, non-deep-link target is the bundle id under the same convention resolveIosApp applies locally, so a cloud `open com.example.app` now records it. `fill` tapped and sent its keys in back-to-back requests. A WebView input — an OAuth page in a Safari view controller — takes first responder asynchronously, so the keys landed with nothing focused while the command 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 cloud interactor now waits on the same signal the Apple runner uses, the software keyboard going from hidden to shown after its tap, and discloses what it observed as `textEntryReadiness` so a fill with no witness cannot pass for a filled field. Where keyboard visibility cannot witness the focus move — back-to-back fills into one form, the shape that failed most often — it spends the runner's full readiness budget rather than racing the app with a short settle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Zjbzf7HdziX9SzFpNP7WW
Adopting an explicitly spelled bundle id on a provider-backed open (the fix that makes snapshot/diff work at all) also made a previously dead precedence rule live: the provider branch returned currentAppBundleId first, so once a first open had populated it, `open com.a` followed by `open com.b` left the session still reporting com.a to every appBundleId-gated command. The local path does the opposite, and is the convention this branch is meant to mirror: resolveIosApp returns a dotted target unchanged and never consults the session's current app. Only its deep-link branches prefer the tracked id. Flip the provider branch to match — an explicit bundle-id target wins, and everything the branch cannot name (deep links, display names, bare open) still falls back to the tracked id.
…lled Review follow-ups on #1658. `not-observed` was still a success: it sent the keys and answered "Filled N chars", and nothing renders `textEntryReadiness` in default CLI output — so the exact silent success this branch exists to remove survived whenever focus never happened. A tap that raises no keyboard now fails with `text_entry_focus_not_observed` and sends no keys, leaving the field untouched rather than half-written, and the readiness vocabulary keeps only outcomes that describe a fill that did type. The readiness budget was advertised but not enforced at the request boundary: each keyboard probe inherited the client's 30s default, so one hung probe could hold a 2s wait for far longer. Probes now carry their own bound, threaded through the client as a per-request timeout override. The probe also swallowed every error as "this driver cannot answer", which degraded a dead session, an auth rejection, or a grid outage into a blind text entry. Only a positively classified unimplemented route counts as unsupported now — classified on the W3C error code rather than the status, since `unknown command` and `invalid session id` share HTTP 404 — and everything else propagates. The provider scenario proved request ordering against a stub that always accepted keys. Its fake now models the device: focus lands a beat after the tap, and keys arriving while the keyboard is down are accepted and dropped, exactly as an unfocused field does. The tests assert the field's own value, and both go red against the pre-fix `fill`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Zjbzf7HdziX9SzFpNP7WW
|
Branch-readiness update: this unchanged head now conflicts with |
Review of cc23f2b found two ways a fill could still report success without evidence that OUR tap focused the field it was aimed at. P1. `settled-keyboard-up` and `settled-unknown` both typed and returned normal success. Keyboard visibility can only witness that *a* field took focus, never *which*: filling a second field in an already-open form reads the same before and after, so a missed tap left the first field focused and `POST /keys` — which the driver routes to whatever holds first responder — appended to it while every request returned 200. Failing those closed outright would have broken ordinary multi-field form fills, which do work: a live AWS Device Farm run types both fields of a WebView login correctly. So witness focus properly instead. W3C `GET /element/active` answers the question keyboard visibility cannot — is the thing focused now the thing I tapped — and answers it whether or not the keyboard was already up. That becomes the primary signal (`focused-element`); the keyboard transition stays as the fallback for drivers without the route, and a keyboard already up on such a driver now refuses rather than typing. The test is identity, not geometry. Containment of the tap point looks like the obvious rule and is wrong: focusing a field can re-lay it out. On a live iPhone 16, tapping Safari's collapsed address bar expands it into a taller field that no longer covers the tapped point, and a containment-only rule refused a fill that plainly worked. So a tap that MOVES focus counts, with containment as the second half of the test — re-filling the already-focused field moves nothing, and only geometry tells that from a tap that missed. Both readings are taken before the tap, since each is evidence only as a change. P2. The 2s budget bounded the loop but not the calls inside it: every probe got a fixed 1500ms, so one begun near the deadline finished well past it. Both the probe timeout and the sleep are now capped by the remaining budget. Also fixes a related escape the review did not name: the poll loop had no catch, so one transient grid error aborted a fill the next poll would have satisfied. Probe failures are now tolerated within the budget, but a budget that expires without a single answered probe rethrows, so a dead session surfaces as itself rather than as "the tap missed". The provider scenario gains the two-field case the review asked for: it begins keyboard-up with the email field focused, misses the password tap, and asserts no keys reach the email field. Verified on AWS Device Farm iPhone 16 / iOS 18.0 at this exact tree: address bar (the re-layout case) and both WebView login fields all report `focused-element`, the second with the keyboard already up, and the device reads back `tomsmith` and a 20-character password.
cc23f2b to
a158434
Compare
|
Re-reviewed |
… the composite probe Two blockers from the review of 3a9aceb. P1. `settled-unknown` was the last path that typed without evidence: when both the active-element and keyboard routes are positively unsupported, `fill` settled 350ms, typed, and returned ordinary success. Nothing renders `textEntryReadiness`, so that reached a caller looking exactly like a fill that worked — the same silent false success #1658 is about, just narrowed to one branch. It now refuses with a distinct reason, `text_entry_focus_unobservable`: nothing is wrong with the target, the driver simply cannot answer, so the caller's next move differs from a missed tap and the hint names it — `press` then `type` stays the deliberate way to enter text unwitnessed. `CLOUD_TEXT_ENTRY_READINESS` is now `focused-element` and `keyboard-shown` only. Every value describes a fill that witnessed focus before sending a key; there is deliberately no value for typing blind. P2. `activeElement(timeoutMs)` bounded each of its two sequential requests by the full timeout rather than bounding the operation, so a probe handed the 1.5s left of a 2s readiness deadline could spend ~3s across `/element/active` and `/element/{id}/rect` and overrun the deadline it was derived from. It now derives one deadline at entry and gives the second request only what the first left, floored at zero so an already-spent budget aborts immediately instead of falling back to the client default. The regression pins elapsed transport time across both calls, which is what the defect is made of: the rect request answers only its own abort, so the time it was allowed to run IS the budget it was handed. It measures ~202ms of a shared 200ms budget before the fix and ~120ms after. Also updates the generic Cloud WebDriver facade scenario, whose stub answered `{value: null}` to everything and so read as a driver with neither route. It now answers the two focus probes, since that scenario exercises facade wiring rather than text-entry semantics — those live in cloud-webdriver-ios-text-entry.test.ts, which models focus properly.
|
Re-reviewed exact head
The supported-driver production behavior and exact-device evidence from the preceding head remain applicable; this delta tightens only the unsupported/error boundary and request budgeting. Code review is clean and this is ready for human review. |
thymikee
added
the
ready-for-human
2 participants
Fixes #1658 — two bugs isolated to the cloud-webdriver iOS path.
Bug A:
snapshot/difffail SESSION_NOT_FOUND on a live cloud sessionTwo defects stacked:
requireIosAppSessionForSnapshot(src/daemon/snapshot-runtime.ts) refused every iOS session with noappBundleId. That guard belongs to the local XCUITest runner, which must attach to a target app. A cloud capture isGET /session/:id/sourceagainst the provider's own driver — no app identity is involved, which is exactly whypress/find/fill/screenshotworked on the same session. It is a pure local pre-check, hence the reporter's 0ms failure with norunner.logever created. It now applies to local Apple targets only.The provider branch of the open path dropped an explicitly spelled bundle id (
resolveSessionAppBundleIdForTarget,src/daemon/handlers/session-open-target.ts). It returnscurrentAppBundleIdand nothing else, because nosimctl/devicectlreaches a hosted device. On a firstopenthat isundefined, soopen com.example.appprintedOpened: ...while storing no app identity at all. A dotted, non-deep-link target is the bundle id under the same conventionresolveIosAppapplies locally, so it is now adopted with zero device round trips.Both changes are needed: (2) makes
openmean something on cloud iOS; (1) makes a capture work on a provider session even before one.Bug B:
fillsilently no-ops on WebView inputsWebDriverInteractor.fillwastap(x, y)followed bysendKeys(text)in back-to-back requests. A WebView input — an OAuth/SSO page in a Safari view controller — does not take first responder synchronously with the tap, andPOST /keysgoes to whatever holds first responder, so the keys vanished while the command still answeredFilled N chars. That also explains the confirmed workaround: tap-then-fill as two separate commands only worked because the round trip between them gave the field time to focus.The cloud interactor now waits on the same signal the local Apple runner uses (
RunnerTests+TextEntry.swift): the software keyboard going hidden → shown after its own tap. The keyboard is read before the tap, because only a transition proves our tap moved focus. Where visibility cannot witness the focus move — back-to-back fills into one form, the shape that failed most often in the report — it spends the runner's full readiness budget rather than racing the app with a short settle.fillnow discloses what it observed astextEntryReadiness, so a fill with no witness can no longer pass for a filled field:keyboard-shownsettled-keyboard-upsettled-unknownnot-observedDeliberately not fixed
The iPhone 17 / iOS 26 report of
fillandpress+typeresolving to a coordinate far from the field (and once navigating the WebView to the app's marketing site) is target resolution, not timing:fillalready re-captures a fresh page source before resolving, soeditable=truemost likely matched a different node — a Safari view controller's top bar exposes text-field-shaped elements. That needs a raw snapshot from the affected screen to chase, and is better handled separately from this fix.Verification
No live device was reachable from this environment, so verification runs against the repo's provider-scenario harness, which drives the real daemon and real provider runtime against a stub Appium server.
test/integration/provider-scenarios/cloud-webdriver-ios-text-entry.test.ts(new) reproduces both bugs end to end. Confirmed failing on the pre-fix code with the issue's exact error text, and passing after.diffguard still firing, and the bundle-id adoption rules including the cases that must adopt nothing.pnpm test:unit(5599 passed),pnpm test:integration:provider(153 passed), typecheck, lint, format,check:layering,check:fallow,check:production-exports— all green.🤖 Generated with Claude Code
https://claude.ai/code/session_015Zjbzf7HdziX9SzFpNP7WW
Generated by Claude Code