Skip to content

chore(deps): Bump openjd-model to 0.7.1 and openjd-sessions to 0.5.8 - #363

Open
wyongzhi wants to merge 1 commit into
OpenJobDescription:mainlinefrom
wyongzhi:deps/openjd-rs-0.7.1
Open

chore(deps): Bump openjd-model to 0.7.1 and openjd-sessions to 0.5.8#363
wyongzhi wants to merge 1 commit into
OpenJobDescription:mainlinefrom
wyongzhi:deps/openjd-rs-0.7.1

Conversation

@wyongzhi

Copy link
Copy Markdown
Contributor

What changed

Bumps two openjd-* Rust crate pins in rust-bindings/Cargo.toml, refreshes Cargo.lock, and regenerates THIRD-PARTY-LICENSES.txt (scripts/check_third_party_licenses.sh --update):

Crate From To
openjd-model 0.7.0 0.7.1
openjd-sessions 0.5.7 0.5.8

openjd-expr stays at 0.7.0. cargo update -p openjd-model -p openjd-sessions moved only these two packages; no new transitive dependency.

Upstream changes

openjd-rs#381 (openjd-model 0.7.1) — stepEnvironment name uniqueness is scoped to its step. The 2023-09 spec requires environment names to be unique within jobEnvironments, within each step's stepEnvironments, and requires a stepEnvironment name to differ from every jobEnvironment name. It does not forbid two different steps from reusing a name, and the v0 (Python) path has always accepted that. The v1 (Rust) path held every environment name in one set and rejected the second step to declare the same name. The pin brings the fix; no binding code change was needed (patch release, no API change).

openjd-sessions 0.5.7 → 0.5.8 carries no source change to the crate (release-train version bump only).

Tests

TestStepEnvironmentNameScope in test/openjd/model_v1/test_parse.py pins the behaviour on the v1 path: four steps each declaring StepEnv decode successfully, plus two controls asserting the spec's two prohibitions still hold (duplicate within one step; step environment named like a job environment), each checked on the error path and message.

Mutation-checked by rebuilding the extension against the previous pins (openjd-model 0.7.0): 1 failed, 2 passed — the positive case fails with duplicate environment name: 'StepEnv' and both controls still pass. Rebuilt on 0.7.1: 3 passed.

Full run: hatch run test 5967 passed, 24 skipped, 3 xfailed, coverage 94.14%; hatch run lint and hatch run typing clean; cargo clippy --manifest-path rust-bindings/Cargo.toml --all-targets clean.

Breaking change / security

No public interface changes. No security impact.


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

Picks up OpenJobDescription/openjd-rs#381: stepEnvironment name
uniqueness is scoped to its step, matching the Python implementation
and the 2023-09 specification. Templates in which different steps
declare a stepEnvironment with the same name now decode on the v1
(Rust) path.

THIRD-PARTY-LICENSES.txt regenerated with scripts/check_third_party_licenses.sh --update.

Signed-off-by: wyongzhi <276409147+wyongzhi@users.noreply.github.com>
decode_job_template(template=template, supported_extensions=[])
message = str(excinfo.value)
assert "steps[1] -> stepEnvironments[1]" in message
assert "duplicate environment name: 'StepEnv'" in message

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two control tests here cover the §3 per-Step rule and the Step-vs-Job rule, but not the rule most at risk from this particular relaxation: uniqueness within jobEnvironments.

Upstream #381 relaxed a check that "held every environment name in the template in one set". Collapsing that single set is exactly the kind of change that can drop the job-level duplicate check along with the cross-step one, and there is currently no v1 coverage of it — grepping jobEnvironments under test/openjd/model_v1/ finds only this new _template() helper, test_fuzz.py:199 (an empty {} entry), and test_template_types.py (constructor round-trip, no validation). The v0 path does pin it (test/openjd/model_v0/v2023_09/test_job_template.py:388, id="duplicate environment names", expecting exactly 1 error), so v1 is the side without a guard.

A third control would close that:

def test_duplicate_job_env_names_is_rejected(self) -> None:
    template = self._template([self._step("Step0", ["StepEnv"])])
    template["jobEnvironments"] = [self._environment("JobEnv"), self._environment("JobEnv")]
    with pytest.raises(ModelValidationError):
        decode_job_template(template=template, supported_extensions=[])

Comment thread rust-bindings/Cargo.toml
openjd-expr = "0.7.0"
openjd-model = "0.7.0"
openjd-sessions = "0.5.7"
openjd-model = "0.7.1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Process note on release visibility: this bump is a user-visible validation relaxation — a template where two Steps each declare a stepEnvironment named StepEnv was rejected on the v1 path and now decodes — but it is very likely to ship with no changelog entry describing it.

