From 82c3be77f0e513d176f923f920fc53d6f808e37d Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 24 Aug 2026 22:35:48 -0700 Subject: [PATCH 1/9] feat: deliver step-scope let bindings to run_task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Session.run_task` had no channel for step-template-scope `let` bindings (RFC 0005 §3.6), so a step script referencing one failed at resolve time with `Undefined variable`. `enter_environment` has accepted `extra_let_bindings` since #333; this adds the same parameter to `run_task`. Why the gap was invisible: step-scope bindings resolve at job instantiation, and `StepTemplate.resolve_syntax_sugar` folds them into the script's own `let` so they survive into the `Job`. Any caller holding a `Job` from `create_job` — openjd-cli, and every test in this repo — therefore never sees the problem. A caller handed an *un-instantiated* `StepTemplate`, where `let` and `script.let` are still separate fields, has no way to deliver them at all. That is the Deadline Cloud worker agent, which receives one from the service; the symptom there was 32 conformance execution cases failing with `Undefined variable` on names their templates plainly define. Ordering matches `enter_environment` exactly: seeded after `Step.Name`, so a step binding may reference it, and before path mapping and env-var evaluation, so both see a complete table. Script-scope bindings shadow step-scope ones rather than colliding, for free — `StepScriptRunner` evaluates `script.let` into a child table sourced from the session-scope one. Wrap-hook isolation is unaffected: `_build_wrap_hook_scope` builds a fresh table, so step bindings reach a wrapped `onRun` but never the hook, which is what RFC 0008 requires. A failing binding fails the action through `_fail_action_before_start` rather than raising out of the public API, the same contract `enter_environment` holds. The parameter is additive and optional, so existing callers are unaffected. 6 tests, mutation-checked 3 of 3 caught: dropping the apply, seeding before `Step.Name`, and dropping the try/except. Coverage includes the negative control that omitting the parameter changes nothing, and that a script-scope binding can build on a step-scope one — the shape the failing fixtures use. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 42 +++++ .../sessions_v0/test_session_let_bindings.py | 168 ++++++++++++++++++ 2 files changed, 210 insertions(+) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 7be303c6..1ba51128 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -1210,6 +1210,7 @@ def run_task( os_env_vars: Optional[dict[str, str]] = None, log_task_banner: bool = True, step_name: Optional[str] = None, + extra_let_bindings: Optional[list[str]] = None, ) -> None: """Run a Task within the Session. This method is non-blocking; it will exit when the subprocess is either confirmed to have @@ -1231,6 +1232,22 @@ def run_task( step_name (Optional[str]): The name of the step whose task is being run. Used by RFC 0008 to populate ``WrappedStep.Name`` in wrap hooks. Required when a wrap Environment is active. + extra_let_bindings (Optional[list[str]]): Additional EXPR ``let`` + bindings (RFC 0005) evaluated into the symbol table before the + step script's own bindings and actions resolve. This is the + step-template-scope ``let`` (``Step.let`` on the instantiated + Job), which resolves at job instantiation and so is not part of + the step script — the ``run_task`` counterpart of + :meth:`enter_environment`'s parameter of the same name, and the + v0 counterpart of the per-step resolved symbol table that + openjd-rs threads into ``run_task``. + + A caller that obtained its step script from + ``create_job`` does not need this: job instantiation folds the + step-scope bindings into the script's own ``let`` + (``StepTemplate.resolve_syntax_sugar``). It is required by a + caller that is handed an *un-instantiated* ``StepTemplate``, + where ``let`` and ``script.let`` are still separate fields. Raises: RuntimeError: If the Session is not in the READY state. @@ -1280,6 +1297,31 @@ def run_task( # not change non-EXPR behavior. if step_name is not None: symtab["Step.Name"] = step_name + + # Step-template-scope `let` bindings (RFC 0005 §3.6) accompany the task: + # evaluate them into the session-scope table so the step script's own + # bindings and its actions can reference them. Seeded after Step.Name so + # a step-level binding may reference it, and before path mapping so + # {{Session.PathMappingRulesFile}} and the env-var evaluation below see a + # complete table -- the same ordering enter_environment uses. + # + # Script-scope bindings shadow these rather than colliding with them: + # StepScriptRunner evaluates `script.let` into a CHILD table sourced from + # this one, so a same-named script binding takes precedence. + if extra_let_bindings: + try: + apply_let_bindings(symtab=symtab, let_bindings=extra_let_bindings) + except ValueError as e: + # ExpressionError and FormatStringError subclass ValueError: a + # binding failed to evaluate (e.g. it referenced an undefined + # symbol). Fail the action through the normal failure path + # rather than raising out of the public API, matching how + # enter_environment reports the same failure. + self._fail_action_before_start( + f"Failed to evaluate the extra `let` bindings for the task: {e}" + ) + return + action_env_vars = self._evaluate_current_session_env_vars(os_env_vars) try: self._materialize_path_mapping(step_script.revision, action_env_vars, symtab) diff --git a/test/openjd/sessions_v0/test_session_let_bindings.py b/test/openjd/sessions_v0/test_session_let_bindings.py index 2b3c5dd0..3061d588 100644 --- a/test/openjd/sessions_v0/test_session_let_bindings.py +++ b/test/openjd/sessions_v0/test_session_let_bindings.py @@ -207,6 +207,174 @@ def test_step_name_resolvable_in_bindings_and_actions( assert any("exit:step is MyStep" in m for m in caplog.messages) +# --------------------------------------------------------------------------- +# run_task(extra_let_bindings=...) delivers step-template-scope `let` +# (RFC 0005 §3.6) to the task, the counterpart of enter_environment's +# parameter of the same name. +# +# Why this needs its own coverage: step-scope bindings resolve at job +# instantiation, so a caller holding a Job from create_job never sees the +# problem -- instantiation folds them into the script's own `let`. A caller +# handed an un-instantiated StepTemplate (the Deadline Cloud worker agent, +# which receives one from the service) has `let` and `script.let` as separate +# fields, and without this parameter the step-scope names are simply absent +# from the table and every reference fails with "Undefined variable". +# --------------------------------------------------------------------------- + + +class TestRunTaskExtraLetBindings: + def test_step_scope_binding_resolvable_in_action( + self, caplog: pytest.LogCaptureFixture + ) -> None: + # GIVEN: a step script whose onRun references a name defined only by + # the step-template-scope bindings. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "task:{{ from_step }}")}, # type: ignore[arg-type] + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + extra_let_bindings=["from_step = 'step value'"], + ) + _run_until_ready(session) + + # THEN + assert session.state == SessionState.READY + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:step value" in m for m in caplog.messages) + + def test_step_scope_binding_can_reference_step_name( + self, caplog: pytest.LogCaptureFixture + ) -> None: + # GIVEN: a step-scope binding referencing Step.Name. Pins the seeding + # order -- Step.Name must be in the table before the bindings evaluate, + # matching enter_environment. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "task:{{ msg }}")}, # type: ignore[arg-type] + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + step_name="MyStep", + extra_let_bindings=["msg = 'step is ' + Step.Name"], + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:step is MyStep" in m for m in caplog.messages) + + def test_script_scope_binding_shadows_step_scope( + self, caplog: pytest.LogCaptureFixture + ) -> None: + # GIVEN: the same name bound at both scopes. RFC 0005 §3.6 scoping + # requires the narrower (script) scope to win rather than the two + # colliding, which holds because the runner evaluates script bindings + # into a child table sourced from the session-scope one. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "task:{{ shared }}")}, # type: ignore[arg-type] + let=["shared = 'from script'"], + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + extra_let_bindings=["shared = 'from step'"], + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:from script" in m for m in caplog.messages) + assert not any("task:from step" in m for m in caplog.messages) + + def test_script_scope_binding_can_reference_step_scope( + self, caplog: pytest.LogCaptureFixture + ) -> None: + # GIVEN: a script-scope binding building on a step-scope one. This is + # the shape the failing conformance fixtures use, and it only works if + # the step bindings are in the parent of the runner's child table. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "task:{{ derived }}")}, # type: ignore[arg-type] + let=["derived = base + '/leaf'"], + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + extra_let_bindings=["base = '/root'"], + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:/root/leaf" in m for m in caplog.messages) + + def test_failing_binding_fails_action_cleanly(self) -> None: + # GIVEN: a step-scope binding referencing an undefined symbol. It must + # fail the action through the callback path, never raise out of the + # public API -- the same contract enter_environment holds. + callback_events: list[ActionStatus] = [] + + def callback(session_id: str, status: ActionStatus) -> None: + callback_events.append(status) + + script = StepScript_2023_09( + actions={"onRun": _action("echo", "unreachable")}, # type: ignore[arg-type] + ) + with Session( + session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback + ) as session: + # WHEN: this must not raise. + session.run_task( + step_script=script, + task_parameter_values={}, + extra_let_bindings=["msg = NoSuchSymbol"], + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.FAILED + assert status.fail_message is not None + assert "let" in status.fail_message + assert callback_events and callback_events[-1].state == ActionState.FAILED + + def test_omitting_the_parameter_changes_nothing(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: the negative control. The parameter is additive and optional, + # so a task that does not use it must behave exactly as before -- this + # is what makes the change safe for every existing caller. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "task:{{ own }}")}, # type: ignore[arg-type] + let=["own = 'script only'"], + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task(step_script=script, task_parameter_values={}) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:script only" in m for m in caplog.messages) + + # --------------------------------------------------------------------------- # Binding-RHS parsing is memoized: re-applying the same bindings (per task, # per env enter/exit) must not re-parse through the engine each time. From f82bb940a088f366e95de3611311c9ea6fb807dc Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:49:40 -0700 Subject: [PATCH 2/9] feat: Accept resolved symbol table on v0 session The service resolves a per-step symbol table at CreateJob (Param.*, RawParam.*, Job.Name, Step.Name, step-level `let` values) and serves it as resolvedSymbolTable, but only the Rust-backed _v1 session could accept it -- the default v0 Python runtime had no channel for it, so job-template-scope `let` never reached v0 and every service-resolved symbol was re-derived or missing on the default runtime. Mirror the _v1 surface: enter_environment, exit_environment, and run_task gain resolved_symtab (a SerializedSymbolTable). Its entries seed the session symbol table first and the session's own values layer on top, matching the openjd-rs layering: Session.WorkingDirectory and the path-mapped Param.* values overwrite the base, Job.Name from the base wins over the constructor value, and script-scope `let` still shadows base symbols. A base that fails entry validation fails the action through the normal callback path instead of raising out of the public API. Extension purity holds: the only runtime import of openjd.expr is inside the conversion helper, reachable only when the caller already holds a native SerializedSymbolTable. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 136 +++++- .../test_session_resolved_symtab.py | 445 ++++++++++++++++++ 2 files changed, 575 insertions(+), 6 deletions(-) create mode 100644 test/openjd/sessions_v0/test_session_resolved_symtab.py diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 1ba51128..3dbf0782 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -69,6 +69,7 @@ from subprocess import HIGH_PRIORITY_CLASS # type: ignore if TYPE_CHECKING: + from openjd.expr import SerializedSymbolTable from openjd.model.v2023_09._model import EnvironmentVariableObject __all__ = ("SessionState", "Session", "EnvironmentIdentifier") @@ -759,6 +760,7 @@ def enter_environment( os_env_vars: Optional[dict[str, str]] = None, extra_let_bindings: Optional[list[str]] = None, step_name: Optional[str] = None, + resolved_symtab: Optional["SerializedSymbolTable"] = None, ) -> EnvironmentIdentifier: """Enters an Open Job Description Environment within this Session. This method is non-blocking; it will exit when the subprocess is either confirmed to have @@ -791,6 +793,16 @@ def enter_environment( and the environment's variables and actions can reference it — openjd-rs threads a per-step resolved symbol table into environment entry, and this is the v0 counterpart. + resolved_symtab (Optional[SerializedSymbolTable]): The step-scope + symbol table generated by ``create_job`` (available as + ``Step.resolved_symtab``). It contains ``Param.*``, + ``RawParam.*``, ``Job.Name``, ``Step.Name``, and the + step-level let-binding values. Its entries seed the session + symbol table first, and the session's own values + (``Session.WorkingDirectory``, path-mapped ``Param.*``) + layer on top — the layering the openjd-rs runtime applies to + the same table. ``None`` is fine when the environment script + doesn't reference any of those names. Returns: EnvironmentIdentifier: An identifier by which the Environment is known by to this Session. @@ -838,7 +850,28 @@ def enter_environment( self._environments_entered.append(identifier) self._running_environment_identifier = identifier - symtab = self._symbol_table(environment.revision) + # Deserialize the service-resolved base table (if given) before + # building the symbol table, so its entries seed first. + resolved_base: Optional[dict[str, Any]] = None + if resolved_symtab is not None: + try: + resolved_base = self._resolved_base_entries(resolved_symtab) + except ValueError as e: + # Same failure shape as the extra `let` bindings guard below: + # fail the action through the normal failure path rather than + # raising out of the public API. The environment is already on + # the entered list, so the caller's cleanup exits it as usual, + # and the empty change record keeps the log-forwarding + # thread's _created_env_vars lookup safe. + self._created_env_vars[identifier] = SimplifiedEnvironmentVariableChanges( + dict[str, str]() + ) + self._fail_action_before_start( + f"Failed to deserialize the resolved symbol table: {e}" + ) + return identifier + + symtab = self._symbol_table(environment.revision, resolved_base=resolved_base) # RFC 0005; Template Schemas §7.3.1 (EXPR): the owning step's name. Only EXPR templates # pass validation referencing Step.Name, so seeding it when known does @@ -1032,6 +1065,7 @@ def exit_environment( identifier: EnvironmentIdentifier, os_env_vars: Optional[dict[str, str]] = None, keep_session_running: bool = False, + resolved_symtab: Optional["SerializedSymbolTable"] = None, ) -> None: """Exits an Open Job Description Environment from this Session. This method is non-blocking; it will exit when the subprocess is either confirmed to have @@ -1052,6 +1086,10 @@ def exit_environment( keep_session_running (bool): This overrides the default of requiring only environment exits after the first exit_environment is called. The caller can set this to True in order to exit the environments of a step and then run tasks from a different step. + resolved_symtab (Optional[SerializedSymbolTable]): See + :meth:`enter_environment` for semantics. Pass the same table + the environment was entered with so its onExit resolves in + the same scope as its onEnter. Raises: RuntimeError: If the Session is not in the READY or READY_ENDING state; @@ -1099,7 +1137,23 @@ def exit_environment( self._running_environment_identifier = identifier - symtab = self._symbol_table(environment.revision) + # Deserialize the service-resolved base table (if given) before + # building the symbol table, so its entries seed first. + resolved_base: Optional[dict[str, Any]] = None + if resolved_symtab is not None: + try: + resolved_base = self._resolved_base_entries(resolved_symtab) + except ValueError as e: + # Fail the action through the normal failure path rather than + # raising out of the public API — the environment was already + # removed from tracking above, matching how the extra `let` + # bindings failure below leaves it. + self._fail_action_before_start( + f"Failed to deserialize the resolved symbol table: {e}" + ) + return + + symtab = self._symbol_table(environment.revision, resolved_base=resolved_base) try: self._materialize_path_mapping(environment.revision, action_env_vars, symtab) except RuntimeError as e: @@ -1211,6 +1265,7 @@ def run_task( log_task_banner: bool = True, step_name: Optional[str] = None, extra_let_bindings: Optional[list[str]] = None, + resolved_symtab: Optional["SerializedSymbolTable"] = None, ) -> None: """Run a Task within the Session. This method is non-blocking; it will exit when the subprocess is either confirmed to have @@ -1248,6 +1303,17 @@ def run_task( (``StepTemplate.resolve_syntax_sugar``). It is required by a caller that is handed an *un-instantiated* ``StepTemplate``, where ``let`` and ``script.let`` are still separate fields. + resolved_symtab (Optional[SerializedSymbolTable]): The step-scope + symbol table generated by ``create_job`` (available as + ``Step.resolved_symtab``). It contains ``Param.*``, + ``RawParam.*``, ``Job.Name``, ``Step.Name``, and the + step-level let-binding values. Its entries seed the session + symbol table first; ``Session.*`` and ``Task.*`` values layer + on top to evaluate the script-level let bindings and the + action arguments — the layering the openjd-rs runtime applies + to the same table. ``None`` is fine when the script has no + let bindings and no expression interpolation that depends on + step-scope state. Raises: RuntimeError: If the Session is not in the READY state. @@ -1291,7 +1357,23 @@ def run_task( ) self._reset_action_state() - symtab = self._symbol_table(step_script.revision, task_parameter_values) + # Deserialize the service-resolved base table (if given) before + # building the symbol table, so its entries seed first. + resolved_base: Optional[dict[str, Any]] = None + if resolved_symtab is not None: + try: + resolved_base = self._resolved_base_entries(resolved_symtab) + except ValueError as e: + # Fail the action through the normal failure path rather than + # raising out of the public API, matching how the extra `let` + # bindings failure below is reported. + self._fail_action_before_start( + f"Failed to deserialize the resolved symbol table: {e}" + ) + return + symtab = self._symbol_table( + step_script.revision, task_parameter_values, resolved_base=resolved_base + ) # RFC 0005; Template Schemas §7.3.1 (EXPR): the running step's name. Only EXPR templates # pass validation referencing Step.Name, so seeding it when known does # not change non-EXPR behavior. @@ -1635,8 +1717,14 @@ def _symbol_table( self, version: SpecificationRevision, task_parameter_values: Optional[TaskParameterSet] = None, + resolved_base: Optional[dict[str, Any]] = None, ) -> SymbolTable: - """Construct a SymbolTable, with fully qualified value names, suitable for running a Script.""" + """Construct a SymbolTable, with fully qualified value names, suitable for running a Script. + + ``resolved_base`` (from :meth:`_resolved_base_entries`) seeds the + table before every session-derived value, so the latter layer on + top — the ordering the openjd-rs runtime applies to its base table. + """ def apply_mapping(path: str) -> str: if self._path_mapping_rules is not None: @@ -1687,14 +1775,25 @@ def record_expr_types( # The session is host scope: enable EXPR host-context functions # (e.g. apply_path_mapping) with this session's rules. symtab.expr_host_rules = self._expr_host_rules + # Seed the service-resolved base first, so every session-derived + # value below overwrites it: Session.WorkingDirectory, the + # path-mapped Param.* values, and Task.* all layer on top, + # matching the openjd-rs runtime's layering over the same table. + # A base entry for a name the session does not know survives. + if resolved_base: + for base_name, base_value in resolved_base.items(): + symtab[base_name] = base_value working_dir_key = ValueReferenceConstants_2023_09.WORKING_DIRECTORY.value symtab[working_dir_key] = str(self.working_directory) # Session.WorkingDirectory is a host-format path value in openjd-rs. symtab.expr_types[working_dir_key] = ParameterValueType.PATH.value # RFC 0005; Template Schemas §7.3.1 (EXPR): the job's resolved name. Only templates # declaring EXPR pass validation referencing Job.Name, so seeding - # it whenever known does not change non-EXPR behavior. - if self._job_name is not None: + # it whenever known does not change non-EXPR behavior. A base + # entry wins over the constructor value: in openjd-rs, Job.Name + # rides the base and is never re-set, and both values come from + # the service anyway. + if self._job_name is not None and "Job.Name" not in symtab: symtab["Job.Name"] = self._job_name for param_name, param_props in self._job_parameter_values.items(): raw_key = ( @@ -1715,6 +1814,31 @@ def record_expr_types( else: raise NotImplementedError(f"Schema version {str(version.value)} is not supported.") + def _resolved_base_entries(self, resolved_symtab: "SerializedSymbolTable") -> dict[str, Any]: + """Deserialize a service-resolved symbol table (``Step.resolved_symtab`` + from ``create_job``) into its flat ``{name: ExprValue}`` entries, in + host path format, ready to seed :meth:`_symbol_table`. + + Raises: + ValueError: An entry failed validation + (``SerializedSymbolTable.to_symtab`` validates entry contents + lazily; ``from_json_str`` only checked JSON well-formedness). + """ + # Guarded runtime import (extension purity): the caller already holds + # a native SerializedSymbolTable, so the engine extension is + # necessarily loaded — reaching this import cannot be what loads it. + # Do not move it to module level; see + # test/openjd/test_import_purity.py. + from openjd.expr import PathFormat + + # The Python engine bindings have no PathFormat.host(); derive it. + host_format = PathFormat.WINDOWS if os.name == "nt" else PathFormat.POSIX + engine_tab = resolved_symtab.to_symtab(path_format=host_format) + # `symbols` is the set of flat dotted leaf names; indexing returns + # the engine's typed ExprValue. (`keys` is top-level only — not what + # is needed here.) + return {name: engine_tab[name] for name in engine_tab.symbols} + def _build_expr_host_rules(self) -> Optional[list[Any]]: """Convert this session's path mapping rules to their engine (``openjd.expr.PathMappingRule``) form for EXPR host-context diff --git a/test/openjd/sessions_v0/test_session_resolved_symtab.py b/test/openjd/sessions_v0/test_session_resolved_symtab.py new file mode 100644 index 00000000..2b0c45a7 --- /dev/null +++ b/test/openjd/sessions_v0/test_session_resolved_symtab.py @@ -0,0 +1,445 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""Tests for the ``resolved_symtab`` parameter on the v0 ``Session``'s +``enter_environment`` / ``exit_environment`` / ``run_task``. + +The parameter mirrors the ``_v1`` (Rust-backed) session: a +``SerializedSymbolTable`` resolved by the service at CreateJob +(``Param.*``, ``RawParam.*``, ``Job.Name``, ``Step.Name``, step-level +``let`` values) seeds the session symbol table first, and the session's +own values layer on top — the layering the openjd-rs runtime applies to +the same table: + +- runtime locals (``Session.WorkingDirectory``, path-mapped ``Param.*``) + overwrite what the base carries; +- ``Job.Name`` from the base wins over the constructor value; +- script-scope ``let`` shadows base symbols (child table); +- a base that fails entry validation fails the action through the normal + callback path, never raising out of the public API. +""" + +from __future__ import annotations + +import json +import os +import time +import uuid +from pathlib import PurePosixPath, PureWindowsPath + +import pytest + +from openjd.expr import SerializedSymbolTable +from openjd.model import ParameterValue, ParameterValueType +from openjd.model.v2023_09 import ( + Action as Action_2023_09, + ArgString as ArgString_2023_09, + CommandString as CommandString_2023_09, + Environment as Environment_2023_09, + EnvironmentActions as EnvironmentActions_2023_09, + EnvironmentScript as EnvironmentScript_2023_09, + ModelParsingContext as ModelParsingContext_2023_09, + StepScript as StepScript_2023_09, +) +from openjd.sessions import ( + ActionState, + ActionStatus, + PathFormat, + PathMappingRule, + Session, + SessionState, +) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _action(command: str, *args: str) -> Action_2023_09: + return Action_2023_09( + command=CommandString_2023_09(command), + args=[ArgString_2023_09(a) for a in args] if args else None, + ) + + +def _env(name: str, **action_kwargs) -> Environment_2023_09: + return Environment_2023_09( + name=name, + script=EnvironmentScript_2023_09( + actions=EnvironmentActions_2023_09(**action_kwargs), + ), + ) + + +def _run_until_ready(session: Session, timeout_s: float = 10.0) -> None: + deadline = time.time() + timeout_s + while session.state == SessionState.RUNNING and time.time() < deadline: + time.sleep(0.05) + + +def _serialized_table(entries: list[dict[str, str]]) -> SerializedSymbolTable: + """Build a SerializedSymbolTable from its wire (JSON) form — the same + shape the service serves as ``resolvedSymbolTable``. Types are the + lowercase engine names (string, int, float, bool, path).""" + return SerializedSymbolTable.from_json_str(json.dumps(entries)) + + +def _expr_step_script(payload: dict) -> StepScript_2023_09: + """Parse a step script with the EXPR extension enabled, needed for + expression interpolation and script-scope ``let``.""" + context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) + return StepScript_2023_09.model_validate(payload, context=context) + + +# --------------------------------------------------------------------------- +# run_task: base symbols resolve, with the session's own values on top. +# --------------------------------------------------------------------------- + + +class TestRunTaskResolvedSymtab: + def test_base_only_symbol_resolves_in_action(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: a step script referencing a name defined only by the + # service-resolved base (no template `let` anywhere) — the headline + # equivalence case with the Rust runtime. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "task:{{ from_base }}")}, # type: ignore[arg-type] + ) + base = _serialized_table([{"name": "from_base", "type": "string", "value": "base value"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN + assert session.state == SessionState.READY + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:base value" in m for m in caplog.messages) + + def test_base_int_keeps_its_type_for_arithmetic(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: an int in the base, consumed by an arithmetic expression. + # Pins type fidelity: the base entry arrives as a typed ExprValue, + # not a string, or `v + 5` would fail to evaluate. + script = _expr_step_script( + {"actions": {"onRun": {"command": "echo", "args": ["result:", "{{ v + 5 }}"]}}} + ) + base = _serialized_table([{"name": "v", "type": "int", "value": "10"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("result: 15" in m for m in caplog.messages) + + def test_job_name_comes_from_base_when_ctor_omits_it( + self, caplog: pytest.LogCaptureFixture + ) -> None: + # GIVEN: no constructor job_name; the base carries Job.Name — the + # channel job-template-scope values ride in on. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "job:{{ Job.Name }}")}, # type: ignore[arg-type] + ) + base = _serialized_table([{"name": "Job.Name", "type": "string", "value": "BaseJob"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("job:BaseJob" in m for m in caplog.messages) + + def test_base_job_name_wins_over_ctor(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: both a constructor job_name and a base Job.Name. The base + # wins: in openjd-rs Job.Name rides the base and is never re-set, + # and both values come from the service anyway. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "job:{{ Job.Name }}")}, # type: ignore[arg-type] + ) + base = _serialized_table([{"name": "Job.Name", "type": "string", "value": "BaseJob"}]) + with Session( + session_id=uuid.uuid4().hex, job_parameter_values={}, job_name="CtorJob" + ) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("job:BaseJob" in m for m in caplog.messages) + assert not any("job:CtorJob" in m for m in caplog.messages) + + def test_runtime_locals_override_base(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: the base carries a bogus Session.WorkingDirectory. The + # session's own value must layer over it — runtime locals are the + # session's to set, exactly as in the Rust layering. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "wd:{{ Session.WorkingDirectory }}")}, # type: ignore[arg-type] + ) + base = _serialized_table( + [{"name": "Session.WorkingDirectory", "type": "path", "value": "/bogus/nowhere"}] + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN: the real working directory, not the base's value. + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + expected = f"wd:{session.working_directory}" + assert any(expected in m for m in caplog.messages) + assert not any("/bogus/nowhere" in m for m in caplog.messages) + + def test_path_param_remapped_over_base(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: a session with a path mapping rule, and a base carrying the + # UNMAPPED path for a PATH job parameter (the base is serialized + # before host rules are known). The session re-seeds Param.* from + # its own values with mapping applied — the v0 counterpart of + # Rust's step 2 re-mapping. + if os.name == "nt": + rule = PathMappingRule( + source_path_format=PathFormat.WINDOWS, + source_path=PureWindowsPath(r"c:\source"), + destination_path=PureWindowsPath(r"c:\dest"), + ) + unmapped, mapped = r"c:\source\file.txt", r"c:\dest\file.txt" + else: + rule = PathMappingRule( + source_path_format=PathFormat.POSIX, + source_path=PurePosixPath("/source"), + destination_path=PurePosixPath("/dest"), + ) + unmapped, mapped = "/source/file.txt", "/dest/file.txt" + script = StepScript_2023_09( + actions={"onRun": _action("echo", "p:{{ Param.P }}")}, # type: ignore[arg-type] + ) + base = _serialized_table([{"name": "Param.P", "type": "path", "value": unmapped}]) + with Session( + session_id=uuid.uuid4().hex, + job_parameter_values={ + "P": ParameterValue(type=ParameterValueType.PATH, value=unmapped) + }, + path_mapping_rules=[rule], + ) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN: the mapped path, not the base's unmapped one. + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any(f"p:{mapped}" in m for m in caplog.messages) + assert not any(f"p:{unmapped}" in m for m in caplog.messages) + + def test_script_scope_let_shadows_base_symbol(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: the same name in the base and in the script's own `let`. + # The narrower (script) scope must win — the runner evaluates script + # bindings into a child table sourced from the session-scope one. + script = _expr_step_script( + { + "let": ["shared = 'from script'"], + "actions": {"onRun": {"command": "echo", "args": ["task:{{ shared }}"]}}, + } + ) + base = _serialized_table([{"name": "shared", "type": "string", "value": "from base"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:from script" in m for m in caplog.messages) + assert not any("task:from base" in m for m in caplog.messages) + + def test_extra_let_bindings_coexist_with_base(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: both channels at once — the base (serving gate open) and + # extra_let_bindings (the fallback channel). Both must resolve. + script = StepScript_2023_09( + actions={"onRun": _action("echo", "a:{{ from_base }}", "b:{{ from_let }}")}, # type: ignore[arg-type] + ) + base = _serialized_table([{"name": "from_base", "type": "string", "value": "base value"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=base, + extra_let_bindings=["from_let = 'let value'"], + ) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("a:base value" in m for m in caplog.messages) + assert any("b:let value" in m for m in caplog.messages) + + def test_omitting_the_parameter_changes_nothing(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: the negative control. The parameter is additive and + # optional, so a task that does not use it must behave exactly as + # before — this is what makes the change safe for every caller. + script = _expr_step_script( + { + "let": ["own = 'script only'"], + "actions": {"onRun": {"command": "echo", "args": ["task:{{ own }}"]}}, + } + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + session.run_task(step_script=script, task_parameter_values={}) + _run_until_ready(session) + + # THEN + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("task:script only" in m for m in caplog.messages) + + +# --------------------------------------------------------------------------- +# enter_environment / exit_environment accept and use the base. +# --------------------------------------------------------------------------- + + +class TestEnvironmentResolvedSymtab: + def test_enter_and_exit_use_the_base(self, caplog: pytest.LogCaptureFixture) -> None: + # GIVEN: an environment whose onEnter and onExit reference a name + # defined only by the base — the worker passes the same table to + # both sides so onExit resolves in the same scope as onEnter. + env = _env( + "BaseEnv", + onEnter=_action("echo", "enter:{{ from_base }}"), + onExit=_action("echo", "exit:{{ from_base }}"), + ) + base = _serialized_table([{"name": "from_base", "type": "string", "value": "base value"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + # WHEN + identifier = session.enter_environment(environment=env, resolved_symtab=base) + _run_until_ready(session) + + # THEN: the enter action resolved the base symbol. + assert session.state == SessionState.READY + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("enter:base value" in m for m in caplog.messages) + + # WHEN + session.exit_environment(identifier=identifier, resolved_symtab=base) + _run_until_ready(session) + + # THEN: the exit action resolved it too. + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert any("exit:base value" in m for m in caplog.messages) + + +# --------------------------------------------------------------------------- +# An invalid base fails the action cleanly, never raising out of the +# public API — the same contract the extra `let` bindings failure holds. +# --------------------------------------------------------------------------- + + +class TestInvalidResolvedSymtab: + def test_invalid_base_fails_action_cleanly(self) -> None: + # GIVEN: a base that is well-formed JSON with invalid entry + # contents. from_json_str only checks JSON well-formedness; entry + # validation happens lazily in to_symtab (raising ValueError) at + # the action boundary. + callback_events: list[ActionStatus] = [] + + def callback(session_id: str, status: ActionStatus) -> None: + callback_events.append(status) + + bad_base = _serialized_table([{"name": "v", "type": "bogus", "value": "5"}]) + script = StepScript_2023_09( + actions={"onRun": _action("echo", "unreachable")}, # type: ignore[arg-type] + ) + with Session( + session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback + ) as session: + # WHEN: this must not raise. + session.run_task( + step_script=script, + task_parameter_values={}, + resolved_symtab=bad_base, + ) + _run_until_ready(session) + + # THEN: the action failed through the callback path. + status = session.action_status + assert status is not None + assert status.state == ActionState.FAILED + assert status.fail_message is not None + assert "resolved symbol table" in status.fail_message + assert callback_events and callback_events[-1].state == ActionState.FAILED + + env = _env("Env", onEnter=_action("true"), onExit=_action("true")) + with Session( + session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback + ) as session: + # WHEN: entering must not raise either. + identifier = session.enter_environment(environment=env, resolved_symtab=bad_base) + _run_until_ready(session) + + # THEN: the action failed cleanly and the environment remains + # entered-but-failed (exactly as a failing onEnter subprocess + # leaves it) so cleanup can exit it. + assert session.state == SessionState.READY_ENDING + status = session.action_status + assert status is not None + assert status.state == ActionState.FAILED + assert status.fail_message is not None + assert "resolved symbol table" in status.fail_message + assert identifier in session.environments_entered + + # WHEN: exiting the failed environment (without a base) works. + session.exit_environment(identifier=identifier) + _run_until_ready(session) + + # THEN + assert identifier not in session.environments_entered From a8e11f9d971eb0170a6f4eb9c73c63c99df09e43 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:15:19 -0700 Subject: [PATCH 3/9] fix: drain exit step context before failures exit_environment removed the environment from tracking but popped its stored Step.Name and extra `let` bindings only after the resolved symbol table deserialization and path mapping failure branches. Either failure stranded both entries permanently: identifiers are allocated per-enter, so no later exit can reach them. Move both pops up beside the wrap embedded-file record pop, so every failure branch below drains them. The replay into the symbol table stays where the table exists. The deserialization branch's comment claimed it matched the extra-lets guard, which pops before failing; it now cites the _materialize_path_mapping failure instead. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 3dbf0782..35170be6 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -1134,6 +1134,14 @@ def exit_environment( # environment's hook invocations; the files themselves live in the # session directory and are cleaned up with it. self._wrap_env_file_records.pop(identifier, None) + # Drain this environment's stored step context here, with the rest of + # its per-enter tracking, so no failure branch below can strand it. + # Identifiers are allocated per-enter, so a stranded entry is + # unreachable to a later exit of the same environment. The replay of + # these values into the symbol table stays below, where the table + # exists. + exit_step_name = self._environment_step_names.pop(identifier, None) + exit_extra_let_bindings = self._environment_extra_let_bindings.pop(identifier, None) self._running_environment_identifier = identifier @@ -1145,9 +1153,10 @@ def exit_environment( resolved_base = self._resolved_base_entries(resolved_symtab) except ValueError as e: # Fail the action through the normal failure path rather than - # raising out of the public API — the environment was already - # removed from tracking above, matching how the extra `let` - # bindings failure below leaves it. + # raising out of the public API — the environment and its + # stored step context were already removed from tracking + # above, matching how the _materialize_path_mapping failure + # below leaves it. self._fail_action_before_start( f"Failed to deserialize the resolved symbol table: {e}" ) @@ -1164,10 +1173,8 @@ def exit_environment( # re-apply the extra `let` bindings this environment was entered with # (e.g. the owning step's step-level bindings, RFC 0005) so its onExit # resolves in the same scope as its onEnter. - exit_step_name = self._environment_step_names.pop(identifier, None) if exit_step_name is not None: symtab["Step.Name"] = exit_step_name - exit_extra_let_bindings = self._environment_extra_let_bindings.pop(identifier, None) if exit_extra_let_bindings: try: apply_let_bindings(symtab=symtab, let_bindings=exit_extra_let_bindings) From 69df6302db4d5c9a270006ec547daf0d18462c4b Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:17:23 -0700 Subject: [PATCH 4/9] test: cover exit with an invalid resolved base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exit-side deserialization branch had no coverage: the existing invalid-base test exits without a base. The new test enters with step context, exits with a bad-typed base, and asserts the action fails cleanly without raising. It also asserts the stored step context was drained, which is what pins the preceding drain fix — the FAILED assertions pass either way. The drain is asserted on private dicts because the strand has no public surface; the docstring records why it still matters, namely that identifiers are caller-supplied and reusable, so a stale entry can replay on a later exit. Verified by mutation: restoring the pops below the failure branches fails this test and only this test. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../test_session_resolved_symtab.py | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/test/openjd/sessions_v0/test_session_resolved_symtab.py b/test/openjd/sessions_v0/test_session_resolved_symtab.py index 2b0c45a7..1f92e447 100644 --- a/test/openjd/sessions_v0/test_session_resolved_symtab.py +++ b/test/openjd/sessions_v0/test_session_resolved_symtab.py @@ -443,3 +443,65 @@ def callback(session_id: str, status: ActionStatus) -> None: # THEN assert identifier not in session.environments_entered + + def test_invalid_base_on_exit_fails_cleanly_and_drains(self) -> None: + """An invalid base handed to ``exit_environment`` fails the action + cleanly AND drains the environment's stored step context. + + The exit-side deserialization branch returns early. Everything the + environment stored at enter time (``Step.Name`` and its extra ``let`` + bindings) must already be popped by then, or it is stranded: those + dicts are keyed by identifier and nothing else removes an entry. + + Why a stranded entry is not merely untidy: ``enter_environment`` + accepts a caller-supplied identifier and does not reject one that + was used before, and the worker agent passes the service's + environment id. Re-entering the same id without step context would + replay the STALE stored values on its next exit or wrap-hook seed. + + The FAILED assertions below pass with or without the drain fix — + only the private-dict assertions catch a revert of it, so this test + doubles as that fix's regression test. + """ + callback_events: list[ActionStatus] = [] + + def callback(session_id: str, status: ActionStatus) -> None: + callback_events.append(status) + + bad_base = _serialized_table([{"name": "v", "type": "bogus", "value": "5"}]) + env = _env("Env", onEnter=_action("true"), onExit=_action("true")) + with Session( + session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback + ) as session: + # GIVEN: an environment entered cleanly WITH step context, so + # both tracking dicts hold an entry for it (non-vacuously: a + # drain assertion on an empty dict pins nothing). + identifier = session.enter_environment( + environment=env, + step_name="S", + extra_let_bindings=["msg = 'from step'"], + ) + _run_until_ready(session) + assert session.state == SessionState.READY + status = session.action_status + assert status is not None + assert status.state == ActionState.SUCCESS + assert session._environment_step_names[identifier] == "S" + assert session._environment_extra_let_bindings[identifier] == ["msg = 'from step'"] + + # WHEN: exiting with an invalid base — this must not raise. + session.exit_environment(identifier=identifier, resolved_symtab=bad_base) + _run_until_ready(session) + + # THEN: the action failed through the callback path. + status = session.action_status + assert status is not None + assert status.state == ActionState.FAILED + assert status.fail_message is not None + assert "resolved symbol table" in status.fail_message + assert callback_events and callback_events[-1].state == ActionState.FAILED + + # AND: the environment's stored step context was drained, not + # stranded by the early return. + assert identifier not in session._environment_step_names + assert identifier not in session._environment_extra_let_bindings From 6a1f41efddd8ad0c9df80393f940d7265a3bd511 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:22:09 -0700 Subject: [PATCH 5/9] fix: seed the resolved base into wrap hook scope _build_wrap_hook_scope built its table with no resolved base, so an RFC 0008 hook could not resolve a name only the service-resolved base defines. openjd-rs resolves a hook against the current action's full symbol table, base included, so the same hook passes there and failed here. Thread the inner entity's base through from the three call sites (enter, exit, run_task). This does not weaken the inner-to-hook isolation the two-scope split exists for: the base a step's action carries never holds Task.Param.*, because the service copies only Param.*, RawParam.*, Job.Name, Step.Name and step-level `let` values into it. Base Step.Name does become hook-visible, which is the parity openjd-rs has. New tests cover all three hook paths, base Step.Name visibility, and an isolation control that fails if the hook scope is built by copying the inner entity's table instead. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 33 ++- .../sessions_v0/test_wrap_scope_isolation.py | 198 ++++++++++++++++++ 2 files changed, 224 insertions(+), 7 deletions(-) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 35170be6..facbf067 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -1011,7 +1011,9 @@ def enter_environment( # failure the environment stays in the entered list, exactly as # when enter() itself fails, so the caller's cleanup exits it # as usual. - hook_symtab = self._build_wrap_hook_scope(environment.revision, symtab) + hook_symtab = self._build_wrap_hook_scope( + environment.revision, symtab, resolved_base=resolved_base + ) if not self._try_inject_wrapped_symbols( scope=EmbeddedFilesScope.ENV, inner_script=environment.script, @@ -1215,7 +1217,9 @@ def exit_environment( # See the onWrapEnvEnter path (_try_inject_wrapped_symbols). On # failure the environment was already removed from tracking # above, matching how a failed exit() behaves. - hook_symtab = self._build_wrap_hook_scope(environment.revision, symtab) + hook_symtab = self._build_wrap_hook_scope( + environment.revision, symtab, resolved_base=resolved_base + ) if not self._try_inject_wrapped_symbols( scope=EmbeddedFilesScope.ENV, inner_script=environment.script, @@ -1440,7 +1444,9 @@ def run_task( # _build_wrap_hook_scope. The wrap environment's own lets/files are # evaluated into the hook's table by the script runner from # wrap_env.script. - hook_symtab = self._build_wrap_hook_scope(step_script.revision, symtab) + hook_symtab = self._build_wrap_hook_scope( + step_script.revision, symtab, resolved_base=resolved_base + ) if not self._try_inject_wrapped_symbols( scope=EmbeddedFilesScope.STEP, inner_script=step_script, @@ -1930,7 +1936,10 @@ def _wrap_env_identifier(self, wrap_env: EnvironmentModel) -> EnvironmentIdentif return identifier def _build_wrap_hook_scope( - self, version: SpecificationRevision, session_symtab: SymbolTable + self, + version: SpecificationRevision, + session_symtab: SymbolTable, + resolved_base: Optional[dict[str, Any]] = None, ) -> SymbolTable: """The scope an RFC 0008 wrap hook resolves in. @@ -1952,17 +1961,27 @@ def _build_wrap_hook_scope( the only gate. What a hook legitimately gets: session scope (``Session.*``, - ``Job.Name``, ``Param.*``/``RawParam.*``), the path-mapping symbols, the - wrap environment's *own* enter-time step context (applied afterwards by + ``Job.Name``, ``Param.*``/``RawParam.*``), the service-resolved base + ``resolved_base`` seeds, the path-mapping symbols, the wrap + environment's *own* enter-time step context (applied afterwards by :meth:`_seed_wrap_env_scope`), the ``WrappedAction.*`` overlay, and its own script-level ``let`` bindings and embedded files (evaluated by the runner). + ``resolved_base`` is the base the *inner* entity's action was handed, + which is what openjd-rs's hook scope carries: a hook there resolves + against the current action's full symbol table, base included. Seeding + it does not weaken the isolation above — the base a step's action + carries never holds ``Task.Param.*`` (the service copies only + ``Param.*``/``RawParam.*``/``Job.Name``/``Step.Name``/step-level + ``let`` values into it), and with no base the scope is unchanged. Base + ``Step.Name`` IS hook-visible as a result, matching openjd-rs. + The path-mapping symbols are copied rather than re-materialized: the rules file has already been written for this action, and both scopes must name the same file. """ - hook_symtab = self._symbol_table(version) + hook_symtab = self._symbol_table(version, resolved_base=resolved_base) for key in ( ValueReferenceConstants_2023_09.HAS_PATH_MAPPING_RULES.value, ValueReferenceConstants_2023_09.PATH_MAPPING_RULES_FILE.value, diff --git a/test/openjd/sessions_v0/test_wrap_scope_isolation.py b/test/openjd/sessions_v0/test_wrap_scope_isolation.py index bd8e6f22..c8865dfb 100644 --- a/test/openjd/sessions_v0/test_wrap_scope_isolation.py +++ b/test/openjd/sessions_v0/test_wrap_scope_isolation.py @@ -32,6 +32,7 @@ from __future__ import annotations +import json import time import uuid from pathlib import PurePosixPath @@ -39,6 +40,7 @@ import pytest +from openjd.expr import SerializedSymbolTable from openjd.model import ParameterValue, ParameterValueType, SymbolTable from openjd.model.v2023_09 import ( Action as Action_2023_09, @@ -96,6 +98,43 @@ def _wrap_env(python_exe: str, name: str = "WrapEnv") -> Environment_2023_09: ) +def _echo(python_exe: str, text: str) -> Action_2023_09: + """An action that prints ``text``. + + Passing the text as an argv element rather than embedding it in the source + keeps quoting out of the picture. A reference inside ``text`` must resolve + for the action to start at all, so a test using this proves resolution + rather than mere presence in a table. + """ + return Action_2023_09( + command=CommandString_2023_09(python_exe), + args=[ + ArgString_2023_09("-c"), + ArgString_2023_09("import sys; print(sys.argv[1])"), + ArgString_2023_09(text), + ], + ) + + +def _echoing_wrap_env(python_exe: str, text: str, name: str = "WrapEnv") -> Environment_2023_09: + """A wrap environment whose three hooks all print ``text``.""" + return Environment_2023_09( + name=name, + script=EnvironmentScript_2023_09( + actions=EnvironmentActions_2023_09( + onWrapEnvEnter=_echo(python_exe, text), + onWrapTaskRun=_echo(python_exe, text), + onWrapEnvExit=_echo(python_exe, text), + ) + ), + ) + + +def _serialized_table(entries: list[dict[str, str]]) -> SerializedSymbolTable: + """A service-resolved base table in its wire (JSON) form.""" + return SerializedSymbolTable.from_json_str(json.dumps(entries)) + + def _inner_env(python_exe: str, name: str = "Inner") -> Environment_2023_09: return Environment_2023_09( name=name, @@ -524,3 +563,162 @@ def test_missing_path_mapping_symbols_are_tolerated(self, python_exe: str) -> No hook_symtab = session._build_wrap_hook_scope(revision, SymbolTable()) assert not _defined(hook_symtab, "Session.PathMappingRulesFile") assert not _defined(hook_symtab, "Session.HasPathMappingRules") + + +class TestResolvedBaseReachesTheHook: + """The service-resolved base IS hook scope, matching openjd-rs. + + A hook in openjd-rs resolves against the current action's full symbol + table, which is built with the base — so a hook referencing a name only + the base defines resolves there. Python built the hook's table with no + base at all, so the same hook failed. These tests pin the convergence. + + They do NOT relax the isolation this file is otherwise about. The base a + step's action carries never holds ``Task.Param.*``: the service copies + only ``Param.*``/``RawParam.*``/``Job.Name``/``Step.Name``/step-level + ``let`` values into it. The last test here is the control for that. + """ + + def test_base_symbol_resolves_in_a_task_hook( + self, python_exe: str, caplog: pytest.LogCaptureFixture + ) -> None: + # GIVEN: a wrap env whose onWrapTaskRun references a base-only name + base = _serialized_table([{"name": "from_base", "type": "string", "value": "base value"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + wrap_id = session.enter_environment( + environment=_echoing_wrap_env(python_exe, "HOOK={{from_base}}") + ) + _run_until_ready(session) + capture = _HookScopeCapture(session) + + # WHEN: a task runs under it WITH a base + session.run_task( + step_script=_step_script(python_exe), + task_parameter_values={}, + step_name="S", + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN: the hook resolved it and ran. + assert session.action_status is not None + assert session.action_status.state == ActionState.SUCCESS + assert any("HOOK=base value" in m for m in caplog.messages) + hook_scope = capture.table(0, expected_count=1) + assert str(hook_scope["from_base"]) == "base value" + + session.exit_environment(identifier=wrap_id) + _run_until_ready(session) + + @pytest.mark.parametrize("phase", ["enter", "exit"]) + def test_base_symbol_resolves_in_an_environment_hook( + self, phase: str, python_exe: str, caplog: pytest.LogCaptureFixture + ) -> None: + """The base rides the INNER environment's call, not the wrap env's. + + A wrap environment's own enter is never itself wrapped, so the base + that reaches an onWrapEnvEnter hook is the one the inner environment + was entered with — and for onWrapEnvExit, the one its exit was given. + """ + # GIVEN + base = _serialized_table([{"name": "from_base", "type": "string", "value": "base value"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + wrap_id = session.enter_environment( + environment=_echoing_wrap_env(python_exe, "HOOK={{from_base}}") + ) + _run_until_ready(session) + capture = _HookScopeCapture(session) + + # WHEN: an inner env is entered (and exited) WITH a base + inner_id = session.enter_environment( + environment=_inner_env(python_exe), resolved_symtab=base + ) + _run_until_ready(session) + if phase == "exit": + session.exit_environment(identifier=inner_id, resolved_symtab=base) + _run_until_ready(session) + + # THEN: the intercepting hook resolved the base-only name. + assert session.action_status is not None + assert session.action_status.state == ActionState.SUCCESS + assert any("HOOK=base value" in m for m in caplog.messages) + expected = 2 if phase == "exit" else 1 + hook_scope = capture.table(expected - 1, expected_count=expected) + assert str(hook_scope["from_base"]) == "base value" + + if phase == "enter": + session.exit_environment(identifier=inner_id) + _run_until_ready(session) + session.exit_environment(identifier=wrap_id) + _run_until_ready(session) + + def test_base_step_name_is_hook_visible(self, python_exe: str) -> None: + """Deliberate, and the opposite of the no-base case above. + + ``test_running_step_name_is_not_in_the_hook_scope`` pins that the + *running* step's name does not leak through the session's own + ``step_name`` channel. A base ``Step.Name`` is a different channel: + openjd-rs carries it into hook scope, so this is parity, not a leak. + Pinned so it is not "fixed" back. + """ + # GIVEN: a base carrying Step.Name, and a wrap env with no step + # context of its own to overwrite it. + base = _serialized_table([{"name": "Step.Name", "type": "string", "value": "BaseStep"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + wrap_id = session.enter_environment(environment=_wrap_env(python_exe)) + _run_until_ready(session) + capture = _HookScopeCapture(session) + + # WHEN + session.run_task( + step_script=_step_script(python_exe), + task_parameter_values={}, + step_name="RunningStep", + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN + hook_scope = capture.table(0, expected_count=1) + assert str(hook_scope["Step.Name"]) == "BaseStep" + assert hook_scope["WrappedStep.Name"] == "RunningStep" + + session.exit_environment(identifier=wrap_id) + _run_until_ready(session) + + @pytest.mark.parametrize("leaked_symbol", ["Task.Param.Frame", "Task.RawParam.Frame"]) + def test_task_parameters_stay_out_with_a_base_present( + self, leaked_symbol: str, python_exe: str + ) -> None: + """The control for the change above. + + The plausible mis-implementation — copying the inner entity's session + table into the hook scope instead of threading the base — passes every + other test in this class and fails this one. + """ + # GIVEN + base = _serialized_table([{"name": "from_base", "type": "string", "value": "base value"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + wrap_id = session.enter_environment(environment=_wrap_env(python_exe)) + _run_until_ready(session) + capture = _HookScopeCapture(session) + + # WHEN: a task with parameters runs under it, WITH a base + session.run_task( + step_script=_step_script(python_exe), + task_parameter_values={ + "Frame": ParameterValue(type=ParameterValueType.INT, value="42") + }, + step_name="RenderStep", + resolved_symtab=base, + ) + _run_until_ready(session) + + # THEN: the base arrived, and the task's parameters still did not. + hook_scope = capture.table(0, expected_count=1) + assert str(hook_scope["from_base"]) == "base value" + assert not _defined(hook_scope, leaked_symbol) + assert leaked_symbol not in hook_scope.expr_types + + session.exit_environment(identifier=wrap_id) + _run_until_ready(session) From 15c9787a30d067c1dcedc263cfec1f07d6d52ba5 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:29:06 -0700 Subject: [PATCH 6/9] fix: replay a wrap env's own base into hook scope A wrap environment's hooks resolve in its enter-time scope, and in openjd-rs that scope is the environment's frozen resolved symbol table merged onto the action's table. Python replayed only the step-name and extra-`let` fallback, so a hook referencing its own step's context delivered through the base resolved on openjd-rs and failed here. Store each environment's converted base at enter time and seed it in _seed_wrap_env_scope. The store happens after the deserialization succeeds, not beside the step-name and extra-lets stores, so a failed deserialization cannot leave a base behind. The base seeds before the existing replay, which keeps the fallback authoritative when the two disagree; exit drains the entry with the other two. Tests: the wrap env's own base resolves in a later task's hook, does not reach the wrapped action's scope, and the fallback wins over a disagreeing base. The exit-drain test now asserts this dict too. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 50 +++++-- .../test_session_resolved_symtab.py | 19 ++- .../sessions_v0/test_wrap_scope_isolation.py | 123 ++++++++++++++++++ 3 files changed, 177 insertions(+), 15 deletions(-) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index facbf067..08c74854 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -472,6 +472,13 @@ def __init__( # exits so the re-applied extra `let` bindings resolve in the same # scope. self._environment_step_names: dict[EnvironmentIdentifier, str] = dict() + # The service-resolved base table (already converted by + # _resolved_base_entries) an environment was entered with. A wrap + # environment's hooks resolve in its own enter-time scope, so its base + # is re-seeded into every hook scope by _seed_wrap_env_scope — + # mirroring how openjd-rs merges the wrap environment's frozen + # enter-time resolved table onto the hook's table. + self._environment_resolved_bases: dict[EnvironmentIdentifier, dict[str, Any]] = dict() self._environments_entered = list() self._runner = None self._running_environment_identifier = None @@ -871,6 +878,14 @@ def enter_environment( ) return identifier + # Remember this environment's base for its own wrap hooks (RFC 0008), + # which resolve in its enter-time scope. Stored here rather than beside + # the step-name/extra-lets stores above because those run before the + # conversion: storing there would leave a base behind on a failed + # deserialization. + if resolved_base: + self._environment_resolved_bases[identifier] = resolved_base + symtab = self._symbol_table(environment.revision, resolved_base=resolved_base) # RFC 0005; Template Schemas §7.3.1 (EXPR): the owning step's name. Only EXPR templates @@ -1144,6 +1159,11 @@ def exit_environment( # exists. exit_step_name = self._environment_step_names.pop(identifier, None) exit_extra_let_bindings = self._environment_extra_let_bindings.pop(identifier, None) + # Safe to drop here even though a wrap hook may intercept this exit: + # the interceptor is always a different, still-entered outer + # environment, and an environment that has exited can never intercept + # again. + self._environment_resolved_bases.pop(identifier, None) self._running_environment_identifier = identifier @@ -1996,18 +2016,26 @@ def _build_wrap_hook_scope( return hook_symtab def _seed_wrap_env_scope(self, symtab: SymbolTable, wrap_env: EnvironmentModel) -> bool: - """Re-seed the scope a wrap hook resolves in with the step context the - wrap environment was *entered* with (RFC 0005 step-level ``let`` - bindings and ``Step.Name``). + """Re-seed the scope a wrap hook resolves in with the context the + wrap environment was *entered* with: its service-resolved base, its + ``Step.Name``, and its RFC 0005 step-level ``let`` bindings. A wrap hook resolves in the wrap environment's own scope, and in openjd-rs that scope is the environment's frozen enter-time resolved - symbol table — so a step environment that defines wrap hooks carries - the owning step's step-level ``let`` bindings into every hook - invocation. Python builds a fresh session-scope table per action, so - those bindings have to be re-applied here from what - :meth:`enter_environment` remembered (it already keeps them to - re-apply on the exit side). + symbol table merged onto the action's table — so a step environment + that defines wrap hooks carries the owning step's base and step-level + ``let`` bindings into every hook invocation. Python builds a fresh + session-scope table per action, so both have to be re-applied here + from what :meth:`enter_environment` remembered (it already keeps the + step context to re-apply on the exit side). + + Ordering: the base seeds first, then the fallback ``Step.Name`` and + ``let`` bindings overwrite it, because those are the same values + arriving through the channel the base does not cover. Known and + accepted divergence: the fallback bindings re-evaluate locally on top + of the base, so a binding whose local result differs from the value + the service resolved takes the local result here and the base's in + openjd-rs. Call this *after* the wrapped action's own scope has been built, so the wrap environment's bindings cannot reach the wrapped action's @@ -2019,6 +2047,10 @@ def _seed_wrap_env_scope(self, symtab: SymbolTable, wrap_env: EnvironmentModel) return. """ identifier = self._wrap_env_identifier(wrap_env) + # Values only, exactly as _symbol_table seeds a base: the EXPR types + # ride the values the engine already built. + for base_name, base_value in self._environment_resolved_bases.get(identifier, {}).items(): + symtab[base_name] = base_value step_name = self._environment_step_names.get(identifier) if step_name is not None: symtab["Step.Name"] = step_name diff --git a/test/openjd/sessions_v0/test_session_resolved_symtab.py b/test/openjd/sessions_v0/test_session_resolved_symtab.py index 1f92e447..d88072fd 100644 --- a/test/openjd/sessions_v0/test_session_resolved_symtab.py +++ b/test/openjd/sessions_v0/test_session_resolved_symtab.py @@ -449,9 +449,10 @@ def test_invalid_base_on_exit_fails_cleanly_and_drains(self) -> None: cleanly AND drains the environment's stored step context. The exit-side deserialization branch returns early. Everything the - environment stored at enter time (``Step.Name`` and its extra ``let`` - bindings) must already be popped by then, or it is stranded: those - dicts are keyed by identifier and nothing else removes an entry. + environment stored at enter time (``Step.Name``, its extra ``let`` + bindings, and its own enter-time base) must already be popped by then, + or it is stranded: those dicts are keyed by identifier and nothing + else removes an entry. Why a stranded entry is not merely untidy: ``enter_environment`` accepts a caller-supplied identifier and does not reject one that @@ -469,17 +470,21 @@ def callback(session_id: str, status: ActionStatus) -> None: callback_events.append(status) bad_base = _serialized_table([{"name": "v", "type": "bogus", "value": "5"}]) + good_base = _serialized_table( + [{"name": "from_base", "type": "string", "value": "base value"}] + ) env = _env("Env", onEnter=_action("true"), onExit=_action("true")) with Session( session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback ) as session: - # GIVEN: an environment entered cleanly WITH step context, so - # both tracking dicts hold an entry for it (non-vacuously: a - # drain assertion on an empty dict pins nothing). + # GIVEN: an environment entered cleanly WITH step context and a + # valid base, so all three tracking dicts hold an entry for it + # (non-vacuously: a drain assertion on an empty dict pins nothing). identifier = session.enter_environment( environment=env, step_name="S", extra_let_bindings=["msg = 'from step'"], + resolved_symtab=good_base, ) _run_until_ready(session) assert session.state == SessionState.READY @@ -488,6 +493,7 @@ def callback(session_id: str, status: ActionStatus) -> None: assert status.state == ActionState.SUCCESS assert session._environment_step_names[identifier] == "S" assert session._environment_extra_let_bindings[identifier] == ["msg = 'from step'"] + assert "from_base" in session._environment_resolved_bases[identifier] # WHEN: exiting with an invalid base — this must not raise. session.exit_environment(identifier=identifier, resolved_symtab=bad_base) @@ -505,3 +511,4 @@ def callback(session_id: str, status: ActionStatus) -> None: # stranded by the early return. assert identifier not in session._environment_step_names assert identifier not in session._environment_extra_let_bindings + assert identifier not in session._environment_resolved_bases diff --git a/test/openjd/sessions_v0/test_wrap_scope_isolation.py b/test/openjd/sessions_v0/test_wrap_scope_isolation.py index c8865dfb..e67cd13a 100644 --- a/test/openjd/sessions_v0/test_wrap_scope_isolation.py +++ b/test/openjd/sessions_v0/test_wrap_scope_isolation.py @@ -652,6 +652,129 @@ def test_base_symbol_resolves_in_an_environment_hook( session.exit_environment(identifier=wrap_id) _run_until_ready(session) + def test_wrap_envs_own_base_reaches_a_later_hook( + self, python_exe: str, caplog: pytest.LogCaptureFixture + ) -> None: + """The second base channel: the wrap env's OWN enter-time base. + + openjd-rs merges the wrap environment's frozen enter-time resolved + table onto the hook's table, so a hook referencing its own step's + context resolves there even when the intercepted action carries no + base at all. Without this the hook passes on openjd-rs and fails here. + """ + # GIVEN: a wrap env entered WITH a base of its own + wrap_base = _serialized_table([{"name": "wrap_own", "type": "string", "value": "WRAP-OWN"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + wrap_id = session.enter_environment( + environment=_echoing_wrap_env(python_exe, "HOOK={{wrap_own}}"), + resolved_symtab=wrap_base, + ) + _run_until_ready(session) + capture = _HookScopeCapture(session) + + # WHEN: a later task runs under it with NO base of its own, so the + # only channel that can supply the symbol is the stored one. + session.run_task( + step_script=_step_script(python_exe), + task_parameter_values={}, + step_name="S", + ) + _run_until_ready(session) + + # THEN + assert session.action_status is not None + assert session.action_status.state == ActionState.SUCCESS + assert any("HOOK=WRAP-OWN" in m for m in caplog.messages) + hook_scope = capture.table(0, expected_count=1) + assert str(hook_scope["wrap_own"]) == "WRAP-OWN" + + session.exit_environment(identifier=wrap_id) + _run_until_ready(session) + + def test_wrap_envs_own_base_does_not_reach_the_wrapped_action(self, python_exe: str) -> None: + """The negative control for the change above. + + Re-seeding the wrap env's base must land in the hook's scope only. If + it reached the wrapped action's scope, a wrap environment could inject + symbols into the work it wraps. Asserted on the INNER scope object the + Session actually resolved the wrapped action against, captured as it is + handed to the hook-scope builder — the same table, live, so this sees + it as it was used. + """ + # GIVEN + wrap_base = _serialized_table([{"name": "wrap_own", "type": "string", "value": "WRAP-OWN"}]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + wrap_id = session.enter_environment( + environment=_wrap_env(python_exe), resolved_symtab=wrap_base + ) + _run_until_ready(session) + capture = _HookScopeCapture(session) + inner_scopes: list[SymbolTable] = [] + original_builder = session._build_wrap_hook_scope + + def _capturing_builder( + version: Any, session_symtab: SymbolTable, **kwargs: Any + ) -> SymbolTable: + inner_scopes.append(session_symtab) + return original_builder(version, session_symtab, **kwargs) + + session._build_wrap_hook_scope = _capturing_builder # type: ignore[method-assign] + + # WHEN + session.run_task( + step_script=_step_script(python_exe), + task_parameter_values={}, + step_name="S", + ) + _run_until_ready(session) + + # THEN: the hook has it, the wrapped action's own scope does not. + hook_scope = capture.table(0, expected_count=1) + assert str(hook_scope["wrap_own"]) == "WRAP-OWN" + assert len(inner_scopes) == 1, "the inner scope was never captured" + assert not _defined(inner_scopes[0], "wrap_own") + + session.exit_environment(identifier=wrap_id) + _run_until_ready(session) + + def test_fallback_step_context_wins_over_the_stored_base(self, python_exe: str) -> None: + """Pins the seeding order inside ``_seed_wrap_env_scope``. + + The stored base seeds first and the ``step_name``/``extra_let_bindings`` + fallback overwrites it, because those carry the same values through the + channel the base does not cover. This is the accepted divergence from + openjd-rs, which takes the service's value: when the two disagree, this + runtime takes the locally supplied one. + """ + # GIVEN: a wrap env entered with BOTH a base Step.Name and the + # step_name fallback, disagreeing on purpose. + wrap_base = _serialized_table( + [{"name": "Step.Name", "type": "string", "value": "FromBase"}] + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + wrap_id = session.enter_environment( + environment=_wrap_env(python_exe), + step_name="FromFallback", + resolved_symtab=wrap_base, + ) + _run_until_ready(session) + capture = _HookScopeCapture(session) + + # WHEN + session.run_task( + step_script=_step_script(python_exe), + task_parameter_values={}, + step_name="RunningStep", + ) + _run_until_ready(session) + + # THEN + hook_scope = capture.table(0, expected_count=1) + assert str(hook_scope["Step.Name"]) == "FromFallback" + + session.exit_environment(identifier=wrap_id) + _run_until_ready(session) + def test_base_step_name_is_hook_visible(self, python_exe: str) -> None: """Deliberate, and the opposite of the no-base case above. From 9e74bdf86e85c32e7a4ae0ff3b9d34c763f381df Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:34:40 -0700 Subject: [PATCH 7/9] docs: tighten the resolved-base comments Trim restated wording and correct one claim: a stranded per-enter entry was described as unreachable, but identifiers may be supplied by the caller and reused after an exit, so a re-entered identifier would replay stale context. That is why the drain matters. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 42 ++++++++++++++++----------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 08c74854..e3b8a10b 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -472,12 +472,11 @@ def __init__( # exits so the re-applied extra `let` bindings resolve in the same # scope. self._environment_step_names: dict[EnvironmentIdentifier, str] = dict() - # The service-resolved base table (already converted by - # _resolved_base_entries) an environment was entered with. A wrap - # environment's hooks resolve in its own enter-time scope, so its base - # is re-seeded into every hook scope by _seed_wrap_env_scope — - # mirroring how openjd-rs merges the wrap environment's frozen - # enter-time resolved table onto the hook's table. + # The converted service-resolved base an environment was entered with. + # A wrap environment's hooks resolve in its enter-time scope, so + # _seed_wrap_env_scope re-seeds this into every hook scope, mirroring + # openjd-rs merging the environment's frozen resolved table onto the + # hook's table. self._environment_resolved_bases: dict[EnvironmentIdentifier, dict[str, Any]] = dict() self._environments_entered = list() self._runner = None @@ -878,11 +877,10 @@ def enter_environment( ) return identifier - # Remember this environment's base for its own wrap hooks (RFC 0008), - # which resolve in its enter-time scope. Stored here rather than beside - # the step-name/extra-lets stores above because those run before the - # conversion: storing there would leave a base behind on a failed - # deserialization. + # Remembered for this environment's own wrap hooks (RFC 0008), which + # resolve in its enter-time scope. Stored after the conversion, not + # beside the step-name/extra-lets stores above: those run before it, so + # storing there would leave a base behind on a failed deserialization. if resolved_base: self._environment_resolved_bases[identifier] = resolved_base @@ -1151,12 +1149,12 @@ def exit_environment( # environment's hook invocations; the files themselves live in the # session directory and are cleaned up with it. self._wrap_env_file_records.pop(identifier, None) - # Drain this environment's stored step context here, with the rest of - # its per-enter tracking, so no failure branch below can strand it. - # Identifiers are allocated per-enter, so a stranded entry is - # unreachable to a later exit of the same environment. The replay of - # these values into the symbol table stays below, where the table - # exists. + # Drained here, with the rest of this environment's per-enter + # tracking, so no failure branch below can strand it. A stranded entry + # is not merely a leak: identifiers may be supplied by the caller and + # reused after an exit, and a re-entered identifier would then replay + # this stale context. The replay into the symbol table stays below, + # where the table exists. exit_step_name = self._environment_step_names.pop(identifier, None) exit_extra_let_bindings = self._environment_extra_let_bindings.pop(identifier, None) # Safe to drop here even though a wrap hook may intercept this exit: @@ -1991,11 +1989,11 @@ def _build_wrap_hook_scope( ``resolved_base`` is the base the *inner* entity's action was handed, which is what openjd-rs's hook scope carries: a hook there resolves against the current action's full symbol table, base included. Seeding - it does not weaken the isolation above — the base a step's action - carries never holds ``Task.Param.*`` (the service copies only - ``Param.*``/``RawParam.*``/``Job.Name``/``Step.Name``/step-level - ``let`` values into it), and with no base the scope is unchanged. Base - ``Step.Name`` IS hook-visible as a result, matching openjd-rs. + it does not weaken the isolation above, because the service copies only + ``Param.*``/``RawParam.*``/``Job.Name``/``Step.Name`` and step-level + ``let`` values into that base, never ``Task.Param.*``. With no base the + scope is unchanged. Base ``Step.Name`` is hook-visible as a result, + matching openjd-rs. The path-mapping symbols are copied rather than re-materialized: the rules file has already been written for this action, and both scopes must From 7836928267da9112e04afb9180556649443138f1 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 12:10:52 -0700 Subject: [PATCH 8/9] refactor: Drop extra_let_bindings parameter The resolved symbol table is now the single authoritative channel for step-scope EXPR `let` values. `extra_let_bindings` was the source-string fallback that predated it; carrying both meant two channels for the same values, with a documented divergence where the locally re-evaluated binding won over the value the service resolved. Removed from `run_task`, `enter_environment` and `exit_environment`, along with the `_environment_extra_let_bindings` tracking dict and the replay in `_seed_wrap_env_scope`. That replay was the method's only failure path, so its return type is now `None` and the three call sites no longer branch on it. `apply_let_bindings` stays imported: it is still used by `_build_wrapped_inner_scope` for inner script lets. The environment case is safe because the service stores a step-scoped environment with the owning step's resolved symbol table, so an ENV_ENTER already receives `Step.Name` and the step's template-scope `let` values in its table. Nothing needed the source-string channel. `step_name` is kept on both methods. On `run_task` it feeds RFC 0008's `WrappedStep.Name`, an injected overlay symbol the table does not carry. On `enter_environment` it is redundant with the base but retained for a caller that has a step name and no resolved table. Tests whose only subject was the parameter are deleted; those that used it to set up step context now deliver the same context through `resolved_symtab`, so the wrap-scope isolation and hook-seeding properties stay pinned. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_session.py | 192 +++---------- .../sessions_v0/test_session_let_bindings.py | 259 ++---------------- .../test_session_resolved_symtab.py | 29 +- .../sessions_v0/test_wrap_scope_isolation.py | 51 ++-- test/openjd/sessions_v0/test_wrap_task_run.py | 30 +- 5 files changed, 107 insertions(+), 454 deletions(-) diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index e3b8a10b..77c55b60 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -463,14 +463,9 @@ def __init__( self._session_id = session_id self._ending_only = False self._environments = dict() - # Extra EXPR `let` bindings supplied when an environment was entered - # (e.g. the owning step's step-level bindings), re-applied when the - # environment exits so its onExit resolves in the same scope. - self._environment_extra_let_bindings: dict[EnvironmentIdentifier, list[str]] = dict() # The owning step's name supplied when an environment was entered # (seeds Step.Name, RFC 0005 EXPR), re-seeded when the environment - # exits so the re-applied extra `let` bindings resolve in the same - # scope. + # exits so its onExit resolves in the same scope as its onEnter. self._environment_step_names: dict[EnvironmentIdentifier, str] = dict() # The converted service-resolved base an environment was entered with. # A wrap environment's hooks resolve in its enter-time scope, so @@ -764,7 +759,6 @@ def enter_environment( environment: EnvironmentModel, identifier: Optional[EnvironmentIdentifier] = None, os_env_vars: Optional[dict[str, str]] = None, - extra_let_bindings: Optional[list[str]] = None, step_name: Optional[str] = None, resolved_symtab: Optional["SerializedSymbolTable"] = None, ) -> EnvironmentIdentifier: @@ -785,20 +779,14 @@ def enter_environment( by values defined in Environments. Key: Environment variable name Value: Value for the environment variable. - extra_let_bindings (Optional[list[str]]): Additional EXPR ``let`` - bindings (RFC 0005) evaluated into the symbol table before the - environment's variables and actions resolve. A step's - environments are entered with the step-level ``let`` bindings - (``Step.let`` on the instantiated Job) so both can reference - them — the v0 counterpart of the per-step resolved symbol - table that openjd-rs threads into enter_environment. step_name (Optional[str]): The name of the step whose stepEnvironments are being entered, if any. Seeds - ``Step.Name`` (RFC 0005 EXPR) into the symbol table before - the extra ``let`` bindings evaluate, so step-level bindings - and the environment's variables and actions can reference - it — openjd-rs threads a per-step resolved symbol table into - environment entry, and this is the v0 counterpart. + ``Step.Name`` (RFC 0005 EXPR) into the symbol table so the + environment's variables and actions can reference it. + Redundant with ``resolved_symtab``, which already carries + ``Step.Name``: kept for a caller that has a step name but no + resolved table, since the table is otherwise the + authoritative source of step scope. resolved_symtab (Optional[SerializedSymbolTable]): The step-scope symbol table generated by ``create_job`` (available as ``Step.resolved_symtab``). It contains ``Param.*``, @@ -849,8 +837,6 @@ def enter_environment( identifier = f"{self._session_id}:{uuid.uuid4().hex}" self._environments[identifier] = environment - if extra_let_bindings: - self._environment_extra_let_bindings[identifier] = list(extra_let_bindings) if step_name is not None: self._environment_step_names[identifier] = step_name self._environments_entered.append(identifier) @@ -863,8 +849,7 @@ def enter_environment( try: resolved_base = self._resolved_base_entries(resolved_symtab) except ValueError as e: - # Same failure shape as the extra `let` bindings guard below: - # fail the action through the normal failure path rather than + # Fail the action through the normal failure path rather than # raising out of the public API. The environment is already on # the entered list, so the caller's cleanup exits it as usual, # and the empty change record keeps the log-forwarding @@ -879,8 +864,8 @@ def enter_environment( # Remembered for this environment's own wrap hooks (RFC 0008), which # resolve in its enter-time scope. Stored after the conversion, not - # beside the step-name/extra-lets stores above: those run before it, so - # storing there would leave a base behind on a failed deserialization. + # beside the step-name store above: that runs before it, so storing + # there would leave a base behind on a failed deserialization. if resolved_base: self._environment_resolved_bases[identifier] = resolved_base @@ -888,33 +873,11 @@ def enter_environment( # RFC 0005; Template Schemas §7.3.1 (EXPR): the owning step's name. Only EXPR templates # pass validation referencing Step.Name, so seeding it when known does - # not change non-EXPR behavior. Seeded before the extra `let` bindings - # evaluate so a step-level binding may reference it. + # not change non-EXPR behavior. Applied after the resolved base seeds + # so an explicit step_name wins over a stale base entry. if step_name is not None: symtab["Step.Name"] = step_name - # Step-level `let` bindings (RFC 0005) accompany a step's - # environments: evaluate them first so the environment's variables - # and actions can reference them. - if extra_let_bindings: - try: - apply_let_bindings(symtab=symtab, let_bindings=extra_let_bindings) - except ValueError as e: - # ExpressionError and FormatStringError subclass ValueError: - # a binding failed to evaluate (e.g. it referenced an - # undefined symbol). Fail the action through the normal - # failure path rather than raising out of the public API — - # the environment stays in the entered list, exactly as when - # a failing onEnter subprocess leaves it, so the caller's - # cleanup exits it as usual. - self._created_env_vars[identifier] = SimplifiedEnvironmentVariableChanges( - dict[str, str]() - ) - self._fail_action_before_start( - f"Failed to evaluate the extra `let` bindings for {environment.name}: {e}" - ) - return identifier - # Note: the environment script's own EXPR `let` bindings (RFC 0005) # are evaluated by the script runner, after embedded-file paths are # allocated (so bindings can reference Env.File.*). The environment's @@ -1016,8 +979,8 @@ def enter_environment( if wrap_env is not None: # The wrapped onEnter resolves against the INNER environment's - # own scope (`symtab`, which carries the step context and - # extra_let_bindings that environment was entered with); the hook + # own scope (`symtab`, which carries the resolved base and step + # context that environment was entered with); the hook # resolves against its own table, which carries none of them -- see # _build_wrap_hook_scope. Its own script's lets/files are evaluated # into that table by the script runner from wrap_env.script. On @@ -1040,8 +1003,7 @@ def enter_environment( ), ): return identifier - if not self._seed_wrap_env_scope(hook_symtab, wrap_env): - return identifier + self._seed_wrap_env_scope(hook_symtab, wrap_env) try: wrap_file_records = self._get_wrap_env_file_records(wrap_env) except RuntimeError as e: @@ -1156,7 +1118,6 @@ def exit_environment( # this stale context. The replay into the symbol table stays below, # where the table exists. exit_step_name = self._environment_step_names.pop(identifier, None) - exit_extra_let_bindings = self._environment_extra_let_bindings.pop(identifier, None) # Safe to drop here even though a wrap hook may intercept this exit: # the interceptor is always a different, still-entered outer # environment, and an environment that has exited can never intercept @@ -1189,25 +1150,11 @@ def exit_environment( self._fail_action_before_start(str(e)) return - # Re-seed the owning step's name (Step.Name, RFC 0005 EXPR) and - # re-apply the extra `let` bindings this environment was entered with - # (e.g. the owning step's step-level bindings, RFC 0005) so its onExit - # resolves in the same scope as its onEnter. + # Re-seed the owning step's name (Step.Name, RFC 0005 EXPR) this + # environment was entered with so its onExit resolves in the same + # scope as its onEnter. if exit_step_name is not None: symtab["Step.Name"] = exit_step_name - if exit_extra_let_bindings: - try: - apply_let_bindings(symtab=symtab, let_bindings=exit_extra_let_bindings) - except ValueError as e: - # ExpressionError and FormatStringError subclass ValueError: - # a binding failed to evaluate. Fail the action through the - # normal failure path rather than raising out of the public - # API — the environment was already removed from tracking - # above, matching how a failing onExit subprocess leaves it. - self._fail_action_before_start( - f"Failed to evaluate the extra `let` bindings for {environment.name}: {e}" - ) - return # Note: the environment script's own EXPR `let` bindings (RFC 0005) # are evaluated by the script runner (after embedded-file path @@ -1255,8 +1202,7 @@ def exit_environment( ), ): return - if not self._seed_wrap_env_scope(hook_symtab, wrap_env): - return + self._seed_wrap_env_scope(hook_symtab, wrap_env) try: wrap_file_records = self._get_wrap_env_file_records(wrap_env) except RuntimeError as e: @@ -1293,7 +1239,6 @@ def run_task( os_env_vars: Optional[dict[str, str]] = None, log_task_banner: bool = True, step_name: Optional[str] = None, - extra_let_bindings: Optional[list[str]] = None, resolved_symtab: Optional["SerializedSymbolTable"] = None, ) -> None: """Run a Task within the Session. @@ -1316,22 +1261,6 @@ def run_task( step_name (Optional[str]): The name of the step whose task is being run. Used by RFC 0008 to populate ``WrappedStep.Name`` in wrap hooks. Required when a wrap Environment is active. - extra_let_bindings (Optional[list[str]]): Additional EXPR ``let`` - bindings (RFC 0005) evaluated into the symbol table before the - step script's own bindings and actions resolve. This is the - step-template-scope ``let`` (``Step.let`` on the instantiated - Job), which resolves at job instantiation and so is not part of - the step script — the ``run_task`` counterpart of - :meth:`enter_environment`'s parameter of the same name, and the - v0 counterpart of the per-step resolved symbol table that - openjd-rs threads into ``run_task``. - - A caller that obtained its step script from - ``create_job`` does not need this: job instantiation folds the - step-scope bindings into the script's own ``let`` - (``StepTemplate.resolve_syntax_sugar``). It is required by a - caller that is handed an *un-instantiated* ``StepTemplate``, - where ``let`` and ``script.let`` are still separate fields. resolved_symtab (Optional[SerializedSymbolTable]): The step-scope symbol table generated by ``create_job`` (available as ``Step.resolved_symtab``). It contains ``Param.*``, @@ -1394,8 +1323,7 @@ def run_task( resolved_base = self._resolved_base_entries(resolved_symtab) except ValueError as e: # Fail the action through the normal failure path rather than - # raising out of the public API, matching how the extra `let` - # bindings failure below is reported. + # raising out of the public API. self._fail_action_before_start( f"Failed to deserialize the resolved symbol table: {e}" ) @@ -1409,30 +1337,6 @@ def run_task( if step_name is not None: symtab["Step.Name"] = step_name - # Step-template-scope `let` bindings (RFC 0005 §3.6) accompany the task: - # evaluate them into the session-scope table so the step script's own - # bindings and its actions can reference them. Seeded after Step.Name so - # a step-level binding may reference it, and before path mapping so - # {{Session.PathMappingRulesFile}} and the env-var evaluation below see a - # complete table -- the same ordering enter_environment uses. - # - # Script-scope bindings shadow these rather than colliding with them: - # StepScriptRunner evaluates `script.let` into a CHILD table sourced from - # this one, so a same-named script binding takes precedence. - if extra_let_bindings: - try: - apply_let_bindings(symtab=symtab, let_bindings=extra_let_bindings) - except ValueError as e: - # ExpressionError and FormatStringError subclass ValueError: a - # binding failed to evaluate (e.g. it referenced an undefined - # symbol). Fail the action through the normal failure path - # rather than raising out of the public API, matching how - # enter_environment reports the same failure. - self._fail_action_before_start( - f"Failed to evaluate the extra `let` bindings for the task: {e}" - ) - return - action_env_vars = self._evaluate_current_session_env_vars(os_env_vars) try: self._materialize_path_mapping(step_script.revision, action_env_vars, symtab) @@ -1485,8 +1389,7 @@ def run_task( # `_build_wrapped_inner_scope`, so seeding it cannot reach the # wrapped action's scope whenever it happens. It stays after # injection to keep all three hook paths reading the same way. - if not self._seed_wrap_env_scope(hook_symtab, wrap_env): - return + self._seed_wrap_env_scope(hook_symtab, wrap_env) try: wrap_file_records = self._get_wrap_env_file_records(wrap_env) @@ -1965,7 +1868,7 @@ def _build_wrap_hook_scope( entity's table, and that distinction is the whole point. The inner entity's table carries symbols belonging to the wrapped work: a task's ``Task.Param.*``/``Task.RawParam.*``, the running step's ``Step.Name``, - and the ``extra_let_bindings`` the *inner* environment was entered with. + and the resolved base the *inner* environment was entered with. A wrap environment must not be able to read any of them. :meth:`_build_wrapped_inner_scope` already blocks the wrap -> inner @@ -2013,36 +1916,28 @@ def _build_wrap_hook_scope( hook_symtab.expr_types[key] = session_symtab.expr_types[key] return hook_symtab - def _seed_wrap_env_scope(self, symtab: SymbolTable, wrap_env: EnvironmentModel) -> bool: + def _seed_wrap_env_scope(self, symtab: SymbolTable, wrap_env: EnvironmentModel) -> None: """Re-seed the scope a wrap hook resolves in with the context the - wrap environment was *entered* with: its service-resolved base, its - ``Step.Name``, and its RFC 0005 step-level ``let`` bindings. + wrap environment was *entered* with: its service-resolved base and + its ``Step.Name``. A wrap hook resolves in the wrap environment's own scope, and in openjd-rs that scope is the environment's frozen enter-time resolved symbol table merged onto the action's table — so a step environment - that defines wrap hooks carries the owning step's base and step-level - ``let`` bindings into every hook invocation. Python builds a fresh - session-scope table per action, so both have to be re-applied here - from what :meth:`enter_environment` remembered (it already keeps the - step context to re-apply on the exit side). - - Ordering: the base seeds first, then the fallback ``Step.Name`` and - ``let`` bindings overwrite it, because those are the same values - arriving through the channel the base does not cover. Known and - accepted divergence: the fallback bindings re-evaluate locally on top - of the base, so a binding whose local result differs from the value - the service resolved takes the local result here and the base's in - openjd-rs. + that defines wrap hooks carries the owning step's base, including the + step-level ``let`` values the service resolved into it, through to + every hook invocation. Python builds a fresh session-scope table per + action, so it has to be re-applied here from what + :meth:`enter_environment` remembered (it already keeps the step + context to re-apply on the exit side). + + Ordering: the base seeds first, then ``Step.Name`` overwrites it, + because a caller that passed ``step_name`` without a resolved table + supplies it through that channel instead. Call this *after* the wrapped action's own scope has been built, so - the wrap environment's bindings cannot reach the wrapped action's + the wrap environment's context cannot reach the wrapped action's resolution — only the hook's. - - Returns: - True on success. On failure the action has already been failed - through :meth:`_fail_action_before_start` and the caller must - return. """ identifier = self._wrap_env_identifier(wrap_env) # Values only, exactly as _symbol_table seeds a base: the EXPR types @@ -2052,21 +1947,6 @@ def _seed_wrap_env_scope(self, symtab: SymbolTable, wrap_env: EnvironmentModel) step_name = self._environment_step_names.get(identifier) if step_name is not None: symtab["Step.Name"] = step_name - let_bindings = self._environment_extra_let_bindings.get(identifier) - if not let_bindings: - return True - try: - apply_let_bindings(symtab=symtab, let_bindings=let_bindings) - except ValueError as e: - # ExpressionError and FormatStringError subclass ValueError. These - # bindings already evaluated successfully when the environment was - # entered, so this is unlikely — but it must not raise out of the - # public API. - self._fail_action_before_start( - f"Failed to evaluate the extra `let` bindings for {wrap_env.name}: {e}" - ) - return False - return True def _get_wrap_env_file_records(self, wrap_env: EnvironmentModel) -> Optional[list[_FileRecord]]: """Return the wrap environment's embedded-file records, allocating diff --git a/test/openjd/sessions_v0/test_session_let_bindings.py b/test/openjd/sessions_v0/test_session_let_bindings.py index 3061d588..115313a7 100644 --- a/test/openjd/sessions_v0/test_session_let_bindings.py +++ b/test/openjd/sessions_v0/test_session_let_bindings.py @@ -3,12 +3,9 @@ """Regression tests for the Session-level handling of EXPR ``let`` bindings (RFC 0005): -- a failing ``extra_let_bindings`` entry on ``enter_environment`` / - ``exit_environment`` fails the action through the normal callback path - instead of raising out of the public API; -- ``enter_environment(step_name=...)`` seeds ``Step.Name`` so step-level - bindings and the environment's actions can reference it (on both the - enter and exit sides); +- ``enter_environment(step_name=...)`` seeds ``Step.Name`` so the + environment's actions can reference it (on both the enter and exit + sides); - binding-RHS parsing is memoized across applications; - the unified optional int-or-format-string field resolver (``resolve_optional_int_field``) enforces consistent bounds; @@ -112,59 +109,9 @@ def __str__(self) -> str: return "str-form" -# --------------------------------------------------------------------------- -# A failing extra `let` binding must FAIL the action via the callback path, -# never raise out of enter_environment()/exit_environment(). -# --------------------------------------------------------------------------- - - -class TestExtraLetBindingFailure: - def test_enter_environment_failing_binding_fails_action_cleanly(self) -> None: - # GIVEN: an extra binding referencing an undefined symbol. - callback_events: list[ActionStatus] = [] - - def callback(session_id: str, status: ActionStatus) -> None: - callback_events.append(status) - - env = _env("Env", onEnter=_action("true"), onExit=_action("true")) - with Session( - session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback - ) as session: - # WHEN: entering must not raise. - identifier = session.enter_environment( - environment=env, - extra_let_bindings=["msg = NoSuchSymbol"], - ) - _run_until_ready(session) - - # THEN: the action failed cleanly, the callback fired, and the - # environment remains entered-but-failed (exactly as a failing - # onEnter subprocess leaves it) so cleanup can exit it. - assert session.state == SessionState.READY_ENDING - status = session.action_status - assert status is not None - assert status.state == ActionState.FAILED - assert status.fail_message is not None - assert "let" in status.fail_message - assert callback_events and callback_events[-1].state == ActionState.FAILED - assert identifier in session.environments_entered - - # WHEN: exiting the failed environment re-applies the failing - # bindings — the exit action must also fail cleanly, not raise. - session.exit_environment(identifier=identifier) - _run_until_ready(session) - - # THEN - assert session.state == SessionState.READY_ENDING - status = session.action_status - assert status is not None - assert status.state == ActionState.FAILED - assert identifier not in session.environments_entered - - # --------------------------------------------------------------------------- # enter_environment(step_name=...) seeds Step.Name (RFC 0005 EXPR), for both -# the enter side and the re-applied bindings on the exit side. +# the enter side and the re-seed on the exit side. # --------------------------------------------------------------------------- @@ -172,20 +119,24 @@ class TestEnterEnvironmentStepName: def test_step_name_resolvable_in_bindings_and_actions( self, caplog: pytest.LogCaptureFixture ) -> None: - # GIVEN: a step-level binding referencing Step.Name, echoed by both - # the environment's onEnter and onExit actions. - env = _env( - "StepEnv", - onEnter=_action("echo", "enter:{{ msg }}"), - onExit=_action("echo", "exit:{{ msg }}"), + # GIVEN: an environment whose own script-level `let` references + # Step.Name, echoed by both its onEnter and onExit actions. The + # binding is what makes the ORDERING observable: Step.Name has to be + # in the session table before the script runner evaluates the script's + # lets into its child table. + env = Environment_2023_09( + name="StepEnv", + script=EnvironmentScript_2023_09( + actions=EnvironmentActions_2023_09( + onEnter=_action("echo", "enter:{{ msg }}"), + onExit=_action("echo", "exit:{{ msg }}"), + ), + let=["msg = 'step is ' + Step.Name"], + ), ) with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: # WHEN - identifier = session.enter_environment( - environment=env, - extra_let_bindings=["msg = 'step is ' + Step.Name"], - step_name="MyStep", - ) + identifier = session.enter_environment(environment=env, step_name="MyStep") _run_until_ready(session) # THEN: the enter action ran with the binding resolved. @@ -195,8 +146,8 @@ def test_step_name_resolvable_in_bindings_and_actions( assert status.state == ActionState.SUCCESS assert any("enter:step is MyStep" in m for m in caplog.messages) - # WHEN: the exit re-applies the bindings — Step.Name must be - # re-seeded so onExit resolves in the same scope as onEnter. + # WHEN: exiting — Step.Name must be re-seeded so onExit resolves + # in the same scope as onEnter. session.exit_environment(identifier=identifier) _run_until_ready(session) @@ -207,174 +158,6 @@ def test_step_name_resolvable_in_bindings_and_actions( assert any("exit:step is MyStep" in m for m in caplog.messages) -# --------------------------------------------------------------------------- -# run_task(extra_let_bindings=...) delivers step-template-scope `let` -# (RFC 0005 §3.6) to the task, the counterpart of enter_environment's -# parameter of the same name. -# -# Why this needs its own coverage: step-scope bindings resolve at job -# instantiation, so a caller holding a Job from create_job never sees the -# problem -- instantiation folds them into the script's own `let`. A caller -# handed an un-instantiated StepTemplate (the Deadline Cloud worker agent, -# which receives one from the service) has `let` and `script.let` as separate -# fields, and without this parameter the step-scope names are simply absent -# from the table and every reference fails with "Undefined variable". -# --------------------------------------------------------------------------- - - -class TestRunTaskExtraLetBindings: - def test_step_scope_binding_resolvable_in_action( - self, caplog: pytest.LogCaptureFixture - ) -> None: - # GIVEN: a step script whose onRun references a name defined only by - # the step-template-scope bindings. - script = StepScript_2023_09( - actions={"onRun": _action("echo", "task:{{ from_step }}")}, # type: ignore[arg-type] - ) - with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: - # WHEN - session.run_task( - step_script=script, - task_parameter_values={}, - extra_let_bindings=["from_step = 'step value'"], - ) - _run_until_ready(session) - - # THEN - assert session.state == SessionState.READY - status = session.action_status - assert status is not None - assert status.state == ActionState.SUCCESS - assert any("task:step value" in m for m in caplog.messages) - - def test_step_scope_binding_can_reference_step_name( - self, caplog: pytest.LogCaptureFixture - ) -> None: - # GIVEN: a step-scope binding referencing Step.Name. Pins the seeding - # order -- Step.Name must be in the table before the bindings evaluate, - # matching enter_environment. - script = StepScript_2023_09( - actions={"onRun": _action("echo", "task:{{ msg }}")}, # type: ignore[arg-type] - ) - with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: - # WHEN - session.run_task( - step_script=script, - task_parameter_values={}, - step_name="MyStep", - extra_let_bindings=["msg = 'step is ' + Step.Name"], - ) - _run_until_ready(session) - - # THEN - status = session.action_status - assert status is not None - assert status.state == ActionState.SUCCESS - assert any("task:step is MyStep" in m for m in caplog.messages) - - def test_script_scope_binding_shadows_step_scope( - self, caplog: pytest.LogCaptureFixture - ) -> None: - # GIVEN: the same name bound at both scopes. RFC 0005 §3.6 scoping - # requires the narrower (script) scope to win rather than the two - # colliding, which holds because the runner evaluates script bindings - # into a child table sourced from the session-scope one. - script = StepScript_2023_09( - actions={"onRun": _action("echo", "task:{{ shared }}")}, # type: ignore[arg-type] - let=["shared = 'from script'"], - ) - with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: - # WHEN - session.run_task( - step_script=script, - task_parameter_values={}, - extra_let_bindings=["shared = 'from step'"], - ) - _run_until_ready(session) - - # THEN - status = session.action_status - assert status is not None - assert status.state == ActionState.SUCCESS - assert any("task:from script" in m for m in caplog.messages) - assert not any("task:from step" in m for m in caplog.messages) - - def test_script_scope_binding_can_reference_step_scope( - self, caplog: pytest.LogCaptureFixture - ) -> None: - # GIVEN: a script-scope binding building on a step-scope one. This is - # the shape the failing conformance fixtures use, and it only works if - # the step bindings are in the parent of the runner's child table. - script = StepScript_2023_09( - actions={"onRun": _action("echo", "task:{{ derived }}")}, # type: ignore[arg-type] - let=["derived = base + '/leaf'"], - ) - with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: - # WHEN - session.run_task( - step_script=script, - task_parameter_values={}, - extra_let_bindings=["base = '/root'"], - ) - _run_until_ready(session) - - # THEN - status = session.action_status - assert status is not None - assert status.state == ActionState.SUCCESS - assert any("task:/root/leaf" in m for m in caplog.messages) - - def test_failing_binding_fails_action_cleanly(self) -> None: - # GIVEN: a step-scope binding referencing an undefined symbol. It must - # fail the action through the callback path, never raise out of the - # public API -- the same contract enter_environment holds. - callback_events: list[ActionStatus] = [] - - def callback(session_id: str, status: ActionStatus) -> None: - callback_events.append(status) - - script = StepScript_2023_09( - actions={"onRun": _action("echo", "unreachable")}, # type: ignore[arg-type] - ) - with Session( - session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback - ) as session: - # WHEN: this must not raise. - session.run_task( - step_script=script, - task_parameter_values={}, - extra_let_bindings=["msg = NoSuchSymbol"], - ) - _run_until_ready(session) - - # THEN - status = session.action_status - assert status is not None - assert status.state == ActionState.FAILED - assert status.fail_message is not None - assert "let" in status.fail_message - assert callback_events and callback_events[-1].state == ActionState.FAILED - - def test_omitting_the_parameter_changes_nothing(self, caplog: pytest.LogCaptureFixture) -> None: - # GIVEN: the negative control. The parameter is additive and optional, - # so a task that does not use it must behave exactly as before -- this - # is what makes the change safe for every existing caller. - script = StepScript_2023_09( - actions={"onRun": _action("echo", "task:{{ own }}")}, # type: ignore[arg-type] - let=["own = 'script only'"], - ) - with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: - # WHEN - session.run_task(step_script=script, task_parameter_values={}) - _run_until_ready(session) - - # THEN - status = session.action_status - assert status is not None - assert status.state == ActionState.SUCCESS - assert any("task:script only" in m for m in caplog.messages) - - # --------------------------------------------------------------------------- # Binding-RHS parsing is memoized: re-applying the same bindings (per task, # per env enter/exit) must not re-parse through the engine each time. diff --git a/test/openjd/sessions_v0/test_session_resolved_symtab.py b/test/openjd/sessions_v0/test_session_resolved_symtab.py index d88072fd..aee0cf59 100644 --- a/test/openjd/sessions_v0/test_session_resolved_symtab.py +++ b/test/openjd/sessions_v0/test_session_resolved_symtab.py @@ -293,30 +293,6 @@ def test_script_scope_let_shadows_base_symbol(self, caplog: pytest.LogCaptureFix assert any("task:from script" in m for m in caplog.messages) assert not any("task:from base" in m for m in caplog.messages) - def test_extra_let_bindings_coexist_with_base(self, caplog: pytest.LogCaptureFixture) -> None: - # GIVEN: both channels at once — the base (serving gate open) and - # extra_let_bindings (the fallback channel). Both must resolve. - script = StepScript_2023_09( - actions={"onRun": _action("echo", "a:{{ from_base }}", "b:{{ from_let }}")}, # type: ignore[arg-type] - ) - base = _serialized_table([{"name": "from_base", "type": "string", "value": "base value"}]) - with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: - # WHEN - session.run_task( - step_script=script, - task_parameter_values={}, - resolved_symtab=base, - extra_let_bindings=["from_let = 'let value'"], - ) - _run_until_ready(session) - - # THEN - status = session.action_status - assert status is not None - assert status.state == ActionState.SUCCESS - assert any("a:base value" in m for m in caplog.messages) - assert any("b:let value" in m for m in caplog.messages) - def test_omitting_the_parameter_changes_nothing(self, caplog: pytest.LogCaptureFixture) -> None: # GIVEN: the negative control. The parameter is additive and # optional, so a task that does not use it must behave exactly as @@ -478,12 +454,11 @@ def callback(session_id: str, status: ActionStatus) -> None: session_id=uuid.uuid4().hex, job_parameter_values={}, callback=callback ) as session: # GIVEN: an environment entered cleanly WITH step context and a - # valid base, so all three tracking dicts hold an entry for it + # valid base, so both tracking dicts hold an entry for it # (non-vacuously: a drain assertion on an empty dict pins nothing). identifier = session.enter_environment( environment=env, step_name="S", - extra_let_bindings=["msg = 'from step'"], resolved_symtab=good_base, ) _run_until_ready(session) @@ -492,7 +467,6 @@ def callback(session_id: str, status: ActionStatus) -> None: assert status is not None assert status.state == ActionState.SUCCESS assert session._environment_step_names[identifier] == "S" - assert session._environment_extra_let_bindings[identifier] == ["msg = 'from step'"] assert "from_base" in session._environment_resolved_bases[identifier] # WHEN: exiting with an invalid base — this must not raise. @@ -510,5 +484,4 @@ def callback(session_id: str, status: ActionStatus) -> None: # AND: the environment's stored step context was drained, not # stranded by the early return. assert identifier not in session._environment_step_names - assert identifier not in session._environment_extra_let_bindings assert identifier not in session._environment_resolved_bases diff --git a/test/openjd/sessions_v0/test_wrap_scope_isolation.py b/test/openjd/sessions_v0/test_wrap_scope_isolation.py index e67cd13a..d93446b8 100644 --- a/test/openjd/sessions_v0/test_wrap_scope_isolation.py +++ b/test/openjd/sessions_v0/test_wrap_scope_isolation.py @@ -19,7 +19,7 @@ - a wrapped task's ``Task.Param.*`` / ``Task.RawParam.*`` - the running step's ``Step.Name`` -- the ``extra_let_bindings`` the *inner* environment was entered with +- the ``step_name`` the *inner* environment was entered with ``WrappedStep.Name`` exists in RFC 0008 precisely because ``Step.Name`` is not meant to be reachable from a hook. openjd-model does not reject any of these @@ -401,11 +401,14 @@ class TestInnerEnvironmentScopeDoesNotReachTheHook: must not be in the hook's scope.""" @pytest.mark.parametrize("phase", ["enter", "exit"]) - def test_inner_extra_let_bindings_are_not_in_the_hook_scope( - self, phase: str, python_exe: str - ) -> None: - # GIVEN: a wrap env, and an inner env entered with its own step-level - # bindings and step name + def test_inner_step_name_is_not_in_the_hook_scope(self, phase: str, python_exe: str) -> None: + """The inner env's ``step_name`` channel must not leak into the hook. + + Deliberately scoped to that channel. An inner env's *resolved base* is + a different matter: ``TestResolvedBaseReachesTheHook`` pins that it is + hook-visible on purpose, matching openjd-rs. + """ + # GIVEN: a wrap env, and an inner env entered with its own step name with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: wrap_id = session.enter_environment(environment=_wrap_env(python_exe)) _run_until_ready(session) @@ -414,7 +417,6 @@ def test_inner_extra_let_bindings_are_not_in_the_hook_scope( # WHEN inner_id = session.enter_environment( environment=_inner_env(python_exe), - extra_let_bindings=["inner_secret = 'INNER-ONLY'"], step_name="InnerStep", ) _run_until_ready(session) @@ -422,13 +424,12 @@ def test_inner_extra_let_bindings_are_not_in_the_hook_scope( session.exit_environment(identifier=inner_id) _run_until_ready(session) - # THEN: neither the inner env's binding nor its step name is - # reachable from the hook that intercepted it. Index the hook we - # care about rather than the most recent capture, so a hook that - # stopped running cannot pass by inheriting the other's table. + # THEN: the inner env's step name is not reachable from the hook + # that intercepted it. Index the hook we care about rather than the + # most recent capture, so a hook that stopped running cannot pass + # by inheriting the other's table. expected = 2 if phase == "exit" else 1 hook_scope = capture.table(expected - 1, expected_count=expected) - assert not _defined(hook_scope, "inner_secret") assert not _defined(hook_scope, "Step.Name") assert hook_scope["WrappedEnv.Name"] == "Inner" @@ -444,14 +445,20 @@ def test_wrap_envs_own_bindings_still_reach_the_hook(self, phase: str, python_ex ``test_wrap_task_run.py::test_env_hooks_resolve_step_level_let_bindings`` covers the same ground by asserting the hook merely SUCCEEDs; this - asserts the binding is in the hook's scope with the right value, which is - what distinguishes "seeded" from "the hook happened not to need it". + asserts the step-level value is in the hook's scope with the right + value, which is what distinguishes "seeded" from "the hook happened not + to need it". ``test_wrap_envs_own_base_reaches_a_later_hook`` is the + onWrapTaskRun counterpart; this covers the two env hooks. """ - # GIVEN: a wrap env entered WITH step-level bindings + # GIVEN: a wrap env entered WITH a step-level let value in its + # resolved table, the channel the service supplies it through + wrap_base = _serialized_table( + [{"name": "wrap_secret", "type": "string", "value": "WRAP-OWN"}] + ) with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: wrap_id = session.enter_environment( environment=_wrap_env(python_exe), - extra_let_bindings=["wrap_secret = 'WRAP-OWN'"], + resolved_symtab=wrap_base, step_name="WrapStep", ) _run_until_ready(session) @@ -464,7 +471,7 @@ def test_wrap_envs_own_bindings_still_reach_the_hook(self, phase: str, python_ex session.exit_environment(identifier=inner_id) _run_until_ready(session) - # THEN. `let` bindings are stored as the EXPR engine's typed value, + # THEN. Base values are stored as the EXPR engine's typed value, # so compare the rendered form rather than the object. expected = 2 if phase == "exit" else 1 hook_scope = capture.table(expected - 1, expected_count=expected) @@ -740,11 +747,11 @@ def _capturing_builder( def test_fallback_step_context_wins_over_the_stored_base(self, python_exe: str) -> None: """Pins the seeding order inside ``_seed_wrap_env_scope``. - The stored base seeds first and the ``step_name``/``extra_let_bindings`` - fallback overwrites it, because those carry the same values through the - channel the base does not cover. This is the accepted divergence from - openjd-rs, which takes the service's value: when the two disagree, this - runtime takes the locally supplied one. + The stored base seeds first and the ``step_name`` fallback overwrites + it, because that carries the same value for a caller with no resolved + table. This is the accepted divergence from openjd-rs, which takes the + service's value: when the two disagree, this runtime takes the locally + supplied one. """ # GIVEN: a wrap env entered with BOTH a base Step.Name and the # step_name fallback, disagreeing on purpose. diff --git a/test/openjd/sessions_v0/test_wrap_task_run.py b/test/openjd/sessions_v0/test_wrap_task_run.py index 1e7803f6..77eaf4b4 100644 --- a/test/openjd/sessions_v0/test_wrap_task_run.py +++ b/test/openjd/sessions_v0/test_wrap_task_run.py @@ -11,11 +11,13 @@ from __future__ import annotations +import json import time import uuid import pytest +from openjd.expr import SerializedSymbolTable from openjd.model import SymbolTable from openjd.model.v2023_09 import ( Action as Action_2023_09, @@ -37,6 +39,11 @@ _NOOP = Action_2023_09(command=CommandString_2023_09("true")) +def _serialized_table(entries: list[dict[str, str]]) -> SerializedSymbolTable: + """A service-resolved base table in its wire (JSON) form.""" + return SerializedSymbolTable.from_json_str(json.dumps(entries)) + + def _wrap_env(name: str, wrap_action: Action_2023_09) -> Environment_2023_09: """Build an Environment with ``onWrapTaskRun`` set to ``wrap_action`` and the other two wrap hooks set to no-ops (all-or-nothing rule).""" @@ -616,16 +623,17 @@ def test_run_task_without_step_name_is_fine_unwrapped(self) -> None: # --------------------------------------------------------------------------- # A wrap hook resolves in the wrap environment's own scope, which in openjd-rs # is that environment's frozen enter-time symbol table. So a step environment -# that defines wrap hooks must carry the step-level `let` bindings it was -# entered with into every hook invocation -- without letting them replace the -# wrapped action's own resolution scope. +# that defines wrap hooks must carry the step-level `let` values it was entered +# with -- which reach it in its resolved symbol table -- into every hook +# invocation, without letting them replace the wrapped action's own resolution +# scope. # --------------------------------------------------------------------------- class TestWrapHookSeesEnterTimeStepScope: def test_hook_resolves_step_level_let_bindings(self, tmp_path) -> None: - # GIVEN: a wrap environment entered with a step's step-level bindings, - # and a hook that references one of them. + # GIVEN: a wrap environment entered with a step's resolved table, + # carrying a step-level `let` value, and a hook referencing it. env = _wrap_env( "WrapEnv", Action_2023_09( @@ -637,7 +645,9 @@ def test_hook_resolves_step_level_let_bindings(self, tmp_path) -> None: with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: identifier = session.enter_environment( environment=env, - extra_let_bindings=["greeting = 'from-step-let'"], + resolved_symtab=_serialized_table( + [{"name": "greeting", "type": "string", "value": "from-step-let"}] + ), step_name="Step1", ) _run_until_ready(session) @@ -679,9 +689,7 @@ def test_wrap_env_step_name_does_not_reach_the_wrapped_action(self) -> None: ) step = _step_script("echo", ["{{Step.Name}}"]) with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: - identifier = session.enter_environment( - environment=env, extra_let_bindings=None, step_name="StepA" - ) + identifier = session.enter_environment(environment=env, step_name="StepA") _run_until_ready(session) original = session._inject_wrapped_task_symbols @@ -737,7 +745,9 @@ def test_env_hooks_resolve_step_level_let_bindings(self, hook_phase: str) -> Non with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: wrap_id = session.enter_environment( environment=env, - extra_let_bindings=["greeting = 'from-step-let'"], + resolved_symtab=_serialized_table( + [{"name": "greeting", "type": "string", "value": "from-step-let"}] + ), step_name="Step1", ) _run_until_ready(session) From c23a6824b202b3e23c6839ee52013f8883777dd4 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Tue, 25 Aug 2026 12:50:49 -0700 Subject: [PATCH 9/9] fix: type the hook-scope capture stand-in exactly CI runs `mypy src test` (hatch.toml) and rejected the monkeypatched _build_wrap_hook_scope stand-in: a **kwargs signature is not assignable to the bound method's type once fix 1a added resolved_base, and the existing `type: ignore[method-assign]` does not cover the `assignment` error code. All 18 Python matrix legs failed at Run Linting. Give the stand-in the real signature and forward resolved_base rather than widening the suppression, so the harness observes the builder transparently instead of altering what it is meant to watch. My miss: I ran `mypy src` locally, not the repo's `mypy src test`. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../sessions_v0/test_wrap_scope_isolation.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/test/openjd/sessions_v0/test_wrap_scope_isolation.py b/test/openjd/sessions_v0/test_wrap_scope_isolation.py index d93446b8..a0122edc 100644 --- a/test/openjd/sessions_v0/test_wrap_scope_isolation.py +++ b/test/openjd/sessions_v0/test_wrap_scope_isolation.py @@ -36,12 +36,17 @@ import time import uuid from pathlib import PurePosixPath -from typing import Any +from typing import Any, Optional import pytest from openjd.expr import SerializedSymbolTable -from openjd.model import ParameterValue, ParameterValueType, SymbolTable +from openjd.model import ( + ParameterValue, + ParameterValueType, + SpecificationRevision, + SymbolTable, +) from openjd.model.v2023_09 import ( Action as Action_2023_09, ArgString as ArgString_2023_09, @@ -719,11 +724,16 @@ def test_wrap_envs_own_base_does_not_reach_the_wrapped_action(self, python_exe: inner_scopes: list[SymbolTable] = [] original_builder = session._build_wrap_hook_scope + # Signature matches _build_wrap_hook_scope exactly, including the + # resolved_base parameter: a **kwargs stand-in is not assignable to + # the bound method's type. def _capturing_builder( - version: Any, session_symtab: SymbolTable, **kwargs: Any + version: SpecificationRevision, + session_symtab: SymbolTable, + resolved_base: Optional[dict[str, Any]] = None, ) -> SymbolTable: inner_scopes.append(session_symtab) - return original_builder(version, session_symtab, **kwargs) + return original_builder(version, session_symtab, resolved_base) session._build_wrap_hook_scope = _capturing_builder # type: ignore[method-assign]