mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
filip131311
2278115028
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.