mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
20b5e8b4f0
## 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>
110 lines
5.4 KiB
TypeScript
110 lines
5.4 KiB
TypeScript
import { describe, expect, it, vi, beforeEach } from "vitest";
|
|
import type { DeviceInfo, Registry } from "@argent/registry";
|
|
import { UnsupportedOperationError } from "../src/utils/capability";
|
|
|
|
// The only collaborator: resolving the focus-driven TV control backend. Stub it
|
|
// so the `key` rejection can be observed as "never reached the service".
|
|
const { resolveTvApi } = vi.hoisted(() => ({ resolveTvApi: vi.fn() }));
|
|
vi.mock("../src/tools/tv/tv-service", () => ({ resolveTvApi }));
|
|
|
|
import { typeTv } from "../src/tools/keyboard/platforms/tv";
|
|
|
|
const APPLE_TV: DeviceInfo = { id: "TV-UDID", platform: "ios", kind: "simulator" };
|
|
const ANDROID_TV: DeviceInfo = { id: "emulator-5554", platform: "android", kind: "emulator" };
|
|
|
|
const registry = {} as Registry;
|
|
|
|
// `typeTv` was reachable only through `vi.mock` stubs — `keyboard-android.test.ts`
|
|
// replaces the whole module — so nothing exercised the real one. Deleting its
|
|
// `key` rejection outright (plus the now-unused import) typechecks and, with
|
|
// this file removed, leaves the rest of the suite byte-identical: same file and
|
|
// test counts, nothing red. Re-run it that way rather than trusting a recorded
|
|
// total — the count moves with every commit, so a stale figure reads as a
|
|
// failed reproduction.
|
|
//
|
|
// That guard is load-bearing beyond its own file. `flow-actions.ts`'s `runType`
|
|
// splits "type, then submit" into two keyboard calls, and on an Android TV the
|
|
// SECOND of them is the one that fails here — the text lands, the submit 400s.
|
|
// If the guard degraded to ignoring `key` instead of throwing, `keyboard
|
|
// { udid: <Apple TV / Android TV>, key: "enter" }` would answer
|
|
// `{ typed: "", keys: 0 }` — a success that pressed nothing — and that
|
|
// directive's submit would silently no-op on a still-filled field.
|
|
//
|
|
// The tool's own text/key exclusivity guard never reaches this backend, so it
|
|
// cannot stand in for this one: it runs above the platform dispatch and only
|
|
// sees requests carrying BOTH parameters, while what a TV rejects is `key` on
|
|
// its own. That is why the exclusivity message carries the TV caveat statically
|
|
// (keyboard-text-key-exclusive.test.ts) instead of relying on this rejection.
|
|
describe("typeTv — the TV keyboard backend", () => {
|
|
beforeEach(() => {
|
|
vi.clearAllMocks();
|
|
});
|
|
|
|
it.each([
|
|
["Apple TV", APPLE_TV],
|
|
["Android TV", ANDROID_TV],
|
|
])("rejects a named key on %s, before resolving the TV service", async (_label, device) => {
|
|
await expect(
|
|
typeTv(registry, device, { udid: device.id, key: "enter" })
|
|
).rejects.toBeInstanceOf(UnsupportedOperationError);
|
|
await expect(typeTv(registry, device, { udid: device.id, key: "enter" })).rejects.toThrow(
|
|
/named keys are not supported on a TV target/
|
|
);
|
|
// Rejected outright, not after doing something: no backend was resolved.
|
|
expect(resolveTvApi).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("rejects the key BEFORE typing any text it was also handed", async () => {
|
|
// The tool rejects `{ text, key }` above the dispatch, so this shape does
|
|
// not arrive from the tool — `blueprints/android-tv-control.ts` is the other
|
|
// caller of this backend. What it pins is the ORDER inside `typeTv`: the
|
|
// rejection comes first, so a request it refuses leaves the field untouched
|
|
// rather than half-typed.
|
|
await expect(
|
|
typeTv(registry, ANDROID_TV, { udid: ANDROID_TV.id, text: "hello", key: "enter" })
|
|
).rejects.toThrow(/named keys are not supported on a TV target/);
|
|
expect(resolveTvApi).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("types text alone through the TV service (positive control)", async () => {
|
|
// Without this the rejection tests above could pass against a backend that
|
|
// does nothing at all.
|
|
const type = vi.fn(async () => {});
|
|
resolveTvApi.mockResolvedValue({ type });
|
|
|
|
// "Hi there", not "hi there": an all-lowercase fixture cannot separate
|
|
// "hands the service the text it was given" from "hands it a case-folded
|
|
// copy", and `typed` echoes the request rather than what `type` received.
|
|
const result = await typeTv(registry, ANDROID_TV, { udid: ANDROID_TV.id, text: "Hi there" });
|
|
|
|
expect(type).toHaveBeenCalledWith("Hi there");
|
|
expect(result).toEqual({ typed: "Hi there", keys: 8 });
|
|
});
|
|
|
|
it("counts `keys` by codepoint, not UTF-16 unit", async () => {
|
|
// Matches the vega and simulator-server backends: a non-BMP char is one key.
|
|
//
|
|
// APPLE_TV, not ANDROID_TV: the Android TV service runs
|
|
// `assertTypeableAndroidText` (`blueprints/android-tv-control.ts:256`),
|
|
// which 400s an emoji, so on a real Android TV this call cannot reach the
|
|
// asserted result — the stubbed `resolveTvApi` is the only reason it does.
|
|
// Apple TV rejects newlines alone (`blueprints/tv-control.ts:468-474`), so
|
|
// the case is genuinely reachable there. The property itself is unaffected:
|
|
// `typeTv` computes `[...text].length` before either service is involved.
|
|
const type = vi.fn(async () => {});
|
|
resolveTvApi.mockResolvedValue({ type });
|
|
|
|
const result = await typeTv(registry, APPLE_TV, { udid: APPLE_TV.id, text: "A😀" });
|
|
|
|
expect(type).toHaveBeenCalledWith("A😀");
|
|
expect(result).toEqual({ typed: "A😀", keys: 2 });
|
|
});
|
|
|
|
it("no-ops on an empty request without resolving the service", async () => {
|
|
const result = await typeTv(registry, ANDROID_TV, { udid: ANDROID_TV.id });
|
|
|
|
expect(result).toEqual({ typed: "", keys: 0 });
|
|
expect(resolveTvApi).not.toHaveBeenCalled();
|
|
});
|
|
});
|