fix(flow): re-probe a recorded wait against the tree the runner reads by hubgan · Pull Request #717 · software-mansion/argent

hubgan · 2026-08-04T06:55:47Z

Stacked on #715.

The problem

await-ui-element evaluates against the accessibility tree. The await: / assert: directive that the polish pass converts a recorded wait into is evaluated against the runner's full hierarchy.

They overlap, but neither contains the other — an id present in one can be absent from the other, and on iOS even the role vocabularies are disjoint. Both directions have been hit in practice: an id visible to the runner but not the recorder, and text-field values visible to the recorder but not the runner.

The consequence is that a check can pass live and fail once converted. That makes the core promise — each step is executed live so you verify it works before it's recorded — untrue at exactly the point it is relied on.

What changes

After a recorded await-ui-element succeeds, the recorder re-runs the same condition against the tree the runner resolves directives against, and reports the result on the step it just wrote.

It warns; it does not refuse. What gets written is a raw tool: await-ui-element step, and at replay that tool reads the same accessibility tree it just passed against — so "this would fail every run" was never true of the form actually recorded. What the probe genuinely reports is whether the polish-time conversion is safe, which is why the warning says to act on it then rather than re-record now (re-recording would duplicate the step).

Three outcomes, kept distinct

Runner tree says	Reported as
condition holds	nothing — step recorded clean
condition does not hold	recorded + warning that converting to `await:`/`assert:` will fail
tree unreadable	recorded + warning that the conversion is UNKNOWN, not known-bad

The third row matters: nothing was compared, so calling it a divergence would send the author off to rewrite a selector that may be perfectly good. It is also the injection-free case the flow skills explicitly sanction, so refusing there would block a supported way of working.

Platform-specific explanation

The why differs per platform, and stating the iOS story everywhere makes the message wrong where someone is trying to act on it. iOS gets AX-vs-native-hierarchy; Chromium gets "the flow tree keeps only addressable nodes"; Android gets the importance trim. The named tool for reading the runner's side follows the same rule — pointing an Android author at native-find-views points at a tool they cannot call.

j-piasecki

Adversarial review of the cross-tree re-probe. Verified locally on 27503f4: tsc --build clean, prettier --check clean, flow-record-cross-tree.test.ts 31/31 green, scripts/extract-tools.test.mjs 46/46 green.

Three findings below. Everything else I chased came back clean and is not reported: settleWithin already consumes the abandoned promise (no unhandled rejection on the timeout path); resolveDevice really has no throw path, so the no-try/catch comment holds; AbortSignal.any is already established in flow-pixels; the giveUp abort genuinely prevents the post-deadline finalPoll read; identifier is a parse-only alias in flow YAML, so "copy the recorded selector: map through unchanged" is sound; and the iOS merge / Android resource-id shield / Chromium [password] divergences all reproduce through the real adapters.

I also dropped two candidates after checking them, since neither survives as a defect:

Flakiness in keeps the tail of an over-long reason. probeStartedAt is captured before startRecording, so the fixture's 900 ms trusted-read window is shortened by however long the setup takes; past ~300 ms the dark tail exceeds CONDITION_DARK_TAIL_TOLERANCE_MS (600 ms) and the verdict flips to indeterminate, failing the test. But I measured the actual gap at 1–4 ms over five runs — a ~100x margin. Worth knowing the threshold exists; not worth changing.
Suite wall-clock (~24 s for the file). Each determinate-warning case burns the full 1 s assert grace, and probeWhenCondition hardcodes DEFAULT_ASSERT_TIMEOUT_MS with no seam to shorten it. Real cost, but the only fixes are a production seam or fake timers over an async poll loop — more risk than the seconds are worth.

j-piasecki

Re-review after the previous round. All three earlier comments are addressed: the determinate consequence is now split per cause in probeAgainstRunnerTree (and flow-record-cross-tree.test.ts asserts the message no longer contains "WILL fail"); SKILL.md's flow-tree drop rule is scoped to Chromium/iOS/Android; the stranded JSDoc block is back on warningOf, with echoedReasonOf keeping its own.

Verified locally on bbbce187b: tsc --build clean, prettier --check clean on all four files, flow-record-cross-tree.test.ts 31/31, scripts/extract-tools.test.mjs 46/46.

Claims I chased and cleared:

