feat(gts): port to gts-spec v0.12.0 & switch to GHCR test runner#12
Merged
Artifizer merged 5 commits intoMay 29, 2026
Merged
Conversation
Align Go field/JSON-tag/route vocabulary with gts-spec v0.11 terminology: REST surface - POST /schemas -> POST /type-schemas - POST /validate-schema -> POST /validate-type-schema - /compatibility query params old_schema_id, new_schema_id -> old_type_id, new_type_id - /cast request body to_schema_id -> to_type_id - /validate-type-schema request body schema_id -> type_id - POST /type-schemas request body schema -> type_schema (the response already used type_id from a prior pass) Result types - Inside /resolve-relationships, SchemaGraphNode JSON key schema_id -> type_id - ValidateSchemaChainResult.schema_id, ValidateSchemaTraitsResult.schema_id -> type_id - EntityInfo.schema_id, EntityInfo.is_schema -> type_id, is_type_schema - ExtractIDResult.schema_id, .selected_schema_id_field, .is_schema -> type_id, selected_type_id_field, is_type_schema - IDValidationResult.is_schema and ParseIDResult.is_schema -> is_type (per spec wording: this boolean answers "is this id a type identifier", while entity-extraction results carry the heavier is_type_schema flag) Internal Go identifiers (kept in sync with JSON tags per maintainer choice) - JsonEntity.SchemaID / SelectedSchemaIDField / IsSchema -> TypeID / SelectedTypeIDField / IsTypeSchema - GtsConfig.SchemaIDFields -> TypeIDFields (config JSON tag schema_id_fields -> type_id_fields) - calcJSONSchemaID -> calcJSONTypeID - Cast/compatibility parameters renamed: oldSchemaID/newSchemaID/ toSchemaID/fromSchemaID -> oldTypeID/newTypeID/toTypeID/fromTypeID - Mirror the rename across the matching CLI flag help text in cmd/gts/. ValidateSchema error string "is not a schema (must end with '~')" updated to "is not a type-schema ID (must end with '~')" for consistency with the renamed entity check; tests adjusted. Hard switch: no aliases or deprecated forwards. Matches gts-rust 4a2c43f + 0aa2104 + 32d38f3. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
… only Per gts-spec v0.11, the type_id of an entity MUST be a GTS Type Identifier (a chained id ending in '~') or null. The previous gts-go behavior carried any non-empty $schema value through to type_id, including JSON Schema dialect URLs like "http://json-schema.org/draft-07/schema#". In calcJSONTypeID, only retain the $schema value as the derived type_id when it both ends in '~' and parses via IsValidGtsID; otherwise drop the value but still record SelectedTypeIDField="$schema" so callers see we did look at it. Existing test TestExtractID_SchemaIDFallback updated to assert the dialect URL is no longer carried through, plus two new positive/negative cases for the tightened check. Mirrors gts-rust 2e1d92c. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
Per gts-spec §9.7.1 / §9.11.1, the x-gts-traits and x-gts-traits-schema annotations are valid only on type-schema documents and implementations MUST reject instances that contain them. gts-go previously only rejected x-gts-final and x-gts-abstract, and only at the top level. - Promote x-gts-traits[-schema] to public constants KeyXGtsTraits / KeyXGtsTraitsSchema in schema_traits.go (with §9.7.1 reference in the doc comment) and replace the string literals in this file. - Extend ValidateInstanceModifiers to a recursive scan over both objects and arrays for any of the four schema-only keywords (x-gts-final, x-gts-abstract, x-gts-traits-schema, x-gts-traits). This also tightens the pre-existing top-level-only check for x-gts-final / x-gts-abstract to match the Rust impl's contains_key_recursive behavior. - Three new unit tests: top-level x-gts-traits, top-level x-gts-traits-schema, and nested x-gts-traits under a metadata object. Mirrors gts-rust 4bfd06d. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- Pin the gts-spec version in .gts-spec-version (v0.11.0). The tag is used verbatim as the GHCR image tag; default format vMAJOR.MINOR.PATCH so every commit is reproducible, override to vMAJOR.MINOR for the rolling minor tag. - Replace `make e2e` and the Python venv it required with `make gts-spec-tests` — one-shot target that builds the gts binary, pulls ghcr.io/globaltypesystem/gts-spec-tests:\$(cat .gts-spec-version), starts the server bound to 0.0.0.0:\$PORT (so the test-runner container can reach it via host.docker.internal on Linux, where Docker doesn't route loopback transparently the way Docker Desktop does on Mac/ Windows), and runs pytest inside the container. TEST=, PORT=, GTS_SPEC_TESTS_DIR= cover single-test, port, and local-tests-mount overrides. - Add `make gts-server` (foreground build + run) and `make gts-spec-tests-run` (docker run against an externally-running server) for the iterative test-edit loop, sharing the docker invocation via a RUN_TESTS_DOCKER macro. - Group all gts-spec-related variables, macros, and targets into a dedicated section at the bottom of the Makefile. - Remove the .gts-spec git submodule (and .gitmodules), drop update-spec/e2e-venv targets and PYTHON/VENV_DIR/VENV_PY vars, and strip submodules: recursive from every CI checkout step. - Rename the CI job e2e -> gts-spec-tests; drop the Python setup steps and inline server wait loop (the Makefile does both now). - Rewrite the README and CLAUDE.md gts-spec-tests sections for the docker flow (one-shot + iterative) and drop the macOS open-files workaround — no Python harness means no leaked sockets means no FD limit problem. Point CLI-example users at a local clone of gts-spec. Mirrors gts-rust a57c57d. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- Bump conformance target from v0.11.0 to v0.12.0. - Treat x-gts-traits-schema as an ordinary JSON Schema subschema (ADR-0002): accept object/`true`/`false` forms; compose the chain via allOf; a `false` anywhere makes the effective schema unsatisfiable (traits prohibited). - Merge x-gts-traits values along the chain via RFC 7396 JSON Merge Patch (ADR-0004, last-wins); drop GTS-specific lock/duplicate-default rules in favor of standard JSON Schema `const`. - Key trait completeness on the effective schema's `required` set (ADR-0003); abstract types skip the completeness check. - additionalProperties only loosens when derived *explicitly* declares a permissive value at its root; omitting it inherits the closed base via $ref/allOf composition. Drop duplicate-sibling-$ref rejection (redundant manual aggregation is allowed) and resolve nested $refs in allOf siblings. - Enforce x-gts-traits / x-gts-traits-schema placement at the schema top level only, wired into add-entity and validate-entity (gts-spec §9.7.1/§9.11). Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
📝 WalkthroughWalkthroughThis PR refactors the codebase from "schema" to "type-schema" semantics across data models, validation logic, and public APIs. It also migrates the test infrastructure from git-submodule-based pytest workflows to Docker-based conformance test runners orchestrated through make targets, removing the ChangesType-Schema Terminology Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
aviator5
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gts/store.go (1)
176-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent panic on empty
type_idinRegisterSchema
RegisterSchemadoestypeID[len(typeID)-1], which panics whentypeID == ""; the HTTP endpointPOST /type-schemasforwardsreq.TypeID(fromjson:"type_id") without non-empty validation.ValidateSchemaalready usesstrings.HasSuffix, soRegisterSchemashould match.🛡️ Proposed fix
- if typeID[len(typeID)-1] != '~' { + if !strings.HasSuffix(typeID, "~") { return fmt.Errorf("schema type_id must end with '~'") }🤖 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/store.go` around lines 176 - 179, RegisterSchema currently accesses typeID[len(typeID)-1] which panics on an empty string; change the check to use strings.HasSuffix(typeID, "~") or first verify len(typeID) > 0 and then check the last char so it safely returns an error instead of panicking. Update the error path in RegisterSchema (referencing the RegisterSchema method) to match ValidateSchema's behavior and error message semantics so empty or non-suffixed typeIDs produce a controlled error rather than a runtime panic.
🧹 Nitpick comments (1)
gts/schema_modifiers.go (1)
110-117: 💤 Low valueRecursive instance validation thoroughness vs performance.
The recursive check for schema-only keywords in instances is comprehensive but will traverse the entire instance tree. For deeply nested or large instances, this could add measurable overhead.
Consider benchmarking typical instance sizes if this becomes a hot path.
🤖 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/schema_modifiers.go` around lines 110 - 117, The recursive check in ValidateInstanceModifiers calling containsKeyRecursive can be very costly for deeply nested instances; modify containsKeyRecursive (and its callers) to support an explicit maxDepth (or convert it to an iterative stack-based traversal) and have ValidateInstanceModifiers pass a reasonable defaultDepth constant to short-circuit traversal beyond that depth, returning no-match rather than exploring arbitrarily deep trees; update the function signatures (containsKeyRecursive) and add/document a DEFAULT_MAX_INSTANCE_CHECK_DEPTH used by ValidateInstanceModifiers to avoid unbounded recursion and improve performance on large instances while preserving correctness for typical cases.
🤖 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 `@cmd/gts/helpers.go`:
- Line 73: Document the breaking config change (rename of schema_id_fields →
type_id_fields) in the PR description and migration notes so users know to
update their JSON config, and add backward-compatible parsing: update the struct
that defines TypeIDFields to accept the old key as well (either add an auxiliary
SchemaIDFields string slice and in a custom UnmarshalJSON or in post-unmarshal
logic copy SchemaIDFields into TypeIDFields when present, or implement
UnmarshalJSON to read both "type_id_fields" and "schema_id_fields"); mention and
update any references to TypeIDFields and ensure both keys are handled to avoid
breaking existing configs while you announce the change.
In `@gts/extract.go`:
- Around line 80-83: Remove the redundant recomputation: delete the conditional
block that checks entity.TypeID == "" and calls entity.calcJSONTypeID(cfg,
entityIDValue) (the later block around the well-known instance handling), since
TypeID was already derived earlier via entity.TypeID =
entity.calcJSONTypeID(cfg, entityIDValue) and calcJSONTypeID depends only on
cfg, entityIDValue, and entity.SelectedEntityField (set by calcJSONEntityID). If
you prefer to keep a comment, replace the block with a single clarifying comment
stating that TypeID has already been derived by calcJSONTypeID and will not
change for instances; do not re-invoke calcJSONTypeID or reference entity.GtsID
in this path.
---
Outside diff comments:
In `@gts/store.go`:
- Around line 176-179: RegisterSchema currently accesses typeID[len(typeID)-1]
which panics on an empty string; change the check to use
strings.HasSuffix(typeID, "~") or first verify len(typeID) > 0 and then check
the last char so it safely returns an error instead of panicking. Update the
error path in RegisterSchema (referencing the RegisterSchema method) to match
ValidateSchema's behavior and error message semantics so empty or non-suffixed
typeIDs produce a controlled error rather than a runtime panic.
---
Nitpick comments:
In `@gts/schema_modifiers.go`:
- Around line 110-117: The recursive check in ValidateInstanceModifiers calling
containsKeyRecursive can be very costly for deeply nested instances; modify
containsKeyRecursive (and its callers) to support an explicit maxDepth (or
convert it to an iterative stack-based traversal) and have
ValidateInstanceModifiers pass a reasonable defaultDepth constant to
short-circuit traversal beyond that depth, returning no-match rather than
exploring arbitrarily deep trees; update the function signatures
(containsKeyRecursive) and add/document a DEFAULT_MAX_INSTANCE_CHECK_DEPTH used
by ValidateInstanceModifiers to avoid unbounded recursion and improve
performance on large instances while preserving correctness for typical cases.
🪄 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: dff84dbd-b1df-42b4-bd05-19510277f19d
📒 Files selected for processing (30)
.github/workflows/ci.yml.gitmodules.gts-spec.gts-spec-versionCLAUDE.mdMakefileREADME.mdcmd/gts/cast.gocmd/gts/compatibility.gocmd/gts/helpers.gogts/cast.gogts/compatibility.gogts/config.gogts/extract.gogts/extract_test.gogts/ops.gogts/parse.gogts/parse_test.gogts/registry_test.gogts/relationships.gogts/schema_compat.gogts/schema_compat_test.gogts/schema_modifiers.gogts/schema_modifiers_test.gogts/schema_traits.gogts/schema_traits_test.gogts/store.gogts/validate.goserver/handlers.goserver/server.go
💤 Files with no reviewable changes (2)
- .gitmodules
- .gts-spec
| var data struct { | ||
| EntityIDFields []string `json:"entity_id_fields"` | ||
| SchemaIDFields []string `json:"schema_id_fields"` | ||
| TypeIDFields []string `json:"type_id_fields"` |
There was a problem hiding this comment.
Document the breaking config file format change.
The JSON config field rename from schema_id_fields to type_id_fields is a breaking change for existing users. Consider documenting this in migration notes or the PR description so users know to update their config files.
Also applies to: 83-83
🤖 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 `@cmd/gts/helpers.go` at line 73, Document the breaking config change (rename
of schema_id_fields → type_id_fields) in the PR description and migration notes
so users know to update their JSON config, and add backward-compatible parsing:
update the struct that defines TypeIDFields to accept the old key as well
(either add an auxiliary SchemaIDFields string slice and in a custom
UnmarshalJSON or in post-unmarshal logic copy SchemaIDFields into TypeIDFields
when present, or implement UnmarshalJSON to read both "type_id_fields" and
"schema_id_fields"); mention and update any references to TypeIDFields and
ensure both keys are handled to avoid breaking existing configs while you
announce the change.
Comment on lines
+80
to
83
| // Type ID should be derived from the chain if not explicitly set | ||
| if entity.TypeID == "" && entity.SelectedEntityField != "" { | ||
| entity.TypeID = entity.calcJSONTypeID(cfg, entityIDValue) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep --pattern $'func (e *JsonEntity) calcJSONTypeID($$$) string {
$$$
}'Repository: GlobalTypeSystem/gts-go
Length of output: 2890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- gts/extract.go (lines 40-110) ----"
nl -ba gts/extract.go | sed -n '40,110p'
echo
echo "---- search for calcJSONTypeID call sites ----"
rg -n "calcJSONTypeID\\(" gts/extract.go
echo
echo "---- inspect JsonEntity struct fields (to see what calcJSONTypeID may read/write) ----"
rg -n "type JsonEntity" gts/extract.goRepository: GlobalTypeSystem/gts-go
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- gts/extract.go excerpt (around reported block) ----"
awk 'NR>=45 && NR<=110 {printf "%6d\t%s\n", NR, $0}' gts/extract.go | sed -n '1,200p'
echo
echo "---- calcJSONTypeID call sites in gts/extract.go ----"
rg -n "calcJSONTypeID\\(" gts/extract.go
echo
echo "---- JsonEntity definition (fields potentially touched by calcJSONTypeID) ----"
# Print enough context around the struct definition
line=$(rg -n "type JsonEntity" gts/extract.go | head -n 1 | cut -d: -f1)
start=$((line-10))
end=$((line+120))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' gts/extract.go | sed -n '1,260p'Repository: GlobalTypeSystem/gts-go
Length of output: 8204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- calcJSONEntityID implementation ----"
ast-grep --pattern $'func (e *JsonEntity) calcJSONEntityID($$$) string { $$$ }' gts/extract.goRepository: GlobalTypeSystem/gts-go
Length of output: 372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- firstNonEmptyField implementation ----"
ast-grep --pattern $'func (e *JsonEntity) firstNonEmptyField($$$) { $$$ }' gts/extract.go
echo
echo "---- getFieldValue implementation (if present in extract.go) ----"
rg -n "func \\(e \\*JsonEntity\\) getFieldValue\\(" gts/extract.go || true
rg -n "getFieldValue\\(" gts/extract.go
echo
# Print nearby code around getFieldValue if we found it
line=$(rg -n "func \\(e \\*JsonEntity\\) getFieldValue\\(" gts/extract.go | head -n 1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-20))
end=$((line+80))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' gts/extract.go
fiRepository: GlobalTypeSystem/gts-go
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- locate firstNonEmptyField ----"
rg -n "firstNonEmptyField" gts/extract.go
echo
echo "---- locate getFieldValue (impl + call sites) ----"
rg -n "getFieldValue" gts/extract.go
echo
echo "---- show likely helper implementations area (around matches) ----"
for pat in "firstNonEmptyField" "getFieldValue"; do
line=$(rg -n "func \\(e \\*JsonEntity\\) ${pat}\\b" gts/extract.go | head -n 1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-30)); end=$((line+120))
echo
echo "### ${pat} around line ${line}"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' gts/extract.go
fi
doneRepository: GlobalTypeSystem/gts-go
Length of output: 11662
Remove redundant calcJSONTypeID recomputation in well-known instance block
entity.TypeID is already set via entity.TypeID = entity.calcJSONTypeID(cfg, entityIDValue) (line 65). The later block (lines 80-83) reruns calcJSONTypeID(cfg, entityIDValue) only when entity.TypeID == "", but calcJSONTypeID for instances depends only on cfg, entityIDValue, and e.SelectedEntityField (which was set once by calcJSONEntityID and isn’t modified between lines 65 and 80-83); it does not use entity.GtsID. So this second call cannot change entity.TypeID (it will recompute "" again). Remove the block or adjust the comment/logic to match the intended derivation behavior.
🤖 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/extract.go` around lines 80 - 83, Remove the redundant recomputation:
delete the conditional block that checks entity.TypeID == "" and calls
entity.calcJSONTypeID(cfg, entityIDValue) (the later block around the well-known
instance handling), since TypeID was already derived earlier via entity.TypeID =
entity.calcJSONTypeID(cfg, entityIDValue) and calcJSONTypeID depends only on
cfg, entityIDValue, and entity.SelectedEntityField (set by calcJSONEntityID). If
you prefer to keep a comment, replace the block with a single clarifying comment
stating that TypeID has already been derived by calcJSONTypeID and will not
change for instances; do not re-invoke calcJSONTypeID or reference entity.GtsID
in this path.
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
3 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.
Brings the Go implementation up to gts-spec v0.12.0 and switches the conformance suite to the published GHCR runner image.
Spec alignment
effective required set (ADR-0003); additionalProperties only loosens on an explicit permissive value at the derived root; enforce top-level-only placement.
Summary by CodeRabbit
New Features
gts-spec-testsDocker-based conformance test runner, replacing Python pytest flow.Bug Fixes
additionalPropertiesinheritance in schema chains to correctly handle omitted vs explicit values.Documentation
make gts-spec-testsworkflow.Chores
SchemaID→TypeID,IsSchema→IsTypeSchema./schemas→/type-schemas,/validate-schema→/validate-type-schema.