fix(flow): run a device-less flow without a device, and say why its counts are zero - #649
Merged
Merged
Conversation
A flow of nothing but `echo` steps still went through device resolution, so it failed outright when nothing was booted — and picked whichever device happened to be running when something was, attributing a run to hardware it never touched. Whether a device is needed is now decided from the flow's own steps, before resolution. The classification is per step kind and defaults to needing one, so a kind added later inherits today's behaviour rather than quietly running against no device; the compiler rejects leaving a new kind unclassified. A `when` block needs a device whatever its body contains, since the guard reads one itself, and a `run` step counts as needing one without the fragment being read here — resolving it twice could disagree with the run-time resolution. `ExecState.device` becomes nullable so every site that acts on a device has to say so, and the executor re-checks each step against the same predicate: if the two decisions ever disagree the step reports it, rather than failing obscurely deeper in. A run with no device reports an empty `device` rather than borrowing one. The summary for such a run said `PASS — 0 passed, 0 failed, 0 errored, 0 skipped`, which reads as though nothing happened. Narration deliberately isn't counted — and should not be, since after a hard stop every remaining step including narration is reported skipped, so counting it would inflate that number on real failing runs. The count is right; what was missing was saying why it is zero. Both the CLI and the MCP renderer now add `(no test steps)`, and only on a pass, where the counts are what needs explaining. Fixes #644
latekvo
added a commit
that referenced
this pull request
Aug 3, 2026
Conflict in packages/tool-server/src/tools/flows/flow-run.ts: this branch rewords the frontChromiumPage comment to name all three guarded gesture tools, while #649 made the run device nullable and changed the guard to `device?.platform`. The two are orthogonal — resolved by keeping the reworded comment and main's optional chaining.
filip131311
added a commit
that referenced
this pull request
Aug 4, 2026
…#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #644.
Before
and when a device was booted:
After
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
neverbinding makesleaving 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:
whenalways 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 stillresolves.
runneeds 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 —deviceis included because a nestedflow-executestep drives a device without being handed therun's own. A tool that declares no input at all (
stop-all-simulator-servers) is device-free, andreading its absent schema must not throw — it previously would have.
ExecState.deviceis nullable, which is the safety mechanism rather than just typing: the compilerenumerates every site that acts on a device, each of which now says so via one
deviceEnv()narrowing.
flow-actions.ts,flow-visual.tsandActionEnvare untouched. The executor alsore-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:
--jsondoesn't only disagree with thetext, 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
skippedon every real failing run — a worse and far more common lie than the vacuousone 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.independent — resolution is keep-both), and its
attachFailureDiagnosticsdereferencesenv.device.platform. It will need a null-device guard, or every failure in a device-less flowsilently degrades to
read-faileddiagnostics. Happy to rebase onto it rather than the reverse.resolveRunDeviceat the same seam and edits the same description sentence.flow-device.ts. Worth noting it makesresolveFlowDevicereject a physicaliPhone 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
PASS on <id>even when a device is booted. That is thepoint: otherwise an identical flow reports differently in CI (no simulator) than on a laptop with
one open. An explicit
--deviceis still honoured and still named.--platformis ignored for a device-free flow, where it previously errored with nothing booted.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);
--jsonshowingdevice: ""; explicit--devicestill 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