feat(cua-driver-rs)(recording): native ScreenCaptureKit on macOS + app_state.json/click.png regressions - #1720
Merged
Conversation
…_index + ffmpeg TCC fast-fail Three regressions introduced by the cross-platform Rust recording refactor (#1718, since the rename to cua-driver-core). User reproduced all three live on macOS Calculator (5+3) — zero app_state.json files, zero click.png files (all 5 turns used element_index), and a 0-byte recording.mp4 with `finalized: false` while ffmpeg silently blocked forever on a TCC prompt the daemon couldn't surface. 1. **app_state.json never written.** `recording::write_turn` had no AX snapshot path. Added `AX_SNAPSHOT_FN` callback mirroring the existing `SCREENSHOT_FN`/`CLICK_MARKER_FN` pattern, wired to `platform_macos::recording_hooks::app_state_json_for` (walks the AX tree fresh for the target pid + window_id and emits the same shape `get_window_state` returns minus the screenshot fields) and the same on Windows via UIA. Linux is intentionally a no-op — ATSPI has no cheap whole-tree snapshot and the file is omitted rather than faked. 2. **click.png never written for element_index clicks.** The check at recording.rs:335 only set `click_point` when `x` AND `y` were present in args, so AX-indexed clicks (the dominant path on Calculator AX buttons — all 5 user-reproed turns) fell through and click.png was never produced. Added `ELEMENT_BOUNDS_FN` resolving `element_index` → window-local screenshot-pixel coords via the live AX cache (macOS: `element_screen_center` minus window origin, multiplied by Retina scale derived from PNG width / logical width; Windows: cached center minus window origin, no scale). The element cache is shared with the recording-hook layer at `register_all` startup so the callback is wired without a registry detour. 3. **recording.mp4 never finalized on macOS.** ffmpeg+avfoundation needs its own per-binary Screen Recording grant; TCC doesn't propagate from the parent process. When spawned by a daemon there's no UI thread to display the consent dialog, so ffmpeg blocks forever on `[ScreenCaptureKit] requesting consent...`. Two fast-fail probes added to `VideoRecorder::start`: (a) `try_wait` polling for the first 1500 ms catches immediate exits with stderr tail, (b) on macOS, after another ~500 ms, if the output file is still 0 bytes we treat it as a TCC hang, kill the subprocess, and return an actionable error pointing the user at Privacy & Security → Screen & System Audio Recording with the resolved ffmpeg path. The error ends up in `session.json` `video.error` AND the `start_recording` response text so callers can't miss it. Deferred (separate follow-up issue): replace ffmpeg+avfoundation on macOS with a native ScreenCaptureKit binding so video works zero-config. Multi-week native-binding work, out of scope for a regression fix. Docs: RECORDING.md updated with the per-turn `app_state.json` shape + when it's omitted (Linux), the click.png element_index path, and the macOS ffmpeg TCC requirement with the planned migration. The `start_recording` MCP tool description picks up the same TCC note. 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 introduces a cross-platform video backend abstraction layer for the cua-driver recording system. macOS migrates from a problematic ffmpeg-subprocess approach (which cannot inherit the host's Screen Recording TCC grant) to native in-process ScreenCaptureKit, while Windows and Linux retain optimized ffmpeg subprocess backends. Recording core gains platform-agnostic hooks for AX/UIA snapshots and element-index-to-window-local-coordinate resolution, enabling click imagery and per-turn state capture across all platforms. ChangesCross-platform video recording backend
🎯 4 (Complex) | ⏱️ ~45 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 |
4 tasks
…S, ffmpeg only on Win/Linux Replaces the macOS ffmpeg+avfoundation subprocess pipeline with an in-process SCStream + SCRecordingOutput. ScreenCaptureKit runs in the cua-driver process, so it inherits the daemon's Screen Recording TCC grant — no separate subprocess prompt, no fast-fail hang heuristic. video.rs now exposes `VideoBackend` + `VideoBackendFactory` traits; each platform crate registers its concrete factory at startup (`set_video_backend_factory`), mirroring how `SCREENSHOT_FN` / `AX_SNAPSHOT_FN` are wired. macOS registers `SckitVideoBackendFactory` (platform-macos/src/video_sckit.rs); Windows + Linux register `FfmpegVideoBackendFactory` (cua-driver-core/src/video_ffmpeg.rs, carrying the existing stderr-tail fast-fail). Requires macOS 15.0+ (SCRecordingOutput introduced in macOS 15). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
f-trycua
changed the title
f-trycua
marked this pull request as ready for review
May 26, 2026 20:54
![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: 7
🤖 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/rust/crates/cua-driver-core/src/recording_tools.rs`:
- Around line 72-83: The documentation string for the `record_video` schema in
recording_tools.rs incorrectly states that ffmpeg is required for macOS; update
the doc text around the `record_video` field (the multiline doc comment block
describing `<output_dir>/recording.mp4` and platform behavior) so it clearly
states: macOS uses native ScreenCaptureKit (no ffmpeg or extra TCC prompt,
requires macOS 15+), and only Windows and Linux require an ffmpeg subprocess
(gdigrab/x11grab + libx264) with ffmpeg on PATH; make the same correction for
the second occurrence of this description later in the file so both blocks
consistently reflect platform-specific behavior.
In `@libs/cua-driver/rust/crates/cua-driver-core/src/recording.rs`:
- Around line 374-376: The match arm that calls ELEMENT_BOUNDS_FN.get()
currently does an unchecked cast with "idx as u32", which can silently truncate
large element_index values; update the arm to validate and convert element_index
safely (e.g., use u32::try_from(idx).ok() or idx.try_into().ok()) and only call
f(wid, p, idx_u32) when conversion succeeds, otherwise return None (preserving
the existing fallback); reference the tuple match containing (Some(wid),
Some(p), Some(idx), Some(f)) and replace the cast with a safe conversion like
let idx_u32 = u32::try_from(idx).ok() and guard the call on that.
In `@libs/cua-driver/rust/crates/cua-driver-core/src/video_ffmpeg.rs`:
- Around line 54-56: The directory-creation error is currently ignored
(std::fs::create_dir_all(parent).ok()), so if output_path.parent() is not
creatable we should fail early with a propagated error and context before
attempting to spawn ffmpeg; replace the .ok() call with proper error propagation
(e.g., std::fs::create_dir_all(parent)? or
std::fs::create_dir_all(parent).with_context(|| format!("creating output
directory for {:?}", output_path))?) so the function returns an Err with a
helpful message when create_dir_all fails, referencing output_path.parent(),
create_dir_all, and the ffmpeg spawn site to ensure failures are reported before
ffmpeg is started.
In `@libs/cua-driver/rust/crates/platform-macos/src/recording_hooks.rs`:
- Around line 28-31: The code currently narrows pid and window_id using
unchecked "as" casts (pid? as i32 and w as u32) which can truncate values;
replace these with fallible/constrained conversions (e.g., TryFrom/TryInto or
explicit range checks) so you return an error or None when the original
pid/window_id is out of target-type range; specifically update the binding where
pid is computed and the match on window_id, and apply the same guarded
conversion approach to the other occurrences around the resolve_main_window_id
call (lines referenced in the review) to ensure resolve_main_window_id(pid) and
any window lookups never receive truncated values.
In `@libs/cua-driver/rust/crates/platform-macos/src/video_sckit.rs`:
- Around line 59-64: The current startup code around output_path silences all
filesystem errors; change it so std::fs::create_dir_all(parent) returns a
handled Result (propagate or log and return Err) instead of calling .ok(), and
for std::fs::remove_file(output_path) only ignore the NotFound/DoesNotExist
error while propagating or logging any other IO errors; locate the logic
referencing output_path, the create_dir_all(parent) call and the
remove_file(output_path) call in this module and replace the blanket .ok() and
unexamined let _ = ... with explicit Result handling that surfaces
permission/path failures but still tolerates a missing stale file.
In `@libs/cua-driver/rust/crates/platform-windows/src/recording_hooks.rs`:
- Around line 25-29: Replace unchecked "as" casts from i64 to u32 for pid and
window IDs with fallible conversions (e.g., TryFrom/try_into or i64::try_into)
and propagate a clear error when conversion fails; update the code around the
pid assignment and the window_id fallback (the block using pid? as u32 and
crate::win32::list_windows(...).first().map(|w| w.hwnd)?) to perform a checked
conversion of the i64 value to u32, returning an Err or mapping to a descriptive
error if the value is negative or out of range; apply the same fix to the other
occurrences mentioned (the similar conversion in the later block handling
window_id) so all PID/window ID conversions are safe and handled explicitly.
In `@libs/cua-driver/rust/Skills/cua-driver/RECORDING.md`:
- Around line 33-38: The README record is inconsistent: change the macOS-only
header in RECORDING.md to reflect cross-platform support (rename or rewrite the
header that currently says "macOS-only" to include Windows and Linux) so it
aligns with the "Windows / Linux — ffmpeg subprocess." section; ensure the
document mentions that Windows uses gdigrab, Linux uses x11grab, ffmpeg must be
on PATH (installation hints), and that missing ffmpeg results in per-turn
capture continuing without video with last_error carrying the install hint and
ffmpeg startup failures fast-fail with a stderr tail in the error.
🪄 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: 6a2d642c-daed-47b6-bec0-27b7071d65d5
⛔ Files ignored due to path filters (1)
libs/cua-driver/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
libs/cua-driver/rust/Skills/cua-driver/RECORDING.mdlibs/cua-driver/rust/crates/cua-driver-core/src/lib.rslibs/cua-driver/rust/crates/cua-driver-core/src/recording.rslibs/cua-driver/rust/crates/cua-driver-core/src/recording_loader.rslibs/cua-driver/rust/crates/cua-driver-core/src/recording_render.rslibs/cua-driver/rust/crates/cua-driver-core/src/recording_tools.rslibs/cua-driver/rust/crates/cua-driver-core/src/video.rslibs/cua-driver/rust/crates/cua-driver-core/src/video_ffmpeg.rslibs/cua-driver/rust/crates/cua-driver/build.rslibs/cua-driver/rust/crates/cua-driver/src/main.rslibs/cua-driver/rust/crates/platform-macos/Cargo.tomllibs/cua-driver/rust/crates/platform-macos/src/lib.rslibs/cua-driver/rust/crates/platform-macos/src/recording_hooks.rslibs/cua-driver/rust/crates/platform-macos/src/tools/mod.rslibs/cua-driver/rust/crates/platform-macos/src/video_sckit.rslibs/cua-driver/rust/crates/platform-windows/src/lib.rslibs/cua-driver/rust/crates/platform-windows/src/recording_hooks.rslibs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs
Comment on lines
+72
to
+83
| `<output_dir>/recording.mp4` (H.264 / 30 fps) for the lifetime of \ | ||
| the session. Pass `record_video: false` to opt out.\n\n\ | ||
| **macOS uses native ScreenCaptureKit** (in-process SCStream + \ | ||
| SCRecordingOutput) so video inherits cua-driver's own Screen \ | ||
| Recording grant — no extra TCC prompt, no ffmpeg subprocess. \ | ||
| Requires macOS 15.0+.\n\n\ | ||
| **Windows + Linux use an ffmpeg subprocess** (`gdigrab` / \ | ||
| `x11grab` + libx264). Requires ffmpeg on PATH (winget install \ | ||
| Gyan.FFmpeg / apt install ffmpeg); when ffmpeg is missing or \ | ||
| fails on startup the per-turn capture (screenshots + \ | ||
| action.json) still runs and the session's `last_error` field \ | ||
| carries the diagnostic.\n\n\ |
Contributor
There was a problem hiding this comment.
Align record_video schema text with the new platform-specific behavior.
Line 74-Line 77 correctly says macOS uses ScreenCaptureKit, but Line 98-Line 100 still says ffmpeg is required. That contradiction can mislead clients that surface schema descriptions.
Suggested doc fix
"record_video": {
"type": "boolean",
- "description": "Capture the main display to <output_dir>/recording.mp4. \
- Default: true. Set to false to record only the per-turn \
- screenshots + JSON. Requires ffmpeg on PATH."
+ "description": "Capture the main display to <output_dir>/recording.mp4. \
+ Default: true. Set to false to record only the per-turn \
+ screenshots + JSON. On macOS this uses native \
+ ScreenCaptureKit; on Windows/Linux it requires ffmpeg on PATH."
}Also applies to: 96-101
🤖 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/recording_tools.rs` around
lines 72 - 83, The documentation string for the `record_video` schema in
recording_tools.rs incorrectly states that ffmpeg is required for macOS; update
the doc text around the `record_video` field (the multiline doc comment block
describing `<output_dir>/recording.mp4` and platform behavior) so it clearly
states: macOS uses native ScreenCaptureKit (no ffmpeg or extra TCC prompt,
requires macOS 15+), and only Windows and Linux require an ffmpeg subprocess
(gdigrab/x11grab + libx264) with ffmpeg on PATH; make the same correction for
the second occurrence of this description later in the file so both blocks
consistently reflect platform-specific behavior.
Comment on lines
+374
to
+376
| _ => match (window_id, pid, element_index, ELEMENT_BOUNDS_FN.get()) { | ||
| (Some(wid), Some(p), Some(idx), Some(f)) => f(wid, p, idx as u32), | ||
| _ => None, |
Contributor
There was a problem hiding this comment.
Validate element_index before narrowing to u32.
idx as u32 silently truncates for values above u32::MAX, which can resolve the wrong element and produce incorrect click_point/click.png.
Proposed fix
- _ => match (window_id, pid, element_index, ELEMENT_BOUNDS_FN.get()) {
- (Some(wid), Some(p), Some(idx), Some(f)) => f(wid, p, idx as u32),
- _ => None,
- },
+ _ => match (window_id, pid, element_index, ELEMENT_BOUNDS_FN.get()) {
+ (Some(wid), Some(p), Some(idx), Some(f)) => {
+ u32::try_from(idx).ok().and_then(|idx32| f(wid, p, idx32))
+ }
+ _ => None,
+ },📝 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
| _ => match (window_id, pid, element_index, ELEMENT_BOUNDS_FN.get()) { | |
| (Some(wid), Some(p), Some(idx), Some(f)) => f(wid, p, idx as u32), | |
| _ => None, | |
| _ => match (window_id, pid, element_index, ELEMENT_BOUNDS_FN.get()) { | |
| (Some(wid), Some(p), Some(idx), Some(f)) => { | |
| u32::try_from(idx).ok().and_then(|idx32| f(wid, p, idx32)) | |
| } | |
| _ => None, | |
| }, |
🤖 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/recording.rs` around lines
374 - 376, The match arm that calls ELEMENT_BOUNDS_FN.get() currently does an
unchecked cast with "idx as u32", which can silently truncate large
element_index values; update the arm to validate and convert element_index
safely (e.g., use u32::try_from(idx).ok() or idx.try_into().ok()) and only call
f(wid, p, idx_u32) when conversion succeeds, otherwise return None (preserving
the existing fallback); reference the tuple match containing (Some(wid),
Some(p), Some(idx), Some(f)) and replace the cast with a safe conversion like
let idx_u32 = u32::try_from(idx).ok() and guard the call on that.
Comment on lines
+54
to
+56
| if let Some(parent) = output_path.parent() { | ||
| std::fs::create_dir_all(parent).ok(); | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate output-directory creation errors before spawning ffmpeg.
Line 55 currently drops filesystem errors. If the directory is not creatable, fail early with context instead of deferring to a generic ffmpeg startup failure.
Suggested patch
if let Some(parent) = output_path.parent() {
- std::fs::create_dir_all(parent).ok();
+ std::fs::create_dir_all(parent).map_err(|e| {
+ anyhow::anyhow!(
+ "failed to create recording output directory {}: {e}",
+ parent.display()
+ )
+ })?;
}🤖 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/video_ffmpeg.rs` around lines
54 - 56, The directory-creation error is currently ignored
(std::fs::create_dir_all(parent).ok()), so if output_path.parent() is not
creatable we should fail early with a propagated error and context before
attempting to spawn ffmpeg; replace the .ok() call with proper error propagation
(e.g., std::fs::create_dir_all(parent)? or
std::fs::create_dir_all(parent).with_context(|| format!("creating output
directory for {:?}", output_path))?) so the function returns an Err with a
helpful message when create_dir_all fails, referencing output_path.parent(),
create_dir_all, and the ffmpeg spawn site to ensure failures are reported before
ffmpeg is started.
Comment on lines
+28
to
+31
| let pid = pid? as i32; | ||
| let resolved_wid = match window_id { | ||
| Some(w) => w as u32, | ||
| None => crate::windows::resolve_main_window_id(pid).ok()?, |
Contributor
There was a problem hiding this comment.
Guard PID/window-id conversions instead of using unchecked as casts.
Unchecked narrowing (i64 -> i32, u64 -> u32) can misroute lookups to the wrong process/window when inputs are out of range.
Proposed fix
pub fn app_state_json_for(window_id: Option<u64>, pid: Option<i64>) -> Option<Vec<u8>> {
- let pid = pid? as i32;
+ let pid = i32::try_from(pid?).ok()?;
let resolved_wid = match window_id {
- Some(w) => w as u32,
+ Some(w) => u32::try_from(w).ok()?,
None => crate::windows::resolve_main_window_id(pid).ok()?,
};
@@
pub fn element_window_local_xy(window_id: u64, pid: i64, element_index: u32) -> Option<(f64, f64)> {
let cache = ELEMENT_CACHE.get()?;
- let ptr = cache.get_element_ptr(pid as i32, window_id as u32, element_index as usize)?;
+ let pid_i32 = i32::try_from(pid).ok()?;
+ let window_id_u32 = u32::try_from(window_id).ok()?;
+ let ptr = cache.get_element_ptr(pid_i32, window_id_u32, element_index as usize)?;
let (sx, sy) = unsafe { element_screen_center(ptr as AXUIElementRef)? };
- let bounds = crate::windows::window_bounds_by_id(window_id as u32)?;
+ let bounds = crate::windows::window_bounds_by_id(window_id_u32)?;Also applies to: 50-54
🤖 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/recording_hooks.rs` around
lines 28 - 31, The code currently narrows pid and window_id using unchecked "as"
casts (pid? as i32 and w as u32) which can truncate values; replace these with
fallible/constrained conversions (e.g., TryFrom/TryInto or explicit range
checks) so you return an error or None when the original pid/window_id is out of
target-type range; specifically update the binding where pid is computed and the
match on window_id, and apply the same guarded conversion approach to the other
occurrences around the resolve_main_window_id call (lines referenced in the
review) to ensure resolve_main_window_id(pid) and any window lookups never
receive truncated values.
Comment on lines
+59
to
+64
| if let Some(parent) = output_path.parent() { | ||
| std::fs::create_dir_all(parent).ok(); | ||
| } | ||
| // SCRecordingOutput appends-or-fails on an existing file; match the | ||
| // Swift impl by clearing any stale recording.mp4 from a prior run. | ||
| let _ = std::fs::remove_file(output_path); |
Contributor
There was a problem hiding this comment.
Handle startup filesystem errors explicitly (except missing stale file).
Line 60 and Line 64 currently suppress all fs errors. That can hide real permission/path failures and produce harder-to-debug capture errors later.
Suggested patch
if let Some(parent) = output_path.parent() {
- std::fs::create_dir_all(parent).ok();
+ std::fs::create_dir_all(parent).map_err(|e| {
+ anyhow::anyhow!(
+ "failed to create recording output directory {}: {e}",
+ parent.display()
+ )
+ })?;
}
@@
- let _ = std::fs::remove_file(output_path);
+ match std::fs::remove_file(output_path) {
+ Ok(()) => {}
+ Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
+ Err(e) => {
+ anyhow::bail!(
+ "failed to remove stale recording file {}: {e}",
+ output_path.display()
+ );
+ }
+ }📝 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
| if let Some(parent) = output_path.parent() { | |
| std::fs::create_dir_all(parent).ok(); | |
| } | |
| // SCRecordingOutput appends-or-fails on an existing file; match the | |
| // Swift impl by clearing any stale recording.mp4 from a prior run. | |
| let _ = std::fs::remove_file(output_path); | |
| if let Some(parent) = output_path.parent() { | |
| std::fs::create_dir_all(parent).map_err(|e| { | |
| anyhow::anyhow!( | |
| "failed to create recording output directory {}: {e}", | |
| parent.display() | |
| ) | |
| })?; | |
| } | |
| // SCRecordingOutput appends-or-fails on an existing file; match the | |
| // Swift impl by clearing any stale recording.mp4 from a prior run. | |
| match std::fs::remove_file(output_path) { | |
| Ok(()) => {} | |
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} | |
| Err(e) => { | |
| anyhow::bail!( | |
| "failed to remove stale recording file {}: {e}", | |
| output_path.display() | |
| ); | |
| } | |
| } |
🤖 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/video_sckit.rs` around lines
59 - 64, The current startup code around output_path silences all filesystem
errors; change it so std::fs::create_dir_all(parent) returns a handled Result
(propagate or log and return Err) instead of calling .ok(), and for
std::fs::remove_file(output_path) only ignore the NotFound/DoesNotExist error
while propagating or logging any other IO errors; locate the logic referencing
output_path, the create_dir_all(parent) call and the remove_file(output_path)
call in this module and replace the blanket .ok() and unexamined let _ = ...
with explicit Result handling that surfaces permission/path failures but still
tolerates a missing stale file.
Comment on lines
+25
to
+29
| let pid = pid? as u32; | ||
| let hwnd = match window_id { | ||
| Some(w) => w, | ||
| None => crate::win32::list_windows(Some(pid)).first().map(|w| w.hwnd)?, | ||
| }; |
Contributor
There was a problem hiding this comment.
Use checked integer conversions for PID/window IDs.
The current as casts (i64 -> u32) can wrap negative or out-of-range values and point to the wrong target window/process.
Proposed fix
pub fn app_state_json_for(window_id: Option<u64>, pid: Option<i64>) -> Option<Vec<u8>> {
- let pid = pid? as u32;
+ let pid = u32::try_from(pid?).ok()?;
let hwnd = match window_id {
- Some(w) => w,
+ Some(w) => w,
None => crate::win32::list_windows(Some(pid)).first().map(|w| w.hwnd)?,
};
@@
pub fn element_window_local_xy(window_id: u64, pid: i64, element_index: u32) -> Option<(f64, f64)> {
let cache = ELEMENT_CACHE.get()?;
- let (sx, sy) = cache.get_element_center(pid as u32, window_id, element_index as usize)?;
+ let pid_u32 = u32::try_from(pid).ok()?;
+ let (sx, sy) = cache.get_element_center(pid_u32, window_id, element_index as usize)?;
@@
- let wins = crate::win32::list_windows(Some(pid as u32));
+ let wins = crate::win32::list_windows(Some(pid_u32));Also applies to: 44-49
🤖 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/recording_hooks.rs` around
lines 25 - 29, Replace unchecked "as" casts from i64 to u32 for pid and window
IDs with fallible conversions (e.g., TryFrom/try_into or i64::try_into) and
propagate a clear error when conversion fails; update the code around the pid
assignment and the window_id fallback (the block using pid? as u32 and
crate::win32::list_windows(...).first().map(|w| w.hwnd)?) to perform a checked
conversion of the i64 value to u32, returning an Err or mapping to a descriptive
error if the value is negative or out of range; apply the same fix to the other
occurrences mentioned (the similar conversion in the later block handling
window_id) so all PID/window ID conversions are safe and handled explicitly.
Comment on lines
+33
to
+38
| **Windows / Linux — ffmpeg subprocess.** Outside macOS the recorder | ||
| shells to ffmpeg with `gdigrab` (Windows) or `x11grab` (Linux). The | ||
| binary needs to be on PATH (`winget install Gyan.FFmpeg` / | ||
| `apt install ffmpeg`); when missing, the per-turn capture continues | ||
| without video and `last_error` carries the install hint. ffmpeg | ||
| startup failures fast-fail with a stderr tail in the error. |
Contributor
There was a problem hiding this comment.
Resolve the platform-support contradiction in this doc.
Line 33-Line 38 now documents Windows/Linux recording via ffmpeg, but the header at Line 3-Line 8 still says recording is macOS-only. Please make those sections consistent.
Suggested doc fix
-> **Platform: macOS-only today.** Trajectory recording / replay is
-> currently implemented on the macOS backend only. On Windows, `cua-driver
-> recording {start,stop,status}` is registered but returns "Recording is
-> currently macOS-only". On Linux (BETA): not supported. See `WINDOWS.md`
-> / `LINUX.md` for capture-state alternatives via `screenshot` and
-> `get_window_state`.
+> **Platform support:** recording is available on macOS (native
+> ScreenCaptureKit) and on Windows/Linux (ffmpeg subprocess backend).
+> Replay is cross-platform as long as required artifacts are present.📝 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
| **Windows / Linux — ffmpeg subprocess.** Outside macOS the recorder | |
| shells to ffmpeg with `gdigrab` (Windows) or `x11grab` (Linux). The | |
| binary needs to be on PATH (`winget install Gyan.FFmpeg` / | |
| `apt install ffmpeg`); when missing, the per-turn capture continues | |
| without video and `last_error` carries the install hint. ffmpeg | |
| startup failures fast-fail with a stderr tail in the error. | |
| **Platform support:** recording is available on macOS (native | |
| ScreenCaptureKit) and on Windows/Linux (ffmpeg subprocess backend). | |
| Replay is cross-platform as long as required artifacts are present. |
🤖 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/Skills/cua-driver/RECORDING.md` around lines 33 - 38,
The README record is inconsistent: change the macOS-only header in RECORDING.md
to reflect cross-platform support (rename or rewrite the header that currently
says "macOS-only" to include Windows and Linux) so it aligns with the "Windows /
Linux — ffmpeg subprocess." section; ensure the document mentions that Windows
uses gdigrab, Linux uses x11grab, ffmpeg must be on PATH (installation hints),
and that missing ffmpeg results in per-turn capture continuing without video
with last_error carrying the install hint and ffmpeg startup failures fast-fail
with a stderr tail in the error.
7 CodeRabbit nits across the recording refactor: 1. recording_tools.rs — `record_video` schema description now says macOS uses ScreenCaptureKit, only Win/Linux need ffmpeg (was contradicting the description text just above) 2. recording.rs — `element_index` u64→u32 narrowing now uses `u32::try_from(...).ok().and_then(...)` instead of silent `as` cast 3. video_ffmpeg.rs — `create_dir_all` error now propagates with context instead of being swallowed 4. platform-macos/recording_hooks.rs — i64→i32 and u64→u32 conversions for pid + window_id now use checked `try_from` 5. video_sckit.rs — fs::create_dir_all error propagates; fs::remove_file ignores only NotFound, propagates other IO errors 6. platform-windows/recording_hooks.rs — same checked conversions for pid + window_id casts 7. RECORDING.md — header no longer claims "macOS-only" since recording now ships cross-platform via SCKit/ffmpeg Build: `cargo build --release -p cua-driver` green on macOS; `cargo check -p platform-windows --target=x86_64-pc-windows-msvc` green.
2 tasks
f-trycua
added a commit
that referenced
this pull request
May 27, 2026
v0.3.0's release CD failed on darwin-universal because the `screencapturekit` crate (added in PR #1720 for the native ScreenCaptureKit recording backend) pulls in `apple-metal v0.8.7`, whose Swift bridge uses `#available(macOS 26.0, *)`-guarded code that references `MTLSamplerReductionMode`, `MTLSamplerDescriptor.reductionMode`, and `.lodBias` — symbols only present in the macOS 26 SDK. The `macos-15` runner image ships a macOS 15 SDK that doesn't expose those, so the transitive Swift bridge fails at compile time even though the offending code is gated by `if #available(macOS 26.0, *)`. macOS 26 runner images are now available in GH Actions (actions/runner-images#13739), so move the build there. Linux + Windows builds are unchanged. After this merges, re-run the bump (cua-driver-rs → 0.3.1) and the CD should produce all four artifact sets cleanly.
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.
Three regressions introduced by #1718 (the cross-platform Rust recording refactor + rename to
cua-driver-core), plus a scope-expanded follow-up that replaces ffmpeg with native ScreenCaptureKit on macOS.User reproed all three regressions live on macOS Calculator (5+3) earlier today: zero
app_state.jsonfiles written, zeroclick.pngfiles (all 5 turns usedelement_index, so the click-marker branch was dead), and a 0-byterecording.mp4withfinalized: falsewhile ffmpeg silently blocked forever on a TCC prompt the daemon couldn't surface.What changed since the first commit on this branch
The original three fixes (
app_state.json,click.pngfor element_index, ffmpeg TCC fast-fail) are unchanged. Added on top:4. macOS video now uses native ScreenCaptureKit (no ffmpeg, no extra TCC grant)
video.rsis now a thin trait abstraction (VideoBackend+VideoBackendFactory) selected at startup by each platform crate.recording.rscallsvideo::start_video(path)— the concrete backend is whatever was registered withset_video_backend_factory(mirroringSCREENSHOT_FN/AX_SNAPSHOT_FN).platform_macos::video_sckit::SckitVideoBackendFactory— in-processSCStream+SCRecordingOutput, H.264 / MP4 / 30 fps, full main display. Because ScreenCaptureKit runs in the cua-driver process, it inherits the daemon's Screen Recording grant — no separate subprocess prompt, no fast-fail hang heuristic. Requires macOS 15.0+ (SCRecordingOutput introduced in macOS 15). Backed by thescreencapturekit6.0 crate (576k downloads, safe bindings, ships a small Swift-bridge build script —cua-driver/build.rsnow bakes the Swift Concurrency rpath into the binary).cua_driver_core::video_ffmpeg::FfmpegVideoBackendFactory— the existing ffmpeg pipeline (gdigrab/x11grab), now relocated to its own module with the macOS-specific TCC-hang detection deleted (it would never run on these platforms).Live verification on macOS:
cua-driver recording start /tmp/x→ 3 s sleep →cua-driver recording stopproduces a 1.5 MB playable MP4,session.jsonreportsfinalized: true, zero TCC prompts, zero ffmpeg involvement.Closes #1721.
The three regression fixes (unchanged from the first commit)
1.
app_state.jsonnever written per turnrecording::write_turnhad no AX snapshot path. AddedAX_SNAPSHOT_FNcallback wired toplatform_macos::recording_hooks::app_state_json_for(AX walk → same shape asget_window_state) on macOS andplatform_windows::recording_hooks::app_state_json_for(UIA walk) on Windows. Linux intentionally no-ops — ATSPI has no cheap whole-tree snapshot.2.
click.pngnever written for element_index clicksThe
click_pointresolver only handled explicitx, y. AddedELEMENT_BOUNDS_FNresolvingelement_indexto window-local screenshot-pixel coords via the live AX/UIA cache (macOS: Retina scale derived from PNG width / logical width).3. ffmpeg subprocess hung on macOS TCC prompt (now obviated by SCKit on macOS, fast-fail still applies on Win/Linux)
The original fast-fail probe is now Windows/Linux-only — ffmpeg failures there fast-fail with stderr tail. On macOS the ffmpeg path is gone entirely.
Docs
Skills/cua-driver/RECORDING.md— macOS section rewritten to "native ScreenCaptureKit, zero-config, requires macOS 15.0+"; ffmpeg note retained for Windows + Linux.recording_tools.rs:START_REC_DEF(the MCP tool description served viatools/list) — same./docsfumadocsmcp-tools.mdxpage still references the olderrecordingtoggle API and is out of date overall; deferred to a separate docs sync.Test plan
cargo build --release -p cua-driveron macOS — green, binary launches with Swift rpaths baked in.cargo check -p platform-windows --target=x86_64-pc-windows-msvc— green.recording start /tmp/x→ 3 s →recording stopproduces a 1.5 MB H.264 MP4,session.jsonfinalized: true. No TCC prompt fired (Screen Recording grant inherited).app_state.json+click.pngwith a red crosshair on the clicked AXButton.app_state.json+ UIA click.png present.app_state.jsonabsent (intentional).PR stays in draft — flip ready after live macOS verification.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation