Skip to content

fix(ios-runner): give app launch its own policy axis - #2899

Merged
thymikee merged 4 commits into
mainfrom
fix/runner-launch-policy
Sep 25, 2026
Merged

thymikee merged 4 commits into
mainfrom
fix/runner-launch-policy

Conversation

@thymikee

@thymikee thymikee commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Summary

CommandTraits.readOnly was documented as retry eligibility and consumed as five decisions, so querySelector'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, and Command.traits owns the switch. Launch is refused where it is taken, never inferred. clearsRememberedTextEntryTap is derived at its one consumer. Only querySelector changes behavior. 10 files, 715 lines. Closes #2890.

User-visible change (iOS runner): a selector read no longer starts the app it was asked about. querySelector naming an app that is not running bare-launched it instead of answering APP_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, and querySelector is 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: a tap in the same state still activates.

Validation

Head 51513719b, rebased onto 6428c54853, which is now origin/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...HEAD carries 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). No hostedByFocusedSurface/clearsRememberedTextEntryTap anywhere. CommandLaunchPolicy is a plain sum type, and presentedSurface's #if os(iOS) exception is written once, at prepareActiveCommandContext. The table test asserts literal per-command facts through its own ExpectedTraits and cannot reach the fileprivate groups.

Gates on this head: pnpm build, check:affected --run, check:xctest-selection, check:packaged-runner-swift (56 files), iOS + macOS build:xcuitest — all green.

