Skip to content

Add confirmed zone metadata (syncToken, atomic) to zone schemas - #427

Open
leogdion wants to merge 5 commits into
v1.0.0-beta.4from
386-zone-schema-metadata
Open

Add confirmed zone metadata (syncToken, atomic) to zone schemas#427
leogdion wants to merge 5 commits into
v1.0.0-beta.4from
386-zone-schema-metadata

Conversation

@leogdion

Copy link
Copy Markdown
Member

Summary

Every zone response modeled a zone as just { zoneID }. This adds a shared Zone schema in openapi.yaml — reused by ZonesListResponse, ZonesLookupResponse, ZonesModifyResponse, and ZoneChangesResponse — carrying the zone metadata that Apple actually documents, and surfaces it on the domain ZoneInfo.

The issue flagged these field names as unverified, and verification changed the scope. Only two of the three proposed response fields exist in a primary source, and neither of the two proposed request changes does. I implemented the confirmed subset and left the rest out rather than guessing.

Verification against primary sources

Verified against Apple's archived CloudKit Web Services Reference (.claude/docs/webservices.md is abbreviated on zone payloads and confirms none of these, per .claude/memory/reference_cloudkit_archived_endpoints.md).

✅ CONFIRMED — implemented

Field Where Apple's wording Source
zoneID Zone Dictionary "The dictionary that identifies a record zone in the database" Types.html
syncToken Zone Dictionary "The current point in the zone's change history." Types.html
atomic Zone Dictionary "A Boolean value indicating whether this zone supports atomic operations." Types.html

The Zone Dictionary documents exactly these three keys — no more. All four zone endpoints route their success payload through it: zones/list, zones/lookup, zones/modify, zones/changes — each says "If successful, the result dictionary contains the keys described in Zone Dictionary."

❌ NOT CONFIRMED — deliberately omitted

Proposed Why omitted
isEager on zone responses Appears in no primary source. Not in the Zone Dictionary, not anywhere in the archived reference, and grep -i isEager over .claude/docs/webservices.md + .claude/docs/cloudkitjs.md returns nothing. No basis to encode a name or type.
atomic on the zones/modify request ModifyZones.html documents the request body as operations only ("This key is required"), with no atomic key. Contrast records/modify, which does document atomic — the asymmetry looks deliberate.
Zone create options on ZoneOperation Same page documents the operation's zone as "A dictionary representing the zone to modify. It has a single zoneID key." No room for create options.

A regression test (ZoneOperation encodes only operationType and zoneID) pins the request shape so create options can't be added back accidentally without a doc update.

Incidental findings (not acted on — filed as an issue comment)

  • zones/changes is documented as deprecated in favor of changes/database.
  • Its token key is documented as metaSyncToken, in both request and response — MistKit currently sends/reads syncToken. Left alone: out of scope, and Apple's own page is internally inconsistent (the moreComing description refers back to "the included syncToken key"), so this needs a live-response check rather than a doc-driven change.

Changes

  • openapi.yaml — new shared Zone schema; all four zone responses now $ref it.
  • Regenerated Sources/MistKitOpenAPI/Types.swift via ./Scripts/generate-openapi.sh (not hand-edited). The four inline anonymous zone structs collapse into one Components.Schemas.Zone, which is why that file shows net deletions.
  • ZoneInfo — adds syncToken: String? and atomic: Bool?, plus an init(from: Components.Schemas.Zone) conversion.
  • CloudKitService+ZoneOperations.swift, CloudKitService+ModifyZones.swift, ZoneChangesResult.swift — converted through the new initializer so metadata reaches callers.
  • AGENTS.md (symlinked as CLAUDE.md) — Result Types entry for ZoneInfo plus a zone-metadata section recording what is confirmed and what must not be added speculatively.

Design note

atomic is Bool?, not defaulted to false — an absent key stays distinguishable from an explicit false. Same for syncToken. Note the per-zone syncToken is distinct from the response-level syncToken on ZoneChangesResult; a test pins both.

Source compatibility

