feat(tool-server): add find tool to locate an element and optionally act on it#427
feat(tool-server): add find tool to locate an element and optionally act on it#427hubgan wants to merge 16 commits into
Conversation
latekvo
left a comment
latekvo
left a comment
There was a problem hiding this comment.
Reviewed end-to-end: built and ran the tool-server suite, drove the compiled find against a real device's accessibility tree (it selected the correct element, flagged the ambiguity, and the computed tap landed on the intended row), and exercised the edge paths directly. The shared-primitive extraction and the await-ui-element refactor are behaviour-preserving, and the locator/selection logic is sound. Two inline notes.
| registry, | ||
| ctx, | ||
| params.udid, | ||
| chosen.value?.length ?? 0 |
There was a problem hiding this comment.
fill derives the number of backspaces to clear the field from chosen.value?.length, but on several platforms a populated field's current editable text is not in node.value. On Chromium an <input>'s text is exposed as node.label (accessibleName → el.value/placeholder), while node.value is ownText(), which is empty for an input; on Android an EditText without a content-desc likewise reports its text as label with value unset (makeUiNode only sets value when text !== label). In those cases chosen.value?.length is 0, so clearField sends only the 2-char CLEAR_BUFFER no matter how much text the field holds, and fill types the new text on top of the leftover — e.g. filling a field containing old@example.com with NEW yields old@example.cNEW. The result still reports found: true, clearedChars: 2, and no note (the only incomplete-clear caveat is the masked-password branch below). Confirmed against the compiled tool with a Chromium-shaped input node (15 chars of text in label): 2 backspaces sent, 13 characters left behind, success reported. (Separately, even when value is correctly populated, the clear is capped at MAX_CLEAR_CHARS = 64 with the same silent-success behavior for longer fields.)
| @@ -0,0 +1,21 @@ | |||
| const FIND_TOOL_ID = "find"; | |||
There was a problem hiding this comment.
FIND_TOOL_ID is re-declared here as a local "find" literal, while the tool already exports FIND_TOOL_ID from tools/find/index.ts — the parallel await-ui-element guard imports its shared AWAIT_UI_ELEMENT_TOOL_ID constant for exactly this purpose. If the tool id ever changes, this private copy silently stops matching and missed-find detection in both flow-add-step and flow-run regresses with no compile error.
latekvo
left a comment
latekvo
left a comment
There was a problem hiding this comment.
Reviewed the full diff and ran it end-to-end against a live iOS 18.5 simulator and a live Chromium (CDP) target, plus the tool-server suite (1665 tests) and argent-mcp suite — all green — and tsc / eslint / prettier clean.
The tool behaves as intended on device: exists, get-attrs, the not-found diagnostics, the ambiguity matchCount/note, tap (verified a real navigation), type, wait (hit and bounded timeout), and fill (clean replacement of a long overflowing single-line field) all worked. The await-ui-element refactor is behaviour-preserving, the flow record/replay integration and the run-sequence exclusion are sound, and the skill/docs changes match the implementation. No regressions in the affected packages.
Two things worth a look, both inline below and neither blocking the core behaviour: a Chromium contenteditable case where fill can silently under-clear, and a cancellation note that misdescribes a field that was already touched.
| // unknowable — clear up to the cap so a populated field is emptied | ||
| // regardless, and flag it. Elsewhere max(value,label) is the true | ||
| // length and never shorter than the real text. | ||
| const lengthHidden = device.platform === "chromium" && !chosen.value; |
There was a problem hiding this comment.
On Chromium, fill sizes its backspace-clear from the described value (via editableTextLength), and the cap-and-warn path is only taken when value is empty (lengthHidden). But for a contenteditable element the DOM accessibility snapshot sets value to the element's own direct text nodes only — it excludes any text inside nested inline children (a <b>, a mention/emoji span, a link). A populated contenteditable therefore reports a non-empty but undercounted value, so lengthHidden stays false and the safeguard just below is skipped: the clear is sized to the undercount, only the leading characters are deleted, the remainder stays in the field, and the new text is inserted in front of it — reported as a successful fill with no caveat.
Reproduced on Chrome: find fill on <div contenteditable>abc<b>defghijklmnopqrst</b></div> (real content abcdefghijklmnopqrst) with text ZZZ sent 5 backspaces (clearedChars: 5), returned note unset, and the element's textContent ended up as abcdefghijklmnoZZZ rather than ZZZ.
| ...baseResult(), | ||
| found: false, | ||
| matchCount: 0, | ||
| note: "find was cancelled before the element was located", |
There was a problem hiding this comment.
This message is also returned from the type and fill abort paths — when the signal fires during the focus settle (the focusAndSettle false-return) or mid-clear just below. On those paths the element has already been located and a device effect has already been dispatched: the focus tap always fires, and for fill one or more backspaces may have already deleted field content. So on a mid-action cancel the field can end up focused and partially cleared, yet the result reports found: false, matchCount: 0 with "cancelled before the element was located" — telling a caller nothing happened when the field was in fact mutated, which is misleading for recovery.
latekvo
left a comment
latekvo
left a comment
There was a problem hiding this comment.
Re-reviewed the full diff independently, drove the compiled find tool end-to-end against a live iOS 18.5 device, and re-ran the tool-server + argent-mcp suites (all green). The shared-primitive extraction (describe/match.ts, describe/fetch-tree.ts, utils/timing.ts) and the await-ui-element refactor are behaviour-preserving; the locator/selection, poll/deadline, and abort logic are sound; the flow record/replay integration and the run-sequence exclusion are correct; and the skill/docs changes match the implementation.
On device I confirmed exists, get-attrs, the not-found diagnostics, the matchCount ambiguity note, tap (verified a real navigation), type, and fill all behave as intended. I specifically stress-tested fill on a wide field holding content that extends past the tap centre on native iOS fields (both a search field and a plain text field, each not pre-focused): it cleared the field completely in every case — the focusing tap parks the caret at the end of the existing text, so the backspace clear is not corrupted by tap position.
The two earlier inline notes that were substantive have been resolved (the fill clear now sizes from the longer of value/label via editableTextLength, and flow-step-results.ts now imports FIND_TOOL_ID rather than re-declaring it). The Chromium-fill under-clear and the cancellation-note wording remain as previously noted and are non-blocking. One small documentation note inline.
Approving.
| Fails if the flow file does not exist or a step tool raises an error (execution stops at that step). | ||
| An \`await-ui-element\` step whose condition is not met before its timeout also stops the flow there | ||
| (its later steps were recorded assuming the condition held), so use one to gate a step on a screen transition. | ||
| A \`find\` step that returns \`{ found: false }\` also stops the flow there, so replay never silently |
There was a problem hiding this comment.
This says a find step returning { found: false } stops the flow, but isMissedFindResult exempts action: "exists" — an exists step reporting found: false is a successful "not present" answer and neither stops the flow here nor blocks recording in flow-add-step. As written, the description (and the matching one in flow-add-step) reads as if every found: false halts replay, which would mislead an agent that uses find … action:"exists" as a presence check inside a flow.
…act on it Combines discovery + action in one round-trip: locate a UI element by a locator (any/text/label/value/role/id) and optionally tap, focus, type, fill, check existence, wait for it, or read its text/attrs. Extracts the shared tree-matching primitives (root-excluding walk, reading-order sort, visibility proxy) into describe/match.ts, reused by both find and await-ui-element.
- Share the per-platform tree fetch (describe/fetch-tree.ts) and settleWithin (utils/timing.ts) with await-ui-element; bound find's poll/fetch by the deadline and abort signal so wait can't overshoot timeoutMs on a hung describe. - fill/type focus with a centre tap (empirically clears the field on iOS+Android) instead of a trailing-edge tap that could hit a field's clear button. - Platform-aware focus settle (Android 450ms / iOS 150ms / Chromium 0) so the first keystroke isn't dropped while the soft keyboard raises. - exists now folds degraded-AX hints into its not-found note; clamp the poll sleep; matchCount counts the actionable pool; action-aware ambiguity note; append (not overwrite) the password warning; honor abort after the focus tap; drop the redundant acted field and the duplicate get-attrs payload. - Add find to the device-interact skill + tapping rule; note why find is kept out of run-sequence; expand tests (degraded note, fetch error, action-phase abort, centre-tap, clear bounds, ambiguity note, focus, get-text label-only).
…lear clearField stops early on abort but can't signal it; without this guard a fill cancelled during the backspace loop would still push `text` in and report a successful fill instead of returning cancelled(). Adds a deterministic test.
- Reference find in the device-interact / RN-workflow / screenshot-diff /
test-ui-flow / setup skills and the argent rule as a locate-and-act tool.
- Clarify flow recording: flow-add-step records a step unless the tool throws,
so a non-throwing failure (find's { found: false }) is still recorded; do not
record find steps in v1 flows, and inspect each result before continuing.
- Note find's actionResult.timestampMs (tap/focus) for profiler annotations.
… than explain in SKILL files
Addresses the outstanding review comments after the rebase onto main: - flow-run / flow-add-step docs now state that a `find` with action:"exists" is exempt from the found:false stop/skip rule (its found:false is a valid "not present" answer), matching isMissedFindResult. - `fill` on Chromium always sizes the clear from the cap: a contenteditable reports a non-empty but undercounted `value` (direct text nodes only), so the old `!value` guard under-cleared and left stale text. Caveat wording updated to cover both inputs and contenteditable. - A cancel that lands after the element was located and a device effect was dispatched (type/fill focus settle, or mid-clear) now reports found:true with an accurate note instead of "cancelled before the element was located". Integration with main: main's describeIos now probes isTvOsSimulator (shelling xcrun) per fetch. find now pre-resolves the tvOS verdict once before the wait clock and threads it through fetchDescribeTree — mirroring await-ui-element — so polling never re-shells xcrun per iteration. find tests stub isTvOsSimulator for determinism. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address code-review findings on the find tool: - H1: bias fill's focus tap to the field trailing edge so the leftward clear starts at end-of-text (not a mid-text caret), and rename the result field clearedChars -> backspacesSent (it counts backspaces sent, not chars proven removed). - M1: for tapping actions, demote a match whose frame encloses another so default index:0 lands on the inner input, not an aggregating Android container that folds the child's text into its content-desc. - M2: reject acting actions (tap/focus/type/fill) on a TV target up front (tvOS + Android TV) before any device effect, so fill can't throw on the first backspace or silently no-op; find is read-only on TV. - L1: exists reports the topmost visible match (not a zero-area ghost). - L2: exists sets presenceUnknown when the tree was unreadable, so a blind read isn't reported as a confirmed absence. - N1: poll-timeout note says "poll budget" for polling non-wait actions. - Docs (D1-D4): SKILL index/TV semantics, run-sequence exclusion rationale, sub-invoke caller list. - Tests (T1-T5): slow/hung/aborted fetch, poll-sleep clamp, tvOS probe once, type mid-settle abort, matchCount all-vs-visible, plus M1/M2. - T6: mock isTvOsSimulator/isAndroidTv in await-ui-element tests so the describe path stops shelling out per poll (the real flakiness root). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ction; flag fill over-delete - fill: extend the Chromium clear caveat to also warn about the over-delete direction (an inner block of a multi-block contenteditable can have its fixed-count backspaces run into the preceding block), so both under- and over-clear are surfaced instead of only leftover text. - flow-execute: assert a recorded `exists` find returning found:false does not stop the flow (replay side of the found:false exemption). - find: assert a read-only action on a tvOS target is not rejected and returns a valid absent answer carrying the tvOS hint (tvOS AX serves no focus tree, so it is found:false, unlike Android TV's real uiautomator tree). - describeChromium: run the real DOM walker against a faithful fake DOM to verify an <input> yields label=placeholder with empty value, and a contenteditable exposes only its direct text nodes.
…edges - fill now clears the enclosing editable region, not the demoted inner proxy, and parks the caret at the field's bottom-trailing corner — fixes the iOS search-bar "abcd" -> "WxyzAbcd" corruption and multi-line residue - editableTextLength prefers `value` when present so an empty field's placeholder label isn't backspaced (and can't trip a false cap warning) - exists reports presenceUnknown for any blind read (empty tree flagged with a hint/should_restart: tvOS, degraded AX, mid-restart), mirroring await-ui-element's isBlindRead — not just lastTree === null - TV acting-guard fails closed on an indeterminate Android form-factor probe; iOS left as-is (a physical iPhone legitimately probes undefined) - not-found note no longer mislabels a genuine miss as a fetch failure after a transient error poll - flow-add-step rejects an unmet await-ui-element (would halt every replay) - read-only find actions use describe's short auto-screenshot delay - clarify action-dependent `index` semantics in the zod help and runtime note; document fill's destructive clear in the tool description and SKILL Tests: enclosing-region fill, empty-field clear, blind-read exists, fail-closed probe, exists topmost-visible, run-sequence exclusion, unmet-wait recording, and auto-screenshot delay. tool-server 1865 pass, argent-mcp 68 pass.
The fake-DOM harness runs describeChromium's own page-eval expression via new Function against injected globals — an intentional, non-attacker-controlled use. eslint-disable-next-line keeps the branch's ESLint check green.
… (fix CI) find.test.ts: also mock isAndroidTv, not just getAndroidRuntimeKind. The Android describe path (`describeAndroid`) calls the real isAndroidTv for a TV hint, which shells `adb`. Mocking only getAndroidRuntimeKind left isAndroidTv real, so the Android find tests passed only where adb is on PATH (a dev laptop / the CI Android image) and shelled adb otherwise. Mocking both keeps every Android path adb-free on any host. chromium-describe-value-extraction.test.ts: evaluate describeChromium's page-eval script in a `vm` sandbox instead of a bare `new Function`. A browser runs it against a complete `window` where every global resolves (to a value or undefined); `new Function` resolves free identifiers against Node's sparse global scope, so a stray DOM-global reference threw "Node is not defined" on the CI runner while passing locally. The sandbox's permissive `has` trap makes any un-provided global resolve to undefined (built-ins fall through to globalThis), matching browser semantics and removing the environment-dependent ReferenceError. Also drops the no-implied-eval disable (vm.runInContext isn't implied eval). Verified: full tool-server suite 1865 pass on macOS (node 24) and in a Linux node-20 container (matching CI, adb installed), isolated and full-suite.
The vm/Proxy sandbox from the previous commit crashed vm.createContext on the CI
runner ("Cannot read properties of undefined (reading 'prototype')") — a permissive
`has` trap doesn't survive Node's vm internals there. Revert to the original
`new Function` harness and instead inject `Node` (and `HTMLElement`) as explicit
params. This repo's page script never references `Node`, yet the CI runner threw
"Node is not defined" evaluating it (a stray DOM-global reference resolved against
Node.js's sparse global scope); binding `Node` to an injected stub makes that
identifier resolve regardless of host, which is a guaranteed fix for that exact
ReferenceError with none of the vm fragility.
… elements by text/label/role/id and optionally act on them Resolves conflicts with main's await-screen-idle refactor by keeping both: - HEAD's await-screen-idle + pollDescribeTree improvements - PR software-mansion#427's find tool + describe/fetch-tree extraction The find tool collapses describe → read-coords → gesture-tap into a single call: find { query: "Sign In", by: "text", action: "tap" } Original PR: software-mansion#427
2 participants
Summary
Adds a
findtool that locates a UI element by a locator and optionally performs an action on the match, collapsing the usualdescribe-> read coordinates ->gesture-tapcycle into a single call. Works on iOS, Android, and Chromium (not Vega, which has no touch model).by):any(label/value/id, the default),text(label or value),label,value,role,id. Case-insensitive substring.action):tap(default, centre),focus,type(focus + type),fill(focus + clear + type),exists(single presence check),wait(block until visible),get-text,get-attrs.{ found, matchCount, match?, actionResult?, elapsed, note? }.matchCountflags ambiguous queries;indexdisambiguates. A single tree snapshot is taken by default;timeoutMsopts into polling until the element appears before acting.How it's built
gesture-tap/keyboardtools viainvokeSubTool(free telemetry attribution; no duplicated transport; read-only actions never spawn the simulator-server).describe/match.ts(root-excluding walk, reading-order sort, visibility proxy) and the tree fetch +settleWithinintodescribe/fetch-tree.ts/utils/timing.ts, reused by bothfindandawait-ui-element(the latter shrinks by ~170 lines, behaviour-preserving).fill(verified to fully clear fields on iOS and Android); fetch bounded by the deadline + abort signal sowaitcan't overshoottimeoutMs.findis intentionally kept out ofrun-sequencefor v1: unlikeawait-ui-element, a missedfindreturns{ found: false }without throwing, which the sequence stop-guard wouldn't catch (documented inline). The skill/flow docs are updated to match.Verification
vitest: newfind.test.ts(38 cases) plusawait-ui-element/run-sequence/sub-invoke/ flows all green;tsc --noEmitandeslintclean.exists,get-attrs,tap(verified tab switch via before/after screenshots),type,fill(clean clear + replace), andwait(hit + timeout). The device run caught and fixed an Android first-keystroke-drop race.Draft for review.