Windows Improvements: Multi-cursor Overlays, Background Input & DPI Awareness - #1836
Windows Improvements: Multi-cursor Overlays, Background Input & DPI Awareness#1836ddupont808 wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
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:
📝 WalkthroughWalkthroughAdds an embedded in-process driver with C/Swift bridges, session naming/labels, an MCP HTTP endpoint with state reporting, major Windows/Linux overlay and background input changes, native Linux AT-SPI, extensive Windows demos, Nix tests producing GIF artifacts, CI matrix updates, and documentation. ChangesEmbedded MCP and background input refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Linux visual regression artifactsMatrix jobs now run independently. Download GIF artifacts from this workflow run:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/content/docs/cua-driver/reference/mcp-tools.mdx (1)
572-572:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTool name mismatch in docs:
move_agent_cursorvsmove_cursor.Line 572 references
move_agent_cursor, but this reference page documentsmove_cursor. This will mislead users to a non-existent/incorrect command name.Proposed docs fix
- issue a pixel `click({pid,x,y})` or a `move_agent_cursor` first to put the cursor on-screen; + issue a pixel `click({pid,x,y})` or a `move_cursor` first to put the cursor on-screen;🤖 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 `@docs/content/docs/cua-driver/reference/mcp-tools.mdx` at line 572, The docs reference uses the wrong tool name: replace the incorrect `move_agent_cursor` mention with the documented command `move_cursor` so the text matches the API; update the sentence that currently reads `move_agent_cursor` to `move_cursor` (search for the token `move_agent_cursor` in the text around the visibility caveat) and ensure any adjacent examples or casing match the `move_cursor` symbol used elsewhere in the reference.libs/cua-driver/rust/crates/cua-driver/src/cli.rs (1)
1879-1944:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynchronize
cli_docs_json()with the new CLI surface.
parse_commandnow supportsname-sessionandserve --http-port, butcli_docs_json()does not describe either. That makesdump-docsoutput stale for downstream docs/tooling.Proposed change outline
{ "name": "serve", "abstract": "Run Cua Driver as a long-running daemon.", "discussion": "The daemon owns per-process state such as element-index caches, recording state, and cursor overlay state.", "arguments": no_args, "options": [ {"name":"socket","short_name":null,"help":"Override the daemon socket or named-pipe path.","type":"String","default_value":null,"is_optional":true}, - {"name":"pid-file","short_name":null,"help":"Override the pid-file path on Unix targets.","type":"String","default_value":null,"is_optional":true} + {"name":"pid-file","short_name":null,"help":"Override the pid-file path on Unix targets.","type":"String","default_value":null,"is_optional":true}, + {"name":"http-port","short_name":null,"help":"Enable MCP HTTP/JSON-RPC transport on 127.0.0.1:<port>.","type":"Number","default_value":null,"is_optional":true} ], "flags": [ {"name":"no-permissions-gate","short_name":null,"help":"Skip the macOS first-launch permissions gate.","default_value":false} ], "subcommands": no_subcommands }, + { + "name": "name-session", + "abstract": "Set a write-once friendly name for the default/anonymous session.", + "discussion": "Requires a running daemon; forwards a name_session tool call.", + "arguments": [{"name":"name","help":"Session name.","type":"String","is_optional":false}], + "options": [{"name":"socket","short_name":null,"help":"Override the daemon socket or named-pipe path.","type":"String","default_value":null,"is_optional":true}], + "flags": no_flags, + "subcommands": no_subcommands + },🤖 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/rust/crates/cua-driver/src/cli.rs` around lines 1879 - 1944, The cli_docs_json() output is missing the new CLI items added to parse_command: the name-session command and the serve subcommand's --http-port option; update the JSON structure built in cli_docs_json() to include a "name-session" command entry (matching the format used by "call"/"describe") and add an {"name":"http-port",...,"type":"String","is_optional":true} option under the "serve" command's options array, ensuring the option's help text and default/optional fields match how parse_command defines them so dump-docs reflects the current CLI surface.libs/cua-driver/rust/crates/cua-driver-core/src/session.rs (1)
103-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the idle-TTL entry in the direct teardown path.
fire_session_endis documented as a first-class teardown entrypoint, but it never removes the session fromSESSION_ACTIVITY. Any caller that ends a session through this branch keeps it counted as live until a later sweep and leaves stale per-session state behind unnecessarily.Suggested fix
pub fn fire_session_end(session_id: &str) { { let mut ended = ended_sessions().lock().unwrap(); if !ended.insert(session_id.to_owned()) { return; // already ended — idempotent no-op. } } + activity().lock().unwrap().remove(session_id); // Clear the write-once name store. The name registry is cross-platform core // state (Linux/Windows read it too), so cleanup belongs here, not only in // the macOS session-end hook. Guard "default" so the anonymous cursor's🤖 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/rust/crates/cua-driver-core/src/session.rs` around lines 103 - 121, fire_session_end currently never clears the session's idle-TTL entry in SESSION_ACTIVITY, leaving stale activity state; update fire_session_end to also remove the session from SESSION_ACTIVITY (use the same locking pattern as session_names) — e.g. call session_activity().lock().unwrap().remove(session_id) in the teardown path (apply the same "default" guard if you want to preserve the anonymous/default entry) so the idle-TTL is cleared when a session is ended.libs/cua-driver/rust/crates/platform-macos/src/ax/tree.rs (1)
217-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe truncation warning now reports the wrong failure mode.
truncatedis also set when the wall-clock deadline fires, but the appended message still always says the walk was "truncated at 2000 nodes." A timeout-driven partial tree will therefore blame the node cap even when it never got close toMAX_ELEMENTS.🤖 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/rust/crates/platform-macos/src/ax/tree.rs` around lines 217 - 223, The truncation warning always blames the node cap even when truncated_flag was set by a wall-clock deadline; change the logic around the tree_markdown.push_str call to pick the correct message: if truncated_flag AND the walked node count reached or exceeded MAX_ELEMENTS (use the same node counter variable used during traversal) append the existing "truncated at {MAX_ELEMENTS} nodes" message, otherwise append a timeout-specific message indicating the walk ended due to the deadline; reference truncated_flag, MAX_ELEMENTS, the node counter variable used in the walk, and the tree_markdown.push_str invocation to implement the conditional message selection.libs/cua-driver/rust/crates/cursor-overlay/src/lib.rs (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the SPDX header to clear the current CI warning.
This file is currently tripping the repository's SPDX header check.
🤖 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/rust/crates/cursor-overlay/src/lib.rs` at line 1, Add the SPDX license header to the top of the crate root (cursor-overlay's lib.rs) so the SPDX header check passes; insert a single-line comment like "// SPDX-License-Identifier: Apache-2.0" (or the project's canonical SPDX string, e.g. "MIT OR Apache-2.0") above the existing module doc comment so the crate-level comment and symbols in cursor-overlay remain unchanged.
🟠 Major comments (25)
.github/workflows/nix-build.yml-21-25 (1)
21-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope PR write permissions to the commenting job only.
pull-requests: writeat workflow scope gives write capability tonix-checks, which doesn’t need it. This widens blast radius unnecessarily.🔒 Suggested least-privilege update
permissions: id-token: write contents: read - pull-requests: write jobs: nix-checks: name: ${{ matrix.name }} @@ comment-linux-visual-artifacts: name: Comment Linux visual artifacts if: always() && github.event_name == 'pull_request' needs: [nix-checks] runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read steps: - name: Comment Linux visual artifacts on PR uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7Also applies to: 191-199
🤖 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 @.github/workflows/nix-build.yml around lines 21 - 25, The workflow-level permissions currently grant pull-requests: write under the top-level permissions: block; remove pull-requests: write from that global permissions section and instead add pull-requests: write to only the job that needs it (the commenting job) by setting permissions: { pull-requests: write } in that job definition, while keeping other jobs like nix-checks scoped to minimal rights (e.g., contents: read / id-token: write as required). Ensure you also remove any other global pull-requests: write entries (the other occurrence) and validate each job has only the least-privilege permissions it needs.nix/cua-driver/tests/linux-cursor-click-gif.nix-79-80 (1)
79-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle multi-line XID output robustly
Line 80 parses the whole file as one integer, but
xdotool search --pidmay return multiple window IDs. That can break this test unexpectedly.Suggested fix
- with open("/tmp/target-click-xid.txt", "r", encoding="utf-8") as f: - window_id = int(f.read().strip()) + with open("/tmp/target-click-xid.txt", "r", encoding="utf-8") as f: + window_id = int(next(line for line in f if line.strip()).strip())🤖 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 `@nix/cua-driver/tests/linux-cursor-click-gif.nix` around lines 79 - 80, The current code reads the entire file and calls int(...) on it, which fails when xdotool returns multiple XIDs; change the open(...) read logic to split the file into lines, strip and filter out empty lines, parse each into integers (e.g. build window_ids = [int(l) for l in lines if l.strip()]) and then use the appropriate one (for this test likely window_id = window_ids[0]) or iterate over window_ids as needed; update the place that uses window_id to handle a list if you choose to keep all IDs and optionally log or assert if multiple IDs were returned.nix/cua-driver/tests/linux-background-terminal-gif.nix-78-84 (1)
78-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse only one window ID from xdotool output
Line 79 and Line 83 assume the XID file contains exactly one line, but
xdotool search --pidcan emit multiple IDs.int(f.read().strip())will then fail and make this test flaky.Suggested fix
- with open("/tmp/background-target-xid.txt", "r", encoding="utf-8") as f: - target_window_id = int(f.read().strip()) + with open("/tmp/background-target-xid.txt", "r", encoding="utf-8") as f: + target_window_id = int(next(line for line in f if line.strip()).strip()) ... - with open("/tmp/background-control-xid.txt", "r", encoding="utf-8") as f: - control_window_id = int(f.read().strip()) + with open("/tmp/background-control-xid.txt", "r", encoding="utf-8") as f: + control_window_id = int(next(line for line in f if line.strip()).strip())🤖 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 `@nix/cua-driver/tests/linux-background-terminal-gif.nix` around lines 78 - 84, The file reads XID files with int(f.read().strip()) which fails if xdotool emitted multiple IDs; update the reads that populate target_window_id and control_window_id to parse only the first token/line (e.g., split the file content on whitespace or newlines and take the first element before converting to int) so the code that sets target_window_id and control_window_id tolerates multiple IDs from xdotool.demo/jukebox/orchestrator/src/main.rs-217-220 (1)
217-220:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid image-wide
taskkillfor cleanup.Line 217 kills every
cua-driver.exe/jukebox-app.exeon the machine, including unrelated user sessions. This can disrupt other active workflows.🤖 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 `@demo/jukebox/orchestrator/src/main.rs` around lines 217 - 220, The current cleanup loop uses Command::new("taskkill") over image names (img) which will kill all processes with those executable names system-wide; replace this with targeted termination of only the processes your orchestrator started: when spawning processes, keep their Child handles (or record their PIDs), then on shutdown call Child::kill() (or taskkill with "/PID" and the recorded PID) for each tracked child and await their exit; update the loop that currently references img and Command::new("taskkill") to iterate your stored children/PIDs, handle errors, and avoid killing by image name.demo/jukebox/orchestrator/src/main.rs-637-657 (1)
637-657:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOverlapping same-pitch notes on one channel are lost.
onstores only one active(channel, pitch)note. A secondNoteOnbefore the priorNoteOffoverwrites the first, so one note never gets emitted.💡 Suggested fix
- let mut on: std::collections::HashMap<(u8, u8), (f64, u8)> = std::collections::HashMap::new(); + let mut on: std::collections::HashMap<(u8, u8), Vec<(f64, u8)>> = std::collections::HashMap::new(); TrackEventKind::Midi { channel, message: MidiMessage::NoteOn { key, vel } } => { let ch = channel.as_int(); let pitch = key.as_int(); if vel.as_int() > 0 { - on.insert((ch, pitch), (now, vel.as_int())); + on.entry((ch, pitch)).or_default().push((now, vel.as_int())); if ch == 9 { drum_notes += 1; } } - else if let Some((t0, v)) = on.remove(&(ch, pitch)) { notes.push(Note { t: t0, pitch, vel: v }); dur = dur.max(now); } + else if let Some((t0, v)) = on.get_mut(&(ch, pitch)).and_then(|stack| stack.pop()) { + notes.push(Note { t: t0, pitch, vel: v }); + dur = dur.max(now); + } } TrackEventKind::Midi { channel, message: MidiMessage::NoteOff { key, .. } } => { - if let Some((t0, v)) = on.remove(&(channel.as_int(), key.as_int())) { notes.push(Note { t: t0, pitch: key.as_int(), vel: v }); dur = dur.max(now); } + if let Some((t0, v)) = on.get_mut(&(channel.as_int(), key.as_int())).and_then(|stack| stack.pop()) { + notes.push(Note { t: t0, pitch: key.as_int(), vel: v }); + dur = dur.max(now); + } }🤖 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 `@demo/jukebox/orchestrator/src/main.rs` around lines 637 - 657, The current HashMap on: HashMap<(u8,u8),(f64,u8)> loses earlier NoteOn when a same (channel,pitch) NoteOn arrives; change on to HashMap<(u8,u8), Vec<(f64,u8)>> and treat it as a stack/queue of active starts: in the TrackEventKind::Midi NoteOn branch push (now, vel) onto on[(ch,pitch)] (and increment drum_notes as before), and in the NoteOn with vel==0 or NoteOff branch pop the most-recent start from on[(ch,pitch)] and create the Note from that start (pushing into notes and updating dur); ensure you handle absent vectors safely (no-op) and update any code that removes entries to clear empty Vecs.libs/cua-driver/rust/crates/cua-driver/src/serve.rs-265-273 (1)
265-273:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMCP HTTP state-file path ignores custom pid-file configuration.
Line 266 always derives from
default_pid_file_path(), butrun_servesupports caller-providedpid_file_path. This violates the “lives beside the pid file” contract and can mix state across daemon instances with custom paths (including incorrectstatusoutput).Suggested direction
-pub fn mcp_http_url_file_path() -> String { - let pid = default_pid_file_path(); +pub fn mcp_http_url_file_path_from_pid(pid_file_path: &str) -> String { + let pid = pid_file_path.to_owned(); std::path::Path::new(&pid) .parent() .map(|d| d.join("cua-driver-mcp-http.url")) .unwrap_or_else(|| std::path::PathBuf::from("cua-driver-mcp-http.url")) .to_string_lossy() .into_owned() }Then thread the effective pid path through spawn/cleanup/status call sites instead of using a global default.
🤖 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/rust/crates/cua-driver/src/serve.rs` around lines 265 - 273, mcp_http_url_file_path currently always uses default_pid_file_path(), ignoring caller-supplied pid paths from run_serve and causing state-file collisions; update the code to accept (or be passed) the effective pid file path instead of calling default_pid_file_path() inside mcp_http_url_file_path, propagate that pid path through the spawn/cleanup/status call sites (the places that call mcp_http_url_file_path, spawn, cleanup, and status), and replace internal uses of default_pid_file_path() with the provided pid_file_path so the MCP HTTP URL file is created beside the actual pid file for each daemon instance.libs/cua-driver/rust/crates/platform-linux/src/tty.rs-71-73 (1)
71-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the PTY write path to avoid request-thread hangs.
write_allhere is fully blocking. If the target PTY isn’t draining, this call can stall indefinitely and tie up tool handling. Please switch to non-blocking writes with a bounded poll/timeout and treat timeout as a fallbackable miss (Ok(false)).🤖 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/rust/crates/platform-linux/src/tty.rs` around lines 71 - 73, The current blocking master.write_all(text.as_bytes()) can hang; change to set the underlying fd non-blocking (use fcntl F_GETFL/F_SETFL + O_NONBLOCK on local.into_raw_fd() or on the fd before creating master), then poll the fd with a bounded timeout (libc::poll or nix::poll) to wait for writability, and perform non-blocking writes using write()/write_all semantics in a loop that respects the same timeout; if poll times out or writes return WouldBlock/EAGAIN before all data is written, return Ok(false) as a fallbackable miss; finally restore the original fd flags and map other errors to the existing ? error path. Make sure to replace the direct call to master.write_all(text.as_bytes())? and reference the local variable master and the write_all site when implementing this change.libs/cua-driver/rust/crates/cua-driver/cua-driver.rc-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the SPDX header to this new resource file.
CI is already reporting a missing SPDX header at Line 1. Please add the repository’s standard SPDX header here to clear compliance warnings.
Proposed change
+// SPDX-License-Identifier: <REPO_STANDARD_IDENTIFIER> 1 RT_MANIFEST "cua-driver.manifest"🤖 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/rust/crates/cua-driver/cua-driver.rc` at line 1, The resource file cua-driver.rc is missing the repository's standard SPDX header; add the standard SPDX header comment at the very top of cua-driver.rc (before the RT_MANIFEST "cua-driver.manifest" line) exactly as used elsewhere in the repo (e.g., the repository’s standard "SPDX-License-Identifier: ..." header), ensuring it's a proper comment/encoding for .rc files and contains the exact text used across the project so CI stops reporting a missing SPDX header.libs/cua-driver/rust/crates/platform-linux/src/atspi/native.rs-358-390 (1)
358-390:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHonor the requested window when choosing the AT-SPI target.
This selector is process-wide:
collect_visited(pid)walks every top-level accessible owned by the PID, theninsert_textpicks the first focused/document/any editable it finds.type_textalready resolves a specificwindow_id, so multi-window apps can still receive text in the wrong window or tab. Thread the XID through this API and constrain candidate selection to that subtree before picking a target.🤖 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/rust/crates/platform-linux/src/atspi/native.rs` around lines 358 - 390, insert_text currently selects a target editable from the process-wide visited list returned by collect_visited(pid), which can pick fields in other windows; update the API and selection to honor a requested window XID: extend insert_text (and the caller/type_text) to accept an optional window_id/XID, make collect_visited or a new helper return/access the tree/subtree for that specific top-level accessible (use the XID to identify the top-level Accessible or node subtree), then constrain the visited iterator to nodes inside that subtree before applying the existing focused/in_web_doc/has_editable priority logic (symbols: insert_text, collect_visited, visited, type_text, window_id/XID, target). Ensure behavior falls back to the original process-wide logic when window_id is None.libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs-637-645 (1)
637-645:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe PTY lookup is positional, so it can hit the wrong terminal window.
terminal_tty_for_windowassumes XID-sorted windows line up with the PTYs discovered from the process tree, but those two sequences are unrelated. In a terminal with multiple windows or tabs,type_text/press_keycan inject into a different shell than the requestedwindow_id. This needs a real per-window association before PTY injection is safe here.🤖 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/rust/crates/platform-linux/src/tools/impl_.rs` around lines 637 - 645, terminal_tty_for_window currently assumes a positional mapping between sorted windows and terminal_descendant_ttys, which is wrong; change it to find the exact window by xid (using crate::x11::list_windows) and use that window's owner pid to resolve PTYs (call terminal_descendant_ttys with that pid) instead of indexing by sorted position. Locate terminal_tty_for_window and replace the windows.index-based lookup with: find the window struct where w.xid == xid, extract its pid (the window owner), then call terminal_descendant_ttys(owner_pid) and return the appropriate tty (e.g., the first match) so PTY selection is tied to the window owner process rather than list ordering.libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs-103-106 (1)
103-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor correctness:
type_into_editableoverwrites existing text (usessetTextContents)
Inlibs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs(lines 103-106),TypeTextTool’s “success” path callsEditableText.setTextContents(...), which replaces the entireEditableTextbuffer rather than inserting at the caret—makingtype_textdestructive for pre-filled widgets. Switch to caret-relative insertion (e.g., read the current caret offset and useEditableText.insertText(...)instead).🤖 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/rust/crates/platform-linux/src/atspi/mod.rs` around lines 103 - 106, The current success path uses EditableText.setTextContents(...) which replaces the whole buffer; instead, read the current caret offset from the editable (e.g., call the EditableText caret/offset getter on the object returned by queryEditableText(), such as getCaretOffset()/caret_offset()), then call EditableText.insertText(offset, safe_text) to insert at the caret; if the caret getter fails or returns None, fall back to appending or to setTextContents as a last resort. Update the code paths in TypeTextTool/type_into_editable (the block that uses queryEditableText() and et.setTextContents(...)) to use the caret-read + et.insertText(...) flow and handle errors from the caret/read/insert calls gracefully.libs/cua-driver/rust/crates/cua-driver-embedded/examples/macos-app-smoke/run.sh-32-35 (1)
32-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoded Xcode runtime path for
libswift_Concurrency.dylib.This path assumption can fail on valid macOS setups (non-default Xcode location/version), causing the smoke test to fail even when the build is otherwise correct.
Portable lookup suggestion
-SWIFT_RUNTIME="/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift-5.5/macosx" +XCODE_DEV_DIR="$(xcode-select -p)" +SWIFT_RUNTIME_CANDIDATES=( + "$XCODE_DEV_DIR/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx" + "$XCODE_DEV_DIR/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift-5.5/macosx" +) if otool -L "$APP/Contents/Frameworks/libcua_driver_embedded.dylib" | grep -q '`@rpath/libswift_Concurrency.dylib`'; then - cp "$SWIFT_RUNTIME/libswift_Concurrency.dylib" "$APP/Contents/Frameworks/" + found=0 + for d in "${SWIFT_RUNTIME_CANDIDATES[@]}"; do + if [[ -f "$d/libswift_Concurrency.dylib" ]]; then + cp "$d/libswift_Concurrency.dylib" "$APP/Contents/Frameworks/" + found=1 + break + fi + done + if [[ "$found" -ne 1 ]]; then + echo "libswift_Concurrency.dylib not found in expected toolchain paths" >&2 + exit 1 + fi fi🤖 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/rust/crates/cua-driver-embedded/examples/macos-app-smoke/run.sh` around lines 32 - 35, The script hardcodes SWIFT_RUNTIME which breaks on nonstandard Xcode locations; replace the fixed assignment with runtime detection: use xcrun --sdk macosx --show-sdk-path or xcode-select -p to derive candidate developer/toolchain paths, then search those locations for libswift_Concurrency.dylib (e.g. with find or globbing) and set SWIFT_RUNTIME to the directory that contains the found dylib; update the cp step that references SWIFT_RUNTIME (the SWIFT_RUNTIME variable and the cp "$SWIFT_RUNTIME/libswift_Concurrency.dylib" "$APP/Contents/Frameworks/") to use the discovered path and fail with a clear error if the dylib cannot be located.libs/cua-driver/rust/crates/cua-driver/build.rs-14-17 (1)
14-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix target gating in
build.rs: don’t use#[cfg(target_os = "windows")].
#[cfg(target_os = "windows")]inbuild.rsis evaluated for the build-script host, so cross-compiling to Windows can skipembed_resource::compile. Gate this with the Cargo-providedCARGO_CFG_TARGET_OS == "windows"instead.🤖 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/rust/crates/cua-driver/build.rs` around lines 14 - 17, The build script currently uses #[cfg(target_os = "windows")] around the embed_resource::compile call in build.rs which gates compilation by the build-host OS; change the gate to check the target OS via Cargo env var by using the Cargo-provided CARGO_CFG_TARGET_OS == "windows" (read from std::env::var("CARGO_CFG_TARGET_OS")) and only call embed_resource::compile("cua-driver.rc", embed_resource::NONE) when that value equals "windows"; update the conditional block around embed_resource::compile accordingly so cross-compiling to Windows still runs the resource embedding.libs/cua-driver/rust/crates/cua-driver/tests/e2e_windows_bg_input_test.rs-266-267 (1)
266-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid killing unrelated processes by image name.
setup()always callstaskkill /IM <basename>, so the Notepad case will terminate every usernotepad.exeinstance on the machine before the test starts. That can destroy unsaved work, and it is unnecessary for the Win32 baseline because there is no fixed app HTTP port to free there.🤖 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/rust/crates/cua-driver/tests/e2e_windows_bg_input_test.rs` around lines 266 - 267, The setup() helper currently calls kill_prior_by_name(target_exe) unconditionally which will terminate every process matching the image name (e.g., all notepad.exe), risking user data loss; change setup() to only kill prior instances when necessary (for apps that use a fixed HTTP port) by adding a guard (e.g., a boolean/enum or checking whether the app uses a fixed port) and replace the unconditional kill with a targeted approach: either skip kill_prior_by_name for Win32 baseline/Notepad, or use a safer alternative such as kill_prior_by_port or verifying the process command line/window title before killing; update references to kill_prior_by_name and target_exe accordingly so only intended test instances are terminated.libs/cua-driver/rust/crates/platform-linux/src/overlay.rs-59-87 (1)
59-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe first Linux
animate_cursor_to()is a no-op.A fresh cursor starts at
(-200, -200), but this path refuses to sendMoveTounlesspos.0 > -50.0. On a brand-new overlay there is no earlier command that can satisfy that guard, so the first animation request is dropped outright instead of moving the cursor on-screen.🤖 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/rust/crates/platform-linux/src/overlay.rs` around lines 59 - 87, The current animate_cursor_to() in overlay.rs ignores the first move because the should_animate guard uses RENDER and requires rs.core.pos.0 > -50.0; remove (or relax) that pos check so MoveTo is sent for the initial off-screen cursor. Concretely, update the should_animate computation (using RENDER.lock()) to base the decision only on rs.core.cfg.enabled and rs.core.visible (i.e., drop the rs.core.pos.0 > -50.0 condition) so that send_command(OverlayCommand::MoveTo { ... }) is called on the first request; keep the existing ARRIVAL_TX handling and rx.await logic unchanged.libs/cua-driver/rust/crates/cua-driver/tests/e2e_windows_bg_input_test.rs-401-419 (1)
401-419:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe no-foreground-steal oracle misses transient steals.
action()blocks until the JSON-RPC call returns, and the polling window only starts afterward. If the target grabs foreground briefly during the click/type and restores it before the response arrives, this assertion still passes. The sentinel loss counters are reset insetup()but never checked, so the test no longer verifies the stronger invariant described in the file header.🤖 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/rust/crates/cua-driver/tests/e2e_windows_bg_input_test.rs` around lines 401 - 419, The current assert_target_stays_background misses transient steals because it polls only after action() returns and never checks the sentinel loss counters; modify assert_target_stays_background (and/or its caller) to record the sentinel state (use read_count(&focus_pid_file()) and any other sentinel loss counters reset in setup()) before running action(), then perform the foreground-polling window concurrently with the action so transient steals during the RPC are observed (e.g., start the polling loop immediately in the main thread and run action() in a spawned thread, or spawn a watcher thread before calling action()); after both complete assert that no steal occurred and that the sentinel loss counters are unchanged compared to the pre-action snapshot.libs/cua-driver/rust/crates/platform-macos/src/cursor/overlay.rs-102-109 (1)
102-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNew cursors still use the collision-prone palette picker.
render_state_for_key()still callsPalette::for_instance(key), so two live sessions that hash to the same alternate palette will keep colliding even thoughPalette::for_instance_distinct()was added to avoid exactly that case. The distinct-color guarantee is not actually enforced on this insertion path.🤖 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/rust/crates/platform-macos/src/cursor/overlay.rs` around lines 102 - 109, render_state_for_key currently uses Palette::for_instance(key) which allows palette collisions; change it to call Palette::for_instance_distinct(key) so the created RenderState (in function render_state_for_key) gets a distinct palette for each live session. Update the line setting rs.core.palette to use for_instance_distinct and keep the rest of the function (cloning template, setting label) unchanged.libs/cua-driver/rust/crates/platform-macos/src/tools/cursor_tools.rs-39-41 (1)
39-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinish the anonymous-session behavior flip.
resolve_cursor_key()now returns"default"here, but the file-local tests still assertNO_CURSORfor anonymous calls on Lines 526-533 and 613-622. As written, this change leaves a deterministic test failure in the same file and theNO_CURSORdocs above stale.🤖 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/rust/crates/platform-macos/src/tools/cursor_tools.rs` around lines 39 - 41, The change in resolve_cursor_key() now returns the string "default" for anonymous sessions but the local tests and documentation still expect the old NO_CURSOR constant; update all tests that assert NO_CURSOR (tests asserting anonymous behavior and assertions referencing NO_CURSOR in this file) to expect "default" (or compare against resolve_cursor_key(None) result) and update the file-local doc comments that mention NO_CURSOR to reflect the new "default" semantics; specifically search for usages of NO_CURSOR in this module and in tests around the anonymous-session assertions and replace expectations/documentation with "default" or the resolved key from resolve_cursor_key.libs/cua-driver/rust/crates/platform-windows/src/input/inject.rs-305-318 (1)
305-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the target's original topmost state.
ZorderGuard::arm()promotes any non-foreground target into the topmost band, anddrop()always demotes it withHWND_NOTOPMOST. For an already-always-on-top app, one background click/drag permanently changes its window state after the gesture completes.🤖 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/rust/crates/platform-windows/src/input/inject.rs` around lines 305 - 318, ZorderGuard::arm currently raises any non-foreground target to topmost and drop() unconditionally demotes it; change arm to query and store the target window's original topmost state (use GetWindowLongPtr with GWL_EXSTYLE and test WS_EX_TOPMOST) and record that value on the ZorderGuard struct (e.g., add orig_topmost: bool), then only call set_topmost(target, true) if it was not already topmost and in drop() restore the original state by calling set_topmost(target, orig_topmost) (or HWND_TOPMOST/HWND_NOTOPMOST accordingly) so existing always-on-top windows are not permanently changed.libs/cua-driver/rust/crates/platform-windows/src/input/dispatch.rs-123-133 (1)
123-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWPF text input is still classified as background-safe.
This branch only flags pointer events, but the new WPF helper docs below and
inject_text_cloaked()both say postedWM_CHARis dropped by WPF text boxes. That leavesdispatch:"background"free to advertiseTextInputas available and route it down a silent no-op path.🤖 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/rust/crates/platform-windows/src/input/dispatch.rs` around lines 123 - 133, The WPF branch currently only flags pointer events but omits TextInput, allowing background-safe routing for text which WPF drops; update the branch in is_wpf_target_window handling to treat TextInput the same as pointer events (i.e., include TextInput alongside MouseClick, MouseMove, MouseScroll when matching on kind) so dispatch:"background" will not advertise text input and routed text will go through inject_text_cloaked() / the no-op path; ensure you reference the kind enum variant TextInput in the matches! expression used in this return.libs/cua-driver/rust/crates/platform-windows/src/input/inject.rs-629-644 (1)
629-644:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbort the WPF drag path when the foreground raise is rejected.
The comments here say WPF only processes injected stylus while active, but the code ignores
force_foreground_hard()'s return value and injects anyway. If the raise fails, this can report success after a drag WPF never handled.Suggested fix
- let _lock = unsafe { ForegroundLockGuard::disable() }; - unsafe { force_foreground_hard(target_h); } + let _lock = unsafe { ForegroundLockGuard::disable() }; + if unsafe { !force_foreground_hard(target_h) } { + bail!("background drag for WPF requires a temporary foreground raise"); + } let r = stroke(());🤖 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/rust/crates/platform-windows/src/input/inject.rs` around lines 629 - 644, The current WPF path ignores force_foreground_hard(target_h) return value and proceeds to call stroke() even if the foreground raise failed; update the block in inject.rs (around is_wpf_target_window, ForegroundLockGuard::disable, force_foreground_hard, stroke, prev_fg, target_h) to check the boolean result of unsafe { force_foreground_hard(target_h) } and if it returns false, immediately restore the previous foreground (if prev_fg is non-null and different from target_h) and return an error/early failure instead of calling stroke(), ensuring the ForegroundLockGuard is dropped in either case; keep the existing restoration logic for prev_fg when the raise succeeds before calling stroke().libs/cua-driver/rust/crates/platform-windows/src/input/inject.rs-742-746 (1)
742-746:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat posted text as an equivalent fallback here.
When
force_foreground_attached()fails, this falls back topost_type_text(), but the new WPF path in this PR explicitly documents that postedWM_CHARcan be silently dropped. That meansinject_text_cloaked()can still returnOk(())after typing nothing on the exact targets this helper was added for.Suggested fix
- let result = if got_fg { - unsafe { send_unicode(text) } - } else { - crate::input::post_type_text(target, text) - }; + let result = if got_fg { + unsafe { send_unicode(text) } + } else if crate::input::dispatch::is_wpf_target_window(target) { + bail!("background text injection requires temporarily focusing this target") + } else { + crate::input::post_type_text(target, text) + };🤖 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/rust/crates/platform-windows/src/input/inject.rs` around lines 742 - 746, The current branch in inject_text_cloaked uses post_type_text as a silent fallback when got_fg is false (result uses send_unicode vs crate::input::post_type_text), but posted WM_CHAR can be dropped on the WPF path so this makes inject_text_cloaked return Ok(()) without typing anything; instead, when force_foreground_attached()/got_fg fails do NOT call post_type_text as an equivalent fallback—either return an Err indicating foreground attach failed (or a specific InjectError), or attempt a reliable alternative (e.g. use SendInput/send_unicode with foreground elevation) before considering success; update inject_text_cloaked to detect got_fg==false and propagate failure rather than calling post_type_text, and adjust callers to handle the new error path.libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs-154-156 (1)
154-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the anonymous-cursor contract in the same patch.
This now resolves missing
session/cursor_idto"default", but the in-filecursor_key_resolution_testsstill assertNO_CURSORfor anonymous calls. That leaves this file internally inconsistent and will fail the Windows test module unless the tests/docs move with the behavior change.🤖 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/rust/crates/platform-windows/src/tools/impl_.rs` around lines 154 - 156, Update the in-file tests and any local uses that still expect NO_CURSOR for anonymous calls to use "default" to match the new anonymous-cursor contract: change assertions and references in cursor_key_resolution_tests (and any helpers referencing NO_CURSOR) to assert "default" (or use the same resolution path used by the implementation) so the test expectations align with the code that returns "default" for missing session/cursor_id.libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs-1852-1864 (1)
1852-1864:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArm
NoActivateGuardafter the pid-only HWND fallback resolves.These guards are created before
hwndis auto-resolved frompid, so legacy background calls that omitwindow_idstill run unguarded and can activate the target window. Move guard setup below the sharedhwndresolution path, or re-arm it once the fallback window is selected.Also applies to: 2424-2434, 2994-3004
🤖 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/rust/crates/platform-windows/src/tools/impl_.rs` around lines 1852 - 1864, The NoActivateGuard is being armed before the code resolves a pid-only HWND fallback so legacy background calls without window_id can run unguarded; move the guard creation (crate::input::NoActivateGuard::arm) to after the shared hwnd resolution logic (i.e., after hwnd_opt is finalized) or re-arm the guard once the pid->hwnd fallback is selected, ensuring you only create Some(NoActivateGuard) when dispatch != DispatchMode::Foreground and the final hwnd is Some; apply the same change to the other two occurrences referenced (around the other ranges).libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs-3754-3757 (1)
3754-3757:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBackground drag drops the caller's
duration_mscontract.The new injection fallback only forwards
steps.max(8)intoinject_drag_screen;duration_msis ignored on this path. That makes background drags behave differently from both the documented tool contract and the foreground/PostMessage branches, which can break hover-sensitive or slow-drag targets.🤖 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/rust/crates/platform-windows/src/tools/impl_.rs` around lines 3754 - 3757, The background-drag fallback is ignoring the caller's duration_ms by only passing steps.max(8) into inject_drag_screen; update the spawn_blocking closure so it forwards the original duration_ms (or a steps value derived from duration_ms) to inject_drag_screen instead of hardcoding steps.max(8). Locate the tokio::task::spawn_blocking block that calls crate::input::inject::inject_drag_screen and change the argument list to include the duration_ms parameter (or compute steps from duration_ms consistent with the foreground/PostMessage branches) so background drags honor the same timing contract as other branches.
🧹 Nitpick comments (3)
libs/cua-driver/rust/crates/cua-driver-core/src/name_session_tool.rs (1)
24-40: ⚡ Quick winDeduplicate session-key resolution into a single shared helper.
This helper is explicitly required to stay identical to another implementation, which is a drift risk. Please centralize the resolver and call it from both paths so cursor/session identity can’t diverge over time.
🤖 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/rust/crates/cua-driver-core/src/name_session_tool.rs` around lines 24 - 40, Extract the session-key logic from resolve_session_key into a single shared helper (e.g., a new function resolve_session_key_shared or move it into a common module) and replace both resolve_session_key and platform-macos::cursor_tools::resolve_cursor_key to call that shared helper so they use identical logic; ensure the helper accepts the same input type (&Value), returns String, and preserves the priority: explicit non-empty "cursor_id" > non-empty "_session_id" > "default".libs/cua-driver/rust/crates/platform-macos/src/ax/bindings.rs (1)
78-83: ⚡ Quick win
AXUIElementCopyMultipleAttributeValuesoptions:u32already matches the C typedefApple’s C API declares
AXUIElementCopyMultipleAttributeValues(..., AXCopyMultipleAttributeOptions options, ...), whereAXCopyMultipleAttributeOptionsis aUInt32; the current Rust signature usesu32, so the ABI width mismatch risk called out isn’t present. Switching toCFOptionFlags/the typedef is optional for type documentation/clarity.🤖 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/rust/crates/platform-macos/src/ax/bindings.rs` around lines 78 - 83, The review notes that AXUIElementCopyMultipleAttributeValues already uses a u32 for the options parameter which matches the C typedef AXCopyMultipleAttributeOptions (UInt32), so no ABI-width change is required; keep the current signature pub fn AXUIElementCopyMultipleAttributeValues(..., options: u32, ...) -> AXError, but if you want clearer documentation/typing replace u32 with a type alias (e.g., type AXCopyMultipleAttributeOptions = CFOptionFlags or type AXCopyMultipleAttributeOptions = u32) and update the function signature to use that alias or CFOptionFlags to convey intent without changing ABI.libs/cua-driver/rust/crates/cua-driver-embedded/examples/macos-app-smoke/CuaEmbeddedAppCheck.c (1)
57-60: ⚡ Quick winConsolidate failure-path cleanup to avoid leaking driver/response buffers.
Several early returns call
finish(ctx, 1)without freeing already-allocated response strings and, in some branches, withoutcua_driver_embedded_free(driver). Please route all exits through one cleanup block.Suggested cleanup pattern
static void *run_check(void *raw) { AppCheckContext *ctx = (AppCheckContext *)raw; FILE *out = ctx->out; + int status = 1; + CuaDriver *driver = NULL; + char *initialize = NULL; + char *tools = NULL; + char *permissions = NULL; + char *notification = NULL; @@ - CuaDriver *driver = cua_driver_embedded_new(false); + driver = cua_driver_embedded_new(false); if (driver == NULL) { fprintf(out, "driver=create_failed\n"); - finish(ctx, 1); - return NULL; + goto cleanup; } - char *initialize = cua_driver_embedded_handle_mcp_json( + initialize = cua_driver_embedded_handle_mcp_json( @@ if (require_contains(out, "initialize", initialize, "\"name\":\"cua-driver\"")) { - finish(ctx, 1); - return NULL; + goto cleanup; } - cua_driver_embedded_string_free(initialize); + cua_driver_embedded_string_free(initialize); + initialize = NULL; - char *tools = cua_driver_embedded_handle_mcp_json( + tools = cua_driver_embedded_handle_mcp_json( @@ if (require_contains(out, "tools/list", tools, "\"get_window_state\"") || require_contains(out, "tools/list", tools, "\"check_permissions\"")) { - finish(ctx, 1); - return NULL; + goto cleanup; } @@ - cua_driver_embedded_string_free(tools); + cua_driver_embedded_string_free(tools); + tools = NULL; - char *permissions = cua_driver_embedded_handle_mcp_json( + permissions = cua_driver_embedded_handle_mcp_json( @@ if (require_contains(out, "check_permissions", permissions, "\"accessibility\"") || require_contains(out, "check_permissions", permissions, "\"screen_recording\"")) { - finish(ctx, 1); - return NULL; + goto cleanup; } @@ - cua_driver_embedded_string_free(permissions); + cua_driver_embedded_string_free(permissions); + permissions = NULL; - char *notification = cua_driver_embedded_handle_mcp_json( + notification = cua_driver_embedded_handle_mcp_json( @@ if (notification != NULL) { fprintf(out, "notification=unexpected_response\n"); - cua_driver_embedded_string_free(notification); - finish(ctx, 1); - return NULL; + goto cleanup; } - cua_driver_embedded_free(driver); fprintf(out, "result=passed\n"); fflush(out); - - finish(ctx, 0); - return NULL; + status = 0; +cleanup: + if (notification) cua_driver_embedded_string_free(notification); + if (permissions) cua_driver_embedded_string_free(permissions); + if (tools) cua_driver_embedded_string_free(tools); + if (initialize) cua_driver_embedded_string_free(initialize); + if (driver) cua_driver_embedded_free(driver); + finish(ctx, status); + return NULL; }Also applies to: 69-73, 83-87, 95-100
🤖 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/rust/crates/cua-driver-embedded/examples/macos-app-smoke/CuaEmbeddedAppCheck.c` around lines 57 - 60, Multiple early-return branches call finish(ctx, 1) without freeing allocated response strings (e.g., out) and without releasing the driver with cua_driver_embedded_free(driver); consolidate cleanup by creating a single exit/cleanup block that always frees any non-NULL response buffers and calls cua_driver_embedded_free(driver) before calling finish(ctx, 1) or returning NULL, and replace the early returns in the require_contains failure branches (including the calls in/around require_contains/initalize checks and the other failure sites referenced) to jump to that cleanup block so all resources are released exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1bb9816-5ab0-435a-bd7e-5482a47959de
⛔ Files ignored due to path filters (4)
demo/jukebox/Cargo.lockis excluded by!**/*.lockdemo/multi-cursor/Cargo.lockis excluded by!**/*.locklibs/cua-driver/rust/Cargo.lockis excluded by!**/*.locklibs/cua-driver/rust/crates/cursor-overlay/assets/DejaVuSans-subset.ttfis excluded by!**/*.ttf
📒 Files selected for processing (88)
.github/workflows/nix-build.yml.gitignore.vscode/settings.jsonPackage.swiftdemo/jukebox/Cargo.tomldemo/jukebox/README.mddemo/jukebox/app/Cargo.tomldemo/jukebox/app/src/main.rsdemo/jukebox/media/.gitignoredemo/jukebox/orchestrator/Cargo.tomldemo/jukebox/orchestrator/src/main.rsdemo/multi-cursor/.gitignoredemo/multi-cursor/Cargo.tomldemo/multi-cursor/README.mddemo/multi-cursor/dotnet/winforms/Program.csdemo/multi-cursor/dotnet/winforms/winforms.csprojdemo/multi-cursor/dotnet/wpf/Program.csdemo/multi-cursor/dotnet/wpf/wpf.csprojdemo/multi-cursor/electron/index.htmldemo/multi-cursor/electron/main.jsdemo/multi-cursor/electron/package.jsondemo/multi-cursor/legacy-app/Cargo.tomldemo/multi-cursor/legacy-app/src/main.rsdemo/multi-cursor/orchestrator/Cargo.tomldemo/multi-cursor/orchestrator/src/main.rsdocs/content/docs/cua-driver/guide/getting-started/embedded-mcp.mdxdocs/content/docs/cua-driver/guide/getting-started/meta.jsondocs/content/docs/cua-driver/guide/getting-started/swift-integration.mdxdocs/content/docs/cua-driver/reference/mcp-tools.mdxflake.nixlibs/cua-driver/rust/Cargo.tomllibs/cua-driver/rust/Skills/cua-driver/SKILL.mdlibs/cua-driver/rust/crates/cua-driver-core/src/lib.rslibs/cua-driver/rust/crates/cua-driver-core/src/name_session_tool.rslibs/cua-driver/rust/crates/cua-driver-core/src/protocol.rslibs/cua-driver/rust/crates/cua-driver-core/src/server.rslibs/cua-driver/rust/crates/cua-driver-core/src/session.rslibs/cua-driver/rust/crates/cua-driver-core/src/tool.rslibs/cua-driver/rust/crates/cua-driver-embedded/Cargo.tomllibs/cua-driver/rust/crates/cua-driver-embedded/examples/macos-app-smoke/CuaEmbeddedAppCheck.clibs/cua-driver/rust/crates/cua-driver-embedded/examples/macos-app-smoke/Info.plistlibs/cua-driver/rust/crates/cua-driver-embedded/examples/macos-app-smoke/README.mdlibs/cua-driver/rust/crates/cua-driver-embedded/examples/macos-app-smoke/run.shlibs/cua-driver/rust/crates/cua-driver-embedded/include/cua_driver_embedded.hlibs/cua-driver/rust/crates/cua-driver-embedded/src/lib.rslibs/cua-driver/rust/crates/cua-driver/Cargo.tomllibs/cua-driver/rust/crates/cua-driver/build.rslibs/cua-driver/rust/crates/cua-driver/cua-driver.manifestlibs/cua-driver/rust/crates/cua-driver/cua-driver.rclibs/cua-driver/rust/crates/cua-driver/src/cli.rslibs/cua-driver/rust/crates/cua-driver/src/main.rslibs/cua-driver/rust/crates/cua-driver/src/mcp_http.rslibs/cua-driver/rust/crates/cua-driver/src/serve.rslibs/cua-driver/rust/crates/cua-driver/tests/e2e_windows_bg_input_test.rslibs/cua-driver/rust/crates/cursor-overlay/Cargo.tomllibs/cua-driver/rust/crates/cursor-overlay/assets/LICENSE-DejaVu.txtlibs/cua-driver/rust/crates/cursor-overlay/src/lib.rslibs/cua-driver/rust/crates/cursor-overlay/src/motion.rslibs/cua-driver/rust/crates/cursor-overlay/src/palette.rslibs/cua-driver/rust/crates/cursor-overlay/src/render_state.rslibs/cua-driver/rust/crates/platform-linux/Cargo.tomllibs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rslibs/cua-driver/rust/crates/platform-linux/src/atspi/native.rslibs/cua-driver/rust/crates/platform-linux/src/input/mod.rslibs/cua-driver/rust/crates/platform-linux/src/lib.rslibs/cua-driver/rust/crates/platform-linux/src/overlay.rslibs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rslibs/cua-driver/rust/crates/platform-linux/src/tty.rslibs/cua-driver/rust/crates/platform-macos/src/ax/bindings.rslibs/cua-driver/rust/crates/platform-macos/src/ax/tree.rslibs/cua-driver/rust/crates/platform-macos/src/cursor/overlay.rslibs/cua-driver/rust/crates/platform-macos/src/tools/cursor_tools.rslibs/cua-driver/rust/crates/platform-macos/src/tools/mod.rslibs/cua-driver/rust/crates/platform-windows/Cargo.tomllibs/cua-driver/rust/crates/platform-windows/examples/zdrop_probe.rslibs/cua-driver/rust/crates/platform-windows/src/capture.rslibs/cua-driver/rust/crates/platform-windows/src/input/dispatch.rslibs/cua-driver/rust/crates/platform-windows/src/input/inject.rslibs/cua-driver/rust/crates/platform-windows/src/input/mod.rslibs/cua-driver/rust/crates/platform-windows/src/input/mouse.rslibs/cua-driver/rust/crates/platform-windows/src/overlay.rslibs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rslibs/cua-driver/swift/Package.swiftlibs/cua-driver/swift/Sources/CuaDriverEmbedded/CuaDriverEmbedded.swiftnix/cua-driver/package.nixnix/cua-driver/tests/linux-background-gui.nixnix/cua-driver/tests/linux-background-terminal-gif.nixnix/cua-driver/tests/linux-cursor-click-gif.nix
c8f93e7 to
fa36a6b
Compare
ddupont808
changed the title
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add high-DPI awareness support for Windows, enabling proper coordinate scaling and display clarity on high-resolution displays. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…lity When no session is provided in cursor tool calls, use "default" instead of NO_CURSOR to maintain overlay functionality for legacy clients. Also add demo/gemini-cli/ to gitignore. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
fa36a6b to
0275983
Compare
ddupont808
changed the title
…t default Tests now correctly expect 'default' instead of NO_CURSOR when no session parameter is provided, matching the backwards compatibility fix from commit 0275983. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fix critical bugs where logical and physical pixel coordinates were mismatched after adding DPI awareness manifest. **Fixes:** 1. screenshot_display_bytes: Scale logical GetSystemMetrics dimensions to physical pixels before BitBlt capture 2. screenshot_via_screen_region: Scale GetWindowRect logical coords to physical pixels for screen DC BitBlt 3. screenshot_window_bytes (occlusion path): Scale window dimensions to physical pixels for bitmap creation 4. get_screen_size: Update comments - GetSystemMetrics already returns logical pixels with permonitorv2, no double-scaling needed With permonitorv2 DPI awareness: - GetSystemMetrics/GetWindowRect return logical pixels (DPI-scaled) - BitBlt/CreateCompatibleBitmap work in physical device pixels - Conversion: physical = logical × (DPI / 96.0) These fixes ensure screenshots and coordinates work correctly at any DPI scaling (125%, 150%, 200%). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The cargoHash needs to be updated to match the modified Cargo.lock from the Windows improvements. Setting to empty string to trigger CI to provide the correct hash.
CI provided the correct hash: sha256-TezobhZKan2E087x8cECCqZS0lafEBAOd0Cx70BgP9w=
1 participant
Summary
This integration branch adds 3 major Windows platform improvements built on top of the recently merged session infrastructure (v0.5.1+).
🎯 Per-Session Overlay Windows (PR #1804)
sessionparameter default to "default" session🔇 Background Input Without Z-Raise (PR #1809)
WS_EX_NOACTIVATEguards🖥️ DPI Awareness Manifest (PR #1821)
🔄 Additional Fixes
sessionparameter provided, defaults to"default"sessionChanged Files
51 files changed (+7,439/-221 lines)
Windows Platform (
platform-windows/)src/input/inject.rs(792 lines) - Background input injection systemsrc/overlay.rs(+433 lines) - Per-session overlay windowssrc/tools/impl_.rs(+186 lines) - Background dispatch routing, fixed testssrc/input/dispatch.rs,src/input/mouse.rs- Framework detectionsrc/capture.rs(+52 lines) - Fixed DPI-aware screenshot capturemacOS Platform (
platform-macos/)src/tools/cursor_tools.rs- Backwards compatibility fixDemos & Tests
demo/jukebox/- MIDI-driven multi-cursor demo (3 crates, ~1.4k lines)demo/multi-cursor/- Cross-framework background input demo (7 apps, ~1.1k lines)tests/e2e_windows_bg_input_test.rs(558 lines) - Comprehensive E2E testsexamples/zdrop_probe.rs(314 lines) - Z-order diagnostic toolBuild System
cua-driver.manifest(27 lines) - DPI awareness manifestcua-driver.rc(1 line) - Resource script for manifest embeddingbuild.rs- Embed manifest on WindowsCargo.toml- Add embed-resource build dependencyBase Branch
This PR is built on top of main (v0.5.1+) which already includes:
Test Plan
Automated Tests
e2e_windows_bg_input_test.rs):Manual Validation
Breaking Changes
None. All changes are backwards compatible:
dispatch:"background"(which is now the default, matching the documented behavior)Clarifications on Behavior
Background Input Default
dispatch:"background"is the DEFAULT (as documented in WINDOWS.md)WS_EX_NOACTIVATEstyle during injection to prevent activationWindow Z-Order
WS_EX_NOACTIVATEprevents foreground stealingDPI Awareness
permonitorv2manifest, all coordinates are in logical pixelsPerformance Impact
Known Limitations
Background Input
Windows-Only
Documentation Status
Per agent review:
Follow-up Work
Documentation improvements (non-blocking):
🤖 Generated with Claude Code