fix: correct launch argv, honor --no-attach, redact app id

--args was emitted once per --arg, handing the launched app a stray
--args token instead of its intended argument; argv assembly is now a
pure, unit-tested fn that emits --args exactly once. --no-attach was
silently ignored: launch_app_with_options_impl always attached-or-waited
regardless of the flag. It now fails with a structured error naming the
running pid when the app is already running, and returns immediately
without the window-wait loop when it is not. The raw app id no longer
lands in trace-reachable error messages; it travels only in
details.app_name, which redacts on trace export.

Split the launch path out of app_ops.rs into a new launch.rs sibling
module (app_ops.rs would otherwise exceed the 400-line cap).
This commit is contained in:
Lahfir 2026-07-03 01:10:37 -07:00
parent 26f884cf8f
commit 4e4b20e5dc
8 changed files with 340 additions and 152 deletions

View file

@ -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<String>,
pub env: HashMap<String, String>,
pub cwd: Option<PathBuf>,
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;

View file

@ -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);
}

View file

@ -301,7 +301,7 @@ impl SystemOps for MacOSAdapter {
}
fn launch_app(&self, id: &str, timeout_ms: u64) -> Result<WindowInfo, AdapterError> {
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<WindowInfo, AdapterError> {
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(

View file

@ -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<WindowInfo, AdapterError> {
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<WindowInfo, AdapterError> {
Err(AdapterError::not_supported("launch_app_with_options"))
}
#[cfg(target_os = "macos")]
pub fn launch_app_impl(id: &str, timeout_ms: u64) -> Result<WindowInfo, AdapterError> {
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<WindowInfo, AdapterError> {
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

View file

@ -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"));

View file

@ -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<WindowInfo, AdapterError> {
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<WindowInfo, AdapterError> {
Err(AdapterError::not_supported("launch_app_with_options"))
}
#[cfg(target_os = "macos")]
pub fn launch_app_impl(id: &str, timeout_ms: u64) -> Result<WindowInfo, AdapterError> {
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<WindowInfo, AdapterError> {
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<String> {
let mut argv: Vec<String> = 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;

View file

@ -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::<Vec<_>>(),
"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);
}

View file

@ -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;