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
| //! owns no keyword semantics of its own: it calls the checker in | ||
| //! [`crate::schema_cast`] and adds the two admission rules that inclusion alone | ||
| //! does not express. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the module reference in the doc comment.
The comment states that this module calls the checker in crate::schema_cast. The import at line 16 takes check_accepted_set_inclusion and flatten_schema from crate::schema_evolution. Point the doc link at crate::schema_evolution.
📝 Proposed doc fix
//! accepted-instance-set inclusion that schema evolution checks, so this module
//! owns no keyword semantics of its own: it calls the checker in
-//! [`crate::schema_cast`] and adds the two admission rules that inclusion alone
+//! [`crate::schema_evolution`] and adds the two admission rules that inclusion alone
//! does not express.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //! owns no keyword semantics of its own: it calls the checker in | |
| //! [`crate::schema_cast`] and adds the two admission rules that inclusion alone | |
| //! does not express. | |
| //! owns no keyword semantics of its own: it calls the checker in | |
| //! [`crate::schema_evolution`] and adds the two admission rules that inclusion alone | |
| //! does not express. |
🤖 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` around lines 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.
| errors.push(format!( | ||
| "property '{name}': derived schema '{derived_id}' disables property defined in base '{base_id}'" | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the stray whitespace run from the error message.
The format string contains 18 consecutive spaces before base '{base_id}'. The rendered validation error reads disables property defined in base 'b'. Other messages in this file wrap with a trailing \ continuation, for example lines 350-352. Apply the same form here.
🐛 Proposed fix for the message text
errors.push(format!(
- "property '{name}': derived schema '{derived_id}' disables property defined in base '{base_id}'"
+ "property '{name}': derived schema '{derived_id}' disables property defined in \
+ base '{base_id}'"
));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| errors.push(format!( | |
| "property '{name}': derived schema '{derived_id}' disables property defined in base '{base_id}'" | |
| )); | |
| errors.push(format!( | |
| "property '{name}': derived schema '{derived_id}' disables property defined in \ | |
| base '{base_id}'" | |
| )); |
🤖 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` around lines 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”.
- 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>
2 participants
Summary by CodeRabbit
v0.*behavior.