Pre-1.0.0 correctness & safety hardening#357
Conversation
Replace the `default: assertionFailure(...); throw .invalidResponse` fallback in every `CloudKitResponseProcessor` function with explicit case lists. The generator's per-operation `Operations.*.Output` enums are frozen, so listing the non-OK cases by name turns "new response variant added to openapi.yaml" into a compile-time break at the processor — exactly when the team needs to decide how to map it — instead of a silent `.invalidResponse` at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Synthetic `httpErrorWithRawResponse(statusCode: 400, ...)` was reporting caller-side argument failures as if they came from the server, so retry middleware, metrics, and callers inspecting `httpStatusCode` couldn't tell "we didn't even send the request" from "the server rejected it." Add a dedicated `invalidArgument(parameter:reason:)` case (no HTTP status code) and migrate `queryRecords`'s empty-recordType + out-of-range-limit guards. `CloudKitService+ZoneOperations.swift`'s synthetic-400 sites migrate in the same-file ZoneOps commit that handles #349. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) `listZones` and `lookupZones` rendered zones with a nil `zoneName` as `ZoneInfo(zoneName: "Unknown", ...)` — indistinguishable from a real zone named "Unknown". Extend the existing `compactMap` guard pattern that already drops entries with a nil `zoneID` to also drop entries with a missing name, so a malformed payload fails closed instead of normalizing to a plausible-but-wrong value. Regression test fixture mocks a response with one valid + one nameless entry. Same-file: migrate the lookupZones empty-zoneIDs / empty-zoneName guards from synthetic `httpErrorWithRawResponse(400, ...)` to the dedicated `.invalidArgument` case introduced in the prior commit (#352). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
At `.debug` level the middleware was eagerly collecting up to 1 MB of every response body — including asset payloads where the bytes aren't loggable text anyway. Lower the cap to 64 KB (enough to surface the JSON envelope + any error reason) and skip body collection entirely for non-`application/json` content types. Production traffic at `.info` and above is unaffected. Tests assert the body round-trips correctly for both JSON and octet-stream content types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CloudKit caps record field data at 1 MB and asset uploads at 15 MB. MistKit was relying on the server to reject oversize payloads, which wastes a network round-trip and surfaces as an opaque server error. Pre-flight both limits as `.invalidArgument`: - `modifyRecords` JSON-encodes each `RecordRequest` and throws if any exceeds `maxRecordDataBytes` (1 MB). The encoded representation is what actually travels over the wire, so it's the closest fidelity measure available without a separate sizing pass. - `uploadAssetData` (the low-level CDN path) and `uploadAssets` (the two-step convenience) both reject `data.count > maxAssetUploadBytes` (15 MB) before invoking the uploader closure. Out of scope: auto-chunking when a caller passes more than 200 records to `modifyRecords`. That has atomicity/partial-failure semantics worth discussing on its own. Also migrates `uploadAssets`'s existing empty-data + oversized-asset guards from synthetic `httpErrorWithRawResponse(400|413, ...)` to `.invalidArgument`, completing the #352 audit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The package floor sat one OS major below where P256.Signing.PrivateKey(pemRepresentation:) became available, so ServerToServerCredentials could be constructed in the gap window but never used to sign — Credentials+TokenManager either silently fell back to web-auth or threw .preferenceRequired despite valid credentials being configured. Raising the floor closes the window; S2S signing is unconditional when credentials are present. Drop the now-redundant @available gates from auth, the README platform table, and the Examples (whose own floors are well above macOS 11). Also fixes a pre-existing non-exhaustive switch in CelestraError.swift after CloudKitError.invalidArgument was added in 0dca94b. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Code Review — Pre-1.0.0 correctness & safety hardeningOverall this is a well-scoped, well-motivated PR. The six commits are clearly isolated and the rationale is strong. A few observations below, ordered roughly by severity. Leftover available guards are now dead code (cleanup)After the platform-minimum bump in commit 1f79d67, guards like guard #available(macOS 11.0, iOS 14.0, tvOS 14.0, watchOS 7.0, *) else { ... }can never be Additionally, the new Double JSON encoding in
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v1.0.0-beta.2 #357 +/- ##
=================================================
- Coverage 68.95% 68.82% -0.13%
=================================================
Files 111 113 +2
Lines 2641 2630 -11
=================================================
- Hits 1821 1810 -11
Misses 820 820
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Reverting the policy added in ee1293e. CloudKit owns the canonical per-record (1 MB) and per-asset (15 MB) limits; pre-flighting them in MistKit duplicated Apple's check and risked drift if the limits change. A developer who wants to bail out before sending can just as easily check sizes themselves. What stays as a faithful thin wrapper: - `modifyRecords` no longer JSON-encodes operations for size; lets CloudKit reject oversized records. - `uploadAssets` / `uploadAssetData` no longer guard against >15 MB; let the CDN reject. The empty-data guard on `uploadAssets` stays (as .invalidArgument) — that's a programmer error, not a CloudKit-imposed size policy. What replaces it for callers who do want a pre-flight: - `CloudKitService.maxRecordDataBytes` / `maxAssetUploadBytes` are now public — the documented constants are available without hardcoding. - New `RecordOperation.encodedRecordSize() throws -> Int` reports the JSON-encoded wire size of the record envelope, so callers can write `try op.encodedRecordSize() <= CloudKitService.maxRecordDataBytes` if they want to check ahead of `modifyRecords`. Tests: deletes the SizeLimits suites; drops the oversized-asset test from Upload+Validation (keeps empty + valid-size coverage); adds RecordOperationEncodedSizeTests for the new helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review: Pre-1.0.0 correctness & safety hardeningOverall this is a well-structured set of changes. The motivation behind each commit is clear, the design note on the size-validation reversal is convincing, and 460 tests passing is a good signal. A few things worth addressing before merge: Bug (medium) — LoggingMiddleware may return a partially-consumed bodyFile: do {
let bodyData = try await Data(
collecting: responseBody,
upTo: Self.responseBodyLogCap // 64 KB
)
logBodyData(bodyData)
return HTTPBody(bodyData)
} catch {
logger.error("📄 Response Body: <failed to read: \(error)>")
return responseBody // ⚠️ partially-consumed stream
}When The comment "Bodies bigger than this still stream through to the caller untouched" is not accurate — the stream has been touched. This existed before this PR at the 1 MB cap, but was rarely triggered in practice. At 64 KB it becomes realistic: a Suggested fixes (pick one):
Minor —
|
…MIC_ERROR; enrich quota errors with QuotaHint This is the larger-than-planned follow-through on the "thin wrapper" philosophy. Two coordinated changes: 1) Drop all client-side validation throws. The previous iteration (ee1293e, caa61d3) removed size pre-flighting; this commit removes the rest: - queryRecords: empty recordType + limit out-of-range - lookupZones: empty zoneIDs + empty zoneName - modifyZones: empty operations + empty zoneName + .public scope - fetchRecordChanges: resultsLimit out-of-range - uploadAssets: empty data CloudKit owns these constraints. The server returns BAD_REQUEST with a specific reason — duplicating that check client-side adds drift risk without ergonomic upside. The only pre-flight that remains is the OpenAPI converting init (RecordOperation type mapping), which catches a programmer error before serialization. Removes the now-producerless .invalidArgument case from CloudKitError. Deletes the +Validation test suites (Query, LookupZones, ModifyZones) and stale ValidationErrorType / validationError helpers. FetchChanges+Validation keeps its non-validation tests; Upload+Validation keeps its happy-path coverage. 2) Split three serverErrorCodes from httpErrorWithDetails into dedicated cases so consumers can pattern-match by intent rather than parsing strings: - .quotaExceeded(reason:, hint: QuotaHint?) // 413 / QUOTA_EXCEEDED - .badRequest(reason:) // 400 / BAD_REQUEST - .atomicFailure(reason:) // 400 / ATOMIC_ERROR Why these three: QUOTA_EXCEEDED is overloaded across storage-quota, per-record size, and per-asset size; ATOMIC_ERROR ≠ BAD_REQUEST semantically (batch rollback vs malformed input). The other 11 serverErrorCodes stay in httpErrorWithDetails — the action is the same regardless of which one fired (re-auth, back off, retry). A follow-up issue will track full enumeration with .unknownServerError. New QuotaHint enum carries the disambiguation CloudKit's response doesn't. modifyRecords / uploadAssetData enrich .quotaExceeded (or a bare 413 from the CDN) with .recordExceedsSizeLimit / .assetExceedsSizeLimit when local request state explains the server error. addingQuotaHint helper is a no-op when the hint doesn't apply, so non-size QUOTA_EXCEEDED (real storage exhaustion) passes through unchanged. Migrates 4 pattern-match sites from .httpErrorWithDetails(_, "BAD_REQUEST", _) to .badRequest. Sites matching 404/409/401 are unaffected (those codes still land in httpErrorWithDetails). Adds DemoErrorsRunner+Output coverage for the new cases. Updates CelestraCloud's CelestraError retriability switch for the new cases. Adds CloudKitServiceTests.SizeLimits + Records + Assets suites covering the post-flight enrichment path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — Pre-1.0.0 Correctness & Safety HardeningOverall this is a solid pass: the exhaustive-switch refactor alone is a meaningful correctness win, and the CorrectnessStale The most obvious case is the new test added in this PR: // Tests/MistKitTests/CloudKitService/LookupZones/CloudKitServiceTests.LookupZones+SuccessCases.swift
@Test("lookupZones() drops zones with missing zoneName instead of substituting placeholder")
internal func lookupZonesDropsZonesWithoutName() async throws {
guard #available(macOS 11.0, iOS 14.0, tvOS 14.0, watchOS 7.0, *) else {
Issue.record("CloudKitService is not available on this operating system.")
return
}Same issue remains in Performance
// CloudKitService+WriteOperations.swift
} catch let cloudKitError as CloudKitError {
throw cloudKitError.addingQuotaHint(
Self.recordSizeQuotaHint(for: apiOperations)
)
}
} catch let cloudKitError as CloudKitError {
if case .quotaExceeded = cloudKitError {
throw cloudKitError.addingQuotaHint(Self.recordSizeQuotaHint(for: apiOperations))
}
throw cloudKitError
}LoggingMiddleware body cap — verify the "stream through untouched" claimThe new constant and comment say: /// Bodies bigger than this still stream through to the caller untouched.
private static let responseBodyLogCap: Int = 64 * 1_024In swift-openapi-runtime, Please confirm the catch path in Minor nits
What's working well
|
1 participant
Summary
Umbrella PR for the pre-1.0 correctness pass tracked in #356. Eight commits, each scoped to a single sub-issue or design follow-up:
Out of scope under #356: the rest of #38 (auto-chunking >200-record batches) and #350 (closed as not-planned).
Design evolution
The PR ended up landing two architectural shifts that emerged from review discussion:
1. Pure thin wrapper for validation. `ee1293e` originally added client-side pre-flight for record/asset size limits. On reflection that was the wrong default — CloudKit owns the canonical limits, our checks duplicate Apple's validation and add drift risk. `caa61d3` reverted size pre-flight; `a8fbcb0` extended the reversal to all other client-side throws (empty-collection guards, numeric range caps, scope checks). The faithful pass-through is now consistent across operations.
Removed the now-producerless `.invalidArgument` case. The only escape hatch for callers who want a pre-flight: `CloudKitService.maxRecordDataBytes` / `maxAssetUploadBytes` (public constants) + `RecordOperation.encodedRecordSize()` (helper).
2. Type-safe error split for the three differentiated codes. `a8fbcb0` introduces:
```swift
case quotaExceeded(reason: String?, hint: QuotaHint?) // 413 / QUOTA_EXCEEDED
case badRequest(reason: String?) // 400 / BAD_REQUEST
case atomicFailure(reason: String?) // 400 / ATOMIC_ERROR
```
Why just these three:
The remaining 11 `serverErrorCode` values stay in `.httpErrorWithDetails` because the action a consumer takes on them is uniform (re-auth / back off / retry / surface). Full enumeration with `.unknownServerError` fallback is tracked in #358 as post-1.0 work.
Catch blocks in `modifyRecords` / `uploadAssetData` enrich `.quotaExceeded` (or a bare 413 from the CDN) with a `QuotaHint.recordExceedsSizeLimit` / `.assetExceedsSizeLimit` when local state explains the server's response. When there's no local size violation (real storage exhaustion), the hint is `nil` and the error passes through unchanged.
Test plan
Notes for reviewer
🤖 Generated with Claude Code