From 78416d3e0c47668161f2c3b5dae8e36068d45acd Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 9 Aug 2026 01:39:18 +0900 Subject: [PATCH 1/5] feat(review-ui): separate API key sign-in flow --- Package.resolved | 4 +- Sources/ReviewUI/SignIn/SignInView.swift | 195 +++++++++++++++--- .../ReviewMonitorAddAccountActionTests.swift | 8 + 3 files changed, 172 insertions(+), 35 deletions(-) diff --git a/Package.resolved b/Package.resolved index 235e6f0..0736ba2 100644 --- a/Package.resolved +++ b/Package.resolved @@ -51,8 +51,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/apple/swift-log.git", "state" : { - "revision" : "a878e7f8f46cfc0e1125e565b5c08e7d5272dc9a", - "version" : "1.14.0" + "revision" : "bbd81b6725ae874c69e9b8c8804d462356b55523", + "version" : "1.10.1" } }, { diff --git a/Sources/ReviewUI/SignIn/SignInView.swift b/Sources/ReviewUI/SignIn/SignInView.swift index 844b608..a33d5a4 100644 --- a/Sources/ReviewUI/SignIn/SignInView.swift +++ b/Sources/ReviewUI/SignIn/SignInView.swift @@ -60,23 +60,38 @@ struct SignInView: View { enum AccessibilityIdentifier { static let chatGPTButton = "review-monitor.sign-in-button" + static let alternateSignInButton = "review-monitor.alternate-sign-in-button" static let apiKeyField = "review-monitor.api-key-field" static let apiKeyButton = "review-monitor.api-key-sign-in-button" + static let apiKeyCancelButton = "review-monitor.api-key-cancel-button" static let cancelButton = "review-monitor.authentication-cancel-button" } let store: CodexReviewStore - @State private var apiKeyInput = ReviewMonitorAPIKeyInput() + @State private var showsAPIKeySignIn = false @State private var authenticationFailureMessage: String? var body: some View { + ZStack{ + if showsAPIKeySignIn { + APIKeySignInView(store: store) { + showsAPIKeySignIn = false + } + } else { + signInOptions + } + } + .animation(.easeInOut(duration: 0.22), value: showsAPIKeySignIn) + } + + private var signInOptions: some View { let controlState = ControlState( - apiKeyIsEmpty: apiKeyInput.isEmpty, + apiKeyIsEmpty: true, isAuthenticating: store.auth.isAuthenticating, canPerformAuthentication: store.canPerformPrimaryAuthenticationAction ) - ContentUnavailableView { + return ContentUnavailableView { Text("Welcome to CodexReviewMonitor") .font(.largeTitle) .fontDesign(.rounded) @@ -94,26 +109,13 @@ struct SignInView: View { .disabled(controlState.providerInputsAreDisabled) .accessibilityIdentifier(AccessibilityIdentifier.chatGPTButton) - HStack(spacing: 8) { - SecureField("OpenAI API key", text: apiKeyBinding) - .textFieldStyle(.roundedBorder) - .accessibilityLabel("OpenAI API key") - .accessibilityIdentifier(AccessibilityIdentifier.apiKeyField) - .onSubmit { - guard controlState.apiKeySubmitIsDisabled == false else { - return - } - submitAPIKey() - } - - Button("Sign in with API Key") { - submitAPIKey() - } - .accessibilityIdentifier(AccessibilityIdentifier.apiKeyButton) - .disabled(controlState.apiKeySubmitIsDisabled) + Button("Sign in another way") { + showsAPIKeySignIn = true } + .buttonSizing(.flexible) + .buttonBorderShape(.capsule) .disabled(controlState.providerInputsAreDisabled) - .frame(maxWidth: 440) + .accessibilityIdentifier(AccessibilityIdentifier.alternateSignInButton) if controlState.showsCancelAction { Button(role: .cancel) { @@ -132,6 +134,7 @@ struct SignInView: View { .accessibilityIdentifier(AccessibilityIdentifier.cancelButton) } } + .frame(maxWidth: 440) .animation(.default, value: controlState) } description: { @@ -140,6 +143,118 @@ struct SignInView: View { } } .scenePadding() + .alert( + "Authentication Request Failed", + isPresented: Binding( + get: { authenticationFailureMessage != nil }, + set: { if $0 == false { authenticationFailureMessage = nil } } + ) + ) { + Button("OK") { + authenticationFailureMessage = nil + } + } message: { + Text(authenticationFailureMessage ?? "Authentication request failed.") + } + } + + private func performAuthentication(_ submission: ReviewMonitorAuthenticationSubmission) { + Task { @MainActor in + do { + try await store.performPrimaryAuthenticationAction(using: submission.method) + } catch { + authenticationFailureMessage = error.localizedDescription + } + } + } + + private func cancelAuthentication() { + Task { @MainActor in + await store.cancelAuthentication() + } + } + + private var descriptionText: String? { + store.auth.progress?.detail ?? store.auth.errorMessage ?? serverFailureMessage + } + + private var serverFailureMessage: String? { + guard case .failed(let message) = store.serverState else { + return nil + } + let trimmedMessage = message.trimmingCharacters(in: .whitespacesAndNewlines) + return trimmedMessage.isEmpty ? nil : trimmedMessage + } +} + +private struct APIKeySignInView: View { + let store: CodexReviewStore + let onCancel: () -> Void + @State private var apiKeyInput = ReviewMonitorAPIKeyInput() + @State private var authenticationFailureMessage: String? + + var body: some View { + let controlState = SignInView.ControlState( + apiKeyIsEmpty: apiKeyInput.isEmpty, + isAuthenticating: store.auth.isAuthenticating, + canPerformAuthentication: store.canPerformPrimaryAuthenticationAction + ) + + ContentUnavailableView { + Text("Welcome to CodexReviewMonitor") + .font(.largeTitle) + .fontDesign(.rounded) + .fontWidth(.compressed) + .fontWeight(.semibold) + .scenePadding(.bottom) + + VStack(spacing: 12) { + VStack(alignment: .leading, spacing: 8) { + Text("OpenAI API key") + + SecureField("sk-…", text: apiKeyBinding) + .textFieldStyle(.roundedBorder) + .accessibilityLabel("OpenAI API key") + .accessibilityIdentifier(SignInView.AccessibilityIdentifier.apiKeyField) + .onSubmit { + guard controlState.apiKeySubmitIsDisabled == false else { + return + } + submitAPIKey() + } + } + + HStack(spacing: 8) { + Button("Cancel", role: .cancel) { + cancel() + } + .buttonSizing(.flexible) + .buttonBorderShape(.capsule) + .accessibilityIdentifier(SignInView.AccessibilityIdentifier.apiKeyCancelButton) + + Button("Continue") { + submitAPIKey() + } + .buttonSizing(.flexible) + .buttonBorderShape(.capsule) + .buttonStyle(.glassProminent) + .disabled(controlState.apiKeySubmitIsDisabled) + .accessibilityIdentifier(SignInView.AccessibilityIdentifier.apiKeyButton) + } + + if controlState.showsCancelAction { + ProgressView() + .controlSize(.small) + } + } + .frame(maxWidth: 440) + .animation(.default, value: controlState) + } description: { + if let descriptionText { + Text(descriptionText) + } + } + .scenePadding() .onDisappear { apiKeyInput.clear() } @@ -168,26 +283,27 @@ struct SignInView: View { private func submitAPIKey() { do { let submission = try apiKeyInput.takeSubmission() - performAuthentication(submission) + Task { @MainActor in + do { + try await store.performPrimaryAuthenticationAction(using: submission.method) + } catch { + authenticationFailureMessage = error.localizedDescription + } + } } catch { authenticationFailureMessage = error.localizedDescription } } - private func performAuthentication(_ submission: ReviewMonitorAuthenticationSubmission) { - Task { @MainActor in - do { - try await store.performPrimaryAuthenticationAction(using: submission.method) - } catch { - authenticationFailureMessage = error.localizedDescription - } - } - } - - private func cancelAuthentication() { + private func cancel() { apiKeyInput.clear() + guard store.auth.isAuthenticating else { + onCancel() + return + } Task { @MainActor in await store.cancelAuthentication() + onCancel() } } @@ -203,3 +319,16 @@ struct SignInView: View { return trimmedMessage.isEmpty ? nil : trimmedMessage } } + +#if DEBUG +#Preview("Sign In") { + SignInView(store: CodexReviewStore.makePreviewStore()) +} + +#Preview("API Key Sign In") { + APIKeySignInView( + store: CodexReviewStore.makePreviewStore(), + onCancel: {} + ) +} +#endif diff --git a/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift b/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift index 8503e4f..72e4f0f 100644 --- a/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift @@ -8,8 +8,16 @@ import Testing struct ReviewMonitorAddAccountActionTests { @Test func signedOutSignInControlsExposeStableProviderIdentifiers() { #expect(SignInView.AccessibilityIdentifier.chatGPTButton == "review-monitor.sign-in-button") + #expect( + SignInView.AccessibilityIdentifier.alternateSignInButton + == "review-monitor.alternate-sign-in-button" + ) #expect(SignInView.AccessibilityIdentifier.apiKeyField == "review-monitor.api-key-field") #expect(SignInView.AccessibilityIdentifier.apiKeyButton == "review-monitor.api-key-sign-in-button") + #expect( + SignInView.AccessibilityIdentifier.apiKeyCancelButton + == "review-monitor.api-key-cancel-button" + ) } @Test func signInControlStateKeepsOnlyCancellationAvailableDuringAuthentication() { From cfe84f2672fc64296752dcdfd51055e8984de568 Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 9 Aug 2026 01:44:05 +0900 Subject: [PATCH 2/5] fix(review-ui): disable API key input during authentication --- Sources/ReviewUI/SignIn/SignInView.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/ReviewUI/SignIn/SignInView.swift b/Sources/ReviewUI/SignIn/SignInView.swift index a33d5a4..4ee4fb8 100644 --- a/Sources/ReviewUI/SignIn/SignInView.swift +++ b/Sources/ReviewUI/SignIn/SignInView.swift @@ -214,6 +214,7 @@ private struct APIKeySignInView: View { SecureField("sk-…", text: apiKeyBinding) .textFieldStyle(.roundedBorder) + .disabled(controlState.providerInputsAreDisabled) .accessibilityLabel("OpenAI API key") .accessibilityIdentifier(SignInView.AccessibilityIdentifier.apiKeyField) .onSubmit { From 217dce770a29cf41eff0a8ac8ec0ce7c9f73ca5e Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 9 Aug 2026 11:38:06 +0900 Subject: [PATCH 3/5] fix(auth): own sign-in cancellation lifecycle --- .../Store/CodexReviewStore.swift | 1 + Sources/ReviewUI/SignIn/SignInView.swift | 211 ++++++++++++------ .../ReviewMonitorAddAccountActionTests.swift | 58 +++++ 3 files changed, 204 insertions(+), 66 deletions(-) diff --git a/Sources/CodexReviewKit/Store/CodexReviewStore.swift b/Sources/CodexReviewKit/Store/CodexReviewStore.swift index c904b20..71f52be 100644 --- a/Sources/CodexReviewKit/Store/CodexReviewStore.swift +++ b/Sources/CodexReviewKit/Store/CodexReviewStore.swift @@ -240,6 +240,7 @@ public final class CodexReviewStore { else { return } + try Task.checkCancellation() try await signIn(using: method) } diff --git a/Sources/ReviewUI/SignIn/SignInView.swift b/Sources/ReviewUI/SignIn/SignInView.swift index 4ee4fb8..87dc3a8 100644 --- a/Sources/ReviewUI/SignIn/SignInView.swift +++ b/Sources/ReviewUI/SignIn/SignInView.swift @@ -1,4 +1,5 @@ import SwiftUI +import Observation import CodexReviewKit struct ReviewMonitorAuthenticationSubmission { @@ -41,6 +42,109 @@ private struct ReviewMonitorAPIKeyValidationFailure: LocalizedError { } } +@MainActor +@Observable +final class ReviewMonitorSignInSession { + enum Screen: Equatable { + case options + case apiKey + } + + private struct Operation { + let id: UUID + let task: Task + } + + private(set) var screen = Screen.options + var failureMessage: String? + + @ObservationIgnored + private var operation: Operation? + + isolated deinit { + operation?.task.cancel() + } + + func showAPIKeySignIn() { + screen = .apiKey + } + + func present(_ error: any Error) { + failureMessage = error.localizedDescription + } + + func authenticate( + _ submission: ReviewMonitorAuthenticationSubmission, + store: CodexReviewStore + ) { + precondition(operation == nil, "A sign-in session can perform one operation at a time.") + failureMessage = nil + + let operationID = UUID() + let task = Task { @MainActor [weak self] in + let failureMessage: String? + do { + try await store.performPrimaryAuthenticationAction(using: submission.method) + failureMessage = nil + } catch is CancellationError { + failureMessage = nil + } catch { + failureMessage = error.localizedDescription + } + self?.finish(operationID, failureMessage: failureMessage) + } + operation = Operation(id: operationID, task: task) + } + + func cancelAuthentication( + store: CodexReviewStore, + returnsToOptions: Bool = false + ) { + let authentication = operation + operation = nil + authentication?.task.cancel() + failureMessage = nil + + let operationID = UUID() + let task = Task { @MainActor [weak self] in + await store.cancelAuthentication() + await authentication?.task.value + guard Task.isCancelled == false else { + self?.finish(operationID, failureMessage: nil) + return + } + if returnsToOptions { + self?.screen = .options + } + self?.finish(operationID, failureMessage: nil) + } + operation = Operation(id: operationID, task: task) + } + + func close(store: CodexReviewStore) { + screen = .options + cancelAuthentication(store: store) + } + + private func finish(_ operationID: UUID, failureMessage: String?) { + guard operation?.id == operationID else { + return + } + operation = nil + if let failureMessage { + self.failureMessage = failureMessage + } + } + +#if DEBUG + func waitUntilIdleForTesting() async { + while let operation { + await operation.task.value + } + } +#endif +} + struct SignInView: View { struct ControlState: Equatable { let providerInputsAreDisabled: Bool @@ -68,20 +172,43 @@ struct SignInView: View { } let store: CodexReviewStore - @State private var showsAPIKeySignIn = false - @State private var authenticationFailureMessage: String? + @State private var session = ReviewMonitorSignInSession() var body: some View { - ZStack{ - if showsAPIKeySignIn { - APIKeySignInView(store: store) { - showsAPIKeySignIn = false - } + ZStack { + if session.screen == .apiKey { + APIKeySignInView( + store: store, + session: session, + onSubmit: performAuthentication, + onCancel: { + session.cancelAuthentication( + store: store, + returnsToOptions: true + ) + } + ) } else { signInOptions } } - .animation(.easeInOut(duration: 0.22), value: showsAPIKeySignIn) + .animation(.easeInOut(duration: 0.22), value: session.screen) + .onDisappear { + session.close(store: store) + } + .alert( + "Authentication Request Failed", + isPresented: Binding( + get: { session.failureMessage != nil }, + set: { if $0 == false { session.failureMessage = nil } } + ) + ) { + Button("OK") { + session.failureMessage = nil + } + } message: { + Text(session.failureMessage ?? "Authentication request failed.") + } } private var signInOptions: some View { @@ -110,7 +237,7 @@ struct SignInView: View { .accessibilityIdentifier(AccessibilityIdentifier.chatGPTButton) Button("Sign in another way") { - showsAPIKeySignIn = true + session.showAPIKeySignIn() } .buttonSizing(.flexible) .buttonBorderShape(.capsule) @@ -119,7 +246,7 @@ struct SignInView: View { if controlState.showsCancelAction { Button(role: .cancel) { - cancelAuthentication() + session.cancelAuthentication(store: store) } label: { LabeledContent { ProgressView() @@ -143,35 +270,10 @@ struct SignInView: View { } } .scenePadding() - .alert( - "Authentication Request Failed", - isPresented: Binding( - get: { authenticationFailureMessage != nil }, - set: { if $0 == false { authenticationFailureMessage = nil } } - ) - ) { - Button("OK") { - authenticationFailureMessage = nil - } - } message: { - Text(authenticationFailureMessage ?? "Authentication request failed.") - } } private func performAuthentication(_ submission: ReviewMonitorAuthenticationSubmission) { - Task { @MainActor in - do { - try await store.performPrimaryAuthenticationAction(using: submission.method) - } catch { - authenticationFailureMessage = error.localizedDescription - } - } - } - - private func cancelAuthentication() { - Task { @MainActor in - await store.cancelAuthentication() - } + session.authenticate(submission, store: store) } private var descriptionText: String? { @@ -189,9 +291,10 @@ struct SignInView: View { private struct APIKeySignInView: View { let store: CodexReviewStore + let session: ReviewMonitorSignInSession + let onSubmit: (ReviewMonitorAuthenticationSubmission) -> Void let onCancel: () -> Void @State private var apiKeyInput = ReviewMonitorAPIKeyInput() - @State private var authenticationFailureMessage: String? var body: some View { let controlState = SignInView.ControlState( @@ -259,19 +362,6 @@ private struct APIKeySignInView: View { .onDisappear { apiKeyInput.clear() } - .alert( - "Authentication Request Failed", - isPresented: Binding( - get: { authenticationFailureMessage != nil }, - set: { if $0 == false { authenticationFailureMessage = nil } } - ) - ) { - Button("OK") { - authenticationFailureMessage = nil - } - } message: { - Text(authenticationFailureMessage ?? "Authentication request failed.") - } } private var apiKeyBinding: Binding { @@ -284,28 +374,15 @@ private struct APIKeySignInView: View { private func submitAPIKey() { do { let submission = try apiKeyInput.takeSubmission() - Task { @MainActor in - do { - try await store.performPrimaryAuthenticationAction(using: submission.method) - } catch { - authenticationFailureMessage = error.localizedDescription - } - } + onSubmit(submission) } catch { - authenticationFailureMessage = error.localizedDescription + session.present(error) } } private func cancel() { apiKeyInput.clear() - guard store.auth.isAuthenticating else { - onCancel() - return - } - Task { @MainActor in - await store.cancelAuthentication() - onCancel() - } + onCancel() } private var descriptionText: String? { @@ -329,6 +406,8 @@ private struct APIKeySignInView: View { #Preview("API Key Sign In") { APIKeySignInView( store: CodexReviewStore.makePreviewStore(), + session: ReviewMonitorSignInSession(), + onSubmit: { _ in }, onCancel: {} ) } diff --git a/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift b/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift index 72e4f0f..73a0b72 100644 --- a/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift @@ -1,6 +1,7 @@ import Foundation import Testing @testable import CodexReviewKit +import CodexReviewTesting @testable import ReviewUI @Suite("ReviewMonitor add account action") @@ -140,4 +141,61 @@ struct ReviewMonitorAddAccountActionTests { #expect(error.localizedDescription == "Enter a valid OpenAI API key.") } } + + @Test func cancellingAPIKeySignInWhileRuntimeStartsPreventsAuthentication() async throws { + let startGate = AsyncGate() + let backend = BlockingSignInStartBackend(startGate: startGate) + let store = CodexReviewStore.makeTestingStore(backend: backend) + let session = ReviewMonitorSignInSession() + let apiKey = try CodexReviewAPIKey(validating: "sk-ui-test-secret") + store.loadForTesting(serverState: .failed("Runtime failed."), authPhase: .signedOut) + + session.showAPIKeySignIn() + session.authenticate(.init(method: .apiKey(apiKey)), store: store) + await startGate.waitUntilBlocked() + session.cancelAuthentication(store: store, returnsToOptions: true) + await startGate.open() + await session.waitUntilIdleForTesting() + + #expect(backend.authenticationRequests.isEmpty) + #expect(session.screen == .options) + } + + @Test func closingSignInSessionRestoresPrimaryOptions() async { + let store = CodexReviewStore.makePreviewStore() + let session = ReviewMonitorSignInSession() + session.showAPIKeySignIn() + + session.close(store: store) + await session.waitUntilIdleForTesting() + + #expect(session.screen == .options) + } +} + +@MainActor +private final class BlockingSignInStartBackend: PreviewCodexReviewStoreBackend { + let startGate: AsyncGate + private(set) var authenticationRequests: [CodexReviewAuthenticationRequest] = [] + + init(startGate: AsyncGate) { + self.startGate = startGate + super.init() + } + + override func start( + store: CodexReviewStore, + forceRestartIfNeeded _: Bool + ) async { + await startGate.waitIgnoringCancellation() + isActive = true + store.transitionToRunning(serverURL: nil) + } + + override func authenticate( + auth _: CodexReviewAuthModel, + request: CodexReviewAuthenticationRequest + ) async throws { + authenticationRequests.append(request) + } } From 970d0b7af389ee99f577a189d5996b53657c841f Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 9 Aug 2026 11:49:00 +0900 Subject: [PATCH 4/5] fix(auth): disable duplicate sign-in submissions --- Sources/ReviewUI/SignIn/SignInView.swift | 24 ++++++++---- .../ReviewMonitorAddAccountActionTests.swift | 37 +++++++++++++++++-- 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/Sources/ReviewUI/SignIn/SignInView.swift b/Sources/ReviewUI/SignIn/SignInView.swift index 87dc3a8..2508ff2 100644 --- a/Sources/ReviewUI/SignIn/SignInView.swift +++ b/Sources/ReviewUI/SignIn/SignInView.swift @@ -58,9 +58,12 @@ final class ReviewMonitorSignInSession { private(set) var screen = Screen.options var failureMessage: String? - @ObservationIgnored private var operation: Operation? + var hasPendingOperation: Bool { + operation != nil + } + isolated deinit { operation?.task.cancel() } @@ -77,7 +80,9 @@ final class ReviewMonitorSignInSession { _ submission: ReviewMonitorAuthenticationSubmission, store: CodexReviewStore ) { - precondition(operation == nil, "A sign-in session can perform one operation at a time.") + guard operation == nil else { + return + } failureMessage = nil let operationID = UUID() @@ -154,11 +159,14 @@ struct SignInView: View { init( apiKeyIsEmpty: Bool, isAuthenticating: Bool, - canPerformAuthentication: Bool + canPerformAuthentication: Bool, + hasPendingOperation: Bool ) { - providerInputsAreDisabled = isAuthenticating || canPerformAuthentication == false + providerInputsAreDisabled = isAuthenticating + || canPerformAuthentication == false + || hasPendingOperation apiKeySubmitIsDisabled = providerInputsAreDisabled || apiKeyIsEmpty - showsCancelAction = isAuthenticating + showsCancelAction = isAuthenticating || hasPendingOperation } } @@ -215,7 +223,8 @@ struct SignInView: View { let controlState = ControlState( apiKeyIsEmpty: true, isAuthenticating: store.auth.isAuthenticating, - canPerformAuthentication: store.canPerformPrimaryAuthenticationAction + canPerformAuthentication: store.canPerformPrimaryAuthenticationAction, + hasPendingOperation: session.hasPendingOperation ) return ContentUnavailableView { @@ -300,7 +309,8 @@ private struct APIKeySignInView: View { let controlState = SignInView.ControlState( apiKeyIsEmpty: apiKeyInput.isEmpty, isAuthenticating: store.auth.isAuthenticating, - canPerformAuthentication: store.canPerformPrimaryAuthenticationAction + canPerformAuthentication: store.canPerformPrimaryAuthenticationAction, + hasPendingOperation: session.hasPendingOperation ) ContentUnavailableView { diff --git a/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift b/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift index 73a0b72..ebbbcf2 100644 --- a/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift +++ b/Tests/ReviewUITests/ReviewMonitorAddAccountActionTests.swift @@ -25,7 +25,8 @@ struct ReviewMonitorAddAccountActionTests { let signedOut = SignInView.ControlState( apiKeyIsEmpty: true, isAuthenticating: false, - canPerformAuthentication: true + canPerformAuthentication: true, + hasPendingOperation: false ) #expect(signedOut.providerInputsAreDisabled == false) #expect(signedOut.apiKeySubmitIsDisabled) @@ -34,18 +35,30 @@ struct ReviewMonitorAddAccountActionTests { let populated = SignInView.ControlState( apiKeyIsEmpty: false, isAuthenticating: false, - canPerformAuthentication: true + canPerformAuthentication: true, + hasPendingOperation: false ) #expect(populated.apiKeySubmitIsDisabled == false) let authenticating = SignInView.ControlState( apiKeyIsEmpty: false, isAuthenticating: true, - canPerformAuthentication: true + canPerformAuthentication: true, + hasPendingOperation: false ) #expect(authenticating.providerInputsAreDisabled) #expect(authenticating.apiKeySubmitIsDisabled) #expect(authenticating.showsCancelAction) + + let pending = SignInView.ControlState( + apiKeyIsEmpty: false, + isAuthenticating: false, + canPerformAuthentication: true, + hasPendingOperation: true + ) + #expect(pending.providerInputsAreDisabled) + #expect(pending.apiKeySubmitIsDisabled) + #expect(pending.showsCancelAction) } @Test func nonChatGPTAccountsUseProviderNamesInsteadOfEmailMasking() { @@ -161,6 +174,24 @@ struct ReviewMonitorAddAccountActionTests { #expect(session.screen == .options) } + @Test func repeatedSubmissionWhileRuntimeStartsRunsOneAuthentication() async throws { + let startGate = AsyncGate() + let backend = BlockingSignInStartBackend(startGate: startGate) + let store = CodexReviewStore.makeTestingStore(backend: backend) + let session = ReviewMonitorSignInSession() + let apiKey = try CodexReviewAPIKey(validating: "sk-ui-test-secret") + let submission = ReviewMonitorAuthenticationSubmission(method: .apiKey(apiKey)) + store.loadForTesting(serverState: .failed("Runtime failed."), authPhase: .signedOut) + + session.authenticate(submission, store: store) + session.authenticate(submission, store: store) + await startGate.waitUntilBlocked() + await startGate.open() + await session.waitUntilIdleForTesting() + + #expect(backend.authenticationRequests.count == 1) + } + @Test func closingSignInSessionRestoresPrimaryOptions() async { let store = CodexReviewStore.makePreviewStore() let session = ReviewMonitorSignInSession() From ee68c406ea12111d225b540dba87b3171d79115f Mon Sep 17 00:00:00 2001 From: Kazuki Nakashima <65545348+lynnswap@users.noreply.github.com> Date: Sun, 9 Aug 2026 12:24:19 +0900 Subject: [PATCH 5/5] fix(auth): preserve cancellation through login admission --- .../AccountRuntimeTransitionCoordinator.swift | 38 +++++++-- .../LiveCodexReviewStoreBackend.swift | 22 +++++ Sources/CodexReviewHost/LoginSession.swift | 21 ++++- .../CodexReviewHostTests.swift | 84 +++++++++++++++++++ 4 files changed, 155 insertions(+), 10 deletions(-) diff --git a/Sources/CodexReviewHost/AccountRuntimeTransitionCoordinator.swift b/Sources/CodexReviewHost/AccountRuntimeTransitionCoordinator.swift index afa0c1e..8fac15a 100644 --- a/Sources/CodexReviewHost/AccountRuntimeTransitionCoordinator.swift +++ b/Sources/CodexReviewHost/AccountRuntimeTransitionCoordinator.swift @@ -141,7 +141,7 @@ final class AccountRuntimeTransitionCoordinator { generation: UInt64, phase: RuntimeAuthPhase ) - case loginAdmission(id: UUID) + case loginAdmission(id: UUID, cancellationRequested: Bool) case primaryLogin(id: UUID) var id: UUID { @@ -150,7 +150,7 @@ final class AccountRuntimeTransitionCoordinator { .primaryReconciliation(let id, _), .explicitRuntimeStart(let id, _, _), .runtimeAuthReconciliation(let id, _, _), - .loginAdmission(let id), + .loginAdmission(let id, _), .primaryLogin(let id): return id } @@ -210,20 +210,44 @@ final class AccountRuntimeTransitionCoordinator { throw CodexReviewAuthenticationFailure.accountMutationBlockedByAuthentication } let id = UUID() - activeTransition = .loginAdmission(id: id) + activeTransition = .loginAdmission(id: id, cancellationRequested: false) return .init(id: id) } func canCommitLoginAdmission(_ admission: LoginAdmission) -> Bool { - guard case .loginAdmission(let id) = activeTransition, - id == admission.id else { + guard case .loginAdmission(let id, let cancellationRequested) = activeTransition, + id == admission.id, + cancellationRequested == false else { return false } return canPublish } + func requestLoginAdmissionCancellation() -> LoginAdmission? { + guard case .loginAdmission(let id, cancellationRequested: false) = activeTransition else { + return nil + } + activeTransition = .loginAdmission(id: id, cancellationRequested: true) + return .init(id: id) + } + + func isLoginAdmissionCancellationRequested(_ admission: LoginAdmission) -> Bool { + guard case .loginAdmission(let id, let cancellationRequested) = activeTransition, + id == admission.id else { + return false + } + return cancellationRequested + } + + func waitForLoginAdmissionCompletion(_ admission: LoginAdmission) async { + guard activeTransition?.id == admission.id else { + return + } + await waitForTransitionCompletion() + } + func finishLoginAdmission(_ admission: LoginAdmission) { - guard case .loginAdmission(let id) = activeTransition, + guard case .loginAdmission(let id, _) = activeTransition, id == admission.id else { preconditionFailure("Only the active login admission can finish.") } @@ -231,7 +255,7 @@ final class AccountRuntimeTransitionCoordinator { } func retainPrimaryLoginAdmission(_ admission: LoginAdmission) { - guard case .loginAdmission(let id) = activeTransition, + guard case .loginAdmission(let id, cancellationRequested: false) = activeTransition, id == admission.id else { preconditionFailure("Only the active login admission can retain primary runtime ownership.") } diff --git a/Sources/CodexReviewHost/LiveCodexReviewStoreBackend.swift b/Sources/CodexReviewHost/LiveCodexReviewStoreBackend.swift index e381343..dd83b93 100644 --- a/Sources/CodexReviewHost/LiveCodexReviewStoreBackend.swift +++ b/Sources/CodexReviewHost/LiveCodexReviewStoreBackend.swift @@ -1447,6 +1447,11 @@ private final class LiveCodexReviewStoreBackend: CodexReviewStoreBackend { func cancelAuthentication(auth _: CodexReviewAuthModel) async { guard let session = loginSession else { + if let admission = accountRuntimeTransitionCoordinator + .requestLoginAdmissionCancellation() { + await accountRuntimeTransitionCoordinator.waitForLoginAdmissionCompletion(admission) + return + } if let activePrimaryAuthenticationReconciliation { _ = await activePrimaryAuthenticationReconciliation.finalResult.wait() } @@ -2002,6 +2007,11 @@ private final class LiveCodexReviewStoreBackend: CodexReviewStoreBackend { do { try await attachedStore?.requireReviewThreadRetentionAcceptance() guard accountRuntimeTransitionCoordinator.canCommitLoginAdmission(loginAdmission) else { + if accountRuntimeTransitionCoordinator + .isLoginAdmissionCancellationRequested(loginAdmission) { + accountRuntimeTransitionCoordinator.finishLoginAdmission(loginAdmission) + return + } throw CodexReviewAuthenticationFailure.accountMutationBlockedByAuthentication } authenticationMutation = try await accountRegistry.beginAuthenticationMutation( @@ -2012,10 +2022,18 @@ private final class LiveCodexReviewStoreBackend: CodexReviewStoreBackend { throw error } let mutationLease = authenticationMutation.lease + let cancellationRequested = accountRuntimeTransitionCoordinator + .isLoginAdmissionCancellationRequested(loginAdmission) guard accountRuntimeTransitionCoordinator.canCommitLoginAdmission(loginAdmission), loginSession == nil else { + if cancellationRequested { + await accountRegistry.requestAuthenticationCancellation(mutationLease) + } await accountRegistry.finishMutation(mutationLease) accountRuntimeTransitionCoordinator.finishLoginAdmission(loginAdmission) + if cancellationRequested { + return + } throw CodexReviewAuthenticationFailure.accountMutationBlockedByAuthentication } let purpose = authenticationMutation.purpose @@ -2162,6 +2180,10 @@ private final class LiveCodexReviewStoreBackend: CodexReviewStoreBackend { return finish(.cancelled) } await self?.authenticationOperationDidBind?() + guard case .proceed = await operationState.claimAPIKeyRequest() else { + await startCompletion.resolve(.success(())) + return finish(.cancelled) + } auth?.updatePhase(.signingIn(.init( title: "Sign in to Codex", detail: "Authenticating with API key." diff --git a/Sources/CodexReviewHost/LoginSession.swift b/Sources/CodexReviewHost/LoginSession.swift index cde24bc..3d24e40 100644 --- a/Sources/CodexReviewHost/LoginSession.swift +++ b/Sources/CodexReviewHost/LoginSession.swift @@ -191,6 +191,7 @@ actor LoginOperationState { case runtimeBound(LoginRuntime) case loginPending(LoginRuntime, CodexLoginHandle) case apiKeyPending(LoginRuntime) + case apiKeyRequestClaimed(LoginRuntime) case resourcesTaken } @@ -214,7 +215,7 @@ actor LoginOperationState { switch phase { case .loginPending(_, let handle): action = .chatGPT(handle) - case .apiKeyPending: + case .apiKeyPending, .apiKeyRequestClaimed: action = .apiKeyRootTask case .acquiringRuntime, .runtimeBound, .resourcesTaken: return nil @@ -282,6 +283,18 @@ actor LoginOperationState { return .cancel } + func claimAPIKeyRequest() -> BindDisposition { + guard case .apiKeyPending(let runtime) = phase else { + preconditionFailure("An API-key request can be claimed only once after binding its runtime.") + } + guard cancellationRequested == false else { + cancellationClaimed = true + return .cancel + } + phase = .apiKeyRequestClaimed(runtime) + return .proceed + } + func claimURLPresentation(handle: CodexLoginHandle) -> BindDisposition { guard case .loginPending(_, let boundHandle) = phase, boundHandle == handle else { @@ -299,7 +312,8 @@ actor LoginOperationState { switch phase { case .acquiringRuntime, .resourcesTaken: return nil - case .runtimeBound(let runtime), .loginPending(let runtime, _), .apiKeyPending(let runtime): + case .runtimeBound(let runtime), .loginPending(let runtime, _), + .apiKeyPending(let runtime), .apiKeyRequestClaimed(let runtime): return runtime } } @@ -313,7 +327,8 @@ actor LoginOperationState { func takeOwnedRuntime() -> LoginRuntime? { switch phase { - case .runtimeBound(let runtime), .loginPending(let runtime, _), .apiKeyPending(let runtime): + case .runtimeBound(let runtime), .loginPending(let runtime, _), + .apiKeyPending(let runtime), .apiKeyRequestClaimed(let runtime): guard runtime.usesPrimaryRuntime == false else { return nil } diff --git a/Tests/CodexReviewHostTests/CodexReviewHostTests.swift b/Tests/CodexReviewHostTests/CodexReviewHostTests.swift index 4245848..30ddef0 100644 --- a/Tests/CodexReviewHostTests/CodexReviewHostTests.swift +++ b/Tests/CodexReviewHostTests/CodexReviewHostTests.swift @@ -131,6 +131,19 @@ private extension CodexReviewStore { @Suite("host composition") @MainActor struct CodexReviewHostTests { + @Test func loginAdmissionCancellationPreventsSessionCommit() async throws { + let coordinator = AccountRuntimeTransitionCoordinator() + let admission = try coordinator.reserveLoginAdmission() + + let cancellation = try #require(coordinator.requestLoginAdmissionCancellation()) + + #expect(coordinator.isLoginAdmissionCancellationRequested(admission)) + #expect(coordinator.canCommitLoginAdmission(admission) == false) + coordinator.finishLoginAdmission(cancellation) + await coordinator.waitForLoginAdmissionCompletion(admission) + #expect(coordinator.hasActiveLoginTransition == false) + } + @Test func stoppedPrimaryReconciliationWaitsOnlyForItsReservedTransition() async throws { let coordinator = AccountRuntimeTransitionCoordinator() var admittedLogin: AccountRuntimeTransitionCoordinator.LoginAdmission? @@ -1154,6 +1167,50 @@ struct CodexReviewHostTests { await store.stop() } + @Test func liveStoreCancelsAPIKeyLoginWhileMutationIsAdmitting() async throws { + let homeURL = try temporaryHome() + let transport = FakeCodexAppServerTransport() + try await transport.enqueueAccount(nil, requiresOpenAIAuth: false) + try await transport.enqueueConfiguration(try makeHostConfigurationReadResult()) + try await transport.enqueueModels(.init(models: [])) + try await transport.enqueueAPIKeyLogin() + let mutationGate = CodexAppServerTestGate() + let cancellationStarted = MainActorOneShotSignal() + let cancellationRequested = OneShotSignal() + let store = CodexReviewStore.makeLiveStoreForTesting( + environment: ["HOME": homeURL.path], + authenticationMutationDidBegin: { + await mutationGate.waitIgnoringCancellation() + }, + authenticationCancellationDidRequest: { + await cancellationRequested.signal() + }, + transport: transport + ) + await store.start(forceRestartIfNeeded: true) + let login = Task { @MainActor in + try await store.signIn(using: .apiKey( + try CodexReviewAPIKey(validating: "test-secret-admission") + )) + } + await mutationGate.waitUntilBlocked() + + let cancellation = Task { @MainActor in + cancellationStarted.signal() + await store.cancelAuthentication() + } + await cancellationStarted.wait() + await mutationGate.open() + await cancellationRequested.wait() + try await login.value + await cancellation.value + + #expect(await transport.recordedRequests(for: .accountLoginStart).isEmpty) + #expect(store.auth.selectedAccount == nil) + #expect(FileManager.default.fileExists(atPath: accountRegistryURL(homeURL: homeURL).path) == false) + await store.stop() + } + @Test func liveStoreCompletesCommittedAPIKeyLoginAfterPostWriteCancellation() async throws { let homeURL = try temporaryHome() let codexHomeURL = homeURL.appendingPathComponent(".codex_review", isDirectory: true) @@ -6621,6 +6678,33 @@ private actor OneShotSignal { } } +@MainActor +private final class MainActorOneShotSignal { + private var isSignaled = false + private var waiters: [CheckedContinuation] = [] + + func signal() { + guard isSignaled == false else { + return + } + isSignaled = true + let waiters = waiters + self.waiters.removeAll(keepingCapacity: false) + for waiter in waiters { + waiter.resume() + } + } + + func wait() async { + guard isSignaled == false else { + return + } + await withCheckedContinuation { continuation in + waiters.append(continuation) + } + } +} + private actor ArmableAsyncHookGate { private var armed = false private let gate = CodexAppServerTestGate()