fix: an unreadable credential must not brick the account

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
         }

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"
         )
     }