mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
f05d17c1d3
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>
255 lines
8.3 KiB
TypeScript
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);
|
|
});
|
|
});
|