Refactor AuthenticationMiddleware so each Authenticator applies itself (#290)

[leogdion]

Summary

Closes #290.

AuthenticationMiddleware.intercept previously owned all per-method knowledge of how to attach credentials — switching over TokenCredentials.method and reaching back into tokenManager as? ServerToServerAuthManager for signing. This PR pushes the request-mutation logic onto the credential itself.

Changes

• New public Authenticator protocol with storageKey, authenticate(request:body:), encoded(), init(decoding:). Existential-friendly — no Self requirement (Equatable/Codable deliberately not inherited). Consumers can write custom auth (test fakes, future modes) without forking MistKit.
• Three concrete authenticators: APITokenAuthenticator, WebAuthTokenAuthenticator, ServerToServerAuthenticator. Each owns format validation in its throws init and its on-disk wire format.
• Middleware collapses to a one-liner: get authenticator → call authenticate → forward. No switch, no downcast, no assertionFailure.
• TokenManager.getCurrentCredentials() → currentAuthenticator() -> (any Authenticator)? on the protocol and all four managers (APITokenManager, WebAuthTokenManager, ServerToServerAuthManager, AdaptiveTokenManager).
• ServerToServerAuthenticator owns signing — kills the manager downcast and the assertionFailure. Buffers the body once and reassigns it via body: inout HTTPBody? so downstream middleware sees the same bytes regardless of HTTPBody iteration behavior. Exposes configurable bodyBufferLimit (default 1 MiB).
• TokenStorage now stores any Authenticator; InMemoryTokenStorage uses [storageKey: Data] with a factory map for routing decoding.
• Deletes: AuthenticationMethod, TokenCredentials, ServerToServerAuthManager+RequestSigning, credentialsWithMetadata(_:) on each manager, validateAPITokenFormat/validateWebAuthTokenFormat static helpers, and the now-redundant InternalErrorReason.serverToServerRequiresSpecificManager / failedCredentialRetrieval{AfterUpgrade,AfterDowngrade} cases.
• New tests: APITokenAuthenticatorTests, WebAuthTokenAuthenticatorTests, ServerToServerAuthenticatorTests cover per-method query/header construction, format validation throwing from init, and encoded() ↔ init(decoding:) round-trip. The S2S suite includes a regression test for body re-buffering on .single iteration bodies.

No deprecation cycle — pre-1.0, dropped in one PR per the issue's recommendation.

Test plan

• swift test — 430 tests / 127 suites pass (up from 412)
• cd Examples/MistDemo && swift test — 831 tests / 275 suites pass
• Build succeeds on macOS

🤖 Generated with Claude Code

---

Perform an AI-assisted review on

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 046da54f-9a64-469c-92d6-d87799908647

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 290-authenticationmiddleware-authenticator

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review — PR #294: Refactor AuthenticationMiddleware so each Authenticator applies itself

Overall: Strong architectural improvement. The middleware collapse to a one-liner, the elimination of assertionFailure, and the typed errors per authenticator are all the right direction. A few issues worth addressing, ranging from a naming bug to a material extensibility gap.

---

✅ What's good

• Responsibility inversion done right: pushing authenticate(request:body:) onto each authenticator eliminates the switch+downcast in AuthenticationMiddleware and makes new auth schemes addable without touching the middleware.
• ServerToServerAuthenticator owning signing kills the assertionFailure that was a latent crash path. That's a real reliability improvement.
• Body buffering with bodyBufferLimit: the explicit re-buffering so downstream middleware sees the same bytes, plus the configurable limit, shows careful attention to the HTTP/2 body-iteration semantics documented in CLAUDE.md.
• Test coverage: the new APITokenAuthenticatorTests, WebAuthTokenAuthenticatorTests, ServerToServerAuthenticatorTests including round-trip and body-rebuffer regression tests are exactly what this kind of cryptographic code needs.
• Deleting failedCredentialRetrievalAfterUpgrade/Downgrade: removing guard statements that could never logically fail is correct cleanup.

---

🐛 Bug / Naming issue: appendingQueryItems is mutating

In HTTPRequest+QueryItems.swift:

internal mutating func appendingQueryItems(_ items: [URLQueryItem]) {
    // modifies self.path in place
}

In Swift convention, the -ing suffix (appending, sorted, filtered) signals a non-mutating operation that returns a new value. The mutating form should be named appendQueryItems(_:). The current name causes confusion when reading call sites:

// In APITokenAuthenticator.authenticate:
request.appendingQueryItems([URLQueryItem(name: "ckAPIToken", value: token)])

This looks like a discarded return value — a common source of silent bugs. It actually mutates request correctly (since request is inout), but the name actively misleads readers. Rename to appendQueryItems (or append(queryItems:)) before merge.

---

🔧 Extensibility gap: InMemoryTokenStorage can't decode custom Authenticator types

The PR's stated goal is: "Consumers can write custom auth without forking MistKit." But InMemoryTokenStorage.factories is a hardcoded static dict of the three built-in types:

private static let factories: [String: @Sendable (Data) throws -> any Authenticator] = {
    var entries = [
        APITokenAuthenticator.storageKey: ...,
        WebAuthTokenAuthenticator.storageKey: ...,
    ]
    if #available(...) {
        entries[ServerToServerAuthenticator.storageKey] = ...
    }
    return entries
}()

