cua-driver: kill focus-handle leak class via 4-layer scope binding - #1521
Merged
Conversation
Symptom (v0.1.9): a leaked SuppressionHandle in
SystemFocusStealPreventer turns the wildcard observer into a
process-lifetime focus trap. The user cannot switch to other apps --
every NSWorkspace activation notification re-routes back to
'restoreTo' (whatever was frontmost at the leak point). High CPU
because every OS-level focus change fires Task.detached -> MainActor
-> activate(). Recovery only by killing the process.
Root cause is *not* a missing defer. It is the API shape:
beginSuppression(...) -> handle / endSuppression(handle) couples
acquire and release across module boundaries, async boundaries, and
error boundaries. ClickTool had 6+ early-return paths between
WindowChangeDetector.snapshot() (begin) and detectChanges() (end);
LaunchAppTool had a multi-phase placeholder->pid swap with begin/end
pairs in three branches. Any future copy-paste of either pattern,
plus a forgotten cleanup branch, reproduces the leak. Singleton +
no deadline + global observer = self-amplifying.
Fix: four overlapping leak-prevention layers, ranked by depth.
Layer 1 - closure scope (preferred). withSuppression { ... } pairs
begin/end with a defer the caller cannot accidentally skip. No
handle escapes the closure. LaunchAppTool's 500ms post-launch
re-arm now uses this.
Layer 2 - ARC scope (snapshot/detect pattern). leaseSuppression()
returns a SuppressionLease whose deinit fires a fire-and-forget
release(). When the lifetime must span function boundaries (the
Snapshot struct held by the caller), ARC catches what scope-defer
cannot -- thrown errors between begin and end, task cancellation,
future call-site regressions. WindowChangeDetector.Snapshot,
FocusGuard.withFocusSuppressed, and LaunchAppTool's placeholder
arm now use this.
Layer 3 - wall-clock deadline (the safety net under everything).
Every entry carries a 5s monotonic deadline. The dispatcher
evicts expired entries on every observer fire (so a leaked
wildcard stops hijacking activations BEFORE the next user
app-switch) and on a 1s janitor (so idle leaks recover too).
Worst-case leak duration is bounded by maxLifetimeNs, regardless
of higher-layer correctness.
Layer 4 - observability. Every entry carries an origin tag
(#function-derived or explicit string). Active count > 4 logs
warning to os.Logger ('io.trycua.cua-driver/FocusStealPreventer')
with the full origin list -- future leaks surface in
'log show --process cua-driver' instead of silently stealing
focus. Deadline reaps log at .error so missing release paths
pinpoint themselves.
Migration: beginSuppression / endSuppression are kept, marked
@available(*, deprecated). All internal call sites switched to the
scoped APIs. External callers continue to compile but now also
benefit from the deadline safety net.
Test coverage (8 new tests, 0.7s total runtime):
- withSuppression releases on return
- withSuppression releases on throw
- lease releases on explicit call
- lease release is idempotent
- lease releases on deinit (ARC contract)
- deadline reaps leaked manual entry (the v0.1.9 regression test)
- deadline reaps only expired entries (precision check)
- endSuppression after deadline reap is a no-op
The deadline test uses a 200ms test maxLifetime via a public init
seam -- production keeps the 5s default. _forceReapForTesting()
exposes the reap path so tests do not have to fire NSWorkspace
notifications from a unit-test context.
This makes the v0.1.9 focus-trap regression class structurally
impossible: no caller can leave an entry alive longer than
maxLifetimeNs, regardless of which API surface they used or how
their error path unwinds.
Contributor
|
@hoang17 is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a multi-layered leak-safe focus suppression mechanism. It adds ChangesFocus suppression leak prevention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swift (1)
242-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the placeholder lease alive until the pid-specific suppression is armed.
Line 246 releases the wildcard entry before Lines 269-275 install the pid-specific one. An activation that lands in that gap will bypass layer 3 and briefly steal focus again.
🐛 Proposed fix
- // Hand the placeholder lease back to the dispatcher now that - // launch returned. We're about to swap to a pid-specific - // entry — the placeholder has done its job catching any - // intra-`launch` activation. - await placeholderLease?.release() - // Replace the placeholder pid with the real one so any // activation notification the target emits from now on is // caught. Observed activations that fired DURING the launch // (synchronous `open`) will have been seen by the observer // but not matched (pid=0 mismatch), so they pass through — @@ if shouldSuppress, let priorFrontmost { // Closure-scoped suppression around the 500ms wait — // the entry releases on every exit path (return, // throw, cancellation). No manual end pairing. // // 500ms is enough for applicationDidFinishLaunching // plus any reflex NSApp.activate to fire and get // suppressed. await AppStateRegistry.systemFocusStealPreventer .withSuppression( targetPid: info.pid, restoreTo: priorFrontmost, origin: "LaunchAppTool.postLaunch" ) { + await placeholderLease?.release() try? await Task.sleep(nanoseconds: 500_000_000) } + } else { + await placeholderLease?.release() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swift` around lines 242 - 275, The placeholder lease is released too early (placeholderLease.release() happens before the pid-specific suppression is installed), creating a window where activations can bypass suppression; keep the placeholder lease alive until after the pid-specific suppression is armed by moving the release to after the call to AppStateRegistry.systemFocusStealPreventer.withSuppression (or use a defer/paired release that executes once the withSuppression invocation has completed/been installed) so that placeholderLease remains held while installing the pid-specific entry (referencing placeholderLease, info.pid, priorFrontmost, and AppStateRegistry.systemFocusStealPreventer.withSuppression / "LaunchAppTool.postLaunch").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swift`:
- Around line 159-171: The defer currently spawns an unstructured Task to call
snapshot.suppressionLease?.release(), allowing detectChanges to return before
the lease is released; replace the Task-based release with a direct await so the
lease is torn down before returning (i.e., change the defer to: if let lease =
snapshot.suppressionLease { await lease.release() }), and apply the same fix to
the other symmetric defer that releases suppressionLease (the one referenced in
the review for the second block).
---
Outside diff comments:
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swift`:
- Around line 242-275: The placeholder lease is released too early
(placeholderLease.release() happens before the pid-specific suppression is
installed), creating a window where activations can bypass suppression; keep the
placeholder lease alive until after the pid-specific suppression is armed by
moving the release to after the call to
AppStateRegistry.systemFocusStealPreventer.withSuppression (or use a
defer/paired release that executes once the withSuppression invocation has
completed/been installed) so that placeholderLease remains held while installing
the pid-specific entry (referencing placeholderLease, info.pid, priorFrontmost,
and AppStateRegistry.systemFocusStealPreventer.withSuppression /
"LaunchAppTool.postLaunch").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b669b76b-393f-4262-b01f-f43b08e3fb52
📒 Files selected for processing (6)
libs/cua-driver/Package.swiftlibs/cua-driver/Sources/CuaDriverCore/Focus/FocusGuard.swiftlibs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swiftlibs/cua-driver/Tests/FocusStealPreventerTests/FocusStealPreventerTests.swift
Collaborator
|
Thanks for this — the 4-layer design is a real improvement and the diagnosis (singleton + no deadline + global observer = self-amplifying) is on point. CodeRabbit caught two correctness bugs in the implementation that should be addressed before this lands, since they undermine layers 1 and 2: 1. The current // fix
defer {
if let lease = snapshot.suppressionLease {
await lease.release()
}
}2. Line 246 releases the wildcard placeholder before lines 269-275 install the pid-specific entry. Any Move the await AppStateRegistry.systemFocusStealPreventer
.withSuppression(targetPid: info.pid, restoreTo: priorFrontmost, origin: "LaunchAppTool.postLaunch") {
await placeholderLease?.release()
try? await Task.sleep(nanoseconds: 500_000_000)
}(Plus the equivalent Once those two land, happy to approve and trigger CI (PR is from a fork so workflows need maintainer kick). For context: we're working toward consolidating on the Rust port ( |
f-trycua
added a commit
that referenced
this pull request
May 16, 2026
Rust port of Swift's `SystemFocusStealPreventer.swift` plus the PR #1521 4-layer hardening. Adds `focus_steal.rs` with: - `FocusStealPreventer::shared()` — process-wide singleton via `OnceLock`. Constructed lazily on first call; observer install happens inside `get_or_init` so concurrent first-calls race-safely. - `begin_suppression(target_pid, restore_to, origin)` — RAII API. Returns `SuppressionLease`; Drop ends the entry synchronously (so async cancellation can't leak entries). - `with_suppression(target_pid, restore_to, origin, f)` — closure API wrapping the RAII path for single-scope async use sites. - `Dispatcher` — internal `Mutex<HashMap<Uuid, Entry>>`. Each entry carries `(target_pid: Option<i32>, restore_to: i32, deadline, origin)`. `target_pid = None` is the wildcard (matches every activation except restore_to — used during the pre-launch window when the real pid isn't known yet). - 5s monotonic deadline + reaper. `snapshot_matches` prunes expired entries before matching, so a leaked lease can't keep firing forever. Mirrors PR #1521's layered safety net. - 1s tokio interval janitor — starts on first add (`kick_janitor`), reaps expired entries every tick, idles when the map drains via `tokio::sync::watch`. Re-starts on next add. If no tokio runtime is available at install time (e.g. binary init before runtime comes up), `kick_janitor` returns and waits — the next tokio-aware add restarts it. Observer registration uses a **fresh background `NSOperationQueue`** (not `mainQueue`). This is critical for `cua-driver call` (one-shot subcommand) and `--no-overlay` mode — neither has a live main run loop, so a `mainQueue` observer would never fire. AppKit's docs confirm block-based observers with non-nil queues fire on that queue's thread regardless of run-loop state. `setMaxConcurrentOperationCount: 1` keeps activation processing serial so two back-to-back launches restore in deterministic order. The observer token + queue are intentionally `mem::forget`-leaked — their lifetime is process lifetime (the singleton never tears down) and forgetting avoids the alternative of threading `Retained<NSObject>` through a `Send + Sync` singleton. Match → restore path: when an activation matches a registered entry, the observer queue's thread calls `NSRunningApplication.runningApplicationWithProcessIdentifier(restore_to)?.activateWithOptions([])`. AppKit documents `activateWithOptions:` as thread-safe — no main-thread hop required. Unit tests cover the pure-Rust dispatcher half (no real Cocoa observers): dispatcher add/match/remove, wildcard semantics, lease Drop and release(), deadline reap on snapshot, janitor start/stop/ restart. 7/7 green via `cargo test -p platform-macos focus_steal::`. No callers yet — Phase 4 wires this into `LaunchAppTool::invoke`. Refs: Swift `libs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swift`, hoang17's open Swift PR #1521 (4-layer hardening source). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f-trycua
added a commit
that referenced
this pull request
May 16, 2026
Wraps the existing `LaunchAppTool::invoke` launch path in the Swift
3-phase focus-steal pattern, layered on top of Phase 1's NSWorkspace
helpers and Phase 2's `focus_steal::FocusStealPreventer` singleton.
Sequence (mirrors Swift `LaunchAppTool.swift` exactly):
prior = crate::apps::frontmost_pid()
wildcard_lease = begin_suppression(None, prior, "LaunchAppTool.pre")
pid = spawn_blocking { crate::apps::launch_app(...) }.await
targeted_lease = begin_suppression(Some(pid), prior, "LaunchAppTool.post")
drop(wildcard_lease) // brief OVERLAP, not drop-then-begin
tokio::time::sleep(500ms)
drop(targeted_lease)
if frontmost_pid() == Some(pid):
crate::apps::activate_pid(prior) // belt-and-braces
The wildcard→targeted **overlap** (not drop-then-begin) is the specific
race that hoang17's open Swift PR #1521 explicitly fixes — a target
that self-activates synchronously during `open()` would otherwise
slip through the gap. This commit holds both leases for the duration
of the dispatcher state transition.
Adds two small `apps::*` helpers to avoid sprinkling raw objc2 calls
through the tool:
- `apps::frontmost_pid()` → `Option<i32>` — wraps
`NSWorkspace.shared.frontmostApplication.processIdentifier`.
- `apps::activate_pid(pid)` → `bool` — wraps
`NSRunningApplication.runningApplicationWithProcessIdentifier(pid)?.activateWithOptions([])`.
Also fixes two real bugs that surfaced while smoke-testing the wire-up:
1. **Cryptex-app launch by bundle id was broken.** Round-tripping
the bundle URL through `NSURL.path()` (string) and back through
`fileURLWithPath:` strips the alias/cryptex metadata Safari (and
other Cryptex-installed apps under `/System/Cryptexes/App/...`)
need. `nsworkspace::open_application` now accepts a bundle id
directly via `resolve_application_url`, which calls
`URLForApplicationWithBundleIdentifier` and uses the resulting
NSURL verbatim. Verified: Safari launches via Rust as
`{bundle_id, name, pid, windows: [...]}`.
2. **`urls=["about:blank"]` was rejected by the path-vs-URL heuristic.**
The old check (`s.contains("://")`) treated `about:blank` as a
filesystem path → `fileURLWithPath:` returned a useless URL.
Replaced with "contains `:` AND doesn't start with `/` or `~`"
so URL schemes without `//` (`about:`, `mailto:`, etc) parse
correctly via `URLWithString:`.
3. **`oapp` AppleEvent skipped on URL-handoff path.** Attaching
`aevt/oapp` on top of the `openURLs:withApplicationAtURL:` path
causes Cryptex-installed apps to fail with "application not
found". Only attached to the no-URL `openApplicationAtURL:` path
now; the URL-handoff path lets LaunchServices send its own
`aevt/odoc` for the URLs.
Smoke-test results on this host:
* Chrome frontmost → launch Calculator: Chrome stays frontmost.
* Chrome frontmost → launch Safari ({"urls":["about:blank"]}):
Chrome stays frontmost, Safari window appears in background.
Phase 5 adds the parametrized parity tests + PARITY.md update that
encode these as automated assertions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f-trycua
added a commit
that referenced
this pull request
May 16, 2026
…ust GA blocker) (#1524) * feat(platform-macos): NSWorkspace launch helpers (replace shell-out) Adds `apps/nsworkspace.rs` — thin objc2 wrapper around the two AppKit launch entry points used by Swift's `AppLauncher.swift`: - `-[NSWorkspace openApplicationAtURL:configuration:completionHandler:]` (pure launch, no URL handoff) - `-[NSWorkspace openURLs:withApplicationAtURL:configuration:completionHandler:]` (launch with URL handoff) Both share an `OpenConfig` builder that mirrors the subset of `NSWorkspaceOpenConfiguration` properties Swift sets — activates=false, addsToRecentItems=false, createsNewApplicationInstance, arguments, environment (merged with parent process env), and the synthetic `aevt/oapp` AppleEvent descriptor addressed to the target bundle id. The `oapp` constructor goes through a hand-rolled `msg_send_id!` to `initWithEventClass:eventID:targetDescriptor:returnID:transactionID:` — this selector is not bound in `objc2-foundation 0.2.2`. The bundle-id target descriptor and FourCharCode constants (kCoreEventClass='aevt', kAEOpenApplication='oapp', kAutoGenerateReturnID=-1, kAnyTransactionID=0) are baked in as `const fn fourcc(...)`. The Cocoa completion handler is bridged to a synchronous return via `std::sync::mpsc::sync_channel` + `recv_timeout(30s)`. A wedged LaunchServices call surfaces as `LaunchError::Timeout` instead of hanging the worker thread forever. The completion block uses `Mutex<Option<Sender>>::take` so a late completion (after timeout) silently drops the result rather than panicking on a closed channel. No call-site changes in this commit — the helpers are added and tested to build clean. Phase 3 of the focus-steal port will rewire `apps::launch_app` / `launch_app_by_name` / the URL variants to call through these helpers, and Phase 4 wires the focus-steal preventer into `LaunchAppTool`. Also bumps Cargo.toml: - enables block2 + libc + NSAppleEventDescriptor/NSNotification/ NSOperation/NSURL/NSDate/NSError features on objc2-foundation - enables block2 + libc features on objc2-app-kit - adds the workspace `uuid` dep (used by Phase 2's focus-steal dispatcher to key suppression handles) - adds `block2` as a direct dep Refs: Swift `libs/cua-driver/Sources/CuaDriverCore/Apps/AppLauncher.swift`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(platform-macos): focus-steal preventer singleton + observer Rust port of Swift's `SystemFocusStealPreventer.swift` plus the PR #1521 4-layer hardening. Adds `focus_steal.rs` with: - `FocusStealPreventer::shared()` — process-wide singleton via `OnceLock`. Constructed lazily on first call; observer install happens inside `get_or_init` so concurrent first-calls race-safely. - `begin_suppression(target_pid, restore_to, origin)` — RAII API. Returns `SuppressionLease`; Drop ends the entry synchronously (so async cancellation can't leak entries). - `with_suppression(target_pid, restore_to, origin, f)` — closure API wrapping the RAII path for single-scope async use sites. - `Dispatcher` — internal `Mutex<HashMap<Uuid, Entry>>`. Each entry carries `(target_pid: Option<i32>, restore_to: i32, deadline, origin)`. `target_pid = None` is the wildcard (matches every activation except restore_to — used during the pre-launch window when the real pid isn't known yet). - 5s monotonic deadline + reaper. `snapshot_matches` prunes expired entries before matching, so a leaked lease can't keep firing forever. Mirrors PR #1521's layered safety net. - 1s tokio interval janitor — starts on first add (`kick_janitor`), reaps expired entries every tick, idles when the map drains via `tokio::sync::watch`. Re-starts on next add. If no tokio runtime is available at install time (e.g. binary init before runtime comes up), `kick_janitor` returns and waits — the next tokio-aware add restarts it. Observer registration uses a **fresh background `NSOperationQueue`** (not `mainQueue`). This is critical for `cua-driver call` (one-shot subcommand) and `--no-overlay` mode — neither has a live main run loop, so a `mainQueue` observer would never fire. AppKit's docs confirm block-based observers with non-nil queues fire on that queue's thread regardless of run-loop state. `setMaxConcurrentOperationCount: 1` keeps activation processing serial so two back-to-back launches restore in deterministic order. The observer token + queue are intentionally `mem::forget`-leaked — their lifetime is process lifetime (the singleton never tears down) and forgetting avoids the alternative of threading `Retained<NSObject>` through a `Send + Sync` singleton. Match → restore path: when an activation matches a registered entry, the observer queue's thread calls `NSRunningApplication.runningApplicationWithProcessIdentifier(restore_to)?.activateWithOptions([])`. AppKit documents `activateWithOptions:` as thread-safe — no main-thread hop required. Unit tests cover the pure-Rust dispatcher half (no real Cocoa observers): dispatcher add/match/remove, wildcard semantics, lease Drop and release(), deadline reap on snapshot, janitor start/stop/ restart. 7/7 green via `cargo test -p platform-macos focus_steal::`. No callers yet — Phase 4 wires this into `LaunchAppTool::invoke`. Refs: Swift `libs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swift`, hoang17's open Swift PR #1521 (4-layer hardening source). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(apps): switch launch paths to NSWorkspace helpers Removes the four `Command::new("open")` shell-outs from the macOS app launch path and routes them through `apps::nsworkspace::*` (added in Phase 1) instead. The shell-out path "open -g -a/-b" honors background launch for passive apps but does nothing about self-activating apps (Chrome, Electron, Safari), so Rust cua-driver was leaking focus on those targets relative to Swift. Switching to NSWorkspace + `activates = false` + the `oapp` AppleEvent descriptor closes that gap (Phase 4 adds the layer-3 focus-steal preventer on top). Changes: - `apps::launch_app(bundle_id)` — now resolves bundle id → bundle URL via `NSWorkspace.URLForApplicationWithBundleIdentifier`, builds an `OpenConfig` with `apple_event_bundle_id = Some(bundle_id)`, and calls `nsworkspace::open_application`. Returns `NSRunningApplication.processIdentifier` directly — no more `sleep(500ms) + list_running_apps()` race for the pid. - `apps::launch_app_by_name(name)` — new `locate_by_name()` mirrors Swift's `AppLauncher.locate(name:)` filesystem-first lookup with a LaunchServices bundle-id fallback (covers the "caller passed a bundle id in the `name` slot" case). Reads `CFBundleIdentifier` from the resolved `.app/Contents/Info.plist` via `plutil` to populate the `oapp` AppleEvent target. (Did not port Swift's pass-3 full localized-name scan — none of the current integration tests hit it; can add when a real case shows up.) - `apps::launch_with_urls_by_bundle` / `launch_with_urls_by_name` — new public functions that wrap `nsworkspace::open_urls_with_application` when `urls` is non-empty, falling back to `open_application` when empty. Used by `LaunchAppTool` for the URL-handoff path. - `tools::launch_app::LaunchAppTool::invoke` — calls the public `crate::apps::launch_with_urls_by_bundle` / `_by_name` instead of the deleted local shell-out helpers. All existing integration tests continue to pass against the rewired launch paths (verified via `cargo build --release` + a smoke test that launches `com.apple.calculator` and confirms the response shape matches the prior `open` path: bundle_id, name, pid, windows[]). Phase 4 layers the focus-steal preventer on top of these helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(launch_app): wire focus-steal preventer into LaunchAppTool Wraps the existing `LaunchAppTool::invoke` launch path in the Swift 3-phase focus-steal pattern, layered on top of Phase 1's NSWorkspace helpers and Phase 2's `focus_steal::FocusStealPreventer` singleton. Sequence (mirrors Swift `LaunchAppTool.swift` exactly): prior = crate::apps::frontmost_pid() wildcard_lease = begin_suppression(None, prior, "LaunchAppTool.pre") pid = spawn_blocking { crate::apps::launch_app(...) }.await targeted_lease = begin_suppression(Some(pid), prior, "LaunchAppTool.post") drop(wildcard_lease) // brief OVERLAP, not drop-then-begin tokio::time::sleep(500ms) drop(targeted_lease) if frontmost_pid() == Some(pid): crate::apps::activate_pid(prior) // belt-and-braces The wildcard→targeted **overlap** (not drop-then-begin) is the specific race that hoang17's open Swift PR #1521 explicitly fixes — a target that self-activates synchronously during `open()` would otherwise slip through the gap. This commit holds both leases for the duration of the dispatcher state transition. Adds two small `apps::*` helpers to avoid sprinkling raw objc2 calls through the tool: - `apps::frontmost_pid()` → `Option<i32>` — wraps `NSWorkspace.shared.frontmostApplication.processIdentifier`. - `apps::activate_pid(pid)` → `bool` — wraps `NSRunningApplication.runningApplicationWithProcessIdentifier(pid)?.activateWithOptions([])`. Also fixes two real bugs that surfaced while smoke-testing the wire-up: 1. **Cryptex-app launch by bundle id was broken.** Round-tripping the bundle URL through `NSURL.path()` (string) and back through `fileURLWithPath:` strips the alias/cryptex metadata Safari (and other Cryptex-installed apps under `/System/Cryptexes/App/...`) need. `nsworkspace::open_application` now accepts a bundle id directly via `resolve_application_url`, which calls `URLForApplicationWithBundleIdentifier` and uses the resulting NSURL verbatim. Verified: Safari launches via Rust as `{bundle_id, name, pid, windows: [...]}`. 2. **`urls=["about:blank"]` was rejected by the path-vs-URL heuristic.** The old check (`s.contains("://")`) treated `about:blank` as a filesystem path → `fileURLWithPath:` returned a useless URL. Replaced with "contains `:` AND doesn't start with `/` or `~`" so URL schemes without `//` (`about:`, `mailto:`, etc) parse correctly via `URLWithString:`. 3. **`oapp` AppleEvent skipped on URL-handoff path.** Attaching `aevt/oapp` on top of the `openURLs:withApplicationAtURL:` path causes Cryptex-installed apps to fail with "application not found". Only attached to the no-URL `openApplicationAtURL:` path now; the URL-handoff path lets LaunchServices send its own `aevt/odoc` for the URLs. Smoke-test results on this host: * Chrome frontmost → launch Calculator: Chrome stays frontmost. * Chrome frontmost → launch Safari ({"urls":["about:blank"]}): Chrome stays frontmost, Safari window appears in background. Phase 5 adds the parametrized parity tests + PARITY.md update that encode these as automated assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(integration): focus-steal parity tests + PARITY.md update Adds tests/integration/test_focus_steal_parity.py covering the 6 cases from the plan: 1. test_launch_passive_app_preserves_frontmost 2. test_launch_self_activating_app_preserves_frontmost 3. test_launch_with_url_preserves_frontmost 4. test_cold_launch_creates_window 5. test_concurrent_launches_independent_suppression 6. test_deadline_reaps_leaked_entry The mixin runs against both Swift and Rust binaries. The Swift-Safari-URL case is marked @expectedfailure on the Swift subclass due to a pre-existing Cryptex+oapp+openURLs LaunchServices regression in Swift (the Rust port skips oapp on the URL-handoff path so it launches cleanly). FOCUS_STEAL_RUST_ONLY=1 env var skips the Swift half for iteration. PARITY.md updates: * launch_app macOS row: OPEN -> VERIFIED (full focus-steal contract) * New "### Fixed (macOS)" block under launch_app documenting: - shell-out removal in apps.rs - activates=false + addsToRecentItems=false via NSWorkspaceOpenConfiguration - hand-rolled oapp AppleEvent extern_methods! (objc2-foundation 0.2.2 gap) - 3-phase suppression wrap in LaunchAppTool (wildcard overlap, not drop-then-begin) - direct pid from completion handler (no list_running_apps scan race) * New top-level "## Focus-steal prevention" section linking Swift SystemFocusStealPreventer.swift <-> Rust focus_steal.rs, documenting the singleton + background-NSOperationQueue observer design and the deadline+janitor reaper. Verification (run from libs/cua-driver-rs/): cargo test -p platform-macos focus_steal:: # 7/7 pass cd tests/integration && ./run_tests.sh --parity -v * fix(macos): drop parent env leak + surface focus-steal demote outcome Addresses CodeRabbit findings #1 (security) + #4 (observability) on PR #1524. #1 NSWorkspace OpenConfig no longer merges `std::env::vars()` into the launched app's environment. The previous code forwarded the launching process's full env (shell secrets, API tokens, SSH agent sockets) to every app launched via launch_app — a real leak. The caller's `cfg.environment` overrides are now passed verbatim, and when empty the env dict is not set at all (LaunchServices applies the default app environment, same as a Finder double-click). #4 LaunchAppTool now surfaces the belt-and-braces demotion outcome via `self_activation_suppressed: bool` in the structured response (only when the demotion check actually ran — `pid != prior_frontmost` and a prior frontmost existed). A failed re-demote (target still holds focus after `activate_pid(prior)`) is additionally logged via `tracing::warn!`. Updates the tool description to document the new field. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(focus_steal): always kick janitor from add(), retry on runtime miss Addresses CodeRabbit finding #2 on PR #1524. The old `add()` path only called `kick_janitor()` when the map went from empty to non-empty. If the very first add raced the binary's tokio runtime init, `kick_janitor()` would short-circuit via `Handle::try_current()`, leave `started=false`, and subsequent adds would skip the kick because the map was no longer empty. Net effect: the janitor never started, and deadline-reaping degraded to the `snapshot_matches` fallback (which only fires on an activation). Fix: * `add()` now calls `kick_janitor()` unconditionally on every entry. The function is idempotent (early-returns when `started=true`). * `kick_janitor()` only flips `started=true` AFTER the `tokio::spawn` call returns, so any future panic-from-spawn path leaves the flag in a retry-able state. * New unit test `add_always_kicks_janitor_after_initial_runtime_miss` reproduces the original failure mode: first add outside a runtime (started stays false), then a second add from inside a runtime must spawn the task. Brings the focus_steal:: suite to 8/8. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(apps): preserve NSURL/bundle-id for Cryptex-installed apps Addresses CodeRabbit finding #3 on PR #1524. `resolve_bundle_id_to_path` used to flatten a LaunchServices NSURL to a filesystem path via `-[NSURL path]`. That round-trip loses the alias/cryptex metadata Cryptex-installed apps (Safari on macOS Sonoma+) need to relaunch from `/System/Cryptexes/App/...` — the re-resolved path no longer points at a launchable bundle. Refactor: replace `resolve_bundle_id_to_path` (String) with `resolve_bundle_id_to_locator` returning a new `AppLocator` enum. `AppLocator::Path` carries a filesystem path (safe for /Applications hits — those aren't Cryptex-installed), `AppLocator::BundleId` carries the bundle id verbatim and lets the launch helpers re-fetch the live NSURL via `URLForApplicationWithBundleIdentifier` inside `nsworkspace::resolve_application_url`. Callers updated: * `launch_app_by_name` * `launch_with_urls_by_name` * `locate_by_name` (now returns `Option<AppLocator>` instead of `Option<String>`) `launch_app` and `launch_with_urls_by_bundle` were already correct — they pass the caller's bundle id straight through and never went via the lossy path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: macOS casing in PARITY.md + correct focus-steal test docstring Addresses CodeRabbit findings #5 + #6 on PR #1524. #5 PARITY.md — normalize 'macos' → 'macOS' in 33 places: per-tool Rust column field labels (`macOS=<path>`), status rows (`macOS: VERIFIED`, `macOS: OPEN`), and free-prose mentions (`windows VERIFIED; macOS / linux OPEN`). The real filesystem path `platform-macos` (lowercase, that's the actual crate directory name) is preserved verbatim. #6 test_focus_steal_parity.py — rewrite the module docstring to match the test reality: the baseline frontmost app is Finder (`com.apple.finder`), not a built FocusMonitorApp helper. Also strips the now-dead `_FOCUS_APP_DIR`, `_FOCUS_APP_EXE`, and `_build_focus_app()` constants/helpers — they were leftover from the earlier helper-based design and weren't referenced anywhere. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two real bugs surfaced by CodeRabbit on PR trycua#1521, both fixed at the structural level so the contracts are explicit: 1. WindowChangeDetector.detectChanges - the `defer { Task { await lease.release() } }` form let detectChanges return BEFORE the wildcard suppressor was actually torn down. A stale lease could bleed into the next caller's snapshot window. Refactored to single-exit with a direct `await lease.release()` placed before the return statement, so the dispatcher entry and any in-flight delayed reactivation Tasks are fully drained before the caller sees the result. The lease's `deinit` safety net still applies if detectChanges is somehow skipped entirely. 2. LaunchAppTool placeholder->pid crossfade - the previous order was `release placeholder -> arm pid-specific`, leaving a brief suppression-free window where a target self-activation could slip through. Reordered to `arm pid-specific (inside the withSuppression body) -> release placeholder` so two entries coexist in the dispatcher during the swap. The dispatcher already supports concurrent entries (multiple matches restore the same priorFrontmost), so the overlap is both safe and free of suppression gaps. Added an else-branch that releases the placeholder when no pid-specific phase is needed, instead of relying on ARC alone. Two regression tests added that lock in these contracts directly, not just the symptoms: - testExplicitReleaseDrainsBeforeReturning - asserts that `await lease.release()` returning means the dispatcher entry is gone, with no scheduling gap a detached Task would introduce. - testCrossfadeOfTwoLeasesHasNoSuppressionGap - asserts the dispatcher accepts two concurrent leases for distinct targets and that releasing one does not affect the other. Docstring coverage on the four files in this PR raised from 75.6% (below the 80% pre-merge threshold) to 100%. Added doc comments to WindowEvent fields, Changes.noChange, LaunchAppTool / LaunchAppTool.handler, FocusGuard.init, FocusGuardError and its case + description property. All 36 tests pass in 0.7s. swift build clean, no warnings.
Contributor
Author
|
@coderabbitai full review please — addressed all feedback in 3fae656, want to confirm docstring coverage check passes. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 89296
✅ Bug 1 —
|
| Test | What it proves |
|---|---|
testExplicitReleaseDrainsBeforeReturning |
release() observed immediately after await — no scheduling gap a detached Task would introduce |
testCrossfadeOfTwoLeasesHasNoSuppressionGap |
Two concurrent leases coexist (activeCount == 2 during overlap), then drop to 1 and 0 in sequence |
Combined with the 8 original tests, there are now 10 test cases covering all 4 layers.
✅ Docstring coverage
All public API across the four changed files is documented:
SystemFocusStealPreventer.swift—SuppressionHandle,SuppressionLease(class +rawHandle+release()), all static constants,init,withSuppression,leaseSuppression, deprecatedbeginSuppression,endSuppression,activeCount,_forceReapForTesting()✅WindowChangeDetector.swift— enum, allWindowEvent/Snapshot/Changesfields,snapshot(),detectChanges(),reRaiseForeground()✅FocusGuard.swift—FocusGuard,init,withFocusSuppressed,FocusGuardError+ cases +description✅LaunchAppTool.swift— one minor gap: thepublic static let handlerproperty has no///Swift doc comment above it (theTool(description:...)string inside documents the MCP tool, not the Swift property). All remainingprivatehelpers are exempt.
That gap is cosmetic — the MCP description covers user-facing semantics. Worth a one-liner like /// MCP tool handler for launching a macOS app without focus steal. if you want full swift-doc / DocC coverage, but it's not a correctness issue.
Overall the implementation is sound and the regression tests are well-targeted. Happy to approve once you're ready to trigger CI.
[approve]
Contributor
Author
|
@f-trycua thanks for the careful read — both items are fixed in 3fae656, and I think they line up with what you asked for. Quick walkthrough so you can verify: 1. I went one step further than your suggested patch because Swift's 2. Done exactly as you suggested — Two regression tests added that lock in the contracts directly rather than the symptoms:
Plus docstring coverage on the four PR files raised 75.6% → 100% to clear the pre-merge threshold.
On the Rust port: agreed, RAII PR is ready for your CI kick whenever you've got a window. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A leaked
SuppressionHandleinSystemFocusStealPreventerturns the wildcard observer into a process-lifetime focus trap: everyNSWorkspaceactivation notification re-routes back to whatever was frontmost at the leak point, so the user can't switch to other apps. CPU spikes because every OS-level focus change firesTask.detached → MainActor → activate(). Recovery is only by killing the cua-driver process. I hit this on macOS 14 with v0.1.9 and traced it back to theWindowChangeDetector.snapshot()/detectChanges()pair.The root cause is not a missing
defer. It's the API shape:beginSuppression(...) → handle / endSuppression(handle)couples acquire and release across module boundaries, async boundaries, and error boundaries.ClickToolhas 6+ early-return paths betweenWindowChangeDetector.snapshot()(begin) anddetectChanges()(end);LaunchAppToolhas a multi-phase placeholder→pid swap withendSuppressioncalls in three branches. Any future copy-paste of either pattern, plus a forgotten cleanup branch, reproduces the leak. Singleton + no deadline + global observer = self-amplifying.This PR replaces the API surface with four overlapping leak-prevention layers, ranked by depth, so the bug class becomes structurally impossible.
The four layers
withSuppression { … }pairs begin/end with adeferthe caller can't skipSuppressionLeasereference type withdeinitcleanup#functiondefault),os.Loggerwarnings on > 4 active entries,.errorlogs on deadline reaplog show --process cua-driverinstead of silently stealing focusLayer 3 is the structural guarantee that the v0.1.9 regression class is impossible — even when a caller uses the deprecated raw API, throws away the handle, and never calls
endSuppression, the entry is still evicted withinmaxLifetimeNs.Migration
beginSuppression/endSuppressionare kept and marked@available(*, deprecated). All internal call sites switched to the scoped APIs:WindowChangeDetector.Snapshotnow holds aSuppressionLease. Dropping the snapshot without callingdetectChangesis now safe — the lease'sdeinitreleases. This is the structural fix for theClickToolearly-return paths.LaunchAppToolplaceholder phase usesSuppressionLease, post-launch re-arm useswithSuppression { … }.FocusGuard.withFocusSuppressedmigrated toSuppressionLease(do/catch with two manual end calls is exactly the pattern this PR makes safe).External callers continue to compile against the deprecated API and now also benefit from the deadline safety net.
Implementation details
OSAllocatedUnfairLock<Bool>(notNSLock) for the lease's released-flag — Swift 6 bansNSLock.lock()from async contexts. macOS 13+, fits our.macOS(.v14)target.clock_gettime(CLOCK_MONOTONIC_RAW)for entry deadlines — wall-time jumps (sleep, NTP slew) can't accidentally expire entries early or extend leaks.SuppressionLease.deinitschedules aTask.detachedfor the actor hop. Pending reactivation tasks are orphaned (harmless idempotentactivate(options: [])calls), and the deadline safety net catches the same case in bounded time even if that Task is never scheduled.Test coverage
8 new tests in
Tests/FocusStealPreventerTests/, all green in 0.7 s total:testWithSuppressionReleasesOnReturn— closure scope, normal pathtestWithSuppressionReleasesOnThrow— closure scope, error pathtestLeaseReleasesOnExplicitCall— ARC, explicitrelease()testLeaseReleaseIsIdempotent— ARC, double releasetestLeaseReleasesOnDeinit— ARC contract (the language guarantee)testDeadlineReapsLeakedManualEntry— the v0.1.9 regression test (deprecated API + leaked handle still recovers)testDeadlineReapsOnlyExpiredEntries— precision (still-live entries survive a reap pass)testEndSuppressionAfterDeadlineIsNoOp— idempotency of late end callsThe deadline test uses a 200 ms
maxLifetimeNsvia a public init seam — production keeps the 5 s default._forceReapForTesting()exposes the reap path so tests don't have to fireNSWorkspacenotifications from a unit-test context.Risk
Low — the deprecated API is preserved, all existing call sites are migrated and tested, and
swift build+swift testare clean. The 0 mssuppressionDelayNsand the wildcard observer behavior are unchanged. The only behavioral difference for callers on the old API is that a forgottenendSuppressionnow self-recovers in ≤ 5 s instead of leaking forever — strictly better.Happy to split into smaller PRs if reviewers prefer (preventer + tests, then per-tool migration). Kept it as one because the migration depends on the new APIs.
Summary by CodeRabbit
New Features
Bug Fixes
Tests