Files
software-mansion__argent/packages/tool-server/test/vega-injection.test.ts
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

143 lines
6.5 KiB
TypeScript
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
import { describe, it, expect, vi, beforeEach } from "vitest";
import { FAILURE_CODES, getFailureSignal } from "@argent/registry";
// Stub the adb round-trip (capture the shell command strings) but keep the real
// `shellQuote` so the text-injection quoting is exercised, not mocked away.
const adbShell = vi.fn();
vi.mock("../src/utils/adb", async (importActual) => {
const actual = await importActual<typeof import("../src/utils/adb")>();
return { ...actual, adbShell: (...args: unknown[]) => adbShell(...args) };
});
// Single VVD: the input path derives its serial from the emulator console port.
vi.mock("../src/utils/vega-automation", () => ({
emulatorSerial: vi.fn(async () => ({ serial: "emulator-5554", consolePort: 5554 })),
}));
import {
NAMED_KEYCODES,
injectVegaButtons,
injectVegaNamedKey,
injectVegaText,
} from "../src/utils/vega-input";
// Real device output (verified on a VVD): get_screen_size prints this when
// developer mode is ON; when OFF the dev-shell service is down and every
// inputd-cli command (get_screen_size included) returns the error below.
const SIZE_OK = "1920 x 1080";
const DEV_SHELL_DOWN = "Error: No running instances of com.amazon.dev.shell.service found";
/** The single adb shell script for the most recent injection. */
function lastScript(): string {
return adbShell.mock.calls.at(-1)?.[1] as string;
}
/** Await a promise expected to reject and return the thrown Error. */
async function captureError(p: Promise<unknown>): Promise<Error> {
try {
await p;
} catch (e) {
return e as Error;
}
throw new Error("expected the promise to reject, but it resolved");
}
beforeEach(() => {
adbShell.mockReset();
// Default: developer mode on, channel live.
adbShell.mockResolvedValue(SIZE_OK);
});
describe("injectViaInputd — developer-mode / liveness gate", () => {
it("runs the presses when get_screen_size reports a live channel", async () => {
await expect(injectVegaButtons(["down", "select"])).resolves.toBeUndefined();
const script = lastScript();
expect(script).toContain("inputd-cli get_screen_size");
// Presses are gated behind the size-shape `case` so a dead channel fails fast.
expect(script).toContain('case "$sz" in *[0-9]*x*[0-9]*)');
expect(script).toContain("button_press KEY_DOWN >/dev/null 2>&1 || true");
expect(script).toContain("sleep 0.3");
// Path order preserved: down before select(=ENTER).
expect(script.indexOf("KEY_DOWN")).toBeLessThan(script.indexOf("KEY_ENTER"));
});
it("fails with an actionable, classified error when developer mode is off", async () => {
adbShell.mockResolvedValue(DEV_SHELL_DOWN);
const err = await captureError(injectVegaButtons(["down"]));
expect(err.message).toMatch(/developer mode is off/i);
expect(err.message).toContain("vsm developer-mode enable");
// Classified for telemetry, not a bare 500.
expect(getFailureSignal(err)?.error_code).toBe(FAILURE_CODES.VEGA_INPUT_UNAVAILABLE);
});
it("uses the generic channel error (not the dev-mode hint) for an unrelated dead channel", async () => {
adbShell.mockResolvedValue(""); // no <W>x<H>, no dev-shell signature
const err = await captureError(injectVegaButtons(["down"]));
expect(err.message).toMatch(/input channel is not usable/i);
expect(err.message).not.toMatch(/developer mode/i);
expect(getFailureSignal(err)?.error_code).toBe(FAILURE_CODES.VEGA_INPUT_UNAVAILABLE);
});
});
describe("injectVegaText", () => {
it("rejects embedded newlines (send_text would truncate the tail)", async () => {
await expect(injectVegaText("line1\nline2")).rejects.toThrow(/newline/i);
// Guard runs before any device round-trip.
expect(adbShell).not.toHaveBeenCalled();
});
it("shell-quotes the text so a quote/space can't break out of the command", async () => {
// Capital "I" so the fixture also pins the case at the `send_text` sink:
// with an all-lowercase string a `.toLowerCase()` here emits the identical
// command, and the keyboard backend's `typed` echoes the request rather
// than what was sent — the same hole the android `input text` test names.
await injectVegaText("It's a test");
expect(lastScript()).toContain("send_text 'It'\\''s a test'");
});
});
describe("injectVegaNamedKey", () => {
it("maps a known key to its KEY_ code", async () => {
await injectVegaNamedKey("enter");
expect(lastScript()).toContain("button_press KEY_ENTER");
});
it("presses each named key with its own keycode (not one hardcoded value)", async () => {
// Half of a two-part scheme, and vacuous without the other half: this
// compares the emitted code against the same map the resolver reads, so it
// pins that the lookup READS the map, for every entry in it — not that any
// entry holds the right value. vega-input.test.ts pins all 24 literals; it
// used to spot-check 8, which left a wrong value for `backspace`, `delete`,
// `tab`, `space`, three of the arrows or f2–f10 satisfying both tests.
//
// What this half catches is a lookup that resolved every name to one code —
// the realistic fold/refactor slip. Only `enter` and `arrow-down` are
// pressed by other tests in this file, so such a fold is green everywhere
// else while `backspace`, `escape`, `tab` and f1–f12 all submit the field
// instead of navigating. Twin of the android exhaustive test.
for (const [name, keycode] of Object.entries(NAMED_KEYCODES)) {
adbShell.mockClear();
adbShell.mockResolvedValue(SIZE_OK);
await injectVegaNamedKey(name);
expect(lastScript().match(/button_press \S+/g), `wrong keycode for "${name}"`).toEqual([
`button_press ${keycode}`,
]);
}
});
it("case-folds the named key, like every other backend", async () => {
// `keyboard`'s `key` is a free `z.string()`, and the sim-server/android
// backends fold case, so an uppercase name must not read as unknown here.
await injectVegaNamedKey("Arrow-Down");
expect(lastScript()).toContain(`button_press ${NAMED_KEYCODES["arrow-down"]}`);
});
it("names the offending key when it is unknown, instead of dropping it", async () => {
// f1–f12 are mapped; f13 is the first out-of-range function key.
// The NAME is part of the contract — it is what a caller needs to retry, and
// a bare `/Unknown Vega key/` prefix leaves stripping it green here, which
// is the only place this backend's message is asserted at all.
await expect(injectVegaNamedKey("f13")).rejects.toThrow(/Unknown Vega key "f13"/);
expect(adbShell).not.toHaveBeenCalled();
});
});