cua-driver: fix #1480 #1482 #1483 #1484 #1485 #1486 #1489 - #1490
Merged
Conversation
…ow / focus steal)
Adds WindowChangeDetector — a snapshot/detect/suppress pipeline wired
into ClickTool's AX and pixel paths that handles the case where a
background click causes a cross-app side-effect (e.g. clicking
"Browse UTM Gallery" opens a Safari window and briefly activates it).
## What changes
### WindowChangeDetector (new)
- `snapshot()` captures window set + frontmost pid, then immediately arms
a wildcard `SystemFocusStealPreventer` suppression (targetPid = 0) so
any app that self-activates during the action is squashed before the
first compositor frame.
- `detectChanges()` polls for new layer-0 windows or frontmost changes
during the action window, then ends the suppressor.
- `resultSuffix` appends `🪟 Action opened new window(s): <App> ("<title>").`
to the tool result so the background agent is aware of side-effect
windows — without surfacing the foreground-restore machinery.
- `needsRestore` triggers on new windows even when foregroundChanged is
false (suppressor may have prevented the OS-level steal before it was
observable in the poll loop).
### SystemFocusStealPreventer (extended)
- `handleActivation` now also matches wildcard entries (targetPid = 0),
firing for any pid other than `restoreTo`. Covers side-effect apps
whose pid is unknown at arm time (e.g. Safari launched by UTM).
### ClickTool (wired)
- Both AX-indexed and pixel-click paths call `snapshot()` before the
action and `detectChanges()` / `reRaiseForeground()` after.
## Test
`test_click_opens_new_window.py::TestBrowseUTMGalleryUXGuard`
- FocusMonitorApp is the simulated user foreground (ux_guard sentinel).
- UTM is launched in the background; the agent clicks "Browse UTM Gallery".
- Asserts: Safari window appears, 🪟 notice in result, FocusMonitorApp
remains frontmost, focus-loss count ≤ 1 (one unavoidable reactive tick).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
#1480 (Intel Mac support): add second matrix job on macos-13 (x86_64) in cd-swift-cua-driver.yml so Intel binaries are built and published. Separate `release` job downloads both arch artifacts and creates the GitHub release. Docs updated to say Intel is supported. #1482 (list_windows empty pid): when pid filter finds no windows, return a warning text (not isError) including the frontmost app name so callers can diagnose wrong-pid bugs quickly. #1483 (doctor subcommand): new DoctorCommand probes AX, SCK, and bundle attribution, then emits a recommendation (capture_mode + next step). --json flag for scripting. Old cleanup logic renamed to CleanupCommand (cua-driver cleanup) to free up the `doctor` name. #1484 (docs contracts): mcp-tools.mdx callout on browsers needing urls= for launch_app; callout on get_window_state sticky (pid, window_id) contract. LaunchAppTool.swift docstring updated with browser warning. #1485 (type_text_chars alias): ToolRegistry.call() remaps deprecated type_text_chars → type_text with a stderr warning. Not registered in handlers so it never appears in tools/list. #1486 (hidden-app capture test): new integration test exercises list_windows surfacing hidden windows and screenshot capturing their backing store, plus the empty-pid warning from #1482. #1489 (frontmostWindow fix): WindowEnumerator.frontmostWindow(forPid:) now uses allWindows() + SpaceMigrator space membership instead of visibleWindows() + isOnScreen, avoiding false negatives when WindowServer marks the frontmost window as occluded. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughDetects post-action window/foreground side-effects and optionally restores focus, adds a diagnostic ChangesWindow Change Detection & Tool Improvements
Diagnostic Command
Multi-Architecture Build Pipeline
Sequence Diagram(s)sequenceDiagram
participant User
participant Snapshot
participant Detect
participant Suppressor
participant Restore
participant App
User->>Snapshot: snapshot()
Snapshot->>Suppressor: arm wildcard suppression (targetPid=0)
User->>Detect: perform action (click / pixel)
Detect->>Detect: poll for new windows / foreground change
Detect->>Suppressor: end suppression (defer)
Detect-->>Restore: return Changes (newWindows, foregroundChanged)
alt Changes.needsRestore
Restore->>App: activate(original PID)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 8
🧹 Nitpick comments (1)
libs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swift (1)
138-142: 💤 Low valueConsider awaiting suppression cleanup.
The defer block ends suppression via an unstructured
Task { await ... }, which fires and forgets. While this is likely intentional to avoid blocking the caller, it meansdetectChanges()can return before the suppression is fully torn down. If timing-sensitive tests or callers depend on suppression being inactive on return, this could cause flakes.Alternative: await the cleanup synchronously
defer { if let handle = snapshot.suppressionHandle { - Task { await AppStateRegistry.systemFocusStealPreventer.endSuppression(handle) } + // Note: this would make detectChanges() block on suppression cleanup + // await AppStateRegistry.systemFocusStealPreventer.endSuppression(handle) } }However, the current fire-and-forget approach is probably fine for the use case, as the caller doesn't need to wait for the suppressor actor to finish cleanup.
🤖 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/WindowChangeDetector.swift` around lines 138 - 142, The defer currently fires-and-forgets suppression teardown using Task { await AppStateRegistry.systemFocusStealPreventer.endSuppression(handle) }, which can let detectChanges() return before suppression ends; change this to await the cleanup directly (call await AppStateRegistry.systemFocusStealPreventer.endSuppression(handle) inside the defer) so suppression is finished before returning, and update the surrounding function signature (e.g., WindowChangeDetector.detectChanges) and its callers to be async/await-compatible so the direct await is allowed.
🤖 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 @.github/workflows/cd-swift-cua-driver.yml:
- Around line 279-285: The "Generate combined checksums" step (id: checksums)
currently only finds and hashes files in ./release-assets, missing the
./release-assets-binary/**/*.tar.gz uploads; update the find invocation used in
that step so it also includes the release-assets-binary directory (e.g., run
find on both ./release-assets and ./release-assets-binary or a single find
starting at ./ that matches both paths) so the produced combined_checksums.txt
contains SHA256 entries for the binary tarballs as well as the regular assets.
- Around line 48-50: The workflow uses an invalid runner label "macos-13" which
breaks the Intel matrix leg; update the runner entry by replacing the value
macos-13 with the supported hosted label macos-15-intel (keeping the existing
arch: x86_64 and xcode: /Applications/Xcode_15.4.app lines intact) so the Intel
job matrix and artifact path succeed.
- Around line 293-294: Update the GitHub Action step that creates releases:
replace the deprecated uses declaration for softprops/action-gh-release@v1 with
the supported version softprops/action-gh-release@v3 (or
softprops/action-gh-release@v2 if you need Node 20 compatibility); locate the
step named "Create Release" and update its uses field accordingly to ensure the
release action is maintained on a supported release of the action.
In `@libs/cua-driver/Sources/CuaDriverCLI/CuaDriverCommand.swift`:
- Line 27: The review notes that adding CleanupCommand.self to the top-level
subcommands isn't enough because the implicit-call bypass list
(managementSubcommands) doesn't include "cleanup", so "cua-driver cleanup" gets
rewritten to "call cleanup" and the CleanupCommand never runs; update the
managementSubcommands array (or the code that builds it) to include the string
"cleanup" (matching the command name used by CleanupCommand) so the
implicit-call rewrite bypasses this command and CleanupCommand is invoked
directly.
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/ClickTool.swift`:
- Around line 439-442: The snapshot is taken too early via
WindowChangeDetector.snapshot(), leaving its wildcard suppression active across
early returns in the click flow; move the snapshot call to after validation (the
fromZoom check) and after coordinate resolution (the code that computes the
click point) so any early returns happen before suppression starts, or
alternatively ensure WindowChangeDetector.detectChanges() is invoked on every
early return—update the ClickTool click function to either relocate the snapshot
to after validation/coordinate resolution or add guaranteed detectChanges()
cleanup paths so the suppression is never leaked.
- Around line 241-242: The snapshot is taken before element validation/lookup
which can early-return and leave the WindowChangeDetector suppression active;
move the call to WindowChangeDetector.snapshot() to after the validation/lookup
steps (i.e., after the element lookup/early-return checks) so detectChanges()
will always be invoked on normal exit, or if you must keep the snapshot earlier,
immediately add a defer that calls WindowChangeDetector.detectChanges() to
guarantee cleanup on all return paths; reference WindowChangeDetector.snapshot()
and WindowChangeDetector.detectChanges() in your changes.
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/ListWindowsTool.swift`:
- Around line 111-125: The early-return in ListWindowsTool that builds a
text-only warning (returning CallTool.Result(content: [.text(...)])) drops the
usual structuredContent payload and breaks clients expecting it; modify this
branch so it still returns the warning text but also populates structuredContent
with an empty "windows" array and the current_space_id (or null/empty if
unknown) matching the normal list_windows shape. Locate the early-return block
that checks `if let pidFilter, windows.isEmpty` and update the CallTool.Result
to include the same structuredContent keys used elsewhere (e.g., "windows" and
"current_space_id") while preserving the warning text in content.
In `@libs/cua-driver/Tests/integration/test_hidden_app_capture.py`:
- Around line 118-148: The frontmost-hint test
(test_list_windows_pid_filter_includes_frontmost_hint) is fragile: it uses a
magic PID (fake_pid = 99999) and only asserts the PID string is present, which
can pass if the hint text is removed or the PID collides; change the PID to a
truly out-of-range value (e.g., use Int32.max) and update the assertion to check
for the actual hint phrase rather than just the numeric PID (look for the
expected phrase like "frontmost" or "frontmost app" in text_content). Also
ensure test_list_windows_pid_filter_returns_warning_on_unknown_pid still checks
for the generic "No windows found" warning and that isError remains False.
---
Nitpick comments:
In `@libs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swift`:
- Around line 138-142: The defer currently fires-and-forgets suppression
teardown using Task { await
AppStateRegistry.systemFocusStealPreventer.endSuppression(handle) }, which can
let detectChanges() return before suppression ends; change this to await the
cleanup directly (call await
AppStateRegistry.systemFocusStealPreventer.endSuppression(handle) inside the
defer) so suppression is finished before returning, and update the surrounding
function signature (e.g., WindowChangeDetector.detectChanges) and its callers to
be async/await-compatible so the direct await is allowed.
🪄 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: eb5fb004-15f7-497c-a30d-732c94feddd2
📒 Files selected for processing (14)
.github/workflows/cd-swift-cua-driver.ymldocs/content/docs/cua-driver/guide/getting-started/installation.mdxdocs/content/docs/cua-driver/reference/mcp-tools.mdxlibs/cua-driver/Sources/CuaDriverCLI/CuaDriverCommand.swiftlibs/cua-driver/Sources/CuaDriverCLI/DoctorCommand.swiftlibs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swiftlibs/cua-driver/Sources/CuaDriverCore/Windows/WindowEnumerator.swiftlibs/cua-driver/Sources/CuaDriverServer/ToolRegistry.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/ClickTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/ListWindowsTool.swiftlibs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swiftlibs/cua-driver/Tests/integration/test_click_opens_new_window.pylibs/cua-driver/Tests/integration/test_hidden_app_capture.py
Replace the macos-13 matrix job approach (which fails because swift-tools-version 6.0 requires Xcode 16, unavailable on Intel runners) with cross-compilation on macos-15: - Build arm64 natively: swift build --arch arm64 - Cross-compile x86_64: swift build --arch x86_64 (Xcode 16 supports this) - Combine with lipo -create → universal binary - Inject universal binary into .app after notarization script runs - Package per-arch tarballs (arm64, x86_64) + universal tarball, all containing the same universal binary for download-by-arch compatibility Verified locally: both slices compile and lipo produces a valid universal binary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/cd-swift-cua-driver.yml (1)
341-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
softprops/action-gh-release@v1is still on the deprecated major.
@v1corresponds to the 0.1.x Docker-only releases from 2019-2020 and is flagged byactionlintas too old to run on current GitHub Actions runners. Pin to@v3(or@v2if Node 20 is required):- uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v3🤖 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 @.github/workflows/cd-swift-cua-driver.yml at line 341, The workflow uses the deprecated action reference softprops/action-gh-release@v1; update that step to use the current maintained major (softprops/action-gh-release@v3) or `@v2` if Node 20 compatibility is required, ensuring any inputs/outputs or invocation syntax are adjusted to match the newer action version (change the uses value in the workflow step that references softprops/action-gh-release@v1).
🤖 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 @.github/workflows/cd-swift-cua-driver.yml:
- Around line 211-235: The current step both mislabels architecture-specific
tarballs by copying the same universal tarball into ARM64_TAR/X86_TAR and uses a
brittle glob (EXISTING=$(ls cua-driver-${VERSION}-darwin-*.tar.gz …)) that can
match pkg tarballs; change the job to only produce/rename the single universal
tarball (use UNIVERSAL_TAR and the convenience symlinks) and stop creating
byte-identical X86_TAR/ARM64_TAR copies, and make the EXISTING glob explicit to
avoid pkg matches (e.g., search for cua-driver-${VERSION}-darwin-arm64.tar.gz or
filter out *.pkg.tar.gz via grep -v) so mv only ever renames a genuine .tar.gz
app archive; also update scripts/install.sh to fetch the universal asset name
instead of arch-specific names.
- Around line 180-204: The workflow injects a universal binary into the .app
after build-release-notarized.sh completes which means tarballs and
notarization/stapling are based on the original single-arch binary and are
invalid; instead produce or place the universal Mach-O before running
build-release-notarized.sh so the script signs, notarizes, and packages the
final universal bundle. Concretely: stop the post-script cp into APP_BINARY and
the subsequent partial re-codesign (remove the cp and the codesign --force
--sign "$CERT_APPLICATION_NAME" ... "$APP_BINARY" || true steps), and modify the
job to either (a) build the universal binary earlier and copy it to the expected
.build/cua-driver-universal path prior to invoking build-release-notarized.sh,
or (b) pass the prebuilt universal binary into build-release-notarized.sh
(update the script to accept an input binary path and use it in its
lipo/sign/package steps); also remove the "|| true" that silently swallows
codesign failures (referencing APP_BINARY, build-release-notarized.sh, and the
codesign invocation/CERT_APPLICATION_NAME).
---
Duplicate comments:
In @.github/workflows/cd-swift-cua-driver.yml:
- Line 341: The workflow uses the deprecated action reference
softprops/action-gh-release@v1; update that step to use the current maintained
major (softprops/action-gh-release@v3) or `@v2` if Node 20 compatibility is
required, ensuring any inputs/outputs or invocation syntax are adjusted to match
the newer action version (change the uses value in the workflow step that
references softprops/action-gh-release@v1).
🪄 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: d866fd0d-35c1-487a-8e56-35fbe113679f
📒 Files selected for processing (1)
.github/workflows/cd-swift-cua-driver.yml
This was referenced May 16, 2026
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
Fixes seven cua-driver issues in one batch:
frontmostWindowfix: useallWindows()+ SpaceMigrator space membership instead ofvisibleWindows()+isOnScreen, avoiding false negatives when WindowServer marks the frontmost window as occluded.list_windowssurfacing hidden windows,screenshotcapturing their backing store, and the empty-pid warning from cua-driver: capture should error (not silently fall back to frontmost) whenapp=is provided but unresolvable #1482.type_text_chars→type_textfor one minor version #1485type_text_charsdeprecated alias:ToolRegistry.call()remaps the old name →type_textwith a stderr warning. Not registered inhandlersso it never shows intools/list.mcp-tools.mdxfor browsers needingurls=inlaunch_app; callout forget_window_statesticky(pid, window_id)contract.LaunchAppTool.swiftdocstring updated with browser warning.doctorsubcommand that probes TCC/SCK/AX and recommends capture mode #1483doctorsubcommand: probes AX, SCK, and bundle attribution, then emits a recommendation (capture_mode+ next step).--jsonfor scripting. Old cleanup logic renamed toCleanupCommand(cua-driver cleanup).app=is provided but unresolvable #1482list_windowsempty-pid warning: when pid filter finds no windows, returns warning text including the frontmost app name rather than a silent empty list.cd-swift-cua-driver.ymlnow runs a matrix build acrossmacos-15(arm64) andmacos-13(x86_64). A separatereleasejob waits for both and uploads both arch tarballs. Docs updated to say Intel is supported.Test plan
swift build -c releasepasses (confirmed locally)cua-driver doctorruns and prints probes + recommendationcua-driver doctor --jsonemits valid JSONcua-driver list_windows --pid 99999returns warning text with frontmost app hinttype_text_charsdispatches totype_textwith stderr warning, does not appear intools/listtest_hidden_app_capturepasses on a machine with Accessibility + Screen Recording grantedcd-swift-cua-driver.ymlmatrix builds both arm64 and x86_64 on tag push🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
doctor) and acleanupcommand.Bug Fixes
list_windowsreturns informative warnings when PID filters match no results.Documentation
Tests