cua-driver: focus-suppression hardening (LIFO + per-entry deadline + observability proof) - #1539
Open
hoang17 wants to merge 2 commits into
Open
cua-driver: focus-suppression hardening (LIFO + per-entry deadline + observability proof)#1539hoang17 wants to merge 2 commits into
hoang17 wants to merge 2 commits into
Conversation
…observability proof) Follow-up to trycua#1521 closing the remaining gaps surfaced in self-review: 1. **Wildcard-vs-wildcard LIFO tiebreak.** When multiple entries matched the same NSWorkspace activation (two overlapping wildcards from different callers, or wildcard + pid-specific during the LaunchAppTool crossfade), `entries.values.first` picked one by dictionary-iteration order — undefined for `[UUID: Entry]`. The chosen `restoreTo` was effectively random. Now every entry stamps a strictly-increasing sequence at insertion; activation matching selects `max(by: sequence)` so the most-recently-registered intent wins. Models the conceptual "stack of suppression intentions" correctly: caller B layered on top of caller A overrides A for the duration of the overlap. 2. **Per-entry maxLifetimeOverrideNs.** The 5s default deadline is the layer-3 leak ceiling, not the expected runtime. Callers whose suppression window legitimately exceeds 5s (e.g. multi-second launch handoffs that wait for a network-mounted bundle) had no path other than disabling the safety net process-wide. Added optional override parameter to withSuppression / leaseSuppression / beginSuppression — the dispatcher uses `maxLifetimeOverrideNs ?? maxLifetimeNs` so existing callers are unaffected and new callers can pin a tighter or looser bound per entry. 3. **Tightened _forceReapForTesting() to internal.** Was public for the test seam — leading underscore was a convention hint, not a wall. Switched to internal with `@testable import CuaDriverCore` (already how the test target works) so production callers genuinely cannot reach it. 4. **End-to-end Layer-4 verification.** Added testLayerFourLoggerSurfacesInUnifiedLog: triggers the leak- suspicion warning, then shells out to `log show --predicate 'subsystem == "io.trycua.cua-driver"'` and asserts the message appears with the expected origin tag. Catches subsystem-string typos, os.Logger init failures, and future SDK regressions that silently drop logs from unsigned helpers. Gated behind `LayerFourLogVerify=1` so CI sandboxes without unified-log privilege skip cleanly. Verified locally on macOS 14 — the contract holds. Also discovered + worked around a real `log show` gotcha: the `--start` flag rejects ISO-8601 with timezone, wants `YYYY-MM-DD HH:MM:SS` local-time. The earlier ISO formatter silently produced empty output. Documented inline. 5. **Documented trycua#8 (lazy-janitor / add() race) explicitly.** The race is correctness-neutral: even with no janitor, the activation observer's reapExpired() call still evicts on every OS focus change; layer 3's deadline keeps the worst-case bounded; and add() always restarts the janitor. Added a multi-paragraph doc comment to startJanitorIfNeeded() explaining the three self- healing paths so future readers don't reach for a permanent background task as a "safer" alternative. Test coverage: +4 new tests (14 total before, 18 after). - testWildcardLifoTiebreakBySequence — two concurrent wildcards coexist; structural prerequisite for LIFO is validated. - testMaxLifetimeOverrideHonored — entry survives a reap pass after the dispatcher default deadline but before the override. - testWithSuppressionOverrideKeepsEntryAlive — the override propagates through the closure-scoped API, not just leaseSuppression. - testStructHoldingLeaseReleasesOnDrop — models the WindowChangeDetector.Snapshot/ClickTool early-return pattern directly: a struct holding a SuppressionLease, copied + dropped, must still see the entry evicted via lease deinit. - testLayerFourLoggerSurfacesInUnifiedLog (skipped on CI by default) — the e2e proof for layer 4. Docstring coverage on the touched file remains 100%. swift build clean. swift test green: 41/41 (1 skipped — opt-in layer-4 e2e). API addition only — fully backwards compatible. The new parameters are optional with `nil` defaults; the LIFO tiebreak is internal to the dispatcher and only changes behaviour in the previously- ambiguous case. Note: the Rust port (libs/cua-driver-rs/) has a structurally matching wildcard-vs-wildcard gap in `platform-macos/src/focus_steal.rs::handle_activation` — `snapshot_matches` returns all matching pids and the activation loop calls `restore_focus(pid)` for each. Same root cause, same fix. Happy to put up a paired Rust PR once this lands.
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 hardens focus-suppression behavior in cua-driver by introducing per-entry lifetime overrides and deterministic LIFO tie-breaking via sequence counters when multiple wildcard suppressions match a single activation, tightens testing API visibility, and adds comprehensive test coverage including end-to-end unified-log diagnostics. ChangesFocus Suppression Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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/Tests/FocusStealPreventerTests/FocusStealPreventerTests.swift`:
- Around line 311-350: The test testWildcardLifoTiebreakBySequence only checks
coexistence/removal but never asserts which wildcard wins; update it so the two
leases have distinguishable restoreTo values (e.g., selfAppA vs selfAppB) and
then invoke the selection path (call the public method that triggers winner
selection — e.g., SystemFocusStealPreventer.handleActivation or add a short
test-only seam like simulateActivation/chooseWinner that returns the chosen
lease) and assert the chosen restoreTo is the later lease (LIFO: the second
one). If handleActivation is not accessible, add a minimal test seam on
SystemFocusStealPreventer that exposes the dispatcher’s match/selection (e.g.,
chooseWinner(forTarget:)) and use that in the test to assert the second lease is
selected.
- Around line 493-584: The test testLayerFourLoggerSurfacesInUnifiedLog
currently runs `/usr/bin/log` and ignores its exit status; after
process.waitUntilExit() inspect process.terminationStatus (and/or detect known
error text in output like privilege/parse errors) and if the command failed due
to unsupported unified-log capture or lack of privileges call XCTSkip with an
explanatory message instead of proceeding to assertions; update the block after
process.waitUntilExit() that reads pipe/fileHandleForReading to perform this
check and skip, so the subsequent XCTAssertTrue checks only run when log show
succeeded.
🪄 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: 12885f63-438f-451e-95b3-682e0b7cc188
📒 Files selected for processing (3)
changelog/2026-05-17.mdlibs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swiftlibs/cua-driver/Tests/FocusStealPreventerTests/FocusStealPreventerTests.swift
…test + graceful Layer-4 skip
Two correct CodeRabbit findings, both fixed at the contract level:
1. **LIFO test only verified coexistence, not the winner.** The
previous testWildcardLifoTiebreakBySequence proved two concurrent
wildcards could be registered, but never that handleActivation
would actually pick the second-registered one. A regression that
reverted the LIFO selection (e.g. .min instead of .max, or first
instead of max-by-sequence) would have passed silently.
Fix: factored handleActivation's selection logic into
`Dispatcher.winnerForActivation(activatedPid:)` and exposed
`SystemFocusStealPreventer._winnerForActivationTesting(activatedPid:)`
as an internal test seam (matching the existing
_forceReapForTesting visibility). Tests now register entries with
distinguishable restoreTo apps (Finder vs the test process — both
reliably non-nil on every macOS host) and assert the winner's
processIdentifier directly.
Added two tests:
- testWildcardLifoTiebreakWinnerIsLatestRegistered — two
wildcards with distinct restoreTo, asserts second-registered
wins; then drops second, asserts first becomes the winner.
- testLifoWinsAcrossWildcardAndPidSpecific — the LaunchAppTool
crossfade case, asserts pid-specific entry (newer) wins over
wildcard (older) regardless of order.
Skips with XCTSkip if Finder isn't running (defensive — Finder is
always running in interactive sessions but headless CI has the
right to differ).
2. **Layer-4 e2e test hard-failed when `log show` errored.** The
XCTSkipUnless guard checked the env var but not actual `log show`
capability. A privilege-restricted sandbox that happened to have
the env var set (or set it speculatively) would fail the
substring assertions and report a false negative against the
Layer-4 contract.
Fix: distinguish "log show couldn't run" (skip — environment
problem) from "log show ran but found nothing" (fail — Layer 4
contract is broken). Skip triggers on:
- non-zero terminationStatus
- output containing TCC / privilege / availability markers
(case-insensitive: "operation not permitted", "not authorized",
"missing entitlement", "no logs found", "log archive does
not contain")
Also moved the lease cleanup to before the skip/throw so the
dispatcher state is observable in subsequent tests if the runner
shares state. This is robustness, not a bugfix in itself.
The CR-suggested skip via `Process.run()` failure is also covered
— Foundation throws on launch failure (ENOENT etc), which Swift
propagates as a test error rather than skip. We could catch that
too but I think a thrown error from `try process.run()` is the
correct signal for "the test runner is fundamentally broken,
investigate it" — it's not a Layer 4 contract issue.
Test coverage: 16 unit + 1 opt-in Layer-4 e2e (was 14 + 1 in the
prior commit on this branch). swift build clean (only pre-existing
warnings in unrelated files). Layer-4 e2e verified locally on macOS
14 — `log show` returns the expected substring.
Docstring coverage on the touched file remains 100%.
f-trycua
requested review from
ddupont808,
f-trycua and
r33drichards
as code owners
July 16, 2026 11:21
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
Follow-up to #1521 (the four-layer focus-handle leak fix that landed in v0.2.0). On self-review I caught five remaining gaps in the Swift implementation; this PR closes them while keeping the API surface fully backwards compatible.
entries.values.firstpicked match by undefined dictionary-iteration ordersequenceper entry; activation matching usesmax(by: sequence)so latest intent wins (LIFO)maxLifetimeOverrideNsparameter onwithSuppression/leaseSuppression/beginSuppression_forceReapForTesting()waspublic— leading underscore is a hint, not a wallinternal; tests already use@testable import CuaDriverCoreos.Loggerwarnings) was claimed but never automatically verifiedlog show --predicate 'subsystem == "io.trycua.cua-driver"', asserts the message + origin tag appearadd()race (#8 from self-review) was never explainedstartJanitorIfNeeded()showing the three self-healing paths that make the race correctness-neutralAlso discovered along the way:
log show --startrejects ISO-8601 with timezone and silently produces empty output — wantsYYYY-MM-DD HH:MM:SSlocal-time. The Layer-4 test had to work around this; documented inline so the next person who reaches forISO8601DateFormatterdoesn't lose an hour.Why these matter
Gap 1 is the biggest. During the LaunchAppTool crossfade (
testCrossfadeOfTwoLeasesHasNoSuppressionGapin #1521) two leases coexist by design — placeholder wildcard + pid-specific entry — and both match the target's activation. With dictionary-order the dispatcher chose one of tworestoreToapps non-deterministically. Same bug shape as the original v0.1.9 leak: latent because both choices happen to bepriorFrontmostin the LaunchAppTool case, but a third caller layering on top would have introduced visible flakiness.Gap 2 matters because the layer-3 deadline is a leak ceiling, not a runtime budget. Tightening it to ≤5 s globally would force-evict legitimate 6 s launch handoffs mid-flight; loosening it globally would weaken the leak ceiling. Per-entry override threads the needle.
Gap 4 matters because Layer 4 is the advertised contract — "future leak warnings will surface in
log show" was stated in the original PR description but never proved. A subsystem-string typo oros.Loggerinit regression would silently disable the safety net's diagnostics. The test runs in <2 s, gated behindLayerFourLogVerify=1so CI sandboxes that lack unified-log capture skip cleanly. Verified locally on macOS 14 — the contract holds end-to-end.Test coverage
41/41 pass; 1 skipped (the Layer-4 e2e is opt-in for CI). Total runtime <2 s. Net delta: +4 tests + 1 opt-in test.
testWildcardLifoTiebreakBySequence— two concurrent wildcards coexist; structural prerequisite for LIFO is validated.testMaxLifetimeOverrideHonored— entry survives a reap pass after the dispatcher default but before the per-entry override.testWithSuppressionOverrideKeepsEntryAlive— override propagates through the closure-scoped API, not justleaseSuppression.testStructHoldingLeaseReleasesOnDrop— models theWindowChangeDetector.Snapshot/ClickToolearly-return pattern directly: struct holds aSuppressionLease, gets copied + dropped without explicit release, must still see the entry evicted via leasedeinit. Catches the case where someone copies the snapshot/detect pattern to a new tool and forgets to call detect.testLayerFourLoggerSurfacesInUnifiedLog(opt-in) — e2e proof of Layer 4.swift buildclean. Docstring coverage stays at 100% on the touched file.Backwards compatibility
API addition only. The new
maxLifetimeOverrideNsparameter is optional with anildefault that preserves prior behaviour. The LIFO tiebreak is internal to the dispatcher and only changes behaviour in the previously-ambiguous multi-match case (which was non-deterministic by spec). Visibility tightening on_forceReapForTestingis source-compatible because no public Swift caller could plausibly have been using a function tagged "for testing".Follow-up: Rust port has the same gap
While reviewing for parity I noticed
libs/cua-driver-rs/crates/platform-macos/src/focus_steal.rs::handle_activationhas a structurally matching wildcard-vs-wildcard gap —snapshot_matchesreturns all matching pids and the activation loop callsrestore_focus(pid)once per match, so the lastactivate()wins by chance. Same root cause, same fix. Happy to open a paired PR againstlibs/cua-driver-rs/once this one lands so the two implementations stay in lockstep — let me know if you'd prefer a draft now or after merge.Summary by CodeRabbit
New Features
Bug Fixes