Add custom/shared zone support to the query path (#146)

[leogdion]

Summary

queryRecords hard-coded zoneID: .init(zoneName: "_defaultZone") into the records/query request body (CloudKitService+Operations.swift:85), making custom and shared zones unqueryable. This threads an optional zone through the whole query path.

Rebased onto current v1.0.0-beta.4 (including #424 deprecated-API removal).

Design decisions

ZoneID rather than zoneName: String

The issue sketched zoneName: String = "_defaultZone". I used the existing ZoneID type instead:

• ZoneID carries ownerName, which a bare string cannot. Shared zones live in the owner's database and are unaddressable without it — so a zoneName-only parameter would have left the issue's own "working with shared zones" use case unsolved.
• It matches the established precedent in this codebase: modifyRecords(_:atomic:zoneID:desiredKeys:numbersAsStrings:database:) already takes zoneID: ZoneID? = nil, with the same Components.Schemas.ZoneID(from:) conversion. Query and modify now name their zone the same way.
• ZoneID.defaultZone remains available for callers who want to say "default zone" explicitly.

Defaulted (= nil) rather than required

Per .claude/memory/feedback_no_silent_policy_defaults.md this was a deliberate call, not an oversight:

• That memory targets parameters carrying credential / attribution policy (PublicAuthPreference), where a silent default caused records to be mis-attributed. Nothing comparable rides on zone selection — a wrong zone yields wrong-or-empty results, visibly, not a silent security/ownership mistake.
• nil does not mean "quietly substitute _defaultZone". It means omit the zoneID key from the request entirely and let CloudKit resolve the default zone server-side. There is no client-side policy being invented.
• It matches the sibling request-option convention (zoneID / desiredKeys / numbersAsStrings / zoneWide on modifyRecords and fetchRecordChanges are all optional-defaulted).
• The public database has only one zone, so requiring the parameter would tax every public-database caller for a choice they don't have.

database: still has no default, as CLAUDE.md requires — untouched.

Surfaces updated

Surface	Change
queryRecords(_:limit:desiredKeys:continuationMarker:zoneID:zoneWide:numbersAsStrings:database:)	The primitive; builds the request body
queryAllRecords(…)	Forwards zoneID on every page it fetches
fetchExistingRecordNames(…)	So a pre-fetch targets the same zone as the modifyRecords(_:zoneID:) that follows it in the classify workflow

RecordManaging.queryAllRecords(recordType:) is intentionally left zone-unaware — that protocol is a database-agnostic abstraction. Callers needing a zone should use CloudKitService directly.

Note: The deprecated queryRecords(recordType:…) overloads were removed by #424; this PR only threads zoneID through the Query primitive and its paginating helpers.

Note:QueryRecordsPhase moving to queryAllRecords(recordType:database: context.database) is a #424 change, not part of this PR's zone work.

No OpenAPI regeneration needed

Components.Schemas.ZoneID already models both zoneName and ownerName, and the records/query request body already $refs it. openapi.yaml and Sources/MistKitOpenAPI/ are untouched.

Source compatibility

Source-compatible. Every new parameter is defaulted, and each was inserted before the non-defaulted database:, so existing call sites keep compiling unchanged.

One behavioral change worth flagging: requests previously always carried "zoneID": {"zoneName": "_defaultZone"}; they now omit the key when no zone is given. CloudKit resolves the same default zone either way, so this is wire-level-different but semantically equivalent. The queryOmitsZoneIDByDefault test pins the new behavior.

Tests

Six new @Tests in Tests/MistKitTests/CloudKitService/Query/CloudKitServiceTests.Query+ZoneID.swift (Zone Selection suite). They assert against the serialized request body captured by MockTransport:

• zoneID omitted when none is supplied (default-zone behavior unchanged)
• custom zoneName forwarded, with no stray ownerName
• shared-zone ownerName forwarded
• ZoneID.defaultZone forwarded explicitly when asked
• queryAllRecords forwards the zone on both pages of a paginated run
• fetchExistingRecordNames forwards the zone

MistDemo web server: two new tests reject zoneOwner without zoneName (400) and verify both zone fields forward to the backend.

MistDemo integration (test-private): new CustomZoneQueryPhase creates a custom zone, writes two records, queries them back with zoneID:, verifies the default-zone query does not see them, and deletes the zone inline.

Verification

Check	Result
swift build	Pass
swift test	Pass — 595 tests, 0 failures
Examples/MistDemoswift build && swift test	Pass — WebServerTests incl. zone validation
./Scripts/lint.sh	Pass — SwiftLint 0 serious violations

Docs

• AGENTS.md (CLAUDE.md symlink): "Zone selection on queries (Feature: Add custom CloudKit zone support for queries #146)" section (updated post-Remove deprecated API, model server error codes, refactor FieldValue conversion, add cloud toolchain #424).
• WorkingWithRecords.md: "Querying a custom or shared zone" subsection.
• DocC symbol links updated for zoneID: across .docc articles and Query.swift.

Closes#146

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d479086e-04c3-4e81-9f81-ea35078086bb

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

Review

Reviewed the diff (gh pr diff 426) and cross-checked the touched files/tests against the current tree. This is a clean, well-scoped fix.

Code quality & design

• The core fix is minimal and correct: zoneID: .init(zoneName: "_defaultZone") → zoneID: zoneID.map { Components.Schemas.ZoneID(from: $0) } (CloudKitService+Operations.swift:87), reusing the existing Components.Schemas.ZoneID(from:) conversion already proven by modifyRecords.
• Choosing ZoneID over a bare zoneName: String is the right call — a string-only parameter would have left shared zones (which need ownerName) unsolved, which is half the issue title.
• New zoneID parameters are consistently inserted right before the non-defaulted database: in every affected signature (queryRecords primitive + 2 deprecated overloads, queryAllRecords, fetchExistingRecordNames), which is exactly what keeps this source-compatible for all labeled call sites.
• Docs (AGENTS.md, 5 DocC articles, WorkingWithRecords.md) were updated in the same PR rather than drifting — including the DocC symbol-link signatures, which is easy to forget and often missed.
• The .claude/memory/ additions are appropriately scoped (worktree layout, MistDemoApp fails to build: NoteEditView/RecordDetailView have no accessible initializers (Swift 6.3) #419 status) and don't leak unrelated content into this PR's diff footprint.

Potential bugs / edge cases

• No client-side interaction check between zoneID and zoneWide (docs note they "pull in opposite directions" but nothing throws or warns if both are set). Confirmed this isn't a regression — zoneWide had no such validation before this PR either — so it's a pre-existing gap this PR merely inherits, not introduces. Worth a follow-up issue if you want CloudKit misconfigurations caught client-side rather than server-side.
• RecordManaging.queryRecords(recordType:) / queryAllRecords(recordType:) intentionally stay zone-unaware. Reasonable given that protocol is already documented as deprecated and database-agnostic, but worth double-checking no current caller actually needs zone selection through that protocol before the next deprecation pass removes it.

Performance

No concerns — this only changes what gets serialized into an existing request body; no new allocations, requests, or pagination behavior beyond forwarding zoneID per page (correctly verified by queryAllRecordsForwardsZoneIDPerPage).

Security

No concerns. ownerName/zoneName are passed through unchanged (no new string interpolation into URLs or logging paths), and the change doesn't touch auth/credential handling.

Test coverage

The new Zone Selection suite (CloudKitServiceTests.Query+ZoneID.swift) is solid and asserts against the actual serialized request body rather than mocking away the interesting part:

• omission when nil (pins the "no silent default" behavior change called out in the PR description)
• custom zone name forwarded, no stray ownerName
• shared-zone ownerName forwarded
• ZoneID.defaultZone forwarded explicitly
• queryAllRecords forwards zoneID on every page (verified against both calls, not just the first)
• fetchExistingRecordNames forwards zoneID

I did not find a test for the zoneWide+zoneID combination, but given there's no code path that treats them specially, that's consistent with the rest of the coverage rather than a gap introduced here.

Nit

The PR description states the wire-level behavior change (dropping the always-present zoneID key when unset) is "semantically equivalent" per CloudKit — reasonable, but since this is the one behavioral (non-additive) change in an otherwise source-compatible PR, it might be worth a one-line mention in a CHANGELOG/release notes entry if this repo tracks one for the beta.4 milestone.

Overall: solid, well-tested, well-documented fix that matches established codebase conventions (ZoneID reuse, optional-defaulted request-option parameter pattern, docs-in-same-PR). No blocking issues found.

[claude]

Review: PR #426 — Custom/shared zone support for the query path

Nice, focused change with strong justification for the design calls. Went through the diff plus the surrounding codebase for consistency.

Code quality / correctness

• zoneID.map { Components.Schemas.ZoneID(from: $0) } in CloudKitService+Operations.swift:90 correctly replaces the hard-coded .init(zoneName: "_defaultZone") and matches the same conversion pattern already used in modifyRecords, uploadAssets, fetchZoneChanges, etc. — good consistency.
• Parameter placement (zoneID inserted before zoneWide/numbersAsStrings, and before the non-defaulted database:) preserves source compatibility for existing labeled call sites, and threading through queryAllRecords, the deprecated recordType: overloads, and fetchExistingRecordNames is complete — no caller of the query path appears to have been missed.
• ZoneID vs. a bare zoneName: String is the right call: ownerName is required to address shared zones, and it mirrors modifyRecords(_:zoneID:)'s existing convention rather than inventing a second zone-naming scheme.
• The nil = "omit the key" semantics (rather than a client-side default of _defaultZone) is a reasonable read of feedback_no_silent_policy_defaults.md — this isn't credential/attribution policy, and it matches the existing optional-field convention on zoneWide/numbersAsStrings/desiredKeys.

Potential issues

• zoneID + zoneWide: true interaction is undocumented-in-code. The docs note they "pull in opposite directions," but nothing prevents a caller from passing both, and there's no test pinning what CloudKit actually does when both are present in the same request (silently ignore zoneID? error?). Not a blocker, but worth either a doc-only clarification test or a note if this is unverified against the live API.
• Scope of the diff: the two new .claude/memory/*.md files (project_419_fixed_in_beta3.md, project_beta4_worktree_layout.md) and their MEMORY.md index entries are unrelated to the zone-selection issue — they're process/worktree notes about a different issue and the beta.4 branch layout. Bundling them into this PR adds unrelated diff noise for reviewers; consider splitting agent-memory housekeeping into its own PR/commit going forward.

Test coverage
The new Zone Selection suite (CloudKitServiceTests.Query+ZoneID.swift) is thorough for what it covers: omission when nil, custom zone name (and that no stray ownerName leaks in), shared zone ownerName forwarding, explicit ZoneID.defaultZone, per-page forwarding through queryAllRecords's pagination, and fetchExistingRecordNames forwarding. All assert against the actual serialized request body via MockTransport, which is the right level (catches wire-format regressions, not just that a value was "passed somewhere"). One gap: no test for the zoneID+zoneWide combination noted above, and no negative/error-path test — though that's arguably CloudKit's problem, not MistKit's, since this is pure request-shaping.

Security / performance
No concerns — this only changes what's serialized into an already-authenticated request; no new attack surface, no perf-sensitive path touched (one optional field, no extra allocations beyond the existing .map).

Docs
AGENTS.md/CLAUDE.md and WorkingWithRecords.md updates are clear and include both the custom-zone and shared-zone examples called out in the originating issue. DocC symbol links across the five .docc articles were updated for the new signature — good catch, easy to miss.

Overall: solid, well-scoped implementation matching established codebase conventions. The memory-file scope creep is the only thing worth asking about before merging.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (v1.0.0-beta.4@9e6b9d6). Learn more about missing BASE report.

Additional details and impacted files

@@ Coverage Diff @@## v1.0.0-beta.4 #426 +/- ##
================================================
Coverage ? 80.60% ================================================
Files ? 177 Lines ? 4366 Branches ? 0 ================================================
Hits ? 3519 Misses ? 847 Partials ? 0 

Flag	Coverage Δ
mistdemo-spm-macos	11.54% <0.00%> (?)
mistdemo-swift-6.2-jammy	11.54% <0.00%> (?)
mistdemo-swift-6.2-noble	11.54% <0.00%> (?)
mistdemo-swift-6.3-jammy	11.54% <0.00%> (?)
mistdemo-swift-6.3-noble	11.54% <0.00%> (?)
mistdemo-swift-6.4-jammy	11.66% <0.00%> (?)
mistdemo-swift-6.4-noble	11.54% <0.00%> (?)
spm	78.95% <100.00%> (?)
swift-6.1-jammy	78.85% <100.00%> (?)
swift-6.1-noble	79.08% <100.00%> (?)
swift-6.2-jammy	78.96% <100.00%> (?)
swift-6.2-noble	78.96% <100.00%> (?)
swift-6.3-jammy	79.06% <100.00%> (?)
swift-6.3-noble	78.92% <100.00%> (?)
swift-6.4-jammy	78.85% <100.00%> (?)
swift-6.4-noble	78.94% <100.00%> (?)

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.

[claude]

Review: Add custom/shared zone support to the query path (#146)

Reviewed against CLAUDE.md/AGENTS.md conventions. Overall this is a well-scoped, well-documented change that follows the codebase's existing patterns closely.

Strengths

• Consistent with modifyRecords: queryRecords's new zoneID: ZoneID? = nil (CloudKitService+Operations.swift:90) uses the exact same zoneID.map { Components.Schemas.ZoneID(from: $0) } pattern and nil-omission semantics as the existing modifyRecords(_:atomic:zoneID:...) (CloudKitService+WriteOperations.swift:111). Good API consistency.
• ZoneID conversion is clean: Components.Schemas.ZoneID.init(from:) (ZoneID.swift:57-64) is a straightforward 1:1 field mapping — no swaps, no force-unwraps, no truncation.
• Pagination threading verified correct: queryAllRecords (CloudKitService+QueryPagination.swift) re-passes zoneID from the outer parameter on every page (not stale/hardcoded), and the stuck-marker/continuation-marker logic is untouched.
• Solid new test suite: CloudKitServiceTests.Query+ZoneID.swift covers omit-by-default, custom zone, shared-zone ownerName, explicit ZoneID.defaultZone, per-page forwarding in queryAllRecords, and fetchExistingRecordNames forwarding — a good spread against the serialized request body rather than just the Swift call site.
• Source-compatible: new params are defaulted and inserted before non-defaulted database:, matching CLAUDE.md's requirement that database: never gets a default.
• Docs (AGENTS.md, DocC articles) are thoughtfully updated and explain the design decisions (ZoneID vs bare zoneName, why nil isn't a "silent policy default" per the repo's own memory convention).

Issues

1. Silent drop of zoneOwner in the MistDemo web backend (Examples/MistDemo/Sources/MistDemoKit/Server/CloudKitService+WebBackend.swift:45-47):

   letzoneID= zoneName.map{ZoneID(zoneName: $0, ownerName: zoneOwner)}

   If a web API caller sends zoneOwner but omits zoneName, the whole zoneID becomes nil and zoneOwner is silently discarded — the query quietly falls back to the default zone instead of failing loudly. There's no cross-field validation anywhere in WebRequests.swift/WebServer+CRUD.swift to catch this. Given the rest of this codebase's "fail loud" philosophy (see the ConversionError.typeValueMismatch pattern in CLAUDE.md), I'd expect this to at least throw a clear error rather than silently mis-resolve the zone.

2. CLI/web inconsistency for the same edge case: QueryCommand (Commands/QueryCommand.swift:85) always builds an explicit ZoneID(zoneName: config.zone, ownerName: config.zoneOwner) because QueryConfig.zone defaults to "_defaultZone" (non-optional) rather than nil. Two consequences:

   • The MistDemo CLI can never exercise the "omit the zoneID key" wire behavior this PR introduces at the library level — every CLI-issued query now always sends an explicit zoneID, which is the opposite of what the PR description highlights as the behavioral change.
   • --zone-owner without --zone behaves differently than the same input on the web backend: the CLI applies it against _defaultZone (a zone that doesn't have a meaningful "owner") instead of dropping it. Worth reconciling, or at least documenting the intended semantics for both surfaces.

3. Test coverage gap on the web HTTP path: the existing queryForwards test in WebServerTests+CRUD.swift (POST /api/records/query forwards to the backend) wasn't updated to include zoneName/zoneOwner in its JSON body or assertions. The new WebRequests.Query.zoneName/.zoneOwner decode wiring is therefore only exercised indirectly (CLI-level QueryConfig tests, and MockBackend's pass-through capture, which bypasses real JSON decoding). A test mirroring the existing recordsChangesForwards test (which does assert zone decode-through-HTTP) would close this gap and would be a natural place to also cover/lock in the zoneOwner-without-zoneName behavior from point 1.

Minor

• The diff also carries unrelated .claude/memory/* housekeeping (new project_419_fixed_in_beta3.md, project_beta4_worktree_layout.md, MEMORY.md index entries). Not wrong, but orthogonal to the zone-query feature — would read cleaner split out, consistent with the repo's own "unrelated changes stay isolated" convention.

Performance / Security

No concerns on either front. The added parameter is optional and zero-cost when omitted, and zone name/owner are opaque identifiers passed straight through to CloudKit with no new attack surface (same handling as the existing modifyRecords zone parameter).

🤖 Generated with Claude Code

[claude]

Review: #146 — custom/shared zone support for the query path

Overall this is a clean, well-scoped change. The zoneID: ZoneID? = nil threading mirrors the existing modifyRecords(_:zoneID:) precedent, the doc comments explain the "omit key vs. explicit default" distinction clearly, and the six new Zone Selection tests assert against the actual serialized request body rather than just return values — good coverage for the specific regression (custom/shared zones being hard-coded out).

Code quality / correctness

• Sources/MistKit/CloudKitService/CloudKitService+Operations.swift:90 — zoneID.map { Components.Schemas.ZoneID(from: $0) } is exactly the right pattern (omit key vs. force _defaultZone), and it's consistent with how modifyRecords already does zone selection. 👍
• All four call sites (queryRecords primitive, deprecated recordType: overloads, queryAllRecords, fetchExistingRecordNames) got the parameter threaded consistently, and queryAllRecords correctly forwards zoneID on every page (verified by queryAllRecordsForwardsZoneIDPerPage).
• RecordManaging.queryRecords(recordType:) intentionally staying zone-unaware is reasonable given it's already a deprecated, database-agnostic abstraction — documented well in AGENTS.md.

Minor observations (non-blocking)

• Examples/MistDemo/Sources/MistDemoKit/Commands/QueryCommand.swift:85 — let zoneID = ZoneID(zoneName: config.zone, ownerName: config.zoneOwner) is built unconditionally (since config.zone defaults to "_defaultZone"). That means the CLI always sends an explicit zoneID in the request body and never exercises the new "omit the key" path that the library-level change specifically added. Functionally equivalent (per queryForwardsExplicitDefaultZone), but it means MistDemo doesn't dogfood the omission behavior. Could special-case config.zone == "_defaultZone" && config.zoneOwner == nil → nil, if that's worth the extra branch.
• Examples/MistDemo/Sources/MistDemoKit/Server/CloudKitService+WebBackend.swift:45-47 — zoneOwner is silently dropped if zoneName is nil but zoneOwner is present (zoneName.map { ZoneID(zoneName: $0, ownerName: zoneOwner) }). An owner-without-a-zone-name request is admittedly nonsensical, so silently ignoring it is probably fine, but it's not validated/rejected either — a caller who typos the zone-name field would get a silent full-database-default-zone query instead of an error.
• The web demo UI (Resources/js/app.jsqueryNotes() → /api/records/query) doesn't send zoneName/zoneOwner even though the backend now accepts them — so zone selection is only reachable via the CLI, not the web panel. Presumably out of scope for this PR, but worth a follow-up issue if the web UI is meant to expose the same surface as the CLI.
• Examples/MistDemo/Sources/MistDemoKit/Integration/Phases/QueryRecordsPhase.swift:47-50 swaps the deprecated queryRecords(recordType:) (which hard-codes .public(.prefers(.serverToServer)) per RecordManaging) for the primitive with database: context.database. This is a real behavior change (the phase now queries whatever database the integration run targets, not always public) bundled into a zone-selection PR. Looks like the right fix and is presumably needed for test-private to make sense, but it's worth calling out explicitly in the PR description/commit message since it's not mentioned there.

Test coverage

Good coverage on the MistKit side (body-level assertions for omission, custom zone, shared-zone ownerName, explicit .defaultZone, per-page forwarding in queryAllRecords, and fetchExistingRecordNames). MistDemo-side tests cover config parsing/defaults for zoneOwner. No test exercises the CloudKitService+WebBackend.webQuery zone-owner-without-zone-name edge case mentioned above, but that's a minor gap given the edge case is arguably a non-issue.

Security / performance

No concerns — this only threads an optional identifier into an existing JSON request body; no new external input parsing beyond the existing Codable decode path, and no hot-path performance impact.

Nice work — approving from a code standpoint modulo the minor nits above.

🤖 Generated with Claude Code

[leogdion]

Review/merge order note

Proposed order for the v1.0.0-beta.4 queue: #425 → #424 → #427 → #426 → #429 → #428

This PR should go after #424. It's small (+334/−19), but it carries the one hard conflict with the deprecated-API removal.

Blocker: rebase onto #424 first

This PR adds +6 lines to Sources/MistKit/CloudKitService/CloudKitService+Operations+Deprecated.swift — a file #424 deletes outright (−159). That hunk has to move on rebase, presumably into CloudKitService+Operations.swift. Doing it in this direction is much cheaper than teaching #424 to delete a file this branch just extended.

Three other files overlap with #424 and want the same treatment:

File	#424	this PR
CloudKitService+Classification.swift	+1/−1	+6/−0
Integration/Phases/QueryRecordsPhase.swift	+7/−6	+4/−2
Server/CloudKitService+WebBackend.swift	+1/−3	+9/−3

Note that #424 rewrites the QueryRecordsPhase / CloudKitService+WebBackend call sites specifically to drop the removed deprecated overloads. This PR rewrites the same call sites to thread zoneID through. Reconcile those by hand rather than taking either side wholesale.

Testing

It also adds a requirement to Server/WebBackend.swift and MockBackend+RecordOperations.swift, which #425/#428/#429 each extend independently — run the MistDemo suite after the rebase, not only the MistKit one.

The design rationale in the description (ZoneID over zoneName: String for ownerName; = nil meaning "omit the key" rather than "substitute _defaultZone") both look right to me and are worth preserving verbatim in the merge commit — they're the kind of call that gets re-litigated later.

[leogdion]

Re: review comment #426 (comment)

Rebased onto current v1.0.0-beta.4 (#424 included) and addressed the review items:

Applied

• zoneOwner without zoneName → 400: validated in WebRequests.Query.init(from:); covered by WebServerTests+QueryZone (queryRejectsZoneOwnerWithoutZoneName + positive forward test).
• QueryRecordsPhase database change: that move to queryAllRecords(recordType:database: context.database) is from Remove deprecated API, model server error codes, refactor FieldValue conversion, add cloud toolchain #424, not this PR — PR description updated to say so.
• Integration coverage: new CustomZoneQueryPhase in test-private — creates a custom zone, writes records, queries with zoneID:, asserts default-zone isolation, inline cleanup.

Considered, left as-is

• CLI always sends explicit zoneID:QueryCommand still builds ZoneID(zoneName:ownerName:) unconditionally. Omit-key behavior is covered by queryOmitsZoneIDByDefault and the integration phase's default-zone isolation assertion.

Follow-up (out of scope for this PR)

• Web UI zone inputs: filed MistDemo web UI: add zoneName/zoneOwner inputs to query panel #438 — backend already accepts zoneName/zoneOwner; app.jsqueryNotes() doesn't send them yet.