v1.0.0 beta.1#298
Conversation
|
Important Review skippedToo many files! This PR contains 300 files, which is 150 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (300)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
===========================================
+ Coverage 25.58% 70.46% +44.88%
===========================================
Files 95 551 +456
Lines 8232 15426 +7194
===========================================
+ Hits 2106 10870 +8764
+ Misses 6126 4556 -1570
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:
|
Code Review: v1.0.0 beta.1This is a large, well-structured release covering CI modernization, example improvements, and library hardening. The overall quality is high — strong test coverage, strict linting, and solid concurrency safety. Below are the findings by area. Overview
Code Quality
Every internal func createQueryRecordsPath(containerIdentifier: String) -> Operations.queryRecords.Input.Path { ... }
internal func createLookupRecordsPath(containerIdentifier: String) -> Operations.lookupRecords.Input.Path { ... }
// ... 8 more identical methodsThese are compiler-safe but are a maintenance burden. If a field like
Potential Bugs
var misdirectedRequestResponse: Components.Responses.UnprocessableEntity? { get }The property is named for HTTP 421 (Misdirected Request) but the type is
public var hasCredentials: Bool {
get async { !apiToken.isEmpty }
}The protocol name implies "valid credentials available," but this only checks that the API token string is non-empty. An expired, malformed, or revoked token will return
PR #279's article code shows: } catch let error as CloudKitError where error.serverErrorCode == .authenticationRequired {
if let redirectURL = error.redirectURL {Neither Performance Considerations
The 11-element closure array is scanned linearly for every error response. At this scale it's not a practical concern, but because only one case can match per response type, a
This helper allocates a new array every call. Since it's used at manifest evaluation time only, this is harmless. But the formatter left inconsistent style in the trailing closure: result.removeAll{
$0 == platform // missing leading space, not 2-space indented
}
SecurityThe The CI/CD
The
if: ${{ !contains(github.event.head_commit.message, 'ci skip') }}
Experimental Swift features in 20+ Test Coverage139 test files for 104 source files is an excellent ratio. The concurrent token refresh suite and the mock uploader infrastructure for asset upload tests are particularly well-designed. No gaps identified in the reviewed areas. Summary
The 421→422 status code mismatch is the item I'd most want to verify before tagging the release. Everything else is either documentation/cleanup or low-risk. |
Code Review: v1.0.0 beta.1This PR has 300+ changed files (43,756 additions / 24,212 deletions), which exceeds the GitHub diff API limit. A full line-by-line review isn't possible without cloning locally. For a release PR of this scope, I'd suggest:
If you'd like me to review specific files or subsystems from this PR, share the paths and I can dig in. Review by Claude Sonnet 4.6 |
Code Review — v1.0.0-beta.1 (#298)OverviewThis is a significant beta release bringing together multiple improvements merged via sub-PRs: typed Bugs
// AdaptiveTokenManager.swift
internal let storage: (any TokenStorage)? // stored, never written/read
if error.localizedDescription.contains("PEM")
|| error.localizedDescription.contains("format")
pushd $PACKAGE_DIR # breaks if PACKAGE_DIR contains spacesShould be Code Quality
Repeated Test CoveragePerformance suite is skeletal Pagination tests are success-only
CI / Scripts
if: ${{ !contains(github.event.head_commit.message, 'ci skip') }}
Security
Positives
Summary of Action Items
|
Code Review: v1.0.0 beta.1OverviewThis is a large release-candidate PR (~49k additions, ~25k deletions, 300+ files) touching three major areas:
Overall quality is high and patterns align well with project conventions. The issues below are refinements. IssuesBug: Wrong field used for error logging in
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…rd returns CodeFactor reported 19 violations on PR #298: - `CredentialsTokenManagerTests.swift` (274 lines, 16× `conditional_returns_on_newline`): Convert the single suite struct into a namespace enum with four nested sub-suites (PublicDatabase, PrivateShared, UserContext, PrivateKeyLoad) following the existing `CloudKitServiceTests.FetchCaller` layout. Each `guard #available(...) else { return }` becomes multi-line. - `CloudKitError+OpenAPI.swift` (237 lines): Move the 11 per-response private initializers into a sibling `CloudKitError+OpenAPI+Responses.swift`. Bump them from `private` to `internal` so the extractor array in the main file can still call them. Also pick up filename-comment corrections in `OpenAPI/Components/*.swift` that `Scripts/header.sh` (part of `Scripts/lint.sh`) normalized — stale `+MistKit.swift` suffixes were trimmed to match the actual filenames. Verified: - `./Scripts/lint.sh` → 0 violations - `swift test --filter CredentialsTokenManagerTests` → 18 tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds tests that don't require live CloudKit access — covers the cheapest slice of the patch-coverage gap on PR #298 (codecov/patch: 13%). New test files (all @testable, no network): - Configuration/AuthTokenConfigTests.swift — memberwise init + parsed init via InMemoryProvider, including the missing/empty api.token error path - Configuration/UploadAssetConfigTests.swift — memberwise init for every field - Configuration/TestIntegrationConfigTests.swift — defaults + custom values - Configuration/TestPrivateConfigTests.swift — defaults + private-DB pinning - Commands/QueryCommand/QueryCommandTests+ParseFilter.swift — full coverage of parseFilter / inferFieldValue / shouldIncludeField / buildComparisonFilter (parameterized over every operator alias and error path) - Commands/CreateCommand/CreateCommandTests+GenerateRecordName.swift — prefix, three-part format, suffix range, distinctness across 200 calls - Commands/DemoErrorsRunnerOutputTests.swift — describe(_:) placeholder + echo Source changes are minimal seams that mirror the existing DeleteCommand.mapConflict pattern (internal-static for testability): - QueryCommand: filter parsing helpers extracted into QueryCommand+FilterParsing.swift; private → internal static - CreateCommand.generateRecordName: private → internal 901 MistDemo tests pass; lint + swift-format clean. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review
OverviewPR #298 ("v1.0.0 beta.1") is a large release PR that includes:
Positive Highlights
Issues and Suggestions1. Breaking availability change — needs changelog entry// Before
@available(macOS 11.0, iOS 14.0, tvOS 14.0, watchOS 7.0, *)
// After
@available(macOS 12.0, iOS 15.0, tvOS 15.0, watchOS 8.0, *)This is a breaking API change for any caller deployed on macOS 11 / iOS 14. The PR description and 2. Logging redaction regression riskFile: The original code had: MistKitLogger.logDebug(
"Unhandled response (HTTP \(statusCode)): \(response)",
logger: MistKitLogger.api,
shouldRedact: false // ← explicit
)The new code drops Redacting debug-level logs of unhandled CloudKit responses makes debugging new/unexpected error shapes harder. Consider keeping 3. Smoke tests don't assert anythingSeveral tests in @Test("printRunnerHeader does not throw")
internal func headerDoesNotThrow() async throws {
let runner = try await DemoErrorsRunner(config: MistDemoConfig())
runner.printRunnerHeader()
// No assertion
}If these methods print to stdout, capturing output (or even checking a return value where possible) would make these tests meaningful rather than just crash guards. For methods that truly have no observable output besides side effects, consider at minimum noting this limitation in the test name, e.g. 4. Force-unwrap in test fileFile: try json.data(using: .utf8)!.write(to: tmp)While try Data(json.utf8).write(to: tmp)5. Multi-line doc comment in test helperFile: The doc comment block (lines explaining // Keys are split on "." to form AbsoluteConfigKey components.
internal static func testing(
_ values: [String: ConfigValue]
) -> MistDemoConfiguration {6. Inconsistent smoke-test commentary
// Smoke test: type-check that the value is constructable.
_ = commandBut equivalent smoke tests in 7. PR title mismatchThe PR title "Update README with fixed serialization and integration test" does not match the actual changes (availability bumps, new test suite, logging changes). A more accurate title would help future SecurityNo security concerns found. The logging redaction change (item #2 above) is the closest thing — it actually increases default redaction, which is conservative. The risk is diagnostic, not security-related. Test Coverage AssessmentThe new config tests are thorough for the happy path and most error paths. One gap: there is no test verifying that SummaryThe PR is in good shape overall. The three items most worth addressing before merge are:
Everything else is low-priority style/polish. |
subrepo: subdir: "Examples/BushelCloud" merged: "123a732" upstream: origin: "git@github.com:brightdigit/BushelCloud.git" branch: "mistkit" commit: "123a732" git-subrepo: version: "0.4.9" origin: "https://github.com/Homebrew/brew" commit: "b9763ee528"
subrepo: subdir: "Examples/CelestraCloud" merged: "4244497" upstream: origin: "git@github.com:brightdigit/CelestraCloud.git" branch: "mistkit" commit: "4244497" git-subrepo: version: "0.4.9" origin: "https://github.com/Homebrew/brew" commit: "b9763ee528"
#28, #34, #35) (#315) * #312 library: add public+web-auth user-identity endpoints and users/caller migration Implements the library side of #312 — adding/renaming user-identity endpoints that require public-database routing with web-auth (user-context) credentials, and unblocking the convenience initializers from their hardcoded database/ environment defaults. #310: `CloudKitService` convenience initializers now accept `database:` and `environment:` parameters with defaults that preserve current behavior. #311: `users/current` → `users/caller`. Renamed in openapi.yaml and the generated client; added a hand-written `fetchCaller()` plus an `@available(*, deprecated, renamed: "fetchCaller")` `fetchCurrentUser()` shim that forwards to the new method. #28: GET `/users/discover` (`discoverAllUserIdentities`). #34: POST `/users/lookup/email` (`lookupUsersByEmail`). #35: POST `/users/lookup/id` (`lookupUsersByRecordName`). The three new endpoints reuse `DiscoverResponse` for parsing — Apple returns `{ users: [UserIdentity] }` for all of them. Each ships with a 5-file test suite mirroring the existing `DiscoverUserIdentities` pattern. #33 (`users/lookup/contacts`) intentionally not implemented: Apple has marked the endpoint deprecated. To be closed as not-planned with a pointer to #34/#35. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #312 MistDemo: separate database from authentication and add user-context phases Refactors MistDemo's CloudKit configuration model and integration runner to support the public+web-auth combination required by the user-identity endpoints landed in the prior commit. **Configuration refactor.** Replaces the `DatabaseCredentials` enum (which coupled database choice to a single auth method per case, baking in a public⇒S2S/private⇒webAuth assumption) with two orthogonal types: - `AuthenticationCredentials` — `serverToServer(keyID:privateKey:)` / `webAuth(apiToken:webAuthToken:)` - `DatabaseConfiguration` — pairs a `MistKit.Database` with an `AuthenticationCredentials`. The `make(database:authentication:)` factory rejects private+S2S and shared+S2S (which CloudKit rejects) so invalid combinations remain unrepresentable, while public+webAuth is now a valid construction. `MistKitClientFactory.create(for:)` consumes `toPrimaryConfiguration()`; the new `createUserContext(for:)` returns the optional public+web-auth service from `toUserContextConfiguration()` when web-auth tokens are configured. **Phase plumbing.** `PhaseContext` and `IntegrationTestRunner` now thread an optional `userContextService: CloudKitService?`. `PublicDatabaseTest` takes `includeUserContextPhases:` and conditionally appends the new user-identity phases: - `FetchCallerPhase` (renamed from `FetchCurrentUserPhase`) - `DiscoverUserIdentitiesPhase` (existed; updated to use userContextService) - `DiscoverAllUserIdentitiesPhase` (#28) - `LookupUsersByEmailPhase` (#34) - `LookupUsersByRecordNamePhase` (#35) `PrivateDatabaseTest` no longer includes `FetchCurrentUserPhase`: CloudKit rejects `users/caller` against the private database, matching the rest of the user-identity family. **Call-site updates.** `CurrentUserCommand` and `DemoErrorsRunner` swap `fetchCurrentUser()` → `fetchCaller()`. `TestIntegrationCommand` and `TestPrivateCommand` now build and pass `userContextService`. Tests for `AuthenticationCredentials`, `DatabaseConfiguration.make` validation, and `MistDemoConfig.toPrimaryConfiguration` / `toUserContextConfiguration` ship alongside. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #312: mark discoverAllUserIdentities() unavailable pending #28 investigation Live verification on 2026-05-08 against iCloud.com.brightdigit.MistDemo returned HTTP 500 from Apple's GET /users/discover. The first 12 phases of mistdemo test-integration --verbose run green (the 8 base public+S2S phases plus FetchCallerPhase, DiscoverUserIdentitiesPhase, LookupUsersByEmailPhase, LookupUsersByRecordNamePhase) — only discoverAllUserIdentities fails, blocking phases beyond it. The endpoint is referenced in CloudKitJS but does not appear in Apple's CloudKit Web Services REST documentation. The actual REST shape is still under investigation under #28. Changes: - Marked `CloudKitService.discoverAllUserIdentities()` `@available(*, unavailable, message: ...)` with a pointer to #28. - Removed `DiscoverAllUserIdentitiesPhase` from MistDemo and from `PublicDatabaseTest.phases`. - Removed the `CloudKitServiceDiscoverAllUserIdentities` test directory (the unavailable method cannot be called from Swift code). The OpenAPI definition, generated client, path builder, response processor, Output extension, and Swift wrapper are all retained. Unblocking is a one-line `@available` removal once the correct REST shape is determined under #28. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #315: resolve PR review — Credentials API, per-call database, cascade unavailable Addresses all four review threads on PR #315: - Comment #1 (error wording): removed `unsupportedDatabaseAuthCombination` along with `MistDemo.DatabaseConfiguration`; invalid combos now surface as `CloudKitError.missingCredentials` from the library. - Comment #2 (per-call database): user-identity ops in `CloudKitService+UserOperations` hardcode `.public`; record/zone/asset/sync ops accept `database: Database? = nil` falling back to a service-level default. - Comment #3 (unified credentials): new `Credentials` / `ServerToServerCredentials` / `APICredentials` value types replace the legacy `apiToken:`/`webAuthToken:` initializers. The token manager is selected based on the target database (S2S for `.public`, web-auth for `.private`/`.shared`). Lifted `PrivateKeyMaterial` into the library. - Comment #4 (cascade unavailable): removed `Operations.discoverAllUserIdentities.Output: CloudKitResponseType` conformance entirely; `processDiscoverAllUserIdentitiesResponse` is now `@available(*, unavailable)` with a `fatalError` body. Also migrates ~15 MistKit test helpers and the MistDemo factory to the new Credentials API. Breaking changes (pre-1.0): removed legacy `CloudKitService` initializers taking `apiToken:`/`webAuthToken:`; `CloudKitService.apiToken` is removed, `.database` is now `internal`. Out of scope: per-call `TokenManager` dispatch (would let one service mix S2S-for-public and web-auth-for-user-context). MistDemo still constructs a separate `userContextService` for that scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #315: drop service-level database, per-call credential resolution [skip ci] Resolves the architectural feedback in the PR-315 review: * CloudKitService no longer carries `database` — operations take `database:` per call (defaulting to `.public`); user-identity routes drop the parameter since CloudKit pins them to `.public`. Subsumes Claude's "fetchCaller bypasses self.database" finding. * Credentials.makeTokenManager(for:requiresUserContext:) resolves the appropriate token manager at dispatch time. A single service can now serve public-database S2S record ops and user-identity web-auth routes from one fully-populated `Credentials`. MistKitClient.swift is obsolete and removed; per-call dispatch lives in CloudKitService+ClientDispatch. * Credentials.swift split per SwiftLint one_file_per_declaration into ServerToServerCredentials.swift + APICredentials.swift + Credentials.swift. New typed CredentialsValidationError; init asserts in debug, throws in release (no more precondition crash for dynamic config). * MistDemo: userContextService workaround collapsed — single service handles all phases via per-call resolution. * CI hotfix: 11 unused `public import` lines demoted to `internal` (the warnings-as-errors regression flagged in the review). * Tests: 12-case routing-matrix unit suite for makeTokenManager and a fetchCaller suite parallel to LookupUsers* (success + validation). Obsolete MistKitClient tests removed. * Polish: shorter @available message on discoverAllUserIdentities, structural comment for GET /users/discover in openapi.yaml, ConfigurationError.missingAPIToken (unused) removed. 475/475 tests pass. Library + MistDemo build clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Per review on PR #315: listZones, lookupZones, fetchZoneChanges now default to .private since the public database only contains _defaultZone, making .public a degenerate default. MistDemo callers pass context.database / config.base.database explicitly so the --database flag still drives the test runs. Also repairs MistDemo test breakage from 7debe8d: toUserContextCredentials() was removed but tests still referenced it; rewritten against the replacement surface (toPrimaryCredentials embeds apiAuth on .public, plus the new hasUserContextCredentials boolean). The CredentialsValidationTests suite was deleted — it asserted init-time validation that no longer exists under per-call credential resolution; the equivalent .missingCredentials behavior is covered in MistKitTests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #312: gate @available(*,unavailable) on processDiscoverAllUserIdentitiesResponse to Swift 6.2+ Swift 6.1 rejects calls to an unavailable function from within another unavailable function; 6.2 relaxed that rule. The internal helper processDiscoverAllUserIdentitiesResponse is unavailable in lockstep with its only caller — the also-unavailable CloudKitService.discoverAllUserIdentities() — which built fine on 6.2+ but failed on Swift 6.1 with: error: 'processDiscoverAllUserIdentitiesResponse' is unavailable: Pending #28: discoverAllUserIdentities is not yet ready. Wrap just the attribute in `#if swift(>=6.2)` so the body is shared and 6.1 compiles. Inline doc records the intent and the one-line cleanup (delete the #if/#endif) once 6.1 is dropped from the matrix. A `swiftlint:disable:next unavailable_function` is required because swiftlint does not evaluate #if and otherwise sees a fatalError-only function without the attribute. Verified: swift build + swift test pass on Swift 6.1.3 (Linux container) and on macOS Swift 6.2+ (475/475 tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #315: split unhandled-response logging into debug (full body) + warning (type/status only) CodeQL's swift/cleartext-logging flagged the existing warning logs because lookupUsersByEmail(_:) propagates email-PII taint through the response object. Move full \(response) interpolation to .debug so the detail stays available for development without flowing into ops logs; keep .warning at type(of:) + HTTP status code only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #312: add --lookup-email / CLOUDKIT_LOOKUP_EMAIL to exercise users/lookup/email LookupUsersByEmailPhase previously skipped whenever fetchCaller() didn't return an email (which is the common case). Plumb a configurable lookup email through TestIntegrationConfig / TestPrivateConfig → PhaseContext so the phase can be driven against a known-discoverable iCloud account. Falls back to caller email, then to a clearer skip message naming the flag/env var. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: point CLAUDE.md lint section at mise (and Scripts/lint.sh) swift-format / swiftlint / periphery are pinned in mise.toml; the previous "requires swiftlint installation" wording led to PATH lookups that fail in this repo. Replace with `mise exec --` invocations and flag the full ./Scripts/lint.sh pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * #315: address review punch list — invalidPrivateKey, recoverable unavailable response, supportsUserContextPhases derivation - CloudKitError: add invalidPrivateKey(path:underlying:) so PEM-load failures carry the file path + original error instead of bare Foundation NSError. Wrap loadPEM() at the single call site in Credentials+TokenManager. Add PrivateKeyMaterial.filePath accessor for the diagnostic. - processDiscoverAllUserIdentitiesResponse: replace fatalError with throw CloudKitError.unsupportedOperationType so a stray Swift 6.1 caller (where the @available cascade does not apply) gets a recoverable error instead of a crash. - TestPrivateCommand: derive supportsUserContextPhases from config.base.hasUserContextCredentials, mirroring TestIntegrationCommand, so user-identity phases skip cleanly when web-auth env vars are absent. - toPrimaryCredentials: replace try? with do/catch + stderr INFO line so operators see when web-auth is missing on a .public setup. - CLAUDE.md: annotate discoverAllUserIdentities() as unavailable pending #28. - CredentialsTokenManagerTests: fill the missing routing-matrix branches (user-context × .private/.shared, .shared + token-only) and cover the new .invalidPrivateKey path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — v1.0.0-beta.1This is a substantial release PR (~50k lines changed across ~18 merged commits). The overall quality is high — the credentials redesign, per-call database routing, and pagination work are all solid. A few real bugs and a handful of nits worth addressing before the final release. Overview of changes
Bugs1.
public func fetchExistingRecordNames(
recordType: String,
limit: Int? = nil
) async throws(CloudKitError) -> Set<String> {
let result: QueryResult = try await queryRecords(
recordType: recordType,
limit: limit ?? Self.maxRecordsPerRequest // ← hardcodes .public via default
)
...
}
public func modifyRecords(
_ operations: [RecordOperation],
classification: OperationClassification,
atomic: Bool = false
) async throws(CloudKitError) -> BatchSyncResult {
let records = try await modifyRecords(operations, atomic: atomic) // ← hardcodes .public
...
}Both the query and the modify in the classify-then-modify pattern silently default to
These should both take 2.
throw CloudKitError.httpErrorWithRawResponse(
statusCode: 400,
rawResponse: "recordType cannot be empty"
)...for preconditions like empty Design / API concerns3. Record changes are almost always relevant for 4. // CloudKitService+Initialization.swift:31
public import OpenAPIRuntimeThe public 5. Cancellation maps to In Code quality / nits6. Per-call
7. Internal deprecated The file has a deprecated 8. Construction-time validation uses a separate Positives worth calling out
Priority summary:
|
CLAUDE.mdThis file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. Project OverviewMistKit is a Swift Package for Server-Side and Command-Line Access to CloudKit Web Services. It targets cross-platform Swift (including Linux, WASI, and Windows) using modern Swift concurrency and code generated from Apple's CloudKit Web Services OpenAPI specification. Key Project Context
Development CommandsSwift Package Commands# Build the package
swift build
# Run tests
swift test
# Run tests with coverage
swift test --enable-code-coverage
# Build for release
swift build -c release
# Clean build artifacts
swift package clean
# Update dependencies
swift package update
# Resolve package dependencies
swift package resolve
# Generate Xcode project (if needed)
swift package generate-xcodeprojOpenAPI Code Generation# Generate OpenAPI client code (run this after modifying openapi.yaml)
./Scripts/generate-openapi.sh
# Or manually with swift-openapi-generator
swift run swift-openapi-generator generate \
--output-directory Sources/MistKit/Generated \
--config openapi-generator-config.yaml \
openapi.yamlDevelopment Workflow# Run specific test
swift test --filter TestClassName.testMethodName
# Run tests in parallel
swift test --parallel
# Show test output
swift test --verbose
# Format + lint
# swift-format, swiftlint, periphery, and swift-openapi-generator are pinned
# in mise.toml — do NOT invoke them from PATH directly. Run them THROUGH mise:
mise exec -- swift-format -i -r Sources/ Tests/
mise exec -- swiftlint # lint
mise exec -- swiftlint --fix # auto-fix
# Or run the full lint pipeline (build + swiftlint + header.sh + periphery):
./Scripts/lint.shMistDemo Commands# MistDemo is located in Examples/MistDemo and must be run from there
cd Examples/MistDemo
# Build MistDemo
swift build
# Run MistDemo commands
swift run mistdemo --help
swift run mistdemo auth-token
swift run mistdemo current-user
swift run mistdemo query
swift run mistdemo lookup
swift run mistdemo create
swift run mistdemo update
swift run mistdemo modify
swift run mistdemo delete
swift run mistdemo upload-asset
swift run mistdemo lookup-zones
swift run mistdemo fetch-changes
swift run mistdemo demo-in-filter
swift run mistdemo demo-errors
swift run mistdemo test-integration
swift run mistdemo test-private
# Run with specific configuration
swift run mistdemo --config-file ~/.mistdemo/config.json queryArchitecture ConsiderationsFieldValue Type ArchitectureMistKit uses separate types for requests and responses at the OpenAPI schema level to accurately model CloudKit's asymmetric API behavior: Type Layers:
Why Separate Request/Response Types?
Generated Types:
Conversion:
Modern Swift Features to Utilize
Package StructureCloudKitService Operations
Sync/Change Operations:
User-Identity Operations (public DB + web-auth required):
In MistDemo, integration runs targeting these endpoints use Result Types (Sources/MistKit/Service/):
Protocols:
Key Design Principles
LoggingMistKit uses swift-log for cross-platform logging support, enabling usage on macOS, Linux, Windows, and other platforms. Key Logging Components:
Logging Subsystems: MistKitLogger.api // CloudKit API operations
MistKitLogger.auth // Authentication and token management
MistKitLogger.network // Network operationsHelper Methods: MistKitLogger.logError(_:logger:shouldRedact:) // Error level
MistKitLogger.logWarning(_:logger:shouldRedact:) // Warning level
MistKitLogger.logInfo(_:logger:shouldRedact:) // Info level
MistKitLogger.logDebug(_:logger:shouldRedact:) // Debug levelPrivacy Controls:
Asset Upload Transport DesignWhen providing a custom
Why URLSession instead of ClientTransport? Asset uploads use
Design:
Implementation Details:
Future Consideration:
FilterBuilder Extensions
IN/NOT_IN serialization: Uses CloudKit Web Services Integration
Testing Strategy
Asset Upload TestingIntegration Test Requirements:
Test Files:
MistDemo Integration Test Runner
Run via Important Implementation Notes
OpenAPI-Driven DevelopmentThe Swift package uses Apple's swift-openapi-generator to create type-safe client code from the OpenAPI specification. Generated code is placed in
The
Key endpoints documented in the OpenAPI spec:
Reference DocumentationApple's official CloudKit documentation is available in When to Consult Each Documentwebservices.md (289 KB) - CloudKit Web Services REST API
cloudkitjs.md (188 KB) - CloudKit JS Framework
testing-enablinganddisabling.md (126 KB) - Swift Testing Framework
swift-openapi-generator.md (235 KB) - Swift OpenAPI Generator Documentation
See MistDemo Documentation
CloudKit Schema Languagecloudkit-schema-reference.md - CloudKit Schema Language Quick Reference
sosumi-cloudkit-schema-source.md - Apple's Official Schema Language Documentation
Comprehensive Schema GuidesFor detailed schema workflows and integration:
Additional Notes
|
|
MistKit
A Swift Package for Server-Side and Command-Line Access to CloudKit Web Services Table of ContentsOverviewMistKit provides a modern Swift interface to CloudKit Web Services REST API, enabling cross-platform CloudKit access for server-side Swift applications, command-line tools, and platforms where the CloudKit framework isn't available. Built with Swift concurrency (async/await) and designed for modern Swift applications, MistKit supports all three CloudKit authentication methods and provides type-safe access to CloudKit operations. Key Features
Getting StartedInstallationAdd MistKit to your dependencies: [
.package(url: "https://github.com/brightdigit/MistKit.git", from: "1.0.0-beta.1")
]Or add it through Xcode:
Requirements
Platform SupportMinimum Platform Versions
Quick Start1. Choose Your Authentication MethodMistKit supports three authentication methods depending on your use case: API Token (Container-level access)import MistKit
let service = try CloudKitService(
containerIdentifier: "iCloud.com.example.MyApp",
apiToken: ProcessInfo.processInfo.environment["CLOUDKIT_API_TOKEN"]!
)Web Authentication (User-specific access)let service = try CloudKitService(
containerIdentifier: "iCloud.com.example.MyApp",
apiToken: ProcessInfo.processInfo.environment["CLOUDKIT_API_TOKEN"]!,
webAuthToken: userWebAuthToken
)Server-to-Server (Enterprise access, public database only)let serverManager = try ServerToServerAuthManager(
keyIdentifier: ProcessInfo.processInfo.environment["CLOUDKIT_KEY_ID"]!,
privateKeyData: privateKeyData
)
let service = try CloudKitService(
containerIdentifier: "iCloud.com.example.MyApp",
tokenManager: serverManager,
environment: .production,
database: .public
)2. Create CloudKit Servicedo {
let service = try CloudKitService(
containerIdentifier: "iCloud.com.example.MyApp",
apiToken: ProcessInfo.processInfo.environment["CLOUDKIT_API_TOKEN"]!
)
// Use service for CloudKit operations
} catch {
print("Failed to create service: \\(error)")
}UsageAuthenticationAPI Token Authentication
Web AuthenticationWeb authentication enables user-specific operations and requires both an API token and a web authentication token obtained through CloudKit JS authentication. let service = try CloudKitService(
containerIdentifier: "iCloud.com.example.MyApp",
apiToken: apiToken,
webAuthToken: webAuthToken
)Server-to-Server AuthenticationServer-to-server authentication provides enterprise-level access using ECDSA P-256 key signing. Note that this method only supports the public database.
Error HandlingMistKit provides comprehensive error handling with typed errors: do {
let service = try CloudKitService(
containerIdentifier: "iCloud.com.example.MyApp",
apiToken: apiToken
)
// Perform operations
} catch let error as CloudKitError {
print("CloudKit error: \\(error.localizedDescription)")
} catch let error as TokenManagerError {
print("Authentication error: \\(error.localizedDescription)")
} catch {
print("Unexpected error: \\(error)")
}Error Types
Advanced UsageUsing AsyncHTTPClient TransportFor server-side applications, MistKit can use swift-openapi-async-http-client as the underlying HTTP transport. This is particularly useful for server-side Swift applications that need robust HTTP client capabilities. import MistKit
import OpenAPIAsyncHTTPClient
// AsyncHTTPClient instance usually supplied by the Server API
let httpClient : HTTPClient
// Create the transport
let transport = AsyncHTTPClientTransport(client: httpClient)
// Use with CloudKit service
let service = try CloudKitService(
containerIdentifier: "iCloud.com.example.MyApp",
apiToken: apiToken,
transport: transport
)Adaptive Token ManagerFor applications that might upgrade from API-only to web authentication: let adaptiveManager = AdaptiveTokenManager(
apiToken: apiToken,
storage: storage
)
// Later, upgrade to web authentication
try await adaptiveManager.upgradeToWebAuthentication(webAuthToken: webToken)ExamplesCheck out the
Documentation
LicenseMistKit is released under the MIT License. See LICENSE for details. Acknowledgments
Roadmapv1.0.0-alpha.1
v1.0.0-alpha.2
v1.0.0-alpha.3v1.0.0-alpha.4
v1.0.0-alpha.5
v1.0.0-beta.1Querying & Sync
Authentication
Error Handling
Concurrency
MistDemo
Tooling & CI
v1.0.0-alpha.X
v1.0.0
v1.1.0
Support
MistKit: Bringing CloudKit to every Swift platform 🌟 |
Code Review — v1.0.0 beta.1This is a large, multi-feature release PR (52k+ additions). Review focuses on the new library surface introduced here rather than example app or CI changes. OverviewIntroduces five major features for the beta milestone:
Code QualityCredentials API ( The Per-call dispatch ( Creating a fresh Pagination ( The two-level API (single-page One edge case to be aware of: the stuck-marker guard ( User operations ( All user-identity routes correctly hardcode Errors ( Comprehensive Minor: Test coverage The extension-per-feature test organization mirrors the source structure well. The routing-matrix tests in IssuesValidation errors use HTTP error codes ( guard !recordType.isEmpty else {
throw CloudKitError.httpErrorWithRawResponse(statusCode: 400, rawResponse: "recordType cannot be empty")
}
guard effectiveLimit > 0 && effectiveLimit <= 200 else {
throw CloudKitError.httpErrorWithRawResponse(statusCode: 400, rawResponse: "limit must be between 1 and 200...")
}Using Import style inconsistency
Suggestions
SummarySolid beta release. The Credentials API refactor and per-call dispatch are well-architected; pagination and user-identity surface are complete and tested. The issues above are minor — none are blockers. #28 ( Reviewed by Claude (claude-sonnet-4-6) |
v1.0.0 beta.1 Code ReviewThis is a large accumulation PR (1,006 files, ~52 k additions). I reviewed it by reading the key new source files directly. Overall the architecture is solid and the code quality is high. Notes below are roughly ordered by impact. OverviewThe PR lands several substantial features for the v1.0 milestone:
Test suite is thorough (178 files, well-organized by feature). Issues1. A CloudKit record type literally named 2. The overload delegates to 3. Same issue — it calls 4. Validation errors masquerade as HTTP errors ( Client-side guard failures (empty 5. Every 6. Empty-array edge case for lookup operations (
Minor Nits
SecurityNo concerns. The CodeQL cleartext-logging flag (user email PII in response objects) was addressed by splitting full-body logging to SummaryIssues 2 and 3 (missing |
|
Code Review: PR #298 — v1.0.0 beta.1 Note on review scope: This is a very large PR (52,607 additions / 25,812 deletions across 300+ files). GitHub's diff API returned a 406 too-large response, and the CI environment uses a shallow clone, so this review is based on the PR metadata, the branch state visible locally, and the 44-file diff between the current branch tip and the draft PR #332. Missing PR Description The PR body is empty. For a major versioned release (v1.0.0 beta.1), a description should include:
This is especially important for a 52K-line PR where reviewers cannot realistically read every diff line. Branch State The v1.0.0-beta.1 branch tip (a28ab3c — Resolve #313: paginationLimitExceeded carries accumulated records) is currently at the same commit as main. This means the branch has been kept in sync with main as sub-PRs were merged, which is good practice for a long-running release branch. Code Quality Observations (from visible diff context) The changes visible through the local diff between v1.0.0-beta.1 and the PR #332 merge ref reveal some patterns in the surrounding codebase that are relevant to review:
Test Coverage Based on the file list visible in the diff context:
Suggestions
|
Code Review — PR #298 (v1.0.0 beta.1)OverviewThis is a large beta release PR combining many changes: the core bug fix is #313 — ✅ What Looks Good
🐛 Bugs / Issues1.
guard ProcessInfo.processInfo.environment["MISTKIT_ENABLE_LOG_REDACTION"] != nil else {
return message // no redaction
}The comment says "Redaction disabled by default." But 2. Every other operation calls } catch let cloudKitError as CloudKitError {
throw cloudKitError
} catch {
throw CloudKitError.underlyingError(error) // ← skips ClientError unwrapping
}A transport-level 3. if error.localizedDescription.contains("PEM") || error.localizedDescription.contains("format") {
throw TokenManagerError.invalidCredentials(.invalidPEMFormat(error))
}
throw TokenManagerError.invalidCredentials(.privateKeyParseFailed(error))
4.
|
Code Review: v1.0.0 beta.1 (#298)This is a substantial release with 52K additions spanning the core library, CI infrastructure, and two example apps. The overall architecture is sound and the code quality is high. Below are the key findings. ✅ Strengths
🔴 High Priority1. Log Redaction Is Opt-In — Sensitive Data Logged by DefaultFile:
// Current: redaction only happens when env var is set
guard ProcessInfo.processInfo.environment["MISTKIT_ENABLE_LOG_REDACTION"] != nil else {
return message // returns raw message with tokens, keys, etc.
}For a library used in server-side contexts, logs are typically forwarded to aggregators. An operator who doesn't know to set Recommendation: Invert the default — redact by default, opt-out with 2. Error Logs Bypass Redaction EntirelyFile: All error-path log calls use MistKitLogger.logError(errorMessage, logger: logger, shouldRedact: false)Error messages can contain raw API responses, which may include record data, field values, or request parameters. These will be logged unredacted regardless of environment variable settings. 🟡 Medium Priority3. Silent Record Conversion Failures in
|
| Area | Status |
|---|---|
| Asset upload transport isolation | ✅ Correctly implemented |
| Authentication / token validation | ✅ Solid |
| Test coverage | ✅ Comprehensive |
| CI/CD structure | ✅ Good, minor head_commit bug |
| Log redaction default | 🔴 Needs inversion |
| Error log redaction bypass | 🔴 Review shouldRedact: false usage |
| Silent pagination failures | 🟡 Consider explicit error |
| Silent record conversion drops | 🟡 Add logging/assertion |
The core design and architecture are solid. The redaction default is the most important thing to address before GA since it affects all server-side deployments.
Code Review: PR #298 — v1.0.0 beta.1This is a comprehensive review of the v1.0.0 beta.1 release PR (52K+ additions, 25K+ deletions, 300+ files). The architectural direction is excellent. I focused on issues that could affect API consumers or correctness. OverviewThis PR delivers the v1.0.0 beta.1 milestone with several significant improvements:
Issues1.
Suggestion: Use the conjunction as the sentinel ( 2. The 3. Local validation errors use In Suggestion: Add a 4. The 5.
Security
Test CoverageStrong overall — 178 test files with per-feature suites for all major new surfaces.
Performance
Misc
OverallThe architecture is sound and this is a strong beta release. The Generated by Claude Sonnet 4.6 |
|
Code Review: PR #298 (v1.0.0 beta.1) This is a large accumulation of changes (100 files, +52k/-26k lines) across the Examples layer, CI infrastructure, and tooling. The core MistKit library (Sources/) is unchanged. Review focuses on the substantive new code in the examples. OVERVIEW The PR bundles several distinct improvements:
WHAT IS GOOD
ISSUES AND SUGGESTIONS
SECURITY No concerns. Key ID validation before file I/O and cryptographic work is correct. JSON report writes are user-specified paths from the CLI, appropriate for a local CLI tool. Private key files are read with existence checks and typed errors (BushelCloudKitError.privateKeyFileNotFound, .privateKeyFileReadFailed). TEST COVERAGE The new error-description and recovery-suggestion tests (CelestraErrorTests+Description.swift, +RecoverySuggestion.swift) provide good coverage of LocalizedError conformance. The MockCloudKitRecordOperator properly captures both query and modify calls for assertion. The split into extension files per-test-category is clean and matches project convention. SUMMARY The code is overall high quality and follows project conventions well. The most actionable items before merge are (1) fix copyright headers on the three new 2025-dated files, (2) add a PR description, and (3) remove the what-not-why comments in FeedUpdateProcessor+Fetch.swift and the deprecated CelestraConfig method body. |
Code Review: v1.0.0 beta.11006 files changed (+52607/-25812). Review covers substantive source changes; generated code and docs excluded. OverviewTwo major changes:
Both are the right call for beta.1. The old Core Library (
|
| Area | Status |
|---|---|
Database simplification |
Good |
| Auth credential resolution | Good — minor DRY opportunity in S2S key loading |
| Per-call auth-mode control | Feature removed — document intent before v1.0.0 final |
AuthTokenCommand rewrite |
Good — responseCompleteChannel timing is fragile |
| Test coverage | Gaps: CloudKitError description tests, unconfigured-public fallthrough |
| Security | No concerns |
Code Review: v1.0.0 beta.1 (#298)
Release Readiness Concerns🔴 No PR description. The
🟡 Patch coverage at 23.6% (per Codecov). 791 lines of new code have no test coverage. Before promoting to a stable 🟡 CodeRabbit skipped (300-file limit exceeded). No automated review tool has been able to fully scan the diff. This means any subtler issues in the later alphabetical files were missed. Structural ObservationsSize: 1,006 changed files is unusually large for a single review unit. Even for a "squash of many sub-PRs" workflow, the standard practice is to merge sub-PRs to an integration branch and keep each sub-PR reviewable. Consider whether the next beta cycle can be structured with more atomic PRs (e.g., one per major feature area: credentials, pagination, CI, examples) so reviewers can give meaningful approval. CI confidence: Multiple automated checks (GitHub Advanced Security, Codecov) are reporting, which is a good signal. Confirm all required status checks are green before merging. What the Prior Reviews Have FlaggedAcross the 17 existing review comments, the recurring findings are:
No review has flagged a blocking bug in the core library surface. RecommendationThis PR appears structurally sound for a beta tag. The two items to address before
Reviewed by Claude (claude-sonnet-4-6) |
Code Review — PR #298: v1.0.0 beta.1Overview: This is a large release-candidate PR (~52k additions / ~26k deletions across 300+ files) promoting BushelCloud, CelestraCloud, and MistDemo examples to a beta-1-ready state, alongside CI matrix, devcontainer, and tooling updates. The core CI StatusSix checks were failing at time of the original review snapshot; all but one are now green:
What Was Fixed WellCompile blockers (BushelCloud)
Deprecation migration (CelestraCloud)
watchOS flake
Thread safety (MockCloudKitRecordOperator)
UpdateReport model quality
Remaining Concern:
|
Code Review: v1.0.0 beta.1OverviewThis is a large release-preparation PR (~52k additions, ~26k deletions) targeting three example projects (BushelCloud, CelestraCloud, MistDemo) plus CI infrastructure. The core MistKit library itself is unchanged. The changes address CI failures, code-review comments from a prior audit, lint violations, and thread-safety gaps identified in the pre-release review. Strengths
CI matrix reduction Typed throws throughout Issues1. Copyright year still stale in new/expanded files Three files added or heavily modified in this PR carry
PR #331 addresses 2. internal struct ExitError: Error {}An empty error type makes it impossible to distinguish why the command exited without examining the surrounding print output. The plan document marks a refactor (use internal struct ExitError: Error {
internal let message: String
}3.
4. Comments describe what, not why, in BushelCloudKitService // Read PEM file from disk
let pemString: String
// Validate PEM format BEFORE passing to MistKit
try PEMValidator.validate(pemString)
// Create Server-to-Server authentication manager
let tokenManager = try ServerToServerAuthManager(…)Per CLAUDE.md, comments should only explain non-obvious why, not narrate what the code does — well-named identifiers already do that. The tutorial intent of the example is understandable, but these inline narration comments don't add signal for anyone reading the code. If the tutorial angle is important, consider a 5. Dual
6. Emoji usage in production code
Minor Notes
Test CoverageThe new SummaryThe thread-safety fix, Codable correctness fix, |
Code Review: v1.0.0 beta.1 (#298)This is a large, well-structured beta release. The overall architecture is solid — typed errors, per-call credential dispatch, OpenAPI-driven generation, and proper Security
MistKitLogger.logError(
"CloudKit queryRecords failed with response: \(response)",
logger: MistKitLogger.api,
shouldRedact: false // ← unconditionally off
)The earlier CodeQL fix on Correctness
assertionHandler(value == 0 || value == 1, "Boolean int64 value must be 0 or 1, got \(value)")
return value != 0 // ← always executedIn release builds (where guard value == 0 || value == 1 else { return nil }
return value != 0
if result.records.isEmpty
&& result.continuationMarker != nil
&& result.continuationMarker == currentMarker {
break // silent partial result
}The doc comment does say "stops early", but callers receive a normal
API Design
Code Quality / Minor
Every if let error = CloudKitError(response) { throw error }
switch response {
case .ok(...): ...
default:
assertionFailure("Unexpected response case after error handling")
throw CloudKitError.invalidResponse
}The Lots of redundant doc comments that narrate what the code already says Throughout /// Process lookupRecords response
/// - Parameter response: The response to process
/// - Returns: The extracted lookup response data
/// - Throws: CloudKitError for various error conditionsThese describe the method signature, not the non-obvious behaviour. The more useful thing to document is which CloudKit error codes each endpoint can return, or edge cases like the stuck-marker or 413 behaviour. Not blocking, but adds noise to DocC output. Positive highlights
Summary
|
2 participants
No description provided.