diff --git a/crates/core/src/launch_options.rs b/crates/core/src/launch_options.rs index 020ab609..6f9f0a33 100644 --- a/crates/core/src/launch_options.rs +++ b/crates/core/src/launch_options.rs @@ -1,10 +1,30 @@ use std::collections::HashMap; use std::path::PathBuf; -#[derive(Debug, Clone, Default)] +/// Options accepted by `launch_app_with_options`. `attach` defaults to +/// `true`, which preserves `launch_app`'s historical behavior of attaching +/// to an already-running instance instead of failing. Set it to `false` to +/// require a fresh launch: the adapter then fails with a structured error +/// naming the running pid instead of attaching. +#[derive(Debug, Clone)] pub struct LaunchOptions { pub args: Vec, pub env: HashMap, pub cwd: Option, pub attach: bool, } + +impl Default for LaunchOptions { + fn default() -> Self { + Self { + args: Vec::new(), + env: HashMap::new(), + cwd: None, + attach: true, + } + } +} + +#[cfg(test)] +#[path = "launch_options_tests.rs"] +mod tests; diff --git a/crates/core/src/launch_options_tests.rs b/crates/core/src/launch_options_tests.rs new file mode 100644 index 00000000..10274566 --- /dev/null +++ b/crates/core/src/launch_options_tests.rs @@ -0,0 +1,26 @@ +use super::*; + +#[test] +fn default_preserves_attach_if_running_semantics() { + let options = LaunchOptions::default(); + + assert!( + options.attach, + "default LaunchOptions must attach to an already-running instance, matching \ + launch_app's historical behavior; a derived Default would silently flip this to \ + false and turn every unmodified caller into a --no-attach launch" + ); + assert!(options.args.is_empty()); + assert!(options.env.is_empty()); + assert!(options.cwd.is_none()); +} + +#[test] +fn explicit_no_attach_overrides_the_default() { + let options = LaunchOptions { + attach: false, + ..Default::default() + }; + + assert!(!options.attach); +} diff --git a/crates/macos/src/adapter.rs b/crates/macos/src/adapter.rs index 6c423074..3daae0b8 100644 --- a/crates/macos/src/adapter.rs +++ b/crates/macos/src/adapter.rs @@ -301,7 +301,7 @@ impl SystemOps for MacOSAdapter { } fn launch_app(&self, id: &str, timeout_ms: u64) -> Result { - crate::system::app_ops::launch_app_impl(id, timeout_ms) + crate::system::launch::launch_app_impl(id, timeout_ms) } fn launch_app_with_options( @@ -310,7 +310,7 @@ impl SystemOps for MacOSAdapter { options: &agent_desktop_core::launch_options::LaunchOptions, timeout_ms: u64, ) -> Result { - crate::system::app_ops::launch_app_with_options_impl(id, options, timeout_ms) + crate::system::launch::launch_app_with_options_impl(id, options, timeout_ms) } fn process_state( diff --git a/crates/macos/src/system/app_ops.rs b/crates/macos/src/system/app_ops.rs index c22b03cb..5d714fec 100644 --- a/crates/macos/src/system/app_ops.rs +++ b/crates/macos/src/system/app_ops.rs @@ -1,5 +1,4 @@ use agent_desktop_core::{ - adapter::WindowFilter, error::{AdapterError, ErrorCode}, node::WindowInfo, }; @@ -97,149 +96,6 @@ pub fn focus_window_impl(_win: &WindowInfo) -> Result<(), AdapterError> { Err(AdapterError::not_supported("focus_window")) } -#[cfg(target_os = "macos")] -pub fn launch_app_with_options_impl( - id: &str, - options: &agent_desktop_core::launch_options::LaunchOptions, - timeout_ms: u64, -) -> Result { - use crate::system::window_list::list_windows_impl; - use std::process::Command; - use std::time::{Duration, Instant}; - - const OPEN_TIMEOUT: Duration = Duration::from_secs(5); - - if options.attach { - let filter = WindowFilter { - focused_only: false, - app: Some(id.to_string()), - }; - if let Ok(wins) = list_windows_impl(&filter) { - if let Some(win) = wins.into_iter().next() { - return Ok(win); - } - } - } - - let mut command = Command::new("/usr/bin/open"); - command.arg("-g").arg("-a").arg(id); - for arg in &options.args { - command.arg("--args").arg(arg); - } - if let Some(cwd) = &options.cwd { - command.current_dir(cwd); - } - for (key, value) in &options.env { - command.env(key, value); - } - crate::system::process::run_with_timeout(&mut command, "open", OPEN_TIMEOUT)?; - - let start = Instant::now(); - let timeout = Duration::from_millis(timeout_ms); - let mut poll_interval = Duration::from_millis(100); - let max_interval = Duration::from_millis(500); - loop { - std::thread::sleep(poll_interval); - let filter = WindowFilter { - focused_only: false, - app: Some(id.to_string()), - }; - if let Ok(wins) = list_windows_impl(&filter) { - if let Some(win) = wins.into_iter().next() { - return Ok(win); - } - } - if start.elapsed() > timeout { - break; - } - poll_interval = (poll_interval * 3 / 2).min(max_interval); - } - Err(AdapterError::new( - agent_desktop_core::error::ErrorCode::AppNotFound, - format!("App '{id}' launched but no window appeared within {timeout_ms} ms"), - ) - .with_suggestion("The app may take longer to start, or it may not create a visible window")) -} - -#[cfg(not(target_os = "macos"))] -pub fn launch_app_with_options_impl( - _id: &str, - _options: &agent_desktop_core::launch_options::LaunchOptions, - _timeout_ms: u64, -) -> Result { - Err(AdapterError::not_supported("launch_app_with_options")) -} - -#[cfg(target_os = "macos")] -pub fn launch_app_impl(id: &str, timeout_ms: u64) -> Result { - tracing::debug!("system: launch app={id:?} timeout={timeout_ms}ms"); - use crate::system::window_list::list_windows_impl; - use std::process::Command; - use std::time::{Duration, Instant}; - - const OPEN_TIMEOUT: Duration = Duration::from_secs(5); - - if id.contains("..") || id.starts_with('/') { - return Err(AdapterError::new( - agent_desktop_core::error::ErrorCode::InvalidArgs, - format!("Invalid app identifier: '{id}'"), - ) - .with_suggestion("Use an app name like 'Safari' or bundle ID like 'com.apple.Safari'.")); - } - - let filter = WindowFilter { - focused_only: false, - app: Some(id.to_string()), - }; - if let Ok(wins) = list_windows_impl(&filter) { - if let Some(win) = wins.into_iter().next() { - return Ok(win); - } - } - - let mut command = Command::new("/usr/bin/open"); - command.args(open_app_args(id)); - crate::system::process::run_with_timeout(&mut command, "open", OPEN_TIMEOUT)?; - - let start = Instant::now(); - let timeout = Duration::from_millis(timeout_ms); - let mut poll_interval = Duration::from_millis(100); - let max_interval = Duration::from_millis(500); - - loop { - std::thread::sleep(poll_interval); - let filter = WindowFilter { - focused_only: false, - app: Some(id.to_string()), - }; - if let Ok(wins) = list_windows_impl(&filter) { - if let Some(win) = wins.into_iter().next() { - return Ok(win); - } - } - if start.elapsed() > timeout { - break; - } - poll_interval = (poll_interval * 3 / 2).min(max_interval); - } - - Err(AdapterError::new( - agent_desktop_core::error::ErrorCode::AppNotFound, - format!("App '{id}' launched but no window appeared within {timeout_ms} ms"), - ) - .with_suggestion("The app may take longer to start, or it may not create a visible window")) -} - -#[cfg(target_os = "macos")] -fn open_app_args(id: &str) -> [&str; 3] { - ["-g", "-a", id] -} - -#[cfg(not(target_os = "macos"))] -pub fn launch_app_impl(_id: &str, _timeout_ms: u64) -> Result { - Err(AdapterError::not_supported("launch_app")) -} - /// Processes whose termination would break the macOS session: the window /// server, login session, launchd, the Dock, and Finder. Matched as an /// exact lowercase name or an exact dot-separated bundle-id component, so diff --git a/crates/macos/src/system/app_ops_tests.rs b/crates/macos/src/system/app_ops_tests.rs index 19c51b68..675fe852 100644 --- a/crates/macos/src/system/app_ops_tests.rs +++ b/crates/macos/src/system/app_ops_tests.rs @@ -23,11 +23,6 @@ fn osascript_quit_keeps_action_failed_for_other_errors() { assert_eq!(err.code, ErrorCode::ActionFailed); } -#[test] -fn open_app_args_preserve_current_focus() { - assert_eq!(open_app_args("Mail"), ["-g", "-a", "Mail"]); -} - #[test] fn protected_processes_match_display_and_bundle_forms() { assert!(is_protected_process("Finder")); diff --git a/crates/macos/src/system/launch.rs b/crates/macos/src/system/launch.rs new file mode 100644 index 00000000..f406929b --- /dev/null +++ b/crates/macos/src/system/launch.rs @@ -0,0 +1,194 @@ +use agent_desktop_core::{ + adapter::WindowFilter, + error::{AdapterError, ErrorCode}, + launch_options::LaunchOptions, + node::WindowInfo, +}; + +#[cfg(target_os = "macos")] +pub fn launch_app_with_options_impl( + id: &str, + options: &LaunchOptions, + timeout_ms: u64, +) -> Result { + use crate::system::window_list::list_windows_impl; + use std::process::Command; + use std::time::{Duration, Instant}; + + const OPEN_TIMEOUT: Duration = Duration::from_secs(5); + + let filter = WindowFilter { + focused_only: false, + app: Some(id.to_string()), + }; + if let Ok(wins) = list_windows_impl(&filter) { + if let Some(win) = wins.into_iter().next() { + if options.attach { + return Ok(win); + } + return Err(launch_conflict_error(id, win.pid)); + } + } + + let mut command = Command::new("/usr/bin/open"); + command.args(open_argv(id, &options.args)); + if let Some(cwd) = &options.cwd { + command.current_dir(cwd); + } + for (key, value) in &options.env { + command.env(key, value); + } + crate::system::process::run_with_timeout(&mut command, "open", OPEN_TIMEOUT)?; + + if !options.attach { + return Ok(WindowInfo { + id: String::new(), + title: String::new(), + app: id.to_string(), + pid: crate::system::app_list::pid_for_app_name(id).unwrap_or(0), + bounds: None, + is_focused: false, + }); + } + + let start = Instant::now(); + let timeout = Duration::from_millis(timeout_ms); + let mut poll_interval = Duration::from_millis(100); + let max_interval = Duration::from_millis(500); + loop { + std::thread::sleep(poll_interval); + let filter = WindowFilter { + focused_only: false, + app: Some(id.to_string()), + }; + if let Ok(wins) = list_windows_impl(&filter) { + if let Some(win) = wins.into_iter().next() { + return Ok(win); + } + } + if start.elapsed() > timeout { + break; + } + poll_interval = (poll_interval * 3 / 2).min(max_interval); + } + Err(launch_no_window_error(id, timeout_ms)) +} + +#[cfg(not(target_os = "macos"))] +pub fn launch_app_with_options_impl( + _id: &str, + _options: &LaunchOptions, + _timeout_ms: u64, +) -> Result { + Err(AdapterError::not_supported("launch_app_with_options")) +} + +#[cfg(target_os = "macos")] +pub fn launch_app_impl(id: &str, timeout_ms: u64) -> Result { + tracing::debug!("system: launch app={id:?} timeout={timeout_ms}ms"); + use crate::system::window_list::list_windows_impl; + use std::process::Command; + use std::time::{Duration, Instant}; + + const OPEN_TIMEOUT: Duration = Duration::from_secs(5); + + if id.contains("..") || id.starts_with('/') { + return Err(AdapterError::new( + ErrorCode::InvalidArgs, + format!("Invalid app identifier: '{id}'"), + ) + .with_suggestion("Use an app name like 'Safari' or bundle ID like 'com.apple.Safari'.")); + } + + let filter = WindowFilter { + focused_only: false, + app: Some(id.to_string()), + }; + if let Ok(wins) = list_windows_impl(&filter) { + if let Some(win) = wins.into_iter().next() { + return Ok(win); + } + } + + let mut command = Command::new("/usr/bin/open"); + command.args(open_app_args(id)); + crate::system::process::run_with_timeout(&mut command, "open", OPEN_TIMEOUT)?; + + let start = Instant::now(); + let timeout = Duration::from_millis(timeout_ms); + let mut poll_interval = Duration::from_millis(100); + let max_interval = Duration::from_millis(500); + + loop { + std::thread::sleep(poll_interval); + let filter = WindowFilter { + focused_only: false, + app: Some(id.to_string()), + }; + if let Ok(wins) = list_windows_impl(&filter) { + if let Some(win) = wins.into_iter().next() { + return Ok(win); + } + } + if start.elapsed() > timeout { + break; + } + poll_interval = (poll_interval * 3 / 2).min(max_interval); + } + + Err(launch_no_window_error(id, timeout_ms)) +} + +#[cfg(not(target_os = "macos"))] +pub fn launch_app_impl(_id: &str, _timeout_ms: u64) -> Result { + Err(AdapterError::not_supported("launch_app")) +} + +#[cfg(target_os = "macos")] +fn open_app_args(id: &str) -> [&str; 3] { + ["-g", "-a", id] +} + +/// Assembles the `open` argv for a launch with optional app-args. `--args` +/// is emitted at most once, immediately before all of `args`: `open` treats +/// everything after the first `--args` as literal argv for the launched +/// app, so repeating the flag per element (the prior bug) handed the app a +/// stray `--args` token instead of its second argument. +#[cfg(target_os = "macos")] +fn open_argv(id: &str, args: &[String]) -> Vec { + let mut argv: Vec = open_app_args(id).into_iter().map(String::from).collect(); + if !args.is_empty() { + argv.push("--args".to_string()); + argv.extend_from_slice(args); + } + argv +} + +/// The `--no-attach` conflict path: the app already owns a window, so a +/// caller that explicitly asked not to attach gets a structured refusal +/// instead of a silent re-attach. `pid` is OS-assigned and safe to name +/// directly; `id` is open-ended caller input, so it stays out of `message` +/// and travels only in `details.app_name`, which redacts on trace export. +#[cfg(target_os = "macos")] +fn launch_conflict_error(id: &str, pid: i32) -> AdapterError { + AdapterError::new( + ErrorCode::ActionFailed, + format!("App is already running as pid {pid}; refusing to launch again with --no-attach"), + ) + .with_details(serde_json::json!({ "app_name": id, "pid": pid })) + .with_suggestion("Close the running instance first, or omit --no-attach to attach to it") +} + +#[cfg(target_os = "macos")] +fn launch_no_window_error(id: &str, timeout_ms: u64) -> AdapterError { + AdapterError::new( + ErrorCode::AppNotFound, + format!("Launched app but no window appeared within {timeout_ms} ms"), + ) + .with_details(serde_json::json!({ "app_name": id })) + .with_suggestion("The app may take longer to start, or it may not create a visible window") +} + +#[cfg(test)] +#[path = "launch_tests.rs"] +mod tests; diff --git a/crates/macos/src/system/launch_tests.rs b/crates/macos/src/system/launch_tests.rs new file mode 100644 index 00000000..b776316e --- /dev/null +++ b/crates/macos/src/system/launch_tests.rs @@ -0,0 +1,96 @@ +use super::*; +use agent_desktop_core::error::ErrorCode; + +#[test] +fn open_app_args_preserve_current_focus() { + assert_eq!(open_app_args("Mail"), ["-g", "-a", "Mail"]); +} + +#[test] +fn open_argv_with_no_args_omits_the_args_flag() { + assert_eq!(open_argv("Mail", &[]), vec!["-g", "-a", "Mail"]); +} + +#[test] +fn open_argv_with_one_arg_emits_the_args_flag_once() { + let args = vec!["foo".to_string()]; + + assert_eq!( + open_argv("Mail", &args), + vec!["-g", "-a", "Mail", "--args", "foo"] + ); +} + +#[test] +fn open_argv_with_two_args_emits_exactly_one_args_flag() { + let args = vec!["foo".to_string(), "bar".to_string()]; + let argv = open_argv("Mail", &args); + + assert_eq!(argv, vec!["-g", "-a", "Mail", "--args", "foo", "bar"]); + assert_eq!( + argv.iter().filter(|a| a.as_str() == "--args").count(), + 1, + "exactly one --args flag regardless of how many app args are passed, got argv: {argv:?}" + ); +} + +#[test] +fn empty_options_argv_is_byte_identical_to_the_pre_options_launch_path() { + let empty_options_argv = open_argv("Preview", &[]); + + assert_eq!(empty_options_argv, vec!["-g", "-a", "Preview"]); + assert_eq!( + empty_options_argv, + open_app_args("Preview") + .into_iter() + .map(String::from) + .collect::>(), + "LaunchOptions with no args must produce the exact argv the options-free launch path used" + ); +} + +#[test] +fn no_attach_against_a_running_app_fails_with_a_structured_error_naming_the_pid() { + let err = launch_conflict_error("Marker9182App", 4242); + + assert_eq!(err.code, ErrorCode::ActionFailed); + assert!( + err.message.contains("4242"), + "structured --no-attach conflict must name the running pid so the caller can act on \ + it, got message: {}", + err.message + ); +} + +#[test] +fn launch_conflict_error_never_puts_the_raw_app_id_in_the_message() { + let marker = "MARKER_APP_ID_9f31c4"; + let err = launch_conflict_error(marker, 777); + + assert!( + !err.message.contains(marker), + "raw app id leaked into a trace-reachable message: {}", + err.message + ); + let details = err + .details + .expect("--no-attach conflict error should carry the app id in details"); + assert_eq!(details["app_name"], marker); +} + +#[test] +fn launch_no_window_error_never_puts_the_raw_app_id_in_the_message() { + let marker = "MARKER_APP_ID_9f31c4"; + let err = launch_no_window_error(marker, 5000); + + assert!( + !err.message.contains(marker), + "raw app id leaked into a trace-reachable message: {}", + err.message + ); + assert!(err.message.contains("5000")); + let details = err + .details + .expect("no-window error should carry the app id in details"); + assert_eq!(details["app_name"], marker); +} diff --git a/crates/macos/src/system/mod.rs b/crates/macos/src/system/mod.rs index eb1e65df..8f6ded6e 100644 --- a/crates/macos/src/system/mod.rs +++ b/crates/macos/src/system/mod.rs @@ -5,6 +5,7 @@ pub(crate) mod cg_window; pub mod display; pub(crate) mod force_close; pub mod key_dispatch; +pub mod launch; pub mod permissions; pub(crate) mod process; pub(crate) mod process_apps;