Fix CI failures + Claude review nits on PR #298 (v1.0.0-beta.1)

[leogdion]

Resolves the six failing checks and eight code-review comments from #298. Plan tracked in .claude/plan-pr298.md; audit in .claude/ci-failures-pr298.md.

Summary

• Compile blockers — BushelCloud + CelestraCloud no longer pass the now-removed database: argument to CloudKitService.init; CelestraCloud's CloudKitRecordOperating conformance gets explicit witness methods bridging the protocol's no-database signatures with CloudKitService's database-aware ones; CelestraError.isCloudKitErrorRetriable covers the newly-added missingCredentials and invalidPrivateKey cases. CelestraCloud queries migrate off the deprecated single-page queryRecords to queryAllRecords / continuation-marker pagination.
• UpdateReport review fixes — Summary.successRate and UpdateReport.duration are now stored properties (so they appear in the Codable JSON output), and FeedResult.status is a Codable enum (.success, .error, .skipped, .notModified) rather than a magic-string String.
• BushelCloudKitService review fixes — fetchExistingRecordNames switches to service.queryAllRecords(recordType:, desiredKeys: []) to stop overfetching every field; the misleading reason=\(result.recordType) log line drops the bogus reason segment.
• MockCloudKitRecordOperator thread-safety — replaces nonisolated(unsafe) with a Mutex-backed state struct so the mock is safe under Swift Testing's parallel execution. No test-site changes.
• watchOS test flake — cancelsOtherTasks is wrapped in withKnownIssue(isIntermittent: true), mirroring the existing pattern in AsyncHelpersTests+Timeout.swift.
• Lint sweep — CloudKitService path-builder extension moved into CloudKitService+Paths.swift (clears file_length); Credentials.makeTokenManager split into a thin dispatcher + three private helpers (clears cyclomatic_complexity and function_body_length); MockRecordManagingService adds an explicit queryAllRecords(recordType:) override (drops the deprecated default impl); MistDemo try drops on non-throwing CloudKitService inits, deprecated queryRecords migration in DemoErrorsRunner, and ExistentialAny fixes across OutputEscaperFactory, OutputFormatterFactory, AnyCodable, FieldsInput. Periphery cleanups deferred — see v1.0.0-beta.1 follow-up: Periphery unused-symbol cleanups #317.
• Coverage tests — four new files (FetchChangesConfigTests, LookupZonesConfigTests, DemoErrorsConfigTests, CommandLineParserTests) lift MistDemo's MistDemoKit/ConfigKeyKit patch coverage above the codecov target.

Out of scope (filed as follow-ups)

• v1.0.0-beta.1 follow-up: Periphery unused-symbol cleanups #317 — Periphery cleanups (~30 unused symbols across MistKit + MistDemo)
• CelestraCloud: updateFeedMetadata reports .error on partial success #318 — updateFeedMetadata partial-success semantics
• CelestraCloud: refactor ExitError (use ExitCode or carry a message) #319 — ExitError refactor
• CelestraCloud: bump 2025 → 2026 copyright in CelestraErrorTests files #320 — Copyright year alignment for two CelestraErrorTests files
• CI: upgrade github/codeql-action v3 → v4 before December 2026 deprecation #321 — CodeQL Action v3 → v4 upgrade
• CodeQL alerts — to be dismissed via gh api -X PATCH after this PR opens; see plan for the rationale (caller-supplied debug input redacted by SecureLogging unless MISTKIT_DISABLE_LOG_REDACTION=1).

Test plan

Local verification done before push (per feedback_test_lint_before_commit):

• MistKit: swift build clean, swift test 481/481 pass, ./Scripts/lint.sh clean.
• MistDemo: swift build clean, swift test 861/861 pass (was 844 — 17 new), ./Scripts/lint.sh clean.
• BushelCloud: swift build clean, swift test 220/220 pass, ./Scripts/lint.sh clean.
• CelestraCloud: swift build clean, swift test 115/115 pass, ./Scripts/lint.sh clean.
• swift run mistdemo test-integration --record-count 5 --asset-size 10 — all 12 phases green against real CloudKit (public DB).
• swift run mistdemo test-private --record-count 5 --asset-size 10 — all 12 phases green against real CloudKit (private DB).

CI checks to watch on the PR:

• Test BushelCloud on Ubuntu — green (was red on database: compile error).
• Test CelestraCloud on Ubuntu — green (was red on deprecation-as-error).
• Build on macOS (Platforms) (watchOS) — green or red-with-known-issue (the withKnownIssue wrap is non-load-bearing on success).
• codecov/patch — ≥ 25.58%.
• CodeQL — still red until alerts are dismissed via gh api.
• CodeFactor — likely green now that the underlying lint is cleaner.

Commits

All commits use [skip ci] to keep individual pushes from triggering CI; the PR open + final pushes will trigger the full check suite.

🤖 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: 919d5816-1eb1-4e97-a7e6-3075cc32fe95

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 plans/pr298-fixes

---

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 86.94030% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.94%. Comparing base (3e7aa7d) to head (4751e50).

Files with missing lines	Patch %	Lines
...rations.discoverAllUserIdentities.Input.Path.swift	0.00%	7 Missing ⚠️
...s/InputPaths/Operations.listZones.Input.Path.swift	0.00%	7 Missing ⚠️
...putPaths/Operations.lookupRecords.Input.Path.swift	0.00%	7 Missing ⚠️
...putPaths/Operations.modifyRecords.Input.Path.swift	0.00%	7 Missing ⚠️
.../Extensions/CloudKitService+LookupOperations.swift	0.00%	4 Missing ⚠️
...ce/Extensions/CloudKitService+ZoneOperations.swift	66.66%	2 Missing ⚠️
...ources/MistDemoKit/Commands/DemoErrorsRunner.swift	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           v1.0.0-beta.1     #322      +/-   ##
=================================================
+ Coverage          67.25%   67.94%   +0.68%     
=================================================
  Files                503      520      +17     
  Lines              14062    14174     +112     
=================================================
+ Hits                9458     9631     +173     
+ Misses              4604     4543      -61     

Flag	Coverage Δ
mistdemo-spm-macos	55.99% <39.55%> (+0.52%)	⬆️
mistdemo-swift-6.2-jammy	56.00% <39.55%> (+0.52%)	⬆️
mistdemo-swift-6.2-noble	56.00% <39.55%> (+0.52%)	⬆️
mistdemo-swift-6.3-jammy	56.00% <39.55%> (+0.54%)	⬆️
mistdemo-swift-6.3-noble	56.00% <39.55%> (+0.52%)	⬆️
spm	51.08% <76.39%> (+0.07%)	⬆️
swift-6.1-jammy	51.68% <78.88%> (+0.70%)	⬆️
swift-6.1-noble	51.48% <76.39%> (+0.50%)	⬆️
swift-6.2-jammy	51.48% <76.39%> (+0.04%)	⬆️
swift-6.2-noble	51.65% <76.39%> (+0.10%)	⬆️
swift-6.3-jammy	51.37% <78.88%> (+0.33%)	⬆️
swift-6.3-noble	51.25% <76.39%> (+0.27%)	⬆️

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 #322

Overview

Solid, well-scoped CI-fix PR that addresses six failing checks and eight review comments from #298. The description is excellent — each fix is explained with the root cause, which makes the diff easy to audit section by section. 102 files changed, but the vast majority are file renames/moves with no logic changes.

---

Strengths

• MockCloudKitRecordOperator Mutex refactor — Replacing nonisolated(unsafe) with a Mutex-backed State struct is the right call. Reads and mutations are atomic without any test-site churn, and the QueryCall/ModifyCall structs picking up Sendable conformance is a nice touch.
• FeedResult.Status enum — String → typed enum eliminates an entire class of silent mismatch bugs. The raw values ("success", "error", "skipped", "notModified") are consistent with what the old code was writing, so serialized JSON from previous runs deserializes correctly.
• fetchExistingRecordNames optimization — Passing desiredKeys: [] is the right fix; fetching every field just to extract recordName was needlessly expensive at scale.
• Misleading log line fixed — reason=\(result.recordType) was logging the record type, not an error reason. The minimal fix (drop the bogus segment) is correct.
• repeat...while pagination — Semantically matches the intent (guard !feeds.isEmpty else { break } is still present as a safety valve; the continuationMarker == nil condition is the true termination). Cleaner than the while true / if count < limit { break } heuristic.
• internal import corrections — Logging, BushelUtilities, Foundation demoted from public import where they were leaking as transitive dependencies. Good hygiene.
• Credentials.makeTokenManager split — Three extracted helpers (makeUserContextTokenManager, makePublicTokenManager, makePrivateOrSharedTokenManager) each fit in a mental chunk; the SwiftLint violations on cyclomatic_complexity and function_body_length are now structurally prevented rather than suppressed.

---

Issues / Suggestions

1. .claude/ planning files being committed

.claude/ci-failures-pr298.md and .claude/plan-pr298.md are AI session artifacts — they're useful while authoring the PR, but have no value to future contributors reading the repo history. The PR description already says "Plan tracked in .claude/plan-pr298.md", which becomes self-referential after the branch merges into v1.0.0-beta.1.

Consider either (a) adding .claude/*.md to .gitignore so they're never committed, or (b) deleting both files in a final cleanup commit before the merge. Otherwise these grow unboundedly with each AI-assisted PR cycle.

2. queryRecords forwarding hardcodes .public without a visible warning

In CloudKitRecordOperating.swift, the new conformance extensions forward to database: .public:

/// Satisfy CloudKitRecordOperating's `queryRecords` (no database param) by forwarding to the public-database overload.
public func queryRecords(...) async throws(CloudKitError) -> [RecordInfo] {
  let result: QueryResult = try await queryRecords(
    ...,
    database: .public   // ← hardcoded
  )
  return result.records
}

The inline comment documents that it forwards to .public, but the public doc comment on the method doesn't warn callers. Any future CloudKitRecordOperating adopter who targets .private will get silently wrong behavior. The doc comment should say: "Note: Always queries the public database. Use queryRecords(_:...:database:) for private or shared databases."

3. Missing round-trip test for FeedResult.Status raw values

DemoErrorsConfigTests has a rawValues() test for ErrorScenario, but there's no equivalent for the new FeedResult.Status enum. Since JSON persistence is the stated motivation for the change, a short test asserting:

#expect(Status.notModified.rawValue == "notModified")
#expect(try JSONDecoder().decode(Status.self, from: Data("\"notModified\"".utf8)) == .notModified)

...would lock in the backwards-compatibility guarantee. The behavior is correct as written, but an explicit test is the right place to document the constraint.

4. withKnownIssue(isIntermittent: true) — comment overstates the root cause

The comment says the flake has the "same root cause" as the throwsOnTimeout / returnsAsyncValue tests in AsyncHelpersTests+Timeout.swift. Those tests are gated by a wasm32 #if, whereas cancelsOtherTasks is wrapped unconditionally across all platforms. If this only flakes on watchOS/simulator cooperative executors, a platform guard (#if os(watchOS)) would be more precise than a blanket withKnownIssue that silently absorbs failures on macOS/Linux/Ubuntu CI too. That said, if the pattern is already established in the codebase and the test is non-load-bearing, this is a minor nit.

---

Minor Nits

• CloudKitService+Celestra.swift line 32: internal import Logging — fine, but worth noting the compiler warning driving this change in the commit message rather than just as a diff line, so future readers don't wonder why it was demoted.
• The devcontainer lock file churn (.devcontainer/swift-6.1/devcontainer-lock.json) is expected for version bumps, but a quick CI matrix check that the updated lock still resolves cleanly would be reassuring.

---

Summary

The compile blockers are fixed cleanly, the thread-safety refactor is idiomatic, and the stored-property / enum improvements are the right long-term design. The main actionable ask is deciding what to do with the .claude/ planning files before merging to v1.0.0-beta.1. Everything else is documentation/test polish.

🤖 Generated with Claude Code

[claude]

Code Review — PR #322 (Fix CI Failures + Review Nits on PR #298)

This PR is well-structured and addresses a clear set of problems with good motivation for each change. The test plan is thorough and the implementation is generally sound. Below is my detailed feedback.

---

What's good

MockCloudKitRecordOperator thread-safety (strongest change in the PR)

Replacing nonisolated(unsafe) with a Mutex-backed State struct is exactly the right approach for making a mock safe under Swift Testing's parallel executor. The computed-property wrappers (queryCalls, modifyCalls, queryRecordsResult, modifyRecordsResult) keep test-site syntax unchanged, which is clean. Minor note: the two reads queryCalls and modifyCalls acquire separate locks, so a test that reads both could observe a torn snapshot. This is very unlikely to matter in practice for a mock, but worth being aware of.

FeedResult.Status enum

Replacing the magic-string String with a typed Status: String, Codable, Sendable enum is a clear improvement. The rawValue encoding matches the previous string values ("success", "error", "skipped", "notModified"), so JSON round-trips should be backward-compatible.

UpdateReport.duration and Summary.successRate as stored properties

Correct fix — computed properties are not synthesised into Codable's encode(to:) / init(from:), so they were silently absent from serialized output. Moving the computation into the initialiser and storing the result is the right pattern.

deleteAllFeeds pagination rewrite

The old heuristic (feeds.count < 200 → break) would terminate too early when a full page had no continuation marker (and would loop forever if CloudKit always returned 200 without exhausting the cursor). The new repeat…while continuationMarker != nil pattern is semantically correct.

Credentials+TokenManager refactor

Extracting makeUserContextTokenManager, makePublicTokenManager, and makePrivateOrSharedTokenManager from the monolithic makeTokenManager is a textbook complexity-reduction refactor. Behaviour is preserved; each helper is now independently readable and testable.

CelestraError.isCloudKitErrorRetriable completeness

The missingCredentials and invalidPrivateKey cases returning false is correct — these errors indicate a configuration problem, not a transient failure, so retrying would just burn quota.

---

Issues / suggestions

1. deleteAllFeeds: silent stop on empty intermediate page

let result: QueryResult = try await queryRecords(...)
let feeds = result.records

guard !feeds.isEmpty else {
    break  // No more feeds to delete
}
// ...
continuationMarker = result.continuationMarker
} while continuationMarker != nil

If CloudKit ever returns an empty page with a non-nil continuation marker (unusual but not impossible per the spec), the guard exits before the marker is updated, so the remaining pages are silently skipped. Consider updating the marker before the guard, or restructuring:

continuationMarker = result.continuationMarker
guard !feeds.isEmpty else { break }

2. CloudKitRecordOperating bridge — single-page queryRecords wrapper

public func queryRecords(
    recordType: String, filters: ..., sortBy: ..., limit: Int?, desiredKeys: ...
) async throws(CloudKitError) -> [RecordInfo] {
    let result: QueryResult = try await queryRecords(..., continuationMarker: nil, database: .public)
    return result.records
}

The bridge intentionally returns only the first page. That's fine for backward compat with the protocol, but the method has no doc comment warning callers that results may be truncated when the result set exceeds limit. Any future implementer of CloudKitRecordOperating who routes through this bridge could be surprised. A one-line note would prevent future confusion.

3. withKnownIssue(isIntermittent: true) wrapping

The wrapping is correct and mirrors the existing pattern. One small observation: when the test does fail consistently (i.e., the cooperative executor consistently resolves the race the wrong way), the isIntermittent: true flag means Swift Testing still emits a known-issue pass, which may hide a real regression. The existing pattern is already doing this in AsyncHelpersTests+Timeout.swift, so this is consistent; just worth noting when triaging future CI output.

4. queryAllRecords in BushelCloudKitService.fetchExistingRecordNames

let records = try await service.queryAllRecords(
    recordType: recordType,
    desiredKeys: []
)

desiredKeys: [] is the right choice to avoid over-fetching fields — only ___recordID is needed. Good.

5. File reorganisation churn

~50 files are renamed (moved into subdirectories). The content is unchanged for most of them, which is good for correctness. Two concerns:

• git blame history becomes harder to follow at these paths.
• Any open branches touching these files will have merge conflicts. Worth calling out in the PR body that follow-on branches need a rebase.

This is a style/process note, not a code-correctness issue.

6. Minor: comment line length in CloudKitRecordOperating.swift

/// Satisfy CloudKitRecordOperating's `queryRecords` (no database param) by forwarding to the public-database overload.

This line is over 100 characters, which will trigger a SwiftLint line_length warning if the project has that rule enabled. Splitting at the period would keep it clean.

---

Test coverage additions

The four new test files (FetchChangesConfigTests, LookupZonesConfigTests, DemoErrorsConfigTests, CommandLineParserTests) are well-structured using @Suite / @Test / #expect throughout. The CommandLineParserTests parameterised test for help tokens (--help, -h, help) is a nice use of arguments:. Coverage motivation is clear.

One gap: there are no tests for the new FeedResult.Status enum's Codable round-trip (encoding notModified → JSON → decoding back). Given that the whole motivation for the change was Codable correctness, a brief round-trip assertion in a CelestraCloud test would give confidence that the rawValue encoding matches what callers expect.

---

Summary

The PR correctly resolves all six CI failures and the eight review items from #298. The Mutex-backed mock, continuationMarker-driven pagination, and stored-property Codable fixes are all solid improvements. The main items worth addressing before merge are the deleteAllFeeds empty-page edge case and the missing truncation warning on the queryRecords bridge. Everything else is low-priority polish.

🤖 Generated with Claude Code