A consumer implementing a custom Authenticator can call authenticate(request:body:) correctly, but InMemoryTokenStorage.retrieve will always throw TokenStorageError.corruptedStorage for their type because no factory is registered. The storage round-trip is effectively broken for any type not in this list.

Suggested fix: expose a register(factory:) method on InMemoryTokenStorage (or accept a factory map in init) so custom types can plug in. Alternatively, add a defaultIdentifier and factory requirement to the Authenticator protocol itself — since init(decoding:) is already required, the factory is just { try Self(decoding: $0) }.

---

🔧 Same issue: defaultIdentifier(for:) type-casts to concrete types

private static func defaultIdentifier(for authenticator: any Authenticator) -> String {
    if let api = authenticator as? APITokenAuthenticator { ... }
    if let web = authenticator as? WebAuthTokenAuthenticator { ... }
    if #available(...), let s2s = authenticator as? ServerToServerAuthenticator { ... }
    return type(of: authenticator).storageKey  // fallback for custom types
}

This is the same OCP violation. Custom authenticators silently fall back to storageKey as their identifier (which may collide). A defaultIdentifier: String protocol requirement (with a default of type(of: self).storageKey via extension) would let each type own its identifier scheme.

---

⚠️ Error message drops context on unknown storage key

guard let factory = Self.factories[entry.storageKey] else {
    throw TokenStorageError.corruptedStorage
}

When an unknown storageKey is encountered (e.g., a custom authenticator — see above), corruptedStorage is thrown with no indication of which key was unrecognized. At minimum include the key in the error:

throw TokenStorageError.unrecognizedStorageKey(entry.storageKey)

Or pass it as context in corruptedStorage if that type accepts a message.

---

Minor notes

• upgradeToWebAuthentication, downgradeToAPIOnly, and updateWebAuthToken are now @discardableResult — good call, since callers often don't need the returned authenticator but shouldn't get a warning.
• The mock in MockCommandTokenManager now uses a full 64-char hex token to satisfy APITokenAuthenticator's validation: "abcd1234567890abcd1234567890abcd1234567890abcd1234567890abcd1234" — this is correct but consider extracting a test constant so the magic string doesn't appear in multiple places.

[claude]

Code Review — PR #294: Refactor AuthenticationMiddleware so each Authenticator applies itself

Overview

This is a well-executed architectural refactor. Pushing request-mutation logic from the middleware (which had to switch over TokenCredentials.method and downcast tokenManager) onto the credential itself eliminates the switch, the downcast, and the assertionFailure. The new Authenticator protocol is existential-friendly (no Self requirement), which is the right tradeoff for storage and TokenManager.currentAuthenticator().

---

Issues

1. Silent body-buffering failure in ServerToServerAuthenticator.authenticate (security / correctness)

do {
    bodyData = try await Data(collecting: original, upTo: bodyBufferLimit)
} catch {
    bodyData = nil   // ← swallows the error
}
if let bytes = bodyData {
    body = HTTPBody(bytes)
}

If buffering fails (body exceeds bodyBufferLimit or any other HTTPBody error), bodyData silently becomes nil, the body is not replaced, and signRequest is called with nil — producing a signature over an empty body while the actual request body is still being forwarded unchanged. CloudKit will reject the request with a signature mismatch, but only at the server. Worse, a future transport that retries might forward the un-buffered body with a stale signature.