"As the raw tool: step it replays fine." flow-run.ts:2149-2171 runs a kind: "tool" step through invokeSubTool, so a recorded await-ui-element reads its own describe tree at replay. Holds.
The Chromium walk-limit numbers. DEFAULT_WALK_LIMITS.maxNodes is 5000 (describe/platforms/chromium.ts:53), FLOW_WALK_LIMITS.maxNodes is 12000 (flow-chromium-tree.ts:94). Both directions of that clause hold.
PROBE_BUDGET_MS's Android claim. getHierarchy is the one method that gets LONG_RPC_TIMEOUT_MS = 15s (android-devtools-client.ts:26,121). Holds.
The probe's cost on the clean path. records the step, with no warning, when both trees agree runs in 16 ms — waitForCondition returns on the first poll, so only a condition that does not hold burns the 1s assert grace.
typeof selector !== "object" silently skipping a real wait. selectorSchema is a refined object schema (ui-tree-match.ts:70), so a recorded await-ui-element can never carry a string selector. Guard is dead by construction, not a gap.
Flake risk in the two ~4s tests. gives each platform a distinct remedy and gives up on a tree read that outruns the probe budget measured 4020-4032 ms and 4057-4062 ms over five runs, and both carry explicit 15_000 timeouts — nothing near vitest's 5s default. (The file's other tests top out at ~1013 ms, and the suite already has 7755 ms tests elsewhere.)

Two comments, both on doc surfaces the two conditioning commits did not reach.

latekvo

[Diplomat]: Ran the full sweep at bbbce187.

End-to-end, on a real device. Branch-built tool-server driven over HTTP against a live Chromium (Electron) target, recording through the real flow-add-step path. All three outcomes fire correctly: a wait both trees agree on records clean; an off-viewport exists and a {role: input, text: Passphrase} wait on a password field each produce the determinate warning; an unmet wait produces the unmet-wait warning with fetchFlowTree never called. I then replayed both forms with flow-execute to test the warning's two predictions - the raw tool: steps pass 2/2, and both assert: conversions fail with exactly the reason the probe quoted. The two remedies the Chromium clause prescribes also pass when run: scroll-to before the check for a zero-height frame, and id/role for a password field.

The reverse divergence direction reproduces too: on a 9000-node page with a viewport-pinned <button id="marker"> last in document order, describe stopped at 4997 nodes without it while the flow tree carried it, so a hidden wait passed live and the probe warned - with the hidden-specific awaitStillNeeds clause ("unless the element LEAVES that tree").

Suite. tsc --build clean; flow-record-cross-tree.test.ts 31/31. I mutation-tested it in an isolated worktree: PROBE_BUDGET_MS (both directions), MAX_PROBE_REASON_CHARS/PROBE_REASON_TAIL_CHARS, deleting giveUp.abort(), flattening awaitStillNeeds's per-condition arms, swapping the ios/android arms of runnerSideReadClause, short-circuiting the indeterminate branch, dropping the isUnmetUiWaitResult gate, and removing cappedReason are each caught by a failing test. Three comments below.

Claims I chased and am not reporting:

A wait nested in a recorded run-sequence gets no message warning. Reproduced E2E - recorded silently, and at replay the flow fails with run-sequence stopped at await-ui-element after 0 of 1 steps. But toolResult comes back {completed: 0, total: 1, steps: [{tool, error}]}, the skill twice instructs the author to read exactly that, and flow-nested-outcome.ts:12-21 states the tool-scoped-branches doctrine deliberately. The rule this PR rewrote ("only steps that RETURNED successfully") describes the case correctly.
The probe's abandoned tree read holding the Android devtools client. The mechanism is real - I measured 5006 ms of head-of-line blocking against the real connectAndroidDevtoolsClient with a fake helper socket, since chain.then(send, send) serialises every RPC and the in-flight getHierarchy is uncancellable. But captureTapSelector already awaited fetchFlowTree on that same chain unbounded at a39e06fe, and the only >4s read measured anywhere in the repo is attributed to Chromium, whose CDP transport has no request chain.
cappedReason returning a longer string than its input for reasons of 201-218 chars. The dropped-character count is correct and the band is cosmetic.
The 12000-vs-5000 sentence at SKILL.md:71 being unqualified where iOS and Vega have no node cap. Android's describe does cap at 5000 - getHierarchy's maxNodes ?? 5000 default in blueprints/android-devtools.ts:340 - so the claim holds on both platforms that have a walk limit at all.
ios-remote reaching UNSUPPORTED_PLATFORM. classifyDevice emits exactly the five platforms the JSDoc names, and platformMatrix maps ios-remote to capability.appleRemote, which await-ui-element does not declare - so assertSupported throws while the step is still executing live. The unreachability claim holds.

