Improve error handling: typed TokenManagerError and safe RecordOperation conversion

[leogdion]

Summary

• Introduce AuthenticationFailedReason and NetworkErrorReason enums to replace untyped any Error in TokenManagerError cases, enabling pattern matching without as? casting
• Replace fatalError in RecordOperation-to-OpenAPI conversion with a throwing initializer using CloudKitError.unsupportedOperationType
• Update all test mocks to use the new structured error types

Test plan

• swift build compiles cleanly
• swift test — all tests pass
• Scripts/lint.sh — no new violations
• Verify AuthenticationFailedReason and NetworkErrorReason cases cover all mock usage patterns
• Verify RecordOperation conversion throws instead of crashing for unknown types

Closes #299, closes #300, resolves #301

🤖 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: 171dcc0b-6869-44bf-8127-2f46aad0edb8

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 301-improve-error-handling-typed-errors-safe-conversion

---

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 #305 — Typed TokenManagerError and Safe RecordOperation Conversion

Overview

This PR replaces untyped any Error in TokenManagerError cases with structured AuthenticationFailedReason / NetworkErrorReason enums, and converts the fatalError in RecordOperation initialization to a proper typed throw. Both changes are clear improvements for correctness and ergonomics.

---

What's Good

• Removing fatalError in favor of throws(CloudKitError) is the right call — crashing on an unknown operation type is unacceptable in a library. The new unsupportedOperationType case cleanly maps into the existing error hierarchy.
• Structured error reasons (AuthenticationFailedReason, NetworkErrorReason) enable exhaustive switch matching on associated values, which was the stated goal.
• Mock simplification is a genuine improvement — removing NSError(domain:code:userInfo:) boilerplate makes intent clearer.
• RecordOperationConversionTests provides solid parameterized coverage of all valid operation types.

---

Issues

1. Unsound Sendable Conformance (any Error in associated values)

AuthenticationFailedReason and NetworkErrorReason both declare Sendable, but carry any Error in some cases:

case tokenGenerationFailed(any Error)  // AuthenticationFailedReason
case signingFailed(any Error)          // AuthenticationFailedReason
case other(any Error)                  // NetworkErrorReason

any Error is not Sendable, so these conformances are technically unsound under strict concurrency. The fix is to use any Error & Sendable:

case tokenGenerationFailed(any Error & Sendable)
case signingFailed(any Error & Sendable)
case other(any Error & Sendable)

This is a correctness issue that will surface as a warning or error under StrictConcurrency checking.

2. Semantic drift in mock network errors

Several mocks previously threw networkError(underlying: NSError(domain: "ConnectionError", code: -1_004, ...)) (which is NSURLErrorCannotConnectToHost) and now throw .notConnectedToInternet (which maps to -1009, NSURLErrorNotConnectedToInternet). These are meaningfully different network conditions. The tests in SimulationTests.swift now check that reason.description.contains("internet"), which passes, but it's worth verifying the semantic intent is preserved for MockTokenManagerWithConnectionError specifically — "cannot connect to host" ≠ "not connected to internet."

3. description property vs CustomStringConvertible

Both new types expose a description: String computed property but don't conform to CustomStringConvertible. This means "\(authFailedReason)" will not use the custom description. Either conform to CustomStringConvertible or rename the property to localizedDescription to align with LocalizedError patterns already used in the codebase.

4. Missing test for the error path in RecordOperationConversionTests

The new test suite covers all valid operation types, but never exercises the failure path. Add a test that constructs a mock RecordOperation.OperationType with an unmapped raw value and asserts unsupportedOperationType is thrown. (This may require using @testable or a raw-value trick depending on how OperationType is defined.)

---

Minor

• CloudKitError.errorDescription now has an unreachable default-less switch again — confirm the unsupportedOperationType case is included in the httpStatusCode computed property's switch (it is, and the PR handles this correctly, just double-check the exhaustiveness stays maintained as new cases are added).

---

Overall this is a solid improvement. The Sendable issue with any Error associated values is the most important thing to address before merging.

🤖 Generated with Claude Code

[claude]

Code Review — PR #305: Typed TokenManagerError and Safe RecordOperation Conversion

Overall this is a solid improvement — replacing fatalError and untyped any Error with structured enums is the right direction. A few things worth addressing before merging.