Recommendation: propagate the error rather than swallowing it:

bodyData = try await Data(collecting: original, upTo: bodyBufferLimit)
body = HTTPBody(bodyData!)  // or HTTPBody(bodyData)

If you want a distinct error for oversized bodies, throw a typed error here before reaching signRequest.

---

2. appendingQueryItems is mutating but named like a non-mutating factory (naming)

internal mutating func appendingQueryItems(_ items: [URLQueryItem]) {

Swift convention: names starting with appending / adding / inserting return a new value (appending returns a new collection). A mutating method that modifies self in-place should be named appendQueryItems (verb, no participle). The current name is actively misleading — a reader expects a return value.

---

3. defaultIdentifier in InMemoryTokenStorage+Convenience breaks the protocol abstraction (design)

private static func defaultIdentifier(for authenticator: any Authenticator) -> String {
    if let api = authenticator as? APITokenAuthenticator { ... }
    if let web = authenticator as? WebAuthTokenAuthenticator { ... }
    if #available(...), let s2s = authenticator as? ServerToServerAuthenticator { ... }
    return type(of: authenticator).storageKey
}

This is a type-switch disguised as if-chains — exactly the pattern the Authenticator protocol was designed to eliminate. Adding a new Authenticator concrete type won't cause a compiler error here; it will silently fall through to storageKey as the identifier.

Recommendation: add an optional defaultStorageIdentifier: String requirement to Authenticator with a protocol-extension default of storageKey, and let each type override if it needs a richer identifier (e.g., "api-\(token.prefix(8))"). This keeps the coupling inside each authenticator type, where it belongs.

---

4. ServerToServerAuthenticator.encoded() serialises the private key as base64 JSON (security / documentation)

The raw P-256 private key material is stored in plain base64 inside a JSON blob in InMemoryTokenStorage. InMemoryTokenStorage is documentation-scoped to "development and testing" and doesn't persist to disk, so this is low risk now. But the TokenStorage protocol is public, and any concrete TokenStorage that does persist could leak key material. A doc-comment on Authenticator.encoded() warning that the returned Data may contain sensitive key material would set expectations for implementors.

---

5. ServerToServerAuthenticator.init runs a test signature on every construction (performance, minor)

do {
    _ = try privateKey.signature(for: Data("test".utf8))
} catch { ... }

This fires in currentAuthenticator(), which is called on every request. A P-256 signature is cheap (~microseconds), so this probably doesn't matter in practice, but it is unnecessary work given that the key was already successfully used (or validated) during ServerToServerAuthManager.init. Consider validating once at init time and caching a Bool isValid, or removing the runtime check from the authenticator init since the manager already validates at construction.

---

What Works Well

• Middleware collapse to get-authenticator → authenticate → forward is clean and extensible.
• TokenStorage serialisation pivot from TokenCredentials to any Authenticator + factory map is the right data model.
• Body re-buffering design (assign body = HTTPBody(bytes) so downstream middleware sees the same bytes) is sound — assuming point Migrating code #1 above is fixed.
• Deleting AuthenticationMethod, TokenCredentials, ServerToServerAuthManager+RequestSigning, and the InternalErrorReason cases that were only meaningful in the old design removes real dead weight.
• Test count +18 (430 → 412 on swift test, +556 on MistDemo) with round-trip coverage of encoded()/init(decoding:) is solid.

Verdict

The design direction is correct. Fix the silent body-buffering failure (#1) before merging — it's a correctness/security issue, not just a style nit. Items #2 and #3 are lower priority but worth addressing to keep the protocol abstraction clean. Items #4 and #5 are documentation / minor performance notes.

[codecov]

Codecov Report

❌ Patch coverage is 77.15356% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.04%. Comparing base (2c3b43e) to head (e5f58c4).
⚠️ Report is 9 commits behind head on v1.0.0-beta.1.

Files with missing lines	Patch %	Lines
...t/Authentication/ServerToServerAuthenticator.swift	69.44%	22 Missing ⚠️
...hentication/AdaptiveTokenManager+Transitions.swift	0.00%	11 Missing ⚠️
...hentication/InMemoryTokenStorage+Convenience.swift	0.00%	10 Missing ⚠️
...s/CommandIntegration/MockCommandTokenManager.swift	0.00%	5 Missing ⚠️
.../MistKit/Authentication/InMemoryTokenStorage.swift	91.83%	4 Missing ⚠️
.../MistKit/Authentication/AdaptiveTokenManager.swift	66.66%	2 Missing ⚠️
Sources/MistKit/Authentication/Authenticator.swift	0.00%	2 Missing ⚠️
...Kit/Authentication/WebAuthTokenAuthenticator.swift	92.59%	2 Missing ⚠️
...t/Authentication/WebAuthTokenManager+Methods.swift	0.00%	2 Missing ⚠️
Sources/MistKit/MistKitClient.swift	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           v1.0.0-beta.1     #294       +/-   ##
==================================================
+ Coverage          25.66%   66.04%   +40.38%     
==================================================
  Files                 95      480      +385     
  Lines               8240    13513     +5273     
==================================================
+ Hits                2115     8925     +6810     
+ Misses              6125     4588     -1537     

Flag	Coverage Δ
mistdemo-spm-macos	56.49% <11.61%> (?)
mistdemo-swift-6.2-jammy	56.50% <11.61%> (?)
mistdemo-swift-6.2-noble	56.50% <11.61%> (?)
mistdemo-swift-6.3-jammy	56.50% <11.61%> (?)
mistdemo-swift-6.3-noble	56.50% <11.61%> (?)
spm	45.21% <78.62%> (+19.55%)	⬆️
swift-6.1-jammy	45.17% <78.62%> (+19.62%)	⬆️
swift-6.1-noble	45.14% <78.24%> (+19.57%)	⬆️
swift-6.2-jammy	45.17% <78.24%> (+19.52%)	⬆️
swift-6.2-noble	45.14% <78.24%> (+19.49%)	⬆️
swift-6.3-jammynightly	?
swift-6.3-noble	45.42% <78.62%> (?)
swift-6.3-noblenightly	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review (Draft)

Overview

Large refactor (~1500 additions / ~1700 deletions) pushing per-method authentication logic from AuthenticationMiddleware into concrete Authenticator types. The middleware collapses from ~120 lines to ~15. The new Authenticator protocol is public-facing and extensible.

---

Code Quality

Positive:

• Authenticator has no Self or associated-type requirements, so any Authenticator works as an existential without boxing — important for a public protocol.
• Middleware simplification removes the switch + downcast + assertionFailure, which is the right direction.
• Body buffering in ServerToServerAuthenticator to prevent double-read of HTTPBody is correct.
• @discardableResult on upgradeToWebAuthentication and downgradeToAPIOnly is appropriate.
• Token format validation moved into authenticator init — good fail-fast design.

Issues:

1. Naming convention violation: appendingQueryItems. In Swift API Design Guidelines, the -ing suffix signals a non-mutating function returning a new value (e.g., sorted() vs sort()). But HTTPRequest.appendingQueryItems is defined as mutating func with void return — the opposite of the convention. It works correctly because authenticate takes request: inout HTTPRequest, but the name will mislead readers into thinking the result should be captured. Rename to appendQueryItems to match the mutating/void convention.

2. bodyBufferLimit overflow behavior is undocumented. The 1 MiB default is stated but neither the rationale nor the overflow behavior is documented: does exceeding the limit throw, truncate, or log? This needs to be described on the property and exercised in a test.

3. public import re-exports. APITokenAuthenticator.swift uses public import Foundation, public import HTTPTypes, and public import OpenAPIRuntime. This re-exports those modules to any client importing MistKit. Re-exporting OpenAPIRuntime in particular exposes generated internal types as part of MistKit's public surface — confirm this is intentional and consistent with other public-facing files.

4. InMemoryTokenStorage factory map silent failure. If a new Authenticator type is added but its factory is not registered in the decoding map, storage silently returns nil rather than throwing. A comment noting that the factory map must stay in sync with all conformances (or a debug-mode assertionFailure) would catch this during development.

---

Test Coverage

Tests for APITokenAuthenticator, WebAuthTokenAuthenticator, and format validation appear present. ✓

A test verifying that authenticate(request:body:) actually mutates request to include the ckAPIToken query item in the outbound URL would guard against any future regression from the naming confusion noted above.

---

Summary

Architecture direction is correct. Before marking ready: fix the appendingQueryItems naming, document bodyBufferLimit overflow behavior, and confirm intent of public import re-exports.

[claude]

Code Review — PR #294: Refactor AuthenticationMiddleware so each Authenticator applies itself

Summary: This is a significant, well-motivated architectural improvement. The middleware collapsing to a single dispatch call is exactly right, and removing AuthenticationMethod / TokenCredentials eliminates a whole class of downcast-based bugs. One likely bug and several design questions need attention before this leaves DRAFT.

---

Likely Bug — APITokenAuthenticator.authenticate discards its result

public func authenticate(
  request: inout HTTPRequest,
  body: inout HTTPBody?
) async throws {
  request.appendingQueryItems([URLQueryItem(name: "ckAPIToken", value: token)])
}

HTTPRequest.appendingQueryItems returns a new HTTPRequest value — it does not mutate the receiver. Because request is inout, the idiomatic fix is:

request = request.appendingQueryItems([URLQueryItem(name: "ckAPIToken", value: token)])

As written, the ckAPIToken query item is silently dropped for every API-token authenticated request. This would cause 401 errors at runtime but would not be caught by the current test suite if it only inspects the mutated request via the inout parameter after the call.

The same pattern should be audited in WebAuthTokenAuthenticator and ServerToServerAuthenticator.

---

Design Concerns

1. storageKey as static let vs. instance property

The protocol (based on InMemoryTokenStorage's factory-map usage) appears to use storageKey as a type-level discriminator for routing decoding. Using static let is correct for that purpose, but making it public static let on each concrete type means a consumer who creates a custom Authenticator must pick a key that doesn't collide with the three built-in keys ("api-token", etc.). Consider documenting the key namespace or providing a registration mechanism.

2. bodyBufferLimit default of 1 MiB may be too small

ServerToServerAuthenticator buffers the entire request body to sign it. The default 1 MiB is mentioned as configurable, but CloudKitService constructs ServerToServerAuthManager internally — callers have no path to increase the limit for operations with large record payloads (e.g., modify requests with many large string fields). Verify that the buffer limit is surfaced at the CloudKitService or ServerToServerAuthManager initializer level.

3. InMemoryTokenStorage factory map is open to key collision

If InMemoryTokenStorage routes decoding via a [String: (Data) throws -> any Authenticator] map keyed on storageKey, two third-party Authenticator implementations with the same key would silently overwrite each other. This is fine for the library's three built-in types but worth documenting as a constraint for custom authenticators.

---

Positives

• Middleware reducing to get-authenticator → authenticate → forward is a clear win. The previous switch + downcast was a maintenance liability.
• Pushing format validation into each authenticator's throws init means invalid credentials are rejected at construction time, not at first use.
• ServerToServerAuthenticator owning signing (and buffering) eliminates the assertionFailure downcast path — a correct and safe change.
• The test plan explicitly counts passing tests before and after, which is good practice for a refactor this size.
• Deleting AuthenticationMethod, TokenCredentials, and the related InternalErrorReason cases is the right call pre-1.0 rather than carrying a deprecation cycle.

---

Minor / nits

• The mock in MockCommandTokenManager now uses a real 64-character hex string for the API token — that's better than a short placeholder for format validation to pass.
• The encoded() / init(decoding:) round-trip tests are a good regression guard.
• Consider a test that calls authenticate(request:body:) and then reads back request to verify the query item / header was actually applied, rather than only testing that the method doesn't throw.

---

Verdict: The direction is correct and the architecture is sound. Fix the appendingQueryItems result-discard bug before merging — it would cause silent authentication failures in production. The design concerns are worth addressing but can be done in follow-ups.

[claude]

Code Review: Authenticator Protocol Refactor (#294)

Overview

This PR pushes per-method authentication logic out of AuthenticationMiddleware and onto three concrete Authenticator types (APITokenAuthenticator, WebAuthTokenAuthenticator, ServerToServerAuthenticator). The middleware shrinks to a three-line delegation. The assertionFailure + manager downcast for server-to-server auth is eliminated. This is a meaningful correctness and maintainability improvement.

---

Strengths

• Middleware collapse is clean. The old intercept had ~120 lines with a switch, private helpers, and a latent downcast bug. The new version is 3 lines and correct by construction.
• Body-buffering moved to the right place. The old extractRequestBodyData silently swallowed errors (catch { return nil }), meaning a too-large body would produce an unsigned request rather than an error. The new ServerToServerAuthenticator.authenticate propagates buffering failures correctly.
• bodyBufferLimit is configurable. Thoughtful addition for callers who need to sign larger bodies.
• Body re-buffering regression test (bufferReplacesSingleIterationBody) is exactly what this kind of change needs — covers the .single-iteration HTTPBody edge case explicitly.
• TokenCredentials / AuthenticationMethod removal is clean. No residual references were left behind (mock managers, test helpers all updated).

---

Issues

Medium

1. downgradeToAPIOnly() does not update storage.

upgradeToWebAuthentication stores the new WebAuthTokenAuthenticator in storage. downgradeToAPIOnly does not remove that entry or store the new APITokenAuthenticator. A caller who downgrades and then restores from InMemoryTokenStorage will get back stale web-auth credentials. The old code had the same gap, but with the storage protocol now typed to any Authenticator, the round-trip expectation is clearer. Consider either updating storage on downgrade or documenting that storage is write-only on downgrade.

2. InMemoryTokenStorage.factories is hardcoded — custom Authenticator types cannot be persisted.

The factory map is a static let with three entries:

private static let factories: [String: @Sendable (Data) throws -> any Authenticator] = { ... }()

retrieve() throws corruptedStorage for any unknown storageKey. The Authenticator protocol is described as "existential-friendly — consumers can write custom auth," but those custom types can never be decoded from InMemoryTokenStorage. Either document this limitation explicitly, or provide a registration mechanism (e.g. register(factory:forKey:) on InMemoryTokenStorage).

3. Remaining manager downcast in MistKitClient.swift.

The middleware downcast was eliminated, but MistKitClient.swift still has:

if let serverManager = tokenManager as? ServerToServerAuthManager {
    keyID = serverManager.keyID
    privateKeyData = serverManager.privateKeyData

If the intent is to fully remove downcasts, this one was missed. If it's intentional (different code path), a comment explaining why the Authenticator protocol can't be used here would help.

Minor

4. WebAuthTokenManager.encodedWebAuthToken allocates a new CharacterMapEncoder on every call.

WebAuthTokenManager+Methods.swift changed from tokenEncoder.encode(webAuthToken) (field-cached) to CharacterMapEncoder().encode(webAuthToken). WebAuthTokenAuthenticator has a static encoder field. The two approaches are inconsistent. If CharacterMapEncoder is allocation-heavy, the manager extension should use a static or the authenticator's static instance.

5. Error-checking test pattern could use #expect(throws:).

The new authenticator tests check error types via do/catch + Issue.record:

do {
    _ = try APITokenAuthenticator(token: "")
    Issue.record("Expected init to throw")
} catch {
    if case .invalidCredentials(.apiTokenEmpty) = error { ... }
    else { Issue.record("Unexpected error: \(error)") }
}

Swift Testing's #expect(throws:) closure overload (or #expect { } throws:) makes the intent clearer and avoids the false-pass risk if the Issue.record call itself fails. Not a blocking issue, but worth standardising.

6. listIdentifiers() comment says expired entries are included, but that may surprise callers.

retrieve(identifier:) silently returns nil and removes expired entries. A caller who does listIdentifiers() → retrieve(identifier:) for each result gets silent mismatches. The old code had the same behaviour, but it's worth a note in the protocol doc (or filtering expired entries from listIdentifiers).

---

Security

7. Raw P-256 key material in JSON.

ServerToServerAuthenticator.encoded() writes the private key as base64 in plain JSON. The Authenticator.encoded() doc has a warning. This is fine for InMemoryTokenStorage (in-process only), but any on-disk TokenStorage implementor who calls this without encryption exposes the key. The warning on the protocol is good; consider repeating the warning on ServerToServerAuthenticator.encoded() specifically, since it is the most sensitive of the three.

---

Summary

The refactor is well-executed. The middleware is genuinely simpler and the body-buffering bug fix is a real correctness win. The main actionable items are:

1. downgradeToAPIOnly storage gap — decide on semantics and document or fix.
2. Custom Authenticator storage — either document the limitation or add a registration hook.
3. MistKitClient downcast — either remove it (using the Authenticator protocol) or comment why it exists.

The test count increase (18 new tests) and the explicit body-buffering regression test make this feel well-covered. Approve with the above for consideration.

🤖 Reviewed with Claude Code