mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
feat/android-atomic-clear
81 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Hubert Gancarczyk
|
6e862a60b3 |
test(keyboard,flow): pin the arms mutation showed unpinned, and the new warning route
Each of these was written against a mutant that kept the suite green, and each
was re-run with that mutant applied to confirm it now fails.
keyboard-clear:
- the atomic cooldown EXPIRING, at the boundary. Arming and not-arming were
covered; nothing advanced the clock, so widening the constant to a permanent
skip stayed green.
- `WEDGED_RPC_MS` driven through `ping`. Its rationale names both legs and only
the write drove it — and `ping` is the leg a helper that goes silent after
accepting the socket actually spends its time in.
- a helper reporting a protocol NEWER than the gate still takes the atomic path
(`>=`, not `===`).
- the `sized` tie-break: an unmeasurable focused field tying the longest reading
keeps the note's BLIND clause, in the walk order where only that arm decides it.
- the read-back arm outranking `no_focused_input`, and the read-back clause
staying off the paths that never took one.
- a `{{secret:…}}` request on the no-writable-field arm is not told to read the
field back.
- the cross-platform result SHAPE, which a destructure had stopped checking.
flow runner:
- a `keyboard` clear's warning carried out of a composed `flow-execute`, its
repeats collapsed, its depth composed, and a green sub-run left warning-free.
- a tool answering `note: ""` raises no warning.
Two gaps from the review are left open, both reported rather than closed: the
blueprint's `setText` wire call (there is no android-devtools blueprint harness
to hang it on, and the call is exercised end to end against a real protocol-2
helper), and `download-native-binaries.sh`'s gate (no shell-test harness in the
repo; both arms were driven by hand against the live release).
|
||
|
Hubert Gancarczyk
|
e4fbf7c5b7 |
test(keyboard,flow): pin the warning's scope, the no-field note, and two boundaries
Each gap below was found by editing the production line and watching the suites stay green. - Nothing pinned the SCOPE of the note-to-warning promotion. Narrowing it to `keyboard` left all 1226 flow tests green, so a tool returning a healthy `note` could be promoted for good with nothing to object. Cover both directions, at the top level and one level down inside `run-sequence`. - The `run-sequence` spelling of a clear had no test at all, in either direction, nor a case for a nested report whose shape is unrecognised. - `dispatchForResult`'s `?? null` was unpinned: dropping it turns a tool that returned nothing into the skip a hang-up produces, and no test noticed. - The note's wording for `no_focused_input` / `not_editable` was unpinned - only the `why` fragment was asserted - with a contrast case that keeps the plain wording for a refusal. - `WEDGED_RPC_MS` was loosely pinned: both arms had a test, the boundary did not, so widening 2s to 14s stayed green. - `keyboard-android.test.ts` never reset the atomic cooldown, so the first clear to fail left every later test in the file on the injected path by a different route. |
||
|
Hubert Gancarczyk
|
6be9deda58 |
fix(flow): surface a tool step's note as its warning, not only a directive's
`note` is the one signal saying whether an Android clear was the verified kind,
and only the `type` directive turned it into a step warning. A flow that carries
the same clear as a RAW tool step got nothing: `flow-add-step` has no
`keyboard` → `type` rewrite, so a recorded clear replays as a tool step whose
report put the note inside `result` — and `argent-cli`'s own `StepReport` has no
`result` field, so the CLI printed `✓ NN tool keyboard` and nothing else.
`argent flow run` is the runner the QA-flow skill prescribes for CI, so on the
raw form the warning gate was vacuous: the flow completed clean over an
unverified clear with no on-screen signal.
Verified end to end against a Pixel 7 / API 34: a one-step flow
`- tool: keyboard / args: { clear: true, text: abc }` now reports
`"status": "pass"` with the note in `warning`.
|
||
|
Hubert Gancarczyk
|
7df9ed6ef5 | feat(keyboard): clear Android fields atomically | ||
|
Hubert Gancarczyk
|
75e4e04c08 |
docs(flow): stop offering two clears a TV cannot do, and an assert that does not exist
`flow-yaml.md` ended its three-TV-family paragraph with "Delete an existing value with repeated backspaces, or use the field's own clear affordance". The `keyboard` tool rejects `key` outright on a TV target, so repeated backspaces are available on Vega alone; on Apple TV and Android TV the field's own affordance is the only way. Both halves were offered to all three families. The clear-only parse test justified itself with "empty the box, then assert the empty state", but there is no such assert: `equals: ""` and `contains: ""` are rejected at parse time and `matches: '^$'` never fires, because absent or empty text is not a haystack — as `references/asserting-field-values.md` already says in words. The comment now names the reasons a clear-only step is legitimate on its own, and points at that reference for what can actually be asserted afterwards. |
||
|
Hubert Gancarczyk
|
2e508c02d9 |
fix(flow): report a type step cancelled mid-Enter as a skip, not a pass
The post-dispatch abort re-check sits before the submit gate, so nothing stood
between the Enter dispatch and `return { ok: true }`. `dispatchOrAbort`
reclassifies only a dispatch that REJECTS under an aborted signal, and the
Android backend takes no signal at all, so a cancel landing mid-Enter left the
call to resolve and handed back `true`. The same cancellation window therefore
flipped a step between `pass` and `skip` on which of its two keyboard calls
happened to be in flight — and the `pass` arm is the one where the submit really
was sent after the caller gave up, which is what the re-check above exists to
prevent.
Driven through the real runner with a signal-less keyboard mock: `{ text }` with
`submit` defaulting on reported `type:pass` with both dispatches issued, and
reports `type:skip` now. An explicit `submit: true` clear-only step, where the
Enter is likewise the last dispatch, is pinned alongside it.
Also corrects two claims the same block makes:
- the backends that take no signal. Chromium now honours one, so Android is the
only one that takes none — and the two that DO only narrow the window rather
than close it, since each checks between keystrokes.
- the `runType` parenthetical's premise. `scrollIncrement` is not "one call per
step": a `scroll-to` calls it up to MAX_SCROLL_ITERATIONS times through
`scrollToVisible`, which guards the signal at the head of every iteration
itself — so the dispatch is safe to leave bare, but not for the stated reason.
|
||
|
Hubert Gancarczyk
|
d91bf6ee51 |
fix(flow): report a cancelled clear-only step as a skip, not a pass
`dispatchOrAbort` reclassifies only a dispatch that REJECTS while the
signal is aborted, and the Android and Chromium keyboard backends take
no signal at all — so a cancel landing mid-call leaves the call to
resolve and returns `true`. The re-check that catches that lived inside
the `if (step.submit ?? step.text !== undefined)` block, which is false
for a clear-only step, so the two shapes diverged: cancel a run during a
`{ into, clear: true }` step's one dispatch and it reported `pass`, while
the identical cancellation on a step with text reported `skip`.
The check moves out of the submit gate. The submitting path is unchanged
— it still runs before the Enter dispatch — and the clear-only path
gains it.
`flow-abort.test.ts` grows the twin of its existing clear-only case: the
existing one mocks the keyboard to throw, which `if (!sent)` already
caught; this one resolves inside the cancelled window, and asserts the
clear DID go out so it cannot pass by the step never reaching the
keyboard. It fails with `type:pass` before this change.
Two review findings on the same file that are documentation, not
behaviour:
- The `FocusOutcome` docblock named the FOCUS_REPORTING_SOURCES guard as
a producer of "unobservable". No flow can reach it: `type` runs only on
ios, android and chromium, and each hard-codes a source already in the
set. It is kept as a forward guard — a new platform would otherwise
poll the full timeout on every type step — and the docblock now says
which of the two producers is live.
- `focusedFromInside`'s containment conjunct was pinned by nothing: every
fixture pairing an out-of-target focused node with a small enough frame
is refused by the namesake counts first, so dropping `frameWithin`
reddened no test. The new fixture puts a same-`resource-id` sibling
outside the container's box, where both counts hold and containment is
the only thing left to refuse on — the ordinary Android shape, since a
resource-id names the layout slot. Verified by mutation: dropping the
conjunct now fails that test and only that test.
|
||
|
Hubert Gancarczyk
|
da61986dec |
test: pin this round's fixes, and the gaps the review's sweep found
New coverage for each fix, every one of them mutation-checked (revert the fix,
the named test goes red):
- an editing host holding identified or password content beside the named
paragraph refuses, while the bare `<div contenteditable><p>…</p></div>`
still clears;
- the tapped element unmounting while a namesake holds focus refuses, and
the same page with it mounted still clears;
- the three ways a focused node borrowed another element's evidence — a
namesake the tap creates, a container that grows onto a field the tap never
reached, a focused field that renames itself out of the namesake set — plus
the control that keeps the name;
- a slow-but-answering tree source is not reported as an outage;
- the chromium walker keeps an invisible element that holds the caret (and
one that contains it), still prunes when the caret is elsewhere, never
resurrects a display:none subtree, and keeps a box-less focused node.
And the gaps the sweep listed:
- no test parsed a non-object `type` body, so the message could name one of
the two valid shapes and stay green;
- no test covered a non-abort `gesture-tap` rejection in `runType`;
- nothing bounded the focus window from ABOVE;
- the skill docs' `clear` YAML examples were outside the parse guard's slice;
- `carriesFocus`'s recursion fixture put the flag at depth 0, where a
non-recursive check reads the same — give the shadow wrapper an id;
- four weak assertions: `toBeDefined()` on a `findById` that returns null,
an `?.identifier` that reads undefined when nothing resolved, a `toBe`
between two strings that pass as `"" === ""`, and a flat-leaf invariant
omitting `focused`, which is now producible.
Also drops the wall-clock half of the early-exit test: `elapsed < 3000` timed
the whole step — temp dir, device resolution, the settle, two keyboard
dispatches — so it measured the host and failed at 3066ms and 3388ms under
load, while the read count beside it pins the exit exactly at any speed.
|
||
|
Hubert Gancarczyk
|
2b9e7b8de8 |
fix: the LOW findings around the gate, the parser and the walker
- `focusedFromInside`'s trailing `underTap.every(...)` is unreachable as false once the `controls` rule holds, so drop it and state why it is implied. - A `type:` step with no `into` rendered its entry as the bare word `undefined`, which in a multi-step flow does not say which step is broken. Catch it in the type arm and echo the step, the way the `claer` typo path already does. - The two clear refusals blamed an overlay or a sibling for what is routinely a field OVERHANGING the container the selector names — default content-box sizing does it for a few pixels, and neither message admitted the possibility. Reproduced and re-read on Chrome 151. - `chromium.ts` claimed the focus flag reaches every describe consumer: `hasContent` does not list `focused`, so a node whose only signal is focus is dropped from the agent-facing render. Verified — an anonymous empty focused contenteditable is absent, the same editor holding a `<p>` is shown. - It also claimed focus inside an iframe leaves the tree with no flag at all. True in the light DOM; inside an OPEN shadow root the HOST is flagged, since the excluded iframe is the root's activeElement. Verified both ways. - The zero-frame sibling drop now consults `focusedSelf` like the two structural collapses beside it — same class of drop, same consequence. |
||
|
Hubert Gancarczyk
|
7076b5f0f8 |
test: pin every fix in this round, and the gaps the review's sweep found
Each of these was verified by mutation: apply the pre-fix code, the named test
goes red; revert, green. Thirteen mutations, thirteen killed.
New coverage for the fixes:
- a focus read that HANGS past the window rather than throwing (H1), asserting
both the outage refusal and that the step answered while the read was still
outstanding - the bound is what keeps "within 3000ms" true;
- a container with a GAP down the middle, once with two controls and once with
one (H3), so what decides is the number of controls and not the gap;
- a focused wrapper around one anonymous FIELD (M3), the editing host's twin;
- `type: { clear: true }` with no `into` as a validation failure, not a crash (L1);
- a replaced getRootNode still reporting focus (L3), and a page describing in
full with ShadowRoot set to a non-constructor (L5).
And the gaps the review named:
- T1 the enclosing-pad sweep now starts at 1px, where the defect lived;
- T4 an UNIDENTIFIED field's draft hoisting into its container's text and
out-ranking a real control for a tap - the three tests beside it all give
their field a placeholder or an id, so they pinned the shielded case while
reading as proof of the general property;
- T5 the new asserting-field-values reference gets a parser guard: `equals: ""`
and `contains: ""` really are rejected, `matches: "^$"` really does parse;
- T6/T7 the ambiguity fixtures now exercise the gates they name. The anonymous
row put focus on the field the tap MISSED, which later gates refuse on their
own, so both arms passed with the ambiguity gate deleted; it now differs in
the colliding name alone, and the same-id re-layout gets the tree order where
"first match wins" would hand back the focused row.
|
||
|
Hubert Gancarczyk
|
3385468cbd |
fix(flow): size the containment test exactly, so no pad hides inside its slack
frameNoLargerThan carried the same per-dimension slack frameWithin applies per edge, so a focused node padded symmetrically around the target by half the threshold satisfied both and took the clear. On a 900px window that is a trap one pixel larger on every edge: reproduced on Chrome 151, where a focusin trap holding DRAFT-do-not-erase over an <input> passed the step, was emptied and rewritten with the new address, and left the named input holding its old value. There is no tolerance that fixes it. A trap enclosing the field by 1px and a child overhanging its wrapper by 1px are the same geometry, and the flow trees carry no ancestry to tell them apart - flattenHoisting returns leaves under one root on every platform. So the size test is exact: equal frames still confirm (the everyday wrapper-around-input shape, verified live), and any overhang refuses, which fails the step loudly instead of emptying a field the step never named. The overhang sweep moves with the contract: pad 0 is now the admitting case and 1-3px refuse, where the 1-2px arm previously pinned the tolerance this defect lived in. |
||
|
Hubert Gancarczyk
|
2b77ccf42a |
test: close the gaps the review's mutation sweep left open
Every pin below was mutation-checked: the pre-fix code was restored in `flow-actions.ts` / `chromium.ts` and each named test went red. - `sameElement`'s LABEL fallback had no coverage, and it is the whole name-is-not-an-identity defect. Every fixture in the runner file gives its nodes a resource-id, so the anonymous path — the everyday React Native and plain-DOM shape — never ran. `anonymousRowXml` runs it both ways: a namesake and a differently named sibling. - `trackTarget`'s proximity tie-break had none either. `twoSameIdRowsXml` is the re-layout that made it adopt the wrong namesake. - `focusedOwnsTargetText` needed the trap whose OWN text equals the target's; the existing editor fixture only ever exercised the arm that should confirm. - `requireEvidence`'s safety half was unpinned AND the test written for it did not reach it: `overlayFocusXml` carries no `email-wrapper`, so every overlay read was consumed by selector resolution before the tap went out and the focus poll's first read already confirmed. `overlayOverWrapperXml` carries the wrapper, so the clear really has to poll past an overlapping read. - The textarea change had no coverage past the walker itself. Four cases now run the real walker THROUGH the real flow adapter and matcher: the container hoist, the page-wide `visible` check, the tap ranking, and the clear-only proof that still has to work on an unlabelled composer — plus an `<input>`-vs-`<textarea>` spelling case. - `describe-chromium-script.test.ts` certified behaviour that cannot happen: it asserted focus inside a same-origin iframe is carried by the inner element. The walker never descends — `el instanceof Element` rejects the inner `documentElement`, whose constructor belongs to the OUTER realm — and the mock passed only because it is single-realm. Verified live on Chrome 151: an `<iframe srcdoc>` with a unique marker leaves no trace in describe, and focusing the inner input leaves the tree with NO focus flag at all. That is the "unobservable" state a destructive clear dispatches on, so the test now pins it as such. - The refusal tests ran at ~75% of vitest's 5000ms default (measured 3757-3760ms) on the one file whose names would make a load-induced timeout look like a real destructive-clear regression. They get their own testTimeout. |
||
|
Hubert Gancarczyk
|
c46eb09844 |
fix(flow): bound the focus wait's dark tail, instead of failing open on it
`lastRead` is only ever written by a read that SUCCEEDED, so the "unreadable" outcome fired only when EVERY read in the window threw. One successful poll disarmed it permanently — including the first, which is the one most likely to report "no focus yet", because the app's focus round-trip has not finished. That maps to "unobservable", the one non-confirmed outcome `runType` deliberately lets a destructive clear through on. A transient tree-source outage in the tail of the window therefore converted a correct refusal into a blind clear. Reproduced on Chrome 151 against this branch's tool-server: nothing focused at the first poll, the tree read poisoned from t+600ms, focus landing on `#other` at t+900ms where no read could see it. The step passed and wrote `WIPED` into `#other`, a field it never named; the same page with the reads left alone refused, and so did the same page poisoned before the first read. The file already states the rule 700 lines up — `CONDITION_DARK_TAIL_TOLERANCE_MS`, "a verdict narrated from the reads before the darkness would describe a screen nobody saw at the deadline". This applies it to the one place where that stale verdict authorises a destructive action, counted in consecutive failed polls rather than in milliseconds: one poll costs a whole `uiautomator dump` on Android, so an elapsed-time budget would read a slow device as an outage. One failure is still the last-poll blip the tolerance exists to absorb. The refusal now says the tree stopped answering rather than that it could not be read at all, which was only ever true of the every-read-failed case. |
||
|
Hubert Gancarczyk
|
9743fbfcaf |
fix(flow): stop a read that cannot SEE focus erasing one that found it
The focus wait classified an empty `focused[]` as the observation "no-focus", which `giveUp` maps to "unobservable" — the one non-confirmed outcome `runType` lets a destructive clear through on. `lastRead` is latest-read-wins with no floor, so a single read in which the focused element's leaf is simply MISSING erased everything the earlier reads had established. Nothing throws; the tree is complete and still holds the target. Both flow adapters produce exactly that for a frame clipping to zero area, which is what keyboard avoidance does to a field it scrolls away, and on Android what a healthy `uiautomator` dump reports for a focused view whose bounds have collapsed. Reproduced on Chrome 151 against this branch's tool-server: `#other` holds the caret and never loses it; the tap on the target does not move focus; collapsing `#other`'s box 1500ms into the wait took the step from "focus never reached" to a pass that emptied `#other` and wrote `WIPED` into it — with `#other` still `document.activeElement` at the end. The same page without the collapse refuses. So a read that saw nothing no longer overwrites one that saw where focus is. Latest-read-wins still decides between sightings, which is what keeps a blur on the focusing tap from being answered with history, and a window in which NO read ever saw focus is still "unobservable" — verified live that a page with nothing focused still dispatches the clear, which is what keeps the directive working on an iOS build whose injected framework omits `firstResponder`. The residual `runType` documents narrows accordingly: an app that blurs, or a field that scrolled away, now only reads as unobservable when it happened before the first read. |
||
|
Hubert Gancarczyk
|
591c8ea4fd |
docs: correct nine claims this PR's own code or platforms disprove
Comments and skill pages that describe something other than what happens. No
behaviour change.
- `live-authoring.md` told an agent to KEEP a recorded `tool: keyboard` clear
raw, on the false premise that `type:` accepts only `into`/`text`/`submit`
(`flow-utils.ts` allowlists `clear`) and pointing at a table exception this PR
deleted. It is the raw form that is unsafe: it replays with no focus tap and
no focus check. Converting it is the whole mitigation, and that paragraph
said not to.
- The same page claimed an unconfirmed poll falls through to typing. It does
for a plain `type`; a `clear` hard-fails, which is what `flow-yaml.md`
already says.
- `dispatchOrAbort`'s docstring said `keyboard` discards the `ToolContext`. All
three platforms `clear` supports forward the signal and check it between
keypresses, so an abort-driven rejection is the designed common case there —
which is what makes the wrapper load-bearing rather than defensive.
- The `requireEvidence` fast path was justified by uiautomator flagging a
WebView instead of the EditText inside it. Surveyed across eight screens on
API 36 / WebView 151, including a real in-app WebView, the WebView is
`focused="false"` and the leaf EditText carries the flag. The exit stays; its
stated reason does not.
- `FOCUS_REPORTING_SOURCES` said a source outside the set never reports focus,
naming Vega. Vega's source-parser does set it and `flow-vega-tree` preserves
it; it is excluded because `runDirective` refuses `type` there first.
- The Android TV comment had a `clear` step failing at the first keyboard
dispatch, which presupposes the focus gate passed. `android-devtools` is in
the reporting set, so the wait reports "unconfirmed" and the refusal returns
before any dispatch — as `flow-yaml.md` says.
- `flow-run.ts` cited `(down)` as a sibling option echo twenty lines above the
branch that suppresses exactly that value.
- `asserting-field-values.md` described the textarea case as either/or, gave an
over-broad rule for when an `<input>` exposes its value (`accessibleName`
gates on truthy/resolving, so `aria-label=""` still falls through), and
recommended `assert: { hidden: … }` as working on every platform when it
passes vacuously on exactly the platforms where the contents never reach the
tree. It now also states the secret exposure a `<textarea>` carries.
- `flow-keyboard-clear.test.ts`'s header still gave the allowlist as the reason
the round trip is worth pinning. The round trip is; that reason is not.
|
||
|
Hubert Gancarczyk
|
d1e28a412d |
test(flow): pin the shapes the static fixtures could not express
Every focus fixture held the tree constant across polls and split its rows unevenly, which is what let five defects through a suite that was otherwise green. Each test below fails against the code as it stood one commit before its fix, and each ships with the control that keeps the fix from being a blanket refusal. - an EVENLY split row whose focused half is not the one the tap hit refuses, and the same row with focus on the half the tap DID hit still clears; - a focused input overhanging its wrapper by 1-2px still clears, and 3px — six pixels of extra width on a 1080px screen — refuses, which pins both sides of the containment epsilon instead of only the far one; - an overlay appearing over the tap point AFTER the tap refuses, and the same overlay already there at tap time still clears; - a named container that GROWS between the tap and the poll still clears; - a selector that re-resolves to a better-ranked node after the tap still clears; - a rich-text composer whose only text sits in a child node clears, and the same editor holding a second paragraph refuses; - a `<textarea>` holding a newline reports its contents once, not once raw and once normalized. Also pinned, all three of which a mutation survived: a `clear` keeps polling past an intermediate overlap read instead of refusing on it (the safety half of `requireEvidence`, whose perf half was already covered), the focus flag is suppressed on a shadow host when the flag sits NESTED inside its subtree rather than one level down, and no refusal reason quotes the focused element's own text — the invariant that keeps a field's value out of a report on disk, until now asserted only in a comment. |
||
|
Hubert Gancarczyk
|
fd4aa70e56 |
docs(test): drop the append model from the two tests still teaching it
`1ab12bd2` corrected "typing appends" everywhere in this branch except here. The test NAME - which is what shows up in suite output, so it is read outside the file - said "instead of silently appending", and its comment "an append, which fails much later at an assert". A third copy in `flow-tools.test.ts` glossed `←` as "(append)" while documenting a production comment that says the opposite: `flow-finish-recording.ts` renders it as "types into whatever is already there - NOT necessarily appending". The measured behaviour is per-platform: on Android and Chromium the focusing tap lands the caret mid-field and the new text is spliced INTO the old value; only on iOS does the caret jump to the end and append. |
||
|
Hubert Gancarczyk
|
7af7161e14 |
fix(flow): stop the containment slack admitting an ENCLOSING focused node
`frameWithin`'s epsilon is per-EDGE, so a frame overhanging the target by under the tolerance on EVERY side satisfies "sits inside it" while being a full epsilon larger in each dimension - which is the `encloses` shape the same file refuses as "not evidence", and gives `clearRefusalReason` its own arm to refuse. A focused `android.webkit.WebView` clearing its target by 4px on a 1080x1920 screen passed containment and took the clear, with the step reporting a pass on the field it never touched. The confirm arm now also compares EXTENTS: a node that stands for the target is not bigger than it. That catches a symmetric pad at half the per-edge threshold while still admitting the overhang the epsilon exists for (a child a border's width wider than the box it sits in). The docblock's rationale was wrong too - it described absorbing disagreement between "three different adapters", but both frames come out of one tree read by one adapter, so there is nothing cross-adapter to absorb. It now gives the reason that applies, matching the identical `WITHIN_EPS` in `ui-tree-match` and `EDGE_EPS` in this file. Pinned at pad 4 / 5 / 12 / 100: the guard's removal fails 4 and 5, and widening FRAME_CONTAINMENT_EPSILON to 0.05 fails 4, 5 and 12 - the value was previously unpinned in either direction. |
||
|
Hubert Gancarczyk
|
0d7a50d457 |
perf(flow): stop a plain type polling for a verdict it never reads
Only "confirmed" returned early, and only a `clear` reads the outcome - so a plain `type` polled to the full TYPE_FOCUS_TIMEOUT_MS on every shape that base returned from immediately. Base ended the wait as soon as ANY focus-flagged node overlapped the target, and the shape that lost it is the ordinary hybrid one: uiautomator puts focused="true" on the enclosing `android.webkit.WebView`, not on the `EditText` inside it, and an enclosing node can no longer confirm. A WebView-hosted form of n fields paid n x 3s on the path with no clear at all. `waitForFocus` now takes `requireEvidence`. A caller that will not act on the verdict stops at the first read that settles it; a `clear` polls on, unchanged. Pinned by a test that counts tree reads (3, not ~13) and asserts the step finishes inside the focus budget - it fails at 3773ms with the early exit removed. On Chrome 151 a plain type into a field under a focused-form shape runs in 1.08s. |
||
|
Hubert Gancarczyk
|
2feb70f123 |
fix(flow): confirm a clear from the input the tap hit, not any inside the box
The containment arm accepted ANY focus-flagged node inside the target,
with no check that it was the node under the point `runType` just tapped.
Its only discriminator was a role test on the TARGET - "skip this arm when
the target is itself a text input" - and that vocabulary misses in both
directions against the roles the adapters really emit:
- over-match, so a legitimate wrapper clear is REFUSED. Material's
`TextInputLayout` is the non-editable wrapper carrying the app's
resource-id, and `deriveUiAutomatorRole` matches "textinput" on the
short class name, so it derives `TextField`; ARIA 1.1 puts `combobox`
on the wrapper around the input, which is in the anchored group.
- under-match, so an overlay is ADMITTED. A `contenteditable` composer
is role `div` and React Native's iOS host views come through as
`AXStaticText`, so the arm fires for exactly the shape it was scoped
to exclude.
And on a container holding more than one input - a currency/amount row,
an OTP row, a card number/expiry/cvc row - there was no discriminator at
all: the step cleared whichever sibling reported focus and passed on the
container.
The arm now asks whether the focused node covers where the tap LANDS (the
centre of the target's current frame, recomputed per round so keyboard
avoidance cannot stale it). Structure would be sharper, but both the
Android and the iOS full-hierarchy adapters flatten to leaves under one
root, so a wrapper and its input arrive as siblings and no ancestry is
available. `TEXT_INPUT_ROLE` / `isTextInput` are gone.
Reproduced on Chrome 151 through the branch tool-server, all four before
and after:
composer + focused #mention overlay -> was pass, #mention overwritten
-> now fail, both intact
#amount-row over currency + amount -> was pass, #currency overwritten
-> now fail, both intact
role=combobox wrapper -> was fail ("a focus trap")
-> now pass, replaced
plain div wrapper (control) -> pass either way
|
||
|
Hubert Gancarczyk
|
24d8b73d49 |
fix(flow): resolve a type step's two halves with one ranked resolver
`flowSelectorToNode` picked its node with a bare `firstInReadingOrder`
while `flowSelectorToFrame` went through `selectorToFrame`, which RANKS
matches (exact field hits first, then smallest area). The two feed
opposite halves of one `type` step: the frame is what gets TAPPED, the
node is what the focus check judges identity against.
On the everyday label-above-input shape they disagree. A `{ text: Email }`
selector matches the label as a substring and the field exactly, so the
tap landed on the field while the identity check resolved to the label -
and could then never match. A `clear` hard-failed with "focus never
reached ...", sending the author after a selector that was already
resolving to the input; a plain `type` burned the whole focus timeout on
every such step.
Reproduced on Chrome 151 through the branch tool-server, `<div id=hdr>
Search results</div>` above `<input id=q aria-label=Search value="OLD
QUERY">`: the no-clear control typed into `#q` and passed in 4.8s, the
same selector with `clear: true` failed and left `#q` untouched. After
the fix the clear passes and `#q` reads "REPLACED", and the control drops
to 1.3s.
`selectorToFrame` is now `selectorToNode(...)?.frame`, so there is one
ranking to keep in step instead of two.
|
||
|
Hubert Gancarczyk
|
035afe4bb4 |
fix(flow): judge focus by identity, and flag the element that really holds it
Review drove two more shapes through the branch build against Chrome 150,
in opposite directions.
A focused node INSIDE the target's frame was accepted as proof, so a
suggestion popover overlaying a composer had its own field cleared and
overwritten while the step reported a pass on the composer it never
touched (`#body` byte-identical, `#mention` replaced, 3/3). Only the
enclosing direction was guarded.
And with focus inside an open shadow root `document.activeElement` is the
HOST, so the describe walker flagged the host - which contains the inner
input by construction. Naming the shadow's own input therefore failed
with "the only element reporting focus CONTAINS it", advice the flow was
already following, while the `keyboard` tool it was about to call handles
that shape correctly. Naming the host instead passed and cleared a
different child.
So the walker now flags the activeElement of the element's own ROOT, not
its document, and excludes a host that delegates into its shadow root -
one flag, on the element holding the keys. And the focus wait resolves
the target NODE, not just its frame: identity is confirmation, while
geometry is accepted only for a target that is not itself a text input,
which is what separates "the input inside the testID container I named"
from "an overlay sitting on the field I named". The two now behave:
`into: { id: sinput }` clears the shadow input and leaves its sibling
alone; the popover case fails and touches nothing; the focus trap and the
ordinary replace are unchanged.
An overlap that is neither is its own outcome with its own advice, rather
than being told it hit a shadow host.
Also here: the committed merge conflict in `argent-create-flow` (prettier
had rewritten the markers into a list bullet and a blockquote, so neither
git nor the formatter saw it, and the two surviving copies gave opposite
instructions about carrying a recorded Enter across); the TV rejection
reason, which stated a transport limit that does not hold for Android TV;
`runPinch` described as a single dispatch when it chains several; and the
per-platform assert table moved to `references/`, which is where the
skill guide puts reference material an agent consults rather than reads
every time.
Three invariants a mutation sweep found unpinned now have a test each,
each verified by re-applying the mutation: the `<textarea>` own-text
guard (which shipped with no test), the per-round target-frame
re-resolve, and the overlay-vs-wrapper distinction.
|
||
|
Hubert Gancarczyk
|
ab7f3412b4 |
docs: correct the type directive's docs where the code stopped matching them
`(cleared)` in the recording summary is past tense for something that has
not happened: no step the recorder produces is a `type` — `flow-add-step`
emits only tap/launch/run/tool and the only `kind: "type"` construction in
`src/` is the YAML parser — so every `type` in a summary arrived by a hand
edit and did not run live. It reads `(clear only)` now, matching the run
report's present-tense `(clear first)`.
`type needs { into, text }` names one of two valid shapes since `clear`
made `text` optional, and it is the message an author sees after
extrapolating the bare-scalar form the other directives accept.
"`submit` serializes only the opt-out" stopped being true two lines below
itself, where a clear-only step serializes `submit: true`.
`dispatchOrAbort`'s docblock described a mechanism that does not exist —
cancelling a run does not tear down the transport under an in-flight call
— which made its real trigger look far broader than it is. It now says
what each of its two callers actually needs it for.
The per-platform caret split omitted Chromium in three places, which
matters because "appended" is the half that leads to leaving `clear` out;
re-measured, Chromium behaves like Android. The Android assert bullet
gained the password exception its Chromium sibling already carried — both
adapters redact identically, so `equals` on the typed value is false
either way. And `flow-ios-tree`'s "leaves the poll unconfirmed" named the
value that now refuses a clear, where the case it describes is the one
that must NOT.
|
||
|
Hubert Gancarczyk
|
aafd9899bc |
fix(flow): require focus INSIDE the target before a clear, not merely overlapping it
The overlap test confirmed a clear by construction whenever a
focus-flagged node was large enough to cover the target, and every shape
that produces one can hide a different element holding the keys.
Reproduced on a live Chromium page through this branch's tool-server, an
ordinary `focusin` focus trap bouncing focus back to a `<textarea>` whose
box contains the target input:
before: #notes = "DRAFT do not erase me" #email = "old.remembered.login"
- type: { into: { id: email }, text: "new@example.com", clear: true }
-> pass, "into id=email (clear first)"
after : #notes = "new@example.com" #email = "old.remembered.login"
The user's draft is cleared and overwritten, the named target is never
touched, and the step reports a pass. The same shape occurs with an open
shadow root (`document.activeElement` is the host, never the inner
element) and with a hybrid app's focused WebView.
Confirmation now requires a focus-flagged node INSIDE the target's frame,
which keeps the case the overlap test was written for — a testID wrapper
whose input reports the focus. A node that merely encloses the target is
a new outcome of its own, refused with the reason that fits it. The same
run against the same page now fails with "the only element reporting
focus CONTAINS id=email rather than sitting inside it" and leaves both
fields alone; an ordinary field replace still passes.
A window in which every tree read threw is also no longer treated as
"nothing was focused". Nothing was OBSERVED, which is not evidence — the
convention `settleTree` already applies to the same condition — so it
refuses rather than clearing on it.
Also uniform now: all three of the step's device calls classify a
cancelled run the same way. The focus tap was bare while the two keyboard
dispatches were wrapped, so one step reported `error` or `skip` depending
on which dispatch was in flight when the caller gave up.
Four guards this stack added were removable with the whole flow suite
still green — the pre-Enter abort re-check, the clear/text dispatch's
return, the Enter dispatch's wrapper, and now the tap's. Each has a test
that fails when it is taken out.
|
||
|
Hubert Gancarczyk
|
b7d68d767f |
fix(flow): judge focus by the latest read, not by any read that ever saw it
The focus outcome accumulated "saw focus on something" across every poll round, which made the verdict a race. An app that blurs when the focusing tap lands reports the PREVIOUS field as focused for however many rounds precede the blur, so the same flow against the same app refused the clear or passed it depending on whether round 1 beat the blur — one being a hard step failure and the other a silent no-op. The question the clear is about to act on is whether something else holds focus NOW, so the outcome now reflects the most recent successful read. An app that blurs lands in the documented residual deterministically, and a target that never takes focus while another field keeps it is still refused. A window in which every read throws still reports "unobservable" rather than claiming focus was seen, and the type's doc no longer says that bucket means only "the tree cannot report focus" — it also covers "the tree could not be read". Also records, where the decision is made, that the overlap test confirms by construction against a focus-flagged node large enough to cover the target (an Android host WebView, an iOS first-responder container, a Chromium focus-trap div). On Chromium that is benign — one activeElement per document, so a focused container means no input holds focus and the clear no-ops. The destructive shape needs two focused nodes at once, which only the Android/iOS adapters can emit; it has not been observed on a real device, so it is documented rather than guessed at. |
||
|
Hubert Gancarczyk
|
2b2ef6ea9b |
fix(flow): pin the abort/focus arms review found unpinned, and cut the tap wrap
Second review pass, all four items proven by mutation rather than reading. Two arms of the new code were asserted by nothing. `dispatchOrAbort` returning false on ANY error — turning every backend rejection into a silent "run aborted" skip — passed all 3131 tests; `runRotate` already carried that positive control, `runType` did not, so it has one now. Likewise the focus-wait's source gate: mapping a non-focus-reporting source to "unconfirmed" instead of "unobservable" passed everything, while refusing `clear` on every target behind such a source — the same platform-wide outage the outcome split exists to prevent. Both mutations now fail. The `gesture-tap` wrap is reverted. It was not needed by any of the fixes, nothing asserted it, and it singled the focus tap out for a guard its five siblings (runTap, runLongPress, scrollIncrement, runPinch) do not have. `runRotate` now shares `dispatchOrAbort` rather than keeping the inline copy the helper was written from, and the helper's doc no longer claims `runLaunch` as a precedent: `runLaunch` returns a failure outcome instead of rethrowing, so it deliberately is not a caller. The clear-only cancellation test keeps its case but drops the rationale about reaching the dispatch "by a different branch" — after the collapse both shapes take the same one. Per-platform assert guidance corrected again: on Chromium an assert on the typed value hard-FAILS like iOS (it reads the accessible name); it is the label-matching assert that passes vacuously. The name also falls back through aria-labelledby, skips password fields, and covers `<textarea>`, and Android only pairs hint with value when a contentDescription is present — without one, most React Native `TextInput`s report the contents alone and `equals` is correct. |
||
|
Hubert Gancarczyk
|
bf28bdef56 |
fix(flow): make clear atomic with the text, and never clear blind
Review of #581 surfaced two ways the `type` directive's `clear` could destroy data it was meant to replace. Both reproduced on a Pixel 3a against Bluesky, and both re-verified fixed on the same device. Clear and text now ride ONE `keyboard` call. Every backend validates the whole request before touching the device precisely so a rejected call leaves no trace — `assertTypeableAndroidText`, and the per-character resolves in the chromium and simulator-server backends, all run ahead of the clear. Issuing them separately stepped outside that guarantee: `{ into: field, text: "José", clear: true }` cleared, then rejected the text, leaving the field EMPTY where the identical args as a single call left it holding its original value. Enter stays a separate call — the `typeTv` argument for splitting applies to `key`, which it rejects outright, and not to `clear`, which it rejects just as early. `waitForFocus` now reports its outcome instead of discarding it, and a clear whose focus landed elsewhere fails the step rather than emptying whatever else holds it — previously reported as a pass, labelled as a clear, on a field it never touched. The outcome keys off whether any node in the tree reported focus, not off the source: an iOS build whose injected framework omits `firstResponder` reports none at all, and conflating that with "focus is elsewhere" refused every clear on the platform. `runType` also reclassifies a dispatch that rejects mid-cancellation as an aborted skip, matching `runRotate` and `runLaunch`; a cancelled run no longer surfaces a raw backend message as a step failure. Docs: `type` does not "append" — the focusing tap lands the caret mid field, so on Android the new text is spliced INTO the old value (forty `A`s typed with `ZZZ` gives `AAA…ZZZ…AAA`), while on iOS it appends. The suggested `assert` on a field's text does not work everywhere either: iOS never exposes a field's contents to the flow tree, and a Chromium `<input>` only does so without an aria-label or placeholder. Both stated per-platform now, with the Vega rejection attributed to the directive gate that actually performs it, and the recording-cleanup rule carrying a recorded Enter across as `submit: true`. The run report says `(clear first)` rather than `(cleared first)`: the label is stamped before the step runs and rides every outcome, so the past tense claimed a clear on steps that failed, errored or were skipped. |
||
|
Hubert Gancarczyk
|
00774162d8 |
fix(flow): drop the post-clear read-back check; it was blind or wrong
The `type` directive verified a `clear` by re-reading the focused node and
failing the step when it still held text. Review plus device probes showed the
check is blind exactly where it matters and wrong where it fires, so it is
removed rather than patched.
Blind:
- iOS never carries a value. `flow-ios-tree` projects leaves as
{role, frame, children, label, identifier, focused} and does not even request
a text field from `getFullHierarchy` — the check was dead on the platform.
- A Chromium `<input>` has no value either: the DOM walker fills `value` from
the element's own child text nodes, and an input has none.
- On Android a field with no contentDescription puts its contents in `label`
(`labelOf` falls back to `text`), so no value again — i.e. most React Native
TextInputs.
Wrong where it fires:
- An emptied Android field WITH a contentDescription reports its hint as the
value, so a successful clear reads as leftover text and fails the step.
`<TextInput accessibilityLabel="Email" placeholder="you@example.com"/>` is
the ordinary shape that hits this.
- A Chromium `<textarea>` exposes its default content once `el.value` empties,
producing the same false failure only after the clear succeeds.
Demonstrating both halves: on an API 30 emulator whose clear genuinely failed
("wifi" -> "wif"), the check returned undefined and passed the step — the exact
append it existed to prevent — while the shapes above would have failed steps
that worked.
The silent-no-op risk lives at the tool layer, which now rejects `clear`
outright on every backend that cannot perform it. Flows wanting proof should
`assert` the field's value, which works on every platform. The skill docs said
the step "fails immediately rather than appending"; that claim is removed and
replaced with the assert guidance, since leaving it would stop an agent adding
the check that actually works.
Removing the read-back also removes its abort-guard gap (a run cancelled
between the clear and the read was reported as a step failure rather than a
skip) and its unsettled single-read race.
Tests: dropped the verification cases (one of which was vacuous — the android
adapter strips `value` from password nodes, so the password guard was
unreachable), pinned that a clear adds no extra tree read, and added coverage
for the run-report label and the recording summary, including a clear-only step
that previously rendered as `<- "undefined"` when the summary regressed.
Docs: corrected the platform scope on the argent-device-interact capability row
(Chromium was missing), softened "one device round trip" to "a single tool
call", noted that a clear-only `type` sends no Enter, dropped the API-30
framing now that the Android guard probes the device rather than a version, and
updated the argent-test-ui-flow login example, which still demonstrated the
bare-append pattern the other skills now warn against. `flow-execute`'s
description also documents `type`'s options for the first time.
|
||
|
Hubert Gancarczyk
|
37515fc610 |
feat(flow): clear on the type directive
A flow that replays against a non-empty field types on top of the old value.
It does not fail at that step — it fails later at an `assert`, pointing at the
wrong place. `type` can now empty the field first:
- type:
into: { text: Email }
text: "new@example.com"
clear: true
Threaded through the step union, the YAML shape, parse, serialize, the runner,
the run-report label and the recording summary.
Two semantics decisions:
- `text` becomes optional when `clear` is set. `type: { into: search, clear:
true }` — empty the box, then assert the empty state — is a legitimate step.
A missing/empty `text` without `clear` still rejects as before.
- `submit` defaults to false when there is no text. It defaults to true
otherwise, and is stored (and serialized) only when it differs from that
default, so YAML round-trips unchanged. Firing Enter into a field the step
just emptied would run an empty search or submit a blank form.
`clear` joins the `rejectUnknownKeys` allowlist, so a misspelled `claer` fails
loudly instead of silently reverting the step to an append.
The clear is dispatched as its own `keyboard` call, before the text. It is NOT
folded into a combined `{ clear, text, key }` call even though the tool now
supports that ordering: `typeTv` rejects `key` outright and does so before it
types, so a combined call on an Apple TV / Android TV target would throw with
the field still empty.
`runType` already holds a settled tree, so it verifies the clear landed and
fails the step when the field is still populated — a flow that silently fails
to clear is far worse than one that fails loudly in the right place. The check
reads the focused node's `value` only: a field's placeholder surfaces as its
LABEL (Android exposes the hint that way), and an emptied field reports no
`value` at all, so treating label text as content failed every successful
clear — observed on a real device before the check was narrowed. Password
fields are skipped outright, since uiautomator reports their text as empty and
"looks empty" there is a false pass rather than evidence.
Skill docs: the `type` row and `submit` paragraph in argent-create-flow, the
keyboard section and capability row in argent-device-interact (including the
"use triple-tap to select all" line, which steered agents at the workaround
this replaces), and the Vega/TV rejection in argent-tv-interact.
The load-bearing one is argent-create-flow's recording-cleanup rule: it now
carries `clear` from a recorded `tool: keyboard` step into the `type:`
directive. Without that, the polish step drops it and the cleaned flow reverts
to appending — a docs omission that produces a data bug.
|
||
|
Hubert Gancarczyk
|
b86862876c |
fix(tool-server): declare flow-add-step long-running, so recording a slow step is not retried
`flow-add-step` dispatches an arbitrary tool by name, so its worst case
is the worst case of whatever it wraps — and several of those are
`longRunning` themselves. This branch raises `keyboard`'s worst case to
~56s (clear 26s + text 15s + key 15s) and declares the flag for exactly
that reason, and the recorder that wraps it is how a clear gets into a
flow at all: the skill copy instructs authors to record the step raw
rather than fold it into `type:`.
Without the flag the MCP adapter applies its 30s FETCH_TIMEOUT_MS to the
RECORDING of such a step and, on abort, re-POSTs the identical body up to
MAX_RETRIES more times. None of that work is cancellable, so each retry
is a fresh device action AND a fresh appended step.
Reproduced through the REAL MCP adapter against a real tool-server,
recording one `await-ui-element` with `timeoutMs: 40000` (a deterministic
>30s step on this hardware, the same mechanism a >30s `keyboard` takes):
before: 153s, "This operation was aborted" — so the agent concludes
nothing was recorded — and FIVE identical steps in the YAML,
which the replay then performs five times.
after: 40s, no error, ONE step.
The flag itself pre-dates this branch; what is new is that a >30s
recorded step is reachable through it.
|
||
|
Hubert Gancarczyk
|
d254a50069 |
test(tool-server): pin the parser fix's effect on a saved flow, and stop a test
claiming a guarantee it cannot give `flows/flow-android-tree.ts` shares `parseUiAutomatorXml`, so reading a single-quoted attribute correctly changes the verdict of a saved `assert` / `await` step that was written against the field reading blank. That direction — the one that alters an existing flow's outcome — had no test. And the `commands` payload test claimed to pin the `if (event.commands !== undefined)` guard. It cannot: the payload is sent through `JSON.stringify`, which drops an `undefined`-valued key, so `payload.commands = undefined` and "no key at all" are byte-identical to the fake CDP server — as they are for every sibling guard in that builder. Renamed and re-commented to say what it does pin (the wire shape) and why the guard itself is not observable from there. |
||
|
Hubert Gancarczyk
|
aebbae1b91 |
test(tool-server): exercise clear through flow record and replay
`clear` was covered at the tool level only. The flow path is where losing it is silent: a recorded `tool: keyboard` step rewritten as a `type:` directive — which the polish table orders, and which accepts only `into`/`text`/`submit` — replays as a plain APPEND into the pre-filled field, and the run still reports PASS. So the property pinned here is the round trip: `clear` survives the recorder (which strips the device id but must keep everything else), and it reaches the tool again on replay. Confirmed by mutation: dropping `clear` alongside the device keys in `stripDeviceKeys` turns both red. |
||
|
Jakub Piasecki
|
568660509c |
fix(flow): floor every settle at two read attempts (#780)
`settleTree` bounded its polling by a 3s window but never bounded the number of reads it took, and a tree read can fail by TIMING OUT. Every tree source's RPC timeout outlasts that window, the shortest being iOS's 5s, so the window fit exactly one such read: "every read in the window failed", the condition the throw reports as a tree-source outage, collapsed into "the first read was slow", erroring a tap/type/scroll step on one transient blip with no retry at all. The floor makes the deadline stop deciding how much evidence an outage verdict is drawn from. `await`/`assert` have never had this flaw, since `waitForCondition`'s final poll already guarantees a second attempt, so this is the same guarantee given to the directives that resolve through a settle. A run whose reads answer inside the window pays nothing: two reads are what a settle already costs there. A run whose reads outlast it pays a second read, and gets a settle it never actually got before, since past the deadline the old code returned its single tree having compared it against nothing. A wedged source costs two full reads rather than one, and a read is several RPCs deep with each hop on its own timeout, so that is tens of seconds on a step that used to fail in one read. The alternative is the step failing on evidence the window was too short to gather. |
||
|
Jakub Piasecki
|
8062da77a5 |
fix(flow): settle before gestures that resolve no selector (#779)
A selector target settles inside `waitForFrame`, but a target that resolves nothing skipped the settle altogether: a coordinate `tap`/`long-press` and a centre-anchored `pinch`/`rotate` dispatched into whatever motion was in flight, so they could land mid-fling, which is the exact race the settle exists to close. Every input directive now settles first, however it is addressed. The settle is best-effort here, unlike on the selector path: these gestures read no frame out of the tree, so an outage must not fail them, which would break the escape hatch coordinates exist to be (an element the tree cannot see). This is the allowance `snapshot` already makes before a capture that reads pixels rather than nodes. Best-effort is not free, though, and the run pays it per gesture. A tree the run can never read is not only the disconnect mid-run: `fetchFlowTree` serves no tree at all on `ios-remote`, and an app that cannot load the instrumentation fails every read off an in-memory list, an Apple system app being the case in point, which flows drive by coordinates for exactly that reason, so every step of such a flow is one of these gestures. Measured against the real fetch on an `ios-remote` device, a coordinate tap went from 0ms to 3022ms, once per gesture, for a settle that was never going to arrive. So a settle that spends its whole window without one successful read records the outage on the run, against the device that proved it, and a later gesture rethrows that instead of buying the same verdict again. Not a shorter budget: any threshold low enough to spare a source that serves no tree would also abandon the mid-navigation blip the settle's retry exists for. But the record is a prediction, not a proof of repetition. One window mints it and nothing re-tests it, and on iOS that window can be a single stalled read, since the deadline is only tested after a read returns and `ViewHierarchy.getFullHierarchy` sits on a 15s RPC tier behind a serial 5s `Application.getState` probe. Being wrong therefore costs a settle rather than a step, and it is not silent: a gesture that spends the record warns its step report that it dispatched unsettled, quoting the source's own reason, so a run whose prediction was wrong says which of its greens mean the gesture was sent rather than that it landed. Every tree read a directive makes now goes through one helper so that a read which comes back clears the record whichever directive asked for it. `await`/`assert` and `idle` never settle, and a run that reads the tree successfully must not keep skipping. A relaunch clears it too, whether written as a `launch:` directive or as one of the raw `tool:` steps that perform the same operation, since a relaunch is the repair the commonest of these errors asks for by name. Two more reads route through the same helper for uniformity but can never be the clearing one: `rotate`'s aspect read skips itself while a record stands, and the `type` focus wait runs only behind a `waitForFrame` whose own settle already cleared it. What it costs, in two directions. An outage that lasted a full window, in a run that then never reads the tree again and never relaunches, leaves later gestures unsettled even once it clears: a flow of nothing but coordinate gestures giving up a best-effort settle it never had before this change, against every one of those gestures otherwise paying the window for a settle that is not coming. And on a screen that never holds still, a spinner, a video, a ticking clock, nothing converges, so every one of these gestures pays the whole window, measured at about 3.05s each. The record buys no relief there by design, because a window that read the tree proves no outage, and the fingerprint cannot be narrowed the way `scrollToVisible` narrows its own: a scroll knows the motion it waits on is its own, inside the containers under its anchor, while a gesture waits on motion of unknown origin that can move what sits under the point without touching it. It is the same window every selector directive already paid on such a screen; what changed is that coordinate targets pay it too. Two smaller pieces ride along. A centre-anchored `rotate` skips its screen-aspect read on a recorded outage as well, degrading to the same legacy orbit a failed read gives it, so the record actually takes the window off that directive instead of handing back a 15s-tier RPC. And the settle's tree read now takes the run's abort signal, so a cancelled run reports its skip immediately rather than once the source finally answers. It deliberately takes no time budget: one cut to what is left of the window would fail the cold start #778 raised that tier to ride out. |
||
|
Jakub Piasecki
|
abe5ff3c56 |
refactor(flow): generalize when's block machinery so a second block directive can't miss a site (#748)
`when` is the only directive with a sibling `steps:` list, and four runner sites asked `kind === "when"` directly to decide whether a step has authored children to expand as skip lines. A second block directive would have had to remember all four, and a forgotten one silently drops a whole block from the report — exactly the invariant `reportBlockSkipped` exists to hold. This introduces `blockSteps(step)` as the single runtime predicate and `BLOCK_DIRECTIVE_KEYS` as its parse-time counterpart, and extracts the parse-side block plumbing (`assertBlockDepth`, `parseBlockSteps`) that a second directive must share — notably the depth cap, renamed `MAX_BLOCK_DEPTH`, which has to be ONE counter across directives or an alternating nesting chain evades it and reopens the cyclic-YAML-alias hole it was added to close. No behavior change, with one deliberate exception: the depth-cap message now reads `block directives nest deeper than 20 levels` where it said `when blocks nest deeper than 20 levels` — the cap stopped being `when`'s the moment it became shared, and a message naming only `when` would misdirect an author who hit it through a different directive. Every other error message, their ordering, and the report shapes are untouched. 559 flow tests pass unchanged. --- **Stack** 1. **#748 — you are here** · `refactor(flow)`: generalize when's block machinery → `main` 2. #749 · `feat(flow)`: add `repeat:` blocks → #748 |
||
|
filip131311
|
6177409c7c |
fix(tools): drop top-level JSON Schema combinators from tool input schemas (#782)
Fixes #773. ## The bug `gesture-rotate` (`anyOf`), `flow-execute` and `flow-read-prerequisite` (`oneOf`) hand-write an `inputSchema` that spreads a top-level combinator onto the derived schema. The Anthropic Messages API rejects that outright: ``` tools.N.custom.input_schema: input_schema does not support oneOf, allOf, or anyOf at the top level ``` The 400 fails the **whole request — every tool in it** — so any client forwarding our schemas verbatim as `custom` tools is bricked entirely, not just for these three tools. Reported from opencode. ## Why we never hit it **Claude Code silently repairs it before sending.** Verified live: it strips the combinator and re-expresses the rule as a prose line prepended to the description. All of our E2E/QA runs through Claude Code, so the defect was invisible from inside our own test loop. Secondary reasons: the code is recent (#546, 2026-07-24 for `gesture-rotate`; #568, 2026-08-05 for the two flow tools — before late July no argent tool had a top-level combinator), and nothing in CI asserted the shape of any tool's `inputSchema`. ## Why removing it is safe The combinators never enforced anything. Nothing in argent validates against the JSON Schema — the zod `.refine()`/`.superRefine()` does all the rejecting, on every dispatch path (HTTP 400, MCP `isError`, CLI flag error, internal registry dispatch), and always did. #546's stated rationale was "to keep invalid calls rejected at the schema layer", but that layer does not exist; the only JSON Schema evaluator in the repo was a 5-line helper inside that PR's own test. Verified equivalence before deleting: the removed `anyOf` accepts exactly the same 8 radius shapes as the two refinements, and `oneOf` the same 4 source combinations as the superRefine. ## What changed - The three hand-written `inputSchema` consts are gone. No tool hand-writes one now, so `Registry.registerTool` is the single emission point for all 76 schemas. - Each rule moves into the tool description, **worded differently per tool** — `tool-description-quality.yml` triggers on these paths and scores `disambiguation_score` (W=1.5), so identical prose on two tools would work against the gate. - `ToolDefinition.inputSchema` documents why it should stay underived. - New per-tool contract test over the whole catalog: no top-level `oneOf`/`allOf`/`anyOf` (API-rejected) and no top-level `not`/`if`/`then`/`else`/`$ref`/`$schema` (blocked pre-emptively — the natural way to re-encode a rejected combinator). Nested combinators stay legal and in use: `tv-remote`'s `button` is a `z.union`. - Catalog walk + expected count extracted to `test/helpers/catalog.ts`, shared with `interaction-messages` so a new tool needs the count bumped once, not twice. - Six existing tests read `.inputSchema` off unregistered definitions (populated only at registration) and now derive it. ## Not done by design **No throw in `registerTool`.** `createRegistry()` runs at tool-server bootstrap (`index.ts:182`), where a throw becomes an uncaught exception → `crashShutdown` → `exit 1`: a total outage, and `update-argent` is itself a tool so it could not run to repair it. The catalog is static and compiled in, so the contract test catches the same regressions before merge — and it also covers `paste` and the Lens tools, which never pass through `registerTool` at all. **Deliberately a denylist, not an allowlist** of permitted top-level keys: `additionalProperties` legitimately appears on schemas derived from `.strict()` objects (`screenshot-diff`, `stop-all-simulator-servers`). ## Follow-up left out `gesture-rotate`'s refinements carry `path: []`, and `deriveInvalidParams` drops empty-path issues — so every cross-field rejection is telemetered with an empty `invalid_params`. Fixing it changes CLI wording and starts populating that field, so it is not riding in this PR. ## Verification - 75 registered tools probed live: **zero** top-level combinators; the three now advertise `type, properties, required` only. - Negative test: re-introducing an `anyOf` fails the gate with `gesture-rotate declares top-level anyOf`. - `packages/registry` 90/90 pass; `packages/tool-server` 3952 pass with **7 pre-existing failures** in `bind-failure-telemetry`, `lens-relay-telemetry`, `startup-telemetry` and `tool-fail-invalid-params-emission` — all failing identically on pristine `fe5976857` (they die in `attachRegistryEventLogger`, unrelated to schemas). - Build, eslint, prettier clean; knip identical to baseline (60 unused exports / 187 unused exported types both before and after). Reaches users on the next npm publish; current version is 0.20.0. |
||
|
filip131311
|
fe59768575 |
fix(flows): ride out iOS main-thread stalls instead of failing the step (#778)
## Problem iOS flows against a heavy app (Expensify NewDot, release sim build) intermittently error with ``` ✗ 4 tap "Continue" — ViewInspector RPC timed out: Application.getState ``` The only known mitigation was enabling **Reduce Motion** on the simulator — which shouldn't be a functional requirement, and (as the investigation showed) wasn't actually the mechanism. ## Root cause The injected ViewInspector handlers hop onto the app's **main thread** (`dispatch_sync` in `DevtoolsConnection.m`). During a heavy first-launch cold start (first Hermes parse, asset decode, on a loaded host) the main thread can be pinned past the flat **5s** RPC timeout, and the flow layer turns one timed-out read into a step error: - every iOS tree read first runs target auto-resolution → an `Application.getState` probe per connected app, per read — the cheap probe is the first to hit the stall; - `settleTree`'s 3s window < the 5s RPC timeout, so an action step's **first** stalled read exhausts the window and throws; - a default `await` (7.5s) gets ~1 stalled attempt; - the flow launch readiness gate (8s) sits below the same stall ceiling, because the dylib's connect is also main-thread-gated. Steady-state RPCs are fast (instrumented: `getState` p50 0ms / max 402ms, `getFullHierarchy` p50 9–12ms — even with animations on and a 14-core CPU load). Only the stall window is the problem; Reduce Motion merely shortened it below the cliff (and correlated with cache warming from repeated relaunches). Error-code note: these failures are `NATIVE_DEVTOOLS_RPC_TIMEOUT` on an **established** connection — distinct from the multi-server socket-contention family (#775), which surfaces as `could not connect to native devtools` at launch. ## Fix (three coordinated changes, tool-server only) 1. **`getFullHierarchy` RPC timeout 5s → 15s** — mirrors the Android devtools client's 5s default / 15s `getHierarchy` tiers. Every other method (incl. `getState` and the interactive point queries) keeps 5s, so one-shot agent tools stay snappy on a wedged app. 2. **Flows remember the launched app** (`launchedNativeApp` on the run state, threaded to iOS tree reads; invalidated by `launch-app`/`restart-app`/`reinstall-app`/`open-url`/`button` `tool:` steps; shared with nested `run:` flows). It is a **timeout arbiter only**: auto-resolution still runs on every read, and any *answered* resolution — including the deliberate "single app but backgrounded" guard — wins. The hint decides the target only when the resolution probe itself dies of an RPC timeout and the hinted app's devtools connection is still up. 3. **Launch readiness gate 8s → 15s** — same stall class, same budget. Accepted trade-offs (from adversarial design review): a persistent outage now takes longer to report (a dead-but-connected app fails an `await` after up to ~2 slow reads instead of three 5s ones), and a stall that clears right at a settle deadline can act on a single post-stall read (previously a hard error). Both were judged worth the removal of the false-failure class; the review also killed an earlier variant of this fix that would have bypassed the backgrounded-app guard. ## Verification - **Fault injection**: a DYLD-inserted dylib pins the app's main thread on demand (12s). Pre-fix, deterministically reproduces both historical signatures — mid-flow stall → `tap error: ViewInspector RPC timed out: Application.getState`; launch-window stall → `launch error: could not connect`. Post-fix: **5/5 green** across both stall placements ×2 + no-stall control. - **Android regression**: the same sign-in flow on the Android emulator through the patched server — green (the hint is iOS-only by construction; Android/Chromium tree paths unchanged). - **Unit tests**: new `flow-ios-tree-launch-hint.test.ts` (6 cases: timeout fallback, no-hint rethrow, disconnected-hint rethrow, backgrounded-guard preserved, non-timeout rethrow, answered-resolution-beats-hint); existing `flow-ios-tree-no-windows` and `native-target-app` suites green. Full tool-server suite: 3875 passed; the 7 failures in `bind-failure/startup/lens-telemetry` tests fail identically on pristine `origin/main` sources in this environment (pre-existing, unrelated). Related: #775 (multi-server devtools socket contention, filed from the same investigation), #774 (sim keyboard shift wedge, unrelated mechanism ruled out during triage). 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Hubert Gancarczyk
|
8623965b8a |
perf(flow): stop the recorder returning the whole flow file per step (#715)
## What changes `flow-add-step` and `flow-add-echo` returned the **entire growing YAML** on every call. Across measured recording sessions that was the majority of all tool-result text, and it grows quadratically with step count. - Both tools now return `stepCount` — how many steps the flow has — instead of `flowFile`. - `flow-add-step` additionally returns `recorded`: the one summary line that was appended. - The full file still comes back once, from `flow-finish-recording`. ## Why The cost is not just tokens. With the whole file echoed per step, context pressure during a long walkthrough visibly changes the artifact being produced — checks get trimmed from a test to buy back room. Returning a line per step removes that pressure without removing any information the author needs *at that moment*. ## Supporting change `summarizeStep` is lifted out of `flow-finish-recording`'s per-file loop so the recorder can render its one line exactly the way the final summary does — one spelling, not two that can drift. That line now also carries `times`, `duration` and `delayMs`. They change what replays, and since the recorder no longer returns the YAML, this line is the author's only per-step view of what landed in the file. ## Compatibility This is a **breaking change to the tool result shape**: `flowFile` is gone from `flow-add-step` and `flow-add-echo`. `savedTo` is unchanged and is still the only field naming the destination in remote-client mode. `flow-finish-recording`'s result **shape** is unchanged, but its `summary` strings are not: taps now render `×N`, long-presses `for Nms`, and tool steps `(after Nms)`, so the two surfaces spell a step the same way. Nothing outside the agent reads `summary`, so that is not a break. ## Tests Recorder tests now assert against the flow **as persisted on disk** rather than a returned string, which is the surface that actually matters. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
Hubert Gancarczyk
|
74cddedf50 |
docs(skills): split argent-create-flow into references and add argent-qa-flows (#730)
> Now rebased directly onto `main`. #728 and #574 have landed, so this PR carries only its own 13 documentation commits; it still does not sit on the #715-#726 chain. > > Guidance describing behaviour owned by that chain is therefore **not** carried here: the recorder's refusal of a vacuous `hidden` and of `await-screen-idle` (#724), its cross-tree re-probe warning (#717), the per-step `recorded` line (#715), and `type:` failing on unconfirmed focus (#726). Each of those paragraphs states what this base actually does instead; they are worth restoring when that chain lands. ## `argent-create-flow`: ~5,000 words to ~900 plus three references Every word of that skill loaded on every invocation, whether the task was replaying a two-step fragment or authoring a full regression test. The core skill now routes to three references read on demand: - **`live-authoring.md`** - recording a walkthrough - **`flow-yaml.md`** - the file format and selector vocabulary - **`reliability-and-recovery.md`** - what to do when a step or a replay goes wrong Nothing is dropped in the split - it moves behind a router. ## New skill: `argent-qa-flows` Turning a **test case, ticket, or acceptance criteria** into a repeatable regression test is a different job from recording a path worth replaying, and it was being done by a skill that never asks for acceptance criteria. It orchestrates `argent-create-flow` as its engine, records the first walkthrough live, requires every requested screen and state to be proved with stable evidence, and completes only after the unchanged flow passes **twice consecutively**. ## Proving a navigation Spelled out here for the first time, as two element-level checks rather than a route read: ```yaml - await: { visible: { id: settings-screen } } # identity: WHICH screen - await: { idle: true } # readiness: it stopped moving ``` Neither implies the other - a dropped tap leaves the source screen perfectly idle, and the destination's elements enter the tree while the transition still animates over them. Both belong after every screen change, including the one `launch:` performs. ## Routing The three skills were being confused by name alone, so the rules now draw the line: | you want | skill | | --- | --- | | a one-off interactive check, nothing saved | `argent-test-ui-flow` | | a saved, replayable path | `argent-create-flow` | | a saved test with acceptance criteria and two-pass proof | `argent-qa-flows` | "Record a flow" also stops being ambiguous with **screen recording**, which is video. ## Tests Every bundled skill's YAML frontmatter is parsed in a test, so a malformed new skill fails here rather than at install time. The `idle` doc guards that came with the warn-instead-of-fail change follow the prose into `references/flow-yaml.md`: they pin the documented defaults, settle cost and minimum timeout to the constants the parser enforces, and hold the tool description and the reference to the same account of what `idle` does. All 43 anchored internal links resolve. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
filip131311
|
4975ff490c |
fix(debugger): cut debugger tool fail rate — structured not-connected results, classified CDP failures, narrow recovery (#610)
## Why
PostHog (30d) shows the debugger tool family is argent's least reliable
surface:
| tool | fail rate | dominant causes |
|---|---|---|
| `debugger-status` | **37%** | METRO_NO_TARGETS /
SERVICE_INITIALIZATION_FAILED / zod |
| `debugger-connect` | **24%** | SERVICE_INITIALIZATION_FAILED (~7k, >1k
users), METRO_NO_TARGETS |
| `debugger-log-registry` | 41.7% raw / 4.2% excl. storms | one user
retry-looped it **12,742×** on NO_TARGETS |
Three design issues drive this: (1) "app/Metro not ready yet" — an
expected precondition — is reported as a tool *error*, which both
inflates fail rates and invites agent retry storms; (2) every CDP
init/lifecycle fault lands in telemetry as the catch-all
`REGISTRY_SERVICE_INITIALIZATION_FAILED` with `error_kind=unknown`; (3)
~600 users/30d hit zod validation failures with zero visibility into
which parameter was wrong.
## What
**1. Classified failures** (`d71efa43`) — six former plain-`Error` sites
in `cdp-client.ts` / `target-selection.ts` now carry dedicated
`DEBUGGER_CDP_*` / `DEBUGGER_TARGET_DEVICE_MISMATCH` signals (messages
byte-identical). The registry's two causeless "terminating window"
`ServiceInitializationError`s get their own
`REGISTRY_SERVICE_TERMINATING` code.
**2. Narrow recovery** (`ff9c1955`) — `recoverable()` on both debugger
blueprints, scoped to windows where retry is provably safe: Metro
recovers only `DEBUGGER_CDP_NOT_CONNECTED` (request never left the host;
a detected socket death already self-heals for the *next* call via the
terminated cascade — recovering `CONNECTION_CLOSED` there would be dead
code, see the blueprint comment). Chromium also recovers
`CONNECTION_CLOSED` for the tab-switch reconnect window.
`REQUEST_TIMEOUT` stays unrecoverable (double-execution risk). A
reachability test proves the retry actually fires for a RUNNING node.
**3. Structured not-connected results** (`48b91ffd`) — `debugger-status`
and `debugger-log-registry` become factories that return `{ status:
"not_connected", reason, detail, guidance }` for classified
preconditions (`metro_not_running | no_app_connected | device_mismatch |
cdp_unreachable | stale_connection | reconnecting`) and still throw on
unexpected faults. `detail` preserves the original error text for
string-matching agents. `debugger-status` gains a socket-state gate
(Metro: dispose stale node; Chromium: report transient reconnect, no
dispose — the client belongs to the ChromiumCdp dependency).
log-registry keeps **no** gate on purpose: captured logs are readable
over a dead socket, and disposal would destroy the post-crash log file.
Flows/run-sequence map a not_connected result to a **failed step**
(mirrors `isUnmetUiWaitResult`) so recorded connectivity gates don't
silently pass. `debugger-evaluate` re-codes agent-expression throws as
`DEBUGGER_EVALUATE_EXPRESSION_THREW`.
**4. Telemetry + guidance + docs** (`7602a70d`) —
`tool:fail.invalid_params` (schema-declared names only; unknown keys →
literal `"unrecognized_keys"`; capped emit-side at 16), new
`debugger:tool_outcome` event (coded enum; joins
`tool:invoke`/`complete` via `tool_invocation_id` —
`ai_client`/`duration` live on the joined row), anti-retry-loop guidance
appended to the Metro discovery errors (first sentences verbatim —
skills match on them), six skills updated, Telemetry.md bumped to v1.03
with the new coded diagnostics disclosed.
## Dashboard migration note
`debugger-status` / `debugger-log-registry` precondition failures stop
emitting `tool:fail` — track them via `debugger:tool_outcome` (`outcome
!= 'connected'`). Per-AI-client breakdowns of those outcomes need a join
on `tool_invocation_id` against `tool:complete`.
## Testing
- Full suites green: tool-server 3,142, telemetry 290, registry 87. New
coverage: recoverable truth tables (incl. non-Error inputs), status
not-connected matrix
(Metro/Chromium/terminating-window/stale/dispose-side-effect/fall-through-rethrow),
log-registry rows, zod `invalid_params` derivation + emission-level
guard, flow gate, evaluate rewrap, sanitize-level enum checks.
- Live-tested against a booted iOS simulator + RN app and an Electron
target (details in PR comments).
Plan hardened through 3 adversarial review rounds (46 confirmed findings
addressed) before implementation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
||
|
Hubert Gancarczyk
|
96ea94f87a |
feat(flow): add an idle condition that waits for the screen to stop moving (#728)
> Stacked on #574. Replaces the route-fingerprint approach that #727 and #729 carried; both are closed. > > Rebased off the #715-#726 chain so it can merge on #574 alone. That chain owns `DirectiveOutcome.warning` / `StepReport.warning` and the `indeterminate` scoring rule; the small amount of each that this condition needs is carried here, scoped to `idle`, and will conflict textually with #724/#725 for whichever lands second. ## One check a selector cannot express ```yaml - await: { idle: true } ``` Has the screen stopped moving? Flows were substituting fixed `wait:` steps for it, which either wait too long on every run or too little on the run that mattered. ## Why it reads pixels as well as the tree Each signal is blind to what the other sees. The tree cannot see presentation-layer motion. An iOS push or modal dismissal commits its hierarchy up front and then animates a layer for a few hundred milliseconds; a cross-fade or a scrim moves no node at all. Measured on a real Android emulator: a page animating a large element continuously reads as a single static `WebView` node, and the existing `await-screen-idle` tool called it settled in **465 ms, five times out of five**. On a deterministic Chromium page whose DOM is byte-identical across the animation, that tool returned `settled: true` in **407 ms** while this condition failed 8/8. Pixels alone would not do either: they cannot see a tree still churning behind an unchanged surface, and anything animated forever (a video, a shimmer) would make a pixel-only settle unsatisfiable on a screen the tree calls ready. Unlike that tool, this cannot be silently stepped over. Readiness is not an acceptance criterion — the verdict belongs to the identity and outcome checks around it — so a screen that never settles still **passes**, carrying a `warning` that says what the green actually bought. That is what makes it safe to persist in a flow: it reports on every run without ever failing one over a video or a shimmer. The only thing that stops a run is a tree source that cannot be read. ## The rule the design follows The absence of evidence is never evidence. - A capture that did not arrive cannot complete the hold. - Neither can a single agreeing pair. Every animation that reverses has a turning point, and two samples straddling it come back identical while the screen is moving. On a live 3 s cross-fade that passed a default-shaped step on roughly **one run in three**; settling now takes two consecutive still intervals, so `minStableMs: 0` still means three reads. - A round is never started without the budget to observe it with. A round begun with nothing left neither captures nor reads, and both absences were otherwise recorded as facts about the device. - No verdict comes from a latch. A screen that settles and then moves again, or goes blank, has not settled. ## `timeout:` is a real bound No describe path takes an abort signal, so the tree read is raced against what is left of the budget. Without that, a wedged ViewInspector RPC ran 2.25 s past an 8000 ms budget, and a busy Android screen ran 5.4-6.0 s against a 3000 ms one. A read that runs out of step budget is the step ending, not the source failing, and only the latter is reported as an environment problem. ## Failure modes stay apart They call for opposite responses. A screen that never stopped moving is a verdict about the app. A tree that could not be read is not, and it names the foreground check first, because a backgrounded app reads identically to an uninstrumented one and relaunching is the wrong repair. A run whose captures never produced a comparable pair still settles on the tree and says so in a warning, rather than passing off half a proof as the whole one. ## Capture routing and tolerance Captures route exactly as the `screenshot` tool routes them, so tvOS and Vega, which have no simulator-server backend but are perfectly screenshottable through `xcrun` and the emulator console, are covered rather than written off. The pixel comparison owns its tolerance rather than borrowing `screenshot-diff`'s. That one holds a baseline stored across sessions, machines and OS versions against a live capture and must absorb real drift; this one holds two captures one poll apart from a single session, where a static screen reads back byte-identical (measured: zero changed pixels across five consecutive pairs on an idle simulator). The margin is load-bearing, because uniform change is all-or-nothing: at the baseline tolerance any cross-fade slower than ~2 s counted zero changed pixels and read as settled mid-animation. ## Not a screen check A dropped tap leaves the source screen perfectly idle. It belongs after the element check that names the destination, never instead of one. #730 documents the pairing. ## Verification - 3307 tests pass; typecheck, `typecheck:tests`, lint and prettier clean. Eleven mutations applied to the source, all eleven caught by tests. - Driven end to end against a real iOS simulator, a real Android emulator, and a Chromium app through a tool-server built from this branch: static screens pass with no warning (proving captures ran), animating screens pass carrying the warning that names the motion, and the pre-existing tool disagrees exactly where it should. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
Hubert Gancarczyk
|
afd722924f |
feat(flow): support concurrent flow recordings (#574)
## Problem
Only one flow could be recorded at a time, anywhere on the machine.
Recording state was three module globals in `flow-utils.ts` — harmless
in a per-client process, but the tool-server is a **host-wide singleton
per install bundle**. Every MCP client, every subagent sharing a
parent's connection, every `argent` CLI call and every project on the
machine attaches to the same process, the same `Registry`, and those
same three globals. HTTP requests are served concurrently with no
serialization.
Three failures followed, all reproduced against the pre-change code:
1. **A second recording clobbered the first.** `startRecordingSession`
just overwrote, so the first agent's next `flow-add-step` silently
appended into the *other* agent's flow file — in a different project.
2. **Any agent could finish any agent's recording.**
`flow-finish-recording`'s schema was literally `z.object({})`.
3. **Replay stomped recording.** `resolveFlowFilePath` called
`setActiveProjectRoot`, so a `flow-execute` in project B rebound the
global root mid-recording of project A. Already worked around, locally
only, in `flow-add-step`.
## Approach
Recording state is now a `Map` keyed by the resolved flow file path,
`<project_root>/.argent/flows/<name>.yaml` — the identity of the
artifact being built, so "same key" means "same output file" and two
sessions on one key are a genuine collision rather than an accident of
scoping. `flow-add-step`, `flow-add-echo` and `flow-finish-recording`
each take `name` + `project_root`, so every call is self-contained;
required rather than optional-with-fallback, since a fallback would
succeed while an agent is alone and fail only once a second recording
exists.
`resolveFlowFilePath` is pure, which fixes defect 3 and retires
`FLOW_PROJECT_ROOT_REQUIRED` (the state it guarded can no longer exist).
Rejected alternatives — keying by device udid (flows are deliberately
device-portable, and `flow-add-echo`/`flow-finish-recording` have no
device at all), an opaque `recordingId` (an agent that loses the token
cannot recover), and per-caller identity from the transport
(`ToolContext` carries no caller identity, and subagents share their
parent's MCP connection, so it would fail the exact scenario being
fixed).
### Locking, and why readers needed more than a lock
Every mutation of one flow file runs under a lock keyed by that path —
appends, `flow-start-recording`'s truncate-and-register, and
`flow-finish-recording`'s read-summarize-clear. Keyed by file rather
than owned by the session, because a restart *replaces* the session: a
session-owned lock cannot exclude the operation that supersedes it. Per
file, not global, so two recordings never queue behind each other. An
append whose session was finished, restarted or evicted meanwhile fails
with `FLOW_NO_ACTIVE_RECORDING` rather than writing into a different
take.
The lock serializes writers only, and no reader of a flow YAML can join
it — `flow-execute`'s own load, its `run:` fragment load,
`flow-read-prerequisite`, `flow-add-step`'s sibling-fragment check, and
the `argent` CLI reading from another process entirely, where an
in-process lock cannot reach. Both writers used a plain `fs.writeFile`,
which opens `O_TRUNC`, so a reader could land between the truncate and
the write. That case is silent: `parseFlow("")` returns `{ steps: [] }`
with no error and `summarize` derives `ok` from "no failures", so a
`flow-execute` racing an append reports a **top-level PASS having
replayed zero steps**. Both writes now go through a sibling temp file
and a rename, so a reader sees either the whole old file or the whole
new one — and unlike the lock, that also holds cross-process.
### `stop-all-simulator-servers` gets a `devices` scope
Every agent is told to call this at session end, and unscoped it walked
the whole registry — so agent A wrapping up tore down agent B's devtools
mid-recording, degrading B's flow to brittle coordinate taps. It now
takes an optional `devices` scope and reports `unmatched` ids, so a
mistyped id doesn't read as a clean machine.
Which namespaces count as "owned by a device" turned out to be the
load-bearing part, because nothing cascades to most of them and
`unmatched` turns any omission into an actively wrong diagnosis — a
correct device id reported as a typo. The set now covers `AXService`
(the in-sim ax daemon, spawned `--timeout 3600`; an iOS session that
only ran boot/launch/describe owns this and nothing else),
`ScreenRecordingSession` (an ffmpeg child plus the touch-visualizer
overlay it enabled on the device), `NativeProfilerSession` (an xctrace
child, or an on-device perfetto process and its trace file) and
`JsRuntimeDebugger` (a bound loopback server, the CDP socket to Metro, a
log handle). The debugger URNs interpose the Metro port —
`<ns>:<port>:<deviceId>` — so the matcher understands both shapes,
consuming only the first colon so a wireless adb serial after the port
still compares whole.
`stop-simulator-server` shared none of this and had drifted: it looked
URNs up with an exact, case-sensitive `services.get()`, so a lower-cased
UDID silently no-op'd there while the scoped stop-all reaped it. Both
tools now share one matcher. Their namespace *sets* stay deliberately
different, documented where they are defined — `stop-simulator-server`
is also the documented recovery for a wedged transport, and widening it
to devtools/AX would make a routine retry drop the native-devtools
connection another agent's recording depends on, which is the hazard the
`devices` scope exists to prevent.
## Verification
**End-to-end, three devices, one shared tool-server** (iPhone 16 Pro,
iPhone 17 Pro, Pixel 3a): three interleaved recordings, plus an
unrelated `flow-execute` in a fourth project while all three were live.
Both iOS devices held their own live native-devtools concurrently (two
per-UDID sockets, `connected: true`). Each YAML contained exactly its
own steps in order, taps captured as selectors rather than coordinates,
and neither picked up another's or the third root's path. All three
replayed green — also through the **released 0.17.0 CLI** against the
modified server, which checks wire compatibility. A device-scoped
teardown of sim 1 left sim 2 and the emulator running.
Re-run against the final build:
- Two interleaved recordings across two project roots on one device,
with a **separate process** polling one of the flow files as fast as it
could for the duration. Zero empty or truncated observations; each YAML
held exactly its own steps; both replayed green (6/6 and 5/5). Against
the pre-change in-place write the same reader observes zero-step reads
interleaved among the real ones — the silent-PASS input.
- A scoped teardown of the iPhone reaped `AXService` and
`NativeDevtools` for that UDID only, reported no `unmatched`, and left
the second sim and the emulator running. Lower-casing the UDID matched;
a genuinely bogus id was still reported.
- With a screen recording live on the Pixel 3a, a scoped teardown reaped
`ScreenRecordingSession` and the **ffmpeg child actually exited** (1 →
0). Before the fix it survived to the 180 s cap while the tool called
the correct serial a typo.
The pre-change repros were re-run afterwards to confirm observed
behavior changed, not just the tests.
`npx vitest run` in `packages/tool-server` (295 files, 3154 tests) and
`packages/argent-cli` (280), `tsc --build`, `tsc --noEmit` on both test
projects, eslint and prettier — all green.
## Review notes
Several rounds of review found real defects in this change; each was
reproduced before fixing and re-verified after. Worth knowing when
reading the diff, since the tests exist to pin them:
- `flow-start-recording` truncated the `.yaml` outside any lock, so a
step from the take being discarded landed in the freshly reset file and
was reported as success (26-29 of 200 runs).
- `flow-finish-recording` never took the lock; its `await fs.readFile`
was a yield during which a concurrent append committed, leaving the
reported summary disagreeing with disk (11-16 of 200).
- A failed finish destroyed the recording. Hand-editing the `.yaml`
mid-recording is a documented workflow, so `parseFlow` can legitimately
throw — and the error told the agent to call `flow-start-recording`,
which truncates the very take it would recover.
- `unmatched` reported a device this session had just stopped, because
`disposeService` returns a node to IDLE and keeps it.
- A bare IP claimed every wireless-adb device at that address, since an
adb serial is itself `ip:port`.
- The eviction backstop stamped with `Date.now()`; ties at millisecond
resolution let it drop the session that was just used.
- `requireRecordingSession` ended with "Call flow-start-recording first"
— reached for a key that was finished, superseded or evicted as well as
one never started, and on those branches the file on disk is fully
populated while that call truncates it and reports no `restarted`.
- The atomic write regressed twice on its own terms: its scratch name
was derived from the flow file's basename, which has no length cap, so a
long flow name that appended fine before failed `ENAMETOOLONG`; and the
cleanup guard began after the temp file was created, leaking a scratch
file into a committed directory on any write that failed after opening.
- Two tests passed against implementations they claimed to reject: the
`stop-simulator-server` narrowness guard used a udid that classifies as
*android*, where the iOS-only services it guards can never appear, so
widening the iOS branch kept it green; and the client-mode finish
asserted only the *shape* of `savedTo`, never its content, though that
directive is the only thing that lands the file in client mode.
## Notes for reviewers
- Client/remote persist mode is the least-exercised path, and now has
its own concurrency tests (overlapping appends, and a restart
superseding an in-flight one) — it previously had none. The
`project_root` key is byte-stable across the file boundary (`kind:
"probe"` passes it through unchanged), which is what keeps a remote
recording addressable from `flow-add-step`, which declares no file
input.
- Two spellings of one flow file still mint two sessions — and two
independent locks — over one file, which bypasses every guarantee here.
Two ways in: the root spelled two ways (a symlink, or a case variant on
APFS), and the likelier one, the *name* cased two ways on a
case-insensitive volume. Neither is normalized away because the correct
normalization is the filesystem's: case-folding the key would wrongly
merge two distinct flows on ext4, and resolving symlinks is impossible
in client mode, where the root does not exist on this host. Documented
at `getFlowPath`.
- No recording tool can detect that another agent took its key.
`flow-add-step`, `flow-add-echo` and `flow-finish-recording` all
re-resolve the key on every call, so after a takeover they act on
whichever take is now live and report success: an append lands in the
other agent's file, and a finish finishes and clears it. The only case
that fails loudly is the narrow one the liveness check covers - an
append already IN FLIGHT when the restart lands. There is no caller
identity to key ownership on (see the rejected alternatives above), so
this is spelled out for the agent under "Pick a name unique to your
task" in `argent-create-flow` rather than mechanized.
- Skew hazard in one direction only: a *new* client against an *older*
tool-server. Zod strips the undeclared `name`/`project_root`, and the
old server falls back to its module globals — so concurrent recordings
silently collapse onto one session. The other direction (released 0.17.0
CLI against this server) was verified end-to-end.
- `packages/argent-private/docs/reference.md` still describes
`flow-add-step` as appending to "the current recording", and names two
tool ids that do not exist (`flow-insert-echo`/`flow-run` are file
names; the ids are `flow-add-echo`/`flow-execute`). It is a submodule,
so the fix cannot ride on this branch.
- A restart reported `discardedSteps` from the superseded session's
in-memory flow while it truncates the *file*. Hand-editing the `.yaml`
mid-recording is documented, so the two diverge: four steps wiped, one
reported. It now counts the file, and reports no number at all when that
file cannot be read or parsed - 0 is the answer a genuinely empty take
gives. Client mode still counts from memory (this host cannot see the
client's file), and the description now says which mode it is promising.
- `ChromiumJsRuntimeDebugger` cascades from `ChromiumCdp` and was
therefore already being torn down - silently, while `NetworkInspector`
and `ReactProfilerSession` cascade the same way and were named.
`stopped` is documented as the services that were live and got shut
down, so it is listed now too.
- The client-mode case named for a superseded in-flight append restarted
*between* calls, so the next append re-resolved the key and succeeded -
the guard was never reached, and neutering it left the whole file green.
It now parks an append in its live sub-tool call across the restart.
- `stop-tools.test.ts`' mock handed the tool the live service map where
the real `getSnapshot` copies, so a cascade rewrote the state the sweep
was still reading and the answer depended on map insertion order. The
mock copies and recurses like `_teardown` now, and the cascade case runs
under both orders.
- Prose the code contradicted, each verified against source before
rewriting: Vega owns `JsRuntimeDebugger`/`NetworkInspector` once the
debugger has run (and can match anything at all through an ERROR node);
"nothing here cascades" was denied twice in its own file; an iOS
boot/launch/describe session owns `NativeDevtools` too; `:tcp` is
mintable but unreached in production; the tool-server is a singleton
*per install bundle*, not per machine; `argent flow list` does enumerate
`.argent/flows`; the stop-tools comments narrated a "before" that only
existed on this branch.
Found while verifying, out of scope, worth their own issues: a `tool:
flow-execute` self-reference recurses past both cycle guards (`run:` is
guarded, a nested `flow-execute` reseeds the stack); react-profiler
artifacts collide host-wide on a second-resolution filename;
`stop-metro` is port-scoped and kills another agent's bundler;
`debugger-reload-metro`'s HTTP fallback broadcasts to every attached
client. A tool's `capability` is enforced only by the HTTP layer, not by
`registry.invokeTool`, so a call that reaches the registry another way -
`flow-add-step` takes `command` as a bare string - can resolve a service
the tool declares it does not support, leaving an ERROR node behind
(e.g. `SimulatorServer:<vega serial>`).
---------
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
|
||
|
Jakub Piasecki
|
6d907cc86b |
feat(flow): boot a Chromium instance per launch step (#585)
A `launch:` on Chromium now starts its own Electron instance, so nested e2e flows and mid-flow relaunches work as they do on iOS/Android. Previously the runner booted one app per run and rejected any second chromium launch. A chromium device id _is_ its CDP port, so a launch that starts an app from scratch has to boot one and move the run onto it: `state.device` became reassignable (steps already read it per call), and `state.owned` tracks what the runner booted, torn down in reverse at run end. An instance the runner did not boot — pinned via `--device`, or auto-detected — is attached to, never killed. Relaunching the same app kills its instance and waits for the process to actually exit, or the replacement loses a race against the app's single-instance lock. The runner needs a device before step 1, so `resolveRunDevice` follows a leading `run:` chain to find the launch the run _begins_ with: a fragment composing a chromium e2e flow now boots instead of failing with "No booted chromium device found". A launch declaring no app id for the run's platform stays an error — the run's platform is fixed start to finish. Also fixes a pre-existing teardown leak: the spawned handle is the npm `electron` node wrapper, not Electron, so signalling it left the app and its helpers running (reparented to launchd, still in the dock). Teardown now sweeps the child's process group. **Behavior change:** a fragment leading with `run:` into a chromium e2e flow now boots a fresh instance where it previously attached to a running one; pass `--device` to keep attaching. ## Tests Cover the boot/attach/settle three-way, teardown ordering and the group sweep, device rebinding after a nested launch, same-app retire ordering, cancellation mid-boot, both "no chromium app declared" guards, and the leading-`run:` chain (multi-hop, cyclic, no-launch). The guard, exit-wait and sweep tests are mutation-checked. |
||
|
Jakub Piasecki
|
22033e1418 |
feat(flow): allow to run every flow in a directory from flow run (#576)
`argent flow run` now accepts a directory and runs every flow in it sequentially: ``` argent flow run .argent/flows --recursive ``` Discovery uses the same acceptance rules as `flow list` (lowercase `.yaml`, safe stem, readable file), sorted for a deterministic run order. `-r`/`--recursive` walks subdirectories too, skipping dot-directories and `node_modules` and never following directory symlinks. A directory named `foo.yaml` still batches (stat wins over extension). Batch output is failures-only: each flow prints a `[i/n] <path>` header, its failing/errored steps — numbered exactly as a single-mode rerun would number them — and a per-flow summary; the run ends with a `PASS/FAIL — N flows: X passed, Y failed, Z skipped` verdict. `--json` prints one aggregate object with per-flow reports. Exit code reflects the batch (0 pass, 1 any failure, 2 discovery/config errors). `--output` exports keep the single-run `<output>/<flow>/` layout, keyed by subdirectory in recursive runs so same-stem flows in different directories can't clobber each other's snapshots. **Failure semantics:** a flow failing its steps — or one the tool-server rejects as invalid (bad YAML, unparseable step) — fails alone and the batch continues. An infra error (transport death, unclassified failure) stops the batch and counts the remaining flows skipped. To make that distinction possible, tool-error responses now carry the server's failure signal (`error_code`/`error_kind`) on the wire — on the streaming terminal error line and on JSON error bodies — and the tools client surfaces server-answered errors as a typed `ToolInvocationError`. Against a pre-signal server this degrades to stopping on any throw. Not breaking: single-file runs behave exactly as before. |
||
|
Jakub Piasecki
|
cc2f907ce9 |
feat(flows): resolve run: directives from explicit YAML paths (#567)
A `run:` value is now a YAML path resolved against the directory of the flow file that contains the step, matching the CLI cutover: ```yaml - run: login.yaml - run: ../shared/login.yaml # cross-directory composition now works ``` Bare names fail at parse with a migration hint (`run: login` → `run: login.yaml`). The filename stem stays the fragment's report attribution; reports also carry the as-written path as `target`. Cycle detection compares realpath-canonical paths (same stem in two dirs ≠ cycle; same file via symlink/spelling = cycle). Snapshot baselines are unchanged — keyed by and stored beside the root flow. An uploaded root flow with `run:` steps is now rejected up front with a contract error instead of a mid-run temp-dir ENOENT (its referenced files stayed on the client, and relative resolution in the temp dir could reach host files); same for a relative chromium app path. The recorder emits the sibling form (`run: <name>.yaml`). **Breaking:** existing `run: <name>` directives fail at parse until renamed to `run: <name>.yaml`. Snapshot baselines for a saved flow whose YAML is a symlink now anchor beside the real file (the symlink target's directory) — move any committed `.argent/flows/__baselines__/<flow>/` set beside the real file once, or the next run fails with a missing baseline. A `run:` target must also resolve inside the project root or beside/below the flow file that references it (arbitrary host YAML is no longer reachable), and an uploaded flow with a `snapshot` step is rejected up front (its baselines cannot anchor to the per-call temp dir). |
||
|
Jakub Piasecki
|
a644d52d39 |
feat(flows): run CLI flows from explicit paths (#568)
`argent flow run` now takes an explicit YAML path instead of a saved-flow name: ``` argent flow run .argent/flows/checkout.yaml --platform ios ``` The path may point anywhere on the local filesystem; the filename stem names the run's report/artifacts, so it must be `[A-Za-z0-9_-]+` with a lowercase `.yaml` extension. A bare name fails with the exact replacement command, and `argent flow list` prints paths instead of names. Because sibling `run:` files and `__baselines__` resolve beside the YAML, the CLI now requires the auto-started local tool server and refuses `ARGENT_TOOLS_URL` / `argent link` routing. The `flow-execute` tool stays remotely callable via `name` + `project_root`, and gains a `flow_path` parameter (exactly one of `name`/`flow_path`) that is only honored when the file-input boundary verified it as a real host path — uploads and boundary bypasses are rejected, so it can't execute arbitrary server YAML. Also: the flow-name pattern is now a single shared export from `@argent/registry`, and the recorder composes a `flow_path` call into a `run:` step when it targets a sibling of the recording. |
||
|
Jakub Piasecki
|
2df2fac43d |
fix(flow): error a no-windows iOS read instead of a false hidden pass (#519)
## Problem On iOS, a flow `assert hidden` (and an unmet `when` guard) could pass / green-skip against a screen where the element was plainly visible — whenever the target's view hierarchy can't be read. A flow that launches a non-injectable app (e.g. an Apple system app, where library validation blocks injection) runs to exit 0 with a false pass while `describe` still shows the element on screen. ## Root cause `queryFullHierarchyTree` returned an empty tree as a *successful* read when `getFullHierarchy` yields no windows, and the iOS source never sets a degraded-read hint. The runner's blind-read guard then falls back to `everMatched` alone, so an element that was *never seen* leaves the empty tree "trusted" — and an empty tree is exactly what a `hidden`/absent check accepts as satisfied. ## Fix Throw on a no-windows read instead of adapting it to an empty tree — consistent with the existing unavailable / requiresAppRestart guards. The caller's retry loop rides out a transient blank; a persistent one surfaces as the step's failure (`assert` fails, `when` guard errors) with an actionable reason, never a false pass. Keys on raw `windows`, not the flattened child count, so a genuinely sparse screen (windows present, no matching leaves) still resolves to "not found". ## Tests - New `flow-ios-tree-no-windows.test.ts`: throws on empty / missing windows, returns a tree when windows are present, still throws on an explicit RPC error. - `flow-hidden-blank-reads.test.ts`: adds the never-seen case (element never matched + dark source) → `hidden` fails with the outage reason, not a pass. |
||
|
filip131311
|
5a64e4e7de |
fix(flows): stop a recorded sub-flow pinning the device it was recorded on (#696)
Fixes #607. ## The bug `DEVICE_BIND_KEYS` covered `udid` and `device_id`, but `flow-execute`'s own device parameter is named `device`. So a recorded `tool: flow-execute` step kept the record-time id in the YAML, and on replay the runner never rebound it. Reproduced live on 0.18.1. Recording a cross-project `flow-execute` step: ```yaml steps: - tool: flow-execute args: name: b-only project_root: /…/projB device: BC0026C7-AAE0-490E-8BD4-A345CF65FB1B # survived stripDeviceKeys ``` Then editing that id to one that does not exist — what a flow committed on one machine looks like on another — and replaying against a real booted device: ``` flow-execute { name: pinned, device: BC0026C7-… } [1] ✓ flow-execute { "device": "00000000-0000-0000-0000-000000000000", <-- baked in, not rebound "ok": false, "errored": 1, … "reason": "…[SimulatorServer:00000000-…] simulator-server exited…" } PASS — 1 passed, 0 failed, 0 errored ``` This contradicted three things the code and docs already claimed: that flows store no device id and are portable, that the runner is authoritative on device and drops any id stored in a step, and that device ids are stripped at record time. It also made the raw `tool:` form behave differently from `run:` composition, which has always inherited the run device. That matters more than it looks: the recorder falls back to the raw form whenever the target is not a resolvable sibling, **and always uses it for a remote recording**, since `run:` composition is host-resolved. Remote recordings are therefore the main producer of the affected artifact. ## The fix Add `device` to `DEVICE_BIND_KEYS`. That fixes both halves at once — `stripDeviceKeys` keeps it out of new recordings, and `bindDeviceArgs` replaces an id already committed to a flow file. Safe because `bindDeviceArgs` only injects keys the target tool declares, and across all 75 registered tools **exactly one** declares `device` — `flow-execute` itself. (24 use `device_id`, the rest `udid`; both were already bound.) ### `platform` is deliberately not stripped There is a test pinning this so a later symmetry edit fails loudly. Two independent reasons: 1. It is read **only when no device was given** — `resolveFlowDevice` returns on `opts.device` before touching it, and the chromium boot spec is gated on `!params.device`. Once `device` is bound it is inert. 2. The strip is schema-blind, and `platform` is *not* device-specific on every tool: `react-profiler-analyze` declares its own. Stripping it would silently rewrite a recorded Android profile analysis into an iOS one. ## Verified live ``` replay, flow bakes in the all-zeros id, run given a real device: before -> nested device 00000000-…, ok false, errored 1 after -> nested device BC0026C7-…, ok true, errored 0 record leg -> recorded YAML has no `device:` key ``` Five new tests, each confirmed to fail before the change and pass after: the two unit cases, an integration case through the runner, the record-side case (using a **non-resolvable** target — a resolvable sibling records as `run:`, which carries no args and so could never show this), and the remote-persist case. Full tool-server suite 3091 passing; lint, prettier, `typecheck:tests` and the tool-description gate clean. ## Behaviour changes worth knowing - **Flows with a baked-in `device` stop pinning** and follow the run device. That is the fix and the documented contract, but it is a silent semantic change for anyone who noticed the pinning and relied on it. No migration is possible or desirable — the baked-in id is host-specific and is exactly what made those flows non-portable. - **A nested `flow-execute` can no longer target a device other than its parent's.** Verified nothing in the repo does this (no tracked `.argent` flows, no flow references in the e2e scripts or workflows), and `run:` already behaved this way. The loud case is a nested Chromium flow that used to boot its own Electron instance; the quiet case is a nested step with a different `platform`, which now silently runs on the parent's device. ## Related - #606 (a nested flow's failure does not fail the outer step) is what let the transcript above report `PASS`. Independent in code — #606 is in the result inspection after dispatch, this is in the args before it — so they do not conflict. Worth knowing when reviewing #606 that the "nested run errored" symptom there had a second, separate cause, fixed here. - `extractDeviceArg` in `http.ts` keeps its own parallel `udid`/`device_id` list and also omits `device`, so a top-level `flow-execute` call gets no platform attribution in telemetry. Not touched here — it is telemetry-only, and unifying it would create an import direction that does not exist today. - `project_root` / `flow_file` are absolute host paths baked into recorded steps — the same portability family, but they cannot simply be stripped (the nested call needs them) and rebinding needs a design decision. Filing separately. Co-authored-by: Filip131311 <f.kaminski2000@gmail.com> |
||
|
filip131311
|
e24b7cac20 |
fix(flows): let a nested run's verdict reach the run that contains it (#697)
Fixes #606. ## The bug A flow step that runs another orchestrator reported `pass` whatever the nested run did. `case "tool"` treats any non-throwing result as a pass, and both `flow-execute` and `run-sequence` report failure *in their result* rather than by throwing. Reproduced live — the **same flow**, run two ways: ``` run DIRECTLY -> ok=False passed=0 failed=1 run NESTED via raw `tool: flow-execute` -> OUTER ok=True passed=1 failed=0 step status = pass sub-report right there: ok=False failed=1 ``` The failing verdict was sitting inside the very object being reported as a pass. A second shape lost the verdict entirely: a sub-flow whose `executionPrerequisite` was never acknowledged returns a notice, runs **zero** steps, and also reported a green pass. ## The fix Two shapes, mapped to statuses the runner already has: | nested result | status | why | |---|---|---| | `ok: false` | **fail** | the composed flow ran and its assertions failed — what an inline `run:` composition already produces | | `notice`, zero steps | **error** | never runnable as written; the class the runner already uses for an unreadable fragment or a cyclic reference | | `aborted: true` | **skip** | matches the runner's own rule that a cancelled step is a skip, never a failure | Both fail and error hard-stop — in this runner *every* fail and error does (`state.stopped`), and there is no continue-on-failure concept: a per-step `optional:` is rejected at parse time because `when:` already expresses it. ### `run-sequence` had the same hole Found while reviewing the plan. `run-sequence` has **no verdict field at all** — every failure path (disallowed tool, unsupported operation, unmet `await-ui-element`, a tool that threw) pushes an `error` entry, `break`s, and returns normally. So a flow step whose sequence stopped at step 1 of 8 also reported a pass. Fixed here rather than left as a known identical bug on the same line. ### Why not a blanket `ok === false` rule There is no `ok` contract in this codebase to generalise. The only other soft-verdict tool spells it `success` (`await-ui-element`), `run-sequence` spells it neither way, and `case "tool"` dispatches tools whose results are typed `unknown` or `Record<string, unknown>` — several carrying app-derived payloads. A blanket rule would silently bind all of those, and every tool added later, to "a key called `ok` decides my flow's verdict". `isUnmetUiWaitResult` set the precedent for naming the tool instead. **There is a test pinning this**, so a future blanket refactor trips. Verified exhaustively: exactly one registered tool returns a top-level `ok` in its result — `flow-execute`. In particular `settings-permissions` does *not*; its `{ok: false}` is a private per-`pm`-invocation type that either throws (already an error) or returns `applied`/`skipped` (a legitimate pass). ## Verified live ``` raw-b-fail direct -> ok=False failed=1 nested -> OUTER ok=False failed=1, step fail reason: flow "b-fail" failed: 0 passed, 1 failed, 0 errored (await: …) sub-report attached raw-b-prereq nested -> OUTER ok=False errored=1, step error reason: flow "b-prereq" did not run — its execution prerequisite was not acknowledged: Settings must be open. Add prerequisiteAcknowledged: true to the step's args, or compose with run: instead. ``` The direct and nested verdicts now agree, which is the exact discrepancy in the issue. 11 new tests in a new file; the 5 behavioural ones each confirmed to fail before and pass after. Full suite 3097 passing; lint, prettier, `typecheck:tests` and the tool-description gate clean. ## Reporting shape One failing step carrying a summarising `reason` **plus the whole sub-report in `result`** — the pass path already attached `result`, so this is the same shape with a non-pass status. The nested steps are deliberately *not* spliced into the outer `steps[]`: `run:` can expand inline only because it shares one `ExecState` (one index sequence, one depth base, one device, one baseline dir), whereas a raw `tool: flow-execute` is a separate runner invocation. Splicing would mean renumbering indices and re-homing artifacts — a wire-format change for a bug fix. Nothing is lost: MCP renders `result` for any step that has one, and the CLI (which renders only `reason`) gets the sub-flow's own first failure inside the reason string. No new `StepReport` fields, no wire-format change; older clients render the new line unchanged. ## Merge ordering and conflicts - **Merge #696 (#607) first.** Before it, a nested `flow-execute` could run against a stale baked-in device and legitimately report `ok: false`; with this landed that becomes a parent failure and would read like a regression *caused by* this PR. No code-level conflict — #696 touches `flow-device.ts` and three other test files. - **#578 rewrites this exact block** (`case "tool"`, adding an `evidence` code to each return) and adds `StepReport.failure`/`durationMs`. Whichever lands second should give the new branches an evidence code — likely `nested-flow-failed` / `nested-flow-prerequisite-unacknowledged` / `nested-flow-aborted` — so its CI diagnostics classify the composition case. **#677** also edits `execLeafStep` and adds `StepReport.warning`. ## Behaviour change worth knowing Previously-green flows containing a nested composition that was silently failing will now go red. That is the fix, but it surfaces pre-existing breakage on upgrade. The likeliest one is a recorded raw step missing `prerequisiteAcknowledged`, which becomes a hard error instead of a silent no-op — the reason names both remedies. ## Follow-up, deliberately not bundled Raw `tool: flow-execute` nesting has **no cycle or depth guard**: `MAX_RUN_DEPTH` and the run-stack cycle check cover only `run:`, so a flow whose raw step names itself recurses through fresh runner invocations. Independent of this issue; filing separately. --- > **Stacked on #649** (`filip/flow-deviceless-and-counts`). Both edit the import block of `packages/tool-server/src/tools/flows/flow-run.ts` — #649 widens the `./flow-device` import for its device-optional run, this one adds `./flow-nested-outcome`. Rebased on #649, so review only the top commit. GitHub retargets it to `main` when #649 merges. Co-authored-by: Filip131311 <f.kaminski2000@gmail.com> |
||
|
filip131311
|
2278115028 |
fix(flow): run a device-less flow without a device, and say why its counts are zero (#649)
Fixes #644. ## Before ``` $ argent flow run echo-only # .argent/flows/echo-only.yaml: two `echo` steps [Tool:flow-execute] No booted device found. Pass a device id or platform explicitly. Available devices: 18BE573F-… (ios, Shutdown), <70 more> ``` and when a device *was* booted: ``` PASS on 18BE573F-… — 0 passed, 0 failed, 0 errored, 0 skipped ``` ## After ``` $ argent flow run echo-only # nothing booted Flow "echo-only" › first step › second step PASS — 0 passed, 0 failed, 0 errored, 0 skipped (no test steps) ``` ## Symptom 1 — the device Whether a device is needed is decided from the flow's own steps, before resolution. Classification is **per step kind and defaults to needing one**, so a kind added later inherits today's behaviour instead of silently running against no device — and the `never` binding makes leaving a new kind unclassified a compile error, backed by a test keyed on the step union so it fails to build rather than to run. Two classifications worth calling out: - **`when` always needs a device**, whatever its body contains, because the guard reads one itself (its platform, or its view tree). A `when: {platform: ios}` block containing only narration still resolves. - **`run` needs one without the fragment being read here.** The named flow is resolved at run time; resolving it a second time would duplicate that lookup and could disagree with it if the file changed in between. A deliberate over-approximation: composing a narration-only fragment still resolves a device. Stated in the code and in the tool description. For `tool:` steps the tool's own schema decides, via a superset of the keys the runner injects — `device` is included because a nested `flow-execute` step drives a device without being handed the run's own. A tool that declares no input at all (`stop-all-simulator-servers`) is device-free, and reading its absent schema must not throw — it previously would have. `ExecState.device` is nullable, which is the safety mechanism rather than just typing: the compiler enumerates every site that acts on a device, each of which now says so via one `deviceEnv()` narrowing. `flow-actions.ts`, `flow-visual.ts` and `ActionEnv` are untouched. The executor also re-checks each step against the **same predicate**, so if the scan and the executor ever disagree the step reports it instead of failing obscurely deeper in. A run that resolved no device reports `device: ""`, and neither renderer claims it ran somewhere. ## Symptom 2 — the counters Reproducing this showed the issue's framing is slightly off: `--json` doesn't only disagree with the text, it disagrees with **itself** — counters all zero while its own `steps[]` marks both steps `"pass"`. So the CLI text was a faithful rendering of the server's counters. **The counters are right and stay as they are.** Narration is deliberately excluded, and it should be: after a hard stop every remaining step *including narration* is reported skipped, so counting it would inflate `skipped` on every real failing run — a worse and far more common lie than the vacuous one being reported here. It would also contradict the JUnit model in #578, where echo is `<system-out>` and not a `<testcase>`. What was missing was saying *why* zero. Both the CLI and the MCP renderer now append `(no test steps)`, and only on a pass — a cancelled run can be a FAIL with all four counters zero, where "no test steps" would read as though the failure had no cause. No wire format changes: `steps[].kind === "echo"` already distinguishes counted from uncounted steps, so the JSON is self-describing without widening the status enum. ## Conflicts, for whoever lands second This is a hot area — seven open PRs touch `flow-run.ts`. - **#578** rewrites the CLI summary line I also change (one hunk, one 7-line function, semantically independent — resolution is keep-both), and its `attachFailureDiagnostics` dereferences `env.device.platform`. **It will need a null-device guard**, or every failure in a device-less flow silently degrades to `read-failed` diagnostics. Happy to rebase onto it rather than the reverse. - **#585** rewrites `resolveRunDevice` at the same seam and edits the same description sentence. - **#538** rewrites `flow-device.ts`. Worth noting it makes `resolveFlowDevice` reject a physical iPhone for flows — skipping resolution means a narration-only flow now passes on a physical-iPhone-only host, which I think is desirable but is a second behaviour change on this seam. All new tests are in new files, since every existing flow test file in the blast radius is rewritten by an open PR. ## Behaviour changes a reviewer should weigh - A device-free flow no longer prints `PASS on <id>` **even when a device is booted**. That is the point: otherwise an identical flow reports differently in CI (no simulator) than on a laptop with one open. An explicit `--device` is still honoured and still named. - `--platform` is ignored for a device-free flow, where it previously errored with nothing booted. - A 0.18.1 CLI against a newer server would print `Flow "x" on ` — cosmetic version skew. ## Verification Live against the built CLI: device-less flow with nothing booted (the reported bug) and with a device booted (identical output, no ambient attribution); `--json` showing `device: ""`; explicit `--device` still naming it; a mixed flow and a composed flow both still demanding a device; a mixed flow with a device booted still reporting `PASS on <id> — 1 passed`. 24 new tests. tool-server 3103 passed, CLI 284 passed, MCP 78 passed — no existing test modified. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- > **#697 is stacked on this branch** — it conflicts with this one in `flow-run.ts`'s import block, so it was rebased on top. Merge this first. |