Skip to content

Allow an environment to define only onExit - #338

Merged
leongdl merged 1 commit into
OpenJobDescription:mainlinefrom
leongdl:fix/queue-environment-actions
Aug 25, 2026
Merged

Allow an environment to define only onExit#338
leongdl merged 1 commit into
OpenJobDescription:mainlinefrom
leongdl:fix/queue-environment-actions

Conversation

@leongdl

@leongdl leongdl commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes:

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

An environment script that defines onExit without onEnter is rejected at template decode:

environment -> script -> actions:
	onEnter is required.

This blocks cleanup-only environments. The motivating case is a queue environment whose only work is tearing down state established elsewhere in the session, but the same environment type is used by jobEnvironments and step environments, so all three are affected.

The rejection comes from a check added in #318 as a validation tightening. Before that change this validator only required that one of the two ordinary actions be present:

if on_enter is None and on_exit is None:
    raise ValueError("Must define one of: onEnter or onExit")

What was the solution? (How)

Remove the added onEnter is required. branch and restore the one-of rule. An ordinary environment script must define onEnter or onExit; either action alone is sufficient.

Unchanged:

  • an actions object defining neither ordinary action is still rejected with Must define one of: onEnter or onExit
  • WRAP_ACTIONS extension gating, the EXPR prerequisite, and the all-or-nothing wrap-hook rule
  • the wrapped-variable scope rules

The removed check was also the only reason the ordinary-action path inspected the parsing context, so validation no longer differs between template decode and job instantiation.

What is the impact of this change?

Templates that were previously rejected are now accepted. Nothing that was accepted becomes rejected.

Environment actions Before After
onEnter only accepted accepted
onEnter and onExit accepted accepted
onExit only rejected accepted
neither rejected rejected
complete wrap-hook set accepted accepted
partial or ungated wrap hooks rejected rejected

Cross-implementation note: openjd-rs still rejects onExit-only, so this change makes the two implementations disagree until the same change lands there. The corresponding conformance case is proposed in openjd-specifications.

How was this change tested?

  • hatch run test — 5483 passed, 24 skipped, 3 xfailed, coverage 94%
  • hatch run lint — ruff, black, and mypy clean
  • hatch build — sdist and wheel built
  • Full 2023-09 conformance suite against the rebuilt wheel — 1160 passed, 2 failed. Both failures are the existing onExit-only fixtures that assert rejection and are updated in the companion openjd-specifications change.
  • The new unit tests were mutation-checked: restoring the removed branch makes them fail.

New tests:

  • test_action.pyonExit-only accepted both with no extensions and with EXPR/WRAP_ACTIONS, asserting the decoded actions; empty actions still rejected, asserted on the full error message

  • test_environment_template.py — a standalone environment-2023-09 template whose script defines only onExit

  • test_job_template.py — a jobEnvironments entry whose script defines only onExit

  • Have you run the unit tests? Yes.

Was this change documented?

  • Are relevant docstrings in the code base updated? The validator docstring already described the one-of rule, which is accurate again. The class docstring already stated "Must define at least one of onEnter or onExit".

Is this a breaking change?

No. The change only widens what is accepted.

Does this change impact security?

No. No files, directories, permissions, or process boundaries are affected.


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

An environment script that defined onExit without onEnter was rejected at
template decode with "onEnter is required." This blocked cleanup-only
environments, including queue environments whose only work is tearing down
state established elsewhere.

Restore the one-of rule this validator applied before the strict check was
added: an ordinary environment script must define onEnter or onExit, and
either action alone is sufficient. An actions object that defines neither is
still rejected, and the WRAP_ACTIONS extension gating, the all-or-nothing
wrap-hook rule, and the wrapped-variable scope rules are unchanged.

The removed check was also the only reason the validator inspected the
parsing context on the ordinary-action path, so the behavior no longer
differs between template decode and job instantiation.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
@leongdl
leongdl requested a review from a team as a code owner August 25, 2026 19:36
pytest.param(["EXPR", "WRAP_ACTIONS"], id="EXPR and WRAP_ACTIONS"),
),
)
def test_parse_onexit_only(self, supported_extensions: list[str]) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This relaxation is only covered on the pure-Python (v0) side. AGENTS.md ("Reference parity") asks that every reference test under test/openjd/model_v0/ have an equivalent under test/openjd/model_v1/ exercising the same behaviour through the Rust binding, and there is currently no v1 test that decodes an environment whose script.actions defines onExit only — the closest fixture (MINIMAL_ENV in test_rust_model_bindings.py:36) defines both actions.

That matters more than usual here because the constraint being removed was introduced in a2cc32f as part of the openjd-rs parity work. If openjd-model 0.5.2 still rejects onExit-only, this change flips the divergence rather than closing it: v0 would accept a template that decode_environment_template through the binding rejects, and nothing in the suite would catch it.

Suggest adding a v1 counterpart that decodes {"actions": {"onExit": {"command": "foo"}}} — if it passes, it is a regression test for the parity you are asserting; if it fails, it belongs in test/openjd/model_v1/test_known_gaps.py as an xfail so the gap is recorded.

@leongdl
leongdl merged commit 5b9c661 into OpenJobDescription:mainline Aug 25, 2026
34 of 50 checks passed
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.

4 participants