---

Breaking API change without deprecation

TokenManagerError.authenticationFailed and .networkError both changed their associated value signatures (from labeled underlying: parameters to unlabeled enum values). Any caller that pattern-matches these cases in downstream code will fail to compile.

Since this is a public enum, consider whether a deprecation shim is warranted or whether the project's current stability level makes a hard cut acceptable. At minimum this should be called out in the PR description as a breaking change.

---

AuthenticationFailedReason and NetworkErrorReason cannot be Equatable

Both enums carry any Error associated values, which blocks automatic Equatable synthesis. This limits test assertions — callers can't write #expect(reason == .timeout) and must fall back to if case pattern matching. The tests in this PR work around it, but it's worth documenting intentionally or designing around (e.g., making the any Error cases use typed errors).

---

description vs CustomStringConvertible

Both new types expose a description: String property but don't conform to CustomStringConvertible. This means "\(reason)" in string interpolation won't use these descriptions — it'll use the default enum representation. Either:

• Add CustomStringConvertible conformance, or
• Rename to something like errorMessage to avoid the false appearance of conforming to the protocol.

---

Potential fidelity loss in mocks

Several mocks that previously simulated specific NSError codes (e.g., -1_004 for "connection failed", -1_009 for "not connected") now all map to .notConnectedToInternet. The test intent is preserved, but if any test was validating the specific error code (not just the case), that signal is silently lost.

MockTokenManagerWithIntermittentFailures previously used code -1_001 (timeout) and now correctly uses .timeout — that's fine. But verifying the mapping is intentional for each mock is worthwhile.

---

internal import Foundation in AuthenticationFailedReason.swift

Foundation is imported but none of its types are used in the file (only any Error, which is stdlib). This import can be removed.

---

Missing test: unknown operation type should throw

RecordOperationConversionTests tests all valid operation types, which is great. However, there's no test verifying that an unknown/invalid operation type actually throws CloudKitError.unsupportedOperationType. This is the core behavior this PR protects against.

@Test("Unknown operation type throws unsupportedOperationType")
func unknownOperationTypeThrows() throws {
    // Construct a RecordOperation with a raw value that's not in the mapping
    // ...
    #expect(throws: CloudKitError.unsupportedOperationType("...")) { ... }
}

Without this, the coverage of the throw path depends on implementation details of the mapping dict rather than being explicitly tested.

---

Minor: CloudKitError.unsupportedOperationType missing from errorDescription

The httpStatusCode switch in CloudKitError correctly handles the new case. The errorDescription also correctly handles it. Looks good.

---

Summary: The fatalError → throwing initializer change is the most impactful and highest-quality part of this PR. The typed error enums are a clear improvement but have a few rough edges (Equatable, CustomStringConvertible, missing throw test) worth addressing before this graduates from draft.

[claude]

CODE REVIEW - PR 305: Typed TokenManagerError and Safe RecordOperation Conversion

OVERVIEW
Replaces untyped 'any Error' in TokenManagerError cases with proper enums (AuthenticationFailedReason and NetworkErrorReason), and replaces a fatalError in RecordOperation-to-OpenAPI conversion with a throwing initializer. These are good improvements. A few issues with API design and semantic accuracy worth addressing.

---

ISSUES

1. Breaking API Change - Not Called Out

Changing .authenticationFailed(underlying: (any Error)?) to .authenticationFailed(AuthenticationFailedReason) and .networkError(underlying: any Error) to .networkError(NetworkErrorReason) breaks all call sites that pattern-match on these cases. This is fine for a pre-1.0 library, but the PR description and any release notes should call it out explicitly as a source-breaking change.

2. public import Foundation in NetworkErrorReason.swift

The file uses 'public import Foundation' rather than 'import Foundation'. In Swift, the 'public' modifier on an import re-exports the module - every consumer of MistKit would automatically import Foundation. This is almost certainly unintentional. Change to 'import Foundation'.

3. Missing CustomStringConvertible Conformance

Both AuthenticationFailedReason and NetworkErrorReason have a 'description: String' computed property but do not declare conformance to CustomStringConvertible. Without the protocol conformance, String(describing: reason) returns the enum case name, not the description. Add ': CustomStringConvertible' to both types.

4. Mock Semantic Changes Lose Precision