No breaks. Both ZoneInfo properties are added with defaulted initializer parameters, so the existing init(zoneName:ownerRecordName:capabilities:) still compiles. modifyZones gained no parameters — the proposed atomic: flag was unconfirmed, so the signature is untouched. ZoneInfo is a struct with a memberwise-style public init, so adding stored properties is additive here.

Out of scope: RecordResult pattern

Per .claude/memory/feedback_record_result_pattern_throughout.md, per-item modify failures should surface via the RecordResult pattern for zones too. modifyZones does not do this today — it returns [ZoneInfo] and silently drops errored entries, exactly the gap that memory describes. Apple documents a "Zone Fetch Error Dictionary" (zoneID, reason, serverErrorCode, retryAfter, redirectURL) returned inline for failed zones in zones/list, zones/lookup, and zones/modify.

Fixing it means a ZoneOperationFailure schema, a oneOf response, and changing modifyZones' return type — a real source break and a distinct piece of work from this schema enrichment. Filed separately rather than folded in here.

Verification

All run locally in the worktree:

  • swift build — ✅ clean
  • swift test — ✅ 560 tests in 176 suites passed (10 new)
  • mise exec -- swift-format -i -r Sources/ Tests/ — ✅ applied
  • ./Scripts/lint.sh — ✅ 0 violations in 393 files, periphery reports no unused code. (One pre-existing swift-format warning in CloudKitService+BatchChunking.swift, a file this PR does not touch.)
  • Examples/MistDemo swift build — ✅ clean (API shape changed, so verified)

New tests cover: Zone payload decoding, ZoneInfo metadata carry-through, absent-stays-nil, atomic: false preservation, the existing missing-zoneName throw path, decoding across all four response types, zone-level vs response-level syncToken, and the ZoneOperation request shape.

Closes #386

🤖 Generated with Claude Code

leogdion and others added 3 commits August 20, 2026 14:16
Adds two project memory files plus their MEMORY.md index entries:

- project_beta4_worktree_layout: the branch/worktree split for the
  remaining v1.0.0-beta.4 issues, the grouping rule (shared openapi.yaml
  path family => shared branch, to avoid Sources/MistKitOpenAPI/
  regeneration collisions), and why #407 was excluded.
- project_419_fixed_in_beta3: #419 is already fixed by 5a58120 and
  verified building on macOS Swift 6.3.2; do not re-implement.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All four zone responses (`zones/list`, `zones/lookup`, `zones/modify`,
`zones/changes`) modeled a zone as just `{ zoneID }`. Introduce a shared
`Zone` schema in `openapi.yaml` carrying the metadata Apple's archived
"Zone Dictionary" documents, and surface it on the domain `ZoneInfo`.

Only fields confirmed against a primary source are encoded:

- `syncToken` — "The current point in the zone's change history."
- `atomic` — "A Boolean value indicating whether this zone supports
  atomic operations."

Both verified against Apple's archived CloudKit Web Services Reference
"Zone Dictionary" (Types.html), which documents exactly three keys:
`zoneID`, `syncToken`, `atomic`.

Deliberately NOT implemented, because no primary source confirms them:

- `isEager` — appears in neither the archived reference nor the local
  CloudKit JS docs.
- `atomic` on the `zones/modify` request — Apple documents the request
  body as `operations` only.
- zone create options on `ZoneOperation` — Apple documents the
  operation's `zone` as having "a single `zoneID` key".

`ZoneInfo.atomic` is `Bool?` rather than defaulting to `false` so an
absent key stays distinguishable from an explicit `false`. Both new
properties are added with defaulted initializer parameters, keeping the
existing public initializer source-compatible.

Refs #386

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

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 Plus

Run ID: 7c196316-6405-444e-a56f-15d8e34b82ff

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

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

Comment @coderabbitai help to get the list of available commands.

Captures the primary-source verification done for #386: the Zone
Dictionary has exactly three keys, and isEager / modify-request `atomic`
/ zone create options do not exist in any Apple source. Also records the
unresolved metaSyncToken discrepancy on zones/changes so it isn't
re-investigated from scratch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review: #427 — Add confirmed zone metadata (syncToken, atomic) to zone schemas