Device evidence on rnav-repro: runner reinstalled from a fresh build:xcuitest:ios — build cache for fingerprint bce9391b… (matches this checkout) deleted, reinstalled as a new bundle carrying presentedSurface, not the retired names. With the app stopped, is exists label="Push article" returned APP_NOT_RUNNING and launchd reported 0 app processes afterwards — left stopped. A coordinate press 75 160 in that state activated it (PID 27091); is exists then passed. A selector press refuses identically, because it resolves through querySelector first. 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.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.81 MB 4.81 MB +797 B
Package (unpacked) 4.81 MB 4.81 MB +797 B
Package (download) 1.44 MB 1.44 MB +297 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 18.7 ms 18.3 ms -0.4 ms
CLI --help 51.3 ms 52.6 ms +1.3 ms

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts uses DEFAULT_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.
  • readOnly is deleted from the runner everywhere, but contracts/fixtures/alert-command-traits.json still keys its column readOnly. 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 unlabeled Bools 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 readOnly trait a single decision; readOnly no 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 {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"} isInteraction and the same for retryOnSessionLoss. 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thymikee

Copy link
Copy Markdown
Member Author

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 hostedByFocusedSurface need to be a computed #if value? Now the row does not show its policy, and the table test in ModelsTests asserts against the same helper, so a wrong platform mapping would pass (RunnerTests+Models.swift#L63). If the rows declared .noApp directly, or the platform exception lived once where the policy is read, the test could assert the real cases.

Is clearsRememberedTextEntryTap needed as its own field? On every row that reaches executeOnMainPrepared it equals convertsRecordedFailure && !{tap, type}, and the other rows return earlier (RunnerTests+Models.swift#L96). Deriving it at that switch would remove a duplicate fact.

Dispatch checks the policy only with == .noApp and == .existingApp, so .mayLaunch is the fall-through (RunnerTests+CommandDispatch.swift#L429). A switch over launchPolicy in prepareActiveCommandContext would make a new case a compile error.

Smaller design question: would named presets per command group, like DEFAULT_TRAITS in packages/platform-apple/src/runner/runner-command-traits.ts, plus the derived field above, give the same behavior with about half as many literal rows? If you rejected that, what was the reason?

The iOS Smoke Tests failure (wait for Automation lab) looks unrelated. The commands on that step do not use the selector route this PR changes, and the same all-APP_NOT_RUNNING signature appeared before this PR; #2902 addresses it. A small CHANGELOG entry would help, because a stopped app on the selector route now returns APP_NOT_RUNNING instead of launching. There are no conflicts.

@thymikee

Copy link
Copy Markdown
Member Author

Per-command effective behavior: origin/main vs this head

Derived by walking each command through both consumer sets — isInteractionCommand / shouldRetryCommand / notRunningReadResponse / isRunnerLifecycleCommand / the inline != .tap && != .type && !isReadOnly guard on origin/main, against isInteraction / retryOnSessionLoss / launchPolicy + notRunningRefusal / convertsRecordedFailure / the derived { tap, type } gate here. 34 commands, alert shown as its two payload branches.

Columns show what both heads do, except a cell written a → b, which is the only kind of move in this table.

  • preflight = foreground guard + interaction stabilization (isInteraction)
  • retry = session-invalidating replay
  • stopped app = iOS, an explicit appBundleId, that app .notRunning: launch (activate = bare launch), refuse (APP_NOT_RUNNING), untouched (target resolved as it stands, nothing brought forward)
  • rec-fail = an XCTest-recorded failure turns the healthy response into a failure and invalidates the session
  • tap witness = clears the remembered text-entry tap on the prepared path; n/a = the command is answered before that path, on both heads
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 / shutdown sit in .noApp instead of isLifecycle, and the refusal guard now reads launchPolicy == .existingApp instead of isReadOnlyCommand — but those commands took the pre-refusal branch on origin/main too, so neither head refuses them.
  • tap / type merged 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 / actionButton declare .presentedSurface where the reviewed head computed .noApp on 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 on origin/main — that difference is now written once, in prepareActiveCommandContext, and testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound covers the iOS half (terminated com.apple.Preferences stays .notRunning, binds nothing, and the next snapshot is refused).

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed the delta since cd621b0 at 7295ee9. The launch-policy split looks correct, and the per-command table answers the earlier evidence question: only querySelector changes behavior. All 5 checks pass, including the new testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound case in the iOS PR lane. The full runner lane and the mutation runs are reported in the PR; I did not rerun them.

The branch now conflicts with main in CHANGELOG.md, which is the next blocker. While you resolve it, could the entry say that the refusal applies to an explicitly named app that is not running? notRunningRefusal fires for any explicit appBundleId whose app is .notRunning, not only for an app the runner had bound.

Not blocking: the extra indentation on the assertTraits call in RunnerTests+ModelsTests.swift.

@thymikee

Copy link
Copy Markdown
Member Author

The iOS lane's fixture E2E failure on cd621b066f, and what I changed about it

The lane failed Run fixture-backed iOS simulator E2E smoke at wait for Automation lab. I pulled the run's step-history.json rather than guess, and the sequence is unambiguous:

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.

@thymikee

Copy link
Copy Markdown
Member Author

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 wait for Automation lab, and that failure is PR-caused: the PR changes the selector/wait route to refuse a not-running app instead of launching it, which is exactly what that step exercises. This delta targets that failure but has not been run.

The recovery at https://github.com/callstack/agent-device/blob/0a8061c/test/integration/ios-simulator-e2e/live-automation-scenario.ts#L299 sends open --relaunch --launch-url again through simctl openurl, the same route that produced the "Open in" confirmation the first time. If that confirmation returns on the relaunch, the final wait at line 306 gets APP_NOT_RUNNING again, and nothing handles that. Does the "Open in" confirmation reappear on a second simctl openurl call in this flow? Resolve the CHANGELOG.md conflict so the iOS workflow can run on a head that contains 0a8061c, then attach the step-history for "Run fixture-backed iOS simulator E2E smoke" on that head. It needs to show the deep-link destination wait reaching status 0, either on the first wait or on the wait after "relaunch the deep link the confirmation held," and it needs to show "launch clear-state fixture through stored URL" passing.

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 assertClearStateLaunchUrl at line 241 doesn't go through acceptDeepLinkConfirmationIfPresent so it may hit the same gap when the f1 lane runs it, and the relaunch at line 299 currently fires on any destination miss rather than only when arrived.json.error.code === 'APP_NOT_RUNNING', so a genuine arrival failure such as a slow or broken route render gets a silent second attempt — both worth a look, but neither has to hold up this PR.

That same run also covers testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound in the iOS PR lane, which the earlier review wrongly counted as CI-verified. I could not tell whether the Maestro clear-state launchApp delivers its URL through openurl and triggers the confirmation, since the step-history of the failing run stopped before that point. The 309-tests-0-failures runner XCTest result in the PR body is the author's local run; I have not rerun it. The earlier review's CHANGELOG wording ask, that the refusal applies to any explicitly named app that is not running, is outside this delta and I could not check it while the conflict is unresolved.

@thymikee
thymikee force-pushed the fix/runner-launch-policy branch from 0a8061c to addda18 Compare September 24, 2026 17:39
@thymikee

Copy link
Copy Markdown
Member Author

Amended as bccaab3f2 (implementation), 10e91873e (docs), addda1897 (gates), rebased onto 0.21.14.

The conflict, resolved at its cause. main deleted CHANGELOG.md in #2922 for generated release notes, so the entry is deleted rather than merged — its text is now the "User-visible change" paragraph in the body, which is what the notes get fed from. The ADR-0014 half of that commit is unchanged. The ios.yml hunk is also gone: #2896 replaced the hand-written -only-testing list with check-xctest-selection.ts --ios-pr-tests, and the derived set already names testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound (63 ids on this head), so the test is in the PR lane with no workflow edit. CI is running on this head now (iOS, CI, macOS, Linux queued 17:39:46Z).

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 openurl — it waits, alert get, alert accept, waits again across five bounded waits — so the second-confirmation risk my relaunch carried does not exist upstream. Your "only retry on APP_NOT_RUNNING" ask is upstream's launchPending gate (details.runnerErrorCode === APP_NOT_RUNNING), and a targetAbsent miss with no prompt returns to the caller's assertion instead of getting a silent second launch. Evidence for the flow as merged: run 36030537177 on main 622435ecc5, job Smoke Tests, step 15 Run fixture-backed iOS simulator E2E smoke success, step 9 Run targeted iOS runner XCTest regressions success.

One gap remains and it is upstream's, not this PR's: on main, launch clear-state fixture through stored URL (live-automation-scenario.ts:235) is followed by assertElementText reads with no acceptDeepLinkConfirmationIfPresent, so if a Maestro stored-URL launchApp surfaces the same confirmation, those reads take APP_NOT_RUNNING. I could not tell from the flows whether that delivery goes through openurl either. Say the word and I will file it rather than widen this diff.

One test shape fixed from the second pass. testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound only required .context, so a .presentedSurface arm that resolved the wrong surface passed. It now asserts the prepared target is the stopped app (state == .notRunning) with no systemSurface. Mutating the iOS arm to ActiveCommandContext(app: springboard) and running just that test:

XCTAssertEqual failed: ("XCUIApplicationState(rawValue: 4)") is not equal to ("XCUIApplicationState(rawValue: 1)") - {"command":"actionButton",...} must be prepared against the stopped app it names, not a live surface
XCTAssertEqual failed: ... {"command":"alert","action":"get",...}
Test Case '...testPresentedSurfaceCommandLeavesAStoppedAppStoppedAndUnbound' failed (0.115 seconds).

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, com.apple.Preferences terminated before each pair:

command result corroboration
is visible 'label="General"' APP_NOT_RUNNING, hint "Reads do not launch the app…" runner log AGENT_DEVICE_RUNNER_READ_TARGET_NOT_RUNNING bundle=com.apple.Preferences command=snapshot; launchctl list shows no com.apple.Preferences before or after
press 190 700 success response carries targetActivation { priorState: 1, reason: "bundle_changed" }; runner log AGENT_DEVICE_RUNNER_ACTIVATE … state=1 reason=bundle_changed + AGENT_DEVICE_RUNNER_ACTIVATE_FACT … priorState=1; launchctl then lists UIKitApplication:com.apple.Preferences[6e66]

The lane and this device evidence are my local runs, as before; CI on this head is the authority.

@thymikee

Copy link
Copy Markdown
Member Author

On addda18 the code looks right: prepareActiveCommandContext now carries its own launch policy axis, and querySelector refusing a stopped app is the correct fix for the earlier failure. I don't have a new code finding beyond the 0a8061c round.

The route this diff changes is exercised by the wait for Automation lab step in the fixture E2E, where cd621b0 failed before. Steps 9 and 11 on addda18 already passed, including the three launch-policy XCTest regressions. Step 15, the fixture-backed simulator E2E, is still in progress and hasn't reported yet.

I have not rerun the author's mutation of the .presentedSurface arm (SpringBoard, state 4 vs 1); the code reading supports it failing, but that's not verified evidence. The local iPhone 17 run (is visible returning APP_NOT_RUNNING, press activating with bundle_changed) and the 63-test local lane are the author's own runs, not something I reproduced; CI step 9 on this head covers the XCTest lane independently. Whether the Maestro stored-URL launchApp in assertClearStateLaunchUrl delivers through openurl and raises the confirmation is still open — the author already offered to file that upstream, so I'm not raising it again here.

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 acceptDeepLinkConfirmationIfPresent recovery) and 'launch clear-state fixture through stored URL' passing.

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.
@thymikee
thymikee force-pushed the fix/runner-launch-policy branch from addda18 to 5151371 Compare September 24, 2026 19:28

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

@cubic-dev-ai cubic-dev-ai Bot Sep 24, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejected — the derivation preserves the merge-base behavior. The predicate at merge-base RunnerTests+CommandExecution.swift:11 was command != .tap && command != .type && !isReadOnlyCommand(command), and querySelector was classified readOnly: .never (merge-base RunnerTests+Models.swift:116-117). isReadOnlyCommand(querySelector) was false, so the merge-base already cleared the remembered text-entry tap for querySelector on the prepared path. Command.invalidatesRememberedTextEntryTap (RunnerTests+Models.swift:176-178: convertsRecordedFailure && !textEntryWitnessOwners.contains(command)) answers true for querySelector, exactly as before — nothing newly clears the witness.

Both alternatives are closed: a dedicated witness trait reinstates the fifth axis deleted on the maintainer's explicit order in #2890, and reclassifying selector resolution with the other reads changes querySelector's launch/replay behavior, which is out of this PR's scope.

Enforcement added instead, in #2954 (5709698): the exhaustive table test gained a witness-clearing column — assertRememberedTextEntryWitnessInvalidation (UnitTests/RunnerTests+ModelsTests.swift:214-238) compares every command against the merge-base predicate applied to a literal copy of the merge-base read-only set (:196-198), not against anything under test. Mutation-proven: excluding querySelector from the derived clearing turns its row red ("{"command":"querySelector"} must invalidate a remembered text-entry tap exactly as the merge-base did").

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 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)))