Several mocks are changed to .notConnectedToInternet that were previously using NSError with code -1004 (connection refused) or -1001 (timed out). These are different failure modes:

• MockTokenManagerWithConnectionError previously simulated server-reachable/connection-refused (-1004), now simulates no-network (-1009). These are very different for debugging.
• MockTokenManagerWithIntermittentFailures previously used -1001 (timeout), now uses .timeout, which is accurate.

Consider using .connectionLost for MockTokenManagerWithConnectionError to preserve the 'server unreachable' semantic.

5. unsupportedOperationType Missing from Equatable

CloudKitError does not currently conform to Equatable (likely due to associated values containing 'any Error'). The new .unsupportedOperationType(String) case could easily be equatable, and tests for it currently require pattern matching instead of simple equality checks. Not a blocker, but worth noting.

6. AuthenticationFailedReason Cases Without Consumers

The tokenGenerationFailed and signingFailed cases are added to the public API but have no production callers and no tests exercising their descriptions. This is forward-looking design, which is fine, but the description strings have no test coverage.

---

MINOR NITS

• The test assertion change from 'contains("Timeout")' to 'contains("timed out")' is correct for the new enum, but consider using exact equality checks on known description strings rather than contains() to make test failures more obvious.

---

POSITIVES

• fatalError -> typed throw is a meaningful safety improvement; unknown operation types now produce recoverable errors
• Named enum cases replace opaque NSError construction in all mocks - much cleaner
• AuthenticationFailedReason.serverRejected(statusCode:message:) carries useful structured data
• The pattern is consistent with existing InternalErrorReason and InvalidCredentialReason enums
• Test cleanup (removing verbose NSError constructions) is a real improvement in readability

[claude]

Code Review — PR #305: Typed TokenManagerError and Safe RecordOperation Conversion

Overview

This PR eliminates two pain points: fatalError in library conversion code and untyped any Error payloads in TokenManagerError. Both are solid improvements. The changes are well-scoped and the test mock cleanup is a nice side effect.

---

✅ Strengths

• fatalError → typed throw — Replacing fatalError("Unknown operation type: ...") with throw CloudKitError.unsupportedOperationType(...) is the most important change here. Crashing in a library is never acceptable; callers can now handle or propagate the error.
• Pattern matching — Typed reason enums let callers switch on specific failure modes without as? casting.
• Sendable intent is present — Both enums are marked Sendable and the mock simplifications are noticeably cleaner.
• throws(CloudKitError) typed throw — Correctly uses Swift's typed throws so callers don't need to catch untyped errors.

---

⚠️ Issues to Address

1. Sendable conformance is incorrect — will break under strict concurrency

AuthenticationFailedReason and NetworkErrorReason both contain any Error associated values and are declared Sendable. any Error does not conform to Sendable, so this will produce warnings (and errors in strict mode):

// AuthenticationFailedReason.swift
case tokenGenerationFailed(any Error)  // ❌ any Error is not Sendable
case signingFailed(any Error)          // ❌

// NetworkErrorReason.swift
case other(any Error)                  // ❌

Fix: use any Error & Sendable for these associated values, or box the errors in a @unchecked Sendable wrapper type.

2. Semantic mismatch in mock updates

Several mocks that previously used NSError code -1_004 ("Could not connect to the server") are now mapped to .notConnectedToInternet (which semantically maps to -1_009). These are distinct conditions. While this doesn't affect test pass/fail here, it could mislead future readers about which error scenarios the mocks represent.

Similarly, MockTokenManagerWithConnectionError was simulating a connection-refused error (-1_004), not "no internet" (-1_009). Consider adding a .connectionRefused case to NetworkErrorReason or using .connectionLost for that mock.

3. authenticationFailed case is now non-optional — breaking change

The old signature was:

case authenticationFailed(underlying: (any Error)?)

The new one requires a specific AuthenticationFailedReason. Any downstream consumer who was catching .authenticationFailed(nil) for "unknown auth failure" no longer has a representation for that state. The mocks replace it with .serverRejected(statusCode: 401, message: nil) which imposes meaning that wasn't there before. Consider adding a .unknown case to AuthenticationFailedReason to preserve that semantic.

4. Neither new enum conforms to Equatable

Without Equatable, you can't write:

#expect(reason == .timeout)