Overall this is a clean, well-scoped change. The PR description's source-verification work (Zone Dictionary vs. isEager/request atomic/create-options) is genuinely good practice, and the openapi.yaml → generated-Types.swift → domain-conversion flow follows the existing FieldValueRequest/Response pattern documented in CLAUDE.md.

Code quality / correctness

  • ZoneInfo(from:) (Sources/MistKit/Models/Zones/ZoneInfo.swift:103) correctly delegates to fromZoneID:syncToken:atomic:, and all three call sites (CloudKitService+ZoneOperations.swift, CloudKitService+ModifyZones.swift, ZoneChangesResult.swift) were updated consistently — no leftover call sites use the old metadata-less form.
  • Keeping atomic/syncToken as Bool?/String? rather than defaulting atomic to false is the right call per the PR's own reasoning — "absent" and "explicitly false" are genuinely different signals from CloudKit.
  • The regenerated Types.swift diff is exactly what I'd expect from collapsing four anonymous zonesPayloadPayload structs into one shared Components.Schemas.Zone — no hand-editing artifacts, no orphaned anonymous types left behind.
  • No naming collisions: Components.Schemas.Zone doesn't clash with anything else in the generated or hand-written code.
  • Source compatibility claim checks out — both new ZoneInfo fields are trailing with = nil defaults, so ZoneInfo(zoneName:ownerRecordName:capabilities:) call sites still compile.

Test coverage — one gap worth flagging
The new tests (ZoneMetadataTests+Responses.swift, +ZoneInfoConversion.swift) are thorough at the unit level: they decode the generated Zone/ZonesXResponse types directly and exercise ZoneInfo(from:) in isolation (including the absent-vs-false distinction, which is exactly the case worth pinning).

However, none of the existing CloudKitService-level suites (Tests/MistKitTests/CloudKitService/LookupZones/, .../ModifyZones/, .../FetchZoneChanges/) were updated to assert that syncToken/atomic actually survive the full path — mock transport → CloudKitResponseProcessorZoneInfo — for lookupZones, modifyZones, and fetchZoneChanges. Right now nothing exercises the real call sites this PR touches (CloudKitService+ZoneOperations.swift:118, CloudKitService+ModifyZones.swift:105, ZoneChangesResult.swift:140) end-to-end with metadata present in a mock response — only the direct decode + conversion paths are pinned. Worth adding at least one success-case assertion per operation (e.g. extend CloudKitServiceTests.LookupZones+SuccessCases.swift to check result.first?.syncToken).

Also pre-existing (not introduced by this PR, but adjacent): listZones has no CloudKitService-level test suite at all, so its edit at CloudKitService+ZoneOperations.swift:118 is compiled but not exercised by any test either before or after this change. Not a regression, but since this PR is already touching that exact line, it'd be a low-cost place to close the gap.

Scope note
The diff also includes two new memory files unrelated to #386.claude/memory/project_419_fixed_in_beta3.md (issue #419 already-fixed note) and .claude/memory/project_beta4_worktree_layout.md (worktree layout for parallel beta.4 work), plus the corresponding MEMORY.md index lines. These look like legitimate proactive-maintenance memory writes per the repo's memory convention, but they're topically unrelated to zone schema metadata — worth a sentence in the PR description (or splitting into their own PR) so reviewers aren't left guessing whether they're intentional or leaked from a shared workspace.

Security / performance
No concerns — this is a pure additive schema/decoding change with no new I/O, auth, or unbounded-data paths.

Nothing here blocks merging; the missing end-to-end assertions are the only actionable item.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review: Add confirmed zone metadata (syncToken, atomic) to zone schemas

