fix(computer-use): bound AX payloads, report true image dimensions, cap osascript

src/apps/desktop/src/computer_use/desktop_host/mod.rs

@@ -48,6 +48,55 @@ const OPEN_APP_WINDOW_WAIT_MS: u64 = 8_000;
 #[cfg(target_os = "macos")]
 const OPEN_APP_POLL_INTERVAL_MS: u64 = 150;
 
+/// How long an `open_app` AppleScript may run before it is killed.
+///
+/// `activate` sends an AppleEvent to the target app and waits for it to answer.
+/// A hung or busy app simply does not answer, and macOS's default AppleEvent
+/// timeout is **120 seconds** — during which `open_app` occupies a blocking
+/// thread and the agent has no idea anything is wrong. An app that has not
+/// acknowledged activation in a few seconds is not going to.
+#[cfg(target_os = "macos")]
+const OSASCRIPT_TIMEOUT_MS: u64 = 10_000;
+
+/// Run `osascript -e <script>`, killing it if it outlives `timeout_ms`.
+///
+/// `Command::output()` has no timeout, so a wedged AppleEvent blocks until
+/// macOS gives up. Polling `try_wait` lets us bound it. Output here is a bundle
+/// id or an error line, far below the pipe buffer, so draining after exit
+/// cannot deadlock — and a child that did fill the buffer would stop making
+/// progress and get killed by this same deadline.
+#[cfg(target_os = "macos")]
+fn run_osascript_bounded(script: &str, timeout_ms: u64) -> std::io::Result<std::process::Output> {
+    use std::process::Stdio;
+
+    let mut child = std::process::Command::new("/usr/bin/osascript")
+        .args(["-e", script])
+        .stdin(Stdio::null())
+        .stdout(Stdio::piped())
+        .stderr(Stdio::piped())
+        .spawn()?;
+
+    let deadline = std::time::Instant::now() + std::time::Duration::from_millis(timeout_ms);
+    loop {
+        match child.try_wait()? {
+            // Exited: `wait_with_output` below returns the recorded status.
+            Some(_) => break,
+            None => {
+                if std::time::Instant::now() >= deadline {
+                    let _ = child.kill();
+                    let _ = child.wait();
+                    return Err(std::io::Error::new(
+                        std::io::ErrorKind::TimedOut,
+                        format!("osascript did not finish within {}ms", timeout_ms),
+                    ));
+                }
+                std::thread::sleep(std::time::Duration::from_millis(20));
+            }
+        }
+    }
+    child.wait_with_output()
+}
+
 /// Quote a string as an AppleScript literal.
 ///
 /// App names reach us from the model and can contain quotes or backslashes;
@@ -193,6 +242,33 @@ end tell"#;
         assert!(compiles(broken).is_err());
     }
 
