refactor(apple): declare journal retention in command traits (#3094)

Author: thymikee

apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift

@@ -125,22 +125,9 @@ final class RunnerCommandJournal {
   }
 
   private func encodeResponseJson(command: Command, response: Response) -> String? {
-    guard shouldRetainResponseJson(command: command) else { return nil }
+    guard command.traits.retainsJournalResponseJson else { return nil }
     guard let data = try? JSONEncoder().encode(response) else { return nil }
     guard data.count <= maxResponseJsonBytes else { return nil }
     return String(data: data, encoding: .utf8)
   }
-
-  private func shouldRetainResponseJson(command: Command) -> Bool {
-    switch command.command {
-    case .snapshot, .screenshot:
-      return false
-    case .tap, .mouseClick, .longPress, .drag,
-         .remotePress, .type, .swipe, .scroll, .desktopScroll, .findText, .querySelector, .readText,
-         .backInApp, .backSystem, .home, .rotate, .appSwitcher, .actionButton, .keyboardDismiss, .keyboardReturn,
-         .alert, .sequence, .gesture, .gestureViewport, .recordStart, .recordStop,
-         .status, .uptime, .appState, .pasteboardWrite, .activate, .terminate, .targetReset, .shutdown:
-      return true
-    }
-  }
 }

apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift

@@ -93,25 +93,29 @@ struct CommandTraits {
   let isInteraction: Bool
   /// Whether the command is eligible for the session-invalidating retry.
   let retryOnSessionLoss: Bool
-  /// What the runner may do when the command's app is not running. The one fact with no default: no
-  /// command inherits a launch answer from how it was classified for anything else.
+  /// What the runner may do when the command's app is not running. No command inherits a launch
+  /// answer from how it was classified for anything else.
   let launchPolicy: CommandLaunchPolicy
   /// Whether an XCTest-recorded failure during this command turns its own healthy response into a
   /// failure and invalidates the session. That conversion is the only evidence a mutation with no
   /// settle and no post-action observation ever landed, while a command that reports the runner's own
   /// state or drives its lifecycle has no user-visible mutation to prove.
   let convertsRecordedFailure: Bool
+  /// Whether the journal may store this command's response JSON, subject to its byte limit.
+  let retainsJournalResponseJson: Bool
 
   init(
     isInteraction: Bool = false,
     retryOnSessionLoss: Bool = false,
     launchPolicy: CommandLaunchPolicy,
-    convertsRecordedFailure: Bool = false
+    convertsRecordedFailure: Bool = false,
+    retainsJournalResponseJson: Bool
   ) {
     self.isInteraction = isInteraction
     self.retryOnSessionLoss = retryOnSessionLoss
     self.launchPolicy = launchPolicy
     self.convertsRecordedFailure = convertsRecordedFailure
+    self.retainsJournalResponseJson = retainsJournalResponseJson
   }
 }
 
