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>
131 lines
7.1 KiB
Bash
131 lines
7.1 KiB
Bash
#!/usr/bin/env bash
|
|
# Phase 3 — Android device tier (Linux + macOS).
|
|
#
|
|
# Drives a happy-path of every tool that applies to an Android emulator. The
|
|
# device is EITHER injected (E2E_ANDROID_SERIAL — e.g. a serial the device
|
|
# allocator handed us on a shared machine, already booted) OR booted here from
|
|
# E2E_ANDROID_AVD. If neither is available the whole tier is skipped with a
|
|
# reason so the report still makes sense.
|
|
|
|
# Verify a booted device with this serial is visible to the tool-server.
|
|
_android_present() { # serial
|
|
run_tool list-devices '{}'
|
|
printf '%s' "$RT_JSON" | jq -e --arg s "$1" 'any(.devices[]?; .serial==$s or .udid==$s)' >/dev/null 2>&1
|
|
}
|
|
|
|
# Grab a screenshot to a file and assert it has real pixels. Sets SHOT_PATH.
|
|
_shot_ok() { # phase udid case
|
|
local phase="$1" udid="$2" case="$3"
|
|
if capture_screenshot "$udid" "$E2E_WORK/android-$case.png"; then
|
|
pass "$phase" screenshot "$case (${SHOT_SIZE}B)"; return 0
|
|
else
|
|
fail "$phase" screenshot "$case" "size=${SHOT_SIZE:-0} rc=${SHOT_RC:-?} (blank framebuffer?)"; return 1
|
|
fi
|
|
}
|
|
|
|
run_phase() {
|
|
local P=android
|
|
ensure_server || { skip "$P" tier all "tool-server unavailable"; return 0; }
|
|
|
|
local DEV=""
|
|
if [ -n "${E2E_ANDROID_SERIAL:-}" ]; then
|
|
DEV="$E2E_ANDROID_SERIAL"
|
|
if _android_present "$DEV"; then
|
|
pass "$P" list-devices "injected serial $DEV present"
|
|
else
|
|
# not yet visible — try a boot-device against it in case the sim-server
|
|
# needs to attach; otherwise skip.
|
|
skip "$P" tier all "injected serial $DEV not visible to tool-server"; return 0
|
|
fi
|
|
elif [ -n "${E2E_ANDROID_AVD:-}" ]; then
|
|
log "booting AVD $E2E_ANDROID_AVD"
|
|
run_tool boot-device "{\"avdName\":\"$E2E_ANDROID_AVD\",\"bootTimeoutMs\":840000}"
|
|
if [ "$RT_RC" -eq 0 ] && printf '%s' "$RT_JSON" | jq -e '.booted==true' >/dev/null 2>&1; then
|
|
DEV="$(printf '%s' "$RT_JSON" | jq -r '.serial // .udid // empty')"
|
|
pass "$P" boot-device "booted $DEV"
|
|
else
|
|
fail "$P" boot-device "$(printf '%s' "$RT_OUT" | tr '\n' ' ' | cut -c1-160)"; return 0
|
|
fi
|
|
else
|
|
skip "$P" tier all "no Android device (set E2E_ANDROID_SERIAL or E2E_ANDROID_AVD)"; return 0
|
|
fi
|
|
local U="{\"udid\":\"$DEV\"}"
|
|
|
|
# --- discovery ------------------------------------------------------------
|
|
_shot_ok "$P" "$DEV" baseline || true
|
|
assert_field "$P" describe describe "$U" '(.description|length>0)' 'true'
|
|
assert_ok "$P" await-screen-idle idle "$U"
|
|
assert_ok "$P" await-ui-element exists-probe "{\"udid\":\"$DEV\",\"condition\":\"exists\",\"selector\":{\"text\":\"a\"},\"timeoutMs\":3000}"
|
|
|
|
# --- interaction ----------------------------------------------------------
|
|
assert_true "$P" gesture-tap tap "{\"udid\":\"$DEV\",\"x\":0.5,\"y\":0.5}" '.tapped'
|
|
assert_true "$P" gesture-swipe swipe "{\"udid\":\"$DEV\",\"fromX\":0.5,\"fromY\":0.8,\"toX\":0.5,\"toY\":0.2}" '.swiped'
|
|
assert_ok "$P" gesture-custom custom "{\"udid\":\"$DEV\",\"events\":[{\"type\":\"Down\",\"x\":0.5,\"y\":0.6},{\"type\":\"Move\",\"x\":0.5,\"y\":0.4},{\"type\":\"Up\",\"x\":0.5,\"y\":0.4}]}"
|
|
assert_true "$P" gesture-pinch pinch "{\"udid\":\"$DEV\",\"centerX\":0.5,\"centerY\":0.5,\"startDistance\":0.1,\"endDistance\":0.4}" '.pinched'
|
|
assert_true "$P" gesture-rotate rotate2 "{\"udid\":\"$DEV\",\"centerX\":0.5,\"centerY\":0.5,\"radius\":0.2,\"startAngle\":0,\"endAngle\":90}" '.rotated'
|
|
assert_ok "$P" button home "{\"udid\":\"$DEV\",\"button\":\"home\"}"
|
|
assert_ok "$P" button back "{\"udid\":\"$DEV\",\"button\":\"back\"}"
|
|
assert_ok "$P" rotate landscape "{\"udid\":\"$DEV\",\"orientation\":\"LandscapeLeft\"}"
|
|
assert_ok "$P" rotate portrait "{\"udid\":\"$DEV\",\"orientation\":\"Portrait\"}"
|
|
assert_ok "$P" keyboard text "{\"udid\":\"$DEV\",\"text\":\"hello e2e\"}"
|
|
assert_ok "$P" keyboard key "{\"udid\":\"$DEV\",\"key\":\"enter\"}"
|
|
# Both halves in ONE call is rejected, and the rejection is what the two calls
|
|
# above cannot show: each of them is a legal request, so a guard that never
|
|
# fires leaves them green. The 400 has to come from the running tool-server
|
|
# rather than from schema validation — the constraint is cross-field, so it
|
|
# lives in `execute` and nothing in the advertised JSON Schema encodes it.
|
|
assert_reject "$P" keyboard text-and-key \
|
|
"{\"udid\":\"$DEV\",\"text\":\"hello e2e\",\"key\":\"enter\"}"
|
|
# And the remedy the rejection prescribes, driven end to end. `run-sequence`
|
|
# dispatches each step through the same tool, so this is the shape that would
|
|
# break if the guard were widened to reject a step carrying only one of the
|
|
# two. `.completed` = 2 proves neither step was skipped.
|
|
assert_field "$P" run-sequence keyboard-text-then-key \
|
|
"{\"udid\":\"$DEV\",\"steps\":[{\"tool\":\"keyboard\",\"args\":{\"text\":\"hello e2e\"}},{\"tool\":\"keyboard\",\"args\":{\"key\":\"enter\"}}]}" \
|
|
'.completed' '2'
|
|
assert_ok "$P" run-sequence seq "{\"udid\":\"$DEV\",\"steps\":[{\"tool\":\"button\",\"args\":{\"button\":\"home\"}},{\"tool\":\"gesture-tap\",\"args\":{\"x\":0.5,\"y\":0.5}}]}"
|
|
|
|
# --- url navigation -------------------------------------------------------
|
|
assert_true "$P" open-url url "{\"udid\":\"$DEV\",\"url\":\"https://example.com\"}" '.opened'
|
|
|
|
# --- screenshot-diff (two live captures) ----------------------------------
|
|
local b c
|
|
_shot_ok "$P" "$DEV" diff-baseline && b="$SHOT_PATH"
|
|
cp "$b" "$E2E_WORK/diff-base.png" 2>/dev/null || true
|
|
run_tool button "{\"udid\":\"$DEV\",\"button\":\"home\"}" >/dev/null 2>&1
|
|
_shot_ok "$P" "$DEV" diff-current && c="$SHOT_PATH"
|
|
if [ -n "${b:-}" ] && [ -n "${c:-}" ]; then
|
|
assert_ok "$P" screenshot-diff diff "{\"udid\":\"$DEV\",\"baselinePath\":\"$E2E_WORK/diff-base.png\",\"currentPath\":\"$c\"}"
|
|
else
|
|
skip "$P" screenshot-diff diff "could not capture two screenshots"
|
|
fi
|
|
|
|
# --- app lifecycle against a stock system app -----------------------------
|
|
local APP="com.android.settings"
|
|
assert_true "$P" launch-app launch "{\"udid\":\"$DEV\",\"bundleId\":\"$APP\"}" '.launched'
|
|
assert_true "$P" restart-app restart "{\"udid\":\"$DEV\",\"bundleId\":\"$APP\"}" '.restarted'
|
|
|
|
# --- state-only + platform-scoped ----------------------------------------
|
|
assert_ok "$P" dismiss-update dismiss "{\"hours\":1}"
|
|
|
|
# iOS-only tools: recorded as skips so coverage is explicit.
|
|
local iostool
|
|
for iostool in native-describe-screen native-devtools-status native-full-hierarchy \
|
|
native-find-views native-view-at-point native-user-interactable-view-at-point \
|
|
native-network-logs; do
|
|
skip "$P" "$iostool" happy-path "iOS-only (not applicable on Android)"
|
|
done
|
|
skip "$P" tv-remote happy-path "TV-only tier (skipped per scope)"
|
|
skip "$P" reinstall-app happy-path "covered in RN tier with the Bluesky apk"
|
|
|
|
# --- teardown for this device --------------------------------------------
|
|
# Only stop the sim-server if we booted the AVD ourselves; a device injected
|
|
# by the allocator is released by the caller.
|
|
if [ -z "${E2E_ANDROID_SERIAL:-}" ]; then
|
|
assert_ok "$P" stop-simulator-server stop "$U"
|
|
else
|
|
run_tool stop-simulator-server "$U" >/dev/null 2>&1 || true
|
|
skip "$P" stop-simulator-server stop "injected device released by caller"
|
|
fi
|
|
}
|