fix(cua-driver-rs): 3 cli.rs bugs — BOM-in-stdin, daemon image merge, --socket plumbing - #1661
Merged
Merged
Conversation
`read_stdin_json` was feeding the buffer directly to serde_json::from_str
after trimming whitespace, but PowerShell 5.1's
`Set-Content -Encoding utf8` silently prepends a UTF-8 BOM
(`EF BB BF` / `U+FEFF`) to the file. When that file is fed to cua-driver
via `Start-Process -RedirectStandardInput`, the BOM stays at the front
of stdin, serde_json fails (a BOM isn't a valid JSON start character),
and the call falls through to default args — producing surprising errors
like "Missing required integer field pid" despite a valid-looking payload.
Surfaced live during the 2026-05-23 overnight Windows dogfood:
$ Set-Content -Path tmp.json -Value '{"pid":8608}' -Encoding utf8
$ cua-driver call hotkey < tmp.json
Missing required integer field pid.
After fix:
$ cua-driver call hotkey < tmp.json
✅ Pressed ctrl+s on pid 8608 via PostMessage (Win32 target).
Implementation: strip a single leading `'\u{feff}'` character from the
trimmed buffer before handing to serde_json. The PS5.1 BOM is a 3-byte
sequence in UTF-8 (`EF BB BF`) but a single char in str terms, so a
`strip_prefix('\u{feff}')` is the right primitive.
Tests in `stdin_bom_tests`:
- BOM-prefixed payload parses successfully
- Plain JSON unchanged (no-op when no BOM)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…arding path
`cua-driver call screenshot` was silently dropping the image bytes when
a daemon was running on the named pipe. The output looked like:
$ cua-driver call screenshot
{"format": "png", "height": 949, "width": 1512}
— metadata only, no `screenshot_png_b64`. The image was 7.4 MB and the
caller had no way to get it back.
Root cause: the CLI in `run_call` has two code paths.
The **in-process path** (no daemon listening) loops `result.content`,
captures any Content::Image into `image_b64`, then merges that into the
JSON-printed `structuredContent`:
if let Some(sc) = &result.structured_content {
let mut obj = sc.clone();
if out_path.is_none() {
if let Some((b64, mime)) = image_b64 {
... insert("screenshot_png_b64", b64) ...
}
}
println!("{pretty}");
}
The **daemon-forwarding path** (daemon listening at default pipe) parsed
the daemon's response, looped `content`, but only handled the image when
`--screenshot-out-file` was set — otherwise the image was just thrown
away:
for item in content {
if item.get("type") == Some("image") {
if let Some(ref path) = screenshot_out_file {
... write to file ...
}
// ← else: silently drop
}
}
if let Some(sc) = result.get("structuredContent") {
let pretty = serde_json::to_string_pretty(sc)...;
println!("{pretty}");
}
Tonight on the cuademo VM (cua-driver-serve task registered, daemon at
\\.\pipe\cua-driver listening), every `cua-driver call screenshot`
invocation went through the daemon path and lost the image. From a
non-daemon shell it worked fine.
Fix: in the daemon-forwarding path, stash the image into `image_b64`
during the content walk (when no out-path is set), then merge it into
the structuredContent object before the pretty-print — mirroring the
in-process path's behavior 1:1. Same final `screenshot_png_b64` +
`screenshot_mime_type` keys.
Empirical verification:
before: cua-driver call screenshot | out-string → 61 chars
after: cua-driver call screenshot | out-string → 7,654,802 chars
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…gression test `--socket <path>` was a CLI-level flag (parsed in cli.rs line 142, listed in VALUE_FLAGS line 93) and threaded into `Command::Serve` / `Command::Stop` / `Command::Status` / `Command::Mcp` / `Command::Config` / `Command::Recording` — but **not into `Command::Call`**. That meant `cua-driver call --socket X screenshot` silently ignored the --socket override and used `default_socket_path()`. The flag worked for every long-running command but not for the per-call invocation. This made it impossible to write a daemon-forwarding regression test that hits a tempfile-socketed test daemon (the existing ServeDaemonTests pattern): the call always routed to the default socket, never to the test's `--socket /tmp/cua-test-XXX.sock` daemon. Changes: - `cli::Command::Call` gains `socket: Option<String>` field - Both parse sites (line 249 + 310) thread `socket` through - `cli::run_call` takes a `socket_override: Option<String>` parameter - When Some, skip the platform default + uia-worker-preferred resolution and route directly to that path - Both main.rs callsites pass the socket through Added a regression test for the 2026-05-23 screenshot fix: `test_call_screenshot_via_daemon_emits_b64` — starts a tempfile-socketed daemon, invokes `cua-driver call --socket X screenshot`, asserts that `screenshot_png_b64` appears in the JSON output. This catches the bug where the daemon-forwarding path in `run_call` was silently dropping the image (only printing `structuredContent` metadata: format / width / height) — a class of bug the in-process-only `test_call_screenshot_returns_b64_image` test couldn't catch because it never exercised the daemon code path. 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 extends ChangesDaemon Socket Override Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/cua-driver-rs/tests/integration/test_cli.py (1)
145-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast if the daemon never comes up.
This helper returns
proceven whenstatus --socketnever succeeds. Becauserun_call(..., socket)falls back in-process on an unreachable socket,test_call_screenshot_via_daemon_emits_b64can still pass without ever exercising the daemon path.Proposed fix
def _start_daemon(self): import subprocess, time proc = subprocess.Popen( [self.binary, "serve", "--socket", self._sock_file], stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) # Wait for daemon to bind. for _ in range(30): time.sleep(0.1) r = _run([self.binary, "status", "--socket", self._sock_file]) if r.returncode == 0: - break - return proc + return proc + + proc.terminate() + proc.wait(timeout=3) + self.fail(f"daemon did not become ready on {self._sock_file}")🤖 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/tests/integration/test_cli.py` around lines 145 - 151, The helper currently returns proc even when the daemon never responds to `status --socket`, which masks daemon-start failures; update the loop that checks `r = _run([self.binary, "status", "--socket", self._sock_file])` so that if no successful `r.returncode == 0` is observed after the retry window you terminate/cleanup `proc` (e.g., proc.kill() or proc.terminate()), wait for it to exit, and raise an exception (or assert) instead of returning `proc`, ensuring tests such as `test_call_screenshot_via_daemon_emits_b64` cannot silently fall back to the in-process path.
🤖 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/cua-driver/src/cli.rs`:
- Around line 771-775: The daemon path currently discards std::fs::write result
when writing decoded bytes for screenshot_out_file, so write failures are
hidden; change the Ok(bytes) arm inside the base64 decode match to check the
Result from std::fs::write(path, &bytes) and surface errors exactly like the
decode Err branch (e.g., eprintln!("--screenshot-out-file: {e}") or otherwise
return/propagate a non-zero/Err), ensuring screenshot_out_file write errors are
reported and cause the same failure behavior as the in-process branch.
---
Outside diff comments:
In `@libs/cua-driver-rs/tests/integration/test_cli.py`:
- Around line 145-151: The helper currently returns proc even when the daemon
never responds to `status --socket`, which masks daemon-start failures; update
the loop that checks `r = _run([self.binary, "status", "--socket",
self._sock_file])` so that if no successful `r.returncode == 0` is observed
after the retry window you terminate/cleanup `proc` (e.g., proc.kill() or
proc.terminate()), wait for it to exit, and raise an exception (or assert)
instead of returning `proc`, ensuring tests such as
`test_call_screenshot_via_daemon_emits_b64` cannot silently fall back to the
in-process path.
🪄 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: 0dd76d0d-d0de-4bec-b10c-ddddf8b2187f
📒 Files selected for processing (3)
libs/cua-driver-rs/crates/cua-driver/src/cli.rslibs/cua-driver-rs/crates/cua-driver/src/main.rslibs/cua-driver-rs/tests/integration/test_cli.py
…mon path The daemon-forwarding branch was silently discarding the result of `std::fs::write`, so a failed write (e.g. permission denied, disk full, parent dir missing) would exit 0 with no indication. The in-process branch already reports these. This commit aligns the daemon branch with the same error-reporting shape, with separate messages for the base64 decode vs. file-write failure paths. Spotted by CodeRabbit on PR #1661. 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
Three small fixes to
cua-driver calldiscovered during overnight dogfooding on the Windows VM (cua-driver-rs v0.2.18). All are real bugs with concrete repros + tests; none changes documented behavior — they fix paths that should have worked but didn't.1. UTF-8 BOM in stdin payloads silently rejected (
5f033aed)PowerShell 5.1's
Set-Content -Encoding utf8writes a 3-byte BOM (`EF BB BF`) at file start. When that file is piped into `cua-driver call `, the cli's `read_stdin_json` failed to parse and the tool returned the misleading error "Missing required integer field pid" (because every field looked missing).Fix: strip the BOM in `read_stdin_json` before serde_json::from_str. Two unit tests guard the behavior (BOM present + BOM absent).
Repro before fix:
```powershell
Set-Content -Path .\req.json -Value '{"pid":99999}' -Encoding utf8 # writes BOM
Get-Content .\req.json -Raw | cua-driver call list_windows
→ "Missing required integer field pid."
```
2. Daemon-forwarded screenshot dropped the image bytes (
cf8f3ca9)`cua-driver call screenshot` over a daemon socket returned `{"format": "png", "width": ..., "height": ...}` and silently discarded the `screenshot_png_b64` field. The in-process path was correct; only the daemon-forwarding branch in `run_call` was missing the image merge into structuredContent.
Fix: in the daemon-forwarding branch, walk the response `content` array, capture any `type: image` item, and merge it into `structuredContent` as `screenshot_png_b64` + `screenshot_mime_type` — same shape as the in-process path produces.
3. `--socket` flag wasn't wired for `Command::Call` (
d375e053)The cli-reference docs document `--socket` for `cua-driver call`, but `Command::Call` didn't actually accept it. Added the `socket` field to `Command::Call` (parsed in both call sites), added a `socket_override` param to `run_call`, and added an integration regression test (`test_call_screenshot_via_daemon_emits_b64`).
The new test is `@unittest.skipIf(sys.platform == "win32")` because the subprocess-spawned daemon on Windows CI runs in Session 0 (no graphics → BitBlt returns `0x80070006`). It exercises the regression on macOS + Linux runners.
Diff
~140 LOC across 3 files. All three commits independently revertable.
Docs
No docs change needed — `cli-reference.mdx` already documents `--socket` for `call` (the docs were aspirational; this PR aligns implementation with documentation). `mcp-tools.mdx` is auto-generated from Swift sources and these are internal cli plumbing fixes that don't change Swift's tool surface.
Test plan
OVERNIGHT_HANDOFF_CHECKS.mdsection 2🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--socketparameter to override daemon socket path forcua-driver callcommandsBug Fixes
Tests