Repository navigation
fix(find): refuse a focus match behind the visible keyboard (#2622) - #3727
Conversation
The #2589 guard and the action vocabulary it needs (`InteractionAction`, `interactionVerb`) lived in the command surface, so the daemon's mutating-`find` focus seam could not reach either. Both move to @agent-device/selectors, the package the selector pipeline and the daemon dispatch seams already share, and the guard's call sites now import it from there. `keyboardCoveredTabBarSnapshot` moves to @agent-device/test-kit with the guard: the daemon seam test, the guard test, and the ADR 0011 contract scenarios need one copy of the #2589 capture. Its variants (a moved app target, a keyboard lifted off the bottom edge) replace the copies those suites were about to grow.
`find <locator> focus` and the focus leg of `find <locator> type` aim a point themselves instead of re-entering the interaction leaf, so the #2589 guard never ran: a match behind the visible keyboard reported `Focused (x, y)` while the key ate the touch (#2622). The seam now runs the same guard the acting paths run, against the tree find matched, at the center it dispatches, ahead of the ref-frame expiry so a refusal leaves the frame pinnable. ADR 0011 gains the classified `find-focus-dispatch` row for this seam, with its enforcement proven by contract scenarios over the daemon harness and its four divergences from the acting rows recorded as owned gaps: no off-screen refusal, no parent-owned touch point, no moving-target gate, and a dispatch-only success claim.
…dispatch row The selectors export enumeration gains `./interaction-action` and `./keyboard-occlusion`, the guarantee matrix's three keyboard cells now name the guard's new module, and the acknowledged-gap list grows by the four cells the new row declares, each linked to its tracking issue.
![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 25 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Size Report
Startup median (7 runs, lower is better):
|
|
The code in 8fe670b needs live evidence before merge. Smoke Tests are still queued, so CI has not shown a result yet.
On the open threads from the other reviewer, the nonHittable cell marked 'runtime' and the find.ts:430 fallback that never runs still apply, and one lower-priority thread on the 'be tapped' default for drag also still applies. The keyboard-occlusion.test.ts:91-101 thread does not apply: the 'cannot safely' message can only come from keyboard-occlusion.ts:120, and the test just before it asserts the typed details. Please resolve it. Not blocking, and fine to take or leave: I did not run the seam, unit, contract or layering suites. I read the diff and the base find.ts, so fail-before is inferred from the test assertions. Smoke Tests may exercise the find route, so a later failure there would need a look before it is called unrelated. Before merge, the live iOS and Android runs above must show the refusal and the focus above the band. |
interactionVerb fell through to 'be tapped' for every action it did not list, so a keyboard- or coverage-blocked drag, swipe, or scroll endpoint was refused in the words of an action nobody asked for. Each action now names itself, and with no default arm a future InteractionAction fails to compile until it chooses the verb its refusal will use.
The action-loop assertion checked only that the message contained the verb, so it would stay green on an unrelated rejection that happened to mention the action. Each iteration now runs the full typed refusal — COMMAND_FAILED, reason tap_keyboard_occludes_target, dispatched: 'no' — and matches the verb on the message of that same refusal.
nonHittable claimed runtime for a guarantee whose second half never runs: the seam promotes a non-hittable match to its hittable ancestor but never annotates the one promotion declines, so it cannot claim the whole cell. It is an owned gap on #1081 now, and the Layer-3 scenario that would have overclaimed the waived cell is gone — the promotion half stays proven by the seam's unit test named in the reason. The responseIdentity rationale named a data.ref the focus and type payloads never emit; it describes the omission instead.
…verb
Both mutating focus legs ended in `response ?? { ref: match.ref }`, but
executeFocusPoint and executeBoundTypeText always return a payload, so
the branch never ran and it was the only place this route could have
issued a data.ref. Removing it makes the responseIdentity waiver a
structural fact rather than a claim about a branch.
rejectCoveredFindMatch now takes the InteractionAction and names itself
through interactionVerb, so the covered and keyboard refusals on this
route draw their verbs from one source.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
ef82156 to
a0135d0
Compare
|
Live evidence is in the body now, on a simulator I created and an emulator instance I started (both deleted afterwards). Point by point: iOS refusal, both legs. Done at Does a real capture carry the keyboard into
Two Android-surface facts, in the body too: the refusal needs One correction I owe you about my own setup: my first Android passes showed The two threads you said still apply ( The Non-blocking suggestions. Both taken: Gate. Your last line ("I did not run the seam, unit, contract or layering suites"). To close that gap cheaply: the gate covers all of them and passed at |
|
Gate re-run on the exact pushed head: |
|
The code at a0135d0 looks good to me, and the earlier findings from 8fe670b are fixed. The typed-reason assertions in the keyboard occlusion test now cover every action, the nonHittable guarantee is a waived gap that cites #3734, the action switch is exhaustive, and the response identity reason matches the payload. I found nothing new to ask for. The five inline threads from the other review are fixed at this head, so you can resolve them: #3727 (comment), #3727 (comment), #3727 (comment), #3727 (comment) and #3727 (comment). I took the live transcripts from the PR body and did not rerun them. I did not run the unit, contract or repo-guard suites. The gate receipt is at ef82156, which differs from this head only by a comment. Both Smoke Tests jobs are still queued, so CI has no result yet. Smoke may exercise find, but there is no failure to attribute. They need to finish green at a0135d0 before merge. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
find <locator> focusand the focus leg offind <locator> typeaimed a point themselves, so they never passed the pipeline door where the #2589 keyboard guard runs: a match behind the visible keyboard reportedFocused (x, y)while the key ate the touch. Closes #2622.Landed as the issue's option 2: the guard plus the vocabulary it needs (
InteractionAction,interactionVerb) move to@agent-device/selectors, which both zones already import, so R2 is satisfied by placement, not exemption. The seam calls that one guard against the tree find matched, at the center it dispatches, ahead ofexpireRefFrame— a refusal leaves the frame pinnable.25 files: the move, the seam fix, an ADR 0011 path row (
find-focus-dispatch, 14 cells classified) with daemon-harness scenarios, one shared #2589 fixture in@agent-device/test-kit. The row declares four gaps that are this seam's pre-existing behavior, newly visible because the row exists:offscreen(findActsaysignore),parentOwnedTouchPoint(taps the rect center),nonHittable(promotion runs, annotation never does),targetStability(no #3471 gate). All four cite #3734, which owns closing them;outcomeObservationstays on #3335.Stale in #2622: paths are now
packages/daemon/..., and option 2's cost was overstated — the pipeline-level half of the moved test still drives the guard throughcreateInteractionDevice, so the split was a rename plus a trimmed unit suite.Live evidence
Devices are mine, not shared: iOS simulator
2622-keyboard-guard(iPhone 17 Pro, iOS 27.0, created for this run) and AVDkb2622_api35(emulator-5580, started for this run). Both deleted afterwards. Agent Device Tester via Metro, Form tab, Gboard on Android. Transcripts at heada0135d0d6, the gate-tested SHA.iOS — refusal on both legs, positive control above the band
Keyboard raised by focusing the email field, seeded with
pre@live.test.Settingsis the tab-bar button at y 795, centered inside the band.On this producer
data.keyboardisnull, so the refusal above came from the guard's tree-derived rule reading thekeynodes in find's own capture. That is the direct answer to the fail-open question: with no measured band, find's tree still carries the keys and the guard refuses.Android — both legs refuse on the measured-band producer
Landscape, Gboard. This is the producer the review asked about — the helper's measured IME window, not the tree rule:
keyNodes: 0is the point of that pair: with no keys in the tree, the tree-derived rule cannot fire, so those refusals can only be the measured band reaching the guard throughmatch.keyboard.Two notes on the Android surface, both reproducible:
--raw. In the interactive projection the same app node is markedinteractionBlocked: coveredby the helper's cross-window scan, so the projected route stops it one stage earlier with a different reason:find label Settings focusanswersAMBIGUOUS_MATCH(the app node plus Gboard's ownSettingskey) andfind label Settings focus --lastanswersreason: "target_covered". So on Android with this app the misfire is blocked by occlusion when the node is projected as covered, and by this guard when it survives as a match. In portrait no app node is centered in the band at all (adjustPanmoves the window), so landscape is the reachable case.ime setis required,settings put secure default_input_methodis not enough. My first Android passes reportedkeyboard: {"kind":"absent"}withdumpsys input_methodsayingmInputShown=true, and the guard dispatched behind what looked like a keyboard. That was my setup, not the producer: the bound IME was the app's bundled headlesscom.callstack.agentdevice.imehelper, which reports itself shown while drawing no window — the screenshots from those runs have no keyboard in them, so there was nothing to occlude and the dispatch was correct. Afteradb shell ime set com.google.android.inputmethod.latin/...the band measures on every capture. No producer bug to report.Validation
a0135d0d6, base9ca333d5f.check:affected --run --head a0135d0d6: all runnable checks passed,exactCommit=true(receipt run-tQKoZm).check:layering: OK.response.oktrue, focus dispatched); with it, 4 of 4. The 6 contract scenarios pass.outcomeObservationstays on click and press report dispatch, not landing: state the contract and track the outcomeObservation gap #3335.🤖 Generated with Claude Code