Conversation
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Identity collisions and unchecked target re-resolution can still redirect typing or repair into another input.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Updates the Apple runner so filling an auto-submitting input can succeed without invalidating the session when the input disappears after delivery.
Changes:
- Uses snapshots for frame reads and captures input identity.
- Skips verification and repair when the delivery input is detected as removed.
- Adds auto-submit fixtures and regression tests.
| File | Description |
|---|---|
| apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TextTypingTests.swift | Tests completed delivery and early removal. |
| apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TextEntryPolicyTests.swift | Tests identity-based removal detection. |
| apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift | Adds the post-delivery identity check. |
| apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextEntry.swift | Captures snapshot-based input identity and frame. |
| apple/runner/AgentDeviceRunner/AgentDeviceRunner/AgentDeviceRunnerApp.m | Adds an auto-submit replacement-input fixture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I reviewed fe0461a. The fix is not ready to merge yet: the identity check does not cover every place the runner re-resolves the input, and the changed device path has no live run on this head. The only live CLI run was on the pre-rebase head. The rebase re-applied deliveredTo and the post-plan withElement onto main's synthesized type plan, which is a different route (https://github.com/callstack/agent-device/blob/fe0461a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift#L400). The reported run does not show that it reached the new branch, because runner.log has no AGENT_DEVICE_RUNNER_TEXT_ENTRY_INPUT_REMOVED_AFTER_DELIVERY. The fixture XCTests call executeTypeCommand directly and skip the daemon fill route, so nothing shows the changed path working on the head being merged. After the thread fixes, please run a live Could this be simpler? The Not blocking, and fine to take or leave: the removed-after-delivery outcome returns plain ok "typed" with no response-level marker, so callers cannot tell it from a verified fill (an unverified marker or warning through the shared response builder could help); the reported state (input removed after the last character, no successor) is covered only by the predicate table, so a fixture variant that removes the field at length 6 without a successor would close that gap; and when the XCTest channel is penalized, fill takes runSynthesizedReplacementRoute, which never reaches the new guard, so either narrow the PR body claim or cover that route too. The open review threads on this PR still apply: #3161 (comment) (replacement after warmup), #3161 (comment) (unnamed field identity), #3161 (comment) (transient error as removed), and #3161 (comment) (verify and repair identity). The three Copilot threads (r4173162982, r4173163019, r4173163055) repeat those same mechanisms, and they should resolve once the fix lands. CI is still pending: Smoke Tests, Repo Guards and Coverage were queued or running with no failure logs. The iOS Smoke jobs run the runner text-entry code this diff changes, so a later failure there cannot be cleared as unrelated. There are no conflicts. Before merge, please make the identity check hold at every resolve inside resolveTextEntryElement, count an input as removed only on a proven no-match, and then run the live CLI fill on the final head as described. I ran no XCTests and had no range-diff against the pre-rebase patch. I also did not confirm which XCTest error domain or code snapshot() throws for no-match versus a transient AX failure, or trace what the penalized-channel route does when the field vanishes. |
fe0461a to
e030d42
Compare
|
[claude-opus-5-5] responding on behalf of Oskar Reworked as suggested and rebased on main. Head: e030d42. Changes
Can the frame change alone fix it? It fixes the reported failure, but not the successor case. Fixture XCTests, iOS 27.0:
Validation on e030d42
CI on the old head
Left open
CI on e030d42: all 12 checks green. The iOS Smoke targeted lane ran 83 runner XCTests, all 5 new fill tests among them, with 0 failures. The macOS host lane ran 281 with 0 failures. |
|
The earlier findings from fe0461a are fixed at e030d42, and I found no new blocking problems in the code. Posts now resolve through the bound resolver, repair is refused for an empty identifier, and a failed probe no longer reads as a removed input. The iOS Smoke Tests are still queued with no logs. They run the runner text-entry code this PR changes (RunnerTests+TextEntry and TextTyping), so they must pass before merge. I know of no conflicts. One open point remains: isBoundInput compares type and identifier. If an unnamed code field is replaced by an unnamed, focused successor mid-delivery, the remaining posts type into the successor and verify reports TEXT_ENTRY_MISMATCH. No test covers this case. Please state this limit next to the same-identifier limit in the PR body. You could also refuse focus and point re-resolution after the first post when the bound identity has no identifier. On the open threads, the unnamed successor in posts and the same point from the other reviewer still apply, and both match the note above. These threads are fixed at e030d42, so you can resolve them: bound resolution at every post, probe error mapping, deliveredTo comparison removed, posts and warmup read-back, and verify polls and repair. The live CLI run and the 50 passing runner XCTests are author-reported, and I ran no XCTests. I did not confirm that snapshot() throws code 10008 for a removed input, and I did not measure the added per-post cost on paced per-character plans. The penalized replacement route stays outside the guard, as you note. |
|
[claude-opus-5-5] responding on behalf of Oskar Follow-up on the e030d42 review. Head: 3dbb8d9, a test-only commit. Unnamed successor mid-delivery: documented as a limit, not fixed. I tried the suggested refusal (after the first post, refuse point and focus re-resolution when the identifier is empty) on an iOS 27.0 sim. It never fires. A coordinate fill binds an index-bound element ( The outcome is the same with and without the refusal, so I reverted it. The refusal would only add a new NOT_FOCUSED failure when an ordinary unnamed field's index shifts. Closing the gap needs a per-instance element identity, which public XCTest does not expose. The new Validation on 3dbb8d9
Threads: replied on r4173162842 and r4173162982 with the above. All 7 threads are resolved. |
|
[claude-opus-5-5] responding on behalf of Oskar I checked the 5/31 paced unnamed-field failures from my last comment. It is not a regression in this diff: upstream main fails the same way, and the failing command never reaches code this PR changes. Setup: one fresh iOS 27.0 sim. The upstream main runner and the 3dbb8d9 runner alternated in batches of 10 (main, PR, main, PR, ...), 40 fills per side. Each iteration ran the same sequence: Results, paced fill ok:
Fisher's exact test p=0.20. All 6 failures fell in the first two batches after the sim booted; the later 60 iterations had none. Where it fails: every failure on both sides is The daemon's snapshot gave the field's position with the keyboard down (y=470). By the time of the paced fill, the keyboard was up and the layout had moved the field to y=306-340, so nothing was at the requested point. Focus-fallback hypothesis: ruled out. The bound-target change only affects posts inside My earlier "main 20/20" was a smaller sample; with 40 per side, main also hits it (1/40). |
|
The earlier findings from e030d42 are fixed at 3dbb8d9: posts, warmup read-back, and verify and repair now all resolve through the bound text-entry lookup, and the probe maps failures to unavailable instead of "gone". I found no remaining code problems. Not blocking, and you can take or leave it: the new test in https://github.com/callstack/agent-device/blob/3dbb8d9/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TextTypingTests.swift#L281 asserts that the successor holds "23456", which pins the known wrong-target side effect as expected output, so you could drop that assertion or say in the test name that it pins the current limit. I did not run the XCTests or the live CLI runs, so the passing runner tests and the auto-submit and paced numbers on the PR are your reports, and I have not confirmed that snapshot() on a removed element returns the no-match error code the probe maps. No conflicts are known. The Smoke Tests job is still in progress, and it covers the runner text-entry route this PR changes, so it needs to finish green before merge. On the other threads, the P1 on indistinguishable inputs (#3161 (comment)) and the Copilot thread on the same limit (#3161 (comment)) are resolved by the PR body note and the new test, and the P1 on bound resolution (#3161 (comment)), the P2 on the unavailable probe (#3161 (comment)), the P3 on the deliveredTo comparison (#3161 (comment)), and the two Copilot threads on bound resolution (#3161 (comment) and #3161 (comment)) are fixed at this commit, so none of the threads still apply and you can resolve them all. |
Reading frame of an input the app removed after delivery recorded an XCTest failure, which the runner converts to XCTEST_RECORDED_FAILURE and the daemon treats as session-fatal. Bind targets through a throwing snapshot and skip verify/repair once the delivered input is gone, so it never verifies or repairs into the next screen's input.
Bind the input's type and identifier at the first resolve and check it in resolveTextEntryElement, which every post, warmup read-back, verify poll and repair call. Count the input as removed only on XCTest's no-match (10008). Refuse repair when the bound input has no identifier.
…amed-successor limit
3dbb8d9 to
0eef3bb
Compare
|
Thanks for the update. The code looks good at 0eef3bb, but the PR is missing live proof on the rebased route. The earlier findings from 3dbb8d9 are fixed. The text-entry element is now resolved only through the bound identity on every post, read-back, verify and repair step. A removed input now maps to "no match" only for ui-testing error 10008, and any other error stays "unavailable". The app-wide focused-typing fallback and the repair path both refuse an unidentified identity. The two cubic threads on the indistinguishable-input limit and the Copilot thread on the same limit are resolved as benign: public XCTest cannot close that limit, a test pins it, and the PR body documents it. These threads are fixed at this head and can be resolved: #3161 (comment), #3161 (comment), #3161 (comment), #3161 (comment), #3161 (comment). These three are benign by design and can be resolved too: #3161 (comment) and #3161 (comment). The live CLI runs (auto-submit Smoke Tests is still running, and its iOS lane covers this exact route (executeTypeCommand, typeTextReliably, verifyTextEntryWithRepairIfNeeded), so a failure there would count against this PR. I know of no conflicts. Before merge, Smoke Tests must finish green on 0eef3bb with the iOS runner text-entry XCTests passing on the rebased route. |
|
[claude-opus-5-5] responding on behalf of Oskar Rebased onto upstream/main 294dc7d (after #3171). Head: 0eef3bb. Conflicts. The only textual conflict was in the fixture app. The fixture now keeps #3171's
#3171 and the bound identity. I kept both identities and did not merge them.
Can the unconfirmed path report success for a replaced input?
Maintainer nit. Taken. The test is renamed to Validation on 0eef3bb
|
|
Thanks, this closes the evidence gap at 0eef3bb. The live CLI runs on the rebased head cover both paths: |
When the XCTest channel is penalized, iOS `fill` taps the daemon's point and types through private synthesis. That route named the field only by the point, but focusing can move the layout (keyboard avoidance, a bottom sheet extending above the keyboard). The commit wait then re-read whatever sat at the point: a fill that landed failed with TEXT_INPUT_COMMIT_NOT_OBSERVED, and `fill ""` cleared the neighbouring field and reported success. The input under the point is now found before the tap, and the target is bound to its identity, the way callstack#3161 binds an element-route target. Reads and clears take that input's handle while it still carries the identity, and otherwise only the focused input or the app-wide identifier match that carries it; a target bound before its tap never re-reads the point. The lookup and the handle check run inside the text-input probe's issue containment, so a read that cannot answer fails the fill closed instead of ending the runner. Finding the input took up to 2 s more on a React Native bottom sheet, so the replacement's focus allowance rises from 2 s to 4 s.

Summary
fillinto an auto-submitting code field failed withXCTEST_RECORDED_FAILURE(readingframeon the removed input) and invalidated the runner.The runner (Swift only) now:
resolveTextEntryElementrefuses any other input at every post, warmup read-back, verify poll and repair.TEXT_INPUT_NOT_FOCUSED. After full delivery, the fill is unverified ok "typed" only on XCTest's no-match (com.apple.dt.xctest.ui-testing.error10008); any other error returnsTEXT_INPUT_COMMIT_NOT_OBSERVED.Limits:
TEXT_ENTRY_MISMATCHwithout repair, but the successor keeps the text.runSynthesizedReplacementRoute) is not covered.5 files.
Validation
Tested commit: 0eef3bb (rebased on upstream/main 294dc7d, after #3171).
pnpm check:affected --run --base upstream/mainpassed.unconfirmed("0 of 6 digits" to "6 of 6 digits"); auto-submit fill ok withINPUT_REMOVED_AFTER_DELIVERY, no repair; then a tap.