MistDemo: --database flag + demo-errors command (closes #259, #269)

[leogdion]

Summary

Implements parent issue #281, bundling two paired MistDemo changes for the "CloudKit as Your Backend" talk:

• Closes Commands should accept --database flag instead of hardcoding database #259 — adds shared --database flag on MistDemoConfig so every command can target public / private / shared without rebuilding. Single source of truth; matches the TestIntegrationConfig reference (which is now de-duplicated).
• Closes MistDemo: Add error demonstration commands for talk demo #269 — new demo-errors command walks 401 / 404 / 409 scenarios with typed CloudKitError handling for the talk's Act 3, Step 4. Also adds a --bad-credentials flag on MistDemoConfig for in-context 401 demos on existing commands.

Behavior change

Commands that previously hardcoded .private (create, update, delete, lookup, modify, current-user, upload-asset, lookup-zones, fetch-changes) now default to .public (matching TestIntegrationConfig's precedent). Pass --database private to keep prior behavior. Two intentional exceptions: test-private is pinned to .private; auth-token ignores the flag (no DB).

What's new

• MistKitClientFactory.create(for:) and .create(_:for:) — the new entry points commands use; honor both database and badCredentials transparently.
• DemoErrorsCommand — mistdemo demo-errors [--scenario all|401|404|409] [--database <db>].
• 401: separate service constructed with placeholder tokens (real config never mutated).
• 404: queries a deliberately bogus record type.
• 409: real round-trip — create → update (advances tag) → second update with the original (now stale) recordChangeTag. Cleans up the demo record on completion.

Test plan

• `swift build` (Examples/MistDemo + repo root) — clean
• `swift test --parallel` — 861 tests, 55 suites, all passing
• `mistdemo --help` lists `demo-errors`
• `mistdemo demo-errors --help` shows `--scenario` flag
• `mistdemo query --database garbage` → `ConfigurationError.invalidDatabase("garbage")`
• Manual run-through against a live container before the talk: `mistdemo demo-errors --database private`
• Confirm `mistdemo create --database private --field "title:string:x"` still creates on private
• Confirm `mistdemo test-private` still pins to private regardless of flag

🤖 Generated with Claude Code

---

Perform an AI-assisted review on

[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: 18d59695-7b94-4141-94c5-2bd54518c736

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 281-mistdemo-error-demo

---

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

Overall: Clean, well-structured PR. The deduplication of database parsing into MistDemoConfig is the right call, and the DemoErrorsRunner/DemoErrorsCommand split mirrors the IntegrationTestRunner pattern well. A few things worth addressing before the talk.

---

Issues

1. 409 scenario: single catch swallows setup errors (High)

runConflict() wraps all three steps — create, first update, stale update — in one do-catch. If the create fails (bad credentials, schema mismatch, network error), the catch block prints it as "expected 409" and then silently skips cleanup. In a live demo that's a confusing failure mode.

runUnauthorized() and runNotFound() use separate catch paths to distinguish "expected error" from "unexpected error". The conflict demo should do the same:

// step 1+2: setup — fail loudly if these error
do {
    created = try await service.createRecord(...)
    updated = try await service.updateRecord(..., recordChangeTag: staleTag)
} catch {
    print("❌ Setup failed (not a 409 demo): \(error)")
    return
}

// step 3: the one that's expected to 409
do {
    _ = try await service.updateRecord(..., recordChangeTag: staleTag)
    print("⚠️  Expected 409 but stale-tag update succeeded.")
} catch let error as CloudKitError {
    printCloudKitError(error, expectedStatus: 409)
    ...
} catch {
    print("❌ Unexpected non-CloudKit error: \(error)")
}

2. Default .public for private-only operations (Medium)

FetchChangesCommand, LookupZonesCommand, and CurrentUserCommand previously hardcoded .private for good reason — those operations are semantically private-database operations (user identity, custom zones, record changes). Defaulting them to .public after this PR means:

• mistdemo current-user (no flag) → hits public DB → likely 401/403
• mistdemo fetch-changes (no flag) → hits public DB → no meaningful data
• mistdemo lookup-zones (no flag) → hits public DB → returns only default zone

Consider keeping .private as the hardcoded default for those three specifically, or at least adding a note in the --help text that --database private is almost always needed.

3. --bad-credentials always injects WebAuthTokenManager (Medium)

In MistKitClientFactory.create(_ database: for config:), the bad-credentials path always builds a WebAuthTokenManager. If a user runs mistdemo query --database public --bad-credentials, the factory injects web-auth tokens into what would normally be a server-to-server signed request. Depending on how the auth selection works downstream, this might produce a confusing error other than 401.

Since --bad-credentials is primarily for the 401 demo (private DB), consider either: (a) ignoring the flag when database == .public, or (b) documenting in the flag's help text that it only applies to private/shared auth flows.

4. Timestamp-based record name has collision risk (Low)

let recordName = "demo-errors-conflict-\(Int(Date().timeIntervalSince1970))"

Re-running within the same second (easy to do during rehearsal) produces duplicate record names and a confusing 409 in step 1 before the demo even starts. UUID().uuidString is collision-free.

5. Duplicate magic string (Low)

"invalid_demo_token" appears in both MistKitClientFactory.create(_:for:) and DemoErrorsRunner.runUnauthorized(). Extract to a private static let demoPlaceholderToken = "invalid_demo_token" in the factory (or a shared constants file) so they stay in sync.

---

Positives

• Typed throws used correctly — createRecord/updateRecord/deleteRecord declare throws(CloudKitError), so the bare catch { printCloudKitError(error, ...) } is valid Swift 6 and error is already CloudKitError. Good use of the language feature.
• badCredentials never mutates real config — constructing a separate WebAuthTokenManager instead of overwriting the live config is the right approach.
• TestIntegrationConfig deduplication — removing the separate database field and parsing from TestIntegrationConfig eliminates the duplicated parsing logic cleanly.
• TestPrivateCommand correctly pins database — using create(.private, for: config.base) is correct; this command's database is identity, not a flag.
• DemoErrorsConfig error message — dynamically building the error string from ErrorScenario.allCases.map(\.rawValue) means it stays correct if cases are added.

---

Nit

No automated tests for the new DemoErrorsConfig parsing (especially the invalidScenario error path and the scenario flag parsing). Even a few @Test cases for config parsing would catch regressions — the existing integration command tests could serve as a model.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 38.03340% with 334 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.10%. Comparing base (2c3b43e) to head (0263d08).
⚠️ Report is 3 commits behind head on v1.0.0-beta.1.

Files with missing lines	Patch %	Lines
...ources/MistDemoKit/Commands/DemoErrorsRunner.swift	0.00%	134 Missing ⚠️
...ources/MistDemoKit/Commands/AuthTokenCommand.swift	16.27%	108 Missing ⚠️
...ces/MistDemoKit/Configuration/MistDemoConfig.swift	17.02%	39 Missing ⚠️
...s/MistDemoKit/Configuration/DemoErrorsConfig.swift	0.00%	19 Missing ⚠️
.../MistDemoKit/Configuration/TestPrivateConfig.swift	0.00%	6 Missing ⚠️
...urces/MistDemoKit/Commands/DemoErrorsCommand.swift	0.00%	5 Missing ⚠️
...DemoTests/CloudKit/MistKitClientFactoryTests.swift	95.89%	3 Missing ⚠️
...MistDemoTests/Commands/AuthTokenCommandTests.swift	96.77%	3 Missing ⚠️
.../MistDemoKit/Commands/TestIntegrationCommand.swift	0.00%	2 Missing ⚠️
...MistDemoKit/Configuration/ConfigurationError.swift	33.33%	2 Missing ⚠️
... and 13 more

Additional details and impacted files

@@                Coverage Diff                 @@
##           v1.0.0-beta.1     #282       +/-   ##
==================================================
+ Coverage          25.66%   53.10%   +27.44%     
==================================================
  Files                 95      245      +150     
  Lines               8240    18193     +9953     
==================================================
+ Hits                2115     9662     +7547     
- Misses              6125     8531     +2406     

Flag	Coverage Δ
mistdemo-spm-macos	42.57% <38.03%> (?)
mistdemo-swift-6.2-jammy	42.59% <38.03%> (?)
mistdemo-swift-6.2-noble	42.57% <38.03%> (?)
mistdemo-swift-6.3-jammy	42.57% <38.03%> (?)
mistdemo-swift-6.3-noble	42.57% <38.03%> (?)
spm	25.53% <ø> (-0.12%)	⬇️
swift-6.1-jammy	25.61% <ø> (+0.05%)	⬆️
swift-6.1-noble	25.55% <ø> (-0.02%)	⬇️
swift-6.2-jammy	25.55% <ø> (-0.11%)	⬇️
swift-6.2-noble	25.64% <ø> (-0.01%)	⬇️
swift-6.3-jammy	25.64% <ø> (?)
swift-6.3-jammynightly	?
swift-6.3-noble	25.55% <ø> (?)
swift-6.3-noblenightly	?

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 282: --database flag + demo-errors command

Overview
Adds a shared --database flag to MistDemoConfig so every command targets the right database without rebuilding, and introduces a new demo-errors command that walks 401/404/409 scenarios with typed CloudKitError handling. 436 additions / 31 deletions.

What is good

• Clean factory consolidation. MistKitClientFactory.create(for:) as the single entry point for commands (and create(_:for:) to pin the database) eliminates the scatter of per-command .private/.public hardcoding. The migration is mechanical and consistent.
• badCredentials flag design is safe. A separate WebAuthTokenManager with placeholder tokens is constructed; the real config is never mutated. This is the right approach.
• 409 scenario is realistic. Create -> update (advances tag) -> retry with stale tag is a genuine production pattern, and cleaning up with try? (best effort) is appropriate for a demo runner.
• DemoErrorsError.invalidScenario error is well-formed with a helpful message listing valid values.
• test-private pins to .private via create(_:for:) — the right exception to the "honor config.database" rule.

Issues

DemoErrorsRunner.runUnauthorized hardcodes .private.
The runner constructs its bad-credentials service with database: .private regardless of config.database. If the talk runs against a public-only container (no web auth token), this will fail with a credentials error before even reaching the expected 401. Suggest using config.database here for consistency.

Missing .shared label in printRunnerHeader.
config.database.rawValue is used directly in the header, which is fine, but the .shared case will print "shared" while the rest of the codebase uses "public" / "private" labels. Not a bug, just worth a quick check that rawValue matches what the factory expects.

Conflict scenario does not print the stale tag when it is nil.
In runConflict, describe(staleTag) returns "" if the tag is nil. In that case the demo prints "Re-using the original (now stale) recordChangeTag" with no actual tag value shown, which may confuse a live audience. A short guard or early-exit with a message ("CloudKit did not return a recordChangeTag — cannot demo conflict") would be cleaner.

DemoErrorsRunner is internal but its methods are also internal. The runUnauthorized/runNotFound/runConflict methods could be private since they are only called from run(scenario:). Marking them private would narrow the surface area.

Minor

• DemoErrorsConfig has a public init(base:scenario:) convenience init and a public init(configuration:base:) async init. The async one is the protocol entry point; the convenience one is for tests. Worth a brief comment distinguishing the two.
• The PHASES: help text blocks in TestIntegrationCommand.swift and TestPrivateCommand.swift are now stale (per PR 283 which this PR is branched alongside). Coordinate with PR 283 on which PR updates them.

Verdict

Solid feature addition. The factory consolidation alone is a meaningful cleanup. Address the hardcoded .private in runUnauthorized and the nil-tag edge case in runConflict before the talk; the rest are polish.

[claude]

Code Review — PR #282: MistDemo --database flag + demo-errors command

Overview

Two well-scoped changes bundled together: a shared --database flag on MistDemoConfig (moving database selection out of individual commands) and a new demo-errors command that walks through 401/404/409 scenarios for a talk demo. The behavior change (commands default to .public instead of .private) is clearly documented.

---

Code Quality & Design

Strengths:

• MistKitClientFactory.create(for:) as a single entry point that handles both database and badCredentials transparently is a clean façade. Commands no longer need to know about the auth variant.
• DemoErrorsRunner keeps the error scenarios well-isolated — the 401 path builds a separate service without touching the real config, which is the right way to demo bad credentials without side effects.
• TestPrivateCommand correctly overrides to .private via create(.private, for: config.base), which means the flag can't accidentally break pinned commands.
• TestIntegrationConfig.database removed (now inherited from MistDemoConfig.database): good deduplication.

Issues / Suggestions:

1. badCredentials flag always uses WebAuthTokenManager regardless of the configured auth method. In MistKitClientFactory.create(_:for:):

   if config.badCredentials {
     let badTokenManager = WebAuthTokenManager(
       apiToken: "invalid_demo_token",
       webAuthToken: "invalid_demo_token"
     )
     return try create(from: config, tokenManager: badTokenManager, database: database)
   }

   If the target database is .public, the factory normally uses server-to-server signing (CLOUDKIT_KEY_ID + CLOUDKIT_PRIVATE_KEY). Injecting a WebAuthTokenManager here may silently change the auth path rather than just injecting bad credentials into the expected auth path. For the demo this is likely fine (you want a 401 regardless), but the comment should clarify this is intentional.

2. DemoErrorsRunner.runConflict uses createRecord / updateRecord / deleteRecord. These are convenience methods on CloudKitService. If any of them don't exist on the protocol (only on a concrete implementation), this will fail to compile against a mock service in tests. Confirm they're on the protocol, not an extension.

3. 409 scenario error catch is too broad. In runConflict:

   } catch {
     printCloudKitError(error, expectedStatus: 409)
   }

   This catches all errors in the entire do block — including network failures during create (step 1) or the first update (step 2). If step 1 fails, printCloudKitError will print "✅ Caught CloudKitError — status: n/a" or an unexpected status, which will look like a successful demo. Consider restructuring with nested do/catch blocks so only step 3 (the stale-tag update) is caught here.

4. ErrorScenario.allCases.map(\.rawValue) produces ["all", "401", "404", "409"]. The error message is:

   Invalid --scenario 'foo'. Must be one of: all, 401, 404, 409
   

   This is correct and user-friendly.

5. database key in MistDemoConfig is parsed before the authentication keys, but the default is "public" which requires server-to-server auth. If a user passes --database private but has no web auth token configured, the error comes from the factory (at runtime), not from config parsing. This is acceptable behavior but worth noting in the help text.

6. DemoInFilterCommand change (create(.public, from:) → create(for:)) appears in both PR MistDemo: --database flag + demo-errors command (closes #259, #269) #282 and PR Refactor IntegrationTestRunner into protocol-based phase pipeline (#254) #283. One of them will conflict on merge. Coordinate which PR owns this change.

---

Behavior Change Risk

Commands that previously hardcoded .private (create, update, delete, lookup, modify, current-user, upload-asset, lookup-zones, fetch-changes) now default to .public. This is documented and intentional, but .private operations require web auth tokens while .public requires server-to-server keys — the auth requirements have silently changed for these commands when run without --database. Users with only private-database credentials configured will get auth errors running mistdemo create where they previously succeeded.

The PR description acknowledges this, but the --help output for each affected command should mention the new default. Without that, the change is a silent behavior break for existing users.

---

Test Coverage

• 861 tests passing (per PR description) — reassuring the factory changes don't break existing paths.
• No automated test for DemoErrorsRunner — acceptable for demo tooling, but the 409 broad-catch issue (point 3 above) is the kind of bug that would be caught immediately with a simple mock.

---

Overall: The factory refactor is clean and correct. The demo-errors command is well-structured for its purpose. Fix the 409 broad-catch before the talk — a live demo where step 1 fails and silently prints "✅" would be embarrassing. The behavior-change risk for existing private-database users is worth a --help mention.

🤖 Generated with Claude Code

[claude]

Code Review: PR #282 — --database flag + demo-errors command

Overall: Well-structured PR. The centralized database flag, with(database:) copy-and-modify pattern, and DemoErrorsCommand/DemoErrorsRunner split are all clean. However there are two functional bugs that will cause the live demo to misbehave, plus several documentation issues from the default database switch.

---

🔴 Critical: 401 Demo Never Reaches the Server

File: MistKitClientFactory.swift lines ~52–58, DemoErrorsRunner.swift lines ~68–87

The placeholder tokens "invalid_demo_token" (19 chars) fail WebAuthTokenManager.validateCredentials() locally before any HTTP request is issued. If the token validator requires a 64-character hex string (a common pattern in this codebase), the validation throws a local TokenManagerError, which mapToCloudKitError wraps as CloudKitError.underlyingError(...). That case's httpStatusCode returns nil.

printCloudKitError then shows:

❌ Caught CloudKitError — status: n/a

…instead of the intended ✅ Caught CloudKitError — status: 401. For a live talk demo this is a showstopper.

Fix: Use syntactically valid but semantically invalid tokens that will pass local format validation and reach Apple's API:

let badTokenManager = WebAuthTokenManager(
    apiToken: String(repeating: "0", count: 64),    // valid hex format, rejected server-side
    webAuthToken: String(repeating: "a", count: 100) // passes length check
)

Tokens that pass local validation will reach Apple's servers and return a genuine HTTP 401, which maps to CloudKitError.httpErrorWithDetails(401, ...).

---

🟠 Major: badCredentials on .public Database Uses Wrong Auth Model

File: MistKitClientFactory.swift lines ~52–58

When config.badCredentials == true, the factory unconditionally creates a WebAuthTokenManager regardless of config.database. For .public database (now the default), this is wrong — the public database uses server-to-server signing (ServerToServerAuthManager), not web auth tokens. The result is the same local validation failure described above, not a real 401.

Fix: Branch on config.database when constructing the bad-credentials service:

if config.badCredentials {
    switch config.database {
    case .public:
        // Use a fake server-to-server key manager for the public DB
        return try create(from: config, keyManager: FakeS2STokenManager())
    case .private, .shared:
        let badTokenManager = WebAuthTokenManager(
            apiToken: String(repeating: "0", count: 64),
            webAuthToken: String(repeating: "a", count: 100)
        )
        return try create(from: config, tokenManager: badTokenManager)
    }
}

---

🟡 Moderate: 409 Catch Block Masks Step 1/2 Failures

File: DemoErrorsRunner.swift lines ~125–158

The entire create→update→stale-update sequence is wrapped in one do/catch. If createRecord or the first updateRecord fails (schema mismatch, bad credentials, etc.), the catch block fires and prints:

💡 Recovery: read the `serverRecord` from the response, merge fields, and retry...

…even though no conflict occurred. For a live talk the audience would see a misleading recovery hint.

Fix: Separate do/catch blocks per step so failure context is accurate.

Note: The cleanup logic is otherwise correct — createdRecordName is only set after createRecord succeeds, so no orphaned records leak on step-1 failure.

---

🟡 Moderate: Stale Help Text After .private → .public Default Change

Files: LookupZonesCommand.swift line ~56, UploadAssetCommand.swift lines ~79–80

These commands still say things like:

• "Uses web authentication (private database) by default" — now incorrect
• "With web authentication: uploads to private database" — no longer the default

Also, the --database flag is not documented in the OPTIONS section of create, lookup-zones, upload-asset, fetch-changes, etc. Users of these commands won't know to pass --database private to restore prior behavior.

---

🟡 Moderate: current-user Semantic Change

File: CurrentUserCommand.swift

fetchCurrentUser() now targets .public by default. The CloudKit /users/current endpoint on the public database uses server-to-server auth. Most existing users of this command expected private DB behavior. The help text still lists --web-auth-token as a required field, implying private DB intent. Consider either pinning current-user to .private (like test-private does) or explicitly documenting the credential requirements per database in its help text.

---

🟢 Low: No Tests for badCredentials Path

Files: MistDemoConfig+Testing.swift, MistKitClientFactoryTests.swift

The test convenience initializer does not include badCredentials, and there are zero unit tests for the badCredentials branch in MistKitClientFactory.create(for:). Missing coverage:

• create(for:) with badCredentials = true does not use real config tokens
• create(for:) with badCredentials = true on .public database
• DemoErrorsRunner.runUnauthorized() produces a CloudKitError (not some other error type)

---

🟢 Low: Factory Docstring Incomplete

File: MistKitClientFactory.swift lines ~35–45

The docstring documents the standard auth-selection logic but doesn't mention that badCredentials == true bypasses it entirely. Add a note that badCredentials takes precedence over database-based auth selection.

---

CLAUDE.md Conventions

• ✅ Async/await throughout — no completion handlers
• ✅ Sendable conformances on MistDemoConfig, DemoErrorsConfig, etc.
• ✅ MistKitClientFactory as a Sendable struct with static factory methods
• ⚠️ DemoErrorsRunner.run(scenario:) is async but not throws — errors are swallowed internally. Acceptable for demo CLI ergonomics but diverges from the "typed errors conforming to LocalizedError" principle. Acceptable given this is presentation-layer code.

---

Priority Summary

Priority	Issue
Critical	Placeholder tokens fail local validation — 401 demo shows n/a, not 401
Major	badCredentials on .public uses WebAuthTokenManager (wrong auth model)
Moderate	409 catch block masks step 1/2 failures with misleading recovery hint
Moderate	Stale help text after default switch from .private to .public
Moderate	current-user semantic change on .public DB not called out
Low	badCredentials path has no unit test coverage
Low	Factory docstring doesn't describe badCredentials short-circuit

The critical fix needed before the talk: use format-valid placeholder tokens (String(repeating: "0", count: 64) for the API token) so the 401 demo actually reaches Apple's servers.

🤖 Generated with Claude Code

[claude]

Code Review — PR #282: --database flag + demo-errors command

Overall: Well-scoped pair of features. The factory refactor is clean and the demo-errors scenarios are pedagogically well-designed for the talk. Key items below.

---

Breaking behavior change (intentional, but verify defaults)

Commands that previously hardcoded .private now default to .public (via MistDemoConfig.database). This is documented in the PR and is the right default to match TestIntegrationConfig. Two things to double-check before removing DRAFT:

• current-user on the public database requires server-to-server signing (CLOUDKIT_KEY_ID + private key). Running mistdemo current-user without private-DB credentials against the public DB will fail with a ConfigurationError, not silently return empty. The updated help text explains this — good. ✅
• auth-token correctly ignores the --database flag (it's about obtaining credentials, not using a DB). ✅

---

MistKitClientFactory

• create(for:) is a clean rename; the badCredentials guard-before-switch keeps the logic readable. ✅
• makeBadCredentialsTokenManager() is internal — accessible from DemoErrorsRunner and the factory's own badCredentials path. Good.
• Concern: create(from:tokenManager:) no longer accepts a database parameter; it always uses config.database. The old overload allowed database: .private as a default arg. Check that no call sites relied on that defaulting behavior differently from what config.database now provides.

---

DemoErrorsRunner

401 scenario — creates a separate service with placeholder tokens and uses .private explicitly (via config.with(database: .private)), isolating from the user's real config. ✅

404 scenario — queries a deliberately bogus type name. The name "DefinitelyNotARealType_DemoErrorsCommand_xyz" is collision-proof in practice. ✅

409 scenario — potential issue:

var staleTag: String?
// ... create → staleTag = created.recordChangeTag
// Step 2: updateRecord(recordChangeTag: staleTag)   // staleTag could be nil
// Step 3: updateRecord(recordChangeTag: staleTag)   // same nil — 409 may not fire

If created.recordChangeTag is nil (e.g., CloudKit didn't return it), staleTag stays nil. Step 2 would then send no change tag, potentially succeeding without advancing the server's internal tag. Step 3 would also send nil — CloudKit may accept it rather than returning 409. The demo could then print the misleading "Expected 409 but stale-tag update was accepted" warning. Consider asserting staleTag != nil after the create step, or at minimum logging a warning if it's nil.

Step 3 bare catch: printCloudKitError(error, expectedStatus: 409) works only because service.updateRecord uses typed throws(CloudKitError) — the bare catch then binds error: CloudKitError. This is correct, but it's non-obvious to a reader. A quick comment noting the typed-throw dependency would help.

Cleanup: cleanupConflictRecord uses try await service.deleteRecord(...) and logs a warning on failure — appropriate best-effort. ✅

---

AuthTokenCommand refactor

The PR reformats the entire struct body to be indented inside #if canImport(Hummingbird). This is functionally equivalent to the previous placement but requires reviewing that AuthTokenError enum is still in scope for callers — it's now also inside the #if block. As long as callers of AuthTokenError also guard with #if canImport(Hummingbird), this is safe.

---

Test coverage

• Unit tests: the new ConfigurationError.badCredentialsNotSupportedOnPublicDatabase case and makeBadCredentialsTokenManager() factory method aren't covered by unit tests. For the factory method this is low-risk (it's deterministic), but the new error case would be good to cover with a simple test.
• The PR notes swift test --parallel passes (861 tests). ✅

---

Minor nits

• describe(staleTag) helper in DemoErrorsRunner — good for nil-safe printing. ✅
• database.rawValue in printRunnerHeader() assumes Database has a stable raw value. If Database is a non-string enum, this might give integer output. Worth checking or using a switch/description property.
• Help text for demo-errors says --database <type> has default from MistDemoConfig: public — clear and accurate. ✅

Verdict: Approve with the 409 staleTag == nil edge case flagged. Everything else is solid.

[claude]

Code Review: PR #282 — MistDemo: --database flag + demo-errors command

Overview

Two paired changes: (1) replaces per-command hardcoded database values with a shared --database flag on MistDemoConfig, and (2) adds a demo-errors command demonstrating 401/404/409 error handling for the talk. Both changes have clear motivation and the implementation is generally clean.

---

Strengths

• Single source of truth for database selection — MistKitClientFactory.create(for:) consuming config.database is the right call. The old create(_ database:, from:) scattered hardcoded .private across eight command files.
• makeBadCredentialsTokenManager() is well-documented — the inline doc explaining exactly why those token formats pass local validation but fail on Apple's servers is the kind of comment that earns its keep.
• 409 scenario is realistic — doing a real create→update→stale-tag update round-trip is the most educational way to demonstrate optimistic locking. The cleanup-on-completion is good practice.
• badCredentials flag throws on .public — rather than silently doing something unexpected, throwing badCredentialsNotSupportedOnPublicDatabase is the right defensive choice.
• Auth token command re-indentation — logically a no-op; the code inside #if canImport(Hummingbird) got reformatted to match the conditional indentation. No behavior changes.

---

Issues

1. Behavior change breaks existing users without a flag
Commands that previously targeted .private by default (create, update, delete, lookup, modify, current-user, upload-asset, lookup-zones, fetch-changes) now default to .public. Anyone running these without --database private will hit a different endpoint. This is documented in the PR body but isn't surfaced in any of the help text strings for those commands. Consider adding a line to each affected command's OPTIONS: block noting the new default, e.g.:

--database <type>   Database to target: public, private, shared (default: public)

Several commands already have this (e.g. create), but not all do.

2. No cleanup-on-failure for the 409 demo scenario
The integration test runner has robust cleanup-on-failure logic. The 409 demo creates a real record; if the second (conflict-triggering) update throws an unexpected error (not a 409), the cleanup block may not run, leaving a test record behind. Consider wrapping the 409 scenario in a defer or try/finally pattern similar to the integration test.

3. AuthTokenIndexHTML.swift diff noise
The entire file content changes due to indentation, producing ~1100 lines of diff for a cosmetic change. This makes the PR harder to review and bisect. For future changes, whitespace-only reformats are easier to review in a separate commit so git log -S and blame history stay clean. Not a blocker, but worth noting for the workflow.

4. Hardcoded environment: 'development' in AuthTokenIndexHTML

environment: 'development'

This is pre-existing behavior but the new PR makes MistDemoConfig more config-aware. The CloudKit JS auth page will always target development even if the Swift config targets production. Not critical for the demo, but a potential source of confusion.

5. create(from:tokenManager:) parameter change is a silent breaking change
The old signature was create(from:tokenManager:database:) — callers could pass a database different from config.database. The new version ignores any caller-specified database override. Within MistDemo this is fine since all call sites are updated. If this function is considered public API (it has public access), callers outside the module will get a compile error, but the removal of the database parameter also changes the semantics silently if they happened to be using the default. Worth calling out in a changelog or migration note.

---

Minor

• The demo-errors --scenario 401 path constructs a new service with bad credentials. If the config also has badCredentials: true AND the user passes --scenario 401 to demo-errors, are there two bad-credential services being created? Confirm the DemoErrorsRunner constructs its own service independently of config.badCredentials.
• The DemoErrorsCommand.helpText mentions --database <db> but the MistDemoConfig shared flag already covers this. Ensure the help text accurately reflects that --database is inherited from the shared config, not a local option.

---

Summary

The --database flag migration is clean and well-executed. The demo-errors command is a solid addition for the talk. Two items worth addressing before the live demo: add --database docs to the affected commands' help text, and add cleanup-on-failure for the 409 scenario to avoid leaving orphan records if something goes sideways mid-demo.

[claude]

Code Review — PR #282: --database flag + demo-errors command

Overview

Two cleanly scoped changes bundled together: promoting database to a shared MistDemoConfig field (removing per-command hardcoding), and a new demo-errors command demonstrating 401/404/409 CloudKit error paths. Both are well-implemented.

---

Strengths

• Single source of truth for database: Moving the field to MistDemoConfig with MistKitClientFactory.create(for:) as the primary entry point correctly eliminates the scatter of per-command database decisions.
• makeBadCredentialsTokenManager() is shared: Both the factory's badCredentials short-circuit and DemoErrorsRunner.runUnauthorized use the same method — no divergence risk.
• 409 demo is realistic: The create → update → stale-tag update round-trip produces a genuine server-side conflict, not a synthetic one. The cleanup step is a nice touch.
• with(database:) pattern: Clean way to pin specific databases for commands like test-private without mutating the original config.
• Error messages include recovery guidance: Each demo prints a 💡 Recovery: line, which is exactly what the talk scenario needs.

---

Issues

1. Behaviour change: default database switches from .private to .public (breaking)

Commands that previously hardcoded .private (create, update, delete, lookup, modify, current-user, upload-asset, lookup-zones, fetch-changes) now default to .public.

This is a silent breaking change for existing users of these commands who relied on the implicit .private default. Anyone with a config file or script targeting the private database without an explicit --database private flag will now silently hit the public database. The PR description calls this out; please make sure it's prominent in release notes / the talk's setup instructions.

2. with(database:) / memberwise init maintenance burden

The internal memberwise init now takes 17 parameters. Every future MistDemoConfig property addition requires updating both the memberwise init and with(database:). Consider whether making database a var property (and relaxing let on other demo-flag fields) would be simpler. Since MistDemoConfig is Sendable, mutating a local copy before passing it is safe — but var properties on a public struct can be surprising. At minimum, add a comment on the memberwise init warning that with(database:) must stay in sync.

3. DemoErrorsRunner.run(scenario:) is not throwing — exit code won't reflect failures

internal func run(scenario: ErrorScenario) async {

Each sub-method (runUnauthorized, runNotFound, runConflict) is also non-throwing and catches all errors internally. For a demo command this is intentional (the point is to show the errors, not to propagate them), but it means mistdemo demo-errors will always exit with code 0 even if credentials are completely missing and every scenario fails. Consider whether this is acceptable for CI or scripted use.

4. Inconsistency in which database the 401 demo targets

runUnauthorized hard-pins to .private:

let service = try MistKitClientFactory.create(
    from: config.with(database: .private),
    tokenManager: MistKitClientFactory.makeBadCredentialsTokenManager()
)

runNotFound and runConflict use config.database (whatever the user passed). The PR description says the 401 demo is "pinned to .private because web-auth tokens are the most representative CloudKit credential failure", which is a valid reason — but it means mistdemo demo-errors --database public --scenario 401 ignores the --database flag silently. A comment explaining this (or a printed note like "401 demo always uses private database (web-auth credentials)") would prevent user confusion.

5. DemoErrorsRunner.cleanupConflictRecord is private but lives in a non-final struct

Minor: since DemoErrorsRunner is an internal struct, private is equivalent to fileprivate. No functional issue, just a consistency note.

6. bogusRecordType string may eventually exist

private static let bogusRecordType = "DefinitelyNotARealType_DemoErrorsCommand_xyz"

If someone adds a record type with this name to the container (unlikely but possible), the 404 demo silently returns success. Appending a UUID suffix per run would guarantee it never exists, though that slightly complicates the output. Acceptable as-is for a talk demo.

---

Test Coverage

• MistKitClientFactoryTests.swift is updated — good.
• MistDemoConfig+Testing.swift is updated — good.
• No test coverage for DemoErrorsRunner itself (expected for a demo-only command, but worth calling out if this moves into a CI-checked path).
• The --bad-credentials flag path in MistKitClientFactory.create(for:) (badCredentialsNotSupportedOnPublicDatabase) should have a unit test to confirm the guard fires correctly. This is a user-facing error and easy to regress.

---

Minor

• DemoErrorsConfig.swift defines ErrorScenario as a public enum but DemoErrorsRunner is internal. The visibility mismatch is harmless but inconsistent — ErrorScenario could be internal unless there is a reason to expose it to the module boundary.
• describe(staleTag) helper referenced in runConflict — I didn't see its definition in the diff. Confirm it's defined somewhere visible (likely a private helper on DemoErrorsRunner).

---

Verdict

Solid PR. The main ask before merging: document (or add a guard with a printed warning) the 401 demo's silent --database override, confirm the breaking default-database change is intentional and communicated in setup instructions, and add a test for badCredentialsNotSupportedOnPublicDatabase.

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR bundles two well-scoped features for the "CloudKit as Your Backend" talk:

1. Database flag unification — moves the database field into MistDemoConfig so every command can target any scope without rebuilding.
2. demo-errors command — live walkthroughs of 401/404/409 error scenarios with typed CloudKitError handling.

The architecture is solid overall. The key concerns are a potential compile error, one breaking public API change, and a few medium-priority security/test gaps.

---

🔴 High Priority

1. error.httpStatusCode in catch-all may not compile

In DemoErrorsRunner.runConflict(), the catch-all block uses:

} catch {
    printCloudKitError(error, expectedStatus: 409)
    if error.httpStatusCode == 409 {

httpStatusCode is a property on CloudKitError, not Error. This will only compile if there is an extension Error that adds it. If that extension returns nil for non-CloudKitError errors, the condition silently evaluates to false for any unexpected error type — the cleanup still runs, but the intent is obscured. Prefer an explicit cast:

if let ckError = error as? CloudKitError, ckError.httpStatusCode == 409 {

2. AuthTokenError nesting is a breaking public API change

The AuthTokenCommand.swift re-indentation moves public enum AuthTokenError from file scope into the AuthTokenCommand struct body. Any external caller that references AuthTokenError by its unqualified name will now need AuthTokenCommand.AuthTokenError. If there are any consumers of MistDemoKit, this is source-breaking. Verify no callers use the unqualified name, or add a typealias AuthTokenError = AuthTokenCommand.AuthTokenError at file scope.

---

🟡 Medium Priority

3. badCredentials flag is file-readable — can persist unintentionally

bad.credentials is parsed from the standard config-file hierarchy. If a user's config.json persists "bad.credentials": true, every subsequent command invocation silently uses placeholder tokens. Consider restricting this flag to command-line arguments only, so it cannot be accidentally persisted.

4. XSS risk in displayUserInfo JS

AuthTokenIndexHTML builds HTML by concatenating userRecordName, firstName, lastName, emailAddress, zone names, and capabilities directly into innerHTML. If any field contains HTML/script content from a malicious server response, it executes. This is localhost-only, so practical risk is low, but prefer textContent / document.createTextNode for user-controlled strings.

5. AuthTokenIndexHTML hardcodes environment: 'development'

The JS CloudKit.configure(...) call always uses environment: 'development' regardless of config.environment. A user running auth-token against a production container will authenticate against the wrong environment. The server-rendered config object should pass the environment string through to the JS.

6. Missing tests for DemoErrorsConfig and DemoErrorsRunner

DemoErrorsRunner.swift (233 lines) and DemoErrorsConfig.swift have no corresponding test files. At minimum:

• DemoErrorsConfig.init: verify "all", "401", "404", "409" parse correctly, and invalid strings throw DemoErrorsError.invalidScenario
• DemoErrorsRunner: verify scenario dispatch and output formatting against a mock service

---

🔵 Low Priority

7. Dead code in DemoErrorsConfig.init

configuration.string(forKey: "scenario", default: "all") ?? "all"

The ?? "all" is unreachable — default: "all" guarantees a non-nil return. Remove it.

8. MistDemoConfig.with(database:) is fragile at 18 parameters

Every future field addition requires updating both the memberwise init and the with(database:) copy method in sync. A simpler approach:

public func with(database: MistKit.Database) -> MistDemoConfig {
    var copy = self
    copy.database = database  // requires var stored property or a mutable copy approach
    return copy
}

If the struct fields can be made var internally, this eliminates the fragile 18-argument mirror.

9. Test helper defaults .private, production defaults .public

MistDemoConfig+Testing.swift defaults to database: .private while the CLI defaults to .public. Tests that don't explicitly set database silently target .private. Consider aligning or adding a comment documenting the intentional divergence.

10. TestPrivateConfig database pinning lacks a test

The new with(database: .private) pin is intentional and commented — but there is no test asserting that passing --database public is silently overridden to .private. Worth adding to prevent a future refactor from removing the pin accidentally.

11. DemoErrorsRunner should be Sendable

The struct holds only Sendable types and all methods are async. Adding : Sendable conformance is free and prevents future actor-isolation warnings if the runner is ever passed across task boundaries.

---

Summary Table

Priority	Issue
🔴 High	error.httpStatusCode in catch-all — prefer explicit CloudKitError cast
🔴 High	AuthTokenError nesting is a breaking public API change
🟡 Medium	badCredentials flag readable from config file — could persist accidentally
🟡 Medium	displayUserInfo inserts server strings into innerHTML without escaping
🟡 Medium	AuthTokenIndexHTML hardcodes environment: 'development'
🟡 Medium	No unit tests for DemoErrorsConfig or DemoErrorsRunner
🔵 Low	Dead ?? "all" in DemoErrorsConfig.init
🔵 Low	with(database:) 18-field copy is fragile; var copy = self pattern preferred
🔵 Low	Test helper .private default diverges from production .public default
🔵 Low	No test for TestPrivateConfig database pinning behavior
🔵 Low	DemoErrorsRunner should declare : Sendable

Overall this is a clean, well-structured PR. The two high-priority items should be verified before merge; the rest can be addressed here or in follow-ups.

🤖 Generated with Claude Code

[claude]

Code Review — PR #282: MistDemo: --database flag + demo-errors command

Overview

This PR moves database selection out of individual command call sites and into MistDemoConfig as a first-class database property, then threads it through MistKitClientFactory.create(for:). It also adds --bad-credentials for in-context 401 demos and a new demo-errors command that walks 401/404/409 scenarios against a live container.

---

✅ What's well done

• Single source of truth for database — MistDemoConfig.database replaces 9 scattered hardcoded .private/.public arguments. create(for:) is a cleaner call site than create(_:from:).
• badCredentials isolation — constructing a separate service with placeholder tokens rather than mutating real config is the correct approach. The flag never touches CLOUDKIT_API_TOKEN or CLOUDKIT_WEB_AUTH_TOKEN.
• TestPrivateConfig correctly pins database — parsedBase.with(database: .private) ensures test-private ignores any --database flag the user supplies. The ordering (parse → pin → validate) is right.
• makeBadCredentialsTokenManager() is internal and shared — DemoErrorsRunner.runUnauthorized and the factory's badCredentials short-circuit use the same definition. Good DRY.
• DemoErrorsRunner.runConflict does a real round-trip — three actual CloudKit calls, not a mock. This produces a genuine HTTP 409 and exercises the error path a user would hit in production.
• Test coverage for badCredentials — 5 new @Test cases cover private, shared, public-throws, false-regression, and token format validation. Particularly good: badCredentialsTokenManagerFormatPassesLocalValidation verifies the 64-char hex + ≥10-char requirement so the demo token actually reaches Apple's servers.

---

⚠️ Issues and suggestions

1. Type mismatch in runConflict's catch clause (potential compile error)

// runConflict, step 3:
} catch {
    printCloudKitError(error, expectedStatus: 409)  // error: Error, but fn takes CloudKitError
    if error.httpStatusCode == 409 {               // httpStatusCode not on Error

catch with no pattern binds error as any Error. printCloudKitError takes CloudKitError, and httpStatusCode is not a standard Error property. Compare with runUnauthorized and runNotFound, which correctly use catch let error as CloudKitError { … }. If this compiles today it's likely because there's an Error extension providing httpStatusCode as a nil-returning fallback — but the function call still passes Error where CloudKitError is expected, which is a type error in Swift unless there's a protocol or overload I'm not seeing. Please verify this compiles cleanly and, if so, note why in a comment. If it doesn't compile, the fix is:

} catch let error as CloudKitError {
    printCloudKitError(error, expectedStatus: 409)
    if error.httpStatusCode == 409 { … }
} catch {
    print("❌ Unexpected non-CloudKit error: \(error)")
}

2. Breaking default database change for existing users

Commands that previously hardcoded .private (create, update, delete, lookup, modify, current-user, upload-asset, lookup-zones, fetch-changes) now default to .public. This is documented in the PR and is intentional, but it's a silent behavior change for any script or CI step calling these commands without --database private. Anyone already relying on the old defaults will get unexpected results. At minimum, the CHANGELOG or migration note should call this out explicitly.

3. with(database:) requires a 17-parameter internal init

MistDemoConfig now has a bespoke memberwise init with 17 parameters just to support with(database:). This is fragile — adding a new property to MistDemoConfig requires updating this init, the ConfigurationParseable init, and the test helper. Consider using a copy-on-write struct trick or at least a comment at the top of the internal init warning maintainers to keep it in sync. Alternatively, making the struct's stored properties var internal (not let) and using mutating func with(database:) -> Self is simpler.

4. No unit tests for DemoErrorsRunner

The 409 scenario (create → update → stale-update → cleanup) is the most complex flow in this PR and has no unit test. At minimum, a test that mocks CloudKitService and verifies the three-step sequence and cleanup call would catch regressions without requiring a live container.

5. DemoErrorsConfig does not validate scenario against the ErrorScenario.allCases enum

The validation is:

guard let scenario = ErrorScenario(rawValue: scenarioString) else {
    throw DemoErrorsError.invalidScenario(scenarioString)
}

This is correct. The error message already lists valid values via ErrorScenario.allCases. No issue here — just confirming it's correct.

---

Minor nits

• AuthTokenCommand.swift / AuthTokenIndexHTML.swift changes are pure re-indentation matching the #if canImport(Hummingbird) block scope. No behavioral change; safe to ignore in review.
• TestIntegrationConfig.swift: removing public import MistKit and the local database property is correct, since database now lives in MistDemoConfig. The de-duplication is good.
• TestPrivateConfig.swift: the public import MistKit → import MistKit downgrade is correct (no re-export needed).

---

Verdict

Approve with one blocking item. The database-flag refactor and demo-errors command are well-designed and the test coverage for badCredentials is solid. Before merge, please confirm the type mismatch in runConflict's catch clause (#1) compiles correctly and behaves as intended — that's the only item I'd consider blocking. The behavior-change documentation (#2) and with(database:) fragility (#3) are worth addressing but are not correctness bugs.

🤖 Generated with Claude Code