Conversation
Size Report
Startup median (7 runs, lower is better):
|
![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 across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Reviewed at 5709698. The restored Not blocking: Since nothing reads a 19 checks are green with 0 failing, and the runner Swift paths this diff touches are covered by the xcuitest build lanes that passed. I did not run a live check against a presented SafariViewService or PassbookUIService session; the analysis above shows no Before this can merge, either the |
|
The code looks right at e8fcd97. The .noApp preparation now matches the pre-#2899 chain for every member, and no .noApp response body reads the prepared app or systemSurface, so the SafariViewService/PassbookUIService route this fixes cannot be observed from a live run anyway. All 19 checks pass, including the xcuitest iOS and macOS build lanes that compile the changed runner Swift, and I see no conflicts. I did not run the Simulator XCTests or any mutation pass; the claim that the test fails without the fix rests on reading the merge-base arm (64854af:466-469), not on a run. I did not measure the latency of the two XCUIApplication.state probes on status, uptime, and shutdown; they match the pre-#2899 cost by inspection. Nothing here blocks merge. Not blocking: could the new 12-line comment on the .noApp arm in apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift around line 467 be one sentence pointing to traits(for:) as the member list instead of a prose copy of the .noApp rows that will drift as members are added? |
thymikee
added
ready-for-human
`prepareActiveCommandContext` consulted `presentedSystemSurfaceHost()` before the lifecycle branch, so a `screenshot`, `status`, or `uptime` taken while a registered system surface was foregrounded was served that host in place with its provenance (#2438). The launch policy axis added in #2899 answered `.noApp` by resolving a target from the request, dropping both the presented route and the standing cached target the merge-base fell through to. Neither answer brings an app forward, so only the target and its disclosure differ; on a foldable the first is the panel that is lit. Restored with a regression test over a forced-presented host, and a second half that names a bundle the session never bound — the only request shape that tells the standing target apart from one resolved from the request.
…on the table The combined `case .existingApp, .mayLaunch:` arm asked every read whether it could skip preflight. That bypass requires a coordinate-only `.tap` with a cached foreground target, so it was unreachable for the `.existingApp` half and read as if a read could skip preflight. Split so the check lives only under `.mayLaunch`; those commands never satisfied it before either, so behavior is unchanged.
…type The consumer derived it inline from `convertsRecordedFailure` and the owner set, so the rule was a predicate inside one function that no table could pin. `Command` now answers it once — still derived rather than declared as a fifth fact the retired axis was — and `executeOnMainPrepared` reads that. The truth table is unchanged for every command.
`notRunningRefusal` is `#if os(iOS)`, so off iOS an `.existingApp` command reaches activation exactly as it did before this axis existed and no refusal can occur there. `existingApp` promised a refusal with no platform stated, and `selectorResolution` repeated it, which is how #2890's own `querySelector` fix reads as a macOS/tvOS guarantee it never was. Enlarging the refusal to those platforms would change behavior this issue never touched, so the scope is stated instead.
…anything The column is true only for `get`; `accept` and `dismiss` are false because they mutate, so the comment naming the shared fact as "the alert request changes nothing" contradicted two of the four rows it then reads. Phrased to match the Swift side, which already names `alert get` as the one alert action that may be replayed.
Whether the prepared path drops a remembered text-entry tap was argued from the retired read-only fact rather than asserted. The table now carries that column for every command, compared against the merge-base predicate plus a literal copy of its read-only set, so neither replay eligibility nor witness clearing can move with the classification under test. The commands `executeOnMain` answers before `executeOnMainPrepared` runs are compared nowhere: the old predicate never evaluated for them and neither does the derived one. `snapshot` stays in scope because it reaches that body, and `appState` postdates the merge-base and owes it no equality. Adding the `querySelector` bundle id to the witness-owner set — the change the review asked for — turns that row red.
…gistered host The override for `systemSurfaceHostState` only forced the hosts a test named, so every other registered host still answered from live simulator state: a registry walk could not be pinned end to end, and a new registry entry made the test's answer depend on the machine again. The override is now total while set — members report foreground, non-members do not. Production is unaffected: the seam is compiled only under the unit-test flag. This is hardening, not a mutation-proofed fix. Mutating the override back to the member-only form stays green on a simulator, because the two registered hosts already report notRunning there — which is the very reason the seam exists. Also write down what the `.noApp` presented arm actually controls. Tracing the first parent of c7b79ee: no `.noApp` command read the prepared target or the disclosed surface even pre-split (`status`/`uptime`/`recordStop`/`terminate`/ `targetReset`/`shutdown` answer before reading a target, iOS `screenshot` re-resolves its capture display (#2728), macOS `screenshot` resolves the named app, and `snapshot` — the only provenance consumer — is `.existingApp` and keeps its route in `prepareActivatedTarget`). The arm restores the prepared subject and disclosure, which is what the merge-base chain gave these commands; the test says so instead of claiming a payload the runner never stamped.
e8fcd97 to
511dbb4
Compare
|
Rebased onto Conflict resolution: #2911 moved target identity into Non-blocking nit applied in Verification on this head: |
|
The rebase at 511dbb4 resolves the conflict with #2911 correctly. The I did not compile the Swift unit tests at this head. The main-actor call to Smoke Tests is still running. It runs this |
thymikee
added
the
ready-for-human
This branch has not been deployed
1 participant
Summary
Follow-up to #2899. Carries the accepted findings from its bot rounds.
.noAppdropped thepresentedSystemSurfaceHost()route the merge-base consulted first, resolving a target from the request instead of the standing cache. The arm serves the presented host in place again, else the standing target (iOS: ASWebAuthenticationSession cannot be automated — binding the session tocom.apple.SafariViewServicedestroys the view, and the tree served afterwards is stale #2438). Both facts are preparation-level and no.noAppresponse body reads either; the arm says so in one sentence pointing atCommand.traitsas the member list.case .existingApp, .mayLaunch:— the coordinate-only tap bypass was dead under.existingApp. No behavior change.querycolumn comment; documented the.existingApprefusal as iOS-enforced.systemSurfaceHostStatetest seam answers every registered host while set.Rejected: deriving witness clearing from
convertsRecordedFailurenewly clears noquerySelectorwitness — the merge-base predicate cleared it too; the table test pins clearing per command.Validation
Head
511dbb4; rebased ontoorigin/mainf4fccd02(#2911). Conflict shape: #2911 moved target identity into@MainActor RunnerMainOwnedStateand annotated the test overrides, so the seam sits besideblockingSystemModalPresenceOverrideForTestingin the same#if AGENT_DEVICE_RUNNER_UNIT_TESTSblock, is read only throughsystemSurfaceHostState, and the test is@MainActorwritingmainOwned.app/.bundleId— no parallel channel, nocurrentAppleft underapple/; the probe's two callers are both@MainActor.check:affected --run,check:xctest-selection,check:packaged-runner-swift,build:xcuitest:ios(clean DerivedData, so the isolation scan covered every file) andbuild:xcuitest:macospass. Simulator XCTests onrnav-repro: 26 tests across the touched dispatch/models/alert-dispatch files, 0 failures; reverting the.noApparm to main's fails 4 assertions, restoring returns green.Pre-#2899 evidence (
64854af:466-469): the chain probed the presented host first and no body consumed it, so the route is unobservable live; an earlier head proved the sibling.existingApppath live onrnav-repro(is exists→APP_NOT_RUNNING).