Repository navigation
fix(apple-runner): serve a macOS read of a background app without raising it - #3339
Conversation
thymikee
added
the
ready-for-human
|
Size Report
Startup median (7 runs, lower is better):
|
dd12a41 to
8ffc692
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…sing it On a desktop, the XCTest foreground repair is not a no-op for the user: a read that names a session app sitting behind other windows took their frontmost app away for a command that only asked to look (#3254). The activation axis already routes by `launchPolicy`, so the fix goes on the existing `.existingApp` arm of `prepareActiveCommandContext`: on macOS, a read whose app answers `.runningBackground` is served in place through `resolveAppWithoutActivation`, and no activation fact is booked for it. Scoping is exact: - a stopped app keeps the launch the activating route performs for it today: `state` answers `.notRunning` and the arm declines, so `open`-then-read still works and no macOS refusal is invented (`notRunningRefusal` stays iOS-only); - interactions keep activating: the macOS XCTest path drives events through the foreground window, and a background click would land on whatever window is on top; - a window-level screenshot keeps its raise (measured: a backgrounded `screenshotRoot(app:).screenshot()` returns the occluding window's content inside the session window's rect), but a `--fullscreen` capture of a running app no longer raises — `XCUIScreen.main` answers from any foreground, and the 0.5 s settle waits belong to the raise and are skipped with it. A stopped app keeps launching either way. The two conditions are functions the call sites read (`macReadMayBeServedInBackground`, `macAppCaptureNeedsRaise`) so the host lane pins every state they branch on. Four host-lane tests: the background read served without a raise or a fact across its three shapes, the interaction on the same app still activating, a stopped read still launching, and the raise-condition table. The canary is the arm's removal: the read test goes red with the app left foreground and a `bundle_changed` fact booked. A live run on the default backend confirms it: with Finder frontmost, `snapshot` and `get` answered from the background app, Finder stayed frontmost, and runner.log booked zero activation facts; the click after them activated with priorState=3 as it must. `help macos` states the new read behavior. Also records at the `.presentedSurface` arm why its macOS route is unreachable from the host (`alert` answers through the helper; `action-button` is refused by its owner fact before dispatch), so no second activation axis is proposed against it. Partial #3254
83304bd to
2df8924
Compare
thymikee
changed the title
Turns two accidents into rules, both asked for in coordinator review of #3339: - `macReadMayBeServedInBackground` splits its state probe from the `state` call so the host lane can pin every state the answer branches on. The pin records why only `.runningBackground` is served in place: on the macOS SDK the suspended case does not exist (the split is pinned by RunnerTests+ApplicationStateRawValueTests), and every other state keeps the activating route so an app that cannot promise an answerable tree pays the repair rather than trading a focus steal for an empty read — the same set `targetNeedsActivation` uses on macOS, so read and capture cannot disagree. - The disclosure decision is made with the reviewer rather than by omission, and pinned: a background-served read books no activation fact and no substitute marker. The existing channels cannot say it honestly — the activation fact records only a repair performed, and the observation payload belongs to the iOS observe-only contract, which refuses exactly this situation. A background tree is live, not degraded (measured rect drift is time, not foreground state), and interactions still activate, so no action path consumes a background tree. `XCTAssertNil(prepared.observation)` makes the absence owned: a future disclosure must edit the assertion on purpose. No behavior change; both live-state tests and the new table pass on the host lane.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
The raise table claimed to pin every state `macAppCaptureNeedsRaise` branches on but omitted `.unknown`, while the read table in the same test pinned it — the state worth pinning on one side could not be unpinnable on the other. The rows convert the silent drift path into a red row: `.unknown` raises for a window-level capture, where the activation is the standing route's attempt to settle an app whose state the SDK cannot report, and stays false for `--fullscreen`, whose pixels answer from any foreground. Expectations verified against the implemented condition, not pasted. No behavior change. Partial #3254
|
This PR is ready for review at 74f3899. The Swift runner change and the help string look correct, and all 19 checks pass, including the macOS runner lanes. No conflicts. Not blocking, and you can take or leave these: (1) in the macOS background arm at https://github.com/callstack/agent-device/blob/74f3899/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift#L552, a read served in place should use the same target identity a bound read would use, so please call The live run is reported by the author only, and I did not see the The fixes for the three resolved inline threads are in at this commit: the removed APP_NOT_RUNNING sentence in cli-help.ts (#3339 (comment)), the region-grab claim now limited to window-level captures (#3339 (comment)), and the LifecycleTests raise rows that now match |
A read served in place resolved through `resolveAppWithoutActivation`, which returns the cached handle when the cache names the app — and a cached handle can record a pid an outside relaunch replaced. The activating route never answers from that residue because it runs `refreshCachedTargetIfProcessChanged` before its `targetNeedsActivation` read; the new arm bypassed the shared identity rule, so between an outside relaunch and the next read, a served read could address the dead process while claiming to observe the live one. Reachability: an `open`-bound session whose app is quit and reopened by the user leaves exactly this residue, and macOS `notRunningRefusal` never intercepts it. The fix runs the shared rule on the arm before resolution, so identity is settled once and both routes read the same live pid. Pinned by a host-lane test that plants the residue (live app, bogus cached pid) and asserts the served preparation refreshes without activating; canary: deleting the refresh call fails the test with the bogus pid still in place. Also drops the probe wrapper the arm no longer needs: a served read is one `state` query, and the activating route's second read cannot reuse the first because the refresh between them may have replaced the process the first answered for. The arm's and the predicate's comments are cut to the invariant they protect, per AGENTS.md; the decision record lives in #3338. Partial #3254
…ents AGENTS.md keeps decision and review narration out of implementation comments: the screenshot raise site, `macAppCaptureNeedsRaise`, and the `.existingApp` policy case each keep the one constraint they encode (region-grab pixels, the state rule, the served-read scope) and point at #3338 for the measured record. No behavior change; host-lane evidence for the pinned conditions is unchanged. Partial #3254
|
All three items acted on; two commits at head 1. Stale pid on the served arm: real defect, confirmed from the caching path, fixed. 2. Review history out of the comments: done in 3. Second state query: confirmed, halved where it can be, kept where it can't. Live-run The two Unverified, staying honest: Validation at head |
|
This PR is ready. The code at e0bd784 is correct, and it fixes what the earlier review at 74f3899 left open. The served macOS arm now refreshes the cached target, and the screenshot and help text no longer overclaim. All 19 checks pass at e0bd784, including the macOS runner lanes that exercise the changed served arm. There are no conflicts. Nothing else needs to happen before a human merges. Not blocking, and you can take or leave it: in I did not run the runner unit tests locally, and the canary failure on removing the refresh is as you reported it, though the code read supports it. The attached live On the other threads, the cubic-dev-ai threads are fixed at this commit and can be resolved. The APP_NOT_RUNNING claim was removed from the CLI help. The screenshot comment now limits the region-grab claim to window-level captures. The raise rows in the lifecycle tests match |
1 participant
Summary
On the default XCTest backend, a macOS read (
snapshot,get,find, an interaction's leading reads) of a session app sitting behind other windows raised it first, taking the user's frontmost app away for a command that only asked to look (#3254). The fix rides the existinglaunchPolicyaxis: the.existingApparm ofprepareActiveCommandContextrefreshes cached target identity through the shared rule, then serves a.runningBackgroundapp in place viaresolveAppWithoutActivation, booking no activation fact. Scope kept exact: stopped apps keep launching (notRunningRefusalstays iOS-only), interactions keep activating, and window-levelscreenshotkeeps its measured-load-bearing raise while--fullscreenof a running app no longer raises or pays its 0.5 s settle. Only.runningBackgroundis served in place; every other state keeps the activating route, matchingtargetNeedsActivation's macOS set.Background reads answer as ordinary reads — decided, not omitted;
XCTAssertNil(prepared.observation)plus the per-state table own the silence. Decision record: #3338.Validation
Head
e0bd78428:check:affected --runpasses; host lane 295/295, 0 failures (selection: host lane reaches 295). Canary: removing the arm fails the read test with abundle_changedfact; removing the identity refresh leaves the planted dead pid. Live run (runner.log attached in review reply): reads answered with Finder frontmost and zeroACTIVATE_FACTlines; the tap after them bookedpriorState=3; fullscreen raised nothing. CI owns Coverage/replay-macos.Still activates (XCTest)
Interactions;
open/activate/home/recordStart;mouseClick; window-levelscreenshotof a non-foreground app; anyscreenshotof a stopped app.Dispositions
Proposals 1/2/4 already exist on native/accessibility/
SCContentFilterpaths; proposal 3's overload is argued against in #3338.Partial #3254