fix: thread step_name to run_task for wrap-action support - #1039
Conversation
openjd-sessions raises when a task runs while a wrap environment is active and no step_name is given, because RFC 0008's WrappedStep.Name has no value to render. The agent never passed step_name, so a job that entered a wrap environment and then ran a task failed. Thread step_name from the step template through Session.run_task, the SessionRuntime interface, and the Python adapter to the underlying openjd session. The argument is keyword-only and optional so the interface and both adapters stay uniform and existing callers are unaffected. The Rust adapter accepts the argument but does not forward it yet: the pinned openjd-sessions release's v1 wrapper does not expose the kwarg. No wrap-action job can reach either runtime today, since the service does not yet accept extension requests at job submission. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
github-actions
Bot
added
the
waiting-on-maintainers
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| task_parameter_values=task_parameter_values, | ||
| os_env_vars=os_env_vars, | ||
| log_task_banner=log_task_banner, | ||
| step_name=step_name, |
There was a problem hiding this comment.
step_name is forwarded unconditionally to self._session.run_task(...) (the real openjd.sessions.Session from the pinned openjd-sessions == 0.10.11). If that pinned release's Session.run_task does not accept a step_name keyword, this raises TypeError at runtime for every task run on the Python runtime.
This is worth double-checking because rust.py (lines 311-314) explicitly states the pinned release "does not expose the kwarg yet." The distinction drawn there is between the Rust _v1 wrapper and the native session — but the Python Session comes from the same pinned package, so please confirm 0.10.11's Session.run_task really does accept step_name.
Note the unit test test_run_task_when_step_name_provided_forwards_to_openjd_session patches OpenJDSession with a MagicMock, which accepts any kwargs — so it would pass even if the real signature rejects step_name, and cannot catch this mismatch.
There was a problem hiding this comment.
refer to this: OpenJobDescription/openjd-sessions-for-python#345
3 participants
What was the problem/requirement? (What/Why)
openjd-sessions raises a
ValueErrorwhen a task runs while a wrap environment (RFC 0008WRAP_ACTIONS) is active and nostep_nameis given, becauseWrappedStep.Namehas no value to render. The worker agent never passedstep_name— the step template's name is available at the call site and was simply dropped — so a job that entered a wrap environment and then ran a task failed mid-session.What was the solution? (How)
Thread
step_namefrom the step template throughSession.run_task, theSessionRuntimeinterface, and the Python adapter down toopenjd.sessions.Session.run_task(the pinnedopenjd-sessions == 0.10.11already accepts it). The argument is keyword-only and optional so the interface and both adapters stay uniform and existing callers are unaffected.The Rust adapter accepts the argument but does not forward it yet: the pinned openjd-sessions release's
_v1wrapper does not expose the kwarg. OpenJobDescription/openjd-sessions-for-python#345 adds the forward upstream; a follow-up will bump the pin and wire it through once that ships.What is the impact of this change?
The Python runtime is correct for RFC 0008 task runs. Inert for all existing jobs — openjd only reads
step_nameinside an active wrap hook, which cannot occur today since no extension is requestable end-to-end yet. No API or behavior change for callers that omit the argument.How was this change tested?
hatch build— cleanhatch run all:test— 3050 passed / 39 skipped on each of Python 3.9, 3.10, and 3.11hatch run lint— ruff check, ruff format, and mypy all cleantest/unit/sessions/test_step_name_threading.pycovers the threading at each layer (action → session → ABC → both adapters), asserting the literal step name arrives at the openjd session and that the Rust adapter accepts the kwarg without forwarding itWas this change documented?
Yes — docstrings/comments on the interface and both adapters explain the parameter and why the Rust adapter defers forwarding.
Is this a breaking change?
No — a new optional keyword-only parameter with a
Nonedefault at every layer.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.