@cubic-dev-ai cubic-dev-ai Bot Sep 24, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
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)))
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #2954 (head 5709698). The .noApp arm now consults presentedSystemSurfaceHost() first and returns .context(ActiveCommandContext(app: presented.app, systemSurface: presented.host)) when a registered host is foregrounded, restoring the merge-base route (#2438); with nothing presented it serves the standing cached target (currentApp ?? app), which is what the merge-base's lifecycle route resolved to.

Evidence: RunnerTests+CommandDispatch.swift:466-478 (new arm), and RunnerTests+CommandDispatchTests.swift:152 arms a registered host and pins both halves — presented host served in place with provenance and no rebind/activation, and the standing target served when nothing is presented. Mutation-proven: restoring the merged .noApp arm turns the test red on both halves (systemSurface nil, standing-target identity), and serving the registry's first entry unconditionally turns the second-host case red.

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) {

@cubic-dev-ai cubic-dev-ai Bot Sep 24, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #2954 (5709698). The combined arm is split: case .existingApp: (RunnerTests+CommandDispatch.swift:489-492) goes straight to prepareActivatedTarget, and case .mayLaunch: (:493-500) keeps the shouldSkipAppActivationPreflight bypass. No behavior change — as you noted, no .existingApp command can satisfy the coordinate-only .tap predicate.

Comment on lines +88 to +90
// 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.

@cubic-dev-ai cubic-dev-ai Bot Sep 24, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
// 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.
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #2954 (5709698). The comment at packages/platform-apple/src/runner/__tests__/runner-command-traits.test.ts:89-92 now says the query column records whether the alert request changes anything — get is the one action that is side-effect-free — matching the Swift phrasing in RunnerTests+Models.swift ("alert get changes nothing, so it is the one alert action that may be replayed").

func notRunningRefusal(command: Command, bundleId: String) -> Response? {
#if os(iOS)
guard isReadOnlyCommand(command),
guard command.traits.launchPolicy == .existingApp,

@cubic-dev-ai cubic-dev-ai Bot Sep 24, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #2954 (5709698). CommandLaunchPolicy.existingApp's case doc (RunnerTests+Models.swift:58-64) now states the refusal is enforced on iOS only — notRunningRefusal is #if os(iOS), the platform that can read an app's state without launching it — and that off iOS these commands keep the activation route they had before the axis existed, where no refusal can occur. The selectorResolution preset comment (:131-138) says the same for the querySelector half. The refusal was deliberately not extended off iOS: that would be a behavior change beyond #2890, and the old querySelector launched stopped apps off iOS too, so nothing regressed.

@thymikee

Copy link
Copy Markdown
Member Author

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 prepareActiveCommandContext resolves the requested bundle without activating it for .noApp commands like screenshot.

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 .presentedSurface arm are the author's own runs, not something I reproduced. It's still open, and upstream, whether a Maestro stored-URL launchApp raises the "Open in" confirmation before the reads in assertClearStateLaunchUrl; the author offered to file that separately.

Not blocking: the screenshot comment at apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandExecution.swift#L460 still describes the old isLifecycle skip-preflight behavior, when screenshot's launchPolicy is now .noApp and prepareActiveCommandContext resolves the bundle via resolveAppWithoutActivation without activation — worth a reword (or dropping the activeApp clause), but can be taken or left.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 24, 2026
@thymikee
thymikee merged commit c7b79ee into main Sep 25, 2026
19 checks passed
@thymikee
thymikee deleted the fix/runner-launch-policy branch September 25, 2026 05:54
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-25 05:54 UTC

@thymikee

Copy link
Copy Markdown
Member Author

@/tmp/wt2890-summary.md

thymikee added a commit that referenced this pull request Sep 25, 2026
`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.
thymikee added a commit that referenced this pull request Sep 25, 2026
…ds (#2954)

* fix(ios-runner): keep the presented surface's route for no-app commands

`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.

* refactor(ios-runner): put the activation bypass only where launch is 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.

* refactor(ios-runner): name the text-entry witness rule at its owning 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.

* docs(ios-runner): scope the existing-app refusal to where it is enforced

`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.

* test(apple): say the alert fixture records whether an action changes 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.

* chore(gates): pin witness clearing for every command in the trait table

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.

* test(ios-runner): make the presented-surface override answer every registered 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.

* docs(ios-runner): point the no-app arm at the trait table instead of copying it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(ios-runner): app-launch policy is decided by the retry flag; querySelector launches a stopped app

1 participant