fix(keyboard): verify Android typed text landed instead of reporting silent success - #592
Draft
latekvo wants to merge 4 commits into
Draft
fix(keyboard): verify Android typed text landed instead of reporting silent success#592latekvo wants to merge 4 commits into
latekvo wants to merge 4 commits into
Conversation
…silent success
`adb shell input text` converts the string to KeyEvents through the virtual
KeyCharacterMap and injects them back-to-back with no cadence. A field whose
owner re-renders per keystroke drops events out of that burst, and the transport
gives no signal: adb exits 0 having "typed" the whole string, so the tool
returned `{ typed, keys: text.length }` for text that never arrived.
Reproduced on a Pixel 6 / API 34 emulator against the Settings search box: one
`input text` of a 76-character sentence landed 45 characters and the tool
reported full success. The same sentence injected in 8-character chunks landed
all 76.
The Android phone/tablet backend now reads the focused editable field before and
after injecting (android-devtools `getHierarchy`, `clearCache: true` — the
helper's long-lived UiAutomation connection otherwise serves stale text) and
compares. On a mismatch it undoes exactly the characters it added and retypes at
the chunked cadence, once; a field whose prior content cannot be provably
restored is left untouched. The result carries `verified?: boolean` and an
advisory `note?: string` following the `open-url` convention, so a call that
could not verify says so rather than implying the text landed.
Notes are value-free by construction (counts and structural facts, never the
field's contents) and `execute` additionally scrubs them on the secret-bearing
path, so a resolved `{{secret:…}}` read back off the screen cannot reach the
transcript.
iOS, Chromium, Vega and the TV backends are unchanged: `adb shell input text` is
the transport that loses characters, and Android TV already types one word per
call at the chunked cadence.
…elies on The repair converts a mismatch into a backspace count, which is only correct because backspace deletes at the cursor and the cursor sits after whatever landed — so it undoes an insertion anywhere in the field, not just a suffix. Nothing pinned that: every repair test inserted at the end, where a planner that assumed a suffix insertion would behave identically. Confirmed on device (Pixel 6 / API 34) before pinning it: with the cursor between the two characters of "ab", `input text XY` gives "aXYb" and two backspaces give "ab" back. Recorded in the planner's docstring, since the arithmetic is otherwise justified only by reasoning about a transport whose behaviour is worth stating as measured.
Review of the read-back turned up three ways it could make things worse than the
silent success it replaced.
A correct type could be doubled. `input text` replaces a selection, so typing
"argent" into a `selectAllOnFocus` field that already reads "argent" succeeds and
changes nothing — byte-identical to the same call having every key event dropped.
The comparison read that as failure and retyped, leaving "argentargent" and
reporting verified. The same shape appears when a field already reads exactly the
typed text and when an empty field's hint equals it. The verdict is now
three-way: readings consistent with both success and failure are `indeterminate`,
which reports and retypes nothing. The other ambiguous shape is a field that now
reads exactly the text while its previous value survived as a prefix or suffix
("abc" replaced by "abcdef" versus "abc" plus a partial landing of "def").
The repair could type into the wrong field. Field identity was `resource-id` plus
class, and `resource-id` carries the React Native `testID`, so every untagged
`TextInput` and Compose `TextField` dumps an empty one and all of them compared
equal. On an auto-advancing form (an OTP code across boxes) focus moves mid-call,
the guard did not fire, and the retype went into the next field. Identity now
includes the bounds origin, which distinguishes fields while surviving the width
change typing causes — measured: the Settings search box's right edge moves from
1080 to 933 with the origin fixed.
A failed retry reported total failure. The undo runs before the retype, so an adb
failure between them left the field emptier than the call found it while the
error said the typing failed. It is now caught and reported as such.
Also from the review: raise the read's node cap to the flow tree's 12000, since
the helper's 5000 default truncates a dense screen and a capture that stops before
the field is indistinguishable from a screen with no focused field — reported as
truncated rather than as "nothing had focus"; give a focus change its own note
instead of a read failure; and correct four claims that were wrong. Android TV
shares this transport and is NOT covered (it splits only on spaces, so a
space-free email or username is still one burst) — the tool description no longer
implies other platforms are immune. A password field's text is not masked in the
dump, so the reason to skip it is that reading a credential back would put it in
the result. The note's character counts do reveal a typed secret's length, which
`keys` already exposed. And the cost analysis now includes the helper's cold
start, not just the warm socket.
Finally, the flow `type` directive discarded the result, so a flow still greened a
step for text that demonstrably did not land — the same defect one layer up. It
now fails the step on an explicit `verified: false`, carrying the tool's note as
the reason and skipping the submitting Enter, while an absent `verified` (not
checked) still passes.
… hint from An empty EditText reports its HINT as `text`, which this code already knew. What the undo did not account for is that a hint can share an edge with the typed text — "https://" under "https://example.com", "0" under "100". Plan A then measures growth against a prior value that was never in the field, deletes hint.length too few, and the retype lands on the residue. The result is a doubled value ("https://https://example.com") whose length is exactly before + text, so classifyTypedText's first branch calls it landed: verified true, no note, and the flow `type` gate greens the step and submits it. Both proofs can hold at once, and where they do they disagree about which characters are ours, so the overlap now returns null and the field is left alone. Where the baseline cannot have been a hint, plan A still applies. Also on the read-back: - readAfter discarded the `truncated` flag and treated "nothing focused" and "a different field focused" alike, so all three reported "focus moved to a different field … the text may have been split across both fields" — inventing a second field for a capture that simply stopped short. They now report separately, and the truncated case says what the baseline read already says. - Those notes claimed "nothing was retyped" on the post-repair read, where the field HAS been backspaced and retyped. Saying so invites a third typing of the value. - repairFailedNote reported text.length as the number of characters removed; the undo deletes `deletions` (8 backspaces were issued while the note said 12). - mismatchNote compared a whole-field length against a typed count: a field holding 2 prior characters, typed 12, 8 landed, read "10 where 12 were expected" — 4 were lost, not 2. It reports what is known instead. - completedMsg said "Entered text" over a verified:false result, putting the silent success back in the log line a user watches. The note is no longer substituted over for secrets. Every note is value-free by construction and pinned as such; the substitution replaced bare occurrences anywhere, so a two-character secret rewrote ordinary words ("uiautomator" → "uiautomat{{secret:W}}") and a numeric one swallowed the character count, on notes a successful call returns. It could not have caught a leak either — a dropped-character read-back holds a PARTIAL secret, which whole-value replacement never matches. Errors are still scrubbed: they quote the argv verbatim. Corrects measured claims that the evidence does not support: the password field reads back as bullets rather than plaintext (API 34, through the helper), the verified latency range is 1.9-3.4 s and not 3.5-4.0 s, a hint CAN share an edge with the typed text, and a whole-field selection replaced by a shorter string is accepted rather than being the stated limitation. Adds coverage for the shapes that were unpinned: the field that received nothing at all (the reported digits-only case), the inter-chunk pause the repair depends on, the truncated read-back, focus lost outright, and the post-repair notes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
QA, Bluesky on a physical Android phone.
keyboardtyped a plain sentence into a text field and reported full success; the field held something else.The same string typed at the same moment into a different field on the same screen arrived character for character. The tool returned
{ typed, keys: 76 }either way — it had no idea.Root cause
typeAndroidPhone→injectAndroidText→adb shell input text <segment>.input textconverts the string to KeyEvents through the virtual KeyCharacterMap and injects them back-to-back with no cadence. A field whose owner re-renders per keystroke — a controlled RNTextInput, a search box that re-queries on every change — drops events out of that burst.adbstill exits 0, so nothing surfaced, andkeys: text.lengthwas returned unconditionally. That silent success is the defect; the dropped characters are a property of the field, which is why one field on the screen was fine and another was not.Reproduced on the emulator, no Bluesky needed — one
input textof the 76-char sentence into the Settings search box landed 45 characters:The same sentence injected in 8-character chunks with a pause between them landed all 76. That asymmetry is what the repair below exploits.
What changed
platforms/android-verify.ts(new) — the Android phone/tablettextpath now:Reads the focused editable field before and after injecting, via the
android-devtoolsblueprint'sgetHierarchywithclearCache: true. Mandatory: the helper's long-lived UiAutomation connection serves staleAccessibilityNodeInfotext, the same reasonflows/flow-android-tree.tspasses it. Without it the "after" read can return the pre-typing value and the check is theatre.Classifies the reading three ways, because two of the shapes it can see are produced by both success and failure and acting on a guess is worse than saying so.
textappears contiguously and the field grew by exactlytext.length(a dropped character fails the substring half, a doubled injection the length half); or the field now reads preciselytextand its previous value did not survive into it. The second clause covers the empty-field case, which the length arithmetic cannot: an emptyEditTextreports its HINT as itstextattribute (confirmed on API 34 — the empty Settings search box readstext="Search settings"), so the baseline is not"".input textreplaces a selection, so typing "argent" into aselectAllOnFocusfield already reading "argent" succeeds and changes nothing, which is byte-identical to the same call having every key event dropped. Likewise a field that now reads exactlytextwhile its old value survived as a prefix/suffix ("abc" correctly replaced by "abcdef" versus "abc" plus a partial landing of "def").Repairs, once. Deletes exactly the characters it added and retypes in 8-char chunks — a different cadence, not a blind repeat. Two proofs of what to delete, conservative first: (A) the field grew by
addedandbeforeis recoverable by deleting one contiguous run ofaddedchars → deleteadded; (B) the field holds only a subsequence oftext, which proves it was empty and the baseline was a hint → deleteafter.length. Dropped key events delete characters without reordering or inventing any, so real corruption is always a subsequence — QA's corrupted string is one. Both proofs cap the count attext.length. If neither proof holds, nothing is deleted and the failure is reported instead: a field an input mask rewrote keeps its content.The undo is a backspace count rather than a rewrite, which is only correct because backspace deletes at the cursor and the cursor sits after whatever landed — so it undoes an insertion anywhere in the field, not only a suffix. Verified on device rather than assumed (second commit): with the cursor between the two characters of
ab,input text XYgivesaXYband two backspaces giveabback. A property check over the planner is what surfaced that this premise was load-bearing and unpinned — every repair test had been inserting at the end of the field, where a planner that assumed a suffix insertion behaves identically.The field's identity — checked between the two reads so a focus change is caught — is
resource-id+ class + bounds origin.resource-idalone is not enough: it carries the RNtestID, so every untaggedTextInputdumps an empty one and an auto-advancing form (an OTP code across boxes) would have the retype land in the next field. The origin distinguishes fields while surviving the width change typing causes (the Settings box's right edge moves 1080 → 933, origin fixed).A retry that cannot reach the device is caught: the undo runs before the retype, so an adb failure between them can leave the field emptier than the call found it, and that is reported rather than surfacing as an error implying nothing happened.
Never reports silent success.
verified?: boolean+note?: stringonKeyboardResult, mirroringopen-url'snoteconvention.verified: true= read back and confirmed.false= read back and wrong, after the retry. Absent = no read-back happened,notesays which reason (no editable focus / password field, whose text uiautomator masks / read failed or focus moved / helper unavailable). A read-back that cannot run never fails the call — the keystrokes are already on the device by then, so throwing would report a failure that may not have happened.Latency, gated deliberately. Verification is unconditional when the fast helper read is available, and skipped-with-a-note when only
uiautomator dumpwould be — so a locked-down device (exactly the device whereadb install -tis blocked) does not pay two dumps per typed string. Measured on one emulator, screen and string, 76 chars into the Settings search box: 1.6-1.8 s unverified, 1.9-3.4 s verified (corrected in 1a5c4de — the five verified trials below are 3.41/1.96/1.91/2.20/3.05 s, none of which fall in the 3.5-4.0 s originally stated). I did not gate on text length: the corruption is a property of the field, not the length (QA's short string was fine in one field, the long one corrupt in another), so a threshold would leave the silent-success bug reachable under it and make the guarantee depend on a magic number. The 500 ms settle is kept on both reads on purpose — dropping it from the "before" read saves 500 ms but risks reading before the framework has publishedfocused="true", turning a healthy call into a spurious "no editable field had focus" note. The numbers are in the module docstring so this is arguable with data.The flow
typedirective now honours the verdict. It discarded the keyboard result, so a flow still greened atypestep for text that demonstrably did not land — the same defect one layer up. An explicitverified: falsenow fails the step, carrying the tool's note as the reason and skipping the submitting Enter (submitting a field holding the wrong value is worse than not submitting). An absentverifiedstill passes: not checked is not failed, which is what keeps iOS, Chromium, Vega and helper-less Android working.Secrets. The read-back holds the resolved plaintext of a
{{secret:…}}type. Notes are value-free by construction — counts and structural facts only, never the field's contents — andexecuteadditionally runs the newredactSecretsovernoteon the secret-bearing path, so the value-free property of one module is not the only thing between a secret and the transcript.redactSecretsFromErrorstill covers the error path (it now shares that helper). Both halves are pinned by tests. The counts in a mismatch note do reveal a typed secret's length;keysalready exposed exactly that for every secret type, so this bounds the leak at what the result already had rather than eliminating it, and the field JSDoc says so instead of claiming counts are leak-free.Verification
Unit —
test/keyboard-android-verify.test.ts(new, 51 tests) + extensions tokeyboard-android.test.ts,keyboard-secrets.test.tsandflows/flow-type-focus.test.ts:makeAndroidImpl().handlerwith a stub registry serving a scripted sequence of hierarchies, one pergetHierarchycall, so a corrupted read-back drops in at a chosen point. Proves the mismatch is detected, the repair runs (asserting the exact ordered adb command list: burst →input keyevent 67 ×8→input text 'abcdefgh'→input text 'ijkl'), and that a still-failing repair reportsverified: falsewith the exact counts.input textreached the device.executeboundary scrubs a value-bearing note to its placeholder.Mutation sweep — 32 mutations applied one at a time to the merged behaviour (both halves of the classification, each ambiguity clause, the identity's bounds origin and the full-bounds over-correction, plan A/B and their ordering, the deletion cap, the suffix-only planner,
clearCache, the raised node cap, the truncation diagnosis, the document-order walk, the focus-moved note, the repair's chunking and its error handling, the 64-per-call undo chunking, the before-read injection, the secret scrub, the flow gate in both directions — swallowingverified: falseand over-strictly failing on an absent one). 32/32 turned the suite red. Three sweeps were needed: the first found the dropped-character tests being caught by the length check rather than the substring check; a property check over the deletion planner found the unpinned mid-field undo; the third found the ambiguity clause that only matters when the field contains but does not equal the text, and the entirely-uncovered flow gate.E2E — two emulators across the two passes,
emulator-5556(Pixel 6 / API 34) andemulator-5554(Pixel 3a / API 34), both allocated viarequest-device, driven over HTTP (ARGENT_PORT=3097 node dist/index.js start,POST /tools/keyboard), read back through the tool-server's owndescribe. (A host-sideuiautomator dumpmust NOT be used for the read-back while the helper runs — the two fight over the UiAutomation connection and corrupt each other's reads. That cost me an hour of bogus measurements and is worth knowing.)Plus the device measurement the repair arithmetic rests on: with the cursor between the two characters of
ab,input text XYgivesaXYband two backspaces giveabback — N backspaces remove exactly the N injected characters wherever the cursor sat.Gates, all from the worktree root:
npx tsc --build,npx prettier --check .,npx eslint . --max-warnings 0,npm run typecheck:tests -w @argent/tool-server,npm test -w @argent/tool-server→ 296 files, 3125 passed, 1 skipped, 0 failures (the knownforwards SIGTERMandlens-tools-platform-gateflakes did not fire).Deliberately left out
adb shell input textis the transport that loses characters; none of the others synthesise a KeyEvent burst through a KeyCharacterMap.platforms/tv.ts, the nearest twin — it funnels through the sameinjectAndroidText). It shares the "no read-back" shape but not the exposure: the TV backend already types one space-free word perinput textcall with aKEYCODE_SPACEbetween words, i.e. the chunked cadence the phone path has to fall back to after catching a drop. AndtypeTvis shared with Apple TV, whose HID transport cannot read a field back at all, so verifying half its callers would mean adding a platform branch to a function that deliberately has none. Noted in a comment there.editableprobe diverges from the twin.blueprints/android-tv-control.tsuses/EditText/; this uses/EditText|AutoComplete/, becauseAutoCompleteTextViewandSearchView$SearchAutoCompleteareEditTextsubclasses whose simple name lacks "EditText". There the verdict only labels an elementtextfieldfor the agent to read; here it decides whether a correctness check runs at all, so a missed subclass would silently disable verification. Left the twin alone to keep TV describe output unchanged; commented at the definition.adb install -t, which works on a normal developer-enabled physical phone, so the reporter's device should get the real check — but an MDM-locked device that refuses the install gets the note instead of a verdict, by design (the alternative is twouiautomator dumps per typed string). The guarantee is "verified, or told it could not be", never "always verified" — which is also why an absentverifiedmust never be read as a pass.keysstill counts the characters the call asked to type, not presses a repair re-sent; making it non-deterministic per repair seemed worse than leaving it as the requested count.Interaction with the open keyboard PRs
Both touch the same lines; this is based on
mainand will conflict textually with either.fix/android-keyboard-key-orderrewritestypeAndroidPhoneend to end (reorders thekey/textbranches, drops the up-frontassertTypeableAndroidText) and rewrites theReturns { … }line. This change also rewrites both. The read-back call site moves with whichever branch order lands; the up-front validation matters here for the same reason it did there — an un-typeable text must reject before the helper is resolved, so no APK install happens for a rejected call.clearon the keyboard tool #580feat/keyboard-clear(stacked on fix(tool-server): reject a keyboard call carrying both text and key #579) addscleared?: booleantoKeyboardResultwith a docblock that states Android "does not read the field back" — this change makes that sentence wrong, so it needs updating on merge. feat(tool-server):clearon the keyboard tool #580 also adds its own read-back machinery for the clear path:utils/android-ui-dump.ts(dumpAndroidUiXml), plusreadHierarchyandmeasureFocusedTextLengthinandroid-input.ts, and exportsattrIsTruefrom the uiautomator parser. That overlaps this module's focused-field read and should be consolidated to one helper rather than shipped twice — notably feat(tool-server):clearon the keyboard tool #580 reads viauiautomator dumpwhile this reads via the android-devtools helper, and a host-sideuiautomator dumpand the helper's UiAutomation connection fight over the accessibility connection and corrupt each other's reads (I hit exactly this while building the E2E harness). Worth resolving deliberately whichever merges second.clearon the keyboard tool #580'skeyboard-clear.test.tsasserts ~28 exact orderedadbShellcommand lists on the Android path; a read-back adds adb round trips, so those go red on the merged result even with no textual conflict.Happy to rebase this onto whichever of the two lands first.
Follow-up: 1a5c4de
Review found a data-corruption path in the repair, plus a set of notes that describe something other than what the code did.
The undo could read a hint as prior content and double the value
FocusedField.textalready documents that an emptyEditTextreports its HINT. What the undo did not account for is that a hint can share an EDGE with the typed text, which the docstring explicitly ruled out ("a hint shares no prefix or suffix with the typed text"). It is not exotic — a URL bar hintedhttps://underhttps://example.com, a quantity box hinted0under100.Worked through the real functions, hint
https://, typedhttps://example.com, first burst landing 12 characters:So the field ends up holding a doubled value, the caller is told
verified: truewith no note, andflow-actions.tsgreens the step and presses Enter. The corruption is shaped to satisfy the success branch, so it can never surface as a failure. A sweep over hint/text/drop-point combinations produced 35 corrupting repairs, all 35 classifiedlanded; the same sweep over real (non-hint) baselines produced none.Both proofs can hold at once, and where they do they disagree about which characters are ours. That overlap now returns null and the field is left untouched. Where the baseline cannot have been a hint —
XYis not a subsequence of the typed text — plan A still applies, so the undo is not disabled for real content.The test that pinned the old behaviour (
plannedUndoDeletions("a", "abcdefghi", "abcdefghijkl") === 8, "prefers plan A over emptying the field") was pinning the trigger:"a"there is equally a prior character and a hint. It now asserts the decline.Notes that said something other than what happened
repairFailedNotemismatchNotecompletedMsgverified: falseverifiedstays quietreadAfterdiscarded thetruncatedflagreadFocusedFieldreturns and treated "nothing focused" and "a different field focused" identically — the asymmetry with the baseline read, which has a dedicated note for exactly that evidence, was the finding.Secret notes are no longer substituted over
redactSecretsran over the advisory prose. Because it replaces bare occurrences of the value anywhere, a two-character secret rewrites ordinary words and a numeric one swallows the count the note exists to report — on notes a successful call returns:The second is worse than noise: it reads as though the field holds the credential. And the substitution could not have closed the gap it was defending — it matches whole values only, while a dropped-character read-back by definition holds a PARTIAL secret. The guarantee that actually holds is that every note is value-free by construction, which is pinned directly. Errors are still scrubbed; they quote the
input textargv verbatim.redactSecretsis now module-private, since nothing outsideredactSecretsFromErroruses it.Flagging this one for your call — it reverses a documented defence-in-depth decision. The corruption is real and lands on healthy calls, but if you would rather keep the scrub, the alternative is to withhold a colliding note entirely rather than rewrite it.
Claims corrected against evidence
EditTextholdingSecretPass1reads backtext="•••••••••••", 0 plaintext occurrences;uiautomator dumpagrees. The module doc and the agent-facing note both asserted the opposite ("NOT masked …attrs.textstill holds the secret"), while this PR's own test title says "whose text is masked". Skipping is still right, for two reasons now stated: the comparison could not work, and the masking is a platform default rather than a guarantee this code can enforce.landed, not reportednot-landedas the "known limitation" paragraph claimed. Narrowed to the inner-selection case, which does behave as described.describe/platforms/android/index.ts, which has no multi-window material. Now points at the helper's owncaptureMode/windowCount.waitForIdleMs; the 500 ms is the blueprint default.resolveServicefailure that also covers a spawn that timed out.flow-actions.tsnarrated the pre-diff state ("greened atypethat typed something else"), which reads as nonsense once the diff is history.Tests
Mutation-checked; each of these previously survived green:
readAfter's three outcomesafter === beforealways indeterminateThe last two are the gaps the review turned up: the pause has JSDoc calling it "the whole point" but only the chunk size was pinned, and no test covered
before === afterwith the field holding none of the typed text — the reported digits-only case. Also added: the truncated read-back, focus lost outright, the post-repair notes, and a short/numeric secret leaving a note intact.androidNotenow resets inafterEachso one failure no longer cascades into the next test.Verification
npx tsc --build packages/tool-server— cleannpx tsc --noEmit -p packages/tool-server/tsconfig.test.json— cleannpx eslint packages/tool-server/src packages/tool-server/test— cleannpx prettier --check— cleanvega-cli-timeout.test.ts("reaps the ENTIRE worker tree on timeout") that passes 9/9 in isolation; it is a real-subprocess test under full-suite load, touched by neither this PR nor the follow-up.Left alone deliberately
keyboard's description says "when both are given, the text is typed first and the key is pressed after it", butplatforms/android.tspresses the key FIRST andkeyboard-android.test.tsasserts that order as the source contract. The line is pre-existing on main, and #579 is described as reordering those branches — fixing it here risks inverting a statement that PR makes true, so it wants its own change.