Skip to content

fix: resolve recommendations #1-#4 from model-bindings quality report#304

Merged
mwiebe merged 1 commit into
OpenJobDescription:mainlinefrom
mwiebe:report-fixes
Jul 3, 2026
Merged

fix: resolve recommendations #1-#4 from model-bindings quality report#304
mwiebe merged 1 commit into
OpenJobDescription:mainlinefrom
mwiebe:report-fixes

Conversation

@mwiebe

@mwiebe mwiebe commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

What was the problem/requirement? (What/Why)

Agent-generated report found issues to fix up.

What was the solution? (How)

Addresses the first four recommendations in
reports/model-bindings-quality-evaluation-report.md. Each item is struck through in the report with a **Resolved** note.

#1 — Document empty-steps decode error class divergence (docs)

Empty-steps decode failures surface under ModelValidationError rather than v0's DecodeValidationError. The v1 binding splits decode-stage (schema-level) failures from model-stage (structural-invariant) failures, with the empty-steps check living in the model validator — this is a deliberate v0 divergence, not a bug.

  • spec's 'Exceptions' section updated: corrected empty-steps example, divergence note explaining the split and recommended catch-tuple
  • strengthened the existing regression test in test_parse.py to assert the full message body per the AGENTS.md test quality standard

#2 — Relocate resolved TemplateSpecificationVersion gap test (test)

The Python str-Enum shim for TemplateSpecificationVersion was already removed in a prior change; the Rust pyclass at openjd._openjd_rs.TemplateSpecificationVersion is the single canonical class, re-exported identity-preserving from openjd.model._v1. The regression test had become passing (no xfail marker) but was still parked in test_known_gaps.py.

  • test moved to test_version_enums.py::TestTemplateSpecificationVersion ::test_template_specification_version_returned_from_decode with strengthened identity / value / string-equality assertions
  • spec's 'Pickle Support' section: stale 'str-Enum shim' paragraph corrected — pickle goes directly through the module-level _reconstruct_enum helper; pickle table now lists both spec-revision enums explicitly

#3 — Emit merge defaults in native Python types (fix)

The upstream MergedParameterDefinition::default is Option<String> — every variant is stringified through default_value(). The binding contract per the spec is that callers receive the default in its native Python type (int for INT, float for FLOAT, list[T] for LIST[T], etc.).

  • new default_to_native helper in create_job_fns.rs dispatches on JobParameterType and parses the stringified default back via str::parse / serde_json::from_str; parsing failures fall back to the raw string as a defensive guard
  • pre-existing parametrized expectations in test_merge_job_parameters.py::TestMergeTemplates_v2023_09 updated to native form ('default': 8 vs 'default': '8')
  • two int/float xfail tests moved out of test_known_gaps.py and expanded into a full TestMergeDefaultNativeTypes class covering all 10 type variants with type-identity assertions

#4 — Propagate parameter description through merge (feat)

