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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates GTS to specification version 0.13. It replaces boolean compatibility results with verdicts and diagnostics, resolves schema references during comparison, improves Draft-07 schema generation and closure, and refines wildcard identifier 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 SchemaEvolution
Client->>GtsOps: request compatibility
GtsOps->>GtsStore: compare schema identifiers
GtsStore->>GtsStore: resolve schema references
GtsStore->>SchemaEvolution: compare resolved schemas
SchemaEvolution-->>GtsStore: return verdicts and diagnostics
GtsStore-->>GtsOps: return compatibility result
GtsOps-->>Client: serialize verdict response
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
- Materialize absent traits from defaults only, leaving const as a validation assertion. - Treat required const-only traits as unresolved and cover const/default behavior. - Pin the conformance suite to v0.13.1. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- Respect Schemars content models on base and derived GTS structs. - Keep automatically added derives before schemars helper attributes. - Document the behavior and add golden coverage for open, closed, map, and combinator schemas. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gts-macros/src/lib.rs (1)
1961-1984: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winProcess root
additionalPropertiesbefore pruning definitions.Schemars can emit a value-type reference under root
additionalPropertiesfor#[serde(flatten)] HashMap<String, Struct>, but the current inlining and reachability passes only scanpropertiesanddefinitions. Preserved definition targets are dropped, leaving the emitted schema with an unresolved reference in both the generic and non-generic branches.
- Make
declared_additional_propertiesmutable.- Pass
declared_additional_propertiesthroughinline_gts_id_definitionsandcollect_definition_refsbeforedefinitions_object.retain(...).- Add a covered golden case for a flattened map whose value type is a named
JsonSchemastruct and assert that theadditionalPropertiesreference resolves.🤖 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 1961 - 1984, Update gts-macros/src/lib.rs lines 1961-1984 and 2120-2128 so declared_additional_properties is mutable and is processed by inline_gts_id_definitions and collect_definition_refs before definitions_object.retain, preserving references emitted under root additionalProperties in both branches. Add a golden test covering a flattened map with a named JsonSchema value type and assert its additionalProperties reference resolves.
🤖 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.
Outside diff comments:
In `@gts-macros/src/lib.rs`:
- Around line 1961-1984: Update gts-macros/src/lib.rs lines 1961-1984 and
2120-2128 so declared_additional_properties is mutable and is processed by
inline_gts_id_definitions and collect_definition_refs before
definitions_object.retain, preserving references emitted under root
additionalProperties in both branches. Add a golden test covering a flattened
map with a named JsonSchema value type and assert its additionalProperties
reference resolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 05ce1680-4590-4f31-8efb-279c14daba82
📒 Files selected for processing (17)
gts-macros/README.mdgts-macros/src/lib.rsgts-macros/tests/golden/additional_properties_content_models.rsgts-macros/tests/golden/additional_properties_content_models/gts.x.test.golden.contentmodels.v1~.schema.jsongts-macros/tests/golden/additional_properties_explicit_open.rsgts-macros/tests/golden/additional_properties_explicit_open/gts.x.test.golden.explicitopen.v1~.schema.jsongts-macros/tests/golden/additional_properties_flattened_map.rsgts-macros/tests/golden/additional_properties_flattened_map/gts.x.test.golden.flattenedmap.v1~.schema.jsongts-macros/tests/golden/additional_properties_gts_derived_open.rsgts-macros/tests/golden/additional_properties_gts_derived_open/gts.x.test.golden.openchain.v1~.schema.jsongts-macros/tests/golden/additional_properties_gts_derived_open/gts.x.test.golden.openchain.v1~x.test.audit.payload.v1~.schema.jsongts-macros/tests/golden/additional_properties_gts_derived_open/gts.x.test.golden.openchain.v1~x.test.audit.payload.v1~x.test.final.payload.v1~.schema.jsongts-macros/tests/golden/additional_properties_gts_root_open.rsgts-macros/tests/golden/additional_properties_gts_root_open/gts.x.test.golden.rootopen.v1~.schema.jsongts-macros/tests/golden/additional_properties_nested_closed.rsgts-macros/tests/golden/additional_properties_nested_closed/gts.x.test.golden.nestedclosed.v1~.schema.jsongts-macros/tests/golden_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- gts-macros/README.md
- Track undecidable allOf intersections separately from flattened schemas. - Preserve compatibility diagnostics for decidable sibling properties.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
gts/src/store.rs (1)
831-844: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winResolution failures collapse into
StoreError::SchemaNotFoundin two new call sites.resolve_schema_refsreturnsStoreError::UnresolvedRefsorStoreError::CircularRef, and both new sites discard that variant and report a missing schema instead. A caller matching onStoreErrorcannot separate an unregistered type from a document this store cannot resolve. One decision about the error contract covers both sites.
gts/src/store.rs#L831-L844: propagate the originalresolve_schema_refserror fromcast, keeping the'{instance_type_id}'and'{target_type_id}'context in the message.gts/src/store.rs#L978-L983: apply the same treatment incompare_documents, and update the# Errorsdoc at lines 969-972 to name the variant the method actually returns.🤖 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 831 - 844, Preserve the original resolve_schema_refs error variants in both cast at gts/src/store.rs:831-844 and compare_documents at gts/src/store.rs:978-983, while retaining the instance_type_id and target_type_id context in each message; update compare_documents’ # Errors documentation at gts/src/store.rs:969-972 to name the propagated variant instead of SchemaNotFound.gts/src/schema_derivation.rs (1)
328-328: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the flattened ancestor across the
allOfbranches.Line 328 calls
flatten_schema(ancestor_schema)on entry to every invocation. TheallOfloop at lines 386-398 re-enters with the sameancestor_schema, so the same ancestor subtree is cloned and folded once per branch, and again at each nested level. A descendant with manyallOfbranches repeats the full ancestor flatten for each one.Pass the already-flattened ancestor into the recursive call, or split the walk so the flatten happens once per distinct ancestor node.
Also applies to: 386-398
🤖 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_derivation.rs` at line 328, Update the schema-flattening flow around flatten_schema and its allOf recursion to flatten each ancestor node once and reuse that result across branches. Pass the already-flattened ancestor into recursive calls in the allOf loop instead of invoking flatten_schema(ancestor_schema) again, while preserving the existing merge behavior.
🤖 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/src/schema_derivation.rs`:
- Around line 12-14: Update the module-level doc comment in schema_derivation to
point its checker reference from crate::schema_cast to crate::schema_evolution,
matching the source module imported by check_accepted_set_inclusion and
flatten_schema.
- Around line 276-278: Remove the excessive whitespace in the error message
format string within the schema derivation validation path, using the file’s
existing trailing-backslash string continuation style so the rendered text
contains a single space before “base”.
In `@gts/src/schema_evolution.rs`:
- Around line 357-366: Update the MINIMUMS and MAXIMUMS handling in
flatten_schema so a bound intersection is marked unproven whenever either
current or candidate_value is non-numeric, including boolean
exclusiveMinimum/exclusiveMaximum values. Only compare and replace bounds when
both values are numeric; preserve the existing minimum/maximum tightening
behavior for numeric values.
---
Nitpick comments:
In `@gts/src/schema_derivation.rs`:
- Line 328: Update the schema-flattening flow around flatten_schema and its
allOf recursion to flatten each ancestor node once and reuse that result across
branches. Pass the already-flattened ancestor into recursive calls in the allOf
loop instead of invoking flatten_schema(ancestor_schema) again, while preserving
the existing merge behavior.
In `@gts/src/store.rs`:
- Around line 831-844: Preserve the original resolve_schema_refs error variants
in both cast at gts/src/store.rs:831-844 and compare_documents at
gts/src/store.rs:978-983, while retaining the instance_type_id and
target_type_id context in each message; update compare_documents’ # Errors
documentation at gts/src/store.rs:969-972 to name the propagated variant instead
of SchemaNotFound.
🪄 Autofix
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: 4ac08ff0-1805-450a-9f75-5cca4075db2e
📒 Files selected for processing (9)
gts/src/lib.rsgts/src/ops.rsgts/src/schema_cast.rsgts/src/schema_compat.rsgts/src/schema_derivation.rsgts/src/schema_evolution.rsgts/src/schema_traits.rsgts/src/store.rsgts/src/store_test.rs
💤 Files with no reviewable changes (1)
- gts/src/schema_compat.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- gts/src/schema_traits.rs
- gts/src/ops.rs
- Move accepted-set compatibility into a dedicated schema evolution module. - Reuse the shared inclusion engine for derivation admission while preserving derivation-specific rules. - Update casting, store, trait validation, and tests to use the focused APIs. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
| format!("{path}.{name}") | ||
| }; | ||
| if let Some(new_property) = new_props.get(name) { | ||
| check_schema_node_compatibility( |
There was a problem hiding this comment.
HIGH
Unbounded recursion: check_schema_node_compatibility recurses into every declared property and into items with no depth counter and no cycle set, while sibling walkers in this crate guard on MAX_RECURSION_DEPTH = 64 explicitly to prevent stack overflow (schema_traits.rs:673, schema_derivation.rs:320). The walk runs on documents reached via GET /compatibility -> GtsStore::is_compatible -> SchemaComparison::of_resolved, and resolve_schema_refs inlines gts:// refs from other registered schemas before the walk, so the walked document can be far deeper than any single posted document.
Thread a depth: usize parameter through check_schema_node_compatibility/check_object_compatibility and emit a CompatibilityFinding::NotProvable diagnostic once depth >= MAX_RECURSION_DEPTH, matching the guard in schema_derivation.rs:320.
| ] | ||
| }); | ||
|
|
||
| let flattened = flatten_schema(&schema); |
There was a problem hiding this comment.
HIGH
Coverage lost in the schema_compat.rs deletion: test_extract_with_allof and test_extract_simple_schema were removed with that file and no new test replaces their assertion that the effective schema unions required across multiple allOf branches. test_all_of_intersects_duplicate_property_schemas asserts only merged minLength/maxLength, and test_check_schema_compatibility_allof_flatten_equivalence declares required in one branch only. Verified by mutation: making the "required" arm of merge_schema_map a no-op leaves all 671 cargo test -p gts --lib tests green.
Add a flatten_schema test asserting the flattened required array contains the union of names declared by two separate allOf branches, restoring the assertion deleted with test_extract_with_allof.
| "properties": {"x": false} | ||
| }); | ||
| let errs = validate_derivation_compatibility(&base, &derived, "b", "d"); | ||
| assert!(!errs.is_empty()); |
There was a problem hiding this comment.
HIGH
test_property_disabled_fails asserts only assert!(!errs.is_empty()), and its base declares required: ["x"] while the derived schema does not, so required-set widening alone produces an error. Verified by mutation: replacing the collect_disabled_base_properties call at schema_derivation.rs:115 with a no-op leaves all 31 schema_derivation tests passing, so the disabled-property admission rule has zero effective coverage.
Assert the specific diagnostic - errs.iter().any(|e| e.contains("disables property defined in base") && e.contains("'x'")) - and drop required from the base so the disabled-property rule is the only source of the error.
| .iter() | ||
| .find(|diagnostic| diagnostic.path == "$.payload") | ||
| .expect("the offending level must be named, not the document root"); | ||
| assert_eq!(finding.finding, CompatibilityFinding::PropertyAdded); |
There was a problem hiding this comment.
HIGH
Only 2 of the 11 CompatibilityFinding variants are ever asserted (PropertyAdded here, NotProvable at 1479/1533). The ~50 other evolution tests assert only the coarse verdict via is_compatible/is_incompatible, leaving the machine-readable classification of the new public serialized diagnostics untested. Verified by mutation: renaming every emitted CompatibilityFinding::RequiredChanged to EnumChanged and TypeChanged to BoundChanged leaves all 671 cargo test -p gts --lib tests green.
In the existing directional tests (test_check_schema_compatibility_added_required_property, _type_change, _enum_reduction, test_numeric_bounds_are_checked_without_a_type_keyword, test_dialect_change_is_not_proven_compatible), call check_backward_diagnostics/check_forward_diagnostics and assert_eq! the expected CompatibilityFinding variant and path.
| unproven.extend(paths); | ||
| effective | ||
| } else { | ||
| old_schema.clone() |
There was a problem hiding this comment.
HIGH
check_schema_node_compatibility deep-clones the entire remaining schema subtree at every recursion step (old_schema.clone() here and new_schema.clone() at 1104) whenever the node has no allOf, then recurses into borrows of those clones per property (1313, 1326, 1357) and for items (1200), so one comparison copies every subtree once per ancestor level instead of borrowing it.
Bind old_effective/new_effective as std::borrow::Cow<'_, Value> - Cow::Owned(flatten_effective(...).0) in the allOf branch and Cow::Borrowed(old_schema) otherwise - since every later use (1108, 1126, 1147, 1199) only reads through a reference.
| return; | ||
| } | ||
|
|
||
| let ancestor = flatten_schema(ancestor_schema); |
There was a problem hiding this comment.
MEDIUM
collect_closed_descendant_branch_errors calls flatten_schema(ancestor_schema) - a full deep copy plus allOf merge of the ancestor document - at the top of every invocation, while the allOf recursion at 389 re-enters with the same ancestor_schema, so the identical ancestor is re-flattened once per descendant allOf branch at every depth.
Add a parameter carrying the already-flattened ancestor (ancestor_flat: &Value) and pass it through unchanged in the allOf loop at 389, flattening only in the property recursion at 375 where the ancestor actually changes.
| flatten_schema(schema) | ||
| .get("properties") | ||
| .and_then(Value::as_object) | ||
| .cloned() |
There was a problem hiding this comment.
MEDIUM
The effective_properties closure deep-clones the entire flattened properties map (.cloned() here) for both the base and the derived schema, although the loop at 274-275 only reads property names and compares each value with Value::Bool(false).
Bind the two flatten_schema results to locals (let base_flat = flatten_schema(base_schema);) and iterate base_flat.get("properties").and_then(Value::as_object).into_iter().flatten() so the maps are borrowed instead of copied.
| /// unregistered type from a request it should not have made at all. | ||
| fn compared_schema_entity(&mut self, type_id: &str) -> Result<GtsEntity, String> { | ||
| self.get_schema_entity(type_id) | ||
| .cloned() |
There was a problem hiding this comment.
MEDIUM
compared_schema_entity clones the whole GtsEntity - including the complete schema document in content - for both sides of is_compatible, and the clones are used only as &old_ent.content / &new_ent.content inputs to resolve_schema_refs at 894 and 902, which itself returns a freshly owned resolved document.
Warm the reader cache with self.get(old_type_id)/self.get(new_type_id) first, then look both entities up through a shared borrow (self.by_id.get(id), as SchemaProvider::schema_content already does) and pass &entity.content to resolve_schema_refs - two shared borrows of self coexist, so no clone is needed.
| let entry = resolved.entry(name.clone()).or_insert_with(|| { | ||
| order.push(name.clone()); | ||
| (sch.clone(), None, None) | ||
| (sch.clone(), None) |
There was a problem hiding this comment.
MEDIUM
sch.clone() deep-copies every property schema a second time into resolved (collect_props_recursive at 686 already deep-cloned each declaration into all_props), and default_val.clone() at 706 copies each default subtree again, once per property declaration along the whole trait chain.
Key resolved on borrowed data - HashMap<&str, (&Value, Option<&Value>)> over all_props, which outlives the loop - and dereference at the two consumption sites (711-734), keeping the single owning clone at 717 where the value is inserted into result.
| } | ||
|
|
||
| let source_map = if check_backward { old_map } else { new_map }; | ||
| if enumerated_source_is_included(source_map, target) { |
There was a problem hiding this comment.
MEDIUM
enumerated_source_is_included is called before any keyword comparison at every schema node, and it compiles a fresh jsonschema validator for the whole counterpart subtree (validator_for at 926) for every node whose source side declares const/enum, so an enum-bearing schema pays a full validator build per node even when the cheap keyword checks would have proven compatibility with no diagnostics.
Collect the keyword diagnostics at 1151-1176 into a local Vec first, return immediately when it is empty, and only call enumerated_source_is_included (discarding the diagnostics if it proves inclusion) when that vector is non-empty.
3 participants
Summary by CodeRabbit
v0.*behavior.