From d75ae87de2b93a5032eeccc1ecc19608ad8dcad2 Mon Sep 17 00:00:00 2001 From: Victor Quiroz Date: Sat, 25 Jul 2026 13:44:32 +0200 Subject: [PATCH] chore: remove legacy keychain and brand migration code The one-time migrations from the previous brand and from pre-consolidated keychain items have served their purpose. Removing them simplifies the Keychain layer, the app startup path, and the test suite. The ClaudeCode provider's own brand migration is deliberately untouched to preserve provider orthogonality. --- Sources/App/AppMain.swift | 9 - Sources/Core/Keychain.swift | 173 +--------- Sources/Core/KeychainStorage.swift | 52 --- Sources/Core/LegacyBrandMigration.swift | 63 ---- Tests/CoreTests/KeychainTests.swift | 163 ++++++++- .../LegacyBrandKeychainMigrationTests.swift | 317 ------------------ .../CoreTests/LegacyBrandMigrationTests.swift | 97 ------ ...06-remove-keychain-and-brand-migrations.md | 169 ++++++++++ 8 files changed, 341 insertions(+), 702 deletions(-) delete mode 100644 Sources/Core/LegacyBrandMigration.swift delete mode 100644 Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift delete mode 100644 Tests/CoreTests/LegacyBrandMigrationTests.swift create mode 100644 specs/core/06-remove-keychain-and-brand-migrations.md diff --git a/Sources/App/AppMain.swift b/Sources/App/AppMain.swift index 38e29e1..d8a4ee1 100644 --- a/Sources/App/AppMain.swift +++ b/Sources/App/AppMain.swift @@ -19,15 +19,6 @@ struct AppMain: App { registry.register(DeepSeekProvider()) registry.register(OpenAICodexProvider()) registry.register(CursorProvider()) - do { - try LegacyBrandMigration.migratePreferences( - providerIds: registry.registeredProviders.map(\.id) - ) - } catch { - FileHandle.standardError.write( - Data("[Filbert] Preference migration failed: \(error.localizedDescription)\n".utf8) - ) - } _viewModel = State(initialValue: QuotaViewModel(registry: registry)) } diff --git a/Sources/Core/Keychain.swift b/Sources/Core/Keychain.swift index fab1aec..d321008 100644 --- a/Sources/Core/Keychain.swift +++ b/Sources/Core/Keychain.swift @@ -20,13 +20,9 @@ public final class Keychain: @unchecked Sendable { public static let shared = Keychain() private let service: String - private let previousService: String? private let storage: any KeychainStorage /// Account under which the consolidated JSON blob is stored. private let account = "providers" - /// Prefix of the pre-consolidation per-provider accounts, read only during - /// the one-time migration below. - private let legacyAccountPrefix = "provider-" /// Decoded provider-ID → provider-owned secret-field map. `nil` until the /// first keychain read; an empty dictionary is a valid loaded state. @@ -45,19 +41,16 @@ public final class Keychain: @unchecked Sendable { private convenience init() { self.init( storage: SecurityKeychainStorage(), - service: "filbert", - previousService: LegacyBrandIdentifiers.keychainService + service: "filbert" ) } init( storage: any KeychainStorage, - service: String, - previousService: String? + service: String ) { self.storage = storage self.service = service - self.previousService = previousService } public func save(_ key: String, for providerId: String) throws { @@ -108,8 +101,7 @@ public final class Keychain: @unchecked Sendable { private extension Keychain { // MARK: - Store access (all callers hold `lock`) - /// Returns the cached store, loading it from the keychain — migrating any - /// legacy per-provider items — on first use. + /// Returns the cached store, loading it from the keychain on first use. private func loadedStore( authenticationContext: KeychainAuthenticationContext ) throws -> [String: [String: String]] { @@ -141,28 +133,19 @@ private extension Keychain { private func readStore( authenticationContext: KeychainAuthenticationContext ) throws -> LoadedStore { - let current: ConsolidatedStore? do { - if let data = try storage.readData( + guard let data = try storage.readData( service: service, account: account, authenticationContext: authenticationContext - ) { - current = try decodeStore(data, error: .loadFailed(errSecDecode)) - } else { - current = nil + ) else { + return LoadedStore(store: [:], data: nil) } + let store = try decodeStore(data) + return LoadedStore(store: store, data: data) } catch let error as KeychainStorageError { throw KeychainError.loadFailed(error.status) } - - if let current, !current.isLegacy { - return LoadedStore(store: current.store, data: current.data) - } - return try migrateStore( - current: current, - authenticationContext: authenticationContext - ) } private func writeStore( @@ -180,99 +163,6 @@ private extension Keychain { return data } - private func migrateStore( - current: ConsolidatedStore?, - authenticationContext: KeychainAuthenticationContext - ) throws -> LoadedStore { - var migrated: [String: [String: String]] = [:] - var itemsToDelete: [(service: String, account: String)] = [] - var requiresWrite = current?.isLegacy ?? false - - if let previousService { - let previousHasItems = try mergeMigrationItems( - service: previousService, - into: &migrated, - itemsToDelete: &itemsToDelete, - authenticationContext: authenticationContext - ) - requiresWrite = requiresWrite || previousHasItems - - if let consolidated = try legacyConsolidatedStore( - service: previousService, - authenticationContext: authenticationContext - ) { - migrated.merge(consolidated.store) { _, latest in latest } - itemsToDelete.append((previousService, account)) - requiresWrite = true - } - } - - let currentHasItems = try mergeMigrationItems( - service: service, - into: &migrated, - itemsToDelete: &itemsToDelete, - authenticationContext: authenticationContext - ) - requiresWrite = requiresWrite || currentHasItems - - if let current { - migrated.merge(current.store) { _, latest in latest } - } - - guard requiresWrite else { - return LoadedStore(store: current?.store ?? [:], data: current?.data) - } - - let data = try JSONEncoder().encode(migrated) - try replaceAndVerify( - data, - previousData: current?.data, - authenticationContext: authenticationContext, - errorFactory: { .migrationFailed($0) } - ) - for item in itemsToDelete { - storage.delete( - service: item.service, - account: item.account, - authenticationContext: authenticationContext - ) - } - return LoadedStore(store: migrated, data: data) - } - - private func mergeMigrationItems( - service: String, - into migrated: inout [String: [String: String]], - itemsToDelete: inout [(service: String, account: String)], - authenticationContext: KeychainAuthenticationContext - ) throws -> Bool { - let items = try migrationItems( - service: service, - authenticationContext: authenticationContext - ) - migrated.merge(items.store) { _, latest in latest } - itemsToDelete.append(contentsOf: items.accounts.map { (service, $0) }) - return !items.accounts.isEmpty - } - - private func legacyConsolidatedStore( - service: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> ConsolidatedStore? { - do { - guard let data = try storage.readData( - service: service, - account: account, - authenticationContext: authenticationContext - ) else { - return nil - } - return try decodeStore(data, error: .migrationFailed(errSecDecode)) - } catch let error as KeychainStorageError { - throw KeychainError.migrationFailed(error.status) - } - } - private func replaceAndVerify( _ data: Data, previousData: Data?, @@ -330,53 +220,15 @@ private extension Keychain { } } - private func migrationItems( - service: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> (store: [String: [String: String]], accounts: [String]) { - let items: [StoredKeychainItem] + private func decodeStore(_ data: Data) throws -> [String: [String: String]] { do { - items = try storage.readLegacyItems( - service: service, - accountPrefix: legacyAccountPrefix, - authenticationContext: authenticationContext - ) - } catch let error as KeychainStorageError { - throw KeychainError.migrationFailed(error.status) - } - - var migrated: [String: [String: String]] = [:] - var accounts: [String] = [] - for item in items where item.account.hasPrefix(legacyAccountPrefix) { - guard let key = String(data: item.data, encoding: .utf8) else { continue } - let providerId = String(item.account.dropFirst(legacyAccountPrefix.count)) - migrated[providerId] = ["value": key] - accounts.append(item.account) - } - return (migrated, accounts) - } - - private func decodeStore( - _ data: Data, - error: KeychainError - ) throws -> ConsolidatedStore { - if let store = try? JSONDecoder().decode([String: [String: String]].self, from: data) { - return ConsolidatedStore(store: store, data: data, isLegacy: false) - } - if let legacyStore = try? JSONDecoder().decode([String: String].self, from: data) { - let store = legacyStore.mapValues { ["value": $0] } - return ConsolidatedStore(store: store, data: data, isLegacy: true) + return try JSONDecoder().decode([String: [String: String]].self, from: data) + } catch { + throw KeychainError.loadFailed(errSecDecode) } - throw error } } -private struct ConsolidatedStore { - let store: [String: [String: String]] - let data: Data - let isLegacy: Bool -} - private struct LoadedStore { let store: [String: [String: String]] let data: Data? @@ -386,7 +238,6 @@ public enum KeychainError: Error, Equatable { case saveFailed(OSStatus) case loadFailed(OSStatus) case deleteFailed(OSStatus) - case migrationFailed(OSStatus) } private extension KeychainStorageError { diff --git a/Sources/Core/KeychainStorage.swift b/Sources/Core/KeychainStorage.swift index 75859af..ad4abe1 100644 --- a/Sources/Core/KeychainStorage.swift +++ b/Sources/Core/KeychainStorage.swift @@ -6,22 +6,12 @@ final class KeychainAuthenticationContext: @unchecked Sendable { let localAuthenticationContext = LAContext() } -struct StoredKeychainItem: Sendable { - let account: String - let data: Data -} - protocol KeychainStorage: Sendable { func readData( service: String, account: String, authenticationContext: KeychainAuthenticationContext ) throws -> Data? - func readLegacyItems( - service: String, - accountPrefix: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> [StoredKeychainItem] func replaceData( _ data: Data, service: String, @@ -69,48 +59,6 @@ struct SecurityKeychainStorage: KeychainStorage { } } - func readLegacyItems( - service: String, - accountPrefix: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> [StoredKeychainItem] { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecReturnAttributes as String: true, - kSecMatchLimit as String: kSecMatchLimitAll, - kSecUseAuthenticationContext as String: authenticationContext.localAuthenticationContext, - ] - - var result: CFTypeRef? - let status = SecItemCopyMatching(query as CFDictionary, &result) - if status == errSecItemNotFound { - return [] - } - guard status == errSecSuccess else { - throw KeychainStorageError.status(status) - } - guard let items = result as? [[String: Any]] else { - throw KeychainStorageError.status(errSecDecode) - } - - var legacyItems: [StoredKeychainItem] = [] - for item in items { - guard let account = item[kSecAttrAccount as String] as? String, - account.hasPrefix(accountPrefix), - let data = try readData( - service: service, - account: account, - authenticationContext: authenticationContext - ) - else { - continue - } - legacyItems.append(StoredKeychainItem(account: account, data: data)) - } - return legacyItems - } - func replaceData( _ data: Data, service: String, diff --git a/Sources/Core/LegacyBrandMigration.swift b/Sources/Core/LegacyBrandMigration.swift deleted file mode 100644 index e0dcd8d..0000000 --- a/Sources/Core/LegacyBrandMigration.swift +++ /dev/null @@ -1,63 +0,0 @@ -import Foundation - -public enum LegacyBrandMigration { - public static func migratePreferences(providerIds: [String]) throws { - try migratePreferences( - sourceDomainName: LegacyBrandIdentifiers.bundleIdentifier, - destination: .standard, - providerIds: providerIds - ) - } - - static func migratePreferences( - sourceDomainName: String, - destination: UserDefaults, - providerIds: [String] - ) throws { - guard let source = destination.persistentDomain(forName: sourceDomainName) else { - return - } - - let keys = preferenceKeys(providerIds: providerIds) - var copiedKeys = Set() - for key in keys where destination.object(forKey: key) == nil { - guard let value = source[key] else { continue } - destination.set(value, forKey: key) - copiedKeys.insert(key) - } - - for key in copiedKeys { - guard let destinationValue = destination.object(forKey: key), - let sourceValue = source[key], - valuesMatch(sourceValue, destinationValue) - else { - throw LegacyBrandMigrationError.preferenceVerificationFailed(key) - } - } - - destination.removePersistentDomain(forName: sourceDomainName) - } - - private static func preferenceKeys(providerIds: [String]) -> [String] { - [ - "provider-order", - "balance-thresholds-low", - "balance-thresholds-ok", - "provider-collapse-state", - "vintage-mac-icon-enabled", - ] + providerIds.map { "provider-\($0)-base-url" } - } - - private static func valuesMatch(_ lhs: Any, _ rhs: Any) -> Bool { - NSDictionary(dictionary: ["value": lhs]).isEqual(to: ["value": rhs]) - } -} - -enum LegacyBrandIdentifiers { - static let bundleIdentifier = "com.victorhqc.ai-usage" - static let keychainService = "ai-usage" -} - -public enum LegacyBrandMigrationError: Error, Equatable { - case preferenceVerificationFailed(String) -} diff --git a/Tests/CoreTests/KeychainTests.swift b/Tests/CoreTests/KeychainTests.swift index 5c38371..a2e7d0b 100644 --- a/Tests/CoreTests/KeychainTests.swift +++ b/Tests/CoreTests/KeychainTests.swift @@ -1,7 +1,11 @@ -import Core +@testable import Core +import Foundation +import Security import XCTest final class KeychainTests: XCTestCase { + private let currentService = "filbert" + func testKeychainError_casesExist() { // Verify KeychainError cases exist and are distinct. // We can't test real Keychain I/O in CI, but the enum must compile. @@ -9,10 +13,9 @@ final class KeychainTests: XCTestCase { .saveFailed(-1), .loadFailed(-1), .deleteFailed(-1), - .migrationFailed(-1), ] - XCTAssertEqual(errors.count, 4) + XCTAssertEqual(errors.count, 3) } func testKeychainError_isEquatable() { @@ -23,4 +26,158 @@ final class KeychainTests: XCTestCase { XCTAssertEqual(lhs, rhs) XCTAssertNotEqual(lhs, other) } + + func testAbsentItemLoadsEmptyStoreThenCreatesOnFirstSave() throws { + let storage = InMemoryKeychainStorage() + let keychain = makeKeychain(storage: storage) + + XCTAssertNil(storage.items[currentService]?["providers"]) + + try keychain.save("zai-key", for: "zai") + let savedData = try XCTUnwrap(storage.items[currentService]?["providers"]) + XCTAssertEqual( + try JSONDecoder().decode([String: [String: String]].self, from: savedData), + ["zai": ["value": "zai-key"]] + ) + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + } + + func testLegacyFlatPayloadSurfacesAsLoadError() throws { + let storage = InMemoryKeychainStorage() + let payload = try JSONEncoder().encode(["zai": "legacy-key"]) + storage.items[currentService] = [ + "providers": payload, + ] + let keychain = makeKeychain(storage: storage) + + XCTAssertThrowsError(try keychain.load(for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .loadFailed(errSecDecode)) + } + // The store is untouched — recovery happens via the normal setup flow, + // not an in-place rewrite (core 06 AC2). + XCTAssertNotNil(storage.items[currentService]?["providers"]) + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testFieldMapsRoundTripAndPreserveOtherProviders() throws { + let storage = InMemoryKeychainStorage() + let zaiFields = ["value": "zai-key", "metadata": "keep"] + let payload = try JSONEncoder().encode(["zai": zaiFields]) + storage.items[currentService] = [ + "providers": payload, + ] + let keychain = makeKeychain(storage: storage) + + try keychain.save( + ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"], + for: "cursor" + ) + + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + XCTAssertEqual( + try keychain.loadFields(for: "cursor"), + ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"] + ) + let savedData = try XCTUnwrap(storage.items[currentService]?["providers"]) + let saved = try JSONDecoder().decode([String: [String: String]].self, from: savedData) + XCTAssertEqual(saved["zai"], zaiFields) + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testFailedUpdatePreservesCachedAndStoredFields() throws { + let storage = InMemoryKeychainStorage() + let original = ["zai": ["value": "zai-key"], "deepseek": ["value": "deepseek-key"]] + let originalData = try JSONEncoder().encode(original) + storage.items[currentService] = ["providers": originalData] + let keychain = makeKeychain(storage: storage) + + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + storage.replaceError = errSecNotAvailable + + XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) + } + XCTAssertEqual(storage.items[currentService]?["providers"], originalData) + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + XCTAssertEqual(try keychain.load(for: "deepseek"), "deepseek-key") + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testCreateFailureDoesNotCreateConsolidatedItem() { + let storage = InMemoryKeychainStorage() + storage.createError = errSecNotAvailable + let keychain = makeKeychain(storage: storage) + + XCTAssertThrowsError(try keychain.save("zai-key", for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) + } + XCTAssertNil(storage.items[currentService]?["providers"]) + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + func testVerificationFailureRestoresExistingConsolidatedItem() throws { + let storage = InMemoryKeychainStorage() + let originalData = try JSONEncoder().encode(["zai": ["value": "zai-key"]]) + storage.items[currentService] = ["providers": originalData] + let keychain = makeKeychain(storage: storage) + + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + storage.corruptNextReplacement = true + + XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in + XCTAssertEqual(error as? KeychainError, .saveFailed(errSecVerifyFailed)) + } + XCTAssertEqual(storage.items[currentService]?["providers"], originalData) + XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") + XCTAssertTrue(storage.deletedItems.isEmpty) + } + + private func makeKeychain(storage: InMemoryKeychainStorage) -> Keychain { + Keychain(storage: storage, service: currentService) + } +} + +private final class InMemoryKeychainStorage: KeychainStorage, @unchecked Sendable { + var items: [String: [String: Data]] = [:] + var deletedItems: [(service: String, account: String)] = [] + var replaceError: OSStatus? + var createError: OSStatus? + var corruptNextReplacement = false + + func readData( + service: String, + account: String, + authenticationContext: KeychainAuthenticationContext + ) throws -> Data? { + _ = authenticationContext + return items[service]?[account] + } + + func replaceData( + _ data: Data, + service: String, + account: String, + authenticationContext: KeychainAuthenticationContext + ) throws { + _ = authenticationContext + if let replaceError { + throw KeychainStorageError.status(replaceError) + } + if items[service]?[account] == nil, let createError { + throw KeychainStorageError.status(createError) + } + let replacement = corruptNextReplacement ? Data("not-json".utf8) : data + corruptNextReplacement = false + items[service, default: [:]][account] = replacement + } + + func delete( + service: String, + account: String, + authenticationContext: KeychainAuthenticationContext + ) { + _ = authenticationContext + items[service]?[account] = nil + deletedItems.append((service, account)) + } } diff --git a/Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift b/Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift deleted file mode 100644 index f4096a6..0000000 --- a/Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift +++ /dev/null @@ -1,317 +0,0 @@ -@testable import Core -import Foundation -import Security -import XCTest - -final class LegacyBrandKeychainMigrationTests: XCTestCase { - private let currentService = "filbert" - private let previousService = "ai-usage" - - func testLoadMigratesConsolidatedAndPerProviderSecretsThenDeletesOldItems() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = try [ - "providers": JSONEncoder().encode(["zai": "consolidated-key"]), - "provider-deepseek": Data("deepseek-key".utf8), - ] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "consolidated-key") - XCTAssertEqual(try keychain.load(for: "deepseek"), "deepseek-key") - - let migratedData = try XCTUnwrap(storage.items[currentService]?["providers"]) - let migrated = try JSONDecoder().decode([String: [String: String]].self, from: migratedData) - XCTAssertEqual( - migrated, - [ - "zai": ["value": "consolidated-key"], - "deepseek": ["value": "deepseek-key"], - ] - ) - XCTAssertNil(storage.items[previousService]?["providers"]) - XCTAssertNil(storage.items[previousService]?["provider-deepseek"]) - } - - func testLoadPrefersCurrentServicePerProviderItemDuringMigration() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = try [ - "providers": JSONEncoder().encode(["zai": "previous-key"]), - ] - storage.items[currentService] = [ - "provider-zai": Data("current-key".utf8), - ] - - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "current-key") - } - - func testMigrationReadsSecretDataOnlyForLegacyProviderAccounts() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = [ - "provider-zai": Data("zai-key".utf8), - "unrelated": Data("unrelated-secret".utf8), - ] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual( - storage.legacyDataReadRequests, - [LegacyDataReadRequest(service: previousService, account: "provider-zai")] - ) - } - - func testMigrationReusesOneAuthenticationContext() throws { - let storage = InMemoryKeychainStorage() - storage.items[previousService] = ["provider-zai": Data("zai-key".utf8)] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual(storage.authenticationContextIdentifiers.count, 1) - } - - func testStructuredStoreBypassesLegacyMigration() throws { - let storage = InMemoryKeychainStorage() - storage.items[currentService] = try [ - "providers": JSONEncoder().encode(["zai": ["value": "zai-key"]]), - ] - storage.items[previousService] = ["provider-deepseek": Data("legacy-key".utf8)] - storage.legacyItemReadError = errSecAuthFailed - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - try keychain.save("deepseek-key", for: "deepseek") - - XCTAssertTrue(storage.legacyItemLookupServices.isEmpty) - XCTAssertEqual( - try keychain.load(for: "deepseek"), - "deepseek-key" - ) - } - - func testMigrationFailureLeavesPreviousItemsIntact() throws { - let storage = InMemoryKeychainStorage() - let previousData = try JSONEncoder().encode(["zai": "secret-key"]) - storage.items[previousService] = ["providers": previousData] - storage.replaceError = errSecNotAvailable - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.load(for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .migrationFailed(errSecNotAvailable)) - } - XCTAssertEqual(storage.items[previousService]?["providers"], previousData) - XCTAssertFalse( - storage.deletedItems.contains { - $0.service == previousService && $0.account == "providers" - } - ) - } - - func testDeniedLegacyReadLeavesEveryItemIntact() { - let storage = InMemoryKeychainStorage() - let legacyData = Data("legacy-key".utf8) - storage.items[previousService] = ["provider-zai": legacyData] - storage.legacyItemReadError = errSecAuthFailed - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.load(for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .migrationFailed(errSecAuthFailed)) - } - XCTAssertEqual(storage.items[previousService]?["provider-zai"], legacyData) - XCTAssertNil(storage.items[currentService]?["providers"]) - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testVerificationFailureLeavesPreviousItemsIntact() throws { - let storage = InMemoryKeychainStorage() - let previousData = try JSONEncoder().encode(["zai": "secret-key"]) - storage.items[previousService] = ["providers": previousData] - storage.corruptNextReplacement = true - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.load(for: "zai")) { error in - guard case .migrationFailed = error as? KeychainError else { - XCTFail("Expected migrationFailed, got \(error)") - return - } - } - XCTAssertEqual(storage.items[previousService]?["providers"], previousData) - XCTAssertFalse( - storage.deletedItems.contains { - $0.service == previousService && $0.account == "providers" - } - ) - } - - func testLoadConvertsCurrentServiceLegacyPayloadWithoutChangingAPIKeyLoads() throws { - let storage = InMemoryKeychainStorage() - storage.items[currentService] = try [ - "providers": JSONEncoder().encode(["zai": "legacy-key"]), - ] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "legacy-key") - - let migratedData = try XCTUnwrap(storage.items[currentService]?["providers"]) - XCTAssertEqual( - try JSONDecoder().decode([String: [String: String]].self, from: migratedData), - ["zai": ["value": "legacy-key"]] - ) - } - - func testFieldMapsRoundTripAndPreserveOtherProviders() throws { - let storage = InMemoryKeychainStorage() - let zaiFields = ["value": "zai-key", "metadata": "keep"] - storage.items[currentService] = try [ - "providers": JSONEncoder().encode(["zai": zaiFields]), - ] - let keychain = makeKeychain(storage: storage) - - try keychain.save( - ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"], - for: "cursor" - ) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual( - try keychain.loadFields(for: "cursor"), - ["accessToken": "cursor-access", "refreshToken": "cursor-refresh"] - ) - let savedData = try XCTUnwrap(storage.items[currentService]?["providers"]) - let saved = try JSONDecoder().decode([String: [String: String]].self, from: savedData) - XCTAssertEqual(saved["zai"], zaiFields) - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testFailedUpdatePreservesCachedAndStoredFields() throws { - let storage = InMemoryKeychainStorage() - let original = ["zai": ["value": "zai-key"], "deepseek": ["value": "deepseek-key"]] - let originalData = try JSONEncoder().encode(original) - storage.items[currentService] = ["providers": originalData] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - storage.replaceError = errSecNotAvailable - - XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) - } - XCTAssertEqual(storage.items[currentService]?["providers"], originalData) - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertEqual(try keychain.load(for: "deepseek"), "deepseek-key") - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testCreateFailureDoesNotCreateConsolidatedItem() { - let storage = InMemoryKeychainStorage() - storage.createError = errSecNotAvailable - let keychain = makeKeychain(storage: storage) - - XCTAssertThrowsError(try keychain.save("zai-key", for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .saveFailed(errSecNotAvailable)) - } - XCTAssertNil(storage.items[currentService]?["providers"]) - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - func testVerificationFailureRestoresExistingConsolidatedItem() throws { - let storage = InMemoryKeychainStorage() - let originalData = try JSONEncoder().encode(["zai": ["value": "zai-key"]]) - storage.items[currentService] = ["providers": originalData] - let keychain = makeKeychain(storage: storage) - - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - storage.corruptNextReplacement = true - - XCTAssertThrowsError(try keychain.save("new-key", for: "zai")) { error in - XCTAssertEqual(error as? KeychainError, .saveFailed(errSecVerifyFailed)) - } - XCTAssertEqual(storage.items[currentService]?["providers"], originalData) - XCTAssertEqual(try keychain.load(for: "zai"), "zai-key") - XCTAssertTrue(storage.deletedItems.isEmpty) - } - - private func makeKeychain(storage: InMemoryKeychainStorage) -> Keychain { - Keychain( - storage: storage, - service: currentService, - previousService: previousService - ) - } -} - -private struct LegacyDataReadRequest: Equatable { - let service: String - let account: String -} - -private final class InMemoryKeychainStorage: KeychainStorage, @unchecked Sendable { - var items: [String: [String: Data]] = [:] - var deletedItems: [(service: String, account: String)] = [] - var replaceError: OSStatus? - var createError: OSStatus? - var corruptNextReplacement = false - var legacyDataReadRequests: [LegacyDataReadRequest] = [] - var legacyItemLookupServices: [String] = [] - var legacyItemReadError: OSStatus? - var authenticationContextIdentifiers = Set() - - func readData( - service: String, - account: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> Data? { - record(authenticationContext) - return items[service]?[account] - } - - func readLegacyItems( - service: String, - accountPrefix: String, - authenticationContext: KeychainAuthenticationContext - ) throws -> [StoredKeychainItem] { - record(authenticationContext) - legacyItemLookupServices.append(service) - if let legacyItemReadError { - throw KeychainStorageError.status(legacyItemReadError) - } - return (items[service] ?? [:]).compactMap { account, data in - guard account.hasPrefix(accountPrefix) else { return nil } - legacyDataReadRequests.append( - LegacyDataReadRequest(service: service, account: account) - ) - return StoredKeychainItem(account: account, data: data) - } - } - - func replaceData( - _ data: Data, - service: String, - account: String, - authenticationContext: KeychainAuthenticationContext - ) throws { - record(authenticationContext) - if let replaceError { - throw KeychainStorageError.status(replaceError) - } - if items[service]?[account] == nil, let createError { - throw KeychainStorageError.status(createError) - } - let replacement = corruptNextReplacement ? Data("not-json".utf8) : data - corruptNextReplacement = false - items[service, default: [:]][account] = replacement - } - - func delete( - service: String, - account: String, - authenticationContext: KeychainAuthenticationContext - ) { - record(authenticationContext) - items[service]?[account] = nil - deletedItems.append((service, account)) - } - - private func record(_ authenticationContext: KeychainAuthenticationContext) { - authenticationContextIdentifiers.insert(ObjectIdentifier(authenticationContext)) - } -} diff --git a/Tests/CoreTests/LegacyBrandMigrationTests.swift b/Tests/CoreTests/LegacyBrandMigrationTests.swift deleted file mode 100644 index 6b8cc6d..0000000 --- a/Tests/CoreTests/LegacyBrandMigrationTests.swift +++ /dev/null @@ -1,97 +0,0 @@ -@testable import Core -import XCTest - -final class LegacyBrandMigrationTests: XCTestCase { - private var destinationDomain: String! - private var sourceDomain: String! - private var defaults: UserDefaults! - - override func setUpWithError() throws { - try super.setUpWithError() - destinationDomain = "filbert.tests.brand-migration.\(UUID().uuidString)" - sourceDomain = "legacy.tests.brand-migration.\(UUID().uuidString)" - defaults = try XCTUnwrap(UserDefaults(suiteName: destinationDomain)) - defaults.removePersistentDomain(forName: destinationDomain) - defaults.removePersistentDomain(forName: sourceDomain) - } - - override func tearDown() { - defaults.removePersistentDomain(forName: destinationDomain) - defaults.removePersistentDomain(forName: sourceDomain) - defaults = nil - sourceDomain = nil - destinationDomain = nil - super.tearDown() - } - - func testMigratePreferencesCopiesOnlyKnownValuesAndRemovesSourceDomain() throws { - defaults.setPersistentDomain( - [ - "provider-order": ["zai", "deepseek"], - "provider-zai-base-url": "https://proxy.example.com", - "balance-thresholds-low": 7.0, - "provider-collapse-state": ["zai": true], - "vintage-mac-icon-enabled": true, - "unrelated-value": "leave behind", - ], - forName: sourceDomain - ) - - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: ["zai"] - ) - - XCTAssertEqual(defaults.array(forKey: "provider-order") as? [String], ["zai", "deepseek"]) - XCTAssertEqual( - defaults.string(forKey: "provider-zai-base-url"), - "https://proxy.example.com" - ) - XCTAssertEqual(defaults.double(forKey: "balance-thresholds-low"), 7) - XCTAssertEqual( - defaults.dictionary(forKey: "provider-collapse-state") as? [String: Bool], - ["zai": true] - ) - XCTAssertTrue(defaults.bool(forKey: "vintage-mac-icon-enabled")) - XCTAssertNil(defaults.object(forKey: "unrelated-value")) - XCTAssertNil(defaults.persistentDomain(forName: sourceDomain)) - } - - func testMigratePreferencesKeepsExistingFilbertValues() throws { - defaults.set(["deepseek", "zai"], forKey: "provider-order") - defaults.setPersistentDomain( - ["provider-order": ["zai", "deepseek"]], - forName: sourceDomain - ) - - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: ["zai", "deepseek"] - ) - - XCTAssertEqual(defaults.array(forKey: "provider-order") as? [String], ["deepseek", "zai"]) - XCTAssertNil(defaults.persistentDomain(forName: sourceDomain)) - } - - func testMigratePreferencesIsIdempotent() throws { - defaults.setPersistentDomain( - ["balance-thresholds-ok": 25.0], - forName: sourceDomain - ) - - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: [] - ) - try LegacyBrandMigration.migratePreferences( - sourceDomainName: sourceDomain, - destination: defaults, - providerIds: [] - ) - - XCTAssertEqual(defaults.double(forKey: "balance-thresholds-ok"), 25) - } -} diff --git a/specs/core/06-remove-keychain-and-brand-migrations.md b/specs/core/06-remove-keychain-and-brand-migrations.md new file mode 100644 index 0000000..1744263 --- /dev/null +++ b/specs/core/06-remove-keychain-and-brand-migrations.md @@ -0,0 +1,169 @@ +## Objective + +Retire the one-time Keychain consolidation migration and the Core preferences +branding migration now that they have served their purpose. + +## Context + +- `Sources/Core/Keychain.swift` — `migrateStore`, `mergeMigrationItems`, + `legacyConsolidatedStore`, `migrationItems`, the `previousService` / + `legacyAccountPrefix` properties, the `[String: String]` fallback + + `isLegacy` in `decodeStore`, and `KeychainError.migrationFailed` are all + migration-only; they exist to land (core 04 AC2, AC3) and are dead weight + afterwards. +- `Sources/Core/KeychainStorage.swift` — `StoredKeychainItem` and + `KeychainStorage.readLegacyItems` (protocol requirement + + `SecurityKeychainStorage` impl) exist only to feed the per-provider item + scan inside `migrateStore`. +- `Sources/Core/LegacyBrandMigration.swift` — the whole file + (`LegacyBrandMigration`, `LegacyBrandIdentifiers`, + `LegacyBrandMigrationError`) is the one-time `com.victorhqc.ai-usage` → + current bundle-id preferences rename. +- `Sources/App/AppMain.swift` (L22–30) — the `do/catch` block that calls + `LegacyBrandMigration.migratePreferences(providerIds:)` in `init()`; it is + the only call site and goes away with the migration. +- `Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift` and + `Tests/CoreTests/LegacyBrandMigrationTests.swift` — migration coverage to + delete; `Tests/CoreTests/KeychainTests.swift` references + `.migrationFailed` and needs trimming. +- Non-migration behavior of (core 04) is unchanged: the consolidated + field-map API (AC1), in-place update-or-create with read-back verify and + restore-on-failure (AC3), and Cursor's shared-vault read / bootstrap / + re-import (AC4, AC7) all stay. +- Out of scope: the ClaudeCode provider's own brand migration + (`Sources/Providers/ClaudeCode/LegacyBrandMigration.swift`, + `StatuslineHelperInstaller`'s `migrateLegacy*` / `hasLegacy*` / + `removeLegacyArtifacts`). It is provider-local, still reachable through + (providers 02) setup paths, and mixing it into a Core cleanup would + violate provider orthogonality (AGENTS.md §1). Track it under its own + provider-scoped spec if desired. + +## Acceptance Criteria + +### AC1: Keychain loads the consolidated item directly, with no migration path + +- **Given** the `filbert` / `providers` item holds a + `[String: [String: String]]` payload (or no item exists yet) +- **When** any provider calls `Keychain.load` / `loadFields` / `save` / + `delete` +- **Then** the store is decoded from the current service and account only, + with no read of any other service, no per-provider item scan, and no + payload rewrite on first load +- **And** an absent item yields an empty in-memory store (as today), and + the first `save` creates it + +### AC2: Legacy Keychain payloads surface as a typed error, not a migration or crash + +- **Given** the `filbert` / `providers` item still holds a pre-(core 04) + `[String: String]` payload, or a stale `ai-usage` service item, or a + `provider-` item exists +- **When** the Keychain is loaded +- **Then** the current item either decodes or throws + `KeychainError.loadFailed(...)`; the app never prompts for a legacy + service, never scans for per-provider items, and never deletes anything +- **And** the user recovers through the normal setup flow — re-entering an + API key or triggering Cursor's re-import (core 04 AC7) — without data + loss beyond the already-stale pre-migration secret + +### AC3: Migration code is gone from Core's Keychain layer + +- **Given** the change lands +- **When** inspecting `Sources/Core/Keychain.swift` and + `Sources/Core/KeychainStorage.swift` +- **Then** none of `migrateStore`, `mergeMigrationItems`, + `legacyConsolidatedStore`, `migrationItems`, `previousService`, + `legacyAccountPrefix`, `ConsolidatedStore.isLegacy`, + `KeychainError.migrationFailed`, `KeychainStorage.readLegacyItems`, + `SecurityKeychainStorage.readLegacyItems`, or `StoredKeychainItem` exist +- **And** `decodeStore` decodes only `[String: [String: String]]` +- **And** `Keychain` no longer references `LegacyBrandIdentifiers` + +### AC4: The preferences branding migration is fully removed + +- **Given** the change lands +- **When** inspecting `Sources/Core/LegacyBrandMigration.swift` and + `Sources/App/AppMain.swift` +- **Then** the file `Sources/Core/LegacyBrandMigration.swift` is deleted + (including `LegacyBrandMigration`, `LegacyBrandIdentifiers`, and + `LegacyBrandMigrationError`) +- **And** the `do/catch` block calling + `migratePreferences(providerIds:)` in `AppMain.init()` is gone, so + startup proceeds straight from provider registration to + `QuotaViewModel` construction +- **And** no remaining production code references any of the removed + symbols + +### AC5: Non-migration Keychain behavior is unchanged + +- **Given** the change lands +- **When** providers save / load / delete API keys, save / load Cursor + field maps, Cursor refresh updates only its access-token field, or a + save fails mid-write +- **Then** behavior matches (core 04 AC1, AC3, AC6, AC8): field-map round + trips preserve every other provider's fields, in-place update-or-create + with read-back verification still runs, a failed save restores the prior + item and leaves the in-memory cache intact, and Cursor's shared-vault + read / bootstrap / re-import path is untouched + +### AC6: Tests reflect the removal and keep non-migration coverage + +- **Given** the completed implementation +- **When** `swift test` runs +- **Then** `Tests/CoreTests/LegacyBrandKeychainMigrationTests.swift` and + `Tests/CoreTests/LegacyBrandMigrationTests.swift` are deleted +- **And** `Tests/CoreTests/KeychainTests.swift` no longer references + `.migrationFailed` (the case no longer exists) +- **And** the non-migration assertions that lived in + `LegacyBrandKeychainMigrationTests` (field-map round trip preserving + other providers, failed-update preservation, create-failure and + verify-failure restoration) survive — relocated into + `KeychainTests.swift` if not already covered elsewhere — so AC5 keeps + coverage +- **And** `swift build` and the full `swift test` suite pass with no + warnings + +## Plan + +1. **Simplify `Keychain.readStore`.** Return the current payload directly + (or an empty store when the item is absent). Delete `migrateStore`, + `mergeMigrationItems`, `legacyConsolidatedStore`, `migrationItems`, + the `previousService` and `legacyAccountPrefix` properties, and the + `previousService` init parameter. In `decodeStore`, keep only the + `[String: [String: String]]` branch and drop `isLegacy` from + `ConsolidatedStore`. Remove `KeychainError.migrationFailed`. +2. **Drop the migration seam from `KeychainStorage`.** Remove + `readLegacyItems` from the protocol and the + `SecurityKeychainStorage` implementation, and delete + `StoredKeychainItem`. `InMemoryKeychainStorage` in the tests loses the + same method. +3. **Remove the branding migration.** Delete + `Sources/Core/LegacyBrandMigration.swift` and the `migratePreferences` + `do/catch` block in `AppMain.init()`. +4. **Update tests.** Delete the two migration test files; trim + `KeychainTests.testKeychainError_casesExist` to the remaining cases; + move any non-migration safety assertions (AC5 behavior) into + `KeychainTests.swift` where they are not already covered. + +No production code is written until this spec is reviewed. + +## Risks + +- **Stragglers lose silent migration.** A user who has not launched Filbert + since before (core 04) still carries a `[String: String]` payload or an + `ai-usage` item. After this change they see a load error / empty store + and must re-enter keys or re-import Cursor credentials (AC2). That is + the explicit cost of retiring the migration; confirm it is acceptable + before implementing, and call it out in release notes. Mitigation: the + failure is typed and non-destructive, and Cursor recovers via its + existing re-import action (core 04 AC7). +- **The consolidated item is the only supported payload.** Removing the + schema fallback means a corrupt or hand-edited item is no longer papered + over by a re-migration; it surfaces as `loadFailed`. Acceptable, but + worth a release-note line. +- **Provider orthogonality.** The ClaudeCode provider's brand migration + (helper/cache rename) is deliberately out of scope. Bundling it would + mix a provider change into a Core cleanup (AGENTS.md §1); track it + separately. +- **Test relocation.** Several assertions in + `LegacyBrandKeychainMigrationTests` cover non-migration safety netted by + AC5. They must survive the file deletion, or AC5 loses coverage.