feat(sdl): align Go and TS parity tests with shared schema#280
Open
incu6us wants to merge 11 commits intoakash-network:mainfrom
Open
feat(sdl): align Go and TS parity tests with shared schema#280incu6us wants to merge 11 commits intoakash-network:mainfrom
incu6us wants to merge 11 commits intoakash-network:mainfrom
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds schema-first strict SDL load paths and strict-vs-lenient parsing APIs in Go, centralizes schema compilation and parity testing, introduces output JSON Schemas and many expected group fixtures, updates TS manifest serialization/endpoint ordering, extends TS parity tests and CI to check generated validators and output schema drift. Changes
Sequence Diagram(s)sequenceDiagram
actor Client as User
participant GoLoader as Go SDL Loader
participant SchemaVal as Schema Validator
participant SemanticVal as Semantic Validator
participant TSGen as TS Manifest Generator
rect rgba(100,150,200,0.5)
Note over Client,GoLoader: Lenient Load (Read)
Client->>GoLoader: YAML bytes
GoLoader->>GoLoader: yaml.Unmarshal
GoLoader->>SemanticVal: validateSDL(obj)
SemanticVal-->>GoLoader: ok / error
GoLoader-->>Client: SDL object / error
end
rect rgba(200,100,100,0.5)
Note over Client,SchemaVal: Strict Load (ReadStrict)
Client->>GoLoader: YAML bytes
GoLoader->>SchemaVal: validateInputAgainstSchema(bytes)
alt schema fails
SchemaVal-->>GoLoader: schema error
GoLoader-->>Client: "strict schema validation failed: ..." error
else schema passes
SchemaVal-->>GoLoader: ok
GoLoader->>GoLoader: yaml.Unmarshal
GoLoader->>SemanticVal: validateSDL(obj)
SemanticVal-->>GoLoader: ok / error
GoLoader-->>Client: SDL object / error
end
end
rect rgba(120,200,120,0.5)
Note over GoLoader,TSGen: Parity test flow (compare outputs)
GoLoader->>TSGen: Go-generated canonical JSON (manifest/groups)
TSGen-->>GoLoader: TS-generated canonical JSON
GoLoader->>SchemaVal: validate outputs against manifest/groups schemas
SchemaVal-->>GoLoader: ok / schema error
GoLoader-->>Client: parity result (match / mismatch)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
specs/sdl/README.md (1)
29-29: Minor: Clarify unknown fields handling.The phrase "TS validates, Go rejects" could be clearer. Consider rephrasing to explicitly state the behavior, e.g., "unknown fields — TS raises validation errors, Go rejects during unmarshal."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/sdl/README.md` at line 29, The README line "Parser-level checks (count ≥ 1, unknown fields — TS validates, Go rejects during unmarshal)" is ambiguous about how unknown fields are handled; update that phrase to explicitly state behavior, e.g., change "TS validates, Go rejects during unmarshal" to "TS raises validation errors, Go rejects during unmarshal" (locate the phrase in the README entry "Parser-level checks (count ≥ 1, unknown fields — ...)").go/sdl/sdl.go (1)
183-221: Consider extracting shared validation logic to reduce duplication.
ReadStrictduplicates the post-unmarshal validation sequence fromRead(lines 100-132). Extracting this into a helper function would improve maintainability.♻️ Proposed refactor to extract common validation
+// validateAndBuild performs semantic validation on an unmarshalled SDL object. +func validateAndBuild(obj *sdl) (SDL, error) { + if err := obj.validate(); err != nil { + return nil, err + } + + dgroups, err := obj.DeploymentGroups() + if err != nil { + return nil, err + } + + vgroups := make([]dtypes.GroupSpec, 0, len(dgroups)) + for _, dgroup := range dgroups { + vgroups = append(vgroups, dgroup) + } + + if err = dtypes.ValidateDeploymentGroups(vgroups); err != nil { + return nil, err + } + + m, err := obj.Manifest() + if err != nil { + return nil, err + } + + if err = m.Validate(); err != nil { + return nil, err + } + + return obj, nil +} func ReadStrict(buf []byte) (SDL, error) { if err := validateInputAgainstSchema(buf); err != nil { return nil, fmt.Errorf("strict schema validation failed: %w", err) } obj := &sdl{} if err := yaml.Unmarshal(buf, obj); err != nil { return nil, err } - if err := obj.validate(); err != nil { - return nil, err - } - - dgroups, err := obj.DeploymentGroups() - if err != nil { - return nil, err - } - - vgroups := make([]dtypes.GroupSpec, 0, len(dgroups)) - for _, dgroup := range dgroups { - vgroups = append(vgroups, dgroup) - } - - if err = dtypes.ValidateDeploymentGroups(vgroups); err != nil { - return nil, err - } - - m, err := obj.Manifest() - if err != nil { - return nil, err - } - - if err = m.Validate(); err != nil { - return nil, err - } - - return obj, nil + return validateAndBuild(obj) }The same refactor can be applied to
Read().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/sdl/sdl.go` around lines 183 - 221, Extract the duplicated post-unmarshal validation sequence in ReadStrict (and apply the same to Read) into a single helper function, e.g., func runPostUnmarshalValidation(obj *sdl) error: move calls to obj.validate(), obj.DeploymentGroups() + conversion to []dtypes.GroupSpec + dtypes.ValidateDeploymentGroups(...), and obj.Manifest() followed by m.Validate() into that helper; then replace the inline validation block in ReadStrict with a call to runPostUnmarshalValidation(obj) (and update Read to call it too) so all shared validation logic (validateInputAgainstSchema remains separate) is centralized and only invoked after yaml.Unmarshal into the sdl instance.ts/src/sdl/manifest/generateManifestVersion.ts (1)
40-50: LegacyDec formatting does not truncate amounts with >18 decimal places.If the input
amountalready contains more than 18 fractional digits, this logic returns it unchanged. While unlikely in practice, for strict parity with Go'sLegacyDecformatting, you may want to truncate to exactly 18 decimal places.♻️ Optional: truncate to 18 decimal places
if (s.includes(".")) { const [, frac] = s.split("."); const pad = 18 - (frac?.length ?? 0); - return pad > 0 ? s + "0".repeat(pad) : s; + if (pad > 0) return s + "0".repeat(pad); + if (pad < 0) return s.slice(0, s.length + pad); // truncate excess + return s; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/src/sdl/manifest/generateManifestVersion.ts` around lines 40 - 50, The amount formatting branch for LegacyDec (the if block checking key === "amount" in generateManifestVersion.ts) currently leaves inputs with >18 fractional digits intact; update it to truncate the fractional part to exactly 18 digits: after splitting value on ".", if frac.length > 18 return `${int}.${frac.slice(0,18)}` (preserving integer part and handling the case where frac is undefined), otherwise pad as before; ensure the branch still returns "0.000000000000000000" for empty/falsey strings and preserves existing padding behavior when frac.length < 18.ts/src/sdl/SDL/parity.spec.ts (1)
14-15: Ajv instantiation workaround is acceptable but consider simplifying.The complex casting to handle Ajv's non-standard ESM/CJS export works, but the type annotation could be simplified if you import types separately.
♻️ Optional: Simplify Ajv import
-import type { ErrorObject, ValidateFunction } from "ajv"; -import AjvModule from "ajv"; +import Ajv, { type ErrorObject, type ValidateFunction } from "ajv"; -// `@ts-expect-error` - AjvModule has non-standard export, cast needed for instantiation -const ajv: { compile: (schema: Record<string, unknown>) => ValidateFunction } = new (AjvModule as unknown as new (options?: { allErrors?: boolean }) => typeof AjvModule)({ allErrors: true }); +const ajv = new Ajv({ allErrors: true });If the default export issue persists, the current approach is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/src/sdl/SDL/parity.spec.ts` around lines 14 - 15, The Ajv instantiation workaround can be simplified by importing the Ajv constructor/type and the ValidateFunction type separately and using those types instead of the long inline annotation; change the code so you import the constructor type (e.g., AjvConstructor or default as AjvCtor) and ValidateFunction from 'ajv', then cast AjvModule to that constructor type when calling new (e.g., new (AjvModule as unknown as AjvCtor)({ allErrors: true })) and declare ajv using the simpler ValidateFunction-based type (referencing ajv, AjvModule, and ValidateFunction in your change).docs/sdl-parity.md (1)
51-51: Minor grammar: use hyphen for compound modifier.Static analysis flagged "TS generated validator" — consider "TS-generated validator" for grammatical correctness.
-4. Keep the TS generated validator in sync with the schema (CI drift check) +4. Keep the TS-generated validator in sync with the schema (CI drift check)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/sdl-parity.md` at line 51, Replace the phrase "TS generated validator" with the grammatically correct hyphenated form "TS-generated validator" in docs/sdl-parity.md (and any other occurrences), updating the line under the CI drift check item ("Keep the TS-generated validator in sync with the schema (CI drift check)"); search the repo for other instances of the exact string to update consistently and run the docs linter or spellcheck before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/sdl-parity.md`:
- Line 51: Replace the phrase "TS generated validator" with the grammatically
correct hyphenated form "TS-generated validator" in docs/sdl-parity.md (and any
other occurrences), updating the line under the CI drift check item ("Keep the
TS-generated validator in sync with the schema (CI drift check)"); search the
repo for other instances of the exact string to update consistently and run the
docs linter or spellcheck before committing.
In `@go/sdl/sdl.go`:
- Around line 183-221: Extract the duplicated post-unmarshal validation sequence
in ReadStrict (and apply the same to Read) into a single helper function, e.g.,
func runPostUnmarshalValidation(obj *sdl) error: move calls to obj.validate(),
obj.DeploymentGroups() + conversion to []dtypes.GroupSpec +
dtypes.ValidateDeploymentGroups(...), and obj.Manifest() followed by
m.Validate() into that helper; then replace the inline validation block in
ReadStrict with a call to runPostUnmarshalValidation(obj) (and update Read to
call it too) so all shared validation logic (validateInputAgainstSchema remains
separate) is centralized and only invoked after yaml.Unmarshal into the sdl
instance.
In `@specs/sdl/README.md`:
- Line 29: The README line "Parser-level checks (count ≥ 1, unknown fields — TS
validates, Go rejects during unmarshal)" is ambiguous about how unknown fields
are handled; update that phrase to explicitly state behavior, e.g., change "TS
validates, Go rejects during unmarshal" to "TS raises validation errors, Go
rejects during unmarshal" (locate the phrase in the README entry "Parser-level
checks (count ≥ 1, unknown fields — ...)").
In `@ts/src/sdl/manifest/generateManifestVersion.ts`:
- Around line 40-50: The amount formatting branch for LegacyDec (the if block
checking key === "amount" in generateManifestVersion.ts) currently leaves inputs
with >18 fractional digits intact; update it to truncate the fractional part to
exactly 18 digits: after splitting value on ".", if frac.length > 18 return
`${int}.${frac.slice(0,18)}` (preserving integer part and handling the case
where frac is undefined), otherwise pad as before; ensure the branch still
returns "0.000000000000000000" for empty/falsey strings and preserves existing
padding behavior when frac.length < 18.
In `@ts/src/sdl/SDL/parity.spec.ts`:
- Around line 14-15: The Ajv instantiation workaround can be simplified by
importing the Ajv constructor/type and the ValidateFunction type separately and
using those types instead of the long inline annotation; change the code so you
import the constructor type (e.g., AjvConstructor or default as AjvCtor) and
ValidateFunction from 'ajv', then cast AjvModule to that constructor type when
calling new (e.g., new (AjvModule as unknown as AjvCtor)({ allErrors: true }))
and declare ajv using the simpler ValidateFunction-based type (referencing ajv,
AjvModule, and ValidateFunction in your change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff7bd8b3-1a5c-447d-8a9a-9d091b1c8071
📒 Files selected for processing (30)
.github/workflows/tests.yamldocs/sdl-parity.mdgo/sdl/parity_test.gogo/sdl/schema_validator.gogo/sdl/sdl.gogo/sdl/strict_test.gomake/test.mkspecs/sdl/README.mdspecs/sdl/groups-output.schema.yamlspecs/sdl/manifest-output.schema.yamltestdata/sdl/input/semantic-only-invalid/duplicate-mount-path.yamltestdata/sdl/input/semantic-only-invalid/endpoint-not-used.yamltestdata/sdl/input/semantic-only-invalid/missing-compute-profile.yamltestdata/sdl/output-fixtures/v2.0/gpu-basic/groups.jsontestdata/sdl/output-fixtures/v2.0/http-options/groups.jsontestdata/sdl/output-fixtures/v2.0/ip-endpoint/groups.jsontestdata/sdl/output-fixtures/v2.0/multiple-services/groups.jsontestdata/sdl/output-fixtures/v2.0/persistent-storage/groups.jsontestdata/sdl/output-fixtures/v2.0/placement/groups.jsontestdata/sdl/output-fixtures/v2.0/port-ranges/groups.jsontestdata/sdl/output-fixtures/v2.0/pricing/groups.jsontestdata/sdl/output-fixtures/v2.0/simple/groups.jsontestdata/sdl/output-fixtures/v2.0/storage-classes/groups.jsontestdata/sdl/output-fixtures/v2.1/credentials/groups.jsontestdata/sdl/output-fixtures/v2.1/ip-endpoint/groups.jsonts/src/sdl/SDL/parity.spec.tsts/src/sdl/manifest/generateManifest.tsts/src/sdl/manifest/generateManifestVersion.tsts/src/sdl/manifest/manifestUtils.tsts/test/functional/sdl-parity.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/sdl-parity.md (1)
27-38: Add a language specifier to the fenced code block.The directory structure code block lacks a language specifier, which triggers markdownlint MD040. Use
textorplaintextfor tree-like directory structures.📝 Proposed fix
-``` +```text testdata/sdl/ ├── input/ # SDL input YAML files🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/sdl-parity.md` around lines 27 - 38, Update the fenced code block showing the directory tree in docs/sdl-parity.md to include a language specifier so markdownlint MD040 is satisfied: change the opening backticks for the directory structure block from ``` to ```text (or ```plaintext) so the tree is treated as plain text; the block is the one that begins with the lines showing "testdata/sdl/" and the nested input/ and output-fixtures/ directories.go/sdl/parity_test.go (1)
282-298: Consider extracting shared validation result formatting.This function duplicates the validation result handling pattern from
validateInputAgainstSchemainschema_validator.go(lines 120-130). While acceptable in test code, you could extract a common helper if this pattern spreads further.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/sdl/parity_test.go` around lines 282 - 298, The validation-result formatting logic in validateDataAgainstCompiledSchema duplicates the same pattern used by validateInputAgainstSchema (schema_validator.go); extract a shared helper (e.g., formatValidationErrors(result *gojsonschema.Result) string or collectValidationErrors(result *gojsonschema.Result) []string) and replace the duplicated loop in validateDataAgainstCompiledSchema to call that helper so both validateDataAgainstCompiledSchema and validateInputAgainstSchema reuse the same error-formatting function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/sdl-parity.md`:
- Around line 27-38: Update the fenced code block showing the directory tree in
docs/sdl-parity.md to include a language specifier so markdownlint MD040 is
satisfied: change the opening backticks for the directory structure block from
``` to ```text (or ```plaintext) so the tree is treated as plain text; the block
is the one that begins with the lines showing "testdata/sdl/" and the nested
input/ and output-fixtures/ directories.
In `@go/sdl/parity_test.go`:
- Around line 282-298: The validation-result formatting logic in
validateDataAgainstCompiledSchema duplicates the same pattern used by
validateInputAgainstSchema (schema_validator.go); extract a shared helper (e.g.,
formatValidationErrors(result *gojsonschema.Result) string or
collectValidationErrors(result *gojsonschema.Result) []string) and replace the
duplicated loop in validateDataAgainstCompiledSchema to call that helper so both
validateDataAgainstCompiledSchema and validateInputAgainstSchema reuse the same
error-formatting function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b144625b-16d7-497b-8514-9651d4cb1f47
📒 Files selected for processing (4)
docs/sdl-parity.mdgo/sdl/parity_test.gogo/sdl/sdl.gospecs/sdl/README.md
✅ Files skipped from review due to trivial changes (1)
- specs/sdl/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- go/sdl/sdl.go
incu6us
force-pushed
the
sdl-parity-fixes-v2
branch
from
7bb20c7 to
f7b6cb0
Compare
cloud-j-luna
reviewed
Apr 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts/src/sdl/manifest/generateManifestVersion.ts`:
- Around line 40-43: The amount normalization branch in
generateManifestVersion.ts only formats string amounts, but PriceCoin.amount can
be number; update the condition handling where (key === "amount") in the
function (the block using formatLegacyDec) to also accept numeric values by
converting number values to strings (e.g., String(value)) before calling
formatLegacyDec, and apply the same change to the similar block around lines
56-64 so numeric literals don't bypass LegacyDec normalization; reference the
exact condition checks using key === "amount" and formatLegacyDec to locate and
modify the code.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e5a515e-17f7-4532-9bdc-52fd7e35d3c0
📒 Files selected for processing (2)
go/sdl/sdl.gots/src/sdl/manifest/generateManifestVersion.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- go/sdl/sdl.go
incu6us
commented
Apr 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts/src/sdl/manifest/generateManifestVersion.ts`:
- Around line 40-43: The formatLegacyDec function (used in the key === "amount"
branch) currently converts values with String(value) which can produce exponent
notation and returns inputs with >18 fractional digits unchanged; update
formatLegacyDec to robustly normalize numeric and string inputs into a plain
decimal string (no exponent) before validation, then enforce the 18-decimal
LegacyDec canonical form by truncating (not rounding) any fractional digits
beyond 18 and padding with trailing zeros up to 18 decimals (or throw if you
prefer strict rejection), and ensure the function always returns a validated
18-decimal string; handle numeric inputs by converting them to a non-exponential
decimal representation first (e.g., via a decimal/big-number routine or fixed
precision conversion) so inputs like 1e-7 become "0.000000100000000000".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7d7c6d1-844f-4ec1-a55d-c524a0aa65f8
📒 Files selected for processing (1)
ts/src/sdl/manifest/generateManifestVersion.ts
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
2 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.
📝 Description
Aligns Go and TS SDL implementations by fixing TS serialization parity (field naming, null handling, endpoint ordering, amount formatting), adding strict schema enforcement in Go (
ReadStrict/ReadFileStrict) and expanding parity test coverage to include group-specs comparison, semantic/schema-only invalid categories, output schema validation and canonical equality checks. Establishesgo/sdl/sdl-input.schema.yamlas the single source of truth for both implementations.🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
Go changes (
go/sdl/)sdl.go: AddedReadStrict()/ReadFileStrict()- strict APIs that fail on schema violations (existingRead/ReadFileremain lenient for backward compatibility)schema_validator.go: Bidirectional mismatch logging - now logs both directions (schema-pass/Go-fail AND schema-fail/Go-pass)parity_test.go: AddedTestSemanticOnlyInvalidfor inputs valid per schema but rejected by parser; added output schema validation viaspecs/sdl/*.schema.yaml-strict_test.go: Tests forReadStrictcovering valid, invalid and schema-only-invalid fixturesTypeScript changes (
ts/)generateManifestVersion.ts: JSON wire format field mapping (signedBy→signed_by,allOf→all_of,anyOf→any_ofduring serialization only), nullableallOf/anyOf, context-awareattributesnull handling, LegacyDec amount formattinggenerateManifest.ts: Fixed endpoint sorting — per-expose sort matching Go'sGetEndpoints(), removed incorrect global sortmanifestUtils.ts: Endpoint ordering fix -[leasedEp, defaultEp].sort()matching Gosdl-parity.spec.ts: Added group-specs comparison, semantic/schema-only-invalid tests, output schema validation, canonical manifest equality checksCI & docs
make/test.mk: Added TS schema drift check, expanded Go test patterns.github/workflows/tests.yaml: Added Go cache-dependency-pathdocs/sdl-parity.md: ADR documenting schema ownership and parity rulesTest fixtures
testdata/sdl/input/semantic-only-invalid/- 3 new fixtures (endpoint-not-used, duplicate-mount-path, missing-compute-profile)Test results