fix: an unreadable credential must not brick the account

[JasonYeYuhe]

Targets #432's branch, not main — it's your PR, so this is yours to take or rework rather than something pushed over you.

Fixes the blocking finding from my review of #432, with the reproduction that found it.

What was wrong

readResult maps every OSStatus other than errSecSuccess/errSecItemNotFound 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 one unreadable entry left the account neither editable nor removable. Fail-closed had become fail-forever, with no route back short of Keychain Access.app.

This is a regression, not a pre-existing gap — before the transaction landed, neither function read anything first, so overwriting an unreadable item worked and so did deleting it.

The change

The checkpoint now stores 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 observed, 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.

makeSecretPersistenceCheckpoint deliberately keeps its optional return type, so every existing guard let call site compiles unchanged and simply stops tripping. Blast radius is one type and one function — I did not touch the editor, the transaction, or the ordering you designed.

The trade this encodes

An entry we could never read cannot be rolled back either, because its previous value was never observed. That is a real cost, and it is strictly better than the alternative, which is the brick.

Verification

Real login Keychain, via a throwaway harness driving the actual code (non-UTF8 payload planted with SecItemAdd):

	before	after
makeSecretPersistenceCheckpoint	nil	produced
saveSecrets	false	true, and the replacement value is what's stored
deleteSecretsForAccountRemoval	false	true, and the entry is actually gone

Offline suite: 2,717 tests, 4 skipped, 0 failures.

testSaveSecretsFailsWithoutMutationWhenCheckpointReadFails pinned the exact promise that caused this — "if any entry cannot be read, refuse to write at all" — so it is replaced by two tests pinning the corrected contract. Both were negative-controlled: I reverted the fix and confirmed they fail, then restored it and confirmed they pass. They are not tautologies.

Not included

The harness is a throwaway .executableTarget; I kept it out of this commit rather than adding a product target that CI can't run (it needs a real Keychain). Happy to hand it over — it inventories the shared provider-scoped namespace first and refuses to run unless the probe kind is empty, then cleans up every key it creates.

Still open from the review, untouched here: ProviderSharedCredentialOwner.defaults resolves to the production suite under XCTest, so running the offline suite rewrites the developer's real shared App Group defaults. Pre-existing on main, not yours.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.