From b07712f3393370c13a20f3124db1645384db3286 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 1 Oct 2026 12:35:55 +0200 Subject: [PATCH] refactor(apple): declare journal retention in command traits --- .../RunnerTests+CommandJournal.swift | 15 +-- .../RunnerTests+Models.swift | 62 +++++++++---- .../UnitTests/RunnerTests+ModelsTests.swift | 92 ++++++++++--------- 3 files changed, 95 insertions(+), 74 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index e3822f1968..18ac85605e 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/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 - } - } } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift index 3060c0510f..5a8f63169a 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift +++ b/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,18 +125,26 @@ 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 @@ -140,33 +152,41 @@ fileprivate extension CommandTraits { /// 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" diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift index a500e1dc05..5104e61801 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift +++ b/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 {