diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cdd109a..0dc8a2f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +## Unreleased + +### BREAKING CHANGES +* Step-level `let` bindings are evaluated once in template scope and are no longer merged into the step's script, so the `Job` + returned by `create_job` no longer carries their values. A caller that runs a job whose steps declare a template-scope `let` + referenced from the step's script must switch to `create_job_with_symbol_tables` and forward the step's `step_symbol_tables` + entry to the session; otherwise the action fails at run time with `Undefined variable`. Callers that only inspect the `Job` + at creation time — `StepDependencyGraph`, `StepParameterSpaceIterator`, `hostRequirements` — are unaffected. + +### Bug Fixes +* Evaluate step-level let bindings in template scope +* Stop merging step-level let into the script + + ## 0.11.6 (2026-08-25) diff --git a/README.md b/README.md index 8a76dba4..c0aa4a32 100644 --- a/README.md +++ b/README.md @@ -211,6 +211,15 @@ except (DecodeValidationError, RuntimeError) as e: print(str(e)) ``` +If any of the job's steps declares a template-scope `let` that the step's script +references, then this `Job` is not sufficient to run the step: step-level `let` +bindings are evaluated once during instantiation and kept in the step's symbol +table rather than lowered onto the script, so a session created from the `Job` +alone has no binding for the name and the action fails with `Undefined variable`. +Use `create_job_with_symbol_tables` instead and forward the step's entry from the +returned `step_symbol_tables` to the session that runs it. The two examples below +only inspect the `Job` at creation time, so plain `create_job` is correct there. + ### Working with Step dependencies ```python diff --git a/THIRD-PARTY-LICENSES.txt b/THIRD-PARTY-LICENSES.txt index a89a1057..f749ff3b 100644 --- a/THIRD-PARTY-LICENSES.txt +++ b/THIRD-PARTY-LICENSES.txt @@ -24,7 +24,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. ------ -** pydantic; version 2.13.4 -- https://pypi.org/project/pydantic/ +** pydantic; version 2.13.5 -- https://pypi.org/project/pydantic/ The MIT License (MIT) Copyright (c) 2017 to present Pydantic Services Inc. and individual contributors. @@ -48,7 +48,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. ------ -** pydantic_core; version 2.46.4 -- https://pypi.org/project/pydantic_core/ +** pydantic_core; version 2.46.5 -- https://pypi.org/project/pydantic_core/ The MIT License (MIT) Copyright (c) 2022 Samuel Colvin diff --git a/src/openjd/model/_create_job.py b/src/openjd/model/_create_job.py index d85f6b04..ede9d53d 100644 --- a/src/openjd/model/_create_job.py +++ b/src/openjd/model/_create_job.py @@ -529,15 +529,27 @@ def create_job( ) -> Job: """Create a job from a Job Template and a set of Job Parameter values. - See :func:`create_job_with_symbol_tables` when you also need the resolved - symbol tables — for instance to transport them to a host that will run the - job's sessions. + The returned ``Job`` does not carry the evaluated step-level ``let`` values. + Those bindings are template-scope: they are evaluated once here, in template + scope, and kept in the step-scope symbol table rather than lowered onto the + step's script. So for a template that declares a step-level ``let`` and + references it from the step's script, this ``Job`` alone is not enough to run + the step — the session has no binding for the name and the action fails with + ``Undefined variable``. + + A caller that intends to *run* such a job must use + :func:`create_job_with_symbol_tables` instead, and forward the returned + ``step_symbol_tables`` entry for the step to the session that runs it. Callers + that only inspect the ``Job`` at creation time — a ``StepDependencyGraph``, a + ``StepParameterSpaceIterator``, ``hostRequirements`` — are unaffected, because + those fields are resolved during instantiation and already hold their values. Raises: DecodeValidationError Returns: - Job: The job generated. + Job: The job generated. Self-contained only if no step declares a + template-scope ``let`` that its script references. """ job, _symtab = _create_job_and_symbol_table( job_template=job_template, diff --git a/src/openjd/model/_internal/_create_job.py b/src/openjd/model/_internal/_create_job.py index 23c823b9..3aafa011 100644 --- a/src/openjd/model/_internal/_create_job.py +++ b/src/openjd/model/_internal/_create_job.py @@ -172,10 +172,21 @@ def instantiate_model( # noqa: C901 # Extend the symbol table for this model's subtree if defined (e.g. a # step's Step.Name and step-level EXPR `let` bindings). This runs before - # the transform: StepTemplate's syntax-sugar transform folds step-level - # `let` bindings into the script (their runtime channel), so the original - # model is the one that still carries them for create_job-time fields - # (parameter space, host requirements). + # the transform, so the hook always sees the model as authored, and that + # ordering is load-bearing for two reasons. + # + # A transform may rebuild the model rather than adjust it -- StepTemplate's + # syntax-sugar transform returns a `model_construct`ed copy -- so the fields + # the hook reads (a step's `name` and its `let`) are only guaranteed to be + # the authored ones on this side of it. The current transform carries `let` + # through deliberately; running the hook first is what keeps that the + # transform's choice rather than a requirement on every future one. + # + # And create_job_with_symbol_tables invokes the same hook on the same + # untransformed StepTemplate to build the step symbol table it publishes for + # the runtime to seed a session with. That table is only the scope the + # step's own fields (script, parameter space, host requirements) were + # instantiated against if both callers hand the hook the same model. if model._job_creation_metadata.extends_symtab is not None: symtab = model._job_creation_metadata.extends_symtab(model, symtab) diff --git a/src/openjd/model/_let_bindings.py b/src/openjd/model/_let_bindings.py index 651880b0..35e014c4 100644 --- a/src/openjd/model/_let_bindings.py +++ b/src/openjd/model/_let_bindings.py @@ -43,7 +43,9 @@ def _parse_rhs(rhs: str) -> Any: return ExprNode(rhs) -def evaluate_let_bindings(*, symtab: SymbolTable, let_bindings: Iterable[str]) -> None: +def evaluate_let_bindings( + *, symtab: SymbolTable, let_bindings: Iterable[str], path_format: Any = None +) -> None: """Evaluate EXPR ``let`` bindings in order, seeding each into ``symtab``. ``let_bindings`` is an ordered list of ``"name = expression"`` strings. @@ -53,6 +55,13 @@ def evaluate_let_bindings(*, symtab: SymbolTable, let_bindings: Iterable[str]) - access, and float rendering fidelity is preserved — matching the Rust runtime's natively typed symbol table. + ``path_format`` is the EXPR ``PathFormat`` that PATH-typed values render + with. Callers evaluating in *template* scope pass ``PathFormat.POSIX``, + matching openjd-rs, whose job instantiation hardcodes POSIX so a create-time + result does not depend on the host that created the job. ``None`` (the + default) leaves the engine's default — the host's format — which is what + session-scope callers want. + Malformed bindings (missing ``=``, empty name or expression) are skipped: the ``let`` field validator rejects them at decode time, so evaluation is defensive here. @@ -76,6 +85,6 @@ def evaluate_let_bindings(*, symtab: SymbolTable, let_bindings: Iterable[str]) - # evaluate_value keeps the engine's typed value (paths stay # paths, float rendering fidelity is preserved) when the binding # is later referenced. - symtab[name] = _parse_rhs(rhs).evaluate_value(symtab=symtab) + symtab[name] = _parse_rhs(rhs).evaluate_value(symtab=symtab, path_format=path_format) except ValueError as exc: raise ValueError(f"let binding {name!r}: {exc}") diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index 763fb053..229caef4 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -3388,8 +3388,9 @@ class Step(OpenJDModel_v2023_09): # RFC 0007 (EXPR): the step-level `let` bindings, preserved from the # StepTemplate so the runtime can seed them when entering the step's # environments — a step environment's variables and actions may reference - # them. The step's own script carries a merged copy (step bindings first) - # for the task-run path. + # them. Their *values* are already resolved at job creation and travel in + # the step's symbol table (see create_job_with_symbol_tables), so they are + # not merged into the script's own `let` for the runtime to re-evaluate. let: Optional[list[str]] = None @@ -3447,6 +3448,15 @@ def _extend_step_symtab(self: Any, symtab: SymbolTable) -> SymbolTable: them. Script-level ``let`` bindings are *not* evaluated here — they resolve at session time. + Template scope renders PATH-typed values with ``PathFormat.POSIX``, + matching openjd-rs, whose job instantiation hardcodes POSIX + (``create_job/instantiate.rs``) and uses the host's format only inside + sessions. Without it a binding's create-time value would depend on the + host that created the job: on Windows ``startswith(path("/foo/bar"), + "/foo")`` is false against a backslash rendering but true against a + POSIX one, so the job would behave differently depending on where it + was created. + ``Step.Name`` and ``let`` references only pass template validation with the EXPR extension enabled, so seeding them unconditionally does not change the behavior of non-EXPR templates. @@ -3454,9 +3464,16 @@ def _extend_step_symtab(self: Any, symtab: SymbolTable) -> SymbolTable: step_symtab = SymbolTable(source=symtab) step_symtab["Step.Name"] = str(self.name) if self.let: + # Both imports are deferred: `openjd.expr` is the native extension, + # and importing openjd.model must not load it. Only an EXPR template + # reaches this branch, so the load is conditional on EXPR use. + from openjd.expr import PathFormat + from .._let_bindings import evaluate_let_bindings - evaluate_let_bindings(symtab=step_symtab, let_bindings=self.let) + evaluate_let_bindings( + symtab=step_symtab, let_bindings=self.let, path_format=PathFormat.POSIX + ) return step_symtab _template_variable_sources = { @@ -3573,17 +3590,13 @@ def resolve_syntax_sugar(self) -> "StepTemplate": StepTemplate: A new StepTemplate with de-sugared script, or self if no sugar. """ if self.script: - # Step-level `let` (RFC 0007) is excluded from the instantiated Step - # by the job-creation metadata, so fold it into the script's own - # `let` (step bindings first, then the script's) so it survives into - # the Job and the runtime resolves it. The model has already - # validated reference/shadowing rules across both scopes at decode. - if self.let: - # The step's own `let` is preserved too (Step.let): the - # runtime seeds it when entering the step's environments. - merged_let = [*self.let, *(self.script.let or [])] - new_script = self.script.model_copy(update={"let": merged_let}) - return self.model_copy(update={"script": new_script}) + # The step-level `let` (RFC 0007) is *not* folded into the script's + # own `let`. It is evaluated in template scope at job creation and + # its resolved values travel in the step's symbol table + # (create_job_with_symbol_tables().step_symbol_tables[name]), which + # the runtime seeds the session with. Merging it here would have the + # session re-evaluate those bindings in host scope, re-rendering + # PATH values and overwriting the correctly formatted seeded value. return self for name, (command, ext, arg_prefix) in _INTERPRETER_MAP.items(): @@ -3608,32 +3621,33 @@ def resolve_syntax_sugar(self) -> "StepTemplate": args.extend(simple_action.args) # Construct directly - inputs are already validated + new_script = StepScript.model_construct( + actions=StepActions.model_construct( + onRun=Action.model_construct( + command=CommandString(command), + args=args, + timeout=simple_action.timeout, + cancelation=simple_action.cancelation, + ) + ), + # Only the SimpleAction's own `let` (RFC 0007) — the step-level one + # is resolved at job creation and travels in the step's symbol + # table, as in the `script:` branch above. + let=simple_action.let, + embeddedFiles=[ + EmbeddedFileText.model_construct( + name=embedded_name, + type=EmbeddedFileTypes.TEXT, + filename=f"{embedded_name}{ext}", + runnable=True, + data=simple_action.script, + ) + ], + ) return StepTemplate.model_construct( name=self.name, description=self.description, - script=StepScript.model_construct( - actions=StepActions.model_construct( - onRun=Action.model_construct( - command=CommandString(command), - args=args, - timeout=simple_action.timeout, - cancelation=simple_action.cancelation, - ) - ), - # Carry step-level `let` (RFC 0007) and the SimpleAction's own - # `let` onto the de-sugared script (step bindings first) so they - # are preserved into the Job and resolved at runtime. - let=([*(self.let or []), *(simple_action.let or [])] or None), - embeddedFiles=[ - EmbeddedFileText.model_construct( - name=embedded_name, - type=EmbeddedFileTypes.TEXT, - filename=f"{embedded_name}{ext}", - runnable=True, - data=simple_action.script, - ) - ], - ), + script=new_script, stepEnvironments=self.stepEnvironments, parameterSpace=self.parameterSpace, hostRequirements=self.hostRequirements, diff --git a/test/openjd/model_v0/test_let_bindings.py b/test/openjd/model_v0/test_let_bindings.py index fe21d020..0d9ec1cf 100644 --- a/test/openjd/model_v0/test_let_bindings.py +++ b/test/openjd/model_v0/test_let_bindings.py @@ -151,3 +151,62 @@ def test_parse_errors_are_not_cached(self) -> None: info = _parse_rhs.cache_info() assert info.hits == 0 assert info.misses == 2 + + +class TestPathFormat: + """``path_format`` selects the rendering PATH-typed values coerce to. + + Template-scope callers (job instantiation) pass ``PathFormat.POSIX`` so a + binding's create-time value does not depend on the host that created the + job; session-scope callers leave it unset and get the host's format. + """ + + # A binding whose result differs per format: `join` coerces each path to a + # string, so the separator the engine renders is visible in the result. + BINDING = 'x = [path("/a"), path("/b")].join(",")' + + def test_posix_renders_forward_slashes(self) -> None: + # GIVEN + from openjd.expr import PathFormat + + symtab = SymbolTable() + + # WHEN + evaluate_let_bindings( + symtab=symtab, let_bindings=[self.BINDING], path_format=PathFormat.POSIX + ) + + # THEN + assert str(symtab["x"]) == "/a,/b" + + def test_windows_renders_backslashes(self) -> None: + # The counterpart to the POSIX case: together they prove the parameter + # reaches the engine on any host, rather than the host default + # happening to match one of them. + # GIVEN + from openjd.expr import PathFormat + + symtab = SymbolTable() + + # WHEN + evaluate_let_bindings( + symtab=symtab, let_bindings=[self.BINDING], path_format=PathFormat.WINDOWS + ) + + # THEN + assert str(symtab["x"]) == "\\a,\\b" + + def test_default_is_the_engine_default(self) -> None: + # Omitting path_format preserves the pre-existing behaviour: the engine + # renders in the host's format. + # GIVEN + import os + + symtab = SymbolTable() + + # WHEN + evaluate_let_bindings(symtab=symtab, let_bindings=[self.BINDING]) + + # THEN + expected = "\\a,\\b" if os.name == "nt" else "/a,/b" + assert str(symtab["x"]) == expected diff --git a/test/openjd/model_v0/v2023_09/test_let_bindings.py b/test/openjd/model_v0/v2023_09/test_let_bindings.py index ac0ddc6a..f407ede4 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -6,7 +6,13 @@ import pytest -from openjd.model import DecodeValidationError, decode_job_template +from openjd.expr import PathFormat +from openjd.model import ( + DecodeValidationError, + SymbolTable, + create_job_with_symbol_tables, + decode_job_template, +) _EXTS = ["EXPR", "FEATURE_BUNDLE_1"] @@ -177,3 +183,147 @@ def test_string_type_mismatch_rejected(self): params=[{"name": "Name", "type": "STRING", "default": "hi"}], ) ) + + +class TestStepScopeIsTemplateScope: + """Step-level `let` bindings resolve in *template* scope at job creation, so + PATH-typed values render POSIX regardless of the host that creates the job — + matching openjd-rs, whose instantiation hardcodes PathFormat::Posix and uses + the host's format only inside sessions.""" + + def test_step_symtab_renders_paths_posix(self): + # GIVEN + template = _decode( + _job( + [ + { + "name": "S", + "let": ['p = string(path("/mnt/out"))'], + "script": _onrun("{{p}}"), + } + ] + ) + ) + + # WHEN + symtab = template.steps[0]._extend_step_symtab(SymbolTable()) + + # THEN + assert str(symtab["p"]) == "/mnt/out" + + def test_step_symtab_path_predicate_is_host_independent(self): + # The conformance failure this guards: on Windows a host-format + # rendering makes a POSIX-prefix test false at create time. + # GIVEN + template = _decode( + _job( + [ + { + "name": "S", + "let": ['under = startswith(path("/foo/bar"), "/foo")'], + "script": _onrun("{{under}}"), + } + ] + ) + ) + + # WHEN + symtab = template.steps[0]._extend_step_symtab(SymbolTable()) + + # THEN + assert str(symtab["under"]) == "true" + + +class TestStepLetIsNotMergedIntoScript: + """A step-level `let` is resolved once, in template scope, at job creation, + and its values travel in the step's symbol table. It is therefore *not* + merged into the script's own `let`: doing so would have the session + re-evaluate the same bindings in the host's scope, re-rendering PATH values + and overwriting the correctly formatted seeded value.""" + + @staticmethod + def _resolved_script(step): + template = _decode(_job([step], extensions=("EXPR", "FEATURE_BUNDLE_1"))) + return template.steps[0].resolve_syntax_sugar().script + + def test_script_branch_keeps_only_the_scripts_own_let(self): + # GIVEN / WHEN + script = self._resolved_script( + { + "name": "S", + "let": ["a = 1", "b = 2"], + "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, + } + ) + + # THEN + assert script.let == ["c = 3"] + + @pytest.mark.parametrize("interpreter", ("python", "bash", "cmd", "powershell", "node")) + def test_simple_action_branch_keeps_only_the_actions_own_let(self, interpreter): + # The de-sugaring path builds a fresh script, so it has to make the same + # choice independently of the `script:` branch. + # GIVEN / WHEN + script = self._resolved_script( + { + "name": "S", + "let": ["a = 1", "b = 2"], + interpreter: {"script": "print(1)", "let": ["c = 3"]}, + } + ) + + # THEN + assert script.let == ["c = 3"] + + +class TestStepLetTravelsInTheStepSymbolTable: + """The transport that replaces the merge: `create_job_with_symbol_tables` + resolves the step-level `let` at creation and hands the values over in the + step's symbol table, PATH values stored so each host renders them itself.""" + + @staticmethod + def _tables(step): + template = _decode(_job([step])) + return create_job_with_symbol_tables(job_template=template, job_parameter_values={}) + + _STEP = { + "name": "S", + "let": ["a = 1", 'root = path("/foo/bar")', 'txt = string(path("/mnt/out"))'], + "script": {"let": ["scriptonly = 7"], **_onrun("{{a}}{{scriptonly}}")}, + } + + def test_step_let_values_are_carried_and_paths_render_per_host(self): + # GIVEN + result = self._tables(self._STEP) + table = result.step_symbol_tables["S"] + + # WHEN + posix = table.to_symtab(path_format=PathFormat.POSIX) + windows = table.to_symtab(path_format=PathFormat.WINDOWS) + + # THEN: the values the deleted merge used to have the session recompute + # are already here, and the PATH one is stored so it renders in each + # host's own format rather than the creating host's. + assert str(posix["a"]) == "1" + assert str(posix["root"]) == "/foo/bar" + assert str(windows["root"]) == "\\foo\\bar" + # Rendered to a string at create time, in template scope, so it stays + # POSIX on every host -- this is what re-evaluating in host scope broke. + assert str(posix["txt"]) == "/mnt/out" + assert str(windows["txt"]) == "/mnt/out" + # AND: the script still carries only its own bindings, for the session + # to evaluate in host scope. + assert result.job.steps[0].script.let == ["scriptonly = 7"] + + def test_script_let_is_not_in_the_step_symbol_table(self): + # A script-level binding resolves at session time in host scope, so it + # must not be evaluated into -- or leak into -- the create-time table. + # GIVEN + result = self._tables(self._STEP) + + # WHEN + symtab = result.step_symbol_tables["S"].to_symtab(path_format=PathFormat.POSIX) + + # THEN + assert "scriptonly" not in symtab + assert "a" in symtab