Conversation
Size Report
Startup median (7 runs, lower is better):
|
thymikee
left a comment
There was a problem hiding this comment.
Thermo-nuclear structural pass (implementation quality only). The core move is genuinely good code-judo: one readOnly bool consumed as five decisions is deleted outright — no re-wrapping (ReadOnly.conditional, isLifecycle, and the five predicates are all gone) — the traits table is a compile-exhaustive traits(for:) switch, querySelector is the only behavior change, and no file crossed 1k.
Not blocking, but two of the new axes reintroduce the same defect class the PR set out to kill (a value read as many decisions, or a value that lies on some rows). I'd resolve those two before merge; the rest are cheap.
Cheaper notes (not inline):
- The table pays the full cartesian product by hand (~9 rows x 5 literals, no default, no named group). The sibling daemon table
packages/platform-apple/src/runner/runner-command-traits.tsusesDEFAULT_TRAITS+ named presets per group. Independent decisions don't require independent literals — named groups would shrink this and (bonus) remove the incentive that produced the dead 5th field, since "not applicable" becomes absence rather than a literal nobody reads. readOnlyis deleted from the runner everywhere, butcontracts/fixtures/alert-command-traits.jsonstill keys its columnreadOnly. Fine if that column now purely describes the daemon TS trait — worth a one-line note (or rename) so the cross-language golden table doesn't imply the runner still has the concept.- Test shapes:
traits(_:_:_:_:_:)passes four unlabeledBools positionally (a swapped pair matching a swapped row passes silently — labels cost nothing), and the expectations reuse.hostedByFocusedSurface, so they can't fail if the iOS/macOS mapping flips.CommandType.traits(for:)has one caller (Command.traits) — one accessor + a switch is the same design with one fewer API. - ADR-0014 still lists "derive the policy from runner read-only traits: rejected" and calls the TS
readOnlytrait a single decision;readOnlyno longer exists as a runner trait, and the TS bool is consumed as 4+ decisions (busy-resend, skip-invalidation, error-classification, readiness-preflight) — the very shape this PR removes on the runner side. A sentence so the ADR doesn't reject a deleted concept / understate the sibling layer.
| /// would cancel exactly what the command is about. Only iOS proved that skip before this axis | ||
| /// existed, so macOS and tvOS keep the route they had: `mayLaunch`, which still serves a presented | ||
| /// surface first and activates only when nothing is presented. | ||
| static var hostedByFocusedSurface: CommandLaunchPolicy { |
There was a problem hiding this comment.
hostedByFocusedSurface is a #if os(iOS) return .noApp #else return .mayLaunch computed var, not a policy value. The axis's own doc says it is "the only fact that decides whether a stopped app is started" — but on macOS/tvOS this resolves to .mayLaunch, i.e. a bare launch, the opposite of what a row named "hosted by focused surface" reads as (exactly the .alert / actionButton rows above it). Three consequences: (a) the table's answer is unreadable at the row — grep .noApp misses the two commands that matter on the platforms where they run; (b) the #else arm is speculative (actionButton can't run off iOS at all); (c) the new completeness test asserts against this same helper, so it can never catch a wrong platform mapping. Keep the enum a plain sum type — declare those rows .noApp unconditionally, or add a real presentedSurface case and write the #if carve-out once at the declaration site with dispatch handling both — then assert concrete cases in the table test.
There was a problem hiding this comment.
Resolved in 95dc700. The computed value is deleted; CommandLaunchPolicy is a plain four-case sum type again.
Row, not computation. hostedByFocusedSurface is gone (grep -rn hostedByFocusedSurface at this head: no hits anywhere in the repo). The enum gained a real presentedSurface case (RunnerTests+Models.swift:45-60) and the rows name it: alert resolves to .presentedSurfaceQuery / .presentedSurfaceMutation at RunnerTests+Models.swift:227-230, actionButton to .presentedSurfaceMutation at :235-236. Grepping either the case or the group by name finds the rows on every platform.
The platform exception is written once, where the policy is read. prepareActiveCommandContext now switches over launchPolicy (RunnerTests+CommandDispatch.swift:431); the single #if os(iOS) sits inside the .presentedSurface case (:436-446) with its reason — off iOS nothing is registered to serve in place, so the command keeps the activation route it had on origin/main. That switch also answers your == .noApp / == .existingApp point: .mayLaunch stopped being a fall-through, and adding a case is a compile error now.
One ordering detail I re-derived while moving the call: the request-dependent bypass (shouldSkipAppActivationPreflight, which reads the cached target's .state) is now reached only from the .existingApp, .mayLaunch arm (:447-450). It used to sit ahead of the policy test, where your launchPolicy == .noApp || … short-circuit kept alert / actionButton out of it — so a hosted-surface command still never probes the cached app, and that position is now a consequence of the switch rather than of || precedence.
Table test. Rows name a concrete case, and expectations no longer pass through anything production uses: the groups are fileprivate to RunnerTests+Models.swift, so the test cannot see them, and it builds a private ExpectedTraits and compares each fact to its own literal (UnitTests/RunnerTests+ModelsTests.swift:41, comparisons at :67-77). Two mutations, each run then reverted:
.selectorResolution→launchPolicy: .mayLaunch⇒XCTAssertEqual failed: ("mayLaunch") is not equal to ("existingApp") - {"command":"querySelector"} launchPolicy- swap the two assignments inside
CommandTraits.init⇒… ("false") is not equal to ("true") - {"command":"tap"} isInteractionand the same forretryOnSessionLoss. This one stays green while the expectation is constructed with the production initializer, which is exactly your point and is why the comparison is per-fact now.
Platform mapping proof. A traits table cannot assert what a platform does, so the mapping is a live test: testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound (UnitTests/RunnerTests+CommandDispatchTests.swift:95) drives actionButton and alert get through prepareActiveCommandContext against a terminated com.apple.Preferences and asserts the app stayed .notRunning, no target was bound, no activation fact was recorded, and the next snapshot for that bundle id is refused with APP_NOT_RUNNING. It sits inside this file's existing #if os(iOS) region (opened at :68), so the host lane never compiles it — confirmed from the built bundles: the macOS .xctest has 0 occurrences of the selector, the iOS one has it. I listed it in the iOS PR lane in 7295ee9, so check:xctest-selection now reports the PR list at 110 and the mapping is checked per pull request instead of only in the nightly.
| let convertsRecordedFailure: Bool | ||
| /// Whether serving this command makes a remembered text-entry tap stale. Consumed by the prepared | ||
| /// command path, so a command that answers before that path has a declaration and no consumer. | ||
| let clearsRememberedTextEntryTap: Bool |
There was a problem hiding this comment.
clearsRememberedTextEntryTap looks like a fifth independent axis but is either dead or derived — never both. Its only consumer is executeOnMainPrepared, and snapshot / status / uptime / activate / terminate / targetReset / shutdown / recordStart / recordStop all return before that path; eight of them still declare : true — a side effect that can never fire (the PR body itself concedes "a declaration and no consumer"). For every command that DOES reach the consumer it equals convertsRecordedFailure && command != .tap && command != .type. So it buys nothing at the rows where it is read and is wrong where it isn't. Can we drop the field and gate it at executeOnMainPrepared's own switch, or derive it from convertsRecordedFailure plus a named { tap, type } set? What shouldn't ship is a hand-set literal that is unread on ~a quarter of rows and information-free on the rest.
There was a problem hiding this comment.
Resolved in 95dc700. The field is deleted from CommandTraits and from all 34 rows; the consumer derives it.
The consumer, where the rule actually lives. RunnerTests+CommandExecution.swift:13-14:
if command.traits.convertsRecordedFailure,
!CommandTraits.textEntryWitnessOwners.contains(command.command)with CommandTraits.textEntryWitnessOwners: Set<CommandType> = [.tap, .type] declared next to the classification (RunnerTests+Models.swift:156-161) and named for the reason those two are excluded — tap records the witness and clears it where a tap demonstrably did not land, type reads the one the current command relies on. Everywhere else on that path, having a mutation to prove is what makes a remembered tap stale, which is why it is derived from convertsRecordedFailure rather than declared beside it.
Four facts, all read. Nothing replaced it, so the "rows that cannot reach the consumer declare a side effect" problem is gone rather than relocated: isInteraction is read by the foreground guard and the presented-surface stabilization, retryOnSessionLoss by the two recovery loops and the exception classifier, launchPolicy by the dispatch switch and the stopped-app refusal, convertsRecordedFailure by the recorded-failure wrapper and this gate. The eight lifecycle rows that declared true for a path they return before entering are simply absent now.
Where the derivation is equivalent. 25 commands reach executeOnMainPrepared, and for all 25 the derived value equals what the reviewed head declared: tap / type keep the witness; findText, readText, screenshot, gestureViewport and alert get do not clear it because convertsRecordedFailure is false for them; the 14 remaining interactions plus mouseClick, home, actionButton, querySelector and alert accept/dismiss clear it. The nine that answer before that path — snapshot, status, uptime, activate, terminate, targetReset, shutdown, recordStart, recordStop — cleared nothing on either head. testXCTestRecordedFailureGateIsTapOnlyAndCountGated and the alert golden table still pin their halves of this.
The per-command table I posted as a review comment shows this row by row against origin/main, and it is where the only changed cell (querySelector, launch → refuse) is recorded.
|
Reviewed at cd621b0. Giving launch its own policy axis is the right direction, and I found no defect in the new behavior. I have some questions about its shape before the label. Does Is Dispatch checks the policy only with Smaller design question: would named presets per command group, like The iOS Smoke Tests failure ( |
Per-command effective behavior:
|
| command | preflight | retry | stopped app | rec-fail | tap witness |
|---|---|---|---|---|---|
| tap | guard | no | launch | yes | keeps |
| type | guard | no | launch | yes | keeps |
| longPress | guard | no | launch | yes | clears |
| drag | guard | no | launch | yes | clears |
| remotePress | guard | no | launch | yes | clears |
| swipe | guard | no | launch | yes | clears |
| scroll | guard | no | launch | yes | clears |
| desktopScroll | guard | no | launch | yes | clears |
| backInApp | guard | no | launch | yes | clears |
| backSystem | guard | no | launch | yes | clears |
| rotate | guard | no | launch | yes | clears |
| appSwitcher | guard | no | launch | yes | clears |
| keyboardDismiss | guard | no | launch | yes | clears |
| keyboardReturn | guard | no | launch | yes | clears |
| sequence | guard | no | launch | yes | clears |
| gesture | guard | no | launch | yes | clears |
| mouseClick | — | no | launch | yes | clears |
| home | — | no | launch | yes | clears |
| recordStart | — | no | launch | yes | n/a |
| activate | — | no | launch | yes | n/a |
| findText | — | yes | refuse | no | keeps |
| readText | — | yes | refuse | no | keeps |
| gestureViewport | — | yes | refuse | no | keeps |
| snapshot | — | yes | refuse | no | n/a |
| screenshot | — | yes | untouched | no | keeps |
| status | — | yes | untouched | no | n/a |
alert (no action / get) |
— | yes | untouched | no | keeps |
alert (accept / dismiss) |
— | no | untouched | yes | clears |
| actionButton | — | no | untouched | yes | clears |
| querySelector | — | no | launch → refuse | yes | clears |
| recordStop | — | no | untouched | no | n/a |
| uptime | — | no | untouched | no | n/a |
| terminate | — | no | untouched | no | n/a |
| targetReset | — | no | untouched | no | n/a |
| shutdown | — | no | untouched | no | n/a |
querySelector is the only command whose effective behavior moved, which is the fix #2890 asks for. Three things that look like moves and are not:
screenshot/status/recordStop/uptime/terminate/targetReset/shutdownsit in.noAppinstead ofisLifecycle, and the refusal guard now readslaunchPolicy == .existingAppinstead ofisReadOnlyCommand— but those commands took the pre-refusal branch onorigin/maintoo, so neither head refuses them.tap/typemerged into the interaction group only because the 5th field they alone contradicted is gone; their witness behavior is unchanged and still owned by their own cases.alert/actionButtondeclare.presentedSurfacewhere the reviewed head computed.noAppon iOS. On iOS the runner resolves them without activating, as before. Off iOS nothing is registered to serve in place, so they keep the activation route they had onorigin/main— that difference is now written once, inprepareActiveCommandContext, andtestPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnboundcovers the iOS half (terminatedcom.apple.Preferencesstays.notRunning, binds nothing, and the nextsnapshotis refused).
thymikee
left a comment
There was a problem hiding this comment.
Amended as 95dc700 (implementation), 8f64d01 (docs), 7295ee9 (gates). Both blocking items are resolved, the cheap items are in, and the per-command behavior table is in my comment below — one cell moved.
Blocking 1 — the computed policy. hostedByFocusedSurface is deleted. CommandLaunchPolicy has a real .presentedSurface case, alert/actionButton name it at their rows, and prepareActiveCommandContext switches over the policy with the single #if os(iOS) inside that case. Full reasoning, the two mutation runs, and the live proof (including why the host lane does not compile it) are in my reply to your first inline comment.
Blocking 2 — the unreachable fifth fact. Field and all 34 declarations deleted; the rule is derived at its one consumer from convertsRecordedFailure minus a named { tap, type } set. Details and the 25/9 equivalence in my reply to your second inline comment.
Named groups. Eight file-private presets, each with the reason for its shape, so a group body reads like DEFAULT_TRAITS: interaction, appMutation, appRead, selectorResolution, runnerCaptureRead, runnerLifecycle, presentedSurfaceMutation, presentedSurfaceQuery. Command.traits (RunnerTests+Models.swift:210-244) now owns the switch directly and CommandType.traits(for:) — one accessor, one caller — is gone, so there is exactly one place to read a command's classification.
Table test. Expectations are literals in a private ExpectedTraits, compared fact by fact; the groups are fileprivate so the test cannot reach them even by accident. Mutating selectorResolution's policy and swapping two assignments inside CommandTraits.init each turn it red, with the messages quoted in the inline reply.
Fixture. readOnly → query in contracts/fixtures/alert-command-traits.json, with both readers moved: the Swift AlertCommandTraitsFixture maps query onto retryOnSessionLoss replay eligibility, the TypeScript test maps it onto the daemon's readOnly. No reader kept the old name — grep -rn '"readOnly"' only finds the daemon trait itself, which is untouched and still a separate concept.
ADR-0014. One commit, both halves: the rejected alternative no longer names the concept this PR deleted ("runner read-only traits" → "the runner's command traits", with the two classification purposes spelled out), and the body sentence no longer says the TypeScript readOnly trait only gates readiness probes — it is consumed as read-only resend, session-invalidation skip, transport error classification, and readiness preflight. ADR-0004/0005/0026 checked; they name the daemon trait only, which survives.
CHANGELOG. Added, since a stopped app on the selector route now answers APP_NOT_RUNNING where it used to launch the app — a caller-visible change.
Rejected, with evidence. I re-scoped testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound in a nested #if os(iOS) after a review pass claimed the macOS host lane would run its iOS-only refusal assertion. Reverted as unnecessary: the test already sits inside this file's simulator-only region (RunnerTests+CommandDispatchTests.swift:68), and the built bundles settle it — 0 occurrences of the selector in the macOS .xctest, present in the iOS one. check:xctest-selection still reports 0 methods reachable by no lane.
Gate status on 7295ee9: pnpm check:affected --run, check:xctest-selection (PR list 110, host 254, nightly 306, 0 unreachable), check:packaged-runner-swift and repo-wide format pass; iOS and macOS build:xcuitest succeed. The full iOS runner lane on this head is reported in the PR body once the run lands.
|
Reviewed the delta since cd621b0 at 7295ee9. The launch-policy split looks correct, and the per-command table answers the earlier evidence question: only The branch now conflicts with main in Not blocking: the extra indentation on the |
The iOS lane's fixture E2E failure on
|
| step | result |
|---|---|
open … --relaunch --launch-url agent-device-test-app:///automation?... |
0 (3.4 s) |
wait text 'Automation lab' 15000 (the probe's arrival check) |
1 after 15.3 s — APP_NOT_RUNNING |
alert get |
0 — message matched ^Open in\b, items contained Open |
alert accept |
0 (3.5 s) |
wait text 'Automation lab' 10000 |
1 after 11.6 s — APP_NOT_RUNNING |
So the confirmation was found and answered, and the app still never came up. That is not the traits table misbehaving: the dialog was answered 18 s after the launch it was holding, and a late tap releases the dialog without launching anything.
What changed is that nothing covers for it any more. On origin/main the first wait bare-launched the session app through the selector route, iOS delivered the pending URL on that launch, and the scenario passed without ever answering the prompt — the probe's own arrived.status === 0 early return meant alert get did not even run in CI. That masked launch is exactly what #2890 asks to remove, so the E2E is now telling the truth: a scenario that opens a held deep link has to relaunch after answering, which is also what the refusal's hint tells a caller to do ("Reads do not launch the app. Relaunch it with open; if a system prompt … holds its launch, answer it with alert accept.").
0a8061c5 makes acceptDeepLinkConfirmationIfPresent do that: when the destination has not arrived, it answers the confirmation if one is up, relaunches with the caller's own URL, and then asserts the destination instead of returning and letting the next step fail. The probe's shape is untouched — arrival still decides whether alert get runs at all, because reaching that probe on a live WebView route is the XCTest query that trips the runner watchdog (#2484).
Verification limit, stated plainly: the fixture app is a trusted CI artifact and this worktree has no test-app dependency install, so I could not reproduce the dialog locally. Locally this is covered by typecheck and the scenario's 14 static gates; the lane is the real verifier. If the relaunch is itself held behind a fresh confirmation, the next step is to loop accept-until-arrived inside that helper, and I would rather hear that from the run than widen it pre-emptively.
|
The PR has merge conflicts with main, so the CI, iOS, Android, and macOS workflows did not run on 0a8061c; only CodeQL ran. The last iOS run, on cd621b0, failed in the fixture E2E at The recovery at https://github.com/callstack/agent-device/blob/0a8061c/test/integration/ios-simulator-e2e/live-automation-scenario.ts#L299 sends Not blocking: every deep-link launch in the iOS E2E should handle arrival before its first read since reads no longer bare-launch a stopped app, and That same run also covers |
0a8061c to
addda18
Compare
|
Amended as The conflict, resolved at its cause. main deleted The deep-link question you asked. It no longer has an answer here, because my fixture delta is dropped: #2902 owned that helper and #2919 refined it. Its recovery never re-runs One gap remains and it is upstream's, not this PR's: on main, launch clear-state fixture through stored URL ( One test shape fixed from the second pass. Reverted, and the same test is green in the 63-test lane below. Device evidence — local iPhone 17 simulator, runner derived path cleared so the daemon rebuilt and reinstalled from this head,
The lane and this device evidence are my local runs, as before; CI on this head is the authority. |
|
On addda18 the code looks right: The route this diff changes is exercised by the I have not rerun the author's mutation of the Merge should wait for iOS Smoke Tests step 15 ('Run fixture-backed iOS simulator E2E smoke') on addda18 to finish green, with the Automation-lab deep-link wait reaching status 0 (directly or via the upstream |
CommandTraits.readOnly was documented as retry eligibility and consumed as five decisions, so opting a command out of one silently opted it out of the rest. querySelector is deliberately not retried, and as a side effect it stopped being refused while its app was stopped: it bare-launched the app, which #2852 forbids for a runner read. Replace readOnly with the facts each decision actually asks for — retryOnSessionLoss, launchPolicy (noApp | existingApp | mayLaunch), convertsRecordedFailure, and clearsRememberedTextEntryTap — and replace isLifecycle, which served both the activation bypass and the recorded-failure exemption. Payload-dependent facts resolve in one exhaustive switch against Command, so no consumer re-derives the payload rule and CommandTraits.ReadOnly.conditional is gone. A command hosted by the surface that already has focus keeps the route its platform proved: the skip is iOS-only, so macOS and tvOS still activate, and only iOS answers a stopped-app read with APP_NOT_RUNNING. querySelector now refuses rather than launching, and stays non-retried. Co-Authored-By: opencode
…nsumer
`CommandLaunchPolicy.hostedByFocusedSurface` was a computed `#if` value, so a row
for a command hosted by the focused surface resolved to a bare launch off iOS,
and the completeness test asserted through the same helper it was meant to check.
The enum now has a real `presentedSurface` case, the one platform exception lives
where the policy is read, and dispatch switches over the policy — so a new case
is a compile error rather than a fall-through, and the request-dependent bypass
that queries the cached target is reached only where activation is on the table.
`clearsRememberedTextEntryTap` was read by one consumer that nine of its rows
never reached, and equalled `convertsRecordedFailure` minus `{ tap, type }`
wherever it was read. It is now derived there, from that fact plus a named set.
Commands that decide alike share a named group, and `Command.traits` owns the
switch directly. The classification test now compares every fact against its own
literal instead of a value built by the type under test, so an initializer that
swapped two facts or a row re-pointed at another policy goes red.
… layers A stopped app on the selector route now answers `APP_NOT_RUNNING` instead of launching, which a caller can see. ADR-0014 rejected deriving from "runner read-only traits", a concept this PR deleted, and described the TypeScript `readOnly` trait as gating readiness probes alone. It gates read-only resend, session-invalidation skip, transport error classification, and readiness preflight.
The runner no longer classifies commands by read-only-ness, so a shared column named after it implied a concept the runner no longer has. `query` names the fact both sides agree on — the request changes nothing — which each maps to its own consumer: replay eligibility in the runner, `readOnly` in the daemon. Also names the `.presentedSurface` dispatch proof in the iOS PR lane, so what the declared policy does to a stopped app is checked on every pull request rather than only in the nightly.
addda18 to
5151371
Compare
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift:13">
P2: Do not use `convertsRecordedFailure` as the text-entry-witness invalidation predicate: it makes the non-mutating `querySelector` clear a valid tap→bare-type witness. Give witness invalidation its own trait, or classify selector resolution consistently with the other read commands.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| if command.command != .tap && command.command != .type && !isReadOnlyCommand(command) { | ||
| // Every command that reaches here with a mutation to prove makes a remembered text-entry tap | ||
| // stale; the two commands that own that witness decide for themselves in their own cases below. | ||
| if command.traits.convertsRecordedFailure, |
There was a problem hiding this comment.
P2: Do not use convertsRecordedFailure as the text-entry-witness invalidation predicate: it makes the non-mutating querySelector clear a valid tap→bare-type witness. Give witness invalidation its own trait, or classify selector resolution consistently with the other read commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift, line 13:
<comment>Do not use `convertsRecordedFailure` as the text-entry-witness invalidation predicate: it makes the non-mutating `querySelector` clear a valid tap→bare-type witness. Give witness invalidation its own trait, or classify selector resolution consistently with the other read commands.</comment>
<file context>
@@ -8,7 +8,11 @@ extension RunnerTests {
- if command.command != .tap && command.command != .type && !isReadOnlyCommand(command) {
+ // Every command that reaches here with a mutation to prove makes a remembered text-entry tap
+ // stale; the two commands that own that witness decide for themselves in their own cases below.
+ if command.traits.convertsRecordedFailure,
+ !CommandTraits.textEntryWitnessOwners.contains(command.command)
+ {
</file context>
There was a problem hiding this comment.
1 existing issue remains and 4 new issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift:435">
P2: A screenshot taken while a registered system surface is foregrounded now resolves the cached/requested app instead of the presented host. On foldable devices this can select the wrong display (or fall back to SpringBoard), so preserve the presented system-surface target before resolving the no-app target.</violation>
<violation number="2" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift:450">
P3: Inside the combined `case .existingApp, .mayLaunch:` arm, the `shouldSkipAppActivationPreflight` bypass is reachable only for `.mayLaunch`: the bypass requires `isCoordinateOnlyTap` (command == `.tap` with x/y and no text/selector), and no `.existingApp` command (.findText/.readText/.snapshot/.gestureViewport/.querySelector) can ever satisfy it. The branch is dead code for half the case label, which reads as if reads could skip the preflight. Split the case so the bypass sits only under `.mayLaunch` (or drop it from the `.existingApp` arm).</violation>
</file>
<file name="packages/platform-apple/src/runner/__tests__/runner-command-traits.test.ts">
<violation number="1" location="packages/platform-apple/src/runner/__tests__/runner-command-traits.test.ts:88">
P3: The new comment states the shared fact is that "the alert request changes nothing", but the same fixture marks `accept` and `dismiss` as `query: false` ("accept mutates", "dismiss mutates"), and the next lines assert those rows map to `readOnly: false`. The `query` column records *whether* the request changes something, not that it never does. Rephrase so `alert get` is identified as the only side-effect-free case — the Swift side already phrases it correctly in `RunnerTests+Models.swift` ("`alert get` changes nothing, so it is the one alert action that may be replayed").</violation>
</file>
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Lifecycle.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Lifecycle.swift:324">
P3: The `.existingApp` refusal is compiled in only under `#if os(iOS)`, but `Command.traits` declares `.existingApp` (and the new `querySelector` refusal that fixes #2890) for every platform. On tvOS/macOS, `prepareActivatedTarget` therefore still reaches `activateTarget` for a stopped app and bare-launches it — on macOS the same activation route has always applied, but `querySelector` now joins it through `.existingApp` with documented "refuses rather than launching" semantics that cannot occur there. State this scope in the `existingApp` case documentation or enforce the refusal off iOS too.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Fix all with cubic | Re-trigger cubic
| case .noApp: | ||
| // Answers from the runner's own capture and state, so the target is resolved exactly as it | ||
| // stands. | ||
| return .context(ActiveCommandContext(app: resolveAppWithoutActivation(command: command))) |
There was a problem hiding this comment.
P2: A screenshot taken while a registered system surface is foregrounded now resolves the cached/requested app instead of the presented host. On foldable devices this can select the wrong display (or fall back to SpringBoard), so preserve the presented system-surface target before resolving the no-app target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift, line 435:
<comment>A screenshot taken while a registered system surface is foregrounded now resolves the cached/requested app instead of the presented host. On foldable devices this can select the wrong display (or fall back to SpringBoard), so preserve the presented system-surface target before resolving the no-app target.</comment>
<file context>
@@ -418,88 +418,121 @@ extension RunnerTests {
+ case .noApp:
+ // Answers from the runner's own capture and state, so the target is resolved exactly as it
+ // stands.
+ return .context(ActiveCommandContext(app: resolveAppWithoutActivation(command: command)))
+ case .presentedSurface:
+ // The command is about the surface that already has focus; activating an app under it would
</file context>
| return .context(ActiveCommandContext(app: resolveAppWithoutActivation(command: command))) | |
| if let presented = presentedSystemSurfaceHost() { | |
| return .context(ActiveCommandContext(app: presented.app, systemSurface: presented.host)) | |
| } | |
| return .context(ActiveCommandContext(app: resolveAppWithoutActivation(command: command))) |
| case .existingApp, .mayLaunch: | ||
| // Asked only where activation is on the table: the bypass decides by querying the cached | ||
| // target's state, and a command that may bring nothing forward has nothing for it to settle. | ||
| if shouldSkipAppActivationPreflight(command) { |
There was a problem hiding this comment.
P3: Inside the combined case .existingApp, .mayLaunch: arm, the shouldSkipAppActivationPreflight bypass is reachable only for .mayLaunch: the bypass requires isCoordinateOnlyTap (command == .tap with x/y and no text/selector), and no .existingApp command (.findText/.readText/.snapshot/.gestureViewport/.querySelector) can ever satisfy it. The branch is dead code for half the case label, which reads as if reads could skip the preflight. Split the case so the bypass sits only under .mayLaunch (or drop it from the .existingApp arm).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift, line 450:
<comment>Inside the combined `case .existingApp, .mayLaunch:` arm, the `shouldSkipAppActivationPreflight` bypass is reachable only for `.mayLaunch`: the bypass requires `isCoordinateOnlyTap` (command == `.tap` with x/y and no text/selector), and no `.existingApp` command (.findText/.readText/.snapshot/.gestureViewport/.querySelector) can ever satisfy it. The branch is dead code for half the case label, which reads as if reads could skip the preflight. Split the case so the bypass sits only under `.mayLaunch` (or drop it from the `.existingApp` arm).</comment>
<file context>
@@ -418,88 +418,121 @@ extension RunnerTests {
+ case .existingApp, .mayLaunch:
+ // Asked only where activation is on the table: the bypass decides by querying the cached
+ // target's state, and a command that may bring nothing forward has nothing for it to settle.
+ if shouldSkipAppActivationPreflight(command) {
+ // The one request-dependent bypass: a coordinate-only synthesized tap whose cached target is
+ // already foreground needs nothing brought forward.
</file context>
| // The fixture's `query` column names the shared fact — the alert request changes nothing — which | ||
| // each side consumes under its own name: `readOnly` for this daemon trait, retry eligibility for | ||
| // the Apple runner, which no longer classifies commands by read-only-ness at all. |
There was a problem hiding this comment.
P3: The new comment states the shared fact is that "the alert request changes nothing", but the same fixture marks accept and dismiss as query: false ("accept mutates", "dismiss mutates"), and the next lines assert those rows map to readOnly: false. The query column records whether the request changes something, not that it never does. Rephrase so alert get is identified as the only side-effect-free case — the Swift side already phrases it correctly in RunnerTests+Models.swift ("alert get changes nothing, so it is the one alert action that may be replayed").
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-apple/src/runner/__tests__/runner-command-traits.test.ts, line 88:
<comment>The new comment states the shared fact is that "the alert request changes nothing", but the same fixture marks `accept` and `dismiss` as `query: false` ("accept mutates", "dismiss mutates"), and the next lines assert those rows map to `readOnly: false`. The `query` column records *whether* the request changes something, not that it never does. Rephrase so `alert get` is identified as the only side-effect-free case — the Swift side already phrases it correctly in `RunnerTests+Models.swift` ("`alert get` changes nothing, so it is the one alert action that may be replayed").</comment>
<file context>
@@ -85,19 +85,22 @@ test('runner command trait helpers read from the shared trait table', () => {
});
test('alert actions match the native read-only golden table', () => {
+ // The fixture's `query` column names the shared fact — the alert request changes nothing — which
+ // each side consumes under its own name: `readOnly` for this daemon trait, retry eligibility for
+ // the Apple runner, which no longer classifies commands by read-only-ness at all.
</file context>
| // The fixture's `query` column names the shared fact — the alert request changes nothing — which | |
| // each side consumes under its own name: `readOnly` for this daemon trait, retry eligibility for | |
| // the Apple runner, which no longer classifies commands by read-only-ness at all. | |
| // The fixture's `query` column records whether the alert request changes anything — only the | |
| // get action is side-effect-free — and each side consumes it under its own name: `readOnly` for | |
| // this daemon trait, retry eligibility for the Apple runner, which no longer classifies commands | |
| // by read-only-ness at all. |
| func notRunningRefusal(command: Command, bundleId: String) -> Response? { | ||
| #if os(iOS) | ||
| guard isReadOnlyCommand(command), | ||
| guard command.traits.launchPolicy == .existingApp, |
There was a problem hiding this comment.
P3: The .existingApp refusal is compiled in only under #if os(iOS), but Command.traits declares .existingApp (and the new querySelector refusal that fixes #2890) for every platform. On tvOS/macOS, prepareActivatedTarget therefore still reaches activateTarget for a stopped app and bare-launches it — on macOS the same activation route has always applied, but querySelector now joins it through .existingApp with documented "refuses rather than launching" semantics that cannot occur there. State this scope in the existingApp case documentation or enforce the refusal off iOS too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Lifecycle.swift, line 324:
<comment>The `.existingApp` refusal is compiled in only under `#if os(iOS)`, but `Command.traits` declares `.existingApp` (and the new `querySelector` refusal that fixes #2890) for every platform. On tvOS/macOS, `prepareActivatedTarget` therefore still reaches `activateTarget` for a stopped app and bare-launches it — on macOS the same activation route has always applied, but `querySelector` now joins it through `.existingApp` with documented "refuses rather than launching" semantics that cannot occur there. State this scope in the `existingApp` case documentation or enforce the refusal off iOS too.</comment>
<file context>
@@ -316,11 +316,12 @@ extension RunnerTests {
+ func notRunningRefusal(command: Command, bundleId: String) -> Response? {
#if os(iOS)
- guard isReadOnlyCommand(command),
+ guard command.traits.launchPolicy == .existingApp,
XCUIApplication(bundleIdentifier: bundleId).state == .notRunning
else { return nil }
</file context>
|
Reviewed at 5151371. This is ready for human review. The activation gap from the earlier pass (addda18) is fixed: launch now has its own policy axis, and All 19 checks pass on 5151371, including the iOS lane that runs the selector/wait route this PR touches (the fixture E2E step "wait for Automation lab" and the derived iOS PR XCTest lane's testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound). I did not open the Smoke Tests step-history for 5151371, so I can't quote the Automation-lab wait reaching status 0 or confirm the "launch clear-state fixture through stored URL" step directly; I'm relying on the packet's report that all checks pass. The device evidence on rnav-repro and the mutation runs of the Not blocking: the screenshot comment at apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift#L460 still describes the old |
Summary
CommandTraits.readOnlywas documented as retry eligibility and consumed as five decisions, soquerySelector's deliberate opt-out of session-loss retry also opted it out of the stopped-app refusal: it bare-launched the app, which #2852 forbids for a runner read. The table now declares each decision separately —retryOnSessionLoss,launchPolicy(noApp | existingApp | mayLaunch | presentedSurface),convertsRecordedFailure— over named groups, andCommand.traitsowns the switch. Launch is refused where it is taken, never inferred.clearsRememberedTextEntryTapis derived at its one consumer. OnlyquerySelectorchanges behavior. 10 files, 715 lines. Closes #2890.User-visible change (iOS runner): a selector read no longer starts the app it was asked about.
querySelectornaming an app that is not running bare-launched it instead of answeringAPP_NOT_RUNNING— the refusal applies to any app the request names by bundle id whose state the runner can read as stopped, not only to one the runner had already bound. It bare-launched because the runner read what to do about a stopped app from the same flag that says whether a command may be replayed after its session was invalidated, andquerySelectoris deliberately never replayed. What the runner may do about a stopped app is now declared per command, so the two can no longer move each other. No other command changed what it does about a stopped app: atapin the same state still activates.Validation
Head
51513719b, rebased onto6428c54853, which is noworigin/main— the merge base is main's tip, so nothing needed re-applying. #2922 (CHANGELOG.md) and #2896 (derived iOS lane) are ancestors of this head:git diff --name-only origin/main...HEADcarries no hunk for either. The note lives in this body, the derived lane is main's (check:xctest-selection: 322 methods, host lane 261, iOS PR lane 63, 0 unreachable). NohostedByFocusedSurface/clearsRememberedTextEntryTapanywhere.CommandLaunchPolicyis a plain sum type, andpresentedSurface's#if os(iOS)exception is written once, atprepareActiveCommandContext. The table test asserts literal per-command facts through its ownExpectedTraitsand cannot reach thefileprivategroups.Gates on this head:
pnpm build,check:affected --run,check:xctest-selection,check:packaged-runner-swift(56 files), iOS + macOSbuild:xcuitest— all green.Device evidence on
rnav-repro: runner reinstalled from a freshbuild:xcuitest:ios— build cache for fingerprintbce9391b…(matches this checkout) deleted, reinstalled as a new bundle carryingpresentedSurface, not the retired names. With the app stopped,is exists label="Push article"returnedAPP_NOT_RUNNINGand launchd reported 0 app processes afterwards — left stopped. A coordinatepress 75 160in that state activated it (PID 27091);is existsthen passed. A selectorpressrefuses identically, because it resolves throughquerySelectorfirst. Session closed, daemon stopped--clean.Rejected: memoizing
Command.traits.Risks: reads with no
appBundleId, and macOS/tvOS reads, keep their pre-change route. The macOS host lane needs host automation permission, so it is CI-verified here.