From 4d7c3da495578be9138ae946e5857f7c3988c31a Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sat, 26 Sep 2026 18:17:12 +0900 Subject: [PATCH] Connect deferred updates and eight-hour Codex checks --- Docs/architecture.md | 20 + README.md | 21 + .../PreviewCodexReviewUpdateBackend.swift | 82 +++ .../ReviewMonitorContentPreview.swift | 29 + .../ReviewMonitorSplitViewController.swift | 51 +- .../ReviewUI/ReviewMonitorUpdatePreview.swift | 48 ++ Tests/ReviewUITests/ReviewUIUpdateTests.swift | 37 +- .../CodexCommandUpdateChecker.swift | 32 +- .../CodexReviewMonitorApp.swift | 71 +-- .../ReviewMonitorCodexUpdater.swift | 315 ++++------ .../ReviewMonitorSettingsWindow.swift | 157 ++++- .../CodexCommandUpdateCheckerTests.swift | 32 +- .../CodexReviewMonitorCITests.swift | 12 +- .../ReviewMonitorCodexUpdaterTests.swift | 583 ++++++++---------- 14 files changed, 861 insertions(+), 629 deletions(-) create mode 100644 Sources/CodexReview/Store/PreviewCodexReviewUpdateBackend.swift create mode 100644 Sources/ReviewUI/ReviewMonitorUpdatePreview.swift diff --git a/Docs/architecture.md b/Docs/architecture.md index c4d46cca..7f2a4e23 100644 --- a/Docs/architecture.md +++ b/Docs/architecture.md @@ -35,6 +35,26 @@ ReviewMonitor composes `ReviewUI` and `CodexReviewMCPServer` over one `CodexReviewHost` to `CodexReviewAppServer`; dependencies do not point back toward the app or UI. +### Codex Updates + +`CodexReviewStore.updateCodex(when:install:)` owns the update operation, its +observable progress, review dispatch suspension, and runtime replacement. It +retains MCP sessions and accepted jobs, joins existing execution and cleanup, +then resumes dispatch after runtime publication. Repeated update requests join +the same operation. Runtime recovery confirms the owned process has closed +without treating a recorded close error as permanent evidence that it is live. + +The app's `ReviewMonitorCodexUpdater` owns update checks and their results. The +sidebar receives its availability projection, and Settings uses the same +instance. A continuous clock anchors automatic checks to launch and eight-hour +boundaries; manual checks share in-flight work without moving that anchor. +Checks due during installation are covered by one post-update check. + +Application termination stops read-only checking, shuts down the Store, and +joins startup before replying to AppKit. The Store can cancel a deferred update +or wait for an installation already in progress. Codex updates do not use an +application relaunch helper. + ## CodexReview `CodexReviewStore` is the single source of truth for review, runtime, auth, diff --git a/README.md b/README.md index f1c1a1b0..d25223d1 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,27 @@ visible. - `codex app-server` runs behind CodexReviewMonitor as the live review backend. - `~/.codex_review` is the dedicated Codex home used by CodexReviewMonitor. +## Codex Updates + +ReviewMonitor checks the selected Codex installation at launch and every eight +hours. Use **Settings → Updates → Check for Updates** for a manual check and its +last-check time. Manual checks do not change the automatic schedule and do not +install an update. Unsupported installations and failed checks are reported +separately from **Up to Date**. Automatic installation currently supports the +stable Homebrew Codex cask. + +When an update is available, choose **Update** in the sidebar toolbar. During a +review, **Update After Reviews** lets current reviews finish and queues new +requests inside ReviewMonitor. **Stop Reviews and Update** cancels current +reviews and updates immediately. ReviewMonitor and its MCP sessions stay open; +queued requests resume after Codex restarts, without being resubmitted. + +If updating fails but Codex can restart, queued reviews resume and the error +remains visible in Settings. If Codex cannot restart, the queue is retained and +**Retry** in the sidebar attempts runtime recovery without reinstalling. Explicitly +quitting the app cancels queued reviews; an installation already in progress is +allowed to finish before the app exits. + ## Timeout Setup Long reviews can exceed the default MCP client timeout. `codex mcp add` does diff --git a/Sources/CodexReview/Store/PreviewCodexReviewUpdateBackend.swift b/Sources/CodexReview/Store/PreviewCodexReviewUpdateBackend.swift new file mode 100644 index 00000000..6854210e --- /dev/null +++ b/Sources/CodexReview/Store/PreviewCodexReviewUpdateBackend.swift @@ -0,0 +1,82 @@ +import Foundation + +@MainActor +package final class PreviewCodexReviewUpdateBackend: PreviewCodexReviewStoreBackend { + package let started = PreviewCodexUpdateGate() + package var completesReviews = false + package var failsNextPreparation = false + private var mailboxes: [String: BackendReviewEventMailbox] = [:] + + package init() { + super.init(seed: .init(initialAccounts: [CodexAccount(email: "reviewer@example.com")])) + } + + package override func prepareRuntime(generation: ReviewRuntimeGeneration, purpose: ReviewRuntimeTransitionPurpose) async throws -> PreparedRuntime { + if failsNextPreparation { + failsNextPreparation = false + throw CodexReviewAPI.Error.io("Codex could not restart after the update. Try again.") + } + let account = CodexReviewBackendModel.Account.Snapshot(id: .init("reviewer@example.com"), label: "reviewer@example.com", isActive: true) + return PreparedRuntime( + snapshot: .init(authentication: .init(accounts: [account], activeAccountID: account.id), settings: currentSettingsSnapshot), + handle: UpdatePreviewRuntime( + onActivate: { [weak self] in self?.isActive = true }, + onClose: { [weak self] in self?.isActive = false } + ) + ) + } + + package override func startReview(_ request: CodexReviewBackendModel.Review.Start, admission: ReviewStartAdmission) async throws -> BackendReviewAttempt { + let run = CodexReviewBackendModel.Review.Run(threadID: request.jobID, turnID: request.jobID) + try await admission.admitThreadStartDispatch() + try await admission.recordPreparedThread(run) + try await admission.admitReviewStartDispatch(for: run) + try await admission.recordActiveRun(run) + let mailbox = BackendReviewEventMailbox() + mailboxes[run.threadID] = mailbox + await started.open() + if completesReviews { await mailbox.append(.completed(summary: "Review completed.", result: "No findings.")) } + return .init(run: run, events: mailbox) + } + + package override func interruptReview(_ admission: ReviewInterruptRequestAdmission, reason: CodexReviewBackendModel.CancellationReason) async throws { + await mailboxes[admission.run.threadID]?.append(.cancelled(reason.message)) + } + + package override func cleanupReview(_ run: CodexReviewBackendModel.Review.Run) async { + await mailboxes.removeValue(forKey: run.threadID)?.finish() + } +} + +@MainActor +private final class UpdatePreviewRuntime: RuntimeLifecycleHandle { + private var closed = false + private let onActivate: @MainActor () -> Void + private let onClose: @MainActor () -> Void + init(onActivate: @escaping @MainActor () -> Void, onClose: @escaping @MainActor () -> Void) { + self.onActivate = onActivate + self.onClose = onClose + } + func activate() async throws { onActivate() } + func closeAdmission() {} + func close(purpose: ReviewRuntimeTransitionPurpose) async throws { closed = true; onClose() } + func waitUntilClosed() async throws { + if closed == false { throw CancellationError() } + } +} + +package actor PreviewCodexUpdateGate { + private var opened = false + private var waiters: [CheckedContinuation] = [] + package init() {} + package func wait() async { + if opened { return } + await withCheckedContinuation { waiters.append($0) } + } + package func open() { + opened = true + let pending = waiters + waiters = [] + for waiter in pending { waiter.resume() } + } +} diff --git a/Sources/ReviewUI/ReviewMonitorContentPreview.swift b/Sources/ReviewUI/ReviewMonitorContentPreview.swift index 4307c141..4495c26e 100644 --- a/Sources/ReviewUI/ReviewMonitorContentPreview.swift +++ b/Sources/ReviewUI/ReviewMonitorContentPreview.swift @@ -85,3 +85,32 @@ private struct ReviewMonitorContentPreviewHost: NSViewControllerRepresentable { } } #endif + +#if DEBUG +#Preview("Update Waiting") { ReviewMonitorUpdateScenarioPreview(scenario: .waiting) } +#Preview("Updating Codex") { ReviewMonitorUpdateScenarioPreview(scenario: .installing) } +#Preview("Update Recovery") { ReviewMonitorUpdateScenarioPreview(scenario: .failed) } +#Preview("Reviews Resumed") { ReviewMonitorUpdateScenarioPreview(scenario: .resumed) } + +@MainActor +private struct ReviewMonitorUpdateScenarioPreview: View { + let scenario: ReviewMonitorUpdatePreview.Scenario + @State private var preview = ReviewMonitorUpdatePreview() + + var body: some View { + UpdateController(store: preview.store, available: scenario != .resumed) + .frame(width: 860, height: 560) + .task { await preview.run(scenario) } + .onDisappear { Task { await preview.stop() } } + } + + private struct UpdateController: NSViewControllerRepresentable { + let store: CodexReviewStore + let available: Bool + func makeNSViewController(context: Context) -> ReviewMonitorRootViewController { + makeReviewMonitorPreviewContentViewControllerForPreview(previewStore: store, isCodexUpdateAvailable: available) + } + func updateNSViewController(_ controller: ReviewMonitorRootViewController, context: Context) {} + } +} +#endif diff --git a/Sources/ReviewUI/ReviewMonitorSplitViewController.swift b/Sources/ReviewUI/ReviewMonitorSplitViewController.swift index c956a29c..721d90ff 100644 --- a/Sources/ReviewUI/ReviewMonitorSplitViewController.swift +++ b/Sources/ReviewUI/ReviewMonitorSplitViewController.swift @@ -2,7 +2,7 @@ import AppKit import Combine import Foundation import ObservationBridge -import CodexReview +@_spi(ApplicationHostSupport) import CodexReview @MainActor final class ReviewMonitorSplitViewController: NSSplitViewController, NSToolbarDelegate { @@ -168,7 +168,7 @@ final class ReviewMonitorSplitViewController: NSSplitViewController, NSToolbarDe toolbarMembershipObservation = withPortableContinuousObservation { [weak self, uiState] _ in let sidebarSelection = uiState.sidebarSelection let isAuthenticating = uiState.auth.isAuthenticating - let isCodexUpdateAvailable = uiState.isCodexUpdateAvailable + let isCodexUpdateAvailable = self?.codexUpdatePresentation != nil guard let self else { return } @@ -179,6 +179,7 @@ final class ReviewMonitorSplitViewController: NSSplitViewController, NSToolbarDe isCodexUpdateAvailable: isCodexUpdateAvailable ) self.applyToolbarItemIdentifiers(identifiers) + self.synchronizeCodexUpdatePresentation() } windowTitleObservation = withPortableContinuousObservation { [weak self, uiState] _ in @@ -248,7 +249,7 @@ final class ReviewMonitorSplitViewController: NSSplitViewController, NSToolbarDe sidebarSelection: uiState.sidebarSelection, isSidebarCollapsed: isSidebarCollapsed, isAuthenticating: uiState.auth.isAuthenticating, - isCodexUpdateAvailable: uiState.isCodexUpdateAvailable + isCodexUpdateAvailable: codexUpdatePresentation != nil ) } @@ -332,9 +333,43 @@ final class ReviewMonitorSplitViewController: NSSplitViewController, NSToolbarDe menuItem.target = self menuItem.state = .on item.menuFormRepresentation = menuItem + applyCodexUpdatePresentation(to: item) return item } + private var codexUpdatePresentation: (title: String, help: String, enabled: Bool)? { + switch store.codexUpdateState { + case .waitingForReviews: + return ("Waiting", "Current reviews will finish before Codex is updated. New requests are queued.", false) + case .stoppingRuntime, .installing: + return ("Updating", "Codex is being updated. New requests are queued.", false) + case .restarting: + return ("Restarting", "Preparing Codex to resume queued reviews.", false) + case .failed(let message): + if case .failed = store.serverState { return ("Retry", message, true) } + return uiState.isCodexUpdateAvailable ? ("Update", message, true) : nil + case .idle: + return uiState.isCodexUpdateAvailable ? ("Update", "A Codex update is available", true) : nil + } + } + + private func synchronizeCodexUpdatePresentation() { + if let item = toolbar?.items.first(where: { $0.itemIdentifier == Self.sidebarUpdateToolbarItemIdentifier }) { + applyCodexUpdatePresentation(to: item) + } + } + + private func applyCodexUpdatePresentation(to item: NSToolbarItem) { + guard let presentation = codexUpdatePresentation, let button = item.view as? NSButton else { return } + button.title = presentation.title + button.isEnabled = presentation.enabled + button.setAccessibilityLabel(store.codexUpdateState == .waitingForReviews ? "Waiting to Update Codex" : presentation.title + " Codex") + item.label = presentation.title + item.toolTip = presentation.help + item.menuFormRepresentation?.title = presentation.title + " Codex" + item.menuFormRepresentation?.isEnabled = presentation.enabled + } + private func handleSidebarPickerSelection(_ selection: SidebarPickerSelection) { guard uiState.sidebarSelection != selection else { toggleSidebar(nil) @@ -439,7 +474,7 @@ final class ReviewMonitorSplitViewController: NSSplitViewController, NSToolbarDe sidebarSelection: uiState.sidebarSelection, isSidebarCollapsed: isSidebarCollapsed, isAuthenticating: uiState.auth.isAuthenticating, - isCodexUpdateAvailable: uiState.isCodexUpdateAvailable + isCodexUpdateAvailable: codexUpdatePresentation != nil )) } @@ -457,9 +492,7 @@ final class ReviewMonitorSplitViewController: NSSplitViewController, NSToolbarDe @objc private func handleCodexUpdate(_ sender: Any?) { (sender as? NSButton)?.state = .on - guard uiState.isCodexUpdateAvailable else { - return - } + guard codexUpdatePresentation?.enabled == true else { return } uiState.isCodexUpdateAvailable = false notificationCenter.post(name: ReviewMonitorCodexUpdateNotification.requested, object: nil) } @@ -563,6 +596,10 @@ extension ReviewMonitorSplitViewController { (sidebarUpdateToolbarItemForTesting?.view as? NSButton)?.accessibilityLabel() } + var sidebarUpdateToolbarIsEnabledForTesting: Bool { + (sidebarUpdateToolbarItemForTesting?.view as? NSButton)?.isEnabled == true + } + var sidebarUpdateToolbarShowsSelectedBackgroundForTesting: Bool { (sidebarUpdateToolbarItemForTesting?.view as? NSButton)?.state == .on } diff --git a/Sources/ReviewUI/ReviewMonitorUpdatePreview.swift b/Sources/ReviewUI/ReviewMonitorUpdatePreview.swift new file mode 100644 index 00000000..7019d638 --- /dev/null +++ b/Sources/ReviewUI/ReviewMonitorUpdatePreview.swift @@ -0,0 +1,48 @@ +import Foundation +@_spi(ApplicationHostSupport) import CodexReview + +@_spi(PreviewSupport) +@MainActor +public final class ReviewMonitorUpdatePreview { + public enum Scenario: Sendable { case waiting, installing, failed, resumed } + public let store: CodexReviewStore + private let backend: PreviewCodexReviewUpdateBackend + private let installation = PreviewCodexUpdateGate() + private var reviews: [Task] = [] + + public init() { + let backend = PreviewCodexReviewUpdateBackend() + self.backend = backend + store = CodexReviewStore.makeTestingStore(backend: backend) + } + + public func run(_ scenario: Scenario) async { + await store.start() + let store = store + if scenario == .waiting { + reviews.append(Task { try await store.startReview( + sessionID: "preview", request: .init(cwd: "/Preview/Current Review", target: .baseBranch("main")) + ) }) + await backend.started.wait() + } + backend.completesReviews = scenario == .resumed + reviews.append(Task { try await store.startReview( + sessionID: "preview", request: .init(cwd: "/Preview/Queued Review", target: .uncommittedChanges) + ) }) + try? await store.updateCodex(when: scenario == .waiting ? .afterCurrentReviews : .immediately) { [self] in + if scenario == .installing { await installation.wait() } + if scenario == .failed { backend.failsNextPreparation = true } + } + if scenario == .resumed { + for review in reviews { _ = try? await review.value } + } + } + + public func stop() async { + await installation.open() + await store.shutdown() + for review in reviews { _ = try? await review.value } + reviews = [] + } +} + diff --git a/Tests/ReviewUITests/ReviewUIUpdateTests.swift b/Tests/ReviewUITests/ReviewUIUpdateTests.swift index 5e7aec03..af905c05 100644 --- a/Tests/ReviewUITests/ReviewUIUpdateTests.swift +++ b/Tests/ReviewUITests/ReviewUIUpdateTests.swift @@ -1,10 +1,45 @@ import AppKit import Testing -@_spi(Testing) @testable import CodexReview +@_spi(Testing) @_spi(ApplicationHostSupport) @testable import CodexReview @_spi(PreviewSupport) @testable import ReviewUI @MainActor extension ReviewUITests { + @Test(arguments: [ReviewMonitorUpdatePreview.Scenario.waiting, .installing, .failed, .resumed]) + func updatePreviewUsesStoreTransitionsInTheSidebar(scenario: ReviewMonitorUpdatePreview.Scenario) async throws { + let preview = ReviewMonitorUpdatePreview() + let harness = makeWindowHarness(store: preview.store) + defer { harness.window.close() } + harness.viewController.splitViewItems.first?.isCollapsed = false + let run = Task { await preview.run(scenario) } + do { + try await waitForCondition { + switch scenario { + case .waiting: return harness.viewController.sidebarUpdateToolbarTitleForTesting == "Waiting" && preview.store.jobs.contains { $0.core.lifecycle.status == .queued } + case .installing: return harness.viewController.sidebarUpdateToolbarTitleForTesting == "Updating" && preview.store.jobs.contains { $0.core.lifecycle.status == .queued } + case .failed: return harness.viewController.sidebarUpdateToolbarTitleForTesting == "Retry" + case .resumed: + return preview.store.jobs.contains { $0.isTerminal } && preview.store.codexUpdateState == .idle + } + } + switch scenario { + case .waiting, .installing: + #expect(harness.viewController.sidebarUpdateToolbarIsEnabledForTesting == false) + #expect(preview.store.jobs.contains { $0.core.lifecycle.status == .queued }) + case .failed: + #expect(harness.viewController.sidebarUpdateToolbarIsEnabledForTesting) + case .resumed: + #expect(harness.viewController.sidebarUpdateToolbarItemIsHiddenForTesting) + } + } catch { + await preview.stop() + await run.value + throw error + } + await preview.stop() + await run.value + } + @Test func updateAvailabilityNotificationControlsToolbarPresentation() async throws { let notificationCenter = NotificationCenter() let harness = makeWindowHarness( diff --git a/Tools/ReviewMonitor/CodexReviewMonitor/CodexCommandUpdateChecker.swift b/Tools/ReviewMonitor/CodexReviewMonitor/CodexCommandUpdateChecker.swift index 1a067ff3..9e19bda8 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor/CodexCommandUpdateChecker.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitor/CodexCommandUpdateChecker.swift @@ -7,8 +7,8 @@ struct CodexCommandUpdatePlan: Equatable, Sendable { } enum CodexCommandUpdateCheckResult: Equatable, Sendable { - case disabled - case unavailable + case upToDate + case unavailable(String) case available(CodexCommandUpdatePlan) } @@ -59,7 +59,7 @@ struct CodexCommandUpdateChecker: Sendable { configuredPath: runtimePreferences.codexExecutablePath, environment: sourceEnvironment ) else { - return .unavailable + return .unavailable("Automatic updates are not supported for this Codex installation.") } let codexHomeURL = runtimePreferences.codexHomePath.map { URL(fileURLWithPath: $0, isDirectory: true) @@ -75,28 +75,17 @@ struct CodexCommandUpdateChecker: Sendable { environment ) let report = try JSONDecoder().decode(DoctorReport.self, from: output) - guard report.schemaVersion == 1 else { - throw CodexCommandUpdateCheckError.unsupportedSchema(report.schemaVersion) - } guard let update = report.checks["updates.status"] else { throw CodexCommandUpdateCheckError.missingUpdateStatus } - switch try update.scalar(named: "check for update on startup") { - case "false": - return .disabled - case "true": - break - case let value: - throw CodexCommandUpdateCheckError.invalidDetail( - name: "check for update on startup", - value: value - ) + if case .scalar(let message) = update.details["latest version probe"] { + throw CodexCommandUpdateCheckError.probeFailed(message) } switch try update.scalar(named: "latest version status") { case "current version is not older": - return .unavailable + return .upToDate case "newer version is available": break case let value: @@ -108,7 +97,7 @@ struct CodexCommandUpdateChecker: Sendable { guard try update.scalar(named: "update action") == "brew upgrade --cask codex" else { - return .unavailable + return .unavailable("This Codex installation does not provide a supported automatic update action.") } return .available(.init( executableURL: installation.launcherURL, @@ -421,7 +410,6 @@ private final class CancellableDoctorProcess: @unchecked Sendable { } private struct DoctorReport: Decodable { - let schemaVersion: Int let checks: [String: DoctorCheck] } @@ -454,7 +442,7 @@ private enum DoctorDetail: Decodable { } private enum CodexCommandUpdateCheckError: LocalizedError { - case unsupportedSchema(Int) + case probeFailed(String) case missingUpdateStatus case missingDetail(String) case nonScalarDetail(String) @@ -462,8 +450,8 @@ private enum CodexCommandUpdateCheckError: LocalizedError { var errorDescription: String? { switch self { - case .unsupportedSchema(let version): - "Codex returned unsupported doctor schema version \(version)." + case .probeFailed(let message): + "The latest Codex version could not be checked. \(message)" case .missingUpdateStatus: "Codex did not include updates.status in its doctor report." case .missingDetail(let name): diff --git a/Tools/ReviewMonitor/CodexReviewMonitor/CodexReviewMonitorApp.swift b/Tools/ReviewMonitor/CodexReviewMonitor/CodexReviewMonitorApp.swift index ba771551..de1dfda0 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor/CodexReviewMonitorApp.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitor/CodexReviewMonitorApp.swift @@ -79,10 +79,6 @@ struct ReviewMonitorLaunchContext: Sendable { && isolatedTestConfiguration?.validationFailure == nil } - var reportsFailedCodexUpdate: Bool { - arguments.contains(ReviewMonitorApplicationRelauncher.failedUpdateLaunchArgument) - } - fileprivate var isolatedTestConfiguration: ReviewMonitorIsolatedTestConfiguration? { ReviewMonitorLaunchEnvironment.isolatedTestConfiguration( environment: environment, @@ -411,9 +407,9 @@ final class ReviewMonitorLifecycleController { let store = store let prepareForApplicationTermination = prepareForApplicationTermination terminationTask = Task { @MainActor [weak self] in - await launchTask?.value await prepareForApplicationTermination() await store.shutdown() + await launchTask?.value self?.terminationTask = nil application.replyToApplicationShouldTerminate(true) } @@ -454,7 +450,7 @@ struct ReviewMonitorAppComposition { CodexReviewStore, @escaping @MainActor () -> Void ) -> NSWindowController - var makeSettingsWindowController: () -> NSWindowController + var makeSettingsWindowController: (ReviewMonitorCodexUpdater?) -> NSWindowController init( makeStore: @escaping ( @@ -477,9 +473,10 @@ struct ReviewMonitorAppComposition { CodexReviewStore, @escaping @MainActor () -> Void ) -> NSWindowController, - makeSettingsWindowController: @escaping () -> NSWindowController = { + makeSettingsWindowController: @escaping (ReviewMonitorCodexUpdater?) -> NSWindowController = { updater in ReviewMonitorSettingsWindowController( - runtimePreferencesStore: CodexReviewRuntime.UserDefaultsPreferencesStore() + runtimePreferencesStore: CodexReviewRuntime.UserDefaultsPreferencesStore(), + updater: updater ) } ) { @@ -572,9 +569,10 @@ struct ReviewMonitorAppComposition { showSettings: showSettings ) }, - makeSettingsWindowController: { + makeSettingsWindowController: { updater in ReviewMonitorSettingsWindowController( - runtimePreferencesStore: runtimePreferencesStore + runtimePreferencesStore: runtimePreferencesStore, + updater: updater ) } ) @@ -601,7 +599,7 @@ final class ReviewMonitorAppDelegate: NSObject, NSApplicationDelegate { lazy var lifecycle: ReviewMonitorLifecycleController = { let lifecycle = composition.makeLifecycleController(store, launchContext) lifecycle.setApplicationTerminationPreparation { [weak self] in - await self?.codexUpdater?.stopAndWait() + await self?.codexUpdater?.stopChecking() } return lifecycle }() @@ -611,6 +609,7 @@ final class ReviewMonitorAppDelegate: NSObject, NSApplicationDelegate { } let store = store return ReviewMonitorCodexUpdater( + store: store, check: checker.check, publishAvailability: { available in NotificationCenter.default.post( @@ -621,14 +620,8 @@ final class ReviewMonitorAppDelegate: NSObject, NSApplicationDelegate { ] ) }, - prepareForUpdate: { [weak self] in - guard let self else { - return false - } - return await prepareForCodexUpdate(store: store) - }, - requestApplicationTermination: { - NSApp.terminate(nil) + chooseTiming: { [weak self] in + self?.codexUpdateTiming(store: store) }, presentFailure: { [weak self] title, message in self?.presentCodexUpdateFailure(title: title, message: message) @@ -643,7 +636,7 @@ final class ReviewMonitorAppDelegate: NSObject, NSApplicationDelegate { presentationAnchorSource.window = windowController.window return windowController }() - lazy var settingsWindowController = composition.makeSettingsWindowController() + lazy var settingsWindowController = composition.makeSettingsWindowController(codexUpdater) override init() { launchContextProvider = { @@ -686,12 +679,6 @@ final class ReviewMonitorAppDelegate: NSObject, NSApplicationDelegate { if launchMode == .application { NSApp.activate(ignoringOtherApps: true) } - if launchContext.reportsFailedCodexUpdate { - presentCodexUpdateFailure( - title: "Codex Could Not Be Updated", - message: "ReviewMonitor restarted without updating Codex. Run `codex update` in Terminal for details." - ) - } startCodexUpdateMonitoring() lifecycle.applicationDidFinishLaunching( launchMode: launchMode @@ -723,22 +710,24 @@ final class ReviewMonitorAppDelegate: NSObject, NSApplicationDelegate { windowController.window?.makeKeyAndOrderFront(sender) } - private func prepareForCodexUpdate( - store: CodexReviewStore - ) async -> Bool { - if store.hasRunningJobs { - let alert = NSAlert() - alert.alertStyle = .warning - alert.messageText = "Stop Active Reviews and Update Codex?" - alert.informativeText = "Active and queued reviews will be cancelled. ReviewMonitor will restart after Codex is updated." - alert.addButton(withTitle: "Stop Reviews and Update").hasDestructiveAction = true - alert.addButton(withTitle: "Cancel") - guard alert.runModal() == .alertFirstButtonReturn else { - return false - } + private func codexUpdateTiming(store: CodexReviewStore) -> CodexReviewStore.CodexUpdateTiming? { + guard store.hasRunningJobs else { return .immediately } + let response = Self.makeCodexUpdateAlert().runModal() + switch response { + case .alertFirstButtonReturn: return .afterCurrentReviews + case .alertSecondButtonReturn: return .immediately + default: return nil } - await store.shutdown() - return true + } + + static func makeCodexUpdateAlert() -> NSAlert { + let alert = NSAlert() + alert.alertStyle = .warning + alert.messageText = "Update Codex After Reviews?" + alert.informativeText = "Let current reviews finish, or stop them to update now. New requests will wait in the queue. ReviewMonitor will stay open." + alert.addButton(withTitle: "Update After Reviews") + alert.addButton(withTitle: "Stop Reviews and Update").hasDestructiveAction = true + return alert } private func startCodexUpdateMonitoring() { diff --git a/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift b/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift index be71cc95..a006f622 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift @@ -1,189 +1,174 @@ -import AppKit import Foundation -import OSLog - -private let codexUpdateLogger = Logger( - subsystem: "CodexReviewMonitor", - category: "codex-update" -) +import Observation +@_spi(ApplicationHostSupport) import CodexReview @MainActor +@Observable final class ReviewMonitorCodexUpdater { - typealias Check = @MainActor @Sendable () async throws -> CodexCommandUpdateCheckResult - typealias Wait = @Sendable () async throws -> Void - typealias PublishAvailability = @MainActor (Bool) -> Void - typealias RunUpdate = @MainActor (CodexCommandUpdatePlan) async throws -> Void - typealias ScheduleRelaunch = @MainActor (_ reportsFailure: Bool) throws -> Void - typealias PresentFailure = @MainActor (_ title: String, _ message: String) -> Void + enum CheckState: Equatable { + case notChecked + case checking + case available(CodexCommandUpdatePlan) + case upToDate + case unavailable(String) + case failed(String) + } - private let check: Check - private let wait: Wait - private let publishAvailability: PublishAvailability - private let prepareForUpdate: @MainActor () async -> Bool - private let runUpdate: RunUpdate - private let scheduleRelaunch: ScheduleRelaunch - private let requestApplicationTermination: @MainActor () -> Void - private let presentFailure: PresentFailure - private var monitorTask: Task? + typealias Check = @MainActor @Sendable () async throws -> CodexCommandUpdateCheckResult + typealias RunUpdate = @MainActor @Sendable (CodexCommandUpdatePlan) async throws -> Void + typealias ChooseTiming = @MainActor () async -> CodexReviewStore.CodexUpdateTiming? + + private(set) var checkState = CheckState.notChecked + private(set) var lastCheckedAt: Date? + let store: CodexReviewStore + @ObservationIgnored private let check: Check + @ObservationIgnored private let now: @MainActor () -> ContinuousClock.Instant + @ObservationIgnored private let date: @MainActor () -> Date + @ObservationIgnored private let sleepUntil: @MainActor (ContinuousClock.Instant) async throws -> Void + @ObservationIgnored private let publishAvailability: @MainActor (Bool) -> Void + @ObservationIgnored private let chooseTiming: ChooseTiming + @ObservationIgnored private let runUpdate: RunUpdate + @ObservationIgnored private let presentFailure: @MainActor (String, String) -> Void + @ObservationIgnored private var monitorTask: Task? + @ObservationIgnored private var checkTask: Task? private var updateTask: Task? - private var availablePlan: CodexCommandUpdatePlan? + @ObservationIgnored private var stopping = false init( + store: CodexReviewStore, check: @escaping Check, - wait: @escaping Wait = { - try await Task.sleep(for: .seconds(20 * 60 * 60)) + now: @escaping @MainActor () -> ContinuousClock.Instant = { .now }, + date: @escaping @MainActor () -> Date = { .now }, + sleepUntil: @escaping @MainActor (ContinuousClock.Instant) async throws -> Void = { + try await ContinuousClock().sleep(until: $0) }, - publishAvailability: @escaping PublishAvailability, - prepareForUpdate: @escaping @MainActor () async -> Bool, + publishAvailability: @escaping @MainActor (Bool) -> Void, + chooseTiming: @escaping ChooseTiming, runUpdate: @escaping RunUpdate = ReviewMonitorCodexUpdateProcess.run, - scheduleRelaunch: @escaping ScheduleRelaunch = ReviewMonitorApplicationRelauncher.schedule, - requestApplicationTermination: @escaping @MainActor () -> Void, - presentFailure: @escaping PresentFailure + presentFailure: @escaping @MainActor (String, String) -> Void ) { + self.store = store self.check = check - self.wait = wait + self.now = now + self.date = date + self.sleepUntil = sleepUntil self.publishAvailability = publishAvailability - self.prepareForUpdate = prepareForUpdate + self.chooseTiming = chooseTiming self.runUpdate = runUpdate - self.scheduleRelaunch = scheduleRelaunch - self.requestApplicationTermination = requestApplicationTermination self.presentFailure = presentFailure } - func start() { - startMonitoring(checkImmediately: true) - } + var isBusy: Bool { checkState == .checking || updateTask != nil } - private func startMonitoring(checkImmediately: Bool) { - guard monitorTask == nil, updateTask == nil else { - return - } + func start() { + guard monitorTask == nil, stopping == false else { return } + let origin = now() monitorTask = Task { @MainActor [weak self] in - await self?.monitorForUpdate(checkImmediately: checkImmediately) - self?.monitorTask = nil + guard let self else { return } + while Task.isCancelled == false, stopping == false { + // Every installation ends with one check, including requests due during it. + if updateTask == nil { await performCheck() } + guard Task.isCancelled == false, stopping == false else { return } + let elapsed = max(0, origin.duration(to: now()).components.seconds) + let nextSlot = elapsed / (8 * 60 * 60) + 1 + let deadline = origin.advanced(by: .seconds(nextSlot * 8 * 60 * 60)) + do { try await sleepUntil(deadline) } catch { return } + } } } - func stopAndWait() async { - let monitorTask = monitorTask - let updateTask = updateTask + /// Stop read-only checking before Store shutdown cancels a deferred update or joins installation. + func stopChecking() async { + stopping = true monitorTask?.cancel() - self.monitorTask = nil + checkTask?.cancel() + await checkTask?.value await monitorTask?.value - await updateTask?.value - let successorMonitorTask = self.monitorTask - successorMonitorTask?.cancel() - self.monitorTask = nil - await successorMonitorTask?.value + monitorTask = nil } - func requestUpdate() { - guard updateTask == nil, - availablePlan != nil else { + func checkForUpdates() async { + guard stopping == false else { return } + if let updateTask { + await updateTask.value return } - publishAvailability(false) - let monitorTask = monitorTask - monitorTask?.cancel() - updateTask = Task { @MainActor [weak self] in - guard let self else { - return - } - await monitorTask?.value - let plan: CodexCommandUpdatePlan - do { - guard case .available(let currentPlan) = try await check() else { - availablePlan = nil - updateTask = nil - startMonitoring(checkImmediately: false) - return + await performCheck() + } + + @discardableResult + func requestUpdate() -> Task? { + guard stopping == false else { return nil } + if let updateTask { return updateTask } + let task = Task { @MainActor [weak self] in + guard let self else { return } + defer { updateTask = nil } + if case .failed = store.codexUpdateState, case .failed = store.serverState { + await store.start() + if stopping == false { + if case .failed(let message) = store.serverState { + presentFailure("Codex Could Not Restart", message) + } + await performCheck() } - plan = currentPlan - availablePlan = currentPlan - } catch { - updateTask = nil - publishAvailability(true) - startMonitoring(checkImmediately: false) - presentFailure( - "Codex Update Could Not Start", - "ReviewMonitor could not confirm that the Codex update is still available. \(error.localizedDescription)" - ) return } - guard await prepareForUpdate() else { - updateTask = nil - publishAvailability(true) - startMonitoring(checkImmediately: false) + await performCheck() + guard stopping == false else { return } + switch checkState { + case .failed(let message), .unavailable(let message): + presentFailure("Codex Update Could Not Start", message) return + default: break } - availablePlan = nil - let updateFailure: (any Error)? + guard case .available(let plan) = checkState, + let timing = await chooseTiming(), stopping == false else { return } do { - try await runUpdate(plan) - updateFailure = nil - } catch { - codexUpdateLogger.error( - "Codex update failed: \(error.localizedDescription, privacy: .public)" - ) - updateFailure = error - } - do { - try scheduleRelaunch(updateFailure != nil) - updateTask = nil - requestApplicationTermination() + let runUpdate = runUpdate + try await store.updateCodex(when: timing) { try await runUpdate(plan) } + } catch is CancellationError { } catch { - updateTask = nil - if let updateFailure { - presentFailure( - "Codex Could Not Be Updated", - "\(updateFailure.localizedDescription) ReviewMonitor also could not schedule an automatic restart. Quit and reopen the app. \(error.localizedDescription)" - ) - } else { - presentFailure( - "ReviewMonitor Could Not Restart", - "Codex was updated, but ReviewMonitor could not restart automatically. Quit and reopen the app. \(error.localizedDescription)" - ) + if stopping == false { + presentFailure("Codex Could Not Be Updated", error.localizedDescription) } } + if stopping == false { await performCheck() } } + updateTask = task + return task } - private func monitorForUpdate(checkImmediately: Bool) async { - if checkImmediately == false { - do { - try await wait() - } catch { - return - } + private func performCheck() async { + guard stopping == false else { return } + if let checkTask { + await checkTask.value + return } - while Task.isCancelled == false, updateTask == nil { + let previousState = checkState + checkState = .checking + publishAvailability(false) + let task = Task { @MainActor [self] in + defer { checkTask = nil } do { - switch try await check() { - case .disabled: - availablePlan = nil - publishAvailability(false) - return - case .unavailable: - availablePlan = nil - publishAvailability(false) - case .available(let plan): - availablePlan = plan - publishAvailability(true) + let result = try await check() + try Task.checkCancellation() + switch result { + case .available(let plan): checkState = .available(plan) + case .upToDate: checkState = .upToDate + case .unavailable(let message): checkState = .unavailable(message) } + lastCheckedAt = date() } catch is CancellationError { - return + checkState = previousState } catch { - codexUpdateLogger.error( - "Failed to check for Codex updates: \(error.localizedDescription, privacy: .public)" - ) - } - - do { - try await wait() - } catch { - return + checkState = .failed(error.localizedDescription) + lastCheckedAt = date() } + if case .available = checkState { publishAvailability(true) } + else { publishAvailability(false) } } + checkTask = task + await task.value } } @@ -225,57 +210,3 @@ private enum ReviewMonitorCodexUpdateProcessError: LocalizedError { } } } - -@MainActor -enum ReviewMonitorApplicationRelauncher { - nonisolated static let failedUpdateLaunchArgument = "--review-monitor-codex-update-failed" - - static func schedule(reportsFailure: Bool) throws { - let applicationURL = Bundle.main.bundleURL.standardizedFileURL - guard applicationURL.pathExtension == "app" else { - throw ReviewMonitorApplicationRelaunchError.invalidApplicationBundle( - applicationURL.path - ) - } - - let helper = Process() - helper.executableURL = URL(fileURLWithPath: "/bin/sh") - helper.arguments = helperArguments( - applicationURL: applicationURL, - processIdentifier: ProcessInfo.processInfo.processIdentifier, - reportsFailure: reportsFailure - ) - helper.environment = ProcessInfo.processInfo.environment - helper.currentDirectoryURL = FileManager.default.temporaryDirectory - helper.standardInput = FileHandle.nullDevice - helper.standardOutput = FileHandle.nullDevice - helper.standardError = FileHandle.nullDevice - try helper.run() - } - - static func helperArguments( - applicationURL: URL, - processIdentifier: pid_t, - reportsFailure: Bool - ) -> [String] { - [ - "-c", - "while /bin/kill -0 \"$1\" 2>/dev/null; do /bin/sleep 0.1; done; if [ -n \"$3\" ]; then exec /usr/bin/open \"$2\" --args \"$3\"; else exec /usr/bin/open \"$2\"; fi", - "reviewmonitor-relaunch", - String(processIdentifier), - applicationURL.path, - reportsFailure ? failedUpdateLaunchArgument : "", - ] - } -} - -private enum ReviewMonitorApplicationRelaunchError: LocalizedError { - case invalidApplicationBundle(String) - - var errorDescription: String? { - switch self { - case .invalidApplicationBundle(let path): - "ReviewMonitor cannot relaunch because its application bundle is invalid: \(path)" - } - } -} diff --git a/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorSettingsWindow.swift b/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorSettingsWindow.swift index 6838888c..856cb846 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorSettingsWindow.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorSettingsWindow.swift @@ -2,15 +2,20 @@ import AppKit import CodexReviewHost import Observation import SwiftUI +@_spi(ApplicationHostSupport) import CodexReview +@_spi(PreviewSupport) import ReviewUI @MainActor enum ReviewMonitorSettingsPane: String, CaseIterable { case runtime + case updates var label: String { switch self { case .runtime: "Runtime" + case .updates: + "Updates" } } @@ -18,15 +23,19 @@ enum ReviewMonitorSettingsPane: String, CaseIterable { switch self { case .runtime: "gearshape" + case .updates: + "arrow.triangle.2.circlepath" } } func tabViewItem( - runtimePreferencesStore: any CodexReviewRuntime.PreferencesStore - ) -> NSTabViewItem { - let viewController = makeViewController( - runtimePreferencesStore: runtimePreferencesStore - ) + runtimePreferencesStore: any CodexReviewRuntime.PreferencesStore, + updater: ReviewMonitorCodexUpdater? + ) -> NSTabViewItem? { + guard let viewController = makeViewController( + runtimePreferencesStore: runtimePreferencesStore, + updater: updater + ) else { return nil } let tabViewItem = NSTabViewItem(viewController: viewController) tabViewItem.label = label tabViewItem.identifier = rawValue @@ -38,25 +47,28 @@ enum ReviewMonitorSettingsPane: String, CaseIterable { } private func makeViewController( - runtimePreferencesStore: any CodexReviewRuntime.PreferencesStore - ) -> NSViewController { + runtimePreferencesStore: any CodexReviewRuntime.PreferencesStore, + updater: ReviewMonitorCodexUpdater? + ) -> NSViewController? { switch self { case .runtime: ReviewMonitorRuntimeSettingsViewController( runtimePreferencesStore: runtimePreferencesStore ) + case .updates: + updater.map { ReviewMonitorUpdateSettingsViewController(updater: $0) } } } } @MainActor final class ReviewMonitorSettingsWindowController: NSWindowController { - init(runtimePreferencesStore: any CodexReviewRuntime.PreferencesStore) { + init(runtimePreferencesStore: any CodexReviewRuntime.PreferencesStore, updater: ReviewMonitorCodexUpdater? = nil) { let tabViewController = NSTabViewController() tabViewController.tabStyle = .toolbar tabViewController.title = "Settings" - tabViewController.tabViewItems = ReviewMonitorSettingsPane.allCases.map { - $0.tabViewItem(runtimePreferencesStore: runtimePreferencesStore) + tabViewController.tabViewItems = ReviewMonitorSettingsPane.allCases.compactMap { + $0.tabViewItem(runtimePreferencesStore: runtimePreferencesStore, updater: updater) } let window = ReviewMonitorSettingsWindow( @@ -77,7 +89,7 @@ final class ReviewMonitorSettingsWindowController: NSWindowController { func openPane(_ pane: ReviewMonitorSettingsPane) { guard let tabViewController = contentViewController as? NSTabViewController, - let index = ReviewMonitorSettingsPane.allCases.firstIndex(of: pane) + let index = tabViewController.tabViewItems.firstIndex(where: { $0.identifier as? String == pane.rawValue }) else { showWindow(nil) return @@ -428,3 +440,126 @@ private final class PreviewRuntimePreferencesStore: CodexReviewRuntime.Preferenc } } #endif + +@MainActor +final class ReviewMonitorUpdateSettingsViewController: NSHostingController { + init(updater: ReviewMonitorCodexUpdater) { + super.init(rootView: ReviewMonitorUpdateSettingsForm(updater: updater)) + title = ReviewMonitorSettingsPane.updates.label + preferredContentSize = NSSize(width: 560, height: 320) + } + + @available(*, unavailable) + required init?(coder: NSCoder) { nil } +} + +struct ReviewMonitorUpdateSettingsForm: View { + @Bindable var updater: ReviewMonitorCodexUpdater + + var body: some View { + Form { + Section("Codex Updates") { + HStack { + status + Spacer() + if updater.checkState == .checking { ProgressView().controlSize(.small) } + Button("Check for Updates") { + Task { await updater.checkForUpdates() } + } + .disabled(updater.isBusy) + } + if let date = updater.lastCheckedAt { + LabeledContent("Last checked", value: date.formatted(date: .abbreviated, time: .shortened)) + } else { + Text("Not checked yet.").foregroundStyle(.secondary) + } + Text("Automatically checks at launch and every 8 hours.") + .font(.callout).foregroundStyle(.secondary) + } + operationStatus + } + .formStyle(.grouped) + .frame(width: 560) + } + + @ViewBuilder + private var status: some View { + switch updater.checkState { + case .notChecked: Text("Not Checked") + case .checking: Text("Checking…") + case .available: Text("Update Available") + case .upToDate: Text("Up to Date") + case .unavailable(let message): + VStack(alignment: .leading) { + Text("Check Unavailable") + Text(message).font(.callout).foregroundStyle(.secondary) + } + case .failed(let message): + VStack(alignment: .leading) { + Text("Check Failed") + Text(message).font(.callout).foregroundStyle(.secondary) + } + } + } + + @ViewBuilder + private var operationStatus: some View { + switch updater.store.codexUpdateState { + case .idle: EmptyView() + case .waitingForReviews: + Section { Text("The update will start after current reviews finish. New requests are queued.") } + case .stoppingRuntime, .installing, .restarting: + Section { Label("Updating Codex…", systemImage: "arrow.triangle.2.circlepath") } + case .failed(let message): + Section("Last Update Error") { Text(message).foregroundStyle(.secondary) } + } + } +} + +#if DEBUG +#Preview("Checking for Updates") { UpdateSettingsPreview(.checking) } +#Preview("Update Available") { UpdateSettingsPreview(.available) } +#Preview("Codex Up to Date") { UpdateSettingsPreview(.upToDate) } +#Preview("Update Check Unavailable") { UpdateSettingsPreview(.unavailable) } +#Preview("Update Check Failed") { UpdateSettingsPreview(.failed) } + +@MainActor +private struct UpdateSettingsPreview: View { + enum Result { case checking, available, upToDate, unavailable, failed } + @State private var updater: ReviewMonitorCodexUpdater + + init(_ result: Result) { + _updater = State(initialValue: ReviewMonitorCodexUpdater( + store: ReviewMonitorUpdatePreview().store, + check: { + switch result { + case .checking: + try await Task.sleep(for: .seconds(600)) + return .upToDate + case .available: + return .available(.init(executableURL: URL(fileURLWithPath: "/preview/codex"), environment: [:])) + case .upToDate: return .upToDate + case .unavailable: return .unavailable("This installation is managed outside ReviewMonitor.") + case .failed: throw PreviewUpdateCheckError.offline + } + }, + publishAvailability: { _ in }, + chooseTiming: { nil }, + runUpdate: { _ in }, + presentFailure: { _, _ in } + )) + } + + var body: some View { + ReviewMonitorUpdateSettingsForm(updater: updater) + .frame(height: 320) + .task { await updater.checkForUpdates() } + .onDisappear { Task { await updater.stopChecking() } } + } +} + +private enum PreviewUpdateCheckError: LocalizedError { + case offline + var errorDescription: String? { "The update server could not be reached. Check your connection and try again." } +} +#endif diff --git a/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexCommandUpdateCheckerTests.swift b/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexCommandUpdateCheckerTests.swift index 485834b4..b987c078 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexCommandUpdateCheckerTests.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexCommandUpdateCheckerTests.swift @@ -148,7 +148,7 @@ struct CodexCommandUpdateCheckerTests { } ) - #expect(try await checker.check() == .unavailable) + if case .unavailable = try await checker.check() {} else { Issue.record("Expected an unsupported installation") } } @Test func explicitRuntimeHomeOwnsDoctorAndPlanEnvironment() async throws { @@ -175,19 +175,19 @@ struct CodexCommandUpdateCheckerTests { ].joined(separator: ":")) } - @Test func disabledSettingWinsWithoutUpdateProbeDetails() async throws { + @Test func startupPreferenceDoesNotDisableMonitorChecks() async throws { let result = try await makeChecker( - output: report(enabled: "false") + output: report(enabled: "false", latestStatus: "newer version is available", action: "brew upgrade --cask codex") ).check() - #expect(result == .disabled) + if case .available = result {} else { Issue.record("Expected a Monitor update despite the CLI startup preference") } } - @Test func currentAndUnsupportedActionsAreUnavailable() async throws { + @Test func currentAndUnsupportedActionsHaveDifferentResults() async throws { let current = try await makeChecker(output: report( enabled: "true", latestStatus: "current version is not older" )).check() - #expect(current == .unavailable) + #expect(current == .upToDate) for action in [ "manual or unknown", @@ -200,17 +200,15 @@ struct CodexCommandUpdateCheckerTests { latestStatus: "newer version is available", action: action )).check() - #expect(unsupported == .unavailable) + if case .unavailable = unsupported {} else { Issue.record("Expected an unsupported update action") } } } - @Test func malformedContractValuesThrow() async { + @Test func missingOrUnknownUpdateStatusThrows() async { let reports = [ - report(enabled: "TRUE"), report(enabled: "true", latestStatus: "unknown"), - #"{"schemaVersion":2,"checks":{}}"#, - #"{"schemaVersion":1,"checks":{}}"#, - #"{"schemaVersion":1,"checks":{"updates.status":{"details":{"check for update on startup":["true","false"]}}}}"#, + #"{"checks":{}}"#, + #"{"checks":{"updates.status":{"details":{"latest version status":["current version is not older","newer version is available"]}}}}"#, ] for output in reports { await #expect(throws: (any Error).self) { @@ -219,6 +217,16 @@ struct CodexCommandUpdateCheckerTests { } } + @Test func failedVersionProbePreservesItsReason() async { + let report = #"{"checks":{"updates.status":{"details":{"latest version probe":"network offline"}}}}"# + do { + _ = try await makeChecker(output: report).check() + Issue.record("Expected a failed check") + } catch { + #expect(error.localizedDescription.contains("network offline")) + } + } + @Test func liveRunnerAcceptsValidJSONFromNonzeroDoctorExit() async throws { let directory = FileManager.default.temporaryDirectory.appendingPathComponent( "codex-update-checker-\(UUID().uuidString)", diff --git a/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexReviewMonitorCITests.swift b/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexReviewMonitorCITests.swift index af3e484c..17edd7cb 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexReviewMonitorCITests.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitorCITests/CodexReviewMonitorCITests.swift @@ -94,7 +94,7 @@ struct CodexReviewMonitorCITests { #expect(responder.replies == [true]) } - @Test func terminationCancelsAndAwaitsLaunchBeforeShutdown() async { + @Test func terminationStartsShutdownBeforeJoiningCancelledLaunch() async { let store = FakeLifecycleStore(blocksStart: true) let lifecycle = ReviewMonitorLifecycleController(store: store) let responder = TerminationReplyRecorder() @@ -106,15 +106,15 @@ struct CodexReviewMonitorCITests { #expect(store.shutdownCallCount == 0) #expect(responder.replies.isEmpty) - await store.startGate.open() await store.shutdownStartedSignal.wait() - - #expect(store.startObservedCancellation == [true]) + #expect(store.startObservedCancellation.isEmpty) #expect(store.shutdownCallCount == 1) #expect(responder.replies.isEmpty) await store.shutdownGate.open() + await store.startGate.open() #expect(await responder.waitForReply() == true) + #expect(store.startObservedCancellation == [true]) } @Test func appDelegateUsesInjectedCompositionForStartupDependencies() { @@ -142,7 +142,7 @@ struct CodexReviewMonitorCITests { capturedShowSettings = showSettings return recorder.makeWindowController() }, - makeSettingsWindowController: { + makeSettingsWindowController: { _ in settingsWindowController } ) @@ -242,7 +242,7 @@ struct CodexReviewMonitorCITests { makeWindowController: { _, _ in CountingWindowController() }, - makeSettingsWindowController: { + makeSettingsWindowController: { _ in settingsWindowController } ) diff --git a/Tools/ReviewMonitor/CodexReviewMonitorCITests/ReviewMonitorCodexUpdaterTests.swift b/Tools/ReviewMonitor/CodexReviewMonitorCITests/ReviewMonitorCodexUpdaterTests.swift index 767132e9..e01fcb76 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitorCITests/ReviewMonitorCodexUpdaterTests.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitorCITests/ReviewMonitorCodexUpdaterTests.swift @@ -1,382 +1,291 @@ import Foundation +import AppKit +import SwiftUI +import CodexReviewHost import Testing -@_spi(ApplicationHostSupport) import CodexReviewHost +@_spi(ApplicationHostSupport) import CodexReview +@_spi(PreviewSupport) import ReviewUI @testable import CodexReviewMonitor @Suite("ReviewMonitor Codex updater", .serialized) @MainActor struct ReviewMonitorCodexUpdaterTests { - @Test func checksAtLaunchAndAgainAfterTheSchedule() async { - let plan = updatePlan() - var results: [CodexCommandUpdateCheckResult] = [ - .unavailable, - .available(plan), - ] - var checkCount = 0 - var publishedAvailability: [Bool] = [] - let available = TestSignal() - let scheduledAfterAvailable = TestSignal() - let waitCount = UpdateTestCounter() - let updater = ReviewMonitorCodexUpdater( - check: { - checkCount += 1 - return results.removeFirst() - }, - wait: { - if await waitCount.increment() == 2 { - await scheduledAfterAvailable.signal() - try await Task.sleep(for: .seconds(60)) - } - }, - publishAvailability: { value in - publishedAvailability.append(value) - if value { - Task { await available.signal() } - } - }, - prepareForUpdate: { true }, - requestApplicationTermination: {}, - presentFailure: { _, _ in } - ) - + @Test func launchAndEightHourChecksStayAnchoredAcrossManualChecks() async { + let clock = UpdateTestClock() + var checks = 0 + let updater = makeUpdater(clock: clock, check: { checks += 1; return .upToDate }) updater.start() - await available.wait() - await scheduledAfterAvailable.wait() - - #expect(checkCount == 2) - #expect(await waitCount.value == 2) - #expect(publishedAvailability == [false, true]) - await updater.stopAndWait() - } - - @Test func disabledCheckStopsWithoutSchedulingAnotherCheck() async { - let checkCompleted = TestSignal() - let waitCount = UpdateTestCounter() - let updater = ReviewMonitorCodexUpdater( - check: { .disabled }, - wait: { - await waitCount.increment() - }, - publishAvailability: { _ in - Task { await checkCompleted.signal() } - }, - prepareForUpdate: { true }, - requestApplicationTermination: {}, - presentFailure: { _, _ in } - ) - updater.start() - await checkCompleted.wait() - await Task.yield() - - #expect(await waitCount.value == 0) + #expect(await waitUntil { checks == 1 && clock.waiterCount == 1 }) + clock.advance(by: .seconds(3 * 3600)) + await updater.checkForUpdates() + #expect(checks == 2) + clock.advance(by: .seconds(5 * 3600)) + #expect(await waitUntil { checks == 3 && clock.deadlines.last == 16 * 3600 }) + clock.advance(by: .seconds(8 * 3600)) + #expect(await waitUntil { checks == 4 && clock.deadlines.last == 24 * 3600 }) + let expectedDeadlines: [Int64] = [8 * 3600, 16 * 3600, 24 * 3600] + #expect(clock.deadlines == expectedDeadlines) + await updater.stopChecking() + #expect(clock.waiterCount == 0) } - @Test func acceptedUpdateSchedulesOneHelperBeforeRequestingTermination() async { - let plan = updatePlan() - let available = TestSignal() - var publishedAvailability: [Bool] = [] - var preparedUpdateCount = 0 - var updatedPlans: [CodexCommandUpdatePlan] = [] - var scheduledFailureValues: [Bool] = [] - var terminationRequestCount = 0 - let terminationRequested = TestSignal() - let updater = ReviewMonitorCodexUpdater( - check: { .available(plan) }, - publishAvailability: { value in - publishedAvailability.append(value) - if value { - Task { await available.signal() } - } - }, - prepareForUpdate: { - preparedUpdateCount += 1 - return true - }, - runUpdate: { updatedPlans.append($0) }, - scheduleRelaunch: { scheduledFailureValues.append($0) }, - requestApplicationTermination: { - terminationRequestCount += 1 - Task { await terminationRequested.signal() } - }, - presentFailure: { _, _ in } - ) + @Test func wakingAfterSeveralIntervalsChecksOnce() async { + let clock = UpdateTestClock() + var checks = 0 + let updater = makeUpdater(clock: clock, check: { checks += 1; return .upToDate }) updater.start() - await available.wait() - - updater.requestUpdate() - updater.requestUpdate() - await terminationRequested.wait() - - #expect(preparedUpdateCount == 1) - #expect(updatedPlans == [plan]) - #expect(scheduledFailureValues == [false]) - #expect(terminationRequestCount == 1) - #expect(publishedAvailability == [true, false]) - await updater.stopAndWait() + #expect(await waitUntil { checks == 1 && clock.waiterCount == 1 }) + clock.advance(by: .seconds(26 * 3600)) + #expect(await waitUntil { checks == 2 && clock.deadlines.last == 32 * 3600 }) + await updater.stopChecking() + #expect(checks == 2) } - @Test func stalePlanIsDiscardedBeforeTheStoreShutsDown() async { - let plan = updatePlan() - let available = TestSignal() - let waitCount = UpdateTestCounter() - var results: [CodexCommandUpdateCheckResult] = [ - .available(plan), - .unavailable, - .available(plan), - ] - var checkCount = 0 - var publishedAvailability: [Bool] = [] - var prepareForUpdateCount = 0 - var runUpdateCount = 0 - let updater = ReviewMonitorCodexUpdater( - check: { - checkCount += 1 - return results.removeFirst() - }, - wait: { - let count = await waitCount.increment() - if count != 2 { - try await Task.sleep(for: .seconds(60)) - } - }, - publishAvailability: { value in - publishedAvailability.append(value) - if value { - Task { await available.signal() } - } - }, - prepareForUpdate: { - prepareForUpdateCount += 1 - return true - }, - runUpdate: { _ in runUpdateCount += 1 }, - requestApplicationTermination: {}, - presentFailure: { _, _ in } - ) + @Test func manualAndAutomaticChecksJoinTheSameRequest() async { + let clock = UpdateTestClock() + let entered = TestSignal() + let release = TestGate() + var checks = 0 + let updater = makeUpdater(clock: clock, check: { + checks += 1 + await entered.signal() + await release.wait() + return .upToDate + }) + let manual = Task { await updater.checkForUpdates() } + await entered.wait() + let second = Task { await updater.checkForUpdates() } updater.start() - await available.wait() - - updater.requestUpdate() - for _ in 0..<100 where checkCount < 3 { - await Task.yield() - } - - #expect(checkCount == 3) - #expect(prepareForUpdateCount == 0) - #expect(runUpdateCount == 0) - #expect(publishedAvailability.filter { $0 }.count == 2) - await updater.stopAndWait() + await release.open() + await manual.value + await second.value + #expect(await waitUntil { clock.waiterCount == 1 }) + #expect(checks == 1) + #expect(updater.checkState == .upToDate) + await updater.stopChecking() } - @Test func reviewArrivingDuringRevalidationRequiresFreshConfirmation() async { + @Test func updateKeepsTheStoreAliveAndCoalescesChecksUntilInstallationFinishes() async throws { + let clock = UpdateTestClock() + let store = ReviewMonitorUpdatePreview().store + await store.start() let plan = updatePlan() - let available = TestSignal() - let revalidationStarted = TestSignal() - let releaseRevalidation = TestGate() - let preparationRejected = TestSignal() - var checkCount = 0 - var publishedAvailability: [Bool] = [] - var runUpdateCount = 0 - let updater = ReviewMonitorCodexUpdater( - check: { - checkCount += 1 - if checkCount == 1 { - return .available(plan) - } - await revalidationStarted.signal() - await releaseRevalidation.wait() - return .available(plan) - }, - publishAvailability: { value in - publishedAvailability.append(value) - if value, publishedAvailability.count == 1 { - Task { await available.signal() } - } - }, - prepareForUpdate: { - await preparationRejected.signal() - return false - }, - runUpdate: { _ in runUpdateCount += 1 }, - requestApplicationTermination: {}, - presentFailure: { _, _ in } - ) + var checks = 0 + var installations = 0 + let entered = TestSignal() + let release = TestGate() + let updater = makeUpdater(store: store, clock: clock, check: { + checks += 1 + return checks <= 2 ? .available(plan) : .upToDate + }, runUpdate: { received in + #expect(received == plan) + installations += 1 + await entered.signal() + await release.wait() + }) updater.start() - await available.wait() - updater.requestUpdate() - await revalidationStarted.wait() - - await releaseRevalidation.open() - await preparationRejected.wait() - - #expect(checkCount == 2) - #expect(runUpdateCount == 0) - #expect(publishedAvailability == [true, false, true]) - await updater.stopAndWait() + #expect(await waitUntil { checks == 1 && clock.waiterCount == 1 }) + let update = try #require(updater.requestUpdate()) + await entered.wait() + let duplicate = try #require(updater.requestUpdate()) + let manual = Task { await updater.checkForUpdates() } + clock.advance(by: .seconds(8 * 3600)) + #expect(await waitUntil { clock.deadlines.last == 16 * 3600 }) + #expect(checks == 2) + await release.open() + await update.value + await duplicate.value + await manual.value + #expect(installations == 1) + #expect(checks == 3) + #expect(updater.checkState == .upToDate) + #expect(store.serverState == .running) + #expect(store.codexUpdateState == .idle) + await updater.stopChecking() + await store.shutdown() } - @Test func updateAndRelaunchFailuresPreserveThePrimaryUpdateError() async { - let plan = updatePlan() - let available = TestSignal() - var publishedAvailability: [Bool] = [] - var terminationRequestCount = 0 - var failures: [(String, String)] = [] - let failurePresented = TestSignal() - let updater = ReviewMonitorCodexUpdater( - check: { .available(plan) }, - publishAvailability: { value in - publishedAvailability.append(value) - if value, publishedAvailability.count == 1 { - Task { await available.signal() } - } - }, - prepareForUpdate: { true }, - runUpdate: { _ in throw UpdateTestFailure.injected }, - scheduleRelaunch: { _ in throw UpdateTestFailure.injected }, - requestApplicationTermination: { - terminationRequestCount += 1 - }, - presentFailure: { - failures.append(($0, $1)) - Task { await failurePresented.signal() } - } - ) - updater.start() - await available.wait() - - updater.requestUpdate() - await failurePresented.wait() - - #expect(terminationRequestCount == 0) - #expect(publishedAvailability == [true, false]) - #expect(failures.count == 1) - #expect(failures.first?.0 == "Codex Could Not Be Updated") - #expect(failures.first?.1.contains("also could not schedule") == true) - await updater.stopAndWait() + @Test func stoppingChecksDoesNotWaitForAnUpdateThatStoreShutdownOwns() async throws { + let store = ReviewMonitorUpdatePreview().store + await store.start() + let entered = TestSignal() + let release = TestGate() + var observedCancellation = false + let updater = makeUpdater(store: store, check: { .available(updatePlan()) }, runUpdate: { _ in + await entered.signal() + await release.wait() + observedCancellation = Task.isCancelled + }) + let update = try #require(updater.requestUpdate()) + await entered.wait() + await updater.stopChecking() + var stopped = false + let shutdown = Task { await store.shutdown(); stopped = true } + await Task.yield() + #expect(stopped == false) + await release.open() + await update.value + await shutdown.value + #expect(observedCancellation == false) + #expect(store.serverState == .stopped) } - @Test func updateFailureIsReportedToTheRelaunchedApplication() async { - let plan = updatePlan() - let available = TestSignal() - let terminationRequested = TestSignal() - var scheduledFailureValues: [Bool] = [] - let updater = ReviewMonitorCodexUpdater( - check: { .available(plan) }, - publishAvailability: { value in - if value { - Task { await available.signal() } - } - }, - prepareForUpdate: { true }, - runUpdate: { _ in throw UpdateTestFailure.injected }, - scheduleRelaunch: { scheduledFailureValues.append($0) }, - requestApplicationTermination: { - Task { await terminationRequested.signal() } - }, - presentFailure: { _, _ in } - ) - updater.start() - await available.wait() + @Test func checkResultsAndAttemptTimeDistinguishFailureAndUnsupportedInstallations() async { + let clock = UpdateTestClock() + var checks = 0 + let updater = makeUpdater(clock: clock, check: { + checks += 1 + if checks == 1 { throw UpdateTestFailure.offline } + if checks == 2 { return .unavailable("Manual installation") } + return .upToDate + }) + await updater.checkForUpdates() + #expect(updater.checkState == .failed("Offline")) + #expect(updater.lastCheckedAt == clock.date) + clock.advance(by: .seconds(5)) + await updater.checkForUpdates() + #expect(updater.checkState == .unavailable("Manual installation")) + #expect(updater.lastCheckedAt == clock.date) + await updater.checkForUpdates() + #expect(updater.checkState == .upToDate) + await updater.stopChecking() + } - updater.requestUpdate() - await terminationRequested.wait() + @Test func updateFailureKeepsItsStoreErrorAndReportsItWithoutRelaunching() async throws { + let store = ReviewMonitorUpdatePreview().store + await store.start() + var failures: [String] = [] + let updater = makeUpdater(store: store, check: { .available(updatePlan()) }, + runUpdate: { _ in throw UpdateTestFailure.offline }, + presentFailure: { _, message in failures.append(message) }) + await updater.requestUpdate()?.value + #expect(failures == ["Offline"]) + #expect(store.codexUpdateState == .failed("Offline")) + #expect(store.serverState == .running) + await updater.stopChecking() + await store.shutdown() + } - #expect(scheduledFailureValues == [true]) - await updater.stopAndWait() + @Test func updateAlertOffersDeferredAndImmediateChoices() { + let alert = ReviewMonitorAppDelegate.makeCodexUpdateAlert() + #expect(alert.buttons.map(\.title) == ["Update After Reviews", "Stop Reviews and Update"]) + #expect(alert.buttons[1].hasDestructiveAction) } - @Test func applicationTerminationWaitsForAnUncancelledUpdate() async { - let plan = updatePlan() - let available = TestSignal() - let updateStarted = TestSignal() - let releaseUpdate = TestGate() - let stopCompleted = UpdateTestCounter() - var updateObservedCancellation: [Bool] = [] - let updater = ReviewMonitorCodexUpdater( - check: { .available(plan) }, - publishAvailability: { value in - if value { - Task { await available.signal() } - } - }, - prepareForUpdate: { true }, - runUpdate: { _ in - await updateStarted.signal() - await releaseUpdate.wait() - updateObservedCancellation.append(Task.isCancelled) - }, - scheduleRelaunch: { _ in }, - requestApplicationTermination: {}, - presentFailure: { _, _ in } + @Test func settingsPaneSharesTheUpdaterAndDoesNotStartCheckingOnOpen() { + var checks = 0 + let updater = makeUpdater(check: { checks += 1; return .upToDate }) + let controller = ReviewMonitorSettingsWindowController( + runtimePreferencesStore: CodexReviewRuntime.UserDefaultsPreferencesStore(), + updater: updater ) - updater.start() - await available.wait() - updater.requestUpdate() - await updateStarted.wait() - - let stop = Task { @MainActor in - await updater.stopAndWait() - await stopCompleted.increment() - } - await Task.yield() - #expect(await stopCompleted.value == 0) - - await releaseUpdate.open() - await stop.value - #expect(updateObservedCancellation == [false]) + let tabs = controller.contentViewController as? NSTabViewController + #expect(tabs?.tabViewItems.map(\.label) == ["Runtime", "Updates"]) + let pane = tabs?.tabViewItems.last?.viewController as? ReviewMonitorUpdateSettingsViewController + #expect(pane?.rootView.updater === updater) + #expect(checks == 0) } - @Test func helperWaitsForThisAppThenRelaunchesWithTheResult() { - let applicationURL = URL(fileURLWithPath: "/Applications/ReviewMonitor.app") - - let arguments = ReviewMonitorApplicationRelauncher.helperArguments( - applicationURL: applicationURL, - processIdentifier: 42, - reportsFailure: true - ) + @Test func explicitUpdateReportsCheckFailureWithoutInstalling() async { + var failures: [String] = [] + var installations = 0 + let updater = makeUpdater(check: { throw UpdateTestFailure.offline }, + runUpdate: { _ in installations += 1 }, + presentFailure: { _, message in failures.append(message) }) + await updater.requestUpdate()?.value + #expect(installations == 0) + #expect(failures == ["Offline"]) + #expect(updater.checkState == .failed("Offline")) + await updater.stopChecking() + } - #expect(arguments[0] == "-c") - #expect(arguments[2...] == [ - "reviewmonitor-relaunch", - "42", - applicationURL.path, - ReviewMonitorApplicationRelauncher.failedUpdateLaunchArgument, - ]) - #expect(arguments[1].contains("/usr/bin/open \"$2\"")) + @Test func retryRecoversTheRuntimeWithoutInstallingAgain() async { + let preview = ReviewMonitorUpdatePreview() + await preview.run(.failed) + var installations = 0 + let updater = makeUpdater(store: preview.store, check: { .upToDate }, + runUpdate: { _ in installations += 1 }) + await updater.requestUpdate()?.value + #expect(installations == 0) + #expect(preview.store.serverState == .running) + await updater.stopChecking() + await preview.stop() } - @Test func failedUpdateRelaunchArgumentIsRecognizedByLaunchContext() { - let context = ReviewMonitorLaunchContext( - environment: [:], - arguments: [ - "ReviewMonitor", - ReviewMonitorApplicationRelauncher.failedUpdateLaunchArgument, - ], - launchMode: .application + private func makeUpdater( + store: CodexReviewStore? = nil, + clock: UpdateTestClock = UpdateTestClock(), + check: @escaping ReviewMonitorCodexUpdater.Check, + runUpdate: @escaping ReviewMonitorCodexUpdater.RunUpdate = { _ in }, + presentFailure: @escaping @MainActor (String, String) -> Void = { _, _ in } + ) -> ReviewMonitorCodexUpdater { + ReviewMonitorCodexUpdater( + store: store ?? ReviewMonitorUpdatePreview().store, + check: check, + now: { clock.instant }, + date: { clock.date }, + sleepUntil: { try await clock.sleep(until: $0) }, + publishAvailability: { _ in }, + chooseTiming: { .afterCurrentReviews }, + runUpdate: runUpdate, + presentFailure: presentFailure ) - - #expect(context.reportsFailedCodexUpdate) } private func updatePlan() -> CodexCommandUpdatePlan { - CodexCommandUpdatePlan( - executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/codex"), - environment: ["PATH": "/opt/homebrew/bin:/usr/bin:/bin"] - ) + .init(executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/codex"), environment: [:]) } } -private enum UpdateTestFailure: Error { - case injected +private enum UpdateTestFailure: LocalizedError { + case offline + var errorDescription: String? { "Offline" } } -private actor UpdateTestCounter { - private(set) var value = 0 +@MainActor +private final class UpdateTestClock { + let origin = ContinuousClock.now + var instant: ContinuousClock.Instant + private var waiters: [UUID: (ContinuousClock.Instant, CheckedContinuation)] = [:] + private(set) var deadlines: [Int64] = [] + var waiterCount: Int { waiters.count } + var date: Date { Date(timeIntervalSince1970: Double(origin.duration(to: instant).components.seconds)) } + + init() { instant = origin } + + func advance(by duration: Duration) { + instant = instant.advanced(by: duration) + let due = waiters.filter { $0.value.0 <= instant } + for (id, waiter) in due { + waiters.removeValue(forKey: id) + waiter.1.resume() + } + } + + func sleep(until deadline: ContinuousClock.Instant) async throws { + let id = UUID() + deadlines.append(origin.duration(to: deadline).components.seconds) + try await withTaskCancellationHandler { + try Task.checkCancellation() + try await withCheckedThrowingContinuation { continuation in + if deadline <= instant { continuation.resume() } + else { waiters[id] = (deadline, continuation) } + } + } onCancel: { + Task { @MainActor in + self.waiters.removeValue(forKey: id)?.1.resume(throwing: CancellationError()) + } + } + } +} - @discardableResult - func increment() -> Int { - value += 1 - return value +@MainActor +private func waitUntil(_ condition: () -> Bool) async -> Bool { + let deadline = ContinuousClock.now.advanced(by: .seconds(2)) + while condition() == false { + if ContinuousClock.now >= deadline { return false } + await Task.yield() } + return true }