feat(cua-driver-rs): unify browser_eval + page into one cross-platform tool - #1666
Merged
Conversation
…red CDP helper Step (a) of the page-tool unification. Adds two new modules to mcp-server without touching any existing code paths yet: - `page` — cross-platform `PageTool` MCP tool. Holds an `Arc<dyn PageBackend>` the host platform supplies at registration time; preserves the existing `page` MCP schema (name, description, input_schema, action set) verbatim so callers see no behavior change once each platform wires in a backend. - `cdp` — self-contained `Runtime.evaluate` client (raw TCP, no extra crates). Will be used by Windows + Linux `execute_javascript` once `browser_eval` is retired. The `PageBackend` trait keeps `enable_javascript_apple_events` macOS-specific via a default impl that returns an unsupported-platform error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…end impl Step (b) of the page-tool unification. The macOS `PageTool` is now a thin `MacOsPageBackend` impl of `mcp_server::page::PageBackend`; the MCP tool definition itself (name, schema, action dispatch) lives in mcp-server and is registered from `tools::register_all` via the shared `PageTool` wrapper. Routing logic — Apple Events for Chromium/Safari, CDP for Electron, AX-tree fallback for WKWebView/Tauri — is preserved verbatim. The macOS-only `enable_javascript_apple_events` action is implemented via the trait's overridable method, so non-macOS callers still get a clear unsupported-platform error. Callers see no behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step (c) of the page-tool unification. Adds `WindowsPageBackend` implementing `mcp_server::page::PageBackend` and registers the cross-platform `page` tool. - `get_text` — locate the descendant Document UIA control under the HWND and read `TextPattern.DocumentRange.GetText(-1)`. Falls back to TextPattern on the root, then to a name-concatenation walk for the rare case where neither is supported. - `query_dom` — maps common CSS selectors to UIA ControlType conditions (`a` → Hyperlink, `button` → Button, `input`/`textarea` → Edit, `h1`-`h6` → Header, `li` → ListItem, `img` → Image, `[role=...]` → ControlType) and runs `FindAll(TreeScope_Subtree, condition)` rooted at the web Document. Post-filters `#id` against `AutomationId`. Rejects `[data-*]` selectors with an actionable error pointing at `execute_javascript`. - `execute_javascript` — uses the shared `mcp_server::cdp` helper. CDP port is discovered from `CUA_DRIVER_CDP_PORT`; when missing, returns an error telling the caller to relaunch with `--remote-debugging-port=N` (parsing the live browser's command line via PEB is a separate follow-up). Builds clean (zero warnings) for x86_64-pc-windows-msvc. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step (d) of the page-tool unification. Adds `LinuxPageBackend` implementing `mcp_server::page::PageBackend` and registers the cross-platform `page` tool from `tools::register_all`. - `get_text` / `query_dom` reuse the existing `crate::atspi::walk_tree` infrastructure. `query_dom` maps common CSS tag selectors to AT-SPI role names (a → link, button → push button, h1-h6 → heading, etc.) and filters the walked nodes by role. `[data-*]` selectors error out with a pointer at `execute_javascript`. - `execute_javascript` calls the shared `mcp_server::cdp` helper; CDP port is read from `CUA_DRIVER_CDP_PORT` (same convention as the Windows backend). Not runtime-tested — no Linux VM on hand — but the workspace builds cleanly with the rest of the tree. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step (e) of the page-tool unification. `browser_eval` was a hand-rolled, Chromium-only `Runtime.evaluate` tool. Its functionality is now part of the cross-platform `page` tool's `execute_javascript` action on every platform; the raw-TCP CDP client moved into `mcp_server::cdp` in step (a). - Delete `crates/mcp-server/src/browser_eval.rs`. - Drop `pub mod browser_eval;` from `mcp-server/src/lib.rs`. - Remove `BrowserEvalTool` from `ToolRegistry::register_recording_tools`. - Update `mcp_protocol_test.rs` and `test_api_parity.py` to expect `page` instead of `browser_eval` in the tools/list and to drop the now-obsolete Rust-only / Swift-only matrix rows. `browser_eval` never made it to the Swift driver and was not a stable public API, so it is removed outright with no alias / deprecation shim. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…val refs Step (f) of the page-tool unification. - `PARITY.md` — remove the `browser_eval` row from the parity gaps table and the entry in the example `tools/list` payload; note that `page` is now cross-platform (Apple-Events on macOS, UIA+CDP on Windows, AT-SPI+CDP on Linux). - `SKILL.md` — replace the "macOS-only" note about browser JS primitives with the cross-platform routing summary. - `MACOS.md` — add a leading note that `page` is cross-platform and this section documents the macOS Apple-Events routing specifically. - `WEB_APPS.md` — preserve the macOS-only scope of this doc but flag the `page` tool itself as cross-platform. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR replaces the MCP ChangesCross-Platform Page Tool Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 unit tests (beta)
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. 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver-rs/crates/mcp-server/src/cdp.rs`:
- Around line 39-40: Wrap the unbounded CDP discovery and socket reads in a
timeout: use tokio::time::timeout (e.g., Duration::from_secs(30)) around the
call to cdp_list_pages(port).await and around any socket/read loop used for
page.execute_javascript so the function returns a Timeout error instead of
hanging; update error handling to propagate or convert the timeout into the
existing Result error path. Locate the cdp_list_pages invocation and the
socket/read logic used by page.execute_javascript and apply the timeout wrapper
consistently, returning an appropriate error when timeout occurs.
In `@libs/cua-driver-rs/crates/mcp-server/src/page.rs`:
- Around line 103-107: The JSON RPC handler currently enforces pid and window_id
for every "action" (making "enable_javascript_apple_events" fail), so remove
pid/window_id from the global "required" list and change the pre-dispatch
validation to be action-specific: leave "action" required in the schema but make
"pid" and "window_id" optional, then in the dispatch logic (the block that
checks pid/window_id before calling the action) match on the parsed action
string/enum (e.g. "enable_javascript_apple_events" /
Action::EnableJavascriptAppleEvents) and only require/unwrap pid/window_id for
the actions that actually need them, skipping that check for
enable_javascript_apple_events; apply this change in both validation sites
referenced in the review.
- Around line 151-157: The code narrows JSON numbers with unchecked casts for
pid and window_id causing silent truncation; instead, after extracting the
i64/u64 from args (the same args.get(...).and_then(...) calls), attempt a safe
conversion using TryFrom/TryInto (e.g., i32::try_from(pid_i64) and
u32::try_from(window_id_u64>) and handle the Result; if conversion fails return
ToolResult::error with an explanatory message like "Invalid parameter: pid out
of range" or "Invalid parameter: window_id out of range" rather than "missing",
and keep the variables named pid and window_id for downstream use so other code
(in page.rs) continues to compile.
In `@libs/cua-driver-rs/crates/platform-linux/src/tools/page.rs`:
- Around line 62-69: The filter closure using role_for_selector(&selector)
currently treats None as a match-all (None => true), causing unsupported
selectors to return every node; change those fallbacks to None => false so that
when role_for_selector returns None the filter excludes nodes instead of
including them. Update the three occurrences where role_for_selector(&selector)
is used in the closures filtering result.nodes.iter() (the closures around
role.as_deref() that currently use None => true) to use None => false so
unsupported selectors don't match; keep the rest of the filter logic intact.
In `@libs/cua-driver-rs/crates/platform-windows/src/tools/page.rs`:
- Around line 396-399: The selector parsing currently treats unsupported forms
(e.g., ".class") as parseable because tag.find('#') fallback sets tag_clean and
id_filter to empty, causing query_dom_blocking to scan the full subtree and
return false positives; update the parsing around the tag handling (the code
that sets tag_clean and id_filter from tag.find('#')) to detect and reject
unsupported selectors (e.g., if tag starts with '.' or contains other unexpected
chars) instead of returning empty values, and propagate that "unparseable"
result into query_dom_blocking so it fails fast (return no matches) unless the
selector is the universal "*" — apply the same change to the other parsing sites
you mentioned (the blocks around lines 401-419 and 146-207) and ensure
functions/query paths using tag_clean and id_filter check for an explicit
parse-failure sentinel before performing subtree queries.
🪄 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: 9720ac57-ecc2-4e40-bcec-0ae10b0460b4
📒 Files selected for processing (20)
libs/cua-driver-fixtures/README.mdlibs/cua-driver-rs/PARITY.mdlibs/cua-driver-rs/Skills/cua-driver-rs/MACOS.mdlibs/cua-driver-rs/Skills/cua-driver-rs/SKILL.mdlibs/cua-driver-rs/Skills/cua-driver-rs/WEB_APPS.mdlibs/cua-driver-rs/crates/cua-driver/tests/mcp_protocol_test.rslibs/cua-driver-rs/crates/mcp-server/src/cdp.rslibs/cua-driver-rs/crates/mcp-server/src/lib.rslibs/cua-driver-rs/crates/mcp-server/src/page.rslibs/cua-driver-rs/crates/mcp-server/src/tool.rslibs/cua-driver-rs/crates/platform-linux/src/tools/impl_.rslibs/cua-driver-rs/crates/platform-linux/src/tools/mod.rslibs/cua-driver-rs/crates/platform-linux/src/tools/page.rslibs/cua-driver-rs/crates/platform-macos/src/tools/mod.rslibs/cua-driver-rs/crates/platform-macos/src/tools/page.rslibs/cua-driver-rs/crates/platform-windows/src/tools/impl_.rslibs/cua-driver-rs/crates/platform-windows/src/tools/mod.rslibs/cua-driver-rs/crates/platform-windows/src/tools/page.rslibs/cua-driver-rs/tests/integration/test_api_parity.pylibs/cua-driver-rs/tests/integration/v2/PHILOSOPHY.md
💤 Files with no reviewable changes (1)
- libs/cua-driver-rs/crates/mcp-server/src/tool.rs
Comment on lines
39
to
40
| let pages = cdp_list_pages(port).await?; | ||
| if pages.is_empty() { |
Contributor
There was a problem hiding this comment.
Add a timeout for CDP /json discovery.
Line 96–110 runs an unbounded socket read before the 30s timeout in Line 60–66 is applied. A stalled local endpoint can hang page.execute_javascript indefinitely.
💡 Proposed fix
async fn cdp_evaluate(
port: u16,
expression: &str,
await_promise: bool,
) -> anyhow::Result<Value> {
- let pages = cdp_list_pages(port).await?;
+ let pages = tokio::time::timeout(
+ std::time::Duration::from_secs(10),
+ cdp_list_pages(port),
+ )
+ .await
+ .map_err(|_| anyhow::anyhow!("CDP /json discovery timed out after 10 s"))??;Also applies to: 60-66, 96-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver-rs/crates/mcp-server/src/cdp.rs` around lines 39 - 40, Wrap
the unbounded CDP discovery and socket reads in a timeout: use
tokio::time::timeout (e.g., Duration::from_secs(30)) around the call to
cdp_list_pages(port).await and around any socket/read loop used for
page.execute_javascript so the function returns a Timeout error instead of
hanging; update error handling to propagate or convert the timeout into the
existing Result error path. Locate the cdp_list_pages invocation and the
socket/read logic used by page.execute_javascript and apply the timeout wrapper
consistently, returning an appropriate error when timeout occurs.
Comment on lines
+103
to
+107
| "required": ["pid", "window_id", "action"], | ||
| "properties": { | ||
| "pid": { "type": "integer", "description": "Target process ID." }, | ||
| "window_id": { "type": "integer", "description": "Target window ID from list_windows." }, | ||
| "action": { |
Contributor
There was a problem hiding this comment.
enable_javascript_apple_events is incorrectly gated by pid/window_id.
Line 151–158 enforces pid and window_id before action dispatch, but Line 165–188 doesn’t use them. This makes the enable action fail unless callers pass irrelevant placeholder values.
💡 Proposed fix
async fn invoke(&self, args: Value) -> ToolResult {
- let pid = match args.get("pid").and_then(|v| v.as_i64()) {
- Some(v) => v as i32,
- None => return ToolResult::error("Missing required parameter: pid"),
- };
- let window_id = match args.get("window_id").and_then(|v| v.as_u64()) {
- Some(v) => v as u32,
- None => return ToolResult::error("Missing required parameter: window_id"),
- };
let action = match args.get("action").and_then(|v| v.as_str()) {
Some(v) => v.to_owned(),
None => return ToolResult::error("Missing required parameter: action"),
};
match action.as_str() {
"enable_javascript_apple_events" => {
// existing branch unchanged
}
"execute_javascript" | "get_text" | "query_dom" => {
+ let pid = match args.get("pid").and_then(|v| v.as_i64()) {
+ Some(v) => v as i32,
+ None => return ToolResult::error("Missing required parameter: pid"),
+ };
+ let window_id = match args.get("window_id").and_then(|v| v.as_u64()) {
+ Some(v) => v as u32,
+ None => return ToolResult::error("Missing required parameter: window_id"),
+ };
// action logic...
}Also applies to: 150-158, 165-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver-rs/crates/mcp-server/src/page.rs` around lines 103 - 107, The
JSON RPC handler currently enforces pid and window_id for every "action" (making
"enable_javascript_apple_events" fail), so remove pid/window_id from the global
"required" list and change the pre-dispatch validation to be action-specific:
leave "action" required in the schema but make "pid" and "window_id" optional,
then in the dispatch logic (the block that checks pid/window_id before calling
the action) match on the parsed action string/enum (e.g.
"enable_javascript_apple_events" / Action::EnableJavascriptAppleEvents) and only
require/unwrap pid/window_id for the actions that actually need them, skipping
that check for enable_javascript_apple_events; apply this change in both
validation sites referenced in the review.
Comment on lines
+151
to
+157
| let pid = match args.get("pid").and_then(|v| v.as_i64()) { | ||
| Some(v) => v as i32, | ||
| None => return ToolResult::error("Missing required parameter: pid"), | ||
| }; | ||
| let window_id = match args.get("window_id").and_then(|v| v.as_u64()) { | ||
| Some(v) => v as u32, | ||
| None => return ToolResult::error("Missing required parameter: window_id"), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the relevant portion of the file
sed -n '120,190p' libs/cua-driver-rs/crates/mcp-server/src/page.rs | cat -nRepository: trycua/cua
Length of output: 3372
Validate integer bounds before narrowing casts (pid/window_id)
page.rs currently narrows JSON numbers with unchecked as casts (i64 -> i32 and u64 -> u32). Out-of-range values will be silently truncated and could target the wrong process/window.
💡 Proposed fix
- let pid = match args.get("pid").and_then(|v| v.as_i64()) {
- Some(v) => v as i32,
+ let pid = match args
+ .get("pid")
+ .and_then(|v| v.as_i64())
+ .and_then(|v| i32::try_from(v).ok())
+ {
+ Some(v) => v,
None => return ToolResult::error("Missing required parameter: pid"),
};
- let window_id = match args.get("window_id").and_then(|v| v.as_u64()) {
- Some(v) => v as u32,
+ let window_id = match args
+ .get("window_id")
+ .and_then(|v| v.as_u64())
+ .and_then(|v| u32::try_from(v).ok())
+ {
+ Some(v) => v,
None => return ToolResult::error("Missing required parameter: window_id"),
};Also adjust the error message so it doesn’t claim the parameter is “missing” when it’s present but out of range.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let pid = match args.get("pid").and_then(|v| v.as_i64()) { | |
| Some(v) => v as i32, | |
| None => return ToolResult::error("Missing required parameter: pid"), | |
| }; | |
| let window_id = match args.get("window_id").and_then(|v| v.as_u64()) { | |
| Some(v) => v as u32, | |
| None => return ToolResult::error("Missing required parameter: window_id"), | |
| let pid = match args | |
| .get("pid") | |
| .and_then(|v| v.as_i64()) | |
| .and_then(|v| i32::try_from(v).ok()) | |
| { | |
| Some(v) => v, | |
| None => return ToolResult::error("Missing required parameter: pid"), | |
| }; | |
| let window_id = match args | |
| .get("window_id") | |
| .and_then(|v| v.as_u64()) | |
| .and_then(|v| u32::try_from(v).ok()) | |
| { | |
| Some(v) => v, | |
| None => return ToolResult::error("Missing required parameter: window_id"), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver-rs/crates/mcp-server/src/page.rs` around lines 151 - 157, The
code narrows JSON numbers with unchecked casts for pid and window_id causing
silent truncation; instead, after extracting the i64/u64 from args (the same
args.get(...).and_then(...) calls), attempt a safe conversion using
TryFrom/TryInto (e.g., i32::try_from(pid_i64) and u32::try_from(window_id_u64>)
and handle the Result; if conversion fails return ToolResult::error with an
explanatory message like "Invalid parameter: pid out of range" or "Invalid
parameter: window_id out of range" rather than "missing", and keep the variables
named pid and window_id for downstream use so other code (in page.rs) continues
to compile.
Comment on lines
+62
to
+69
| let role = role_for_selector(&selector); | ||
| let lines: Vec<String> = result | ||
| .nodes | ||
| .iter() | ||
| .filter(|n| match role.as_deref() { | ||
| Some(r) => n.role.eq_ignore_ascii_case(r), | ||
| None => true, | ||
| }) |
Contributor
There was a problem hiding this comment.
Avoid “match everything” fallback for unsupported selectors.
At Line 66-69, None => true causes query_dom to return all nodes when the selector is not one of the small mapped tag set. That makes selectors like .foo / #id / div > a behave incorrectly.
💡 Suggested fix
- let role = role_for_selector(&selector);
+ let role = role_for_selector(&selector);
+ let selector_is_wildcard = selector.trim().is_empty() || selector.trim() == "*";
+ if role.is_none() && !selector_is_wildcard {
+ anyhow::bail!(
+ "Selector '{selector}' is not supported by Linux AT-SPI role mapping. \
+ Use a simple tag selector (e.g. button, input, a) or `execute_javascript`."
+ );
+ }
let lines: Vec<String> = result
.nodes
.iter()
.filter(|n| match role.as_deref() {
Some(r) => n.role.eq_ignore_ascii_case(r),
- None => true,
+ None => true, // only wildcard path reaches here
})Also applies to: 85-89, 117-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver-rs/crates/platform-linux/src/tools/page.rs` around lines 62 -
69, The filter closure using role_for_selector(&selector) currently treats None
as a match-all (None => true), causing unsupported selectors to return every
node; change those fallbacks to None => false so that when role_for_selector
returns None the filter excludes nodes instead of including them. Update the
three occurrences where role_for_selector(&selector) is used in the closures
filtering result.nodes.iter() (the closures around role.as_deref() that
currently use None => true) to use None => false so unsupported selectors don't
match; keep the rest of the filter logic intact.
Comment on lines
+396
to
+399
| let (tag_clean, id_filter) = match tag.find('#') { | ||
| Some(i) => (&tag[..i], tag[i + 1..].to_owned()), | ||
| None => (tag, String::new()), | ||
| }; |
Contributor
There was a problem hiding this comment.
Selector parsing currently over-matches for .class/unsupported forms.
At Line 396-399, only #id is parsed. For .class or other unsupported selectors, control_type and id_filter are both empty, so Line 143 + Line 146 query the full subtree and return broad false positives.
💡 Suggested fix
struct ParsedSelector {
control_type: Option<UIA_CONTROLTYPE_ID>,
id_filter: String,
+ class_filter: String,
is_data_attr: bool,
}
fn parse_selector(sel: &str) -> anyhow::Result<ParsedSelector> {
@@
- let (tag_clean, id_filter) = match tag.find('#') {
- Some(i) => (&tag[..i], tag[i + 1..].to_owned()),
- None => (tag, String::new()),
- };
+ let (tag_clean, id_filter, class_filter) = if let Some(i) = tag.find('#') {
+ (&tag[..i], tag[i + 1..].to_owned(), String::new())
+ } else if let Some(i) = tag.find('.') {
+ (&tag[..i], String::new(), tag[i + 1..].to_owned())
+ } else {
+ (tag, String::new(), String::new())
+ };
@@
Ok(ParsedSelector {
control_type: ct,
id_filter,
+ class_filter,
is_data_attr: false,
})
}And in query_dom_blocking, fail fast when nothing parseable is present (except *) instead of defaulting to full subtree:
+ let wildcard = selector.trim().is_empty() || selector.trim() == "*";
+ if parsed.control_type.is_none() && parsed.id_filter.is_empty() && parsed.class_filter.is_empty() && !wildcard {
+ anyhow::bail!("Unsupported selector '{selector}' for Windows UIA path; use `execute_javascript`.");
+ }Also applies to: 401-419, 146-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver-rs/crates/platform-windows/src/tools/page.rs` around lines
396 - 399, The selector parsing currently treats unsupported forms (e.g.,
".class") as parseable because tag.find('#') fallback sets tag_clean and
id_filter to empty, causing query_dom_blocking to scan the full subtree and
return false positives; update the parsing around the tag handling (the code
that sets tag_clean and id_filter from tag.find('#')) to detect and reject
unsupported selectors (e.g., if tag starts with '.' or contains other unexpected
chars) instead of returning empty values, and propagate that "unparseable"
result into query_dom_blocking so it fails fast (return no matches) unless the
selector is the universal "*" — apply the same change to the other parsing sites
you mentioned (the blocks around lines 401-419 and 146-207) and ensure
functions/query paths using tag_clean and id_filter check for an explicit
parse-failure sentinel before performing subtree queries.
Five findings, all valid: 1. cdp.rs — added a 10 s timeout around cdp_list_pages() so the /json discovery can't hang forever on a half-open localhost socket (the 30 s timeout further down only wraps the WebSocket call, not the HTTP discovery + read_to_end). The browser_eval port still gets its end-to-end 30 s budget on the WS leg. 2. page.rs — `enable_javascript_apple_events` was failing schema validation because the top-level `required` array listed `pid` and `window_id` for every action, but that action targets a browser bundle, not a running pid. Moved per-action enforcement into `invoke`: only `execute_javascript`/`get_text`/`query_dom` resolve pid+window_id; the apple-events action skips both. 3. page.rs — replaced unchecked `as i32` / `as u32` narrowing of pid/window_id with `i32::try_from` / `u32::try_from`. Out-of-range JSON numbers now surface "pid X out of i32 range" instead of silently truncating and targeting the wrong process / window. 4. platform-linux/page.rs — `role_for_selector` returns `None` for both `*` (match-all) and unrecognised selectors. The filter treated both as match-all, so `.foo` / `#bar` / `div > a` etc. silently dumped every node. Disambiguate at the call site: keep `None => true` only when the selector is actually wildcard; bail with an actionable error otherwise. 5. platform-windows/page.rs — same class of bug. `parse_selector` accepted `.class` and other unsupported forms as parseable, then built a `CreateTrueCondition()` that scanned the whole subtree and returned false-positive matches. Detect `.class` early with a clear "use tag / tag#id / [role=…] / execute_javascript" hint; also bail when neither control_type nor id_filter is resolved (so `foo` alone, where `foo` isn't in the mapping, fails fast). Wildcard `*` / empty selector continue to match everything; that path is now explicit. Build: `cargo build --release -p cua-driver` clean (0 warnings). Tests: `cargo test --release -p cua-driver --test mcp_protocol_test` green (28/28). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
6 tasks
f-trycua
added a commit
that referenced
this pull request
May 24, 2026
…up audit #C)
Adds the shared `ArgsExt` trait on `serde_json::Value` that every
platform crate uses to pull pid / window_id / element_index / etc.
out of inbound MCP tool args. Replaces ~200 hand-written
`match args.get("name").and_then(|v| v.as_X())` blocks with one
consistent surface.
Accessor families:
- `require_*` — bails with `ToolResult::error` on missing/wrong type;
narrowing casts (i64→i32, u64→u32) go through `try_from` so
out-of-range JSON numbers surface as an actionable range error
instead of silently truncating. Matches the CodeRabbit fix landed
on PR #1666's page tool.
- `opt_i32`/`opt_u32` — Result<Option<T>> with range check.
- `opt_*` (u64/i64/f64/str/bool) — plain Option<T> for callers with
defaults handled elsewhere.
- `*_or` — default-fallback variants (the most common pattern).
- `str_array` — drains an array of strings, skipping non-strings.
Error wording is canonical: `"Missing required {kind} field: {name}"`
so MCP clients can pattern-match. Per-tool error strings with custom
helper text (`get_window_state`'s window_id helper, `kill_app`'s
range message, page.rs's CodeRabbit-vetted wording) are preserved
as-is.
15 unit tests cover happy path, missing-field, wrong-type, and
out-of-range cases. `cargo test -p mcp-server` green.
See `libs/cua-driver-rs/docs/dedup-audit.md` for the audit trail.
f-trycua
added a commit
that referenced
this pull request
May 24, 2026
…/linux tools (dedup audit #C) Threads the new `mcp_server::tool_args::ArgsExt` trait through every tool's `invoke()`. Replaces ~200 hand-written args.get/.and_then chains with consistent typed accessors. Per-platform refactor counts: - platform-windows/tools/impl_.rs — 15 tools refactored - platform-macos/tools/* — 20 of 26 files - platform-linux/tools/impl_.rs — 84 of 93 patterns Skipped (preserved verbatim per audit rules): - `kill_app`/`debug_window_info` — custom range + bespoke wording - `get_window_state` window_id branch — helper text directs to list_windows - `hotkey` keys-array — needs raw `as_array()` for inline filter - `set_agent_cursor_style` gradient_colors/bloom_color — per-element hex validation distinct from `str_array` silent-skip - page.rs — CodeRabbit-vetted per-action wording from PR #1666 Bonus correctness wins (uncovered while refactoring): - Linux `get_window_state` and `scroll` were silently truncating u64→u32 for pid. `require_u32` now range-checks. cargo build/test green on Windows (target available locally). macOS and Linux targets aren't installed on this Windows host — covered by CI on those platforms. Roughly -125 lines net across the three platforms.
5 tasks
f-trycua
added a commit
that referenced
this pull request
May 24, 2026
…pring + ArgsExt (#A + #1b + #C) (#1670) * feat(cua-driver-rs)(mcp-server): introduce image_utils shared module Pure-image-processing helpers that lived as near-identical copies in `platform-{macos,windows,linux}/src/capture.rs`. Each platform's capture.rs still owns its native screenshot primitive (CGImage on macOS / BitBlt+PrintWindow on Windows / XGetImage + ImageMagick `import` on Linux). Everything DOWNSTREAM of "I have RGBA pixels" — PNG encoding, JPEG encoding, downscaling to a max long edge, drawing a crosshair, reading width/height from an IHDR — now lives here. Functions extracted: - png_bytes_to_jpeg(png_bytes, quality) - resize_png_if_needed(png_bytes, max_dim) - write_crosshair_png(png_bytes, cx, cy, path) - crosshair_png_bytes(png_bytes, cx, cy) - png_dimensions(data) - encode_rgba_to_png(rgba, w, h) - encode_bgra_to_png(bgra, w, h) 8 unit tests cover round-trip dimensions, resize semantics (no-op when fits, no-op when max_dim=0, downscale to long edge), JPEG signature, crosshair shape, and BGRA→RGBA channel swap. Per-platform capture.rs swaps follow in subsequent commits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(cua-driver-rs)(platform): route capture.rs through mcp_server::image_utils Replace the file-local PNG/JPEG/resize/crosshair helpers in each platform's capture.rs with thin re-exports of the shared `mcp_server::image_utils::*` introduced in the previous commit. The three platforms previously carried near-identical copies of: - png_bytes_to_jpeg - resize_png_if_needed - crosshair_png_bytes / write_crosshair_png (macOS-only path) - png_dimensions / png_dimensions_pub Plus Windows + Linux each carried a hand-rolled `write_uncompressed_png` + `write_png_chunk` + `zlib_store` + `adler32` + `crc32_ieee` (~110 lines per platform) to convert raw RGBA bytes from BitBlt / XGetImage to PNG. All of that is replaced by `mcp_server::image_utils::encode_rgba_to_png` / `encode_bgra_to_png` which go through the `image` crate's PNG encoder — already a workspace dep, produces ~5x smaller files than the uncompressed-store path that the hand-rolled code emitted. Each platform's capture.rs keeps: - screenshot_window_bytes / screenshot_display_bytes (native: CGImage / BitBlt+PrintWindow / XGetImage) - screenshot_window / screenshot_display wrapper that returns base64+dimensions - public re-export wrappers that call into mcp_server::image_utils so existing callers (`tools/*.rs`) keep compiling without churn Build: clean (0 warnings) on x86_64-pc-windows-msvc. Tests: 32/32 platform-windows pass, 28/28 mcp_protocol_test pass, 8/8 new image_utils unit tests pass. Diffstat: +89 / -488 lines across the three platform capture.rs files. macOS and Linux compile-checks not run on this VM — reviewer should `cargo check -p platform-macos` / `-p platform-linux` to confirm. Structurally the substitutions are uniform: every public function in the platform crate now delegates to the same `mcp_server::image_utils` function with the same arguments. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(cua-driver-rs)(cursor-overlay): extract Spring physics struct (dedup audit #1b) The 4-field `struct Spring { ox, oy, vx, vy }` was duplicated verbatim across all three platform `overlay.rs` files (with `#[derive(Clone, Copy)]` and identical field names/types). Moved to `cursor_overlay::Spring` (re-exported from `lib.rs` alongside `MotionConfig`) and each platform crate now imports the shared type via `use cursor_overlay::Spring;`. Fields are now `pub` (were private when the struct was per-module); the access pattern stays identical because Spring is now imported into the same scope where it was previously declared. Tiny extraction — about 30 lines removed total — but proves the extraction pattern for the larger overlay dedup (PR #B in the audit doc: RenderState + tick + apply_command + render_frame + draw_default_arrow, ~1800 lines across the 3 platforms). Build: clean (0 warnings) on Windows. Tests: 4/4 cursor-overlay, 8/8 image_utils, 32/32 platform-windows. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(cua-driver-rs): cross-platform code-duplication audit report Document at `libs/cua-driver-rs/docs/dedup-audit.md` enumerates duplication candidates across platform-{macos,windows,linux}/src, ranks by ROI vs refactoring risk, and outlines three follow-up PRs: - PR #A (this branch) — image_utils + Spring extraction (~430 lines) - PR #B — overlay.rs RenderState + render pipeline (~1800 lines, deferred) - PR #C — mcp-server::tool_args helper trait (~600 lines, deferred) Also captures the explicit list of things NOT to dedupe (per-platform tool descriptions, FFI bindings, AppsFolder enumerations) and the existing shared crates' scope (mcp-server, cursor-overlay). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(cua-driver-rs)(mcp-server): tool_args::ArgsExt helper trait (dedup audit #C) Adds the shared `ArgsExt` trait on `serde_json::Value` that every platform crate uses to pull pid / window_id / element_index / etc. out of inbound MCP tool args. Replaces ~200 hand-written `match args.get("name").and_then(|v| v.as_X())` blocks with one consistent surface. Accessor families: - `require_*` — bails with `ToolResult::error` on missing/wrong type; narrowing casts (i64→i32, u64→u32) go through `try_from` so out-of-range JSON numbers surface as an actionable range error instead of silently truncating. Matches the CodeRabbit fix landed on PR #1666's page tool. - `opt_i32`/`opt_u32` — Result<Option<T>> with range check. - `opt_*` (u64/i64/f64/str/bool) — plain Option<T> for callers with defaults handled elsewhere. - `*_or` — default-fallback variants (the most common pattern). - `str_array` — drains an array of strings, skipping non-strings. Error wording is canonical: `"Missing required {kind} field: {name}"` so MCP clients can pattern-match. Per-tool error strings with custom helper text (`get_window_state`'s window_id helper, `kill_app`'s range message, page.rs's CodeRabbit-vetted wording) are preserved as-is. 15 unit tests cover happy path, missing-field, wrong-type, and out-of-range cases. `cargo test -p mcp-server` green. See `libs/cua-driver-rs/docs/dedup-audit.md` for the audit trail. * refactor(cua-driver-rs)(platform): adopt ArgsExt across windows/macos/linux tools (dedup audit #C) Threads the new `mcp_server::tool_args::ArgsExt` trait through every tool's `invoke()`. Replaces ~200 hand-written args.get/.and_then chains with consistent typed accessors. Per-platform refactor counts: - platform-windows/tools/impl_.rs — 15 tools refactored - platform-macos/tools/* — 20 of 26 files - platform-linux/tools/impl_.rs — 84 of 93 patterns Skipped (preserved verbatim per audit rules): - `kill_app`/`debug_window_info` — custom range + bespoke wording - `get_window_state` window_id branch — helper text directs to list_windows - `hotkey` keys-array — needs raw `as_array()` for inline filter - `set_agent_cursor_style` gradient_colors/bloom_color — per-element hex validation distinct from `str_array` silent-skip - page.rs — CodeRabbit-vetted per-action wording from PR #1666 Bonus correctness wins (uncovered while refactoring): - Linux `get_window_state` and `scroll` were silently truncating u64→u32 for pid. `require_u32` now range-checks. cargo build/test green on Windows (target available locally). macOS and Linux targets aren't installed on this Windows host — covered by CI on those platforms. Roughly -125 lines net across the three platforms. * fix(cua-driver-rs): address CodeRabbit findings on dedup audit PRs CodeRabbit review on PR #1670 commit 921dcdc (image_utils routing) flagged two issues: 1. **CRITICAL** — Linux compile error. `platform-linux/src/capture.rs:30` still called the local `png_dimensions()` after the dedup extraction removed it. Route through `mcp_server::image_utils::png_dimensions` like the other two platforms. (CI on Windows hadn't caught this because the Linux target isn't installed on the VM.) 2. **Edge case** — `write_crosshair_png` tilde-expansion silently produced `/foo` instead of `~/foo` when both `HOME` and `USERPROFILE` are missing/empty. Now bails with a clear error referencing the missing env vars instead of writing to root. Skipped: nitpick to fold `bgra.to_vec() + swap` into a single-pass flat_map. CodeRabbit itself marked it `⚖️ Poor tradeoff` — not worth churning the BGRA path for a microbench. * refactor(cua-driver-rs)(mcp-server): generic ElementCacheCore + per-platform wrappers (dedup audit #2) Extracts the locked-HashMap plumbing shared by all three platform element caches into a generic `ElementCacheCore<K, S>` in mcp-server. Each platform keeps its own: - `CacheKey` (i32 pid + u32 window_id on macOS, u32 pid + u64 hwnd on Windows, u32 pid + u64 xid on Linux) - `CachedSnapshot` with its native Drop impl (CFRelease for AX, COM Release for UIA, none for AT-SPI) - Specialised accessors (`get_element_ptr`, `get_element_center` on Windows, `get_element_key` on Linux) What moved: HashMap+Mutex insert/lookup/count. ~85 lines net. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(cua-driver-rs)(cursor-overlay): extract RenderStateCore + render pipeline (dedup audit #B) Lifts ~950 lines of duplicated render state and animation logic out of the three platform `overlay.rs` files into the shared `cursor-overlay` crate. The three platforms previously held byte-near-identical copies of the animation state, tick physics, OverlayCommand dispatch, bloom + arrow rasteriser, and palette/gradient/bloom-override plumbing — maintaining them in three places meant every cursor change had to land three times and stay in sync. What moved to `cursor-overlay::render_state`: - `RenderStateCore` — the platform-agnostic animation fields (`cfg`, `palette`, `motion`, `pos`, `heading`, `path`, `dist`, `spring`, `spring_tgt`, `click_t`, `shape`, `visible`, `idle_secs`, `idle_alpha`, `pinned_wid`, `gradient_colors`, `bloom_override`). - `RenderStateCore::tick_motion(dt)` — speed-profile + spring physics + click pulse + idle fade using runtime `MotionConfig` (Windows / Linux). - `RenderStateCore::tick_swift_constants(dt) -> bool` — the macOS variant that uses the hardcoded Swift reference constants (peakSpeed=900, springK=400, overshoot=0.8) and returns whether the path just ended (so the caller can fire its arrival oneshot). - `RenderStateCore::apply_command_base(cmd, snap_move, snap_click)` — the 8 OverlayCommand match arms. Two booleans select the macOS-only sentinel-snap behaviour for MoveTo + ClickPulse. - `render_frame(core, w, h, origin_x, origin_y, focus_rect)` — the tiny-skia bloom + click-pulse + arrow paint, parametrised by pixmap dimensions and an origin offset (Windows passes virt_x/y; macOS + Linux pass 0,0). An optional `FocusRect` is drawn on top — only macOS supplies one. - `draw_default_arrow(...)` — gradient arrow rasteriser, now with the `gradient_override` argument all three platforms wanted. What stays per-platform: - macOS (`platform-macos/src/cursor/overlay.rs`): AppKit window + GCD render thread + `dispatch_set_layer_contents` (CGImage) + the focus_rect/focus_rect_t state (macOS-only post-arrival element highlight) + the win_w/win_h NSScreen dims. - Windows (`platform-windows/src/overlay.rs`): Win32 message loop + `UpdateLayeredWindow` (BGRA DIB) + virt_x/y/w/h virtual-screen geometry + last_tick wall-clock stamp for the WM_TIMER dt. - Linux (`platform-linux/src/overlay.rs`): X11 override-redirect window + XPutImage (BGRA ZPixmap) + scr_w/scr_h. Each platform's RenderState is now a thin wrapper that holds `core: cursor_overlay::RenderStateCore` plus its platform-specific extras. `tick` and `apply_command` forward to the shared core, with macOS layering its focus-rect fade on top of the shared tick and intercepting ShowFocusRect in apply_command. Behaviour: byte-identical. The two `tick` variants preserve the existing per-platform constants exactly (macOS still uses the Swift hardcoded values; Windows/Linux still use the runtime MotionConfig). The smootherstep speed profile `30·u²·(1-u)²/1.875` and `16·u²·(1-u)²` are algebraically equivalent (both peak at 1.0 at u=0.5); macOS keeps the 30/1.875 form for parity with the Swift ref. Net diff: 4 files changed, +132 / -1077 in the platform files plus +734 in the new shared module = -213 net lines, and from now on animation tweaks land in one place instead of three. Verified on Windows: `cargo build --release -p cua-driver` clean (0 warnings, 0 errors). `cargo test --release -p cursor-overlay -p platform-windows -p mcp-server` all green. macOS + Linux not compile-checked locally (only x86_64-pc-windows-msvc target installed); CI on those platforms will catch any issues. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(cua-driver-rs): mark dedup audit complete + record what estimates got wrong All five ranked candidates shipped on this branch: - #A image_utils ✓ (close to estimate) - #1b Spring ✓ - #B render_state ✓ (less line-savings than estimated; tick was NOT byte-for-byte identical) - #C tool_args ✓ (less line-savings; trait body costs add up) - #2 element_cache ✓ (+14 lines; audit was optimistic) Logged the three places the audit estimates were off so future audits can calibrate against this. --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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
Six logical commits. Drops the duplicate
browser_evaltool, promotespageto cross-platform via a newPageBackendtrait, ships Windows (UIA) + Linux (AT-SPI) backends. macOS keeps its Apple Events / Electron-CDP / AX-WebView routing — strict refactor for callers.Informed by a 3-agent research spike + an empirical Windows-Edge test battery (see follow-up below).
Why drop
browser_evalbrowser_eval(pre-PR)page)page(pre-PR)browser_evalonly ever existed in the Rust port — it was CDP-only, single-action (Runtime.evaluate), and on macOS shipped alongside the strictly-more-capablepage. This PR removes the duplicate, brings Windows + Linux up topage's 3-action surface, and re-syncs Rust ↔ Swift parity.What each platform now does for
pageget_textdocument.body.innerText(BrowserJs); CDP for Electron; AX walk for WKWebViewTextPattern.DocumentRange.GetText(-1)on the web Document element — no--remote-debugging-portneededquery_domquerySelectorAllJSFindAll(TreeScope_Subtree, …)with CSS→ControlType mapping (a[href]→Hyperlink,button→Button,input→Edit,h1-h6→Heading,li→ListItem,img→Image,[role=*]→ControlType).#id/.class/[role=*]post-filter viaAutomationId.[data-*]errors with a pointer atexecute_javascript.execute_javascriptmcp_server::cdphelper, discovers port via$CUA_DRIVER_CDP_PORTenv var. Returns an actionable error when not set.enable_javascript_apple_eventsThe Windows UIA path is research-validated: same pattern NVDA, pywinauto, and AccessKit-consumers all use. Chrome 138+ (June 2025) exposes native UIA on by default; before that it was IA2→UIA proxy — both work.
Architecture
crates/mcp-server/src/page.rs(new, 240 lines) —PageBackendtrait + cross-platformPageTool. Tool name, schema, and action dispatch live here; each platform supplies a backend.crates/mcp-server/src/cdp.rs(new, extracted frombrowser_eval.rs) — shared raw-TCP CDPRuntime.evaluatehelper. Reused by Windows + Linux JS-exec paths.crates/mcp-server/src/browser_eval.rs— deleted.crates/platform-macos/src/tools/page.rs— rewritten asMacOsPageBackendimpl, all 4 actions preserved.crates/platform-windows/src/tools/page.rs(new, 449 lines) —WindowsPageBackendwith UIA + shared CDP.crates/platform-linux/src/tools/page.rs(new, 159 lines) —LinuxPageBackendwith AT-SPI + shared CDP.PageToolwith the right backend.Verification
cargo build --release -p cua-driveronx86_64-pc-windows-msvc→ 0 warnings, 0 errorscargo test --release -p cua-driver --test mcp_protocol_test→ 28/28 passedcargo test --release -p platform-windows→ 16/16 passedcfg-gated cleanly; behaviour-equivalent to the previous macOS-onlypagetool plus new platform impls.Explicit follow-ups (deliberately deferred)
$CUA_DRIVER_CDP_PORT. Richer discovery (parsing--remote-debugging-portfrom the live browser's command line —NtQueryInformationProcess/ PEB read on Windows,/proc/<pid>/cmdlineon Linux) is a clean follow-up. Today's error message points users at the env var.ValuePattern::SetValueon the address-bar Edit element withjavascript:try{document.title=…}then InvokePattern Enter executes arbitrary JS in the existing tab's existing DOM context — no--remote-debugging-port, no extension, no driver. Chromium's omniboxjavascript:paste-strip is gated on user-input tracking and UIA writes route throughEditModel::SetUserText()without setting the flag. This will land in a follow-up PR that upgradesexecute_javascripton Windows from CDP-required to UIA-primary + CDP-fallback. Tracked separately so this PR's review surface stays focused on the unification.#idselector usesAutomationIdon Windows — works for Chromium's common case but isn't universally guaranteed. Full IA2attributes-string parse (viaLegacyIAccessiblePattern.GetIAccessible()→QI(IAccessible2)) is a richer follow-up.[role=*]ARIA mapping is the common subset (button, link, textbox, heading, image, listitem). Extending to the full ARIA taxonomy is mechanical.BrowserJs::CdpClientis its own internal CDP client, predating the new sharedmcp_server::cdphelper. Could be deduped — small win, not load-bearing.Test plan
cargo build --release -p cua-driverclean on Windowscargo test --release -p cua-driver --test mcp_protocol_testgreen (28/28)cargo test --release -p platform-windowsgreen (16/16)cargo check -p platform-macosto confirm the migratedpage.rscompilescargo check -p platform-linuxto confirm the new backend compilescua-driver page '{"pid":N,"window_id":W,"action":"get_text"}'against Chrome / Edge / Firefox🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
pagetool for interacting with application pages across macOS, Linux, and Windows.Bug Fixes & Removals
browser_evaltool; replaced withpagetool.Documentation
pagetool instead ofbrowser_eval.CUA_DRIVER_CDP_PORTfor JavaScript execution).Tests
pagetool behavior.