Docs/talk prep archive#355
Draft
leogdion wants to merge 30 commits into
Draft
Conversation
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>
subrepo: subdir: "Examples/BushelCloud" merged: "55f2092" upstream: origin: "git@github.com:brightdigit/BushelCloud.git" branch: "mistkit" commit: "55f2092" git-subrepo: version: "0.4.9" origin: "https://github.com/Homebrew/brew" commit: "6f293daa9f"
subrepo: subdir: "Examples/CelestraCloud" merged: "c44dc4f" upstream: origin: "git@github.com:brightdigit/CelestraCloud.git" branch: "mistkit" commit: "c44dc4f" git-subrepo: version: "0.4.9" origin: "https://github.com/Homebrew/brew" commit: "6f293daa9f"
…ud CI; exclude internal Python from CodeFactor
- README.md, Examples/BushelCloud/{CLAUDE.md,.docc,.claude/s2s-auth-details.md},
Examples/CelestraCloud/{CLAUDE.md,README.md,.claude/IMPLEMENTATION_NOTES.md}:
drop `try CloudKitService(... database: .public)` from init examples (init is
non-throwing, `database:` moved per-call); rewrite Quick Start auth around
`Credentials` + `APICredentials` / `ServerToServerCredentials` and show
`database: .public(.prefers(.serverToServer))` at the call site.
- Examples/BushelCloud/.github/workflows/{BushelCloud.yml,bushel-cloud-build.yml}:
pin MISTKIT_BRANCH to v1.0.0-beta.1 (matches CelestraCloud) so the subrepo PR
builds against the branch that actually carries the new API. Revert to `main`
once #298 merges.
- .codefactor.yml: exclude Scripts/mermaid-to-pptx.py (internal-use helper).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
subrepo: subdir: "Examples/BushelCloud" merged: "5bb4490" upstream: origin: "git@github.com:brightdigit/BushelCloud.git" branch: "mistkit" commit: "5bb4490" git-subrepo: version: "0.4.9" origin: "https://github.com/Homebrew/brew" commit: "6f293daa9f"
subrepo: subdir: "Examples/CelestraCloud" merged: "ea897c3" upstream: origin: "git@github.com:brightdigit/CelestraCloud.git" branch: "mistkit" commit: "ea897c3" git-subrepo: version: "0.4.9" origin: "https://github.com/Homebrew/brew" commit: "6f293daa9f"
|
Important Review skippedDraft detected. 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 — PR #355 (DRAFT)
OverviewThis is a large archival/prep PR (~80k additions, ~47k deletions across 100+ files) that bundles roughly 25 commits covering:
Code Quality & Best PracticesPositive observations from commit history:
Potential concerns to verify before un-drafting:
CI / Workflow Changes
Test CoverageThe commit messages describe test additions for:
No obvious gaps from commit descriptions, but with this many changes the overall coverage picture should be verified once CI runs clean. Minor / Nits
SummaryThe code direction and decisions reflected in the commit messages look solid. The main pre-merge items are: (a) verify the Once CI is green and the DRAFT is lifted, this looks close to mergeable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.