mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
feat/flow-failure-diagnostics
14 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Hubert Gancarczyk
|
4fdc749a15 |
fix(cli): keep the withheld-capture note on a snapshot failure
Both renderers suppress the `screenshot:` line when the step carries its own `current` image: on a snapshot failure the producer reuses that handle, so one picture must not be listed under two paths. The guard cannot tell a path from prose, so it swallowed the omission note as well — a snapshot step that failed after a secret was typed printed neither, in the terminal block AND in the JUnit `<failure>` body, which is the artifact CI publishes. The suppression now bounds paths only. A note beside the snapshot's own image paths would over-promise on its own, so it comes with a warning naming the roles that hold the same screen — on a diff those are annotated, with the changed pixels boxed in red. The export itself is unchanged and out of scope: `--output` copied every role of a failed snapshot on the merge base too, with no secret awareness anywhere in that path. |
||
|
Hubert Gancarczyk
|
4902981771 |
fix(flow): skip the capture wherever the screen read was already declined
`resolveScreen` short-circuits for three shapes with no screen of their own — an aborted run, a launch that never started the app, and a device-free run — and every reason it gives applies to a full-resolution capture too. `captureScreenshot` checked none of them. So a `run-cyclic` failure, a pure YAML authoring error on a flow that never touched the device, did a post-hoc tree read AND captured whatever app happened to be foregrounded, and `--output` exported the pair as this failure's evidence. Composition failures now short-circuit alongside launch-failed, and the resolution carries a `screenless` verdict the capture obeys. Declining leaves the slot empty, which the renderers were guessing at: the environmental fallback said "the device did not return an image", the opposite of what happened. The producer stamps `data.screenshotOmitted: "no-screen"` instead, and both renderers key their note off that field — an unknown reason renders no line, so a newer server's vocabulary degrades to silence. |
||
|
Hubert Gancarczyk
|
4eabc77eb9 |
fix(flow): scope the secret screenshot guard to the device, not the run
`typedSecret` lived on `ExecState`, so the guard covered exactly one `flow-execute` invocation. The screen it protects has no such scope: a credential one flow types is still rendered when the next flow runs, and a flow with no leading `launch:` runs against whatever is on screen by design — which is what a directory run does, flow after flow, on one device. So a flow containing no secret, and typing nothing, captured and exported the previous flow's credential: `--output .../step-01-screen.png` came out at full resolution with the value legible. The same gap defeated the latch through `tool: flow-execute` in both directions, since a nested run is a separate invocation with its own run state but the same device. Latch on the device instead, in a process-scoped set the runner writes and the diagnostics read. Never cleared, for the reason the run latch was never cleared — the runner cannot know when the app stops rendering the value back — which is the doctrine `createSecretScrubber` already applies to text, applied to the one projection no scrubber reaches. The renderers' note says "onto this device" rather than "by this run", since the run reading it need not be the one that typed the value. |
||
|
Hubert Gancarczyk
|
ab3be263de |
fix(flow,cli,mcp): close seven defects a second review pass found in the fixes
A mutation and correctness pass over the previous 20 commits. Each item was reproduced before being fixed. - A raw NUL byte in source. The H1 secret-scrubber dedupe key used a literal U+0000 as its separator (flow-failure.ts:550). It ran fine and passed tsc, eslint and prettier, and `grep` reported the file as binary — nothing in CI could catch it. Now an explicit `` escape. - The typed-secret screenshot guard leaked anyway on a snapshot failure. A snapshot registers `current` itself, independently of the diagnostics, so declining `failure.screenshot` removed only a pointer — and the previous commit's change to inline `current` when there is no `diff` then put those pixels straight into the agent's context, directly above the line saying a capture "could reveal it". The renderer now inlines neither image when the run typed a secret. Paths still print. - `normalizeFailure`'s root-flow guard was a tautology. It compared `failure.step.flow` against `fallback.flow`, which the callers derive from the same step — so it always held and every nested fragment's failure named the ROOT flow's file. The callers know both names and now decide. The test that "covered" this passed for the wrong reason: its fixture set the fragment name on `failure.step` but not on the step report, a shape no tool-server emits. - The emitted partial was still aliased to the running capture. A screenshot resolving after the deadline landed on the payload the runner had already emitted, making the field a timing race — and the image is of a screen that has since torn down. The timeout now copies before trimming. - The scroll reclassification contradicted its own message. `reason` is built at the call site and `failure.message` is pinned byte-identical to it, so a zero-area match shipped `selector-not-visible` beside "reached the end of the scroll" — the same defect the wait verdict was just fixed for. The reason is now rewritten with the code, and the reclassification is confined to the end-of-scroll exit: running out of ITERATIONS means the container was still producing content, where nothing can claim scrolling cannot reveal it. - Candidate rows with unusable suggestions crowded out usable ones. Splitting the suggestion into a cheap half and an expensive half made the dedupe key stop depending on whether a suggestion resolves, so five same-rectangle rows each took a slot and pushed the one that does resolve out of the list. The emit loop now collapses them, with verification bounded to a constant multiple of the output so the cost fix stands. `total` counts PROPOSED suggestions, which the doc comment now says rather than implies. - A directory run's JUnit file reported green for a red run. A flow the tool-server rejects before running it produces no report, and those were filtered out — leaving failures="0" errors="0" beside exit 1. Each now gets its own suite carrying the rejection reason. Also: the `flow-execute` docstring claimed "Each step carries `durationMs`" when skips omit it (flow-run.ts:1856) — and every step after a failure is a skip, so on the failing run that paragraph describes, most steps have none. The evidence-sweep test's 1s wait is widened; it stats a directory shared with every other run on the machine and flaked once under load. |
||
|
Hubert Gancarczyk
|
83f7cafb72 |
test(cli,mcp): drive the live-tail branch and the unpinned renderer slots
`callTool` was a `vi.fn()` that never invoked `onProgress`, so `liveSteps` was always 0 and the buffered renderer owned every assertion in the file — the branch a modern STREAMING tool-server takes was unreached. It now runs, and is identified by the header it prints (no device) rather than `renderReport`'s. Six renderer paths the PR added and left unpinned: the `invisibleMatches` → `match:` row (with visibility DERIVED from the frame, never from wire prose), the `carriesCause` branch that keeps a tree-source error visible on an environmental failure, `normalizeScreen`'s `(captured after the failure)` and `(last trusted read)` suffixes, `normalizeExpected`'s scroll arm, and the candidate row's `selectorYaml`/`note` columns. Also pinned: the pre-`determinacy` prefix inference — the ONE place a code STRING drives CLI behaviour rather than being printed verbatim, and therefore the only place a renamed `FlowFailureCode` can change what an operator reads. `twoFailureRun` now says why it builds a shape the runner cannot emit: it is the buggy-or-hostile server the renderer's bounds exist for, and a single-failure fixture could not tell a RUN image budget from a per-step one. |
||
|
Hubert Gancarczyk
|
19eea0ff9f |
fix(cli): report the flow file that ran, not one derived from its name
`flowFilePath` always built `.argent/flows/<name>.yaml` from the wire report's flow NAME, which is a guess — and a wrong one for both documented invocations that don't live there. Running `~/shared-flows/checkout.yaml` printed `@ .argent/flows/checkout.yaml` (no such file), and every nested flow of a recursive directory run emitted `argent.flowFile=".argent/flows/two.yaml"` for a flow at `.argent/flows/batch/two.yaml`. The CLI already holds the resolved path at every call site, so thread it through to the block header and the JUnit property. A step belonging to a NESTED flow keeps the derived guess: the resolved path is the root flow's file, not the fragment's, and the convention directory is the better of the two guesses there. |
||
|
Hubert Gancarczyk
|
da1006052b |
fix(flow): do not capture the failure screenshot once a run has typed a secret
`captureScreenshot` took an unconditional full-resolution capture with no
secret check. The image is copied to `step-NN-screen.png` under `--output`
and inlined as an image block by the MCP renderer, so a credential the app
rendered back into a non-password field reached the CI artifact and the
agent's context in plain pixels.
This is independent of the text scrubber: pixels are the one projection no
mask can reach, so it leaked even with the secret in `ARGENT_SECRET_*` and
every string in the report correctly masked. Verified against the exported
PNG.
The codebase already declines exactly this capture one layer up — the MCP
auto-screenshot skips whenever a tool input contains `{{secret:`. Apply the
same rule, spelled the same way: the runner latches a flag when a step
carrying a placeholder is about to run (latched, never cleared — the runner
cannot know when the app stops rendering the value back), and the assembler
declines both the fresh capture and a snapshot's reused `current` handle.
The omission is stated rather than silent. `data.screenshotOmitted` drives a
line in both renderers, because an agent that merely sees no image calls
`screenshot` itself — which is the leak the omission exists to prevent.
|
||
|
Hubert Gancarczyk
|
838cbc5706 |
feat(cli): failure block, step timings and a JUnit reporter for argent flow
The terminal block is a slot system, not a template: code + message is mandatory and the rest (context window, expected/actual, candidates, screen, reads, artifact paths) appears only when it applies, which is what lets one renderer serve six very different failures. Step lines gain a duration only when the server sent one, so a pre-timing tool-server renders byte-identically. --reporter junit:<path> maps each STEP to a testcase, because per-failure attribution in the checks UI is the entire value of JUnit in CI. A report-file write failure warns and leaves the verdict alone - failing a build on argent's own I/O would make the reporter a source of flake. Everything the renderers read is clamped first: the failure object can arrive from a remote argent link server. |
||
|
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> |
||
|
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
|
a0d1bbb1e7 |
feat(flow): indent nested steps in reports by block depth (#524)
Flow reports rendered every step flat, hiding which steps ran inside a `when:` block or a `run:` fragment. The tool-server now stamps `StepReport.depth` (+1 per block directive, omitted at top level) and the CLI/MCP renderers indent step labels and under-step lines by it: ``` ✓ 1 launch ✓ 2 when visible "What's new" — condition met ✓ 3 tap "Skip" ✓ 4 run dismiss [dismiss] ✓ 5 tap "OK" [dismiss] ``` Invariants: - **Wire-compatible both ways** — a flow with no block directives reports byte-identical to the pre-depth shape; a pre-depth server renders flat as before. - **Skipped ≡ entered** — skipped/errored/aborted blocks report the same depths as entered ones, keeping reports comparable run-to-run. - **Untrusted wire data** — renderers clamp depth (display cap, not a producer bound): hostile values render flat or at the cap, never throw or allocate huge strings. Also fixes a pre-existing bug: under-step warning/artifact lines now stay aligned past 99 steps. Tests pin depth on every producer path (entered/skipped/errored/aborted blocks, `run:` success/failure markers, `run:`-in-`when:`) and renderer alignment incl. the clamp boundary. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Jakub Piasecki
|
3f602a7a9b |
feat(flow): conditional when blocks (#498)
## What
A `when:` block runs a step list only when one condition holds — a UI
condition (`exists`/`visible`/`hidden`/`text`, the await/assert shapes)
or a static `platform` test. **No else**, by design: a block exists to
restore determinism (dismiss an interstitial, reconverge), never to
test two paths. Tap-if-present is a one-step block:
`when: { visible: "Got it" }` + `steps: [tap: "Got it"]`.
No per-step `optional:` — `when:` covers every directive once. The key
is rejected at parse with a pointer to `when:`, not silently dropped (a
Maestro habit that would leave a "can't fail" step hard-stopping the
flow).
## How
- The guard reuses the assert engine at the 1s grace (`timeout` is a
parse error), so a skipped block barely costs a clean run.
- Verdicts are evidence-based: no trustworthy read in the window →
indeterminate → the step **errors** ("could not evaluate" is not
"condition false"); a transient read failure after trusted reads showed
the condition false stays a clean skip.
- `platform` folds `ios-remote` into `ios` — the parser rejects that
spelling, so the fold is the only way a guard matches a remote sim.
- A skipped block reports one line per authored step, same shape for
unmet guard, hard stop, or cancellation; everything goes through
`pushReport`, so when lines stream live. Failures inside an entered
block hard-stop as usual.
- Results carry an explicit `aborted` flag; `ok` keys off
fail/error/cancel, not skip lines.
- Nesting is capped at 20, so a cyclic YAML alias is a structured parse
error, not a stack overflow.
## Testing
`vitest` 2546 pass (17 when-specific); `tsc --noEmit` (tool-server,
argent-mcp, argent-cli), eslint, prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Jakub Piasecki
|
551c781fb0 |
feat(flow): support running end-to-end flows without agent (#429)
# Flow E2E
This PR turns flows from replayable tool-call recordings into a
declarative, portable e2e test system that runs headlessly via `argent
flow run <name>` — no LLM in the loop, non-zero exit on failure,
suitable for CI.
Agents can still replay flows via `flow-execute` and get the full output
from each step after it finishes.
## Flow format
Flows are YAML files in `.argent/flows/`, now built from **directives**
interpreted by the runner rather than raw tool calls:
- `launch` — terminate + relaunch the app under test (per-platform app
id map supported); its presence marks a flow as **e2e** (controls its
own start state) vs. a **fragment** (runs against current state,
declares an `executionPrerequisite`, composable via `run:` with
cycle/depth guards).
- `tap`, `type` (focus handshake + optional submit), `scroll-to`
(momentum-free, increments by half the container, `within:` for nested
scrollers), `await`/`assert` (`visible`/`exists`/`hidden`/`text`
conditions), `wait`, `snapshot`, `echo`, and raw `tool:` as escape hatch
— still fully supported, so previously recorded flows keep working
unchanged.
- **Selectors** are `{ text, identifier, role }`; a bare string is a
loose selector resolving identifier-first, then visible text. Any
directive failure hard-stops the flow; later steps report `skip`.
## Execution engine
- Flows store no device id — the runner binds a device at replay
(explicit `--device`/`--platform` or the single booted one).
- Selector resolution runs against **platform-unified UI trees**: iOS
native view hierarchy (full testID coverage, gated on native-devtools
connecting), Android full accessibility hierarchy (full resource-id
coverage), Chromium CDP DOM, and the Vega automation toolkit. A shared
flatten/text-hoisting pass lets `text` assertions read what an
identified container visibly shows. Vega is remote-driven, so touch
directives fail with guidance to use `tv-remote`/`keyboard`.
- Steps wait for the tree to **settle** (two consecutive identical
reads) before acting; `launch` additionally gates on platform
tree-source readiness instead of silently degrading.
## Visual snapshots
`snapshot` diffs a full-resolution screenshot against baselines in
`.argent/flows/__baselines__/<flow>/`, keyed by platform + resolution,
with configurable `maxMismatch` and `--update-baselines`. The status bar
is pinned for the whole run (iOS `simctl status_bar`, Android demo mode)
so clocks/battery don't drive diffs. Baseline/current/diff images are
returned as artifacts on failure (and the written baseline on first
capture/update); a clean pass carries none, keeping reports and agent
context lean.
## Recording
The record-live workflow (`flow-start-recording` → `flow-add-step` →
`flow-finish-recording`) now emits directives automatically: coordinate
taps are rewritten to portable selector `tap:` steps (selector captured
from the pre-tap tree), a leading `restart-app` becomes `launch:`,
`flow-execute` of a sibling fragment becomes `run:`, and device ids are
stripped. The skill instructs the agent to polish the saved YAML
afterwards (fold tap+keyboard into `type:`, swipes-to-reach into
`scroll-to:`, awaits into sugar form).
## Also included
- `argent flow run` / `argent flow list` CLI with per-step report
rendering, artifact materialization, and `--json` output.
- New structured failure codes for flow validation, device resolution,
composition, and devtools readiness.
- Extensive unit test coverage (tree adapters, selector matching,
scrolling, composition, type focus, snapshots, status bar).
|