diff --git a/packages/tool-server/src/tools/flows/flow-add-step.ts b/packages/tool-server/src/tools/flows/flow-add-step.ts index 6f2828c24..85dada621 100644 --- a/packages/tool-server/src/tools/flows/flow-add-step.ts +++ b/packages/tool-server/src/tools/flows/flow-add-step.ts @@ -31,12 +31,13 @@ import { resolveDevice } from "../../utils/device-info"; import { settleWithin } from "../../utils/timing"; import { stripDeviceKeys } from "./flow-device"; import { fetchFlowTree } from "./flow-tree"; -import type { DescribeSource } from "../describe/contract"; +import type { DescribeFrame, DescribeNode, DescribeSource } from "../describe/contract"; import { nodeAtPoint, deriveSelector, selectorToFrame, frameContains, + GENERIC_ROLES, type Selector, type TextMatchMode, type WaitCondition, @@ -113,6 +114,23 @@ const UNSUPPORTED_PLATFORM = { read: "No read-only tool is known to report the runner's projection on this platform — keep the step raw", } as const; +/** + * The read-only tool that reads the tree the RUNNER resolves against, for the + * platforms where one exists. Android is deliberately routed elsewhere (see + * {@link runnerSideReadClause}): no read-only tool exposes its runner tree, so + * this helper is only ever called here for iOS / Chromium / Vega. + * + * `native-find-views` declares Apple capability only, so it is named for iOS + * alone; iOS `describe` is the AX tree — the RECORDER's side — so it is NOT + * listed here, where the point is to name the runner's reader. + */ +function treeReaderFor(udid: unknown): string { + const platform = platformOf(udid); + if (platform === "ios" || platform === "ios-remote") return "`native-find-views`"; + if (platform === "chromium") return "`describe` (this platform's DOM walker)"; + return "`describe`"; +} + /** * The clause naming how to read the tree the RUNNER resolves against — or, on * iOS, Android and Chromium, that no read-only tool does. @@ -589,6 +607,92 @@ function roleOnlySelectorWarning(selector: Selector): string | undefined { ); } +/** + * A tap target has to be small enough that tapping its CENTRE reproduces the + * tap. Frames are normalized to the viewport, so this is a share of the screen. + * + * The number is a judgement, and the two failures it sits between are both + * real and both were observed. Too permissive and a container gets recorded: + * a tap on blank space in a drawer resolved to the drawer's whole scroll area + * (0.72 of the screen), and replay — which taps a selector's centre — hit the + * "Chat" item and reported pass while navigating somewhere the walkthrough + * never went. Too strict and ordinary widgets become unrecordable: a feed post + * is half the screen and tapping it is a perfectly normal QA step. + */ +const MAX_TAP_TARGET_AREA = 0.6; + +function isContainerSized(frame: DescribeFrame): boolean { + return frame.width * frame.height > MAX_TAP_TARGET_AREA; +} + +/** + * A narrower form of a selector that resolved to the WRONG element — the tapped + * node's own specific role added to the base. Returned best-first, or empty + * when nothing narrower is available. + * + * A derived selector is the plainest thing that describes the tapped node, so + * on a screen with repeats — a "Search" label shared by a field and a tab — it + * is ambiguous rather than absent. Ambiguity is not the same failure as "this + * element cannot be addressed", and it must not be answered with coordinates: + * the runner resolves the narrower form. + * + * Only the node's OWN role is added, and only when it is specific (not + * {@link GENERIC_ROLES}). The identifier is deliberately NOT narrowed on: + * {@link deriveSelector} already makes any stable, non-positional id the BASE + * selector, so when `base` carries no identifier the node has none left to + * add — its id is either absent or POSITIONAL, and a positional id is exactly + * what the recorder refuses. There is nothing an identifier branch here could + * contribute that deriveSelector has not already used or refused. + * + * A `within` scope is deliberately NOT derived here either, even though it + * would separate one feed row's button from another's: the flow tree is + * flattened, so a container can only be found geometrically, and geometry is + * z-order blind. With a modal open, the background screen's elements are still + * the smallest nodes under the point and the FOREGROUND modal's container is a + * perfectly good geometric ancestor — a tap on the composer's text input + * recorded as a feed post "inside" the composer, which then failed on any + * screen whose feed content differed. The scopes that survive are the ones an + * author writes knowingly at polish, against a container they have chosen. + */ +function narrowedSelectors(node: DescribeNode, base: Selector): Selector[] { + if (base.role !== undefined || !node.role || GENERIC_ROLES.has(node.role.toLowerCase())) { + return []; + } + return [{ ...base, role: node.role }]; +} + +/** + * Would replaying this selector reproduce the tap? + * + * Two things have to hold, and it is worth saying why it is not one. + * + * The frame must CONTAIN the tapped point — otherwise the selector matched + * some other element and lost the ranking, so the step targets the wrong + * control from the start. + * + * And the frame must be small enough to be a control rather than a container + * (see {@link MAX_TAP_TARGET_AREA}), because replay taps its CENTRE, not the + * point recorded here. A tap on blank space inside a drawer resolved to the + * drawer's whole scroll area and replayed onto the "Chat" item, reporting + * pass while navigating somewhere the walkthrough never went. + * + * What this deliberately does NOT do is require the centre to resolve back to + * the same tree node. That test was tried and is wrong on a FLATTENED tree: a + * control's own label is a SIBLING rect sitting on its centre, so a like + * button, a search field, a full-width row and every grid cell were refused — + * while replaying perfectly, because the touch is still inside the control. + * Node identity cannot tell a label from an independent control; size can tell + * a control from a container, which is the distinction that matters here. + */ +function replayReproducesTap( + frame: DescribeFrame, + point: { x: number; y: number } +): "ok" | "container" | "retargets" { + if (isContainerSized(frame)) return "container"; + if (!frameContains(frame, point.x, point.y)) return "retargets"; + return "ok"; +} + /** * For a recorded `gesture-tap`, look up the element under the tapped point and * record a portable `tap: { selector }` step instead of raw coordinates. @@ -608,15 +712,14 @@ async function captureTapSelector( registry: Registry, udid: string, point: { x: number; y: number } -): Promise<{ selector?: Selector; warning?: string }> { +): Promise<{ selector?: Selector; warning?: string; ambiguous?: boolean; container?: boolean }> { try { const device = resolveDevice(udid); const { tree, source } = await fetchFlowTree(registry, device); const node = nodeAtPoint(tree, point); - if (!node) return { warning: "no element found under the tap; kept coordinates (brittle)" }; + if (!node) return { warning: "no element found under the tap" }; const selector = deriveSelector(node); - if (!selector) - return { warning: "tapped element has no stable text/id; kept coordinates (brittle)" }; + if (!selector) return { warning: "tapped element has no stable text/id" }; // Replay resolves through selectorToFrame, whose ranking (exact match → // smallest frame → reading order) is free to elect a DIFFERENT element // than the tapped one — e.g. the same label on an earlier row. Re-resolve @@ -629,12 +732,44 @@ async function captureTapSelector( // always find something. Keep the guard (and an accurate message) in // case derivation and matching ever drift apart again. return { - warning: `selector ${describeSelector(selector)} matches no element on this screen; kept coordinates (brittle)`, + warning: `selector ${describeSelector(selector)} matches no element on this screen`, }; } - if (!frameContains(resolved, point.x, point.y)) { + const verdict = replayReproducesTap(resolved, point); + if (verdict === "container") { + // The selector resolves to an element covering most of the screen — on + // some trees a point on empty margin resolves to the screen root itself, + // which is addressable and looks like a perfectly good `{ id: }`. + // At this size the tree cannot tell a container from a genuinely + // full-bleed control, and narrowing cannot help: the problem is the + // element, not the selector. Kept coordinates either way — for a real + // container a centre-tap replay would fire elsewhere, and for a full-bleed + // control a coordinate replays as well as a selector would. return { - warning: `selector ${describeSelector(selector)} resolves to a different element on this screen; kept coordinates (brittle)`, + container: true, + warning: + `the tap landed on ${describeSelector(selector)}, which covers most of the screen — ` + + `at that size a container is indistinguishable from a control, and replay taps a ` + + `selector's CENTRE, so if it is a container a step recorded with it would fire ` + + `somewhere else entirely`, + }; + } + if (verdict === "retargets") { + // The selector matches the tapped element AND something else, and ranks + // the other one first. Narrow it before giving up — the runner resolves + // either narrower form, so answering ambiguity with coordinates would + // throw away a perfectly good target. + for (const candidate of narrowedSelectors(node, selector)) { + const frame = selectorToFrame(tree, candidate); + if (frame && replayReproducesTap(frame, point) === "ok") { + return { selector: candidate, warning: fallbackSourceWarning(source, device.platform) }; + } + } + return { + ambiguous: true, + warning: + `selector ${describeSelector(selector)} also matches another element on this screen, ` + + `and ranks it first — narrowing by the tapped element's own role did not single it out`, }; } const warnings = [ @@ -644,7 +779,7 @@ async function captureTapSelector( return { selector, ...(warnings.length > 0 ? { warning: warnings.join("; ") } : {}) }; } catch (err) { return { - warning: `selector capture failed (${err instanceof Error ? err.message : String(err)}); kept coordinates`, + warning: `selector capture failed (${err instanceof Error ? err.message : String(err)})`, }; } } @@ -868,6 +1003,87 @@ function directiveCommandHint(command: string): string | undefined { ); } +/** + * What to do about a tap whose selector could not be captured, now that the + * raw point has been kept. + * + * Three different failures, and they call for different responses: an element + * nothing can address, one that several things address equally, and one that + * covers most of the screen. Saying "no selector could be derived" for the + * second sends the author to re-discover a selector they already have; saying + * "an element with no id or label" for the third is simply false — the warning + * names the container's own id. The advice rides on the recorded step's warning + * because that is the only moment it is read while the screen is still there to + * retarget against — a coordinate step replays fine today and breaks on the + * first layout change, which is why the skills treat this warning as a stop + * rather than a note. + */ +function coordinateRemedy( + captured: { ambiguous?: boolean; container?: boolean }, + udid: unknown +): string { + if (captured.ambiguous) { + return ( + `Disambiguate it: give the intended element its own testID, or tap a target whose id is ` + + `unique on this screen. At polish, a hand-written \`within\`/\`after\`/\`next\` scope can ` + + `also single out the element this point hit.` + ); + } + if (captured.container) { + return ( + `Find the specific control under the point with ${treeReaderFor(udid)} and tap ITS centre — ` + + `the smallest element that is genuinely the target, not the full-screen container it sits in.` + ); + } + return ( + `Find the real target with ${treeReaderFor(udid)} and tap its centre. If the element ` + + `genuinely has no id or label, that is usually worth fixing in the app.` + ); +} + +function rawCoordinateWarning( + command: string, + args: Record, + delayMs: number | undefined +): string | undefined { + if (command === "gesture-tap" && delayMs !== undefined) { + return ( + "gesture-tap was kept as a raw coordinate tool step because flow-add-step delayMs prevents " + + "selector capture; remove delayMs, add a separate wait step before the tap if the pre-action " + + "delay is necessary, then record the tap again" + ); + } + if (command === "restart-app" && delayMs !== undefined) { + return ( + "restart-app was kept as a raw tool step because flow-add-step delayMs prevents the launch rewrite; " + + "remove delayMs so restart-app records as the leading launch, then record a post-launch " + + "await-ui-element readiness gate" + ); + } + if (command === "gesture-custom") { + return ( + "gesture-custom was recorded with raw coordinates because it has no selector-capture rewrite; " + + "if it contains a tap, record that tap individually with gesture-tap so selector capture can run" + ); + } + if ( + command === "run-sequence" && + Array.isArray(args.steps) && + args.steps.some( + (step) => + typeof step === "object" && + step !== null && + (step as { tool?: unknown }).tool === "gesture-tap" + ) + ) { + return ( + "run-sequence contains coordinate taps and was recorded as one opaque raw step; record taps " + + "individually so each can become a tap selector" + ); + } + return undefined; +} + /** * Whether this step must be refused rather than recorded, and why. * @@ -1406,7 +1622,9 @@ If a step was recorded by mistake, edit the .yaml to remove it — against a rem typeof args.x === "number" && typeof args.y === "number"; - let captured: { selector?: Selector; warning?: string } | undefined; + let captured: + | { selector?: Selector; warning?: string; ambiguous?: boolean; container?: boolean } + | undefined; if (isTap) { captured = await captureTapSelector(registry, args.udid as string, { x: args.x as number, @@ -1505,16 +1723,27 @@ If a step was recorded by mistake, edit the .yaml to remove it — against a rem step = { kind: "tap", selector: captured.selector, ...tapTimes }; warning = captured.warning; } else if (isTap) { - // No stable selector — keep a coordinate tap, but still as a `tap:` - // directive so every tap reads uniformly. + // No stable selector — keep a coordinate tap (still as a `tap:` + // directive so every tap reads uniformly), but recording the point is + // not an endorsement of it: say what failed AND what to do instead, + // since this warning is the whole of the author's signal that the flow + // just took on a step that survives only until the layout moves. step = { kind: "tap", x: args.x as number, y: args.y as number, ...tapTimes }; - warning = captured?.warning; + warning = captured?.warning + ? `${captured.warning}; kept coordinates, which replay at a fixed point and break on ` + + `any layout change. ${coordinateRemedy(captured, args.udid)} Keep the point only for ` + + `a genuinely unaddressable target (a canvas, a map, an unlabeled image), preceded by ` + + `an echo naming what it is.` + : undefined; } else if (isLaunch) { step = { kind: "launch", app: strippedArgs.bundleId as string }; } else if (runTarget?.flow) { step = { kind: "run", flow: runTarget.flow }; } else { - warning = crossTreeWarning ?? runTarget?.warning; + warning = + crossTreeWarning ?? + runTarget?.warning ?? + rawCoordinateWarning(params.command, args, params.delayMs); // The step ran live with the full args (incl. the device id), but the // recorded form drops the device id so the flow stays portable — the // runner injects whatever device it resolves at replay. diff --git a/packages/tool-server/src/utils/ui-tree-match.ts b/packages/tool-server/src/utils/ui-tree-match.ts index 5796e023e..3c7dc82d4 100644 --- a/packages/tool-server/src/utils/ui-tree-match.ts +++ b/packages/tool-server/src/utils/ui-tree-match.ts @@ -978,7 +978,7 @@ export function matchNode(node: DescribeNode, selector: Selector): boolean { const WITHIN_EPS = 0.005; /** Is `inner` contained in `outer`, within {@link WITHIN_EPS} per edge? */ -function frameWithin(inner: DescribeFrame, outer: DescribeFrame): boolean { +export function frameWithin(inner: DescribeFrame, outer: DescribeFrame): boolean { return ( inner.x >= outer.x - WITHIN_EPS && inner.y >= outer.y - WITHIN_EPS && @@ -1539,7 +1539,7 @@ export function selectorToFrame(root: DescribeNode, selector: Selector): Describ * then text; falls back to a specific (non-generic) role. Returns null when the * node has nothing stable to match on — the caller then keeps coordinates. */ -const GENERIC_ROLES = new Set([ +export const GENERIC_ROLES = new Set([ "axgroup", "group", "view", @@ -1551,8 +1551,27 @@ const GENERIC_ROLES = new Set([ "android.view.viewgroup", ]); +/** + * A POSITIONAL id — `profilePager-selector-2`, `tab-selector-0`. The number is + * the element's index among its siblings, so the id names a slot rather than a + * thing: it survives no re-order and silently addresses a different control + * once one is inserted before it. Recording one only ever produced a fragile + * step that looked strict — one an author has to notice and replace by hand. + * + * It matters most where the recorder is least reliable. The flow tree is + * flattened and carries no z-order, so a tap inside a full-screen modal can + * resolve against a view BEHIND it; observed on Bluesky's edit-profile sheet, + * where a tap on the display-name field derived the profile pager's "Media" + * tab. An ambiguous or oversized background match is already caught and warned + * about, but a positional id on a background node passes every one of those + * checks and records silently. Refusing it turns that case back into the + * kept-coordinate warning the author is told to act on. + */ +const POSITIONAL_ID = /-selector-\d+$/i; + export function deriveSelector(node: DescribeNode): Selector | null { - if (node.identifier && node.identifier.trim()) return { identifier: node.identifier }; + const id = node.identifier?.trim(); + if (id && !POSITIONAL_ID.test(id)) return { identifier: node.identifier! }; // Derive text from label OR value individually — never nodeText's joined // form: matchNode compares a text selector against label and value // separately, so a joined "Volume 50%" would match nothing, not even the diff --git a/packages/tool-server/test/flows/flow-record-tap.test.ts b/packages/tool-server/test/flows/flow-record-tap.test.ts index d77f6489d..37d2c67d0 100644 --- a/packages/tool-server/test/flows/flow-record-tap.test.ts +++ b/packages/tool-server/test/flows/flow-record-tap.test.ts @@ -183,7 +183,17 @@ describe("flow-add-step tap selector capture", () => { const result = await recordTap({ x: 0.2, y: 0.52 }); - expect(result.message).toContain("resolves to a different element"); + expect(result.message).toContain("also matches another element"); + expect(result.message).toContain("kept coordinates"); + // The warning names only what narrowing actually tries — the node's own + // role. Identifier-narrowing does not exist (deriveSelector already uses any + // stable id as the base), so the message must not claim it was attempted. + expect(result.message).toContain("narrowing by the tapped element's own role"); + expect(result.message).not.toContain("or identifier"); + // Ambiguity gets its own remedy — re-discovering a selector the recorder + // already derived is not the fix. + expect(result.message).toContain("Disambiguate it"); + expect(result.message).not.toContain("Find the real target"); expect(await recordedSteps()).toEqual([{ kind: "tap", x: 0.2, y: 0.52 }]); }); @@ -276,4 +286,236 @@ describe("flow-add-step tap selector capture", () => { await expect(recordTap({ x: 1.5, y: 0.52 })).rejects.toThrow(/normalized 0–1 fractions/i); expect(await recordedSteps()).toEqual([]); }); + + // An ambiguous selector is not an absent one. Refusing it, and offering + // coordinates as the only way forward, made the recorder unable to record + // two of the hottest targets in a real app — the search field and a list + // row — even though the runner resolves the narrower form perfectly. + it("narrows an ambiguous text selector by role rather than refusing", async () => { + setTree([ + n({ role: "Button", label: "Search", frame: { x: 0.2, y: 0.9, width: 0.2, height: 0.05 } }), + n({ + role: "TextField", + label: "Search", + frame: { x: 0.1, y: 0.1, width: 0.8, height: 0.06 }, + }), + ]); + + const result = await recordTap({ x: 0.5, y: 0.13 }); + + expect(result.message).toContain("Step added"); + expect(await recordedSteps()).toEqual([ + { kind: "tap", selector: { text: "Search", role: "TextField" } }, + ]); + }); + + it("keeps coordinates when the tapped element cannot be told apart by its own attributes", async () => { + // A `within` scope is deliberately NOT derived: the flow tree is + // flattened, so a container can only be found geometrically, and geometry + // is z-order blind — with a modal open, the foreground's container is a + // perfectly good "ancestor" of a background element. + setTree([ + n({ + identifier: "row-1", + frame: { x: 0, y: 0.1, width: 1, height: 0.1 }, + children: [ + n({ + role: "Button", + label: "Reply", + frame: { x: 0.1, y: 0.12, width: 0.2, height: 0.05 }, + }), + ], + }), + n({ + identifier: "row-2", + frame: { x: 0, y: 0.4, width: 1, height: 0.1 }, + children: [ + n({ + role: "Button", + label: "Reply", + frame: { x: 0.1, y: 0.42, width: 0.2, height: 0.05 }, + }), + ], + }), + ]); + + const result = await recordTap({ x: 0.2, y: 0.44 }); + + expect(result.message).toContain("also matches another element"); + expect(await recordedSteps()).toEqual([{ kind: "tap", x: 0.2, y: 0.44 }]); + }); + + // Regression: no derived form may carry a POSITIONAL id that deriveSelector + // refused. The Bluesky edit-profile modal — a tap resolves against a + // background pager tab whose ambiguous text ("Media") retargets, its role is + // generic (so nothing narrows), and its only other anchor is + // `profilePager-selector-2`. deriveSelector refuses that slot id, and + // narrowing is role-only, so no path can reintroduce it: the recorder keeps + // coordinates with the ambiguity warning instead. + it("does not re-inject a positional id when narrowing an ambiguous selector", async () => { + setTree([ + // A smaller, non-containing "Media" out-ranks the tapped node for a bare + // { text: "Media" }, so the base selector retargets. + n({ role: "view", label: "Media", frame: { x: 0.0, y: 0.9, width: 0.1, height: 0.03 } }), + // The tapped node: generic role (no role narrowing) and only a positional + // id to fall back on. + n({ + identifier: "profilePager-selector-2", + role: "view", + label: "Media", + frame: { x: 0.4, y: 0.15, width: 0.2, height: 0.1 }, + }), + ]); + + const result = await recordTap({ x: 0.5, y: 0.2 }); + + expect(result.message).toContain("also matches another element"); + expect(result.message).toContain("Disambiguate it"); + // Critically: the positional id was NOT smuggled back into a recorded step. + expect(result.message).not.toContain("profilePager-selector-2"); + expect(await recordedSteps()).toEqual([{ kind: "tap", x: 0.5, y: 0.2 }]); + }); + + // A tap on empty space resolves to whatever container spans that spot, and + // on some trees every screen root is addressable — so the derived selector + // looked perfect and `frameContains` passed trivially. Observed on Bluesky + // web: a tap on a profile page's empty left margin recorded + // `tap: { id: profileView }`, which on replay fired at the screen centre and + // activated a tab 45% of the screen away. + it("keeps coordinates for a tap that lands on a container rather than a control", async () => { + setTree([ + n({ identifier: "profileView", frame: { x: 0, y: 0, width: 1, height: 1 } }), + n({ role: "Button", label: "Follow", frame: { x: 0.45, y: 0.45, width: 0.1, height: 0.06 } }), + ]); + + const result = await recordTap({ x: 0.05, y: 0.5 }); + + expect(result.message).toContain("covers most of the screen"); + expect(result.message).toContain("a container is indistinguishable from a control"); + expect(result.message).toContain("not the full-screen container it sits in"); + // The point reproduces the tap; the container selector would not. Keeping + // it beats recording a step that fires 45% of the screen away. + expect(await recordedSteps()).toEqual([{ kind: "tap", x: 0.05, y: 0.5 }]); + }); + + it("still records a normal control on a screen that has a full-screen root", async () => { + setTree([ + n({ identifier: "profileView", frame: { x: 0, y: 0, width: 1, height: 1 } }), + n({ role: "Button", label: "Follow", frame: { x: 0.45, y: 0.45, width: 0.1, height: 0.06 } }), + ]); + + const result = await recordTap({ x: 0.5, y: 0.48 }); + + expect(result.message).toContain("Step added"); + expect(await recordedSteps()).toEqual([{ kind: "tap", selector: { text: "Follow" } }]); + }); + + // MAX_TAP_TARGET_AREA (0.6) is the line between a recordable control and a + // refused container; pin BOTH sides so the constant cannot silently drift and + // start recording containers (or refusing ordinary large controls) unnoticed. + it("records a control whose area sits just under the container threshold", async () => { + setTree([n({ label: "Banner", frame: { x: 0, y: 0.2, width: 1, height: 0.59 } })]); + + const result = await recordTap({ x: 0.5, y: 0.49 }); + + expect(result.message).toContain("Step added"); + expect(result.message).not.toContain("covers most of the screen"); + expect(await recordedSteps()).toEqual([{ kind: "tap", selector: { text: "Banner" } }]); + }); + + it("keeps coordinates for a target whose area sits just over the container threshold", async () => { + setTree([n({ label: "Banner", frame: { x: 0, y: 0.2, width: 1, height: 0.61 } })]); + + const result = await recordTap({ x: 0.5, y: 0.5 }); + + expect(result.message).toContain("covers most of the screen"); + expect(await recordedSteps()).toEqual([{ kind: "tap", x: 0.5, y: 0.5 }]); + }); + + // Neither ambiguous nor container: the tapped node has a generic role and no + // id or label, so deriveSelector returns null. The remedy must point at the + // tree reader (not "disambiguate", there is no selector; not "find a smaller + // control", it is not a container) and only CONDITIONALLY suggest the element + // itself is worth fixing — the failure may be that no node was addressable. + it("sends the author to find a real target when the tapped element is unaddressable", async () => { + setTree([n({ role: "AXGroup", frame: { x: 0.3, y: 0.5, width: 0.2, height: 0.05 } })]); + + const result = await recordTap({ x: 0.4, y: 0.52 }); + + expect(result.message).toContain("tapped element has no stable text/id"); + expect(result.message).toContain("Find the real target"); + expect(result.message).toContain("If the element genuinely has no id or label"); + expect(result.message).not.toContain("Disambiguate it"); + expect(result.message).not.toContain("covers most of the screen"); + expect(await recordedSteps()).toEqual([{ kind: "tap", x: 0.4, y: 0.52 }]); + }); + + // The flow tree is FLATTENED, so a control's own label is a SIBLING rect + // sitting on the control's centre. Requiring the centre to resolve back to + // the same node refused a like button, a search field, a full-width row and + // every grid cell — all of which replay perfectly, because the touch is + // still inside the control. Two reviewers reproduced this independently. + it("records a control whose own label sits on its centre", async () => { + setTree([ + n({ + identifier: "likeBtn", + label: "Like (393 likes)", + frame: { x: 0.522, y: 0.647, width: 0.14, height: 0.032 }, + }), + n({ + identifier: "likeCount", + label: "393", + frame: { x: 0.59, y: 0.653, width: 0.061, height: 0.02 }, + }), + ]); + + // Off-centre, on the glyph rather than the count — exactly how an agent + // taps a like button. + const result = await recordTap({ x: 0.556, y: 0.6625 }); + + expect(result.message).toContain("Step added"); + expect(await recordedSteps()).toEqual([{ kind: "tap", selector: { identifier: "likeBtn" } }]); + }); + + it("records a full-width row tapped off-centre, past its centred content", async () => { + setTree([ + n({ label: "Edit interests", frame: { x: 0.04, y: 0.281, width: 0.92, height: 0.038 } }), + // A centred chevron/icon leaf sitting on the row's centre — addressable, + // but not what the tap was aimed at. + n({ role: "AXImage", frame: { x: 0.47, y: 0.29, width: 0.06, height: 0.02 } }), + ]); + + const result = await recordTap({ x: 0.113, y: 0.2996 }); + + expect(result.message).toContain("Step added"); + expect(await recordedSteps()).toEqual([{ kind: "tap", selector: { text: "Edit interests" } }]); + }); + + it.each(["emulator-5554", "chromium-cdp-9222"])( + "does not consult native devtools while recording a tap on %s", + async (udid) => { + setTree([n({ label: "Continue", frame: { x: 0.3, y: 0.5, width: 0.4, height: 0.06 } })]); + const resolveService = vi.fn(async () => { + throw new Error("must not be consulted"); + }); + const registry = { + invokeTool: vi.fn(async () => ({ tapped: true })), + getTool: vi.fn(() => ({ inputSchema: { properties: { udid: {} } } })), + resolveService, + } as unknown as Registry; + const tool = createFlowAddStepTool(registry); + + await tool.execute( + {}, + { + name: FLOW, + project_root: tmpDir, + command: "gesture-tap", + args: JSON.stringify({ udid, x: 0.5, y: 0.52 }), + } + ); + + expect(resolveService).not.toHaveBeenCalled(); + } + ); }); diff --git a/packages/tool-server/test/flows/flow-tools.test.ts b/packages/tool-server/test/flows/flow-tools.test.ts index 41dc75fd4..61c897611 100644 --- a/packages/tool-server/test/flows/flow-tools.test.ts +++ b/packages/tool-server/test/flows/flow-tools.test.ts @@ -3566,4 +3566,114 @@ describe("summarizeStep rendering", () => { "4. tool: screenshot {} (after 2000ms)" ); }); + + it("warns when delayMs prevents gesture-tap selector capture", async () => { + const registry = createMockRegistry({ + "gesture-tap": { result: { tapped: true } }, + }); + const tool = createFlowAddStepTool(registry); + await flowStartRecordingTool.execute( + {}, + { name: "delayed-tap", project_root: tmpDir, executionPrerequisite: PREREQ } + ); + + const result = await tool.execute( + {}, + { + name: "delayed-tap", + project_root: tmpDir, + command: "gesture-tap", + args: '{"udid":"ABC","x":0.5,"y":0.3}', + delayMs: 500, + } + ); + + expect(result.message).toContain("raw coordinate tool step"); + expect(result.message).toContain("remove delayMs"); + expect(parseFlow(await onDisk("delayed-tap")).steps).toEqual([ + { + kind: "tool", + name: "gesture-tap", + args: { x: 0.5, y: 0.3 }, + delayMs: 500, + }, + ]); + }); + + it("warns when delayMs prevents restart-app from becoming the leading launch", async () => { + const registry = createMockRegistry({ + "restart-app": { result: { restarted: true } }, + }); + const tool = createFlowAddStepTool(registry); + await flowStartRecordingTool.execute({}, { name: "delayed-launch", project_root: tmpDir }); + + const result = await tool.execute( + {}, + { + name: "delayed-launch", + project_root: tmpDir, + command: "restart-app", + args: '{"udid":"ABC","bundleId":"com.acme.app"}', + delayMs: 500, + } + ); + + expect(result.message).toContain("prevents the launch rewrite"); + expect(result.message).toContain("post-launch await-ui-element"); + expect(parseFlow(await onDisk("delayed-launch")).steps[0]).toMatchObject({ + kind: "tool", + name: "restart-app", + delayMs: 500, + }); + }); + + it("warns when gesture-custom records an opaque coordinate gesture", async () => { + const registry = createMockRegistry({ + "gesture-custom": { result: { completed: true } }, + }); + const tool = createFlowAddStepTool(registry); + await flowStartRecordingTool.execute( + {}, + { name: "custom-gesture", project_root: tmpDir, executionPrerequisite: PREREQ } + ); + + const result = await tool.execute( + {}, + { + name: "custom-gesture", + project_root: tmpDir, + command: "gesture-custom", + args: '{"udid":"ABC","events":[{"type":"Down","x":0.5,"y":0.3},{"type":"Up","x":0.5,"y":0.3}]}', + } + ); + + expect(result.message).toContain("raw coordinates"); + expect(result.message).toContain("record that tap individually"); + }); + + it("warns when run-sequence hides coordinate taps in one opaque step", async () => { + const registry = createMockRegistry({ + "run-sequence": { + result: { completed: 1, total: 1, steps: [{ tool: "gesture-tap", result: {} }] }, + }, + }); + const tool = createFlowAddStepTool(registry); + await flowStartRecordingTool.execute( + {}, + { name: "sequence-tap", project_root: tmpDir, executionPrerequisite: PREREQ } + ); + + const result = await tool.execute( + {}, + { + name: "sequence-tap", + project_root: tmpDir, + command: "run-sequence", + args: '{"udid":"ABC","steps":[{"tool":"gesture-tap","args":{"x":0.5,"y":0.3}}]}', + } + ); + + expect(result.message).toContain("opaque raw step"); + expect(result.message).toContain("record taps individually"); + }); }); diff --git a/packages/tool-server/test/utils/ui-tree-match.test.ts b/packages/tool-server/test/utils/ui-tree-match.test.ts index 6ddbeb798..b7e62738d 100644 --- a/packages/tool-server/test/utils/ui-tree-match.test.ts +++ b/packages/tool-server/test/utils/ui-tree-match.test.ts @@ -1667,3 +1667,34 @@ describe("after / next (sibling) scoping", () => { ); }); }); + +describe("deriveSelector refuses a positional id", () => { + const at = (partial: Partial): DescribeNode => + node({ frame: { x: 0, y: 0, width: 0.1, height: 0.1 }, ...partial }); + + // `…-selector-` numbers an element by its slot among siblings, so it + // addresses a different control after any re-order or insertion. The skill's + // blocking audit rejects it, and it is the one shape that reached the flow + // file SILENTLY: an ambiguous or container-sized background match is warned + // about, but a positional id on a view behind a modal passes every check. + it("falls through to text rather than a positional id", () => { + expect(at({ identifier: "profilePager-selector-2", label: "Media" })).toBeTruthy(); + expect(deriveSelector(at({ identifier: "profilePager-selector-2", label: "Media" }))).toEqual({ + text: "Media", + }); + }); + + it("keeps no selector at all when the positional id was the only anchor", () => { + expect(deriveSelector(at({ identifier: "tab-selector-0" }))).toBeNull(); + }); + + it("leaves ordinary ids alone, including ones merely containing digits", () => { + expect(deriveSelector(at({ identifier: "bottomBarProfileBtn" }))).toEqual({ + identifier: "bottomBarProfileBtn", + }); + expect(deriveSelector(at({ identifier: "selector-2-row" }))).toEqual({ + identifier: "selector-2-row", + }); + expect(deriveSelector(at({ identifier: "post-2" }))).toEqual({ identifier: "post-2" }); + }); +});