Resolve #338: per-call PublicAuthPreference encoded in Database

[leogdion]

Summary

• Database.public now carries a PublicAuthPreference (.prefers(...) or .requires(...)); the dispatcher's silent "S2S wins when present" policy is gone. Caller picks attribution explicitly at every public-DB call site.
• Credentials.makeTokenManager(for:) lost the requiresUserContext flag — user-context routes (users/*) pass .public(.requires(.webAuth)) directly, collapsing the carve-out into the same machinery (per MistKit + Public signing: S2S vs web-auth identity attribution #338).
• CloudKitError.missingCredentials gained CredentialAvailability so .preferenceRequired (caller used .requires(_:) but creds absent) is distinguishable from .notConfigured.
• Two departures from the design comment in MistKit + Public signing: S2S vs web-auth identity attribution #338, agreed on during planning:
  • No defaults on PublicAuthPreference — a defaulted auth: parameter would recreate the silent-policy problem at a different layer.
  • Encoded in Database, not a separate auth: parameter — so the knob appears only where it's meaningful. .private / .shared callers don't type meaningless auth: arguments.

Library-only PR. The demo's Option C toggle (and the "You" badge fix under MistKit + Public) live in a follow-up branch off 330-interactive-mistdemo.

Test plan

• swift build — clean
• swift test — 485/485 pass (rewrote CredentialsTokenManagerTests+PublicDatabase.swift for the new prefers/requires matrix; rewrote +UserContext.swift to assert the carve-out collapse; updated DatabaseTests for pathSegment; mechanical sweep added explicit database: arguments to 14 Service test files)
• ./Scripts/lint.sh — Linting completed successfully
• Demo end-to-end verification (mistdemo web + "You" badge under MistKit + Public + .requires(.webAuth)) — lives in the follow-up branch off 330-interactive-mistdemo

🤖 Generated with Claude Code

[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: 82db2701-ddc0-4635-8559-a878e365b963

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 338-auth-preference

---

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.}

[codecov]

Codecov Report

❌ Patch coverage is 56.10860% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.39%. Comparing base (bad7b1e) to head (f9c2bd3).
⚠️ Report is 1 commits behind head on 330-interactive-mistdemo.

Files with missing lines	Patch %	Lines
...ces/MistDemoKit/Commands/DemoInFilterCommand.swift	0.00%	11 Missing ⚠️
...ources/MistDemoKit/Commands/DemoErrorsRunner.swift	0.00%	8 Missing ⚠️
...rces/MistDemoKit/Commands/UploadAssetCommand.swift	0.00%	8 Missing ⚠️
...ication/Credentials/Credentials+TokenManager.swift	88.33%	7 Missing ⚠️
...ce/Extensions/CloudKitService+Classification.swift	28.57%	5 Missing ⚠️
...ces/MistDemoKit/Commands/FetchChangesCommand.swift	0.00%	4 Missing ⚠️
...mo/Sources/MistDemoKit/Commands/QueryCommand.swift	0.00%	4 Missing ⚠️
.../MistDemoKit/Integration/Phases/CleanupPhase.swift	0.00%	4 Missing ⚠️
...oKit/Integration/Phases/IncrementalSyncPhase.swift	0.00%	4 Missing ⚠️
...emoKit/Integration/Phases/LookupRecordsPhase.swift	0.00%	4 Missing ⚠️
... and 17 more

Additional details and impacted files

@@                     Coverage Diff                      @@
##           330-interactive-mistdemo     #340      +/-   ##
============================================================
- Coverage                     70.54%   70.39%   -0.15%     
============================================================
  Files                           549      551       +2     
  Lines                         15317    15422     +105     
============================================================
+ Hits                          10806    10857      +51     
- Misses                         4511     4565      +54     

Flag	Coverage Δ
mistdemo-spm-macos	60.02% <20.36%> (-0.29%)	⬇️
mistdemo-swift-6.2-jammy	?
mistdemo-swift-6.2-noble	?
mistdemo-swift-6.3-jammy	?
mistdemo-swift-6.3-noble	60.03% <20.36%> (-0.31%)	⬇️
spm	52.30% <82.40%> (+0.40%)	⬆️
swift-6.1-jammy	?
swift-6.1-noble	?
swift-6.2-jammy	?
swift-6.2-noble	?
swift-6.3-jammy	?
swift-6.3-noble	52.36% <82.40%> (+0.99%)	⬆️

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.

[claude]

Code Review: PR #340 — Per-call PublicAuthPreference encoded in Database

Overview

This PR eliminates the silent "S2S wins when present" dispatch policy by encoding the auth signing choice directly in Database.public(PublicAuthPreference). It also removes the requiresUserContext flag, collapsing user-context routes into the same machinery by passing .public(.requires(.webAuth)) at each call site. The design is sound and the test suite is thorough (2×2 matrix of prefers/requires × serverToServer/webAuth).

---

Strengths

• Type-safe attribution — encoding the preference in Database means .private / .shared callers never type a meaningless auth: argument; the compiler enforces this.
• requiresUserContext removal — replacing a hidden flag with an explicit .public(.requires(.webAuth)) at each user-route call site is cleaner and self-documenting.
• CredentialAvailability — distinguishing .notConfigured from .preferenceRequired gives callers actionable error signal.
• Extracted helpers — makeServerToServerManager and makeAPITokenManager eliminate duplicated PEM-loading code.
• CLAUDE.md update — the decision rationale (no defaults, encoded in Database) is documented where future contributors will find it.

---

Issues and Suggestions

1. Fallback chain for prefers(.webAuth) is undocumented and inconsistent with the matrix comment

The inline comment in Credentials+TokenManager.swift states:

.prefers + mode's creds absent → fall back to the other mode.

But makePublicWithWebAuthPreference has a three-level fallback:

webAuth → S2S → bare API token → throws

If the caller has only apiAuth (no webAuthToken, no S2S), .prefers(.webAuth) quietly returns APITokenManager — a bare API-token manager, not either "mode". The matrix comment implies the fallback is S2S only. This gap should be documented (or the bare-API-token fallback should be removed, or the matrix comment updated).

2. makeAPITokenManager is misleading — it can return WebAuthTokenManager

private func makeAPITokenManager(_ api: APICredentials) -> any TokenManager {
  if let webAuthToken = api.webAuthToken {
    return WebAuthTokenManager(apiToken: api.apiToken, webAuthToken: webAuthToken)
  }
  return APITokenManager(apiToken: api.apiToken)
}

In every call-site that reaches this helper, webAuthToken is already known to be nil, so it will always return APITokenManager in practice. But the name and body imply it might return a WebAuthTokenManager. Consider naming it makeAPIOnlyTokenManager and dropping the webAuthToken branch, or simplifying to a single expression: return APITokenManager(apiToken: api.apiToken).

3. Missing test: prefers(.webAuth) + bare API token (no webAuthToken, no S2S)

apiAuth with no webAuthToken (API-token-only) is a valid Credentials state, and prefers(.webAuth) reaches the bare-API-token fallback in that case. Adding a test would pin the actual behavior:

@Test(".public(.prefers(.webAuth)) + API token only → APITokenManager (bare fallback)")
internal func prefersWebAuthAPITokenOnlyFallsBackToAPIToken() async throws { … }

4. Breaking change — no deprecation path documented

Removing all database: default values is a hard compile-time break for every existing caller that relied on defaults (e.g., queryRecords(recordType: "Note")). If this is pre-1.0 and breakage is acceptable, that's fine — but adding a Migration note to the PR description would help adopters.

5. Removed UserContext tests for .private / .shared

Four tests were removed: userContextOnPrivatePicksWebAuth, userContextOnSharedPicksWebAuth, userContextOnPrivateRejectsServerToServerOnly, userContextOnSharedRejectsServerToServerOnly. That behavior is now handled by the unchanged makePrivateOrSharedTokenManager, but the explicit coverage is gone. Retaining two regression tests for .private / .shared (independent of the user-context machinery) would guard against future regressions.

6. Minor: error message leaks credential configuration detail

"Missing credentials for database 'public' (required by preference but not configured): …"

"required by preference but not configured" signals to the caller that some credentials exist but not the required type. Excellent for debugging; worth keeping in mind if these messages ever surface in user-visible UI.

---

Test Coverage Matrix

Scenario	Covered
prefers(.serverToServer) + S2S only	✅
prefers(.serverToServer) + both creds	✅
prefers(.serverToServer) + webAuth only (fallback)	✅
prefers(.serverToServer) + API token only (fallback)	✅
prefers(.webAuth) + both creds	✅
prefers(.webAuth) + S2S only (fallback)	✅
prefers(.webAuth) + API token only	❌ missing
requires(.serverToServer) + both creds	✅
requires(.serverToServer) without S2S → preferenceRequired	✅
requires(.webAuth) + both creds	✅
requires(.webAuth) without webAuth → preferenceRequired	✅
requires(.webAuth) + API token only → preferenceRequired	✅

---

Summary

The design is correct and well-motivated. Main items to address before merging:

1. Clarify or fix the prefers(.webAuth) + bare-API-token fallback — document it or remove it from the fallback chain, and update the matrix comment to match.
2. Rename or simplify makeAPITokenManager to reflect that it always returns APITokenManager at every real call-site.
3. Add the missing test for the bare-API-token fallback path.
4. Document the breaking change in the PR description.

🤖 Generated with Claude Code

[claude]

Code Review — PR #340: Per-call PublicAuthPreference on Database

Overview

This PR replaces the implicit "S2S wins when both creds are present" dispatcher policy with an explicit, per-call signing choice encoded in Database.public(PublicAuthPreference). The requiresUserContext escape hatch is deleted; user-context routes now pass .public(.requires(.webAuth)) directly. This is a clean, well-motivated design change.

---

What Works Well

Architecture

• Encoding the signing choice in Database rather than a separate parameter is the right call. .private / .shared callers don't see a parameter that doesn't apply to them, and .public callers can't omit the choice.
• Removing requiresUserContext simplifies the dispatch path and moves the intent to the call site, which is easier to audit.
• CredentialAvailability giving .preferenceRequired vs .notConfigured is a nice addition — callers can now distinguish "misconfiguration" from "preference couldn't be satisfied".
• The makeServerToServerManager and makeAPITokenManager extractions in Credentials+TokenManager.swift eliminate the duplicated PEM-load + error-wrap pattern from the old code.

Tests

• The new PublicDatabase suite is thorough: all four (mode, required) combinations are covered, plus the fallback directions. The comment noting that notConfigured is unreachable via the public API is honest and useful.
• Renaming test methods from vague verbs (publicPicksServerToServer) to scenario descriptions (prefersS2SOnlyS2SPicksS2S) is a genuine improvement.

---

Issues / Suggestions

1. Breaking API change with no default — migration burden on callers

Every public method that previously defaulted to .public now requires an explicit database: argument. This is intentional (per the PR description), but there's no @available(*, deprecated, renamed:) bridge for the old zero-arg call sites. Any downstream consumer will hit a compile error with no migration hint. Consider:

// Transitional shim (can be removed in a future breaking release)
@available(*, deprecated, renamed: "modifyRecords(_:atomic:database:)")
public func modifyRecords(_ ops: [RecordOperation], atomic: Bool = false) async throws(CloudKitError) -> [RecordInfo] {
    try await modifyRecords(ops, atomic: atomic, database: .public(.prefers(.serverToServer)))
}

If this is intentionally a breaking change targeted at a major version bump, it should be called out explicitly in the PR body and a CHANGELOG / migration note added.

2. PublicAuthPreference is public but init is internal — not documented

The struct comment says "never via the (internal) memberwise init", but the internal init is still synthesized. There's no explicit private or internal modifier on it. Because the struct is declared public, the synthesized memberwise init is internal by default (Swift's rule), which is fine — but this is a footgun if a test in the same module ever calls .init(mode:required:) directly and bypasses the documented interface. Explicitly mark the init:

internal init(mode: Mode, required: Bool) { ... }

This makes the restriction visible in code rather than only in a comment, and prevents accidental bypass via @testable import.

3. PhasedIntegrationTest.swift uses pathSegment string comparison for enum identity

let dbLabel = database.pathSegment == "public" ? "public" : "private"
let dbName  = database.pathSegment == "public" ? "Public" : "Private"

This replaces database == .public (which broke when .public gained an associated value) with a stringly-typed workaround. The correct pattern is:

if case .public = database { ... }

Or add a computed property to Database:

public var isPublic: Bool {
    if case .public = self { return true }
    return false
}

The pathSegment comparison works today, but it will silently mismatch if a future database scope's path segment happens to equal "public", and it obscures intent.

4. makePublicWithS2SPreference falls back to bare APITokenManager — is this intentional?

When .prefers(.serverToServer) is set but S2S is absent, the fallback chain is:

1. Try S2S → not present
2. If .required → throw
3. Try apiAuth → if present, call makeAPITokenManager

makeAPITokenManager itself picks WebAuthTokenManager if webAuthToken is set, or APITokenManager if not. So .prefers(.serverToServer) with only a bare API token (no web-auth) falls all the way back to an unauthenticated-user request. That's probably correct — it preserves previous behaviour — but it's surprising and worth a comment explaining the fallback hierarchy explicitly in the source (not just in CLAUDE.md).

5. MistKitConfiguration+ConvenienceInitializers.swift uses .requires(.serverToServer)

database: .public(.requires(.serverToServer)),
// Server-to-server only supports public database

This is right for the stated purpose, but it means any consumer who builds a MistKitConfiguration via the server-to-server convenience initializer and doesn't have S2S credentials configured will get a hard throw instead of a fallback. The previous .public behaviour was effectively .prefers(.serverToServer). Confirm that .requires is the right strictness here — if the convenience init is specifically for S2S use then yes, but the function signature (apiToken: "") suggests it's the only path to S2S, so it could never have web-auth creds in the first place and .requires vs .prefers makes no observable difference. Either is fine; just worth confirming.

6. Minor: PublicDatabaseTest precondition pattern is awkward

if case .public = database {
} else {
    preconditionFailure("PublicDatabaseTest only supports the public database")
}

This is valid Swift, but the empty if body is visually confusing. Use guard instead:

guard case .public = database else {
    preconditionFailure("PublicDatabaseTest only supports the public database")
}

---

Security

No regressions found. The PR doesn't change the content of tokens — it changes which token manager is selected. The makeServerToServerManager PEM-load / error-wrap path is unchanged in effect, just refactored into a shared helper.

---

Test Coverage

485/485 passing. The matrix of (prefers|requires) × (serverToServer|webAuth) × (S2S present|absent) × (webAuth present|absent) is fully covered. The only uncovered path (no credentials at all) is correctly called out as unreachable given Credentials.init's assertions.

---

Summary

Solid change. The core design is sound and the test coverage is excellent. The main actionable items are:

1. Required: Add deprecation shims or clearly mark this as a major-version breaking change.
2. Recommended: Replace pathSegment == "public" string comparisons in PhasedIntegrationTest.swift with proper enum pattern matching.
3. Nice to have: Explicit internal init on PublicAuthPreference to document the restriction in code.

🤖 Generated with Claude Code

[claude]

PR 340 Review: Per-call PublicAuthPreference

Summary: Encodes the signing choice for .public requests in the Database type via a new PublicAuthPreference, removes the requiresUserContext flag from the dispatcher, and promotes CredentialAvailability as a first-class error distinction. Well-reasoned design and clean implementation.

Strengths

• Design rationale is solid. Encoding auth choice in Database.public(PublicAuthPreference) means .private/.shared call sites never see a meaningless auth: parameter.
• CredentialAvailability is a good addition. .notConfigured vs .preferenceRequired lets callers distinguish configuration errors from deliberate-but-unsatisfied preference constraints.
• makeAPITokenManager / makeServerToServerManager helpers eliminate duplication and keep the S2S PEM-load error path in one place.
• Test matrix for prefers/requires x serverToServer/webAuth is thorough. The inline comment explaining why the no-creds-at-all path is untested (Credentials.init prevents that state) is exactly the right kind of note.
• Callsite sweep (14+ service test files updated) is exactly what a no-defaults change needs.

Issues

1. Missing test: .prefers(.webAuth) + API-token-only fallback. makePublicWithWebAuthPreference has an uncovered path: when auth.required == false, webAuthToken is absent, S2S is absent, but bare apiAuth is present, the code falls through to makeAPITokenManager(api) returning an APITokenManager with no user identity. A caller passing .prefers(.webAuth) with only a bare API token silently gets an unauthenticated manager — arguably the same class of silent-policy problem the PR was designed to fix. Suggestion: add a test asserting this is intentional, or throw .notConfigured here since the fallback has lost any notion of user identity and there is no S2S to substitute meaningfully.

2. Prefer guard case over the empty if case in PublicDatabaseTest. The current pattern (if case .public = database {} else { preconditionFailure(...) }) should use guard case .public = database else { preconditionFailure(...) } instead.

3. String comparison against pathSegment in PhasedIntegrationTest. database.pathSegment == "public" is fragile. Pattern matching (if case .public = database) is more robust and removes coupling to the path-segment string. A small isPublic computed property on Database would make both lines one-liners.

4. Minor: comment placement in MistKitConfiguration+ConvenienceInitializers. The comment "Server-to-server only supports public database" now sits on the line after what it describes; inline trailing or the line before the argument is more conventional.

No-action notes

• Breaking API change: Removing database: defaults is source-breaking for existing consumers. Intentional per the PR description, but worth a prominent changelog entry.
• PublicAuthPreference memberwise init is correctly internal. Swift generates an internal memberwise init for a public struct with no explicit public init, so external callers genuinely cannot bypass the factory methods. The doc comment is accurate.

Overall: Architecture is a clear improvement. The dispatch logic is far easier to follow now that the signing choice travels with the database value. The four items above are mostly minor; the missing test for .prefers(.webAuth) + bare-API-token fallback is the one worth addressing before merge.

[claude]

Code Review — PR #340: Per-call PublicAuthPreference encoded in Database

Overview

This PR replaces the implicit "S2S wins when present" policy with an explicit, caller-supplied PublicAuthPreference carried inside Database.public(_:). It removes requiresUserContext from the dispatcher, collapses user-context routes into .public(.requires(.webAuth)), and adds CredentialAvailability to CloudKitError.missingCredentials so callers can distinguish a misconfiguration from a failed preference requirement.

---

Strengths

• Design clarity: encoding the signing choice in Database rather than a separate parameter is the right call. .private / .shared callers never see a meaningless auth: argument, and the compiler enforces explicit attribution on every .public call.
• Removal of silent policy: deleting the requiresUserContext flag and defaulted database: .public parameters eliminates the "who wins?" ambiguity that motivated the issue.
• makeAPITokenManager extraction: the new helper neatly collapses the duplicate "webAuthToken present? WebAuth : APIToken" logic that was scattered before.
• makeServerToServerManager extraction: similarly removes the duplicated PEM-loading block.
• Test matrix is comprehensive: the 4×3 (prefers/requires × S2S only / webAuth only / both) grid in CredentialsTokenManagerTests+PublicDatabase.swift covers the full resolution table from the doc comment.
• No breaking drift on RecordManaging: using .public(.prefers(.serverToServer)) for the deprecated conformance preserves old behaviour while the comment explains why.

---

Issues & Suggestions

1. fallback to APITokenManager semantics may surprise callers (medium)

In makePublicWithWebAuthPreference, when .prefers(.webAuth) is requested but web-auth isn't available, the fallback chain is: S2S → then bare APITokenManager. A bare API token produces anonymous requests, which is arguably worse than throwing. The same issue existed before, but it's now explicitly reachable via a prefers(.webAuth) path.

Consider whether the .prefers path should ever produce a bare APITokenManager, or whether it should stop at the highest-attribution fallback. At minimum, document this in PublicAuthPreference's doc-comment.

// Current behaviour (surprise):
// .prefers(.webAuth) + only bare-APIToken configured → APITokenManager (anonymous)

2. PhasedIntegrationTest comparison is fragile (low)

// PhasedIntegrationTest.swift line 374
let dbLabel = database.pathSegment == "public" ? "public" : "private"

This pathSegment string comparison replaces database == .public and now silently mislabels .shared as "private". Since pathSegment for .shared is "shared", the label would read "private" for a shared database. The original equality check had a different bug (no longer compiles), but the intent should be captured with a proper switch or if case:

let dbLabel: String
switch database {
case .public: dbLabel = "public"
case .private: dbLabel = "private"
case .shared: dbLabel = "shared"
}

3. MistKitConfiguration convenience initializer uses .requires where .prefers may be better (low)

// MistKitConfiguration+ConvenienceInitializers.swift
database: .public(.requires(.serverToServer)),

This is a convenience initialiser for server-to-server configs. Using .requires means any caller who accidentally passes an API-token config here gets a hard throw instead of a graceful fallback. .prefers(.serverToServer) would be consistent with the MistDemoConfig.parseDatabase("public") policy and would be less surprising.

4. Removed tests for requiresUserContext on .private / .shared (low)

The deleted tests (userContextOnPrivatePicksWebAuth, userContextOnSharedPicksWebAuth, etc.) validated that passing the old requiresUserContext: true flag with .private/.shared still routed through web-auth. With the rewrite these paths now go through makePrivateOrSharedTokenManager unconditionally (always web-auth), so the behaviour is preserved — but there are no new tests confirming that .private and .shared ignore any auth-attribution concept entirely. A couple of sentences in a test comment or a dedicated test like privateAlwaysUsesWebAuth would close that gap.

5. CredentialAvailability default parameter in missingCredentials (cosmetic)

case missingCredentials(
  database: Database,
  availability: CredentialAvailability = .notConfigured,
  reason: String
)

A default on an enum associated value is uncommon and means availability can be omitted at call sites. All new call sites in this PR pass it explicitly, which is good — but future contributors may omit it and get a silently wrong availability. Consider removing the default and making the argument required, or add a lint note.

6. Comment style (minor)

The project defaults to no comments unless the why is non-obvious. A few blocks added in this PR narrate what rather than why, e.g. the /// Query records of a specific type from CloudKit (deprecated single-page form) block in CloudKitService+RecordManaging.swift. The why explanation below it (RecordManaging is a database-agnostic abstraction predating per-call PublicAuthPreference...) is the genuinely useful part — the first line just repeats the function name.

---

Test Coverage

• 485/485 pass per the PR description. ✅
• New CloudKitErrorTests.swift covers errorDescription formatting for both availability cases. ✅
• DatabaseTests updated to use pathSegment. ✅
• The prefers/requires matrix in CredentialsTokenManagerTests+PublicDatabase.swift is thorough. ✅
• Gap: no test for the .shared database pathSegment comparison fix noted in item Fix "method_lines" issue in Sources/MistKit/MKDatabase.swift #2.

---

Summary

The design is sound and a genuine improvement over the previous implicit-policy approach. The two items worth addressing before merge are the pathSegment string comparison in PhasedIntegrationTest (#2 — silent mislabelling) and documentation of the bare-APITokenManager fallback (#1 — observable behaviour that should be intentional). The rest are low-priority polish.

🤖 Generated with Claude Code