feat: Emit None for WrappedAction.Cancelation.Mode; review fixes

Change WrappedAction.Cancelation.Mode from an empty-string sentinel to
None (string?) when the wrapped action defines no <Cancelation>,
matching the EXPR semantics for optional data and the int? typing of
Cancelation.NotifyPeriodInSeconds. None renders as the empty string in
format-string interpolation, and the nullness is observable via EXPR
null-coalescing.

Also address review findings on the wrap-action implementation:

- Fail gracefully when WrappedAction.* injection cannot resolve the
  wrapped action's format strings (e.g. a wrapped onRun referencing
  {{Task.File.*}} or onEnter referencing {{Env.File.*}} — embedded
  files are not materialized on the wrap path yet, a known limitation
  shared with the Rust runtime). The action now FAILs through the
  normal callback path via a new _fail_action_before_start helper and
  the session transitions to READY_ENDING, instead of a raw
  FormatStringError escaping the public API — which, for
  enter_environment, previously left the session stuck in RUNNING
  with no runner. Also covers non-integer FEATURE_BUNDLE_1
  timeout/notifyPeriod resolutions.
- WrappedAction.Environment now carries only openjd_env-defined
  variables per RFC 0008, excluding the environment's declarative
  variables: map seed (parity with the Rust runtime, which was
  already correct).
- WrappedAction.Environment is flattened cumulatively in
  environment-entry order, so a later openjd_env set yields one
  effective entry (not a duplicate) and a later unset removes the
  name — matching the real subprocess environment and Rust's single
  cumulative env_vars map.
- On the onWrapEnvExit path, the openjd_env list is captured before
  the exiting environment is removed from tracking, so the wrapped
  environment's own openjd_env variables appear in
  WrappedAction.Environment just as they do in the real subprocess
  environment.
- Log a warning when a wrap environment is active but run_task() was
  not given step_name, since {{WrappedStep.Name}} renders empty.
- Hoist the Template Schemas 5.3.2 notify-period defaults (120/30)
  into shared constants used by the seeding and both cancel paths.
- Guard _run_wrap_hook against unknown hook names so a typo cannot
  become a silent SUCCESS no-op.

Tests assert `is None` for the undeclared cancelation case, the
injection-failure and READY_ENDING behavior, the openjd_env-only
Environment contents (including override/unset flattening and the
exit-path capture), and the hook-name guard. All 53 WRAP_ACTIONS
conformance tests pass against the Python CLI.

Addresses review feedback on openjd-rs PR #261 (discussion
r3597572812) and openjd-specifications PR #148 (r3597560515).

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

Author: leongdl

src/openjd/sessions/_runner_env_script.py

@@ -22,7 +22,12 @@
     TerminateCancelMethod,
 )
 from ._session_user import SessionUser
-from ._types import ActionModel, ActionState, EnvironmentScriptModel
+from ._types import (
+    ENV_ACTION_DEFAULT_NOTIFY_PERIOD_SECONDS,
+    ActionModel,
+    ActionState,
+    EnvironmentScriptModel,
+)
 
 __all__ = ("EnvironmentScriptRunner",)
 
@@ -195,11 +200,13 @@ def wrap_env_exit(self) -> None:
         substituting it for an inner environment's ``onExit``."""
         self._run_wrap_hook("onWrapEnvExit", default_timeout=_ENV_EXIT_DEFAULT_TIMEOUT)
 
-    def _run_wrap_hook(
-        self, hook: str, *, default_timeout: Optional[timedelta] = None
-    ) -> None:
+    def _run_wrap_hook(self, hook: str, *, default_timeout: Optional[timedelta] = None) -> None:
         """Common dispatch for the three RFC 0008 wrap hooks. ``hook`` is
         one of ``onWrapEnvEnter``, ``onWrapTaskRun``, or ``onWrapEnvExit``."""
+        if hook not in ("onWrapEnvEnter", "onWrapTaskRun", "onWrapEnvExit"):
+            # Guard the getattr below: without this, a typo'd hook name
+            # would silently become a SUCCESS no-op.
+            raise ValueError(f"Unknown wrap hook name: {hook}")
         if self.state != ScriptRunnerState.READY:
             raise RuntimeError("This cannot be used to run a second subprocess.")
 