@@ -121,52 +125,68 @@ fileprivate extension CommandTraits {
   static let interaction = CommandTraits(
     isInteraction: true,
     launchPolicy: .mayLaunch,
-    convertsRecordedFailure: true
+    convertsRecordedFailure: true,
+    retainsJournalResponseJson: true
   )
 
   /// Mutations the runner performs without the element-interaction preflight. NOTE: `mouseClick`
   /// stays non-interaction for now — it is macOS-only and the foreground guard interacts with
   /// bespoke macOS activation, so classifying it needs a macOS smoke check first (tracked as a
   /// follow-up).
-  static let appMutation = CommandTraits(launchPolicy: .mayLaunch, convertsRecordedFailure: true)
+  static let appMutation = CommandTraits(
+    launchPolicy: .mayLaunch, convertsRecordedFailure: true, retainsJournalResponseJson: true
+  )
 
   /// Reads of the session app: replayable after session invalidation, and refused rather than
   /// answered by starting the app.
-  static let appRead = CommandTraits(retryOnSessionLoss: true, launchPolicy: .existingApp)
+  static func appRead(retainsJournalResponseJson: Bool) -> CommandTraits {
+    CommandTraits(
+      retryOnSessionLoss: true, launchPolicy: .existingApp,
+      retainsJournalResponseJson: retainsJournalResponseJson
+    )
+  }
 
   /// Selector resolution is an observation: it refuses a stopped app instead of bare-launching it,
   /// and the runner still must not replay it after session invalidation. Those are two facts about
   /// one command, which is why they are two declarations (#2890). The refusal is the iOS-enforced
   /// half: off iOS a selector read of a stopped app still activates it, as it did before this axis.
   static let selectorResolution = CommandTraits(
     launchPolicy: .existingApp,
-    convertsRecordedFailure: true
+    convertsRecordedFailure: true,
+    retainsJournalResponseJson: true
   )
 
   /// Reads the runner answers from its own capture and state, so preparation never brings an app
   /// forward; a capture aimed at an app still observes that app while it executes.
-  static let runnerCaptureRead = CommandTraits(retryOnSessionLoss: true, launchPolicy: .noApp)
+  static func runnerCaptureRead(retainsJournalResponseJson: Bool) -> CommandTraits {
+    CommandTraits(
+      retryOnSessionLoss: true, launchPolicy: .noApp,
+      retainsJournalResponseJson: retainsJournalResponseJson
+    )
+  }
 
   /// The runner's own lifecycle: no session app is brought forward, and no mutation is proven.
-  static let runnerLifecycle = CommandTraits(launchPolicy: .noApp)
+  static let runnerLifecycle = CommandTraits(launchPolicy: .noApp, retainsJournalResponseJson: true)
 
   /// Device state the runner sets from its own process, such as the pasteboard: no app is brought
   /// forward, since the state belongs to the device rather than to the session app, and no UI
   /// mutation is proven.
-  static let deviceState = CommandTraits(launchPolicy: .noApp)
+  static let deviceState = CommandTraits(launchPolicy: .noApp, retainsJournalResponseJson: true)
 
   /// Commands hosted by the surface that already has focus, which no activation may cancel. A
   /// hardware press belongs to the system rather than to the session app, and an alert answers from
   /// the modal where it sits; both mutate.
   static let presentedSurfaceMutation = CommandTraits(
     launchPolicy: .presentedSurface,
-    convertsRecordedFailure: true
+    convertsRecordedFailure: true,
+    retainsJournalResponseJson: true
   )
 
   /// `alert get` changes nothing, so it is the one alert action that may be replayed.
   static let presentedSurfaceQuery = CommandTraits(
     retryOnSessionLoss: true,
-    launchPolicy: .presentedSurface
+    launchPolicy: .presentedSurface,
+    retainsJournalResponseJson: true
   )
 }
 
@@ -179,8 +199,8 @@ extension CommandTraits {
 
 extension Command {
   /// Whether arriving at the prepared command path invalidates a remembered text-entry tap. Not a
-  /// fifth trait: everywhere but the two owner commands, it is having a mutation to prove that makes
-  /// the witness stale, so this reads `convertsRecordedFailure` and that set rather than declaring a
+  /// separate trait: everywhere but the two owner commands, having a mutation to prove makes the
+  /// witness stale, so this reads `convertsRecordedFailure` and that set rather than declaring a
   /// fact no command would answer for itself (#2890 review). `executeOnMainPrepared` is its only
   /// consumer, and the exhaustive table test pins the answer for every command.
   var invalidatesRememberedTextEntryTap: Bool {
@@ -243,13 +263,19 @@ extension Command {
          .keyboardDismiss, .keyboardReturn, .sequence, .gesture:
       return .interaction
 
-    case .findText, .readText, .snapshot, .gestureViewport:
-      return .appRead
+    case .findText, .readText, .gestureViewport:
+      return .appRead(retainsJournalResponseJson: true)
+
+    case .snapshot:
+      return .appRead(retainsJournalResponseJson: false)
 
     // appState reads the session app's XCUIApplication.state; bringing no app forward is what makes
     // its answer the state the app is in, not the one a repair leaves.
-    case .screenshot, .status, .appState:
-      return .runnerCaptureRead
+    case .status, .appState:
+      return .runnerCaptureRead(retainsJournalResponseJson: true)
+
+    case .screenshot:
+      return .runnerCaptureRead(retainsJournalResponseJson: false)
 
     case .alert:
       return (action ?? "get").lowercased() == "get"

apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift

@@ -153,19 +153,22 @@ extension RunnerTests {
     let retryOnSessionLoss: Bool
     let launchPolicy: CommandLaunchPolicy
     let convertsRecordedFailure: Bool
+    let retainsJournalResponseJson: Bool
   }
 
   private func expectation(
     interaction: Bool,
     retry: Bool,
     launch: CommandLaunchPolicy,
-    converts: Bool
+    converts: Bool,
+    retains: Bool
   ) -> ExpectedTraits {
     ExpectedTraits(
       isInteraction: interaction,
       retryOnSessionLoss: retry,
       launchPolicy: launch,
-      convertsRecordedFailure: converts
+      convertsRecordedFailure: converts,
+      retainsJournalResponseJson: retains
     )
   }
 
@@ -186,6 +189,11 @@ extension RunnerTests {
       expectation.convertsRecordedFailure,
       "\(request) convertsRecordedFailure"
     )
+    XCTAssertEqual(
+      traits.retainsJournalResponseJson,
+      expectation.retainsJournalResponseJson,
+      "\(request) retainsJournalResponseJson"
+    )
   }
 
   /// The commands the merge-base classified read-only, copied from its `CommandType.traits`
@@ -244,54 +252,54 @@ extension RunnerTests {
   /// a concrete launch case, so re-pointing a command at another policy fails that row.
   func testEveryCommandDeclaresEveryRunnerSideDecisionTogether() throws {
     let table: [(CommandType, ExpectedTraits)] = [
-      (.tap, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.mouseClick, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true)),
-      (.longPress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.drag, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.remotePress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.type, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.swipe, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.scroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.desktopScroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.findText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false)),
+      (.tap, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.mouseClick, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.longPress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.drag, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.remotePress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.type, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.swipe, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.scroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.desktopScroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.findText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true)),
       (
         .querySelector,
-        expectation(interaction: false, retry: false, launch: .existingApp, converts: true)
+        expectation(interaction: false, retry: false, launch: .existingApp, converts: true, retains: true)
       ),
-      (.readText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false)),
-      (.snapshot, expectation(interaction: false, retry: true, launch: .existingApp, converts: false)),
-      (.screenshot, expectation(interaction: false, retry: true, launch: .noApp, converts: false)),
-      (.backInApp, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.backSystem, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.home, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true)),
-      (.rotate, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.appSwitcher, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
+      (.readText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true)),
+      (.snapshot, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: false)),
+      (.screenshot, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: false)),
+      (.backInApp, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.backSystem, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.home, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.rotate, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.appSwitcher, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
       (
         .actionButton,
-        expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true)
+        expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true)
       ),
