feat(flow): support running end-to-end flows without agent#429
feat(flow): support running end-to-end flows without agent#429j-piasecki wants to merge 80 commits into
Conversation
4ce868a to
b71479a
Compare
0994fdc to
885eaf5
Compare
latekvo
left a comment
There was a problem hiding this comment.
Reviewed the flow end-to-end system — the runtime engine, the per-platform tree adapters, YAML parse/serialize, snapshot/visual diffing, the recording→directive rewrites, and the argent flow run CLI — and ran the CLI end-to-end against a booted Chromium/Electron app (happy path, a failing flow, the snapshot baseline/diff path, and the --device path). Specific observations are left inline.
| return { ok: false, reason: `no chromium app declared — add a chromium launch entry` }; | ||
| } | ||
| try { | ||
| await invokeOnDevice(state, "launch-app", { bundleId: appPath }); |
There was a problem hiding this comment.
When a Chromium e2e flow does not boot its own app — the documented --device opt-out that pins an already-running instance (resolveRunDevice skips chromiumBootSpec when params.device is set), or a multi-platform launch map like { ios: …, chromium: … } with no --platform that auto-resolves to a single booted chromium device — this branch passes the flow's chromium app path to launch-app as bundleId. launch-app validates bundleId against ^[A-Za-z_][A-Za-z0-9._-]*$, and the registry validates tool inputs before execute, so any real path (leading / or ., or an embedded /) is rejected: the launch step is reported as error, state.stopped is set, and every later step is skipped. I reproduced this end-to-end against a real Electron instance — a chromium flow run with --device chromium-cdp-<port> fails on step 1 with Invalid params for tool "launch-app": … bundleId may only contain letters, digits, '.', '_' and '-'. The in-place launch-app fallback the surrounding doc-comment describes never succeeds for a path-based chromium launch.
There was a problem hiding this comment.
Fixed in 5bc92d4 — the chromium launch path no longer routes through launch-app; it attaches to the pinned running instance in place (resolveService + refreshViewport) and reports a clear error when the instance is unreachable.
| expect(bootElectronApp.mock.calls[0][0]).toMatchObject({ appPath: "/abs/app" }); | ||
| }); | ||
|
|
||
| it("does not boot or tear down when an explicit --device pins an existing instance", async () => { |
There was a problem hiding this comment.
This test drives the --device opt-out path (launch: { chromium: ./app }, device: chromium-cdp-9999) and asserts ok === true. It passes only because the stub registry's invokeTool (vi.fn(async () => ({}))) skips the input-schema validation the real Registry runs before execute; against the real registry that same call is rejected by launch-app's bundleId pattern, so the launch step errors. The test also asserts that launch-app was invoked but never checks the bundleId value it was invoked with, so a filesystem path passed as a bundleId goes unnoticed here.
There was a problem hiding this comment.
Fixed in 5bc92d4 — the test now asserts launch-app is NOT invoked for chromium and refreshViewport is, plus a new test that an unreachable pinned instance errors the launch step and skips the rest.
| ); | ||
| } | ||
| const step: FlowStep = { kind: "snapshot", name: b.name }; | ||
| if (b.maxMismatch !== undefined) step.maxMismatch = Number(b.maxMismatch); |
There was a problem hiding this comment.
maxMismatch is coerced with Number(...) and stored without validation. A non-numeric value such as maxMismatch: "5%" becomes NaN; in runSnapshot, mismatchPercentage <= (maxMismatch ?? DEFAULT) is then a comparison against NaN (nullish-coalescing does not catch NaN), which is always false, so the snapshot always fails — even when the frames are byte-identical. I reproduced it end-to-end: an unchanged screen (0.00% diff) reports diff 0.00% > NaN% and the run exits 1. A negative value fails identically, and null/""/false silently coerce to 0 (exact-match only). The sibling wait directive guards its Number(...) with a finite/non-negative check; snapshot.maxMismatch has none.
There was a problem hiding this comment.
Fixed in 94b1ef1 — maxMismatch is now validated (finite number, 0–100) and rejects at parse with a clear error; tests cover "5%", negatives, >100, and .nan.
| ): Promise<{ selector?: Selector; warning?: string }> { | ||
| try { | ||
| const device = resolveDevice(udid); | ||
| const { tree } = await fetchTree(registry, device); |
There was a problem hiding this comment.
Selector capture reads the tree via fetchTree (the trimmed AX / uiautomator describe tree), but the runner resolves the recorded selector at replay via fetchFlowTree (the full native / accessibility hierarchy) — two different tree sources. On iOS an accessible container collapses in the AX tree to a single node whose merged label (e.g. "Add, to cart") is what deriveSelector records, but in the full UIView hierarchy that text is split across child leaves and no single node's label contains the merged string, so the recorded tap: { text: … } resolves to nothing at replay. On Android the AX trim drops the testID container the full hierarchy keeps, so a weaker selector is captured than the replay tree could match. The step is recorded as a portable selector with no warning, and reported as a success, yet can fail (or hit a different element) on replay — and fetchFlowTree itself already degrades to fetchTree when the full hierarchy is unavailable, so record and replay reading two different tree sources is not forced by any constraint.
There was a problem hiding this comment.
Fixed in 1406671 — the recorder now captures selectors from the same full tree replay resolves against, and the derived selector is re-resolved via selectorToFrame with the winning frame required to contain the tapped point (else coordinates are kept).
| const currentPath = shot.image.hostPath; | ||
|
|
||
| const { w, h } = await pngDimensions(currentPath); | ||
| const key = `${opts.name}__${env.device.platform}-${w}x${h}.png`; |
There was a problem hiding this comment.
The baseline is keyed by the current capture's resolution (${name}__${platform}-${w}x${h}.png). On a first run, or any run whose device resolution differs from the committed baselines (a different simulator model, an auto-detected device, a rotation), no file matches the key, so a fresh baseline is written and the step returns pass — with a warning in the default mode, silently under --update-baselines. The warning does not feed report.ok, so the run exits 0 while comparing nothing. Since argent flow run's exit code is the headless/CI signal, a visual-regression gate goes green having asserted nothing whenever the run device class diverges from the baselines'. I observed this: a first run reports baseline created … nothing was compared, PASS, exit 0.
There was a problem hiding this comment.
Fixed in 343251a — a missing baseline now fails the comparison ("nothing was compared") instead of silently seeding and passing; baselines are only created via explicit --update-baselines.
| FLOW_FILE_INVALID: "FLOW_FILE_INVALID", | ||
| FLOW_ENTRY_UNRECOGNIZED: "FLOW_ENTRY_UNRECOGNIZED", | ||
| FLOW_E2E_HAS_PREREQUISITE: "FLOW_E2E_HAS_PREREQUISITE", | ||
| FLOW_APP_ID_MISSING: "FLOW_APP_ID_MISSING", |
There was a problem hiding this comment.
Six of the eight new flow failure codes are never referenced anywhere in the source: FLOW_APP_ID_MISSING, FLOW_NOT_E2E, FLOW_RUN_TARGET_MISSING, FLOW_RUN_TARGET_NOT_FRAGMENT, FLOW_RUN_CYCLE, and FLOW_NATIVE_DEVTOOLS_UNAVAILABLE. The corresponding conditions (no app id, run target is e2e / missing, cyclic run:, native-devtools unavailable) are all reported as plain step-report reason strings without a structured error code, so these six constants are dead as added.
There was a problem hiding this comment.
Fixed in 579223b — all six deleted rather than wired: each condition surfaces as a step-report reason string by design and never crosses the FailureError tool boundary (FLOW_NOT_E2E had no matching condition at all). Only the two genuinely wired codes remain.
| }; | ||
| } | ||
|
|
||
| const outputDir = await fs.mkdtemp(path.join(os.tmpdir(), "argent-flow-diff-")); |
There was a problem hiding this comment.
The mkdtemp diff directory is never removed. On the within-tolerance PASS path runSnapshot returns without registering or deleting anything, so the directory and its two PNGs are pure garbage; on FAIL only contextDiffPath is registered while the directory and the full-res diffPath still linger. A long-lived shared tool-server running snapshot flows repeatedly accumulates argent-flow-diff-* directories in the temp dir with no cleanup.
There was a problem hiding this comment.
Fixed in c92b8cd — the diff dir is cleaned on every exit path (try/finally): removed entirely on pass and on the dimension-mismatch bail, and on fail only the registered context-diff artifact is kept (its path backs artifact serving); the unregistered full-res diff is deleted. Cleanup is best-effort so it never fails the snapshot.
| // applied (e.g. Android demo mode entered but a later broadcast failed). | ||
| // The caller sees `false` and never restores, so undo here; both cleanup | ||
| // commands are harmless no-ops when nothing was applied. | ||
| await restoreStatusBar(device); |
There was a problem hiding this comment.
On Android, if a demo-mode broadcast fails after enter (#2) has already succeeded, this catch calls restoreStatusBar (demo exit); if that exit also throws (transient adb), it is swallowed, pinStatusBar returns false, and the caller's state.pinned is false — so the run-end restore never fires and demo mode (frozen clock/battery) stays applied on the device after the run. sysui_demo_allowed is also never reset to 0.
There was a problem hiding this comment.
Fixed in 5539526 — restoreStatusBar now resets sysui_demo_allowed to 0 in a finally and never throws; when the in-catch undo itself fails, pinStatusBar returns true on Android so the run-end restore fires and retries instead of stranding demo mode.
| const blocks = asLines(entries); | ||
|
|
||
| const failed = status === "fail" || status === "error"; | ||
| if (failed && typeof resolved.diff === "string") { |
There was a problem hiding this comment.
On a failed snapshot, materializeArtifacts downloads every image handle (baseline, current, diff), but only diff is inlined here; baseline and current (full-resolution PNGs) are fetched solely so their local paths can be printed as text. Against a remote tool-server that pulls two extra full-res PNGs over the wire per failed snapshot, and reads them fully into memory when co-located — which is the cost flow-visual.ts's own comment says the design avoids by omitting artifacts on a clean pass.
There was a problem hiding this comment.
Fixed in 0684ff9 — only the diff handle is materialized, and only on failure; baseline/current render as path references with zero fetches and zero full-file reads.
| * nothing, and the scroll itself will surface a real transport error. | ||
| */ | ||
| async function assertWindowVisible(chromium: ChromiumCdpApi): Promise<void> { | ||
| const raw = (await chromium.cdp.send("Runtime.evaluate", { |
There was a problem hiding this comment.
The doc-comment states "a failed or empty read proves nothing, and the scroll itself will surface a real transport error," but this probe is not wrapped in try/catch — only the returned value is guarded. A rejected Runtime.evaluate (most plausibly during a navigation, when the JS execution context is being destroyed) propagates out and fails the scroll before any wheel event is dispatched, even though Input.dispatchMouseEvent needs no JS execution context and would have worked. So the "failed read proves nothing" intent is not implemented, and a scroll issued right after a navigation can spuriously error.
There was a problem hiding this comment.
Fixed in 54098aa — the probe is wrapped in try/catch so a rejected Runtime.evaluate is treated as inconclusive and the scroll proceeds; only an explicit hidden result refuses.
latekvo
left a comment
There was a problem hiding this comment.
A few observations from reviewing the flow-e2e changes — details inline.
| ); | ||
| } | ||
| const step: FlowStep = { kind: "snapshot", name: b.name }; | ||
| if (b.maxMismatch !== undefined) step.maxMismatch = Number(b.maxMismatch); |
There was a problem hiding this comment.
maxMismatch is coerced with a bare Number() and never validated here, so a non-numeric or malformed value (e.g. maxMismatch: 2%, which is easy to write since the reason string prints ≤ 0.5%, or maxMismatch: loose) silently becomes NaN. The ?? DEFAULT_MAX_MISMATCH in flow-run.ts only substitutes for null/undefined, so NaN flows through, and mismatchPercentage <= NaN in flow-visual.ts is always false — so every snapshot comparison then fails, even on a pixel-identical screen, with the self-contradictory reason diff 0.00% > NaN%. The first run still passes (the baseline-write branch ignores maxMismatch), so it only breaks from the second run on. The sibling wait and await.timeout fields are finiteness/range-checked at parse; this one is not. Reproduced end-to-end: with maxMismatch: loose and an existing baseline, an identical capture reports fail — diff 0.00% > NaN%.
There was a problem hiding this comment.
Fixed in 94b1ef1 (see the sibling thread) — maxMismatch is validated as a finite 0–100 number at parse.
| if (frame && axisFullyInside(frame, direction, region)) return { frame }; | ||
|
|
||
| const fp = treeFingerprint(tree); | ||
| if (prevFp !== undefined && fp === prevFp) { |
There was a problem hiding this comment.
scrollToVisible detects the end of a scroll only when two consecutive settled trees share an identical treeFingerprint, and that fingerprint includes every node's text. Any continuously-animating node anywhere on screen (an infinite-scroll spinner, a live timestamp, a ticking counter) changes the fingerprint on every read, so the end is never detected. The last item of a container can only be accepted through this end-of-scroll fallback — it sits flush against the entry edge, so axisFullyInside returns false for it by construction — so a scroll-to targeting it fails after MAX_SCROLL_ITERATIONS with … not found, even though it was fully present and reachable the whole time, and it burns ~3s (settle timeout) per iteration first. Reproduced: a page with one 150ms ticker plus a 60-row scroller, scroll-to: { target: item-59, within: list } returns item-59 not found after 25 scroll attempts after ~88s; the same page without the ticker resolves it immediately.
There was a problem hiding this comment.
Fixed in b4fd37c — the end-of-scroll fingerprint is now scoped to nodes overlapping the scroll region, so a ticker/spinner outside the container can't mask end-of-scroll (your repro passes). Text deliberately stays in the fingerprint for in-region nodes — a snapping list recycles identical frames with new content, so a structure-only fingerprint would misread real progress as a stuck scroll; an animating node inside the region remains a documented limitation.
| flow-add-echo { message: "Start Settings from scratch" } | ||
| flow-add-step { command: "restart-app", args: "{\"udid\": \"ABC\", \"bundleId\": \"com.apple.Preferences\"}" } # ⇒ captured as `- launch: com.apple.Preferences` — this is now an e2e flow | ||
| flow-add-echo { message: "On the Settings root list, tapping the 'General' row" } | ||
| flow-add-step { command: "gesture-tap", args: "{\"udid\": \"ABC\", \"x\": 0.5, \"y\": 0.35}" } # ⇒ captured as `- tap: General` (portable selector, no udid) |
There was a problem hiding this comment.
This documents a recorded coordinate gesture-tap as being captured into a bare-string selector step (⇒ captured as - tap: General; see also line 125 “text-only selectors are emitted as bare strings” and the polished examples at 164/167). But the recorder's deriveSelector returns a plain { text } with no loose flag, and selectorToYaml only collapses to a bare string when loose is set (a unit test pins that a strict { text } is never collapsed). So a recorded text tap is actually written as - tap: { text: General }. That is not just a spelling difference: per line 37 a bare string is a loose (identifier-first) selector, whereas { text: General } is strict text-only — so the doc misstates both the file the recording agent is told to read back and the resolution semantics of recorded taps.
There was a problem hiding this comment.
Fixed in f480ea8 — the doc now matches the code: recorded taps are documented as - tap: { text: General } in all three spots, and the bare-string claim is corrected. Strict is deliberate (the recorder knows the tap hit text), so the code was left unchanged.
| ): Promise<{ selector?: Selector; warning?: string }> { | ||
| try { | ||
| const device = resolveDevice(udid); | ||
| const { tree } = await fetchTree(registry, device); |
There was a problem hiding this comment.
Selector capture during recording reads the tree via fetchTree (the trimmed AX/uiautomator/cdp-dom tree), but replay resolves the captured selector against fetchFlowTree (the full native/accessibility hierarchy). For a testID-bearing element the two agree, but for an element with no stable id whose AX label is a VoiceOver-aggregated compound string, deriveSelector captures { text: <compound> } from the collapsed AX node, and no single node in the full hierarchy carries that whole compound label — so matchNode's substring test finds nothing and the recorded tap fails at replay even though the record-time tap succeeded. Nothing verifies the captured selector resolves against the tree the runner will actually use.
There was a problem hiding this comment.
Fixed in 1406671 — the captured selector is now verified by re-resolving it against the replay tree source; if the resolved frame doesn't contain the tapped point, the recorder keeps coordinates instead.
| ): Promise<DescribeFrame | undefined> { | ||
| const deadline = Date.now() + DEFAULT_ACTION_TIMEOUT_MS; | ||
| for (;;) { | ||
| if (env.signal?.aborted) return undefined; |
There was a problem hiding this comment.
waitForFrame returns undefined for both an elapsed deadline and an aborted run (the env.signal?.aborted check here and the one via sleepOrAbort), so runTap/runType can't tell the two apart and, on abort, return { ok:false, reason: offscreenHint(...) }. execLeafStep maps that to status:"fail", marking the run ok:false. So cancelling a long flow while a tap: Checkout step is auto-waiting reports it as a genuine failure with the reason “no visible element matched … add a scroll-to step before this one” — for an element that was present. Every other cancellation path reports differently: wait and the pre-step guard report skip + “run aborted”, and waitForCondition/scrollToVisible say “cancelled”. Only tap/type both misclassify (fail vs skip) and emit a wrong reason.
There was a problem hiding this comment.
Fixed in 58b0363 — waitForFrame now distinguishes abort from timeout, and all cancellation paths (tap, type, await/assert, scroll-to) flow through a shared aborted outcome that execLeafStep maps to skip + "run aborted", so a cancelled tap no longer reports a false element-not-found failure.
| const tok = argv[i]!; | ||
| if (tok === "--update-baselines") out.updateBaselines = true; | ||
| else if (tok === "--json") out.json = true; | ||
| else if (tok === "--device") out.device = argv[++i]; |
There was a problem hiding this comment.
When --device (or --platform, next line) is the final token with no value, argv[++i] is undefined and the flag is silently dropped from the payload, so the run falls back to auto-detection instead of reporting the missing argument. A truncated or mistyped argent flow run x --device then runs against whatever single device happens to be booted (or errors as “N booted devices matched”) rather than the device the operator meant. This whole file (parseRunArgs, the divergent renderReport numbering, the exit codes, and the artifact-materialization seam) has no test coverage, unlike the sibling run.ts, so gaps like this slip through.
There was a problem hiding this comment.
Fixed in 6e6b878 — --device/--platform with a missing value (or a flag where the value should be, e.g. --device --json) now errors with exit 2 and help instead of silently auto-detecting. Also added test/flow.test.ts mirroring the sibling harnesses: parser paths, payload forwarding, renderReport (numbering, fragment tags, warnings), --json, and exit codes.
|
|
||
| let timeout: number | undefined; | ||
| if (kind === "await" && "timeout" in b) { | ||
| if (typeof b.timeout !== "number" || b.timeout <= 0) { |
There was a problem hiding this comment.
await.timeout is validated only as typeof number and > 0, not finite. A YAML .inf (or a value like 1e400) parses to Infinity and passes, so waitForCondition computes deadline = Date.now() + Infinity = Infinity and the Date.now() >= deadline guard is never true — the poll loop then runs forever (until the condition happens to hold or the run is aborted), an unbounded hang on a longRunning tool. The sibling wait step performs the Number.isFinite check this one omits.
There was a problem hiding this comment.
Fixed in 8abaa78 — await.timeout now requires Number.isFinite, matching the sibling wait step; .inf, .nan, and 1e400 are rejected at parse.
| } else if (device.platform === "chromium") { | ||
| return queryChromiumTree(registry, device); | ||
| } | ||
| return fetchTree(registry, device); |
There was a problem hiding this comment.
subtreeText (the hoisted descendant text that assert/text reads via assertText) is produced only by the dedicated flow trees built above for iOS/Android/chromium. Vega falls through to fetchTree here, which never sets subtreeText — and so does iOS/Android when the devtools helper is unavailable and the run degrades to the trimmed AX/uiautomator tree. On those paths a text assert against a testID container that visibly wraps its text reads the container's own (empty) text and reports a false “text not found”, so the flagship assert behaves inconsistently per platform. No test covers the Vega / AX-fallback text-assert case.
There was a problem hiding this comment.
iOS/Android fixed in 057a6e5 (fallback removed, outages fail loudly). The Vega half is fixed in fdc4c89 — Vega now runs the shared flatten/hoisting so text asserts against wrapping testID containers see subtreeText, with the missing Vega test coverage added (only authored testIDs shield, since the toolkit auto-stamps numeric ids on every node).
| node.identifier || node.label || node.value || node.clickable || node.focused | ||
| ); | ||
|
|
||
| const leaf: DescribeNode | null = onScreen && addressable ? { ...node, children: [] } : null; |
There was a problem hiding this comment.
The password handling here is incomplete. ownText is forced to "" and shield is set for a password node, and the comment states “the walker already withholds its value” — but that only stops the secret bubbling into an ancestor's subtreeText. The leaf itself is emitted as { ...node }, retaining node.label, and describeChromium's accessibleName falls through to el.value for an <input> with no aria-label/aria-labelledby/placeholder/title (no password guard there). So a placeholder-less <input type="password"> (a common floating/uncontrolled-label pattern) with a typed value has its plaintext stored in node.label; assertText(leaf) returns it (no subtreeText → nodeText → label), and a failing text assert echoes it verbatim in assertReason (… but its text was "<plaintext>"), plus any consumer that renders the leaf label. The Android and iOS adapters redact this; the chromium one doesn't.
There was a problem hiding this comment.
Fixed in 5e16190 — accessibleName no longer falls through to el.value for password inputs (guarded at the source, protecting every describe consumer), and the chromium flow leaf additionally redacts a password node's label to [password], mirroring the Android adapter.
| if (!invisibleSelf) { | ||
| const doc = getOwnerDocument.call(el); | ||
| if (doc && getActiveElement.call(doc) === el && getDocBody.call(doc) !== el) { | ||
| node.focused = true; |
There was a problem hiding this comment.
This sets node.focused on the active element for every describeChromium call, including the agent-facing describe tool (which calls describeChromium(services.chromium) with default limits). Chromium describe never emitted focused before this PR, so agent-facing output now gains a [focused] flag on any page with an active field — a behavior change beyond the flow path. It also double-reports for a focused input inside a same-origin iframe: both the input (inner document's activeElement) and its host <iframe> element (outer document's activeElement) are flagged.
There was a problem hiding this comment.
Fixed in e1f98d2 — host iframe/frame elements are no longer flagged, so only the inner document's activeElement carries focused (test pins exactly one flag). Keeping the flag in agent-facing describe output is deliberate — the comment now states it's exposed to all describe consumers.
… flows without agent
Turns flows from replayable tool-call recordings into a portable e2e test system:
- Declarative YAML directives: launch, tap, type, scroll-to, await, assert, snapshot
- Selectors: { text, identifier, role }
- Headless execution via 'argent flow run <name>' — no LLM in the loop
- Non-zero exit on failure — CI-ready
- Fragments with executionPrerequisite for composability
Original PR: software-mansion#429
…tics - flow-add-step: missed find / unmet await-ui-element are now recorded as normal steps instead of throwing (PR software-mansion#429 behavior change) - flow-execute: add list-devices mock (needed for device auto-resolution) - flow-execute: bump invokeTool call count (+1 for list-devices) - flow-execute: found:false no longer halts flow (normal pass result)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Of the flow codes added on this branch only FLOW_E2E_HAS_PREREQUISITE and FLOW_DEVICE_RESOLUTION cross the tool boundary as FailureErrors. The other six named conditions that surface as step-report reason strings by design (the cycle guard, the e2e-as-fragment rejection, the missing-app-id launch failure, the fragment-load failure, and the devtools-unavailable tree reads) — or, for FLOW_NOT_E2E, no condition at all — so they could never be attached to telemetry. Delete them rather than invent new plumbing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The recorder's deriveSelector returns a plain { text } with no loose flag,
and selectorToYaml only collapses to a bare string when loose is set, so a
recorded text tap is written as `- tap: { text: General }` — strict
text-only, not the loose (identifier-first) bare-string spelling the skill
showed. Fix the captured-as example, the polished flow file, and the prose
claiming text-only selectors are emitted as bare strings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A trailing `--device`/`--platform` (or one followed by another flag) was silently dropped from the payload — argv[++i] came back undefined — so a truncated command ran against whatever device auto-detection picked instead of reporting the missing argument. Consume values through a takeValue helper (the parseLinkFlags/parseStartFlags pattern) that throws FlagParseException, and exit 2 with the message + help like `argent run` does on flag-parse failures. Add flow.test.ts covering parseRunArgs, report rendering, and the exit codes, mirroring the run.ts test harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Under the parallel suite load, unit tests that incidentally reached real `xcrun simctl` / adb (status-bar pinning on every flow run, the isTvOsSimulator / isAndroidTv form-factor probes on fake device ids, and the native-devtools factory's ensureEnv) tripped a rotating set of 5s/10s per-test timeouts. Mock status-bar suite-wide via a vitest setup file (status-bar.test.ts opts back in with vi.unmock) and pin the remaining probes at their module seams in the affected files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Stream flow progress to the CLI
`argent flow run` used to block until the whole flow finished and then dump
the full report. Long flows (a launch step alone takes seconds, an auto-wait
up to 7.5s per step) gave no feedback until the very end. Each step line now
prints the moment the step completes; the final summary is unchanged.
## How it works
- **Runner** (`flow-run.ts`): every `StepReport` is appended through a single
`pushReport` choke point that also fires `ctx.emitProgress`. Nested `run:`
fragments share the same report stream, so their steps (and the skip-flood
after a failure) stream too. The aggregated `FlowRunResult` is untouched.
- **Registry** (`InvokeToolOptions.emitProgress`): a generic, optional
progress hook — not flow-specific, so other long-running tools (e.g.
`run-sequence`) and the MCP path can adopt it later without transport
changes. `invokeSubTool` does not forward it, so sub-tools can't interleave
into an outer stream.
- **HTTP** (`http.ts`): when a request sends `Accept: application/x-ndjson`,
the tool endpoint streams one `{event: "progress"}` line per emit, then a
terminal `{event: "result"}` line — or an in-band `{event: "error"}` line,
since the 200 is already committed once streaming starts. All pre-invoke
gates (404/400/422/424) still answer with their plain-JSON status codes.
- **Client** (`callTool(name, args, { onProgress })`): opts into NDJSON via
the Accept header and treats the response Content-Type as the mode signal.
- **CLI** (`flow.ts`): renders each step line live; snapshot artifact paths
print as a labeled block before the summary (they only exist after
materialization of the final report). `--json` output is byte-identical to
before.
## Compatibility
Version skew is safe in both directions: an old server ignores the Accept
header and replies plain JSON (the client falls back to the buffered path and
the CLI renders the report as before); an old client never sends the header
and gets today's behavior. Ctrl-C still aborts the run via the existing
socket-close → AbortSignal path.
## Testing
18 new tests: runner emission (ordering, fragments, skip-flood, no-consumer),
HTTP framing (NDJSON lines, in-band error, no-Accept and pre-invoke failures
stay JSON), client streaming against a real local server (happy path, header
gating, old-server fallback, error, truncated stream), and CLI rendering
(buffered output locked to its historical shape, live lines match it
verbatim). Full suites for the touched packages pass.
# Label flow report steps with their target; stop counting echo as a step A flow report line used to show only the step kind, which makes a run full of selector steps unreadable — thirty lines of bare `tap` / `await` / `assert` with nothing saying what they acted on. The summary also counted `echo` narration as passed steps, so a flow whose last step is numbered 33 reported "39 passed". Before / after: ``` ✓ 4 tap ✓ 4 tap "Nested touchables" ✓ 5 await ✓ 5 await visible "Nested gestures & touchables" ✓ 10 assert ✓ 10 assert hidden "] outer Touchable" ``` ## Step targets `StepReport` gains a display-only `target` string, derived from the step definition in the runner (`stepTarget()` in `flow-run.ts`): - `tap` / `scroll-to`: the selector (`"text"`, `id=…`, `role=…`); coordinate taps show `(x, y)`; `scroll-to` appends a non-default direction (`(up)`). - `await` / `assert`: condition-prefixed (`hidden "]"`); text conditions show the comparator (`id=total contains "Total"`). - `type`: the field it types into (`into id=email`) — not the typed text. - `snapshot`: its name (`"home"`). Skipped steps carry it too, so the skip-flood after a failure shows what didn't run. Both renderers pick it up — the CLI's step line and the MCP content blocks — falling back to today's bare kind when absent. The field is derived and display-only; nothing parses it back. ## Echo counter `summarize()` now skips `echo` steps (pass or skip), so the counters agree with the renderers' step numbering, which never numbered echo lines. ## Compatibility `target` is a new optional field on the wire and in `--json` output; old clients ignore it, and a report from an old server renders as before. ## Testing New coverage: target derivation across every step shape (selector, coordinate, text-condition, direction, type, snapshot, skipped), and counter assertions for echo exclusion on passing and failing runs. CLI render goldens updated to the new counting. Full suites for tool-server flows, argent-cli, and argent-mcp pass.
Snapshot diffs are written to a `mkdtemp` dir under `os.tmpdir()` with no
cleanup: they leak locally and vanish with the runner in CI. The path is
printed and the failed diff renders inline, so the live/interactive agent is
already served — the gap is durable retrieval. A CI `upload-artifacts` step
can't glob a random `argent-flow-diff-<hex>` path, and reopening yesterday's
failing diff means scraping a tmp path out of a transcript and hoping it
wasn't reaped.
`argent flow run <name> --output <dir>` copies each **failed** snapshot's
images — all three roles, side by side for triage — to a stable, globbable
path:
```
<dir>/<flow>/<name>__<platform>-WxH-{baseline,current,diff}.png
```
The report's artifact paths are rewritten to the durable locations before
rendering, so every output mode (live, buffered, `--json`) prints the path CI
will actually upload. Without the flag, behaviour is unchanged (tmp dir).
Failure-only by design: a clean pass carries no artifacts, and a seeded
baseline is already durable under `__baselines__/`. Export is best-effort per
file — a copy error warns on stderr and keeps the temp path; it never changes
the run's verdict or exit code.
The layout reuses the existing baseline key, so a run that hits several
flows/snapshots can't clobber itself, and the directory is intended as the
shared destination for the rest of the failure bundle (crash screenshot,
flow-tree dump, device logs, JUnit XML) when those land.
- **Client-side copy from materialized artifacts, not runner-side `outputDir`
plumbing.** A runner-side output dir would write to the tool-server host —
the wrong machine whenever the server is remote (`argent link`) — and would
grow the `flow-execute` schema, which the interactive MCP path doesn't need
(inline diff + printed path already serve a live agent). The CLI copies the
files the artifact transport has already materialized locally, so remote
servers work for free and the MCP surface is untouched.
- **`snapshotKey` on snapshot step reports** is the only runner change:
artifact-bearing snapshot reports now carry the baseline key stem
(`name__platform-WxH`) so the exporter can name files by the same
collision-free key the baseline store uses. Version skew degrades
gracefully: against an older server without the field, the CLI derives the
key from the baseline artifact's filename.
- `--output` follows the Playwright model ("folder for run artifacts"); a
future `--reporter junit` writes *into* this dir rather than needing its own
destination flag.
- No auto-clear semantics: a reused dir keeps files from earlier runs — CI
should point it at a fresh workspace dir.
- Unit: `exportFailureArtifacts` (copy + path rewrite, failure-only, key
fallback, null-role skip, warn-and-continue on copy error) and `runSnapshot`
emitting `snapshotKey`.
- End-to-end against the Electron smoke-app fixture: seeded a baseline,
tampered it, and ran the built CLI against a from-source tool-server —
verified the three PNGs land at the namespaced paths, the report and
`--json` print the durable paths with exit 1, the no-flag default still uses
tmp, an unwritable `--output` warns on stderr without changing the verdict,
and re-running into the same dir overwrites cleanly.
Updated the `argent-create-flow` skill's standalone-runner section; the flow
docs (`docs/writing-flows-by-hand.md`, `docs/flow-system-internals.md`,
`docs/flow-roadmap.md`) received matching updates.
9c408b3 to
3edb401
Compare
latekvo
left a comment
There was a problem hiding this comment.
Notes from reviewing the flow-e2e runner, recorder, tree adapters, snapshotting, and CLI. Details inline.
|
|
||
| export function deriveSelector(node: DescribeNode): Selector | null { | ||
| if (node.identifier && node.identifier.trim()) return { identifier: node.identifier }; | ||
| const text = nodeText(node).trim(); |
There was a problem hiding this comment.
The recorder derives a text selector from nodeText, which joins label and value with a space (a control with label "Volume" and value "50%" yields { text: "Volume 50%" }). But matchNode compares a text selector as a substring of label OR value individually, never their concatenation, so the derived selector matches no node, including the one it was derived from. For any labelled control that also exposes a distinct value and has no identifier (Android nodes with a content-desc plus text, Chromium nodes with a label plus value), captureTapSelector fails its re-resolve check and records a coordinate tap instead of a portable selector. The warning it emits then reads "resolves to a different element on this screen", which is inaccurate: it resolves to no element at all. Verified end-to-end through the Android adapter: a SeekBar with content-desc "Volume" and text "50%" derives { text: "Volume 50%" } (self-match false, 0 matches, coordinate fallback), while the same node with only a label derives { text: "Volume" } and matches.
| if (t) childText.push(t); | ||
| } | ||
|
|
||
| const subtree = [view.ownText, ...childText].filter(Boolean).join(" "); |
There was a problem hiding this comment.
subtree concatenates the node's own text with every descendant's text unconditionally, and it is stamped as subtreeText whenever it differs from the node's own text. When an identified container carries its own label (an accessibilityLabel / content-desc) that a descendant also renders, the two are concatenated: a testID button labelled "Submit" wrapping a <Text>Submit</Text> yields subtreeText = "Submit Submit". assertText prefers subtreeText, so assert: { text: { in: <that id>, equals: "Submit" } } fails even though the screen shows exactly "Submit". Partial overlap compounds it ("Submit" + child "Submit now" -> "Submit Submit now"), and concatenation follows traversal order rather than visual order. Verified end-to-end on the Android adapter (equals returns false, contains still passes); a testID container with no own label is unaffected.
| }; | ||
| if (tok === "--update-baselines") out.updateBaselines = true; | ||
| else if (tok === "--json") out.json = true; | ||
| else if (tok === "--device") out.device = takeValue("--device"); |
There was a problem hiding this comment.
parseRunArgs only recognizes space-separated flags. A --flag=value token (--platform=ios, --device=<UDID>, --output=dir) matches none of these branches, and because it begins with - it isn't captured as the positional flow name either, so it is discarded with no error. --device=… then falls through to device auto-detection and the run targets whichever device happens to be booted; --output=… silently skips the artifact export; --platform=… is ignored. This is the same silent fallback the takeValue guard just above exists to prevent, and it differs from the argent run / argent tools parser (flag-parser.ts), which accepts the --name=value form. Verified against the built CLI.
| // Wait for the UI to settle (a transition/reflow finished) so the capture is | ||
| // stable run-to-run, rather than guessing a fixed delay. `settleTree` returns | ||
| // undefined only on abort; a best-effort timeout still proceeds to capture. | ||
| await settleTree(env); |
There was a problem hiding this comment.
runSnapshot awaits settleTree with no try/catch, but settleTree throws (rather than returning) when every tree read in its window fails — the sustained native-devtools-disconnected outage its own docstring cites. That throw propagates to the snapshot step's catch and reports the step as error, even though the capture (screenshot) reads pixels and needs no describe tree. The comment here states the intended behavior — "a best-effort timeout still proceeds to capture" — which holds for the abort and stale-timeout cases but not the throw. Verified end-to-end: with the tree source down, the snapshot reports error and screenshot is never invoked, failing a capture that would have succeeded.
|
|
||
| case "launch": { | ||
| const r = await runLaunch(state, step.app); | ||
| return { ...base, status: r.ok ? "pass" : "error", reason: r.reason }; |
There was a problem hiding this comment.
The launch case is the only leaf step with no abort handling: it maps r.ok ? "pass" : "error", whereas the directive case just below (line 759), wait, and tool-delay all report a cancelled run as skip + "run aborted". Inside runLaunch the sleepOrAbort return is discarded and treeSourceGate returns null (ready) on abort, so a run cancelled during the post-launch settle/gate reports the launch as pass (ok:true, having verified nothing); a cancellation during restart-app makes that sub-tool reject and the launch is reported as error: restart-app failed: …, attributing the cancellation to the app. Verified end-to-end both ways (launch:pass ok=true and launch:error ok=false). A CI gate or agent reading the report can misread a cancelled run as a passed or failed launch; flow-abort.test.ts covers tap/type/await but not launch.
| case "hidden": | ||
| return !matches.some(isVisible); | ||
| case "text": { | ||
| const first = firstInReadingOrder(matches); |
There was a problem hiding this comment.
The text case picks firstInReadingOrder(matches) over all matches, unfiltered by visibility, so a zero-area match at the top of the screen shadows the visible element the check was meant to read: a stale zero-area "Total 0" node makes equals "42" against a visible "Total 42" return false. The failure message (assertReason) instead inspects matches.filter(isVisible) first, so it quotes the visible node — producing a self-contradictory message such as its text was "Total 42" (wanted to contain "42"), which does contain "42". The success check and the message read different elements.
| * escalation as {@link killChildEscalating}: SIGTERM, then SIGKILL after a grace | ||
| * period. An already-exited process is a no-op, not an error. | ||
| */ | ||
| export function killChromiumByPid(pid: number): void { |
There was a problem hiding this comment.
Unlike killChildEscalating just above — which guards its delayed SIGKILL with child.exitCode === null && child.signalCode === null — this variant holds only a raw pid and issues the SIGKILL unconditionally after the grace period. If the Electron process exits within the 2s window (SIGTERM often succeeds quickly) and the OS recycles that pid, the SIGKILL lands on an unrelated process. The comment's "same escalation as killChildEscalating" doesn't hold, since the exit-status check that makes that one safe isn't available without a process handle. This runs in the flow's chromium teardown (teardownBootedChromium) after every runner-booted Chromium e2e flow.
| args.json ? undefined : { onProgress: onStepReport } | ||
| ); | ||
| const { url, token } = await baseUrl(); | ||
| const materialized = await materializeArtifacts(resp.data, { toolsUrl: url, authToken: token }); |
There was a problem hiding this comment.
materializeArtifacts deep-walks the entire report and downloads every image/* handle it finds, but the command uses only materialized.result and renders artifact paths as text (the CLI StepReport type has no result field, so tool-step results are never displayed). Against a remote tool-server (argent link), this downloads every screenshot captured by tool steps during the run — and, when --output is absent, all three baseline/current/diff PNGs of each failed snapshot — solely to print paths in a terminal that shows no inline images. The MCP renderer deliberately avoids fetching the full-res baseline/current for this reason.
| * copy error warns on stderr and leaves the temp path in place; artifact export | ||
| * must never change a run's verdict. | ||
| */ | ||
| export async function exportFailureArtifacts(report: FlowReport, outputDir: string): Promise<void> { |
There was a problem hiding this comment.
exportFailureArtifacts builds the destination from report.flow and snapshotKey (path.join(outputDir, report.flow, "<key>-<role>.png")) with no containment check, so a report whose flow or snapshotKey contained .. would write the copied PNG outside --output. The legitimate tool-server validates both, so this isn't reachable through it today, but it is inconsistent with the containment applied everywhere else here: the keyFromBaselinePath fallback sanitizes via path.basename, and getFlowPath adds a path.relative check on top of the same name regex. This is the one artifact path that trusts the server value verbatim.
| .describe("Milliseconds to sleep before executing this step during replay."), | ||
| }); | ||
|
|
||
| function formatSelector(selector: Selector): string { |
There was a problem hiding this comment.
formatSelector duplicates describeSelector in flow-actions.ts (both introduced in this PR): the same Object.entries -> map identifier->id -> join(" "), with the same explanatory comment. describeSelector differs only by a .filter that drops the loose key, which is a no-op for these inputs. The two selector-to-string paths have diverged into copies.
| executionPrerequisite: flow.executionPrerequisite, | ||
| steps: flow.steps.map(toYamlStep), | ||
| }; | ||
| const doc: YamlFlowFile = { steps: flow.steps.map(toYamlStep) }; |
There was a problem hiding this comment.
serializeFlow writes the flow via yamlStringify, which emits any multi-line string value (a type step's text, an await/assert expectedText, an echo message, executionPrerequisite) as a YAML block scalar, and block-scalar chomping is not round-trip-safe for these shapes. So parseFlow(serializeFlow(x)) is not the identity: a value whose final line has trailing whitespace loses it (type text "line1\nline2 " replays as "line1\nline2", and equals "Total\n99 " asserts "Total\n99"), and a whitespace-only-with-newline value serializes to |+ and then fails to re-parse ("type needs a non-empty text") - leaving a corrupt flow file on disk in host mode, or shipping lossy YAML to the client that only fails at replay. Reproduced end-to-end through the save/reload path; recorded selectors are unaffected (they are trimmed), so this is scoped to the free-text value fields.
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-executeand 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 anexecutionPrerequisite, composable viarun: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/textconditions),wait,snapshot,echo, and rawtool:as escape hatch — still fully supported, so previously recorded flows keep working unchanged.{ 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 reportskip.Execution engine
--device/--platformor the single booted one).textassertions read what an identified container visibly shows. Vega is remote-driven, so touch directives fail with guidance to usetv-remote/keyboard.launchadditionally gates on platform tree-source readiness instead of silently degrading.Visual snapshots
snapshotdiffs a full-resolution screenshot against baselines in.argent/flows/__baselines__/<flow>/, keyed by platform + resolution, with configurablemaxMismatchand--update-baselines. The status bar is pinned for the whole run (iOSsimctl 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 selectortap:steps (selector captured from the pre-tap tree), a leadingrestart-appbecomeslaunch:,flow-executeof a sibling fragment becomesrun:, and device ids are stripped. The skill instructs the agent to polish the saved YAML afterwards (fold tap+keyboard intotype:, swipes-to-reach intoscroll-to:, awaits into sugar form).Also included
argent flow run/argent flow listCLI with per-step report rendering, artifact materialization, and--jsonoutput.