Encode the keychain "not found" contract in `KeychainAccessible` (test doubles have drifted)

[jkmassel]

Problem

KeychainAccessible types the signature but not the contract:

func getPassword(for username: String, serviceName: String) throws -> String

A missing item can only be signaled by throwing, but nothing says which throw means "not found" vs. a real failure. The real implementations (AppKeychain, SharedKeychain, KeychainUtils) throw a bridged SFHFKeychainUtils NSError on a miss, and the not-found-vs-failure distinction is recovered after the fact by a free function — isRealKeychainFailure — that sniffs the NSError domain/code. The contract is implicit and stringly-typed.

Because nothing pins it down, the test doubles have each guessed differently:

Double	not-found behavior
Tests/KeystoneTests/Tests/Jetpack/KeychainUtilsMock.swift	returns "" (no throw)
Tests/KeystoneTests/Helpers/TestKeychain.swift	throws TestKeychainErrors.keychainItemNotFound
Modules/Tests/WordPressDataTests/Helpers/MockKeychainService.swift	throws MockKeychainError.notFound (and keys storage by username only, ignoring serviceName)
Modules/Tests/WordPressSharedTests/KeychainStub.swift	throws StubError.notFound

Three throw (three different error types); one returns an empty string; they also disagree on the storage/key model.

Why it bites

SharedDataIssueSolver.migrateAuthKey has no test coverage, and the ""-returning KeychainUtilsMock (injected by the migrator tests) makes it hard to add: the "already migrated" guard if let _ = try? appKeychain.getPassword(…, .jetpack) always succeeds against that mock, so the migration write path can never be exercised. More broadly, a test written against the wrong double can pass while the real keychain behaves differently on a miss.

Proposed fix

Move the contract into the type:

enum KeychainError: Error { case notFound; case failure(OSStatus) }
func getPassword(for username: String, serviceName: String) throws(KeychainError) -> String

• Every conformer — real and test — must produce .notFound for a miss; the compiler enforces it, and "" is no longer a way to signal absence.
• The classification happens once, at the boundary (in AppKeychain/SharedKeychain, mapping the SFHFKeychainUtils NSError), and isRealKeychainFailure goes away — it only exists because the current contract can't express the distinction.
• Then consolidate the four doubles into a single faithful in-memory implementation.

A lighter version is throws -> String? (nil = not found), which beats today's contract but doesn't carry the failure distinction.

Provenance

Pre-existing. The protocol and the "" mock are both on trunk (the mock since at least #24462, Apr 2025; trunk's migrateAuthKey has the identical untested try? pattern). Surfaced while reviewing #25654 (the keychain access-group split); #25658 is a companion fix for an unrelated logout bug. This is not a blocker for either — filing separately so the redesign isn't loaded onto the access-group work.

[dangermattic]

Thanks for reporting! 👍

[jkmassel]

Two more things surfaced while reviewing #25654 that belong against this redesign:

The brand gate is a second, independent obstacle to testing migrateAuthKey. Even with a faithful not-found double, the test still can't reach the write path: migrateAuthKey reads BuildSettings.current.brand == .jetpack directly, and KeystoneTests run in WordPress.app (brand .wordpress), where BuildSettings.withSettings (which only mutates .preview) doesn't apply. So whoever fixes the mock here will close the "" problem and still be blocked on the brand guard. Injecting it — brand: AppBrand = BuildSettings.current.brand in SharedDataIssueSolver.init — clears it in one line and removes a hidden global dependency.

The -1999 classification decision lands in this redesign too. Moving the mapping into AppKeychain/SharedKeychain and deleting isRealKeychainFailure puts the SFHFKeychainUtils NSError → .notFound | .failure(OSStatus) decision in one place — which is exactly where we'd have to decide what -1999 ("item present, no password data") maps to. Today isRealKeychainFailure treats it as a real failure (code != errSecItemNotFound), so a -1999 on the private read rethrows instead of falling back to the shared group. Near-unreachable on the new private group (we only ever write items with password data there, and the legacy malformed form lives in the shared group), but the typed-error version should make that mapping a deliberate choice rather than a side effect of the errSecItemNotFound-only check.