diff --git a/src/openjd/cli/_run/_local_session/_actions.py b/src/openjd/cli/_run/_local_session/_actions.py index d8b757c..7c8d013 100644 --- a/src/openjd/cli/_run/_local_session/_actions.py +++ b/src/openjd/cli/_run/_local_session/_actions.py @@ -1,11 +1,22 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +import inspect from enum import Enum +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): """ @@ -47,7 +58,11 @@ def __init__(self, session: Session, step: Step, parameters: TaskParameterSet): def run(self): self._session.run_task( - step_script=self._step.script, task_parameter_values=self._parameters + step_script=self._step.script, + task_parameter_values=self._parameters, + # RFC 0008: the step name feeds the WrappedStep.Name template + # variable inside an active onWrapTaskRun hook. + step_name=self._step.name, ) def __str__(self): @@ -58,14 +73,50 @@ def __str__(self): class EnterEnvironmentAction(SessionAction): _environment: Environment _id: str - - def __init__(self, session: Session, environment: Environment, env_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): - self._session.enter_environment(environment=self._environment, identifier=self._id) + # 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". `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: + 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}'" diff --git a/src/openjd/cli/_run/_local_session/_session_manager.py b/src/openjd/cli/_run/_local_session/_session_manager.py index 2413505..344180a 100644 --- a/src/openjd/cli/_run/_local_session/_session_manager.py +++ b/src/openjd/cli/_run/_local_session/_session_manager.py @@ -102,6 +102,8 @@ def __init__( self._openjd_session = Session( session_id=self.session_id, job_parameter_values=job_parameter_values, + # RFC 0007 §7.3.1 (EXPR): seeds the Job.Name template variable. + job_name=str(job.name), path_mapping_rules=self._path_mapping_rules, callback=self._action_callback, retain_working_dir=retain_working_dir, @@ -197,7 +199,14 @@ def _sigint_handler(self, signum: int, frame: Optional[FrameType]) -> None: LOG.info("Interruption signal recieved.") self.cancel() - def run_environment_enters(self, environments: Optional[list[Any]], type: EnvironmentType): + def run_environment_enters( + self, + environments: Optional[list[Any]], + 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: return @@ -211,10 +220,43 @@ def run_environment_enters(self, environments: Optional[list[Any]], type: Enviro env_id = f"{type.name} - {env.name}" self._action_ended.clear() self._current_action = EnterEnvironmentAction( - session=self._openjd_session, environment=env, env_id=env_id + session=self._openjd_session, + environment=env, + env_id=env_id, + # 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 @@ -348,8 +390,22 @@ def run_step( if task_parameters is None: task_parameters = StepParameterSpaceIterator(space=step.parameterSpace) - # Enter all the step environments - self.run_environment_enters(step.stepEnvironments, EnvironmentType.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. + # 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) + 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 diff --git a/src/openjd/cli/_run/_run_command.py b/src/openjd/cli/_run/_run_command.py index 0672780..6f1b837 100644 --- a/src/openjd/cli/_run/_run_command.py +++ b/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, @@ -547,9 +556,17 @@ def do_run(args: Namespace) -> OpenJDCliResult: return OpenJDCliResult(status="error", message=str(rte)) # Create a RevisionExtensions object with the default specification version and enabled extensions - # We use the default v2023_09 since that's what we're currently supporting + # We use the default v2023_09 since that's what we're currently supporting. + # Extension behaviors activate only when declared by the templates being + # run (the job template and any external environment templates) — the + # CLI's --extensions list is what the CLI *accepts*, not what is enabled. + declared_extensions: list[str] = list(the_job.extensions or []) + for env_template in environments or []: + for ext_name in env_template.extensions or []: + if ext_name not in declared_extensions: + declared_extensions.append(ext_name) revision_extensions = RevisionExtensions( - spec_rev=the_job.revision, supported_extensions=extensions + spec_rev=the_job.revision, supported_extensions=declared_extensions ) return _run_local_session( diff --git a/test/openjd/cli/templates/step_name_env_job.yaml b/test/openjd/cli/templates/step_name_env_job.yaml new file mode 100644 index 0000000..daf10a1 --- /dev/null +++ b/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') diff --git a/test/openjd/cli/test_local_session.py b/test/openjd/cli/test_local_session.py index 2a23847..8ccc245 100644 --- a/test/openjd/cli/test_local_session.py +++ b/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 diff --git a/test/openjd/cli/test_run_command.py b/test/openjd/cli/test_run_command.py index df0d656..e8c5e01 100644 --- a/test/openjd/cli/test_run_command.py +++ b/test/openjd/cli/test_run_command.py @@ -11,7 +11,7 @@ import shlex import pytest -from unittest.mock import Mock +from unittest.mock import Mock, patch from . import MOCK_TEMPLATE, SampleSteps, run_openjd_cli_main, format_capsys_outerr @@ -20,8 +20,9 @@ _process_task_params, _process_tasks, ) +from openjd.cli._run._local_session._actions import _ENTER_ENVIRONMENT_ACCEPTS_STEP_NAME from openjd.cli._run._local_session._session_manager import LoggingTimestampFormat -from openjd.sessions import LOG as SessionsLogger, PathMappingRule, PathFormat +from openjd.sessions import LOG as SessionsLogger, PathMappingRule, PathFormat, Session PARAMETRIZE_CASES: tuple = ( pytest.param( @@ -642,6 +643,61 @@ def test_run_local_session_failed( ), f"Regex r'{expected_error_regex}' does not match the output:\n{format_capsys_outerr(outerr)}" +def test_run_local_session_enter_environment_raises(capsys: pytest.CaptureFixture): + """ + Test that an exception raised out of Session.enter_environment (e.g. the + RFC 0008 "at most one wrap environment" RuntimeError, or a ValueError from + the extra `let` bindings) produces a clean error result instead of a raw + traceback propagating out of the CLI. + """ + template_dir = Path(__file__).parent / "templates" + args = [ + "run", + str(template_dir / "job_with_test_steps.yaml"), + "--step", + SampleSteps.NormalStep.name, + "--extensions", + "", + ] + + error_text = ( + "RFC 0008: a session may have at most one Environment defining wrap hooks " + "(onWrapEnvEnter / onWrapTaskRun / onWrapEnvExit)." + ) + with patch.object(Session, "enter_environment", side_effect=RuntimeError(error_text)): + outerr = run_openjd_cli_main(capsys, args=args, expected_exit_code=1) + + assert re.search( + "Session ended with errors", outerr.out, re.MULTILINE + ), f"Expected a clean error result in the output:\n{format_capsys_outerr(outerr)}" + assert error_text in outerr.out + assert "Traceback" not in outerr.out + outerr.err + + +@pytest.mark.skipif( + not _ENTER_ENVIRONMENT_ACCEPTS_STEP_NAME, + reason="Installed openjd-sessions does not accept step_name on enter_environment", +) +def test_do_run_step_name_in_step_environment(capsys: pytest.CaptureFixture) -> None: + """ + RFC 0007 §7.3.1 (EXPR) parity with openjd-rs: a step-level `let` binding + may reference Step.Name, and the step's environments are entered with the + binding so their actions can echo it. + """ + template_dir = Path(__file__).parent / "templates" + args = [ + "run", + str(template_dir / "step_name_env_job.yaml"), + "--step", + "EchoStepName", + ] + outerr = run_openjd_cli_main(capsys, args=args, expected_exit_code=0) + assert ( + "EnvSaw=EchoStepName" in outerr.out + ), f"Step.Name did not resolve in the step environment:\n{format_capsys_outerr(outerr)}" + assert "TaskRan" in outerr.out + + class TestProcessTaskParams: """Testing that we properly handle the values of the --task-param/-tp command-line argument"""