From bce41535f440037fbb0a9c8900a1e24833867d8c Mon Sep 17 00:00:00 2001 From: Filip131311 Date: Sat, 1 Aug 2026 06:53:50 +0200 Subject: [PATCH] fix(flow): let a flow launch an app that can never be instrumented MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flow's launch step waits for native devtools on iOS, and an Apple system app is a platform binary with library validation, so that connection can never happen. The step burned the full 8s timeout, failed, and took the rest of the flow with it — even when nothing in the flow needed the view hierarchy. The advice it gave ("re-run", "restart the argent server") could never work, which is the retry loop #453 set out to remove. The signal was already there: isInjectableBundleId, used by native-devtools-status and by describe, whose own docs call injectable:false a terminal state. The gate never consulted it. It does now, and the launch passes with a warning saying what does and does not work for such an app. Removing the wait exposes something the wait was accidentally hiding, so the same change has to close it. Selector steps reach resolveNativeTargetApp, which auto-targets whatever app is connected and accepts one reporting "inactive" as foreground-like. An app the user was driving reports exactly that while it backgrounds. Today the first tree read lands ~9.5s after launch, past that window; without the wait it lands at ~1.5s, inside it — so a selector could resolve against a different app's tree, and a hidden assertion against it would pass. The run now records that the launched app cannot be instrumented and fails tree reads immediately with a terminal reason. The guard sits on settleTree and waitForCondition rather than on step kinds, so it covers tap, long-press, await, assert, scroll-to and snapshot at once, and leaves selector-less pinch and rotate working — those degrade to a default aspect and never read a tree. Reporting a pass rather than a failure is deliberate: the launch did succeed, and the control case in the issue proves the rest of such a flow runs fine. The warning field was already rendered by both the CLI and MCP as legacy wire-compat; it is now produced, so those comments no longer describe it as something only an old server sends. The recording walkthrough in the create-flow skill used com.apple.Preferences with selector steps, which could never have been captured or replayed. It now uses a third-party app. --- .../skills/skills/argent-create-flow/SKILL.md | 32 ++++----- .../src/tools/flows/flow-actions.ts | 33 +++++++++ .../tool-server/src/tools/flows/flow-run.ts | 37 +++++++++- .../test/flows/flow-composition.test.ts | 72 +++++++++++++++++++ 4 files changed, 156 insertions(+), 18 deletions(-) diff --git a/packages/skills/skills/argent-create-flow/SKILL.md b/packages/skills/skills/argent-create-flow/SKILL.md index 664861b26..21f4de8a4 100644 --- a/packages/skills/skills/argent-create-flow/SKILL.md +++ b/packages/skills/skills/argent-create-flow/SKILL.md @@ -161,20 +161,20 @@ Record an `await-ui-element` step to **gate** the next step on a screen transiti - `tool: gesture-pinch` → `pinch: { on: "", scale: … }`, deriving `scale` as `endDistance / startDistance`. Set `on:` to the element under the pinch center when the pinch was aimed at one (the map or image being zoomed); omit it for a screen-center pinch. Don't carry the recorded distances/angle over — the directive re-derives the geometry (finger placement, system-edge avoidance, chaining of large scales) at run time, so the conversion swaps device-specific coordinates for a portable selector with auto-wait. Keep the raw `tool: gesture-pinch` step when the pinch is anchored at a specific point _inside_ a large element (zooming toward a particular map location, not the map's center) or deliberately pans via `endCenterX`/`endCenterY` — `on:` takes only a selector and re-centers the pinch on the element's frame center, so converting would silently move the zoom anchor. - `tool: gesture-rotate` → `rotate: { on: "", by: … }`, deriving `by` as `endAngle − startAngle` (the tool's `endAngle` > `startAngle` turns clockwise, matching the directive's positive `by`). Set `on:` to the element under the rotation center when the rotation was aimed at one (the map or image being rotated); omit it for a screen-center rotation. Don't carry the recorded `centerX`/`centerY`, radii (`radius` or `radiusX`/`radiusY`), `startAngle`, or `durationMs` over — the directive re-derives the geometry (finger placement, physical-circle radius, system-edge avoidance) and runs at a fixed pace (~90° per 300 ms), so the conversion swaps device-specific coordinates for a portable selector with auto-wait. Keep the raw `tool: gesture-rotate` step when the rotation is anchored at a specific point _inside_ a large element rather than its center (the directive re-centers on the element's frame center, so converting would silently move the pivot), when the gesture's speed itself matters (the directive's pace is fixed), or when the sweep exceeds the directive's ±3000° bound. -Every other recorded tool (a velocity-dependent `gesture-swipe`, a fixed-distance `gesture-scroll` not aimed at an element, `button`, `screenshot`, …) has no directive form — leave it as a `tool:` step. The recorder already handles the rest: coordinate `gesture-tap`s are captured as portable `tap:` selector steps, a `restart-app` is captured as a `launch:` step, a `flow-execute` of a sibling fragment is captured as a `run: ` composition directive, and device ids are stripped. Captured selectors are emitted in the strict map form (`tap: { text: General }`), never as a loose bare string — the recorder verified the exact element the tap hit, and a bare string would re-parse as loose and route through the identifier-first fallback it was never checked against. After editing, re-run with `flow-execute` to confirm the cleaned flow still passes. +Every other recorded tool (a velocity-dependent `gesture-swipe`, a fixed-distance `gesture-scroll` not aimed at an element, `button`, `screenshot`, …) has no directive form — leave it as a `tool:` step. The recorder already handles the rest: coordinate `gesture-tap`s are captured as portable `tap:` selector steps, a `restart-app` is captured as a `launch:` step, a `flow-execute` of a sibling fragment is captured as a `run: ` composition directive, and device ids are stripped. Captured selectors are emitted in the strict map form (`tap: { text: Settings }`), never as a loose bare string — the recorder verified the exact element the tap hit, and a bare string would re-parse as loose and route through the identifier-first fallback it was never checked against. After editing, re-run with `flow-execute` to confirm the cleaned flow still passes. ### Example session ``` flow-start-recording { name: "open-about", project_root: "/Users/dev/MyApp" } -flow-add-echo { message: "Start Settings from scratch" } -flow-add-step { command: "restart-app", args: "{\"udid\": \"ABC\", \"bundleId\": \"com.apple.Preferences\"}" } # ⇒ captured as `- launch: com.apple.Preferences` — this is now an e2e flow -flow-add-echo { message: "On the Settings root list, tapping the 'General' row" } -flow-add-step { command: "gesture-tap", args: "{\"udid\": \"ABC\", \"x\": 0.5, \"y\": 0.35}" } # ⇒ captured as `- tap: { text: General }` (portable selector, no udid) -flow-add-step { command: "await-ui-element", args: "{\"udid\": \"ABC\", \"condition\": \"visible\", \"selector\": {\"text\": \"About\"}}" } # gate the transition -flow-add-echo { message: "On Settings > General, tapping 'About'" } +flow-add-echo { message: "Start the app from scratch" } +flow-add-step { command: "restart-app", args: "{\"udid\": \"ABC\", \"bundleId\": \"com.example.myapp\"}" } # ⇒ captured as `- launch: com.example.myapp` — this is now an e2e flow +flow-add-echo { message: "On the app's root list, tapping the 'Settings' row" } +flow-add-step { command: "gesture-tap", args: "{\"udid\": \"ABC\", \"x\": 0.5, \"y\": 0.35}" } # ⇒ captured as `- tap: { text: Settings }` (portable selector, no udid) +flow-add-step { command: "await-ui-element", args: "{\"udid\": \"ABC\", \"condition\": \"visible\", \"selector\": {\"text\": \"Account\"}}" } # gate the transition +flow-add-echo { message: "On Settings, tapping 'Account'" } flow-add-step { command: "gesture-tap", args: "{\"udid\": \"ABC\", \"x\": 0.5, \"y\": 0.17}" } -flow-add-step { command: "await-ui-element", args: "{\"udid\": \"ABC\", \"condition\": \"visible\", \"selector\": {\"text\": \"Model Name\"}}" } +flow-add-step { command: "await-ui-element", args: "{\"udid\": \"ABC\", \"condition\": \"visible\", \"selector\": {\"text\": \"Email\"}}" } flow-finish-recording {} ``` @@ -198,14 +198,14 @@ The polished result of the example session above: ```yaml steps: - - echo: Start Settings from scratch - - launch: com.apple.Preferences - - echo: On the Settings root list, tapping the 'General' row - - tap: { text: General } - - await: { visible: About } - - echo: On Settings > General, tapping 'About' - - tap: { text: About } - - await: { visible: Model Name } + - echo: Start the app from scratch + - launch: com.example.myapp + - echo: On the app's root list, tapping the 'Settings' row + - tap: { text: Settings } + - await: { visible: Account } + - echo: On Settings, tapping 'Account' + - tap: { text: Account } + - await: { visible: Email } ``` Note there is **no device id** anywhere in the file — the recorder strips them and the runner injects the bound device. diff --git a/packages/tool-server/src/tools/flows/flow-actions.ts b/packages/tool-server/src/tools/flows/flow-actions.ts index f45054b17..1155ed2d5 100644 --- a/packages/tool-server/src/tools/flows/flow-actions.ts +++ b/packages/tool-server/src/tools/flows/flow-actions.ts @@ -52,12 +52,22 @@ export interface ActionEnv { ctx?: ToolContext; device: DeviceInfo; signal?: AbortSignal; + /** + * Bundle id of an app the run launched that can never carry the + * view-hierarchy instrumentation. Set means every tree read is doomed, so + * they fail immediately with a terminal reason instead of polling a source + * that will never appear — and, more importantly, instead of auto-targeting + * whichever other app happens to still be connected. + */ + nonInjectableApp?: string; } /** Outcome of a selector directive: ok, or a machine-readable reason it failed. */ export interface DirectiveOutcome { ok: boolean; reason?: string; + /** Caveat carried by a step that still succeeded. */ + warning?: string; /** The run was cancelled mid-step — reported as a skip, not a step failure. */ aborted?: boolean; /** @@ -314,7 +324,24 @@ function flowSelectorToFrame(tree: DescribeNode, sel: FlowSelector): DescribeFra * convert the outage into a misleading "element not found" downstream. The * throw lands in the step's structured report via `execLeafStep`'s catch. */ +/** + * Why no selector can resolve against an app that cannot be instrumented. + * Deliberately not the native-devtools recovery text, which tells the caller to + * use `describe`/`screenshot` — the answer for a flow is coordinate steps. + */ +export function nonInjectableTreeReason(bundleId: string): string { + return ( + `\`${bundleId}\` is an Apple system app, so argent's view-hierarchy instrumentation can never ` + + `be injected into it and selector-based steps cannot resolve. This is terminal — relaunching or ` + + `restarting the argent server will not change it. Target this screen by coordinate ` + + `(\`tap: { x, y }\`) instead.` + ); +} + export async function settleTree(env: ActionEnv): Promise { + if (env.nonInjectableApp) { + throw new Error(nonInjectableTreeReason(env.nonInjectableApp)); + } const deadline = Date.now() + SETTLE_TIMEOUT_MS; let prevFp: string | undefined; let prevTree: DescribeNode | undefined; @@ -992,6 +1019,12 @@ async function waitForCondition( }, timeoutMs: number ): Promise { + // Not routed through settleTree, so it needs its own guard — and a `hidden` + // wait is the one condition that would otherwise resolve TRUE off an + // unreadable screen. + if (env.nonInjectableApp) { + return { ok: false, reason: nonInjectableTreeReason(env.nonInjectableApp) }; + } const deadline = Date.now() + timeoutMs; let lastMatches: ReturnType = []; diff --git a/packages/tool-server/src/tools/flows/flow-run.ts b/packages/tool-server/src/tools/flows/flow-run.ts index 645f5c636..2e121ec92 100644 --- a/packages/tool-server/src/tools/flows/flow-run.ts +++ b/packages/tool-server/src/tools/flows/flow-run.ts @@ -41,7 +41,11 @@ import { type ActionEnv, type DirectiveOutcome, } from "./flow-actions"; -import { nativeDevtoolsRef, type NativeDevtoolsApi } from "../../blueprints/native-devtools"; +import { + nativeDevtoolsRef, + isInjectableBundleId, + type NativeDevtoolsApi, +} from "../../blueprints/native-devtools"; import { androidDevtoolsRef, type AndroidDevtoolsApi } from "../../blueprints/android-devtools"; import { chromiumCdpRef, @@ -111,6 +115,12 @@ export interface StepReport { * percentage, baseline written/updated). */ reason?: string; + /** + * A caveat about a step that still passed — currently, launching an app that + * can never carry the view-hierarchy instrumentation. Renderers show it as a + * `⚠` in place of the pass glyph and count it in the summary. + */ + warning?: string; /** Underlying tool id for `tool` steps. */ tool?: string; /** Tool result for `tool` steps. */ @@ -282,6 +292,10 @@ async function treeSourceGate( signal?: AbortSignal ): Promise { if (device.platform === "ios" && !signal?.aborted) { + // An app that can never be injected will never connect, so waiting the full + // timeout only delays advice that cannot work. The flow can still run every + // step that does not read the view hierarchy. + if (!isInjectableBundleId(bundleId)) return null; const connected = await waitForNativeDevtools(registry, device, bundleId, signal); if (!connected && !signal?.aborted) { return ( @@ -396,11 +410,25 @@ async function runLaunch(state: ExecState, app: Launch): Promise { // …while a non-map, non-string body still gets the shape error. expect(() => parseFlow("steps:\n - launch: 42\n")).toThrow(/launch needs an app id/i); }); + it("launches a non-injectable system app and keeps running coordinate steps", async () => { + // An Apple system app is a platform binary with library validation, so the + // instrumentation can never load. Waiting for it only delayed advice that + // could never work, and took the rest of the flow down with it — even when + // nothing in the flow needed the view hierarchy. + await writeFlow("main", { + executionPrerequisite: "", + steps: [ + { kind: "launch", app: "com.apple.Preferences" }, + { kind: "echo", message: "coordinate work still runs" }, + ], + }); + const registry = { + invokeTool: vi.fn(async (id: string) => + id === "list-devices" ? { devices: [] } : { ok: true } + ), + getTool: vi.fn(() => undefined), + // Would throw if the gate consulted it — it must not for this bundle. + resolveService: vi.fn(async () => { + throw new Error("native-devtools unavailable"); + }), + } as unknown as Registry; + + const result = asRun( + await createRunFlowTool(registry).execute( + {}, + { name: "main", project_root: tmpDir, device: DEVICE } + ) + ); + + expect(result.steps.map((s) => `${s.kind}:${s.status}`)).toEqual(["launch:pass", "echo:pass"]); + expect(result.steps[0].warning).toMatch(/system app/i); + // The advice that can never work must be gone. + expect(result.steps[0].warning).not.toMatch(/restart the argent server/i); + expect(result.ok).toBe(true); + }); + + it("fails a selector step against a non-injectable app terminally, not by timing out", async () => { + // Without this the step reaches auto-targeting and reports "Launch or + // restart the app first" — worse advice than the gate gave — and, if any + // other app is still connected, could resolve against ITS tree instead. + await writeFlow("main", { + executionPrerequisite: "", + steps: [ + { kind: "launch", app: "com.apple.Preferences" }, + { kind: "await", condition: "visible", selector: { text: "General" } }, + ], + }); + const registry = { + invokeTool: vi.fn(async (id: string) => + id === "list-devices" ? { devices: [] } : { ok: true } + ), + getTool: vi.fn(() => undefined), + resolveService: vi.fn(async () => { + throw new Error("native-devtools unavailable"); + }), + } as unknown as Registry; + + const started = Date.now(); + const result = asRun( + await createRunFlowTool(registry).execute( + {}, + { name: "main", project_root: tmpDir, device: DEVICE } + ) + ); + + expect(result.steps.map((s) => `${s.kind}:${s.status}`)).toEqual(["launch:pass", "await:fail"]); + expect(result.steps[1].reason).toMatch(/can never be injected/i); + expect(result.steps[1].reason).toMatch(/terminal/i); + // Immediate, not after the selector timeout. + expect(Date.now() - started).toBeLessThan(8000); + }); });