The upstream MergedParameterDefinition struct does not surface a description field — only name / param_type / default / object_type / data_flow / source / merged constraints. v0 carried it on the typed pyclass merged result, so v1 was losing per-parameter human-readable text that downstream tooling (deadline-cli's parameter-prompt UI) relies on for parameter labels.

  • py_merge_job_parameter_definitions now walks env templates in order then the job template, building a name→description HashMap. Later descriptions overwrite earlier ones, matching how the upstream merge tracks default
  • the merged dict carries the description key only when at least one contributing template provided one (consistent with how default / objectType / dataFlow are conditionally emitted)
  • spec's 'Return shape' key list now lists description with its last-wins ordering semantics
  • xfail test moved out of test_known_gaps.py and expanded into a 5-case TestMergeDescriptionPropagation class

Verification

  • python -m pytest test/openjd/model_v0 test/openjd/model_v1 → 3269 passed (+5 from the new tests), 0 xfails
  • python -m pytest test/ → 5131 passed, 24 skipped, 3 unrelated expr xfails
  • cargo clippy --manifest-path rust-bindings/Cargo.toml --all-targets -- -D warnings → clean
  • No public binding signatures changed; _openjd_rs.pyi unchanged

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mwiebe
mwiebe requested a review from a team as a code owner June 30, 2026 22:37
@mwiebe
mwiebe force-pushed the report-fixes branch 3 times, most recently from 7efe146 to 1261ab1 Compare June 30, 2026 22:51
jericht
jericht previously approved these changes Jun 30, 2026
…from model-bindings quality report

Addresses the first four recommendations in
`reports/model-bindings-quality-evaluation-report.md`. Each item is
struck through in the report with a `**Resolved**` note.

Empty-steps decode failures surface under `ModelValidationError` rather
than v0's `DecodeValidationError`. The v1 binding splits decode-stage
(schema-level) failures from model-stage (structural-invariant)
failures, with the empty-steps check living in the model validator —
this is a deliberate v0 divergence, not a bug.

* spec's 'Exceptions' section updated: corrected empty-steps example,
  divergence note explaining the split and recommended catch-tuple
* strengthened the existing regression test in test_parse.py to assert
  the full message body per the AGENTS.md test quality standard

The Python str-Enum shim for `TemplateSpecificationVersion` was
already removed in a prior change; the Rust pyclass at
`openjd._openjd_rs.TemplateSpecificationVersion` is the single
canonical class, re-exported identity-preserving from
`openjd.model._v1`. The regression test had become passing (no
`xfail` marker) but was still parked in `test_known_gaps.py`.

* test moved to test_version_enums.py::TestTemplateSpecificationVersion
  ::test_template_specification_version_returned_from_decode with
  strengthened identity / value / string-equality assertions
* spec's 'Pickle Support' section: stale 'str-Enum shim' paragraph
  corrected — pickle goes directly through the module-level
  `_reconstruct_enum` helper; pickle table now lists both
  spec-revision enums explicitly

The upstream `MergedParameterDefinition::default` is `Option<String>` —
every variant is stringified through `default_value()`. The binding
contract per the spec is that callers receive the default in its
native Python type (`int` for INT, `float` for FLOAT, `list[T]` for
LIST[T], etc.).

* new `default_to_native` helper in create_job_fns.rs dispatches on
  `JobParameterType` and parses the stringified default back via
  `str::parse` / `serde_json::from_str`; parsing failures fall back
  to the raw string as a defensive guard
* pre-existing parametrized expectations in
  test_merge_job_parameters.py::TestMergeTemplates_v2023_09 updated
  to native form (`'default': 8` vs `'default': '8'`)
* two int/float xfail tests moved out of test_known_gaps.py and
  expanded into a full TestMergeDefaultNativeTypes class covering
  all 10 type variants with type-identity assertions

The upstream `MergedParameterDefinition` struct does not surface a
description field — only `name` / `param_type` / `default` /
`object_type` / `data_flow` / `source` / merged constraints. v0
carried it on the typed pyclass merged result, so v1 was losing
per-parameter human-readable text that downstream tooling
(deadline-cli's parameter-prompt UI) relies on for parameter labels.

* py_merge_job_parameter_definitions now walks env templates in order
  then the job template, building a name→description `HashMap`. Later
  descriptions overwrite earlier ones, matching how the upstream merge
  tracks `default`
* the merged dict carries the `description` key only when at least
  one contributing template provided one (consistent with how
  `default` / `objectType` / `dataFlow` are conditionally emitted)
* spec's 'Return shape' key list now lists `description` with its
  last-wins ordering semantics
* xfail test moved out of test_known_gaps.py and expanded into a
  5-case TestMergeDescriptionPropagation class

Also refreshes `THIRD-PARTY-LICENSES.txt` for a Python-side dep bump
(`typing_extensions` 4.15.0 → 4.16.0) so the
`third_party_licenses` CI job stays green. Regenerated via
`bash scripts/check_third_party_licenses.sh --update`.

* python -m pytest test/openjd/model_v0 test/openjd/model_v1 → 3269
  passed (+5 from the new tests), 0 xfails
* python -m pytest test/ → 5131 passed, 24 skipped, 3 unrelated
  `expr` xfails
* cargo fmt --check → clean
* cargo clippy --all-targets -- -D warnings → clean
* cargo-deny check licenses bans sources → ok
* THIRD-PARTY-LICENSES.txt sync check → up to date
* No public binding signatures changed; _openjd_rs.pyi unchanged

Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
@mwiebe
mwiebe merged commit 8f1f9f1 into OpenJobDescription:mainline Jul 3, 2026
30 checks passed
@mwiebe
mwiebe deleted the report-fixes branch July 3, 2026 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants