fix(flows): let a nested run's verdict reach the run that contains it - #697
Merged
Conversation
filip131311
force-pushed
the
filip/flow-nested-verdict
branch
from
August 3, 2026 07:47
5075147 to
9f1ad58
Compare
filip131311
changed the base branch from
main
to
filip/flow-deviceless-and-counts
August 3, 2026 07:47
filip131311
added a commit
that referenced
this pull request
Aug 3, 2026
…ounts 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.
A flow step that runs another orchestrator reported `pass` whatever the nested run actually did. The generic `tool` step treats any non-throwing result as a pass, and both `flow-execute` and `run-sequence` report failure in their result rather than by throwing. So the same flow reported `ok: false, failed: 1` run directly and `ok: true, passed: 1` run nested — with the failing sub-report sitting inside the very result object being called a pass. Two shapes were lost for `flow-execute`: a composed flow that ran and failed, and one that ran nothing at all because its execution prerequisite was never acknowledged. They map to the two statuses the runner already has — a flow that ran and failed its assertions is a failure, a step that was never runnable as written is an error, the class the runner already uses for an unreadable fragment or a cyclic reference. Both hard-stop, because in this runner every fail and every error hard-stops. A cancelled nested run is a skip, matching the rule the runner applies to its own steps. `run-sequence` had the same hole and no verdict field at all: every one of its failure paths pushes an error entry, breaks the loop and returns normally, so a sequence that stopped on its first of eight steps looked like an ordinary result. Fixed here rather than left as a known identical bug on the same line. These are two named, tool-scoped branches, not a general "a result with ok: false fails the step" rule. There is no such contract here to generalise — the only other soft-verdict tool spells it `success`, run-sequence spells it neither way, and this step dispatches tools whose results are typed `unknown`, several carrying app-derived payloads. A blanket rule would bind all of them, and everything added later, to a key name. There is a test pinning that. The whole sub-report still rides on the step's `result`, so nothing that was visible before is lost; the reason string carries the sub-flow's own first failure, because the CLI renders only the reason. Fixes #606
filip131311
force-pushed
the
filip/flow-nested-verdict
branch
from
August 3, 2026 14:16
9f1ad58 to
4dd9c19
Compare
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 #606.
The bug
A flow step that runs another orchestrator reported
passwhatever the nested run did.case "tool"treats any non-throwing result as a pass, and bothflow-executeandrun-sequencereport failure in their result rather than by throwing.Reproduced live — the same flow, run two ways:
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
executionPrerequisitewas 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:
ok: falserun:composition already producesnotice, zero stepsaborted: trueBoth 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-stepoptional:is rejected at parse time becausewhen:already expresses it.run-sequencehad the same holeFound while reviewing the plan.
run-sequencehas no verdict field at all — every failure path (disallowed tool, unsupported operation, unmetawait-ui-element, a tool that threw) pushes anerrorentry,breaks, 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 === falseruleThere is no
okcontract in this codebase to generalise. The only other soft-verdict tool spells itsuccess(await-ui-element),run-sequencespells it neither way, andcase "tool"dispatches tools whose results are typedunknownorRecord<string, unknown>— several carrying app-derived payloads. A blanket rule would silently bind all of those, and every tool added later, to "a key calledokdecides my flow's verdict".isUnmetUiWaitResultset 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
okin its result —flow-execute. In particularsettings-permissionsdoes not; its{ok: false}is a private per-pm-invocation type that either throws (already an error) or returnsapplied/skipped(a legitimate pass).Verified live
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:testsand the tool-description gate clean.Reporting shape
One failing step carrying a summarising
reasonplus the whole sub-report inresult— the pass path already attachedresult, so this is the same shape with a non-pass status. The nested steps are deliberately not spliced into the outersteps[]:run:can expand inline only because it shares oneExecState(one index sequence, one depth base, one device, one baseline dir), whereas a rawtool: flow-executeis 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 rendersresultfor any step that has one, and the CLI (which renders onlyreason) gets the sub-flow's own first failure inside the reason string.No new
StepReportfields, no wire-format change; older clients render the new line unchanged.Merge ordering and conflicts
flow-executestep pins the record-time device —deviceis not a DEVICE_BIND_KEY #607) first. Before it, a nestedflow-executecould run against a stale baked-in device and legitimately reportok: false; with this landed that becomes a parent failure and would read like a regression caused by this PR. No code-level conflict — fix(flows): stop a recorded sub-flow pinning the device it was recorded on #696 touchesflow-device.tsand three other test files.case "tool", adding anevidencecode to each return) and addsStepReport.failure/durationMs. Whichever lands second should give the new branches an evidence code — likelynested-flow-failed/nested-flow-prerequisite-unacknowledged/nested-flow-aborted— so its CI diagnostics classify the composition case. fix(flow): let a flow launch an app that can never be instrumented #677 also editsexecLeafStepand addsStepReport.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-executenesting has no cycle or depth guard:MAX_RUN_DEPTHand the run-stack cycle check cover onlyrun:, so a flow whose raw step names itself recurses through fresh runner invocations. Independent of this issue; filing separately.