Skip to content

Commit 7db5ad7

Browse files
authored
fix(ios): grant the text-entry commit wait time against progress (#2035)
* fix(ios): grant the text-entry commit wait time against progress The synthesized commit wait used a flat 3s deadline, which cannot tell a throttled simulator input pipeline (characters keep landing, slowly) from a wedged one (nothing lands) — it condemned both at the same instant and reported TEXT_INPUT_COMMIT_NOT_OBSERVED over a `type`/`fill` that was still working, on branches touching no iOS code. SynthesizedCommitBudget grants time against progress instead: while the observed value's expected-prefix grows — the same length-only evidence logCommitCadence already emits — the wait continues, up to a 10s ceiling. A pipeline making no progress expires at exactly the 3s the flat deadline used, so a wedge is condemned no later than before. It is a reference type, and the observe/expire coupling carries a structural guard, because as a struct that coupling would rest on Swift boxing one captured var and could revert to the flat deadline silently. Text-entry readiness' hardware-keyboard fallback also stops returning a possibly-unfocused element after 0.35s of "no software keyboard seen"; it now returns only on confirmed focus of the target and re-arms otherwise. And the keyboard-hidden precondition of testBareTypeUsesTappedInputWhenSoftwareKeyboardIsHidden skips rather than fails, so an environment flip cannot read as a product regression. The issue's remaining ask — pinning the simulator keyboard preference — is deliberately not done: measured on a dedicated simulator, per-device ConnectHardwareKeyboard makes no difference to a headless `simctl boot`, which always shows the software keyboard. See the PR body for the A/B. Refs #1874 — not a closing keyword on purpose. This is a mitigation; the unidentified simulator input-throttle mechanism that issue tracks is untouched here, so it stays open. * refactor(ios): move the commit-wait budget into the wait itself Review follow-up. The budget was a detached object tested in isolation, with a TypeScript parser asserting that two escaping Swift closures happened to share it — a guard that only existed because the seam was in the wrong place. The budget is now a local `var` inside `awaitSynthesizedCommitOutcome` and its replacement counterpart, advanced from the same observation the progress check already reads, with the clock injected alongside the existing observation and pacing seams. Recording progress and asking whether time is up are two statements in one loop, so there is no coupling left to guard. The detached tests and the TypeScript wiring guard are deleted. In their place, four sequence tests drive the shipped waits through a hand-driven clock: a prefix that keeps growing outlives the flat 3s deadline, a frozen prefix is condemned at exactly 3s, an indefinitely throttled pipeline stops at the 10s ceiling, and a value churning between two lengths buys no time. Verified red first — the two progress tests fail against a no-op `record`, and the two unchanged-behavior tests stay green. * fix(ios): grant the text-entry commit wait time against progress The synthesized commit wait started its clock before reading the field's placeholder, and that read is an AX round-trip which takes seconds on exactly the loaded host this budget exists for. Slow setup therefore spent the budget: with a 3.5s placeholder read the first observation already exceeded the 3s stall budget, so `type` reported TEXT_INPUT_COMMIT_NOT_OBSERVED after a single poll — sooner than the flat deadline this replaced, in the one condition it was written for. The budget is now two durations, and only the poll loop starts it, from its own first `now()`. Passing a pre-loop timestamp is no longer expressible. The poll also takes one clock sample instead of two, so the instant an observation is recorded at is the instant it is judged against. testCommitWaitBudgetStartsAtTheLoopRatherThanBeforeIt pins it: 60s of setup before the wait must still leave the full stall budget. Verified red against a deadline started outside the loop. * fix(test-app): stop the form fixture placing its own placeholder in every fill The `smoke:form-input` half of #1874 is not the commit deadline. This PR's own iOS lane reproduced it (run 32889322172) and the trace settles it: `wait start expectedLen=12`, then zero `[DEBUG-1874] poll` lines, then `wait outcome=notObserved elapsedMs=3608`. The wait never polled — it returned from the `textMatchesPlaceholder` guard, which refuses before polling because an empty text field renders its placeholder AS its accessibility value, so a match cannot prove a commit. `field-name`'s placeholder was "Ada Lovelace" and every checkout-form suite fills exactly "Ada Lovelace"; `field-email` had the same collision with "[email protected]". Twelve fills across eight files, so `fill` into those fields is unverifiable by contract. It looked intermittent only because the synthesized-replacement route is gated on `xCTestChannelPenalized` — it fires when the host is loaded — which is also why re-running a failed job on the same commit reproduced it identically. The collision also made the read-back assertions vacuous: `assertJsonContains( name, 'Ada Lovelace')` is satisfied by an empty field rendering the placeholder. Fixed in the fixture rather than in the values, because frozen replay-compat corpora carry the same fills and must not be edited. fixture-fill-placeholder-collision.test.ts guards the class: it fails on any repository fill whose value equals the target field's placeholder. * refactor(ios): drop the fill/placeholder source guard and flatten the commit deadline Review: the 83-line guard was a source-reconstruction test, not a fixture invariant. It regex-parsed JSX and two literal fill spellings and duplicated the Swift trim/equality rule in TypeScript, so it could stay green while its "every fill" claim was false — expressions, variables, typed clients and unlisted roots are all outside what a regex can enumerate. The owning evidence already exists: the Swift tests prove a placeholder-equal AX value is unobservable, and live smoke:form-input failed on the prior head for exactly this collision. Deleted; the two placeholder changes stay. Same pass over the rest of the change, for the same reason. The commit deadline was a budget value type, a nested Deadline type and a factory method; it is now one flat struct the poll loop constructs, with the two durations as defaulted parameters. Production call sites name no budget at all, tests name one only when they are asking about time, and SynthesizedCommitBudget.standard and the tests' unboundedCommitBudget both disappear. * refactor(ios): split the text-entry readiness and commit-wait seams Review: the change grew three files past their budgets. Splitting them along the seams they already had, no behavior change. RunnerTests+TextEntry.swift (607) keeps the vocabulary, field clearing and value reading at 259; everything that decides "which element is about to receive text, and has it taken focus" moves to RunnerTests+TextEntryReadiness.swift at 354. RunnerTests+SynthesizedTextEntry.swift (503) keeps the private-XCTest synthesis boundary, the replacement route and the route policies at 356. The commit wait moves next to the deadline that bounds it: the two waits, the observation and pacing they poll through, and the value-free cadence line that path may log now sit together in RunnerTests+SynthesizedCommitDeadline.swift at 206. That also puts every line touching the polled field value in one file, so apple-runner-log-redaction.test.ts guards a single surface — its path constant moves with it. The deadline's clock and sequence tests leave the policy tests (641 -> 494) for a sibling RunnerTests+SynthesizedCommitDeadlineTests.swift, which gains the replacement-route case the review asked for: a growing prefix carries the wait past the 3s stall budget and the 10s ceiling is what ends it. The injected clock is now defaulted, so only a test actually asking about time names it. * refactor(ios): split text-entry target acquisition from readiness Review residual: the readiness extraction was 354 lines and still owned two questions. Acquisition — the one-shot tap witness, post-tap stabilization, both focusTextInputForTextEntry entry points and the refresh point — moves to RunnerTests+TextEntryFocus.swift (206). Readiness keeps the waits, the keyboard signals they read and the focus corroboration (158). The dependency is one-way: acquisition asks readiness, never the reverse, so waitForTextEntryReadiness and keyboardBecameVisible lose file-private scope and nothing else does.
1 parent 8af2660 commit 7db5ad7

11 files changed

Lines changed: 882 additions & 489 deletions

‎.github/workflows/ios.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ jobs:
140140
-only-testing:AgentDeviceRunnerUITests/RunnerTests/testSinglePointerFlingFallsBackToXCTestCoordinateDragWhenPrivateSynthesisFails \
141141
-only-testing:AgentDeviceRunnerUITests/RunnerTests/testTypeWithoutResolvedInputReturnsTypedFailureBeforeDispatchingText \
142142
-only-testing:AgentDeviceRunnerUITests/RunnerTests/testBareTypeUsesTappedInputWhenSoftwareKeyboardIsHidden \
143+
-only-testing:AgentDeviceRunnerUITests/RunnerTests/testHardwareKeyboardResponderConfirmsItsOwnKeyboardFocus \
143144
-only-testing:AgentDeviceRunnerUITests/RunnerTests/testPenalizedCoordinateTapOnNonTextControlDoesNotAuthorizeBareType \
144145
-only-testing:AgentDeviceRunnerUITests/RunnerTests/testBareDelayedTypeFailsWhenTappedInputDisappearsMidCommand \
145146
-only-testing:AgentDeviceRunnerUITests/RunnerTests/testSynthesizedTextCommitProgressWalksExpectedPrefixOnly \

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

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -379,9 +379,13 @@ extension RunnerTests {
379379
)
380380
let tapResponse = try executeOnMainPrepared(command: tapCommand, activeApp: app)
381381
XCTAssertTrue(tapResponse.ok, String(describing: tapResponse.error))
382-
XCTAssertFalse(
382+
// A precondition, not a product claim. The fixture's empty `inputView` is what keeps the
383+
// keyboard down, but nothing in this bundle owns the simulator's own keyboard settings, so an
384+
// ambient flip that raised one here would be an environment fact — and reporting it as a
385+
// failed assertion is what made this read as a product regression on unrelated PRs (#1874).
386+
try XCTSkipIf(
383387
isKeyboardVisible(app: app),
384-
"the test must exercise a focused responder with the software keyboard hidden"
388+
"software keyboard is up: this simulator cannot exercise the hidden-keyboard responder path"
385389
)
386390

387391
let failureCountBefore = currentXCTestFailureCount()
@@ -407,6 +411,56 @@ extension RunnerTests {
407411
XCTAssertEqual(String(describing: textField.value ?? ""), "hardware-keyboard")
408412
}
409413

414+
// `waitForTextEntryReadiness`'s hardware-keyboard fallback returns early only on confirmed
415+
// focus (#1874), and `keyboardFocusConfirmed` reads that from the app-wide focus predicate this
416+
// bundle otherwise refuses to trust. Two XCTest facts it rests on, neither a repository
417+
// invariant: the predicate reports a responder that shows NO software keyboard at all, and it
418+
// names the element well enough to tell the tapped field from another one. The fixture field is
419+
// the exact shape the fallback exists for — a real responder with an empty `inputView` — so this
420+
// is where both are observable. If either regressed, readiness would silently stop taking the
421+
// fallback and spend the full readinessTimeout on every hardware-keyboard field, which no other
422+
// assertion would notice.
423+
func testHardwareKeyboardResponderConfirmsItsOwnKeyboardFocus() throws {
424+
app.launchArguments = ["--agent-device-text-entry-regression"]
425+
app.launch()
426+
defer {
427+
invalidateCachedTarget(reason: "unit_test_cleanup")
428+
app.terminate()
429+
}
430+
XCTAssertTrue(app.waitForExistence(timeout: appExistenceTimeout))
431+
432+
let textField = app.textFields["agent-device-hardware-keyboard-input"]
433+
XCTAssertTrue(textField.waitForExistence(timeout: appExistenceTimeout))
434+
let otherElement = app.staticTexts["Agent Device Runner"]
435+
XCTAssertTrue(otherElement.waitForExistence(timeout: appExistenceTimeout))
436+
XCTAssertFalse(
437+
keyboardFocusConfirmed(app: app, element: textField),
438+
"an untapped field must not confirm focus, or the fallback would fire immediately"
439+
)
440+
441+
let tapCommand = try runnerCommandFixture(
442+
#"{"command":"tap","commandId":"tap-focus-confirmation","selectorKey":"id","selectorValue":"agent-device-hardware-keyboard-input"}"#
443+
)
444+
let tapResponse = try executeOnMainPrepared(command: tapCommand, activeApp: app)
445+
XCTAssertTrue(tapResponse.ok, String(describing: tapResponse.error))
446+
try XCTSkipIf(
447+
isKeyboardVisible(app: app),
448+
"software keyboard is up: this simulator cannot exercise the hidden-keyboard responder path"
449+
)
450+
451+
let deadline = Date().addingTimeInterval(TextEntryTiming.readinessTimeout)
452+
var confirmed = keyboardFocusConfirmed(app: app, element: textField)
453+
while !confirmed && Date() < deadline {
454+
sleepFor(TextEntryTiming.pollInterval)
455+
confirmed = keyboardFocusConfirmed(app: app, element: textField)
456+
}
457+
XCTAssertTrue(confirmed, "a tapped responder must confirm its own keyboard focus")
458+
XCTAssertFalse(
459+
keyboardFocusConfirmed(app: app, element: otherElement),
460+
"focus held by another element must read as a refusal, never as this element's focus"
461+
)
462+
}
463+
410464
func testBareDelayedTypeFailsWhenTappedInputDisappearsMidCommand() throws {
411465
app.launchArguments = [
412466
"--agent-device-text-entry-regression",
Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
import XCTest
2+
3+
// The synthesized text-entry commit wait, end to end: the deadline that bounds it, the two waits
4+
// that run it, the observation/pacing they poll through, and the value-free cadence line that path
5+
// is allowed to log. Split from RunnerTests+SynthesizedTextEntry.swift, which keeps the
6+
// private-XCTest synthesis boundary and the route policies; the pure outcome functions these wrap
7+
// stay there next to the rules they encode. Everything that touches the polled field value now
8+
// lives in this one file, which is the surface apple-runner-log-redaction.test.ts guards.
9+
extension RunnerTests {
10+
/// One commit wait's running deadline.
11+
///
12+
/// A synthesized burst can be *throttled* rather than dropped: on a loaded simulator the
13+
/// characters keep landing, just slowly, and everything touching the input system slows with
14+
/// them (#1874). A flat wall-clock budget cannot tell that apart from a wedged pipeline, so it
15+
/// condemned both at the same instant — and the throttled case is a working command reported as
16+
/// `TEXT_INPUT_COMMIT_NOT_OBSERVED`, which is what turned an environment episode into a red
17+
/// lane on branches touching no iOS code.
18+
///
19+
/// So time is granted against *progress* — the observed value's expected-prefix growing, the
20+
/// same length-only evidence `logCommitCadence` already emits — with an absolute `ceiling`, so a
21+
/// pipeline delivering one character per stall window cannot hold a command open forever. A
22+
/// pipeline making no progress at all still expires at exactly the `stallBudget` the flat
23+
/// deadline used, so nothing that fails today starts passing merely by waiting longer.
24+
///
25+
/// Started by the poll loop from its own first `now()`, and held as a local `var` there. The
26+
/// wait's setup reads the field's placeholder first, an AX round-trip that takes seconds on
27+
/// exactly the loaded host this exists for; charging that to the deadline made the wait give up
28+
/// sooner than the flat budget it replaced.
29+
struct SynthesizedCommitDeadline {
30+
private let startedAt: Date
31+
private let stallBudget: TimeInterval
32+
private let ceiling: TimeInterval
33+
// Nothing has landed yet, and an observation of "still nothing" must not read as progress.
34+
private var bestPrefixLength = 0
35+
private var lastProgressAt: Date
36+
37+
init(startedAt: Date, stallBudget: TimeInterval, ceiling: TimeInterval) {
38+
self.startedAt = startedAt
39+
self.stallBudget = stallBudget
40+
self.ceiling = ceiling
41+
self.lastProgressAt = startedAt
42+
}
43+
44+
/// Records one observation's expected-prefix length. Only forward movement counts: a shorter
45+
/// read — the app clearing the field mid-flight, or a value that could not be read at all,
46+
/// which measures as 0 — is not evidence the burst is still landing, so it neither buys time
47+
/// nor takes any back.
48+
mutating func record(expectedPrefixLength: Int, at now: Date) {
49+
guard expectedPrefixLength > bestPrefixLength else { return }
50+
bestPrefixLength = expectedPrefixLength
51+
lastProgressAt = now
52+
}
53+
54+
func isExpired(at now: Date) -> Bool {
55+
now.timeIntervalSince(startedAt) >= ceiling
56+
|| now.timeIntervalSince(lastProgressAt) >= stallBudget
57+
}
58+
}
59+
60+
/// Blocks until the synthesized bare-type text is observable in the target field, so `type`
61+
/// cannot report ok while trailing characters are still uncommitted on a slow simulator.
62+
///
63+
/// Observation only. A stalled prefix cannot be told apart from a suffix still queued in the
64+
/// event stream, so re-synthesizing the difference risks committing it twice after the command
65+
/// already reported success (#1676 rejected exactly that repair). Reporting `.notObserved` is
66+
/// what the caller does instead: the partial value is the agent's to resolve, and a named
67+
/// failure beats a success that misdescribes the field. Text carrying a submit key is skipped
68+
/// outright: the app may clear or rewrite the field on submit, so `textBefore + typedText` is
69+
/// not the value to wait for.
70+
func awaitSynthesizedFirstResponderCommit(
71+
app: XCUIApplication,
72+
target: TextEntryTarget,
73+
textBefore: String?,
74+
typedText: String
75+
) -> SynthesizedTextCommitOutcome {
76+
guard let textBefore, !typedText.contains("\n"), !typedText.contains("\r") else {
77+
return .unobservable
78+
}
79+
let expectedText = textBefore + typedText
80+
let waitStartedAt = Date()
81+
NSLog("[DEBUG-1874] wait start expectedLen=%ld route=append", expectedText.count)
82+
let ingredients = synthesizedCommitPollingIngredients(app: app, target: target, expectedText: expectedText)
83+
let outcome = Self.awaitSynthesizedCommitOutcome(
84+
expectedText: expectedText,
85+
placeholder: ingredients.placeholder,
86+
now: { Date() },
87+
observe: ingredients.observe,
88+
waitForNextObservation: ingredients.waitForNextObservation
89+
)
90+
NSLog(
91+
"[DEBUG-1874] wait outcome=%@ elapsedMs=%.0f route=append",
92+
String(describing: outcome),
93+
waitStartedAt.timeIntervalSinceNow * -1000
94+
)
95+
return outcome
96+
}
97+
98+
/// Blocks until the synthesized replacement text (`fill`) is observable in the target field, so
99+
/// `fill` cannot report ok while the select-all-and-retype it posted is still uncommitted — or
100+
/// silently wrong — on a slow or channel-penalized simulator (this route runs only when the
101+
/// XCTest channel is already penalized, and never resolves an `XCUIElement`, so it previously had
102+
/// no verification at all).
103+
///
104+
/// Unlike the append route, the expected value is the final text itself — replacement mode
105+
/// clears the field first, so there is no `textBefore` prefix to account for — and unlike the
106+
/// append route, a settled mismatch is always reported rather than trusted: see
107+
/// `awaitSynthesizedReplacementCommitOutcome`'s doc comment.
108+
func awaitSynthesizedReplacementCommit(
109+
app: XCUIApplication,
110+
target: TextEntryTarget,
111+
expectedText: String
112+
) -> SynthesizedTextCommitOutcome {
113+
guard !expectedText.contains("\n"), !expectedText.contains("\r") else {
114+
return .unobservable
115+
}
116+
let waitStartedAt = Date()
117+
NSLog("[DEBUG-1874] wait start expectedLen=%ld route=replacement", expectedText.count)
118+
let ingredients = synthesizedCommitPollingIngredients(app: app, target: target, expectedText: expectedText)
119+
let outcome = Self.awaitSynthesizedReplacementCommitOutcome(
120+
expectedText: expectedText,
121+
placeholder: ingredients.placeholder,
122+
now: { Date() },
123+
observe: ingredients.observe,
124+
waitForNextObservation: ingredients.waitForNextObservation
125+
)
126+
NSLog(
127+
"[DEBUG-1874] wait outcome=%@ elapsedMs=%.0f route=replacement",
128+
String(describing: outcome),
129+
waitStartedAt.timeIntervalSinceNow * -1000
130+
)
131+
return outcome
132+
}
133+
134+
/// The emitted cadence line, as a pure function so its output is assertable. Only lengths and
135+
/// a timestamp are representable here; there is no String parameter, so observed field
136+
/// contents cannot reach runner.log through this boundary whatever they contain.
137+
static func commitCadenceLogLine(
138+
elapsedMs: Int,
139+
observedLen: Int,
140+
expectedPrefixLen: Int
141+
) -> String {
142+
"[DEBUG-1874] poll t=\(elapsedMs)ms observedLen=\(observedLen) expectedPrefixLen=\(expectedPrefixLen)"
143+
}
144+
145+
/// The typed boundary for commit-wait cadence evidence. The poll path must log through this
146+
/// function and never through a raw NSLog: every parameter is an Int, so the polled value's
147+
/// contents are unrepresentable at the call site.
148+
static func logCommitCadence(
149+
elapsedMs: Int,
150+
observedLen: Int,
151+
expectedPrefixLen: Int
152+
) {
153+
NSLog(
154+
"%@",
155+
commitCadenceLogLine(
156+
elapsedMs: elapsedMs,
157+
observedLen: observedLen,
158+
expectedPrefixLen: expectedPrefixLen
159+
)
160+
)
161+
}
162+
163+
/// The placeholder/observe/pacing ingredients shared by the append route
164+
/// (`awaitSynthesizedFirstResponderCommit`) and the replacement route
165+
/// (`awaitSynthesizedReplacementCommit`). What must NOT be shared is which outcome function
166+
/// consumes them: see `awaitSynthesizedReplacementCommitOutcome`'s doc comment for why append
167+
/// mode's "trust a diverged value" rule is wrong for replacement mode. Each caller therefore
168+
/// calls its own named outcome function directly, with real argument labels — deliberately not
169+
/// a stored closure/function-value parameter here, which would erase those labels at the call
170+
/// site and make the observe closure unrecognizable to the static content-redaction check in
171+
/// `apple-runner-log-redaction.test.ts` (`extractObserveClosure` locates the labeled closure
172+
/// literal by its text; a closure passed as a plain function value carries no such label).
173+
private func synthesizedCommitPollingIngredients(
174+
app: XCUIApplication,
175+
target: TextEntryTarget,
176+
expectedText: String
177+
) -> (
178+
placeholder: String?,
179+
observe: () -> String?,
180+
waitForNextObservation: () -> Void
181+
) {
182+
let placeholder = resolveTextEntryElement(app: app, target: target)?.placeholderValue
183+
let waitStartedAt = Date()
184+
return (
185+
placeholder: placeholder,
186+
observe: {
187+
let observedText = self.editableTextValue(
188+
for: self.resolveTextEntryElement(app: app, target: target),
189+
treatingPlaceholderAsEmpty: true
190+
)
191+
// Cadence evidence stays value-free: the polled value is user content typed through
192+
// `type`/`fill` and must never reach runner.log. Lengths and the expected-prefix walk
193+
// are enough to distinguish throttling (prefix grows slowly) from a wedge (it freezes).
194+
Self.logCommitCadence(
195+
elapsedMs: Int(waitStartedAt.timeIntervalSinceNow * -1000),
196+
observedLen: observedText?.count ?? -1,
197+
expectedPrefixLen: observedText.map { Self.commonPrefixLength($0, expectedText) } ?? -1
198+
)
199+
return observedText
200+
},
201+
// XCUI resolution shares the automation channel with the in-flight synthesized event.
202+
// Sparse reads let the target consume that event instead of continuously interrupting it.
203+
waitForNextObservation: { self.sleepFor(TextEntryTiming.synthesizedCommitPollInterval) }
204+
)
205+
}
206+
}

0 commit comments

Comments
 (0)