Solid, well-scoped PR. The verification-against-primary-sources approach (confirming zoneID/syncToken/atomic from Apple's archived Zone Dictionary and explicitly declining to add unconfirmed isEager/modify-request atomic/zone create options) is exactly the right way to handle an ambiguous issue spec, and the regression test pinning ZoneOperation's request shape is a good guard against that scope creep creeping back in later.

Code quality

  • ZoneInfo(from:) cleanly layers on top of the existing fromZoneID: initializer, and the new syncToken/atomic properties are correctly left as non-defaulted optionals so "absent" stays distinguishable from an explicit false — matches the stated design intent and is tested (atomic decodes false without collapsing into nil).
  • The openapi.yaml change collapsing four duplicated anonymous zonesPayloadPayload structs into one shared Zone schema is a nice DRY win, and the generated Types.swift diff is a pure mechanical consequence of that (net deletions, as the PR description notes).
  • Source compatibility claim checks out: both new ZoneInfo initializer overloads use defaulted trailing parameters, so no existing call site breaks.

Test coverage

Good breadth — all four response shapes, zone-level vs. response-level syncToken distinction, absent-stays-nil, atomic: false preservation, and the pre-existing missing-zoneName throw path. No gaps I could find for the stated scope.

Minor nit

  • Tests/MistKitTests/Models/Zones/ZoneMetadataTests+Responses.swift and ZoneMetadataTests+ZoneInfoConversion.swift both have // ZoneMetadataTests.swift in their file-header comment instead of their own filenames. Every other split-test file in the repo (e.g. CloudKitRecordTests+FieldConversion.swift) keeps the header's filename line in sync with the actual file. Scripts/header.sh only walks Sources/, not Tests/, so ./Scripts/lint.sh won't catch this — worth a manual fix.

Process concern (not a code issue in this PR itself)

.claude/memory/project_419_fixed_in_beta3.md and .claude/memory/project_beta4_worktree_layout.md are unrelated to zone metadata (#386) and appear to have been picked up from shared worktree context. The same two files, plus the same MEMORY.md index lines, are also being added byte-for-byte in sibling PRs #425 and #426. Whichever of these three merges first is fine, but the other two will hit avoidable merge conflicts on MEMORY.md and redundant duplicate memory files. Might be worth pulling those two memory files out into their own small PR (or a shared base branch) so the zone-metadata PR stays scoped to #386 and the conflict is resolved once instead of twice.

Security / performance

No concerns — this is a response-schema addition with no new network surface, no untrusted-input parsing beyond what already existed, and no hot-path changes.

Nice work overall — the "verify against primary source, omit what isn't confirmed" discipline here is a good pattern to keep using on the remaining beta.4 issues.

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (v1.0.0-beta.4@d295c30). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...oudKitService/CloudKitService+ZoneOperations.swift 50.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##             v1.0.0-beta.4     #427   +/-   ##
================================================
  Coverage                 ?   76.45%           
================================================
  Files                    ?      158           
  Lines                    ?     3771           
  Branches                 ?        0           
================================================
  Hits                     ?     2883           
  Misses                   ?      888           
  Partials                 ?        0           
Flag Coverage Δ
mistdemo-spm-macos 11.32% <17.64%> (?)
mistdemo-swift-6.2-jammy 11.43% <17.64%> (?)
mistdemo-swift-6.2-noble 11.32% <17.64%> (?)
mistdemo-swift-6.3-jammy 11.35% <17.64%> (?)
mistdemo-swift-6.3-noble 11.32% <17.64%> (?)
spm 74.43% <94.11%> (?)
swift-6.1-jammy 74.50% <94.11%> (?)
swift-6.1-noble 74.19% <94.11%> (?)
swift-6.2-jammy 74.69% <94.11%> (?)
swift-6.2-noble 74.11% <94.11%> (?)
swift-6.3-jammy 74.13% <94.11%> (?)
swift-6.3-noble 74.11% <94.11%> (?)

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

☔ View full report in Codecov by Harness.
📢 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.


/// Convert a CloudKit `Zone` payload into a `ZoneInfo`, carrying the
/// zone-level metadata (`syncToken`, `atomic`) alongside the identity.
internal init(from zone: Components.Schemas.Zone) throws(ConversionError) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of (from zone:) make this (zone: )

/// boundary instead.
internal init(fromZoneID zoneID: Components.Schemas.ZoneID?) throws(ConversionError) {
internal init(
fromZoneID zoneID: Components.Schemas.ZoneID?,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove fromZoneID and just do zoneID

Print the new ZoneInfo metadata fields from list/lookup/create CLI and
verbose zone integration phases so zone schema changes are testable.

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review

Reviewed the diff for PR #427 (386-zone-schema-metadatav1.0.0-beta.4). Overall this is a clean, well-scoped, well-documented change — nice work on the source verification discipline.

Code quality / best practices

  • The new shared Zone schema in openapi.yaml (collapsing 4 duplicated anonymous zonesPayloadPayload structs into one Components.Schemas.Zone) is a good simplification — the generated Types.swift diff is net-negative even though it adds fields, confirming the dedup pays for itself.
  • ZoneInfo.init(from:) layering on top of the existing init(fromZoneID:syncToken:atomic:) is a clean, minimal-surface way to thread the new fields through without touching the "missing zoneID/zoneName throws" logic. Good reuse.
  • atomic: Bool? (not defaulted to false) and the accompanying "absent stays nil" test is the right call — collapsing "server omitted the key" into false would have been a real (if subtle) bug for API consumers who need to distinguish the two.
  • Source compatibility is preserved correctly: both new ZoneInfo initializers use defaulted trailing parameters, so no existing call site breaks.
  • Doc comments on ZoneInfo.syncToken/.atomic are clear about the nil semantics, which matters given the "don't default to false" design.

Potential bugs / issues

  • Pre-existing behavior, not a regression, but worth a callout: if CloudKit ever returns a mix of successful zones and inline "Zone Fetch Error Dictionary" entries in the same zones array (which Apple's docs describe for zones/list/lookup/modify), a single malformed/error entry will make .map { try ZoneInfo(from: $0) } throw and abort the entire batch rather than surfacing per-zone failures. This predates this PR (same .map + throws pattern existed before), so it's not something this PR needs to fix, but it's the same gap the PR body already flags under "Out of scope: RecordResult pattern" — good that it's tracked separately rather than silently left implicit.
  • No functional bugs found in the conversion/decoding logic itself. The four call sites (listZones, lookupZones, modifyZones, fetchZoneChanges) were all updated consistently to ZoneInfo(from:), and a grep confirms no leftover callers of the old fromZoneID: two-arg call shape.

Performance

No concerns — this only adds two optional scalar fields to an existing decode path; no new network calls, loops, or allocation patterns.

Security

No concerns — no new external input handling beyond what already existed (decoding additional optional fields from an already-trusted CloudKit response).

Test coverage

Strong. New tests cover:

  • Decoding syncToken/atomic across all four response shapes (list/lookup/modify/changes).
  • Zone-level vs. response-level syncToken staying distinct (ZoneChangesResult vs. per-Zone).
  • Absent-key-stays-nil vs. explicit atomic: false (the trickiest part of this change) — good, explicit regression test for that.
  • The pre-existing "missing zoneName throws" path still holds with the new initializer.
  • A ZoneOperation encodes only operationType and zoneID test that pins the request shape so atomic/create-options can't be silently reintroduced without a doc-backed reason — nice defensive test given the PR explicitly chose not to add those.

One gap: no test exercises the actual CloudKitService.listZones/lookupZones/modifyZones/fetchZoneChanges methods end-to-end with a mocked response containing the new fields (existing CloudKitServiceTests suites for those methods don't appear to have been extended) — the new tests validate the ZoneZoneInfo conversion in isolation but not that the service methods actually plumb the converted values back out. Given the conversion is a one-line change at each of the four call sites and the isolated tests are solid, this is a minor gap rather than a real risk.

Minor / non-blocking

Nothing here blocks merging — the scope discipline (explicitly declining isEager, request-side atomic, and zone create options due to lack of primary-source confirmation) is exactly the right call, and it's backed by a regression test so the omission can't regress silently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant