Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
This PR emits version first in SDK serialization and strengthens OrionBelt fallback validation for invalid roots and legacy wrappers.
Changes:
- Preserves serialization exclusions and schemas while ordering
versionfirst. - Adds validation regression coverage and documentation.
- Updates dbt snapshots.
| File | Description |
|---|---|
python/tests/test_models.py |
Tests serialization ordering and schema preservation. |
python/src/ossie/models.py |
Orders serialized version first. |
python/README.md |
Documents serialization ordering. |
converters/wisdom/tests/test_wisdom_to_ossie.py |
Verifies version ordering. |
converters/sigma/tests/test_roundtrip.py |
Verifies version ordering. |
converters/orionbelt/tests/test_ossie_validation.py |
Tests fallback validation cases. |
converters/orionbelt/src/ossie_orionbelt/validation.py |
Adds fallback validation errors. |
converters/dbt/tests/test_msi_to_ossie.py |
Verifies JSON ordering. |
converters/dbt/tests/__snapshots__/test_ossie_to_msi.ambr |
Updates expected ordering snapshot. |
converters/dbt/tests/__snapshots__/test_msi_to_ossie.ambr |
Updates expected ordering snapshot. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hi @khush-bhatia, here is a followup for your offline feedback. PTAL. |
|
|
||
| # Legacy wrappers are schema errors, not model contents to traverse. | ||
| if not isinstance(ossie_dict, dict) or "semantic_model" in ossie_dict: | ||
| if not isinstance(ossie_dict, dict): |
There was a problem hiding this comment.
This fallback only catches two specific malformations (ossie_dict not a dict, or a semantic_model wrapper).
When jsonschema is unavailable (or schema_path points at a missing schema), every other structurally invalid document silently comes back as valid=True with empty schema_errors/semantic_errors. For instance datasets as a string instead of a list, datasets missing entirely, name missing, or a dataset dict missing its required source field.
Since _as_dict_list() already silently drops non-dict items instead of erroring, and the downstream unique-name/reference checks only ever see whatever survives that filter, none of these malformations get flagged anywhere in this fallback.
Given this function docstring says it mirrors validate.py three layer validation, could we add guards at least the missing/malformed datasets and missing name cases here too? Or explicitly document that JSON-Schema-unavailable mode is a reduced-coverage fallback and only guarantees the two checks above?
I believe it's currently confusing because it reads as full validation but it isn't when jsonschema is absent.
Summary
SDK-generated documents currently place
versionafter the inherited model fields. Serialize it first across the SDK's dictionary, JSON, and YAML output, matching the spec examples and other converters. This changes presentation only; field selection, aliases, and the generated serialization schema are preserved.OrionBelt's
validate_ossiecan report a legacy wrapped document as valid when the schema file orjsonschemapackage is unavailable. Record explicit semantic errors for legacysemantic_modelwrappers and non-object roots so those inputs remain invalid when schema validation is skipped.Follow-up to #396
Checklist