Tag and validate ambiguous FieldValue scalar types (#375)

[leogdion]

Fixes #375. Follow-up tracked in #376.

Problem

CloudKit infers a field's type from its value structure. A .date was serialized as a bare millisecond number, so CloudKit read it as INT64 and rejected it against TIMESTAMP schema fields with BAD_REQUEST — blocking every CelestraCloud record write that includes a timestamp.

Write path

• Added the scalar types to FieldValueRequest.type in openapi.yaml and regenerated MistKitOpenAPI.
• makeScalarRequest now emits type for the three ambiguous scalars: .date→TIMESTAMP, .bytes→BYTES, .double→DOUBLE. Object/array-shaped values and STRING/INT64 stay untagged (unambiguous).

Read path

The generated value oneOf is undiscriminated (first-match-wins: String → Int64 → Double → Bytes → Date), so a whole-millisecond TIMESTAMP decoded as Int64Value and read back as .int64 even with type present. Response conversion (FieldValue+Components+Scalar.swift) now honors an explicit scalar type over the decoded case — recovering TIMESTAMP/DOUBLE from any numeric case and BYTES from any string case.

Fail-loud on contradiction

When a scalar type contradicts the value's category (a numeric tag over a non-number, or a string tag over a non-string), conversion throws the new ConversionError.typeValueMismatch instead of silently coercing — matching the existing unmappableFieldValue philosophy. Strict validation is scoped to scalar tags; complex/list tags stay lenient (follow-up: #376).

Tests

• Request-side tag assertions in FieldValueConversionTests+BasicTypes.swift.
• New FieldValueConversionTests+ResponseTypes.swift covering the read-path round-trips (incl. the whole-millisecond TIMESTAMP→.date case that was broken), the contradiction throws, and the lenient-complex-type boundary.

479 tests pass; build, swift-format, and swiftlint all clean.

Note: this PR also points CelestraCloud's MISTKIT_BRANCH at this branch (via subrepo) so its CI exercises the fix before it lands on v1.0.0-beta.2. That pin should be reverted to a tag before/after merge.

🤖 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: de2dd070-438b-4790-a75f-dd30cbfb3531

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 375-fieldvaluerequest-timestamp

---

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 #377: Tag and validate ambiguous FieldValue scalar types

This is a well-motivated and well-executed fix. The root cause is clearly understood, the write-path and read-path are both addressed, and the new FieldValue+Components+Scalar.swift file cleanly separates scalar conversion concerns from complex-type handling. Overall the design is sound. A few items below worth addressing before merge.

---

Must fix before merge

MISTKIT_BRANCH still points at the feature branch — both CelestraCloud.yml and update-feeds.yml are set to 375-fieldvaluerequest-timestamp. The PR description flags this as a known todo, but it's a merge blocker. Leaving the subrepo pinned to a transient branch means CelestraCloud CI will break the moment this branch is deleted post-merge.

---

Design discussion

INT64 over a fractional double silently defers to inference rather than throwing. In makeTypedScalar, INT64 validates that the value is numeric (requireNumeric succeeds for 3.5), then returns nil to let makeInferredScalar produce .double(3.5). This means {"value": 3.5, "type": "INT64"} returns .double(3.5) — the declared type is quietly ignored. The motivation (avoid truncating information) is reasonable and the test documents it explicitly. But the asymmetry is subtle: a category mismatch (e.g. string tagged INT64) throws, while an intra-category mismatch (fraction tagged INT64) silently coerces. A brief comment in makeTypedScalar on the INT64 arm explaining why it returns nil rather than throwing (or truncating) would help future readers who might see it as an oversight.

BYTES stores the raw base64 string, not decoded bytes. requireString returns the base64-encoded string and .bytes stores it directly — consistent with the pre-existing BytesValue → .bytes path, so this is correct. Just confirming the convention is intentional and not an oversight from the new code path.

---

Minor observations

expectThrows suppresses ConversionFailureReporter.$assertionHandler. The pattern is correct — the @TaskLocal handler is swapped out so the assertionFailure trap doesn't fire in tests, allowing the throw to be observed. A one-line comment on withValue({ _, _, _ in }, ...) noting that it suppresses the debug assertion trap would help readers unfamiliar with this codebase pattern. Something like // suppress the debug assertionFailure so #expect(throws:) can observe the throw.

ASSETID is a new addition to FieldValueRequest._typePayload. It's correctly sourced from the openapi.yaml enum change and the generated Types.swift reflects it. Worth confirming CloudKit actually accepts this type in write contexts (not just reads) before shipping; if it's read-only or internal, it's harmless in the enum but could be confusing.

.allCases ordering change for LIST. LIST moved from the end of the old enum to just before STRING_LIST. I confirmed no code in the codebase iterates .allCases on FieldValueRequest._typePayload, so this is safe.

Sources/MistKitOpenAPI/Types.swift changes are correctly from regeneration, not manual edits — the diff is consistent with running ./Scripts/generate-openapi.sh after the openapi.yaml change. No concern there.

---

Test coverage

The new FieldValueConversionTests+ResponseTypes.swift is thorough:

• The motivating bug (whole-millisecond TIMESTAMP → .date, not .int64) ✓
• Fractional TIMESTAMP ✓
• BYTES/STRING/DOUBLE/INT64 with explicit type ✓
• Contradicting scalar type throws ✓
• Complex type stays lenient (doesn't validate) ✓
• No-type fallback to first-match-wins inference ✓

The #available guards are consistent with existing test style — not a new issue.

---

PR #331

Trivial and correct — copyright year updates only, no functional changes.

---

Overall: Approve once the MISTKIT_BRANCH pin is reverted. The fix is correct and the tests give good confidence.

🤖 Generated with Claude Code

[codecov]

Codecov Report

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

Files with missing lines	Patch %	Lines
...els/FieldValues/FieldValue+Components+Scalar.swift	94.28%	4 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             v1.0.0-beta.2     #377   +/-   ##
================================================
  Coverage                 ?   68.56%           
================================================
  Files                    ?      133           
  Lines                    ?     3025           
  Branches                 ?        0           
================================================
  Hits                     ?     2074           
  Misses                   ?      951           
  Partials                 ?        0           

Flag	Coverage Δ
mistdemo-spm-macos	11.47% <0.00%> (?)
mistdemo-swift-6.2-jammy	11.47% <0.00%> (?)
mistdemo-swift-6.2-noble	11.66% <0.00%> (?)
mistdemo-swift-6.3-jammy	11.47% <0.00%> (?)
mistdemo-swift-6.3-noble	11.47% <0.00%> (?)
spm	69.46% <95.40%> (?)
swift-6.1-jammy	69.64% <95.40%> (?)
swift-6.1-noble	69.29% <95.40%> (?)
swift-6.2-jammy	69.32% <95.40%> (?)
swift-6.2-noble	69.22% <95.40%> (?)
swift-6.3-jammy	69.46% <95.40%> (?)
swift-6.3-noble	69.29% <95.40%> (?)

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 #377: Tag and validate ambiguous FieldValue scalar types

The core fix is correct and well-motivated. The three-layer architecture (typed → inferred → complex) is clean, and the request-side tagging logic is straightforward. Here are my findings, ranked by severity.

---

🔴 Merge blocker — CelestraCloud branch pin

Files: Examples/CelestraCloud/.github/workflows/CelestraCloud.yml:24, Examples/CelestraCloud/.github/workflows/update-feeds.yml:177

Both files pin MISTKIT_BRANCH: 375-fieldvaluerequest-timestamp. After this PR merges and the branch is deleted, every CelestraCloud CI run will either fail to resolve the ref or build against a stale/missing branch. The PR description acknowledges this ("That pin should be reverted to a tag before/after merge") but it reads as a pre-merge requirement, not a post-merge note.

---

🟡 Dead code — BytesValue branch in makeInferredScalar is unreachable

File: Sources/MistKit/Models/FieldValues/FieldValue+Components+Scalar.swift:137

if case .BytesValue(let bytesVal) = value {
    return .bytes(bytesVal)   // never reached
}

The valuePayload oneOf decoder is first-match-wins (String → Int64 → Double → Bytes → Date). A JSON string will always match StringValue before BytesValue, so this branch never executes in practice. The docstring immediately above it explicitly says "a base64 BYTES reads back as .string", which is accurate — the dead BytesValue case below contradicts that intuition. Worth removing (or adding a comment explaining why it's kept as a defensive guard) to avoid misleading future readers about what inference actually does without a type tag.

---

🟡 Weak test assertion — expectThrows checks ConversionError.self, not typeValueMismatch

File: Tests/MistKitTests/Models/FieldValues/FieldValueConversionTests+ResponseTypes.swift:28

#expect(throws: ConversionError.self) {
    _ = try Self.decode(json)
}

The 6 contradictingScalarTypeThrows cases are testing specifically that typeValueMismatch is thrown. If a future change accidentally throws unmappableFieldValue instead (the other ConversionError variant), all 6 assertions still pass. Consider pinning:

#expect {
    _ = try Self.decode(json)
} throws: { error in
    guard let err = error as? ConversionError,
          case .typeValueMismatch = err else { return false }
    return true
}

---

🟡 Semantic surprise — INT64-typed response with fractional value returns .double

File: Sources/MistKit/Models/FieldValues/FieldValue+Components+Scalar.swift:73-75, Tests/…/FieldValueConversionTests+ResponseTypes.swift:111

case .INT64:
    _ = try requireNumeric(value, fieldName: fieldName, declaredType: "INT64")
    return nil   // defers to inference → .double(3.5) for {"value": 3.5, "type": "INT64"}

The test explicitly validates {"value": 3.5, "type": "INT64"} == .double(3.5). I understand the motivation (avoid silently truncating 3.5 → 3), but it means a CloudKit field whose schema declares INT64 returns a Swift .double case. Any caller pattern-matching .int64 on an INT64-typed field would silently miss these values. Since CloudKit shouldn't send fractional values for INT64 fields, this is a low-risk edge case — but it's worth a note in the makeTypedScalar docstring explaining the semantic (it mentions the truncation avoidance but not the consequence for callers).

---

🟠 Precision loss — numericValue converts Int64 to Double

File: Sources/MistKit/Models/FieldValues/FieldValue+Components+Scalar.swift:150

if case .Int64Value(let intVal) = value {
    return Double(intVal)   // loses precision for |intVal| > 2^53
}

For TIMESTAMP fields, this means dates after roughly year 2255 (milliseconds > 2^53 ≈ 9 × 10^15) will have sub-millisecond rounding errors. For DOUBLE fields with large integral values the loss is similarly negligible today. Not a practical concern for current-year timestamps (1_748_000_000_000 < 2^41), but worth a comment since the requireNumeric function is general-purpose.

---

🟠 Shell injection — action.yml single-quote quoting

File: .github/actions/setup-mistkit/action.yml:26

BRANCH='${{ inputs.branch }}'

GitHub Actions template expansion (${{ ... }}) happens server-side before the shell sees the string. A branch name containing a single quote (e.g., it's-a-fix) produces BRANCH='it'$'s-a-fix'' on the shell, breaking the assignment. Low risk for this repo since branch names are controlled, but the standard mitigation is to use ${{ inputs.branch }} only inside env: where GitHub escapes it, or pass it as an environment variable:

env:
  BRANCH: ${{ inputs.branch }}
run: |
  REF=$(git ls-remote ... "$BRANCH" | ...)

---

✅ What's working well

• The split into makeTypedScalar / makeInferredScalar is a clean separation of concerns.
• The requireNumeric / requireString helpers composing category validation + extraction are well-named.
• Fail-loud on typeValueMismatch is consistent with the existing unmappableFieldValue philosophy.
• Request-side tagging in makeScalarRequest is minimal — only the three genuinely ambiguous scalars.
• Test coverage for the round-trip cases (whole-ms TIMESTAMP, whole-valued DOUBLE, BYTES) is solid.
• The BushelCloudKit type annotation fix and counting refactor are correct.
• CI cache-busting via SHA pinning is a good improvement over branch-name pinning.

🤖 Generated with Claude Code

[claude]

Code Review

This is a well-targeted, clearly-reasoned fix. The root-cause diagnosis is solid, the implementation is clean, and the test coverage is good. A few observations below.

---

Correctness / Design

INT64 validation defers to inference for fractional values — intentional inconsistency worth a note

makeTypedScalar validates that INT64 carries a numeric value, then returns nil to defer to inference. This means {"value": 3.5, "type": "INT64"} → .double(3.5), preserving the wire value over the declared type, while {"value": "hello", "type": "INT64"} → throws typeValueMismatch. The test int64DefersToInference covers this explicitly and the CLAUDE.md paragraph explains why (avoid truncation). The deliberate asymmetry — strict for TIMESTAMP/DOUBLE/BYTES, lenient for INT64 — is documented, but a short inline note in makeTypedScalar above the case .INT64: branch would make it immediately obvious to the next reader without needing the CLAUDE.md.

Double precision for Int64 millisecond timestamps

In numericValue, Int64 values are widened to Double (Double(intVal)). Double has 53-bit mantissa (~9 × 10¹⁵ exactly representable). Current epoch timestamps in milliseconds are ~1.75 × 10¹² — well within range. No action needed, but worth being aware of if CloudKit ever returns timestamps for dates in the distant past/future.

@frozen enum _typePayload is gaining 10 new cases

Types.swift is generated, and consumers are expected to recompile against new versions of MistKitOpenAPI. But any downstream switch on _typePayload without a default case will now fail to compile. Since accessModifier: public is set in the generator config and _typePayload is @frozen public, this is a source-breaking change if any consumer is exhaustively switching on it. Worth a mention in the changelog / PR notes, even if breaking is acceptable here. (In practice, most code will hit the _type parameter via initializer rather than switch it directly, so impact is likely minimal.)

---

CI / Infrastructure

CelestraCloud MISTKIT_BRANCH pin needs revert before merge

Examples/CelestraCloud/.github/workflows/CelestraCloud.yml and update-feeds.yml both point MISTKIT_BRANCH at 375-fieldvaluerequest-timestamp. The PR body already flags this, but since it's in the subrepo it's easy to forget. Consider adding a # TODO: revert to v1.0.0-beta.2 before merge comment in the workflow files so it's obvious at review time.

setup-mistkit SHA-pinning: solid approach, one edge case

The fallback to branch: pinning when git ls-remote fails is the right safety net. Worth noting: if git ls-remote is slow (rate-limited, network hiccup) it may silently time out and return empty, silently falling back to branch pinning. Since this is a CI action and not performance-critical, this is acceptable.

---

Code Quality

makeTypedScalar is clean and the helper split is good

Separating requireNumeric / requireString / numericValue / stringValue from the dispatch logic makes each piece testable in isolation and easy to follow. No issues.

makeInferredScalar uses if case chains rather than a switch

Minor style point: a switch value { case .StringValue: ... case .Int64Value: ... } with a default: return nil would be slightly more idiomatic Swift here, but the if case chains are not wrong and are consistent with the rest of the file. Not a blocker.

BushelCloudKitService.swift fix is correct

Moving batchSucceeded/batchFailed into the loop (instead of the old results.filter { $0.record != nil }.count which wouldn't compile) is correct and more efficient (single pass). The explicit [RecordResult] annotation with the Linux type-inference note is a good, justified comment.

---

Tests

FieldValueConversionTests+ResponseTypes.swift coverage is thorough

The round-trip matrix (whole-millisecond TIMESTAMP, BYTES, whole-valued DOUBLE, explicit STRING/INT64 passthrough, contradiction throws, INT64 non-truncation, complex-type leniency, inferred fallback) covers the key cases. The use of ConversionFailureReporter.$assertionHandler.withValue to suppress the debug trap so the throw is observable is exactly right.

Test fixture JSON is literal and readable — using raw string literals (#"..."#) for the JSON snippets is clean.

---

Summary

The core fix is correct and well-tested. Main pre-merge items:

1. Revert MISTKIT_BRANCH to the stable tag in both CelestraCloud workflows (already called out in the PR).
2. Consider adding a short // INT64 defers to inference to avoid truncating fractional values note in makeTypedScalar above the case .INT64: branch.
3. Note the _typePayload source-breaking change in the changelog.

479 tests passing, format and lint clean — looks ready to land once the branch pin is reverted.

[leogdion]

why are we using strings instead of enumeration or rawValue of enum

[leogdion]

why does it need to be rounded?

[leogdion]

why does it need to be rounded?

[leogdion]

Is LIST an actual value or should it be a "*_LIST"

[claude]

Code Review — PR #375: Tag and validate ambiguous FieldValue scalar types

Overall this is a well-structured fix for a real CloudKit API bug. The problem statement is clear, the two-sided fix (write path + read path) is sound, and the test suite covers the key round-trip scenarios. A few items worth addressing before merge:

---

Correctness / potential bugs

1. INT64 over a non-integer numeric value silently yields .double

makeTypedScalar validates that an INT64-declared value is numeric, then returns nil to defer to inference. This means {"value": 3.5, "type": "INT64"} produces .double(3.5) instead of throwing. The test int64DefersToInference documents this intentionally ("avoids truncating a fractional number"), but the behavior is counter-intuitive: a response that declares INT64 returns a .double. Consider whether it would be cleaner to either:

• throw typeValueMismatch when the numeric value has a fractional part, or
• document in the error type or a comment why this specific lenient path is preferable.

The current approach is defensible (CloudKit is the source of truth, don't lose data), but a future reader will find this surprising without an explicit callout.

2. Truncated docstring in generated Types.swift

///   - _type: Optional CloudKit field type. Sent for scalar values whose JSON form is
public init(

The sentence is cut off. Since Types.swift is a committed generated file with manual edits in this PR, the truncation will persist. Easy fix: complete the sentence.

3. location.timestamp.rounded() change is untested

makeLocationRequest now rounds the location timestamp to whole milliseconds, applying the same fix as the date field. There's no test asserting this rounding. Given that the same fractional-millisecond rejection from CloudKit presumably applies here, a test parallel to convertFractionalDate() is worth adding.

---

Design choices worth a comment

4. @frozen enum expansion

_typePayload is @frozen public enum, and this PR adds 10 new cases (STRING, INT64, DOUBLE, BYTES, TIMESTAMP, REFERENCE, ASSET, ASSETID, LOCATION, LIST). Pre-1.0 this is fine, but any downstream consumer doing exhaustive switch on _typePayload will get a compile error after updating. Worth noting in a CHANGELOG entry or the PR description since this is a source-breaking change to MistKitOpenAPI.

---

Correctness (positive notes)

• BYTES type recovery — stringValue(from:) correctly handles both .StringValue and .BytesValue cases since both arrive as JSON strings. The test confirms .bytes("aGVsbG8=") round-trips correctly.
• TIMESTAMP type recovery — requireNumeric correctly extracts from all three numeric oneOf cases (Int64Value, DoubleValue, DateValue). The whole-millisecond test case is exactly the broken scenario described in FieldValueRequest can't tag scalar TIMESTAMP — Date/Time writes fail with BAD_REQUEST #375.
• Fail-loud philosophy — The typeValueMismatch error is well-placed. The requireNumeric/requireString helpers cleanly separate the validation from the conversion.
• makeInferredScalar vs makeTypedScalar — The two-path design is readable and the lossy-inference documentation in makeInferredScalar is accurate and honest.
• BushelCloud [RecordResult] annotation — The explicit type on modifyRecords is a legitimate Linux compiler workaround, and moving batchSucceeded/batchFailed to in-loop counters is more correct (old code used .record != nil instead of pattern-matching the actual enum case).

---

CI / pre-merge reminder

The PR description notes that MISTKIT_BRANCH: 375-fieldvaluerequest-timestamp in CelestraCloud's two workflow files needs to be reverted to a tag before (or immediately after) merge. That revert should probably be a follow-up commit on this branch or a squash-time edit to avoid landing a CI-branch-pin on v1.0.0-beta.2.

---

Summary: The core fix is correct and well-tested. Three actionable items: (1) complete the truncated docstring in Types.swift, (2) add a test for location.timestamp.rounded(), and (3) decide on / document the INT64-over-fractional-double behavior. The CelestraCloud branch pin must be reverted before merge.