V0.13 support - #110
Conversation
- Compare resolved accepted-instance sets and expose compatible, incompatible, or unknown verdicts with diagnostics. - Honor dialect-aware content models and boolean-equivalent schemas while preserving nested definitions. - Pin the conformance suite to v0.13.0. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- Store major versions as optional values so an explicit v0 is not treated as unspecified. - Match and serialize v0 minor wildcards without leaking into other major versions. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- inline GTS ID references in retained schema definitions - compare const, boolean, bounds, and inferred types by directional inclusion - add regression coverage for compatibility verdicts and dangling local refs Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
📝 WalkthroughWalkthroughThis PR updates GTS to specification version 0.13, replacing boolean compatibility results with verdicts and diagnostics, resolving schema references during comparison, improving Draft-07 schema generation and closure, and refining wildcard identifier version matching. ChangesGTS 0.13 compatibility and schema generation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GtsOps
participant GtsStore
participant SchemaCast
Client->>GtsOps: request compatibility
GtsOps->>GtsStore: compare schema identifiers
GtsStore->>GtsStore: resolve schema references
GtsStore->>SchemaCast: compare resolved schemas
SchemaCast-->>GtsStore: return verdicts and diagnostics
GtsStore-->>GtsOps: return compatibility result
GtsOps-->>Client: serialize verdict-based response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
gts-macros/src/lib.rs (1)
1797-1808: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider hoisting the definitions-emptiness block into a shared
quote!fragment.This block is byte-identical in the generic and non-generic branches, like the three pipeline fragments above it. Folding it into the existing shared fragments (or a fourth one) keeps the two branches from drifting.
Also applies to: 1935-1946
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts-macros/src/lib.rs` around lines 1797 - 1808, Extract the duplicated definitions-emptiness logic into a shared quote! fragment alongside the existing pipeline fragments, then reuse it in both the generic and non-generic branches. Preserve the current removal of GtsInstanceId, GtsTypeId, and GtsSchemaId and the behavior of setting definitions to None when the resulting object is empty.gts/src/store.rs (1)
829-842: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
$refresolution failures are reported asStoreError::SchemaNotFound. The shared root cause is thatStoreErrorhas no variant for "a reference in this document could not be resolved", so both new resolution sites reuseSchemaNotFound, whoseDisplayreadsGTS type schema with ID '{0}' not found in store— the payload here is a sentence, not an id. Callers matching on the variant cannot distinguish a genuinely unregistered type from an unresolvable reference (a circular$refis reported as "not found").
gts/src/store.rs#L829-L842: map theresolve_schema_refserrors incastto a resolution-specific variant (or propagate the underlyingStoreErrorwith context) instead ofSchemaNotFound.gts/src/store.rs#L986-L991: do the same for bothcompare_documentsarms; notestore_test.rscurrently assertsStoreError::SchemaNotFound(_)there, so that assertion needs updating alongside.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts/src/store.rs` around lines 829 - 842, The cast and compare_documents paths incorrectly classify resolve_schema_refs failures as SchemaNotFound. Add or reuse a resolution-specific StoreError variant, update both mapping arms in gts/src/store.rs lines 829-842 and 986-991 to use it or propagate the underlying error with context, and update the corresponding store_test.rs assertions to match the new variant.gts/src/store_test.rs (1)
6062-6163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood cross-operation invariant test. OP#8/OP#9 agreement is exactly the regression the
castresolution change is meant to prevent.One gap: the test only asserts the two verdict pairs match. Consider also asserting the concrete expected values (
backwardcompatible,forwardincompatible) on thecastside, so a future change that makes both paths wrong in the same way still fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts/src/store_test.rs` around lines 6062 - 6163, Extend test_cast_and_compatibility_agree_on_referenced_schemas to assert cast.backward_compatibility is compatible and cast.forward_compatibility is incompatible, in addition to comparing the cast verdict pair with compatibility. Keep the existing cross-operation equality assertion unchanged.gts/src/schema_compat.rs (2)
36-51: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueIntersecting two nontrivial
additionalPropertiesschemas still last-wins.
else { *current = Some(candidate.clone()) }overwrites an existing nontrivial constraint with the new one, soallOf: [{ap: {"type":"string"}}, {ap: {"maxLength": 5}}]losestype: string. The doc comment only promises closedness preservation, so this is consistent with the stated lattice — but the resulting effective schema is weaker than the real intersection and can under-report incompatibilities. Worth a follow-up if schema-valuedadditionalPropertiesbecomes common in composed types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts/src/schema_compat.rs` around lines 36 - 51, Update merge_additional_properties_constraint to preserve the intersection of two non-boolean, schema-valued additionalProperties constraints instead of overwriting current with candidate in the final else branch. Reuse the existing schema-combination mechanism in the surrounding module, while retaining the current false-constraint short-circuit and true-constraint no-op behavior.
193-203: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNontrivial derived
additionalPropertiesis treated as loosening.
boolean_schema_value(value) != Some(false)makesadditionalProperties: {"type": "string"}on a derived schema count as "explicitly allows", flagging a case thatallOf/$refcomposition still keeps closed via the base. It errs conservative (fails closed), which matches the surrounding intent, but the message ("loosens additionalProperties") will read as a false positive to authors of partially-open overlays. Consider a distinct message for the non-boolean case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts/src/schema_compat.rs` around lines 193 - 203, The derived additionalProperties check in the compatibility validation block should distinguish non-boolean schemas from explicit boolean allowance. Update the branch using boolean_schema_value so object-valued additionalProperties produces a separate diagnostic message, while preserving the existing “loosens additionalProperties” message for an explicit true value and the current closed-base validation behavior.gts/src/schema_semantics.rs (1)
3-18: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider whether
$refsiblings should defeat boolean-equivalence.
$refis deliberately absent fromNON_ASSERTION_KEYWORDS, so{"$ref": ...}correctly yieldsNone. Just confirming that's intentional given callers pass resolved schemas — an unresolved$refalongside no other keyword should never be read astrue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts/src/schema_semantics.rs` around lines 3 - 18, Confirm and preserve the intentional exclusion of "$ref" from NON_ASSERTION_KEYWORDS so a schema containing only an unresolved reference continues returning None rather than true. Review the boolean-equivalence logic and its callers’ resolved-schema contract to ensure $ref siblings do not alter this behavior; make no changes unless the implementation violates that contract.gts/src/lib.rs (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
GTS_SPECIFICATION_VERSIONfrom the spec pin.
gts/src/lib.rshardcodes"0.13", while.gts-spec-versionisv0.13.0. These formats can fall out of sync on future bumps; include the trimmed file value or add a unit test asserting the constant matches the pinned spec revision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts/src/lib.rs` around lines 20 - 24, Update GTS_SPECIFICATION_VERSION to derive from the pinned .gts-spec-version value by including and trimming the file contents, or add a unit test that asserts the constant matches that pin. Remove the hardcoded "0.13" value while preserving the public constant’s existing major/minor format.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gts-macros/src/lib.rs`:
- Around line 1619-1640: Update close_schema and its definitions traversal to
avoid inserting additionalProperties: false into definitions referenced by
allOf, anyOf, or oneOf branches. Track reachability from combinator schema
references, or restrict closing to definitions reached through non-combinator
properties, while preserving closure for ordinary non-combinator schemas.
In `@gts/src/store.rs`:
- Around line 854-857: Update is_compatible to resolve both old_type_id and
new_type_id through get_schema_entity instead of get, ensuring only schema
entities are compared. Preserve the existing GtsEntityCastResult behavior and
retain the exact "Schema not found" message for missing schemas.
- Around line 882-901: Introduce GtsEntityCastResult::undecided(old_type_id,
new_type_id, message) to centralize unknown/error result initialization,
including verdicts, version strings, and empty collections. In
gts/src/store.rs:860-879, replace the schema-not-found literal while preserving
"Schema not found" and the "unknown" direction; in gts/src/store.rs:882-901,
replace resolution_failure’s literal with the constructor; and in
gts/src/ops.rs:676-695, replace the GtsOps::cast error literal, passing
e.to_string().
---
Nitpick comments:
In `@gts-macros/src/lib.rs`:
- Around line 1797-1808: Extract the duplicated definitions-emptiness logic into
a shared quote! fragment alongside the existing pipeline fragments, then reuse
it in both the generic and non-generic branches. Preserve the current removal of
GtsInstanceId, GtsTypeId, and GtsSchemaId and the behavior of setting
definitions to None when the resulting object is empty.
In `@gts/src/lib.rs`:
- Around line 20-24: Update GTS_SPECIFICATION_VERSION to derive from the pinned
.gts-spec-version value by including and trimming the file contents, or add a
unit test that asserts the constant matches that pin. Remove the hardcoded
"0.13" value while preserving the public constant’s existing major/minor format.
In `@gts/src/schema_compat.rs`:
- Around line 36-51: Update merge_additional_properties_constraint to preserve
the intersection of two non-boolean, schema-valued additionalProperties
constraints instead of overwriting current with candidate in the final else
branch. Reuse the existing schema-combination mechanism in the surrounding
module, while retaining the current false-constraint short-circuit and
true-constraint no-op behavior.
- Around line 193-203: The derived additionalProperties check in the
compatibility validation block should distinguish non-boolean schemas from
explicit boolean allowance. Update the branch using boolean_schema_value so
object-valued additionalProperties produces a separate diagnostic message, while
preserving the existing “loosens additionalProperties” message for an explicit
true value and the current closed-base validation behavior.
In `@gts/src/schema_semantics.rs`:
- Around line 3-18: Confirm and preserve the intentional exclusion of "$ref"
from NON_ASSERTION_KEYWORDS so a schema containing only an unresolved reference
continues returning None rather than true. Review the boolean-equivalence logic
and its callers’ resolved-schema contract to ensure $ref siblings do not alter
this behavior; make no changes unless the implementation violates that contract.
In `@gts/src/store_test.rs`:
- Around line 6062-6163: Extend
test_cast_and_compatibility_agree_on_referenced_schemas to assert
cast.backward_compatibility is compatible and cast.forward_compatibility is
incompatible, in addition to comparing the cast verdict pair with compatibility.
Keep the existing cross-operation equality assertion unchanged.
In `@gts/src/store.rs`:
- Around line 829-842: The cast and compare_documents paths incorrectly classify
resolve_schema_refs failures as SchemaNotFound. Add or reuse a
resolution-specific StoreError variant, update both mapping arms in
gts/src/store.rs lines 829-842 and 986-991 to use it or propagate the underlying
error with context, and update the corresponding store_test.rs assertions to
match the new variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 241afd0d-1127-4f88-b18d-dbec7da18205
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockgts-dylint/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.gts-spec-versionCargo.tomlREADME.mdgts-id/src/gts_id_pattern.rsgts-id/src/gts_id_segment.rsgts-macros/README.mdgts-macros/src/lib.rsgts-macros/tests/inheritance_tests.rsgts-macros/tests/integration_tests.rsgts/Cargo.tomlgts/src/lib.rsgts/src/ops.rsgts/src/schema_cast.rsgts/src/schema_compat.rsgts/src/schema_semantics.rsgts/src/schema_traits.rsgts/src/store.rsgts/src/store_test.rs
- compare const, enum, and numeric constraints by accepted values - handle exact mixed numeric equality and signed-zero bounds - apply the validator's default dialect to unevaluated properties
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gts-macros/tests/inheritance_tests.rs (1)
591-605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso pin the outer alias hop so the test keeps exercising the fixed-point walk.
The comment on the fixtures states the branch reaches
NestedContactthrough two hops, and the fixed-point loop ingts-macros/src/lib.rsexists for exactly that. The assertion only pinsContactAlias; if Schemars stopped aliasingContactAliasAlias(e.g. by inlining it), the test would still pass while silently degrading to a one-hop case.♻️ Assert both hops of the chain
let schema = SchemaWithAliasedDefinitionV1::gts_schema_with_refs(); + assert_eq!( + schema.pointer("/definitions/ContactAliasAlias/$ref"), + Some(&serde_json::json!("`#/definitions/ContactAlias`")), + "test relies on a two-hop alias chain:\n{}", + serde_json::to_string_pretty(&schema).unwrap() + ); assert_eq!( schema.pointer("/definitions/ContactAlias/$ref"), Some(&serde_json::json!("`#/definitions/NestedContact`")), "test relies on Schemars emitting a bare $ref alias:\n{}", serde_json::to_string_pretty(&schema).unwrap() );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gts-macros/tests/inheritance_tests.rs` around lines 591 - 605, Extend test_definition_aliased_by_combinator_branch_stays_open to assert that the outer alias definition, ContactAliasAlias, points to ContactAlias before checking ContactAlias’s reference and NestedContact openness. This must pin both alias hops so the test continues exercising the fixed-point traversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gts-macros/tests/inheritance_tests.rs`:
- Around line 591-605: Extend
test_definition_aliased_by_combinator_branch_stays_open to assert that the outer
alias definition, ContactAliasAlias, points to ContactAlias before checking
ContactAlias’s reference and NestedContact openness. This must pin both alias
hops so the test continues exercising the fixed-point traversal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e75d363-8fbf-4d4b-8d2d-1e68cd3a44fb
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockgts-dylint/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlgts-macros/src/lib.rsgts-macros/tests/inheritance_tests.rsgts/Cargo.tomlgts/src/ops.rsgts/src/schema_cast.rsgts/src/store.rsgts/src/store_test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- gts/Cargo.toml
- Cargo.toml
2 participants
Summary by CodeRabbit
v0.*).additionalPropertieshandling.