4 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	c821a463b0	fix(e2e): pin the absence of an npm postinstall hook (#556) ... Found while running the full E2E harness against the packed 0.17.0 bundle. ## Problem The `install` phase asserted that `scripts/postinstall.cjs` ships in the package: ```bash if [ -f "$pkg/scripts/postinstall.cjs" ]; then ... else fail "$P" postinstall init-hint "postinstall.cjs not shipped" fi ``` But the postinstall was deliberately dropped in #510 ("drop the npm postinstall - its jobs move to runtime"), and neither the file nor a `postinstall` entry in `package.json` exists any more. So this check failed on every run, and since the harness exits non-zero on any failure, the release gate was red for a non-issue - which also buries any real failure in the noise. ## Change Pin the intended state instead of the old one: assert the package ships neither `scripts/postinstall.cjs` nor a `postinstall` entry in the shipped `package.json`, so the hook can't creep back in. Installs that pass `--ignore-scripts` (pnpm, hardened CI) skip postinstall silently, which is why #510 removed it in the first place. ## Verification - The assertion discriminates across every input it has to separate: no `postinstall` key and no `scripts` key at all pass; an injected `.scripts.postinstall`, a present `scripts/postinstall.cjs`, a missing `package.json` and a malformed one each fail, with the manifest cases reporting that rather than "no hook". `jq -e` exits non-zero for an unreadable file just as it does for an absent key, so the manifest is parsed first — otherwise this absence check would pass whenever it could not look. - Full `install` phase re-run against `swmansion-argent-0.17.0.tgz`: no failures, harness exit code 0 (it was one failing check before). ## Not covered here The deleted check was the only assertion touching #510's postinstall. Its init-hint banner went away with the hook, so nothing is left uncovered there; its other job — killing a stale tool-server so a freshly installed binary takes effect — moved to runtime in #510 and this phase does not exercise that. Worth its own check, but it is a new test rather than a repair of this one.	2026-08-17 13:23:38 +02:00
Hubert Gancarczyk	afd722924f	feat(flow): support concurrent flow recordings (#574) ... ## Problem Only one flow could be recorded at a time, anywhere on the machine. Recording state was three module globals in `flow-utils.ts` — harmless in a per-client process, but the tool-server is a **host-wide singleton per install bundle**. Every MCP client, every subagent sharing a parent's connection, every `argent` CLI call and every project on the machine attaches to the same process, the same `Registry`, and those same three globals. HTTP requests are served concurrently with no serialization. Three failures followed, all reproduced against the pre-change code: 1. **A second recording clobbered the first.** `startRecordingSession` just overwrote, so the first agent's next `flow-add-step` silently appended into the *other* agent's flow file — in a different project. 2. **Any agent could finish any agent's recording.** `flow-finish-recording`'s schema was literally `z.object({})`. 3. **Replay stomped recording.** `resolveFlowFilePath` called `setActiveProjectRoot`, so a `flow-execute` in project B rebound the global root mid-recording of project A. Already worked around, locally only, in `flow-add-step`. ## Approach Recording state is now a `Map` keyed by the resolved flow file path, `<project_root>/.argent/flows/<name>.yaml` — the identity of the artifact being built, so "same key" means "same output file" and two sessions on one key are a genuine collision rather than an accident of scoping. `flow-add-step`, `flow-add-echo` and `flow-finish-recording` each take `name` + `project_root`, so every call is self-contained; required rather than optional-with-fallback, since a fallback would succeed while an agent is alone and fail only once a second recording exists. `resolveFlowFilePath` is pure, which fixes defect 3 and retires `FLOW_PROJECT_ROOT_REQUIRED` (the state it guarded can no longer exist). Rejected alternatives — keying by device udid (flows are deliberately device-portable, and `flow-add-echo`/`flow-finish-recording` have no device at all), an opaque `recordingId` (an agent that loses the token cannot recover), and per-caller identity from the transport (`ToolContext` carries no caller identity, and subagents share their parent's MCP connection, so it would fail the exact scenario being fixed). ### Locking, and why readers needed more than a lock Every mutation of one flow file runs under a lock keyed by that path — appends, `flow-start-recording`'s truncate-and-register, and `flow-finish-recording`'s read-summarize-clear. Keyed by file rather than owned by the session, because a restart *replaces* the session: a session-owned lock cannot exclude the operation that supersedes it. Per file, not global, so two recordings never queue behind each other. An append whose session was finished, restarted or evicted meanwhile fails with `FLOW_NO_ACTIVE_RECORDING` rather than writing into a different take. The lock serializes writers only, and no reader of a flow YAML can join it — `flow-execute`'s own load, its `run:` fragment load, `flow-read-prerequisite`, `flow-add-step`'s sibling-fragment check, and the `argent` CLI reading from another process entirely, where an in-process lock cannot reach. Both writers used a plain `fs.writeFile`, which opens `O_TRUNC`, so a reader could land between the truncate and the write. That case is silent: `parseFlow("")` returns `{ steps: [] }` with no error and `summarize` derives `ok` from "no failures", so a `flow-execute` racing an append reports a **top-level PASS having replayed zero steps**. Both writes now go through a sibling temp file and a rename, so a reader sees either the whole old file or the whole new one — and unlike the lock, that also holds cross-process. ### `stop-all-simulator-servers` gets a `devices` scope Every agent is told to call this at session end, and unscoped it walked the whole registry — so agent A wrapping up tore down agent B's devtools mid-recording, degrading B's flow to brittle coordinate taps. It now takes an optional `devices` scope and reports `unmatched` ids, so a mistyped id doesn't read as a clean machine. Which namespaces count as "owned by a device" turned out to be the load-bearing part, because nothing cascades to most of them and `unmatched` turns any omission into an actively wrong diagnosis — a correct device id reported as a typo. The set now covers `AXService` (the in-sim ax daemon, spawned `--timeout 3600`; an iOS session that only ran boot/launch/describe owns this and nothing else), `ScreenRecordingSession` (an ffmpeg child plus the touch-visualizer overlay it enabled on the device), `NativeProfilerSession` (an xctrace child, or an on-device perfetto process and its trace file) and `JsRuntimeDebugger` (a bound loopback server, the CDP socket to Metro, a log handle). The debugger URNs interpose the Metro port — `<ns>:<port>:<deviceId>` — so the matcher understands both shapes, consuming only the first colon so a wireless adb serial after the port still compares whole. `stop-simulator-server` shared none of this and had drifted: it looked URNs up with an exact, case-sensitive `services.get()`, so a lower-cased UDID silently no-op'd there while the scoped stop-all reaped it. Both tools now share one matcher. Their namespace *sets* stay deliberately different, documented where they are defined — `stop-simulator-server` is also the documented recovery for a wedged transport, and widening it to devtools/AX would make a routine retry drop the native-devtools connection another agent's recording depends on, which is the hazard the `devices` scope exists to prevent. ## Verification **End-to-end, three devices, one shared tool-server** (iPhone 16 Pro, iPhone 17 Pro, Pixel 3a): three interleaved recordings, plus an unrelated `flow-execute` in a fourth project while all three were live. Both iOS devices held their own live native-devtools concurrently (two per-UDID sockets, `connected: true`). Each YAML contained exactly its own steps in order, taps captured as selectors rather than coordinates, and neither picked up another's or the third root's path. All three replayed green — also through the **released 0.17.0 CLI** against the modified server, which checks wire compatibility. A device-scoped teardown of sim 1 left sim 2 and the emulator running. Re-run against the final build: - Two interleaved recordings across two project roots on one device, with a **separate process** polling one of the flow files as fast as it could for the duration. Zero empty or truncated observations; each YAML held exactly its own steps; both replayed green (6/6 and 5/5). Against the pre-change in-place write the same reader observes zero-step reads interleaved among the real ones — the silent-PASS input. - A scoped teardown of the iPhone reaped `AXService` and `NativeDevtools` for that UDID only, reported no `unmatched`, and left the second sim and the emulator running. Lower-casing the UDID matched; a genuinely bogus id was still reported. - With a screen recording live on the Pixel 3a, a scoped teardown reaped `ScreenRecordingSession` and the **ffmpeg child actually exited** (1 → 0). Before the fix it survived to the 180 s cap while the tool called the correct serial a typo. The pre-change repros were re-run afterwards to confirm observed behavior changed, not just the tests. `npx vitest run` in `packages/tool-server` (295 files, 3154 tests) and `packages/argent-cli` (280), `tsc --build`, `tsc --noEmit` on both test projects, eslint and prettier — all green. ## Review notes Several rounds of review found real defects in this change; each was reproduced before fixing and re-verified after. Worth knowing when reading the diff, since the tests exist to pin them: - `flow-start-recording` truncated the `.yaml` outside any lock, so a step from the take being discarded landed in the freshly reset file and was reported as success (26-29 of 200 runs). - `flow-finish-recording` never took the lock; its `await fs.readFile` was a yield during which a concurrent append committed, leaving the reported summary disagreeing with disk (11-16 of 200). - A failed finish destroyed the recording. Hand-editing the `.yaml` mid-recording is a documented workflow, so `parseFlow` can legitimately throw — and the error told the agent to call `flow-start-recording`, which truncates the very take it would recover. - `unmatched` reported a device this session had just stopped, because `disposeService` returns a node to IDLE and keeps it. - A bare IP claimed every wireless-adb device at that address, since an adb serial is itself `ip:port`. - The eviction backstop stamped with `Date.now()`; ties at millisecond resolution let it drop the session that was just used. - `requireRecordingSession` ended with "Call flow-start-recording first" — reached for a key that was finished, superseded or evicted as well as one never started, and on those branches the file on disk is fully populated while that call truncates it and reports no `restarted`. - The atomic write regressed twice on its own terms: its scratch name was derived from the flow file's basename, which has no length cap, so a long flow name that appended fine before failed `ENAMETOOLONG`; and the cleanup guard began after the temp file was created, leaking a scratch file into a committed directory on any write that failed after opening. - Two tests passed against implementations they claimed to reject: the `stop-simulator-server` narrowness guard used a udid that classifies as *android*, where the iOS-only services it guards can never appear, so widening the iOS branch kept it green; and the client-mode finish asserted only the *shape* of `savedTo`, never its content, though that directive is the only thing that lands the file in client mode. ## Notes for reviewers - Client/remote persist mode is the least-exercised path, and now has its own concurrency tests (overlapping appends, and a restart superseding an in-flight one) — it previously had none. The `project_root` key is byte-stable across the file boundary (`kind: "probe"` passes it through unchanged), which is what keeps a remote recording addressable from `flow-add-step`, which declares no file input. - Two spellings of one flow file still mint two sessions — and two independent locks — over one file, which bypasses every guarantee here. Two ways in: the root spelled two ways (a symlink, or a case variant on APFS), and the likelier one, the *name* cased two ways on a case-insensitive volume. Neither is normalized away because the correct normalization is the filesystem's: case-folding the key would wrongly merge two distinct flows on ext4, and resolving symlinks is impossible in client mode, where the root does not exist on this host. Documented at `getFlowPath`. - No recording tool can detect that another agent took its key. `flow-add-step`, `flow-add-echo` and `flow-finish-recording` all re-resolve the key on every call, so after a takeover they act on whichever take is now live and report success: an append lands in the other agent's file, and a finish finishes and clears it. The only case that fails loudly is the narrow one the liveness check covers - an append already IN FLIGHT when the restart lands. There is no caller identity to key ownership on (see the rejected alternatives above), so this is spelled out for the agent under "Pick a name unique to your task" in `argent-create-flow` rather than mechanized. - Skew hazard in one direction only: a *new* client against an *older* tool-server. Zod strips the undeclared `name`/`project_root`, and the old server falls back to its module globals — so concurrent recordings silently collapse onto one session. The other direction (released 0.17.0 CLI against this server) was verified end-to-end. - `packages/argent-private/docs/reference.md` still describes `flow-add-step` as appending to "the current recording", and names two tool ids that do not exist (`flow-insert-echo`/`flow-run` are file names; the ids are `flow-add-echo`/`flow-execute`). It is a submodule, so the fix cannot ride on this branch. - A restart reported `discardedSteps` from the superseded session's in-memory flow while it truncates the *file*. Hand-editing the `.yaml` mid-recording is documented, so the two diverge: four steps wiped, one reported. It now counts the file, and reports no number at all when that file cannot be read or parsed - 0 is the answer a genuinely empty take gives. Client mode still counts from memory (this host cannot see the client's file), and the description now says which mode it is promising. - `ChromiumJsRuntimeDebugger` cascades from `ChromiumCdp` and was therefore already being torn down - silently, while `NetworkInspector` and `ReactProfilerSession` cascade the same way and were named. `stopped` is documented as the services that were live and got shut down, so it is listed now too. - The client-mode case named for a superseded in-flight append restarted *between* calls, so the next append re-resolved the key and succeeded - the guard was never reached, and neutering it left the whole file green. It now parks an append in its live sub-tool call across the restart. - `stop-tools.test.ts`' mock handed the tool the live service map where the real `getSnapshot` copies, so a cascade rewrote the state the sweep was still reading and the answer depended on map insertion order. The mock copies and recurses like `_teardown` now, and the cascade case runs under both orders. - Prose the code contradicted, each verified against source before rewriting: Vega owns `JsRuntimeDebugger`/`NetworkInspector` once the debugger has run (and can match anything at all through an ERROR node); "nothing here cascades" was denied twice in its own file; an iOS boot/launch/describe session owns `NativeDevtools` too; `:tcp` is mintable but unreached in production; the tool-server is a singleton *per install bundle*, not per machine; `argent flow list` does enumerate `.argent/flows`; the stop-tools comments narrated a "before" that only existed on this branch. Found while verifying, out of scope, worth their own issues: a `tool: flow-execute` self-reference recurses past both cycle guards (`run:` is guarded, a nested `flow-execute` reseeds the stack); react-profiler artifacts collide host-wide on a second-resolution filename; `stop-metro` is port-scoped and kills another agent's bundler; `debugger-reload-metro`'s HTTP fallback broadcasts to every attached client. A tool's `capability` is enforced only by the HTTP layer, not by `registry.invokeTool`, so a call that reaches the registry another way - `flow-add-step` takes `command` as a bare string - can resolve a service the tool declares it does not support, leaving an ERROR node behind (e.g. `SimulatorServer:<vega serial>`). --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>	2026-08-10 10:54:58 +02:00
Ignacy Łątka	cd4190b80e	Full tgz-only E2E harness (scripts/e2e-full) (#495) ... Now verified end-to-end on the real Mac: install/introspection/validation, the android tier (against a booted emulator), and the chromium tier all pass from a locally packed tgz; the rn tier auto-skips without `~/dev/bluesky` as designed. ## What this is A release-gating end-to-end harness under `scripts/e2e-full/` that starts from **only a `swmansion-argent-*.tgz` bundle** and exercises the whole product: the install flow, every CLI command, all 70 tools' argument validation, and a happy-path run of every tool against real devices. Intended to be run on the real Linux box and the real Mac before a release. Everything runs under a throwaway `HOME` + npm prefix, so it never touches the machine's real `~/.argent`, editor MCP configs, or global packages (safe on a shared box). It emits a JSONL log + a markdown coverage report and exits non-zero if any hard assertion fails. ## Phases | phase | needs | covers | |---|---|---| | `install` | npm + network | `npm i -g <tgz>`, bundled binaries, `init` (global + `--local`), `update`, `uninstall`, telemetry, MCP-config generation | | `introspection` | — | `--version/--help`, `tools describe` for **all 70** tools, feature flags, `server start/status/logs/stop`, `link/unlink` | | `validation` | — | per tool: missing-required / bad-enum / bad-type rejection (schema-driven, no hardware) | | `android` | Android emulator | happy-path of every touch/gesture/screenshot/app-lifecycle tool | | `chromium` | Electron (bundled optional dep) + a display | boots a generated Electron app; drives CDP tools (scroll/drag/tabs/cookies/storage) | | `rn` | `~/dev/bluesky` + Android device | debugger + react/native profiler + network chain against the real Bluesky app | Tiers auto-skip with a recorded reason when prerequisites are missing. iOS / tvOS / Vega are intentionally out of scope for now. ## What I actually verified (real runs, Linux) - **install** 18✓/0✗, **introspection** 85✓/0✗ (all 70 `tools describe`), **validation** 81✓/0✗, **chromium** 24✓/0✗ (real Electron app + CDP). - Holistic run: **190✓ / 0✗ / 34∼, exit 0.** Failure-gate confirmed (non-zero on fail), `bash -n` clean on all files, all 72 android/rn JSON arg literals linted. ## What I did NOT verify live (please scrutinize) - **`android` and `rn` tiers were never run against a live device.** The Android emulator would not stay up on my box (memory-constrained shared machine; QEMU threads hung seconds after boot across ~7 attempts). The code is complete and uses the same machinery the `chromium` tier exercised live, but it has **not** been proven end-to-end. This is the main reason for the "not certain how well it works" caveat. - The RN tier's exact debugger/profiler arg shapes are derived from `tools describe`, not confirmed against a running Metro. ## Run it ```bash bash scripts/e2e-full/run-e2e.sh # everything applicable to this OS bash scripts/e2e-full/run-e2e.sh --skip-install --phase introspection,validation # fast offline core bash scripts/e2e-full/run-e2e.sh --phase android --android-serial emulator-5554 # inject a device ```	2026-07-20 16:43:34 +02:00