From dbfd1b55ae6a8c32e1e350cebfe75e343ec1b24a Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:41:26 -0700 Subject: [PATCH 01/11] fix: Re-evaluate a step's template-scope let bindings in template scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An instantiated Step's `script.let` is a merged list: the step-level bindings the template declared, followed by the script's own. The step-level prefix was already evaluated at job creation in template scope, which openjd-rs — and openjd-model as of the companion change — evaluate with `PathFormat::Posix` so that a create-time result cannot depend on the host that created the job. The runners re-evaluated the whole merged list in the host's format, so on Windows the prefix's PATH values were re-rendered with backslashes and a binding silently held a different value in the two evaluations: `startswith(path("/foo/bar"), "/foo")` flipped from true to false. `apply_script_let_bindings` now owns the split. The leading `_template_scope_let_count` entries are evaluated with `PathFormat.POSIX` and the remainder with the host's format, unchanged — a script's own bindings are session scope and legitimately reference `Session.WorkingDirectory`, `Task.File.*` and `apply_path_mapping`. Both halves go into the same symbol table in the same order, so a script-level binding can still reference a step-level one. Applied at all three sites that evaluate a step script's merged list: the step runner, the step runner's embedded-files path, and RFC 0008's `_build_wrapped_inner_scope`. Environment scripts are untouched. The count is read with `getattr(script, "_template_scope_let_count", 0)`, so an openjd-model without the companion change degrades to the previous behaviour. `path_format` is likewise forwarded to `evaluate_let_bindings` only when set, because the parameter does not exist at the currently declared openjd-model floor and passing it there raises TypeError rather than being ignored. This is the openjd-sessions half of a two-repo fix; see OpenJobDescription/openjd-model-for-python#341. The openjd-model floor in pyproject.toml must be raised once that half releases, and until then `hatch run typing` reports one error against the PyPI model. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_runner_base.py | 110 +++- src/openjd/sessions/_runner_step_script.py | 7 +- src/openjd/sessions/_session.py | 19 +- .../test_template_scope_let_split.py | 493 ++++++++++++++++++ test/openjd/test_import_purity.py | 60 +++ 5 files changed, 679 insertions(+), 10 deletions(-) create mode 100644 test/openjd/sessions_v0/test_template_scope_let_split.py diff --git a/src/openjd/sessions/_runner_base.py b/src/openjd/sessions/_runner_base.py index c98c13cb..ba0f4a7a 100644 --- a/src/openjd/sessions/_runner_base.py +++ b/src/openjd/sessions/_runner_base.py @@ -39,6 +39,7 @@ "NotifyCancelMethod", "ScriptRunnerBase", "apply_let_bindings", + "apply_script_let_bindings", "resolve_action_arg_values", "resolve_effective_cancelation", "resolve_optional_int_field", @@ -494,7 +495,9 @@ def resolve_period(period: Any) -> Optional[int]: ) -def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: +def apply_let_bindings( + *, symtab: SymbolTable, let_bindings: list[str], path_format: Any = None +) -> None: """Evaluate EXPR ``let`` bindings (RFC 0005) and add them to ``symtab``. ``let_bindings`` is a script's ``let`` field: an ordered list of @@ -510,6 +513,12 @@ def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: ``Env.File.*``/``Task.File.*`` and a file's ``data`` may reference let-bound values (mirroring openjd-rs's runner ordering). + ``path_format`` is the EXPR ``PathFormat`` that PATH-typed results render + with. ``None`` -- the default, and what every session-scope binding wants -- + leaves the engine's default, which is the host's format. Callers + re-evaluating a *template*-scope binding pass ``PathFormat.POSIX``; see + :func:`apply_script_let_bindings`. + Raises: ValueError (FormatStringError/ExpressionError): if a binding's expression cannot be evaluated, or if a binding is too long to @@ -528,7 +537,84 @@ def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: ) # Single-sourced in openjd.model (parse-memoized; skips malformed # bindings; raises ValueError naming the failing binding). - evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings) + # + # The kwarg is forwarded only when it is set, because `path_format` does not + # exist on openjd-model at this package's declared floor (>= 0.11.6) and + # passing it there is a TypeError, not a no-op -- which would break EVERY + # EXPR template rather than degrading. On such a model the else branch is + # unreachable: apply_script_let_bindings reads the template-scope count + # through getattr, and a model without `path_format` has no + # `_template_scope_let_count` either, so the count is 0 and nothing asks for + # a non-default format. On a model that does have it, `None` and "omitted" + # are the same call. Collapse this to an unconditional forward once the + # openjd-model floor carries the parameter. + if path_format is None: + evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings) + else: + evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings, path_format=path_format) + + +def apply_script_let_bindings( + *, symtab: SymbolTable, let_bindings: list[str], script: Any = None +) -> None: + """Evaluate a script's ``let`` list into ``symtab``, honouring the + template-scope / session-scope boundary inside it. + + An instantiated Step's script carries a *merged* ``let`` list: the + step-level bindings the template declared, followed by the script's own + (openjd-model's ``StepTemplate.resolve_syntax_sugar``). The step-level + prefix was already evaluated at job creation, in **template** scope, which + openjd-rs -- and now openjd-model -- evaluate with ``PathFormat::Posix`` so + that a create-time result cannot depend on the host that created the job. + Re-evaluating that prefix here in the host's format re-renders its PATH + values: on Windows ``path("/foo/bar")`` becomes ``\\foo\\bar``, so + ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` to ``false`` + and the binding's value silently differs between the two evaluations. + + So the list is split at the boundary and the two halves are evaluated in + different path formats -- the prefix as POSIX, the remainder (the script's + own bindings) with the host's format, unchanged. Script-level bindings + legitimately see host-scope symbols (``Session.WorkingDirectory``, + ``Task.File.*``, ``apply_path_mapping``), so their format must stay the + host's. + + Both halves are evaluated into the **same** table in the **same** order, + because a later binding may reference an earlier one -- including a + script-level binding referencing a step-level one. + + ``script`` is the model object the ``let`` list came from. The boundary is + read off it as ``_template_scope_let_count``, through ``getattr`` with a + default of 0: an openjd-model that predates the model-side half of this fix + does not carry the attribute, and must degrade to exactly the previous + behaviour (everything in host format) rather than raising. ``None`` -- what + an environment script's caller passes -- means the same thing: an + environment script's own bindings are session scope and correctly use the + host format. + + Raises: + ValueError: as :func:`apply_let_bindings`. + """ + # min() because the count comes from a separate distribution: a model/ + # sessions version skew that reported a longer prefix than the list would + # otherwise silently evaluate nothing at all here. + template_scope_count = min(getattr(script, "_template_scope_let_count", 0), len(let_bindings)) + if template_scope_count: + # Lazy AND conditional (see the module comment on _EXTENSION_MODULE): a + # function-local import still fires unconditionally once its function is + # called, so it sits behind "there is a template-scope prefix to + # evaluate". Only a step script with step-level `let` bindings reaches + # here, and a `let` field only parses under the EXPR extension -- which + # has already loaded the extension. A non-EXPR session never gets here. + from openjd.expr import PathFormat + + apply_let_bindings( + symtab=symtab, + let_bindings=let_bindings[:template_scope_count], + path_format=PathFormat.POSIX, + ) + host_scope_bindings = let_bindings[template_scope_count:] + if host_scope_bindings: + apply_let_bindings(symtab=symtab, let_bindings=host_scope_bindings) class ScriptRunnerBase(ABC): @@ -1020,10 +1106,17 @@ def _materialize_files( symtab: SymbolTable, let_bindings: Optional[list[str]] = None, preallocated_records: Optional[list[_FileRecord]] = None, + script: Any = None, ) -> None: """Helper for derived classes that wraps all of the logic around materializing embedded files to disk. + ``script`` is the model object ``let_bindings`` came from, forwarded to + :func:`apply_script_let_bindings` so a step script's merged list is + split at its template-scope boundary. Omitting it evaluates every + binding in the host's path format, which is what an environment script + wants. + When ``let_bindings`` is given, they are evaluated between file-path allocation and content writing (RFC 0005, mirroring the openjd-rs runners): a file's *path* never depends on ``let`` values (filenames @@ -1061,7 +1154,7 @@ def _materialize_files( else: records = file_writer.allocate_file_paths(files, symtab) if let_bindings: - apply_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) file_writer.write_file_contents(records, symtab) except (RuntimeError, ValueError) as exc: # Had a problem writing at least one file to disk, or evaluating @@ -1069,12 +1162,17 @@ def _materialize_files( # ValueError). Surface the error. self._fail_action(str(exc)) - def _apply_let_bindings_or_fail(self, symtab: SymbolTable, let_bindings: list[str]) -> bool: + def _apply_let_bindings_or_fail( + self, symtab: SymbolTable, let_bindings: list[str], script: Any = None + ) -> bool: """Evaluate the script's EXPR ``let`` bindings into ``symtab``. On an evaluation error the action is failed through the normal failure path - (openjd_fail log, FAILED state, callback). Returns True on success.""" + (openjd_fail log, FAILED state, callback). Returns True on success. + + ``script`` is the model object the bindings came from; see + :func:`apply_script_let_bindings` for what it is read for.""" try: - apply_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) except ValueError as exc: self._fail_action(str(exc)) return False diff --git a/src/openjd/sessions/_runner_step_script.py b/src/openjd/sessions/_runner_step_script.py index 1d49736b..d6fd5426 100644 --- a/src/openjd/sessions/_runner_step_script.py +++ b/src/openjd/sessions/_runner_step_script.py @@ -102,6 +102,10 @@ def run(self) -> None: # the script's EXPR `let` bindings evaluate (so bindings can reference # Task.File.*), and contents are written after (so `data` can # reference let-bound values) — mirroring the openjd-rs runner. + # + # `script=self._script` is what tells the evaluation where this merged + # `let` list stops being template scope and starts being session scope; + # see apply_script_let_bindings. if self._script.embeddedFiles is not None: symtab = SymbolTable(source=self._symtab) self._materialize_files( @@ -110,12 +114,13 @@ def run(self) -> None: self._session_files_directory, symtab, let_bindings=let_bindings, + script=self._script, ) if self.state == ScriptRunnerState.FAILED: return elif let_bindings: symtab = SymbolTable(source=self._symtab) - if not self._apply_let_bindings_or_fail(symtab, let_bindings): + if not self._apply_let_bindings_or_fail(symtab, let_bindings, self._script): return else: symtab = self._symtab diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 77c55b60..05fccb9c 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -44,7 +44,7 @@ from ._path_mapping import PathMappingRule from ._runner_base import ( ScriptRunnerBase, - apply_let_bindings, + apply_script_let_bindings, resolve_action_arg_values, resolve_effective_cancelation, resolve_optional_int_field, @@ -1998,6 +1998,7 @@ def _build_wrapped_inner_scope( let_bindings: Optional[list[str]], embedded_files: Optional[Any], base: SymbolTable, + script: Any = None, ) -> SymbolTable: """Build the scope a wrapped action would have resolved against had it run unwrapped: a copy of ``base`` (the session-scope table) plus @@ -2012,6 +2013,17 @@ def _build_wrapped_inner_scope( symmetrically, the inner entity's lets never apply to the hook's own resolution scope. Mirrors openjd-rs's ``build_wrapped_inner_scope``. + ``script`` is the inner entity's script -- the model object + ``let_bindings`` came from -- forwarded so that a wrapped *step* script's + merged ``let`` list is split at its template-scope boundary exactly as + the step runner splits it (see + :func:`~._runner_base.apply_script_let_bindings`). Without it a wrapped + action would resolve against template-scope values re-rendered in the + host's path format, i.e. against a scope that differs from the one it + would have had unwrapped -- which is the whole property this method + exists to reproduce. An inner *environment* script has no such prefix and + is unaffected. + Raises: ValueError (FormatStringError/ExpressionError): a binding or file reference did not resolve. @@ -2027,10 +2039,10 @@ def _build_wrapped_inner_scope( ) records = file_writer.allocate_file_paths(embedded_files, symtab) if let_bindings: - apply_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) file_writer.write_file_contents(records, symtab) elif let_bindings: - apply_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) return symtab def _try_inject_wrapped_symbols( @@ -2061,6 +2073,7 @@ def _try_inject_wrapped_symbols( inner_script.let if inner_script is not None else None, inner_script.embeddedFiles if inner_script is not None else None, symtab, + inner_script, ) inject(inner_symtab) except (FormatStringError, ValueError, RuntimeError) as e: diff --git a/test/openjd/sessions_v0/test_template_scope_let_split.py b/test/openjd/sessions_v0/test_template_scope_let_split.py new file mode 100644 index 00000000..cee5d0a2 --- /dev/null +++ b/test/openjd/sessions_v0/test_template_scope_let_split.py @@ -0,0 +1,493 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""A step script's merged ``let`` list spans two scopes; each half must be +evaluated in its own path format. + +An instantiated Step's ``script.let`` is ``step-level bindings + the script's +own``, in that order. The step-level prefix was already evaluated at job +creation in *template* scope, which openjd-rs (and now openjd-model) evaluate +with ``PathFormat::Posix`` so a create-time value cannot depend on the host that +created the job. Re-evaluating that prefix at session time in the host's format +re-renders its PATH values -- on Windows ``path("/foo/bar")`` becomes +``\\foo\\bar``, so ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` +to ``false`` and the same binding holds a different value in the two +evaluations. That is what broke 11 conformance fixtures on the Python-on-Windows +CI leg. + +``apply_script_let_bindings`` owns the split. The script's own bindings are +session scope and keep the host's format, because they legitimately reference +``Session.WorkingDirectory``, ``Task.File.*`` and ``apply_path_mapping``. + +Note on what these tests can and cannot prove. The path format handed to each +half is asserted directly, by spying on the model's ``evaluate_let_bindings``, +rather than by comparing rendered values. On a POSIX host the host format *is* +POSIX, so a value comparison cannot distinguish "POSIX because we asked for it" +from "POSIX because that is the host" -- it would pass on this host no matter +what the code did. ``test_path_format_is_load_bearing`` supplies the missing +anchor: it shows a non-POSIX format really does change rendering, so the +argument these tests assert on is the argument that matters. The Windows +behaviour itself is not exercised here. +""" + +from __future__ import annotations + +import time +import uuid +from pathlib import Path +from typing import Any, Optional +from unittest.mock import patch as mock_patch + +import pytest + +from openjd.model import SymbolTable, evaluate_let_bindings +from openjd.model.v2023_09 import ( + ModelParsingContext as ModelParsingContext_2023_09, + StepScript as StepScript_2023_09, +) +from openjd.sessions import ActionState, Session, SessionState +from openjd.sessions._embedded_files import EmbeddedFilesScope +from openjd.sessions._runner_base import apply_let_bindings, apply_script_let_bindings +from openjd.sessions._runner_step_script import StepScriptRunner + +from .conftest import build_logger + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_A_PATH_BINDING = "path('/foo/bar')" +"""A binding RHS whose value renders differently per path format, which is the +whole reason the split exists.""" + + +class _FakeScript: + """Stands in for an instantiated ``StepScript``. + + A fake rather than a real model object because these tests are about the + *boundary index*, and the model only produces indices its own templates can + express. Reading the count off a plain attribute is exactly what the + ``getattr`` in the helper does, and the real-model wiring is pinned + separately by :class:`TestStepScriptWiring`. + """ + + def __init__(self, count: int) -> None: + self._template_scope_let_count = count + + +class _NoCountScript: + """An openjd-model that predates the model-side half of the fix: no + ``_template_scope_let_count`` attribute at all.""" + + +def _set_count(script: Any, count: int) -> None: + """Set the template-scope boundary on a real model object. + + ``setattr`` rather than a direct assignment because openjd-sessions builds + against ``openjd-model >= 0.11.6``, which does not declare the private + attribute -- a direct assignment fails ``hatch run typing`` against the + declared floor. Reaching it through ``setattr`` keeps the tests type-clean + on both model versions, which is the same reason the helper under test + reads it through ``getattr``. + """ + setattr(script, "_template_scope_let_count", count) + + +def _spy_on_evaluation(): + """Patch the model's ``evaluate_let_bindings`` where openjd-sessions imports + it, recording every call while still evaluating for real. + + Spying here rather than on ``apply_let_bindings`` keeps the + ``MAX_LET_BINDING_LENGTH`` guard and the real evaluation in the loop, so a + test can assert both the calls and the resulting symbol values. + """ + return mock_patch( + "openjd.sessions._runner_base.evaluate_let_bindings", + side_effect=evaluate_let_bindings, + ) + + +def _calls(spy: Any) -> list[tuple[list[str], Any]]: + """The spy's calls as ``[(let_bindings, path_format), ...]``. + + ``path_format`` is read with ``.get`` because ``apply_let_bindings`` omits + the kwarg entirely for the host format -- it does not exist on openjd-model + at this package's declared floor. Omitted and ``None`` are the same request + (the engine's default, i.e. the host's format), so both read as ``None`` + here. + """ + return [ + (call.kwargs["let_bindings"], call.kwargs.get("path_format")) for call in spy.call_args_list + ] + + +def _posix_format() -> Any: + from openjd.expr import PathFormat + + return PathFormat.POSIX + + +def _step_script( + let: list[str], command: str = "echo", args: Optional[list[str]] = None +) -> StepScript_2023_09: + """A real ``StepScript`` carrying ``let``. The merged-list *boundary* is set + by the caller via ``_template_scope_let_count``, because building it through + ``create_job`` would drag a whole job template into a test about one + index.""" + context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) + return StepScript_2023_09.model_validate( + {"let": let, "actions": {"onRun": {"command": command, "args": args or ["ok"]}}}, + context=context, + ) + + +# --------------------------------------------------------------------------- +# The helper +# --------------------------------------------------------------------------- + + +class TestApplyScriptLetBindings: + def test_splits_at_the_template_scope_count(self) -> None: + # GIVEN: four bindings, of which the first two are step level. + bindings = ["a = 1", "b = 2", "c = 3", "d = 4"] + symtab = SymbolTable() + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(2)) + + # THEN: exactly two evaluations, split at index 2, prefix first. + assert _calls(spy) == [ + (["a = 1", "b = 2"], _posix_format()), + (["c = 3", "d = 4"], None), + ] + # ...and both halves landed in the SAME table. + assert [str(symtab[name]) for name in ("a", "b", "c", "d")] == ["1", "2", "3", "4"] + + def test_prefix_evaluates_posix_and_suffix_evaluates_host_format(self) -> None: + # GIVEN + bindings = [f"tmpl = {_A_PATH_BINDING}", f"own = {_A_PATH_BINDING}"] + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(1) + ) + + # THEN: the template-scope half is pinned to POSIX; the session-scope + # half is left at the engine default, which is the host's format. + prefix, suffix = _calls(spy) + assert prefix == ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()) + assert suffix == ([f"own = {_A_PATH_BINDING}"], None) + + def test_path_format_is_load_bearing(self) -> None: + """The anchor for the assertions above: a path format other than POSIX + really does change how a PATH value renders, so forwarding the argument + is not cosmetic. Without this, a mutant that passed the host format for + the prefix would only be caught by an argument comparison that could + itself be dismissed as testing the mock.""" + from openjd.expr import PathFormat + + posix, windows = SymbolTable(), SymbolTable() + + # WHEN + apply_let_bindings( + symtab=posix, let_bindings=[f"p = {_A_PATH_BINDING}"], path_format=PathFormat.POSIX + ) + apply_let_bindings( + symtab=windows, let_bindings=[f"p = {_A_PATH_BINDING}"], path_format=PathFormat.WINDOWS + ) + + # THEN + assert str(posix["p"]) == "/foo/bar" + assert str(windows["p"]) == "\\foo\\bar" + # And the flip that broke the fixtures, reproduced without a Windows host. + assert str(posix["p"]).startswith("/foo") + assert not str(windows["p"]).startswith("/foo") + + def test_no_template_scope_prefix_behaves_exactly_as_before(self) -> None: + # GIVEN: a script with only its own bindings -- count 0. + bindings = ["a = 1", "b = 2"] + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(0) + ) + + # THEN: one evaluation, host format, whole list. No POSIX evaluation at + # all -- a count of 0 must not produce an empty extra call. + assert _calls(spy) == [(bindings, None)] + + def test_missing_count_attribute_falls_back_to_host_format(self) -> None: + """An openjd-model without the model-side half of the fix must degrade + to the previous behaviour, not raise.""" + # GIVEN + bindings = ["a = 1", "b = 2"] + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=SymbolTable(), let_bindings=bindings, script=_NoCountScript() + ) + + # THEN + assert _calls(spy) == [(bindings, None)] + + def test_no_script_falls_back_to_host_format(self) -> None: + """What an environment script's caller passes: its own bindings are + session scope and correctly use the host format.""" + # GIVEN + bindings = ["a = 1"] + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=SymbolTable(), let_bindings=bindings) + + # THEN + assert _calls(spy) == [(bindings, None)] + + def test_count_beyond_the_list_is_clamped(self) -> None: + """A model/sessions version skew reporting a longer prefix than the list + must still evaluate every binding, not silently none.""" + # GIVEN + bindings = ["a = 1"] + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(5) + ) + + # THEN + assert _calls(spy) == [(bindings, _posix_format())] + + def test_ordering_is_preserved_across_the_boundary(self) -> None: + """A script-level binding may reference a step-level one, so the prefix + must be evaluated -- into the same table -- before the suffix.""" + # GIVEN: `under` is session scope and reads `root`, which is template + # scope. + bindings = [f"root = {_A_PATH_BINDING}", "under = startswith(root, '/foo')"] + symtab = SymbolTable() + + # WHEN + apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) + + # THEN + assert str(symtab["root"]) == "/foo/bar" + assert str(symtab["under"]) == "true" + + def test_a_failing_suffix_binding_still_raises(self) -> None: + """The split must not swallow an evaluation error in either half.""" + # WHEN / THEN + with pytest.raises(ValueError, match="let binding 'bad'"): + apply_script_let_bindings( + symtab=SymbolTable(), + let_bindings=["ok = 1", "bad = NoSuchSymbol"], + script=_FakeScript(1), + ) + + +# --------------------------------------------------------------------------- +# The wiring: the runner and the RFC 0008 wrapped-inner-scope path +# --------------------------------------------------------------------------- + + +@pytest.mark.usefixtures("message_queue", "queue_handler") +class TestStepScriptWiring: + """The helper is only useful if the sites that evaluate a step script's + merged ``let`` actually hand it the script.""" + + def _run( + self, + queue_handler: Any, + session_dir: Path, + script: StepScript_2023_09, + count: int, + ) -> list[tuple[list[str], Any]]: + _set_count(script, count) + # `with runner:` rather than `with StepScriptRunner(...) as runner:` -- + # ScriptRunnerBase.__enter__ is annotated as returning the base class, so + # the `as` form loses the subclass and with it `run()`. + runner = StepScriptRunner( + logger=build_logger(queue_handler), + script=script, + symtab=SymbolTable(), + session_working_directory=session_dir, + session_files_directory=session_dir, + ) + with runner: + with _spy_on_evaluation() as spy: + runner.run() + deadline = time.time() + 20 + while runner.state.value == "running" and time.time() < deadline: + time.sleep(0.05) + return _calls(spy) + + def test_step_runner_splits_its_merged_let( + self, queue_handler: Any, tmp_path: Path, python_exe: str + ) -> None: + # GIVEN: a step script whose first binding is step level. + script = _step_script( + [f"tmpl = {_A_PATH_BINDING}", "own = 1"], + command=python_exe, + args=["-c", "pass"], + ) + + # WHEN + calls = self._run(queue_handler, tmp_path, script, count=1) + + # THEN + assert calls == [ + ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()), + (["own = 1"], None), + ] + + def test_step_runner_with_embedded_files_splits_its_merged_let( + self, queue_handler: Any, tmp_path: Path, python_exe: str + ) -> None: + """The embedded-files branch evaluates the same merged list through + ``_materialize_files``, so it needs the same split. Missing this leaves + the bug live for any step that has both step-level bindings and + embedded files.""" + # GIVEN + context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) + script = StepScript_2023_09.model_validate( + { + "let": [f"tmpl = {_A_PATH_BINDING}", "own = 1"], + "embeddedFiles": [{"name": "F", "type": "TEXT", "data": "{{ tmpl }}"}], + "actions": {"onRun": {"command": python_exe, "args": ["-c", "pass"]}}, + }, + context=context, + ) + + # WHEN + calls = self._run(queue_handler, tmp_path, script, count=1) + + # THEN + assert calls == [ + ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()), + (["own = 1"], None), + ] + + def test_wrapped_inner_scope_splits_a_step_scripts_merged_let(self) -> None: + """RFC 0008: the wrapped action's scope is rebuilt from the inner + script's ``let``, so it must be split the same way -- otherwise a + wrapped action resolves against a scope that differs from the one it + would have had unwrapped.""" + # GIVEN + script = _step_script([f"tmpl = {_A_PATH_BINDING}", "own = 1"]) + _set_count(script, 1) + session = Session(session_id=uuid.uuid4().hex, job_parameter_values={}) + + # WHEN + try: + with _spy_on_evaluation() as spy: + inner = session._build_wrapped_inner_scope( + EmbeddedFilesScope.STEP, script.let, None, SymbolTable(), script + ) + finally: + session.cleanup() + + # THEN + assert _calls(spy) == [ + ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()), + (["own = 1"], None), + ] + assert str(inner["tmpl"]) == "/foo/bar" + + def test_environment_script_bindings_stay_host_format( + self, queue_handler: Any, tmp_path: Path, python_exe: str + ) -> None: + """The other half of the contract: an environment script's own ``let`` + is session scope. Nothing about it may change.""" + from openjd.model.v2023_09 import EnvironmentScript as EnvironmentScript_2023_09 + from openjd.sessions._runner_env_script import EnvironmentScriptRunner + + # GIVEN + context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) + env_script = EnvironmentScript_2023_09.model_validate( + { + "let": [f"a = {_A_PATH_BINDING}", "b = 1"], + "actions": {"onEnter": {"command": python_exe, "args": ["-c", "pass"]}}, + }, + context=context, + ) + + # WHEN + runner = EnvironmentScriptRunner( + logger=build_logger(queue_handler), + environment_script=env_script, + symtab=SymbolTable(), + session_working_directory=tmp_path, + session_files_directory=tmp_path, + ) + with runner: + with _spy_on_evaluation() as spy: + runner.enter() + deadline = time.time() + 20 + while runner.state.value == "running" and time.time() < deadline: + time.sleep(0.05) + calls = _calls(spy) + + # THEN: one evaluation, host format, whole list. + assert calls == [([f"a = {_A_PATH_BINDING}", "b = 1"], None)] + + +@pytest.mark.usefixtures("message_queue", "queue_handler") +class TestEndToEndScopeAgreement: + """The property the conformance fixtures actually check: the value a + step-level binding holds at session time equals the value it held at job + creation.""" + + def test_a_step_level_path_binding_agrees_across_the_two_evaluations( + self, python_exe: str + ) -> None: + # GIVEN: a step script whose step-level prefix is a path predicate -- + # the shape that flipped on Windows. + script = _step_script( + [f"root = {_A_PATH_BINDING}", "under = startswith(root, '/foo')"], + command=python_exe, + ) + _set_count(script, 2) + + # AND: the value the model computed at job creation, in template scope. + create_time = SymbolTable() + apply_let_bindings( + symtab=create_time, let_bindings=script.let or [], path_format=_posix_format() + ) + + # WHEN: the session re-evaluates the same list. + session_time = SymbolTable() + apply_script_let_bindings(symtab=session_time, let_bindings=script.let or [], script=script) + + # THEN: the two agree. On a POSIX host they would agree either way; the + # per-half format assertions above are what make this host-independent. + assert str(session_time["root"]) == str(create_time["root"]) + assert str(session_time["under"]) == str(create_time["under"]) == "true" + + def test_a_session_runs_a_step_with_a_step_level_path_binding(self, python_exe: str) -> None: + """End to end through the public API, so the split cannot break the + ordinary run.""" + # GIVEN: the action's exit status is driven by the step-level binding's + # value, so a scope disagreement fails the action rather than passing + # quietly. + script = _step_script( + [f"root = {_A_PATH_BINDING}", "under = startswith(root, '/foo')"], + command=python_exe, + args=["-c", "import sys; sys.exit(0 if sys.argv[1] == 'true' else 1)", "{{ under }}"], + ) + _set_count(script, 1) + + session = Session(session_id=uuid.uuid4().hex, job_parameter_values={}) + try: + # WHEN + session.run_task(step_script=script, task_parameter_values={}) + deadline = time.time() + 20 + while session.state == SessionState.RUNNING and time.time() < deadline: + time.sleep(0.05) + + # THEN + status = session.action_status + assert status is not None and status.state == ActionState.SUCCESS, status + finally: + session.cleanup() diff --git a/test/openjd/test_import_purity.py b/test/openjd/test_import_purity.py index 3e59e8bd..f3725675 100644 --- a/test/openjd/test_import_purity.py +++ b/test/openjd/test_import_purity.py @@ -407,3 +407,63 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: "extension; if this is now False the limitation has been fixed and " "this control should become a purity assertion" ) + + +# --------------------------------------------------------------------------- +# `apply_script_let_bindings` imports `openjd.expr.PathFormat` to pin a step +# script's template-scope `let` prefix to POSIX. A function-local import is not +# enough on its own (rule: lazy is not conditional) -- the enclosing function is +# reachable from every script that has any `let` at all, so the import has to sit +# behind "there is a template-scope prefix to evaluate". +# +# Both probes use a MALFORMED binding, which openjd-model skips without parsing. +# That removes the evaluation itself as a possible cause of the load, leaving the +# PathFormat import as the only crossing either probe can observe. +# --------------------------------------------------------------------------- + + +_LET_SPLIT_PROBE = """ +from openjd.model import SymbolTable +from openjd.sessions._runner_base import apply_script_let_bindings + + +class Script: + _template_scope_let_count = %d + + +apply_script_let_bindings( + symtab=SymbolTable(), let_bindings=["malformed"], script=Script() +) +print(RS in sys.modules) +""" + + +def test_a_let_list_with_no_template_scope_prefix_stays_pure(tmp_path: Path) -> None: + """A script whose ``let`` is entirely its own -- the only shape a non-EXPR + template can even produce -- must not reach the ``PathFormat`` import.""" + # WHEN + loaded = _run_probe(tmp_path, _LET_SPLIT_PROBE % 0) + + # THEN + assert loaded == "False", ( + "evaluating a let list with no template-scope prefix loaded the native " + "extension. The `if template_scope_count:` guard around the PathFormat " + "import in apply_script_let_bindings is what prevents this; a bare " + "function-local import is not sufficient." + ) + + +def test_a_template_scope_prefix_does_load_the_extension(tmp_path: Path) -> None: + """Positive control for the probe above. Without it, ``False`` would be + indistinguishable from the probe being unable to observe the load at all -- + and it confirms the guarded import is the only crossing on this path, since + the malformed binding is never parsed.""" + # WHEN + loaded = _run_probe(tmp_path, _LET_SPLIT_PROBE % 1) + + # THEN + assert loaded == "True", ( + "a template-scope prefix must load the extension to reach " + "PathFormat.POSIX; if this is False the prefix is no longer being pinned " + "to POSIX at all" + ) From 32eafb28217b74e1288660bc338b9dc9ea8b2c96 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:56:06 -0700 Subject: [PATCH 02/11] fix: Re-tag template-scope let values to host format before seeding them The previous commit evaluated the template-scope prefix with `PathFormat.POSIX` and seeded the result directly. That is wrong, and on Windows it is worse than the bug it replaced. An EXPR path value carries its format, and reading one under a different format is a hard error rather than a re-render: ExpressionError: Path format mismatch for 'root': value has Posix but evaluator uses Windows Action arguments, embedded-file `data` and environment-variable values all resolve in the host's format, so a Posix-tagged path seeded into the session table made every one of those reads raise. That would have broken `EXPR/jobs/expr2.3.2--path-construction`, a conformance fixture that passes today and asserts `STR:\a\b` on Windows. The conformance suite states the rule precisely. A binding whose result leaves path-space must freeze the text template scope produced, so `expr2.2.1--string-conversion` wants `/mnt/out` on both platforms. A binding whose result is still a path must render in the host's format, so `expr2.3.2--path-construction` wants `\a\b` on Windows. So `_apply_template_scope_let_bindings` now evaluates the prefix into a child table with POSIX, then re-tags the results to the host's format through a `SerializedSymbolTable` round trip before seeding them. That leaves a frozen string alone and re-renders a live path, which is both halves of the rule. It mirrors how a create-time table already reaches a session, via `Session._resolved_base_entries` and its `to_symtab(path_format=host_format)`. Verified with the host format forced to Windows: a step-level `path('/a/b')` reads back as `\a\b`, `string(path('/mnt/out'))` stays `/mnt/out`, `startswith(path('/foo/bar'), '/foo')` stays `true`, and a session-scope binding is untouched. Also from review: an out-of-range boundary now falls back to session scope instead of being clamped into range. Clamping would evaluate a genuinely session-scope binding in template scope; the fallback is the pre-fix behaviour, which never mis-scopes. Two comments that over-claimed are corrected, one about this module having a single crossing into `openjd.expr` and one about a branch being unreachable when this PR's own tests reach it. Suite: 1012 passed, 0 failed, 40 skipped, 16 xfailed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_runner_base.py | 154 ++++++++++++------ .../test_template_scope_let_split.py | 27 ++- 2 files changed, 129 insertions(+), 52 deletions(-) diff --git a/src/openjd/sessions/_runner_base.py b/src/openjd/sessions/_runner_base.py index ba0f4a7a..bbc3f5c4 100644 --- a/src/openjd/sessions/_runner_base.py +++ b/src/openjd/sessions/_runner_base.py @@ -166,12 +166,14 @@ class _ExprKind(Enum): def _classify_expr_value(value: Any) -> _ExprKind: """Classify ``value`` against the EXPR type system. - This is the *only* place in this module that imports ``openjd.expr``, so - that every crossing into the native extension sits behind the one + This is one of two places in this module that import ``openjd.expr``, and + it is the one reached from the non-EXPR path, so it sits behind the ``sys.modules`` guard below. Doing the whole classification here rather than exposing a separate "is it a list" predicate keeps that property structural instead of merely documented: there is no second, unguarded - entry point for a future caller to reach with an arbitrary value. + entry point for a future caller to reach with an arbitrary value. The other + crossing is in :func:`_apply_template_scope_let_bindings`, guarded instead + by a non-zero template-scope prefix, which only an EXPR template produces. ``ExprValue`` instances are created only by the native extension, so if that extension has not been loaded then ``value`` cannot be one and the @@ -542,10 +544,10 @@ def apply_let_bindings( # exist on openjd-model at this package's declared floor (>= 0.11.6) and # passing it there is a TypeError, not a no-op -- which would break EVERY # EXPR template rather than degrading. On such a model the else branch is - # unreachable: apply_script_let_bindings reads the template-scope count - # through getattr, and a model without `path_format` has no - # `_template_scope_let_count` either, so the count is 0 and nothing asks for - # a non-default format. On a model that does have it, `None` and "omitted" + # unreachable from any internal caller: apply_script_let_bindings reads + # the template-scope count through getattr, and a model without + # `path_format` has no `_template_scope_let_count` either, so the count is 0 + # and nothing asks for a non-default format. Tests call it directly. On a model that does have it, `None` and "omitted" # are the same call. Collapse this to an unconditional forward once the # openjd-model floor carries the parameter. if path_format is None: @@ -554,6 +556,76 @@ def apply_let_bindings( evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings, path_format=path_format) +def _host_path_format() -> Any: + """The EXPR ``PathFormat`` for this host. + + The Python engine bindings expose no ``PathFormat.host()``, so it is derived + the same way :meth:`Session._resolved_base_entries` derives it. + """ + import os + + from openjd.expr import PathFormat + + return PathFormat.WINDOWS if os.name == "nt" else PathFormat.POSIX + + +def _apply_template_scope_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: + """Evaluate template-scope ``let`` bindings and seed them in host format. + + Two steps, and both are load-bearing. + + The bindings are evaluated with ``PathFormat.POSIX``, because that is what + openjd-model and openjd-rs use at job creation, so a value that leaves + path-space during evaluation freezes the same text it froze there. That + covers ``string(path(...))``, ``join(...)``, ``repr_sh(...)``, ``.parts`` + and every comparison against a POSIX literal. + + The results are then re-tagged to the host's format before they are seeded. + An EXPR path value carries its format, and reading one under a different + format is a hard error rather than a re-render:: + + ExpressionError: Path format mismatch for 'root': + value has Posix but evaluator uses Windows + + Action arguments, embedded-file ``data`` and environment-variable values all + resolve in the host's format, so seeding a Posix-tagged path would make + every one of those reads raise on Windows. The re-tag round trip leaves a + frozen string alone and re-renders a live path, which is exactly the split + the conformance suite asks for: ``expr2.2.1--string-conversion`` wants + ``/mnt/out`` on both platforms, while ``expr2.3.2--path-construction`` wants + ``\\a\\b`` on Windows. + + This mirrors how a create-time table already reaches a session: + :meth:`Session._resolved_base_entries` deserializes it with + ``to_symtab(path_format=host_format)``. + """ + from openjd.expr import PathFormat, SerializedSymbolTable + + from openjd.model._format_strings._expr_support import symtab_to_expr_values + + # A child table, so a binding can read the symbols already in scope without + # the POSIX evaluation writing host-format neighbours back into `symtab`. + scratch = SymbolTable(source=symtab) + apply_let_bindings(symtab=scratch, let_bindings=let_bindings, path_format=PathFormat.POSIX) + + # Only the names these bindings defined. A malformed binding is skipped by + # the evaluator, so membership is checked rather than assumed. + holder = SymbolTable() + for binding in let_bindings: + name = binding.partition("=")[0].strip() + if name and name in scratch: + holder[name] = scratch[name] + if not holder.symbols: + return + + engine = symtab_to_expr_values( + holder, types=getattr(holder, "expr_types", None), path_format=PathFormat.POSIX + ) + retagged = SerializedSymbolTable.from_symtab(engine).to_symtab(path_format=_host_path_format()) + for name in retagged.symbols: + symtab[name] = retagged[name] + + def apply_script_let_bindings( *, symtab: SymbolTable, let_bindings: list[str], script: Any = None ) -> None: @@ -563,54 +635,40 @@ def apply_script_let_bindings( An instantiated Step's script carries a *merged* ``let`` list: the step-level bindings the template declared, followed by the script's own (openjd-model's ``StepTemplate.resolve_syntax_sugar``). The step-level - prefix was already evaluated at job creation, in **template** scope, which - openjd-rs -- and now openjd-model -- evaluate with ``PathFormat::Posix`` so - that a create-time result cannot depend on the host that created the job. - Re-evaluating that prefix here in the host's format re-renders its PATH - values: on Windows ``path("/foo/bar")`` becomes ``\\foo\\bar``, so - ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` to ``false`` - and the binding's value silently differs between the two evaluations. - - So the list is split at the boundary and the two halves are evaluated in - different path formats -- the prefix as POSIX, the remainder (the script's - own bindings) with the host's format, unchanged. Script-level bindings - legitimately see host-scope symbols (``Session.WorkingDirectory``, - ``Task.File.*``, ``apply_path_mapping``), so their format must stay the - host's. - - Both halves are evaluated into the **same** table in the **same** order, - because a later binding may reference an earlier one -- including a - script-level binding referencing a step-level one. + prefix is template scope and was already evaluated as such at job creation. + Re-evaluating it here in the host's format changes its value: on Windows + ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` to ``false``. + + So the list is split. The prefix goes through + :func:`_apply_template_scope_let_bindings`, which reproduces the + create-time value and then seeds it in host format. The remainder is the + script's own bindings, evaluated unchanged in the host's format, because + they legitimately reference host-scope symbols + (``Session.WorkingDirectory``, ``Task.File.*``, ``apply_path_mapping``). + + Both halves land in the **same** table in the **same** order, so a + script-level binding can reference a step-level one. ``script`` is the model object the ``let`` list came from. The boundary is - read off it as ``_template_scope_let_count``, through ``getattr`` with a - default of 0: an openjd-model that predates the model-side half of this fix - does not carry the attribute, and must degrade to exactly the previous - behaviour (everything in host format) rather than raising. ``None`` -- what - an environment script's caller passes -- means the same thing: an - environment script's own bindings are session scope and correctly use the - host format. + read off it as ``_template_scope_let_count`` through ``getattr`` with a + default of 0, so an openjd-model that predates the model-side half of this + fix degrades to the previous behaviour rather than raising. ``None`` -- + what an environment script's caller passes -- means the same thing: an + environment script's own bindings are session scope. Raises: ValueError: as :func:`apply_let_bindings`. """ - # min() because the count comes from a separate distribution: a model/ - # sessions version skew that reported a longer prefix than the list would - # otherwise silently evaluate nothing at all here. - template_scope_count = min(getattr(script, "_template_scope_let_count", 0), len(let_bindings)) + template_scope_count = getattr(script, "_template_scope_let_count", 0) + if not 0 <= template_scope_count <= len(let_bindings): + # A model/sessions version skew reported a boundary this list cannot + # have. Treating everything as session scope is the previous behaviour, + # which is wrong on Windows but never raises; guessing a prefix could + # evaluate a genuinely session-scope binding in the wrong scope. + template_scope_count = 0 if template_scope_count: - # Lazy AND conditional (see the module comment on _EXTENSION_MODULE): a - # function-local import still fires unconditionally once its function is - # called, so it sits behind "there is a template-scope prefix to - # evaluate". Only a step script with step-level `let` bindings reaches - # here, and a `let` field only parses under the EXPR extension -- which - # has already loaded the extension. A non-EXPR session never gets here. - from openjd.expr import PathFormat - - apply_let_bindings( - symtab=symtab, - let_bindings=let_bindings[:template_scope_count], - path_format=PathFormat.POSIX, + _apply_template_scope_let_bindings( + symtab=symtab, let_bindings=let_bindings[:template_scope_count] ) host_scope_bindings = let_bindings[template_scope_count:] if host_scope_bindings: diff --git a/test/openjd/sessions_v0/test_template_scope_let_split.py b/test/openjd/sessions_v0/test_template_scope_let_split.py index cee5d0a2..4320dacd 100644 --- a/test/openjd/sessions_v0/test_template_scope_let_split.py +++ b/test/openjd/sessions_v0/test_template_scope_let_split.py @@ -246,9 +246,14 @@ def test_no_script_falls_back_to_host_format(self) -> None: # THEN assert _calls(spy) == [(bindings, None)] - def test_count_beyond_the_list_is_clamped(self) -> None: - """A model/sessions version skew reporting a longer prefix than the list - must still evaluate every binding, not silently none.""" + def test_count_beyond_the_list_falls_back_to_session_scope(self) -> None: + """A model/sessions version skew reporting a boundary the list cannot + have is not guessed at. + + Clamping to the list length was the earlier behaviour and it is worse: + it would evaluate a genuinely session-scope binding in template scope. + Falling back to 0 is the pre-fix behaviour, which is wrong on Windows + but never raises and never mis-scopes a binding.""" # GIVEN bindings = ["a = 1"] @@ -258,8 +263,22 @@ def test_count_beyond_the_list_is_clamped(self) -> None: symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(5) ) + # THEN: one evaluation, host format, whole list. + assert _calls(spy) == [(bindings, None)] + + def test_negative_count_falls_back_to_session_scope(self) -> None: + """Same guard, other side: a negative boundary is impossible.""" + # GIVEN + bindings = ["a = 1", "b = 2"] + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(-1) + ) + # THEN - assert _calls(spy) == [(bindings, _posix_format())] + assert _calls(spy) == [(bindings, None)] def test_ordering_is_preserved_across_the_boundary(self) -> None: """A script-level binding may reference a step-level one, so the prefix From 473d2eb488a19145d22f75f8017f01eb9dd6c079 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 21:22:27 -0700 Subject: [PATCH 03/11] test: Put both step-level bindings in the template-scope prefix The end-to-end test used count=1, leaving the second binding in session scope. On a Windows host that binding reads its path neighbour in host format and evaluates to false, which is the designed behaviour, so the test asserted true for a value that is false there. It passed on POSIX and would have failed Windows CI the first time those legs ran. Both step-level bindings belong in the prefix, which is what the test is about. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/sessions_v0/test_template_scope_let_split.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/openjd/sessions_v0/test_template_scope_let_split.py b/test/openjd/sessions_v0/test_template_scope_let_split.py index 4320dacd..a3432fa1 100644 --- a/test/openjd/sessions_v0/test_template_scope_let_split.py +++ b/test/openjd/sessions_v0/test_template_scope_let_split.py @@ -495,7 +495,12 @@ def test_a_session_runs_a_step_with_a_step_level_path_binding(self, python_exe: command=python_exe, args=["-c", "import sys; sys.exit(0 if sys.argv[1] == 'true' else 1)", "{{ under }}"], ) - _set_count(script, 1) + # count=2, so BOTH bindings are template scope. With count=1, `under` + # would be session scope, and on a Windows host it reads `root` in host + # format and evaluates to `false` -- correct behaviour, but it would make + # this assertion fail there while passing on POSIX. The step-level pair + # is what this test is about, so both belong in the prefix. + _set_count(script, 2) session = Session(session_id=uuid.uuid4().hex, job_parameter_values={}) try: From 2d2cfc1c8a66b774a7550022e5b0a620566d757b Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 22:31:02 -0700 Subject: [PATCH 04/11] fix: Narrow the template-scope let prefix to format-neutral symbols The prefix was evaluated with PathFormat.POSIX against a child of the session symbol table, which holds host-format values. On a Windows host that either raises `Path format mismatch` for a PATH job parameter or a natively seeded create-time value, or silently succeeds against a re-rendered one -- `.parent` of a Windows path read as POSIX is '.', because a backslash is an ordinary POSIX path character. The previous commit fixed the output side and left this input side broken. The prefix now evaluates against only the symbols in scope that carry no path format, plus the prefix bindings already bound. The filter tests shape rather than name, because the set of session symbols grows and a name denylist would rot: a symbol is excluded if its value is a native path-typed engine value, or its expr_types entry declares PATH or LIST[PATH]. That matches the measured blast radius exactly. A prefix binding that needs an excluded symbol now fails with `Undefined variable`, and the whole let list falls back to one host-format evaluation. All-or-nothing on purpose: freezing per binding would leave a POSIX-evaluated binding reading a host-evaluated sibling, which is the same cross-format read somewhere less visible. The fallback is the pre-fix behaviour exactly, so it cannot regress, and a genuine evaluation error still surfaces from it with the same message. This makes the change a smaller claim: it freezes self-contained template-scope bindings and declines the rest. Reproducing the rest needs the create-time value carried to the session rather than recomputed. 9 tests added, each mutation-checked against a revert of the production change. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_runner_base.py | 163 +++++++++++++--- .../test_template_scope_let_split.py | 178 ++++++++++++++++++ 2 files changed, 315 insertions(+), 26 deletions(-) diff --git a/src/openjd/sessions/_runner_base.py b/src/openjd/sessions/_runner_base.py index bbc3f5c4..99b26e69 100644 --- a/src/openjd/sessions/_runner_base.py +++ b/src/openjd/sessions/_runner_base.py @@ -569,44 +569,139 @@ def _host_path_format() -> Any: return PathFormat.WINDOWS if os.name == "nt" else PathFormat.POSIX -def _apply_template_scope_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: - """Evaluate template-scope ``let`` bindings and seed them in host format. +def _type_carries_path_format(expr_type: Any) -> bool: + """Whether an EXPR type renders a path, and so carries a path format. - Two steps, and both are load-bearing. + ``list[path]`` carries one as much as ``path`` does, so the type parameters + are walked rather than only the outer type code. + """ + from openjd.expr import TypeCode + + if expr_type.type_code == TypeCode.PATH: + return True + return any(_type_carries_path_format(param) for param in expr_type.type_params) + + +def _is_format_neutral(value: Any, declared_type: Optional[str]) -> bool: + """Whether a symbol can be read under a path format other than the one it + was built in. + + Tested by *shape*, not by name, because the set of session symbols grows and + a name denylist would rot. Two shapes carry a path format, and the session + symbol table holds both: + + - a native engine value that is itself path-typed, which is what + :meth:`Session._resolved_base_entries` seeds (a create-time table + deserialized in host format); and + - a plain string (or list of strings) whose ``expr_types`` entry declares it + ``PATH`` or ``LIST[PATH]``, which is what ``Session.WorkingDirectory`` and + every path-typed ``Param.*``/``Task.Param.*`` are. + + ``"PATH" in declared_type`` covers both ``PATH`` and ``LIST[PATH]``; no other + OpenJD parameter type name contains it. + + Fails closed: a value whose type cannot be determined is treated as carrying + a format, because wrongly *including* a path-typed symbol is the defect this + filter exists to prevent, while wrongly excluding one only triggers the + fallback in :func:`_apply_template_scope_let_bindings`. + """ + if declared_type is not None and "PATH" in declared_type: + return False + + from openjd.expr import ExprValue + + if not isinstance(value, ExprValue): + # A plain Python value carries no format of its own; its `expr_types` + # entry, checked above, is the only thing that could give it one. + return True + try: + return not _type_carries_path_format(value.type) + except Exception: + return False - The bindings are evaluated with ``PathFormat.POSIX``, because that is what - openjd-model and openjd-rs use at job creation, so a value that leaves + +def _apply_template_scope_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> bool: + """Evaluate self-contained template-scope ``let`` bindings and seed them in + host format. Returns whether the bindings could be evaluated this way. + + Three parts, and all three are load-bearing. + + **The bindings are evaluated with ``PathFormat.POSIX``**, because that is + what openjd-model and openjd-rs use at job creation, so a value that leaves path-space during evaluation freezes the same text it froze there. That covers ``string(path(...))``, ``join(...)``, ``repr_sh(...)``, ``.parts`` and every comparison against a POSIX literal. - The results are then re-tagged to the host's format before they are seeded. - An EXPR path value carries its format, and reading one under a different - format is a hard error rather than a re-render:: - - ExpressionError: Path format mismatch for 'root': - value has Posix but evaluator uses Windows - - Action arguments, embedded-file ``data`` and environment-variable values all + **They are evaluated against only the format-neutral symbols in scope** + (:func:`_is_format_neutral`), plus the prefix bindings already bound. A + POSIX evaluation cannot read a symbol that carries a path format: the + session table holds host-format values, and on a Windows host reading one + either raises:: + + ValueError: let binding 'out': Path format mismatch for + 'Session.WorkingDirectory': value has Windows but + evaluator uses Posix + + or, worse, silently succeeds against a re-rendered value -- a ``.parent`` of + a Windows path read as POSIX is ``'.'``, because a backslash is an ordinary + character in a POSIX path. Neither is acceptable, so those symbols are not + in scope for this evaluation at all. + + **The results are then re-tagged to the host's format** before they are + seeded. An EXPR path value carries its format, and reading one under a + different format is the same hard error in the other direction. Action + arguments, embedded-file ``data`` and environment-variable values all resolve in the host's format, so seeding a Posix-tagged path would make every one of those reads raise on Windows. The re-tag round trip leaves a frozen string alone and re-renders a live path, which is exactly the split the conformance suite asks for: ``expr2.2.1--string-conversion`` wants ``/mnt/out`` on both platforms, while ``expr2.3.2--path-construction`` wants - ``\\a\\b`` on Windows. - - This mirrors how a create-time table already reaches a session: - :meth:`Session._resolved_base_entries` deserializes it with + ``\\a\\b`` on Windows. This mirrors how a create-time table already reaches + a session: :meth:`Session._resolved_base_entries` deserializes it with ``to_symtab(path_format=host_format)``. + + Returns: + bool: ``True`` when the prefix was evaluated in template scope and + seeded. ``False`` when it could not be -- because a binding reads a + symbol that carries a path format, and so is missing from the + narrowed scope, or the POSIX evaluation failed for any other reason. + The caller then abandons the split for the whole ``let`` list. This + is all-or-nothing on purpose: freezing per binding would let a + POSIX-evaluated binding read a host-evaluated sibling, which is the + same cross-format read in a new place. Nothing has been written to + ``symtab`` when ``False`` is returned, so the caller's fallback + starts from an untouched table. """ from openjd.expr import PathFormat, SerializedSymbolTable from openjd.model._format_strings._expr_support import symtab_to_expr_values - # A child table, so a binding can read the symbols already in scope without - # the POSIX evaluation writing host-format neighbours back into `symtab`. - scratch = SymbolTable(source=symtab) - apply_let_bindings(symtab=scratch, let_bindings=let_bindings, path_format=PathFormat.POSIX) + # A separate table, so the POSIX evaluation neither writes host-format + # neighbours back into `symtab` nor reads a symbol whose format it cannot + # honour. Bindings land in it in order, so a later prefix binding still + # reads an earlier one. + declared_types = getattr(symtab, "expr_types", None) or {} + scratch = SymbolTable() + # Host-context rules ride along unchanged: a prefix binding that resolved + # through `apply_path_mapping` before this narrowing still resolves. + if symtab.expr_host_rules is not None: + scratch.expr_host_rules = list(symtab.expr_host_rules) + for name in symtab.symbols: + if _is_format_neutral(symtab[name], declared_types.get(name)): + scratch[name] = symtab[name] + if name in declared_types: + scratch.expr_types[name] = declared_types[name] + + try: + apply_let_bindings(symtab=scratch, let_bindings=let_bindings, path_format=PathFormat.POSIX) + except ValueError: + # Most often an `Undefined variable` for a symbol the filter removed. + # The catch is deliberately not narrowed to that: every other failure + # mode is also one where this prefix cannot be reproduced in template + # scope, and the caller's fallback -- evaluating the whole list in the + # host's format -- is precisely the pre-fix behaviour, which re-raises + # a genuine error with the same message rather than swallowing it. + return False # Only the names these bindings defined. A malformed binding is skipped by # the evaluator, so membership is checked rather than assumed. @@ -616,7 +711,7 @@ def _apply_template_scope_let_bindings(*, symtab: SymbolTable, let_bindings: lis if name and name in scratch: holder[name] = scratch[name] if not holder.symbols: - return + return True engine = symtab_to_expr_values( holder, types=getattr(holder, "expr_types", None), path_format=PathFormat.POSIX @@ -624,6 +719,7 @@ def _apply_template_scope_let_bindings(*, symtab: SymbolTable, let_bindings: lis retagged = SerializedSymbolTable.from_symtab(engine).to_symtab(path_format=_host_path_format()) for name in retagged.symbols: symtab[name] = retagged[name] + return True def apply_script_let_bindings( @@ -649,6 +745,16 @@ def apply_script_let_bindings( Both halves land in the **same** table in the **same** order, so a script-level binding can reference a step-level one. + The claim is deliberately narrow: only a *self-contained* prefix is + reproduced. Template scope is POSIX, so it cannot read a symbol that carries + the host's path format, and the session table is full of those. When a prefix + binding needs one, ``_apply_template_scope_let_bindings`` declines and the + whole list falls back to a single host-format evaluation -- the previous + behaviour, still wrong on Windows for that script, but never raising and + never silently reading a path under the wrong format. Fixing that case needs + the create-time value carried to the session (``Step.resolved_symtab``) + rather than recomputed here. + ``script`` is the model object the ``let`` list came from. The boundary is read off it as ``_template_scope_let_count`` through ``getattr`` with a default of 0, so an openjd-model that predates the model-side half of this @@ -666,10 +772,15 @@ def apply_script_let_bindings( # which is wrong on Windows but never raises; guessing a prefix could # evaluate a genuinely session-scope binding in the wrong scope. template_scope_count = 0 - if template_scope_count: - _apply_template_scope_let_bindings( - symtab=symtab, let_bindings=let_bindings[:template_scope_count] - ) + if template_scope_count and not _apply_template_scope_let_bindings( + symtab=symtab, let_bindings=let_bindings[:template_scope_count] + ): + # The prefix is not self-contained. Abandon the split for the whole list + # rather than for the one binding that needed a host-format symbol: + # freezing the rest would leave a POSIX-evaluated binding reading a + # host-evaluated sibling, which is the same cross-format read moved + # somewhere less visible. A count of 0 is the pre-fix path exactly. + template_scope_count = 0 host_scope_bindings = let_bindings[template_scope_count:] if host_scope_bindings: apply_let_bindings(symtab=symtab, let_bindings=host_scope_bindings) diff --git a/test/openjd/sessions_v0/test_template_scope_let_split.py b/test/openjd/sessions_v0/test_template_scope_let_split.py index a3432fa1..aefd7cb7 100644 --- a/test/openjd/sessions_v0/test_template_scope_let_split.py +++ b/test/openjd/sessions_v0/test_template_scope_let_split.py @@ -18,6 +18,15 @@ session scope and keep the host's format, because they legitimately reference ``Session.WorkingDirectory``, ``Task.File.*`` and ``apply_path_mapping``. +The claim is narrow, and ``TestPrefixScopeIsNarrowedToFormatNeutralSymbols`` +is where the boundary is drawn. Template scope is POSIX, so the prefix is +evaluated against only the symbols in scope that carry no path format. A prefix +binding that needs one -- a PATH job parameter, ``Session.WorkingDirectory``, a +create-time value seeded natively -- cannot be reproduced here at all, and the +whole list falls back to a single host-format evaluation instead: the previous +behaviour, still wrong on Windows for that script, but never raising and never +reading a path under a format it was not built in. + Note on what these tests can and cannot prove. The path format handed to each half is asserted directly, by spying on the model's ``evaluate_let_bindings``, rather than by comparing rendered values. On a POSIX host the host format *is* @@ -306,6 +315,175 @@ def test_a_failing_suffix_binding_still_raises(self) -> None: ) +# --------------------------------------------------------------------------- +# The narrowed prefix scope, and the all-or-nothing fallback +# --------------------------------------------------------------------------- + + +class TestPrefixScopeIsNarrowedToFormatNeutralSymbols: + """Template scope is POSIX, so the prefix cannot read a session symbol that + carries the host's path format. + + Reading one either raises ``Path format mismatch`` or -- worse -- silently + succeeds against a re-rendered value: ``.parent`` of a Windows path read as + POSIX is ``'.'``, because a backslash is an ordinary POSIX path character. + So those symbols are not in scope for the prefix, and a prefix that needs one + abandons the split for the whole list. + + These assertions are host-independent: the filter removes the symbol on + either host, so the binding fails with ``Undefined variable`` on both. + """ + + @staticmethod + def _session_shaped_symtab() -> SymbolTable: + """A symbol table with one symbol of each shape the session seeds.""" + symtab = SymbolTable() + symtab["Job.Name"] = "a-job" + symtab["Param.S"] = "text" + symtab.expr_types["Param.S"] = "STRING" + symtab["Param.N"] = "3" + symtab.expr_types["Param.N"] = "INT" + symtab["Param.Out"] = "/mnt/out" + symtab.expr_types["Param.Out"] = "PATH" + symtab["Param.Ins"] = ["/mnt/a", "/mnt/b"] + symtab.expr_types["Param.Ins"] = "LIST[PATH]" + symtab["Session.WorkingDirectory"] = "/sessions/s1" + symtab.expr_types["Session.WorkingDirectory"] = "PATH" + return symtab + + @staticmethod + def _unsplit(symtab: SymbolTable, bindings: list[str]) -> SymbolTable: + """The pre-fix behaviour: the whole list, once, in the host's format.""" + before = SymbolTable(source=symtab) + apply_let_bindings(symtab=before, let_bindings=bindings) + return before + + def _assert_fell_back(self, bindings: list[str], count: int) -> None: + """The prefix was attempted in POSIX, declined, and the WHOLE list was + then evaluated once in the host's format -- with the pre-fix values.""" + symtab = self._session_shaped_symtab() + expected = self._unsplit(symtab, bindings) + + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=symtab, let_bindings=bindings, script=_FakeScript(count) + ) + + assert _calls(spy) == [(bindings[:count], _posix_format()), (bindings, None)] + bound = [b.partition("=")[0].strip() for b in bindings] + assert [str(symtab[n]) for n in bound] == [str(expected[n]) for n in bound] + + def test_a_self_contained_prefix_binding_still_freezes(self) -> None: + """The case the fix exists for is untouched: a prefix that reads nothing + format-carrying is still evaluated in template scope.""" + # GIVEN + bindings = [f"tmpl = string({_A_PATH_BINDING})", "own = 1"] + symtab = self._session_shaped_symtab() + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) + + # THEN: still split, and the value froze the POSIX text on either host. + assert _calls(spy) == [([bindings[0]], _posix_format()), (["own = 1"], None)] + assert str(symtab["tmpl"]) == "/foo/bar" + + def test_a_format_neutral_symbol_is_readable_from_the_prefix(self) -> None: + """The filter is a *shape* test, not a name denylist: a STRING or INT + parameter and ``Job.Name`` carry no path format, so they stay in scope + and do not trigger the fallback. + + ``Param.N * 2`` is 6 only if the symbol's declared INT type came across + with it; an untyped ``"3"`` would make it the string ``"33"``.""" + # GIVEN + bindings = ["label = join([Job.Name, Param.S], '-')", "doubled = Param.N * 2"] + symtab = self._session_shaped_symtab() + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(2)) + + # THEN: one POSIX evaluation of the whole prefix, and no fallback call. + assert _calls(spy) == [(bindings, _posix_format())] + assert str(symtab["label"]) == "a-job-text" + assert str(symtab["doubled"]) == "6" + + def test_a_prefix_binding_reading_a_path_parameter_falls_back(self) -> None: + # GIVEN: `Param.Out` is a host-format PATH. + self._assert_fell_back(["out = string(Param.Out)", "own = 1"], count=1) + + def test_a_prefix_binding_reading_a_list_path_parameter_falls_back(self) -> None: + # GIVEN: LIST[PATH] carries a format as much as PATH does. + self._assert_fell_back(["ins = string(Param.Ins[0])", "own = 1"], count=1) + + def test_a_prefix_binding_reading_the_session_working_directory_falls_back(self) -> None: + # GIVEN: the symbol whose `.parent` silently yields '.' on Windows. + self._assert_fell_back(["under = string(Session.WorkingDirectory.parent)"], count=1) + + @pytest.mark.parametrize( + "base_rhs, read", + [ + pytest.param(_A_PATH_BINDING, "base", id="path"), + pytest.param(f"[{_A_PATH_BINDING}, path('/a')]", "base[0]", id="list[path]"), + ], + ) + def test_a_prefix_binding_reading_a_native_path_value_falls_back( + self, base_rhs: str, read: str + ) -> None: + """The other half of the filter. A create-time table reaches the session + as native engine values (``Session._resolved_base_entries``), so a + path-typed one carries its format in the value itself with no + ``expr_types`` entry to declare it. A native ``list[path]`` carries one + just as much, one type parameter down.""" + # GIVEN: `base` in the shape `_resolved_base_entries` produces -- a + # native path value tagged with the host's format. + seed = SymbolTable() + apply_let_bindings(symtab=seed, let_bindings=[f"base = {base_rhs}"]) + symtab = self._session_shaped_symtab() + symtab["base"] = seed["base"] + assert "base" not in symtab.expr_types + bindings = [f"out = string({read})", "own = 1"] + expected = self._unsplit(symtab, bindings) + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) + + # THEN + assert _calls(spy) == [([bindings[0]], _posix_format()), (bindings, None)] + assert str(symtab["out"]) == str(expected["out"]) + + def test_the_fallback_leaves_no_partial_prefix_behind(self) -> None: + """All-or-nothing. A prefix binding that succeeded in POSIX before a + later one declined must not be seeded: it would leave a POSIX-evaluated + value for a host-evaluated sibling to read.""" + # GIVEN: `first` evaluates fine in POSIX; `second` needs a PATH param. + bindings = [f"first = string({_A_PATH_BINDING})", "second = string(Param.Out)"] + symtab = self._session_shaped_symtab() + expected = self._unsplit(symtab, bindings) + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(2)) + + # THEN: both names hold the host-format value, not the POSIX one. + assert _calls(spy) == [(bindings, _posix_format()), (bindings, None)] + assert str(symtab["first"]) == str(expected["first"]) + assert str(symtab["second"]) == str(expected["second"]) + + def test_a_failing_prefix_binding_still_raises_through_the_fallback(self) -> None: + """The fallback must not turn a genuine error into silence. Evaluating + the whole list in the host's format re-raises it -- which is exactly what + the pre-fix code did.""" + # WHEN / THEN + with pytest.raises(ValueError, match="let binding 'bad'"): + apply_script_let_bindings( + symtab=self._session_shaped_symtab(), + let_bindings=["bad = NoSuchSymbol", "own = 1"], + script=_FakeScript(1), + ) + + # --------------------------------------------------------------------------- # The wiring: the runner and the RFC 0008 wrapped-inner-scope path # --------------------------------------------------------------------------- From 3bb92adc93f1835cfb254a581acf8613e820768f Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 22:38:22 -0700 Subject: [PATCH 05/11] test: Make the let-split assertions host-aware instead of POSIX-only Four assertions in this file compared a rendered value against a POSIX literal. Three of them are wrong on Windows, not merely weaker: a binding still holding a path is seeded in the host's format, so `path('/foo/bar')` reads `\foo\bar` there and both texts are correct. The Windows leg was red because of them, and fail-fast cancelled it before it reported, so these assertions had never been judged on Windows at all. The rule they now state: a binding whose result has left path-space freezes the POSIX text it froze at job creation; a binding still holding a path renders in the host's format. `_as_the_host_renders` builds the expectation by evaluating the same expression through the same machinery at the engine default -- which is the host's format -- rather than hardcoding one literal per platform behind a conditional. It reaches the answer by a different route than the code under test, a direct host-format evaluation rather than a POSIX evaluation re-tagged, so it is not asserting the code against itself. Fixed: - `root` and `under` in test_ordering_is_preserved_across_the_boundary. `under` is session scope and reads `root` in host format, so `false` on Windows is correct behaviour, not a defect. - `inner["tmpl"]` in the RFC 0008 wrapped-inner-scope test. - The end-to-end agreement test, where the *fixture* was at fault: `session_time` is re-tagged to the host and `create_time` came from a bare POSIX `apply_let_bindings` with no re-tag, so the two sides sat at different points of the journey a create-time table takes to reach a session. Both sides are now read at the same point, and the test still makes its point that they agree. Verified on both legs, 25 passed each: unpatched, and with the host path format and the engine default both forced to WINDOWS in-process. Three production mutants confirm the corrected assertions pin behaviour -- dropping the re-tag is caught by all three on the Windows leg and by none on POSIX, which is precisely the coverage this commit restores. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../test_template_scope_let_split.py | 69 ++++++++++++++++--- 1 file changed, 60 insertions(+), 9 deletions(-) diff --git a/test/openjd/sessions_v0/test_template_scope_let_split.py b/test/openjd/sessions_v0/test_template_scope_let_split.py index aefd7cb7..e6b419a5 100644 --- a/test/openjd/sessions_v0/test_template_scope_let_split.py +++ b/test/openjd/sessions_v0/test_template_scope_let_split.py @@ -34,8 +34,15 @@ from "POSIX because that is the host" -- it would pass on this host no matter what the code did. ``test_path_format_is_load_bearing`` supplies the missing anchor: it shows a non-POSIX format really does change rendering, so the -argument these tests assert on is the argument that matters. The Windows -behaviour itself is not exercised here. +argument these tests assert on is the argument that matters. + +Where a rendered value *is* compared, the expectation goes through +``_as_the_host_renders`` rather than a POSIX literal. A POSIX literal is not a +weaker assertion, it is a wrong one on Windows: a binding still holding a path +is seeded in the host's format, so ``path('/foo/bar')`` reads ``\\foo\\bar`` +there and both texts are correct. Hardcoding one made the Windows leg red, and a +red leg judges nothing -- fail-fast cancelled it before it reported, so these +assertions had never been run on Windows at all. """ from __future__ import annotations @@ -135,6 +142,28 @@ def _posix_format() -> Any: return PathFormat.POSIX +def _as_the_host_renders(rhs: str) -> str: + """The text ``rhs`` reads as on *this* host. + + A binding whose result has left path-space freezes the POSIX text it froze + at job creation; a binding still holding a path renders in the host's + format. So an assertion about the second kind cannot be a POSIX literal -- + ``path('/foo/bar')`` is ``/foo/bar`` on a POSIX host and ``\\foo\\bar`` on + Windows, and both are correct. + + The expectation is built by evaluating the same expression through the same + machinery at the engine default -- which *is* the host's format, and is what + every session-scope binding gets -- rather than by hardcoding one literal + per platform behind a conditional. It reaches the answer by a different + route than the code under test (a direct host-format evaluation, not a POSIX + evaluation re-tagged to the host), so it is not asserting the code against + itself. + """ + symtab = SymbolTable() + apply_let_bindings(symtab=symtab, let_bindings=[f"value = {rhs}"]) + return str(symtab["value"]) + + def _step_script( let: list[str], command: str = "echo", args: Optional[list[str]] = None ) -> StepScript_2023_09: @@ -300,9 +329,16 @@ def test_ordering_is_preserved_across_the_boundary(self) -> None: # WHEN apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) - # THEN - assert str(symtab["root"]) == "/foo/bar" - assert str(symtab["under"]) == "true" + # THEN: `root` is still a path when it is seeded, so it reads in the + # host's format. `under` is session scope and reads it there, so its + # answer is host-dependent too -- `false` on Windows, where `root` is + # `\foo\bar` and does not start with `/foo`. That is the correct + # behaviour for a session-scope binding, not a defect: the ordering this + # test is about is that `under` sees `root` at all. + assert str(symtab["root"]) == _as_the_host_renders(_A_PATH_BINDING) + assert str(symtab["under"]) == _as_the_host_renders( + f"startswith({_A_PATH_BINDING}, '/foo')" + ) def test_a_failing_suffix_binding_still_raises(self) -> None: """The split must not swallow an evaluation error in either half.""" @@ -590,7 +626,8 @@ def test_wrapped_inner_scope_splits_a_step_scripts_merged_let(self) -> None: ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()), (["own = 1"], None), ] - assert str(inner["tmpl"]) == "/foo/bar" + # `tmpl` is still a path, so it is seeded in the host's format. + assert str(inner["tmpl"]) == _as_the_host_renders(_A_PATH_BINDING) def test_environment_script_bindings_stay_host_format( self, queue_handler: Any, tmp_path: Path, python_exe: str @@ -657,9 +694,23 @@ def test_a_step_level_path_binding_agrees_across_the_two_evaluations( session_time = SymbolTable() apply_script_let_bindings(symtab=session_time, let_bindings=script.let or [], script=script) - # THEN: the two agree. On a POSIX host they would agree either way; the - # per-half format assertions above are what make this host-independent. - assert str(session_time["root"]) == str(create_time["root"]) + # THEN: the two agree, once the create-time side is read at the point in + # its journey that `session_time` occupies. A create-time table does not + # reach a session POSIX-tagged: `Session._resolved_base_entries` + # deserializes it with `to_symtab(path_format=host_format)`, and the + # split applies the same re-tag. So a create-time value still holding a + # path is re-rendered on the way in, while one that left path-space keeps + # its frozen text. + # + # `create_time` here is the raw POSIX evaluation, one step earlier. That + # is the fixture's doing, and comparing the two raw is what made this + # host-dependent -- not the rule. + assert str(create_time["root"]) == "/foo/bar" + assert str(session_time["root"]) == _as_the_host_renders(_A_PATH_BINDING) + # `under` left path-space during the create-time evaluation, so both + # sides hold the same frozen text on every host. Both bindings are + # template scope here (count=2), which is what makes this the whole + # property the fixtures check. assert str(session_time["under"]) == str(create_time["under"]) == "true" def test_a_session_runs_a_step_with_a_step_level_path_binding(self, python_exe: str) -> None: From d5398e571120abb981885837c4c29fa25d245a47 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:12:50 -0700 Subject: [PATCH 06/11] fix: Keep host-context rules out of the template-scope let scope `_apply_template_scope_let_bindings` copied `symtab.expr_host_rules` into the scratch table it evaluates the template-scope prefix against. The copy was inert for every valid binding and the comment defending it was wrong. `apply_path_mapping` is the only host-context function, and RFC 0005 bars those from template scope: openjd-model invokes the create-time hook with no host context, so a template-scope binding calling one raises `Unknown function: 'apply_path_mapping'` at job creation and never had a create-time value to reproduce. Measured separately, `expr_host_rules` of `None` versus `[]` render `path().parent`, arithmetic and `join` identically, so the copy changed nothing for a conforming template. For a template that does violate RFC 0005 the copy was actively harmful: it let the binding resolve here against the *host's* rules while evaluating in POSIX, freezing a mixed-separator value such as `C:\Users\test/bar` instead of declining. Dropping the copy makes such a binding raise `Unknown function`, which the existing `except ValueError` turns into the whole-list host-format fallback -- the documented behaviour for a prefix that cannot be reproduced in template scope, and the pre-fix value. Two tests. The first pins the fallback (not the raise, which is internal) for a prefix binding calling `apply_path_mapping`. The second guards the derived-table shape the runners actually pass, `SymbolTable(source=...)`, and pins openjd-model's `_expr_types` copy in `SymbolTable.__init__`: if that is ever dropped, `declared_types` goes empty and the PATH filter silently stops filtering. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_runner_base.py | 10 ++- .../test_template_scope_let_split.py | 81 +++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/openjd/sessions/_runner_base.py b/src/openjd/sessions/_runner_base.py index 99b26e69..e1c4dc71 100644 --- a/src/openjd/sessions/_runner_base.py +++ b/src/openjd/sessions/_runner_base.py @@ -682,10 +682,12 @@ def _apply_template_scope_let_bindings(*, symtab: SymbolTable, let_bindings: lis # reads an earlier one. declared_types = getattr(symtab, "expr_types", None) or {} scratch = SymbolTable() - # Host-context rules ride along unchanged: a prefix binding that resolved - # through `apply_path_mapping` before this narrowing still resolves. - if symtab.expr_host_rules is not None: - scratch.expr_host_rules = list(symtab.expr_host_rules) + # Host-context rules are deliberately NOT carried over. `apply_path_mapping` + # is a host-context function and RFC 0005 bars those from template scope: + # the create-time hook runs with no host context, so a binding calling one + # raises `Unknown function` there and has no create-time value to reproduce. + # Without the rules this evaluation raises the same way, and the + # `except ValueError` below turns that into the whole-list fallback. for name in symtab.symbols: if _is_format_neutral(symtab[name], declared_types.get(name)): scratch[name] = symtab[name] diff --git a/test/openjd/sessions_v0/test_template_scope_let_split.py b/test/openjd/sessions_v0/test_template_scope_let_split.py index e6b419a5..aaa249e8 100644 --- a/test/openjd/sessions_v0/test_template_scope_let_split.py +++ b/test/openjd/sessions_v0/test_template_scope_let_split.py @@ -507,6 +507,87 @@ def test_the_fallback_leaves_no_partial_prefix_behind(self) -> None: assert str(symtab["first"]) == str(expected["first"]) assert str(symtab["second"]) == str(expected["second"]) + def test_a_prefix_binding_calling_apply_path_mapping_falls_back(self) -> None: + """``apply_path_mapping`` is a *host-context* function, and RFC 0005 bars + those from template scope: openjd-model invokes the create-time hook with + no host context, so a template-scope binding calling one raises + ``Unknown function: 'apply_path_mapping'`` at job creation and never had + a create-time value to reproduce. So this evaluation must not resolve it + either -- it must decline and let the whole list fall back. + + The assertion is the fallback, not the raise: the raise is internal to + the helper and is swallowed by design. + + An earlier revision copied ``symtab.expr_host_rules`` into the scratch + table, which made such a binding resolve here against the *host's* rules + while evaluating in POSIX -- freezing a mixed-separator value like + ``C:\\Users\\test/bar`` instead of falling back to the host-format value. + """ + # GIVEN: a session table in the shape `Session._resolved_base_entries` + # leaves it -- host rules attached (`_session.py` seeds `[]` even with no + # rules, so `apply_path_mapping` stays available in session scope). + from openjd.expr import PathFormat, PathMappingRule + + bindings = ["mapped = string(apply_path_mapping(path('/foo/bar')))", "own = 1"] + rule = PathMappingRule( + source_path_format=PathFormat.POSIX, + source_path="/foo", + destination_path="C:\\Users\\test", + ) + symtab = self._session_shaped_symtab() + symtab.expr_host_rules = [rule] + # `_unsplit` derives its table with `SymbolTable(source=...)`, which + # carries the host rules across, so the expectation has them too. + expected = self._unsplit(symtab, bindings) + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) + + # THEN: the POSIX prefix was attempted, declined, and the WHOLE list was + # re-evaluated in the host's format. Without the fallback the second call + # would be `["own = 1"]` alone, with `mapped` holding the POSIX-evaluated + # value. + assert _calls(spy) == [([bindings[0]], _posix_format()), (bindings, None)] + assert str(symtab["mapped"]) == str(expected["mapped"]) + + def test_the_filter_survives_the_derived_table_production_hands_it(self) -> None: + """The narrowing must still work on the table shape the runners actually + pass, which is not the session table itself but a *derived* one: + ``SymbolTable(source=self._symtab)`` at ``_runner_step_script.py:110`` + and ``:122``, and ``SymbolTable(source=base)`` at ``_session.py:2032``. + + Why this is worth its own test, given the unwrapped cases above already + pass. The filter identifies a plain-string PATH symbol purely from its + ``expr_types`` entry, and that entry only survives the wrap because + openjd-model's ``SymbolTable.__init__`` copies it -- + ``self._expr_types.update(source._expr_types)`` in + ``openjd/model/_symbol_table.py`` (line 131 at the version this pins + against). Nothing in openjd-sessions re-declares those types. If a future + openjd-model refactor drops that one line, ``declared_types`` here goes + empty, every plain-string PATH symbol reads as format-neutral, and the + filter silently stops filtering -- the split would then read + ``Session.WorkingDirectory`` under POSIX on a Windows host, which is the + exact defect the filter exists to prevent. No other test in this file + would notice, because they all pass the undertived table. + """ + # GIVEN: the session-shaped table, wrapped exactly as production wraps it. + base = self._session_shaped_symtab() + derived = SymbolTable(source=base) + # The wrap is what is under test, so state the precondition it depends on. + assert derived.expr_types.get("Session.WorkingDirectory") == "PATH" + bindings = ["under = string(Session.WorkingDirectory.parent)", "own = 1"] + expected = self._unsplit(derived, bindings) + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings(symtab=derived, let_bindings=bindings, script=_FakeScript(1)) + + # THEN: the PATH symbol was still filtered out, so the prefix declined + # and the whole list fell back to the host's format. + assert _calls(spy) == [([bindings[0]], _posix_format()), (bindings, None)] + assert str(derived["under"]) == str(expected["under"]) + def test_a_failing_prefix_binding_still_raises_through_the_fallback(self) -> None: """The fallback must not turn a genuine error into silence. Evaluating the whole list in the host's format re-raises it -- which is exactly what From 5c3c8c95bf3133b280d8aa6b37b71754dde71e34 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:34:11 -0700 Subject: [PATCH 07/11] fix: Read a step's resolved let instead of re-deriving it openjd-model no longer merges a step's template-scope `let` into `script.let`. The step's bindings are resolved once at job creation and travel in the step symbol table, which reaches a session through the existing `resolved_symtab` parameter and `_resolved_base_entries`, so `script.let` now holds only the script's own bindings. Those are genuinely session scope and correctly evaluate in the host's format, which is what they did before this branch. That makes the prefix-splitting apparatus redundant, and worse than redundant: with both mechanisms active the session-side re-evaluation wrote last and clobbered the correctly-formatted seeded value. Remove `_apply_template_scope_let_bindings`, `_is_format_neutral`, `_type_carries_path_format`, `_host_path_format`, every read of `_template_scope_let_count`, the caller logic that split the list, the all-or-nothing fallback, and the private `openjd.model._format_strings._expr_support` import that existed only for this feature. `apply_script_let_bindings` is now a single host-format evaluation of the whole list. This also drops the last use of the `path_format` keyword argument to `evaluate_let_bindings`, which does not exist on the released openjd-model 0.11.6 that CI resolves from PyPI. `mypy src test` against 0.11.6 is now clean, so this branch no longer waits on the openjd-model change releasing. Replace the 27 tests of the deleted design, plus the import-purity positive control that pinned its guarded `PathFormat` import, with four tests of the behaviour that matters: a seeded create-time path binding survives a script's own `let`, a step-level binding is never evaluated at session time, and a script's own `let` still evaluates in the host's format and still sees `Session.WorkingDirectory`. The Windows simulation patches both seams that choose a path format, because patching only the host-format derivation leaves the engine default at POSIX and yields a host that cannot exist. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_runner_base.py | 298 +------ src/openjd/sessions/_runner_step_script.py | 6 +- src/openjd/sessions/_session.py | 14 +- .../sessions_v0/test_let_binding_scopes.py | 275 ++++++ .../test_template_scope_let_split.py | 827 ------------------ test/openjd/test_import_purity.py | 47 +- 6 files changed, 338 insertions(+), 1129 deletions(-) create mode 100644 test/openjd/sessions_v0/test_let_binding_scopes.py delete mode 100644 test/openjd/sessions_v0/test_template_scope_let_split.py diff --git a/src/openjd/sessions/_runner_base.py b/src/openjd/sessions/_runner_base.py index e1c4dc71..f7f501a2 100644 --- a/src/openjd/sessions/_runner_base.py +++ b/src/openjd/sessions/_runner_base.py @@ -166,14 +166,12 @@ class _ExprKind(Enum): def _classify_expr_value(value: Any) -> _ExprKind: """Classify ``value`` against the EXPR type system. - This is one of two places in this module that import ``openjd.expr``, and - it is the one reached from the non-EXPR path, so it sits behind the + This is the *only* place in this module that imports ``openjd.expr``, so + that every crossing into the native extension sits behind the one ``sys.modules`` guard below. Doing the whole classification here rather than exposing a separate "is it a list" predicate keeps that property structural instead of merely documented: there is no second, unguarded - entry point for a future caller to reach with an arbitrary value. The other - crossing is in :func:`_apply_template_scope_let_bindings`, guarded instead - by a non-zero template-scope prefix, which only an EXPR template produces. + entry point for a future caller to reach with an arbitrary value. ``ExprValue`` instances are created only by the native extension, so if that extension has not been loaded then ``value`` cannot be one and the @@ -497,9 +495,7 @@ def resolve_period(period: Any) -> Optional[int]: ) -def apply_let_bindings( - *, symtab: SymbolTable, let_bindings: list[str], path_format: Any = None -) -> None: +def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: """Evaluate EXPR ``let`` bindings (RFC 0005) and add them to ``symtab``. ``let_bindings`` is a script's ``let`` field: an ordered list of @@ -515,11 +511,12 @@ def apply_let_bindings( ``Env.File.*``/``Task.File.*`` and a file's ``data`` may reference let-bound values (mirroring openjd-rs's runner ordering). - ``path_format`` is the EXPR ``PathFormat`` that PATH-typed results render - with. ``None`` -- the default, and what every session-scope binding wants -- - leaves the engine's default, which is the host's format. Callers - re-evaluating a *template*-scope binding pass ``PathFormat.POSIX``; see - :func:`apply_script_let_bindings`. + PATH-typed results render in the engine's default format, which is the + host's. That is the only format a session ever evaluates in: a step's + template-scope ``let`` is resolved once at job creation and its values reach + the session already resolved, through ``Step.resolved_symtab`` (see + :meth:`Session._resolved_base_entries`), so nothing here re-evaluates a + binding that belongs to another scope. Raises: ValueError (FormatStringError/ExpressionError): if a binding's @@ -538,254 +535,45 @@ def apply_let_bindings( f"which exceeds the maximum of {MAX_LET_BINDING_LENGTH}" ) # Single-sourced in openjd.model (parse-memoized; skips malformed - # bindings; raises ValueError naming the failing binding). - # - # The kwarg is forwarded only when it is set, because `path_format` does not - # exist on openjd-model at this package's declared floor (>= 0.11.6) and - # passing it there is a TypeError, not a no-op -- which would break EVERY - # EXPR template rather than degrading. On such a model the else branch is - # unreachable from any internal caller: apply_script_let_bindings reads - # the template-scope count through getattr, and a model without - # `path_format` has no `_template_scope_let_count` either, so the count is 0 - # and nothing asks for a non-default format. Tests call it directly. On a model that does have it, `None` and "omitted" - # are the same call. Collapse this to an unconditional forward once the - # openjd-model floor carries the parameter. - if path_format is None: - evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings) - else: - evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings, path_format=path_format) - - -def _host_path_format() -> Any: - """The EXPR ``PathFormat`` for this host. - - The Python engine bindings expose no ``PathFormat.host()``, so it is derived - the same way :meth:`Session._resolved_base_entries` derives it. - """ - import os - - from openjd.expr import PathFormat - - return PathFormat.WINDOWS if os.name == "nt" else PathFormat.POSIX - - -def _type_carries_path_format(expr_type: Any) -> bool: - """Whether an EXPR type renders a path, and so carries a path format. - - ``list[path]`` carries one as much as ``path`` does, so the type parameters - are walked rather than only the outer type code. - """ - from openjd.expr import TypeCode - - if expr_type.type_code == TypeCode.PATH: - return True - return any(_type_carries_path_format(param) for param in expr_type.type_params) - - -def _is_format_neutral(value: Any, declared_type: Optional[str]) -> bool: - """Whether a symbol can be read under a path format other than the one it - was built in. - - Tested by *shape*, not by name, because the set of session symbols grows and - a name denylist would rot. Two shapes carry a path format, and the session - symbol table holds both: - - - a native engine value that is itself path-typed, which is what - :meth:`Session._resolved_base_entries` seeds (a create-time table - deserialized in host format); and - - a plain string (or list of strings) whose ``expr_types`` entry declares it - ``PATH`` or ``LIST[PATH]``, which is what ``Session.WorkingDirectory`` and - every path-typed ``Param.*``/``Task.Param.*`` are. - - ``"PATH" in declared_type`` covers both ``PATH`` and ``LIST[PATH]``; no other - OpenJD parameter type name contains it. - - Fails closed: a value whose type cannot be determined is treated as carrying - a format, because wrongly *including* a path-typed symbol is the defect this - filter exists to prevent, while wrongly excluding one only triggers the - fallback in :func:`_apply_template_scope_let_bindings`. - """ - if declared_type is not None and "PATH" in declared_type: - return False - - from openjd.expr import ExprValue - - if not isinstance(value, ExprValue): - # A plain Python value carries no format of its own; its `expr_types` - # entry, checked above, is the only thing that could give it one. - return True - try: - return not _type_carries_path_format(value.type) - except Exception: - return False - - -def _apply_template_scope_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> bool: - """Evaluate self-contained template-scope ``let`` bindings and seed them in - host format. Returns whether the bindings could be evaluated this way. - - Three parts, and all three are load-bearing. - - **The bindings are evaluated with ``PathFormat.POSIX``**, because that is - what openjd-model and openjd-rs use at job creation, so a value that leaves - path-space during evaluation freezes the same text it froze there. That - covers ``string(path(...))``, ``join(...)``, ``repr_sh(...)``, ``.parts`` - and every comparison against a POSIX literal. - - **They are evaluated against only the format-neutral symbols in scope** - (:func:`_is_format_neutral`), plus the prefix bindings already bound. A - POSIX evaluation cannot read a symbol that carries a path format: the - session table holds host-format values, and on a Windows host reading one - either raises:: - - ValueError: let binding 'out': Path format mismatch for - 'Session.WorkingDirectory': value has Windows but - evaluator uses Posix - - or, worse, silently succeeds against a re-rendered value -- a ``.parent`` of - a Windows path read as POSIX is ``'.'``, because a backslash is an ordinary - character in a POSIX path. Neither is acceptable, so those symbols are not - in scope for this evaluation at all. - - **The results are then re-tagged to the host's format** before they are - seeded. An EXPR path value carries its format, and reading one under a - different format is the same hard error in the other direction. Action - arguments, embedded-file ``data`` and environment-variable values all - resolve in the host's format, so seeding a Posix-tagged path would make - every one of those reads raise on Windows. The re-tag round trip leaves a - frozen string alone and re-renders a live path, which is exactly the split - the conformance suite asks for: ``expr2.2.1--string-conversion`` wants - ``/mnt/out`` on both platforms, while ``expr2.3.2--path-construction`` wants - ``\\a\\b`` on Windows. This mirrors how a create-time table already reaches - a session: :meth:`Session._resolved_base_entries` deserializes it with - ``to_symtab(path_format=host_format)``. - - Returns: - bool: ``True`` when the prefix was evaluated in template scope and - seeded. ``False`` when it could not be -- because a binding reads a - symbol that carries a path format, and so is missing from the - narrowed scope, or the POSIX evaluation failed for any other reason. - The caller then abandons the split for the whole ``let`` list. This - is all-or-nothing on purpose: freezing per binding would let a - POSIX-evaluated binding read a host-evaluated sibling, which is the - same cross-format read in a new place. Nothing has been written to - ``symtab`` when ``False`` is returned, so the caller's fallback - starts from an untouched table. - """ - from openjd.expr import PathFormat, SerializedSymbolTable - - from openjd.model._format_strings._expr_support import symtab_to_expr_values - - # A separate table, so the POSIX evaluation neither writes host-format - # neighbours back into `symtab` nor reads a symbol whose format it cannot - # honour. Bindings land in it in order, so a later prefix binding still - # reads an earlier one. - declared_types = getattr(symtab, "expr_types", None) or {} - scratch = SymbolTable() - # Host-context rules are deliberately NOT carried over. `apply_path_mapping` - # is a host-context function and RFC 0005 bars those from template scope: - # the create-time hook runs with no host context, so a binding calling one - # raises `Unknown function` there and has no create-time value to reproduce. - # Without the rules this evaluation raises the same way, and the - # `except ValueError` below turns that into the whole-list fallback. - for name in symtab.symbols: - if _is_format_neutral(symtab[name], declared_types.get(name)): - scratch[name] = symtab[name] - if name in declared_types: - scratch.expr_types[name] = declared_types[name] - - try: - apply_let_bindings(symtab=scratch, let_bindings=let_bindings, path_format=PathFormat.POSIX) - except ValueError: - # Most often an `Undefined variable` for a symbol the filter removed. - # The catch is deliberately not narrowed to that: every other failure - # mode is also one where this prefix cannot be reproduced in template - # scope, and the caller's fallback -- evaluating the whole list in the - # host's format -- is precisely the pre-fix behaviour, which re-raises - # a genuine error with the same message rather than swallowing it. - return False - - # Only the names these bindings defined. A malformed binding is skipped by - # the evaluator, so membership is checked rather than assumed. - holder = SymbolTable() - for binding in let_bindings: - name = binding.partition("=")[0].strip() - if name and name in scratch: - holder[name] = scratch[name] - if not holder.symbols: - return True - - engine = symtab_to_expr_values( - holder, types=getattr(holder, "expr_types", None), path_format=PathFormat.POSIX - ) - retagged = SerializedSymbolTable.from_symtab(engine).to_symtab(path_format=_host_path_format()) - for name in retagged.symbols: - symtab[name] = retagged[name] - return True + # bindings; raises ValueError naming the failing binding). No `path_format` + # kwarg: the engine default is the host's format, which is the only format a + # session evaluates in, and the parameter does not exist on openjd-model at + # this package's declared floor (>= 0.11.6). + evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings) def apply_script_let_bindings( *, symtab: SymbolTable, let_bindings: list[str], script: Any = None ) -> None: - """Evaluate a script's ``let`` list into ``symtab``, honouring the - template-scope / session-scope boundary inside it. - - An instantiated Step's script carries a *merged* ``let`` list: the - step-level bindings the template declared, followed by the script's own - (openjd-model's ``StepTemplate.resolve_syntax_sugar``). The step-level - prefix is template scope and was already evaluated as such at job creation. - Re-evaluating it here in the host's format changes its value: on Windows + """Evaluate a script's own ``let`` list into ``symtab`` in the host's path + format. + + Every binding in ``let_bindings`` is session scope, so there is one scope + here and one format. A step's *template*-scope ``let`` does not appear in + this list: openjd-model resolves it once at job creation and its values + travel to the session in the step symbol table, reaching ``symtab`` through + ``Step.resolved_symtab`` (:meth:`Session._resolved_base_entries`) already + resolved and deserialized into the host's format. + + That division matters because the two are not interchangeable. A + template-scope value is frozen at creation with ``PathFormat::Posix`` so it + cannot depend on the host that created the job, and re-deriving one here + would re-render its PATH values -- on Windows ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` to ``false``. + Both a seeded value and a re-evaluated one would land in this same table, so + a re-evaluation would also *win*, overwriting the correctly-formatted seeded + value. Nothing in a session re-evaluates a step's bindings; it reads the + resolved ones. - So the list is split. The prefix goes through - :func:`_apply_template_scope_let_bindings`, which reproduces the - create-time value and then seeds it in host format. The remainder is the - script's own bindings, evaluated unchanged in the host's format, because - they legitimately reference host-scope symbols - (``Session.WorkingDirectory``, ``Task.File.*``, ``apply_path_mapping``). - - Both halves land in the **same** table in the **same** order, so a - script-level binding can reference a step-level one. - - The claim is deliberately narrow: only a *self-contained* prefix is - reproduced. Template scope is POSIX, so it cannot read a symbol that carries - the host's path format, and the session table is full of those. When a prefix - binding needs one, ``_apply_template_scope_let_bindings`` declines and the - whole list falls back to a single host-format evaluation -- the previous - behaviour, still wrong on Windows for that script, but never raising and - never silently reading a path under the wrong format. Fixing that case needs - the create-time value carried to the session (``Step.resolved_symtab``) - rather than recomputed here. - - ``script`` is the model object the ``let`` list came from. The boundary is - read off it as ``_template_scope_let_count`` through ``getattr`` with a - default of 0, so an openjd-model that predates the model-side half of this - fix degrades to the previous behaviour rather than raising. ``None`` -- - what an environment script's caller passes -- means the same thing: an - environment script's own bindings are session scope. + ``script`` is the model object the ``let`` list came from. It is accepted so + the runners and ``Session._build_wrapped_inner_scope`` can pass the script + they already have, but nothing is read off it: a script's own ``let`` needs + no per-script scope information. Raises: ValueError: as :func:`apply_let_bindings`. """ - template_scope_count = getattr(script, "_template_scope_let_count", 0) - if not 0 <= template_scope_count <= len(let_bindings): - # A model/sessions version skew reported a boundary this list cannot - # have. Treating everything as session scope is the previous behaviour, - # which is wrong on Windows but never raises; guessing a prefix could - # evaluate a genuinely session-scope binding in the wrong scope. - template_scope_count = 0 - if template_scope_count and not _apply_template_scope_let_bindings( - symtab=symtab, let_bindings=let_bindings[:template_scope_count] - ): - # The prefix is not self-contained. Abandon the split for the whole list - # rather than for the one binding that needed a host-format symbol: - # freezing the rest would leave a POSIX-evaluated binding reading a - # host-evaluated sibling, which is the same cross-format read moved - # somewhere less visible. A count of 0 is the pre-fix path exactly. - template_scope_count = 0 - host_scope_bindings = let_bindings[template_scope_count:] - if host_scope_bindings: - apply_let_bindings(symtab=symtab, let_bindings=host_scope_bindings) + apply_let_bindings(symtab=symtab, let_bindings=let_bindings) class ScriptRunnerBase(ABC): @@ -1283,10 +1071,8 @@ def _materialize_files( materializing embedded files to disk. ``script`` is the model object ``let_bindings`` came from, forwarded to - :func:`apply_script_let_bindings` so a step script's merged list is - split at its template-scope boundary. Omitting it evaluates every - binding in the host's path format, which is what an environment script - wants. + :func:`apply_script_let_bindings`. Every binding is evaluated in the + host's path format. When ``let_bindings`` is given, they are evaluated between file-path allocation and content writing (RFC 0005, mirroring the openjd-rs @@ -1340,8 +1126,8 @@ def _apply_let_bindings_or_fail( evaluation error the action is failed through the normal failure path (openjd_fail log, FAILED state, callback). Returns True on success. - ``script`` is the model object the bindings came from; see - :func:`apply_script_let_bindings` for what it is read for.""" + ``script`` is the model object the bindings came from, forwarded to + :func:`apply_script_let_bindings`.""" try: apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) except ValueError as exc: diff --git a/src/openjd/sessions/_runner_step_script.py b/src/openjd/sessions/_runner_step_script.py index d6fd5426..6193bb83 100644 --- a/src/openjd/sessions/_runner_step_script.py +++ b/src/openjd/sessions/_runner_step_script.py @@ -103,9 +103,9 @@ def run(self) -> None: # Task.File.*), and contents are written after (so `data` can # reference let-bound values) — mirroring the openjd-rs runner. # - # `script=self._script` is what tells the evaluation where this merged - # `let` list stops being template scope and starts being session scope; - # see apply_script_let_bindings. + # This `let` list is the script's own, and is entirely session scope. A + # step's template-scope `let` is resolved at job creation and arrives + # through `Step.resolved_symtab` instead; see apply_script_let_bindings. if self._script.embeddedFiles is not None: symtab = SymbolTable(source=self._symtab) self._materialize_files( diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 05fccb9c..89df9ec4 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -2014,15 +2014,11 @@ def _build_wrapped_inner_scope( resolution scope. Mirrors openjd-rs's ``build_wrapped_inner_scope``. ``script`` is the inner entity's script -- the model object - ``let_bindings`` came from -- forwarded so that a wrapped *step* script's - merged ``let`` list is split at its template-scope boundary exactly as - the step runner splits it (see - :func:`~._runner_base.apply_script_let_bindings`). Without it a wrapped - action would resolve against template-scope values re-rendered in the - host's path format, i.e. against a scope that differs from the one it - would have had unwrapped -- which is the whole property this method - exists to reproduce. An inner *environment* script has no such prefix and - is unaffected. + ``let_bindings`` came from -- forwarded to + :func:`~._runner_base.apply_script_let_bindings` exactly as the step + runner forwards it. A script's own ``let`` is session scope either way, + so a wrapped action resolves against the same scope it would have had + unwrapped, which is the property this method exists to reproduce. Raises: ValueError (FormatStringError/ExpressionError): a binding or file diff --git a/test/openjd/sessions_v0/test_let_binding_scopes.py b/test/openjd/sessions_v0/test_let_binding_scopes.py new file mode 100644 index 00000000..2c5500e1 --- /dev/null +++ b/test/openjd/sessions_v0/test_let_binding_scopes.py @@ -0,0 +1,275 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""A session evaluates exactly one scope of ``let`` bindings: the script's own. + +A step's *template*-scope ``let`` is resolved once at job creation, with +``PathFormat::Posix`` so a create-time value cannot depend on the host that +created the job. Those resolved values reach a session in the step symbol table +(``Step.resolved_symtab``) and are seeded by +:meth:`Session._resolved_base_entries`, deserialized into the host's format. A +script's own ``let`` is session scope and is evaluated here, in the host's +format, against the live session symbols. + +The two must not be confused, and the failure mode is asymmetric. Both a seeded +value and a session-time re-evaluation land in the *same* symbol table, so when +both happen the re-evaluation writes **last** and clobbers the correctly +formatted seeded value. That overwrite is the bug these tests exist to prevent: +:class:`TestSeededStepValuesAreNotReEvaluated` pins the seeded value surviving, +and it is what fails if anyone reintroduces session-side re-evaluation of a +step's bindings. + +On simulating a Windows host. A POSIX host renders both scopes identically, so a +value comparison here proves nothing about format on this machine -- it would +pass whatever the code did. ``_windows_host`` forces the other format, and it +patches **both** seams that choose one: + +- ``openjd.sessions._session.os.name``, which + :meth:`Session._resolved_base_entries` reads to pick the format it + deserializes a create-time table with; and +- ``ExprNode._evaluate_raw``'s ``path_format=None`` default, which is the engine + default and is POSIX on this host. + +Patching only the first is not a Windows host, it is a self-inconsistent one: +seeded values would render Windows while a script's own ``let`` still rendered +POSIX, and a test built on that would be asserting an arrangement that cannot +occur in production. +""" + +from __future__ import annotations + +import json +import uuid +from contextlib import contextmanager +from typing import Any, Generator, Optional +from unittest.mock import patch as mock_patch + +import pytest + +from openjd.expr import PathFormat, SerializedSymbolTable +from openjd.model import SpecificationRevision, SymbolTable, evaluate_let_bindings +from openjd.model._format_strings._nodes import ExprNode +from openjd.model.v2023_09 import ( + ModelParsingContext as ModelParsingContext_2023_09, + StepScript as StepScript_2023_09, +) +from openjd.sessions import Session +from openjd.sessions._runner_base import apply_script_let_bindings + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_SEEDED_NAME = "step_out" +"""The name a step-level (template-scope) ``let`` binding resolved to at job +creation, arriving in the session's ``resolved_symtab``.""" + +_SEEDED_POSIX_TEXT = "/foo/bar" +"""The create-time value, as the service serialized it.""" + +_SEEDED_WINDOWS_TEXT = r"\foo\bar" +"""The same value once deserialized in a Windows host's format, which is how a +session must read it. Distinct from ``_SEEDED_POSIX_TEXT``, which is what a +session would show if the host-format deserialization were skipped.""" + + +@contextmanager +def _windows_host() -> Generator[None, None, None]: + """Force a Windows path format at both seams that decide one. + + See this module's docstring for why one seam is not enough. + """ + original = ExprNode._evaluate_raw + + def _evaluate_raw_windows( + self: ExprNode, *, symtab: SymbolTable, path_format: Any = None + ) -> Any: + # Substitute only the *default*. An explicit format from a caller is + # left alone, so this stands in for the engine default rather than + # overriding evaluation everywhere. + if path_format is None: + path_format = PathFormat.WINDOWS + return original(self, symtab=symtab, path_format=path_format) + + with mock_patch("openjd.sessions._session.os.name", "nt"): + with mock_patch.object(ExprNode, "_evaluate_raw", _evaluate_raw_windows): + yield + + +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``.""" + return SerializedSymbolTable.from_json_str(json.dumps(entries)) + + +def _seeded_step_table() -> SerializedSymbolTable: + """A create-time table carrying one path-valued step-level ``let`` result.""" + return _serialized_table([{"name": _SEEDED_NAME, "type": "path", "value": _SEEDED_POSIX_TEXT}]) + + +def _expr_step_script(let: list[str]) -> StepScript_2023_09: + """A step script whose ``let`` is its own -- the only thing a script's ``let`` + field carries now that openjd-model no longer merges a step's bindings into + it.""" + context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) + return StepScript_2023_09.model_validate( + {"let": let, "actions": {"onRun": {"command": "echo", "args": ["ok"]}}}, + context=context, + ) + + +def _spy_on_evaluation() -> Any: + """Patch the model's ``evaluate_let_bindings`` where openjd-sessions imports + it, recording every call while still evaluating for real. + + Spying here rather than on ``apply_let_bindings`` keeps the real evaluation + in the loop, so a test can assert both the calls and the resulting values. + """ + return mock_patch( + "openjd.sessions._runner_base.evaluate_let_bindings", + side_effect=evaluate_let_bindings, + ) + + +def _evaluated_bindings(spy: Any) -> list[str]: + """Every binding string handed to the evaluator, flattened across calls.""" + return [b for call in spy.call_args_list for b in call.kwargs["let_bindings"]] + + +def _session_symtab( + session: Session, + *, + resolved_symtab: Optional[SerializedSymbolTable] = None, +) -> SymbolTable: + """The session-scope symbol table a script would be resolved against. + + Built through the session's own ``_resolved_base_entries`` / + ``_symbol_table`` rather than end to end through ``run_task``, because + ``_windows_host`` patches the process-wide ``os.name`` and running a real + subprocess under that would exercise Windows user and path handling on a + POSIX host -- unrelated machinery, and not what these tests are about. + """ + resolved_base = ( + session._resolved_base_entries(resolved_symtab) if resolved_symtab is not None else None + ) + return session._symbol_table( + SpecificationRevision.v2023_09, + resolved_base=resolved_base, + ) + + +# --------------------------------------------------------------------------- +# The regression test for the overwrite bug. +# --------------------------------------------------------------------------- + + +class TestSeededStepValuesAreNotReEvaluated: + """A create-time value seeded from ``resolved_symtab`` must survive a script + that has its own ``let``. This is the test that fails if session-side + re-evaluation of a step's bindings is reintroduced.""" + + def test_a_seeded_path_binding_survives_a_scripts_own_let(self) -> None: + # GIVEN: a Windows host, a create-time table carrying a path-valued + # step-level binding, and a script with a `let` of its own. + script = _expr_step_script(["mine = 1 + 1"]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + with _windows_host(): + symtab = _session_symtab(session, resolved_symtab=_seeded_step_table()) + # The seeded value is in host format before the script's `let` + # runs; the assertion after is that it is still there. + assert str(symtab[_SEEDED_NAME]) == _SEEDED_WINDOWS_TEXT + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=symtab, let_bindings=script.let or [], script=script + ) + + # THEN: the seeded value is untouched, in the host's format. + assert str(symtab[_SEEDED_NAME]) == _SEEDED_WINDOWS_TEXT, ( + "the seeded create-time value was overwritten. A session must " + "read a step's resolved bindings, never re-derive them: a " + "re-evaluation lands in this same table and so wins." + ) + # AND: the script's own binding did land. + assert symtab["mine"].item() == 2 + # AND: nothing re-evaluated the seeded name. This is the half of + # the assertion that a value comparison cannot make -- on a + # faithful Windows host a re-evaluation of the same expression + # would render the same text, so only the absence of the call + # distinguishes "seeded" from "recomputed". + assert _evaluated_bindings(spy) == ["mine = 1 + 1"] + + def test_a_step_level_binding_is_not_evaluated_at_session_time(self) -> None: + # GIVEN: a create-time table whose step-level binding is *also* named in + # nothing the script declares -- the shape openjd-model now produces, + # where `script.let` holds only the script's own bindings. + script = _expr_step_script(["mine = 'x'"]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + symtab = _session_symtab(session, resolved_symtab=_seeded_step_table()) + + # WHEN + with _spy_on_evaluation() as spy: + apply_script_let_bindings( + symtab=symtab, let_bindings=script.let or [], script=script + ) + + # THEN: the evaluator saw the script's own bindings and nothing else. + evaluated = _evaluated_bindings(spy) + assert evaluated == ["mine = 'x'"] + assert not any(b.split("=")[0].strip() == _SEEDED_NAME for b in evaluated), ( + f"a step-level binding ({_SEEDED_NAME}) was evaluated at session " + "time; it is resolved at job creation and only read here" + ) + + +# --------------------------------------------------------------------------- +# A script's own `let` is session scope: host format, live session symbols. +# --------------------------------------------------------------------------- + + +class TestAScriptsOwnLetIsSessionScope: + def test_it_evaluates_in_the_host_format_and_sees_session_symbols(self) -> None: + # GIVEN: a Windows host and a script whose own `let` both builds a path + # (so the format is observable) and reads a session symbol (so the + # session scope is observable). + script = _expr_step_script( + [ + "built = path('/a/b')", + "wd = Session.WorkingDirectory", + ] + ) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + with _windows_host(): + symtab = _session_symtab(session) + + # WHEN + apply_script_let_bindings( + symtab=symtab, let_bindings=script.let or [], script=script + ) + + # THEN: the path rendered in the *host's* format, not POSIX. + assert str(symtab["built"]) == r"\a\b", ( + "a script's own `let` is session scope and must render in the " + "host's path format" + ) + # AND: it resolved against the live session symbol table. + # `Session.WorkingDirectory` is PATH-typed, so on the simulated + # Windows host it renders with backslashes too. The claim here is + # *which* path the binding saw, not how it renders, so the + # separator is normalised before comparing; the format claim is + # the `built` assertion above. + rendered_wd = str(symtab["wd"]).replace("\\", "/") + assert rendered_wd == str(session.working_directory) + + def test_a_failing_binding_still_raises(self) -> None: + """Negative control for the two tests above: the evaluation is real, so a + broken binding is still an error rather than being silently skipped.""" + script = _expr_step_script(["bad = Undefined.Symbol"]) + with Session(session_id=uuid.uuid4().hex, job_parameter_values={}) as session: + symtab = _session_symtab(session) + + # WHEN / THEN + with pytest.raises(ValueError, match="bad"): + apply_script_let_bindings( + symtab=symtab, let_bindings=script.let or [], script=script + ) diff --git a/test/openjd/sessions_v0/test_template_scope_let_split.py b/test/openjd/sessions_v0/test_template_scope_let_split.py deleted file mode 100644 index aaa249e8..00000000 --- a/test/openjd/sessions_v0/test_template_scope_let_split.py +++ /dev/null @@ -1,827 +0,0 @@ -# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - -"""A step script's merged ``let`` list spans two scopes; each half must be -evaluated in its own path format. - -An instantiated Step's ``script.let`` is ``step-level bindings + the script's -own``, in that order. The step-level prefix was already evaluated at job -creation in *template* scope, which openjd-rs (and now openjd-model) evaluate -with ``PathFormat::Posix`` so a create-time value cannot depend on the host that -created the job. Re-evaluating that prefix at session time in the host's format -re-renders its PATH values -- on Windows ``path("/foo/bar")`` becomes -``\\foo\\bar``, so ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` -to ``false`` and the same binding holds a different value in the two -evaluations. That is what broke 11 conformance fixtures on the Python-on-Windows -CI leg. - -``apply_script_let_bindings`` owns the split. The script's own bindings are -session scope and keep the host's format, because they legitimately reference -``Session.WorkingDirectory``, ``Task.File.*`` and ``apply_path_mapping``. - -The claim is narrow, and ``TestPrefixScopeIsNarrowedToFormatNeutralSymbols`` -is where the boundary is drawn. Template scope is POSIX, so the prefix is -evaluated against only the symbols in scope that carry no path format. A prefix -binding that needs one -- a PATH job parameter, ``Session.WorkingDirectory``, a -create-time value seeded natively -- cannot be reproduced here at all, and the -whole list falls back to a single host-format evaluation instead: the previous -behaviour, still wrong on Windows for that script, but never raising and never -reading a path under a format it was not built in. - -Note on what these tests can and cannot prove. The path format handed to each -half is asserted directly, by spying on the model's ``evaluate_let_bindings``, -rather than by comparing rendered values. On a POSIX host the host format *is* -POSIX, so a value comparison cannot distinguish "POSIX because we asked for it" -from "POSIX because that is the host" -- it would pass on this host no matter -what the code did. ``test_path_format_is_load_bearing`` supplies the missing -anchor: it shows a non-POSIX format really does change rendering, so the -argument these tests assert on is the argument that matters. - -Where a rendered value *is* compared, the expectation goes through -``_as_the_host_renders`` rather than a POSIX literal. A POSIX literal is not a -weaker assertion, it is a wrong one on Windows: a binding still holding a path -is seeded in the host's format, so ``path('/foo/bar')`` reads ``\\foo\\bar`` -there and both texts are correct. Hardcoding one made the Windows leg red, and a -red leg judges nothing -- fail-fast cancelled it before it reported, so these -assertions had never been run on Windows at all. -""" - -from __future__ import annotations - -import time -import uuid -from pathlib import Path -from typing import Any, Optional -from unittest.mock import patch as mock_patch - -import pytest - -from openjd.model import SymbolTable, evaluate_let_bindings -from openjd.model.v2023_09 import ( - ModelParsingContext as ModelParsingContext_2023_09, - StepScript as StepScript_2023_09, -) -from openjd.sessions import ActionState, Session, SessionState -from openjd.sessions._embedded_files import EmbeddedFilesScope -from openjd.sessions._runner_base import apply_let_bindings, apply_script_let_bindings -from openjd.sessions._runner_step_script import StepScriptRunner - -from .conftest import build_logger - -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -_A_PATH_BINDING = "path('/foo/bar')" -"""A binding RHS whose value renders differently per path format, which is the -whole reason the split exists.""" - - -class _FakeScript: - """Stands in for an instantiated ``StepScript``. - - A fake rather than a real model object because these tests are about the - *boundary index*, and the model only produces indices its own templates can - express. Reading the count off a plain attribute is exactly what the - ``getattr`` in the helper does, and the real-model wiring is pinned - separately by :class:`TestStepScriptWiring`. - """ - - def __init__(self, count: int) -> None: - self._template_scope_let_count = count - - -class _NoCountScript: - """An openjd-model that predates the model-side half of the fix: no - ``_template_scope_let_count`` attribute at all.""" - - -def _set_count(script: Any, count: int) -> None: - """Set the template-scope boundary on a real model object. - - ``setattr`` rather than a direct assignment because openjd-sessions builds - against ``openjd-model >= 0.11.6``, which does not declare the private - attribute -- a direct assignment fails ``hatch run typing`` against the - declared floor. Reaching it through ``setattr`` keeps the tests type-clean - on both model versions, which is the same reason the helper under test - reads it through ``getattr``. - """ - setattr(script, "_template_scope_let_count", count) - - -def _spy_on_evaluation(): - """Patch the model's ``evaluate_let_bindings`` where openjd-sessions imports - it, recording every call while still evaluating for real. - - Spying here rather than on ``apply_let_bindings`` keeps the - ``MAX_LET_BINDING_LENGTH`` guard and the real evaluation in the loop, so a - test can assert both the calls and the resulting symbol values. - """ - return mock_patch( - "openjd.sessions._runner_base.evaluate_let_bindings", - side_effect=evaluate_let_bindings, - ) - - -def _calls(spy: Any) -> list[tuple[list[str], Any]]: - """The spy's calls as ``[(let_bindings, path_format), ...]``. - - ``path_format`` is read with ``.get`` because ``apply_let_bindings`` omits - the kwarg entirely for the host format -- it does not exist on openjd-model - at this package's declared floor. Omitted and ``None`` are the same request - (the engine's default, i.e. the host's format), so both read as ``None`` - here. - """ - return [ - (call.kwargs["let_bindings"], call.kwargs.get("path_format")) for call in spy.call_args_list - ] - - -def _posix_format() -> Any: - from openjd.expr import PathFormat - - return PathFormat.POSIX - - -def _as_the_host_renders(rhs: str) -> str: - """The text ``rhs`` reads as on *this* host. - - A binding whose result has left path-space freezes the POSIX text it froze - at job creation; a binding still holding a path renders in the host's - format. So an assertion about the second kind cannot be a POSIX literal -- - ``path('/foo/bar')`` is ``/foo/bar`` on a POSIX host and ``\\foo\\bar`` on - Windows, and both are correct. - - The expectation is built by evaluating the same expression through the same - machinery at the engine default -- which *is* the host's format, and is what - every session-scope binding gets -- rather than by hardcoding one literal - per platform behind a conditional. It reaches the answer by a different - route than the code under test (a direct host-format evaluation, not a POSIX - evaluation re-tagged to the host), so it is not asserting the code against - itself. - """ - symtab = SymbolTable() - apply_let_bindings(symtab=symtab, let_bindings=[f"value = {rhs}"]) - return str(symtab["value"]) - - -def _step_script( - let: list[str], command: str = "echo", args: Optional[list[str]] = None -) -> StepScript_2023_09: - """A real ``StepScript`` carrying ``let``. The merged-list *boundary* is set - by the caller via ``_template_scope_let_count``, because building it through - ``create_job`` would drag a whole job template into a test about one - index.""" - context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) - return StepScript_2023_09.model_validate( - {"let": let, "actions": {"onRun": {"command": command, "args": args or ["ok"]}}}, - context=context, - ) - - -# --------------------------------------------------------------------------- -# The helper -# --------------------------------------------------------------------------- - - -class TestApplyScriptLetBindings: - def test_splits_at_the_template_scope_count(self) -> None: - # GIVEN: four bindings, of which the first two are step level. - bindings = ["a = 1", "b = 2", "c = 3", "d = 4"] - symtab = SymbolTable() - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(2)) - - # THEN: exactly two evaluations, split at index 2, prefix first. - assert _calls(spy) == [ - (["a = 1", "b = 2"], _posix_format()), - (["c = 3", "d = 4"], None), - ] - # ...and both halves landed in the SAME table. - assert [str(symtab[name]) for name in ("a", "b", "c", "d")] == ["1", "2", "3", "4"] - - def test_prefix_evaluates_posix_and_suffix_evaluates_host_format(self) -> None: - # GIVEN - bindings = [f"tmpl = {_A_PATH_BINDING}", f"own = {_A_PATH_BINDING}"] - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(1) - ) - - # THEN: the template-scope half is pinned to POSIX; the session-scope - # half is left at the engine default, which is the host's format. - prefix, suffix = _calls(spy) - assert prefix == ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()) - assert suffix == ([f"own = {_A_PATH_BINDING}"], None) - - def test_path_format_is_load_bearing(self) -> None: - """The anchor for the assertions above: a path format other than POSIX - really does change how a PATH value renders, so forwarding the argument - is not cosmetic. Without this, a mutant that passed the host format for - the prefix would only be caught by an argument comparison that could - itself be dismissed as testing the mock.""" - from openjd.expr import PathFormat - - posix, windows = SymbolTable(), SymbolTable() - - # WHEN - apply_let_bindings( - symtab=posix, let_bindings=[f"p = {_A_PATH_BINDING}"], path_format=PathFormat.POSIX - ) - apply_let_bindings( - symtab=windows, let_bindings=[f"p = {_A_PATH_BINDING}"], path_format=PathFormat.WINDOWS - ) - - # THEN - assert str(posix["p"]) == "/foo/bar" - assert str(windows["p"]) == "\\foo\\bar" - # And the flip that broke the fixtures, reproduced without a Windows host. - assert str(posix["p"]).startswith("/foo") - assert not str(windows["p"]).startswith("/foo") - - def test_no_template_scope_prefix_behaves_exactly_as_before(self) -> None: - # GIVEN: a script with only its own bindings -- count 0. - bindings = ["a = 1", "b = 2"] - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(0) - ) - - # THEN: one evaluation, host format, whole list. No POSIX evaluation at - # all -- a count of 0 must not produce an empty extra call. - assert _calls(spy) == [(bindings, None)] - - def test_missing_count_attribute_falls_back_to_host_format(self) -> None: - """An openjd-model without the model-side half of the fix must degrade - to the previous behaviour, not raise.""" - # GIVEN - bindings = ["a = 1", "b = 2"] - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=SymbolTable(), let_bindings=bindings, script=_NoCountScript() - ) - - # THEN - assert _calls(spy) == [(bindings, None)] - - def test_no_script_falls_back_to_host_format(self) -> None: - """What an environment script's caller passes: its own bindings are - session scope and correctly use the host format.""" - # GIVEN - bindings = ["a = 1"] - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=SymbolTable(), let_bindings=bindings) - - # THEN - assert _calls(spy) == [(bindings, None)] - - def test_count_beyond_the_list_falls_back_to_session_scope(self) -> None: - """A model/sessions version skew reporting a boundary the list cannot - have is not guessed at. - - Clamping to the list length was the earlier behaviour and it is worse: - it would evaluate a genuinely session-scope binding in template scope. - Falling back to 0 is the pre-fix behaviour, which is wrong on Windows - but never raises and never mis-scopes a binding.""" - # GIVEN - bindings = ["a = 1"] - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(5) - ) - - # THEN: one evaluation, host format, whole list. - assert _calls(spy) == [(bindings, None)] - - def test_negative_count_falls_back_to_session_scope(self) -> None: - """Same guard, other side: a negative boundary is impossible.""" - # GIVEN - bindings = ["a = 1", "b = 2"] - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=SymbolTable(), let_bindings=bindings, script=_FakeScript(-1) - ) - - # THEN - assert _calls(spy) == [(bindings, None)] - - def test_ordering_is_preserved_across_the_boundary(self) -> None: - """A script-level binding may reference a step-level one, so the prefix - must be evaluated -- into the same table -- before the suffix.""" - # GIVEN: `under` is session scope and reads `root`, which is template - # scope. - bindings = [f"root = {_A_PATH_BINDING}", "under = startswith(root, '/foo')"] - symtab = SymbolTable() - - # WHEN - apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) - - # THEN: `root` is still a path when it is seeded, so it reads in the - # host's format. `under` is session scope and reads it there, so its - # answer is host-dependent too -- `false` on Windows, where `root` is - # `\foo\bar` and does not start with `/foo`. That is the correct - # behaviour for a session-scope binding, not a defect: the ordering this - # test is about is that `under` sees `root` at all. - assert str(symtab["root"]) == _as_the_host_renders(_A_PATH_BINDING) - assert str(symtab["under"]) == _as_the_host_renders( - f"startswith({_A_PATH_BINDING}, '/foo')" - ) - - def test_a_failing_suffix_binding_still_raises(self) -> None: - """The split must not swallow an evaluation error in either half.""" - # WHEN / THEN - with pytest.raises(ValueError, match="let binding 'bad'"): - apply_script_let_bindings( - symtab=SymbolTable(), - let_bindings=["ok = 1", "bad = NoSuchSymbol"], - script=_FakeScript(1), - ) - - -# --------------------------------------------------------------------------- -# The narrowed prefix scope, and the all-or-nothing fallback -# --------------------------------------------------------------------------- - - -class TestPrefixScopeIsNarrowedToFormatNeutralSymbols: - """Template scope is POSIX, so the prefix cannot read a session symbol that - carries the host's path format. - - Reading one either raises ``Path format mismatch`` or -- worse -- silently - succeeds against a re-rendered value: ``.parent`` of a Windows path read as - POSIX is ``'.'``, because a backslash is an ordinary POSIX path character. - So those symbols are not in scope for the prefix, and a prefix that needs one - abandons the split for the whole list. - - These assertions are host-independent: the filter removes the symbol on - either host, so the binding fails with ``Undefined variable`` on both. - """ - - @staticmethod - def _session_shaped_symtab() -> SymbolTable: - """A symbol table with one symbol of each shape the session seeds.""" - symtab = SymbolTable() - symtab["Job.Name"] = "a-job" - symtab["Param.S"] = "text" - symtab.expr_types["Param.S"] = "STRING" - symtab["Param.N"] = "3" - symtab.expr_types["Param.N"] = "INT" - symtab["Param.Out"] = "/mnt/out" - symtab.expr_types["Param.Out"] = "PATH" - symtab["Param.Ins"] = ["/mnt/a", "/mnt/b"] - symtab.expr_types["Param.Ins"] = "LIST[PATH]" - symtab["Session.WorkingDirectory"] = "/sessions/s1" - symtab.expr_types["Session.WorkingDirectory"] = "PATH" - return symtab - - @staticmethod - def _unsplit(symtab: SymbolTable, bindings: list[str]) -> SymbolTable: - """The pre-fix behaviour: the whole list, once, in the host's format.""" - before = SymbolTable(source=symtab) - apply_let_bindings(symtab=before, let_bindings=bindings) - return before - - def _assert_fell_back(self, bindings: list[str], count: int) -> None: - """The prefix was attempted in POSIX, declined, and the WHOLE list was - then evaluated once in the host's format -- with the pre-fix values.""" - symtab = self._session_shaped_symtab() - expected = self._unsplit(symtab, bindings) - - with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=symtab, let_bindings=bindings, script=_FakeScript(count) - ) - - assert _calls(spy) == [(bindings[:count], _posix_format()), (bindings, None)] - bound = [b.partition("=")[0].strip() for b in bindings] - assert [str(symtab[n]) for n in bound] == [str(expected[n]) for n in bound] - - def test_a_self_contained_prefix_binding_still_freezes(self) -> None: - """The case the fix exists for is untouched: a prefix that reads nothing - format-carrying is still evaluated in template scope.""" - # GIVEN - bindings = [f"tmpl = string({_A_PATH_BINDING})", "own = 1"] - symtab = self._session_shaped_symtab() - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) - - # THEN: still split, and the value froze the POSIX text on either host. - assert _calls(spy) == [([bindings[0]], _posix_format()), (["own = 1"], None)] - assert str(symtab["tmpl"]) == "/foo/bar" - - def test_a_format_neutral_symbol_is_readable_from_the_prefix(self) -> None: - """The filter is a *shape* test, not a name denylist: a STRING or INT - parameter and ``Job.Name`` carry no path format, so they stay in scope - and do not trigger the fallback. - - ``Param.N * 2`` is 6 only if the symbol's declared INT type came across - with it; an untyped ``"3"`` would make it the string ``"33"``.""" - # GIVEN - bindings = ["label = join([Job.Name, Param.S], '-')", "doubled = Param.N * 2"] - symtab = self._session_shaped_symtab() - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(2)) - - # THEN: one POSIX evaluation of the whole prefix, and no fallback call. - assert _calls(spy) == [(bindings, _posix_format())] - assert str(symtab["label"]) == "a-job-text" - assert str(symtab["doubled"]) == "6" - - def test_a_prefix_binding_reading_a_path_parameter_falls_back(self) -> None: - # GIVEN: `Param.Out` is a host-format PATH. - self._assert_fell_back(["out = string(Param.Out)", "own = 1"], count=1) - - def test_a_prefix_binding_reading_a_list_path_parameter_falls_back(self) -> None: - # GIVEN: LIST[PATH] carries a format as much as PATH does. - self._assert_fell_back(["ins = string(Param.Ins[0])", "own = 1"], count=1) - - def test_a_prefix_binding_reading_the_session_working_directory_falls_back(self) -> None: - # GIVEN: the symbol whose `.parent` silently yields '.' on Windows. - self._assert_fell_back(["under = string(Session.WorkingDirectory.parent)"], count=1) - - @pytest.mark.parametrize( - "base_rhs, read", - [ - pytest.param(_A_PATH_BINDING, "base", id="path"), - pytest.param(f"[{_A_PATH_BINDING}, path('/a')]", "base[0]", id="list[path]"), - ], - ) - def test_a_prefix_binding_reading_a_native_path_value_falls_back( - self, base_rhs: str, read: str - ) -> None: - """The other half of the filter. A create-time table reaches the session - as native engine values (``Session._resolved_base_entries``), so a - path-typed one carries its format in the value itself with no - ``expr_types`` entry to declare it. A native ``list[path]`` carries one - just as much, one type parameter down.""" - # GIVEN: `base` in the shape `_resolved_base_entries` produces -- a - # native path value tagged with the host's format. - seed = SymbolTable() - apply_let_bindings(symtab=seed, let_bindings=[f"base = {base_rhs}"]) - symtab = self._session_shaped_symtab() - symtab["base"] = seed["base"] - assert "base" not in symtab.expr_types - bindings = [f"out = string({read})", "own = 1"] - expected = self._unsplit(symtab, bindings) - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) - - # THEN - assert _calls(spy) == [([bindings[0]], _posix_format()), (bindings, None)] - assert str(symtab["out"]) == str(expected["out"]) - - def test_the_fallback_leaves_no_partial_prefix_behind(self) -> None: - """All-or-nothing. A prefix binding that succeeded in POSIX before a - later one declined must not be seeded: it would leave a POSIX-evaluated - value for a host-evaluated sibling to read.""" - # GIVEN: `first` evaluates fine in POSIX; `second` needs a PATH param. - bindings = [f"first = string({_A_PATH_BINDING})", "second = string(Param.Out)"] - symtab = self._session_shaped_symtab() - expected = self._unsplit(symtab, bindings) - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(2)) - - # THEN: both names hold the host-format value, not the POSIX one. - assert _calls(spy) == [(bindings, _posix_format()), (bindings, None)] - assert str(symtab["first"]) == str(expected["first"]) - assert str(symtab["second"]) == str(expected["second"]) - - def test_a_prefix_binding_calling_apply_path_mapping_falls_back(self) -> None: - """``apply_path_mapping`` is a *host-context* function, and RFC 0005 bars - those from template scope: openjd-model invokes the create-time hook with - no host context, so a template-scope binding calling one raises - ``Unknown function: 'apply_path_mapping'`` at job creation and never had - a create-time value to reproduce. So this evaluation must not resolve it - either -- it must decline and let the whole list fall back. - - The assertion is the fallback, not the raise: the raise is internal to - the helper and is swallowed by design. - - An earlier revision copied ``symtab.expr_host_rules`` into the scratch - table, which made such a binding resolve here against the *host's* rules - while evaluating in POSIX -- freezing a mixed-separator value like - ``C:\\Users\\test/bar`` instead of falling back to the host-format value. - """ - # GIVEN: a session table in the shape `Session._resolved_base_entries` - # leaves it -- host rules attached (`_session.py` seeds `[]` even with no - # rules, so `apply_path_mapping` stays available in session scope). - from openjd.expr import PathFormat, PathMappingRule - - bindings = ["mapped = string(apply_path_mapping(path('/foo/bar')))", "own = 1"] - rule = PathMappingRule( - source_path_format=PathFormat.POSIX, - source_path="/foo", - destination_path="C:\\Users\\test", - ) - symtab = self._session_shaped_symtab() - symtab.expr_host_rules = [rule] - # `_unsplit` derives its table with `SymbolTable(source=...)`, which - # carries the host rules across, so the expectation has them too. - expected = self._unsplit(symtab, bindings) - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=bindings, script=_FakeScript(1)) - - # THEN: the POSIX prefix was attempted, declined, and the WHOLE list was - # re-evaluated in the host's format. Without the fallback the second call - # would be `["own = 1"]` alone, with `mapped` holding the POSIX-evaluated - # value. - assert _calls(spy) == [([bindings[0]], _posix_format()), (bindings, None)] - assert str(symtab["mapped"]) == str(expected["mapped"]) - - def test_the_filter_survives_the_derived_table_production_hands_it(self) -> None: - """The narrowing must still work on the table shape the runners actually - pass, which is not the session table itself but a *derived* one: - ``SymbolTable(source=self._symtab)`` at ``_runner_step_script.py:110`` - and ``:122``, and ``SymbolTable(source=base)`` at ``_session.py:2032``. - - Why this is worth its own test, given the unwrapped cases above already - pass. The filter identifies a plain-string PATH symbol purely from its - ``expr_types`` entry, and that entry only survives the wrap because - openjd-model's ``SymbolTable.__init__`` copies it -- - ``self._expr_types.update(source._expr_types)`` in - ``openjd/model/_symbol_table.py`` (line 131 at the version this pins - against). Nothing in openjd-sessions re-declares those types. If a future - openjd-model refactor drops that one line, ``declared_types`` here goes - empty, every plain-string PATH symbol reads as format-neutral, and the - filter silently stops filtering -- the split would then read - ``Session.WorkingDirectory`` under POSIX on a Windows host, which is the - exact defect the filter exists to prevent. No other test in this file - would notice, because they all pass the undertived table. - """ - # GIVEN: the session-shaped table, wrapped exactly as production wraps it. - base = self._session_shaped_symtab() - derived = SymbolTable(source=base) - # The wrap is what is under test, so state the precondition it depends on. - assert derived.expr_types.get("Session.WorkingDirectory") == "PATH" - bindings = ["under = string(Session.WorkingDirectory.parent)", "own = 1"] - expected = self._unsplit(derived, bindings) - - # WHEN - with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=derived, let_bindings=bindings, script=_FakeScript(1)) - - # THEN: the PATH symbol was still filtered out, so the prefix declined - # and the whole list fell back to the host's format. - assert _calls(spy) == [([bindings[0]], _posix_format()), (bindings, None)] - assert str(derived["under"]) == str(expected["under"]) - - def test_a_failing_prefix_binding_still_raises_through_the_fallback(self) -> None: - """The fallback must not turn a genuine error into silence. Evaluating - the whole list in the host's format re-raises it -- which is exactly what - the pre-fix code did.""" - # WHEN / THEN - with pytest.raises(ValueError, match="let binding 'bad'"): - apply_script_let_bindings( - symtab=self._session_shaped_symtab(), - let_bindings=["bad = NoSuchSymbol", "own = 1"], - script=_FakeScript(1), - ) - - -# --------------------------------------------------------------------------- -# The wiring: the runner and the RFC 0008 wrapped-inner-scope path -# --------------------------------------------------------------------------- - - -@pytest.mark.usefixtures("message_queue", "queue_handler") -class TestStepScriptWiring: - """The helper is only useful if the sites that evaluate a step script's - merged ``let`` actually hand it the script.""" - - def _run( - self, - queue_handler: Any, - session_dir: Path, - script: StepScript_2023_09, - count: int, - ) -> list[tuple[list[str], Any]]: - _set_count(script, count) - # `with runner:` rather than `with StepScriptRunner(...) as runner:` -- - # ScriptRunnerBase.__enter__ is annotated as returning the base class, so - # the `as` form loses the subclass and with it `run()`. - runner = StepScriptRunner( - logger=build_logger(queue_handler), - script=script, - symtab=SymbolTable(), - session_working_directory=session_dir, - session_files_directory=session_dir, - ) - with runner: - with _spy_on_evaluation() as spy: - runner.run() - deadline = time.time() + 20 - while runner.state.value == "running" and time.time() < deadline: - time.sleep(0.05) - return _calls(spy) - - def test_step_runner_splits_its_merged_let( - self, queue_handler: Any, tmp_path: Path, python_exe: str - ) -> None: - # GIVEN: a step script whose first binding is step level. - script = _step_script( - [f"tmpl = {_A_PATH_BINDING}", "own = 1"], - command=python_exe, - args=["-c", "pass"], - ) - - # WHEN - calls = self._run(queue_handler, tmp_path, script, count=1) - - # THEN - assert calls == [ - ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()), - (["own = 1"], None), - ] - - def test_step_runner_with_embedded_files_splits_its_merged_let( - self, queue_handler: Any, tmp_path: Path, python_exe: str - ) -> None: - """The embedded-files branch evaluates the same merged list through - ``_materialize_files``, so it needs the same split. Missing this leaves - the bug live for any step that has both step-level bindings and - embedded files.""" - # GIVEN - context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) - script = StepScript_2023_09.model_validate( - { - "let": [f"tmpl = {_A_PATH_BINDING}", "own = 1"], - "embeddedFiles": [{"name": "F", "type": "TEXT", "data": "{{ tmpl }}"}], - "actions": {"onRun": {"command": python_exe, "args": ["-c", "pass"]}}, - }, - context=context, - ) - - # WHEN - calls = self._run(queue_handler, tmp_path, script, count=1) - - # THEN - assert calls == [ - ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()), - (["own = 1"], None), - ] - - def test_wrapped_inner_scope_splits_a_step_scripts_merged_let(self) -> None: - """RFC 0008: the wrapped action's scope is rebuilt from the inner - script's ``let``, so it must be split the same way -- otherwise a - wrapped action resolves against a scope that differs from the one it - would have had unwrapped.""" - # GIVEN - script = _step_script([f"tmpl = {_A_PATH_BINDING}", "own = 1"]) - _set_count(script, 1) - session = Session(session_id=uuid.uuid4().hex, job_parameter_values={}) - - # WHEN - try: - with _spy_on_evaluation() as spy: - inner = session._build_wrapped_inner_scope( - EmbeddedFilesScope.STEP, script.let, None, SymbolTable(), script - ) - finally: - session.cleanup() - - # THEN - assert _calls(spy) == [ - ([f"tmpl = {_A_PATH_BINDING}"], _posix_format()), - (["own = 1"], None), - ] - # `tmpl` is still a path, so it is seeded in the host's format. - assert str(inner["tmpl"]) == _as_the_host_renders(_A_PATH_BINDING) - - def test_environment_script_bindings_stay_host_format( - self, queue_handler: Any, tmp_path: Path, python_exe: str - ) -> None: - """The other half of the contract: an environment script's own ``let`` - is session scope. Nothing about it may change.""" - from openjd.model.v2023_09 import EnvironmentScript as EnvironmentScript_2023_09 - from openjd.sessions._runner_env_script import EnvironmentScriptRunner - - # GIVEN - context = ModelParsingContext_2023_09(supported_extensions=["EXPR"]) - env_script = EnvironmentScript_2023_09.model_validate( - { - "let": [f"a = {_A_PATH_BINDING}", "b = 1"], - "actions": {"onEnter": {"command": python_exe, "args": ["-c", "pass"]}}, - }, - context=context, - ) - - # WHEN - runner = EnvironmentScriptRunner( - logger=build_logger(queue_handler), - environment_script=env_script, - symtab=SymbolTable(), - session_working_directory=tmp_path, - session_files_directory=tmp_path, - ) - with runner: - with _spy_on_evaluation() as spy: - runner.enter() - deadline = time.time() + 20 - while runner.state.value == "running" and time.time() < deadline: - time.sleep(0.05) - calls = _calls(spy) - - # THEN: one evaluation, host format, whole list. - assert calls == [([f"a = {_A_PATH_BINDING}", "b = 1"], None)] - - -@pytest.mark.usefixtures("message_queue", "queue_handler") -class TestEndToEndScopeAgreement: - """The property the conformance fixtures actually check: the value a - step-level binding holds at session time equals the value it held at job - creation.""" - - def test_a_step_level_path_binding_agrees_across_the_two_evaluations( - self, python_exe: str - ) -> None: - # GIVEN: a step script whose step-level prefix is a path predicate -- - # the shape that flipped on Windows. - script = _step_script( - [f"root = {_A_PATH_BINDING}", "under = startswith(root, '/foo')"], - command=python_exe, - ) - _set_count(script, 2) - - # AND: the value the model computed at job creation, in template scope. - create_time = SymbolTable() - apply_let_bindings( - symtab=create_time, let_bindings=script.let or [], path_format=_posix_format() - ) - - # WHEN: the session re-evaluates the same list. - session_time = SymbolTable() - apply_script_let_bindings(symtab=session_time, let_bindings=script.let or [], script=script) - - # THEN: the two agree, once the create-time side is read at the point in - # its journey that `session_time` occupies. A create-time table does not - # reach a session POSIX-tagged: `Session._resolved_base_entries` - # deserializes it with `to_symtab(path_format=host_format)`, and the - # split applies the same re-tag. So a create-time value still holding a - # path is re-rendered on the way in, while one that left path-space keeps - # its frozen text. - # - # `create_time` here is the raw POSIX evaluation, one step earlier. That - # is the fixture's doing, and comparing the two raw is what made this - # host-dependent -- not the rule. - assert str(create_time["root"]) == "/foo/bar" - assert str(session_time["root"]) == _as_the_host_renders(_A_PATH_BINDING) - # `under` left path-space during the create-time evaluation, so both - # sides hold the same frozen text on every host. Both bindings are - # template scope here (count=2), which is what makes this the whole - # property the fixtures check. - assert str(session_time["under"]) == str(create_time["under"]) == "true" - - def test_a_session_runs_a_step_with_a_step_level_path_binding(self, python_exe: str) -> None: - """End to end through the public API, so the split cannot break the - ordinary run.""" - # GIVEN: the action's exit status is driven by the step-level binding's - # value, so a scope disagreement fails the action rather than passing - # quietly. - script = _step_script( - [f"root = {_A_PATH_BINDING}", "under = startswith(root, '/foo')"], - command=python_exe, - args=["-c", "import sys; sys.exit(0 if sys.argv[1] == 'true' else 1)", "{{ under }}"], - ) - # count=2, so BOTH bindings are template scope. With count=1, `under` - # would be session scope, and on a Windows host it reads `root` in host - # format and evaluates to `false` -- correct behaviour, but it would make - # this assertion fail there while passing on POSIX. The step-level pair - # is what this test is about, so both belong in the prefix. - _set_count(script, 2) - - session = Session(session_id=uuid.uuid4().hex, job_parameter_values={}) - try: - # WHEN - session.run_task(step_script=script, task_parameter_values={}) - deadline = time.time() + 20 - while session.state == SessionState.RUNNING and time.time() < deadline: - time.sleep(0.05) - - # THEN - status = session.action_status - assert status is not None and status.state == ActionState.SUCCESS, status - finally: - session.cleanup() diff --git a/test/openjd/test_import_purity.py b/test/openjd/test_import_purity.py index f3725675..df25d180 100644 --- a/test/openjd/test_import_purity.py +++ b/test/openjd/test_import_purity.py @@ -410,25 +410,22 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: # --------------------------------------------------------------------------- -# `apply_script_let_bindings` imports `openjd.expr.PathFormat` to pin a step -# script's template-scope `let` prefix to POSIX. A function-local import is not -# enough on its own (rule: lazy is not conditional) -- the enclosing function is -# reachable from every script that has any `let` at all, so the import has to sit -# behind "there is a template-scope prefix to evaluate". +# `apply_script_let_bindings` evaluates a script's own `let` list and nothing +# else, so on its own it must not drag the native extension in. # -# Both probes use a MALFORMED binding, which openjd-model skips without parsing. -# That removes the evaluation itself as a possible cause of the load, leaving the -# PathFormat import as the only crossing either probe can observe. +# The probe uses a MALFORMED binding, which openjd-model skips without parsing. +# That removes the evaluation itself as a cause of the load, so anything the +# probe observes would have to be an unguarded import on the call path. # --------------------------------------------------------------------------- -_LET_SPLIT_PROBE = """ +_LET_PROBE = """ from openjd.model import SymbolTable from openjd.sessions._runner_base import apply_script_let_bindings class Script: - _template_scope_let_count = %d + pass apply_script_let_bindings( @@ -438,32 +435,14 @@ class Script: """ -def test_a_let_list_with_no_template_scope_prefix_stays_pure(tmp_path: Path) -> None: - """A script whose ``let`` is entirely its own -- the only shape a non-EXPR - template can even produce -- must not reach the ``PathFormat`` import.""" +def test_applying_a_scripts_let_list_stays_pure(tmp_path: Path) -> None: + """Evaluating a script's ``let`` must not itself load the native extension.""" # WHEN - loaded = _run_probe(tmp_path, _LET_SPLIT_PROBE % 0) + loaded = _run_probe(tmp_path, _LET_PROBE) # THEN assert loaded == "False", ( - "evaluating a let list with no template-scope prefix loaded the native " - "extension. The `if template_scope_count:` guard around the PathFormat " - "import in apply_script_let_bindings is what prevents this; a bare " - "function-local import is not sufficient." - ) - - -def test_a_template_scope_prefix_does_load_the_extension(tmp_path: Path) -> None: - """Positive control for the probe above. Without it, ``False`` would be - indistinguishable from the probe being unable to observe the load at all -- - and it confirms the guarded import is the only crossing on this path, since - the malformed binding is never parsed.""" - # WHEN - loaded = _run_probe(tmp_path, _LET_SPLIT_PROBE % 1) - - # THEN - assert loaded == "True", ( - "a template-scope prefix must load the extension to reach " - "PathFormat.POSIX; if this is False the prefix is no longer being pinned " - "to POSIX at all" + "applying a script's let list loaded the native extension. Nothing on " + "this path should import openjd.expr: the bindings are evaluated by " + "openjd-model, which skips a malformed binding without parsing it." ) From 27054b3696042c80ef23252d51ad6b2683e26b69 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 01:32:16 -0700 Subject: [PATCH 08/11] refactor: Drop unused script param from let helpers `apply_script_let_bindings` took a `script` model object that nothing read. It was threaded there through `ScriptRunnerBase._materialize_files`, `ScriptRunnerBase._apply_let_bindings_or_fail` and `Session._build_wrapped_inner_scope`, each of which carried the parameter only to forward it, and each of which documented in a paragraph that it was unused. The parameter was live when it was introduced in dbfd1b5: a step script's `let` list was then a merge of template scope and session scope, and the script was read to find the boundary between them. 5c3c8c9 removed that re-evaluation, so a script's `let` is now entirely its own scope and needs no per-script information to evaluate. An unused parameter plus a paragraph explaining that it is unused is worse than neither. No behaviour change: the parameter had no reader on any path. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_runner_base.py | 27 ++++--------------- src/openjd/sessions/_runner_step_script.py | 3 +-- src/openjd/sessions/_session.py | 16 +++++------ .../sessions_v0/test_let_binding_scopes.py | 16 +++-------- test/openjd/test_import_purity.py | 8 +----- 5 files changed, 17 insertions(+), 53 deletions(-) diff --git a/src/openjd/sessions/_runner_base.py b/src/openjd/sessions/_runner_base.py index f7f501a2..0946a80d 100644 --- a/src/openjd/sessions/_runner_base.py +++ b/src/openjd/sessions/_runner_base.py @@ -542,9 +542,7 @@ def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings) -def apply_script_let_bindings( - *, symtab: SymbolTable, let_bindings: list[str], script: Any = None -) -> None: +def apply_script_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: """Evaluate a script's own ``let`` list into ``symtab`` in the host's path format. @@ -565,11 +563,6 @@ def apply_script_let_bindings( value. Nothing in a session re-evaluates a step's bindings; it reads the resolved ones. - ``script`` is the model object the ``let`` list came from. It is accepted so - the runners and ``Session._build_wrapped_inner_scope`` can pass the script - they already have, but nothing is read off it: a script's own ``let`` needs - no per-script scope information. - Raises: ValueError: as :func:`apply_let_bindings`. """ @@ -1065,15 +1058,10 @@ def _materialize_files( symtab: SymbolTable, let_bindings: Optional[list[str]] = None, preallocated_records: Optional[list[_FileRecord]] = None, - script: Any = None, ) -> None: """Helper for derived classes that wraps all of the logic around materializing embedded files to disk. - ``script`` is the model object ``let_bindings`` came from, forwarded to - :func:`apply_script_let_bindings`. Every binding is evaluated in the - host's path format. - When ``let_bindings`` is given, they are evaluated between file-path allocation and content writing (RFC 0005, mirroring the openjd-rs runners): a file's *path* never depends on ``let`` values (filenames @@ -1111,7 +1099,7 @@ def _materialize_files( else: records = file_writer.allocate_file_paths(files, symtab) if let_bindings: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) file_writer.write_file_contents(records, symtab) except (RuntimeError, ValueError) as exc: # Had a problem writing at least one file to disk, or evaluating @@ -1119,17 +1107,12 @@ def _materialize_files( # ValueError). Surface the error. self._fail_action(str(exc)) - def _apply_let_bindings_or_fail( - self, symtab: SymbolTable, let_bindings: list[str], script: Any = None - ) -> bool: + def _apply_let_bindings_or_fail(self, symtab: SymbolTable, let_bindings: list[str]) -> bool: """Evaluate the script's EXPR ``let`` bindings into ``symtab``. On an evaluation error the action is failed through the normal failure path - (openjd_fail log, FAILED state, callback). Returns True on success. - - ``script`` is the model object the bindings came from, forwarded to - :func:`apply_script_let_bindings`.""" + (openjd_fail log, FAILED state, callback). Returns True on success.""" try: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) except ValueError as exc: self._fail_action(str(exc)) return False diff --git a/src/openjd/sessions/_runner_step_script.py b/src/openjd/sessions/_runner_step_script.py index 6193bb83..3c683bbd 100644 --- a/src/openjd/sessions/_runner_step_script.py +++ b/src/openjd/sessions/_runner_step_script.py @@ -114,13 +114,12 @@ def run(self) -> None: self._session_files_directory, symtab, let_bindings=let_bindings, - script=self._script, ) if self.state == ScriptRunnerState.FAILED: return elif let_bindings: symtab = SymbolTable(source=self._symtab) - if not self._apply_let_bindings_or_fail(symtab, let_bindings, self._script): + if not self._apply_let_bindings_or_fail(symtab, let_bindings): return else: symtab = self._symtab diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 89df9ec4..f994a270 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -1998,7 +1998,6 @@ def _build_wrapped_inner_scope( let_bindings: Optional[list[str]], embedded_files: Optional[Any], base: SymbolTable, - script: Any = None, ) -> SymbolTable: """Build the scope a wrapped action would have resolved against had it run unwrapped: a copy of ``base`` (the session-scope table) plus @@ -2013,12 +2012,10 @@ def _build_wrapped_inner_scope( symmetrically, the inner entity's lets never apply to the hook's own resolution scope. Mirrors openjd-rs's ``build_wrapped_inner_scope``. - ``script`` is the inner entity's script -- the model object - ``let_bindings`` came from -- forwarded to - :func:`~._runner_base.apply_script_let_bindings` exactly as the step - runner forwards it. A script's own ``let`` is session scope either way, - so a wrapped action resolves against the same scope it would have had - unwrapped, which is the property this method exists to reproduce. + A script's own ``let`` is session scope here exactly as it is in the + runners, so a wrapped action resolves against the same scope it would + have had unwrapped, which is the property this method exists to + reproduce. Raises: ValueError (FormatStringError/ExpressionError): a binding or file @@ -2035,10 +2032,10 @@ def _build_wrapped_inner_scope( ) records = file_writer.allocate_file_paths(embedded_files, symtab) if let_bindings: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) file_writer.write_file_contents(records, symtab) elif let_bindings: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script) + apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) return symtab def _try_inject_wrapped_symbols( @@ -2069,7 +2066,6 @@ def _try_inject_wrapped_symbols( inner_script.let if inner_script is not None else None, inner_script.embeddedFiles if inner_script is not None else None, symtab, - inner_script, ) inject(inner_symtab) except (FormatStringError, ValueError, RuntimeError) as e: diff --git a/test/openjd/sessions_v0/test_let_binding_scopes.py b/test/openjd/sessions_v0/test_let_binding_scopes.py index 2c5500e1..19ee9955 100644 --- a/test/openjd/sessions_v0/test_let_binding_scopes.py +++ b/test/openjd/sessions_v0/test_let_binding_scopes.py @@ -180,9 +180,7 @@ def test_a_seeded_path_binding_survives_a_scripts_own_let(self) -> None: # WHEN with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=symtab, let_bindings=script.let or [], script=script - ) + apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) # THEN: the seeded value is untouched, in the host's format. assert str(symtab[_SEEDED_NAME]) == _SEEDED_WINDOWS_TEXT, ( @@ -209,9 +207,7 @@ def test_a_step_level_binding_is_not_evaluated_at_session_time(self) -> None: # WHEN with _spy_on_evaluation() as spy: - apply_script_let_bindings( - symtab=symtab, let_bindings=script.let or [], script=script - ) + apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) # THEN: the evaluator saw the script's own bindings and nothing else. evaluated = _evaluated_bindings(spy) @@ -243,9 +239,7 @@ def test_it_evaluates_in_the_host_format_and_sees_session_symbols(self) -> None: symtab = _session_symtab(session) # WHEN - apply_script_let_bindings( - symtab=symtab, let_bindings=script.let or [], script=script - ) + apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) # THEN: the path rendered in the *host's* format, not POSIX. assert str(symtab["built"]) == r"\a\b", ( @@ -270,6 +264,4 @@ def test_a_failing_binding_still_raises(self) -> None: # WHEN / THEN with pytest.raises(ValueError, match="bad"): - apply_script_let_bindings( - symtab=symtab, let_bindings=script.let or [], script=script - ) + apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) diff --git a/test/openjd/test_import_purity.py b/test/openjd/test_import_purity.py index df25d180..53e4287b 100644 --- a/test/openjd/test_import_purity.py +++ b/test/openjd/test_import_purity.py @@ -424,13 +424,7 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: from openjd.sessions._runner_base import apply_script_let_bindings -class Script: - pass - - -apply_script_let_bindings( - symtab=SymbolTable(), let_bindings=["malformed"], script=Script() -) +apply_script_let_bindings(symtab=SymbolTable(), let_bindings=["malformed"]) print(RS in sys.modules) """ From 8d0f1c8c12f51d94f9bec79cbb86cea3530db2dd Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 08:22:12 -0700 Subject: [PATCH 09/11] test: Fix two unsound let-binding assertions The host-format test normalised path separators on only one side, so on Windows CI, where both sides already render with backslashes, the normalised left side no longer matched: assert 'C:/ProgramData/Amazon/OpenJD/60htba6m' == 'C:\ProgramData\Amazon\OpenJD\60htba6m' `session.working_directory` is a real path object in the host OS's flavour, while the binding renders in the format `_windows_host` forces. Compare both sides through `PureWindowsPath` instead: the claim is *which* path the binding saw, not how it renders, and the format claim is the neighbouring `built` assertion. Reproduced on Windows 3.11, 3.12 and 3.14. Also make the `apply_script_let_bindings` purity probe say something. It used a malformed binding, which openjd-model skips without parsing, so the evaluation path never ran and the assertion was near-tautological. Split it in two: an empty `let` list must not load the native extension, which is the production-reachable purity claim, and a valid non-path binding (`mine = 1 + 1`, with its bound value asserted so a silent skip fails) does load it. The latter is recorded as a documented limitation rather than forced green, alongside `test_path_mapping_rules_do_load_the_extension`. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../sessions_v0/test_let_binding_scopes.py | 21 ++++-- test/openjd/test_import_purity.py | 72 +++++++++++++++---- 2 files changed, 73 insertions(+), 20 deletions(-) diff --git a/test/openjd/sessions_v0/test_let_binding_scopes.py b/test/openjd/sessions_v0/test_let_binding_scopes.py index 19ee9955..af8dcf2b 100644 --- a/test/openjd/sessions_v0/test_let_binding_scopes.py +++ b/test/openjd/sessions_v0/test_let_binding_scopes.py @@ -40,6 +40,7 @@ import json import uuid from contextlib import contextmanager +from pathlib import PureWindowsPath from typing import Any, Generator, Optional from unittest.mock import patch as mock_patch @@ -247,13 +248,19 @@ def test_it_evaluates_in_the_host_format_and_sees_session_symbols(self) -> None: "host's path format" ) # AND: it resolved against the live session symbol table. - # `Session.WorkingDirectory` is PATH-typed, so on the simulated - # Windows host it renders with backslashes too. The claim here is - # *which* path the binding saw, not how it renders, so the - # separator is normalised before comparing; the format claim is - # the `built` assertion above. - rendered_wd = str(symtab["wd"]).replace("\\", "/") - assert rendered_wd == str(session.working_directory) + # `Session.WorkingDirectory` is PATH-typed, so under the forced + # Windows format it renders with backslashes -- while + # `session.working_directory` is a real path object in the *host + # OS's* flavour, which is POSIX here and Windows on CI. The claim + # is *which* path the binding saw, not how it renders, so both + # sides are compared as paths rather than as text. + # `PureWindowsPath` is the right parser for the rendered side + # because the format was forced to Windows; it also accepts `/` + # as a separator, so a POSIX `working_directory` parses to the + # same parts. The format claim is the `built` assertion above. + assert PureWindowsPath(str(symtab["wd"])) == PureWindowsPath( + session.working_directory + ) def test_a_failing_binding_still_raises(self) -> None: """Negative control for the two tests above: the evaluation is real, so a diff --git a/test/openjd/test_import_purity.py b/test/openjd/test_import_purity.py index 53e4287b..af987d95 100644 --- a/test/openjd/test_import_purity.py +++ b/test/openjd/test_import_purity.py @@ -410,33 +410,79 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: # --------------------------------------------------------------------------- -# `apply_script_let_bindings` evaluates a script's own `let` list and nothing -# else, so on its own it must not drag the native extension in. -# -# The probe uses a MALFORMED binding, which openjd-model skips without parsing. -# That removes the evaluation itself as a cause of the load, so anything the -# probe observes would have to be an unguarded import on the call path. +# `apply_script_let_bindings` is on the session path for every script, EXPR or +# not, so reaching it must not by itself load the native extension. Actually +# *evaluating* a binding must, because the expression engine is the extension -- +# the two cases are split below so each says which it is. # --------------------------------------------------------------------------- +_NO_BINDINGS_PROBE = """ +from openjd.model import SymbolTable +from openjd.sessions._runner_base import apply_script_let_bindings + + +apply_script_let_bindings(symtab=SymbolTable(), let_bindings=[]) +print(RS in sys.modules) +""" + + _LET_PROBE = """ from openjd.model import SymbolTable from openjd.sessions._runner_base import apply_script_let_bindings -apply_script_let_bindings(symtab=SymbolTable(), let_bindings=["malformed"]) +symtab = SymbolTable() +apply_script_let_bindings(symtab=symtab, let_bindings=["mine = 1 + 1"]) +assert symtab["mine"].item() == 2, symtab["mine"] print(RS in sys.modules) """ -def test_applying_a_scripts_let_list_stays_pure(tmp_path: Path) -> None: - """Evaluating a script's ``let`` must not itself load the native extension.""" +def test_applying_an_empty_let_list_stays_pure(tmp_path: Path) -> None: + """The no-``let`` script, which is every non-EXPR script: reaching + ``apply_script_let_bindings`` must not load the native extension. + + This is the production-reachable purity claim on this path. Nothing here + should import openjd.expr -- not the module-level imports in + ``_runner_base``, and not the binding-length guard ahead of the evaluator. + """ # WHEN - loaded = _run_probe(tmp_path, _LET_PROBE) + loaded = _run_probe(tmp_path, _NO_BINDINGS_PROBE) # THEN assert loaded == "False", ( - "applying a script's let list loaded the native extension. Nothing on " - "this path should import openjd.expr: the bindings are evaluated by " - "openjd-model, which skips a malformed binding without parsing it." + "applying an empty let list loaded the native extension, so something " + "on the call path imports openjd.expr unguarded" + ) + + +def test_evaluating_a_scripts_let_does_load_the_extension(tmp_path: Path) -> None: + """Negative control, and a documented limitation rather than a goal. + + A script's own ``let`` is evaluated by the EXPR engine, and the engine *is* + the native extension, so any real binding loads it. The probe binds + ``mine = 1 + 1`` -- valid, and deliberately not path-valued, so the load + cannot be blamed on path handling -- and asserts the bound value, which is + what proves the evaluation actually ran rather than being skipped. + + openjd-sessions cannot close this alone, and should not: a session that + evaluates an expression needs the evaluator. It is bounded instead, by + ``test_applying_an_empty_let_list_stays_pure`` above and by + ``TestSessionLifecycleStaysExtensionFree``, which together pin that only a + template that actually uses EXPR pays for it. + + Asserted so that the boundary is visible and so a future change that moves + it -- in either direction -- is noticed here rather than passing silently. + """ + # WHEN + loaded = _run_probe(tmp_path, _LET_PROBE) + + # THEN + assert loaded == "True", ( + "evaluating a script's let binding no longer loads the native " + "extension. If openjd-model has gained a pure-Python evaluator this " + "control should become a purity assertion; if the binding is being " + "silently skipped instead, the value assertion in the probe is what " + "will have failed first." ) From b93b33846ad79c2e99056e37a57553eb4c0e7f4f Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 08:38:55 -0700 Subject: [PATCH 10/11] refactor: Collapse the duplicate let-binding helper and correct two docstrings apply_script_let_bindings was a pure pass-through to apply_let_bindings, both public in __all__ with identical runtime behaviour. apply_let_bindings was already public on mainline and this branch added the wrapper, so delete the wrapper and keep the scope documentation -- the valuable part -- on apply_let_bindings. Repoints the four production call sites (_session.py x2, _runner_base.py x2) plus the test and comment references. Also corrects two claims in test_let_binding_scopes.py that measurement did not support: - The module docstring said TestSeededStepValuesAreNotReEvaluated "is what fails if anyone reintroduces session-side re-evaluation". It does not: it builds the script's `let` list itself, so it cannot observe a re-merge. What it does pin is host-format deserialization of resolved_symtab -- forcing _session.py's host_format to POSIX fails it. The re-merge half is pinned model-side by TestStepLetIsNotMergedIntoScript, whose six cases all fail against the pre-fix _model.py. The docstring now says both. - _windows_host's mock_patch of openjd.sessions._session.os.name is process-wide, not module-scoped, because _session.py does `import os` (openjd.sessions._session.os is os). It is inert today -- os.name is read exactly once in _session.py, at the intended seam -- and a module-scoped patch is unavailable without changing that import, so the global scope is now noted for whoever next adds a call inside the block. No behaviour change. Suite unchanged at 1000 passed, 40 skipped, 16 xfailed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_runner_base.py | 56 +++++++------------ src/openjd/sessions/_runner_step_script.py | 2 +- src/openjd/sessions/_session.py | 6 +- .../sessions_v0/test_let_binding_scopes.py | 35 +++++++++--- test/openjd/test_import_purity.py | 12 ++-- 5 files changed, 56 insertions(+), 55 deletions(-) diff --git a/src/openjd/sessions/_runner_base.py b/src/openjd/sessions/_runner_base.py index 0946a80d..21921f82 100644 --- a/src/openjd/sessions/_runner_base.py +++ b/src/openjd/sessions/_runner_base.py @@ -39,7 +39,6 @@ "NotifyCancelMethod", "ScriptRunnerBase", "apply_let_bindings", - "apply_script_let_bindings", "resolve_action_arg_values", "resolve_effective_cancelation", "resolve_optional_int_field", @@ -511,12 +510,24 @@ def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: ``Env.File.*``/``Task.File.*`` and a file's ``data`` may reference let-bound values (mirroring openjd-rs's runner ordering). - PATH-typed results render in the engine's default format, which is the - host's. That is the only format a session ever evaluates in: a step's - template-scope ``let`` is resolved once at job creation and its values reach - the session already resolved, through ``Step.resolved_symtab`` (see - :meth:`Session._resolved_base_entries`), so nothing here re-evaluates a - binding that belongs to another scope. + Every binding in ``let_bindings`` is session scope, so there is one scope + here and one format: PATH-typed results render in the engine's default + format, which is the host's. A step's *template*-scope ``let`` does not + appear in this list — openjd-model resolves it once at job creation and its + values travel to the session in the step symbol table, reaching ``symtab`` + through ``Step.resolved_symtab`` + (:meth:`Session._resolved_base_entries`) already resolved and deserialized + into the host's format. + + That division matters because the two are not interchangeable. A + template-scope value is frozen at creation with ``PathFormat::Posix`` so it + cannot depend on the host that created the job, and re-deriving one here + would re-render its PATH values — on Windows + ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` to ``false``. + Both a seeded value and a re-evaluated one would land in this same table, so + a re-evaluation would also *win*, overwriting the correctly-formatted seeded + value. Nothing in a session re-evaluates a step's bindings; it reads the + resolved ones. Raises: ValueError (FormatStringError/ExpressionError): if a binding's @@ -542,33 +553,6 @@ def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings) -def apply_script_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None: - """Evaluate a script's own ``let`` list into ``symtab`` in the host's path - format. - - Every binding in ``let_bindings`` is session scope, so there is one scope - here and one format. A step's *template*-scope ``let`` does not appear in - this list: openjd-model resolves it once at job creation and its values - travel to the session in the step symbol table, reaching ``symtab`` through - ``Step.resolved_symtab`` (:meth:`Session._resolved_base_entries`) already - resolved and deserialized into the host's format. - - That division matters because the two are not interchangeable. A - template-scope value is frozen at creation with ``PathFormat::Posix`` so it - cannot depend on the host that created the job, and re-deriving one here - would re-render its PATH values -- on Windows - ``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` to ``false``. - Both a seeded value and a re-evaluated one would land in this same table, so - a re-evaluation would also *win*, overwriting the correctly-formatted seeded - value. Nothing in a session re-evaluates a step's bindings; it reads the - resolved ones. - - Raises: - ValueError: as :func:`apply_let_bindings`. - """ - apply_let_bindings(symtab=symtab, let_bindings=let_bindings) - - class ScriptRunnerBase(ABC): """Base class for a runnable Environment or Step Script. Responsible for running a *single* Action, and optionally canceling it. @@ -1099,7 +1083,7 @@ def _materialize_files( else: records = file_writer.allocate_file_paths(files, symtab) if let_bindings: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_let_bindings(symtab=symtab, let_bindings=let_bindings) file_writer.write_file_contents(records, symtab) except (RuntimeError, ValueError) as exc: # Had a problem writing at least one file to disk, or evaluating @@ -1112,7 +1096,7 @@ def _apply_let_bindings_or_fail(self, symtab: SymbolTable, let_bindings: list[st evaluation error the action is failed through the normal failure path (openjd_fail log, FAILED state, callback). Returns True on success.""" try: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_let_bindings(symtab=symtab, let_bindings=let_bindings) except ValueError as exc: self._fail_action(str(exc)) return False diff --git a/src/openjd/sessions/_runner_step_script.py b/src/openjd/sessions/_runner_step_script.py index 3c683bbd..ca179a5b 100644 --- a/src/openjd/sessions/_runner_step_script.py +++ b/src/openjd/sessions/_runner_step_script.py @@ -105,7 +105,7 @@ def run(self) -> None: # # This `let` list is the script's own, and is entirely session scope. A # step's template-scope `let` is resolved at job creation and arrives - # through `Step.resolved_symtab` instead; see apply_script_let_bindings. + # through `Step.resolved_symtab` instead; see apply_let_bindings. if self._script.embeddedFiles is not None: symtab = SymbolTable(source=self._symtab) self._materialize_files( diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index f994a270..b7efa6d1 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -44,7 +44,7 @@ from ._path_mapping import PathMappingRule from ._runner_base import ( ScriptRunnerBase, - apply_script_let_bindings, + apply_let_bindings, resolve_action_arg_values, resolve_effective_cancelation, resolve_optional_int_field, @@ -2032,10 +2032,10 @@ def _build_wrapped_inner_scope( ) records = file_writer.allocate_file_paths(embedded_files, symtab) if let_bindings: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_let_bindings(symtab=symtab, let_bindings=let_bindings) file_writer.write_file_contents(records, symtab) elif let_bindings: - apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings) + apply_let_bindings(symtab=symtab, let_bindings=let_bindings) return symtab def _try_inject_wrapped_symbols( diff --git a/test/openjd/sessions_v0/test_let_binding_scopes.py b/test/openjd/sessions_v0/test_let_binding_scopes.py index af8dcf2b..c68fb811 100644 --- a/test/openjd/sessions_v0/test_let_binding_scopes.py +++ b/test/openjd/sessions_v0/test_let_binding_scopes.py @@ -13,10 +13,18 @@ The two must not be confused, and the failure mode is asymmetric. Both a seeded value and a session-time re-evaluation land in the *same* symbol table, so when both happen the re-evaluation writes **last** and clobbers the correctly -formatted seeded value. That overwrite is the bug these tests exist to prevent: -:class:`TestSeededStepValuesAreNotReEvaluated` pins the seeded value surviving, -and it is what fails if anyone reintroduces session-side re-evaluation of a -step's bindings. +formatted seeded value. That overwrite is the bug these tests exist to prevent. + +What :class:`TestSeededStepValuesAreNotReEvaluated` pins, measured rather than +assumed, is the *host-format deserialization* of ``resolved_symtab``: forcing +:mod:`openjd.sessions._session`'s ``host_format`` to POSIX fails it. It does not +by itself fail if the model starts re-merging a step's bindings into the script, +because it builds the script's ``let`` list itself rather than getting one from +job creation. That other half is pinned model-side, by +``TestStepLetIsNotMergedIntoScript`` in +``test/openjd/model_v0/v2023_09/test_let_bindings.py``, whose six cases all fail +against the pre-fix ``_model.py``. Together the two cover the clobber; neither +covers it alone. On simulating a Windows host. A POSIX host renders both scopes identically, so a value comparison here proves nothing about format on this machine -- it would @@ -54,7 +62,7 @@ StepScript as StepScript_2023_09, ) from openjd.sessions import Session -from openjd.sessions._runner_base import apply_script_let_bindings +from openjd.sessions._runner_base import apply_let_bindings # --------------------------------------------------------------------------- # Helpers @@ -78,6 +86,15 @@ def _windows_host() -> Generator[None, None, None]: """Force a Windows path format at both seams that decide one. See this module's docstring for why one seam is not enough. + + Note the scope of the ``os.name`` patch: ``_session.py`` does ``import os``, + so ``openjd.sessions._session.os`` *is* the ``os`` module and patching the + attribute is **process-wide**, not module-scoped. It is inert today because + ``os.name`` is read exactly once in ``_session.py``, at the seam this is + aiming at, and nothing else runs inside the block. A module-scoped patch is + not available without changing that import, so if you add a call inside this + context manager, check first that it does not read ``os.name`` for an + unrelated reason. """ original = ExprNode._evaluate_raw @@ -181,7 +198,7 @@ def test_a_seeded_path_binding_survives_a_scripts_own_let(self) -> None: # WHEN with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) + apply_let_bindings(symtab=symtab, let_bindings=script.let or []) # THEN: the seeded value is untouched, in the host's format. assert str(symtab[_SEEDED_NAME]) == _SEEDED_WINDOWS_TEXT, ( @@ -208,7 +225,7 @@ def test_a_step_level_binding_is_not_evaluated_at_session_time(self) -> None: # WHEN with _spy_on_evaluation() as spy: - apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) + apply_let_bindings(symtab=symtab, let_bindings=script.let or []) # THEN: the evaluator saw the script's own bindings and nothing else. evaluated = _evaluated_bindings(spy) @@ -240,7 +257,7 @@ def test_it_evaluates_in_the_host_format_and_sees_session_symbols(self) -> None: symtab = _session_symtab(session) # WHEN - apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) + apply_let_bindings(symtab=symtab, let_bindings=script.let or []) # THEN: the path rendered in the *host's* format, not POSIX. assert str(symtab["built"]) == r"\a\b", ( @@ -271,4 +288,4 @@ def test_a_failing_binding_still_raises(self) -> None: # WHEN / THEN with pytest.raises(ValueError, match="bad"): - apply_script_let_bindings(symtab=symtab, let_bindings=script.let or []) + apply_let_bindings(symtab=symtab, let_bindings=script.let or []) diff --git a/test/openjd/test_import_purity.py b/test/openjd/test_import_purity.py index af987d95..f0b47167 100644 --- a/test/openjd/test_import_purity.py +++ b/test/openjd/test_import_purity.py @@ -410,7 +410,7 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: # --------------------------------------------------------------------------- -# `apply_script_let_bindings` is on the session path for every script, EXPR or +# `apply_let_bindings` is on the session path for every script, EXPR or # not, so reaching it must not by itself load the native extension. Actually # *evaluating* a binding must, because the expression engine is the extension -- # the two cases are split below so each says which it is. @@ -419,21 +419,21 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: _NO_BINDINGS_PROBE = """ from openjd.model import SymbolTable -from openjd.sessions._runner_base import apply_script_let_bindings +from openjd.sessions._runner_base import apply_let_bindings -apply_script_let_bindings(symtab=SymbolTable(), let_bindings=[]) +apply_let_bindings(symtab=SymbolTable(), let_bindings=[]) print(RS in sys.modules) """ _LET_PROBE = """ from openjd.model import SymbolTable -from openjd.sessions._runner_base import apply_script_let_bindings +from openjd.sessions._runner_base import apply_let_bindings symtab = SymbolTable() -apply_script_let_bindings(symtab=symtab, let_bindings=["mine = 1 + 1"]) +apply_let_bindings(symtab=symtab, let_bindings=["mine = 1 + 1"]) assert symtab["mine"].item() == 2, symtab["mine"] print(RS in sys.modules) """ @@ -441,7 +441,7 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: def test_applying_an_empty_let_list_stays_pure(tmp_path: Path) -> None: """The no-``let`` script, which is every non-EXPR script: reaching - ``apply_script_let_bindings`` must not load the native extension. + ``apply_let_bindings`` must not load the native extension. This is the production-reachable purity claim on this path. Nothing here should import openjd.expr -- not the module-level imports in From ded3168eae635d8aa26bba8a829423922e313cfd Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 09:12:33 -0700 Subject: [PATCH 11/11] docs: Correct the empty-let purity test to a dependency-boundary claim The section comment and docstring said reaching apply_let_bindings with an empty let list was "the session path for every script, EXPR or not" and a "production-reachable purity claim". Neither holds: every call site guards on truthiness first (_session.py, _runner_base._materialize_files, and both callers of _apply_let_bindings_or_fail), so let_bindings=[] is a test-only shape. The docstring's clause about the binding-length guard was also vacuous, since that loop body never runs on an empty list. Reword to what the test does pin: openjd-model's evaluate_let_bindings staying pure on an empty list, as a dependency-boundary control. Point at the two tests that already cover production purity for a non-EXPR script. Text only, no behaviour change. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/test_import_purity.py | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/test/openjd/test_import_purity.py b/test/openjd/test_import_purity.py index f0b47167..b0275f47 100644 --- a/test/openjd/test_import_purity.py +++ b/test/openjd/test_import_purity.py @@ -410,10 +410,10 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: # --------------------------------------------------------------------------- -# `apply_let_bindings` is on the session path for every script, EXPR or -# not, so reaching it must not by itself load the native extension. Actually -# *evaluating* a binding must, because the expression engine is the extension -- -# the two cases are split below so each says which it is. +# `apply_let_bindings` delegates to openjd-model's `evaluate_let_bindings`. +# The two probes below pin that boundary from both sides: an empty list must +# not load the native extension, and a real binding must, because the +# expression engine *is* the extension. # --------------------------------------------------------------------------- @@ -440,12 +440,20 @@ def test_path_mapping_rules_do_load_the_extension(self, tmp_path: Path) -> None: def test_applying_an_empty_let_list_stays_pure(tmp_path: Path) -> None: - """The no-``let`` script, which is every non-EXPR script: reaching - ``apply_let_bindings`` must not load the native extension. - - This is the production-reachable purity claim on this path. Nothing here - should import openjd.expr -- not the module-level imports in - ``_runner_base``, and not the binding-length guard ahead of the evaluator. + """Dependency-boundary control: openjd-model's ``evaluate_let_bindings`` + must stay pure when handed an empty list. + + ``let_bindings=[]`` is a test-only shape rather than a production path -- + every call site guards on truthiness first (``_session.py``, + ``_runner_base._materialize_files``, and both callers of + ``_apply_let_bindings_or_fail``) -- so what this pins is the boundary, not + session behaviour. Production purity for a non-EXPR script is covered by + ``test_running_a_non_expr_task_stays_pure_end_to_end`` and + ``test_importing_sessions_does_not_load_native_extension``. + + Nothing on this call path should import openjd.expr: not the module-level + imports in ``_runner_base``, and not openjd-model's evaluator entry point + ahead of any actual evaluation. """ # WHEN loaded = _run_probe(tmp_path, _NO_BINDINGS_PROBE)