feat(platform-macos): port focus-steal prevention from Swift (macOS-Rust GA blocker) - #1524
Merged
Conversation
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>
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>
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>
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>
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
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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 implements macOS focus-steal prevention for the Rust CUA driver by introducing a suppression dispatcher with NSWorkspace observer, refactoring app enumeration to direct NSWorkspace APIs, wiring suppression around LaunchAppTool, and adding comprehensive parity tests against the Swift implementation. ChangesmacOS Focus-Steal Prevention for launch_app
Sequence Diagram(s)sequenceDiagram
participant User as User (Finder)
participant LaunchAppTool
participant FocusStealPreventer
participant NSWorkspace
participant TargetApp
User->>LaunchAppTool: invoke launch_app(Safari)
Note over LaunchAppTool: Capture prior_pid (Finder)
LaunchAppTool->>FocusStealPreventer: begin_suppression(target_pid=None, restore_to=Finder)
LaunchAppTool->>NSWorkspace: openApplication(Safari)
NSWorkspace->>TargetApp: Launch Safari
TargetApp->>NSWorkspace: Activate self (reflex)
NSWorkspace->>FocusStealPreventer: DidActivateApplication(Safari)
FocusStealPreventer->>FocusStealPreventer: Match wildcard, restore Finder
FocusStealPreventer->>User: Activate Finder
Note over LaunchAppTool: Upgrade to targeted suppression(target_pid=Safari)
LaunchAppTool->>LaunchAppTool: Sleep 500ms
LaunchAppTool->>FocusStealPreventer: Drop suppression leases
LaunchAppTool->>User: Return (Finder remains frontmost)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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: 6
🤖 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-rs/crates/platform-macos/src/apps/mod.rs`:
- Around line 207-218: resolve_bundle_id_to_path currently converts a
LaunchServices NSURL to a filesystem path, which breaks relaunch semantics;
instead preserve and return a bundle-id/NSURL-style reference so name-based
launchers (locate_by_name, launch_app_by_name, launch_with_urls_by_name and
launch_app) receive the original bundle identifier/URL metadata rather than a
path. Change resolve_bundle_id_to_path (and the other affected branch at
~230-255) to return the NSURL or an unmodified bundle-id/URL string (and update
callers to accept that type) rather than calling url.path()/to_string(),
ensuring the Cryptex/alias metadata is retained for relaunch.
In `@libs/cua-driver-rs/crates/platform-macos/src/apps/nsworkspace.rs`:
- Around line 208-225: The code currently merges the entire current process
environment (std::env::vars()) with cfg.environment and forwards it via
config.setEnvironment, which can leak secrets; change the logic in this block to
only pass the explicit overrides (cfg.environment) — or an explicit allowlist if
you must inherit some keys — by constructing keys/vals/NSString entries from
cfg.environment alone (use cfg.environment.iter() to build keys, vals and
key_refs) and then call config.setEnvironment with that NSDictionary, avoiding
use of merged or std::env::vars().
In `@libs/cua-driver-rs/crates/platform-macos/src/focus_steal.rs`:
- Around line 233-259: The add() path can miss starting the janitor if the first
kick happens off-runtime and the map is left non-empty; change the logic to base
kicking on the janitor's started state rather than the map emptiness — i.e.,
after inserting into self.entries in add(), always check whether the janitor is
running (use the same flag/condition kick_janitor() uses, e.g. the internal
started/handle state) and call kick_janitor() whenever the janitor hasn't
successfully started, or simply always call kick_janitor() (it is idempotent)
instead of only calling when the map transitioned from empty; update add(),
kick_janitor(), and the code that sets started to ensure a failed spawn doesn't
leave started=true so subsequent adds will attempt to start again, and keep the
janitor_active.send(true) signaling as-is.
In `@libs/cua-driver-rs/crates/platform-macos/src/tools/launch_app.rs`:
- Around line 229-250: The computed self_activation_suppressed boolean is being
discarded at the end (let _ = self_activation_suppressed;) which hides failures
to restore activation state; instead propagate or return this result from the
surrounding function (the launch handling routine in launch_app.rs) so callers
can observe real success/failure. Update the code that currently drops
wildcard_lease and ends with let _ = self_activation_suppressed; to use the
self_activation_suppressed value in the function's return or status summary (or
convert it into an Err when demotion failed), and ensure the caller receives
that status; reference the self_activation_suppressed variable and the demotion
check that calls crate::apps::frontmost_pid() so you locate and wire the correct
branch into the function's returned result.
In `@libs/cua-driver-rs/PARITY.md`:
- Around line 420-428: Normalize Apple platform casing to "macOS" throughout the
parity doc: replace instances like
`macos=`crates/platform-macos/src/tools/launch_app.rs``, the `macos: VERIFIED`
entry under Status, the `macos=`
tests/`crates/platform-macos/src/focus_steal.rs` references under Tests, and the
occurrences around lines 1296-1297 so every mention uses "macOS" (e.g.,
macOS=`crates/platform-macos/...`) for consistent naming and searchability.
In `@libs/cua-driver-rs/tests/integration/test_focus_steal_parity.py`:
- Around line 31-34: The module docstring and test implementation disagree about
the baseline frontmost app: the docstring mentions FocusMonitorApp but the tests
launch Finder (com.apple.finder); update the module docstring to state that
Finder (com.apple.finder) is used as the baseline frontmost app or change the
test setup to launch the FocusMonitorApp fixture instead; locate and edit the
module-level docstring and the test setup/fixture references (look for the
symbols FocusMonitorApp and com.apple.finder in this test module and the suite
setup) so both the documentation and the code consistently refer to the same
baseline app.
🪄 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: 2d9512ba-b5ef-4ace-8f08-d02b029e2263
⛔ Files ignored due to path filters (1)
libs/cua-driver-rs/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
libs/cua-driver-rs/PARITY.mdlibs/cua-driver-rs/crates/platform-macos/Cargo.tomllibs/cua-driver-rs/crates/platform-macos/src/apps.rslibs/cua-driver-rs/crates/platform-macos/src/apps/mod.rslibs/cua-driver-rs/crates/platform-macos/src/apps/nsworkspace.rslibs/cua-driver-rs/crates/platform-macos/src/focus_steal.rslibs/cua-driver-rs/crates/platform-macos/src/lib.rslibs/cua-driver-rs/crates/platform-macos/src/tools/launch_app.rslibs/cua-driver-rs/tests/integration/test_focus_steal_parity.py
💤 Files with no reviewable changes (1)
- libs/cua-driver-rs/crates/platform-macos/src/apps.rs
Comment on lines
+207
to
+218
| pub(crate) fn resolve_bundle_id_to_path(bundle_id: &str) -> Option<String> { | ||
| use objc2_app_kit::NSWorkspace; | ||
| use objc2_foundation::NSString; | ||
| unsafe { | ||
| let ws = NSWorkspace::sharedWorkspace(); | ||
| let ns = NSString::from_str(bundle_id); | ||
| let url = ws.URLForApplicationWithBundleIdentifier(&ns)?; | ||
| // -[NSURL path] gives us the absolute filesystem path; convert | ||
| // to UTF-8. | ||
| let path = url.path()?; | ||
| Some(path.to_string()) | ||
| } |
Contributor
There was a problem hiding this comment.
Keep bundle-id fallbacks as bundle ids.
When locate_by_name falls back for inputs like com.apple.Safari, it converts the LaunchServices result into url.path() and hands a path string to the name-based launch helpers. The comments above launch_app already call out that this round-trip drops Cryptex/alias metadata and breaks Safari-style relaunches, so this branch reintroduces the bug for launch_app_by_name / launch_with_urls_by_name. Preserve a bundle-id/NSURL-style reference here instead of converting it to a filesystem path.
Also applies to: 230-255
🤖 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-rs/crates/platform-macos/src/apps/mod.rs` around lines 207 -
218, resolve_bundle_id_to_path currently converts a LaunchServices NSURL to a
filesystem path, which breaks relaunch semantics; instead preserve and return a
bundle-id/NSURL-style reference so name-based launchers (locate_by_name,
launch_app_by_name, launch_with_urls_by_name and launch_app) receive the
original bundle identifier/URL metadata rather than a path. Change
resolve_bundle_id_to_path (and the other affected branch at ~230-255) to return
the NSURL or an unmodified bundle-id/URL string (and update callers to accept
that type) rather than calling url.path()/to_string(), ensuring the
Cryptex/alias metadata is retained for relaunch.
Comment on lines
+208
to
+225
| if !cfg.environment.is_empty() { | ||
| // Merge the launching process's env with caller overrides — same | ||
| // contract as Swift `AppLauncher.launch`. | ||
| let mut merged: Vec<(String, String)> = std::env::vars().collect(); | ||
| for (k, v) in &cfg.environment { | ||
| if let Some(slot) = merged.iter_mut().find(|(mk, _)| mk == k) { | ||
| slot.1 = v.clone(); | ||
| } else { | ||
| merged.push((k.clone(), v.clone())); | ||
| } | ||
| } | ||
| let keys: Vec<Retained<NSString>> = | ||
| merged.iter().map(|(k, _)| NSString::from_str(k)).collect(); | ||
| let vals: Vec<Retained<NSString>> = | ||
| merged.iter().map(|(_, v)| NSString::from_str(v)).collect(); | ||
| let key_refs: Vec<&NSString> = keys.iter().map(|s| s.as_ref()).collect(); | ||
| let dict = NSDictionary::from_vec(&key_refs, vals); | ||
| config.setEnvironment(&dict); |
Contributor
There was a problem hiding this comment.
Avoid forwarding the full driver environment.
As soon as cfg.environment is non-empty, this copies every variable from std::env::vars() into the launched app. That can leak secrets from the driver process to arbitrary GUI apps and makes launches depend on ambient process state instead of only the explicit overrides the caller supplied. Prefer passing just the requested overrides, or a narrowly defined allowlist if parity requires inheriting a subset.
🤖 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-rs/crates/platform-macos/src/apps/nsworkspace.rs` around
lines 208 - 225, The code currently merges the entire current process
environment (std::env::vars()) with cfg.environment and forwards it via
config.setEnvironment, which can leak secrets; change the logic in this block to
only pass the explicit overrides (cfg.environment) — or an explicit allowlist if
you must inherit some keys — by constructing keys/vals/NSString entries from
cfg.environment alone (use cfg.environment.iter() to build keys, vals and
key_refs) and then call config.setEnvironment with that NSDictionary, avoiding
use of merged or std::env::vars().
Comment on lines
+233
to
+259
| fn add( | ||
| self: &Arc<Self>, | ||
| target_pid: Option<i32>, | ||
| restore_to: i32, | ||
| origin: &'static str, | ||
| ) -> SuppressionHandle { | ||
| let id = Uuid::new_v4(); | ||
| let entry = Entry { | ||
| target_pid, | ||
| restore_to, | ||
| deadline: Instant::now() + ENTRY_DEADLINE, | ||
| origin, | ||
| }; | ||
| let needs_start = { | ||
| let mut guard = self.entries.lock().unwrap(); | ||
| let was_empty = guard.is_empty(); | ||
| guard.insert(id, entry); | ||
| was_empty | ||
| }; | ||
| if needs_start { | ||
| self.kick_janitor(); | ||
| } | ||
| // Signal the janitor that there's work to do (it will start a | ||
| // fresh tokio interval on the next tick). | ||
| let _ = self.janitor_active.send(true); | ||
| SuppressionHandle(id) | ||
| } |
Contributor
There was a problem hiding this comment.
Janitor may never start if timing is unlucky.
The janitor is only kicked when the map transitions from empty to non-empty (line 252-254). If the first add() fails to spawn the janitor because no tokio runtime exists yet (line 326-327 returns early without setting started=true), and a second add() is called while the map is still non-empty, kick_janitor() will be skipped. Later, when a tokio runtime does become available, subsequent add() calls will continue to skip kick_janitor() because the map is non-empty, and the janitor will never spawn until all entries expire and the map drains.
The comment at lines 323-325 claims "the next add from a tokio-aware caller will start it," but this is only true if the map is empty at that next add.
🔧 Proposed fix: always attempt to start janitor when not yet started
Replace the needs_start logic to check whether the janitor has successfully started, rather than whether the map was empty:
- let needs_start = {
- let mut guard = self.entries.lock().unwrap();
- let was_empty = guard.is_empty();
- guard.insert(id, entry);
- was_empty
- };
- if needs_start {
- self.kick_janitor();
- }
+ {
+ let mut guard = self.entries.lock().unwrap();
+ guard.insert(id, entry);
+ }
+ // Always try to kick the janitor if it hasn't started yet.
+ // kick_janitor is idempotent if already started.
+ if !*self.janitor_started.lock().unwrap() {
+ self.kick_janitor();
+ }Alternatively, always call kick_janitor() and rely on its idempotency (lines 319-322 already guard against redundant spawns).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn add( | |
| self: &Arc<Self>, | |
| target_pid: Option<i32>, | |
| restore_to: i32, | |
| origin: &'static str, | |
| ) -> SuppressionHandle { | |
| let id = Uuid::new_v4(); | |
| let entry = Entry { | |
| target_pid, | |
| restore_to, | |
| deadline: Instant::now() + ENTRY_DEADLINE, | |
| origin, | |
| }; | |
| let needs_start = { | |
| let mut guard = self.entries.lock().unwrap(); | |
| let was_empty = guard.is_empty(); | |
| guard.insert(id, entry); | |
| was_empty | |
| }; | |
| if needs_start { | |
| self.kick_janitor(); | |
| } | |
| // Signal the janitor that there's work to do (it will start a | |
| // fresh tokio interval on the next tick). | |
| let _ = self.janitor_active.send(true); | |
| SuppressionHandle(id) | |
| } | |
| fn add( | |
| self: &Arc<Self>, | |
| target_pid: Option<i32>, | |
| restore_to: i32, | |
| origin: &'static str, | |
| ) -> SuppressionHandle { | |
| let id = Uuid::new_v4(); | |
| let entry = Entry { | |
| target_pid, | |
| restore_to, | |
| deadline: Instant::now() + ENTRY_DEADLINE, | |
| origin, | |
| }; | |
| { | |
| let mut guard = self.entries.lock().unwrap(); | |
| guard.insert(id, entry); | |
| } | |
| // Always try to kick the janitor if it hasn't started yet. | |
| // kick_janitor is idempotent if already started. | |
| if !*self.janitor_started.lock().unwrap() { | |
| self.kick_janitor(); | |
| } | |
| // Signal the janitor that there's work to do (it will start a | |
| // fresh tokio interval on the next tick). | |
| let _ = self.janitor_active.send(true); | |
| SuppressionHandle(id) | |
| } |
🤖 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-rs/crates/platform-macos/src/focus_steal.rs` around lines 233
- 259, The add() path can miss starting the janitor if the first kick happens
off-runtime and the map is left non-empty; change the logic to base kicking on
the janitor's started state rather than the map emptiness — i.e., after
inserting into self.entries in add(), always check whether the janitor is
running (use the same flag/condition kick_janitor() uses, e.g. the internal
started/handle state) and call kick_janitor() whenever the janitor hasn't
successfully started, or simply always call kick_janitor() (it is idempotent)
instead of only calling when the map transitioned from empty; update add(),
kick_janitor(), and the code that sets started to ensure a failed spawn doesn't
leave started=true so subsequent adds will attempt to start again, and keep the
janitor_active.send(true) signaling as-is.
Comment on lines
+229
to
+250
| // Re-check; the structured warning depends on | ||
| // whether the demote actually took. | ||
| if crate::apps::frontmost_pid() == Some(*pid) { | ||
| self_activation_suppressed = false; | ||
| } else { | ||
| self_activation_suppressed = true; | ||
| } | ||
| } else { | ||
| self_activation_suppressed = true; | ||
| } | ||
| } else { | ||
| // pid == prior frontmost (re-launch of an already- | ||
| // frontmost app). Just drop the wildcard. | ||
| drop(wildcard_lease); | ||
| } | ||
| } | ||
| } else { | ||
| // Launch failed; just drop the lease. | ||
| drop(wildcard_lease); | ||
| } | ||
| let _ = self_activation_suppressed; | ||
|
|
Contributor
There was a problem hiding this comment.
Suppression failure state is computed but dropped, so failed restore is silently reported as success.
Line 249 discards self_activation_suppressed, and the success summary still claims background behavior even when demotion fails. That hides real focus-steal regressions from callers.
💡 Proposed fix
- let mut self_activation_suppressed = false;
+ let mut suppression_warning: Option<&'static str> = None;
@@
- if crate::apps::frontmost_pid() == Some(*pid) {
- self_activation_suppressed = false;
- } else {
- self_activation_suppressed = true;
- }
+ if crate::apps::frontmost_pid() == Some(*pid) {
+ suppression_warning = Some(
+ "Launched app remained frontmost after suppression; background invariant was not fully restored."
+ );
+ }
@@
- } else {
- self_activation_suppressed = true;
- }
+ }
@@
- let _ = self_activation_suppressed;
@@
- let mut summary = format!("Launched {app_name} (pid {pid}) in background.{port_summary}");
+ let mut summary = format!("Launched {app_name} (pid {pid}) in background.{port_summary}");
+ if let Some(warn) = suppression_warning {
+ summary.push_str(&format!("\n\n⚠️ {warn}"));
+ }🤖 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-rs/crates/platform-macos/src/tools/launch_app.rs` around
lines 229 - 250, The computed self_activation_suppressed boolean is being
discarded at the end (let _ = self_activation_suppressed;) which hides failures
to restore activation state; instead propagate or return this result from the
surrounding function (the launch handling routine in launch_app.rs) so callers
can observe real success/failure. Update the code that currently drops
wildcard_lease and ends with let _ = self_activation_suppressed; to use the
self_activation_suppressed value in the function's return or status summary (or
convert it into an Err when demotion failed), and ensure the caller receives
that status; reference the self_activation_suppressed variable and the demotion
check that calls crate::apps::frontmost_pid() so you locate and wire the correct
branch into the function's returned result.
Comment on lines
+420
to
+428
| - macos=`crates/platform-macos/src/tools/launch_app.rs` (full focus-steal contract) | ||
| - linux=`crates/platform-linux/src/tools/impl_.rs` (TBD audit) | ||
| - Status: | ||
| - windows: VERIFIED | ||
| - macos: OPEN | ||
| - macos: VERIFIED (full focus-steal contract — see [Focus-steal prevention](#focus-steal-prevention)) | ||
| - linux: OPEN | ||
| - Test: `crates/platform-windows/examples/launch_app_parity.rs` | ||
| - Tests: | ||
| - windows=`crates/platform-windows/examples/launch_app_parity.rs` | ||
| - macos=`tests/integration/test_focus_steal_parity.py` + `crates/platform-macos/src/focus_steal.rs` (Rust unit tests) |
Contributor
There was a problem hiding this comment.
Use “macOS” casing consistently in the parity doc.
The updated sections use macos in several places; standard Apple/platform naming here should be macOS for consistency and searchability.
✏️ Suggested doc patch
- - macos=`crates/platform-macos/src/tools/launch_app.rs` (full focus-steal contract)
+ - macOS=`crates/platform-macos/src/tools/launch_app.rs` (full focus-steal contract)
@@
- - macos: VERIFIED (full focus-steal contract — see [Focus-steal prevention](`#focus-steal-prevention`))
+ - macOS: VERIFIED (full focus-steal contract — see [Focus-steal prevention](`#focus-steal-prevention`))
@@
- - macos=`tests/integration/test_focus_steal_parity.py` + `crates/platform-macos/src/focus_steal.rs` (Rust unit tests)
+ - macOS=`tests/integration/test_focus_steal_parity.py` + `crates/platform-macos/src/focus_steal.rs` (Rust unit tests)
@@
-- Status: macos VERIFIED. windows/linux N/A (no equivalent OS focus-steal
+- Status: macOS VERIFIED. windows/linux N/A (no equivalent OS focus-stealAlso applies to: 1296-1297
🧰 Tools
🪛 LanguageTool
[uncategorized] ~420-~420: The operating system from Apple is written “macOS”.
Context: ...src/tools/impl_.rs (LaunchAppTool) - macos=crates/platform-macos/src/tools/launch...
(MAC_OS)
[uncategorized] ~424-~424: The operating system from Apple is written “macOS”.
Context: ...it) - Status: - windows: VERIFIED - macos: VERIFIED (full focus-steal contract — ...
(MAC_OS)
[uncategorized] ~428-~428: The operating system from Apple is written “macOS”.
Context: ...dows/examples/launch_app_parity.rs - macos=tests/integration/test_focus_steal_par...
(MAC_OS)
🤖 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-rs/PARITY.md` around lines 420 - 428, Normalize Apple
platform casing to "macOS" throughout the parity doc: replace instances like
`macos=`crates/platform-macos/src/tools/launch_app.rs``, the `macos: VERIFIED`
entry under Status, the `macos=`
tests/`crates/platform-macos/src/focus_steal.rs` references under Tests, and the
occurrences around lines 1296-1297 so every mention uses "macOS" (e.g.,
macOS=`crates/platform-macos/...`) for consistent naming and searchability.
Comment on lines
+31
to
+34
| The frontmost-baseline app is FocusMonitorApp (shared fixture under | ||
| `libs/cua-driver/Tests/FocusMonitorApp/`). It's launched as the | ||
| baseline frontmost between tests so we have a stable, predictable | ||
| "prior" pid. |
Contributor
There was a problem hiding this comment.
Test module docs and implementation disagree on the baseline frontmost app.
The docstring says FocusMonitorApp is the baseline, but the suite actually uses Finder (com.apple.finder). Please align the docs with code (or switch the code to the fixture) to avoid confusion.
📝 Suggested doc correction
-The frontmost-baseline app is FocusMonitorApp (shared fixture under
-`libs/cua-driver/Tests/FocusMonitorApp/`). It's launched as the
-baseline frontmost between tests so we have a stable, predictable
-"prior" pid.
+The frontmost-baseline app is Finder (`com.apple.finder`), which is
+always present in a logged-in macOS session. Tests activate it before
+each case so we have a stable, predictable "prior" app.Also applies to: 159-163
🤖 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-rs/tests/integration/test_focus_steal_parity.py` around lines
31 - 34, The module docstring and test implementation disagree about the
baseline frontmost app: the docstring mentions FocusMonitorApp but the tests
launch Finder (com.apple.finder); update the module docstring to state that
Finder (com.apple.finder) is used as the baseline frontmost app or change the
test setup to launch the FocusMonitorApp fixture instead; locate and edit the
module-level docstring and the test setup/fixture references (look for the
symbols FocusMonitorApp and com.apple.finder in this test module and the suite
setup) so both the documentation and the code consistently refer to the same
baseline app.
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>
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>
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>
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>
Collaborator
Author
|
Follow-up commits addressing all 6 CodeRabbit findings — pushed to @coderabbitai please re-review the four new commits below. Commits
Verification
|
5 tasks
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 92658
✅
|
This was referenced May 16, 2026
Merged
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
1 participant
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
Ports macOS launch focus-steal prevention from Swift cua-driver to Rust cua-driver-rs. This is the last code blocker to flipping cua-driver-rs macOS from BETA → GA and deprecating the Swift binary.
Background
Rust port was 110/110 green on the documented parity contract, but a deeper read found one real GA gap: app launch.
NSWorkspace.openApplication(at:configuration:completionHandler:)withNSWorkspaceOpenConfiguration.activates = false+ anoappAppleEvent descriptor, wrapped inSystemFocusStealPreventer(an NSWorkspace observer that re-activates the prior frontmost app if the launched app self-activates inapplicationDidFinishLaunching)./usr/bin/open -g -a <name>everywhere.open -gcovers passive apps but does nothing about self-activations — Chrome/Electron/Safari launched via cua-driver-rs would steal the user's focus.The existing parity tests didn't catch this because they checked outcomes (did the app launch?) not focus-steal characteristics.
What this PR does
Full Swift parity, including the 4-layer hardening from open Swift PR #1521 (closure scope, RAII lease, 5s monotonic deadline, 1s janitor). Out of scope:
FocusGuard.withFocusSuppressedandWindowChangeDetector.snapshot— those wrap other tools (click, AX actions) that don't exist in Rust yet anyway; they'll port with their respective tools.Commits
1209e135feat(platform-macos): NSWorkspace launch helpers (replace shell-out) — newcrates/platform-macos/src/apps/nsworkspace.rswithopen_application/open_urls_with_applicationwrapping objc2-app-kit'sNSWorkspace.openApplication(at:configuration:completionHandler:). Hand-rolledextern_methods!block bindsNSAppleEventDescriptor.init(eventClass:eventID:targetDescriptor:returnID:transactionID:)(not exposed byobjc2-foundation 0.2.2). Completion handler bridged viatokio::sync::oneshot+ 30s timeout.9f740972feat(platform-macos): focus-steal preventer singleton + observer — newcrates/platform-macos/src/focus_steal.rs. Singleton viaOnceLock<Arc<FocusStealPreventer>>. OneNSWorkspaceDidActivateApplicationNotificationobserver registered on a fresh backgroundNSOperationQueue(notmainQueue) so the block fires regardless of whether the binary's run loop is running on the main thread — critical forCalland--no-overlaymodes. Dispatcher with closure + RAII APIs; 5s monotonic deadline + reaper-on-fire + 1s tokio interval janitor coordinated viawatch::Sender(start-on-first-add, stop-on-last-remove). 7 unit tests: dispatcher add/match/remove, wildcard, lease drop/release, deadline reap, janitor lifecycle.319a2c10refactor(apps): switch launch paths to NSWorkspace helpers —apps::launch_app,launch_app_by_name,launch_with_urls_*rewired to callnsworkspace::*. AllCommand::new(\"open\")paths removed; no fallback.4cc62faafeat(launch_app): wire focus-steal preventer into LaunchAppTool — 3-phase wrap inLaunchAppTool::invoke:The wildcard→targeted overlap (not drop-then-begin) is the specific race that hoang17's Swift PR cua-driver: kill focus-handle leak class via 4-layer scope binding #1521 explicitly fixes — preserved here.
714dda06test(integration): focus-steal parity tests + PARITY.md update —tests/integration/test_focus_steal_parity.pycovers the 6 cases (passive, self-activating, URL-handoff, cold-launch window, back-to-back, deadline reaper). Runs against both binaries. Swift's URL-handoff case is@expectedFailuredue to a known Cryptex+oapp+openURLs:regression in Swift that the Rust port sidesteps. PARITY.mdlaunch_appmacOS row flips to VERIFIED; new top-level## Focus-steal preventionsection documents the singleton/observer design.Verification
Focus-steal unit tests passing:
dispatcher_add_match_removewildcard_matches_all_but_restore_tolease_release_removes_entrylease_drop_removes_entrymultiple_entries_match_independentlydeadline_reaps_leaked_entryjanitor_starts_stops_restartsIntegration tests (require both binaries built + a real macOS host):
Risk notes
Callmode (the parity test harness uses this mode). Verified at design time; tests exercise it end-to-end.oappAppleEvent constructor is hand-rolled becauseobjc2-foundation 0.2.2doesn't expose it. If objc2 is bumped to 0.5+ later, theextern_methods!block can be removed.Follow-ups (not in this PR)
FocusGuard.withFocusSuppressedwhen Rust gains the click/AX action path.WindowChangeDetector.snapshot/detectChangeswhen Rust gains the snapshot/detect pattern.libs/cua-driver-rs/scripts/install.shand announce Swift deprecation.Summary by CodeRabbit
New Features
Tests
Documentation