From 1647791a9e6f7c35cd5222efd3a28ed5cb53e56f Mon Sep 17 00:00:00 2001 From: Ryan Ward Date: Tue, 27 Oct 2026 21:04:03 +0000 Subject: [PATCH] Fix macOS computer use and restored command replay --- .../execute/request_computer_use.rs | 22 +++--- .../action_model/execute/use_computer.rs | 6 ++ app/src/terminal/model/block.rs | 15 +++- app/src/terminal/model/blocks.rs | 6 ++ app/src/terminal/model/blocks_tests.rs | 68 +++++++++++++++++++ crates/computer_use/src/bin/use_computer.rs | 8 ++- crates/computer_use/src/lib.rs | 14 ++++ crates/computer_use/src/mac/keycode_cache.rs | 44 +++++------- crates/computer_use/src/mac/mod.rs | 10 +++ crates/computer_use/src/mac/screenshot.rs | 44 ++++++++++-- 10 files changed, 192 insertions(+), 45 deletions(-) diff --git a/app/src/ai/blocklist/action_model/execute/request_computer_use.rs b/app/src/ai/blocklist/action_model/execute/request_computer_use.rs index b1873e8b..65d8b1a6 100644 --- a/app/src/ai/blocklist/action_model/execute/request_computer_use.rs +++ b/app/src/ai/blocklist/action_model/execute/request_computer_use.rs @@ -95,15 +95,19 @@ impl RequestComputerUseExecutor { let background_enabled = FeatureFlag::BackgroundComputerUse.is_enabled(); ActionExecution::Async { execute_future: Box::pin(async move { - let result = actor - .perform_actions( - &[], - computer_use::Options { - screenshot_params, - background_enabled, - }, - ) - .await; + let result = async { + computer_use::request_screen_capture_access()?; + actor + .perform_actions( + &[], + computer_use::Options { + screenshot_params, + background_enabled, + }, + ) + .await + } + .await; (result, platform) }), on_complete: Box::new(|action_result, _ctx| match action_result { diff --git a/app/src/ai/blocklist/action_model/execute/use_computer.rs b/app/src/ai/blocklist/action_model/execute/use_computer.rs index ba5d085c..e2340b7c 100644 --- a/app/src/ai/blocklist/action_model/execute/use_computer.rs +++ b/app/src/ai/blocklist/action_model/execute/use_computer.rs @@ -44,8 +44,14 @@ impl UseComputerExecutor { // Gate per-window targeting behind the client feature flag. When off, the actor forces the // legacy full-screen path so results are identical to the pre-existing implementation. let background_enabled = FeatureFlag::BackgroundComputerUse.is_enabled(); + // Actor initialization reads the macOS keyboard layout through Carbon APIs that require + // the main thread. Construct it here, just like RequestComputerUseExecutor, before + // moving the Send actor into the background action future. + #[cfg(target_os = "macos")] + let mut actor = computer_use::create_actor(); ActionExecution::new_async( async move { + #[cfg(not(target_os = "macos"))] let mut actor = computer_use::create_actor(); match actor .perform_actions( diff --git a/app/src/terminal/model/block.rs b/app/src/terminal/model/block.rs index 005df9e4..8964989d 100644 --- a/app/src/terminal/model/block.rs +++ b/app/src/terminal/model/block.rs @@ -301,6 +301,9 @@ pub struct Block { render_delay_complete: Arc, was_long_running: AtomicBool, bootstrap_stage: BootstrapStage, + /// Snapshot replay must not emit live command lifecycle events, even when using + /// the normal block appearance after bootstrapping. + is_replaying_snapshot: bool, show_bootstrap_block: bool, show_in_band_command_blocks: bool, @@ -509,6 +512,9 @@ pub struct PromptInfo { impl From<&Block> for BlockType { fn from(block: &Block) -> Self { + if block.is_replaying_snapshot { + return BlockType::Restored; + } if block.is_for_in_band_command { return BlockType::InBandCommand; } @@ -980,6 +986,7 @@ impl Block { background_executor, event_proxy, bootstrap_stage, + is_replaying_snapshot: false, show_bootstrap_block: show_warp_bootstrap_input, show_in_band_command_blocks, show_memory_stats, @@ -1945,6 +1952,10 @@ impl Block { self.bootstrap_stage } + pub(super) fn set_replaying_snapshot(&mut self, is_replaying_snapshot: bool) { + self.is_replaying_snapshot = is_replaying_snapshot; + } + pub(super) fn should_emit_visible_bootstrap_block_event(&self) -> bool { self.bootstrap_stage == BootstrapStage::ScriptExecution && !self.visible_bootstrap_block_event_sent @@ -2986,7 +2997,9 @@ impl Block { self.header_grid.preexec(data.clone()); let is_for_in_band_command = command_executor::is_in_band_command(data.command.as_str()); - if self.bootstrap_stage() == BootstrapStage::PostBootstrapPrecmd { + if self.bootstrap_stage() == BootstrapStage::PostBootstrapPrecmd + && !self.is_replaying_snapshot + { self.event_proxy .send_terminal_event(Event::AfterBlockStarted { block_id: self.id.clone(), diff --git a/app/src/terminal/model/blocks.rs b/app/src/terminal/model/blocks.rs index 6b84bd9e..f191117c 100644 --- a/app/src/terminal/model/blocks.rs +++ b/app/src/terminal/model/blocks.rs @@ -2971,6 +2971,9 @@ impl BlockList { Some(prompt_metadata), block.is_local, ); + // Restoring a snapshot rebuilds terminal state; it does not execute its command. + // Keep this separate from bootstrap_stage, which also controls block appearance. + self.active_block_mut().set_replaying_snapshot(true); if let Some(shell_host) = &block.shell_host { self.active_block_mut().set_shell_host(shell_host.clone()); } @@ -2981,6 +2984,7 @@ impl BlockList { None => { // The start_ts is set iff the block was started. So if it's not set, we're done. self.update_active_block_height(); + self.active_block_mut().set_replaying_snapshot(false); return; } Some(start_ts) => start_ts, @@ -3035,6 +3039,7 @@ impl BlockList { None => { // The completed_ts is set iff the block was `finish`ed. So if it's not set, we're done. self.update_active_block_height(); + self.active_block_mut().set_replaying_snapshot(false); return; } Some(completed_ts) => completed_ts, @@ -3061,6 +3066,7 @@ impl BlockList { self.active_block_mut().set_prompt_snapshot(prompt_snapshot); } } + self.active_block_mut().set_replaying_snapshot(false); } /// Marks the end of the active block and creates the next block. diff --git a/app/src/terminal/model/blocks_tests.rs b/app/src/terminal/model/blocks_tests.rs index 414f99e4..afbed975 100644 --- a/app/src/terminal/model/blocks_tests.rs +++ b/app/src/terminal/model/blocks_tests.rs @@ -702,6 +702,74 @@ pub fn test_restore_completed_blocks() { )); } +#[test] +fn restored_agent_command_snapshots_do_not_emit_live_command_events() { + for shared_session in [false, true] { + let (events_tx, events_rx) = async_channel::unbounded(); + let mut block_list = new_bootstrapped_block_list( + None, + None, + ChannelEventListener::builder_for_test() + .with_terminal_events_tx(events_tx) + .build(), + ); + while events_rx.try_recv().is_ok() {} + + let conversation_id = AIConversationId::new(); + let action_id = AIAgentActionId::from("saved-command".to_owned()); + let metadata = SerializedAIMetadata::from(AgentInteractionMetadata::new_hidden( + action_id.clone(), + conversation_id, + )); + let mut snapshot = SerializedBlock::new_for_test(b"echo saved".to_vec(), b"saved".to_vec()); + snapshot.ai_metadata = Some(serde_json::to_string(&metadata).unwrap()); + snapshot.agent_view_visibility = + Some(AgentViewVisibility::new_from_conversation(conversation_id).into()); + + if shared_session { + block_list.load_shared_session_scrollback(&[snapshot.clone()]); + } else { + block_list.insert_restored_block(&snapshot); + } + + let restored = block_list.block_with_id(&snapshot.id).unwrap(); + assert_eq!(restored.requested_command_action_id(), Some(&action_id)); + assert_eq!(restored.ai_conversation_id(), Some(conversation_id)); + assert_eq!(restored.is_restored(), false); + assert_eq!(restored.completed_ts(), snapshot.completed_ts.as_ref()); + let mut restored_completion_count = 0; + while let Ok(event) = events_rx.try_recv() { + match event { + Event::AfterBlockStarted { .. } => { + panic!("snapshot replay must not start a live command") + } + Event::BlockCompleted(completed) if completed.block_id == snapshot.id => { + assert!(matches!(completed.block_type, BlockType::Restored)); + restored_completion_count += 1; + } + _ => {} + } + } + assert_eq!(restored_completion_count, 1); + + // A subsequent actual command must still drive the normal lifecycle. + insert_block(&mut block_list, "echo live", "live"); + let mut live_started = false; + let mut live_completed = false; + while let Ok(event) = events_rx.try_recv() { + match event { + Event::AfterBlockStarted { .. } => live_started = true, + Event::BlockCompleted(completed) => { + live_completed |= matches!(completed.block_type, BlockType::User(_)); + } + _ => {} + } + } + assert!(live_started); + assert!(live_completed); + } +} + #[test] pub fn test_restore_blocks_with_local_status() { let (events_tx, events_rx) = async_channel::unbounded(); diff --git a/crates/computer_use/src/bin/use_computer.rs b/crates/computer_use/src/bin/use_computer.rs index 0ef3d5b6..381d9fa1 100644 --- a/crates/computer_use/src/bin/use_computer.rs +++ b/crates/computer_use/src/bin/use_computer.rs @@ -147,7 +147,6 @@ async fn main() { } let target = cli.target(); - let mut actor = computer_use::create_actor(); let (actions, screenshot_params, output_path) = match cli.command { Command::Click { x, y, button } => { @@ -224,6 +223,13 @@ async fn main() { background_enabled: true, }; + // A manual CLI invocation also explicitly initiates computer use, so it must be able to + // request access before the actor's nonprompting checks. + if let Err(error) = computer_use::request_screen_capture_access() { + eprintln!("Error: {error}"); + std::process::exit(1); + } + let mut actor = computer_use::create_actor(); match actor.perform_actions(&actions, options).await { Ok(result) => { if let Some(pos) = result.cursor_position { diff --git a/crates/computer_use/src/lib.rs b/crates/computer_use/src/lib.rs index 9c9110cd..a3cc6156 100644 --- a/crates/computer_use/src/lib.rs +++ b/crates/computer_use/src/lib.rs @@ -37,6 +37,9 @@ pub fn is_supported_on_current_platform() -> bool { } /// Returns an actor that can perform actions on the computer. +/// +/// On macOS, create the actor on the main thread before moving it into a background action +/// future: initialization reads the keyboard layout through main-thread-only Carbon APIs. pub fn create_actor() -> Box { if cfg!(feature = "test-util") { Box::new(noop::Actor::new()) @@ -45,6 +48,17 @@ pub fn create_actor() -> Box { } } +/// Requests screen capture access when computer use is explicitly initiated. Subsequent action +/// batches only check permission and must not reopen the system permission prompt. +pub fn request_screen_capture_access() -> Result<(), String> { + #[cfg(macos)] + if !cfg!(feature = "test-util") { + return imp::request_screen_capture_access(); + } + + Ok(()) +} + /// Returns whether background, per-window control (driving a specific window without raising it /// or moving the cursor) is available on this client and OS. When false, callers should target /// the whole screen / frontmost application. diff --git a/crates/computer_use/src/mac/keycode_cache.rs b/crates/computer_use/src/mac/keycode_cache.rs index 02b6d092..d4fbecdd 100644 --- a/crates/computer_use/src/mac/keycode_cache.rs +++ b/crates/computer_use/src/mac/keycode_cache.rs @@ -3,15 +3,14 @@ //! This module provides a way to translate characters to macOS virtual keycodes. //! The translation depends on the current keyboard layout. //! -//! The Carbon APIs used for translation (`TISCopyCurrentKeyboardInputSource`, -//! `UCKeyTranslate`) are not thread-safe, so all cache builds are serialized through a -//! process-wide mutex. This matters because concurrent computer-use agents can build the cache -//! from different worker threads at the same time. +//! The Carbon Text Input Source APIs must run on the main thread. Serializing worker calls +//! through a mutex does not satisfy their internal main-queue assertion. Build the cache while +//! creating the actor on the main thread, then move the actor to a worker for input/capture. use std::collections::HashMap; use std::ptr::NonNull; -use std::sync::Mutex; +use objc2::MainThreadMarker; use objc2_core_foundation::{CFData, CFRetained, CFString, CFType}; use objc2_core_graphics::CGKeyCode; @@ -60,20 +59,17 @@ const SHIFT_MODIFIER: u32 = 1 << 1; /// Builds a character-to-keycode cache for the current keyboard layout. /// -/// The Carbon translation APIs are not thread-safe, so the build is serialized through a -/// process-wide mutex. The work runs on the calling thread rather than being dispatched to the -/// main thread: in the headless `agent run` CLI the main thread never services the GCD main -/// queue, so a synchronous main-queue dispatch would deadlock. +/// Callers must initialize the actor on the main thread. Refuse an accidental worker-thread +/// build before entering Carbon, which can abort the process rather than return an error. Do not +/// synchronously dispatch to the main queue: headless CLI callers may not service that queue. /// TODO(QUALITY-271): Store the modifier keys as well. pub fn build_cache() -> HashMap { - static BUILD_LOCK: Mutex<()> = Mutex::new(()); - let _guard = BUILD_LOCK.lock().unwrap(); - build_cache_locked() -} - -/// Builds the cache while the process-wide build lock is held, serializing access to the -/// non-thread-safe Carbon APIs. -fn build_cache_locked() -> HashMap { + let Some(_main_thread) = MainThreadMarker::new() else { + log::error!( + "Computer-use actors must be initialized on the main thread; character keycode mapping is unavailable" + ); + return HashMap::new(); + }; let mut cache = HashMap::new(); // Get the keyboard layout data. @@ -110,11 +106,7 @@ fn build_cache_locked() -> HashMap { unsafe fn get_keyboard_layout_data() -> Option> { // TISCopy* functions follow CF "Copy" semantics - caller owns the reference. // Wrap in CFRetained so they're released when dropped. - let source = unsafe { - CFRetained::from_raw( - TISCopyCurrentKeyboardInputSource().expect("Attempted to create a NULL object."), - ) - }; + let source = unsafe { CFRetained::from_raw(TISCopyCurrentKeyboardInputSource()?) }; let mut layout_data = unsafe { TISGetInputSourceProperty(&source, kTISPropertyUnicodeKeyLayoutData) }; @@ -123,12 +115,8 @@ unsafe fn get_keyboard_layout_data() -> Option> { let _layout_source; if layout_data.is_null() { // Keep this alive until we're done with layout_data. - _layout_source = unsafe { - CFRetained::from_raw( - TISCopyCurrentKeyboardLayoutInputSource() - .expect("Attempted to create a NULL object."), - ) - }; + _layout_source = + unsafe { CFRetained::from_raw(TISCopyCurrentKeyboardLayoutInputSource()?) }; layout_data = unsafe { TISGetInputSourceProperty(&_layout_source, kTISPropertyUnicodeKeyLayoutData) }; } diff --git a/crates/computer_use/src/mac/mod.rs b/crates/computer_use/src/mac/mod.rs index 2832ebba..193a4211 100644 --- a/crates/computer_use/src/mac/mod.rs +++ b/crates/computer_use/src/mac/mod.rs @@ -25,6 +25,10 @@ pub fn background_supported() -> bool { true } +pub fn request_screen_capture_access() -> Result<(), String> { + screenshot::request_access() +} + /// Enumerates the on-screen windows as crate-level [`crate::WindowInfo`] records. pub fn enumerate_windows() -> Vec { window::enumerate_windows() @@ -134,6 +138,12 @@ impl super::Actor for Actor { actions: &[TargetedAction], options: Options, ) -> Result { + // Permission can be revoked between turns. Stop before posting input when we can no + // longer observe its result; only the initial request may trigger a permission prompt. + if !actions.is_empty() || options.screenshot_params.is_some() { + screenshot::check_access()?; + } + // When background computer use is disabled, force the legacy full-screen path: ignore any // window target, deliver events through the HID tap, and treat coordinates as global // pixels. This keeps behavior byte-identical to the pre-existing implementation. diff --git a/crates/computer_use/src/mac/screenshot.rs b/crates/computer_use/src/mac/screenshot.rs index 9c0efbaf..37215402 100644 --- a/crates/computer_use/src/mac/screenshot.rs +++ b/crates/computer_use/src/mac/screenshot.rs @@ -1,9 +1,33 @@ use command::blocking::Command; use image::GenericImageView; +use objc2_core_graphics::{CGPreflightScreenCaptureAccess, CGRequestScreenCaptureAccess}; use super::util::main_display_scale_factor; use crate::{CapturedWindow, ScreenshotParams, Target}; +const SCREEN_CAPTURE_PERMISSION_ERROR: &str = "macOS Screen Recording access is unavailable for Galaxy. Enable Galaxy in System Settings > Privacy & Security > Screen Recording (or Screen & System Audio Recording), then quit and reopen Galaxy. If already enabled after an update or reinstall, toggle Galaxy off and on and reopen it."; + +/// Called only when the user initiates computer use, so macOS can register Galaxy and offer its +/// native permission prompt. A rejected request must not proceed to screen capture or input. +pub(super) fn request_access() -> Result<(), String> { + // Some macOS versions need a relaunch after approval. The actor checks access again before + // doing any work and returns the same recovery instructions if approval is not effective yet. + if CGPreflightScreenCaptureAccess() || CGRequestScreenCaptureAccess() { + Ok(()) + } else { + Err(SCREEN_CAPTURE_PERMISSION_ERROR.to_string()) + } +} + +/// Checks without prompting before every action batch, including batches without a screenshot. +pub(super) fn check_access() -> Result<(), String> { + if CGPreflightScreenCaptureAccess() { + Ok(()) + } else { + Err(SCREEN_CAPTURE_PERMISSION_ERROR.to_string()) + } +} + /// Captures a screenshot according to `params`, using the built-in macOS `screencapture` CLI. /// /// When the params target a window, that specific window is captured (without raising it) and the @@ -48,7 +72,7 @@ fn take_screen(params: ScreenshotParams) -> Result { .output() .map_err(|e| format!("Failed to run screencapture: {e}"))?; - check_status(&output)?; + check_status(&output, Target::Screen)?; crate::screenshot_utils::load_and_process_screenshot(&output_path, params) } @@ -76,7 +100,7 @@ fn take_window( .output() .map_err(|e| format!("Failed to run screencapture: {e}"))?; - check_status(&output)?; + check_status(&output, params.target)?; let image = image::ImageReader::open(&output_path) .map_err(|e| format!("Failed to open screenshot file: {e}"))? @@ -118,15 +142,23 @@ fn take_window( } /// Returns an error describing a failed `screencapture` invocation. -fn check_status(output: &std::process::Output) -> Result<(), String> { +fn check_status(output: &std::process::Output, target: Target) -> Result<(), String> { if output.status.success() { return Ok(()); } let stderr = String::from_utf8_lossy(&output.stderr); let detail = if stderr.trim().is_empty() { - format!("exit code {}", output.status) + output.status.to_string() } else { - format!("exit code {}: {}", output.status, stderr.trim()) + format!("{}: {}", output.status, stderr.trim()) }; - Err(format!("screencapture failed with {detail}")) + // A capture can still fail with permission granted, for example when the Mac is locked or + // the selected window has closed. Preserve the OS error rather than labelling it a denial. + let recovery = match target { + Target::Screen => "Make sure the Mac is unlocked and has an active display.", + Target::Window { .. } => "Make sure the selected window is still open and available.", + }; + Err(format!( + "macOS screen capture failed ({detail}). {recovery} If Screen Recording was recently enabled or Galaxy was reinstalled, quit and reopen Galaxy; verify that Screen Recording is enabled for the current Galaxy application." + )) }