mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
fix/docs-high-severity
324 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ignacy Łątka
|
00e3e07cf9 | Merge origin/main into fix/docs-high-severity | ||
|
Ignacy Łątka
|
6e1e1c2770 |
docs(tool-server): correct why auth can be disabled
The empty-string token is no longer just an npm-run-dev artifact: the launcher ships it on purpose for `argent server start --no-auth`. Say what the warning actually guards (non-loopback reachability) instead of implying the flag does not exist. |
||
|
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
|
efae6a8fb6 |
feat(tools): opt-in emulator sound on boot-device + boot-sound default flag (#813)
Field report ([Slack thread](https://swmansion.slack.com)): when a session needs to test audio, it is very non-obvious that argent boots the Android emulator muted — the agent (and the user) burned significant time discovering it. The mute default is intentional (several agent-driven emulators all playing sound on the host would be worse), so this PR keeps it and instead makes it **discoverable and overridable**: ## 1. `sound` argument on `boot-device` (default `false`) - `sound: true` boots the emulator with audio output (drops the mute launch flag). - Applied consistently to the snapshot-loadability probe, hot boot, and cold boot — the qemu-device-topology parity invariant is preserved. A `default_boot` snapshot saved in the other audio mode fails the probe and falls through to a cold boot that re-saves it, so toggling costs one slower boot and nothing else. - iOS/Vega/Electron are unaffected (argent never mutes them); the argument documents itself as Android-only. ## 2. Agent-facing information: argument description only Per the thread, the info lives in the tool argument's description (no skill changes): it states the muted default, *why* (multiple agent-driven devices sharing the host's speakers), the `force: true` hint for an already-running muted emulator, the cold-boot cost of toggling, and the `boot-sound` flag. The underlying emulator flag is described behaviorally ("muted"), not by name, per our agent-docs convention. **Validated that the description alone is sufficient** by driving three models through the Cursor `agent` CLI with the schema excerpt and the exact scenario from the field report ("testing notification sound, hear nothing, emulator already booted with defaults"). All three — composer-2.5, gemini-3.7-flash, gpt-5.2 — correctly diagnosed the muted default and produced the exact fix `boot-device({avdName, sound: true, force: true})`, including the weakest model. Transcript excerpts in the PR discussion below if wanted. ## 3. `boot-sound` feature flag `argent enable boot-sound` defaults the `sound` **argument** to `true`. It only moves the argument's default, read live per call: an explicit `sound: false` on a call still boots muted, and without the flag the default remains muted. Registered in `FLAG_REGISTRY` so `argent enable`/`argent flags` pick it up with no further wiring. ## Tests - New `boot-device-sound.test.ts`: mute-by-default, `sound: true` dropping the mute flag from **both** probe and hot-boot spawn (parity), cold-boot spawn, flag-driven default, and explicit-`sound:false`-beats-flag. - The three existing Android boot test files now pin `isFlagEnabled` to `false` so a developer's real `flags.json` can't flip the args they assert on. - `tool-server` boot suites 62/62 green, `argent-cli` flags 33/33, `configuration-core` 122/122; lint, `typecheck:tests`, and knip all pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
filip131311
|
b232114422 |
docs: audit every comment in src against the code it describes (#931)
Audits every comment in `src/` and `scripts/` against the code it
describes.
**462 files, 57 commits, net −8,194 lines.** Comment text only — the
whole branch is code-identical to `main`.
## Method
One subagent per file, strictly sequential. Scope was `src/` +
`scripts/` (459 files); three more files were added at the end because
they carried dead references of the same kind — two test headers citing
design docs that do not exist, and `publish-npm.yml` citing a retired
workflow. Each agent verified every comment — line, block, JSDoc, file
header, trailing — against the surrounding code and the rest of the
repo, following identifiers, paths, tool ids, config keys, env vars and
issue links to see whether they still exist and still behave as
described. Rules:
- **A false or misleading comment is deleted, not reworded.** If a claim
could not be confirmed by reading the source, it went. That is why the
deletion count is so much larger than the rewrite count.
- Survivors are cut to the shortest form carrying something the code
does not already say. Restatement, preamble, hedging, changelog prose
and ASCII banners are gone; the non-obvious *why* stays.
- Preserved byte-identical: license headers, pragmas and directives
(`@ts-*`, `eslint-disable`, shebangs, `/// <reference>`), JSDoc tag
tokens, everything inside a string or template literal, and the sole
comment inside an otherwise empty block (ESLint `no-empty` counts a
comment-bearing block as non-empty).
## Verification
Every file passed two independent gates before being recorded as done:
1. `comments-only` — the required check.
2. A second comment-stripping comparator with a proper mode stack,
written for this pass because `comments-only`'s flat scanner desyncs on
nested template literals and quote-bearing regex literals and then
reports comment lines as code changes. Two files hit that false FAIL
(`utils/android-profiler/pipeline/index.ts`,
`scripts/extract-tools.mjs`); in both the "changed code" it printed was
literally `//` lines, and the second checker confirmed the code was
byte-identical.
After the last file, all 462 changed files were re-checked against
`main` with the same comparator, rather than trusting any agent's
self-report. **459 code-identical; 2 are non-code (`.svg`, `.md`); 1
intentional.**
The intentional one is `packages/argent/scripts/bundle-tools.cjs`: the
changed template literal *is* the comment header of the file it
generates, `packages/native-devtools-android/src/bundled-meta.ts`.
Fixing only the generated file would have been reverted by the next
build, so the generator changed too — and it has been verified to
reproduce the committed generated file byte-for-byte.
## Representative false claims removed
Not wording nits — statements a reader would have acted on:
- **Reversed directions.** `proxyStart`'s JSDoc had the tunnel backwards
(it is a reverse tunnel: the host binds first and the simulator dials
in). A `paste()` doc had the pasteboard copy direction reversed.
- **Contradicted by the code below it.** A timeout budget multiplied by
three where the probes run concurrently — the same comment said so six
lines later. A "warn once" that warns on every call. A "binary search"
that is a linear scan.
- **Named things that do not exist.** A `vega-fast-cli` binary, a
`finish-recording.ts`, a `publish-next.yml` workflow, two
`profiler-react19-*.md` design docs, a `DebuggerTarget.ts`, a commit
hash git does not know, two tool ids, an `ensureEnv` cycle.
- **Wrong by construction.** "Welford accumulators" across four files
where the code keeps naive `n`/`sum`/`sumSq`; `sum`/`sumSq` documented
over `actualDuration` when reduce sums `selfDuration`; a strict-mode
halving written `n/2` where the code ceils; field docs listing enum
values the producers never emit.
- **Guarantees the code does not make.** A validation matrix claiming to
cover "EVERY tool" that skips flagless ones; a Pareto cutoff that
`slice(0, 20)` makes inert; an idempotence claim where the real rule is
at-or-ahead; a capability note describing a clean 400 the shape-based
device resolver can never produce.
- **Unverifiable assertions** about prebuilt binaries, external CLIs and
the cloud SDK — deleted rather than kept as folklore, since nothing in
the repo can confirm them.
- **Stale numbers**: invented Android tool versions, hard-coded tool
counts and description lengths that had drifted.
## Review
A Fable agent reviewed both halves adversarially for over-deletion,
misread code, `no-empty` hazards and byte-identity violations.
Second-half verdict: **SHIP**, with two one-line restores, both applied
in the final commit — the `npm view ""` rationale behind a blank-token
guard, and the note that `argent-mcp` keeps a copy of
`SECRET_PLACEHOLDER_MARKER` it cannot import.
## Code issues surfaced but deliberately not fixed
This pass changes comments only. Eight genuine findings are logged for a
follow-up:
1. `telemetry/src/consent.ts` — a non-ENOENT read error returns null and
falls through to the default-on path, so file errors *can* silently flip
telemetry on.
2. `chromium-server/navigation.ts` — `navigate()` is reachable from
`POST /api/navigate` with only a `typeof === "string"` check; open-url's
schema is a bare `z.string()`, so the "already validated by zod" premise
never held.
3. `http.ts` — `constantTimeEqual` returns early on a length mismatch,
so the auth token's length is observable.
4. `describe/index.ts:~114` — the ios-remote branch passes `{ isTvOs:
false }` unconditionally, so a remote tvOS simulator takes the iOS
ax-service path, though `isRemoteTvOsSimulator` exists and shake/paste
do use it.
5. `devices/boot-device.ts` — `-crash-report-mode never` is passed
unconditionally *and* appended again by the feature-detecting path, so
every emulator spawn passes it twice.
6. `react-profiler/pipeline/04-rank.ts` — `PARETO_THRESHOLD_PCT` is
dead: `slice(0, 20)` always wins.
7. `reaped-sessions.ts` — a user-facing hint string tells the agent that
`react-profiler-start { force: true }` disposes the debugger and
profiler session; it does not. Left byte-identical because it is a
string literal, not a comment.
8. `utils/simctl-backend.ts` — `localSimctl` is exported with no
importers anywhere.
## Docs
No documentation change is needed: this pass touches only source
comments, and no user-facing capability, tool, CLI flag, config key or
flow-file behaviour changed.
---------
Co-authored-by: filip131311 <f.kaminski2000@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
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> |
||
|
filip131311
|
172e4d3a64 |
fix(android): capture the screen the way it is actually oriented (#693)
Part of #609. ## What was wrong On a rotated Android device `screenshot` returned a portrait-framed image with the content lying sideways. `describe` frames and gesture coordinates were **already upright** — uiautomator measures against the rotated display — so the capture was the one surface out of step. An agent reading that image saw geometry that disagreed with every coordinate it was given. Measured on a landscape Pixel_9 (API 36, `mCurrentOrientation=1`): | | space | |---|---| | `describe` | upright — `"Search Settings"` centre `(0.530, 0.234)` | | taps | upright — tapping describe's `"Display & touch"` centre opens it | | `screenshot` (default) | **portrait-native, sideways** | ## The fix simulator-server already applies `rotation` correctly; nothing was telling it which one. The rotation is now queried from the device per capture, so `screenshot`, `screenshot-diff`'s live captures and the auto-attached "Screen after action" screenshot all land in `describe`/tap space. **Nothing is tracked.** A cached orientation could only be staler than the ~40 ms probe, and serving a stale one is exactly the failure being fixed. When the rotation cannot be read the request is left byte-identical to before rather than guessing — a wrong orientation is a confidently wrong image, which is worse than a sideways one. **iOS is deliberately untouched.** There the whole surface is consistently portrait-native, so rotating only the capture would *break* an agreement rather than restore one. The iOS half of #609 needs a different change and is not in this PR. ### Also fixed: the legacy uiautomator describe path Same root cause, worse symptom. `wm size` reports the **unrotated** size — on the landscape Pixel_9 it still answered `Physical size: 1080x2424`. Dividing rotated bounds by that does not merely squash frames: `isVisibleRect` prunes nodes whose left edge is past the screen width, so **the right-hand half of a landscape screen vanished from the tree**. The dump carries `<hierarchy rotation="N">`, so the divisor is oriented from bytes already in hand — no extra round-trip on the path that runs precisely when the helper could not be reached. ## On the rotation-name table Pinned by measurement, never by what the names mean — the reporter of #609 found the same names are 180° inverted on iOS. `adb exec-out screencap` is rotation-aware, so it is ground truth; each candidate was scored as mean absolute difference against it both as-is and rotated 180°, so an inverted mapping could not pass: ``` rotation 1 LandscapeLeft 2.19 (vs 6.54 for its 180° twin) rotation 3 LandscapeRight 2.19 (vs 6.34) rotation 2 PortraitUpsideDown 2.67 (vs 13.13) rotation 0 Portrait identity ``` This table is the **inverse** of simulator-server's own `90° → LandscapeRight` convention, because it compensates for a rotation that layer applies while decoding the scrcpy stream. That makes it a property of a particular simulator-server build rather than of Android — which is why a table test alone is not enough, and the delivered PNG's aspect is checked against the rotation requested, falling back to an unrotated capture if they disagree. ## Verified live Branch tool-server against a real emulator: - landscape, default capture → **2424×1080 upright**, MAD 2.19 vs ground truth (6.54 vs its 180° twin) - cropping `describe`'s frame for the toolbar out of that default capture shows exactly the "Display & touch" toolbar — the two spaces agree - rotation 0 → 1080×2424, MAD 2.82, unchanged - rotated **iPad** → still 2064×2752 sideways, i.e. iOS provably untouched Plus 27 new unit tests, including one that asserts the legacy path *drops* the right-hand node before the fix and keeps it after. Full suite 3125 passing; lint, prettier, `typecheck:tests` and the tool-description gate all clean. ## Not in scope - The iOS half of #609. - `rotate` on Android silently no-ops when auto-rotate is enabled (it returned `{orientation:"LandscapeLeft"}` while `dumpsys` still read `cur=1080x2424`). Filing separately. - `screen-recording` still records sideways — same root cause, different pipeline. Worth noting a session's screenshots are now upright while its video is not. - Existing visual baselines captured on a rotated Android device will stop matching, since the baseline key embeds capture dimensions and those now swap. This fails loudly as a missing baseline rather than silently comparing the wrong thing — and those baselines were sideways captures of an upright UI. --- > **#673 and #658 are both stacked on this branch** — each conflicts with this one (`screenshot/index.ts` and `screenshot-diff/index.ts` respectively) but not with each other, so both were rebased onto this branch as siblings rather than chained. Merge this first; GitHub then retargets both to `main`. --------- Co-authored-by: Filip131311 <f.kaminski2000@gmail.com> |
||
|
filip131311
|
d0b3356ebf |
feat(tool-server): paste tool (device clipboard + paste shortcut) (#879)
## Summary Registers a real `paste` tool. The `tools/paste/index.ts` from #506 was never registered and sent a `{cmd:"paste"}` WebSocket command simulator-server doesn't implement — dead on every platform. - **iOS simulator**: simulator-server `POST /api/clipboard/text` fills a private pasteboard and pushes it into the sim synchronously (host clipboard untouched); the tool then sends the ⌘V chord over the existing `key` command (the same `simulatorSendKeyEvent` path the keyboard tool uses). - **Android emulator**: same route over the emulator gRPC bridge, then `KEYCODE_PASTE` via `adb shell input`. - **ios-remote**: the MoQ transport's existing paste primitive, awaited. - Clean 400 for tvOS / Android TV (runtime probe), physical devices, Chromium (a renderer paste would clobber the *host* clipboard), Vega. - `{{secret:<NAME>}}` placeholders as in `keyboard`; allowed inside `run-sequence`; device-interact SKILL updated. - Description/skill tell the agent `paste` is **not** a `keyboard` substitute — only where a real user would paste (OTP/2FA code, long link/token, testing paste handling). ## Blocked on a simulator-server built with the `clipboard` feature: software-mansion/radon#185 adds it to the `argent` alias (no sim-server API change). Until a release + bin bump, the tool reports `PASTE_CLIPBOARD_UNSUPPORTED` (with an update hint) against the shipped binary. ## Test plan - 13 new unit tests (`test/paste.test.ts`) + secret-placeholder tests; full tool-server suite, lint, knip green (the 4 telemetry/bind test files that fail locally fail identically on `main`). - Live against a locally built simulator-server: iOS 26.5 Settings search (12 back-to-back pastes in order), Messages composer (multi-line/tab/emoji/diacritics preserved), `{{secret:EMAIL}}` never echoed or logged; Pixel_9 Settings search (ASCII + `Zażółć🔑`), Expensify RN login field; `run-sequence` paste steps; live 400s for tvOS sim / phone serial / chromium id. |
||
|
Ignacy Łątka
|
08f91fffd1 |
perf(screenshot): lower the default capture scale to 0.25 (#878)
Screenshots are billed to an agent's context by **pixel area**, not by file size — so PNG level, JPEG quality and codec choice buy nothing here, and the default scale is the only real lever. `0.3` had never been measured against what a model can actually read, so I measured it. ## What was run Bluesky (`bsky.app`) built from source and driven on an iOS 18.6 simulator, signed in against the repo's own seeded dev PDS (`yarn e2e:mock-server` + `e2eSignInAlice`) — a live feed, profile, notifications, Explore and Settings, not marketing screens. Every image came out of argent's own `screenshot` tool over the tool-server HTTP API, so the downscaler under test is the shipped one. **160 API calls**: 5 screens × 5 scales × 2 models × 3 reps, plus a blind control per screen per model. Questions were the things an agent actually needs off a screen — an `@handle` two rows down, a relative timestamp, a follower count, a badge number, which tab carries the underline, the label of the row above another row. Two controls decide whether the numbers mean anything: - **Blind pass.** Each model answered every question with *no image at all*. The 6 of 30 questions it still got right from priors alone (the screen title, the tab selected by default) were struck from scoring. 24 survived. - **Counting split out.** "How many rows between X and Y" was unreliable at *every* scale including full resolution — it measures arithmetic, not legibility — so it is excluded from the headline and reported separately. ## Result Text-reading accuracy, 66 graded answers per cell. Token counts are the API's own reported `input_tokens`, image column net of the ~345-token prompt. | scale | image tokens | Haiku 4.5 | Opus 5 | |---|---|---|---| | 1.0 | 2692 | 100.0% (66/66) | 100.0% (66/66) | | 0.3 *(old default)* | 451 | 98.5% (65/66) | 100.0% (66/66) | | **0.25** *(new default)* | **298** | **100.0% (66/66)** | **100.0% (66/66)** | | 0.2 | 196 | 98.5% (65/66) | 100.0% (66/66) | | 0.15 | 114 | 81.8% (54/66) | 97.0% (64/66) | 0.25 is the only scale that scored perfectly for both models on every repetition, and it costs **34% less context** than 0.3. ## The cliff, and why the margin Every wrong answer at 0.15: | question | truth | answered | model | | |---|---|---|---|---| | 2nd author's handle | `@bob.test` | `@carla` | Haiku ×3 | read the row above | | selected feed tab | `Following` | `Feeds` | Haiku ×3 | couldn't resolve the underline | | 4th profile tab | `Videos` | `Likes` | Haiku ×3 | drifted two positions | | 3rd settings row | `Moderation and content filters` | `Privacy and security` | Haiku ×3 | off by one row | | bell badge number | `1` | `UNREADABLE` | Opus ×2 | declined rather than guess | **Opus fails safe; Haiku fails silent.** Opus refused the unreadable badge. Haiku returned a confidently wrong *neighbouring* row — the same wrong answer three times out of three. For an agent that turns a read into a tap, a confident off-by-one row is worse than a refusal, because nothing downstream catches it. ## Why 0.25 and not 0.2 0.2 measured no worse than the old default and would have saved 57% instead of 34%. I didn't take it: the evidence is five screens of one app at one device size, and Bluesky sets fairly generous type. A denser interface — a table, a settings screen with subtitles, a smaller phone — reaches the floor sooner. 0.25 keeps two steps of margin above the cliff instead of one. `ARGENT_SCREENSHOT_SCALE=0.2` stays available for anyone who wants to spend that margin deliberately. ## Scope - **Chromium is untouched.** Its default is 1.0 and its `scale` is silently dropped without the optional `sharp` dep — that's #673's territory and I'm not duplicating it. Worth noting the two interact: once Chromium can downscale (via the in-repo `resizeDecodedPng`, as #673 suggests as a follow-up), it should pick up this default too, and it is currently by far the most expensive platform per capture. - **`ARGENT_SCREENSHOT_SCALE` is documented as `0.3` in `argent-private/docs/environment-variables.md`** — a submodule, so it needs its own PR to stay in sync. - Stale prose fixed in passing: `getScreenshotScale` carried a `// default: halve the resolution` comment next to a `0.3`, and `vega-screen.ts` documented the fallback as `0.3`. ## Checks `npm run build` clean, `tsc --noEmit -p packages/tool-server/tsconfig.test.json` clean, eslint `--max-warnings 0` clean, prettier clean. Shard 1/4 of the tool-server suite: 88 files / 1261 tests pass; the four files touching screenshot scale pass individually (22 tests). The default had **no test coverage at all** — the constant could be edited to any value and the suite stayed green. This adds one. Mutation-checked: reverting the constant to `0.3` fails 6 of the 8 new cases. ### On the red Unit tests check Inherited, not introduced. This branch's Unit tests job fails on exactly one test — `describe-tool.test.ts > states the limitation without ordering a reboot when the degraded read returned a tree` — which is the same single test failing on `main` itself (main has been red since #583, this branch's base). Already tracked as #871 and fixed by #877. `flow-composition.test.ts` passes in CI on both. Everything else is green: ESLint, Prettier, Knip, Static checks, SpiderShield, lockfile sync, Linux + Chromium E2E, macOS + Chromium E2E, macOS + iOS E2E. |
||
|
Ignacy Łątka
|
6d31aed752 |
fix(describe): stop ordering a simulator reboot on every externally-booted read (#583)
Fixes #521. ## The report holds, and is worse than described Reproduced against an unmodified `origin/main` tool-server driving a simulator booted the way a developer boots one (plain `xcrun simctl boot`, no argent involvement), iOS 18.6: ``` ===== describe call #1 ===== ===== #2 ===== ===== #3 ===== hint: "This simulator was not booted through argent — system dialogs and native modals may not appear. You MUST call boot-device with force=true now to restart the simulator and apply full accessibility settings before continuing." ``` Identical on every call, as reported. The part that makes it more than a wording nit is what came back *alongside* it — a complete, working tree: ``` ROOT AXGroup (0.000, 0.000, 1.000, 1.000) AXStaticText "19:51" (0.139, 0.023, 0.102, 0.023) AXGroup "Maps" value="Widget" (0.082, 0.096, 0.395, 0.206) ... ``` And the caveat's own claim does not hold either. Triggering a real system permission alert on that same externally-booted sim (`simctl privacy reset location com.apple.Maps` + launch): ``` AXGroup "Allow “Maps” to use your location?" (0.193, 0.367, 0.614, 0.296) AXButton "Allow Once" (0.193, 0.524, 0.614, 0.046) AXButton "Allow While Using App" (0.193, 0.570, 0.614, 0.046) AXButton "Don’t Allow" (0.193, 0.617, 0.614, 0.046) ``` The system dialog reads fine, buttons and all. So an agent obeying "You MUST ... now" restarts the simulator — killing the Metro session, dev client and staged app state — to fix nothing. ## Root cause `AXServiceApi.degraded` is `!entitlementBypassActive`, read from `IgnoreAXServerEntitlements`. That pref is only writable pre-boot, so `boot-device` is the only thing that ever sets it and *any* externally-booted sim reads as degraded for its whole life. The pref bypasses the AX-server entitlement check introduced in iOS 26.5 — on earlier runtimes it is irrelevant to whether the read works. The flag describes how the sim was booted, never whether describe succeeded, and the hint conflated the two. ## Change Resolve the caveat against what the accessibility read produced: - **Blind read** — the reboot has something to gain, so the actionable guidance stays, on every call, and now names what the reboot costs. - **Populated read** — the read is its own proof no reboot is needed. States the limitation instead of ordering anything, once per device per server lifetime. The per-device gate is applied at the `describe` tool boundary only. `await-ui-element`, the flow runner and `await-screen-idle` poll `describeIos` directly, treat a present `hint` on an empty tree as "this emptiness is untrustworthy" (`isBlindRead`), and fold it into one terminal timeout note — they keep every copy, so a `hidden` wait still cannot resolve off a blind read. The native-devtools fallback can fill a tree the accessibility read left empty, and that split has to key on the accessibility read rather than on the tree that comes back. Its elements are the injected app's own view hierarchy: no SpringBoard chrome, no system dialog, and no evidence the accessibility subsystem works. Found doing exactly that on a fresh iOS 18.6 sim. Describing within seconds of `simctl boot` leaves the accessibility read blind for the rest of that boot (3/3; it survives a tool-server restart, and only `boot-device force=true` recovers it). With the Maps location prompt on screen in that state, the fallback returned the app's own 20 elements with the dialog absent — and resolving against *that* tree picked the standing note, which the per-device gate then deleted, so from the second call the agent had a tree silently missing a modal blocking the screen and no caveat at all. The note it briefly carried says "everything else reads normally", which nothing there did. Keying on the accessibility read makes both wordings true of the state that produces them. I skipped the suppression env var / flag the issue floated as a "maybe" — once the caveat is a single non-directive line per device, there is little left to suppress, and it seemed better not to add config surface for it. Happy to add it if you'd rather have the switch. ## Verification E2E against the branch build, same externally-booted sim: | | elements | hint | |---|---|---| | describe #1 | 14 | standing caveat, no MUST/now | | describe #2-4 | 14 | *(none)* | | after `simctl shutdown` (blind) ×2 | 0 | blind caveat, both times | | after `boot-device force=true` ×2 | 28 | *(none)* — `IgnoreAXServerEntitlements` = 1 | Unit: 6 new tests in `describe-tool.test.ts`. Each was mutation-checked — restoring the MUST wording, removing the dedup, collapsing the blind/standing split, keying the gate globally instead of per device, and re-resolving the caveat against the native-devtools tree each fail exactly one test and nothing else. Full gate: `vitest run` 4715 passed / 1 skipped, `tsc --build`, `typecheck:tests`, `eslint --max-warnings 0`, `prettier --check`, `test:scripts` 59 passed. |
||
|
filip131311
|
82d0209b9e |
fix(android-profiler): explain an empty CPU section instead of rendering it as an idle app (#669)
Fixes #629. ## Reproduced, then diagnosed harder than the report Pixel_9 / API 36, `com.android.settings`, real interaction — all three symptoms reproduced: ``` native-profiler-analyze → 70 hangs + RSS populated, NO CPU section, "showing top 0 CPU hotspots", exportErrors: {} thread_breakdown → 296 samples across 9 threads function_callers epoll_wait → not found ``` The report infers "unsymbolized frames". It's stronger than that — I queried the `.pftrace` directly: | | value | |---|---| | samples for the process | 296 | | samples with a `callsite_id` | **0** | | `stack_profile_frame` rows (trace-wide) | **0** | | `stats.perf_samples_skipped` | **296** | | `unwind_error` | NULL on every row | **The unwinder never ran.** There are no frames to symbolize. That's why `thread_breakdown` — which counts `perf_sample` rows and never joins the symbol tables — reports 296 while every stack-dependent view is empty. ## Positive control: same emulator, debuggable app ``` adb shell getprop ro.build.type → user dumpsys package com.android.settings | grep pkgFlags → [ SYSTEM HAS_CODE … ] ← no DEBUGGABLE dumpsys package com.anonymous.myapp | grep pkgFlags → [ DEBUGGABLE HAS_CODE … ] ``` | | com.android.settings | com.anonymous.myapp (debuggable) | |---|---|---| | samples / with callsite / named leaf | 296 / **0** / **0** | 86 / **86** / **78** | | frames in trace | **0** | **723** | | `perf_samples_skipped` | **296** | **0** | | analyze | no CPU section | `writel` 48.72%, `internal_get_user_pages_fast` 6.41% | So this is **not** an emulator limitation and **not** symbolization — it's target profileability. The remedy the message gives is verified to work on the very device it describes. ## The fix The pipeline detects "samples exist, none carry a usable stack" and says so. It names the count and the package, and distinguishes two causes using **`leaf_mapping`, already on the row** — no new query needed. It comes from the same join chain as `leaf_function`, so a mapping with no name means the stack *was* unwound and only the symbol is missing (stripped `.so`s), while neither means nothing was unwound (not profileable). Different problems, different fixes. Live output on the bad trace: > ⚠️ **CPU sampling:** 296 CPU samples were captured for `com.android.settings`, but none carry a usable call stack, so no CPU hotspots could be computed and `profiler-stack-query` mode=`function_callers` / mode=`hang_stacks` will find nothing. The app was running — this is a capture limitation, not an idle app. mode=`thread_breakdown` still works, since it counts samples and needs no stacks. No stack was unwound at all: Perfetto only unwinds a process it can read `/proc/<pid>/mem` for. Profile a debug build, or add `<profileable android:shell="true"/>` to `<application>` and rebuild — verify with `adb shell dumpsys package com.android.settings`, which shows `DEBUGGABLE` for a profileable target. **Not routed through `exportErrors`.** That looked free — it renders above the Summary with no plumbing — but `renderAllClear` counts its entries as failed queries and would print *"1 of 3 queries errored"* about three queries that all succeeded. It goes through a `cpuDiagnostic` note instead, alongside the existing `freshnessNote`, and `status` stays `ok`. **Also fixed:** the footer no longer says "showing top 0 CPU hotspots" — the literal artifact in the report title. ## What I deliberately did NOT touch The existing no-samples-at-all manifest hint stays gated on `hangRows.length === 0`. That gate is intentional and pinned by `manifest-hint.test.ts:52` ("does NOT emit a manifest hint when hangs were found but CPU was empty") — once hangs come back the app clearly *was* traced, so blaming the manifest is wrong advice. It also wouldn't have helped: in the reported case `cpuRows.length` is **9, not 0**, so state A is never reached. Ungating it would have fired the hint on ~6 more existing fixtures and flipped their `status`, for no benefit. I only corrected its text, which claimed "Perfetto silently drops the linux.perf data source" — this issue proves a non-profileable app still yields samples. ## Verified live, both directions | trace | CPU warning | CPU section | footer | status | |---|---|---|---|---| | stackless (`com.android.settings`) | ✅ shown | absent | `showing top 3 hangs` | `ok`, `exportErrors: {}` | | healthy (debuggable app) | ✅ absent | present | `showing top 2 CPU hotspots and top 3 hangs` | `ok` | ## Deliberately deferred (filed separately) - `thread_breakdown` / `function_callers` notes — the issue's "and ideally" half. Needs a new `thread-breakdown.sql` aggregate plus fixture updates in two test files. - A partially-stackless trace renders percentages that **sum to 100%** while describing only the surviving fraction — arguably more misleading than the empty case here, and a distinct defect. - Repo docs assert the behaviour this PR only now implements. ## Checks - 3093 tests pass; 7 new. **All 200 existing android-perfetto tests pass unmodified**, including the manifest-hint gate. - `tool-description-quality.yml` doesn't run (path-filtered to `src/tools/**`); prettier, eslint, both typechecks clean; lock untouched. |
||
|
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
|
||
|
Kacper Kapuściak
|
f8d58489d0 |
feat(shake): wobble the host window behind a microinteractions flag (#803)
`shake` only touches the guest, so nothing on screen explains why an app popped its dev menu. This wobbles the host window carrying that device while the gesture runs. - Opt-in `microinteractions` flag, off by default (`argent enable microinteractions`). Read inside the handlers, not as `ToolDefinition.featureFlag`, which would hide the whole tool. - Can never fail a shake: every path warns once to stderr and resolves. Skipped off macOS, on headless boots, and for `sim-remote`. - Windows are title-matched on the iOS display name, so a host with several booted simulators wobbles the right one. - `prepareHostWindowShake` resolves flag/platform/device-set/script once per call; `begin()` per gesture, `settle()` once after the loop. Not in the `finally`, so a real failure throws immediately rather than waiting on cosmetics. - `ARGENT_SIMULATOR_NO_WINDOW` / `ARGENT_EMULATOR_NO_WINDOW` parsing extracted to `utils/no-window-env`, shared with `boot-device`. ### iOS simulators only The wobble originally covered Android emulators too and has been removed there. Picking the emulator's window meant matching a title among several `qemu-system-*` host processes, using needles built from the console port plus an extra `adb emu avd name` round trip per shake. That lookup was the fragile half of the feature: a build that drops the port from the title, or a serial with no console port, falls back to either the wrong window or no animation at all. An Android shake also already produces visible in-guest feedback, so the amplification bought less there. `shake` itself is unchanged on Android: the same accelerometer burst, the same resting-vector capture and restore, the same per-device lock. Only the cosmetic host-window animation is gone. `ARGENT_EMULATOR_NO_WINDOW` handling stays in `utils/no-window-env`, where `boot-device` still uses it. Verified live on an iPhone 17 Pro Max: window returns to its exact origin and the wobble fits inside one gesture. The Android sensor path was verified live on a Pixel_9 before the removal and is unchanged by it; it has not been re-run since. Full tool-server suite green. The AppleScript motion itself is only verified by eye; unit tests stub `osascript`. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Hubert Gancarczyk
|
20b5e8b4f0 |
fix(tool-server): reject a keyboard call carrying both text and key (#579)
## Problem
`keyboard` accepted `text` and `key` in one call, and that shape has no
meaning a caller can rely on. The same request reads two ways:
- `{ text: "hello", key: "enter" }` — "type, then submit"
- `{ text: "abc", key: "backspace" }` — "delete, then type" reads just
as naturally
Whichever order a backend picks, the other reading silently corrupts the
field. The backends did not even agree: `typeAndroidPhone` pressed
`params.key` **before** `params.text`, while simulator-server, chromium
and vega all did the reverse and the tool's own description promised
"the text is typed first and the key is pressed after it".
## Change
**`keyboard` takes `text` or `key`, never both.** A combined call is
rejected in `execute`, above the platform dispatch, so it resolves no
`ARGENT_SECRET_*` value and reaches no device — no backend has to defend
against the shape.
**To mix text and a key, use `run-sequence`.** That is what the error
message, both parameter descriptions, the tool description and the skill
docs now say:
```json
{ "udid": "<UDID>", "steps": [
{ "tool": "keyboard", "args": { "text": "hello world" } },
{ "tool": "keyboard", "args": { "key": "enter" } }
]}
```
Two separate calls work too, but cost an extra round-trip — and are
**not** equivalent when the text carries a secret (see below).
### Why the rule is prose, not JSON Schema
An earlier revision hand-wrote `inputSchema` with `not: { required:
["text", "key"] }`. That is no longer legal: #782 banned every top-level
combinator repo-wide, `not` included, because the Messages API rejects a
request whose tool schemas declare one — and that 400 fails **every tool
in the request**, not just this one.
`tool-input-schema-contract.test.ts` enforces it across the catalog.
So the constraint is restated in **both** fields' `.describe()` (the
`boot-device` house rule) and in the tool description.
`keyboard-text-key-exclusive.test.ts` pins both texts, and pins that the
advertised schema stays combinator-free.
### What the error message carries
Three things a caller needs and cannot get elsewhere:
1. **The literal retry**, not an ellipsis the Android backend cannot
type.
2. **The TV caveat.** A TV target rejects `key` outright
(`platforms/tv.ts`), so the prescribed `{ key: "enter" }` step cannot
succeed there. The guard runs above the dispatch on purpose, so it would
otherwise swallow that diagnosis; the caveat is carried statically
rather than by an async TV probe.
3. **The secret steer**, only when `text` holds a `{{secret:…}}`
placeholder. The auto-screenshot skip is decided per tool call from the
whole request, so:
| call shape | auto-screenshot |
| --- | --- |
| one `run-sequence` carrying both steps | skipped |
| `{ text: "{{secret:…}}" }` alone | skipped |
| a separate `{ key: "enter" }` | **not** skipped — captured after the
key lands, and can hand the still-visible secret back as pixels |
The check is the syntactic `.includes` `flow-utils.ts` already uses, so
the guard still resolves nothing. A plain combined call does not carry
the sentence, with a positive control pinning that.
### Simplifications the rule makes possible
With at most one of the two ever reaching a backend, the
resolve-then-inject splits that existed only to order a combined call
fold back into their injectors (`android-input.ts`, `vega-input.ts`),
and each backend collapses to two exclusive branches.
### Fixed alongside
`run-sequence`'s description claimed "No screenshot is captured
automatically". `shouldAutoScreenshot("run-sequence")` is `true` with a
15000 ms settle cap, so that line was already false — and this change
makes it load-bearing, since the secret guidance rests on it.
## Compatibility
A stored flow or scripted `run-sequence` whose step is `{ tool:
keyboard, args: { text: "…", key: "enter" } }` now errors on replay, and
the steps after it are skipped. The recorder never emitted the combined
shape (`flow-add-step` executes the call before it appends the step), so
the blast radius is hand-authored flows and scripted `run-sequence`
calls — which is exactly what the old worked example taught. The fix is
to split the step in two.
## Test coverage
- **`keyboard-text-key-exclusive.test.ts`** (new, replaces
`keyboard-key-order.test.ts`) — the rejection on all four backends with
nothing injected and no service resolved, plus a positive control per
backend that text alone and a key alone still work; shape-not-truthiness
(`{ text: "", key: "enter" }` rejects); at-most-one rather than
exactly-one (`{}` stays a no-op); rejection before secret resolution;
the run-sequence retry, TV and secret sentences, with a positive control
against an unconditional secret steer; and the two channels the
constraint actually reaches a client through.
- **`keyboard-backend-fidelity.test.ts`** — absorbs what key-order
uniquely pinned. The unknown-key 400 now names the offending key on iOS
and chromium here, on android in `keyboard-android.test.ts`, and on vega
in `vega-injection.test.ts` (tightened from the bare `/Unknown Vega
key/` prefix), so stripping the name is red on every backend.
- **`keyboard-android.test.ts`** — the combined-call tests are gone.
What they uniquely covered survives as single-parameter tests: the
multi-segment `%` codepoint count, the pressed key's identity against a
second literal keycode, and un-typeable text rejected with no on-device
side effect. The exhaustive 24-keycode loop now runs through
`injectAndroidNamedKey` and asserts the emitted `input keyevent <code>`,
so it pins the wire and not just the lookup.
- **`interaction-messages.test.ts`** — the two formatters see different
shapes now. `startedMsg` renders before the rejection, so it still words
four (including the combined one); `completedMsg` runs only after
success, so it words three. Both empty-request branches stay pinned.
- **`keyboard-tv.test.ts`** — the TV `key` rejection is independent of
this guard, which only ever sees requests carrying both. Its header says
so, since the exclusivity message's TV clause depends on it.
- **`scripts/e2e-full/phases/30-android.sh`** — the combined call
becomes an `assert_reject`, and the prescribed `run-sequence` remedy is
driven end to end.
## Live verification
Isolated tool-server built from this branch (own port, shared MCP server
untouched), against the Android settings search field on an API 36 AVD,
read back out of the view hierarchy rather than off a screenshot:
| Call | HTTP | Field / result |
| --- | --- | --- |
| seed `{ text: "XYZ" }` | 200 | `XYZ` |
| `{ text: "abc", key: "backspace" }` | **400** | field still `XYZ` —
byte-identical, nothing typed |
| `run-sequence` [`{text:"abc"}`, `{key:"backspace"}`] | 200,
`completed: 2` | `XYZab` — exactly what the combined call used to
produce |
| `{ text: "hello" }` | 200 | `{ typed: "hello", keys: 5 }` |
| `{ key: "enter" }` | 200 | `{ typed: "enter", keys: 1 }` |
| `{}` | 200 | `{ typed: "", keys: 0 }` — still a no-op |
| `{ text: "", key: "enter" }` | **400** | shape, not truthiness |
| `{ text: "{{secret:APP_PASSWORD}}", key: "enter" }` | **400** |
message carries the ONE-`run-sequence` steer and the
still-visible-secret reason |
| `{ text: "hi", key: "enter" }` | **400** | message does **not** carry
the secret sentence |
`GET /tools` reports `["properties", "required", "type"]` for `keyboard`
— no top-level combinator.
Suite: `packages/tool-server` 336 files / 4041 passed,
`packages/registry` 90 passed, `packages/argent-mcp` 79 passed. `tsc
--build`, `tsc --noEmit -p tsconfig.test.json`, eslint (0 warnings),
prettier and knip all clean.
---------
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
|
||
|
Ignacy Łątka
|
123cc6abe1 |
fix(screenshot-diff): honor an outputDir that does not exist yet on the host (#558)
## Problem
Passing `outputDir` to `screenshot-diff` silently did nothing when the
directory didn't exist yet. It was never created, the diff images went
to a temp dir instead, and nothing in the result said the requested
location had been discarded.
Found during an end-to-end pass over the 0.17.0 release: the diff itself
was correct, but the requested `outputDir` was never created and no diff
PNG existed anywhere under it.
## Cause
`outputDir` is declared as a `kind: "probe"` file input, and
`probeHostPath` for that kind is just `stat(path)`. It answers "does
this path already exist on the tool-server host", nothing more.
`resolveOutputDir` read `presentOnHost: false` as "this is a remote
client's directory":
```ts
if (params.outputDir && (probe === undefined || probe.presentOnHost)) {
return params.outputDir;
}
// ...otherwise a temp dir
```
But a local agent naming a fresh output directory produces exactly the
same `presentOnHost: false`, so it took the remote fallback too.
## Fix
When the probe says the path is absent, try to create it
non-recursively. That succeeds only when the parent already exists on
this host - true for a local agent picking a new subfolder, false for a
remote client's unrelated directory tree - so the remote fallback is
preserved. `EEXIST` (raced into existence between probe and create)
counts as ours.
It also makes the tool self-consistent: `captureLiveInput` already did
`fs.mkdir(params.outputDir, { recursive: true })` when writing
live-capture intermediates into that same directory.
## Tests
- `creates and honors an outputDir that does not exist yet on this host`
- asserts the diff artifact's `hostPath` is inside the requested
directory and that the file is really there. Fails on main, where the
artifact lands in a temp dir.
- `falls back to a temp dir when outputDir is not creatable on this
host` - parent missing, i.e. the remote case. Passes before and after,
pinning the behavior that has to survive.
## Verification
- `vitest run` in `packages/tool-server`: 288 files, 2916 passed, 1
skipped.
- `tsc --noEmit -p tsconfig.test.json`, `tsc --build` across the
workspace, and `prettier --check` on both changed files: clean.
The comment on the `fileInputs` block still described the old fallback,
so it's updated in the same commit.
|
||
|
filip131311
|
9e45b98135 |
fix(profiler): measure CPU windows from the samples, not from the question (#685)
Fixes #619. ## Reproduced, then verified, on the reporter's own session Their artifacts were still on disk (`react-profiler-20260731-150333`: 1873 samples, 25114.0 ms, one hot commit at t=12307.5 ms), so every number below is from the session in the issue rather than a fixture. ## Two defects **Self-time was the question, not the answer.** `avgIntervalMs = (endMs - startMs) / totalSamples`, so self-time = *requested window width* × share of hits. Same samples, three windows, three answers: ``` [0, 25114] → createAnimatedNode 29.12 [0, 50000] → 2× [0, 100000] → 4× ``` **Samples were displaced by the first hot commit.** `clockOffsetMs = firstCommitTimestampMs - cpuStartMs`, applied when `|diff| > 1000`. `startTime` is a since-boot monotonic value (1_276_275_277_894 µs ≈ 14.8 days), so that guard is *always* satisfied and the offset was *always* the first hot commit — 12307.5 ms here, which is exactly how far every sample moved. **Together they predict the reported 2×**: displaced samples covered `25114.0 − 12307.5 = 12806.5` ms of a full-range query, so `25114.0 / 12806.5 = 1.961`. The reporter measured `completeRoot` 52.87 vs 26.82 — a ratio of 1.971. ## The fix **Clock: subtract, don't infer.** React DevTools emits every commit as `performance.now() - profilingStartTime`, so commit timestamps are already ms-since-profiling-start. Sample times are now the same thing — `(accumulatedUs - startTime) / 1000` — and `buildCpuSampleIndex` no longer accepts a commit timestamp at all, so the inference cannot come back. **Weights: each sample's own interval, clipped to the window.** A sample stands for the interval that *ended* at it, so a window cutting through one counts only the part inside. That gives four properties the old formula had none of: independent of the requested bounds, additive across adjoining windows, bounded by the window (a 45 ms commit can no longer be credited with more than 45 ms), and summing to the profile's own duration. Per-sample rather than averaged because the sampler is not isochronous — measured on the real profile, deltas run 0–36.6 ms around a 13.1 ms median. An average smears a stall evenly across every function in the window. *(A note on my own reasoning: I initially justified per-sample weighting with a synthetic fixture showing a 20× cost difference reported as a tie. That demonstrates the mechanism but overstates it — real Hermes sampling is roughly periodic, so the effect is a redistribution, not a rank inversion. The durable arguments are window-invariance and additivity.)* **Finding nothing now says which kind of nothing.** Outside the recorded range; inside it but unreached by any sample; a profile with no samples at all; and — the case that actually dominates — a window **fully covered by samples that were all idle**. 99% of samples in this session were idle, so a window can be completely covered and still rank nothing. Reporting that as "no CPU hotspots found" is what made a miss read as "this commit was cheap". Every table now states what it measured: sample count, covered ms, idle ms, and the sampling interval — plus a warning when the window is narrower than ~3 intervals, or when a sampler stall lands wholly on one function. **The on-disk index is versioned.** A file written before this change holds displaced timestamps and no interval starts; reusing one would answer from cache with exactly the numbers being removed. Readers reject it and rebuild from the raw profile, which is always retained. The same validation stops a truncated index deserializing into an empty profile that would answer "no hotspots" forever (`new Float64Array(undefined)` is silently empty). ## The reporter's five queries, after ``` 12300–12345 → recordProfilingDurations 15.05ms, completeRoot 4.86ms (was: nothing) 10000–15000 → 371 samples, real hotspots (was: nothing) 0–1000 → 71 samples, 1000.0ms covered, ALL IDLE — stated (was: nothing) 24000–25114 → 85 samples, 1113.1ms covered, ALL IDLE — stated (was: touch events from t≈12s) 0–25114 → 1872 samples, 25113.1ms covered, self-times correct (was: 2×) ``` Two remain empty — correctly, and they now say why. ## Tests 15 new cases in `test/react-profiler/cpu-correlate.test.ts`, all pure. There was **no test at all** for either file before, which is why this survived. Covered: window invariance across four widths, sub-window partitioning, boundedness by window width, additivity, per-interval weighting, rebasing a since-boot start time, coverage within one interval of the profile's reported duration, defensive deltas (missing/negative/NaN), the four empty-window shapes, and index round-trip plus v1/truncated rejection. Mutation-verified: restoring the window-derived average fails 8 of 15. Full tool-server suite green — 3101 passed / 298 files. ## Behaviour change — worth calling out in the release notes Numbers move in both directions. Windowed self-times fall by the ratio of the window to its sampled coverage; previously-empty windows now return data; and per-commit CPU blocks in **every** analyze report change wholesale, because they were reading a region of the profile the commit never occupied. **Any before/after optimisation baseline recorded with the current build must be re-measured.** ## Not in scope - **`time-align.ts:50-58` has the same family of bug** — `buildReactAnchor` sets `monotonicStartMs` from the since-boot `cpuProfileStartTimeUs` while commit timestamps are profile-relative, so commits are projected ~device-uptime into the past and **no hang can ever correlate with a commit**. It is invisible to the existing test because that test passes `cpuProfilePath: null`. Filing separately: fixing it makes `buildReactAnchor`'s second parameter and a `readCpuProfile` call dead, so it is not the one-liner it looks like. - `queryCpuWindow` emits one row per *node id*, so a function reached via several call paths is listed repeatedly with its cost split. Real, and moves numbers again — separate issue. - `component_cpu` applies its top-50 cut per window *before* aggregating, truncating the long tail. - `renderCallTree` and `react-profiler-cpu-summary` still derive self-time their own ways; unifying them onto these weights is a follow-up. - Hermes ignores the requested `sample_interval_us` (100 µs requested, ~13.4 ms observed). Not argent's bug, but it is why no fix may derive an interval from the requested config. |
||
|
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. |
||
|
filip131311
|
b1abdec13a |
feat(tools): add a shake tool for iOS simulators and Android emulators (#786)
Shaking a device was the one hardware gesture argent couldn't perform, putting iOS's "Undo Typing" prompt, React Native's dev menu, and app-side shake-to-report handlers out of reach. iOS needs no simulator-server work: Simulator.app drives Device > Shake by posting a plain Darwin notification, `com.apple.UIKit.SimulatorShake`, which UIKit turns into the same motion event a physical device raises. So the whole path is one `simctl spawn <udid> notifyutil -p <name>` — and the same argv runs on a remote simulator through `sim-remote spawn`, whose exit code must be read explicitly because it exits 0 even when the child fails. Android has no OS-level shake event at all; every shake feature is an app-side accelerometer detector, so the sensor is driven over the emulator console with a burst of hard direction changes. That override outlives the call, so the resting vector is captured up front and always restored. Simulators and emulators only. Physical phones have real accelerometers with no host-side hook, and TV targets have no shake gesture — but Apple TV and Android TV are not separate platforms, so the capability matrix cannot exclude them and each handler probes the runtime and rejects, pointing at `tv-remote`. Live testing on a packaged build found three Android defects, each now fixed and covered by a test that fails without the fix: - The resting-vector regex could not read scientific notation, which is exactly how a rotated emulator reports its near-zero axes. The read failed, a portrait fallback was adopted, and the restore then rotated a landscape device back to portrait while still reporting success. - Overlapping shakes raced: the second captured a mid-burst sample as its resting vector and, finishing last, left the accelerometer pinned at ±30 m/s² — a self-perpetuating state every later shake re-restored. Capture, burst and restore are now one critical section per device, and an implausible resting magnitude is rejected so a foreign tilt heals instead of being adopted. - `adb emu` exits 0 whether or not the console accepted the command, reporting a refusal only as a `KO:` line, so a rejected sensor write read as a successful shake. The reply is now read, taking the last non-empty line as the verdict. Verified live: iOS raises the undo alert (local and remote), the Android burst reaches the framework per `dumpsys sensorservice` with the resting pose restored, landscape survives a shake, four concurrent shakes serialize cleanly, and a refusing console fails loudly. |
||
|
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) |
||
|
filip131311
|
8b3a8851a2 |
fix(profiler): honour top_n in profiler-commit-query by_index (#667)
Fixes #630. ## The bug `by_index` never received `top_n`: `renderByIndex` had no such parameter and the dispatch passed only `(commits, commit_index)`. Reproduced against the released 0.18.1 CLI on a real session — `top_n: 8` returned **306 rows / 20,245 characters**. It matters because `react-profiler-analyze` points at this exact mode for drilling into a slow commit, so it is both the most likely call and the most likely to blow an agent's context. ## Correction to the issue (and to my own first pass) The issue says "the other three modes honour `top_n` correctly". That's not right — **`cascade_tree` ignores it too** (`renderCascadeTree` takes no `topN`; its dispatch also passes two arguments). Only `by_component` and `by_time_range` slice. I've deliberately **not** fixed `cascade_tree` here, and not for the reason that first looked right. A DFS-prefix budget would in fact be structurally safe there. The real problems are that its roots come from a `Set` in insertion order with no subtree-cost aggregation, so any cut keeps an *arbitrary* subtree rather than the expensive one — making it useful needs a cost-ranking pass, which is design work, not a slice. And it dedups by `name:depth`, so it never had the 306-row blow-up that motivated this issue. Filed separately with the other findings. ## Why `top_n` is honoured only when explicitly passed Applying the schema's existing default of 20 looks obviously right and is a trap. These rows are **fibers, not components**, and they collapse hard. Measured on the real commit: | top_n | distinct components covered | |---|---| | 8 | 3 | | 20 | **10** | | 50 | 18 | | (all 306 fibers) | 70 | The analyze report that recommends this mode already shows up to **15** distinct components (`MAX_COMPONENT_ENTRIES`). So a default of 20 would make the escape hatch return *strictly less than the report it escapes from* — silently, since the footer would only say "298 hidden", not "you learned nothing new". `top_n` is therefore `.optional()`, and the two modes that genuinely defaulted to 20 keep it explicitly at their call sites (`params.top_n ?? 20`), so their behaviour is byte-identical. Since that collapse is invisible from a row count, the footer states it outright: ``` _Showing the 8 costliest of 306 fibers — 298 cheaper fibers hidden. Note these are fibers, not components: the full set covers 70 distinct components and this view covers 3. Omit top_n for everything._ ``` ## The trap this had to avoid The root-cause chain block scans the **full** commit, not the truncated table. The fiber carrying the root cause is typically *cheap* — it triggers the cascade rather than doing the work — so it falls outside any `top_n`. Slicing before that lookup would silently drop the single most useful line in the output. There's a regression test that pins exactly this: the only fiber with a `rootCauseParent` is the cheapest, `top_n=5`, and the chain must still be printed. ## Also fixed The analyze report suggested `` `profiler-commit-query mode=by_index` to see all `` **without passing `commit_index`** — an agent following it verbatim queries a different commit than the one being described. Now names the commit. Note I did *not* change it to name a `top_n` value: `totalComponentCount` counts distinct component names, while `by_index` emits one row per fiber, so passing it would still truncate — a subtler falsehood inside the line being corrected. With no default cap, "to see all" is simply true again. ## Verified live | call | before | after | |---|---|---| | `by_index top_n=8` | 306 rows, 20,245 chars | 8 rows, 898 chars, heading `Top Fibers`, `**Fibers:** 306 (showing 8)` | | `by_index` (no top_n) | 306 rows | 306 rows, heading `Full Detail`, no notice | | `by_component` | — | unchanged | ## Checks - 3093 tests pass; 7 new. Note the A/B against the pre-fix source is not meaningful for this file — `renderByIndex` wasn't exported, so those failures are import errors, not behavioural. The real before/after is the live table above, captured against the released 0.18.1 build. - extract-tools 46/46; SpiderShield average 9.10 (tool `description:` untouched); prettier, eslint, both typechecks clean; lock untouched. - Touches `renderByIndex` only — no overlap with PR #663, which owns `renderByComponent` in the same file. |
||
|
filip131311
|
7e7f7f73b6 |
fix(profiler): resolve a Metro logicalDeviceId instead of calling it an Android device (#687)
Fixes #618. ## The reported symptom is the mildest part `debugger-connect` returns a Metro `logicalDeviceId`, and when several devices share one Metro the debugger tools *instruct* the user to pass it (#522). But profiler sessions are keyed by the list-devices id, and `classifyDevice` decides platform purely from an id's shape — an opaque logical id matches nothing, so it falls through to `"android"` (`utils/device-info.ts:52`). Reproduced against the built code: ``` ios ← 1E273101-2926-4A76-88D0-544C7EA5C2FD (real UDID) android ← bb2e9dd6a3f14c8e9d0f1a2b3c4d5e6f (Metro logicalDeviceId) android ← "" (empty string) android ← "totally-bogus" ``` Because that id names **no existing session**, `services()` builds `NativeProfilerSession:<logicalId>`, and the registry mints services on demand — so it resolves to a **brand-new session, frozen to android, holding no capture**. The user isn't told the wrong word; the tool is reporting on a session that never recorded anything. Fixing only the platform string would still be wrong, since the data sits under the UDID. It reaches past this one message: - `native-profiler-start` dispatches the same misclassified id down the Android path, where `platforms/android.ts` hands it to **adb as a serial**. - `react-profiler-status`, `-fiber-tree` and `-renders` build session URNs with **no canonicalization**, so following the debugger tools' advice opens a **second CDP connection** to a device that already has one — precisely what the alias map in #528 exists to prevent. ## The fix `device_id` resolves through the alias map **in the schema itself**. That placement is the whole point: the registry parses params exactly once and hands the same object to `services()` and `execute()` (`registry.ts:125-153`), so one transform covers URN construction, the platform branch, and every id the tool forwards to a platform impl — including the ones passed to `adb` and `simctl`. Canonicalizing inside `services()` alone would leave those disagreeing with each other, which is how the bug got past the places that *did* canonicalize. Verified on the repo's zod 4.4.3 that this is invisible to callers: ``` input JSON schema → {"type":"string","minLength":1,"description":"…"} parse(LOGICAL) → REAL-UDID double-parse → REAL-UDID (http.ts parses, then invokeTool parses again) parse("") → rejected ``` **`classifyDevice` is deliberately untouched.** Its android fallback is load-bearing — a physical Android serial is an arbitrary manufacturer string with no matchable shape (the fixtures in-tree are `8a44101d` for a logical id and `HT82A0203045` for a real serial; both are opaque alphanumerics, so any rule strict enough to reject one rejects the other). There is a test pinning that. ## Canonicalization alone would not have been enough Aliases are learned on debugger connect (`js-runtime-debugger.ts:267`) and **forgotten on dispose** (`:287`). A user who profiles, lets the app reload, and then asks for a report has nothing left to resolve through — so "alias unknown" is the *ordinary* state, not an edge case. So a session with **no capture state at all** now reports that without naming a platform, and points at `list-devices` as the id to use. The platform-specific messages are kept for the states that actually prove a platform — an Android user who started but never stopped still gets *"No Android trace loaded"*, and that existing test fixture required no change. ## Tests 12 new cases: alias resolution, the URN/platform actually produced by `services()`, pass-through once the alias is forgotten, the published schema keeping `string`/`minLength`/description while hiding the transform, empty-id rejection, and the neutral gate firing for a never-captured session **regardless of which platform the shape guessed** — while the Android-specific message survives when a trace proves the platform. Three of them exist to stop a future "tidy-up" reintroducing this: a physical Android serial, a wireless-debugging address, and an emulator serial must all still classify as they do today. Mutation-verified: removing the canonicalization fails 2; removing the neutral gate fails 2. Full tool-server suite green — 3098 passed / 297 files. ## Scope Deliberately limited to the eight profiler tools that route to a `NativeProfilerSession`, which is what closes this bug. **Not in this PR**, because it rewrites agent-facing semantics rather than fixing a defect, and a revert of the doc churn shouldn't revert the fix: - The remaining 16 Metro-family tools carrying `device_id` (24 in total across the registry). - Seven tool descriptions that say *"Device logicalDeviceId from debugger-connect"* — stale, since `react-profiler-start`/`-stop` both canonicalize before building their URN and cache under the canonical id, so the session is keyed by the list-devices id either way. - **The skill docs, which drive the agent harder than any zod description**: `argent-metro-debugger/SKILL.md:26` calls `device_id` *"a.k.a. logicalDeviceId"*, `:34` says to *"use it as the device_id for every subsequent debugger call"*, and `references/failure-scenarios.md:10` repeats that after a crash — which is exactly the alias-forgotten path. Also noted for a follow-up: `platforms/android.ts` uses `params.device_id` as the adb serial in some places and `api.deviceId` in others; that divergence closes by construction once the family change lands. And a bogus android session minted *before* this fix can linger in a long-running server — inert, and new calls no longer reach it. --- > **Stacked on #663** (`filip/profiler-component-name-resolution`). Both add an import after `readCommitTree` in `profiler/query/profiler-commit-query.ts`. Rebased on #663, so review only the top commit. GitHub retargets it to `main` when #663 merges. |
||
|
filip131311
|
f136c024cc |
fix(profiler): accept the component names the analyze report actually prints (#663)
Fixes #632. ## The problem `react-profiler-analyze` strips `Forget()` / `Memo()` / `ForwardRef()` wrappers for readability, then the drill-down tools matched the raw DevTools name with `===`. The report names a component and the very next call refuses it. React Compiler is on by default in Expo SDK 54, so this hits essentially every new Expo app. ## Reproduced live Real session on an Expo SDK 54 app (`experiments.reactCompiler: true`, RN 0.81.5) — 821 fiber renders, 16 commits. The report printed `` `ParallaxScrollView` [React Compiler] ``: ``` profiler-cpu-query component_cpu ParallaxScrollView → "_Component `ParallaxScrollView` not found in commit data._" profiler-cpu-query component_cpu Forget(ParallaxScrollView) → "_No CPU samples found during `Forget(...)` commits._" ``` The two messages differ, and that is the proof: "not found in commit data" means the name never matched; the other is emitted only *after* a match. **Two things beyond the report:** 1. **`profiler-commit-query mode=by_component` has the same bug** — same refusal, full table under the raw name. 2. **It is not React-Compiler-specific.** `Memo()` and `ForwardRef()` are stripped too; the same report printed `` `InnerScreen` [forwardRef] ``. ## The workflow was a deadlock The AST index is keyed on bare source identifiers, so `react-profiler-component-source` accepts only the name the query tools reject: | name | query tools | component-source | |---|---|---| | `IconSymbol` | ✗ not found | ✓ file + line + source | | `Forget(IconSymbol)` | ✓ resolves | ✗ not found | There was **no single name an agent could carry** from analyze → drill-down → read source. Fixing only the query half would have left the issue's stated workflow broken. ## Approach Match a query against each recorded name's **display form**, rather than stripping both sides. Stripping both sides looks equivalent and is not: - it would quietly answer a query for `Forget(Foo)` with `Memo(Foo)` when only the latter exists — the agent asked about the compiler-wrapped fiber and silently got the memo one; - it isn't even reliable, because the display stripper stops after 4 wrappers, so a deeply wrapped name still *displays* with a wrapper and would never match. Comparing against the display form compares against exactly the string that was printed. **Exact matches always win and are never widened.** `StaticContainer` and `Memo(StaticContainer)` both appear in one ordinary 18-second session — I checked all 70 distinct names in the dump, and that is the one real collision. Asking for the bare name returns the bare fiber, and the output now names the sibling instead of silently showing one of two. **Ambiguity is refused, not merged.** Where a display name maps to several distinct fibers the tools list the exact strings to retry with. `Memo(X)` and `X` are often one component's two fibers whose durations nest, so a merged total describes no real component; and picking the largest silently picks the memo wrapper's inclusive time, which is the wrong fiber for self-cost analysis. The AST lookups get the reverse fallback: raw → stripped → expo-router file suffix removed (the report shows `TabTwoScreen(./(tabs)/explore.tsx)`; the source declares `TabTwoScreen`). Loosening is safe there in a way it is not for commit data — a wrong guess against the index simply misses and returns `found: false`. ## Verified against the live session | Case | Result | |---|---| | `component_cpu ParallaxScrollView` | resolves to `Forget(ParallaxScrollView)` — was refused | | `by_component ParallaxScrollView` | full table + `> Resolved … (React Compiler). Either name works here.` | | `by_component Forget(ParallaxScrollView)` | **byte-identical to before**, no note | | `by_component ParallaxScroll` (typo) | miss + `Forget(ParallaxScrollView)` as a re-queryable suggestion | | `component-source Forget(IconSymbol)` | `found: true`, `icon-symbol.tsx:28` — was `found: false` | | `component-source Forget(TabTwoScreen(./(tabs)/explore.tsx))` | `found: true`, `app/(tabs)/explore.tsx:12` | ## Visible report change (intentional) Fixing the analyze-side lookup means wrapped components stop losing their source location. In the live session exactly one row changes — every other row is a node_modules component with no user source: ``` before: | `IconSymbol` [React Compiler] | 6 | 0.1ms | … | — | after: | `IconSymbol` [React Compiler] | 6 | 0.1ms | … | `ui/icon-symbol.tsx:28` | ``` ## Deliberately out of scope - **`profiler-combined-report.ts:448`** correlates leaks to components by substring-matching raw names into native symbols, so wrapped components are structurally excluded. Same bug class — *affected, deferred*, not unaffected. - **A deeper namespace bug, filed separately:** `react-profiler-stop` joins fiber metadata by name across two different namespaces, so wrapped fibers lose `parentName`, `hookTypes` and `isCompilerOptimized` entirely. Measured on this session: **41 wrapped fibers, 0 with any of the three**, versus 181/272 bare fibers carrying `parentName`. That is why the report's header says `React Compiler: ✗` while the same document tags six components `[React Compiler]`. ## Checks - 3104 tests pass (298 files); 18 new, built on names taken verbatim from the real session - the `annotateComponentName` move is a pure refactor — verified output-identical before anything else changed - `03-tag.ts`'s `ANIMATED_PATTERN` deliberately left alone (it matches unanchored and covers `Motion`/`Transition`, which the stripper must not) - tool `description:` fields unchanged, so the SpiderShield average is untouched; skills gate 10.0; prettier, eslint, both typechecks clean --- > **#687 is stacked on this branch** — it conflicts with this one in `profiler-commit-query.ts`'s imports, so it was rebased on top. Merge this first. |
||
|
Ignacy Łątka
|
c876fe8dbc |
ci: add knip anti-dead-code gate (#572)
## What Adds a CI gate for dead code, using [Knip](https://knip.dev). We didn't have one. ESLint's `no-unused-vars` only sees unused locals inside a module, so whole files nobody imports, exports nobody uses, and dependencies declared but never required all slipped through. Knip covers that gap in one tool (roughly `ts-prune` + `depcheck` + `unimported` combined). The new `.github/workflows/knip.yml` workflow ("Dead Code", job `Knip`) runs `npm run knip` on push and PR to `main`. `CONTRIBUTING.md`'s pull-request checklist gains it as a step, with the reason it has to run against a tree with no build output. Knip reads source directly, mapping each package's `dist` entry back to `src`, so the job needs no workspace build. It installs with `--ignore-scripts`: an analysis that only reads `package.json` and source gains nothing from the seven packages here that carry a native install script, six of which reach this runner (`fsevents` is darwin-only). Five try a prebuilt binary before compiling, and `dtrace-provider` always runs node-gyp but ends in `|| node suppress-error.js` — and off mac/solaris/freebsd its `binding.gyp` picks a stub target with no sources, so on ubuntu-latest node-gyp builds nothing for it and `node-pty` is the only package that reaches a compiler at all — an `optionalDependency`, whose failure npm ignores. The exposure is a bump that leaves a package with no prebuild for the runner, which is exactly what `dependabot.yml` pins `tree-sitter` against (0.25.0 ships no Node 24 prebuild and its compile breaks `npm ci`). While adding it I checked whether any other installed checker was missing a gate. None were: ESLint (`lint.yml`), Prettier (`format.yml`), `tsc` build + `typecheck:tests` + `typecheck:scripts` + vitest (`unit-tests.yml`), the workspace-version / focused-test / merge-marker checks (`repo-hygiene.yml`) and lockfile sync (`lockfile.yml`) are all already enforced. Dead code was the only gap. ## How it's enforced `npm run knip` is two invocations: ``` knip --exclude exports,types && knip --include exports,types,classMembers,nsExports,nsTypes --max-issues 206 ``` Every issue type knip reports is an error — that is its default severity, so `knip.jsonc` sets no `rules` block at all. The pre-existing backlog is bounded by the `--max-issues` ceiling on the second pass instead of being hidden behind a non-failing severity: it cannot grow unnoticed, and the number comes down as follow-up passes pay it off. When a change legitimately adds an export nothing imports yet, the number moves up in the same commit — that bump is what review should see, rather than the contributor deleting someone else's backlog to make room. Two PRs making that identical one-line edit merge without conflicting, so `main` lands one short of the truth and its push-triggered run is what catches it. Splitting the passes also keeps a real failure out from under the backlog — the enforced classes report on their own, and the second pass only runs once the first is clean. The second pass names `classMembers`, `nsExports` and `nsTypes` explicitly because they sit in knip's own `defaultExcludedIssueTypes`: `--exclude exports,types` never covered them and `--include exports,types` did not ask for them, so a new unused class member cost nothing and moving an export behind opaque namespace use took it out of the ceiling's scope entirely. `classMembers` has 12 findings on the unbuilt tree the ceiling counts (9 on a built one); the other two have none today. None of the three is gated at zero — all five parked classes share the one ceiling, so a new `nsExports` finding passes if something else in the backlog is retired in the same PR. Naming `classMembers` also turns knip's `isSkipLibs` off (`create-options.js:122`), which makes it search declaration files for external references and **suppresses** 44 export/type findings the same run reports without it — 57/181 becomes 44/150. That, not the 12 findings it adds, is why the ceiling is lower than the two counts alone would suggest; `--include-libs` with no `classMembers` reproduces 44/150 exactly. The ceiling counts what CI counts, against a tree with no build output. Whether a workspace's `main`/`types` path resolves to an existing file changes the result, and not by way of anything in that file — a zero-byte `packages/registry/dist/index.js` moves the count just as a real build does, while a zero-byte `dist/zzz.js` does nothing. A built tree reports fewer issues — 43/147/9 = 199 built, against 44/150/12 = 206 unbuilt — so `npm run knip` passing after a local `tsc --build` is not evidence the job will pass, which is how the Knip job first went red on 199. The ceiling is 206 and it is exact: 206 passes, 205 fails. Remove every `dist/` before trusting a number from it. The ceiling is one total across those classes, so it bounds the backlog rather than each class: paying off one parked export licences one new one elsewhere in the same PR. Knip has no per-class ceiling. `treatConfigHintsAsErrors` is on, so an exemption that stops suppressing anything fails the job rather than printing a hint below the backlog. ## Config `knip.jsonc` documents the false positives so the gate is both green and meaningful. - **`@swmansion/argent`** is a thin dispatcher whose real payload is an esbuild bundle of the other workspaces (`packages/argent/scripts/bundle-tools.cjs`). Knip can't follow imports across that boundary, so packages consumed only by the bundled-in code read as unused; 11 are exempted by name here, though not all for that reason — the http3-quiche transport is pulled in by `@fails-components/webtransport` itself at runtime, and no first-party file imports it. They're exempted **by name**, not behind a catch-all: `ignoreDependencies` also suppresses `unlisted`, and a pattern that matches everything would take this workspace's own `src/` and `scripts/` out of that check — where an import with no matching declaration is a genuine bug rather than a bundler artifact. (It reaches `unresolved` only nominally: knip matches the list against a package name parsed out of the specifier, and an unresolvable relative path yields none, so even a catch-all leaves those reported. `ignoreUnresolved` is the option that does silence them.) - **`tool-server`** reaches a few packages in ways static analysis can't see. The exemptions cover two distinct classes, kept apart in the config so each stated reason matches the finding it silences: - *declared but loaded too dynamically to detect* (would be reported as **unused dependencies**): `tree-sitter` / `tree-sitter-typescript` via dynamic `_require`, and the http3-quiche transport as a runtime companion of `@fails-components/webtransport`; - *imported but deliberately absent from that `package.json`* (would be reported as **unlisted dependencies**): `sharp` (kept out so the ~30 MB native binary isn't a cost every consumer pays) and `electron` (an `optionalDependency` of the published package at one call site, the Electron main-process API at the other). - Root `scripts/`: knip resolves those from `package.json` scripts **and** from `run:` steps in `.github/workflows` via its github-actions plugin — multi-line `run: |` blocks included. What the plugin doesn't parse is a PowerShell body, so `scripts/ci/windows-chromium-e2e.mjs` — invoked at `windows-e2e.yml:176`, in the `windows-latest` job where PowerShell is the default shell — is the one entry declared by hand. ### Known limits - `ignoreDependencies` exempts a package **name across its whole workspace**, in both directions; knip has nothing narrower. While `sharp` is listed for `tool-server`, an undeclared `import "sharp"` anywhere in that workspace passes too. The list is held to packages that genuinely cannot be declared, for exactly that reason. - Knip analyzes JavaScript and TypeScript only (`DEFAULT_EXTENSIONS`), so **only 11 of the 30 files under `scripts/` are in the gate at all**. The other 19 — 15 `.sh` files, `e2e-full/lib/report.py`, `ci/e2e-chromium-page.html`, and e2e-full's `README.md` and `results/.gitignore` — are invisible to it however they are reached — including the whole `scripts/e2e-full/` harness (13 files), invoked from nothing outside its own directory, its only mention elsewhere being a `.prettierignore` entry. - Reachability's widest hole is the workspace entry. Whatever `src/index.ts` re-exports is reached, so a single `export { x } from "./dead-module";` takes that module out of `files` altogether — verified against a module reported as an unused file one run earlier. The same line applied to parked exports retires them from the count without deleting anything: re-exporting `equalsCI`, `includesCI` and `Settled` from `tool-server`'s entry takes 206 to 203 and frees three ceiling slots. The ceiling bounds what knip can see, not what is dead. - Test files are entry points, so a production module kept alive only by its own test still passes `files`. Being reached by a test says nothing about being reachable in production, which is what the check approximates, and two modules sit in exactly that gap: `tools/paste` is imported by two test files and registered by nothing, so it is unreachable over MCP and HTTP, and `utils/ios-profiler/lifecycle.ts` is a re-export barrel whose header keeps it "for source compatibility with existing imports", of which its own test is now the only one — its four production consumers all import `profiler-shared/lifecycle` directly. That single test import is exactly what keeps `files` from reporting it: repoint it and knip returns `Unused files (1)`. `knip --production` is the lens that reports the class; the repo isn't clean under it — 13 unused files (those two, seven root scripts, `packages/argent/scripts/{bundle-tools,observe,sync-readme}.cjs` and `packages/registry/tests/helpers.ts`) with the export/type backlog at 227/206 — measured with `classMembers` off, so comparable to the 57/181 above rather than to the ceiling's 44/150 — so a production-mode pass is a follow-up of its own. - The same entry treatment hides the test trees themselves. Root `package.json`'s `test:scripts` runs `node --test`, which is what makes knip's node plugin contribute its test entries, and that plugin registers four entry patterns in **every** workspace: `**/test/**`, plus `*{.,-,_}test.<ext>`, `test-*.<ext>` and `test.<ext>` matched anywhere. So none of the **380** JS/TS files under `packages/*/test/` can be reported unused, and under `packages/*/tests/` only a name none of those three match can — of today's 15 that is `registry/tests/helpers.ts` alone, which is why it is the lone `tests/` entry in the `--production` list above. A `tests/test-utils.ts` or `tests/test.ts` would be just as invisible. A dead `zz.test.ts` planted in `packages/registry/tests/` passes; a dead `zz-helper.ts` in the same directory fails. - `knip@5.88.1` is the last 5.x release ever published — 6.0.0 landed the next day and `latest` is now 6.x. The pin is deliberate: knip 6 removed the `classMembers` issue type outright, and the shipped script hard-errors on it (`ERROR: Invalid issue type: classMembers`, exit 2). Upgrading is therefore not a version bump — it means re-deriving the ceiling, rewriting the paragraph that explains it, and resolving the pass-1 findings knip 6 adds (six unlisted binaries: `tail`, `plutil`, `xctrace`, `netstat`, `pkill`, `pgrep`; two unresolved imports: `/usr/bin/osascript`, `/usr/libexec/PlistBuddy`). Dependabot's `npm-minor-patch` group cannot move it, since no 5.x exists above 5.88.1; the `npm-major` group will keep offering the 6.x jump. - Knip folds `optionalDependencies` into the declared set, so importing `electron` or `node-pty` is not `unlisted` — they pass because it read them. What it never reports is an optionalDependency nothing uses, so only that direction is outside the gate, along with `argent-cli`'s runtime `req("node-pty")`, resolved through a `createRequire(import.meta.url)` in `lens-pty.ts`, which knip can't see either. ## Dead code this found - `@types/semver` and `@argent/telemetry` from `packages/argent` — both were being suppressed by exemptions whose stated bundler-boundary reason could not apply. esbuild strips types and never resolves `@types/*`, and `packages/argent` names `semver` nowhere; `@argent/telemetry` is aliased to an absolute source path by `bundle-tools.cjs`, so the declaration bought nothing. Removing them is proved safe by building: four of the five bundles still emit and telemetry is inlined in each with no runtime `require` of it. The fifth, preview-window's main, sits behind a downloaded native asset absent from a script-free install, so it is covered by mechanism rather than observation — `bundle-tools.cjs` passes the same `alias` map to every build, and esbuild's `alias` bypasses node resolution entirely: deleting `node_modules/@argent/telemetry` outright leaves all four emitted bundles byte-identical. Removed alongside, but **not** gate-reported: the `benchmark` npm script in `packages/argent`, whose `scripts/benchmark.cjs` does not exist. A dangling script target is a blind spot of this gate — restoring the script leaves knip at exit 0. - Four files with no importers: the `render-hook` injected script, react-profiler's `source-map` + `snippet-extractor`, and `scripts/setup-in-workspace.cjs` (referenced nowhere, and its own usage string names a `scripts/setup-project.cjs` that doesn't exist). - `superagent` in `tool-server`'s `test/http-streaming.test.ts` — imported for a type but declared in no manifest in the repo, resolving only through npm hoisting of supertest's own dependency. Reverting that import to `superagent` reports `Unlisted dependencies (1) … :3:32` and exits 1; `@types/supertest/index.d.ts:10` re-exports the identical type (`type Response = superagent.Response`). - `@clack/prompts` and `@moq/lite` from `tool-server`'s `package.json` — declared, imported nowhere. (`@moq/net` is the one actually used; `@moq/lite` isn't a peer of it.) Also deleted here, but **not** something this gate reported: `src/tools/simulator/simulator-server.ts`. It sat in the `files` blind spot above — `interaction-messages.test.ts` imported it, which is enough to count as reached. `setup-registry.ts` never registers it, so it was unreachable over MCP and HTTP, and that catalog roll-call was its only reference; the import and the catalog count (77 → 76) went with it. ## Lockfile Adding knip moves two transitive packages: `@napi-rs/wasm-runtime` 1.1.5 → 1.1.6 and `@tybys/wasm-util` 0.10.2 → 0.10.3. Knip pulls in `@oxc-resolver/binding-wasm32-wasi`, which floors `@napi-rs/wasm-runtime` at `^1.1.6`, lifting the hoisted entry that had resolved to 1.1.5 for `@rolldown/binding-wasm32-wasi`'s `^1.1.4`, which then cascades to `@tybys/wasm-util`. No runtime impact — both are support packages for the wasm32-wasi fallback bindings, which don't get installed where a native binding exists; `npm ls` reports both extraneous on darwin-arm64. Those two are the only resolved versions that change in the diff; the rest is knip's own tree (49 packages added) and `@moq/lite` removed. ## Verification - `npm run knip` exits 0. - **Coverage of the enforced set.** With no `rules` block, knip's included set is 15 issue types: ten enforced at zero tolerance by the first pass, plus the five the second pass names under the ceiling. Eleven mutations cover all ten, each flipping the gate to exit 1 and back to 0 on revert: | class | mutation | | --- | --- | | `files` | a throwaway `.mjs` in `scripts/`; a throwaway `.ts` in `packages/argent/src` | | `dependencies` | a bogus declared dep in `packages/tool-server/package.json` | | `devDependencies` | a bogus declared devDep in the same manifest | | `optionalPeerDependencies` | `bytes` moved to an optional `peerDependency` there | | `unlisted` | an undeclared import in `packages/argent/src/cli.ts` | | `unresolved` | an unresolvable relative specifier in the same file | | `binaries` | a root `package.json` script invoking a nonexistent binary | | `duplicates` | `export default sleep` alongside the existing named export in `utils/timing.ts` | | `enumMembers` | an unused member on an enum a second module imports and uses | | `catalog` | a `catalog` block in the root `package.json` naming a package nothing uses | One nuance worth recording: `duplicates` is narrower than it looks. It reports a symbol exported both by name and as `default` (`sleep|default`); a second *named* alias of the same symbol is not a duplicate, and a freshly added symbol nothing references is not reported at all — so several naive probes read as a miss. `enumMembers` needed a shape the repo does not currently contain, not an unreachable one. The only exported enum here, `ServiceState`, is re-exported through `@argent/registry`'s entry, where knip counts exports as used, so an unused member added to it is **not** reported — but an enum in a non-entry module that another module imports and uses does report one (`Blue ZzColor …/zz-enum.ts:3:3`, exit 1; deleting the unused member returns exit 0). That is the ninth of the ten covered. `catalog` is reachable here despite the repo using npm workspaces: knip's catalog loader falls back to `manifest.catalog` when there is no pnpm or yarn workspace file, so a `catalog` block in the root `package.json` reports `Unused catalog entries (1)` and exits 1. - The two new mechanisms discriminate as well: one extra unused export takes the backlog to 207 and fails the ceiling, and a stale `ignoreDependencies` entry produces `Configuration hints (1) … Remove from ignoreDependencies` and exit 1. - `npm ci --ignore-scripts` in a clean worktree, followed by `npx knip`, is green — the job doesn't need any of the postinstalls it now skips. - `tsc --build`, `typecheck:tests` (all workspaces), `typecheck:scripts`, `test:scripts`, `eslint . --max-warnings 0`, `prettier --check .` and the full workspace vitest suite (4711 passed, 3 skipped across 13 workspaces; 3070 of those are `@argent/tool-server` alone) are green; `package-lock.json` is untouched by the review pass. |
||
|
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>
|
||
|
Juliusz Wajgelt
|
67a4481868 |
refactor(tool-server): use @swmansion/argent-cloud-sdk for MoQ + input protocol (#743)
## Summary argent and the other client of the same device backend each hand-rolled the same MoQ client and `DataChannelCommand` protobuf encoder, with no shared source — argent carried a vendored copy of the `.proto` as an inline string that could silently drift from the server schema. Both now use `@swmansion/argent-cloud-sdk`, where the encoder is checked byte-for-byte against the canonical `.proto` in tests, so drift fails the build instead of shipping. - **`utils/moq-client.ts`** (195 → ~50 lines) is now a thin adapter over the SDK's `MoqDeviceSession`. It keeps the `MoqClient` interface exactly (`sendControl`, `screenshot` → `Buffer`, `close`), so `blueprints/simulator-server.ts` and `createMoqTransport` are untouched. argent still resolves its endpoint through `sim-remote moq-info` (preserving daemon-based session sharing) and still publishes on its own `"argent"` broadcast path. - **`utils/datachannel-proto.ts`** (201 lines → re-exports) so its ~10 import sites keep working unchanged. - **Dependencies:** drops `protobufjs` and `@moq/lite`, both now unused. `@moq/net` is kept and still dedupes to a single copy. No behavioural change intended. ## Verified end-to-end against live infrastructure Ran the migrated client against a real remote simulator (iPhone 17 Pro, iOS 26.5) on a live staging machine, driving this branch's `moq-client.ts` through the published SDK **0.1.2**: | Check | Result | |---|---| | MoQ session open (WebTransport, cert-pinned) | **PASS** | | Screenshot | **PASS** — valid PNG 1206×2622 | | Sequential screenshots | **PASS** | | Concurrent screenshots | **PASS** | | Input round-trip (tap an app icon) | **PASS** — tapping Settings opened Settings, confirmed visually in the returned screenshot | | Screenshot after concurrent burst | **PASS** | | Session teardown | **PASS** | Plus the existing suite: full workspace `tsc --build`, **3372 tests passing** (1 skipped), and `npm ls @moq/net` confirming a single deduped copy. ### A bug this surfaced, now fixed Concurrent `screenshot()` calls hung under SDK **0.1.0**. The server answers one screenshot per session at a time; 0.1.0 had dropped the serialisation the old hand-rolled client had, so a second in-flight request was never answered and its promise never settled — a regression against pre-SDK behaviour that only showed up under live traffic, with no unit test catching it. Fixed in SDK 0.1.1 (queue requests, plus a 30s request timeout so a dropped response errors instead of hanging). This PR depends on `^0.1.2` — a republish of 0.1.1 through the release workflow carrying npm build provenance, whose shipped `dist` is byte-identical (verified by SHA256), so the live results above hold for it unchanged. ## Notes for reviewers - The SDK subscribes to the screenshot track lazily, so a video-only consumer doesn't open a subscription it never reads. argent requests screenshots, so this path is exercised. - Screenshot responses are matched by the request id the server echoes, so a late or duplicate frame can't be handed to the wrong waiter. |
||
|
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
|
19782f4e1a |
feat(secrets): resolve {{secret:…}} from .env files, not just the environment (#734)
## Why
A credential could only reach a typing tool through an
`ARGENT_SECRET_<NAME>` environment variable. That is the right CI
contract — and stays the highest-priority source — but interactively it
is poor ergonomics: an editor-hosted tool-server inherits the
environment its editor was launched with, so changing a credential means
restarting the editor, and there is nowhere to write one down that
survives a reboot. "Is loading these from a `.env` file on the roadmap?"
is the recurring ask.
## What
`{{secret:NAME}}` now resolves through an ordered chain, first match
wins, re-read on every resolution — so editing a file applies to the
next keystroke with **no server restart** (the same runtime-read design
`ios.additionalDeviceSets` already uses).
| # | Source | Which keys it exposes |
| - | ------ | --------------------- |
| 1 | `ARGENT_SECRET_<NAME>` in the environment | prefixed variables
only — unchanged, still wins |
| 2 | `<project>/.argent/secrets.env` | every key (gitignore it) |
| 3 | `<project>/.env.local`, then `<project>/.env` | **only
`ARGENT_SECRET_`-prefixed keys** |
| 4 | `~/.argent/secrets.env` | every key, any project |
Covers `keyboard`, `paste`, `run-sequence` keyboard steps, and flow
`type:` steps — all four already funnel through the same resolution
point, so nothing else changed.
Also adds `argent secrets [list] [--json]` so a setup is verifiable
without an agent: names only, never values, shadowed names marked, and a
warning when a linked remote tool-server means resolution happens on a
different machine.
## The one design decision worth reviewing
**Row 3 is why this isn't just "read the `.env`".** An agent composes
the tool call, so anything reachable through `{{secret:…}}` is
exfiltratable by a prompt-injected one — type it into a field, then read
the field back. "Which values may argent type" has to stay an allowlist
the user wrote deliberately.
So: a *dedicated* argent secrets file exists for no other purpose and
therefore **is** the allowlist; a file the app *shares* keeps the prefix
as the allowlist, exactly as the process environment does. Your
`STRIPE_SECRET_KEY` sitting in the same `.env` stays unreachable —
verified live, not just asserted. Adding `ARGENT_SECRET_` to one line is
the deliberate act that exposes it.
Two smaller calls, both reversible:
- The chain lives in `@argent/configuration-core`, not the tool-server,
so the CLI reads the same code; the tool-server keeps only the
placeholder engine.
- The project scope is discovered by the **cwd walk-up every other
project-scoped configuration uses**. A per-request project root (header
→ `ToolContext`) was considered and rejected as new protocol surface
across two client paths; reusing flows' module-global
`setActiveProjectRoot` was rejected as a cross-project leak on a reused
server. The cost is that a tool-server whose cwd sits outside the
project skips project files — mitigated by the failure now listing every
path it consulted, and by `~/.argent/secrets.env` needing no project at
all.
## Verification
Unit + integration: tool-server 3122, configuration-core 122 (19 new),
argent-cli 370 (6 new). Lint and prettier clean. All four publish
bundles build.
Live, driving this branch's tool-server on :3001 against iPhone 17 Pro /
iOS 26.4 and the Electron testbed:
- global `~/.argent/secrets.env` → typed correctly; the response echoed
only the placeholder
- prefixed key in the project `.env` → typed; **unprefixed
`STRIPE_SECRET_KEY` in that same file → rejected**
- `.argent/secrets.env` beat `.env`; the environment variable beat both
- edited a secrets file mid-session → applied on the very next call,
same server pid
- a chromium flow with `type: { text: "{{secret:APP_PASSWORD}}" }` → 3/3
pass, asserting on the greeting the app rendered, so the *correct value*
provably landed
- an undefined name inside a flow surfaces the full source list in the
step report
Sample of the failure output (no value ever appears):
```
Unknown secret "LOGIN_PASSWORD" — no source on the machine running the tool-server defines
LOGIN_PASSWORD. Available secrets: APP_USER.
Sources consulted, first match wins:
1. environment (ARGENT_SECRET_*) — 0 secrets
2. /path/to/project/.argent/secrets.env — found, 1 secret
3. /path/to/project/.env.local — not found
4. /path/to/project/.env — found, 1 secret
5. /Users/me/.argent/secrets.env — not found
To make it available, ask the user to add `LOGIN_PASSWORD=…` to …
```
## Out of scope
No general flow variable/parameter system — a non-secret external value
(a test account's email, a staging tenant id) can use the same
placeholder, but argent treats every such value as a secret: redacted
from errors, never echoed, so it is not for something a report should
show. No config key pointing at an arbitrary secrets file. Remote
(`argent link`) still resolves on the remote host, which the CLI now
says out loud.
|
||
|
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. |
||
|
filip131311
|
c52f2dba9e |
fix(screenshot-diff): say when two screenshots were resized to be compared (#689)
Fixes #617. ## Reproduced three ways **The reported false green**, using an image whose 2× downscale is lossless so no resampling noise can mask it: ``` baseline 800x1600 current 400x800 (exact half) → status: unchanged pixel_mismatch: 0% (size difference never mentioned) ``` **Worse on real screenshots** — a 1152×648 capture vs its aspect-preserving half: ``` → status: changed pixel_mismatch: 0.35% changed_areas: 8 Text changes: Disappeared "General" / Appeared "Remotes and Devices" ``` The content is identical; every region and both OCR "text changes" are resampling artifacts. So the tool doesn't merely stay silent about the size difference — it manufactures a plausible-looking regression report out of noise. **The control shows the asymmetry is the bug** — aspect-*breaking* correctly reports `dimension_mismatch: expected=1,152x648 actual=1,152x400`. Sizes are disclosed exactly when the tool refuses to compare, and never when it silently rescales. ## The fix `PngDiffResult` gains `sizeNormalization` (both original sizes plus the size compared at), populated only when a rescale actually happened — so its presence *is* the signal that the comparison carries resampling uncertainty. The summary prints it directly under the status: ``` - status: resized_no_change - size_normalized: baseline=800x1,600 current=400x800 compared_at=400x800 - the inputs share an aspect ratio but not a resolution, so the larger was downscaled before comparing; any pixel or text differences below may include resampling artifacts, and the diff images are at compared_at rather than full size - re-capture the baseline at the same scale as the current image (screenshot with scale: 1.0) to compare without resampling ``` **`resized_no_change` is checked last**, after the difference checks, so a rescaled comparison that *did* find something is still a plain `changed` — a caveat status must never soften a real regression. There's a test pinning that ordering specifically. **The name matters.** `unchanged_after_resize` contains `"unchanged"`, so the one naive gate this protects — `summary.includes("status: unchanged")` — would still read it green. `resized_no_change` contains neither `"unchanged"` nor `"changed"`. ## What I deliberately did not do **Report `dimension_mismatch` whenever sizes differ** — the issue's primary suggestion. The normalization is deliberate: `screenshot-diff/index.ts:292-294` states that same-aspect normalization *"keeps a scaled capture diff-compatible with a baseline saved at any scale"*. The `screenshot` tool defaults to **0.3** for iOS/Android while the diff's live capture is full-res, and on Android the live side falls back to the default scale when full-res capture fails. Hard-failing would break the case this code exists for. **Expose the internal `normalizeSizes` flag.** `screenshot-diff-tool.test.ts:10-23` is titled *"rejects public tuning options so defaults stay internal"* and asserts exactly this kind of param is refused. And the better remedy is the one the summary now gives — re-capture the baseline at the same scale, which the skill already instructs and which removes the resampling rather than reporting it. Worth reconsidering separately if a user actually wants "green means byte-identical". *(An earlier draft proposed exposing it via `z.coerce.boolean()`. Worth recording why that was dropped beyond the posture conflict: in zod, `z.coerce.boolean("false")` is **`true`** — so a user asking for strict comparison would have silently got normalization anyway, re-creating this exact bug through the fix's own escape hatch.)* **Suppress the OCR noise.** Text analysis runs on each *original file at its own resolution* and only rescales the resulting bounds, so a diff-time filter cannot separate "OCR read a 576px image badly" from a real text change. Suppressing it whenever normalization occurred would silence text analysis on every scaled comparison — a new, worse false green of exactly the class this issue is about. Disclosure is the honest treatment; running OCR on the normalized pair is a separate change to `text-diff`'s contract. ## Tests Three new cases plus three strengthened ones: the false green now discloses and no longer reads `unchanged`; a normalized comparison with a real change still reports `changed` *and* still discloses; equal sizes say nothing; and the retina case still compares rather than hard-failing. Mutation-verified both halves — removing the status fails the #617 regression test; moving its check before the difference checks fails the ordering test. Full tool-server suite green — 3089 passed. No existing assertion needed changing, which is also why this survived: nothing pinned it. ## Notes for the reviewer - The flow snapshot path is untouched and **cannot** reach normalization: full-screen baselines key on the capture's own pixel dimensions (`flow-visual.ts:212`), so a differently-sized capture misses the baseline entirely and fails; `cropOn` diffs already pass `normalizeSizes: false`. - `SummaryStatus` is unexported and nothing switches on it — the status reaches the outside only as text inside `summary`, which the MCP layer passes through verbatim. No skill doc, flow assertion or e2e script matches on a status string. - The diff artifacts were already produced at the normalized size; that was simply never stated. The caveat now says so. |
||
|
filip131311
|
35ab44dc03 |
fix(chromium-tabs): say why a tab cannot be opened in an Electron app (#659)
Fixes #634. ``` before: [Tool:chromium-tabs] Not supported after: [Tool:chromium-tabs] Could not open a tab: Not supported. An Electron app has no browser-level target creation, so a tab cannot be made from outside it — have the app open one (e.g. `window.open()` through `debugger-evaluate`) and it will show up in `chromium-tabs list`. The list, select and close actions are unaffected. ``` The old text is Electron's own CDP answer to `Target.createTarget`, passed through untouched. The limitation is real, but the message names neither the cause nor a way forward, and reads as though the tool were unavailable — when only this one action is. ## Two things I checked before writing the message - **The workaround actually works.** On the Electron testbed I ran `window.open('about:blank')` via `debugger-evaluate` and the new window appeared in `chromium-tabs list` as `t2`. I didn't want to ship advice I hadn't run. - **The rest of the tool really is fine** — `list` returns correctly both before and after. ## Why the cause is kept rather than matched on `Target.createTarget` can fail for reasons other than Electron's missing browser shell — a dropped connection, or a real Chrome refusing the url. The CDP client keeps only the error's `message` and discards its numeric code, so branching would mean matching on the string `"Not supported"`, which breaks the moment Electron rephrases it. Instead any failure is wrapped, with the original message embedded. The explanation is worded as a property of Electron rather than a claim about *this* failure, so it stays accurate when the cause is something else — and the underlying error is still right there to read. ## Verification Live against the Electron testbed (Electron 42 / Chrome 148): the message above is the real output, and `list` still works alongside it. tool-server 3087 passed, plus a new test asserting the message keeps the original cause *and* carries the reason, the workaround, and the note that the other actions are unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
filip131311
|
11f384d37f |
fix(chromium): make Electron apps testable without window focus (#605)
## Problem Field report: *"electron needs focus to be tested."* Analysis confirmed three focus/visibility dependencies in the Chromium/Electron surface: 1. **Hidden window → throttled renderer.** A minimized/occluded window's compositor throttles: mouse-input acks stall **~5 s per event** (they wait on hit-testing), wheel scrolls hang (`gesture-scroll` already refused), `visibilityState` flips to `hidden`. 2. **Unfocused window → focus-gated app logic breaks.** `document.hasFocus()` is `false` before any input arrives; focus/blur-dependent UI misbehaves. 3. Focus-gated web APIs (clipboard already worked around it locally with `Page.bringToFront`). ## Fixes **1. CDP focus emulation on every page session** (`primePageSession`, né `enableCoreDomains`): `Emulation.setFocusEmulationEnabled(true)`, best-effort, re-applied automatically after tab switches (Playwright precedent). Live A/B on the Electron testbed: identical boot→unfocus→probe sequence gives `hasFocus:false` on main, `true` on this branch. Bonus verified by A/B (kill server → same minimized window flips to `hidden`): in current Chromium/Electron, emulation also pins reported visibility to `visible` and keeps input unthrottled while genuinely minimized — minimized tap latency **~5 s → ~65 ms**, wheel scroll works. *Trade-off (same as Playwright ships):* while attached, the app can observe neither real blur nor real hidden, so pause-on-hidden behavior is unobservable during a test. **2. Anti-throttling switches for every argent-booted Electron app** (`--disable-background-timer-throttling --disable-backgrounding-occluded-windows --disable-renderer-backgrounding`). Sessions are created lazily and die with the idle-exiting tool-server while the app outlives it — the flags cover that detached lifecycle plus emulation-less runtimes. Also covers flow-booted apps via the shared `bootElectronApp`. **3. Hidden-window fail-fast extended from gesture-scroll to gesture-tap + gesture-drag** via shared `assertChromiumWindowVisible`, under a new dedicated `CHROMIUM_WINDOW_HIDDEN` failure code (scroll migrated; `failure_stage` strings unchanged so telemetry stays joinable — analytics keyed on `CHROMIUM_INPUT_INVALID` + `chromium_scroll_window_hidden` should switch to the new code). With emulation active the guard is a backstop that fires only where emulation couldn't be applied — exactly where the stall is real. Keyboard/button stay unguarded on purpose: measured live, key events skip hit-testing and stay fast on hidden windows. ## Verification - Unit: 3075 tool-server + 87 registry tests green, `typecheck:tests` green, new tests for emulation priming, boot args, and the guard (all three tools). - Live matrix on the Electron testbed (dev tool-server): focused baseline; unfocused (`hasFocus` true pre-input, typing lands); minimized booted app (tap 69 ms, scroll works, typing + slider drag land, screenshot fine); minimized **external** un-flagged app on :9222 (tap 63 ms, scroll 460 ms — emulation alone suffices while attached). - Plan + implementation went through three adversarial review rounds (scope, CDP semantics, failure-code taxonomy/telemetry compat, doc accuracy). 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
filip131311
|
05f53502ab |
feat(tool-server): drive iOS simulators from additional CoreSimulator device sets (#600)
> Recreates #543 — GitHub auto-closed it when the base branch of the stacked PR (#542) was deleted on merge, and a closed PR cannot be reopened after its head is force-pushed. Same commit, rebased onto main. ## Summary **Was stacked on #542** (now merged). Consumes the `ios.additionalDeviceSets` configuration across the iOS tool surface, so simulators living in extra CoreSimulator device sets — e.g. **Radon IDE's** `~/Library/Caches/com.swmansion.radon-ide/Devices/iOS` — are discovered by `list-devices` and drivable by every tool, by UDID, exactly like default-set devices. ### The core piece: a UDID → device-set map (`utils/ios-device-sets.ts`) simctl scopes every operation to one device set, so each per-device call must know the target's owning set. The new module owns that mapping and the single argv choke point: - **Learned from discovery**: `list-devices` and the simulator watcher enumerate the default set + every configured set and `rememberDeviceSet` each sighting — any tool used after a list hits a warm cache. - **Lazy fallback**: an unknown UDID triggers one `simctl list` probe per configured set, cached on hit. An unknown-everywhere UDID resolves to the default set **without** caching (a device that appears later is found on the next call) — preserving pre-feature behavior and error messages byte-for-byte. - **`simctlArgsForUdid(udid, args)`** builds the full argv (`--set` injected only for additional-set devices); `simctlPrefix` covers multi-call sites that resolve once; sync variants (`simctlArgsForUdidSync`, `cachedDeviceSetForUdid`) cover the `ChildProcess`-returning and `execFileSync` call sites, each of which runs after an async step that warmed the cache. - Config is **read off disk per call** (the #534 design): editing `ios.additionalDeviceSets` applies without a tool-server restart. - **Guard**: a configured set whose directory doesn't exist is never queried — simctl would materialize the dir, turning a config typo into a stray directory. ### Wired consumers - **`list-devices`** — enumerates every set, tags additional-set devices with `deviceSet`, documents the field. - **`simulator-watcher`** — polls every set (a set failing to list throws → the tick is skipped, so a transient error still can't mass-dispose tracked services); warms the map for the injection path. - **simulator-server blueprint** — passes `--device-set` (the same flag Radon IDE passes to this shared binary); default-set spawns unchanged. - **`ax-prefs`** — the pre-boot AX plist now lands under the owning set's device dir (previously hardcoded to the default `CoreSimulator/Devices` root, so the write would land nowhere for these devices and describe would degrade). - **`boot-device`** — set-aware shutdown/boot/bootstatus; skips the Simulator.app attach + `CurrentDeviceUDID` write for additional-set devices (one Simulator.app instance displays one device set — these run headless and are streamed, exactly how Radon IDE presents them). - **Every remaining per-device simctl call site**: launch-app / restart-app / reinstall-app (both the direct impls and the `SimctlBackend` shared handlers), open-url, settings-permissions (privacy + install probe), the tvOS screenshot fallback, status-bar pin/restore, device-shutdown, the ios-host `simctl spawn` family behind describe/native-devtools, tv-control's AX daemon, and the native profiler's sync helpers (cache warmed at `startNativeProfilerIos`). **Out of scope**: `sim-remote` — the remote orchestrator host has its own device-set story. ## Testing - **Unit**: new `ios-device-sets.test.ts` (mapping, lazy probe + caching semantics, no-probe fast path with no sets configured, discovery enumeration/tagging/dedup, missing-dir guard); blueprint test asserts the `--device-set` argv; the profiler shell-injection mock extended for the new import. Full tool-server suite green: **276 files / 2752 tests**; `tsc --build`, eslint, prettier clean. - **Live, end-to-end on a real Radon IDE device** (iPhone Air, iOS 26.4, in `com.swmansion.radon-ide/Devices/iOS`), driving this branch's tool-server over HTTP with the config set at project scope: - `list-devices` → the Radon device appears, tagged with its `deviceSet` (invisible to argent 0.16.1 on the same machine); - `boot-device` → boots headless in its set, no Simulator.app window; - `screenshot` → real home-screen PNG via simulator-server spawned with `--device-set`; - `launch-app` (Settings) → launched via `simctl --set launch`; - `describe` → full accessibility tree from the Settings screen, i.e. the AX bootstrap (set-aware prefs + in-sim ax-service daemon via `simctl --set spawn`) works; - clean shutdown; default-set devices meanwhile listed and behaved exactly as before. |
||
|
filip131311
|
fc09612f30 |
feat(config): recordings.directory — configurable save location for screen recordings (#601)
## Summary Adds a `recordings.directory` configuration entry (built on the #534 config system) that lets users choose where finished screen recordings (mp4) are saved, instead of the fixed `<project>/.argent/recordings/`. - **Schema entry** (`configuration-core`): scopes `project` + `global`, `prioritize-local` merge (project wins when both are set). Picked up automatically by `argent config set/get/unset/list`. - **Value semantics**: absolute path, `~`-prefixed (expands to home), or relative — relative anchors at the project root (home dir when not in a project). - **Client-side wiring** (`argent-tools-client` `durableSaveTarget`): the wire `saveDir` hint stays `.argent/recordings` (wire contract unchanged → old/new client-server interop). Past the existing client allowlist, the recordings hint is remapped to the configured directory. ## Security model unchanged The allowlist exists because `saveDir` arrives from a possibly-hostile remote tool-server. That gate still runs first and is unchanged: a server can only *select* the sanctioned recordings category, never name a destination. The configured directory comes from the client's own trusted config files. Exclusive never-overwrite writes (`name (2).mp4`), the durable download size caps, and the symlink real-path guard all remain; for a configured dir the realpath check degenerates to "the dir resolves to itself" (a user-chosen symlink is legitimate, matching how the default base may be symlinked). A test asserts hostile wire values (`.git`, `..`, absolute) stay rejected even with config set. ## `argent link` interaction Materialization always runs on the **client** (co-located → copy from `hostPath`; remote → download over `/artifacts/:id`), and the config is read there at materialize time — so with a remote tool-server the mp4 lands on the client host per the *client's* config, on both paths. ## Testing - New unit tests: 10 in `argent-tools-client` (absolute/`~`/relative resolution, local-vs-global precedence, both materialization paths, hostile wire values with config set) + 4 in `configuration-core` (scope writes, trim, precedence, validation). - Suites green: tools-client 188, configuration-core 87, tool-server screen-recording 62, CLI config 16; full workspace vitest 4727 pass (3 pre-existing `scripts/*.test.mjs` node:test files don't parse under root vitest — unrelated). - **Live E2E** on an iPhone 15 Pro simulator via the branch tool-server: real recording captured, then materialized through the branch client with configs written by the real `argent config` CLI in a sandboxed HOME/project. 7 scenarios green: default location, global-only (`~` path), project-only (relative), both-set → project wins (co-located **and** remote download with Bearer auth), unset fallbacks, `argent config list` rendering. Remote-downloaded mp4 ffprobe-verified (byte-exact size, 4.17 s duration); name-collision suffixing observed live. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Stanisław Chmiela
|
7189333ac4 |
feat(tool-server): add human-readable log message (#582)
## Summary Adds tool-owned, human-readable interaction messages for every tool definition. Each tool defines its own started, completed, and failed wording (as implemented in https://github.com/software-mansion/argent/pull/367. Messages can use validated parameters, successful results, and safe failure codes. They are to avoid sensitive input such as typed text, cookie values, JavaScript expressions, and URL credentials. Bridges the gap between current Argent <img width="954" height="493" alt="Screenshot%202026-07-28%20at%2015 48 37" src="https://github.com/user-attachments/assets/2c689a93-8ba8-4208-b35e-66eb53c96671" /> and current agent-device <img width="1188" height="476" alt="Zrzut ekranu 2026-07-28 o 20 14 42" src="https://github.com/user-attachments/assets/6aed0f38-625b-4748-83e5-d384e79266c2" /> ## Test Selected fields from the generated JSONL: ```jsonl {"level":30,"type":"tool.invoked","toolId":"list-devices","toolInvocationId":"decb864f-51c1-41ee-91fc-15bc2494e268","msg":"Listing devices","time":"2026-07-29T10:37:19.896Z"} {"level":30,"type":"tool.completed","toolId":"list-devices","toolInvocationId":"decb864f-51c1-41ee-91fc-15bc2494e268","durationMs":96.77333299999918,"msg":"Listed 22 devices and 0 AVDs","time":"2026-07-29T10:37:19.993Z"} {"level":30,"type":"tool.invoked","toolId":"screenshot","toolInvocationId":"8504abb8-83ed-4490-8210-3355d6c638c9","msg":"Capturing screenshot","time":"2026-07-29T10:37:24.503Z"} {"level":50,"type":"tool.failed","toolId":"screenshot","toolInvocationId":"8504abb8-83ed-4490-8210-3355d6c638c9","failureSignal":{"error_code":"SIMULATOR_SERVER_READY_EXITED","failure_stage":"simulator_server_spawn_ready","failure_area":"tool_server","error_kind":"subprocess","failure_command":"simulator_server","failure_exit_code":1},"durationMs":750.4639999999999,"msg":"Failed to capture screenshot: SIMULATOR_SERVER_READY_EXITED","time":"2026-07-29T10:37:25.254Z"} {"level":30,"type":"tool.invoked","toolId":"screenshot","toolInvocationId":"63b938fc-3d86-4eb5-9972-a008474ffb01","msg":"Capturing screenshot","time":"2026-07-29T10:38:01.107Z"} {"level":30,"type":"tool.completed","toolId":"screenshot","toolInvocationId":"63b938fc-3d86-4eb5-9972-a008474ffb01","durationMs":25.509208000003127,"msg":"Captured screenshot argent-screenshot-chromium-cdp-9222-1785321481132-81477.png","time":"2026-07-29T10:38:01.133Z"} ``` |
||
|
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).
|