+    /// A wedged AppleEvent must not pin a blocking thread for macOS's 120s
+    /// default. `delay` inside osascript is a real hang from our side: the
+    /// process is alive and unresponsive, exactly like an app that never
+    /// acknowledges activation.
+    #[test]
+    fn a_hung_applescript_is_killed_at_the_deadline() {
+        let started = std::time::Instant::now();
+        let err = run_osascript_bounded("delay 30", 700)
+            .expect_err("a 30s script under a 700ms budget must not succeed");
+
+        assert_eq!(err.kind(), std::io::ErrorKind::TimedOut, "{err}");
+        assert!(
+            started.elapsed() < std::time::Duration::from_secs(5),
+            "returned after {:?} — the deadline did not take effect",
+            started.elapsed()
+        );
+    }
+
+    /// The bounded runner must stay a drop-in for the normal path: same stdout,
+    /// same exit status.
+    #[test]
+    fn a_normal_applescript_still_returns_its_output() {
+        let out = run_osascript_bounded("return \"ok\"", OSASCRIPT_TIMEOUT_MS).expect("should run");
+        assert!(out.status.success());
+        assert_eq!(String::from_utf8_lossy(&out.stdout).trim(), "ok");
+    }
+
     #[test]
     fn applescript_quote_escapes_quotes_and_backslashes() {
         // App names come from the model, so a name containing a quote must not
@@ -576,24 +652,33 @@ impl DesktopComputerUseHost {
         // `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()
-            .map_err(|e| BitFunError::tool(format!("open_app osascript: {}", e)))?;
+        let bundle_id = run_osascript_bounded(
+            &format!("id of application {}", applescript_quote(&name)),
+            OSASCRIPT_TIMEOUT_MS,
+        )
+        .ok()
+        .filter(|o| o.status.success())
+        .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string())
+        .filter(|s| !s.is_empty());
+
+        let activate = match run_osascript_bounded(
+            &format!("tell application {} to activate", applescript_quote(&name)),
+            OSASCRIPT_TIMEOUT_MS,
+        ) {
+            Ok(out) => out,
+            // A timeout is a real outcome, not an internal error: the app is
+            // installed but not answering. Report it as a failed launch the
+            // agent can act on rather than bubbling an opaque io error.
+            Err(e) if e.kind() == std::io::ErrorKind::TimedOut => {
+                return Ok(failure(format!(
+                    "'{}' did not respond to activation within {}s — it may be hung or showing a modal dialog. \
+Check the app directly, or ask the user to bring it up.",
+                    name,
+                    OSASCRIPT_TIMEOUT_MS / 1000
+                )));
+            }
+            Err(e) => return Err(BitFunError::tool(format!("open_app osascript: {}", e))),
+        };
         if !activate.status.success() {
             return Ok(failure(
                 String::from_utf8_lossy(&activate.stderr).trim().to_string(),
@@ -1266,6 +1351,18 @@ impl ComputerUseHost for DesktopComputerUseHost {
             recommended_next_action = Some("screenshot".to_string());
         }
 
+        // `interaction_state` rides on *every* ComputerUse result, and the
+        // display list is the bulk of it. On a single-screen machine it is pure
+        // repetition: `active_display_id` already names the only screen, and
+        // there is nothing to disambiguate. It earns its bytes only when the
+        // model actually has to choose, so send it only then — `list_displays`
+        // and `describe_screen` still report the full list on demand.
+        let displays = if displays.len() > 1 {
+            displays
+        } else {
+            Vec::new()
+        };
+
         ComputerUseInteractionState {
             click_ready,
             enter_ready: !click_needs_fresh,

src/crates/assembly/core/src/agentic/image_analysis/image_processing.rs

@@ -21,8 +21,41 @@ use tokio::fs;
 pub struct ProcessedImage {
     pub data: Vec<u8>,
     pub mime_type: String,
+    /// Width of the image **as sent to the model** — not of the source file.
+    /// Downscaling to fit the provider's limits can shrink this a long way
+    /// (repeated 0.75× passes, floor 64px).
     pub width: u32,
+    /// Height as sent to the model. See [`Self::width`].
     pub height: u32,
+    /// Width of the source image, before any resizing.
+    ///
+    /// Reported separately because callers surface these numbers to a model,
+    /// and a resized dimension presented as the file's dimension is silently
+    /// wrong: anything the vision model says about position or size is in the
+    /// resized frame, and the caller has no way to map it back without knowing
+    /// the original.
+    pub original_width: u32,
+    /// Height of the source image, before any resizing.
+    pub original_height: u32,
+}
+
+impl ProcessedImage {
+    /// Linear factor from source pixels to sent pixels (1.0 when untouched).
+    ///
+    /// Aspect ratio is preserved by every resize path here, so one factor
+    /// describes both axes; it is derived from width to avoid disagreeing with
+    /// itself on rounding.
+    pub fn scale(&self) -> f64 {
+        if self.original_width == 0 {
+            return 1.0;
+        }
+        f64::from(self.width) / f64::from(self.original_width)
+    }
+
+    /// Whether the image was downscaled on the way to the model.
+    pub fn was_resized(&self) -> bool {
+        self.width != self.original_width || self.height != self.original_height
+    }
 }
 
 pub fn resolve_vision_model_from_ai_config(
@@ -178,6 +211,8 @@ pub fn optimize_image_with_size_limit(
             mime_type,
             width: orig_width,
             height: orig_height,
+            original_width: orig_width,
+            original_height: orig_height,
         });
     }
 
@@ -231,6 +266,8 @@ pub fn optimize_image_with_size_limit(
         mime_type: encoded.1,
         width: working.width(),
         height: working.height(),
+        original_width: orig_width,
+        original_height: orig_height,
     })
 }
 
@@ -484,3 +521,78 @@ fn encode_dynamic_image(
 
     Ok((buffer, mime))
 }
+
+#[cfg(test)]
+mod resize_reporting_tests {
+    use super::*;
+
+    fn png_of(width: u32, height: u32) -> Vec<u8> {
+        let img =
+            image::DynamicImage::ImageRgb8(image::RgbImage::from_fn(width, height, |x, y| {
+                // Non-uniform content so the encoder cannot collapse it to nothing,
+                // which would let the size-based resize passes be skipped.
+                image::Rgb([(x % 251) as u8, (y % 253) as u8, ((x ^ y) % 247) as u8])
+            }));
+        let mut out = std::io::Cursor::new(Vec::new());
+        img.write_to(&mut out, ImageFormat::Png)
+            .expect("encode test png");
+        out.into_inner()
+    }
+
+    /// A Retina screenshot is wider than every provider's limit, so it always
+    /// takes the resize path. The source dimensions were being computed and
+    /// then dropped, so callers reported the *resized* size as the file's —
+    /// silently wrong data, and the reason a caller mapping a reported position
+    /// back to the screen would land in the wrong place.
+    #[test]
+    fn a_resized_screenshot_still_reports_its_source_dimensions() {
+        let processed = optimize_image_with_size_limit(
+            png_of(3024, 1964),
+            "anthropic",
+            Some("image/png"),
+            Some(1024 * 1024),
+        )
+        .expect("optimize");
+
+        assert_eq!(processed.original_width, 3024);
+        assert_eq!(processed.original_height, 1964);
+        assert!(
+            processed.was_resized(),
+            "3024px exceeds every provider limit"
+        );
+        assert!(
+            processed.width < processed.original_width,
+            "sent {}px for a {}px source",
+            processed.width,
+            processed.original_width
+        );
+
+        // The factor has to actually describe the transform, or mapping back
+        // through it is worse than not having it.
+        let mapped = f64::from(processed.original_width) * processed.scale();
+        assert!(
+            (mapped - f64::from(processed.width)).abs() <= 1.0,
+            "scale {} maps {} to {}, expected ~{}",
+            processed.scale(),
+            processed.original_width,
+            mapped,
+            processed.width
+        );
+    }
+
+    /// An image already within limits must pass through untouched, and say so.
+    #[test]
+    fn a_small_image_is_not_reported_as_resized() {
+        let processed =
+            optimize_image_with_size_limit(png_of(80, 60), "anthropic", Some("image/png"), None)
+                .expect("optimize");
+
+        assert_eq!((processed.width, processed.height), (80, 60));
+        assert_eq!(
+            (processed.original_width, processed.original_height),
+            (80, 60)
+        );
+        assert!(!processed.was_resized());
+        assert!((processed.scale() - 1.0).abs() < f64::EPSILON);
+    }
+}

src/crates/assembly/core/src/agentic/tools/implementations/analyze_image_tool.rs

@@ -368,16 +368,48 @@ impl Tool for AnalyzeImageTool {
             .take(180)
             .collect::<String>();
 
-        let data = json!({
+        // Coordinate contract. Two separate traps live here:
+        //
+        // 1. `width`/`height` are the dimensions of the image **the model
+        //    saw**, which is often not the file's — large screenshots get
+        //    downscaled to fit the provider's limits. Reporting only those, as
+        //    this did, silently hands back numbers that do not describe the
+        //    file the caller passed in.
+        // 2. Any position or size in `analysis` is prose from a vision model:
+        //    estimated, not measured, and expressed in the resized frame. It is
+        //    not a click target, and treating it as one means compounding a
+        //    guess with a scale factor and (on Retina) a device-pixel ratio.
+        let mut data = json!({
             "path": resolved.display_path(),
             "model_id": vision_model.id,
             "model_name": vision_model.model_name,
             "mime_type": processed.mime_type,
             "width": processed.width,
             "height": processed.height,
+            "original_width": processed.original_width,
+            "original_height": processed.original_height,
+            "was_resized": processed.was_resized(),
             "summary": summary,
             "analysis": analysis,
+            "coordinate_note": "Any position or size mentioned in `analysis` is the vision model's estimate, in the frame of the image it was shown (`width`x`height`). It is not a measurement and not a click target. To act on something on screen, use `ComputerUse` `locate` / `move_to_text` / `describe_screen`, which return real coordinates.",
         });
+        if processed.was_resized() {
+            if let Some(obj) = data.as_object_mut() {
+                obj.insert("scale_from_original".to_string(), json!(processed.scale()));
+                obj.insert(
+                    "resize_note".to_string(),
+                    json!(format!(
+                        "The image was downscaled {}x{} -> {}x{} (x{:.4}) to fit the vision model. \
+Divide by that factor to map anything in `analysis` back to source pixels — but prefer not to: see `coordinate_note`.",
+                        processed.original_width,
+                        processed.original_height,
+                        processed.width,
+                        processed.height,
+                        processed.scale(),
+                    )),
+                );
+            }
+        }
 
         Ok(vec![ToolResult::ok(
             data,

src/crates/assembly/core/src/agentic/tools/implementations/computer_use_actions.rs

@@ -775,12 +775,20 @@ impl ComputerUseActions {
         fn snap_state_json(
             snap: &crate::agentic::tools::computer_use_host::AppStateSnapshot,
         ) -> serde_json::Value {
+            // Bounded here rather than at the `get_app_state` call site: every
+            // `app_click` / `app_type_text` / `app_scroll` / `app_key_chord` /
+            // `app_wait_for` result carries this same post-action tree, so they
+            // all shared the same unbounded-payload risk.
+            let tree_text = super::computer_use_tool::clip_tree_text(
+                snap.tree_text.clone(),
+                super::computer_use_tool::APP_STATE_TREE_TEXT_MAX_BYTES,
+            );
             let mut v = json!({
                 "app": snap.app,
                 "window_title": snap.window_title,
                 "digest": snap.digest,
                 "captured_at_ms": snap.captured_at_ms,
-                "tree_text": snap.tree_text,
+                "tree_text": tree_text,
                 "node_count": snap.nodes.len(),
                 "has_screenshot": snap.screenshot.is_some(),
             });