feat(cua-driver): consolidate type_text; remove get_accessibility_tree - #1415
Closed
f-trycua wants to merge 4 commits into
Closed
feat(cua-driver): consolidate type_text; remove get_accessibility_tree#1415f-trycua wants to merge 4 commits into
f-trycua wants to merge 4 commits into
Conversation
…base64 by default get_window_state now accepts screenshot_out_file (optional absolute path). When set, screenshot bytes are written to that file, the MCP image content block is omitted, and screenshot_file_path is returned in structuredContent instead — callers get the AX tree without paying the ~31 KB base64-in-context token cost. CLI change: removed mergeImageContentIntoJSON — stdout no longer splices screenshot_png_b64 into the JSON by default. Use --image-out <path> or the new screenshot_out_file param to get the image on disk. This fixes context saturation in agents that call get_window_state via CLI subprocess (OpenCode with local Ollama models being the motivating case). Docs: integrations.mdx OpenCode section now warns about the CLI fallback problem and shows the modalities config fix for local vision models. SKILL.md updated to remove the legacy jq .screenshot_png_b64 path and document screenshot_out_file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… consistency Aligns the CLI flag name with the tool param name (screenshot_out_file) so both interfaces use the same terminology. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_file - GetWindowStateTool: only set resolvedScreenshotFilePath when write actually succeeds; fall through to inline image block on I/O error - AppState: fix doc comment — saved file is JPEG not PNG - SKILL.md: remove stale has_screenshot entry; add bash fence language - integrations.mdx: update callout to reflect actual CLI behavior after mergeImageContentIntoJSON removal Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
type_text now auto-falls back to CGEvent character synthesis when the AX write is rejected — covers Chromium/Electron inputs without requiring the caller to choose between two tools. Adds delay_ms param (default 30ms, used only in the CGEvent path). TypeTextCharsTool deleted; all references updated to type_text throughout docs, skills, and tests. get_accessibility_tree removed — its functionality (running apps + window list) is fully covered by list_apps and list_windows. Tool file deleted, removed from ToolRegistry, stale doc references updated to list_windows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR consolidates text input tooling by removing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/docs/cua-driver/reference/mcp-tools.mdx (1)
8-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the tool count in the intro.
This page now enumerates 29 tools, so
28is stale after thetype_textconsolidation andget_accessibility_treeremoval.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/cua-driver/reference/mcp-tools.mdx` around lines 8 - 10, Update the introductory count from "28 MCP tools" to "29 MCP tools" in the `cua-driver` docs and ensure the same numeric reference is updated anywhere else on the page; specifically edit the sentence that currently reads "`cua-driver` exposes 28 MCP tools through a single stdio server (`cua-driver mcp`)" to use 29, and double-check references to the tool list (e.g., mentions of `type_text` consolidation and `get_accessibility_tree` removal) to ensure the count matches the enumerated tools.
🧹 Nitpick comments (1)
docs/content/docs/cua-driver/reference/cli-reference.mdx (1)
370-371: ⚡ Quick winMention
delay_msin thetype_textinventory entry for discoverability.The new fallback pacing control is a key part of the tool contract and worth surfacing in this summary line too.
Suggested doc tweak
-- `type_text` — insert text via `AXSelectedText` with automatic CGEvent fallback for Chromium/Electron inputs. Pid-scoped. +- `type_text` — insert text via `AXSelectedText` with automatic CGEvent fallback for Chromium/Electron inputs; supports `delay_ms` (0–200, default 30) for fallback pacing. Pid-scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/cua-driver/reference/cli-reference.mdx` around lines 370 - 371, Update the inventory summary line for the type_text entry to mention the new delay_ms parameter for discoverability: locate the `type_text` entry in the CLI reference summary (the line that currently reads "`type_text` — insert text via `AXSelectedText` with automatic CGEvent fallback for Chromium/Electron inputs. Pid-scoped.") and append or integrate "supports delay_ms for pacing/fallback control" (or equivalent short phrasing) so the summary includes the `delay_ms` symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/cua-driver/Skills/cua-driver/SKILL.md`:
- Around line 444-446: Update the SKILL.md example so it only claims
--screenshot-out-file works for screenshot-bearing capture modes: change or
annotate the `cua-driver get_window_state '{"pid":N,"window_id":W}'
--screenshot-out-file /tmp/shot.jpg` example to explicitly state it applies to
`som`/`vision` capture modes (or add a short note that `ax` snapshots are
tree-only and the flag is a no-op or emits a warning when no screenshot exists);
reference the CLI flag `--screenshot-out-file`, the command `cua-driver
get_window_state`, and the capture modes `ax`, `som`, `vision` in the updated
text so readers know the limitation.
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/TypeTextTool.swift`:
- Around line 90-91: The code currently reads delay_ms into delayMs and relies
on KeyboardInput.typeCharacters to clamp it, causing the response summary to
show the raw value; update TypeTextTool to clamp delay_ms once right after
reading it (e.g., compute clampedDelay = max(minDelay, min(maxDelay, delayMs)))
and pass clampedDelay into KeyboardInput.typeCharacters and into any
response/summary text so reported and actual behavior match; apply the same
change where delay_ms is read in the other block referenced (around the 154-160
area) so both uses use the clampedDelay variable.
---
Outside diff comments:
In `@docs/content/docs/cua-driver/reference/mcp-tools.mdx`:
- Around line 8-10: Update the introductory count from "28 MCP tools" to "29 MCP
tools" in the `cua-driver` docs and ensure the same numeric reference is updated
anywhere else on the page; specifically edit the sentence that currently reads
"`cua-driver` exposes 28 MCP tools through a single stdio server (`cua-driver
mcp`)" to use 29, and double-check references to the tool list (e.g., mentions
of `type_text` consolidation and `get_accessibility_tree` removal) to ensure the
count matches the enumerated tools.
---
Nitpick comments:
In `@docs/content/docs/cua-driver/reference/cli-reference.mdx`:
- Around line 370-371: Update the inventory summary line for the type_text entry
to mention the new delay_ms parameter for discoverability: locate the
`type_text` entry in the CLI reference summary (the line that currently reads
"`type_text` — insert text via `AXSelectedText` with automatic CGEvent fallback
for Chromium/Electron inputs. Pid-scoped.") and append or integrate "supports
delay_ms for pacing/fallback control" (or equivalent short phrasing) so the
summary includes the `delay_ms` symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 928e64b4-0c1b-4b5a-9dbd-65f7766a10e0
📒 Files selected for processing (16)
docs/content/docs/cua-driver/guide/getting-started/integrations.mdxdocs/content/docs/cua-driver/reference/cli-reference.mdxdocs/content/docs/cua-driver/reference/mcp-tools.mdxlibs/cua-driver/Skills/cua-driver/RECORDING.mdlibs/cua-driver/Skills/cua-driver/SKILL.mdlibs/cua-driver/Skills/cua-driver/TESTS.mdlibs/cua-driver/Skills/cua-driver/WEB_APPS.mdlibs/cua-driver/Sources/CuaDriverCLI/CallCommand.swiftlibs/cua-driver/Sources/CuaDriverCore/AppState/AppState.swiftlibs/cua-driver/Sources/CuaDriverCore/Capture/WindowCapture.swiftlibs/cua-driver/Sources/CuaDriverServer/ToolRegistry.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/GetAccessibilityTreeTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/GetWindowStateTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/ScreenshotTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/TypeTextCharsTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/TypeTextTool.swift
💤 Files with no reviewable changes (2)
- libs/cua-driver/Sources/CuaDriverServer/Tools/TypeTextCharsTool.swift
- libs/cua-driver/Sources/CuaDriverServer/Tools/GetAccessibilityTreeTool.swift
Comment on lines
+444
to
446
| # CLI --screenshot-out-file flag is equivalent and works for all capture modes | ||
| cua-driver get_window_state '{"pid":N,"window_id":W}' --screenshot-out-file /tmp/shot.jpg | ||
| ``` |
Contributor
There was a problem hiding this comment.
Scope the --screenshot-out-file example to screenshot-bearing modes.
ax snapshots are tree-only, so this flag cannot produce a file there. As written, the example reads as if --screenshot-out-file works for every capture mode; please limit it to som / vision, or note the no-op warning when no screenshot exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Skills/cua-driver/SKILL.md` around lines 444 - 446, Update
the SKILL.md example so it only claims --screenshot-out-file works for
screenshot-bearing capture modes: change or annotate the `cua-driver
get_window_state '{"pid":N,"window_id":W}' --screenshot-out-file /tmp/shot.jpg`
example to explicitly state it applies to `som`/`vision` capture modes (or add a
short note that `ax` snapshots are tree-only and the flag is a no-op or emits a
warning when no screenshot exists); reference the CLI flag
`--screenshot-out-file`, the command `cua-driver get_window_state`, and the
capture modes `ax`, `som`, `vision` in the updated text so readers know the
limitation.
Comment on lines
+90
to
91
| let delayMs = arguments?["delay_ms"]?.intValue ?? 30 | ||
| guard let pid = Int32(exactly: rawPid) else { |
Contributor
There was a problem hiding this comment.
Clamp delay_ms once before use so behavior and summary stay consistent.
KeyboardInput.typeCharacters clamps internally, but the response text reports the raw value. That can misreport the effective delay.
Suggested patch
- let delayMs = arguments?["delay_ms"]?.intValue ?? 30
+ let requestedDelayMs = arguments?["delay_ms"]?.intValue ?? 30
+ let delayMs = max(0, min(200, requestedDelayMs))Also applies to: 154-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/TypeTextTool.swift` around
lines 90 - 91, The code currently reads delay_ms into delayMs and relies on
KeyboardInput.typeCharacters to clamp it, causing the response summary to show
the raw value; update TypeTextTool to clamp delay_ms once right after reading it
(e.g., compute clampedDelay = max(minDelay, min(maxDelay, delayMs))) and pass
clampedDelay into KeyboardInput.typeCharacters and into any response/summary
text so reported and actual behavior match; apply the same change where delay_ms
is read in the other block referenced (around the 154-160 area) so both uses use
the clampedDelay variable.
Collaborator
Author
|
Superseded by #1416 (clean rebase off main). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
type_textconsolidationtype_text_charsis deleted.type_textnow auto-falls back to CGEvent character synthesis (CGEvent.postToPid) when the AX attribute write is rejected — covers Chromium/Electron inputs transparently.delay_msparam (0–200, default 30ms) controls inter-character delay in the CGEvent fallback path; ignored when the AX path succeeds.via AXorvia CGEvent (AX fallback)).get_accessibility_treeremovallist_appsandlist_windows.list_windows.Test plan
swift buildclean ✅cua-driver type_text '{"pid":N,"text":"hello"}'— succeeds on a Cocoa text field (AX path, summary saysvia AX)via CGEvent (AX fallback)cua-driver type_text '{"pid":N,"text":"hello","delay_ms":50}'— delay_ms respected in fallback pathcua-driver get_accessibility_tree→Unknown toolerror (removed)cua-driver list_appsandcua-driver list_windowsstill work as the canonical discovery tools🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
get_window_statenow supports file-based screenshot output viascreenshot_out_fileparametertype_texttool now includes keystroke fallback with optionaldelay_msparameter (0–200ms)Removed Tools
get_accessibility_treetooltype_text_charstool; functionality consolidated intotype_textCLI Changes
--image-outoption to--screenshot-out-fileDocumentation