mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
feat/flow-bash-scripts
8 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Mateusz Aliyev
|
6e9e45c9f0 |
feat: attach to devices offered by external providers (#735)
This pull request lets Argent drive a simulator or emulator another process is already running, attaching to its simulator-server rather than spawning a second one. A provider writes a JSON descriptor to `~/.argent/providers/` listing the devices it offers and the mechanisms Argent may use on each. They appear in `list-devices` with an `ext:` id and work with the existing tools. The file is re-read on every call, so a withdrawal or a narrowed grant applies immediately. `ios.additionalDeviceSets` (#600) already makes such a simulator reachable by UDID. What changes is ownership. `boot-device` and `stop-simulator-server` refuse these devices, anything the provider did not grant is refused with a message naming it and Argent uses only the endpoints its own simulator-server build serves. A grant binds to the device rather than to one of its names, so the real udid or serial is gated exactly like the `ext:` id. Android emulators are covered too and a device visible to both a provider and `adb`/`simctl` is listed once. The contract ships as `schemas/device-provider-v1.json`, validated by `argent providers check`, and is documented in `docs/reference/device-providers.mdx`. Includes unit tests and an e2e phase where the harness acts as its own provider. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for discovering and connecting to externally managed devices. * Added `argent providers` commands for listing, validating, publishing, withdrawing, and pruning providers. * Added provider-aware simulator, debugger, profiler, native tools, and device listings. * Added capability controls, endpoint validation, revocation handling, and provider-specific diagnostics. * Added automatic CLI discovery for provider integrations. * **Bug Fixes** * Improved service recovery and paused-runtime error reporting. * Improved simulator keyboard and paste command reliability. * **Documentation** * Documented provider descriptors, capabilities, lifecycle rules, and CLI usage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
filip131311
|
b232114422 |
docs: audit every comment in src against the code it describes (#931)
Audits every comment in `src/` and `scripts/` against the code it
describes.
**462 files, 57 commits, net −8,194 lines.** Comment text only — the
whole branch is code-identical to `main`.
## Method
One subagent per file, strictly sequential. Scope was `src/` +
`scripts/` (459 files); three more files were added at the end because
they carried dead references of the same kind — two test headers citing
design docs that do not exist, and `publish-npm.yml` citing a retired
workflow. Each agent verified every comment — line, block, JSDoc, file
header, trailing — against the surrounding code and the rest of the
repo, following identifiers, paths, tool ids, config keys, env vars and
issue links to see whether they still exist and still behave as
described. Rules:
- **A false or misleading comment is deleted, not reworded.** If a claim
could not be confirmed by reading the source, it went. That is why the
deletion count is so much larger than the rewrite count.
- Survivors are cut to the shortest form carrying something the code
does not already say. Restatement, preamble, hedging, changelog prose
and ASCII banners are gone; the non-obvious *why* stays.
- Preserved byte-identical: license headers, pragmas and directives
(`@ts-*`, `eslint-disable`, shebangs, `/// <reference>`), JSDoc tag
tokens, everything inside a string or template literal, and the sole
comment inside an otherwise empty block (ESLint `no-empty` counts a
comment-bearing block as non-empty).
## Verification
Every file passed two independent gates before being recorded as done:
1. `comments-only` — the required check.
2. A second comment-stripping comparator with a proper mode stack,
written for this pass because `comments-only`'s flat scanner desyncs on
nested template literals and quote-bearing regex literals and then
reports comment lines as code changes. Two files hit that false FAIL
(`utils/android-profiler/pipeline/index.ts`,
`scripts/extract-tools.mjs`); in both the "changed code" it printed was
literally `//` lines, and the second checker confirmed the code was
byte-identical.
After the last file, all 462 changed files were re-checked against
`main` with the same comparator, rather than trusting any agent's
self-report. **459 code-identical; 2 are non-code (`.svg`, `.md`); 1
intentional.**
The intentional one is `packages/argent/scripts/bundle-tools.cjs`: the
changed template literal *is* the comment header of the file it
generates, `packages/native-devtools-android/src/bundled-meta.ts`.
Fixing only the generated file would have been reverted by the next
build, so the generator changed too — and it has been verified to
reproduce the committed generated file byte-for-byte.
## Representative false claims removed
Not wording nits — statements a reader would have acted on:
- **Reversed directions.** `proxyStart`'s JSDoc had the tunnel backwards
(it is a reverse tunnel: the host binds first and the simulator dials
in). A `paste()` doc had the pasteboard copy direction reversed.
- **Contradicted by the code below it.** A timeout budget multiplied by
three where the probes run concurrently — the same comment said so six
lines later. A "warn once" that warns on every call. A "binary search"
that is a linear scan.
- **Named things that do not exist.** A `vega-fast-cli` binary, a
`finish-recording.ts`, a `publish-next.yml` workflow, two
`profiler-react19-*.md` design docs, a `DebuggerTarget.ts`, a commit
hash git does not know, two tool ids, an `ensureEnv` cycle.
- **Wrong by construction.** "Welford accumulators" across four files
where the code keeps naive `n`/`sum`/`sumSq`; `sum`/`sumSq` documented
over `actualDuration` when reduce sums `selfDuration`; a strict-mode
halving written `n/2` where the code ceils; field docs listing enum
values the producers never emit.
- **Guarantees the code does not make.** A validation matrix claiming to
cover "EVERY tool" that skips flagless ones; a Pareto cutoff that
`slice(0, 20)` makes inert; an idempotence claim where the real rule is
at-or-ahead; a capability note describing a clean 400 the shape-based
device resolver can never produce.
- **Unverifiable assertions** about prebuilt binaries, external CLIs and
the cloud SDK — deleted rather than kept as folklore, since nothing in
the repo can confirm them.
- **Stale numbers**: invented Android tool versions, hard-coded tool
counts and description lengths that had drifted.
## Review
A Fable agent reviewed both halves adversarially for over-deletion,
misread code, `no-empty` hazards and byte-identity violations.
Second-half verdict: **SHIP**, with two one-line restores, both applied
in the final commit — the `npm view ""` rationale behind a blank-token
guard, and the note that `argent-mcp` keeps a copy of
`SECRET_PLACEHOLDER_MARKER` it cannot import.
## Code issues surfaced but deliberately not fixed
This pass changes comments only. Eight genuine findings are logged for a
follow-up:
1. `telemetry/src/consent.ts` — a non-ENOENT read error returns null and
falls through to the default-on path, so file errors *can* silently flip
telemetry on.
2. `chromium-server/navigation.ts` — `navigate()` is reachable from
`POST /api/navigate` with only a `typeof === "string"` check; open-url's
schema is a bare `z.string()`, so the "already validated by zod" premise
never held.
3. `http.ts` — `constantTimeEqual` returns early on a length mismatch,
so the auth token's length is observable.
4. `describe/index.ts:~114` — the ios-remote branch passes `{ isTvOs:
false }` unconditionally, so a remote tvOS simulator takes the iOS
ax-service path, though `isRemoteTvOsSimulator` exists and shake/paste
do use it.
5. `devices/boot-device.ts` — `-crash-report-mode never` is passed
unconditionally *and* appended again by the feature-detecting path, so
every emulator spawn passes it twice.
6. `react-profiler/pipeline/04-rank.ts` — `PARETO_THRESHOLD_PCT` is
dead: `slice(0, 20)` always wins.
7. `reaped-sessions.ts` — a user-facing hint string tells the agent that
`react-profiler-start { force: true }` disposes the debugger and
profiler session; it does not. Left byte-identical because it is a
string literal, not a comment.
8. `utils/simctl-backend.ts` — `localSimctl` is exported with no
importers anywhere.
## Docs
No documentation change is needed: this pass touches only source
comments, and no user-facing capability, tool, CLI flag, config key or
flow-file behaviour changed.
---------
Co-authored-by: filip131311 <f.kaminski2000@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Ignacy Łątka
|
8c8cdb885e |
fix(e2e): stop the harness reporting checks it never made (#595)
Found while running the full E2E harness against the packed 0.17.0 bundle for the release check. All of these are harness bugs, not product bugs. ## The core problem The harness reported results it had not actually measured. `assert_reject` looked for zod issues on **stdout**, but the CLI prints them on stderr and leaves stdout empty on a rejection. Both arms of the check called `pass()`, so the miss was invisible: the zod-path argument every case passes in was decorative, and a CLI that simply exited 1 for every input would have scored the same 88/88. **All 88 validation cases asserted nothing beyond "exit code was non-zero."** Once that check could fail, it exposed the rest: - **awk has no `\b` word boundary.** `/\bnumber\b/` matches nothing, so every flag kind resolved to `unknown`, `model_number_flags` was always empty, and **the bad-type matrix never emitted a single case** - while `_build_args` handed the string `"x"` to every required numeric/boolean/object field. - The describe parser matched `--[a-zA-Z0-9-]+`, **truncating every snake_case flag at the underscore** (`--device_id` → `device`), and kept the `-json` display suffix (`--selector-json`) that is not part of the schema key. Both produced payloads carrying keys no schema knows. - `missing-required` asserted `invalid_type` for enum flags, which zod reports as `invalid_value`. Parsed kinds went from **270 unknown / 21 enum / 1 array** to 131 string, 68 number, 28 boolean, 25 integer, 21 enum, 16 array, 3 object, 2 unknown (the last two are `any`, which is correct). ## Teardown and the exit code `run-e2e.sh` had no `trap`. An unbound variable inside a phase is fatal to the whole shell - `run_phase || warn` does not catch it - so an abort skipped the cleanup phase, the report, and `rm -rf $E2E_WORK`, leaking a ~500MB sandbox plus a tool-server, Electron, Metro and a fixture http server. Report generation, teardown and the exit code now live in an EXIT trap, with a completion flag: without it an aborted run exits **0** on "0 failures", which is verified below by mutation. Teardown runs *before* the sandbox is removed - `$E2E_HOME` holds the detached tool-server's pid file and that server is started with no idle timeout, so deleting the sandbox first strands a process `argent server stop` can no longer find. `90-cleanup.sh` gated `stop-all-simulator-servers` and `stop-metro` on `ARGENT_TOOLS_URL`, which the harness never sets (`ensure_server` deliberately does not pin it), so **neither ever ran** and every run left a simulator-server and Metro alive - while `teardown-complete` passed regardless. ## Other findings fixed - **`--android-avd` could never work**: `boot-device` was asked for 840s inside `run_tool`'s 120s timeout, and the booted serial stayed in a `local`, so the RN tier blanket-skipped all 24 debugger/profiler tools for want of a device that was right there. Nothing shut the emulator down either - the tool-server only reaps devices Lens booted, not ones `boot-device` started. - **The validation tier was byte-identical green against a dead server.** `argent run` does no client-side validation, so every call failed with a transport error that no one distinguished from a rejection. It refuses to run instead, and no longer counts `timeout(1)`'s 124-127 as a rejection. - **The post-uninstall restore** hardcoded `--omit=optional` and the sandbox prefix - dropping electron on a full run (the chromium tier then skipped itself as "electron not installed"), and under `--system` restoring the machine's real global `argent` into the sandbox instead of putting it back. - **The Android teardown claimed to leave an injected device alone** and then issued the same `stop-simulator-server` call as the self-booted branch, discarded the result, and recorded a skip saying it had not - tearing down the allocator's device service immediately before the RN tier drove that same serial. - **The chromium port-owner fallback** matched `:$port` as a substring and took the first pid anywhere in `ss -ltnp` output, so cleaning up `:1234` would kill a process listening on `:12345`. - **The RN tier's `stop-metro`** sat after every early return (a failed `debugger-connect` left Metro running) and ran unconditionally, killing a developer's own Metro on :8081 that the harness never started. - **The chromium display gate** accepted an installed `xvfb-run` as a substitute for `DISPLAY`, but nothing wraps the Electron spawn in it - so a headless box with it installed cleared the gate and failed `boot-device`, turning the gate red for exactly the environment it exists to skip. - A phase file with a syntax error silently re-ran the **previous** phase under the new phase's name; an unknown `--phase` selected nothing and exited 0; `-h` printed 7 lines of source code; a value-taking flag with no value died on an unbound `$2`; two runs in the same second shared one results file. - `flow-add-step` declares its own `args` property, which makes `--args` that field's value rather than the whole-payload escape hatch, so `run_tool` cannot address its fields. Skipped with that reason rather than failing on a payload the tool never received. ## Verification Against `swmansion-argent-0.17.0.tgz`, `introspection,validation`: | | before | after | |---|---|---| | totals | 177 pass / 0 fail / 5 skip | 221 pass / 0 fail / 6 skip | | rejection cases naming the field they test | **0 of 88** | **130 of 131** | | bad-type cases emitted | **0** | **43** | - `assert_reject` checked against right-field, wrong-field, wrong-code, no-path and unreachable-binary inputs. Every one of those cases passes on the un-fixed code. - Abort path: injected an unbound variable into a phase - cleanup phase runs, report written, sandbox removed, exit 1, `PARTIAL` reported. With the completion flag mutated to always-set, the same abort exits **0**, which is what pins it. - awk `\b`, the `-json` suffix, the underscore truncation, the `ss` port collision and the `timeout` 124-127 contract were each reproduced standalone before being fixed. ## Found by running against a live Android emulator The android tier executed end-to-end for the first time: **23 pass / 1 fail / 10 skip**, including the `screenshot-diff` pair that the original unbound-variable bug aborted on. Three further defects only surfaced by running: - **Nine `pass`/`fail`/`skip` calls passed their message as the third argument**, which `_record()` takes as the *case name* — so the detail column was empty and the case string changed every run. The chromium boot failure was the visible cost: it recorded `case=" "` with **no diagnostic whatsoever**. - **A call killed by `timeout(1)` writes nothing**, so `RT_OUT` is empty exactly when the operator most needs to know what happened. A hung `boot-device` was recorded as a failure with a blank detail. All nine ad-hoc detail formatters now share `rt_detail()`, which names the timeout (124) and a command `timeout` could not run (125-127). - **The chromium display gate tested that `$DISPLAY` was non-empty, not that it was usable.** A display-manager session switch on this box left `$DISPLAY` pointing at a server the process holds no cookie for; Electron then bound its CDP port and never answered on it (verified: the TCP connection is accepted, the HTTP response never arrives), so the tier hard-failed instead of skipping. Moving `profiler-stack-query` and `profiler-combined-report` into the native block also closed a hole their removal had opened: both were listed in `RN_TOOLS` and in the failure-arm skips but had no case on the success path, so a successful `react-profiler-start` left them with no pass, fail or skip at all. ## Not covered here - The **postinstall assertion** is left alone - #556 already fixes it, and this branch does not touch it. Until that merges the install phase still records that one failure. - The **RN tier still has not executed.** The emulator itself is fine — it cold-boots in ~24-30s and ran the whole android tier — but it dies from CPU starvation (`detected a hanging thread 'QEMU2 main loop'`) whenever this machine's load average climbs, which it does to 90-120 on 16 cores from concurrent work. Four attempts: it survived the android tier at load ~10, and was starved each time by a competing `npm install` or by Metro's own bundler startup. The four RN profiler fixes are verified against the tools' source contracts (the throw sites and their required params), not against a live run. - The **chromium tier still has not executed** past `boot-device`, for the display reason above. - Known remaining, deliberately left: `assert_ok` cannot fail for calls whose result is a constant `true`; the cookie/storage set→get pair never verifies the round-trip value; a tier-level skip writes one row so `report.py`'s coverage matrix silently drops the tools that tier would have recorded; `report.py` renders an empty or partial log as a clean green report. |
||
|
Ignacy Łątka
|
3761fb331b |
fix(e2e): make the debugger assertions fail when the debugger is not connected (#704)
Found while reviewing #610 and set aside as out of scope for that review: the weaker of the two assertions is a live defect on `main` today, the other becomes one when #610 lands. ## The defect `assert_ok` (`lib/common.sh:122-130`) records PASS on `RT_RC == 0` and never reads `RT_JSON`. Both tools on lines 84 and 86 answer **HTTP 200 while reporting that the debugger is not connected**, so the two assertions that exist to check the debugger chain pass over a dead session. **Line 84 is broken on main today.** Half-close the CDP socket server-side (the socket lands in CLOSING with no `close` event, so no terminated cascade) and the cached service still resolves: ``` ground truth: {"connected":false} === shipped assertions (lines 84/86) === ✓ debugger-status status ✓ debugger-log-registry logs === patched assertions === ✗ debugger-status [status] expected .connected to be true, got 'false' ✓ debugger-log-registry logs ``` **Line 86 breaks once #610 lands**, which turns the unreachable-runtime 500 into a 200 `not_connected` payload. Same two lines against a build of that branch with nothing listening on the Metro port: ``` ground truth: {"status":"not_connected","connected":false,"reason":"metro_not_running"} === shipped === ✓ debugger-status status ✓ debugger-log-registry logs === patched === ✗ debugger-status [status] expected .connected to be true, got 'false' ✗ debugger-log-registry [logs] expected (.status // "connected")=connected got 'not_connected' ``` ## Why these two jq paths Measured against live tool-servers built from both `main` and #610, in both the connected and the not-connected state: | | main connected | main dead socket | #610 connected | #610 not-connected | |---|---|---|---|---| | `debugger-status` | `.connected=true` | `.connected=false` | `.connected=true` | `.connected=false` | | `debugger-log-registry` | **no `.connected`** | - | **no `.connected`**, `.status="connected"` | `.status="not_connected"` | `.connected` is absent on log-registry's success path in *both* builds, so `assert_true '.connected'` would fail a healthy run - that is why the second line uses `(.status // "connected")`, which is `"connected"` on main (field absent) and on #610's success, and `"not_connected"` on its failure. Both assertions therefore work on main and keep working after #610. No new helper: `assert_true` / `assert_field` already exist and the harness already gates `launch-app '.launched'`, `restart-app '.restarted'`, `gesture-tap '.tapped'` the same way. `debugger-status`'s `.connected` was the one flag of that shape left bare. ## Verification Repro is a real tool-server per build plus the mock Metro/CDP target, driving the assertion lines lifted verbatim out of the patched phase file. The unpatched `assert_ok` passes in **both** states - that is the mutation control. Timing note for anyone re-running it: the half-closed socket self-heals after ~30s when the `ws` close timeout fires, so steps 5 and 6 have to run back to back. ## Deliberately not in this PR The same sweep found ~20 sibling `assert_ok` sites whose tool reports failure inside a 200 payload - `await-ui-element` `{success:false}`, `await-screen-idle` `{settled:false}`, `run-sequence` `{completed:0}` with the error in `steps[]`, `stop-metro` (whose entire body is inside a `try/catch` returning `{stopped:false}`, so `assert_ok` on it can never fail), `chromium-cookies set` `{set:false}`, `react-profiler-status` `{session_status:"no_react_runtime"}`, and the string-returning tools that answer `"No network traffic captured..."` at 200. Those verdicts are read off each tool's result contract, not driven live - confirming them needs the Android and Chromium tiers - so they are left for #595, which is already rewriting this harness with the same intent. One adjacent bug worth recording while it is fresh: `50-rn-bluesky.sh:96` extracts `RID` with `jq '(.requests // .entries // [])[0].requestId'` from `view-network-logs`, which returns a formatted **string**, not an object. `RID` is therefore always empty, line 98 is dead and line 100's skip is always taken. Merges cleanly with #595 (test-merged: `Automatic merge went well`; that PR rewrites the `local D=` line above but touches neither assertion). |
||
|
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>
|
||
|
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. |
||
|
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>
|
||
|
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 ``` |