`await-ui-element` evaluates against the accessibility tree. The `await:`/`assert:` directive that polish converts a recorded wait into is evaluated against the runner's full hierarchy. The two overlap but neither contains the other: an id present in one can be absent from the other, and on iOS even the role vocabularies are disjoint. So a check could pass live and fail once converted — which makes "each step is executed live so you verify it works before it's recorded" untrue exactly where it matters most. The recorder now re-runs the same condition against the runner's tree and reports the answer on the step it just wrote. It warns rather than refuses, deliberately. What gets written is a raw `tool: await-ui-element` step, and at replay that tool reads the same tree it just passed against — so "it would fail every run" was never true of the form actually recorded. What the probe really reports is whether the polish-time conversion is safe. An unreadable runner tree is reported as UNKNOWN, not as a divergence: nothing was compared, and calling it a mismatch would send the author to rewrite a selector that may be perfectly good. The explanation is platform-specific — iOS AX-vs-native, Chromium's addressable-nodes filter, Android's importance trim — because stating the iOS story everywhere makes the message false where the author is trying to act on it.

…t message The cross-tree warning ended by telling the author which tool reads the tree the RUNNER resolves against. On Android that clause read "`describe` reads the runner's side" — but Android `describe` returns the TRIMMED interactables tree the recorder already read, while the runner reads the full accessibility hierarchy, which no read-only tool exposes (`native-find-views` / `native-full-hierarchy` are Apple-only). So the message pointed the author at the recorder's own tree under the banner of the runner's — the exact wrong-tree steer this warning exists to prevent. Route Android through a clause that says no read-only tool reads it, rather than naming one that reads the wrong tree. iOS listed `describe` next to `native-find-views`, but iOS `describe` is the AX tree — the recorder's side — so it is dropped; `native-find-views` is named alone, where the point is to name the runner's reader. The abort error claimed "aborted before the recorded tool was executed," but its only throw site runs during the re-probe, strictly after the recorded tool has executed. It now says what actually happened. Tests cover the Android and Chromium reader clauses; only iOS was asserted, and the Android wording was wrong.

…rsion The determinate cross-tree warning told the author that converting the recorded wait to `await:`/`assert:` at polish WILL fail. The probe reaches that verdict after DEFAULT_ASSERT_TIMEOUT_MS (1s), which is exactly the grace an `assert:` conversion polls on — so it predicts `assert:` precisely. But an `await:` conversion polls `step.timeout ?? DEFAULT_ACTION_TIMEOUT_MS` (7.5s by default, or a custom timeout), strictly longer than the probe's window. An element that reaches the runner's tree between the probe's 1s and await:'s timeout would make the converted `await:` pass while the flat "WILL fail" said it wouldn't. Word the two conversions apart: `assert:` WILL fail (same short grace the probe just used), and `await:` will too only unless the element reaches that tree within its longer timeout. The advice is unchanged.

…e runner's reader On Chromium the recorder and the runner both walk the DOM, but the runner's tree keeps only addressable nodes (id/label/value/clickable/focused) while `describe` returns the full DOM the recorder read — a superset that still shows the very non-addressable nodes the runner drops. So when a recorded wait diverges (e.g. an exists/role selector on a role-only element), telling the author "`describe` reads the runner's side" sends them to a tool that shows the element the runner can't see: they conclude the warning is a false alarm and ship an `assert:` that fails every replay. Special-case Chromium in runnerSideReadClause the same way Android already is, so the clause says no read-only tool exposes the runner's trimmed tree and points the author at an addressable selector instead. Update the Chromium test to pin the corrected message and that it no longer claims describe reads the runner's side.

…e text and abort probe paths The cross-tree warning is composed as `${probe.warning}. ${treeDivergenceFor} ${runnerSideReadClause}`, so the Android and Chromium reader clauses always follow the divergence sentence's period as their own sentence — yet they began lowercase, rendering "...never reaches the runner. no read-only tool exposes...". Capitalize the two clauses so the sentence reads correctly; the iOS/default clause already starts with a code-span subject and is unaffected. Also cover two probe paths the suite never exercised: - a `condition: "text"` wait, which forwards expectedText/textMatch into the runner-tree evaluation. One test pins that a matching value records with no warning (guards against the fields being dropped, which would score `text` false and warn on every agreeing text wait); the other pins that an absent value warns (guards against expectedText being ignored). - the abort path: a run cancelled during the re-probe must throw AbortError and record nothing, since the abort lands after the recorded tool ran.

