fix(tool-server): reject a keyboard call carrying both text and key - #579
Open
hubgan wants to merge 10 commits into
Open
fix(tool-server): reject a keyboard call carrying both text and key#579hubgan wants to merge 10 commits into
hubgan wants to merge 10 commits into
Conversation
This was referenced Jul 28, 2026
hubgan
marked this pull request as ready for review
July 29, 2026 09:23
latekvo
reviewed
Jul 29, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
[Diplomat]: Reviewed the ordering fix end to end on a live Android emulator (Pixel_9a, API 37), running a branch build and a merge-base build of the tool-server side by side against the same focused field. { text: "abc", key: "backspace" } over XYZ gives XYabc on the merge-base build and XYZab on this branch, and { text: "100%safe", key: "backspace" } lands the key after both % segments (100%saf) — so the multi-segment case the new test pins holds on a real device too. Mutation-checked the new tests as well: swapping the block order back, or dropping the up-front resolveAndroidNamedKeycode, turns them red. Two notes on the runType comment inline.
Member
This is so weird I don't think we should even allow inputs like this. |
…backend `typeAndroidPhone` pressed `params.key` before `params.text`, while every other keyboard backend (simulator-server, chromium, vega) does the reverse and the tool's own description promises "the text is typed first and the key is pressed after it". Commit e45f148 (#486) fixed the other three backends; Android was not included, and keyboard-android.test.ts then locked the wrong order in as the "source contract". Reproduced on a Pixel_3a (API 36): a field containing `XYZ` given `{ text: "abc", key: "backspace" }` produced `XYabc` — the backspace fired first and ate a character of the field's *previous* value. It now produces `XYZab`. Because the key is now pressed after the text, an unknown key name would reject only once the text had already landed on the device. Split `resolveAndroidNamedKeycode` out of `injectAndroidNamedKey` and resolve the name up front, next to `assertTypeableAndroidText`, so a bad request in either half still rejects with no on-device side effect. Mirrors the resolve-then-inject shape the vega backend already uses. Also asserts the Android ordering in keyboard-key-order.test.ts, so all four backends are pinned in one place.
…ard call
The previous note said the split was purely an Android-ordering workaround
and could now be collapsed. That is not safe: `typeTv` rejects `key` before
it types anything, so a combined `{ text, key: "enter" }` on an Apple TV /
Android TV target throws with the field still empty, whereas two calls type
the text and fail only on the Enter. `runDirective` gates `type` on Vega
only, so TV targets do reach this path.
Two gaps from review of the ordering fix: - Nothing asserted that the named key lands after ALL `input text` segments. `%`-bearing text is split into one `input text` per segment, so an Enter arriving between them would submit `100%` and drop `safe`. - The comment above the android block in keyboard-key-order.test.ts framed both tests as the ordering regression, but only the first pins the flip — the reject-before-typing test guards the up-front keycode resolution and is green against the old order. A reader trusting the comment could delete the real regression test. Scoped each comment to what its test actually pins.
…history The comment claimed the split began as a workaround for the Android backend pressing `key` before `text`. It did not: at 3700c20, where the split and its original wording were written, `platforms/android.ts` routed phones to `typeSimulatorServer` and `utils/android-input.ts` did not exist — the `adb input` ordering this branch fixes only arrived nine days later in 44b38d4. Every backend pressed the key first back then, which is what the original platform-neutral wording said. Drop the history and state the reason the split is kept today. Also correct the TV claim: an Apple TV never reaches these keyboard calls, because the focus `gesture-tap` above resolves simulator-server, which rejects a tvOS UDID. Android TV is the one TV kind that gets here.
…oard path Mutation-testing the branch surfaced three survivors, all on one axis: the key's identity was never verified beyond `enter`. - `resolveAndroidNamedKeycode` returning a hardcoded 66 for every name passed the whole suite. The keycode map's literal values were pinned, but nothing pinned that the resolver reads the map. - The combined text+key path pressing a hardcoded "enter" also passed. Every combined-call test used `key:"enter"`, so `key:"backspace"` - the case the ordering exists for - was argued only in prose. - Dropping the up-front `assertTypeableAndroidText` passed too. With the key pressed after the text, `injectAndroidText` self-validates before its first adb call, so that line no longer buys the no-side-effect property; its only remaining effect is which error a request bad in both halves reports. Add a test for each, plus one pinning that a failed text injection leaves the key unpressed. Correct the comments accordingly: the up-front text check is now described by what it actually does, and the resolver's docblock no longer documents one caller's sequencing decision. Trim the remaining diff-scoped comments to state-describing ones.
The comment on the resolver test claimed a constant-returning resolver would otherwise be green. It would not: the backspace ordering test added in the same commit already catches that. What only this test catches is a single mistyped entry among the 22 names no injection test presses, so say that instead. Also drop the claim that every other combined-call test uses `key:"enter"` (the both-halves-invalid one passes `"bogus"`), and restate the un-typeable-text case in terms of the property it guards — nothing on the device - rather than attributing it to the up-front text check, which the previous commit established is not what buys that.
hubgan
force-pushed
the
fix/android-keyboard-key-order
branch
from
July 30, 2026 11:52
fe4272b to
bae58b6
Compare
A combined `{ text, key }` request has no meaning a caller can rely on. With
`key: "enter"` it reads as "type, then submit"; with `key: "backspace"` it reads
just as naturally as "delete, then type" — and whichever order a backend picks,
the other reading silently corrupts the field. That is what made the Android
backend's key-before-text order hard to call a bug rather than a choice: against
a field holding `XYZ`, `{ text: "abc", key: "backspace" }` left `XYabc` on
Android and `XYZab` on the other three backends, and neither is what the caller
asked for.
So reject the shape instead of picking an order. The check is the first thing
`execute` does — above the secret resolution and the platform dispatch — so a
combined request resolves no `ARGENT_SECRET_*` value, reaches no device, and no
backend has to defend against it. It keys off the parameters being PRESENT
rather than truthy, so the rule stays "one or the other" with no empty-string
carve-out to document. `run-sequence` already batches two keyboard steps into
one round-trip, so "type, then submit" loses no ergonomics.
The Android backend keeps its text-then-key block order for symmetry with the
other three, but the machinery that existed only to make a combined call safe is
gone: `resolveAndroidNamedKeycode` (split out purely so a bad key name could be
rejected before the text landed) folds back into `injectAndroidNamedKey`, the
hoisted `assertTypeableAndroidText` goes with it, and so does vega's duplicate
pre-resolve. All four backends' "key after text" comments described a request
they can no longer be handed.
Live verification on a Pixel_3a (API 36), an iPhone 16 Pro and Chrome 150, all
against a focused field holding `XYZ`. The released 0.17.0 build accepts
`{ text: "abc", key: "backspace" }` and leaves `XYabc` (Android) / `XYZab` (iOS,
Chromium); this build answers 400 with the field untouched, and the two separate
calls leave `XYZab` everywhere — including as two `keyboard` steps inside one
`run-sequence`, where a combined step is rejected the same way and stops the
sequence with nothing typed.
The `keyboard` example in argent-device-interact passed `{ text, key: "enter" }`
as a single call, and the secrets example under it did the same — the shape the
tool now rejects. Show the one-action call, state the rule once, and point at the
`run-sequence` batching the skill already documents in § 8 for the two-step form.
hubgan
changed the title
Collaborator
Author
|
Fixed in 99939f9 — agreed, so the combination is gone rather than ordered.
The machinery that only existed to make such a call safe went with it: the Android keycode-resolver split, the hoisted text check next to it, vega's duplicate pre-resolve, and all four backends' "key after text" comments. "Type, then submit" is two calls, which Re-verified on Pixel_3a (API 36), iPhone 16 Pro and Chrome 150, against a field holding #580 and #581 are rebased on top: |
…submit
Review of the agent-facing surfaces:
- The rule was stated twice in the tool description four lines apart, and again
in the `run-sequence` example title that the two-step JSON below already
demonstrates. Each surface now states it once.
- The remedy examples used `{ text: "…" }`. U+2026 is outside printable ASCII,
which the Android backend rejects — an agent copying the placeholder verbatim
would have traded this error for `KEYBOARD_CHARACTER_UNSUPPORTED`. They now
show a literal string.
- The error says "nothing was typed", so a caller retries instead of first
inspecting the field to find out how much of the request landed.
- `key`'s parameter description no longer prescribes "type first, then press the
key": that order is right for `enter` and wrong for `backspace`, and the
ambiguity is exactly what the rejection removes.
- Restored the platform column's alignment in the `run-sequence` args table.
One real consequence of forcing two calls, found in the same pass: the MCP layer
suppresses the auto-screenshot when a call's args contain a `{{secret:…}}`
placeholder, so the old combined `{ text: "{{secret:PW}}", key: "enter" }`
submitted with no screenshot — whereas a bare second `{ key: "enter" }` call
carries no placeholder and gets one, of a field that may still show the value.
`containsSecretPlaceholder` stringifies args in full, so batching both steps in
one `run-sequence` keeps the skip; the `text` description and the skill's secrets
rules now say so.
Also noted in `argent-create-flow`: a recorded submit is now its own
`tool: keyboard { key: enter }` step, so the recording-polish rule has to say to
drop it (or set `submit: false`) — `type` already presses Enter, and leaving both
submits twice.
… dead A review pass over the change found the same "resolve the key, then use it 20 lines later" shape still standing in the two backends where the resolution fed a variable — while the diff had already collapsed it in the other two: - `simulator-server-keys.ts` and `platforms/chromium.ts` now resolve the named key inside the block that presses it. Hoisting it only ever bought "reject before the text lands", and text and key can no longer arrive together — the comment left behind said both things and cancelled itself. - `resolveVegaNamedKeycode` folds back into `injectVegaNamedKey`, the same way `resolveAndroidNamedKeycode` did. Its docblock advertised a caller that wanted a key name validated without pressing it; that caller was the combined-call pre-check this branch removed, and nothing else ever imported it. Two coverage holes the deleted ordering tests left behind: - Every remaining handler-level test pressed `key: "enter"`, so a backend that ignored `params.key` and pressed a hardcoded Enter was green everywhere. The positive control now presses `backspace` and asserts the TRANSPORT saw that key (HID usage 42 / `input keyevent 67` / CDP `Backspace` / `injectVegaNamedKey`), not just that `typed` echoed it back. Verified by mutation on all four. - `keyboard`'s interaction messages were asserted nowhere, so the collapsed `completedMsg` could have returned a constant. Pinned in interaction-messages.test.ts, including the startedMsg arm that renders for a request the guard is about to reject. Also: the comment on those messages claimed a successful call has "exactly one" of the two, which the legal empty request breaks; and the shortened `runType` comment had dropped the one fact worth keeping from the old one — on an Android TV target the trailing Enter is the call that fails, since `typeTv` rejects `key` unconditionally.
Collaborator
|
Independent confirmation of the ordering premise from 0.18.1 QA on a Pixel_9 / API 36 emulator — the discriminating test, since Starting from a field holding
So Android is confirmed key-before-text on 0.18.1, it reports success either way, and the shipped code for this path is byte-identical to 0.18.1's. Rejecting the combination outright (this PR) resolves the ambiguity that made it hard to call a bug. iOS ordering in the same sweep was correct (text then key). From a full-surface QA sweep of 0.18.1 (iOS 26.5, Android API 36, tvOS 26.5, Electron 42, Linux aarch64 + remote link). |
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
3 participants
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.
Problem
A
keyboardcall could carry bothtextandkey, and the two readings of that request contradict each other:{ text: "hello", key: "enter" }obviously means type, then submit.{ text: "abc", key: "backspace" }reads just as naturally as delete, then type — and under text-then-key it instead deletes a character of the text it just typed.Whichever order a backend picks, the other reading silently corrupts the field. That is why the Android backend pressing
keybeforetext(its transport arrived in 44b38d4/#449 and was not part of the cross-backend ordering fix in e45f148/#486) was hard to call a bug rather than a choice: against a field holdingXYZ,{ text: "abc", key: "backspace" }leftXYabcon Android andXYZabon the other three backends, and neither is what the caller asked for.An earlier revision of this PR aligned Android with the other three. Review pushed back on the premise — "I don't think we should even allow inputs like this" — which is the right call: there is no order that makes a combined call mean one thing.
Change
keyboardrejects a request carrying bothtextandkey(HTTP 400,InvalidToolInputError, newKEYBOARD_TEXT_AND_KEY_COMBINEDtelemetry code). The error names the replacement:execute— above the secret resolution and above the platform dispatch. A combined request therefore resolves noARGENT_SECRET_*value and reaches no device, and no backend has to defend against the shape.executeruns on every entry path (HTTP, the MCP bridge,invokeSubTool,run-sequencesteps, flowtool:steps), so there is one gate rather than five.{ text: "", key: "enter" }is rejected too. The rule stays statable as "one or the other" instead of needing an empty-string carve-out documented on both parameters.textandkeyindividually are untouched, as is a call with neither (still the documented no-op).resolveAndroidNamedKeycode(split out purely so a bad key name could reject before the text landed) folds back intoinjectAndroidNamedKey, the hoistedassertTypeableAndroidTextgoes with it, and so does vega's duplicate pre-resolve. All four backends' "key after text" comments described a request they can no longer be handed. Android keeps the text-then-key block order for symmetry with the other three.keyparameter description; thekeyboardline and the worked "type text and submit" example inrun-sequence(which shipped the combined form as the example); thekeyboardand secrets examples in theargent-device-interactskill.runType's comment inflow-actions.tsshrinks — flows already split the Enter into its own call, and now the tool is the reason why.run-sequencebatches the two calls into one round-trip, so "type, then submit" costs no extra turn.Test coverage
keyboard-key-order.test.ts→keyboard-text-key-exclusive.test.ts: for each of the four backends (ios, android, chromium, vega, picked by udid shape through the tool's realexecute), a combined call rejects with the new code and nothing is injected — no keys, and not even a service resolved. Each is paired with a positive control that drives text-alone and key-alone through the same harness, so the rejection test cannot pass vacuously and a guard widened to reject either half on its own turns red. Plus: the empty-half shape, rejection before secret resolution, and the message naming the two-call replacement.Six tests in
keyboard-android.test.tsdrove combined calls straight at the backend. They now pin an unreachable request, so they are gone; what they were really worth is kept — the key-identity check (a resolver returning a hardcoded 66) now loops every named key throughinjectAndroidNamedKeyand asserts the exactinput keyevent <code>, which also covers the 23 names no other test presses.Live verification
Pixel_3a (API 36), iPhone 16 Pro (iOS 18.3) and Chrome 150, driven over the tool-server's HTTP edge (the same one the MCP bridge posts to), against a focused field holding
XYZwhose value is read back withdescribe:{ text: "abc", key: "backspace" }XYabc❌ (backspace ate the pre-existingZ)XYZabXYZabXYZ— untouched ✅XYZ— untouched ✅XYZ— untouched ✅The released-build rows are the negative control: the same harness that reports 400 here accepts the call there, so the rejection is this branch's guard and not something else in the setup.
Then, on this branch, on all three platforms:
{ text: "abc" }→XYZabc;{ key: "backspace" }→XYZab(the key lands on the text just typed, which is the whole point);{ key: "enter" }→ the page records theEnterkeydown; arun-sequenceof[{ text }, { key: "enter" }]→ both steps complete; arun-sequencewith one combined step →completed: 0, the step error is the 400 message, and the field is unchanged.Local checks:
npm test -w @argent/tool-server(3074 passed, 1 skipped),npm run lint,npm run build,npm run typecheck:tests -w @argent/tool-server,prettier.Stack
#580 (
clearon the keyboard tool) and #581 (clearon the flowtypedirective) are rebased onto this.clearstill combines with either half —{ clear: true, text: "…" }replaces a field's value in one call — just not with both at once, so their three "orders clear → text → key" tests now pinclear → key(the combination nothing else covered) and their docs drop the trailingkey: "enter".