fix(flows): stop a recorded sub-flow pinning the device it was recorded on - #696
Merged
Conversation
…ed on `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 device id in the flow file, and on replay the runner never rebound it — the nested run drove the baked-in device rather than the one the replay was given. That contradicted three statements the code and docs already made: 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 — and 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. Adding `device` to the list fixes both halves at once: `stripDeviceKeys` keeps it out of the recording, and `bindDeviceArgs` replaces any id already committed to a flow file. Verified live — a flow whose nested step names a device that does not exist now runs against the device the replay was given, where before every device-dependent sub-step errored. `platform` is deliberately left alone, and there is a test pinning that. It is read only when no device was given, so once `device` is bound it is inert. And the strip is schema-blind while `platform` is not device-specific on every tool — react-profiler-analyze declares its own — so stripping it would silently retarget an unrelated recorded step. One behaviour change worth knowing: a nested flow-execute can no longer target a device other than its parent's. Nothing in the repo does this, and `run:` already worked that way. Fixes #607
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 #607.
The bug
DEVICE_BIND_KEYScoveredudidanddevice_id, butflow-execute's own device parameter is nameddevice. So a recordedtool: flow-executestep 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-executestep: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:
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 fromrun: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, sincerun:composition is host-resolved. Remote recordings are therefore the main producer of the affected artifact.The fix
Add
devicetoDEVICE_BIND_KEYS. That fixes both halves at once —stripDeviceKeyskeeps it out of new recordings, andbindDeviceArgsreplaces an id already committed to a flow file.Safe because
bindDeviceArgsonly injects keys the target tool declares, and across all 75 registered tools exactly one declaresdevice—flow-executeitself. (24 usedevice_id, the restudid; both were already bound.)platformis deliberately not strippedThere is a test pinning this so a later symmetry edit fails loudly. Two independent reasons:
resolveFlowDevicereturns onopts.devicebefore touching it, and the chromium boot spec is gated on!params.device. Oncedeviceis bound it is inert.platformis not device-specific on every tool:react-profiler-analyzedeclares its own. Stripping it would silently rewrite a recorded Android profile analysis into an iOS one.Verified live
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:testsand the tool-description gate clean.Behaviour changes worth knowing
devicestop 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.flow-executecan no longer target a device other than its parent's. Verified nothing in the repo does this (no tracked.argentflows, no flow references in the e2e scripts or workflows), andrun: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 differentplatform, which now silently runs on the parent's device.Related
tool: flow-executestep always reports PASS, whatever the composed flow did #606 (a nested flow's failure does not fail the outer step) is what let the transcript above reportPASS. Independent in code — flow replay: a rawtool: flow-executestep always reports PASS, whatever the composed flow did #606 is in the result inspection after dispatch, this is in the args before it — so they do not conflict. Worth knowing when reviewing flow replay: a rawtool: flow-executestep always reports PASS, whatever the composed flow did #606 that the "nested run errored" symptom there had a second, separate cause, fixed here.extractDeviceArginhttp.tskeeps its own paralleludid/device_idlist and also omitsdevice, so a top-levelflow-executecall 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_fileare 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.