Repository navigation
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Thanks for this. #965 just landed and restructured the decision package, and main also changed Could you rebase onto main once #843 is ported, and fit the decision side to the new layout? The fingerprint in |
d9bf14d to
7d3cc6a
Compare
…utors EngineSnapshot and ExecutorObservation gain an optional keyboardVisible: whether an on-screen keyboard is showing, as the engine measured it apart from the tree. false means it measured none; the field is omitted only when the engine did not measure it. The session copies it onto the semantic observation and the observation feed hands it to the executor, so an executor no longer has to infer the keyboard from keyboard and key nodes that a platform may list only in part. It is a boolean, not the producer's band: dismissing the keyboard only needs to know whether one is up, and the band's frame is in the producer's own coordinate space.
…observation observe() sets keyboardVisible from the capture's keyboard fact: true for a visible band, false for a proven absence, and nothing when the producer could not measure it or published nothing. agent-device's iOS runner publishes the fact with every capture; Android captures carry none, so Android observations leave the field out. The gesture fallback for dismissing the keyboard reads the same fact through the same helper.
The action space reads the observation's keyboardVisible when the engine measured one and falls back to the tree's keyboard and key nodes when it did not. A keyboard the tree leaves out still offers the control, and key nodes left behind after the keyboard closed no longer do. The page fingerprint hashes the measured state beside the pixels, so a dismissal that closes the keyboard without changing the tree no longer reads as unchanged and cannot complete the three-action stall streak. An unmeasured state hashes apart from a measured false.
|
Rebased onto main after #1019; only the docs needed re-fitting to the rewritten pages. |
7d3cc6a to
641a61d
Compare
|
hey, @pvedula7 I bumped agent-device on main - can u retest everything on a live run and report back here? |
okwasniewski
left a comment
There was a problem hiding this comment.
Thanks for the rebases! A few things before this can land:
- Retest on main's agent-device (0.21.25). callstack/agent-device#3240 shipped, so Android captures now carry the keyboard band. A live iOS and Android run (a real IME on Android) would be great, with the output here.
- Docs: please drop the new paragraph in
reference/engine.mdx(it explains agent-device internals and still says Android has no measurement). Keep the field's doc comment, and cutexecutors.mdxanddecision-models.mdxto one sentence each, saying what users see. Please also merge the two changesets into a short one that skips the iOS-runner detail. - Small:
fingerprint()could take the observation instead of 5 positional args, and thekeyboardVisiblechecks could be the same in every layer (=== undefined). packages/mobile/tests/unit/engine.test.tsconflicts with main again.
What and why
raw.keyboard.kind), but that never reachedExecutorObservation, so the decision executor could only guess fromkeyboard/keynodes in the tree.EngineSnapshotandExecutorObservationgain an optionalkeyboardVisible?: boolean.truemeans the engine measured a keyboard on screen,falsemeans it measured none, and absent means it did not measure. The session copies it onto the observation and the feed hands it to the executor (packages/e2e/src/engine/session.ts,packages/e2e/src/agent/observation-feed.ts).dismiss_keyboardonly needs to know whether a keyboard is up, and the band's frame comes back in the producer's own coordinate space (a 402-wide iPhone reportedy: 4583), so I didn't want it in the contract.visiblegivestrue,absentgivesfalse, andunmeasurableor no fact leaves it out. Its gesture dismissal reads the same helper (packages/mobile/src/surface.ts).keyboardVisiblefirst and falls back to fix(decision): offer dismiss_keyboard while an on-screen keyboard is showing #843's tree probe when it's absent:packages/decision/src/elements.tshashes the measured state next to the pixels, withtrue,falseand absent kept apart. A dismissal that leaves the tree as it was still counts as a page change, so it does not feed the stall guard.Android: needs callstack/agent-device#3240
The review guessed Android is where this matters. I checked on a Pixel 8 emulator (API 35), and the field doesn't change anything there:
keyboard?: SnapshotKeyboardBandFactis documented as "iOS:"), and Android snapshots carry nokeyboardkey. So on Android,keyboardVisibleis always absent and the executor falls back to the tree.testImedefaults on forkind === 'emulator'), and the mobile engine doesn't opt out. No soft keyboard is ever drawn, so nothing covers Continue anddismiss_keyboardcorrectly isn't offered.agent-device open --no-test-ime, the case on a physical phone), Gboard does show up in the tree: 127 nodes undercom.google.android.inputmethod.latin. But they're typedandroid.widget.FrameLayoutandTextView, neverkeyboardorkey, so fix(decision): offer dismiss_keyboard while an on-screen keyboard is showing #843's probe misses them too. Onlyagent-device keyboard status(dumpsys input_method) saidvisible: true.callstack/agent-device#3240 adds the fact to Android captures: the band is the
TYPE_INPUT_METHODwindow's bounds, which its snapshot helper already records, so there's no extra adb call. Gboard up readsvisiblewith the window frame, a dismissed keyboard readsabsent, and the headless test IME readsabsent. Once that ships in an agent-device release, this PR picks it up on Android with no change here.Verified
Ran it locally: yes
visible,absent,unmeasurableand no fact map totrue,false, absent, absent), and e2e (the executor gets what the engine reported).decisionExecutorin a throwaway config with a scripted decision model that picksdismiss_keyboardwhen it's offered anddoneotherwise, then logged every observation'skeyboardVisibleand the tree's keyboard/key node count. No API key was needed. The test fills the email, runsagent.act('Hide the keyboard so the Continue button can be reached'), then tapscontinue-buttonwith a locator.e2e run --config e2e.kbprobe.config.ts --target ios-simulator --videoon an iPhone 17 Pro simulator: passed on both runs. The measured state and the tree agree on this screen, and the control is offered and dispatched:--target android-emulatoron a Pixel 8 emulator: passed. No keyboard is drawn (headless test IME), the field is absent, anddismiss_keyboardisn't offered:Android with a real IME, using the agent-device CLI directly (
open --no-test-ime, open the scenario,snapshot --json): Gboard is on screen, the capture has nokeyboardfact, and none of its 127 nodes is typedkeyboardorkey.On iOS the measured state never disagreed with the tree on this benchmark, so I don't have a before/after where fix(decision): offer dismiss_keyboard while an on-screen keyboard is showing #843 alone gets it wrong. On iOS this makes the check use the runner's measurement and stop depending on the tree. The unit tests cover the two cases the tree can't handle: a measured keyboard the tree doesn't list, and key nodes left behind after a measured absence.
Checklist
pnpm changeset) for any change to a published package, or the change is testbed, docs, or CI only.!, the changeset body names what breaks and what replaces it, and the deprecation warning landed a window earlier (stability policy in CONTRIBUTING.md).docs/**/*.mdxpage for the behavior, andskills/e2e/if the skill describes it..d.tsreviewed,tests/types/sdk-types.tsupdated; wire change edits the schema and both fixtures.e2e/engine): changesets fore2e,@e2e-dev/web, and@e2e-dev/mobile.