-      (.keyboardDismiss, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.keyboardReturn, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
+      (.keyboardDismiss, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.keyboardReturn, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
       (
         .alert,
-        expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false)
+        expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true)
       ),
-      (.sequence, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
-      (.gesture, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true)),
+      (.sequence, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.gesture, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)),
       (
         .gestureViewport,
-        expectation(interaction: false, retry: true, launch: .existingApp, converts: false)
+        expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true)
       ),
-      (.recordStart, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true)),
-      (.recordStop, expectation(interaction: false, retry: false, launch: .noApp, converts: false)),
-      (.status, expectation(interaction: false, retry: true, launch: .noApp, converts: false)),
-      (.uptime, expectation(interaction: false, retry: false, launch: .noApp, converts: false)),
-      (.appState, expectation(interaction: false, retry: true, launch: .noApp, converts: false)),
-      (.pasteboardWrite, expectation(interaction: false, retry: false, launch: .noApp, converts: false)),
-      (.activate, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true)),
-      (.terminate, expectation(interaction: false, retry: false, launch: .noApp, converts: false)),
-      (.targetReset, expectation(interaction: false, retry: false, launch: .noApp, converts: false)),
-      (.shutdown, expectation(interaction: false, retry: false, launch: .noApp, converts: false))
+      (.recordStart, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.recordStop, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)),
+      (.status, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: true)),
+      (.uptime, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)),
+      (.appState, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: true)),
+      (.pasteboardWrite, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)),
+      (.activate, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)),
+      (.terminate, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)),
+      (.targetReset, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)),
+      (.shutdown, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true))
     ]
     for (type, rowExpectation) in table {
       let request = #"{"command":"\#(type.rawValue)"}"#
@@ -314,15 +322,15 @@ extension RunnerTests {
     // The one payload-dependent command settles each fact per action: `get` changes nothing and may
     // be replayed, while `accept` and `dismiss` mutate and must not be.
     let alertCases: [(action: String?, expectation: ExpectedTraits)] = [
-      (nil, expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false)),
-      ("get", expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false)),
+      (nil, expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true)),
+      ("get", expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true)),
       (
         "accept",
-        expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true)
+        expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true)
       ),
       (
         "dismiss",
-        expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true)
+        expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true)
       )
     ]
     for alertCase in alertCases {