Validate complex/list response type tags against value shape (#376)

[leogdion]

Epic #410. Completes the FieldValue TIMESTAMP-correctness epic.

What

Extends the fail-loud type/value contradiction check (added for #375) from scalar tags to the complex/list response type tags. A declared REFERENCE/ASSET/ASSETID/LOCATION/LIST whose decoded value isn't the matching oneOf case now throws ConversionError.typeValueMismatch instead of silently coercing.

• makeTypedComplex gates on an ExpectedComplexValue mapping, then reuses makeComplexFieldValue so a tagged value converts identically to the same value untagged.
• Untagged responses are unchanged (asset-vs-location oneOf regression stays green).
• Flipped complexTypeContradictionStaysLenient → complexTypeContradictionThrows, added complexTypeMatchingValueConverts, updated the CLAUDE.md policy note.

Note on #375

The scalar TIMESTAMP write tagging and scalar contradiction validation #375 describes are already present on v1.0.0-beta.3 (landed via beta.2 #395). This PR is the remaining read-path validation. Recommend closing #375 as already-resolved.

Closes #376.

Verification

swift build, full swift test (539 pass), swift-format + swiftlint --strict all clean.

[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: 69198519-3355-4481-a9fd-2a430645a5d8

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 fix/fieldvalue-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.}

[claude]

Code Review

This PR extends the fail-loud type/value contradiction check from scalar FieldValueResponse tags to complex/list tags (REFERENCE, ASSET, ASSETID, LOCATION, LIST). The implementation is clean — ExpectedComplexValue is a tight enum that keeps the matching logic co-located, the dispatch order change in init(valuePayload:typePayload:fieldName:) is correct, and reusing makeComplexFieldValue for the matched path means tagged and untagged responses convert identically. The CLAUDE.md update accurately documents the new policy. Three things worth addressing before merge:

---

PLAUSIBLE — makeTypedComplex can silently fall through after a successful match

Sources/MistKit/Models/FieldValues/FieldValue+Components.swift (new makeTypedComplex)

After both guards pass — a complex type tag is present and expected.matches(value) is true — the function calls makeComplexFieldValue(from:fieldName:) and returns its FieldValue? result directly. makeComplexFieldValue returns non-nil for the same four cases that matches() accepts, so this is safe today. But the invariant between the two methods is maintained only by convention: two independent if case chains that happen to cover the same four valuePayload cases.

If a fifth complex type is later added to ExpectedComplexValue and matches() but makeComplexFieldValue is not updated, makeTypedComplex returns nil instead of throwing, and the caller falls through to makeSimpleFieldValue → unmappableFieldValue rather than typeValueMismatch. This is the opposite of the project's fail-loud philosophy.

Suggested fix: guard-unwrap the result after a successful match and preconditionFailure (or throw) on nil:

guard let result = try makeComplexFieldValue(from: value, fieldName: fieldName) else {
  preconditionFailure("matches() returned true but makeComplexFieldValue returned nil for \(value)")
}
return result

---

CONFIRMED — complexTypeContradictionThrows omits ASSETID

Tests/MistKitTests/Models/FieldValues/FieldValueConversionTests+ResponseTypes.swift (new complexTypeContradictionThrows)

ASSETID is mapped in ExpectedComplexValue to .asset, so {"value": 42, "type": "ASSETID"} should throw typeValueMismatch under the new code. But it is not covered by complexTypeContradictionThrows, which tests REFERENCE, ASSET, LOCATION, and LIST. A regression that drops ASSETID from the complex map — returning it to the old lenient path — would not be caught.

Suggested addition:

expectThrows(#"{"value": 42, "type": "ASSETID"}"#)

---

PLAUSIBLE — complexTypeMatchingValueConverts never tests ASSET with an explicit type: "ASSET" tag

Tests/MistKitTests/Models/FieldValues/FieldValueConversionTests+ResponseTypes.swift (new complexTypeMatchingValueConverts)

The success test for the asset path uses type: "ASSETID". type: "ASSET" + matching AssetValue is never exercised in the tagged path. ConversionFailureTests.assetFieldValueConvertsToAsset covers only the untagged path. The code gap is minimal (both hit makeTypedScalar's default: return nil then makeComplexFieldValue), but an explicit tagged-ASSET success case would close the coverage symmetry with the contradiction test.

---

Minor note on pattern consistency

requireNumeric / requireString (scalar helpers) perform both the guard check and the throw in a single step, returning a typed value to the caller. reportComplexMismatch is a throw-only helper split from an external guard check. This is a minor idiom divergence from the scalar helpers — not a bug, but a future reader using the scalar pattern as a template won't naturally land on the two-step approach.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.22%. Comparing base (7791b09) to head (3736d5e).
⚠️ Report is 4 commits behind head on v1.0.0-beta.3.

Additional details and impacted files

@@                Coverage Diff                @@
##           v1.0.0-beta.3     #411      +/-   ##
=================================================
+ Coverage          73.85%   76.22%   +2.37%     
=================================================
  Files                156      158       +2     
  Lines               3698     3761      +63     
=================================================
+ Hits                2731     2867     +136     
+ Misses               967      894      -73     

Flag	Coverage Δ
mistdemo-spm-macos	11.30% <0.00%> (+0.10%)	⬆️
mistdemo-swift-6.2-jammy	11.30% <0.00%> (+0.10%)	⬆️
mistdemo-swift-6.2-noble	11.30% <0.00%> (+0.10%)	⬆️
mistdemo-swift-6.3-jammy	11.30% <0.00%> (+0.10%)	⬆️
mistdemo-swift-6.3-noble	11.30% <0.00%> (+0.10%)	⬆️
spm	74.34% <100.00%> (+2.76%)	⬆️
swift-6.1-jammy	74.09% <100.00%> (+2.49%)	⬆️
swift-6.1-noble	74.22% <100.00%> (+2.57%)	⬆️
swift-6.2-jammy	74.25% <100.00%> (+2.65%)	⬆️
swift-6.2-noble	74.25% <100.00%> (+2.41%)	⬆️
swift-6.3-jammy	74.38% <100.00%> (+2.81%)	⬆️
swift-6.3-noble	74.22% <100.00%> (+2.68%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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

Summary: Extends the fail-loud type/value contradiction check from scalar tags to complex/list response type tags. Well-motivated and cleanly executed.

---

Strengths

• ExpectedComplexValue enum is a clean, self-contained mapping — adding a new complex type in the future just requires adding a case here and in matches().
• makeTypedComplex correctly delegates to makeComplexFieldValue on a match, so tagged complex values convert identically to untagged ones (no duplication).
• ASSETID → AssetValue sharing with ASSET is explicitly documented.
• LIST element types staying lenient is the right call — the response type enum has no granular *_LIST variants, so element-level validation would require parsing the value array items (over-engineering for a tag that only says "this is a list").
• Test suite covers the full matrix: all contradiction combos throw, cross-complex contradiction throws (LOCATION shape + REFERENCE tag), and all matching shapes round-trip correctly.

---

Observations / Minor Notes

Unusual guard … else { try f() } pattern with throws -> Never

guard expected.matches(value) else {
  try reportComplexMismatch(value, fieldName: fieldName, declaredType: fieldType.rawValue)
}

reportComplexMismatch is throws(ConversionError) -> Never, so the compiler sees the else-branch as always-exiting (either throws or can't return). This is valid Swift 6, but it's unusual enough that a future reader unfamiliar with this pattern might wonder why there's no throw or return. A single-line comment like // always throws or simply rewriting as throw reportComplexMismatch(...) (if reportComplexMismatch returned a ConversionError rather than being Never) might be clearer. Not a bug — just a readability note.

"\(value)" in error messages

value: "\(value)" uses Swift string interpolation on a generated oneOf enum (valuePayload). The output could be quite verbose (e.g. AssetValue(fileChecksum: Optional("chk"), …)). Since this only appears in error/debugging paths, it's acceptable — but if ConversionError.typeValueMismatch is ever user-facing, consider a more compact representation.

Doc-comment length on makeTypedComplex

The doc comment is detailed and accurate, but it's substantially longer than comments elsewhere in the file. CLAUDE.md guidance is one short line max for comments where the WHY is non-obvious. The most critical WHY ("fail-loud to match scalar contradiction treatment") is already captured in the method name and the ExpectedComplexValue doc; consider trimming the block.

---

Test nit

complexTypeContradictionThrows calls expectThrows(...) five times without try/#expect(throws:). That helper must be swallowing or ignoring errors. As long as expectThrows records a Testing Issue on failure, this is fine — just worth confirming the test would actually fail if the throws were removed.

---

Overall: Correct, well-tested, and closes the issue cleanly. The edge cases (ASSETID, LIST container-only validation, untagged-path unchanged) are all handled thoughtfully. LGTM pending the minor readability notes above.

[claude]

Review: #411 — Validate complex/list response type tags against value shape

Nicely scoped follow-up to #375 that closes the loop on #376. I traced the logic against the generated Components.Schemas.FieldValueResponse._typePayload/valuePayload oneOf decode order (Sources/MistKitOpenAPI/Types.swift) and the new/updated tests in FieldValueConversionTests+ResponseTypes.swift; I did not find any correctness bugs.

Code quality / design

• ExpectedComplexValue + matches(_:) mirror the existing makeTypedScalar/requireNumeric/requireString pattern in FieldValue+Components+Scalar.swift well — same "typed tag wins, contradiction throws" shape, so the two files read consistently.
• Good reuse: makeTypedComplex validates the tag and then delegates back to makeComplexFieldValue rather than re-deriving the domain value, so a tagged value converts identically to the same value untagged (verified by complexTypeMatchingValueConverts).
• The _typePayload switch coverage is exhaustive by construction — all 10 response type cases split between makeTypedScalar (STRING/INT64/DOUBLE/BYTES/TIMESTAMP) and ExpectedComplexValue.init? (REFERENCE/ASSET/ASSETID/LOCATION/LIST) — no gap where a real CloudKit type tag would silently fall through both.
• Minor nit: reportComplexMismatch duplicates the same 4-line ConversionError.typeValueMismatch(...); try failure.reportAndThrow() shape already inlined in requireNumeric/requireString (scalar file). Not a real problem given how small each site is and the codebase's stated preference against premature abstraction, but if a fourth call site ever appears it would be worth a shared throwTypeValueMismatch(fieldName:declaredType:value:) helper.

Potential bugs

• None found. I specifically checked the trickier cases:
  • ASSETID correctly reuses AssetValue/.asset (no distinct domain case) — matches the CLAUDE.md note and the complexTypeMatchingValueConverts test.
  • LIST is validated only at the container level, consistent with the response schema only having a single LIST tag (unlike the request's granular *_LIST family in FieldValueRequest._typePayload) — the doc comment's claim checks out against the generated types.
  • The "wrong complex shape" case (a LOCATION-shaped value under a declared REFERENCE tag) correctly throws because LocationValue decodes first in the oneOf and does not match .reference.
  • Untagged complex/list responses are unaffected — makeTypedComplex returns nil for a nil/scalar type and falls through to the existing makeComplexFieldValue value-shape path, so well-formed untagged responses cannot regress.

Performance

• Non-issue — this adds a constant-time enum match per field conversion, no allocations beyond what already existed.

Security

• N/A — this only tightens validation on already-decoded response data (fail loud instead of silently coercing), which if anything reduces the risk of misinterpreted CloudKit data being treated as the wrong domain type.

Test coverage

• Solid: the flip from complexTypeContradictionStaysLenient to complexTypeContradictionThrows covers all four complex/list tags against an incompatible value, plus the complex-vs-complex mismatch case; the new complexTypeMatchingValueConverts test exercises REFERENCE/LOCATION/ASSETID/LIST on the happy path. One gap: there is no test for a plain ASSET-tagged value matching (only ASSETID is exercised on the happy path) — low risk since ExpectedComplexValue.init? maps both to .asset via the same case, but a one-line addition would make the ASSET/ASSETID equivalence airtight in both directions.

CLAUDE.md

• The doc update accurately reflects the new behavior and supersedes the now-stale "complex/list types... are not validated against the tag" sentence it replaces. Good practice per this repo's convention of keeping the FieldValue architecture writeup in sync with the code.

Overall: solid, well-tested, and consistent with the established scalar-validation pattern. No blocking issues from me.

[claude]

Review

Reviewed the diff (gh pr diff 411) against the actual source (Sources/MistKit/Models/FieldValues/FieldValue+Components.swift, FieldValue+Components+Scalar.swift, generated Types.swift) and tests. Overall this is a clean, well-scoped extension of the #375 fail-loud pattern to complex/list response types — good symmetry with makeTypedScalar, sensible reuse of makeComplexFieldValue so a matching tag converts identically to an untagged value, and solid new coverage (complexTypeContradictionThrows / complexTypeMatchingValueConverts) including the ASSETID→AssetValue aliasing case and the LOCATION-shape-under-REFERENCE-tag cross-contradiction case. The CLAUDE.md doc update accurately reflects the new behavior (verified against the generated _typePayload/valuePayload enums — LIST really is a single response-side tag, container-only validation is correctly described).

Bug: schema.ckdb not updated for the new timestamp field (Examples/MistDemo)

CreateRecordsPhase.swift now writes a "timestamp": .date(...) field on every created Note record, and LookupRecordsPhase.swift asserts it round-trips. But Examples/MistDemo/schema.ckdb still only declares title, index, and image for the Note record type — timestamp was never added:

RECORD TYPE Note (
    "___recordID"            REFERENCE QUERYABLE,
    "___createTime"          TIMESTAMP QUERYABLE SORTABLE,
    "___modTime"             TIMESTAMP QUERYABLE SORTABLE,
    "title"                  STRING QUERYABLE SORTABLE SEARCHABLE,
    "index"                  INT64 QUERYABLE SORTABLE,
    "image"                  ASSET,
    ...
)

Per Apple's CloudKit docs, undeclared fields are only auto-created via "just-in-time schema" in the development environment; production has no such fallback and rejects writes for fields that aren't in the deployed schema. MistDemoConfig defaults to .development, so this won't surface in default CI/local runs today, but:

• Running test-public/test-private against a production environment will fail on CreateRecordsPhase with a schema error.
• schema.ckdb is the checked-in, cktool-validated source of truth for schema deploys (per Examples/SCHEMA_QUICK_REFERENCE.md); it's now out of sync with what the demo actually writes, so a future cktool import-schema/dashboard deploy from this file would silently omit the field.

Suggest adding "timestamp" TIMESTAMP, to the Note record type in schema.ckdb alongside this change.

Minor / non-blocking

• ExpectedComplexValue's init?/matches are declared fileprivate while the enclosing enum itself is private (extension-scoped) — the fileprivate doesn't buy anything beyond what private already gives here since all call sites (makeTypedComplex) are in the same file/extension. Not wrong, just slightly wider than needed; either is fine but worth a glance since the sibling scalar file doesn't use fileprivate at all.
• complexTypeMatchingValueConverts covers ASSETID→AssetValue but doesn't add a direct positive case for the plain ASSET tag (only the mismatch test exercises ASSET). Given ASSETID and ASSET share the exact same mapping branch, this is very low risk, but a one-line addition would make the matrix complete.
• ConversionError.typeValueMismatch's doc comment ("e.g. a TIMESTAMP tag over a string value") was written for the scalar-only case in FieldValueRequest can't tag scalar TIMESTAMP — Date/Time writes fail with BAD_REQUEST #375 and reads a bit stale now that the same case backs complex/list mismatches too — could mention both in a follow-up, not blocking.

Not an issue (checked, confirmed correct)

• Dispatch ordering in init(valuePayload:typePayload:fieldName:): makeTypedComplex is tried first but returns nil immediately for nil/scalar tags (via ExpectedComplexValue.init?), so scalar-tagged and untagged responses still fall through to makeSimpleFieldValue/makeComplexFieldValue unchanged — no regression for the FieldValueRequest can't tag scalar TIMESTAMP — Date/Time writes fail with BAD_REQUEST #375 scalar path.
• The oneOf decode order (String → Int64 → Double → Bytes → Date → Location → Reference → Asset → List) means the new cross-contradiction test cases (e.g. a {"latitude":...,"longitude":...} payload under a REFERENCE tag) decode into the case the test expects, so those assertions are testing what they claim to.
• Test coverage traces back to real generated types (_typePayload has ASSETID as a distinct case sharing AssetValue's shape) — the mapping in ExpectedComplexValue.init? matches the schema.

No security concerns (pure data-shape validation, no new I/O/parsing surface). No performance concerns (single extra switch on an already-decoded enum, no new allocations on the hot path).

[claude]

Review: #411 — Validate complex/list response type tags against value shape

Reviewed the diff (7 files) against CLAUDE.md's existing scalar-validation conventions (issue #375) since this PR extends that same pattern to complex/list types (issue #376).

Code quality

• makeTypedComplex/ExpectedComplexValue in FieldValue+Components.swift mirrors the existing makeTypedScalar pattern in FieldValue+Components+Scalar.swift closely (same ordering: explicit-type check → fall through to inference/shape-based conversion), keeping the two conversion paths easy to read side by side.
• Reusing ConversionError.typeValueMismatch (rather than adding a new case) and the existing reportComplexMismatch/reportAndThrow plumbing is the right call — no new public surface for a change that's conceptually "the same rule, wider scope."
• Doc comments on makeTypedComplex and the ExpectedComplexValue enum clearly spell out the ASSETID→AssetValue aliasing and why LIST is only validated at the container level (no per-item type on the wire) — this matches what's in FieldValue+Components+List.swift and the generated _typePayload enum in Types.swift.
• The CLAUDE.md policy note is updated in the same PR, so docs and code won't drift.

Potential bugs / issues

• Schema drift: CreateRecordsPhase.swift now writes a "timestamp" field on the Note record type used by the MistDemo integration tests, but Examples/MistDemo/schema.ckdb still only declares ___recordID, ___createTime, ___modTime, title, index, and image for Note — no timestamp field. Since the default environment is development (MistDemoConfig/AuthTokenConfig default to .development), CloudKit's auto-schema-extension on first write will likely mask this locally, but the checked-in schema no longer reflects what test-public/test-private actually writes, and running against a production container (where the schema must already exist) would fail at createRecord with a schema violation. Worth adding a "timestamp" TIMESTAMP field to the Note record type in schema.ckdb to keep it in sync.
• No functional bugs found in the core makeTypedComplex logic — tracing the ordering in FieldValue+Components.swift's init(valuePayload:typePayload:fieldName:) (complex-typed check first, then scalar, then shape-based fallback) confirms there's no overlap between ExpectedComplexValue's cases and the scalar switch in makeTypedScalar, so a scalar type still falls through correctly to makeSimpleFieldValue.

Performance

No concerns. The added check is O(1) per field (a single enum init + switch), and only runs on the response-decoding path, not the hot request path.

Security

No concerns. This only tightens validation (fail loud on an internally-inconsistent response) rather than relaxing it, and doesn't touch auth, tokens, or transport code.

Test coverage

• Good: complexTypeContradictionThrows (renamed/flipped from the old "stays lenient" test) and the new complexTypeMatchingValueConverts cover REFERENCE/ASSET/ASSETID/LOCATION/LIST both matching and contradicting, including the "wrong complex shape" case (LOCATION-shaped value under a REFERENCE tag). Thorough at the unit-test layer.
• The MistDemo integration addition (verifyTimestampRoundTrip in LookupRecordsPhase.swift) is a nice end-to-end sanity check for the scalar TIMESTAMP recovery (FieldValueRequest can't tag scalar TIMESTAMP — Date/Time writes fail with BAD_REQUEST #375) via a live CloudKit round trip, though it doesn't actually exercise the complex/list validation this PR adds — that's fully covered by the unit tests instead, which is fine.

Minor / out of scope

• The .github/workflows/MistDemo.yml change (-Xswiftc -disable-batch-mode for the WASM build flags) is unrelated to the stated purpose of this PR. Low-risk, but if it's fixing an unrelated flaky-build issue it might be worth its own commit/PR with a note on why it's needed.

Overall: a clean, well-scoped, well-tested extension of an already-established pattern. The main actionable item is updating schema.ckdb for the new timestamp field to avoid schema/code drift.

[claude]

Review

Solid, well-scoped change — this is a clean extension of the #375 fail-loud scalar validation to the complex/list response type tags, and it follows the existing pattern (makeTypedScalar → makeTypedComplex) closely enough that it reads naturally alongside the code it complements.

Code quality / correctness

• ExpectedComplexValue.init?(_:) correctly partitions the full _typePayload enum: 5 scalar cases fall through to nil (deferring to makeTypedScalar) and all 5 complex/list cases (REFERENCE, ASSET, ASSETID, LOCATION, LIST) are mapped — verified against the generated _typePayload in Types.swift, no case is silently dropped.
• makeTypedComplex is tried before makeSimpleFieldValue in the init(valuePayload:typePayload:...) chain (FieldValue+Components.swift:88-99). Since it returns nil immediately for scalar/absent type, this reordering doesn't change scalar behavior — confirmed by reading through makeTypedScalar's own guard.
• Reuses makeComplexFieldValue for the actual conversion once the shape is validated, so a tagged value converts identically to an untagged one — avoids duplicating the Location/Reference/Asset/List construction logic.
• ASSETID → .asset mapping matches the existing Asset(from:) domain model (no distinct AssetID domain case), and is covered by a dedicated test.
• Scoping LIST validation to the container level only (not per-element) is a reasonable, explicitly-documented tradeoff given the response type enum has a single LIST case rather than the request's granular *_LIST family.

Test coverage

Good symmetry: complexTypeContradictionThrows covers scalar-under-complex-tag and wrong-complex-under-complex-tag cases, and complexTypeMatchingValueConverts verifies well-formed responses (including ASSETID) still convert faithfully — so the "throws only on genuine contradiction" invariant is exercised from both sides. The MistDemo end-to-end addition (seed a .date field in CreateRecordsPhase, assert round-trip in LookupRecordsPhase) is a nice belt-and-suspenders integration check beyond the unit tests, and the new TimestampWireRoundTrip suite closes the loop on the request-side encoding with a wasm32 overflow guard.

One gap worth flagging

Examples/MistDemo/schema.ckdb (Note record type) doesn't declare a timestamp field, but CreateRecordsPhase now writes one and LookupRecordsPhase asserts on it. CloudKit's development environment auto-infers new fields on save, so test-public/test-private will likely pass there, but if this schema is ever promoted to production without a corresponding cktool schema update, the create will fail with a schema-mismatch error. Worth adding a "timestamp" TIMESTAMP QUERYABLE SORTABLE, line to schema.ckdb (mirroring ___createTime/___modTime) so the checked-in schema stays truthful, per the project's schema-as-source-of-truth convention.

Minor / non-blocking

• The .github/workflows/MistDemo.yml -disable-batch-mode addition is unrelated to the stated PR scope (per the commit history it's an OOM workaround for the wasm matrix build, presumably needed because this PR's new test files pushed compile memory over the edge). Worth a one-line mention in the PR description so reviewers aren't left guessing why a CI flag change rode along with a FieldValue fix.
• CLAUDE.md doc update is accurate and consistent with the code (ASSETID shares AssetValue, LIST validated only at container level) — good that the doc and implementation were updated together.

Security / performance

No concerns — this is pure validation logic on already-decoded local data, no new I/O, no new attack surface. The added checks make error reporting more precise (throwing with field name + declared type + value) rather than silently mis-typing data, which is a net safety improvement for consumers.

Nice work — approve modulo the schema.ckdb gap, which I'd consider a should-fix rather than a blocker since dev-environment tests will pass either way.