fix: Address review findings — clean errors, Step.Name threading

Review fixes on top of the RFC 0007/0008 CLI commit:

- Exceptions raised out of session actions (the RFC 0008 "at most one
  wrap environment" RuntimeError, expression ValueErrors) now surface
  as a clean error-status result instead of a raw traceback: guarded in
  run_environment_enters (routed through LocalSessionFailed) with a
  belt-and-suspenders catch in _run_local_session.
- The enter-failure rollback only drops the environment from the CLI's
  entered list when the session did not register it — a post-
  registration raise (e.g. a failing `variables` expression) keeps the
  env tracked so cleanup exits it in LIFO order instead of masking the
  original error with "Must exit Environment X first".
- Thread step_name into Session.enter_environment for step-environment
  enters so Step.Name (RFC 0007 §7.3.1) resolves in step-level `let`
  bindings and step-env actions — Rust CLI parity (openjd-rs
  run/mod.rs threads the step's resolved symtab). Capability-gated on
  the installed openjd-sessions accepting the kwarg.
- run_step's redundant pass-kwarg-only-when-truthy branch collapsed
  (the compat guard lives in EnterEnvironmentAction.run); version-skew
  getattr documented with its removal condition.

Tests: 294 passed under plain hatch and against the editable fixed
model/sessions trees (new: Step.Name end-to-end template, step_name
propagation, post-registration rollback, clean-error-result).
ruff/black/mypy clean.

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

Author: leongdl

src/openjd/cli/_run/_local_session/_actions.py

@@ -1,12 +1,22 @@
 # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 
+import inspect
 from enum import Enum
-from typing import Optional
+from typing import Any, Optional
 
 from openjd.model import Step, TaskParameterSet
 from openjd.model.v2023_09 import Environment
 from openjd.sessions import Session
 
+# Version-skew guard: the step_name keyword was added to
+# Session.enter_environment alongside RFC 0007 Step.Name support. Detect it
+# once so this CLI keeps working against older openjd-sessions releases that
+# don't accept the keyword (every step-environment enter carries a step name,
+# so a value-presence check alone isn't enough here).
+_ENTER_ENVIRONMENT_ACCEPTS_STEP_NAME: bool = (
+    "step_name" in inspect.signature(Session.enter_environment).parameters
+)
+
 
 class EnvironmentType(str, Enum):
     """
@@ -64,39 +74,49 @@ class EnterEnvironmentAction(SessionAction):
     _environment: Environment
     _id: str
     _extra_let_bindings: Optional[list[str]]
+    _step_name: Optional[str]
 
     def __init__(
         self,
         session: Session,
         environment: Environment,
         env_id: str,
         extra_let_bindings: Optional[list[str]] = None,
+        step_name: Optional[str] = None,
     ):
         super(EnterEnvironmentAction, self).__init__(session)
         self._environment = environment
         self._id = env_id
         # RFC 0007: a step's environments are entered with the step-level
         # `let` bindings so their variables/actions can reference them.
         self._extra_let_bindings = extra_let_bindings
+        # RFC 0007 §7.3.1 (EXPR): the owning step's name seeds Step.Name for
+        # a step environment's `let` bindings, variables, and actions. Only
+        # step-environment enters carry a step name; job/external enters
+        # leave it None.
+        self._step_name = step_name
 
     def run(self):
         # Backwards compatibility: only forward `extra_let_bindings` when the
         # step actually defines `let` bindings (RFC 0007). Older
         # openjd-sessions releases don't accept the keyword, so omitting it
         # by default keeps this CLI working with any sessions version for
         # every template that doesn't use step-level lets — the reasonable
-        # default is simply "no extra bindings".
+        # default is simply "no extra bindings". `step_name` follows the same
+        # pattern, but since every step-environment enter has a step name, it
+        # is additionally gated on the installed openjd-sessions accepting
+        # the keyword; without it, Step.Name simply stays undefined (the
+        # pre-RFC 0007 behavior).
+        optional_kwargs: dict[str, Any] = {}
         if self._extra_let_bindings:
-            self._session.enter_environment(
-                environment=self._environment,
-                identifier=self._id,
-                extra_let_bindings=self._extra_let_bindings,
-            )
-        else:
-            self._session.enter_environment(
-                environment=self._environment,
-                identifier=self._id,
-            )
+            optional_kwargs["extra_let_bindings"] = self._extra_let_bindings
+        if self._step_name is not None and _ENTER_ENVIRONMENT_ACCEPTS_STEP_NAME:
+            optional_kwargs["step_name"] = self._step_name
+        self._session.enter_environment(
+            environment=self._environment,
+            identifier=self._id,
+            **optional_kwargs,
+        )
 
     def __str__(self):
         return f"Enter Environment '{self._environment.name}'"

src/openjd/cli/_run/_local_session/_session_manager.py

@@ -205,6 +205,7 @@ def run_environment_enters(
         type: EnvironmentType,
         *,
         extra_let_bindings: Optional[list[str]] = None,
+        step_name: Optional[str] = None,
     ):
         """Enter one or more environments in the session."""
         if environments is None:
@@ -225,9 +226,37 @@ def run_environment_enters(
                 # RFC 0007: a step's environments see the step-level `let`
                 # bindings.
                 extra_let_bindings=extra_let_bindings,
+                # RFC 0007 §7.3.1 (EXPR): a step's environments see Step.Name.
+                # Only step-environment enters carry a step name.
+                step_name=step_name,
             )
             self._environments_entered.append((type, env_id))
-            self._current_action.run()
+            try:
+                self._current_action.run()
+            except (RuntimeError, ValueError) as exc:
+                # Session.enter_environment raises (rather than reporting
+                # through the action-status callback) when it rejects the
+                # environment up front — e.g. the RFC 0008 "at most one wrap
+                # environment" RuntimeError, or a ValueError from the extra
+                # `let` bindings — but it can also raise *after* registering
+                # the environment (e.g. an environment `variables` expression
+                # that fails to evaluate). Only when the session did NOT
+                # register the environment may we drop it from our entered
+                # list (cleanup must not try to exit it). If the session did
+                # register it, it must stay in our list so cleanup exits it —
+                # popping it here would skip its onExit and desynchronize us
+                # from the session's LIFO exit ordering check, masking the
+                # original error with "Must exit Environment X first".
+                if env_id not in self._openjd_session.environments_entered:
+                    self._environments_entered.pop()
+                LOG.info(
+                    msg=f"Open Job Description CLI: ERROR entering environment '{env.name}': {exc}",
+                    extra={"session_id": self.session_id},
+                )
+                self.failed = True
+                self._failed_action = self._current_action
+                self._current_action = None
+                raise LocalSessionFailed(self._failed_action) from exc
             self._action_ended.wait()
             if self.failed:
                 self._failed_action = self._current_action
@@ -363,18 +392,20 @@ def run_step(
 
         # Enter all the step environments. When the step defines step-level
         # `let` bindings (RFC 0007), its environments are entered with them so
-        # their variables and actions can reference them. The keyword is only
-        # passed when bindings exist, keeping the call (and the sessions API
-        # it reaches) identical to the pre-RFC 0007 behavior by default.
+        # their variables and actions can reference them.
+        # getattr guard: requires an openjd-model with Step.let on the
+        # instantiated Job (openjd-model PR #318+); collapse to plain
+        # `step.let` once the version pin floor guarantees it.
         step_let_bindings = getattr(step, "let", None)
-        if step_let_bindings:
-            self.run_environment_enters(
-                step.stepEnvironments,
-                EnvironmentType.STEP,
-                extra_let_bindings=step_let_bindings,
-            )
-        else:
-            self.run_environment_enters(step.stepEnvironments, EnvironmentType.STEP)
+        self.run_environment_enters(
+            step.stepEnvironments,
+            EnvironmentType.STEP,
+            extra_let_bindings=step_let_bindings or None,
+            # RFC 0007 §7.3.1 (EXPR): Step.Name is available to a step's
+            # environments (openjd-rs threads the step's resolved symbol
+            # table into enter_environment; this is the CLI counterpart).
+            step_name=step.name,
+        )
 
         try:
             # Run the tasks

src/openjd/cli/_run/_run_command.py

@@ -338,6 +338,7 @@ def _run_local_session(
     Creates a Session object and listens for log messages to synchronously end the session.
     """
 
+    error_message = "Session ended with errors; see Task logs for details"
     try:
         start_seconds = time.perf_counter()
 
@@ -367,6 +368,14 @@ def _run_local_session(
     except LocalSessionFailed:
         duration = time.perf_counter() - start_seconds
         session = None
+    except (RuntimeError, ValueError) as exc:
+        # Exceptions raised by openjd.sessions from within session actions
+        # (e.g. the RFC 0008 "at most one wrap environment" RuntimeError)
+        # rather than reported through the action-status callback. Report
+        # them as a clean error result instead of a raw traceback.
+        duration = time.perf_counter() - start_seconds
+        session = None
+        error_message = f"Session ended with errors: {exc}"
 
     preserved_message: str = ""
     if retain_working_dir and session is not None:
@@ -377,7 +386,7 @@ def _run_local_session(
     if session is None or session.failed:
         return OpenJDRunResult(
             status="error",
-            message="Session ended with errors; see Task logs for details" + preserved_message,
+            message=error_message + preserved_message,
             job_name=job.name,
             step_name=step_name,
             duration=duration,

test/openjd/cli/templates/step_name_env_job.yaml

@@ -0,0 +1,27 @@
+# RFC 0007 §7.3.1 (EXPR): Step.Name is available to a step's environments.
+# The step-level `let` binding references Step.Name, and the step's
+# environment is entered with the binding so its onEnter action can echo it.
+specificationVersion: jobtemplate-2023-09
+extensions:
+- EXPR
+name: StepNameEnvJob
+steps:
+- name: EchoStepName
+  let:
+  - bound_name = Step.Name
+  stepEnvironments:
+  - name: NameEcho
+    script:
+      actions:
+        onEnter:
+          command: python
+          args:
+          - -c
+          - print('EnvSaw={{ bound_name }}')
+  script:
+    actions:
+      onRun:
+        command: python
+        args:
+        - -c
+        - print('TaskRan')

test/openjd/cli/test_local_session.py

@@ -7,6 +7,7 @@
 from . import SampleSteps, SESSION_PARAMETERS
 from openjd.model import StepParameterSpaceIterator
 from openjd.sessions import Session, SessionState
+from openjd.cli._run._local_session._actions import _ENTER_ENVIRONMENT_ACCEPTS_STEP_NAME
 from openjd.cli._run._local_session._session_manager import (
     LocalSession,
     EnvironmentType,
@@ -168,7 +169,13 @@ def test_localsession_run_success(
         assert patched_run_environment_enters.call_args_list == [
             call(session, None, EnvironmentType.EXTERNAL),
             call(session, sample_job.jobEnvironments, EnvironmentType.JOB),
-            call(session, sample_job.steps[step_index].stepEnvironments, EnvironmentType.STEP),
+            call(
+                session,
+                sample_job.steps[step_index].stepEnvironments,
+                EnvironmentType.STEP,
+                extra_let_bindings=None,
+                step_name=sample_job.steps[step_index].name,
+            ),
         ]
         # It should have run one step
         assert patched_run_step.call_args_list == [
@@ -193,6 +200,115 @@ def test_localsession_run_success(
         )
 
 
+@pytest.mark.usefixtures("sample_job_and_dirs")
+def test_localsession_step_env_enter_receives_step_name(
+    sample_job_and_dirs: tuple,
+    patched_actions,
+):
+    """
+    RFC 0007 §7.3.1 (EXPR): a step-environment enter passes the owning step's
+    name to Session.enter_environment (when the installed openjd-sessions
+    accepts the keyword), while job/external environment enters never do.
+    """
+    sample_job, sample_job_parameters, template_dir, current_working_dir = sample_job_and_dirs
+    patched_enter = patched_actions[0]
+
+    with LocalSession(
+        job=sample_job, job_parameter_values=sample_job_parameters, session_id="step-name"
+    ) as session:
+        session.run_step(sample_job.steps[SampleSteps.NormalStep])
+
+    assert not session.failed
+
+    step_env_calls = [
+        c
+        for c in patched_enter.call_args_list
+        if c.kwargs["identifier"].startswith(f"{EnvironmentType.STEP.name} - ")
+    ]
+    other_env_calls = [
+        c
+        for c in patched_enter.call_args_list
+        if not c.kwargs["identifier"].startswith(f"{EnvironmentType.STEP.name} - ")
+    ]
+
+    assert step_env_calls
+    for enter_call in step_env_calls:
+        if _ENTER_ENVIRONMENT_ACCEPTS_STEP_NAME:
+            assert enter_call.kwargs["step_name"] == sample_job.steps[SampleSteps.NormalStep].name
+        else:
+            # Older openjd-sessions releases don't accept the keyword; the
+            # version-skew guard must omit it.
+            assert "step_name" not in enter_call.kwargs
+
+    # Job/external environment enters never carry a step name.
+    assert other_env_calls
+    for enter_call in other_env_calls:
+        assert "step_name" not in enter_call.kwargs
+
+
+@pytest.mark.usefixtures("sample_job_and_dirs", "capsys")
+def test_localsession_enter_environment_post_registration_raise(
+    sample_job_and_dirs: tuple, capsys: pytest.CaptureFixture, patched_actions
+):
+    """
+    A raise out of Session.enter_environment *after* the session registered
+    the environment (e.g. an environment `variables` expression that fails to
+    evaluate) must leave the environment in the CLI's entered list so cleanup
+    exits it. Popping it would desynchronize CLI and session state: the
+    environment's onExit would be skipped and cleanup would trip the session's
+    LIFO exit check ("Must exit Environment X first"), masking the original
+    error.
+    """
+    sample_job, sample_job_parameters, template_dir, current_working_dir = sample_job_and_dirs
+    patched_enter = patched_actions[0]
+
+    # The autouse fixture set the mock's side_effect to the real (pre-patch)
+    # Session.enter_environment; capture it before redirecting the mock.
+    real_enter = patched_enter.side_effect
+    error_text = "Failed to evaluate the environment's variables"
+
+    def register_then_raise(session, *, environment, identifier=None, **kwargs):
+        if identifier is not None and identifier.startswith(f"{EnvironmentType.STEP.name} - "):
+            # Mirror the state Session.enter_environment leaves behind when an
+            # environment `variables` expression fails to evaluate: the
+            # environment is registered in the session's entered list, but the
+            # enter raises before any action runs.
+            session._environments[identifier] = environment
+            session._environments_entered.append(identifier)
+            raise ValueError(error_text)
+        return real_enter(session, environment=environment, identifier=identifier, **kwargs)
+
+    step_env_id = f"{EnvironmentType.STEP.name} - env1"
+    job_env_id = f"{EnvironmentType.JOB.name} - rootEnv"
+
+    # Redirect the autouse fixture's Session.enter_environment mock from the
+    # real method to the registering-then-raising simulation.
+    patched_enter.side_effect = register_then_raise
+    with LocalSession(
+        job=sample_job, job_parameter_values=sample_job_parameters, session_id="post-reg"
+    ) as session:
+        with pytest.raises(LocalSessionFailed):
+            session.run_step(sample_job.steps[SampleSteps.NormalStep])
+
+        # The session registered the environment, so the CLI must keep it
+        # in its own entered list for cleanup to exit.
+        assert step_env_id in session._openjd_session.environments_entered
+        assert (EnvironmentType.STEP, step_env_id) in session._environments_entered
+
+    # Cleanup exited the registered step environment and then the job
+    # environment, in LIFO order, rather than skipping the step environment.
+    exited_ids = [
+        exit_call.kwargs["identifier"]
+        for exit_call in session._openjd_session.exit_environment.call_args_list  # type: ignore
+    ]
+    assert exited_ids == [step_env_id, job_env_id]
+
+    assert session.failed
+    output = capsys.readouterr().out
+    assert error_text in output
+    assert "Must exit Environment" not in output
+
+
 @pytest.mark.usefixtures("sample_job_and_dirs", "capsys")
 def test_localsession_run_failed(sample_job_and_dirs: tuple, capsys: pytest.CaptureFixture):
     """
@@ -221,6 +337,8 @@ def test_localsession_run_failed(sample_job_and_dirs: tuple, capsys: pytest.Capt
                 session,
                 sample_job.steps[SampleSteps.BadCommand].stepEnvironments,
                 EnvironmentType.STEP,
+                extra_let_bindings=None,
+                step_name=sample_job.steps[SampleSteps.BadCommand].name,
             ),
         ]
         session._openjd_session.exit_environment.assert_called_once()  # type: ignore