[wip] bump gts-spec, implement op12 and op13#10
Merged
Artifizer merged 7 commits intoGlobalTypeSystem:mainfrom Feb 27, 2026
Merged
[wip] bump gts-spec, implement op12 and op13#10Artifizer merged 7 commits intoGlobalTypeSystem:mainfrom
Artifizer merged 7 commits intoGlobalTypeSystem:mainfrom
Conversation
Signed-off-by: mattgarmon <mjg2790@gmail.com>
Signed-off-by: mattgarmon <mjg2790@gmail.com>
Signed-off-by: mattgarmon <mjg2790@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:
📝 WalkthroughWalkthroughAdds UUID-tail support for GTS IDs; implements schema-vs-schema chain compatibility (OP#12) and schema traits validation (OP#13); exposes CLI subcommands and HTTP endpoints to run those validations; integrates x-gts-ref checks into the validation and instance/entity flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as HTTP Handler
participant Store as GtsStore
participant SchemaValidator as Schema Validator
participant TraitsValidator as Traits Validator
participant Output as JSON Output
Client->>HTTP: POST /validate-schema { schema_id }
HTTP->>Store: ValidateSchemaChain(schema_id)
Store->>SchemaValidator: resolve $ref/allOf, validate adjacent pairs
SchemaValidator-->>Store: chain result (ok/error)
alt chain ok
Store->>TraitsValidator: ValidateSchemaTraits(schema_id)
TraitsValidator-->>Store: traits result (ok/error)
Store-->>HTTP: combined OK result / errors
else chain failed
Store-->>HTTP: chain error result
end
HTTP->>Output: writeJSON(result)
Output-->>Client: JSON response
sequenceDiagram
participant Client
participant HTTP as HTTP Handler
participant Store as GtsStore
participant EntityValidator as Entity Validator
participant XGtsRef as x-gts-ref Checker
participant Output as JSON Output
Client->>HTTP: POST /validate-entity { entity_id | gts_id }
HTTP->>Store: ValidateEntity(id)
Store->>EntityValidator: resolve schema, collect traits, apply defaults
EntityValidator-->>Store: validation result (schema+traits)
alt schema/traits ok
Store->>XGtsRef: run x-gts-ref validation on instance
XGtsRef-->>Store: x-gts-ref errors / ok
end
Store-->>HTTP: final result { ok, entity_type, error }
HTTP->>Output: writeJSON(result)
Output-->>Client: JSON response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 6
🧹 Nitpick comments (1)
server/handlers.go (1)
467-533: Normalizegts://inputs for validation endpoints.
ValidateSchemaChain/ValidateEntityexpectgts.IDs, but callers may pass$idvalues that includegts://. Normalizing here reduces client friction and aligns with other handlers.🔧 Suggested fix
result := s.store.ValidateSchemaChain(req.SchemaID) + // Normalize gts:// prefix if present + schemaID := strings.TrimPrefix(req.SchemaID, gts.GtsURIPrefix) + result := s.store.ValidateSchemaChain(schemaID) @@ - id := req.EntityID + id := req.EntityID if id == "" { id = req.GtsID } + id = strings.TrimPrefix(id, gts.GtsURIPrefix)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 467 - 533, The handlers handleValidateSchema and handleValidateEntity must normalize incoming gts:// style IDs before calling store methods because ValidateSchemaChain, ValidateSchemaTraits and ValidateEntity expect gts.* IDs; update handleValidateSchema to normalize req.SchemaID (strip "gts://" prefix and any trailing slashes, optionally convert to the canonical "gts." form) before calling s.store.ValidateSchemaChain/ValidateSchemaTraits, and update handleValidateEntity to normalize whichever id is chosen from req.EntityID or req.GtsID the same way before calling s.store.ValidateEntity so both endpoints accept $id values containing the gts:// URI form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gts-spec:
- Line 1: The .gts-spec submodule reference uses a non-existent commit hash
(48f15e0e2fdb14f8cda10b02f1f13b4253173959); verify that hash exists in the
remote https://github.com/GlobalTypeSystem/gts-spec.git and, if not, replace it
with the correct commit. To fix: fetch or clone the remote to confirm the right
commit SHA, then update the submodule pointer (the gitlink recorded in
.gts-spec/.gitmodules or the submodule entry you added) to the valid SHA and
commit that change; finally run git submodule sync && git submodule update
--init --recursive to ensure the repo and CI will fetch the valid ref.
In `@gts/gts.go`:
- Around line 418-425: The current isUUIDSegment(s string) uses uuid.Parse and a
lowercase check which still accepts non-canonical forms (URN, raw hex, braced,
uppercase) — change it to parse into a uuid.UUID (e.g., u, err :=
uuid.Parse(s)), then verify the parsed value's canonical hyphenated lowercase
form matches the input by comparing s == strings.ToLower(u.String()), and
additionally confirm the UUID has an RFC4122 variant (using u.Variant() or
equivalent) before returning true; update isUUIDSegment to return false on parse
error, variant mismatch, or canonical-string mismatch.
In `@gts/schema_compat.go`:
- Around line 193-203: In checkTypeCompatibility, avoid direct comparison
baseType != derivedType (which can panic for slices) and also detect when the
derived property omits "type" (which loosens constraints): change the logic to
first return a compatibility error when hasBase is true and hasDerived is false,
and when both exist compare them using reflect.DeepEqual(baseType, derivedType)
(or otherwise normalize both values to comparable forms such as converting
single string vs slice to a canonical slice) instead of !=; update the error
message to indicate the type change when reflect.DeepEqual reports a difference.
Ensure you import reflect if needed and only operate on the map keys
baseProp["type"], derivedProp["type"] to avoid panics.
- Around line 304-318: The function checkRequiredRemoval currently returns early
when len(derived.required) == 0 which treats an explicitly empty required list
the same as an omitted one and lets derived schemas silently drop base required
fields; change the early-return to check for a nil map instead (i.e., if
derived.required == nil return nil) so only truly omitted `required` is ignored
and an explicitly empty required map will be validated and flagged for removing
base required fields; update the logic in checkRequiredRemoval to use
derived.required == nil and keep the existing loop over base.required / the
error messages using derivedID/baseID.
In `@gts/schema_traits.go`:
- Around line 161-175: The code currently overwrites non-object values with an
empty object when a schema expects an object; update the branch in the loop over
collectAllProperties(traitSchema, 0) so that when p.schema["type"] == "object"
and p.schema has "properties" you only call applyDefaults(p.schema, existing,
depth+1) if result[p.name] type-asserts to map[string]any (i.e., existing != nil
after assertion); do not create or replace with map[string]any{} when the
existing value is not a map—leave the original value untouched to avoid masking
invalid trait values.
- Around line 296-353: The code builds levelSchemaProps by iterating
traitSchemas but only reads direct "properties" keys, so properties introduced
via allOf are missed and descendant trait overrides may be wrongly treated as
locked; update the logic that populates levelSchemaProps (the loop over
traitSchemas[prevCount:]) to call collectAllProperties (and propagate discovered
defaults into knownDefaults similarly to the current branch) for each non-nil ts
instead of only inspecting ts["properties"], ensuring nested/allOf-defined
properties and their defaults are recorded for this level; keep the later use of
levelSchemaProps, collectTraitsFromValue, knownDefaults and lockedTraits
unchanged.
---
Nitpick comments:
In `@server/handlers.go`:
- Around line 467-533: The handlers handleValidateSchema and
handleValidateEntity must normalize incoming gts:// style IDs before calling
store methods because ValidateSchemaChain, ValidateSchemaTraits and
ValidateEntity expect gts.* IDs; update handleValidateSchema to normalize
req.SchemaID (strip "gts://" prefix and any trailing slashes, optionally convert
to the canonical "gts." form) before calling
s.store.ValidateSchemaChain/ValidateSchemaTraits, and update
handleValidateEntity to normalize whichever id is chosen from req.EntityID or
req.GtsID the same way before calling s.store.ValidateEntity so both endpoints
accept $id values containing the gts:// URI form.
Signed-off-by: mattgarmon <mjg2790@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/server.go (1)
90-95:⚠️ Potential issue | 🔴 CriticalUpgrade Go to 1.25.7 or later to patch GO-2026-4337 (crypto/tls vulnerability).
The project is currently vulnerable. The
go.modpins go1.25.2, which falls within the affected range (1.25.0–1.25.6). The vulnerability (CVE-2025-68121) involves TLS session resumption—a resumed connection may incorrectly succeed when authentication requirements have changed. Upgrade bothgo.modand.github/workflows/ci.ymlto go1.25.7+ (or go1.24.13+).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/server.go` around lines 90 - 95, The project currently pins Go to 1.25.2 which is vulnerable; update the Go version to at least 1.25.7 (or 1.24.13+) by modifying the go directive in go.mod and updating the Go version used in CI (the workflow file referenced in the review) so builds and tests run with go1.25.7+; ensure the module line in go.mod reflects "go 1.25.7" (or higher) and replace the Go tool/version entry in the CI workflow with the same version to ensure the patched runtime is used across local builds and CI.
♻️ Duplicate comments (1)
gts/schema_compat.go (1)
316-330:⚠️ Potential issue | 🟠 MajorDerived omission of
requiredstill bypasses base requirements.
If base has required fields and derived omitsrequiredentirely, the check returns nil. That allows loosening.🛠️ Suggested fix
func checkRequiredRemoval(base, derived *effectiveSchema, baseID, derivedID string) []string { - // Only skip if derived omits required entirely; an explicit empty list must be validated - if !derived.requiredSet { + if len(base.required) == 0 { return nil } + if !derived.requiredSet { + return []string{fmt.Sprintf( + "derived schema '%s' omits required fields defined in base '%s'", + derivedID, baseID, + )} + } var errors []string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/schema_compat.go` around lines 316 - 330, The function checkRequiredRemoval currently returns nil whenever derived.requiredSet is false, letting a derived schema that omits the required list bypass base requirements; change this so that in checkRequiredRemoval (working on effectiveSchema, inspecting base.required and derived.required/derived.requiredSet) you only skip when base.required is empty—otherwise, if base.required has entries and derived.requiredSet is false, treat those as removed and append errors for each base.required (same format using derivedID/baseID/field), otherwise proceed with the existing loop comparing base.required to derived.required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gts/validate_entity.go`:
- Around line 34-41: The runValidateEntity function calls cmd.Usage() when
validateEntityID == "" but then continues to create a store and call
store.ValidateEntity with an empty ID; update runValidateEntity to return
immediately after cmd.Usage() when validateEntityID is empty (i.e., guard the
rest of the function) so that none of the subsequent calls (newStore(),
store.ValidateEntity, writeJSON) run with an empty validateEntityID.
In `@cmd/gts/validate_schema.go`:
- Around line 31-38: runValidateSchema currently calls cmd.Usage() when
validateSchemaID == "" but continues and invokes store.ValidateSchemaChain with
an empty ID; modify runValidateSchema so that after calling cmd.Usage() it
immediately returns (e.g., add a return right after cmd.Usage()) to prevent
calling newStore().ValidateSchemaChain("") and writeJSON on an invalid result;
reference the runValidateSchema function and the validateSchemaID check to
locate the change.
In `@gts/schema_compat.go`:
- Around line 556-735: The cycle-detection logic in resolveRefsInner treats
repeated legitimate references as cycles because visited[canonical] is only
removed when strict is false; update the resolution branch in resolveRefsInner
(the block that sets visited[canonical] = true and calls
s.resolveRefsInner(entity.Content, ...)) to unmark the canonical entry after
recursive resolution regardless of strict (i.e., ensure visited[canonical] is
deleted when leaving that scope, using a defer-like/unmark-after-recursion
pattern) so repeated, non-circular refs are not flagged; keep existing behavior
for other parts (removing $id/$schema, merging, etc.) unchanged.
- Around line 295-314: In checkItemsCompatibility, the function currently skips
compatibility checks when base "items" is a schema object but derived "items" is
not an object (e.g., boolean or array), which loosens constraints; update
checkItemsCompatibility so that after determining baseItemsMap,derivedItemsMap,
if baseOk is true and derivedOk is false return a descriptive incompatibility
message (use itemsName and propName) indicating that derived replaced an object
schema with a non-object items value; keep the existing path that calls
comparePropertyConstraints when both are objects and preserve the existing nil
returns for other valid cases.
- Around line 83-144: validateSchemaCompatibility only iterates
derived.properties so constraints from base.properties that are removed or
re-enabled by derived go undetected; update validateSchemaCompatibility to also
iterate base.properties and for each baseProp check if it exists in
derived.properties and if not append an error (similar phrasing to existing
messages) indicating the derived omitted a base property, and if baseProp is a
bool false but derived provides a non-false (or object) or if baseProp is an
object and derived either omits it or replaces it with a non-object append an
error about loosening base constraints; reuse comparePropertyConstraints where
appropriate and mirror the existing message style and identifiers
(validateSchemaCompatibility, base.properties, derived.properties,
comparePropertyConstraints, baseID, derivedID) so removals and re-enablings are
correctly reported.
In `@server/handlers.go`:
- Around line 504-533: The error message in handleValidateEntity incorrectly
only mentions "entity_id" when neither field is provided; update the
missing-field check to return a clearer message that lists both accepted fields
(e.g., "Missing entity_id or gts_id") by changing the string passed to
s.writeError in the block that checks id == "" inside handleValidateEntity so
callers know both acceptable parameters.
---
Outside diff comments:
In `@server/server.go`:
- Around line 90-95: The project currently pins Go to 1.25.2 which is
vulnerable; update the Go version to at least 1.25.7 (or 1.24.13+) by modifying
the go directive in go.mod and updating the Go version used in CI (the workflow
file referenced in the review) so builds and tests run with go1.25.7+; ensure
the module line in go.mod reflects "go 1.25.7" (or higher) and replace the Go
tool/version entry in the CI workflow with the same version to ensure the
patched runtime is used across local builds and CI.
---
Duplicate comments:
In `@gts/schema_compat.go`:
- Around line 316-330: The function checkRequiredRemoval currently returns nil
whenever derived.requiredSet is false, letting a derived schema that omits the
required list bypass base requirements; change this so that in
checkRequiredRemoval (working on effectiveSchema, inspecting base.required and
derived.required/derived.requiredSet) you only skip when base.required is
empty—otherwise, if base.required has entries and derived.requiredSet is false,
treat those as removed and append errors for each base.required (same format
using derivedID/baseID/field), otherwise proceed with the existing loop
comparing base.required to derived.required.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gts-specREADME.mdcmd/gts/main.gocmd/gts/validate_entity.gocmd/gts/validate_schema.gogts/gts.gogts/schema_compat.gogts/schema_traits.goserver/handlers.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/gts/main.go
- .gts-spec
- gts/schema_traits.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gts/schema_compat.go (1)
659-663:⚠️ Potential issue | 🟠 MajorCycle detection false-positives remain unfixed for
strict=true— diamond/multi-ref schemas are incorrectly flagged as circular.When
strict=true(the only path called byresolveRefs),visited[canonical]is never deleted after recursing into an entity. Any schema referenced more than once in the dependency graph (e.g., a shared base in a diamond) will hitvisited[canonical] == trueon its second traversal and set*cycleFound = true, causingresolveRefsto return an error even though no cycle exists. Thestrict=falsebranch correctly removes the entry after recursion; the same cleanup must be applied unconditionally.🛠️ Proposed fix
visited[canonical] = true resolved := s.resolveRefsInner(entity.Content, visited, cycleFound, strict) - if !strict { - delete(visited, canonical) - } + delete(visited, canonical)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/schema_compat.go` around lines 659 - 663, The code marks visited[canonical] = true before recursing in resolveRefsInner and only deletes that entry when strict == false, which causes false-positive cycle detection when strict == true; change the logic in resolveRefsInner so that after calling s.resolveRefsInner(entity.Content, visited, cycleFound, strict) you unconditionally remove visited[canonical] (i.e., delete(visited, canonical)) regardless of the strict flag so shared nodes in diamond/multi-ref graphs are not treated as cycles; keep the initial visited[canonical] = true and preserve cycleFound handling.
🧹 Nitpick comments (1)
server/handlers.go (1)
482-501: Simplify the duplicatedresult.OKcheck.The two separate
if result.OKblocks are confusing: when the chain validation succeeds and traits also pass, execution falls through the first block without taking any branch, then re-evaluatesresult.OKin the second block. While functionally correct, the second check reads like dead code at a glance. A single structuredif/elseis clearer:♻️ Proposed refactor
result := s.store.ValidateSchemaChain(req.SchemaID) - if result.OK { - // Also run OP#13 traits validation - traitsResult := s.store.ValidateSchemaTraits(req.SchemaID) - if !traitsResult.OK { - s.writeJSON(w, http.StatusOK, map[string]any{ - "ok": false, - "error": traitsResult.Error, - }) - return - } - } - - if result.OK { - s.writeJSON(w, http.StatusOK, map[string]any{"ok": true}) - } else { - s.writeJSON(w, http.StatusOK, map[string]any{ - "ok": false, - "error": result.Error, - }) - } + if !result.OK { + s.writeJSON(w, http.StatusOK, map[string]any{ + "ok": false, + "error": result.Error, + }) + return + } + // Chain validation passed — also run OP#13 traits validation + traitsResult := s.store.ValidateSchemaTraits(req.SchemaID) + if !traitsResult.OK { + s.writeJSON(w, http.StatusOK, map[string]any{ + "ok": false, + "error": traitsResult.Error, + }) + return + } + s.writeJSON(w, http.StatusOK, map[string]any{"ok": true})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 482 - 501, The duplicated result.OK checks should be merged into a single if/else: in the handler, check if result.OK once; inside that branch call s.store.ValidateSchemaTraits(req.SchemaID) and if traitsResult.OK is false write the JSON error via s.writeJSON and return, otherwise write the success response with s.writeJSON({"ok": true}); in the else branch write the original result.Error JSON; adjust to use the existing symbols result.OK, traitsResult.OK, s.store.ValidateSchemaTraits, s.writeJSON and req.SchemaID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gts/schema_compat.go`:
- Around line 123-135: The loop currently only handles baseProp being a bool
false; extend it to also detect when baseProp is a schema object (e.g.,
map[string]any) that the derived schema has omitted or replaced: inside the same
iteration over base.properties, check if baseProp is not a bool (treat as a
schema-object) and if derived.properties does not contain propName or contains a
boolean (e.g., true) instead of the schema-object, append an error describing
that the derived schema omitted or dropped the base constraint for propName (use
the existing variables propName, baseProp, derivedProp, derived.properties and
base.properties to locate and implement this check).
---
Duplicate comments:
In `@gts/schema_compat.go`:
- Around line 659-663: The code marks visited[canonical] = true before recursing
in resolveRefsInner and only deletes that entry when strict == false, which
causes false-positive cycle detection when strict == true; change the logic in
resolveRefsInner so that after calling s.resolveRefsInner(entity.Content,
visited, cycleFound, strict) you unconditionally remove visited[canonical]
(i.e., delete(visited, canonical)) regardless of the strict flag so shared nodes
in diamond/multi-ref graphs are not treated as cycles; keep the initial
visited[canonical] = true and preserve cycleFound handling.
---
Nitpick comments:
In `@server/handlers.go`:
- Around line 482-501: The duplicated result.OK checks should be merged into a
single if/else: in the handler, check if result.OK once; inside that branch call
s.store.ValidateSchemaTraits(req.SchemaID) and if traitsResult.OK is false write
the JSON error via s.writeJSON and return, otherwise write the success response
with s.writeJSON({"ok": true}); in the else branch write the original
result.Error JSON; adjust to use the existing symbols result.OK,
traitsResult.OK, s.store.ValidateSchemaTraits, s.writeJSON and req.SchemaID.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/gts/validate_entity.gocmd/gts/validate_schema.gogts/schema_compat.goserver/handlers.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
gts/schema_compat.go (2)
125-137:⚠️ Potential issue | 🟠 MajorDisabled base property can be silently re-allowed via omission.
When
basePropisfalse, omission in derived is currently accepted. For open schemas, that drops an explicit deny constraint.🔧 Proposed fix
for propName, baseProp := range base.properties { derivedProp, exists := derived.properties[propName] if b, ok := baseProp.(bool); ok && !b { // base disabled the property; derived must not re-enable it - if exists { - if db, dOk := derivedProp.(bool); !dOk || db { - errors = append(errors, fmt.Sprintf( - "property '%s': derived schema '%s' re-enables property disabled in base '%s'", - propName, derivedID, baseID, - )) - } + if !exists { + if !baseDisallowsAdditional { + errors = append(errors, fmt.Sprintf( + "property '%s': derived schema '%s' omits property disabled in base '%s', loosening base constraints", + propName, derivedID, baseID, + )) + } + } else if db, dOk := derivedProp.(bool); !dOk || db { + errors = append(errors, fmt.Sprintf( + "property '%s': derived schema '%s' re-enables property disabled in base '%s'", + propName, derivedID, baseID, + )) } } else if _, baseIsObj := baseProp.(map[string]any); baseIsObj && !nested {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/schema_compat.go` around lines 125 - 137, In the loop over base.properties (iterating propName, baseProp) treat a base boolean false as a DENY that must be explicitly preserved: if baseProp is a bool and false, then absence in derived (exists == false) or derivedProp being a non-bool or true should both be considered re-enabling and append the same error; update the conditional around derived.properties lookup (the block using derivedProp, exists, db, dOk and the error append) so it also triggers when !exists, referencing baseProp, derivedProp, exists, db/dOk, derivedID and baseID to construct the error message.
374-378:⚠️ Potential issue | 🟠 MajorOmitting
requiredstill bypasses required-field removal detection.If base has required fields and derived omits
required, the function currently skips validation and may miss a loosening.🔧 Proposed fix
func checkRequiredRemoval(base, derived *effectiveSchema, baseID, derivedID string) []string { - // Only skip if derived omits required entirely; an explicit empty list must be validated - if !derived.requiredSet { + if len(base.required) == 0 { return nil } + if !derived.requiredSet { + return []string{fmt.Sprintf( + "derived schema '%s' omits required fields defined in base '%s'", + derivedID, baseID, + )} + } var errors []string for baseReq := range base.required { if !derived.required[baseReq] { errors = append(errors, fmt.Sprintf( "derived schema '%s' removes required field '%s' defined in base '%s'",Also applies to: 380-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/schema_compat.go` around lines 374 - 378, The early-return in checkRequiredRemoval incorrectly skips validation when derived.requiredSet is false (derived omitted `required`), allowing required-field removals to go undetected; change the logic so omission is treated as an empty list and compare base.required vs derived.required to detect loosening: remove or invert the `if !derived.requiredSet { return nil }` branch, and instead use base.required/base.requiredSet and derived.required/derived.requiredSet to compute removed fields (treat derived missing `required` as empty) and return those names; reference checkRequiredRemoval, effectiveSchema, base.required, derived.required, baseID, derivedID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gts/schema_compat.go`:
- Around line 724-733: The current logic in the allOf processing block
(variables allOfRefs, itemMap, refVal, canonical, and cycleFound) marks repeated
sibling $ref entries as circular; change it so duplicate canonical refs within
the same allOf are ignored rather than setting *cycleFound = true — i.e., if
canonical already exists in allOfRefs, skip/continue without mutating
cycleFound; only set *cycleFound when a true ancestor/parent-level circular
reference is detected (not for sibling duplicates).
- Around line 20-24: The validation for JSON Schema minLength/maxLength uses
Go's len(string) which counts bytes; change it to count Unicode code points by
importing "unicode/utf8" and replacing len(s) with utf8.RuneCountInString(s)
inside numericValueFor (the branch that handles schema keywords "minLength" and
"maxLength") so length checks use rune counts rather than UTF-8 byte length.
---
Duplicate comments:
In `@gts/schema_compat.go`:
- Around line 125-137: In the loop over base.properties (iterating propName,
baseProp) treat a base boolean false as a DENY that must be explicitly
preserved: if baseProp is a bool and false, then absence in derived (exists ==
false) or derivedProp being a non-bool or true should both be considered
re-enabling and append the same error; update the conditional around
derived.properties lookup (the block using derivedProp, exists, db, dOk and the
error append) so it also triggers when !exists, referencing baseProp,
derivedProp, exists, db/dOk, derivedID and baseID to construct the error
message.
- Around line 374-378: The early-return in checkRequiredRemoval incorrectly
skips validation when derived.requiredSet is false (derived omitted `required`),
allowing required-field removals to go undetected; change the logic so omission
is treated as an empty list and compare base.required vs derived.required to
detect loosening: remove or invert the `if !derived.requiredSet { return nil }`
branch, and instead use base.required/base.requiredSet and
derived.required/derived.requiredSet to compute removed fields (treat derived
missing `required` as empty) and return those names; reference
checkRequiredRemoval, effectiveSchema, base.required, derived.required, baseID,
derivedID.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/gts/validate_entity.gocmd/gts/validate_schema.gogts/schema_compat.goserver/handlers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/handlers.go
- cmd/gts/validate_entity.go
Signed-off-by: mattgarmon <mjg2790@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
gts/schema_compat.go (1)
731-744:⚠️ Potential issue | 🟠 MajorDuplicate sibling
$refvalues are being treated as circular refs.At Line 739-Line 741, repeated refs inside the same
allOfsetcycleFound=true. That is not an ancestor cycle and can fail valid schemas.🔧 Suggested fix
- // Pre-scan: flag duplicate $ref targets within this allOf as a cycle. - // Sibling duplicates are degenerate schemas and must not be resolved - // twice (which would misfire the ancestor visited-map check). - allOfRefs := make(map[string]bool) - for _, item := range allOf { - if itemMap, ok := item.(map[string]any); ok { - if refVal, ok := itemMap["$ref"].(string); ok { - canonical := strings.TrimPrefix(refVal, GtsURIPrefix) - if allOfRefs[canonical] { - *cycleFound = true - } - allOfRefs[canonical] = true - } - } - } + // Note: duplicate sibling $ref targets are not circular by themselves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/schema_compat.go` around lines 731 - 744, The code incorrectly treats duplicate sibling $ref values in the same allOf as a circular reference by setting *cycleFound = true; instead, detect and ignore sibling duplicates rather than marking them as cycles: in the loop that builds allOfRefs (use symbols allOfRefs, canonical, GtsURIPrefix and cycleFound to locate the code), if canonical is already present (allOfRefs[canonical]) simply continue/skip further processing for that item instead of setting *cycleFound = true, leaving cycle detection to the existing ancestor/visited-map logic elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gts/schema_compat_test.go`:
- Around line 847-868: The test TestValidateSchemaChain_CircularRef currently
ignores the returned result from store.ValidateSchemaChain; update the test to
assert the expected behavior by checking the returned result (from
ValidateSchemaChain) is not OK and contains a circular-ref indication: call
ValidateSchemaChain("gts.x.cyclic.ns.self.v1~x.cyclic.ns.derived.v1~"), then
assert result.Ok == false (or use t.Fatalf/t.Errorf if true) and assert
result.Error (or the returned error/message field) contains "circular" or
"circular $ref" to ensure regression detection while still avoiding
panics/hangs; keep mustRegister and NewGtsStore usage as-is.
In `@gts/schema_compat.go`:
- Around line 753-793: The allOf flattening currently copies only "properties"
and "required" from each resolvedMap into mergedProps/mergedRequired and drops
other constraints; update the merge logic (around mergedProps, mergedRequired,
resolvedAllOf handling) to preserve and merge all remaining constraint keys from
each resolvedMap into the final merged map: for each resolvedMap, deep-merge
keys like "additionalProperties", "type", numeric/string bounds, format, enums,
etc. into the merged result (not just properties/required), resolving conflicts
deterministically (e.g., prefer more restrictive constraints or combine
arrays/objects) and ensure merged["allOf"] still contains any unresolved items
via resolvedAllOf; keep existing behavior for properties/required while adding a
generic merge for all other keys to avoid silently dropping constraints.
- Around line 129-137: When baseProp is the boolean false and the derived schema
omits the property (exists == false) we currently always treat that as
re-enabling; instead check the derived schema's additionalProperties setting
before flagging an error: retrieve the derived schema's additionalProperties
(e.g., via derivedSchema["additionalProperties"] or helper that resolves
additional properties for the schema) and only append the re-enable error for
propName/derivedID/baseID when additionalProperties in the derived schema allows
unknown properties (i.e., is absent or true); if additionalProperties is
explicitly false then omission is valid and do not add the error.
---
Duplicate comments:
In `@gts/schema_compat.go`:
- Around line 731-744: The code incorrectly treats duplicate sibling $ref values
in the same allOf as a circular reference by setting *cycleFound = true;
instead, detect and ignore sibling duplicates rather than marking them as
cycles: in the loop that builds allOfRefs (use symbols allOfRefs, canonical,
GtsURIPrefix and cycleFound to locate the code), if canonical is already present
(allOfRefs[canonical]) simply continue/skip further processing for that item
instead of setting *cycleFound = true, leaving cycle detection to the existing
ancestor/visited-map logic elsewhere.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/gts/validate_entity.gocmd/gts/validate_schema.gogts/schema_compat.gogts/schema_compat_test.gogts/schema_traits_test.goserver/handlers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/gts/validate_entity.go
Signed-off-by: mattgarmon <mjg2790@gmail.com>
mattgarmon
changed the title
mattgarmon
changed the title
Signed-off-by: mattgarmon <mjg2790@gmail.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Artifizer
approved these changes
Feb 27, 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
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.
Summary by CodeRabbit
New Features
Documentation
Tests