2 Commits

Author	SHA1	Message	Date
Hubert Gancarczyk	20b5e8b4f0	fix(tool-server): reject a keyboard call carrying both text and key (#579) ... ## Problem `keyboard` accepted `text` and `key` in one call, and that shape has no meaning a caller can rely on. The same request reads two ways: - `{ text: "hello", key: "enter" }` — "type, then submit" - `{ text: "abc", key: "backspace" }` — "delete, then type" reads just as naturally Whichever order a backend picks, the other reading silently corrupts the field. The backends did not even agree: `typeAndroidPhone` pressed `params.key` **before** `params.text`, while simulator-server, chromium and vega all did the reverse and the tool's own description promised "the text is typed first and the key is pressed after it". ## Change **`keyboard` takes `text` or `key`, never both.** A combined call is rejected in `execute`, above the platform dispatch, so it resolves no `ARGENT_SECRET_*` value and reaches no device — no backend has to defend against the shape. **To mix text and a key, use `run-sequence`.** That is what the error message, both parameter descriptions, the tool description and the skill docs now say: ```json { "udid": "<UDID>", "steps": [ { "tool": "keyboard", "args": { "text": "hello world" } }, { "tool": "keyboard", "args": { "key": "enter" } } ]} ``` Two separate calls work too, but cost an extra round-trip — and are **not** equivalent when the text carries a secret (see below). ### Why the rule is prose, not JSON Schema An earlier revision hand-wrote `inputSchema` with `not: { required: ["text", "key"] }`. That is no longer legal: #782 banned every top-level combinator repo-wide, `not` included, because the Messages API rejects a request whose tool schemas declare one — and that 400 fails **every tool in the request**, not just this one. `tool-input-schema-contract.test.ts` enforces it across the catalog. So the constraint is restated in **both** fields' `.describe()` (the `boot-device` house rule) and in the tool description. `keyboard-text-key-exclusive.test.ts` pins both texts, and pins that the advertised schema stays combinator-free. ### What the error message carries Three things a caller needs and cannot get elsewhere: 1. **The literal retry**, not an ellipsis the Android backend cannot type. 2. **The TV caveat.** A TV target rejects `key` outright (`platforms/tv.ts`), so the prescribed `{ key: "enter" }` step cannot succeed there. The guard runs above the dispatch on purpose, so it would otherwise swallow that diagnosis; the caveat is carried statically rather than by an async TV probe. 3. **The secret steer**, only when `text` holds a `{{secret:…}}` placeholder. The auto-screenshot skip is decided per tool call from the whole request, so: | call shape | auto-screenshot | | --- | --- | | one `run-sequence` carrying both steps | skipped | | `{ text: "{{secret:…}}" }` alone | skipped | | a separate `{ key: "enter" }` | **not** skipped — captured after the key lands, and can hand the still-visible secret back as pixels | The check is the syntactic `.includes` `flow-utils.ts` already uses, so the guard still resolves nothing. A plain combined call does not carry the sentence, with a positive control pinning that. ### Simplifications the rule makes possible With at most one of the two ever reaching a backend, the resolve-then-inject splits that existed only to order a combined call fold back into their injectors (`android-input.ts`, `vega-input.ts`), and each backend collapses to two exclusive branches. ### Fixed alongside `run-sequence`'s description claimed "No screenshot is captured automatically". `shouldAutoScreenshot("run-sequence")` is `true` with a 15000 ms settle cap, so that line was already false — and this change makes it load-bearing, since the secret guidance rests on it. ## Compatibility A stored flow or scripted `run-sequence` whose step is `{ tool: keyboard, args: { text: "…", key: "enter" } }` now errors on replay, and the steps after it are skipped. The recorder never emitted the combined shape (`flow-add-step` executes the call before it appends the step), so the blast radius is hand-authored flows and scripted `run-sequence` calls — which is exactly what the old worked example taught. The fix is to split the step in two. ## Test coverage - **`keyboard-text-key-exclusive.test.ts`** (new, replaces `keyboard-key-order.test.ts`) — the rejection on all four backends with nothing injected and no service resolved, plus a positive control per backend that text alone and a key alone still work; shape-not-truthiness (`{ text: "", key: "enter" }` rejects); at-most-one rather than exactly-one (`{}` stays a no-op); rejection before secret resolution; the run-sequence retry, TV and secret sentences, with a positive control against an unconditional secret steer; and the two channels the constraint actually reaches a client through. - **`keyboard-backend-fidelity.test.ts`** — absorbs what key-order uniquely pinned. The unknown-key 400 now names the offending key on iOS and chromium here, on android in `keyboard-android.test.ts`, and on vega in `vega-injection.test.ts` (tightened from the bare `/Unknown Vega key/` prefix), so stripping the name is red on every backend. - **`keyboard-android.test.ts`** — the combined-call tests are gone. What they uniquely covered survives as single-parameter tests: the multi-segment `%` codepoint count, the pressed key's identity against a second literal keycode, and un-typeable text rejected with no on-device side effect. The exhaustive 24-keycode loop now runs through `injectAndroidNamedKey` and asserts the emitted `input keyevent <code>`, so it pins the wire and not just the lookup. - **`interaction-messages.test.ts`** — the two formatters see different shapes now. `startedMsg` renders before the rejection, so it still words four (including the combined one); `completedMsg` runs only after success, so it words three. Both empty-request branches stay pinned. - **`keyboard-tv.test.ts`** — the TV `key` rejection is independent of this guard, which only ever sees requests carrying both. Its header says so, since the exclusivity message's TV clause depends on it. - **`scripts/e2e-full/phases/30-android.sh`** — the combined call becomes an `assert_reject`, and the prescribed `run-sequence` remedy is driven end to end. ## Live verification Isolated tool-server built from this branch (own port, shared MCP server untouched), against the Android settings search field on an API 36 AVD, read back out of the view hierarchy rather than off a screenshot: | Call | HTTP | Field / result | | --- | --- | --- | | seed `{ text: "XYZ" }` | 200 | `XYZ` | | `{ text: "abc", key: "backspace" }` | **400** | field still `XYZ` — byte-identical, nothing typed | | `run-sequence` [`{text:"abc"}`, `{key:"backspace"}`] | 200, `completed: 2` | `XYZab` — exactly what the combined call used to produce | | `{ text: "hello" }` | 200 | `{ typed: "hello", keys: 5 }` | | `{ key: "enter" }` | 200 | `{ typed: "enter", keys: 1 }` | | `{}` | 200 | `{ typed: "", keys: 0 }` — still a no-op | | `{ text: "", key: "enter" }` | **400** | shape, not truthiness | | `{ text: "{{secret:APP_PASSWORD}}", key: "enter" }` | **400** | message carries the ONE-`run-sequence` steer and the still-visible-secret reason | | `{ text: "hi", key: "enter" }` | **400** | message does **not** carry the secret sentence | `GET /tools` reports `["properties", "required", "type"]` for `keyboard` — no top-level combinator. Suite: `packages/tool-server` 336 files / 4041 passed, `packages/registry` 90 passed, `packages/argent-mcp` 79 passed. `tsc --build`, `tsc --noEmit -p tsconfig.test.json`, eslint (0 warnings), prettier and knip all clean. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>	2026-08-17 15:01:23 +02:00
Ignacy Łątka	44b38d490e	fix(tool-server): inject Android keyboard/button over adb input (#449) (#455) ... ## What & why `keyboard` and `button` drive Android through the simulator-server's USB-HID transport (`pressKey` / `cmd:"button"`). The guest only receives those HID events when the AVD exposes a hardware keyboard (`hw.keyboard = yes`) — the default, but CI / headless AVDs are routinely created with `hw.keyboard = no` (and `hw.mainKeys = no`), where the guest **silently drops** them. Because that transport is fire-and-forget, both tools returned `{ typed }` / `{ pressed }` **success while injecting nothing**. This still affects current `main`: after the keyboard refactor, `makeAndroidImpl` routes Android **TV** to `adb input` but Android **phones/tablets** still to `typeSimulatorServer` (the HID path). Fixes #449 (found during 0.13.0 RC E2E testing on a `hw.keyboard=no` AVD — `argent-test-ci-repro`, the config CI uses). ## Change Route Android **phone/tablet** key / text / button injection through `adb shell input text` and `input keyevent` (new `utils/android-input.ts`), the same on-device transport Android TV and Vega already use: - **`keyboard`** — `platforms/android.ts` now sends the non-TV branch to a `typeAndroidPhone` helper (adb `input`); the Android-TV and iOS branches are untouched. - **`button`** — the Android branch presses via `injectAndroidKeycode`; iOS keeps the simulator-server HID path. `adb input` lands regardless of `hw.keyboard`, on emulators (any config) and physical devices alike, and a non-zero exit surfaces as a **thrown error** instead of a silent no-op — directly addressing the fire-and-forget concern already called out in the `button` tool. Touch gestures are unchanged. ## Verification - **Unit tests — 11/11 pass** (rebased on current `main`): new `keyboard-android.test.ts` (vocabulary parity vs `NAMED_KEYS`, `input text` shell-quoting incl. spaces, `input keyevent` mapping, empty-text no-op, newline rejection) + updated `button.test.ts` (Android `back` now asserts `KEYCODE_BACK` over adb). - **eslint** on the changed files: clean. - **Live tool-server** on a `hw.keyboard=no` AVD: the `keyboard` tool now issues `adb … shell input text '…'` (vs the RC's HID `pressKey`), and a failed inject **raises an error** rather than a fake success. The `adb input` path was independently observed typing/deleting on this exact AVD during RC testing (where the HID path silently no-op'd). - Note: a full `tsc --build` / whole-suite run couldn't be completed in my environment (the `argent-private` submodule isn't checked out, which breaks the npm workspace install, and `main` added upstream deps my base lacked). `platforms/android.ts` mirrors the existing `platforms/ios.ts` branch structure one-for-one, and the pre-rebase revision of this change passed `tsc --build` + eslint. CI here is authoritative for the full typecheck. ## Reviewer notes - `input text` receives the string as one shell-quoted argv token, so spaces are preserved verbatim; newlines are rejected (they'd truncate the `input` line) — callers use `key: "enter"`. - `keys` counts by codepoint, matching the tv/vega/simulator-server backends.	2026-07-10 12:51:24 +02:00