fix(ios-runner): slow synthesized replacement so a value-owning field loses fewer characters - #2904
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 97d03fb. The typing pace is now a fixed 12 characters per second with no limit tied to text length (RunnerSynthesizedTextEntry.m#L17). The characters are sent inside the synthesize call, so a Small note, not blocking: the same constant also paces the plain The iOS Smoke Tests jobs were still running; their fill steps reach this route, so a failure there would be relevant to this change. |
|
Addressed at Delivery ceiling. Pace ownership. The constant stays declared where the typing happens, on CHANGELOG now names both routes and the new refusal. Your smoke-lane note was right, and it caught this PR's weaker half. |
|
Following the earlier review at 97d03fb, the delta still doesn't bound the delayed route that fix was meant to close. SynthesizedDeliveryBudget.projectedSeconds charges each character max(delaySeconds, characterInterval), but a plan with delay > 0 pays a synthesize call per character at 12 cps plus a sleepFor(delaySeconds), so the real per-step cost is closer to delay + interval + round trip (https://github.com/callstack/agent-device/blob/de01c96/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift#L110). For example, The check needs to guarantee that every burst it admits finishes within synthesizedDeliveryCeiling at its real per-step cost: for a delayed plan that means counting n × (delay + characterInterval) plus a per-synthesize round-trip allowance, and using that same formula in maxTextLength and in the hint text it prints. SynthesizedDeliveryBudget is the single definition both callers read from, so fixing it there covers both without touching other sites. Could a boundary assertion at delay 0.08 s, next to the existing delay-0 one, pin this down going forward? Before this can merge, the iOS runner lane on ios.yml needs to go green on de01c96 running testSynthesizedReplacementRefusesTextBeyondTheDeliveryBudget and testSynthesizedReplacementSurvivesFieldValueWrittenBackByTheApp. Once the delay-cost fix lands, a live There are no known conflicts with main. All five checks were still queued or in progress with no logs yet, so none of them speak to this fix yet; the iOS Smoke Tests exercise the paced synthesized route this PR touches and would be the relevant signal, while Repo Guards and Coverage run the TS/repo-shape gates the delta doesn't touch, so a failure there would most likely be unrelated. The 58 s figure above is inferred from reading the code path, not timed, and the delay-0.08 device runs from the earlier review weren't reproduced here, so is that figure worth confirming with an actual timed run before relying on it? A few smaller things worth a look separately: the CI lane's 5 ms window is also cleared by the pre-fix 60 cps pace, so it only proves the fixture runs, not that the fix holds; testSynthesizedPaceLeavesRoomForAnAppToAcknowledgeEachEdit looks like the only guard left that would go red without the fix; TextTyping.swift:170's type-route budget fallback still has no test running through the production route; and TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED is missing from website/docs/docs/commands.md next to the other TEXT_INPUT_* codes. |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Reviewed at de01c96. The pace/budget split looks right. I checked the delaySeconds: 0 guard in typeIntoCurrentTarget and concluded it is intentional: delayed routes enter that helper one character at a time and the inter-character sleep happens outside the helper. The user-facing hint is the remaining place where the default-pace ceiling can be mistaken for the actual request budget.
| case .commitNotObserved: | ||
| return "The field may hold none, part, or all of the text. Run snapshot -i and inspect the field: if it already matches, continue; otherwise retry fill with the full text quoted and --delay-ms 80. Do not use type, which appends to whatever committed." | ||
| case .synthesisBudgetExceeded: | ||
| return "Fill about \(SynthesizedDeliveryBudget.maxTextLength(delaySeconds: 0)) characters at a time and append the rest with separate type commands, keeping each command inside that budget. A lower --delay-ms does not help: this route is chosen when the accessibility channel is already degraded, and every character interval counts against the same budget." |
There was a problem hiding this comment.
The hint hardcodes maxTextLength(delaySeconds: 0), but a synthesis-budget refusal can be produced by a request with a nondefault delay. Above the synthesized character interval, a higher delay lowers the per-command ceiling, so this hint can over-promise and lead the operator to retry a fill that is still too large. Carry the actual maxTextLength(delaySeconds:) for the failing request into the response, or phrase the number as the default-pace ceiling and explain that higher delays lower it.
|
The iOS lane went red twice more on my fixture, and the second failure changed its design — worth recording here because it is a fact about the pace this PR adds.
Also from your smoke note: the lane's failure was the targeted XCTest list, not the smoke lane's
|
|
This is a follow-up on the earlier review at de01c96. The budget undercount is still open, the rewritten lane test no longer reproduces #2080, and no live run backs the new pace or budget at b3a304d. The delay/length budget still charges max(delay, 1/12 s) per character while the real cost of a delayed plan is one synthesize call plus sleepFor(delay) per character (apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift#L109). The lane test at apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+SynthesizedTextEntryTests.swift#L78 now accepts either an ok with the exact value or a TEXT_INPUT_COMMIT_NOT_OBSERVED refusal after a write-back. Main at f47e9c9 already produces that refusal through awaitSynthesizedReplacementCommit, and #2080 recorded exactly that refusal, so at the pre-fix 60 cps pace this test passes through the refusal branch and no longer catches a reverted fix. testSynthesizedPaceLeavesRoomForAnAppToAcknowledgeEachEdit only compares a constant, and the author reports XCTest doesn't honor that constant's spacing. Can this get a deterministic, non-racy assertion that the 12 cps route keeps a field whose render window is well inside the pace, shown red at 60 cps? If XCTest spacing can't be bounded, the PR's claim should narrow to "reduces" rather than "keeps," said in the changelog, with the guarantee moved to #2906, and the doc comment at line 18 corrected either way. This changes a device-facing iOS fill route: the pace, the delivery-budget refusal, and the TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED code (apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift#L126). No live run on this head is attached, including the Not blocking: the NSLog marker at RunnerTests+SynthesizedTextEntryTests.swift#L93 has no consumer at head and the sawKeepUp || sawRewrite assertion is always true since each iteration sets one of the two flags, so either delete both or make the branch outcome part of a real assertion, and the still-unaddressed ask to have the TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED hint use the request's delay folds into the budget formula fix above. I read the code rather than timing it, so the ~0.16 s per character behind this comes from the formula, not a measurement; I didn't see the CI log for the 4 ms inter-character observation or the 15:21 UTC lane failure; and I didn't run the lane tests myself, so the refusal-branch conclusion rests on main's existing commit wait plus the author's reported result after 20 write-backs at 40 ms. Smoke Tests is queued at b3a304d, and its iOS fill steps reach the synthesized-first-responder-replacement route this PR changes; the ios.yml runner lane now also runs both new lane tests, and a failure in either matters here. The budget formula needs to charge the real per-step cost so an admitted delayed fill finishes inside 45 s, and then #2080 needs a regression that actually fails at the pre-fix pace. |
…ning field keeps it A replacement burst typed at XCTest's default 60 characters/second outruns an app that owns its field's value and re-applies it after the edit that produced it, which is how a controlled React Native `TextInput` behaves. That write erases whatever the burst typed while it was in flight and the field then sits stable short of the request: first characters and tail survive, a middle run is gone, which is the `fill id="field-email" ada@example` -> `aexample` CI signature. The commit wait can refuse such a value but cannot repair it, because a retype races the same write. 12 characters/second spaces characters ~83 ms apart. The new `--agent-device-text-entry-async-value-write` fixture owns its field's value and the iOS lane test pins the pace against it: raising the pace re-opens the race and turns that test red. A write-back still in flight 150 ms after an edit corrupts a burst at this pace too, so this narrows the window rather than closing it, and the measured wall-clock cost of the pace is recorded next to the commit ceiling it sits upstream of. Closes #2080
…r, not a timer Review asked for a delivery ceiling tied to text length, and the iOS lane showed the lane test itself was the weaker half of this change: a fixture that schedules a write lands differently depending on how loaded the host is, and it failed on CI holding `ad@example` at the shipped pace. `TextEntryTiming.synthesizedDeliveryCeiling` bounds how long a burst may spend posting. The private synthesize call delivers as it returns, so text beyond the ceiling would still have been arriving when the transport gave up on the command, leaving the runner typing into a field nobody waits for and the next command finding it busy. `fill` now answers `TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED` before posting the first character, and the element-less `type` fallback keeps its long text on the verified application-wide route it already had. The pace stays owned by `RunnerSynthesizedTextEntry` and is read from Swift, so the delivery budget and the acknowledge-window policy derive from one declaration. `testSynthesizedPaceLeavesRoomForAnAppToAcknowledgeEachEdit` pins the pace at two windows per character or slower, and the budget test pins the refusal boundary; both are pure and run on the host lane. The fixture now decides at the edit whether a character overtook a render instead of scheduling one against a timer. A loaded host stretches the gaps between characters, which can only make that app keep up better, so the safe half no longer depends on how busy the machine is. The lane run watches a 5ms window, an app no pace this runner could ship outruns; the red half was verified at the 40ms policy window, where the pre-fix pace leaves the field holding `a` against 20 write-backs.
The lane test failed on CI twice: first holding `ad@example`, then `ada@exampl` with a single write-back. Both were honest `TEXT_INPUT_COMMIT_NOT_OBSERVED` refusals over a field the app had rewritten, and both were the test assuming it knew whether the app had won a round trip. XCTest does not deliver `typingSpeed:` characters evenly — CI saw two characters of a paced burst 4 ms apart — so no acknowledge window a fixture watches is a promise about the host. The test now follows the fixture's own counter: an app that kept up has to show the exact value and an ok, and an app that overtook the burst has to show the refusal. Verified on both branches, the strict one at the 5 ms lane window and the refusal one at 150 ms. That is the invariant the runner owns, and the one #2080 was about: never an ok over a field the app rewrote.
…all and delay A delayed plan posts each character in its own synthesize call and sleeps between calls, but the delivery budget charged it max(delay, interval) per character. At --delay-ms 80 it admitted 360 characters; measured on a simulator, one call costs about 222 ms, so a 155-character fill already took 48 s, past the 45 s command budget. The budget now charges every call the pace for its characters plus a measured 150 ms overhead, and the delay between calls. maxTextLength is derived from the same projection, and the refusal hint names both the undelayed budget and the one at --delay-ms 80.
… other text-entry codes
b3a304d to
a27335e
Compare
|
Addressed at Delayed plan cost (14:54 note): fixed. You were right, and the real cost was higher than 58 s. On an iPhone 17 Pro simulator, one one-character synthesize call took 222 ms on average (212–617 ms over 235 calls), not 83 ms. Under the old rule, Timed boundary runs (your live-run ask): on my own simulator, through Hint used the delay-0 ceiling (15:22 inline): fixed. The hint now names both numbers: the undelayed budget and the budget at
Rebase: main removed Validation: |
|
|
This is a follow-up on b3a304d (#2904 (comment)) at a27335e. The second blocking finding from that review is still open, and the reply doesn't address it: c62366e is byte-identical to b3a304d. RunnerTests+SynthesizedTextEntryTests.swift:78 (https://github.com/callstack/agent-device/blob/a27335e/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+SynthesizedTextEntryTests.swift#L78) still accepts either ok with the exact value or TEXT_INPUT_COMMIT_NOT_OBSERVED after a write-back. At the pre-fix 60 cps pace, main already returns that refusal through awaitSynthesizedReplacementCommit, and #2080 recorded the same refusal, so a reverted pace still goes through the rewrite branch and the test stays green; the 5 ms acknowledge window makes the fixture's race unlikely at either pace. The author's timed boundary runs cover the budget question — 358 chars at 32.6 s and 96 chars at --delay-ms 80 at 31.2 s, both ok — but two things are still missing before this counts as validated. First, that run was in-process and uncommitted: could you attach its output showing textEntryRoute=synthesized-first-responder-replacement for both boundary fills plus the follow-up command's ok response, or rerun it through the daemon CLI ( Both Smoke Tests jobs are queued at a27335e. The iOS smoke fill steps and the ios.yml runner lane reach the synthesized-first-responder-replacement route this PR changes — the pace, the delivery budget, and the refusal — so a failure there would be attributable to this PR until shown otherwise. The earlier ETIMEDOUT failure was in the toolchain probe after TEST BUILD SUCCEEDED, and that doesn't overlap this diff. A couple of limits on what backs this: the claim that main already refuses at 60 cps rests on the previous review's reading of awaitSynthesizedReplacementCommit and on the #2080 report, not a fresh run; the 222 ms per-call figure and the 32.6 s / 31.2 s timings are author-reported with no attached logs, and I can't tell how the 32.6 s splits between delivery and the 10 s commit wait — the worst case of roughly 30 s delivery plus 10 s commit plus focus looks like it fits inside 45 s, but that's not measured. I also didn't verify that check:xctest-selection picks up both lane tests at this head, and I reviewed the logical delta from the range-diff and the two new commits rather than the full diff, since it's dominated by upstream rebase commits. Before this can merge: add a #2080 regression test that goes red at 60 cps (strict ok plus the exact value, no refusal branch) or narrow the "keeps" claim to "reduces," then attach route-confirmed live output for the boundary fills and the ios.yml lane results at a27335e. |
…og and pin the pace the app sees A 358-character fill through the daemon was abandoned by the runner's 30 s main-thread watchdog with the burst still typing. The delivery ceiling was a free-standing 30 s sized against the 45 s transport budget, while the whole command runs under mainThreadExecutionTimeout. The ceiling is now derived from that watchdog minus a focus allowance and the commit wait's ceiling (18 s), so the admitted text fits with the longest commit wait: 214 characters undelayed, 57 at --delay-ms 80. The app-owned-value lane test accepted either outcome and stayed green at 60 characters per second. It now asserts the spacing the fixture app receives: a burst's characters average at least one acknowledge window apart. That is red at 60 (11 edits in about 110 ms) and green at 12 (about 780 ms). A strict "the app keeps up" assertion is not possible: at a 40 ms window, 12 characters per second still lost one character in 13 of 20 bursts, because XCTest spaces the characters unevenly. The comments that claimed survival are narrowed to match. The call overhead and the --delay-ms 80 recovery delay move into TextEntryTiming beside the other text-entry timings, and both hints read the recovery delay from there.
thymikee
changed the title
|
Addressed at 1. No test went red at 60 cps: fixed, and the claim is narrowed. I measured before choosing. 20 bursts of
The closest pair of characters at 12 cps was 11–44 ms, although the burst averages ~78 ms per gap. So a strict "ok + exact value at 40 ms" test would fail most runs at the shipped pace. That is option (b): the title and body now say the pace reduces the loss, and #2906 owns the guarantee (body says What goes red instead:
The write-back branch stays, but only as the runner's half (a rewritten field never reports ok). 2. Daemon CLI evidence: this found a real bug. My earlier 358-character "32.6 s ok" was in-process, and that harness never runs under the runner's 30 s main-thread watchdog. Through the CLI, It is now derived:
3. Lane results. 4. Reuse pass.
|
|
The delivery-budget fix looks correct at 80bf831, and the issues from the earlier pass are resolved. Not blocking: a policy or lane test that sends append-mode text past The CLI-reported timing runs and the 60 cps run are author-reported without attached raw output, and those runs used an uncommitted build that forced |
|
CI at
The non-blocking note, a lane test for the keyboard-visible append fallback at |
The element-less, keyboard-visible branch tested the budget against the chunk it was about to post. An append peels its first character for warmup and a `--delay-ms` plan posts one character per call, so no chunk ever looked long and the paced synthesizer kept a text the command cannot carry: 215 characters arrived as 1 + 214, both inside the budget. The branch is now charged the command's own length and delay. That branch had no test, so it gained one: 215 characters through an unresolved target type application-wide, and the value arrives unverified because the branch has no element to read back. The fixture field keeps a real input view under `--agent-device-text-entry-soft-keyboard`, without which no branch requiring a visible keyboard is reachable.
|
Took the non-blocking note at
Local at |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
This is ready for human review at a58713b; the replacement path now trims the smaller share of characters instead of the larger one, and nothing is left open from the earlier evidence-pending review. Not blocking: projectedSeconds in apple/runner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift charges one synthesize call for an append-mode type, but the route actually makes two calls (a warmup character, then the remainder) with a wait between them, so the projection undercounts by one call overhead plus the warmup wait — worth lining up if you want the estimate to match, but it can wait. No conflicts. All 21 checks pass at a58713b, including the ios.yml targeted-runner lane, which is the job that exercises the keyboard-visible, no-element branch this delta touches. A couple of things worth flagging that I couldn't confirm myself: the new lane test starts with XCTSkipIf when the software keyboard isn't visible, so a CI simulator with a hardware keyboard attached would show a skip, not a pass — did the per-test log confirm it ran rather than skipped? I also didn't rerun check:xctest-selection, so the test's inclusion in the iOS PR lane is as reported. The 12 cps synthesized-typing pace, and this delta's use of it, were already covered in the earlier review and are unchanged here. Can you confirm from the ios.yml per-test log that testOverBudgetTypeWithoutResolvableElementTypesApplicationWide actually ran and passed, rather than being skipped? |
thymikee
added
the
ready-for-human
The review found `projectedSeconds` charging one synthesize call for an append-mode `type` while the route posts a warmup character and then the rest, so the estimate undercounted one call and the wait between them. The over-budget branch that asks is the one with no element, so its warmup wait is one poll: `waitForWarmupValue` has no expected value to wait for. That branch now charges 2 calls and the poll, which is one character less than a replacement of the same length. Also: the budget hint and the docs named one limit and said "the number" for a delayed retry, which reads as the larger one; both now name the limit for the delay in play and say the delay falls between characters. The pace's header comment claimed the acknowledge window is derived from it, which it is not. The synthesized-replacement helper builds its command with JSONSerialization, and the over-budget lane test polls the field under its own deadline instead of reading `value` once on a route that verifies nothing.
Confirmed from the iOS job log at It ran rather than skipped, so the CI simulator brings the software keyboard up for a field with a real input view. I will post the same three lines for the head this comment names once its iOS lane finishes.
Rebase not done: main's 22 commits touch none of these files (its Local at |
There was a problem hiding this comment.
3 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift:182">
P3: This log reports `value.count`, but the budget decision above it was made on `text.count` (the full command text). In the warmup split and the `--delay-ms` per-character path, `typeIntoCurrentTarget` is called with a single character, so this prints `chars=1` (or `chars=N-1`) for a command that actually overflowed the budget on the full text, which is misleading when triaging `reason=delivery-budget` logs. Log `text.count` instead to match the decision.</violation>
</file>
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextEntry.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextEntry.swift:71">
P1: The delivery budget omits the Command-A synthesis call that every replacement performs. A boundary-sized fill therefore passes the refusal check with only about 17 ms of slack before delivery, then can exceed the 30-second watchdog and leave partial input; charge that selection call or reserve its overhead before calculating the maximum length.</violation>
</file>
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift:128">
P2: The warmup wait is charged as one pollInterval (0.02s), but for `repairMode == .replacement` the element-less type path actually spends the full `warmupValueTimeout` (0.4s). In that mode `warmupExpectedText` is the first character (non-nil), so `waitForWarmupValue` skips its one-poll guard and loops until the deadline because `editableTextValue(for: nil)` never equals the expected string. The budget check in `typeIntoCurrentTarget` passes `typeWarmup: repairMode != .none`, so this undercounts a replacement-mode warmup by ~0.38s and lets text slightly past the 18s delivery ceiling through the synthesized route instead of the app-wide fallback — the watchdog-abandonment case this change exists to prevent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| /// main-thread watchdog leaves after focus and the longest commit wait. The private synthesize | ||
| /// call delivers as it returns, so text that does not fit is refused before the first character | ||
| /// is posted; otherwise the watchdog abandons the command with the runner still typing. | ||
| static let synthesizedDeliveryCeiling: TimeInterval = RunnerTests.mainThreadExecutionTimeout |
There was a problem hiding this comment.
P1: The delivery budget omits the Command-A synthesis call that every replacement performs. A boundary-sized fill therefore passes the refusal check with only about 17 ms of slack before delivery, then can exceed the 30-second watchdog and leave partial input; charge that selection call or reserve its overhead before calculating the maximum length.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextEntry.swift, line 71:
<comment>The delivery budget omits the Command-A synthesis call that every replacement performs. A boundary-sized fill therefore passes the refusal check with only about 17 ms of slack before delivery, then can exceed the 30-second watchdog and leave partial input; charge that selection call or reserve its overhead before calculating the maximum length.</comment>
<file context>
@@ -49,10 +58,31 @@ extension RunnerTests {
+ /// main-thread watchdog leaves after focus and the longest commit wait. The private synthesize
+ /// call delivers as it returns, so text that does not fit is refused before the first character
+ /// is posted; otherwise the watchdog abandons the command with the runner still typing.
+ static let synthesizedDeliveryCeiling: TimeInterval = RunnerTests.mainThreadExecutionTimeout
+ - synthesizedReplacementFocusAllowance
+ - synthesizedCommitCeiling
</file context>
| return Double(textLength) * characterInterval | ||
| + Double(calls) * TextEntryTiming.synthesizeCallOverhead | ||
| + Double(calls - 1) * delaySeconds | ||
| + (warmupSplit ? TextEntryTiming.pollInterval : 0) |
There was a problem hiding this comment.
P2: The warmup wait is charged as one pollInterval (0.02s), but for repairMode == .replacement the element-less type path actually spends the full warmupValueTimeout (0.4s). In that mode warmupExpectedText is the first character (non-nil), so waitForWarmupValue skips its one-poll guard and loops until the deadline because editableTextValue(for: nil) never equals the expected string. The budget check in typeIntoCurrentTarget passes typeWarmup: repairMode != .none, so this undercounts a replacement-mode warmup by ~0.38s and lets text slightly past the 18s delivery ceiling through the synthesized route instead of the app-wide fallback — the watchdog-abandonment case this change exists to prevent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift, line 128:
<comment>The warmup wait is charged as one pollInterval (0.02s), but for `repairMode == .replacement` the element-less type path actually spends the full `warmupValueTimeout` (0.4s). In that mode `warmupExpectedText` is the first character (non-nil), so `waitForWarmupValue` skips its one-poll guard and loops until the deadline because `editableTextValue(for: nil)` never equals the expected string. The budget check in `typeIntoCurrentTarget` passes `typeWarmup: repairMode != .none`, so this undercounts a replacement-mode warmup by ~0.38s and lets text slightly past the 18s delivery ceiling through the synthesized route instead of the app-wide fallback — the watchdog-abandonment case this change exists to prevent.</comment>
<file context>
@@ -82,23 +82,97 @@ extension RunnerTests {
+ return Double(textLength) * characterInterval
+ + Double(calls) * TextEntryTiming.synthesizeCallOverhead
+ + Double(calls - 1) * delaySeconds
+ + (warmupSplit ? TextEntryTiming.pollInterval : 0)
+ }
+
</file context>
| NSLog( | ||
| "AGENT_DEVICE_RUNNER_TEXT_ENTRY_ROUTE route=xctest-application-fallback " | ||
| + "reason=delivery-budget chars=%d", | ||
| value.count |
There was a problem hiding this comment.
P3: This log reports value.count, but the budget decision above it was made on text.count (the full command text). In the warmup split and the --delay-ms per-character path, typeIntoCurrentTarget is called with a single character, so this prints chars=1 (or chars=N-1) for a command that actually overflowed the budget on the full text, which is misleading when triaging reason=delivery-budget logs. Log text.count instead to match the decision.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift, line 182:
<comment>This log reports `value.count`, but the budget decision above it was made on `text.count` (the full command text). In the warmup split and the `--delay-ms` per-character path, `typeIntoCurrentTarget` is called with a single character, so this prints `chars=1` (or `chars=N-1`) for a command that actually overflowed the budget on the full text, which is misleading when triaging `reason=delivery-budget` logs. Log `text.count` instead to match the decision.</comment>
<file context>
@@ -162,6 +162,28 @@ extension RunnerTests {
+ NSLog(
+ "AGENT_DEVICE_RUNNER_TEXT_ENTRY_ROUTE route=xctest-application-fallback "
+ + "reason=delivery-budget chars=%d",
+ value.count
+ )
+ app.typeText(value)
</file context>
|
CI green at The new |
|
This is ready for human review at 7baa118. All checks pass at this commit, and the iOS targeted lane runs the changed no-element keyboard-visible branch (https://github.com/callstack/agent-device/blob/7baa118/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SynthesizedTextEntry.swift#L173), which the author's quoted output from run 36101110733 shows passing. I did not open the per-test iOS logs for runs 36045558970 and 36101110733, so the ran-not-skipped confirmation rests on the lines the author quotes, and I did not re-run check:xctest-selection to confirm the new policy test sits on the macOS host lane. Not blocking: the replacement-mode lane test could restore the |
1 participant
Summary
fill's synthesized replacement typed at XCTest's default 60 characters/second. An app that owns its field's value renders it after the edit that produced it — a controlled React NativeTextInput— and that render erases whatever the burst typed while it was in flight, then reads its own erasure back into its model. The field sits stable short of the request: theada@example->aexamplesignature in #2080.Both routes that type without resolving an
XCUIElementare now paced at 12 characters/second, declared once onRunnerSynthesizedTextEntry. This reduces the loss; it does not prevent it. Against a fixture app that acknowledges each edit within 40 ms (20 bursts ofada@exampleeach, iPhone 17 Pro simulator, iOS 26.2):XCTest does not space
typingSpeed:characters evenly: at 12 cps a burst averages ~78 ms per gap, but its closest pair was 11–44 ms. So no fixed pace can promise that an app with a 40 ms window keeps up. Every short value was refused withTEXT_INPUT_COMMIT_NOT_OBSERVED(the commit wait main already has). Preventing the loss is #2906 (back-pressure from the field).Because the pace makes length the burst's wall clock, text that cannot be posted inside one command is refused before the first character:
fillanswersTEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED, and the element-lesstypefallback keeps its application-wide route, which cannot read the field back and so reports the value unverified. The budget is derived from the runner's 30 s main-thread watchdog (mainThreadExecutionTimeout, which bounds the whole command), minus a 2 s focus allowance and the 10 s commit-wait ceiling: 18 s of delivery. Each synthesize call is charged its characters at the pace plus a measured 150 ms call overhead, and a--delay-msplan pays one call and the delay per character. That admits 214 characters without--delay-msand 57 at--delay-ms 80. The refusal hint names both.Follow-ups: #2903 (verified element route), #2906 (back-pressure; owns the "field keeps its value" guarantee).
User-facing note (release notes;
CHANGELOG.mdis gone on main): on iOS, synthesized text entry types about 83 ms apart instead of 13 ms, so a field whose app writes its own value back loses far fewer characters. The command still refuses a field left short. Afilltoo long for one runner command (about 214 characters, fewer with--delay-ms) now fails withTEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDEDbefore typing anything.Refs #2080
Validation
At
80bf831fe4(pnpm check:affected --runexit 0):testSynthesizedReplacementPacesAnAppOwnedFieldAtItsAcknowledgeWindowasserts that a burst's characters reach the app at least one acknowledge window (40 ms) apart on average. WithRunnerTextEntryTypingSpeedCharactersPerSecondflipped to 60 it failed on both fills: "11 edits reached the app in 111 ms (closest pair 2 ms)" and 125 ms, and the host policy test failed too. Flipped back to 12: green (bursts 753–801 ms).AgentDeviceTesterForm tab. The synthesized route only runs while the XCTest channel is penalized, so a temporary, uncommitted build forcedisSnapshotXCTestChannelPenalizedto true.runner.logshowsroute=synthesized-first-responder-replacementfor every fill below:fill @e10 <358 chars>failed at 30.2 s withCOMMAND_FAILED"main thread execution timed out". Delivery alone took 30.3 s, so the watchdog abandoned the command mid-burst. The in-process harness behind the earlier 358-character "32.6 s ok" never ran under that watchdog.fill @e10 <215 chars>:TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDEDin 0.9 s,budgetChars=214.fill @e10 <214 chars>: ok, 20.1 s wall. Focus 491 ms, delivery 18.3 s, commit wait 1.1 s. Thensnapshot -i: ok, 0.24 s.fill @e10 <57 chars> --delay-ms 80: ok, 19.2 s wall. Focus 503 ms, 57 synthesize calls of 213–556 ms each, commit wait 1.1 s. Thensnapshot -i: ok, 0.54 s.check:xctest-selection --ios-pr-tests, 66 tests, including both lane tests) plus 7 touched host tests: 73 tests, 0 failures on my simulator.At
a58713b50(check:xctest-selection: 323 declared methods, iOS PR lane 67, 0 unreachable):TextTyping.swift) had no coverage, and charging the budget per dispatch left it unreachable in practice: an append peels its first character for warmup, so 215 characters arrived as 1 + 214 and both pieces fit.testOverBudgetTypeWithoutResolvableElementTypesApplicationWidefailed onroute=synthesized-first-responderbefore the charge moved to the command's length and delay; it passes after, assertingroute=xctest-application-fallback, no failure,verified == nil(the branch has no element to read), and all 215 characters in the field.--agent-device-text-entry-soft-keyboardgives the fixture field a real input view; without it no keyboard-visible branch is reachable.typeroutes): 0 failures, 71.6 s; the new test is 14.8 s.format:check,lint,check:xctest-selection,check:gate-manifest,check:packaged-runner-swiftgreen.Risk: the 2 s focus allowance and the 150 ms call overhead come from one host. A slower host has less watchdog left for the commit wait. The pace's cost on long text is bounded only by the refusal (#2906).