@@ -247,8 +254,10 @@ def cancel(
             # For the type checker
             assert isinstance(model_cancel_method, CancelationMethodNotifyThenTerminate_2023_09)
             if model_cancel_method.notifyPeriodInSeconds is None:
-                # Default grace period is 30s for a 2023-09 Environment Script's notify cancel
-                method = NotifyCancelMethod(terminate_delay=timedelta(seconds=30))
+                # Default grace period for a 2023-09 Environment Script's notify cancel
+                method = NotifyCancelMethod(
+                    terminate_delay=timedelta(seconds=ENV_ACTION_DEFAULT_NOTIFY_PERIOD_SECONDS)
+                )
             else:
                 method = NotifyCancelMethod(
                     terminate_delay=timedelta(seconds=model_cancel_method.notifyPeriodInSeconds)  # type: ignore[arg-type]

src/openjd/sessions/_runner_step_script.py

@@ -21,7 +21,7 @@
     TerminateCancelMethod,
 )
 from ._session_user import SessionUser
-from ._types import ActionState, StepScriptModel
+from ._types import TASK_RUN_DEFAULT_NOTIFY_PERIOD_SECONDS, ActionState, StepScriptModel
 
 __all__ = ("StepScriptRunner",)
 
@@ -139,8 +139,10 @@ def cancel(
             # For the type checker
             assert isinstance(model_cancel_method, CancelationMethodNotifyThenTerminate_2023_09)
             if model_cancel_method.notifyPeriodInSeconds is None:
-                # Default grace period is 120s for a 2023-09 Step Script's notify cancel
-                method = NotifyCancelMethod(terminate_delay=timedelta(seconds=120))
+                # Default grace period for a 2023-09 Step Script's notify cancel
+                method = NotifyCancelMethod(
+                    terminate_delay=timedelta(seconds=TASK_RUN_DEFAULT_NOTIFY_PERIOD_SECONDS)
+                )
             else:
                 method = NotifyCancelMethod(
                     terminate_delay=timedelta(seconds=model_cancel_method.notifyPeriodInSeconds)  # type: ignore[arg-type]

src/openjd/sessions/_session.py

@@ -17,6 +17,7 @@
 from typing import TYPE_CHECKING, Any, Callable, Optional, Type, Union
 
 from openjd.model import (
+    FormatStringError,
     JobParameterValues,
     ParameterValue,
     ParameterValueType,
@@ -48,6 +49,8 @@
 from ._subprocess import LoggingSubprocess
 from ._tempdir import TempDir, custom_gettempdir
 from ._types import (
+    ENV_ACTION_DEFAULT_NOTIFY_PERIOD_SECONDS,
+    TASK_RUN_DEFAULT_NOTIFY_PERIOD_SECONDS,
     ActionState,
     EnvironmentIdentifier,
     EnvironmentModel,
@@ -136,6 +139,11 @@ class SimplifiedEnvironmentVariableChanges:
 
     def __init__(self, initial_variables: Union[dict[str, str], "EnvironmentVariableObject"]):
         self._to_set: dict[str, Optional[str]]
+        # Names of variables that were set/unset via openjd_env stdout
+        # messages (RFC 0008: only these are surfaced through
+        # ``WrappedAction.Environment``; the initial ``variables:`` map
+        # seed is intentionally excluded, matching the Rust runtime).
+        self._openjd_env_names: set[str] = set()
 
         if is_windows():
             self._to_set = {}
@@ -147,12 +155,14 @@ def __init__(self, initial_variables: Union[dict[str, str], "EnvironmentVariable
     def simplify_ordered_changes(self, changes: list[EnvironmentVariableChange]) -> None:
         """Apply a given list of sets and unsets to the current state in order"""
         for change in changes:
+            name = change.name.upper() if is_windows() else change.name
             if isinstance(change, EnvironmentVariableSetChange):
-                self._to_set[change.name.upper() if is_windows() else change.name] = change.value
+                self._to_set[name] = change.value
             elif isinstance(change, EnvironmentVariableUnsetChange):
-                self._to_set[change.name.upper() if is_windows() else change.name] = None
+                self._to_set[name] = None
             else:
                 raise ValueError("Unknown type of environment variable change.")
+            self._openjd_env_names.add(name)
 
     def apply_to_environment(self, env_vars: dict[str, Optional[str]]) -> None:
         """Modify a given dictionary of environment variables to reflect the changes"""
@@ -723,7 +733,19 @@ def enter_environment(
             wrap_env = None
 
         if wrap_env is not None:
-            self._inject_wrapped_env_symbols(symtab, environment, on_enter_action)
+            try:
+                self._inject_wrapped_env_symbols(symtab, environment, on_enter_action)
+            except (FormatStringError, ValueError) as e:
+                # e.g. the wrapped onEnter references {{Env.File.*}} (embedded
+                # files are not materialized on the wrap path yet). Fail the
+                # action through the normal failure path — the environment
+                # stays in the entered list, exactly as when enter() itself
+                # fails, so the caller's cleanup exits it as usual.
+                self._fail_action_before_start(
+                    f"Failed to resolve the wrapped onEnter action of "
+                    f"{environment.name} for {wrap_env.name}'s onWrapEnvEnter: {e}"
+                )
+                return identifier
             self._runner = EnvironmentScriptRunner(
                 logger=self._logger,
                 user=self._user,
@@ -805,6 +827,14 @@ def exit_environment(
         # Must be run _before_ we pop _environments_entered
         action_env_vars = self._evaluate_current_session_env_vars(os_env_vars)
 
+        # RFC 0008: capture the openjd_env list for WrappedAction.Environment
+        # _before_ the exiting environment is removed from tracking, so the
+        # list includes that environment's own openjd_env variables — the
+        # real subprocess environment (action_env_vars, computed above)
+        # includes them, and the wrapped onExit runs with them in the
+        # unwrapped case too.
+        wrapped_session_env_list = self._collect_session_env_list()
+
         # Remove the environment from our tracking since we're now exiting it.
         del self._environments[identifier]
         self._environments_entered.pop()
@@ -835,7 +865,23 @@ def exit_environment(
         )
 
         if wrap_env is not None:
-            self._inject_wrapped_env_symbols(symtab, environment, on_exit_action)
+            try:
+                self._inject_wrapped_env_symbols(
+                    symtab,
+                    environment,
+                    on_exit_action,
+                    session_env_list=wrapped_session_env_list,
+                )
+            except (FormatStringError, ValueError) as e:
+                # Mirror of the onWrapEnvEnter injection-failure handling:
+                # fail the action through the normal failure path. The
+                # environment was already removed from tracking above,
+                # matching how a failed exit() behaves.
+                self._fail_action_before_start(
+                    f"Failed to resolve the wrapped onExit action of "
+                    f"{environment.name} for {wrap_env.name}'s onWrapEnvExit: {e}"
+                )
+                return
             self._runner = EnvironmentScriptRunner(
                 logger=self._logger,
                 user=self._user,
@@ -918,7 +964,28 @@ def run_task(
         # wrap action instead of the step script's onRun (RFC 0008).
         wrap_env = self._find_wrap_environment(hook="onWrapTaskRun")
         if wrap_env is not None:
-            self._inject_wrapped_task_symbols(symtab, step_script, step_name or "")
+            if step_name is None:
+                # RFC 0008: without a step name, {{WrappedStep.Name}}
+                # renders as the empty string in the wrap script. Callers
+                # predating the step_name kwarg won't pass it; make the
+                # gap visible rather than silently rendering empty.
+                self._logger.warning(
+                    "A wrap environment is active but run_task() was not given a "
+                    "step_name; WrappedStep.Name will render as an empty string."
+                )
+            try:
+                self._inject_wrapped_task_symbols(symtab, step_script, step_name or "")
+            except (FormatStringError, ValueError) as e:
+                # e.g. the wrapped onRun references {{Task.File.*}} (embedded
+                # files are not materialized on the wrap path yet), or a
+                # FEATURE_BUNDLE_1 timeout/notifyPeriod format string did not
+                # resolve to an integer. Fail the action through the normal
+                # failure path rather than raising out of the public API.
+                self._fail_action_before_start(
+                    f"Failed to resolve the wrapped Task action for {wrap_env.name}'s "
+                    f"onWrapTaskRun: {e}"
+                )
+                return
 
             self._runner = EnvironmentScriptRunner(
                 logger=self._logger,
@@ -1227,23 +1294,37 @@ def _environment_defines_any_wrap_hook(self, env: EnvironmentModel) -> bool:
         if env.script is None:
             return False
         return any(
-            hasattr(env.script.actions, name)
-            and getattr(env.script.actions, name) is not None
+            hasattr(env.script.actions, name) and getattr(env.script.actions, name) is not None
             for name in self._WRAP_HOOK_NAMES
         )
 
     def _collect_session_env_list(self) -> list[str]:
         """Collect all ``openjd_env``-defined variables across the active
         environment stack as ``["KEY=value", ...]`` for
-        ``WrappedAction.Environment``."""
-        env_list: list[str] = []
+        ``WrappedAction.Environment``.
+
+        RFC 0008 defines this variable as carrying only ``openjd_env``
+        definitions from the current session; an environment's declarative
+        ``variables:`` map (and host-inherited variables) are intentionally
+        excluded, matching the Rust runtime.
+
+        The per-environment changes are flattened cumulatively, in
+        environment-entry order, so the list reflects the *effective*
+        state exactly like the real subprocess environment does: a later
+        ``openjd_env`` set overrides an earlier value for the same name
+        (one entry, not two), and a later unset removes the name entirely.
+        This mirrors the Rust runtime's single cumulative ``env_vars``
+        map."""
+        effective: dict[str, Optional[str]] = {}
         for env_id in self._environments_entered:
             if env_id in self._created_env_vars:
                 changes = self._created_env_vars[env_id]
+                # Iterate _to_set (insertion-ordered) rather than the name
+                # set so the list order is deterministic.
                 for key, value in changes._to_set.items():
-                    if value is not None:
-                        env_list.append(f"{key}={value}")
-        return env_list
+                    if key in changes._openjd_env_names:
+                        effective[key] = value
+        return [f"{key}={value}" for key, value in effective.items() if value is not None]
 
     def _resolve_action_timeout(self, action: Any, symtab: SymbolTable) -> int:
         """Return the wrapped action's timeout as an int (seconds), or 0
@@ -1262,11 +1343,12 @@ def _inject_wrapped_cancelation_symbols(
         action's ``<Cancelation>`` (RFC 0008 follow-up,
         openjd-specifications#148).
 
-        ``Mode`` is ``"TERMINATE"``, ``"NOTIFY_THEN_TERMINATE"``, or the
-        empty string when the wrapped action defines no ``<Cancelation>`` —
-        the empty case is deliberately distinct from an explicit
-        ``TERMINATE`` so wrap scripts can tell "author declared TERMINATE"
-        apart from "author declared nothing".
+        ``Mode`` is typed ``string?``: ``"TERMINATE"``,
+        ``"NOTIFY_THEN_TERMINATE"``, or ``None`` (rendering as
+        ``null``/empty in format strings) when the wrapped action defines
+        no ``<Cancelation>`` — the null case is deliberately distinct from
+        an explicit ``TERMINATE`` so wrap scripts can tell "author declared
+        TERMINATE" apart from "author declared nothing".
 
         ``NotifyPeriodInSeconds`` is typed ``int?``: the effective grace
         period when the mode is ``NOTIFY_THEN_TERMINATE``, applying the
@@ -1278,10 +1360,10 @@ def _inject_wrapped_cancelation_symbols(
         applicable" is not conflated with a zero-length notify period.
         """
         cancelation = getattr(action, "cancelation", None)
-        mode: str
+        mode: Optional[str]
         notify_period: Optional[int]
         if cancelation is None:
-            mode = ""
+            mode = None
             notify_period = None
         elif cancelation.mode == CancelationMode_2023_09.TERMINATE:
             mode = CancelationMode_2023_09.TERMINATE.value
@@ -1290,7 +1372,11 @@ def _inject_wrapped_cancelation_symbols(
             mode = CancelationMode_2023_09.NOTIFY_THEN_TERMINATE.value
             period = getattr(cancelation, "notifyPeriodInSeconds", None)
             if period is None:
-                notify_period = 120 if is_task_run else 30
+                notify_period = (
+                    TASK_RUN_DEFAULT_NOTIFY_PERIOD_SECONDS
+                    if is_task_run
+                    else ENV_ACTION_DEFAULT_NOTIFY_PERIOD_SECONDS
+                )
             elif isinstance(period, int):
                 notify_period = period
             else:
@@ -1304,21 +1390,24 @@ def _inject_wrapped_env_symbols(
         symtab: SymbolTable,
         environment: EnvironmentModel,
         inner_action: Any,
+        session_env_list: Optional[list[str]] = None,
     ) -> None:
         """Populate ``WrappedAction.*`` and ``WrappedEnv.Name`` for
-        ``onWrapEnvEnter`` / ``onWrapEnvExit`` scripts (RFC 0008)."""
+        ``onWrapEnvEnter`` / ``onWrapEnvExit`` scripts (RFC 0008).
+
+        ``session_env_list`` overrides the collected openjd_env list when
+        the caller must capture it at a different point in time — the
+        ``onWrapEnvExit`` path collects it before the exiting environment
+        is removed from tracking, so the wrapped environment's own
+        ``openjd_env`` variables are included."""
         command = inner_action.command.resolve(symtab=symtab)
-        args = (
-            [a.resolve(symtab=symtab) for a in inner_action.args]
-            if inner_action.args
-            else []
-        )
+        args = [a.resolve(symtab=symtab) for a in inner_action.args] if inner_action.args else []
         symtab["WrappedAction.Command"] = command
         symtab["WrappedAction.Args"] = args
-        symtab["WrappedAction.Environment"] = self._collect_session_env_list()
-        symtab["WrappedAction.Timeout"] = self._resolve_action_timeout(
-            inner_action, symtab
+        symtab["WrappedAction.Environment"] = (
+            session_env_list if session_env_list is not None else self._collect_session_env_list()
         )
+        symtab["WrappedAction.Timeout"] = self._resolve_action_timeout(inner_action, symtab)
         self._inject_wrapped_cancelation_symbols(symtab, inner_action, is_task_run=False)
         symtab["WrappedEnv.Name"] = environment.name
 
@@ -1339,9 +1428,7 @@ def _inject_wrapped_task_symbols(
 
         symtab["WrappedAction.Command"] = on_run.command.resolve(symtab=symtab)
         symtab["WrappedAction.Args"] = (
-            [arg.resolve(symtab=symtab) for arg in on_run.args]
-            if on_run.args
-            else []
+            [arg.resolve(symtab=symtab) for arg in on_run.args] if on_run.args else []
         )
         symtab["WrappedAction.Environment"] = self._collect_session_env_list()
         symtab["WrappedAction.Timeout"] = self._resolve_action_timeout(on_run, symtab)
@@ -1526,6 +1613,26 @@ def _action_log_filter_callback(
             assert action_status is not None
             self._callback(self._session_id, action_status)
 
+    def _fail_action_before_start(self, message: str) -> None:
+        """Mark the pending action as FAILED before any runner/subprocess
+        exists (RFC 0008: e.g. when resolving the wrapped action's format
+        strings for ``WrappedAction.*`` injection fails).
+
+        Mirrors the failure branch of :meth:`_action_callback` — the
+        session transitions to READY_ENDING so the caller can exit the
+        entered environments — but does not require ``self._runner``.
+        """
+        self._logger.error(message)
+        self._action_fail_message = message
+        self._action_exit_code = None
+        self._action_state = ActionState.FAILED
+        self._state = SessionState.READY_ENDING
+        if self._callback:
+            action_status = self.action_status
+            # for the type checker
+            assert action_status is not None
+            self._callback(self._session_id, action_status)
+
     def _action_callback(self, state: ActionState) -> None:
         """This callback is invoked:
         1. When the Action process is successfully started, by the same thread that is running the

src/openjd/sessions/_types.py

@@ -20,6 +20,14 @@
 EnvironmentModel = Environment_2023_09
 EnvironmentScriptModel = EnvironmentScript_2023_09
 
+# Default notifyPeriodInSeconds for a NOTIFY_THEN_TERMINATE cancelation
+# when the action omits the field (2023-09 Template Schemas 5.3.2).
+TASK_RUN_DEFAULT_NOTIFY_PERIOD_SECONDS = 120
+"""Default notify period for a Step Script's onRun action."""
+ENV_ACTION_DEFAULT_NOTIFY_PERIOD_SECONDS = 30
+"""Default notify period for any other action (e.g. an Environment's
+onEnter/onExit)."""
+
 
 class ActionState(str, Enum):
     RUNNING = "running"