diff --git a/packages/argent-cli/src/flow.ts b/packages/argent-cli/src/flow.ts index 92e135b78..ee427653c 100644 --- a/packages/argent-cli/src/flow.ts +++ b/packages/argent-cli/src/flow.ts @@ -25,9 +25,13 @@ export interface StepReport { status: "pass" | "fail" | "skip" | "error"; reason?: string; /** - * Legacy: older tool-servers passed a snapshot that adopted a missing - * baseline and annotated it with this caveat (a missing baseline now fails - * the step). Rendered for wire compat with a not-yet-updated server. + * A step that passed in a way that weakens it as proof — raised today by + * `await: { idle: true }`, which never fails a run and says here what its + * green actually bought (see StepReport.warning in the tool-server's + * flow-run). Also carries the caveat older tool-servers put on a snapshot + * that adopted a missing baseline, which now fails the step instead. Live + * either way: dropping the field would silently delete the only thing the + * readiness check reports. */ warning?: string; tool?: string; @@ -340,6 +344,11 @@ export function renderArtifactLines(report: FlowReport): string[] { * Batch mode prints only what needs attention: each fail/error step with its * under-lines, numbered by walking the full step list so the numbers match a * single-mode rerun of the same flow. + * + * A PASSING step carrying a warning needs attention too. `await: { idle: true }` + * only ever warns on a step that passed, and renderSummary counts every warning + * whatever its status — so skipping those here printed "1 warning" with the + * text nowhere on screen, which is the whole of what the step reports. */ export function renderFailedSteps(report: FlowReport): string[] { const lines: string[] = []; @@ -347,7 +356,7 @@ export function renderFailedSteps(report: FlowReport): string[] { for (const s of report.steps) { if (s.kind === "echo") continue; n++; - if (s.status !== "fail" && s.status !== "error") continue; + if (s.status !== "fail" && s.status !== "error" && !s.warning) continue; lines.push(renderStepLine(s, n, report.flow)); if (s.warning) lines.push(renderUnderStepLine(s, n, `⚠ ${s.warning}`)); if (s.artifacts && typeof s.artifacts === "object") { diff --git a/packages/argent-cli/test/flow-render.test.ts b/packages/argent-cli/test/flow-render.test.ts index 0f59a3d28..33662f101 100644 --- a/packages/argent-cli/test/flow-render.test.ts +++ b/packages/argent-cli/test/flow-render.test.ts @@ -296,6 +296,21 @@ describe("flow report rendering", () => { expect(renderFailedSteps(mkReport([{ index: 0, kind: "tap", status: "pass" }]))).toEqual([]); }); + it("renderFailedSteps prints a passing step's warning, which renderSummary counts", () => { + // `await: { idle: true }` only ever warns on a step that PASSED, and the + // summary counts warnings whatever the status — so a directory run used to + // report "1 warning" with the text nowhere on screen. + const report = mkReport([ + { index: 0, kind: "tap", status: "pass" }, + { index: 1, kind: "idle", status: "pass", warning: "the screen never held still" }, + ]); + expect(renderFailedSteps(report)).toEqual([ + " ⚠ 2 idle", + " ⚠ the screen never held still", + ]); + expect(renderSummary(report)).toContain("1 warning"); + }); + it("renderBatchSummary mirrors the step summary's verdict shape", () => { expect(renderBatchSummary({ total: 3, passed: 2, failed: 1, skipped: 0 })).toBe( "FAIL — 3 flows: 2 passed, 1 failed, 0 skipped" diff --git a/packages/argent-mcp/src/content.ts b/packages/argent-mcp/src/content.ts index 799f9efc6..59c6431a0 100644 --- a/packages/argent-mcp/src/content.ts +++ b/packages/argent-mcp/src/content.ts @@ -211,9 +211,13 @@ export type FlowStepResult = { status?: "pass" | "fail" | "skip" | "error"; reason?: string; /** - * Legacy: older tool-servers passed a snapshot that adopted a missing - * baseline and annotated it with this caveat (a missing baseline now fails - * the step). Rendered for wire compat with a not-yet-updated server. + * A step that passed in a way that weakens it as proof — raised today by + * `await: { idle: true }`, which never fails a run and says here what its + * green actually bought (see StepReport.warning in the tool-server's + * flow-run). Also carries the caveat older tool-servers put on a snapshot + * that adopted a missing baseline, which now fails the step instead. Live + * either way: dropping the field would silently delete the only thing the + * readiness check reports. */ warning?: string; tool?: string; diff --git a/packages/skills/skills/argent-create-flow/SKILL.md b/packages/skills/skills/argent-create-flow/SKILL.md index 00876ee6c..d6f65fddc 100644 --- a/packages/skills/skills/argent-create-flow/SKILL.md +++ b/packages/skills/skills/argent-create-flow/SKILL.md @@ -31,7 +31,7 @@ Beyond raw `tool:` steps and `echo:`, flows support declarative directives inter | `scroll-to` | `- scroll-to: "Order #1234"` (scrolls down) or `- scroll-to: { target: …, direction: right, within: … }` | momentum-free scroll until the target is visible | | `pinch` | `- pinch: { on: "Map", scale: 3 }` or `- pinch: { scale: 0.5 }` | two-finger zoom in (`scale` > 1) or out (`< 1`); big scales chain gestures; `on` optional — defaults to screen center; open-loop — assert the visible result | | `rotate` | `- rotate: { on: "Map", by: 90 }` or `- rotate: { by: -45 }` | two-finger rotation by degrees (+ CW, − CCW, within ±3000°; options map only); `on` optional — screen center default; not `tool: rotate` (orientation) | -| `await` | `- await: { visible: Home }` | wait for a UI condition | +| `await` | `- await: { visible: Home }` or `- await: { idle: true }` | wait for a UI condition, or for the screen to stop moving | | `wait` | `- wait: 500` | pause for a fixed number of milliseconds (last resort — prefer `await`) | | `assert` | `- assert: { visible: Welcome }` | check a condition, hard-fail if it never holds | | `snapshot` | `- snapshot: home` or `- snapshot: { name: home, maxMismatch: 0.5, cropOn: { id: order-summary } }` | diff a screenshot — or one element's region — against a stored baseline | @@ -85,6 +85,20 @@ This condition-as-key form is the only spelling. `await` also accepts an optiona For a custom poll interval or bundleId, drop to an explicit `- tool: await-ui-element` step — but the raw tool polls the trimmed `describe` tree, so a testID it reports as not found can still resolve fine as an `await:` directive (see Selectors). Prefer the directive. +**`await: { idle: true }` — the one condition with no selector.** It waits until the screen has content and stops moving in **both** the UI tree and the rendered pixels. Options: `stableFor` (how long stillness must hold, default 250) and `timeout` (default 7500, and it has to leave room for the hold plus the 600ms a settle costs — three reads spanning two 200ms polls, plus the 200ms of budget the closing round has to have left to be allowed to start — or the parser rejects the step). Reach for it when a transition's motion is invisible to the tree — an iOS push or modal dismissal commits its hierarchy up front and then animates a layer for a few hundred milliseconds, and a cross-fade or scrim moves no node at all — which is exactly when an element `await:` returns while the screen is still sliding, and the next tap lands on a moving target. + +It **never fails a run.** Readiness is not an acceptance criterion, so every outcome short of a clean settle passes carrying a `warning` on the step — read it rather than stepping over it: + +- **the screen never held still** — it spent the timeout and went ahead. Plenty of healthy screens never stop (a video, a shimmer, a carousel, live-updating text); a screen that never finished loading looks the same from here. +- **a small part of it was still changing** — a spinner, a caret, a progress dot, moving during the stretch of stillness the step settled on. Too small to be the screen moving, so the settle completed anyway; if it is a loading spinner, the screen was still loading when this step returned. +- **the tree stayed empty** — the screen rendered no accessible content. Sometimes the app (a canvas, a video surface), sometimes a screen that never arrived. +- **settled on the UI tree alone** — the screen could not be screenshotted often enough to compare a pair, so presentation-layer motion (a push, a fade, a dismissing modal) was not waited out. +- **the screen came back with content on too few reads** — a settle takes three of them spanning two polls, and this step got fewer, so it ended without ever being able to tell whether the screen was moving. A slow tree source, or a window that was blank for most of the wait. + +Only a tree source that cannot be read stops the run, as an `error` — one that is still failing when the wait ends, one that answers and then wedges, or one that never answers at all within the step (that last one may simply be slow: raise the step's `timeout` before suspecting the app). That is a broken window, not a verdict about the app: the run is not ok and every later step is skipped. A single failed read is not that window: the hold restarts from the next good one, and a read that fails at the very end of the wait is named in the warning rather than stopping the run. + +It is **not** a screen check either: a dropped tap leaves the source screen perfectly idle. Put it **after** the element `await:` that names the destination, never instead of one. There is no `assert` form (waiting is the whole point), no `when:` form, and the recorder cannot emit one — every `idle` step is hand-written. Do not sprinkle it after every step: each one costs a settle, and it cannot fail, so a flow full of them is slower without being stricter. + ### `type` and `scroll-to` `type` presses Enter after typing to commit the value and dismiss the keyboard, so it can't cover later targets. For a chained form whose fields feed one explicit submit — e.g. email then password then a `tap: "Log in"` — set `submit: false` on the intermediate fields so a premature Enter doesn't fire the form early: `type: { into: password, text: "hunter2", submit: false }`. diff --git a/packages/tool-server/src/tools/flows/flow-actions.ts b/packages/tool-server/src/tools/flows/flow-actions.ts index f45054b17..4356efd45 100644 --- a/packages/tool-server/src/tools/flows/flow-actions.ts +++ b/packages/tool-server/src/tools/flows/flow-actions.ts @@ -19,10 +19,16 @@ import { type WaitCondition, type TextMatchMode, } from "../../utils/ui-tree-match"; -import { sleepOrAbort } from "../../utils/timing"; +import { settleWithin, sleepOrAbort } from "../../utils/timing"; import { invokeSubTool } from "../../utils/sub-invoke"; import { bindDeviceArgs } from "./flow-device"; import { fetchFlowTree } from "./flow-tree"; +import { + capturePixelsWithin, + comparePixels, + statusBarMaskFraction, + type PixelFrame, +} from "./flow-pixels"; import { buildAxisCandidate, decomposePinch, @@ -40,6 +46,10 @@ import { import { describeSelector, describeTextExpectation, + IDLE_DEFAULT_STABLE_FOR_MS, + IDLE_DEFAULT_TIMEOUT_MS, + IDLE_MIN_STILL_INTERVALS, + IDLE_POLL_MS, SELECTOR_RELATIONS, type FlowSelector, type FlowStep, @@ -66,9 +76,16 @@ export interface DirectiveOutcome { * blind/degraded tree), or a `hidden` check ended on a blind or failed * read after the element had matched. Read by the `when:` guard probe, * which must error rather than silently skip a block a broken tree source - * can't vouch for; a plain `assert` reports it as an ordinary failure. + * can't vouch for; a plain `assert` reports it as an ordinary failure. An + * `idle` step is the exception — it has no condition to fall back on, so the + * runner scores its indeterminate outcome `error` rather than `fail`. */ indeterminate?: boolean; + /** + * The step passed, but the WAY it passed weakens it as proof — carried into + * the step report so the author is told what the green actually bought. + */ + warning?: string; } /** @@ -84,10 +101,21 @@ export const ABORTED_OUTCOME: DirectiveOutcome = { reason: "run aborted", }; -/** The selector-acting steps {@link runDirective} handles. */ +/** The condition/action steps {@link runDirective} handles. */ export type DirectiveStep = Extract< FlowStep, - { kind: "tap" | "long-press" | "type" | "await" | "assert" | "scroll-to" | "pinch" | "rotate" } + { + kind: + | "tap" + | "long-press" + | "type" + | "await" + | "assert" + | "idle" + | "scroll-to" + | "pinch" + | "rotate"; + } >; /** Dispatch a tool with the run's resolved device id bound into its args. */ @@ -621,7 +649,11 @@ export function offscreenHint(sel: FlowSelector): string { return `no visible element matched selector ${describeSelector(sel)} — if it is off-screen, add a scroll-to step before this one`; } -/** Execute one selector-acting directive (`tap` / `long-press` / `type` / `await` / `assert` / `scroll-to` / `pinch` / `rotate`). */ +/** + * Execute one directive step: the selector-acting ones (`tap` / `long-press` / + * `type` / `await` / `assert` / `scroll-to` / `pinch` / `rotate`) plus `idle`, + * which takes no selector because stillness is a property of the whole screen. + */ export async function runDirective(env: ActionEnv, step: DirectiveStep): Promise { // Vega is remote-driven — there is no touch input, so the touch directives // can never act on it. Fail upfront with authoring guidance instead of a @@ -663,6 +695,8 @@ export async function runDirective(env: ActionEnv, step: DirectiveStep): Promise return waitForCondition(env, step, step.timeout ?? DEFAULT_ACTION_TIMEOUT_MS); case "assert": return waitForCondition(env, step, DEFAULT_ASSERT_TIMEOUT_MS); + case "idle": + return waitForIdle(env, step); case "scroll-to": { const r = await scrollToVisible(env, step.target, step.direction, step.within); if (r.aborted) return ABORTED_OUTCOME; @@ -1117,6 +1151,467 @@ async function waitForCondition( }; } +// ── Screen readiness ───────────────────────────────────────────────── +// +// `await: { idle: true }` asks one question a selector condition cannot: has +// the screen stopped moving? It is deliberately NOT an identity check — a +// dropped tap leaves the source screen perfectly idle — so it belongs next to +// the element check that says WHICH screen, never instead of it. +// +// It never fails a run. Readiness is not an acceptance criterion: the flow's +// verdict belongs to the identity and outcome checks around it, and a screen +// that keeps moving is usually a property of the app rather than a regression +// — a video, a shimmer, a carousel. On Android it is also routine: that tree +// carries live text, so a ticking timer or a relative timestamp moves it on +// every read, where the iOS tree cannot see either. Hard-failing on a signal +// that sensitive, and that different per platform, turns one flow file into +// two verdicts. So a screen that never settles is reported as a WARNING on a +// passing step, naming what to look at. + +/** + * The cadence, the interval count and the defaults all live in flow-utils + * beside the parser, which needs every one of them to reject a wait that could + * never contain the settle it asks for. Aliased here for readability. + */ +const MIN_STILL_INTERVALS = IDLE_MIN_STILL_INTERVALS; + +/** + * How much budget must be left for another round to be worth STARTING — which + * is checked before the poll sleep, so it is spent on the sleep and the round + * that follows begins with whatever is left. It is a floor on the wait, not on + * the round: what it rules out is starting a round in the last few + * milliseconds of the step, where the capture is skipped and the read is + * abandoned, and both absences used to be recorded as facts about the device — + * "captures do not work here", "the tree source is not answering" — when the + * step had simply run out of time. + * + * The first round always runs, so an unusually short `timeout:` still buys one + * honest look, and ending up to one round early is strictly better than + * judging a screen nobody managed to observe. + */ +const MIN_ROUND_BUDGET_MS = IDLE_POLL_MS; + +/** + * The screen settled, but something small on it moved while it did. A spinner + * is the case that matters: it is far too small to move the screen (a stock one + * measured 50-66 changed pixels of a phone capture — see + * LOCALIZED_MOTION_MIN_PIXELS) and it does not move the tree either, since it + * spins in a layer without its box ever changing — so both halves of the check + * agree the screen is at rest while it is still loading. This is the only place + * that difference is visible, so it is said outright. + * + * The claim is deliberately about the settle being reported and not about the + * whole step: the flag is set by ANY interval of the winning hold and cleared + * with the hold, so what it promises is that something small moved inside the + * stretch of stillness this step is passing on — not that it moved from the + * first read to the last. + */ +const LOCALIZED_MOTION_WARNING = + `the screen settled, but a small part of it was still changing while it did — a spinner, a ` + + `caret, a progress dot. If it is a loading spinner then the screen had not finished loading, ` + + `and stillness cannot tell those apart: look at what is moving, and gate the next action on ` + + `the element the loading produces rather than on this settle.`; + +/** + * How long a tree read may go unanswered before the SOURCE is what stopped + * working, rather than the step running out of time. + * + * A read is still given the whole remaining budget — a tree read on a busy + * screen genuinely takes seconds, and Android's `uiautomator dump` allows + * itself twenty — so this is not a bound on the read. It is the size of the + * gap that separates the two reasons a read fails to come back: the last read + * of a step routinely times out with a couple of hundred milliseconds to its + * name, and that is the step ending. One abandoned with seconds of budget in + * hand is a source that has wedged, and no verdict about the app may be drawn + * from a window nobody could see through. + */ +const HUNG_TREE_READ_MS = 2_000; + +/** + * Evidence-gap bound for the post-loop verdict, and the idle twin of + * {@link CONDITION_DARK_TAIL_TOLERANCE_MS}: how much of the end of the wait the + * tree source may have spent failing before "the source stopped answering" is + * the better account of the window than whatever the screen was doing. + * + * A tree-source blip is expected mid-settle — the loop restarts the hold and + * carries on — and the read that happens to END the step is no more meaningful + * than any other. Without this bound, one failed read on the last poll turned + * every benign outcome into a run-stopping error, while the identical + * transient one poll earlier passed with a warning: whether a flow survived a + * screen this step is explicit about wanting to pass came down to where the + * blip landed. + * + * Counted in ROUNDS rather than in milliseconds, unlike its `waitForCondition` + * twin, because a round here is not a poll: it is `Promise.all([read, + * capture])`, so it lasts `max(read, capture)`, and neither half is held to a + * poll — `capturePixelsWithin` grants a capture seconds of its own + * (PIXEL_CAPTURE_TIMEOUT_MS) and the read gets what is left of the step. A + * wall-clock tolerance sized at two polls therefore expired whenever a round + * merely ran long, which a capture backend that is slow but working is enough + * to do — putting the verdict back on where the blip landed, the very thing + * this bound exists to take it off. + * + * One unanswered round is what a blip costs. Consecutive ones mean the source + * went dark, which is the window this step cannot describe. + */ +const IDLE_TOLERATED_DARK_READS = 1; + +/** How the last tree read ended. Only `value` licenses a verdict about the app. */ +type TreeReadOutcome = "value" | "error" | "timeout"; + +/** + * Wait until the screen has content and stops moving — in the UI tree AND in + * the rendered pixels. + * + * Both signals are required because each is blind to what the other sees. The + * tree cannot see presentation-layer motion: an iOS push or modal dismissal + * commits its hierarchy up front and then animates a layer over ~300-500ms, and + * a cross-fade or a scrim moves no node at all — so a tree-only settle reports + * a screen that is still sliding. Pixels cannot see a tree that is still + * churning behind an unchanged-looking surface, and anything genuinely animated + * forever (a video, a shimmer) would make a pixel-only settle unsatisfiable on + * a screen the tree calls ready. + * + * This is `await-screen-idle`'s question asked against the tree the directives + * actually resolve against. It returns early the moment the screen is still, + * which is the point: the following tap resolves its target against a screen + * that has stopped, instead of racing a transition still in flight. + * + * A screen that never settles spends the whole timeout and then passes with a + * warning (see the section note above). Only an unreadable window is a hard + * stop, and it is `indeterminate` — the check could not run, which is not a + * verdict about the app. + * + * Every verdict is drawn from the LAST round that observed something, never + * from a latch remembering that the screen was once still: a screen that + * settles and then moves again has not settled. + */ +async function waitForIdle( + env: ActionEnv, + step: Extract +): Promise { + const timeoutMs = step.timeout ?? IDLE_DEFAULT_TIMEOUT_MS; + const stableFor = step.stableFor ?? IDLE_DEFAULT_STABLE_FOR_MS; + // Resolved once: it depends only on the device, and on iOS it costs a + // runtime probe the capture path memoizes anyway. + const maskTopFraction = await statusBarMaskFraction(env.device); + const deadline = Date.now() + timeoutMs; + + // Two hold clocks, because the tree can settle while the pixels have not. + // The combined one decides; the tree-only one feeds the degraded report at + // the bottom, for a run whose captures never produced a comparable pair. + let treeSignature: string | undefined; + let treeSince = 0; + let treeStillIntervals = 0; + let treeSettledAtLastRead = false; + let previousFrame: PixelFrame | undefined; + let bothSince = 0; + let stillIntervals = 0; + // Small, persistent motion seen across the intervals that produced the + // current hold. Cleared with the hold, so it only ever describes the settle + // actually being reported. + let localizedMotionDuringHold = false; + + let readsSucceeded = 0; + // Reads that came back with a tree AND something in it. Only these can + // measure an interval, so this — not readsSucceeded — is what the + // "too few reads to judge" guard at the bottom counts. A blank read is an + // observation (it resets both holds) but never evidence about motion. + let contentReads = 0; + // Definitely assigned: the loop below always completes at least one round, + // and every arm of that round sets it. + let lastRead!: TreeReadOutcome; + let treeErrorMessage: string | undefined; + // Rounds since the last read that ANSWERED. Post-loop this is the dark tail: + // how much of the window's final stretch went without a look at the screen. + // A blank read clears it — it is an observation; see the blank branch. + let darkReads = 0; + let treeReadHung = false; + let sawContent = false; + let pixelsEverMoved = false; + let captureFailed = false; + let firstCapture = true; + + for (;;) { + if (env.signal?.aborted) return ABORTED_OUTCOME; + // The tree read is bounded by what is left of the step's budget, the same + // way the capture is. Without that bound `timeout:` was not an upper bound + // at all: no describe path takes a signal, and a wedged one (a hung + // ViewInspector RPC, an `adb` that has stopped answering) ran the round + // past the deadline — measured at 2.25s over an 8000ms budget. + const roundBudget = Math.max(1, deadline - Date.now()); + // Read both signals from as close to one instant as possible: they describe + // the same screen, and any gap between them is a window motion hides in. + // They also travel over different channels (tree source vs. capture + // backend), so serializing them would double the round without buying + // anything. + const [read, frame] = await Promise.all([ + settleWithin(fetchFlowTree(env.registry, env.device), roundBudget, env.signal), + capturePixelsWithin(env, deadline, firstCapture), + ]); + firstCapture = false; + // Before anything is concluded from this round: a capture abandoned by an + // abort comes back indistinguishable from one that failed, and a verdict + // about the app must never be derived from a run that was cancelled. + if (env.signal?.aborted || read.type === "aborted") return ABORTED_OUTCOME; + + if (read.type === "timeout") { + // The read did not come back inside the round. That is the absence of an + // observation, not an observation: it neither refutes the last known + // tree state nor stands in for one, so the hold state is left as it was + // and the bottom decides what, if anything, it means. + lastRead = "timeout"; + darkReads += 1; + // ...except for one thing it does say. A read abandoned with seconds of + // budget left is a source that has wedged, not a step that ran out of + // time, and the difference decides whether the bottom may describe the + // app at all. + if (roundBudget >= HUNG_TREE_READ_MS) treeReadHung = true; + } else if (read.type === "error") { + // A tree-source blip mid-animation is expected; keep polling. Only its + // presence on the LAST read is reportable. + lastRead = "error"; + darkReads += 1; + treeErrorMessage = read.error; + treeSignature = undefined; + previousFrame = undefined; + treeSince = 0; + treeStillIntervals = 0; + treeSettledAtLastRead = false; + bothSince = 0; + stillIntervals = 0; + } else { + lastRead = "value"; + readsSucceeded += 1; + darkReads = 0; + treeErrorMessage = undefined; + // It answered, so whatever wedged it has cleared. + treeReadHung = false; + const tree = read.value.tree; + if (tree.children.length === 0) { + // Blank or still loading — never "settled", and it resets both holds. + // Unlike a failed read this IS an observation, so it also clears the + // tree-only verdict: a screen showing nothing has not settled on + // anything. + treeSignature = undefined; + previousFrame = undefined; + treeSince = 0; + treeStillIntervals = 0; + treeSettledAtLastRead = false; + bothSince = 0; + stillIntervals = 0; + } else { + sawContent = true; + contentReads += 1; + const signature = treeFingerprint(tree); + const now = Date.now(); + + // Stillness is a property of an INTERVAL, so no verdict comes from one + // observation — and, per MIN_STILL_INTERVALS, none comes from one + // interval either. `stableFor: 0` therefore still means three reads: + // a single sample proves nothing about motion, and a single agreeing + // pair can be two points of an animation that reversed between them. + const treeHeld = signature === treeSignature; + treeSignature = signature; + if (!treeHeld) { + treeSince = now; + treeStillIntervals = 0; + } else { + treeStillIntervals += 1; + } + treeSettledAtLastRead = + treeStillIntervals >= MIN_STILL_INTERVALS && now - treeSince >= stableFor; + + // A missing frame is the ABSENCE of visual evidence, never evidence of + // stillness. Letting it stand in for "the pixels held" is what turned a + // screen that never stopped moving into a pass. + let pixelsHeld = false; + let localizedThisInterval = false; + if (frame === undefined) { + captureFailed = true; + } else { + if (previousFrame !== undefined) { + const change = comparePixels(previousFrame, frame, maskTopFraction); + if (change === "moving") pixelsEverMoved = true; + else { + pixelsHeld = true; + localizedThisInterval = change === "localized"; + } + } + // Only a frame that arrived replaces the reference. A missed capture + // used to overwrite it with `undefined`, which cost the NEXT round its + // comparison too — one slow capture blinded two intervals, so a + // backend that is merely intermittently slow ended up reported as one + // that could not be screenshotted at all. Holding the last good frame + // asks the same question across the gap, over a longer interval. + previousFrame = frame; + } + + if (treeHeld && pixelsHeld) { + stillIntervals += 1; + if (localizedThisInterval) localizedMotionDuringHold = true; + if (stillIntervals >= MIN_STILL_INTERVALS && now - bothSince >= stableFor) { + return localizedMotionDuringHold + ? { ok: true, warning: LOCALIZED_MOTION_WARNING } + : { ok: true }; + } + } else { + bothSince = now; + stillIntervals = 0; + localizedMotionDuringHold = false; + } + } + } + + if (env.signal?.aborted) return ABORTED_OUTCOME; + const left = deadline - Date.now(); + if (left < MIN_ROUND_BUDGET_MS) break; + if (!(await sleepOrAbort(Math.min(IDLE_POLL_MS, left), env.signal))) return ABORTED_OUTCOME; + } + + // An unreadable window is never a verdict about the app. Which flavour of + // unreadable it was decides the repair, so they stay apart. + const unreadable = (underlying: string): DirectiveOutcome => ({ + ok: false, + indeterminate: true, + // The underlying reader reports an instrumentation failure, whose remedy + // (relaunch the app) is the wrong repair for the commonest cause of it + // here: the app is simply not in the foreground, which reads exactly the + // same from the tree source. Name that first so the author checks it + // before relaunching anything. + reason: + `could not read the UI tree while waiting for the screen to settle — check the app is ` + + `still in the foreground (a backgrounded app reads the same as an uninstrumented one). ` + + `Underlying error: ${underlying}`, + }); + + if (readsSucceeded === 0) { + if (treeErrorMessage !== undefined) return unreadable(treeErrorMessage); + return { + ok: false, + indeterminate: true, + reason: + `the tree source never answered within the step's ${timeoutMs}ms — raise this step's ` + + `\`timeout:\` if it is merely slow (a tree read on a busy screen can take seconds), or ` + + `repair it if it has stopped answering altogether`, + }; + } + // Reads worked, then stopped: a backgrounded app, a dropped instrumentation + // session. One early success does not license a verdict drawn from a window + // that went dark afterwards. (A read that merely ran out of budget is NOT + // this case — it is the step ending, and the evidence below still stands.) + // + // Measured as a tail, not as a single read: the source failing on the last + // poll and the source having stopped answering are different windows, and + // only the second is unreadable. See IDLE_TOLERATED_DARK_READS. + if ( + lastRead === "error" && + treeErrorMessage !== undefined && + darkReads > IDLE_TOLERATED_DARK_READS + ) { + return unreadable(treeErrorMessage); + } + // The same window going dark the other way: the source answered, then stopped + // answering with seconds of budget still in hand. A failing read has a + // dedicated error above, but a HANGING one used to fall through to the + // motion warning and tell the author a frozen screen was a carousel. + if (lastRead === "timeout" && treeReadHung) { + return { + ok: false, + indeterminate: true, + reason: + `the UI tree source answered and then stopped: a read given at least ` + + `${HUNG_TREE_READ_MS}ms never came back, so the screen could not be observed for the ` + + `rest of the wait — check the app is still in the foreground and responding (a wedged ` + + `app reads the same as a backgrounded one)`, + }; + } + // A tolerated blip is not a silently dropped error: whichever warning below + // describes the window carries the failed read with it, the way + // waitForCondition appends its own. (The tree-only settle is the one verdict + // below that cannot be reached with a failed final read — that read cleared + // `treeSettledAtLastRead` — so it is left alone rather than given a note it + // could never print.) + const blipNote = + lastRead === "error" && treeErrorMessage !== undefined + ? ` (the read that ended the wait failed: ${treeErrorMessage})` + : ""; + + // Readable throughout and never once carrying content: the screen rendered + // nothing, which is not the same claim as "it never stopped moving". + // + // A warning, not a stop. The tree read back fine — this is an observation + // about the app, and readiness is never this step's to fail a run over. It + // also is not always the app's fault: a screen legitimately renders no + // accessible content (a bare canvas, a video surface, a splash image), and + // stopping the flow there took every later step with it, including the + // element check that would have said what was actually wrong. + if (!sawContent) { + return { + ok: true, + warning: + `the UI tree stayed empty for ${timeoutMs}ms — the screen never rendered content, so ` + + `there was nothing to settle. If the screen is meant to render accessible content, this ` + + `is where it did not; if it is a canvas or a video surface, it has none to read. Gate ` + + `the next action on an element check either way.` + + blipNote, + }; + } + // Too few reads to have judged anything. A settle needs three of them + // spanning two intervals, so a step that got fewer has no evidence either + // way — and both verdicts below would be claims about an app that was never + // observed for long enough to make one. The parser rejects a `timeout:` too + // short to fit a settle, so what reaches here is a source slow enough to eat + // the wait, or a window blank for most of it, either of which is worth + // saying rather than dressing up as motion. + // + // Counted in reads that CARRIED CONTENT, not in reads that answered: a blank + // one resets both holds and measures no interval. Counting it let a window + // that was blank for all but its last two reads sail past this guard and + // assert instead that "the screen never held still ... something on it never + // stops" — a claim about motion drawn from a single measured interval. + if (contentReads <= MIN_STILL_INTERVALS) { + return { + ok: true, + warning: + `the screen came back with content on ${contentReads} read` + + `${contentReads === 1 ? "" : "s"} in ${timeoutMs}ms, and a settle takes ` + + `${MIN_STILL_INTERVALS + 1} of them spanning ${MIN_STILL_INTERVALS} ${IDLE_POLL_MS}ms ` + + `polls — so this step ended without ever being able to tell whether the screen was ` + + `moving. Raise its \`timeout:\`, and gate the next action on a stable element rather ` + + `than on stillness.` + + blipNote, + }; + } + // The tree was settled as of the last read and no pair of captures ever + // showed motion, yet the combined hold never completed. With captures + // arriving this is unreachable: a pair either agrees — and the tree was + // holding, so the hold would have run — or disagrees, which sets + // pixelsEverMoved. So `captureFailed` is what is left, and it is required + // here rather than assumed. The hierarchy genuinely held still, so this is a + // pass; half of the proof is missing, so it is a warned one. + if (treeSettledAtLastRead && !pixelsEverMoved && captureFailed) { + return { + ok: true, + warning: + `settled on the UI tree alone — this screen could not be screenshotted on enough polls ` + + `to compare a pair of them, so animation that moves pixels without moving nodes (a push, ` + + `a fade, a dismissing modal) was not waited out. Follow this with the element check the ` + + `next step actually needs.`, + }; + } + return { + ok: true, + warning: + `the screen never held still for ${stableFor}ms within ${timeoutMs}ms, so this step went ` + + `ahead without waiting it out. Either something on it never stops (a video, a looping ` + + `animation, a carousel, live-updating text) or the screen never finished loading. Look at ` + + `what is moving, and make sure the next action is gated on a stable element rather than on ` + + `stillness.` + + blipNote, + }; +} + function assertReason( condition: WaitCondition, selector: FlowSelector, diff --git a/packages/tool-server/src/tools/flows/flow-device.ts b/packages/tool-server/src/tools/flows/flow-device.ts index 22a2d1da4..93a34b574 100644 --- a/packages/tool-server/src/tools/flows/flow-device.ts +++ b/packages/tool-server/src/tools/flows/flow-device.ts @@ -201,10 +201,12 @@ export function stripDeviceKeys(args: Record): Record { + if (device.platform === "android") return STATUS_BAR_MASK_FRACTION; + if (device.platform !== "ios") return 0; + return (await isTvOsSimulator(device.id)) ? 0 : STATUS_BAR_MASK_FRACTION; +} + +// `httpScreenshot` may spend its full first-frame wait before it even returns +// a file path. Leave a separate completion margin for reading, decoding, and +// removing that PNG. Warm captures get the tighter bound below. +const FIRST_CAPTURE_COMPLETION_MARGIN_MS = 500; +export const FIRST_PIXEL_CAPTURE_TIMEOUT_MS = + FIRST_FRAME_WAIT_MS + FIRST_CAPTURE_COMPLETION_MARGIN_MS; +export const PIXEL_CAPTURE_TIMEOUT_MS = 2_000; + +/** + * Per-capture bound — a ceiling, not a wait, so granting more than a route + * needs costs nothing until it is actually spent. + * + * Only a simulator-server-backed capture can spend {@link FIRST_FRAME_WAIT_MS} + * polling for its stream's first frame. Chromium answers from CDP and Vega + * shells out to the emulator console, so neither has a stream to warm up and + * both keep the warm bound throughout. A tvOS simulator shells out too, but it + * is not distinguishable from an iOS one without an async runtime probe, so it + * keeps the wider first-capture ceiling it will not use. + */ +export function pixelCaptureTimeoutMs(device: ActionEnv["device"], firstCapture: boolean): number { + const warmFromTheStart = device.platform === "chromium" || device.platform === "vega"; + return firstCapture && !warmFromTheStart + ? FIRST_PIXEL_CAPTURE_TIMEOUT_MS + : PIXEL_CAPTURE_TIMEOUT_MS; +} + +/** + * Chromium's downscale, taken from the compositor rather than from `sharp`. + * + * The `screenshot` tool's Chromium route resizes the captured PNG with + * `sharp`, which is an optional dependency nothing in this repo installs — so + * asking it for a quarter-scale frame returned a full-resolution one, and a + * settle decoded a 2400x2558 PNG into a 23MB buffer twice a second, blocking + * the shared tool-server's event loop for ~79ms of every 200ms round. + * + * `Page.captureScreenshot`'s own `clip.scale` is applied while rasterizing, so + * the small frame is the only one that ever exists: no resize step, no + * dependency, and nothing to decode but the quarter-scale image. `clip` is + * measured in CSS pixels but its scale composes with the page's device scale + * factor, so passing CAPTURE_SCALE straight through lands on a quarter of the + * frame this route used to return — the same reduction every other route + * applies. Measured on a 900x613 viewport at dpr 2: 1800x1226 and ~25ms of + * blocking decode per poll became 450x307 and ~3ms. + * + * A `clip` is measured from the top of the DOCUMENT, not of the window, so its + * origin has to follow the scroll — `{ x: 0, y: 0 }` names the top of the page, + * which on a scrolled document is off-screen and rasterizes as a blank + * rectangle. Two blank rectangles compare as identical, so that origin made the + * pixel half of the check vote "still" on every interval of a visibly animating + * screen, and vote it silently: the capture succeeded, so nothing warned. The + * offset comes from `Page.getLayoutMetrics`, which is a browser-side read of + * the frame's own layout — unlike the `Runtime.evaluate` behind the cached + * viewport, it does not depend on a live main world, so it survives the + * mid-navigation renderer this check runs against. A page whose scrolling lives + * in an inner element reports no document scroll and clips at the origin, which + * is already the right rectangle for it. + * + * The viewport SIZE is still the cached one: refreshing it does cost a + * `Runtime.evaluate` per poll. A window resized mid-step therefore clips + * against the previous size for the rest of it, and registers as content + * change like anything else that moves. + */ +async function captureChromiumPng(env: ActionEnv): Promise { + const ref = chromiumCdpRef(env.device); + const api = (await env.registry.resolveService(ref.urn, ref.options)) as ChromiumCdpApi; + const { width, height } = api.getViewport(); + const { x, y } = await chromiumScrollOffset(api); + const shot = (await api.cdp.send("Page.captureScreenshot", { + format: "png", + captureBeyondViewport: false, + clip: { x, y, width, height, scale: CAPTURE_SCALE }, + })) as { data?: string }; + if (!shot.data) throw new Error("Page.captureScreenshot returned no data"); + return Buffer.from(shot.data, "base64"); +} + +/** What `Page.getLayoutMetrics` reports about where the window sits in the page. */ +interface CssViewportMetrics { + pageX?: number; + pageY?: number; +} + +/** + * Where the window's top-left corner sits in document coordinates, in CSS + * pixels — the origin {@link captureChromiumPng} must clip from. + * + * `cssVisualViewport` is preferred because it also carries a pinch-zoom offset; + * `cssLayoutViewport` is the fallback for a protocol that predates it. A read + * that fails or answers with nothing usable falls back to the document origin, + * which is the unscrolled answer and no worse than not asking. + */ +async function chromiumScrollOffset(api: ChromiumCdpApi): Promise<{ x: number; y: number }> { + try { + const metrics = (await api.cdp.send("Page.getLayoutMetrics")) as { + cssVisualViewport?: CssViewportMetrics; + cssLayoutViewport?: CssViewportMetrics; + layoutViewport?: CssViewportMetrics; + }; + const vp = metrics.cssVisualViewport ?? metrics.cssLayoutViewport ?? metrics.layoutViewport; + const x = vp?.pageX; + const y = vp?.pageY; + return { + x: typeof x === "number" && Number.isFinite(x) ? x : 0, + y: typeof y === "number" && Number.isFinite(y) ? y : 0, + }; + } catch { + return { x: 0, y: 0 }; + } +} + +/** + * Capture one downscaled screenshot to a temp file, routed exactly as the + * `screenshot` tool routes it: tvOS and Vega through their own shells (neither + * has a simulator-server backend), everything else through the simulator-server + * both iOS and Android share. Chromium does not appear here — it answers with + * bytes, never a file (see captureChromiumPng). + * + * The `screenshot` tool itself is deliberately not reused: it registers every + * capture as an artifact, and a settle takes tens of them per step. + */ +async function captureFile(env: ActionEnv, budgetMs: number): Promise { + if (env.device.platform === "vega") { + return captureVegaScreenshotPng({ scale: CAPTURE_SCALE }); + } + // Shape alone cannot tell tvOS from iOS — both are 8-4-4-4-12 UUIDs tagged + // `platform: "ios"` — so ask the runtime, which is memoized per UDID. + if (env.device.platform === "ios" && (await isTvOsSimulator(env.device.id))) { + // This one DOES take the signal, unlike the simulator-server arm below. + // `tvScreenshot` forwards it to `execFileAsync`, and without it a wedged + // `xcrun simctl io screenshot` is never killed — the round abandons the + // promise and the next poll 200ms later spawns another, so a stuck + // subprocess becomes a growing pile of them. The `screenshot` tool's own + // tvOS route has always bounded it the same way. Nothing is orphaned that + // is not already: on a severed capture the temp path never comes back, but + // the file is a deterministic one under tmpdir and the alternative is an + // unbounded process. + return tvScreenshot(env.device.id, CAPTURE_SCALE, captureAbortSignal(env, budgetMs)); + } + const ref = simulatorServerRef(env.device); + const api = (await env.registry.resolveService(ref.urn, ref.options)) as SimulatorServerApi; + // Deliberately NOT threading env.signal into this capture: the + // simulator-server writes its temp PNG to disk before replying, and the + // reply is the only place the path is learned — severing the fetch on abort + // would orphan that file. Cancellation stays responsive regardless, because + // callers abandon this promise via settleWithin; the capture just runs to + // completion on its own bounds, learns the path, and the `finally` in + // capturePixels removes the file — the same ownership the Chromium arm above + // has, whose captureScreenshot takes no signal either. + const { path } = await httpScreenshot(api, undefined, undefined, CAPTURE_SCALE); + return path; +} + +/** + * One capture as PNG bytes. Every route but Chromium's writes a temp file, + * which is scratch and never an artifact — it is removed as soon as it has + * been read, whether or not the read worked. + */ +async function capturePng(env: ActionEnv, budgetMs: number): Promise { + if (env.device.platform === "chromium") return captureChromiumPng(env); + const file = await captureFile(env, budgetMs); + try { + return await fs.readFile(file); + } finally { + await fs.rm(file, { force: true }).catch(() => {}); + } +} + +/** + * One capture as decoded pixels, or `undefined` when the pixels could not be + * read (any capture or decode failure). Soft by design — the caller treats it + * as the ABSENCE of visual evidence, never as evidence of stillness. + */ +async function capturePixels(env: ActionEnv, budgetMs: number): Promise { + try { + const png = PNG.sync.read(await capturePng(env, budgetMs)); + return { width: png.width, height: png.height, data: png.data }; + } catch { + return undefined; + } +} + +/** + * One capture bounded by both its own per-capture budget and the caller's + * deadline. Every way of not getting a frame collapses to `undefined` — the + * caller has one response to all of them (no visual evidence this round), so + * distinguishing them here would only invent a difference it cannot act on. + * Abort is the caller's to notice: it holds the signal and checks it either + * side of this call. + */ +export async function capturePixelsWithin( + env: ActionEnv, + deadline: number, + firstCapture: boolean +): Promise { + const budget = Math.min(deadline - Date.now(), pixelCaptureTimeoutMs(env.device, firstCapture)); + if (budget <= 0) return undefined; + const result = await settleWithin(capturePixels(env, budget), budget, env.signal); + return result.type === "value" ? result.value : undefined; +} + +/** + * The signal a shell-out capture is bounded by: the run's own abort, plus this + * capture's budget, so the subprocess dies with the round that stopped waiting + * for it rather than outliving the whole step. + */ +function captureAbortSignal(env: ActionEnv, budgetMs: number): AbortSignal { + const bound = AbortSignal.timeout(budgetMs); + return env.signal ? AbortSignal.any([env.signal, bound]) : bound; +} + +/** + * How much of the screen changed between two captures. + * + * - `moving` — the screen is in motion and has not settled. + * - `localized` — something small never stopped: a spinner, a caret, a + * progress dot. Not enough to call the screen unsettled, but the caller + * reports it, because a spinner means the screen never finished loading and + * nothing else here can see the difference. + * - `still`. + * + * Alpha is ignored — a screen capture is opaque. + * + * `maskTopFraction` excludes that fraction of rows at the top of the frame, + * both from the count and from the total the fractions are taken against — see + * {@link statusBarMaskFraction} for which devices need it and why. + * + * Different dimensions count as motion. It is NOT how a device rotation is + * caught: the Android capture keeps its portrait shape across one, so rotation + * registers through content change like anything else. On Chromium the branch + * is effectively unreachable — the clip is built from the cached viewport, so a + * resize does not change the captured dimensions until something refreshes it. + */ +export function comparePixels(a: PixelFrame, b: PixelFrame, maskTopFraction = 0): PixelChange { + if (a.width !== b.width || a.height !== b.height) return "moving"; + const maskedRows = Math.min(a.height, Math.floor(a.height * maskTopFraction)); + const total = a.width * (a.height - maskedRows); + if (total <= 0) return "still"; + const limit = Math.min(a.data.length, b.data.length); + let changed = 0; + for (let o = maskedRows * a.width * 4; o + 2 < limit; o += 4) { + const dr = a.data[o] - b.data[o]; + const dg = a.data[o + 1] - b.data[o + 1]; + const db = a.data[o + 2] - b.data[o + 2]; + if (dr * dr + dg * dg + db * db > PIXEL_THRESHOLD_SQUARED) changed++; + } + const fraction = changed / total; + if (fraction > MOTION_FRACTION) return "moving"; + return changed >= LOCALIZED_MOTION_MIN_PIXELS ? "localized" : "still"; +} diff --git a/packages/tool-server/src/tools/flows/flow-run.ts b/packages/tool-server/src/tools/flows/flow-run.ts index 6e10c88f0..e7cfb6413 100644 --- a/packages/tool-server/src/tools/flows/flow-run.ts +++ b/packages/tool-server/src/tools/flows/flow-run.ts @@ -188,6 +188,17 @@ export interface StepReport { * the runner does not own reports no reason. */ reason?: string; + /** + * The step passed, but the WAY it passed weakens it as proof. Rendered as a + * "⚠" suffix by the MCP client, and under the step line by the CLI. Raised by + * `await: { idle: true }`: the screen never settled at all (it waits, then + * goes ahead); something small on it never stopped, which is what a spinner + * looks like; it rendered no content to settle; too few reads came back with + * content for it to judge anything; or its captures never produced a + * comparable pair, leaving stillness proved on the UI tree alone without the + * presentation-layer motion the pixel half exists to catch. + */ + warning?: string; /** Underlying tool id for `tool` steps. */ tool?: string; /** Tool result for `tool` steps. */ @@ -915,7 +926,13 @@ reads "inside card inside list", each container's frame inside the next); (\`pinch: { on?, scale }\` — scale > 1 in, < 1 out; screen center when \`on\` is omitted); \`rotate\` is the two-finger rotation gesture (\`rotate: { on?, by }\` — degrees, + clockwise, within ±3000°; screen center when \`on\` is omitted; distinct from the \`rotate\` tool, which changes device orientation); \`await\` waits -for a UI condition; \`wait\` pauses for a fixed number of milliseconds; \`assert\` checks one now; \`snapshot\` +for a UI condition, and additionally takes the one condition that has no selector: \`idle: true\` waits +until the screen has content and stops moving in BOTH the UI tree and the rendered pixels (it never +fails a run — a screen that never settles passes carrying a \`warning\`, which is what makes it safe to +persist; the one outcome that does stop the run is an \`error\` for a tree source that could not be read +at all — a broken window rather than a verdict about the app, which leaves the run not-ok and skips +every later step; it says nothing about WHICH screen settled — a dropped tap leaves the source screen +perfectly idle — so pair it with the element check that names the destination); \`wait\` pauses for a fixed number of milliseconds; \`assert\` checks one now; \`snapshot\` diffs a screenshot — or, with \`cropOn: \`, one element's cropped region — against a stored baseline (a missing baseline fails the step — set updateBaselines to adopt the current screen; a cropped element whose size drifted fails on dimensions); \`echo\` annotates; \`run\` executes another flow @@ -1549,6 +1566,10 @@ function stepTarget(step: FlowStep): string | undefined { case "await": case "assert": return conditionLabel(step, selectorLabel); + case "idle": + // The caller already prints the kind, and this step has no target beyond + // the screen itself: returning one would render as "idle screen idle". + return undefined; case "when": return step.condition.kind === "platform" ? `platform ${step.condition.platform}` @@ -2098,6 +2119,7 @@ async function execLeafStep( case "type": case "await": case "assert": + case "idle": case "scroll-to": case "pinch": case "rotate": { @@ -2109,7 +2131,25 @@ async function execLeafStep( // A run cancelled mid-directive is a skip (matching the pre-step guard // and `wait`), never a step failure — the app did nothing wrong. if (r.aborted) return { ...base, status: "skip", reason: r.reason }; - return { ...base, status: r.ok ? "pass" : "fail", reason: r.reason }; + // `indeterminate` is `idle`'s only non-passing outcome: a screen that + // merely kept moving passes with a warning, and so does one that + // rendered nothing, so what is left here is a wait that could not run + // at all — a tree source that failed, or one that answered and then + // wedged. Scoring that `fail` would make CI + // read an environment problem as a regression and a QA author reset a + // pass streak over it. `error` keeps the run non-ok while saying + // plainly that the app was never judged. Scoped to `idle`, whose whole + // verdict rests on being able to observe the screen; the selector + // conditions keep their existing `fail` mapping. + if (!r.ok && r.indeterminate && step.kind === "idle") { + return { ...base, status: "error", reason: r.reason }; + } + return { + ...base, + status: r.ok ? "pass" : "fail", + reason: r.reason, + ...(r.warning !== undefined ? { warning: r.warning } : {}), + }; } catch (err) { return { ...base, status: "error", reason: errMsg(err) }; } diff --git a/packages/tool-server/src/tools/flows/flow-utils.ts b/packages/tool-server/src/tools/flows/flow-utils.ts index 3b42e93d6..1824df085 100644 --- a/packages/tool-server/src/tools/flows/flow-utils.ts +++ b/packages/tool-server/src/tools/flows/flow-utils.ts @@ -706,6 +706,14 @@ export type FlowStep = expectedText?: string; textMatch?: TextMatchMode; } + /** + * Screen READINESS: the UI tree has content, and neither it nor the rendered + * pixels are still changing. Spelled `await: { idle: true }` — a condition + * like any other, but the only one that takes no selector, because stillness + * is a property of the whole screen. There is no `assert` form: "has it + * stopped moving yet" is inherently a wait. + */ + | { kind: "idle"; timeout?: number; stableFor?: number } | { kind: "wait"; ms: number } | { kind: "scroll-to"; target: FlowSelector; direction: ScrollDirection; within?: FlowSelector } | { kind: "pinch"; selector?: FlowSelector; scale: number } @@ -843,6 +851,14 @@ type YamlWaitCondition = type YamlTextWaitCondition = Extract; +/** + * The one condition that takes no selector. It shares the `await:` key with + * {@link YamlWaitCondition} but is deliberately NOT part of that union — a step + * body carries either a selector condition or this one, never a mix — so it is + * parsed by {@link parseIdleFields} rather than by parseWaitFields. + */ +type YamlIdleCondition = { idle: true; stableFor?: number; timeout?: number }; + /** `scroll-to` body: a bare target (scrolls down), or a map with options. */ type YamlScrollBody = | YamlSelector @@ -867,7 +883,7 @@ type YamlStep = | { tap: TapBody } | { "long-press": YamlTarget | { on: YamlTarget; duration?: number } } | { type: { into: YamlSelector; text: string; submit?: boolean } } - | { await: YamlWaitCondition & { timeout?: number } } + | { await: (YamlWaitCondition & { timeout?: number }) | YamlIdleCondition } | { assert: YamlWaitCondition } | { wait: number } | { "scroll-to": YamlScrollBody } @@ -1150,10 +1166,24 @@ function waitToYaml( return body; } +/** + * Sugar an `idle` step back under its `await:` key. Optional fields are emitted + * only when set, so the canonical minimal spelling (`await: { idle: true }`) + * round-trips unchanged. + */ +function idleToYaml(step: Extract): YamlStep { + const body: YamlIdleCondition = { idle: true }; + if (step.stableFor !== undefined) body.stableFor = step.stableFor; + if (step.timeout !== undefined) body.timeout = step.timeout; + return { await: body }; +} + function toYamlStep(step: FlowStep): YamlStep { switch (step.kind) { case "echo": return { echo: step.message }; + case "idle": + return idleToYaml(step); case "launch": return { launch: step.app }; case "run": @@ -1568,18 +1598,22 @@ type WaitFields = { * `assert` carrying one is rejected rather than silently ignored. */ function parseWaitFields(raw: unknown, kind: "await" | "assert" | "when"): WaitFields { + // What the author is allowed to write, which is NOT the same as what this + // function parses: a body naming `idle` is routed to parseIdleFields before + // we get here, so this list is only ever read by an author whose body named + // no legal condition, or more than one — and omitting `idle` from it left the + // one condition they may have been reaching for out of the answer. Only + // `await` gains it; `assert` and `when:` genuinely have no readiness form. + const legalKeys = kind === "await" ? [...WAIT_CONDITIONS, IDLE_CONDITION] : WAIT_CONDITIONS; if (raw === null || typeof raw !== "object") { - badEntry({ [kind]: raw }, `${kind} needs a condition (${WAIT_CONDITIONS.join(", ")})`); + badEntry({ [kind]: raw }, `${kind} needs a condition (${legalKeys.join(", ")})`); } const b = raw as Record; // The condition is the key; its value is the selector. const present = WAIT_CONDITIONS.filter((c) => c in b); if (present.length !== 1) { - badEntry( - { [kind]: b }, - `${kind} needs exactly one condition key (${WAIT_CONDITIONS.join(", ")})` - ); + badEntry({ [kind]: b }, `${kind} needs exactly one condition key (${legalKeys.join(", ")})`); } const condition = present[0]!; @@ -1591,16 +1625,7 @@ function parseWaitFields(raw: unknown, kind: "await" | "assert" | "when"): WaitF "assert has no timeout — it is an immediate check; use `await` for a timed wait" ); } - // Like `wait`, reject non-finite values: YAML `.inf` (or an overflowing - // literal like 1e400) parses to Infinity — typeof number and > 0 — which - // would make the runner's poll deadline unreachable and the await unbounded. - if (typeof b.timeout !== "number" || !Number.isFinite(b.timeout) || b.timeout <= 0) { - badEntry( - { [kind]: b }, - "await.timeout needs a positive number of milliseconds (e.g. `timeout: 10000`)" - ); - } - timeout = b.timeout as number; + timeout = parseAwaitTimeout({ [kind]: b }, b.timeout); } // `await` takes the condition key plus `timeout`; `assert` the condition key @@ -1658,6 +1683,177 @@ function parseWaitFields(raw: unknown, kind: "await" | "assert" | "when"): WaitF return { condition, selector: parseSelector(b[condition], `${kind}.${condition}`), timeout }; } +/** + * The one condition key that takes no selector. Its presence in an + * `await`/`assert` body routes parsing away from {@link parseWaitFields} — see + * {@link parseIdleFields}. + */ +const IDLE_CONDITION = "idle"; + +/** + * `idle`'s defaults and cadence, spelled here rather than beside the runner + * because the parser needs all of them: a wait that cannot contain the settle + * it asks for can never be satisfied, and this file rejects unsatisfiable + * gates. The runner imports them back. + */ +export const IDLE_DEFAULT_TIMEOUT_MS = 7500; +export const IDLE_DEFAULT_STABLE_FOR_MS = 250; + +/** `idle` poll cadence, matching `await-screen-idle`'s own. */ +export const IDLE_POLL_MS = 200; + +/** + * How many consecutive intervals must read as still before the screen is + * called settled. Two, not one, because a single agreeing pair of captures is + * not evidence of stillness: any animation that reverses — a cross-fade, a + * pulse, a bounce — has a turning point, and two samples straddling it come + * back identical while the screen is very much moving. Observed on a 3s + * white/indigo cross-fade, where a default-shaped step passed on roughly one + * run in three. A second agreeing interval needs a third sample, which the + * same phase symmetry cannot supply unless the animation's period happens to + * match the poll — so the aliasing that survives one comparison does not + * survive two. + */ +export const IDLE_MIN_STILL_INTERVALS = 2; + +/** + * What a settle costs before any hold is counted: the intervals it is measured + * over, plus the budget the round that closes it needs to be allowed to start + * ({@link IDLE_POLL_MS} again — see the runner's MIN_ROUND_BUDGET_MS). A + * `timeout:` under this cannot produce a clean settle however still the screen + * is, so the step would report on a screen it never had the chance to judge. + */ +export const IDLE_SETTLE_OVERHEAD_MS = (IDLE_MIN_STILL_INTERVALS + 1) * IDLE_POLL_MS; + +/** + * Absolute ceiling on the hold, so an obviously wrong unit (seconds, or a + * pasted timestamp) is rejected as a number rather than silently becoming a + * gate no run can pass. The relationship that actually matters is with + * `timeout`, checked separately. + */ +const IDLE_MAX_STABLE_FOR_MS = 600_000; + +/** + * The `timeout` sibling key an `await` may carry, spelled once for both the + * selector conditions and `idle`. + * + * Non-finite values are rejected alongside non-positive ones: YAML `.inf` (or + * an overflowing literal like 1e400) parses to Infinity — typeof number and + * > 0 — which would make the runner's poll deadline unreachable and the await + * unbounded. + */ +function parseAwaitTimeout(entry: unknown, value: unknown): number { + if (typeof value !== "number" || !Number.isFinite(value) || value <= 0) { + badEntry( + entry, + "await.timeout needs a positive number of milliseconds (e.g. `timeout: 10000`)" + ); + } + return value as number; +} + +/** Bounded non-negative integer option, in milliseconds. */ +function parseBoundedMs(entry: unknown, value: unknown, where: string, max: number): number { + if (typeof value !== "number" || !Number.isInteger(value) || value < 0 || value > max) { + badEntry(entry, `${where} needs an integer between 0 and ${max} (milliseconds)`); + } + return value as number; +} + +/** + * Parse an `await`/`assert` body carrying the `idle` condition. Returns the + * finished step, because unlike the selector conditions it has no selector to + * hand back as fields. + * + * `assert` is rejected outright: waiting is the whole point of the check. + */ +function parseIdleFields(raw: Record, kind: "await" | "assert"): FlowStep { + const entry = { [kind]: raw }; + + if (kind !== "await") { + // Name the other condition's home too when the body carries one. Reporting + // the mixing error first sent the author to a second round trip: splitting + // `assert: { idle: true, visible: X }` as instructed yields + // `assert: { idle: true }`, which has no assert form either. + const mixed = WAIT_CONDITIONS.filter((c) => c in raw); + badEntry( + entry, + "idle has no assert form — it waits for the screen to stop changing, which is an `await`" + + (mixed.length > 0 + ? `. Give it its own step as \`await: { idle: true }\` and leave \`${mixed.join( + "`, `" + )}\` in the assert — a step checks exactly one condition` + : "") + ); + } + rejectUnknownKeys(entry, raw, ["idle", "stableFor", "timeout"], kind); + + // `idle: true` only. A falsey value would spell "assert the screen is NOT + // settled", which no flow wants and the runner cannot answer. + if (raw.idle !== true) { + badEntry(entry, "idle takes only `true` (`await: { idle: true }`)"); + } + + const step: Extract = { kind: "idle" }; + if ("timeout" in raw) step.timeout = parseAwaitTimeout(entry, raw.timeout); + if (raw.stableFor !== undefined) { + step.stableFor = parseBoundedMs(entry, raw.stableFor, "idle.stableFor", IDLE_MAX_STABLE_FOR_MS); + } + + // A wait that cannot contain the settle it asks for is a gate that never + // passes, however still the screen is — and it does not fail quietly: the + // step spends its whole timeout and then reports either that the screen + // never stopped moving or that it could not be screenshotted, both of them + // claims about an app that did nothing. Which one it picks depends on where + // the budget ran out, so the same file yields different verdicts run to run. + // Caught here, deviceless. + // + // Checked against the EFFECTIVE hold, not just a written-out one: the + // default is what most steps run with, so leaving it out was the way to get + // an unsatisfiable step past the parser (`timeout: 100` was accepted while + // the identical `timeout: 100, stableFor: 250` was rejected). + const timeoutMs = step.timeout ?? IDLE_DEFAULT_TIMEOUT_MS; + const stableFor = step.stableFor ?? IDLE_DEFAULT_STABLE_FOR_MS; + const needed = stableFor + IDLE_SETTLE_OVERHEAD_MS; + if (timeoutMs < needed) { + badEntry( + entry, + `idle needs a timeout of at least ${needed}ms to hold still for ` + + `${step.stableFor === undefined ? `the default ` : ``}${stableFor}ms: a settle is ` + + `${IDLE_MIN_STILL_INTERVALS + 1} reads spanning ${IDLE_MIN_STILL_INTERVALS} ` + + `${IDLE_POLL_MS}ms polls, plus the ${IDLE_POLL_MS}ms of budget the closing round has to ` + + `have left to be allowed to start, and the wait has to contain all of that as well as ` + + `the hold. Raise ` + + `\`timeout\`${step.stableFor === undefined ? "" : " or lower `stableFor`"}` + ); + } + return step; +} + +/** + * Whether an `await`/`assert` body names the `idle` condition rather than an + * ordinary selector one. Rejects a body that mixes the two rather than silently + * preferring one. + */ +function isIdleCondition(raw: unknown, kind: "await" | "assert"): boolean { + if (raw === null || typeof raw !== "object" || Array.isArray(raw)) return false; + const body = raw as Record; + if (!(IDLE_CONDITION in body)) return false; + // An `assert` body naming idle is wrong however it is spelled, so let + // parseIdleFields raise the one error that ends the matter — it folds the + // mixing advice in rather than making the author earn it on a second run. + if (kind === "assert") return true; + const selectorConditions = WAIT_CONDITIONS.filter((c) => c in body); + if (selectorConditions.length > 0) { + badEntry( + { [kind]: body }, + `${kind} mixes \`${IDLE_CONDITION}\` with \`${selectorConditions.join("`, `")}\` — a step ` + + `checks exactly one condition` + ); + } + return true; +} + /** * The platform set, spelled once: launch maps, `when: { platform }` guards * ({@link WhenPlatform}), flow-device's `FlowPlatform`, and flow-run's @@ -1991,6 +2187,17 @@ function parseWhenCondition(raw: unknown): WhenCondition { badEntry({ when: raw }, `when needs exactly one condition key (${conditionKeys})`); } const b = raw as Record; + // A guard asks what is on the screen NOW, so "has it stopped moving yet" is + // not a question it can ask. Say that outright, the way the assert form does, + // rather than listing the keys the author could have written and leaving them + // to infer that the one they did write is not among them. + if (IDLE_CONDITION in b) { + badEntry( + { when: raw }, + "when has no idle form — stillness is a wait, and a guard asks what is on the screen now. " + + "Put `await: { idle: true }` before the block instead" + ); + } const present = [...WAIT_CONDITIONS, "platform"].filter((c) => c in b); if (present.length !== 1) { badEntry({ when: raw }, `when needs exactly one condition key (${conditionKeys})`); @@ -2204,6 +2411,14 @@ function fromYamlStep(raw: YamlStep, whenDepth = 0): FlowStep { } const kinds = STEP_DIRECTIVE_KEYS.filter((k) => k in entry); if (kinds.length === 0) { + // `idle` is a condition, not a step kind, and it is the one near-miss the + // docs actively produce: every other condition is written with a selector + // beside it, so `await:` comes along for free, while this one reads like a + // directive of its own. Spell the answer rather than reporting that a step + // kind nobody wrote is unrecognized. + if (IDLE_CONDITION in entry) { + badEntry(raw, `idle is a condition, not a step kind — write it as \`await: { idle: true }\``); + } const hint = Object.keys(entry) .map((k) => closestKey(k, STEP_DIRECTIVE_KEYS)) .find((h) => h !== null); @@ -2267,12 +2482,24 @@ function fromYamlStep(raw: YamlStep, whenDepth = 0): FlowStep { return step; } + // `await:` / `assert:` carry two families of condition: the selector ones + // (visible/hidden/exists/text, matched against the UI tree) and `idle`, which + // takes no selector because stillness is a property of the whole screen. The + // body's key decides which. if ("await" in raw) { - return { kind: "await", ...parseWaitFields((raw as { await: unknown }).await, "await") }; + const body = (raw as { await: unknown }).await; + if (isIdleCondition(body, "await")) { + return parseIdleFields(body as Record, "await"); + } + return { kind: "await", ...parseWaitFields(body, "await") }; } if ("assert" in raw) { - return { kind: "assert", ...parseWaitFields((raw as { assert: unknown }).assert, "assert") }; + const body = (raw as { assert: unknown }).assert; + if (isIdleCondition(body, "assert")) { + return parseIdleFields(body as Record, "assert"); + } + return { kind: "assert", ...parseWaitFields(body, "assert") }; } if ("wait" in raw) { diff --git a/packages/tool-server/src/tools/screenshot-diff/screenshot-diff.ts b/packages/tool-server/src/tools/screenshot-diff/screenshot-diff.ts index eb7d22cd1..f0e4d0d56 100644 --- a/packages/tool-server/src/tools/screenshot-diff/screenshot-diff.ts +++ b/packages/tool-server/src/tools/screenshot-diff/screenshot-diff.ts @@ -105,6 +105,10 @@ interface DiffArtifactPaths { } const MAX_RGB_DISTANCE_SQUARED = 255 * 255 * 3; +// Sized for a baseline PNG stored across sessions, machines and OS versions, +// so it absorbs real drift. flow-pixels' PIXEL_THRESHOLD deliberately does NOT +// mirror it (it compares two captures from one live session, a far lower noise +// floor) — the two are independent by design; see the rationale there. const DEFAULT_THRESHOLD = 0.1; const DEFAULT_IGNORE_TOP_NORMALIZED_Y = 0.06; const DEFAULT_REGION_MERGE_DISTANCE = 8; diff --git a/packages/tool-server/src/tools/screenshot/index.ts b/packages/tool-server/src/tools/screenshot/index.ts index 7ee753085..38af5ae17 100644 --- a/packages/tool-server/src/tools/screenshot/index.ts +++ b/packages/tool-server/src/tools/screenshot/index.ts @@ -75,8 +75,11 @@ const capability: ToolCapability = { * tvOS screenshot path. The simulator-server backend does not support tvOS, so * capture via `xcrun simctl io screenshot` instead and (optionally) * downscale with `sips` to match the iOS/Android scale behaviour. + * + * Exported for the flow settle, which captures for motion detection rather than + * for an artifact and so cannot go through the tool wrapper above. */ -async function tvScreenshot( +export async function tvScreenshot( udid: string, scale: number, signal: AbortSignal | undefined diff --git a/packages/tool-server/src/utils/simulator-client.ts b/packages/tool-server/src/utils/simulator-client.ts index 788b85202..a0bd1bdd2 100644 --- a/packages/tool-server/src/utils/simulator-client.ts +++ b/packages/tool-server/src/utils/simulator-client.ts @@ -30,7 +30,7 @@ const DEFAULT_SCREENSHOT_SCALE = 0.3; // https://github.com/software-mansion/argent/issues/391). Poll past that // transient instead of surfacing it as a hard failure. const NO_IMAGE_ERROR = /no image to export/i; -const FIRST_FRAME_WAIT_MS = 6_000; +export const FIRST_FRAME_WAIT_MS = 6_000; const FIRST_FRAME_POLL_MS = 250; /** diff --git a/packages/tool-server/test/flows/flow-deviceless.test.ts b/packages/tool-server/test/flows/flow-deviceless.test.ts index 5f1bc5716..fe7c4773f 100644 --- a/packages/tool-server/test/flows/flow-deviceless.test.ts +++ b/packages/tool-server/test/flows/flow-deviceless.test.ts @@ -251,6 +251,7 @@ describe("stepRequiresDevice", () => { "type": true, "await": true, "assert": true, + "idle": true, "scroll-to": true, "pinch": true, "rotate": true, @@ -268,6 +269,7 @@ describe("stepRequiresDevice", () => { "type": { kind: "type", into: { text: "f" }, text: "hi" }, "await": { kind: "await", condition: "visible", selector: { text: "f" } }, "assert": { kind: "assert", condition: "visible", selector: { text: "f" } }, + "idle": { kind: "idle" }, "scroll-to": { kind: "scroll-to", target: { text: "f" }, direction: "down" }, "pinch": { kind: "pinch", scale: 2 }, "rotate": { kind: "rotate", by: 90 }, diff --git a/packages/tool-server/test/flows/flow-idle-condition.test.ts b/packages/tool-server/test/flows/flow-idle-condition.test.ts new file mode 100644 index 000000000..78ef427bf --- /dev/null +++ b/packages/tool-server/test/flows/flow-idle-condition.test.ts @@ -0,0 +1,203 @@ +import { describe, expect, it } from "vitest"; +import { parseFlow, serializeFlow, type FlowStep } from "../../src/tools/flows/flow-utils"; + +// `idle` is the one condition that takes no selector. It shares the `await:` +// key with the selector conditions, so the parse/serialize round trip and the +// mutual exclusion between the two families are the load-bearing behaviors. + +const flow = (steps: string): string => `executionPrerequisite: ""\nsteps:\n${steps}`; + +function parseSteps(steps: string): FlowStep[] { + return parseFlow(flow(steps)).steps; +} + +/** A flow's steps survive serialize → parse unchanged (canonical spelling). */ +function expectRoundTrip(steps: string): FlowStep[] { + const parsed = parseSteps(steps); + expect(parseFlow(serializeFlow({ executionPrerequisite: "", steps: parsed })).steps).toEqual( + parsed + ); + return parsed; +} + +describe("await { idle }", () => { + it("parses the readiness gate and round-trips its minimal spelling", () => { + const steps = expectRoundTrip(` - await: { idle: true }\n`); + expect(steps).toEqual([{ kind: "idle" }]); + // Minimal in, minimal out — no defaults materialize into the file. + expect(serializeFlow({ executionPrerequisite: "", steps })).toContain( + "await:\n idle: true" + ); + }); + + it("carries the optional hold and timeout", () => { + expect(expectRoundTrip(` - await: { idle: true, stableFor: 400, timeout: 9000 }\n`)).toEqual([ + { kind: "idle", stableFor: 400, timeout: 9000 }, + ]); + }); + + it("has no assert form — waiting is the whole point of the check", () => { + expect(() => parseSteps(` - assert: { idle: true }\n`)).toThrow(/idle has no assert form/); + }); + + it("takes only `true` — there is no useful 'prove the screen is moving'", () => { + expect(() => parseSteps(` - await: { idle: false }\n`)).toThrow(/idle takes only/); + }); + + it("bounds stableFor", () => { + expect(() => parseSteps(` - await: { idle: true, stableFor: -1 }\n`)).toThrow( + /idle.stableFor/ + ); + expect(() => parseSteps(` - await: { idle: true, stableFor: 1.5 }\n`)).toThrow( + /idle.stableFor/ + ); + // And from above, so a hold written in the wrong unit (seconds, a pasted + // timestamp) is rejected as a number rather than becoming a gate no run + // can pass. The ceiling is ten minutes; a `timeout` wide enough to contain + // it is what the case below checks separately. + expect(() => parseSteps(` - await: { idle: true, stableFor: 600001 }\n`)).toThrow( + /between 0 and 600000/ + ); + expect(parseSteps(` - await: { idle: true, stableFor: 600000, timeout: 600600 }\n`)).toEqual([ + { kind: "idle", stableFor: 600000, timeout: 600600 }, + ]); + }); + + // A wait that cannot contain the settle it asks for is a gate that fails on + // every run — and fails blaming the app, which is the one thing it is not + // evidence about. Caught at parse, deviceless, rather than against a live + // screen. The settle costs 600ms before any hold is counted: three reads + // spanning two 200ms polls, plus the budget the closing round has to start + // with. + it("rejects a wait that could never contain the settle it asks for", () => { + expect(() => parseSteps(` - await: { idle: true, timeout: 500, stableFor: 1000 }\n`)).toThrow( + /idle needs a timeout of at least 1600ms to hold still for 1000ms/ + ); + // With no explicit hold the DEFAULT is what has to fit — the spelling that + // slipped through, since leaving `stableFor` out was the way past the + // check that only looked at a written-out one. + expect(() => parseSteps(` - await: { idle: true, timeout: 100 }\n`)).toThrow( + /idle needs a timeout of at least 850ms to hold still for the default 250ms/ + ); + // Which is the same step as writing the default out, so it is rejected the + // same way. + expect(() => parseSteps(` - await: { idle: true, timeout: 100, stableFor: 250 }\n`)).toThrow( + /idle needs a timeout of at least 850ms/ + ); + // With no explicit timeout the default is what the hold has to fit inside. + expect(() => parseSteps(` - await: { idle: true, stableFor: 9000 }\n`)).toThrow( + /idle needs a timeout of at least 9600ms/ + ); + // The boundary itself is legal on both sides. + expect(parseSteps(` - await: { idle: true, timeout: 900, stableFor: 300 }\n`)).toEqual([ + { kind: "idle", timeout: 900, stableFor: 300 }, + ]); + expect(() => parseSteps(` - await: { idle: true, timeout: 899, stableFor: 300 }\n`)).toThrow( + /idle needs a timeout of at least 900ms/ + ); + }); + + // The one near-miss the docs actively produce: every other condition is + // written with a selector beside it, so `await:` comes along for free, while + // this one reads like a directive of its own. + it("names the spelling when `idle` is written as a step of its own", () => { + expect(() => parseSteps(` - idle: true\n`)).toThrow( + /idle is a condition, not a step kind — write it as `await: \{ idle: true \}`/ + ); + }); + + // The hold is validated as an integer while the timeout is not, so a + // fractional timeout used to be turned into a bound for the hold and asked + // for "an integer between 0 and -0.5". Nothing is derived from it now: the + // two are checked against each other as a sum, which has an answer whatever + // the timeout is. + it("rejects a timeout too small to settle in without inventing a range", () => { + const parse = (): FlowStep[] => + parseSteps(` - await: { idle: true, timeout: 0.5, stableFor: 0 }\n`); + expect(parse).toThrow(/idle needs a timeout of at least 600ms/); + expect(parse).not.toThrow(/between 0 and -/); + }); + + it("rejects a non-positive timeout", () => { + expect(() => parseSteps(` - await: { idle: true, timeout: 0 }\n`)).toThrow(/await.timeout/); + expect(() => parseSteps(` - await: { idle: true, timeout: "soon" }\n`)).toThrow( + /await.timeout/ + ); + }); +}); + +describe("condition families are mutually exclusive", () => { + it("rejects mixing a selector condition with the readiness one", () => { + expect(() => parseSteps(` - await: { idle: true, visible: { id: x } }\n`)).toThrow( + /mixes `idle` with `visible`/ + ); + }); + + // The same mix under `assert` used to cost two round trips: it reported the + // mixing first, and the split the author was told to write — + // `assert: { idle: true }` — is not valid either. One error has to end it. + it("sends an assert body that mixes the two straight to the form it needs", () => { + let message = ""; + try { + parseSteps(` - assert: { idle: true, visible: { id: x } }\n`); + } catch (err) { + message = err instanceof Error ? err.message : String(err); + } + expect(message).toContain("idle has no assert form"); + expect(message).toContain("await: { idle: true }"); + expect(message).toContain("`visible`"); + // And what it tells the author to write must itself parse. + expect(() => + parseSteps(` - await: { idle: true }\n - assert: { visible: { id: x } }\n`) + ).not.toThrow(); + }); + + it("rejects a stray key rather than ignoring it", () => { + expect(() => parseSteps(` - await: { idle: true, settleMs: 500 }\n`)).toThrow(/settleMs/); + }); + + // A typo next to an `idle:` gate used to be told that `idle` itself was not a + // legal key — the parser lists what the AUTHOR may write, which is not the + // same set as what its selector-condition branch parses. + it("offers idle when an await names no legal condition", () => { + expect(() => parseSteps(` - await: { visble: { id: home } }\n`)).toThrow( + /await needs exactly one condition key \(exists, visible, hidden, text, idle\)/ + ); + // Same list when the body isn't a condition map at all. + expect(() => parseSteps(` - await: visible\n`)).toThrow( + /await needs a condition \(exists, visible, hidden, text, idle\)/ + ); + }); + + it("does not offer it to assert or to a `when:` guard, which have no readiness form", () => { + const assertMiss = (): FlowStep[] => parseSteps(` - assert: { visble: { id: home } }\n`); + expect(assertMiss).toThrow( + /assert needs exactly one condition key \(exists, visible, hidden, text\)/ + ); + expect(assertMiss).not.toThrow(/idle/); + + const guard = (body: string) => (): FlowStep[] => + parseSteps(` - when: ${body}\n steps:\n - echo: guarded\n`); + + // A stray key carrying neither substring, so the rejected entry echoed + // back into the message cannot satisfy the negative assertion. + const stray = guard("{ visble: { id: home } }"); + expect(stray).toThrow( + /when needs exactly one condition key \(exists, visible, hidden, text, platform\)/ + ); + expect(stray).not.toThrow(/idle/); + + // And `idle` itself is not a guard — which is said outright, the way the + // assert form is, rather than left to be inferred from a list it is + // missing from. + expect(guard("{ idle: true }")).toThrow( + /when has no idle form .* Put `await: \{ idle: true \}` before the block/ + ); + }); + + it("leaves the selector conditions untouched", () => { + expect(parseSteps(` - await: { visible: { id: home-screen } }\n`)).toEqual([ + { kind: "await", condition: "visible", selector: { identifier: "home-screen" } }, + ]); + }); +}); diff --git a/packages/tool-server/test/flows/flow-idle-run.test.ts b/packages/tool-server/test/flows/flow-idle-run.test.ts new file mode 100644 index 000000000..d1508298c --- /dev/null +++ b/packages/tool-server/test/flows/flow-idle-run.test.ts @@ -0,0 +1,1045 @@ +import { afterEach, beforeEach, describe, expect, it, vi } 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 type { DescribeNode, DescribeTreeData } from "../../src/tools/describe/contract"; +import type { PixelFrame } from "../../src/tools/flows/flow-pixels"; + +// The status-bar mask asks the iOS runtime whether this UDID is a tvOS +// simulator. These UDIDs are fabricated, so a real probe would shell out to +// `xcrun simctl list` on every step and answer "unknown" each time — pin it to +// the mobile answer the cases are written against. +vi.mock("../../src/utils/ios-devices", async (importOriginal) => ({ + ...(await importOriginal()), + isTvOsSimulator: vi.fn(async () => false), +})); + +// Serve the flow tree directly (see flow-when.test.ts) — `idle` polls it. +let currentTree: () => DescribeNode; +/** Simulates a tree source that is slow, or wedged when it exceeds the step. */ +let treeDelayMs = 0; +vi.mock("../../src/tools/flows/flow-tree", () => ({ + fetchFlowTree: vi.fn(async (): Promise => { + // Pinned to the source this call started against, not to whatever the + // module-level `currentTree` holds when the delay elapses. A read the + // runner abandoned (`settleWithin` gives up; the underlying fetch does not) + // resolves after its own step, and often after `beforeEach` has repointed + // `currentTree` at the NEXT case — so an unpinned call reported an orphan + // read into a case that never made it, moving where that case's blank read + // landed and how many it saw. + const source = currentTree; + if (treeDelayMs > 0) await new Promise((r) => setTimeout(r, treeDelayMs)); + return { + tree: source(), + source: "native-devtools", + screen: { width: 390, height: 844 }, + }; + }), +})); + +// Stub only the capture; the real `comparePixels` decides whether two frames +// moved, so the comparison the check depends on is the one under test. +let currentFrame: () => PixelFrame | undefined; +/** + * How long a capture takes to come back. A live backend is not instant, and a + * round is `Promise.all([read, capture])` — so this, not the poll, is what a + * round lasts once the capture is the slow half. + */ +let captureDelayMs = 0; +/** Every `firstCapture` flag the runner passed, in order. */ +const captureFirstFlags: boolean[] = []; +vi.mock("../../src/tools/flows/flow-pixels", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + // Carries the same three inputs the real one has, so nothing the runner + // hands it can go unexercised: + // - `deadline`, which it honours by returning undefined without capturing + // once the budget is gone. A stub that ignored it hid the case where the + // runner judges a screen on a round it had no time to observe, which is + // where a missing frame used to read as stillness. + // - the abort signal, which the real capture is abandoned on. + // - `firstCapture`, which buys a cold stream's first frame a wider bound + // and must be true exactly once per step. + capturePixelsWithin: vi.fn( + async ( + env: { signal?: AbortSignal }, + deadline: number, + firstCapture: boolean + ): Promise => { + captureFirstFlags.push(firstCapture); + if (env.signal?.aborted) return undefined; + const budget = deadline - Date.now(); + if (budget <= 0) return undefined; + if (captureDelayMs > 0) { + // Bounded by what is left, the way `settleWithin` bounds the real + // one, and abandoned rather than answered late if it outlasts that. + await new Promise((r) => setTimeout(r, Math.min(captureDelayMs, budget))); + if (captureDelayMs > budget || env.signal?.aborted) return undefined; + } + return currentFrame(); + } + ), + }; +}); + +import { createRunFlowTool, type FlowRunResult } from "../../src/tools/flows/flow-run"; + +const DEVICE = "00000000-0000-0000-0000-0000000000ab"; // iOS UDID shape +let tmpDir: string; + +function n(partial: Partial & { frame: DescribeNode["frame"] }): DescribeNode { + return { role: "AXOther", children: [], ...partial }; +} + +const FULL: DescribeNode["frame"] = { x: 0, y: 0, width: 1, height: 1 }; + +function screenWith(label: string): DescribeNode { + return n({ + role: "AXWindow", + frame: FULL, + children: [n({ frame: { x: 0, y: 0, width: 1, height: 0.1 }, label })], + }); +} + +/** A 10x10 frame filled with one grey level — a uniform "screen". */ +function frameAt(level: number): PixelFrame { + const data = Buffer.alloc(10 * 10 * 4, level); + return { width: 10, height: 10, data }; +} + +/** + * A capture-sized frame (180k pixels, the order a real one has at + * CAPTURE_SCALE) that is still apart from `movingPixels` of it. Sized so a + * spinner's share of a screen can be expressed at all: on the 10x10 frames + * above, one pixel is already 1% of the screen. + */ +function frameWithMovingPixels(movingPixels: number, level: number): PixelFrame { + const [width, height] = [300, 600]; + const data = Buffer.alloc(width * height * 4, 255); + for (let i = 0; i < movingPixels; i++) { + const o = (STATUS_BAR_ROWS * width + i) * 4; + data[o] = level; + data[o + 1] = level; + data[o + 2] = level; + } + return { width, height, data }; +} + +/** + * The first row of a 600-row frame the comparison actually looks at: the + * comparator masks the top 6% on a device with a system status bar, so motion + * a case means to be SEEN has to be placed below it. Frames that move + * everywhere (frameAt) are unaffected, as is a 10-row one, where 6% floors to + * no rows at all. + */ +const STATUS_BAR_ROWS = Math.floor(600 * 0.06); + +function mockRegistry(): Registry { + return { + invokeTool: vi.fn(async (id: string) => { + if (id === "list-devices") return { devices: [] }; + if (id === "restart-app") return { restarted: true }; + return { ok: true }; + }), + getTool: vi.fn(() => undefined), + resolveService: vi.fn(async () => ({ isConnected: () => true })), + } as unknown as Registry; +} + +async function writeFlow(name: string, yaml: string): Promise { + const dir = path.join(tmpDir, ".argent", "flows"); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, `${name}.yaml`), yaml, "utf8"); +} + +async function run(name: string, signal?: AbortSignal): Promise { + const tool = createRunFlowTool(mockRegistry()); + const result = await tool.execute( + {}, + { name, project_root: tmpDir, device: DEVICE }, + // Only the signal matters here; the runner does not touch the rest. + signal ? ({ signal } as unknown as ToolContext) : undefined + ); + if (!("steps" in result)) throw new Error("expected a run result"); + return result; +} + +beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "flow-idle-")); + currentTree = () => screenWith("Home"); + currentFrame = () => frameAt(120); + treeDelayMs = 0; + captureDelayMs = 0; + captureFirstFlags.length = 0; +}); + +afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); + vi.clearAllMocks(); +}); + +// `await: { idle: true }` is the readiness check: it returns the moment the +// screen is still, so the next tap resolves against a screen that has stopped. +// It never fails a run — a screen that never settles passes with a warning, +// because readiness is not an acceptance criterion and a screen that keeps +// moving (a video, a shimmer, live-updating text on Android) is usually a +// property of the app. Only an unreadable window is a hard stop, as an error. +describe("await: { idle }", () => { + it("passes once both the tree and the pixels hold still", async () => { + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + expect(r.steps.at(-1)).toMatchObject({ kind: "idle", status: "pass" }); + // Both signals were available, so nothing about this pass is weakened. + expect(r.steps.at(-1)!.warning).toBeUndefined(); + }); + + // Stillness is a property of an interval, and one interval can alias (see + // the reversing-animation case below), so `stableFor: 0` still means "the + // first two agreeing intervals" — three reads — not "the first read". + it("never settles on one read or one interval, even with no hold requested", async () => { + let reads = 0; + currentTree = () => { + reads += 1; + return screenWith("Home"); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + expect((await run("ready")).ok).toBe(true); + // Exactly three, pinned from both sides: two would settle on a single + // agreeing pair (the aliasing case below), four would make every settle a + // poll slower than it needs to be. + expect(reads).toBe(3); + // And only the first capture of the step may claim the cold-stream bound. + expect(captureFirstFlags).toEqual([true, false, false]); + }); + + // `stableFor` is a clock, not a label: a screen that is still from the + // first read must still be held for it before the step returns. Without that + // the option means nothing, since three reads take about 400ms whatever it + // is set to. + it("holds a still screen for the whole requested hold before passing", async () => { + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2500, stableFor: 800 } +` + ); + const started = Date.now(); + const r = await run("ready"); + const elapsed = Date.now() - started; + expect(r.steps.at(-1)).toMatchObject({ kind: "idle", status: "pass" }); + expect(r.steps.at(-1)!.warning).toBeUndefined(); + expect(elapsed).toBeGreaterThanOrEqual(750); + expect(elapsed).toBeLessThan(2_400); + }); + + it("warns, and does not fail, when the tree never stops changing", async () => { + let tick = 0; + currentTree = () => screenWith(`frame ${tick++}`); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900, stableFor: 300 } +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + const step = r.steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("never held still"); + // The warning has to say what to do next, not merely that it gave up. + expect(step.warning).toContain("stable element"); + }); + + // The point of warning instead of failing: a screen that never stops moving + // is usually the app working as built (a video, a shimmer, a carousel, or — + // on Android, whose tree carries live text — a ticking timestamp). The run + // has to reach the checks that actually carry its verdict. + it("lets the rest of the flow run when the screen never settles", async () => { + let tick = 0; + currentTree = () => screenWith(`frame ${tick++}`); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900, stableFor: 300 } + - echo: reached +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + expect(r.failed).toBe(0); + expect(r.errored).toBe(0); + expect(r.steps.at(-1)).toMatchObject({ kind: "echo", status: "pass", message: "reached" }); + }); + + // The reason this check reads pixels at all: an iOS push or modal dismissal + // commits its hierarchy up front and then animates a layer for a few hundred + // milliseconds. The tree is perfectly still the whole time. + it("warns when the pixels keep moving under a motionless tree", async () => { + let level = 0; + currentFrame = () => frameAt((level += 60) % 240); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 600, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + const step = r.steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("never held still"); + }); + + // A spinner is the reason this warning exists. It is far too small to move + // the screen (a stock one covers ~0.1% of a phone display) and it does not + // move the tree either — it spins in a layer whose box never changes — so + // both halves of the check call the screen settled while it is still + // loading. The step still passes, because waiting out a caret or a spinner + // that never stops is worse than saying so, but it must SAY so. + it("warns when the screen settles with something small still moving on it", async () => { + let tick = 0; + // 40 of 180_000 pixels (0.022%) alternating: an order of magnitude under + // the motion fraction, an order above the noise floor. + currentFrame = () => frameWithMovingPixels(40, tick++ % 2 === 0 ? 0 : 40); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } + - echo: reached +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + const step = r.steps.at(-2)!; + expect(step).toMatchObject({ kind: "idle", status: "pass" }); + expect(step.warning).toContain("small part of it was still changing"); + expect(step.warning).toContain("spinner"); + // The warning is about how the settle was reached, not a refusal to settle. + expect(step.warning).not.toContain("never held still"); + }); + + // The runner pins the status bar for the whole run, but the pin lands a few + // hundred milliseconds AFTER the run starts and a nested `tool: flow-execute` + // clears it for the rest of the outer run — so this step regularly compared a + // real clock against a pinned one, or against a ticking one, and reported a + // static screen as moving or as carrying a spinner. The band is masked. + it("ignores the system status bar the run's own pin repaints", async () => { + let tick = 0; + // 400 pixels — over the motion budget for this frame — confined to the + // masked band, which is where the measured clock repaint landed. + currentFrame = () => { + const level = tick++ % 2 === 0 ? 0 : 255; + const [width, height] = [300, 600]; + const data = Buffer.alloc(width * height * 4, 255); + for (let i = 0; i < 400; i++) { + const o = (width + i) * 4; // row 1, inside the status bar + data[o] = level; + data[o + 1] = level; + data[o + 2] = level; + } + return { width, height, data }; + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step).toMatchObject({ kind: "idle", status: "pass" }); + expect(step.warning).toBeUndefined(); + }); + + it("says nothing about small motion when the screen is genuinely still", async () => { + currentFrame = () => frameWithMovingPixels(0, 0); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + expect((await run("ready")).steps.at(-1)!.warning).toBeUndefined(); + }); + + // Sub-threshold drift is encoder noise, not motion — treating it as motion + // would make the check unsatisfiable on a screen that is genuinely at rest. + it("tolerates capture noise below the motion threshold", async () => { + let level = 120; + currentFrame = () => frameAt((level += 1)); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2000, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + // `ok` alone would hold for every idle outcome but an unreadable tree, so + // it says nothing about the threshold. What this case is about is that + // +1 per channel settles CLEANLY: no motion warning, and none about a + // spinner either — noise must not be reported as something small moving. + const step = r.steps.at(-1)!; + expect(step).toMatchObject({ kind: "idle", status: "pass" }); + expect(step.warning).toBeUndefined(); + }); + + // A reversing animation — a cross-fade, a pulse, a bounce — has a turning + // point, and two samples straddling it come back identical while the screen + // is still moving. Measured on a live 3s cross-fade: a default-shaped step + // passed on roughly one run in three until a second agreeing interval was + // required. Here every other capture repeats its predecessor, so a + // one-interval rule settles and a two-interval rule cannot. + it("does not settle on a single agreeing pair of a reversing animation", async () => { + let tick = 0; + currentFrame = () => { + // 0, 60, 60, 120, 120, 180, 180, … — one still interval, never two. + const level = Math.floor((tick + 1) / 2) * 60; + tick += 1; + return frameAt(level % 240); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 1500, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.steps.at(-1)!.status).toBe("pass"); + expect(r.steps.at(-1)!.warning).toContain("never held still"); + }); + + // `timeout:` is the author's answer to "how long may this take", so it has to + // be the answer. No describe path takes an abort signal, so a wedged tree + // source (a hung ViewInspector RPC, an adb that stopped answering) used to + // run the round past the deadline — measured at 2.25s over an 8s budget on a + // live simulator. + it("honours its timeout even when the tree source stops answering", async () => { + treeDelayMs = 5_000; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 800, stableFor: 0 } +` + ); + const started = Date.now(); + const r = await run("ready"); + const elapsed = Date.now() - started; + + expect(elapsed).toBeLessThan(3_000); + const step = r.steps.at(-1)!; + expect(step.status).toBe("error"); + expect(step.reason).toContain("never answered within the step's 800ms"); + }); + + // The sibling of the case above, and the one that used to slip through: a + // source that FAILS is caught by the unreadable-tree error, but one that + // HANGS is a different outcome internally, and after any earlier read had + // succeeded it fell through to the motion warning — telling the author that + // a screen frozen by a wedged renderer was a video or a carousel. + it("reports a tree source that wedges mid-wait as indeterminate, not as motion", async () => { + let reads = 0; + currentTree = () => { + reads += 1; + // Answer the first read, then wedge for longer than the step can wait. + treeDelayMs = 60_000; + return screenWith("Home"); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2500, stableFor: 0 } + - echo: reached +` + ); + const r = await run("ready"); + expect(reads).toBe(1); + expect(r.ok).toBe(false); + const step = r.steps.find((s) => s.kind === "idle")!; + expect(step.status).toBe("error"); + expect(step.reason).toContain("answered and then stopped"); + expect(step.reason).toContain("foreground"); + // And it must not be dressed up as a verdict about what was on screen. + expect(step.reason).not.toContain("never held still"); + }); + + // A settle is three reads spanning two intervals. A step that got fewer has + // no evidence either way, and both of the verdicts it used to reach for — + // "the screen never stopped moving", "no screenshot could be read" — are + // claims about an app nobody observed for long enough to make one. + it("says it ran out of looks rather than judging a screen it barely read", async () => { + treeDelayMs = 700; // two of these do not fit in the wait + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 1200 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("content on 1 read in 1200ms"); + expect(step.warning).toContain("`timeout:`"); + // The screen was static the whole time; it must not be described as moving. + expect(step.warning).not.toContain("never held still"); + expect(step.warning).not.toContain("no pair of screenshots"); + }); + + // A blank read is an observation — it resets both holds — but it measures no + // interval, so it is not one of the three a settle takes. Counting it let a + // window blank for all but its last two reads slip past the guard above and + // reach for the motion verdict instead, telling the author that "something on + // it never stops" on the strength of one measured interval. + it("does not count a blank read as a look at the screen", async () => { + let reads = 0; + // A 250ms read plus a 200ms poll fits three rounds in 1200ms; the first + // comes back blank, so only one interval could ever be measured. + treeDelayMs = 250; + currentTree = () => (reads++ < 1 ? n({ role: "AXWindow", frame: FULL }) : screenWith("Home")); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 1200, stableFor: 0 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("content on 2 reads in 1200ms"); + expect(step.warning).not.toContain("never held still"); + }); + + it("settles on the tree alone when no screenshot can be captured, and says so", async () => { + currentFrame = () => undefined; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + const step = r.steps.at(-1)!; + expect(step.status).toBe("pass"); + // The step has no target beyond the screen itself, and the renderer + // already prints the kind — a target here would read "idle screen idle". + expect(step.target).toBeUndefined(); + expect(step.warning).toContain("UI tree alone"); + // Attributed to the capture, not to the platform: on a device where + // screenshots normally work this is a per-capture failure, not a property + // of the OS. + expect(step.warning).not.toContain("could not be captured on"); + }); + + // The tree-only report is a claim that the HIERARCHY settled, so it owes the + // same hold every other settle does. Every case that reaches it elsewhere + // runs with `stableFor: 0`, where the hold term is vacuous — drop it and + // the suite stays green while a tree that had only just stopped moving is + // reported as having settled. + it("does not report a tree-only settle when the hold was never served", async () => { + currentFrame = () => undefined; + // The tree keeps changing for the first 700ms, then holds. The last read + // lands ~1200ms in, so the hierarchy has been still for well under the + // 800ms hold, however many agreeing intervals it managed. + const startedAt = Date.now(); + let churn = 0; + currentTree = () => screenWith(Date.now() - startedAt < 700 ? `Loading ${churn++}` : "Settled"); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 1400, stableFor: 800 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).not.toContain("UI tree alone"); + expect(step.warning).toContain("never held still for 800ms"); + }); + + // The localized flag describes the hold being REPORTED, so a hold that broke + // has to clear it. Without the reset, a spinner that stopped — then a screen + // that went still — still told the author something "kept changing the whole + // time". + it("forgets small motion that stopped before the settle that gets reported", async () => { + // Round 1 has no predecessor; 2 moves a spinner's worth; 3 moves the whole + // screen, breaking the hold; 4 and 5 are identical, which is the settle. + const WHOLE_FRAME = 300 * 600; + const frames = [ + frameWithMovingPixels(0, 0), + frameWithMovingPixels(40, 0), + frameWithMovingPixels(WHOLE_FRAME, 90), + frameWithMovingPixels(WHOLE_FRAME, 90), + frameWithMovingPixels(WHOLE_FRAME, 90), + ]; + let i = 0; + currentFrame = () => frames[Math.min(i++, frames.length - 1)]; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step).toMatchObject({ kind: "idle", status: "pass" }); + expect(step.warning).toBeUndefined(); + }); + + // A capture that goes missing used to cost the settle TWO intervals, not + // one: the missing frame was also stored as the previous frame, so the next + // round had nothing to compare against either. Holding the last good frame + // asks the same question across the gap. The witness is the number of + // captures the settle takes — rounds run in lockstep, so it is exact. + it("loses only the interval a capture went missing in, not the one after it", async () => { + let captures = 0; + currentFrame = () => { + captures += 1; + return captures === 3 ? undefined : frameAt(120); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step.status).toBe("pass"); + // 1,2 hold; 3 is missing; 4 and 5 hold across the gap and settle. Six would + // mean round 4 was blinded by round 3's absence. + expect(captures).toBe(5); + // And one missed capture out of five is not "no screenshot could be read". + expect(step.warning).toBeUndefined(); + }); + + // A capture that goes missing is the ABSENCE of visual evidence. Treating it + // as evidence of stillness is how a moving screen used to pass: the round + // that outran the deadline skipped its capture, and the skip stood in for + // "the pixels held". + it("never lets a missing capture stand in for stillness while the pixels move", async () => { + let level = 0; + currentFrame = () => frameAt((level += 60) % 240); + await writeFlow( + "ready", + // A default-shaped step, whose last poll round routinely starts with no + // capture budget left. + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 1200 } +` + ); + const r = await run("ready"); + // Passing is fine; claiming the screen SETTLED is not — the warning must + // still report the motion, not the tree-only settle. + expect(r.steps.at(-1)!.warning).toContain("never held still"); + expect(r.steps.at(-1)!.warning).not.toContain("UI tree alone"); + }); + + // The default hold is what nearly every step runs with, so it is worth + // pinning somewhere the number is actually used rather than only in a parser + // message. + it("holds for 250ms by default", async () => { + let tick = 0; + currentTree = () => screenWith(`frame ${tick++}`); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900 } +` + ); + expect((await run("ready")).steps.at(-1)!.warning).toContain( + "never held still for 250ms within 900ms" + ); + }); + + // A tree source that never answers at all: no read succeeded, so there is + // nothing to reason from and the advice is about the window, not the app. + // The existing mid-wait case lets the first read land, so this branch — the + // one that produces the foreground advice from a standing start — was never + // taken. + it("reports a tree source that never answered as unreadable, naming the underlying error", async () => { + currentTree = () => { + throw new Error("native-devtools is not connected"); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900, stableFor: 0 } + - echo: unreachable +` + ); + const r = await run("ready"); + expect(r.ok).toBe(false); + const step = r.steps.find((s) => s.kind === "idle")!; + expect(step.status).toBe("error"); + expect(step.reason).toContain("could not read the UI tree"); + expect(step.reason).toContain("foreground"); + expect(step.reason).toContain("native-devtools is not connected"); + // An indeterminate readiness check stops the run rather than recording a + // regression the app never had. + expect(r.steps.at(-1)!.status).toBe("skip"); + }); + + // A blip mid-settle is expected — the hold restarts from the next good read + // rather than the step giving up or carrying its pre-blip state across. + it("restarts the hold after a failed read, and still settles", async () => { + let reads = 0; + currentTree = () => { + reads += 1; + if (reads === 2) throw new Error("transient describe failure"); + return screenWith("Home"); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).toBeUndefined(); + // 1 ok, 2 failed, 3 is a fresh start (nothing to compare against), 4 and 5 + // are the two agreeing intervals. Three would mean the blip was ignored. + expect(reads).toBe(5); + }); + + // A blip on the read that ENDS the step is still a blip. Which poll it lands + // on used to decide the whole verdict: one poll earlier it restarted the hold + // and the step passed with a warning, on the last poll it stopped the run and + // skipped every later step. So a screen this check is explicit about wanting + // to pass — a video, a shimmer, a carousel, live-updating text — turned a run + // red on timing luck alone. + it("does not stop the run when only the read that ended the wait failed", async () => { + let firstReadAt: number | undefined; + let tick = 0; + // Never settles, so the step always exits through the bottom of the loop + // and its last read is the one that decides. Measured from the first read + // rather than from the run, the threshold sits midway between the closing + // two rounds of a 900ms step (600ms and 800ms in), so only the last throws. + currentTree = () => { + firstReadAt ??= Date.now(); + if (Date.now() - firstReadAt >= 700) throw new Error("transient describe failure"); + return screenWith(`frame ${tick++}`); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900, stableFor: 0 } + - echo: reached +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + const step = r.steps.find((s) => s.kind === "idle")!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("never held still"); + // Tolerated, not swallowed: the read that failed is still named. + expect(step.warning).toContain("transient describe failure"); + // And the checks that actually carry the flow's verdict still run. + expect(r.steps.at(-1)).toMatchObject({ kind: "echo", status: "pass" }); + }); + + // ...and it stays a blip when the round runs longer than a poll. The tail + // between the last read that answered and the end of the wait is + // `sleep + max(read, capture)`, and nothing holds either half to a poll: a + // capture gets seconds of its own. Measuring the tolerance in milliseconds + // therefore expired it on any slow-but-working capture backend, which put the + // verdict back on where the blip landed — the exact thing the tolerance + // exists to remove. It is counted in rounds for that reason. + it("does not stop the run when the closing read failed and captures are slow", async () => { + captureDelayMs = 300; // > IDLE_POLL_MS, so the round outlasts the poll + let firstReadAt: number | undefined; + let tick = 0; + // Never settles. With a 300ms capture the rounds start every ~500ms, so of + // the five that fit in 2400ms only the last (t≈2000) is past the threshold. + currentTree = () => { + firstReadAt ??= Date.now(); + if (Date.now() - firstReadAt >= 1750) throw new Error("transient describe failure"); + return screenWith(`frame ${tick++}`); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2400, stableFor: 0 } + - echo: reached +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + const step = r.steps.find((s) => s.kind === "idle")!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("never held still"); + expect(step.warning).toContain("transient describe failure"); + expect(r.steps.at(-1)).toMatchObject({ kind: "echo", status: "pass" }); + }); + + // The other side of that bound: a slow round does not buy a second failing + // read the same tolerance. Two consecutive dark reads are the window this + // step cannot describe, however long the rounds that carried them took. + it("still errors when two consecutive reads failed, however slow the captures", async () => { + captureDelayMs = 300; + let firstReadAt: number | undefined; + let tick = 0; + // Threshold one round earlier than the case above, so the last two reads + // (t≈1500 and t≈2000) both throw. + currentTree = () => { + firstReadAt ??= Date.now(); + if (Date.now() - firstReadAt >= 1250) throw new Error("native-devtools is not connected"); + return screenWith(`frame ${tick++}`); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2400, stableFor: 0 } + - echo: unreachable +` + ); + const r = await run("ready"); + expect(r.ok).toBe(false); + const step = r.steps.find((s) => s.kind === "idle")!; + expect(step.status).toBe("error"); + expect(step.reason).toContain("could not read the UI tree"); + expect(r.steps.at(-1)!.status).toBe("skip"); + }); + + // The same blip against the other window it used to redden: a screen that + // read back empty throughout is an observation about the app, and a transient + // on the closing read does not turn it into a window nobody could see. + it("still reports an empty screen as empty when the closing read failed", async () => { + let firstReadAt: number | undefined; + currentTree = () => { + firstReadAt ??= Date.now(); + if (Date.now() - firstReadAt >= 700) throw new Error("transient describe failure"); + return n({ role: "AXWindow", frame: FULL, children: [] }); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900, stableFor: 0 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("never rendered content"); + expect(step.warning).toContain("transient describe failure"); + }); + + // The same for a screen that goes blank in the middle: an observation that + // resets both holds, not a gap and not a reason to give up. + it("restarts the hold after the screen goes blank, and still settles", async () => { + let reads = 0; + currentTree = () => { + reads += 1; + return reads === 2 ? n({ role: "AXWindow", frame: FULL, children: [] }) : screenWith("Home"); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, stableFor: 0 } +` + ); + const step = (await run("ready")).steps.at(-1)!; + expect(step.status).toBe("pass"); + expect(step.warning).toBeUndefined(); + expect(reads).toBe(5); + }); + + // Cancelling a run is not a verdict about the screen. The check has to stop + // promptly and report a skip, never a pass, a warning or an error. + it("stops on abort without judging the screen", async () => { + let tick = 0; + currentTree = () => screenWith(`frame ${tick++}`); // never settles + const controller = new AbortController(); + setTimeout(() => controller.abort(), 300); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 7500 } + - echo: unreachable +` + ); + const started = Date.now(); + const r = await run("ready", controller.signal); + expect(Date.now() - started).toBeLessThan(3_000); + const step = r.steps.find((s) => s.kind === "idle")!; + expect(step.status).toBe("skip"); + expect(step.reason).toContain("aborted"); + expect(step.warning).toBeUndefined(); + }); + + // One good read early does not license an app verdict drawn from a window + // that went dark afterwards: a backgrounded app or a dropped instrumentation + // session reads as "unknown", never as "still animating". + it("reports a tree source that dies mid-wait as indeterminate, not as motion", async () => { + let reads = 0; + currentTree = () => { + reads += 1; + if (reads > 1) throw new Error("native-devtools is not connected"); + return screenWith("Home"); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 700, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.ok).toBe(false); + const step = r.steps.at(-1)!; + // `indeterminate` is scored `error`, which is what stops a QA run rather + // than recording a regression the app never had. + expect(step.status).toBe("error"); + expect(step.reason).toContain("could not read the UI tree"); + expect(step.reason).toContain("foreground"); + }); + + // H1: a screen that settles and then moves again has NOT settled. The + // tree-only verdict used to be a write-once latch, so an early quiet stretch + // licensed a pass drawn from a window that spent the rest of its time + // churning — which is precisely the regression this step exists to catch. + it("does not pass on a screen that settled early and then started moving again", async () => { + currentFrame = () => undefined; // force the tree-only path + let reads = 0; + currentTree = () => { + reads += 1; + return reads <= 4 ? screenWith("Home") : screenWith(`churn ${reads}`); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2500, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.steps.at(-1)!.warning).toContain("never held still"); + }); + + // H2: the same latch let a screen that had gone BLANK by the deadline report + // ready. A blank tree is an observation, not a gap — it clears the verdict. + it("does not pass on a screen that settled early and then went blank", async () => { + currentFrame = () => undefined; + let reads = 0; + currentTree = () => { + reads += 1; + return reads <= 4 ? screenWith("Home") : n({ role: "AXWindow", frame: FULL, children: [] }); + }; + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2500, stableFor: 0 } +` + ); + expect((await run("ready")).steps.at(-1)!.warning).toContain("never held still"); + }); + + // H3: bounding the tree read by the remaining budget made the LAST read + // time out on every run, which turned every honest timeout into an + // environment `error` — deleting the hard-fail that justifies this step over + // the soft `await-screen-idle` tool. A round is not started without a budget + // to observe it with, and a read that ran out of step budget is the step + // ending, not the source failing. + it("still warns, rather than erroring, when a slow tree source keeps changing", async () => { + // 300ms per read against a 200ms tail budget: the LAST read runs out of + // step budget, which is the step ending, not the source failing. Earlier + // reads landed and saw a moving screen, so the verdict is theirs — and the + // budget the last read was given is what separates this from a source that + // wedged (see the case above). + treeDelayMs = 300; + let tick = 0; + currentTree = () => screenWith(`frame ${tick++}`); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 2000, stableFor: 0 } +` + ); + const r = await run("ready"); + expect(r.steps.at(-1)!.status).toBe("pass"); + expect(r.steps.at(-1)!.warning).toContain("never held still"); + }); + + // A step whose budget is spent mid-settle must not invent a verdict out of + // what it did not manage to observe. It has three ways to do that — blaming + // the capture, blaming motion, or claiming a settle — so this pins the + // outcome exactly rather than ruling one wording out. + it("reaches a real settle rather than a verdict about the budget that ran out", async () => { + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 1600, stableFor: 900 } +` + ); + const r = await run("ready"); + const step = r.steps.at(-1)!; + // A still screen and a working capture path: the only honest outcome is a + // clean settle, reached before the hold could exhaust the wait. + expect(step.status).toBe("pass"); + expect(step.warning).toBeUndefined(); + }); + + // A tree that reads back fine and is empty is an observation about the app, + // not a window that could not be read — so it warns like any other screen + // that did not settle, and the flow goes on to the checks that carry its + // verdict. Stopping there used to take every later step with it, including + // the element check that would have named what was actually wrong. + it("distinguishes a screen that never rendered from one that never settled", async () => { + currentTree = () => n({ role: "AXWindow", frame: FULL, children: [] }); + await writeFlow( + "ready", + `executionPrerequisite: "" +steps: + - await: { idle: true, timeout: 900 } + - echo: reached +` + ); + const r = await run("ready"); + expect(r.ok).toBe(true); + expect(r.errored).toBe(0); + const step = r.steps.find((s) => s.kind === "idle")!; + expect(step.status).toBe("pass"); + expect(step.warning).toContain("never rendered content"); + expect(step.warning).not.toContain("never held still"); + expect(r.steps.at(-1)).toMatchObject({ kind: "echo", status: "pass" }); + }); +}); diff --git a/packages/tool-server/test/flows/flow-pixels.test.ts b/packages/tool-server/test/flows/flow-pixels.test.ts new file mode 100644 index 000000000..233fb6850 --- /dev/null +++ b/packages/tool-server/test/flows/flow-pixels.test.ts @@ -0,0 +1,746 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { PNG } from "pngjs"; +import type { DeviceInfo } from "@argent/registry"; +import type { ActionEnv } from "../../src/tools/flows/flow-actions"; +import { + capturePixelsWithin, + FIRST_PIXEL_CAPTURE_TIMEOUT_MS, + PIXEL_CAPTURE_TIMEOUT_MS, + PIXEL_THRESHOLD, + comparePixels, + pixelCaptureTimeoutMs, + statusBarMaskFraction, + type PixelFrame, +} from "../../src/tools/flows/flow-pixels"; +import { isTvOsSimulator } from "../../src/utils/ios-devices"; +import { captureVegaScreenshotPng } from "../../src/utils/vega-screen"; +import { tvScreenshot } from "../../src/tools/screenshot"; +import { FIRST_FRAME_WAIT_MS } from "../../src/utils/simulator-client"; + +// The capture backends shell out to xcrun / adb / a live simulator-server, so +// stub the four routes and assert which one a device is sent down. +vi.mock("../../src/utils/ios-devices", async (importOriginal) => ({ + ...(await importOriginal()), + isTvOsSimulator: vi.fn(async () => false), +})); +vi.mock("../../src/utils/vega-screen", () => ({ + captureVegaScreenshotPng: vi.fn(), +})); +vi.mock("../../src/tools/screenshot", () => ({ tvScreenshot: vi.fn() })); + +let tmpDir: string; + +beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "flow-pixels-")); + vi.mocked(isTvOsSimulator).mockReset().mockResolvedValue(false); + vi.mocked(captureVegaScreenshotPng).mockReset(); + vi.mocked(tvScreenshot).mockReset(); +}); + +afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); +}); + +/** A solid-color RGBA frame — the unit under test only compares RGB. */ +function solid(width: number, height: number, [r, g, b]: [number, number, number]): PixelFrame { + const data = Buffer.alloc(width * height * 4); + for (let i = 0; i < width * height; i++) { + data[i * 4] = r; + data[i * 4 + 1] = g; + data[i * 4 + 2] = b; + data[i * 4 + 3] = 255; + } + return { width, height, data }; +} + +/** Flip `count` pixels of `base` to `color`, in place, returning it. */ +function withChangedPixels(base: PixelFrame, count: number, color: number): PixelFrame { + for (let i = 0; i < count; i++) { + base.data[i * 4] = color; + base.data[i * 4 + 1] = color; + base.data[i * 4 + 2] = color; + } + return base; +} + +/** Rewrite every pixel's alpha in place, returning the frame. */ +function withAlpha(base: PixelFrame, alpha: number): PixelFrame { + for (let i = 0; i < base.width * base.height; i++) { + base.data[i * 4 + 3] = alpha; + } + return base; +} + +describe("comparePixels", () => { + it("reports no motion for two identical frames", () => { + expect(comparePixels(solid(30, 30, [10, 20, 30]), solid(30, 30, [10, 20, 30]))).toBe("still"); + }); + + it("reports motion when the whole frame changes", () => { + expect(comparePixels(solid(30, 30, [0, 0, 0]), solid(30, 30, [255, 255, 255]))).toBe("moving"); + }); + + it.each<[string, [number, number, number]]>([ + ["red", [255, 0, 0]], + ["green", [0, 255, 0]], + ["blue", [0, 0, 255]], + ])("registers a full-frame change confined to the %s channel as motion", (_channel, color) => { + // Motion that lives in a single channel must clear the per-pixel gate on + // that channel's term alone — the other two contribute zero, so dropping + // any one term from the distance goes blind to exactly one of these. + expect(comparePixels(solid(30, 30, [0, 0, 0]), solid(30, 30, color))).toBe("moving"); + }); + + it("ignores a change confined to the alpha channel (a screen capture is opaque)", () => { + // Identical RGB, alpha 255 → 0 on every pixel: the docstring promises alpha + // is ignored, so this must read as still. This also pins the byte offsets — + // a comparator that read o+3 (alpha) where it meant o+2 (blue) would count + // every pixel here as changed. + expect( + comparePixels(solid(30, 30, [10, 20, 30]), withAlpha(solid(30, 30, [10, 20, 30]), 0)) + ).toBe("still"); + }); + + it("treats a dimension change as motion (a resized window)", () => { + expect(comparePixels(solid(30, 30, [0, 0, 0]), solid(30, 31, [0, 0, 0]))).toBe("moving"); + }); + + it("reads a frame with no pixels to compare as still, never as motion", () => { + // Same dimensions, so the branch above does not catch it, and there is + // nothing to count — a decoder that handed back an empty frame must not + // manufacture a verdict either way. Also the shape a full-height mask + // would take. + expect(comparePixels(solid(0, 0, [0, 0, 0]), solid(0, 0, [0, 0, 0]))).toBe("still"); + expect(comparePixels(solid(30, 30, [0, 0, 0]), solid(30, 30, [255, 255, 255]), 1)).toBe( + "still" + ); + }); + + it("compares only the bytes both frames actually carry", () => { + // Same declared dimensions but a truncated buffer — a partially decoded + // capture. Reading past the shorter one would compare against undefined + // and count NaN distances, so the loop stops at the shared length. + const short = solid(200, 200, [0, 0, 0]); + short.data = short.data.subarray(0, 40); // ten pixels' worth + expect(comparePixels(solid(200, 200, [0, 0, 0]), short)).toBe("still"); + // Only those ten differ, and ten of 40k is localized — not the whole + // frame a run past the buffer's end would report. + expect(comparePixels(solid(200, 200, [255, 255, 255]), short)).toBe("localized"); + }); + + it("ignores a sub-threshold per-pixel color drift (encoder / resample noise)", () => { + // +5 on every channel is well under the per-pixel tolerance, so no pixel + // counts as changed — two captures of a static screen must read as still. + expect(comparePixels(solid(30, 30, [100, 100, 100]), solid(30, 30, [105, 105, 105]))).toBe( + "still" + ); + }); + + it("brackets the per-pixel tolerance from both sides", () => { + // The tolerance is 0.03 x ~441.7 ~= 13.25. A uniform +7 per channel is a + // distance of ~12.1 (just under) and +8 is ~13.9 (just over), so this pair + // pins the constant tightly: loosening it trips the second expectation and + // tightening it trips the first. + expect(comparePixels(solid(30, 30, [100, 100, 100]), solid(30, 30, [107, 107, 107]))).toBe( + "still" + ); + expect(comparePixels(solid(30, 30, [100, 100, 100]), solid(30, 30, [108, 108, 108]))).toBe( + "moving" + ); + }); + + it("registers two consecutive samples of a slow uniform cross-fade as motion", () => { + // The case this tolerance sits at its current value for. A spatially + // uniform fade moves every pixel by the same amount, so it clears the + // per-pixel gate on all pixels or on none — the motion fraction is never + // the deciding term. These are two samples one poll apart of a 2s + // indigo-over-white dismissal, a per-channel delta of (16, 23, 12) — + // distance ~30.5. Above the current ~13.25 gate, but BELOW the ~44.2 that + // screenshot-diff's baseline-sized 0.1 imposes, where the settle would + // count zero pixels and report stillness while the overlay was still + // painted and still hit-testing. + expect(comparePixels(solid(30, 30, [165, 128, 193]), solid(30, 30, [149, 105, 181]))).toBe( + "moving" + ); + }); + + it("keeps its tolerance pinned, and stricter than a stored-baseline one", () => { + // Pin the value so the gate — the whole motion oracle — cannot drift + // silently, and so "restore parity with screenshot-diff" (0.1) is a + // deliberate act that trips a test rather than a quiet 3.3x widening of + // the cross-fade blind spot the case above measures. + expect(PIXEL_THRESHOLD).toBe(0.03); + }); + + it("ignores a handful of changed pixels below the motion fraction", () => { + // 900 px, fraction 0.002 → ~1.8 px budget: one changed pixel is not the + // screen moving, three is. + const base = solid(30, 30, [0, 0, 0]); + expect(comparePixels(base, withChangedPixels(solid(30, 30, [0, 0, 0]), 1, 255))).not.toBe( + "moving" + ); + expect(comparePixels(base, withChangedPixels(solid(30, 30, [0, 0, 0]), 3, 255))).toBe("moving"); + }); + + // Every case above runs on a 30x30 frame, where the motion budget is 1.8 + // pixels and anything visible trips it. A real capture at CAPTURE_SCALE is + // 158k-198k pixels, and at that size the small-but-permanent movers a + // readiness check exists to notice — a spinner above all — sit two orders of + // magnitude below the same fraction. Measured on real captures taken at the + // scale the check uses: a stock spinner moved 66 pixels of an iPhone 16 Pro + // frame (302x656) and 57 of a Pixel 5 one (270x585). + describe("at a real capture size", () => { + const IPHONE = [302, 656] as const; // 198k px: 396 px of motion budget + const PIXEL5 = [270, 585] as const; // 158k px: 316 px of motion budget + + function changed([w, h]: readonly [number, number], count: number): [PixelFrame, PixelFrame] { + return [ + solid(w, h, [255, 255, 255]), + withChangedPixels(solid(w, h, [255, 255, 255]), count, 0), + ]; + } + + it.each([ + ["iPhone 16 Pro", IPHONE, 66], + ["Pixel 5", PIXEL5, 57], + ] as const)("sees a spinner on a %s frame", (_device, size, spinnerPixels) => { + // Under the motion fraction, so the screen is not called unsettled — but + // never "still", which is what let a still-loading screen report ready + // with nothing said about it. + expect(comparePixels(...changed(size, spinnerPixels))).toBe("localized"); + }); + + it("still calls a real transition motion at that size", () => { + // 1% of the frame — a sheet edge, a scrolling row, a moving cursor bar. + expect(comparePixels(...changed(IPHONE, Math.round(IPHONE[0] * IPHONE[1] * 0.01)))).toBe( + "moving" + ); + }); + + it("keeps a few stray pixels below even the localized floor", () => { + // The floor exists so a backend that is not bit-exact between two + // captures of a static screen does not warn on every settle. Three + // pixels of 198k is an order of magnitude under a caret. + expect(comparePixels(...changed(IPHONE, 3))).toBe("still"); + }); + + // The floor is a pixel COUNT, not a share of the frame: a spinner and a + // caret are the same handful of captured pixels whatever window they sit + // in. As a fraction it only held at phone size — on a desktop-sized + // Chromium window the same 0.005% is ~46 pixels, above every indicator + // ever measured, so the warning was silently off on the largest windows. + it("still sees a caret on a desktop-sized window", () => { + const DESKTOP = [1200, 767] as const; // 920k px, where the old floor was ~46 + expect(comparePixels(...changed(DESKTOP, 10))).toBe("localized"); + expect(comparePixels(...changed(DESKTOP, 45))).toBe("localized"); + // And the floor still holds at that size: noise stays noise. + expect(comparePixels(...changed(DESKTOP, 9))).toBe("still"); + }); + + // The two frames below are the ones the runner was actually caught + // comparing on a static iPhone 16 Pro screen: the run-level status-bar pin + // lands a few hundred milliseconds AFTER the run starts, so frame A holds + // the real clock and frame B the pinned one. Both changes sit inside the + // top band, which is why masking it is what fixes them. + describe("with the status bar masked", () => { + const MASK = 0.06; + + /** Change `count` pixels confined to rows [top, bottom] of an iPhone frame. */ + function changedInRows(count: number, top: number): [PixelFrame, PixelFrame] { + const before = solid(IPHONE[0], IPHONE[1], [255, 255, 255]); + const after = solid(IPHONE[0], IPHONE[1], [255, 255, 255]); + for (let i = 0; i < count; i++) { + const o = (top * IPHONE[0] + i) * 4; + after.data[o] = 0; + after.data[o + 1] = 0; + after.data[o + 2] = 0; + } + return [before, after]; + } + + it("stops the pin's own clock repaint from reading as a moving screen", () => { + // 408 changed pixels at y[19..29] — over the 396-pixel motion budget, + // so unmasked this static screen was judged to be in motion. + const frames = changedInRows(408, 19); + expect(comparePixels(...frames)).toBe("moving"); + expect(comparePixels(...frames, MASK)).toBe("still"); + }); + + it("stops the pin's battery-fill tail from reading as a spinner", () => { + // The same repaint a moment later: 13 pixels at y[19..25], which is + // over the localized floor and became "a spinner, a caret, a progress + // dot ... the screen had not finished loading" on a loaded screen. + const frames = changedInRows(13, 19); + expect(comparePixels(...frames)).toBe("localized"); + expect(comparePixels(...frames, MASK)).toBe("still"); + }); + + it("still sees a spinner just below the masked band", () => { + // The mask must cost the check only the system's own band. 39 rows of + // 656 are masked, so a spinner at row 40 is still fully visible. + expect(comparePixels(...changedInRows(66, 40), MASK)).toBe("localized"); + }); + + it("still sees a transition below the masked band", () => { + const frames = changedInRows(0, 0); + for (let i = 0; i < Math.round(IPHONE[0] * IPHONE[1] * 0.01); i++) { + const o = (39 * IPHONE[0] + i) * 4; + frames[1].data[o] = 0; + frames[1].data[o + 1] = 0; + frames[1].data[o + 2] = 0; + } + expect(comparePixels(...frames, MASK)).toBe("moving"); + }); + + it("takes its fractions against the unmasked area, not the whole frame", () => { + // 1% of the REMAINING rows must still read as motion; measuring against + // the full frame would quietly raise every threshold by the mask. + const visible = IPHONE[0] * (IPHONE[1] - 39); + expect(comparePixels(...changedInRows(Math.round(visible * 0.0025), 39), MASK)).toBe( + "moving" + ); + }); + }); + }); +}); + +describe("statusBarMaskFraction", () => { + // Only iOS and Android paint a status bar into the capture. Masking a + // Chromium window's top band would hide page content, and Vega / tvOS render + // full-screen with no system chrome at all. + it("masks the band on Android", async () => { + await expect( + statusBarMaskFraction({ platform: "android", kind: "emulator", id: "emulator-5554" }) + ).resolves.toBe(0.06); + }); + + it("masks the band on an iOS simulator", async () => { + vi.mocked(isTvOsSimulator).mockResolvedValue(false); + await expect( + statusBarMaskFraction({ platform: "ios", kind: "simulator", id: "ios-udid" }) + ).resolves.toBe(0.06); + }); + + it("masks nothing on a tvOS simulator, which shares the iOS platform tag", async () => { + vi.mocked(isTvOsSimulator).mockResolvedValue(true); + await expect( + statusBarMaskFraction({ platform: "ios", kind: "simulator", id: "tv-udid" }) + ).resolves.toBe(0); + }); + + it.each(["chromium", "vega"] as const)("masks nothing on %s", async (platform) => { + await expect( + statusBarMaskFraction({ platform, kind: "unknown", id: "some-device" }) + ).resolves.toBe(0); + expect(isTvOsSimulator).not.toHaveBeenCalled(); + }); +}); + +/** Write a decodable 2x1 PNG and return its path. */ +async function pngAt(dir: string, name: string): Promise { + const file = path.join(dir, name); + const png = new PNG({ width: 2, height: 1 }); + png.data.set([10, 20, 30, 255, 40, 50, 60, 255]); + await fs.writeFile(file, PNG.sync.write(png)); + return file; +} + +function envFor(device: DeviceInfo, resolveService?: unknown): ActionEnv { + return { device, registry: { resolveService } } as unknown as ActionEnv; +} + +/** The production call path, with a deadline generous enough to stay out of the way. */ +function capture(env: ActionEnv): Promise { + return capturePixelsWithin(env, Date.now() + 30_000, false); +} + +/** What the fake page below paints inside the rendered window. */ +const VISIBLE_BAND_RGB: [number, number, number] = [200, 30, 10]; +/** What Chrome hands back for a clip rectangle outside the rendered window. */ +const OFF_SCREEN_RGB: [number, number, number] = [255, 255, 255]; + +/** The RGB triple of one pixel of a decoded frame. */ +function pixelAt(frame: PixelFrame | undefined, index: number): [number, number, number] { + if (!frame) throw new Error("expected a decoded frame"); + const o = index * 4; + return [frame.data[o], frame.data[o + 1], frame.data[o + 2]]; +} + +interface Clip { + x: number; + y: number; + width: number; + height: number; + scale: number; +} + +/** + * A Chromium page that answers `Page.captureScreenshot` the way the compositor + * does: `clip` is in DOCUMENT coordinates, and with `captureBeyondViewport` + * false only the rendered window has pixels — a rectangle outside it comes back + * blank white. That is what makes a wrong clip origin visible as a colour here + * rather than only as an argument. + */ +function fakeChromiumPage(opts: { metricsError?: Error } = {}): { + api: unknown; + scrollTo(y: number): void; + lastClip(): Clip | undefined; +} { + const viewport = { width: 900, height: 700, devicePixelRatio: 2 }; + let scrollY = 0; + let lastClip: Clip | undefined; + + const send = vi.fn(async (method: string, params?: Record) => { + if (method === "Page.getLayoutMetrics") { + if (opts.metricsError) throw opts.metricsError; + return { + cssVisualViewport: { + pageX: 0, + pageY: scrollY, + clientWidth: viewport.width, + clientHeight: viewport.height, + }, + }; + } + if (method !== "Page.captureScreenshot") throw new Error(`unexpected CDP call ${method}`); + lastClip = params?.clip as Clip; + const insideWindow = + lastClip !== undefined && + lastClip.y >= scrollY && + lastClip.y + lastClip.height <= scrollY + viewport.height; + const [r, g, b] = insideWindow ? VISIBLE_BAND_RGB : OFF_SCREEN_RGB; + const png = new PNG({ width: 2, height: 1 }); + png.data.set([r, g, b, 255, r, g, b, 255]); + return { data: PNG.sync.write(png).toString("base64") }; + }); + + return { + api: { + cdp: { send }, + getViewport: () => viewport, + // The route that WOULD go through sharp. Reaching for it is the bug. + captureScreenshot: vi.fn(() => { + throw new Error("the sharp-backed capture must not be used for a settle"); + }), + }, + scrollTo(y: number) { + scrollY = y; + }, + lastClip: () => lastClip, + }; +} + +describe("capturePixels routing", () => { + // Every platform argent can screenshot has a route here, and each one is a + // different backend — sending a device down the wrong one silently costs the + // settle its visual half, which then degrades to a tree-only pass. + it("captures and cleans up decodable pixels through the simulator-server backend", async () => { + const file = await pngAt(tmpDir, "native.png"); + const screenshot = vi.fn(async () => ({ path: file, url: `file://${file}` })); + const device: DeviceInfo = { platform: "ios", kind: "simulator", id: "ios-device" }; + const resolveService = vi.fn(async () => ({ transport: { screenshot } })); + + const pixels = await capture(envFor(device, resolveService)); + + expect(pixels).toMatchObject({ width: 2, height: 1 }); + expect([...pixels!.data]).toEqual([10, 20, 30, 255, 40, 50, 60, 255]); + expect(resolveService).toHaveBeenCalledWith(`SimulatorServer:${device.id}`, { device }); + expect(screenshot).toHaveBeenCalledWith({ + rotation: undefined, + scale: 0.25, + signal: undefined, + }); + // The temp PNG is scratch, never an artifact — it must not outlive the decode. + await expect(fs.access(file)).rejects.toThrow(); + }); + + it("routes a tvOS simulator to xcrun, not to the simulator-server it has no backend for", async () => { + vi.mocked(isTvOsSimulator).mockResolvedValue(true); + vi.mocked(tvScreenshot).mockImplementation(async () => pngAt(tmpDir, "tv.png")); + const resolveService = vi.fn(() => { + throw new Error("simulator-server must not be resolved for tvOS"); + }); + // A tvOS simulator's platform is "ios" — only the runtime tells them apart. + const device: DeviceInfo = { platform: "ios", kind: "simulator", id: "tv-udid" }; + + await expect(capture(envFor(device, resolveService))).resolves.toMatchObject({ + width: 2, + height: 1, + }); + expect(tvScreenshot).toHaveBeenCalledWith("tv-udid", 0.25, expect.any(AbortSignal)); + expect(resolveService).not.toHaveBeenCalled(); + }); + + // `tvScreenshot` forwards its signal to `execFileAsync`. Without one, a + // wedged `xcrun simctl io screenshot` is never killed and the next poll + // 200ms later spawns another, so one stuck subprocess becomes a pile of + // them — the round abandons the promise, but nothing abandons the process. + it("kills a wedged tvOS capture when its budget runs out", async () => { + vi.mocked(isTvOsSimulator).mockResolvedValue(true); + let signal: AbortSignal | undefined; + vi.mocked(tvScreenshot).mockImplementation( + (_udid, _scale, sig) => + new Promise((_resolve, reject) => { + signal = sig; + sig?.addEventListener("abort", () => reject(new Error("aborted"))); + }) + ); + const env = envFor({ platform: "ios", kind: "simulator", id: "tv-udid" }); + + expect(await capturePixelsWithin(env, Date.now() + 50, false)).toBeUndefined(); + // The budget bounds the round and the subprocess with the same deadline, + // so which of the two timers lands first is not fixed — only that the + // capture does not outlive the round it belonged to. + await vi.waitFor(() => expect(signal?.aborted).toBe(true)); + }); + + it("kills a tvOS capture when the run itself is cancelled", async () => { + vi.mocked(isTvOsSimulator).mockResolvedValue(true); + const controller = new AbortController(); + let signal: AbortSignal | undefined; + vi.mocked(tvScreenshot).mockImplementation( + (_udid, _scale, sig) => + new Promise((_resolve, reject) => { + signal = sig; + sig?.addEventListener("abort", () => reject(new Error("aborted"))); + }) + ); + const env = { ...envFor({ platform: "ios", kind: "simulator", id: "tv-udid" }) } as ActionEnv; + (env as { signal?: AbortSignal }).signal = controller.signal; + + const pending = capturePixelsWithin(env, Date.now() + 30_000, false); + await vi.waitFor(() => expect(signal).toBeDefined()); + controller.abort(); + + expect(await pending).toBeUndefined(); + expect(signal?.aborted).toBe(true); + }); + + it("routes Vega to the emulator console, and never probes the iOS runtime for it", async () => { + vi.mocked(captureVegaScreenshotPng).mockImplementation(async () => pngAt(tmpDir, "vega.png")); + const resolveService = vi.fn(() => { + throw new Error("simulator-server must not be resolved for vega"); + }); + + await expect( + capture(envFor({ platform: "vega", kind: "vvd", id: "vega-serial" }, resolveService)) + ).resolves.toMatchObject({ width: 2, height: 1 }); + expect(captureVegaScreenshotPng).toHaveBeenCalledWith({ scale: 0.25 }); + expect(isTvOsSimulator).not.toHaveBeenCalled(); + expect(resolveService).not.toHaveBeenCalled(); + }); + + // Chromium is the one route that never touches the filesystem, and the one + // that cannot use the `screenshot` tool's scaling: that resizes with `sharp`, + // an optional dependency nothing here installs, so asking it for a quarter + // scale returned a full-resolution PNG and a settle decoded a 23MB buffer + // twice a second. The compositor applies `clip.scale` while rasterizing, so + // the small frame is the only one that exists. + it("captures Chromium through the compositor's own scale, and never via a file", async () => { + const device: DeviceInfo = { platform: "chromium", kind: "app", id: "chromium-cdp-9222" }; + const page = fakeChromiumPage(); + const resolveService = vi.fn(async () => page.api); + + const pixels = await capture(envFor(device, resolveService)); + + expect(pixels).toMatchObject({ width: 2, height: 1 }); + expect(resolveService).toHaveBeenCalledWith(`ChromiumCdp:${device.id}`, { device }); + // The clip is the viewport in CSS pixels; its scale composes with the + // page's device scale factor, so the plain capture scale lands on a quarter + // of the frame this route used to return. + expect(page.lastClip()).toMatchObject({ width: 900, height: 700, scale: 0.25 }); + }); + + // `clip` is measured from the top of the DOCUMENT. Pinning its origin at + // (0, 0) therefore aimed the capture at the top of the page rather than at + // the window, and on a scrolled document Chrome rasterizes that off-screen + // rectangle as blank white. Two blank captures compare as identical, so the + // pixel half of the settle voted "still" on every interval of a visibly + // animating screen — and voted it silently, because the capture succeeded. + // + // The mock below is the compositor's actual behaviour rather than an + // argument matcher: it serves whatever the clip rectangle overlaps in the + // rendered window, and white for anything outside it. A capture aimed at the + // wrong origin therefore comes back the wrong COLOR, which is the thing the + // comparison acts on. + it("captures the scrolled window, not the top of the document", async () => { + const device: DeviceInfo = { platform: "chromium", kind: "app", id: "chromium-cdp-9222" }; + const page = fakeChromiumPage(); + page.scrollTo(1839); + + const pixels = await capture( + envFor( + device, + vi.fn(async () => page.api) + ) + ); + + expect(page.lastClip()).toMatchObject({ x: 0, y: 1839 }); + // The visible band, not the blank rectangle above the fold. + expect(pixelAt(pixels, 0)).toEqual(VISIBLE_BAND_RGB); + expect(pixelAt(pixels, 0)).not.toEqual(OFF_SCREEN_RGB); + }); + + it("still clips at the document origin for an unscrolled page", async () => { + const device: DeviceInfo = { platform: "chromium", kind: "app", id: "chromium-cdp-9222" }; + const page = fakeChromiumPage(); + + const pixels = await capture( + envFor( + device, + vi.fn(async () => page.api) + ) + ); + + expect(page.lastClip()).toMatchObject({ x: 0, y: 0 }); + expect(pixelAt(pixels, 0)).toEqual(VISIBLE_BAND_RGB); + }); + + it("reads a Chromium capture that came back with no data as no evidence", async () => { + // The compositor answering without `data` is a capture failure like any + // other: soft, so the settle records "no visual evidence this round" + // rather than failing the step on it. + const device: DeviceInfo = { platform: "chromium", kind: "app", id: "chromium-cdp-9222" }; + const send = vi.fn(async (method: string) => + method === "Page.getLayoutMetrics" ? { cssVisualViewport: { pageX: 0, pageY: 0 } } : {} + ); + const api = { cdp: { send }, getViewport: () => ({ width: 900, height: 700 }) }; + + expect( + await capture( + envFor( + device, + vi.fn(async () => api) + ) + ) + ).toBeUndefined(); + }); + + it("falls back to the document origin when the layout metrics cannot be read", async () => { + // A renderer that will not answer the metrics read leaves the capture no + // worse off than never asking — an unscrolled clip, not a failed settle. + const device: DeviceInfo = { platform: "chromium", kind: "app", id: "chromium-cdp-9222" }; + const page = fakeChromiumPage({ metricsError: new Error("renderer is navigating") }); + page.scrollTo(1839); + + const pixels = await capture( + envFor( + device, + vi.fn(async () => page.api) + ) + ); + + expect(page.lastClip()).toMatchObject({ x: 0, y: 0 }); + expect(pixels).toMatchObject({ width: 2, height: 1 }); + }); + + it("leaves Android on the simulator-server route without an iOS runtime probe", async () => { + const file = await pngAt(tmpDir, "android.png"); + const resolveService = vi.fn(async () => ({ + transport: { screenshot: async () => ({ path: file, url: `file://${file}` }) }, + })); + + await expect( + capture( + envFor({ platform: "android", kind: "emulator", id: "emulator-5554" }, resolveService) + ) + ).resolves.toMatchObject({ width: 2, height: 1 }); + expect(isTvOsSimulator).not.toHaveBeenCalled(); + }); + + it.each(["ios", "android", "chromium", "vega"] as const)( + "returns undefined (never throws) on %s when the capture backend fails", + async (platform) => { + // Soft by design: the caller reads undefined as "no visual evidence", so + // a throw escaping here would fail the step on an environment problem. + vi.mocked(captureVegaScreenshotPng).mockRejectedValue(new Error("no vvd")); + const env = envFor({ platform, kind: "unknown", id: "some-device" }); // no resolveService + + expect(await capture(env)).toBeUndefined(); + } + ); + + it("returns undefined when the capture succeeds but the file is not a decodable PNG", async () => { + const file = path.join(tmpDir, "garbage.png"); + await fs.writeFile(file, "not a png"); + const resolveService = vi.fn(async () => ({ + transport: { screenshot: async () => ({ path: file, url: `file://${file}` }) }, + })); + + expect( + await capture(envFor({ platform: "ios", kind: "simulator", id: "x" }, resolveService)) + ).toBeUndefined(); + // Still cleaned up — a failed decode must not leak the file either. + await expect(fs.access(file)).rejects.toThrow(); + }); +}); + +describe("capturePixelsWithin", () => { + const iosDevice: DeviceInfo = { platform: "ios", kind: "simulator", id: "ios-udid" }; + + function envWith(screenshot: () => Promise<{ path: string; url: string }>): ActionEnv { + return envFor( + iosDevice, + vi.fn(async () => ({ transport: { screenshot } })) + ); + } + + it("returns the frame when the capture lands inside the deadline", async () => { + const env = envWith(async () => { + const file = await pngAt(tmpDir, "in-time.png"); + return { path: file, url: `file://${file}` }; + }); + await expect(capturePixelsWithin(env, Date.now() + 5_000, false)).resolves.toMatchObject({ + width: 1 + 1, + height: 1, + }); + }); + + it("gives up rather than overrunning the caller's deadline", async () => { + // A capture that never returns must not hold the settle past the step's + // own timeout — the caller degrades to tree-only, it does not wait. + const env = envWith(() => new Promise(() => {})); + const started = Date.now(); + await expect(capturePixelsWithin(env, started + 120, false)).resolves.toBeUndefined(); + expect(Date.now() - started).toBeLessThan(2_000); + }); + + it("does not attempt a capture once the deadline has already passed", async () => { + const screenshot = vi.fn(async () => { + const file = await pngAt(tmpDir, "too-late.png"); + return { path: file, url: `file://${file}` }; + }); + await expect(capturePixelsWithin(envWith(screenshot), Date.now() - 1, false)).resolves.toBe( + undefined + ); + expect(screenshot).not.toHaveBeenCalled(); + }); + + it("allows the first capture the cold-stream wait and later ones the warm bound", () => { + // The simulator-server serves captures from a live frame stream, so the + // first read after it starts can spend the whole first-frame window; every + // later one is answered from a stream that is already producing. + expect(pixelCaptureTimeoutMs(iosDevice, true)).toBe(FIRST_PIXEL_CAPTURE_TIMEOUT_MS); + expect(FIRST_PIXEL_CAPTURE_TIMEOUT_MS).toBeGreaterThan(FIRST_FRAME_WAIT_MS); + expect(pixelCaptureTimeoutMs(iosDevice, false)).toBe(PIXEL_CAPTURE_TIMEOUT_MS); + // Chromium answers from CDP with no stream to warm up, so its first + // capture gets no extra grace. + expect( + pixelCaptureTimeoutMs({ platform: "chromium", kind: "app", id: "chromium-cdp-9222" }, true) + ).toBe(PIXEL_CAPTURE_TIMEOUT_MS); + // Nor does Vega, which shells out to the emulator console — no stream + // either. Untested, this arm could be deleted and only Chromium would tell. + expect(pixelCaptureTimeoutMs({ platform: "vega", kind: "vvd", id: "vega-serial" }, true)).toBe( + PIXEL_CAPTURE_TIMEOUT_MS + ); + // A tvOS simulator shells out too, but nothing here can tell it from an + // iOS one without an async probe, so it keeps the wider bound it will not + // spend. + expect(pixelCaptureTimeoutMs({ platform: "ios", kind: "simulator", id: "tv-udid" }, true)).toBe( + FIRST_PIXEL_CAPTURE_TIMEOUT_MS + ); + }); +}); diff --git a/packages/tool-server/test/flows/flow-skill-docs.test.ts b/packages/tool-server/test/flows/flow-skill-docs.test.ts index 880570dcc..41eaeaaf0 100644 --- a/packages/tool-server/test/flows/flow-skill-docs.test.ts +++ b/packages/tool-server/test/flows/flow-skill-docs.test.ts @@ -1,7 +1,16 @@ import { describe, it, expect } from "vitest"; import { readFileSync } from "node:fs"; import * as path from "node:path"; -import { parseFlow } from "../../src/tools/flows/flow-utils"; +import type { Registry } from "@argent/registry"; +import { + IDLE_DEFAULT_STABLE_FOR_MS, + IDLE_DEFAULT_TIMEOUT_MS, + IDLE_MIN_STILL_INTERVALS, + IDLE_POLL_MS, + IDLE_SETTLE_OVERHEAD_MS, + parseFlow, +} from "../../src/tools/flows/flow-utils"; +import { createRunFlowTool } from "../../src/tools/flows/flow-run"; /** * The create-flow skill is the agent-facing reference for selector scopes, so @@ -42,6 +51,57 @@ describe("create-flow SKILL.md scope snippets", () => { } }); + // The two agent-facing descriptions of `idle` have to agree with what it + // does. The commit that turned its timeout from a failure into a warning + // updated the skill, the comments and the tests, and left the tool + // description — the surface an authoring agent actually reads — saying the + // opposite, with "so it is safe to persist" hung off the claim that was now + // backwards. + it("the flow-execute description and the skill agree that idle warns rather than fails", () => { + const description = createRunFlowTool({} as unknown as Registry).description; + expect(description).toContain("idle: true"); + expect(description).toMatch(/never\s+fails a run/); + expect(description).not.toMatch(/FAILS on timeout/i); + + const skill = readFileSync(SKILL, "utf8"); + expect(skill).toContain("It **never fails a run.**"); + // The one outcome that does stop a run is the window, never the app. + expect(skill).toMatch(/Only a tree source that cannot be read stops the run/); + // Both surfaces have to carry that caveat: the description is what an + // authoring agent reads, and "never fails a run" on its own is not true + // of a tree nobody could read. + expect(description).toMatch(/unreadable|cannot be read|could not be read/); + }); + + // The claims above are prose until something ties them to the runner. These + // pin the numbers the skill quotes to the constants the parser enforces, so + // a default that moves takes the sentence describing it with it. + it("the skill's idle defaults and settle cost are the ones the parser enforces", () => { + const skill = readFileSync(SKILL, "utf8"); + expect(skill).toContain(`default ${IDLE_DEFAULT_STABLE_FOR_MS}`); + expect(skill).toContain(`default ${IDLE_DEFAULT_TIMEOUT_MS}`); + expect(skill).toContain(`${IDLE_SETTLE_OVERHEAD_MS}ms a settle costs`); + expect(skill).toContain(`${IDLE_POLL_MS}ms polls`); + // The gloss has to add up to the cost it explains: the polls the intervals + // span, plus the round-start floor. Without the second term it described + // 400ms while demanding 600. + expect(IDLE_SETTLE_OVERHEAD_MS).toBe((IDLE_MIN_STILL_INTERVALS + 1) * IDLE_POLL_MS); + expect(skill).toContain(`plus the ${IDLE_POLL_MS}ms of budget the closing round`); + }); + + it("the smallest timeout the skill's arithmetic allows is the one the parser accepts", () => { + // The skill tells an author the wait has to contain the hold plus the + // settle. Take it at its word and check the boundary both ways — a parser + // that demanded a millisecond more would make the documented sum a lie. + const smallest = IDLE_DEFAULT_STABLE_FOR_MS + IDLE_SETTLE_OVERHEAD_MS; + expect(() => + parseFlow(`steps:\n - await: { idle: true, timeout: ${smallest} }\n`) + ).not.toThrow(); + expect(() => + parseFlow(`steps:\n - await: { idle: true, timeout: ${smallest - 1} }\n`) + ).toThrow(new RegExp(`at least ${smallest}ms`)); + }); + it("the paragraph's rejected `any` spelling really is rejected", () => { // The docs tell agents to write `{ role: Switch, next: … }` and NOT // `{ any: true, role: Switch, next: … }`. If the parser ever started diff --git a/packages/tool-server/test/flows/flow-tools.test.ts b/packages/tool-server/test/flows/flow-tools.test.ts index afdc03d46..0d7b4fc33 100644 --- a/packages/tool-server/test/flows/flow-tools.test.ts +++ b/packages/tool-server/test/flows/flow-tools.test.ts @@ -1549,6 +1549,28 @@ describe("flow-finish-recording", () => { expect(result.summary).toEqual(["1. echo: Before tap", '2. tool: tap {"x":0.5}']); }); + // `idle` has no recorder command — it is written by hand into the YAML, + // which the finish re-reads. Without a case here it fell through to the + // `tool:` default and the summary described the step as a tool call. + it("summarizes a hand-written idle step as the wait it is", async () => { + await flowStartRecordingTool.execute( + {}, + { name: "idle-summary", project_root: tmpDir, executionPrerequisite: PREREQ } + ); + await fs.writeFile( + path.join(tmpDir, ".argent", "flows", "idle-summary.yaml"), + `executionPrerequisite: ${JSON.stringify(PREREQ)}\nsteps:\n - await: { idle: true }\n`, + "utf8" + ); + + const result = await flowFinishRecordingTool.execute( + {}, + { name: "idle-summary", project_root: tmpDir } + ); + + expect(result.summary).toEqual(["1. await: screen idle"]); + }); + it("distinguishes contains, equals, and regex text comparisons in the summary", async () => { const name = "text-comparison-summary"; await flowStartRecordingTool.execute(