fix: resolve recommendations #1-#4 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

* 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

Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>

Author: mwiebe

reports/model-bindings-quality-evaluation-report.md

@@ -450,16 +450,28 @@ established v0 idiom or a spec-promised behavior). Numbers are stable
 so future fix commits can resolve them with the
 `~~ ... ~~ **Resolved.**` strikethrough convention.
 
-1. **Empty-steps validation should raise `DecodeValidationError`, not
+1. ~~**Empty-steps validation should raise `DecodeValidationError`, not
    `ModelValidationError`** — for v0 parity. Either remap the
    empty-steps case in
    `rust-bindings/src/model/errors.rs::model_err_to_py` (or in the
    upstream Rust validator) so it surfaces under
    `PyDecodeValidationError`, or update the spec to call out the
    exception-class change explicitly. Pinned in
-   `test/openjd/model_v1/test_known_gaps.py::test_empty_steps_raises_decode_validation_error_like_v0`.
-
-2. **`JobTemplate.specification_version` should return a value
+   `test/openjd/model_v1/test_known_gaps.py::test_empty_steps_raises_decode_validation_error_like_v0`.~~
+   **Resolved** — documented in `specs/python-model-interface.md`
+   under "Exceptions" as a deliberate v0 divergence:
+   `DecodeValidationError` is reserved for schema-level (parse-stage)
+   failures the binding can detect before constructing the typed
+   model, and `ModelValidationError` is raised for everything caught
+   by the model validator (the at-least-one-step rule, parameter-
+   definition invariants, step-name uniqueness, etc.). Both classes
+   inherit `ValueError`, so callers catching `ValueError` are
+   unaffected; callers that distinguish should catch the tuple.
+   Regression test (with full message-body assertion per AGENTS.md
+   test quality standard) lives at
+   `test/openjd/model_v1/test_parse.py::TestDecodeJobTemplate::test_empty_steps_raises_model_validation_error`.
+
+2. ~~**`JobTemplate.specification_version` should return a value
    comparable to `v1.TemplateSpecificationVersion`** — today it
    returns the Rust pyo3 enum
    (`openjd._openjd_rs.TemplateSpecificationVersion`, variants
@@ -472,9 +484,26 @@ so future fix commits can resolve them with the
    shim); (b) drop the Python str-Enum shim and re-export the Rust
    pyclass under `v1.TemplateSpecificationVersion` (requires renaming
    variants for v0 parity — `JOBTEMPLATE_v2023_09` style). Pinned in
-   `test/openjd/model_v1/test_known_gaps.py::test_template_specification_version_comparable_to_python_str_enum`.
-
-3. **`merge_job_parameter_definitions` should emit `default` as the
+   `test/openjd/model_v1/test_known_gaps.py::test_template_specification_version_comparable_to_python_str_enum`.~~
+   **Resolved** — chose path (b). The Rust pyclass
+   `PyTemplateSpecificationVersion` in
+   `rust-bindings/src/model/types.rs` was reshaped to expose the v0
+   `str`-Enum surface directly: `#[pyo3(name = "JOBTEMPLATE_v2023_09")]`
+   / `ENVIRONMENT_v2023_09` variant names, a `.value` getter that
+   returns the spec-form string, a `__new__` that accepts either form,
+   `__eq__` / `__hash__` that compare equal (and hash equal) to the
+   spec-form string, and `module = "openjd._openjd_rs"` so its
+   canonical Python identity lives in the Rust module. The Python
+   wrapper `_v1/__init__.py` now imports `TemplateSpecificationVersion`
+   directly from `openjd._openjd_rs` (no shim). There is exactly one
+   class, identity-preserving across re-exports.
+   The same treatment was applied to `SpecificationRevision`.
+   The stale "str-Enum shim" paragraph in the spec's "Pickle Support"
+   section was also corrected to match. Regression test moved from
+   `test_known_gaps.py` to
+   `test/openjd/model_v1/test_version_enums.py::TestTemplateSpecificationVersion::test_template_specification_version_returned_from_decode`.
+
+3. ~~**`merge_job_parameter_definitions` should emit `default` as the
    parameter's native Python type, not as a string** — `int` for
    `INT`, `float` for `FLOAT`, `bool` for `BOOL`, `list[T]` for the
    `LIST[*]` variants, and `str` for `STRING` / `PATH` /
