perf(flow): stop the recorder returning the whole flow file per step - #715
Conversation
e769e4f to
007ccb0
Compare
007ccb0 to
4b67151
Compare
4b67151 to
5acfbc4
Compare
j-piasecki
left a comment
There was a problem hiding this comment.
Adversarial review of the diff at 2fa6897. Four things worth acting on, all small — nothing here blocks. Two are load-bearing (a comment whose stated invariant this PR falsifies, and a coverage gap on stepCount); two are nits.
One point on the PR description rather than the code: Compatibility says "flow-finish-recording is unchanged", and its result shape is — but its summary strings are not. Taps now render ×N, long-presses for Nms, tool steps (after Nms). The PR's own finish-recording's summary carries the same delay/times spellings as recorded test pins the new output. Nothing outside the agent consumes summary, so it isn't a break — the section just reads broader than it is.
Checked and found nothing, for the record
- No consumer of
flowFilefromflow-add-step/flow-add-echoexists outside the tool-server tests this PR updates — grepped.ts/.tsx/.md/.jsonacrosspackages/. The breaking change is contained. - Telemetry is an allowlist (
ALLOWEDinpackages/telemetry/src/sanitize.ts), so the newrecordedfield is not emitted. Swapping a whole-YAML field for a per-step summary line cannot widen what leaves the machine. recordedrenders the in-memory step while the file holds the serialize→parse round-trip of it. That is only sound because parse/serialize are exact inverses, and the one place the in-memory copy can hold something the file cannot —times: 1— is explicitly handled and tested. The round-trip is also pinned directly (result.recorded === summarizeStep(parseFlow(onDisk)…)) in five places, across both host and client mode.- The takeover scenario in the skill's "Pick a name unique to your task" bullet does not get worse: an agent whose key was stolen now sees
stepCount: 1where it expected 6, which is a sharper signal than the truncated YAML it used to get. savedTo: null, as the SKILL.md commit documents, is real —applyClientFileDirectivesinpackages/argent-tools-client/src/file-inputs.tsresolves a rejected path or a failed write tonull.yarn ts-checkclean;flow-tools,flow-remote-recording,flow-record-tap,flow-concurrent-recordingandinteraction-messagesall green.
Not filed inline: flow-add-step now imports from flow-finish-recording for a pure renderer. No cycle today, and moving summarizeStep + selectorLabel + textConditionLabel + renderToolArgs into their own module is more churn than this PR warrants — noting it only because the dependency direction (recorder → finisher) is the one that will read oddly to the next person in this file.
2fa6897 to
a39e06f
Compare
| } | ||
| }); | ||
| case "type": | ||
| return `${n}. type: ${selectorLabel(step.into)} ← "${step.text}"`; |
There was a problem hiding this comment.
[Diplomat]: type.submit and await.timeout are dropped while times, duration and delayMs are now rendered, and the argument stated at 202-203 covers all five equally: "times (tap) and duration (long-press) change what replays, so a summary line that drops them misdescribes the file."
Both fields survive the parser and neither arm reads them, so a step renders identically with and without. Running parseFlow + summarizeStep at d185472:
type submit:false step: {"kind":"type","into":{"identifier":"pw"},"text":"hunter2","submit":false}
render: 1. type: {"id":"pw"} ← "hunter2"
await timeout:15000 step: {"kind":"await","condition":"visible","selector":{"identifier":"home"},"timeout":15000}
render: 1. await: visible {"id":"home"}
submit: true normalises away, so submit: false — the case that suppresses the Enter press — is exactly the one that reaches here. long-press.duration was added under this rule despite having no recorder path at all, so recorder-reachability is not the line being drawn between them. Neither field appears in the new summarizeStep rendering block.
There was a problem hiding this comment.
Not fixed, and I think out of scope here. The behaviour is unchanged by this PR — the type and await arms are byte-identical to 4c30ae2, and both fields render identically with and without on base. What this PR added was the rule, stated more broadly than it is applied. 000fee5 records the boundary instead: neither kind is recorder-built, so both reach an author only through the finish summary, beside the flowFile that spells them out. Rendering them is a fair follow-up, just not one this per-step view opened.
d9b0249 to
b3ff6e7
Compare
b3ff6e7 to
c69578b
Compare
…oving (#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>
…-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>
`flow-add-step` and `flow-add-echo` returned the entire growing YAML on every call. That made the recorder the single largest consumer of a recording session's context — it grows quadratically with step count, and on the measured walkthroughs it accounted for the majority of all tool-result text. The pressure was observable in the work itself: checks get dropped from a test to buy back context. Both tools now return `stepCount` instead, and `flow-add-step` adds `recorded` — the one summary line that was appended. The full file still comes back once, from `flow-finish-recording`. `summarizeStep` moves out of `flow-finish-recording`'s per-file loop so the recorder can render that single line the same way the final summary does, and it now carries `times`, `duration` and `delayMs`: those change what replays, and this line is the author's only per-step view of the file. Tests assert against the flow as PERSISTED rather than a returned string.
…s edit note The recorder read `session.flow.steps.length` for `stepCount` AFTER appendStepToFlow returned, i.e. outside the flow-file lock. A concurrent same-key append can reassign `session.flow` in that window, so the reported count and the `n.` prefix in `recorded` could be wrong (persisted YAML stayed correct — it is written in-lock). appendStepToFlow now returns the count it takes inside the lock, and both callers use it. The flow-add-step description claimed editing the .yaml mid-recording "can be overwritten by the in-memory copy". In host mode that is false: appendStep re-reads the file before every append and finish re-reads it too, so an edit between steps is kept. Scope the caveat to remote-client recordings, where the in-memory copy is authoritative. Drop the dead `delayLabel` from the tap/long-press summary branch: neither kind carries a `delayMs` (only `tool` steps do), so it never rendered. Correct the comment to name `times`/`duration`, not `delayMs`.
…ndering
The PR's headline outputs had no coverage: `recorded` was never asserted, the
new `×times` / `for Nms` / `(after Nms)` summary branches were never exercised,
and `stepCount` was only ever checked as a single value. Add:
- a recorded-line assertion for a tool step carrying delayMs, checked against
summarizeStep so the recorder and finish stay one spelling,
- a stepCount 1 -> 2 assertion across two appends,
- direct summarizeStep tests for tap times, long-press duration (which has no
live recorder path), and tool delayMs.
Refresh the flow-add-echo interaction fixture to the current result shape
({ stepCount } rather than the removed { flowFile }).
…a dead return
flow-add-step always returns `recorded` and `savedTo`, but the result type had
widened them to optional. Restore them to required, matching flow-add-echo,
which routes through the same appendStepToFlow. Also drop `flowFile` from
appendStepToFlow's return — once the per-step YAML was removed both callers
read only { savedTo, stepCount }, so the returned field was dead.
… the double-tap The summarizeStep rendering case built a tap selector with `id`, which FlowSelector's strict schema rejects — `tsc -p tsconfig.test.json` failed on it, while the runtime assertion passed because summarizeStep stringifies whatever it is given. Use `identifier`, the key a recorded selector actually carries; selectorToYaml maps it to the file's `id` spelling, so the expected line is unchanged. Also positively pin that add-step and add-echo no longer return `flowFile`, and cover the clickCount→times rewrite through the recorder (a recorded double-tap must replay as one) — a path no recorder test reached before.
…surfaces
summarizeStep printed `×${times}` whenever `times` was set, but a valid flow
file never carries `times: 1` — parseTapTimes normalizes it to absent — so a
stray in-memory `times: 1` would render a `×1` that describes a file that
cannot exist. Guard the count on `> 1` so the summary matches the file's own
spelling.
Tests: pin that summarizeStep never emits ×1; that flow-finish-recording's
`summary` renders the same delay/times spellings as the per-step `recorded`
line (the other summarizeStep consumer, previously only unit-covered); and
that client/remote mode reports a running stepCount without echoing the YAML
per step.
The run:-composition cases main added since this branch forked assert on `result.flowFile`, the full YAML this branch stops returning per step. Read the same steps back from the flow file instead, which is what the other cases here already do, and keep main's `<name>.yaml` run-target spelling.
Mutating both arms of summarizeStep to constants failed zero tests, even though the recorder builds both kinds and now echoes each through the new per-step `recorded` line - so a reader had no coverage telling them a restart-app was stored as a launch, or a flow-execute as a run:. Pin both surfaces: the pure rendering (string app, per-platform map, and the as-written run target) and the two live recorder paths that produce them, asserting `recorded` against summarizeStep of the step on disk.
Hardcoding `stepCount: 1` in the return passed every flow test: only flow-add-echo's running total was held, and add-step's was asserted only at the value 1. stepCount is the recorder's sole per-step size signal now that the growing YAML is no longer returned, and it is also the number `recorded` is rendered with. Record three steps and pin 1, 2, 3 against the file on disk, plus the invariant that each `recorded` line opens with the count it was reported alongside.
Returning a bogus path from appendStepToFlow's host branch passed every flow test. With `flowFile` no longer returned per step, `savedTo` is the only field naming where the YAML landed, so nothing was holding it to the real path. Assert it on the two callers of that branch - flow-add-echo and flow-add-step.
flow-add-step gained a `recorded` line and flow-add-echo did not, and nothing held the asymmetry - only the absence of `flowFile` was asserted, on both tools. Pin it, with the reason: an echo step is entirely the message the caller just passed, whereas a recorded step can be rewritten on the way in (a coordinate tap into a selector, a restart-app into a launch) and needs a line saying what actually landed.
`recorded` was covered only for coordinate/tool steps on the host branch: nothing exercised it through selector capture, where the caller's coordinates are rewritten into a portable selector and the line is the only thing saying so - nor in client mode, which counts and serializes off a different branch of appendStepToFlow and could drift from the bytes the client is told to write. Add both, including the file's `id` spelling (capture produces `identifier`) and the coordinate fallback, and check every client-mode line against the YAML carried in the write directive.
One shared { message, flowFile, savedTo } object stood in for all four
recording tools' formatters. It stopped describing any of them once the
recorder dropped the per-step YAML: flowFile survives on start and finish
only, while add-step and add-echo report stepCount instead - so the file
described the result shape two ways, the sibling fixture below having been
updated already.
Split it per tool. Inert today (no formatter reads a field that differs),
but this is the fixture a later test copies.
flow-add-step now splits this by persistence mode, but the two places an agent reads FIRST - flow-start-recording's description and the skill's "Mistakes can be edited out" rule - still gave it unqualified, so one tool family answered the same question two ways. Give both the same host/client split, and add the failure mode none of them mentioned: the append re-parses the whole file, so a botched edit fails the NEXT step instead of being kept.
The description called it "the one line that was appended". It is neither
the appended text nor one line of it: a tap appends two lines of YAML
(`- tap:` / `id: PLACARD`) and `recorded` reports `1. tap: {"id":"PLACARD"}`,
a numbered summary in a different notation.
Say summary, give the spelling, and point at the reason to read it - the
tap and restart-app rewrites mean the stored step is not always the call
that was made.
"an edit made between steps is kept" describes only the good case. The append re-parses and re-validates the whole file, so a botched edit fails the NEXT step rather than being kept - the same hazard flow-finish-recording already documents for the finish path. Verified both halves against the recorder: deleting a step mid-recording is kept and renumbers, while an unparseable edit rejects the next append.
Exporting summarizeStep gave renderToolArgs a caller that comes through neither parseFlow nor fromYamlStep, so the comment's argument for why no reachable args is undefined no longer covered the input it now sees. State both paths: the finisher's parseFlow output, and the step the recorder builds, whose args is a fresh spread out of stripDeviceKeys.
Every stepCount assertion in the suite appends without editing, so the file's length and this session's tally of appends agree in all of them - swapping the count for a session-local counter kept the suite green. The hand-edit case is where they diverge, and where the number is load-bearing: it is what numbers the recorded line the author reads per step.
Typing the parameter over the whole FlowStep union was the only reason the field needed a cast to be read - and the cast is also what stopped the compiler checking it. Its one call site has already narrowed to the tool arm. The runtime typeof check stays: fromYamlStep copies delayMs across without validating it, so a hand-edited 'delayMs: soon' reaches the renderer as a string. Pinned, since nothing covered that before.
long-press is not a kind the recorder builds, so half the justification did not apply to half the branch it was attached to: that line reaches an author only through finish-recording's summary, which still returns the file beside it. duration is justified by misdescribing the file on its own.
The tool descriptions and the skill both re-explained what every return field means, why the YAML is not echoed per step and how to recover from a failed write. Name the fields and leave it there.
…ed one
delayLabel tested `typeof step.delayMs === "number"`, but flow-run gates on
truthiness and hands the raw value to setTimeout, which coerces it. The two
tests disagree in both directions, and this PR is what made the disagreement
visible - the tool: arm rendered no delay at all before it.
A hand-edited `delayMs: "2000"` is not a number and sleeps two real seconds;
`delayMs: .nan` is a number and sleeps none. fromYamlStep copies delayMs across
unvalidated and validateFlow does not check it, so both survive a parse, and a
quoted numeric is an ordinary slip in the hand-edit workflow the skill
documents.
Gate on truthiness the way the runner does, then render what setTimeout will
wait, dropping anything it floors to an immediate tick.
Replaying four one-step flows through flow-execute against a server built from
this commit, timed, and finished through the same server for the rendered line:
(absent) 721ms 1. tool: list-devices {}
2000 2777ms 1. tool: list-devices {} (after 2000ms)
"2000" 3467ms 1. tool: list-devices {} (after 2000ms)
.nan 810ms 1. tool: list-devices {}
Before the fix the same four runs rendered nothing for "2000" and
`(after NaNms)` for .nan.
Every assertion that rendered a count used `times: 2`, so replacing
`×${step.times}` with a constant `×2` left the whole suite green. The rest of
the range is reachable - gesture-tap takes clickCount up to 10, flow-add-step
records it as `times`, and parseTapTimes admits 2..10 - and under that mutation
a recorded triple-tap renders ×2 on the `recorded` line, which is the author's
only per-step view of what was appended.
Confirmed the gap: with the constant in place the new case is the only one that
fails (`expected '1. tap: {"id":"b"} ×2' to be '1. tap: {"id":"b"} ×3'`); the
other eight in the block still pass.
The comment justifies `times` and `duration` with "they change what replays, so a summary line that drops them misdescribes the file" - an argument that covers `type.submit` and `await.timeout` just as well, and neither renders. Both survive the parser, neither arm reads them, and `submit: true` normalises away, so `submit: false` - the case that suppresses the Enter press - is exactly the one that reaches the renderer. The behaviour predates this PR and is unchanged by it; what this PR added was the rule, stated more broadly than it is applied. Record the boundary instead: neither kind is recorder-built, so both reach an author only through flow-finish-recording's summary, beside the flowFile that spells them out.
…ions 6759872 added the remote-client caveat to flow-start-recording and flow-add-step; d185472 cut it again while keeping SKILL.md's half, so the skill and the two tool descriptions answered the same question in opposite ways. The unqualified advice is wrong against a remote client: appendStepToFlow never re-reads the client's file, it pushes onto the in-memory flow and returns a whole-file directive that still contains the step the author deleted. The client applies it and the tool reports success, so the removal is silently undone. Restore one clause on each, scoped to "every write" rather than "the next append" - flow-finish-recording serializes the same in-memory copy, so it overwrites an edit too. Verified against a tool-server built from this commit: both descriptions come back over GET /tools carrying the caveat.
…elds The recorder no longer returns the whole flow file per step, so the three places that told the author to read the returned YAML now describe a field that is not there. Name what each call does return - recorded, stepCount, savedTo - and say where the full file still comes from.
c69578b to
90f1524
Compare
main's #715 split the recorder's per-step line out of the step-summary switch (exported `summarizeStep`) and taught it three fields the old rendering dropped: a tap's `times`, a long-press's `duration`, and a tool step's `delayMs`. This branch had replaced that switch with the per-kind `FLOW_STEP_DEFINITIONS` record. Both sides land: the record gains the three renderings and `delayLabel`, and now backs an exported `summarizeStep` that `summarizeSteps` maps over, so the recorder's echoed line and the finish summary still share one spelling. `flow-add-step` and the tests take it from flow-step-definitions, the module that owns rendering after the refactor.
What changes
flow-add-stepandflow-add-echoreturned 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.stepCount— how many steps the flow has — instead offlowFile.flow-add-stepadditionally returnsrecorded: the one summary line that was appended.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
summarizeStepis lifted out offlow-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,durationanddelayMs. 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:
flowFileis gone fromflow-add-stepandflow-add-echo.savedTois unchanged and is still the only field naming the destination in remote-client mode.flow-finish-recording's result shape is unchanged, but itssummarystrings are not: taps now render×N, long-pressesfor Nms, and tool steps(after Nms), so the two surfaces spell a step the same way. Nothing outside the agent readssummary, 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.