diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 7be303c6..77c55b60 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") @@ -462,15 +463,16 @@ 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 + # _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 self._running_environment_identifier = None @@ -757,8 +759,8 @@ 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: """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 @@ -777,20 +779,24 @@ 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.*``, + ``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. @@ -831,44 +837,47 @@ 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) self._running_environment_identifier = identifier - symtab = self._symbol_table(environment.revision) - - # 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. - 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: + # 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: - apply_let_bindings(symtab=symtab, let_bindings=extra_let_bindings) + resolved_base = self._resolved_base_entries(resolved_symtab) 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. + # 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 evaluate the extra `let` bindings for {environment.name}: {e}" + f"Failed to deserialize the resolved symbol table: {e}" ) return identifier + # 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 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 + + 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 + # 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 + # 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 @@ -970,15 +979,17 @@ 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 # 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, @@ -992,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: @@ -1032,6 +1042,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 +1063,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; @@ -1096,37 +1111,50 @@ 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) + # 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) + # 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 - 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 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}" + ) + 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: 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. - exit_step_name = self._environment_step_names.pop(identifier, None) + # 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 - 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) - 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 @@ -1154,7 +1182,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, @@ -1172,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: @@ -1210,6 +1239,7 @@ def run_task( os_env_vars: Optional[dict[str, str]] = None, log_task_banner: bool = True, step_name: Optional[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 @@ -1231,6 +1261,17 @@ 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. + 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. @@ -1274,12 +1315,28 @@ 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. + 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. if step_name is not None: symtab["Step.Name"] = step_name + action_env_vars = self._evaluate_current_session_env_vars(os_env_vars) try: self._materialize_path_mapping(step_script.revision, action_env_vars, symtab) @@ -1309,7 +1366,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, @@ -1330,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) @@ -1593,8 +1651,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: @@ -1645,14 +1709,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 = ( @@ -1673,6 +1748,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 @@ -1757,7 +1857,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. @@ -1765,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 @@ -1779,17 +1882,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, 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 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, @@ -1803,48 +1916,37 @@ 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: - """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``). + 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 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 — 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, 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 + # 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 - 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 2b3c5dd0..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) 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..aee0cf59 --- /dev/null +++ b/test/openjd/sessions_v0/test_session_resolved_symtab.py @@ -0,0 +1,487 @@ +# 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_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 + + 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``, 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 + 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"}]) + 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 and a + # 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", + resolved_symtab=good_base, + ) + _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 "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) + _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_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 bd8e6f22..a0122edc 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 @@ -32,14 +32,21 @@ from __future__ import annotations +import json import time import uuid from pathlib import PurePosixPath -from typing import Any +from typing import Any, Optional import pytest -from openjd.model import ParameterValue, ParameterValueType, SymbolTable +from openjd.expr import SerializedSymbolTable +from openjd.model import ( + ParameterValue, + ParameterValueType, + SpecificationRevision, + SymbolTable, +) from openjd.model.v2023_09 import ( Action as Action_2023_09, ArgString as ArgString_2023_09, @@ -96,6 +103,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, @@ -362,11 +406,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) @@ -375,7 +422,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) @@ -383,13 +429,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" @@ -405,14 +450,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) @@ -425,7 +476,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) @@ -524,3 +575,290 @@ 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_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 + + # 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: SpecificationRevision, + session_symtab: SymbolTable, + resolved_base: Optional[dict[str, Any]] = None, + ) -> SymbolTable: + inner_scopes.append(session_symtab) + return original_builder(version, session_symtab, resolved_base) + + 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`` 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. + 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. + + ``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) 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)