`await-ui-element` reports an unmet condition by returning { success: false } rather than throwing, so flow-add-step's success path records the step and — since this branch — attached the cross-tree warning to it. That warning says the raw step "replays fine, it reads the same tree it just passed against", which is false twice over: it never passed, and at replay an unmet raw wait fails the step and stops the run (the same shape run-sequence and flow-run already read through isUnmetUiWaitResult). It also blames a recorder-vs-runner divergence and asks for "a selector present in both" when the element is on neither tree. Consult isUnmetUiWaitResult like the other two orchestrators do and report that path for what it is, skipping the re-probe: the probe asks whether a check that PASSED survives conversion to `await:`/`assert:`, and this one did not pass. Recording the step anyway is unchanged — only the narration.

…low tree probeWhenCondition budgets its POLL LOOP at the 1s assert grace, but that bounds only the loop: each fetchFlowTree inside it is awaited with no time bound, the clock is checked between reads, and one more read fires back-to-back after the deadline. A single read is 10s on Chromium CDP and up to 20s for an Android uiautomator dump, so the advertised "short grace" really ceilings at about two full reads - measured at 8.3s and 18.9s against a throttled background renderer, and once reported as a 10003ms overrun of its own 1s window. The live await-ui-element does not have this problem because pollDescribeTree races every fetch through settleWithin. The flow runner's copy of the loop does not, and its own callers run unattended where an overrun costs only time - so bound it at the recorder's call site rather than changing the shared loop. An overrun is reported as indeterminate, never as a verdict, exactly like a tree source that could not be read.

… question The cross-tree warning ended "`native-find-views` reads the runner's side" on iOS. It does not. A recorded wait's selector matches `text` as a case-insensitive substring of an element's label or value and `role` as a substring of a derived role name; native-find-views accepts only className, identifier, label, tag and nativeID, all exact - no text, no role. It also returns matches straight from the RPC with none of the filtering queryFullHierarchyTree applies, so it reports the hidden, transparent, scroll-clipped and unlabelled container views the runner drops while missing the substring matches the runner makes. This is the same trap Android and Chromium are already special-cased for - the comment there says naming a superset "would point the author at the recorder's own tree under the banner of the runner's" - so give iOS the same treatment. treeReaderFor's iOS arm goes with it.

…divergence The indeterminate branch's own comment says it must not claim the two trees differ, because nothing was compared. One line later the caller appended the divergence explanation and its "so re-record with a selector present in both" remedy to BOTH warning kinds, so a message whose whole point is that the runner's tree could not be read still ended by telling the author to rewrite a selector that may be perfectly good. Compose the warning where the branch is chosen instead of at the call site, so only the determinate verdict - the one that really did compare the two trees - carries the explanation and the remedy.

…nner The re-probe paragraph I added interpolated AWAIT_UI_ELEMENT_TOOL_ID into the description template literal. scripts/extract-tools.mjs only reads a description that is a single string/template literal with no interpolation - anything else it warns about and SKIPS, so flow-add-step silently dropped out of tools.json and out of the Tool Description Quality gate's scoring (75 tools -> 74). Spell the tool id literally. Verified with the CI gate run locally: 75 tools scored, average 9.069, identical to the base and above the 9.0 threshold.

The probe re-evaluates the recorded selector strictly, on its own fields. The conversion the skill prescribed was the bare-string sugar, which parses as a LOOSE selector - identifier first, text only as a fallback - so the two resolve different elements whenever some node's id equals the recorded text, and the verdict was wrong in both directions. Reproduced on Chrome via a branch-built tool-server: <button id="Continue">Proceed</button> record { condition: hidden, selector: { text: Continue } } -> message is exactly `Step added to "…" flow`, no warning -> `- await: { hidden: Continue }` FAILS ("an element matching text=\"Continue\" was still visible") -> `- await: { hidden: { text: Continue } }` passes the same button plus an off-viewport <div id=alpha>Continue</div> record { condition: exists, selector: { text: Continue } } -> message warns "an `assert:` conversion WILL fail" -> `- assert: { exists: Continue }` PASSES -> `- assert: { exists: { text: Continue } }` fails, as warned Rather than predict both spellings, name the one judged: the strict map form, which is a straight copy of the recorded `selector:` map. That is already the recorder's doctrine for a captured `tap:` - it emits `tap: { text: General }`, never the bare string, because a bare string re-parses as loose and routes through a fallback it never checked. Both repros re-run after the change: the verdict and the prescribed conversion now agree in both directions.

