From f1f5a93e48cb7e5c79eb9abfc6b4a0c503c19ee2 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 16 Apr 2026 06:20:35 -0700 Subject: [PATCH] docs(ffi): sync skill references + crate rustdoc with shipped ABI (todo 011) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes P2 todo 011. Reference docs and the crate-level rustdoc had drifted against shipped APIs: - ownership.md table still listed the removed raw-array list surface (`ad_list_apps(... &apps, &count)`, `ad_free_apps`, `ad_free_window`, `ad_free_windows`, `ad_free_surfaces`) that Unit 5 replaced with opaque list handles. - build-and-link.md's minimal C example used the same stale API. - error-handling.md still called `ad_free_window` (renamed to `ad_release_window_fields` in Unit 5). - threading.md described debug-only `debug_assert!` with release-build UB, but todo 002 made the check runtime-enforced in every profile. - lib.rs crate-level rustdoc repeated the same debug/release phrasing. Rewrite all four reference pages and the lib.rs rustdoc against what actually ships: ownership.md: - Full opaque-list entry per type (Apps/Windows/Surfaces/Notifications). - AdImageBuffer accessor pattern documented. - ad_free_handle: *mut AdNativeHandle, zero-on-success double-free safety. - Out-param zeroing happens before guards, not after (todo 006 contract). threading.md: - "Runtime, every build profile" — no debug/release split. - Full exempt list: lists accessors, image-buffer accessors, release_window_fields, free_handle, free_tree, free_action_result, free_string. build-and-link.md: - C example now uses `ad_list_apps(adapter, &list)` + `ad_app_list_count/_get/_free`. error-handling.md: - `ad_free_window` → `ad_release_window_fields`. - Comment clarified to reflect out-param zero-init. lib.rs //! rustdoc: - Matches threading.md's runtime-enforced phrasing and exempt list. - cbindgen propagates to agent_desktop.h so header consumers see the right contract. 85 FFI tests pass, clippy clean. --- crates/ffi/src/lib.rs | 21 ++++-- .../references/build-and-link.md | 16 +++-- .../references/error-handling.md | 4 +- .../agent-desktop-ffi/references/ownership.md | 70 +++++++++++++------ .../agent-desktop-ffi/references/threading.md | 38 ++++++---- 5 files changed, 101 insertions(+), 48 deletions(-) diff --git a/crates/ffi/src/lib.rs b/crates/ffi/src/lib.rs index c150e3f..9c374a2 100644 --- a/crates/ffi/src/lib.rs +++ b/crates/ffi/src/lib.rs @@ -6,12 +6,21 @@ //! //! ## ⚠ Thread safety (macOS) //! -//! **Every FFI entry other than `ad_adapter_create`, `ad_adapter_destroy`, -//! `ad_last_error_*`, and the `ad_free_*` family must be invoked on the -//! process's main thread.** macOS accessibility and Cocoa APIs require -//! this and will misbehave silently on worker threads. Debug builds -//! assert this constraint; release builds do not (no-op `debug_assert!`) -//! but violators invoke undefined behavior. +//! Every adapter-touching FFI entry must be invoked on the process's +//! main thread. The guard runs at **runtime in every build profile**: +//! a worker-thread call returns `AD_RESULT_ERR_INTERNAL` with a +//! `'static` diagnostic message — no silent UB even under +//! `--profile release-ffi`. +//! +//! Operations exempt from the guard (safe from any thread): +//! +//! - `ad_adapter_create` / `ad_adapter_destroy` +//! - `ad_last_error_*` readers +//! - `ad_check_permissions` (process-wide query, no AX/Cocoa state) +//! - All `ad_*_list_{count,get,free}` accessors and +//! `ad_image_buffer_*` accessors +//! - `ad_release_window_fields`, `ad_free_handle`, `ad_free_tree`, +//! `ad_free_action_result`, `ad_free_string` //! //! ## Build profile //! diff --git a/skills/agent-desktop-ffi/references/build-and-link.md b/skills/agent-desktop-ffi/references/build-and-link.md index 86c3c74..fae1594 100644 --- a/skills/agent-desktop-ffi/references/build-and-link.md +++ b/skills/agent-desktop-ffi/references/build-and-link.md @@ -34,14 +34,20 @@ int main(void) { return 1; } - AdAppInfo *apps = NULL; - uint32_t count = 0; - rc = ad_list_apps(adapter, &apps, &count); + /* Opaque list handle — walk via _count / _get, free with _free. */ + AdAppList *list = NULL; + rc = ad_list_apps(adapter, &list); if (rc == AD_RESULT_OK) { + uint32_t count = ad_app_list_count(list); for (uint32_t i = 0; i < count; i++) { - printf("%s (pid %d)\n", apps[i].name, apps[i].pid); + const AdAppInfo *app = ad_app_list_get(list, i); + if (app) { + printf("%s (pid %d)\n", app->name, app->pid); + } } - ad_free_apps(apps, count); + ad_app_list_free(list); + } else { + fprintf(stderr, "list_apps failed: %s\n", ad_last_error_message()); } ad_adapter_destroy(adapter); diff --git a/skills/agent-desktop-ffi/references/error-handling.md b/skills/agent-desktop-ffi/references/error-handling.md index cc7e98c..777db03 100644 --- a/skills/agent-desktop-ffi/references/error-handling.md +++ b/skills/agent-desktop-ffi/references/error-handling.md @@ -14,11 +14,11 @@ if (rc != AD_RESULT_OK) { const char *sug = ad_last_error_suggestion(); // may be NULL fprintf(stderr, "launch_app failed (%d): %s\n", (int)rc, msg ? msg : "(no message)"); if (sug) fprintf(stderr, " suggestion: %s\n", sug); - // no need to call ad_free_window(&win) — out-param was zeroed + // no need to release the struct — out-param was zero-initialized return -1; } // ...use win... -ad_free_window(&win); +ad_release_window_fields(&win); ``` ## Lifetime contract diff --git a/skills/agent-desktop-ffi/references/ownership.md b/skills/agent-desktop-ffi/references/ownership.md index 630b074..c0d6568 100644 --- a/skills/agent-desktop-ffi/references/ownership.md +++ b/skills/agent-desktop-ffi/references/ownership.md @@ -7,34 +7,60 @@ free function. Always call it; the allocator the FFI uses is Rust's ## Allocation / release table -| Allocates | Frees with | -|----------------------------------------|----------------------------------| -| `ad_adapter_create()` | `ad_adapter_destroy(adapter)` | -| `ad_list_apps(... &apps, &count)` | `ad_free_apps(apps, count)` | -| `ad_list_windows(... &wins, &count)` | `ad_free_windows(wins, count)` | -| `ad_launch_app(... &out)` | `ad_free_window(&out)` | -| `ad_list_surfaces(... &sfs, &count)` | `ad_free_surfaces(sfs, count)` | -| `ad_get_tree(... &out)` | `ad_free_tree(&out)` | -| `ad_resolve_element(... &handle)` | `ad_free_handle(adapter, &handle)` | -| `ad_execute_action(... &out)` | `ad_free_action_result(&out)` | -| `ad_screenshot(... &img)` | `ad_free_image(&img)` | -| `ad_get_clipboard(... &text)` | `ad_free_string(text)` | +| Allocates | Frees with | +|---------------------------------------------------------|-----------------------------------------| +| `ad_adapter_create()` | `ad_adapter_destroy(adapter)` | +| `ad_list_apps(adapter, &list)` | `ad_app_list_free(list)` | +| `ad_list_windows(adapter, app, focused, &list)` | `ad_window_list_free(list)` | +| `ad_list_surfaces(adapter, pid, &list)` | `ad_surface_list_free(list)` | +| `ad_list_notifications(adapter, filter, &list)` | `ad_notification_list_free(list)` | +| `ad_dismiss_all_notifications(adapter, f, &ok, &fail)` | `ad_notification_list_free` on each (or `ad_dismiss_all_notifications_free(ok, fail)`) | +| `ad_launch_app(adapter, id, timeout, &out)` | `ad_release_window_fields(&out)` (free interior strings; struct itself lives on caller's stack) | +| `ad_get_tree(adapter, win, opts, &out)` | `ad_free_tree(&out)` | +| `ad_resolve_element(adapter, entry, &handle)` | `ad_free_handle(adapter, &handle)` — `*mut AdNativeHandle`; the call zeroes `handle.ptr` on success so a follow-up call is a no-op | +| `ad_find(adapter, win, query, &handle)` | same as `ad_resolve_element` | +| `ad_execute_action(adapter, handle, action, &out)` | `ad_free_action_result(&out)` | +| `ad_notification_action(adapter, idx, name, &out)` | `ad_free_action_result(&out)` | +| `ad_screenshot(adapter, target, &buf)` | `ad_image_buffer_free(buf)` (buf is opaque; read via `ad_image_buffer_{data,size,width,height,format}`) | +| `ad_get_clipboard(adapter, &text)` | `ad_free_string(text)` | +| `ad_get(adapter, handle, property, &text)` | `ad_free_string(text)` (text may be null on "property absent"; `ad_free_string(NULL)` is a no-op) | ## Rules - Every free function is **null-tolerant**. `ad_free_tree(NULL)`, - `ad_free_handle(adapter, NULL)`, etc. are no-ops. -- Double-free is **undefined behavior**. Set the pointer to `NULL` - after freeing. -- Pointers inside a struct (`.id`, `.title`, `.app_name`) are freed by - the struct's free function — do not `ad_free_string()` them - individually. + `ad_free_handle(adapter, NULL)`, `ad_free_string(NULL)`, etc. are + no-ops. List accessors (`ad_*_list_count`, `_get`) also accept null + and return `0` / `NULL` respectively. +- **Double-free of list handles and `AdImageBuffer` is undefined.** The + opaque wrappers are allocated by `Box::into_raw`; the second call + would invoke `Box::from_raw` on a freed allocation. Always set the + pointer to `NULL` after freeing. +- **`ad_free_handle` is safe to double-call** — it zeroes + `handle.ptr` after the platform release, so a follow-up call sees + `NULL` and returns `AD_RESULT_OK` without re-entering `CFRelease`. +- Pointers inside a struct (`.id`, `.title`, `.app_name`, each + `AdNotificationInfo.body`, etc.) are freed by the struct's owning + free function (list_free / release_fields) — do not + `ad_free_string()` them individually. - Ownership does **not** transfer back to Rust after you free. Keep a local `NULL` to prevent accidental reuse. ## Out-param zeroing -Every fallible FFI function zeroes its out-param at entry, before any -fallible work. On error, calling the paired free function is safe: all -pointers inside are guaranteed null, all counts zero, so the free is a -no-op rather than a double-free on a previous caller's allocation. +Every fallible FFI function zeroes its out-param **before** any guard +(pointer validation, main-thread check, UTF-8 validation). On error, +calling the paired free function is safe: all pointers inside are +guaranteed null, all counts zero, so the free is a no-op rather than +a double-free on a previous caller's allocation. + +In particular: + +- `ad_get_clipboard` writes `*out = NULL` before the adapter call — + no stale buffer visible on error. +- `ad_launch_app` writes `*out = zeroed AdWindowInfo` before the + platform call — `ad_release_window_fields(&out)` on the zero-init + struct is a no-op. +- `ad_screenshot` writes `*out = NULL` before allocating the image + buffer — no stale pointer when the screenshot fails. +- `ad_*_list` and `ad_resolve_element` / `ad_find` all apply the same + pattern to their handle / list out-params. diff --git a/skills/agent-desktop-ffi/references/threading.md b/skills/agent-desktop-ffi/references/threading.md index 5161c8f..1cb778f 100644 --- a/skills/agent-desktop-ffi/references/threading.md +++ b/skills/agent-desktop-ffi/references/threading.md @@ -2,24 +2,36 @@ ## macOS: main-thread rule -Every adapter-touching entrypoint (`ad_get_tree`, `ad_resolve_element`, -`ad_execute_action`, `ad_screenshot`, clipboard, mouse, drag, launch, -focus, window-op, list-*) **must be invoked on the process's main -thread**. macOS accessibility and Cocoa APIs require this. +Every adapter-touching entrypoint (`ad_get_tree`, `ad_find`, `ad_get`, +`ad_is`, `ad_resolve_element`, `ad_execute_action`, `ad_screenshot`, +clipboard get/set/clear, mouse, drag, launch, close, focus, window-op, +list-apps/windows/surfaces, notification list/dismiss/action) +**must be invoked on the process's main thread**. macOS accessibility +and Cocoa APIs require this. -- **Debug builds** assert the constraint via `pthread_main_np()` and - convert violations into `AD_RESULT_ERR_INTERNAL` with message - `"agent_desktop FFI entry called off the main thread"`. -- **Release builds** skip the check (no `debug_assert!`). Violations - are silent undefined behavior. +The check runs at **runtime, in every build profile** — worker-thread +calls return `AD_RESULT_ERR_INTERNAL` with a `'static` diagnostic +`"agent_desktop FFI entry called off the main thread (macOS requires +main-thread AX/Cocoa calls)"`. No build-config difference; no silent +UB window in release builds. -Operations that are **safe off-main-thread**: +On non-macOS targets the check is a compile-time `true` and has zero +runtime cost. + +Operations that are **safe off-main-thread** (no runtime guard): - `ad_adapter_create` / `ad_adapter_destroy` - `ad_last_error_{code,message,suggestion,platform_detail}` -- `ad_free_*` family (handle, tree, apps, windows, surfaces, image, - string, action_result) -- `ad_check_permissions` +- `ad_check_permissions` (pure process-wide query) +- `ad_app_list_count` / `_get` / `_free` +- `ad_window_list_count` / `_get` / `_free` +- `ad_surface_list_count` / `_get` / `_free` +- `ad_notification_list_count` / `_get` / `_free` +- `ad_image_buffer_data` / `_size` / `_width` / `_height` / `_format` / `_free` +- `ad_release_window_fields` +- `ad_free_handle` (invokes `CFRelease` which is thread-safe) — but + still prefer calling from the thread that produced the handle. +- `ad_free_tree`, `ad_free_action_result`, `ad_free_string` ## Python consumers