From c9ef498219768cf8075f363b9b6a3bbaae13d129 Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:49:57 +0800 Subject: [PATCH 01/10] fix: isolate browser cookie import from XCTest --- .../Sources/CLIPulseCore/CookieResolver.swift | 20 +++++++++++++++++++ .../CookieResolverTests.swift | 7 +++++++ 2 files changed, 27 insertions(+) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/CookieResolver.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/CookieResolver.swift index 9ef2900d..ed35fb45 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/CookieResolver.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/CookieResolver.swift @@ -30,10 +30,30 @@ public enum CookieResolutionResult: Sendable { } public enum CookieResolver { + /// Unit tests must never inherit the production browser importer. Reading + /// Chrome/Edge cookie stores can cross into the user's login Keychain and + /// present an interactive consent dialog from an XCTest process. + private static let isRunningUnderXCTest: Bool = { + let environment = ProcessInfo.processInfo.environment + let bundlePath = Bundle.main.bundleURL.path + let processName = ProcessInfo.processInfo.processName.lowercased() + + return NSClassFromString("XCTestCase") != nil + || environment["XCTestConfigurationFilePath"] != nil + || environment["XCTestBundlePath"] != nil + || bundlePath.hasSuffix(".xctest") + || bundlePath.contains(".xctest/") + || processName == "xctest" + || processName.hasSuffix("tests.xctest") + }() + /// Platform-appropriate default importer: SweetCookieKit on macOS, /// a no-op everywhere else. public static var platformDefaultImporter: CookieImporting { #if os(macOS) + if isRunningUnderXCTest { + return NullCookieImporter() + } return BrowserCookieAutoImporter.shared #else return NullCookieImporter() diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/CookieResolverTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/CookieResolverTests.swift index e52a4220..083f8b91 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/CookieResolverTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/CookieResolverTests.swift @@ -90,4 +90,11 @@ final class CookieResolverTests: XCTestCase { importer: NullCookieImporter()) if case .unavailable = result {} else { XCTFail("NullCookieImporter must yield .unavailable") } } + + func test_platform_default_importer_is_null_in_xctest_runtime() { + XCTAssertTrue( + CookieResolver.platformDefaultImporter is NullCookieImporter, + "xctest must never receive the importer that reads real browser cookies or Keychain entries" + ) + } } From e588a23b1c3ae58acb0ab4caa0ce5851286b7ce6 Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:54:46 +0800 Subject: [PATCH 02/10] fix: fail closed on ambiguous helper account attribution --- .../CLIPulseCore/DataRefreshManager.swift | 40 +++++- .../HelperProviderAccountsIPCTests.swift | 122 +++++++++++++++++- 2 files changed, 154 insertions(+), 8 deletions(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/DataRefreshManager.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/DataRefreshManager.swift index 72d23ccb..ad2dde6a 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/DataRefreshManager.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/DataRefreshManager.swift @@ -1470,7 +1470,10 @@ internal final class DataRefreshManager { nonisolated static func parseHelperCollectorResults( _ data: Data, providerConfigs: [ProviderConfig], - now: Date = Date() + now: Date = Date(), + sharedCredentialOwner: (ProviderKind) -> ProviderSharedCredentialOwner.Lookup = { + ProviderSharedCredentialOwner.lookup(kind: $0) + } ) -> HelperCollectorSnapshot { guard let decoded = try? HelperIPC.decodeCollectorResults(data, now: now) else { return .empty @@ -1499,12 +1502,35 @@ internal final class DataRefreshManager { ) } let accountResults = providerResults.compactMap { result -> AccountScopedCollectorResult? in - guard let kind = ProviderKind(rawValue: result.usage.provider), - let config = providerConfigs - .filter({ $0.kind == kind && $0.isEnabled }) - .sorted(by: providerConfigComesBefore) - .first - else { return nil } + guard let kind = ProviderKind(rawValue: result.usage.provider) else { + return nil + } + let usesSharedCredentials = kind == .claude || kind == .gemini + let eligible = providerConfigs + .filter { + guard $0.kind == kind && $0.isEnabled else { + return false + } + return !usesSharedCredentials + || $0.sharedCredentialFallbackDisabled != true + } + .sorted(by: providerConfigComesBefore) + let config: ProviderConfig? + + if usesSharedCredentials { + switch sharedCredentialOwner(kind) { + case let .owned(accountID): + config = eligible.first { $0.accountID == accountID } + case .unowned: + config = eligible.count == 1 ? eligible[0] : nil + case .unavailable, .corrupt: + config = nil + } + } else { + config = eligible.count == 1 ? eligible[0] : nil + } + + guard let config else { return nil } return AccountScopedCollectorResult( accountID: config.accountID, config: config, diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/HelperProviderAccountsIPCTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/HelperProviderAccountsIPCTests.swift index 16a7d959..d647a5a5 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/HelperProviderAccountsIPCTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/HelperProviderAccountsIPCTests.swift @@ -33,7 +33,8 @@ final class HelperProviderAccountsIPCTests: XCTestCase { let snapshot = DataRefreshManager.parseHelperCollectorResults( data, providerConfigs: [config], - now: now + now: now, + sharedCredentialOwner: { _ in .unowned } ) XCTAssertEqual(snapshot.providerResults.count, 1) @@ -42,6 +43,110 @@ final class HelperProviderAccountsIPCTests: XCTestCase { XCTAssertEqual(snapshot.accountResults.first?.result.usage.remaining, 64) } + func testV1SharedProviderMapsToPersistedOwnerInsteadOfFirstSortOrder() throws { + let firstID = try XCTUnwrap(UUID(uuidString: "12121212-1212-4212-8212-121212121212")) + let ownerID = try XCTUnwrap(UUID(uuidString: "34343434-3434-4434-8434-343434343434")) + let configs = [ + ProviderConfig(kind: .claude, accountID: firstID, isEnabled: true, sortOrder: 0), + ProviderConfig(kind: .claude, accountID: ownerID, isEnabled: true, sortOrder: 1), + ] + + let snapshot = DataRefreshManager.parseHelperCollectorResults( + makeV1Data(provider: .claude), + providerConfigs: configs, + now: now, + sharedCredentialOwner: { kind in + kind == .claude ? .owned(ownerID) : .unowned + } + ) + + XCTAssertEqual(snapshot.accountResults.count, 1) + XCTAssertEqual(snapshot.accountResults.first?.accountID, ownerID) + } + + func testV1SharedProviderDoesNotGuessBetweenUnownedAccounts() throws { + let firstID = try XCTUnwrap(UUID(uuidString: "56565656-5656-4656-8656-565656565656")) + let secondID = try XCTUnwrap(UUID(uuidString: "78787878-7878-4878-8878-787878787878")) + let configs = [ + ProviderConfig(kind: .claude, accountID: firstID, isEnabled: true, sortOrder: 0), + ProviderConfig(kind: .claude, accountID: secondID, isEnabled: true, sortOrder: 1), + ] + + let snapshot = DataRefreshManager.parseHelperCollectorResults( + makeV1Data(provider: .claude), + providerConfigs: configs, + now: now, + sharedCredentialOwner: { _ in .unowned } + ) + + XCTAssertTrue(snapshot.accountResults.isEmpty) + XCTAssertEqual(snapshot.providerResults.count, 1) + } + + func testV1SharedProviderFailsClosedForInvalidOwnerState() throws { + let accountID = try XCTUnwrap(UUID(uuidString: "89898989-8989-4989-8989-898989898989")) + let staleOwnerID = try XCTUnwrap(UUID(uuidString: "90909090-9090-4090-8090-909090909090")) + let config = ProviderConfig(kind: .claude, accountID: accountID, isEnabled: true) + let invalidStates: [ProviderSharedCredentialOwner.Lookup] = [ + .unavailable, + .corrupt, + .owned(staleOwnerID), + ] + + for ownerState in invalidStates { + let snapshot = DataRefreshManager.parseHelperCollectorResults( + makeV1Data(provider: .claude), + providerConfigs: [config], + now: now, + sharedCredentialOwner: { _ in ownerState } + ) + + XCTAssertTrue( + snapshot.accountResults.isEmpty, + "owner state \(ownerState) must not be guessed" + ) + XCTAssertEqual(snapshot.providerResults.count, 1) + } + } + + func testV1SharedProviderRejectsFallbackDisabledAccount() throws { + let accountID = try XCTUnwrap(UUID(uuidString: "93939393-9393-4393-8393-939393939393")) + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + isEnabled: true, + sharedCredentialFallbackDisabled: true + ) + + let snapshot = DataRefreshManager.parseHelperCollectorResults( + makeV1Data(provider: .claude), + providerConfigs: [config], + now: now, + sharedCredentialOwner: { _ in .owned(accountID) } + ) + + XCTAssertTrue(snapshot.accountResults.isEmpty) + XCTAssertEqual(snapshot.providerResults.count, 1) + } + + func testV1NonSharedProviderDoesNotGuessBetweenAccounts() throws { + let firstID = try XCTUnwrap(UUID(uuidString: "91919191-9191-4191-8191-919191919191")) + let secondID = try XCTUnwrap(UUID(uuidString: "92929292-9292-4292-8292-929292929292")) + let configs = [ + ProviderConfig(kind: .codex, accountID: firstID, isEnabled: true, sortOrder: 0), + ProviderConfig(kind: .codex, accountID: secondID, isEnabled: true, sortOrder: 1), + ] + + let snapshot = DataRefreshManager.parseHelperCollectorResults( + makeV1Data(provider: .codex), + providerConfigs: configs, + now: now + ) + + XCTAssertTrue(snapshot.accountResults.isEmpty) + XCTAssertEqual(snapshot.providerResults.count, 1) + } + func testV1DisabledProviderProjectionIsRejected() throws { let accountID = try XCTUnwrap(UUID(uuidString: "77777777-7777-4777-8777-777777777777")) let data = Data(""" @@ -506,5 +611,20 @@ final class HelperProviderAccountsIPCTests: XCTestCase { metadata: nil ) } + + private func makeV1Data(provider: ProviderKind) -> Data { + Data(""" + { + "timestamp": "2026-03-21T04:00:00Z", + "providers": { + "\(provider.rawValue)": { + "quota": 100, + "remaining": 64, + "tiers": [] + } + } + } + """.utf8) + } } #endif From dae987e94c76db3c9d71730bd8c364db0c23b043 Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Sun, 9 Aug 2026 18:59:17 +0800 Subject: [PATCH 03/10] fix: make provider account saves recoverable --- .../CLI Pulse Bar/ProviderConfigEditor.swift | 147 ++++++- .../Sources/CLIPulseCore/AppState.swift | 121 +++++- .../Sources/CLIPulseCore/KeychainHelper.swift | 73 +++- .../ProviderAccountSaveTransaction.swift | 65 +++ .../Sources/CLIPulseCore/ProviderConfig.swift | 175 +++++++- .../ProviderConfigMetadataStore.swift | 166 ++++++++ .../ProviderSharedCredentialOwner.swift | 112 +++++ ...roviderAccountKeychainMigrationTests.swift | 163 ++++++- .../ProviderAccountSaveTransactionTests.swift | 171 ++++++++ .../ProviderConfigMetadataStoreTests.swift | 400 ++++++++++++++++++ .../ProviderSharedCredentialOwnerTests.swift | 82 ++++ 11 files changed, 1635 insertions(+), 40 deletions(-) create mode 100644 CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountSaveTransaction.swift create mode 100644 CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountSaveTransactionTests.swift diff --git a/CLI Pulse Bar/CLI Pulse Bar/ProviderConfigEditor.swift b/CLI Pulse Bar/CLI Pulse Bar/ProviderConfigEditor.swift index 1c00b412..7dadfb41 100644 --- a/CLI Pulse Bar/CLI Pulse Bar/ProviderConfigEditor.swift +++ b/CLI Pulse Bar/CLI Pulse Bar/ProviderConfigEditor.swift @@ -835,11 +835,28 @@ struct ProviderConfigEditor: View { @discardableResult private func save() -> Bool { + state.withProviderAccountPersistenceLock(or: false) { + saveAssumingProviderAccountPersistenceLock() + } + } + + @discardableResult + private func saveAssumingProviderAccountPersistenceLock() + -> Bool + { guard let idx = state.providerConfigs.firstIndex( where: { $0.accountID == accountID } ) else { return false } + guard state.recoverPendingProviderAccountSave(accountID) else { + #if os(macOS) + testState = .failure( + "Could not restore the previous provider configuration. Please retry before saving new changes." + ) + #endif + return false + } guard state.persistProviderAccountCredentialRecoveryAnchor( accountID @@ -852,22 +869,39 @@ struct ProviderConfigEditor: View { #endif return false } - #if os(macOS) - if kind == .gemini && allowsLiveProviderActions { + let secretCheckpoint: + ProviderConfig.SecretPersistenceCheckpoint? + if allowsLiveProviderActions { guard - geminiCredentialDraft.commit( - accountID: accountID - ) + let captured = + state.providerConfigs[idx] + .makeSecretPersistenceCheckpoint() else { + #if os(macOS) testState = .failure( - GeminiOAuthError - .credentialPersistenceFailed - .localizedDescription + "Could not safely read existing provider credentials. Please retry." ) + #endif return false } + secretCheckpoint = captured + } else { + secretCheckpoint = nil + } + let secretRecoveryConfig = state.providerConfigs[idx] + guard + let persistenceCheckpoint = + state.makeProviderAccountPersistenceCheckpoint( + accountID + ) + else { + #if os(macOS) + testState = .failure( + "Could not safely prepare provider configuration. Please retry." + ) + #endif + return false } - #endif state.providerConfigs[idx].sourceMode = sourceMode state.providerConfigs[idx].accountLabel = accountLabel.isEmpty ? nil : accountLabel state.providerConfigs[idx].setPlanOverride( @@ -899,24 +933,99 @@ struct ProviderConfigEditor: View { ? true : nil #endif - if allowsLiveProviderActions { - guard state.providerConfigs[idx].saveSecrets() else { + let transactionResult = ProviderAccountSaveTransaction.commit( + persistSecrets: { + guard allowsLiveProviderActions else { + return true + } + guard state.providerConfigs[idx].saveSecrets() else { + #if os(macOS) + testState = .failure( + "Could not safely save provider credentials. Please retry." + ) + #endif + return false + } + return true + }, + rollbackSecrets: { + guard allowsLiveProviderActions else { + return true + } + guard let secretCheckpoint else { + return false + } + return secretRecoveryConfig + .restoreSecrets( + from: secretCheckpoint + ) + }, + persistMetadata: { + guard state.persistProviderAccountDraftMetadata(accountID) else { + #if os(macOS) + testState = .failure( + "Could not safely save provider configuration. Please retry." + ) + #endif + return false + } + return true + }, + rollbackMetadata: { + persistenceCheckpoint.restore() + }, + commitProviderCredential: { #if os(macOS) - testState = .failure( - "Could not safely save provider credentials. Please retry." - ) + guard kind == .gemini && allowsLiveProviderActions else { + return true + } + guard geminiCredentialDraft.commit(accountID: accountID) else { + testState = .failure( + GeminiOAuthError + .credentialPersistenceFailed + .localizedDescription + ) + return false + } #endif - return false + return true + }, + finalize: { + state.finalizeProviderAccountDraft(accountID) } - } - guard state.commitProviderAccountDraft(accountID) else { + ) + switch transactionResult { + case .committed: + return true + case .failedRolledBack: + return false + case .failedRollbackIncomplete: + let recovery = ProviderAccountSaveRecovery( + restoreMetadata: { + persistenceCheckpoint.restore() + }, + restoreSecrets: { + guard allowsLiveProviderActions else { + return true + } + guard let secretCheckpoint else { + return false + } + return secretRecoveryConfig.restoreSecrets( + from: secretCheckpoint + ) + } + ) + state.retainProviderAccountSaveRecovery( + recovery, + for: accountID + ) #if os(macOS) testState = .failure( - "Could not safely save provider configuration. Please retry." + "Save failed and recovery is incomplete. Retry to restore the previous configuration before saving again." ) #endif return false } - return true } } diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift index 3cbf5078..96547755 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift @@ -74,6 +74,8 @@ public final class AppState: ObservableObject { // computed forwarders so setProviderEnabled/toggleProvider/DemoDataProvider/ // DataRefreshManager-payload-assign compile unchanged. public let providerState = ProviderState() + private var pendingProviderAccountSaveRecoveries: + [UUID: ProviderAccountSaveRecovery] = [:] public var providers: [ProviderUsage] { get { providerState.providers } @@ -1431,9 +1433,91 @@ public final class AppState: ObservableObject { return resolvedStore.save(providerConfigs) } + public func makeProviderAccountPersistenceCheckpoint( + _ accountID: UUID, + using metadataStore: ProviderConfigMetadataStore? = nil + ) -> ProviderAccountPersistenceCheckpoint? { + guard providerConfigs.contains(where: { + $0.accountID == accountID + }) else { + return nil + } + let allowsHelperMirror = + runtimeEnvironment.capabilities.allowsHelperRegistration + let resolvedStore = metadataStore ?? ProviderConfigMetadataStore( + defaults: providerConfigDefaults, + helperDefaults: allowsHelperMirror + ? providerConfigHelperDefaults + : nil + ) + let ownerCheckpoint: + ProviderSharedCredentialOwner.PersistenceCheckpoint? + if allowsHelperMirror { + guard + let captured = + ProviderSharedCredentialOwner + .makePersistenceCheckpoint() + else { + return nil + } + ownerCheckpoint = captured + } else { + ownerCheckpoint = nil + } + return ProviderAccountPersistenceCheckpoint( + metadataStore: resolvedStore, + metadataCheckpoint: + resolvedStore.makePersistenceCheckpoint(), + ownerCheckpoint: ownerCheckpoint, + restoresOwner: allowsHelperMirror + ) + } + + public func withProviderAccountPersistenceLock( + or failure: T, + _ body: () -> T + ) -> T { + ProviderSharedCredentialOwner.withPersistenceLock( + or: failure, + body + ) + } + + public func retainProviderAccountSaveRecovery( + _ recovery: ProviderAccountSaveRecovery, + for accountID: UUID + ) { + // Never replace the original baseline with one captured from a + // partially compensated state. + guard pendingProviderAccountSaveRecoveries[accountID] == nil else { + return + } + pendingProviderAccountSaveRecoveries[accountID] = recovery + } + + /// Retry an incomplete compensation before the editor is allowed to + /// capture a fresh baseline. A failed recovery remains pending. @discardableResult - public func commitProviderAccountDraft( + public func recoverPendingProviderAccountSave( _ accountID: UUID + ) -> Bool { + guard + let recovery = + pendingProviderAccountSaveRecoveries[accountID] + else { + return true + } + guard recovery.recover() else { + return false + } + pendingProviderAccountSaveRecoveries[accountID] = nil + return true + } + + @discardableResult + public func persistProviderAccountDraftMetadata( + _ accountID: UUID, + using metadataStore: ProviderConfigMetadataStore? = nil ) -> Bool { guard providerConfigs.contains(where: { $0.accountID == accountID @@ -1442,11 +1526,42 @@ public final class AppState: ObservableObject { } // Persist final metadata while the in-memory draft marker still // exists. A failed write leaves the editor transaction retryable. - guard saveProviderConfigMetadata() else { - return false + let allowsHelperMirror = + runtimeEnvironment.capabilities.allowsHelperRegistration + if allowsHelperMirror { + guard ProviderSharedCredentialOwner.reconcile(configs: providerConfigs) else { + return false + } + } + let resolvedStore = metadataStore ?? ProviderConfigMetadataStore( + defaults: providerConfigDefaults, + helperDefaults: allowsHelperMirror + ? providerConfigHelperDefaults + : nil + ) + return resolvedStore.save(providerConfigs) + } + + /// Complete the in-memory portion only after every fallible persistence + /// step, including provider-specific credential mutation, has succeeded. + public func finalizeProviderAccountDraft(_ accountID: UUID) { + guard providerConfigs.contains(where: { + $0.accountID == accountID + }) else { + return } _ = providerState.commitProviderAccountDraft(accountID) buildProviderDetails() + } + + @discardableResult + public func commitProviderAccountDraft( + _ accountID: UUID + ) -> Bool { + guard persistProviderAccountDraftMetadata(accountID) else { + return false + } + finalizeProviderAccountDraft(accountID) return true } diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/KeychainHelper.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/KeychainHelper.swift index a9a2dda5..d615d184 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/KeychainHelper.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/KeychainHelper.swift @@ -1,6 +1,19 @@ import Foundation import Security +enum ProviderSecretReadResult: Equatable { + case value(String) + case missing + case failure + + var value: String? { + guard case let .value(value) = self else { + return nil + } + return value + } +} + protocol ProviderSecretStoring { @discardableResult func save( @@ -9,10 +22,26 @@ protocol ProviderSecretStoring { accessGroup: String? ) -> Bool func load(key: String, accessGroup: String?) -> String? + func read( + key: String, + accessGroup: String? + ) -> ProviderSecretReadResult @discardableResult func delete(key: String, accessGroup: String?) -> Bool } +extension ProviderSecretStoring { + func read( + key: String, + accessGroup: String? + ) -> ProviderSecretReadResult { + guard let value = load(key: key, accessGroup: accessGroup) else { + return .missing + } + return .value(value) + } +} + struct KeychainProviderSecretStore: ProviderSecretStoring { @discardableResult func save( @@ -31,6 +60,16 @@ struct KeychainProviderSecretStore: ProviderSecretStoring { KeychainHelper.load(key: key, accessGroup: accessGroup) } + func read( + key: String, + accessGroup: String? + ) -> ProviderSecretReadResult { + KeychainHelper.readResult( + key: key, + accessGroup: accessGroup + ) + } + @discardableResult func delete(key: String, accessGroup: String?) -> Bool { KeychainHelper.delete(key: key, accessGroup: accessGroup) @@ -174,7 +213,10 @@ public enum KeychainHelper { return false } - public static func load(key: String, accessGroup: String? = nil) -> String? { + static func readResult( + key: String, + accessGroup: String? = nil + ) -> ProviderSecretReadResult { var query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: service, @@ -186,12 +228,35 @@ public enum KeychainHelper { query[kSecAttrAccessGroup as String] = group } if isRunningUnderXCTest { - return inMemoryStoreForTesting[testStoreKey(key, accessGroup)] + guard + let value = inMemoryStoreForTesting[ + testStoreKey(key, accessGroup) + ] + else { + return .missing + } + return .value(value) } var item: CFTypeRef? let status = SecItemCopyMatching(query as CFDictionary, &item) - guard status == errSecSuccess, let data = item as? Data else { return nil } - return String(data: data, encoding: .utf8) + if status == errSecItemNotFound { + return .missing + } + guard + status == errSecSuccess, + let data = item as? Data, + let value = String(data: data, encoding: .utf8) + else { + return .failure + } + return .value(value) + } + + public static func load( + key: String, + accessGroup: String? = nil + ) -> String? { + readResult(key: key, accessGroup: accessGroup).value } @discardableResult diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountSaveTransaction.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountSaveTransaction.swift new file mode 100644 index 00000000..54ac9287 --- /dev/null +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountSaveTransaction.swift @@ -0,0 +1,65 @@ +/// Orders provider-account persistence and compensates every completed phase +/// when a later phase fails. Each persistence primitive must also restore its +/// own partial writes before returning `false`. +public enum ProviderAccountSaveTransactionResult: Equatable { + case committed + case failedRolledBack + case failedRollbackIncomplete +} + +/// Keeps the original rollback closures alive after an incomplete +/// compensation. Callers must clear this gate successfully before capturing a +/// fresh persistence baseline for the same account. +public final class ProviderAccountSaveRecovery { + private let restoreMetadata: () -> Bool + private let restoreSecrets: () -> Bool + + public init( + restoreMetadata: @escaping () -> Bool, + restoreSecrets: @escaping () -> Bool + ) { + self.restoreMetadata = restoreMetadata + self.restoreSecrets = restoreSecrets + } + + @discardableResult + public func recover() -> Bool { + let metadataRestored = restoreMetadata() + let secretsRestored = restoreSecrets() + return metadataRestored && secretsRestored + } +} + +public enum ProviderAccountSaveTransaction { + @discardableResult + public static func commit( + persistSecrets: () -> Bool, + rollbackSecrets: () -> Bool, + persistMetadata: () -> Bool, + rollbackMetadata: () -> Bool, + commitProviderCredential: () -> Bool, + finalize: () -> Void + ) -> ProviderAccountSaveTransactionResult { + guard persistSecrets() else { + return rollbackSecrets() + ? .failedRolledBack + : .failedRollbackIncomplete + } + guard persistMetadata() else { + let metadataRestored = rollbackMetadata() + let secretsRestored = rollbackSecrets() + return metadataRestored && secretsRestored + ? .failedRolledBack + : .failedRollbackIncomplete + } + guard commitProviderCredential() else { + let metadataRestored = rollbackMetadata() + let secretsRestored = rollbackSecrets() + return metadataRestored && secretsRestored + ? .failedRolledBack + : .failedRollbackIncomplete + } + finalize() + return .committed + } +} diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift index d885e2d4..704b3ba5 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift @@ -153,23 +153,64 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { saveSecrets(using: KeychainProviderSecretStore()) } + /// Opaque rollback point for the account-scoped secret entries and their + /// migration markers. The values stay in memory only for the duration of + /// the editor save transaction. + public struct SecretPersistenceCheckpoint { + fileprivate let apiKey: String? + fileprivate let apiKeyMarker: String? + fileprivate let cookie: String? + fileprivate let cookieMarker: String? + } + + public func makeSecretPersistenceCheckpoint() + -> SecretPersistenceCheckpoint? + { + makeSecretPersistenceCheckpoint( + using: KeychainProviderSecretStore() + ) + } + + @discardableResult + public func restoreSecrets( + from checkpoint: SecretPersistenceCheckpoint + ) -> Bool { + restoreSecrets( + from: checkpoint, + using: KeychainProviderSecretStore() + ) + } + @discardableResult func saveSecrets( using store: any ProviderSecretStoring ) -> Bool { - let apiKeySaved = + guard + let checkpoint = makeSecretPersistenceCheckpoint( + using: store + ) + else { + return false + } + guard persistSecret( apiKey, suffix: "apiKey", using: store - ) - let cookieSaved = + ), persistSecret( manualCookieHeader, suffix: "cookie", using: store ) - return apiKeySaved && cookieSaved + else { + _ = restoreSecrets( + from: checkpoint, + using: store + ) + return false + } + return true } /// Load secrets from Keychain into the in-memory fields. @@ -192,11 +233,114 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { func deleteSecrets( using store: any ProviderSecretStoring ) -> Bool { - let apiKeyDeleted = - persistSecret(nil, suffix: "apiKey", using: store) - let cookieDeleted = + guard + let checkpoint = makeSecretPersistenceCheckpoint( + using: store + ) + else { + return false + } + guard + persistSecret(nil, suffix: "apiKey", using: store), persistSecret(nil, suffix: "cookie", using: store) - return apiKeyDeleted && cookieDeleted + else { + _ = restoreSecrets( + from: checkpoint, + using: store + ) + return false + } + return true + } + + func makeSecretPersistenceCheckpoint( + using store: any ProviderSecretStoring + ) -> SecretPersistenceCheckpoint? { + let group = Self.secretsAccessGroup + let apiKey = store.read( + key: Self.accountKeychainKey(accountID, "apiKey"), + accessGroup: group + ) + let apiKeyMarker = store.read( + key: Self.migrationMarkerKey(accountID, "apiKey"), + accessGroup: group + ) + let cookie = store.read( + key: Self.accountKeychainKey(accountID, "cookie"), + accessGroup: group + ) + let cookieMarker = store.read( + key: Self.migrationMarkerKey(accountID, "cookie"), + accessGroup: group + ) + guard + apiKey != .failure, + apiKeyMarker != .failure, + cookie != .failure, + cookieMarker != .failure + else { + return nil + } + return SecretPersistenceCheckpoint( + apiKey: apiKey.value, + apiKeyMarker: apiKeyMarker.value, + cookie: cookie.value, + cookieMarker: cookieMarker.value + ) + } + + @discardableResult + func restoreSecrets( + from checkpoint: SecretPersistenceCheckpoint, + using store: any ProviderSecretStoring + ) -> Bool { + let group = Self.secretsAccessGroup + let entries: [(String, String?)] = [ + ( + Self.accountKeychainKey(accountID, "apiKey"), + checkpoint.apiKey + ), + ( + Self.migrationMarkerKey(accountID, "apiKey"), + checkpoint.apiKeyMarker + ), + ( + Self.accountKeychainKey(accountID, "cookie"), + checkpoint.cookie + ), + ( + Self.migrationMarkerKey(accountID, "cookie"), + checkpoint.cookieMarker + ), + ] + var restored = true + for (key, value) in entries { + let entryRestored: Bool + if let value { + entryRestored = + store.save( + key: key, + value: value, + accessGroup: group + ) + && store.read( + key: key, + accessGroup: group + ) == .value(value) + } else { + entryRestored = + store.delete( + key: key, + accessGroup: group + ) + && store.read( + key: key, + accessGroup: group + ) == .missing + } + restored = entryRestored && restored + } + return restored } @discardableResult @@ -217,7 +361,12 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { ) else { return false } - guard store.load(key: accountKey, accessGroup: group) == value else { + guard + store.read( + key: accountKey, + accessGroup: group + ) == .value(value) + else { return false } } else { @@ -226,10 +375,10 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { key: accountKey, accessGroup: group ), - store.load( + store.read( key: accountKey, accessGroup: group - ) == nil + ) == .missing else { return false } @@ -242,10 +391,10 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { ) else { return false } - return store.load( + return store.read( key: markerKey, accessGroup: group - ) == "1" + ) == .value("1") } private func loadSecret( diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfigMetadataStore.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfigMetadataStore.swift index 4388e9de..3bee3194 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfigMetadataStore.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfigMetadataStore.swift @@ -5,6 +5,13 @@ import Foundation /// `ProviderConfig.saveSecrets()` explicitly; passive onboarding completion /// uses this store so a transient Keychain read failure cannot erase a secret. public struct ProviderConfigMetadataStore { + struct PersistenceCheckpoint { + let appConfigs: Data? + let appSchemaVersion: Int? + let helperConfigs: Data? + let helperWriteV2: Bool? + } + private let defaults: UserDefaults private let helperDefaults: UserDefaults? private let encoder: JSONEncoder @@ -26,6 +33,7 @@ public struct ProviderConfigMetadataStore { guard let data = try? encoder.encode(configs) else { return false } + let checkpoint = makePersistenceCheckpoint() defaults.set( data, @@ -43,6 +51,7 @@ public struct ProviderConfigMetadataStore { forKey: ProviderAccountMigration.schemaVersionKey ) == ProviderAccountMigration.currentSchemaVersion else { + _ = restore(checkpoint) return false } helperDefaults?.set( @@ -68,9 +77,166 @@ public struct ProviderConfigMetadataStore { .writeDefaultsKey ) else { + _ = restore(checkpoint) return false } } return true } + + func makePersistenceCheckpoint() -> PersistenceCheckpoint { + PersistenceCheckpoint( + appConfigs: defaults.data( + forKey: ProviderAccountMigration.configsKey + ), + appSchemaVersion: + defaults.object( + forKey: + ProviderAccountMigration.schemaVersionKey + ) == nil + ? nil + : defaults.integer( + forKey: + ProviderAccountMigration.schemaVersionKey + ), + helperConfigs: helperDefaults?.data( + forKey: HelperIPC.providerConfigsKey + ), + helperWriteV2: + helperDefaults?.object( + forKey: + HelperIPC.providerAccountsWriteV2Key + ) == nil + ? nil + : helperDefaults?.bool( + forKey: + HelperIPC.providerAccountsWriteV2Key + ) + ) + } + + @discardableResult + func restore( + _ checkpoint: PersistenceCheckpoint + ) -> Bool { + restore( + checkpoint.appConfigs, + in: defaults, + forKey: ProviderAccountMigration.configsKey + ) + restore( + checkpoint.appSchemaVersion, + in: defaults, + forKey: ProviderAccountMigration.schemaVersionKey + ) + if let helperDefaults { + restore( + checkpoint.helperConfigs, + in: helperDefaults, + forKey: HelperIPC.providerConfigsKey + ) + restore( + checkpoint.helperWriteV2, + in: helperDefaults, + forKey: HelperIPC.providerAccountsWriteV2Key + ) + } + + let appRestored = + defaults.data( + forKey: ProviderAccountMigration.configsKey + ) == checkpoint.appConfigs + && optionalInteger( + defaults, + key: ProviderAccountMigration.schemaVersionKey + ) == checkpoint.appSchemaVersion + let helperRestored: Bool + if let helperDefaults { + helperRestored = + helperDefaults.data( + forKey: HelperIPC.providerConfigsKey + ) == checkpoint.helperConfigs + && optionalBool( + helperDefaults, + key: HelperIPC.providerAccountsWriteV2Key + ) == checkpoint.helperWriteV2 + } else { + helperRestored = true + } + return appRestored && helperRestored + } + + private func restore( + _ value: Any?, + in defaults: UserDefaults, + forKey key: String + ) { + if let value { + defaults.set(value, forKey: key) + } else { + defaults.removeObject(forKey: key) + } + } + + private func optionalInteger( + _ defaults: UserDefaults, + key: String + ) -> Int? { + defaults.object(forKey: key) == nil + ? nil + : defaults.integer(forKey: key) + } + + private func optionalBool( + _ defaults: UserDefaults, + key: String + ) -> Bool? { + defaults.object(forKey: key) == nil + ? nil + : defaults.bool(forKey: key) + } +} + +/// Opaque rollback point spanning both provider metadata copies and the +/// provider-global compatibility owner records. +public final class ProviderAccountPersistenceCheckpoint { + private let metadataStore: ProviderConfigMetadataStore + private let metadataCheckpoint: + ProviderConfigMetadataStore.PersistenceCheckpoint + private let ownerCheckpoint: + ProviderSharedCredentialOwner.PersistenceCheckpoint? + private let restoresOwner: Bool + + init( + metadataStore: ProviderConfigMetadataStore, + metadataCheckpoint: + ProviderConfigMetadataStore.PersistenceCheckpoint, + ownerCheckpoint: + ProviderSharedCredentialOwner.PersistenceCheckpoint?, + restoresOwner: Bool + ) { + self.metadataStore = metadataStore + self.metadataCheckpoint = metadataCheckpoint + self.ownerCheckpoint = ownerCheckpoint + self.restoresOwner = restoresOwner + } + + @discardableResult + public func restore() -> Bool { + let metadataRestored = metadataStore.restore( + metadataCheckpoint + ) + let ownerRestored: Bool + if restoresOwner { + guard let ownerCheckpoint else { + return false + } + ownerRestored = ProviderSharedCredentialOwner.restore( + ownerCheckpoint + ) + } else { + ownerRestored = true + } + return metadataRestored && ownerRestored + } } diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift index 4830143d..2331871c 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift @@ -21,6 +21,15 @@ enum ProviderSharedCredentialOwner { ] private static let lock = NSLock() + struct PersistenceCheckpoint { + fileprivate struct Entry { + let kind: ProviderKind + let rawValue: String? + } + + fileprivate let entries: [Entry] + } + /// Injectable for focused tests. The production value is shared with the /// helper so both processes make the same legacy-account assignment. static var defaults: UserDefaults? = @@ -48,6 +57,13 @@ enum ProviderSharedCredentialOwner { return false } _ = synchronizeDefaults(defaults) + guard + let checkpoint = persistenceCheckpoint( + defaults: defaults + ) + else { + return false + } let kinds = supportedKinds.sorted { $0.rawValue < $1.rawValue } @@ -107,6 +123,10 @@ enum ProviderSharedCredentialOwner { forKey: key ) else { + _ = restore( + checkpoint, + defaults: defaults + ) return false } } else { @@ -117,6 +137,10 @@ enum ProviderSharedCredentialOwner { forKey: key ) else { + _ = restore( + checkpoint, + defaults: defaults + ) return false } } @@ -126,6 +150,46 @@ enum ProviderSharedCredentialOwner { } } + static func makePersistenceCheckpoint() + -> PersistenceCheckpoint? + { + withMutationLock(or: nil) { + lock.withLock { + guard let defaults else { + return nil + } + _ = synchronizeDefaults(defaults) + return persistenceCheckpoint( + defaults: defaults + ) + } + } + } + + @discardableResult + static func restore( + _ checkpoint: PersistenceCheckpoint + ) -> Bool { + withMutationLock(or: false) { + lock.withLock { + guard let defaults else { + return false + } + return restore( + checkpoint, + defaults: defaults + ) + } + } + } + + static func withPersistenceLock( + or failure: T, + _ body: () -> T + ) -> T { + withMutationLock(or: failure, body) + } + /// Returns true for the existing owner, or atomically claims an unowned /// global source. Production refreshes call `reconcile` before fan-out, /// while this fallback keeps direct strategy calls deterministic. @@ -337,6 +401,54 @@ enum ProviderSharedCredentialOwner { return true } + private static func persistenceCheckpoint( + defaults: UserDefaults + ) -> PersistenceCheckpoint? { + let kinds = supportedKinds.sorted { + $0.rawValue < $1.rawValue + } + var entries: [PersistenceCheckpoint.Entry] = [] + for kind in kinds { + let raw = defaults.object( + forKey: ownerKey(for: kind) + ) + guard raw == nil || raw is String else { + return nil + } + entries.append( + PersistenceCheckpoint.Entry( + kind: kind, + rawValue: raw as? String + ) + ) + } + return PersistenceCheckpoint(entries: entries) + } + + private static func restore( + _ checkpoint: PersistenceCheckpoint, + defaults: UserDefaults + ) -> Bool { + for entry in checkpoint.entries { + let key = ownerKey(for: entry.kind) + if let rawValue = entry.rawValue { + defaults.set(rawValue, forKey: key) + } else { + defaults.removeObject(forKey: key) + } + } + guard synchronizeDefaults(defaults) else { + return false + } + return checkpoint.entries.allSatisfy { entry in + let key = ownerKey(for: entry.kind) + if let rawValue = entry.rawValue { + return defaults.string(forKey: key) == rawValue + } + return defaults.object(forKey: key) == nil + } + } + private static func ownerKey(for kind: ProviderKind) -> String { "cli_pulse_provider_shared_credential_owner_\(kind.rawValue)" } diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift index bbd74e84..f1492343 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift @@ -247,6 +247,139 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { ) } + func testSaveSecretsRestoresBothEntriesWhenSecondWriteFails() + throws + { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "ABABABAB-ABAB-4BAB-8BAB-ABABABABABAB" + ) + ) + let original = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "old-api-key", + manualCookieHeader: "old-cookie" + ) + XCTAssertTrue(original.saveSecrets(using: store)) + store.failingSaveAttemptsByKey[ + accountKey(accountID, "cookie") + ] = 1 + let edited = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "new-api-key", + manualCookieHeader: "new-cookie" + ) + + XCTAssertFalse(edited.saveSecrets(using: store)) + XCTAssertEqual( + store.load( + key: accountKey(accountID, "apiKey"), + accessGroup: ProviderConfig.secretsAccessGroup + ), + "old-api-key" + ) + XCTAssertEqual( + store.load( + key: accountKey(accountID, "cookie"), + accessGroup: ProviderConfig.secretsAccessGroup + ), + "old-cookie" + ) + } + + func testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails() + throws + { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "ACACACAC-ACAC-4CAC-8CAC-ACACACACACAC" + ) + ) + let original = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "old-api-key", + manualCookieHeader: "old-cookie" + ) + XCTAssertTrue(original.saveSecrets(using: store)) + let apiKey = accountKey(accountID, "apiKey") + store.failingLoadKeys.insert(apiKey) + let edited = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "new-api-key", + manualCookieHeader: "new-cookie" + ) + + XCTAssertFalse(edited.saveSecrets(using: store)) + + store.failingLoadKeys.remove(apiKey) + XCTAssertEqual( + store.load( + key: apiKey, + accessGroup: ProviderConfig.secretsAccessGroup + ), + "old-api-key" + ) + XCTAssertEqual( + store.load( + key: accountKey(accountID, "cookie"), + accessGroup: ProviderConfig.secretsAccessGroup + ), + "old-cookie" + ) + } + + func testRestoreMissingSecretReportsReadFailureAfterDelete() + throws + { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "ADADADAD-ADAD-4DAD-8DAD-ADADADADADAD" + ) + ) + let config = ProviderConfig( + kind: .claude, + accountID: accountID + ) + let checkpoint = try XCTUnwrap( + config.makeSecretPersistenceCheckpoint(using: store) + ) + let apiKey = accountKey(accountID, "apiKey") + XCTAssertTrue( + store.save( + key: apiKey, + value: "must-be-removed", + accessGroup: ProviderConfig.secretsAccessGroup + ) + ) + store.failingLoadKeys.insert(apiKey) + + XCTAssertFalse( + config.restoreSecrets( + from: checkpoint, + using: store + ), + "a read failure must not verify deletion as successful" + ) + + store.failingLoadKeys.remove(apiKey) + XCTAssertNil( + store.load( + key: apiKey, + accessGroup: ProviderConfig.secretsAccessGroup + ) + ) + } + private func legacyKey(_ kind: ProviderKind, _ suffix: String) -> String { "cli_pulse_provider_\(kind.rawValue)_\(suffix)" } @@ -264,7 +397,9 @@ private final class InMemoryProviderSecretStore: ProviderSecretStoring { private var values: [Slot: String] = [:] var failingSaveKeys: Set = [] + var failingSaveAttemptsByKey: [String: Int] = [:] var failingDeleteKeys: Set = [] + var failingLoadKeys: Set = [] @discardableResult func save( @@ -275,12 +410,38 @@ private final class InMemoryProviderSecretStore: ProviderSecretStoring { guard !failingSaveKeys.contains(key) else { return false } + if let remaining = failingSaveAttemptsByKey[key], + remaining > 0 + { + failingSaveAttemptsByKey[key] = remaining - 1 + return false + } values[Slot(key: key, accessGroup: accessGroup)] = value return true } func load(key: String, accessGroup: String?) -> String? { - values[Slot(key: key, accessGroup: accessGroup)] + guard !failingLoadKeys.contains(key) else { + return nil + } + return values[Slot(key: key, accessGroup: accessGroup)] + } + + func read( + key: String, + accessGroup: String? + ) -> ProviderSecretReadResult { + guard !failingLoadKeys.contains(key) else { + return .failure + } + guard + let value = values[ + Slot(key: key, accessGroup: accessGroup) + ] + else { + return .missing + } + return .value(value) } @discardableResult diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountSaveTransactionTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountSaveTransactionTests.swift new file mode 100644 index 00000000..3e1d1413 --- /dev/null +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountSaveTransactionTests.swift @@ -0,0 +1,171 @@ +import XCTest +@testable import CLIPulseCore + +final class ProviderAccountSaveTransactionTests: XCTestCase { + func testCredentialMutationRunsAfterAllFallibleConfigWrites() { + var events: [String] = [] + + let result = ProviderAccountSaveTransaction.commit( + persistSecrets: { + events.append("secrets") + return true + }, + rollbackSecrets: { + events.append("rollback-secrets") + return true + }, + persistMetadata: { + events.append("metadata") + return true + }, + rollbackMetadata: { + events.append("rollback-metadata") + return true + }, + commitProviderCredential: { + events.append("credential") + return true + }, + finalize: { + events.append("finalize") + } + ) + + XCTAssertEqual(result, .committed) + XCTAssertEqual(events, ["secrets", "metadata", "credential", "finalize"]) + } + + func testMetadataFailurePreventsCredentialMutationAndFinalization() { + var events: [String] = [] + + let result = ProviderAccountSaveTransaction.commit( + persistSecrets: { + events.append("secrets") + return true + }, + rollbackSecrets: { + events.append("rollback-secrets") + return true + }, + persistMetadata: { + events.append("metadata") + return false + }, + rollbackMetadata: { + events.append("rollback-metadata") + return true + }, + commitProviderCredential: { + events.append("credential") + return true + }, + finalize: { + events.append("finalize") + } + ) + + XCTAssertEqual(result, .failedRolledBack) + XCTAssertEqual( + events, + [ + "secrets", + "metadata", + "rollback-metadata", + "rollback-secrets", + ] + ) + } + + func testCredentialFailureLeavesDraftUnfinalized() { + var events: [String] = [] + + let result = ProviderAccountSaveTransaction.commit( + persistSecrets: { + events.append("secrets") + return true + }, + rollbackSecrets: { + events.append("rollback-secrets") + return true + }, + persistMetadata: { + events.append("metadata") + return true + }, + rollbackMetadata: { + events.append("rollback-metadata") + return true + }, + commitProviderCredential: { + events.append("credential") + return false + }, + finalize: { + events.append("finalize") + } + ) + + XCTAssertEqual(result, .failedRolledBack) + XCTAssertEqual( + events, + [ + "secrets", + "metadata", + "credential", + "rollback-metadata", + "rollback-secrets", + ] + ) + } + + func testSecretFailureRetriesSecretRollbackBeforeReturning() { + var events: [String] = [] + + let result = ProviderAccountSaveTransaction.commit( + persistSecrets: { + events.append("secrets") + return false + }, + rollbackSecrets: { + events.append("rollback-secrets") + return true + }, + persistMetadata: { + events.append("metadata") + return true + }, + rollbackMetadata: { + events.append("rollback-metadata") + return true + }, + commitProviderCredential: { + events.append("credential") + return true + }, + finalize: { + events.append("finalize") + } + ) + + XCTAssertEqual(result, .failedRolledBack) + XCTAssertEqual(events, ["secrets", "rollback-secrets"]) + } + + func testRollbackFailureIsDistinguishableFromCleanCompensation() { + var finalized = false + + let result = ProviderAccountSaveTransaction.commit( + persistSecrets: { true }, + rollbackSecrets: { true }, + persistMetadata: { true }, + rollbackMetadata: { false }, + commitProviderCredential: { false }, + finalize: { + finalized = true + } + ) + + XCTAssertEqual(result, .failedRollbackIncomplete) + XCTAssertFalse(finalized) + } +} diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift index b4fbadd8..f8f33bd6 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift @@ -83,6 +83,62 @@ final class ProviderConfigMetadataStoreTests: XCTestCase { } } + func testHelperWriteFailureRestoresAppAndHelperMetadata() + throws + { + let appSuite = + "ProviderConfigMetadataStoreTests.rollback.app.\(UUID())" + let helperSuite = + "ProviderConfigMetadataStoreTests.rollback.helper.\(UUID())" + let appDefaults = try XCTUnwrap( + UserDefaults(suiteName: appSuite) + ) + let helperDefaults = try XCTUnwrap( + DroppingUserDefaults(suiteName: helperSuite) + ) + defer { + appDefaults.removePersistentDomain(forName: appSuite) + helperDefaults.removePersistentDomain(forName: helperSuite) + } + let accountID = try XCTUnwrap( + UUID( + uuidString: + "ACACACAC-ACAC-4CAC-8CAC-ACACACACACAC" + ) + ) + let original = ProviderConfig( + kind: .claude, + accountID: accountID, + accountLabel: "Original" + ) + let edited = ProviderConfig( + kind: .claude, + accountID: accountID, + accountLabel: "Edited" + ) + let store = ProviderConfigMetadataStore( + defaults: appDefaults, + helperDefaults: helperDefaults + ) + XCTAssertTrue(store.save([original])) + helperDefaults.droppedSetKeys.insert( + HelperIPC.providerConfigsKey + ) + + XCTAssertFalse(store.save([edited])) + for (defaults, key) in [ + (appDefaults, ProviderAccountMigration.configsKey), + (helperDefaults, HelperIPC.providerConfigsKey), + ] { + let data = try XCTUnwrap(defaults.data(forKey: key)) + let configs = try JSONDecoder().decode( + [ProviderConfig].self, + from: data + ) + XCTAssertEqual(configs.first?.accountLabel, "Original") + } + } + @MainActor func testDraftRecoveryAnchorSurvivesRestartBeforeCredentialCommit() throws @@ -159,9 +215,353 @@ final class ProviderConfigMetadataStoreTests: XCTestCase { ) } + @MainActor + func testIncompleteSaveRecoveryIsRetainedUntilCompensationSucceeds() + throws + { + let defaults = try XCTUnwrap( + UserDefaults( + suiteName: + "ProviderConfigMetadataStoreTests.pending-recovery.\(UUID())" + ) + ) + defer { + clear(defaults) + } + let state = AppState( + api: APIClient(), + providerAccountDeletionOutbox: + ProviderAccountDeletionOutbox( + defaults: defaults, + storageKey: + "ProviderConfigMetadataStoreTests.pending-recovery.outbox" + ), + performLaunchSetup: false + ) + let accountID = UUID() + var metadataAttempts = 0 + var secretAttempts = 0 + let recovery = ProviderAccountSaveRecovery( + restoreMetadata: { + metadataAttempts += 1 + return metadataAttempts >= 2 + }, + restoreSecrets: { + secretAttempts += 1 + return true + } + ) + state.retainProviderAccountSaveRecovery( + recovery, + for: accountID + ) + + XCTAssertFalse( + state.recoverPendingProviderAccountSave(accountID) + ) + XCTAssertTrue( + state.recoverPendingProviderAccountSave(accountID) + ) + XCTAssertTrue( + state.recoverPendingProviderAccountSave(accountID), + "a successful recovery must clear the gate" + ) + XCTAssertEqual(metadataAttempts, 2) + XCTAssertEqual(secretAttempts, 2) + } + + #if os(macOS) + @MainActor + func testFinalDraftMetadataPersistenceReconcilesSharedOwner() throws { + let appDefaults = try XCTUnwrap( + UserDefaults( + suiteName: "ProviderConfigMetadataStoreTests.final.app.\(UUID())" + ) + ) + let helperDefaults = try XCTUnwrap( + UserDefaults( + suiteName: "ProviderConfigMetadataStoreTests.final.helper.\(UUID())" + ) + ) + let originalOwnerDefaults = ProviderSharedCredentialOwner.defaults + let originalSynchronizeDefaults = ProviderSharedCredentialOwner.synchronizeDefaults + let originalMutationLock = ProviderSharedCredentialOwner.mutationLock + let lockPath = FileManager.default.temporaryDirectory + .appendingPathComponent("ProviderConfigMetadataStoreTests.final.\(UUID()).lock") + .path + defer { + ProviderSharedCredentialOwner.defaults = originalOwnerDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = originalSynchronizeDefaults + ProviderSharedCredentialOwner.mutationLock = originalMutationLock + clear(appDefaults) + clear(helperDefaults) + try? FileManager.default.removeItem(atPath: lockPath) + } + ProviderSharedCredentialOwner.defaults = helperDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = { _ in true } + ProviderSharedCredentialOwner.mutationLock = GeminiCredentialMutationLock( + lockFilePath: lockPath + ) + + let accountID = try XCTUnwrap( + UUID(uuidString: "CCCCCCCC-CCCC-4CCC-8CCC-CCCCCCCCCCCC") + ) + let state = AppState( + runtimeEnvironment: TestRuntimeFixtures.productionApp, + defaults: appDefaults, + helperDefaults: helperDefaults, + providerAccountDeletionOutbox: ProviderAccountDeletionOutbox( + defaults: appDefaults, + storageKey: "ProviderConfigMetadataStoreTests.final.outbox" + ), + performLaunchSetup: false + ) + state.providerConfigs = [ + ProviderConfig( + kind: .claude, + accountID: accountID, + isEnabled: true + ), + ] + let metadataStore = ProviderConfigMetadataStore( + defaults: appDefaults, + helperDefaults: helperDefaults + ) + + XCTAssertEqual(ProviderSharedCredentialOwner.lookup(kind: .claude), .unowned) + XCTAssertTrue( + state.persistProviderAccountDraftMetadata( + accountID, + using: metadataStore + ) + ) + XCTAssertEqual( + ProviderSharedCredentialOwner.lookup(kind: .claude), + .owned(accountID) + ) + } + + @MainActor + func testOwnerReconcileFailureStopsCredentialCommitAndDraftFinalization() throws { + let appDefaults = try XCTUnwrap( + UserDefaults( + suiteName: "ProviderConfigMetadataStoreTests.failed-owner.app.\(UUID())" + ) + ) + let helperDefaults = try XCTUnwrap( + UserDefaults( + suiteName: "ProviderConfigMetadataStoreTests.failed-owner.helper.\(UUID())" + ) + ) + let originalOwnerDefaults = ProviderSharedCredentialOwner.defaults + let originalSynchronizeDefaults = ProviderSharedCredentialOwner.synchronizeDefaults + let originalMutationLock = ProviderSharedCredentialOwner.mutationLock + let lockPath = FileManager.default.temporaryDirectory + .appendingPathComponent("ProviderConfigMetadataStoreTests.failed-owner.\(UUID()).lock") + .path + defer { + ProviderSharedCredentialOwner.defaults = originalOwnerDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = originalSynchronizeDefaults + ProviderSharedCredentialOwner.mutationLock = originalMutationLock + clear(appDefaults) + clear(helperDefaults) + try? FileManager.default.removeItem(atPath: lockPath) + } + ProviderSharedCredentialOwner.defaults = helperDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = { _ in true } + ProviderSharedCredentialOwner.mutationLock = GeminiCredentialMutationLock( + lockFilePath: lockPath + ) + helperDefaults.set( + "not-a-uuid", + forKey: "cli_pulse_provider_shared_credential_owner_Claude" + ) + + let accountID = try XCTUnwrap( + UUID(uuidString: "DDDDDDDD-DDDD-4DDD-8DDD-DDDDDDDDDDDD") + ) + let state = AppState( + runtimeEnvironment: TestRuntimeFixtures.productionApp, + defaults: appDefaults, + helperDefaults: helperDefaults, + providerAccountDeletionOutbox: ProviderAccountDeletionOutbox( + defaults: appDefaults, + storageKey: "ProviderConfigMetadataStoreTests.failed-owner.outbox" + ), + performLaunchSetup: false + ) + _ = state.addProviderAccount(kind: .gemini, accountID: accountID) + state.providerConfigs[0].isEnabled = true + let metadataStore = ProviderConfigMetadataStore( + defaults: appDefaults, + helperDefaults: helperDefaults + ) + var credentialCommitted = false + + let result = ProviderAccountSaveTransaction.commit( + persistSecrets: { true }, + rollbackSecrets: { true }, + persistMetadata: { + state.persistProviderAccountDraftMetadata( + accountID, + using: metadataStore + ) + }, + rollbackMetadata: { true }, + commitProviderCredential: { + credentialCommitted = true + return true + }, + finalize: { + state.finalizeProviderAccountDraft(accountID) + } + ) + + XCTAssertEqual(result, .failedRolledBack) + XCTAssertFalse(credentialCommitted) + XCTAssertTrue(state.providerState.isProviderAccountDraft(accountID)) + } + + @MainActor + func testCredentialFailureRestoresMetadataAndSharedOwner() + throws + { + let appDefaults = try XCTUnwrap( + UserDefaults( + suiteName: + "ProviderConfigMetadataStoreTests.credential-rollback.app.\(UUID())" + ) + ) + let helperDefaults = try XCTUnwrap( + UserDefaults( + suiteName: + "ProviderConfigMetadataStoreTests.credential-rollback.helper.\(UUID())" + ) + ) + let originalOwnerDefaults = ProviderSharedCredentialOwner.defaults + let originalSynchronizeDefaults = + ProviderSharedCredentialOwner.synchronizeDefaults + let originalMutationLock = + ProviderSharedCredentialOwner.mutationLock + let lockPath = FileManager.default.temporaryDirectory + .appendingPathComponent( + "ProviderConfigMetadataStoreTests.credential-rollback.\(UUID()).lock" + ) + .path + defer { + ProviderSharedCredentialOwner.defaults = originalOwnerDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = + originalSynchronizeDefaults + ProviderSharedCredentialOwner.mutationLock = + originalMutationLock + clear(appDefaults) + clear(helperDefaults) + try? FileManager.default.removeItem(atPath: lockPath) + } + ProviderSharedCredentialOwner.defaults = helperDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = { _ in true } + ProviderSharedCredentialOwner.mutationLock = + GeminiCredentialMutationLock(lockFilePath: lockPath) + + let accountID = try XCTUnwrap( + UUID( + uuidString: + "EFEFEFEF-EFEF-4FEF-8FEF-EFEFEFEFEFEF" + ) + ) + let state = AppState( + runtimeEnvironment: TestRuntimeFixtures.productionApp, + defaults: appDefaults, + helperDefaults: helperDefaults, + providerAccountDeletionOutbox: + ProviderAccountDeletionOutbox( + defaults: appDefaults, + storageKey: + "ProviderConfigMetadataStoreTests.credential-rollback.outbox" + ), + performLaunchSetup: false + ) + state.providerConfigs = [ + ProviderConfig( + kind: .claude, + accountID: accountID, + isEnabled: true, + accountLabel: "Original" + ), + ] + let metadataStore = ProviderConfigMetadataStore( + defaults: appDefaults, + helperDefaults: helperDefaults + ) + XCTAssertTrue( + state.persistProviderAccountDraftMetadata( + accountID, + using: metadataStore + ) + ) + let checkpoint = try XCTUnwrap( + state.makeProviderAccountPersistenceCheckpoint( + accountID, + using: metadataStore + ) + ) + state.providerConfigs[0].accountLabel = "Edited" + state.providerConfigs[0] + .sharedCredentialFallbackDisabled = true + + XCTAssertEqual( + ProviderAccountSaveTransaction.commit( + persistSecrets: { true }, + rollbackSecrets: { true }, + persistMetadata: { + state.persistProviderAccountDraftMetadata( + accountID, + using: metadataStore + ) + }, + rollbackMetadata: { + checkpoint.restore() + }, + commitProviderCredential: { false }, + finalize: {} + ), + .failedRolledBack + ) + XCTAssertEqual( + ProviderSharedCredentialOwner.lookup(kind: .claude), + .owned(accountID) + ) + for (defaults, key) in [ + (appDefaults, ProviderAccountMigration.configsKey), + (helperDefaults, HelperIPC.providerConfigsKey), + ] { + let data = try XCTUnwrap(defaults.data(forKey: key)) + let configs = try JSONDecoder().decode( + [ProviderConfig].self, + from: data + ) + XCTAssertEqual(configs.first?.accountLabel, "Original") + XCTAssertNil( + configs.first?.sharedCredentialFallbackDisabled + ) + } + } + #endif + private func clear(_ defaults: UserDefaults) { for key in defaults.dictionaryRepresentation().keys { defaults.removeObject(forKey: key) } } } + +private final class DroppingUserDefaults: UserDefaults { + var droppedSetKeys: Set = [] + + override func set(_ value: Any?, forKey defaultName: String) { + guard !droppedSetKeys.contains(defaultName) else { + return + } + super.set(value, forKey: defaultName) + } +} diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift index 20d5152b..6d844649 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift @@ -240,6 +240,37 @@ final class ProviderSharedCredentialOwnerTests: XCTestCase { ) } + @MainActor + func testAppSaveTransactionLockAllowsReentrantOwnerMutation() + throws + { + let accountID = try XCTUnwrap( + UUID( + uuidString: + "05050505-0505-4505-8505-050505050505" + ) + ) + let state = AppState( + runtimeEnvironment: TestRuntimeFixtures.productionApp, + defaults: testDefaults, + helperDefaults: testDefaults, + performLaunchSetup: false + ) + + XCTAssertTrue( + state.withProviderAccountPersistenceLock(or: false) { + ProviderSharedCredentialOwner.claim( + kind: .gemini, + accountID: accountID + ) + } + ) + XCTAssertEqual( + ProviderSharedCredentialOwner.lookup(kind: .gemini), + .owned(accountID) + ) + } + func testMutationLockRejectsHardLinkBeforeChangingPermissions() throws { @@ -339,6 +370,57 @@ final class ProviderSharedCredentialOwnerTests: XCTestCase { ) } + func testReconcileRestoresEveryProviderWhenLaterOwnerWriteFails() + throws + { + let claudeID = try XCTUnwrap( + UUID( + uuidString: + "DADADADA-DADA-4ADA-8ADA-DADADADADADA" + ) + ) + let geminiID = try XCTUnwrap( + UUID( + uuidString: + "EAEAEAEA-EAEA-4AEA-8AEA-EAEAEAEAEAEA" + ) + ) + var synchronizeCalls = 0 + ProviderSharedCredentialOwner.synchronizeDefaults = { _ in + synchronizeCalls += 1 + return synchronizeCalls != 3 + } + + XCTAssertFalse( + ProviderSharedCredentialOwner.reconcile( + configs: [ + ProviderConfig( + kind: .claude, + accountID: claudeID, + isEnabled: true + ), + ProviderConfig( + kind: .gemini, + accountID: geminiID, + isEnabled: true + ), + ] + ) + ) + XCTAssertNil( + testDefaults.object( + forKey: + "cli_pulse_provider_shared_credential_owner_Claude" + ) + ) + XCTAssertNil( + testDefaults.object( + forKey: + "cli_pulse_provider_shared_credential_owner_Gemini" + ) + ) + } + func testReconcileSkipsAccountsThatDisableSharedFallback() throws { let isolatedID = try XCTUnwrap( UUID(uuidString: "12121212-1212-4121-8121-121212121212") From 0882f2b260a7f0daadb29399a0c937582d8dcfc7 Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Sun, 9 Aug 2026 22:46:55 +0800 Subject: [PATCH 04/10] fix: retire legacy provider secrets on account deletion --- .../Sources/CLIPulseCore/AppState.swift | 4 +- .../Sources/CLIPulseCore/ProviderConfig.swift | 58 +++ ...roviderAccountKeychainMigrationTests.swift | 351 ++++++++++++++++++ .../QARuntimeSideEffectPolicyTests.swift | 23 +- 4 files changed, 433 insertions(+), 3 deletions(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift index 96547755..5623ecb6 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift @@ -1641,7 +1641,9 @@ public final class AppState: ObservableObject { private func deleteLocalProviderAccountSecrets( _ config: ProviderConfig ) -> Bool { - guard config.deleteSecrets(using: providerSecretStore) else { + guard config.deleteSecretsForAccountRemoval( + using: providerSecretStore + ) else { return false } #if os(macOS) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift index 704b3ba5..02001d85 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift @@ -253,6 +253,47 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { return true } + /// Retire every credential entry owned by an explicitly deleted account. + /// + /// `deleteSecrets(using:)` intentionally retains migration markers so a + /// disconnected account cannot resurrect a legacy provider-scoped value. + /// Once the account itself is being removed, those markers would become + /// orphaned. The one account designated as the legacy migration owner must + /// also retire the provider-scoped entries before its retry anchor (the + /// account metadata) can be removed. + @discardableResult + func deleteSecretsForAccountRemoval( + using store: any ProviderSecretStoring + ) -> Bool { + guard deleteSecrets(using: store) else { + return false + } + + let group = Self.secretsAccessGroup + if legacySecretMigrationEligible == true { + for suffix in ["apiKey", "cookie"] { + guard deleteAndConfirmMissing( + key: Self.legacyKeychainKey(kind, suffix), + accessGroup: group, + using: store + ) else { + return false + } + } + } + + for suffix in ["apiKey", "cookie"] { + guard deleteAndConfirmMissing( + key: Self.migrationMarkerKey(accountID, suffix), + accessGroup: group, + using: store + ) else { + return false + } + } + return true + } + func makeSecretPersistenceCheckpoint( using store: any ProviderSecretStoring ) -> SecretPersistenceCheckpoint? { @@ -397,6 +438,23 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { ) == .value("1") } + private func deleteAndConfirmMissing( + key: String, + accessGroup: String?, + using store: any ProviderSecretStoring + ) -> Bool { + guard store.delete( + key: key, + accessGroup: accessGroup + ) else { + return false + } + return store.read( + key: key, + accessGroup: accessGroup + ) == .missing + } + private func loadSecret( suffix: String, using store: any ProviderSecretStoring diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift index f1492343..afd716cd 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift @@ -380,6 +380,324 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { ) } + @MainActor + func testRemovingMigrationOwnerDeletesLegacySlotsAndOnlyItsAccountSecrets() + throws + { + let store = InMemoryProviderSecretStore() + let ownerID = try XCTUnwrap( + UUID( + uuidString: + "AEAEAEAE-AEAE-4EAE-8EAE-AEAEAEAEAEAE" + ) + ) + let siblingID = try XCTUnwrap( + UUID( + uuidString: + "AFAFAFAF-AFAF-4FAF-8FAF-AFAFAFAFAFAF" + ) + ) + let group = ProviderConfig.secretsAccessGroup + let owner = ProviderConfig( + kind: .claude, + accountID: ownerID, + apiKey: "owner-api-key", + manualCookieHeader: "owner-cookie", + legacySecretMigrationEligible: true + ) + let sibling = ProviderConfig( + kind: .claude, + accountID: siblingID, + apiKey: "sibling-api-key", + manualCookieHeader: "sibling-cookie" + ) + XCTAssertTrue(owner.saveSecrets(using: store)) + XCTAssertTrue(sibling.saveSecrets(using: store)) + XCTAssertTrue( + store.save( + key: legacyKey(.claude, "apiKey"), + value: "legacy-api-key", + accessGroup: group + ) + ) + XCTAssertTrue( + store.save( + key: legacyKey(.claude, "cookie"), + value: "legacy-cookie", + accessGroup: group + ) + ) + let state = try makeIsolatedState(store: store) + state.providerConfigs = [owner, sibling] + + XCTAssertTrue(state.removeProviderAccount(ownerID)) + + XCTAssertEqual(state.providerConfigs.map(\.accountID), [siblingID]) + XCTAssertNil( + store.load( + key: legacyKey(.claude, "apiKey"), + accessGroup: group + ) + ) + XCTAssertNil( + store.load( + key: legacyKey(.claude, "cookie"), + accessGroup: group + ) + ) + XCTAssertNil( + store.load( + key: markerKey(ownerID, "apiKey"), + accessGroup: group + ) + ) + XCTAssertNil( + store.load( + key: markerKey(ownerID, "cookie"), + accessGroup: group + ) + ) + XCTAssertEqual( + store.load( + key: accountKey(siblingID, "apiKey"), + accessGroup: group + ), + "sibling-api-key" + ) + XCTAssertEqual( + store.load( + key: accountKey(siblingID, "cookie"), + accessGroup: group + ), + "sibling-cookie" + ) + } + + @MainActor + func testLegacyDeleteFailureKeepsAccountRetryableUntilCleanupCompletes() + throws + { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "B0B0B0B0-B0B0-40B0-80B0-B0B0B0B0B0B0" + ) + ) + let group = ProviderConfig.secretsAccessGroup + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "account-api-key", + manualCookieHeader: "account-cookie", + legacySecretMigrationEligible: true + ) + XCTAssertTrue(config.saveSecrets(using: store)) + XCTAssertTrue( + store.save( + key: legacyKey(.claude, "apiKey"), + value: "legacy-api-key", + accessGroup: group + ) + ) + let legacyCookie = legacyKey(.claude, "cookie") + XCTAssertTrue( + store.save( + key: legacyCookie, + value: "legacy-cookie", + accessGroup: group + ) + ) + store.failingDeleteKeys.insert(legacyCookie) + let state = try makeIsolatedState(store: store) + state.providerConfigs = [config] + + XCTAssertFalse(state.removeProviderAccount(accountID)) + XCTAssertEqual(state.providerConfigs.map(\.accountID), [accountID]) + XCTAssertNil( + store.load( + key: legacyKey(.claude, "apiKey"), + accessGroup: group + ), + "completed deletion steps may remain monotonic while metadata stays retryable" + ) + XCTAssertEqual( + store.load(key: legacyCookie, accessGroup: group), + "legacy-cookie" + ) + XCTAssertEqual( + store.load( + key: markerKey(accountID, "apiKey"), + accessGroup: group + ), + "1", + "the marker must prevent retained legacy data from reappearing before retry" + ) + + store.failingDeleteKeys.remove(legacyCookie) + + XCTAssertTrue(state.removeProviderAccount(accountID)) + XCTAssertTrue(state.providerConfigs.isEmpty) + XCTAssertNil(store.load(key: legacyCookie, accessGroup: group)) + XCTAssertNil( + store.load( + key: markerKey(accountID, "apiKey"), + accessGroup: group + ) + ) + XCTAssertNil( + store.load( + key: markerKey(accountID, "cookie"), + accessGroup: group + ) + ) + } + + @MainActor + func testLegacyReadFailureCannotVerifyAccountRemoval() throws { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "B1B1B1B1-B1B1-41B1-81B1-B1B1B1B1B1B1" + ) + ) + let group = ProviderConfig.secretsAccessGroup + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "account-api-key", + legacySecretMigrationEligible: true + ) + XCTAssertTrue(config.saveSecrets(using: store)) + let legacyAPIKey = legacyKey(.claude, "apiKey") + XCTAssertTrue( + store.save( + key: legacyAPIKey, + value: "legacy-api-key", + accessGroup: group + ) + ) + store.failingLoadKeys.insert(legacyAPIKey) + let state = try makeIsolatedState(store: store) + state.providerConfigs = [config] + + XCTAssertFalse(state.removeProviderAccount(accountID)) + XCTAssertEqual(state.providerConfigs.map(\.accountID), [accountID]) + + store.failingLoadKeys.remove(legacyAPIKey) + + XCTAssertTrue(state.removeProviderAccount(accountID)) + XCTAssertTrue(state.providerConfigs.isEmpty) + } + + @MainActor + func testMarkerDeleteFailureKeepsAccountRetryableUntilCleanupCompletes() + throws + { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "B2B2B2B2-B2B2-42B2-82B2-B2B2B2B2B2B2" + ) + ) + let group = ProviderConfig.secretsAccessGroup + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "account-api-key", + manualCookieHeader: "account-cookie" + ) + XCTAssertTrue(config.saveSecrets(using: store)) + let cookieMarker = markerKey(accountID, "cookie") + store.failingDeleteKeys.insert(cookieMarker) + let state = try makeIsolatedState(store: store) + state.providerConfigs = [config] + + XCTAssertFalse(state.removeProviderAccount(accountID)) + XCTAssertEqual(state.providerConfigs.map(\.accountID), [accountID]) + XCTAssertNil( + store.load( + key: accountKey(accountID, "apiKey"), + accessGroup: group + ) + ) + XCTAssertNil( + store.load( + key: accountKey(accountID, "cookie"), + accessGroup: group + ) + ) + XCTAssertNil( + store.load( + key: markerKey(accountID, "apiKey"), + accessGroup: group + ) + ) + XCTAssertEqual( + store.load(key: cookieMarker, accessGroup: group), + "1" + ) + + store.failingDeleteKeys.remove(cookieMarker) + + XCTAssertTrue(state.removeProviderAccount(accountID)) + XCTAssertTrue(state.providerConfigs.isEmpty) + XCTAssertNil(store.load(key: cookieMarker, accessGroup: group)) + } + + @MainActor + func testRemovingNonMigrationSiblingPreservesLegacySlotsAndRemovesMarkers() + throws + { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "B3B3B3B3-B3B3-43B3-83B3-B3B3B3B3B3B3" + ) + ) + let group = ProviderConfig.secretsAccessGroup + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "sibling-api-key" + ) + XCTAssertTrue(config.saveSecrets(using: store)) + XCTAssertTrue( + store.save( + key: legacyKey(.claude, "apiKey"), + value: "migration-owner-legacy-key", + accessGroup: group + ) + ) + let state = try makeIsolatedState(store: store) + state.providerConfigs = [config] + + XCTAssertTrue(state.removeProviderAccount(accountID)) + + XCTAssertEqual( + store.load( + key: legacyKey(.claude, "apiKey"), + accessGroup: group + ), + "migration-owner-legacy-key" + ) + XCTAssertNil( + store.load( + key: markerKey(accountID, "apiKey"), + accessGroup: group + ) + ) + XCTAssertNil( + store.load( + key: markerKey(accountID, "cookie"), + accessGroup: group + ) + ) + } + private func legacyKey(_ kind: ProviderKind, _ suffix: String) -> String { "cli_pulse_provider_\(kind.rawValue)_\(suffix)" } @@ -387,6 +705,39 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { private func accountKey(_ accountID: UUID, _ suffix: String) -> String { "cli_pulse_provider_account_\(accountID.uuidString)_\(suffix)" } + + private func markerKey(_ accountID: UUID, _ suffix: String) -> String { + "\(accountKey(accountID, suffix))_legacy_migrated" + } + + @MainActor + private func makeIsolatedState( + store: InMemoryProviderSecretStore + ) throws -> AppState { + let suiteName = + "ProviderAccountKeychainMigrationTests.\(UUID().uuidString)" + let defaults = try XCTUnwrap( + UserDefaults(suiteName: suiteName) + ) + defaults.removePersistentDomain(forName: suiteName) + addTeardownBlock { + defaults.removePersistentDomain(forName: suiteName) + } + let runtime = + CLIPulseRuntimeEnvironment.resolveForTesting( + infoDictionary: [ + "CFBundleIdentifier": + "tests.clipulse.provider-account-removal", + ], + environment: [:] + ) + return AppState( + runtimeEnvironment: runtime, + defaults: defaults, + providerSecretStore: store, + performLaunchSetup: false + ) + } } private final class InMemoryProviderSecretStore: ProviderSecretStoring { diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/QARuntimeSideEffectPolicyTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/QARuntimeSideEffectPolicyTests.swift index 5b61a1cc..7a2518ab 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/QARuntimeSideEffectPolicyTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/QARuntimeSideEffectPolicyTests.swift @@ -840,7 +840,10 @@ final class QARuntimeSideEffectPolicyTests: XCTestCase { ownerFixture.defaults.string(forKey: ownerKey), draftID.uuidString ) - XCTAssertEqual(secretStore.deletedKeys.count, 2) + XCTAssertEqual( + secretStore.deletedKeys, + expectedAccountDeletionKeys(draftID) + ) } @MainActor @@ -886,7 +889,23 @@ final class QARuntimeSideEffectPolicyTests: XCTestCase { ownerFixture.defaults.string(forKey: ownerKey), accountID.uuidString ) - XCTAssertEqual(secretStore.deletedKeys.count, 2) + XCTAssertEqual( + secretStore.deletedKeys, + expectedAccountDeletionKeys(accountID) + ) + } + + private func expectedAccountDeletionKeys( + _ accountID: UUID + ) -> [String] { + let prefix = + "cli_pulse_provider_account_\(accountID.uuidString)" + return [ + "\(prefix)_apiKey", + "\(prefix)_cookie", + "\(prefix)_apiKey_legacy_migrated", + "\(prefix)_cookie_legacy_migrated", + ] } private func makeRuntime( From f5b813435fd92a9ac7a82037f33c143b513aad6e Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Mon, 10 Aug 2026 00:53:52 +0800 Subject: [PATCH 05/10] fix: keep provider deletion retryable --- .../Sources/CLIPulseCore/AppState.swift | 12 ++- .../ProviderSharedCredentialOwner.swift | 4 +- ...roviderAccountKeychainMigrationTests.swift | 79 ++++++++++++++++++- .../ProviderSharedCredentialOwnerTests.swift | 9 +++ 4 files changed, 97 insertions(+), 7 deletions(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift index 5623ecb6..a0fb0314 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift @@ -1658,10 +1658,14 @@ public final class AppState: ObservableObject { } #endif if runtimeEnvironment.capabilities.allowsHelperRegistration { - ProviderSharedCredentialOwner.release( - kind: config.kind, - accountID: config.accountID - ) + guard + ProviderSharedCredentialOwner.release( + kind: config.kind, + accountID: config.accountID + ) + else { + return false + } } return true } diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift index 2331871c..8157b2ae 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderSharedCredentialOwner.swift @@ -327,8 +327,10 @@ enum ProviderSharedCredentialOwner { kind: ProviderKind, accountID: UUID ) -> Bool { + // Idempotent cleanup: providers without a shared compatibility source + // have no owner record to release. guard supportedKinds.contains(kind) else { - return false + return true } return withMutationLock(or: false) { lock.withLock { diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift index afd716cd..76837358 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift @@ -698,6 +698,79 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { ) } + #if os(macOS) + @MainActor + func testSharedOwnerReleaseFailureKeepsAccountRetryable() throws { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "B4B4B4B4-B4B4-44B4-84B4-B4B4B4B4B4B4" + ) + ) + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "account-api-key" + ) + XCTAssertTrue(config.saveSecrets(using: store)) + let state = try makeIsolatedState( + store: store, + bundleIdentifier: "yyh.CLI-Pulse" + ) + state.providerConfigs = [config] + + let ownerSuiteName = + "ProviderAccountKeychainMigrationTests.Owner.\(UUID().uuidString)" + let ownerDefaults = try XCTUnwrap( + UserDefaults(suiteName: ownerSuiteName) + ) + ownerDefaults.removePersistentDomain(forName: ownerSuiteName) + let ownerKey = + "cli_pulse_provider_shared_credential_owner_\(ProviderKind.claude.rawValue)" + ownerDefaults.set(accountID.uuidString, forKey: ownerKey) + + let originalOwnerDefaults = ProviderSharedCredentialOwner.defaults + let originalSynchronizeDefaults = + ProviderSharedCredentialOwner.synchronizeDefaults + let originalMutationLock = + ProviderSharedCredentialOwner.mutationLock + let mutationLockPath = FileManager.default.temporaryDirectory + .appendingPathComponent("\(ownerSuiteName).lock") + .path + defer { + ProviderSharedCredentialOwner.defaults = originalOwnerDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = + originalSynchronizeDefaults + ProviderSharedCredentialOwner.mutationLock = + originalMutationLock + ownerDefaults.removePersistentDomain(forName: ownerSuiteName) + try? FileManager.default.removeItem( + atPath: mutationLockPath + ) + } + ProviderSharedCredentialOwner.defaults = ownerDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = { _ in false } + ProviderSharedCredentialOwner.mutationLock = + GeminiCredentialMutationLock( + lockFilePath: mutationLockPath + ) + + XCTAssertFalse(state.removeProviderAccount(accountID)) + XCTAssertEqual(state.providerConfigs.map(\.accountID), [accountID]) + XCTAssertEqual( + ownerDefaults.string(forKey: ownerKey), + accountID.uuidString + ) + + ProviderSharedCredentialOwner.synchronizeDefaults = { _ in true } + + XCTAssertTrue(state.removeProviderAccount(accountID)) + XCTAssertTrue(state.providerConfigs.isEmpty) + XCTAssertNil(ownerDefaults.string(forKey: ownerKey)) + } + #endif + private func legacyKey(_ kind: ProviderKind, _ suffix: String) -> String { "cli_pulse_provider_\(kind.rawValue)_\(suffix)" } @@ -712,7 +785,9 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { @MainActor private func makeIsolatedState( - store: InMemoryProviderSecretStore + store: InMemoryProviderSecretStore, + bundleIdentifier: String = + "tests.clipulse.provider-account-removal" ) throws -> AppState { let suiteName = "ProviderAccountKeychainMigrationTests.\(UUID().uuidString)" @@ -727,7 +802,7 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { CLIPulseRuntimeEnvironment.resolveForTesting( infoDictionary: [ "CFBundleIdentifier": - "tests.clipulse.provider-account-removal", + bundleIdentifier, ], environment: [:] ) diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift index 6d844649..bd06ce5d 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderSharedCredentialOwnerTests.swift @@ -211,6 +211,15 @@ final class ProviderSharedCredentialOwnerTests: XCTestCase { ) } + func testReleaseForProviderWithoutSharedSourceIsSuccessfulNoOp() { + XCTAssertTrue( + ProviderSharedCredentialOwner.release( + kind: .codex, + accountID: UUID() + ) + ) + } + func testOwnerCallIsReentrantInsideCredentialMutationLock() throws { From 7e67489fa2c4a1e0d3f92c3513090f59b8969f7a Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Mon, 10 Aug 2026 01:44:49 +0800 Subject: [PATCH 06/10] fix: preserve newer provider deletion intents --- .../Sources/CLIPulseCore/AppState.swift | 5 +- .../ProviderAccountDeletionOutbox.swift | 36 +++++++---- .../AppStateProviderAccountSyncTests.swift | 63 +++++++++++++++++++ .../ProviderAccountDeletionOutboxTests.swift | 17 +++-- 4 files changed, 102 insertions(+), 19 deletions(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift index a0fb0314..4c5c8154 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift @@ -1778,10 +1778,7 @@ public final class AppState: ObservableObject { // refresh instead of hammering the endpoint. return } - providerAccountDeletionOutbox.markCompleted( - userID: expectedUserID, - accountID: intent.accountID - ) + providerAccountDeletionOutbox.markCompleted(intent) } } diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountDeletionOutbox.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountDeletionOutbox.swift index f05f34c8..5b47382a 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountDeletionOutbox.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderAccountDeletionOutbox.swift @@ -15,15 +15,21 @@ final class ProviderAccountDeletionOutbox { /// Every newly enqueued delete records the non-secret provider and /// provider-less legacy intents are never sent to the server. let provider: ProviderKind? + /// Identifies one enqueue operation so a stale in-flight response + /// cannot complete a newer retry for the same owner and account. + /// Optional only so existing v1 queue payloads remain decodable. + let generation: UUID? init( userID: String, accountID: UUID, - provider: ProviderKind? + provider: ProviderKind?, + generation: UUID? = UUID() ) { self.userID = userID self.accountID = accountID self.provider = provider + self.generation = generation } } @@ -114,11 +120,10 @@ final class ProviderAccountDeletionOutbox { } @discardableResult - func markCompleted( - userID: String, - accountID: UUID - ) -> Bool { - guard let owner = normalizedUserID(userID) else { + func markCompleted(_ completedIntent: Intent) -> Bool { + guard + let owner = normalizedUserID(completedIntent.userID) + else { return false } guard case var .valid(records) = @@ -127,9 +132,14 @@ final class ProviderAccountDeletionOutbox { preserveCorruptStorage() return false } - records = Set(records.filter { - $0.userID != owner || $0.accountID != accountID - }) + records.remove( + Intent( + userID: owner, + accountID: completedIntent.accountID, + provider: completedIntent.provider, + generation: completedIntent.generation + ) + ) return saveRecords(records) } @@ -242,8 +252,12 @@ final class ProviderAccountDeletionOutbox { return lhs.accountID.uuidString < rhs.accountID.uuidString } - return (lhs.provider?.rawValue ?? "") - < (rhs.provider?.rawValue ?? "") + if lhs.provider != rhs.provider { + return (lhs.provider?.rawValue ?? "") + < (rhs.provider?.rawValue ?? "") + } + return (lhs.generation?.uuidString ?? "") + < (rhs.generation?.uuidString ?? "") } } diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift index ecca9d90..1f334d49 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift @@ -284,6 +284,69 @@ final class AppStateProviderAccountSyncTests: XCTestCase { ) } + func testStaleSuccessfulDeleteDoesNotClearNewerIntentForSameAccount() + async throws + { + let accountID = try XCTUnwrap( + UUID( + uuidString: + "34343434-3434-4434-8434-343434343434" + ) + ) + let requestStarted = expectation( + description: "original delete request started" + ) + let responseGate = DispatchSemaphore(value: 0) + AppStateProviderAccountStubProtocol.configure( + responses: [ + .json( + #"{"accounts_deleted":1,"tombstones_persisted":1}"#, + responseGate: responseGate + ), + ], + onRequest: { _ in requestStarted.fulfill() } + ) + let outbox = makeOutbox() + XCTAssertTrue( + outbox.enqueue( + userID: "user-a", + accountID: accountID, + provider: .claude + ) + ) + let state = makeState( + api: await makeAuthenticatedAPI(), + outbox: outbox, + userID: "user-a" + ) + + let flush = Task { @MainActor in + await state.flushPendingProviderAccountDeletions( + for: "user-a" + ) + } + await fulfillment(of: [requestStarted], timeout: 3) + XCTAssertTrue( + outbox.enqueue( + userID: "user-a", + accountID: accountID, + provider: .claude + ), + "a retry while the original request is in flight must create a new durable intent" + ) + responseGate.signal() + await flush.value + + let pending = outbox.pendingIntents(for: "user-a") + XCTAssertEqual( + pending.count, + 1, + "an old response must not clear a newer retry authority" + ) + XCTAssertEqual(pending.first?.accountID, accountID) + XCTAssertEqual(pending.first?.provider, .claude) + } + func testStatusSyncFiltersCurrentOwnerAndDoesNotRetryStaleLeaseWithNewSession() async throws { diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountDeletionOutboxTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountDeletionOutboxTests.swift index c0001e99..a323f530 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountDeletionOutboxTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountDeletionOutboxTests.swift @@ -90,12 +90,12 @@ final class ProviderAccountDeletionOutboxTests: XCTestCase { accountID: account, provider: .codex ) - - outbox.markCompleted( - userID: "user-a", - accountID: account + let userAIntent = try XCTUnwrap( + outbox.pendingIntents(for: "user-a").first ) + outbox.markCompleted(userAIntent) + XCTAssertTrue( outbox.pendingAccountIDs(for: "user-a").isEmpty ) @@ -134,6 +134,10 @@ final class ProviderAccountDeletionOutboxTests: XCTestCase { restored.pendingIntents().first?.provider, .claude ) + XCTAssertNotNil( + restored.pendingIntents().first?.generation, + "new intent identity must survive process recreation" + ) } func testProviderlessV1IntentDecodesAndCanBeSafelyEnriched() @@ -160,6 +164,10 @@ final class ProviderAccountDeletionOutboxTests: XCTestCase { ) XCTAssertNil(outbox.pendingIntents().first?.provider) + XCTAssertNil( + outbox.pendingIntents().first?.generation, + "an older payload without generation must remain decodable" + ) XCTAssertTrue( outbox.enqueue( userID: "user-a", @@ -172,6 +180,7 @@ final class ProviderAccountDeletionOutboxTests: XCTestCase { outbox.pendingIntents().first?.provider, .claude ) + XCTAssertNotNil(outbox.pendingIntents().first?.generation) } func testPersistedIntentRecoversLocalConfigAfterCrash() throws { From 237d13ecbdbf76a8b94b1fe150f4fd5f68e79a73 Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:48:33 +0800 Subject: [PATCH 07/10] fix: gate provider deletion on durable metadata --- .../Sources/CLIPulseCore/AppState.swift | 65 +++- .../AppStateProviderAccountSyncTests.swift | 167 ++++++++++ .../ProviderConfigMetadataStoreTests.swift | 311 +++++++++++++++++- 3 files changed, 537 insertions(+), 6 deletions(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift index 4c5c8154..62212db8 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/AppState.swift @@ -1620,10 +1620,21 @@ public final class AppState: ObservableObject { guard deleteLocalProviderAccountSecrets(config) else { return false } + let remainingConfigs = providerConfigs.filter { + $0.accountID != accountID + } + guard saveProviderConfigMetadata( + remainingConfigs, + requireSharedCredentialOwnerReconciliation: true + ) else { + // Keep the in-memory account as the visible retry anchor. The + // durable outbox (when present) remains pending, and cloud + // deletion must not start until local metadata commits. + return false + } guard providerState.removeProviderAccount(accountID) != nil else { return false } - saveProviderConfigMetadata() buildProviderDetails() if let deletionOwnerID { Task { [weak self] in @@ -1751,9 +1762,15 @@ public final class AppState: ObservableObject { else { return } + let locallyPresentAccountIDs = Set( + providerConfigs.map(\.accountID) + ) let pending = providerAccountDeletionOutbox .pendingIntents(for: expectedUserID) - .filter { $0.provider != nil } + .filter { + $0.provider != nil + && !locallyPresentAccountIDs.contains($0.accountID) + } .prefix(10) for intent in pending { guard @@ -1767,6 +1784,14 @@ public final class AppState: ObservableObject { // local metadata recovery can safely enrich them. continue } + guard !providerConfigs.contains(where: { + $0.accountID == intent.accountID + }) else { + // The preceding delete awaits, so MainActor can restore a + // later account while this batch is in progress. Recheck each + // intent at the last local gate before contacting the server. + continue + } let deleted = await api.deleteProviderAccount( intent.accountID, provider: provider, @@ -2090,17 +2115,35 @@ public final class AppState: ObservableObject { /// for enable/order/label changes that must never mutate Keychain state. @discardableResult public func saveProviderConfigMetadata() -> Bool { + saveProviderConfigMetadata( + providerConfigs, + requireSharedCredentialOwnerReconciliation: false + ) + } + + private func saveProviderConfigMetadata( + _ configs: [ProviderConfig], + requireSharedCredentialOwnerReconciliation: Bool + ) -> Bool { let allowsHelperMirror = runtimeEnvironment.capabilities.allowsHelperRegistration if allowsHelperMirror { - ProviderSharedCredentialOwner.reconcile(configs: providerConfigs) + let reconciled = ProviderSharedCredentialOwner.reconcile( + configs: configs + ) + guard + reconciled + || !requireSharedCredentialOwnerReconciliation + else { + return false + } } return ProviderConfigMetadataStore( defaults: providerConfigDefaults, helperDefaults: allowsHelperMirror ? providerConfigHelperDefaults : nil - ).save(providerConfigs) + ).save(configs) } public func buildProviderDetails() { @@ -2280,7 +2323,10 @@ public final class AppState: ObservableObject { /// Finish a local deletion whose durable intent was committed before a /// previous process stopped. Owner matching is mandatory so a queued /// user-A intent can never remove a user-B account after an account switch. - private func recoverPendingLocalProviderAccountDeletions() { + /// Internal so deterministic composition tests can exercise the relaunch + /// recovery transaction without running AppState's production launch + /// migrations or touching the real Keychain. + func recoverPendingLocalProviderAccountDeletions() { let accountIDs = ProviderAccountDeletionRecovery.accountIDsToRemove( from: providerConfigs, @@ -2307,6 +2353,15 @@ public final class AppState: ObservableObject { else { continue } + let remainingConfigs = providerConfigs.filter { + $0.accountID != accountID + } + guard saveProviderConfigMetadata( + remainingConfigs, + requireSharedCredentialOwnerReconciliation: true + ) else { + continue + } _ = providerState.removeProviderAccount(accountID) } } diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift index 1f334d49..bfba8784 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/AppStateProviderAccountSyncTests.swift @@ -96,6 +96,173 @@ final class AppStateProviderAccountSyncTests: XCTestCase { ) } + func testFailedLocalMetadataRemovalIsNotFlushedToCloud() + async throws + { + let removalSuiteName = + "AppStateProviderAccountSyncTests.metadata-failure.\(UUID())" + let removalDefaults = try XCTUnwrap( + DroppingUserDefaults(suiteName: removalSuiteName) + ) + defer { + removalDefaults.removePersistentDomain( + forName: removalSuiteName + ) + } + let accountID = try XCTUnwrap( + UUID( + uuidString: + "12121212-1212-4212-8212-121212121212" + ) + ) + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + syncOwnerUserID: "user-a" + ) + XCTAssertTrue( + ProviderConfigMetadataStore( + defaults: removalDefaults, + helperDefaults: nil + ).save([config]) + ) + let outbox = ProviderAccountDeletionOutbox( + defaults: removalDefaults, + storageKey: + "AppStateProviderAccountSyncTests.metadata-failure.outbox" + ) + AppStateProviderAccountStubProtocol.configure( + responses: [ + .json( + #"{"accounts_deleted":1,"tombstones_persisted":1}"# + ), + ] + ) + let runtime = + CLIPulseRuntimeEnvironment.resolveForTesting( + infoDictionary: [ + "CFBundleIdentifier": + "tests.clipulse.failed-local-removal", + ], + environment: [:] + ) + let state = AppState( + runtimeEnvironment: runtime, + defaults: removalDefaults, + providerSecretStore: MemoryProviderSecretStore(), + api: await makeAuthenticatedAPI(), + providerAccountDeletionOutbox: outbox, + performLaunchSetup: false + ) + state.isAuthenticated = true + state.userId = "user-a" + state.providerConfigs = [config] + removalDefaults.droppedSetKeys.insert( + ProviderAccountMigration.configsKey + ) + + XCTAssertFalse(state.removeProviderAccount(accountID)) + await state.flushPendingProviderAccountDeletions( + for: "user-a" + ) + + XCTAssertTrue( + AppStateProviderAccountStubProtocol.recordedRequests() + .isEmpty, + "a prepared intent must not reach the server before local metadata commits" + ) + XCTAssertEqual( + outbox.pendingAccountIDs(for: "user-a"), + [accountID] + ) + } + + func testRestoredAccountIsRecheckedBeforeEachBatchedCloudDelete() + async throws + { + let firstAccountID = try XCTUnwrap( + UUID( + uuidString: + "10101010-1010-4010-8010-101010101010" + ) + ) + let restoredAccountID = try XCTUnwrap( + UUID( + uuidString: + "20202020-2020-4020-8020-202020202020" + ) + ) + let firstRequestStarted = expectation( + description: "first batched delete started" + ) + let firstResponseGate = DispatchSemaphore(value: 0) + AppStateProviderAccountStubProtocol.configure( + responses: [ + .json( + #"{"accounts_deleted":1,"tombstones_persisted":1}"#, + responseGate: firstResponseGate + ), + .json( + #"{"accounts_deleted":1,"tombstones_persisted":1}"# + ), + ], + onRequest: { request in + if AppStateProviderAccountStubProtocol + .recordedRequests().count == 1 + { + firstRequestStarted.fulfill() + } + } + ) + let outbox = makeOutbox() + XCTAssertTrue( + outbox.enqueue( + userID: "user-a", + accountID: firstAccountID, + provider: .claude + ) + ) + XCTAssertTrue( + outbox.enqueue( + userID: "user-a", + accountID: restoredAccountID, + provider: .codex + ) + ) + let state = makeState( + api: await makeAuthenticatedAPI(), + outbox: outbox, + userID: "user-a" + ) + + let flush = Task { @MainActor in + await state.flushPendingProviderAccountDeletions( + for: "user-a" + ) + } + await fulfillment(of: [firstRequestStarted], timeout: 3) + state.providerConfigs = [ + ProviderConfig( + kind: .codex, + accountID: restoredAccountID, + syncOwnerUserID: "user-a" + ), + ] + firstResponseGate.signal() + await flush.value + + XCTAssertEqual( + AppStateProviderAccountStubProtocol.recordedRequests() + .count, + 1, + "each awaited delete must recheck whether its account became local again" + ) + XCTAssertEqual( + outbox.pendingAccountIDs(for: "user-a"), + [restoredAccountID] + ) + } + func testProviderlessLegacyIntentsDoNotStarveTypedDeletion() async throws { diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift index f8f33bd6..0715c0e3 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderConfigMetadataStoreTests.swift @@ -139,6 +139,184 @@ final class ProviderConfigMetadataStoreTests: XCTestCase { } } + @MainActor + func testAccountRemovalMetadataFailureKeepsAccountRetryable() + throws + { + let suiteName = + "ProviderConfigMetadataStoreTests.remove-failure.\(UUID())" + let defaults = try XCTUnwrap( + DroppingUserDefaults(suiteName: suiteName) + ) + defer { + defaults.removePersistentDomain(forName: suiteName) + } + let accountID = try XCTUnwrap( + UUID( + uuidString: + "ABABABAB-ABAB-4BAB-8BAB-ABABABABABAB" + ) + ) + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + syncOwnerUserID: "user-a" + ) + let metadataStore = ProviderConfigMetadataStore( + defaults: defaults, + helperDefaults: nil + ) + XCTAssertTrue(metadataStore.save([config])) + + let outbox = ProviderAccountDeletionOutbox( + defaults: defaults, + storageKey: + "ProviderConfigMetadataStoreTests.remove-failure.outbox" + ) + let runtime = + CLIPulseRuntimeEnvironment.resolveForTesting( + infoDictionary: [ + "CFBundleIdentifier": + "tests.clipulse.provider-metadata-removal", + ], + environment: [:] + ) + XCTAssertFalse( + runtime.capabilities.allowsHelperRegistration + ) + let secretStore = MemoryProviderSecretStore() + XCTAssertTrue( + config.deleteSecretsForAccountRemoval( + using: secretStore + ) + ) + let state = AppState( + runtimeEnvironment: runtime, + defaults: defaults, + providerSecretStore: secretStore, + providerAccountDeletionOutbox: outbox, + performLaunchSetup: false + ) + state.providerConfigs = [config] + defaults.droppedSetKeys.insert( + ProviderAccountMigration.configsKey + ) + XCTAssertFalse( + metadataStore.save([]), + "the fixture must reach the metadata write failure" + ) + + XCTAssertFalse(state.removeProviderAccount(accountID)) + XCTAssertEqual( + state.providerConfigs.map(\.accountID), + [accountID], + "a failed metadata write must leave a visible retry anchor" + ) + XCTAssertEqual( + outbox.pendingAccountIDs(for: "user-a"), + [accountID], + "the durable deletion intent must remain available for retry" + ) + let persistedData = try XCTUnwrap( + defaults.data( + forKey: ProviderAccountMigration.configsKey + ) + ) + XCTAssertEqual( + try JSONDecoder().decode( + [ProviderConfig].self, + from: persistedData + ).map(\.accountID), + [accountID] + ) + } + + @MainActor + func testPendingDeletionRecoveryMetadataFailureKeepsAccountRetryable() + throws + { + let suiteName = + "ProviderConfigMetadataStoreTests.recovery-failure.\(UUID())" + let defaults = try XCTUnwrap( + DroppingUserDefaults(suiteName: suiteName) + ) + defer { + defaults.removePersistentDomain(forName: suiteName) + } + let accountID = try XCTUnwrap( + UUID( + uuidString: + "CDCDCDCD-CDCD-4DCD-8DCD-CDCDCDCDCDCD" + ) + ) + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + syncOwnerUserID: "user-a" + ) + XCTAssertTrue( + ProviderConfigMetadataStore( + defaults: defaults, + helperDefaults: nil + ).save([config]) + ) + let outbox = ProviderAccountDeletionOutbox( + defaults: defaults, + storageKey: + "ProviderConfigMetadataStoreTests.recovery-failure.outbox" + ) + XCTAssertTrue( + outbox.enqueue( + userID: "user-a", + accountID: accountID, + provider: .claude + ) + ) + let runtime = + CLIPulseRuntimeEnvironment.resolveForTesting( + infoDictionary: [ + "CFBundleIdentifier": + "tests.clipulse.provider-deletion-recovery", + ], + environment: [:] + ) + let state = AppState( + runtimeEnvironment: runtime, + defaults: defaults, + providerSecretStore: MemoryProviderSecretStore(), + providerAccountDeletionOutbox: outbox, + performLaunchSetup: false + ) + state.providerConfigs = [config] + defaults.droppedSetKeys.insert( + ProviderAccountMigration.configsKey + ) + + state.recoverPendingLocalProviderAccountDeletions() + + XCTAssertEqual( + state.providerConfigs.map(\.accountID), + [accountID], + "relaunch recovery must keep the account visible when metadata cannot commit" + ) + XCTAssertEqual( + outbox.pendingAccountIDs(for: "user-a"), + [accountID] + ) + let persistedData = try XCTUnwrap( + defaults.data( + forKey: ProviderAccountMigration.configsKey + ) + ) + XCTAssertEqual( + try JSONDecoder().decode( + [ProviderConfig].self, + from: persistedData + ).map(\.accountID), + [accountID] + ) + } + @MainActor func testDraftRecoveryAnchorSurvivesRestartBeforeCredentialCommit() throws @@ -271,6 +449,108 @@ final class ProviderConfigMetadataStoreTests: XCTestCase { } #if os(macOS) + @MainActor + func testOwnerReconcileFailureBlocksDeletionButNotOrdinaryMetadataSave() + throws + { + let appSuiteName = + "ProviderConfigMetadataStoreTests.remove-owner.app.\(UUID())" + let helperSuiteName = + "ProviderConfigMetadataStoreTests.remove-owner.helper.\(UUID())" + let appDefaults = try XCTUnwrap( + UserDefaults(suiteName: appSuiteName) + ) + let helperDefaults = try XCTUnwrap( + UserDefaults(suiteName: helperSuiteName) + ) + let originalOwnerDefaults = + ProviderSharedCredentialOwner.defaults + let originalSynchronizeDefaults = + ProviderSharedCredentialOwner.synchronizeDefaults + let originalMutationLock = + ProviderSharedCredentialOwner.mutationLock + let lockPath = FileManager.default.temporaryDirectory + .appendingPathComponent( + "ProviderConfigMetadataStoreTests.remove-owner.\(UUID()).lock" + ) + .path + defer { + ProviderSharedCredentialOwner.defaults = + originalOwnerDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = + originalSynchronizeDefaults + ProviderSharedCredentialOwner.mutationLock = + originalMutationLock + appDefaults.removePersistentDomain( + forName: appSuiteName + ) + helperDefaults.removePersistentDomain( + forName: helperSuiteName + ) + try? FileManager.default.removeItem(atPath: lockPath) + } + ProviderSharedCredentialOwner.defaults = helperDefaults + ProviderSharedCredentialOwner.synchronizeDefaults = { _ in + false + } + ProviderSharedCredentialOwner.mutationLock = + GeminiCredentialMutationLock( + lockFilePath: lockPath + ) + + let accountID = try XCTUnwrap( + UUID( + uuidString: + "DEDEDEDE-DEDE-4EDE-8EDE-DEDEDEDEDEDE" + ) + ) + let config = ProviderConfig( + kind: .codex, + accountID: accountID + ) + let metadataStore = ProviderConfigMetadataStore( + defaults: appDefaults, + helperDefaults: helperDefaults + ) + XCTAssertTrue(metadataStore.save([config])) + let state = AppState( + runtimeEnvironment: TestRuntimeFixtures.productionApp, + defaults: appDefaults, + helperDefaults: helperDefaults, + providerSecretStore: MemoryProviderSecretStore(), + providerAccountDeletionOutbox: + ProviderAccountDeletionOutbox( + defaults: appDefaults, + storageKey: + "ProviderConfigMetadataStoreTests.remove-owner.outbox" + ), + performLaunchSetup: false + ) + state.providerConfigs = [config] + + XCTAssertFalse(state.removeProviderAccount(accountID)) + XCTAssertEqual( + state.providerConfigs.map(\.accountID), + [accountID] + ) + let persistedData = try XCTUnwrap( + appDefaults.data( + forKey: ProviderAccountMigration.configsKey + ) + ) + XCTAssertEqual( + try JSONDecoder().decode( + [ProviderConfig].self, + from: persistedData + ).map(\.accountID), + [accountID] + ) + XCTAssertTrue( + state.saveProviderConfigMetadata(), + "the deletion-only owner gate must not change legacy save callers that do not handle reconciliation failure" + ) + } + @MainActor func testFinalDraftMetadataPersistenceReconcilesSharedOwner() throws { let appDefaults = try XCTUnwrap( @@ -555,7 +835,7 @@ final class ProviderConfigMetadataStoreTests: XCTestCase { } } -private final class DroppingUserDefaults: UserDefaults { +final class DroppingUserDefaults: UserDefaults { var droppedSetKeys: Set = [] override func set(_ value: Any?, forKey defaultName: String) { @@ -565,3 +845,32 @@ private final class DroppingUserDefaults: UserDefaults { super.set(value, forKey: defaultName) } } + +final class MemoryProviderSecretStore: ProviderSecretStoring { + private struct Slot: Hashable { + let key: String + let accessGroup: String? + } + + private var values: [Slot: String] = [:] + + func save( + key: String, + value: String, + accessGroup: String? + ) -> Bool { + values[Slot(key: key, accessGroup: accessGroup)] = value + return true + } + + func load(key: String, accessGroup: String?) -> String? { + values[Slot(key: key, accessGroup: accessGroup)] + } + + func delete(key: String, accessGroup: String?) -> Bool { + values.removeValue( + forKey: Slot(key: key, accessGroup: accessGroup) + ) + return true + } +} From 3d4940bc1c16208381b044c468427e9e3f9e37c3 Mon Sep 17 00:00:00 2001 From: cyq <61975706+cyq1017@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:23:21 +0800 Subject: [PATCH 08/10] test: isolate Claude helper state from XCTest --- .../Claude/ClaudeHelperContract.swift | 3 +- .../Claude/ClaudeSourceStrategy.swift | 20 ++++++- .../ClaudeStrategyTests.swift | 55 +++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeHelperContract.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeHelperContract.swift index 67cf8746..34e0dd43 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeHelperContract.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeHelperContract.swift @@ -79,7 +79,8 @@ public enum ClaudeHelperContract { /// App-group container directory when available to the running process. public static var appGroupHelperDir: String? { - FileManager.default + if ClaudeCredentials.isolatedTestHomeDirectory != nil { return nil } + return FileManager.default .containerURL(forSecurityApplicationGroupIdentifier: appGroupID)? .path } diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift index f494952c..ecbec407 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift @@ -226,9 +226,27 @@ public enum ClaudeResultBuilder { /// Shared helpers for Claude credential resolution, used by multiple strategies. public enum ClaudeCredentials { + private static let isRunningUnderXCTest = NSClassFromString("XCTestCase") != nil + + static var isolatedTestHomeDirectory: String? { + guard isRunningUnderXCTest else { return nil } + if let fixedUserHome = ProcessInfo.processInfo.environment["CFFIXED_USER_HOME"]? + .trimmingCharacters(in: .whitespacesAndNewlines), + !fixedUserHome.isEmpty + { + return fixedUserHome + } + return (NSTemporaryDirectory() as NSString).appendingPathComponent( + "clipulse-xctest-\(ProcessInfo.processInfo.processIdentifier)" + ) + } + /// Real home directory (not sandbox-remapped). Uses the thread-safe - /// `passwdHomeDirectory()` (`getpwuid_r`). + /// `passwdHomeDirectory()` (`getpwuid_r`). XCTest may opt into an isolated + /// home so offline tests never read or overwrite the developer's real + /// Claude credentials and helper snapshots. public static var realHomeDir: String { + if let isolatedTestHomeDirectory { return isolatedTestHomeDirectory } if let dir = passwdHomeDirectory() { return dir } let nsHome = NSHomeDirectory() if let range = nsHome.range(of: "/Library/Containers/") { diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift index f3fd6367..5129ef19 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift @@ -1,9 +1,64 @@ #if os(macOS) import XCTest @testable import CLIPulseCore +import Darwin final class ClaudeStrategyTests: XCTestCase { + func testRealHomeDirDefaultsToProcessIsolatedTemporaryHomeUnderXCTest() { + let key = "CFFIXED_USER_HOME" + let previousValue = getenv(key).map { String(cString: $0) } + _ = unsetenv(key) + defer { + if let previousValue { + _ = setenv(key, previousValue, 1) + } + } + + let expectedSuffix = "clipulse-xctest-\(ProcessInfo.processInfo.processIdentifier)" + XCTAssertTrue(ClaudeCredentials.realHomeDir.hasPrefix(NSTemporaryDirectory())) + XCTAssertTrue(ClaudeCredentials.realHomeDir.hasSuffix(expectedSuffix)) + } + + func testRealHomeDirUsesExplicitFixedHomeUnderXCTest() { + let key = "CFFIXED_USER_HOME" + let previousValue = getenv(key).map { String(cString: $0) } + let isolatedHome = NSTemporaryDirectory() + + "clipulse-claude-home-\(UUID().uuidString)" + + XCTAssertEqual(setenv(key, isolatedHome, 1), 0) + defer { + if let previousValue { + _ = setenv(key, previousValue, 1) + } else { + _ = unsetenv(key) + } + } + + XCTAssertEqual(ClaudeCredentials.realHomeDir, isolatedHome) + } + + func testFixedTestHomeExcludesTheRealAppGroupContainer() { + let key = "CFFIXED_USER_HOME" + let previousValue = getenv(key).map { String(cString: $0) } + let isolatedHome = NSTemporaryDirectory() + + "clipulse-claude-home-\(UUID().uuidString)" + + XCTAssertEqual(setenv(key, isolatedHome, 1), 0) + defer { + if let previousValue { + _ = setenv(key, previousValue, 1) + } else { + _ = unsetenv(key) + } + } + + XCTAssertEqual( + ClaudeHelperContract.helperDirCandidates, + [(isolatedHome as NSString).appendingPathComponent(".clipulse")] + ) + } + // MARK: - ClaudeSnapshot → CollectorResult func testResultBuilderFullSnapshot() { From 0b8a62ca4d5a9d1d1382b1a9fd63e4843b184db8 Mon Sep 17 00:00:00 2001 From: cyq <15000851237@163.com> Date: Mon, 17 Aug 2026 09:08:20 +0800 Subject: [PATCH 09/10] fix: skip Claude keychain reads under XCTest --- .../Claude/ClaudeSourceStrategy.swift | 9 ++++- .../ClaudeStrategyTests.swift | 37 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift index ecbec407..c75f6606 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/Collectors/Claude/ClaudeSourceStrategy.swift @@ -298,6 +298,10 @@ public enum ClaudeCredentials { bypassCooldown: Bool = false, cacheResult: Bool = true ) -> Creds? { + // XCTest uses an isolated Claude home and must not fall through to the + // developer's app cache or Claude Code's cross-app Keychain item. + if isolatedTestHomeDirectory != nil { return nil } + // 1. Try the app's own keychain cache (never triggers a prompt) if let cached = KeychainHelper.load(key: keychainCacheKey), let data = cached.data(using: .utf8), @@ -413,7 +417,10 @@ public enum ClaudeCredentials { return creds } - private static let keychainCacheKey = "claude-code-creds-cache" + // Internal so XCTest can seed the exact cache entry without duplicating a + // security-sensitive key name and accidentally falling through to the + // cross-app Keychain path when that name changes. + static let keychainCacheKey = "claude-code-creds-cache" enum TokenSource: Equatable { case accountConfig diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift index 5129ef19..27883868 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ClaudeStrategyTests.swift @@ -59,6 +59,43 @@ final class ClaudeStrategyTests: XCTestCase { ) } + func testKeychainCredentialReadIsDisabledUnderXCTest() { + let cacheKey = ClaudeCredentials.keychainCacheKey + let previousCachedCredentials = KeychainHelper.load(key: cacheKey) + let cachedCredentials = """ + { + "claude_ai_oauth": { + "access_token": "sk-ant-oat-test", + "rate_limit_tier": "pro" + } + } + """ + + XCTAssertTrue( + KeychainHelper.save(key: cacheKey, value: cachedCredentials) + ) + defer { + if let previousCachedCredentials { + XCTAssertTrue( + KeychainHelper.save( + key: cacheKey, + value: previousCachedCredentials + ) + ) + } else { + XCTAssertTrue(KeychainHelper.delete(key: cacheKey)) + } + } + + XCTAssertNil( + ClaudeCredentials.readKeychainCredentials( + bypassCooldown: true, + cacheResult: false + ), + "XCTest must not consult either the app cache or Claude Code's cross-app Keychain item" + ) + } + // MARK: - ClaudeSnapshot → CollectorResult func testResultBuilderFullSnapshot() { From 412fa950566389b6b1b6885ecb810211f62c399f Mon Sep 17 00:00:00 2001 From: YE <69640321+JasonYeYuhe@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:47:54 +0900 Subject: [PATCH 10/10] fix: an unreadable credential must not brick the account (#435) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readResult` maps every OSStatus other than success/itemNotFound to `.failure`, and `makeSecretPersistenceCheckpoint` returned nil if ANY of its four reads came back that way. Both `saveSecrets` and `deleteSecrets` guarded on that checkpoint, so a single unreadable entry left the account neither editable nor removable — fail-closed had become fail-forever, with no route back for the user short of Keychain Access.app. Reproduced against the real login Keychain by planting a non-UTF8 payload: readResult -> .failure makeSecretPersistence… -> nil saveSecrets -> false deleteSecretsForAccount…-> false This is a regression rather than a pre-existing gap: before the save transaction landed, neither function read anything first, so overwriting an unreadable item succeeded and so did deleting it. The checkpoint now keeps the full `ProviderSecretReadResult` per entry instead of collapsing to `String?`, so "unreadable" is recorded rather than fatal, and `restoreSecrets` skips those entries. Skipping is the only honest option: writing would invent a value we never saw and deleting would destroy one. It does not count against the rollback, because we end up no worse off than before the attempt. Readable entries are still rolled back exactly as before. Same repro after the change: checkpoint is produced, save succeeds and the replacement value is what is stored, and an account holding a corrupt credential can be deleted. `makeSecretPersistenceCheckpoint` keeps its optional return type on purpose, so every existing `guard let` call site compiles unchanged and simply stops tripping. The blast radius is one type and one function. `testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails` pinned the promise that caused this ("if any entry cannot be read, refuse to write at all") and is replaced by two tests that pin the corrected contract. Both were negative- controlled: with the guard restored they fail, with it removed they pass. 2,717 tests, 4 skipped, 0 failures. Co-authored-by: Claude Opus 5 --- .../Sources/CLIPulseCore/ProviderConfig.swift | 63 ++++++++++----- ...roviderAccountKeychainMigrationTests.swift | 78 +++++++++++++++++-- 2 files changed, 113 insertions(+), 28 deletions(-) diff --git a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift index 02001d85..4d79112a 100644 --- a/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift +++ b/CLI Pulse Bar/CLIPulseCore/Sources/CLIPulseCore/ProviderConfig.swift @@ -156,11 +156,18 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { /// Opaque rollback point for the account-scoped secret entries and their /// migration markers. The values stay in memory only for the duration of /// the editor save transaction. + /// + /// Each entry keeps the FULL read result, not just a value, so an entry we + /// could not read is recorded as exactly that rather than collapsing the + /// whole checkpoint. A checkpoint is a rollback *aid*; it must never become + /// a precondition for writing, or one unreadable Keychain item would make + /// an account permanently un-editable and un-deletable — see the header on + /// `makeSecretPersistenceCheckpoint(using:)`. public struct SecretPersistenceCheckpoint { - fileprivate let apiKey: String? - fileprivate let apiKeyMarker: String? - fileprivate let cookie: String? - fileprivate let cookieMarker: String? + fileprivate let apiKey: ProviderSecretReadResult + fileprivate let apiKeyMarker: ProviderSecretReadResult + fileprivate let cookie: ProviderSecretReadResult + fileprivate let cookieMarker: ProviderSecretReadResult } public func makeSecretPersistenceCheckpoint() @@ -314,19 +321,26 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { key: Self.migrationMarkerKey(accountID, "cookie"), accessGroup: group ) - guard - apiKey != .failure, - apiKeyMarker != .failure, - cookie != .failure, - cookieMarker != .failure - else { - return nil - } + // Deliberately NOT `guard ... != .failure else { return nil }`. + // + // That is what this used to do, and it was a trap: `readResult` maps + // EVERY OSStatus other than success/itemNotFound to `.failure`, so a + // single unreadable entry — a denied authorization prompt, an ACL + // mismatch after a re-sign or a MAS/Developer-ID channel switch, a + // missing entitlement, a non-UTF8 payload — made this return nil, and + // both `saveSecrets` and `deleteSecrets` guarded on it. The account + // could then be neither overwritten nor removed: fail-closed had become + // fail-forever, with no route back for the user. Reproduced against the + // real Keychain by planting a non-UTF8 item. + // + // An entry we cannot read has no known previous value, so there is + // nothing to preserve and nothing a caller could usefully refuse over. + // Record it as `.failure` and let `restoreSecrets` skip it. return SecretPersistenceCheckpoint( - apiKey: apiKey.value, - apiKeyMarker: apiKeyMarker.value, - cookie: cookie.value, - cookieMarker: cookieMarker.value + apiKey: apiKey, + apiKeyMarker: apiKeyMarker, + cookie: cookie, + cookieMarker: cookieMarker ) } @@ -336,7 +350,7 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { using store: any ProviderSecretStoring ) -> Bool { let group = Self.secretsAccessGroup - let entries: [(String, String?)] = [ + let entries: [(String, ProviderSecretReadResult)] = [ ( Self.accountKeychainKey(accountID, "apiKey"), checkpoint.apiKey @@ -355,9 +369,10 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { ), ] var restored = true - for (key, value) in entries { + for (key, captured) in entries { let entryRestored: Bool - if let value { + switch captured { + case let .value(value): entryRestored = store.save( key: key, @@ -368,7 +383,7 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { key: key, accessGroup: group ) == .value(value) - } else { + case .missing: entryRestored = store.delete( key: key, @@ -378,6 +393,14 @@ public struct ProviderConfig: Codable, Identifiable, Sendable { key: key, accessGroup: group ) == .missing + case .failure: + // The entry was already unreadable when the checkpoint was + // taken, so there is no prior state to put back. Skipping is + // the only honest option: writing would invent a value and + // deleting would destroy one we never managed to see. It does + // NOT count against the rollback — we are no worse off here + // than before the attempt. + entryRestored = true } restored = entryRestored && restored } diff --git a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift index 76837358..3fe187e5 100644 --- a/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift +++ b/CLI Pulse Bar/CLIPulseCore/Tests/CLIPulseCoreTests/ProviderAccountKeychainMigrationTests.swift @@ -291,9 +291,22 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { ) } - func testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails() - throws - { + /// An unreadable entry must NOT collapse the checkpoint. + /// + /// Replaces `testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails`, + /// which pinned the opposite promise — "if any entry cannot be read, refuse + /// to write at all". That promise was the bug. `readResult` maps every + /// OSStatus other than success/itemNotFound to `.failure`, so one denied + /// authorization prompt, ACL mismatch after a re-sign, missing entitlement + /// or non-UTF8 payload made `makeSecretPersistenceCheckpoint` return nil — + /// and both `saveSecrets` and `deleteSecrets` guarded on it, leaving the + /// account neither editable nor removable with no route back for the user. + /// Reproduced against the real Keychain by planting a non-UTF8 item. + /// + /// The trade this encodes: an entry we could never read cannot be rolled + /// back either, because its previous value was never observed. Readable + /// entries are still rolled back exactly as before. + func testCheckpointSurvivesAnUnreadableEntry() throws { let store = InMemoryProviderSecretStore() let accountID = try XCTUnwrap( UUID( @@ -310,29 +323,78 @@ final class ProviderAccountKeychainMigrationTests: XCTestCase { XCTAssertTrue(original.saveSecrets(using: store)) let apiKey = accountKey(accountID, "apiKey") store.failingLoadKeys.insert(apiKey) + + XCTAssertNotNil( + original.makeSecretPersistenceCheckpoint(using: store), + "an unreadable entry must be recorded, not turned into a refusal" + ) + let edited = ProviderConfig( kind: .claude, accountID: accountID, apiKey: "new-api-key", manualCookieHeader: "new-cookie" ) - + // This double fails reads for that key permanently, so the write-back + // verification inside `persistSecret` can never succeed and the save + // still reports failure. What matters here is what happens to the OTHER + // entry while that is going on. XCTAssertFalse(edited.saveSecrets(using: store)) - store.failingLoadKeys.remove(apiKey) XCTAssertEqual( store.load( - key: apiKey, + key: accountKey(accountID, "cookie"), accessGroup: ProviderConfig.secretsAccessGroup ), - "old-api-key" + "old-cookie", + "the readable entry must still be rolled back" + ) + } + + /// The rollback path itself: a `.failure` entry is skipped rather than + /// invented or destroyed, and skipping it does not count as a failed + /// rollback — we end up no worse off than before the attempt. + func testRestoreSkipsUnreadableEntryAndStillReportsSuccess() throws { + let store = InMemoryProviderSecretStore() + let accountID = try XCTUnwrap( + UUID( + uuidString: + "ADADADAD-ADAD-4DAD-8DAD-ADADADADADAD" + ) + ) + let config = ProviderConfig( + kind: .claude, + accountID: accountID, + apiKey: "seed-api-key", + manualCookieHeader: "seed-cookie" + ) + XCTAssertTrue(config.saveSecrets(using: store)) + + store.failingLoadKeys.insert(accountKey(accountID, "apiKey")) + let checkpoint = try XCTUnwrap( + config.makeSecretPersistenceCheckpoint(using: store) + ) + store.failingLoadKeys.removeAll() + + XCTAssertTrue( + store.save( + key: accountKey(accountID, "cookie"), + value: "drifted-cookie", + accessGroup: ProviderConfig.secretsAccessGroup + ) + ) + + XCTAssertTrue( + config.restoreSecrets(from: checkpoint, using: store), + "skipping an unreadable entry must not be reported as a failed rollback" ) XCTAssertEqual( store.load( key: accountKey(accountID, "cookie"), accessGroup: ProviderConfig.secretsAccessGroup ), - "old-cookie" + "seed-cookie", + "the readable entry is restored" ) }