mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
fix/flow-dir-export-throw
57 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jakub Piasecki
|
d4a712c401 |
refactor(gesture): rename the momentum-free flag to momentum (#732)
`settle: true` collided with the runner's other `settle` — waiting for the tree to quiesce — which is the very thing a swipe step does after every dispatch, whichever way the flag is set. One sentence of the flow skill used both senses at once. This names the physics instead: `momentum: false` is the momentum-free gesture, and the natural fling is the default the prose already described. ### Renamed | surface | before | after | | --- | --- | --- | | `swipe` flow directive | `settle: true` | `momentum: false` | | `gesture-swipe` param | `settle` | `momentum` | | `gesture-drag` param | `settle` | `momentum` | The directive passes the flag straight through to the tools, so all three move together. `momentum: true` is the default and normalizes to absent, mirroring what `settle: false` did — parse/serialize stay exact inverses. ### Also - Recording summary label `(settle)` → `(momentum-free)` - Internal constants → `MOMENTUM_FREE_EASE_EXPONENT`, `MOMENTUM_FREE_MIN_STEPS` - `flow-run` tool description and `argent-create-flow` skill updated ### Testing `tsc -b`, eslint (`--max-warnings 0`) and prettier clean; `test/flows` + `test/tools` pass 671/671, including a new case covering the legacy-spelling rejection. |
||
|
Jakub Piasecki
|
e314b3ef4a |
feat(flow): swipe directive (#520)
Add one semantic finger flick for dismissing cards, paging carousels, opening drawers, and other gesture-driven actions distinct from goal-seeking scroll-to. Support bare directions plus from with exactly one of direction, to, or by, optional settle, and positive finite duration. Resolve selector or normalized-point anchors through the shared target path. Direction uses the Maestro geometry presets and now rejects anchored gestures that would collapse or travel opposite to the requested direction. Relative by deltas saturate supplied axes to [0, 1] and reject any effective zero or sign reversal. Resolved starts must remain on-screen, while explicit endpoints stay author-controlled. Keep parse and serialization invariants aligned for programmatic steps, improve unknown-key hints and selector diagnostics, surface swipe targets in run reports and recording summaries, share positive-millisecond validation, and reuse the common flow test harness. Dispatch gesture-swipe on touch platforms and gesture-drag on Chromium, reject touch directives on Vega, and document when to prefer scroll-to or a raw system-edge gesture. |
||
|
Jakub Piasecki
|
2e182f7971 |
fix(flows): pin iOS tree reads to the launched app instead of auto-resolving every connection (#772)
## The problem Flow runs on CI iOS fail with `ViewInspector RPC timed out: Application.getState` at seemingly random steps - mid-flow after typing, on the first read after a fresh launch, in a probe flow 25 s after the original failure - while the app under test is provably healthy: spindumps taken at failure time show its main thread idle in the runloop, display links firing, and the injected devtools queue empty. The same flows pass locally. The chain: argent's iOS injection is simulator-wide (`launchctl setenv DYLD_INSERT_LIBRARIES` inside the simulator's launchd), so every process the simulator spawns afterwards loads the devtools dylib, not just the app under test. The dylib's constructor connects unconditionally (no UIKit or `com.apple.*` guard) and the tool-server registers whatever bundle id shakes hands, so Spotlight workers (`mdworker_shared`), background-launched system apps doing first-boot work (`MobileCal`), and bare daemons (nil bundle id, registered as `"unknown"`) all become connections indistinguishable from the app under test. Flow tree reads auto-resolved the target on every read: `queryFullHierarchyTree` passed `undefined` to `resolveNativeTargetApp`, which `Promise.all`s `Application.getState` over every connection. The dylib serves `getState` with a `dispatch_sync` onto the main thread, and a background-launched or suspended process never services its main queue, so its `getState` parks until the 5 s RPC timeout and the `Promise.all` rejects. One poisoned sibling connection fails every tree read in the run; the app under test is never even asked. That one mechanism explains everything the failures did. CI spindumps caught the poisoner red-handed (`MobileCal` parked in `HandleViewInspectorRequest -> CurrentApplicationState`) next to a healthy app. "Permanent wedge" vs "recovery" was just poisoner lifetime: a Spotlight worker exits and its connection drops with it, so reads recover; Calendar lingers and the whole job stays poisoned, fresh app launches and probe flows included. And it never reproduced locally because a warm simulator did its first-boot indexing and migrations long ago, while CI boots a pristine device and does exactly that work during the test window - which also made the failures correlate with typing and tapping purely by timing. ## The fix Pin the target instead of resolving it per read. `runLaunch` records the launched app on the run state once the tree-source gate passes, as `ActionEnv.treeTarget` - a `FlowTreeTarget` carrying the `bundleId`, whether the runner still vouches for it (`pinned`), and whether its probe has ever answered in this run (`probeAnswered`). `fetchFlowTree` threads that target down to `queryFullHierarchyTree`, the only platform source that consumes it. A pinned read talks to the pinned app and nobody else: it never calls `resolveNativeTargetApp` at all, and runs four gates of its own instead (`flow-ios-tree.ts`). A `com.apple.*` id is refused first, terminally and before anything is spent - a system app is never the app under test. Then `isConnected`, a map lookup, so a dead pin reports the dropped connection instead of driving env repair from 300 ms read polls, which could otherwise latch the device's process-wide give-up. Then a probe of the pinned app's own `getAppState`, and only that app's: a timeout before this pin has ever answered is ridden out as the cold start it usually is, one after an earlier answer is refused as an app that stopped servicing its main queue. Finally a foreground check on the answered state, as lenient over one app as auto-resolve is, so a flow that left the app fails with the crisp diagnosis instead of describing a screen that is not on screen. A poisoned sibling connection is never touched. The pin is only trusted while the runner can vouch for what is on screen, and it comes off in two tiers. The five `FOREGROUND_CHANGING_TOOLS` (`launch-app`, `restart-app`, `reinstall-app`, `open-url`, `button`) drop the target outright, because the launched app may no longer be on screen at all; `launch-app` and `restart-app` then re-set it from their own `bundleId` arg, unpinned, since they name the app they switched to - which is how a recorded flow switches apps, the recorder keeping them as raw `tool:` steps. Every other raw `tool:` step demotes the pin to an unpinned hint, its effect on the foreground being opaque to the runner; a launch attempt drops the target outright until it succeeds. Either tier also retires a proven tree-outage memo: a verdict proven against the pinned branch's four gates says nothing about the path the next read runs. An unpinned read is the pre-pin behavior with one arbiter added. `resolveNativeTargetApp` fans out over every connection, and only when that fan-out times out - the CI condition this change exists for - does the hint take the read, and only after passing the same terminal `com.apple.*` policy gate, the connections check, and a foreground-checked probe of its own. A resolution that answers always wins, including its deliberate "single app but backgrounded" refusal, so the arbiter never overrides a guard that fired: it only rides out a probe a stalled sibling made unanswerable. Nested `run:` fragments share the run state, so they inherit the pin, its demotion and its clearing. E2e flows begin with a `launch` by definition, so every selector read in a CI run happens pinned. Only iOS consumes the target; the other platforms' tree sources are per-device and never auto-resolve. The review rounds also surfaced that the `isInjectableBundleId` "library validation, the dylib can never load" rationale was disproven by this PR's own spindump evidence, so that text - and every other home of the same claim, `unreadableHierarchyReason` and `treeSourceGate` included - is rewritten as the policy it actually is (system apps are never the app under test; their observed connections are background processes whose reads hang or lie) with the gates' behavior unchanged. ## What this doesn't cover Auto-resolve's own fan-out is still all-or-nothing, so every read that reaches it without a hint the gates accept stays poisonable: top-level fragment runs, `describe` without an explicit bundle id, and the stretch after a raw `tool:` step whose hint is refused or cannot answer. Recording (`captureTapSelector`) now passes the session's last `launch` id as an unpinned hint, so it arbitrates the same timed-out fan-out, but it never pins - the recorder cannot vouch for the foreground app the way a `launch:` step can. Closing the rest means hardening auto-resolve itself (exclude connections that don't answer instead of failing the fan-out) and refusing the connections at the source (dylib constructor bail for non-UIKit and `com.apple.*` processes, plus a server-side handshake filter to cover already-shipped dylibs). Both are follow-ups. ## Tests Three files, and every behavior below is mutation-proven: disabling the seam, dropping the target from any single call site, removing an invalidation, or removing any one of the four gates each fails at least one dedicated test. The reviewers' repros were turned into tests rather than argued away. `flow-ios-tree-pinned-target.test.ts` works at the tree source. It reproduces the failure shape - a healthy app plus a poisoned `com.apple.mobilecal` sibling whose `getState` only times out - and proves a pinned read probes only the pinned app while an unpinned read still fails. It then covers the states auto-resolve could never reach: the terminal `com.apple.*` refusal, including its position ahead of the connections gate so a disconnected system app is not sent into a restart loop; a dead pin, where the dropped connection is reported and no env repair runs; both sides of the `probeAnswered` split; and the foreground guard over the states a real device produces (`inactive` under a permission dialog, `inactive` with no scenes, `background` still holding a foreground-inactive scene, and `background` with none). A second block covers the unpinned arbiter: the hint read on a timeout, refused when its own probe answers from the background, and rethrowing the fan-out timeout when the hint is disconnected, cannot answer, or when the resolution failed for any other reason. `flow-launch-pins-tree-target.test.ts` proves the runner wiring through the real `fetchFlowTree` seam: every read after a `launch` carries the pin across all five read paths (assert, tap, type, rotate, idle), retry polls included; fragments inherit it; a foreground-neutral `tool:` step demotes it and a foreground-changing one that names no app drops it outright; a later launch re-pins on a fresh target with the ride-out re-armed; and a launch-free run keeps the fallback. `flow-tool-step-tree-target-e2e.test.ts` drives `createRunFlowTool` with only `resolveService` stubbed, so the real `fetchFlowTree` and `queryFullHierarchyTree` run. It pins what the two tiers cost against a wedged sibling: the launched app still arbitrating across a foreground-neutral `tool:` step, the outage memo being retired when that step demotes the pin, the `com.apple.*` and foreground refusals staying in force through the demote, and a `hidden:` assert failing after the demote instead of going green against a backgrounded app. |
||
|
Hubert Gancarczyk
|
50c9580e30 |
fix(tools): make a schema failure name the parameter that was sent (#718)
> Stacked on #717. ## The problem An invalid tool call returned Zod's raw issue array. Someone who wrote `flow_name` instead of `name` got back, depending on the tool: ``` Invalid params for tool "flow-start-recording": [{"expected":"string","code":"invalid_type","path":["name"]}] Invalid params for tool "flow-execute": [{"code":"custom","path":["flow_path"],"message":"Pass exactly one flow source: name or flow_path."}] ``` The first names the parameter the tool **wanted** and never the one the caller **sent**. The second — flow-execute's, whose `name` is optional so `flow_path` can stand in — names neither: it is anchored on the one source field the caller had no reason to send. Either way the mistake is invisible, and working it out costs a whole turn. ## What changes **1. `describeParamIssues` — one sentence per bad parameter.** Missing fields read as `` `name` is required (string) and was not provided`` rather than as a type error about `undefined`. Unknown keys are named and path-qualified, so a key nested in `selector` is not reported as a bare name that contradicts the top-level list printed beside it. The message closes with the caller's own keys — `You sent: \`flow_name\`, \`project_root\`.` — which is what makes the mistake self-evident. Key names only, never values: this string reaches logs, telemetry and the agent transcript, and a params object can carry a secret. Wired into both entry points: the registry's `invokeTool` and the HTTP boundary. **2. `flow-execute` accepts `flow_name` as an alias for `name`.** The tool is called `flow-execute`, so "the flow's name" spells itself that way; this is a mistake worth absorbing rather than rejecting. The name is resolved in code rather than by the schema — a Zod `required` failure would once again name only the field it wanted — and the error text names the spellings that are silently discarded (`flowName`, `flow`, `flow_file`), because by then Zod has already stripped them and the caller has no other way to learn their key was dropped. **3. A flow DIRECTIVE passed as `command` points at the tool that records it.** `command: "echo"` used to come back as a bare "Tool not found". It now names `flow-add-echo` — and says to call it **directly**, since routing it through the recorder would write the echo *and* a `tool: flow-add-echo` step that fails on every replay. The hint distinguishes directives the recorder **rewrites** from those it stores **raw** for the polish pass, because promising a rewrite that does not happen sends the author looking for a directive that is not in the file. `wait` and `long-press` get their own answers: neither has a recording tool at all. Only a genuine not-found is rewritten — a tool that ran and failed still reports its own error. **4. A recorder tool as `command` is refused.** `flow-add-echo`, `flow-add-step`, `flow-start-recording`, `flow-finish-recording` each mutate the recording itself, and nesting one reported success either way. Each is refused for its own reason: `flow-add-echo` would write the echo **and** a replay-breaking `tool:` step, `flow-start-recording` would **erase** this flow at replay, `flow-finish-recording` would end the take it is a step in, and `flow-add-step` cannot record itself. Now nothing is executed and nothing is written. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
filip131311
|
5d76ed4d66 |
feat: tag artifacts with a semantic kind at registration (#564)
Fresh, from-current-main take on the artifact-kind idea from #398 / #466. Knowing what an artifact *represents* (not just its MIME type) is what a UI needs to group and name artifacts (e.g. in EAS); this PR delivers exactly that — the tagged handle — without the declared-output-map machinery. ## Summary - Adds a closed `ArtifactKind` union to the artifact wire contract; `kind` is carried on `ArtifactHandle`, `ArtifactEntry`, and the `/artifacts` inventory items. - `ArtifactStore.register` now takes a single `{ hostPath, kind, ... }` options object, so the compiler forces every producer — current and future — to declare what it is emitting. - Tags **all** producers on main today, including the ones #398/#466 predate: screenshot (all four capture paths), screenshot-diff, **flow snapshot baselines/diffs** (`screenshot` / `screenshot-diff-context`), **screen recordings** (new `screen-recording` kind), and the native/React profiler exports and reports. - Types the native-profiler export keys at their source (`IosExportKey` from the `EXPORTS` table, `AndroidExportKey`) and maps them through a kind table that is **total over both unions** — a newly added export key fails to compile until classified, replacing the previous stringly-keyed loop (and the `?? "native-profile-trace"` silent-default this design would otherwise need). - Client-side (`@argent/tools-client`), the mirrored handle type keeps `kind` **optional and widened** (`ArtifactKind | (string & {})`): an older, already-running tool-server sends no kind, and a newer server may send kinds this client build predates. Consumers treat it as advisory metadata; unknown kinds are opaque, never an error. ## Differences from #398 / #466 (deliberate) - No `@argent/artifacts` package, no third `ToolDefinition` generic, no per-tool output-name registrar, no runtime throw for “undeclared output” — the kind is plain data at the registration call site, checked at compile time where it can be, and a wrong label can never fail a tool at runtime. If a pre-invocation `artifacts` declaration in tool-listing metadata is ever needed (an actual EAS consumer), it can be layered on later without changing this wire contract. - The two `ArtifactKind` definitions (registry / tools-client) stay deliberately separate — same as the existing `ARTIFACT_MARKER` must-match convention — because the client-side type is intentionally *looser* (optional + open) than the server-side one for version skew. - Extending the union stays backward-compatible on the wire. ## 2026-08-24 update Merged current main (0.22.0) into the branch. The flow-visual rewrite that landed since (cropOn crops, per-app snapshot-key dedupe, diff scratch-dir hygiene) is preserved verbatim; its five registration sites now carry kinds (`screenshot` for baseline/current, `screenshot-diff-context` for the annotated diff — the cropped `current` included). No producer added since the PR was opened needed a new kind: `paste`, `shake`, and the other new tools emit no file artifacts, and every register call site in the tree now goes through the typed options object. No docs update is needed: the docs site (added since this PR) does not document the artifact handle wire contract; `kind` is advisory wire metadata with no user-facing surface. ## Validation At the merge with main (0.22.0), 2026-08-24: - `npm run build` (full `tsc --build`) — clean - `npm run typecheck:tests` in registry, tools-client, tool-server, mcp, cli — clean - Full suites green: registry 90, tools-client 191, tool-server 4495 (+1 skipped), mcp 80, cli 472 - `prettier --check .`, `eslint . --max-warnings 0`, and the knip gate — all pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Hubert Gancarczyk
|
f21811d069 |
fix(flow): re-probe a recorded wait against the tree the runner reads (#717)
> Stacked on #715. ## The problem `await-ui-element` evaluates against the tree it reads — the **accessibility hierarchy** on iOS and Android, the **CDP DOM** on Chromium, the **toolkit page source** on Vega. The `await:` / `assert:` directive that the polish pass converts a recorded wait into is evaluated against the **runner's** tree instead. They overlap, but outside Vega neither contains the other — an id present in one can be absent from the other, and on iOS even the role vocabularies are disjoint. Both directions have been hit in practice: an id visible to the runner but not the recorder, and text-field values visible to the recorder but not the runner. The consequence is that a check can pass live and fail once converted. That makes the core promise — *each step is executed live so you verify it works before it's recorded* — untrue at exactly the point it is relied on. ## What changes After a recorded `await-ui-element` succeeds, the recorder re-runs the same condition against the tree the runner resolves directives against, and reports the result on the step it just wrote. **It warns; it does not refuse.** What gets written is a raw `tool: await-ui-element` step, and at replay that tool reads the same tree it just passed against — so "this would fail every run" was never true of the form actually recorded. What the probe genuinely reports is whether the **polish-time conversion** is safe, which is why the warning says to act on it *then* rather than re-record now (re-recording would duplicate the step). Because polish begins after `flow-finish-recording`, each verdict is also carried there: it is repeated on its own step's `summary` line, and `message` counts how many steps carry one. A warning raised at step 7 of a 40-step recording is otherwise long out of reach by the time it is actionable. ## Three outcomes, kept distinct | Runner tree says | Reported as | | --- | --- | | condition holds | nothing — step recorded clean | | condition does not hold | recorded + warning naming the causes to rule out before converting | | tree unreadable, or too slow to answer | recorded + warning that the conversion is **UNKNOWN, not known-bad** | The middle row is deliberately not "the conversion will fail". The identical verdict comes back when the screen simply moved on between the live wait and the re-probe, and on `text` checks when the selector matches several elements and the two sides elect different ones — so the consequence stays conditional on the cause, and the message names the causes in the order worth ruling out. The third row matters: nothing was compared, so calling it a divergence would send the author off to rewrite a selector that may be perfectly good. It is also the injection-free case the flow skills explicitly sanction, so refusing there would block a supported way of working. On iOS the quoted reason ends "provide bundleId explicitly" — advice aimed at the native tools. The warning corrects it, because no directive carries a bundleId and forwarding one would make the probe answer a question the runner never asks. ## Platform-specific explanation The *why* differs per platform, and stating the iOS story everywhere makes the message wrong where someone is trying to act on it. iOS gets AX-vs-native-hierarchy; Chromium gets "the flow tree keeps only addressable nodes"; Android gets the importance trim; Vega gets "the trees agree, so the screen changed". The named tool for reading the runner's side follows the same rule — pointing an Android author at `native-find-views` points at a tool they cannot call. The remedy is per **condition** as well as per platform: on `hidden` a determinate verdict means the runner's tree still *has* the element, so "retarget at an id the full hierarchy carries" is the opposite of what helps. ## A failed live wait is reported by cause `await-ui-element` returns `{ success: false }` for three different things: a condition read and found false, a tree source that never answered, and a cancelled wait. Only the first judges the condition, so only the first is narrated as "the wait never held" and prescribes re-recording or deleting the step. The other two observed nothing and are reported as UNKNOWN — the same unknown-vs-known-bad distinction the probe draws for the runner's tree. A wait nested inside a recorded `run-sequence` gets neither warning: that tool reports its own shape, which carries no `success` key. The skill says so, and says to read `toolResult` there. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
Ignacy Łątka
|
5d8fc10799 |
test: stop tests binding machine-wide resources they do not own (#714)
## Why Three flaky or failing tests were diagnosed separately this week, and all three turned out to be one defect wearing different clothes: **a test binding a machine-wide resource it does not own**, so a second concurrent run — another checkout, a CI matrix cell, a developer's own app — perturbs it. Those three are fixed in #707, #709 and #711. Each of those was found by chasing a symptom, and each audit covered only its own resource type. This is the systematic sweep: all **448 test files across 13 packages**, plus the 4 `node --test` scripts. (The sweep itself ran over the 400 files that existed when it started; merging `main` brought the count to 448, and every category below was re-run over the full set — 25 files assign `env.HOME` and none omit `USERPROFILE`, one test file calls `pkill`/`pgrep` (`vega-cli-timeout.test.ts`, scoped out to #711 below), and no `listen()` names a fixed port.) ## Findings | Category | Files checked | Real findings | |---|---|---| | Network endpoints (bind / probe / unix socket) | 448 | 4 | | Filesystem — shared temp namespace (`os.tmpdir()` and literal `/tmp`) | 448 | 8 | | Filesystem — real `$HOME` / `~/.argent` | 448 | 4 (28 files) | | Ambient environment | 448 + all `src` readers | 12 | | Device / emulator / SDK state | 448 | 2 | | Process table (`pgrep` / `pkill` / fixed-pattern sweeps) | 448 | **0** | | In-process globals leaking across files | 13 packages × 13 modes | **0** | Every finding was reproduced with a concrete instrument before being touched, and the instrument re-run after. Representative: - **`stop-tools.test.ts`** ran the real `stop-metro` against port 59999, which `lsof`s and **SIGTERMs whatever it finds**. With a squatter there: `16 passed` → `1 failed`, and the squatter logged `received SIGTERM #1`. Now port 1. - **`bind-failure-telemetry` / `lens-relay-telemetry`** were gated on the real `~/.argent/flags.json`. Enabling the documented `tool-server-event-log` flag: `4 passed` → **`4 failed`**, and records were appended to the developer's real event log. - **LogFileWriter (12 files)** — its constructor `mkdir -p`s `os.homedir()/.argent/tmp`. Instrument: run each of the 16 test files that reference a writer, or a blueprint that builds one, under a scratch `HOME`/`USERPROFILE`, then test `$HOME/.argent/tmp` for existence. 4 created it; the 12 that actually construct a writer now all call `scopeTempHome`, and none creates it. - **`screen-recording.test.ts`** — the device ids are fixtures and `vi.useFakeTimers()` seeds its clock from the real time, so only the millisecond a run starts in separates two runs' `.mp4` paths, and each test deletes the path it derived. Self-concurrent: **2 of 3 rounds failed** → 0 of 4. Scoping `TMPDIR` removes the window rather than narrowing it. - **`DO_NOT_TRACK=1`** → telemetry `282 passed` → **`13 failed`**. **`npm_config_user_agent=pnpm|yarn|bun`** → installer `535 passed` → 4/5/3 failed. **19 tests required a real `adb`** — PATH shadow + empty HOME: `30 passed` → **`19 failed`**. - A fixed `/tmp/argent-nd-<udid8>.sock` **destroyed a concurrent holder's live socket** (proven by inode) in **3** files that run the native-devtools factory. Two of them also fail outright: `native-devtools-app-state` lost **3 of 3** rounds of two concurrent runs on `expected 'unregistered' to be 'connected'`. All three now derive the UDID from `process.pid`. - `flow-visual`, `react-profiler/dump`, `file-inputs`, `http-upload` — shared `/tmp` paths and prefix scans, each reproduced with a squatter. In `http-upload` the collision surfaced *as a timeout*, inside `vi.waitFor`. (`flow-chromium-boot` was fixed on this branch too, but main's #585 rewrote that file afterwards and the merge kept main's copy, so it is byte-identical to `origin/main` here and carries no fix.) ## Clean, with the evidence - **Process table** — an `LD_PRELOAD` exec interposer over a full run (507 execs) plus all 12 other suites. Excluding `vega-cli-timeout.test.ts`, which is #711's and does sweep the table (`pkill -f '^sleep 6910[0-9]$'` at line 134): no `pgrep`/`pkill`/`killall`/fixed-pattern sweep anywhere, and **no test signals a pid it did not spawn**. - **Real device state** — **zero** exec attempts of real `adb` / `simctl` / `emulator` / `avdmanager` / `vega`; no test asserts a device list. That list was too narrow: `workspace-reader-integration` spawned the real `yarn` / `pnpm` / `bun` / `pod` / `eas` / `expo`, which is the second finding in this row. - **Cross-file in-process leakage** — structurally impossible: vitest 4.1.9 resolves `pool: "forks"` / `isolate: true`, confirmed by a scratch experiment (distinct pids, no env leak, module `Map` = 1,1,1). 120 default-mode runs across 12 packages produced 1 failure. - **`scripts/`** — 4 files, 86 tests: `mkdtemp` throughout, no ports, no sweeps, writes nothing. ## Verification Each fix was mutated back and killed by a **named** test under its instrument — e.g. `× reads a plain absolute path`, `× returns null when only the extensionless binary exists`, `× pins both names os.homedir() consults, at the same directory`. Restores verified with `git diff` / `git status`, never a substring grep. **Pinned by an instrument only:** the socket fix for `ios-only-blueprint-gate`, the `~/.argent/tmp` and `~/.argent/flags.json` fixes, the `/tmp` fixture cleanup, the `stop-metro` port change (the test asserts `stopped: false` / `pids: []`, which hold for either port whenever nothing is listening) and the `workspace-reader` `PATH` pin (only `node` is asserted; the other seven probes are not) are not pinned by any suite assertion — reverting them still passes, and only the external squatter / scratch-`HOME` instruments catch them. They are pollution fixes. The `native-devtools-app-state` socket fix is the exception in the other direction: reverting it turns two concurrent runs red. Gates, unpiped and read by exit code: `eslint .` 0, `prettier --check .` 0, `tsc --build` 0, `typecheck:tests` 0. Suite, run as six shards: tool-server **4287 passed, 1 skipped, 0 failed**; argent-cli 462; telemetry 306; argent-tools-client 191; configuration-core 122; argent-mcp 80; argent-installer 555. All other packages and `test:scripts` green, identical to baseline. `lens-tools-platform-gate.test.ts` times out at the 5s default under full-suite load and passes at `--testTimeout=30000` (24.7s, almost all of it transform). It predates this branch and is untouched by it. ## Scope This branch is **test-only** — no `src/` file is touched. Both production defects the sweep turned up are separated out, so each can be reviewed as the user-facing fix it is: `ps` without `-ww` orphaning live tool-servers is #713, and #852 is `ensureToolsServer` respawning on every call when `ARGENT_HOST` is exported — `buildToolsServerEnv` passes the inherited value to the child while the state records the literal `127.0.0.1` the health check then probes. The second surfaced here as eight stranded `fake-tool-server` processes, and pinning the environment in this branch removes that signal, which is why it is filed rather than merely worked around. A third, outside this branch entirely, is #853: ten `tool-server` test servers omit the host on `listen(0)`, binding every interface where twenty-two sibling sites pass `127.0.0.1`. It is hardening, not a live exposure — `createHttpApp`'s Host allow-list answers a LAN-addressed request with 403, and only a raw client forging `Host: 127.0.0.1` reaches the stub registry behind it, inside the ~100 ms one `it()` holds the socket. It is also not a collision risk: ephemeral ports are kernel-assigned and never reissued while in use, which is why the fixed-port audit above is unaffected. All ten predate this branch. Left untouched on purpose: `boot-electron-spawn-error.test.ts`, `chromium-discovery.test.ts` and `vega-cli-timeout.test.ts` — owned by the still-open #709 and #711, and all three still carrying their defects on this base. (#707 has since merged into this base, so `boot-device-hotboot.test.ts` and `vitest.config.ts` carry its fix rather than a defect; `test/setup/clear-argent-env.ts` is its work, and this branch relies on it.) **Not fixed, deliberately** — an adjacent class (test-*order* dependence, not machine-global resources), reproducible if wanted: `link-config.test.ts` (deterministic: `-t "returns null when required field"` → 4 failed), `network/network-integration.test.ts` and `android-perfetto/dispatch.test.ts` (fail under `--sequence.shuffle` seeds). **Also observed, not addressed:** one green tool-server run leaves **56** entries behind in a scratch `TMPDIR` — **30** in the `argent-screenshot-diff*` family, 4 `argent-events-*`, 3 `argent-flow-crop-*`, 3 `argent-tar-upload-*`, 2 `argent-flow-diff-*`, 2 `argent-file-input-*`, and 12 assorted singletons (`ast-index-*`, `component-source-*`, `argent-profiler-cwd`, `argent-chromium-media`, …). Measured per run against a scoped temp dir, so it ranks the contributors rather than counting one machine's accumulated history — screenshot-diff, not flow-crop, is the dominant one. A separate leak. That instrument is blind to a *literal* path, which is how it missed the two `/tmp/native-profiler-*-report.md` writers the fourth pass found. Re-run across all 13 packages, `argent-mcp` is the only one outside `tool-server` that leaves anything — two entries per run, filed as #854. ## Second pass A review of this branch re-ran every category above with its own instrument rather than reading the diff, and found the sweep had pinned four resources in one file while leaving a sibling that does the same thing live. Fixed in `4289e3d8f` and `8164955b4`; the table and bullets above are the corrected totals. - **`tool-server-event-log`** was mocked off in 2 of the 4 files that call the real `start()`. With the flag on, `startup-telemetry` and `tool-fail-invalid-params-emission` failed 3 tests in `attachRegistryEventLogger` and truncated the real `~/.argent/tool-server-events.jsonl` to 0 bytes first. `ARGENT_EVENT_LOG` cannot redirect that: `test/setup/clear-argent-env.ts` strips every `ARGENT_*` var before the module graph loads. - **`scopeTempHome`** reached 8 files; 4 more built a real `LogFileWriter` through the JS-runtime-debugger blueprint. - **`os.homedir()` on Windows** reads `USERPROFILE`, not `HOME`. `android-binary-windows` and `adb-resolve-avd-path` pinned only `HOME`, and both run on a real windows-latest host. Proven by running the resolver under a `node:os` mock that reproduces the Windows lookup: with `HOME` alone the stock Studio SDK under the profile resolves; with both pinned it misses. `update-checker` had the same gap. - **`getConsentState`** honours a falsy `ARGENT_TELEMETRY` at the same precedence as `DO_NOT_TRACK`, ahead of the config override the suite writes. Only `DO_NOT_TRACK` was pinned: `ARGENT_TELEMETRY=0` gave `17 failed | 6 passed`. - **`workspace-reader-integration`** let the version probe spawn the real package managers off `PATH`; their shims self-install, so the file pulled **~44 MB** into `~/.cache/node/corepack` and `~/Library/Caches/eas-cli`. `PATH` is now a directory holding only `node`, the one version the test asserts — 44 MB → 4 KB. - **`argent-mcp`** snapshotted `ARGENT_AUTO_SCREENSHOT_DELAY_MS` without clearing it, and that package has no `ARGENT_*`-stripping setup file, so 3 tests asserted the developer's exported value. Smaller items in the same commits: the `/tmp` socket fixtures now remove the directories they create per run; `http-upload`'s pre-scan snapshot is gone, since scoping `TMPDIR` to a fresh directory made it permanently empty and its filter inert, as its two siblings already did; and two `detectProjectPackageManager` assertions that enumerated the entire return union are pinned to a single value. The confirmation pass also caught a defect the first round of fixes introduced: pinning `HOME`/`USERPROFILE` to the fixture root in `android-binary-windows` let `<home>/Android/Sdk` shadow the `%LOCALAPPDATA%` root the two positive tests are named after, so deleting `roots.push(join(localAppData, …))` from the resolver left all three green. Home has its own directory in `6aee3ee7a`, and that mutation now fails both tests. A third pass, running the nearest-twin move repo-wide, found the `os.homedir()`-on-Windows gap in one more package: eight `argent-tools-client` test files redirect only `HOME`, and four of them recursively delete `STATE_DIR` — two before each test, two after — which on Windows is the developer's real `%USERPROFILE%\.argent`. Fixed in `139447d4a`, together with the same gap in `avd-snapshot`. Nothing runs that package on Windows CI today, so no current run fires it. ## Fourth pass A fourth pass audited the three fix commits and re-ran the sweep with two instruments the earlier ones did not have. Both holes were in the *measurement*, not the diff — which is why three passes missed what they found. Fixed in `a67c43f8d`. - **`os.tmpdir()` does not read the same variable on every platform.** POSIX takes `TMPDIR`, `TMP`, `TEMP` in that order; Windows takes `TEMP`, then `TMP`, and never looks at `TMPDIR`. The five pins this branch added set `TMPDIR` alone, so on Windows production still materialized into the machine-wide `%TEMP%` while the test scanned a scratch directory nothing ever wrote to — and `file-inputs`, `http-upload` and `flow-visual` each end in a leak assertion over that listing, which then passes whatever the code does. Against `origin/main`, which diffed a before/after listing of the real tmpdir, that is a regression rather than a wash. `redirectTmpdir` (`test/helpers/tmpdir-env.ts`) sets all three and returns the restore; the **7** files that scope a tmpdir use it, including the two `android-perfetto` ones that predate this branch. `test/tmpdir-env.test.ts` pins both lookup chains — reverting the helper to `TMPDIR` alone fails two of its three tests. All three leak assertions were re-confirmed live on POSIX by mutating the production cleanup each one covers. - **A hardcoded absolute path is invisible to a `TMPDIR`-scoped instrument.** `session-correctness` and `ios-instruments/analyze-freshness` passed a literal `/tmp/native-profiler-{x,ios}.trace` as the session's trace file, and the renderer writes its report beside it (`deriveReportPath` = `dirname(traceFile)/<base>-report.md`). Every run left `native-profiler-x-report.md` and `native-profiler-ios-report.md` in the shared `/tmp`; nothing removed them, and two concurrent runs write the same names. Reproduced with `TMPDIR` pointed at a scratch dir — the files still landed in `/tmp`, which is exactly why the `os.tmpdir()` row and the leftover census below both read clean here. Both tests now own the directory. - **A prefix-scanned env var is invisible to a grep for variable names.** `secretSources` enumerates `process.env` by the `ARGENT_SECRET_` prefix and ranks it *above* every file the test writes, so `secrets-command.test.ts` — which already pins `HOME`, `USERPROFILE` and cwd — still read the developer's shell. `ARGENT_SECRET_FOO=bar`, argent's own documented way to supply a secret: `6 passed` → **`3 failed | 3 passed`**, deterministic. `argent-cli` has no `ARGENT_*`-stripping setup file, so the prefix is cleared per test. Its sibling `configuration-core/tests/secrets.test.ts` was already immune — it threads an explicit `env: NO_ENV` into every call. - **`android-binary` and `vega-cli-fallback`** were the last two files redirecting `HOME` without `USERPROFILE` — the former the direct twin of `android-binary-windows`, in the same directory, driving the same resolver. Under a `node:os` mock reproducing the Windows lookup with an SDK planted in the profile, it loses **5 of 11** tests. No test file in the repo redirects `HOME` alone any more. (`6fbb66ce1` later skipped both of this file's describes on win32 — its fixtures are extensionless and the win32 resolver only takes `.exe` — so its own `USERPROFILE` pin is now symmetry with the sibling files that do run there, not a live guard.) Two smaller items in the same commit. `native-devtools-socket-bind`'s ENOENT fixture still rooted at `os.tmpdir()` while the comment three screens above explains why the others do not: with a 142-character ambient `$TMPDIR` that path exceeds `sun_path` and `listen` rejects it for *that* reason, which the assertions cannot tell from the missing-directory one they were written for. And `<home>\AppData\Local\Android\Sdk` — the resolver's fallback for a GUI-spawned server that never inherited `%LOCALAPPDATA%` — could be deleted with the whole suite green; it now has a test, and deleting either win32 root fails exactly the tests named for it. ## Fifth pass A fifth pass took the platform-branch shape the fourth pass exposed and ran it as its own move: *for every function that branches on `process.platform`, does the test's guard cover every arm, or only the one the author's machine takes?* Fixed in `e08b466b6`. - **`init-stale-config.test.ts` deletes the developer's real VS Code MCP config on Windows.** The file mocks `os.homedir()` and says it does so "so hidden-scope probes (`~/.claude.json`, VS Code user profile) never touch the real home directory". `vscodeUserDirs()` branches `darwin → homedir()/Library/Application Support`, `win32 → %APPDATA%`, `else → homedir()/.config` — the win32 arm never calls `homedir()`, so the mock is inert there, and the test's own `userMcpJsonPath()` mirrors the same branch. Both user-profile tests then write a one-entry fixture over `%APPDATA%\Code\User\mcp.json` with a whole-file `writeFileSync`, and assert that `remove()` deleted it. Reproduced by forcing `process.platform` to `win32` with a two-server `mcp.json` planted at a probe `%APPDATA%`: **`3 passed`, and the file and its directory were gone.** With `APPDATA` pinned, the whole file passes under the same instrument — 31 tests — and the planted config is byte-identical. - **`argent-tools-client` hands its ambient `ARGENT_*` to a real spawned child.** The launcher spawns an actual tool-server, and `buildToolsServerEnv` overrides only the keys it is given — everything else rides in on `...baseEnv`. `ARGENT_HOST` is the one that bites: the fixture binds the host it names while the launcher records and health-checks `127.0.0.1`, so reuse misses and a second server is spawned. With six overrides exported: **`10 failed | 181 passed`, and eight stray `fake-tool-server` processes still holding TCP listen sockets after the run.** With the prefix stripped: `191 passed`, none surviving. The package now carries the same setup file `tool-server` has, for the same reason — and it is the second of the two packages whose missing `ARGENT_*` strip this review found. ## Later passes A sixth pass audited the fix commits themselves and bounded the SDK probe (`6fbb66ce1`); a seventh ran the moves against the diff one more time and turned up the two below, plus the `listen(0)` defect filed as #853. - **The `PATH` pin made `workspace-reader-integration` POSIX-only without the guard its siblings got.** The shim it puts on `PATH` is an extensionless `node`, and `symlink()` to a file needs an elevated process or Developer Mode on Windows, so `beforeAll` throws `EPERM` there before any assertion runs; a copy in place of the symlink would not rescue it either, since a bare command name resolves through `PATHEXT`. This sweep already skips `android-binary` and `native-devtools-socket-bind` on win32 for exactly this class, and the Windows job runs a curated list of seven `tool-server` files that excludes this one, so nothing was catching it. Fixed in `095d75bd8` — verified POSIX `2 passed`, and under a forced win32 platform `2 skipped` with the top-level `beforeAll` never entered. - **A rationale that rested on a case the file excludes.** `android-binary`'s header justified pinning `USERPROFILE` by what `os.homedir()` reads on Windows, two sentences before noting that both describes skip on win32. Fixed in `f37b3605e`; the variable stays in the snapshot list, the justification no longer claims a platform this file never reaches. A seventh pass also re-ran the sweep against a hostile machine: every feature flag on in a scratch `~/.argent/flags.json`, no `adb` on `PATH`, and a `TMPDIR` long enough to overrun the 104-byte `sun_path` limit. It found one resource this branch had not closed. - **A red `launcher-version-gate` run strands a real tool-server.** `ensureToolsServer` spawns a fixture server, but its pid only reached the reaper after three or four assertions on the state it returned; any of those throwing left the pid unrecorded, and the same `afterEach` then deleted `STATE_DIR`, the only other record of what to kill. **Eight such processes were alive on the development machine**, spread from 14:33 to 18:23, every one from an `argent-version-gate-test-*` fixture and none from any other family — each still holding a TCP listen socket with its fixture directory long gone. Forcing one assertion to fail took the `tool-server.cjs start` count from 10 to 11. The nearest twin, `launcher-duplicate-spawn`, guards this twice: it records the pid straight after the null check and sets `FAKE_TTL_MS` so an escaped server self-exits. Fixed in `26c2c60ec` with both, the TTL also covering the window between the spawn and the state read where no pid exists yet — the same forced failure now leaves **zero** strays. An eighth pass audited the seventh's own fixes, and found the version-gate fix had been applied to one file when the defect was a property of four. - **Three more files strand a real tool-server on a red run.** `launcher-spawn` and `launcher-exit` set `FAKE_TTL_MS` nowhere; `launcher-duplicate-spawn` — the file `26c2c60ec` cites as the model — armed it inside one test's `try/finally`, so eight of its nine spawns ran without it and the `finally` disarmed it for the remainder of that test. Reproduced per file by forcing a failure before the pid is recorded: a detached node process survives, reparented to init, holding a listening socket, with its `TEST_HOME` removed and its state record already cleared. Fixed in `3bb69ded6` by moving the net to a file-wide `beforeEach` in all three. Verified end to end — the child carries `FAKE_TTL_MS=60000`, and a stranded server spawned at 21:28:36 exited by itself at 21:29:36 with nothing signalling it; three consecutive green runs stay at 191 passed. - **A 29th file writes the real `~/.argent`.** `boot-electron-spawn-error`'s success-path test completes a boot, and a completed boot calls `trackChromiumPort`, persisting to `os.homedir()/.argent/chromium-cdp-ports.json`. The real file on the development machine had accumulated **106 dead ephemeral ports**. Its two siblings in the same directory already mock `chromium-discovery` for exactly this reason; this was the third, with no mock. Fixed in `3306b6f40`. - **`scopeTempHome` was itself unpinned.** Deleting its `USERPROFILE` line — the Windows half, the same property the sibling helper's test pins — left all twelve consumers green at 48 passed. `3306b6f40` adds the missing test; it kills that mutation plus dropping the `HOME` pin and replacing the per-test `mkdtemp` with one shared directory. Two findings from that pass were refuted on verification and are recorded rather than acted on: the tool-server tmpdir leftovers (mechanism real, but they are the separate leak this body already scopes out) and a claim that `26c2c60ec`'s message overstated its twin. Four comments were also corrected against their code: `deriveReportPath` joins `<base>-report.md` *inside* the trace's directory rather than appending to the directory's name, which is what makes the `rm(traceDir)` added in `a67c43f8d` cover it; two `android-perfetto` comments still named `TMPDIR` as the mechanism after the switch to `redirectTmpdir`; and `android-binary`'s header claimed the home pin lets its two positive tests resolve "on either platform", though its fixtures are extensionless and the win32 resolver only accepts `.exe`. |
||
|
Ignacy Łątka
|
5ac6805040 |
fix(native-devtools): derive restart_required from the running process instead of asserting it (#560)
this is urgent
---
## Problem
Every `native-*` tool can get stuck telling the agent to do something
that will never work.
I hit this during an end-to-end pass over 0.17.0. After a mid-session
simulator reboot, `native-devtools-status` reported:
```json
{ "envSetup": true, "appRunning": true, "connected": false,
"requiresRestart": true, "nextLaunchWillBeInjected": true, "injectable": true }
```
and every `native-*` tool returned `restart_required` with "Call
restart-app then retry." Following that advice never helped, and the
message offered nothing else. `flow-execute` was blocked too, since its
`launch` step needs the connection, and flow recording silently degraded
a portable `tap: { selector }` to raw coordinates when it couldn't reach
native devtools.
The app was injected the whole time. All three dylibs were mapped into
the process and the simulator log showed a successful dial on every
launch, while the tool-server held the listening end of that exact
socket and still reported `connected: false`. `argent server stop &&
argent server start` cleared it instantly.
## The real defect is the state, not the wording
`requiresAppRestart` returned `true` for every app without a live
connection:
```ts
async requiresAppRestart(bundleId) {
if (connections.has(bundleId)) return false;
await reverifyEnv();
return true; // <- asserted, never measured
}
```
That is not a measurement. `restart_required` was a constant wearing a
state's clothes, and the advice built on it stays literally true
forever, so an agent obeying it restarts the app indefinitely.
Softening the message would have left that intact. This PR measures the
state instead.
## Fix — derive it from the running process
`restart-app` relaunches into the simulator's **current** launchd
environment, so it can only ever help a process launched under different
terms than a fresh launch would get. That is answerable: read the
process. `launchctl list` already gives its pid; `ps eww -p <pid> -o
etime=,command=` gives the environment it was exec'd with and its age.
Simulator apps are ordinary host processes owned by the same user, so
both are readable.
`appConnectionState` (replacing `requiresAppRestart`) returns:
| state | meaning | remedy |
| --- | --- | --- |
| `connected` | devtools are live | — |
| `not_running` | no process to read | `launch-app` |
| `stale_process` | no bootstrap dylib, a previous run's endpoint, or
older than this service's listener | `restart-app` |
| `unregistered` | injected against **this** endpoint, started after the
listener came up, still silent | restart the **tool-server** |
| `indeterminate` | process could not be read (ios-remote, unreadable
`ps`) | `restart-app`, with the loop warning |
Only `not_running` / `stale_process` / `indeterminate` report
`restart_required`. `unregistered` gets a new `service_stale` precheck
status that names the tool-server restart and explicitly says not to
relaunch the app — the launch a restart would perform has demonstrably
already happened.
The listener-age comparison is load-bearing: after a tool-server restart
the unix path is identical (it is derived from the udid) but the inode
is new, so an injected process older than the listener still needs
relaunching. Without that check this would send people off to restart a
tool-server they had just restarted — a new loop in place of the old
one. Both grace comparisons lean away from `unregistered` - the first
towards `stale_process`, the second towards `indeterminate`, and both of
those ask for a relaunch - so an uncertain read costs at most one wasted
`restart-app`.
Call sites follow the same rule rather than each inventing guidance:
- `describe` withholds `should_restart` for `unregistered` (setting it
would rebuild the restart-app → describe loop through a live service
instead of a system app) and carries the diagnosis as a `hint`, which
still marks the empty read untrustworthy for `await-ui-element`.
- `flow-run` raises the measured diagnosis instead of a blanket
"relaunch it" — both when a flow
resolves a selector against the full hierarchy, and at the `launch`
step's readiness gate, which
previously polled for the connection and then guessed at "a stale or
duplicate argent server".
- `native-devtools-status` reports `state`, and `requiresRestart` is now
false for `unregistered`.
- The six `native-*` tools type their block as the shared
`NativeDevtoolsPrecheckBlock` instead of restating it, so a new precheck
outcome cannot reach an agent through a tool that never mentions it.
## Scope
Why the service can stop registering a connection it is listening for is
still #561 and is not fixed here. What changes is that the state is now
measured, so that defect reports itself accurately instead of
masquerading as an app that needs restarting.
## Verification
**E2E, real iOS 18.6 simulator, Bluesky (RN 0.81.5).** Two tool-servers
were raced onto the same per-udid socket — the #561 shape, where the app
is genuinely injected and connected, just to the newer instance. Both
were then queried on the same app at the same instant:
| | `connected` | `requiresRestart` | `state` | `native-describe-screen`
|
| --- | --- | --- | --- | --- |
| main, stale instance | false | **true** | — | `restart_required` —
"Call restart-app then retry." |
| this branch, stale instance | false | **false** | `unregistered` |
`service_stale` — "Restarting the app cannot change that … restart the
tool-server" |
| healthy instance | true | false | `connected` | `ok`, 44 elements |
Restarting the app in main's row lands on the newer instance again,
leaving the queried one stale forever. That is the loop, reproduced on
demand.
The other states were walked on the same simulator: a pre-existing
process against a fresh listener → `stale_process` (`requiresRestart:
true`); `restart-app` → `connected`; terminate → `not_running` ("Call
launch-app (or restart-app)").
**Unit.** `native-devtools-app-state.test.ts` drives the real blueprint
factory with a stubbed process table and covers all five states plus the
`etime` and env-token parsers. Mutation-checked — dropping the
listener-age comparison, the endpoint match, or the connect grace each
fails exactly one test and nothing else.
**Suite.** `vitest run` in `packages/tool-server`: 296 files, 3113
passed, 1 skipped. `tsc --build` across the workspace, `tsc --noEmit -p
tsconfig.test.json`, `eslint` and `prettier --check` on every changed
file: clean. `origin/main` merged in first.
## Review round
A review pass over the derivation itself, with each finding reproduced
before it was acted on.
**`describe` computed the diagnosis and threw it away.**
`buildAppStateMessage` ran for every
unconnected state, but only `unregistered` carried it out; the rest
returned a bare
`should_restart: true`. That boolean reaches an agent as an undocumented
JSON field — `describe`'s
own description never mentions it — and the single place it is rendered
as prose is
`await-ui-element`'s timeout note, which spells it *"call restart-app
and retry"*. So the two states
whose message carries the escape hatch lost it exactly where it was
needed. `indeterminate` is the
sharp end: on `ios-remote`, whose app processes are out of reach of the
local process table, it is
the **only** reachable unconnected state, so that path had no exit at
all. The diagnosis now rides
out as a `hint` for every state; `should_restart` still marks only the
states a relaunch fixes.
**`native-devtools-status` measured the same device twice.**
`isAppRunning` ran its own
`launchctl list`, and `appConnectionState` ran another one after
re-verifying the launchd env — so
the two snapshots sat seconds apart and could contradict each other
(`appRunning: true` beside
`state: "not_running"`), with `requiresRestart` gated on the staler of
the pair. Both are derived
from one measurement now; only `indeterminate`, which genuinely leaves
running-ness unanswered,
pays for a second probe.
**Coverage.** Mutation testing found the 3 s grace term entirely
unpinned — the fixtures sat 200x
away from the boundary on either side, so both dropping the term and
weakening `>=` to `>` kept the
suite green while flipping a relaunchable process to `unregistered`, the
one verdict the file exists
to withhold. Also unpinned: the `reverifyEnv` call (swapping it for the
latching `ensureEnvReady`
the comment explicitly forbids passed the whole suite) and an
unparseable `ps` age staying "no
evidence" rather than becoming a measurement. All three are now covered,
and each new test was
confirmed to fail on the un-fixed code.
**Verified against the real thing.** The `launchctl` and `ps` contracts
were re-checked on an
iOS 18.6 simulator rather than reasoned about: row layout and pid
column, one-line `ps eww` output
with the argent tokens present in a live app's environment, `etime`
matched against `lstart` to the
second, and the four `etime` format strings read out of the `ps` binary.
Two corrections came out of
it — the environment can reach `kern.argmax`, which is exactly Node's
default `maxBuffer`, so the
probe now sets the same 16 MiB its siblings use; and `ps` suppresses the
environment of a
SIP-protected platform binary, which the docstring's "never as 'not
injected'" promise did not
account for (unreachable here, since `com.apple.*` is rejected as
non-injectable upstream, but the
promise now says so).
**Also corrected:** the `argent-device-interact` skill offered
`restart-app` as the only remedy for
a missing injection; the dylib-platform check in
`download-native-binaries.sh` and a
`requiresAppRestart` reference in `boot-device.ts` described states that
no longer exist; and three
tool descriptions still said *"Fails if native devtools are not
connected"* directly beside the new
sentence explaining that this case returns a status rather than failing.
**Known and deliberately left** (both pre-existing, neither introduced
here):
`describe` with no `bundleId` auto-targets from the connected list, so
an unregistered app throws
`NATIVE_TARGET_NO_CONNECTED_APPS` before any state is measured and the
message is swallowed by the
fallback's catch — surfacing it would newly mark those empty reads blind
for `await-ui-element`'s
`hidden` condition, which wants its own verification pass. And a dylib
that is inserted but silently
skipped by dyld (a platform mismatch) reads as injected-but-silent, so
`service_stale` → tool-server
restart → `stale_process` → `restart-app` is a two-step cycle;
distinguishing it needs a load
confirmation the process table cannot give.
## Review round 2
A second pass over the derivation, run as four independent sweeps
(claims-vs-code, nearest-twin
and reachability, non-happy paths and inputs, and absence). Three of the
four independently landed
on the same defect.
**The flow launch gate was the one caller that never asked whether the
app could be injected at
all.** Every other consumer of `appConnectionState` gates `com.apple.*`
first - the precheck
throws, `describe` returns its terminal hint, `native-devtools-status`
short-circuits,
`flow-ios-tree` only ever auto-resolves a connected app.
`waitForNativeDevtools` measured the raw
launch id. That matters because the launchd environment carrying the
bootstrap dylib is
simulator-wide, so a system app's process inherits the very tokens the
measurement reads.
Confirmed on an iOS 18.6 simulator: `com.apple.Preferences`, launched
under argent, reports
```
71940 0 UIKitApplication:com.apple.Preferences[9a63][rb-legacy]
etime=00:03
DYLD_INSERT_LIBRARIES=.../libArgentInjectionBootstrap.dylib
NATIVE_DEVTOOLS_IOS_CDP_SOCKET=/tmp/argent-nd-39B9C547.sock
```
Run through the shipped algorithm that is `processCarriesInjection:
true` against an older
listener, i.e. **`unregistered`** - so a flow with an `ios:` launch
entry of `com.apple.Preferences`
failed its launch step with *"restart the tool-server"*, for an app no
tool-server restart can ever
help. Library validation means it can never connect, so the gate now
says that terminally.
**`describe` dropped the entire diagnosis when the probe rejected.**
`appConnectionState` re-applies
the launchd env before it can answer anything, so it throws outright on
a sim that goes away
mid-call; that landed in the native fallback's outer `catch` and
returned an empty tree with no
hint. An agent saw "nothing on screen" rather than "could not be read",
and `await-ui-element` had
nothing to append to its timeout note. The two other consumers of the
same call already degraded to
`indeterminate` instead - this one now matches them.
**`native-devtools-status` re-ran the probe that had just failed.** One
route into `indeterminate`
is `inspectRunningApp` rejecting, i.e. `simctl spawn <udid> launchctl
list` failing. The
running-ness fallback then issues that same command unguarded, so a
shut-down simulator surfaced a
raw subprocess error rather than the structured `init_failed` guidance -
which the branch 40 lines
above it handles correctly, for exactly this reason.
**`native-devtools-status` was the only surface withholding the loop
escape.** Every other consumer
carries `buildAppStateMessage`, whose `indeterminate` text ends *"do not
keep restarting the app;
restart the tool-server"*. This tool returned booleans only, and
`requiresRestart: true` is the
whole of what an `indeterminate` app tells you. On ios-remote, where no
app process can ever be
inspected, that is the only state a running app reaches - so the tool
whose stated job is verifying
readiness answered with the restart loop and nothing else. It now
returns `message`.
**Also fixed:** both probes log their own failure (a broken probe
degrades every app to
`indeterminate`, indistinguishable at the tool surface from a genuinely
uninspectable one - the
sibling `ps` probe in `vega-process.ts` logs for the same reason); the
gate skips its
several-second measurement once the run is cancelled; and the gate no
longer repeats the bundle id
its caller's prefix already names.
**Prose that had drifted from the code:** `not_running` is reachable on
ios-remote too, so
`indeterminate` is only the sole unconnected state a *running* app
reaches there; the second grace
comparison leans to `indeterminate`, not `stale_process`; four of the
five states settle
running-ness, not four besides `not_running`; a null pid means a row
that did not parse, not a job
launchd reported as having no process (measured: a `UIKitApplication`
row is removed outright when
the app exits, and `-` appears only on daemon labels);
`waitForNativeDevtools`'s documented return
contract described the branch the change had rewritten; and a rationale
comment was attached to the
test above the one it described.
**Coverage.** The gate's measured reason had none at all - replacing the
whole diagnosis with a
literal left all 542 flow tests green. Eleven mutations were run against
the fixes and every one is
now killed: the injectability guard, the measured reason, the doubled
bundle id, `describe`'s reject
guard, `describe`'s `unregistered` hint merge, the `message` field, the
precheck fallback, the
`appRunning` guard on `requiresRestart`, a state renamed in the
description only, the connect grace
widened from above (the old fixtures all sat 10x clear of it, so it
could have grown to 25 s
unnoticed), and the `indeterminate` guidance line.
---
## Review round 3 — external review findings
Six findings from @hubgan's E2E review, each reproduced before it was
acted on. Fixed in
|
||
|
Jakub Piasecki
|
568660509c |
fix(flow): floor every settle at two read attempts (#780)
`settleTree` bounded its polling by a 3s window but never bounded the number of reads it took, and a tree read can fail by TIMING OUT. Every tree source's RPC timeout outlasts that window, the shortest being iOS's 5s, so the window fit exactly one such read: "every read in the window failed", the condition the throw reports as a tree-source outage, collapsed into "the first read was slow", erroring a tap/type/scroll step on one transient blip with no retry at all. The floor makes the deadline stop deciding how much evidence an outage verdict is drawn from. `await`/`assert` have never had this flaw, since `waitForCondition`'s final poll already guarantees a second attempt, so this is the same guarantee given to the directives that resolve through a settle. A run whose reads answer inside the window pays nothing: two reads are what a settle already costs there. A run whose reads outlast it pays a second read, and gets a settle it never actually got before, since past the deadline the old code returned its single tree having compared it against nothing. A wedged source costs two full reads rather than one, and a read is several RPCs deep with each hop on its own timeout, so that is tens of seconds on a step that used to fail in one read. The alternative is the step failing on evidence the window was too short to gather. |
||
|
Jakub Piasecki
|
8062da77a5 |
fix(flow): settle before gestures that resolve no selector (#779)
A selector target settles inside `waitForFrame`, but a target that resolves nothing skipped the settle altogether: a coordinate `tap`/`long-press` and a centre-anchored `pinch`/`rotate` dispatched into whatever motion was in flight, so they could land mid-fling, which is the exact race the settle exists to close. Every input directive now settles first, however it is addressed. The settle is best-effort here, unlike on the selector path: these gestures read no frame out of the tree, so an outage must not fail them, which would break the escape hatch coordinates exist to be (an element the tree cannot see). This is the allowance `snapshot` already makes before a capture that reads pixels rather than nodes. Best-effort is not free, though, and the run pays it per gesture. A tree the run can never read is not only the disconnect mid-run: `fetchFlowTree` serves no tree at all on `ios-remote`, and an app that cannot load the instrumentation fails every read off an in-memory list, an Apple system app being the case in point, which flows drive by coordinates for exactly that reason, so every step of such a flow is one of these gestures. Measured against the real fetch on an `ios-remote` device, a coordinate tap went from 0ms to 3022ms, once per gesture, for a settle that was never going to arrive. So a settle that spends its whole window without one successful read records the outage on the run, against the device that proved it, and a later gesture rethrows that instead of buying the same verdict again. Not a shorter budget: any threshold low enough to spare a source that serves no tree would also abandon the mid-navigation blip the settle's retry exists for. But the record is a prediction, not a proof of repetition. One window mints it and nothing re-tests it, and on iOS that window can be a single stalled read, since the deadline is only tested after a read returns and `ViewHierarchy.getFullHierarchy` sits on a 15s RPC tier behind a serial 5s `Application.getState` probe. Being wrong therefore costs a settle rather than a step, and it is not silent: a gesture that spends the record warns its step report that it dispatched unsettled, quoting the source's own reason, so a run whose prediction was wrong says which of its greens mean the gesture was sent rather than that it landed. Every tree read a directive makes now goes through one helper so that a read which comes back clears the record whichever directive asked for it. `await`/`assert` and `idle` never settle, and a run that reads the tree successfully must not keep skipping. A relaunch clears it too, whether written as a `launch:` directive or as one of the raw `tool:` steps that perform the same operation, since a relaunch is the repair the commonest of these errors asks for by name. Two more reads route through the same helper for uniformity but can never be the clearing one: `rotate`'s aspect read skips itself while a record stands, and the `type` focus wait runs only behind a `waitForFrame` whose own settle already cleared it. What it costs, in two directions. An outage that lasted a full window, in a run that then never reads the tree again and never relaunches, leaves later gestures unsettled even once it clears: a flow of nothing but coordinate gestures giving up a best-effort settle it never had before this change, against every one of those gestures otherwise paying the window for a settle that is not coming. And on a screen that never holds still, a spinner, a video, a ticking clock, nothing converges, so every one of these gestures pays the whole window, measured at about 3.05s each. The record buys no relief there by design, because a window that read the tree proves no outage, and the fingerprint cannot be narrowed the way `scrollToVisible` narrows its own: a scroll knows the motion it waits on is its own, inside the containers under its anchor, while a gesture waits on motion of unknown origin that can move what sits under the point without touching it. It is the same window every selector directive already paid on such a screen; what changed is that coordinate targets pay it too. Two smaller pieces ride along. A centre-anchored `rotate` skips its screen-aspect read on a recorded outage as well, degrading to the same legacy orbit a failed read gives it, so the record actually takes the window off that directive instead of handing back a 15s-tier RPC. And the settle's tree read now takes the run's abort signal, so a cancelled run reports its skip immediately rather than once the source finally answers. It deliberately takes no time budget: one cut to what is left of the window would fail the cold start #778 raised that tier to ride out. |
||
|
Jakub Piasecki
|
abe5ff3c56 |
refactor(flow): generalize when's block machinery so a second block directive can't miss a site (#748)
`when` is the only directive with a sibling `steps:` list, and four runner sites asked `kind === "when"` directly to decide whether a step has authored children to expand as skip lines. A second block directive would have had to remember all four, and a forgotten one silently drops a whole block from the report — exactly the invariant `reportBlockSkipped` exists to hold. This introduces `blockSteps(step)` as the single runtime predicate and `BLOCK_DIRECTIVE_KEYS` as its parse-time counterpart, and extracts the parse-side block plumbing (`assertBlockDepth`, `parseBlockSteps`) that a second directive must share — notably the depth cap, renamed `MAX_BLOCK_DEPTH`, which has to be ONE counter across directives or an alternating nesting chain evades it and reopens the cyclic-YAML-alias hole it was added to close. No behavior change, with one deliberate exception: the depth-cap message now reads `block directives nest deeper than 20 levels` where it said `when blocks nest deeper than 20 levels` — the cap stopped being `when`'s the moment it became shared, and a message naming only `when` would misdirect an author who hit it through a different directive. Every other error message, their ordering, and the report shapes are untouched. 559 flow tests pass unchanged. --- **Stack** 1. **#748 — you are here** · `refactor(flow)`: generalize when's block machinery → `main` 2. #749 · `feat(flow)`: add `repeat:` blocks → #748 |
||
|
filip131311
|
6177409c7c |
fix(tools): drop top-level JSON Schema combinators from tool input schemas (#782)
Fixes #773. ## The bug `gesture-rotate` (`anyOf`), `flow-execute` and `flow-read-prerequisite` (`oneOf`) hand-write an `inputSchema` that spreads a top-level combinator onto the derived schema. The Anthropic Messages API rejects that outright: ``` tools.N.custom.input_schema: input_schema does not support oneOf, allOf, or anyOf at the top level ``` The 400 fails the **whole request — every tool in it** — so any client forwarding our schemas verbatim as `custom` tools is bricked entirely, not just for these three tools. Reported from opencode. ## Why we never hit it **Claude Code silently repairs it before sending.** Verified live: it strips the combinator and re-expresses the rule as a prose line prepended to the description. All of our E2E/QA runs through Claude Code, so the defect was invisible from inside our own test loop. Secondary reasons: the code is recent (#546, 2026-07-24 for `gesture-rotate`; #568, 2026-08-05 for the two flow tools — before late July no argent tool had a top-level combinator), and nothing in CI asserted the shape of any tool's `inputSchema`. ## Why removing it is safe The combinators never enforced anything. Nothing in argent validates against the JSON Schema — the zod `.refine()`/`.superRefine()` does all the rejecting, on every dispatch path (HTTP 400, MCP `isError`, CLI flag error, internal registry dispatch), and always did. #546's stated rationale was "to keep invalid calls rejected at the schema layer", but that layer does not exist; the only JSON Schema evaluator in the repo was a 5-line helper inside that PR's own test. Verified equivalence before deleting: the removed `anyOf` accepts exactly the same 8 radius shapes as the two refinements, and `oneOf` the same 4 source combinations as the superRefine. ## What changed - The three hand-written `inputSchema` consts are gone. No tool hand-writes one now, so `Registry.registerTool` is the single emission point for all 76 schemas. - Each rule moves into the tool description, **worded differently per tool** — `tool-description-quality.yml` triggers on these paths and scores `disambiguation_score` (W=1.5), so identical prose on two tools would work against the gate. - `ToolDefinition.inputSchema` documents why it should stay underived. - New per-tool contract test over the whole catalog: no top-level `oneOf`/`allOf`/`anyOf` (API-rejected) and no top-level `not`/`if`/`then`/`else`/`$ref`/`$schema` (blocked pre-emptively — the natural way to re-encode a rejected combinator). Nested combinators stay legal and in use: `tv-remote`'s `button` is a `z.union`. - Catalog walk + expected count extracted to `test/helpers/catalog.ts`, shared with `interaction-messages` so a new tool needs the count bumped once, not twice. - Six existing tests read `.inputSchema` off unregistered definitions (populated only at registration) and now derive it. ## Not done by design **No throw in `registerTool`.** `createRegistry()` runs at tool-server bootstrap (`index.ts:182`), where a throw becomes an uncaught exception → `crashShutdown` → `exit 1`: a total outage, and `update-argent` is itself a tool so it could not run to repair it. The catalog is static and compiled in, so the contract test catches the same regressions before merge — and it also covers `paste` and the Lens tools, which never pass through `registerTool` at all. **Deliberately a denylist, not an allowlist** of permitted top-level keys: `additionalProperties` legitimately appears on schemas derived from `.strict()` objects (`screenshot-diff`, `stop-all-simulator-servers`). ## Follow-up left out `gesture-rotate`'s refinements carry `path: []`, and `deriveInvalidParams` drops empty-path issues — so every cross-field rejection is telemetered with an empty `invalid_params`. Fixing it changes CLI wording and starts populating that field, so it is not riding in this PR. ## Verification - 75 registered tools probed live: **zero** top-level combinators; the three now advertise `type, properties, required` only. - Negative test: re-introducing an `anyOf` fails the gate with `gesture-rotate declares top-level anyOf`. - `packages/registry` 90/90 pass; `packages/tool-server` 3952 pass with **7 pre-existing failures** in `bind-failure-telemetry`, `lens-relay-telemetry`, `startup-telemetry` and `tool-fail-invalid-params-emission` — all failing identically on pristine `fe5976857` (they die in `attachRegistryEventLogger`, unrelated to schemas). - Build, eslint, prettier clean; knip identical to baseline (60 unused exports / 187 unused exported types both before and after). Reaches users on the next npm publish; current version is 0.20.0. |
||
|
filip131311
|
fe59768575 |
fix(flows): ride out iOS main-thread stalls instead of failing the step (#778)
## Problem iOS flows against a heavy app (Expensify NewDot, release sim build) intermittently error with ``` ✗ 4 tap "Continue" — ViewInspector RPC timed out: Application.getState ``` The only known mitigation was enabling **Reduce Motion** on the simulator — which shouldn't be a functional requirement, and (as the investigation showed) wasn't actually the mechanism. ## Root cause The injected ViewInspector handlers hop onto the app's **main thread** (`dispatch_sync` in `DevtoolsConnection.m`). During a heavy first-launch cold start (first Hermes parse, asset decode, on a loaded host) the main thread can be pinned past the flat **5s** RPC timeout, and the flow layer turns one timed-out read into a step error: - every iOS tree read first runs target auto-resolution → an `Application.getState` probe per connected app, per read — the cheap probe is the first to hit the stall; - `settleTree`'s 3s window < the 5s RPC timeout, so an action step's **first** stalled read exhausts the window and throws; - a default `await` (7.5s) gets ~1 stalled attempt; - the flow launch readiness gate (8s) sits below the same stall ceiling, because the dylib's connect is also main-thread-gated. Steady-state RPCs are fast (instrumented: `getState` p50 0ms / max 402ms, `getFullHierarchy` p50 9–12ms — even with animations on and a 14-core CPU load). Only the stall window is the problem; Reduce Motion merely shortened it below the cliff (and correlated with cache warming from repeated relaunches). Error-code note: these failures are `NATIVE_DEVTOOLS_RPC_TIMEOUT` on an **established** connection — distinct from the multi-server socket-contention family (#775), which surfaces as `could not connect to native devtools` at launch. ## Fix (three coordinated changes, tool-server only) 1. **`getFullHierarchy` RPC timeout 5s → 15s** — mirrors the Android devtools client's 5s default / 15s `getHierarchy` tiers. Every other method (incl. `getState` and the interactive point queries) keeps 5s, so one-shot agent tools stay snappy on a wedged app. 2. **Flows remember the launched app** (`launchedNativeApp` on the run state, threaded to iOS tree reads; invalidated by `launch-app`/`restart-app`/`reinstall-app`/`open-url`/`button` `tool:` steps; shared with nested `run:` flows). It is a **timeout arbiter only**: auto-resolution still runs on every read, and any *answered* resolution — including the deliberate "single app but backgrounded" guard — wins. The hint decides the target only when the resolution probe itself dies of an RPC timeout and the hinted app's devtools connection is still up. 3. **Launch readiness gate 8s → 15s** — same stall class, same budget. Accepted trade-offs (from adversarial design review): a persistent outage now takes longer to report (a dead-but-connected app fails an `await` after up to ~2 slow reads instead of three 5s ones), and a stall that clears right at a settle deadline can act on a single post-stall read (previously a hard error). Both were judged worth the removal of the false-failure class; the review also killed an earlier variant of this fix that would have bypassed the backgrounded-app guard. ## Verification - **Fault injection**: a DYLD-inserted dylib pins the app's main thread on demand (12s). Pre-fix, deterministically reproduces both historical signatures — mid-flow stall → `tap error: ViewInspector RPC timed out: Application.getState`; launch-window stall → `launch error: could not connect`. Post-fix: **5/5 green** across both stall placements ×2 + no-stall control. - **Android regression**: the same sign-in flow on the Android emulator through the patched server — green (the hint is iOS-only by construction; Android/Chromium tree paths unchanged). - **Unit tests**: new `flow-ios-tree-launch-hint.test.ts` (6 cases: timeout fallback, no-hint rethrow, disconnected-hint rethrow, backgrounded-guard preserved, non-timeout rethrow, answered-resolution-beats-hint); existing `flow-ios-tree-no-windows` and `native-target-app` suites green. Full tool-server suite: 3875 passed; the 7 failures in `bind-failure/startup/lens-telemetry` tests fail identically on pristine `origin/main` sources in this environment (pre-existing, unrelated). Related: #775 (multi-server devtools socket contention, filed from the same investigation), #774 (sim keyboard shift wedge, unrelated mechanism ruled out during triage). 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Hubert Gancarczyk
|
8623965b8a |
perf(flow): stop the recorder returning the whole flow file per step (#715)
## What changes `flow-add-step` and `flow-add-echo` returned the **entire growing YAML** on every call. Across measured recording sessions that was the majority of all tool-result text, and it grows quadratically with step count. - Both tools now return `stepCount` — how many steps the flow has — instead of `flowFile`. - `flow-add-step` additionally returns `recorded`: the one summary line that was appended. - The full file still comes back once, from `flow-finish-recording`. ## Why The cost is not just tokens. With the whole file echoed per step, context pressure during a long walkthrough visibly changes the artifact being produced — checks get trimmed from a test to buy back room. Returning a line per step removes that pressure without removing any information the author needs *at that moment*. ## Supporting change `summarizeStep` is lifted out of `flow-finish-recording`'s per-file loop so the recorder can render its one line exactly the way the final summary does — one spelling, not two that can drift. That line now also carries `times`, `duration` and `delayMs`. They change what replays, and since the recorder no longer returns the YAML, this line is the author's only per-step view of what landed in the file. ## Compatibility This is a **breaking change to the tool result shape**: `flowFile` is gone from `flow-add-step` and `flow-add-echo`. `savedTo` is unchanged and is still the only field naming the destination in remote-client mode. `flow-finish-recording`'s result **shape** is unchanged, but its `summary` strings are not: taps now render `×N`, long-presses `for Nms`, and tool steps `(after Nms)`, so the two surfaces spell a step the same way. Nothing outside the agent reads `summary`, so that is not a break. ## Tests Recorder tests now assert against the flow **as persisted on disk** rather than a returned string, which is the surface that actually matters. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
Hubert Gancarczyk
|
74cddedf50 |
docs(skills): split argent-create-flow into references and add argent-qa-flows (#730)
> Now rebased directly onto `main`. #728 and #574 have landed, so this PR carries only its own 13 documentation commits; it still does not sit on the #715-#726 chain. > > Guidance describing behaviour owned by that chain is therefore **not** carried here: the recorder's refusal of a vacuous `hidden` and of `await-screen-idle` (#724), its cross-tree re-probe warning (#717), the per-step `recorded` line (#715), and `type:` failing on unconfirmed focus (#726). Each of those paragraphs states what this base actually does instead; they are worth restoring when that chain lands. ## `argent-create-flow`: ~5,000 words to ~900 plus three references Every word of that skill loaded on every invocation, whether the task was replaying a two-step fragment or authoring a full regression test. The core skill now routes to three references read on demand: - **`live-authoring.md`** - recording a walkthrough - **`flow-yaml.md`** - the file format and selector vocabulary - **`reliability-and-recovery.md`** - what to do when a step or a replay goes wrong Nothing is dropped in the split - it moves behind a router. ## New skill: `argent-qa-flows` Turning a **test case, ticket, or acceptance criteria** into a repeatable regression test is a different job from recording a path worth replaying, and it was being done by a skill that never asks for acceptance criteria. It orchestrates `argent-create-flow` as its engine, records the first walkthrough live, requires every requested screen and state to be proved with stable evidence, and completes only after the unchanged flow passes **twice consecutively**. ## Proving a navigation Spelled out here for the first time, as two element-level checks rather than a route read: ```yaml - await: { visible: { id: settings-screen } } # identity: WHICH screen - await: { idle: true } # readiness: it stopped moving ``` Neither implies the other - a dropped tap leaves the source screen perfectly idle, and the destination's elements enter the tree while the transition still animates over them. Both belong after every screen change, including the one `launch:` performs. ## Routing The three skills were being confused by name alone, so the rules now draw the line: | you want | skill | | --- | --- | | a one-off interactive check, nothing saved | `argent-test-ui-flow` | | a saved, replayable path | `argent-create-flow` | | a saved test with acceptance criteria and two-pass proof | `argent-qa-flows` | "Record a flow" also stops being ambiguous with **screen recording**, which is video. ## Tests Every bundled skill's YAML frontmatter is parsed in a test, so a malformed new skill fails here rather than at install time. The `idle` doc guards that came with the warn-instead-of-fail change follow the prose into `references/flow-yaml.md`: they pin the documented defaults, settle cost and minimum timeout to the constants the parser enforces, and hold the tool description and the reference to the same account of what `idle` does. All 43 anchored internal links resolve. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
filip131311
|
4975ff490c |
fix(debugger): cut debugger tool fail rate — structured not-connected results, classified CDP failures, narrow recovery (#610)
## Why
PostHog (30d) shows the debugger tool family is argent's least reliable
surface:
| tool | fail rate | dominant causes |
|---|---|---|
| `debugger-status` | **37%** | METRO_NO_TARGETS /
SERVICE_INITIALIZATION_FAILED / zod |
| `debugger-connect` | **24%** | SERVICE_INITIALIZATION_FAILED (~7k, >1k
users), METRO_NO_TARGETS |
| `debugger-log-registry` | 41.7% raw / 4.2% excl. storms | one user
retry-looped it **12,742×** on NO_TARGETS |
Three design issues drive this: (1) "app/Metro not ready yet" — an
expected precondition — is reported as a tool *error*, which both
inflates fail rates and invites agent retry storms; (2) every CDP
init/lifecycle fault lands in telemetry as the catch-all
`REGISTRY_SERVICE_INITIALIZATION_FAILED` with `error_kind=unknown`; (3)
~600 users/30d hit zod validation failures with zero visibility into
which parameter was wrong.
## What
**1. Classified failures** (`d71efa43`) — six former plain-`Error` sites
in `cdp-client.ts` / `target-selection.ts` now carry dedicated
`DEBUGGER_CDP_*` / `DEBUGGER_TARGET_DEVICE_MISMATCH` signals (messages
byte-identical). The registry's two causeless "terminating window"
`ServiceInitializationError`s get their own
`REGISTRY_SERVICE_TERMINATING` code.
**2. Narrow recovery** (`ff9c1955`) — `recoverable()` on both debugger
blueprints, scoped to windows where retry is provably safe: Metro
recovers only `DEBUGGER_CDP_NOT_CONNECTED` (request never left the host;
a detected socket death already self-heals for the *next* call via the
terminated cascade — recovering `CONNECTION_CLOSED` there would be dead
code, see the blueprint comment). Chromium also recovers
`CONNECTION_CLOSED` for the tab-switch reconnect window.
`REQUEST_TIMEOUT` stays unrecoverable (double-execution risk). A
reachability test proves the retry actually fires for a RUNNING node.
**3. Structured not-connected results** (`48b91ffd`) — `debugger-status`
and `debugger-log-registry` become factories that return `{ status:
"not_connected", reason, detail, guidance }` for classified
preconditions (`metro_not_running | no_app_connected | device_mismatch |
cdp_unreachable | stale_connection | reconnecting`) and still throw on
unexpected faults. `detail` preserves the original error text for
string-matching agents. `debugger-status` gains a socket-state gate
(Metro: dispose stale node; Chromium: report transient reconnect, no
dispose — the client belongs to the ChromiumCdp dependency).
log-registry keeps **no** gate on purpose: captured logs are readable
over a dead socket, and disposal would destroy the post-crash log file.
Flows/run-sequence map a not_connected result to a **failed step**
(mirrors `isUnmetUiWaitResult`) so recorded connectivity gates don't
silently pass. `debugger-evaluate` re-codes agent-expression throws as
`DEBUGGER_EVALUATE_EXPRESSION_THREW`.
**4. Telemetry + guidance + docs** (`7602a70d`) —
`tool:fail.invalid_params` (schema-declared names only; unknown keys →
literal `"unrecognized_keys"`; capped emit-side at 16), new
`debugger:tool_outcome` event (coded enum; joins
`tool:invoke`/`complete` via `tool_invocation_id` —
`ai_client`/`duration` live on the joined row), anti-retry-loop guidance
appended to the Metro discovery errors (first sentences verbatim —
skills match on them), six skills updated, Telemetry.md bumped to v1.03
with the new coded diagnostics disclosed.
## Dashboard migration note
`debugger-status` / `debugger-log-registry` precondition failures stop
emitting `tool:fail` — track them via `debugger:tool_outcome` (`outcome
!= 'connected'`). Per-AI-client breakdowns of those outcomes need a join
on `tool_invocation_id` against `tool:complete`.
## Testing
- Full suites green: tool-server 3,142, telemetry 290, registry 87. New
coverage: recoverable truth tables (incl. non-Error inputs), status
not-connected matrix
(Metro/Chromium/terminating-window/stale/dispose-side-effect/fall-through-rethrow),
log-registry rows, zod `invalid_params` derivation + emission-level
guard, flow gate, evaluate rewrap, sanitize-level enum checks.
- Live-tested against a booted iOS simulator + RN app and an Electron
target (details in PR comments).
Plan hardened through 3 adversarial review rounds (46 confirmed findings
addressed) before implementation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
||
|
Hubert Gancarczyk
|
96ea94f87a |
feat(flow): add an idle condition that waits for the screen to stop moving (#728)
> Stacked on #574. Replaces the route-fingerprint approach that #727 and #729 carried; both are closed. > > Rebased off the #715-#726 chain so it can merge on #574 alone. That chain owns `DirectiveOutcome.warning` / `StepReport.warning` and the `indeterminate` scoring rule; the small amount of each that this condition needs is carried here, scoped to `idle`, and will conflict textually with #724/#725 for whichever lands second. ## One check a selector cannot express ```yaml - await: { idle: true } ``` Has the screen stopped moving? Flows were substituting fixed `wait:` steps for it, which either wait too long on every run or too little on the run that mattered. ## Why it reads pixels as well as the tree Each signal is blind to what the other sees. The tree cannot see presentation-layer motion. An iOS push or modal dismissal commits its hierarchy up front and then animates a layer for a few hundred milliseconds; a cross-fade or a scrim moves no node at all. Measured on a real Android emulator: a page animating a large element continuously reads as a single static `WebView` node, and the existing `await-screen-idle` tool called it settled in **465 ms, five times out of five**. On a deterministic Chromium page whose DOM is byte-identical across the animation, that tool returned `settled: true` in **407 ms** while this condition failed 8/8. Pixels alone would not do either: they cannot see a tree still churning behind an unchanged surface, and anything animated forever (a video, a shimmer) would make a pixel-only settle unsatisfiable on a screen the tree calls ready. Unlike that tool, this cannot be silently stepped over. Readiness is not an acceptance criterion — the verdict belongs to the identity and outcome checks around it — so a screen that never settles still **passes**, carrying a `warning` that says what the green actually bought. That is what makes it safe to persist in a flow: it reports on every run without ever failing one over a video or a shimmer. The only thing that stops a run is a tree source that cannot be read. ## The rule the design follows The absence of evidence is never evidence. - A capture that did not arrive cannot complete the hold. - Neither can a single agreeing pair. Every animation that reverses has a turning point, and two samples straddling it come back identical while the screen is moving. On a live 3 s cross-fade that passed a default-shaped step on roughly **one run in three**; settling now takes two consecutive still intervals, so `minStableMs: 0` still means three reads. - A round is never started without the budget to observe it with. A round begun with nothing left neither captures nor reads, and both absences were otherwise recorded as facts about the device. - No verdict comes from a latch. A screen that settles and then moves again, or goes blank, has not settled. ## `timeout:` is a real bound No describe path takes an abort signal, so the tree read is raced against what is left of the budget. Without that, a wedged ViewInspector RPC ran 2.25 s past an 8000 ms budget, and a busy Android screen ran 5.4-6.0 s against a 3000 ms one. A read that runs out of step budget is the step ending, not the source failing, and only the latter is reported as an environment problem. ## Failure modes stay apart They call for opposite responses. A screen that never stopped moving is a verdict about the app. A tree that could not be read is not, and it names the foreground check first, because a backgrounded app reads identically to an uninstrumented one and relaunching is the wrong repair. A run whose captures never produced a comparable pair still settles on the tree and says so in a warning, rather than passing off half a proof as the whole one. ## Capture routing and tolerance Captures route exactly as the `screenshot` tool routes them, so tvOS and Vega, which have no simulator-server backend but are perfectly screenshottable through `xcrun` and the emulator console, are covered rather than written off. The pixel comparison owns its tolerance rather than borrowing `screenshot-diff`'s. That one holds a baseline stored across sessions, machines and OS versions against a live capture and must absorb real drift; this one holds two captures one poll apart from a single session, where a static screen reads back byte-identical (measured: zero changed pixels across five consecutive pairs on an idle simulator). The margin is load-bearing, because uniform change is all-or-nothing: at the baseline tolerance any cross-fade slower than ~2 s counted zero changed pixels and read as settled mid-animation. ## Not a screen check A dropped tap leaves the source screen perfectly idle. It belongs after the element check that names the destination, never instead of one. #730 documents the pairing. ## Verification - 3307 tests pass; typecheck, `typecheck:tests`, lint and prettier clean. Eleven mutations applied to the source, all eleven caught by tests. - Driven end to end against a real iOS simulator, a real Android emulator, and a Chromium app through a tool-server built from this branch: static screens pass with no warning (proving captures ran), animating screens pass carrying the warning that names the motion, and the pre-existing tool disagrees exactly where it should. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
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>
|
||
|
Jakub Piasecki
|
6d907cc86b |
feat(flow): boot a Chromium instance per launch step (#585)
A `launch:` on Chromium now starts its own Electron instance, so nested e2e flows and mid-flow relaunches work as they do on iOS/Android. Previously the runner booted one app per run and rejected any second chromium launch. A chromium device id _is_ its CDP port, so a launch that starts an app from scratch has to boot one and move the run onto it: `state.device` became reassignable (steps already read it per call), and `state.owned` tracks what the runner booted, torn down in reverse at run end. An instance the runner did not boot — pinned via `--device`, or auto-detected — is attached to, never killed. Relaunching the same app kills its instance and waits for the process to actually exit, or the replacement loses a race against the app's single-instance lock. The runner needs a device before step 1, so `resolveRunDevice` follows a leading `run:` chain to find the launch the run _begins_ with: a fragment composing a chromium e2e flow now boots instead of failing with "No booted chromium device found". A launch declaring no app id for the run's platform stays an error — the run's platform is fixed start to finish. Also fixes a pre-existing teardown leak: the spawned handle is the npm `electron` node wrapper, not Electron, so signalling it left the app and its helpers running (reparented to launchd, still in the dock). Teardown now sweeps the child's process group. **Behavior change:** a fragment leading with `run:` into a chromium e2e flow now boots a fresh instance where it previously attached to a running one; pass `--device` to keep attaching. ## Tests Cover the boot/attach/settle three-way, teardown ordering and the group sweep, device rebinding after a nested launch, same-app retire ordering, cancellation mid-boot, both "no chromium app declared" guards, and the leading-`run:` chain (multi-hop, cyclic, no-launch). The guard, exit-wait and sweep tests are mutation-checked. |
||
|
Jakub Piasecki
|
22033e1418 |
feat(flow): allow to run every flow in a directory from flow run (#576)
`argent flow run` now accepts a directory and runs every flow in it sequentially: ``` argent flow run .argent/flows --recursive ``` Discovery uses the same acceptance rules as `flow list` (lowercase `.yaml`, safe stem, readable file), sorted for a deterministic run order. `-r`/`--recursive` walks subdirectories too, skipping dot-directories and `node_modules` and never following directory symlinks. A directory named `foo.yaml` still batches (stat wins over extension). Batch output is failures-only: each flow prints a `[i/n] <path>` header, its failing/errored steps — numbered exactly as a single-mode rerun would number them — and a per-flow summary; the run ends with a `PASS/FAIL — N flows: X passed, Y failed, Z skipped` verdict. `--json` prints one aggregate object with per-flow reports. Exit code reflects the batch (0 pass, 1 any failure, 2 discovery/config errors). `--output` exports keep the single-run `<output>/<flow>/` layout, keyed by subdirectory in recursive runs so same-stem flows in different directories can't clobber each other's snapshots. **Failure semantics:** a flow failing its steps — or one the tool-server rejects as invalid (bad YAML, unparseable step) — fails alone and the batch continues. An infra error (transport death, unclassified failure) stops the batch and counts the remaining flows skipped. To make that distinction possible, tool-error responses now carry the server's failure signal (`error_code`/`error_kind`) on the wire — on the streaming terminal error line and on JSON error bodies — and the tools client surfaces server-answered errors as a typed `ToolInvocationError`. Against a pre-signal server this degrades to stopping on any throw. Not breaking: single-file runs behave exactly as before. |
||
|
Jakub Piasecki
|
cc2f907ce9 |
feat(flows): resolve run: directives from explicit YAML paths (#567)
A `run:` value is now a YAML path resolved against the directory of the flow file that contains the step, matching the CLI cutover: ```yaml - run: login.yaml - run: ../shared/login.yaml # cross-directory composition now works ``` Bare names fail at parse with a migration hint (`run: login` → `run: login.yaml`). The filename stem stays the fragment's report attribution; reports also carry the as-written path as `target`. Cycle detection compares realpath-canonical paths (same stem in two dirs ≠ cycle; same file via symlink/spelling = cycle). Snapshot baselines are unchanged — keyed by and stored beside the root flow. An uploaded root flow with `run:` steps is now rejected up front with a contract error instead of a mid-run temp-dir ENOENT (its referenced files stayed on the client, and relative resolution in the temp dir could reach host files); same for a relative chromium app path. The recorder emits the sibling form (`run: <name>.yaml`). **Breaking:** existing `run: <name>` directives fail at parse until renamed to `run: <name>.yaml`. Snapshot baselines for a saved flow whose YAML is a symlink now anchor beside the real file (the symlink target's directory) — move any committed `.argent/flows/__baselines__/<flow>/` set beside the real file once, or the next run fails with a missing baseline. A `run:` target must also resolve inside the project root or beside/below the flow file that references it (arbitrary host YAML is no longer reachable), and an uploaded flow with a `snapshot` step is rejected up front (its baselines cannot anchor to the per-call temp dir). |
||
|
Jakub Piasecki
|
a644d52d39 |
feat(flows): run CLI flows from explicit paths (#568)
`argent flow run` now takes an explicit YAML path instead of a saved-flow name: ``` argent flow run .argent/flows/checkout.yaml --platform ios ``` The path may point anywhere on the local filesystem; the filename stem names the run's report/artifacts, so it must be `[A-Za-z0-9_-]+` with a lowercase `.yaml` extension. A bare name fails with the exact replacement command, and `argent flow list` prints paths instead of names. Because sibling `run:` files and `__baselines__` resolve beside the YAML, the CLI now requires the auto-started local tool server and refuses `ARGENT_TOOLS_URL` / `argent link` routing. The `flow-execute` tool stays remotely callable via `name` + `project_root`, and gains a `flow_path` parameter (exactly one of `name`/`flow_path`) that is only honored when the file-input boundary verified it as a real host path — uploads and boundary bypasses are rejected, so it can't execute arbitrary server YAML. Also: the flow-name pattern is now a single shared export from `@argent/registry`, and the recorder composes a `flow_path` call into a `run:` step when it targets a sibling of the recording. |
||
|
Jakub Piasecki
|
2df2fac43d |
fix(flow): error a no-windows iOS read instead of a false hidden pass (#519)
## Problem On iOS, a flow `assert hidden` (and an unmet `when` guard) could pass / green-skip against a screen where the element was plainly visible — whenever the target's view hierarchy can't be read. A flow that launches a non-injectable app (e.g. an Apple system app, where library validation blocks injection) runs to exit 0 with a false pass while `describe` still shows the element on screen. ## Root cause `queryFullHierarchyTree` returned an empty tree as a *successful* read when `getFullHierarchy` yields no windows, and the iOS source never sets a degraded-read hint. The runner's blind-read guard then falls back to `everMatched` alone, so an element that was *never seen* leaves the empty tree "trusted" — and an empty tree is exactly what a `hidden`/absent check accepts as satisfied. ## Fix Throw on a no-windows read instead of adapting it to an empty tree — consistent with the existing unavailable / requiresAppRestart guards. The caller's retry loop rides out a transient blank; a persistent one surfaces as the step's failure (`assert` fails, `when` guard errors) with an actionable reason, never a false pass. Keys on raw `windows`, not the flattened child count, so a genuinely sparse screen (windows present, no matching leaves) still resolves to "not found". ## Tests - New `flow-ios-tree-no-windows.test.ts`: throws on empty / missing windows, returns a tree when windows are present, still throws on an explicit RPC error. - `flow-hidden-blank-reads.test.ts`: adds the never-seen case (element never matched + dark source) → `hidden` fails with the outage reason, not a pass. |
||
|
filip131311
|
5a64e4e7de |
fix(flows): stop a recorded sub-flow pinning the device it was recorded on (#696)
Fixes #607. ## The bug `DEVICE_BIND_KEYS` covered `udid` and `device_id`, but `flow-execute`'s own device parameter is named `device`. So a recorded `tool: flow-execute` step kept the record-time id in the YAML, and on replay the runner never rebound it. Reproduced live on 0.18.1. Recording a cross-project `flow-execute` step: ```yaml steps: - tool: flow-execute args: name: b-only project_root: /…/projB device: BC0026C7-AAE0-490E-8BD4-A345CF65FB1B # survived stripDeviceKeys ``` Then editing that id to one that does not exist — what a flow committed on one machine looks like on another — and replaying against a real booted device: ``` flow-execute { name: pinned, device: BC0026C7-… } [1] ✓ flow-execute { "device": "00000000-0000-0000-0000-000000000000", <-- baked in, not rebound "ok": false, "errored": 1, … "reason": "…[SimulatorServer:00000000-…] simulator-server exited…" } PASS — 1 passed, 0 failed, 0 errored ``` This contradicted three things the code and docs already claimed: that flows store no device id and are portable, that the runner is authoritative on device and drops any id stored in a step, and that device ids are stripped at record time. It also made the raw `tool:` form behave differently from `run:` composition, which has always inherited the run device. That matters more than it looks: the recorder falls back to the raw form whenever the target is not a resolvable sibling, **and always uses it for a remote recording**, since `run:` composition is host-resolved. Remote recordings are therefore the main producer of the affected artifact. ## The fix Add `device` to `DEVICE_BIND_KEYS`. That fixes both halves at once — `stripDeviceKeys` keeps it out of new recordings, and `bindDeviceArgs` replaces an id already committed to a flow file. Safe because `bindDeviceArgs` only injects keys the target tool declares, and across all 75 registered tools **exactly one** declares `device` — `flow-execute` itself. (24 use `device_id`, the rest `udid`; both were already bound.) ### `platform` is deliberately not stripped There is a test pinning this so a later symmetry edit fails loudly. Two independent reasons: 1. It is read **only when no device was given** — `resolveFlowDevice` returns on `opts.device` before touching it, and the chromium boot spec is gated on `!params.device`. Once `device` is bound it is inert. 2. The strip is schema-blind, and `platform` is *not* device-specific on every tool: `react-profiler-analyze` declares its own. Stripping it would silently rewrite a recorded Android profile analysis into an iOS one. ## Verified live ``` replay, flow bakes in the all-zeros id, run given a real device: before -> nested device 00000000-…, ok false, errored 1 after -> nested device BC0026C7-…, ok true, errored 0 record leg -> recorded YAML has no `device:` key ``` Five new tests, each confirmed to fail before the change and pass after: the two unit cases, an integration case through the runner, the record-side case (using a **non-resolvable** target — a resolvable sibling records as `run:`, which carries no args and so could never show this), and the remote-persist case. Full tool-server suite 3091 passing; lint, prettier, `typecheck:tests` and the tool-description gate clean. ## Behaviour changes worth knowing - **Flows with a baked-in `device` stop pinning** and follow the run device. That is the fix and the documented contract, but it is a silent semantic change for anyone who noticed the pinning and relied on it. No migration is possible or desirable — the baked-in id is host-specific and is exactly what made those flows non-portable. - **A nested `flow-execute` can no longer target a device other than its parent's.** Verified nothing in the repo does this (no tracked `.argent` flows, no flow references in the e2e scripts or workflows), and `run:` already behaved this way. The loud case is a nested Chromium flow that used to boot its own Electron instance; the quiet case is a nested step with a different `platform`, which now silently runs on the parent's device. ## Related - #606 (a nested flow's failure does not fail the outer step) is what let the transcript above report `PASS`. Independent in code — #606 is in the result inspection after dispatch, this is in the args before it — so they do not conflict. Worth knowing when reviewing #606 that the "nested run errored" symptom there had a second, separate cause, fixed here. - `extractDeviceArg` in `http.ts` keeps its own parallel `udid`/`device_id` list and also omits `device`, so a top-level `flow-execute` call gets no platform attribution in telemetry. Not touched here — it is telemetry-only, and unifying it would create an import direction that does not exist today. - `project_root` / `flow_file` are absolute host paths baked into recorded steps — the same portability family, but they cannot simply be stripped (the nested call needs them) and rebinding needs a design decision. Filing separately. Co-authored-by: Filip131311 <f.kaminski2000@gmail.com> |
||
|
filip131311
|
e24b7cac20 |
fix(flows): let a nested run's verdict reach the run that contains it (#697)
Fixes #606. ## The bug A flow step that runs another orchestrator reported `pass` whatever the nested run did. `case "tool"` treats any non-throwing result as a pass, and both `flow-execute` and `run-sequence` report failure *in their result* rather than by throwing. Reproduced live — the **same flow**, run two ways: ``` run DIRECTLY -> ok=False passed=0 failed=1 run NESTED via raw `tool: flow-execute` -> OUTER ok=True passed=1 failed=0 step status = pass sub-report right there: ok=False failed=1 ``` The failing verdict was sitting inside the very object being reported as a pass. A second shape lost the verdict entirely: a sub-flow whose `executionPrerequisite` was never acknowledged returns a notice, runs **zero** steps, and also reported a green pass. ## The fix Two shapes, mapped to statuses the runner already has: | nested result | status | why | |---|---|---| | `ok: false` | **fail** | the composed flow ran and its assertions failed — what an inline `run:` composition already produces | | `notice`, zero steps | **error** | never runnable as written; the class the runner already uses for an unreadable fragment or a cyclic reference | | `aborted: true` | **skip** | matches the runner's own rule that a cancelled step is a skip, never a failure | Both fail and error hard-stop — in this runner *every* fail and error does (`state.stopped`), and there is no continue-on-failure concept: a per-step `optional:` is rejected at parse time because `when:` already expresses it. ### `run-sequence` had the same hole Found while reviewing the plan. `run-sequence` has **no verdict field at all** — every failure path (disallowed tool, unsupported operation, unmet `await-ui-element`, a tool that threw) pushes an `error` entry, `break`s, and returns normally. So a flow step whose sequence stopped at step 1 of 8 also reported a pass. Fixed here rather than left as a known identical bug on the same line. ### Why not a blanket `ok === false` rule There is no `ok` contract in this codebase to generalise. The only other soft-verdict tool spells it `success` (`await-ui-element`), `run-sequence` spells it neither way, and `case "tool"` dispatches tools whose results are typed `unknown` or `Record<string, unknown>` — several carrying app-derived payloads. A blanket rule would silently bind all of those, and every tool added later, to "a key called `ok` decides my flow's verdict". `isUnmetUiWaitResult` set the precedent for naming the tool instead. **There is a test pinning this**, so a future blanket refactor trips. Verified exhaustively: exactly one registered tool returns a top-level `ok` in its result — `flow-execute`. In particular `settings-permissions` does *not*; its `{ok: false}` is a private per-`pm`-invocation type that either throws (already an error) or returns `applied`/`skipped` (a legitimate pass). ## Verified live ``` raw-b-fail direct -> ok=False failed=1 nested -> OUTER ok=False failed=1, step fail reason: flow "b-fail" failed: 0 passed, 1 failed, 0 errored (await: …) sub-report attached raw-b-prereq nested -> OUTER ok=False errored=1, step error reason: flow "b-prereq" did not run — its execution prerequisite was not acknowledged: Settings must be open. Add prerequisiteAcknowledged: true to the step's args, or compose with run: instead. ``` The direct and nested verdicts now agree, which is the exact discrepancy in the issue. 11 new tests in a new file; the 5 behavioural ones each confirmed to fail before and pass after. Full suite 3097 passing; lint, prettier, `typecheck:tests` and the tool-description gate clean. ## Reporting shape One failing step carrying a summarising `reason` **plus the whole sub-report in `result`** — the pass path already attached `result`, so this is the same shape with a non-pass status. The nested steps are deliberately *not* spliced into the outer `steps[]`: `run:` can expand inline only because it shares one `ExecState` (one index sequence, one depth base, one device, one baseline dir), whereas a raw `tool: flow-execute` is a separate runner invocation. Splicing would mean renumbering indices and re-homing artifacts — a wire-format change for a bug fix. Nothing is lost: MCP renders `result` for any step that has one, and the CLI (which renders only `reason`) gets the sub-flow's own first failure inside the reason string. No new `StepReport` fields, no wire-format change; older clients render the new line unchanged. ## Merge ordering and conflicts - **Merge #696 (#607) first.** Before it, a nested `flow-execute` could run against a stale baked-in device and legitimately report `ok: false`; with this landed that becomes a parent failure and would read like a regression *caused by* this PR. No code-level conflict — #696 touches `flow-device.ts` and three other test files. - **#578 rewrites this exact block** (`case "tool"`, adding an `evidence` code to each return) and adds `StepReport.failure`/`durationMs`. Whichever lands second should give the new branches an evidence code — likely `nested-flow-failed` / `nested-flow-prerequisite-unacknowledged` / `nested-flow-aborted` — so its CI diagnostics classify the composition case. **#677** also edits `execLeafStep` and adds `StepReport.warning`. ## Behaviour change worth knowing Previously-green flows containing a nested composition that was silently failing will now go red. That is the fix, but it surfaces pre-existing breakage on upgrade. The likeliest one is a recorded raw step missing `prerequisiteAcknowledged`, which becomes a hard error instead of a silent no-op — the reason names both remedies. ## Follow-up, deliberately not bundled Raw `tool: flow-execute` nesting has **no cycle or depth guard**: `MAX_RUN_DEPTH` and the run-stack cycle check cover only `run:`, so a flow whose raw step names itself recurses through fresh runner invocations. Independent of this issue; filing separately. --- > **Stacked on #649** (`filip/flow-deviceless-and-counts`). Both edit the import block of `packages/tool-server/src/tools/flows/flow-run.ts` — #649 widens the `./flow-device` import for its device-optional run, this one adds `./flow-nested-outcome`. Rebased on #649, so review only the top commit. GitHub retargets it to `main` when #649 merges. Co-authored-by: Filip131311 <f.kaminski2000@gmail.com> |
||
|
filip131311
|
2278115028 |
fix(flow): run a device-less flow without a device, and say why its counts are zero (#649)
Fixes #644. ## Before ``` $ argent flow run echo-only # .argent/flows/echo-only.yaml: two `echo` steps [Tool:flow-execute] No booted device found. Pass a device id or platform explicitly. Available devices: 18BE573F-… (ios, Shutdown), <70 more> ``` and when a device *was* booted: ``` PASS on 18BE573F-… — 0 passed, 0 failed, 0 errored, 0 skipped ``` ## After ``` $ argent flow run echo-only # nothing booted Flow "echo-only" › first step › second step PASS — 0 passed, 0 failed, 0 errored, 0 skipped (no test steps) ``` ## Symptom 1 — the device Whether a device is needed is decided from the flow's own steps, before resolution. Classification is **per step kind and defaults to needing one**, so a kind added later inherits today's behaviour instead of silently running against no device — and the `never` binding makes leaving a new kind unclassified a compile error, backed by a test keyed on the step union so it fails to build rather than to run. Two classifications worth calling out: - **`when` always needs a device**, whatever its body contains, because the guard reads one itself (its platform, or its view tree). A `when: {platform: ios}` block containing only narration still resolves. - **`run` needs one without the fragment being read here.** The named flow is resolved at run time; resolving it a second time would duplicate that lookup and could disagree with it if the file changed in between. A deliberate over-approximation: composing a narration-only fragment still resolves a device. Stated in the code and in the tool description. For `tool:` steps the tool's own schema decides, via a superset of the keys the runner injects — `device` is included because a nested `flow-execute` step drives a device without being handed the run's own. A tool that declares no input at all (`stop-all-simulator-servers`) is device-free, and reading its absent schema must not throw — it previously would have. `ExecState.device` is nullable, which is the safety mechanism rather than just typing: the compiler enumerates every site that acts on a device, each of which now says so via one `deviceEnv()` narrowing. `flow-actions.ts`, `flow-visual.ts` and `ActionEnv` are untouched. The executor also re-checks each step against the **same predicate**, so if the scan and the executor ever disagree the step reports it instead of failing obscurely deeper in. A run that resolved no device reports `device: ""`, and neither renderer claims it ran somewhere. ## Symptom 2 — the counters Reproducing this showed the issue's framing is slightly off: `--json` doesn't only disagree with the text, it disagrees with **itself** — counters all zero while its own `steps[]` marks both steps `"pass"`. So the CLI text was a faithful rendering of the server's counters. **The counters are right and stay as they are.** Narration is deliberately excluded, and it should be: after a hard stop every remaining step *including narration* is reported skipped, so counting it would inflate `skipped` on every real failing run — a worse and far more common lie than the vacuous one being reported here. It would also contradict the JUnit model in #578, where echo is `<system-out>` and not a `<testcase>`. What was missing was saying *why* zero. Both the CLI and the MCP renderer now append `(no test steps)`, and only on a pass — a cancelled run can be a FAIL with all four counters zero, where "no test steps" would read as though the failure had no cause. No wire format changes: `steps[].kind === "echo"` already distinguishes counted from uncounted steps, so the JSON is self-describing without widening the status enum. ## Conflicts, for whoever lands second This is a hot area — seven open PRs touch `flow-run.ts`. - **#578** rewrites the CLI summary line I also change (one hunk, one 7-line function, semantically independent — resolution is keep-both), and its `attachFailureDiagnostics` dereferences `env.device.platform`. **It will need a null-device guard**, or every failure in a device-less flow silently degrades to `read-failed` diagnostics. Happy to rebase onto it rather than the reverse. - **#585** rewrites `resolveRunDevice` at the same seam and edits the same description sentence. - **#538** rewrites `flow-device.ts`. Worth noting it makes `resolveFlowDevice` reject a physical iPhone for flows — skipping resolution means a narration-only flow now passes on a physical-iPhone-only host, which I think is desirable but is a second behaviour change on this seam. All new tests are in new files, since every existing flow test file in the blast radius is rewritten by an open PR. ## Behaviour changes a reviewer should weigh - A device-free flow no longer prints `PASS on <id>` **even when a device is booted**. That is the point: otherwise an identical flow reports differently in CI (no simulator) than on a laptop with one open. An explicit `--device` is still honoured and still named. - `--platform` is ignored for a device-free flow, where it previously errored with nothing booted. - A 0.18.1 CLI against a newer server would print `Flow "x" on ` — cosmetic version skew. ## Verification Live against the built CLI: device-less flow with nothing booted (the reported bug) and with a device booted (identical output, no ambient attribution); `--json` showing `device: ""`; explicit `--device` still naming it; a mixed flow and a composed flow both still demanding a device; a mixed flow with a device booted still reporting `PASS on <id> — 1 passed`. 24 new tests. tool-server 3103 passed, CLI 284 passed, MCP 78 passed — no existing test modified. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- > **#697 is stacked on this branch** — it conflicts with this one in `flow-run.ts`'s import block, so it was rebased on top. Merge this first. |
||
|
Hubert Gancarczyk
|
5ac69349f0 |
feat(flow): CSS combinator scopes for flow selectors (#569)
## What
Adds the CSS combinators to flow selectors, read off frames. Any action
or condition can now scope its matches by where an element sits relative
to another:
```yaml
- tap: { text: Delete, within: { id: profile-card } } # CSS "A B" — inside that container
- tap: { role: Switch, next: { text: Wi-Fi } } # CSS "A + B" — the nearest follower
- assert: { visible: { role: Button, after: { text: Danger zone } } } # CSS "A ~ B" — any follower
- assert: { hidden: { any: true, within: { id: empty-state } } } # CSS "*" — the universal selector
```
| Scope | CSS | Reads as |
| --- | --- | --- |
| `within: <sel>` | `A B` | the match's frame sits inside the scope
element's frame |
| `after: <sel>` | `A ~ B` | the match follows the scope element in
reading order |
| `next: <sel>` | `A + B` | as `after`, narrowed to the nearest follower
of each anchor |
Plus `any: true` — the CSS `*` universal selector, accepted only
alongside a scope and never beside `text`/`id`/`role`.
The child combinator `>` has deliberately **no analog**: flow trees
flatten to leaves under one synthetic root, so parent/child structure
does not survive to replay.
## How
- **Geometric, not ancestry-based.** Frames are what survives
flattening, so every scope is resolved from them by `findAll` in
`ui-tree-match`. The synthetic root never counts as a scope element, and
every relation requires a *distinct* node (nothing scopes itself).
- **Reading order is row-band aware.** An element follows the anchor
when it starts below the anchor's bottom edge, *or* shares its row band
and sits entirely to its right. That band rule is what makes `{ role:
Switch, next: { text: Wi-Fi } }` resolve the Wi-Fi row's own switch even
though the taller switch's frame starts a couple of pixels *higher* than
the label's — a raw top-y comparison gets that backwards. `followKind`
is the single classification both the follower test and the nearest-pick
grouping read, so they cannot disagree.
- **`next` is the union over anchors**, exactly as CSS `A + B` is: three
rows on screen give three switches, one per label.
- **Scopes compose and nest**, capped at `MAX_SELECTOR_SCOPES` (6)
counted across the whole relation *tree* — a size bound, not a depth
bound, because three relations branch and the runner's loose-alternative
expansion is exponential in the bare-string scopes it finds.
- **Flow-YAML only.** `await-ui-element`'s strict schema rejects
`within`/`after`/`next`/`any`, so a scope can never be silently dropped
on that path.
- Adding a fourth relation is a **compile error**: `RELATION_RESOLVERS`
is a `Record` keyed by the relation union, so a new scope cannot be
wired through parse/serialize/reporting and then ignored by the match
engine.
## Testing
- **Unit:** full tool-server suite green — **3006 passed / 1 skipped**.
New coverage in `ui-tree-match.test.ts`, `flow-utils.test.ts`,
`flow-selector-scopes.test.ts` (renamed from
`flow-within-selector.test.ts`, now covering all scopes) and a new
`flow-skill-docs.test.ts` that parses every YAML snippet in the skill so
the docs cannot rot. Each fix is mutation-checked: reverting it fails at
least one test.
- **Real-device E2E** (Pixel 3a, Bluesky on Metro 8081, via a branch
tool-server): three flows, 20 steps, all green — geometric ordering both
ways, two `text` asserts that pin `next`'s pick *by identity* out of
five candidates, empty-scope `hidden` semantics, and two observable taps
(`next`-scoped and `any: true`-scoped) that navigated to the *adjacent*
topic rather than the anchor's own.
- **iOS not covered.** Native-devtools does not attach on either booted
simulator in this environment, so *every* flow selector fails there — an
unscoped `{ text: "Video" }` fails identically. Verified environmental,
not caused by this branch: the globally-installed argent reports the
same, direct `SIMCTL_CHILD_DYLD_INSERT_LIBRARIES` injection also fails
to connect, and it survives a forced simulator reboot on both iOS 18.3
and 26.5. Worth re-running the iOS leg once that is healthy.
## Notes for reviewers
Two deliberate deviations from CSS, both documented in the skill:
- **`next` is looser than `+`.** Where `A + B` matches nothing unless
the immediate sibling is a `B`, `next` keeps looking and returns the
nearest match further on. That is what makes it survive the wrapper and
spacer nodes a flattened tree is full of — but a row *missing* the
control you asked for resolves to the next row's rather than failing.
Happy to switch to strict `+` semantics if the loud failure is
preferred.
- **"follows" is not transitive.** Against a tall anchor an element can
follow something that follows the anchor without following it directly,
so nesting `after` scopes is not the same as chaining `~`.
Other things worth a look:
- `nearestAfter` and `afterTester` are direct scans, not indexed. An
earlier sorted index was a *second* implementation of the nearest rule
and the two disagreed on frames near the tolerance; it also measured
2-13x slower than the scan on realistic shapes, with a prune that
provably never fired. The quadratic tail is documented with measured
numbers: 1.2 ms for a single-anchor `next` at the 12k-node tree cap,
~555 ms only when the anchor selector matches essentially every node.
- A bare-string *anchor* cannot self-correct the way the loose fallback
usually does — a decoy `testID` matching your anchor text wins the
identifier pass whenever anything sits after it. The skill tells authors
to spell sibling anchors as maps.
- `MAX_SELECTOR_SCOPES = 6` replaces the previous
`MAX_SELECTOR_WITHIN_DEPTH = 10`, so a `within` chain deeper than 6 that
parsed on the earlier commits of this branch now rejects. Nothing
released depends on it.
---------
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
|
||
|
Jakub Piasecki
|
e23f0e285e |
feat(flow): snapshot cropOn for single-element visual diffs (#516)
`snapshot` gains an optional `cropOn: <selector>` — diff one element's
region instead of the whole screen:
`- snapshot: { name: cart-total, cropOn: "Order summary" }`
- The selector resolves like a directive target: settled tree,
auto-wait, the standard
not-found failure. Selector-only — no point form (a point has no extent
to crop to).
- The frame resolves **before** the capture, and the **cropped** image
is what gets
compared, stored as the baseline, and reported as the `current`
artifact. The pixel
rect is the normalized frame × capture size; the crop is a pngjs
`bitblt` written to
a swept scratch dir and registered only when a failure returns it.
- The baseline key stays on the **full** capture's WxH (device-class
identity), so the
previously unreachable `dimensionMismatch` guard becomes load-bearing:
an element that
grew or shrank fails with "nothing was compared" plus a
crop-fixed-size-containers
hint. Deliberately strict — a 1px layout drift fails on dimensions, not
content (the
same tradeoff as Playwright element screenshots).
- A tree-source outage while resolving `cropOn` errors the step instead
of degrading to
a full-screen compare against a cropped baseline.
- No tool-schema changes; `snapshotKey` and `--output` behavior ride
along unchanged.
Tests: parse/serialize round-trip + rejection cases, and runSnapshot
coverage with real
PNGs (crop dims, key vs. content, artifact retention/sweep, unresolved
selector, abort,
size-drift message, sub-pixel region).
|
||
|
Jakub Piasecki
|
6a53e66a96 |
feat(flow): rotate directive (#546)
# feat(flow): rotate directive
Adds a selector-targeted two-finger rotation directive to flows:
`rotate: { on?, by }`. `on` resolves like `tap` (settled tree +
auto-wait); omitted, the rotation centers on the screen. `by` is
degrees, positive = clockwise. Distinct from `tool: rotate`, which
changes device orientation — the docs and step labels call this out.
## Design
**One continuous gesture, fixed pace, no duration option.** Fingers
orbit at ~90°/300 ms; duration derives from the angle. Unlike pinch
there is no sub-gesture chaining — any angle dispatches as one sweep, so
the angular delta is exact with no settle delays. `by` is parse-bounded
to ±3000°: the largest sweep the 10 s single-gesture envelope delivers
at that pace, derived from the constants rather than picked as policy —
rejected at parse, never silently clamped. A consequence embraced here:
frame count always scales with the angle, so a speed-sensitive recording
stays a raw `tool:` step (the create-flow skill's polish list covers the
conversion).
**Fixed centroid, constant physical radius.** Drifting the centroid
would couple pan into the turn; growing the radius would couple pinch.
Both stay fixed — the trade is a smaller orbit near edges and small
targets, in exchange for a pure rotation.
**Physical circle, not a normalized-space ellipse.** A single normalized
radius on a non-square screen traces a physical ellipse, coupling an
unintended pinch into every turn. `gesture-rotate` grows optional
per-axis `radiusX`/`radiusY` (fractions of width/height with `radiusX·W
= radiusY·H`), and the flow tree adapters surface screen dimensions via
an optional `screen` field on `DescribeTreeData` so the directive can
compute the aspect. Missing dimensions fail soft to the legacy
single-radius call rather than erroring.
**Placement reuses pinch's edge model.** The initial finger axis
(horizontal or vertical) is scored against the same per-platform
system-edge guards, preferring a start parallel to the nearest violated
edge and staying inside the target's axis-matching span. Guards only
score placement — they never shrink the radius, and a violated guard is
attempted, never rejected. The only geometry failure is no positive
on-screen orbit radius.
**Abort correctness.** `gesture-rotate` now honors `ctx.signal`: each
frame checks it, lifts the synthetic fingers with a terminal Up (so a
cancelled run doesn't leave them held on the glass), then throws with
name `AbortError`; the directive maps a mid-gesture abort to the run's
aborted skip rather than a step failure.
**Explicit `inputSchema`.** The radius requirement is a zod `.refine()`
cross-field rule, which `z.toJSONSchema` drops — so the tool ships a
derived schema plus an explicit `anyOf` (`radius` or
`radiusX`+`radiusY`) to keep invalid calls rejected at the schema layer.
**Platform rejections.** Vega is remote-driven (no touch) and Chromium
has no rotate-gesture idiom at all — both fail upfront with guidance to
drive the app's rotate controls directly.
## Tests
Unit tests cover the geometry module (duration derivation, candidate
construction, aspect handling, edge-guard ranking), the directive
end-to-end against a fake device, and `gesture-rotate`'s per-axis radii
and abort path.
|
||
|
Jakub Piasecki
|
f32659df31 |
feat(flow): pinch directive (#545)
Adds a selector-targeted pinch-zoom directive to flows: `pinch: { on?,
scale }`. `on` resolves like `tap` (settled tree + auto-wait); omitted,
the pinch centers on the screen. `scale > 1` zooms in, `< 1` zooms out.
## Design
**Options-map only, no bare form.** `tap: "Map"` is unambiguous; `pinch:
"Map"` isn't (in or out?), so the parser requires `{ on?, scale }` and
rejects a bare selector with authoring guidance.
**Validity checks, not policy caps.** `scale` must be finite, > 0, and ≠
1 (a no-op scale is almost certainly a mistake) — but there is
deliberately no magnitude cap. An extreme scale decomposes
logarithmically into equal-ratio sub-gestures of ≤4× each, chained with
a 250 ms recognizer-reset delay (`scale: 1e6` is just 10 gestures).
**Pure geometry module.** All placement math lives in
`flow-pinch-geometry.ts` as deterministic data-in/data-out functions (no
device access), so tests drive it with synthetic centers, spans, and
edge guards. Per sub-gesture, a horizontal and a vertical candidate are
built from the axis-matching target dimension (normalized X and Y are
different units, so never `min(width, height)`), and the better one is
dispatched.
**Edge safety scores, it never rejects.** Per-platform system-edge
guards (wider on Android's back-gesture zones) demote candidates in
ranking but never fail a step: viable travel beats edge safety (a
risky-but-perceptible gesture beats a safe no-op the recognizer
ignores), then fully-safe with the most travel, then motion parallel to
the violated edge (edge swipes need motion *away* from the edge). Guards
are resolved once per directive and passed as data — the seam for a
future per-device query. The only geometry failure is literally zero
on-screen finger travel; a tiny target is still attempted.
**Centroid drift near edges.** When expanding fingers would leave the
screen, the centroid drifts inward — only on the moving axis, and only
as far as needed. `gesture-pinch` grows optional
`endCenterX`/`endCenterY` to interpolate the centroid alongside
separation; omitted, behavior is unchanged.
**Open-loop by design.** There is no "current zoom" to read back, so the
directive doesn't try to verify the multiplier — flows assert on the
resulting UI instead.
**Platform rejections.** Vega is remote-driven (no touch input) and
Chromium has no uniform pinch-zoom mapping for a desktop app (ctrl+wheel
or app-specific controls) — both fail upfront with guidance to drive the
app's zoom UI directly.
## Tests
Unit tests cover the geometry module (decomposition, candidate
construction, edge-guard ranking), the directive end-to-end against a
fake device, and the `gesture-pinch` centroid-drift interpolation.
|
||
|
Jakub Piasecki
|
a0d1bbb1e7 |
feat(flow): indent nested steps in reports by block depth (#524)
Flow reports rendered every step flat, hiding which steps ran inside a `when:` block or a `run:` fragment. The tool-server now stamps `StepReport.depth` (+1 per block directive, omitted at top level) and the CLI/MCP renderers indent step labels and under-step lines by it: ``` ✓ 1 launch ✓ 2 when visible "What's new" — condition met ✓ 3 tap "Skip" ✓ 4 run dismiss [dismiss] ✓ 5 tap "OK" [dismiss] ``` Invariants: - **Wire-compatible both ways** — a flow with no block directives reports byte-identical to the pre-depth shape; a pre-depth server renders flat as before. - **Skipped ≡ entered** — skipped/errored/aborted blocks report the same depths as entered ones, keeping reports comparable run-to-run. - **Untrusted wire data** — renderers clamp depth (display cap, not a producer bound): hostile values render flat or at the cap, never throw or allocate huge strings. Also fixes a pre-existing bug: under-step warning/artifact lines now stay aligned past 99 steps. Tests pin depth on every producer path (entered/skipped/errored/aborted blocks, `run:` success/failure markers, `run:`-in-`when:`) and renderer alignment incl. the clamp boundary. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Jakub Piasecki
|
3f602a7a9b |
feat(flow): conditional when blocks (#498)
## What
A `when:` block runs a step list only when one condition holds — a UI
condition (`exists`/`visible`/`hidden`/`text`, the await/assert shapes)
or a static `platform` test. **No else**, by design: a block exists to
restore determinism (dismiss an interstitial, reconverge), never to
test two paths. Tap-if-present is a one-step block:
`when: { visible: "Got it" }` + `steps: [tap: "Got it"]`.
No per-step `optional:` — `when:` covers every directive once. The key
is rejected at parse with a pointer to `when:`, not silently dropped (a
Maestro habit that would leave a "can't fail" step hard-stopping the
flow).
## How
- The guard reuses the assert engine at the 1s grace (`timeout` is a
parse error), so a skipped block barely costs a clean run.
- Verdicts are evidence-based: no trustworthy read in the window →
indeterminate → the step **errors** ("could not evaluate" is not
"condition false"); a transient read failure after trusted reads showed
the condition false stays a clean skip.
- `platform` folds `ios-remote` into `ios` — the parser rejects that
spelling, so the fold is the only way a guard matches a remote sim.
- A skipped block reports one line per authored step, same shape for
unmet guard, hard stop, or cancellation; everything goes through
`pushReport`, so when lines stream live. Failures inside an entered
block hard-stop as usual.
- Results carry an explicit `aborted` flag; `ok` keys off
fail/error/cancel, not skip lines.
- Nesting is capped at 20, so a cyclic YAML alias is a structured parse
error, not a stack overflow.
## Testing
`vitest` 2546 pass (17 when-specific); `tsc --noEmit` (tool-server,
argent-mcp, argent-cli), eslint, prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Jakub Piasecki
|
8d633700d7 |
feat(flow): coordinate targets for tap and long-press (#504)
## Summary
This adds raw normalized screen coordinates as first-class targets for
both `tap` and `long-press`. Gestures can now target either an element
selector or a point, while keeping the existing selector auto-wait and
off-screen diagnostics.
Supported forms include:
```yaml
- tap: { x: 0.5, y: 0.6 }
- tap: { on: { x: 0.5, y: 0.6 }, times: 2 }
- long-press: { x: 0.5, y: 0.6 }
- long-press: { on: { x: 0.5, y: 0.6 }, duration: 1200 }
```
## What changed
- Introduced a shared gesture-target parser for selector and point
targets.
- Added raw-point execution for `long-press` and shared point resolution
between tap and long-press.
- Validate coordinates as finite normalized `0–1` screen fractions,
rejecting pixels, malformed points, mixed selector/coordinate maps, and
unknown point keys.
- Canonicalize option-bearing coordinate gestures under `on`, keeping
option keys separate from target fields.
- Preserve coordinate targets in recording summaries and flow progress
reports instead of displaying an undefined selector.
- Added serialization validation so recording cannot persist a flow that
would fail when parsed for replay.
- Updated the flow authoring skill and tool description with the new
coordinate forms.
## Behavior notes
- Raw points act immediately and do not resolve or observe the UI tree.
- Selector targets retain settling, auto-wait, and the existing
`scroll-to` hint when an element cannot be found.
- Observation directives such as `type`, `await`, `assert`, and
`scroll-to` remain selector-only.
- The mixed spelling `tap: { x, y, times }` is now intentionally
rejected; use `tap: { on: { x, y }, times }` instead.
## Validation
- Full tool-server suite: 2,618 tests passed across 266 files.
- Added parser, serialization, recording, touch execution, Chromium
execution, reporting, and malformed-input coverage.
- Tool-server build, test typecheck, ESLint, Prettier, and skill
validation passed.
|
||
|
Jakub Piasecki
|
1a4f89cd5f |
feat(flow): allow to compose all flows via run: (#507)
## Summary `run:` accepted **fragments only** — an e2e flow (one starting with `launch:`) was rejected as a `run:` target. This lifts that: an e2e flow can now be nested via `run:`, its `launch` running inline (a real terminate + relaunch on native). Composition becomes uniform — any flow can be a reusable building block. ## What changed - **`flow-run.ts`**: dropped the `isE2eFlow` rejection in `execRunStep`; cycle / `MAX_RUN_DEPTH` guards still bound recursion. - **`flow-add-step.ts`**: the recorder now captures a recorded `flow-execute` of an e2e sibling as a portable `run: <name>` directive (was: raw `tool:` step). - **`flow-utils.ts`, `argent-create-flow/SKILL.md`**: updated docs/skill that said e2e flows can't be `run:` targets. ## Known limitation — Chromium Chromium `launch` boots an Electron app from a *path*, wired to the top-level flow only — so a nested **chromium** e2e flow doesn't boot its own app. Native nesting is correct; chromium nested e2e is unsupported. ## Testing Rewrote the two affected tests (composition + recorder). Full flows suite green (349 tests); `tsc --noEmit` clean. |
||
|
filip131311
|
92b8c039c3 |
fix(flow): refuse invisible-only text selectors — record coordinates instead (#511)
## The bug (found in the 0.15.0 full-surface QA, on both the Android
emulator and a physical S24)
Recording a `gesture-tap` on an icon-font tab-bar item wrote a flow step
that displays as an **empty selector**, with the coordinates gone:
```yaml
steps:
- tap:
text: # ← actually U+E163, an icon-font Private Use Area glyph
```
and `flow-execute` then **silently passed** against target `""`, tapping
an element no reader of the flow can predict. Root cause (hexdump of the
QA-recorded file: `ee 85 a3` = U+E163): icon fonts expose their glyph as
the element's accessibility label; `deriveSelector` accepted any label
that survived `trim()`, so a PUA code point counted as "stable text".
This violated the rewrite's own documented contract — *"otherwise
coordinates are kept with a warning"* — whose fallback path existed and
worked, but never ran for this case.
## The fix — one predicate, three layers
`hasVisibleText()` (strip Unicode `Cf` format / `Co` private-use / `Cc`
control, then trim; require something left):
1. **`deriveSelector`** — invisible-only label/value is not stable text;
fall through to role, then to the recorder's existing kept-coordinates
path.
2. **`selectorToYaml`** — the empty-text guard previously covered only
the bare-string spelling; it now also rejects empty/invisible text on
the strict `{text: …}` map form, so a hand-built selector fails loudly
instead of writing an unreadable flow file.
3. **`selectorSchema`** — parse-side refinement, so flow files already
recorded broken on 0.15.0 fail with an actionable message ("text must
contain at least one visible character … select by identifier or role
instead") instead of false-passing at replay. This also hardens the
`await-ui-element` tool surface, which shares the schema.
Text that merely *contains* an icon glyph alongside real text ("⟨U+E163⟩
Explore") stays valid throughout.
## Verification
- Unit: `deriveSelector` PUA/ZWSP/mixed cases, serialize + parse
rejection round-trips (explicit `\uXXXX` escapes, no invisible bytes in
the test source).
- Full tool-server suite: **2667 passed**.
- **Live on the Pixel_9 emulator against the original QA repro** (my-app
tab bar, same coordinates): recording now produces `tap: {x: 0.75, y:
0.947}` with the documented *"kept coordinates (brittle)"* warning — the
role fallback was correctly caught by the existing
resolves-to-a-different-element guard — replay passes 2/2 on the
coordinate targets, and executing the old broken YAML now fails parse
with the visible-character error.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
||
|
Jakub Piasecki
|
3d3b82a076 |
feat(flow): regex matching via matches in assertions and selectors (#503)
## What
- Add `matches` as a case-sensitive JavaScript regex comparator for
`await` and `assert` text conditions.
- Support `{ text: { matches: ... } }` in every flow selector slot,
combined with `id` and `role`.
- Preserve regex matchers through parsing, YAML serialization, recording
summaries, and run reports.
## Correctness
- Prevent empty-matchable patterns from matching unlabeled nodes or
empty assertion text.
- Reject unknown raw `await-ui-element` selector keys instead of
silently dropping constraints.
- Reject selector combinations YAML cannot round-trip without changing
their semantics.
- Document native label aggregation accurately and keep action ranking
consistent.
## Maintenance
- Share selector-field and regex-syntax validation across both matcher
parse paths.
- Compile selector regexes once per tree walk and full-match regexes
once per ranking pass.
- Make comparator serialization exhaustive, extract the duplicated flow
test harness, and scope slow-test timeouts narrowly.
## Verification
- Full tool-server suite: 2,588 tests across 263 files.
- Source and test typechecks, ESLint, Prettier, and skill-copy
validation pass.
|
||
|
Jakub Piasecki
|
5082b63082 |
feat(flow): multi-tap via tap times and gesture-tap clickCount (#497)
Flows can now express double-taps declaratively:
```yaml
- tap: { on: Login, times: 2 } # selector double-tap
- tap: { x: 0.5, y: 0.57, times: 2 } # coordinate double-tap
```
The selector nests under `on` so option keys never share a map with
selector fields — zod would silently strip them; mixing errors with a
hint instead. `times: 1` normalizes to absent and the serializer emits
the minimal spelling, keeping parse/serialize exact inverses.
The taps dispatch as **one gesture** via `gesture-tap`'s new
`clickCount` (1–10): N Down/Up pairs ~100 ms apart on touch, escalating
CDP `clickCount` per click on Chromium (what makes `dblclick` actually
fire) — never N separate calls whose RPC gaps could miss the OS
double-tap window.
The recorder carries a recorded `clickCount` into `times`, so a recorded
double-tap can't silently replay as a single tap.
|
||
|
Jakub Piasecki
|
7bf693a573 |
feat(flow): long-press directive (#500)
Press-and-hold on an element — context menus, reorder activation — with
the same resolution behavior as `tap` (settled tree, auto-wait, no
auto-scroll) and the same options shape: a bare/strict selector, or
`long-press: { on: <selector>, duration: <ms> }` with the selector
nested under `on` so option keys never mix with selector fields.
The default 800ms clears both platforms' recognizers (iOS 500ms,
Android ~400ms). Touch platforms dispatch one `gesture-custom` train
(`Down`, then `Up` delayed by the duration) so the hold length is
exact; Chromium maps to a mouse press-hold-release (`gesture-drag`,
from == to) — a desktop context menu is a right-click and is
deliberately not aliased. Vega rejects upfront with the other touch
directives. No coordinate form: a point long-press stays the
`gesture-custom` escape hatch.
|
||
|
Jakub Piasecki
|
551c781fb0 |
feat(flow): support running end-to-end flows without agent (#429)
# Flow E2E
This PR turns flows from replayable tool-call recordings into a
declarative, portable e2e test system that runs headlessly via `argent
flow run <name>` — no LLM in the loop, non-zero exit on failure,
suitable for CI.
Agents can still replay flows via `flow-execute` and get the full output
from each step after it finishes.
## Flow format
Flows are YAML files in `.argent/flows/`, now built from **directives**
interpreted by the runner rather than raw tool calls:
- `launch` — terminate + relaunch the app under test (per-platform app
id map supported); its presence marks a flow as **e2e** (controls its
own start state) vs. a **fragment** (runs against current state,
declares an `executionPrerequisite`, composable via `run:` with
cycle/depth guards).
- `tap`, `type` (focus handshake + optional submit), `scroll-to`
(momentum-free, increments by half the container, `within:` for nested
scrollers), `await`/`assert` (`visible`/`exists`/`hidden`/`text`
conditions), `wait`, `snapshot`, `echo`, and raw `tool:` as escape hatch
— still fully supported, so previously recorded flows keep working
unchanged.
- **Selectors** are `{ text, identifier, role }`; a bare string is a
loose selector resolving identifier-first, then visible text. Any
directive failure hard-stops the flow; later steps report `skip`.
## Execution engine
- Flows store no device id — the runner binds a device at replay
(explicit `--device`/`--platform` or the single booted one).
- Selector resolution runs against **platform-unified UI trees**: iOS
native view hierarchy (full testID coverage, gated on native-devtools
connecting), Android full accessibility hierarchy (full resource-id
coverage), Chromium CDP DOM, and the Vega automation toolkit. A shared
flatten/text-hoisting pass lets `text` assertions read what an
identified container visibly shows. Vega is remote-driven, so touch
directives fail with guidance to use `tv-remote`/`keyboard`.
- Steps wait for the tree to **settle** (two consecutive identical
reads) before acting; `launch` additionally gates on platform
tree-source readiness instead of silently degrading.
## Visual snapshots
`snapshot` diffs a full-resolution screenshot against baselines in
`.argent/flows/__baselines__/<flow>/`, keyed by platform + resolution,
with configurable `maxMismatch` and `--update-baselines`. The status bar
is pinned for the whole run (iOS `simctl status_bar`, Android demo mode)
so clocks/battery don't drive diffs. Baseline/current/diff images are
returned as artifacts on failure (and the written baseline on first
capture/update); a clean pass carries none, keeping reports and agent
context lean.
## Recording
The record-live workflow (`flow-start-recording` → `flow-add-step` →
`flow-finish-recording`) now emits directives automatically: coordinate
taps are rewritten to portable selector `tap:` steps (selector captured
from the pre-tap tree), a leading `restart-app` becomes `launch:`,
`flow-execute` of a sibling fragment becomes `run:`, and device ids are
stripped. The skill instructs the agent to polish the saved YAML
afterwards (fold tap+keyboard into `type:`, swipes-to-reach into
`scroll-to:`, awaits into sugar form).
## Also included
- `argent flow run` / `argent flow list` CLI with per-step report
rendering, artifact materialization, and `--json` output.
- New structured failure codes for flow validation, device resolution,
composition, and devtools readiness.
- Extensive unit test coverage (tree adapters, selector matching,
scrolling, composition, type focus, snapshots, status bar).
|
||
|
Hubert Gancarczyk
|
55587745a7 |
feat(tool-server): implement await-ui-element tool (#396)
## Summary
Adds a `await-ui-element` tool that blocks until a UI condition is
satisfied or a timeout elapses.
This is one of the most impactful missing tools. Without it, agents have
to poll manually with `screenshot → describe → check` loops — slow,
token-heavy, and unreliable (fixed `sleep` delays either over-wait or
fire before the screen settles). `await-ui-element` moves the poll loop
server-side: one call in, a definitive verdict out.
Works on **iOS, Android, and Chromium (CDP)**, polling the same
accessibility / DOM tree as `describe`.
## Conditions
- `visible <selector>` — wait until an element appears on screen
- `hidden <selector>` — wait until an element disappears
- `exists <selector>` — wait until an element is in the tree
- `text <selector> <expectedText>` — wait until the matched element
contains the text
**selector** is `{ text?, identifier?, role? }` — every field provided
must match (case-insensitive substring). Polls internally; default
`timeoutMs` **5000**, default `pollIntervalMs` **400**. Returns `{
success: boolean, elapsed: number }`, with a `note` explaining what was
seen on timeout.
```json
{ "udid": "<UDID>", "condition": "visible", "selector": { "text": "Continue" } }
```
Also usable as a step inside `run-sequence`, so a tap → await-ui-element
→ tap chain runs in a single call.
## Notable details
- **Multi-match aware** — a substring selector can hit several nodes, so
conditions evaluate the whole match set: `visible` holds if any match is
on-screen, `hidden` only if none is (a zero-area container can't flip
the verdict).
- **`hidden` guard** — a selector that matches nothing counts as
already-hidden; the `note` flags when the selector never matched, so a
typo doesn't read as a silent success.
- **Deadline-clamped poll** — a large `pollIntervalMs` can't overshoot
`timeoutMs`.
- **Abort-aware** — both the `time` sleep and the poll loop stop
promptly on cancellation.
## Changes
- `tools/await-ui-element/index.ts` — new tool (factory, mirrors
`describe`'s service resolution)
- `utils/setup-registry.ts` — registers the tool
- `tools/run-sequence/index.ts` — allows `await-ui-element` as a step +
arg-doc + example
- `test/await-ui-element.test.ts` — new test suite
- `skills/rules/argent.md` + `argent-device-interact/SKILL.md` —
document `await-ui-element`, replace the "don't poll `screenshot`"
guidance with it
## Test plan
- [x] `time` sleeps and succeeds without touching a device
- [x] `visible` succeeds once the element appears; times out with a
diagnostic note when it doesn't
- [x] `exists` / `hidden` / `text` happy paths; `text` timeout note
reports last-seen text
- [x] `hidden` instant success **and** the "selector never matched" flag
- [x] poll sleep clamped to the deadline (large `pollIntervalMs` can't
overshoot `timeoutMs`)
- [x] prompt cancellation aborts both the poll loop and the `time` sleep
- [x] multi-match evaluation (`findAll` / `evaluateMatches`)
- [x] schema validation (required fields per condition, `durationMs`
cap)
|
||
|
Hubert Gancarczyk
|
0edefebe06 |
fix(telemetry): orchestator sub-tools invocations was missing ai_client and platform attributes (#400)
## Summary Fixes telemetry attribution for tools dispatched by orchestrators. `run-sequence`, `flow-execute`, and `flow-add-step` run their steps via `registry.invokeTool()` directly, which minted a fresh, unregistered invocation id per nested call — so the `ai_client` / `platform` the HTTP layer captured for the outer request never reached the nested gestures, and they were recorded as **anonymous** in telemetry. Remove `opt_out` event that is a leftover. ## Changes - Add an optional `recordChildInvocation` recorder to `InvokeToolOptions` / `ToolContext`, bound by the HTTP layer to the outer request's attribution. - New `invokeSubTool` helper: registers each sub-tool under a minted id inheriting the request's AI client, with the platform re-derived from the sub-tool's **own** device arg (`udid` / `device_id` / `avdName`); forwards the recorder so propagation survives arbitrary nesting (e.g. `flow-execute → run-sequence → gesture-tap`). - Route the three orchestrators' sub-dispatch through - Refactor platform inference in the HTTP layer into a shared `platformFromArgs` helper (also hardened to not throw on a malformed device arg). - Remove opt_out event leftover Why re-derive platform per child: an orchestrator like `flow-execute` has no platform of its own and a single flow can target several devices, so the child's device arg is the only correct platform source; the parent's platform is the fallback. |
||
|
Hubert Gancarczyk
|
8483fe23f9 |
feat: add telemetry (#274)
## Summary Adds anonymous, opt-out telemetry for Argent installer, update/uninstall flows, CLI telemetry controls, tool-server lifecycle, and tool invocation health signals. ## What Changed - Added a new private `@argent/telemetry` package - Introduced typed telemetry events with runtime property allowlisting/sanitization. - Added lazy anonymous identity creation in `~/.argent/telemetry-id`. - Added consent handling via: `argent telemetry enable`, `argent telemetry disable`, `argent telemetry status` - Added local debug logging with `ARGENT_TELEMETRY_DEBUG=1`. - Tracks installer funnel events for init, global install/update decisions, editor selection, allowlist decisions, skill installation, rules/agents copy, update, and uninstall. - Tracks tool-server start/stop and tool invocation/complete/fail events. - Adds per-tool invocation IDs so request metadata can be correlated without relying on ordering. - Records only coarse platform metadata (`ios` / `android`) for device tools - Ensures short-lived commands drain telemetry with bounded shutdown before exit. - Wires telemetry package into bundling and injects full Argent CLI version metadata at build time. - Added central `FAILURE_CODES` and `FailureSignal` helpers in `@argent/registry`. - Attached structured, sanitized failure metadata across registry errors, tool-server tools/services, HTTP validation/dependency gates, installer flows, and `argent run`. - Added telemetry allowlisting for safe failure fields only: static error codes/stages, coarse area/kind/command, bounded exit codes, allowlisted signals/spawn codes, and coarse network failure classes. - Split CLI-local `argent run` failures into `cli:run_fail` so they are not double-counted as tool failures. - Added installer telemetry for failed package actions, update/uninstall failures, rules/agents copy results, and skill refresh results. - Made telemetry consent writes atomic and kept telemetry status side-effect free via `peekAnonId()`. - Adds a coarse, anonymous signal to tool telemetry: which AI coding tool is driving the MCP server (Claude Code, Codex, Cursor, Gemini, VS Code, Windsurf, Zed, opencode, Copilot, or other). ## Tests Added / Updated - Telemetry unit coverage for consent precedence, identity creation, base props, sanitization, PostHog config, debug logging, shutdown, opt-out, and forget behavior. - Registry/tool-server tests for invocation IDs, platform metadata boundaries, startup failure flushing, and tool lifecycle telemetry. - Installer tests for final telemetry drain, update action classification, and uninstall identity cleanup ordering. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ignacy Łątka
|
f3d108a5a1 |
feat: Argent Lens (#271)
Adds a UI for proposing and choosing between multiple agent-generated variants on a running app. **MCP tools** (`packages/tool-server/src/tools/variants/`) - `propose_variant` — non-blocking; agent stages a variant for an on-screen element (text/label/identifier/role matcher). - `await_user_selection` — one blocking call; parks until the human submits, then returns the picks + free-form annotations + a global comment. **Preview UI** (`packages/ui`) - Floating cards anchored to live element frames via the existing `describe` tree, with connector lines. - Comment-mode inspector: hover spotlight (blur+dim everything else), right-click any element to pin a comment. - Sticky padded hitbox + trajectory-aware dismissal so focus doesn't drop on cursor jitter. - Footbar [Complete] [Clear], confirm modal for destructive actions, green toast on submit. - Single source of feel constants in `theme.css`. **Electron host** (`packages/preview-window`) - New workspace package. Frameless transparent BrowserWindow at fixed size; the squeeze open/close is a CSS `transform: scaleY` on `<html>`, GPU-composited — no OS-level resize. - Tool-server spawns it on `await_user_selection` park, reuses it across re-awaits, and asks it to animate-close on submit (1s grace so the toast is visible). - Stdio-line IPC (no extra ports). Ephemeral throughout — the renderer hits the tool-server's already-ephemeral `/preview/*` HTTP. --------- Co-authored-by: Filip131311 <contact@filipkaminski.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
filip131311
|
c01f41cd5f |
feat(remote): input-side file boundary — flows, screenshot-diff, profiler reports, workspace tools (#319)
## Why PR #278 fixed the **output** half of the remote file boundary: files a tool *produces* travel to the client as `ArtifactHandle`s. But seven tools still break when the tool-server runs on a different machine than the agent, in one (or both) of two ways: - **Output-side leftovers** — `native-profiler-analyze` and `screenshot-diff` still returned raw host paths (and embedded them in report/summary prose), so the agent can't open them remotely. - **Input-side gap** — nothing moved a file the *agent* owns onto the tool-server. Any tool taking a client-local path as a parameter (`screenshot-diff` baselines, the three flow tools' `project_root`, `react-profiler-component-source`, `gather-workspace-data`) silently read/wrote the wrong machine's disk. Flows were fully broken in remote mode: the YAML landed on the server's disk under the agent's path. ## What ### Output side (apply the existing pattern) - **native-profiler-analyze** — `reportFile` is now an `ArtifactHandle` (mirrors `react-profiler-analyze`'s `fileArtifact()`); the inline report now says *"use the Read tool on the `reportFile` path in this result"* instead of embedding the server path. - **screenshot-diff** — `diffPath` / `contextDiffPath` are now `ArtifactHandle`s; the MCP adapter renders the context-diff image from materialized bytes (legacy string-path results from older servers still render co-located). The summary references the result fields instead of embedding host paths. ### Input side (new, symmetric mechanism) A declarative **file-input boundary**, mirroring the artifact gate in the opposite direction: - A tool declares `fileInputs: [{ target, path: "${param}…", kind }]` on its `ToolDefinition`; the declaration ships to the client via `GET /tools`. - Before sending a call, the client (`@argent/tools-client`, shared by CLI and MCP) interpolates each template from the args and replaces the target arg with a wrapper: `{ __argentFileInput, path, size, mtimeMs, content? }`. File bytes are base64-inlined **only when routed to an external tool-server** (`argent link` / `ARGENT_TOOLS_URL`) — unlinked local calls never pay for encoding. - The server resolves wrappers **before zod validation**, so tools always execute against a plain local path: - path matches on the server's own filesystem (existence for dirs, size+mtime for files) → used **in place**, zero copies — the exact dual of the artifact materializer's co-located gate; - otherwise `kind: "file"` content is materialized into a temp file (with size-integrity check, 32 MB cap); - `kind: "directory"` (trees that can't ride in a call) → 422 with actionable remote-mode guidance; - `kind: "probe"` → never fails; the tool learns host-presence via `ctx.fileInputs` and adapts. - The reverse channel: a tool whose output belongs in the **agent's project** returns a `__argentClientFile` directive (path + content); the client writes it and rewrites the directive to the path. Writes are constrained to `**/.argent/flows/<safe-name>.yaml` so a tool-server can't direct writes anywhere else on the client machine. ### Tool migrations | Tool | Boundary use | |---|---| | `flow-start-recording` | `probe` on `project_root`: present → host persistence exactly as before; absent (remote) → recording kept in server memory, YAML returned as a client-write directive | | `flow-add-step` / `flow-add-echo` / `flow-finish-recording` | follow the session's persist mode; every mutation returns `savedTo` (path locally, directive remotely) so the agent-side file is always current | | `flow-execute` / `flow-read-prerequisite` | `file` input derived from `${project_root}/.argent/flows/${name}.yaml` into an internal `flow_file` param — read in place locally, uploaded when remote | | `screenshot-diff` | `baselinePath`/`currentPath` as `file` inputs; `outputDir` now **optional** (temp default) and probe-ignored when absent on the host | | `react-profiler-component-source`, `gather-workspace-data` | `directory` gate — unchanged locally; clear error remotely instead of a silent empty AST index / all-nulls snapshot | ### Version skew Both halves degrade to today's behavior: an old client sends plain strings (the resolver passes them through, tools take their legacy paths), and an old server doesn't advertise `fileInputs` (the new client never wraps). Wrappers on undeclared params fail the tool's own schema validation, so nothing can be smuggled through free-form args. ## Tests **Unit** (all green: registry 68, tool-server 992, tools-client 107, argent-mcp 66, argent-cli 117): - `tool-server/test/file-inputs.test.ts` — in-place gate, stat-mismatch → upload, truncated-upload rejection, directory gate, probe semantics, undeclared-target smuggling. - `argent-tools-client/test/file-inputs.test.ts` — wrapping (stat-only vs inlined content), multi-param template derivation, explicit-override respect, directive writes incl. path-validation refusals (relative, traversal, outside `.argent/flows`, bad charset/extension). - `tool-server/test/flows/flow-remote-recording.test.ts` — remote recording stays in memory + returns directives, nothing written on the host, replay/read from a boundary-resolved `flow_file`. - Updated existing screenshot-diff / native-profiler tests to the artifact-handle contract. **Local / co-located (unlink scenario)** — isolated tool-server + CLI on an iPhone 17 Pro simulator: full flow record→finish→read-prerequisite→execute loop (YAML written via the same path as before, in-place gate hit, no byte copies); screenshot-diff with and without `outputDir`; wire-level checks of the unlinked client shape (wrapper without content → in-place read) and the legacy plain-string shape; missing-file → 422 with a precise message. **Remote (Lima Linux VM as the agent machine, tool-server on the host Mac, `--host` bind + bearer auth)** — via both the CLI and the real MCP stdio server: - flow recording from the VM: YAML lands **in the VM's project**, `savedTo` rewritten to the VM path, *no* agent-path directories created on the host; replay from the VM uploads the YAML and drives the host simulator. - `screenshot` materializes into the VM temp cache; `screenshot-diff` round-trips both directions (VM baseline uploaded, live capture on host, diff artifacts downloaded back; MCP returns `image,text` content with the context diff inlined). - `gather-workspace-data` / `react-profiler-component-source` with VM-only paths → the new actionable 422. ## Notes - `native-profiler-analyze` was not exercised live (xctrace export is unusable on this machine's iOS 26.4 simulators — known, unrelated); its change is the same one-line `fileArtifact` pattern `react-profiler-analyze` already ships, covered by unit tests. - `react-profiler-analyze`'s own `project_root` (AST resolve stage) is left as-is deliberately: it degrades gracefully (report still renders) and gating it would break currently-working remote analysis. Tracked as follow-up alongside nested invocations (`run-sequence`/`flow-execute` steps bypass the HTTP boundary by design). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Ignacy Łątka
|
74c2065ca0 |
fix: multiple minor bugs (#194)
## Human Summary
This PR aims to fix bugs that are benign for now, could become serious
in the future:
- multiple CSRF via CORS misconfiguration on `tool-server`
- multiple SSRF that were possible due to the CSRF
- path traversal via flow creator
- multiple shell injections
- lack of auth on `MCP/CLI <-> tool-server` path (bug details discussed
in person). After the other fixes this is somewhat redundant, but
doesn't hurt and protects against theoretical access attempts from
within other processes running on the user's device.
## AI Summary
<details>
Bundles six independent fixes that together close a CSRF cluster on the
local tool-server. The vulnerabilities chain because the HTTP API has no
auth and serves wildcard CORS, so any web origin (Firefox
unconditionally; Chrome via PNA-disabled or loopback-served) can drive
the tool-server from a cross-origin tab. Each individual fix lives on
its own branch (linked below) and stands alone — this PR exists so the
cluster can be reviewed and shipped as one piece.
### Fixes
1. **`fix(http): require auth token + drop wildcard CORS`** —
per-process 32-byte hex Bearer token, generated by the launcher,
persisted in `~/.argent/tool-server.json` with mode `0600`, required on
every route except `/preview`. Constant-time compare. Wildcard CORS
dropped. Threaded through MCP server + CLI + tools-client.
Backwards-compat: when `ARGENT_AUTH_TOKEN` is unset (e.g. `npm run
dev`), the server logs a one-shot warning and skips the gate.
2. **`fix(http): reject non-loopback Host headers`** — middleware
(before the auth gate) returns 403 for any request whose Host is not
`127.0.0.1`, `localhost`, or `::1`. Closes DNS-rebinding bypass.
3. **`fix(ios-profiler-start): prevent shell injection via device_id`**
— `detectRunningApp(udid)` no longer interpolates `params.device_id`
into `execSync` template strings. Both `xcrun simctl spawn ${udid} …`
and the piped `xcrun simctl listapps ${udid} | plutil …` are rewritten
to `execFileSync` array form, with the pipe to `plutil` happening in JS
(input option) rather than the shell.
4. **`fix(flows): validate name to close path-traversal in
getFlowPath`** — `getFlowPath()` requires `^[A-Za-z0-9_\\-]+$` and
verifies `path.relative(getFlowsDir(), filePath)` does not start with
`..`. Centralizing the check in `getFlowPath` covers every caller
(start-recording, add-step, insert-echo, finish-recording, run,
read-prerequisite) without per-tool churn.
5. **`fix(source-resolver): reject paths outside projectRoot +
non-source extensions`** — `readSourceFragment()` previously accepted
any `file` field on the SourceLocation it was given (the field traces
back to a React fiber's `_debugSource.fileName`, which is
attacker-controllable). Now: containment check via
`path.relative(projectRoot, abs)` plus an extension allowlist
(`.js/.jsx/.ts/.tsx/.mjs/.cjs/.json`).
6. **`fix(source-maps): reject non-loopback URLs (close SSRF)`** —
`doRegister()` validates `sourceMapURL` before fetching: only
`http://`/`https://` URLs whose hostname is loopback. Closes a
blind-SSRF probe of the host network (cloud metadata at
`169.254.169.254`, intranet pages, etc.).
### Individual branches
- `fix/auth-token-drop-cors`
- `fix/host-header-validation`
- `fix/profiler-command-injection`
- `fix/flow-name-validation`
- `fix/source-resolver-traversal`
- `fix/source-map-ssrf`
If you want to land them as separate PRs, the integration branch can be
dropped after each lands.
## Test plan
Unit tests (already in CI):
- [x] `npm run build` passes (workspace tsc clean)
- [x] `npm run test -w @argent/tool-server` — **510 passed (was 473 on
`main`)**, +37 new tests across:
- `test/http-auth.test.ts` — 10 tests (auth, CORS removal, dev-mode
escape hatch)
- `test/http-host-validation.test.ts` — 7 tests (loopback allow,
DNS-rebinding reject)
- `test/flows/flow-utils.test.ts` — +5 tests (path-traversal name
rejection)
- `test/metro/source-resolver.test.ts` — +5 tests (containment +
extension allowlist)
- `test/metro/source-maps-ssrf.test.ts` — 10 tests (loopback-only allow)
Live end-to-end (run locally against this branch's bundled artifacts):
- [x] Launcher spawns tool-server; `~/.argent/tool-server.json` contains
a 64-char hex token and is mode `0600`
- [x] `GET /tools` without `Authorization` → **401**
- [x] `GET /tools` with wrong token → **401**
- [x] `GET /tools` with correct token → **200** (61 tools)
- [x] Request with `Host: attacker.example` → **403**
- [x] No `Access-Control-Allow-Origin` header on responses
- [x] Cross-origin command-injection probe via `device_id=\"x;touch
/tmp/marker;#\"` does NOT create the marker file (Vuln 4 closed)
- [x] `flow-start-recording` with `name=\"../../../tmp/escape\"`
rejected with `Invalid flow name`
- [x] `flow-start-recording` with a valid name still works
- [x] `tools-client.fetchTools()` and
`tools-client.callTool('list-simulators')` succeed via the auth-token
threading (the path the MCP server uses internally)
- [x] MCP-stdio entry point (`startMcpServer`) end-to-end: `initialize`
→ `tools/list` (61 tools) → `tools/call('list-simulators')` returns
content. Confirms the launcher → token → fetch → tool-server flow is
intact for the production `argent mcp` path.
## Notes for reviewers
- `/preview` is intentionally exempt from the auth gate (it was not
gated before; tightening it deserves a separate PR).
- The `ARGENT_AUTH_TOKEN`-unset escape hatch keeps `npm run dev`
workable; production `argent mcp` always passes a token via env, so the
warning never fires there.
- The auth bundle changes the public return shape of `ensureToolsServer`
from `Promise<string>` to `Promise<{ url, token }>`. The two in-tree
callers (`argent-mcp/mcp-server.ts`,
`argent-tools-client/tools-client.ts`) were updated; external consumers
using `ARGENT_TOOLS_URL` should also set `ARGENT_AUTH_TOKEN` to match
the server.
</details>
|
||
|
Hubert Gancarczyk
|
de0dbbda66 |
feat(tools): implement screenshot-diff tool (#229)
## Summary Adds a new `screenshot-diff` tool for comparing mobile screenshots during UI QA and visual regression checks. The tool can compare two saved PNGs, or compare one saved PNG against one live full-resolution device capture, then returns a compact text summary and generated diff artifacts. ## Context Mobile UI validation often needs more than a single screenshot: small layout, color, typography, copy, clipping, and regression changes are easy to miss manually. This feature gives agents a deterministic before/after comparison path while keeping `screenshot-diff` as supporting evidence. ## What Changed - Registered a new `screenshot-diff` tool in the tool server. - Supports saved PNG comparisons and one-sided live capture via `captureBaseline` or `captureCurrent`. - Adds PNG diffing with fixed internal defaults, changed-region detection, status-bar-band ignoring, full-size diff output, and downscaled context diff output. - Adds optional OCR/text analysis for moved, appeared, disappeared, changed, and restyled text. - Updates MCP content rendering to attach the downscaled context diff image plus the text summary. 1. Capture a stable full-resolution baseline. 2. Perform an interaction or apply a UI change. 3. Compare the current screen against the baseline. 4. Review a summarized diff plus generated artifacts. |
||
|
Ignacy Łątka
|
11df817616 |
test: drop real-time timeouts from the slowest unit tests (#236)
The slowest unit tests were waiting on real wall-clock timers, not doing
work. This makes them deterministic and fast with **no production
behavior change and no production test seams**.
## Speedup
**Suite-level**
| Scope | Before | After | Faster |
|---|---|---|---|
| tool-server — wall clock | 2.72s | ~1.3s | ~2× |
| tool-server — aggregate test time | 9.70s | 4.03s | 2.4× |
| argent-mcp — aggregate test time | 0.90s | 0.28s | 3.2× |
**Per file**
| File | Before | After | Faster |
|---|---|---|---|
| simulator-server-blueprint.test.ts | 2.55s | 0.05s | ~50× |
| boot-device-hardening.test.ts | 2.17s | 0.18s | 12× |
| boot-device-spawn-error.test.ts | 1.11s | 0.005s | ~200× |
| boot-device-hotboot.test.ts | 1.01s | 0.06s | 16× |
| fetch-timeout.test.ts | 0.81s | 0.18s | 4.5× |
| flow-tools.test.ts (delayMs test) | 0.31s | 0.04s | ~7× |
**Worst individual tests, before → after**
| Test | Before | After |
|---|---|---|
| rejects the boot promise…when spawn emits error | 1.06s | ~1ms |
| reports the emulator crash error… | 1.00s | ~2ms |
| falls back to cold boot when hot-boot child exits early | 1.00s | ~1ms
|
| reports a signal-terminated emulator (SIGSEGV)… | 1.00s | ~1ms |
| 5× simulator-server-blueprint tests | ~0.50s ea | ~0.001–0.02s ea |
| throws after retries when POST never responds | 0.60s | 0.14s |
| sleeps the step's delayMs | 0.31s | 0.04s |
No test exceeds ~0.7s anymore (the slowest,
`workspace-reader-integration`, is genuine subprocess I/O — `node
--version` etc. — left as-is on purpose; it is not a timeout).
## What changed
- **`boot-device.ts` (only production change):** the three hardcoded
poll cadences are extracted into a single immutable
`BOOT_POLL_INTERVALS_MS` constant (values unchanged: 1500/1000/500ms)
and referenced at the four `setTimeout` sites. Pure readability
extraction — **no mutable state, no exported test hooks, no runtime
behavior change.**
- **Boot-device tests:** the timing-sensitive tests now use vitest fake
timers (`vi.useFakeTimers()` + `vi.advanceTimersByTimeAsync`).
`boot-device-hardening` / `-spawn-error` add the same partial
`vi.mock("../src/utils/adb")` already used by
`boot-device-hotboot.test.ts`, so pre-flight does no real filesystem I/O
and the timers drive deterministically.
- **`simulator-server-blueprint.test.ts`:** `signalReady` now emits
`stream_ready` then `api_ready` (matching the real streaming server's
ordering) so the blueprint resolves immediately instead of waiting out
the 500ms `STREAM_GRACE_MS` fallback. Adds a dedicated fake-timer test
that explicitly covers the non-streaming grace path (coverage gained,
not lost).
- **`fetch-timeout.test.ts` / `flow-tools.test.ts`:** smaller timing
knobs; same behaviors asserted.
## No regressions
- 1011/1011 tests pass (tool-server 656, argent-mcp 53, argent 6,
registry 64, tools-client 1, installer 231); `tsc --build` and Prettier
clean. The converted boot-device, simulator-server-blueprint, and
flow-tools tests are deterministic under fake timers (verified across
stress runs).
- No production behavior change; `simulator-server.ts` is byte-identical
to `main`; net +1 test.
- Heads-up: `argent-mcp/test/fetch-timeout.test.ts` has a
**pre-existing** macOS flake (~10–20% rate; `connect EADDRNOTAVAIL
127.0.0.1:<port> - Local (0.0.0.0:0)` — kernel source-port exhaustion
under rapid-fire loopback fetches). Stress-testing showed it on `main`'s
pristine file too (2/10 failures vs 1/10 on this branch), so this PR
does not introduce or worsen it. Fixing it (e.g., EADDRNOTAVAIL retry,
or moving the test off real loopback fetch) is worth a follow-up.
|
||
|
Ignacy Łątka
|
2180dd8e6a |
ci: typecheck test files in CI (#202)
We've had lots of type errors in our tests, scripts and other non-core
code despite all CI being green. This PR fixes that.
This PR also removes some redundant features like `@ts-check` inline
directives which have been replaced by proper ts config files.
<details>
## AI Summary
Audit of every CI workflow against every package's `package.json` and
`tsconfig` surfaced several gaps where CI did partial coverage. This PR
closes them and fixes every pre-existing type error the new gates
surfaced.
## Gaps closed
| # | Gap | Fix |
|---|---|---|
| 1 | `tsc --build` only covers `src/**`; test files (`test/**`,
`tests/**`) never typecheck. Vitest transforms tests with esbuild —
strips types instead of checking them. | New `tsconfig.test.json` per
test-bearing package (`composite: false`, `noEmit`, `rootDir: "."`,
includes `src/**` + `test/**` (or `tests/**`) + `vitest.config.ts`). New
`typecheck:tests` npm script per package. New CI step `npm run
typecheck:tests --workspaces --if-present`. |
| 2 | Build/publish scripts (`scripts/*.{cjs,mjs}`,
`packages/argent/scripts/*.cjs`, `packages/skills/scripts/install.js`) —
JS, never typechecked. `sync-readme.cjs` runs in `prepack`;
`postinstall.cjs` runs on every install; `bundle-tools.cjs` is
build-critical. A typo only surfaces at runtime. | Root
`tsconfig.scripts.json` with `allowJs` + `checkJs` + `noImplicitReturns`
covers all script paths. Root `typecheck:scripts` npm script. New CI
step. Per-file `// @ts-check` comments removed — tsconfig drives
inclusion. |
| 3 | `argent-cli` had no `typecheck:tests` script. When tests get added
there they'd silently bypass the gate. | Added `tsconfig.test.json` +
`typecheck:tests` script. `--if-present` would have skipped it anyway,
so this is future-proofing. |
| 4 | Local `prettier --check .` walked into the `argent-private`
submodule and reported violations there. CI didn't see this because
`actions/checkout@v4` doesn't init submodules — so local `prettier
--check` did not match CI behavior. | New `.prettierignore` excludes the
submodule plus `dist/`, `node_modules/`, lock files, `tsbuildinfo`.
No-op in CI; aligns local with remote. |
## Test type errors fixed
These were already present. The `npm test` passed because esbuild
stripped the offending types:
- **argent-installer**: widen `scope` literal type for
dead-code-elimination guard; non-null `addAllowlist!`/`removeAllowlist!`
(optional methods on adapter).
- **argent-mcp**: add `.js` extensions for NodeNext module resolution.
- **registry**: broaden `StaticBlueprintResult` blueprint api from `{
id; deps? }` to `Record<string, unknown>` so existing tests can pass
arbitrary api shapes.
- **tool-server**:
- non-null `zodSchema!` on tool defs that always carry one
(`launchAppTool`, `restartAppTool`, etc.)
- cast sentinel `"ignored"` strings to `DeviceInfo` in factory-rejection
tests
- match the 4-arg `dispatchByPlatform<IosServices, AndroidServices,
Params, Result>` signature (tests still passed the old 3-arg shape)
- `assertFlowRunResult` type guard for the `FlowRunResult |
FlowPrerequisiteNotice` union before `.steps` access
- swap broken `typeof import("supertest").default` for static `import
supertest` (supertest is `export = supertest`)
- fill in the 4 fields missing from a `NativeProfilerSessionApi` mock
(`xctraceProcess`, `recordingTimedOut`, `recordingExitedUnexpectedly`,
`lastExitInfo`)
- cast a vitest `Mock` to a plain function at one direct call site where
the `Procedure | Constructable` union loses callability
- fix `update-checker.test.ts` reading `../../package.json` (off-by-one
— went past tool-server; was relying on a Vite resolver quirk)
- replace one cross-package `../../../registry/src/index` import with
`@argent/registry` so registry's private `services` field isn't compared
between src and dist declarations
All fixes are type-level. Full `npm test --workspaces --if-present`
still reports **991 passed across 87 test files**.
</details>
|
||
|
Ignacy Łątka
|
bea43f555a |
feat: per-step delayMs on flow steps (#198)
- Tool steps in a flow YAML may carry an optional `delayMs` that sleeps that long *before* the step runs; `flow-add-step` exposes the same field. - New `longRunning?: boolean` field on `ToolDefinition`; when set, the MCP fetch wrapper bypasses its 30s per-request timeout (`fetchTimeoutMs: null`) so long invocations aren't aborted. `run-sequence` and `flow-execute` opt in. |
||
|
Ignacy Łątka
|
bef319134e |
fix: add project_root arg to flow-start-recording (#114)
* fix: detect project root via .argent dir instead of git Flow tools previously shelled out to 'git rev-parse --show-toplevel' to locate the project root, which failed outside git repos and pulled flows from the wrong place in nested worktrees. Walk up from cwd looking for an existing .argent/ directory; fall back to cwd so the first flow-start-recording still creates it in place. * fix: skip $HOME when walking up for .argent/ The MCP launcher stores its own state in ~/.argent/ (logs, tool-server pid). Without this guard, running any flow tool from anywhere under $HOME — but outside a real project with its own .argent/ — would stop at the home directory and treat it as the project root, mixing personal MCP state with flow files. * revert: restore getGitRoot in flow-utils The walk-up .argent approach is fundamentally broken because the tool-server is a singleton daemon — its process.cwd() is not the calling project's cwd. Reverting both |