Fix macOS computer use and restored command replay
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -301,6 +301,9 @@ pub struct Block {
|
||||
render_delay_complete: Arc<AtomicBool>,
|
||||
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(),
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<dyn Actor> {
|
||||
if cfg!(feature = "test-util") {
|
||||
Box::new(noop::Actor::new())
|
||||
@@ -45,6 +48,17 @@ pub fn create_actor() -> Box<dyn Actor> {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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.
|
||||
|
||||
@@ -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<char, CGKeyCode> {
|
||||
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<char, CGKeyCode> {
|
||||
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<char, CGKeyCode> {
|
||||
unsafe fn get_keyboard_layout_data() -> Option<CFRetained<CFData>> {
|
||||
// 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<CFRetained<CFData>> {
|
||||
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) };
|
||||
}
|
||||
|
||||
@@ -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<crate::WindowInfo> {
|
||||
window::enumerate_windows()
|
||||
@@ -134,6 +138,12 @@ impl super::Actor for Actor {
|
||||
actions: &[TargetedAction],
|
||||
options: Options,
|
||||
) -> Result<ActionResult, String> {
|
||||
// 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.
|
||||
|
||||
@@ -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<crate::Screenshot, String> {
|
||||
.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."
|
||||
))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user