docs: add ADR template and contribution process - #92
Conversation
📝 WalkthroughWalkthroughThe change updates contributor guidance, adds an ADR template, clarifies four ADR decisions, documents ChangesDocumentation and ADR decisions
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
code-ranker report for this PR (built on fork): https://reports.code-ranker.com/0FGX7CpmwJdMIFYuUtk0jA/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@adr/0003-x-gts-traits-completeness.md`:
- Line 230: Resolve the contradiction in the Option 3 section of the ADR by
aligning its completeness requirement with the `false` schema behavior: either
explicitly exempt types whose materialized effective traits object is empty, or
revise the `x-gts-traits-schema: false` edge-case wording to state that no
non-abstract descendant can be complete. Ensure the selected wording
consistently matches the completeness algorithm.
In `@adr/0004-x-gts-traits-merge-strategy.md`:
- Around line 483-489: Update the trait merge and validation semantics in §9.7.5
so RFC 7396 null-based deletion cannot bypass an optional const-locked trait:
either require locked properties in the effective trait-schema or reject
deletion during merge. Ensure the selected behavior preserves const enforcement,
and add a conformance test covering a descendant setting an optional
const-locked trait to null.
🪄 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: b378a9fb-af05-415a-b218-fe9db5024981
📒 Files selected for processing (6)
CONTRIBUTING.mdadr/0001-derivation-form.mdadr/0002-x-gts-traits-schema.mdadr/0003-x-gts-traits-completeness.mdadr/0004-x-gts-traits-merge-strategy.mdadr/template.md
| - **§9.7.5 ("Trait merge and validation semantics")** carries the normative wording of RFC 7396 merge along the `$id` chain and the `const`-based lock mechanism. | ||
| - **ADR-0003** stays correct as written; the "chain-merged effective traits object" referenced there is now formally defined as the result of applying each layer's `x-gts-traits` as a JSON Merge Patch (RFC 7396) to the chain-merged object so far, root → leaf. | ||
| - **OP#13 description (§9.7)** is unaffected; it speaks generically of "chain-merged" values. | ||
| - **§9.11.4 (modifiers ↔ traits)** is unaffected; completeness keying on `x-gts-abstract` is independent of merge policy. | ||
| - **Reference implementations (gts-go, gts-rust)** must implement RFC 7396 merge along the chain. Available implementations exist in both ecosystems. The registry MUST NOT enforce a "different value MUST fail" rule on its own — it relies on standard JSON Schema validation against the effective trait-schema (which catches `const` violations naturally). | ||
| - **Conformance test suite** should exercise: (a) descendant overrides a top-level scalar — succeeds (last-wins); (b) descendant overrides one field of a nested-object trait — other nested fields preserved; (c) descendant overrides an array-valued trait — array replaces wholesale; (d) descendant writes `null` at a leaf — the key is removed; (e) descendant repeats the same value — succeeds (idempotent); (f) publisher locks via `const`, descendant attempts override — fails JSON Schema validation; (g) chain with three layers; middle layer overrides base; leaf overrides middle. | ||
| - **Backward compatibility.** This decision defines behavior that was previously unspecified; implementations using a different merge strategy must update to RFC 7396 semantics. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant ADR section =="
sed -n '440,510p' adr/0004-x-gts-traits-merge-strategy.md 2>/dev/null || true
echo
echo "== Search lock/null/merger definitions =="
rg -n "x-gts-traits|x-gts-lock|lock|json merge|RFC 7396|constrains|requires|required|null|#\\{|const" adr/0004-x-gts-traits-merge-strategy.md 2>/dev/null || true
echo
echo "== Search test/import fixtures for trait lock behavior =="
rg -n "gts-lock|x-gts-lock|trait.*lock|RFC 7396|merge.*null|null.*trait|const" -S . -g '!**/.git/**' --iglob '*.*' | head -n 200 || trueRepository: GlobalTypeSystem/gts-spec
Length of output: 50383
🌐 Web query:
JSON Merge Patch RFC 7396 null deletes key
💡 Result:
Yes, according to RFC 7396, a null value in a JSON Merge Patch indicates that the corresponding key should be removed from the target document [1][2]. In the JSON Merge Patch specification, the algorithm for applying a patch is defined as follows [1][3]: - If the patch member value is null, the corresponding member is removed from the target document if it exists [1][3]. - If the patch member value is not null, the value is updated or added [1][3]. Because null is assigned this specific "delete" meaning, JSON Merge Patch does not support setting a key's value to null [4][5]. If your application requires the ability to explicitly set fields to null, you should use a different format, such as JSON Patch (RFC 6902) [4][6].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc7396.html
- 2: https://datatracker.ietf.org/doc/rfc7396/
- 3: https://datatracker.ietf.org/doc/html/rfc7396.html
- 4: https://jsonic.io/guides/json-merge-patch
- 5: https://docs.oracle.com/en/database/oracle/oracle-database/26/adjsn/oracle-sql-function-json_mergepatch.html
- 6: https://stackoverflow.com/questions/45121985/handling-nulls-in-json-merge-patch
🌐 Web query:
JSON Schema const keyword validates only when member present does not make required
💡 Result:
In JSON Schema, the "const" keyword does not require the property it validates to be present in the JSON instance [1][2]. The "const" keyword is an assertion that validates the value of an instance against a fixed value [3][4][5]. When "const" is applied to a property within an object's "properties" schema, it only evaluates the value of that property if the property is actually present in the data [1][2]. If the property is absent, the validation for that specific schema branch is skipped, resulting in a successful validation (or "pass") for that property constraint [1][2]. This behavior is consistent with the core principle of JSON Schema, where keywords define constraints that only apply if the data they reference exists [2][6]. To mandate that a property must be present, you must explicitly use the "required" keyword [2][6]. If you need a property to be present and equal to a specific value, you should use both keywords together: { "type": "object", "properties": { "myProperty": { "const": "expectedValue" } }, "required": ["myProperty"] } In this example, the "required" keyword ensures the property exists [6], and the "const" keyword ensures that if it exists, it must have the specified value [3][5].
Citations:
- 1: rule not validated when property is not present at all eclipsesource/jsonforms#1686
- 2: conditional validation is always true when the properties is
undefinedajv-validator/ajv#913 - 3: https://json-schema.org/draft-07/json-schema-validation
- 4: https://github.com/json-schema-org/json-schema-spec/blob/main/specs/jsonschema-validation.md
- 5: https://json-schema.org/understanding-json-schema/reference/const
- 6: https://www.learnjsonschema.com/draft7/validation/required/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ADR-0003 relevant lines =="
sed -n '230,275p' adr/0003-x-gts-traits-completeness.md 2>/dev/null || true
echo
echo "== Search const/required lock examples in relevant ADRs =="
rg -n 'locked lock publisher const required|null|delete|RFC 7396|x-gts-traits-schema|implication|conformance' adr/0004-x-gts-traits-merge-strategy.md adr/0003-x-gts-traits-completeness.md -SRepository: GlobalTypeSystem/gts-spec
Length of output: 21693
🌐 Web query:
"I should write: const is not a requirement" JSON Schema
💡 Result:
The const keyword is not a requirement in JSON Schema; it is an optional keyword used to restrict a value to a single, specific JSON value [1][2]. Using the const keyword is functionally equivalent to an enum keyword with a single value [3][2]. Because const restricts an instance to a specific value, it implicitly determines the type of that value, making the separate use of the type keyword redundant or, in cases where the types do not align, potentially invalid [4][5][6]. It is generally considered an anti-pattern to use the type keyword alongside const [4][6]. For example, a schema definition like this is sufficient to enforce a constant value: { "const": "my-constant-value" } When using const, you do not need to explicitly declare the type [5]. The validator will enforce that the instance matches the provided constant value exactly [7][3].
Citations:
- 1: https://json-schema.org/understanding-json-schema/reference/const
- 2: https://json-schema.org/draft/2020-12/json-schema-validation
- 3: https://github.com/json-schema-org/json-schema-spec/blob/main/specs/jsonschema-validation.md
- 4: https://www.learnjsonschema.com/2020-12/validation/const/
- 5: https://tour.json-schema.org/content/02-Primitive-Types/08-Defining-Constant-Values
- 6: https://www.learnjsonschema.com/draft7/validation/const/
- 7: validation of a "const" and whether "type" is required json-schema-org/json-schema-spec#1256
Explicitly prevent null from removing const-locked traits.
RFC 7396 deletes keys on null; JSON Schema const rejects only present mismatching values, not missing properties. For optional locked traits, a descendant using "trait": null can bypass the lock and still pass validation. Require locked properties in the effective trait-schema, or reject deletion during the RFC 7396 merge. Add a conformance case for an optional const lock being bypassed by null.
🤖 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 `@adr/0004-x-gts-traits-merge-strategy.md` around lines 483 - 489, Update the
trait merge and validation semantics in §9.7.5 so RFC 7396 null-based deletion
cannot bypass an optional const-locked trait: either require locked properties
in the effective trait-schema or reject deletion during merge. Ensure the
selected behavior preserves const enforcement, and add a conformance test
covering a descendant setting an optional const-locked trait to null.
- Require ADRs for normative specification changes and document the validation workflow. - Add a project-adapted ADR template and align existing ADR headings with it. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
1f6357c to
5537fa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@adr/0004-x-gts-traits-merge-strategy.md`:
- Around line 437-441: Update adr/0004-x-gts-traits-merge-strategy.md lines
437-441 and README.md line 1625 to state that nested required traits are
protected only when every parent path segment is present, and that protection
requires requiring each parent, restoring the complete path with defaults, or
rejecting parent deletion. Extend tests/test_op13_schema_traits_validation.py
lines 3394-3441 with nested-parent deletion coverage, distinguishing parent
removal from literal null, and assert the specific required-property validation
failure.
In `@tests/test_op13_schema_traits_validation.py`:
- Around line 3394-3441: Extend TestCaseOp13_Merge_ConstLock_NullDeleteFails
with a nested parent object containing a required const property, then attempt
to delete that parent through merge and assert the resulting validation failure
specifically reflects the missing required property. Ensure the assertion
distinguishes RFC 7396 null-deletion semantics from incorrectly validating
literal null.
🪄 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: 9498ea54-d1a8-49ae-a28b-da0ffa473303
📒 Files selected for processing (8)
CONTRIBUTING.mdREADME.mdadr/0001-derivation-form.mdadr/0002-x-gts-traits-schema.mdadr/0003-x-gts-traits-completeness.mdadr/0004-x-gts-traits-merge-strategy.mdadr/template.mdtests/test_op13_schema_traits_validation.py
🚧 Files skipped from review as they are similar to previous changes (4)
- adr/0001-derivation-form.md
- adr/0003-x-gts-traits-completeness.md
- adr/0002-x-gts-traits-schema.md
- adr/template.md
| `const` locks the **value**, not the presence: JSON Schema asserts nothing about an absent property, so a descendant can still drop a `const`-constrained trait with an RFC 7396 `null` patch. This is the general presence rule for traits, not a `const`-specific gap — any optional inherited trait without a `default` is removable the same way. A publisher who wants presence guaranteed too picks one of: | ||
|
|
||
| - add the property to the containing object schema's `required` array — the deletion then fails the ADR-0003 completeness check for non-abstract types (loud failure); | ||
| - declare a `default` equal to the `const` value — the deletion becomes a no-op, since materialization restores the value before validation (self-healing). | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make const deletion protection path-aware and test the defined behavior.
A nested required property is protected only while every parent object on its path is present. RFC 7396 can delete an optional parent, so JSON Schema can skip the nested required constraint. The current test also cannot distinguish deletion from validating literal null.
adr/0004-x-gts-traits-merge-strategy.md#L437-L441: require every parent path segment, restore the complete path with a default, or reject parent deletion.README.md#L1625-L1625: mirror the path-aware rule.tests/test_op13_schema_traits_validation.py#L3394-L3441: add nested-parent coverage and assert the specific required-property failure.
📍 Affects 3 files
adr/0004-x-gts-traits-merge-strategy.md#L437-L441(this comment)README.md#L1625-L1625tests/test_op13_schema_traits_validation.py#L3394-L3441
🤖 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 `@adr/0004-x-gts-traits-merge-strategy.md` around lines 437 - 441, Update
adr/0004-x-gts-traits-merge-strategy.md lines 437-441 and README.md line 1625 to
state that nested required traits are protected only when every parent path
segment is present, and that protection requires requiring each parent,
restoring the complete path with defaults, or rejecting parent deletion. Extend
tests/test_op13_schema_traits_validation.py lines 3394-3441 with nested-parent
deletion coverage, distinguishing parent removal from literal null, and assert
the specific required-property validation failure.
| class TestCaseOp13_Merge_ConstLock_NullDeleteFails(HttpRunner): | ||
| """ADR-0004 §"Conformance test suite" (g): null cannot delete a required lock. | ||
|
|
||
| RFC 7396 removes `indexed` from the merged traits object, but the effective | ||
| trait-schema also requires the property. The materialized object therefore | ||
| fails OP#13 instead of bypassing the const constraint. | ||
| """ | ||
|
|
||
| config = Config("OP#13 ADR-0004: const lock rejects null delete").base_url( | ||
| get_gts_base_url() | ||
| ) | ||
|
|
||
| def test_start(self): | ||
| super().test_start() | ||
|
|
||
| teststeps = [ | ||
| _register( | ||
| "gts://gts.x.test13.mconstdel.event.v1~", | ||
| { | ||
| "type": "object", | ||
| "x-gts-traits-schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "indexed": {"type": "boolean", "const": True}, | ||
| }, | ||
| "required": ["indexed"], | ||
| }, | ||
| "x-gts-traits": {"indexed": True}, | ||
| "required": ["id"], | ||
| "properties": {"id": {"type": "string"}}, | ||
| }, | ||
| "register base with const-and-required locked indexed=true", | ||
| ), | ||
| _register_derived( | ||
| "gts://gts.x.test13.mconstdel.event.v1~x.test13._.kid.v1~", | ||
| "gts://gts.x.test13.mconstdel.event.v1~", | ||
| { | ||
| "type": "object", | ||
| "x-gts-traits": {"indexed": None}, | ||
| }, | ||
| "register descendant trying to delete indexed", | ||
| ), | ||
| _validate_type_schema( | ||
| "gts.x.test13.mconstdel.event.v1~x.test13._.kid.v1~", | ||
| False, | ||
| "validate descendant - required prevents const-lock deletion", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Make the conformance test distinguish the merge contract.
This fixture covers only a top-level property. It does not catch deletion of a parent object containing a required const property. The False assertion also passes if an implementation incorrectly validates literal null instead of deleting it. Add a nested-parent case and assert the missing-required failure, or use another observable that distinguishes the two behaviors.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 3409-3441: Mutable default value for class attribute
(RUF012)
🤖 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/test_op13_schema_traits_validation.py` around lines 3394 - 3441, Extend
TestCaseOp13_Merge_ConstLock_NullDeleteFails with a nested parent object
containing a required const property, then attempt to delete that parent through
merge and assert the resulting validation failure specifically reflects the
missing required property. Ensure the assertion distinguishes RFC 7396
null-deletion semantics from incorrectly validating literal null.
1 participant
Summary by CodeRabbit
constvalues, required properties, defaults, and merge-based deletion.