Push Notifications & Subscriptions epic (#379)

[leogdion]

Summary

Delivers the full server-side push-notification workflow for CloudKit Web Services — subscriptions (the triggers) plus APNs tokens (the delivery path) — closing epic #379 and sub-issues #49, #50, #51, #52, #53.

• Subscriptions (subscriptions/list, /lookup, /modify): new CloudKitService methods, CLI commands (list-subscriptions, lookup-subscription, modify-subscriptions), SubscriptionRoundtripPhase in the live runner. Working live since the first commit on this branch.
• APNs tokens (tokens/create, tokens/register): new CloudKitService methods, CLI commands (create-token, register-token), TokenRoundtripPhase. The endpoint route turned out to be /device/1/… (CloudKit JS uses setApiModuleName("device")), not /database/1/… as Apple's archived REST reference documents — that detail was the missing piece that resolved the persistent HTTP 405 on every server-side call. The clientId field CloudKit JS always sends is now part of both bodies, exposed as an optional caller-supplied parameter (defaults to a fresh UUID per call).
• Verbose logging: --verbose on test-private / test-public now bootstraps LoggingSystem so MistKit's existing LoggingMiddleware actually emits the wire trace — necessary for diagnosing the 405 and useful for anyone reverse-engineering CloudKit JS in the future.

Live verification

swift run mistdemo test-private --verbose against iCloud.com.brightdigit.MistDemo — all 17 phases green, including:

• Phase 15 SubscriptionRoundtripPhase (create / list / lookup / delete)
• Phase 16 TokenRoundtripPhase (POST /device/1/.../tokens/create → 200, then tokens/register → 200)

test-public --verbose blocked on an unrelated pre-existing config bug (S2S key path has escaped backslashes — com\\~apple\\~CloudDocs instead of com~apple~CloudDocs). Filing as a separate follow-up; not in scope here.

Test plan

• swift test — 498/498 MistKit unit tests
• cd Examples/MistDemo && swift test — 941/941 MistDemo unit tests
• ./Scripts/lint.sh — clean (only 4 pre-existing warnings unrelated to this branch)
• swift run mistdemo test-private --verbose — 17/17 phases green live
• CI on this PR
• (Reviewer) re-run test-private --verbose from a clean checkout if you want to see the wire trace

Known follow-ups (out of scope for this PR)

• Tighten tokens/register response spec — live server actually returns the same {apnsToken, apnsEnvironment, webcourierURL} body as tokens/create, not the empty 200 our spec assumes. Behaviorally harmless (we ignore the body) but accurate.
• Doc-comment cleanup — CloudKitService+TokenOperations.swift still cites Apple's archived RegisterTokens.html as the source of truth; CloudKit JS is the actual authority now.
• Wire-format unit test — MockTransport doesn't capture the request body, so we can't assert clientId lands in the JSON from unit tests. A capturing transport variant would let us regression-pin the wire shape.
• Resources/js/tokens.js doesn't yet surface a clientId input. Backend accepts requests without it (auto-UUID), so the panel works; adding a field is pure UX polish.
• S2S key-path escape bug in test-public config loading.

Closes #49, #50, #51, #52, #53, #379.

[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

Run ID: e8bedf13-79ec-48e0-98ae-e45a4022b0fd

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 379-push-notifications-subscriptions-epic

---

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 and usage tips.}

[claude]

Code Review: Push Notifications & Subscriptions epic (#379)

Overall this is a solid, well-structured implementation that follows the project's established patterns faithfully. The architecture mirrors CloudKitService+ZoneOperations.swift and friends cleanly, the ContainerOperationInputPath abstraction is clever and minimal, the /device/ URL discovery is well-documented, and the fail-loud conversion philosophy from the rest of the codebase is correctly extended to subscription/token types. The integration test suite in SubscriptionRoundtripPhase + TokenRoundtripPhase is nicely self-cleaning. Minor and moderate items below.

---

Moderate

1. Web server silently forwards empty strings for missing subscription fields
WebRequests+Subscriptions.swift:105-107:

subscriptionID: input.subscriptionID ?? "",
recordType: input.query?.recordType ?? "",
zoneID: ZoneID(zoneName: input.zoneID?.zoneName ?? "")

If the browser omits subscriptionID, recordType, or zoneName, the server sends an empty string to CloudKit rather than returning 400. CloudKit then decides whether to reject it or create a broken subscription. The SubscriptionCommandError types for the CLI side validate these, but the web path skips validation entirely. Prefer decoding these with decode (non-optional) or adding an explicit guard before calling backend.webModifySubscriptions, returning .badRequest to the client.

2. WebRequests.RegisterToken.apnsToken defaults to ""
WebRequests+Tokens.swift:85-86:

self.apnsToken =
  try container.decodeIfPresent(String.self, forKey: .apnsToken) ?? ""

A missing apnsToken in the request body becomes "", which passes decoding but hits CloudKitService.registerAPNsToken's empty-string guard and throws CloudKitError.badRequest. That error surfaces from runOperation as a 500 (internal server error) rather than a 400 to the caller. Either decode with decode (required field) or validate before the service call and return .badRequest.

3. registerRejectsEmpty test uses an overly broad error match
CloudKitServiceTests.Tokens+SuccessCases.swift:85:

await #expect(throws: CloudKitError.self) {

This catches any CloudKitError, including .invalidResponse or .conversionFailed. Narrowing to .badRequest(reason:) (or at least checking the error via #require + a pattern match) would prevent a future refactor from silently changing the error kind while the test still passes.

---

Minor / Informational

4. APNsEnvironment has two near-identical init(from:) extensions
APNsEnvironment.swift:46-66: The two extensions for createToken and registerToken payload types share identical switch bodies. This is unavoidable given the generated types, but a brief comment noting the duplication is structural (different generated namespaces, same semantics) would help future readers avoid mistaking it for an oversight.

5. schema computed properties are internal var instead of let
SubscriptionInfo.swift:56 and SubscriptionQuery.swift:41: Both are internal var on value types but are never mutated after initialization. Declaring them let (or internal var { get } in a protocol context) would signal immutability at the source.

6. Public memberwise init allows structurally invalid SubscriptionInfo
SubscriptionInfo.swift:67: The public init(subscriptionID:subscriptionType:query:zoneID:firesOn:) allows callers to create a .query type with no query payload, or .zone with no zoneID. The static factory methods .query(...) and .zone(...) are the intended construction surface. Making the memberwise init internal and only exposing the factories would prevent accidental invalid construction from library consumers.

7. clientId wire format not unit-tested (acknowledged)
The PR description notes this correctly — the current MockTransport doesn't capture request bodies, so there's no unit test pinning clientId to the wire JSON. Just confirming this is visible: when a capturing transport variant lands, a regression test for the clientId presence in both request bodies would be valuable.

8. AtomicBool custom implementation is appropriate
MistDemoLoggingBootstrap.swift:59-71: The NSLock-based flag is clean and the comment explaining why Swift Atomics isn't pulled in is helpful. No concern here — it's a one-shot flag and the implementation is correct.

---

Nit

CloudKitService+SubscriptionOperations.swift:37: The listSubscriptions doc-comment references createAPNsToken(environment:database:) (omitting clientId: from the label), which won't resolve as a symbol link now that clientId is part of the method signature. Update to createAPNsToken(environment:clientId:database:).

---

Overall the PR is in good shape. The two web-server input-validation issues (#1 and #2) are the most worth fixing before merge since they result in confusing errors for the browser demo; the rest are polish items.

[codecov]

Codecov Report

❌ Patch coverage is 64.31015% with 313 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (v1.0.0-beta.2@3452060). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...oudKitService/CloudKitError+ErrorDescription.swift	23.77%	109 Missing ⚠️
...istKit/Models/Subscriptions/NotificationInfo.swift	0.00%	37 Missing ⚠️
...istKit/Models/Notifications/WebCourierPoller.swift	0.00%	34 Missing ⚠️
Sources/MistKit/Models/Notifications/Courier.swift	20.00%	20 Missing ⚠️
...odels/Subscriptions/SubscriptionInfo+Codable.swift	58.69%	19 Missing ⚠️
.../Models/Subscriptions/SubscriptionFireEvents.swift	57.89%	16 Missing ⚠️
.../Models/Notifications/URLSession+CourierPoll.swift	0.00%	10 Missing ⚠️
...istKit/Models/Subscriptions/SubscriptionInfo.swift	84.61%	8 Missing ⚠️
Sources/MistKit/Models/RecordTarget.swift	58.82%	7 Missing ⚠️
...erations/Operations.listSubscriptions.Output.swift	0.00%	7 Missing ⚠️
... and 14 more

Additional details and impacted files

@@               Coverage Diff                @@
##             v1.0.0-beta.2     #381   +/-   ##
================================================
  Coverage                 ?   68.51%           
================================================
  Files                    ?      162           
  Lines                    ?     3725           
  Branches                 ?        0           
================================================
  Hits                     ?     2552           
  Misses                   ?     1173           
  Partials                 ?        0           

Flag	Coverage Δ
mistdemo-spm-macos	10.95% <6.95%> (?)
mistdemo-swift-6.2-jammy	10.95% <6.96%> (?)
mistdemo-swift-6.2-noble	10.95% <6.96%> (?)
mistdemo-swift-6.3-jammy	10.95% <6.96%> (?)
mistdemo-swift-6.3-noble	10.95% <6.96%> (?)
spm	68.87% <62.94%> (?)
swift-6.1-jammy	68.80% <63.01%> (?)
swift-6.1-noble	68.94% <63.01%> (?)
swift-6.2-jammy	68.69% <63.01%> (?)
swift-6.2-noble	68.86% <63.01%> (?)
swift-6.3-jammy	68.86% <63.01%> (?)
swift-6.3-noble	68.72% <63.01%> (?)

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

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

[leogdion]

we can delete this

[leogdion]

Deleted in 9806678.

[leogdion]

rather then using URLSession it should use the same client as the MistKit client. See how Client transport work

[leogdion]

Deliberately not the CloudKit ClientTransport here: the webcourierURL is a different host from api.apple-cloudkit.com, so reusing that transport's HTTP/2 connection pool across the two hosts risks 421 Misdirected Request — the same hazard documented in CLAUDE.md ("Asset Upload Transport Design") that makes MistKit upload assets through a separate AssetUploader closure instead of the shared transport.

To address the underlying intent (don't hard-code URLSession), 9806678 makes the transport an injectable @Sendable closure that defaults to the dedicated ephemeral URLSession. That keeps host/connection-pool separation while making the poller testable without a live courier. Open to revisiting if we later add a transport abstraction that pools per-host.

[claude]

Code Review: Push Notifications & Subscriptions Epic (#379)

A large, well-scoped PR that completes the server-side push-notification surface for CloudKit Web Services. The key discovery — that the tokens/ endpoints live under /device/1/…, not /database/1/… — is well documented and resolves a real API mystery. Overall the code quality is high. Notes below are in priority order.

---

Correctness

createSubscription — order of checks is wrong (bug)

guard let created = results.first else {
  throw CloudKitError.invalidResponse
}
if case .failure(let failure) = created, failure.isLikelyDuplicate {
  throw CloudKitError.subscriptionLikelyDuplicate(failure)
}
return try created.get()   // ← throws subscriptionOperationFailed for non-duplicate failures

If modifySubscriptions returns a non-duplicate per-subscription failure, created.get() throws a ConversionError (via OperationResult.get()) that is not caught by the surrounding do/catch in modifySubscriptions — it propagates out of createSubscription as an uncaught error rather than being mapped through mapToCloudKitError. The pattern used elsewhere (do { … } catch { throw mapToCloudKitError(error, …) }) is missing here. Consider either wrapping the whole body or calling throw CloudKitError.subscriptionOperationFailed(failure) explicitly before the get().

deleteSubscription — empty results treated as success on any path

let results = try await modifySubscriptions([.delete(subscriptionID: id)], database: database)
for result in results { _ = try result.get() }

modifySubscriptions already skips deletion acknowledgement entries (guard subscription.subscriptionType != nil). If CloudKit returns a non-empty response that is entirely made of ack entries, results is empty and the delete silently succeeds even if the subscription didn't exist. That's arguably fine — idempotent deletes are a common choice — but it should be a conscious decision, not an accidental one. Worth a comment.

SubscriptionInfo.init(subscriptionID:kind:notificationInfo:) — precondition trap vs. thrown error

The precondition(!firesOn.isEmpty, …) path causes a hard crash in release builds when a caller passes an empty SubscriptionFireEvents. The schema decoder and Codable path already throw ConversionError.subscriptionQueryMissingFiresOn for the same case on ingress — so the defensive check on the public factory is sound, but crashing may surprise API consumers. Consider precondition only in DEBUG or convert to a throwing factory (static func query(…) throws) for consistency with the decoder path.

---

Design

modifySubscriptions return type loses deletion results

The doc comment says "deletions are typically omitted" but actually they are always silently skipped by the nil-returning OperationResult.init?. If CloudKit ever returns a per-subscription error for a delete operation (e.g., deleting a non-existent subscription), that error is swallowed. The deleteSubscription(id:) convenience wrapper would silently succeed on a not-found delete. At minimum, document that per-subscription failures on .delete operations are suppressed.

clientId param on registerAPNsToken can diverge from createAPNsToken

The docs say "reuse the value passed to createAPNsToken" but this is unenforced. A caller can accidentally pass a different UUID and silently attribute token registration to a different logical client. The PR notes this in the doc comment, which is good. For a future enhancement, a ClientID struct wrapping the resolved UUID (returned by createAPNsToken and required by registerAPNsToken) would make it impossible to diverge at the call site without an explicit cast.

---

Code Quality

SwiftLint rule removal scope is too broad

.swiftlint.yml removes discouraged_optional_boolean project-wide. The motivation is the firesOnce: Bool? field where nil means "don't send the field; let CloudKit apply its default". That's a legitimate use of Bool?, but the project-wide removal allows Bool? everywhere — including places where an Optional<Bool> is genuinely ambiguous. The two swiftlint:disable:next inline suppressions in SubscriptionInfo+Schema.swift were the right approach; the opt-in rule removal should be reverted or replaced with a path-scoped exclusion targeting the Subscriptions files.

APNsTokenResult.init(from:) doc comment references wrong method name

The doc comment at line 62 references CloudKitService/createAPNsToken(environment:database:) but the current method signature has three parameters: createAPNsToken(environment:clientId:database:). A minor doc rot, but easy to fix since it's on a new type.

SubscriptionInfo firesOn accessor returns [] for zone/database kinds

public var firesOn: SubscriptionFireEvents {
  if case .query(_, let firesOn, _) = kind { return firesOn }
  return []
}

Returning [] for non-query subscriptions is consistent with "zone/database subscriptions fire on any change", but an empty SubscriptionFireEvents is also the value the precondition guards against on .query. A caller iterating over subscriptions and checking sub.firesOn.isEmpty can't distinguish "this is a zone subscription" from "this query subscription is misconfigured". Consider returning nil (with var firesOn: SubscriptionFireEvents?) or .all for zone/database, and documenting the distinction.

---

Test Coverage

No test covers createSubscription non-duplicate failure path

The failure test matrix (+FailureCases.swift) covers the isLikelyDuplicate == true path and the general failure case in modifySubscriptions. There is no test for the case where createSubscription receives a non-duplicate per-subscription failure — which is also where the error-mapping gap noted above would surface.

Token tests lack wire-format assertions

The PR notes this in the known follow-ups: MockTransport doesn't capture the request body, so clientId in the serialized JSON can't be asserted. This is an acceptable gap for an initial implementation, but worth tracking. The workaround of verifying the live roundtrip via test-private --verbose is good, but a unit test with a capturing transport would lock down resolveClientId's behavior (nil → fresh UUID, empty → throw) in a way that survives transport refactors.

No test for SubscriptionInfo.init(subscriptionID:kind:notificationInfo:) precondition

If the precondition is changed to a throw in a future refactor, there's no test asserting it fires. Tests use the factory methods (SubscriptionInfo.query(…)) which don't exercise the direct init path.

---

Known Follow-ups (acknowledged in PR)

The PR description already calls out several of these — registering them here for completeness:

• tokens/register response body is ignored (live server sends a body our spec treats as empty 200).
• CloudKitService+TokenOperations.swift doc still cites Apple's archived REST reference; CloudKit JS is the actual authority.
• probe-duplicate-subscription command is a good operational tool — consider wiring it to a structured output that can be diff'd in CI.
• S2S key-path escape bug in test-public config loading is pre-existing but should be filed/linked.

---

Summary

The core subscription and token implementations are correct and well-structured. The main actionable items before merging:

1. Fix the error-mapping gap in createSubscription so non-duplicate per-subscription failures go through mapToCloudKitError.
2. Revert (or scope) the discouraged_optional_boolean rule removal — the inline suppressions in SubscriptionInfo+Schema.swift were the right approach.
3. Add a test for the non-duplicate failure path in createSubscription.
4. Fix the doc comment method reference in APNsTokenResult.init(from:).

Everything else is lower priority. The live verification story (17/17 phases green) and the existing 498 + 941 unit tests give good confidence in the implementation.

🤖 Generated with Claude Code

[leogdion]

why not make this a let

[claude]

Code Review: Push Notifications & Subscriptions epic (#379)

Overall: This is a well-architected addition. The OperationInputPath / ContainerOperationInputPath split correctly models the /device/1/… vs /database/1/… URL difference, the OperationResult<SubscriptionInfo, SubscriptionTarget> pattern is a clean reuse of the existing record machinery, and the Courier long-poll design is sound. All new types are Sendable, imports carry explicit access modifiers, and the test suite uses Swift Testing idioms correctly throughout.

A few items worth addressing before merge:

---

Bugs / Correctness

1. Stale DocC links in createSubscription — will resolve to nothing

CloudKitService+ModifySubscriptions.swift:77-78:

/// `subscription` with ``SubscriptionInfo/query(subscriptionID:recordType:filters:sortBy:firesOn:)``
/// or ``SubscriptionInfo/zone(subscriptionID:zoneID:firesOn:)``.

Both links are wrong:

• The query factory also has firesOnce:notificationInfo: parameters not listed.
• The zone factory's third parameter is notificationInfo:, not firesOn: — firesOn doesn't exist on .zone at all.

DocC will silently fail to resolve these. Correct links are:

/// ``SubscriptionInfo/query(subscriptionID:recordType:filters:sortBy:firesOn:firesOnce:notificationInfo:)``
/// or ``SubscriptionInfo/zone(subscriptionID:zoneID:notificationInfo:)``

---

Missing Test Coverage

2. Delete-acknowledgement path in modifySubscriptions is never covered

SubscriptionTarget.swift:94–97 returns nil from OperationResult.init?(from:) when CloudKit echoes a deleted subscription as a bare { subscriptionID } entry (no subscriptionType). This documented behaviour is intentionally exercised by compactMap in CloudKitService+ModifySubscriptions.swift:66-67 but no unit test exercises it.

The deleteSubscription test in CloudKitServiceTests.Subscriptions+SuccessCases.swift:121-124 uses { "subscriptions": [] } — an empty array — instead of { "subscriptions": [{"subscriptionID": "query-sub"}] } which is the actual CloudKit response shape. Adding a test that passes the real deletion-ack shape would cover this path.

3. SubscriptionInfo.Kind.database has zero test coverage

The .database kind is implemented in full (factory, SubscriptionInfo+Schema.swift with zoneWide: true, SubscriptionInfo+Codable.swift decode from zoneWide: true) but SubscriptionConversionTests.swift and the CloudKitServiceTests.Subscriptions suite have no cases for it. At minimum worth adding:

• Schema encode of SubscriptionInfo.database(subscriptionID:notificationInfo:) → verify zoneWide: true is set and no zoneID field appears.
• Codable round-trip: decode { "subscriptionType": "zone", "zoneWide": true, "subscriptionID": "…" } → .database.

4. Token operations have no failure-case tests

The CloudKitServiceTests.Tokens suite has a SuccessCases file but no FailureCases counterpart (compare with the subscription suite which has both). The registerAPNsToken empty-token rejection is tested, but HTTP 400 and 401 responses from processCreateTokenResponse / processRegisterTokenResponse are never exercised. Given that the token endpoint is on a non-standard module (/device/1/…) and was the source of the 405 mystery, a test confirming the error mapping is load-bearing here.

---

Design Concern (not a blocker)

5. Courier.pollOnce discards the HTTP status code

Courier.swift:95:

let (_, data) = try await transport(courierURL, perPollTimeout)
return try? CourierNotification(data: data)

The transport returns (statusCode: Int?, data: Data) but the status code is thrown away. A persistent 4xx from the courier (expired token, permissions change) would return nil each call — indistinguishable from an empty keepalive — and notifications() would loop forever, hammering the endpoint. Even a > 299 check that throws a typed error (or at least logs a warning) would surface this situation to the caller. The Transport typealias already surfaces statusCode, it just isn't used.

---

Minor / Nits

• CloudKitService+TokenOperations.swift cites Apple's archived RegisterTokens.html as the authoritative reference — the PR description notes this is already tracked as a follow-up.
• Resources/js/tokens.js has no clientId input field — also a tracked follow-up per the PR description.

---

Summary

Area	Status
Architecture (device vs database URL split)	✅ Correct
Concurrency / Sendable	✅ All correct
Error handling (subscriptions)	✅ Consistent with existing patterns
Error handling (tokens)	⚠️ Success-only test coverage
Doc links in createSubscription	❌ Both wrong — will break DocC resolution
Delete-ack path in compactMap	⚠️ No test for real CloudKit response shape
.database subscription kind	⚠️ No tests at all
Courier status code	⚠️ Silent failure on persistent HTTP errors

🤖 Generated with Claude Code

[claude]

Code Review — PR #381: Push Notifications & Subscriptions epic

Overview

This PR delivers the complete server-side push-notification workflow: subscriptions (list/lookup/modify) and APNs tokens (create/register), closing issues #49–53 and epic #379. The key non-obvious discovery — that tokens/create and tokens/register route through /device/1/… rather than /database/1/… — is well-documented and resolved a persistent HTTP 405. The implementation is live-verified with 17 integration phases green.

Files changed: 111, +8372/−900 lines.

---

Code Quality and Style ✅

Overall quality is high and patterns follow the codebase's conventions well:

• OperationFailure<Target: OperationFailureTarget> is a clean phantom-type refactor. RecordOperationFailure = OperationFailure<RecordTarget> and SubscriptionOperationFailure = OperationFailure<SubscriptionTarget> prevent mixing them in batch-result arrays at compile time. The .identifier rename propagates cleanly through ModifyCommand and BushelCloudKitService.
• WebCourierPoller offers a sensible three-level initializer hierarchy (raw transport → URLSession → configuration).
• AtomicBool using NSLock + nonisolated(unsafe) is correct — mutation is serialized through the lock.
• Import access modifiers (internal import, public import) are consistently applied per CLAUDE.md convention.
• WebBackend conformance extracted to CloudKitService+WebBackend.swift is a clean organization improvement.
• NotificationRoundtripPhase correctly classifies itself as a probe (soft warning on non-delivery) rather than a hard assertion, with cleanup on both success and failure paths.

---

Potential Bugs / Issues

1. Silent fallback in WebRequests+Subscriptions bare-subscription decoder (minor)

} else if let single = try? SubscriptionInput(from: decoder),
  single.subscriptionType != nil
{
    self.create = [single]
} else {
    self.create = []
}

The try? swallows all decoding errors on the fallback path. A valid-shaped but partially-malformed bare subscription silently becomes create: [] instead of surfacing a decoding error to the caller. Consider explicit error propagation or logging the failure so malformed browser payloads are visible.

2. RegisterToken.apnsToken defaults to "" on missing field (minor)

self.apnsToken =
    try container.decodeIfPresent(String.self, forKey: .apnsToken) ?? ""

decodeIfPresent + ?? "" means a missing apnsToken key decodes successfully as an empty string. The web server then calls registerAPNsToken(""), which throws CloudKitError.badRequest — but the error path is correct only by accident. try container.decode(String.self, forKey: .apnsToken) would give a cleaner decoder error when the field is truly absent.

3. _ = seedResult in ProbeDuplicateSubscriptionCommand+Experiment.swift (style)

seedResult is stored but never actually used — _ = seedResult suppresses an unused-variable warning. A comment explaining why it's captured (or restructuring the code) would make the intent clearer.

---

Known Follow-ups (acknowledged in PR, confirming they're tracked)

• tokens/register response spec: live server returns {apnsToken, apnsEnvironment, webcourierURL} but spec assumes an empty 200. Harmless now (body ignored), but worth spec-pinning.
• CloudKitService+TokenOperations.swift doc comment still cites Apple's archived RegisterTokens.html; CloudKit JS is the actual authority now.
• Wire-format unit test: MockTransport doesn't capture the request body, so clientId presence in the JSON can't be asserted from unit tests.

---

Test Coverage

Good:

• CloudKitServiceTests.Subscriptions+{SuccessCases,FailureCases,LikelyDuplicateCases} cover batch-result paths including the isLikelyDuplicate heuristic.
• CloudKitServiceTests.Tokens+{SuccessCases,FailureCases} cover both token endpoints.
• CourierNotificationTests and CourierTests pin wire-format parsing for the undocumented courier payload.
• WebServerTests+{Subscriptions,Tokens} test HTTP routes through MockBackend.

Gap (already noted): request-body wire format is not covered at unit level; the live integration test fills this for now.

---

Security

No concerns. APNs tokens are treated as opaque hex strings with no logging at non-debug levels. clientId defaults to a fresh UUID per call, avoiding any persistent tracking vector beyond what the caller explicitly passes.

---

SwiftLint Change

discouraged_optional_boolean was removed from .swiftlint.yml without explanation. A brief comment in the diff would help future reviewers understand the intent.

---

Summary

Well-structured, thoroughly verified feature addition. The phantom-type OperationFailure refactor is an improvement for the library's type safety, and 17 live integration phases give strong confidence in correctness. The three items above are minor; the two decoder issues (#1 and #2) are worth addressing before merge to avoid subtle failures in the web panel.

[claude]

Code Review — Push Notifications & Subscriptions epic (#381)

Great PR overall. The implementation is well-structured, the key discovery (token endpoints live at /device/1/… not /database/1/…) is thoroughly documented, and the live verification gives real confidence. Here are findings from most-to-least significant.

---

Breaking API change: RecordOperationFailure.recordName → identifier

Files: Sources/MistKit/Models/RecordOperationFailure.swift, Tests/MistKitTests/Models/BatchSyncResultTests.swift, Examples/BushelCloud/…

The rename is intentional — unifying record/subscription failures under OperationFailure<Target> with a generic identifier field is the right call. But recordName was a public property on a public struct, so any consumer code outside this repo that binds error.recordName will break at compile time. Since this targets v1.0.0-beta.2 a breaking change is acceptable; a short deprecation alias would make upgrades less friction:

@available(*, deprecated, renamed: "identifier")
public var recordName: String { identifier }

BushelCloud was already updated in this PR — just flagging it so no other consumers get surprised.

---

isLikelyDuplicate keyed on an undocumented CloudKit error message

File: Sources/MistKit/Models/SubscriptionTarget.swift (via OperationFailure+SubscriptionTarget)

public var isLikelyDuplicate: Bool {
    serverErrorCode == .internalError && reason == Self.duplicateMarker
}
// duplicateMarker = "could not find subscription we just created"

This exact-string match on an undocumented CloudKit reason string is fragile — if Apple tweaks the message the detection silently stops working. Exact-match is the right choice over contains (the test for "Internal: … (post-write)" correctly returning false confirms this), but I'd add a // MARK: - Duplicate detection comment calling out that this is reverse-engineered and could need updating if CloudKit changes its wording.

---

Dead _ = seedResult in ProbeDuplicateSubscriptionCommand+Experiment.swift

File: Examples/MistDemo/Sources/MistDemoKit/Commands/ProbeDuplicateSubscriptionCommand+Experiment.swift ~line 866

let seedResult: SubscriptionResult?
do {
    let seedResults = try await service.modifySubscriptions(...)
    seedResult = seedResults.first
    print("   seed result:  \(formatResult(seedResults.first))")  // ← printed inline
} catch { … }
// later:
_ = seedResult   // ← only use of the binding
return probeOutcome

seedResult is never read after assignment — the inline formatResult(seedResults.first) already prints it. Remove the variable declaration and the _ = seedResult suppress; the only code difference is seedResults.first is evaluated twice. If the intent was to use seedResult in the summary table, that would also be a clean follow-up.

---

discouraged_optional_boolean rule removed globally but per-line suppressions remain

Files: .swiftlint.yml, Sources/MistKit/Models/Subscriptions/SubscriptionInfo+Schema.swift

.swiftlint.yml removes - discouraged_optional_boolean from opt_in_rules, but SubscriptionInfo+Schema.swift still has two // swiftlint:disable:next discouraged_optional_boolean comments that are now dead. Either:

• Remove the per-line comments (rule is globally disabled, comments are noise), or
• Keep the rule and keep only the per-line suppressions where actually needed

The per-line approach is more surgical — it documents why Bool? is intentional at those specific spots — but either is fine as long as the rule state and the comment state agree.

---

Deletion acknowledgements silently dropped from modifySubscriptions results

File: Sources/MistKit/Models/SubscriptionResult.swift

The SubscriptionResult.swift doc comment says deletions are "omitted from results rather than represented here." A caller who issues a batch of creates and deletes receives fewer results than operations — which could be surprising. The current doc comment explains this, so it's not a bug, but consider whether the count mismatch is the right tradeoff vs a subscriptionDeleted result case. No change required, just flagging for consideration.

---

Wire-format test gap for clientId (acknowledged follow-up)

File: Sources/MistKit/CloudKitService/CloudKitService+TokenOperations.swift

The PR description calls this out: MockTransport doesn't capture the request body, so we can't assert clientId lands in the JSON. This is a known limitation tracked as a follow-up. Just confirming the omission is intentional and not missed.

---

tokens/register response spec vs live behaviour (acknowledged follow-up)

Also called out in the PR: the live server returns {apnsToken, apnsEnvironment, webcourierURL} on registerToken, but the current spec models it as an empty 200. Behaviorally harmless since the body is ignored. Flagging so it isn't forgotten.

---

Minor: completionHandler(.newData) always returned in UIKit/WatchKit push bridges

File: Examples/MistDemo/Sources/MistDemoApp/Services/PushNotificationDelegate.swift

The ObjC shim always calls completionHandler(.newData) for didReceiveRemoteNotification. For a demo this is fine, but real apps should inspect userInfo before deciding between .newData/.noData/.failed. A single-line doc comment pointing this out would help consumers who copy the pattern.

---

Summary

498/498 unit tests ✅	941/941 MistDemo tests ✅
lint clean ✅	17/17 live phases green ✅

The architecture — OperationFailure<Target>, ContainerOperationInputPath, WebCourierPoller — is solid and the endpoint-path discovery is a genuinely useful piece of reverse-engineering. The issues above are all minor or acknowledged follow-ups. The PR is in good shape to merge after the dead code cleanup and comment alignment on discouraged_optional_boolean.