Follow-up to the echo-numbering fix: with narration unnumbered, a `failure`
hung on an echo step — which the runner never produces, since echo only passes
or skips — would head its block with the previous step's number, or with 0
before any real step has run. The CLI's `renderFailures` already drops them for
exactly that reason; the MCP renderer now matches.
Refines the screenless-capture fix: the composition/launch branch was checked
before the device-free one, so a device-free composition failure — exactly what
the `no-device` screen state was written for — would have reported
`never-readable` with "this step failed before it reached the device". True,
but the less precise of the two answers about a run that never had a device at
all.
- The JUnit reporter is PARSED, not grepped. Every assertion was `toContain` or
a regex, which passes wherever in the document a fragment sits — which is
exactly how an `<error>` in a position no schema allows survived the suite.
`flow.test.ts` validates the document it actually writes, too.
- The MCP failure block gets a verbatim pin. The section was entirely
`toContain`, so an injected extra line or a silently dropped slot survived.
- `inlineRole` — the headline MCP change in this diff — was unreachable by the
suite: the two near-miss tests set `secretTyped` (killing the flag it sits
behind) and passed no ctx. Both arms are now driven, and `MAX_RENDER_FAILURES`
has a fixture that reaches it.
- The other new code the suite never executed: `wireTimestamp` (load-bearing —
removing it makes `buildJUnitXml` throw), `resolvedPlatform`, the JUnit body's
`expected:`/`actual:`/`match:`/`reads:`/`device:` slots, and
`sink.error = undefined`, the only stale-error clear, which now has a test
that recovers after a read failure.
- The "unclassified" test asserted nothing: its body was `if (failure) { … }`,
vacuous when the failure is absent — and its fixture produces a PASSING step,
so the guard never opened. `unclassified` is now driven at the assembler, the
site that decides it, with a literal pin.
- `expect(indexOf(a)).toBeLessThan(indexOf(b))` is satisfied by `a` being absent
entirely (-1 < N); presence is asserted first.
The nested-`flow-execute` secret case the harness could not express is covered
by the device-scoped latch's own two-run test.
- The `failure.code` table advised a `scroll-to` (or an `await`) for
`selector-not-visible`, which it defines two columns earlier as "everything
that matched had a zero-area frame" — the producer's own message says
scrolling cannot reveal it, and the code only arrives after the full timeout
has expired, so the `await` is inert too. That remedy belongs to the
neighbouring `selector-not-found`.
- "The whole indeterminate family carries no `tree`" is false for
`condition-dark-tail`, which attaches its last trusted read and renders a
full element list. "A run that typed a `{{secret:…}}` value carries no
`screenshot`" understated the guard (it is per-device, not per-run) and
overstated the protection (a failed `snapshot:` still exports its own images
of that screen).
- The `screen:` decoding table quoted only the CLI's strings, while this
skill's reader consumes the MCP block, which words them differently and has
no indeterminate marker at all — so a dark-tail failure's stale tree read
exactly like the screen that failed. Both spellings are now listed.
- The `flow-execute` description had a spliced seam leaving a dangling em-dash
clause on the wrong sentence, and claimed "Each step carries `durationMs`",
which skipped steps do not. This string is a contract an LLM reads.
- The taxonomy list claimed `gesture-geometry-unsatisfiable` cannot be driven
honestly. That is true of the pinch arm only: the rotate arm needs a target
with no fitting orbit, and `flow-rotate.test.ts` already drives it. The
fixture now pins the code so the two files stay tied together.
`hasContent`'s false-equivalence comment went with the code change that made it
true (`shouldEmit` is now what both the report and `describe` gate on), and the
`displayOrdinal` comment describing the MCP renderer as echo-skipping became
accurate when the renderer started skipping it.
Producer:
- `assert-hidden-unmet` now fills `actual.element`, the shape both renderers
document their `match:` slot for. Candidates are suppressed there and bare
counts render nowhere, so the block carried no element evidence at all.
- The element list uses `shouldEmit`, not `hasContent`. The latter is half of
describe's gate; the role half is what keeps unlabeled AXButton/AXStaticText/
AXImage/AXTextField nodes visible, so the count undercut the `describe` the
report tells the operator to compare against.
- A `selector-not-visible` miss no longer advises a `scroll-to` while its own
hint says scrolling cannot help — reason and evidence are derived together.
- `hint` is byte-capped like every other scrubbed field; it is not always a
literal, and `trimToBudget` cannot shed it.
- `evidence.observation` is scrubbed and capped instead of spread raw — the one
hole left in "every string that reaches the wire is masked".
- The byte budget reserves room for the `overflow` handle that is attached
after the final measurement, and the evidence directory is swept at the spill
as well as at the tree dump.
- A candidate for a hidden node is refused when its derived selector resolves
to a different element — the retargeting `resolvesToNode` exists to prevent,
reached through its lenient branch.
- A capture that was ATTEMPTED and came back empty says so
(`screenshotOmitted: "capture-failed"`), instead of vanishing on every
non-environmental failure.
- The device-free guard, the fourth failure-emission site, now attaches
diagnostics like the other three.
CLI:
- `--reporter default:` is rejected, like `default::` and `default:x`.
- `time=` stays a decimal for an absurd duration, and is omitted entirely when
a step was never measured — "never measured" is not "took no time".
- Flows a stopped batch skipped get a `<skipped/>` suite instead of vanishing
while the terminal counted them.
MCP:
- `match:` and candidate rows carry the visibility marker the CLI derives, and
a frame missing width/height yields no tap centre at all. The zero-area shape
whose whole diagnosis is "hidden" was the one losing the marker.
`projectNode`'s password redaction still covers only Android and Chromium; the
comment now says so rather than claiming more. Closing it needs the flag set in
the iOS and Vega tree adapters, which is outside this area.
`execWhenStep` set `durationMs` only on the guard-error path; the pass marker,
the skip marker and `execRunStep`'s marker never did. A UI guard probes with
the assert grace, and an unmet one spends the whole of it — on a flow with one
unmet guard the step durations summed to 1825 ms against a run total of
2956 ms, so 1131 ms was invisible in the step list, in `--json` and in JUnit,
where both testcases reported `time="0.000"` under a suite reporting 2.9 s.
Optional interstitials are the documented primary use of `when:`, so a suite
full of guarded blocks hid most of its runtime from the feature whose stated
point is showing where time goes.
The `run:` marker is timed with its own cost — canonicalizing the target, the
case check, the read and the parse — which is what it is measurably
responsible for; the fragment's expanded steps carry their own durations.
A step inside a block that never ran still reports none, which is honest.
The MCP step number was the raw array position (`step.index + 1`), which counts
`echo` narration. Nothing else does: the server's own `displayOrdinal`, the
`failure.step.ordinal` it puts on the wire, the CLI's step list and failure
block, and the `step-NN-*` filenames the export writes all skip echo. So a flow
with an `echo:` before the failing step — the idiom the skill docs prescribe —
had the MCP block heading say "3)" while the CLI said "2)", the evidence file
beside it said step-02, and the wire said ordinal 2.
Narration is now unnumbered, carrying the CLI's `›` marker, and the ordinal
counts only real steps. Derived rather than read off `failure.step.ordinal` so
the heading and the step list above it stay in lockstep by construction, which
is what `displayOrdinal`'s comment is actually about.
The verbatim fixture that let this through pinned the wrong number as correct;
it now pins the right one.
`junitSuite` emitted the `incomplete` case as `<error type="run-incomplete">`
directly under `<testsuite>`, where JUnit's content model — `properties?,
testcase*, system-out?, system-err?` — allows no such element. A validating
consumer rejected the whole document, which in CI means no annotations at all
out of a file that exists and looks plausible.
And because `tests` counts steps, a rejected or cancelled flow shipped
`tests="0"` with zero `<testcase>` elements: any consumer that derives results
from testcases, which is most of them, read an empty GREEN report for a build
that exited 1.
Both go away with the conventional encoding pytest and surefire emit — a
synthetic `<testcase name="run">` wrapping the `<error>`, counted in `tests`.
It carries no `time`: the case was never measured, and charging it the suite's
wall clock would double-count against the real testcases.
The reporter tests now PARSE the document instead of grepping it. Every
assertion was `toContain`, which passes wherever a fragment sits — which is
exactly how an element in an illegal position survived the suite.
A rejection (bad YAML, an unknown step key), a transport error or a non-report
result returns through `exitAfterFlush` before the only `writeReporterFiles`
call, so the process exited 1 having written nothing.
In CI that is worse than a wrong file: the publisher picks up whichever
junit.xml sits at that path, which is the previous run's, so a red build
reports the last green build's results. The directory path already synthesises
a suite for exactly this case — its comment calls a `failures="0"` file for a
run that exited 1 "the one thing a CI artifact must never do" — so the two
paths simply disagreed. Both now go through `rejectedFlowReport`, carrying the
real rejection message.
Both renderers suppress the `screenshot:` line when the step carries its own
`current` image: on a snapshot failure the producer reuses that handle, so one
picture must not be listed under two paths. The guard cannot tell a path from
prose, so it swallowed the omission note as well — a snapshot step that failed
after a secret was typed printed neither, in the terminal block AND in the
JUnit `<failure>` body, which is the artifact CI publishes.
The suppression now bounds paths only. A note beside the snapshot's own image
paths would over-promise on its own, so it comes with a warning naming the
roles that hold the same screen — on a diff those are annotated, with the
changed pixels boxed in red.
The export itself is unchanged and out of scope: `--output` copied every role
of a failed snapshot on the merge base too, with no secret awareness anywhere
in that path.
`resolveScreen` short-circuits for three shapes with no screen of their own —
an aborted run, a launch that never started the app, and a device-free run —
and every reason it gives applies to a full-resolution capture too.
`captureScreenshot` checked none of them.
So a `run-cyclic` failure, a pure YAML authoring error on a flow that never
touched the device, did a post-hoc tree read AND captured whatever app happened
to be foregrounded, and `--output` exported the pair as this failure's
evidence. Composition failures now short-circuit alongside launch-failed, and
the resolution carries a `screenless` verdict the capture obeys.
Declining leaves the slot empty, which the renderers were guessing at: the
environmental fallback said "the device did not return an image", the opposite
of what happened. The producer stamps `data.screenshotOmitted: "no-screen"`
instead, and both renderers key their note off that field — an unknown reason
renders no line, so a newer server's vocabulary degrades to silence.
`typedSecret` lived on `ExecState`, so the guard covered exactly one
`flow-execute` invocation. The screen it protects has no such scope: a
credential one flow types is still rendered when the next flow runs, and a
flow with no leading `launch:` runs against whatever is on screen by design —
which is what a directory run does, flow after flow, on one device.
So a flow containing no secret, and typing nothing, captured and exported the
previous flow's credential: `--output .../step-01-screen.png` came out at full
resolution with the value legible. The same gap defeated the latch through
`tool: flow-execute` in both directions, since a nested run is a separate
invocation with its own run state but the same device.
Latch on the device instead, in a process-scoped set the runner writes and the
diagnostics read. Never cleared, for the reason the run latch was never
cleared — the runner cannot know when the app stops rendering the value back —
which is the doctrine `createSecretScrubber` already applies to text, applied
to the one projection no scrubber reaches.
The renderers' note says "onto this device" rather than "by this run", since
the run reading it need not be the one that typed the value.
`data.screenshotOmitted` was stamped onto `report.failure` after `withBudget`
returned. That worked, but it put the field outside the object's construction:
it was applied after `applyByteBudget` had measured and trimmed, and after the
overflow spill had serialized its copy — so `step-NN-failure.json` recorded a
payload without the marker while the wire payload had it.
`baseFailure` is the one constructor every path goes through, including the two
that emit it alone (a capture timeout with nothing assembled, and the
assembly-threw catch). Setting it there covers the same paths with one site
instead of a trailing special case, and puts the field in place before anything
measures or serializes.
No behaviour change on the wire. The byte-budget angle is ordering being
correct by construction rather than a bug anyone has hit: the marker is 35
bytes against a 24 KiB cap, and the shedding loop lands far below it, so a late
stamp did not breach the budget. Both comments now say that rather than imply
a fix, and the accompanying test says what it pins rather than claiming to
catch the earlier ordering.
A third review pass, over the seven fixes in e79f0aff.
- The MCP secret gate failed open on both degraded-diagnostics paths. The
renderer decides whether to inline a snapshot's own images from
`failure.data.screenshotOmitted`, which `buildFailure` set only AFTER
`await resolveScreen`. `baseFailure` writes no `data` at all, and it is the
entire payload on the two paths that skip that await: a capture timeout with
nothing assembled yet, and the assembly-threw catch. A snapshot failure is
exactly the shape that reaches them — snapshot evidence carries no tree, so
the assembler falls through to a post-hoc read that can overrun the budget —
and the pixels then went to the agent. The marker is now stamped last, on
whichever payload the paths above produced, so it cannot be missing while
the run's flag is set. `typedSecret` is run state and never reaches the wire,
so this field is the renderer's only signal and has to be unconditional.
- `meta.incompleteMessage` was the one message attribute in `junitSuite`
reaching `xmlEscape` without `wireText`. It is the transport error from a
possibly-remote tool-server — the untrusted wire data this module's header
says is normalized once, here — so a multi-line rejection put raw newlines
and an unbounded string into a CI attribute.
- `rankCandidates`'s JSDoc was orphaned: inserting `VERIFY_ALLOWANCE` and its
own doc block between the comment and the function detached it, so the
"`total` counts PROPOSED suggestions" clarification added in the previous
commit documented nothing. The constant moves above it.
Both behavioural fixes are pinned by tests that fail without them.
A mutation and correctness pass over the previous 20 commits. Each item was
reproduced before being fixed.
- A raw NUL byte in source. The H1 secret-scrubber dedupe key used a literal
U+0000 as its separator (flow-failure.ts:550). It ran fine and passed tsc,
eslint and prettier, and `grep` reported the file as binary — nothing in CI
could catch it. Now an explicit `` escape.
- The typed-secret screenshot guard leaked anyway on a snapshot failure. A
snapshot registers `current` itself, independently of the diagnostics, so
declining `failure.screenshot` removed only a pointer — and the previous
commit's change to inline `current` when there is no `diff` then put those
pixels straight into the agent's context, directly above the line saying a
capture "could reveal it". The renderer now inlines neither image when the
run typed a secret. Paths still print.
- `normalizeFailure`'s root-flow guard was a tautology. It compared
`failure.step.flow` against `fallback.flow`, which the callers derive from
the same step — so it always held and every nested fragment's failure named
the ROOT flow's file. The callers know both names and now decide. The test
that "covered" this passed for the wrong reason: its fixture set the
fragment name on `failure.step` but not on the step report, a shape no
tool-server emits.
- The emitted partial was still aliased to the running capture. A screenshot
resolving after the deadline landed on the payload the runner had already
emitted, making the field a timing race — and the image is of a screen that
has since torn down. The timeout now copies before trimming.
- The scroll reclassification contradicted its own message. `reason` is built
at the call site and `failure.message` is pinned byte-identical to it, so a
zero-area match shipped `selector-not-visible` beside "reached the end of
the scroll" — the same defect the wait verdict was just fixed for. The
reason is now rewritten with the code, and the reclassification is confined
to the end-of-scroll exit: running out of ITERATIONS means the container was
still producing content, where nothing can claim scrolling cannot reveal it.
- Candidate rows with unusable suggestions crowded out usable ones. Splitting
the suggestion into a cheap half and an expensive half made the dedupe key
stop depending on whether a suggestion resolves, so five same-rectangle rows
each took a slot and pushed the one that does resolve out of the list. The
emit loop now collapses them, with verification bounded to a constant
multiple of the output so the cost fix stands. `total` counts PROPOSED
suggestions, which the doc comment now says rather than implies.
- A directory run's JUnit file reported green for a red run. A flow the
tool-server rejects before running it produces no report, and those were
filtered out — leaving failures="0" errors="0" beside exit 1. Each now gets
its own suite carrying the rejection reason.
Also: the `flow-execute` docstring claimed "Each step carries `durationMs`"
when skips omit it (flow-run.ts:1856) — and every step after a failure is a
skip, so on the failing run that paragraph describes, most steps have none.
The evidence-sweep test's 1s wait is widened; it stats a directory shared with
every other run on the machine and flaked once under load.
The header claimed "adding a code fails here until it is listed AND driven
from a real run". The list closes the first half; the second half is the
individual tests, and it is not total.
Two members are defensive branches a parsed flow cannot reach:
`step-kind-unsupported` is `execLeafStep`'s `default:` arm, which every
`FlowStep` kind already has a case for, and `gesture-geometry-unsatisfiable`
needs a pinch whose finger travel rounds to exactly zero — a zero-area target
reports `selector-not-visible` before it resolves to a frame, and any span
that does resolve leaves positive travel however small (measured against a
1e-9 frame, which passes).
Naming the gap beats a comment that promises coverage the file does not have.
`FlowFailureScreen` is discriminated, so `screen.reason` only exists on the
`unavailable` arm. `tsc --noEmit -p tsconfig.test.json` — the CI step that
typechecks tests, which the runtime suite does not cover — rejected the
unnarrowed access. Assert the shape instead, which also pins the state the
reason belongs to.
`callTool` was a `vi.fn()` that never invoked `onProgress`, so `liveSteps` was
always 0 and the buffered renderer owned every assertion in the file — the
branch a modern STREAMING tool-server takes was unreached. It now runs, and is
identified by the header it prints (no device) rather than `renderReport`'s.
Six renderer paths the PR added and left unpinned: the `invisibleMatches` →
`match:` row (with visibility DERIVED from the frame, never from wire prose),
the `carriesCause` branch that keeps a tree-source error visible on an
environmental failure, `normalizeScreen`'s `(captured after the failure)` and
`(last trusted read)` suffixes, `normalizeExpected`'s scroll arm, and the
candidate row's `selectorYaml`/`note` columns.
Also pinned: the pre-`determinacy` prefix inference — the ONE place a code
STRING drives CLI behaviour rather than being printed verbatim, and therefore
the only place a renamed `FlowFailureCode` can change what an operator reads.
`twoFailureRun` now says why it builds a shape the runner cannot emit: it is
the buggy-or-hostile server the renderer's bounds exist for, and a
single-failure fixture could not tell a RUN image budget from a per-step one.
The head commit shipped five source files and no test lines. Its three
behaviours now have assertions:
- All four `waitForIdle` tree-source outcomes were executed but never had a
single `step.failure` assertion. Each now pins `tree-source-unavailable`,
the indeterminate determinacy, and the SUPPRESSED screen slot — which is
the whole reason for classifying them, since an unclassified code defaults
to determinate and sends the assembler reading the source that just wedged.
- `launch-failed` was asserted only on code/category/message. It now asserts
that the post-hoc read is not attempted at all (counted locally: the module
mock is shared across the file and never reset).
- The `no-device` screen state appeared in no test. A `run:` step requires a
device, so the device-free shape is a flow whose steps declare no device
argument; the test drives one and asserts the state, the absent platform,
and that no read was made.
`selector-scope-unresolved` and its determinacy guard had no end-to-end test —
`diagnoseScope` was unit-tested in isolation while the rewrite that consumes
it never ran through the runner. Both arms are now driven: a determinate
failure gets the rewrite, and an indeterminate one keeps its code, category
and determinacy. The guard arm needs a TRUSTED tree that is missing the scope
followed by a dark window; with no tree at all the scope is never diagnosed
and the guard is never reached, so a simpler fixture would have proved
nothing. Verified by removing the guard and watching it fail.
`unclassified` is pinned as a wire value that always carries a category.
`flow-visual.ts` ships five evidence payloads and only `snapshot-baseline-missing`
was proven. Worse, the one relevant assertion was LOOSENED: `toHaveBeenCalledWith(env,
cropOn)` became `(env, cropOn, expect.anything())`, which cannot distinguish
the evidence sink from any object at all — and the `waitForFrame` mock ignored
the third argument and never wrote to it, so the payload derived from that sink
was never exercised.
The mock now writes to the sink the way the real call does, the assertion pins
the sink's shape and that it was written, and `snapshot-diff`,
`snapshot-dimension-mismatch`, `snapshot-crop-empty` and both arms of the
cropOn miss are asserted. The miss splits three ways, which the prose-only
tests could not see: no readable tree is `tree-source-unavailable` (the
selector was never evaluated), a readable tree without the element is
`selector-not-found`, and a zero-area match is `selector-not-visible`.
Two assertions in flow-failure-budget also allowed more than the budget they
guard: `FLOW_FAILURE_BYTE_LIMIT * 2` accepted 47 KB from a 24 KB cap and was
the only assertion on the trimmed size, and `FIELD_BYTE_LIMIT + 8` cited an
`it.fails` that no longer exists. Both now assert the real limit, which the
implementation already meets.
Five claims the code does not make good on:
- `reliability-and-recovery.md` said the diagnosis is captured "at the moment
of failure" and presented `screenshot`/`tree` as always present. The screen
can be a post-hoc read tagged `(captured after the failure)` or a `(last
trusted read)`, `tree` is absent for the entire indeterminate family, and
a run that typed a secret now carries no screenshot. The passage now has a
table mapping each `screen:` line to which read it came from, and step 4's
"teardown has since run" is rewritten so it cannot be read as a claim about
when diagnostics run (they run inside `execSteps`, well before teardown).
- `argent-test-ui-flow/SKILL.md` still said "classify the failure, inspect the
actual screen" — the instruction the PR replaced with its opposite in the
skill it links to. The two now agree.
- The `flow-execute` tool description listed the pre-PR report shape. It is
the one text an agent reads, and the new skill guidance ("start from the
failure block") assumed a contract it never stated.
- `flow-yaml.md` listed the `flow run` flags without `--reporter` or
`-r/--recursive`, and said `--output` writes only baseline/current/diff.
- Two comments justified scrubbing with a `{{secret:…}}`-in-a-selector path
that cannot occur — the parser rejects one. The scrubbing is right; the
reason is that a selector and an expectation quote ON-SCREEN text, which is
exactly where an echoed credential lands.
`expected:` was gated on `expected.text`, so only a text condition filled the
slot. The `scroll`, `snapshot`, `gesture` and text-less `condition` arms
rendered nothing at all — `condition: "visible"`, `timeoutMs` and
`maxMismatch` never reached the agent, though the CLI has rendered all four
since the block existed. A bare gesture expectation that only restates the
step kind is still suppressed, the same rule the CLI draws.
`actual.invisibleMatches` was absent from the MCP wire type entirely. Both the
producer and the CLI call it "THE diagnosis for selector-not-visible": the
element the selector named IS on the tree, it just has a zero-area frame, and
its candidate list is deliberately empty because no other element was meant.
So the one failure shape whose fix is "find out why it has no size" reached
the agent with no element at all. It now renders a `match:` row carrying the
element's tap centre, as the CLI does.
On a snapshot failure `failure.screenshot` is the SAME stored artifact as the
step's `current` — the producer reuses the handle deliberately, because a
second capture would show a different screen than the one diffed. Both
surfaces then treated it as two images.
`exportRunArtifacts` copied it twice: `home__chromium-1800x1336-current.png`
and `step-03-screen.png` came out md5-identical, and against a remote
tool-server the same bytes were downloaded twice too. It now points the
failure at the copy the snapshot roles already made, matched by artifact id
(the report is parsed JSON, so the producer's one handle is two objects by
the time the CLI sees it).
The JUnit body listed four paths for three images; it now draws the same
suppression the terminal block already had.
The MCP renderer printed the same image under two different path strings and,
on the three snapshot shapes that carry `current` but no `diff`, spent the
run's single inlined image re-rendering it. The step's own artifact block now
inlines `current` when there is no `diff` — beside the path that names it —
and the failure block leaves the slot to it.
`scrollEvidence` hardcoded `scroll-target-not-found` on both target-miss
returns, including when every match had a zero-area frame. That is the state
`selectorMissEvidence` classifies as `selector-not-visible` on a `tap`, with
the "behind an overlay / collapsed / not laid out yet" hint — and it is the
one state where the scroll code is actively the wrong instruction, since no
amount of scrolling reveals an element that has no frame.
The scroll path now derives its code from the observation the way every other
selector site does. The scroll expectation still rides along, because that is
what the step asked for.
`rankCandidates` called `flowMatchAll` unguarded while the identical call in
`diagnoseScope` twelve lines below was wrapped, with a comment claiming it was
"the one call in this module that can throw". Both can: a selector carrying an
uncompilable `textMatches` throws out of the match engine, and a throw on the
ranking path degrades a perfectly readable screen to `screen: read-failed` —
the module header promises the opposite ("no throwing... a ranking that threw
would replace a real diagnosis with a crash").
`parseFlow` validates every pattern, so this is a contract defect rather than
a crash anyone has hit — which is the reason to state it in the code instead
of relying on the parser staying the only way in.
One shared helper, returning `undefined` on a throw rather than an empty
array: ranking treats both as "nothing to exclude", while `diagnoseScope` must
not report a scope it could not PROBE as one that resolved to nothing.
`$TMPDIR/argent-flow-failure` accreted forever: filenames embed a randomUUID
and nothing removed them. The comment claimed a shared directory "keeps the
entry count constant" — true only of the PARENT's entry count, so on a
host-wide, never-exiting tool-server the contents themselves grew unbounded
(111 entries after a few hours of test runs).
A registered artifact's host path must outlive the CALL, so these cannot be
swept the way flow-visual sweeps its diff scratch — but they can be swept by
AGE. Files older than an hour go on the next evidence write: one readdir per
FAILING run, fire-and-forget so it never spends the diagnostics budget, and
fail-soft per entry so another process's file is left alone.
The directory is also created 0700 (and tightened if it already exists): the
contents are screen dumps and whole failure payloads — masked for the secrets
this run resolved, but still the app's UI text on a world-readable path.
`baseFailure` truncates `message` to MESSAGE_BYTE_LIMIT while
`attachFailureDiagnostics` scrubbed `reason` and left it uncapped. Both copies
ship on every NDJSON progress event, so bounding one of two bounds nothing —
and the invariant every renderer relies on, `failure.message` byte-identical
to `reason`, then held everywhere EXCEPT above the cap, which is the one case
the cap exists for.
Reachable wherever a `text` assertion runs against a container: the flow
adapters hoist a container's whole subtree text, so one mismatch against a
screen-sized element produced a six-figure-byte reason.
`MAX_WIRE_ELEMENTS` was exported as "matches the producer's
FLOW_FAILURE_ELEMENT_LIMIT; re-applied on receipt", but `normalizeScreen`
never slices `elements` — it reads only `.length` as a count fallback and
renders no element list at all, so there was nothing to clamp and no walk to
bound. A named constant asserting a guard that isn't there is worse than no
constant.
`MAX_WIRE_FIELD_CHARS` and `MAX_WIRE_CANDIDATES` are real and used, but only
inside this module; knip flagged all three as unused exports (46 -> 43).
`flowFilePath` always built `.argent/flows/<name>.yaml` from the wire
report's flow NAME, which is a guess — and a wrong one for both documented
invocations that don't live there. Running `~/shared-flows/checkout.yaml`
printed `@ .argent/flows/checkout.yaml` (no such file), and every nested flow
of a recursive directory run emitted `argent.flowFile=".argent/flows/two.yaml"`
for a flow at `.argent/flows/batch/two.yaml`.
The CLI already holds the resolved path at every call site, so thread it
through to the block header and the JUnit property. A step belonging to a
NESTED flow keeps the derived guess: the resolved path is the root flow's
file, not the fragment's, and the convention directory is the better of the
two guesses there.
`xmlEscape` stripped the C0 controls but not U+FFFE/U+FFFF, which XML 1.0
forbids anywhere in a document — there is no escape that makes them legal.
A failure message quoting an on-screen label carrying one produced a file
that neither Python's ElementTree nor xmllint would read ("Char 0xFFFF out
of allowed range"), while `writeReporterFiles` reported nothing wrong. CI
gets zero annotations from an artifact that exists and looks plausible,
which is worse than no artifact at all.
A mis-decoded UTF-16 BOM lands as U+FFFE the same way.
`wireText` had the same gap and now collapses them like the other
non-displayable characters, so the terminal block does not print garbage
either. U+FFFD and the astral noncharacters are legal and are left alone.
`writeReporterFiles` had one call site, on the single-flow path. `flow()`
dispatches a directory to `runFlowDirectory`, which returns via
`exitAfterFlush` and never reads `args.reporter` — while `parseRunArgs`
validates the spec regardless, so the flag looked accepted. `argent flow run
.argent/flows -r --reporter junit:results/junit.xml` is the canonical CI
invocation, and it wrote nothing and said nothing.
JUnit already models this: a document holds many `<testsuite>` elements. Split
the builder into a per-flow suite and a document wrapper whose counters are
the sum, so a directory run writes every flow it ran into the one file the
operator named, each suite carrying the real path it ran from. The
single-flow path is the one-run case of the same wrapper, byte-identical.
`renderFailures`, the indeterminate note and the CI `--output` hint were
unreachable on that path too, so a directory run showed only the pre-existing
one-line reason — no code, candidates, screen, hint or evidence paths, even
though the evidence files had already been downloaded and written. All three
now run there.
`captureScreenshot` took an unconditional full-resolution capture with no
secret check. The image is copied to `step-NN-screen.png` under `--output`
and inlined as an image block by the MCP renderer, so a credential the app
rendered back into a non-password field reached the CI artifact and the
agent's context in plain pixels.
This is independent of the text scrubber: pixels are the one projection no
mask can reach, so it leaked even with the secret in `ARGENT_SECRET_*` and
every string in the report correctly masked. Verified against the exported
PNG.
The codebase already declines exactly this capture one layer up — the MCP
auto-screenshot skips whenever a tool input contains `{{secret:`. Apply the
same rule, spelled the same way: the runner latches a flag when a step
carrying a placeholder is about to run (latched, never cleared — the runner
cannot know when the app stops rendering the value back), and the assembler
declines both the fresh capture and a snapshot's reused `current` handle.
The omission is stated rather than silent. `data.screenshotOmitted` drives a
line in both renderers, because an agent that merely sees no image calls
`screenshot` itself — which is the leak the omission exists to prevent.
`waitForCondition`'s tier-3 verdict was computed from `lastMatches` — the
last read that merely SUCCEEDED — while the evidence beside it was built from
`lastTrustedMatches`. A blind read (empty tree, which `everMatched` classifies
as untrusted) empties the former and leaves the latter holding the element, so
a blind read landing inside the 600ms dark-tail tolerance handed `assertVerdict`
a match set it should never have seen.
The result contradicted itself: `code: selector-not-found`, `category:
selector` and "no element matched selector id=…" beside an `actual` reporting
the element, its frame and its text. Same app, same assertion, only the blank
timing changed. `code` and `category` are what a CI dashboard groups by and
what a repair loop branches on, so "fix your selector" for a text mismatch is
the expensive kind of wrong.
Verdict from `lastTrustedMatches` instead. On a trusted final read the two are
the same object, so only tier 3 changes; `lastMatches` had no other reader
left, so the loop's working set is now a `const` that never leaves the loop.
Two mechanisms compounded, and the result failed in the one direction that
cannot be tolerated: the bigger and more confusing the screen, the more
likely the operator got nothing — and the report said "the screen could not
be read" about a screen that had been read in full.
Cost. `rankCandidates` resolved EVERY ranked entry's suggested selector back
through the tree, `limit` notwithstanding: that is O(ranked x nodes) of
synchronous work for rows the slice then discards. Split the suggestion into
its cheap half (derive and spell, pure over the node — enough to dedupe) and
its expensive half (resolve it back), and pay the expensive half only for the
rows actually offered. On an 11 000-element screen whose selector ranked
every node, diagnostics + teardown drop from 7.0s to 1.1s.
Cliff. The capture runs under a 5s `Promise.race`, whose fallback threw away
the screen, selector, expectation, observation and candidates it had already
computed. `buildFailure` now publishes the failure as it enriches it, and the
timeout reports that partial — trimmed to the byte budget synchronously —
falling back to `capture-timeout` only when nothing was assembled yet.
The `CaptureToken` is no longer inert. It was read at exactly one place that
executes before the timer is even registered, so it was always false; it now
carries the deadline (a synchronous stretch cannot be preempted, so the flag
alone is set too late) and is consulted before the screenshot invoke, the
tree dump and the overflow spill — the post-teardown capture and orphan
artifacts its own comment claimed it prevented.
`createSecretScrubber` built its mask list from `ARGENT_SECRET_*` process
environment entries alone, but `{{secret:NAME}}` resolves through the whole
four-source chain `secretSources()` owns: the environment, the project's
`.argent/secrets.env`, the `ARGENT_SECRET_`-prefixed keys of `.env.local` /
`.env`, and `~/.argent/secrets.env`. The three file sources type onto the
device and were invisible to the scrubber, so a credential the app echoed
back into a non-password element reached `screen.elements[]`, the `tree`
artifact, the `--json` report and the NDJSON progress stream verbatim — and
`--output` copied it into the CI artifact.
The dedicated files are the placement `secretPlacementAdvice` recommends
first, so the default-advised setup was exactly the one the scrubber could
not see.
Build the mask list from `secretSources()` instead, with no options of its
own so the scrubber and the resolver cannot diverge on scope, and fail soft
to "no mask" if the chain cannot be read.
Rebasing onto main brought three features this branch had never met, and
each one needed the diagnostics to learn something:
`await: { idle }` reports four tree-source outcomes, none of which carried
an evidence code. Unclassified defaults to determinate, so the assembler
fell through to its post-hoc `fetchFlowTree` — against the very source that
had just wedged, which then hung for as long as it liked. Classifying them
`tree-source-unavailable` is both the honest label (they are environment
failures, not verdicts about the app) and what keeps the reader off a
source already known to be broken.
A launch failure gets the same treatment for a different reason: there is
no app screen to read, and on chromium the read attaches to the instance
the launch just declined to attach to.
A device-free flow has no device at all, so `DiagnosticsEnv.device` is
nullable and every device-backed section degrades — a new `no-device`
screen state — rather than fabricating a screen. Its composition failures
(a cyclic `run:`, an unloadable fragment) are fully explained without one.
Also drops `export` from nine symbols nothing outside their module reads,
which is what the knip gate on main is for.
"unavailable" is not one of the ten FAILURE_KINDS, so tsconfig.test.json
rejected it. vitest strips types, which is why the test passed locally
while the test-project typecheck failed - the fixture now mirrors the
production site it stands in for.
Adversarial device testing found four shapes whose report contradicted
its own payload or withheld what it had captured.
A scroll container that matched NOTHING was reported as "is not visible"
- the message claiming it exists while the matchCount beside it said it
never did. The code now comes from the observation, the same rule the
selector pair already follows.
A tool step that failed because the tree source was unreachable was
always determinate, even while its own screen block said the read had
failed. It now gets the classification a directive in the same situation
gets.
A snapshot failure skipped the second capture (correct - it would show a
different screen than the one diffed) but never reused the handle it
already had, leaving the one category whose repair starts with pixels
with no image at all.
And the CLI dropped selectorYaml, so the paste-able selector - the
headline output of ranking - reached the agent surface but never CI.
A tree-source failure rendered as "no visible element matched selector
… add a scroll-to step" with no trace of the read error, because the
environmental shapes suppressed the screen slot wholesale - and that slot
is exactly where the producer puts the cause. It now survives whenever it
carries a detail, so the block says what actually broke.
stepLabel was the one wire path with no clamp, and it now feeds the
failure heading and the JUnit name attribute: a target carrying newlines
and ANSI escapes rendered raw, positioned to repaint the verdict above
it. The artifact role key had the same gap while the path beside it was
already clamped. MCP's wireText collapsed whitespace but not control
characters, and its failure loop had no upper bound.
An out-of-range startedAt threw inside the reporter's own try/catch, so
one bad number cost CI the entire JUnit file rather than one attribute.
SKILL.md's resilience section still told the agent to repair via
describe - the workflow the new Diagnose step bans - and pointed at a
table row this change deleted.
Review pass found the diagnostics were honest about most of the payload
and quietly wrong about the rest.
Secrets: a candidate's selectorYaml was built from the node's RAW
strings, so the same row carried a masked label beside a suggestion
quoting the credential in full - straight into the model's context, which
is the one thing {{secret:}} exists to prevent. cause.message and
expected were never masked at all, and the selector projections were
scrubbed as SERIALIZED JSON, so a secret containing a quote or a
backslash was invisible to the mask.
Budget: the 24 KiB limit dropped only the element list and never
re-measured, so a screen-sized message left the payload eight times over
while overflow.omittedBytes reported the few hundred bytes that had
actually gone. It now bounds the message and sheds in order of
diagnostic value until it fits.
Tree source: a blind read SUCCEEDS, so it recorded no error, so a
post-hoc read that happened to work rendered a healthy screen next to
"every read of the UI tree was empty or degraded". Gate on the verdict,
not on whether a read threw.
Also: the scope diagnosis rewrote an indeterminate verdict to
determinate, turning "re-run" into "your flow is wrong" on exactly the
mid-run devtools drop the tier exists for; a timed-out capture kept
running and touched the device after teardown; and each failing run left
a temp directory behind forever on a host-wide server.
truncateUtf8Field reserved ONE byte for the ellipsis, but that character
is three bytes in UTF-8 - so every truncation overshot the limit by two,
and a limit below three produced a longer string than it was given.
selector.alternatives was the one projection of the selector that skipped
the scrubber, so a secret masked in described and fields beside it rode
out raw.
The indeterminate tiers narrated match counts from the last read that
merely succeeded while shipping the tree from the last TRUSTED read: a
blind read left the counts empty, so a hidden check that could not be
confirmed reported that nothing matched - the opposite of its diagnosis.
The degradation file is the load-bearing one: every case asserts the
verdict, the counters and the step statuses are unchanged, because the
entire capture path is only allowed to degrade its own payload.
These caught three real bugs, now fixed: truncateUtf8Field reserved one
byte for a three-byte ellipsis; selector.alternatives was the one
unscrubbed projection of the selector; and the indeterminate tiers
reported match counts from a different read than the tree they shipped.
The terminal block is a slot system, not a template: code + message is
mandatory and the rest (context window, expected/actual, candidates,
screen, reads, artifact paths) appears only when it applies, which is
what lets one renderer serve six very different failures. Step lines
gain a duration only when the server sent one, so a pre-timing
tool-server renders byte-identically.
--reporter junit:<path> maps each STEP to a testcase, because per-failure
attribution in the checks UI is the entire value of JUnit in CI. A
report-file write failure warns and leaves the verdict alone - failing a
build on argent's own I/O would make the reporter a source of flake.
Everything the renderers read is clamped first: the failure object can
arrive from a remote argent link server.
The old Diagnose steps told the agent to call screenshot/describe after
a failure. That is not merely redundant now - it is wrong: the run has
aborted and torn down, so it captures a different screen from the one
that failed. The symptom-guessing table is replaced by a failure.code to
strategy mapping, which is what lets an agent tell "fix the selector"
from "re-run, argent could not see the screen".
Every frame in the report is normalized, so the size they were
normalized against is what makes a candidate's tap centre meaningful to
a reader comparing it against the screenshot.
Two refinements found by running the diagnostics against a real device.
A normalized frame means nothing without the size it was normalized
against, so the available screen now carries it (the tree adapters
already knew it and threw it away).
Candidates answer "which element did you MEAN instead", so they are
noise when the selector DID resolve: a text mismatch or an unmet hidden
was ranking the very element the step already matched, which reads as
advice to make a change that would do nothing. Those shapes are
diagnosed by expected/actual instead.
A failing flow carried one prose string. This adds the structured
counterpart: a classified cause, the tree the step actually read, ranked
candidate elements, and handles to a screenshot and element dump captured
at the moment of failure.
Evidence plumbing is additive - every read loop keeps its control flow,
return values and throw behaviour, so the abort mapping, the three
evidence tiers and the one-failure-per-run invariant are untouched by
construction. Capture is fail-soft end to end and budgeted, so it can
degrade the payload but never a verdict.
> Stacked on #715.
## The problem
`await-ui-element` evaluates against the tree it reads — the
**accessibility hierarchy** on iOS and Android, the **CDP DOM** on
Chromium, the **toolkit page source** on Vega. The `await:` / `assert:`
directive that the polish pass converts a recorded wait into is
evaluated against the **runner's** tree instead.
They overlap, but outside Vega neither contains the other — an id
present in one can be absent from the other, and on iOS even the role
vocabularies are disjoint. Both directions have been hit in practice: an
id visible to the runner but not the recorder, and text-field values
visible to the recorder but not the runner.
The consequence is that a check can pass live and fail once converted.
That makes the core promise — *each step is executed live so you verify
it works before it's recorded* — untrue at exactly the point it is
relied on.
## What changes
After a recorded `await-ui-element` succeeds, the recorder re-runs the
same condition against the tree the runner resolves directives against,
and reports the result on the step it just wrote.
**It warns; it does not refuse.** What gets written is a raw `tool:
await-ui-element` step, and at replay that tool reads the same tree it
just passed against — so "this would fail every run" was never true of
the form actually recorded. What the probe genuinely reports is whether
the **polish-time conversion** is safe, which is why the warning says to
act on it *then* rather than re-record now (re-recording would duplicate
the step).
Because polish begins after `flow-finish-recording`, each verdict is
also carried there: it is repeated on its own step's `summary` line, and
`message` counts how many steps carry one. A warning raised at step 7 of
a 40-step recording is otherwise long out of reach by the time it is
actionable.
## Three outcomes, kept distinct
| Runner tree says | Reported as |
| --- | --- |
| condition holds | nothing — step recorded clean |
| condition does not hold | recorded + warning naming the causes to rule
out before converting |
| tree unreadable, or too slow to answer | recorded + warning that the
conversion is **UNKNOWN, not known-bad** |
The middle row is deliberately not "the conversion will fail". The
identical verdict comes back when the screen simply moved on between the
live wait and the re-probe, and on `text` checks when the selector
matches several elements and the two sides elect different ones — so the
consequence stays conditional on the cause, and the message names the
causes in the order worth ruling out.
The third row matters: nothing was compared, so calling it a divergence
would send the author off to rewrite a selector that may be perfectly
good. It is also the injection-free case the flow skills explicitly
sanction, so refusing there would block a supported way of working. On
iOS the quoted reason ends "provide bundleId explicitly" — advice aimed
at the native tools. The warning corrects it, because no directive
carries a bundleId and forwarding one would make the probe answer a
question the runner never asks.
## Platform-specific explanation
The *why* differs per platform, and stating the iOS story everywhere
makes the message wrong where someone is trying to act on it. iOS gets
AX-vs-native-hierarchy; Chromium gets "the flow tree keeps only
addressable nodes"; Android gets the importance trim; Vega gets "the
trees agree, so the screen changed". The named tool for reading the
runner's side follows the same rule — pointing an Android author at
`native-find-views` points at a tool they cannot call.
The remedy is per **condition** as well as per platform: on `hidden` a
determinate verdict means the runner's tree still *has* the element, so
"retarget at an id the full hierarchy carries" is the opposite of what
helps.
## A failed live wait is reported by cause
`await-ui-element` returns `{ success: false }` for three different
things: a condition read and found false, a tree source that never
answered, and a cancelled wait. Only the first judges the condition, so
only the first is narrated as "the wait never held" and prescribes
re-recording or deleting the step. The other two observed nothing and
are reported as UNKNOWN — the same unknown-vs-known-bad distinction the
probe draws for the runner's tree.
A wait nested inside a recorded `run-sequence` gets neither warning:
that tool reports its own shape, which carries no `success` key. The
skill says so, and says to read `toolResult` there.
---------
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
Hubert Gancarczyk