From 007f212aabfd225c74825d0cc7add439445bcfae Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:29:45 -0700 Subject: [PATCH 1/6] chore(deps): Bump openjd-* Rust crates to the 0.6.0 release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the bindings crate onto the openjd-rs release published by OpenJobDescription/openjd-rs#357: openjd-expr 0.5.0 -> 0.6.0 (breaking) openjd-model 0.5.4 -> 0.6.0 (breaking) openjd-sessions 0.5.4 -> 0.5.5 The breaking part of both minor bumps is openjd-rs#354, "Keep the decimal places of a floatstring range element", the Rust counterpart of #345 on this side. `TaskParameter::Float` now carries `Vec` rather than `Vec`, because a `` range element has to keep the scale it was written with: '02.50' renders `2.50`, not `2.5` (Template Schemas §7.5). That reaches Python through `TaskParameterValue`, which renders the preserved spelling verbatim, so the per-task value a command line receives now matches the pure-Python reference. `FloatTaskParameter.range` stays `list[float]` -- it is the numeric introspection view of the resolved definition -- so the conversion takes `Float64::value()` there. openjd-model 0.6.0 also carries openjd-rs#355 (two chunking parity gaps) and openjd-rs#358 (the 512-character cap on a let binding identifier), and openjd-sessions 0.5.5 carries openjd-rs#361 (persist a resolved symbol table supplied by argument). Verified: cargo build --all-targets, cargo clippy --all-targets -D warnings, cargo test and cargo test --doc all pass. The Python suite is 5546 passed with 5 failures that are all gap markers this release closes; they are addressed in the following commit. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- Cargo.lock | 12 ++++++------ rust-bindings/Cargo.toml | 6 +++--- rust-bindings/src/model/task_parameter.rs | 6 +++++- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 14bdc543..9deba2a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -679,9 +679,9 @@ checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" [[package]] name = "openjd-expr" -version = "0.5.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e20c50bf19d788b8b491e87cecf7e3e0ea312ffcd17cfb2e2240ff1e64c49887" +checksum = "a05a6e060a42d2b5f681ade212f38482c5fadd460fd73dbc12ebf964828bc290" dependencies = [ "regex", "regex-syntax", @@ -696,9 +696,9 @@ dependencies = [ [[package]] name = "openjd-model" -version = "0.5.4" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "29cc8faeffd5da33c6e47c7eaff87169d9ed275292c894f950d215d99d5bfb97" +checksum = "62f9668a4b5663ce7897d34c4d3294b214b499c84360dc88ce2d2dedc444fad3" dependencies = [ "indexmap", "openjd-expr", @@ -728,9 +728,9 @@ dependencies = [ [[package]] name = "openjd-sessions" -version = "0.5.4" +version = "0.5.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4fe33608faf28b7ef2449814ad9040605775cbf5a5f7c3d30e53fcd2842b1aff" +checksum = "ef2f96d8fe1e21d255c34cd466d4619d4f809e5105e01fc6f03463a34194fe2c" dependencies = [ "bitflags", "futures-util", diff --git a/rust-bindings/Cargo.toml b/rust-bindings/Cargo.toml index e87cad1b..3142af02 100644 --- a/rust-bindings/Cargo.toml +++ b/rust-bindings/Cargo.toml @@ -12,9 +12,9 @@ name = "_openjd_rs" crate-type = ["cdylib", "rlib"] [dependencies] -openjd-expr = "0.5.0" -openjd-model = "0.5.4" -openjd-sessions = "0.5.4" +openjd-expr = "0.6.0" +openjd-model = "0.6.0" +openjd-sessions = "0.5.5" tokio = { version = "1", features = ["rt-multi-thread"] } uuid = { version = "1", features = ["v4"] } serde_json = "1" diff --git a/rust-bindings/src/model/task_parameter.rs b/rust-bindings/src/model/task_parameter.rs index 28ff4c75..57cc1868 100644 --- a/rust-bindings/src/model/task_parameter.rs +++ b/rust-bindings/src/model/task_parameter.rs @@ -548,8 +548,12 @@ pub(crate) fn task_parameter_to_py<'py>( } .into_bound_py_any(py) } + // `Float64` carries the spelling a `` range element was + // written with (§7.5). It reaches a command line through + // `TaskParameterValue`, which renders it verbatim; this getter is the + // numeric introspection view, so take the value and drop the spelling. TaskParameter::Float { range } => PyFloatTaskParameter { - range: range.clone(), + range: range.iter().map(|f| f.value()).collect(), } .into_bound_py_any(py), TaskParameter::String { range } => PyStringTaskParameter { From e5b9b2c5daec37abf50bb1aaecdddab2ad8ddf53 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:42:39 -0700 Subject: [PATCH 2/6] test: Promote the two chunking gaps openjd-model 0.6.0 closes openjd-rs#355 closed both divergences that `test/openjd/model_v1/test_known_gaps.py` recorded as strict xfails, so with `xfail_strict = true` they now fail as xpasses. That file's own rule is to promote a resolved gap to its proper home rather than drop the marker in place, so both move to `test_step_param_space_iter.py` beside the rest of `TestChunksTaskCountOverride`. - A CONTIGUOUS chunked space supports random access. `it[0]`, `it[1]` and `it[-1]` answer, indexing observes `chunks_task_count_override`, and one past the end is still an IndexError. This replaces `test_a_contiguous_space_refuses_indexing_with_or_without_the_override`, which asserted the refusal and named this exact swap as its counterpart. - `chunks_parameter_name` and `chunks_default_task_count` report for any chunked space, not only an adaptive one, which is what v0 has always done. Added `test_an_adaptive_space_still_refuses_indexing` as the negative control and the remaining limitation: an adaptive space has no knowable count, so `len()` raises ValueError and every index is out of range, while iteration still yields. Measured, along with everything asserted above, against openjd-model 0.6.0 before the assertions were written. Both promoted assertions are falsifiable by the version alone: they were strict xfails passing on mainline at openjd-model 0.5.4, so they failed there, and the CI run on 007f212 reports them xpassing at 0.6.0. `specs/python-model-interface.md` claimed both limitations and pointed at the xfails by name; it now states the random access that works and the one adaptive limitation that remains. The `Optional[str]`/`Optional[int]` signatures are unchanged -- both getters still answer None for a space that is not chunked. Verified: 5551 passed, 24 skipped, 3 xfailed with the 94% coverage gate enforced; ruff, black and mypy clean. `test_known_gaps.py` is now down to one test, which is a passing regression test rather than a gap. Left where it is rather than widen this change. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- specs/python-model-interface.md | 24 ++-- test/openjd/model_v1/test_known_gaps.py | 120 ------------------ .../model_v1/test_step_param_space_iter.py | 86 +++++++++++-- 3 files changed, 82 insertions(+), 148 deletions(-) diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index 3150975b..29da99f7 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -1176,20 +1176,16 @@ least 1, so 0 would otherwise silently mean 1, and the `chunks_default_task_count` setter already rejects it. The pure-Python reference does not validate this argument. -Two current-implementation limitations, both divergences from the v0 -reference rather than intended behaviour. Each has a `strict` xfail in -`test/openjd/model_v1/test_known_gaps.py`, so clearing either will fail -CI until this text is updated with it. - -- Indexing observes the override only for a space that supports random - access. A `CONTIGUOUS` chunked space requires sequential iteration, so - `it[i]` raises `IndexError` for any index — with or without the - override — even though `len(it)` reports a count. See - `test_a_contiguous_chunked_space_supports_indexing`. -- `chunks_parameter_name` and `chunks_default_task_count` both return - `None` once the space is non-adaptive, which supplying the override - makes it. v0 reports the parameter name and the override value. See - `test_chunk_metadata_is_reported_for_a_non_adaptive_space`. +Indexing observes the override. `openjd-model` 0.6.0 gave a `CONTIGUOUS` +chunked space random access, so `it[i]` answers there as it does for a +`NONCONTIGUOUS` one, and reports the overridden granularity rather than +the template's. + +One current-implementation limitation remains. An *adaptive* space has no +knowable count until it is walked, so `len(it)` raises `ValueError` and +every index — including `-1` — raises `IndexError`. Iteration still +yields, which is what distinguishes an unknown count from an empty space. +Supplying the override makes the space static and lifts both. ### `StepDependencyGraph` diff --git a/test/openjd/model_v1/test_known_gaps.py b/test/openjd/model_v1/test_known_gaps.py index bbc50c6b..3cb9c745 100644 --- a/test/openjd/model_v1/test_known_gaps.py +++ b/test/openjd/model_v1/test_known_gaps.py @@ -19,8 +19,6 @@ from __future__ import annotations -from typing import Any - # ── Top-level package no longer leaks typing imports ── # # An earlier draft of ``openjd.model._v1`` imported ``Any``, @@ -43,121 +41,3 @@ def test_no_internal_imports_leak_at_top_level(name: str) -> None: import openjd.model._v1 as v1 assert not hasattr(v1, name), f"{name} leaks as a public attribute on openjd.model._v1" - - -# ── Chunked parameter spaces: two divergences from the v0 reference ── -# -# Found while adding `chunks_task_count_override` to -# `StepParameterSpaceIterator`. Neither is caused by that argument — both -# reproduce without it — so they are recorded here rather than fixed in -# passing. - - -def _chunked_step(constraint: str) -> Any: - from openjd.model._v1 import create_job, decode_job_template - - template = { - "specificationVersion": "jobtemplate-2023-09", - "name": "T", - "extensions": ["TASK_CHUNKING"], - "steps": [ - { - "name": "S", - "parameterSpace": { - "taskParameterDefinitions": [ - { - "name": "Frame", - "type": "CHUNK[INT]", - "range": "1-10", - "chunks": {"defaultTaskCount": 5, "rangeConstraint": constraint}, - } - ] - }, - "script": { - "actions": {"onRun": {"command": "echo", "args": ["{{Task.Param.Frame}}"]}} - }, - } - ], - } - job_template = decode_job_template(template=template, supported_extensions=["TASK_CHUNKING"]) - return create_job(job_template=job_template, job_parameter_values={}).steps[0] - - -@pytest.mark.xfail( - reason="v1 derives chunks_parameter_name and chunks_default_task_count from adaptive " - "detection, so both are None for any non-adaptive chunked space. v0 reports them for " - "any chunked space. See openjd-model step_param_space.rs: chunks_param_name and " - "adaptive_chunk_size are both built from adaptive_info.", - strict=True, -) -@pytest.mark.parametrize( - "chunks,override,expected_count", - [ - # Statically chunked: no override involved, v0 reports the template's size. - ({"defaultTaskCount": 5, "rangeConstraint": "CONTIGUOUS"}, None, 5), - # Statically chunked, re-chunked by the override. - ({"defaultTaskCount": 5, "rangeConstraint": "CONTIGUOUS"}, 1, 1), - # Adaptive, turned static by the override. v0 reports the override as the size. - ( - {"defaultTaskCount": 5, "targetRuntimeSeconds": 60, "rangeConstraint": "CONTIGUOUS"}, - 1, - 1, - ), - ], - ids=["static", "static-overridden", "adaptive-overridden"], -) -def test_chunk_metadata_is_reported_for_a_non_adaptive_space( - chunks: dict, override: int | None, expected_count: int -) -> None: - """v0 returns ``"Frame"`` and the chunk size for each of these. v1 returns ``None``. - - Neither value is unknowable — both are in the template, or are the override the caller - just passed — so a consumer inspecting a non-adaptive chunked space through v1 cannot - learn which parameter chunks, or at what size. One root cause, three ways to reach it: - anything that leaves the space non-adaptive drops both getters. - """ - from openjd.model._v1 import create_job, decode_job_template - from openjd.model._v1.job import StepParameterSpaceIterator - - template = { - "specificationVersion": "jobtemplate-2023-09", - "name": "T", - "extensions": ["TASK_CHUNKING"], - "steps": [ - { - "name": "S", - "parameterSpace": { - "taskParameterDefinitions": [ - {"name": "Frame", "type": "CHUNK[INT]", "range": "1-10", "chunks": chunks} - ] - }, - "script": { - "actions": {"onRun": {"command": "echo", "args": ["{{Task.Param.Frame}}"]}} - }, - } - ], - } - job_template = decode_job_template(template=template, supported_extensions=["TASK_CHUNKING"]) - step = create_job(job_template=job_template, job_parameter_values={}).steps[0] - - it = StepParameterSpaceIterator(step=step, chunks_task_count_override=override) - assert it.chunks_adaptive is False - assert it.chunks_parameter_name == "Frame" - assert it.chunks_default_task_count == expected_count - - -@pytest.mark.xfail( - reason="v1 refuses random access whenever the space needs sequential iteration, and " - "contiguous chunking always does. v0 supports indexing the same space.", - strict=True, -) -def test_a_contiguous_chunked_space_supports_indexing() -> None: - """v0 answers ``it[0]`` with ``1-5``. v1 raises ``IndexError``. - - ``len()`` works on this space, so the count is known; only ``get`` declines. - """ - from openjd.model._v1.job import StepParameterSpaceIterator - - it = StepParameterSpaceIterator(step=_chunked_step("CONTIGUOUS")) - assert len(it) == 2 - assert it[0]["Frame"].value == "1-5" diff --git a/test/openjd/model_v1/test_step_param_space_iter.py b/test/openjd/model_v1/test_step_param_space_iter.py index acb233a0..03cd0478 100644 --- a/test/openjd/model_v1/test_step_param_space_iter.py +++ b/test/openjd/model_v1/test_step_param_space_iter.py @@ -14,7 +14,7 @@ ``{type, range, [chunks]}`` matching the YAML/JSON template syntax. """ -from typing import Any, Callable +from typing import Any, Callable, Optional import pytest @@ -741,21 +741,79 @@ def test_indexing_observes_the_override(self) -> None: assert fresh[9]["Frame"].value == yielded[-1] assert fresh[-1]["Frame"].value == yielded[-1] - def test_a_contiguous_space_refuses_indexing_with_or_without_the_override(self) -> None: - """Pre-existing behaviour the override does not change: openjd-model requires - sequential iteration for contiguous chunking, so ``get`` always declines. - Pinned here so a future change to random access is a deliberate one. Its - counterpart is ``test_known_gaps.py::test_a_contiguous_chunked_space_supports_indexing``, - a strict xfail asserting the opposite: closing that gap fails there as an xpass - *and* here as a hard assertion, so both move together, along with the - limitation noted in ``specs/python-model-interface.md``.""" - for override in (None, 1): - it = StepParameterSpaceIterator( - step=self._step(self._STATIC), chunks_task_count_override=override - ) + def test_a_contiguous_space_supports_indexing_with_or_without_the_override(self) -> None: + """openjd-model 0.6.0 (openjd-rs#355) gave a contiguous chunked space random + access, so ``get`` now answers where it used to decline. Promoted from + ``test_known_gaps.py::test_a_contiguous_chunked_space_supports_indexing``, + which pinned the v0 reading as a strict xfail. + + Indexing observes the override, same as the NONCONTIGUOUS case above, and the + end of the space is still an ``IndexError``. + """ + it = StepParameterSpaceIterator(step=self._step(self._STATIC)) + assert [it[0]["Frame"].value, it[1]["Frame"].value, it[-1]["Frame"].value] == [ + "1-5", + "6-10", + "6-10", + ] + with pytest.raises(IndexError) as excinfo: + _ = it[2] + assert str(excinfo.value) == "index out of range" + + overridden = StepParameterSpaceIterator( + step=self._step(self._STATIC), chunks_task_count_override=1 + ) + assert [overridden[0]["Frame"].value, overridden[-1]["Frame"].value] == ["1-1", "10-10"] + + def test_an_adaptive_space_still_refuses_indexing(self) -> None: + """The negative control for the test above, and the remaining limitation + recorded in ``specs/python-model-interface.md``: an adaptive space has no + knowable count, so ``len()`` raises and every index is out of range. Iteration + still yields, which is what distinguishes "unknown count" from "empty".""" + it = StepParameterSpaceIterator(step=self._step(self._ADAPTIVE)) + assert it.chunks_adaptive is True + for index in (0, -1): with pytest.raises(IndexError) as excinfo: - _ = it[0] + _ = it[index] assert str(excinfo.value) == "index out of range" + assert self._frames(StepParameterSpaceIterator(step=self._step(self._ADAPTIVE))) == [ + "1-5", + "6-10", + ] + + @pytest.mark.parametrize( + "chunks,override,expected_count", + [ + # Statically chunked: no override involved, the template's own size. + ({"defaultTaskCount": 5, "rangeConstraint": "CONTIGUOUS"}, None, 5), + # Statically chunked, re-chunked by the override. + ({"defaultTaskCount": 5, "rangeConstraint": "CONTIGUOUS"}, 1, 1), + # Adaptive, turned static by the override, which becomes the size. + ( + { + "defaultTaskCount": 5, + "targetRuntimeSeconds": 60, + "rangeConstraint": "CONTIGUOUS", + }, + 1, + 1, + ), + ], + ids=["static", "static-overridden", "adaptive-overridden"], + ) + def test_chunk_metadata_is_reported_for_a_non_adaptive_space( + self, chunks: dict[str, Any], override: Optional[int], expected_count: int + ) -> None: + """openjd-model 0.6.0 (openjd-rs#355) reports ``chunks_parameter_name`` and + ``chunks_default_task_count`` for any chunked space, not only an adaptive one, + which is what the v0 reference has always done. Promoted from + ``test_known_gaps.py``, where it was a strict xfail.""" + it = StepParameterSpaceIterator( + step=self._step(chunks), chunks_task_count_override=override + ) + assert it.chunks_adaptive is False + assert it.chunks_parameter_name == "Frame" + assert it.chunks_default_task_count == expected_count def test_an_intermediate_override_regroups_the_chunks(self) -> None: """Not just 1: any positive size regroups the space.""" From bfa28bc9649f18dbba99e500184e6caf5755a6db Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:42:50 -0700 Subject: [PATCH 3/6] chore: Regenerate THIRD-PARTY-LICENSES for the openjd-* 0.6.0 bump `scripts/check_third_party_licenses.sh` fails on a Cargo.lock change alone, and the three crate bumps are exactly what the diff contains: openjd-expr 0.5.0 -> 0.6.0, openjd-model 0.5.4 -> 0.6.0, openjd-sessions 0.5.4 -> 0.5.5. No other line moves, and no transitive dependency changed. Regenerating it needed a portability fix first. `sed -i 's/\r//'` on the EOL normalization line is GNU-only: BSD sed reads the next argument as the backup suffix, so on macOS the script died with `sed: 1: "/var/folders/...": invalid command code f` before writing anything. Rewriting through a temp file behaves identically on both. The failure was not specific to this change -- the script could not be run on macOS at all -- and CI regenerates with the same script, so the committed file and the check stay in agreement. Verified: `scripts/check_third_party_licenses.sh --update` then `scripts/check_third_party_licenses.sh` reports the file up to date, with cargo-about 0.9.2, the version CI installs. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- THIRD-PARTY-LICENSES.txt | 6 +++--- scripts/check_third_party_licenses.sh | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/THIRD-PARTY-LICENSES.txt b/THIRD-PARTY-LICENSES.txt index 10c7af66..1137dde5 100644 --- a/THIRD-PARTY-LICENSES.txt +++ b/THIRD-PARTY-LICENSES.txt @@ -2534,9 +2534,9 @@ limitations under the License. ** itoa; version 1.0.18 -- https://crates.io/crates/itoa ** libc; version 0.2.189 -- https://crates.io/crates/libc ** manyhow-macros; version 0.11.4 -- https://crates.io/crates/manyhow-macros -** openjd-expr; version 0.5.0 -- https://crates.io/crates/openjd-expr -** openjd-model; version 0.5.4 -- https://crates.io/crates/openjd-model -** openjd-sessions; version 0.5.4 -- https://crates.io/crates/openjd-sessions +** openjd-expr; version 0.6.0 -- https://crates.io/crates/openjd-expr +** openjd-model; version 0.6.0 -- https://crates.io/crates/openjd-model +** openjd-sessions; version 0.5.5 -- https://crates.io/crates/openjd-sessions ** pin-project-lite; version 0.2.17 -- https://crates.io/crates/pin-project-lite ** portable-atomic; version 1.15.0 -- https://crates.io/crates/portable-atomic ** proc-macro2; version 1.0.107 -- https://crates.io/crates/proc-macro2 diff --git a/scripts/check_third_party_licenses.sh b/scripts/check_third_party_licenses.sh index 2e2f8bd9..b2e9d2fc 100755 --- a/scripts/check_third_party_licenses.sh +++ b/scripts/check_third_party_licenses.sh @@ -158,8 +158,9 @@ awk -v re="^[*][*] ($workspace_pattern); version " ' cat "$rust_section" } > "$generated" -# Ensure consistent EOL. -sed -i 's/\r//' "$generated" +# Ensure consistent EOL. Rewritten through a temp file rather than `sed -i`, +# which needs a backup suffix on BSD sed and rejects one on GNU sed. +sed 's/\r//' "$generated" > "$generated.eol" && mv "$generated.eol" "$generated" if [[ "$mode" == "update" ]]; then cp "$generated" "$OUTPUT_FILE" From 65da21610d6e048030635be4907250020c475cd8 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:12:48 -0700 Subject: [PATCH 4/6] fix: Keep a FLOAT range element's spelling built from Python `StepParameterSpace(taskParameterDefinitions={"F": {"type": "FLOAT", "range": ["1.50"]}})` rendered the task parameter value `1.5`, while the same element decoded from a template rendered `1.50`. Review on #349 found it; measured, 5 of 6 elements disagreed between the two routes: '1.50'->1.5, '1e3'->1000.0, '5.'->5.0, '.5'->0.5, '02.50'->2.5. `task_param_def_from_dict` read every element through `v.str()` and re-emitted the float variant as a JSON *number*. `Float64` reads a JSON number as a bare value and a JSON string as a preserved spelling, so the number form threw the decimal places away -- the thing openjd-rs#354 added `Float64` to carry. The template makes this distinction with the ` | ` union, so the binding now makes it the same way: an element that arrives as a Python `str` is forwarded as a JSON string, anything else keeps the numeric path. `v.str()` collapsed the two, which is why the fix is a branch rather than a cast. Deliberately not widened: - A Python `float` must not acquire a spelling from `str()`. Measured, both routes already agree on 1.5, 1000.0 and 0.5, and forwarding '1000.0' as text would have been a new divergence in the other direction. - An unparseable element still fails, in `Float64`'s deserializer rather than here, exactly as it did when this re-emitted a number. - `FloatTaskParameter.range` stays `list[float]`. It is the numeric introspection view, so '2.50' still reports 2.5 there while rendering 2.50. Two of them can compare equal by `.range` and render differently; the spec now says so rather than the API changing shape. - A redundant leading zero is still not stripped here. That is a `create_job` normalization (openjd-model `create_job/ranges.rs`), not part of reading a resolved value, and constructing the Rust `TaskParameter` directly behaves the same way -- `Float64`'s deserializer trims whitespace and unsigns zero but does not strip. So '02.50' given straight to the resolved type keeps its zero. Pinned by name. `TestFloatRangeSpellingIsPreserved` covers it: 10 string spellings, 3 float negative controls, agreement with the template path, the leading-zero divergence, and the rejection path. Mutation-checked by flipping the new branch back to the numeric path, which fails 10 of the 16. The two survivors are labelled in the test as not pinning the fix: '2.5' renders the same either way, and ' 2.50 ' already took the string path because Rust's f64 parse rejects the spaces. Separately measured against openjd-rs#354 itself: all 26 input/expected pairs from that PR's own assertions render identically through both this repo's implementations, v0 and v1 -- including '-0.0'->0.0, '0e5', '-0.0E+2'->0.0E+2, '1e-400', a 403-character 0.000...1, '5.' and '.5'. Verified: 5567 passed, 24 skipped, 3 xfailed with the 94% coverage gate enforced; cargo clippy --all-targets -D warnings, cargo fmt --check, cargo test, ruff, black and mypy all clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- rust-bindings/src/model/job.rs | 16 +++- specs/python-model-interface.md | 18 ++++ .../model_v1/test_step_param_space_iter.py | 95 +++++++++++++++++++ 3 files changed, 126 insertions(+), 3 deletions(-) diff --git a/rust-bindings/src/model/job.rs b/rust-bindings/src/model/job.rs index c956e3f1..0894754f 100644 --- a/rust-bindings/src/model/job.rs +++ b/rust-bindings/src/model/job.rs @@ -899,15 +899,25 @@ fn task_param_def_from_dict( // List of values — coerce each to the variant's element type. let mut items: Vec = Vec::with_capacity(list.len()); for v in list.iter() { - let s: String = match v.extract::() { - Ok(s) => s, - Err(_) => v.str()?.extract()?, + // Whether the element arrived as a Python `str` decides which member of + // the template's own ` | ` union it is, and a + // `` has to keep the spelling it was written with (§7.5). + // `Float64` reads a JSON string as that spelling and a JSON number as a + // bare value, so the distinction has to survive to here -- `v.str()` + // collapses it. + let (s, is_text): (String, bool) = match v.extract::() { + Ok(s) => (s, true), + Err(_) => (v.str()?.extract()?, false), }; items.push(match variant { "int" | "chunkInt" => match s.parse::() { Ok(n) => serde_json::Value::Number(n.into()), Err(_) => serde_json::Value::String(s), }, + // The text carries as-is. `Float64`'s deserializer parses it, and + // rejects an unparseable one there rather than here -- which is also + // what happened when this re-emitted a number. + "float" if is_text => serde_json::Value::String(s), "float" => match s.parse::() { Ok(n) => serde_json::Number::from_f64(n) .map(serde_json::Value::Number) diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index 29da99f7..89c68fab 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -718,6 +718,24 @@ does. (The underlying Rust struct has `chunks: Option` on the `Int` variant for shape reasons, but no resolver path ever populates it; the binding mirrors the runtime *behaviour*.) +`FloatTaskParameter.range` is numeric, so it does not show the decimal +places a `` range element was written with. A range element +`'2.50'` reports `2.5` here and renders `2.50` as the task parameter +value, which is the form that reaches a command line (Template Schemas +§7.5). Two `FloatTaskParameter`s that compare equal by `range` can +therefore render different task values. Read +`StepParameterSpaceIterator` for the rendered form. + +Constructing a space directly follows the same rule as the template: a +range element given as a `str` is a `` and keeps its +spelling, and one given as a `float` is a `` and renders as the +number. `StepParameterSpace(taskParameterDefinitions={"F": {"type": +"FLOAT", "range": ["1.50"]}})` renders `1.50`, and `range=[1.5]` renders +`1.5`. Stripping a redundant leading zero is a `create_job` +normalization rather than part of reading a resolved value, so `'02.50'` +given directly to the constructor keeps its zero where the same element +in a template does not. + ### `ChunkIntTaskParameter` Available only when the `TASK_CHUNKING` extension is enabled. diff --git a/test/openjd/model_v1/test_step_param_space_iter.py b/test/openjd/model_v1/test_step_param_space_iter.py index 03cd0478..ed2c6857 100644 --- a/test/openjd/model_v1/test_step_param_space_iter.py +++ b/test/openjd/model_v1/test_step_param_space_iter.py @@ -563,6 +563,101 @@ def test_len_raises_on_adaptive_chunking(self) -> None: len(it) +class TestFloatRangeSpellingIsPreserved: + """A FLOAT range element given as a ``str`` keeps the decimal places it was + written with, so the value reaching a command line is the one the caller asked + for (Template Schemas §7.5, openjd-rs#354). + + ``StepParameterSpace`` builds the *resolved* space directly, without the + ``decode_job_template`` + ``create_job`` path, so it is its own route to a + ``Float64`` and has to make the same choice: the template's range element is a + `` | `` union, and only the string member carries a + spelling. Before openjd-model 0.6.0 there was no spelling to carry -- the + resolved range was ``Vec`` -- so this could not be observed. + """ + + @staticmethod + def _rendered(elements: list[Any]) -> list[str]: + space = StepParameterSpace( + taskParameterDefinitions={"F": {"type": "FLOAT", "range": elements}} + ) + return [params["F"].value for params in StepParameterSpaceIterator(space=space)] + + @pytest.mark.parametrize( + "element,expected", + [ + ("1.50", "1.50"), # the trailing zero is the requested scale + ("3.500", "3.500"), + ("1e3", "1e3"), # exponent notation is not expanded + ("1E+2", "1E+2"), + ("5.", "5."), # no digit is invented after the point + (".5", ".5"), # nor before it + ("0.50", "0.50"), + ("-0.00", "0.00"), # zero has no sign, but keeps its places + # The last two do not pin the fix -- both render the same without it, and + # both survive the mutant that reverts it. Kept as controls: canonical text + # must not change, and padded text must still be trimmed (it took the + # string path already, because Rust's `f64` parse rejects the spaces). + ("2.5", "2.5"), + (" 2.50 ", "2.50"), + ], + ) + def test_a_string_element_keeps_its_spelling(self, element: str, expected: str) -> None: + assert self._rendered([element]) == [expected] + + @pytest.mark.parametrize("element,expected", [(1.5, "1.5"), (1000.0, "1000.0"), (0.5, "0.5")]) + def test_a_float_element_renders_as_the_number(self, element: float, expected: str) -> None: + """The negative control. A Python ``float`` is the union's ```` member and + has no spelling to keep, so it must not acquire one from ``str()`` -- these are + the values where preserving the text would have been indistinguishable from + rendering the number, and they must stay that way.""" + assert self._rendered([element]) == [expected] + + def test_it_matches_what_the_template_path_renders(self) -> None: + """The two routes to a resolved space agree, which is the point of the fix: + before it, this space rendered ``1.5`` where the template rendered ``1.50``.""" + elements = ["1.50", "1e3", "5.", ".5", "2.5"] + template = { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "steps": [ + { + "name": "S", + "parameterSpace": { + "taskParameterDefinitions": [ + {"name": "F", "type": "FLOAT", "range": elements} + ] + }, + "script": { + "actions": {"onRun": {"command": "echo", "args": ["{{Task.Param.F}}"]}} + }, + } + ], + } + step = create_job( + job_template=decode_job_template(template=template), job_parameter_values={} + ).steps[0] + from_template = [params["F"].value for params in StepParameterSpaceIterator(step=step)] + assert self._rendered(elements) == from_template == elements + + def test_a_redundant_leading_zero_is_not_stripped_here(self) -> None: + """The one place the two routes still differ, and deliberately. Stripping a + redundant leading zero is a ``create_job`` normalization (openjd-model + ``create_job/ranges.rs``), not part of reading a resolved value, so the + template path renders ``2.50`` while a value handed straight to the resolved + type keeps the ``0``. Constructing the Rust ``TaskParameter`` directly behaves + the same way, because ``Float64``'s deserializer trims whitespace and unsigns + zero but does not strip. + """ + assert self._rendered(["02.50"]) == ["02.50"] + + def test_an_unparseable_element_is_still_rejected(self) -> None: + """Forwarding the text rather than a number must not turn a bad element into an + accepted one; it fails in ``Float64``'s deserializer instead of here.""" + with pytest.raises(ValueError, match="not-a-float"): + self._rendered(["not-a-float"]) + + class TestChunkIntContains: """``__contains__`` must round-trip values yielded by a chunked iterator. The iterator yields ``TaskParameterValue`` instances From e27a4fb4ce8bf93f289e7f674c906a637f642622 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:21:25 -0700 Subject: [PATCH 5/6] fix: Enforce the adaptive indexing refusal, and three review follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from review on #349, all against bfa28bc. `__getitem__` now rejects an adaptive space explicitly. Review pointed out that it resolved a negative index against `self.len` — the count `__len__` refuses to report for an adaptive space, on the grounds that it is not knowable — and that `it[-1]` therefore declined only because `get` happened to decline downstream. Confirmed by reading it: `self.len` is captured unconditionally at construction and `__getitem__` had no adaptive check. Since 0.6.0 just extended `get` to contiguous chunked spaces, a later extension to adaptive would have made `it[-1]` answer against a count `len()` still withholds. The guard makes the documented behaviour the enforced one and lets the message name the reason. The message is now the pure-Python reference's, verbatim. The type stays `IndexError` rather than moving to v0's bare `LookupError`: `IndexError` is a `LookupError` subclass, so a caller catching either type is served by both implementations, while changing the type would break anyone catching `IndexError`. Full type parity is a breaking change and is not smuggled into a dependency bump. The spec now records the divergence, which review noted the rewritten text had made read as though the area were at parity. `test_indexing_observes_the_override` had a docstring the previous commit falsified: it said random access needs a non-sequential space and that a contiguous chunked space is always sequential, then pointed at "the test below" — which is now the test proving both clauses wrong. Replaced with the reason the NONCONTIGUOUS choice actually still holds, which review identified correctly: it renders a single-task chunk as a bare `1` where CONTIGUOUS renders `1-1`, and that is what the assertions depend on. `scripts/check_third_party_licenses.sh` no longer creates a second temp file. The `$generated.eol` sibling the previous commit introduced was not registered with the `trap`, so it leaked if the script died between the redirect and the `mv`. Folding `sed` into the redirect that already builds `$generated` drops the file, the `mv` and the portability caveat at once. Review suggested `tr -d`; kept `sed 's/\r//'` because `tr -d` also deletes a CR that is not a line ending, and the point here is EOL normalization. The script still reports the file up to date, so the output is byte-identical. Verified: 5567 passed, 24 skipped, 3 xfailed with the 94% coverage gate; cargo clippy --all-targets -D warnings, cargo fmt, ruff, black, mypy clean; scripts/check_third_party_licenses.sh reports up to date. Removing the new guard fails `test_an_adaptive_space_still_refuses_indexing` and nothing else, from a green baseline of 18 in that class. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- rust-bindings/src/model/step_param_space.rs | 27 ++++++++++++----- scripts/check_third_party_licenses.sh | 8 ++--- specs/python-model-interface.md | 16 +++++++++- .../model_v1/test_step_param_space_iter.py | 30 +++++++++++++++---- 4 files changed, 62 insertions(+), 19 deletions(-) diff --git a/rust-bindings/src/model/step_param_space.rs b/rust-bindings/src/model/step_param_space.rs index a764c807..5f0df8e7 100644 --- a/rust-bindings/src/model/step_param_space.rs +++ b/rust-bindings/src/model/step_param_space.rs @@ -214,6 +214,26 @@ impl PyStepParameterSpaceIterator { } fn __getitem__(&self, py: Python<'_>, index: isize) -> PyResult> { + // Random access uses a fresh iterator — don't disturb the + // persistent iter's cursor or its adaptive Arc. It must carry the + // same chunk override, or indexing would report chunks that + // iteration never yields. + let iter = + StepParameterSpaceIterator::new_with_chunk_override(&self.space, self.chunk_override) + .map_err(model_err_to_py)?; + // Rejected here rather than left to `get`, because the negative-index + // arithmetic below resolves against `self.len` — the count `__len__` + // refuses to report for an adaptive space. Without this, `it[-1]` declines + // only incidentally, and would start answering against that hidden count + // if `get` were ever extended to adaptive spaces the way 0.6.0 extended it + // to contiguous ones. The message is the pure-Python reference's; the type + // stays `IndexError`, which is a `LookupError` as v0 raises, so `except` + // clauses for either keep working. + if iter.chunks_adaptive() { + return Err(pyo3::exceptions::PyIndexError::new_err( + "Items cannot be retrieved by index because the parameter space uses adaptive chunking.", + )); + } let idx = if index < 0 { let adjusted = self.len as isize + index; if adjusted < 0 { @@ -225,13 +245,6 @@ impl PyStepParameterSpaceIterator { } else { index as usize }; - // Random access uses a fresh iterator — don't disturb the - // persistent iter's cursor or its adaptive Arc. It must carry the - // same chunk override, or indexing would report chunks that - // iteration never yields. - let iter = - StepParameterSpaceIterator::new_with_chunk_override(&self.space, self.chunk_override) - .map_err(model_err_to_py)?; match iter.get(idx) { Some(params) => task_param_set_to_py(py, ¶ms), None => Err(pyo3::exceptions::PyIndexError::new_err( diff --git a/scripts/check_third_party_licenses.sh b/scripts/check_third_party_licenses.sh index b2e9d2fc..19fb3f33 100755 --- a/scripts/check_third_party_licenses.sh +++ b/scripts/check_third_party_licenses.sh @@ -150,17 +150,15 @@ awk -v re="^[*][*] ($workspace_pattern); version " ' # ── Combine ─────────────────────────────────────────────────────────── +# Piped through `sed` on the way in, so the EOL strip needs neither a second temp +# file nor `sed -i`, which wants a backup suffix on BSD sed and refuses one on GNU. { echo "" echo "" cat "$python_section" echo "" cat "$rust_section" -} > "$generated" - -# Ensure consistent EOL. Rewritten through a temp file rather than `sed -i`, -# which needs a backup suffix on BSD sed and rejects one on GNU sed. -sed 's/\r//' "$generated" > "$generated.eol" && mv "$generated.eol" "$generated" +} | sed 's/\r//' > "$generated" if [[ "$mode" == "update" ]]; then cp "$generated" "$OUTPUT_FILE" diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index 89c68fab..96e587f2 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -1201,10 +1201,24 @@ the template's. One current-implementation limitation remains. An *adaptive* space has no knowable count until it is walked, so `len(it)` raises `ValueError` and -every index — including `-1` — raises `IndexError`. Iteration still +`it[i]` is refused for every index, negative included. Iteration still yields, which is what distinguishes an unknown count from an empty space. Supplying the override makes the space static and lifts both. +That refusal is enforced rather than incidental: `__getitem__` rejects an +adaptive space before it resolves a negative index, because the +arithmetic for a negative index would otherwise run against the length +`__len__` declines to report. + +One divergence from the v0 reference here, in the exception *type*. v0 +raises a bare `LookupError` for an adaptive `it[i]`; v1 raises +`IndexError`, which is a `LookupError` subclass, so a caller catching +either type is served by both implementations. The messages agree. +Because `IndexError` also means "past the end" in v1, the two conditions +are told apart by the message rather than the type — the adaptive refusal +says `Items cannot be retrieved by index because the parameter space uses +adaptive chunking.` where a real overrun says `index out of range`. + ### `StepDependencyGraph` Step dependency graph for topological ordering. diff --git a/test/openjd/model_v1/test_step_param_space_iter.py b/test/openjd/model_v1/test_step_param_space_iter.py index ed2c6857..9101322b 100644 --- a/test/openjd/model_v1/test_step_param_space_iter.py +++ b/test/openjd/model_v1/test_step_param_space_iter.py @@ -817,8 +817,10 @@ def test_indexing_observes_the_override(self) -> None: """``__getitem__`` builds a fresh iterator, so it has to carry the override too, or indexing reports chunks iteration never yields. - Uses NONCONTIGUOUS because random access needs a non-sequential space, and a - CONTIGUOUS chunked space is always sequential (see the test below). + NONCONTIGUOUS because that is what the assertions below depend on: it renders a + single-task chunk as a bare ``"1"``, where CONTIGUOUS renders ``"1-1"``. Both + support random access — see + ``test_a_contiguous_space_supports_indexing_with_or_without_the_override``. """ it = StepParameterSpaceIterator( step=self._step({"defaultTaskCount": 5, "rangeConstraint": "NONCONTIGUOUS"}), @@ -863,14 +865,30 @@ def test_a_contiguous_space_supports_indexing_with_or_without_the_override(self) def test_an_adaptive_space_still_refuses_indexing(self) -> None: """The negative control for the test above, and the remaining limitation recorded in ``specs/python-model-interface.md``: an adaptive space has no - knowable count, so ``len()`` raises and every index is out of range. Iteration - still yields, which is what distinguishes "unknown count" from "empty".""" + knowable count, so ``len()`` raises and every index is refused. Iteration + still yields, which is what distinguishes "unknown count" from "empty". + + The refusal is enforced rather than incidental. ``__getitem__`` resolves a + negative index against the length ``__len__`` declines to report, so leaving + the rejection to ``get`` would let ``it[-1]`` answer against that hidden count + if ``get`` were extended to adaptive spaces, as 0.6.0 extended it to + contiguous ones. The message therefore names the reason instead of saying + "index out of range", and it is the pure-Python reference's wording. + """ it = StepParameterSpaceIterator(step=self._step(self._ADAPTIVE)) assert it.chunks_adaptive is True - for index in (0, -1): + for index in (0, 1, -1, -100): with pytest.raises(IndexError) as excinfo: _ = it[index] - assert str(excinfo.value) == "index out of range" + assert str(excinfo.value) == ( + "Items cannot be retrieved by index because the parameter space " + "uses adaptive chunking." + ) + # v0 raises a bare LookupError here, not an IndexError. IndexError is a + # LookupError subclass, so a caller catching either is served by both + # implementations; the type itself still differs, which the spec records. + with pytest.raises(LookupError): + _ = it[0] assert self._frames(StepParameterSpaceIterator(step=self._step(self._ADAPTIVE))) == [ "1-5", "6-10", From 63f7735d0812fd746f8978bd1add31b6db3642dc Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:35:52 -0700 Subject: [PATCH 6/6] test: Pin the 512-character let identifier cap on the v1 path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit openjd-model 0.6.0 carries openjd-rs#358, which enforces Template Schemas §3.6.1's 512-character cap on a `let` binding's ``; before it a 513-character name was accepted. The v0 side has covered this since #348, in `test/openjd/model_v0/v2023_09/test_let_bindings.py`. The v1 path had nothing, so the bump brought the enforcement with no test on this side to hold it. Measured through `decode_job_template` on both implementations before writing the assertions. 512 characters is accepted and 513 rejected, identically, with `EXPR` alone and with `EXPR` plus `FEATURE_BUNDLE_1`. The accept at 512 with `EXPR` alone is the case worth having. The cap is flat, not §7.1's `` cap of 64 rising to 512 under `FEATURE_BUNDLE_1`, so a fix built on the wrong constant would reject a template the spec permits. A 65-character control covers the same ground from the other side: over the §7.1 cap, under §3.6.1's, and accepted. The two implementations' messages differ in wording, so the assertion is on the error path and the phrase `exceeds 512 characters`, not on the whole string. Verified: 5572 passed, 24 skipped, 3 xfailed with the 94% coverage gate; ruff, black and mypy clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/model_v1/test_parse.py | 63 ++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/test/openjd/model_v1/test_parse.py b/test/openjd/model_v1/test_parse.py index 482a9bca..35267aef 100644 --- a/test/openjd/model_v1/test_parse.py +++ b/test/openjd/model_v1/test_parse.py @@ -232,6 +232,69 @@ def test_template_extensions_list(template, template_type, decode_function) -> N decode_function(template=template, supported_extensions=["TASK_CHUNKING", "EXPR"]) +class TestLetIdentifierLengthCap(object): + """A ``let`` binding's ```` is capped at 512 characters + (Template Schemas §3.6.1). Enforced by openjd-model 0.6.0 (openjd-rs#358); + before that a 513-character name was accepted here. The v0 side has its own + coverage in ``test/openjd/model_v0/v2023_09/test_let_bindings.py``; this is the + v1 path, which had none. + + The cap is flat, not the §7.1 ```` cap of 64 rising to 512 with + ``FEATURE_BUNDLE_1``. The 512-character accept with ``EXPR`` alone is what makes + the difference observable, and is why the fix upstream did not reuse + ``EffectiveLimits::max_identifier_len``. + """ + + @staticmethod + def _template(name: str, extensions: list[str]) -> dict[str, Any]: + return { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "extensions": extensions, + "steps": [ + { + "name": "Step1", + "let": [f"{name} = 42"], + "script": {"actions": {"onRun": {"command": "echo", "args": ["hi"]}}}, + } + ], + } + + @pytest.mark.parametrize( + "extensions", + [ + pytest.param(["EXPR"], id="EXPR only"), + pytest.param(["EXPR", "FEATURE_BUNDLE_1"], id="with FEATURE_BUNDLE_1"), + ], + ) + def test_512_characters_is_accepted(self, extensions: list[str]) -> None: + """Both extension sets accept the boundary. Without ``FEATURE_BUNDLE_1`` the + §7.1 cap would be 64, so this is the case that pins the cap as flat.""" + template = self._template("a" * 512, extensions) + assert decode_job_template(template=template, supported_extensions=extensions) + + @pytest.mark.parametrize( + "extensions", + [ + pytest.param(["EXPR"], id="EXPR only"), + pytest.param(["EXPR", "FEATURE_BUNDLE_1"], id="with FEATURE_BUNDLE_1"), + ], + ) + def test_513_characters_is_rejected(self, extensions: list[str]) -> None: + template = self._template("a" * 513, extensions) + with pytest.raises(ModelValidationError) as excinfo: + decode_job_template(template=template, supported_extensions=extensions) + message = str(excinfo.value) + assert "steps[0] -> let[0]" in message + assert "exceeds 512 characters" in message + + def test_a_short_name_is_unaffected(self) -> None: + """Control. 65 characters is over the §7.1 cap of 64 and under §3.6.1's, so it + must be accepted -- a regression to the wrong constant would reject it.""" + template = self._template("a" * 65, ["EXPR"]) + assert decode_job_template(template=template, supported_extensions=["EXPR"]) + + class TestDecodeJobTemplateStr: """``decode_job_template_str`` wrapper: parses YAML or JSON directly, no intermediate dict. The wrapper lives on