Files
software-mansion__argent/packages/tool-server/test/flows/flow-progress.test.ts
Ignacy Łątka f05d17c1d3 refactor(flow): let each step kind own how it reads (#744)
Stacked on #574 (merged; retargeted at `main`).

## Summary

`summarizeSteps` (flow-finish-recording.ts) was a 50-line `switch` over
`FlowStep["kind"]`. `stepTarget` (flow-run.ts) was a second one, over
the same union, with its own selector and condition spellings. Neither
could be found from the other, and `summarizeSteps`' `case "tool":
default:` arm meant a step kind added without wording rendered as a
`tool:` step instead of failing.

Both surfaces now read from one definition per kind — the same shape as
a tool owning its own log wording in `ToolDefinition.interaction`
(#582):

```ts
const FLOW_STEP_DEFINITIONS: {
  [K in FlowStep["kind"]]: FlowStepDefinition<Extract<FlowStep, { kind: K }>>;
} = {
  "scroll-to": {
    summary: (step) => `${yamlSelectorLabel(step.target)} (${step.direction})`,
    target: (step) => { ... },
  },
  ...
};
```

Keying the record by `FlowStep["kind"]` is what replaces the default
arm: a new kind is a compile error until it says how it reads on
**both** surfaces. That also retires the `renderToolArgs` interpolation
guard, which existed only because the default arm could hand it a step
with no `args` field.

`flow-run.ts` sheds a net 113 lines and five imports;
`flow-finish-recording.ts` sheds a net 180 and is now just the tool. The
two shared label templates (`conditionLabel` parameterized by selector
spelling, and a new `whenLabel` for the platform-vs-UI guard arm) mean
the recorder's `when:` tail and the runner's no longer duplicate the
platform arm.

## Test

The extraction is rendering-neutral: every summary line and report
target the switches produced moved over byte-for-byte (with four later
corrections, below).

- Proved with a throwaway equivalence test that inlined both
pre-refactor switches verbatim and compared them against the new
definitions over 1264 generated steps: all 15 kinds, 10 selector shapes
(loose, regex, scoped, quote-bearing), every wait condition × text-match
mode × expectation, every scroll direction, and the cyclic-`args` path.
Identical on all of them. Mutating two renderings (`equals` comparator,
default scroll direction) failed it, so it discriminates.
- Replaced by
`packages/tool-server/test/flows/flow-step-definitions.test.ts`, kept:
assertions pinning the exact summary line and report target of every
kind, in a table keyed by `FlowStep["kind"]` so a new kind fails
`typecheck:tests` here too (verified by deleting the `wait` entry:
`TS1360`). Deleting a kind from the definitions record fails the build
the same way (`TS2741`).
- `npm run build`, `eslint --max-warnings 0`, `prettier --check .`, `npm
run typecheck:tests --workspaces`: clean.
- `packages/tool-server`: 329 files / 3906 tests passed.
`packages/argent-cli`: 459 tests passed.

## Review follow-up (9cf51322)

Four corners where the carried-over rendering misdescribed what replays,
fixed with pinning tests:

- `delayLabel`: a hand-edited `delayMs` above `setTimeout`'s 2³¹−1
ceiling sleeps a clamped 1ms tick but read `(after 3000000000ms)` — the
summary now describes no delay there.
- `scroll-to`: the `within` container is parsed, serialized, and changes
what replays, yet neither surface rendered it; both do now.
- `snapshot`: the summary now carries `cropOn` and `maxMismatch`
(baseline key and tolerance). Only `cropOn` was on the report target;
`maxMismatch` was rendered on neither surface (the run report surfaces
the tolerance through the step's `reason`, not its target).
- `type`: the summary JSON-quotes the text, the module's own convention
for literals, so embedded quotes/newlines stay one unambiguous line.


## Review follow-up (7c804907)

A second pass (7 lenses + a 32-mutation pass that also re-derived the
neutrality claim against main over a 136-step corpus, and confirmed the
mapped-type compile gate by adding a kind) found no HIGH or MEDIUM. One
rendering defect this PR introduced, plus the boundaries the case table
left open:

- `scroll-to`'s report target spelled the scroll container ` within (…)`
— exactly how `selectorLabel` spells a selector's own relational scope.
`scroll-to: { target: Footer, within: { id: list } }` (scroll that
container) and `scroll-to: { target: { text: Footer, within: { id: list
} } }` (match only inside it) both rendered `"Footer" within (id=list)`;
carrying both put the direction between two identical-looking scopes. It
now reads ` in scroll container (…)`, the noun flow-actions already uses
in its own "scroll container … is not visible" failure. `main` did not
collide here only because it dropped `within` entirely.
- New pinning cases, each verified to fail without the code it pins:
`setTimeout`'s ceiling from both sides (`2**31-1` keeps its label,
`2**31` loses it) and its 1ms floor; `maxMismatch: 0`, the one value
separating `!== undefined` from a truthiness test; `cropOn` +
`maxMismatch` together, which fixes their suffix order; and a
multi-field selector, which fixes the report target's field order.
- `Number.isFinite(ms) &&` in `delayLabel` became redundant the moment
correction #1 added the ceiling — NaN and ±Infinity both fail the range
test. Removed (proved a no-op by mutation).
- Prose corrected where it had stopped matching the code: the
point-gesture comment argued a criterion the file does not follow
(`snapshot` and `scroll-to` are equally not recorder-built and this PR
renders their fields anyway) and listed two of the four still-unrendered
replay-affecting fields; `flow-tools.test.ts` still claimed to be the
only coverage of the long-press rendering; and the new case table
justified a no-op `as number` cast with a reachability claim that is
wrong in both halves (`flow_add_step`'s `delayMs` is
`z.number().int().min(0)` with no ceiling, so the recorder reaches these
values too).

Follow-ups filed rather than folded in, both pre-existing and unchanged
here: #808 (an `echo` message or `tool` name containing a newline breaks
the one-line-per-step summary contract, the same hazard correction #4
fixed for `type`) and #809 (`type.submit`, `await.timeout`,
`idle.timeout`, `idle.stableFor` change what replays and render on
neither surface).


## Review follow-up (4560f412)

Round from @hubgan's review (11 inline) plus CodeRabbit's one. Nine
acted on, two verified and rejected — the rejection reason is now in the
code, so it stays answered rather than being re-raised.

**Rendering**

- `type`'s report target carried the typed text with **no bound**.
Unlike the summary, that target is emitted on every replay — stdout,
`--json`, the MCP blocks handed to the agent, and the appended
`~/.argent/mcp-calls.log`, which has no cap or rotation — so a literal
written into a flow file shipped its whole length to each. Now capped at
200 chars with the same `…(+N chars)` elision `parseFlow` uses on the
entry it echoes (`MAX_ENTRY_RENDER_CHARS`). The **summary keeps the text
whole**: it is also `stepAnchor`, so eliding there would collide two
steps differing only past the cap and drop the recording's verdicts on
both. Elides before quoting, so a cut cannot halve an escape.
- `selectorLabel` and `describeSelector` interpolated `textMatches`,
`identifier` and `role` raw — a newline in one splits a step across two
report lines, an escape byte drives the terminal. That is the hazard
`text`'s quoting already names, so it now covers its neighbours: new
`escapeInline` (flow-utils) spells out control characters **only**. Not
JSON escaping: doubling a backslash would print a regex source nobody
could copy back into the .yaml. `describeSelector`'s literal arm moved
from a raw `"${v}"` to `JSON.stringify` — byte-identical for ordinary
values, unambiguous for a quote in the content.

**Types**

- `FlowStepDefinition`'s two renderers were method shorthand, and a
method's parameters are checked **bivariantly even under `strict`**: a
definition annotated narrower than its slot (one requiring `times` on
`tap`) was accepted and then read a field the step need not carry, with
a green typecheck. Property types now — the same assignment is `TS2322`.

**Tests** — each verified against the mutation it exists to catch

- Every case in the table goes through `serializeFlow` → `parseFlow` and
is compared on its anchor. That round-trip stability is what
`scroll-to.within`, `snapshot.cropOn` and `snapshot.maxMismatch` newly
depend on, and nothing asserted it; deleting `yamlSelectorLabel`'s key
sort now reddens the suite. The `when` case with `steps: []` became a
two-step block — `parseFlow` refuses an empty guarded block, so no count
below 1 is renderable, and `when #1` already pins the singular.
- The case table's value type is a non-empty tuple of cases **narrowed
to the key's own kind**. `"wait": []` and a `wait` slot holding an
`echo` case both typechecked clean before; both are `TS2322` now.
- The `when:` pluralization coupling only ran a two-step block, so
hardcoding either surface's tail to `(N steps)` left everything green.
Both counts run now, and against the whole skip reason rather than its
suffix.
- New pins for the escaping above, on both surfaces, each confirmed to
fail without the line it pins.

**Prose corrected against the code**

- `stepAnchor`: there are **two** anchor comparisons, not three, and
only the per-verdict one is unconditionally raw-vs-parsed — host mode
re-parses both sides of the whole-file check, which is raw-vs-parsed
only in client persist mode.
- `delayMs` bounds: `.int()` admits nothing strictly between 0 and 1, so
the 1 ms floor is hand-edit-only; the ceiling is recorder-reachable, as
stated.
- `0` is not a parse-only `delayMs`: `min(0)` accepts it and
`serializeFlow` writes it back out.

**Rejected, verified**

- Escaping `snapshot.name` (CodeRabbit) would be dead code: `parseFlow`
gates it on `FLOW_NAME_PATTERN` — letters, digits, `_`, `-` — because it
becomes a baseline filename, and that parse is the **only** construction
site of a snapshot step. A comment there now says so.
- "`textMatches`' `/…/` delimiters are unpinned" no longer holds at this
head: `flow-progress.test.ts`'s swipe-`from` target asserts `to (0.1,
0.5) from /^Card \d+$/`, and dropping the delimiters fails it. Measured
by mutation.

**Verification**: 371 files / 4950 tests in `packages/tool-server`;
`argent-cli` 527, `argent-mcp` 87, `registry` 120. `npm run build`,
`typecheck:tests --workspaces`, `eslint --max-warnings 0`, `prettier
--check .`, `npm run knip`: clean. 13 mutations aimed at the new code,
all killed.

**Docs**: no update needed. `packages/docs/` documents flow YAML, the
CLI and the tools; it does not specify how a step summary or a
`StepReport.target` is spelled, so nothing there goes stale. That covers
the two earlier rendering changes from `3e2244b3` as well —
report-target text literals becoming JSON-quoted, and the `type` target
carrying the typed text (now bounded, above).



## Review follow-up (9f8a1e29)

The two threads left open after approval.

- `typedTextLabel` compared and cut on `text.length`, which counts
UTF-16 units. 199 ASCII chars plus one astral char is 200 characters but
201 units, so the target quoted a lone high surrogate and announced `(+1
chars)` for text that fits under the cap whole. It counts and slices
code points; the boundary is pinned on both sides of the cap and each
assertion is red without the fix. Two sibling truncations are
deliberately left, both pre-existing and outside the step-summary code:
`badEntry` slices an already JSON-stringified entry, and
`cappedReason`'s cap bounds the EMITTED length, where counting code
points would let the emitted string overshoot its own bound.
- The `textMatches` delimiter thread, re-measured at this head: dropping
the `/…/` fails `flow-progress.test.ts:231` and
`flow-step-definitions.test.ts:451` - 2 failed | 1571 passed of 1573.
Restoring them returns the suite to green.

**Verification**: `packages/tool-server` 371 files / 4950 tests; `eslint
--max-warnings 0`, `prettier --check .`, `tsc --noEmit` on both
`tsconfig.json` and `tsconfig.test.json`: clean.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Improvements**
* Flow step summaries and run reports now use consistent formatting
across supported step types.
* Type steps display their target identifier and entered text more
clearly.
* Selector, gesture, condition, delay, and snapshot details are rendered
consistently, including safer handling of special characters.
  * Tool steps with cyclic arguments are handled safely in summaries.

* **Tests**
* Added comprehensive coverage for step rendering, flow numbering,
conditions, edge cases, and report output.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Hubert Gancarczyk <hubert.gancarczyk@swmansion.com>
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
2026-09-04 12:54:13 +02:00

255 lines
8.3 KiB
TypeScript

import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
import type { Registry, ToolContext } from "@argent/registry";
import {
createRunFlowTool,
type FlowRunResult,
type StepReport,
} from "../../src/tools/flows/flow-run";
import { serializeFlow } from "../../src/tools/flows/flow-utils";
const DEVICE = "00000000-0000-0000-0000-0000000000ab";
let tmpDir: string;
function mockRegistry(): Registry {
return {
invokeTool: vi.fn(async (id: string) => {
if (id === "list-devices") return { devices: [] };
if (id === "boom") throw new Error("kaput");
return { ok: true };
}),
getTool: vi.fn(() => undefined),
// iOS flows gate on a native-devtools connection: report connected so the
// run proceeds, but expose no target app so the tree fetch falls back to
// the AX tree (these tests don't drive the native hierarchy).
resolveService: vi.fn(async () => ({
isConnected: () => true,
listConnectedBundleIds: () => [],
})),
} as unknown as Registry;
}
async function writeFlow(name: string, yaml: Parameters<typeof serializeFlow>[0]): Promise<void> {
const dir = path.join(tmpDir, ".argent", "flows");
await fs.mkdir(dir, { recursive: true });
await fs.writeFile(path.join(dir, `${name}.yaml`), serializeFlow(yaml), "utf8");
}
function asRun(r: FlowRunResult | { notice: string }): FlowRunResult {
if (!("steps" in r)) throw new Error(`expected a run result, got notice: ${r.notice}`);
return r;
}
/** ToolContext carrying only the progress hook — the flows under test touch nothing else. */
function progressCtx(events: StepReport[]): ToolContext {
return { emitProgress: (e: unknown) => events.push(e as StepReport) } as unknown as ToolContext;
}
beforeEach(async () => {
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "flow-progress-"));
});
afterEach(async () => {
await fs.rm(tmpDir, { recursive: true, force: true });
});
describe("flow progress streaming (ctx.emitProgress)", () => {
it("emits every report in execution order, identical to the final result", async () => {
await writeFlow("main", {
executionPrerequisite: "",
steps: [
{ kind: "echo", message: "starting" },
{ kind: "tool", name: "tap", args: { x: 0.5 } },
{ kind: "tool", name: "swipe", args: {} },
],
});
const events: StepReport[] = [];
const runFlow = createRunFlowTool(mockRegistry());
const result = asRun(
await runFlow.execute(
{},
{ name: "main", project_root: tmpDir, device: DEVICE },
progressCtx(events)
)
);
// Every appended report streamed, in order, as the same objects.
expect(events).toEqual(result.steps);
expect(events.map((e) => `${e.kind}:${e.status}`)).toEqual([
"echo:pass",
"tool:pass",
"tool:pass",
]);
// Echo is narration — the counters cover only the two real steps.
expect(result.passed).toBe(2);
expect(result.ok).toBe(true);
});
it("streams nested fragment steps (run:) with their source-flow attribution", async () => {
await writeFlow("login", {
executionPrerequisite: "On login screen",
steps: [{ kind: "tool", name: "tap", args: { x: 0.5 } }],
});
await writeFlow("main", {
executionPrerequisite: "",
steps: [
{ kind: "run", flow: "login.yaml" },
{ kind: "echo", message: "done" },
],
});
const events: StepReport[] = [];
const runFlow = createRunFlowTool(mockRegistry());
const result = asRun(
await runFlow.execute(
{},
{ name: "main", project_root: tmpDir, device: DEVICE },
progressCtx(events)
)
);
expect(events).toEqual(result.steps);
// The run marker and the fragment's expanded step both stream, attributed
// to the fragment.
expect(events.map((e) => `${e.kind}:${e.flow}`)).toEqual([
"run:login",
"tool:login",
"echo:main",
]);
});
it("streams the failing step and the skip-flood that follows it", async () => {
await writeFlow("main", {
executionPrerequisite: "",
steps: [
{ kind: "tool", name: "tap", args: {} },
{ kind: "tool", name: "boom", args: {} },
{ kind: "tool", name: "swipe", args: {} },
{ kind: "echo", message: "never reached" },
],
});
const events: StepReport[] = [];
const runFlow = createRunFlowTool(mockRegistry());
const result = asRun(
await runFlow.execute(
{},
{ name: "main", project_root: tmpDir, device: DEVICE },
progressCtx(events)
)
);
expect(events).toEqual(result.steps);
expect(events.map((e) => e.status)).toEqual(["pass", "error", "skip", "skip"]);
expect(result.ok).toBe(false);
// The skipped echo stays out of the counters like a passed one would.
expect(result.passed).toBe(1);
expect(result.errored).toBe(1);
expect(result.skipped).toBe(1);
});
it("labels directive steps with their target selector", async () => {
// A failing first step hard-stops the run, so every directive below is
// reported as a skip — which must still carry its target label.
await writeFlow("main", {
executionPrerequisite: "",
steps: [
{ kind: "tool", name: "boom", args: {} },
{ kind: "tap", selector: { text: "Clear logs", loose: true } },
{ kind: "tap", x: 0.5, y: 0.25 },
{ kind: "long-press", selector: { text: "Row 3", loose: true }, duration: 1200 },
{
kind: "assert",
condition: "hidden",
selector: { text: "] outer Touchable", loose: true },
},
{
kind: "await",
condition: "text",
selector: { identifier: "total" },
expectedText: "Total",
textMatch: "contains",
},
{ kind: "scroll-to", target: { text: "Nested touchables", loose: true }, direction: "up" },
{ kind: "type", into: { identifier: "email" }, text: "a@b.c" },
{ kind: "snapshot", name: "home" },
],
});
const runFlow = createRunFlowTool(mockRegistry());
const result = asRun(
await runFlow.execute({}, { name: "main", project_root: tmpDir, device: DEVICE })
);
expect(result.steps.map((s) => s.target)).toEqual([
undefined,
'"Clear logs"',
"(0.5, 0.25)",
'"Row 3"',
'hidden "] outer Touchable"',
'id=total contains "Total"',
'"Nested touchables" (up)',
'into id=email ← "a@b.c"',
'"home"',
]);
});
it("labels swipe reports with their travel and optional starting target", async () => {
// Stop before executing gestures: skipped directives still need complete
// labels so the CLI can identify every swipe in a failed flow.
await writeFlow("swipe-labels", {
executionPrerequisite: "",
steps: [
{ kind: "tool", name: "boom", args: {} },
{ kind: "swipe", direction: "left" },
{
kind: "swipe",
from: { selector: { text: "Card", loose: true } },
direction: "right",
},
{ kind: "swipe", by: { x: 0.25, y: -0.4 } },
{
kind: "swipe",
from: { x: 0.5, y: 0.8 },
to: { selector: { identifier: "archive" } },
},
{
kind: "swipe",
from: { selector: { textMatches: "^Card \\d+$" } },
to: { x: 0.1, y: 0.5 },
},
],
});
const runFlow = createRunFlowTool(mockRegistry());
const result = asRun(
await runFlow.execute({}, { name: "swipe-labels", project_root: tmpDir, device: DEVICE })
);
expect(result.steps.map((s) => s.target)).toEqual([
undefined,
"left",
'right from "Card"',
"by x=0.25, y=-0.4",
"to id=archive from (0.5, 0.8)",
"to (0.1, 0.5) from /^Card \\d+$/",
]);
});
it("runs identically when no progress consumer is attached", async () => {
await writeFlow("main", {
executionPrerequisite: "",
steps: [{ kind: "tool", name: "tap", args: {} }],
});
const runFlow = createRunFlowTool(mockRegistry());
const result = asRun(
await runFlow.execute({}, { name: "main", project_root: tmpDir, device: DEVICE })
);
expect(result.ok).toBe(true);
expect(result.steps).toHaveLength(1);
});
});