Tests currently use reason.description.contains(...) which is fragile string matching. Making the enums Equatable (where possible) would make tests more precise and readable.

Note: .other(any Error) and .urlError(URLError) make auto-synthesis harder, but you can implement Equatable manually or use @unchecked conformance for test purposes.

5. Missing test for the unsupportedOperationType error path

RecordOperationConversionTests tests all valid operation types but there's no test verifying that an unknown/future operation type actually throws .unsupportedOperationType. If RecordOperation.OperationType is a closed enum, constructing an invalid one requires @testable access to raw values or a stub. Worth adding even if it's only possible with a future extensibility mechanism.

---

Minor Notes

• BatchSyncResultTests.swift formatting change (wrapping the #expect expression) is unrelated to this PR.
• CloudKitError errorDescription switch is exhaustive after adding unsupportedOperationType — good.

---

Summary

The fatalError elimination is correct and necessary. The Sendable issue on the error reason enums should be resolved before merging to avoid future strict-concurrency breakage. The semantic mismatch in a few mock updates is low-risk but worth cleaning up for accuracy.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 80.39216% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.38%. Comparing base (394c0e1) to head (5fc9337).

Files with missing lines	Patch %	Lines
...it/Authentication/AuthenticationFailedReason.swift	70.00%	3 Missing ⚠️
...es/MistKit/Authentication/NetworkErrorReason.swift	83.33%	2 Missing ⚠️
Sources/MistKit/Service/CloudKitError.swift	50.00%	2 Missing ⚠️
...tKit/Service/CloudKitService+WriteOperations.swift	0.00%	2 Missing ⚠️
...I/Components.Schemas.RecordOperation+MistKit.swift	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           v1.0.0-beta.1     #305      +/-   ##
=================================================
- Coverage          66.40%   66.38%   -0.03%     
=================================================
  Files                484      486       +2     
  Lines              13590    13623      +33     
=================================================
+ Hits                9024     9043      +19     
- Misses              4566     4580      +14     

Flag	Coverage Δ
mistdemo-spm-macos	56.20% <37.25%> (-0.08%)	⬇️
mistdemo-swift-6.2-jammy	56.21% <37.25%> (-0.05%)	⬇️
mistdemo-swift-6.2-noble	56.21% <37.25%> (-0.13%)	⬇️
mistdemo-swift-6.3-jammy	56.19% <37.25%> (-0.10%)	⬇️
mistdemo-swift-6.3-noble	56.19% <37.25%> (-0.08%)	⬇️
spm	47.08% <64.70%> (+0.67%)	⬆️
swift-6.1-jammy	47.08% <64.70%> (+0.61%)	⬆️
swift-6.1-noble	47.08% <64.70%> (+0.67%)	⬆️
swift-6.2-jammy	47.08% <64.70%> (-0.03%)	⬇️
swift-6.2-noble	47.08% <64.70%> (+0.67%)	⬆️
swift-6.3-jammy	47.35% <64.70%> (+0.98%)	⬆️
swift-6.3-noble	47.35% <64.70%> (+0.89%)	⬆️

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 — PR #305: Typed TokenManagerError and safe RecordOperation conversion

Overview

This PR makes two meaningful improvements: replacing untyped any Error payloads in TokenManagerError with structured reason enums, and replacing a fatalError in RecordOperation conversion with a typed throw. Both directions are correct. The implementation is mostly clean, with a few issues worth addressing before merging.

---

Issues

1. Sendable conformance with any Error associated values — potential Swift 6 violation

AuthenticationFailedReason is declared public enum AuthenticationFailedReason: Sendable but two of its cases carry any Error:

case tokenGenerationFailed(any Error)
case signingFailed(any Error)

Error does not imply Sendable, so any Error is not Sendable. Under Swift 6's strict concurrency checking this will be a compiler error. The same issue exists in NetworkErrorReason.other(any Error).

Fix options:

• Use any Error & Sendable as the associated value type
• Or use @unchecked Sendable on the enum (weaker, but pragmatic if existing callers pass non-sendable errors)

Note: TokenManagerError.networkError(underlying: any Error) already existed pre-PR, so if the project is already compiling without warnings, there may be a project-level @preconcurrency suppression — but the new enums still introduce the same risk surface.

2. New enums are missing CustomStringConvertible conformance

Both enums have a description computed property, but neither conforms to CustomStringConvertible. This means "\(reason)" in string interpolation will use Swift's default reflection output, not the human-readable description. Since TokenManagerError.errorDescription calls reason.description explicitly, the error messages themselves are correct — but callers who interpolate a NetworkErrorReason directly will get the wrong output.

// Add to both enums:
public enum AuthenticationFailedReason: CustomStringConvertible, Sendable { ... }
public enum NetworkErrorReason: CustomStringConvertible, Sendable { ... }

3. Test assertions use fragile string matching instead of pattern matching

In SimulationTests.swift and RecoveryTests.swift:

#expect(reason.description.contains("timed out"))
#expect(reason.description.contains("internet"))

These break silently if description text changes and don't communicate intent clearly. Since reason is already the typed NetworkErrorReason, pattern matching is both safer and more expressive:

#expect(reason == .timeout)
#expect(reason == .notConnectedToInternet)

This requires NetworkErrorReason: Equatable. The urlError and other cases would need custom == implementations, but timeout, connectionLost, and notConnectedToInternet are trivially equatable.

4. Missing test for the unsupportedOperationType error path

RecordOperationConversionTests covers all valid operation types but has no test for the new error path. Since RecordOperation.OperationType is a closed enum, reaching the guard failure requires injecting an operation constructed via raw value or some other backdoor. If the type is fully exhaustive, the error case is dead code — worth either:

• Adding a comment noting the guard is a defensive assertion (valid approach)
• Or verifying whether OperationType has a raw value that could produce an unmapped case

5. NetworkErrorReason placed in the Authentication/ directory

NetworkErrorReason represents network connectivity issues broadly — it's not authentication-specific. Placing it in Sources/MistKit/Authentication/ will cause confusion when it's used in non-auth contexts. Consider Sources/MistKit/Errors/ or Sources/MistKit/Core/.

---

Observations / Minor Points

urlError case overlaps with specific cases. timeout, connectionLost, and notConnectedToInternet can all be represented as URLError codes — and urlError(URLError) can represent any of them. This creates an ambiguity: callers converting a real URLError must decide whether to match on the specific cases or always use .urlError. Consider documenting the intended precedence, or using a factory method to canonicalize URLError → NetworkErrorReason.

Semantic shift in authenticationFailed: The old .authenticationFailed(underlying: nil) case expressed "failure with no known cause." All mocks replace it with .serverRejected(statusCode: 401, message: nil), implying an HTTP response was received. For mocks this is fine, but the original nil case was a valid real-world scenario (e.g., token generation failed before any network call). The new .tokenGenerationFailed and .signingFailed cases on AuthenticationFailedReason address this — but they're unused in the codebase. Worth verifying these cases are wired up in production token managers, not just declared.

guard #available in tests: The RecordOperationConversionTests file uses guard #available(macOS 11.0, ...) with Issue.record and early return. Swift Testing's withKnownIssue or #require(#available(...)) would be cleaner, but this works.

BatchSyncResultTests formatting change is unrelated to the PR's purpose — consider keeping formatting-only changes out of functional PRs.

---

Summary

The core design decisions here are sound — typed reason enums improve pattern matching and remove the need for as? casts; replacing fatalError with a throw eliminates a crash vector. The Sendable issue (#1) is the most important to resolve before merging. The missing CustomStringConvertible conformance (#2) and fragile test assertions (#3) are straightforward fixes.

🤖 Generated with Claude Code

[claude]

Code Review — PR #305: Improve error handling: typed errors and safe RecordOperation conversion

Overview

This PR makes two meaningful improvements:

1. Replaces any Error existentials in TokenManagerError with structured AuthenticationFailedReason / NetworkErrorReason enums enabling exhaustive pattern matching.
2. Eliminates fatalError in RecordOperation → OpenAPI conversion, replacing it with a typed throws(CloudKitError) initializer.

Both changes are in the right direction. The implementation is clean and the test coverage is solid. A few issues are worth addressing before merge.

---

Issues

🔴 AuthenticationFailedReason and NetworkErrorReason don't conform to CustomStringConvertible

Both types define a description: String property, but without CustomStringConvertible conformance, string interpolation ("\(reason)") uses the compiler-generated representation (e.g. serverRejected(statusCode: 401, message: nil)) rather than the human-readable .description. This defeats the purpose of the property.

// Fix:
public enum AuthenticationFailedReason: Sendable, CustomStringConvertible { ... }
public enum NetworkErrorReason: Sendable, CustomStringConvertible { ... }

🔴 withKnownIssue wrapping hides permanently-passing tests

throwsOnTimeout() and returnsAsyncValue() are now wrapped in withKnownIssue(isIntermittent: true) with no issueMatcher. This means these tests will silently pass forever even after the underlying flakiness is fixed — the framework will never report "this was expected to fail but didn't." Consider adding an issueMatcher closure so Swift Testing can flag when the issue resolves.

🟡 Missing URLError → NetworkErrorReason factory

NetworkErrorReason exposes structured cases (.timeout, .connectionLost, .notConnectedToInternet, .urlError(URLError)), but there's no helper to map from a raw URLError. Production code hitting the real network will land in .urlError(error) or .other(error) rather than the semantic cases, since the mapping has to be done manually. A static func from(_ error: URLError) -> NetworkErrorReason factory would complete the design and make future callers do the right thing by default.

🟡 MockTokenManagerWithConnectionError semantic shift

The original mock used domain ConnectionError / code -1004 (connection refused / kCFURLErrorCannotConnectToHost), which is now silently replaced with .notConnectedToInternet. These are semantically different conditions — one means the server rejected the TCP connection, the other means there's no network interface at all. The tests that depend on this mock likely aren't testing for this distinction today, but it's worth a comment or a rename if the intent is "device offline" not "server unreachable."

🟡 RecordOperationConversionTests availability guard pattern

guard #available(macOS 11.0, iOS 14.0, ...) else {
    Issue.record("...")
    return
}

Issue.record records a test failure, not a skip. If these tests run on older simulators in CI they'll record a failure on every parametrized argument and then silently return. Use .disabled() trait on the @Test / @Suite, or withKnownIssue if the intent is "this is an acceptable limitation", or remove the guard if the minimum deployment target already covers these versions.

🟡 Source-breaking addition to public enum CloudKitError

Adding .unsupportedOperationType to a public enum is a source-breaking change for any caller with an exhaustive switch. Given the package appears to be pre-1.0 this is likely acceptable, but worth noting in the changelog / release notes.

🟢 Two test-plan items unchecked

- [ ] Verify AuthenticationFailedReason and NetworkErrorReason cases cover all mock usage patterns
- [ ] Verify RecordOperation conversion throws instead of crashing for unknown types

The new RecordOperationConversionTests covers most of the second item, but there's no test for the unknown operation type path (the guard that now throws instead of fatalError). Add a test with a custom RecordOperation.OperationType value not in the mapping dictionary to prove the throw path is exercised.

---

Positives

• Eliminating fatalError in an initializer called from production modifyRecords code is a clear win — crashes in async network paths are hard to diagnose.
• The throws(CloudKitError) typed-throw is consistent with the rest of the CloudKitService interface.
• Mock simplification (e.g. throw .networkError(.timeout) vs 8-line NSError construction) is a real readability improvement.
• RecordOperationConversionTests parametrized test across all OperationType cases is well-structured.
• The withKnownIssue approach for flaky timing tests is correct in principle; the concern above is only about the missing issue matcher.

---

Summary

The two main items to address before merge: add CustomStringConvertible conformance to both reason types (otherwise description is never called in string interpolation), and add a negative test for the unknown operation type throw path. The flaky-test handling and availability-guard pattern are lower priority but worth cleaning up.

[claude]

Code Review — PR #305: Typed TokenManagerError + Safe RecordOperation Conversion

Overview

This PR replaces untyped any Error in TokenManagerError with structured reason enums (AuthenticationFailedReason, NetworkErrorReason), and eliminates a fatalError in RecordOperation→OpenAPI conversion by replacing it with a typed throw. Both are meaningful safety improvements. The scope is well-contained and the mock updates are thorough.

---

What Works Well

• Eliminating fatalError in Components.Schemas.RecordOperation.init(from:) is a high-value change. A crash for an unsupported operation type is unacceptable in a library; a thrown CloudKitError.unsupportedOperationType lets callers decide how to handle it.
• NetworkErrorReason cases (timeout, connectionLost, notConnectedToInternet) make pattern matching natural and readable. Mock code is dramatically cleaner.
• New RecordOperationConversionTests is well-structured with parameterized tests covering all valid operation types.
• withKnownIssue(isIntermittent: true) in AsyncHelpersTests+Timeout.swift is a pragmatic fix for cooperative-executor flakiness rather than chasing an ever-increasing sleep budget.

---

Issues and Suggestions

1. Test assertions should pattern-match the enum, not the description string

In RecoveryTests.swift and SimulationTests.swift:

// Current — brittle, coupled to the exact wording of description
if case .networkError(let reason) = error {
  #expect(reason.description.contains("timed out"))
}

// Better — pattern-match on the typed case directly
if case .networkError(.timeout) = error {
  // pass — the case itself is the assertion
} else {
  Issue.record("Expected .networkError(.timeout), got: \(error)")
}

The string "timed out" is correct today because NetworkErrorReason.timeout.description returns "Request timed out", but it will silently break if the description wording ever changes. The whole point of introducing typed error reasons is to enable this kind of pattern matching.

2. Missing test for the throw path in RecordOperationConversionTests

RecordOperationConversionTests covers all valid operation types but has no test that verifies an unsupported type throws CloudKitError.unsupportedOperationType. Since RecordOperation.OperationType is presumably a closed enum, this path may currently be unreachable — but documenting that guarantee in a comment, or testing it if the type allows, would make the intent explicit.

3. AuthenticationFailedReason and NetworkErrorReason should conform to CustomStringConvertible

Both types have a description: String computed property, but neither conforms to CustomStringConvertible. Without the conformance, "\(reason)" in string interpolation will produce the compiler's default enum representation (e.g. AuthenticationFailedReason.timeout) rather than the human-readable message. Conforming to CustomStringConvertible makes the descriptions discoverable and consistent:

public enum NetworkErrorReason: Sendable, CustomStringConvertible {
  // ...
}

4. Sendable conformance with any Error associated values

Both new enums are declared public enum ...: Sendable while carrying any Error in associated values (.tokenGenerationFailed(any Error), .signingFailed(any Error), .other(any Error)). In Swift 6 strict concurrency mode, any Error is not inherently Sendable, which means these may emit warnings or errors depending on the project's concurrency settings. The original TokenManagerError had the same pattern (networkError(underlying: any Error)), so this may already be handled (e.g. @unchecked Sendable somewhere upstream), but it's worth verifying that swift build -strict-concurrency=complete passes cleanly.

5. Semantic mismatch in MockTokenManagerWithAuthenticationError

The original mock threw authenticationFailed(underlying: nil) (i.e., "auth failed, unknown reason"). It's now authenticationFailed(.serverRejected(statusCode: 401, message: nil)), which implies a specific HTTP 401 response was received. These mocks test middleware behavior on any auth failure, not specifically on a 401 — so the specific status code is incidental and could mislead future readers about what conditions the mock simulates. A dedicated AuthenticationFailedReason.unknown case (or at minimum a comment) would be clearer, though this is a minor concern.

6. returnsAsyncValue is now non-throwing, but withKnownIssue body still uses try

internal func returnsAsyncValue() async {   // no `throws`
  await withKnownIssue(isIntermittent: true) {
    let result = try await withTimeout(seconds: 30.0) { ... }  // `try` inside

This compiles because withKnownIssue accepts a () throws -> Void body and catches errors as known issues. The behavior is correct, but readers who see a non-throwing function with try inside it may be confused. A brief comment that withKnownIssue absorbs the thrown error would help.

---

Minor Nits

• BatchSyncResultTests.swift reformatting (splitting the #expect across lines) is a pure style change with no logic impact — fine to include, but unrelated to the PR's stated goals.
• internal import Foundation in AuthenticationFailedReason.swift is correct since Foundation types don't appear in the public interface of that file.

---

Summary

Approve with suggestions. The core changes — typed error reasons and safe RecordOperation conversion — are solid improvements. The main actionable items before merging:

1. Replace reason.description.contains(...) checks with direct enum pattern matching.
2. Confirm Sendable compliance under strict concurrency.
3. Consider CustomStringConvertible conformance for the two new reason types.

🤖 Generated with Claude Code