`await-ui-element` compares with `contains` unless the step passed `textMatch: equals`, and the recorded YAML omits the field when it was defaulted. The directive has no default and forces a pick, and the polish bullet showed only the `equals` spelling - so the recorder blesses the step and the conversion it steers you to is false on the very screen the probe just approved. Reproduced on Chrome via a branch-built tool-server, page `<div id="total-row">Total: $5.00</div>`: record { condition: text, selector: { text: Total }, expectedText: "$5.00" } (no textMatch) -> message is exactly `Step added to "…" flow` -> recorded YAML carries condition/selector/expectedText, no comparator -> `- assert: { text: { in: { text: Total }, equals: "$5.00" } }` FAILS: its text was "Total: $5.00" (wanted to equal "$5.00") -> `- assert: { text: { in: { text: Total }, contains: "$5.00" } }` passes State the mapping instead: no `textMatch` converts to `contains:`, only `textMatch: equals` converts to `equals:`. The new test pins the two comparators diverging on one tree, which is what makes that rule sound.

…know The Chromium warning had a single causal story - the runner dropped a node for being non-addressable or off-viewport - and every remedy followed from it. When the divergence is anything else the verdict is still right and the explanation is false, so an agent acting on the message churns on the selector or inserts a pointless `scroll-to`. Two reachable counter-cases, both reproduced on Chrome via a branch-built tool-server: <input id="pw-field" type="password" placeholder="Enter your secret"> the flow tree KEEPS the node and redacts its name to `[password]` (`assert: { visible: { id: pw-field } }` passes), so "an element with no id, label … never reaches the runner" was false in both halves and its "re-record with a text or label" remedy is unreachable by construction. 6000 <div id=deep-N> plus a fixed-position <div id=late-visible> `describe` walks 5000 nodes, the flow tree 12000 - `late-visible` is ABSENT from describe and present for the runner. The recorder is the short side, and "`describe` returns the full DOM the recorder read" is false exactly where the author would go looking. Say what the two projections do differently without asserting which side lost the element, name the password redaction and the two walk limits, and point at the one check that always settles it: run the conversion. Every platform also now admits the cause no tree story can rule out - the screen having moved on between the live wait and the re-probe - which until now only Vega said. The cap test's final assertion measured the whole warning against its own fixture, so re-wording the message broke it; it now bounds the echoed reason and pins the 200-char limit itself.

`settleWithin` only stops WAITING for the probe. The poll loop it walks away from is still awaiting its tree read, and when that read lands the loop finds itself past its own deadline and fires one more full read back-to-back - against a device the recorder has already returned from. The ceiling relocated the stall into a later step instead of removing it. Abort the loop the moment the ceiling decides, so its per-iteration signal check ends it before that read. The budget test used a read that never settled, which could not have caught this: a loop parked forever on its first read issues no second one either. It now holds the read open past the ceiling and then releases it with a tree that leaves the condition unmet - the only shape that reaches `finalPoll`. Against the previous code it fails with "expected 2 to be 1"; with the abort it stays at one read.

…rive Every divergence warning ended "…an `await:` will too unless the element REACHES that tree within its longer timeout". A `hidden` await passes when the element LEAVES, so on the one condition whose whole point is absence the escape hatch was described as its own opposite. Observed live on the dense-page repro from the review, which is a `hidden` check; re-run against a branch-built tool-server after the change, the same step now reads "…unless the element LEAVES that tree within its longer timeout". Word it per condition: `hidden` waits for the element to leave, `text` for its text to come to match, `visible`/`exists` for it to arrive.

The iOS clause answered "re-record with a selector a testID'd or labelled view carries". The create-flow skill's workflow for a testID the trimmed tree hides is the opposite: you cannot wait on it live, so gate on visible text to get the step recorded and retarget the id at polish - and that workflow is exactly what produces this divergence. So the warning sends the author back to record the step the skill just explained cannot be recorded, where they land on the unmet-wait warning instead. Point at the conversion, which is the thing that is actually wrong and the step that finishes the skill's workflow: retarget the directive at an id the full hierarchy carries (its testID coverage is complete) and prove it with `flow-execute`. Android's clause said "re-record with a selector an interactable carries" for the same reason and is wrong the same way, so it moves with it.