[tool.semantic_release.commit_parser_options] lists chore in patch_tags, so a chore(deps): subject does bump the version. But .semantic_release/CHANGELOG.md.j2 only renders the breaking, features, bug fixes, and performance improvements element groups — there is no chore section, so a chore-typed commit produces a release with nothing written about it. That is the observed history: #349 (chore(deps): Bump openjd-* Rust crates to the 0.6.0 release) appears in neither the 0.11.8 nor the 0.11.9 CHANGELOG section. The 0.11.11 entry for #359 exists only because its text was hand-written (it carries no commit-hash link, unlike every template-generated line).

Since this bump changes what templates are accepted rather than just refreshing a pin, consider a fix:-typed subject so the behavior change lands in the released notes on its own. The relaxation is exactly the kind of thing a downstream consumer pinning this library would want to read in the changelog.

template = self._template([self._step(f"Step{i}", ["StepEnv"]) for i in range(4)])
job_template = decode_job_template(template=template, supported_extensions=[])
names = [[e.name for e in (s.step_environments or [])] for s in job_template.steps]
assert names == [["StepEnv"]] * 4

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream #381 carried a second behavior change that this PR neither mentions nor covers: max_env_count enforcement.

From the upstream PR body: "max_env_count now counts environments (env_count) instead of HashSet::len(). The two were equal only because duplicates were always rejected; with names allowed to repeat across steps, counting distinct names would under-count."

That limit is reachable from this repo — it is a public CallerLimits field (rust-bindings/src/model/profile.rs:506, _openjd_rs.pyi:314) passed to decode_job_template(..., caller_limits=...). And the new relaxation is precisely what makes the two counting strategies diverge: the template shape added in test_same_name_across_steps_is_accepted above (4 steps x StepEnv) is the first shape where distinct-name count (1) and total environment count (5, with JobEnv) differ at all.

Searching test/ for max_env_count turns up only test_pickle.py:141,247 — round-tripping the field through __reduce__. Nothing asserts the limit is actually enforced, so the counting change is invisible to this suite in both directions: an upstream regression back to HashSet::len() would go unnoticed, and so would one that double-counted.

A test in this class would pin it, reusing the helpers already here (4 steps x StepEnv plus JobEnv is 5 environments but only 2 distinct names, so a limit of 4 must reject):

def test_max_env_count_counts_repeated_names_separately(self) -> None:
    template = self._template([self._step(f"Step{i}", ["StepEnv"]) for i in range(4)])
    with pytest.raises(ModelValidationError):
        decode_job_template(
            template=template,
            supported_extensions=[],
            caller_limits=CallerLimits(max_env_count=4),
        )

(CallerLimits is importable from openjd.model._v1; it is not currently imported in this module.)

decode_job_template(template=template, supported_extensions=[])
message = str(excinfo.value)
assert "steps[1] -> stepEnvironments[0]" in message
assert "duplicate environment name: 'JobEnv'" in message

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions newly pin v1 error text for the step-vs-job rule that does not match the v0 reference — worth a deliberate decision rather than being locked in as a side effect of a dependency bump.

AGENTS.md:176-180 ("Test Quality Standard") states the goal is that failure messages "match the pure-Python reference where one exists", and that "the Rust bindings reproduce the same format" as pydantic's path-prefixed output. For this specific rule the two diverge on both halves of what is asserted here:

  • Message. v0 raises Name JobEnv must differ from the names of Environments defined at the root of the template. (src/openjd/model/v2023_09/_model.py:4488). v1 says duplicate environment name: 'JobEnv'. The v0 wording names the rule; the v1 wording reads as if the name were duplicated inside one list, which is the other rule — the one test_duplicate_within_one_step_is_rejected covers two tests up. Both tests here assert the same duplicate environment name: string for what are two distinct spec rules, so neither assertion can distinguish them.
  • Path. v0's loc is ("step", i, "stepEnvironments", j, "name") (_model.py:4485), rendering through _convert_pydantic_error._loc_to_str as step[1] -> stepEnvironments[0] -> name. The asserted v1 path is steps[1] -> stepEnvironments[0] — plural steps, and no trailing -> name segment. (v0's singular step looks like the v0-side bug of the pair, since the field is steps; either way they are not equal.)

If the divergence is intended, a note in the class docstring would keep the next reader from reading these as parity assertions. If not, it is a real gap to file — but pinning the current strings in a test makes it harder to notice later.

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.

1 participant