COMPUTER-USE part 1: CU-1, CU-3, CU-4, CU-5, CU-6, CU-7 (THE-104)#134
COMPUTER-USE part 1: CU-1, CU-3, CU-4, CU-5, CU-6, CU-7 (THE-104)#134thecombatwombat wants to merge 13 commits into
Conversation
`visual-snapshot` was returning COMMAND_BLOCKED in production because UiAutomatorAdapter.getCurrentApp built shell payloads like `dumpsys activity activities | grep mResumedActivity` and `dumpsys window | grep mCurrentFocus`. The `|` was correctly rejected by validateShellPayload in src/services/process-runner.ts. The guard is the right behavior — the caller is wrong. Fix: run `dumpsys activity activities` and `dumpsys window` directly (no pipe) and parse the multi-line output in TypeScript. New parsers live in src/parsers/dumpsys-current-app.ts so the logic is unit-testable in isolation. External shape and primary/secondary semantics of getCurrentApp are unchanged. Tests: - New unit tests in tests/parsers/ for the pure parsers, covering realistic multi-line dumpsys output, ignoring non-mResumedActivity lines that also reference ActivityRecords, multi-user devices, CRLF, and empty input. - Existing tests/adapters/ui-automator.test.ts getCurrentApp suite now asserts the issued shell commands contain no metacharacters the safety guard blocks — this is the test that would have caught the original bug. - New tests/integration/visual-snapshot-shell-safety.test.ts wires the real ProcessRunner (with the real validateShellPayload) into AdbAdapter + UiAutomatorAdapter and exercises getCurrentApp end-to-end with execa mocked, so any future regression that re-introduces shell metacharacters on this code path will fail at the same layer where production fails today. DECISIONS.md updated to record the invariant: internal adb.shell callers never use composition operators; parse output in TS instead. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ui-query dump, ui-query find, and ui-capture screenshot now all return a
top-level `app: { packageName, activityName } | null` field so the agent
can tell which app is in focus without an extra round-trip. visualSnapshot
already exposed this; this PR extends it to the other three surfaces.
New shared helper `getCurrentAppSafe` wraps `context.ui.getCurrentApp` and
returns null on failure rather than propagating, so a transient adb hiccup
never tears down the parent operation. The failure is logged at WARN.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ui-query find and dump now accept `interactiveOnly: boolean` (default false). When true, the result is filtered to nodes where any of clickable, long-clickable, focusable, editable, or scrollable is true — the standard set of Android interactivity flags. To make long-clickable and editable observable, the ui-dump parser is extended to capture them (previously only clickable / focusable / scrollable were extracted). New `isInteractiveNode` helper in the parser module keeps the rule canonical. Filter applies after selector matching, before pagination. OCR / grid results in find pass through unchanged — they're inherently tap targets without the AX flags. Default behavior is preserved; this is purely additive. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Schema describe text on `direction` and the ui-action tool description now both state explicitly that direction is gesture-named, not content- named: `down` = swipe up = content scrolls down. No schema change, no new alias values — the decision and rationale are recorded in DECISIONS.md. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds opt-in `rank: "bestTappable"` to the selector schemas of both
ui-query find and ui-action. The new util-rank module scores candidates:
+ clickable — strong bonus
+ long-clickable — medium bonus
+ focusable — small bonus
- bounding-box area — smaller wins
- root-container — area >= 90% of largest in set gets a heavy
penalty (full-screen wrappers stay last)
ui-query find with rank: returns the candidates reordered, plus
`pickedRationale` and `alternatives` (other candidates' summaries).
ui-action tap with rank + matches > 1 + no nearestTo: auto-picks the
top-ranked candidate and includes the same `pickedRationale` /
`alternatives` in the response. Without `rank`, the existing
AMBIGUOUS_MATCH error is preserved exactly. The error suggestion now
also mentions `rank: 'bestTappable'` as a disambiguation option.
Non-accessibility match types (OCR text, grid candidates) pass through
ranking unchanged — they're already point-shaped tap targets.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ui-capture screenshot now returns a stable `screenshotId` and pins
that screenshot's scaling triple (scaleFactor + device dims + image
dims) in the existing CacheManager under a new SCREENSHOT_SCALING TTL
(5 min). ui-action tap accepts a new `{ screenshotId, imageX, imageY }`
input combo and converts to device-space using THAT screenshot's
scaling — not whatever global adapter scalingState happens to be in
effect when the next screenshot races in.
Unknown / expired screenshotId surfaces as a new UNKNOWN_SCREENSHOT_ID
ErrorCode with a "take a new screenshot — entries last 5 min"
remediation. screenshotId without imageX/imageY rejects up front with
INPUT_VALIDATION_FAILED.
Existing tap input shapes (raw x/y, elementIndex, selector) are
unchanged. The DECISIONS.md entry recording the TTL choice will be
added separately — the auto-mode classifier blocked the inline edit
on this commit; the rationale is captured in the PR description.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 624201e7fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| return handleFullDump(tree, dumpId, deviceId, emptyWarning, coordMeta); | ||
| return handleFullDump(tree, dumpId, deviceId, emptyWarning, coordMeta, app); |
There was a problem hiding this comment.
When callers request ui-query dump with compact: false and interactiveOnly: true, this branch sends the original tree to handleFullDump, so non-interactive nodes are still returned. The new schema documents interactiveOnly for dump in general, and the compact path applies it, so full-tree dumps give contradictory results for the same option; filter or prune the tree before this full-dump return path.
Useful? React with 👍 / 👎.
| deviceId: string, | ||
| ): Promise<CurrentAppField | null> { | ||
| try { | ||
| return await context.ui.getCurrentApp(deviceId); |
There was a problem hiding this comment.
When dumpsys parsing cannot identify the foreground app, UiAutomatorAdapter.getCurrentApp() returns the sentinel { packageName: "unknown", activityName: "unknown" } rather than throwing, so this helper now exposes that sentinel in the new app response field instead of the documented app: ... | null. In that parse-failure scenario, clients cannot distinguish a real package from an unresolved lookup; normalize the sentinel to null here as the helper comment describes.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR delivers six CU tickets under THE-104: a shell-safety fix for
Confidence Score: 5/5All three previously flagged defects are resolved; no new logic errors found in six tickets of new functionality. The shell-safety fix is clean and backed by a real-stack integration test. The bestTappable scoring inversion is eliminated by switching from a relative-area threshold to a sqrt-penalty that keeps the clickable bonus dominant for typical-size targets. The interactiveOnly flag now applies consistently in both compact and full-tree dump paths. The screenshotId is conditionally minted only when the adapter supplies all required scaling metadata, and the image-to-device conversion uses the pinned scaleFactor from that specific screenshot. No auth, injection, or data-loss issues identified. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant ui_capture as ui-capture screenshot
participant CacheManager
participant ui_action as ui-action tap
participant UiAutomator
Agent->>ui_capture: "{ operation: screenshot }"
ui_capture->>UiAutomator: screenshot() + getCurrentApp() [parallel]
UiAutomator-->>ui_capture: "ScreenshotResult { scaleFactor, device, image }"
ui_capture->>CacheManager: "set(screenshotId, ScreenshotScalingEntry, TTL=5min)"
ui_capture-->>Agent: "{ screenshotId, app, ... }"
Agent->>ui_action: "{ operation: tap, screenshotId, imageX, imageY }"
ui_action->>CacheManager: get(screenshotId)
alt entry found and not expired
ui_action->>ui_action: toDeviceSpace(imageX, imageY, scaleFactor)
ui_action->>UiAutomator: "tap(deviceX, deviceY, deviceSpace=true)"
ui_action-->>Agent: "{ tapped, viaScreenshotId, imageCoords }"
else expired or unknown
ui_action-->>Agent: UNKNOWN_SCREENSHOT_ID error
end
Reviews (5): Last reviewed commit: "docs: list foreground app identity in PR..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (input.compact !== false) { | ||
| return handleCompactDump(tree, input, dumpId, deviceId, emptyWarning, coordMeta); | ||
| return handleCompactDump(tree, input, dumpId, deviceId, emptyWarning, coordMeta, app); | ||
| } | ||
|
|
||
| return handleFullDump(tree, dumpId, deviceId, emptyWarning, coordMeta); | ||
| return handleFullDump(tree, dumpId, deviceId, emptyWarning, coordMeta, app); |
There was a problem hiding this comment.
interactiveOnly silently ignored for full dump
When compact: false, input is not forwarded to handleFullDump, so interactiveOnly: true is a no-op and the caller receives the unfiltered tree. A user or agent who sets interactiveOnly: true, compact: false will see all nodes including non-interactive containers, contradicting the documented behavior and their explicit intent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/ui-query.ts
Line: 126-130
Comment:
**`interactiveOnly` silently ignored for full dump**
When `compact: false`, `input` is not forwarded to `handleFullDump`, so `interactiveOnly: true` is a no-op and the caller receives the unfiltered tree. A user or agent who sets `interactiveOnly: true, compact: false` will see all nodes including non-interactive containers, contradicting the documented behavior and their explicit intent.
How can I resolve this? If you propose a fix, please make it concise.| if (node.focusable) score += 100; | ||
| const area = nodeArea(node); | ||
| score -= area; | ||
| if (maxAreaInSet > 0 && area >= maxAreaInSet * ROOT_AREA_RATIO) { | ||
| score -= ROOT_PENALTY; | ||
| } | ||
| return score; | ||
| } | ||
|
|
||
| function summarizeAxNode(node: AccessibilityNode): Record<string, unknown> { | ||
| return { | ||
| text: node.text || node.contentDesc || undefined, |
There was a problem hiding this comment.
ROOT_AREA_RATIO penalizes the largest element in any small set
scoreAxNode applies ROOT_PENALTY when area >= maxAreaInSet * 0.9. In a two-element candidate set { clickable Button (100x40 = 4000 px2), non-clickable TextView (80x16 = 1280 px2) }, the Button is the largest element so 4000 >= 4000 x 0.9 evaluates to true — the clickable Button is penalized as a "root container" and scores 1000 - 4000 - 10000 = -13000, losing to the non-clickable label at 0 - 1280 = -1280. bestTappable then picks the non-interactive child, defeating its own purpose. The relative threshold should be anchored to an absolute screen-size reference, not the local max in the candidate set.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/util-rank.ts
Line: 33-44
Comment:
**`ROOT_AREA_RATIO` penalizes the largest element in any small set**
`scoreAxNode` applies `ROOT_PENALTY` when `area >= maxAreaInSet * 0.9`. In a two-element candidate set `{ clickable Button (100x40 = 4000 px2), non-clickable TextView (80x16 = 1280 px2) }`, the Button is the largest element so `4000 >= 4000 x 0.9` evaluates to true — the clickable Button is penalized as a "root container" and scores `1000 - 4000 - 10000 = -13000`, losing to the non-clickable label at `0 - 1280 = -1280`. `bestTappable` then picks the non-interactive child, defeating its own purpose. The relative threshold should be anchored to an absolute screen-size reference, not the local max in the candidate set.
How can I resolve this? If you propose a fix, please make it concise.- src/tools/util-rank.ts: replace `as unknown as AccessibilityNode`
with a local type predicate `isAx` so generic T narrows to
(T & AccessibilityNode) inside the conditional. No behavior change.
- src/tools/ui-action.ts: extract the screenshotId branch of handleTap
into resolveScreenshotIdTap so handleTap drops back under the 80-line
function limit.
- tests/tools/token-budget.test.ts: raise ceiling 2200 → 2600 to
accommodate the new interactiveOnly / rank / imageX/Y / screenshotId
fields landed in this PR. History line records the bump.
- docs/contracts/{replicant-mcp.contract.json,tool-schema-tokens.json}:
regenerate to match the new schemas (CU-3 / CU-4 / CU-5 / CU-7).
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(CU-4 follow-up) Greptile flagged a scoring inversion: a clickable Button (100x40 = 4000 px², score 1000-4000-10000=-13000 with root-penalty-fires-on-largest) lost the rank to a non-clickable TextView (80x16 = 1280 px², score 0-1280=-1280). Even with the root penalty removed, the clickable bonus (+1000) was too small relative to the area-penalty difference (2720) — the area term dominates for any candidate set where the clickable peer isn't tiny. Fix: scale CLICKABLE_BONUS (1000→100000), LONG_CLICKABLE_BONUS (500→ 50000), and FOCUSABLE_BONUS (100→10000) by 100x. The proportions stay the same. With these constants, interactivity wins for elements up to ~100k px² (typical UI widget size), while area still distinguishes among interactive candidates AND still demotes truly full-screen containers (millions of px², where the area dominates and the root penalty piles on). All existing ranking tests continue to pass; the Greptile repro case is covered by a new regression test. Co-Authored-By: Claude <noreply@anthropic.com>
Greptile flagged that `handleFullDump` doesn't receive `input`, so
`{ compact: false, interactiveOnly: true }` returns the unfiltered tree
— silently ignoring the caller's intent. The flag worked in compact
mode but was a no-op for the tree view.
Fix: thread `input` into `handleFullDump` and, when `interactiveOnly`
is true, prune subtrees that contain no interactive descendants. Tree
shape is preserved: structural ancestors of interactive nodes stay so
the caller still gets hierarchical context (which is the reason to
prefer full-tree over compact mode in the first place).
Co-Authored-By: Claude <noreply@anthropic.com>
|
@greptileai please re-review — addressed the two findings on this PR:
|
|
@codex review — addressed the two findings on this PR since your initial pass on commit
Head is now |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad60f3678e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (node.longClickable) score += LONG_CLICKABLE_BONUS; | ||
| if (node.focusable) score += FOCUSABLE_BONUS; | ||
| const area = nodeArea(node); | ||
| score -= area; |
There was a problem hiding this comment.
For common full-width list rows or cards on 1080px devices, a legitimate clickable target can be about 1080×120 (~130k px²). With this scoring it gets 100000 - 129600 and, because it is the largest candidate, another root penalty, while a non-clickable child label at 400×30 scores -12000; rank=bestTappable will still pick the label over the tappable row. This preserves the CU-4 inversion for a common Android target shape and can make ui-action tap non-interactive text instead of the clickable container.
Useful? React with 👍 / 👎.
| offset, | ||
| limit, | ||
| deviceId, | ||
| app, |
There was a problem hiding this comment.
This adds app to ui-query dump responses, and similar new fields are added to find/tap responses, but the generated contract still declares UiDump*Output, UiFindOutput, and UiTapOutput with additionalProperties: false and without these fields. Clients or tests validating responses against docs/contracts/replicant-mcp.contract.json will reject valid runtime responses from these new paths, so the source output schemas/contracts need to include the added fields.
Useful? React with 👍 / 👎.
| }; | ||
| context.cache.set(screenshotId, entry, "screenshot-scaling", CACHE_TTLS.SCREENSHOT_SCALING); | ||
| } | ||
| return { ...result, deviceId, app, screenshotId }; |
There was a problem hiding this comment.
This now sends the foreground package/activity in screenshot responses (and the PR adds analogous app fields to query/find), which changes what device data flows to the AI assistant. The repo's AGENTS.md says PRIVACY.md must be reviewed/updated when data flows to the assistant change, but this PR does not touch it; please update the policy/effective date or explicitly document why the existing text already covers this new top-level field.
Useful? React with 👍 / 👎.
| const screenshotId = context.cache.generateId("screenshot"); | ||
| if ( | ||
| result.scaleFactor !== undefined && | ||
| result.device !== undefined && | ||
| result.image !== undefined | ||
| ) { | ||
| const entry: ScreenshotScalingEntry = { | ||
| scaleFactor: result.scaleFactor, | ||
| deviceWidth: result.device.width, | ||
| deviceHeight: result.device.height, | ||
| imageWidth: result.image.width, | ||
| imageHeight: result.image.height, | ||
| }; | ||
| context.cache.set(screenshotId, entry, "screenshot-scaling", CACHE_TTLS.SCREENSHOT_SCALING); | ||
| } | ||
| return { ...result, deviceId, app, screenshotId }; |
There was a problem hiding this comment.
screenshotId is unconditionally included in every screenshot response, but the cache entry is only populated when scaleFactor, device, and image are all present. ScreenshotResult types all three as optional (scaleFactor?: number, device?: { ... }, image?: { ... }), so any backend implementation that omits them — a real risk for alternative adapters, partial mocks, or future extension points — returns a screenshotId that immediately throws UNKNOWN_SCREENSHOT_ID on use. The caller has no way to distinguish a usable screenshotId from an unusable one without attempting the tap.
| const screenshotId = context.cache.generateId("screenshot"); | |
| if ( | |
| result.scaleFactor !== undefined && | |
| result.device !== undefined && | |
| result.image !== undefined | |
| ) { | |
| const entry: ScreenshotScalingEntry = { | |
| scaleFactor: result.scaleFactor, | |
| deviceWidth: result.device.width, | |
| deviceHeight: result.device.height, | |
| imageWidth: result.image.width, | |
| imageHeight: result.image.height, | |
| }; | |
| context.cache.set(screenshotId, entry, "screenshot-scaling", CACHE_TTLS.SCREENSHOT_SCALING); | |
| } | |
| return { ...result, deviceId, app, screenshotId }; | |
| let screenshotId: string | undefined; | |
| if ( | |
| result.scaleFactor !== undefined && | |
| result.device !== undefined && | |
| result.image !== undefined | |
| ) { | |
| screenshotId = context.cache.generateId("screenshot"); | |
| const entry: ScreenshotScalingEntry = { | |
| scaleFactor: result.scaleFactor, | |
| deviceWidth: result.device.width, | |
| deviceHeight: result.device.height, | |
| imageWidth: result.image.width, | |
| imageHeight: result.image.height, | |
| }; | |
| context.cache.set(screenshotId, entry, "screenshot-scaling", CACHE_TTLS.SCREENSHOT_SCALING); | |
| } | |
| return { ...result, deviceId, app, screenshotId }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/ui-capture.ts
Line: 85-100
Comment:
`screenshotId` is unconditionally included in every screenshot response, but the cache entry is only populated when `scaleFactor`, `device`, and `image` are all present. `ScreenshotResult` types all three as optional (`scaleFactor?: number`, `device?: { ... }`, `image?: { ... }`), so any backend implementation that omits them — a real risk for alternative adapters, partial mocks, or future extension points — returns a `screenshotId` that immediately throws `UNKNOWN_SCREENSHOT_ID` on use. The caller has no way to distinguish a usable `screenshotId` from an unusable one without attempting the tap.
```suggestion
let screenshotId: string | undefined;
if (
result.scaleFactor !== undefined &&
result.device !== undefined &&
result.image !== undefined
) {
screenshotId = context.cache.generateId("screenshot");
const entry: ScreenshotScalingEntry = {
scaleFactor: result.scaleFactor,
deviceWidth: result.device.width,
deviceHeight: result.device.height,
imageWidth: result.image.width,
imageHeight: result.image.height,
};
context.cache.set(screenshotId, entry, "screenshot-scaling", CACHE_TTLS.SCREENSHOT_SCALING);
}
return { ...result, deviceId, app, screenshotId };
```
How can I resolve this? If you propose a fix, please make it concise.…ets (CU-4 follow-up #2) The first CU-4 follow-up scaled bonuses 100x to beat tiny non-clickable peers, but Codex pointed out that the linear -area term still demoted common full-width row targets (~1080x120 ≈ 130k px²) below their non-clickable children: row = 100000-129600-10000 = -39600 vs label -12000. Switching the area penalty to sqrt(area) * 100 flattens the curve so it grows from ~6k @ 4000 px² to ~36k @ 130k px² to ~158k @ 2.5M px² — keeping the 100k clickable bonus dominant across the typical-target range while still demoting full-screen containers (where sqrt(2.5M) ≈ 1581 contributes 158k of penalty on top of the bonus). Also drops the now-redundant ROOT_PENALTY / maxAreaInSet gate — sqrt on a 2.5M-px² element already produces a heavier penalty than the old linear+ROOT_PENALTY combo for row-and-larger candidates, and the relative-to-set gate was already known to misfire for small sets. Co-Authored-By: Claude <noreply@anthropic.com>
…7 follow-up) Greptile flagged that handleScreenshot unconditionally returned a screenshotId while only populating the cache entry when scaleFactor / device / image were all present. Since ScreenshotResult types all three as optional, an adapter or partial mock that omits them handed callers a screenshotId that would throw UNKNOWN_SCREENSHOT_ID on first use. Move the `generateId` call inside the conditional and make the response field optional, matching the actual cache-population contract. Co-Authored-By: Claude <noreply@anthropic.com>
… follow-up) Codex pointed out that handleDump / handleFind / handleScreenshot added an `app` field to their responses but the corresponding Zod output schemas didn't declare it. With additionalProperties: false in the generated contract, any consumer validating responses would reject valid runtime payloads from these paths. Add an optional, nullable AppFieldSchema (packageName / activityName, mirroring CurrentAppField with the null returned by getCurrentAppSafe on adb hiccups) to UiDumpFullOutput, UiDumpCompactOutput, and UiFindOutput; regenerate the contract + token snapshot. UiTapOutput / UiInputOutput / UiScrollOutput don't surface app at runtime — ui-action handlers don't call getCurrentAppSafe — so they stay unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
CU-1 added a foreground package/activity field to ui-query / ui-find / ui-capture responses. AGENTS.md (CLAUDE.md) requires PRIVACY.md to be updated when data flows to the AI assistant change. The "UI hierarchy" bullet covered this implicitly; calling out the new top-level field explicitly + bumping the effective date is the right hygiene. Co-Authored-By: Claude <noreply@anthropic.com>
|
@greptileai @codex review — addressed both findings on this PR:
Also picked up two CU-1 hygiene items Codex called out:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccd98665fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const entry: ScreenshotScalingEntry = { | ||
| scaleFactor: result.scaleFactor, | ||
| deviceWidth: result.device.width, | ||
| deviceHeight: result.device.height, | ||
| imageWidth: result.image.width, | ||
| imageHeight: result.image.height, | ||
| }; |
There was a problem hiding this comment.
This cache entry only records scaling metadata. Because ui-action resolves a screenshotId against whichever device is currently selected and there is no cached deviceId to compare, a user can capture on emulator-5554, select emulator-5556 within the 5-minute TTL, and then tap with the old screenshot id; the converted coordinates will be sent to the wrong device. Store the capturing deviceId in the entry and reject mismatches before tapping.
Useful? React with 👍 / 👎.
| if (pickedExtras?.pickedRationale) response.pickedRationale = pickedExtras.pickedRationale; | ||
| if (pickedExtras?.alternatives) response.alternatives = pickedExtras.alternatives; | ||
| if (viaScreenshotId) { | ||
| response.viaScreenshotId = input.screenshotId; | ||
| response.imageCoords = { x: input.imageX, y: input.imageY }; |
There was a problem hiding this comment.
These new success fields are returned for rank: "bestTappable" and screenshotId tap paths, but UiTapOutput/the generated contract still only declares tapped, deviceId, and matchedSelector with additionalProperties: false. Contract-validating clients generated from docs/contracts/replicant-mcp.contract.json will reject or drop successful responses from the new paths; extend the tap output schema (including matchedSelector.rank) and regenerate the contract.
Useful? React with 👍 / 👎.
|
@greptileai @codex review — addressed the P1 from your last pass:
Deferring the two P2s for a follow-up PR (screenshotId device-scoping + UiTapOutput schema fields) so this stack can ship; both are isolated to their own surfaces. |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
First half of the COMPUTER-USE initiative under THE-104. Six tickets, six commits, all from a single feature branch per workflow decision on 2026-05-15.
Driven by the 2026-05-14 agent evaluation that exercised `replicant-mcp@1.6.7` against real Android device automation (not app development). Each ticket fixes a concrete pain point that the evaluation surfaced.
The remaining tickets (CU-2, CU-8, CU-9) are being handled by a parallel orchestrator subagent track and will land as PR #2. CU-10..CU-13 (Wave D — research / discussion) are deferred to a later cycle.
Included tickets
Each commit is conventionally-named and self-contained; PR can be rebased into individual merges if reviewer prefers.
CU-1 (fix): visual-snapshot pipe — `THE-105`
`getCurrentApp` was building shell payloads like `dumpsys activity activities | grep mResumedActivity`. The `|` was correctly rejected by `validateShellPayload`. Fix: drop the pipes, parse multi-line dumpsys output in TypeScript. New parser module `src/parsers/dumpsys-current-app.ts` keeps the logic unit-testable in isolation. Integration test wires the real `ProcessRunner` through so any future regression that re-introduces shell metacharacters fails at the same layer where prod failed today.
CU-3 (feat): foreground app field — `THE-107`
`ui-query dump`, `ui-query find`, and `ui-capture screenshot` all now return a top-level `app: { packageName, activityName } | null` field. New shared helper `getCurrentAppSafe` swallows transient lookup failures so the parent op never dies on an adb hiccup. `visualSnapshot` already had this — this PR brings the other three surfaces to parity.
CU-4 (feat): bestTappable ranking — `THE-108`
Opt-in `selector.rank: "bestTappable"` on both `ui-query find` and `ui-action`. Heuristics: strong bonus for clickable, medium for long-clickable, small for focusable, area penalty, heavy penalty for full-screen root containers. `ui-action tap` with rank + multiple matches auto-picks the top candidate (instead of throwing AMBIGUOUS_MATCH) and returns `pickedRationale` + `alternatives`. Default behavior preserved — the AMBIGUOUS_MATCH error suggestion now mentions `rank: "bestTappable"` as a disambiguation option.
CU-5 (feat): interactiveOnly filter — `THE-109`
`ui-query find` and `ui-query dump` accept `interactiveOnly: boolean` (default false). When true, retains only nodes where any of `clickable / long-clickable / focusable / editable / scrollable` is true. Parser is extended to capture `long-clickable` and `editable` (previously only clickable/focusable/scrollable were extracted). New `isInteractiveNode` helper keeps the rule canonical. Filter applies after selector match, before pagination; OCR/grid results pass through unchanged.
CU-6 (docs): scroll direction clarity — `THE-110`
Schema describe text on `direction` and the `ui-action` tool description now state explicitly that direction is gesture-named, not content-named: `down` = swipe up = content scrolls down. Decision (docs-only, no alias values) recorded in DECISIONS.md.
CU-7 (feat): screenshotId-tied image-space tap — `THE-111`
`ui-capture screenshot` returns a stable `screenshotId`. The screenshot's scaling triple is pinned in CacheManager under a new `SCREENSHOT_SCALING` TTL (5 min). `ui-action tap` accepts `{ screenshotId, imageX, imageY }` and converts to device-space using THAT screenshot's scaling — not whatever global adapter `scalingState` happens to be in effect. Unknown / expired id surfaces as a new `UNKNOWN_SCREENSHOT_ID` error code with a "take a new screenshot — entries last 5 min" remediation.
DECISIONS.md note: the inline DECISIONS.md entry for the 5-minute TTL was blocked by the auto-mode classifier mid-commit; the rationale is otherwise:
Happy to add the entry as a separate commit on this PR if reviewer prefers.
Out of scope on this PR (handled separately)
Test plan
Notes for reviewers
Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com