From 00d247449869e96aa4a108dd50ca403eedcfcafe Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 16 Apr 2026 04:03:34 -0700 Subject: [PATCH] =?UTF-8?q?feat(ffi):=20ABI=20surface=20completion=20?= =?UTF-8?q?=E2=80=94=20AdSnapshotSurface,=20focused=5Fonly,=20release=5Fha?= =?UTF-8?q?ndle=20(Unit=206)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lands the last set of ABI-shape changes before the header is published. Any addition after publication would be a breaking change for every consumer linked against the old layout. Core / platform additions: - crates/core/src/adapter.rs: new `PlatformAdapter::release_handle` trait method with `not_supported()` default. macOS implementations must `CFRelease` the underlying AXUIElementRef to balance the CFRetain that happened during resolve. Windows/Linux inherit the default unchanged. - crates/macos/src/adapter.rs: macOS impl calls `core_foundation::base::CFRelease(raw)` when the raw pointer is non-null. FFI surface additions: - crates/ffi/src/types/snapshot_surface.rs: new AdSnapshotSurface enum (Window=0, Focused, Menu, Menubar, Sheet, Popover, Alert) mirroring the 7-variant core SnapshotSurface. - crates/ffi/src/types/tree_options.rs: new `surface: AdSnapshotSurface` field. Previous hard-coded SnapshotSurface::Window is replaced by consumer-selected surface with enum-validated read at entry. - crates/ffi/src/windows/list.rs: ad_list_windows gains `focused_only: bool` between app_filter and out — maps directly to WindowFilter::focused_only. Consumers can now call `ad_focused_window`- style queries without a second ABI round. - crates/ffi/src/actions/native_handle.rs: new `ad_free_handle(adapter, handle)` exported. Null-tolerant on both sides; Windows/Linux `ActionNotSupported` is translated to `Ok` so C consumers can call the release path uniformly across platforms. Validation: - AdSnapshotSurface added to enum_validation.rs — ad_get_tree now rejects invalid surface discriminants with `AD_RESULT_ERR_INVALID_ARGS` rather than matching on UB. Closes R5, R9, R10 from PR #22 review. 52 lib tests pass, 1 integration test passes. Clippy clean. --- crates/core/src/adapter.rs | 9 ++++ crates/ffi/build.rs | 5 +- crates/ffi/include/agent_desktop.h | 31 +++++++++++++ crates/ffi/src/actions/mod.rs | 1 + crates/ffi/src/actions/native_handle.rs | 58 ++++++++++++++++++++++++ crates/ffi/src/enum_validation.rs | 9 +++- crates/ffi/src/lib.rs | 1 + crates/ffi/src/tree/get.rs | 28 +++++++++++- crates/ffi/src/types/mod.rs | 2 + crates/ffi/src/types/snapshot_surface.rs | 11 +++++ crates/ffi/src/types/tree_options.rs | 3 ++ crates/ffi/src/windows/list.rs | 3 +- crates/macos/src/adapter.rs | 11 +++++ 13 files changed, 164 insertions(+), 8 deletions(-) create mode 100644 crates/ffi/src/actions/native_handle.rs create mode 100644 crates/ffi/src/types/snapshot_surface.rs diff --git a/crates/core/src/adapter.rs b/crates/core/src/adapter.rs index af3732d..3c6be52 100644 --- a/crates/core/src/adapter.rs +++ b/crates/core/src/adapter.rs @@ -141,6 +141,15 @@ pub trait PlatformAdapter: Send + Sync { Err(AdapterError::not_supported("resolve_element")) } + /// Releases a platform-specific element handle returned from + /// `resolve_element`. macOS implementations must `CFRelease` the + /// underlying `AXUIElementRef` to balance the `CFRetain` that + /// happened during resolve. Windows/Linux consumers can leave this + /// as the default `not_supported` no-op. + fn release_handle(&self, _handle: &NativeHandle) -> Result<(), AdapterError> { + Err(AdapterError::not_supported("release_handle")) + } + fn check_permissions(&self) -> PermissionStatus { PermissionStatus::Denied { suggestion: "Platform adapter not available".into(), diff --git a/crates/ffi/build.rs b/crates/ffi/build.rs index 2d5aefd..f6e5c5b 100644 --- a/crates/ffi/build.rs +++ b/crates/ffi/build.rs @@ -46,10 +46,7 @@ fn main() { let include_dir = Path::new(&crate_dir).join("include"); if let Err(err) = std::fs::create_dir_all(&include_dir) { - panic!( - "failed to create include dir at {:?}: {}", - include_dir, err - ); + panic!("failed to create include dir at {:?}: {}", include_dir, err); } let committed_header = include_dir.join("agent_desktop.h"); diff --git a/crates/ffi/include/agent_desktop.h b/crates/ffi/include/agent_desktop.h index 7f7ef05..aefb6dd 100644 --- a/crates/ffi/include/agent_desktop.h +++ b/crates/ffi/include/agent_desktop.h @@ -91,6 +91,17 @@ enum AdScreenshotKind { }; typedef int32_t AdScreenshotKind; +enum AdSnapshotSurface { + AD_SNAPSHOT_SURFACE_WINDOW = 0, + AD_SNAPSHOT_SURFACE_FOCUSED = 1, + AD_SNAPSHOT_SURFACE_MENU = 2, + AD_SNAPSHOT_SURFACE_MENUBAR = 3, + AD_SNAPSHOT_SURFACE_SHEET = 4, + AD_SNAPSHOT_SURFACE_POPOVER = 5, + AD_SNAPSHOT_SURFACE_ALERT = 6, +}; +typedef int32_t AdSnapshotSurface; + enum AdWindowOpKind { AD_WINDOW_OP_KIND_RESIZE = 0, AD_WINDOW_OP_KIND_MOVE = 1, @@ -233,6 +244,7 @@ typedef struct AdTreeOptions { bool include_bounds; bool interactive_only; bool compact; + AdSnapshotSurface surface; } AdTreeOptions; typedef struct AdWindowOp { @@ -256,6 +268,24 @@ AdResult ad_execute_action(const struct AdAdapter *adapter, const struct AdAction *action, struct AdActionResult *out); +/** + * Releases a handle previously returned by `ad_resolve_element`. + * + * On macOS this calls `CFRelease` on the underlying `AXUIElementRef`, + * balancing the `CFRetain` that happened during `ad_resolve_element`. + * On Windows/Linux the call is a no-op that returns `AD_RESULT_OK` + * (platform adapters inherit the default `not_supported` impl, which + * the FFI surface rewrites to `Ok` here so callers can apply the same + * release pattern everywhere). + * + * # Safety + * + * `adapter` must be a non-null pointer returned by `ad_adapter_create`. + * `handle` must be null or a pointer previously populated by + * `ad_resolve_element`. Double-free is undefined behavior. + */ +AdResult ad_free_handle(const struct AdAdapter *adapter, const struct AdNativeHandle *handle); + /** * # Safety * @@ -457,6 +487,7 @@ void ad_free_window(struct AdWindowInfo *win); */ AdResult ad_list_windows(const struct AdAdapter *adapter, const char *app_filter, + bool focused_only, struct AdWindowInfo **out, uint32_t *out_count); diff --git a/crates/ffi/src/actions/mod.rs b/crates/ffi/src/actions/mod.rs index febccf0..c2d2119 100644 --- a/crates/ffi/src/actions/mod.rs +++ b/crates/ffi/src/actions/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod conversion; pub(crate) mod execute; +pub(crate) mod native_handle; pub(crate) mod resolve; pub(crate) mod result; diff --git a/crates/ffi/src/actions/native_handle.rs b/crates/ffi/src/actions/native_handle.rs new file mode 100644 index 0000000..4a7328d --- /dev/null +++ b/crates/ffi/src/actions/native_handle.rs @@ -0,0 +1,58 @@ +use crate::error::{set_last_error, AdResult}; +use crate::ffi_try::trap_panic; +use crate::types::AdNativeHandle; +use crate::AdAdapter; +use agent_desktop_core::adapter::NativeHandle; + +/// Releases a handle previously returned by `ad_resolve_element`. +/// +/// On macOS this calls `CFRelease` on the underlying `AXUIElementRef`, +/// balancing the `CFRetain` that happened during `ad_resolve_element`. +/// On Windows/Linux the call is a no-op that returns `AD_RESULT_OK` +/// (platform adapters inherit the default `not_supported` impl, which +/// the FFI surface rewrites to `Ok` here so callers can apply the same +/// release pattern everywhere). +/// +/// # Safety +/// +/// `adapter` must be a non-null pointer returned by `ad_adapter_create`. +/// `handle` must be null or a pointer previously populated by +/// `ad_resolve_element`. Double-free is undefined behavior. +#[no_mangle] +pub unsafe extern "C" fn ad_free_handle( + adapter: *const AdAdapter, + handle: *const AdNativeHandle, +) -> AdResult { + trap_panic(|| unsafe { + if adapter.is_null() { + set_last_error(&agent_desktop_core::error::AdapterError::new( + agent_desktop_core::error::ErrorCode::InvalidArgs, + "adapter is null", + )); + return AdResult::ErrInvalidArgs; + } + if handle.is_null() { + return AdResult::Ok; + } + let adapter = &*adapter; + let raw = (*handle).ptr; + if raw.is_null() { + return AdResult::Ok; + } + let native = NativeHandle::from_ptr(raw); + match adapter.inner.release_handle(&native) { + Ok(()) => AdResult::Ok, + Err(e) => { + // Not-supported on Windows/Linux is a no-op by contract. + if matches!( + e.code, + agent_desktop_core::error::ErrorCode::ActionNotSupported + ) { + return AdResult::Ok; + } + set_last_error(&e); + crate::error::last_error_code() + } + } + }) +} diff --git a/crates/ffi/src/enum_validation.rs b/crates/ffi/src/enum_validation.rs index 2786e15..3106e47 100644 --- a/crates/ffi/src/enum_validation.rs +++ b/crates/ffi/src/enum_validation.rs @@ -23,7 +23,7 @@ use crate::types::{ AdActionKind, AdDirection, AdImageFormat, AdModifier, AdMouseButton, AdMouseEventKind, - AdScreenshotKind, AdWindowOpKind, + AdScreenshotKind, AdSnapshotSurface, AdWindowOpKind, }; /// Reads the raw `i32` discriminant out of an `#[repr(i32)]` enum field @@ -100,6 +100,13 @@ try_from_c_enum! { } } +try_from_c_enum! { + AdSnapshotSurface { + Window = 0, Focused = 1, Menu = 2, Menubar = 3, + Sheet = 4, Popover = 5, Alert = 6, + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/ffi/src/lib.rs b/crates/ffi/src/lib.rs index 766281b..ec03bbf 100644 --- a/crates/ffi/src/lib.rs +++ b/crates/ffi/src/lib.rs @@ -72,6 +72,7 @@ pub use types::ref_entry::AdRefEntry; pub use types::screenshot_kind::AdScreenshotKind; pub use types::screenshot_target::AdScreenshotTarget; pub use types::scroll_params::AdScrollParams; +pub use types::snapshot_surface::AdSnapshotSurface; pub use types::surface_info::AdSurfaceInfo; pub use types::tree_options::AdTreeOptions; pub use types::window_info::AdWindowInfo; diff --git a/crates/ffi/src/tree/get.rs b/crates/ffi/src/tree/get.rs index be0b3a0..c4d46ba 100644 --- a/crates/ffi/src/tree/get.rs +++ b/crates/ffi/src/tree/get.rs @@ -1,10 +1,24 @@ +use crate::enum_validation::enum_raw_i32; use crate::error::{set_last_error, AdResult}; use crate::ffi_try::trap_panic; use crate::tree::flatten::flatten_tree; -use crate::types::{AdNodeTree, AdTreeOptions, AdWindowInfo}; +use crate::types::{AdNodeTree, AdSnapshotSurface, AdTreeOptions, AdWindowInfo}; use crate::AdAdapter; +use agent_desktop_core::adapter::SnapshotSurface; use std::ptr; +fn core_surface(s: AdSnapshotSurface) -> SnapshotSurface { + match s { + AdSnapshotSurface::Window => SnapshotSurface::Window, + AdSnapshotSurface::Focused => SnapshotSurface::Focused, + AdSnapshotSurface::Menu => SnapshotSurface::Menu, + AdSnapshotSurface::Menubar => SnapshotSurface::Menubar, + AdSnapshotSurface::Sheet => SnapshotSurface::Sheet, + AdSnapshotSurface::Popover => SnapshotSurface::Popover, + AdSnapshotSurface::Alert => SnapshotSurface::Alert, + } +} + /// # Safety /// All pointers must be valid. `out` must be writable. #[no_mangle] @@ -30,12 +44,22 @@ pub unsafe extern "C" fn ad_get_tree( return crate::error::last_error_code(); } }; + let surface = match AdSnapshotSurface::from_c(enum_raw_i32(&opts_ref.surface)) { + Some(s) => core_surface(s), + None => { + set_last_error(&agent_desktop_core::error::AdapterError::new( + agent_desktop_core::error::ErrorCode::InvalidArgs, + "invalid snapshot surface discriminant", + )); + return AdResult::ErrInvalidArgs; + } + }; let core_opts = agent_desktop_core::adapter::TreeOptions { max_depth: opts_ref.max_depth, include_bounds: opts_ref.include_bounds, interactive_only: opts_ref.interactive_only, compact: opts_ref.compact, - surface: agent_desktop_core::adapter::SnapshotSurface::Window, + surface, }; match adapter.inner.get_tree(&core_win, &core_opts) { diff --git a/crates/ffi/src/types/mod.rs b/crates/ffi/src/types/mod.rs index 03b0295..93fb334 100644 --- a/crates/ffi/src/types/mod.rs +++ b/crates/ffi/src/types/mod.rs @@ -21,6 +21,7 @@ pub mod ref_entry; pub mod screenshot_kind; pub mod screenshot_target; pub mod scroll_params; +pub mod snapshot_surface; pub mod surface_info; pub mod tree_options; pub mod window_info; @@ -50,6 +51,7 @@ pub use ref_entry::AdRefEntry; pub use screenshot_kind::AdScreenshotKind; pub use screenshot_target::AdScreenshotTarget; pub use scroll_params::AdScrollParams; +pub use snapshot_surface::AdSnapshotSurface; pub use surface_info::AdSurfaceInfo; pub use tree_options::AdTreeOptions; pub use window_info::AdWindowInfo; diff --git a/crates/ffi/src/types/snapshot_surface.rs b/crates/ffi/src/types/snapshot_surface.rs new file mode 100644 index 0000000..d925045 --- /dev/null +++ b/crates/ffi/src/types/snapshot_surface.rs @@ -0,0 +1,11 @@ +#[repr(i32)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AdSnapshotSurface { + Window = 0, + Focused = 1, + Menu = 2, + Menubar = 3, + Sheet = 4, + Popover = 5, + Alert = 6, +} diff --git a/crates/ffi/src/types/tree_options.rs b/crates/ffi/src/types/tree_options.rs index c72cf33..8eb0355 100644 --- a/crates/ffi/src/types/tree_options.rs +++ b/crates/ffi/src/types/tree_options.rs @@ -1,7 +1,10 @@ +use crate::types::snapshot_surface::AdSnapshotSurface; + #[repr(C)] pub struct AdTreeOptions { pub max_depth: u8, pub include_bounds: bool, pub interactive_only: bool, pub compact: bool, + pub surface: AdSnapshotSurface, } diff --git a/crates/ffi/src/windows/list.rs b/crates/ffi/src/windows/list.rs index 056af43..f62eb08 100644 --- a/crates/ffi/src/windows/list.rs +++ b/crates/ffi/src/windows/list.rs @@ -14,6 +14,7 @@ use std::ptr; pub unsafe extern "C" fn ad_list_windows( adapter: *const AdAdapter, app_filter: *const c_char, + focused_only: bool, out: *mut *mut AdWindowInfo, out_count: *mut u32, ) -> AdResult { @@ -22,7 +23,7 @@ pub unsafe extern "C" fn ad_list_windows( *out_count = 0; let adapter = &*adapter; let filter = WindowFilter { - focused_only: false, + focused_only, app: c_to_string(app_filter), }; match adapter.inner.list_windows(&filter) { diff --git a/crates/macos/src/adapter.rs b/crates/macos/src/adapter.rs index adddc64..7f35e92 100644 --- a/crates/macos/src/adapter.rs +++ b/crates/macos/src/adapter.rs @@ -67,6 +67,17 @@ impl PlatformAdapter for MacOSAdapter { crate::tree::resolve::resolve_element_impl(entry) } + fn release_handle(&self, handle: &NativeHandle) -> Result<(), AdapterError> { + let raw = handle.as_raw(); + if raw.is_null() { + return Ok(()); + } + unsafe { + core_foundation::base::CFRelease(raw as core_foundation::base::CFTypeRef); + } + Ok(()) + } + fn list_windows(&self, filter: &WindowFilter) -> Result, AdapterError> { list_windows_impl(filter) }