Skip to content

feat(flow): conditional when blocks#498

Open
j-piasecki wants to merge 1 commit into
mainfrom
pr498
Open

feat(flow): conditional when blocks#498
j-piasecki wants to merge 1 commit into
mainfrom
pr498

Conversation

@j-piasecki

Copy link
Copy Markdown
Member

What

A when: block runs a step list only when one condition holds — a UI
condition (exists/visible/hidden/text, the await/assert shapes)
or a static platform test. No else, by design: a block exists to
restore determinism (dismiss an interstitial, reconverge), never to
test two paths. Tap-if-present is a one-step block:
when: { visible: "Got it" } + steps: [tap: "Got it"].

No per-step optional:when: covers every directive once. The key
is rejected at parse with a pointer to when:, not silently dropped (a
Maestro habit that would leave a "can't fail" step hard-stopping the
flow).

How

  • The guard reuses the assert engine at the 1s grace (timeout is a
    parse error), so a skipped block barely costs a clean run.
  • Verdicts are evidence-based: no trustworthy read in the window →
    indeterminate → the step errors ("could not evaluate" is not
    "condition false"); a transient read failure after trusted reads showed
    the condition false stays a clean skip.
  • platform folds ios-remote into ios — the parser rejects that
    spelling, so the fold is the only way a guard matches a remote sim.
  • A skipped block reports one line per authored step, same shape for
    unmet guard, hard stop, or cancellation; everything goes through
    pushReport, so when lines stream live. Failures inside an entered
    block hard-stop as usual.
  • Results carry an explicit aborted flag; ok keys off
    fail/error/cancel, not skip lines.
  • Nesting is capped at 20, so a cyclic YAML alias is a structured parse
    error, not a stack overflow.

Testing

vitest 2546 pass (17 when-specific); tsc --noEmit (tool-server,
argent-mcp, argent-cli), eslint, prettier clean.

Co-Authored-By: Claude Fable 5 [email protected]

## What

A `when:` block runs a step list only when one condition holds — a UI
condition (`exists`/`visible`/`hidden`/`text`, the await/assert shapes)
or a static `platform` test. **No else**, by design: a block exists to
restore determinism (dismiss an interstitial, reconverge), never to
test two paths. Tap-if-present is a one-step block:
`when: { visible: "Got it" }` + `steps: [tap: "Got it"]`.

No per-step `optional:` — `when:` covers every directive once. The key
is rejected at parse with a pointer to `when:`, not silently dropped (a
Maestro habit that would leave a "can't fail" step hard-stopping the
flow).

## How

- The guard reuses the assert engine at the 1s grace (`timeout` is a
parse error), so a skipped block barely costs a clean run.
- Verdicts are evidence-based: no trustworthy read in the window →
indeterminate → the step **errors** ("could not evaluate" is not
"condition false"); a transient read failure after trusted reads showed
the condition false stays a clean skip.
- `platform` folds `ios-remote` into `ios` — the parser rejects that
spelling, so the fold is the only way a guard matches a remote sim.
- A skipped block reports one line per authored step, same shape for
unmet guard, hard stop, or cancellation; everything goes through
`pushReport`, so when lines stream live. Failures inside an entered
block hard-stop as usual.
- Results carry an explicit `aborted` flag; `ok` keys off
fail/error/cancel, not skip lines.
- Nesting is capped at 20, so a cyclic YAML alias is a structured parse
error, not a stack overflow.

## Testing

`vitest` 2546 pass (17 when-specific); `tsc --noEmit` (tool-server,
argent-mcp, argent-cli), eslint, prettier clean.

Co-Authored-By: Claude Fable 5 <[email protected]>

@latekvo latekvo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of observations on the reporting/evaluation paths, left inline.

Comment on lines +840 to +850
if (!probe.ok && probe.indeterminate) {
pushReport(state, {
index,
kind: "when",
status: "error",
reason: `could not evaluate when guard (${label}): ${probe.reason}`,
flow: sourceFlow,
target,
});
state.stopped = true;
return;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the guard probe comes back indeterminate, this branch emits the when:error marker and hard-stops, but returns without expanding the block's own guarded steps. Every other path where the block does not run expands them one line per authored step: the unmet-guard branch just below and the aborted branch above both call reportBlockSkipped, and so does the outer-loop hard-stop in execSteps. Because of that, the same authored block produces a different number of report lines depending on why it didn't run: an unmet two-step block reports when:skip, <step>:skip, <step>:skip, whereas an errored guard reports only when:error with the two guarded steps missing entirely. That contradicts the invariant reportBlockSkipped documents ('a run where the block was skipped (unmet guard, hard stop, or cancellation) produces the same report shape (one line per authored step) ... reports stay comparable run-to-run'), and the same omission recurs when the errored guard is nested, dropping its inner subtree. The run verdict is unaffected (still ok:false); the divergence is in the report shape and the skipped/errored tallies.

// confirmed — also unknown, not false. (A trusted read WITHOUT a visible
// match would have satisfied `hidden` inside the loop; a final trusted
// read WITH one falls through to the determinate "still visible" below.)
if (step.condition === "hidden" && !lastMatches.some(isVisible)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hidden indeterminate branch is gated on !lastMatches.some(isVisible), but the fetch catch earlier in the loop only records fetchError and leaves lastMatches holding the matches from the last successful read. So when the element matched on an early trusted read and then every remaining read in the grace window throws (for example a native-devtools disconnect, which fetchFlowTree surfaces as a throw), lastMatches still contains the visible match, this branch never fires, and evaluation falls through to the determinate-false return: the guard reports when:skip with reason 'condition not met' and the run stays green. The morally identical case where the post-match reads instead return an empty tree does reach this branch and errors. Concretely, a when: { hidden: Spinner } block with Spinner visible on read 1 and every later read throwing yields when:skip / ok:true, while the same block whose later reads return an empty degraded tree yields when:error / ok:false. That is the reverse of the stated intent that an unreadable tree must error rather than silently turn a guarded dismissal into a green no-op, and the 'condition not met' reason reports an unevaluable read as a confirmed-false condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants