Adopt configkeykit in celestracloud

[leogdion]

No description provided.

[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: 1d5240d0-7bbb-483a-b4b6-f28b3682c190

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 405-adopt-configkeykit-in-celestracloud

---

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 — PR #406: Adopt ConfigKeyKit in CelestraCloud

Overview

This PR replaces the dual-string constant pattern (containerID + containerIDEnv) in CelestraCloud's configuration layer with typed ConfigKey<T> / OptionalConfigKey<T> values backed by the ConfigKeyKit package. The bulk of the line count (+300/-121) is mechanical: copyright year updates (2025 → 2026) and full license header standardization across BushelCloud test files, plus extending lint.sh to run header.sh against the Tests/ directory.

The core logic change is small and well-targeted. Overall this is a clean refactor.

---

What's Good

• Eliminates key duplication. Every config key previously needed two string constants (CLI-style dot-key and ENV-style SCREAMING_SNAKE_CASE). The new typed keys collapse that to one declaration with a single base name.
• Defaults are colocated with keys. Moving 2.0, false, and CloudKitConfiguration.defaultContainerID into ConfigKey declarations removes magic numbers from loadConfiguration().
• loadConfiguration() is dramatically cleaner. The old ?? read(cliKey) ?? read(envKey) ?? defaultValue chains are gone.
• Docstring in ConfigurationKeys.swift clearly explains the naming-style derivation (StandardNamingStyle: dash-case → SCREAMING_SNAKE_CASE), which would otherwise be invisible to readers.

---

Issues / Suggestions

Potential bug — read(_ key: OptionalConfigKey<Date>) silent failure

readDate(forKey:) uses:

configReader.string(forKey: ConfigKey(key), as: Date.self)

If the Configuration library's string(forKey:as:) doesn't perform ISO8601 conversion here, this silently returns nil instead of throwing, and lastAttemptedBefore is always nil regardless of what the user passes. The comment says "Swift Configuration automatically converts ISO8601 strings to Date" — it would be worth a unit test confirming this path actually parses a date string, since a silent nil is hard to diagnose in production.

Implicit reliance on ConfigKeySource.allCases ordering

All five read() overloads (except Bool) iterate ConfigKeySource.allCases and return the first match:

for source in ConfigKeySource.allCases { … }

This depends on allCases being ordered CLI-first, then ENV. If ConfigKeyKit ever reorders the cases the precedence silently flips. A short comment noting this assumption would help future readers.

Bool handling is inconsistent with other types

read(_ key: ConfigKey<Bool>) is the only overload that does not use the for source in allCases loop — instead it manually checks CLI then ENV with custom parsing (presence = true for CLI; true/1/yes for ENV). This is correct given how boolean flags work, but the asymmetry can confuse readers. The existing docstring explains it well; adding a single inline // non-loop: flag presence semantics differ at the top of the method would make the divergence immediately visible before the reader gets to the docstring.

No new tests for the new read() generics

ConfigurationLoaderTests.swift and FetchConfigurationTests.swift received only license-header updates. The new read(_ key: ConfigKey<T>) / read(_ key: OptionalConfigKey<T>) helpers — especially the Bool flag-presence logic and the Date path — have no unit test coverage. Since these helpers encode meaningful behaviour (what counts as truthy for a Bool ENV var, CLI precedence over ENV), tests here would be valuable.

Five near-identical read() overloads

This is a minor structural note rather than a bug: the pattern

for source in ConfigKeySource.allCases {
    guard let keyString = key.key(for: source) else { continue }
    if let value = readXxx(forKey: keyString) { return value }
}
return key.defaultValue // or nil

is repeated for String, Double, OptionalString, OptionalInt, and OptionalDate. If ConfigKeyKit gains a conformance hook (e.g., ConfigKeyReadable) this could collapse to one generic. Not blocking, just a note for a future iteration.

readDouble removed but comment section name not updated

The // MARK: - Per-key-string Primitives section now contains readString and readInt, which are implementation details called only by the new generics — readDouble and readBool were removed. Consider renaming the mark to // MARK: - Low-level Reader Primitives or similar to reflect that these are no longer the primary reading surface.

---

Summary

Category	Finding
Potential bug	Date read() may silently return nil — worth a targeted test
Design	ConfigKeySource.allCases ordering assumption undocumented
Design	Bool overload asymmetry — documented but visually surprising
Test coverage	No new tests for new read() generics (especially Bool and Date)
Minor	5 near-identical overloads; section comment name stale

The mechanical changes (copyright years, license headers, lint script) are correct. The ConfigKeyKit adoption is a clear improvement. The main asks before merge are a test confirming Date parsing works end-to-end, and a comment noting the allCases ordering dependency.

[codecov]

Codecov Report

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

Additional details and impacted files

@@               Coverage Diff                @@
##             v1.0.0-beta.3     #406   +/-   ##
================================================
  Coverage                 ?   72.12%           
================================================
  Files                    ?      156           
  Lines                    ?     3698           
  Branches                 ?        0           
================================================
  Hits                     ?     2667           
  Misses                   ?     1031           
  Partials                 ?        0           

Flag	Coverage Δ
spm	71.74% <ø> (?)
swift-6.1-jammy	71.92% <ø> (?)
swift-6.1-noble	71.76% <ø> (?)
swift-6.2-jammy	71.81% <ø> (?)
swift-6.2-noble	71.62% <ø> (?)
swift-6.3-jammy	71.84% <ø> (?)
swift-6.3-noble	71.59% <ø> (?)

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: Adopt ConfigKeyKit in CelestraCloud

Overview

This PR migrates CelestraCloud's configuration system from parallel string constant pairs (one for CLI, one for env var) to typed ConfigKey<T>/OptionalConfigKey<T> instances from the new ConfigKeyKit package. The refactor eliminates a verbose readX ?? readXEnv ?? default chain pattern throughout ConfigurationLoader, removes two now-redundant source files (ConfigSource.swift, ConfigurationError.swift), and cleans up a deprecated createCloudKitService() method.

---

What Works Well

• Typed keys are a clear improvement. Replacing dual string constants with a single ConfigKey<String>("cloudkit.container-id", default: …) eliminates the bug-prone parallel naming where containerID and containerIDEnv had to stay in sync manually.
• Import visibility is correct. internal import ConfigKeyKit and the public import Foundation → internal import Foundation fix in CelestraConfig.swift both follow the project's explicit-import convention.
• Removed dead weight cleanly. Deleting ConfigSource.swift, ConfigurationError.swift, and the deprecated createCloudKitService() reduces surface area without breaking anything.
• CI wiring is thorough. ConfigKeyKit setup is added to all four relevant job paths across three workflow files, including the conditional cache check in update-feeds.yml.
• lint.sh fix for BushelCloud Tests/ is a correct housekeeping catch.

---

Issues and Suggestions

1. Implicit dependency on ConfigKeySource.allCases ordering

All read(_:) overloads rely on ConfigKeySource.allCases producing keys in CLI → ENV priority order:

for source in ConfigKeySource.allCases {
    guard let keyString = key.key(for: source) else { continue }
    if let value = readString(forKey: keyString) { return value }
}

This silently breaks if ConfigKeyKit ever reorders its cases. Consider adding a comment that the priority contract depends on ConfigKeyKit's CaseIterable ordering, and checking whether ConfigKeyKit documents this as stable.

2. Missing read overload for OptionalConfigKey<Bool>

Required-default overloads exist for String, Double, Bool. Optional overloads exist for String, Int, Date. There is no read(_ key: OptionalConfigKey<Bool>). No current key needs this, but the gap is a future footgun — a developer adding an optional boolean key will miss the pattern or write an inconsistent one-off. Either add the overload now or add a comment noting the intentional omission.

3. Boolean CLI-flag semantics are implicit

read(_ key: ConfigKey<Bool>) treats any non-nil string from the CLI source as true (flag presence):

if let cliKey = key.key(for: .commandLine),
  configReader.string(forKey: ConfigKey(cliKey)) != nil {
  return true
}

The doc comment explains this, but there are no tests for this behavior. If a user passes --update-skip-robots-check false, the flag still evaluates to true because the value exists. This is probably intentional, but worth a note in the doc comment: "passing any value, including 'false', is treated as true; omit the flag entirely to get the default." A unit test for this edge case would also help.

4. readDate helper is inconsistent with other private helpers

readString and readInt delegate to configReader.string/int(forKey:). But readDate uses:

configReader.string(forKey: ConfigKey(key), as: Date.self)

Passing as: Date.self to a method named string(forKey:) is surprising — it implies an overload or conversion that isn't obvious from the call site. The comment explains why, but it leaks a non-obvious API detail upward. If the Swift Configuration framework has a dedicated date(forKey:) method, prefer that; otherwise a brief comment at the call site (not just at the method definition) would help future readers.

5. source removed from EnhancedConfigurationError reduces debuggability

Removing source: ConfigSource? is a simplification, but it degrades the diagnostic value of configuration errors — callers can no longer distinguish "this key was missing from the CLI" vs "this key was missing from the environment." If ConfigKeyKit surfaces source information through its own API, consider threading that through instead of dropping it entirely.

6. CI: missing v prefix on MISTKIT_BRANCH

MISTKIT_BRANCH changed from v1.0.0-beta.2 to 1.0.0-beta.2 across all three workflow files. If this is a tag format correction, fine — but confirm the tag 1.0.0-beta.2 (without v) actually exists in MistKit. If the real tag is v1.0.0-beta.2, CI will silently fall back to branch resolution behavior and may pick up unintended commits.

7. Missing blank line in one CI job (minor)

In CelestraCloud.yml the Linux test job, the Setup ConfigKeyKit step is immediately followed by - uses: brightdigit/swift-build@v1 with no blank line between them, unlike the other jobs. Not functional, but inconsistent.

---

Test Coverage

• Existing ConfigurationLoaderTests.swift and FetchConfigurationTests.swift get proper license headers — good.
• No new tests cover the read(_:) overloads. At minimum, the boolean CLI-flag behavior (flag-present = true regardless of value) and CLI → ENV priority ordering warrant unit tests, as they are the two non-obvious behavioral contracts of the new implementation.

---

Summary

The direction is correct and the code is meaningfully cleaner than before. Main actionable items before merging:

1. Verify ConfigKeySource.allCases ordering is documented as stable in ConfigKeyKit, or add a guard/comment.
2. Confirm the 1.0.0-beta.2 tag (no v) exists in MistKit before CI relies on it.
3. Add tests for bool flag semantics and source priority in ConfigurationLoaderTests.

Everything else is minor polish.