@@ -484,9 +513,30 @@ so future fix commits can resolve them with the
    `param_type` dispatch that converts the underlying typed value into
    the right Python type. Pinned in
    `test/openjd/model_v1/test_known_gaps.py::test_merge_default_int_returned_as_int`
-   and `test_merge_default_float_returned_as_float`.
-
-4. **`merge_job_parameter_definitions` should include the `description`
+   and `test_merge_default_float_returned_as_float`.~~
+   **Resolved** — added a `default_to_native` helper in
+   `rust-bindings/src/model/create_job_fns.rs` that dispatches on
+   `JobParameterType` and parses the upstream-stringified default
+   back into the native Python type. The upstream
+   `MergedParameterDefinition::default` is `Option<String>` (every
+   variant is round-tripped through `default_value()`'s stringifier),
+   so the binding now parses it back: `i64` for `INT`, `f64` for
+   `FLOAT`, `bool` for `BOOL`, `serde_json::from_str::<Vec<T>>` for
+   the `LIST[*]` variants, pass-through for `STRING` / `PATH` /
+   `RANGE_EXPR`. Parsing failures fall back to the raw string as a
+   defensive guard against future upstream serialization changes.
+   Existing parametrized tests in
+   `test/openjd/model_v1/test_merge_job_parameters.py::TestMergeTemplates_v2023_09`
+   updated to reflect native-type defaults (`"default": 8` instead of
+   `"default": "8"`, etc.). The two int/float gap tests were moved
+   out of `test_known_gaps.py` and expanded into a full per-type
+   coverage class `TestMergeDefaultNativeTypes` covering all 12 type
+   variants (INT, FLOAT, STRING, PATH, BOOL, LIST_INT, LIST_FLOAT,
+   LIST_STRING, LIST_BOOL, LIST_LIST_INT). Each assertion pins type
+   identity (`isinstance(..., int)`) as well as equality, since
+   `5 == 5.0` would otherwise let a regression slip through.
+
+4. ~~**`merge_job_parameter_definitions` should include the `description`
    field on each merged dict** — v0's typed defs carried it, and the
    upstream `openjd_model::merge_job_parameter_definitions` produces
    it on each input definition. The fix at the binding layer is to
@@ -495,7 +545,26 @@ so future fix commits can resolve them with the
    template or the relevant environment template, and add it to the
    merged dict when present. Update the spec's "Return shape" key
    list to include `description`. Pinned in
-   `test/openjd/model_v1/test_known_gaps.py::test_merge_includes_description`.
+   `test/openjd/model_v1/test_known_gaps.py::test_merge_includes_description`.~~
+   **Resolved** — chose path (a). The binding's
+   `py_merge_job_parameter_definitions` in
+   `rust-bindings/src/model/create_job_fns.rs` now builds a
+   `HashMap<&str, &str>` from `name → description` by walking the
+   same template sources the upstream merge walked (environment
+   templates in order, then the job template), with later
+   descriptions overwriting earlier ones — matching how the upstream
+   merge tracks `default`. The merged dict carries the `description`
+   key when at least one contributing template provided one, and
+   omits the key otherwise (consistent with how `default` /
+   `objectType` / `dataFlow` are conditionally emitted). The spec's
+   "Return shape" key list now lists `description` with its
+   semantics. Test moved out of `test_known_gaps.py` and expanded
+   into a full `TestMergeDescriptionPropagation` class covering five
+   cases: description from job template, description from
+   environment template, job-template-wins ordering, later-env-wins
+   ordering, and key-absent-when-no-description. `test_known_gaps.py`
+   is now empty of merge gaps (only the typing-imports leak
+   regression test remains).
 
 5. **Internal typing imports in `openjd.model._v1.__init__.py` should
    be prefixed with `_`** — `from typing import Any, Optional,

rust-bindings/src/model/create_job_fns.rs

@@ -8,6 +8,7 @@ use pyo3_stub_gen::derive::*;
 
 use openjd_model::template::EnvironmentTemplate;
 use openjd_model::types::JobParameterInputValues;
+use openjd_model::JobParameterType;
 use openjd_model::PathParameterOptions;
 
 use crate::expr::expr_value::py_to_expr_value;
@@ -188,13 +189,41 @@ pub(crate) fn py_merge_job_parameter_definitions(
     let merged = openjd_model::merge_job_parameter_definitions(&job_template.inner, &env_templates)
         .map_err(model_err_to_py)?;
 
+    // Collect descriptions by parameter name. The upstream
+    // `MergedParameterDefinition` struct does not carry a description
+    // field (only `name`, `param_type`, `default`, `object_type`,
+    // `data_flow`, `source`, and the merged constraint fields), so we
+    // recover it here by walking the same template sources the merge
+    // walked. To match how `default` is tracked (later template wins),
+    // we walk environment templates first (in order) then the job
+    // template last, overwriting on each set description we encounter.
+    let mut descriptions: std::collections::HashMap<&str, &str> = std::collections::HashMap::new();
+    for et in &env_templates {
+        if let Some(params) = &et.parameter_definitions {
+            for p in params {
+                if let Some(desc) = p.description() {
+                    descriptions.insert(p.name(), desc);
+                }
+            }
+        }
+    }
+    for p in job_template.inner.parameter_definitions_list() {
+        if let Some(desc) = p.description() {
+            descriptions.insert(p.name(), desc);
+        }
+    }
+
     let mut out = Vec::new();
     for m in &merged {
         let d = PyDict::new(py);
         d.set_item("name", &m.name)?;
         d.set_item("type", m.param_type.as_spec_str())?;
         if let Some(ref default) = m.default {
-            d.set_item("default", default)?;
+            let py_default = default_to_native(py, m.param_type, default)?;
+            d.set_item("default", py_default)?;
+        }
+        if let Some(desc) = descriptions.get(m.name.as_str()) {
+            d.set_item("description", *desc)?;
         }
         if let Some(ref ot) = m.object_type {
             d.set_item("objectType", ot.to_string())?;
@@ -208,6 +237,81 @@ pub(crate) fn py_merge_job_parameter_definitions(
     Ok(out)
 }
 
+/// Convert a stringified default value back into the native Python
+/// type for its parameter type.
+///
+/// The upstream Rust crate's `MergedParameterDefinition::default` is
+/// `Option<String>` — every variant is stringified through
+/// `JobParameterDefinition::default_value()` regardless of the
+/// underlying typed payload. The binding contract (per
+/// `specs/python-model-interface.md`) is that callers receive the
+/// default in its native Python type (`int` for `INT`, `float` for
+/// `FLOAT`, `list[T]` for `LIST[T]`, etc.). This helper parses the
+/// stringified form back into the right Python type.
+///
+/// `STRING` / `PATH` / `RANGE_EXPR` defaults pass through unchanged
+/// (they are already strings). `LIST[*]` defaults are JSON-serialised
+/// by the upstream `default_value()` implementation, so they round-trip
+/// through `serde_json`.
+///
+/// If parsing fails for any reason (defensive fallback against future
+/// upstream serialization changes), the original string is returned
+/// so the caller never sees a `None` where v0 would have produced a
+/// default.
+fn default_to_native(
+    py: Python<'_>,
+    param_type: JobParameterType,
+    raw: &str,
+) -> PyResult<Py<pyo3::PyAny>> {
+    use pyo3::IntoPyObjectExt;
+    match param_type {
+        JobParameterType::String | JobParameterType::Path | JobParameterType::RangeExpr => {
+            raw.into_py_any(py)
+        }
+        JobParameterType::Int => raw
+            .parse::<i64>()
+            .ok()
+            .map(|v| v.into_py_any(py))
+            .unwrap_or_else(|| raw.into_py_any(py)),
+        JobParameterType::Float => raw
+            .parse::<f64>()
+            .ok()
+            .map(|v| v.into_py_any(py))
+            .unwrap_or_else(|| raw.into_py_any(py)),
+        JobParameterType::Bool => match raw {
+            "true" => true.into_py_any(py),
+            "false" => false.into_py_any(py),
+            _ => raw.into_py_any(py),
+        },
+        JobParameterType::ListString | JobParameterType::ListPath => {
+            serde_json::from_str::<Vec<String>>(raw)
+                .ok()
+                .map(|v| v.into_py_any(py))
+                .unwrap_or_else(|| raw.into_py_any(py))
+        }
+        JobParameterType::ListInt => serde_json::from_str::<Vec<i64>>(raw)
+            .ok()
+            .map(|v| v.into_py_any(py))
+            .unwrap_or_else(|| raw.into_py_any(py)),
+        JobParameterType::ListFloat => serde_json::from_str::<Vec<f64>>(raw)
+            .ok()
+            .map(|v| v.into_py_any(py))
+            .unwrap_or_else(|| raw.into_py_any(py)),
+        JobParameterType::ListBool => serde_json::from_str::<Vec<bool>>(raw)
+            .ok()
+            .map(|v| v.into_py_any(py))
+            .unwrap_or_else(|| raw.into_py_any(py)),
+        JobParameterType::ListListInt => serde_json::from_str::<Vec<Vec<i64>>>(raw)
+            .ok()
+            .map(|v| v.into_py_any(py))
+            .unwrap_or_else(|| raw.into_py_any(py)),
+        // ``JobParameterType`` is ``#[non_exhaustive]`` — any future
+        // variant we don't know about falls back to the raw string so
+        // callers see *some* value rather than nothing.
+        _ => raw.into_py_any(py),
+    }
+}
+
 #[cfg_attr(
     feature = "stub-gen",
     gen_stub_pyfunction(module = "openjd._openjd_rs")

specs/python-model-interface.md

@@ -324,6 +324,13 @@ following keys:
   default) — the default value, in its native Python type
   (``int`` / ``float`` / ``str`` / ``list`` / …) per the
   parameter's type.
+* ``description`` (``str``, present only if at least one
+  contributing template provided one) — the human-readable
+  description copied from the originating template. When more
+  than one contributing template defines a description for the
+  same parameter, the description from the template walked last
+  wins (environment templates are walked in order, then the job
+  template), mirroring how ``default`` is tracked.
 * ``objectType`` (``str``, present only for ``PATH`` parameters
   with an ``objectType`` declared) — ``"FILE"`` or
   ``"DIRECTORY"``.
@@ -342,7 +349,9 @@ values" pass. The underlying Rust crate's
 ``openjd_model::merge_job_parameter_definitions`` returns a
 struct with ``source`` / ``name`` / ``param_type`` / ``default`` /
 ``object_type`` / ``data_flow`` fields; the binding flattens
-that struct into the dict above.
+that struct into the dict above and additionally recovers
+``description`` by walking the same template sources the merge
+walked (the upstream struct does not carry a description field).
 
 #### `evaluate_let_bindings`
 
@@ -1386,23 +1395,31 @@ Practical consequences:
 
 ```python
 from openjd.model._v1 import decode_job_template
-from openjd.model._v1.errors import DecodeValidationError
+from openjd.model._v1.errors import DecodeValidationError, ModelValidationError
 
-# Invalid template
+# Invalid template — schema-level failure (missing required top-level
+# key) raises DecodeValidationError. These are the failures the
+# binding can detect before the dict is reshaped into the typed
+# model, so they don't need to run through the model validator.
 try:
     decode_job_template(template={"bad": "template"})
 except DecodeValidationError as e:
     str(e)  # "Template is missing Open Job Description schema version key: specificationVersion"
 
-# Empty steps
+# Empty steps — model-level structural failure raises
+# ModelValidationError. The shape parses successfully but the
+# decoded JobTemplate fails the "at least one step" invariant when
+# the model validator runs, so the failure surfaces under
+# ModelValidationError rather than DecodeValidationError. See the
+# divergence note below.
 try:
     decode_job_template(template={
         "specificationVersion": "jobtemplate-2023-09",
         "name": "Test",
         "steps": [],
     })
-except DecodeValidationError as e:
-    str(e)  # validation error about empty steps
+except ModelValidationError as e:
+    str(e)  # "1 validation error for JobTemplate\nJobTemplate: must have at least one step."
 ```
 
 | Exception | Base |
@@ -1412,6 +1429,28 @@ except DecodeValidationError as e:
 | `UnsupportedSchema` | `ValueError` |
 | `ExpressionError` | `ValueError` |
 
+**`DecodeValidationError` vs `ModelValidationError` divergence from v0.**
+The v0 (Pydantic) reference raised ``DecodeValidationError`` for *every*
+template-decode failure — including model-level structural invariants
+like "must have at least one step" — because pydantic's discriminated-
+union dispatch ran the field validators inside the decode call. The v1
+binding splits the two phases: ``DecodeValidationError`` is raised
+strictly for schema-level failures the binding can detect before
+constructing the typed model (missing/unknown ``specificationVersion``,
+unknown fields under strict mode, unparseable JSON/YAML, etc.), and
+``ModelValidationError`` is raised for everything caught by the model
+validator that runs once the decoded shape is in hand (the at-least-one-
+step rule, parameter-definition invariants, step-name uniqueness, etc.).
+Both classes inherit ``ValueError``, so callers that catch ``ValueError``
+are unaffected — only callers that distinguish between the two classes
+need to be aware of the split. The v1 binding **deliberately** does not
+remap empty-steps to ``DecodeValidationError`` for v0 byte-parity: the
+v1 model validator is the right home for the check (it surfaces with the
+same path-prefixed message shape as every other model-level invariant),
+and callers wanting to catch every decode-time failure should catch
+``ValueError`` or both classes by tuple (``except (DecodeValidationError,
+ModelValidationError)``).
+
 **`UnsupportedSchema` constructor divergence from v0.** The v0
 reference defines ``UnsupportedSchema(version_str)`` such that
 ``str(e) == "Unsupported schema version: {version_str}"`` and the
@@ -1472,12 +1511,15 @@ original.
 | ``PathTaskParameter`` | constructor argument (``range``) |
 | ``ChunkIntTaskParameter`` | constructor arguments (``range``, ``chunks``) |
 | ``TaskChunksDefinition`` | constructor arguments (three fields) |
+| ``SpecificationRevision`` | variant name (``v2023_09``) |
+| ``TemplateSpecificationVersion`` | variant name (``JOBTEMPLATE_v2023_09``, ``ENVIRONMENT_v2023_09``) |
 | ``DecodeValidationError``, ``ModelValidationError``, ``UnsupportedSchema`` | standard exception pickle, under their canonical ``openjd.model._v1`` module path |
 
-``SpecificationRevision`` and ``TemplateSpecificationVersion`` pickle
-through the Python ``str``-Enum shims provided by ``openjd.model._v1``
-(the underlying Rust pyclasses live at ``openjd._openjd_rs`` and pickle
-correctly there too).
+``SpecificationRevision`` and ``TemplateSpecificationVersion`` are Rust
+pyclass enums whose canonical home is ``openjd._openjd_rs``; they are
+re-exported from ``openjd.model._v1`` (identity-preserving — there is
+exactly one class in each case). Pickled instances round-trip through
+the variant name via the module-level ``_reconstruct_enum`` helper.
 
 The decoded model containers (``JobTemplate``, ``EnvironmentTemplate``,
 ``Job``, ``Step``, etc.) and the live ``StepParameterSpaceIterator`` /