ADRs: types derivation & x-gts-traits-schema & x-gts-traits changes#77
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds ADR-0001 through ADR-0004, README updates, example schema edits, and test changes to formalize dialect-agnostic GTS Type Schemas, allow optional ChangesGTS Type Schema Derivation and Trait Validation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 1359-1366: The README currently contradicts itself about
x-gts-traits-schema: update the earlier normative phrasing that currently
asserts x-gts-traits-schema "MUST" have "type": "object" and the keyword table
listing JSON type as object so it matches the later allowance of boolean values;
change that MUST to a conditional statement (e.g., "If x-gts-traits-schema is an
object subschema, it MUST include \"type\": \"object\"" or "x-gts-traits-schema
MUST be either a subschema (object) or a boolean"), and update the keyword/table
entry for x-gts-traits-schema to list its JSON type as "object | boolean" (or
equivalent text clarifying booleans are allowed) while preserving the existing
sentence that the effective trait-schema must constrain trait values to objects
when an object subschema is used.
🪄 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
Run ID: 35608ef5-2971-4b5d-87d8-2c542fa71332
📒 Files selected for processing (3)
README.mdadr/0001-derivation-form.mdadr/0002-x-gts-traits-schema.md
aviator5
force-pushed
the
adr-derivation-form
branch
from
9b3d0db to
3a45d41
Compare
aviator5
changed the title
aviator5
force-pushed
the
adr-derivation-form
branch
from
6c83be3 to
06225c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 1509-1513: Change the normative language that says "Defaults
declared in the effective trait-schema SHOULD be applied" to a MUST and clarify
that defaults must be materialized into the effective trait object prior to the
OP#13 completeness/registration validation; specifically update the sentence
mentioning "Defaults declared in the effective trait-schema" / "ADR-0003
materialization" so it reads that defaults MUST be applied during
materialization before completeness checks run (aligning the README's
registration-time completeness flow with OP#13).
🪄 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
Run ID: 8f938104-ba5e-47a4-869a-a0e3b0c3465d
📒 Files selected for processing (2)
README.mdadr/0004-x-gts-traits-merge-strategy.md
✅ Files skipped from review due to trivial changes (1)
- adr/0004-x-gts-traits-merge-strategy.md
aviator5
force-pushed
the
adr-derivation-form
branch
2 times, most recently
from
832b739 to
708c7b0
Compare
aviator5
changed the title
Artifizer
reviewed
May 28, 2026
| | 0.9 | Add `x-gts-final` and `x-gts-abstract` schema modifiers; enforce final/abstract semantics in OP#6 and OP#12 | | ||
| | 0.10 | BREAKING: terminology unified around GTS Type / GTS Instance; rename API fields `schema_id` → `type_id` (also `old_schema_id`/`new_schema_id`/`to_schema_id`/`selected_schema_id_field`); rename API field `is_schema` → `is_type` (type-definition vs instance discriminator); `type_id` MUST be a GTS Type Identifier or `null` — no longer falls back to JSON Schema dialect URL; rename endpoints `/validate-schema` → `/validate-type`, `/schemas` → `/types`; rename OP#12 'Schema vs Schema Validation' → 'Type Derivation Validation'; rename OpenAPI components `ValidateSchemaRequest` → `ValidateTypeRequest`, `SchemaRegister` → `TypeRegister`; rename example directories `examples/**/schemas/` → `examples/**/types/` (file extensions `.schema.json` retained); add Terminology section | | ||
| | 0.11 | Introduce term **GTS Type Schema** as the canonical definition of a GTS Type; remove the standalone `Schema` term from Terminology; rewrite `GTS Type` entry to name the abstract registered entity; rename `GTS Type Registry` → `GTS Registry` (registry now scopes both Type Schemas and well-known Instances). **Conformance tests for reference implementations** also updated: rename API endpoints `/validate-type` → `/validate-type-schema` and `/types` → `/type-schemas`; rename OpenAPI components `TypeRegister` → `TypeSchemaRegister`, `ValidateTypeRequest` → `ValidateTypeSchemaRequest`; rename request field `TypeSchemaRegister.schema` → `TypeSchemaRegister.type_schema`; rename helper `validate_type` → `validate_type_schema`. | | ||
| | 0.12 | BREAKING: frame GTS Type Schemas as a **JSON Schema Dialect** over Draft-07 — derivation compatibility (§3.2, OP#12) applies whether the derived schema uses `allOf` + `$ref` or re-declares parent fields directly; finality guard (§9.11) is determined from the chained `$id` alone (ADR-0001). **Reframe `x-gts-traits-schema` as a JSON Schema subschema** (object, `true`, or `false`); the registry composes all `x-gts-traits-schema` declarations along the `$id` chain via `allOf` into a single effective trait-schema — descendants no longer need to repeat ancestor trait-schemas (ADR-0002). **Trait completeness keyed on `x-gts-abstract`**: completeness is enforced at registration on non-abstract types (not only leaves); the materialized effective traits object (defaults applied) is validated against the effective trait-schema (ADR-0003). **Trait-value merge replaced with JSON Merge Patch (RFC 7396)** along the `$id` chain — descendant last-wins at scalar/array/`null` leaves, nested objects merge recursively, arrays replace wholesale, `null` deletes the key (intended fallback to the trait-schema's `default` via materialization); cross-descendant locking moves from a GTS-specific immutability rule to standard JSON Schema `const` in `x-gts-traits-schema` (ADR-0004). | |
Contributor
There was a problem hiding this comment.
Should not we use draft-2020 finally?
Collaborator
Author
There was a problem hiding this comment.
Since we don't rely on any specific features of draft-07 / draft-2019 / draft-2020 for now, we decided to be dialect-agnostic.
aviator5
force-pushed
the
adr-derivation-form
branch
from
ad7b221 to
cc040cb
Compare
…nd add ADR-0001 on derivation form - Rewrite §11.0 to frame GTS Type Schemas as an extension of JSON Schema with vendor x-gts-* keywords and registry-enforced semantic rules. The framing is dialect-agnostic: implementations MUST honour whichever JSON Schema dialect is declared in $schema. The spec's examples use Draft-07 as the baseline for maximum interoperability, but Draft 2019-09 and Draft 2020-12 are equally acceptable. Post-Draft-07 keywords ($defs, prefixItems, unevaluatedProperties, etc.) MAY be used provided the chosen $schema admits them. GTS is not a JSON Schema Dialect in the formal sense (no dedicated $schema URI or meta-schema); GTS-specific constraints are enforced at the registry. - Rename §11.0 to "Relationship to JSON Schema". - Update Terminology entry for "GTS Type Schema" to match the dialect-agnostic framing (the spec's examples use Draft-07 as baseline; 2019-09 / 2020-12 are equally supported). - Clarify §3.2 and OP#12 wording so that derivation compatibility applies regardless of whether the derived schema references the parent via allOf + $ref or re-declares parent fields directly. - Clarify §9.11 finality guard is determined from the chained $id alone, not the body's use of allOf. - Add adr/0001-derivation-form.md documenting the decision to treat GTS as a JSON Schema extension (dialect-agnostic; not a formal Dialect) and not mandate allOf for derivation. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
…DR-0002 - README §9.7: reframe x-gts-traits-schema as a JSON Schema subschema (object or boolean true/false), document chain aggregation via allOf, and link the new ADR. - adr/0002-x-gts-traits-schema.md: record the decision (Option 2A — subschema + implicit chain aggregation along the $id chain), including rejected alternatives, normative consequences, and cross-references to ADR-0001. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- Rewrite §9.7.5 'Validation' to require completeness only for non-abstract types, with materialization via effective trait-schema defaults before validating against required. - Rewrite §9.11.4 to express the rule via the x-gts-abstract modifier; final types fall out as a corollary (non-abstract by definition). - Refine OP#13 wording in §9.7 to match: completeness applies to non-abstract types and operates on the materialized effective traits object. - Add ADR-0003 documenting the decision to enforce trait completeness at type registration (Option 3), keyed on x-gts-abstract rather than the registry-state-dependent 'leaf' notion. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
…d ADR-0004 - Replace prior trait-value merge rule in README §9.7 with chain-applied JSON Merge Patch (RFC 7396) along the $id chain: descendant-last-wins at leaves, nested objects merge recursively, arrays replace wholesale, `null` deletes the key. - Make standard JSON Schema `const` in x-gts-traits-schema the publisher's lock mechanism (no GTS-specific immutability rule); a descendant that attempts to override a const-locked value fails ordinary JSON Schema validation against the effective trait-schema. - Document the principal use of `null` as a fallback to the trait-schema's `default` via ADR-0003 materialization, including the failure mode when the deleted key is required-without-default. - Update OP#13 description to drop the obsolete immutability sentence and point at `const` as the lock mechanism. - Add ADR-0004 with drivers, alternatives (no-merge, shallow last-wins, shallow immutable-once-set, RFC 7396, per-property keyword), decision, worked examples (override / nested merge / const lock / null-to-default fallback), edge cases, and conformance expectations. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
adr-derivation-form
branch
3 times, most recently
from
d4fffbf to
bfdeb1b
Compare
- README §9.7.5/§9.7.2: remove the "immutable defaults" rule for x-gts-traits-schema. Defaults are JSON Schema annotations and do not participate in OP#12 narrowing (narrowing is defined over the set of valid instances, which `default` does not affect); descendants MAY redeclare them. Publishers who want a trait value to be fixed across descendants use `const` — the same lock mechanism already endorsed by ADR-0004 for trait values. This removes the asymmetry with regular property defaults, which were never restricted. - adr/0003-x-gts-traits-completeness.md: drop the stale "subject to the immutable-once-set rule" parenthetical in the algorithm description; reference ADR-0004's RFC 7396 merge instead. ADR-0004 explicitly chose Option 2c (RFC 7396) over Option 2b (immutable-once-set), so the parenthetical was a leftover from an earlier draft. - README version footer: bump to 0.12 (carried over from the prior commit, now consolidated here). Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
ADR-0001 (derivation form is dialect-agnostic):
- Add register_derived_redeclared helper for tests that author derived
schemas without allOf (chained $id alone establishes derivation).
- op12: add Redeclared_* cases covering compatible re-declaration,
type tightening, constraint loosening, dropping parent's required,
AP:false tightening, final-base rejection by $id chain, and the
hybrid allOf+toplevel form.
- Refresh "x-gts-{final,abstract} inside allOf rejected" docstrings to
cite ADR-0001 explicitly.
ADR-0002 (x-gts-traits-schema as a JSON Schema subschema):
- Drop TraitsSchemaNotObject (object/true/false are all admissible
under the new value space); replace with TraitsValueViolatesInteger
Schema covering value-level failure against an integer subschema.
- Add cases for chain allOf aggregation, descendant-omits-decl,
descendant-adds-new-field, boolean true/false (positive and
negative), incompatible descendant schema, redundant ancestor ref,
object form regression, standalone ref-pattern + 3-level chain.
- Rework CyclingRef_SelfRef into a TRUE self-cycle (the prior shape
— two identical $refs inside allOf — is allowed in v0.12).
ADR-0003 (trait completeness keyed on x-gts-abstract):
- Add register_abstract helper.
- Add cases: abstract type skips completeness with unresolved
required, non-abstract intermediate must be complete, abstract base
+ concrete descendant satisfies, default satisfies required,
null-delete on required-no-default fails for non-abstract.
- Update interaction tests' docstrings (final-with-traits-resolved,
final-with-traits-missing, abstract-with-incomplete-traits-ok) to
cite ADR-0003.
ADR-0004 (x-gts-traits as RFC 7396 JSON Merge Patch):
- Remove the three "override MUST fail" classes:
TraitsInvalid_OverrideInChain and OverrideTopicRef3Level (value
overrides are now last-wins); ChangeDefaultInMid (trait-schema
default redeclaration is allowed per the relaxed §9.7.5 rule).
- Add Merge_* cases: scalar override last-wins, 3-layer last-wins,
nested object recursive merge, array wholesale replacement
(positive + negative proving no element-merge), null-delete falls
back to default, null-delete + abstract descendant OK, const lock
rejects override, const lock idempotent restatement, idempotent
scalar restatement.
Positive coverage for the relaxed default rule:
- op12: Redeclared_ChangePropertyDefaultAllowed — derived may freely
redeclare `properties.<name>.default` (annotation, not a constraint).
- op13: TraitsSchema_RedeclareDefaultAllowed — descendant may freely
redeclare a trait-schema `default`; effective default at
materialization time is the leaf-most one along the chain.
Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
adr-derivation-form
branch
from
bfdeb1b to
e1a7eb2
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/helpers/http_run_helpers.py (2)
80-86: 💤 Low valueSpread order allows
bodyto override$$idand$$schema.Lines 80-84 spread
bodyafter setting$$idand$$schema, so ifbodycontains these keys it would silently overwrite the function's injected values. The same pattern appears inregister()(line 15) andregister_abstract()(line 105), so this is consistent across helpers.If this is intentional (to allow edge-case testing), no change needed. Otherwise, consider either:
- Spreading
bodyfirst:full = {**body, "$$id": gts_id, ...}, or- Documenting that
bodymust not contain$$idor$$schema.Current usage in tests is safe (no test passes these keys in
body), so this is a preventive suggestion.🤖 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 `@tests/helpers/http_run_helpers.py` around lines 80 - 86, The construction of the `full` dict currently spreads `body` after setting `$$id` and `$$schema`, allowing keys in `body` to silently override the injected values; change the merge order so injected keys win by spreading `body` first (e.g., build `full` from `body` then set `$$id`/`$$schema`), and apply the same fix to the analogous places in `register()` and `register_abstract()` so `gts_id` and the schema cannot be overwritten by `body`.
38-55: 💤 Low valueDocument interaction when trait keywords appear in both
overlayandtop_level.The hoisting logic extracts trait keywords from
overlay(lines 39-43) and merges them at line 53, buttop_levelis merged afterward (line 55), so iftop_levelcontainsx-gts-traitsorx-gts-traits-schema, it would silently override the hoisted values. The docstring describestop_levelas "extra keys" but doesn't explicitly forbid trait keywords.Consider either:
- Documenting this override behavior in the docstring, or
- Adding a runtime check that raises if
top_levelcontains trait keywords.For test helpers this is low risk, but clarifying the intended behavior would prevent confusion.
🤖 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 `@tests/helpers/http_run_helpers.py` around lines 38 - 55, The hoisting logic moves trait keys from overlay into hoisted and then merges hoisted into body before merging top_level, but top_level can silently override hoisted values; update the helper (the block that builds body using overlay, hoisted, and top_level) to either document this behavior in the function docstring or add a runtime check that raises if top_level contains any of the trait keywords ("x-gts-traits", "x-gts-traits-schema"); specifically, after computing hoisted and before body.update(top_level) verify that none of those keys are present in top_level (raise a ValueError with a clear message identifying the conflicting keys) so callers cannot accidentally override hoisted trait values.
🤖 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 `@tests/helpers/http_run_helpers.py`:
- Around line 80-86: The construction of the `full` dict currently spreads
`body` after setting `$$id` and `$$schema`, allowing keys in `body` to silently
override the injected values; change the merge order so injected keys win by
spreading `body` first (e.g., build `full` from `body` then set
`$$id`/`$$schema`), and apply the same fix to the analogous places in
`register()` and `register_abstract()` so `gts_id` and the schema cannot be
overwritten by `body`.
- Around line 38-55: The hoisting logic moves trait keys from overlay into
hoisted and then merges hoisted into body before merging top_level, but
top_level can silently override hoisted values; update the helper (the block
that builds body using overlay, hoisted, and top_level) to either document this
behavior in the function docstring or add a runtime check that raises if
top_level contains any of the trait keywords ("x-gts-traits",
"x-gts-traits-schema"); specifically, after computing hoisted and before
body.update(top_level) verify that none of those keys are present in top_level
(raise a ValueError with a clear message identifying the conflicting keys) so
callers cannot accidentally override hoisted trait values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5a8729f-9741-471a-b899-65a862839b3a
📒 Files selected for processing (13)
README.mdadr/0001-derivation-form.mdadr/0002-x-gts-traits-schema.mdadr/0003-x-gts-traits-completeness.mdadr/0004-x-gts-traits-merge-strategy.mdexamples/events/types/gts.x.core.events.type.v1~x.commerce.orders.order_placed.v1.0~.schema.jsonexamples/events/types/gts.x.core.events.type.v1~x.commerce.orders.order_placed.v1.1~.schema.jsonexamples/events/types/gts.x.core.events.type.v1~x.core.idp.contact_created.v1~.schema.jsontests/helpers/http_run_helpers.pytests/test_op12_type_derivation_validation.pytests/test_op13_schema_traits_validation.pytests/test_refimpl_x_gts_final_abstract.pytests/test_xgts_keyword_placement.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_refimpl_x_gts_final_abstract.py
- adr/0003-x-gts-traits-completeness.md
…ening - Reframe two OP#12 additionalProperties cases to expect acceptance, since draft-07 allOf is a conjunction that composes rather than merges branches. - AP:true in a derived overlay cannot override the base branch's AP:false, so the combined schema stays closed and is not loosening. - Omitting AP in a derived schema inherits closedness via the $ref half of the allOf, so OP#12 must accept it. - Document the draft-07 semantics in each test docstring. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
adr-derivation-form
branch
from
e1a7eb2 to
a25de85
Compare
Artifizer
approved these changes
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tighten the GTS spec by formalising four design decisions as ADRs and aligning README §3.2, §9.7, §9.11 and §11.0 accordingly:
Summary by CodeRabbit