feat: support x-gts-final/abstract modifiers and anonymous instances#11
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds schema-only modifiers ( Changes
Sequence DiagramssequenceDiagram
participant Client
participant Handler as HTTP Handler
participant Store as GTS Store
participant Validator as Validator
Client->>Handler: POST /add-entity (schema with modifiers)
Handler->>Handler: Extract entity, derive EffectiveID()
Handler->>Handler: ValidateSchemaModifiers
alt Modifier validation fails
Handler-->>Client: 422 {ok:false, error:...}
else Modifier validation passes
Handler->>Store: Register(entity by EffectiveID)
Handler->>Validator: ValidateSchemaChain (checks x-gts-final)
Validator->>Store: Get(baseID)
Handler->>Validator: ValidateSchemaTraits
alt Validation fails
Handler->>Store: Unregister(previous or current)
Handler-->>Client: 422 {ok:false, error:...}
else Validation passes
Handler-->>Client: 200 {gts_id: <effective id>}
end
end
sequenceDiagram
participant Client
participant Handler as HTTP Handler
participant Store as GTS Store
participant Validator as Validator
Client->>Handler: POST /add-entity (instance)
Handler->>Handler: Extract entity, derive EffectiveID()
Handler->>Handler: ValidateInstanceModifiers
alt Instance contains schema-only keywords
Handler-->>Client: 422 {ok:false, error:...}
else Instance is clean
Handler->>Store: Register(instance by EffectiveID)
Handler->>Validator: ValidateInstance(instanceID)
Validator->>Store: Resolve schema, check x-gts-abstract
alt Schema is abstract
Handler->>Store: Unregister(previous or current)
Handler-->>Client: 422 {ok:false, error:"abstract cannot be instantiated"}
else Validation passes
Handler-->>Client: 200 {gts_id: <effective id>}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/handlers.go (1)
44-47:⚠️ Potential issue | 🔴 CriticalAvoid panicking when reading anonymous instances.
Line 137 can return a raw anonymous ID, but
handleGetEntity()still dereferencesentity.GtsID.ID. A successful anonymous add followed by GET can panic.Proposed fix
+ responseID := entity.EffectiveID() + if responseID == "" { + responseID = id + } s.writeJSON(w, http.StatusOK, map[string]any{ - "id": entity.GtsID.ID, + "id": responseID, "content": entity.Content, })Also applies to: 134-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 44 - 47, handleGetEntity dereferences entity.GtsID.ID which can be nil or represent a raw anonymous ID; update handleGetEntity to safely handle anonymous instances by checking entity.GtsID and its ID before dereferencing and producing the JSON: if GtsID is nil or its ID is empty/anonymous return a safe representation (e.g., omit "id" or return null/anonymous marker) instead of accessing entity.GtsID.ID; adjust any related code paths that build the response (the block that calls s.writeJSON) to use a helper or conditional based on entity.GtsID to avoid panics when reading anonymous instances.
🤖 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: Update and verify e2e tests against the updated .gts-spec (submodule
at commit 23ed727f v0.9.0) by pulling the submodule changes, running the test
target, and reporting results: run the submodule update/fetch to ensure
.gts-spec matches commit 23ed727f, then execute the test command `make e2e` in
the repository root, address any failures (pay attention to new conformance
checks for x-gts-final/abstract modifiers and anonymous instances), and confirm
all tests pass or include failing test names and logs in your PR comment.
In `@gts/schema_modifiers.go`:
- Around line 51-68: ValidateModifierPlacement only checks direct children of
the top-level allOf; update it (or add a helper) to recursively traverse nested
allOf arrays and inspect every nested map[string]any for KeyXGtsFinal and
KeyXGtsAbstract so modifiers anywhere inside any depth of allOf are rejected;
implement a DFS/BFS helper (e.g., validateAllOfRecursive) that accepts an
interface{}, unwraps slices and maps, enqueues nested "allOf" entries, and
returns an error with the same messages when it finds KeyXGtsFinal or
KeyXGtsAbstract, then call this helper from ValidateModifierPlacement in place
of the current single-level loop.
In `@gts/schema_traits.go`:
- Around line 651-660: The ValidateEntity path for schemas currently only calls
ExtractSchemaModifiers which checks values and mutual exclusion but not
placement, allowing x-gts-final/x-gts-abstract to be hidden inside allOf; update
ValidateEntity (the branch where entity.IsSchema is true) to perform placement
validation on entity.Content — either extend ExtractSchemaModifiers to return a
placement error or call a new function (e.g., ValidateSchemaModifierPlacement)
that scans the schema node tree and rejects modifier keys found under
allOf/other nested combinators, then return that error in the same
ValidateEntityResult format so modifiers inside allOf are rejected server-side.
In `@gts/validate.go`:
- Around line 69-118: When validating an instance in ValidateInstance (the block
that fetches obj := s.Get(lookupID) and later calls s.validateWithSchema),
explicitly reject schema-only modifiers appearing in the instance content: check
obj.Content for KeyXGtsFinal and KeyXGtsAbstract (and any other schema-only
keys), and if present return a ValidationResult with ID: instanceID, OK: false
and a clear error message (e.g., "instance contains schema-only modifier
'x-gts-final'") before proceeding to schema validation; ensure you reference
obj.Content, instanceID, KeyXGtsFinal, and KeyXGtsAbstract so the check runs
prior to s.validateWithSchema.
In `@Makefile`:
- Around line 90-96: The e2e target currently backgrounds ./bin/gts server and
blindly sleeps 2s before running pytest, which can mask a server that failed to
bind PORT; change the flow in the e2e recipe (target "e2e") to start the server
backgrounded, write its PID to .server.pid (as already done for ./bin/gts
server), then actively wait for the server to be listening on $(PORT) or for the
server process to exit: poll the port (or try connecting) with a timeout and if
the process exits or the port is not accepting within the timeout, kill the
server, remove .server.pid and exit 1; only proceed to run the pytest invocation
once the port is confirmed listening. Ensure you reference and use the existing
symbols ./bin/gts server, .server.pid, PORT and the pytest invocation so the
change replaces the sleep logic with the wait-and-fail-fast checks.
- Around line 73-84: The e2e-venv target currently only depends on $(VENV_PY) so
Make skips the pip install steps when the venv binary exists; update the
Makefile so the pip install/upgrade lines run unconditionally by moving the
install commands (the three @$(VENV_PY) -m pip ...) from the $(VENV_PY) recipe
into the e2e-venv recipe (keep $(VENV_PY) as a prerequisite so venv creation
still happens if missing, and keep `@touch` $(VENV_PY) to preserve the sentinel).
This ensures e2e-venv always verifies/installs httprunner and other test deps
even when the venv file exists.
In `@server/handlers.go`:
- Around line 235-273: The handler registers the entity via s.store.Register
before running full validation (ValidateSchemaChain, ValidateSchemaTraits,
ValidateInstance), but on validation failure it returns 422 without removing the
already-registered entity; modify the flow so that after a failed validation you
roll back the registration by calling the store delete/unregister method (e.g.,
s.store.Delete(responseID) or s.store.Unregister(responseID)) before writing the
JSON error response, and handle/log any error from that rollback; apply this
rollback step for both the schema branch (after chainResult/traitsResult
failures) and the instance branch (after result failure) so invalid entities are
removed from the store.
---
Outside diff comments:
In `@server/handlers.go`:
- Around line 44-47: handleGetEntity dereferences entity.GtsID.ID which can be
nil or represent a raw anonymous ID; update handleGetEntity to safely handle
anonymous instances by checking entity.GtsID and its ID before dereferencing and
producing the JSON: if GtsID is nil or its ID is empty/anonymous return a safe
representation (e.g., omit "id" or return null/anonymous marker) instead of
accessing entity.GtsID.ID; adjust any related code paths that build the response
(the block that calls s.writeJSON) to use a helper or conditional based on
entity.GtsID to avoid panics when reading anonymous instances.
🪄 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: 4799e156-03b8-4b63-a764-ca610a38fdeb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignore.gts-specCLAUDE.mdMakefilego.modgts/extract.gogts/schema_compat.gogts/schema_modifiers.gogts/schema_modifiers_test.gogts/schema_traits.gogts/store.gogts/validate.goserver/handlers.go
aviator5
force-pushed
the
gts-spec-0.9-support
branch
from
f7146e2 to
76a9b0e
Compare
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)
server/handlers.go (1)
44-47:⚠️ Potential issue | 🟠 MajorUse
EffectiveID()when returning anonymous entities.Anonymous instances registered by the new add paths can have
GtsID == nil;entity.GtsID.IDwill panic onGET /entities/{id}for those entries. Echo the effective ID instead.🐛 Proposed fix
s.writeJSON(w, http.StatusOK, map[string]any{ - "id": entity.GtsID.ID, + "id": entity.EffectiveID(), "content": entity.Content, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 44 - 47, The handler currently accesses entity.GtsID.ID which can be nil for anonymous entities and will panic; replace that with entity.EffectiveID() when building the JSON response in the code that calls s.writeJSON (so the "id" field uses entity.EffectiveID()), ensuring GET /entities/{id} returns the effective identifier even for anonymous instances and avoids dereferencing GtsID.
🧹 Nitpick comments (1)
Makefile (1)
3-3: Optional: add acleanphony target.
checkmakeflags the missingcleantarget (minphony). Given the e2e flow produceslogs/,e2e.log,.server.pid, andbin/, acleantarget would also be convenient beyond silencing the linter.Suggested addition
-.PHONY: help build dev-fmt all check fmt vet lint test security coverage update-spec e2e-venv e2e +.PHONY: help build dev-fmt all check fmt vet lint test security coverage update-spec e2e-venv e2e clean @@ +# Remove build artifacts and e2e run state +clean: + `@rm` -rf bin logs e2e.log .server.pid coverage.out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 3, Add a new phony target named "clean" to the Makefile and include it in the existing .PHONY list so checkmake/minphony is satisfied; implement the clean target to safely remove the artifacts produced by the e2e flow (at minimum remove logs/, e2e.log, .server.pid, and bin/) and any other build output, ensuring the command is idempotent (no errors if files/dirs are missing).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 93-99: The Makefile target starting the backgrounded `./bin/gts
server` leaves the server running if the user interrupts tests; wrap the server
lifecycle in a shell that installs a trap to kill the PID in `.server.pid` and
remove the file on SIGINT/SIGTERM/EXIT. Specifically, run the server and test
sequence via a single `sh -c`/`bash -c` invocation that registers `trap 'kill
$(cat .server.pid) 2>/dev/null || true; rm -f .server.pid' INT TERM EXIT` before
launching `./bin/gts server --port $(PORT) & echo $$! > .server.pid`, then run
the pytest command and finally perform the same `kill`/`rm` cleanup in the
normal exit path so both normal completion and interruptions remove
`.server.pid` and stop the server.
In `@server/handlers.go`:
- Around line 211-227: Non-schema entities currently bypass the instance-only
modifier check; call gts.ValidateInstanceModifiers(entity.Content) for
non-schema entities (when entity.IsSchema is false) before registration and
return http.StatusUnprocessableEntity via s.writeJSON on error (same pattern
used for gts.ExtractSchemaModifiers and gts.ValidateModifierPlacement). Also add
the same guard where entities are bulk-added (the other registration block that
mirrors this logic) so x-gts-final / x-gts-abstract are rejected for instances
consistently.
---
Outside diff comments:
In `@server/handlers.go`:
- Around line 44-47: The handler currently accesses entity.GtsID.ID which can be
nil for anonymous entities and will panic; replace that with
entity.EffectiveID() when building the JSON response in the code that calls
s.writeJSON (so the "id" field uses entity.EffectiveID()), ensuring GET
/entities/{id} returns the effective identifier even for anonymous instances and
avoids dereferencing GtsID.
---
Nitpick comments:
In `@Makefile`:
- Line 3: Add a new phony target named "clean" to the Makefile and include it in
the existing .PHONY list so checkmake/minphony is satisfied; implement the clean
target to safely remove the artifacts produced by the e2e flow (at minimum
remove logs/, e2e.log, .server.pid, and bin/) and any other build output,
ensuring the command is idempotent (no errors if files/dirs are missing).
🪄 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: d1046c20-16a8-429a-824d-2f744a82bde3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignore.gts-specCLAUDE.mdMakefilego.modgts/extract.gogts/schema_compat.gogts/schema_modifiers.gogts/schema_modifiers_test.gogts/schema_traits.gogts/store.gogts/validate.goserver/handlers.go
✅ Files skipped from review due to trivial changes (4)
- go.mod
- .gitignore
- CLAUDE.md
- gts/schema_modifiers.go
🚧 Files skipped from review as they are similar to previous changes (5)
- gts/schema_compat.go
- gts/store.go
- gts/extract.go
- gts/schema_traits.go
- .gts-spec
aviator5
force-pushed
the
gts-spec-0.9-support
branch
from
76a9b0e to
7545cbb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/handlers.go (2)
57-60: Consolidate the duplicatevalidate/validationquery-param read.
validationParamis read at lines 57–60 and then re-read intovalidationat 229–233 with identical logic.validationParamis only consumed once (line 140) to pick the status code whenresponseID == "". Either drop the duplicate read or compute once up-front and reuse.♻️ Proposed refactor
- validationParam := r.URL.Query().Get("validate") - if validationParam == "" { - validationParam = r.URL.Query().Get("validation") - } + validation := r.URL.Query().Get("validate") + if validation == "" { + validation = r.URL.Query().Get("validation") + } @@ - if responseID == "" { - status := http.StatusOK - if validationParam == "true" { - status = http.StatusUnprocessableEntity - } + if responseID == "" { + status := http.StatusOK + if validation == "true" { + status = http.StatusUnprocessableEntity + } @@ - // Check if validation is requested via query parameter - validation := r.URL.Query().Get("validate") - if validation == "" { - validation = r.URL.Query().Get("validation") - } - if validation == "true" { + if validation == "true" {Also applies to: 229-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 57 - 60, The duplicate query-param read for "validate"/"validation" should be consolidated: compute a single variable (currently named validationParam) once near the start of the request handler (where validationParam is first set) by checking r.URL.Query().Get("validate") then fallback to r.URL.Query().Get("validation"), then remove the later redundant block that reassigns `validation` (the second read at the later lines). Ensure all usages (including the branch that checks responseID == "" and uses validationParam to choose the status code) reference the single computed variable (validationParam) so the logic is not duplicated.
235-261: Rollback reaches throughItems()into the raw map.
s.store.Items()[responseID] = previousmutates the store’s internal map directly. It works becauseItems()returns the live reference, but it leaks the store's internal representation into the handler and bypasses any future synchronization or bookkeepingRegister/Unregistermay grow. Consider exposing a small helper onGtsStore(e.g.,ReplaceOrUnregister(id, prev *JsonEntity, had bool)or pairing asnapshot(id) (prev, had)withrestore(id, prev, had)) and keep the handler talking only through the store API.Not a correctness issue today, just a boundary nit — handlers should stay thin and leave this bookkeeping to
gts/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 235 - 261, The rollback defer currently mutates the store internals via s.store.Items()[responseID] = previous and s.store.Unregister(responseID); replace that direct map access by adding a small GtsStore API (e.g., RestoreOrUnregister(id string, prev *JsonEntity, had bool) or Restore(id string, prev *JsonEntity, had bool)) and call it from the handler’s defer instead of touching Items(); keep the handler using Register/Unregister/Items() symbols shown (responseID, previous, hadPrevious, committed) but invoke the new store method so the store implementation can encapsulate synchronization/bookkeeping.gts/schema_modifiers_test.go (1)
1-708: LGTM — thorough coverage of modifier parsing, placement, and end-to-end behavior.Good coverage across:
ExtractSchemaModifiers(defaults, true/false, non-boolean, mutual exclusion).ValidateModifierPlacement(top-level vs. nestedallOf, recursive).ValidateInstanceModifiers(clean/final/abstract).- Integration over chain/traits validation including mid-chain final, abstract chains, sibling unaffected, and both-modifiers rejected.
A couple of small, non-blocking notes:
strings.Contains(result.Error, "final")/"abstract"/"priority"/"x-gts-final"(lines 208, 341, 559, 582, 654) couples tests to exact wording of error messages. Consider asserting on a stable sentinel (error type/code) or a narrower prefix ifgtsexposes one, to reduce churn when error strings are rephrased.mustRegisterModmutates the caller's map by injecting$schema(line 20). Fine for these literal-map tests, but if any future test reuses a shared fixture it will silently mutate it — worth a brief comment on the helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/schema_modifiers_test.go` around lines 1 - 708, Tests assert on substrings of result.Error (e.g., in TestFinal_RejectDerived, TestAbstract_RejectDirectInstance, TestFinal_WithTraitsMissing, TestSchemaKeywordsInInstance_FinalRejected) which couples them to exact wording and makes them brittle; change these assertions to check a stable sentinel (prefer an exported error value/type or a narrower prefix) returned by the validation functions (ValidateSchemaChain/ValidateInstance/ValidateEntity/ValidateSchemaTraits) instead of raw string contains, and update those tests to reference the sentinel; also add a brief comment to mustRegisterMod noting that it mutates the caller's map by injecting "$schema" so future shared-fixture usage is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gts/schema_modifiers_test.go`:
- Around line 1-708: Tests assert on substrings of result.Error (e.g., in
TestFinal_RejectDerived, TestAbstract_RejectDirectInstance,
TestFinal_WithTraitsMissing, TestSchemaKeywordsInInstance_FinalRejected) which
couples them to exact wording and makes them brittle; change these assertions to
check a stable sentinel (prefer an exported error value/type or a narrower
prefix) returned by the validation functions
(ValidateSchemaChain/ValidateInstance/ValidateEntity/ValidateSchemaTraits)
instead of raw string contains, and update those tests to reference the
sentinel; also add a brief comment to mustRegisterMod noting that it mutates the
caller's map by injecting "$schema" so future shared-fixture usage is explicit.
In `@server/handlers.go`:
- Around line 57-60: The duplicate query-param read for "validate"/"validation"
should be consolidated: compute a single variable (currently named
validationParam) once near the start of the request handler (where
validationParam is first set) by checking r.URL.Query().Get("validate") then
fallback to r.URL.Query().Get("validation"), then remove the later redundant
block that reassigns `validation` (the second read at the later lines). Ensure
all usages (including the branch that checks responseID == "" and uses
validationParam to choose the status code) reference the single computed
variable (validationParam) so the logic is not duplicated.
- Around line 235-261: The rollback defer currently mutates the store internals
via s.store.Items()[responseID] = previous and s.store.Unregister(responseID);
replace that direct map access by adding a small GtsStore API (e.g.,
RestoreOrUnregister(id string, prev *JsonEntity, had bool) or Restore(id string,
prev *JsonEntity, had bool)) and call it from the handler’s defer instead of
touching Items(); keep the handler using Register/Unregister/Items() symbols
shown (responseID, previous, hadPrevious, committed) but invoke the new store
method so the store implementation can encapsulate synchronization/bookkeeping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47f0cfc7-ef82-4607-b9bc-6febabc4432d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignore.gts-specCLAUDE.mdMakefilego.modgts/extract.gogts/schema_compat.gogts/schema_modifiers.gogts/schema_modifiers_test.gogts/schema_traits.gogts/store.gogts/validate.goserver/handlers.go
✅ Files skipped from review due to trivial changes (4)
- go.mod
- .gts-spec
- .gitignore
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- gts/schema_compat.go
- gts/schema_traits.go
- gts/extract.go
- gts/validate.go
- gts/schema_modifiers.go
aviator5
force-pushed
the
gts-spec-0.9-support
branch
from
7545cbb to
70bbd90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/handlers.go (1)
309-337:⚠️ Potential issue | 🟡 MinorBulk handler skips
ValidateSchemaModifiers, diverging from single-entity path.
handleAddEntityalways rejects schemas wherex-gts-final/x-gts-abstractare malformed or both-true (lines 211-220), independent ofvalidate=true.handleAddEntitiesregisters each entity straight vias.store.Registerwith no equivalent gate, so a schema with both modifiers true (or non-boolean values) will be persisted via bulk add and only fail later at validate time. This is inconsistent.(Note: per the spec-conformance constraint, instances carrying these keys must still register successfully — the suggestion only covers the
entity.IsSchemabranch.)🛡️ Proposed fix
for i, content := range contents { entity := gts.NewJsonEntity(content, gts.DefaultGtsConfig()) responseID := entity.EffectiveID() if responseID == "" { result[i] = map[string]any{ "ok": false, "error": "Unable to extract GTS ID from entity", } continue } + if entity.IsSchema { + if err := gts.ValidateSchemaModifiers(entity.Content); err != nil { + result[i] = map[string]any{ + "ok": false, + "error": err.Error(), + } + continue + } + } err := s.store.Register(entity)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 309 - 337, handleAddEntities currently bypasses the schema modifier validations applied in handleAddEntity, allowing schemas with malformed or both-true x-gts-final/x-gts-abstract to be registered; update handleAddEntities to call the same validation path: after creating entity via gts.NewJsonEntity and before calling s.store.Register, if entity.IsSchema is true invoke ValidateSchemaModifiers (or the same validation logic used in handleAddEntity) and on validation error populate result[i] with the same "ok": false / "error": ... response and skip registration; otherwise proceed to s.store.Register as before so bulk adds behave consistently with single-entity adds.
🧹 Nitpick comments (5)
Makefile (1)
91-91: Minor:logs/cleanup looks vestigial.Nothing in the current
e2erecipe writes alogs/directory anymore — onlye2e.logvia--log-file. You can droplogsfrom therm -rf(or keep it for back-compat with stale workspaces; either is fine).Optional cleanup
- `@rm` -rf logs e2e.log + `@rm` -f e2e.log🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 91, The Makefile's e2e recipe still removes a vestigial "logs" directory via the command containing "@rm -rf logs e2e.log"; update that command to remove only "e2e.log" (or keep "logs" if you prefer back-compat) so the cleanup matches current behavior — locate the rm invocation in the e2e recipe and delete "logs" from the argument list (or add a short comment explaining why "logs" is kept).server/handlers.go (2)
57-60: Consolidate the duplicatevalidate/validationquery-param parse.
validationParam(57-60) andvalidation(222-226) parse the exact same two query params with the same fallback. Pull this once at the top of the handler (or into a small helper likes.getValidateFlag(r)) and reusevalidationParamthroughout — the second variable is just dead code that can drift.♻️ Proposed refactor
validationParam := r.URL.Query().Get("validate") if validationParam == "" { validationParam = r.URL.Query().Get("validation") } @@ - // Check if validation is requested via query parameter - validation := r.URL.Query().Get("validate") - if validation == "" { - validation = r.URL.Query().Get("validation") - } - if validation == "true" { + if validationParam == "true" {Also applies to: 222-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 57 - 60, The handler duplicates parsing the same query params into two variables (`validationParam` at top and `validation` later); extract that logic once and reuse it: move the query parsing into a single shared spot at the start of the handler (or implement a small helper method like `s.getValidateFlag(r *http.Request) string`) and replace the later `validation` parsing (lines creating `validation`) with a call to that single source; remove the dead/duplicate variable and update any uses to reference the single `validationParam`/helper result.
211-220: Optional: fold the schema-modifier check into the existingentity.IsSchemablock above.Lines 151-209 already gate on
if entity.IsSchema. Adding a secondif entity.IsSchemablock immediately after for the modifier check is fine but needlessly fragments the schema-only validation pipeline. Merging keeps all schema-content checks contiguous and reads as a single ordered pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/handlers.go` around lines 211 - 220, The schema-only validation is split into two separate `if entity.IsSchema` blocks; merge the modifier check into the existing `if entity.IsSchema` block earlier so all schema validations run in one contiguous pipeline. Move the call to `gts.ValidateSchemaModifiers(entity.Content)` (and its error handling that calls `s.writeJSON(w, http.StatusUnprocessableEntity, ...)` and `return`) into the earlier `entity.IsSchema` block immediately after the other schema checks, preserving the same error response shape and flow.gts/schema_modifiers_test.go (1)
17-25:mustRegisterModsilently injects$schema.The helper sets a default
$schemaonly when one is absent. Convenient, but readers scanning a test body won't realize the registered content differs from what the test literally constructs. Consider naming it more explicitly (e.g.mustRegisterSchemaWithDefaultDraft) or always require the caller to pass it. Low priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/schema_modifiers_test.go` around lines 17 - 25, The helper mustRegisterMod silently injects a default "$schema", which hides differences between the test fixture and what gets registered; update it to make the behavior explicit by either renaming the helper to mustRegisterSchemaWithDefaultDraft (so callers/readers see the defaulting) or stop auto-injecting and require callers to include "$schema" in the content; implement the chosen approach by editing mustRegisterMod (or renaming it) and adjusting all call sites that use NewJsonEntity(content, DefaultGtsConfig()) / GtsStore.Register to pass an explicit "$schema" when needed.gts/validate.go (1)
115-124: ReusereadBoolModifierfor the abstract check.Inline
_, ok := schemaEntity.Content[KeyXGtsAbstract]+ type assertion duplicates the helper ingts/schema_modifiers.go. While stored schemas have already passed type validation at registration, defensively going through the helper keeps a single source of truth for "is the modifier truthy?" semantics and surfaces a clearer error if a malformed schema ever reaches this path.♻️ Proposed refactor
- // Check x-gts-abstract: abstract types cannot have direct instances. - if isAbstract, ok := schemaEntity.Content[KeyXGtsAbstract]; ok { - if abstract, isBool := isAbstract.(bool); isBool && abstract { - return &ValidationResult{ - ID: instanceID, - OK: false, - Error: fmt.Sprintf("type '%s' is abstract and cannot have direct instances", obj.SchemaID), - } - } - } + // Check x-gts-abstract: abstract types cannot have direct instances. + abstract, err := readBoolModifier(schemaEntity.Content, KeyXGtsAbstract) + if err != nil { + return &ValidationResult{ID: instanceID, OK: false, Error: err.Error()} + } + if abstract { + return &ValidationResult{ + ID: instanceID, + OK: false, + Error: fmt.Sprintf("type '%s' is abstract and cannot have direct instances", obj.SchemaID), + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts/validate.go` around lines 115 - 124, Replace the inline abstract check with the existing readBoolModifier helper: call readBoolModifier(schemaEntity, KeyXGtsAbstract), handle its returned (bool, error) — if error is non-nil return a ValidationResult with OK:false and Error set to the helper error message; if the bool is true return the same ValidationResult that says the type is abstract and cannot have direct instances (using instanceID and obj.SchemaID). This centralizes "truthy modifier" logic in readBoolModifier and surfaces a clear error for malformed schemas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/handlers.go`:
- Around line 228-254: Create a new method on gts.GtsStore, e.g.
RegisterWithValidation(entity, validateFn func() error) error, that acquires the
store's mutex and performs the entire critical section: snapshot current value
via Items()[responseID], call the existing Register(entity) write, run
validateFn(), and on validation failure restore the snapshot or call
Unregister(responseID) before returning the error; update the handler to call
GtsStore.RegisterWithValidation instead of doing snapshot/Register/rollback
itself and remove the manual deferred rollback/committed logic from handlers.go
so the TOCTOU race between Items(), Register(), and Unregister() is eliminated.
---
Outside diff comments:
In `@server/handlers.go`:
- Around line 309-337: handleAddEntities currently bypasses the schema modifier
validations applied in handleAddEntity, allowing schemas with malformed or
both-true x-gts-final/x-gts-abstract to be registered; update handleAddEntities
to call the same validation path: after creating entity via gts.NewJsonEntity
and before calling s.store.Register, if entity.IsSchema is true invoke
ValidateSchemaModifiers (or the same validation logic used in handleAddEntity)
and on validation error populate result[i] with the same "ok": false / "error":
... response and skip registration; otherwise proceed to s.store.Register as
before so bulk adds behave consistently with single-entity adds.
---
Nitpick comments:
In `@gts/schema_modifiers_test.go`:
- Around line 17-25: The helper mustRegisterMod silently injects a default
"$schema", which hides differences between the test fixture and what gets
registered; update it to make the behavior explicit by either renaming the
helper to mustRegisterSchemaWithDefaultDraft (so callers/readers see the
defaulting) or stop auto-injecting and require callers to include "$schema" in
the content; implement the chosen approach by editing mustRegisterMod (or
renaming it) and adjusting all call sites that use NewJsonEntity(content,
DefaultGtsConfig()) / GtsStore.Register to pass an explicit "$schema" when
needed.
In `@gts/validate.go`:
- Around line 115-124: Replace the inline abstract check with the existing
readBoolModifier helper: call readBoolModifier(schemaEntity, KeyXGtsAbstract),
handle its returned (bool, error) — if error is non-nil return a
ValidationResult with OK:false and Error set to the helper error message; if the
bool is true return the same ValidationResult that says the type is abstract and
cannot have direct instances (using instanceID and obj.SchemaID). This
centralizes "truthy modifier" logic in readBoolModifier and surfaces a clear
error for malformed schemas.
In `@Makefile`:
- Line 91: The Makefile's e2e recipe still removes a vestigial "logs" directory
via the command containing "@rm -rf logs e2e.log"; update that command to remove
only "e2e.log" (or keep "logs" if you prefer back-compat) so the cleanup matches
current behavior — locate the rm invocation in the e2e recipe and delete "logs"
from the argument list (or add a short comment explaining why "logs" is kept).
In `@server/handlers.go`:
- Around line 57-60: The handler duplicates parsing the same query params into
two variables (`validationParam` at top and `validation` later); extract that
logic once and reuse it: move the query parsing into a single shared spot at the
start of the handler (or implement a small helper method like
`s.getValidateFlag(r *http.Request) string`) and replace the later `validation`
parsing (lines creating `validation`) with a call to that single source; remove
the dead/duplicate variable and update any uses to reference the single
`validationParam`/helper result.
- Around line 211-220: The schema-only validation is split into two separate `if
entity.IsSchema` blocks; merge the modifier check into the existing `if
entity.IsSchema` block earlier so all schema validations run in one contiguous
pipeline. Move the call to `gts.ValidateSchemaModifiers(entity.Content)` (and
its error handling that calls `s.writeJSON(w, http.StatusUnprocessableEntity,
...)` and `return`) into the earlier `entity.IsSchema` block immediately after
the other schema checks, preserving the same error response shape and flow.
🪄 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: 4edb4c54-91ad-4b22-8ab8-6812aa06cbed
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignore.gts-specCLAUDE.mdMakefilego.modgts/extract.gogts/schema_compat.gogts/schema_modifiers.gogts/schema_modifiers_test.gogts/schema_traits.gogts/store.gogts/validate.goserver/handlers.go
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- go.mod
- .gts-spec
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- gts/schema_compat.go
- gts/extract.go
- gts/schema_traits.go
- gts/store.go
aviator5
force-pushed
the
gts-spec-0.9-support
branch
2 times, most recently
from
579def7 to
a27b614
Compare
- x-gts-final blocks derived schemas; x-gts-abstract blocks direct instances - enforce mutual exclusion and reject schema-only keywords in instance content - anonymous instances (spec §3.7): non-GTS id paired with a GTS type field - server accepts ?validate=true alongside ?validation=true - Makefile: bootstrap .gts-spec venv, raise FD ulimit for e2e runs - bump .gts-spec submodule and golang.org/x/text to 0.36 Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
gts-spec-0.9-support
branch
from
a27b614 to
c7ae4fc
Compare
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
Bug Fixes
Tests
Documentation / Chores