Feature - spec upgrade to 0.13 - #16
Conversation
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
…LOG and pin spec version Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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:
📝 WalkthroughWalkthroughThe project upgrades to GTS spec v0.13.1. It adds tri-state compatibility, Type Schema API contracts, modifier and trait validation, revised inheritance rules, updated identifier matching, and aligned tooling, documentation, and tests. ChangesGTS specification upgrade
Type Schema contracts and compatibility
Validation and identifier behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant GtsStore
participant GtsCompatibility
Client->>Server: register or validate a Type Schema
Server->>GtsStore: validate modifiers, inheritance, and traits
GtsStore-->>Server: validation result
Server-->>Client: Type Schema response
Client->>Server: request compatibility
Server->>GtsCompatibility: compare old and new type IDs
GtsCompatibility->>GtsStore: resolve schemas
GtsStore-->>GtsCompatibility: effective schemas
GtsCompatibility-->>Server: directional verdicts
Server-->>Client: compatibility result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cast.ts (1)
60-70: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the
/castroute with the documented cast implementation.
POST /castcallsthis.store['store'].castInstance(...), while the CLI and publicGTS.castInstance(...)path returnGtsCast.castInstance(...). The changelog documentsGtsCast.castInstance()removing the compatibility gate and always returningok: true, but/caststill reports success fromGtsStore.castInstance()compatibility/validation. Route the HTTP cast throughGtsCast.castInstance()or update the changelog/docs to describe the HTTP route 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 `@src/cast.ts` around lines 60 - 70, Align the POST /cast handler with the documented cast behavior by routing it through GtsCast.castInstance(), matching the CLI and public GTS.castInstance() paths. Update the handler around performCast and remove reliance on this.store['store'].castInstance() compatibility validation so successful casts return the documented ok: true result.
🤖 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 @.github/workflows/ci.yml:
- Around line 155-161: Provide the missing .gts-spec/tests/requirements.txt
dependency source referenced by the “Install pytest and dependencies” workflow
step, along with the matching .gts-spec/tests/Dockerfile setup if the workflow
relies on it. Ensure the requirements include compatible pytest, requests, and
pydantic dependencies, while preserving the separate no-deps installation of
httprunner.
In `@package.json`:
- Line 3: Update the OpenAPI metadata returned by the server’s OpenAPI
generation logic to use version 0.4.0, matching the package version declared in
package.json. Locate the version field in the relevant server implementation and
replace the stale 0.1.0 value without changing other metadata.
In `@src/compatibility.ts`:
- Around line 571-573: Update the compatibility result handling around
added_properties, removed_properties, and changed_properties to address their
now-always-empty values: either repopulate these fields from the resolved
effective schemas, or document the behavior change in CHANGELOG.md and mark the
fields optional or deprecated in CompatibilityResult. Keep all consumers of GET
/compatibility and the library result aligned with the chosen contract.
- Around line 514-523: Update the missing-entity handling before the buildResult
call so each message appears only once in incompatibility_reasons. Do not pass
missing as both backward and forward error lists; assign the reasons to one
appropriate list while leaving the other empty, or deduplicate the concatenated
lists inside buildResult if both directions require them.
- Around line 196-256: Update mergeSchemas to validate enum operands are arrays
before filtering; otherwise preserve the divergence as an unknown/incompatible
result instead of throwing. Likewise validate numeric bound operands for
minimum, exclusiveMinimum, minLength, minItems, maximum, exclusiveMaximum,
maxLength, and maxItems, and ensure malformed values are propagated so
compareBounds reports unknown rather than producing a definitive verdict.
In `@src/gts.ts`:
- Around line 522-543: Update parseSegment in src/gts.ts (lines 522-543) to
accept a seventh token only when it is the terminal wildcard, parse the explicit
minor version, and set seg.isWildcard after parsing it so
wildcardPatternVersion’s minorSpecified path is reachable. Add coverage in
tests/id-patterns.test.ts (lines 5-18) verifying v0.2.* matches v0.2 and rejects
v0.3.
In `@src/modifiers.ts`:
- Around line 85-86: Update Modifiers.scan to traverse all nested JSON values
iteratively with cycle protection instead of silently returning when depth
exceeds MAX_DEPTH. If a traversal limit remains necessary, propagate a
validation error when it is reached, and add a regression test confirming deeply
nested x-gts-final, x-gts-abstract, x-gts-traits, or x-gts-traits-schema
keywords are detected.
In `@src/server/server.ts`:
- Around line 175-208: Move the final-base, abstract-type, and
misplaced-schema-keyword checks out of the validate-gated logic in the
single-registration handler into shared registration validation. Invoke that
shared validation unconditionally from both the single registration flow and
handleAddEntities before register, while preserving the existing 422 responses
and error messages.
- Line 71: Update getOpenAPIPaths() in src/server/server.ts at lines 71-71 and
106-107 to publish both new Type Schema operations: add POST /type-schemas with
its request body and result schema, and POST /validate-type-schema with its
request body and result schema, so GET /openapi exposes both routes.
- Around line 437-439: Update the content construction in the handleAddEntity
flow to remove type_schema.$id before applying type_id as $$id. Ensure
validateSchemaStrict cannot read an embedded $id and the explicit type_id
remains the sole identifier used for validation.
In `@src/store.ts`:
- Around line 1003-1015: Move the traitsProhibited check in the schema
validation flow before the abstract-type early return, so abstract schemas
carrying materialized traits are rejected when the inheritance chain sets
x-gts-traits-schema to false. Keep the existing abstract completeness exemption
and successful return unchanged for schemas without prohibited traits.
- Line 628: Update every failure return in castInstance to use the to_type_id
field instead of to_schema_id, matching the existing success response and the
field consumed by handleCast. Ensure all seven castInstance error paths
consistently include the target type identifier under to_type_id.
- Around line 907-908: Update the inheritance detection around findParentRef and
compareOverlayToBase so it verifies that an allOf $ref or $$ref targets the
chain parent identified by parentId, rather than treating any reference as
inheritance. If multiple references are supported, return all references from
findParentRef and set inheritsViaRef only when parentId is among them; preserve
the existing loosening checks for unrelated references.
---
Outside diff comments:
In `@src/cast.ts`:
- Around line 60-70: Align the POST /cast handler with the documented cast
behavior by routing it through GtsCast.castInstance(), matching the CLI and
public GTS.castInstance() paths. Update the handler around performCast and
remove reliance on this.store['store'].castInstance() compatibility validation
so successful casts return the documented ok: true result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92f0b60f-8297-48ba-9b01-432f7a59f509
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/workflows/ci.yml.gts-spec.gts-spec-versionCHANGELOG.mdMakefileREADME.mdpackage.jsonsrc/cast.tssrc/cli/index.tssrc/compatibility.tssrc/extract.tssrc/gts.tssrc/index.tssrc/modifiers.tssrc/query.tssrc/server/server.tssrc/server/types.tssrc/store.tssrc/types.tstests/compatibility.test.tstests/gts.test.tstests/id-patterns.test.tstests/modifiers.test.tstests/traits.test.ts
…ed schemas Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
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)
src/store.ts (1)
1552-1565: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject omitted optional parent properties for standalone derivation.
When
inheritsViaRefis false, this block checks onlyrequiredandadditionalProperties. It does not require the derived schema to restate optional parent properties.For example, a parent property
{ a: { type: 'string' } }and a derived schema withadditionalProperties: trueaccepts{ a: 1 }. The parent rejects that value.validateSchemaAgainstParentaccepts this loosening.Iterate
basePropsin this branch and reject every non-false parent property thatoverlayPropsomits. Add a regression test for an open parent with an optional constrained property.Proposed fix
if (!inheritsViaRef) { + if (overlay.additionalProperties !== false) { + for (const [propName, baseProp] of Object.entries(baseProps)) { + if (baseProp !== false && !(propName in overlayProps)) { + const propPath = path ? `${path}.${propName}` : propName; + errors.push(`Property '${propPath}' is declared in base but not restated in derived`); + } + } + } + // The derived schema stands alone, so it must carry the base's // constraints itself rather than inherit them through a $ref.🤖 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 `@src/store.ts` around lines 1552 - 1565, When inheritsViaRef is false, update the standalone-derivation validation near baseResolved.required to iterate baseProps and reject every parent property whose schema is not false when overlayProps omits it, preserving the existing required and additionalProperties checks. Add a regression test covering an open parent with an optional constrained property and a derived schema that omits it, ensuring invalid values are rejected.
♻️ Duplicate comments (1)
src/compatibility.ts (1)
232-257: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve malformed
allOfbranches asunknown.When the left branch is valid and the right branch is malformed, this code keeps the valid left value. It removes evidence of the malformed keyword before
compareBoundsor fixed-value comparison runs.For example,
{ allOf: [{ maximum: 10 }, { maximum: 'ten' }] }can compare as compatible with{ maximum: 10 }. The same issue applies to a validenumfollowed by a non-arrayenum.Track malformed merged keywords through
SchemaResolver, then returnunknownfrom the comparison. Add tests for bothallOfoperand orders and assertunknown.🤖 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 `@src/compatibility.ts` around lines 232 - 257, Update mergeSchemas and SchemaResolver to preserve markers for malformed right- or left-hand allOf keywords instead of retaining the valid operand value. Have comparison logic, including compareBounds and fixed-value/enum comparison, detect these markers and return unknown. Add coverage for malformed maximum and enum operands in both allOf operand orders, asserting unknown.
🤖 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 `@src/store.ts`:
- Around line 1015-1019: Update validateEntity to perform a separate
entity-level trait completeness check across the full inheritance chain, rather
than relying on validateSchemaTraits and its materialized defaults. Require
explicit x-gts-traits values for required traits on concrete schemas, while
preserving the existing abstract-schema exemption in the shown self check.
---
Outside diff comments:
In `@src/store.ts`:
- Around line 1552-1565: When inheritsViaRef is false, update the
standalone-derivation validation near baseResolved.required to iterate baseProps
and reject every parent property whose schema is not false when overlayProps
omits it, preserving the existing required and additionalProperties checks. Add
a regression test covering an open parent with an optional constrained property
and a derived schema that omits it, ensuring invalid values are rejected.
---
Duplicate comments:
In `@src/compatibility.ts`:
- Around line 232-257: Update mergeSchemas and SchemaResolver to preserve
markers for malformed right- or left-hand allOf keywords instead of retaining
the valid operand value. Have comparison logic, including compareBounds and
fixed-value/enum comparison, detect these markers and return unknown. Add
coverage for malformed maximum and enum operands in both allOf operand orders,
asserting unknown.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eeac8f78-1e44-4f52-a67c-5d8e57c44e21
📒 Files selected for processing (11)
CHANGELOG.mdsrc/compatibility.tssrc/gts.tssrc/modifiers.tssrc/server/server.tssrc/store.tssrc/types.tstests/compatibility.test.tstests/gts.test.tstests/modifiers.test.tstests/traits.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/traits.test.ts
- src/modifiers.ts
- src/types.ts
- src/server/server.ts
- CHANGELOG.md
gs-layer
left a comment
gs-layer
left a comment
There was a problem hiding this comment.
Requesting changes based on the confirmed compatibility/cast/validation correctness issues below. I reproduced the main cases locally against the PR snapshot; the regular suite still passes, so these need focused regression tests.
…rmalization Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
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 (2)
src/store.ts (2)
990-994: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack trait-schema references by recursion path.
resolveTraitSchemaRefsretains resolved references invisitedafter it returns. A valid schema with the same$refin two siblingallOfbranches therefore fails withCyclic reference detected.Use a copied set for the recursive reference path. Keep the parent set unchanged for sibling branches.
Proposed fix
- visited.add(refId); + const nextVisited = new Set(visited); + nextVisited.add(refId); const refEntity = this.get(refId); if (!refEntity || !refEntity.content) { throw new Error(`Unresolvable trait schema reference: ${refUri}`); } - const resolved = this.resolveTraitSchemaRefs(refEntity.content, visited, depth + 1); + const resolved = this.resolveTraitSchemaRefs(refEntity.content, nextVisited, depth + 1);🤖 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 `@src/store.ts` around lines 990 - 994, Update resolveTraitSchemaRefs to pass a copied visited set when following each recursive $ref, so references are tracked only along the current recursion path. Preserve the parent visited set while processing sibling allOf branches, preventing repeated valid references from being reported as cyclic.
1025-1048: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDetect all effective trait-schema contradictions before the abstract exemption.
findUnsatisfiableTraitonly detects closed-object omissions and conflictingtypevalues. It does not detect conflictingconstvalues, disjoint enums, or incompatible bounds.For example, two inherited trait branches can declare the same property with different
constvalues. The effectiveallOfis impossible, but Line 1047 accepts an abstract schema before AJV validates it.Extend satisfiability detection for the supported JSON Schema constraints. Add regression tests for abstract schemas with conflicting trait assertions.
🤖 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 `@src/store.ts` around lines 1025 - 1048, Extend findUnsatisfiableTrait and its supporting trait-schema analysis to detect contradictions in effective allOf assertions beyond required/type conflicts, including incompatible const values, disjoint enum values, and incompatible numeric or length bounds. Ensure these checks run before the abstract-type exemption in the schema validation flow, and add regression coverage for abstract schemas whose inherited trait branches contain conflicting assertions.
🤖 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 `@src/store.ts`:
- Around line 793-800: Update validateCastResult to run XGtsRefValidator against
the casted value and target schema after AJV validation succeeds, matching the
x-gts-ref enforcement performed by validateInstance. Return the
reference-validation error when the constraint fails; otherwise preserve the
existing null success result.
---
Outside diff comments:
In `@src/store.ts`:
- Around line 990-994: Update resolveTraitSchemaRefs to pass a copied visited
set when following each recursive $ref, so references are tracked only along the
current recursion path. Preserve the parent visited set while processing sibling
allOf branches, preventing repeated valid references from being reported as
cyclic.
- Around line 1025-1048: Extend findUnsatisfiableTrait and its supporting
trait-schema analysis to detect contradictions in effective allOf assertions
beyond required/type conflicts, including incompatible const values, disjoint
enum values, and incompatible numeric or length bounds. Ensure these checks run
before the abstract-type exemption in the schema validation flow, and add
regression coverage for abstract schemas whose inherited trait branches contain
conflicting assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52f71725-0bab-428c-abb1-bc53abe92c6c
📒 Files selected for processing (6)
src/cast.tssrc/compatibility.tssrc/store.tstests/compatibility.test.tstests/gts.test.tstests/modifiers.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/modifiers.test.ts
- tests/gts.test.ts
- src/compatibility.ts
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)
src/store.ts (1)
530-555: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive cast direction from the source Type Schema ID.
Line 530 passes
fromInstanceIdtoinferDirection. The rightmost segment of an instance ID identifies the instance, not its Type Schema. This can reversedirection,backward_errors, andforward_errorswhen an instance version differs from its Type Schema version.Pass
fromSchemaIdintoperformCastand use it forinferDirection.Proposed fix
- return this.performCast(instanceId, toSchemaId, instanceContent, fromSchemaContent, toSchemaContent); + return this.performCast(instanceId, fromSchemaId, toSchemaId, instanceContent, fromSchemaContent, toSchemaContent); private performCast( fromInstanceId: string, + fromSchemaId: string, toSchemaId: string, fromInstanceContent: any, fromSchemaContent: any, toSchemaContent: any ): any { - const direction = GtsCompatibility.inferDirection(fromInstanceId, toSchemaId); + const direction = GtsCompatibility.inferDirection(fromSchemaId, toSchemaId);🤖 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 `@src/store.ts` around lines 530 - 555, Update performCast to accept fromSchemaId and use that Type Schema ID when calling GtsCompatibility.inferDirection instead of fromInstanceId, preserving the existing old/new schema selection and compatibility error handling.
🤖 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 `@CHANGELOG.md`:
- Around line 59-63: Update the GtsCast removal entry to make the registry
implementation the explicit subject of both actions: it resolved allOf/$ref on
the target and validated the result. Preserve the existing explanation and
meaning while removing the ambiguous phrasing around “validated.”
In `@src/store.ts`:
- Around line 1306-1307: Update detectRefCycle so reaching MAX_SCHEMA_DEPTH
returns an error string rather than null, while non-object or absent content
still returns null. Preserve the existing cycle-detection behavior below the
limit and ensure validateSchemaAgainstParent receives the failure result.
---
Outside diff comments:
In `@src/store.ts`:
- Around line 530-555: Update performCast to accept fromSchemaId and use that
Type Schema ID when calling GtsCompatibility.inferDirection instead of
fromInstanceId, preserving the existing old/new schema selection and
compatibility error handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89e3ac06-d370-4193-ae51-4ab36d740b3c
📒 Files selected for processing (10)
CHANGELOG.mdsrc/cast.tssrc/compatibility.tssrc/index.tssrc/modifiers.tssrc/server/server.tssrc/store.tssrc/types.tstests/compatibility.test.tstests/gts.test.ts
💤 Files with no reviewable changes (1)
- src/cast.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/gts.test.ts
- src/modifiers.ts
- src/server/server.ts
- src/compatibility.ts
gs-layer
left a comment
gs-layer
left a comment
There was a problem hiding this comment.
Repeat review after the update. The previous blockers around x-gts-ref handling, cast-result target validation, local $ref compatibility, disjoint allOf type intersection, inclusive/exclusive bounds, and misplaced modifier validation look addressed on the current head. I still found the issues below, including several that overlap with unresolved automated review threads.
…le, unify depth and validation entry points Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
a527e5b to
60c29cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Line 65: Update the changelog statement about the unchanged CastResult shape
to limit it explicitly to GTS.castInstance() and the CLI, and distinguish POST
/cast as returning the registry response with instance_id and to_type_id.
In `@src/compatibility.ts`:
- Around line 79-92: Update hasMalformedKeyword and the SchemaResolver.resolve
flow to recursively validate every schema-bearing value before compatibility
shortcuts: validate each properties child schema, require allOf to be an array,
and require $ref/$$ref to be strings. Mark invalid composition or nested schema
values as unresolved or malformed so compatibility returns unknown instead of
resolving them to unconstrained schemas, and add regression coverage for
malformed properties, allOf, $ref, and $$ref values.
In `@tests/traits.test.ts`:
- Around line 373-383: Add required constraints to the trait schemas in
tests/traits.test.ts at lines 373-383 and 397-406: require k in both schemas
with conflicting const values, and require n in both schemas with crossed
numeric bounds. No other changes are needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b061c4be-6ce1-4194-ba8c-b117c5ff664f
📒 Files selected for processing (5)
CHANGELOG.mdsrc/compatibility.tssrc/store.tstests/compatibility.test.tstests/traits.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/compatibility.test.ts
- src/store.ts
gs-layer
left a comment
gs-layer
left a comment
There was a problem hiding this comment.
Repeat review after the latest update. The five prior blockers from my previous review are fixed in their original form, and local verification is green (build, typecheck, lint, npm test -- --runInBand, make e2e with pinned gts-spec caecc273). I still found the functional issues below; also note as a non-blocking follow-up that the generated OpenAPI document still omits several exposed routes such as /cast, /validate-type-schema, and /validate-entity.
…cycle detection, ref-following flatten Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
code-ranker: 🟢 improvedBuilt on a fork. View full report ↗ md
ts
|
…lts and top-level inheritance Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
…d top-level ; add OP#8 property test Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
itechmeat
left a comment
itechmeat
left a comment
There was a problem hiding this comment.
🟢 Approving - all three blockers from the previous round are genuinely fixed, and I checked each by running the failing case at the old head and at this one rather than trusting the commit messages: the position-aware walker no longer eats data properties named title/description, number intersected with number stays number, and the cast really does follow $refs now, with the CHANGELOG, README and doc comment all corrected to match. The ajv-oracle property test has real teeth - I re-applied both mutations its docblock names and both were caught. Three non-blocking notes below, the first of which is the one thing I would fix before this gets exercised on deep hierarchies: ref-following flatten has no visited set, so a diamond hierarchy re-walks shared ancestors exponentially. Nothing here needs to hold up the merge.
…ds, share position table Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
gs-layer
left a comment
gs-layer
left a comment
There was a problem hiding this comment.
Repeat review after the latest update. The four previously reported blockers now look fixed: malformed schema-bearing keywords return unknown, inclusive/exclusive trait bounds are detected, sibling $ref reuse is no longer treated as a cycle, and nested trait defaults materialize recursively.
I still found one correctness blocker in trait satisfiability: optional property-local conflicts are rejected as if they make the whole object schema unsatisfiable. I also left a non-blocking OpenAPI contract drift note because the implemented route surface is wider than /openapi reports.
Validation run on a7f18be: npm run build, npm run typecheck, npm run lint, npm test -- --runInBand (163 tests), and .gts-spec make e2e (424 tests) all passed. Targeted repro confirms the remaining issue: AJV accepts {} for optional conflicting property constraints, while GTS.validateEntity() rejects the analogous abstract trait schema.
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
df7e91a to
ac94866
Compare
…f gaps Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
a8cfe92 to
f25f972
Compare
4 participants
Summary by CodeRabbit
New Features
Breaking Changes
Documentation