The 200-char cap exists because a failed `text` check can quote a whole screen - the flow tree hoists a container's text from every descendant. It was applied to two other things it should never have touched. The indeterminate branch. That reason is an environment error, it carries no screen content on any branch that produces it, and its TAIL is routinely the instruction for getting the tree source back: on a real iOS run against a non-injected app the cap cut "…use screenshot to inspect visible Home/…" ten characters from the end. Quote it whole. The determinate branch's tail. `waitForCondition` closes the reason with the note recording that its final poll went dark, and head-only truncation dropped it - silently removing the qualifier on the very verdict the warning is built on. The cap now elides the MIDDLE, keeping 140 chars of head and 60 of tail. Coverage: the boundary the wall-of-text fixture could not reach (a short reason quoted verbatim), the surviving final-poll note, and the uncapped environment error with a >200-char tail. All three fail against the previous code.

The fixture put the target inside a `com.android.systemui` node. Both parses drop system chrome - the flow adapter's `isSystemChrome`, the trim's `!opts.includeSystem && isSystemChrome` - so on a real device the live wait would have failed too and the recorder would have reported the unmet-wait warning instead. It only went green because the live tool is stubbed to succeed, which means it pinned the Android wording and proved nothing about Android. Both Android sides parse the SAME android-devtools getHierarchy dump, so a real divergence has to show up on identical input. Ran both parsers over one dump to find one: <LinearLayout id=continue-row clickable> <TextView id=continue-label text="Continue"/> trim: one node, id=continue-row label="Continue" (a clickable with no own label BORROWS its descendant's text and the child collapses into it) flow: two nodes; the row carries NO text, because the inner node's own resource-id shields its text from hoisting An everyday RN `Pressable testID` wrapping a `Text testID`. A `text` check on the row holds live and not for the runner, and the test now asserts that live premise against the trimmed parse rather than assuming it.

The iOS case served an `alpha: 0` view, on the premise that the AX tree still reports a fully transparent one. UIKit generally excludes hidden and transparent views from accessibility and nothing here re-adds them, so that premise is a device question this suite cannot settle - the fixture asserted a divergence it could not show was reachable. Use the `accessible`-container merge instead, which both sides settle from the sources: `captureTapSelector`'s own comment already states that the AX tree collapses such a container into one leaf whose merged label exists on no single view in the replay hierarchy, and the skill names it as the iOS divergence. The runner side is checked against the real adapter in the test - a container with two labels projects to ONE addressable leaf whose children's text is hoisted into `subtreeText`, which `findAll` does not match on, so the merged string resolves nothing. The projection rule the old fixture was reaching for stays covered, as what it is: a direct assertion that the adapter drops a transparent view, with no claim about what the recorder saw.

Two of its items are not divergences at all. Both trees drop system chrome on Android (the flow adapter's `isSystemChrome`, the trim's `!opts.includeSystem && isSystemChrome`), and the iOS adapter's own comment says its scroll-clip prune matches "the AX describe path, which never reports scroll-clipped elements". Listing them as things the flow tree drops "that those tools still report" sends an author looking for a mismatch that cannot happen. Replace them with the ones that do occur, each demonstrated while reviewing this PR: the Chromium password redaction (the node reaches the runner as `[password]`), the iOS `accessible`-container merge, and the Android clickable-row collapse where the trim borrows the child's text and the flow tree leaves the row with none. Keep the two former items as what they are - projections both sides share. Also state the direction the list omitted entirely: the flow tree walks 12000 nodes to the agent-facing describe's 5000, so it can hold elements `describe` never reached.

It told you a warned step "should stay raw or be re-recorded", then one sentence later that the raw form is for a custom `pollIntervalMs`/ `bundleId` and nothing else. Reading it in order, the second sentence withdraws the first. Give the warned case a remedy that is neither: retarget the directive at something the flow tree carries and prove it by running the flow. Then say plainly what the standing exceptions are, so "keep it raw" is a short list rather than a rule contradicted by its own example.

"The raw tool polls the trimmed `describe` tree" is true on iOS and Android and false on the other two: on Chromium it is the CDP DOM and on Vega the automation toolkit's page source, neither of which is trimmed nor a full hierarchy. This PR's own code comment calls naming `describe` that way on Chromium "the exact steer this warning exists to prevent", and the warning was fixed to avoid it while the skill kept saying it. Name the reader and let the source vary by platform, the same way the indeterminate warning already does.

The rule seven lines below was rewritten in this PR precisely because "only successful steps are recorded" misleads: `await-ui-element` reports an unmet condition by RETURNING `{ success: false }`, so an unmet wait IS recorded. The summary row kept the old wording, and it is the line a reader hits first.

This PR rewrote the rule to "Only steps that returned successfully are recorded". The `run:` paragraph still cites it as "only successful steps are recorded" - a phrase that no longer appears in the file, so the caveat names a rule the reader cannot find.

The unmet-wait warning tells you to delete the step from the file. That works in host mode, where the recorder re-reads the file before each append. Against a remote client the in-memory copy is authoritative mid-recording, so the next append writes the step straight back - and nothing reports the restore. The tool description already carries the caveat for mid-recording edits in general; the warning that instructs one, and the skill's copy of the rule, did not.

The budget comment blamed a 20s `uiautomator dump`. The probe's Android read is `queryAndroidFullHierarchy` → the devtools `getHierarchy` RPC, bounded by `LONG_RPC_TIMEOUT_MS` at 15s; `flow-android-tree` explicitly refuses to degrade to the uiautomator fallback and throws instead, so that dump is never on this path. The 18.9s measurement it quoted cannot have come from the read the ceiling is sizing against.

`echoedReasonOf` was inserted above `warningOf` and landed between `warningOf`'s doc block and its body, leaving two blocks stacked on `echoedReasonOf` and `warningOf` undocumented. The stranded block carries the reason the helper exists — asserting `message` contains "Step added" proves nothing, since that prefix is unconditional — which is exactly the rationale a future edit to `warningOf` needs and would not find there.

"It keeps only views a selector can address and that are actually on screen" was stated as an unqualified property of the flow tree, and it is false on Vega: `projectVegaNode` returns `skip: false` for every node, keeping zero-area scrolled-off nodes deliberately so `exists` accepts them. This is the same overstatement already removed from the runtime warning, whose Vega arm now says the flow tree "drops no element". The per-platform list that follows covers Chromium, iOS and Android only, so scoping the leading clause to those three leaves Vega's absence reading correctly as no cost.

…ds on The determinate warning asserted an `assert:` conversion "WILL fail", then two sentences later named a cause under which it does not: SCREEN_MAY_HAVE_MOVED admits the screen may simply have moved on between the live wait and the re-probe. On that branch the conversion passes — at replay the directive occupies the live wait's position in the sequence, not the moment after it where the probe looked — so a toast that expired mid-probe yields this exact verdict with the two trees in perfect agreement. "Rule that out first" only works as an instruction if ruling it out settles the outcome, and the message had already decided. Vega is where it was most plainly wrong: `treeDivergenceFor`'s Vega arm states that a disagreement there MEANS the screen changed, and the sentence still told that author the conversion would fail. State the consequence per branch and leave the verdict to the platform clause. The `await:` clause, which inherited the same certainty, moves with it.

The conditional consequence said that if the screen simply moved on, both conversions "convert fine". That trades one overclaim for a smaller one: an `assert:` still gets only its short grace, so it can fail at that position for a reason that has nothing to do with the two trees. Say what this verdict actually establishes — that it is no evidence against either conversion — and leave the assert-is-not-a-wait question to where it belongs.

The iOS arm of treeDivergenceFor was reachable by no positive assertion: gutting it to "" — or dropping just its SCREEN_MAY_HAVE_MOVED append — kept the suite at 31/31. The two places its text is named are not.toContain, on the unmet-wait and indeterminate branches, so they pin its absence. That left the screen-moved admission unguarded on the platform where it matters most: the consequence sentence conditions itself on "if the SCREEN simply moved on since the live wait", a cause the platform clause would no longer raise. Assert the divergence sentence, the screen-moved append, and the join to the reader clause — the same anchoring Android and Chromium already have. Both mutations now fail the iOS case.

runnerSideReadClause's block said Chromium's `describe` "returns the FULL DOM the recorder read — a superset that still shows the very nodes the runner drops". treeDivergenceFor's chromium arm quotes that sentence 160 lines down and calls it false: past DEFAULT_WALK_LIMITS' 5000 nodes it is the RECORDER's tree that is short, and `describe` cannot show the element at all, while FLOW_WALK_LIMITS carries the flow tree to 12000. This block is the stated rationale for how the Chromium clause is worded, and that clause was deliberately made two-directional. Left as a one-directional premise it is the argument for collapsing it back. State both directions here too, and name the arm that works the case out.

The description said a disagreement means `message` "carries a warning saying the conversion would fail". Two commits rewrote that sentence: the message now splits the consequence across two causes — the trees really differing, or the screen having simply moved on since the live wait, where the conversion is fine — and the suite asserts it no longer contains "WILL fail". The description did not move with them. Say what the warning is for (read it before converting, the cause decides), and name the third outcome it also has to describe: a tree that could not be read reports UNKNOWN, not known-bad.

"warns in `message` when the check would not survive conversion" was the skill's definition of what a warning on the step means, and it admits only the determinate reading. The probe has a third outcome and emits it as its own warning: an unreadable runner tree compared nothing, so it reports the conversion UNKNOWN, not known-bad — and its reachable triggers (no iOS full-hierarchy service, a failed Android getHierarchy, a read past PROBE_BUDGET_MS) are exactly the ones the skill's injection-free workflow runs into. An author who hits one was being told a warning means the check would not survive, while the warning itself says the answer is unknown. Name both warnings and say they read opposite ways.

The bullet answered every divergence warning with "converting is exactly what would break it: retarget the directive". On Vega the warning says the opposite — the flow tree drops no element there, so a disagreement means the SCREEN changed and the remedy is to re-run the wait, which the suite pins (not.toContain "retarget the DIRECTIVE" / "re-record with a selector", toContain "re-run the wait"). It also outran the other three platforms once the consequence was split per cause: the message now says a screen that moved on is no evidence against the conversion, and this bullet stated the tree-divergence cause as if it were the only one. Send the author to the message: divergence retargets, a moved screen re-runs the wait, an unreadable tree settles nothing either way.

Follow-up to the polish bullet: its closing exception still said "a warned step you haven't retargeted yet", which is the tree-divergence remedy named as if it were the only one. On Vega — and on any platform where the screen simply moved on — the step is settled by re-running the wait, not by retargeting.

hubgan mentioned this pull request Aug 4, 2026

fix(tools): make a schema failure name the parameter that was sent #718

Open

hubgan force-pushed the fix/recorder-cross-tree-probe branch from 9f3b2be to 5ed32f2 Compare August 5, 2026 10:15

hubgan mentioned this pull request Aug 5, 2026

docs(skills): split argent-create-flow into references and add argent-qa-flows #730

Open

hubgan force-pushed the fix/recorder-cross-tree-probe branch from 5ed32f2 to 6053373 Compare August 5, 2026 10:38

hubgan force-pushed the fix/recorder-cross-tree-probe branch from 6053373 to cc79393 Compare August 6, 2026 09:25

hubgan marked this pull request as ready for review August 7, 2026 07:50

hubgan requested review from j-piasecki and latekvo and removed request for latekvo August 7, 2026 07:50

j-piasecki reviewed Aug 7, 2026

View reviewed changes

Comment thread packages/tool-server/src/tools/flows/flow-add-step.ts Outdated

Comment thread packages/skills/skills/argent-create-flow/SKILL.md Outdated

Comment thread packages/tool-server/test/flows/flow-record-cross-tree.test.ts Outdated

hubgan force-pushed the fix/recorder-cross-tree-probe branch from 27503f4 to f888b4b Compare August 7, 2026 08:45

hubgan requested a review from j-piasecki August 7, 2026 10:59

j-piasecki reviewed Aug 7, 2026

View reviewed changes

Comment thread packages/skills/skills/argent-create-flow/SKILL.md Outdated

Comment thread packages/tool-server/src/tools/flows/flow-add-step.ts Outdated

latekvo reviewed Aug 7, 2026

View reviewed changes

Comment thread packages/tool-server/src/tools/flows/flow-add-step.ts

Comment thread packages/tool-server/src/tools/flows/flow-add-step.ts Outdated

Comment thread packages/skills/skills/argent-create-flow/SKILL.md Outdated

hubgan force-pushed the fix/recorder-cross-tree-probe branch from ed77fde to 299e7a5 Compare August 9, 2026 19:45

hubgan added 9 commits August 10, 2026 10:55

hubgan and others added 28 commits August 10, 2026 10:55

docs(flow): the recorder's tree is only the a11y tree on iOS/Android

7c55ee0

hubgan force-pushed the fix/recorder-cross-tree-probe branch from 299e7a5 to 9037b89 Compare August 10, 2026 08:55

Navigation Menu

Uh oh!

fix(flow): re-probe a recorded wait against the tree the runner reads - #717

fix(flow): re-probe a recorded wait against the tree the runner reads#717
hubgan wants to merge 47 commits into
perf/flow-recorder-step-outputfrom
fix/recorder-cross-tree-probe

hubgan commented Aug 4, 2026

Uh oh!

j-piasecki left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

j-piasecki left a comment

Uh oh!

Uh oh!

Uh oh!

latekvo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

hubgan commented Aug 4, 2026

The problem

What changes

Three outcomes, kept distinct

Platform-specific explanation

Uh oh!

j-piasecki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

j-piasecki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

latekvo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants