From f370719a263bed83253af3d50f0e676745561b88 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Tue, 11 Aug 2026 07:04:24 -0700 Subject: [PATCH 1/6] fix(computer-use): restore text-only observation and unblock the Enter path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single Feishu "send one message" run took 73 tool round-trips. Tracing the log back, most of them came from four defects that compound. **`describe_screen` was blind on macOS.** `macos_foreground_application` built its AppleScript with a `try … end try` *block* in expression position, which AppleScript rejects at compile time (-2741). The command always exited non-zero, so the function always returned `None` — and `describe_screen` derives its target app from that value. Every call reported `foreground_application: null` and `ax_tree_text: null`. The agent reasonably concluded its own tool output was being truncated, stopped trusting results, and fell back to `screencapture` + image-analysis as a substitute for eyes: 10 extra model round-trips that returned prose instead of coordinates and misread the screen repeatedly. Replaced with an in-process `NSWorkspace.frontmostApplication` read, and did the same for `macos_ax_ui::frontmost_pid`, which shelled out to `osascript` on every call — three spawns per `describe_screen`, each ~120ms, each able to block on a System Events AppleEvent timeout. **Enter was permanently refused in text-only mode.** The stale-capture guard is cleared only by a successful capture, but a text-only `screenshot` short-circuits before capturing. So the guard latched: its own error said "call `screenshot` first", calling it changed nothing, and every `click`/Enter stayed blocked. The observed escape was the agent bypassing the tool with raw `osascript … keystroke return`, which skips every check the guard exists to enforce. `describe_screen` (the text-only equivalent of looking) and the text-only `screenshot` stub now waive it. `paste` also clears it unconditionally instead of only when `submit:true` — paste-then-Enter is the common shape and the pointer never moved. **Results were mostly duplicate.** Every `app_state` carried both `tree_text` and `app_state_nodes`, the same nodes re-serialised as verbose JSON. `render_tree_text` already emits every addressable field, nothing consumed the array, and it was 82 KB of a 107 KB result. Dropped, `node_count` kept. **`get_app_state` returned mostly closed menus.** Observing a windowless app produced 188 nodes, 180 of them menu items at zero-size off-screen frames — unclickable until the menu opens. Closed `AXMenu` subtrees are no longer walked; the container stays visible and a note points at `get_app_shortcuts`. Also: `open_app` reported `success: true` for an app running with no window, which is how this run lost ~15 calls rediscovering that `activate` does not reopen an Electron window. It now resolves the bundle id (the launch name, executable name and bundle id are routinely three different strings), polls for a window, retries via `open -b`, and reports `window_count` / `windowless`. Tests: AppleScript templates are now compile-checked with `osacompile`, which catches exactly the class of bug above without executing anything. Drive-by: `embedded_relay_host` tests reserved an ephemeral port, dropped the listener, then assumed it was still free. Harmless on an idle machine and ~80% failing once the suite spawns subprocesses. Port acquisition and the release assertions now retry. --- .../src/computer_use/desktop_host/mod.rs | 357 +++++++++++++++--- .../desktop/src/computer_use/macos_ax_dump.rs | 76 +++- .../desktop/src/computer_use/macos_ax_ui.rs | 64 +++- .../src/computer_use/macos_bg_input.rs | 126 ++++++- src/apps/desktop/src/embedded_relay_host.rs | 102 +++-- .../src/agentic/tools/computer_use_host.rs | 13 + .../implementations/computer_use_actions.rs | 34 +- .../implementations/computer_use_tool.rs | 285 +++++++++++++- .../execution/agent-runtime/src/prompt.rs | 9 + .../tool-contracts/src/computer_use.rs | 22 ++ 10 files changed, 961 insertions(+), 127 deletions(-) diff --git a/src/apps/desktop/src/computer_use/desktop_host/mod.rs b/src/apps/desktop/src/computer_use/desktop_host/mod.rs index 929303c55..defde3460 100644 --- a/src/apps/desktop/src/computer_use/desktop_host/mod.rs +++ b/src/apps/desktop/src/computer_use/desktop_host/mod.rs @@ -36,6 +36,37 @@ const STALE_CAPTURE_TOOL_MESSAGE: &str = "Computer use refused: call **`screensh static SCREENSHOT_ID_COUNTER: AtomicU64 = AtomicU64::new(1); +/// How long `open_app` waits for a freshly activated app to show up in +/// LaunchServices before giving up on resolving its pid. +#[cfg(target_os = "macos")] +const OPEN_APP_SETTLE_MS: u64 = 3_000; +/// How long `open_app` waits for the app to put a window on screen. Cold +/// Electron launches routinely need several seconds; reporting `window_count: +/// 0` too early would send the agent down a false "app is broken" path. +#[cfg(target_os = "macos")] +const OPEN_APP_WINDOW_WAIT_MS: u64 = 8_000; +#[cfg(target_os = "macos")] +const OPEN_APP_POLL_INTERVAL_MS: u64 = 150; + +/// Quote a string as an AppleScript literal. +/// +/// App names reach us from the model and can contain quotes or backslashes; +/// interpolating them raw would let a name break out of the string and change +/// what the script does. +#[cfg(target_os = "macos")] +fn applescript_quote(s: &str) -> String { + let mut out = String::with_capacity(s.len() + 2); + out.push('"'); + for ch in s.chars() { + if ch == '"' || ch == '\\' { + out.push('\\'); + } + out.push(ch); + } + out.push('"'); + out +} + #[cfg(test)] mod visual_grid_tests { use super::*; @@ -109,6 +140,96 @@ mod visual_grid_tests { } } +#[cfg(all(test, target_os = "macos"))] +mod macos_applescript_tests { + use super::*; + + /// Compile an AppleScript source **without running it**. `osacompile` + /// reports the same syntax errors `osascript` would, so this checks that a + /// template is valid AppleScript with no side effects. + fn compiles(script: &str) -> Result<(), String> { + let out = std::process::Command::new("/usr/bin/osacompile") + .args(["-o", "/dev/null", "-e", script]) + .output() + .map_err(|e| format!("spawn osacompile: {e}"))?; + if out.status.success() { + Ok(()) + } else { + Err(String::from_utf8_lossy(&out.stderr).trim().to_string()) + } + } + + /// The bug that motivated this test: the frontmost-app lookup embedded a + /// `try … end try` **block** in expression position. AppleScript rejects + /// that at compile time, so the command always exited non-zero and the + /// caller silently saw `None` — for every call, forever. Nothing in the + /// build or the test suite noticed, because an AppleScript template is just + /// a string until something runs it. + /// + /// Every AppleScript this module generates now has to compile. + #[test] + fn every_generated_applescript_compiles() { + let templates = [ + format!("id of application {}", applescript_quote("Safari")), + format!( + "tell application {} to activate", + applescript_quote("Safari") + ), + ]; + for t in templates { + assert!(compiles(&t).is_ok(), "template failed to compile: {t}"); + } + } + + #[test] + fn applescript_compile_check_actually_rejects_bad_syntax() { + // Guards the guard: if `compiles` ever silently passed everything, the + // test above would be worthless. This is the exact broken spelling. + let broken = r#"tell application "System Events" + return (try (bundle identifier of p as text) on error "" end try) +end tell"#; + assert!(compiles(broken).is_err()); + } + + #[test] + fn applescript_quote_escapes_quotes_and_backslashes() { + // App names come from the model, so a name containing a quote must not + // be able to terminate the literal and change what the script does. + assert_eq!(applescript_quote("Safari"), "\"Safari\""); + assert_eq!(applescript_quote("a\"b"), "\"a\\\"b\""); + assert_eq!(applescript_quote("a\\b"), "\"a\\\\b\""); + assert_eq!(applescript_quote("飞书"), "\"飞书\""); + } + + #[test] + fn quoted_app_names_stay_inside_the_literal() { + // `" to activate` + a payload would otherwise become script code. + let hostile = "X\" to activate\ntell application \"Calculator"; + let script = format!( + "tell application {} to activate", + applescript_quote(hostile) + ); + assert!( + !script.contains("tell application \"Calculator\""), + "injected tell survived quoting: {script}" + ); + } + + /// The foreground lookup must return a real app in a GUI session. Ignored + /// by default because it needs a logged-in window server. + #[test] + #[ignore] + fn frontmost_application_resolves_in_a_gui_session() { + let app = DesktopComputerUseHost::macos_foreground_application() + .expect("a GUI session always has a frontmost application"); + assert!(app.process_id.unwrap_or(0) > 0); + assert!( + app.name.is_some() || app.bundle_id.is_some(), + "frontmost app must be identifiable: {app:?}" + ); + } +} + #[cfg(all(test, target_os = "windows"))] mod windows_foreground_tests { use super::*; @@ -128,8 +249,11 @@ mod windows_foreground_tests { #[test] fn foreground_app_falls_back_to_title_only_when_process_lookup_fails() { - let app = - DesktopComputerUseHost::windows_foreground_application("Search".to_string(), 4242, None); + let app = DesktopComputerUseHost::windows_foreground_application( + "Search".to_string(), + 4242, + None, + ); assert_eq!(app.name.as_deref(), Some("Search")); assert_eq!(app.process_name, None); @@ -411,33 +535,169 @@ impl DesktopComputerUseHost { } } + /// Launch (or re-front) a macOS app and report enough identity for the + /// agent to keep working with it. + /// + /// Three things the previous implementation got wrong, each of which cost + /// the agent a long recovery detour: + /// + /// 1. It reported only a pid. The name the caller launches by, the + /// executable name and the bundle id are frequently three different + /// strings (`Lark` / `Feishu` / `com.electron.lark`), so every follow-up + /// `tell process "…"` or `open -a …` guessed wrong. + /// 2. It slept a flat `delay 1` and declared success, whether or not a + /// window ever appeared. + /// 3. `activate` does not reopen a window for an app that is already + /// running with none — the usual state for an Electron client the user + /// closed earlier. The result was `success: true` with an empty screen. + /// + /// So: resolve the bundle id via LaunchServices, activate, poll for a + /// window, and re-open the bundle when the poll comes up empty. #[cfg(target_os = "macos")] - fn macos_foreground_application() -> Option { - let out = std::process::Command::new("/usr/bin/osascript") - .args(["-e", r#"tell application "System Events" - set p to first process whose frontmost is true - return (unix id of p as text) & "|" & (name of p) & "|" & (try (bundle identifier of p as text) on error "" end try) -end tell"#]) + fn open_app_macos( + name: String, + ) -> BitFunResult { + use crate::computer_use::macos_bg_input::running_app_identity_macos; + use bitfun_core::agentic::tools::computer_use_host::OpenAppResult; + + let failure = |err: String| OpenAppResult { + app_name: name.clone(), + success: false, + process_id: None, + error_message: Some(err), + bundle_id: None, + process_name: None, + window_count: None, + launch_path: None, + }; + + // `id of application "X"` asks LaunchServices to resolve the name the + // same way `tell application "X"` will, so the bundle id we report is + // guaranteed to describe the app we are about to activate. + let bundle_id = std::process::Command::new("/usr/bin/osascript") + .args([ + "-e", + &format!("id of application {}", applescript_quote(&name)), + ]) + .output() + .ok() + .filter(|o| o.status.success()) + .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) + .filter(|s| !s.is_empty()); + + let activate = std::process::Command::new("/usr/bin/osascript") + .args([ + "-e", + &format!("tell application {} to activate", applescript_quote(&name)), + ]) .output() - .ok()?; - if !out.status.success() { - return None; - } - let s = String::from_utf8_lossy(&out.stdout); - let parts: Vec<&str> = s.trim().splitn(3, '|').collect(); - if parts.len() < 2 { - return None; - } - let pid = parts[0].trim().parse::().ok()?; - let name = parts[1].trim(); - let bundle = parts.get(2).map(|x| x.trim()).filter(|x| !x.is_empty()); + .map_err(|e| BitFunError::tool(format!("open_app osascript: {}", e)))?; + if !activate.status.success() { + return Ok(failure( + String::from_utf8_lossy(&activate.stderr).trim().to_string(), + )); + } + + let mut launch_path = "activate"; + // Resolving by bundle id beats "whoever is frontmost right now" — + // activation is asynchronous, so the frontmost app during the first + // poll ticks is often still the previous one. + let mut pid = Self::poll_for_app_pid(bundle_id.as_deref(), OPEN_APP_SETTLE_MS); + let mut window_count = Self::poll_for_window(pid, OPEN_APP_WINDOW_WAIT_MS); + + // Alive but windowless: `open -b` asks the app to reopen its main + // window (AppKit `applicationShouldHandleReopen:`), which `activate` + // alone never triggers. + if window_count == Some(0) { + if let Some(bid) = bundle_id.as_deref() { + let reopened = std::process::Command::new("/usr/bin/open") + .args(["-b", bid]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false); + if reopened { + launch_path = "reopen_bundle"; + pid = Self::poll_for_app_pid(Some(bid), OPEN_APP_WINDOW_WAIT_MS).or(pid); + window_count = Self::poll_for_window(pid, OPEN_APP_WINDOW_WAIT_MS); + } + } + } + + let (localized_name, resolved_bundle) = pid + .and_then(running_app_identity_macos) + .unwrap_or((None, None)); + + Ok(OpenAppResult { + app_name: name, + success: true, + process_id: pid, + error_message: None, + bundle_id: resolved_bundle.or(bundle_id), + process_name: localized_name, + window_count, + launch_path: Some(launch_path.to_string()), + }) + } + + /// Poll until the app owning `bundle_id` is running (or the frontmost app + /// settles, when no bundle id could be resolved). Returns its pid. + #[cfg(target_os = "macos")] + fn poll_for_app_pid(bundle_id: Option<&str>, budget_ms: u64) -> Option { + use crate::computer_use::macos_bg_input::{frontmost_pid_macos, pid_for_bundle_id_macos}; + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(budget_ms); + loop { + let found = match bundle_id { + Some(bid) => pid_for_bundle_id_macos(bid), + None => frontmost_pid_macos(), + }; + if found.is_some() { + return found; + } + if std::time::Instant::now() >= deadline { + return None; + } + std::thread::sleep(std::time::Duration::from_millis(OPEN_APP_POLL_INTERVAL_MS)); + } + } + + /// Poll until the app owns at least one window, or the budget expires. + /// Returns the final observed count so callers can report `Some(0)` — a + /// windowless-but-alive app is a real state, not a failure to measure. + #[cfg(target_os = "macos")] + fn poll_for_window(pid: Option, budget_ms: u64) -> Option { + use crate::computer_use::macos_ax_ui::window_count_for_pid; + let pid = pid?; + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(budget_ms); + let mut last = window_count_for_pid(pid); + while last.unwrap_or(0) == 0 && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(OPEN_APP_POLL_INTERVAL_MS)); + last = window_count_for_pid(pid); + } + last + } + + /// Identity of the frontmost macOS application, read from `NSWorkspace`. + /// + /// This used to shell out to `osascript`. That spelling embedded a + /// `try … end try` **block** in expression position, which AppleScript + /// rejects at compile time (`-2741`), so the command always exited + /// non-zero and this function always returned `None`. Because + /// `describe_screen` derives its target app from this value, the text-only + /// observation path was permanently blind: it reported + /// `foreground_application: null` and `ax_tree_text: null` on every call, + /// and the agent could only conclude its own output was being truncated. + #[cfg(target_os = "macos")] + fn macos_foreground_application() -> Option { + let app = crate::computer_use::macos_bg_input::frontmost_app_identity_macos()?; Some(ComputerUseForegroundApplication { - name: Some(name.to_string()), - // `name of p` from System Events is already the process name, not a - // window title, so it doubles as the process identity here. - process_name: Some(name.to_string()), - bundle_id: bundle.map(|b| b.to_string()), - process_id: Some(pid), + name: app.name.clone(), + // `localizedName` is the app's user-visible name ("飞书"), which on + // localised or re-branded bundles differs from both the executable + // name ("Feishu") and the bundle name ("Lark"). Callers that need + // to address the process by name should prefer `bundle_id`. + process_name: app.name, + bundle_id: app.bundle_id, + process_id: Some(app.pid), }) } @@ -477,7 +737,11 @@ end tell"#]) } else { crate::computer_use::windows_list_apps::exe_basename_for_pid(pid) }; - Some(Self::windows_foreground_application(title, pid, exe_basename)) + Some(Self::windows_foreground_application( + title, + pid, + exe_basename, + )) }; ComputerUseSessionSnapshot { @@ -1176,37 +1440,7 @@ impl ComputerUseHost for DesktopComputerUseHost { #[cfg(target_os = "macos")] { let result = tokio::task::spawn_blocking(move || -> BitFunResult { - let output = std::process::Command::new("/usr/bin/osascript") - .args([ - "-e", - &format!( - r#"tell application "{}" to activate -delay 1 -tell application "System Events" to get unix id of first process whose frontmost is true"#, - name - ), - ]) - .output() - .map_err(|e| BitFunError::tool(format!("open_app osascript: {}", e)))?; - - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - let pid = stdout.trim().parse::().ok(); - Ok(OpenAppResult { - app_name: name, - success: true, - process_id: pid, - error_message: None, - }) - } else { - let stderr = String::from_utf8_lossy(&output.stderr); - Ok(OpenAppResult { - app_name: name, - success: false, - process_id: None, - error_message: Some(stderr.trim().to_string()), - }) - } + Self::open_app_macos(name) }) .await .map_err(|e| BitFunError::tool(e.to_string()))??; @@ -1398,6 +1632,13 @@ tell application "System Events" to get unix id of first process whose frontmost } } + fn computer_use_waive_fresh_capture_guard(&self) { + if let Ok(mut s) = self.state.lock() { + s.click_needs_fresh_screenshot = false; + s.pending_verify_screenshot = false; + } + } + fn computer_use_guard_click_allowed(&self) -> BitFunResult<()> { let s = self .state diff --git a/src/apps/desktop/src/computer_use/macos_ax_dump.rs b/src/apps/desktop/src/computer_use/macos_ax_dump.rs index 7298eeffb..8d8461ed7 100644 --- a/src/apps/desktop/src/computer_use/macos_ax_dump.rs +++ b/src/apps/desktop/src/computer_use/macos_ax_dump.rs @@ -431,6 +431,15 @@ pub(super) struct DumpOpts { pub max_depth: u32, pub max_nodes: usize, pub focus_window_only: bool, + /// Walk into menus that are currently closed. Off by default. + /// + /// A closed `AXMenu` still reports its whole item hierarchy, but every item + /// comes back collapsed at a zero-size off-screen frame — unclickable until + /// the menu is opened, and useless for addressing. They dominate the dump + /// anyway: observing a windowless app produced 188 nodes, 180 of them + /// closed menu items. `get_app_shortcuts` is the supported way to read menu + /// structure (and walks menus itself), so `get_app_state` stops at the menu. + pub include_closed_menus: bool, } impl Default for DumpOpts { @@ -439,10 +448,28 @@ impl Default for DumpOpts { max_depth: 32, max_nodes: 4_000, focus_window_only: false, + include_closed_menus: false, } } } +/// Whether a node is a menu container that is not currently open, and whose +/// children are therefore off-screen and unclickable. +/// +/// macOS gives an open menu's items real on-screen frames; a closed one leaves +/// them at a zero-size origin. Size is the reliable signal here — `AXExpanded` +/// is not exposed consistently by `AXMenu` across apps. +fn is_closed_menu_container(role: &str, frame: Option<(f64, f64, f64, f64)>) -> bool { + if role != "AXMenu" { + return false; + } + match frame { + // No frame at all: treat as closed. + None => true, + Some((_, _, w, h)) => w < 1.0 || h < 1.0, + } +} + pub(super) fn dump_app_ax(pid: i32, opts: DumpOpts) -> BitFunResult { let app = unsafe { AXUIElementCreateApplication(pid) }; if app.is_null() { @@ -501,6 +528,7 @@ pub(super) fn dump_app_ax(pid: i32, opts: DumpOpts) -> BitFunResult opts.max_depth || visited >= opts.max_nodes { @@ -530,10 +558,13 @@ pub(super) fn dump_app_ax(pid: i32, opts: DumpOpts) -> BitFunResult BitFunResult BitFunResult 0 { + tree_text.push_str(&format!( + "\n[note] {} closed menu subtree(s) omitted — their items are off-screen and \ +unclickable until the menu opens. Use `get_app_shortcuts` for menu commands and their key \ +equivalents, or AXPress the menu first.\n", + pruned_menu_subtrees + )); + } let digest = compute_digest(&nodes); let captured_at_ms = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -887,6 +937,28 @@ mod tests { assert!(out.contains("actions=[AXShowMenu]")); } + #[test] + fn closed_menus_are_pruned_but_open_ones_are_kept() { + // A closed menu reports its items at a zero-size off-screen frame. + assert!(is_closed_menu_container( + "AXMenu", + Some((0.0, 982.0, 0.0, 0.0)) + )); + assert!(is_closed_menu_container("AXMenu", None)); + // An open menu has a real frame and must still be walked. + assert!(!is_closed_menu_container( + "AXMenu", + Some((100.0, 40.0, 220.0, 380.0)) + )); + // Only menus are ever pruned — a zero-size button is still a node the + // model may need to reason about. + assert!(!is_closed_menu_container( + "AXButton", + Some((0.0, 0.0, 0.0, 0.0)) + )); + assert!(!is_closed_menu_container("AXMenuItem", None)); + } + #[test] fn quote_clip_truncates_on_char_boundary() { let s = "中文字符测试abcdef"; diff --git a/src/apps/desktop/src/computer_use/macos_ax_ui.rs b/src/apps/desktop/src/computer_use/macos_ax_ui.rs index 721b6a1e4..e2b899455 100644 --- a/src/apps/desktop/src/computer_use/macos_ax_ui.rs +++ b/src/apps/desktop/src/computer_use/macos_ax_ui.rs @@ -48,24 +48,17 @@ unsafe extern "C" { const K_AX_VALUE_CGPOINT: u32 = 1; const K_AX_VALUE_CGSIZE: u32 = 2; +/// Pid of the frontmost application, via `NSWorkspace.frontmostApplication`. +/// +/// Reads in-process in microseconds. The previous implementation shelled out to +/// `osascript` on every call — and this is on the hot path for +/// `describe_screen` (which reaches it up to three times per call), so a single +/// observation used to cost several hundred milliseconds of process spawns plus +/// the risk of a System Events AppleEvent timeout. fn frontmost_pid() -> BitFunResult { - let out = std::process::Command::new("/usr/bin/osascript") - .args([ - "-e", - "tell application \"System Events\" to get unix id of first process whose frontmost is true", - ]) - .output() - .map_err(|e| BitFunError::tool(format!("osascript spawn: {}", e)))?; - if !out.status.success() { - return Err(BitFunError::tool(format!( - "osascript failed: {}", - String::from_utf8_lossy(&out.stderr) - ))); - } - let s = String::from_utf8_lossy(&out.stdout); - s.trim() - .parse::() - .map_err(|_| BitFunError::tool("Could not parse frontmost process id.".to_string())) + crate::computer_use::macos_bg_input::frontmost_pid_macos().ok_or_else(|| { + BitFunError::tool("NSWorkspace reported no frontmost application.".to_string()) + }) } unsafe fn ax_release(v: CFTypeRef) { @@ -525,8 +518,10 @@ unsafe fn is_ax_hidden(elem: AXUIElementRef) -> bool { return false; // No AXHidden attribute = not hidden }; // AXHidden is a CFBoolean - let hidden = - std::ptr::eq(val, core_foundation::boolean::kCFBooleanTrue as *const c_void); + let hidden = std::ptr::eq( + val, + core_foundation::boolean::kCFBooleanTrue as *const c_void, + ); ax_release(val); hidden } @@ -1123,6 +1118,37 @@ pub(super) fn frontmost_window_bounds_global() -> BitFunResult<(i32, i32, u32, u window_bounds_global_for_pid(pid) } +/// Number of windows the app currently owns, per the AX `AXWindows` attribute. +/// +/// A launched-but-windowless app — common for Electron clients whose window was +/// closed while the process kept running — is otherwise indistinguishable from a +/// healthy launch: `open_app` reports `success: true` with a live pid while +/// there is nothing on screen to act on. Returning the count lets `open_app` +/// detect that case and re-open the app instead of leaving the agent to +/// discover it by trial and error. +/// +/// `None` means the AX handle could not be created at all (dead pid, no +/// Accessibility trust); `Some(0)` means the app is alive with no windows. +pub(super) fn window_count_for_pid(pid: i32) -> Option { + // SAFETY: `AXUIElementCreateApplication` accepts any pid and returns null + // rather than an invalid handle, which we check before use. + let app = unsafe { AXUIElementCreateApplication(pid) }; + if app.is_null() { + return None; + } + // SAFETY: `app` is a live non-null AXUIElementRef we own. `ax_copy_attr` + // follows the CF *Copy* rule, so the returned array carries a +1 retain that + // `wrap_under_create_rule` takes over. `app` is released as soon as the + // attribute has been copied out of it, on both the Some and None paths. + unsafe { + let arr_ref = ax_copy_attr(app, "AXWindows"); + ax_release(app as CFTypeRef); + let arr_ref = arr_ref?; + let arr = CFArray::<*const c_void>::wrap_under_create_rule(arr_ref as CFArrayRef); + Some(arr.len() as usize) + } +} + /// Bounds of the selected app's focused or main window in global screen coordinates. pub(super) fn window_bounds_global_for_pid(pid: i32) -> BitFunResult<(i32, i32, u32, u32)> { let app = unsafe { AXUIElementCreateApplication(pid) }; diff --git a/src/apps/desktop/src/computer_use/macos_bg_input.rs b/src/apps/desktop/src/computer_use/macos_bg_input.rs index 092c71606..33a9f5872 100644 --- a/src/apps/desktop/src/computer_use/macos_bg_input.rs +++ b/src/apps/desktop/src/computer_use/macos_bg_input.rs @@ -388,8 +388,39 @@ pub(super) fn bg_click( /// Returns `None` when the AppKit lookup is not available (e.g. headless tests /// or non-main-thread contexts where we don't want to assert). pub(super) fn frontmost_pid_macos() -> Option { + frontmost_app_identity_macos().map(|id| id.pid) +} + +/// Identity of the macOS frontmost application, read straight from +/// `NSWorkspace.frontmostApplication`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct MacFrontmostApp { + pub pid: i32, + /// `NSRunningApplication.localizedName` — what the user sees in the menu + /// bar (e.g. "飞书"), which is **not** always the executable or bundle + /// name (`Feishu` / `Lark.app`). + pub name: Option, + pub bundle_id: Option, +} + +/// Best-effort identity (pid + localized name + bundle id) of the frontmost +/// application. +/// +/// This deliberately avoids `osascript`: the previous AppleScript spelling +/// (`tell application "System Events" to … first process whose frontmost is +/// true`) cost a process spawn on every single tool result, could block on an +/// AppleEvent timeout when System Events was busy, and — because it embedded a +/// `try … end try` block in expression position — never actually compiled, so +/// the caller silently saw `None` forever. `NSWorkspace` answers in-process in +/// microseconds and needs no Automation permission. +pub(super) fn frontmost_app_identity_macos() -> Option { use objc2::msg_send; use objc2::runtime::AnyObject; + // SAFETY: every selector is sent to a class/instance that was just checked + // non-null. `sharedWorkspace`, `frontmostApplication`, `localizedName` and + // `bundleIdentifier` are all +0 (autoreleased/borrowed) returns, so nothing + // here owns a retain to balance. `NSWorkspace.frontmostApplication` is + // documented as safe to read from any thread. unsafe { let cls = objc2::runtime::AnyClass::get(c"NSWorkspace")?; let ws: *mut AnyObject = msg_send![cls, sharedWorkspace]; @@ -402,9 +433,102 @@ pub(super) fn frontmost_pid_macos() -> Option { } let pid: i32 = msg_send![app, processIdentifier]; if pid <= 0 { + return None; + } + let name: *mut AnyObject = msg_send![app, localizedName]; + let bundle: *mut AnyObject = msg_send![app, bundleIdentifier]; + Some(MacFrontmostApp { + pid, + name: ns_string_to_rust(name), + bundle_id: ns_string_to_rust(bundle), + }) + } +} + +/// Pid of a running application with the given bundle identifier, preferring +/// the most recently activated instance. `None` when nothing with that bundle +/// id is running. +pub(super) fn pid_for_bundle_id_macos(bundle_id: &str) -> Option { + use objc2::msg_send; + use objc2::runtime::AnyObject; + use objc2_foundation::NSString; + // SAFETY: `runningApplicationsWithBundleIdentifier:` returns a +0 NSArray; + // indices stay in `0..count` and every element is null-checked before use. + unsafe { + let cls = objc2::runtime::AnyClass::get(c"NSRunningApplication")?; + let ns_bundle = NSString::from_str(bundle_id); + let arr: *mut AnyObject = + msg_send![cls, runningApplicationsWithBundleIdentifier: &*ns_bundle]; + if arr.is_null() { + return None; + } + let count: usize = msg_send![arr, count]; + // Prefer an instance that already owns windows; fall back to the first. + let mut fallback: Option = None; + for i in 0..count { + let app: *mut AnyObject = msg_send![arr, objectAtIndex: i]; + if app.is_null() { + continue; + } + let pid: i32 = msg_send![app, processIdentifier]; + if pid <= 0 { + continue; + } + if fallback.is_none() { + fallback = Some(pid); + } + if crate::computer_use::macos_ax_ui::window_count_for_pid(pid).unwrap_or(0) > 0 { + return Some(pid); + } + } + fallback + } +} + +/// Localized name and bundle id of a running application, by pid. +pub(super) fn running_app_identity_macos(pid: i32) -> Option<(Option, Option)> { + use objc2::msg_send; + use objc2::runtime::AnyObject; + // SAFETY: `runningApplicationWithProcessIdentifier:` returns nil for an + // unknown pid, which is checked; the two property reads are +0 returns. + unsafe { + let cls = objc2::runtime::AnyClass::get(c"NSRunningApplication")?; + let app: *mut AnyObject = msg_send![cls, runningApplicationWithProcessIdentifier: pid]; + if app.is_null() { + return None; + } + let name: *mut AnyObject = msg_send![app, localizedName]; + let bundle: *mut AnyObject = msg_send![app, bundleIdentifier]; + Some((ns_string_to_rust(name), ns_string_to_rust(bundle))) + } +} + +/// Copy an `NSString *` into an owned Rust `String`. Returns `None` for a null +/// pointer or a string whose UTF-8 buffer is unavailable. +/// +/// # Safety +/// `s` must be null or a valid `NSString` pointer. +pub(super) unsafe fn ns_string_to_rust(s: *mut objc2::runtime::AnyObject) -> Option { + use objc2::msg_send; + if s.is_null() { + return None; + } + // SAFETY: `s` is a valid NSString per this function's contract, checked + // non-null above. `UTF8String` hands back a NUL-terminated buffer owned by + // the autorelease pool; `CStr::to_string_lossy().into_owned()` copies out of + // it before returning, so nothing borrows the pool past this block. + unsafe { + let utf8: *const std::os::raw::c_char = msg_send![s, UTF8String]; + if utf8.is_null() { + return None; + } + let out = std::ffi::CStr::from_ptr(utf8) + .to_string_lossy() + .into_owned(); + if out.is_empty() { None } else { - Some(pid) + Some(out) } } } diff --git a/src/apps/desktop/src/embedded_relay_host.rs b/src/apps/desktop/src/embedded_relay_host.rs index 1c1a6f243..72fd79376 100644 --- a/src/apps/desktop/src/embedded_relay_host.rs +++ b/src/apps/desktop/src/embedded_relay_host.rs @@ -206,6 +206,56 @@ mod tests { .port() } + /// Assert `port` became bindable again, i.e. the host really did drop its + /// listener. + /// + /// A single bind attempt conflates two different things: our host leaking + /// the listener, and some other socket on the machine transiently holding + /// the port — it came from the ephemeral range, so a busy test run reissues + /// it constantly. Retrying separates them: a leaked listener is held until + /// the process exits and never frees up, while a transient steal clears in + /// milliseconds. + async fn assert_port_released(port: u16, what: &str) { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + let mut last_err = None; + loop { + match tokio::net::TcpListener::bind(("0.0.0.0", port)).await { + Ok(l) => { + drop(l); + return; + } + Err(e) => last_err = Some(e), + } + if std::time::Instant::now() >= deadline { + panic!("{what}: port {port} never became bindable again: {last_err:?}"); + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + } + + /// Start `host` on a port nothing else has taken, returning that port. + /// + /// `unused_port` can only report a port that was free a moment ago: it + /// binds an ephemeral port, reads the number and drops the listener, so + /// anything else on the machine may claim it before the caller binds. A + /// single attempt is a race that stays invisible on a quiet machine and + /// fails most of the time when the rest of the suite is busy enough to + /// churn through ephemeral ports. Retry rather than assume. + async fn start_on_free_port( + host: &DesktopEmbeddedRelayHost, + static_dir: Option, + ) -> u16 { + let mut last_err = String::new(); + for _ in 0..16 { + let port = unused_port().await; + match host.start(port, static_dir.clone()).await { + Ok(()) => return port, + Err(e) => last_err = e.to_string(), + } + } + panic!("could not find a free port for the embedded relay: {last_err}"); + } + #[tokio::test] async fn bind_failure_does_not_create_an_active_runtime() { let occupied = tokio::net::TcpListener::bind("0.0.0.0:0") @@ -241,11 +291,8 @@ mod tests { std::fs::write(static_dir.join("assets").join("app.js"), "test asset") .expect("test asset should be written"); - let port = unused_port().await; let host = DesktopEmbeddedRelayHost::default(); - host.start(port, Some(static_dir.to_string_lossy().into_owned())) - .await - .expect("embedded relay should start"); + let port = start_on_free_port(&host, Some(static_dir.to_string_lossy().into_owned())).await; let client = reqwest::Client::new(); let index = client @@ -290,35 +337,44 @@ mod tests { .expect("embedded relay should restart immediately on the same port"); host.stop().await; - let released = tokio::net::TcpListener::bind(("0.0.0.0", port)) - .await - .expect("stop must release the listener before returning"); - drop(released); + assert_port_released(port, "stop must release the listener before returning").await; std::fs::remove_dir_all(&static_dir).expect("test static directory should be removed"); } #[tokio::test] async fn cancelled_start_releases_listener_without_committing_runtime() { - let port = unused_port().await; + // Same port race as `start_on_free_port`, but this test aborts `start` + // mid-flight and so cannot use its success as the signal: a port stolen + // between reservation and bind shows up here as readiness never firing. + // Retry until we get a port `start` could actually take. let host = Arc::new(DesktopEmbeddedRelayHost::default()); - let start_task = tokio::spawn({ - let host = host.clone(); - async move { host.start(port, None).await } - }); + let mut acquired: Option<(u16, tokio::task::JoinHandle<_>)> = None; + for _ in 0..16 { + let port = unused_port().await; + let start_task = tokio::spawn({ + let host = host.clone(); + async move { host.start(port, None).await } + }); + if tokio::time::timeout( + std::time::Duration::from_secs(1), + host.start_candidate_ready.notified(), + ) + .await + .is_ok() + { + acquired = Some((port, start_task)); + break; + } + start_task.abort(); + let _ = start_task.await; + } + let (port, start_task) = + acquired.expect("start should create the candidate runtime before readiness completes"); - tokio::time::timeout( - std::time::Duration::from_secs(1), - host.start_candidate_ready.notified(), - ) - .await - .expect("start should create the candidate runtime before readiness completes"); start_task.abort(); let _ = start_task.await; assert!(host.runtime.lock().await.is_none()); - let released = tokio::net::TcpListener::bind(("0.0.0.0", port)) - .await - .expect("cancelling start must release the listener"); - drop(released); + assert_port_released(port, "cancelling start must release the listener").await; } } diff --git a/src/crates/assembly/core/src/agentic/tools/computer_use_host.rs b/src/crates/assembly/core/src/agentic/tools/computer_use_host.rs index 88b02c649..8bf08aa44 100644 --- a/src/crates/assembly/core/src/agentic/tools/computer_use_host.rs +++ b/src/crates/assembly/core/src/agentic/tools/computer_use_host.rs @@ -210,6 +210,19 @@ pub trait ComputerUseHost: Send + Sync + std::fmt::Debug { /// is not blocked solely because of a prior click / scroll. fn computer_use_trust_pointer_after_text_input(&self) {} + /// Clear the stale-capture guard because it **cannot be satisfied** on this + /// run, not because the pointer became trustworthy. + /// + /// The guard exists to force a fresh look before a committing action. That + /// only means something for a model that can look. When the primary model + /// is text-only, `screenshot` returns no image and never reaches + /// `transition_after_screenshot`, so the guard latches on forever: the + /// error says "call `screenshot` first", the model calls `screenshot`, + /// nothing changes, and every `click` / Enter `key_chord` is refused for the + /// rest of the session. Text-only observation (`describe_screen`) is the + /// real equivalent of a capture there, so it waives the guard instead. + fn computer_use_waive_fresh_capture_guard(&self) {} + /// Refuse `mouse_click` if the pointer moved (or a click happened) since the last screenshot, /// or if the latest capture is not a valid “fine” basis (desktop: ~500×500 point crop **or** /// quadrant navigation region with longest side < [`COMPUTER_USE_QUADRANT_CLICK_READY_MAX_LONG_EDGE`]). diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_actions.rs b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_actions.rs index 93df273d7..2c4b161ca 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_actions.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_actions.rs @@ -427,8 +427,15 @@ impl ComputerUseActions { host.key_chord(select_all).await?; } host.key_chord(paste_chord).await?; + // A paste lands in whatever already had focus and never moves + // the pointer, so it is not a reason to demand a fresh capture. + // This must run for *every* paste, not just `submit: true`: + // pasting and then sending a separate Enter `key_chord` is the + // common shape, and leaving the guard armed refuses that Enter + // with advice ("call `screenshot` first") that a text-only model + // cannot act on. + host.computer_use_trust_pointer_after_text_input(); if submit { - host.computer_use_trust_pointer_after_text_input(); host.key_chord(submit_keys.clone()).await?; } @@ -542,9 +549,9 @@ impl ComputerUseActions { // ── Desktop AX-first dispatch (Codex parity) ────────────────────── // Routes the seven new app-targeted actions through the typed // `ComputerUseHost` API. Every successful response carries a - // unified envelope: `target_app`, `background_input`, - // `before_digest` and (for state queries) `app_state` / - // `app_state_nodes` so the model can reason about the AX tree + // unified envelope: `target_app`, `background_input`, `before_digest` + // and (for state queries) `app_state`, whose `tree_text` is the single + // rendering of the AX tree — so the model can reason about state // before/after each action without re-querying. async fn handle_desktop_ax( &self, @@ -755,6 +762,16 @@ impl ComputerUseActions { // the heavy `screenshot` payload (it is attached out-of-band as a // multimodal image, not as base64 inside the JSON tree, to keep token // budgets under control and let the provider deliver it as `image_url`). + // + // `tree_text` is the **only** rendering of the AX tree we send. Results + // used to carry a sibling `app_state_nodes` array holding the same + // nodes as verbose JSON — one object per node, ~15 lines each. It was + // strictly redundant (`render_tree_text` already emits idx, role, + // title, value, identifier, description, help, url, frame and the + // enabled/focused/selected/expanded flags, with parentage implied by + // indentation) and nothing consumed it, yet it accounted for ~78% of a + // `get_app_state` result: a single observation of a windowless app + // measured 107 KB, of which 82 KB was that duplicate. fn snap_state_json( snap: &crate::agentic::tools::computer_use_host::AppStateSnapshot, ) -> serde_json::Value { @@ -764,6 +781,7 @@ impl ComputerUseActions { "digest": snap.digest, "captured_at_ms": snap.captured_at_ms, "tree_text": snap.tree_text, + "node_count": snap.nodes.len(), "has_screenshot": snap.screenshot.is_some(), }); if let Some(shot) = snap.screenshot.as_ref() { @@ -904,7 +922,6 @@ impl ComputerUseActions { let mut v = json!({ "target_app": app, "app_state": snap_state_json(&res.snapshot), - "app_state_nodes": res.snapshot.nodes, "loop_warning": res.snapshot.loop_warning, "execution_note": res.execution_note, "interactive_view": res.view.as_ref().map(build_interactive_view_json), @@ -925,7 +942,6 @@ impl ComputerUseActions { let mut v = json!({ "target_app": app, "app_state": snap_state_json(&res.snapshot), - "app_state_nodes": res.snapshot.nodes, "loop_warning": res.snapshot.loop_warning, "execution_note": res.execution_note, "visual_mark_view": res.view.as_ref().map(build_visual_mark_view_json), @@ -1069,7 +1085,6 @@ impl ComputerUseActions { "background_input": bg, "ax_tree": ax, "app_state": snap_state_json(&snap), - "app_state_nodes": snap.nodes, "before_digest": snap.digest, "loop_warning": snap.loop_warning, }); @@ -1164,7 +1179,6 @@ impl ComputerUseActions { "background_input": bg, "before_digest": before, "app_state": snap_state_json(&after), - "app_state_nodes": after.nodes, "loop_warning": after.loop_warning, }); Ok(vec![snap_result(data, Some("clicked".to_string()), &after)]) @@ -1212,7 +1226,6 @@ impl ComputerUseActions { "focus": focus, "before_digest": before, "app_state": snap_state_json(&after), - "app_state_nodes": after.nodes, "loop_warning": after.loop_warning, }); Ok(vec![snap_result( @@ -1237,7 +1250,6 @@ impl ComputerUseActions { "dy": dy, "focus": focus, "app_state": snap_state_json(&after), - "app_state_nodes": after.nodes, "loop_warning": after.loop_warning, }); Ok(vec![snap_result( @@ -1268,7 +1280,6 @@ impl ComputerUseActions { "keys": keys, "focus_idx": focus_idx, "app_state": snap_state_json(&after), - "app_state_nodes": after.nodes, "loop_warning": after.loop_warning, }); Ok(vec![snap_result( @@ -1302,7 +1313,6 @@ impl ComputerUseActions { "background_input": bg, "predicate": predicate, "app_state": snap_state_json(&after), - "app_state_nodes": after.nodes, "loop_warning": after.loop_warning, }); Ok(vec![snap_result( diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs index cf9ce31d5..23efc80d8 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs @@ -446,7 +446,14 @@ The **primary model cannot consume images** in tool results — **do not** use * async fn describe_screen( host: &dyn ComputerUseHost, _input: &Value, + text_only: bool, ) -> BitFunResult> { + // For a text-only model this *is* the observation step, so it clears + // the same guard a `screenshot` would. Without this the guard can only + // ever be cleared by a capture the model cannot consume. + if text_only { + host.computer_use_waive_fresh_capture_guard(); + } let session_snap = host.computer_use_session_snapshot().await; let interaction = host.computer_use_interaction_state(); let pointer = session_snap.pointer_global.clone(); @@ -469,8 +476,13 @@ The **primary model cannot consume images** in tool results — **do not** use * let mut ax_nodes_count: Option = None; let mut ax_digest: Option = None; let mut window_title: Option = None; - if let Some(app) = selector.as_ref() { - match host.get_app_state(app.clone(), 8, true).await { + // Why `ax_tree_text` is empty, when it is. A bare `null` here reads as + // truncated tool output, and an agent that believes its own results are + // being cut off will keep re-issuing the same call instead of switching + // tactic — which is exactly what a null `ax_tree_text` used to cause. + let ax_tree_status: &str = match selector.as_ref() { + None => "no_foreground_app", + Some(app) => match host.get_app_state(app.clone(), 8, true).await { Ok(snap) => { // Deliberately drop `snap.screenshot` (JPEG) — describe_screen // never returns image bytes so text-only models are safe. @@ -478,15 +490,45 @@ The **primary model cannot consume images** in tool results — **do not** use * ax_nodes_count = Some(snap.nodes.len()); ax_digest = Some(snap.digest.clone()); ax_tree_text = Some(snap.tree_text).filter(|t| !t.trim().is_empty()); + if ax_tree_text.is_some() { + "ok" + } else { + "empty_tree" + } } Err(e) => { debug!("describe_screen: get_app_state failed: {}", e); + "query_failed" } - } - } + }, + }; let ui_tree_text = host.enumerate_ui_tree_text().await; + // Turn each non-`ok` status into the tactic that actually works there, + // so a sparse tree costs one redirect instead of a search. + let ax_tree_note = match ax_tree_status { + "ok" => None, + "no_foreground_app" => Some( + "No application is frontmost, so there is no AX tree to read. Use `list_apps` to \ +find the target, then `open_app` (or `app_click` with an explicit `app` selector) to bring it forward." + .to_string(), + ), + "empty_tree" => Some( + "The frontmost app exposes an empty accessibility tree — usual for Electron / \ +WebView apps that have not enabled their web-content AX tree, and for an app running with no \ +window. This is NOT truncated output: re-calling `describe_screen` returns the same thing. \ +Check `window_count` via `open_app`, or target visible text with `move_to_text` / `click_target`." + .to_string(), + ), + "query_failed" => Some( + "The AX query failed (commonly missing Accessibility trust, or the app exited). \ +Grant Accessibility permission, or fall back to `move_to_text` / `click_target` on visible text." + .to_string(), + ), + _ => None, + }; + let mut body = json!({ "success": true, "action": "describe_screen", @@ -496,9 +538,12 @@ The **primary model cannot consume images** in tool results — **do not** use * "displays": displays, "window_title": window_title, "ax_tree_text": ax_tree_text, + "ax_tree_status": ax_tree_status, + "ax_tree_note": ax_tree_note, "ax_nodes_count": ax_nodes_count, "ax_state_digest": ax_digest, "ui_tree_text": ui_tree_text, + "output_is_complete": true, }); let input_coords = json!({ @@ -510,8 +555,18 @@ The **primary model cannot consume images** in tool results — **do not** use * // pick `node_idx` from `ax_tree_text` for `app_click`/`click_element`, or // match visible text via `move_to_text`, and compare `ax_state_digest` // before/after an action to verify a mutation. - let hint = "describe_screen: text snapshot returned (no image). Use `ax_tree_text` node indices for `app_click`/`click_element`, match visible text with `move_to_text`, and compare `ax_state_digest` across actions to verify state changes."; - Ok(vec![ToolResult::ok(body, Some(hint.to_string()))]) + let hint = format!( + "describe_screen: complete text snapshot returned (no image, ax_tree_status={}). \ +Use `ax_tree_text` node indices for `app_click`/`click_element`, match visible text with `move_to_text`, \ +and compare `ax_state_digest` across actions to verify state changes.{}", + ax_tree_status, + if ax_tree_status == "ok" { + "" + } else { + " No AX tree available — read `ax_tree_note` and switch tactic rather than repeating this call." + } + ); + Ok(vec![ToolResult::ok(body, Some(hint))]) } /// Screenshot tool results attach JPEGs via `tool_image_attachments`; only providers whose @@ -1216,7 +1271,8 @@ impl Tool for ComputerUseTool { // + pointer + displays) with NO image bytes. This is the observe and // verify step that closes the cowork loop for text-only models. "describe_screen" => { - return Self::describe_screen(host_ref, input).await; + let text_only = !context.primary_model_supports_image_understanding(); + return Self::describe_screen(host_ref, input, text_only).await; } // Unified target resolver: AX first, OCR second, explicit screen @@ -1715,12 +1771,19 @@ impl Tool for ComputerUseTool { // at the text-only observe action. The model keeps its turn and // switches to `describe_screen` / AX / OCR / keyboard tactics. if !context.primary_model_supports_image_understanding() { + // A text-only `screenshot` never captures anything, so it + // can never clear the stale-capture guard the usual way. + // Waive it here: otherwise the guard's own recovery advice + // ("call `screenshot` first") is an instruction the model + // can follow forever without ever being allowed to click. + host_ref.computer_use_waive_fresh_capture_guard(); let body = json!({ "success": true, "action": "screenshot", "screenshot_unavailable": true, "reason": "primary_model_is_text_only", - "instruction": "The primary model cannot consume image bytes, so `screenshot` produced nothing. Use `describe_screen` to observe the desktop as text (frontmost app + AX tree + UI tree text + pointer), then act with `click_target`/`click_element`/`move_to_text`/`key_chord`/`paste`. Never retry `screenshot`." + "stale_capture_guard": "waived", + "instruction": "The primary model cannot consume image bytes, so `screenshot` produced nothing. Use `describe_screen` to observe the desktop as text (frontmost app + AX tree + UI tree text + pointer), then act with `click_target`/`click_element`/`move_to_text`/`key_chord`/`paste`. Never retry `screenshot`. The fresh-capture guard has been waived, so `click` and Enter `key_chord` are unblocked." }); let input_coords = json!({ "kind": "screenshot", "text_only": true }); let body = @@ -1937,6 +2000,25 @@ impl Tool for ComputerUseTool { BitFunError::tool("open_app requires `app_name` parameter.".to_string()) })?; let result = host_ref.open_app(app_name).await?; + // A live process with zero windows is the one launch outcome + // that looks like success but leaves nothing to act on. Name it + // explicitly and say what to do, rather than letting the agent + // rediscover it through a chain of failing AX queries. + let windowless = result.success && result.window_count == Some(0); + let next_step = if windowless { + Some(format!( + "'{}' is running (PID {}) but owns no window, so there is nothing on screen to click. \ +The host already retried via `open -b`. Re-run `open_app`, or ask the user to open the app's main window (e.g. from its Dock icon). \ +Do not fall back to screen-coordinate clicks — there is no window to hit.", + result.app_name, + result + .process_id + .map(|p| p.to_string()) + .unwrap_or_else(|| "?".to_string()), + )) + } else { + None + }; let body = computer_use_augment_result_json( host_ref, json!({ @@ -1945,13 +2027,28 @@ impl Tool for ComputerUseTool { "app_name": result.app_name, "process_id": result.process_id, "error_message": result.error_message, + // Address the app by `bundle_id` from here on: the name + // used to launch it, its executable name and its bundle + // id are often three different strings. + "bundle_id": result.bundle_id, + "process_name": result.process_name, + "window_count": result.window_count, + "launch_path": result.launch_path, + "windowless": windowless, + "next_step": next_step, }), None, ) .await; - let summary = if result.success { + let summary = if !result.success { format!( - "Opened app '{}'{}.", + "Failed to open '{}': {}", + result.app_name, + result.error_message.as_deref().unwrap_or("unknown error") + ) + } else if windowless { + format!( + "Opened '{}'{} but it has NO window — nothing is on screen to act on.", result.app_name, result .process_id @@ -1960,9 +2057,16 @@ impl Tool for ComputerUseTool { ) } else { format!( - "Failed to open '{}': {}", + "Opened app '{}'{}{}.", result.app_name, - result.error_message.as_deref().unwrap_or("unknown error") + result + .process_id + .map(|p| format!(" (PID {})", p)) + .unwrap_or_default(), + result + .window_count + .map(|n| format!(", {} window(s)", n)) + .unwrap_or_default() ) }; Ok(vec![ToolResult::ok(body, Some(summary))]) @@ -2360,6 +2464,163 @@ mod tests { } } + /// Host that records whether the stale-capture guard was waived, and + /// reports no frontmost app so `describe_screen` exercises its + /// nothing-to-observe branch. + #[derive(Debug, Default)] + struct GuardRecordingHost { + waived: std::sync::atomic::AtomicBool, + } + + #[async_trait::async_trait] + impl ComputerUseHost for GuardRecordingHost { + async fn permission_snapshot(&self) -> BitFunResult { + not_expected() + } + async fn request_accessibility_permission(&self) -> BitFunResult<()> { + not_expected() + } + async fn request_screen_capture_permission(&self) -> BitFunResult<()> { + not_expected() + } + async fn screenshot_display( + &self, + _params: ComputerUseScreenshotParams, + ) -> BitFunResult { + not_expected() + } + fn map_image_coords_to_pointer(&self, _x: i32, _y: i32) -> BitFunResult<(i32, i32)> { + not_expected() + } + fn map_normalized_coords_to_pointer(&self, _x: i32, _y: i32) -> BitFunResult<(i32, i32)> { + not_expected() + } + async fn mouse_move(&self, _x: i32, _y: i32) -> BitFunResult<()> { + not_expected() + } + async fn pointer_move_relative(&self, _dx: i32, _dy: i32) -> BitFunResult<()> { + not_expected() + } + async fn mouse_click(&self, _button: &str) -> BitFunResult<()> { + not_expected() + } + async fn scroll(&self, _delta_x: i32, _delta_y: i32) -> BitFunResult<()> { + not_expected() + } + async fn key_chord(&self, _keys: Vec) -> BitFunResult<()> { + not_expected() + } + async fn type_text(&self, _text: &str) -> BitFunResult<()> { + not_expected() + } + async fn wait_ms(&self, _ms: u64) -> BitFunResult<()> { + not_expected() + } + async fn computer_use_session_snapshot(&self) -> ComputerUseSessionSnapshot { + ComputerUseSessionSnapshot::default() + } + fn computer_use_waive_fresh_capture_guard(&self) { + self.waived.store(true, std::sync::atomic::Ordering::SeqCst); + } + } + + fn text_only_context( + host: std::sync::Arc, + ) -> (ToolUseContext, std::sync::Arc) { + let mut context = ToolUseContext::for_tool_listing(None, None); + context.primary_model_facts = + tool_runtime::context::PrimaryModelFacts::new("m", "m", "anthropic", false); + context.computer_use_host = Some(host.clone()); + (context, host) + } + + /// A text-only `screenshot` captures nothing, so it can never clear the + /// stale-capture guard through the normal path — yet the guard's own error + /// tells the model to "call `screenshot` first". Left as it was, that is a + /// closed loop: every `click` and Enter `key_chord` stays refused for the + /// rest of the session, and the only way out is to bypass the tool entirely + /// (the observed failure was an agent falling back to raw + /// `osascript … keystroke return`, which skips every safety check the guard + /// exists to enforce). + #[tokio::test] + async fn text_only_screenshot_waives_the_unsatisfiable_capture_guard() { + let (context, host) = text_only_context(std::sync::Arc::new(GuardRecordingHost::default())); + let results = ComputerUseTool::new() + .call_impl(&json!({ "action": "screenshot" }), &context) + .await + .expect("text-only screenshot returns a soft envelope"); + assert!( + host.waived.load(std::sync::atomic::Ordering::SeqCst), + "text-only screenshot must waive the guard it can never satisfy" + ); + let body = results[0].content(); + assert_eq!( + body.get("stale_capture_guard").and_then(Value::as_str), + Some("waived"), + "the waiver must be visible to the model: {body}" + ); + // The guard's own error text says "call `screenshot` first"; the + // instruction here has to say that path is now open, or the model has + // no reason to believe retrying the click will work. + let instruction = body + .get("instruction") + .and_then(Value::as_str) + .unwrap_or_default(); + assert!( + instruction.contains("waived"), + "instruction must tell the model the guard is cleared: {instruction}" + ); + } + + /// `describe_screen` is the text-only equivalent of taking a look, so it + /// clears the same guard a capture would. + #[tokio::test] + async fn text_only_describe_screen_waives_the_capture_guard() { + let (context, host) = text_only_context(std::sync::Arc::new(GuardRecordingHost::default())); + let _ = ComputerUseTool::new() + .call_impl(&json!({ "action": "describe_screen" }), &context) + .await + .expect("describe_screen should succeed"); + assert!( + host.waived.load(std::sync::atomic::Ordering::SeqCst), + "describe_screen is the text-only observation step and must waive the guard" + ); + } + + /// An empty snapshot must say *why* it is empty. A bare `ax_tree_text: + /// null` reads as truncated tool output, and an agent that believes its + /// results are being cut off re-issues the same call instead of changing + /// tactic. + #[tokio::test] + async fn describe_screen_explains_an_empty_ax_tree_instead_of_returning_bare_nulls() { + let (context, _host) = + text_only_context(std::sync::Arc::new(GuardRecordingHost::default())); + let results = ComputerUseTool::new() + .call_impl(&json!({ "action": "describe_screen" }), &context) + .await + .expect("describe_screen should succeed"); + let body = results[0].content(); + let data = body.get("data").unwrap_or(&body); + assert_eq!( + data.get("ax_tree_status").and_then(Value::as_str), + Some("no_foreground_app"), + "status must name the reason the tree is empty: {body}" + ); + assert_eq!( + data.get("output_is_complete").and_then(Value::as_bool), + Some(true), + "result must assert it is not truncated: {body}" + ); + let note = data + .get("ax_tree_note") + .and_then(Value::as_str) + .unwrap_or_default(); + assert!( + note.contains("list_apps") || note.contains("open_app"), + "note must offer a concrete next action: {note}" + ); + } + /// The browser-boundary guard must be reachable from `call_impl`: a /// physical input action while a Chromium-family browser is frontmost is /// rejected with the ControlHub browser-domain redirect instead of diff --git a/src/crates/execution/agent-runtime/src/prompt.rs b/src/crates/execution/agent-runtime/src/prompt.rs index a576206bc..d2f4a6939 100644 --- a/src/crates/execution/agent-runtime/src/prompt.rs +++ b/src/crates/execution/agent-runtime/src/prompt.rs @@ -493,7 +493,16 @@ fn computer_use_text_only_model_guidance() -> Vec { vec![ "- The configured primary model does not accept image inputs.".to_string(), "- When using `ComputerUse` or `ControlHub` with `domain: \"browser\"`, do not use `screenshot` and avoid `domain:\"browser\" action:\"screenshot\"`; image bytes will be unreadable.".to_string(), + // Banning `screenshot` without naming the replacement is what pushes a + // text-only agent into improvising `screencapture` + `analyze_image` as + // a substitute for eyes — a loop that costs an extra model call per + // look, returns prose instead of coordinates, and misreads the screen + // often enough to send the run down false paths. + "- `describe_screen` is your eyes: it returns the frontmost app, the AX tree (`ax_tree_text`, with `node_idx`s you can click by), `ui_tree_text` and the pointer, as text with no image. Call it when UI state is unknown, and again after an action to confirm `ax_state_digest` changed.".to_string(), + "- Do NOT shell out to `screencapture` and feed the file to an image-analysis tool as a substitute for looking. It costs an extra model round-trip per glance and returns descriptions, not clickable coordinates. Use `describe_screen`, `get_app_state`, `locate` and `move_to_text` — they return exact targets.".to_string(), + "- If `ax_tree_text` is empty, read `ax_tree_status` / `ax_tree_note` in the same result: they say why (no frontmost app, an app with no window, a WebView that exposes no tree, or a permission failure) and which tactic to switch to. The result is never truncated — re-calling returns the same thing.".to_string(), "- Action priority: 1) Terminal/CLI/system commands (`ExecCommand`, or `ComputerUse` `run_script`; use `WriteStdin`/`ExecControl` for running ExecCommand sessions) 2) Keyboard shortcuts (`key_chord`, `type_text`) 3) UI control: `click_element` (AX) -> `locate` -> `move_to_text` (use `move_to_text_match_index` when multiple OCR hits are listed) -> `mouse_move` (`use_screen_coordinates: true` with coordinates from tool JSON) -> `click`. For browser work, prefer `snapshot` then click by `@e*` ref over screenshots.".to_string(), + "- To type and send in one step use `paste` with `submit: true` (and `submit_keys` when the app sends on a chord, e.g. `[\"command\",\"return\"]`). `paste` is also the reliable path for CJK and emoji.".to_string(), "- Never guess coordinates. Always use precise methods: AX, OCR, system coordinates from tool results, or browser snapshot refs.".to_string(), ] } diff --git a/src/crates/execution/tool-contracts/src/computer_use.rs b/src/crates/execution/tool-contracts/src/computer_use.rs index fbfbc617e..e2d345c52 100644 --- a/src/crates/execution/tool-contracts/src/computer_use.rs +++ b/src/crates/execution/tool-contracts/src/computer_use.rs @@ -1131,6 +1131,28 @@ pub struct OpenAppResult { pub process_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub error_message: Option, + /// Bundle identifier of the launched app (macOS). The agent needs this to + /// address the app afterwards: the name it launched by (`Lark`), the + /// executable name (`Feishu`) and the bundle id (`com.electron.lark`) are + /// routinely three different strings, and only the bundle id works with + /// every follow-up path. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub bundle_id: Option, + /// Process name as the OS reports it — the identity AppleScript's + /// `tell process "…"` and `ps` expect, which is often **not** `app_name`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub process_name: Option, + /// Windows the app owns once the launch settled. `Some(0)` means the + /// process is alive but has nothing on screen — a real state for Electron + /// apps whose window was closed while the process stayed resident, and one + /// the agent otherwise has no way to distinguish from a healthy launch. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub window_count: Option, + /// How the app ended up in front. Diagnostic: tells the agent whether a + /// plain activate sufficed or the host had to re-open the bundle to force + /// a window. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub launch_path: Option, } /// Whether the latest screenshot JPEG was the full display, a point crop, or a quadrant-drill region. From 8d257cb84ce22e594428e4430cccbd127153fda2 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Tue, 11 Aug 2026 07:08:48 -0700 Subject: [PATCH 2/6] fix(computer-use): fill new OpenAppResult fields on Windows and Linux The macOS branch gained bundle_id / process_name / window_count / launch_path; the other two construct the same struct and would not compile without them. Both leave the identity fields None rather than guessing: neither `start` nor `xdg-open` reports what it launched, so there is no pid to resolve identity or a window count from. `window_count: Some(0)` would tell the model the app is definitely windowless when it was simply never measured. --- .../src/computer_use/desktop_host/mod.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/apps/desktop/src/computer_use/desktop_host/mod.rs b/src/apps/desktop/src/computer_use/desktop_host/mod.rs index defde3460..453317898 100644 --- a/src/apps/desktop/src/computer_use/desktop_host/mod.rs +++ b/src/apps/desktop/src/computer_use/desktop_host/mod.rs @@ -1463,6 +1463,16 @@ impl ComputerUseHost for DesktopComputerUseHost { } else { Some(String::from_utf8_lossy(&output.stderr).trim().to_string()) }, + // `start` hands off to the shell and returns immediately + // without telling us what it launched, so there is no pid + // to resolve identity or window count from. Left as `None` + // (the "not measured" value) rather than faked — the model + // reads `window_count: Some(0)` as a definite windowless + // app and would act on it. + bundle_id: None, + process_name: None, + window_count: None, + launch_path: Some("shell_start".to_string()), }) }) .await @@ -1487,6 +1497,14 @@ impl ComputerUseHost for DesktopComputerUseHost { } else { Some(String::from_utf8_lossy(&output.stderr).trim().to_string()) }, + // Linux is the legacy tier: no AX layer, so there is no pid + // to resolve identity or window count from. `None` means + // "not measured" — do not substitute `Some(0)`, which the + // model reads as a definite windowless app. + bundle_id: None, + process_name: None, + window_count: None, + launch_path: Some("xdg_open".to_string()), }) }) .await From 2d20f76828ff1bd7c1e6da53bdc2897f597157a6 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Tue, 11 Aug 2026 07:12:07 -0700 Subject: [PATCH 3/6] test(computer-use): compile-check AppleScript templates with escaped names Only "Safari" was covered, which never exercises applescript_quote. Compiling the escaped forms of a quote, a backslash and CJK is what proves the escaping matches AppleScript's actual string-literal syntax rather than a plausible guess about it. --- .../src/computer_use/desktop_host/mod.rs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/apps/desktop/src/computer_use/desktop_host/mod.rs b/src/apps/desktop/src/computer_use/desktop_host/mod.rs index 453317898..d2a3fa13e 100644 --- a/src/apps/desktop/src/computer_use/desktop_host/mod.rs +++ b/src/apps/desktop/src/computer_use/desktop_host/mod.rs @@ -169,15 +169,17 @@ mod macos_applescript_tests { /// Every AppleScript this module generates now has to compile. #[test] fn every_generated_applescript_compiles() { - let templates = [ - format!("id of application {}", applescript_quote("Safari")), - format!( - "tell application {} to activate", - applescript_quote("Safari") - ), - ]; - for t in templates { - assert!(compiles(&t).is_ok(), "template failed to compile: {t}"); + // Includes the names that exercise `applescript_quote`: a quote, a + // backslash and CJK. Asserting the *escaped* form compiles is what + // proves the escaping is genuine AppleScript rather than a plausible + // guess about its string-literal syntax. + for name in ["Safari", "a\"b", "a\\b", "飞书", "Visual Studio Code"] { + for t in [ + format!("id of application {}", applescript_quote(name)), + format!("tell application {} to activate", applescript_quote(name)), + ] { + assert!(compiles(&t).is_ok(), "template failed to compile: {t}"); + } } } From b29970bc7832f51b1f2a8fe2e68a500a91593151 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Tue, 11 Aug 2026 07:14:57 -0700 Subject: [PATCH 4/6] docs(computer-use): tell the subagent an empty AX tree is an answer The two lines this prompt was missing are the ones that would have ended the observed failure early. When describe_screen returned nulls the agent concluded its own output was truncated and improvised screencapture plus image analysis as a substitute for eyes. State plainly that an empty ax_tree_text is a result with a reason attached (ax_tree_status / ax_tree_note), that re-calling returns the same thing, and that building eyes out of screencapture costs a model round-trip per glance and returns prose instead of coordinates. --- .../assembly/agent-content/prompts/agents/computer_use_mode.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/crates/assembly/agent-content/prompts/agents/computer_use_mode.md b/src/crates/assembly/agent-content/prompts/agents/computer_use_mode.md index f66a2f04a..8ccde914e 100644 --- a/src/crates/assembly/agent-content/prompts/agents/computer_use_mode.md +++ b/src/crates/assembly/agent-content/prompts/agents/computer_use_mode.md @@ -60,6 +60,8 @@ If the same GUI tactic fails twice, switch strategy: use keyboard navigation, ap When Runtime Context indicates the primary model does not support image understanding, the vision-only actions — `screenshot`, `build_interactive_view`, `interactive_click`, `build_visual_mark_view`, `visual_click` — are unavailable: they are absent from your tool schema, `screenshot` returns no image (`screenshot_unavailable: true`), and the other four return NOT_AVAILABLE. Do NOT retry them and do NOT call them to verify — they cannot help you see. Instead: - **Observe with `describe_screen`** — it returns a text snapshot (frontmost app, `ax_tree_text` with `node_idx`s, `ui_tree_text`, pointer, displays) with no image. This is your eyes. Call it before acting when state is unknown, and after an action to verify `ax_state_digest` changed. +- **An empty `ax_tree_text` is an answer, not a truncated result.** The same result carries `ax_tree_status` and `ax_tree_note` explaining why it is empty — no frontmost app, an app running with no window, a WebView that exposes no tree, or a missing Accessibility permission — and which tactic to switch to. Calling `describe_screen` again returns exactly the same thing. Never conclude your tool output is being cut off; act on the note instead. +- **Do not build your own eyes out of `screencapture`.** Shelling out to capture the screen and passing the file to an image-analysis tool costs an extra model round-trip per glance, returns prose rather than clickable coordinates, and misreads the screen often enough to send you down false paths. `describe_screen`, `get_app_state`, `locate` and `move_to_text` return exact targets — use them. - **Target with AX / OCR, never guessed coordinates** — `click_element`/`app_click` with `node_idx`/`text_contains`/`title_contains`/`role_substring`; `move_to_text`/`click_target` with `target_text` (+ `move_to_text_match_index` when several OCR hits are returned as text candidates). - **Prefer keyboard** — `key_chord` shortcuts (command+F search, Tab/Shift+Tab focus, Return confirm, Escape cancel) and `paste` (clipboard) for CJK / long text before `type_text`. - **Drive hard-to-reach apps directly** — `run_apple_script` (macOS) for messaging/desktop apps whose AX tree is sparse. From e94485f8f1384d36e3f0cb762b7cc4745d5964de Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Tue, 11 Aug 2026 07:24:10 -0700 Subject: [PATCH 5/6] fix(computer-use): let describe_screen see deep enough to act on Electron apps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixing `foreground_application` gave `describe_screen` an app to look at. It was still walking only 8 levels into the focused window, which is fine for a native Cocoa app and far too shallow for the Electron / WebView clients agents are most often asked to drive. Measured against a real Electron window (focused window only): depth 8: 17 nodes, 7 actionable, 1 KB depth 12: 25 nodes, 15 actionable, 2 KB depth 16: 50 nodes, 40 actionable, 5 KB depth 20: 207 nodes, 197 actionable, 27 KB depth 24: 233 nodes, 223 actionable, 31 KB depth 32: 1289 nodes, 1279 actionable, 206 KB Seven actionable elements is not enough to find a search field or a send button, so the tree read as "this app has no AX tree" and pushed the agent onto OCR and screenshot guessing. The actionable layer appears around 20; past it the payload grows far faster than the number of things worth clicking. Depth is a poor proxy for size, though — a document or a long list can multiply that 27 KB — so the returned tree is also capped at 60 KB, cut on a line boundary. The clip announces itself and says a control that is missing from the view may still exist: an agent that reads a truncated tree as the whole UI concludes the control is not there and gives up. The `#[ignore]`d dump test now prints this depth profile, so the constant can be retuned against evidence rather than intuition. --- .../desktop/src/computer_use/macos_ax_dump.rs | 77 +++++++++++++++ .../implementations/computer_use_tool.rs | 99 ++++++++++++++++++- 2 files changed, 173 insertions(+), 3 deletions(-) diff --git a/src/apps/desktop/src/computer_use/macos_ax_dump.rs b/src/apps/desktop/src/computer_use/macos_ax_dump.rs index 8d8461ed7..86c24eefe 100644 --- a/src/apps/desktop/src/computer_use/macos_ax_dump.rs +++ b/src/apps/desktop/src/computer_use/macos_ax_dump.rs @@ -975,6 +975,83 @@ mod tests { assert_ne!(d1, d2); } + /// Measure the closed-menu pruning against a real running app rather than + /// trusting the unit test's synthetic frames. + /// + /// Dumps the frontmost application twice — once with menus walked, once + /// with the default pruning — and reports both node counts. Requires + /// Accessibility permission and a GUI session, so it is `#[ignore]`d. + #[test] + #[ignore] + fn closed_menu_pruning_shrinks_a_real_app_dump() { + let pid = crate::computer_use::macos_bg_input::frontmost_pid_macos() + .expect("a GUI session has a frontmost app"); + + let with_menus = dump_app_ax( + pid, + DumpOpts { + include_closed_menus: true, + ..Default::default() + }, + ) + .expect("dump with menus"); + let pruned = dump_app_ax(pid, DumpOpts::default()).expect("pruned dump"); + + // What `describe_screen` actually asks for: depth 8, focused window + // only. Reported alongside so the cost of the observe path is visible + // next to the cost of a full `get_app_state`. + let observe = dump_app_ax( + pid, + DumpOpts { + max_depth: 8, + focus_window_only: true, + ..Default::default() + }, + ) + .expect("describe_screen-shaped dump"); + + eprintln!( + "pid={pid}\n full+menus: {:>5} nodes, {:>7} bytes\n full pruned: {:>5} nodes, {:>7} bytes\n observe: {:>5} nodes, {:>7} bytes", + with_menus.nodes.len(), + with_menus.tree_text.len(), + pruned.nodes.len(), + pruned.tree_text.len(), + observe.nodes.len(), + observe.tree_text.len(), + ); + // Depth profile of the focused window. Run this when retuning + // `DESCRIBE_SCREEN_AX_DEPTH`: "actionable" (has AX actions and a real + // frame) is what the agent can actually click, and it is the column + // that matters — node count and bytes grow long after it plateaus. + for d in [8u32, 12, 16, 20, 24, 32] { + let s = dump_app_ax( + pid, + DumpOpts { + max_depth: d, + focus_window_only: true, + ..Default::default() + }, + ) + .expect("depth dump"); + let actionable = s + .nodes + .iter() + .filter(|n| !n.actions.is_empty() && n.frame_global.is_some()) + .count(); + eprintln!( + " depth {:>2}: {:>5} nodes, {:>4} actionable, {:>7} bytes", + d, + s.nodes.len(), + actionable, + s.tree_text.len() + ); + } + assert!( + pruned.nodes.len() <= with_menus.nodes.len(), + "pruning must never grow the tree" + ); + } + /// Smoke test: dump the AX tree of *this* test process. The test process /// usually has no AX windows of its own, so we only assert the call /// returns *something* (possibly an empty tree) without panicking and diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs index 23efc80d8..0a8c39644 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs @@ -119,6 +119,63 @@ const COMPUTER_USE_DEBUG_SCREENSHOTS_ENV: &str = "BITFUN_COMPUTER_USE_DEBUG_SCRE /// Newest debug screenshots retained in [`COMPUTER_USE_DEBUG_SUBDIR`]; older files are deleted. const COMPUTER_USE_DEBUG_MAX_FILES: usize = 20; +/// AX depth `describe_screen` walks into the focused window. +/// +/// This was 8, which is fine for a native Cocoa app but far too shallow for +/// Electron / WebView clients — the ones agents are most often asked to drive. +/// Measured against a real Electron window (focused window only): +/// +/// | depth | nodes | actionable | tree_text | +/// |------:|------:|-----------:|----------:| +/// | 8 | 17 | 7 | 1 KB | +/// | 12 | 25 | 15 | 2 KB | +/// | 16 | 50 | 40 | 5 KB | +/// | 20 | 207 | 197 | 27 KB | +/// | 24 | 233 | 223 | 31 KB | +/// | 32 | 1289 | 1279 | 206 KB | +/// +/// At 8 the agent could see seven actionable elements in an entire app — not +/// enough to find a search field or a send button, which reads as "this app has +/// no AX tree" and pushes it onto OCR or screenshot guessing. The actionable +/// layer appears around 20; past that the payload grows far faster than the +/// number of things worth clicking. +const DESCRIBE_SCREEN_AX_DEPTH: u32 = 20; + +/// Byte ceiling on the AX tree `describe_screen` returns. +/// +/// The depth above is tuned against a typical rich window (~27 KB), but depth +/// is a poor proxy for size: a document, a long list or a deeply nested canvas +/// can multiply that. `describe_screen` is the action an agent calls most, so +/// it needs a bound that does not depend on the app behaving reasonably. +const DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES: usize = 60_000; + +/// Trim an AX tree to [`DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES`] on a line +/// boundary, appending a note that says what was dropped and how to get it. +/// +/// Silent truncation would be worse than the problem it solves: the agent would +/// read a partial tree as the whole UI and conclude a control does not exist. +fn clip_tree_text(text: String) -> String { + if text.len() <= DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES { + return text; + } + let cut = text[..DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES] + .rfind('\n') + .unwrap_or(DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES); + let kept_lines = text[..cut].lines().count(); + let total_lines = text.lines().count(); + format!( + "{}\n[truncated] showing the first {} of {} AX nodes ({} of {} bytes). \ +This is a size limit, not the end of the UI — a control you cannot find here may still exist. \ +Narrow the view with `get_app_state` (`focus_window_only`, a smaller `max_depth`) or target it \ +directly with `locate` / `move_to_text`.\n", + &text[..cut], + kept_lines, + total_lines, + cut, + text.len(), + ) +} + pub struct ComputerUseTool; impl Default for ComputerUseTool { @@ -482,14 +539,18 @@ The **primary model cannot consume images** in tool results — **do not** use * // tactic — which is exactly what a null `ax_tree_text` used to cause. let ax_tree_status: &str = match selector.as_ref() { None => "no_foreground_app", - Some(app) => match host.get_app_state(app.clone(), 8, true).await { + Some(app) => match host + .get_app_state(app.clone(), DESCRIBE_SCREEN_AX_DEPTH, true) + .await + { Ok(snap) => { // Deliberately drop `snap.screenshot` (JPEG) — describe_screen // never returns image bytes so text-only models are safe. window_title = snap.window_title.clone(); ax_nodes_count = Some(snap.nodes.len()); ax_digest = Some(snap.digest.clone()); - ax_tree_text = Some(snap.tree_text).filter(|t| !t.trim().is_empty()); + ax_tree_text = + Some(clip_tree_text(snap.tree_text)).filter(|t| !t.trim().is_empty()); if ax_tree_text.is_some() { "ok" } else { @@ -2178,7 +2239,7 @@ fn req_i32(input: &Value, key: &str) -> BitFunResult { #[cfg(test)] mod tests { - use super::ComputerUseTool; + use super::{clip_tree_text, ComputerUseTool, DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES}; use crate::agentic::tools::computer_use_host::{ ComputerScreenshot, ComputerUseForegroundApplication, ComputerUseHost, ComputerUsePermissionSnapshot, ComputerUseScreenshotParams, ComputerUseSessionSnapshot, @@ -2587,6 +2648,38 @@ mod tests { ); } + #[test] + fn tree_text_under_the_cap_is_returned_verbatim() { + let small = "[0] AXApplication\n [1] AXWindow\n".to_string(); + assert_eq!(clip_tree_text(small.clone()), small); + } + + /// Truncation must announce itself. An agent that reads a clipped tree as + /// the whole UI concludes a control does not exist and gives up on it. + #[test] + fn oversized_tree_text_is_clipped_on_a_line_boundary_and_says_so() { + let line = "[0] AXButton title=\"x\" frame=(0,0,10x10)\n"; + let big = line.repeat(DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES / line.len() + 500); + let out = clip_tree_text(big.clone()); + + assert!(out.len() < big.len(), "must actually shrink"); + assert!( + out.contains("[truncated]"), + "must announce the clip: {out:.200}" + ); + assert!( + out.contains("not the end of the UI"), + "must warn that a missing control may still exist" + ); + // Cutting mid-line would hand the model a malformed node. + let body = out.split("\n[truncated]").next().unwrap(); + assert!( + body.lines() + .all(|l| l.is_empty() || l.starts_with("[0] AXButton")), + "clip must land on a line boundary" + ); + } + /// An empty snapshot must say *why* it is empty. A bare `ax_tree_text: /// null` reads as truncated tool output, and an agent that believes its /// results are being cut off re-issues the same call instead of changing From 6bb658fb4df56696f9245517b5155d464e9a79c6 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Tue, 11 Aug 2026 07:31:12 -0700 Subject: [PATCH 6/6] fix(computer-use): clip the AX tree on a char boundary, not a raw byte index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 60 KB cap I added in the previous commit sliced the tree at a byte offset. `&str[..n]` panics when `n` falls inside a multi-byte character, so a CJK app tree — the kind most likely to be large enough to hit the cap in the first place — would panic the whole tool call roughly two times in three. Walk back to a char boundary before slicing. The first tests I wrote for this passed against the broken version: repeating a fixed line, and repeating a bare 3-byte character, both happen to land exactly on 60_000. Shifting the content by one and two bytes is what exposes it, so the test now covers all three alignments and was confirmed to fail without the fix. --- .../implementations/computer_use_tool.rs | 56 ++++++++++++++++++- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs index 0a8c39644..0650323d7 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/computer_use_tool.rs @@ -158,9 +158,16 @@ fn clip_tree_text(text: String) -> String { if text.len() <= DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES { return text; } - let cut = text[..DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES] - .rfind('\n') - .unwrap_or(DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES); + // Walk back to a char boundary before slicing. The cap is a byte count, and + // slicing a `str` at a byte index inside a multi-byte character panics — + // which CJK app trees (the ones most likely to be large) would hit + // constantly. + let mut end = DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES; + while end > 0 && !text.is_char_boundary(end) { + end -= 1; + } + // A newline is single-byte, so its index is always a valid boundary too. + let cut = text[..end].rfind('\n').unwrap_or(end); let kept_lines = text[..cut].lines().count(); let total_lines = text.lines().count(); format!( @@ -2680,6 +2687,49 @@ mod tests { ); } + /// The cap is a byte count but the tree is a `str`, so the clip has to land + /// on a char boundary. A CJK app — exactly the kind whose tree gets large — + /// would otherwise panic the whole tool call on a mid-character slice. + #[test] + fn oversized_cjk_tree_text_clips_without_panicking() { + for label in ["范明裕", "飞书 · 消息", "🙂 emoji", "混合 mixed 内容"] { + let line = format!("[0] AXStaticText title=\"{label}\"\n"); + let big = line.repeat(DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES / line.len() + 500); + assert!(big.len() > DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES); + + let out = clip_tree_text(big.clone()); + assert!(out.contains("[truncated]"), "must announce the clip"); + assert!(out.len() < big.len(), "must actually shrink"); + } + } + + /// The cut offset must be safe for *every* alignment, not the one a given + /// repeated line happens to produce. + /// + /// Shifting the content by one and two bytes is what makes this bite: a + /// 3-byte character misaligns against the byte cap at two of every three + /// offsets, and only those two panic. An unshifted string of `范` lands + /// exactly on 60_000 and sails through a completely broken implementation — + /// which is how the first version of this test passed without the fix. + #[test] + fn clip_lands_on_a_char_boundary_at_every_alignment() { + for pad in 0..3 { + // No newline anywhere, so the cut falls back to the boundary walk + // rather than being rescued by `rfind('\n')`. + let mut s = "a".repeat(pad); + s.push_str(&"范".repeat(DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES / 3 + 10)); + assert!(s.len() > DESCRIBE_SCREEN_TREE_TEXT_MAX_BYTES); + + let out = clip_tree_text(s.clone()); + assert!(out.contains("[truncated]"), "pad={pad}"); + let body = out.split("\n[truncated]").next().unwrap(); + assert!( + body.chars().all(|c| c == 'a' || c == '范'), + "clip split a character at pad={pad}" + ); + } + } + /// An empty snapshot must say *why* it is empty. A bare `ax_tree_text: /// null` reads as truncated tool output, and an agent that believes its /// results are being cut off re-issues the same call instead of changing